aac: refactor approximate frame count, fuzzy fixes #343
Conversation
symphonia-codec-aac/src/adts.rs
Outdated
| if source.seek(SeekFrom::Start(cur_pos)).is_err() { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Nice catch for an optimization! However, I think ignore_bytes will work even better since the MSS will decide if it should just consume from it's internal ring buffer or resort to an actual seek. In other words, you can just ignore_bytes(header.frame_len).
symphonia-codec-aac/src/adts.rs
Outdated
| @@ -428,34 +428,48 @@ fn approximate_frame_count(mut source: &mut MediaSourceStream<'_>) -> Result<Opt | |||
| else { | |||
| // The number of points to sample within the stream. | |||
| const NUM_SAMPLE_POINTS: u64 = 4; | |||
| const NUM_FRAMES: u32 = 100; | |||
There was a problem hiding this comment.
A slightly more descriptive name.
| const NUM_FRAMES: u32 = 100; | |
| const NUM_FRAMES_PER_SAMPLE: u32 = 100; |
|
Thanks for suggestion. |
symphonia-codec-aac/src/adts.rs
Outdated
| // Reading frames from the next sample point could lead to reprocessing already calculated frames. | ||
| // Also, if no sync word was found, better to avoid redundant searches. | ||
| if next_frame_pos > new_pos + step { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Suggestion: Remove this since it wouldn't be required anymore.
symphonia-codec-aac/src/adts.rs
Outdated
| next_frame_pos = source.pos() + header.payload_len() as u64; | ||
|
|
||
| for _ in 0..=100 { | ||
| let header = match AdtsHeader::read(&mut source) { | ||
| Ok(header) => header, | ||
| _ => break, | ||
| }; | ||
| // if reading NUM_FRAMES_PER_SAMPLE_POINT frames overflow the next sample point position then break | ||
| if next_frame_pos > new_pos + step { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Suggestion: Remove this since it wouldn't be required anymore.
| // file can be small enough and not have enough NUM_FRAMES_PER_SAMPLE_POINT, but we can still read at least one frame | ||
| if step > 0 { | ||
| for new_pos in (original_pos..(original_pos + remaining_len)).step_by(step as usize) { | ||
| if source.seek(SeekFrom::Start(new_pos)).is_err() { |
There was a problem hiding this comment.
Suggestion: Add a test here before the seek to make sure we never go backwards or continue from the current position.
// Skip the current sample point if a previous sample read past it.
if new_pos <= source.pos() {
continue;
}
I added some additional suggestions that could help clean-up the implementation after these changes. Let me know what you think. |
|
Thanks, merged! |
refactored aac aproximate_frame_count() method. It had some issues which causes panic. Should fix cases from https://github.com/qarmin/Automated-Fuzzer/actions/runs/12516290494 (REPORTS___SYMPHONIA*)
I haven't tested on VBR aac to observe any improvements, It was #196 (comment) approximate method is not perfect there