-
Notifications
You must be signed in to change notification settings - Fork 505
[Rust] Added GXF Module #1711
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 GXF Module #1711
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 099fa05...:
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. |
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. |
| ctx.cdp = match std::panic::catch_unwind(|| vec![0u8; (rem_len / 2) as usize]) { | ||
| Ok(buf) => Some(buf), | ||
| Err(_) => { | ||
| info!("Failed to allocate buffer {}\n", rem_len / 2); | ||
| return Err(DemuxerError::OutOfMemory); | ||
| } | ||
| }; |
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.
Are you really re-allocating it or just wiping existing one and creating new?
My point being data might get lost, please confirm.
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.
resolved
| } | ||
| if let Some(ad_track) = ctx.ad_track.as_mut() { | ||
| ad_track.id = track_id; | ||
| let _ = parse_ad_track_desc(demux, track_len, ccx_options); |
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.
You should not continue the function further if DemuxerError::EOF comes.
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.
resolved
src/rust/src/libccxr_exports/gxf.rs
Outdated
|
|
||
| // Convert to CStr and then to String | ||
| let slice = std::slice::from_raw_parts(ptr as *const u8, len); | ||
| String::from_utf8_lossy(slice).into_owned() |
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.
By using this it will convert invalid UTF-8 characters to U+FFFD, whilst C preserves the characters.
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.
resolved
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 working on the GXF Rust migration! The dependency (#1662) has been merged, so this should be rebased.
However, the sample platform test results are concerning:
Linux failures (~40 tests):
- Options: 26 failures
- XDS: 8 failures
- General: 4 crashes (exit code 139 = SIGSEGV)
Windows failures (~30 tests):
- Crashes with exit code -1073741819 (ACCESS_VIOLATION)
- Timeouts on WTV tests
- Various output mismatches
These crashes suggest memory safety issues, possibly at the FFI boundary.
Additionally, the three review comments from @prateekmedia need proper resolution:
- Memory allocation - Please confirm data isn't being lost during reallocation
- EOF handling - Verify functions stop properly on EOF
- UTF-8 lossy conversion -
String::from_utf8_lossy()replaces invalid bytes with U+FFFD, which changes behavior from C. Consider usingString::from_utf8_unchecked()or a byte buffer to preserve original bytes.
Before this can be merged:
- Rebase on master (now that #1662 is merged)
- Investigate and fix the crash causes
- Re-run sample platform tests to verify fixes
- Address the maintainer's review comments with explanation, not just "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 migrates
ccx_gxf.candccx_gxf.hDependant on #1662
PR Moved from My Fork to Here.