Conversation
…asticode-master
asticode
left a comment
There was a problem hiding this comment.
Thanks for the PR and sorry for the slow review! ❤️
I've quite a few remarks 👍
webvtt.go
Outdated
| // WriteToWebVTT writes subtitles in .vtt format | ||
| func (s Subtitles) WriteToWebVTT(o io.Writer) (err error) { | ||
| // if set true in second args write index as item index | ||
| func (s Subtitles) WriteToWebVTT(args ...interface{}) (err error) { |
There was a problem hiding this comment.
Can you remove replacing the function signature from this PR?
webvtt.go
Outdated
| } | ||
|
|
||
| // Init writer | ||
| w := bufio.NewWriter(o) |
There was a problem hiding this comment.
Can you avoid using a bufio.Writer here?
webvtt.go
Outdated
| } | ||
|
|
||
| func (l Line) webVTTBytes() (c []byte) { | ||
| func (l Line) writeWebVTT(w io.Writer) (err error) { |
There was a problem hiding this comment.
Can you provide the *astikit.WriteChainer instead ?
webvtt.go
Outdated
| } | ||
|
|
||
| func (li LineItem) webVTTBytes(previous, next *LineItem) (c []byte) { | ||
| func (li LineItem) writeWebVTT(w io.Writer, previous, next *LineItem) (err error) { |
There was a problem hiding this comment.
Can you provide the *astikit.WriteChainer instead ?
| }, | ||
| }}.webVTTBytes())) | ||
| }}.writeWebVTT(w) | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
Can you use require instead of assert?
…title formats and add astikit dependency.
| module github.com/asticode/go-astisub | ||
|
|
||
| go 1.13 | ||
| go 1.24.0 |
There was a problem hiding this comment.
It seems your "revert to go 1.13" change was reverted by one of the last commit. Could you revert it back?
| o = appendStringToBytesWithNewLine(o, "; "+c) | ||
| // write writes the block to the writer | ||
| func (b *ssaScriptInfo) write(c *astikit.WriteChainer) (err error) { | ||
| if _, err = c.Write(astikit.WriteWithLabel("script info header", []byte("[Script Info]"))); err != nil { |
There was a problem hiding this comment.
Can you group/chain the 2 writes here?
| } | ||
| for _, comment := range b.comments { | ||
| if _, err = c.Write( | ||
| astikit.WriteWithLabel("comment start", []byte("; ")), |
There was a problem hiding this comment.
Can you simplify with
if _, err = c.Write(
astikit.WriteWithLabel("comment", append([]byte("; "+comment), bytesLineSeparator...)),
); err != nil {
return
}| } | ||
| if len(b.collisions) > 0 { | ||
| o = appendStringToBytesWithNewLine(o, ssaScriptInfoNameCollisions+": "+b.collisions) | ||
| if _, err = c.Write( |
There was a problem hiding this comment.
Can you simplify all writes below with one big write with conditions:
if _, err = c.Write(
astikit.WriteWithCondition("collisions", append([]byte(ssaScriptInfoNameCollisions+": "b.collisions), bytesLineSeparator...), len(b.collisions) > 0),
astikit.WriteWithCondition("original editing", append([]byte(ssaScriptInfoNameOriginalEditing+": "b.originalEditing), bytesLineSeparator...), len(b.originalEditing) > 0),
[...]
); err != nil {
return
}| if len(b.collisions) > 0 { | ||
| o = appendStringToBytesWithNewLine(o, ssaScriptInfoNameCollisions+": "+b.collisions) | ||
| if _, err = c.Write( | ||
| astikit.WriteWithLabel("collisions label", []byte(ssaScriptInfoNameCollisions+": ")), |
There was a problem hiding this comment.
Also could you delete the appendStringToBytesWithNewLine() function since it's not used anymore?
| } | ||
|
|
||
| // WriteToWebVTT writes subtitles in .vtt format | ||
| // if set true in second args write index as item index |
There was a problem hiding this comment.
Can you remove this comment too?
| } | ||
|
|
||
| // Lines | ||
| lines := r.InlineStyle.WebVTTLines |
There was a problem hiding this comment.
Can you "compute" all variables first (lines, ra, scroll, etc.) and then add one big write with condition writes? Something like
lines := r.InlineStyle.WebVTTLines
[...]
ra := r.InlineStyle.WebVTTRegionAnchor
[...]
if _, err = c.Write(
astikit.WriteWithLabel("region id", []byte("Region: id="+r.ID)),
astikit.WriteWithCondition("lines", append(bytesSpace, []byte("lines="+strconv.Itoa(lines))...)),
[...]
); err != nil {
return
}| } else if item.Style != nil && item.Style.InlineStyle != nil && item.Style.InlineStyle.WebVTTPosition != nil { | ||
| c = append(c, bytesSpace...) | ||
| c = append(c, []byte("position:"+item.Style.InlineStyle.WebVTTPosition.String())...) | ||
| // Align |
There was a problem hiding this comment.
Same as above, could you compute all variables first and make one big Write?
| // Voice name | ||
| if l.VoiceName != "" { | ||
| c = append(c, []byte("<v "+l.VoiceName+">")...) | ||
| if _, err = c.Write( |
There was a problem hiding this comment.
Can you use WriteWithCondition to remove the if instead?
| }, | ||
| }}.webVTTBytes())) | ||
| }}.writeWebVTT(w) | ||
| assert.NoError(t, err) |
Description
This PR refactors the WriteToSRT WriteToWebVTT and WriteToSSA methods to use
io.Writerstreaming (viabufio.Writer) instead of accumulating the entire file content in an in-memory byte slice.Motivation
Previously, methods like
WriteToSRT
built the entire output in a
[]bytebuffer before writing it. For large subtitle files or high-throughput services, this caused excessive memory allocation (O(N) with file size) and GC pressure. This change ensures memory usage remains constant (O(1)) regardless of the subtitle duration/line count, as data is flushed to the writer in chunks.Changes
srtBytes()building with writeSRT(w io.Writer)webVTTBytes()(which used append/copy) with writeWebVTT(w io.Writer)Benchmarks
While there is a negligible overhead for tiny files due to bufio initialization, the memory scalability is significantly improved.
Results (Apple M3 Max):
note: The static
B/opin benchmarks reflects the bufio buffer allocation (default 4KB). In real-world large file scenarios, the previous implementation's allocation would grow linearly, whereas this remains constant.Supersedes
Supersedes #61 (if applicable)