[IMPROVEMENT] --out report now probes both EIA-608 fields by default#2184
Open
Atul-Chahar wants to merge 2 commits intoCCExtractor:masterfrom
Open
[IMPROVEMENT] --out report now probes both EIA-608 fields by default#2184Atul-Chahar wants to merge 2 commits intoCCExtractor:masterfrom
Atul-Chahar wants to merge 2 commits intoCCExtractor:masterfrom
Conversation
…C frame delays Replace all 6 hardcoded 1000/29.97 frame delay calculations in dtvcc_write_scc() with 1000/current_fps so that CEA-708 SCC output uses the actual stream framerate instead of assuming NTSC 29.97. Fixes CCExtractor#2172
…ault When --out report is used, set extract=12 (both fields) and is_608_enabled=true so that CC3/CC4 are correctly detected without requiring --output-field both. Also exempt OutputFormat::Null from the broadcast-mode stdout conflict check, since report mode produces no actual output. Fixes CCExtractor#2177
Contributor
Author
|
Tested locally on Linux: Build: Version: Rust tests: Format check: New tests added:
|
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 f377be9...:
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. |
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 f377be9...:
Your PR breaks these cases:
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. |
Contributor
Author
Manual verification — CC3 detection now works with the fix:
|
Contributor
Author
|
Yes! I tested it against standard NTSC files that only contain CC1. Probing CC3/CC4 does not cause any harm or crashes; it gracefully returns empty streams as expected, while ensuring we capture all data on streams that do utilize both fields. |
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
2 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):
Summary
--out reportonly probes EIA-608 field 1 by default, so CC3/CC4 always showNoeven when captions exist in field 2. This is misleading — a user seesCC3: Noand concludes the file has no CC3 content, when in reality the report just never looked at field 2.Root Cause
In the
OutFormat::Reporthandler inparser.rs,extractis never set — it defaults to1(field 1 only). CC3/CC4 belong to field 2, so they are never decoded and the report always saysNofor them.Fix
extract = 12(both fields) andis_608_enabled = truein the report handler so both fields are probed.OutputFormat::Nullfrom the broadcast-mode stdout conflict check at the validation stage — report mode setswrite_format = Null, so there is no actual stdout output and the "can't extract both fields to stdout" error does not apply.Changes
src/rust/src/parser.rs— 3 locations:extract = 12+is_608_enabled = truewrite_format != Nullguardtest_out_report_enables_file_reports, addedtest_out_report_with_stdout_no_conflictdocs/CHANGES.TXT— added changelog entryPrecedence
If a user explicitly passes
--output-field 1alongside--out report, their choice wins —--output-fieldis parsed after--outand overwritesextract.AI disclosure: 🟡 AI-assisted — I reviewed, tested, and understand every line.
Fixes #2177