Fix SCC timing by accounting for EIA-608 transmission bandwidth #1827
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
DhanushVarma-2
force-pushed
the
fix/scc-accurate-timing
branch
2 times, most recently
from
e242dbc to
703c579
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 b728dda...:
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 b728dda...:
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
requested changes
Dec 16, 2025
Contributor
cfsmp3
left a comment
cfsmp3
left a comment
There was a problem hiding this comment.
Thanks for working on this timing fix! I did a deep review with actual testing.
The Good
The byte budgeting calculation is correct - you're correctly counting:
- Control codes (RCL, ENM, EOC = 6 bytes)
- Text bytes per row
- Padding to even bytes
- Frame duration at 29.97fps
The collision detection logic also looks correct.
Critical Issue: Timing Never Applied
However, the calculated tx_start_ms is never actually used to modify the SCC output. After the calculation block:
encoder->last_scc_tx_end_ms = tx_start_ms + tx_duration_ms;
// tx_start_ms is never used after this line!The rest of the function still uses time_show.time_in_ms for all print_scc_time() calls. I verified this by:
- Building both master and this PR
- Running them on the same test file
- Comparing output: byte-for-byte identical
Example from test file:
- First caption: 92 bytes, needs 46 frames (~1535ms) to transmit
- Display time: 00:00:10:02 (~10066ms)
- For broadcast compliance should start at: ~00:00:08:17
- Actual SCC output: Still uses 00:00:10:02
What Needs to Change
You need to use tx_start_ms for the actual timestamp output, something like:
struct ccx_boundary_time tx_start_time = get_time(tx_start_ms);
// Use tx_start_time instead of time_show for print_scc_time()Also
- Please remove the unrelated formatting changes in
ccx_decoders_isdb.c - Run
clang-formaton the modified files
Happy to review again once the timing is actually applied to the output!
- Add last_scc_tx_end_ms field to encoder_ctx - Initialize it to -1 in init_encoder - Use encoder->last_scc_tx_end_ms instead of static variable - This ensures SCC timing only affects SCC output format - Other formats (SRT, TXT, TTXT) are unaffected Fixes CCExtractor#1221
DhanushVarma-2
force-pushed
the
fix/scc-accurate-timing
branch
from
74c98ec to
399b939
Compare
The transmission end should be the original display time (when caption becomes visible), not the pre-rolled transmission start time.
Contributor
PR Review - Testing ResultsI tested this PR against the sample files from issue #1120 (Broadcast Source.ts) and compared the output between master, this PR, and the reference SCC file generated by scc_tools. Issue FoundThis PR doesn't fix the reported issue #1221 because it modifies the wrong code path:
Test Results
The main SCC output is identical between master and this PR - no timing improvement for 608 captions. What This PR Actually Does
Requested ChangesTo fix issue #1221, the same byte budgeting and pre-roll timing logic needs to be applied to the EIA-608 SCC output path. This would likely involve changes to:
The 708 timing changes in this PR may be independently useful, but they don't solve the reported 608 timing problem. Consider either:
|
added 2 commits
December 22, 2025 23:41
- Add byte budgeting calculation for 608 captions - Pre-roll transmission start time by transmission duration - Prevent collision with previous caption transmissions - Track transmission end time for collision detection This applies the same timing logic from the 708 path to fix issue CCExtractor#1221 for EIA-608 SCC output.
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):
My familiarity with the project is as follows (check one):
This PR fixes inaccurate SCC timing by accounting for the 2-bytes-per-frame
EIA-608 transmission limit (29.97 fps).
Changes:
This makes SCC output compliant with broadcast rules and fixes #1221.