Skip to content

Conversation

@steel-bucket
Copy link
Contributor

@steel-bucket steel-bucket commented Jul 21, 2025

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 starts the migration of the TS module with the Migration of ts_tables.c.
Preview PR
Dependant on #1713

@ccextractor-bot
Copy link
Collaborator

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...:
Report Name Tests Passed
Broken 12/13
CEA-708 13/14
DVB 7/7
DVD 0/3
DVR-MS 2/2
General 22/27
Hauppage 3/3
MP4 3/3
NoCC 10/10
Options 81/86
Teletext 21/21
WTV 13/13
XDS 33/34

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-bot
Copy link
Collaborator

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...:
Report Name Tests Passed
Broken 11/13
CEA-708 13/14
DVB 2/7
DVD 0/3
DVR-MS 2/2
General 17/27
Hauppage 1/3
MP4 3/3
NoCC 10/10
Options 78/86
Teletext 4/21
WTV 10/13
XDS 31/34

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.

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.

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_RUST guards

Issues Found

  • Has merge conflicts - needs rebase
  • Causes regression: --myth test 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:

  1. find_next_start_code - header_state handling differs from C
  2. PTS/DTS extraction in get_pts_mythtv
  3. Stream type detection and codec ID assignment
  4. 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

  1. Fix and merge #1711 (GXF) - needs work on crashes
  2. Merge #1712 (MXF)
  3. Merge #1713 (MythTV) - all CI passes
  4. Rebase this PR, fix --myth regression
  5. Continue with #1720, #1724

Questions

  1. Would you be willing to work on fixing the --myth regression after dependencies are merged?
  2. 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.

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.

3 participants