Fix initial numbering of frames in TTML to SRT converter#12671
Fix initial numbering of frames in TTML to SRT converter#12671TobiGr merged 1 commit intoTeamNewPipe:devfrom
Conversation
…e#12670) - The SubRip (.srt) specification requires subtitle numbering to begin from 1. - Please refer to https://en.wikipedia.org/wiki/SubRip - Previously numbering started from 0, which is accepted by most players (tested on mpv, VLC, MPlayer, Totem) but not strictly compliant.
| throws IOException { | ||
| writeString(String.valueOf(frameIndex++)); | ||
| writeString(String.valueOf(frameIndex)); | ||
| frameIndex += 1; |
There was a problem hiding this comment.
I'd prefer to use this because is just a write / increment and no read. Thus there is only little room for confusion.
| frameIndex += 1; | |
| frameIndex++; |
There was a problem hiding this comment.
Thanks for your suggestion.
I agree that frameIndex++ works fine here and avoids confusion.
My earlier comment about i++ / ++i was only about using them directly inside something like writeString(String.valueOf(...)), which can indeed be confusing.
Between frameIndex += 1, frameIndex++, and frameIndex = frameIndex + 1, I personally prefer += 1 because it’s very explicit and leaves no room for overthinking.
I’m fine with switching to frameIndex++ if the team prefers, but my preference would be to keep += 1 if that’s acceptable.
There was a problem hiding this comment.
Let me know if you’d like me to change it.
What is it?
Description of the changes in your PR
This PR fixes the numbering in .srt subtitle files generated from YouTube streams.
Previously, numbering started at 0, which is accepted by most players but does not comply with the SubRip specification. Now numbering starts at 1, ensuring strict compliance.
-1Why not
i++or++i?Every time I see
i++or++i, I need to pause and think:i++) or “increment first, then use” (++i)?To avoid this confusion, I prefer splitting the current line
writeString(String.valueOf(frameIndex++));into two lines: one to use current value offrameIndex, and the other to increment it.Before/After Screenshots/Screen Record
Fixes the following issue(s)
Fixes #12670
APK testing
The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR. You can find more info and a video demonstration on this wiki page.
Due diligence