fix(608): anchor pop-on->roll-up transition start time to first character, not CR - #2290
Open
x15sr71 wants to merge 2 commits into
Open
fix(608): anchor pop-on->roll-up transition start time to first character, not CR#2290x15sr71 wants to merge 2 commits into
x15sr71 wants to merge 2 commits into
Conversation
x15sr71
force-pushed
the
fix/608-rollup-transition-start-time
branch
from
August 9, 2026 19:39
072f7df to
3442bcf
Compare
…tion In the pop-on -> roll-up transition with changes==0 (roll-up window not yet full), only set ts_start_of_current_line to the CR time when in transcript mode. SRT leaves it for write_char()'s existing == -1 guard to set on the first character typed, which is what the SRT reference corpus anchors to. Fixes a 133 ms transcript timing regression on RT29/RT30 introduced by the earlier condition flip, while preserving the RT84 fix.
…ipt2 writes nothing 03ad9e8 refactored write_cc_buffer_as_transcript2 to write encoded_end_frame at the end of each caption block. It set wrote_something = 1 unconditionally after calling write_cc_line_as_transcript2, even when that function returned without writing anything (blank cursor row, length = 0). Result: a bare newline (encoded_end_frame) was emitted for every blank cursor row, producing spurious blank lines in transcript output. Fix: change write_cc_line_as_transcript2 from void to int (returns 1 if it wrote, 0 if not) and use wrote_something |= to only set the flag when content was actually emitted. Fixes RT41 blank line regression introduced by PR CCExtractor#2105.
x15sr71
force-pushed
the
fix/608-rollup-transition-start-time
branch
from
August 9, 2026 19:44
3b1ee1e to
1782e17
Compare
Collaborator
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit e7ad5c7...:
Your PR breaks these cases:
NOTE: The following tests have been failing on the master branch as well as the PR:
Congratulations: Merging this PR would fix the following tests:
It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). Check the result page for more info. |
Collaborator
CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results, when compared to test for commit e7ad5c7...:
Your PR breaks these cases:
NOTE: The following tests have been failing on the master branch as well as the PR:
Congratulations: Merging this PR would fix the following tests:
It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). Check the result page for more info. |
cfsmp3
self-requested a review
August 10, 2026 14:19
cfsmp3
requested changes
Aug 10, 2026
cfsmp3
left a comment
cfsmp3
left a comment
Contributor
There was a problem hiding this comment.
One thing that needs fixing
On sample 7236304cfc, SRT output regresses:
master: 1 00:00:05,922 --> 00:00:07,089
2 00:00:07,091 --> 00:00:08,273
this PR: 1 00:00:00,000 --> 00:00:07,089
2 00:00:07,273 --> 00:00:08,273
Two problems. The first cue anchors to 00:00:00,000, so it claims to be on screen from the very start of the file. And the second cue's start moves 182 ms later, which is the opposite
direction from everything else in the corpus and opens a gap where master had a 2 ms join.
This is the only sample of the 25 where either happens, so it looks like an edge case in how ts_start_of_current_line is anchored when the transition happens near the start of the stream
— plausibly it is still 0/unset rather than a real first-character time, and the > 0 guard elsewhere in the file doesn't cover this path.
Please note the regression suite cannot catch this: 7236304cfc is only exercised as --autoprogram --out=ttxt --latin1 (rt5), which passes precisely because transcript output is unchanged
now. So a green run here would not mean it is fixed.
Two smaller points
Please split the transcript fix out. Making write_cc_line_as_transcript2() return whether it wrote, and folding that into wrote_something, is a correct fix and has nothing to do with
pop-on→roll-up timing. On its own it is small, low-risk and I would merge it right away. Bundled here, it is stuck behind the timing discussion.
Please justify the output_format gate in the description. After this change the 608 decoder produces different start times depending on which encoder is selected, so --out=srt and
--out=ttxt disagree on when the same caption begins. That may well be right, given transcript times individual lines rather than buffers, but it is surprising enough that it should be
written down — otherwise it gets reported as a bug later, and whoever looks at it will not know it was deliberate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In raising this pull request, I confirm the following (please check boxes):
Reason for this PR:
Sanity check:
Repro instructions:
This PR fixes Regression Test 84 which is currently failing on both linux and windows. Reference Test Run - Windows, Linux.
Problem
Regression test 84 (sample 79,
f23a544b...,--out=srt --latin1) fails exact SHA-256 comparison against referencee5c9d9ec...Confirmed via CI history (sample-platform DB) failing continuously since test_id 6698 (Dec 2025, shortly after54df50fmerged), including at the v0.96.6 release build (test 8446/8447).8 caption entries in an 11,000-line file show incorrect start times when a 608 stream transitions from pop-on to roll-up mode mid-broadcast:
End times match exactly in all 8 cases; only start times diverge, by 2-4.5s.
Root cause
The bug is in
COM_CARRIAGERETURN's handling of therollup_from_popontransition-timing mechanism, added by PR #1808.54df50f("preserve CR time during pop-on to roll-up transition") added:During a transition, CRs with
changes==0fire repeatedly while lines accumulate; this reassigns on every one, not just the first — the eventualchanges==1CR inherits the last CR's time instead of the transition's actual start (per--608trace on sample 79).The correct anchor is the fts of the first character written after the transition, via
write_char()'s guard:if (ts_start_of_current_line == -1) ts_start_of_current_line = get_fts(...). This guard is not new either — present, unmodified, at least since this repo's first commit; its actual origin predates that and isn't determinable from git history.Fix
Two changes, both required:
In the
COM_CARRIAGERETURNhandler: for therollup_from_popon && !changescase, only setts_start_of_current_lineto the CR time whenoutput_format == CCX_OF_TRANSCRIPT. For SRT, leave it untouched sowrite_char()'s existing== -1guard anchors to the first character typed — which is what the SRT reference corpus (RT84) anchors to. Transcript mode needs CR time (RT29/RT30); SRT needs first-char time (RT84). A single unconditional change breaks one or the other.In
write_cc_buffer_as_transcript2: changewrite_cc_line_as_transcript2fromvoidtoint(returns 1 if content was written, 0 if not) and usewrote_something |=in the caller. PR #2105 (03ad9e8e) setwrote_something = 1unconditionally even when nothing was written for a blank cursor row, causing a bare newline to be emitted for every blank row. The--null-terminatedWebSocket feature introduced by #2105 is unaffected.Verification
sha256sumof full ccextractor output on sample 79 matches referencee5c9d9ec...exactly. Diff before fix: 8 lines. After: 0, full file.--out=ttxt --latin1): transcript timing restored. 0-line diff vs reference.01509...ts,--out=ttxt --latin1): two spurious blank lines removed. Output is byte-identical to accepted variant ref9f139b8c....--out=srt): first-line timing error from current master (00:00:06,173vs reference00:00:01,868) is fixed. Two residual sub-frame deltas (~68ms, ~34ms) are pre-existing and unrelated to this PR.725a49f871.mpg) and 156 (c83f765c.ts),54df50f's own motivating samples: neither currently hash-matches its stored reference, and this predates this PR (confirmed via CI history, both platforms). This PR does not regress either: test 98's affected line is restored to exact match; test 156's largest timing error improves from 1301ms to 67ms. Remaining deltas in both (an XDS-path timing offset, a#vs music-note character substitution from300f8ca, and an unexplained residual ~67ms/2-field offset) are unrelated to this fix and out of scope for this PR.300f8ca.