Skip to content

[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.

Sign up for GitHub

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
wants to merge 5 commits into
base: master
Choose a base branch
from
+10 −6
Open

[FIX] The first TODO in avc_functions.c and FIXME in avc_functions.c #1695

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dafe728
fix: The first TODO in avc_functions.c and FIXME in avc_functions.c
steel-bucket Apr 26, 2025
d1f2dbb
fix: AVC - Update CHANGES.txt
steel-bucket Apr 26, 2025
b2353f8
fix: Clippy fix for encoder
steel-bucket Aug 10, 2025
42e70ab
Merge branch 'master' into avc-todo
steel-bucket Sep 7, 2025
28ab492
Merge branch 'CCExtractor:master' into avc-todo
steel-bucket Sep 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions src/lib_ccx/avc_functions.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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). "
Comment on lines +101 to +102
Copy link

Copilot AI Jul 15, 2025

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.

Suggested change
"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.
Copy link
Contributor Author

@steel-bucket steel-bucket Jul 15, 2025 •
edited
Loading

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

"This may indicate a corrupted AVC/H.264 stream. NAL unit skipped.\n",
Comment on lines +100 to +103
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

The 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
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.
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 104 to +110
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

The 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
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.
if (nal_unit_type == CCX_NAL_TYPE_ACCESS_UNIT_DELIMITER_9)
{
// Found Access Unit Delimiter
Expand All @@ -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)
Expand Down Expand Up @@ -308,7 +312,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;
Expand Down
Loading