-
Notifications
You must be signed in to change notification settings - Fork 513
MXF: improve ANC/VANC caption detection (fix #1647) #1831
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
MXF: improve ANC/VANC caption detection (fix #1647) #1831
Conversation
cfsmp3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your first contribution to CCExtractor! I appreciate the detailed PR description and your effort to investigate issue #1647.
After reviewing the code changes, I have some concerns:
Code doesn't fix the extraction issue
The PR adds a flag to track when ANC captions are detected, but doesn't actually change the extraction logic. Setting found_anc_captions = 1 when we see SDID 0x01/0x02 confirms captions exist, but if they're not being extracted, knowing they exist doesn't help.
The change in ccx_mxf_getmoredata():
if (ret <= 0 && mxf && mxf->found_anc_captions)
return 0;This masks error conditions but doesn't cause captions to be extracted.
To actually fix #1647, we need to debug:
- Is the
cap_essence_keymatching the caption track in the problem file? - Is
mxf_read_vanc_data()being called at all for this file? - Are there different DID values (not 0x61/0x80) in some MXF files?
- Is
mxf_read_cdp_data()parsing the CDP correctly?
Would you be able to add some debug logging and test with the sample file from issue #1647? That would help identify the actual root cause.
Code quality issues to fix:
- Remove commented-out code: Delete the old
ccx_mxf_init()duplicate - Fix indentation: The code has inconsistent tabs/spaces mixing
- Add NULL check: Check
ctxinmxf_read_vanc_data()before dereferencing - Remove redundant init:
found_anc_captions = 0after memset is unnecessary - Resolve conflicts: Rebase on current master
Sample file needed
To properly test and validate any fix for this issue, we need access to a sample MXF file that exhibits this problem. The original issue #1647 includes a Google Drive link to a sample file - could you confirm you're testing with that file, or provide a sample we can use to reproduce and verify the fix?
Without a test file, we can't validate that any changes actually solve the problem.
Happy to help debug this if you can provide more information about what's happening when CCExtractor processes MXF files with 708 captions!
|
Thank you for working on this issue! This is a good area to investigate. However, several changes are needed:
Do you have a sample MXF file that demonstrates the issue? That would help us verify the fix works correctly. |
|
Hi @03-SudheshnaReddy, any update on this PR? It needs:
Let us know if you're still working on this or if we should close it. |
|
Hi @cfsmp3 Thank you for the detailed review and for following up — I’m really sorry for going quiet on this PR. That wasn’t intentional. I want to be honest: this is my first time working in this part of the CCExtractor codebase, and I underestimated the complexity of the MXF demuxer. After your feedback, I understand that my current changes don’t actually fix the extraction path and mostly add detection/guard logic without addressing the root cause. I think I froze a bit because I realized the PR wasn’t in a good state yet, but I should have communicated instead — that’s on me. I’m still very interested in learning and contributing properly. If you’re okay with it, I’d like to either: rework this PR into a diagnostics-focused change (better logging to understand why the sample fails), or close this PR and come back with a better-tested fix once I understand the full pipeline better. I’m a beginner here, but I’m serious about improving and putting in the work. I appreciate your time and guidance, and I’m happy to follow whatever direction you think makes most sense. Thanks again. |
|
@03-SudheshnaReddy Let's close it for you, you can work on it whenever it's convenient for you, no worries. |
Fixes #1647
What I changed
ccx_demuxer_mxf.c/.h)Why
CCExtractor currently reports “No subtitles found” for some MXF files that clearly contain captions, as confirmed by Premiere Pro.
Investigation showed that ANC/VANC caption packets can be present but not detected due to demuxer-level logic gaps.
This PR aims to ensure CCExtractor correctly identifies and processes those caption packets.
Scope
Notes