Skip to content

Fix/dtvcc allocation panic#2050

Open
pranavshar223 wants to merge 4 commits intoCCExtractor:masterfrom
pranavshar223:fix/dtvcc-allocation-panic
Open

Fix/dtvcc allocation panic#2050
pranavshar223 wants to merge 4 commits intoCCExtractor:masterfrom
pranavshar223:fix/dtvcc-allocation-panic

Conversation

@pranavshar223
Copy link

In raising this pull request, I confirm the following (please check boxes):

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.
  • I have mentioned this change in the changelog.

My familiarity with the project is as follows (check one):

  • I have never used CCExtractor.
  • I have used CCExtractor just a couple of times.
  • I absolutely love CCExtractor, but have not contributed previously.
  • I am an active contributor to CCExtractor.

This PR removes panics from the CEA-708 decoder initialization path.

Previously, allocation failures for dtvcc_tv_screen or
dtvcc_service_decoder would cause a hard panic, crashing CCExtractor.

Instead, allocation failures are now handled gracefully by logging
a debug message and disabling the affected service decoder while
allowing processing to continue.

This improves robustness and aligns Rust behavior with the legacy C
implementation.

Copy link
Contributor

@cfsmp3 cfsmp3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on improving the robustness of the CEA-708 decoder initialization!

The concept is good - handling allocation failures gracefully instead of panicking. However, there are several issues that need to be fixed before this can be merged:

1. CRLF Line Endings Breaking Linux Builds

All files in this PR have Windows-style CRLF line endings instead of Unix LF:

decoder/mod.rs: ASCII text, with CRLF line terminators
pre-build.sh: ASCII text, with CRLF line terminators

This breaks the Linux build - shell scripts with CRLF don't work on Unix systems. This is why the CI is failing:

  • build_shell - FAILURE
  • build_autoconf - FAILURE
  • format_rust - FAILURE

Fix: Configure your git to handle line endings properly:

git config --global core.autocrlf input

Then convert the files:

dos2unix src/rust/src/decoder/mod.rs linux/pre-build.sh

Or rebase your branch on a fresh master checkout.

2. Unrelated Changes from PR #2045

This PR includes changes from #2045 (userdata.rs documentation and debug logging). These should be in a separate PR - #2045 is already pending review.

Please remove src/rust/src/es/userdata.rs from this PR.

3. Messy Commit History

The commits include "retry the test" and "restore all changes" which don't describe the actual changes. Consider squashing into a clean commit with a descriptive message.

4. Minor: Trailing Whitespace

                    continue;
                }       // <-- extra spaces here

The Actual Fix Looks Good

Once the above issues are fixed, the core change is reasonable:

// Before: panic on allocation failure
panic!("Failed to allocate dtvcc_tv_screen");

// After: log and gracefully skip the service
debug!(msg_type = DebugMessageFlag::DECODER_708;
    "DTVCC: Failed to allocate tv_screen for service {}, disabling service", i + 1);
continue;

The cleanup of the already-allocated tv_screen when decoder allocation fails is also correct:

unsafe {
    std::alloc::dealloc(tv_ptr as *mut u8, tv_layout);
}

Summary

Please:

  1. Fix line endings (dos2unix or fresh checkout)
  2. Remove userdata.rs changes (belongs in #2045)
  3. Squash commits into clean history
  4. Push updated branch

Looking forward to the updated PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants