-
Notifications
You must be signed in to change notification settings - Fork 507
[FIX]: Prevent CEA-708 decoder panic with -stdout on Windows #1880
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
base: master
Are you sure you want to change the base?
[FIX]: Prevent CEA-708 decoder panic with -stdout on Windows #1880
Conversation
cfsmp3
left a comment
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.
Review Summary
Thanks for this fix! The code changes look good and testing confirms the fix works correctly.
✅ What's Good
- Proper use of
as_ref()for null-pointer safety - Error handling with
if let Err(e)instead of.unwrap()panic - Consistent fix in both
screen_print()andflush()methods - All 299 Rust tests pass
- Tested with CEA-708 and CEA-608 samples - no regressions
📋 Required Before Merge
-
Rebase on master - Your branch is 12 commits behind. Run:
git fetch origin git rebase origin/master
-
Update CHANGES.TXT - After rebase, move your changelog entry to the
0.96section (version was bumped while your PR was open)
Once rebased, this is ready to merge! 🎉
… disabled (observed with -stdout on Windows)
7a9f87d to
14aa72c
Compare
|
Thanks for the review. I’ve rebased the branch on |
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit d573548...:
Congratulations: Merging this PR would fix the following tests:
All tests passing on the master branch were passed completely. Check the result page for more info. |
In raising this pull request, I confirm the following (please check boxes):
My familiarity with the project is as follows (check one):
Fix #1478: Prevent CEA-708 service decoder panic when using
-stdouton WindowsIssue Summary
Issue #1478 reports a panic in CCExtractor 0.94 when using
-stdoutwith HDHomeRun-recorded.tsfiles on Windows:Original crash reported:
Reproduction
I reproduced a similar panic on Windows 0.94 portable build using
-stdoutand sample file from #1478:My reproduction:
Root Cause Analysis
Although the panics are reported at different line numbers across builds, they stem from the same underlying issue in the CEA-708 service decoder.
On Windows, when running with
-stdout, transcript output is effectively disabled or not fully initialized (no output filename, different writer setup). However, the decoder still executes code paths that assume a fully configured transcript writer.This results in panics in different places depending on build and execution path:
Failed
Writer::new()being unconditionally used (e.g. “Filename missing”)Accessing a non-existent writer context (index out of bounds)
While the exact panic location varies, both failures indicate that writer-related state is accessed when transcript output is not valid.
This fix adds an explicit check for
transcript_settingsand exits early when transcript output is disabled, preventing invalid writer usage. Behavior when transcript output is enabled remains unchanged.Changes Made
Modified
src/rust/src/decoder/service_decoder.rsto add defensive checks in two methods:1.
screen_print()method:transcript_settingsexists before attempting Writer creation.unwrap()call that caused the panic2.
flush()method:transcript_settingsTesting
Cross-platform tested:
Request for Windows testing
Why This Fix Should Work
Even though the panic line numbers differ between the original report (275) and my reproduction (897), this fix addresses what appears to be the root cause based on available evidence:
The Fix Strategy:
Before any Writer operations, check if transcript output is properly configured
If not configured, skip transcript output gracefully instead of attempting to use an uninitialized Writer
This should prevent panics at any point where Writer is accessed (creation, usage, or cleanup)
Even if this doesn't address the exact root cause of the line 275 panic (due to build differences or additional factors), the fix provides important safety guarantees:
Prevents panics from null/missing transcript_settings
Fails gracefully instead of crashing
At minimum, this fix makes the codebase more robust against Writer initialization issues on Windows.