-
Notifications
You must be signed in to change notification settings - Fork 465
[FIX] The first TODO in avc_functions.c and FIXME in avc_functions.c #1695
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
Open
steel-bucket
wants to merge
3
commits into
CCExtractor:master
Choose a base branch
from
steel-bucket:avc-todo
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -95,15 +95,19 @@ void do_NAL(struct encoder_ctx *enc_ctx, struct lib_cc_decode *dec_ctx, unsigned | |||||||||||||||||||
| NAL_stop = NAL_length + NAL_start; | ||||||||||||||||||||
| NAL_stop = remove_03emu(NAL_start + 1, NAL_stop); // Add +1 to NAL_stop for TS, without it for MP4. Still don't know why | ||||||||||||||||||||
|
|
||||||||||||||||||||
| 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); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if (NAL_stop == NULL) // remove_03emu failed. | ||||||||||||||||||||
| { | ||||||||||||||||||||
| mprint("\rNotice: NAL of type %u had to be skipped because remove_03emu failed.\n", nal_unit_type); | ||||||||||||||||||||
| 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", | ||||||||||||||||||||
|
Comment on lines
+100
to
+103
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.'
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||||
| nal_unit_type, nal_unit_type); | ||||||||||||||||||||
| 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); | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
Comment on lines
105
to
+110
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||||
| if (nal_unit_type == CCX_NAL_TYPE_ACCESS_UNIT_DELIMITER_9) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| // Found Access Unit Delimiter | ||||||||||||||||||||
|
|
@@ -127,7 +131,7 @@ void do_NAL(struct encoder_ctx *enc_ctx, struct lib_cc_decode *dec_ctx, unsigned | |||||||||||||||||||
| else if (dec_ctx->avc_ctx->got_seq_para && nal_unit_type == CCX_NAL_TYPE_SEI) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| // Found SEI (used for subtitles) | ||||||||||||||||||||
| // set_fts(ctx->timing); // FIXME - check this!!! | ||||||||||||||||||||
| set_fts(enc_ctx->timing); | ||||||||||||||||||||
| sei_rbsp(dec_ctx->avc_ctx, NAL_start + 1, NAL_stop); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| else if (dec_ctx->avc_ctx->got_seq_para && nal_unit_type == CCX_NAL_TYPE_PICTURE_PARAMETER_SET) | ||||||||||||||||||||
|
|
@@ -301,7 +305,7 @@ u32 avc_remove_emulation_bytes(const unsigned char *buffer_src, unsigned char *b | |||||||||||||||||||
| unsigned char *remove_03emu(unsigned char *from, unsigned char *to) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| int num = to - from; | ||||||||||||||||||||
| int newsize = EBSPtoRBSP(from, num, 0); // TODO: Do something if newsize == -1 (broken NAL) | ||||||||||||||||||||
| int newsize = EBSPtoRBSP(from, num, 0); | ||||||||||||||||||||
| if (newsize == -1) | ||||||||||||||||||||
| return NULL; | ||||||||||||||||||||
| return from + newsize; | ||||||||||||||||||||
|
|
||||||||||||||||||||
Oops, something went wrong.
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.
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.
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.
Copilot uses AI. Check for mistakes.
Uh oh!
There was an error while loading. Please reload this page.
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.
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