New scap-* crates recording engine #922
Conversation
WalkthroughRepository-wide rewrite replacing legacy cap_media screen/window and clocked pipeline code with cap_displays and cap_recording subsystems; adds DisplayId/WindowId and LogicalBounds/PhysicalBounds types, platform-specific capture backends (macOS SCK, Windows Direct3D), CPAL↔FFmpeg bridges, callback-based editor frames, and many public API/type migrations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Desktop UI (TS)
participant Tauri as Tauri Commands
participant Rec as cap_recording (tauri)
participant Disp as cap_displays
participant SC as ScreenCapture Backend (SCK / Direct3D)
participant Pipe as Pipeline
participant Enc as Encoder/Muxer
participant Edit as Editor (frame_cb)
User->>UI: Click "Start Recording" (Display/Window/Area)
UI->>Tauri: startRecording(captureTarget)
Tauri->>Rec: create_screen_capture(target,...)
Rec->>Disp: list_displays/list_windows / from_id
Disp-->>Rec: Display/Window (Option sizes/bounds)
Rec->>SC: init backend (SCK or Direct3D) + optional audio capturer
SC-->>Rec: ready
Rec->>Pipe: Pipeline::builder().spawn_source(...)
Pipe-->>SC: start capture
SC-->>Pipe: video frames (+ audio)
Pipe->>Enc: encode/mux/write
Pipe->>Edit: frame_cb(RenderedFrame)
Edit-->>UI: update preview
alt Stop
User->>UI: Stop
UI->>Tauri: stopRecording()
Tauri->>Pipe: shutdown
Pipe->>SC: StopCapturing
Pipe-->>Tauri: finished
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Possibly related PRs
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 (
|
Brendonovich
force-pushed
the
scap-crate
branch
from
a30d883 to
8b773dc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (29)
crates/rendering/src/decoder/ffmpeg.rs (1)
81-88: Refactor FFmpegDecoder::spawn to use OS-appropriate HW devices with a software fallbackThe current implementation in
crates/rendering/src/decoder/ffmpeg.rsunconditionally selectsD3D12VAon all non-macOS targets—this will fail on Linux (and older Windows GPUs) sinceAV_HWDEVICE_TYPE_D3D12VAisn’t available there. We should:
- Pick the correct HW device per platform.
- Gracefully fall back to software decoding if hardware initialization errors.
Affected location:
- File:
crates/rendering/src/decoder/ffmpeg.rs- Method:
FfmpegDecoder::spawn, around the call tocap_video_decode::FFmpegDecoder::newSuggested change:
- let mut this = cap_video_decode::FFmpegDecoder::new( - path, - Some(if cfg!(target_os = "macos") { - AVHWDeviceType::AV_HWDEVICE_TYPE_VIDEOTOOLBOX - } else { - AVHWDeviceType::AV_HWDEVICE_TYPE_D3D12VA - }), - )?; + // Choose the best HW device per-OS; on failure, fall back to software decoding. + let hw_type = if cfg!(target_os = "macos") { + AVHWDeviceType::AV_HWDEVICE_TYPE_VIDEOTOOLBOX + } else if cfg!(target_os = "windows") { + AVHWDeviceType::AV_HWDEVICE_TYPE_D3D12VA + } else { + // Linux and other Unix-like systems: VAAPI is broadly supported. + AVHWDeviceType::AV_HWDEVICE_TYPE_VAAPI + }; + + let mut this = cap_video_decode::FFmpegDecoder::new(path.clone(), Some(hw_type)) + .or_else(|e| { + debug!("HW decode ({hw_type:?}) init failed: {e}. Falling back to software."); + cap_video_decode::FFmpegDecoder::new(path, None) + })?;Notes:
- Linux is supported via the FFmpeg path in
decoder/mod.rs(non-macOS branch), so VAAPI is an appropriate default.- If desired, you can additionally probe NVDEC (CUDA) on Linux before falling back to VAAPI, or try
D3D11VAon Windows prior to software decoding.- No additional
#[cfg]gating is needed—this logic will apply correctly on each platform.apps/desktop/src-tauri/src/audio_meter.rs (2)
109-142: Fix unsigned PCM normalization (U8/U16/U32/U64) to center at 0 before converting.Currently unsigned formats are divided by MAX (0..1 range). That skews the signal positive and overstates levels vs signed/float paths. Map to [-1.0, 1.0] first: value_norm = 2*(value/MAX) - 1.
Apply this refactor to the match arms for unsigned formats:
- SampleFormat::U8 => u8::from_ne_bytes([data[0]]) as f64 / u8::MAX as f64, + SampleFormat::U8 => (2.0 * (u8::from_ne_bytes([data[0]]) as f64 / u8::MAX as f64) - 1.0), - SampleFormat::U16 => u16::from_ne_bytes([data[0], data[1]]) as f64 / u16::MAX as f64, + SampleFormat::U16 => (2.0 * (u16::from_ne_bytes([data[0], data[1]]) as f64 / u16::MAX as f64) - 1.0), - SampleFormat::U32 => { - u32::from_ne_bytes([data[0], data[1], data[2], data[3]]) as f64 / u32::MAX as f64 - } + SampleFormat::U32 => (2.0 * (u32::from_ne_bytes([data[0], data[1], data[2], data[3]]) as f64 / u32::MAX as f64) - 1.0), - SampleFormat::U64 => { - u64::from_ne_bytes([ - data[0], data[1], data[2], data[3], data[4], data[5], data[6], data[7], - ]) as f64 - / u64::MAX as f64 - } + SampleFormat::U64 => (2.0 * (u64::from_ne_bytes([ + data[0], data[1], data[2], data[3], data[4], data[5], data[6], data[7], + ]) as f64 / u64::MAX as f64) - 1.0),This brings unsigned formats in line with signed/float paths and yields correct dBFS calculations.
140-141: Avoidtodo!()in production paths; gracefully handle unknown formats.A
todo!()here will panic at runtime if CPAL introduces/returns an unhandled format. Prefer a safe fallback (e.g., emit 0.0) and optionallydebug_assert!in debug builds.- _ => todo!(), + _ => { + // Unknown/unsupported format; treat as silence to avoid runtime panics. + #[cfg(debug_assertions)] + debug_assert!(false, "Unsupported SampleFormat in samples_to_f64"); + 0.0 + },apps/desktop/src/utils/createPresets.ts (2)
33-35: Off-by-one when marking the newly added preset as default.After
push, the new preset’s index islength - 1, notlength.- store.default = preset.default ? store.presets.length : store.default; + store.default = preset.default ? store.presets.length - 1 : store.default;
37-44: Default index maintenance on deletion is incomplete.Only adjusts when deleting the last element; doesn’t handle:
- deleting the current default (should move to a neighbor),
- deleting an earlier index (should decrement default when
default > index),- deleting the sole preset (should set default to
null).updatePresets((store) => { store.presets.splice(index, 1); - store.default = - index > store.presets.length - 1 - ? store.presets.length - 1 - : store.default; + if (store.presets.length === 0) { + store.default = null; + return; + } + // If we deleted the default, pick the previous item when possible, else the new last. + if (store.default === index) { + store.default = Math.min(index, store.presets.length - 1); + } else if (store.default !== null && store.default > index) { + // Shift right-side indices left by one. + store.default = store.default - 1; + } })crates/recording/src/pipeline/control.rs (1)
25-47:last()is not actually non-blocking whenlast_valueisNoneorShutdown.
last()callsblocking_last_if(false), but your else-branch blocks onrecv()for all cases exceptSome(Play). That can hang a non-blocking poll.Make blocking conditional only on
should_block:- pub fn blocking_last_if(&mut self, should_block: bool) -> Option<Control> { - match self.last_value { - Some(Control::Play) if !should_block => { - // Only peek for a new signal, else relinquish control to the caller - match self.receiver.try_recv() { - Ok(control) => { - debug!("Received new signal: {control:?}"); - self.last_value = Some(control) - } - Err(TryRecvError::Empty) => {} - Err(TryRecvError::Disconnected) => self.last_value = None, - }; - self.last_value - } - _ => { - // For all else, block until a signal is sent. - // TODO: Maybe also spin down until the signal is different from the last value we have? - self.last_value = self.receiver.recv().ok(); - self.last_value - } - } - } + pub fn blocking_last_if(&mut self, should_block: bool) -> Option<Control> { + if should_block { + self.last_value = self.receiver.recv().ok(); + return self.last_value; + } + match self.receiver.try_recv() { + Ok(control) => { + debug!("Received new signal: {control:?}"); + self.last_value = Some(control); + } + Err(TryRecvError::Empty) => {} + Err(TryRecvError::Disconnected) => self.last_value = None, + } + self.last_value + }crates/recording/src/pipeline/audio_buffer.rs (2)
55-58: BUG: Packed branch slices by samples, not bytes (drops 3/4 of data).frame.data(0) is a byte slice; the upper bound must be in bytes (samples × channels × 4). Current code truncates to 1/4 the intended length.
- self.data[0].extend(unsafe { - cast_bytes_to_f32_slice( - &frame.data(0)[0..frame.samples() * frame.channels() as usize], - ) - }); + let bytes_per_sample = std::mem::size_of::<f32>(); + let needed = frame.samples() * frame.channels() as usize * bytes_per_sample; + let plane0 = frame.data(0); + let len = needed.min(plane0.len()); + self.data[0].extend(unsafe { cast_bytes_to_f32_slice(&plane0[..len]) });
62-95: Planar input is dropped in next_frame; packed-only output path ignores channels > 0.self.frame is constructed as F32 packed, so the planar branch here is dead code. When consume() ingests planar frames, samples for channels 1..N accumulate but are never interleaved into the output buffer. Also, readiness checks rely on channel 0 only, which is incorrect for planar input.
Refactor next_frame to:
- Detect whether input is planar (any channel queue beyond 0 is non-empty).
- Compute availability based on planar vs packed.
- Interleave planar channels into the packed output frame.
- pub fn next_frame(&mut self, drain: bool) -> Option<&FFAudio> { - if self.is_empty() { - return None; - } - - if !drain && self.len() < self.frame_size * self.config.channels { - return None; - } - - let actual_samples_per_channel = if drain { - (self.len() / self.config.channels).min(self.frame_size) - } else { - self.frame_size - }; - - if self.frame.is_planar() { - for channel in 0..self.config.channels { - for (index, byte) in self.data[channel] - .drain(0..actual_samples_per_channel) - .enumerate() - { - self.frame.plane_data_mut(channel)[index * 4..(index + 1) * 4] - .copy_from_slice(&byte.to_ne_bytes()); - } - } - } else { - for (index, byte) in self.data[0] - .drain(0..actual_samples_per_channel * self.config.channels) - .enumerate() - { - self.frame.plane_data_mut(0)[index * 4..(index + 1) * 4] - .copy_from_slice(&byte.to_ne_bytes()); - } - } - - Some(&self.frame) - } + pub fn next_frame(&mut self, drain: bool) -> Option<&FFAudio> { + let channels = self.config.channels; + let is_planar_input = channels > 1 && self.data[1..].iter().any(|q| !q.is_empty()); + + // Determine available samples across all channels. + let available = if is_planar_input { + // Minimum per-channel samples, interleaved on write. + self.data.iter().map(|q| q.len()).min().unwrap_or(0) * channels + } else { + // Interleaved samples already in data[0]. + self.data[0].len() + }; + + if available == 0 { + return None; + } + if !drain && available < self.frame_size * channels { + return None; + } + + let actual_samples_per_channel = if drain { + (available / channels).min(self.frame_size) + } else { + self.frame_size + }; + + // Always write packed into plane 0 (self.frame was created as packed). + let dst = self.frame.plane_data_mut(0); + if is_planar_input { + // Interleave from per-channel queues into packed output. + let mut write_idx = 0; + for i in 0..actual_samples_per_channel { + for ch in 0..channels { + let sample = self.data[ch].pop_front().expect("checked availability"); + let off = write_idx * 4; + dst[off..off + 4].copy_from_slice(&sample.to_ne_bytes()); + write_idx += 1; + } + } + } else { + // Packed input: drain from data[0] straight into packed output. + for (idx, sample) in self + .data[0] + .drain(0..actual_samples_per_channel * channels) + .enumerate() + { + let off = idx * 4; + dst[off..off + 4].copy_from_slice(&sample.to_ne_bytes()); + } + } + + Some(&self.frame) + }Optional: adjust is_empty() and len() to be topology-aware or remove them to avoid misuse elsewhere.
apps/desktop/src/routes/editor/ExportDialog.tsx (2)
134-147: Fix payload typing: current assertion hides a missingqualityfor GIF and sendscompressionfor GIFThe cast to
(Mp4ExportSettings & GifExportSettings) & { format: "Mp4" | "Gif" }paperovers that the GIF payload lacks the requiredqualityfield and includes an MP4-onlycompression. Build the discriminated union explicitly and type it asExportSettings.-const exportWithSettings = (onProgress: (progress: FramesRendered) => void) => - exportVideo( - projectPath, - { - format: settings.format, - fps: settings.fps, - resolution_base: { - x: settings.resolution.width, - y: settings.resolution.height, - }, - compression: settings.compression, - } as (Mp4ExportSettings & GifExportSettings) & { format: "Mp4" | "Gif" }, - onProgress, - ); +const exportWithSettings = (onProgress: (progress: FramesRendered) => void) => { + const base = { + fps: settings.fps, + resolution_base: { + x: settings.resolution.width, + y: settings.resolution.height, + }, + } as const; + + const payload: ExportSettings = + settings.format === "Mp4" + ? { + ...base, + format: "Mp4", + compression: settings.compression, + } + : { + ...base, + format: "Gif", + // TODO: wire a real quality control; null lets backend pick a default. + quality: null, + }; + + return exportVideo(projectPath, payload, onProgress); +};
524-529: Bug: wrong predicate when validating allowed GIF FPS values
every(v => v.value === settings.fps)is almost always false. You likely meant “if current FPS is not one of the allowed GIF options, reset to 15”.- if ( - option.value === "Gif" && - GIF_FPS_OPTIONS.every( - (v) => v.value === settings.fps, - ) - ) + if ( + option.value === "Gif" && + GIF_FPS_OPTIONS.every((v) => v.value !== settings.fps) + ) newSettings.fps = 15;Alternatively:
if (option.value === "Gif" && !GIF_FPS_OPTIONS.some(v => v.value === settings.fps)) { ... }.crates/ffmpeg-hw-device/src/lib.rs (1)
80-86: Critical: bitmask test is incorrect — use!= 0, not== 1
methodsis a bitmask; comparing& FLAG == 1fails whenFLAG != 1. This can incorrectly report “no hw config”.- let Some(hw_config) = codec.hw_configs().find(|&config| { - (*config).device_type == device_type - && (*config).methods & (AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX as i32) == 1 - }) else { + let Some(hw_config) = codec.hw_configs().find(|&config| { + (*config).device_type == device_type + && ((*config).methods & (AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX as i32)) != 0 + }) else { return Err("no hw config"); };apps/desktop/src-tauri/src/camera_legacy.rs (1)
18-41: Bug: unconditional upscale to 1280-wide when only converting formatCurrent logic sets output to 1280x(AR) whenever format != RGBA, even if the input is already ≤ 1280x720. That upscales small frames unnecessarily (quality + perf regression). Also, when only height > 720, you still force width=1280, which can upscale width too.
Fix: separate “need format convert” from “need resize”, and compute output size with a max-dimension clamp preserving aspect ratio. Rebuild the scaler when either input or output params change.
Apply:
- if frame.format() != Pixel::RGBA || frame.width() > 1280 || frame.height() > 720 { - let converter = match &mut converter { - Some((format, converter)) - if *format == frame.format() - && converter.input().width == frame.width() - && converter.input().height == frame.height() => - { - converter - } - _ => { - &mut converter - .insert(( - frame.format(), - ffmpeg::software::scaling::Context::get( - frame.format(), - frame.width(), - frame.height(), - Pixel::RGBA, - 1280, - (1280.0 / (frame.width() as f64 / frame.height() as f64)) - as u32, - ffmpeg::software::scaling::flag::Flags::FAST_BILINEAR, - ) - .unwrap(), - )) - .1 - } - }; + let need_format = frame.format() != Pixel::RGBA; + let need_resize = frame.width() > 1280 || frame.height() > 720; + if need_format || need_resize { + // Compute target size: clamp to 1280x720 while preserving aspect ratio. + let (out_w, out_h) = if need_resize { + let wr = 1280.0 / frame.width() as f64; + let hr = 720.0 / frame.height() as f64; + let scale = wr.min(hr); + let w = (frame.width() as f64 * scale).round() as u32; + let h = (frame.height() as f64 * scale).round() as u32; + (w.max(1), h.max(1)) + } else { + (frame.width(), frame.height()) + }; + + let converter = match &mut converter { + Some((in_fmt, ctx)) + if *in_fmt == frame.format() + && ctx.input().width == frame.width() + && ctx.input().height == frame.height() + && ctx.output().width == out_w + && ctx.output().height == out_h => + { + ctx + } + _ => { + &mut converter + .insert(( + frame.format(), + ffmpeg::software::scaling::Context::get( + frame.format(), + frame.width(), + frame.height(), + Pixel::RGBA, + out_w, + out_h, + ffmpeg::software::scaling::flag::Flags::FAST_BILINEAR, + ) + .expect("Failed to create scaling context"), + )) + .1 + } + };crates/editor/src/audio.rs (2)
161-166: Guard against out-of-bounds segment indexing (panic risk).Indexing
self.data[self.cursor.segment_index as usize]will panic ifdatais empty orsegment_indexis stale/out-of-range (e.g., after timeline edits). Useget()and early-return.Apply:
- let tracks = &self.data[self.cursor.segment_index as usize].tracks; + let Some(segment) = self.data.get(self.cursor.segment_index as usize) else { + // Advance elapsed time to keep playhead moving even if no segment is available + self.elapsed_samples += samples; + return None; + }; + let tracks = &segment.tracks;
220-235: Ring buffer capacity is computed in bytes, but HeapRb capacity is in items of T.
HeapRb::<T>::new(capacity)expects “number of T items,” not bytes. Current computation inflates memory by a factor ofsample_size. If you intend “~1s of prerendered audio,” capacity should besample_rate * channelsT-items.Apply:
- // Up to 1 second of pre-rendered audio - let capacity = (output_info.sample_rate as usize) - * output_info.channels - * output_info.sample_format.bytes(); + // Up to 1 second of pre-rendered audio (measured in T items, not bytes) + let capacity = (output_info.sample_rate as usize) * output_info.channels;apps/desktop/src/routes/recordings-overlay.tsx (1)
762-771: Bug: Upload quick-progress branch checks for “copy” instead of “upload”During upload, the post-render “quick complete” animation is gated on
s.type === "copy", so it never runs for uploads. This is copy-pasted from the “copy” path.Apply:
- if ( - s.type === "copy" && - s.state.type === "rendering" && - s.state.state.type === "rendering" - ) + if ( + s.type === "upload" && + s.state.type === "rendering" && + s.state.state.type === "rendering" + ) s.state.state.renderedFrames = s.state.state.totalFrames;crates/recording/src/sources/camera.rs (1)
101-107: Borrowing bug:frames_rx.take()will conflict with the long-livedframesborrowYou take a reference to the inner
Receiverviaget_or_insert_with, then later try totake()theOption. This will not compile due to the active borrow. Also, the indirection isn’t needed.Refactor to own the
Receiverdirectly and move it intopause_and_drain_frameson shutdown:- let mut frames_rx: Option<Receiver<RawCameraFrame>> = None; - let frames = frames_rx.get_or_insert_with(|| self.feed_connection.attach()); + let mut frames = self.feed_connection.attach();- Some(Control::Shutdown) | None => { - if let Some(rx) = frames_rx.take() { - self.pause_and_drain_frames(rx); - } + Some(Control::Shutdown) | None => { + // Move the receiver into the draining function, then break. + let rx = frames; + self.pause_and_drain_frames(rx); info!("Camera source stopped"); break; }apps/desktop/src/components/Cropper.tsx (1)
157-169: Initial constraint uses wrong coordinate space and discards constrained resultBox is created in mapped-space, but
constrainAllis called withcontainerSize()(display-space). Also, you compute constraints but then set crop from the original width/height instead of the constrained box. Use mapped size for constraints and commitbox.toBounds().Apply this diff:
- box.constrainAll(box, containerSize(), ORIGIN_CENTER, props.aspectRatio); + box.constrainAll(box, mapped, ORIGIN_CENTER, props.aspectRatio); @@ - setCrop({ - size: { x: width, y: height }, - position: { - x: (mapped.x - width) / 2, - y: (mapped.y - height) / 2, - }, - }); + setCrop(box.toBounds());apps/desktop/src-tauri/src/fake_window.rs (1)
61-66: Fix window-local coordinate mismatch — drop outer_position and compare in window-local physical pixelswindow.cursor_position() is window-local; adding outer_position converts bounds to screen space and causes hover mis-detection (especially on HiDPI / multi-DPI setups). Compute bounds in the same window-local physical space (bounds * scale_factor) and compare directly to cursor_position.
- File: apps/desktop/src-tauri/src/fake_window.rs — spawn_fake_window_listener loop (tuple destructuring + x/y min-max calculations, ~lines 61–78).
Apply this diff:
@@ - let (Ok(window_position), Ok(mouse_position), Ok(scale_factor)) = ( - window.outer_position(), - window.cursor_position(), - window.scale_factor(), - ) else { - let _ = window.set_ignore_cursor_events(true); - continue; - }; + let (Ok(mouse_position), Ok(scale_factor)) = ( + window.cursor_position(), + window.scale_factor(), + ) else { + let _ = window.set_ignore_cursor_events(true); + continue; + }; @@ - let x_min = (window_position.x as f64) + bounds.position().x() * scale_factor; - let x_max = (window_position.x as f64) - + (bounds.position().x() + bounds.size().width()) * scale_factor; - let y_min = (window_position.y as f64) + bounds.position().y() * scale_factor; - let y_max = (window_position.y as f64) - + (bounds.position().y() + bounds.size().height()) * scale_factor; + let x_min = bounds.position().x() * scale_factor; + let x_max = (bounds.position().x() + bounds.size().width()) * scale_factor; + let y_min = bounds.position().y() * scale_factor; + let y_max = (bounds.position().y() + bounds.size().height()) * scale_factor;Please test on HiDPI displays (scale factor > 1.0) and multi-monitor setups to confirm hover toggling near edges behaves correctly.
crates/recording/src/sources/audio_mixer.rs (1)
139-147: Hard-coded sample rate and uncheckedpts()can desync or panic; compute PTS from the actual frame rate and handleNone.
filtered.pts().unwrap()may beNone, and the conversion assumes 48 kHz. Usefiltered.rate()and guard thepts()option.Apply this diff:
- while abuffersink.sink().frame(&mut filtered).is_ok() { - let adjusted_pts = - filtered.pts().unwrap() as f64 / 48000.0 * AV_TIME_BASE_Q.den as f64; - filtered.set_pts(Some(adjusted_pts as i64)); + while abuffersink.sink().frame(&mut filtered).is_ok() { + let sr = filtered.rate().max(1) as f64; + let adjusted_pts = match filtered.pts() { + Some(pts) => (pts as f64 / sr * AV_TIME_BASE_Q.den as f64) as i64, + None => { + // Fallback: advance by frame sample count + let samples = filtered.samples() as f64; + (samples / sr * AV_TIME_BASE_Q.den as f64) as i64 + } + }; + filtered.set_pts(Some(adjusted_pts));crates/scap-ffmpeg/examples/cli.rs (1)
41-45: Avoidfutures::executor::block_oninside a Tokio runtime; useawaitinstead.Calling
block_onunder#[tokio::main]can cause subtle deadlocks and blocks the runtime. Use nativeawait.Apply this diff:
- { - use futures::executor::block_on; + { use scap_screencapturekit::*; @@ - block_on(capturer.start()).expect("Failed to start capturing"); + capturer.start().await.expect("Failed to start capturing"); @@ - block_on(capturer.stop()).expect("Failed to stop capturing"); + capturer.stop().await.expect("Failed to stop capturing");And remove the now-unused import:
- use futures::executor::block_on;Also applies to: 73-79
crates/editor/src/editor_instance.rs (1)
33-39: New callback-based frame delivery is a good simplification; add an explicit'staticbound to the callback.
Renderer::spawnruns the callback inside atokio::spawn, which effectively requires'static. Make the bound explicit here to surface lifetime issues at call sites.- pub async fn new( + pub async fn new( project_path: PathBuf, on_state_change: impl Fn(&EditorState) + Send + Sync + 'static, - frame_cb: Box<dyn FnMut(RenderedFrame) + Send>, + frame_cb: Box<dyn FnMut(RenderedFrame) + Send + 'static>, ) -> Result<Arc<Self>, String> {Ensure the corresponding
Renderer::spawnsignature matches.apps/desktop/src-tauri/src/recording.rs (1)
435-466: Handle actor channel drop (Err) by cleaning up and notifying.
actor_done_rx.awaitcan returnErr(_)if the sender dropped without sending a result (panic/abort). Current code just logs and leaves state/windows possibly dangling. Treat it as a failure: emitFailed, close windows, and clear state viahandle_recording_end.- match res { + match res { Ok(Ok(_)) => { let _ = finish_upload_tx.send(()); let _ = RecordingEvent::Stopped.emit(&app); } Ok(Err(e)) => { let mut state = state_mtx.write().await; @@ handle_recording_end(app, None, &mut state).await.ok(); } - // Actor hasn't errored, it's just finished - v => { - info!("recording actor ended: {v:?}"); - } + Err(e) => { + error!("recording actor channel dropped: {e:?}"); + let _ = RecordingEvent::Failed { error: "Recording actor ended unexpectedly".into() }.emit(&app); + let mut state = state_mtx.write().await; + // this clears the current recording for us + handle_recording_end(app, None, &mut state).await.ok(); + } }apps/cli/src/record.rs (1)
66-69: Mandatory refactor: own themic_feedoption instead of borrowingThe
RecordingBaseInputsstruct currently holdspub mic_feed: &'a Option<AudioInputFeed>and in your CLI you do
mic_feed: &None,which borrows a temporary
Nonethat immediately goes out of scope—any actor that stores this reference will end up with a dangling pointer.To fix this soundness issue, refactor
mic_feedto own itsOption<AudioInputFeed>and update all callsites accordingly:• In
crates/recording/src/lib.rs- #[derive(Clone)] - pub struct RecordingBaseInputs<'a> { - pub capture_target: ScreenCaptureTarget, - pub capture_system_audio: bool, - pub mic_feed: &'a Option<AudioInputFeed>, - } + #[derive(Clone)] + pub struct RecordingBaseInputs { + pub capture_target: ScreenCaptureTarget, + pub capture_system_audio: bool, + pub mic_feed: Option<AudioInputFeed>, + }• In your CLI (
apps/cli/src/record.rs)- mic_feed: &None, + mic_feed: None,• In the recording example (
crates/recording/examples/recording-cli.rs)- mic_feed: &None, + mic_feed: None,• In the desktop app (
apps/desktop/src-tauri/src/recording.rs)- mic_feed: &state.mic_feed, + mic_feed: state.mic_feed.clone(),This change ensures that you pass ownership of the
Option(and its innerAudioInputFeedwhen present), eliminating any lifetime or soundness problems around temporary references.apps/desktop/src/routes/(window-chrome)/(main).tsx (1)
752-756: Incorrect analytics payload: camera_name is an object, not a string.You’re passing the whole CameraInfo object as camera_name. Send display_name (or another string) instead.
-trackEvent("camera_selected", { - camera_name: cameraInfo, - enabled: !!cameraInfo, -}); +trackEvent("camera_selected", { + camera_name: cameraInfo?.display_name ?? null, + enabled: !!cameraInfo, +});apps/desktop/src-tauri/src/windows.rs (1)
254-286: Avoid unwraps on display geometry; return proper tauri::Error instead.Multiple paths call .unwrap() on positions/sizes. If a driver returns None, the app will panic. Prefer early-return with WindowNotFound or a geometry error.
- let position = display.raw_handle().physical_position().unwrap(); - let size = display.physical_size().unwrap(); + let position = display + .raw_handle() + .physical_position() + .ok_or(tauri::Error::WindowNotFound)?; + let size = display + .physical_size() + .ok_or(tauri::Error::WindowNotFound)?; @@ - let position = display.raw_handle().physical_position().unwrap(); - let bounds = display.physical_size().unwrap(); + let position = display + .raw_handle() + .physical_position() + .ok_or(tauri::Error::WindowNotFound)?; + let bounds = display + .physical_size() + .ok_or(tauri::Error::WindowNotFound)?;Repeat similarly for macOS logical sizes/positions.
Also applies to: 403-440, 441-471
apps/desktop/src-tauri/src/editor_window.rs (1)
58-69: Avoid unwrap when extracting per-window EditorInstance; return an InvokeError.from_command panics if the instance doesn’t exist yet. Return a proper error instead.
-Ok(Self(instance.get(window.label()).cloned().unwrap())) +match instance.get(window.label()).cloned() { + Some(inst) => Ok(Self(inst)), + None => Err(tauri::ipc::InvokeError::from_anyhow(anyhow::anyhow!( + "Editor instance not found for window '{}'", + window.label() + ))), +}apps/desktop/src-tauri/src/lib.rs (1)
673-685: Blocking sleep inside async function (copy_file_to_path) stalls the Tokio runtime workersstd::thread::sleep blocks an OS thread and can starve other async tasks. Replace with tokio::time::sleep.
- while attempts < 10 { - std::thread::sleep(std::time::Duration::from_secs(1)); + while attempts < 10 { + tokio::time::sleep(tokio::time::Duration::from_secs(1)).await; if is_valid_mp4(src_path) { break; } attempts += 1; }crates/displays/src/platform/win.rs (1)
645-653: Construct PhysicalPosition via constructor; fields are privateDirect struct literal construction will fail because
x/yare private. Use thenewconstructor forPhysicalPosition.Apply this diff:
- Some(PhysicalPosition { - x: point.x as f64, - y: point.y as f64, - }) + Some(PhysicalPosition::new(point.x as f64, point.y as f64))crates/recording/src/capture_pipeline.rs (1)
49-108: macOS studio mode drops the first frame; enqueue it before loopingYou
recv()one frame to send the timestamp but never encode it, effectively dropping the first video frame.Apply this diff:
- builder.spawn_task("screen_capture_encoder", move |ready| { + builder.spawn_task("screen_capture_encoder", move |ready| { let mut timestamp_tx = Some(timestamp_tx); let _ = ready.send(Ok(())); - let Ok(frame) = source.1.recv() else { + let Ok(first) = source.1.recv() else { return Ok(()); }; - if let Some(timestamp_tx) = timestamp_tx.take() { - let _ = timestamp_tx.send(frame.1); - } + if let Some(timestamp_tx) = timestamp_tx.take() { + let _ = timestamp_tx.send(first.1); + } + // Do not drop the first frame. + let _ = screen_encoder.queue_video_frame(first.0.as_ref()); let result = loop { match source.1.recv() { Ok(frame) => { let _ = screen_encoder.queue_video_frame(frame.0.as_ref()); } // Err(RecvTimeoutError::Timeout) => { // break Err("Frame receive timeout".to_string()); // } Err(_) => { break Ok(()); } } }; screen_encoder.finish(); result });
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 (5)
apps/desktop/src/routes/(window-chrome)/(main).tsx (3)
750-753: Telemetry payload uses object instead of name string.You’re sending the entire
cameraInfoobject ascamera_name. Send a string (e.g.,display_name) to keep analytics clean.trackEvent("camera_selected", { - camera_name: cameraInfo, + camera_name: cameraInfo?.display_name ?? null, enabled: !!cameraInfo, });
955-964: Fix TargetSelect empty-options fallback and equality check.
value()indexesprops.options[0]without guarding emptiness → can yieldundefined.- Menu
checkeduses referential equality; use stable id comparison to mark the selected item.function TargetSelect<T extends { id: string; name: string }>(props: { @@ const value = () => { const v = props.value; if (!v) return null; + if (props.options.length === 0) return null; + const o = props.options.find((o) => o.id === v.id); if (o) return props.value; props.onChange(props.options[0]); return props.options[0]; }; @@ Promise.all( props.options.map((o) => CheckMenuItem.new({ text: getName(o), - checked: o === props.value, + checked: props.value ? o.id === props.value.id : false, action: () => props.onChange(o), }), ), )Also applies to: 969-974, 988-994
267-268: Direction attribute always “rtl”.This ternary sets
dir="rtl"for both branches. Likely intended: Windows “rtl”, others “ltr”.- dir={ostype() === "windows" ? "rtl" : "rtl"} + dir={ostype() === "windows" ? "rtl" : "ltr"}apps/desktop/src-tauri/src/recording.rs (2)
434-466: Treat actor JoinError as failure to ensure cleanup and user feedback.Currently only Ok(Err(e)) emits a Failure. A JoinError (panic/cancel) logs and exits silently, potentially leaving state dangling and no failure dialog.
- let res = actor_done_rx.await; - info!("recording wait actor done: {:?}", &res); - match res { + let res = actor_done_rx.await; + info!("recording wait actor done: {:?}", &res); + match res { Ok(Ok(_)) => { let _ = finish_upload_tx.send(()); let _ = RecordingEvent::Stopped.emit(&app); } Ok(Err(e)) => { @@ - // Actor hasn't errored, it's just finished - v => { - info!("recording actor ended: {v:?}"); - } + // Join error (panic/cancel) – treat as failure + Err(e) => { + let mut state = state_mtx.write().await; + let msg = format!("Recording task failed: {e}"); + let _ = RecordingEvent::Failed { error: msg.clone() }.emit(&app); + let mut dialog = MessageDialogBuilder::new( + app.dialog().clone(), + "An error occurred".to_string(), + msg, + ) + .kind(tauri_plugin_dialog::MessageDialogKind::Error); + if let Some(window) = CapWindowId::InProgressRecording.get(&app) { + dialog = dialog.parent(&window); + } + dialog.blocking_show(); + handle_recording_end(app, None, &mut state).await.ok(); + } }
503-507: Compile error: unnecessary?afterreturn Err(...).
return Err(...)?;won’t compile. Remove the?.let Some(current_recording) = state.clear_current_recording() else { - return Err("Recording not in progress".to_string())?; + return Err("Recording not in progress".to_string()); };
♻️ Duplicate comments (1)
apps/desktop/src/routes/(window-chrome)/(main).tsx (1)
340-359: Clickable should be a button (keyboard/a11y).This span is interactive and not focusable. Convert to a button (or add role+key handling), matching earlier guidance.
-<span - onClick={async () => { - if (license.data?.type !== "pro") { - await commands.showWindow("Upgrade"); - } - }} - class={cx( - "text-[0.6rem] rounded-lg px-1 py-0.5", - license.data?.type === "pro" - ? "bg-[--blue-400] text-gray-1 dark:text-gray-12" - : "bg-gray-3 cursor-pointer hover:bg-gray-5", - )} -> +<button + type="button" + aria-label="Account tier" + onClick={async () => { + if (license.data?.type !== "pro") { + await commands.showWindow("Upgrade"); + } + }} + class={cx( + "text-[0.6rem] rounded-lg px-1 py-0.5", + license.data?.type === "pro" + ? "bg-[--blue-400] text-gray-1 dark:text-gray-12" + : "bg-gray-3 hover:bg-gray-5", + )} +> {license.data?.type === "commercial" ? "Commercial" : license.data?.type === "pro" ? "Pro" : "Personal"} -</span> +</button>
🧹 Nitpick comments (3)
biome.json (1)
37-39: Group-level a11y disable is valid; consider downgrading to "warn" or targeting specific rulesDisabling the whole a11y group works, but it may hide legitimate issues. If you mainly want to quiet noisy findings during the refactor, consider setting the group to "warn" or selectively disabling specific rules under a11y. (biomejs.dev)
Example (set to warn):
- "rules": { "a11y": "off" } + "rules": { "a11y": "warn" }Or selectively disable only the problematic rules (adjust to your needs):
"linter": { "rules": { "a11y": { "noAutofocus": "off" } } }apps/desktop/src/routes/(window-chrome)/(main).tsx (1)
179-181: Remove leftover debug logging (spams console in production).The
_windows()dump runs on every reactive change and will spam logs. Guard it or remove.createEffect(() => { - console.log(_windows()); + if (import.meta.env.DEV) console.log(_windows()); });apps/desktop/src-tauri/src/recording.rs (1)
230-236: Consistency nit: prefer “Display” label over “Screen”.
ScreenCaptureTarget::Displayfalls back to the string “Screen”. For UI/telemetry consistency with the new model, consider “Display”.- ScreenCaptureTarget::Display { .. } => title.unwrap_or_else(|| "Screen".to_string()), + ScreenCaptureTarget::Display { .. } => title.unwrap_or_else(|| "Display".to_string()),
📜 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 (7)
apps/desktop/src-tauri/src/lib.rs(8 hunks)apps/desktop/src-tauri/src/recording.rs(9 hunks)apps/desktop/src/routes/(window-chrome)/(main).tsx(15 hunks)apps/desktop/src/routes/editor/ExportDialog.tsx(2 hunks)apps/desktop/src/utils/tauri.ts(7 hunks)biome.json(1 hunks)packages/ui-solid/src/auto-imports.d.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/ui-solid/src/auto-imports.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/routes/editor/ExportDialog.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
apps/desktop/src/routes/(window-chrome)/(main).tsx (1)
apps/desktop/src/utils/tauri.ts (2)
CaptureDisplay(465-469)CaptureWindow(470-476)
apps/desktop/src-tauri/src/lib.rs (4)
apps/desktop/src-tauri/src/target_select_overlay.rs (1)
cap_displays(51-54)apps/desktop/src-tauri/src/recording.rs (2)
id(140-145)list_capture_displays(164-169)crates/displays/src/lib.rs (4)
id(28-30)id(118-120)from_id(32-34)from_id(122-124)crates/editor/src/editor_instance.rs (1)
new(34-96)
apps/desktop/src-tauri/src/recording.rs (3)
crates/recording/src/studio_recording.rs (1)
resume(114-116)crates/recording/src/sources/screen_capture/mod.rs (3)
list_displays(386-400)list_windows(402-440)title(178-184)crates/recording/src/instant_recording.rs (1)
resume(69-71)
⏰ 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 (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (8)
apps/desktop/src-tauri/src/recording.rs (1)
164-169: Display listing API alignment looks good.
list_capture_displayscorrectly adapts the newscreen_capture::list_displays()tuple return to the public vector.apps/desktop/src-tauri/src/lib.rs (4)
468-499: Current recording mapping to new target types is correct.
- Window: resolves bounds via
cap_displays::Window::from_id(...).display_relative_logical_bounds().- Display and Area shapes align with new IDs and LogicalBounds.
1921-1923: Command surface includes displays list (TS bindings will match).Adding
recording::list_capture_displaysto the exported commands aligns the UI with the new display-centric flow.
2017-2023: Conditionally exporting TS bindings only in debug is sensible.Prevents shipping regenerated client stubs in release builds while keeping dev DX.
2335-2345: Editor instance wiring passes frame callback through correctly.
EditorInstance::new(..., frame_cb)and event bridge setup match the new rendering API.apps/desktop/src/utils/tauri.ts (3)
36-38: New listCaptureDisplays command is wired correctly.TS command name and Rust handler
list_capture_displaysmatch.
48-51: Bounds type migration acknowledged.
setFakeWindowBoundsnow takesLogicalBounds, matching the Rust side. Verify all call sites were updated (e.g., overlays).Would you like me to scan the repo to ensure no callers still pass the old Bounds shape?
684-686: Type surface matches display-centric model.
- ScreenCaptureTarget: adds
displayvariant withDisplayId.- ShowCapWindow:
WindowCaptureOccluderandCaptureAreanow useDisplayId.Also applies to: 717-720
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
crates/recording/examples/recording-cli.rs (1)
30-32: Resolved: Display selection is now safe and portable.Switching to Display::primary().id() removes the out-of-bounds risk from Display::list()[1] and works on single-monitor setups. Nice.
crates/recording/src/sources/screen_capture/mod.rs (1)
76-159: Guard against Option::unwrap paths in cursor_crop()The
cursor_crop()method calls several methods that returnOptionwithout proper error handling. These could panic if windows/displays disappear during selection.Let me verify the actual return types of these methods to ensure the concern is valid:
#!/bin/bash # Check the return types of the geometry methods to see if they return Option ast-grep --pattern 'fn logical_position($$$) $$$' ast-grep --pattern 'fn logical_bounds($$$) $$$' ast-grep --pattern 'fn physical_bounds($$$) $$$' ast-grep --pattern 'fn physical_position($$$) $$$'crates/recording/src/sources/screen_capture/windows.rs (3)
6-10: Missing import for SystemTime
SystemTimeis used inFrameHandlerandWindowsAudioCapturebut not imported explicitly.
30-40: Add error handling for missing audio devicesThe
audio_info()method callsunwrap()ondefault_output_device()anddefault_output_config(), which will panic if no audio device is available (e.g., in VMs or RDP sessions).
161-171: High drop rate only stops FrameHandler, not the captureWhen the drop rate exceeds the threshold, the code only stops the
FrameHandleractor but doesn't signal theWindowsScreenCaptureto stop, leaving capture running.Add proper cleanup to stop both the capture and the handler:
if drop_rate > MAX_DROP_RATE_THRESHOLD && total_count >= 10 { error!( "High frame drop rate detected: {:.1}% ({}/{} frames in last {}s). Exiting capture.", drop_rate * 100.0, dropped_count, total_count, WINDOW_DURATION.as_secs() ); + // Stop the capture first + if let Some(capturer) = self.capturer.upgrade() { + let _ = capturer.reply_recipient::<StopCapturing>() + .ask(StopCapturing) + .send() + .await; + } + // Then stop the frame handler let _ = ctx.actor_ref().stop_gracefully().await; return; - // return ControlFlow::Break(Err("Recording can't keep up with screen capture. Try reducing your display's resolution or refresh rate.".to_string())); }
🧹 Nitpick comments (18)
crates/camera/Cargo.toml (1)
5-5: Consider inheriting license from the workspace to avoid drift.If the workspace root defines a license in
[workspace.package], preferlicense.workspace = trueso all member crates stay consistent. Otherwise, confirm that “MIT” matches the repository’s top-level licensing strategy (e.g., single MIT vs. dual “MIT OR Apache-2.0”) and the LICENSE files present.Proposed change:
-license = "MIT" +license.workspace = truecrates/scap-ffmpeg/src/lib.rs (1)
11-13: Prefer curated re-exports overpub use ...::*to keep API surface intentional.Glob re-exporting the entire module can leak internal helpers/types and make future refactors harder (name clashes, semver churn). Consider re-exporting only the intended public API items from the CPAL bridge.
Example (adjust item names to match the module):
#[cfg(feature = "audio")] pub use cpal_audio::{AudioEncoder, AudioDevice, AudioSampleFormat};crates/scap-screencapturekit/Cargo.toml (2)
5-5: Centralize workspace licensing and declare MSRVOur workspace currently has only one explicit
licensefield—incrates/scap-screencapturekit/Cargo.toml—while all other crates lack any license declaration. In addition, the rootCargo.tomldoes not define a[workspace.package]section to enforce common metadata.Key findings:
- Only
crates/scap-screencapturekit/Cargo.tomlcontains
license = "MIT". All other crates are missing alicenseentry.- No
[workspace.package]block exists in the rootCargo.toml.- There’s no centralized
editionorrust-version(MSRV) declared.Recommended changes:
Add a
[workspace.package]section to the rootCargo.tomlto centralize common fields:[workspace.package] license = "MIT OR Apache-2.0" # or "MIT" if single-licensed edition = "2024" rust-version = "1.70" # declare your MSRV explicitlyRemove (or update) per-crate
licenselines now that they are centrally managed.If dual-licensing is desired, update the crate-level diff example accordingly:
-license = "MIT" +license = "MIT OR Apache-2.0"Ensure every crate inherits the workspace settings and omit redundant metadata from individual
Cargo.tomlfiles.These refactors will prevent license drift, simplify crate maintenance, and make your minimum supported Rust version explicit.
23-23: Proposed cleanup for cap-displays dev-dependencyI verified that:
- The root
Cargo.tomlhas a[workspace.dependencies]section, but currently does not listcap-displaysthere.- This crate already gates its entire API on macOS (
#![cfg(target_os = "macos")]insrc/lib.rs), and the only usage ofcap-displaystoday is in theexamples/cli.rs.With that in mind, you can pick one (or both) of these optional refactors:
• Centralize under workspace
- In the workspace root
Cargo.toml, add:[workspace.dependencies] cap-displays = { path = "crates/displays" }- In
crates/scap-screencapturekit/Cargo.toml, replace the inline path with:[dev-dependencies] - cap-displays = { path = "../displays" } + cap-displays = { workspace = true }• Gate to macOS only
If you’d rather keep the path declaration but avoid resolving it on non-macOS hosts:- [dev-dependencies] - cap-displays = { path = "../displays" } + [target.'cfg(target_os = "macos")'.dev-dependencies] + cap-displays = { path = "../displays" }Neither change is strictly required for correctness—everything builds today on all platforms—but these tweaks can help prevent accidental CI hiccups on Linux/Windows and keep dependency declarations consistent across the workspace.
crates/recording/src/pipeline/control.rs (4)
17-19: Guard against external draining of the control channelReturning a reference to the underlying Receiver allows external code to consume messages without updating the internal cache (last_value), which can desync observability. If external crates don’t need direct access, consider narrowing visibility or at least documenting the caveat.
Option A — document consumption semantics (preferred if you intend to expose it publicly):
impl PipelineControlSignal { - pub fn receiver(&self) -> &Receiver<Control> { + /// Returns a reference to the underlying control channel. + /// + /// Caution: Receiving from this handle bypasses PipelineControlSignal's cache (`last_value`). + /// Prefer `last()`/`blocking_last()` unless you intentionally manage consumption externally. + pub fn receiver(&self) -> &Receiver<Control> { &self.receiver }Option B — restrict visibility to within the crate:
- pub fn receiver(&self) -> &Receiver<Control> { + pub(crate) fn receiver(&self) -> &Receiver<Control> { &self.receiver }
21-23: Nit: clarify cached value semanticsConsider a brief doc comment to set expectations (e.g., when it returns None and that it doesn’t attempt to poll the channel).
- pub fn last_cached(&self) -> Option<Control> { + /// Returns the last cached control value without polling the channel. + /// This does not block and may be `None` before any value is received or after disconnect. + pub fn last_cached(&self) -> Option<Control> { self.last_value }
76-79: Broadcast head-of-line blocking; consider “drop-latest or await-on-shutdown” strategyBecause channels are bounded(1),
send_asyncmay serialize and await the slowest listener, delaying everyone. For control signals, it’s often fine to drop transient updates (e.g., repeatedPlay) but guarantee delivery for terminal signals (e.g.,Shutdown).- pub async fn broadcast(&mut self, value: Control) { - for (_, listener) in self.listeners.iter() { - let _ = listener.send_async(value).await; - } - } + pub async fn broadcast(&mut self, value: Control) { + for (_, listener) in self.listeners.iter() { + // Guarantee shutdown delivery; otherwise prefer non-blocking send. + if matches!(value, Control::Shutdown) { + let _ = listener.send_async(value).await; + } else { + let _ = listener.try_send(value); + } + } + }Alternative: send to all listeners concurrently (avoids one slow listener delaying others), if ordering isn’t required:
- Collect futures from
send_asyncandjoin_allthem.
67-74: Handle duplicate listener names (overwrite silently today)
IndexMap::insertreturns the replaced sender if a listener with the same name exists. Overwriting silently can hide wiring bugs. Either assert uniqueness, log, or return a Result.pub fn add_listener(&mut self, name: String) -> PipelineControlSignal { let (sender, receiver) = flume::bounded(1); - self.listeners.insert(name, sender); + if let Some(_old) = self.listeners.insert(name.clone(), sender) { + debug!("Replacing existing control listener: {name}"); + } PipelineControlSignal { last_value: None, receiver, } }crates/recording/examples/recording-cli.rs (8)
12-15: Prefer Per-Monitor v2 DPI awareness and avoid unwrap-induced panic (Windows).Use SetProcessDpiAwarenessContext with a fallback to SetProcessDpiAwareness, and don’t panic if the call fails (e.g., if it’s called twice or not supported). This is safer and opts into PMv2 where available.
- use windows::Win32::UI::HiDpi::{PROCESS_PER_MONITOR_DPI_AWARE, SetProcessDpiAwareness}; + use windows::Win32::UI::HiDpi::{ + PROCESS_PER_MONITOR_DPI_AWARE, + SetProcessDpiAwareness, + SetProcessDpiAwarenessContext, + DPI_AWARENESS_CONTEXT_PER_MONITOR_AWARE_V2, + }; - unsafe { SetProcessDpiAwareness(PROCESS_PER_MONITOR_DPI_AWARE).unwrap() }; + unsafe { + // Prefer Per-Monitor v2; fallback to Per-Monitor v1. + if !SetProcessDpiAwarenessContext(DPI_AWARENESS_CONTEXT_PER_MONITOR_AWARE_V2).as_bool() { + let _ = SetProcessDpiAwareness(PROCESS_PER_MONITOR_DPI_AWARE); + } + }
19-21: Remove stray /tmp directory mutations (unused, cross-platform noise).These lines don’t affect the chosen output dir (you use tempfile below) and create unnecessary side effects. Safer to drop them.
- let _ = std::fs::remove_dir_all("/tmp/bruh"); - let _ = std::fs::create_dir("/tmp/bruh");
24-24: Use tracing instead of println! to integrate with the initialized subscriber.Keeps logging consistent with tracing_subscriber.
- println!("Recording to directory '{}'", dir.path().display()); + tracing::info!("Recording to directory '{}'", dir.path().display());
46-47: Prefer expect(...) over unwrap() for clearer failure context.If spawning fails (e.g., system audio not available), a contextual message helps.
- .await - .unwrap(); + .await + .expect("failed to spawn recording actor");
51-51: Don’t discard stop() errors; surface or log them.If stopping fails, it’s worth knowing. Either expect(...) or log the error.
- let _ = handle.stop().await; + handle.stop().await.expect("failed to stop recording actor");
53-53: Avoid mem::forget; use into_path() to persist the temp dir without leaking.into_path() preserves the directory on disk while avoiding a memory leak of the TempDir guard.
- std::mem::forget(dir); + let _keep_dir = dir.into_path(); // persist directory on disk without leaking TempDir
44-44: Boolean blindness at the call site—hard to tell what thistruetoggles.If changing the API isn’t feasible here, at least annotate with a comment; long-term, prefer an options struct or enum flags.
- true, + /* flag: */ true,
26-27: Await the readiness signal before timing the capture windowCurrently,
spawn_studio_recording_actorreturns a(StudioRecordingHandle, oneshot::Receiver<Result<(), String>>)and_ready_rxis ignored, so your 10 s sleep may include actor startup time (and any initialization errors won’t surface early). To ensure the capture interval starts only once the actor is ready—and to catch startup failures—you can:• File:
crates/recording/examples/recording-cli.rs(around lines 26–27)
• Replace the ignored receiver and simple sleep with something like:- let (handle, _ready_rx) = cap_recording::spawn_studio_recording_actor( - "test".to_string(), - dir.path().into(), - RecordingBaseInputs { … }, - ) - .await - .unwrap(); - tokio::time::sleep(Duration::from_secs(10)).await; + let (handle, ready_rx) = cap_recording::spawn_studio_recording_actor( + "test".to_string(), + dir.path().into(), + RecordingBaseInputs { … }, + ) + .await + .unwrap(); + + // Wait until the actor signals it’s fully initialized + let startup_result = ready_rx.await.unwrap(); + startup_result.map_err(|e| { + eprintln!("Recording actor failed to start: {}", e); + e + })?; + + // Now record for a full 10 s after initialization + tokio::time::sleep(Duration::from_secs(10)).await;This ensures your recording covers the intended duration and surfaces any startup errors immediately.
crates/scap-direct3d/src/lib.rs (1)
407-411: Consider handling RoInitialize S_FALSE return more explicitlyThe condition
&& e.code() != S_FALSEcorrectly handles the case where COM is already initialized, but the logic could be clearer with an explicit comment explaining that S_FALSE means COM was already initialized in a compatible state.if let Err(e) = unsafe { RoInitialize(RO_INIT_MULTITHREADED) } && e.code() != S_FALSE { + // S_FALSE indicates COM was already initialized in a compatible state return Err(CreateRunnerError::FailedToInitializeWinRT); }crates/recording/src/sources/screen_capture/windows.rs (1)
344-357: Consider handling ffmpeg conversion errorsThe
as_ffmpeg()call usesunwrap()which could panic if the conversion fails.Handle the potential error gracefully:
move |frame| { let display_time = SystemTime::now(); - let ff_frame = frame.as_ffmpeg().unwrap(); + let ff_frame = match frame.as_ffmpeg() { + Ok(frame) => frame, + Err(e) => { + error!("Failed to convert frame to ffmpeg format: {}", e); + return Ok(()); + } + }; let _ = msg .frame_handler
📜 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 (21)
LICENSE(1 hunks)crates/camera-avfoundation/Cargo.toml(1 hunks)crates/camera-directshow/Cargo.toml(1 hunks)crates/camera-directshow/examples/cli.rs(2 hunks)crates/camera-ffmpeg/Cargo.toml(1 hunks)crates/camera-mediafoundation/Cargo.toml(1 hunks)crates/camera-windows/Cargo.toml(1 hunks)crates/camera/Cargo.toml(1 hunks)crates/cursor-capture/src/main.rs(1 hunks)crates/recording/examples/recording-cli.rs(2 hunks)crates/recording/src/pipeline/control.rs(1 hunks)crates/recording/src/sources/screen_capture/macos.rs(1 hunks)crates/recording/src/sources/screen_capture/mod.rs(1 hunks)crates/recording/src/sources/screen_capture/windows.rs(1 hunks)crates/scap-cpal/Cargo.toml(1 hunks)crates/scap-direct3d/Cargo.toml(2 hunks)crates/scap-direct3d/src/lib.rs(5 hunks)crates/scap-ffmpeg/Cargo.toml(2 hunks)crates/scap-ffmpeg/src/cpal.rs(1 hunks)crates/scap-ffmpeg/src/lib.rs(1 hunks)crates/scap-screencapturekit/Cargo.toml(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- crates/camera-mediafoundation/Cargo.toml
- crates/camera-avfoundation/Cargo.toml
- crates/camera-ffmpeg/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/camera-directshow/Cargo.toml
- crates/cursor-capture/src/main.rs
- crates/scap-direct3d/Cargo.toml
- crates/camera-windows/Cargo.toml
- crates/scap-cpal/Cargo.toml
- crates/camera-directshow/examples/cli.rs
- crates/recording/src/sources/screen_capture/macos.rs
- crates/scap-ffmpeg/src/cpal.rs
- LICENSE
🧰 Additional context used
🧬 Code graph analysis (4)
crates/recording/examples/recording-cli.rs (5)
apps/desktop/src-tauri/src/target_select_overlay.rs (1)
cap_displays(51-54)crates/recording/src/sources/screen_capture/mod.rs (1)
list_windows(420-458)crates/displays/src/platform/win.rs (2)
windows(640-640)primary(75-96)crates/displays/src/platform/macos.rs (2)
windows(195-195)primary(23-25)crates/displays/src/lib.rs (1)
primary(20-22)
crates/scap-direct3d/src/lib.rs (2)
crates/displays/src/platform/win.rs (1)
windows(640-640)crates/recording/src/sources/screen_capture/windows.rs (3)
new(303-307)new(401-431)run(198-294)
crates/recording/src/sources/screen_capture/windows.rs (5)
crates/scap-direct3d/src/lib.rs (4)
pixel_format(262-264)pixel_format(363-365)new(142-144)data(355-357)crates/recording/src/sources/screen_capture/mod.rs (5)
info(395-397)pixel_format(218-218)audio_info(220-220)audio_info(399-401)display(68-74)crates/media-info/src/lib.rs (1)
from_stream_config(46-64)crates/displays/src/lib.rs (5)
id(28-30)id(118-120)display(146-148)from_id(32-34)from_id(122-124)crates/scap-cpal/src/lib.rs (1)
create_capturer(17-48)
crates/recording/src/sources/screen_capture/mod.rs (7)
crates/displays/src/platform/win.rs (16)
windows(640-640)display(1298-1305)from_id(131-134)id(132-132)id(804-806)physical_size(190-192)physical_size(1290-1292)format(415-417)format(483-485)logical_size(136-151)logical_size(1216-1252)list(98-121)list(663-693)name(221-258)name(1307-1327)name(1319-1324)crates/displays/src/platform/macos.rs (15)
windows(195-195)display(461-481)Self(272-281)from_id(43-46)id(44-44)id(288-290)scale(106-108)physical_size(89-104)physical_size(362-372)logical_size(48-55)logical_size(358-360)list(27-33)list(223-255)name(123-142)name(320-332)crates/displays/src/lib.rs (13)
display(146-148)from_id(32-34)from_id(122-124)id(28-30)id(118-120)physical_size(44-46)physical_size(126-128)logical_size(48-50)logical_size(130-132)list(16-18)list(103-105)name(40-42)name(150-152)crates/cursor-capture/src/position.rs (4)
display(60-62)display(176-178)new_macos(124-131)new_windows(134-141)crates/recording/src/sources/screen_capture/macos.rs (3)
new(272-301)pixel_format(11-13)audio_info(15-22)crates/recording/src/sources/screen_capture/windows.rs (4)
new(303-307)new(401-431)pixel_format(26-28)audio_info(30-40)crates/displays/src/bounds.rs (8)
new(11-13)new(38-40)new(65-67)new(85-87)new(112-114)new(132-134)size(19-21)size(46-48)
⏰ 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 (14)
crates/camera/Cargo.toml (1)
8-8: Good: normalized workspace dependency syntax for thiserror.Switching to
{ workspace = true }is idiomatic and keeps the crate aligned with the workspace’s pinned version. No further action needed.crates/scap-ffmpeg/src/lib.rs (1)
11-13: No shadowing issues detected in the newcpalmoduleI ran the suggested repo-local scan and confirmed:
- The only
cpalmodule file iscrates/scap-ffmpeg/src/cpal.rs.- All
use cpal::…statements incpal.rsrefer to the external crate (Rust resolves barecpal::to the extern crate, not the module).- Fully-qualified paths (
::cpal::Data) are used where needed to disambiguate from the local module.- There are no other
pub use cpal::*outside oflib.rs, so no accidental exports of external items under the wrong namespace.With no shadowing or re-export leaks detected at the code level, you can consider this check resolved. Please ensure your CI pipelines cover cross-platform (Windows, macOS, Linux) builds and exercise both with and without the
audiofeature.crates/recording/src/pipeline/control.rs (2)
17-23: Encapsulation accessors: resolved and consistent with earlier feedbackSwitching from public fields to accessors looks good and aligns with the past review request. Thanks for tightening encapsulation.
33-56: The above script will surface the exact branches taken whenlast()returnsShutdown(or other values) in each source loop. Once we see how each site handles non-Playcases, we can confirm whether addingShutdownto the non-blocking arm is safe or if documentation is a better path.crates/scap-direct3d/src/lib.rs (4)
96-108: Fix swapped property names in capability check methodsThe property names in the capability check methods appear to be swapped.
can_is_border_required()is checking for "IsBorderRequired" andcan_is_cursor_capture_enabled()is checking for "IsCursorCaptureEnabled", which is correct. However, upon review of past comments, it appears there was confusion about the swapping.
151-167: Consider validating feature support before using the featuresThe error handling correctly returns specific error types when features are not supported. However, the checks could be more defensive by also validating the return values from the capability check methods.
274-330: LGTM! Frame buffer creation is well-structuredThe
as_buffer()method properly creates a staging texture and maps it for CPU access. The buffer management with proper stride and pixel format handling is correctly implemented.
474-499: Crop texture creation looks correctThe optional crop texture creation properly handles the cropping bounds and creates a texture with the correct dimensions and format.
crates/scap-ffmpeg/Cargo.toml (1)
1-28: LGTM! Dependencies are properly configuredThe new dependencies align well with the architecture changes:
scap-cpalfor audio capture integrationcap-displaysas a dev dependency for testing- Platform-specific dependencies are correctly gated
crates/recording/src/sources/screen_capture/mod.rs (4)
278-279: Replace unwrap with proper error handling for Window::from_id
Window::from_id(&id)returns anOptionbut usesok_orwhich properly handles the None case. This is correct.
281-298: Properly handle Option returns from geometry methodsThe geometry methods (logical_bounds, physical_bounds, etc.) return Options and are properly handled with
ok_or(ScreenCaptureInitError::NoBounds), preventing panics.Also applies to: 300-318, 324-352
356-372: Consider handling the final unwrap on output_size more explicitlyThe output size calculation properly handles Option chaining with
ok_or(ScreenCaptureInitError::NoBounds)at the end, which prevents panics.
420-458: LGTM! Window filtering logic is appropriateThe
list_windows()function properly filters out invalid windows on both platforms:
- macOS: Filters out non-level-0 windows and Window Server owned windows
- Windows: Filters out invalid and off-screen windows
crates/recording/src/sources/screen_capture/windows.rs (1)
117-186: Frame drop tracking implementation looks solidThe frame drop rate tracking with windowed statistics and periodic cleanup is well-implemented. The helper methods properly manage the deque and calculate statistics accurately.
| mod cpal; | ||
| pub use cpal::*; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid local module name collision with the external cpal crate; gate the export behind a feature.
Declaring mod cpal; at the crate root shadows the external cpal crate in Rust 2018, which easily leads to broken imports inside the new module (e.g., use cpal::... resolves to the local module, not the external crate). This is a subtle but common footgun and can become a compile error depending on how cpal.rs is written. Also, the unconditional pub use cpal::*; forces this path into all builds even when audio is not needed.
- Rename the internal module to avoid shadowing, e.g.,
cpal_audio. - Add a feature gate (e.g.,
audio) to keep parity with the existing OS-gated exports and avoid pulling audio dependencies when not required.
Apply this diff here:
-mod cpal;
-pub use cpal::*;
+#[cfg(feature = "audio")]
+mod cpal_audio;
+#[cfg(feature = "audio")]
+pub use cpal_audio::*;Follow-ups:
- Rename the file to
src/cpal_audio.rs(orsrc/cpal_audio/mod.rs). - If you must keep the module named
cpal, ensure all references to the external crate inside it are fully qualified as::cpal::...to bypass the shadowing.
📝 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.
| mod cpal; | |
| pub use cpal::*; | |
| // crates/scap-ffmpeg/src/lib.rs | |
| // … other imports … | |
| #[cfg(feature = "audio")] | |
| mod cpal_audio; | |
| #[cfg(feature = "audio")] | |
| pub use cpal_audio::*; |
🤖 Prompt for AI Agents
In crates/scap-ffmpeg/src/lib.rs around lines 11-13, the declaration `mod cpal;`
and `pub use cpal::*;` shadows the external cpal crate and unconditionally
exports audio symbols; rename the internal module to avoid collision (e.g., `mod
cpal_audio;`), gate the public re-export behind a feature (e.g., `#[cfg(feature
= "audio")] pub use cpal_audio::*;`), and update the module file name to
src/cpal_audio.rs (or src/cpal_audio/mod.rs); alternatively, if you must keep
the name `cpal`, ensure all references to the external crate inside the module
are fully qualified as `::cpal::...`.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src-tauri/src/recording.rs (1)
462-494: Treat dropped actor_done_rx (RecvError) as an error and clean up UIWhen actor_done_rx.await returns Err(_), the code logs and exits without cleanup. That typically indicates the actor crashed/panicked or the sender dropped without sending, leaving overlays and state potentially stale.
Handle Err(_) like an error: emit Failed, close overlays, clear current recording, and ensure finish_upload_tx is dropped or signaled.
- match res { + match res { Ok(Ok(_)) => { let _ = finish_upload_tx.send(()); let _ = RecordingEvent::Stopped.emit(&app); } Ok(Err(e)) => { let mut state = state_mtx.write().await; let _ = RecordingEvent::Failed { error: e.clone() }.emit(&app); @@ handle_recording_end(app, None, &mut state).await.ok(); } - // Actor hasn't errored, it's just finished - v => { - info!("recording actor ended: {v:?}"); - } + Err(e) => { + error!("recording actor channel dropped: {e}"); + let mut state = state_mtx.write().await; + let _ = RecordingEvent::Failed { error: "Recording ended unexpectedly".into() }.emit(&app); + handle_recording_end(app, None, &mut state).await.ok(); + } }
🧹 Nitpick comments (18)
crates/recording/src/studio_recording.rs (1)
265-266: Consider reducing verbose logging in productionMultiple
info!andprintln!statements have been added for debugging purposes. Consider usingdebug!ortrace!levels for pipeline lifecycle events to avoid cluttering production logs.- tracing::info!("pipeline shuting down"); + tracing::debug!("pipeline shutting down"); pipeline.inner.shutdown().await?; - tracing::info!("pipeline shutdown"); + tracing::debug!("pipeline shutdown complete");- println!("recording successfully stopped"); + tracing::info!("recording successfully stopped");Also applies to: 269-270, 420-421
crates/recording/src/sources/screen_capture/macos.rs (7)
45-48: PTS/relative time calculation: ensure monotonicity and timebase alignment.The conversion CMTime(value/scale) → Unix seconds via (start_time_unix + pts - start_cmtime) looks right, but rounding could occasionally yield a tiny negative relative time if clocks jitter. Consider clamping to 0 and documenting the expected timebase downstream (video/audio code mixes float seconds and FFmpeg timebases).
- let relative_time = unix_timestamp - self.start_time_f64; + let relative_time = (unix_timestamp - self.start_time_f64).max(0.0);Please confirm the pipeline expects f64 seconds for relative timestamps for both video and audio. If it expects AV_TIME_BASE or stream time-bases, we should rescale here for consistency.
170-181: Deriving scale via physical/logical sizes risks Option unwrap panics; prefer NSScreen scale().Using physical/logical widths is okay, but macOS already exposes backingScaleFactor. It’s safer to prefer Display::scale() and only fall back to the ratio if needed.
- let scale = display.physical_size().unwrap().width() - / display.logical_size().unwrap().width(); + let scale = display + .scale() + .unwrap_or_else(|| { + display.physical_size().unwrap().width() + / display.logical_size().unwrap().width() + });Confirm that display.logical_size() and physical_size() are infallible for all attached displays (including headless/Sidecar/Stage Manager configurations). If not, we should avoid unwraps and bubble a SourceError.
279-296: Surface output callback backpressure/drop events in logs.We currently fire try_send and ignore failure silently. Adding a trace helps diagnose dropped frames during load.
- let _ = frame_handler.tell(NewFrame(frame)).try_send(); + if let Err(e) = frame_handler.tell(NewFrame(frame)).try_send() { + trace!("Dropped NewFrame: {}", e); + }
315-316: Remove unused type CaptureError.It’s not referenced; drop it to reduce API noise. If you plan to use it in a follow-up, add a TODO with intended usage.
-pub struct CaptureError(pub arc::R<ns::Error>); +// (removed) Unused type; consider reintroducing when error routing needs it.
217-224: Minor: Prefer start().send().await? pattern clarity and error mapping.Current ask().send().await is fine. If we standardize across sources on ask().await (if supported by kameo) or a helper wrapper, it could simplify SourceError mapping. Optional.
185-193: Even-dimension rounding responsibility.We set width/height directly from computed size. If the “round to nearest 2” invariant isn’t enforced upstream of this module, enforce it here to keep encoders happy.
- let mut settings = scap_screencapturekit::StreamCfgBuilder::default() - .with_width(size.width() as usize) - .with_height(size.height() as usize) + let w = (size.width().round() as usize) & !1; + let h = (size.height().round() as usize) & !1; + let mut settings = scap_screencapturekit::StreamCfgBuilder::default() + .with_width(w) + .with_height(h)Confirm if rounding is already applied in config or a higher layer; if so, we can skip it here to avoid double rounding.
267-303: Optional: centralize cap_fail injection guards for consistency.The inline closures around fail points work, but extracting a tiny helper (e.g., fail_or_ok(tag)) would reduce repetition and make intent clearer.
apps/desktop/src-tauri/src/recording.rs (8)
79-83: Inconsistent error types between pause() and resume() on InProgressRecordingresume() now returns Result<(), String> while pause() returns Result<(), RecordingError>. This asymmetry is harmless (both are mapped to String at the Tauri boundary), but it increases cognitive overhead and can lead to accidental pattern-matching mistakes later.
- Option A: keep both returning RecordingError and map at the Tauri boundary.
- Option B: switch pause() to return Result<(), String> for consistency with resume() and the actor oneshot.
162-169: Display listing API looks good; consider stable orderingThe new list_capture_displays() correctly projects screen_capture::list_displays() to CaptureDisplay. For a more stable UI, consider sorting by name (then id as tiebreaker) to avoid flicker between calls on platforms that don’t guarantee enumeration order.
pub async fn list_capture_displays() -> Vec<CaptureDisplay> { - screen_capture::list_displays() + let mut displays = screen_capture::list_displays() .into_iter() .map(|(v, _)| v) - .collect() + .collect::<Vec<_>>(); + displays.sort_by(|a, b| a.name.cmp(&b.name).then(a.id.cmp(&b.id))); + displays }
173-178: Windows listing API looks good; consider stable orderingSame note as displays: a stable ordering reduces UI churn.
pub async fn list_capture_windows() -> Vec<CaptureWindow> { - screen_capture::list_windows() + let mut windows = screen_capture::list_windows() .into_iter() .map(|(v, _)| v) - .collect() + .collect::<Vec<_>>(); + windows.sort_by(|a, b| a.owner_name.cmp(&b.owner_name).then(a.name.cmp(&b.name))); + windows }
229-236: “Screen” vs “Display” terminologyFor the Display variant, the fallback name currently uses "Screen". The rest of the PR and UI surfaces have moved to “Display”. Consider aligning to "Display" for consistency in pretty names and logs.
- ScreenCaptureTarget::Display { .. } => title.unwrap_or_else(|| "Screen".to_string()), + ScreenCaptureTarget::Display { .. } => title.unwrap_or_else(|| "Display".to_string()),
337-338: Prefer tracing over println! for spawn logsUse the existing tracing infrastructure to avoid stdout noise in production builds.
- println!("spawning actor"); + tracing::trace!("spawning actor");If you adopt this, also add
traceto the imports:// outside selected lines, but required: use tracing::{error, info, trace};
534-535: Redundant?on an explicitreturn Err(...)return Err(...)? is confusing; just return the Err.
- return Err("Recording not in progress".to_string())?; + return Err("Recording not in progress".to_string());
507-515: No-op when pausing/resuming without an active recordingpause_recording and resume_recording silently return Ok(()) if no recording is in progress. That’s acceptable UX, but if you want explicit feedback (consistent with stop_recording), consider returning an error when no recording exists.
- if let Some(recording) = state.current_recording_mut() { - recording.pause().await.map_err(|e| e.to_string())?; - } + if let Some(recording) = state.current_recording_mut() { + recording.pause().await.map_err(|e| e.to_string())?; + } else { + return Err("Recording not in progress".to_string()); + }(Apply similarly to resume_recording.)
Also applies to: 519-527
283-291: Double-check occluder vs. area-capture window for Area targetsIn
apps/desktop/src-tauri/src/recording.rs, you’re currently reusing theWindowCaptureOccluderfor bothWindowandAreatargets. Since there’s a dedicatedShowCapWindow::CaptureArea { screen_id }variant (which maps toCapWindowId::CaptureAreainwindows.rs), consider using that for clarity and to enable any future area-specific behavior.• Location
ScreenCaptureTarget::Window { … }(lines 283–291): OK to useif let Some(show) = inputs .capture_target .display() .map(|d| ShowCapWindow::WindowCaptureOccluder { screen_id: d.id() }) { let _ = show.show(&app).await; }ScreenCaptureTarget::Area { screen, .. }(lines 293–298): suggest switching toCaptureArea- let _ = ShowCapWindow::WindowCaptureOccluder { + let _ = ShowCapWindow::CaptureArea { screen_id: screen.clone(), } .show(&app) .await;This change is purely optional but will make the intent clearer and allow you to customize the area-capture window separately in the future.
apps/desktop/src/utils/tauri.ts (2)
559-573: GifExportSettings/GifQuality additionsPublic API expansion looks fine. If any defaults are assumed in UI, guard against nulls (quality and fast can be null).
591-593: LayoutMode/LayoutSegment addedNice forward‑compat addition. Validate editor/preview code applies a sensible default when mode is undefined.
📜 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 (10)
apps/desktop/src-tauri/src/recording.rs(9 hunks)apps/desktop/src/utils/tauri.ts(8 hunks)crates/fail/src/lib.rs(2 hunks)crates/recording/examples/recording-cli.rs(2 hunks)crates/recording/src/pipeline/builder.rs(5 hunks)crates/recording/src/pipeline/control.rs(1 hunks)crates/recording/src/sources/screen_capture/macos.rs(1 hunks)crates/recording/src/sources/screen_capture/mod.rs(1 hunks)crates/recording/src/sources/screen_capture/windows.rs(1 hunks)crates/recording/src/studio_recording.rs(18 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/recording/src/pipeline/control.rs
- crates/recording/src/sources/screen_capture/mod.rs
- crates/recording/src/sources/screen_capture/windows.rs
- crates/recording/examples/recording-cli.rs
🧰 Additional context used
🧬 Code graph analysis (3)
crates/recording/src/studio_recording.rs (6)
crates/utils/src/lib.rs (1)
spawn_actor(37-44)crates/recording/src/cursor.rs (5)
std(238-238)std(268-268)std(297-297)spawn_cursor_recorder(38-188)stop(31-34)crates/recording/src/capture_pipeline.rs (2)
tokio(152-152)create_screen_capture(373-394)crates/recording/src/sources/screen_capture/mod.rs (2)
info(395-397)display(68-74)crates/recording/src/feeds/camera.rs (1)
camera_info(136-138)crates/recording/src/pipeline/mod.rs (1)
builder(22-24)
apps/desktop/src-tauri/src/recording.rs (3)
crates/recording/src/studio_recording.rs (2)
ensure_dir(937-940)spawn_studio_recording_actor(131-231)crates/recording/src/sources/screen_capture/mod.rs (5)
info(395-397)list_displays(404-418)list_windows(420-458)title(178-184)clone(224-234)crates/recording/src/instant_recording.rs (1)
spawn_instant_recording_actor(154-248)
crates/recording/src/sources/screen_capture/macos.rs (2)
crates/recording/src/sources/screen_capture/mod.rs (5)
pixel_format(218-218)audio_info(220-220)audio_info(399-401)display(68-74)info(395-397)crates/displays/src/platform/macos.rs (6)
Self(272-281)display(461-481)from_id(43-46)logical_size(48-55)logical_size(358-360)scale(106-108)
⏰ 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 (25)
crates/fail/src/lib.rs (2)
25-25: Good improvement for qualified fail point namingThe change to use
concat!(env!("CARGO_PKG_NAME"), "::", $name)creates fully-qualified fail point names that include the crate name, which helps prevent collisions across crates and improves debugging clarity.Also applies to: 28-28, 31-31, 34-34
45-46: Consistent implementation in fail_err macroThe same qualified naming pattern is correctly applied to the
fail_errmacro, maintaining consistency across both macros.Also applies to: 48-48, 51-51, 54-54
crates/recording/src/studio_recording.rs (6)
1-1: Well-structured error type hierarchyThe introduction of
SpawnStudioRecordingErrorandCreateSegmentPipelineErrorprovides clear, granular error handling that properly distinguishes between different failure modes. The use ofthiserrorwith appropriate#[from]attributes ensures good error propagation ergonomics.Also applies to: 8-13, 123-129, 642-658
137-137: Consistent error type propagation throughout the APIThe error type changes are consistently applied across the public API surface, from
spawn_studio_recording_actorthroughSegmentPipelineFactory::create_nexttocreate_segment_pipeline. This provides better error context to callers.Also applies to: 618-619, 680-680
689-694: Improved display and bounds validationGood addition of explicit validation for display and crop bounds with descriptive error variants. This fail-fast approach prevents downstream issues from missing configuration.
713-713: Simplified pipeline construction without clockThe removal of clock-based pipeline construction aligns well with the broader refactoring to eliminate clock generics. The error handling properly maps build and play errors to specific
CreateSegmentPipelineErrorvariants.Also applies to: 891-899
902-904: Updated cursor recorder integrationThe cursor recorder now correctly receives
crop_boundsanddisplaydirectly instead of extracting them from screen objects, which aligns with the new display-centric API design.
328-331: Ensure cursor actor cleanup idempotence across all error pathsI searched for other instances of
cursor.actor.take()andactor.stop().awaitand didn’t find any beyond the cleanup blocks in
- crates/recording/src/studio_recording.rs: lines 328–331
- crates/recording/src/studio_recording.rs: lines 348–351
Please manually verify that:
- Every exit path (success, error, early return) that takes the cursor actor also calls
stop().awaitexactly once.- The implementation of
Actor::stopis idempotent and will never hang if invoked on an already-stopped or failed actor.crates/recording/src/pipeline/builder.rs (4)
22-33: Clean removal of clock genericsThe simplification from
PipelineBuilder<T>to non-genericPipelineBuildersuccessfully removes unnecessary complexity. The builder pattern remains intact while eliminating the clock field entirely.
35-53: Improved error propagation in spawn_sourceGood addition of error checking and propagation when task.run() fails. The formatted error message includes both the task name and the original error, providing helpful context for debugging.
134-134: Enhanced error context in task failure messagesThe error messages now include "Task/{name}/{error}" format consistently, providing clear traceability of which task failed and why. This improves debuggability significantly.
Also applies to: 150-151
113-116: Simplified public API surfaceThe build method signature change from returning
(Pipeline<T>, ...)to(Pipeline, ...)removes the generic complexity from the public API, making it easier for consumers to use.crates/recording/src/sources/screen_capture/macos.rs (4)
11-22: Confirm SCK audio format assumptions (48 kHz, stereo, planar F32).Hard-coding 48 kHz stereo planar F32 is fine if ScreencaptureKit always delivers that. If SCK can output other sample rates/channel counts (aggregate devices, user locale), we’ll need a resampler or early return. Please verify the upstream guarantees or guard accordingly.
Would you like me to add a defensive probe that logs and drops unexpected formats to avoid crashes further downstream?
205-256: Orchestration loop and error handling look solid.Nice wiring: start, signal readiness, then select over error/control with graceful stop, including DidStopWithError propagation. This matches our pipeline lifecycle model well.
1-3: Overall: strong structure and actor-based wiring.Good separation of concerns: format descriptor, frame handler, capture actor, and pipeline task. The macOS backend integrates cleanly with the new ScreenCaptureSource model and cap_fail injection points are well-placed for testing.
Also applies to: 25-33, 261-266, 268-303
81-106: Review audio capture: handle unwraps, buffer contiguity, non-blocking send, and correct PTS baseA few runtime panics and blocking semantics remain in the audio path. Please update as follows and verify the channel type and expected PTS timebase:
• Replace
unwrap()onaudio_buf_list::<2>()and.as_slice()withOptionchecks and early returns (withtrace!/warn!), so that non-stereo or non-contiguous buffers don’t panic.
• Guard the per-plane slice copy with a bounds check before callingcopy_from_slice.
• Avoid depending onAV_TIME_BASE_Q; compute PTS explicitly in your pipeline’s timebase (e.g. microseconds or 1/48 kHz samples). Consider usingffmpeg::Rationalandrescale_q.
• Use a non-blocking send onaudio_tx(e.g.try_sendorsend_async), so the capture callback can never stall.Suggested diff:
- let Some(audio_tx) = &self.audio_tx else { - return; - }; - - let buf_list = sample_buffer.audio_buf_list::<2>().unwrap(); - let slice = buf_list.block().as_slice().unwrap(); + let Some(audio_tx) = &self.audio_tx else { + // No consumer; drop the frame + return; + }; + let Some(buf_list) = sample_buffer.audio_buf_list::<2>() else { + trace!("Skipping audio frame: not stereo planar"); + return; + }; + let Some(slice) = buf_list.block().as_slice() else { + warn!("Skipping audio frame: non-contiguous buffer"); + return; + }; let mut frame = ffmpeg::frame::Audio::new( ffmpeg::format::Sample::F32(ffmpeg::format::sample::Type::Planar), sample_buffer.num_samples() as usize, ChannelLayout::STEREO, ); frame.set_rate(48_000); - let data_bytes_size = buf_list.list().buffers[0].data_bytes_size; - for i in 0..frame.planes() { + let data_bytes_size = buf_list.list().buffers[0].data_bytes_size as usize; + for i in 0..frame.planes() { use cap_media_info::PlanarData; - frame.plane_data_mut(i).copy_from_slice( - &slice[i * data_bytes_size..(i + 1) * data_bytes_size], - ); + let start = i * data_bytes_size; + let end = start + data_bytes_size; + if end > slice.len() { + warn!("Plane {i} out-of-bounds: {start}..{end} > {}", slice.len()); + return; + } + frame.plane_data_mut(i).copy_from_slice(&slice[start..end]); } - frame.set_pts(Some((relative_time * AV_TIME_BASE_Q.den as f64) as i64)); + // Compute PTS explicitly—microseconds shown; adjust if your pipeline expects sample-rate units + frame.set_pts(Some((relative_time * 1_000_000.0) as i64)); - let _ = audio_tx.send((frame, relative_time)); + // Use non-blocking send to avoid stalling the capture thread + if let Err(_) = audio_tx.try_send((frame, relative_time)) { + warn!("Audio pipeline full; dropping frame"); + }Please verify
- The concrete channel type for
audio_txand whether it supportstry_send(sync) or requires an asyncsend_async().await.- The expected PTS timebase (µs vs. 1/48 kHz samples). If it’s sample-based, use
ffmpeg::Rationalandrescale_qto convert from seconds.apps/desktop/src-tauri/src/recording.rs (2)
362-388: Error mapping and actor spawn flow looks solidGood conversion of actor-spawn errors to Strings at the tauri boundary and clear separation of Instant vs Studio paths. The added cap_fail injection point (“recording::spawn_actor”) is well placed for testing.
Also applies to: 394-417
430-453: Good user-facing error handling for spawn failuresOn spawn failure, you emit RecordingEvent::Failed, show a dialog (parented if possible), clear current recording, and return Err. This is consistent and user friendly.
apps/desktop/src/utils/tauri.ts (7)
465-469: CaptureDisplay type: IDs and fields align with Rustid: DisplayId (string), name, refresh_rate. Matches recording.rs list_capture_displays mapping. LGTM.
719-722: ShowCapWindow: capture overlays use DisplayId nowThe typing matches Rust. Ensure any invocations still using numeric screen indices are updated.
48-53: Validation Passed: Rust command signature matches LogicalBoundsThe
set_fake_window_boundsTauri command inapps/desktop/src-tauri/src/fake_window.rsis declared as:pub async fn set_fake_window_bounds( window: tauri::Window, name: String, bounds: LogicalBounds, )which aligns perfectly with the TypeScript invocation signature in
apps/desktop/src/utils/tauri.ts. No serialization mismatches detected.
686-689: Double-check legacy “screen” references in persisted dataI ran searches across the repository for JSON keys using
variant: "screen"- numeric
"screen": <id>No matches were found in source or storage files.
Please manually verify that any of the following have been updated to use the new
window/display/areavariants instead of the oldscreenidentifier:
- Persisted user preference files or settings exports
- Deep-link serialization/deserialization logic
- Migration or data-upgrade scripts handling legacy storage formats
36-38: ✅ Rename Verified: Old API References RemovedAll references to the deprecated
listCaptureScreensand its invoke route"list_capture_screens"have been successfully removed. Call sites now uniformly uselistCaptureDisplays()invokinglist_capture_displays().Verification steps:
- Ran
rg -n "listCaptureScreens"– no matches found- Ran
rg -n "\"list_capture_screens\""– no matches foundThe API rename is complete. Ready to merge.
490-493: Verify handling of new CurrentRecordingTarget variants and DisplayId stringsThe CurrentRecordingTarget union in
apps/desktop/src/utils/tauri.ts(lines 490–493) now includes{ screen: { id: DisplayId } }and{ area: { screen: DisplayId; bounds: LogicalBounds } }, and theidfields are strings (DisplayId) instead of numbers. There aren’t any Redux reducers or “selectors” in the classic sense—most code consumes the data viacreateCurrentRecordingQuery()—so you’ll need to audit all consumers to ensure they handle:
- The new
"screen"variant (which has noboundsby default)- The
"area"variant (screen + bounds)- The fact that every
.idis now a stringAction items:
- In
window-capture-occluder.tsx, add a branch for"screen"incurrentRecording.data.target(e.g., derive full-screen bounds from the DisplayId or call a helper).- Search for all uses of
currentRecording.data.target.*.idand confirm downstream calls (e.g.,commands.getWindow(id)) accept a string ID or convert it appropriately.- Update any comparisons, arithmetic, or parsing that previously assumed a numeric window or display ID to treat it as a string.
This audit will prevent runtime errors when the target is a screen or area variant and ensure type safety across the codebase.
471-476: Bounds usage verified as logical coordinatesWe’ve audited all
.boundsreferences and confirmed UI code consistently treatsboundsvalues as CSS px (i.e. logical units) or uses them directly for canvas drawing, matching the newLogicalBoundssemantics. No callers assume device-pixel (physical) coordinates, so no changes are required.Key locations reviewed:
- apps/desktop/src/routes/target-select-overlay.tsx (lines 121–124):
width,height,left,topare set in CSS px frombounds.sizeandbounds.position.- apps/desktop/src/components/CropAreaRenderer.tsx:
draw(ctx, props.bounds, …)uses logical coordinates for canvas rendering.- apps/desktop/src/routes/window-capture-occluder.tsx: returns
boundsfor overlay positioning in logical space.
| // done in spawn to catch panics just in case | ||
| let spawn_actor_res = async { | ||
| spawn_actor({ | ||
| let state_mtx = Arc::clone(&state_mtx); | ||
| let general_settings = general_settings.cloned(); | ||
| let capture_target = inputs.capture_target.clone(); | ||
| async move { | ||
| fail!("recording::spawn_actor"); | ||
| let mut state = state_mtx.write().await; | ||
|
|
||
| let base_inputs = cap_recording::RecordingBaseInputs { | ||
| capture_target, | ||
| capture_system_audio: inputs.capture_system_audio, | ||
| mic_feed: &state.mic_feed, | ||
| }; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Possible lifetime hazard: passing &state.mic_feed into RecordingBaseInputs
base_inputs borrows mic_feed from a write-locked state guard:
- let mut state = state_mtx.write().await;
- mic_feed: &state.mic_feed,
If RecordingBaseInputs is only used synchronously within spawn_*_recording_actor to clone the Arc and does not store the reference, this is fine. If the reference escapes into the actor, the borrow would outlive the guard, which the compiler should prevent (but could be obscured by indirection).
Please confirm that RecordingBaseInputs does not store the borrowed reference beyond the spawn call and that mic_feed is cloned into any long-lived context. Suggested checks:
If it does escape, switch base_inputs.mic_feed to an owned Option<Arc<...>> created via clone to avoid lifetime coupling to the write guard.
🏁 Script executed:
#!/bin/bash
# 1) Locate RecordingBaseInputs and its fields
rg -nP -C3 'struct\s+RecordingBaseInputs\b'
# 2) Inspect its mic_feed type and usages
rg -nP -C3 'mic_feed\s*:\s*&'
rg -nP -C3 'mic_feed\b' crates/recording
# 3) Confirm that spawn_instant_recording_actor and spawn_studio_recording_actor
# only read/clone mic_feed during setup and do not store &'_ Option<Arc<...>>.
rg -nP -C5 'spawn_(instant|studio)_recording_actor\s*\(' crates/recording -g '!**/target/**'Length of output: 7875
Ensure mic_feed is owned in RecordingBaseInputs to eliminate any borrow escaping
In its current form, RecordingBaseInputs<'a> holds a reference into the locked state.mic_feed, which in the “studio” actor path is immediately cloned into an owned Option<AudioInputFeed>…
…however in the “instant” actor path it’s only passed as inputs.mic_feed.as_ref(), meaning a borrow could persist beyond the write‐lock’s scope if create_pipeline ever retains that reference.
To avoid any lifetime coupling or potential data-race risk, switch RecordingBaseInputs to take an owned Option<AudioInputFeed> instead of a &Option<…>, and clone at the call site:
• Change the struct (crates/recording/src/lib.rs:42–46)
- pub struct RecordingBaseInputs<'a> {
- pub capture_target: ScreenCaptureTarget,
- pub capture_system_audio: bool,
- pub mic_feed: &'a Option<AudioInputFeed>,
- }
+ pub struct RecordingBaseInputs {
+ pub capture_target: ScreenCaptureTarget,
+ pub capture_system_audio: bool,
+ pub mic_feed: Option<AudioInputFeed>,
+ }• In the Tauri launcher (apps/desktop/src-tauri/src/recording.rs:359)
- let base_inputs = cap_recording::RecordingBaseInputs {
- capture_target,
- capture_system_audio: inputs.capture_system_audio,
- mic_feed: &state.mic_feed,
- };
+ let base_inputs = cap_recording::RecordingBaseInputs {
+ capture_target,
+ capture_system_audio: inputs.capture_system_audio,
+ mic_feed: state.mic_feed.clone(),
+ };• Adjust the “instant” actor (crates/recording/src/instant_recording.rs:194) to accept an owned Option<AudioInputFeed> (and remove any as_ref() usage), so that create_pipeline takes Option<AudioInputFeed> directly.
• Adjust the “studio” actor (crates/recording/src/studio_recording.rs) similarly—or continue to clone from base_inputs.mic_feed if desired, but no longer from a borrowed &Option.
This guarantees that all audio‐feed state is fully owned by each actor at spawn time and never borrows from a lock guard.
📝 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.
| // done in spawn to catch panics just in case | |
| let spawn_actor_res = async { | |
| spawn_actor({ | |
| let state_mtx = Arc::clone(&state_mtx); | |
| let general_settings = general_settings.cloned(); | |
| let capture_target = inputs.capture_target.clone(); | |
| async move { | |
| fail!("recording::spawn_actor"); | |
| let mut state = state_mtx.write().await; | |
| let base_inputs = cap_recording::RecordingBaseInputs { | |
| capture_target, | |
| capture_system_audio: inputs.capture_system_audio, | |
| mic_feed: &state.mic_feed, | |
| }; | |
| // done in spawn to catch panics just in case | |
| let spawn_actor_res = async { | |
| spawn_actor({ | |
| let state_mtx = Arc::clone(&state_mtx); | |
| let general_settings = general_settings.cloned(); | |
| let capture_target = inputs.capture_target.clone(); | |
| async move { | |
| fail!("recording::spawn_actor"); | |
| let mut state = state_mtx.write().await; | |
| let base_inputs = cap_recording::RecordingBaseInputs { | |
| capture_target, | |
| capture_system_audio: inputs.capture_system_audio, | |
| mic_feed: state.mic_feed.clone(), | |
| }; | |
| // … rest of the code … |
🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/recording.rs around lines 346–361, the code builds
a RecordingBaseInputs with mic_feed borrowed as &state.mic_feed which can let a
borrow escape the write-lock; change the call site to pass an owned
Option<AudioInputFeed> by cloning the feed (e.g. use state.mic_feed.clone()),
update RecordingBaseInputs definition to hold an owned Option<AudioInputFeed>
instead of a &Option<...>, and then update the instant and studio actor code
paths (and create_pipeline signatures) to take/consume Option<AudioInputFeed>
(remove as_ref() uses) so each actor owns its audio feed and no borrow from the
lock can escape.
| if let Some(crop_bounds) = config.crop_bounds { | ||
| tracing::info!("crop bounds: {:?}", crop_bounds); | ||
| settings.set_src_rect(cg::Rect::new( | ||
| crop_bounds.position().x(), | ||
| crop_bounds.position().y(), | ||
| crop_bounds.size().width(), | ||
| crop_bounds.size().height(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Crop rect likely needs physical-space coordinates; values appear unscaled on Retina.
We set stream width/height in physical pixels, but pass crop_bounds in logical units. On Retina, this will crop the wrong area. Scale the rect by backing scale.
- if let Some(crop_bounds) = config.crop_bounds {
- tracing::info!("crop bounds: {:?}", crop_bounds);
- settings.set_src_rect(cg::Rect::new(
- crop_bounds.position().x(),
- crop_bounds.position().y(),
- crop_bounds.size().width(),
- crop_bounds.size().height(),
- ));
- }
+ if let Some(crop_bounds) = config.crop_bounds {
+ tracing::info!("crop bounds (logical): {:?}", crop_bounds);
+ let scale = display
+ .scale()
+ .unwrap_or_else(|| {
+ display.physical_size().unwrap().width()
+ / display.logical_size().unwrap().width()
+ });
+ settings.set_src_rect(cg::Rect::new(
+ crop_bounds.position().x() * scale,
+ crop_bounds.position().y() * scale,
+ crop_bounds.size().width() * scale,
+ crop_bounds.size().height() * scale,
+ ));
+ }If upstream already rounds bounds to even physical pixels (“Round capture bounds to nearest 2”), ensure this scaling happens before rounding to preserve evenness post-scale.
🏁 Script executed:
#!/bin/bash
# Search for config.crop_bounds usage and surrounding context
rg -n 'config\.crop_bounds' -C3 crates/recording/src/sources/screen_capture/macos.rs
# Locate set_src_rect calls to see how src_rect is used elsewhere
rg -n 'set_src_rect' -C3 crates/recording/src/sources/screen_capture/macos.rs
# Check for any rounding of capture bounds
rg -n -C3 -e 'round' crates/recording/src/sources/screen_capture/macos.rs
# Inspect for existing scale, physical_size, or logical_size usage in this module
rg -n -C3 -e 'scale' -e 'physical_size' -e 'logical_size' crates/recording/src/sources/screen_capture/macos.rsLength of output: 2428
Action Required: Scale crop_bounds to Physical Pixels Before Setting src_rect
The current implementation applies crop_bounds in logical (point) units to settings.set_src_rect, while stream width/height are already converted to physical pixels. On Retina displays, this mismatch will crop the wrong region.
Key points:
- Location:
crates/recording/src/sources/screen_capture/macos.rs, lines ~195–203. - Confirmed absence of any rounding logic around
src_rect(no calls toround, “nearest 2”, etc.). - Existing scaling for overall size (lines 171–180) multiplies logical bounds by the display’s scale factor—but this is not applied to the crop rectangle.
Suggested fix (replace lines ~195–203 with):
-if let Some(crop_bounds) = config.crop_bounds {
- tracing::info!("crop bounds: {:?}", crop_bounds);
- settings.set_src_rect(cg::Rect::new(
- crop_bounds.position().x(),
- crop_bounds.position().y(),
- crop_bounds.size().width(),
- crop_bounds.size().height(),
- ));
-}
+if let Some(crop_bounds) = config.crop_bounds {
+ tracing::info!("crop bounds (logical): {:?}", crop_bounds);
+ // Compute scale factor: use display.scale() if available, fallback to physical/logical ratio
+ let scale = display
+ .scale()
+ .unwrap_or_else(|| {
+ let phys = display.physical_size().unwrap().width() as f64;
+ let logi = display.logical_size().unwrap().width() as f64;
+ phys / logi
+ });
+ // Scale to physical pixels
+ let src_rect = cg::Rect::new(
+ crop_bounds.position().x() * scale,
+ crop_bounds.position().y() * scale,
+ crop_bounds.size().width() * scale,
+ crop_bounds.size().height() * scale,
+ );
+ settings.set_src_rect(src_rect);
+}No upstream rounding is currently applied, so there’s no ordering conflict; just ensure that any future rounding or alignment happens after this scaling step.
📝 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.
| if let Some(crop_bounds) = config.crop_bounds { | |
| tracing::info!("crop bounds: {:?}", crop_bounds); | |
| settings.set_src_rect(cg::Rect::new( | |
| crop_bounds.position().x(), | |
| crop_bounds.position().y(), | |
| crop_bounds.size().width(), | |
| crop_bounds.size().height(), | |
| )); | |
| } | |
| if let Some(crop_bounds) = config.crop_bounds { | |
| tracing::info!("crop bounds (logical): {:?}", crop_bounds); | |
| // Compute scale factor: use display.scale() if available, fallback to physical/logical ratio | |
| let scale = display | |
| .scale() | |
| .unwrap_or_else(|| { | |
| let phys = display.physical_size().unwrap().width() as f64; | |
| let logi = display.logical_size().unwrap().width() as f64; | |
| phys / logi | |
| }); | |
| // Scale to physical pixels | |
| let src_rect = cg::Rect::new( | |
| crop_bounds.position().x() * scale, | |
| crop_bounds.position().y() * scale, | |
| crop_bounds.size().width() * scale, | |
| crop_bounds.size().height() * scale, | |
| ); | |
| settings.set_src_rect(src_rect); | |
| } |
🤖 Prompt for AI Agents
In crates/recording/src/sources/screen_capture/macos.rs around lines 195–203,
the crop_bounds are being applied in logical (point) units while the stream size
was already converted to physical pixels; multiply the crop rectangle's position
and size by the display scale factor (the same scale used earlier for overall
size conversion) to convert crop_bounds to physical pixels before calling
settings.set_src_rect, making any rounding/alignment happen after this scaling
step and performing any necessary type casts to match cg::Rect constructor
arguments.
| impl Message<StopCapturing> for ScreenCaptureActor { | ||
| type Reply = Result<(), StopCapturingError>; | ||
|
|
||
| async fn handle( | ||
| &mut self, | ||
| _: StopCapturing, | ||
| _: &mut Context<Self, Self::Reply>, | ||
| ) -> Self::Reply { | ||
| if !self.capturing { | ||
| return Err(StopCapturingError::NotCapturing); | ||
| }; | ||
|
|
||
| if let Err(e) = self.capturer.stop().await { | ||
| error!("Silently failed to stop macOS capturer: {}", e); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Set capturing=false after a successful stop; else subsequent Start/Stop semantics break.
Not flipping the flag means the actor stays “capturing” even after stop; future StartCapturing yields AlreadyCapturing and StopCapturingError::NotCapturing may never trigger correctly.
- if let Err(e) = self.capturer.stop().await {
- error!("Silently failed to stop macOS capturer: {}", e);
- }
-
- Ok(())
+ if let Err(e) = self.capturer.stop().await {
+ error!("Silently failed to stop macOS capturer: {}", e);
+ } else {
+ self.capturing = false;
+ }
+ Ok(())📝 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.
| impl Message<StopCapturing> for ScreenCaptureActor { | |
| type Reply = Result<(), StopCapturingError>; | |
| async fn handle( | |
| &mut self, | |
| _: StopCapturing, | |
| _: &mut Context<Self, Self::Reply>, | |
| ) -> Self::Reply { | |
| if !self.capturing { | |
| return Err(StopCapturingError::NotCapturing); | |
| }; | |
| if let Err(e) = self.capturer.stop().await { | |
| error!("Silently failed to stop macOS capturer: {}", e); | |
| } | |
| Ok(()) | |
| } | |
| } | |
| impl Message<StopCapturing> for ScreenCaptureActor { | |
| type Reply = Result<(), StopCapturingError>; | |
| async fn handle( | |
| &mut self, | |
| _: StopCapturing, | |
| _: &mut Context<Self, Self::Reply>, | |
| ) -> Self::Reply { | |
| if !self.capturing { | |
| return Err(StopCapturingError::NotCapturing); | |
| }; | |
| if let Err(e) = self.capturer.stop().await { | |
| error!("Silently failed to stop macOS capturer: {}", e); | |
| } else { | |
| self.capturing = false; | |
| } | |
| Ok(()) | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/recording/src/sources/screen_capture/macos.rs around lines 348 to 366,
the actor never flips self.capturing to false after stopping, so subsequent
Start/Stop semantics break; after calling self.capturer.stop().await and
confirming it succeeded, set self.capturing = false before returning Ok() (keep
the existing error logging path unchanged so you only flip the flag on
successful stop).
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/scap-direct3d/src/lib.rs (1)
313-368: Frame::as_buffer is unsound: returns a slice into a temporary texture and never Unmap()s
- The staging
ID3D11Texture2Dis dropped whenas_bufferreturns, leavingdatapointing to freed memory (UAF).- The mapped resource is never unmapped, leaking a mapping and risking GPU/CPU sync issues.
- The texture format is constructed from a DirectXPixelFormat discriminant; prefer
as_dxgi().Own the staging texture within
FrameBufferand Unmap inDrop. Also, requestREADmapping if you don’t intend to write.@@ - pub fn as_buffer(&self) -> windows::core::Result<FrameBuffer<'a>> { + pub fn as_buffer(&self) -> windows::core::Result<FrameBuffer<'a>> { let texture_desc = D3D11_TEXTURE2D_DESC { Width: self.width, Height: self.height, MipLevels: 1, ArraySize: 1, - Format: DXGI_FORMAT(DirectXPixelFormat::R8G8B8A8UIntNormalized.0), // (self.color_format as i32), + Format: self.pixel_format.as_dxgi(), SampleDesc: DXGI_SAMPLE_DESC { Count: 1, Quality: 0, }, Usage: D3D11_USAGE_STAGING, BindFlags: 0, CPUAccessFlags: D3D11_CPU_ACCESS_READ.0 as u32 | D3D11_CPU_ACCESS_WRITE.0 as u32, MiscFlags: 0, }; @@ - let texture = texture.unwrap(); + let cpu_texture = texture.unwrap(); @@ // Copies GPU only texture to CPU-mappable texture unsafe { - self.d3d_context.CopyResource(&texture, &self.texture); + self.d3d_context.CopyResource(&cpu_texture, &self.texture); }; @@ - unsafe { - self.d3d_context.Map( - &texture, + unsafe { + self.d3d_context.Map( + &cpu_texture, 0, - D3D11_MAP_READ_WRITE, + D3D11_MAP_READ, 0, Some(&mut mapped_resource), )?; }; @@ - Ok(FrameBuffer { - data, - width: self.width, - height: self.height, - stride: mapped_resource.RowPitch, - pixel_format: self.pixel_format, - resource: mapped_resource, - }) + Ok(FrameBuffer { + data, + width: self.width, + height: self.height, + stride: mapped_resource.RowPitch, + resource: mapped_resource, + texture: cpu_texture, + d3d_context: self.d3d_context, + pixel_format: self.pixel_format, + }) }Update the buffer type to own the texture and context reference:
pub struct FrameBuffer<'a> { data: &'a mut [u8], width: u32, height: u32, stride: u32, - resource: D3D11_MAPPED_SUBRESOURCE, + resource: D3D11_MAPPED_SUBRESOURCE, + texture: ID3D11Texture2D, + d3d_context: &'a ID3D11DeviceContext, pixel_format: PixelFormat, }Optionally expose a mutable accessor if writes are intended:
- pub fn data(&self) -> &[u8] { - self.data - } + pub fn data(&self) -> &[u8] { self.data } + pub fn data_mut(&mut self) -> &mut [u8] { self.data }Add a Drop implementation (outside the shown ranges) to unmap:
impl<'a> Drop for FrameBuffer<'a> { fn drop(&mut self) { unsafe { self.d3d_context.Unmap(&self.texture, 0) }; } }Also applies to: 371-404
♻️ Duplicate comments (10)
crates/displays/src/platform/win.rs (3)
138-142: Fix invalid out-parameter in GetDpiForMonitor (&mut 0causes UB/compile error).You’re taking a mutable reference to a temporary for the Y-DPI and using an implicitly-typed
0fordpi_x. Initialize both asu32locals and pass their addresses.Apply this diff:
- let dpi = unsafe { - let mut dpi_x = 0; - GetDpiForMonitor(self.0, MDT_EFFECTIVE_DPI, &mut dpi_x, &mut 0).ok()?; - dpi_x - }; + let dpi = unsafe { + let mut dpi_x: u32 = 0; + let mut dpi_y: u32 = 0; + GetDpiForMonitor(self.0, MDT_EFFECTIVE_DPI, &mut dpi_x, &mut dpi_y).ok()?; + dpi_x + };
1231-1237: Use addr_of_mut! and qualify size_of in DwmGetWindowAttribute (logical_size).Prefer std::ptr::addr_of_mut! to avoid creating intermediate references, and qualify size_of via
mem::size_of. Current code won’t compile due to unqualifiedsize_of.Apply this diff:
- DwmGetWindowAttribute( - self.0, - DWMWA_EXTENDED_FRAME_BOUNDS, - (&raw mut rect).cast(), - size_of::<RECT>() as u32, - ) + DwmGetWindowAttribute( + self.0, + DWMWA_EXTENDED_FRAME_BOUNDS, + std::ptr::addr_of_mut!(rect).cast(), + mem::size_of::<RECT>() as u32, + )
1268-1274: Same fix: addr_of_mut! + qualified size_of in DwmGetWindowAttribute (physical_bounds).Mirror the logical_size change here as well.
Apply this diff:
- DwmGetWindowAttribute( - self.0, - DWMWA_EXTENDED_FRAME_BOUNDS, - (&raw mut rect).cast(), - size_of::<RECT>() as u32, - ) + DwmGetWindowAttribute( + self.0, + DWMWA_EXTENDED_FRAME_BOUNDS, + std::ptr::addr_of_mut!(rect).cast(), + mem::size_of::<RECT>() as u32, + )crates/recording/src/sources/screen_capture/windows.rs (3)
7-10: Fix missing imports: SystemTime, flume::Sender, and tracing macros (warn/error).SystemTime is used (Lines 46, 380) but not imported. Sender is referenced in multiple structs/args but flume::Sender isn’t imported. You also call warn! and error! but only imported info/trace, which will not compile.
use std::{ collections::VecDeque, - time::{Duration, Instant}, + time::{Duration, Instant, SystemTime}, }; -use tracing::{info, trace}; +use flume::Sender; +use tracing::{info, trace, warn, error};Also applies to: 11-11
31-41: Avoid panics in audio_info() by handling missing/default device/config gracefully.default_output_device() and default_output_config() can return None/Err on systems without an output device (VMs, RDP, headless). unwrap() will terminate the recording path.
fn audio_info() -> AudioInfo { - let host = cpal::default_host(); - let output_device = host.default_output_device().unwrap(); - let supported_config = output_device.default_output_config().unwrap(); - - let mut info = AudioInfo::from_stream_config(&supported_config); - - info.sample_format = ffmpeg::format::Sample::F32(ffmpeg::format::sample::Type::Packed); - - info + let host = cpal::default_host(); + // Try to derive from the system, otherwise fall back to a safe default (48 kHz stereo F32). + let mut info = (|| { + let output_device = host.default_output_device()?; + let supported_config = output_device.default_output_config().ok()?; + Some(AudioInfo::from_stream_config(&supported_config)) + })() + .unwrap_or_else(|| AudioInfo { + sample_format: ffmpeg::format::Sample::F32(ffmpeg::format::sample::Type::Packed), + sample_rate: 48_000, + channels: 2, + time_base: AV_TIME_BASE_Q, + buffer_size: 1024, + }); + + // Force F32 packed for our encoding pipeline consistency. + info.sample_format = ffmpeg::format::Sample::F32(ffmpeg::format::sample::Type::Packed); + info }
162-171: On high drop rate, also stop the capturer, not just this handler.Currently you only stop the FrameHandler; the capture thread continues to produce frames until an external shutdown occurs. Proactively stop the ScreenCaptureActor too to free resources immediately.
if drop_rate > MAX_DROP_RATE_THRESHOLD && total_count >= 10 { error!( "High frame drop rate detected: {:.1}% ({}/{} frames in last {}s). Exiting capture.", drop_rate * 100.0, dropped_count, total_count, WINDOW_DURATION.as_secs() ); - let _ = ctx.actor_ref().stop_gracefully().await; + if let Some(c) = self.capturer.upgrade() { + let _ = c.ask(StopCapturing).await; + } + let _ = ctx.actor_ref().stop_gracefully().await; return; }crates/recording/src/sources/screen_capture/macos.rs (3)
61-68: Do not use blocking flume::Sender::send in async handler; await send_asyncCalling the blocking
sendinside an async actor can stall the runtime when the channel is full. Usesend_async(...).await(ortry_send) to avoid deadlocks and dropped wakers.- if check_skip_send().is_ok() - && self - .video_tx - .send((sample_buffer.retained(), relative_time)) - .is_err() - { - warn!("Pipeline is unreachable"); - } + if check_skip_send().is_ok() { + if let Err(_) = self + .video_tx + .send_async((sample_buffer.retained(), relative_time)) + .await + { + warn!("Pipeline is unreachable"); + } + }Run to find other blocking sends that might need conversion:
#!/bin/bash # Search for flume blocking send usages in this crate rg -nP '\.send\s*\(' -C2 crates/recording | sed -n '1,200p'
195-203: Crop rect must be scaled to physical pixels (and aligned to even)
sizeis computed in physical pixels, butcrop_boundsappears to be in logical units. Apply the same scale used above, then align to even integers to match the “Round capture bounds to nearest 2” objective.- if let Some(crop_bounds) = config.crop_bounds { - tracing::info!("crop bounds: {:?}", crop_bounds); - settings.set_src_rect(cg::Rect::new( - crop_bounds.position().x(), - crop_bounds.position().y(), - crop_bounds.size().width(), - crop_bounds.size().height(), - )); - } + if let Some(crop_bounds) = config.crop_bounds { + tracing::info!("crop bounds (logical): {:?}", crop_bounds); + let scale = display + .scale() + .unwrap_or_else(|| { + let phys = display.physical_size().unwrap().width() as f64; + let logi = display.logical_size().unwrap().width() as f64; + phys / logi + }); + // Convert to physical pixels and align to even (nearest 2) + let to_even = |v: f64| ((v.round() as i64) & !1) as f64; + let x = to_even(crop_bounds.position().x() * scale); + let y = to_even(crop_bounds.position().y() * scale); + let w = to_even(crop_bounds.size().width() * scale); + let h = to_even(crop_bounds.size().height() * scale); + settings.set_src_rect(cg::Rect::new(x, y, w, h)); + }
352-369: Flip capturing=false on successful stopWithout resetting the flag, subsequent Start/Stop semantics break (AlreadyCapturing on next start).
- if let Err(e) = self.capturer.stop().await { - error!("Silently failed to stop macOS capturer: {}", e); - } - - Ok(()) + if let Err(e) = self.capturer.stop().await { + error!("Silently failed to stop macOS capturer: {}", e); + } else { + self.capturing = false; + } + Ok(())crates/scap-direct3d/src/lib.rs (1)
93-107: Capability property name swap from prior review is fixed
can_is_border_required()now checks"IsBorderRequired"andcan_is_cursor_capture_enabled()checks"IsCursorCaptureEnabled". Good catch and correction.
🧹 Nitpick comments (24)
crates/displays/src/platform/win.rs (6)
220-257: Consider richer monitor naming fallback chain.
name()fetches the adapter’s device string, which is fine but often generic. You already implemented better fallbacks (get_friendly_name_from_displayconfig,get_friendly_name_from_edid,get_friendly_name_from_registry,get_friendly_name_from_wmi). Consider trying them (in that order) if EnumDisplayDevicesW returns empty/generic.Example sketch (outside this range):
- Try
get_friendly_name_from_displayconfig()first.- Fallback to EDID, then registry paths, then WMI.
- If all fail, keep current string.
684-686: EnumChildWindows(GetDesktopWindow()) vs EnumWindows: confirm coverage across desktops.Enumerating child windows of the desktop window typically yields top-level windows, but behavior can differ across desktop/virtual desktop scenarios. If you relied on
EnumWindowsbefore, double-check that UWP, UAC prompts, and virtual desktops remain discoverable as expected.Would you like me to add a fallback to
EnumWindowsifEnumChildWindowsyields zero results?
1356-1358: Case-insensitive, canonical path check for SystemApps filter.
PathBuf::starts_with(&str)is case-sensitive; on Windows this can miss matches (e.g., “c:\windows\SystemApps”). Canonicalize and compare paths (or compare lowercase drive+components) to avoid false negatives.Apply this diff:
- if owner_process_path.starts_with("C:\\Windows\\SystemApps") { - return false; - } + if let Ok(system_root) = std::env::var("SystemRoot") { + let system_apps = PathBuf::from(system_root).join("SystemApps"); + if let (Ok(apps), Ok(owner_abs)) = (std::fs::canonicalize(system_apps), std::fs::canonicalize(&owner_process_path)) { + if owner_abs.starts_with(apps) { + return false; + } + } else { + // Fallback: case-insensitive string prefix check + if owner_process_path + .to_string_lossy() + .to_lowercase() + .starts_with("c:\\windows\\systemapps") + { + return false; + } + } + }
1078-1212: Icon extraction loop is very heavy; cap sizes and short-circuit earlier.Trying 2048→16 for every icon can be expensive (GDI allocations + DrawIconEx + GetDIBits per size). Prioritize native size, then a small fixed set (e.g., 256, 128, 64) and break once you’ve captured a sufficiently large, non-transparent bitmap.
Proposed adjustment within this block:
- If native size is available and ≥128, try only [native, 256] (unique).
- Otherwise try [256, 128, 64, 32]; break after first success ≥128.
- Consider SHGetStockIconInfo / SHGetImageList for file/known-class icons when applicable.
1524-1545: Makepid_to_exe_patha safe function; removeunsafefrom signature.The body already contains the necessary unsafe blocks for FFI calls. Exposing it as
unsafe fnleaks implementation details and burdens callers.Apply this diff:
-unsafe fn pid_to_exe_path(pid: u32) -> Result<PathBuf, windows::core::Error> { +fn pid_to_exe_path(pid: u32) -> Result<PathBuf, windows::core::Error> { @@ - let query = unsafe { - QueryFullProcessImageNameW( - handle, - PROCESS_NAME_FORMAT::default(), - windows::core::PWSTR(lpexename.as_mut_ptr()), - &mut lpdwsize, - ) - }; - unsafe { CloseHandle(handle) }.ok(); + let query = QueryFullProcessImageNameW( + handle, + PROCESS_NAME_FORMAT::default(), + windows::core::PWSTR(lpexename.as_mut_ptr()), + &mut lpdwsize, + ); + CloseHandle(handle).ok();Also update callers to drop the
unsafe { ... }aroundpid_to_exe_path.
644-651: Minor consistency: use constructors from bounds types.Elsewhere you construct bounds via
PhysicalBounds::newetc. ConsiderPhysicalPosition::new(point.x as f64, point.y as f64)for consistency.crates/recording/src/sources/screen_capture/windows.rs (4)
144-148: Reduce log spam on backpressure drops.warn! per dropped frame can be noisy at high FPS. Consider trace!/debug! here and rely on the periodic drop-rate log for visibility.
- warn!("Screen capture sender is full, dropping frame"); + trace!("Screen capture sender is full, dropping frame");
495-497: Replace dbg! in audio error callback with structured logging.dbg! is noisy and not user-friendly in production. Prefer tracing.
- move |e| { - dbg!(e); - }, + move |e| { + warn!(error = ?e, "WindowsAudioCapture stream error"); + },
395-399: Optional: add a cap_fail point for CreateCapturer to aid fault-injection tests.You already have a failpoint for StartCapturing; adding one here helps validate recovery paths earlier in setup.
fail_err!( "WindowsScreenCapture.StartCapturing", StartCapturingError::CreateCapturer(scap_direct3d::NewCapturerError::NotSupported) ); + fail_err!( + "WindowsScreenCapture.CreateCapturer", + StartCapturingError::CreateCapturer(scap_direct3d::NewCapturerError::NotSupported) + );
300-339: Minor: consider annotating run() with #[instrument(skip_all)] for observability.The comment suggests it was intended. Re-enabling it would improve trace spans for source lifecycle.
crates/recording/src/sources/screen_capture/macos.rs (5)
45-48: Guard against negative relative_time (clock skew/drift)A small host/unix clock skew can yield negative
relative_time, which will later produce negative PTS. Clamp to zero to avoid downstream timestamp underflow.- let relative_time = unix_timestamp - self.start_time_f64; + let relative_time = (unix_timestamp - self.start_time_f64).max(0.0);
103-104: Set audio time_base to match PTS unitsYou set PTS in AV_TIME_BASE units but don’t set the frame’s time_base. Some consumers assume PTS is in the frame’s time_base. Set it explicitly.
- frame.set_pts(Some((relative_time * AV_TIME_BASE_Q.den as f64) as i64)); + frame.set_time_base(AV_TIME_BASE_Q); + frame.set_pts(Some((relative_time * AV_TIME_BASE_Q.den as f64) as i64));
217-217: Remove unused stop_recipient binding
stop_recipientis created but never used; drop it to avoid warnings.- let stop_recipient = capturer.clone().reply_recipient::<StopCapturing>(); + // (removed) unused stop_recipient
205-206: Consider larger error channel or coalescing errorsA bounded(1) error channel plus blocking sends in callbacks can deadlock; after switching to
try_send, bursts of errors may still be dropped. If you need to surface the first error only, great; otherwise increase capacity or coalesce error kinds.
319-321: Remove unusedCaptureErrorstruct or integrate its usageThe
CaptureErrortype is defined atcrates/recording/src/sources/screen_capture/macos.rs:319but isn’t referenced anywhere in the crate (aside from a commented‐out hint in the Windows variant). You should either:
- Remove the
pub struct CaptureError(pub arc::R<ns::Error>);definition if it’s no longer needed.- Or integrate it into your error‐handling API (for example, by adding an
error_handler: Option<Recipient<CaptureError>>parameter or documenting its intended use).This will keep the codebase clean and avoid unused‐type warnings.
apps/desktop/src-tauri/src/windows.rs (3)
733-741: Trait naming and API: MonitorExt now targets Display and leaks scale as parameterThe trait is named MonitorExt but is implemented for Display and requires callers to supply a scale. Prefer renaming (DisplayExt) and encapsulating scale retrieval inside the implementation to avoid call-site mistakes (as seen above).
If Display can provide its own scale, drop the scale argument:
-trait MonitorExt { - fn intersects( - &self, - position: PhysicalPosition<i32>, - size: PhysicalSize<u32>, - scale: f64, - ) -> bool; -} +trait DisplayExt { + fn intersects( + &self, + position: PhysicalPosition<i32>, + size: PhysicalSize<u32>, + ) -> bool; +}And read the scale internally on macOS via raw_handle().scale_factor(). This avoids future bugs.
646-647: Avoid unnecessary clone if DisplayId is CopyIf cap_displays::DisplayId is Copy (likely a newtype around an integer/handle), you can pass by value without cloning.
- screen_id: screen_id.clone(), + screen_id: *screen_id,If it isn’t Copy, keep clone().
800-814: set_window_transparent is macOS-only; consider Windows implementation or explicit no-opOn Windows, toggling opacity typically involves layered windows (WS_EX_LAYERED) or DWM attributes; currently this command is a silent no-op. Either implement Windows behavior or document that Windows ignores it to prevent confusion at call sites.
I can sketch a Windows implementation using DwmSetWindowAttribute if you want to support per-window toggles, or add an early platform check that logs a warning on non-macOS.
crates/scap-direct3d/src/lib.rs (6)
254-263: Avoid magic HRESULT; use the named constant for “no message queue yet”Hard-coding
-2147023452is brittle. Prefer the Win32 constantERROR_INVALID_THREAD_IDviaHRESULT::from_win32.- if e.code().0 != -2147023452 { + use windows::Win32::Foundation::{HRESULT, ERROR_INVALID_THREAD_ID}; + if e.code() != HRESULT::from_win32(ERROR_INVALID_THREAD_ID.0) { return Err(StopCapturerError::PostMessageFailed); }Also add the import near your other Foundation imports:
- Foundation::{HANDLE, HMODULE, LPARAM, S_FALSE, WPARAM}, + Foundation::{HANDLE, HMODULE, LPARAM, S_FALSE, WPARAM, ERROR_INVALID_THREAD_ID},
534-593: Handle dynamic size changes by recreating the frame poolWindows Graphics Capture requires recreating the frame pool when
ContentSizechanges. Without this, you risk bad frames or access violations when the target is resized.A minimal approach:
- Track
last_sizeoutside the handler:let mut last_size = item.Size()?;- Inside
FrameArrived, after readinglet size = frame.ContentSize()?;, ifsize != last_size, call:
frame_pool.as_ref().unwrap().Recreate(&direct3d_device, settings.pixel_format.as_directx(), 1, size)?;thenlast_size = size;
56-75: PixelFormat API looks good; minor forward-compatibility noteThe new
as_dxgi()complementsas_directx()well. Consider addingPartialEq/Eqif you’ll compare formats and usingFrom<PixelFormat> for DXGI_FORMATto ease call sites, but this is fine to land as-is.
116-132: Enum variant name mismatch:RecvTimeoutactually wrapsRecvErrorThis variant name suggests a timeout, but
std::sync::mpsc::RecvErrorindicates channel disconnect. Either rename toRecvFailedfor clarity or remove the variant if unused.
243-270: Stop flow is solid; consider surfacing the underlying OS errorIf
PostThreadMessageWkeeps failing with a different error, returning a barePostMessageFailedcan be opaque. Consider carrying thewindows::core::Errorfor diagnostics.- #[error("PostMessageFailed")] - PostMessageFailed, + #[error("PostMessageFailed: {0}")] + PostMessageFailed(#[source] windows::core::Error),And pass
ethrough in the error path.
534-590: Callback error handling is propagated to WinRT; confirm desired behaviorReturning an
ErrfromFrameArrivedwill bubble up to WinRT and may stop further callbacks. If you’d rather keep capturing and only log failures, swallow errors inside the closure.- (callback)(frame) + if let Err(e) = (callback)(frame) { + // TODO: route to your error bus/log + let _ = e; + } + Ok(())
📜 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 (7)
apps/desktop/src-tauri/src/windows.rs(13 hunks)crates/displays/src/main.rs(3 hunks)crates/displays/src/platform/win.rs(12 hunks)crates/recording/src/sources/screen_capture/macos.rs(1 hunks)crates/recording/src/sources/screen_capture/windows.rs(1 hunks)crates/scap-cpal/src/lib.rs(1 hunks)crates/scap-direct3d/src/lib.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/scap-cpal/src/lib.rs
- crates/displays/src/main.rs
🧰 Additional context used
🧬 Code graph analysis (2)
crates/scap-direct3d/src/lib.rs (1)
crates/recording/src/sources/screen_capture/windows.rs (8)
new(352-357)new(471-501)handle(121-186)handle(386-435)handle(441-457)handle(510-518)handle(524-532)run(207-342)
crates/recording/src/sources/screen_capture/windows.rs (4)
crates/scap-direct3d/src/lib.rs (7)
run(611-617)new(142-172)can_is_border_required(94-99)can_is_cursor_capture_enabled(101-106)can_min_update_interval(108-113)stop(244-269)data(393-395)crates/media-info/src/lib.rs (1)
from_stream_config(46-64)crates/displays/src/lib.rs (5)
id(28-30)id(118-120)display(146-148)from_id(32-34)from_id(122-124)crates/scap-cpal/src/lib.rs (2)
config(67-69)create_capturer(17-48)
⏰ 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 (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (9)
crates/displays/src/platform/win.rs (2)
1218-1229: DPI-awareness gate is good; ensure this matches the process manifest.Bailing out unless
PROCESS_PER_MONITOR_DPI_AWAREis set aligns with your file-level assumption. Just ensure the host process actually opts into Per-Monitor V2/V1 DPI awareness; otherwise these Option-returning APIs will frequently be None.
638-641: Interop creation looks solid.Using
factory::<GraphicsCaptureItem, IGraphicsCaptureItemInterop>()andCreateForMonitoris the right way to bridge WinRT interop for capture.crates/recording/src/sources/screen_capture/windows.rs (1)
283-297: Good fix: removed unwrap() from WindowsAudioCapture::new call site.Gracefully mapping the constructor error to SourceError avoids panics when audio init fails. Thanks for addressing the earlier feedback.
crates/recording/src/sources/screen_capture/macos.rs (2)
11-13: BGRA pixel format selection matches CoreVideo cfgMapping
ScreenCaptureFormat::pixel_format()to FFmpeg BGRA aligns withcv::PixelFormat::_32_BGRAset later in the stream config. Good consistency.
15-22: Audio info defaults are sane for macOS capturePlanar F32 at 48 kHz stereo is a solid default and matches typical CoreAudio output. No issues here.
apps/desktop/src-tauri/src/windows.rs (3)
233-241: Main window content_protected(true) affects OS-level screenshots and overlaysEnabling content_protected(true) on the Main window will block OS screenshotting (and sometimes thumbnails) and may interfere with your own in-app capture previews depending on platform. Confirm this is intended UX.
494-494: Increase InProgressRecording width to 250.0 — LGTMThe new width looks fine and should improve label legibility.
39-41: DisplayId parse/format symmetry confirmed
A quick audit shows no remaining legacyu32‐based labels and theDisplay/FromStrimplementations for your window variants use exactly matching prefixes:
- Running
rg -nP 'window-capture-occluder-\d+|target-select-overlay-\d+'returned no hits, so there are no leftoveru32labels.- The
Displayimpl forCapWindowIdemits"window-capture-occluder-{}"and"target-select-overlay-{}", and theFromStrarm strips the identical prefixes before parsing intoDisplayId.No further changes needed here.
crates/scap-direct3d/src/lib.rs (1)
77-82: Support checks are conservative and correctCombining
UniversalApiContract >= 8withGraphicsCaptureSession::IsSupported()avoids false positives on older systems. LGTM.
| WindowCaptureOccluder { screen_id: DisplayId }, | ||
| TargetSelectOverlay { display_id: DisplayId }, | ||
| CaptureArea { screen_id: u32 }, | ||
| CaptureArea { screen_id: DisplayId }, | ||
| Camera, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
CaptureArea window can’t switch displays once created (ID doesn’t carry DisplayId)
ShowCapWindow::CaptureArea carries a screen_id but CapWindowId::CaptureArea has no payload. If a CaptureArea window already exists, show() focuses and returns early without relocating/resizing for a different display. Users changing displays will see the stale placement.
Two approaches:
- Make CapWindowId::CaptureArea carry DisplayId to allow multiple instances or re-init per display.
- Or, keep a single CaptureArea window but, when it exists, update its size/position based on the new screen_id before returning.
Here’s a minimal change to update position/size when the window already exists (avoid changing CapWindowId for now):
- if let Some(window) = self.id(app).get(app) {
- window.set_focus().ok();
- return Ok(window);
- }
+ if let Some(window) = self.id(app).get(app) {
+ if let ShowCapWindow::CaptureArea { screen_id } = self {
+ if let Some(display) = Display::from_id(screen_id) {
+ #[cfg(target_os = "macos")]
+ if let Some(bounds) = display.raw_handle().logical_bounds() {
+ let _ = window.set_size(tauri::Size::Logical(tauri::LogicalSize {
+ width: bounds.size().width(),
+ height: bounds.size().height(),
+ }));
+ let _ = window.set_position(tauri::Position::Logical(tauri::LogicalPosition {
+ x: bounds.position().x(),
+ y: bounds.position().y(),
+ }));
+ }
+ #[cfg(windows)]
+ if let Some(bounds) = display.raw_handle().physical_bounds() {
+ let scale = display.raw_handle().scale_factor().unwrap_or(1.0);
+ let _ = window.set_size(tauri::Size::Logical(tauri::LogicalSize {
+ width: bounds.size().width() as f64 / scale,
+ height: bounds.size().height() as f64 / scale,
+ }));
+ let _ = window.set_position(tauri::Position::Logical(tauri::LogicalPosition {
+ x: bounds.position().x() as f64 / scale,
+ y: bounds.position().y() as f64 / scale,
+ }));
+ }
+ }
+ } else {
+ window.set_focus().ok();
+ return Ok(window);
+ }
+ }Also applies to: 644-649
🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/windows.rs around lines 183-186 (also apply same
fix to the logic at lines ~644-649), the problem is that
ShowCapWindow::CaptureArea includes a screen_id but CapWindowId::CaptureArea
carries no DisplayId, so when a CaptureArea window already exists show() simply
focuses and returns without relocating/resizing for a new display; update the
existing window before returning by: look up the DisplayId passed to show(),
compute the target position/size for that display (using the same display bounds
logic used when creating a new CaptureArea), call the window APIs to set outer
position and size (or inner size/position) to those computed values, then focus
and return; apply this same update-at-existing-window behavior in both places
noted (183-186 and 644-649).
| let Some(display) = cap_displays::Display::from_id(display_id) else { | ||
| return Err(tauri::Error::WindowNotFound); | ||
| }; | ||
|
|
||
| let size = display.raw_handle().logical_size(); | ||
| #[cfg(target_os = "macos")] | ||
| let position = display.raw_handle().logical_position(); | ||
| #[cfg(target_os = "macos")] | ||
| let size = display.logical_size().unwrap(); | ||
|
|
||
| #[cfg(windows)] | ||
| let position = display.raw_handle().physical_position().unwrap(); | ||
| #[cfg(windows)] | ||
| let size = display.physical_size().unwrap(); | ||
|
|
There was a problem hiding this comment.
Windows: Using physical bounds directly leads to misplacement on HiDPI; convert to logical units
Tauri’s builder expects logical coordinates. On Windows you’re using physical_position()/physical_size() directly, which will misplace and mis-size TargetSelectOverlay on non-100% scale monitors. Convert using the display’s scale factor (keep macOS path as-is).
Apply:
- #[cfg(windows)]
- let position = display.raw_handle().physical_position().unwrap();
- #[cfg(windows)]
- let size = display.physical_size().unwrap();
+ #[cfg(windows)]
+ let (position, size) = {
+ let pos = display.raw_handle().physical_position().unwrap();
+ let sz = display.physical_size().unwrap();
+ let scale = display.raw_handle().scale_factor().unwrap_or(1.0);
+ (
+ (pos.x() as f64 / scale, pos.y() as f64 / scale),
+ (sz.width() as f64 / scale, sz.height() as f64 / scale),
+ )
+ };
@@
- .inner_size(size.width(), size.height())
- .position(position.x(), position.y())
+ .inner_size(size.0, size.1)
+ .position(position.0, position.1)Also applies to: 281-283
| Self::WindowCaptureOccluder { screen_id } => { | ||
| let Some(bounds) = logical_monitor_bounds(*screen_id) else { | ||
| let Some(display) = Display::from_id(screen_id) else { | ||
| return Err(tauri::Error::WindowNotFound); | ||
| }; | ||
|
|
||
| #[cfg(target_os = "macos")] | ||
| let position = display.raw_handle().logical_position(); | ||
|
|
||
| #[cfg(windows)] | ||
| let position = display.raw_handle().physical_position().unwrap(); | ||
|
|
||
| let bounds = display.physical_size().unwrap(); | ||
|
|
||
| let mut window_builder = self |
There was a problem hiding this comment.
Mixed coordinate spaces (macOS logical position + physical size; Windows physical position/size)
WindowCaptureOccluder uses logical position with physical size on macOS, and physical position/size on Windows. Both are inconsistent with Tauri’s logical expectations and will produce wrong placements on scaled displays.
Use logical units for both OSes:
- #[cfg(target_os = "macos")]
- let position = display.raw_handle().logical_position();
+ #[cfg(target_os = "macos")]
+ let position = display.raw_handle().logical_position();
@@
- #[cfg(windows)]
- let position = display.raw_handle().physical_position().unwrap();
+ #[cfg(windows)]
+ let position = {
+ let p = display.raw_handle().physical_position().unwrap();
+ let scale = display.raw_handle().scale_factor().unwrap_or(1.0);
+ (p.x() as f64 / scale, p.y() as f64 / scale)
+ };
@@
- let bounds = display.physical_size().unwrap();
+ #[cfg(target_os = "macos")]
+ let bounds = display.logical_size().ok_or(tauri::Error::WindowNotFound)?;
+ #[cfg(windows)]
+ let bounds = {
+ let s = display.physical_size().unwrap();
+ let scale = display.raw_handle().scale_factor().unwrap_or(1.0);
+ (s.width() as f64 / scale, s.height() as f64 / scale)
+ };
@@
- .inner_size(bounds.width(), bounds.height())
- .position(position.x(), position.y())
+ .inner_size(
+ #[cfg(target_os = "macos")]
+ { bounds.width() }
+ #[cfg(windows)]
+ { bounds.0 },
+ #[cfg(target_os = "macos")]
+ { bounds.height() }
+ #[cfg(windows)]
+ { bounds.1 },
+ )
+ .position(
+ #[cfg(target_os = "macos")]
+ { position.x() }
+ #[cfg(windows)]
+ { position.0 },
+ #[cfg(target_os = "macos")]
+ { position.y() }
+ #[cfg(windows)]
+ { position.1 },
+ )Also applies to: 427-429
🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/windows.rs around lines 404-417 (and also applied
to 427-429), the code mixes macOS logical position with physical size and uses
physical position/size on Windows; change it to use logical coordinates
consistently: obtain the display position and size in logical units (either by
calling logical_position() and logical_size() where available or by dividing
physical values by the display scale factor) for both macOS and Windows, replace
uses of physical_position()/physical_size() with their logical equivalents,
handle Option/Result unwraps safely (propagate/return an error if absent) and
ensure the window builder uses the computed logical position and logical size.
| let Some(display) = Display::from_id(screen_id) else { | ||
| return Err(tauri::Error::WindowNotFound); | ||
| }; | ||
|
|
||
| #[cfg(target_os = "macos")] | ||
| if let Some(bounds) = display.raw_handle().logical_bounds() { | ||
| window_builder = window_builder | ||
| .inner_size(bounds.size().width(), bounds.size().height()) | ||
| .position(bounds.position().x(), bounds.position().y()); | ||
| } | ||
|
|
||
| #[cfg(windows)] | ||
| if let Some(bounds) = display.raw_handle().physical_bounds() { | ||
| window_builder = window_builder | ||
| .inner_size(bounds.size().width(), bounds.size().height()) | ||
| .position(bounds.position().x(), bounds.position().y()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
CaptureArea: Windows path should convert physical bounds to logical
The macOS path uses logical_bounds(); Windows path applies physical_bounds() directly to inner_size/position, which will be wrong under DPI scaling. Convert by the display scale.
- #[cfg(windows)]
- if let Some(bounds) = display.raw_handle().physical_bounds() {
- window_builder = window_builder
- .inner_size(bounds.size().width(), bounds.size().height())
- .position(bounds.position().x(), bounds.position().y());
- }
+ #[cfg(windows)]
+ if let Some(bounds) = display.raw_handle().physical_bounds() {
+ let scale = display.raw_handle().scale_factor().unwrap_or(1.0);
+ window_builder = window_builder
+ .inner_size(
+ bounds.size().width() as f64 / scale,
+ bounds.size().height() as f64 / scale,
+ )
+ .position(
+ bounds.position().x() as f64 / scale,
+ bounds.position().y() as f64 / scale,
+ );
+ }📝 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.
| let Some(display) = Display::from_id(screen_id) else { | |
| return Err(tauri::Error::WindowNotFound); | |
| }; | |
| #[cfg(target_os = "macos")] | |
| if let Some(bounds) = display.raw_handle().logical_bounds() { | |
| window_builder = window_builder | |
| .inner_size(bounds.size().width(), bounds.size().height()) | |
| .position(bounds.position().x(), bounds.position().y()); | |
| } | |
| #[cfg(windows)] | |
| if let Some(bounds) = display.raw_handle().physical_bounds() { | |
| window_builder = window_builder | |
| .inner_size(bounds.size().width(), bounds.size().height()) | |
| .position(bounds.position().x(), bounds.position().y()); | |
| } | |
| #[cfg(windows)] | |
| if let Some(bounds) = display.raw_handle().physical_bounds() { | |
| // On Windows, physical_bounds are in device pixels—convert to logical pixels using the scale factor | |
| let scale = display.raw_handle().scale_factor().unwrap_or(1.0); | |
| window_builder = window_builder | |
| .inner_size( | |
| bounds.size().width() as f64 / scale, | |
| bounds.size().height() as f64 / scale, | |
| ) | |
| .position( | |
| bounds.position().x() as f64 / scale, | |
| bounds.position().y() as f64 / scale, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/windows.rs around lines 455 to 471, the Windows
branch uses physical_bounds() directly which yields device (physical) pixels and
will be incorrect under DPI scaling; compute the display scale
(display.scale_factor()), convert the physical bounds to logical pixels by
dividing width/height and x/y by the scale factor (handle scale > 0), then pass
those converted logical dimensions/positions to window_builder.inner_size(...)
and .position(...); ensure you convert the resulting floating values to the
integer types expected by inner_size/position (round/ceil as appropriate).
| if let Some(main_window) = CapWindowId::Main.get(app) | ||
| && let (Ok(outer_pos), Ok(outer_size)) = | ||
| (main_window.outer_position(), main_window.outer_size()) | ||
| && target_monitor.intersects(outer_pos, outer_size) | ||
| && let Ok(scale_factor) = main_window.scale_factor() | ||
| && display.intersects(outer_pos, outer_size, scale_factor) | ||
| { |
There was a problem hiding this comment.
Scale factor bug: using main window scale instead of target display scale
On macOS you convert display logical bounds to physical using a scale parameter, but you pass main_window.scale_factor(). That’s incorrect if the main window is on a different DPI display than the target display. Use the target display’s scale.
Replace the chained if-let with nested checks and get the display scale:
- if let Some(main_window) = CapWindowId::Main.get(app)
- && let (Ok(outer_pos), Ok(outer_size)) =
- (main_window.outer_position(), main_window.outer_size())
- && let Ok(scale_factor) = main_window.scale_factor()
- && display.intersects(outer_pos, outer_size, scale_factor)
- {
- let _ = main_window.minimize();
- };
+ if let Some(main_window) = CapWindowId::Main.get(app) {
+ if let (Ok(outer_pos), Ok(outer_size)) =
+ (main_window.outer_position(), main_window.outer_size())
+ {
+ #[cfg(target_os = "macos")]
+ let scale_factor = display.raw_handle().scale_factor().unwrap_or(1.0);
+ #[cfg(windows)]
+ let scale_factor = 1.0; // Windows path uses physical bounds, scale unused
+ if display.intersects(outer_pos, outer_size, scale_factor) {
+ let _ = main_window.minimize();
+ }
+ }
+ };Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/windows.rs around lines 482 to 487, the chained
if-let uses main_window.scale_factor() when converting display logical bounds to
physical, which is wrong if the main window is on a different DPI display;
replace the chained pattern with nested checks: first get
CapWindowId::Main.get(app) and retrieve outer_position and outer_size, then
fetch the target display's scale using display.scale_factor() and finally call
display.intersects(outer_pos, outer_size, display_scale). Ensure you stop using
main_window.scale_factor() and use the display's scale value, restructuring the
condition into nested ifs to avoid borrowing/temporary issues.
| if let Ok(true) = scap_direct3d::Settings::can_min_update_interval() { | ||
| settings.min_update_interval = | ||
| Some(Duration::from_secs_f64(1.0 / config.fps as f64)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against fps == 0 when setting min_update_interval.
Division by zero or an infinite Duration can happen if config.fps is zero. Add a guard and log.
- if let Ok(true) = scap_direct3d::Settings::can_min_update_interval() {
- settings.min_update_interval =
- Some(Duration::from_secs_f64(1.0 / config.fps as f64));
- }
+ if let Ok(true) = scap_direct3d::Settings::can_min_update_interval() {
+ if config.fps > 0 {
+ settings.min_update_interval =
+ Some(Duration::from_secs_f64(1.0 / config.fps as f64));
+ } else {
+ warn!("config.fps == 0; skipping min_update_interval configuration");
+ }
+ }📝 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.
| if let Ok(true) = scap_direct3d::Settings::can_min_update_interval() { | |
| settings.min_update_interval = | |
| Some(Duration::from_secs_f64(1.0 / config.fps as f64)); | |
| } | |
| if let Ok(true) = scap_direct3d::Settings::can_min_update_interval() { | |
| if config.fps > 0 { | |
| settings.min_update_interval = | |
| Some(Duration::from_secs_f64(1.0 / config.fps as f64)); | |
| } else { | |
| warn!("config.fps == 0; skipping min_update_interval configuration"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/recording/src/sources/screen_capture/windows.rs around lines 261 to
264, guard against config.fps being zero before computing
Duration::from_secs_f64(1.0 / config.fps as f64); change the logic to check that
config.fps > 0 (or as u32/u64 conversion yields non-zero) before setting
settings.min_update_interval, compute the interval only when fps is non-zero,
otherwise leave min_update_interval as None and emit a warning/info log that fps
was zero and min_update_interval was skipped; ensure no division by zero or
creation of infinite/NaN Duration occurs.
| let display_time = SystemTime::now(); | ||
| let ff_frame = frame.as_ffmpeg().unwrap(); | ||
|
|
||
| let _ = msg | ||
| .frame_handler | ||
| .tell(NewFrame { | ||
| ff_frame, | ||
| display_time, | ||
| }) | ||
| .try_send(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t unwrap() frame.as_ffmpeg(); log and drop bad frames.
If conversion fails, unwrap() will panic and kill capture. Drop the frame with a warning.
- let display_time = SystemTime::now();
- let ff_frame = frame.as_ffmpeg().unwrap();
+ let display_time = SystemTime::now();
+ let ff_frame = match frame.as_ffmpeg() {
+ Ok(v) => v,
+ Err(e) => {
+ warn!(error = ?e, "Failed to convert D3D frame to ffmpeg frame; dropping");
+ return Ok(());
+ }
+ };📝 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.
| let display_time = SystemTime::now(); | |
| let ff_frame = frame.as_ffmpeg().unwrap(); | |
| let _ = msg | |
| .frame_handler | |
| .tell(NewFrame { | |
| ff_frame, | |
| display_time, | |
| }) | |
| .try_send(); | |
| let display_time = SystemTime::now(); | |
| let ff_frame = match frame.as_ffmpeg() { | |
| Ok(v) => v, | |
| Err(e) => { | |
| warn!(error = ?e, "Failed to convert D3D frame to ffmpeg frame; dropping"); | |
| return Ok(()); | |
| } | |
| }; | |
| let _ = msg | |
| .frame_handler | |
| .tell(NewFrame { | |
| ff_frame, | |
| display_time, | |
| }) | |
| .try_send(); |
🤖 Prompt for AI Agents
In crates/recording/src/sources/screen_capture/windows.rs around lines 409 to
419, calling frame.as_ffmpeg().unwrap() can panic on conversion failure; instead
attempt the conversion with match or if let, log a warning when conversion
returns Err and drop that frame (do not call .tell/.try_send for it), and only
create/send NewFrame when as_ffmpeg() succeeds; use the crate's existing logging
mechanism (e.g., warn! or tracing::warn!) and return/continue early on failure
to avoid killing the capture loop.
| if let Err(e) = unsafe { RoInitialize(RO_INIT_MULTITHREADED) } | ||
| && e.code() != S_FALSE | ||
| { | ||
| return; | ||
| // return Err(CreateRunnerError::FailedToInitializeWinRT); | ||
| } | ||
|
|
There was a problem hiding this comment.
RoInitialize check is invalid and swallows actionable error context
- The
if let Err(e) = ... && e.code() != S_FALSEconstruct does not compile in Rust. RoInitializein windows-rs treatsS_FALSEas success, so the special-case is unnecessary.- Currently you early-return without informing the caller;
start()then fails withRecvFailedinstead of a preciseFailedToInitializeWinRT.
Fix by sending a typed error to the starter channel and returning:
- if let Err(e) = unsafe { RoInitialize(RO_INIT_MULTITHREADED) }
- && e.code() != S_FALSE
- {
- return;
- // return Err(CreateRunnerError::FailedToInitializeWinRT);
- }
+ match unsafe { RoInitialize(RO_INIT_MULTITHREADED) } {
+ Ok(()) => {}
+ Err(_) => {
+ let _ = started_tx.send(Err(StartRunnerError::FailedToInitializeWinRT));
+ return;
+ }
+ }📝 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.
| if let Err(e) = unsafe { RoInitialize(RO_INIT_MULTITHREADED) } | |
| && e.code() != S_FALSE | |
| { | |
| return; | |
| // return Err(CreateRunnerError::FailedToInitializeWinRT); | |
| } | |
| match unsafe { RoInitialize(RO_INIT_MULTITHREADED) } { | |
| Ok(()) => {} | |
| Err(_) => { | |
| let _ = started_tx.send(Err(StartRunnerError::FailedToInitializeWinRT)); | |
| return; | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/scap-direct3d/src/lib.rs around lines 202–208, the RoInitialize check
is incorrect (invalid Rust syntax) and currently swallows the error by
early-returning; instead capture the Result from unsafe { RoInitialize(...) },
treat any Err as failure (no special-case for S_FALSE needed), send a
CreateRunnerError::FailedToInitializeWinRT (or the wrapped error) back through
the starter channel so the caller receives a typed error, and then return
immediately after sending; ensure you propagate or log the original error
details when constructing the CreateRunnerError before returning.
| #[derive(Clone)] | ||
| struct Runner { | ||
| _session: GraphicsCaptureSession, | ||
| _frame_pool: Direct3D11CaptureFramePool, | ||
| } |
There was a problem hiding this comment.
Keep the DispatcherQueueController alive for the thread’s lifetime
The dispatcher queue controller is dropped at the end of Runner::start, which can prevent WinRT events (FrameArrived/Closed) from firing on an MTA thread. Retain it as part of Runner.
@@
-struct Runner {
- _session: GraphicsCaptureSession,
- _frame_pool: Direct3D11CaptureFramePool,
-}
+struct Runner {
+ _session: GraphicsCaptureSession,
+ _frame_pool: Direct3D11CaptureFramePool,
+ _dispatcher: windows::Win32::System::WinRT::DispatcherQueueController,
+}
@@
- let _controller = unsafe { CreateDispatcherQueueController(queue_options) }
- .map_err(StartRunnerError::DispatchQueue)?;
+ let dispatcher = unsafe { CreateDispatcherQueueController(queue_options) }
+ .map_err(StartRunnerError::DispatchQueue)?;
@@
- Ok(Self {
- _session: session,
- _frame_pool: frame_pool,
- })
+ Ok(Self {
+ _session: session,
+ _frame_pool: frame_pool,
+ _dispatcher: dispatcher,
+ })Also add the missing import at the top alongside DispatcherQueueOptions:
- DispatcherQueueOptions, RO_INIT_MULTITHREADED, RoInitialize,
+ DispatcherQueueOptions, DispatcherQueueController, RO_INIT_MULTITHREADED, RoInitialize,Also applies to: 446-454, 605-608
🤖 Prompt for AI Agents
In crates/scap-direct3d/src/lib.rs around lines 432-436 (and also apply same
change at 446-454 and 605-608), the DispatcherQueueController created in
Runner::start is being dropped when the function exits which prevents WinRT
events from firing on the MTA thread; add a new field to Runner (e.g.,
dispatcher_queue_controller: DispatcherQueueController) and store the created
controller there so it lives for the Runner/thread lifetime, update any
constructors/initialization to populate this field, and keep the existing
_session and _frame_pool fields; also add the missing import for
DispatcherQueueController at the top alongside DispatcherQueueOptions.
| let frame_pool = Direct3D11CaptureFramePool::Create( | ||
| &direct3d_device, | ||
| PixelFormat::R8G8B8A8Unorm.as_directx(), | ||
| 1, | ||
| item.Size()?, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Honor Settings.pixel_format when creating the frame pool
The frame pool is hard-coded to R8G8B8A8UIntNormalized. Use the caller-provided settings.pixel_format for consistency with cropping and downstream conversion.
- let frame_pool = Direct3D11CaptureFramePool::Create(
- &direct3d_device,
- PixelFormat::R8G8B8A8Unorm.as_directx(),
- 1,
- item.Size()?,
- )
+ let frame_pool = Direct3D11CaptureFramePool::Create(
+ &direct3d_device,
+ settings.pixel_format.as_directx(),
+ 1,
+ item.Size()?,
+ )📝 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.
| let frame_pool = Direct3D11CaptureFramePool::Create( | |
| &direct3d_device, | |
| PixelFormat::R8G8B8A8Unorm.as_directx(), | |
| 1, | |
| item.Size()?, | |
| ) | |
| let frame_pool = Direct3D11CaptureFramePool::Create( | |
| &direct3d_device, | |
| settings.pixel_format.as_directx(), | |
| 1, | |
| item.Size()?, | |
| ) |
🤖 Prompt for AI Agents
In crates/scap-direct3d/src/lib.rs around lines 483 to 488 the
Direct3D11CaptureFramePool is being created with a hard-coded
PixelFormat::R8G8B8A8Unorm; replace that hard-coded format with the
caller-provided settings.pixel_format (calling .as_directx() if required) so the
frame pool honors Settings.pixel_format; ensure the expression types match the
API (convert or map the settings enum to the expected DirectX value and adjust
imports/signatures if necessary).
💡 Verification agent
🧩 Analysis chain
Consistency: use the configured pixel format everywhere
You already use settings.pixel_format.as_dxgi() for the crop texture. Align the frame pool creation (see prior comment) so the entire pipeline uses a single declared format.
You can quickly grep for hard-coded R8G8B8A8 usages to ensure there are no stragglers.
Also applies to: 510-525
🏁 Script executed:
#!/bin/bash
rg -nP -C2 'R8G8B8A8(UNORM|UIntNormalized)' crates/scap-direct3d/src/lib.rsLength of output: 515
🏁 Script executed:
#!/bin/bash
rg -nP -C2 'R8G8B8A8Unorm' crates/scap-direct3d/src/lib.rsLength of output: 694
🏁 Script executed:
#!/bin/bash
rg -nP -C2 'settings\.pixel_format' crates/scap-direct3d/src/lib.rsLength of output: 923
🏁 Script executed:
#!/bin/bash
sed -n '480,550p' crates/scap-direct3d/src/lib.rsLength of output: 2722
Ensure consistent use of settings.pixel_format throughout the capture pipeline
Please replace the two remaining hard-coded R8G8B8A8 formats with your configured pixel format:
• In crates/scap-direct3d/src/lib.rs at the frame-pool creation (around line 483), change
PixelFormat::R8G8B8A8Unorm.as_directx()to
settings.pixel_format.as_directx()• In the same file where you build the swap-chain/PSO texture desc (around line 318), change
Format: DXGI_FORMAT(DirectXPixelFormat::R8G8B8A8UIntNormalized.0),to
Format: settings.pixel_format.as_dxgi(),The crop-texture and final frame packaging already use settings.pixel_format, so no other changes are needed.
🤖 Prompt for AI Agents
In crates/scap-direct3d/src/lib.rs around lines 483–488 (frame-pool creation)
and around line 318 (swap-chain/PSO texture desc), two hard-coded R8G8B8A8
formats must be replaced with the configured pixel format: change the frame-pool
PixelFormat::R8G8B8A8Unorm.as_directx() call to use
settings.pixel_format.as_directx(), and change the DXGI Format field that
currently uses the R8G8B8A8 DXGI value to settings.pixel_format.as_dxgi();
ensure the correct helper methods (as_directx/as_dxgi) are called and that
settings is in scope for both locations so the entire capture pipeline
consistently uses settings.pixel_format.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
crates/displays/src/platform/win.rs (3)
136-140: Fix invalid out-parameter to GetDpiForMonitor (avoid &mut 0) — compile-time errorYou’re taking a mutable reference to a temporary literal for the Y DPI. Initialize a second mutable and pass both. Keep using the X DPI for scale.
- let dpi = unsafe { - let mut dpi_x = 0; - GetDpiForMonitor(self.0, MDT_EFFECTIVE_DPI, &mut dpi_x, &mut 0).ok()?; - dpi_x - }; + let dpi = unsafe { + let mut dpi_x: u32 = 0; + let mut dpi_y: u32 = 0; + GetDpiForMonitor(self.0, MDT_EFFECTIVE_DPI, &mut dpi_x, &mut dpi_y).ok()?; + dpi_x + };
1229-1234: Replace unstable &raw and unqualified size_of with stable, qualified formsUse addr_of_mut! (stable) and fully qualify size_of to avoid both stability and name-resolution issues.
- DwmGetWindowAttribute( - self.0, - DWMWA_EXTENDED_FRAME_BOUNDS, - (&raw mut rect).cast(), - size_of::<RECT>() as u32, - ) + DwmGetWindowAttribute( + self.0, + DWMWA_EXTENDED_FRAME_BOUNDS, + std::ptr::addr_of_mut!(rect).cast(), + mem::size_of::<RECT>() as u32, + )
1266-1271: Same fix here for physical_bounds: addr_of_mut! + mem::size_of::()Mirror the logical_size correction for this DwmGetWindowAttribute call.
- DwmGetWindowAttribute( - self.0, - DWMWA_EXTENDED_FRAME_BOUNDS, - (&raw mut rect).cast(), - size_of::<RECT>() as u32, - ) + DwmGetWindowAttribute( + self.0, + DWMWA_EXTENDED_FRAME_BOUNDS, + std::ptr::addr_of_mut!(rect).cast(), + mem::size_of::<RECT>() as u32, + )
🧹 Nitpick comments (8)
crates/displays/src/platform/win.rs (8)
1254-1264: Don’t gate physical_bounds on PROCESS_PER_MONITOR_DPI_AWAREPhysical (pixel) bounds from DWMWA_EXTENDED_FRAME_BOUNDS are DPI-agnostic. Returning None for processes that aren’t per-monitor DPI aware needlessly withholds valid data. Recommend logging and proceeding.
- match GetProcessDpiAwareness(None) { - Ok(PROCESS_PER_MONITOR_DPI_AWARE) => {} - Err(e) => { - error!("Failed to get process DPI awareness: {e}"); - return None; - } - Ok(v) => { - error!("Unsupported DPI awareness {v:?}"); - return None; - } - } + if let Err(e) = GetProcessDpiAwareness(None) { + error!("GetProcessDpiAwareness failed: {e} — continuing with physical bounds"); + }
1217-1227: Consider soft-failing DPI awareness for logical_size as wellIf DPI awareness can’t be queried or isn’t per-monitor, you can still compute a best-effort logical size using GetDpiForWindow with a 96 DPI fallback (which you already do below). Avoid returning None unless strictly required.
Would you like me to push a variant that soft-fails here and adds a telemetry counter so we can measure how often we’re in a non-per-monitor DPI context?
681-687: Tighten window enumeration to exclude child/tool windowsYou switched to EnumChildWindows(GetDesktopWindow()), which is fine for top-level enumeration. However, is_window_valid_for_enumeration doesn’t filter WS_CHILD/WS_EX_TOOLWINDOW, so “utility” and embedded child windows can leak into the list.
Apply the filter where we already have style queries (mirroring is_valid):
fn is_window_valid_for_enumeration(hwnd: HWND, current_process_id: u32) -> bool { unsafe { // Skip invisible or minimized windows if !IsWindowVisible(hwnd).as_bool() || IsIconic(hwnd).as_bool() { return false; } - // Skip own process windows + // Filter by style to avoid tool/child windows + let styles = GetWindowLongPtrW(hwnd, GWL_STYLE); + let ex_styles = GetWindowLongPtrW(hwnd, GWL_EXSTYLE); + if (ex_styles & isize::try_from(WS_EX_TOOLWINDOW.0).unwrap()) != 0 { + return false; + } + if (styles & isize::try_from(WS_CHILD.0).unwrap()) != 0 { + return false; + } + + // Skip own process windows let mut process_id = 0u32; GetWindowThreadProcessId(hwnd, Some(&mut process_id)); process_id != current_process_id } }To verify impact, I can provide a small diagnostic to log class names and styles for any enumerated window that would be filtered by this change.
Also applies to: 1392-1404
150-161: Minor: prefer is_invalid() over raw pointer checkFor consistency with other checks in this file, prefer
if !monitor.is_invalid()toif monitor.0 as usize != 0.- if monitor.0 as usize != 0 { + if !monitor.is_invalid() { Some(Self(monitor)) } else { None }
218-255: Improve monitor name resolution by falling back to DisplayConfig/EDID/RegistryEnumDisplayDevicesW’s DeviceString is often “Generic PnP Monitor”. You already have robust helpers below (displayconfig/edid/registry). Consider using them as fallbacks when DeviceString is empty/generic.
Example approach (pseudo):
- Try get_friendly_name_from_displayconfig()
- Else try get_friendly_name_from_edid()
- Else try get_friendly_name_from_registry()
- Else keep current DeviceString path
703-734: Comment accuracy: we’re moving down the Z-order, not “up”You call GetWindow with GW_HWNDNEXT which walks towards the background. Consider clarifying the comment to match behavior.
- // Walk up the Z-order chain to find the topmost valid window + // Walk down the Z-order (towards background) to find the next candidate
642-650: Consistency: construct PhysicalPosition via new()Keeps style consistent with other constructors in this module.
- Some(PhysicalPosition { - x: point.x as f64, - y: point.y as f64, - }) + Some(PhysicalPosition::new(point.x as f64, point.y as f64))
844-905: Optional: localize FileDescription lookupYou hard-code 040904B0 (en-US, Unicode). For better coverage, query VarFileInfo\Translation and try each (lang, codepage) pair dynamically, falling back to 040904B0.
I can wire this up with a small helper that builds the “\StringFileInfo\{lang}{codepage}\FileDescription” key list and tries them in order.
📜 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 (2)
crates/displays/src/main.rs(3 hunks)crates/displays/src/platform/win.rs(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/displays/src/main.rs
🧰 Additional context used
🧬 Code graph analysis (1)
crates/displays/src/platform/win.rs (4)
crates/displays/src/lib.rs (9)
logical_size(48-50)logical_size(130-132)physical_size(44-46)physical_size(126-128)name(40-42)name(150-152)display(146-148)id(28-30)id(118-120)crates/recording/src/sources/screen_capture/mod.rs (2)
physical_size(161-176)display(68-74)crates/displays/src/bounds.rs (8)
new(11-13)new(38-40)new(65-67)new(85-87)new(112-114)new(132-134)x(116-118)x(136-138)crates/cursor-capture/src/position.rs (5)
from_raw(36-58)x(143-145)x(168-170)display(60-62)display(176-178)
⏰ 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 (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (3)
crates/displays/src/platform/win.rs (3)
908-1006: Icon extraction path looks solidGood multi-strategy approach with size prioritization, proper DC/bitmap cleanup, and DI_NORMAL draw flags. The native-size prioritization and BGRA→RGBA fixups are correct.
Also applies to: 1068-1211
151-189: Display physical/logical helpers LGTMThe rcMonitor→PhysicalBounds mapping and the subsequent logical scaling (post-DPI fix) are straightforward and correct.
Also applies to: 165-181
1522-1543: pid_to_exe_path: good handle hygiene and bounds handlingUsing QueryFullProcessImageNameW with a sized buffer and converting with from_wide is correct. The extra is_invalid() warning is helpful.
| Storage::FileSystem::{GetFileVersionInfoSizeW, GetFileVersionInfoW, VerQueryValueW}, | ||
| System::{ |
There was a problem hiding this comment.
Wrong module for version-info APIs (GetFileVersionInfo)*
These functions live under Win32::System::Version, not Storage::FileSystem.
- Storage::FileSystem::{GetFileVersionInfoSizeW, GetFileVersionInfoW, VerQueryValueW},
+ System::Version::{GetFileVersionInfoSizeW, GetFileVersionInfoW, VerQueryValueW},📝 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.
| Storage::FileSystem::{GetFileVersionInfoSizeW, GetFileVersionInfoW, VerQueryValueW}, | |
| System::{ | |
| System::Version::{GetFileVersionInfoSizeW, GetFileVersionInfoW, VerQueryValueW}, | |
| System::{ |
🤖 Prompt for AI Agents
In crates/displays/src/platform/win.rs around lines 27-28, the
GetFileVersionInfoSizeW/GetFileVersionInfoW/VerQueryValueW functions are
imported from the wrong module (Storage::FileSystem); update the use/import to
reference Win32::System::Version instead, replacing
Storage::FileSystem::{GetFileVersionInfoSizeW, GetFileVersionInfoW,
VerQueryValueW} with the corresponding Win32::System::Version import so the
version-info APIs are pulled from the correct module.
| GetLayeredWindowAttributes, GetWindow, GetWindowLongPtrW, GetWindowLongW, | ||
| GetWindowRect, GetWindowTextLengthW, GetWindowTextW, GetWindowThreadProcessId, | ||
| HICON, ICONINFO, IsIconic, IsWindowVisible, SendMessageW, WM_GETICON, WS_CHILD, | ||
| WS_EX_LAYERED, WS_EX_TOOLWINDOW, WS_EX_TOPMOST, WS_EX_TRANSPARENT, WindowFromPoint, | ||
| }, | ||
| }, | ||
| }, | ||
| core::{BOOL, PCWSTR, PWSTR}, |
There was a problem hiding this comment.
Unify BOOL type to Win32::Foundation::BOOL for callbacks
You import windows::core::BOOL, but the Win32 callbacks expect windows::Win32::Foundation::BOOL. Mixing them can cause signature/type mismatches. Import the Foundation BOOL and drop the core BOOL.
- core::{BOOL, PCWSTR, PWSTR},
+ core::{PCWSTR, PWSTR},- Foundation::{CloseHandle, HWND, LPARAM, POINT, RECT, TRUE, WIN32_ERROR, WPARAM},
+ Foundation::{BOOL, CloseHandle, HWND, LPARAM, POINT, RECT, TRUE, WIN32_ERROR, WPARAM},No further code changes are needed if the callback signatures continue to use BOOL (they’ll now refer to the correct Foundation type).
Also applies to: 96-106, 748-757
🤖 Prompt for AI Agents
In crates/displays/src/platform/win.rs around lines 56 (and also apply same
change at 96-106 and 748-757), the file imports windows::core::BOOL which is
incompatible with Win32 callback signatures that expect
windows::Win32::Foundation::BOOL; replace the core BOOL import with
Win32::Foundation::BOOL (remove core::BOOL) so all callback signatures use the
Foundation BOOL type to avoid type/signature mismatches.
Replaces all usage of
scapwith a fewscap-*helper crates which are consumed inScreenCaptureSource, providing more direct access to native APIs and opening the door to better encoding performance.cap_failpoints and test themSummary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Chores