feat: 0.4 windows fixes + optimisations #1453
Conversation
|
Warning Rate limit exceeded@richiemcilroy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughAdds Windows Media Foundation and D3D integration, segmented Windows muxers, CPU/SIMD YUV converters, Windows-targeted D3D texture interop, WebGPU resilience and direct-canvas fast-path, adapter hardware→software fallback flag propagation, many texture-format changes (sRGB→linear), OS-specific in-progress window behavior, and several JS/TS UI/query tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application (UI)
participant Socket as SocketWorker
participant WS as WebSocket
participant Canvas as HTMLCanvasElement
participant Renderer as WebGPU/Canvas2D
App->>Socket: initDirectCanvas(canvas)
Socket->>Socket: store directCanvas & directCtx
WS->>Socket: incoming frame bytes
Socket->>Socket: if directCanvas set decode trailing meta (stride/size)
Socket->>Canvas: putImageData(reconstructed ImageData)
Socket->>App: postRenderSignal()
sequenceDiagram
participant Segment as WindowsSegmentedMuxer
participant Producer as CapturePipeline
participant Encoder as EncoderThread
participant MF as MediaFoundation/Encoder
participant FS as Filesystem/Manifest
Producer->>Segment: send_video_frame(frame, timestamp)
Segment->>Segment: PauseTracker adjusts timestamps
Segment->>Encoder: queue frame for encode
Encoder->>MF: encode frame
Encoder->>FS: write segment file (fragment.mp4)
Segment->>Segment: if segment_duration elapsed rotate_segment()
Segment->>Encoder: finalize current encoder (flush & close)
Segment->>FS: write/update manifest.json
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (25)
crates/scap-direct3d/src/lib.rs (2)
387-392: Consider logging ignored errors during cleanup.The stop sequence intentionally ignores errors from
RemoveFrameArrivedandsession.Close()to ensure frame pool cleanup always runs. While this pattern is acceptable for cleanup code, the ignored errors could be logged for diagnostics.Optional improvement:
pub fn stop(&mut self) -> windows::core::Result<()> { if self.stop_flag.swap(true, Ordering::SeqCst) { return Ok(()); } - let _ = self.frame_pool.RemoveFrameArrived(self.frame_arrived_token); - let _ = self.session.Close(); + if let Err(e) = self.frame_pool.RemoveFrameArrived(self.frame_arrived_token) { + tracing::debug!("Failed to remove frame handler: {}", e); + } + if let Err(e) = self.session.Close() { + tracing::debug!("Failed to close session: {}", e); + } self.frame_pool.Close() }
278-278: Consider stronger memory ordering for stop flag read.The
stop()method usesOrdering::SeqCstfor the write, but the handler usesOrdering::Relaxedfor the read. For better synchronization guarantees, the load should use at leastOrdering::Acquireto pair with the release semantics.-if stop_flag.load(Ordering::Relaxed) { +if stop_flag.load(Ordering::Acquire) { return Ok(()); }Also applies to: 387-387
crates/recording/src/output_pipeline/win_segmented_camera.rs (3)
48-95: Consider extractingPauseTrackerto a shared module.This
PauseTrackerimplementation is duplicated inwin.rs,win_segmented.rs, and this file. Consider moving it to a shared Windows utilities module to reduce duplication.
540-552: Duplicateduration_to_timespanhelper.This function is duplicated across
win.rs,win_segmented.rs, and this file. Consider consolidating into a shared Windows utilities module alongsidePauseTracker.
501-508: Redundantsegment_start_timecheck.Lines 506-508 appear unreachable after the block at lines 501-504. If
current_state.is_none(),segment_start_timeis already set at line 502 before creating the segment.if self.current_state.is_none() { self.segment_start_time = Some(adjusted_timestamp); self.create_segment(&frame)?; } - - if self.segment_start_time.is_none() { - self.segment_start_time = Some(adjusted_timestamp); - }crates/recording/src/output_pipeline/win_segmented.rs (5)
188-205: Consider logging encoder thread panics instead of silently discarding them.The
handle.join()result is discarded, which means any panic in the encoder thread is silently lost. This could hide important errors.if handle.is_finished() { - let _ = handle.join(); + if let Err(panic_payload) = handle.join() { + error!("Screen encoder thread panicked: {:?}", panic_payload); + } break; }
247-252: Manifest write failures are silently ignored.If writing the manifest fails (e.g., disk full, permissions), the error is discarded. For a recording application, this could impact recovery of fragmented recordings.
Consider at least logging the error:
let manifest_path = self.base_path.join("manifest.json"); - let _ = std::fs::write( + if let Err(e) = std::fs::write( manifest_path, serde_json::to_string_pretty(&manifest).unwrap_or_default(), - ); + ) { + warn!("Failed to write manifest: {e}"); + }
450-458: Mutex unwrap could propagate panic if output mutex is poisoned.Using
unwrap()on the mutex lock means if any thread holding this lock panics, all subsequent frames will cause panics. Additionally, thewrite_sampleerror is discarded after formatting.- let mut output = output_clone.lock().unwrap(); - - let _ = muxer - .write_sample(&output_sample, &mut output) - .map_err(|e| format!("WriteSample: {e}")); + let mut output = match output_clone.lock() { + Ok(guard) => guard, + Err(poisoned) => { + error!("Output mutex poisoned: {poisoned}"); + return Err(anyhow!("Output mutex poisoned")); + } + }; + + if let Err(e) = muxer.write_sample(&output_sample, &mut output) { + warn!("WriteSample failed: {e}"); + }
489-489: Same mutex unwrap issue on main thread.This
unwrap()on the output mutex could panic if the encoder thread panicked while holding the lock.- output.lock().unwrap().write_header()?; + output + .lock() + .map_err(|_| anyhow!("Failed to lock output for header write"))? + .write_header()?;
566-573: Potentially redundantsegment_start_timeassignment.The assignment at lines 571-573 appears unreachable. When
current_state.is_none()(line 566),segment_start_timeis set at line 567 beforecreate_segment(). Thecreate_segment()method doesn't resetsegment_start_timetoNone, so the check at line 571 would never be true after line 567 executes.If this is intentional defensive coding, consider removing the redundant block:
if self.current_state.is_none() { self.segment_start_time = Some(adjusted_timestamp); self.create_segment()?; } - if self.segment_start_time.is_none() { - self.segment_start_time = Some(adjusted_timestamp); - }crates/video-decode/src/media_foundation.rs (2)
505-523:get_shared_handleappears to be dead code.This function is defined but never called. The
YuvPlaneTexturesstruct always returnsNonefory_handleanduv_handle(lines 629-631), andcopy_texture_subresourcealso returnsNonefor the shared handle (line 672).Is this intentional for a future implementation, or should shared handles be created? If unused, consider removing to avoid confusion.
#!/bin/bash # Check if get_shared_handle is used anywhere in the codebase rg -n "get_shared_handle" --type=rust
362-375: Consider adding software driver fallback.The PR description mentions "Windows GPU adapter fallback (hardware→software)", but this implementation only attempts
D3D_DRIVER_TYPE_HARDWARE. If hardware acceleration fails (e.g., on VMs or systems with incompatible GPUs), the decoder will be unusable.Consider adding a fallback to
D3D_DRIVER_TYPE_WARP(software rasterizer):unsafe { - D3D11CreateDevice( + let result = D3D11CreateDevice( None, D3D_DRIVER_TYPE_HARDWARE, HMODULE::default(), flags, Some(&feature_levels), D3D11_SDK_VERSION, Some(&mut device), None, Some(&mut context), - ) - .map_err(|e| format!("D3D11CreateDevice failed: {e:?}"))?; + ); + + if result.is_err() { + tracing::warn!("Hardware D3D11 device creation failed, falling back to WARP"); + D3D11CreateDevice( + None, + windows::Win32::Graphics::Direct3D::D3D_DRIVER_TYPE_WARP, + HMODULE::default(), + flags, + Some(&feature_levels), + D3D11_SDK_VERSION, + Some(&mut device), + None, + Some(&mut context), + ) + .map_err(|e| format!("D3D11CreateDevice (WARP fallback) failed: {e:?}"))?; + } }apps/desktop/src/utils/socket.ts (1)
111-144: Direct canvas state should be cleaned up.The
directCanvasanddirectCtxvariables are initialized but not reset in thecleanup()function (lines 114-131). This could cause issues if the WebSocket reconnects or if there are references preventing garbage collection.Add cleanup for direct canvas state:
function cleanup() { if (isCleanedUp) return; isCleanedUp = true; if (producer) { producer.signalShutdown(); producer = null; } worker.onmessage = null; worker.terminate(); pendingFrame = null; nextFrame = null; isProcessing = false; + directCanvas = null; + directCtx = null; setIsConnected(false); }crates/rendering/src/cpu_yuv.rs (1)
287-288: Potential unaligned memory access.The
*(u_ptr as *const i32)and*(v_ptr as *const i32)dereferences assume 4-byte alignment. If the UV plane pointers aren't aligned, this could cause undefined behavior on some platforms (though x86 generally tolerates unaligned access with a performance penalty).Consider using
read_unalignedfor safety:- let u4 = _mm_set_epi32(0, 0, 0, *(u_ptr as *const i32)); - let v4 = _mm_set_epi32(0, 0, 0, *(v_ptr as *const i32)); + let u4 = _mm_set_epi32(0, 0, 0, std::ptr::read_unaligned(u_ptr as *const i32)); + let v4 = _mm_set_epi32(0, 0, 0, std::ptr::read_unaligned(v_ptr as *const i32));apps/desktop/src/routes/in-progress-recording.tsx (1)
64-65: Debug logging statements left in production code.These
console.logstatements appear to be debugging artifacts. Consider removing them before merging, or converting to a conditional debug flag if they're needed for troubleshooting.export default function () { - console.log("[in-progress-recording] Wrapper rendering"); - document.documentElement.setAttribute("data-transparent-window", "true"); document.body.style.background = "transparent"; return <InProgressRecordingInner />; } function InProgressRecordingInner() { - console.log("[in-progress-recording] Inner component rendering"); - const [state, setState] = createSignal<State>(Also applies to: 73-74
apps/desktop/src-tauri/src/windows.rs (1)
744-766: Minor duplication between macOS and Windows blocks.The two blocks differ only in
skip_taskbar. You could extract the common builder chain and apply the OS-specific setting afterward, but the current approach is explicit and acceptable for maintainability. No action required unless more divergence is expected.apps/desktop/src-tauri/src/lib.rs (1)
841-898: get_current_recording logging and optional window bounds look correctThe new logging around
get_current_recordingand the early return forRecordingState::Nonemake the flow clearer, and resolving window bounds viascap_targets::Window::from_id(id)with anOption<LogicalBounds>cleanly handles failures. If the UI polls this command at a high frequency, consider keeping these attracein production configs to avoid log noise, but functionally this looks solid.crates/rendering/src/layers/display.rs (2)
66-90: Per-frame trace logging in prepare is useful but potentially verboseThe new
tracing::trace!with format, dimensions, and data length is great for debugging decode/render issues. If this function is hit every frame, ensure your default tracing filter in production does not enabletracefor this target to avoid overhead.
117-426: NV12 handling across platforms looks robust; consider one extra macOS fallbackThe refactored NV12 branch:
- macOS: prefers
iosurface→convert_nv12_from_iosurface, then falls back to planarconvert_nv12/convert_nv12_cpu(depending onprefer_cpu_conversion).- Windows: prefers
convert_nv12_from_d3d11_shared_handlesand falls back to planar CPU/GPU conversion with detailed debug logs.- Other platforms: uses planar CPU vs GPU conversion based on
prefer_cpu_conversion.Overall this is a solid separation of zero-copy and CPU/GPU paths with good diagnostics. One minor edge case: on macOS, when
iosurface_resultisSome(Ok(_))butoutput_texture()isNone, you currently just returnfalsewithout attempting the planar fallback. It might be safer to treat that as a failure case and reuse the same planar fallback path used wheniosurface_resultisNone/Err.apps/desktop/src/utils/frame-worker.ts (1)
340-425: WebGPU frame buffering and drop tracking behave as intended; minor consideration for lastRawFrameDataIn
processFrame, the WebGPU path:
- Drops any previously queued but unrendered frame, tracking drops per second with a throttled console warning.
- Reuses
webgpuFrameBufferto hold the latest frame data and enqueues that as the sole pending frame.This ensures there is never a backlog and avoids per-frame allocations, which is ideal for a real-time stream. Note that
lastRawFrameDatais only updated when not in WebGPU mode, so during a long WebGPU-only run a later re-init will fall back to the last pre-WebGPU frame rather than the latest frame; if you care about reusing the true latest frame across WebGPU reinitializations, you could also updatelastRawFrameDatafromprocessedFrameDatainside the WebGPU branch.Also applies to: 427-433
apps/desktop/src-tauri/src/screenshot_editor.rs (1)
235-267: Shared GPU reuse and is_software_adapter propagation are consistent; local fallback could mirror software pathReusing the shared GPU context (including
is_software_adapter) when available keeps screenshot editor rendering aligned with the rest of the app. In the local fallback branch you currently request only a high-performance adapter withforce_fallback_adapter: false; ifget_shared_gpu()ever returnsNoneon a system that only has a software adapter, you’ll hit"No GPU adapter found". Consider optionally mirroring the hardware→software fallback pattern fromgpu_context::get_shared_gpuhere for maximum robustness.crates/rendering/src/lib.rs (2)
373-459: RenderVideoConstants adapter selection and is_software_adapter flag look correctThe new
RenderVideoConstants::new:
- Attempts a high-performance adapter first, logging adapter name/backend on success.
- Falls back to a low-power adapter with
force_fallback_adapter: true, logging that it’s using a software adapter and surfacingRenderingError::NoAdapterif that also fails.- Records
is_software_adapteron the constants struct.This is a sensible hardware→software fallback with useful tracing. If you care about distinguishing “no adapter found” vs “request_device failed” in logs, you might also want to log the actual error from the fallback
request_adapterfailure rather than mapping it straight toNoAdapter.
1782-1798: Screen and cursor rendering now gated on should_render_screenUsing
uniforms.scene.should_render_screen()to gate both the display and cursor passes reduces unnecessary work when the screen layer is fully invisible/blurred out, and the trace log will help debug unexpected blank frames. Just confirm that you never rely on a “cursor-only over background” state; in that case, this gating would suppress the cursor as well.crates/rendering/src/decoder/mod.rs (1)
446-475: Windows decoder fallback logic is correct, though duplicates timeout handling.The fallback from Media Foundation to FFmpeg correctly creates new channels and returns early, avoiding the orphaned
ready_rxfrom the MF path. However, the timeout logic at lines 463-472 duplicates lines 483-490.Consider extracting the timeout wrapper into a helper to reduce duplication (optional for this PR).
crates/rendering/src/d3d_texture.rs (1)
188-233: Stub implementations will cause runtime failures in dependent code.These functions always return errors, which is fine as placeholders. However,
convert_nv12_from_d3d11_shared_handlesinyuv_converter.rs(lines 766-837) callsimport_d3d11_texture_to_wgpuand will always fail at runtime. Consider either:
- Marking the calling function as
#[allow(dead_code)]or removing it temporarily- Adding a compile-time feature flag to exclude the unusable code path
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
apps/desktop/src-tauri/src/editor_window.rs(2 hunks)apps/desktop/src-tauri/src/gpu_context.rs(2 hunks)apps/desktop/src-tauri/src/lib.rs(2 hunks)apps/desktop/src-tauri/src/screenshot_editor.rs(4 hunks)apps/desktop/src-tauri/src/windows.rs(1 hunks)apps/desktop/src/App.tsx(0 hunks)apps/desktop/src/routes/editor/Player.tsx(1 hunks)apps/desktop/src/routes/in-progress-recording.tsx(4 hunks)apps/desktop/src/utils/frame-worker.ts(4 hunks)apps/desktop/src/utils/queries.ts(1 hunks)apps/desktop/src/utils/socket.ts(4 hunks)apps/desktop/src/utils/tauri.ts(1 hunks)crates/editor/src/editor.rs(1 hunks)crates/mediafoundation-utils/src/lib.rs(2 hunks)crates/recording/src/capture_pipeline.rs(1 hunks)crates/recording/src/feeds/camera.rs(5 hunks)crates/recording/src/output_pipeline/mod.rs(1 hunks)crates/recording/src/output_pipeline/win.rs(1 hunks)crates/recording/src/output_pipeline/win_segmented.rs(1 hunks)crates/recording/src/output_pipeline/win_segmented_camera.rs(1 hunks)crates/recording/src/studio_recording.rs(2 hunks)crates/rendering/Cargo.toml(1 hunks)crates/rendering/src/composite_frame.rs(1 hunks)crates/rendering/src/cpu_yuv.rs(1 hunks)crates/rendering/src/d3d_texture.rs(1 hunks)crates/rendering/src/decoder/media_foundation.rs(1 hunks)crates/rendering/src/decoder/mod.rs(8 hunks)crates/rendering/src/frame_pipeline.rs(2 hunks)crates/rendering/src/layers/background.rs(2 hunks)crates/rendering/src/layers/blur.rs(1 hunks)crates/rendering/src/layers/camera.rs(1 hunks)crates/rendering/src/layers/captions.rs(2 hunks)crates/rendering/src/layers/cursor.rs(2 hunks)crates/rendering/src/layers/display.rs(10 hunks)crates/rendering/src/layers/mask.rs(1 hunks)crates/rendering/src/layers/text.rs(1 hunks)crates/rendering/src/lib.rs(8 hunks)crates/rendering/src/yuv_converter.rs(17 hunks)crates/scap-direct3d/src/lib.rs(4 hunks)crates/video-decode/Cargo.toml(1 hunks)crates/video-decode/src/lib.rs(1 hunks)crates/video-decode/src/media_foundation.rs(1 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop/src/App.tsx
🧰 Additional context used
📓 Path-based instructions (7)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Use duration.saturating_sub(other) instead of duration - other to avoid panics on underflow
Merge nested if statements: write 'if a && b { }' instead of 'if a { if b { } }'
Don't call .clone() on Copy types (integers, bools, etc.); copy them directly
Use function references directly: iter.map(foo) instead of iter.map(|x| foo(x))
Accept &[T] or &str instead of &Vec or &String in function parameters for flexibility
Use .is_empty() instead of .len() == 0 or .len() > 0 / .len() != 0
Don't assign () to a variable: write foo(); instead of let _ = foo(); or let x = foo(); when return is unit
Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Use 'for item in &collection' or 'for (i, item) in collection.iter().enumerate()' instead of 'for i in 0..collection.len()'
Use value.clamp(min, max) instead of manual if chains or .min(max).max(min) patterns
Always handle Result/Option or types marked #[must_use]; never ignore them
**/*.rs: Userustfmtand workspace clippy lints for Rust code formatting and linting
Use snake_case for Rust module names and kebab-case for crate names
Never usedbg!()macro in Rust code; use proper logging instead (Clippy:dbg_macro= deny)
Always handleResult/Optionor types marked#[must_use]; never ignore them (Rust compiler lint:unused_must_use= deny)
Never writelet _ = async_fn()which silently drops futures; await or explicitly handle them (Clippy:let_underscore_future= deny)
Usesaturating_subinstead of-forDurationto avoid panics (Clippy:unchecked_duration_subtraction= deny)
Merge nestedifstatements: useif a && b { }instead ofif a { if b { } }(Clippy:collapsible_if= deny)
Don't call.clone()onCopytypes; just copy them directly (Clippy:clone_on_copy= deny)
U...
Files:
crates/rendering/src/layers/cursor.rsapps/desktop/src-tauri/src/editor_window.rscrates/recording/src/output_pipeline/win.rscrates/video-decode/src/lib.rsapps/desktop/src-tauri/src/gpu_context.rscrates/rendering/src/layers/mask.rscrates/rendering/src/layers/background.rscrates/rendering/src/layers/camera.rscrates/recording/src/output_pipeline/mod.rscrates/rendering/src/composite_frame.rscrates/recording/src/studio_recording.rscrates/rendering/src/decoder/media_foundation.rscrates/scap-direct3d/src/lib.rscrates/mediafoundation-utils/src/lib.rscrates/rendering/src/layers/blur.rscrates/rendering/src/layers/display.rsapps/desktop/src-tauri/src/lib.rscrates/rendering/src/frame_pipeline.rscrates/editor/src/editor.rscrates/recording/src/capture_pipeline.rscrates/rendering/src/layers/text.rscrates/rendering/src/layers/captions.rscrates/recording/src/feeds/camera.rsapps/desktop/src-tauri/src/windows.rsapps/desktop/src-tauri/src/screenshot_editor.rscrates/recording/src/output_pipeline/win_segmented.rscrates/video-decode/src/media_foundation.rscrates/rendering/src/lib.rscrates/rendering/src/decoder/mod.rscrates/recording/src/output_pipeline/win_segmented_camera.rscrates/rendering/src/yuv_converter.rscrates/rendering/src/cpu_yuv.rscrates/rendering/src/d3d_texture.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Never add comments to code (
//,/* */,///,//!,#, etc.); code must be self-explanatory through naming, types, and structure
Files:
crates/rendering/src/layers/cursor.rsapps/desktop/src-tauri/src/editor_window.rscrates/recording/src/output_pipeline/win.rscrates/video-decode/src/lib.rsapps/desktop/src-tauri/src/gpu_context.rscrates/rendering/src/layers/mask.rscrates/rendering/src/layers/background.rsapps/desktop/src/utils/frame-worker.tscrates/rendering/src/layers/camera.rscrates/recording/src/output_pipeline/mod.rscrates/rendering/src/composite_frame.rsapps/desktop/src/utils/queries.tscrates/recording/src/studio_recording.rscrates/rendering/src/decoder/media_foundation.rscrates/scap-direct3d/src/lib.rscrates/mediafoundation-utils/src/lib.rscrates/rendering/src/layers/blur.rscrates/rendering/src/layers/display.rsapps/desktop/src-tauri/src/lib.rscrates/rendering/src/frame_pipeline.rsapps/desktop/src/routes/editor/Player.tsxcrates/editor/src/editor.rscrates/recording/src/capture_pipeline.rsapps/desktop/src/utils/tauri.tscrates/rendering/src/layers/text.rsapps/desktop/src/utils/socket.tscrates/rendering/src/layers/captions.rscrates/recording/src/feeds/camera.rsapps/desktop/src-tauri/src/windows.rsapps/desktop/src-tauri/src/screenshot_editor.rscrates/recording/src/output_pipeline/win_segmented.rscrates/video-decode/src/media_foundation.rsapps/desktop/src/routes/in-progress-recording.tsxcrates/rendering/src/lib.rscrates/rendering/src/decoder/mod.rscrates/recording/src/output_pipeline/win_segmented_camera.rscrates/rendering/src/yuv_converter.rscrates/rendering/src/cpu_yuv.rscrates/rendering/src/d3d_texture.rs
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use TanStack Query v5 for all client-side server state and data fetching in TypeScript files
Files:
apps/desktop/src/utils/frame-worker.tsapps/desktop/src/utils/queries.tsapps/desktop/src/utils/tauri.tsapps/desktop/src/utils/socket.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use strict TypeScript; avoid any type; leverage shared types from @cap/* packages
Follow camelCase naming for variables and functions; PascalCase for components; hooks must start with 'use' prefix
Use Biome for linting and formatting; match existing formatting conventions in the codebase
Use Tailwind CSS for styling in web components; stay consistent with spacing and tokens
Use static skeletons for loading states that mirror content; avoid bouncing animations
Memoize expensive work, code-split naturally, and use Next/Image for remote assets
**/*.{ts,tsx,js,jsx}: Use 2-space indent for TypeScript files; format with Biome usingpnpm format
Use Biome for code formatting and linting; runpnpm formatregularly
Use kebab-case for file names (e.g.,user-menu.tsx); use PascalCase for components
Files:
apps/desktop/src/utils/frame-worker.tsapps/desktop/src/utils/queries.tsapps/desktop/src/routes/editor/Player.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src/utils/socket.tsapps/desktop/src/routes/in-progress-recording.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never add any form of comments to code (single-line //, multi-line /* /, JSDoc /* */, or any other comment syntax); code must be self-explanatory through naming, types, and structure
Directory naming must use lowercase-dashed convention
Files:
apps/desktop/src/utils/frame-worker.tsapps/desktop/src/utils/queries.tsapps/desktop/src/routes/editor/Player.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src/utils/socket.tsapps/desktop/src/routes/in-progress-recording.tsx
apps/desktop/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/**/*.ts: Use @tanstack/solid-query for server state management in SolidJS components
Use generated commands and events from tauri_specta for IPC; never manually construct IPC calls
Listen directly to generated events from tauri_specta and use typed event interfaces
Files:
apps/desktop/src/utils/frame-worker.tsapps/desktop/src/utils/queries.tsapps/desktop/src/utils/tauri.tsapps/desktop/src/utils/socket.ts
**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Use React Query hooks with Server Actions for mutations and perform precise cache updates using setQueryData/setQueriesData instead of broad invalidations
Files:
apps/desktop/src/routes/editor/Player.tsxapps/desktop/src/routes/in-progress-recording.tsx
🧠 Learnings (5)
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/video-decode/src/lib.rscrates/recording/src/studio_recording.rscrates/rendering/src/decoder/media_foundation.rscrates/recording/src/capture_pipeline.rscrates/recording/src/feeds/camera.rscrates/recording/src/output_pipeline/win_segmented.rscrates/rendering/src/decoder/mod.rscrates/recording/src/output_pipeline/win_segmented_camera.rscrates/rendering/src/yuv_converter.rs
📚 Learning: 2025-12-07T14:29:19.180Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T14:29:19.180Z
Learning: Applies to **/*.tsx : Use React Query hooks with Server Actions for mutations and perform precise cache updates using setQueryData/setQueriesData instead of broad invalidations
Applied to files:
apps/desktop/src/utils/queries.ts
📚 Learning: 2025-12-07T14:29:19.180Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T14:29:19.180Z
Learning: Applies to **/*.rs : Never use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Applied to files:
crates/recording/src/feeds/camera.rs
📚 Learning: 2025-10-17T05:58:22.586Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1219
File: crates/enc-avfoundation/src/mp4.rs:350-373
Timestamp: 2025-10-17T05:58:22.586Z
Learning: In crates/enc-avfoundation/src/mp4.rs, the `finish()` method intentionally skips video extension when `is_paused` is true. This is correct behavior because if recording is paused, the video should not be extended beyond the pause point—the pause is user-initiated, unlike the case where ScreenCaptureKit stops providing frames during static content.
Applied to files:
crates/recording/src/output_pipeline/win_segmented.rscrates/recording/src/output_pipeline/win_segmented_camera.rs
📚 Learning: 2025-12-07T14:29:19.180Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T14:29:19.180Z
Learning: Applies to apps/desktop/**/*.ts : Use tanstack/solid-query for server state management in SolidJS components
Applied to files:
apps/desktop/src/routes/in-progress-recording.tsx
🧬 Code graph analysis (14)
apps/desktop/src-tauri/src/gpu_context.rs (1)
crates/rendering/src/lib.rs (7)
new(116-186)new(386-459)new(501-503)new(1091-1566)new(1583-1588)new(1636-1638)default(731-739)
apps/desktop/src/utils/frame-worker.ts (1)
apps/desktop/src/utils/webgpu-renderer.ts (2)
isWebGPUSupported(49-59)initWebGPU(61-149)
crates/rendering/src/decoder/media_foundation.rs (2)
crates/rendering/src/decoder/mod.rs (9)
new_nv12(156-169)width(306-308)height(310-312)new(33-35)new(57-64)new(141-154)data(302-304)y_stride(380-382)uv_stride(384-386)crates/video-decode/src/media_foundation.rs (4)
width(113-115)height(117-119)new(69-71)None(515-515)
crates/editor/src/editor.rs (2)
crates/rendering/src/layers/display.rs (1)
new_with_options(33-64)crates/rendering/src/lib.rs (1)
new_with_options(1640-1656)
crates/rendering/src/layers/text.rs (1)
crates/rendering/src/layers/captions.rs (1)
new(256-375)
apps/desktop/src/utils/socket.ts (3)
crates/rendering/src/decoder/mod.rs (3)
data(302-304)height(310-312)width(306-308)apps/desktop/src/routes/editor/context.ts (1)
meta(670-672)crates/editor/src/editor_instance.rs (1)
meta(183-185)
crates/recording/src/feeds/camera.rs (1)
crates/mediafoundation-utils/src/lib.rs (2)
lock(24-24)lock(29-41)
apps/desktop/src-tauri/src/windows.rs (2)
apps/desktop/src-tauri/src/fake_window.rs (2)
app(52-52)spawn_fake_window_listener(48-106)apps/desktop/src-tauri/src/platform/macos/mod.rs (1)
set_window_level(18-27)
apps/desktop/src-tauri/src/screenshot_editor.rs (2)
apps/desktop/src-tauri/src/gpu_context.rs (1)
get_shared_gpu(50-108)crates/rendering/src/layers/display.rs (2)
new(29-31)new_with_options(33-64)
apps/desktop/src/routes/in-progress-recording.tsx (1)
apps/desktop/src/store.ts (1)
authStore(59-59)
crates/rendering/src/lib.rs (3)
crates/rendering/src/layers/display.rs (2)
new_with_options(33-64)new(29-31)crates/rendering/src/layers/cursor.rs (2)
new(48-156)new(187-198)crates/rendering/src/scene.rs (1)
should_render_screen(302-304)
crates/rendering/src/decoder/mod.rs (4)
crates/rendering/src/decoder/avassetreader.rs (4)
new(76-78)new(190-212)new(225-230)spawn(236-246)crates/scap-direct3d/src/lib.rs (1)
texture(438-440)crates/rendering/src/decoder/ffmpeg.rs (1)
spawn(134-360)crates/rendering/src/decoder/media_foundation.rs (1)
spawn(47-250)
crates/recording/src/output_pipeline/win_segmented_camera.rs (2)
crates/recording/src/output_pipeline/win.rs (5)
upload_mf_buffer_to_texture(724-778)new(33-39)adjust(41-72)timestamp(409-411)duration_to_timespan(381-393)crates/recording/src/output_pipeline/win_segmented.rs (3)
new(58-64)adjust(66-97)duration_to_timespan(608-620)
crates/rendering/src/d3d_texture.rs (1)
crates/scap-direct3d/src/lib.rs (1)
texture(438-440)
⏰ 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 (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (javascript-typescript)
| pass.set_pipeline(&self.pipeline.render_pipeline); | ||
| pass.set_bind_group(0, bind_group, &[]); | ||
| pass.draw(0..4, 0..1); | ||
| pass.draw(0..3, 0..1); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all pass.draw calls in rendering layers
rg -n 'pass\.draw' crates/rendering/src/layers/ | head -50Repository: CapSoftware/Cap
Length of output: 586
🏁 Script executed:
#!/bin/bash
# Check all draw calls in rendering layers for vertex counts
echo "=== Checking all pass.draw calls in rendering layers ==="
rg -n 'pass\.draw\(' crates/rendering/src/layers/ -A 0 -B 0
echo ""
echo "=== Checking specific files mentioned in review ==="
echo "Camera layer (around line 216):"
rg -n -C 3 'pass\.draw' crates/rendering/src/layers/camera.rs
echo ""
echo "Blur layer (around line 74):"
rg -n -C 3 'pass\.draw' crates/rendering/src/layers/blur.rsRepository: CapSoftware/Cap
Length of output: 1145
Inconsistency confirmed: camera.rs uses fullscreen triangle optimization while blur.rs does not.
The layer now draws 3 vertices (single triangle optimization) but blur.rs (line 74), cursor.rs (line 496), and background.rs (line 264) still draw 4 vertices. While mask.rs and display.rs also use the 3-vertex approach, this mixed pattern should be standardized. Either update blur.rs and similar layers to the fullscreen triangle optimization for consistency, or document why this layer differs from the others.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (8)
apps/desktop/src-tauri/src/windows.rs (1)
744-766: Simplify redundant window assignment.The window is built and assigned to a variable, then immediately returned without any intervening operations. This can be simplified.
Apply this diff to simplify:
#[cfg(windows)] let window = { - let window = self + self .window_builder(app, "/in-progress-recording") .maximized(false) .resizable(false) .fullscreen(false) .shadow(false) .always_on_top(true) .transparent(true) .visible_on_all_workspaces(true) .content_protected(should_protect) .inner_size(width, height) .position(pos_x, pos_y) .skip_taskbar(false) .initialization_script(format!( "window.COUNTDOWN = {};", countdown.unwrap_or_default() )) - .build()?; - - window + .build()? };crates/recording/src/output_pipeline/win_segmented_camera.rs (1)
425-425: Unwrap on mutex lock could panic.Using
.unwrap()on the mutex lock (line 425) will panic if the mutex is poisoned. Consider handling this more gracefully with proper error propagation.Apply this diff:
- output.lock().unwrap().write_header()?; + output + .lock() + .map_err(|e| anyhow!("Failed to lock output mutex: {e}"))? + .write_header()?;crates/rendering/src/layers/display.rs (3)
153-213: Consider simplifying the nested conditional logic.The nested structure
if !prefer_cpu_conversion { if iosurface { } else if planes { } }handles multiple paths correctly but is somewhat complex. If possible, consider early returns or extracting helper methods to flatten the logic, though the current structure is acceptable given the platform-specific requirements.
422-422: Consider adding error logging for consistency.Unlike the macOS and Windows paths, the error case here (line 422) silently returns
falsewithout logging. For consistency and debuggability, consider adding a debug log similar to the other platforms.
428-480: Consider adding error logging for YUV420P conversions.Similar to the NV12 path on non-macOS/Windows platforms, the YUV420P error case (line 475) silently returns
false. Adding debug logging would improve consistency and debuggability across all conversion paths.crates/video-decode/src/media_foundation.rs (3)
294-303: Consider logging YUV plane creation errors before dropping.The
.ok()on line 302 silently converts theResulttoOption, discarding error information. If YUV plane texture creation fails, debugging will be difficult without any indication of what went wrong.Consider logging the error:
let yuv_planes = unsafe { create_yuv_plane_textures( &self.d3d11_device, &self.d3d11_context, &output_texture, self.width, self.height, ) - .ok() + .map_err(|e| { + tracing::warn!("Failed to create YUV plane textures: {}", e); + e + }) + .ok() };
187-187: Consider usingexpectfor clearer invariant documentation.The
unwrap()is safe becausestaging_textureis always initialized in the block above (line 182), but usingexpectwould document this invariant more clearly.Apply this diff:
- let staging = self.staging_texture.as_ref().unwrap(); + let staging = self + .staging_texture + .as_ref() + .expect("staging texture must be initialized above");
79-87: Note: Per-instance COM initialization pattern.Each
MediaFoundationDecoderinstance callsCoInitializeExandMFStartupin its constructor andCoUninitialize/MFShutdownin its destructor. While COM's reference counting makes this safe, the typical pattern is to initialize COM once per thread rather than per-object. This works correctly but may add overhead if multiple decoder instances are created on the same thread.Also applies to: 351-358
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/desktop/src-tauri/src/windows.rs(1 hunks)crates/recording/src/output_pipeline/win_segmented_camera.rs(1 hunks)crates/rendering/src/cpu_yuv.rs(1 hunks)crates/rendering/src/decoder/ffmpeg.rs(1 hunks)crates/rendering/src/decoder/frame_converter.rs(1 hunks)crates/rendering/src/layers/display.rs(10 hunks)crates/rendering/src/yuv_converter.rs(18 hunks)crates/video-decode/src/media_foundation.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/rendering/src/decoder/ffmpeg.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Use duration.saturating_sub(other) instead of duration - other to avoid panics on underflow
Merge nested if statements: write 'if a && b { }' instead of 'if a { if b { } }'
Don't call .clone() on Copy types (integers, bools, etc.); copy them directly
Use function references directly: iter.map(foo) instead of iter.map(|x| foo(x))
Accept &[T] or &str instead of &Vec or &String in function parameters for flexibility
Use .is_empty() instead of .len() == 0 or .len() > 0 / .len() != 0
Don't assign () to a variable: write foo(); instead of let _ = foo(); or let x = foo(); when return is unit
Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Use 'for item in &collection' or 'for (i, item) in collection.iter().enumerate()' instead of 'for i in 0..collection.len()'
Use value.clamp(min, max) instead of manual if chains or .min(max).max(min) patterns
Always handle Result/Option or types marked #[must_use]; never ignore them
**/*.rs: Userustfmtand workspace clippy lints for Rust code formatting and linting
Use snake_case for Rust module names and kebab-case for crate names
Never usedbg!()macro in Rust code; use proper logging instead (Clippy:dbg_macro= deny)
Always handleResult/Optionor types marked#[must_use]; never ignore them (Rust compiler lint:unused_must_use= deny)
Never writelet _ = async_fn()which silently drops futures; await or explicitly handle them (Clippy:let_underscore_future= deny)
Usesaturating_subinstead of-forDurationto avoid panics (Clippy:unchecked_duration_subtraction= deny)
Merge nestedifstatements: useif a && b { }instead ofif a { if b { } }(Clippy:collapsible_if= deny)
Don't call.clone()onCopytypes; just copy them directly (Clippy:clone_on_copy= deny)
U...
Files:
crates/rendering/src/decoder/frame_converter.rscrates/rendering/src/layers/display.rscrates/video-decode/src/media_foundation.rsapps/desktop/src-tauri/src/windows.rscrates/rendering/src/yuv_converter.rscrates/rendering/src/cpu_yuv.rscrates/recording/src/output_pipeline/win_segmented_camera.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Never add comments to code (
//,/* */,///,//!,#, etc.); code must be self-explanatory through naming, types, and structure
Files:
crates/rendering/src/decoder/frame_converter.rscrates/rendering/src/layers/display.rscrates/video-decode/src/media_foundation.rsapps/desktop/src-tauri/src/windows.rscrates/rendering/src/yuv_converter.rscrates/rendering/src/cpu_yuv.rscrates/recording/src/output_pipeline/win_segmented_camera.rs
🧠 Learnings (3)
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/rendering/src/decoder/frame_converter.rscrates/video-decode/src/media_foundation.rscrates/rendering/src/yuv_converter.rscrates/recording/src/output_pipeline/win_segmented_camera.rs
📚 Learning: 2025-12-07T14:29:40.743Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.743Z
Learning: Applies to **/*.rs : Never write `let _ = async_fn()` which silently drops futures; await or explicitly handle them (Clippy: `let_underscore_future` = deny)
Applied to files:
crates/video-decode/src/media_foundation.rs
📚 Learning: 2025-10-17T05:58:22.586Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1219
File: crates/enc-avfoundation/src/mp4.rs:350-373
Timestamp: 2025-10-17T05:58:22.586Z
Learning: In crates/enc-avfoundation/src/mp4.rs, the `finish()` method intentionally skips video extension when `is_paused` is true. This is correct behavior because if recording is paused, the video should not be extended beyond the pause point—the pause is user-initiated, unlike the case where ScreenCaptureKit stops providing frames during static content.
Applied to files:
crates/recording/src/output_pipeline/win_segmented_camera.rs
🧬 Code graph analysis (3)
crates/rendering/src/layers/display.rs (3)
crates/rendering/src/yuv_converter.rs (1)
new(133-377)crates/rendering/src/lib.rs (7)
new(116-186)new(386-459)new(501-503)new(1091-1566)new(1583-1588)new(1636-1638)new_with_options(1640-1656)crates/rendering/src/iosurface_texture.rs (1)
device(188-188)
apps/desktop/src-tauri/src/windows.rs (2)
apps/desktop/src-tauri/src/fake_window.rs (2)
app(52-52)spawn_fake_window_listener(48-106)apps/desktop/src-tauri/src/platform/macos/mod.rs (1)
set_window_level(18-27)
crates/recording/src/output_pipeline/win_segmented_camera.rs (1)
crates/recording/src/output_pipeline/win.rs (1)
upload_mf_buffer_to_texture(724-778)
⏰ 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: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (23)
crates/rendering/src/decoder/frame_converter.rs (1)
1-2: Remove the#![allow(dead_code)]attribute—the module's public APIs are actively used.The
FrameConverterstruct is imported and instantiated inffmpeg.rs, andcopy_rgba_planeis called fromavassetreader.rs. This suppression masks active code usage and should be deleted.Likely an incorrect or invalid review comment.
apps/desktop/src-tauri/src/windows.rs (1)
711-721: LGTM: Clean position computation with proper logging.The per-monitor centered positioning logic is correct, and the debug logging provides useful diagnostic information for window placement issues.
crates/rendering/src/yuv_converter.rs (8)
1-43: LGTM! Error handling is well-structured.The error enum properly handles platform-specific errors and provides detailed context for dimension limit violations.
85-104: Dimension validation properly addresses previous review concern.The pre-allocated 4K texture limits are now enforced at the entry of each conversion method, preventing validation errors from
write_texturecalls.
106-377: Pre-allocation strategy improves performance predictability.The constructor pre-allocates all textures at maximum 4K dimensions (~82MB per instance), eliminating runtime allocation overhead. The double-buffering design allows the GPU to process one frame while the next is being prepared.
379-389: LGTM! Double-buffer management is clean and correct.The toggle pattern (1 - current_output) and helper methods provide a clear abstraction for managing the ping-pong buffers.
872-971: CPU conversion paths provide software fallback with SIMD optimization.These methods allocate temporary RGBA buffers (~33MB for 4K) for CPU-side conversion using SIMD, which is appropriate for the software fallback path. The unused
_deviceparameter maintains API consistency across conversion methods.
403-403: Consistent validation pattern across all conversion entry points.All seven conversion methods properly validate dimensions before processing and swap buffers after validation succeeds, ensuring pre-allocated texture limits are never exceeded.
Also applies to: 497-497, 574-574, 654-654, 806-806, 883-883, 934-934
973-976: LGTM! Output texture access aligns with double-buffering.The method now returns the currently active output texture, enabling callers to access the latest conversion result.
714-719: The NV12 memory layout is correct for D3D11 mapped staging textures. WhenID3D11DeviceContext::Map()is called on an NV12 staging texture, it returns a pointer to contiguous subresource data, with RowPitch indicating the bytes-per-row spacing. NV12 format compresses the chrominance plane to one quarter of the resolution, with the chroma plane containing half the height and identical width of the luminance plane. The code correctly calculatesy_sizeusingy_stride * height(which accounts for alignment padding via RowPitch), then accesses the UV plane at offsety_sizefrom the mapped base pointer—this is the standard and safe approach for D3D11 staging textures. No edge cases exist across D3D11 drivers for this pattern.crates/rendering/src/layers/display.rs (5)
25-32: LGTM!The constructor delegation pattern is clean, and the backward-compatible
new()method ensures existing code continues to work while allowing new code to opt into CPU conversion.
34-65: LGTM!The new constructor properly initializes all fields and includes helpful tracing when CPU conversion is preferred.
83-90: LGTM!The structured tracing provides valuable debugging information for frame processing.
252-376: LGTM!The Windows-specific NV12 handling correctly implements a fallback chain: D3D11 zero-copy → CPU/GPU conversion based on preference. The extensive debug logging is appropriate for troubleshooting platform-specific interop issues.
525-540: LGTM!The render method correctly implements a fullscreen triangle (3 vertices) which is a common optimization. The added tracing and warning for missing bind groups improve debuggability.
crates/rendering/src/cpu_yuv.rs (5)
1-45: LGTM!The scalar NV12 to RGBA conversion is well-implemented with proper bounds checking using
.get()and safe default values. The YUV to RGB conversion uses standard BT.601 coefficients.
47-93: LGTM!The scalar YUV420P to RGBA conversion correctly handles separate Y, U, V planes with proper 4:2:0 chroma subsampling and safe bounds checking.
220-231: LGTM!The fallback implementation correctly delegates to the scalar version for non-x86 architectures, providing consistent API across platforms.
361-376: LGTM!The fallback implementation correctly delegates to the scalar version for non-x86 architectures.
378-464: LGTM!The
clamp_u8helper correctly uses.clamp()as per coding guidelines. The tests appropriately verify basic conversion correctness and SIMD/scalar parity with reasonable tolerance for rounding differences.crates/video-decode/src/media_foundation.rs (3)
63-121: LGTM! Resource leak issue resolved.The
MFInitGuardpattern correctly addresses the resource leak flagged in the previous review. The guard ensuresMFShutdown()andCoUninitialize()are called if any initialization step fails between lines 89 and 106, and is disarmed viastd::mem::forget()on success so that theMediaFoundationDecoder'sDropimplementation handles cleanup.
504-513: LGTM! Good defensive programming.The
.max(1)on line 513 ensures the frame rate denominator is never zero, preventing potential division-by-zero errors in downstream code. The fallback default frame rate on line 507 is also sensible.
329-348: LGTM! Seek implementation follows Media Foundation API requirements.The manual
PROPVARIANTconstruction is necessary for the Media Foundation API. The implementation correctly sets up aVT_I8variant for the 100-nanosecond timestamp and includes proper error handling.
| let multithread: windows::Win32::Graphics::Direct3D11::ID3D11Multithread = device | ||
| .cast() | ||
| .map_err(|e| format!("Failed to get ID3D11Multithread: {e:?}"))?; | ||
| unsafe { | ||
| let _ = multithread.SetMultithreadProtected(true); | ||
| } |
There was a problem hiding this comment.
Handle the Result from SetMultithreadProtected.
Ignoring the Result on line 395 violates the coding guideline to always handle Result types. If enabling multithread protection fails, thread-safety issues could arise silently.
Apply this diff:
let multithread: windows::Win32::Graphics::Direct3D11::ID3D11Multithread = device
.cast()
.map_err(|e| format!("Failed to get ID3D11Multithread: {e:?}"))?;
unsafe {
- let _ = multithread.SetMultithreadProtected(true);
+ multithread
+ .SetMultithreadProtected(true)
+ .map_err(|e| tracing::warn!("Failed to set multithread protection: {e:?}"))
+ .ok();
}🤖 Prompt for AI Agents
In crates/video-decode/src/media_foundation.rs around lines 391–396, the Result
returned by multithread.SetMultithreadProtected(true) is ignored; capture and
handle that Result instead (for example call .ok() or check the HRESULT and
propagate or return an error), mapping any failure into a descriptive error like
"Failed to set multithread protected" and use the ? operator to propagate the
error from the enclosing function so failures don't silently go unnoticed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/recording/src/output_pipeline/win_segmented_camera.rs (1)
182-195: Thread panic information is still being lost.Despite the past review comment being marked as addressed, line 184 still uses
let _ = handle.join();without checking for panics. This is inconsistent with therotate_segmentmethod (lines 461-466), which properly logs panic information.Apply this diff to match the behavior in
rotate_segment:loop { if handle.is_finished() { - let _ = handle.join(); + if let Err(e) = handle.join() { + warn!("Camera encoder thread panicked: {e:?}"); + } break; }
🧹 Nitpick comments (5)
apps/desktop/src-tauri/src/windows.rs (1)
744-766: Simplify the redundant block structure.The inner block creates a variable and immediately returns it, which can be simplified to match the macOS pattern.
#[cfg(windows)] -let window = { - let window = self - .window_builder(app, "/in-progress-recording") - .maximized(false) - .resizable(false) - .fullscreen(false) - .shadow(false) - .always_on_top(true) - .transparent(true) - .visible_on_all_workspaces(true) - .content_protected(should_protect) - .inner_size(width, height) - .position(pos_x, pos_y) - .skip_taskbar(false) - .initialization_script(format!( - "window.COUNTDOWN = {};", - countdown.unwrap_or_default() - )) - .build()?; - - window -}; +let window = self + .window_builder(app, "/in-progress-recording") + .maximized(false) + .resizable(false) + .fullscreen(false) + .shadow(false) + .always_on_top(true) + .transparent(true) + .visible_on_all_workspaces(true) + .content_protected(should_protect) + .inner_size(width, height) + .position(pos_x, pos_y) + .skip_taskbar(false) + .initialization_script(format!( + "window.COUNTDOWN = {};", + countdown.unwrap_or_default() + )) + .build()?;crates/recording/src/output_pipeline/win_segmented_camera.rs (4)
48-95: Extract duplicated code to shared module.Both
PauseTrackerandduration_to_timespanare duplicated fromwin_segmented.rs. This violates the DRY principle and creates maintenance burden.Consider extracting these to a shared module (e.g.,
win_common.rsorwin_utils.rs) that both files can import.Also applies to: 559-571
415-425: Handle mutex poisoning explicitly.Line 416 uses
.unwrap()on mutex lock, which will panic if the mutex is poisoned. This can crash the encoder thread. Consider handling the poisoned mutex error explicitly.Apply this diff:
|output_sample| { - let mut output = output_clone.lock().unwrap(); + let mut output = output_clone.lock().map_err(|e| { + windows::core::Error::new( + windows::core::HRESULT(-1), + format!("Failed to lock output mutex: {e}"), + ) + })?; muxer .write_sample(&output_sample, &mut output)
525-527: Redundant defensive check.Lines 525-527 check if
segment_start_timeis None, but this condition cannot be true at this point. Ifcurrent_statewas None on line 520, we would have setsegment_start_timeon line 521. Ifcurrent_statewas already Some, thensegment_start_timemust have been set when that segment was created.Consider removing this redundant check to simplify the logic:
if self.segment_start_time.is_none() { self.segment_start_time = Some(adjusted_timestamp); } - - if self.segment_start_time.is_none() { - self.segment_start_time = Some(adjusted_timestamp); - }
434-434: Handle mutex poisoning explicitly.The
.lock().unwrap()call will panic if the mutex is poisoned. Consider using error propagation instead.Apply this diff:
- output.lock().unwrap().write_header()?; + output + .lock() + .map_err(|e| anyhow!("Failed to lock output for header: {e}"))? + .write_header()?;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src-tauri/src/windows.rs(1 hunks)crates/recording/src/output_pipeline/win_segmented_camera.rs(1 hunks)crates/rendering/src/cpu_yuv.rs(1 hunks)crates/rendering/src/yuv_converter.rs(18 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Use duration.saturating_sub(other) instead of duration - other to avoid panics on underflow
Merge nested if statements: write 'if a && b { }' instead of 'if a { if b { } }'
Don't call .clone() on Copy types (integers, bools, etc.); copy them directly
Use function references directly: iter.map(foo) instead of iter.map(|x| foo(x))
Accept &[T] or &str instead of &Vec or &String in function parameters for flexibility
Use .is_empty() instead of .len() == 0 or .len() > 0 / .len() != 0
Don't assign () to a variable: write foo(); instead of let _ = foo(); or let x = foo(); when return is unit
Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Use 'for item in &collection' or 'for (i, item) in collection.iter().enumerate()' instead of 'for i in 0..collection.len()'
Use value.clamp(min, max) instead of manual if chains or .min(max).max(min) patterns
Always handle Result/Option or types marked #[must_use]; never ignore them
**/*.rs: Userustfmtand workspace clippy lints for Rust code formatting and linting
Use snake_case for Rust module names and kebab-case for crate names
Never usedbg!()macro in Rust code; use proper logging instead (Clippy:dbg_macro= deny)
Always handleResult/Optionor types marked#[must_use]; never ignore them (Rust compiler lint:unused_must_use= deny)
Never writelet _ = async_fn()which silently drops futures; await or explicitly handle them (Clippy:let_underscore_future= deny)
Usesaturating_subinstead of-forDurationto avoid panics (Clippy:unchecked_duration_subtraction= deny)
Merge nestedifstatements: useif a && b { }instead ofif a { if b { } }(Clippy:collapsible_if= deny)
Don't call.clone()onCopytypes; just copy them directly (Clippy:clone_on_copy= deny)
U...
Files:
apps/desktop/src-tauri/src/windows.rscrates/recording/src/output_pipeline/win_segmented_camera.rscrates/rendering/src/yuv_converter.rscrates/rendering/src/cpu_yuv.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Never add comments to code (
//,/* */,///,//!,#, etc.); code must be self-explanatory through naming, types, and structure
Files:
apps/desktop/src-tauri/src/windows.rscrates/recording/src/output_pipeline/win_segmented_camera.rscrates/rendering/src/yuv_converter.rscrates/rendering/src/cpu_yuv.rs
🧠 Learnings (8)
📚 Learning: 2025-12-07T14:29:19.180Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T14:29:19.180Z
Learning: Applies to **/*.rs : Always handle Result/Option or types marked #[must_use]; never ignore them
Applied to files:
apps/desktop/src-tauri/src/windows.rs
📚 Learning: 2025-12-07T14:29:40.743Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.743Z
Learning: Applies to **/*.rs : Always handle `Result`/`Option` or types marked `#[must_use]`; never ignore them (Rust compiler lint: `unused_must_use` = deny)
Applied to files:
apps/desktop/src-tauri/src/windows.rs
📚 Learning: 2025-12-07T14:29:40.743Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.743Z
Learning: Applies to **/*.rs : Never write `let _ = async_fn()` which silently drops futures; await or explicitly handle them (Clippy: `let_underscore_future` = deny)
Applied to files:
apps/desktop/src-tauri/src/windows.rscrates/rendering/src/yuv_converter.rs
📚 Learning: 2025-12-07T14:29:19.180Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T14:29:19.180Z
Learning: Applies to **/*.rs : Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Applied to files:
apps/desktop/src-tauri/src/windows.rscrates/recording/src/output_pipeline/win_segmented_camera.rs
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/recording/src/output_pipeline/win_segmented_camera.rscrates/rendering/src/yuv_converter.rs
📚 Learning: 2025-10-17T05:58:22.586Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1219
File: crates/enc-avfoundation/src/mp4.rs:350-373
Timestamp: 2025-10-17T05:58:22.586Z
Learning: In crates/enc-avfoundation/src/mp4.rs, the `finish()` method intentionally skips video extension when `is_paused` is true. This is correct behavior because if recording is paused, the video should not be extended beyond the pause point—the pause is user-initiated, unlike the case where ScreenCaptureKit stops providing frames during static content.
Applied to files:
crates/recording/src/output_pipeline/win_segmented_camera.rs
📚 Learning: 2025-12-07T14:29:40.743Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.743Z
Learning: Applies to **/*.rs : Use `.unwrap_or(val)` instead of `.unwrap_or_else(|| val)` for cheap values (Clippy: `unnecessary_lazy_evaluations` = deny)
Applied to files:
crates/rendering/src/yuv_converter.rs
📚 Learning: 2025-12-07T14:29:19.180Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T14:29:19.180Z
Learning: Applies to **/*.rs : Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Applied to files:
crates/rendering/src/yuv_converter.rs
🧬 Code graph analysis (3)
apps/desktop/src-tauri/src/windows.rs (2)
apps/desktop/src-tauri/src/fake_window.rs (2)
app(52-52)spawn_fake_window_listener(48-106)apps/desktop/src-tauri/src/platform/macos/mod.rs (1)
set_window_level(18-27)
crates/recording/src/output_pipeline/win_segmented_camera.rs (2)
crates/recording/src/output_pipeline/win.rs (4)
upload_mf_buffer_to_texture(724-778)adjust(41-72)timestamp(409-411)duration_to_timespan(381-393)crates/recording/src/output_pipeline/win_segmented.rs (3)
new(58-64)adjust(66-97)duration_to_timespan(608-620)
crates/rendering/src/yuv_converter.rs (1)
crates/rendering/src/d3d_texture.rs (1)
import_d3d11_texture_to_wgpu(189-201)
⏰ 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: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (19)
apps/desktop/src-tauri/src/windows.rs (4)
711-721: LGTM!Position calculation correctly converts physical to logical coordinates, and debug logging aids troubleshooting window placement across displays with varying scale factors.
723-742: LGTM!macOS window configuration is complete and follows the established builder pattern.
768-773: LGTM!Debug logging correctly captures window state for diagnostics. Using
{:?}on the Result types will show success/failure which is useful for troubleshooting.
780-800: LGTM!OS-specific show behavior is properly implemented. The Windows path correctly uses
.ok()to handle theset_focus()Result, and the 100ms delay addresses platform-specific timing needs.crates/rendering/src/cpu_yuv.rs (5)
1-45: LGTM! Safe scalar implementation with bounds checking.The scalar NV12 conversion correctly uses
.get()for bounds-checked access and validates output indices before writing.
47-93: LGTM! Safe YUV420P scalar implementation.The scalar YUV420P conversion follows the same safe patterns as the NV12 variant, with proper bounds checking throughout.
95-269: Validation addresses past review concerns.The comprehensive validation at lines 119-144 (buffer sizes, stride checks) followed by fallback to the scalar path when validation fails properly addresses the bounds checking concerns from the previous review. The SIMD implementation correctly processes 8-pixel blocks with a scalar fallback for the remainder.
Based on past review comments, the bounds checking concern has been resolved.
284-469: Unaligned reads correctly handled.Lines 399-400 use
std::ptr::read_unalignedto safely load i32 values from potentially unaligned u8 pointers, properly addressing the undefined behavior concern raised in the previous review. The validation and SIMD processing logic mirrors the NV12 variant correctly.Based on past review comments, the unaligned read UB has been fixed.
488-575: LGTM! Helper and tests are well-structured.The
clamp_u8helper correctly uses.clamp()as per coding guidelines. The tests validate both basic conversion correctness and SIMD/scalar parity with an appropriate tolerance for rounding differences.crates/rendering/src/yuv_converter.rs (10)
1-43: LGTM! Cross-platform error handling and imports.The new error variants (DimensionExceedsLimit, D3DTextureError, D3D11Error) and platform-guarded imports properly support the Windows D3D11 conversion paths.
85-104: Dimension validation addresses past review concern.The
validate_dimensionsfunction properly checks input dimensions against the 4K pre-allocated texture limits and returns a descriptive error when exceeded. This resolves the concern raised in the previous review about lacking dimension checks.Based on past review comments, dimension validation has been implemented.
106-377: LGTM! Pre-allocated textures with double-buffering.The refactor from optional per-call textures to pre-allocated, double-buffered output textures improves performance by avoiding repeated allocations. The constructor properly initializes all textures at the maximum supported dimensions (4K).
379-389: LGTM! Clean double-buffer management.The buffer swapping and accessor methods provide a simple, correct double-buffering mechanism.
391-474: LGTM! NV12 conversion properly uses validation and double-buffering.The conversion method correctly validates dimensions, swaps output buffers, and leverages pre-allocated textures throughout.
476-559: LGTM! IOSurface conversion correctly handles external textures.The IOSurface path appropriately creates temporary views for the externally-managed IOSurface textures while using the pre-allocated double-buffered output.
561-641: LGTM! YUV420P conversion follows the same validated pattern.The YUV420P conversion correctly applies dimension validation, buffer swapping, and pre-allocated texture usage consistent with the NV12 path.
643-796: LGTM! D3D11 texture conversion with proper error handling.The D3D11 staging texture path correctly:
- Validates dimensions upfront
- Creates/reuses staging textures based on size changes
- Uses
.ok_or_else()at line 695 for lazy error creation (appropriate since String allocation isn't cheap)- Properly maps, copies, and unmaps D3D11 resources
- Leverages the double-buffered output pattern
This addresses the previous review concern about
.unwrap()usage.Based on past review comments, error handling has been improved from
.unwrap()to.ok_or_else().
798-973: LGTM! CPU and D3D11 shared handle conversion paths.The CPU conversion methods (
convert_nv12_cpu,convert_yuv420p_cpu) correctly delegate to the SIMD-acceleratedcpu_yuvmodule and write results to the double-buffered output. Theconvert_nv12_from_d3d11_shared_handlesmethod provides scaffolding for the zero-copy D3D11 import path (note: the underlyingimport_d3d11_texture_to_wgpureturns a stub error per the relevant code context).
975-977: LGTM! Output texture accessor.The accessor correctly returns the current output texture wrapped in
Some, maintaining API compatibility.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.