fix(608): only apply pop-on→roll-up EOF fallback when start time is unset - #2291
fix(608): only apply pop-on→roll-up EOF fallback when start time is unset#2291x15sr71 wants to merge 1 commit into
Conversation
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit 2feb09a...:
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. |
|
I applied this change onto current master (the branch base is from July, so building it directly would confound the comparison) and compared RT11's output against its approved baseline. It moves away from the baseline rather than toward it:
The regression is cue 25: Master already gets that cue exactly right, and the guard makes it start 2 ms after cue 24 ends and run for 32 seconds — for The underlying reasoning is right — a fallback that overwrites an already-valid One process note: the platform runs cited (9350/9351) predate the baseline fix in sample-platform#1175, so at the time a PR's red/green on the suite was partly determined by which run preceded it. Worth re-running against a current baseline before drawing conclusions from those numbers. Happy to look again if you have a sample where the current guard demonstrably helps — the diff itself is small and the intent is clearly right. |
|
Agreed, closing — I'd already left this in draft with the same doubt, and your numbers settle it. Thanks for re-running against a current baseline — that's conclusive. You're right that Appreciate the offer to look again, but I don't have a sample where the guard demonstrably helps. I'm still working the underlying timing issue, specifically RT84 and RT11, which both fail on current master. I have a candidate and I'm validating it against the ~165 regression tests I have samples for on my Linux box; if it holds up across the suite I'll open a PR right away. |
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 11, which fails on master on both Windows and Linux. On Linux, it currently shows a separate “No output generated but there should be” issue that is still under investigation; Reference test run - Windows, Linux.
Problem
A 608 regression caused incorrect cue start times on a WTV sample, while subtitle text and end times remained correct.
Root cause
689b27ceadded an EOF fallback for pop-on -> roll-up transitions by back-fillingcurrent_visible_start_msfromts_first_char_rollup_transition.That fallback was too broad: if
rollup_from_poponwas still set whenwrite_cc_buffer()ran, it could overwrite an already validcurrent_visible_start_mswith the first-character FTS.Fix
Only apply the EOF fallback when
current_visible_start_msis still unset:This keeps the EOF fix from
689b27cewhile avoiding clobbering valid cue start times on normal paths.Testing
611b4a9235c08....write_cc_buffer().2af8168380d0...) is clean.