Mixed audio #1471
Mixed audio #1471
Conversation
📝 WalkthroughWalkthroughSimplifies DeviceEvent::DefaultInputChanged to a unit variant; centralizes headphone detection for default output via a new macOS util; adds a mixed-audio capture/streaming path (macOS + fallback) with MixedInput/MixedStream and wires it through AudioInput/AudioStream and the listener pipeline; renames ProcMsg variants and updates the source actor to choose mixed vs mic+speaker streams based on headphone state. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Source as Source Actor
participant Audio as AudioInput / MixedStream
participant Proc as AudioProcessor
Note right of Audio: utils::is_headphone_from_default_output_device() used for DefaultOutputChanged (macOS)
User->>Source: Start
Source->>Audio: get_default_mic_name() / is_using_headphone()
alt use_mixed (no headphones on macOS)
Source->>Audio: from_mixed().stream()
loop per sample
Audio-->>Source: f32 sample
Source->>Proc: ProcMsg::Mixed(AudioChunk)
Proc-->>Proc: join/mix -> transcript flow
end
else mic+speaker path
par mic stream
Audio-->>Source: mic chunk
Source->>Proc: ProcMsg::Mic(AudioChunk)
and speaker stream
Audio-->>Source: spk chunk
Source->>Proc: ProcMsg::Speaker(AudioChunk)
end
Proc-->>Proc: join -> transcript flow
end
Note over Source,Audio: Cancellation tokens stop streams and actor
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 8
🧹 Nitpick comments (8)
crates/audio/src/speaker/macos.rs (1)
109-114: Nominal sample rate change: avoid truncation; confirm intent.Switching to nominal rate is fine, but casting f64 → u32 via
astruncates. Prefer rounding to prevent 47999 on 48k, etc. Also confirm that reporting nominal (not actual) is desired for downstream consumers.- ctx.current_sample_rate.store( - device - .nominal_sample_rate() - .unwrap_or(ctx.format.absd().sample_rate) as u32, - Ordering::Release, - ); + ctx.current_sample_rate.store( + device + .nominal_sample_rate() + .unwrap_or(ctx.format.absd().sample_rate) + .round() as u32, + Ordering::Release, + );plugins/listener/src/actors/listen.rs (1)
163-175: Drop unnecessary deep clone when updating the session.You can build the flattened words vector from the existing map without cloning the entire structure.
- update_session( - &app, - &session_id, - final_words_by_channel - .clone() - .values() - .flatten() - .cloned() - .collect(), - ) + update_session( + &app, + &session_id, + final_words_by_channel + .values() + .flatten() + .cloned() + .collect(), + ) .await .unwrap();crates/audio/src/utils.rs (1)
28-40: Test module visibility/naming nit.Consider making the test module private and naming it
testsfor convention.-#[cfg(test)] -pub mod test { +#[cfg(test)] +mod tests {crates/audio/src/device_monitor.rs (1)
120-126: Make stop() responsive by bounding the run loop wait.
run_until_date(distant_future)can block indefinitely; stop may not break until another event occurs. Poll the run loop with a short horizon.- loop { - run_loop.run_until_date(&ns::Date::distant_future()); + loop { + // Short horizon to periodically check the stop notifier. + run_loop.run_until_date(&ns::Date::with_time_interval_since_now(0.1)); if stop_notifier_rx.try_recv().is_ok() { break; } std::thread::sleep(std::time::Duration::from_millis(100)); }If
with_time_interval_since_nowisn’t available, use the equivalent API incidre::ns.crates/audio/src/mixed/macos.rs (1)
137-156: Avoid per-frame per-channel lookups in the mixing loop.Cache channel slices and reuse them to reduce overhead in the callback.
- let format = view.format(); - - if format.channel_count() > 1 { - let frame_count = view.frame_len() as usize; - let mut mixed_buffer = Vec::with_capacity(frame_count); - - for frame_idx in 0..frame_count { - let mut mixed_sample = 0.0f32; - let channel_count = format.channel_count() as usize; - - for channel in 0..channel_count { - if let Some(channel_data) = view.data_f32_at(channel) { - if frame_idx < channel_data.len() { - mixed_sample += channel_data[frame_idx]; - } - } - } - - // Average the mixed sample - mixed_sample /= channel_count as f32; - mixed_buffer.push(mixed_sample); - } + let format = view.format(); + let channel_count = format.channel_count() as usize; + if channel_count > 1 { + let slices: Vec<&[f32]> = (0..channel_count) + .filter_map(|ch| view.data_f32_at(ch)) + .collect(); + let frame_count = view.frame_len() as usize; + let mut mixed_buffer = Vec::with_capacity(frame_count); + for frame_idx in 0..frame_count { + let mut acc = 0.0f32; + let mut n = 0usize; + for data in &slices { + if frame_idx < data.len() { + acc += data[frame_idx]; + n += 1; + } + } + mixed_buffer.push(if n > 0 { acc / n as f32 } else { 0.0 }); + }plugins/listener/src/actors/source.rs (3)
246-254: Avoid hot-path allocations when applying mutesZero buffers in place instead of allocating a new Vec every chunk.
Apply these diffs:
- let output_data = if mic_muted.load(Ordering::Relaxed) { - vec![0.0; data.len()] - } else { - data - }; - - let msg = ProcMsg::Mic(AudioChunk{ data: output_data }); + let mut data = data; + if mic_muted.load(Ordering::Relaxed) { + data.fill(0.0); + } + let msg = ProcMsg::Mic(AudioChunk{ data });- let output_data = if spk_muted.load(Ordering::Relaxed) { - vec![0.0; data.len()] - } else { - data - }; - - let msg = ProcMsg::Speaker(AudioChunk{ data: output_data }); + let mut data = data; + if spk_muted.load(Ordering::Relaxed) { + data.fill(0.0); + } + let msg = ProcMsg::Speaker(AudioChunk{ data });Also applies to: 260-268
173-175: Log level nit: prefer debug for path selectionuse_mixed toggles often; debug is less noisy for normal runs.
Apply this diff:
- tracing::info!(use_mixed = use_mixed); + tracing::debug!(use_mixed = use_mixed);
24-31: Align naming: SourceArgs.device vs SourceState.mic_deviceConsider renaming SourceArgs.device to mic_device for clarity and symmetry (or add a doc comment explaining the distinction).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
crates/audio/src/device_monitor.rs(3 hunks)crates/audio/src/lib.rs(8 hunks)crates/audio/src/mixed/macos.rs(1 hunks)crates/audio/src/mixed/mod.rs(1 hunks)crates/audio/src/speaker/macos.rs(1 hunks)crates/audio/src/speaker/mod.rs(1 hunks)crates/audio/src/utils.rs(1 hunks)plugins/listener/src/actors/listen.rs(2 hunks)plugins/listener/src/actors/processor.rs(3 hunks)plugins/listener/src/actors/session.rs(1 hunks)plugins/listener/src/actors/source.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. Do not add any error handling. Keep the existing one.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
crates/audio/src/speaker/macos.rsplugins/listener/src/actors/session.rsplugins/listener/src/actors/processor.rscrates/audio/src/mixed/mod.rscrates/audio/src/speaker/mod.rsplugins/listener/src/actors/listen.rscrates/audio/src/utils.rscrates/audio/src/device_monitor.rscrates/audio/src/mixed/macos.rsplugins/listener/src/actors/source.rscrates/audio/src/lib.rs
🧬 Code graph analysis (6)
crates/audio/src/speaker/mod.rs (1)
crates/audio/src/lib.rs (1)
play_sine_for_sec(247-270)
plugins/listener/src/actors/listen.rs (1)
plugins/listener/src/manager.rs (1)
with_unix_timestamp(21-27)
crates/audio/src/device_monitor.rs (1)
crates/audio/src/utils.rs (1)
is_headphone_from_default_output_device(22-25)
crates/audio/src/mixed/macos.rs (2)
crates/audio/src/speaker/macos.rs (8)
std(125-125)sample_rate(36-38)new(51-92)start_device(94-143)proc(98-136)stream(145-180)poll_next(218-240)drop(244-246)crates/audio/src/lib.rs (4)
sample_rate(236-243)stream(175-191)poll_next(204-228)play_sine_for_sec(247-270)
plugins/listener/src/actors/source.rs (2)
crates/audio/src/lib.rs (5)
get_default_mic_name(82-90)is_using_headphone(92-105)from_mixed(144-154)from_mic(122-132)from_speaker(134-142)plugins/listener/src/actors/processor.rs (2)
new(168-173)handle(78-113)
crates/audio/src/lib.rs (3)
crates/audio/src/mic.rs (3)
device(84-84)new(34-66)device_name(20-24)crates/audio/src/utils.rs (1)
is_headphone_from_default_output_device(22-25)crates/audio/src/mixed/macos.rs (1)
new(52-65)
⏰ 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). (2)
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci (macos, macos-14)
🔇 Additional comments (9)
plugins/listener/src/actors/listen.rs (1)
50-52: Avoid panicking in actor startup (verify acceptable).
unwrap()onspawn_rx_task(...)will crash the actor if connection setup fails. If fail‑fast here is intentional, all good—otherwise consider propagating the error in a follow‑up.crates/audio/src/speaker/mod.rs (1)
102-105: LGTM: central test helper import reduces duplication.Switching tests to
crate::play_sine_for_secis cleaner and keeps one source of truth.plugins/listener/src/actors/session.rs (1)
315-315: Enum rename verified — no remaining AttachListen references.
ProcMsg::AttachListener is defined/handled in plugins/listener/src/actors/processor.rs and invoked from plugins/listener/src/actors/session.rs; search found no occurrences of the old AttachListen variant.plugins/listener/src/actors/processor.rs (2)
19-23: Renamed variants + new Mixed path look consistent.Enum surface aligns with the broader rename and addition. No issues spotted here.
101-110: AGC choice for Mixed: confirm intent.Mixed uses mic AGC (agc_m). If Mixed already contains speaker+mic, AGC tuned for mic may pump the program audio. Confirm this is desired.
crates/audio/src/utils.rs (1)
1-26: macOS headphone detection util looks solid.Approach is reasonable and safely handles None/Err.
crates/audio/src/device_monitor.rs (1)
66-86: Centralizing headphone detection LGTM.Import/use of
is_headphone_from_default_output_devicesimplifies the macOS path.crates/audio/src/lib.rs (1)
82-90: Panic risk if no default input device.
get_default_mic_name()unwraps the default device; confirm this is only called when a device is guaranteed to exist.plugins/listener/src/actors/source.rs (1)
173-174: Confirm intended policy: mixed when not using headphones (macOS-only detection found)AudioInput::is_using_headphone() is used to decide mixed (plugins/listener/src/actors/source.rs:173). is_using_headphone() calls the macOS helper (crates/audio/src/lib.rs -> utils::macos::is_headphone_from_default_output_device()) and device monitoring sends DefaultOutputChanged only on macOS (crates/audio/src/device_monitor.rs). Consequence: non-macOS platforms lack detection and will default to “not using headphones” → use_mixed = true. Confirm this default is intended; otherwise implement cross‑platform detection or document the behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugins/listener/src/actors/processor.rs (1)
118-141: Bypass AEC and avoid double-summing when mic/spk share the same Arc (Mixed case)Detect identical buffers with
Arc::ptr_eq. If identical: (1) skip AEC, (2) recorder forwards the single stream as‑is, (3) listener gets identical bytes on both channels.Apply this diff:
async fn process_ready(st: &mut ProcState) { while let Some((mic, spk)) = st.joiner.pop_pair() { - let mic = st - .aec - .process_streaming(&mic, &spk) - .unwrap_or_else(|_| mic.to_vec()); + let identical = Arc::ptr_eq(&mic, &spk); + let mic = if identical { + // Already mixed; preserve as-is. + mic.to_vec() + } else { + st.aec + .process_streaming(&mic, &spk) + .unwrap_or_else(|_| mic.to_vec()) + }; @@ - if let Some(rec) = &st.recorder { - let mixed: Vec<f32> = mic - .iter() - .zip(spk.iter()) - .map(|(m, s)| (m + s).clamp(-1.0, 1.0)) - .collect(); - rec.cast(RecMsg::Audio(mixed)).ok(); - } + if let Some(rec) = &st.recorder { + let mixed: Vec<f32> = if identical { + mic.clone() + } else { + mic.iter() + .zip(spk.iter()) + .map(|(m, s)| (m + s).clamp(-1.0, 1.0)) + .collect() + }; + rec.cast(RecMsg::Audio(mixed)).ok(); + } @@ - if let Some(actor) = &st.listen { - let mic_bytes = hypr_audio_utils::f32_to_i16_bytes(mic.into_iter()); - let spk_bytes = hypr_audio_utils::f32_to_i16_bytes(spk.iter().copied()); - - actor - .cast(ListenMsg::Audio(mic_bytes.into(), spk_bytes.into())) - .ok(); - } + if let Some(actor) = &st.listen { + if identical { + let spk_bytes = hypr_audio_utils::f32_to_i16_bytes(spk.iter().copied()); + actor + .cast(ListenMsg::Audio(spk_bytes.clone().into(), spk_bytes.into())) + .ok(); + } else { + let mic_bytes = hypr_audio_utils::f32_to_i16_bytes(mic.into_iter()); + let spk_bytes = hypr_audio_utils::f32_to_i16_bytes(spk.iter().copied()); + actor + .cast(ListenMsg::Audio(mic_bytes.into(), spk_bytes.into())) + .ok(); + } + }Side effect to confirm: VU metering (Lines 153–161) will show identical mic/spk amplitudes in Mixed. If UI expects different semantics, we can gate meter updates behind
identical.Also applies to: 143-150
🧹 Nitpick comments (1)
plugins/listener/src/actors/processor.rs (1)
101-112: Mixed path: avoid zero-mic allocation; treat as identical buffers and bypass AEC downstreamFeeding a zeroed mic causes needless AEC work and mutes the mic channel for listeners. Prefer pushing the same Arc to mic+spk, run speaker AGC (or none), and let
process_readydetect identical buffers to skip AEC and avoid double-summing. This also fixes VU metering so mic isn’t always silent in Mixed.Apply this diff here:
- ProcMsg::Mixed(mut c) => { - st.agc_m.process(&mut c.data); - - let empty_arc = Arc::<[f32]>::from(vec![0.0; c.data.len()]); - let arc = Arc::<[f32]>::from(c.data); - - st.last_mic = Some(empty_arc.clone()); - st.last_spk = Some(arc.clone()); - st.joiner.push_mic(empty_arc.clone()); - st.joiner.push_spk(arc); - process_ready(st).await; - } + ProcMsg::Mixed(mut c) => { + // Mixed stream gain-shaping should follow speaker characteristics. + st.agc_s.process(&mut c.data); + let arc = Arc::<[f32]>::from(c.data); + // Identical buffers => let process_ready() special‑case this. + st.last_mic = Some(arc.clone()); + st.last_spk = Some(arc.clone()); + st.joiner.push_mic(arc.clone()); + st.joiner.push_spk(arc); + process_ready(st).await; + }Note: This expects
process_readyto handle identical mic/spk (see next comment).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/listener/src/actors/processor.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. Do not add any error handling. Keep the existing one.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
plugins/listener/src/actors/processor.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). (2)
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci (macos, macos-14)
🔇 Additional comments (3)
plugins/listener/src/actors/processor.rs (3)
85-86: LGTM: rename wiring is consistentState assignment for
AttachListenermirrors the previousAttachListenbehavior.
94-100: LGTM: speaker pathAGC on speaker, state update, and joiner enqueue remain consistent with prior semantics.
19-22: Resolved: public message renames and Mixed wiring verifiedAll call sites migrated — no
SpkorAttachListenremains. Processor handlesProcMsg::AttachListener(plugins/listener/src/actors/processor.rs:85),ProcMsg::Speaker(plugins/listener/src/actors/processor.rs:94),ProcMsg::Mixed(plugins/listener/src/actors/processor.rs:101); producers sendProcMsg::Mixed/ProcMsg::Speaker(plugins/listener/src/actors/source.rs:205,266); session castsAttachListener(plugins/listener/src/actors/session.rs:314).
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)
plugins/listener/src/actors/source.rs (1)
8-12: Fix DeviceEvent match: DefaultInputChanged is a unit variantplugins/listener/src/actors/source.rs (around line 67) matches DefaultInputChanged with
{ .. }but DeviceEvent::DefaultInputChanged is a unit variant (crates/audio/src/device_monitor.rs). Replace the match arm with the unit pattern, e.g.:DeviceEvent::DefaultInputChanged
| DeviceEvent::DefaultOutputChanged { .. } => { /* ... */ }
♻️ Duplicate comments (4)
crates/audio/src/lib.rs (1)
144-155: Make from_mixed consistent: return Self and drop the Result/unwrap mismatchFunction returns Result but constructs MixedInput with unwrap, so Err can never occur; callers then unwrap again. Prefer a Self return for clarity.
Apply:
- #[cfg(target_os = "macos")] - pub fn from_mixed() -> Result<Self, crate::Error> { - let mixed = MixedInput::new().unwrap(); - - Ok(Self { + #[cfg(target_os = "macos")] + pub fn from_mixed() -> Self { + let mixed = MixedInput::new().unwrap(); + + Self { source: AudioSource::RealtimeMixed, mic: None, speaker: None, mixed: Some(mixed), data: None, - }) + } }plugins/listener/src/actors/source.rs (3)
66-71: Don’t overwrite user‑selected mic on output changes; treat output changes as restart‑onlyUpdate to only change mic on DefaultInputChanged; for DefaultOutputChanged, just trigger restart logic.
Apply:
- match event { - DeviceEvent::DefaultInputChanged { .. } - | DeviceEvent::DefaultOutputChanged { .. } => { - let new_device = AudioInput::get_default_mic_name(); - let _ = myself_clone.cast(SourceCtrl::SetMicDevice(Some(new_device))); - } - } + match event { + DeviceEvent::DefaultInputChanged { .. } => { + let new_device = AudioInput::get_default_mic_name(); + let _ = myself_clone.cast(SourceCtrl::SetMicDevice(Some(new_device))); + } + DeviceEvent::DefaultOutputChanged { .. } => { + // restart stream, keep current mic selection + let _ = myself_clone.cast(SourceCtrl::SetMicDevice(None)); + } + }
185-191: If from_mixed returns Self, drop the extra unwrap hereAlign call site with the proposed signature change.
Apply:
- let mut mixed_input = AudioInput::from_mixed().unwrap(); + let mut mixed_input = AudioInput::from_mixed();
204-213: Avoid per‑chunk allocations when muting; zero in placeAllocate‑free path reduces GC/allocator pressure in hot loop.
Apply:
- if let Some(data) = mixed_next { - // TODO: should be able to mute each stream - let output_data = if mic_muted.load(Ordering::Relaxed) && spk_muted.load(Ordering::Relaxed) { - vec![0.0; data.len()] - } else { - data - }; - - let msg = ProcMsg::Mixed(AudioChunk{ data: output_data }); + if let Some(mut data) = mixed_next { + // Why: avoid allocs; only silence when both are muted + if mic_muted.load(Ordering::Relaxed) && spk_muted.load(Ordering::Relaxed) { + data.fill(0.0); + } + let msg = ProcMsg::Mixed(AudioChunk{ data });
🧹 Nitpick comments (3)
crates/audio/src/lib.rs (1)
82-90: get_default_mic_name() can panic on systems without an input devicedefault_input_device().unwrap() will crash if no mic is present. If that invariant can’t be guaranteed at startup, consider a non‑panicking fallback; otherwise document the assumption.
plugins/listener/src/actors/source.rs (2)
125-136: Preserve mic selection when restart‑signaling with NoneOnly update st.mic_device when Some(...) is provided; None should mean “restart with existing device.”
Apply:
- SourceCtrl::SetMicDevice(dev) => { - st.mic_device = dev; + SourceCtrl::SetMicDevice(dev) => { + if let Some(d) = dev { + st.mic_device = Some(d); + }
252-260: Same zero‑in‑place optimization for mic/spk branchesRemove vec allocations on every muted chunk.
Apply:
- if let Some(data) = mic_next { - let output_data = if mic_muted.load(Ordering::Relaxed) { - vec![0.0; data.len()] - } else { - data - }; - let msg = ProcMsg::Mic(AudioChunk{ data: output_data }); + if let Some(mut data) = mic_next { + if mic_muted.load(Ordering::Relaxed) { + data.fill(0.0); + } + let msg = ProcMsg::Mic(AudioChunk{ data });- if let Some(data) = spk_next { - let output_data = if spk_muted.load(Ordering::Relaxed) { - vec![0.0; data.len()] - } else { - data - }; - let msg = ProcMsg::Speaker(AudioChunk{ data: output_data }); + if let Some(mut data) = spk_next { + if spk_muted.load(Ordering::Relaxed) { + data.fill(0.0); + } + let msg = ProcMsg::Speaker(AudioChunk{ data });Also applies to: 266-274
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/audio/src/lib.rs(8 hunks)crates/audio/src/mixed/macos.rs(1 hunks)crates/audio/src/mixed/mod.rs(1 hunks)plugins/listener/src/actors/source.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/audio/src/mixed/mod.rs
- crates/audio/src/mixed/macos.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. Do not add any error handling. Keep the existing one.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
crates/audio/src/lib.rsplugins/listener/src/actors/source.rs
🧬 Code graph analysis (2)
crates/audio/src/lib.rs (3)
crates/audio/src/mic.rs (3)
device(84-84)new(34-66)device_name(20-24)crates/audio/src/utils.rs (1)
is_headphone_from_default_output_device(22-25)crates/audio/src/mixed/macos.rs (1)
new(52-65)
plugins/listener/src/actors/source.rs (2)
crates/audio/src/lib.rs (6)
get_default_mic_name(82-90)silence(48-63)is_using_headphone(92-105)from_mixed(145-155)from_mic(122-132)from_speaker(134-142)plugins/listener/src/actors/processor.rs (2)
new(170-175)handle(78-115)
⏰ 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). (2)
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci (macos, macos-14)
🔇 Additional comments (5)
crates/audio/src/lib.rs (2)
247-271: LGTM: handy test helperThe gated sine helper is tidy and localized to tests.
184-186: Unwrap safe — RealtimeMixed only constructed with mixed = Some(...).
The only construction found is crates/audio/src/lib.rs (lines ~149–152) which sets mixed: Some(mixed); no other constructors referencing AudioSource::RealtimeMixed were found.plugins/listener/src/actors/source.rs (3)
76-79: Honoring SourceArgs.device: nice fixPreferring args.device over the system default removes the unused‑param smell and respects user intent.
175-180: Mixed path gating looks rightmacOS uses headphone detection; other OSes keep use_mixed=false.
181-221: Cancellation paths: good cleanup and actor stop semanticsDropping pinned streams and stopping the actor on token cancellation is clear and consistent across branches.
Also applies to: 222-281
| mod device_monitor; | ||
| mod errors; | ||
| mod mic; | ||
| mod mixed; |
There was a problem hiding this comment.
🧩 Analysis chain
Cross‑platform MixedInput availability: verify non‑mac builds compile
MixedInput is referenced in the public struct and re‑exported unconditionally. Please confirm mixed defines a cross‑platform symbol (stub or cfg‑guarded module exposing MixedInput/MixedStream) so Linux/Windows builds don’t break.
Run to check symbol presence and cfg guards:
Also applies to: 13-13, 77-77
🏁 Script executed:
#!/bin/bash
# Expect: at least one unconditional `pub struct MixedInput` (or a re-export)
# or lib.rs fields gated accordingly; otherwise non-mac will fail.
rg -nP '^\s*(pub\s+)?struct\s+MixedInput\b|type\s+MixedInput\b|pub\s+use\s+.*MixedInput' -C2
rg -nP 'cfg\s*\(\s*target_os\s*=\s*"macos"\s*\)' crates/audio/src/mixed -n
# Verify lib.rs doesn't reference MixedInput behind cfg on non-mac
rg -nP 'MixedInput' crates/audio/src/lib.rs -n -C2Length of output: 783
Unconditional MixedInput reference breaks non‑mac builds — fix required
mixed/macos.rs defines pub struct MixedInput (crates/audio/src/mixed/macos.rs:17) and crates/audio/src/mixed/mod.rs is #[cfg(target_os = "macos")] (crates/audio/src/mixed/mod.rs:1), but lib.rs references MixedInput unguarded (crates/audio/src/lib.rs:75–77: mixed: Option<MixedInput>). This will fail on Linux/Windows.
Actionable fixes (choose one):
- Gate the module/uses:
#[cfg(target_os = "macos")] mod mixed;and#[cfg(target_os = "macos")]on the field/funcs that useMixedInput; or - Provide a non‑mac stub/re‑export in mixed/mod.rs (e.g.
#[cfg(not(target_os = "macos"))] pub struct MixedInput;or a no‑op reexport) soMixedInputexists cross‑platform.
🤖 Prompt for AI Agents
In crates/audio/src/lib.rs (mod declaration at line 4 and usages around lines
75–77) the code references MixedInput unconditionally even though mixed/mod.rs
is only compiled on macOS, which breaks non‑mac builds; fix by either gating the
module and all declarations/uses with #[cfg(target_os = "macos")] (e.g.
#[cfg(target_os = "macos")] mod mixed; and #[cfg(target_os = "macos")] on the
MixedInput field and any functions that touch it) or by adding a non‑mac
stub/reexport in mixed/mod.rs (e.g. behind #[cfg(not(target_os = "macos"))]
declare a pub struct MixedInput; or a no‑op reexport) so MixedInput is defined
on all platforms; pick one approach and make the corresponding cfg changes
consistently for the module, field, and any functions that reference MixedInput.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
crates/audio/src/lib.rs (1)
144-155: Don’t unwrap inside a function that returns Result; propagate instead.Keeps error semantics consistent and prevents panic.
- pub fn from_mixed() -> Result<Self, crate::Error> { - let mixed = MixedInput::new().unwrap(); + pub fn from_mixed() -> Result<Self, crate::Error> { + let mixed = MixedInput::new()?; Ok(Self { source: AudioSource::RealtimeMixed, mic: None, speaker: None, mixed: Some(mixed), data: None, }) }#!/bin/bash # Verify error conversion exists (anyhow -> crate::Error) so `?` compiles. rg -nP 'type\s+Error\s*=\s*anyhow::Error|impl\s+From<\s*anyhow::Error\s*>\s+for\s+crate::Error' -g 'crates/**/errors.rs' -S
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/audio/src/lib.rs(8 hunks)crates/audio/src/mixed/mod.rs(1 hunks)crates/audio/src/mixed/other.rs(1 hunks)plugins/listener/src/actors/session.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/listener/src/actors/session.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. Do not add any error handling. Keep the existing one.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
crates/audio/src/mixed/other.rscrates/audio/src/mixed/mod.rscrates/audio/src/lib.rs
🧬 Code graph analysis (3)
crates/audio/src/mixed/other.rs (2)
crates/audio/src/mixed/mod.rs (3)
new(23-26)stream(28-31)poll_next(41-46)crates/audio/src/lib.rs (2)
stream(176-192)poll_next(205-229)
crates/audio/src/mixed/mod.rs (4)
crates/audio/src/mixed/other.rs (3)
new(7-9)stream(11-13)poll_next(21-26)crates/audio/src/mixed/macos.rs (5)
new(52-65)stream(190-224)poll_next(262-284)std(170-170)sample_rate(32-34)crates/audio/src/speaker/mod.rs (7)
new(33-36)new(39-43)stream(46-49)stream(52-56)poll_next(67-80)sample_rate(89-91)sample_rate(94-96)crates/audio/src/lib.rs (3)
stream(176-192)poll_next(205-229)sample_rate(237-244)
crates/audio/src/lib.rs (7)
crates/audio/src/mic.rs (3)
device(84-84)new(34-66)device_name(20-24)crates/audio/src/utils.rs (1)
is_headphone_from_default_output_device(22-25)crates/audio/src/mixed/mod.rs (1)
new(23-26)crates/audio/src/mixed/other.rs (1)
new(7-9)crates/audio/src/mixed/macos.rs (1)
new(52-65)crates/audio/src/speaker/mod.rs (2)
new(33-36)new(39-43)crates/audio/src/speaker/macos.rs (1)
new(51-92)
⏰ 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). (2)
- GitHub Check: ci (macos, macos-14)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (2)
crates/audio/src/mixed/mod.rs (1)
4-16: Good cfg‑gating of platform implementations.macOS vs. non‑mac split is clean and keeps non‑mac builds green.
crates/audio/src/lib.rs (1)
66-71: API surface extension looks consistent.New
RealtimeMixedvariants acrossAudioSource/AudioStreamare wired through stream dispatch and sample_rate.Also applies to: 195-200
| pub fn get_default_mic_name() -> String { | ||
| let name = { | ||
| let host = cpal::default_host(); | ||
| let device = host.default_input_device().unwrap(); | ||
| device.name().unwrap_or("Unknown Microphone".to_string()) | ||
| }; | ||
|
|
||
| name | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Potential panic when no default input device.
default_input_device().unwrap() will panic on systems without an input device; verify this is acceptable for the environments you target.
🏁 Script executed:
#!/bin/bash
# Locate all call sites of get_default_mic_name to ensure it's not used in headless/CI paths.
rg -nP '\bget_default_mic_name\s*\(' --type=rsLength of output: 26
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Rust file count:"
find . -type f -name '*.rs' | wc -l
echo
echo "Searching for occurrences of get_default_mic_name..."
rg -nP '\bget_default_mic_name\s*\(' -S -g '**/*.rs' || true
rg -nP '\bget_default_mic_name\b' -S -g '**/*.rs' || true
echo
echo "Show crates/audio/src/lib.rs (lines 1-200) if exists:"
if [ -f crates/audio/src/lib.rs ]; then
sed -n '1,200p' crates/audio/src/lib.rs
else
echo "crates/audio/src/lib.rs not found"
fiLength of output: 6061
Avoid panic: handle missing default input device in get_default_mic_name
default_input_device().unwrap() will panic if no input device; change get_default_mic_name (crates/audio/src/lib.rs, ~lines 82–90) to avoid unwrap — return Option or use .default_input_device().and_then(|d| d.name().ok()).unwrap_or_else(|| "Unknown Microphone".to_string()). Call sites to review: plugins/listener/src/actors/source.rs:69, plugins/listener/src/actors/source.rs:78.
🤖 Prompt for AI Agents
In crates/audio/src/lib.rs around lines 82–90, get_default_mic_name currently
calls default_input_device().unwrap() which will panic when no input device;
change it to avoid unwrap by either (A) changing the signature to pub fn
get_default_mic_name() -> Option<String> and return
host.default_input_device().and_then(|d| d.name().ok()), or (B) keep returning
String but use host.default_input_device().and_then(|d|
d.name().ok()).unwrap_or_else(|| "Unknown Microphone".to_string()) so it never
panics. Update call sites in plugins/listener/src/actors/source.rs (around lines
69 and 78) to handle the new Option<String> if you choose (A), or leave as-is if
you choose (B); ensure all .unwrap() usages are removed and replace with safe
handling.
| fn sample_rate(&self) -> u32 { | ||
| #[cfg(target_os = "macos")] | ||
| { | ||
| self.inner.sample_rate() | ||
| } | ||
| #[cfg(not(target_os = "macos"))] | ||
| { | ||
| 0 | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Non‑mac sample_rate() returns 0 — confirm no callers divide/use it as a real rate.
If any consumer assumes >0, this can cause divide‑by‑zero or timing bugs.
🏁 Script executed:
#!/bin/bash
# Grep for uses of sample_rate() and nearby arithmetic.
rg -nP -C3 '\bsample_rate\s*\(' --type=rs
rg -nP -C2 '/\s*\(?\s*sample_rate\s*\(' --type=rs
rg -nP -C2 'Duration::from_secs_f(32|64)\s*\(\s*1\.0\s*/' --type=rsLength of output: 80
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "checking for ripgrep (rg)..."
if ! command -v rg >/dev/null 2>&1; then
echo "rg not found; falling back to find+grep for .rs files"
find . -type f -name '*.rs' -print0 | xargs -0 grep -n -E '\bsample_rate' || true
exit 0
fi
rg --version || true
echo
echo "=== occurrences of 'sample_rate' (context -C3) ==="
rg -n -S '\bsample_rate' -C3 || true
echo
echo "=== occurrences of '.sample_rate(' ==="
rg -n -S '\.sample_rate\s*\(' -C3 || true
echo
echo "=== explicit 'sample_rate()' calls ==="
rg -n -S '\bsample_rate\s*\(\)' -C3 || true
echo
echo "=== possible divisions using sample_rate as denominator ==="
rg -n -S '/\s*\(?\s*sample_rate\s*\(' -C3 || true
rg -n -S '1\.\d+\s*/\s*\(?\s*sample_rate\s*\(' -C3 || true
rg -n -S '\/\s*sample_rate\s*\(\)' -C3 || true
echo
echo "=== casts to float near sample_rate ==="
rg -n -S 'sample_rate\s*\(\)\s*as\s*f(32|64)' -C3 || true
rg -n -S 'as\s*f(32|64)\s*.*sample_rate' -C3 || true
echo
echo "=== search results (paths only) for 'sample_rate()' ==="
rg -n --hidden --no-ignore-vcs '\bsample_rate\s*\(' -g '!*target/**' || true
echo
echo "=== print crates/audio/src/mixed/mod.rs (if present) ==="
if [ -f crates/audio/src/mixed/mod.rs ]; then
sed -n '1,240p' crates/audio/src/mixed/mod.rs || true
else
echo "file not found: crates/audio/src/mixed/mod.rs"
fiLength of output: 48672
MixedStream::sample_rate() returns 0 on non‑mac — callers assume >0 and will misbehave or panic.
- crayons/audio/src/mixed/mod.rs:54-60 — non‑mac branch returns 0.
- crates/vad/src/continuous.rs:42 (used to compute chunk_samples at 46) — will set config.sample_rate = 0.
- crates/pyannote-local/src/segmentation.rs:134-139 — divides by sample_rate and casts indices; 0 -> inf/NaN -> invalid integer casts.
- crates/audio-utils/src/lib.rs:90-93 — resampling math uses source.sample_rate() as f64.
- crates/audio/src/resampler.rs:15,35,41 — resampler logic relies on source.sample_rate().
- crates/audio/src/norm.rs:66-77 — EbuR128/TruePeakLimiter created with sample_rate (must be >0).
- crates/audio/src/speaker/mod.rs:154 — assert!(sample_rate > 0) (will fail if sample_rate() == 0).
Action: fix MixedStream::sample_rate() on non‑mac to return a valid sample rate (or change the API to return Option/Result and update callers to handle missing/zero rates).
🤖 Prompt for AI Agents
In crates/audio/src/mixed/mod.rs around lines 54-63, MixedStream::sample_rate()
currently returns 0 on non-mac which breaks callers; change the non-mac branch
to return the real sample rate instead of 0 by delegating to
self.inner.sample_rate() (remove the platform-gated 0 return), ensuring inner
exposes a valid sample_rate(); if inner cannot provide a valid rate, change the
method signature to return Option<u32> or Result<u32, Error> and update all
callers to handle None/Error (replace blind divisions/casts and asserts with
guarded logic that fails gracefully or propagates the error).
| pub fn stream(self) -> Result<MixedStream> { | ||
| Ok(MixedStream {}) | ||
| } |
There was a problem hiding this comment.
Align non‑mac API with wrapper to avoid type mismatch (compile‑blocking).
MixedInput::stream should return MixedStream (not Result) to match macOS and the wrapper in mixed/mod.rs, otherwise let inner = self.inner.stream(); in the wrapper becomes Result<_> on non‑mac and fails to compile.
- pub fn stream(self) -> Result<MixedStream> {
- Ok(MixedStream {})
- }
+ pub fn stream(self) -> MixedStream {
+ MixedStream {}
+ }📝 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.
| pub fn stream(self) -> Result<MixedStream> { | |
| Ok(MixedStream {}) | |
| } | |
| pub fn stream(self) -> MixedStream { | |
| MixedStream {} | |
| } |
🤖 Prompt for AI Agents
In crates/audio/src/mixed/other.rs around lines 11 to 13, the non-mac
implementation of MixedInput::stream currently returns Result<MixedStream>
causing a type mismatch with the macOS API and the wrapper in mixed/mod.rs;
change the function signature to return MixedStream (not Result), and update the
body to directly return a MixedStream value (e.g., MixedStream {}) instead of
Ok(...); ensure any Result-related imports or error handling are removed or
adjusted so the function and its callers expect a plain MixedStream.
| fn poll_next( | ||
| mut self: std::pin::Pin<&mut Self>, | ||
| cx: &mut std::task::Context<'_>, | ||
| ) -> Poll<Option<Self::Item>> { | ||
| Poll::Pending | ||
| } |
There was a problem hiding this comment.
Fix missing Poll path and silence the unused cx warning.
Poll isn’t in scope and cx is unused; both trigger errors/warnings on strict builds.
- fn poll_next(
- mut self: std::pin::Pin<&mut Self>,
- cx: &mut std::task::Context<'_>,
- ) -> Poll<Option<Self::Item>> {
- Poll::Pending
- }
+ fn poll_next(
+ mut self: std::pin::Pin<&mut Self>,
+ _cx: &mut std::task::Context<'_>,
+ ) -> std::task::Poll<Option<Self::Item>> {
+ std::task::Poll::Pending
+ }📝 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.
| fn poll_next( | |
| mut self: std::pin::Pin<&mut Self>, | |
| cx: &mut std::task::Context<'_>, | |
| ) -> Poll<Option<Self::Item>> { | |
| Poll::Pending | |
| } | |
| fn poll_next( | |
| mut self: std::pin::Pin<&mut Self>, | |
| _cx: &mut std::task::Context<'_>, | |
| ) -> std::task::Poll<Option<Self::Item>> { | |
| std::task::Poll::Pending | |
| } |
🤖 Prompt for AI Agents
In crates/audio/src/mixed/other.rs around lines 21-26, the code uses
Poll::Pending but Poll is not in scope and the cx parameter is unused; either
bring Poll into scope with use std::task::Poll; or fully-qualify the return as
std::task::Poll::Pending, and mark the context as used to silence the warning by
renaming the parameter to _cx (or add let _ = cx; inside the function). Apply
one of these fixes so the Poll path is resolved and the unused cx warning is
suppressed.
Fixes fastrepl#1371 On macOS 15.x, creating a private aggregate device for speaker audio capture triggers system-wide device change notifications. The DeviceMonitor was reacting to these events during initialization, causing the audio pipeline to restart in a loop and never stabilize. This change adds an initialization_complete flag that prevents device change events from being processed until both mic and speaker streams have successfully initialized. The flag is reset when the user explicitly changes devices to allow proper reinitialization. Root Cause: - PR fastrepl#1471 introduced aggregate device for speaker capture - Creating "private" aggregate device unexpectedly triggers device change events - DeviceMonitor caught in reinitialization loop during startup - Audio pipeline never stabilized, no audio captured Solution: - Added Arc<AtomicBool> to track initialization completion - Device events ignored during initialization phase - Flag set to true after both mic+speaker streams start - Flag reset on explicit device changes for proper reinitialization Testing: - Verified on macOS 15.7.2 (M4 MacBook Pro) - Audio capture now works correctly - No device switching loop observed - Device changes handled gracefully during recording - Success message "audio_streams_initialized" confirms stable pipeline Changes: - Modified: plugins/listener/src/actors/source.rs (+17 lines) - No breaking changes - Backward compatible
No description provided.