[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.
Are you really re-allocating it or just wiping existing one and creating new?
My point being data might get lost, please confirm.
| } | ||
| 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.
You should not continue the function further if DemuxerError::EOF comes.
|
|
||
| // 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.
By using this it will convert invalid UTF-8 characters to U+FFFD, whilst C preserves the characters.
cfsmp3
left a comment
There was a problem hiding this comment.
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"
|
Closed due to inactivity, feel free to submit a new one (make sure it's based off master) when you are ready to resume work. |
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.