fix(608): anchor pop-on->roll-up transition start time to first character, not CR#2290
Open
x15sr71 wants to merge 1 commit into
Open
fix(608): anchor pop-on->roll-up transition start time to first character, not CR#2290x15sr71 wants to merge 1 commit into
x15sr71 wants to merge 1 commit into
Conversation
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
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 sincetest_id 6698(Dec 2025, shortly after 54df50f merged), including at thev0.96.6release 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: don't touchts_start_of_current_lineonchanges==0CRs during a transition. If still -1, the first subsequentwrite_char()call sets it (per the 2014 guard above). If already set, preserve it.In
write_cc_line()'s caller (CCX_OF_TRANSCRIPToutput path): resetts_start_of_current_line = -1immediately after each call. Transcript mode treats every CR as a new line boundary and expects this reset; without it, fix (1) causes a value to leak into the next line's start time. Found via regression test 98 (sample 725a49f871...mpg): applying fix (1) alone regressed one line (133ms -> 617ms error vs reference); adding this reset restored an exact match.Verification
sha256sumof full ccextractor output on sample 79 matches reference e5c9d9ec... exactly. Diff before fix: 8 lines. After: 0, full file.#vs music-note character substitution from 300f8ca, and an unexplained residual ~67ms/2-field offset) are unrelated to this fix and out of scope for this PR.