Audio fixes #1319
Audio fixes #1319
Conversation
- For devices with >2 channels, only use first 2 when loading into ffmpeg - If output pipeline only has 1 audio source, don't use audio mixer
|
Warning Rate limit exceeded@Brendonovich has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 37 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughExtended audio channel support and added channel-limiting APIs in media-info; refactored recording pipeline audio setup with new BuildCtx and PreparedAudioSources to support optional mixing; updated microphone source and CLI to use bounded-channel wrapping; minor UI import/formatting edits and richer error contexts in pipeline shutdowns. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Builder
participant Setup as SetupCtx
participant Build as BuildCtx
participant Prepared as PreparedAudioSources
participant Mixer as AudioMixer
participant Muxer as OutputMuxer
participant Pipeline as OutputPipeline
User->>Setup: initiate setup_build()
Setup->>Prepared: setup_audio_sources() (may return none)
alt no audio
Prepared-->>Setup: None
else audio present
Prepared->>Prepared: determine single vs multiple sources
alt single source
Prepared->>Muxer: wire source directly (use source audio_info)
else multiple sources
Prepared->>Mixer: create & register sources
Mixer->>Mixer: start mixing
Mixer->>Muxer: push mixed frames
Prepared->>Muxer: register mixer stop task
end
end
Prepared->>Build: create BuildCtx (stop token, done channel, pause flag)
Build->>Muxer: configure lifecycle and spawn tasks
Muxer->>Pipeline: return configured pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/media-info/src/lib.rs (1)
152-171: Handle packed multi-channel truncation instead oftodo!()When
self.sample_formatis packed andself.channels > max_channels, thistodo!()will panic. Windows system audio explicitly switches toSample::F32(Type::Packed)(seecrates/recording/src/sources/screen_capture/windows.rs), so as soon as we try to clamp a packed 5.1 device down to two channels the application will crash. Please implement the truncation logic for packed buffers so we actually keep the firstmax_channelssamples instead of panicking.Apply this diff to copy only the first
out_channelsworth of packed samples:- } else if frame.is_packed() && self.channels > max_channels { - todo!(); + } else if frame.is_packed() && self.channels > max_channels { + let truncated_stride = sample_size * out_channels; + let dst = frame.data_mut(0); + + for (chunk_index, interleaved_chunk) in data.chunks(interleaved_chunk_size).enumerate() + { + let copy_len = truncated_stride.min(interleaved_chunk.len()); + let start = chunk_index * truncated_stride; + let end = start + copy_len; + dst[start..end].copy_from_slice(&interleaved_chunk[..copy_len]); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/app/s/[videoId]/_components/AuthOverlay.tsx(1 hunks)apps/web/app/s/[videoId]/_components/Sidebar.tsx(1 hunks)crates/media-info/src/lib.rs(4 hunks)crates/recording/examples/recording-cli.rs(3 hunks)crates/recording/src/output_pipeline/core.rs(8 hunks)crates/recording/src/sources/microphone.rs(1 hunks)crates/recording/src/studio_recording.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.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/recording/src/sources/microphone.rscrates/recording/src/studio_recording.rscrates/recording/examples/recording-cli.rscrates/media-info/src/lib.rscrates/recording/src/output_pipeline/core.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/recording/src/sources/microphone.rscrates/recording/src/studio_recording.rscrates/media-info/src/lib.rscrates/recording/src/output_pipeline/core.rs
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/web/app/s/[videoId]/_components/AuthOverlay.tsxapps/web/app/s/[videoId]/_components/Sidebar.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/web/app/s/[videoId]/_components/AuthOverlay.tsxapps/web/app/s/[videoId]/_components/Sidebar.tsx
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/app/s/[videoId]/_components/AuthOverlay.tsxapps/web/app/s/[videoId]/_components/Sidebar.tsx
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for all client-side server state and fetching in the web app
Mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData
Run server-side effects via the ManagedRuntime from apps/web/lib/server.ts using EffectRuntime.runPromise/runPromiseExit; do not create runtimes ad hoc
Client code should use helpers from apps/web/lib/EffectRuntime.ts (useEffectQuery, useEffectMutation, useRpcClient); never call ManagedRuntime.make inside components
Files:
apps/web/app/s/[videoId]/_components/AuthOverlay.tsxapps/web/app/s/[videoId]/_components/Sidebar.tsx
apps/web/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Server components needing Effect services must call EffectRuntime.runPromise(effect.pipe(provideOptionalAuth))
Files:
apps/web/app/s/[videoId]/_components/AuthOverlay.tsxapps/web/app/s/[videoId]/_components/Sidebar.tsx
🧠 Learnings (1)
📚 Learning: 2025-10-28T08:39:42.216Z
Learnt from: Brendonovich
PR: CapSoftware/Cap#1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.216Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/recording/src/sources/microphone.rscrates/recording/src/output_pipeline/core.rs
🧬 Code graph analysis (4)
crates/recording/src/sources/microphone.rs (5)
crates/recording/src/output_pipeline/core.rs (2)
audio_info(740-742)audio_info(826-826)crates/recording/src/sources/screen_capture/macos.rs (2)
audio_info(37-44)audio_info(442-444)crates/recording/src/sources/screen_capture/windows.rs (2)
audio_info(44-54)audio_info(369-371)crates/recording/src/sources/screen_capture/mod.rs (2)
audio_info(231-231)audio_info(356-358)crates/recording/src/feeds/microphone.rs (1)
audio_info(243-245)
crates/recording/examples/recording-cli.rs (3)
crates/recording/src/studio_recording.rs (2)
new(377-388)new(613-632)crates/recording/src/instant_recording.rs (1)
new(259-269)crates/recording/src/feeds/microphone.rs (2)
new(87-97)list(104-126)
crates/media-info/src/lib.rs (1)
crates/audio/src/audio_data.rs (1)
channels(162-164)
crates/recording/src/output_pipeline/core.rs (1)
crates/recording/src/sources/audio_mixer.rs (3)
new(48-52)mpsc(521-521)builder(409-411)
⏰ 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: Clippy
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
apps/web/app/s/[videoId]/_components/AuthOverlay.tsx (1)
1-14: Import reordering looks good.The movement of the
OtpFormimport afterusePublicEnvmaintains clarity and has no functional impact.apps/web/app/s/[videoId]/_components/Sidebar.tsx (1)
97-102: Formatting adjustment in nested ternary is appropriate.The indentation change improves readability of the conditional logic without altering behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/media-info/src/lib.rs (1)
325-378: Consider additional edge-case tests.The current tests cover the primary scenarios well. Consider adding tests for edge cases to ensure robustness:
max_channels > self.channels(should work transparently)max_channels == 0(should handle gracefully or panic with clear error)- Mono audio with
max_channels > 1(should correctly keep mono format)- Interaction between
with_max_channelsandwrap_frame_with_max_channels(to document expected behavior)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/media-info/src/lib.rs(4 hunks)crates/recording/examples/recording-cli.rs(2 hunks)crates/recording/src/studio_recording.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/recording/src/studio_recording.rs
- crates/recording/examples/recording-cli.rs
🧰 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/media-info/src/lib.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/media-info/src/lib.rs
🧬 Code graph analysis (1)
crates/media-info/src/lib.rs (5)
crates/recording/src/output_pipeline/core.rs (7)
new(307-325)new(675-677)new(718-720)new(751-753)new(795-812)audio_info(740-742)audio_info(826-826)crates/recording/src/sources/screen_capture/windows.rs (2)
audio_info(44-54)audio_info(369-371)crates/recording/src/sources/screen_capture/macos.rs (2)
audio_info(37-44)audio_info(442-444)crates/recording/src/sources/microphone.rs (1)
audio_info(54-56)crates/recording/src/sources/screen_capture/mod.rs (3)
audio_info(231-231)audio_info(356-358)info(352-354)
⏰ 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: Clippy
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (3)
crates/media-info/src/lib.rs (3)
27-27: Verify the rationale for doubling MAX_AUDIO_CHANNELS.The PR objectives focus on limiting to 2 channels for multi-channel devices, yet this change expands the maximum from 8 to 16. Please confirm whether this increase is necessary for the current changes or if it addresses a different requirement.
189-192: LGTM!Clean refactoring that delegates to
wrap_frame_with_max_channelswhile maintaining backward compatibility.
136-187: No issues found. The usage pattern is safe and correct.The
with_max_channelsmethod is a consuming builder that returns a new instance—it does not mutate the original. In microphone.rs,wrap_frame_with_max_channelsis called on the originalaudio_infoinstance (which retains its full channel count for correct data format interpretation), whileaudio_info.with_max_channels(2)creates a separate limited instance stored inself.info. The original concern aboutself.channelsbeing incorrect is not applicable here.
Summary by CodeRabbit
New Features
Improvements