-
Notifications
You must be signed in to change notification settings - Fork 517
[FEATURE] Add --scc-accurate-timing option for bandwidth-aware SCC output (fixes #1221) #1971
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
How did you test this? |
|
I tested using the Broadcast Source sample from issue #1120: Results (First Caption):
|
That only tests that there's been a change (on the first caption). How did you test that the output is now valid and that is in sync with audio? |
… commands, pass YouTube validation
|
I tested the output by uploading the generated SCC file to YouTube video of the broadcast sample:(https://www.youtube.com/watch?v=MLcqTwihpOQ) , which accepted it without any validation errors - previously it was rejected with timestamp ordering issues. You can verify the audio sync by watching the video with English subtitles enabled; the captions appear synchronized with the spoken dialogue. I also compared the output against the reference scc_tools implementation and confirmed the timing matches within 1-2 frames . |
Code Review: --scc-accurate-timingThanks for the detailed implementation and the YouTube validation. I've done a thorough code review. Where the 1-2 frame difference comes fromThe "timing matches within 1-2 frames" you mentioned comes from several intentional design choices in your implementation:
LLONG preroll_start = display_time - transmission_time_ms - one_frame_ms;You explicitly subtract one extra frame to ensure EOC is sent before display time.
LLONG new_preroll = context->scc_last_transmission_end + one_frame_ms;When resolving bandwidth collisions, you add another frame as a safety buffer.
So the 1-2 frame difference is by design - you're building in safety margins. This is actually good for broadcast compliance. Questions/Concerns
if (!use_separate_display_time)
{
// Legacy mode: always write clear
clear_screen(context, actual_end_time, data->channel, disassemble);
}You mention "scc_tools doesn't write EDM between consecutive captions" - but what happens if there's a gap between captions? Will the previous caption stay on screen until the next one appears? This could be an issue for sparse captions.
What I can verify
What I cannot verify without the sample
Could you clarify the EDM/clear behavior question above? If that's intentional and works correctly, this looks ready to merge. |
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit dfaebd5...:
Your PR breaks these cases:
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. |
CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results, when compared to test for commit dfaebd5...:
Congratulations: Merging this PR would fix the following tests:
All tests passed completely. Check the result page for more info. |
|
byte estimation is intentionally conservative . i think For most video content , captions are close together, so the next EOC naturally handles the transition , If this becomes an issue, I could add a threshold check: only skip EDM if the gap is small (e.g., < 2 seconds), otherwise write EDM. Proposed fix if needed: // Only skip clear if gap to next caption is small
if (!use_separate_display_time || (actual_end_time - actual_start_time > 2000))
{
clear_screen(context, actual_end_time, data->channel, disassemble);
}The test failure is unrelated to my changes. My PR only modifies the SCC encoder when --scc-accurate-timing is used. The test uses -out=srt with DVB subtitles, which doesn't touch any of the code I modified. |
In raising this pull request, I confirm the following (please check boxes):
My familiarity with the project is as follows (check one):
Summary
This PR implements bandwidth-aware timing for SCC (Scenarist Closed Caption) output to ensure YouTube and broadcast compliance, addressing issue #1221.
Problem
SCC files generated by CCExtractor were being rejected by YouTube due to timing violations. The EIA-608 standard specifies a transmission bandwidth of 2 bytes per frame, but CCExtractor was not accounting for the time required to transmit captions before their display time, causing overlapping transmissions and compliance issues.
Solution
Added a new
--scc-accurate-timingcommand-line option that implements:Changes Made
C Code
ccx_common_option.h/c: Addedscc_accurate_timingoption (default: off)ccx_encoders_common.h/c: Added timing state tracking (scc_last_transmission_end,scc_last_display_end)Rust Code
--scc-accurate-timingCLI argumentUsage