[QUESTION] Clarify VCL HRD parameters TODO in avc_functions.c:907 #862

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:

While exploring the codebase for GSoC 2026 preparation, I found a TODO comment that needs clarification:

Location: src/lib_ccx/avc_functions.c:907

Current code:

if (tmp)
{
    // TODO.
    mprint("vcl_hrd. Not implemented for now. Hopefully not needed. Skipping rest of NAL\n");
    ctx->num_vcl_hrd++;
    // exit(1);
}

Context:
This code handles VCL (Video Coding Layer) HRD parameters in H.264/AVC video processing. Currently:

The functionality is deliberately skipped with a warning message
A counter (ctx->num_vcl_hrd++) tracks occurrences
The exit(1) was commented out, suggesting the skip is acceptable

Questions:

Is VCL HRD parameter support still needed, or has this proven unnecessary in practice?
Should the TODO be:
- Removed if VCL HRD support isn't needed?
- Expanded with details if implementation is still desired?
- Converted to an issue if it's a substantial feature?
Does the num_vcl_hrd counter show this case occurs frequently in real-world files?

Proposed action:
Based on guidance:

If not needed: Remove TODO, improve the warning message
If needed: Create detailed specification of what needs implementation
If rare: Document that it's an edge case being tracked

Background:
Working toward GSoC 2026 with CCExtractor. First PR (#1889) merged. Focusing on video codec processing and decoder improvements for the CCExtractor 1.00 project.

Happy to submit a PR once I understand the preferred approach!

Originally created by @Harshdhall01 on GitHub (Dec 25, 2025). Description: While exploring the codebase for GSoC 2026 preparation, I found a TODO comment that needs clarification: Location: `src/lib_ccx/avc_functions.c:907` Current code: ```c if (tmp) { // TODO. mprint("vcl_hrd. Not implemented for now. Hopefully not needed. Skipping rest of NAL\n"); ctx->num_vcl_hrd++; // exit(1); } ``` Context: This code handles VCL (Video Coding Layer) HRD parameters in H.264/AVC video processing. Currently: - The functionality is deliberately skipped with a warning message - A counter (`ctx->num_vcl_hrd++`) tracks occurrences - The `exit(1)` was commented out, suggesting the skip is acceptable Questions: 1. Is VCL HRD parameter support still needed, or has this proven unnecessary in practice? 2. Should the TODO be: - **Removed** if VCL HRD support isn't needed? - **Expanded** with details if implementation is still desired? - **Converted to an issue** if it's a substantial feature? 3. Does the `num_vcl_hrd` counter show this case occurs frequently in real-world files? **Proposed action:** Based on guidance: - If not needed: Remove TODO, improve the warning message - If needed: Create detailed specification of what needs implementation - If rare: Document that it's an edge case being tracked Background: Working toward GSoC 2026 with CCExtractor. First PR (#1889) merged. Focusing on video codec processing and decoder improvements for the CCExtractor 1.00 project. Happy to submit a PR once I understand the preferred approach!

claunia closed this issue

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

claunia commented

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

@cfsmp3 commented on GitHub (Dec 26, 2025):

Great question, and thanks for diving into the codebase for GSoC 2026 prep!

Answer to your questions:

VCL HRD support is NOT needed for CCExtractor's purpose. CCExtractor extracts closed captions from SEI (Supplemental Enhancement Information) NAL units. VCL HRD parameters are about video buffering model compliance for decoders - completely unrelated to caption extraction.
The TODO can be cleaned up. The current code correctly skips these parameters. The message "Hopefully not needed" has proven true - we've never needed VCL HRD parsing for caption extraction.
The counter (num_vcl_hrd) is useful for tracking if this case occurs in the wild, but even when it does, it doesn't affect caption extraction.

Suggested fix:

Replace the TODO with a clear comment explaining why we skip VCL HRD
Keep the counter for diagnostics
Remove the commented-out exit(1)

Something like:

if (tmp)
{
    // VCL HRD parameters are for video buffering compliance, not needed for caption extraction.
    // Just skip and continue - this doesn't affect our ability to extract captions.
    mprint("Skipping VCL HRD parameters (not needed for caption extraction)\n");
    ctx->num_vcl_hrd++;
}

Feel free to submit a PR with this cleanup! It's a good first contribution.

@cfsmp3 commented on GitHub (Dec 26, 2025): Great question, and thanks for diving into the codebase for GSoC 2026 prep! **Answer to your questions:** 1. **VCL HRD support is NOT needed** for CCExtractor's purpose. CCExtractor extracts closed captions from SEI (Supplemental Enhancement Information) NAL units. VCL HRD parameters are about video buffering model compliance for decoders - completely unrelated to caption extraction. 2. **The TODO can be cleaned up.** The current code correctly skips these parameters. The message "Hopefully not needed" has proven true - we've never needed VCL HRD parsing for caption extraction. 3. **The counter (`num_vcl_hrd`)** is useful for tracking if this case occurs in the wild, but even when it does, it doesn't affect caption extraction. **Suggested fix:** - Replace the TODO with a clear comment explaining why we skip VCL HRD - Keep the counter for diagnostics - Remove the commented-out `exit(1)` Something like: ```c if (tmp) { // VCL HRD parameters are for video buffering compliance, not needed for caption extraction. // Just skip and continue - this doesn't affect our ability to extract captions. mprint("Skipping VCL HRD parameters (not needed for caption extraction)\n"); ctx->num_vcl_hrd++; } ``` Feel free to submit a PR with this cleanup! It's a good first contribution.

claunia referenced this issue

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

[PR #862] [CLOSED] README FILE #1692

claunia referenced this issue

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

[PR #862] README FILE #1697

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: starred/ccextractor#862