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
ccx_decoders_608.c,COM_CARRIAGERETURNhandler. Introduced by 54df50f (2025-12-13, "preserve CR time during pop-on to roll-up transition"):When a stream transitions pop-on -> roll-up, CRs with
changes==0(roll-up window not yet full) fire repeatedly while lines accumulate. This line unconditionally reassigns on every such CR, not just the first. Confirmedvia
--608debug trace on sample 79: CR#1/CR#2/CR#3 each overwrite the value; the eventualchanges==1CR (which emits the cue) inherits CR#3's time instead of the transition's actual start.Fixing only the overwrite closed ~98% of the gap (2.6s -> 66ms residual, uniform across all 8 entries, ~2 field-durations at 29.97fps). Runtime instrumentation (fts logged at CR, PAC, mid-row text-attribute, and first character) showed the reference anchors to the fts of the first character written after the transition, not the CR:
This matches pre-existing behavior in
write_char():if (ts_start_of_current_line == -1) ts_start_of_current_line = get_fts(...)— confirmed viagit log -S, this guard is unmodified since commit 617d2d3 (2014-10-07), 11+ years before 54df50f. The fix removes 54df50f's override on thechanges==0path and lets that guard govern again.Cross-check against 54df50f's own stated verification: its PR (#1808) reports, for sample
725a49f871(also used below): "Before fix: 1,501ms... After Fix 3: 2,118ms (133ms late)... After Fix 4: 1,985ms (0ms offset, shipped)" measured against ffmpeg. Our data for this same file: the sample-platform's actual stored reference is 2,118ms for this caption —matching "after Fix 3," not the shipped "after Fix 4" value. This suggests Fix 3 alone was already correct here, and Fix 4's ffmpeg-based verification was itself mismeasured for this transition (consistent with ffmpeg's
eia608decoder failing outright on sample 79 — not a reliable oracle forthis corpus).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.