[ENHANCEMENT] Handle idr_pic_id in AVC slice header parsing #863

New Issue

claunia · 2026-01-29T16:55:37Z

claunia commented

2026-01-29 16:55:37 +00:00

Originally created by @Harshdhall01 on GitHub (Dec 25, 2025).

Description:

Found a TODO in the AVC slice header parsing that suggests incomplete handling of IDR picture IDs:

Location:src/lib_ccx/avc_functions.c:998

Current code:

if (nal_unit_type == 5)
{
    tmp = read_exp_golomb_unsigned(&q1);
    dvprint("idr_pic_id=            % 4lld (%#llX)\n", tmp, tmp);
    // TODO
}

Issue:
The IDR (Instantaneous Decoder Refresh) picture ID is:

✅ Successfully read from the bitstream
✅ Printed for debugging (dvprint)
❌ Not stored in any context structure
❌ Not used for any further processing

Potential improvements:

Store the value in an appropriate context structure (e.g., dec_ctx->avc_ctx->idr_pic_id)
Use the value for frame identification or ordering if needed
Document why it's read but not used (if intentional)

Questions for maintainers:

Should idr_pic_id be stored in the AVC context?
Is this value needed for caption extraction, or is reading it sufficient?
Are there cases where this ID would be useful for timing or frame ordering?

Research:
According to H.264 spec, idr_pic_id identifies IDR pictures and can range from 0-65535. It's used to identify different IDR pictures when multiple occur in sequence.

Proposed solution:

Add idr_pic_id field to the appropriate context structure
Store the value when read
Add comment explaining its purpose
(Optional) Expose it in debug output or logging

Background:
Working toward GSoC 2026 with CCExtractor. First PR (#1889) merged. Focusing on core video processing and subtitle extraction for the CCExtractor 1.00 release.

Would love to implement this if you can guide me on whether/how the ID should be used!

Originally created by @Harshdhall01 on GitHub (Dec 25, 2025). Description: Found a TODO in the AVC slice header parsing that suggests incomplete handling of IDR picture IDs: Location:`src/lib_ccx/avc_functions.c:998` Current code: ```c if (nal_unit_type == 5) { tmp = read_exp_golomb_unsigned(&q1); dvprint("idr_pic_id= % 4lld (%#llX)\n", tmp, tmp); // TODO } ``` Issue: The IDR (Instantaneous Decoder Refresh) picture ID is: - ✅ Successfully read from the bitstream - ✅ Printed for debugging (`dvprint`) - ❌ Not stored in any context structure - ❌ Not used for any further processing Potential improvements: 1. **Store the value** in an appropriate context structure (e.g., `dec_ctx->avc_ctx->idr_pic_id`) 2. **Use the value** for frame identification or ordering if needed 3. **Document why** it's read but not used (if intentional) Questions for maintainers: 1. Should `idr_pic_id` be stored in the AVC context? 2. Is this value needed for caption extraction, or is reading it sufficient? 3. Are there cases where this ID would be useful for timing or frame ordering? Research: According to H.264 spec, `idr_pic_id` identifies IDR pictures and can range from 0-65535. It's used to identify different IDR pictures when multiple occur in sequence. Proposed solution: 1. Add `idr_pic_id` field to the appropriate context structure 2. Store the value when read 3. Add comment explaining its purpose 4. (Optional) Expose it in debug output or logging Background: Working toward GSoC 2026 with CCExtractor. First PR (#1889) merged. Focusing on core video processing and subtitle extraction for the CCExtractor 1.00 release. Would love to implement this if you can guide me on whether/how the ID should be used!

claunia closed this issue

2026-01-29 16:55:38 +00:00

claunia commented

2026-01-29 16:55:38 +00:00

@cfsmp3 commented on GitHub (Jan 1, 2026):

Thanks for the thorough analysis! After reviewing the code, the TODO can simply be removed - no additional implementation is needed.

Why idr_pic_id doesn't need to be stored:

Bitstream parsing: The value must be read to advance the bitstream position correctly. The H.264 slice header has a specific structure, and we need to parse past idr_pic_id to reach subsequent fields like pic_order_cnt_lsb.
Not needed for caption extraction: CCExtractor extracts closed captions from SEI NAL units. For timing and frame ordering, we use:
- pic_order_cnt_lsb (when --usepicorder is set)
- PTS timestamps
Neither of these requires idr_pic_id.
Purpose in H.264 spec: idr_pic_id is used by decoders for error recovery - to distinguish between consecutive IDR pictures when recovering from stream errors. This is a decoder concern, not relevant for caption extraction.

The TODO was likely a "maybe we'll need this later" placeholder that never materialized. I've created PR #1947 to remove the TODO and add a clarifying comment explaining why the value is read but not stored.

@cfsmp3 commented on GitHub (Jan 1, 2026): Thanks for the thorough analysis! After reviewing the code, the TODO can simply be removed - no additional implementation is needed. **Why `idr_pic_id` doesn't need to be stored:** 1. **Bitstream parsing**: The value must be read to advance the bitstream position correctly. The H.264 slice header has a specific structure, and we need to parse past `idr_pic_id` to reach subsequent fields like `pic_order_cnt_lsb`. 2. **Not needed for caption extraction**: CCExtractor extracts closed captions from SEI NAL units. For timing and frame ordering, we use: - `pic_order_cnt_lsb` (when `--usepicorder` is set) - PTS timestamps Neither of these requires `idr_pic_id`. 3. **Purpose in H.264 spec**: `idr_pic_id` is used by decoders for error recovery - to distinguish between consecutive IDR pictures when recovering from stream errors. This is a decoder concern, not relevant for caption extraction. The TODO was likely a "maybe we'll need this later" placeholder that never materialized. I've created PR #1947 to remove the TODO and add a clarifying comment explaining why the value is read but not stored.

claunia referenced this issue

2026-01-29 17:18:01 +00:00

[PR #863] [CLOSED] [IMPROVEMENT] ccx_encoders_common.c #1695

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: starred/ccextractor#863