-
Notifications
You must be signed in to change notification settings - Fork 505
[Rust]Added Transport Stream Module #1717
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?
[Rust]Added Transport Stream Module #1717
Conversation
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit 81fdecd...:
Your PR breaks these cases:
NOTE: The following tests have been failing on the master branch as well as the PR:
Congratulations: Merging this PR would fix the following tests:
It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). Check the result page for more info. |
CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results, when compared to test for commit 81fdecd...:
NOTE: The following tests have been failing on the master branch as well as the PR:
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. |
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.
Thank you for this comprehensive TS module migration! I've done a deep review and here's the status:
Good News
- ✅ Clean FFI architecture with proper type sharing
- ✅ Comprehensive unit tests for MythTV parsing (
get_pts_mythtv,find_next_start_code, buffer handling) - ✅ Would FIX ~24 currently failing tests on Linux
- ✅ Bug fix for MXF debug message (
klv.key[5]duplicated →klv.key[6]) - ✅ Preserves build flexibility with
#ifndef DISABLE_RUSTguards
Issues Found
- ❌ Has merge conflicts - needs rebase
- ❌ Causes regression:
--mythtest on sample 121 (c83f765c66...) fails - ❌ Windows shows more failures than Linux - platform-specific issues in Rust implementation
- ❌ Blocked by dependency chain (#1711 → #1712 → #1713)
Regression Analysis
The --myth regression is in the Rust mpegps_read_packet implementation. Potential areas to investigate:
find_next_start_code- header_state handling differs from C- PTS/DTS extraction in
get_pts_mythtv - Stream type detection and codec ID assignment
- FFI boundary data copying in
copy_av_packet_from_rust_to_c
Dependency Chain Status
All PRs in the chain currently have merge conflicts:
| PR | Status |
|---|---|
| #1711 (GXF) | CONFLICTING - has crashes and review comments |
| #1712 (MXF) | CONFLICTING - blocked by #1711, but code looks good |
| #1713 (MythTV) | CONFLICTING - all CI passes! Best candidate |
| #1717 (this PR) | CONFLICTING - breaks --myth test |
| #1720, #1724 | CONFLICTING - preview PRs |
Recommended Merge Order
- Fix and merge #1711 (GXF) - needs work on crashes
- Merge #1712 (MXF)
- Merge #1713 (MythTV) - all CI passes
- Rebase this PR, fix
--mythregression - Continue with #1720, #1724
Questions
- Would you be willing to work on fixing the
--mythregression after dependencies are merged? - Can you investigate why Windows shows more failures than Linux?
This is a preview PR as noted in the description, so the above issues are expected to be addressed once the dependency chain is resolved.
In raising this pull request, I confirm the following (please check boxes):
My familiarity with the project is as follows (check one):
This PR starts the migration of the TS module with the Migration of
ts_tables.c.Preview PR
Dependant on #1713