Refactor audio input feed to use actor #937
Conversation
WalkthroughReplaces the legacy AudioInputFeed with an actor-based MicrophoneFeed and MicrophoneSamples across desktop and recording crates, updates RecordingBaseInputs to own an Option<Arc>, adds a cap-cpal-ffmpeg crate and related conversions, adjusts UI initialization and mutation API, and updates several signatures and dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Desktop UI
participant App as Tauri App
participant Mic as MicrophoneFeed (actor)
participant Stream as CPAL Stream Thread
participant Meter as Audio Meter
participant Rec as Recording Flow
UI->>App: setMicInput(label)
App->>Mic: SetInput { label }
par build_stream
Mic-->>Stream: spawn input stream
Stream-->>Mic: ready / error
Mic-->>App: reply (Ok / Err)
end
Note over App,Meter: subscription
App-->>Meter: provide flume::Receiver<MicrophoneSamples>
Stream--)Mic: on_data(MicrophoneSamples)
Mic--)Meter: broadcast samples
Note over Rec,Mic: recording lock
Rec->>Mic: Lock
Mic-->>Rec: MicrophoneFeedLock (if connected)
Rec->>Rec: build pipelines with Arc<MicrophoneFeedLock>
UI->>App: clear mic
App->>Mic: RemoveInput
Mic-->>Stream: stop / teardown
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src-tauri/src/audio_meter.rs (1)
112-145: samples_to_f64: add 24-bit support and properly center unsigned PCMThe current
samples_to_f64match only covers I8/I16/I32/U8/U16/U32/F32/F64 and falls through to_ => todo!(), which will panic on any unhandled format (e.g. I24) and mis-normalizes unsigned PCM by mapping 0..MAX to 0..1 (introducing a DC offset).Please refactor as follows:
• Add an explicit arm for
SampleFormat::I24before I32 to sign-extend the three bytes into an i32 and divide by 2²³−1.
• Center and normalize all unsigned integer formats:
– U8: let v = u8::from_ne_bytes([data[0]]) as f64;(v - 127.5) / 127.5
– U16: let v = u16::from_ne_bytes([data[0], data[1]]) as f64;(v - 32768.0) / 32767.0
– U32: let v = u32::from_ne_bytes([data[0], data[1], data[2], data[3]]) as f64;(v - 2_147_483_648.0) / 2_147_483_647.0
– (Optionally handle U64 similarly or keep it mapped to 0.0 if your backends never emit it.)
• Replace the wildcard_ => todo!()with a safe fallback (e.g.unreachable!()in release or returning 0.0 + a warning in debug builds) so new or unsupported formats don’t crash.
• Ensure the match remains exhaustive (or add a final arm forSampleFormat::I64/U64if you choose to keep them).
• (Optional) Add a debug log at startup or in a dev build to printsamples.formatso you can confirm which formats your device actually produces.This refactor will prevent runtime panics on 24-bit inputs and eliminate the DC offset in your dBFS calculations.
apps/desktop/src-tauri/src/lib.rs (1)
2219-2234: Avoid awaiting under a write lock in window-destroy path.Inside
WindowEvent::Destroyed,app_state.mic_feed.ask(...).awaithappens while holding a write lock. Clone the actor, drop the guard, then await; reacquire the guard for subsequent state mutations if needed.For example:
- Clone
let mic = app_state.mic_feed.clone();- Record any booleans you need, then
drop(app_state);let _ = mic.ask(microphone::RemoveInput).await;- Reacquire
let app_state = &mut *state.write().await;to mutate the rest.If you want, I can provide a concrete diff that reshapes this block accordingly.
🧹 Nitpick comments (26)
crates/recording/Cargo.toml (1)
48-48: Consider whetherreplace_withis necessary hereIt looks like
replace_with_or_abortis indeed being used incrates/recording/src/feeds/microphone.rs(around thehandlemethod on line 499), so this dependency isn’t unused—however, it may not be needed if you can achieve the same behavior with the standard library:
File and location to review:
- crates/recording/src/feeds/microphone.rs (use of
replace_with_or_abortinasync fn handle)Alternative with
std:
- You can swap out the enum state using
std::mem::takeorstd::mem::replaceplus manual re-assignment, e.g.:let prev = std::mem::take(&mut self.state); self.state = match prev { State::Locked { inner } => State::Open(OpenState { connecting: None, /* … */ }), other => other, };- This approach avoids an extra crate unless you specifically require panic-safe, abort-on-panic semantics.
If you do need the abort-on-panic guarantee that
replace_with_or_abortprovides, it’s fine to keep the crate. In that case, for consistency across the workspace, consider moving thereplace_with = "0.1.8"declaration into the[workspace.dependencies]section of your rootCargo.toml.apps/desktop/src-tauri/Cargo.toml (1)
104-104: Unify kameo versioning via workspace dependenciesWe’ve identified three crates pinning
kameo = "0.17.2"explicitly:
crates/recording/Cargo.toml(line 46)apps/desktop/src-tauri/Cargo.toml(line 104)crates/media/Cargo.toml(line 42)To prevent version drift and duplicate feature resolution, consolidate the version in your workspace root and switch each crate to reference it:
• In your root
Cargo.toml, add a workspace‐scoped dependency:[workspace.dependencies] kameo = "0.17.2"• Then, in each affected crate’s
Cargo.toml, replace the explicit version with a workspace link:[dependencies] - kameo = "0.17.2" + kameo.workspace = trueThis ensures there’s a single source of truth for the
kameoversion across all member crates.crates/cpal-ffmpeg/Cargo.toml (1)
1-12: LGTM: minimal, workspace-aligned crate manifestThe new crate cleanly relies on workspace-managed cpal/ffmpeg and lints. If this crate is internal-only, consider publish = false to avoid accidental publishing. Otherwise, adding license/description/readme is a nice-to-have.
[package] name = "cap-cpal-ffmpeg" version = "0.1.0" edition = "2024" +publish = false +# license = "MIT OR Apache-2.0" +# description = "Bridge CPAL audio buffers to FFmpeg frames" +# readme = "README.md"crates/scap-direct3d/examples/cli.rs (2)
21-24: Confirm intent: border now always shown in example.Switching
is_border_requiredtoSome(true)makes the capture border visible by default. If this was just for local debugging, consider leaving it asNone(use library default) or keeping the earlier behavior in the example to avoid surprising users.
27-31: Surface errors from the frame callback or add an error callback equivalent.The example previously demonstrated error handling via a second callback. With the new one-closure API, any per-frame failure should still be surfaced. Right now, the closure always returns
Ok(()), so errors won’t be demonstrated. Consider logging or propagating failures as an example for users.apps/desktop/src/utils/queries.ts (3)
155-170: Ensure focus action is intentional and non-blocking; consider awaiting or explicitly discarding.
getCurrentWindow().setFocus()is not awaited. If the intent is fire-and-forget, mark it explicitly to avoid unhandled promise noise and make intent clear.- getCurrentWindow().setFocus(); + void getCurrentWindow().setFocus();
163-170: Rollback logic relies on JSON stringification; prefer structural compare.Using
JSON.stringifyto compare{ModelID|DeviceID}works today but is brittle to key-order and future extensions. Safer to compare discriminants/fields explicitly.- if (JSON.stringify(before) === JSON.stringify(model) || !before) { + const same = + (!!before && "ModelID" in before && "ModelID" in (model ?? {}) && before.ModelID === (model as any).ModelID) || + (!!before && "DeviceID" in before && "DeviceID" in (model ?? {}) && before.DeviceID === (model as any).DeviceID); + if (same || !before) {
176-184: Avoid Proxy to expose rawMutate; return a plain object for stability.JS Proxy can interfere with type inference, devtools, and reactivity tooling. Prefer returning an object that spreads the mutation and adds
rawMutate.- return new Proxy( - setCameraInput as typeof setCameraInput & { rawMutate: typeof rawMutate }, - { - get(target, key) { - if (key === "rawMutate") return rawMutate; - return Reflect.get(target, key); - }, - }, - ); + return Object.assign({}, setCameraInput, { rawMutate });apps/desktop/src/routes/(window-chrome)/(main).tsx (1)
256-258: Guard mic initialization with permission state and handle errors.We now eagerly set the mic on mount. If OS permission is denied, this may throw in the Tauri command path. Consider checking
getPermissionsfirst or catching errors to avoid noisy logs and a bad first-run UX.- if (rawOptions.micName) commands.setMicInput(rawOptions.micName); + if (rawOptions.micName) { + commands.setMicInput(rawOptions.micName).catch((e) => { + console.warn("Failed to set mic input on mount:", e); + }); + }crates/cpal-ffmpeg/src/lib.rs (3)
12-14: Validate sample count calculation and frame capacity.
sample_count = bytes.len() / (sample_size * channels)is correct for interleaved buffers, but we should assert the remainder is zero and that the destination frame’s plane/buffer sizes match before copying to avoid potential out-of-bounds on exotic formats.+ debug_assert_eq!(self.bytes().len() % (sample_size * config.channels as usize), 0, "audio buffer size not aligned to frame");
41-44: Set sample rate before data copy or verify it does not affect buffer allocation.If the FFmpeg frame’s internal buffer sizing depends on rate (some wrappers derive line sizes from format only, others consult rate), set the rate before copying. If not required, ignore.
30-31: Clippy: unnecessary cast.
sample_sizeis alreadyusize. Remove the cast.- let plane_size = sample_count * sample_size as usize; + let plane_size = sample_count * sample_size;crates/recording/src/feeds/microphone.rs (2)
283-333: Consider moving stream build to a dedicated runtime or threadpool and tighten error signaling.The raw thread is fine, but:
- If
build_input_stream_rawpanics inside CPAL/host callback,BuildStreamCrashedis indistinguishable from early drop. You already map this, but consider more granular logging around the ready channel.- When the error callback fires, you
actor_ref.kill(). Ensure higher layers expect the actor to die on device errors vs. attempting reconnection.
395-407: Backpressure handling: distinguish Full vs Disconnected and consider dropping or coalescing.
try_senderrors other thanDisconnected(e.g.,Full) are silently ignored, dropping samples without visibility. At minimum, logFulloccasionally, or coalesce samples to the latest to avoid unbounded lag.- if let Err(TrySendError::Disconnected(_)) = sender.try_send(msg.clone()) { + match sender.try_send(msg.clone()) { + Ok(_) => {} + Err(TrySendError::Full(_)) => { + // Optional: log at trace or count metrics to avoid log spam + trace!("Audio sender {} queue full; dropping frame", i); + } + Err(TrySendError::Disconnected(_)) => { + warn!("Audio sender {} disconnected, will be removed", i); + to_remove.push(i); + } + }apps/desktop/src-tauri/src/recording.rs (2)
355-361: Handle microphone lock edge-cases and avoid indefinite waits.Good mapping of
NoInput→ None. Consider:
- Handling a potential
AlreadyLocked(or similar) error by degrading toNonerather than failing the start.- Adding a short timeout to
.ask(...)to avoid hanging if the mic actor stalls.Example refactor:
- use kameo::error::SendError; - let mic_feed = match state.mic_feed.ask(microphone::Lock).await { - Ok(lock) => Some(Arc::new(lock)), - Err(SendError::HandlerError(microphone::LockFeedError::NoInput)) => None, - Err(e) => return Err(e.to_string()), - }; + use kameo::error::SendError; + use tokio::time::{timeout, Duration}; + let mic_feed = match timeout(Duration::from_secs(2), state.mic_feed.ask(microphone::Lock)).await { + Err(_elapsed) => None, // degrade gracefully on timeout + Ok(Ok(lock)) => Some(Arc::new(lock)), + Ok(Err(SendError::HandlerError(microphone::LockFeedError::NoInput))) => None, + // If applicable in your API, treat AlreadyLocked as "unavailable" + Ok(Err(SendError::HandlerError(microphone::LockFeedError::AlreadyLocked))) => None, + Ok(Err(e)) => return Err(e.to_string()), + };If
AlreadyLockedisn’t a thing in your API, keep the first two match arms.
656-657: Don’t block on mic input removal; prefer fire-and-forget or timeout.On teardown, awaiting
RemoveInputcan hang shutdown if the actor is slow/stopped. Prefer a non-blocking send or add a timeout and ignore errors.Example:
- let _ = app.mic_feed.ask(microphone::RemoveInput).await; + use tokio::time::{timeout, Duration}; + let _ = timeout(Duration::from_secs(1), app.mic_feed.ask(microphone::RemoveInput)).await;If Kameo supports a one-way send (no reply), use that instead.
crates/recording/src/lib.rs (1)
42-46: Public API change: lifetime removal and Arc-owned mic lock are sound; add Debug and docs.Moving to owned
Option<Arc<MicrophoneFeedLock>>removes borrowing complexity and fits the actor model. To improve debuggability and clarity, deriveDebugand document ownership semantics.-#[derive(Clone)] +#[derive(Clone, Debug)] pub struct RecordingBaseInputs { pub capture_target: ScreenCaptureTarget, pub capture_system_audio: bool, pub mic_feed: Option<Arc<MicrophoneFeedLock>>, }And consider a brief doc comment on
mic_feedindicating it can be None when mic is unavailable or not selected.crates/recording/src/instant_recording.rs (1)
126-132: Prefer trace-level or structured logging for mic info.Dumping full
AudioInfoat debug may be noisy. Considertrace!or structured fields.- debug!( - "mic audio info: {:#?}", - AudioInfo::from_stream_config(mic_feed.config()) - ); + trace!(mic = ?AudioInfo::from_stream_config(mic_feed.config()), "mic audio info");apps/desktop/src-tauri/src/audio_meter.rs (1)
11-11: Nit: naming clarity for amplitude constant.
MAX_AMPLITUDE_F32actually represents the i16 full-scale magnitude. Consider renaming toMAX_AMPLITUDE_I16to avoid confusion. Low priority.Also applies to: 109-110
crates/recording/src/capture_pipeline.rs (1)
127-131: Minor:*audio.audio_info()relies onCopy; alternatively avoid the deref.If
AudioInfoever stops beingCopy, this will break. Optional alternative:- let sink = audio_mixer.sink(*audio.audio_info()); + let sink = audio_mixer.sink(audio.audio_info().clone());Not required if
AudioInfo: Copyis a stable guarantee in your codebase.Also applies to: 296-301
apps/desktop/src-tauri/src/lib.rs (1)
2016-2037: Keep draining the mic error channel; current task exits after first error.The spawned task returns after the first
recv_async, silently dropping later errors.Apply this diff:
- tokio::spawn(async move { - let Ok(err) = error_rx.recv_async().await else { - return; - }; - - error!("Mic feed actor error: {err}"); - }); + tokio::spawn(async move { + while let Ok(err) = error_rx.recv_async().await { + error!("Mic feed actor error: {err}"); + } + });crates/recording/src/studio_recording.rs (2)
744-748: Consider a slightly larger mic channel buffer.Bound of 8 can drop frames under transient backpressure (disk spikes, scheduler jitter). Studio segments are short-lived but I’d suggest 32–64 for safety, consistent with other audio queues in the codebase.
- let (tx, rx) = flume::bounded(8); + let (tx, rx) = flume::bounded(64);
762-775: Surface encoder errors instead of ignoring them.
mic_encoder.queue_frame(frame.0);return value is ignored; failures will silently discard audio and produce confusing output.Minimal logging:
- mic_encoder.queue_frame(frame.0); + if let Err(e) = mic_encoder.queue_frame(frame.0) { + tracing::error!("microphone encoder error: {e}"); + break; + }Or wire the error to the task result if you want the segment to abort early.
crates/recording/src/sources/audio_input.rs (3)
126-128: Fix log/message text: it’s the microphone feed, not the camera feed.Apply this diff:
- error!("Lost connection with the camera feed"); - break Err("Lost connection with the camera feed".to_string()); + error!("Lost connection with the microphone feed"); + break Err("Lost connection with the microphone feed".to_string());
15-16: Potentially dead code:AudioInputDeviceMapalias is no longer used here.This type alias (and the
Device/SupportedStreamConfigimports) look leftover from the pre-actor design. If it’s not referenced by external modules, consider removing to reduce surface area.
104-117: Ensure correct synchronous actor message API for AddSenderCurrently in crates/recording/src/sources/audio_input.rs (lines 113–115), you’re calling:
let _ = self.feed .ask(microphone::AddSender(tx)) .blocking_send();Per Kameo’s docs,
ask(...).blocking_send()will block the current thread until the actor both accepts the message and sends a reply—not just until the mailbox has capacity (docs.page). SinceAddSenderonly registers the sender and doesn’t return any useful data, this extra blocking can stall your audio thread.Consider using the fire-and-forget API instead:
- let _ = self.feed - .ask(microphone::AddSender(tx)) - .blocking_send(); + let _ = self.feed + .tell(microphone::AddSender(tx)) + .blocking_send();This variant blocks only until the message is enqueued (mailbox capacity), avoiding waiting on a reply (docs.page).
• File: crates/recording/src/sources/audio_input.rs
• Lines: 113–115Also, rather than discarding the
SendError, consider logging or handling it to avoid silent failures when adding a sender.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
apps/cli/src/record.rs(1 hunks)apps/desktop/src-tauri/Cargo.toml(1 hunks)apps/desktop/src-tauri/src/audio_meter.rs(3 hunks)apps/desktop/src-tauri/src/lib.rs(8 hunks)apps/desktop/src-tauri/src/recording.rs(3 hunks)apps/desktop/src/routes/(window-chrome)/(main).tsx(2 hunks)apps/desktop/src/utils/queries.ts(2 hunks)crates/cpal-ffmpeg/Cargo.toml(1 hunks)crates/cpal-ffmpeg/src/lib.rs(1 hunks)crates/media/Cargo.toml(1 hunks)crates/recording/Cargo.toml(1 hunks)crates/recording/src/capture_pipeline.rs(6 hunks)crates/recording/src/feeds/audio_input.rs(0 hunks)crates/recording/src/feeds/microphone.rs(1 hunks)crates/recording/src/feeds/mod.rs(1 hunks)crates/recording/src/instant_recording.rs(6 hunks)crates/recording/src/lib.rs(2 hunks)crates/recording/src/sources/audio_input.rs(4 hunks)crates/recording/src/sources/screen_capture/macos.rs(0 hunks)crates/recording/src/studio_recording.rs(9 hunks)crates/scap-direct3d/examples/cli.rs(1 hunks)packages/ui-solid/src/auto-imports.d.ts(1 hunks)
💤 Files with no reviewable changes (2)
- crates/recording/src/sources/screen_capture/macos.rs
- crates/recording/src/feeds/audio_input.rs
🧰 Additional context used
🧬 Code graph analysis (12)
apps/desktop/src-tauri/src/recording.rs (1)
crates/recording/src/studio_recording.rs (1)
new(581-604)
crates/cpal-ffmpeg/src/lib.rs (2)
apps/desktop/src/utils/tauri.ts (1)
Audio(359-364)crates/media-info/src/lib.rs (1)
sample_size(92-94)
apps/desktop/src/utils/queries.ts (1)
apps/desktop/src/utils/tauri.ts (3)
DeviceOrModelID(516-516)commands(5-305)setCameraInput(9-11)
apps/desktop/src-tauri/src/audio_meter.rs (1)
crates/audio/src/audio_data.rs (1)
samples(166-168)
crates/recording/src/feeds/microphone.rs (1)
crates/media-info/src/lib.rs (2)
ffmpeg_sample_format_for(256-266)from_stream_config(46-64)
apps/desktop/src/routes/(window-chrome)/(main).tsx (1)
apps/desktop/src/utils/tauri.ts (1)
commands(5-305)
crates/recording/src/lib.rs (1)
apps/desktop/src/utils/tauri.ts (1)
ScreenCaptureTarget(685-688)
crates/recording/src/studio_recording.rs (2)
crates/recording/src/capture_pipeline.rs (1)
create_screen_capture(373-394)crates/recording/src/cursor.rs (1)
spawn_cursor_recorder(38-188)
crates/recording/src/instant_recording.rs (3)
crates/media-info/src/lib.rs (1)
from_stream_config(46-64)crates/recording/src/studio_recording.rs (1)
new(581-604)crates/recording/src/capture_pipeline.rs (3)
make_instant_mode_pipeline(34-46)make_instant_mode_pipeline(109-224)make_instant_mode_pipeline(273-359)
crates/recording/src/sources/audio_input.rs (1)
crates/media-info/src/lib.rs (1)
from_stream_config(46-64)
crates/recording/src/capture_pipeline.rs (1)
crates/recording/src/sources/audio_mixer.rs (1)
sink(22-28)
apps/desktop/src-tauri/src/lib.rs (2)
crates/recording/src/studio_recording.rs (1)
new(581-604)apps/desktop/src-tauri/src/audio_meter.rs (2)
new(46-52)spawn_event_emitter(18-36)
🪛 GitHub Check: Clippy
crates/cpal-ffmpeg/src/lib.rs
[warning] 30-30: casting to the same type is unnecessary (usize -> usize)
warning: casting to the same type is unnecessary (usize -> usize)
--> crates/cpal-ffmpeg/src/lib.rs:30:49
|
30 | let plane_size = sample_count * sample_size as usize;
| ^^^^^^^^^^^^^^^^^^^^ help: try: sample_size
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
= note: #[warn(clippy::unnecessary_cast)] on by default
⏰ 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: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (20)
packages/ui-solid/src/auto-imports.d.ts (1)
6-6: Ignore generated auto-imports.d.ts via .biomeignoreThe path
packages/ui-solid/src/auto-imports.d.tshas been added to your.biomeignore, so Biome will now skip linting this generated file. You can safely remove the inline// biome-ignore lint: disabledirective from the generator output (or update the generator to omit it), since it’s no longer needed.
apps/desktop/src/utils/queries.ts (1)
172-175: Double-check Solid Query API usage: useMutation vs createMutation.If you upgraded TanStack Solid Query to v5+,
useMutationis fine; otherwise, v4 typically usescreateMutation. Make sure the package version aligns with this import to avoid runtime/reactivity pitfalls.Would you like me to scan the repo to confirm the installed version and usages of
useMutationvscreateMutation?apps/desktop/src/routes/(window-chrome)/(main).tsx (1)
884-893: LGTM: audio meter only when mic selected.Showing the level indicator only when a mic is selected reduces confusion and visual noise.
crates/cpal-ffmpeg/src/lib.rs (1)
10-26: I’ve initiated a search for theffmpeg_sample_format_forfunction to verify its mapping of CPAL formats to FFmpeg sample layouts. I’ll use the results to confirm whether the review’s concern about a Packed vs. Planar mismatch is valid.crates/recording/src/feeds/microphone.rs (2)
441-450: Lock handoff is clean; AudioInfo derivation aligns with media-info.Good use of
AudioInfo::from_stream_config(&config)and returning theActorRefviaDereffor ergonomic use, plusDrop-based unlock.
496-509: Unlock retains the attached stream for reuse; good state transition.The
replace_with_or_abortpattern here is clear and avoids intermediate inconsistent states.crates/recording/src/feeds/mod.rs (1)
2-2: I've requested scripts to verify there are no leftoveraudio_inputfiles or references specifically within thefeedsmodule. Once we confirm that, we can close out this check.apps/desktop/src-tauri/src/recording.rs (1)
362-366: LGTM: correct propagation of base inputs to recording actors.Passing
capture_target,capture_system_audio, and the optionalmic_feedaligns with the newRecordingBaseInputs.crates/recording/src/instant_recording.rs (4)
117-119: Signature update aligns with Arc-owned mic feed.Accepting
Option<Arc<MicrophoneFeedLock>>at the pipeline boundary is consistent with the new pipeline builder expectations.
140-144: LGTM: correct propagation of optional mic to pipeline builder.Passing the
Option<Arc<MicrophoneFeedLock>>through matchesMakeCapturePipeline::make_instant_mode_pipeline’s signature.
194-199: LGTM: forwarding cloned mic lock to pipeline.Cloning the
Arcbefore passing into the pipeline is correct; ownership remains shared with the handle.
161-165: No internal usages—confirm external impact and bump versionI searched the repo for both
spawn_instant_recording_actorandRecordingBaseInputsand didn’t find any remaining callsites using the old lifetime-parameterized signature. Internally all references appear updated.However, since dropping the lifetime parameter on
spawn_instant_recording_actorand switching to an ownedRecordingBaseInputsis an API-breaking change, please:
- Verify any downstream or external crates consuming this function have been updated accordingly.
- Bump the crate version (per SemVer, e.g. increment the major version) and note the breaking change in the CHANGELOG.
apps/desktop/src-tauri/src/audio_meter.rs (1)
18-21: API switch to Flume + MicrophoneSamples looks correct.Receiver type and loop semantics align with the new actor-driven mic feed. No issues spotted here.
crates/recording/src/capture_pipeline.rs (1)
34-47: Migration toArc<MicrophoneFeedLock>is consistent and sound.Trait signatures and both macOS/Windows impls correctly adopt
Option<Arc<MicrophoneFeedLock>>. This aligns with the new actor lock semantics and avoids lifetime coupling.Also applies to: 109-121, 273-286
apps/desktop/src-tauri/src/lib.rs (3)
116-118: Good: store actor ref inAppand skip serialization.Using
ActorRef<MicrophoneFeed>on state and#[serde(skip)]is the right shape for the new mic architecture.
1111-1112: Device list routing looks good.Delegating to
MicrophoneFeed::list()guarded by the permission check is appropriate.
2185-2186: Good wiring: audio meter now listens to mic samples.Bounded channel size (8) is reasonable for UI metering.
crates/recording/src/studio_recording.rs (2)
129-135: API change: ownedRecordingBaseInputsDropping the lifetime parameter simplifies ownership across the actor boundary. Looks good.
158-161: Mic info logging is correct.Using the lock’s
audio_info()for logging avoids rebuilding configs.crates/recording/src/sources/audio_input.rs (1)
26-41: Switch toArc<MicrophoneFeedLock>and derivingAudioInfofrom stream config looks good.The start time baseline is computed once and used consistently for timestamping.
| pub struct MicrophoneSamples { | ||
| pub data: Vec<u8>, | ||
| pub format: SampleFormat, | ||
| pub info: InputCallbackInfo, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid per-recipient cloning of audio buffers; switch to Arc<[u8]> or Bytes.
MicrophoneSamples is cloned per sender; cloning a Vec<u8> duplicates the buffer N times. Use a shared backing store to reduce allocations and memory bandwidth.
- pub struct MicrophoneSamples {
- pub data: Vec<u8>,
+ pub struct MicrophoneSamples {
+ pub data: std::sync::Arc<[u8]>,
pub format: SampleFormat,
pub info: InputCallbackInfo,
}And in the callback:
- data: data.bytes().to_vec(),
+ data: std::sync::Arc::<[u8]>::from(data.bytes().to_vec().into_boxed_slice()),Or adopt bytes::Bytes for zero-cost cloning if the ecosystem already uses it.
I can provide a follow-up patch converting the code paths to Arc<[u8]> end-to-end, including consumers.
📝 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 struct MicrophoneSamples { | |
| pub data: Vec<u8>, | |
| pub format: SampleFormat, | |
| pub info: InputCallbackInfo, | |
| } | |
| // crates/recording/src/feeds/microphone.rs | |
| pub struct MicrophoneSamples { | |
| pub data: std::sync::Arc<[u8]>, | |
| pub format: SampleFormat, | |
| pub info: InputCallbackInfo, | |
| } |
🤖 Prompt for AI Agents
In crates/recording/src/feeds/microphone.rs around lines 20 to 24,
MicrophoneSamples currently owns a Vec<u8> which gets cloned per recipient
causing N full buffer copies; change the buffer field to a shared,
cheaply-clonable backing store (e.g. Arc<[u8]> or bytes::Bytes) and update
creation sites to convert the produced Vec into
Arc::from(vec.into_boxed_slice()) or Bytes::from(vec) so clones only clone the
Arc/Bytes handle; update all callsites/traits that expect &Vec<u8> to accept
&[u8] (or Bytes/Arc<[u8]> references), ensure Send+Sync bounds remain satisfied,
and replace any .clone() of the Vec with a cheap clone of the Arc/Bytes so
sending to multiple recipients no longer duplicates the underlying audio buffer.
| c.min_sample_rate().0 <= 48000 && c.max_sample_rate().0 <= 48000 | ||
| }) | ||
| .find(|c| ffmpeg_sample_format_for(c.sample_format()).is_some()) | ||
| }) | ||
| .and_then(|config| { |
There was a problem hiding this comment.
Bug: device filter incorrectly excludes most devices that support 48 kHz.
c.min_sample_rate().0 <= 48000 && c.max_sample_rate().0 <= 48000 filters out devices whose max > 48k, even if they support 48k. You likely want ranges that include 48k.
- .filter(|c| {
- c.min_sample_rate().0 <= 48000 && c.max_sample_rate().0 <= 48000
- })
+ .filter(|c| {
+ let min = c.min_sample_rate().0;
+ let max = c.max_sample_rate().0;
+ min <= 48_000 && max >= 48_000
+ })📝 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.
| c.min_sample_rate().0 <= 48000 && c.max_sample_rate().0 <= 48000 | |
| }) | |
| .find(|c| ffmpeg_sample_format_for(c.sample_format()).is_some()) | |
| }) | |
| .and_then(|config| { | |
| .filter(|c| { | |
| let min = c.min_sample_rate().0; | |
| let max = c.max_sample_rate().0; | |
| min <= 48_000 && max >= 48_000 | |
| }) | |
| .find(|c| ffmpeg_sample_format_for(c.sample_format()).is_some()) |
🤖 Prompt for AI Agents
In crates/recording/src/feeds/microphone.rs around lines 119 to 123, the device
filter currently uses c.min_sample_rate().0 <= 48000 && c.max_sample_rate().0 <=
48000 which incorrectly excludes devices whose max sample rate is above 48 kHz;
change the condition to check that 48_000 falls within the device's supported
range (i.e., c.min_sample_rate().0 <= 48_000 && c.max_sample_rate().0 >= 48_000)
so devices that support 48 kHz even with a higher max are included.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/desktop/src-tauri/src/lib.rs (2)
220-239: Good: no lock held across await inset_mic_inputCloning the actor under a read lock, then dropping the guard before awaits, resolves the starvation/deadlock risk noted previously.
2225-2227: Fix: write-lock held across await in window destroy path
app_state(a write guard onArc<RwLock<App>>) is held while awaitingmic_feed.ask(...). This can block unrelated commands that need the same lock and lead to deadlocks/starvation under load. Clone the actor and compute the condition under the lock, release it, then await and finalize.- let state = app.state::<Arc<RwLock<App>>>(); - let app_state = &mut *state.write().await; - - if !app_state.is_recording_active_or_pending() { - let _ = - app_state.mic_feed.ask(microphone::RemoveInput).await; - app_state.camera_feed.take(); - - if let Some(camera) = CapWindowId::Camera.get(&app) { - let _ = camera.close(); - } - } + let state = app.state::<Arc<RwLock<App>>>(); + // Extract needed data while holding the lock, then drop it before awaiting. + let (should_clear, mic) = { + let mut app_state = state.write().await; + let should_clear = !app_state.is_recording_active_or_pending(); + let mic = app_state.mic_feed.clone(); + if should_clear { + app_state.camera_feed.take(); + } + (should_clear, mic) + }; + + if should_clear { + let _ = mic.ask(microphone::RemoveInput).await; + if let Some(camera) = CapWindowId::Camera.get(&app) { + let _ = camera.close(); + } + }
🧹 Nitpick comments (3)
apps/desktop/src-tauri/src/lib.rs (3)
116-118: Markmic_feedas non-IPC type explicitly
#[serde(skip)]prevents serialization, but to avoid Specta attempting to derive a TS type forActorRef<...>, also mark it with#[specta(skip)]for clarity and to future-proof changes.#[serde(skip)] + #[specta(skip)] mic_feed: ActorRef<feeds::microphone::MicrophoneFeed>,
1104-1112: Return a deterministic, sorted device listFor stable UI and testability, sort device names before returning. If
MicrophoneFeed::list()isn’t ordered, consumers will otherwise see non-deterministic ordering.- Ok(MicrophoneFeed::list().keys().cloned().collect()) + let mut devices: Vec<_> = MicrophoneFeed::list().keys().cloned().collect(); + devices.sort_unstable(); + Ok(devices)
2016-2037: Harden mic actor wiring: drain error channel and logAddSenderfailures
- The error handler only logs the first error and then exits; drain the channel in a loop so subsequent errors aren’t lost.
- Don’t ignore a failed
AddSender—log it so issues in the audio meter pipeline are observable.- let (mic_samples_tx, mic_samples_rx) = flume::bounded(8); + let (mic_samples_tx, mic_samples_rx) = flume::bounded(8); let mic_feed = { let (error_tx, error_rx) = flume::bounded(1); - let mic_feed = MicrophoneFeed::spawn(MicrophoneFeed::new(error_tx)); + let mic_feed = MicrophoneFeed::spawn(MicrophoneFeed::new(error_tx)); - // TODO: make this part of a global actor one day - tokio::spawn(async move { - let Ok(err) = error_rx.recv_async().await else { - return; - }; - - error!("Mic feed actor error: {err}"); - }); + // TODO: make this part of a global actor one day + tokio::spawn(async move { + while let Ok(err) = error_rx.recv_async().await { + error!("Mic feed actor error: {err}"); + } + }); - let _ = mic_feed - .ask(feeds::microphone::AddSender(mic_samples_tx)) - .await; + if let Err(e) = mic_feed + .ask(feeds::microphone::AddSender(mic_samples_tx)) + .await + { + error!("Failed to add mic_samples sender to mic feed: {e}"); + } mic_feed }; - audio_meter::spawn_event_emitter(app.clone(), mic_samples_rx); + audio_meter::spawn_event_emitter(app.clone(), mic_samples_rx);Also applies to: 2185-2186
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/desktop/src-tauri/src/lib.rs(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/desktop/src-tauri/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use tauri_specta on the Rust side for IPC (typed commands/events) and emit events via generated types
Files:
apps/desktop/src-tauri/src/lib.rs
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/lib.rs (3)
apps/desktop/src-tauri/src/audio_meter.rs (2)
new(46-52)spawn_event_emitter(18-36)crates/recording/src/studio_recording.rs (1)
new(581-604)crates/editor/src/editor_instance.rs (1)
new(34-96)
⏰ 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)
- 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 (1)
apps/desktop/src-tauri/src/lib.rs (1)
30-39: Actor-based microphone integration imports look correct
- Bringing kameo::Actor into scope enables method-style
MicrophoneFeed::spawn(...);ActorRefis used as expected.- IPC remains typed via
tauri_spectamacros and collected commands/events, aligning with the repo guideline forapps/desktop/src-tauri/**/*.rs.Also applies to: 42-48, 53-56, 61-72, 81-85, 87-88
Converts the audio input feed (now microphone feed) to be a Kameo actor, making the logic of initialisation, selection, and locking much more correct and easy to follow.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores