fix: prevent panic in AudioInfo::channel_layout() for unsupported channel counts #1536
Conversation
…nnel counts The channel_layout() method was calling .unwrap() on channel_layout_raw() which returns None for channel counts outside 1-8. This caused a panic when: - Audio devices reported 0 channels (edge case) - Audio devices reported more than 8 channels (e.g., 9-16 channel configurations) The crash occurred in the audio playback path: AudioPlayback::spawn -> AudioResampler::new -> output_info.channel_layout() Changes: - channel_layout() now clamps channels to 1-8 range and falls back to STEREO - MAX_AUDIO_CHANNELS reduced from 16 to 8 to match supported FFmpeg layouts - Added tests for edge cases (0 channels, >8 channels)
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (2)
-
crates/media-info/src/lib.rs, line 162-163 (link)logic:
unreachable!()is now reachable sincenew_rawallows 0 channelsWith the changes in this PR,
AudioInfo::new_raw()can now create instances with 0 channels (as shown in the test at line 419). If such an instance callswrap_frame_with_max_channels(), it will hit thisunreachable!()and panic.Either handle this case properly or add validation to prevent 0-channel
AudioInfofrom calling this method. -
crates/media-info/src/lib.rs, line 152-153 (link)logic: Division by zero when
self.channels == 0If
self.channelsis 0, thenpacked_sample_sizewill be 0, causing a division by zero panic on line 153. Sincenew_rawnow allows 0 channels, this is a reachable panic condition.
1 file reviewed, 2 comments
Address code review feedback: - Replace unreachable!() with proper handling by treating 0 channels as mono - Fix division by zero by using effective_channels (clamped to minimum 1) - Also clamp max_channels to minimum 1 for safety - Add test for wrap_frame with 0 channels
…aces) Problem: Cap crashed or produced chipmunk audio on systems with professional audio interfaces that report many channels (like RME Digiface with 34 outputs). Root causes: 1. channel_layout() panicked when channel count exceeded 8 (FFmpeg's max) 2. Audio data was processed assuming device's channel count, but FFmpeg can only handle up to 8 channels, causing buffer size mismatches 3. Output audio stream was configured for 34 channels but only 8 channels of data were being sent, causing ~4x playback speed (chipmunk effect) Fixes: - AudioInfo now stores actual device channel count for correct data parsing - Added for_ffmpeg_output() helper that clamps channels to 8 for FFmpeg ops - channel_layout() gracefully handles any channel count by clamping - AudioResampler, AudioPlaybackBuffer, and PrerenderedAudioBuffer all clamp - Output stream config now matches clamped channel count The key insight: we need the real channel count to correctly parse interleaved audio data from the device, but must clamp to 8 channels for FFmpeg processing and audio output streams.
Greptile found no issues!From now on, if a review finishes and we haven't found any issues, we will not post anything, but you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
|
Legend... thank you. |
|
@richiemcilroy When will this be officially published in the build? |
2 participants
Summary
Fixes crash and chipmunk audio on systems with professional audio interfaces that report high channel counts (e.g., RME Digiface with 34 outputs).
The Problem
When using Cap with a multi-channel audio interface, users experienced:
Option::unwrap()on aNonevalue" inchannel_layout()Root Cause
FFmpeg only supports channel layouts up to 8 channels (7.1 surround). When a device reported more channels (like 34), several things broke:
channel_layout()called.unwrap()onNonebecause there's no FFmpeg layout for 34 channelsThe Fix
for_ffmpeg_output()helper for consistent clamping across the codebasechannel_layout()gracefully handle any channel countFiles Changed
crates/media-info/src/lib.rs- Core AudioInfo fixes and helper methodcrates/editor/src/audio.rs- AudioResampler and buffer fixescrates/editor/src/playback.rs- Stream configuration fixesTesting
Tested on Mac with RME Digiface (34 output channels) + RØDE NT-USB Mini microphone:
Greptile Summary
This PR fixes crashes and chipmunk audio playback on systems with professional audio interfaces reporting more than 8 channels (e.g., RME Digiface with 34 outputs).
Key Changes:
AudioInfonow stores the actual device channel count for correct audio data parsing, while clamping to 8 channels when interfacing with FFmpeg (which only supports up to 7.1 surround)for_ffmpeg_output()helper method for consistent channel clamping across the codebasechannel_layout()to gracefully handle 0-channel and excessive channel counts instead of panickingwrap_frame_with_max_channels()to use actual channel count for input parsing while limiting output to FFmpeg-compatible layoutsImpact:
The fix properly separates the concern of parsing device audio data (which needs the real channel count) from FFmpeg encoding constraints (max 8 channels), eliminating both the panic and the playback speed issue.
Confidence Score: 5/5
Important Files Changed
for_ffmpeg_output()helper, prevents panics inchannel_layout(), handles 0-channel and 34+ channel edge cases correctly, includes comprehensive testsfor_ffmpeg_output()in AudioPlaybackBuffer, AudioResampler, and PrerenderedAudioBuffer constructors to ensure consistent FFmpeg compatibility across all audio processing pipelinescreate_stream()andcreate_stream_prerendered(), preventing channel count mismatches during playbackSequence Diagram
sequenceDiagram participant Device as Audio Device<br/>(34 channels) participant AudioInfo as AudioInfo participant Resampler as AudioResampler participant FFmpeg as FFmpeg<br/>(max 8 channels) participant Output as Audio Output Stream Note over Device,Output: Recording/Playback Initialization Device->>AudioInfo: from_stream_config(34 channels) activate AudioInfo Note over AudioInfo: Store actual channel count (34)<br/>for correct data parsing AudioInfo-->>Device: AudioInfo { channels: 34 } deactivate AudioInfo Note over Device,Output: Audio Processing Pipeline Device->>AudioInfo: wrap_frame_with_max_channels(data, 8) activate AudioInfo Note over AudioInfo: Use 34 channels to parse input<br/>Extract only first 8 channels<br/>Avoid div-by-zero with max(1) AudioInfo->>FFmpeg: Create frame with 8 channels activate FFmpeg FFmpeg-->>AudioInfo: Audio frame (8 channels) deactivate FFmpeg AudioInfo-->>Device: Properly formatted frame deactivate AudioInfo Device->>Resampler: new(output_info) activate Resampler Note over Resampler: Call for_ffmpeg_output()<br/>Clamp to 8 channels Resampler->>AudioInfo: for_ffmpeg_output() activate AudioInfo Note over AudioInfo: Return AudioInfo { channels: 8 } AudioInfo-->>Resampler: Clamped AudioInfo deactivate AudioInfo Resampler->>FFmpeg: Create resampler context (8 channels) FFmpeg-->>Resampler: Resampler ready deactivate Resampler Device->>Output: Configure stream activate Output Note over Output: Set config.channels = 8<br/>(matches clamped AudioInfo) Output-->>Device: Stream configured deactivate Output Note over Device,Output: Result: No panic, correct playback speed