Skip to content

[Fix](608): restore current_visible_start_ms set-on-first-char for rollup#2268

Merged
cfsmp3 merged 1 commit intoCCExtractor:masterfrom
gaurav02081:fix/options-tests-228-230
Apr 22, 2026
Merged

[Fix](608): restore current_visible_start_ms set-on-first-char for rollup#2268
cfsmp3 merged 1 commit intoCCExtractor:masterfrom
gaurav02081:fix/options-tests-228-230

Conversation

@gaurav02081
Copy link
Copy Markdown
Contributor

@gaurav02081 gaurav02081 commented Apr 19, 2026 •
edited
Loading

In raising this pull request, I confirm the following (please check boxes):

Reason for this PR:

  • This PR adds new functionality.
  • This PR fixes a bug that I have personally experienced or that a real user has reported and for which a sample exists.
  • This PR is porting code from C to Rust.

Sanity check:

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • If the PR adds new functionality, I've added it to the changelog. If it's just a bug fix, I have NOT added it to the changelog.
  • I am NOT adding new C code unless it's to fix an existing, reproducible bug.

Repro instructions:

This is essential. We will not merge ANY PR that doesn't come with detailed instructions, including a sample. We don't want
"fixes" for theoretical issues that an AI agent found, without context. If you can't reproduce the bug, don't send a PR.

Creating PRs with AI is very quick, but we still have humans (even if AI assisted) going over each.

Be mindful of reviewers' time.

## Summary

Fixes an issue where CEA-608 roll-up captions were emitted with a start time of `00:00:00,000 " instead of the actual first-character FTS.

This occurs in roll-up mode streams where fewer than the required number of lines are written before EOF (or more generally, when no CR event results in changes == true).

Before: 00:00:00,000 --> ... " **After:** 00:00:13,913 --> ...` (matches first-character FTS)

## Root cause

Commits bc5d6055 / 54df50f4 introduced a rollup_from_popon flag in ccx_decoder_608_context to delay setting current_visible_start_ms until a CR event triggered scrolling.

However, in cases where:

  • no CR produces changes == true, or
  • the stream ends before enough lines are written (e.g., roll-up-2),

the flag is never cleared. As a result, current_visible_start_ms remains 0, and captions are emitted starting at 00:00:00,000.

In contrast, commit 042716ad (pre-change) set current_visible_start_ms immediately when the first character was written to an empty buffer (in non-POPON mode), which correctly handles these cases.

Fix

Remove the rollup_from_popon mechanism and restore the previous behaviour of setting the start time on the first character.

Changes

  • src/lib_ccx/ccx_decoders_608.c

    • Remove rollup_from_popon guard from character handler
    • Restore original CR handling logic
    • Remove related initialization

  • src/lib_ccx/ccx_decoders_608.h

    • Remove rollup_from_popon field

  • Cleanup:

    • Remove debug traces (dbg_print, printf) from:
      • ccx_decoders_common.c
      • sequencing.c
      • avc_functions.c
    • No functional changes in these files

Test plan

  • Default build passes (cmake --build .) with no new warnings
  • Reproducer sample:
  • Start time corrected (00:00:13,913 vs 00:00:00,000)
  • Caption content, count, and ordering unchanged vs pre-bc5d6055 behaviour.

@cfsmp3
Copy link
Copy Markdown
Contributor

cfsmp3 commented Apr 20, 2026

What sample did you use to test it? I haven't been able to repro with any of the ones I have locally

@gaurav02081
Copy link
Copy Markdown
Contributor Author

gaurav02081 commented Apr 20, 2026 •
edited
Loading

here is the sample i used.

* File: c4dd893cb9d67be50f88bdbd2368111e16b9d1887741d66932ff2732969d9478.ts

  • Duration: ~15s (roll-up-2 never reaches required lines before EOF)

Command:
ccextractor sample.ts -o out.srt

On master:
1
00:00:00,000 --> 00:00:14,980

WHICH OF THESE STORIES WILL
YOU BE TALKING ABOUT TRO

This PR:

1
00:00:13,913 --> 00:00:14,980

WHICH OF THESE STORIES WILL
YOU BE TALKING ABOUT TRO

Diff (only the start timestamp changes; caption text, end time, and cue count remain identical):

  • 00:00:00,000 --> 00:00:14,980
  • 00:00:13,913 --> 00:00:14,980

cfsmp3
cfsmp3 requested changes Apr 21, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@cfsmp3 cfsmp3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep Review — Regressions Found

The rollup timing fix works on your sample (c4dd893cb9d6): start time corrects from 00:00:00,000 to 00:00:13,913. That bug is real.

However, the broad revert of commits bc5d6055/54df50f4 (slice_header, do_cb, sequencing changes) introduces timing regressions on multiple other samples. We tested against reference files we have on disk:

Sample Format Master (matches reference) This PR Delta
0069dffd2180.mpg MPG CEA-608 00:00:03,603 00:00:00,766 -2.8s
5cbb21adb6e0.dvr-ms DVR-MS CEA-608 00:00:00,534 00:00:00,734 +200ms
6395b281adf0.asf ASF CEA-608 00:00:02,436 00:00:01,068 -1.4s
8849331ddae9.mp4 MP4 c608 00:00:03,771 00:00:04,738 +1s
132d7df7e993.mov MOV CEA-608 00:00:12,812 00:00:13,980 +1.2s
b22260d065ab.ts TS CEA-608 00:00:04,838 00:00:02,635 -2.2s

These are seconds-level shifts, not rounding. Master matches the reference files; this PR does not.

Root cause

The reverted commits were fixing real timing issues across container formats:

  • slice_header IDR-only flush — flushing on every reference frame (as this PR restores) causes reordering problems in I/P-only streams
  • do_cb field counting guards — the H.264/PES guards on cb_field1++ prevent double-counting in container formats where PTS already represents the correct timestamp
  • sequencing.c PES reset — removing PES from reset_cb breaks timing for MPEG-2 PES streams

What needs to happen

The rollup 00:00:00,000 bug is real and should be fixed, but with a narrower fix that only addresses the rollup_from_popon edge case. Specifically:

  1. Keep the slice_header changes (IDR-only flush, large PTS gap handling) — do NOT revert
  2. Keep the do_cb field counting guards — do NOT revert
  3. Keep the sequencing.c PES reset — do NOT revert
  4. Keep the get_visible_start/get_visible_end Rust calls — do NOT revert
  5. Fix ONLY the rollup_from_popon logic so it handles the case where no CR fires before EOF

The bug is that rollup_from_popon never clears when the stream ends with fewer lines than the roll-up window requires. A targeted fix would be to clear rollup_from_popon and set current_visible_start_ms during the end-of-stream flush path, not by reverting the entire mechanism.

Test commands

# The bug (should NOT be 00:00:00,000):
ccextractor c4dd893cb9d6...ts -o out.srt

# Regressions (should match reference):
ccextractor 0069dffd2180...mpg -o out.srt   # expect 00:00:03,603
ccextractor 5cbb21adb6e0...dvr-ms -o out.srt # expect 00:00:00,534
ccextractor 6395b281adf0...asf -o out.srt    # expect 00:00:02,436

@gaurav-dev02
fix(608): handle pop-on to roll-up transition ending at EOF
1d22517 
When a stream switches from pop-on to roll-up mode and ends before
check_roll_up() reports changes=1 (fewer lines than the roll-up window
requires), the CR handler never runs the backfill at line 836 and
current_visible_start_ms is still zero when flush_608_context fires
EraseDisplayedMemory. Result: the first emitted caption is timestamped
at 00:00:00,000.

Capture the FTS of the first character after the transition in a new
field ts_first_char_rollup_transition. Unlike ts_start_of_current_line,
this field is not overwritten by intermediate changes=0 CRs, so the
first-char time survives to the end-of-stream flush path.
write_cc_buffer uses it to backfill current_visible_start_ms when
rollup_from_popon is still set at emit time, and clears it together
with the flag. The normal popon->roll-up flow (where a scrolling CR
eventually fires) and every other caption path are unchanged.

Verified on c4dd893cb9d6...ts: first subtitle now starts at
00:00:13,913 (was 00:00:00,000). Regression tested against six master
samples the mentor flagged (0069dffd.mpg 03,603; 5cbb21ad.dvr-ms
00,534; 132d7df7.mov 12,812; 6395b281.asf 02,436; 8849331d.mp4 03,771;
b22260d0.ts 04,838) - all match the reference timings unchanged.
@gaurav02081 gaurav02081 force-pushed the fix/options-tests-228-230 branch from c6647a6 to 1d22517 Compare April 21, 2026 19:51
@gaurav02081
Copy link
Copy Markdown
Contributor Author

gaurav02081 commented Apr 21, 2026

Thanks for your time :-)

I restored all three commits I had reverted. I added a precise fix just for the EOF edge case.

Reshaped the branch to a single commit (1d22517).

Fix

Added a new field, ts_first_char_rollup_transition, that captures the first-char FTS right after the popon→rollup switch. Unlike ts_start_of_current_line, it doesn't get clobbered by changes=0 CRs in between, so it survives to the end-of-stream flush. write_cc_buffer uses it to backfill current_visible_start_ms when rollup_from_popon is still set at emit time. The normal CR path is untouched.
Numbers

c4dd893c still fixed: first caption at 00:00:13.913 (was 00:00:00.000).

Ran all six regression samples you listed = every one matches the master exactly:

0069dffd.mpg    Expected: 00:00:03,603 | Got: 00:00:03,603
5cbb21ad.dvr-ms Expected: 00:00:00,534 | Got: 00:00:00,534
6395b281.asf    Expected: 00:00:02,436 | Got: 00:00:02,436
8849331d.mp4    Expected: 00:00:03,771 | Got: 00:00:03,771
132d7df7.mov    Expected: 00:00:12,812 | Got: 00:00:12,812
b22260d0.ts     Expected: 00:00:04,838 | Got: 00:00:04,838

@ccextractor-bot
Copy link
Copy Markdown
Collaborator

ccextractor-bot commented Apr 21, 2026

CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit 283bfd9...:
Report Name Tests Passed
Broken 8/13
CEA-708 1/14
DVB 2/7
DVD 3/3
DVR-MS 2/2
General 22/27
Hardsubx 1/1
Hauppage 3/3
MP4 3/3
NoCC 10/10
Options 73/86
Teletext 20/21
WTV 12/13
XDS 29/34

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:

  • ccextractor --autoprogram --out=ttxt --latin1 132d7df7e9..., Last passed: Never
  • ccextractor --autoprogram --out=srt --latin1 b22260d065..., Last passed: Never

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.

@ccextractor-bot
Copy link
Copy Markdown
Collaborator

ccextractor-bot commented Apr 21, 2026

CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results, when compared to test for commit 283bfd9...:
Report Name Tests Passed
Broken 8/13
CEA-708 1/14
DVB 3/7
DVD 3/3
DVR-MS 2/2
General 22/27
Hardsubx 1/1
Hauppage 3/3
MP4 3/3
NoCC 10/10
Options 74/86
Teletext 20/21
WTV 12/13
XDS 28/34

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:

  • ccextractor --autoprogram --out=srt --latin1 --quant 0 85271be4d2..., Last passed: Never
  • ccextractor --autoprogram --out=ttxt --latin1 --ucla dab1c1bd65..., Last passed: Never
  • ccextractor --out=srt --latin1 --autoprogram 29e5ffd34b..., Last passed: Never
  • ccextractor --out=spupng c83f765c66..., Last passed: Never

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 cfsmp3 merged commit 689b27c into CCExtractor:master Apr 22, 2026
25 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cfsmp3 cfsmp3 cfsmp3 requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gaurav02081 @cfsmp3 @ccextractor-bot @gaurav-dev02