fix SEI payload size handling and type correctness #2040
Conversation
cfsmp3
left a comment
cfsmp3
left a comment
There was a problem hiding this comment.
Thanks for working on this! The intent to clean up the type handling is good, but the implementation uses the wrong type.
Issue: usize is platform-dependent
usize varies by platform (32-bit on 32-bit systems, 64-bit on 64-bit systems). Our coding guidelines require fixed-width types for data values:
Use fixed-width types (
int64_t,uint32_t) over platform-dependent types (size_t,long)
Recommended fix
Use u32 instead of usize:
let mut payload_type: u32 = 0;
// ...
payload_type += seibuf[seibuf_idx] as u32;
let mut payload_size: u32 = 0;
// ...
payload_size += seibuf[seibuf_idx] as u32;Keep the as usize casts where needed for indexing/slicing - those are correct and explicit:
seibuf_idx += payload_size as usize;
// ...
if !broken && payload_type == 4 && payload_start + payload_size as usize <= seibuf.len() {
let payload_data = &seibuf[payload_start..payload_start + payload_size as usize];Why this matters
payload_typeis never used for indexing - it's only compared against constants and printed.u32works directly.payload_sizeneedsusizeonly for indexing operations, and the cast is cheap (just a register widen on 64-bit).- The original
i32was close to correct - it just used signed when unsigned is more appropriate. The fix should bei32→u32, noti32→usize.
The goal of removing casts is less important than using the correct fixed-width type.
|
@cfsmp3 |
|
@cfsmp3 |
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit d494286...:
Your PR breaks these cases:
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 d494286...:
Your PR breaks these cases:
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. |
3 participants
In raising this pull request, I confirm the following (please check boxes):
My familiarity with the project is as follows (check one):
Description
payload_typeandpayload_sizewere previously inferred or explicitly defined asi32, which required repeated casting when indexing buffers or comparing against slice lengths. This added unnecessary casts and could theoretically lead to incorrect behavior for large payload values.Fix
payload_typeandpayload_sizeto explicitusize.as i32casts during accumulation.as usizecasts during indexing and bounds checks.seibuf_idxarithmetic remains type-safe and consistent.