[FIX] The first TODO in avc_functions.c and FIXME in avc_functions.c #1695
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses two specific items in the AVC functions library: a TODO comment about handling broken NAL units and a FIXME comment about calling set_fts for SEI NAL units. The changes improve error handling for corrupted AVC/H.264 streams and ensure proper timestamp handling for SEI units.
- Enhanced error messaging for corrupted NAL units with detailed explanations
- Enabled set_fts function call for SEI NAL units to fix timestamp handling
- Updated changelog to document the fixes
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/lib_ccx/avc_functions.c | Improved error message for corrupted NAL units and enabled set_fts call for SEI units |
| docs/CHANGES.TXT | Added changelog entry documenting the TODO and FIXME fixes |
| mprint("\rWarning: Invalid prevention bytes detected in NAL unit type %u (0x%02X). " | ||
| "This NAL unit contains an illegal byte sequence (0x000000, 0x000001, or 0x000002) or " | ||
| "improper prevention byte (0x03). " | ||
| "This may indicate a corrupted AVC/H.264 stream. NAL unit skipped.\n", |
There was a problem hiding this comment.
The error message incorrectly describes the failure condition. The remove_03emu function can fail for reasons other than invalid prevention bytes, such as general parsing errors or memory issues. Consider using a more generic message like 'Failed to process NAL unit type %u (0x%02X) due to emulation prevention byte removal failure.'
| mprint("\rWarning: Invalid prevention bytes detected in NAL unit type %u (0x%02X). " | |
| "This NAL unit contains an illegal byte sequence (0x000000, 0x000001, or 0x000002) or " | |
| "improper prevention byte (0x03). " | |
| "This may indicate a corrupted AVC/H.264 stream. NAL unit skipped.\n", | |
| mprint("\rWarning: Failed to process NAL unit type %u (0x%02X) due to emulation prevention byte removal failure. " | |
| "This may indicate a corrupted AVC/H.264 stream or an internal processing error. NAL unit skipped.\n", |
Copilot uses AI. Check for mistakes.
| "This NAL unit contains an illegal byte sequence (0x000000, 0x000001, or 0x000002) or " | ||
| "improper prevention byte (0x03). " |
There was a problem hiding this comment.
The error message contains technical inaccuracies. The sequences 0x000000, 0x000001, and 0x000002 are not inherently illegal in H.264 streams - 0x000001 is actually the standard start code prefix. The message should focus on the actual failure rather than speculating about specific byte patterns.
| "This NAL unit contains an illegal byte sequence (0x000000, 0x000001, or 0x000002) or " | |
| "improper prevention byte (0x03). " | |
| "This NAL unit contains improper prevention bytes (0x03) or other anomalies. " |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I think this suggestion is incorrect, The comment at Line 274 states that 0x000000, 0x000001 or 0x000002 shall not occur at any byte-aligned position
| return; | ||
| } | ||
|
|
||
| dvprint("BEGIN NAL unit type: %d length %d ref_idc: %d - Buffered captions before: %d\n", | ||
| nal_unit_type, NAL_stop - NAL_start - 1, dec_ctx->avc_ctx->nal_ref_idc, !dec_ctx->avc_ctx->cc_buffer_saved); | ||
|
|
There was a problem hiding this comment.
Moving the dvprint statement after the error check means debug information won't be printed for NAL units that fail emulation prevention byte removal. This could make debugging more difficult. Consider keeping the debug print before the error check or adding a separate debug message for failed NAL units.
| return; | |
| } | |
| dvprint("BEGIN NAL unit type: %d length %d ref_idc: %d - Buffered captions before: %d\n", | |
| nal_unit_type, NAL_stop - NAL_start - 1, dec_ctx->avc_ctx->nal_ref_idc, !dec_ctx->avc_ctx->cc_buffer_saved); | |
| dvprint("Failed to process NAL unit type: %d - Emulation prevention byte removal failed.\n", | |
| nal_unit_type); | |
| return; | |
| } |
Copilot uses AI. Check for mistakes.
|
@prateekmedia I have AVC Functions in 11th week of my Plan, should we scrap this PR as the code will be redundant anyway? |
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit 1c7e2a0...:
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 1c7e2a0...:
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. |
In raising this pull request, I confirm the following (please check boxes):
My familiarity with the project is as follows (check one):
This PR resolves one TODO and one FIXME in the avc_functions.c.
The first TODO was -
TODO: Do something if newsize == -1 (broken NAL)So, what my changes do is, print out a more verbose error message.
The first FIXME was to check the set_fts function for
CCX_NAL_TYPE_SEIwhich was only being checked in slice_header for NAL Type asCCX_NAL_TYPE_CODED_SLICE_IDR_PICTUREorCCX_NAL_TYPE_CODED_SLICE_NON_IDR_PICTURE_1.There are more TODOs in the AVC Functions library, which need to be resolved before or during the porting to Rust.
This PR will help in debugging and resolving the AVC Issues currently prevalent in CCExtractor.( #1626 , #1597 , #1592 )