Audio fixes #1238
Audio fixes #1238
Conversation
Enhanced the audio mixer to handle zero-duration buffer gaps by inserting a minimal silence duration, and switched print statements to debug logging. Also improved debug assertions in cpal.rs for better clarity and formatting.
WalkthroughConsolidates audio mixer gap detection and silence insertion using a dynamic min_gap and structured debug logging; moves silence frame construction into the guarded branch and updates timestamps/state accordingly. Adds bounds-checked copying for planar and non-planar CPAL buffers to avoid out-of-bounds writes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Mixer as AudioMixer
participant Source as SourceBuffer
participant FrameQ as FrameQueue
Note over Mixer,Source: Incoming buffer processed
Mixer->>Source: read buffer_last_timestamp, buffer_last_duration
Mixer->>Mixer: compute diff = now - (buffer_last_timestamp + buffer_last_duration)
alt diff >= min_gap
Mixer->>Mixer: compute silence_samples_count from gap
Mixer->>Mixer: build silence_frame (fill samples, set rate)
Mixer->>Source: update buffer_last = (timestamp, silence_duration)
Mixer->>FrameQ: push silence_frame
Note right of FrameQ: debug log gap insertion
else diff < min_gap
Note right of Mixer: no silence inserted
end
Mixer->>FrameQ: continue with normal downstream frames
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🧹 Nitpick comments (2)
crates/scap-ffmpeg/src/cpal.rs (2)
29-45: Consider logging when audio truncation occurs.The bounds checking prevents crashes, which is good defensive programming. However, when
copy_len < src.len(), audio data is silently truncated, which could indicate an upstream logic error and lead to audio glitches.Consider adding a warning log when truncation occurs:
let copy_len = dst.len().min(src.len()); +if copy_len < src.len() { + tracing::warn!( + "Audio data truncated: channel={}, src_len={}, dst_len={}", + i, src.len(), dst.len() + ); +} dst[..copy_len].copy_from_slice(&src[..copy_len]);
47-54: Consider logging when audio truncation occurs.Same concern as the planar case: silent truncation could mask upstream bugs and cause audio quality issues.
Add warning log when truncation occurs:
let copy_len = dst.len().min(bytes.len()); +if copy_len < bytes.len() { + tracing::warn!( + "Audio data truncated: src_len={}, dst_len={}", + bytes.len(), dst.len() + ); +} dst[..copy_len].copy_from_slice(&bytes[..copy_len]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/recording/src/sources/audio_mixer.rs(1 hunks)crates/scap-ffmpeg/src/cpal.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
crates/scap-ffmpeg/src/cpal.rscrates/recording/src/sources/audio_mixer.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/scap-ffmpeg/src/cpal.rscrates/recording/src/sources/audio_mixer.rs
🧬 Code graph analysis (2)
crates/scap-ffmpeg/src/cpal.rs (3)
crates/camera-windows/src/lib.rs (1)
bytes(188-199)crates/camera-mediafoundation/src/lib.rs (2)
bytes(468-483)len(69-71)crates/media-info/src/lib.rs (1)
sample_size(121-123)
crates/recording/src/sources/audio_mixer.rs (1)
crates/media-info/src/lib.rs (2)
rate(125-127)new(29-45)
⏰ 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). (4)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
crates/scap-ffmpeg/src/cpal.rs (1)
13-14: LGTM: Efficient local variable usage.Storing the bytes result in a local variable avoids redundant method calls and ensures consistency across the function.
crates/recording/src/sources/audio_mixer.rs (2)
286-290: Verify the min_gap threshold is appropriate.The 1 microsecond threshold when
buffer_last_duration.is_zero()is extremely small and may trigger silence insertion for normal timing jitter. Usingbuffer_last_durationas the threshold when non-zero means gaps smaller than the last buffer are ignored, which could miss legitimate small gaps.Consider whether a fixed minimum threshold (e.g., 1-5ms) would be more appropriate than 1 microsecond to avoid spurious silence insertion from timing jitter.
295-295: LGTM: Improved debugging with structured logging.The structured debug logging is a good improvement over print-based debugging and will help diagnose audio gap issues.
This pull request focuses on improving the robustness and correctness of audio buffer handling in both the audio mixing and FFmpeg interfacing code. The main changes address edge cases in buffer gap detection and ensure safe memory operations when copying audio data between buffers.
This fixes recording with certain microphones on Windows, preventing a crash from occurring.
Summary by CodeRabbit
Bug Fixes
Chores