allow apm >=10ms frames #843
Conversation
📝 WalkthroughWalkthroughAudio processing in the APM module was refactored to handle input data in consecutive 10ms chunks instead of processing it all at once. Input validation was added to ensure data length is a non-empty multiple of the chunk size. Error handling and overall success behavior remain unchanged. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
✏️ Tip: You can disable this entire section by setting Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libwebrtc/src/native/apm.rs`:
- Around line 49-54: Guard against zero sample_rate or num_channels before
computing samples_per_10ms and doing the modulo: add an early check (e.g.,
assert!(sample_rate > 0 && num_channels > 0, "sample_rate and num_channels must
be > 0") or return an Err) using the existing variables sample_rate and
num_channels, then compute samples_per_10ms and keep the existing assertion on
data.len() % samples_per_10ms; this prevents a division-by-zero when evaluating
samples_per_10ms or the modulo operation.
🧹 Nitpick comments (1)
libwebrtc/src/native/apm.rs (1)
86-91: Duplicate validation logic.This validation is identical to
process_stream. Consider extracting the chunk size calculation and validation into a helper function or macro to reduce duplication and ensure consistency.Example helper extraction
fn samples_per_10ms_chunk(sample_rate: i32, num_channels: i32) -> usize { assert!(sample_rate > 0 && num_channels > 0, "sample_rate and num_channels must be positive"); (sample_rate as usize / 100) * num_channels as usize } fn validate_chunk_alignment(data_len: usize, chunk_size: usize) { assert!( data_len % chunk_size == 0 && data_len >= chunk_size, "slice must have a multiple of 10ms worth of samples" ); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libwebrtc/src/native/apm.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Build (aarch64-apple-ios)
- GitHub Check: Build (armv7-linux-androideabi)
- GitHub Check: Build (x86_64-apple-darwin)
- GitHub Check: Build (x86_64-linux-android)
- GitHub Check: Build (x86_64-unknown-linux-gnu)
- GitHub Check: Build (aarch64-pc-windows-msvc)
- GitHub Check: Build (aarch64-unknown-linux-gnu)
- GitHub Check: Build (aarch64-linux-android)
- GitHub Check: Build (aarch64-apple-ios-sim)
- GitHub Check: Build (x86_64-pc-windows-msvc)
- GitHub Check: Build (aarch64-apple-darwin)
- GitHub Check: Test (x86_64-unknown-linux-gnu)
- GitHub Check: Test (x86_64-apple-darwin)
- GitHub Check: Test (x86_64-pc-windows-msvc)
🔇 Additional comments (2)
libwebrtc/src/native/apm.rs (2)
56-75: Partial buffer modification on mid-processing failure.If processing fails on a later chunk, earlier chunks will have already been modified in-place. Callers should be aware that on error, the
databuffer may be in a partially processed state. Verify this is acceptable for all call sites, or consider processing into a temporary buffer and copying back on success if atomicity is required.
93-114: LGTM!The chunked processing loop correctly iterates over 10ms segments and handles errors appropriately. Same partial-modification caveat as
process_streamapplies.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
libwebrtc/src/native/apm.rs
Outdated
| // 10ms worth of samples per chunk | ||
| let samples_per_10ms = (sample_rate as usize / 100) * num_channels as usize; | ||
| assert!( | ||
| data.len() % samples_per_10ms == 0 && data.len() >= samples_per_10ms, | ||
| "slice must have a multiple of 10ms worth of samples" | ||
| ); |
There was a problem hiding this comment.
Potential division-by-zero before assertion.
If sample_rate or num_channels is zero, samples_per_10ms will be zero, causing a panic on the modulo operation before the assertion message is shown. Consider adding a guard or separate assertion for these parameters if they could ever be invalid.
Suggested defensive check
// 10ms worth of samples per chunk
+ assert!(sample_rate > 0 && num_channels > 0, "sample_rate and num_channels must be positive");
let samples_per_10ms = (sample_rate as usize / 100) * num_channels as usize;
assert!(
data.len() % samples_per_10ms == 0 && data.len() >= samples_per_10ms,
"slice must have a multiple of 10ms worth of samples"
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 10ms worth of samples per chunk | |
| let samples_per_10ms = (sample_rate as usize / 100) * num_channels as usize; | |
| assert!( | |
| data.len() % samples_per_10ms == 0 && data.len() >= samples_per_10ms, | |
| "slice must have a multiple of 10ms worth of samples" | |
| ); | |
| // 10ms worth of samples per chunk | |
| assert!(sample_rate > 0 && num_channels > 0, "sample_rate and num_channels must be positive"); | |
| let samples_per_10ms = (sample_rate as usize / 100) * num_channels as usize; | |
| assert!( | |
| data.len() % samples_per_10ms == 0 && data.len() >= samples_per_10ms, | |
| "slice must have a multiple of 10ms worth of samples" | |
| ); |
🤖 Prompt for AI Agents
In `@libwebrtc/src/native/apm.rs` around lines 49 - 54, Guard against zero
sample_rate or num_channels before computing samples_per_10ms and doing the
modulo: add an early check (e.g., assert!(sample_rate > 0 && num_channels > 0,
"sample_rate and num_channels must be > 0") or return an Err) using the existing
variables sample_rate and num_channels, then compute samples_per_10ms and keep
the existing assertion on data.len() % samples_per_10ms; this prevents a
division-by-zero when evaluating samples_per_10ms or the modulo operation.
allow to reduce overhead of protocol buf for tiny frames
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.