Improve desktop recording device selection and stop handling#1780
Improve desktop recording device selection and stop handling#1780richiemcilroy merged 5 commits intomainfrom
Conversation
|
Paragon Review Skipped Hi @richiemcilroy! Your Polarity credit balance is insufficient to complete this review. Please visit https://app.paragon.run to finish your review. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| type MicDetailsCache = Record< | ||
| string, | ||
| { sampleRate: number; channels: number; formats?: MicrophoneFormatInfo[] } | ||
| >; |
There was a problem hiding this comment.
MicDetailsCache type vs actual values mismatch
MicDetailsCache declares sampleRate: number and channels: number as required non-optional fields, but getMicrophoneInfo may return those fields as undefined (consistent with MicrophoneWithDetails at line 34–39 which marks both optional). At runtime, the detail object stored under micDetailsCache[name] can contain undefined in place of a required number, silently propagating stale or missing format data into the stable devices store.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/utils/devices.ts
Line: 73-76
Comment:
**`MicDetailsCache` type vs actual values mismatch**
`MicDetailsCache` declares `sampleRate: number` and `channels: number` as required non-optional fields, but `getMicrophoneInfo` may return those fields as `undefined` (consistent with `MicrophoneWithDetails` at line 34–39 which marks both optional). At runtime, the detail object stored under `micDetailsCache[name]` can contain `undefined` in place of a required `number`, silently propagating stale or missing format data into the stable devices store.
How can I resolve this? If you propose a fix, please make it concise.| fn select_preferred_config( | ||
| configs: &[SupportedStreamConfigRange], | ||
| settings: &MicrophoneDeviceSettings, | ||
| ) -> Option<SupportedStreamConfig> { | ||
| let rate = settings.sample_rate.map(cpal::SampleRate); | ||
|
|
||
| configs | ||
| .iter() | ||
| .find(|config| { | ||
| ffmpeg_sample_format_for(config.sample_format()).is_some() | ||
| && settings | ||
| .channels | ||
| .is_none_or(|channels| config.channels() == channels) | ||
| && rate.is_none_or(|rate| { | ||
| config.min_sample_rate().0 <= rate.0 && config.max_sample_rate().0 >= rate.0 | ||
| }) | ||
| }) | ||
| .map(|config| config.with_sample_rate(rate.unwrap_or_else(|| select_sample_rate(config)))) | ||
| } |
There was a problem hiding this comment.
select_preferred_config silently drops settings on partial mismatch
When settings.channels is set but no config with that exact channel count is found, select_preferred_config returns None and get_usable_device falls through to the fallback path which ignores both requested channels and sample rate entirely. The user's persisted format preferences are silently discarded with no warning log.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/recording/src/feeds/microphone.rs
Line: 410-428
Comment:
**`select_preferred_config` silently drops settings on partial mismatch**
When `settings.channels` is set but no config with that exact channel count is found, `select_preferred_config` returns `None` and `get_usable_device` falls through to the fallback path which ignores both requested channels and sample rate entirely. The user's persisted format preferences are silently discarded with no warning log.
How can I resolve this? If you propose a fix, please make it concise.![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| const extendedInfo = info as typeof info & { | ||
| formats?: MicrophoneFormatInfo[]; | ||
| }; | ||
| const details = { | ||
| sampleRate: info.sampleRate, | ||
| channels: info.channels, | ||
| formats: extendedInfo.formats, | ||
| }; |
There was a problem hiding this comment.
If getMicrophoneInfo now returns formats (looks like MicrophoneInfo was updated), you can drop the cast and just read info.formats.
| const extendedInfo = info as typeof info & { | |
| formats?: MicrophoneFormatInfo[]; | |
| }; | |
| const details = { | |
| sampleRate: info.sampleRate, | |
| channels: info.channels, | |
| formats: extendedInfo.formats, | |
| }; | |
| const details = { | |
| sampleRate: info.sampleRate, | |
| channels: info.channels, | |
| formats: info.formats, | |
| }; |
| .ask(microphone::SetInput { | ||
| label: name, | ||
| settings: None, | ||
| }) |
There was a problem hiding this comment.
Worth keeping the selected mic format on reconnect (sample rate/channels) instead of resetting to None here, otherwise a transient stall can silently switch the stream format away from what the user picked.
| ) -> anyhow::Result<()> { | ||
| match (task_result, muxer_result) { | ||
| (Err(error), _) | (_, Err(error)) => Err(error), | ||
| (_, Ok(Ok(()))) if stop_signal.user_stopped() => Err(anyhow!(PipelineStoppedByUser)), |
There was a problem hiding this comment.
This makes a macOS-sharing-controls stop look like a pipeline failure to any done_fut consumer (e.g. the studio watcher treats required track Err as fatal). If the goal is a clean external stop, consider either returning Ok(()) here, or ensuring upstream explicitly treats PipelineStoppedByUser as non-fatal in completion handling (not just in OutputPipeline::stop()).
|
please re-review the pr @greptileai |
| @@ -2488,6 +2542,17 @@ impl std::error::Error for PipelineDoneError { | |||
| } | |||
| } | |||
|
|
|||
| impl PipelineDoneError { | |||
| pub fn is_caused_by<T>(&self) -> bool | |||
| where | |||
| T: std::error::Error + 'static, | |||
| { | |||
| self.0 | |||
| .chain() | |||
| .any(|cause| cause.downcast_ref::<T>().is_some()) | |||
| } | |||
| } | |||
|
|
|||
| impl OutputPipeline { | |||
| pub fn path(&self) -> &PathBuf { | |||
| &self.path | |||
| @@ -2498,6 +2563,7 @@ impl OutputPipeline { | |||
|
|
|||
| const PIPELINE_STOP_TIMEOUT: Duration = Duration::from_secs(10); | |||
| match tokio::time::timeout(PIPELINE_STOP_TIMEOUT, self.done_fut.clone()).await { | |||
| Ok(Err(error)) if error.is_caused_by::<PipelineStoppedByUser>() => {} | |||
There was a problem hiding this comment.
PipelineStoppedByUser is defined but never placed in an error chain
PipelineStoppedByUser is defined and stop() contains a handler Ok(Err(error)) if error.is_caused_by::<PipelineStoppedByUser>() => {} to suppress it. However PipelineStoppedByUser is never used to produce an error anywhere — mark_user_stopped() only sets an AtomicBool, and resolve_pipeline_completion's guarded arm (_, Ok(Ok(()))) if stop_signal.user_stopped() => Ok(()) returns exactly the same Ok(()) as the unguarded arm below it, making the guard redundant. The is_caused_by check in stop() can therefore never match.
If any pipeline task exits with an error when pipeline_cancel is cancelled after a user stop (e.g. a closed channel or an interrupted muxer write), the recording will fail instead of completing cleanly. To make the suppression mechanism work, the guarded arm would need to return Err(anyhow::Error::new(PipelineStoppedByUser)) rather than Ok(()).
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/recording/src/output_pipeline/core.rs
Line: 2519-2566
Comment:
**`PipelineStoppedByUser` is defined but never placed in an error chain**
`PipelineStoppedByUser` is defined and `stop()` contains a handler `Ok(Err(error)) if error.is_caused_by::<PipelineStoppedByUser>() => {}` to suppress it. However `PipelineStoppedByUser` is never used to produce an error anywhere — `mark_user_stopped()` only sets an `AtomicBool`, and `resolve_pipeline_completion`'s guarded arm `(_, Ok(Ok(()))) if stop_signal.user_stopped() => Ok(())` returns exactly the same `Ok(())` as the unguarded arm below it, making the guard redundant. The `is_caused_by` check in `stop()` can therefore never match.
If any pipeline task exits with an error when `pipeline_cancel` is cancelled after a user stop (e.g. a closed channel or an interrupted muxer write), the recording will fail instead of completing cleanly. To make the suppression mechanism work, the guarded arm would need to return `Err(anyhow::Error::new(PipelineStoppedByUser))` rather than `Ok(())`.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Greptile Summary
This PR wires per-device camera resolution/FPS and microphone sample-rate/channel settings through the Tauri and recording pipelines, defers device startup until the UI needs it, and treats macOS sharing-control stops as clean recording completions rather than failures.
PipelineStoppedByUserdead code:PipelineStoppedByUseris defined andstop()contains a handler for it, but the error type is never placed in any error chain. Theuser_stoppedguard inresolve_pipeline_completionreturns the sameOk(())as the unguarded arm beneath it, and theis_caused_by::<PipelineStoppedByUser>()check instop()can therefore never match. If any pipeline task exits with an error whenpipeline_cancelis cancelled after a user stop (e.g. an interrupted muxer write), the recording will fail rather than completing cleanly.MicDetailsCachenon-optional fields andselect_preferred_configsilent fallback remain open.Confidence Score: 3/5
Safe to merge for settings persistence and UI changes; the user-stop flow works in the clean-exit path but has an incomplete error-suppression mechanism that could surface recording failures on edge-case cancellation errors.
One P1 finding: PipelineStoppedByUser is defined and its handler is wired in stop(), but the error is never actually generated, leaving the is_caused_by guard unreachable. Two P1 findings from prior review rounds also remain unresolved (MicDetailsCache type mismatch, select_preferred_config silent drop).
crates/recording/src/output_pipeline/core.rs — PipelineStoppedByUser never placed in error chain; crates/recording/src/feeds/microphone.rs — silent settings drop on partial mismatch (prior comment)
Important Files Changed
Sequence Diagram
sequenceDiagram participant UI as Desktop UI participant RS as RecordingSettingsStore participant Pipeline as OutputPipeline participant SC as ScreenCaptureSource (macOS) participant PS as PipelineStopSignal UI->>RS: save camera/mic settings (per device key) UI->>Pipeline: start_recording (SetInput with settings) Pipeline->>SC: run video/audio capture loop Note over SC: macOS sharing controls stop SC->>SC: is_user_stop_error(err)? SC->>PS: mark_user_stopped() SC->>Pipeline: pipeline_cancel.cancel() SC-->>Pipeline: break Ok(()) Pipeline->>Pipeline: resolve_pipeline_completion(Ok(()), Ok(Ok(())), &stop_signal) Note over Pipeline: user_stopped guard returns Ok(()) Note over Pipeline: same as unguarded arm — guard is redundant Pipeline-->>UI: done_fut resolves Ok(()) Note over Pipeline: stop() checks is_caused_by PipelineStoppedByUser Note over Pipeline: always false — PipelineStoppedByUser never generatedComments Outside Diff (1)
crates/recording/src/sources/screen_capture/macos.rs, line 1112-1118 (link)The
rebuild_capturerfunction handles audio frames using direct slice indexing —&slice[i * data_bytes_size as usize .. (i + 1) * data_bytes_size as usize]— and callscopy_from_slicewithout checking that the destination length matchesdata_bytes_size. The original capturer callback (built into_sources) usesslice.get()with a bounds check and verifiesdestination.len() == source.len()before copying. If the audio buffer is shorter than expected or the ffmpeg frame plane size differs fromdata_bytes_size, the rebuilt callback will panic, crashing the recording process on capture restart.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "comments + clippy" | Re-trigger Greptile