[FIX] Warn when non-DVB caption PIDs are silently ignored - #2334
Open
kaihere14 wants to merge 1 commit into
Open
[FIX] Warn when non-DVB caption PIDs are silently ignored#2334kaihere14 wants to merge 1 commit into
kaihere14 wants to merge 1 commit into
Conversation
Closes CCExtractor#2333 When a program contains multiple caption-carrying PIDs of the same non-DVB codec (Teletext, ISDB, or ATSC), only one is ever decoded — every other same-type PID is silently discarded with no warning, no log line, and no indication to the user that a second language track existed. This adds a warning at the actual root cause: the DVB-only exemption in `ignore_other_stream()` and `ignore_other_sib_stream()` (`src/lib_ccx/ts_info.c`). A new static helper, `warn_ignored_caption_stream()`, prints the ignored PID and points the user to `--datapid` to recover it manually. DVB is explicitly excluded since it already has its own extra-PID handling and doesn't need this. **Scope**: this is a warning only. It does NOT add multi-PID decoding support for Teletext/ISDB/ATSC — see CCExtractor#2333 for why that's a separate, larger effort (per-PID decoder allocation, per-language encoder routing, and ISO-639 storage for Teletext don't exist today for non-DVB codecs). **Real broadcast sample** (two Teletext PIDs, one program — from samples.ffmpeg.org/ffmpeg-bugs/trac/ticket3514): $ ccextractor --tpages-all t3514.ts VBI/teletext stream ID 3401 (0xd49) for SID 1002 (0x3ea) VBI/teletext stream ID 3402 (0xd4a) for SID 1002 (0x3ea) Warning: Teletext caption stream ID 3402 (0xd4a) for SID 1002 (0x3ea) will be ignored - only one caption stream per program is extracted. Use --datapid 3402 to extract it in a separate run. Correct PID, correct SID. The 8 DVB subtitle PIDs in the same file stay silent, as expected. **Synthetic two-PID split** (dvbteletext.ts split across two PIDs): same correct behavior, warns on the dropped PID (202/0xca). **ISDB and ATSC branches** — verified against a constructed sample (`mixed_codecs.ts`, built from `dvbteletext.ts` by injecting two ES entries into its PMT: PID 0xca as ISDB via a data_component descriptor, PID 0xcb as ATSC via a caption_service descriptor): $ ccextractor --tpages-all mixed_codecs.ts VBI/teletext stream ID 201 (0xc9) for SID 201 (0xc9) *****ISDB subtitles detected Warning: ISDB caption stream ID 202 (0xca) for SID 201 (0xc9) will be ignored - only one caption stream per program is extracted. Use --datapid 202 to extract it in a separate run. Warning: ATSC caption stream ID 203 (0xcb) for SID 201 (0xc9) will be ignored - only one caption stream per program is extracted. Use --datapid 203 to extract it in a separate run. Teletext output (`mixed_codecs_p694.srt`, `_p765.srt`) is byte-identical to the unmodified `dvbteletext.ts` baseline — the injected PIDs change nothing except triggering the new warnings. Note: I could not source a real multi-PID ISDB recording — the one public ARIB sample I found (samples.ffmpeg.org/MPEG2/subcc, single PID) has a multi-TS-packet PMT my synthetic splitter doesn't handle, so the ISDB/ATSC branches above are exercised by a constructed sample rather than a genuine broadcast recording. Flagging this rather than implying otherwise — the Teletext path is verified on real broadcast data (see above), ISDB/ATSC are verified by construction. **DVB control** — confirms no regression: ran both a single-PID and a split two-PID DVB sample before and after the patch. No new warnings on either, and output is byte-identical (PNG counts and `diff -rq` match exactly pre- and post-patch). All decoded `.srt` files from the Teletext samples are also byte-identical to their pre-patch versions — this change only adds a print statement, no decoding logic is touched. **Caught during testing, fixed before this PR**: the first version of this warned on every video PID in every file, because 608/708 captions are internally tagged `CCX_CODEC_ATSC_CC` even though they live inside the video stream, not a separate PID (`ts_tables.c:492-498`). Fixed by excluding `CCX_STREAM_TYPE_VIDEO_MPEG2/_H264/_HEVC` from the warning. Re-verified against a European DVB file (now silent, as it should be) and a US ATSC file with real 608/708 captions in video (still silent, correct — genuine caption-only ATSC PIDs still warn correctly). **Known cosmetic issue, pre-existing, not introduced by this patch**: under `--pmt`, the ATSC warning can visually run together with a preceding debug line (`ts_tables.c:445-449` emits its `dbg_print` without a trailing newline, so whatever prints next collides with it on the same line). This only occurs with `--pmt` and would happen with any output following that debug line, not just this warning. Out of scope for this PR per its stated scope, noting it here so it isn't mistaken for something this patch caused. **Reason for this PR:** - [ ] This PR adds new functionality. - [x] 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:** - [x] I have read and understood the contributors guide. - [x] I have checked that another pull request for this purpose does not exist. - [x] 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. - [x] I am NOT adding new C code unless it's to fix an existing, reproducible bug.
Collaborator
CCExtractor CI platform finished running the test files on windows. 171/237 tests matched the approved output:
66 tests do not match the approved output. That is the pass/fail verdict. Whether this branch caused it is a separate question, answered below.
Compared with the tip of master — test 9519, commit cf42e6c:
Compared with the commit this branch was cut from — test 9521, commit 2ad84df:
No test changes behaviour relative to the tip of master: every failure above fails there too, byte for byte. The approved output for those tests is out of date, which is a baseline to review rather than a regression in this branch. |
Collaborator
CCExtractor CI platform finished running the test files on linux. 171/237 tests matched the approved output:
66 tests do not match the approved output. That is the pass/fail verdict. Whether this branch caused it is a separate question, answered below.
Compared with the tip of master — test 9524, commit 3875e84:
Compared with the commit this branch was cut from — test 9520, commit 2ad84df:
No test changes behaviour relative to the tip of master: every failure above fails there too, byte for byte. The approved output for those tests is out of date, which is a baseline to review rather than a regression in this branch. |
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.
Closes #2333
When a program contains multiple caption-carrying PIDs of the same non-DVB codec (Teletext, ISDB, or ATSC), only one is ever decoded — every other same-type PID is silently discarded with no warning, no log line, and no indication to the user that a second language track existed.
This adds a warning at the actual root cause: the DVB-only exemption in
ignore_other_stream()andignore_other_sib_stream()(src/lib_ccx/ts_info.c). A new static helper,warn_ignored_caption_stream(), prints the ignored PID and points the user to--datapidto recover it manually. DVB is explicitly excluded since it already has its own extra-PID handling and doesn't need this.Scope: this is a warning only. It does NOT add multi-PID decoding support for Teletext/ISDB/ATSC — see #2333 for why that's a separate, larger effort (per-PID decoder allocation, per-language encoder routing, and ISO-639 storage for Teletext don't exist today for non-DVB codecs).
Real broadcast sample (two Teletext PIDs, one program — from samples.ffmpeg.org/ffmpeg-bugs/trac/ticket3514):
$ ccextractor --tpages-all t3514.ts
VBI/teletext stream ID 3401 (0xd49) for SID 1002 (0x3ea) VBI/teletext stream ID 3402 (0xd4a) for SID 1002 (0x3ea) Warning: Teletext caption stream ID 3402 (0xd4a) for SID 1002 (0x3ea) will be ignored - only one caption stream per program is extracted. Use --datapid 3402 to extract it in a separate run.
Correct PID, correct SID. The 8 DVB subtitle PIDs in the same file stay silent, as expected.
Synthetic two-PID split (dvbteletext.ts split across two PIDs): same correct behavior, warns on the dropped PID (202/0xca).
ISDB and ATSC branches — verified against a constructed sample (
mixed_codecs.ts, built fromdvbteletext.tsby injecting two ES entries into its PMT: PID 0xca as ISDB via a data_component descriptor, PID 0xcb as ATSC via a caption_service descriptor):$ ccextractor --tpages-all mixed_codecs.ts
VBI/teletext stream ID 201 (0xc9) for SID 201 (0xc9) *****ISDB subtitles detected
Warning: ISDB caption stream ID 202 (0xca) for SID 201 (0xc9) will be ignored - only one caption stream per program is extracted. Use --datapid 202 to extract it in a separate run. Warning: ATSC caption stream ID 203 (0xcb) for SID 201 (0xc9) will be ignored - only one caption stream per program is extracted. Use --datapid 203 to extract it in a separate run.
Teletext output (
mixed_codecs_p694.srt,_p765.srt) is byte-identical to the unmodifieddvbteletext.tsbaseline — the injected PIDs change nothing except triggering the new warnings.Note: I could not source a real multi-PID ISDB recording — the one public ARIB sample I found (samples.ffmpeg.org/MPEG2/subcc, single PID) has a multi-TS-packet PMT my synthetic splitter doesn't handle, so the ISDB/ATSC branches above are exercised by a constructed sample rather than a genuine broadcast recording. Flagging this rather than implying otherwise — the Teletext path is verified on real broadcast data (see above), ISDB/ATSC are verified by construction.
DVB control — confirms no regression: ran both a single-PID and a split two-PID DVB sample before and after the patch. No new warnings on either, and output is byte-identical (PNG counts and
diff -rqmatch exactly pre- and post-patch). All decoded.srtfiles from the Teletext samples are also byte-identical to their pre-patch versions — this change only adds a print statement, no decoding logic is touched.Caught during testing, fixed before this PR: the first version of this warned on every video PID in every file, because 608/708 captions are internally tagged
CCX_CODEC_ATSC_CCeven though they live inside the video stream, not a separate PID (ts_tables.c:492-498). Fixed by excludingCCX_STREAM_TYPE_VIDEO_MPEG2/_H264/_HEVCfrom the warning. Re-verified against a European DVB file (now silent, as it should be) and a US ATSC file with real 608/708 captions in video (still silent, correct — genuine caption-only ATSC PIDs still warn correctly).Known cosmetic issue, pre-existing, not introduced by this patch: under
--pmt, the ATSC warning can visually run together with a preceding debug line (ts_tables.c:445-449emits itsdbg_printwithout a trailing newline, so whatever prints next collides with it on the same line). This only occurs with--pmtand would happen with any output following that debug line, not just this warning. Out of scope for this PR per its stated scope, noting it here so it isn't mistaken for something this patch caused.Reason for this PR:
Sanity check: