feat: Lots of Windows + desktop optimisations #1459
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 12 minutes and 58 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 (2)
WalkthroughAdds platform system diagnostics and a Tauri command; implements HEVC encoders (FFmpeg + Media Foundation); enhances GPU discovery, D3D11/frame-conversion, capturer staging/shared-handle support; threads encoder preferences and hardware/software fallback through Windows muxers; introduces encoder/decoder health monitoring and richer decoder init/status; adjusts desktop window sizing and local CLAUDE permissions. Changes
Sequence Diagram(s)sequenceDiagram
participant Frontend as Desktop UI
participant Tauri as Tauri Backend
participant Diag as recording::diagnostics
participant GPU as frame-converter / D3D11
participant Capturer as scap-direct3d
Frontend->>Tauri: getSystemDiagnostics()
Tauri->>Diag: collect_diagnostics()
Diag->>GPU: detect_primary_gpu() / get_all_gpus()
GPU-->>Diag: GPU info
Diag->>Diag: get_windows_version_info()
Diag->>Capturer: check_graphics_capture_support()
Capturer-->>Diag: capture support result
Diag-->>Tauri: SystemDiagnostics
Tauri-->>Frontend: SystemDiagnostics
Frontend->>Frontend: render System Information
sequenceDiagram
participant Muxer as Windows Muxer
participant Resolver as Encoder Resolver
participant HW as Hardware Encoder
participant SW as Software Encoder
Muxer->>Muxer: read encoder_preferences
alt force_software
Muxer->>SW: init software encoder
SW-->>Muxer: ready
else try_hardware
Muxer->>Resolver: resolve encoder (platform/GPU)
Resolver->>HW: init hardware encoder
alt validation_ok
HW-->>Muxer: hardware ready
else validation_fail
HW-->>Resolver: failure
Resolver->>SW: init software encoder (fallback)
SW-->>Muxer: software ready
end
end
Muxer->>Muxer: start encoding frames (selected encoder)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
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/windows.rs (1)
275-567: Extract duplicated Windows sizing workaround into helper function.Six window variants contain nearly identical Windows-specific blocks that import
LogicalSize, callset_size(), and callcenter(). This duplication violates DRY principles and makes the code harder to maintain.🔎 Suggested refactor
Add a helper function at module level:
#[cfg(windows)] fn apply_windows_size_workaround( window: &WebviewWindow, width: f64, height: f64, ) -> tauri::Result<()> { use tauri::LogicalSize; window.set_size(LogicalSize::new(width, height))?; window.center()?; Ok(()) }Then replace each Windows block with:
#[cfg(windows)] if let Err(e) = apply_windows_size_workaround(&window, 600.0, 600.0) { tracing::warn!("Failed to apply Windows size workaround for Setup window: {}", e); }This eliminates duplication, centralizes the workaround logic, and makes the pattern easier to maintain.
🟡 Minor comments (8)
crates/enc-mediafoundation/src/video/h264.rs-627-633 (1)
627-633: SameResulthandling issue for empty samples path.🔎 Suggested fix
- let _ = self.cleanup_encoder(); + if let Err(e) = self.cleanup_encoder() { + tracing::warn!("Cleanup failed after too many empty samples: {e:?}"); + }crates/frame-converter/src/d3d11.rs-1146-1161 (1)
1146-1161:CloseHandleresult should be handled explicitly.The
let _ = CloseHandle(handle);pattern discards the result, violating coding guidelines. While handle closure errors during drop are typically not actionable, they should be logged for diagnostics.🔎 Suggested fix
impl Drop for D3D11Resources { fn drop(&mut self) { unsafe { if let Some(handle) = self.input_shared_handle.take() { if !handle.is_invalid() { - let _ = CloseHandle(handle); + if let Err(e) = CloseHandle(handle) { + tracing::debug!("Failed to close input shared handle: {e:?}"); + } } } if let Some(handle) = self.output_shared_handle.take() { if !handle.is_invalid() { - let _ = CloseHandle(handle); + if let Err(e) = CloseHandle(handle) { + tracing::debug!("Failed to close output shared handle: {e:?}"); + } } } } } }Based on coding guidelines requiring explicit handling of Result types.
crates/enc-mediafoundation/src/video/h264.rs-547-554 (1)
547-554: DiscardedResultfromcleanup_encoderviolates guidelines.The coding guidelines state: "Always handle Result/Option or types marked #[must_use]; never ignore them." The
let _ = self.cleanup_encoder();pattern silently discards the cleanup result. While cleanup errors during an unhealthy state may be intentional to ignore, this should be explicit.Consider logging the error or using a more explicit pattern:
🔎 Suggested fix
- let _ = self.cleanup_encoder(); + if let Err(e) = self.cleanup_encoder() { + tracing::warn!("Cleanup failed during unhealthy exit: {e:?}"); + }Based on coding guidelines, Result types should always be handled explicitly.
crates/enc-mediafoundation/src/video/h264.rs-593-604 (1)
593-604: SameResulthandling issue in process failure path.This is the same pattern as above - discarding the cleanup result without explicit handling.
🔎 Suggested fix
- let _ = self.cleanup_encoder(); + if let Err(e) = self.cleanup_encoder() { + tracing::warn!("Cleanup failed after process failures: {e:?}"); + }crates/recording/src/output_pipeline/win_segmented_camera.rs-517-528 (1)
517-528: Potential panic on poisoned mutex.Line 519 uses
.unwrap()on a mutex lock. If the mutex is poisoned (e.g., a previous panic while holding the lock), this will panic the encoder thread. Consider handling the poisoned case gracefully:Suggested fix
|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: {e}"), + ) + })?; muxer .write_sample(&output_sample, &mut output)crates/recording/src/output_pipeline/win.rs-953-968 (1)
953-968: Silent data truncation when source buffer is smaller than expected.The
camera_frame_to_ffmpegfunction silently produces incomplete frames iffinal_data.len()is less than the expected size. This could result in partially filled frames with uninitialized/zero data being encoded without any warning.🔎 Consider logging when data is truncated:
match final_format { ffmpeg::format::Pixel::NV12 => { let y_size = (frame.width * frame.height) as usize; let uv_size = y_size / 2; - if final_data.len() >= y_size + uv_size { + if final_data.len() >= y_size + uv_size { ffmpeg_frame.data_mut(0)[..y_size].copy_from_slice(&final_data[..y_size]); ffmpeg_frame.data_mut(1)[..uv_size].copy_from_slice(&final_data[y_size..]); + } else { + tracing::warn!( + "NV12 buffer undersize: got {} bytes, expected {}", + final_data.len(), + y_size + uv_size + ); } } ffmpeg::format::Pixel::YUYV422 => { let size = (frame.width * frame.height * 2) as usize; - if final_data.len() >= size { + if final_data.len() >= size { ffmpeg_frame.data_mut(0)[..size].copy_from_slice(&final_data[..size]); + } else { + tracing::warn!( + "YUYV422 buffer undersize: got {} bytes, expected {}", + final_data.len(), + size + ); } } _ => {} }crates/enc-mediafoundation/src/video/hevc.rs-419-431 (1)
419-431: Empty sample handling could be more informative.The consecutive empty sample counter is a good safeguard, but
MAX_CONSECUTIVE_EMPTY_SAMPLES = 20might be too generous. Also, the error returned hasHRESULT(0)which indicates success, not an error.🔎 Use a proper error HRESULT:
} else { consecutive_empty_samples += 1; if consecutive_empty_samples > MAX_CONSECUTIVE_EMPTY_SAMPLES { return Err(windows::core::Error::new( - windows::core::HRESULT(0), + windows::core::HRESULT(-1), "Too many consecutive empty samples", )); } }crates/enc-ffmpeg/src/video/hevc.rs-243-248 (1)
243-248: Potential division by zero if frame_rate denominator is zero.If
input_config.frame_rate.1(the denominator) is zero, this will cause a division by zero panic.🔎 Apply this diff to add a guard:
let bitrate = get_bitrate( output_width, output_height, - input_config.frame_rate.0 as f32 / input_config.frame_rate.1 as f32, + input_config.frame_rate.0 as f32 / input_config.frame_rate.1.max(1) as f32, bpp, );
🧹 Nitpick comments (14)
apps/desktop/src-tauri/src/general_settings.rs (1)
144-146: Consider consolidating duplicate helper functions.Both
default_true()(line 144) andtrue_b()(line 209) serve the same purpose. Consider using a single function for consistency.🔎 Suggested consolidation
-fn default_true() -> bool { - true -} - ... -fn true_b() -> bool { - true -} +fn default_true() -> bool { + true +}Then update references from
true_btodefault_trueat lines 75 and 80.crates/scap-direct3d/src/windows_version.rs (1)
26-32: Consider consolidating identical methods.
supports_border_control()andis_windows_11()have identical implementations. Consider having one delegate to the other for clarity:🔎 Suggested refactor
pub fn supports_border_control(&self) -> bool { - self.build >= 22000 + self.is_windows_11() } pub fn is_windows_11(&self) -> bool { self.build >= 22000 }crates/enc-mediafoundation/src/video/h264.rs (1)
654-664:cleanup_encoderalso discards Results internally.Lines 656-661 discard the results of
ProcessMessagecalls. While these are best-effort cleanup operations, the pattern still violates coding guidelines.🔎 Consider logging cleanup failures
fn cleanup_encoder(&mut self) -> windows::core::Result<()> { unsafe { - let _ = self - .transform - .ProcessMessage(MFT_MESSAGE_NOTIFY_END_OF_STREAM, 0); - let _ = self - .transform - .ProcessMessage(MFT_MESSAGE_NOTIFY_END_STREAMING, 0); + if let Err(e) = self.transform.ProcessMessage(MFT_MESSAGE_NOTIFY_END_OF_STREAM, 0) { + tracing::debug!("END_OF_STREAM cleanup message failed: {e:?}"); + } + if let Err(e) = self.transform.ProcessMessage(MFT_MESSAGE_NOTIFY_END_STREAMING, 0) { + tracing::debug!("END_STREAMING cleanup message failed: {e:?}"); + } self.transform.ProcessMessage(MFT_MESSAGE_COMMAND_FLUSH, 0) } }crates/frame-converter/src/d3d11.rs (1)
818-820: Redundant check:frame_count > 0is always true.Since
frame_count = count + 1andcountstarts at 0 (fromfetch_add),frame_countis always >= 1. Theframe_count > 0check is redundant.🔎 Suggested fix
let frame_count = count + 1; - if frame_count > 0 && frame_count % 300 == 0 { + if frame_count % 300 == 0 {apps/desktop/src/routes/(window-chrome)/settings/feedback.tsx (1)
125-131: Consider adding error state handling.When
fetchDiagnosticsfails and returnsnull, theShowcondition!diagnostics.loading && diagnostics()will be falsy, causing the fallback "Loading system information..." to display indefinitely. This could confuse users.Consider distinguishing between loading and error states:
🔎 Suggested approach
<Show - when={!diagnostics.loading && diagnostics()} + when={!diagnostics.loading} fallback={ <p class="text-sm text-gray-10"> Loading system information... </p> } > - {(diag) => ( + <Show + when={diagnostics()} + fallback={ + <p class="text-sm text-gray-10"> + Unable to load system information. + </p> + } + > + {(diag) => ( + // ... existing content + )} + </Show>crates/recording/src/output_pipeline/win_segmented.rs (1)
764-775: Consider deduplicatingduration_to_timespan.This function is duplicated from
win.rs(lines 405-417). Consider extracting it to a shared utility module to reduce code duplication.crates/rendering/src/decoder/media_foundation.rs (1)
13-86: Good addition of health monitoring for decoder diagnostics.The
DecoderHealthMonitorprovides useful observability. A few observations:
average_decode_time()divides by 32 regardless of how many frames have been decoded. Early in decoding, this will undercount the average. Consider tracking the actual count or using a different calculation.The health check warns but doesn't take corrective action. This is fine for observability, but consider whether unhealthy state should trigger recovery (e.g., seek to clear state).
Optional: More accurate average calculation
struct DecoderHealthMonitor { consecutive_errors: u32, consecutive_texture_read_failures: u32, total_frames_decoded: u64, total_errors: u64, last_successful_decode: Instant, frame_decode_times: [Duration; 32], frame_decode_index: usize, + frame_decode_count: usize, slow_frame_count: u32, } fn average_decode_time(&self) -> Duration { + if self.frame_decode_count == 0 { + return Duration::ZERO; + } + let count = self.frame_decode_count.min(32); let sum: Duration = self.frame_decode_times.iter().sum(); - sum / 32 + sum / count as u32 }crates/recording/src/output_pipeline/win.rs (1)
887-907: Unsafe SIMD block has redundantunsafenesting.The outer function is already marked with
#[target_feature(enable = "ssse3")]andunsafe, so the innerunsafeblock is redundant.🔎 Suggested simplification:
#[cfg(target_arch = "x86_64")] #[target_feature(enable = "ssse3")] unsafe fn convert_uyvy_to_yuyv_ssse3(src: &[u8], dst: &mut [u8], len: usize) { use std::arch::x86_64::*; - unsafe { - let shuffle_mask = _mm_setr_epi8(1, 0, 3, 2, 5, 4, 7, 6, 9, 8, 11, 10, 13, 12, 15, 14); - - let mut i = 0; - let simd_end = len & !15; - - while i < simd_end { - let chunk = _mm_loadu_si128(src.as_ptr().add(i) as *const __m128i); - let shuffled = _mm_shuffle_epi8(chunk, shuffle_mask); - _mm_storeu_si128(dst.as_mut_ptr().add(i) as *mut __m128i, shuffled); - i += 16; - } - - convert_uyvy_to_yuyv_scalar(&src[i..], &mut dst[i..], len - i); - } + let shuffle_mask = _mm_setr_epi8(1, 0, 3, 2, 5, 4, 7, 6, 9, 8, 11, 10, 13, 12, 15, 14); + + let mut i = 0; + let simd_end = len & !15; + + while i < simd_end { + let chunk = _mm_loadu_si128(src.as_ptr().add(i) as *const __m128i); + let shuffled = _mm_shuffle_epi8(chunk, shuffle_mask); + _mm_storeu_si128(dst.as_mut_ptr().add(i) as *mut __m128i, shuffled); + i += 16; + } + + convert_uyvy_to_yuyv_scalar(&src[i..], &mut dst[i..], len - i); }crates/camera-windows/src/lib.rs (1)
222-227: Potential overflow inis_high_bandwidthcalculation.The pixel count and fps are cast to
u64after multiplication on the left side of comparison, but the right side3840 * 2160is computed asi32by default. While this works, consider making the intent clearer.🔎 Minor clarity improvement:
pub fn is_high_bandwidth(&self) -> bool { if !self.is_capture_card() { return false; } self.formats().iter().any(|f| { let pixels = f.width() as u64 * f.height() as u64; let fps = f.frame_rate() as u64; - pixels >= 3840 * 2160 && fps >= 30 + pixels >= 3840_u64 * 2160 && fps >= 30 }) }crates/enc-ffmpeg/src/video/hevc.rs (3)
15-20: Duplicate helper functionis_420exists in both h264.rs and hevc.rs.This function is identical to the one in
crates/enc-ffmpeg/src/video/h264.rs(lines 14-19). Consider extracting it to a shared location (e.g., acommon.rsor the parent module) to avoid duplication.
386-396: macOS encoder priority list includes Windows-only encoders.The macOS priority list includes
hevc_qsv,hevc_nvenc,hevc_amf, andhevc_mfwhich are not available on macOS. Whileencoder::find_by_namewill returnNonefor these, iterating through unavailable encoders is wasteful. Consider a cleaner macOS-specific list.🔎 Suggested fix:
#[cfg(target_os = "macos")] { - &[ - "hevc_videotoolbox", - "hevc_qsv", - "hevc_nvenc", - "hevc_amf", - "hevc_mf", - "libx265", - ] + &["hevc_videotoolbox", "libx265"] }
501-507: Duplicateget_bitratefunction exists in both h264.rs and hevc.rs.This function is identical to the one in
crates/enc-ffmpeg/src/video/h264.rs(lines 501-508). Consider extracting to a shared module alongsideis_420.crates/recording/src/diagnostics.rs (2)
69-103: Consider usingdebug!instead ofinfo!for routine diagnostics.These diagnostics are useful for troubleshooting but may be verbose for normal operation. Using
tracing::debug!would allow filtering while keeping the warning atwarn!level for actionable issues.
127-137: Integer division loses precision for dedicated video memory.Line 131 performs integer division before casting to
f64, losing fractional MB values.🔎 Apply this diff:
fn gpu_info_to_diag(info: &cap_frame_converter::GpuInfo) -> GpuInfoDiag { GpuInfoDiag { vendor: info.vendor_name().to_string(), description: info.description.clone(), - dedicated_video_memory_mb: (info.dedicated_video_memory / (1024 * 1024)) as f64, + dedicated_video_memory_mb: info.dedicated_video_memory as f64 / (1024.0 * 1024.0), adapter_index: info.adapter_index, is_software_adapter: info.is_software_adapter, is_basic_render_driver: info.is_basic_render_driver(), supports_hardware_encoding: info.supports_hardware_encoding(), } }
📜 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 (37)
.claude/settings.local.json(1 hunks)apps/desktop/src-tauri/src/general_settings.rs(2 hunks)apps/desktop/src-tauri/src/lib.rs(2 hunks)apps/desktop/src-tauri/src/windows.rs(5 hunks)apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/settings/feedback.tsx(3 hunks)apps/desktop/src/utils/tauri.ts(6 hunks)crates/camera-mediafoundation/Cargo.toml(1 hunks)crates/camera-mediafoundation/src/lib.rs(7 hunks)crates/camera-windows/src/lib.rs(4 hunks)crates/enc-ffmpeg/Cargo.toml(1 hunks)crates/enc-ffmpeg/src/video/h264.rs(3 hunks)crates/enc-ffmpeg/src/video/hevc.rs(1 hunks)crates/enc-ffmpeg/src/video/mod.rs(1 hunks)crates/enc-mediafoundation/src/video/h264.rs(7 hunks)crates/enc-mediafoundation/src/video/hevc.rs(1 hunks)crates/enc-mediafoundation/src/video/mod.rs(1 hunks)crates/frame-converter/Cargo.toml(1 hunks)crates/frame-converter/src/d3d11.rs(21 hunks)crates/recording/src/diagnostics.rs(1 hunks)crates/recording/src/lib.rs(1 hunks)crates/recording/src/output_pipeline/win.rs(8 hunks)crates/recording/src/output_pipeline/win_segmented.rs(3 hunks)crates/recording/src/output_pipeline/win_segmented_camera.rs(6 hunks)crates/recording/src/sources/screen_capture/windows.rs(1 hunks)crates/recording/src/studio_recording.rs(2 hunks)crates/recording/tests/hardware_compat.rs(1 hunks)crates/rendering/src/decoder/avassetreader.rs(4 hunks)crates/rendering/src/decoder/ffmpeg.rs(5 hunks)crates/rendering/src/decoder/media_foundation.rs(9 hunks)crates/rendering/src/decoder/mod.rs(5 hunks)crates/rendering/src/lib.rs(1 hunks)crates/scap-direct3d/Cargo.toml(2 hunks)crates/scap-direct3d/src/lib.rs(17 hunks)crates/scap-direct3d/src/windows_version.rs(1 hunks)crates/scap-ffmpeg/src/direct3d.rs(4 hunks)crates/video-decode/src/ffmpeg.rs(1 hunks)
🧰 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/enc-mediafoundation/src/video/mod.rscrates/recording/src/lib.rscrates/recording/src/output_pipeline/win_segmented.rsapps/desktop/src-tauri/src/lib.rscrates/recording/src/sources/screen_capture/windows.rsapps/desktop/src-tauri/src/windows.rscrates/enc-ffmpeg/src/video/mod.rscrates/scap-direct3d/src/windows_version.rscrates/recording/tests/hardware_compat.rscrates/rendering/src/lib.rscrates/enc-ffmpeg/src/video/h264.rscrates/enc-mediafoundation/src/video/hevc.rscrates/video-decode/src/ffmpeg.rscrates/scap-ffmpeg/src/direct3d.rscrates/rendering/src/decoder/avassetreader.rscrates/recording/src/studio_recording.rscrates/rendering/src/decoder/ffmpeg.rscrates/recording/src/output_pipeline/win_segmented_camera.rscrates/recording/src/output_pipeline/win.rscrates/enc-mediafoundation/src/video/h264.rscrates/rendering/src/decoder/mod.rscrates/camera-mediafoundation/src/lib.rsapps/desktop/src-tauri/src/general_settings.rscrates/scap-direct3d/src/lib.rscrates/rendering/src/decoder/media_foundation.rscrates/enc-ffmpeg/src/video/hevc.rscrates/frame-converter/src/d3d11.rscrates/recording/src/diagnostics.rscrates/camera-windows/src/lib.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/enc-mediafoundation/src/video/mod.rscrates/recording/src/lib.rscrates/recording/src/output_pipeline/win_segmented.rsapps/desktop/src-tauri/src/lib.rscrates/recording/src/sources/screen_capture/windows.rsapps/desktop/src/routes/(window-chrome)/settings/feedback.tsxapps/desktop/src-tauri/src/windows.rscrates/enc-ffmpeg/src/video/mod.rscrates/scap-direct3d/src/windows_version.rscrates/recording/tests/hardware_compat.rscrates/rendering/src/lib.rscrates/enc-ffmpeg/src/video/h264.rscrates/enc-mediafoundation/src/video/hevc.rsapps/desktop/src/routes/(window-chrome)/settings/experimental.tsxcrates/video-decode/src/ffmpeg.rscrates/scap-ffmpeg/src/direct3d.rscrates/rendering/src/decoder/avassetreader.rscrates/recording/src/studio_recording.rsapps/desktop/src/utils/tauri.tscrates/rendering/src/decoder/ffmpeg.rscrates/recording/src/output_pipeline/win_segmented_camera.rscrates/recording/src/output_pipeline/win.rscrates/enc-mediafoundation/src/video/h264.rscrates/rendering/src/decoder/mod.rscrates/camera-mediafoundation/src/lib.rsapps/desktop/src-tauri/src/general_settings.rscrates/scap-direct3d/src/lib.rscrates/rendering/src/decoder/media_foundation.rscrates/enc-ffmpeg/src/video/hevc.rscrates/frame-converter/src/d3d11.rscrates/recording/src/diagnostics.rscrates/camera-windows/src/lib.rs
**/*.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/(window-chrome)/settings/feedback.tsxapps/desktop/src/routes/(window-chrome)/settings/experimental.tsx
**/*.{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/routes/(window-chrome)/settings/feedback.tsxapps/desktop/src/routes/(window-chrome)/settings/experimental.tsxapps/desktop/src/utils/tauri.ts
**/*.{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/routes/(window-chrome)/settings/feedback.tsxapps/desktop/src/routes/(window-chrome)/settings/experimental.tsxapps/desktop/src/utils/tauri.ts
**/*.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/tauri.ts
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/tauri.ts
🧠 Learnings (3)
📚 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 `rustfmt` and workspace clippy lints for Rust code formatting and linting
Applied to files:
crates/enc-ffmpeg/Cargo.toml
📚 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.rscrates/enc-ffmpeg/src/video/h264.rscrates/recording/src/studio_recording.rscrates/rendering/src/decoder/ffmpeg.rscrates/recording/src/output_pipeline/win_segmented_camera.rscrates/recording/src/output_pipeline/win.rscrates/enc-mediafoundation/src/video/h264.rscrates/rendering/src/decoder/mod.rscrates/camera-mediafoundation/src/lib.rscrates/rendering/src/decoder/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 (17)
crates/recording/src/output_pipeline/win_segmented.rs (1)
crates/recording/src/output_pipeline/win.rs (1)
duration_to_timespan(406-418)
apps/desktop/src-tauri/src/lib.rs (2)
apps/desktop/src/utils/tauri.ts (1)
SystemDiagnostics(503-503)crates/recording/src/diagnostics.rs (2)
collect_diagnostics(60-114)collect_diagnostics(295-306)
crates/recording/src/sources/screen_capture/windows.rs (1)
crates/scap-direct3d/src/lib.rs (1)
settings(521-523)
apps/desktop/src/routes/(window-chrome)/settings/feedback.tsx (1)
apps/desktop/src/utils/tauri.ts (2)
SystemDiagnostics(503-503)commands(7-304)
apps/desktop/src-tauri/src/windows.rs (1)
apps/desktop/src/utils/tauri.ts (1)
LogicalSize(439-439)
crates/scap-direct3d/src/windows_version.rs (1)
crates/scap-direct3d/src/lib.rs (2)
new(90-97)new(285-515)
crates/recording/tests/hardware_compat.rs (2)
crates/frame-converter/src/lib.rs (1)
create_converter_with_details(145-237)crates/scap-direct3d/src/windows_version.rs (3)
is_windows_11(30-32)meets_minimum_requirements(22-24)supports_border_control(26-28)
crates/enc-ffmpeg/src/video/h264.rs (2)
crates/enc-ffmpeg/src/video/hevc.rs (1)
get_encoder_priority(385-423)crates/frame-converter/src/d3d11.rs (1)
detect_primary_gpu(114-142)
crates/enc-mediafoundation/src/video/hevc.rs (4)
crates/enc-mediafoundation/src/media.rs (2)
MFSetAttributeRatio(36-43)MFSetAttributeSize(26-33)crates/enc-mediafoundation/src/video/h264.rs (7)
std(332-332)new_with_scaled_output_with_flags(221-420)bitrate(496-498)calculate_bitrate(667-670)new(71-78)new(462-477)new_with_scaled_output(422-440)crates/enc-mediafoundation/src/mft.rs (1)
enumerate_with_flags(42-74)crates/enc-ffmpeg/src/video/h264.rs (1)
new(52-60)
crates/video-decode/src/ffmpeg.rs (1)
crates/rendering/src/decoder/mod.rs (2)
is_hardware_accelerated(29-38)is_hardware_accelerated(490-492)
crates/scap-ffmpeg/src/direct3d.rs (1)
crates/scap-direct3d/src/lib.rs (4)
height(596-598)height(683-685)width(592-594)width(679-681)
crates/rendering/src/decoder/avassetreader.rs (1)
crates/rendering/src/decoder/mod.rs (5)
pts_to_frame(447-450)oneshot(514-514)oneshot(547-547)oneshot(598-598)oneshot(634-634)
crates/recording/src/studio_recording.rs (2)
crates/recording/src/output_pipeline/win.rs (1)
default(466-473)crates/recording/src/output_pipeline/win_segmented_camera.rs (1)
default(163-169)
crates/rendering/src/decoder/ffmpeg.rs (2)
crates/rendering/src/decoder/mod.rs (11)
pts_to_frame(447-450)oneshot(514-514)oneshot(547-547)oneshot(598-598)oneshot(634-634)new(85-87)new(109-116)new(193-206)width(360-362)height(364-366)decoder_type(486-488)crates/rendering/src/decoder/media_foundation.rs (1)
spawn(123-368)
crates/scap-direct3d/src/lib.rs (2)
crates/scap-direct3d/src/windows_version.rs (4)
std(61-61)detect(18-20)display_name(34-45)meets_minimum_requirements(22-24)crates/enc-mediafoundation/src/d3d.rs (1)
create_d3d_device_with_type(19-37)
crates/enc-ffmpeg/src/video/hevc.rs (1)
crates/enc-ffmpeg/src/video/h264.rs (11)
is_420(15-20)new(52-60)with_preset(62-65)with_bpp(67-70)with_output_size(72-79)with_external_conversion(81-84)build(86-134)get_codec_and_options(425-500)build_with_codec(137-272)get_bitrate(502-509)get_encoder_priority(385-423)
crates/recording/src/diagnostics.rs (1)
crates/frame-converter/src/d3d11.rs (8)
gpu_info(605-607)get_gpu_info(296-334)is_basic_render_driver(80-85)detect_primary_gpu(114-142)get_all_gpus(144-146)name(836-838)new(454-603)vendor_name(98-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). (3)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/scap-direct3d/examples/cli.rs (1)
15-52: Remove commented-out code blocks.The extensive commented-out code violates the coding guideline: "Never add comments to code (
//,/* */,///,//!,#, etc.); code must be self-explanatory through naming, types, and structure." Additionally, the guideline specifies that "Unused code (dead code, commented-out code, debug artifacts)" should be removed.The example currently demonstrates no functional behavior beyond sleeping. Either remove this example file entirely if it's no longer relevant, or restore it with a proper working implementation without commented-out code.
As per coding guidelines, commented-out code must be removed.
crates/recording/examples/encoding-benchmark.rs (1)
409-416: Consider logging when an unrecognized mode is used.The wildcard pattern now silently accepts any unrecognized mode (including typos) and runs the full benchmark suite. While this preserves the "full" behavior and provides a sensible default, it could mask user input errors without feedback.
Consider one of these approaches:
Option 1: Log a warning for unrecognized modes
- _ => { + _ => { + if mode != "full" { + eprintln!("Warning: Unknown mode '{}', running full benchmark suite", mode); + } benchmark_conversion_formats(&config); benchmark_encode_times(&config); benchmark_worker_counts(&config);Option 2: Keep "full" explicit and warn on unknown
- _ => { + "full" => { benchmark_conversion_formats(&config); benchmark_encode_times(&config); benchmark_worker_counts(&config); benchmark_resolutions(&config); run_full_benchmark(&config); } + unknown => { + eprintln!("Unknown mode '{}'. Valid modes: formats, encode, workers, resolutions, full", unknown); + std::process::exit(1); + }crates/enc-mediafoundation/examples/cli.rs (1)
102-102: Handle channel send errors instead of silently ignoring them.Both
send()calls ignore theResultreturn value usinglet _ =, which violates the#[must_use]contract. If the receiver is dropped or the channel is full, frames will be silently lost without any indication that capture has failed.🔎 Apply this diff to properly handle send errors:
- let _ = frame_tx.send(Some((frame.texture().clone(), timestamp))); + frame_tx.send(Some((frame.texture().clone(), timestamp))) + .map_err(|_| windows::core::Error::from_win32())?;- let _ = frame_tx.send(None); + frame_tx.send(None) + .map_err(|_| windows::core::Error::from_win32())?;Based on coding guidelines: "Always handle Result/Option or types marked #[must_use]; never ignore them" and "Don't assign () to a variable: write foo(); instead of let _ = foo();"
Also applies to: 108-108
🧹 Nitpick comments (7)
crates/camera-windows/examples/cli.rs (1)
72-73: Remove unused struct instead of suppressing the warning.The
FormatSelectionstruct is public but unused in this example. Rather than suppressing the dead code warning with#[allow(dead_code)], consider removing the struct entirely or making it private if it's intended for future use. Dead code in examples reduces clarity.🔎 Apply this diff to remove the unused struct:
- #[allow(dead_code)] - pub struct FormatSelection(VideoFormat);crates/scap-direct3d/examples/cli.rs (1)
13-13: Remove unused variable assignment.The
_displayvariable is assigned but never used in the current code. Since all the capturer logic is commented out, this line serves no purpose and should be removed.🔎 Apply this diff to remove the unused assignment:
- let _display = display.raw_handle();crates/enc-mediafoundation/examples/cli.rs (1)
315-319: Avoid panicking in Drop implementation.The
.ok().unwrap()pattern will panic ifUnregisterHotKeyfails. Panicking inDropis problematic because it can cause aborts during stack unwinding. Consider either logging the error or silently ignoring it.🔎 Apply this diff to handle errors gracefully:
impl Drop for HotKey { fn drop(&mut self) { - unsafe { UnregisterHotKey(None, self.id).ok().unwrap() } + unsafe { + let _ = UnregisterHotKey(None, self.id); + } } }crates/recording/tests/hardware_compat.rs (2)
565-565: Handle or log the result ofstop_capturing().Per coding guidelines,
Resulttypes should not be silently ignored. Even in test teardown, capturing the result provides better diagnostics if stopping fails.🔎 Suggested fix
- let _ = handle.stop_capturing(); + if let Err(e) = handle.stop_capturing() { + println!("Warning: Failed to stop capture: {e:?}"); + }
780-789: Discardedconvert()results may hide failures during benchmarking.The conversion results are silently dropped. If conversions fail, the benchmark becomes meaningless. Consider handling errors to ensure valid measurements.
🔎 Suggested fix
for _ in 0..warmup_iterations { let frame = test_frame.clone(); - let _ = selection.converter.convert(frame); + selection.converter.convert(frame).ok(); } let start = std::time::Instant::now(); for _ in 0..test_iterations { let frame = test_frame.clone(); - let _ = selection.converter.convert(frame); + selection.converter.convert(frame).ok(); }Or, if failures should abort the test:
selection.converter.convert(frame).expect("Conversion failed during benchmark");crates/frame-converter/src/d3d11.rs (2)
217-264: Replace.unwrap()with.expect()for better error messages.Lines 244 and 261 use
.unwrap()after verifying the collection is not empty. While technically safe, using.expect()with a descriptive message is more idiomatic and aids debugging.🔎 Suggested improvement
let best = discrete_gpus .iter() .max_by_key(|g| g.dedicated_video_memory) - .unwrap(); + .expect("discrete_gpus is not empty, max_by_key should return Some");Apply similar change at line 261:
let best = hardware_gpus .iter() .max_by_key(|g| g.dedicated_video_memory) - .unwrap(); + .expect("hardware_gpus is not empty, max_by_key should return Some");
553-561: Consider flattening the nested if statement.The nested
ifchecking shared handle presence could be simplified per coding guidelines: "Merge nested if statements: write 'if a && b { }' instead of 'if a { if b { } }'."🔎 Suggested refactor
- if input_shared_handle.is_some() && output_shared_handle.is_some() { - tracing::info!("D3D11 converter created with shared texture handles enabled"); - } else { - tracing::warn!( - "D3D11 converter created without shared handles (input: {}, output: {})", - input_shared_handle.is_some(), - output_shared_handle.is_some() - ); - } + match (input_shared_handle.is_some(), output_shared_handle.is_some()) { + (true, true) => tracing::info!("D3D11 converter created with shared texture handles enabled"), + (input, output) => tracing::warn!( + "D3D11 converter created without shared handles (input: {}, output: {})", + input, output + ), + }Alternatively, keep the existing structure if you find it clearer—this is a minor stylistic improvement.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
crates/camera-directshow/examples/cli.rs(2 hunks)crates/camera-windows/examples/cli.rs(1 hunks)crates/cursor-info/examples/cli.rs(2 hunks)crates/enc-mediafoundation/examples/cli.rs(7 hunks)crates/frame-converter/src/d3d11.rs(21 hunks)crates/recording/examples/camera-benchmark.rs(4 hunks)crates/recording/examples/encoding-benchmark.rs(2 hunks)crates/recording/examples/recording-benchmark.rs(1 hunks)crates/recording/tests/hardware_compat.rs(1 hunks)crates/rendering/src/cpu_yuv.rs(3 hunks)crates/scap-direct3d/examples/cli.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/recording/examples/recording-benchmark.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/cpu_yuv.rscrates/scap-direct3d/examples/cli.rscrates/camera-directshow/examples/cli.rscrates/recording/examples/camera-benchmark.rscrates/recording/examples/encoding-benchmark.rscrates/recording/tests/hardware_compat.rscrates/camera-windows/examples/cli.rscrates/enc-mediafoundation/examples/cli.rscrates/cursor-info/examples/cli.rscrates/frame-converter/src/d3d11.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/cpu_yuv.rscrates/scap-direct3d/examples/cli.rscrates/camera-directshow/examples/cli.rscrates/recording/examples/camera-benchmark.rscrates/recording/examples/encoding-benchmark.rscrates/recording/tests/hardware_compat.rscrates/camera-windows/examples/cli.rscrates/enc-mediafoundation/examples/cli.rscrates/cursor-info/examples/cli.rscrates/frame-converter/src/d3d11.rs
🧠 Learnings (1)
📚 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/examples/camera-benchmark.rs
🧬 Code graph analysis (2)
crates/scap-direct3d/examples/cli.rs (5)
crates/recording/src/sources/screen_capture/mod.rs (1)
display(66-72)crates/cursor-capture/src/position.rs (2)
display(60-62)display(176-178)crates/scap-targets/src/platform/win.rs (1)
display(1041-1048)crates/scap-targets/src/platform/macos.rs (1)
display(519-539)crates/scap-targets/src/lib.rs (1)
display(146-148)
crates/camera-directshow/examples/cli.rs (1)
crates/camera-directshow/src/lib.rs (2)
video_info(226-226)video_info(230-232)
⏰ 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 (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (25)
crates/rendering/src/cpu_yuv.rs (3)
993-993: LGTM: Improved test output readability.The use of named format fields makes the assertion message more self-documenting and easier to understand when tests fail.
1063-1063: LGTM: Improved test output readability.Consistent improvement to test assertion formatting using named format fields.
1114-1114: LGTM: Improved test output readability.The named format fields provide clear, self-documenting test failure messages. All three test assertion updates follow a consistent pattern and improve maintainability.
crates/camera-directshow/examples/cli.rs (2)
59-59: LGTM! Redundant reference pattern removed.The
&*pattern was unnecessary sincevideo_info()already returns a reference. This change simplifies the code without altering behavior.
117-117: LGTM! Redundant nested unsafe block removed.The explicit
unsafeblock here was redundant since this code is already within the outerunsafeblock starting at line 25. This change reduces unnecessary nesting.crates/recording/examples/encoding-benchmark.rs (1)
195-195: LGTM! Cleaner inline formatting.The switch from positional to named formatting improves readability.
crates/enc-mediafoundation/examples/cli.rs (1)
192-194: Verify the error handling approach.The code extracts and unwraps the error code but doesn't use the value. The comment suggests this is for "nicer HRESULT printing," but the current implementation will either panic (if
code()returnsNone) or silently succeed without displaying anything.Consider explicitly printing the error:
if let Err(error) = result { eprintln!("Error: {:?}", error); std::process::exit(1); }crates/cursor-info/examples/cli.rs (2)
1-7: LGTM! Platform-specific imports are correctly gated.The
#[cfg(target_os = "macos")]attributes properly isolate macOS-specific dependencies (CursorShapeMacOS,sha2,HashMap), preventing unused imports on Windows builds while keeping the sharedCursorShapeimport ungated.
109-109: LGTM! Cleaner formatting.Named formatting improves readability.
crates/recording/examples/camera-benchmark.rs (3)
130-130: LGTM: Formatting improvement.The inline variable binding in the format string improves readability with no functional change.
373-374: LGTM: Formatting improvements.The inline variable bindings improve readability with no functional changes.
383-385: LGTM: Formatting improvements.The inline variable bindings improve readability with no functional changes.
crates/recording/tests/hardware_compat.rs (6)
5-22: LGTM!The
test_utilsmodule properly usesOncefor idempotent tracing initialization, and.ok()appropriately handles the case where tracing is already initialized.
24-62: LGTM!Both tests follow proper patterns: assert the condition first, then unwrap safely. The test logic correctly validates software encoder availability and frame conversion fallback.
658-738: LGTM!The hardware compatibility summary test is well-structured with clear status indicators. Note: the
truncate_stringusage at line 686 is affected by the UTF-8 safety issue flagged separately.
220-266: LGTM!The camera enumeration test properly handles the case of no cameras (headless/VM environments) and provides useful diagnostic output for format capabilities.
409-509: LGTM!The GPU-specific encoding tests are properly gated with
#[ignore]and include descriptive reasons. The early-return pattern when the required GPU isn't detected is appropriate for hardware-dependent tests.
832-928: LGTM!The minimum requirements check test is comprehensive, covering both required and recommended components. The final assertion ensures CI catches systems that don't meet minimum requirements.
crates/frame-converter/src/d3d11.rs (7)
1-95: LGTM! Well-structured GPU detection foundation.The additions to imports,
GpuInfofields, and helper methods provide a solid foundation for GPU enumeration and classification. Theis_basic_render_driver(),is_warp(), andsupports_hardware_encoding()methods correctly identify software adapters and unsuitable GPUs.
608-643: LGTM! Well-designed accessor methods.The new accessor methods (
input_shared_handle(),output_shared_handle(),output_texture(),has_shared_handles(),average_conversion_time_ms(),format_info()) provide appropriate visibility into the converter's state. Mutex locking and atomic operations are used correctly.
646-873: LGTM! Format support correctly extended.The additions to
pixel_to_dxgi()and new helper functions (supported_input_formats(),is_format_supported(),dxgi_format_name()) properly extend format support to BGRA, RGBA, and P010LE. The#[allow(dead_code)]ondxgi_format_name()is acceptable for debugging utilities.
913-986: LGTM! Shared texture creation follows Windows best practices.The
create_shared_texture()andextract_shared_handle()functions correctly implement Windows shared texture creation with appropriate flags (D3D11_RESOURCE_MISC_SHARED|D3D11_RESOURCE_MISC_SHARED_NTHANDLE) and handle extraction viaIDXGIResource1::CreateSharedHandle(). Error handling is robust with fallback toNoneon failure.
662-829: LGTM! Performance instrumentation is well-implemented.The frame conversion timing (lines 662, 813-829) adds minimal overhead and provides valuable diagnostic data. Using
AtomicU64withOrdering::Relaxedfor accumulation is appropriate for statistics, and logging every 300 frames strikes a good balance.
988-1140: LGTM! Frame copy functions correctly handle extended formats.The additions to
copy_frame_to_mapped()andcopy_mapped_to_frame()properly implement memory layout handling for YUYV422 (2 bytes/pixel), BGRA/RGBA (4 bytes/pixel), and P010LE (16-bit per component, planar). The unsafe pointer operations are correctly bounded.
337-451: LGTM! Robust multi-GPU device creation logic.The
create_device_on_best_adapter()function implements intelligent GPU selection:
- Correctly identifies and skips software adapters (Microsoft Basic Render Driver, WARP)
- Prioritizes discrete GPUs (NVIDIA, AMD, Qualcomm, ARM) over integrated
- Selects based on VRAM for multi-GPU systems
- Uses
D3D_DRIVER_TYPE_UNKNOWNappropriately when providing an explicit adapter- Comprehensive logging aids debugging
This is a significant improvement for multi-GPU Windows systems.
| fn truncate_string(s: &str, max_len: usize) -> String { | ||
| if s.len() <= max_len { | ||
| s.to_string() | ||
| } else { | ||
| format!("{}...", &s[..max_len - 3]) | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential panic on UTF-8 multi-byte characters or small max_len.
Two issues:
- Byte-based slicing
&s[..max_len - 3]will panic if the index falls within a multi-byte UTF-8 character (GPU descriptions may contain non-ASCII). - If
max_len < 3, the subtraction underflows (panic in debug mode).
🔎 Suggested fix using char boundaries
fn truncate_string(s: &str, max_len: usize) -> String {
- if s.len() <= max_len {
+ if s.chars().count() <= max_len {
s.to_string()
} else {
- format!("{}...", &s[..max_len - 3])
+ let truncate_at = max_len.saturating_sub(3);
+ let truncated: String = s.chars().take(truncate_at).collect();
+ format!("{truncated}...")
}
}🤖 Prompt for AI Agents
In crates/recording/tests/hardware_compat.rs around lines 740 to 746, the
current byte-slice truncation (&s[..max_len - 3]) can panic on UTF-8 multi-byte
characters and underflow when max_len < 3; change the function to work on char
boundaries and handle small max_len safely: compute the allowed character count
(if max_len > 3, reserve 3 for "..." and take that many chars from s.chars(),
otherwise take up to max_len chars with no ellipsis), collect into a String and
append "..." only when you reserved space, ensuring you never perform byte-index
arithmetic or subtract when max_len < 3.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
crates/frame-converter/src/d3d11.rs (1)
1145-1164: LGTM: Drop implementation correctly handles CloseHandle errors.The Drop implementation now properly addresses the previous review feedback by:
- Checking the
ResultfromCloseHandle()- Logging errors with
tracing::error!for diagnostics- Avoiding panics in the destructor
This ensures proper resource cleanup while providing visibility into any handle closure failures.
Based on learnings and past review comments.
crates/recording/src/output_pipeline/win.rs (1)
709-714: Same mutex unwrap issue in camera encoder path.Line 709 uses
.unwrap()on the mutex lock. Apply the same graceful handling pattern.crates/enc-mediafoundation/src/video/hevc.rs (1)
429-434: Replaceeprintln!with proper logging.The code correctly avoids
panic!for unknown event types (addressing the previous review concern), buteprintln!is not proper logging. According to the coding guidelines, use tracing macros instead.🔎 Recommended fix
_ => { - eprintln!( - "[cap-enc-mediafoundation] Ignoring unknown media event type: {}", - event_type.0 - ); + tracing::warn!( + "Ignoring unknown media event type: {}", + event_type.0 + ); }As per coding guidelines, never use
dbg!()macro; use proper logging (tracing::debug!, etc.) instead. The same principle applies toeprintln!.
🧹 Nitpick comments (1)
apps/desktop/src-tauri/src/windows.rs (1)
288-297: Consider extracting the Windows sizing workaround into a helper function.The Windows-specific workaround for window sizing is repeated identically across 6 different window creation paths (Setup, Settings, Editor, ScreenshotEditor, Upgrade, ModeSelect). The only differences are the window name in error messages and the size values, which violates DRY principles.
🔎 Proposed refactor to reduce duplication
Add a helper function after the
window_buildermethod:#[cfg(windows)] fn apply_windows_size_workaround( window: &WebviewWindow, width: f64, height: f64, window_name: &str, ) { use tauri::LogicalSize; if let Err(e) = window.set_size(LogicalSize::new(width, height)) { warn!("Failed to set {} window size on Windows: {}", window_name, e); } if let Err(e) = window.center() { warn!("Failed to center {} window on Windows: {}", window_name, e); } }Then replace each Windows-specific block with a single call:
#[cfg(windows)] -{ - use tauri::LogicalSize; - if let Err(e) = window.set_size(LogicalSize::new(600.0, 600.0)) { - warn!("Failed to set Setup window size on Windows: {}", e); - } - if let Err(e) = window.center() { - warn!("Failed to center Setup window on Windows: {}", e); - } -} +apply_windows_size_workaround(&window, 600.0, 600.0, "Setup");Apply similar changes to Settings (600.0, 465.0), Editor (1275.0, 800.0), ScreenshotEditor (1240.0, 800.0), Upgrade (950.0, 850.0), and ModeSelect (580.0, 340.0).
Also applies to: 462-471, 491-500, 520-532, 553-562, 583-592
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/desktop/src-tauri/src/windows.rs(5 hunks)crates/enc-mediafoundation/src/video/hevc.rs(1 hunks)crates/frame-converter/src/d3d11.rs(21 hunks)crates/recording/examples/camera-benchmark.rs(4 hunks)crates/recording/src/output_pipeline/win.rs(9 hunks)crates/recording/src/output_pipeline/win_segmented.rs(3 hunks)crates/recording/src/output_pipeline/win_segmented_camera.rs(6 hunks)crates/recording/tests/hardware_compat.rs(1 hunks)crates/rendering/src/decoder/ffmpeg.rs(5 hunks)crates/scap-ffmpeg/src/direct3d.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/scap-ffmpeg/src/direct3d.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/recording/examples/camera-benchmark.rscrates/recording/tests/hardware_compat.rscrates/rendering/src/decoder/ffmpeg.rscrates/recording/src/output_pipeline/win.rsapps/desktop/src-tauri/src/windows.rscrates/recording/src/output_pipeline/win_segmented.rscrates/recording/src/output_pipeline/win_segmented_camera.rscrates/enc-mediafoundation/src/video/hevc.rscrates/frame-converter/src/d3d11.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/recording/examples/camera-benchmark.rscrates/recording/tests/hardware_compat.rscrates/rendering/src/decoder/ffmpeg.rscrates/recording/src/output_pipeline/win.rsapps/desktop/src-tauri/src/windows.rscrates/recording/src/output_pipeline/win_segmented.rscrates/recording/src/output_pipeline/win_segmented_camera.rscrates/enc-mediafoundation/src/video/hevc.rscrates/frame-converter/src/d3d11.rs
🧠 Learnings (9)
📚 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/examples/camera-benchmark.rscrates/rendering/src/decoder/ffmpeg.rscrates/recording/src/output_pipeline/win.rscrates/recording/src/output_pipeline/win_segmented.rscrates/recording/src/output_pipeline/win_segmented_camera.rscrates/enc-mediafoundation/src/video/hevc.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 `saturating_sub` instead of `-` for `Duration` to avoid panics (Clippy: `unchecked_duration_subtraction` = deny)
Applied to files:
crates/recording/tests/hardware_compat.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/recording/src/output_pipeline/win.rsapps/desktop/src-tauri/src/windows.rscrates/recording/src/output_pipeline/win_segmented.rscrates/frame-converter/src/d3d11.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:
crates/recording/src/output_pipeline/win.rsapps/desktop/src-tauri/src/windows.rscrates/frame-converter/src/d3d11.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.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 : 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.rscrates/frame-converter/src/d3d11.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 : Always handle Result/Option or types marked #[must_use]; never ignore them
Applied to files:
apps/desktop/src-tauri/src/windows.rscrates/frame-converter/src/d3d11.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 use `dbg!()` macro in Rust code; use proper logging instead (Clippy: `dbg_macro` = deny)
Applied to files:
crates/frame-converter/src/d3d11.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 use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Applied to files:
crates/frame-converter/src/d3d11.rs
🧬 Code graph analysis (5)
crates/recording/tests/hardware_compat.rs (3)
crates/frame-converter/src/d3d11.rs (2)
new(453-602)backend(839-841)crates/frame-converter/src/lib.rs (1)
create_converter_with_details(145-237)crates/scap-direct3d/src/lib.rs (1)
is_supported(156-161)
crates/rendering/src/decoder/ffmpeg.rs (3)
crates/rendering/src/decoder/mod.rs (11)
pts_to_frame(447-450)oneshot(514-514)oneshot(547-547)oneshot(598-598)oneshot(634-634)new(85-87)new(109-116)new(193-206)width(360-362)height(364-366)decoder_type(486-488)crates/rendering/src/decoder/media_foundation.rs (1)
spawn(123-368)crates/video-decode/src/ffmpeg.rs (1)
new(182-257)
crates/recording/src/output_pipeline/win.rs (1)
crates/recording/src/capture_pipeline.rs (5)
new(34-38)default(27-29)output(115-115)output(119-119)output(122-123)
apps/desktop/src-tauri/src/windows.rs (1)
apps/desktop/src/utils/tauri.ts (1)
LogicalSize(439-439)
crates/enc-mediafoundation/src/video/hevc.rs (3)
crates/enc-mediafoundation/src/media.rs (2)
MFSetAttributeRatio(36-43)MFSetAttributeSize(26-33)crates/enc-mediafoundation/src/video/h264.rs (8)
new_with_scaled_output_with_flags(221-420)bitrate(496-498)calculate_bitrate(667-670)new(71-78)new(462-477)output_type(500-502)new_with_scaled_output(422-440)sample(131-133)crates/enc-mediafoundation/src/mft.rs (1)
enumerate_with_flags(42-74)
⏰ 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 (39)
crates/frame-converter/src/d3d11.rs (8)
79-95: LGTM: Clear helper methods for GPU capability detection.The helper methods appropriately distinguish between hardware and software adapters, with clear logic for detecting Basic Render Driver scenarios.
111-264: LGTM: Robust multi-GPU detection and selection.The GPU enumeration and selection logic is well-structured:
- Thread-safe lazy initialization using
OnceLock- Prioritizes discrete GPUs (NVIDIA/AMD/Qualcomm/ARM) by VRAM
- Falls back gracefully to integrated or software adapters
- Comprehensive logging for diagnostics
The
.unwrap()calls on lines 244 and 261 are safe because the collections are verified non-empty before callingmax_by_key().
337-451: LGTM: Well-structured device creation on best available GPU.The adapter enumeration and device creation logic correctly:
- Filters out software adapters
- Prioritizes discrete GPUs by VRAM
- Uses
D3D_DRIVER_TYPE_UNKNOWNwhen creating on a specific adapter- Provides clear error messages referencing the selected adapter
517-561: LGTM: Shared texture handling with graceful fallback.The shared texture creation appropriately:
- Returns
Option<HANDLE>for optional interop scenarios- Logs success/failure for diagnostics
- Allows the converter to operate even without shared handles
608-643: LGTM: Clean accessor methods for converter state.The new methods provide appropriate access to:
- Shared texture handles for interop scenarios
- Performance metrics (average conversion time)
- Format configuration details
Cloning the
ID3D11Texture2DCOM interface correctly increments the reference count.
662-829: LGTM: Comprehensive conversion timing instrumentation.The timing logic correctly:
- Captures the entire conversion pipeline duration
- Uses atomic operations for thread-safe accumulation
- Logs periodic averages (every 300 frames) to avoid spam
- Provides useful diagnostics without impacting performance
913-986: LGTM: Robust shared texture creation with defensive error handling.The shared texture utilities correctly:
- Create textures with appropriate sharing flags
- Gracefully handle
CreateSharedHandlefailures (not all GPU/driver combinations support this)- Return
Option<HANDLE>for optional interop- Log warnings for diagnostic purposes without failing the converter
988-1140: LGTM: Correct format-specific copy implementations.The extended copy routines correctly handle:
- YUYV422: 2 bytes per pixel (packed 4:2:2)
- BGRA/RGBA: 4 bytes per pixel
- P010LE: 2 bytes per pixel with separate Y and UV planes (10-bit format)
The byte calculations and plane offsets are correct for each format.
crates/recording/examples/camera-benchmark.rs (3)
130-130: LGTM - Cleaner inline field binding.The updated
println!using inline field binding{input_format:?} {width}x{height}is more concise and idiomatic.
301-304: Good fix - Error handling during drain phase now matches the main encoding loop.This addresses the previous inconsistency where encoding failures during the drain phase were silently ignored. Now errors are logged and dropped frames are properly recorded in metrics.
377-378: LGTM - Consistent inline field binding for output formatting.The frame rate and inter-frame timing output now use inline field bindings consistently.
Also applies to: 387-389
crates/recording/src/output_pipeline/win_segmented.rs (5)
357-359: LGTM - Adaptive queue depth based on frame rate.The dynamic queue depth calculation
((frame_rate / 30.0 * 5.0).ceil() as usize).clamp(3, 12)appropriately scales the buffer size for higher frame rates while maintaining reasonable bounds.
427-431: Good addition - Hardware encoder validation before use.Validating the encoder after construction and before use helps catch initialization issues early and triggers the software fallback path when needed.
523-536: Good improvement - Proper mutex and write_sample error handling.The output callback now gracefully handles mutex poisoning (lines 523-531) and logs write_sample failures (lines 534-536) instead of silently discarding them. This addresses the concerns from the previous review.
542-560: LGTM - Health-aware encoder completion with fallback trigger.The result handling properly distinguishes between successful completion (logging frame count) and recoverable failures (marking for software fallback). The
should_fallback()check ensures future segments use software encoding when hardware encoding proves unreliable.
518-540: Mutex unwrap may panic on poisoned mutex.The output callback at line 519 uses
.unwrap()on the mutex lock, which will panic if the mutex is poisoned. The frame generator callback (lines 523-531) properly handles this case with a match statement. Consider applying the same pattern here for consistency and resilience.🔎 Suggested fix:
|output_sample| { - let mut output = match output_clone.lock() { - Ok(guard) => guard, - Err(e) => { - error!("Failed to lock output mutex: {e}"); - return Err(windows::core::Error::new( - windows::core::HRESULT(0x80004005u32 as i32), - format!("Mutex poisoned: {e}"), - )); - } - }; + let mut output = output_clone.lock().map_err(|e| { + error!("Failed to lock output mutex: {e}"); + windows::core::Error::new( + windows::core::HRESULT(0x80004005u32 as i32), + format!("Mutex poisoned: {e}"), + ) + })?;Wait - I see the code already handles this properly at lines 523-531. The implementation looks correct. Let me re-read...
Actually, looking more carefully at lines 518-540, the mutex handling is properly implemented with the match statement. The error logging and custom HRESULT error return are appropriate.
crates/recording/tests/hardware_compat.rs (5)
5-22: LGTM - Well-structured test initialization utility.Using
std::sync::Onceensures tracing is initialized exactly once across all tests, andtry_init().ok()gracefully handles cases where it's already initialized.
24-36: Good baseline test - Ensures software fallback is always available.This test validates that
libx264is present as the ultimate fallback encoder, which is essential for the fallback strategy implemented in the Windows muxers.
64-108: Comprehensive diagnostics test.The test exercises the new
cap_recording::diagnostics::collect_diagnostics()API and validates that the minimum requirement (libx264) is met.
740-748: Good fix - UTF-8 safe string truncation.The function now correctly uses
.chars().count()for length comparison and.chars().take()for truncation, preventing panics on multi-byte UTF-8 characters. Thesaturating_sub(3)also handles the edge case wheremax_len < 3.
658-738: Well-designed compatibility summary with visual formatting.The summary aggregates multiple diagnostic checks into a clear, user-friendly report. The logic correctly identifies systems as compatible when Graphics Capture works and libx264 is available.
crates/recording/src/output_pipeline/win_segmented_camera.rs (3)
150-151: LGTM - Encoder preferences integration.The
encoder_preferencesfield is properly added to both the muxer struct and config, with sensible defaults usingEncoderPreferences::new().Also applies to: 159-159, 167-167, 201-201
590-594: Good addition - Software encoder flush before completion.This addresses the previous review concern about potentially losing final buffered frames. The software encoder path now properly flushes before returning.
486-549: LGTM - Hardware encoder path with health-aware fallback.The hardware encoder path properly handles the run result, logging success with frame counts and triggering software fallback on recoverable errors. This is consistent with the pattern in
win_segmented.rsandwin.rs.crates/recording/src/output_pipeline/win.rs (5)
116-118: LGTM - Adaptive queue depth matching segmented muxer.The same dynamic queue depth formula is used here, ensuring consistent buffering behavior across muxer implementations.
192-196: Good addition - Hardware encoder validation before use.Consistent with the segmented muxers, the encoder is now validated after construction with automatic fallback to software encoding on validation failure.
869-919: Well-implemented SIMD-accelerated UYVY to YUYV conversion.The implementation correctly:
- Bounds
src_lento prevent out-of-bounds access- Uses SSSE3 shuffle for efficient byte swapping on x86_64
- Falls back to scalar implementation on non-x86_64 or when SSSE3 is unavailable
- The scalar path handles partial chunks safely with bounds checking
921-973: LGTM - Comprehensive camera frame to FFmpeg conversion.The
camera_frame_to_ffmpeghelper properly handles:
- NV12 (separate Y and UV plane copies)
- YUYV422 (packed format)
- UYVY422 (converted to YUYV422 before use)
The bounds checking (
final_data.len() >= ...) prevents buffer overruns.
740-797: LGTM - Software camera encoder path with milestone logging.The software encoder path properly:
- Tracks frame count and logs every 30 frames for debugging
- Uses the new
camera_frame_to_ffmpeghelper- Reports total frames encoded on completion
crates/rendering/src/decoder/ffmpeg.rs (7)
13-13: LGTM: Import additions support enhanced initialization diagnostics.The addition of
tracing::infoand the newDecoderInitResult/DecoderTypeimports align with the enhanced decoder initialization pattern used across the codebase.Also applies to: 17-20
140-147: LGTM: Function signature updates support richer initialization reporting.The signature changes enable passing decoder dimensions, hardware acceleration status, and decoder type through the initialization flow, consistent with the
DecoderInitResultpattern used inmedia_foundation.rs.
150-158: LGTM: Platform-specific hardware decoder selection now correct.The code now properly selects hardware device types for each platform:
- macOS → VideoToolbox
- Linux → VAAPI
- Windows → DXVA2
This addresses the previous review concern about DXVA2 being incorrectly used on Linux.
170-173: LGTM: Structured logging enhances decoder diagnostics.The
tracing::infocall provides valuable runtime telemetry including decoder name, dimensions, and hardware acceleration status.
183-204: LGTM: Decoder initialization result construction is correct.The code properly:
- Determines
DecoderTypebased on hardware acceleration status- Constructs
DecoderInitResultwith dimensions and decoder type- Sends initialization details through the ready channel
This matches the initialization pattern established in
media_foundation.rs.
383-383: LGTM: Return value mapping is correct.The
map(|_| ())discards the initialization tuple since the actualDecoderInitResultis sent throughready_tx. The main thread only needs to verify that decoder initialization succeeded.
167-174: Theis_hardware_accelerated()method exists and is correctly called.The method is defined at
crates/video-decode/src/ffmpeg.rs:285and is properly accessible when called on theFFmpegDecoderinstance at line 167 ofcrates/rendering/src/decoder/ffmpeg.rs. No issues found.Likely an incorrect or invalid review comment.
crates/enc-mediafoundation/src/video/hevc.rs (3)
185-193: Verify the safety of the transmute operation.The
std::mem::transmutecall convertsIMFDXGIDeviceManagertousizeto pass it viaProcessMessage. While this pattern matches the Windows Media Foundation API requirements, transmute bypasses type safety.The code correctly clones the device manager before transmuting (Line 186), which ensures the COM reference count is maintained. However, verify that the transform properly manages the lifetime of this pointer and doesn't access it after the encoder is dropped.
Based on learnings, similar patterns exist in encoder implementations where Media Foundation APIs require passing COM interface pointers as usize parameters to ProcessMessage.
354-360: LGTM!The accessor methods are straightforward and provide appropriate read-only access to the encoder's configuration.
450-453: The 0.6 bitrate multiplier for HEVC is technically sound but undocumented.HEVC typically achieves 40-50% bitrate reduction compared to H.264, which aligns with the 0.6 multiplier applied in Line 452. However, this value appears as a magic number without explanation. Consider:
- Adding a named constant to document why HEVC uses 60% of the baseline bitrate
- Including a comment or referencing technical specifications for this compression ratio
- Verifying this value is intentional and tested against the H.264 encoder's calculations
| InputType(windows::core::Error), | ||
| } | ||
|
|
||
| unsafe impl Send for HevcEncoder {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find HevcEncoder struct definition and VideoProcessor type
rg -A 30 "struct HevcEncoder" crates/enc-mediafoundation/src/video/hevc.rsRepository: CapSoftware/Cap
Length of output: 1023
Remove or justify the unsafe impl Send with proper documentation.
The struct contains multiple windows crate COM interface types (ID3D11Device, IMFTransform, IMFMediaEventGenerator, etc.) that are !Send by default. The current unsafe impl Send is unsound unless:
- These specific COM types have been verified to implement Send (provide documentation/links), OR
- The struct is refactored to use thread-safe wrappers (Arc<Mutex<...>>, agile references), OR
- A detailed safety comment explains why the usage pattern is safe despite containing !Send types
As currently written, this violates Rust's Send safety contract.
🤖 Prompt for AI Agents
In crates/enc-mediafoundation/src/video/hevc.rs at line 75, the blanket `unsafe
impl Send for HevcEncoder {}` is unsound because the struct holds windows COM
interface types that are !Send; remove this unsafe impl or make the type
actually Send: either (A) remove the impl and keep the default, (B) wrap
non-Send COM fields in thread-safe wrappers (e.g., Arc<Mutex<_>> or cross-thread
agile references) so all fields are Send, or (C) if you can prove safety,
replace the impl with a precise, well-documented safety comment and include
links to authoritative docs showing each COM interface is Send and explain the
concurrency guarantees and invariants that make sending instances to other
threads safe. Ensure the code changes compile and add unit/compile-time tests
where possible to validate the new threading guarantees.
| let mut output = output.lock().unwrap(); | ||
|
|
||
| Ok(()) | ||
| }, | ||
| ) | ||
| .context("run native encoder") | ||
| if let Err(e) = muxer.write_sample(&output_sample, &mut output) { | ||
| tracing::error!("WriteSample failed: {e}"); | ||
| } |
There was a problem hiding this comment.
Mutex unwrap may panic; however, write_sample error handling is improved.
The write_sample error is now properly logged (addressing the past review), but line 288 still uses .unwrap() on the mutex lock. Consider handling the poisoned mutex case gracefully.
🔎 Suggested fix:
|output_sample| {
- let mut output = output.lock().unwrap();
+ let Ok(mut output) = output.lock() else {
+ tracing::error!("Failed to lock output mutex - poisoned");
+ return Ok(());
+ };
if let Err(e) = muxer.write_sample(&output_sample, &mut output) {
tracing::error!("WriteSample failed: {e}");
}
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 mut output = output.lock().unwrap(); | |
| Ok(()) | |
| }, | |
| ) | |
| .context("run native encoder") | |
| if let Err(e) = muxer.write_sample(&output_sample, &mut output) { | |
| tracing::error!("WriteSample failed: {e}"); | |
| } | |
| let Ok(mut output) = output.lock() else { | |
| tracing::error!("Failed to lock output mutex - poisoned"); | |
| return Ok(()); | |
| }; | |
| if let Err(e) = muxer.write_sample(&output_sample, &mut output) { | |
| tracing::error!("WriteSample failed: {e}"); | |
| } |
🤖 Prompt for AI Agents
In crates/recording/src/output_pipeline/win.rs around lines 288 to 292, the code
currently calls output.lock().unwrap() which can panic on a poisoned mutex;
replace the unwrap with a match (or if let) that handles both Ok and
Err(PoisonError) cases: attempt to lock, and on Ok(mut output) call
muxer.write_sample(&output_sample, &mut output) with the existing error logging,
while on Err(poison) recover the inner data via poison.into_inner() (or log the
poisoning and recover similarly) and then call write_sample on that recovered
inner value; also log that the mutex was poisoned to aid debugging rather than
panicking.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crates/frame-converter/src/d3d11.rs (1)
1145-1164: LGTM! Drop implementation correctly handles CloseHandle errors.The Drop implementation now properly checks the Result from
CloseHandle()and logs errors usingtracing::error!, addressing the previous review concern.crates/enc-mediafoundation/src/video/hevc.rs (1)
75-75: Remove or justify theunsafe impl Sendwith proper documentation.The struct contains multiple Windows COM interface types (
ID3D11Device,IMFTransform,IMFMediaEventGenerator, etc.) that are !Send by default. The currentunsafe impl Sendis unsound unless these specific COM types have been verified to implement Send or the struct is refactored to use thread-safe wrappers.
🧹 Nitpick comments (3)
.claude/agents/coderabbit-pr-reviewer.md (1)
1-6: Use YAML block scalar syntax for multiline description.Line 3 contains escaped
\ncharacters that may render as literal text (\n) rather than newlines in YAML. Use block scalar syntax for improved clarity and proper rendering.🔎 Proposed YAML formatting improvement
-description: Use this agent when you need to automatically implement CodeRabbit PR review suggestions from a GitHub pull request. This agent fetches review comments from the GitHub API, parses CodeRabbit's AI agent instructions, and systematically applies the suggested fixes while respecting project conventions.\n\nExamples:\n\n<example>... +description: | + Use this agent when you need to automatically implement CodeRabbit PR review suggestions from a GitHub pull request. This agent fetches review comments from the GitHub API, parses CodeRabbit's AI agent instructions, and systematically applies the suggested fixes while respecting project conventions. + + Examples: + + <example>...crates/enc-ffmpeg/src/video/hevc.rs (2)
15-20: Consider extracting theis_420helper to reduce duplication.This function is identical to the one in
crates/enc-ffmpeg/src/video/h264.rs(lines 14-19). Consider extracting it to a shared location (e.g., autilsmodule or the parentvideomodule) to follow DRY principles.
501-507: Consider extractingget_bitrateto reduce duplication.This function is identical to the one in
crates/enc-ffmpeg/src/video/h264.rs(lines 501-508). Consider extracting it to a shared location alongside theis_420helper to avoid code duplication.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.claude/agents/coderabbit-pr-reviewer.md(1 hunks).claude/settings.local.json(1 hunks)apps/desktop/src-tauri/src/general_settings.rs(3 hunks)crates/enc-ffmpeg/src/video/hevc.rs(1 hunks)crates/enc-mediafoundation/src/video/hevc.rs(1 hunks)crates/frame-converter/src/d3d11.rs(21 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .claude/settings.local.json
🧰 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/enc-mediafoundation/src/video/hevc.rsapps/desktop/src-tauri/src/general_settings.rscrates/frame-converter/src/d3d11.rscrates/enc-ffmpeg/src/video/hevc.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/enc-mediafoundation/src/video/hevc.rsapps/desktop/src-tauri/src/general_settings.rscrates/frame-converter/src/d3d11.rscrates/enc-ffmpeg/src/video/hevc.rs
🧠 Learnings (6)
📚 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/enc-mediafoundation/src/video/hevc.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 : Always handle Result/Option or types marked #[must_use]; never ignore them
Applied to files:
crates/frame-converter/src/d3d11.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:
crates/frame-converter/src/d3d11.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 use `dbg!()` macro in Rust code; use proper logging instead (Clippy: `dbg_macro` = deny)
Applied to files:
crates/frame-converter/src/d3d11.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 use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Applied to files:
crates/frame-converter/src/d3d11.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/frame-converter/src/d3d11.rs
🧬 Code graph analysis (2)
crates/enc-mediafoundation/src/video/hevc.rs (2)
crates/enc-mediafoundation/src/media.rs (2)
MFSetAttributeRatio(36-43)MFSetAttributeSize(26-33)crates/enc-mediafoundation/src/mft.rs (1)
enumerate_with_flags(42-74)
crates/enc-ffmpeg/src/video/hevc.rs (1)
crates/enc-ffmpeg/src/video/h264.rs (11)
is_420(15-20)new(52-60)with_preset(62-65)with_bpp(67-70)with_output_size(72-79)with_external_conversion(81-84)build(86-134)get_codec_and_options(425-500)build_with_codec(137-272)get_bitrate(502-509)get_encoder_priority(385-423)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
.claude/agents/coderabbit-pr-reviewer.md (1)
8-117: Well-structured and comprehensive agent specification.The document provides clear, detailed guidance for automating CodeRabbit suggestion implementation. The four-phase workflow is logical, the critical rules are appropriately strict (especially the NO COMMENTS enforcement), and error handling covers main failure scenarios. The GitHub API endpoint and bot username filter are correct.
apps/desktop/src-tauri/src/general_settings.rs (1)
75-75: LGTM: Helper function rename improves clarity.The rename from
true_btodefault_trueis more descriptive and follows better naming conventions.Also applies to: 79-79, 143-145
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
crates/enc-mediafoundation/src/video/hevc.rs (2)
430-430: Usingtracing::warn!as requested.The implementation now uses
tracing::warn!instead ofeprintln!for unknown media event types, which properly integrates with the application's logging infrastructure as previously requested.
75-75: Remove or justify theunsafe impl Sendwith proper documentation.This has already been flagged: the struct contains COM interface types (
ID3D11Device,IMFTransform,IMFMediaEventGenerator, etc.) that are !Send by default. The blanketunsafe impl Sendis unsound unless you can verify that these specific COM types are safe to send across threads or refactor to use thread-safe wrappers.crates/recording/src/output_pipeline/win.rs (1)
712-712: Mutex unwrap may panic on poisoned mutex.Line 712 uses
.unwrap()on the mutex lock, which will panic if the mutex becomes poisoned. The screen encoder path (lines 288-291) handles this gracefully with proper error recovery. Apply the same pattern here for consistency.🔎 Suggested fix
|output_sample| { - let mut output = output.lock().unwrap(); - if let Err(e) = muxer.write_sample(&output_sample, &mut output) - { + let Ok(mut output) = output.lock() else { + tracing::error!("Failed to lock output mutex - poisoned"); + return Ok(()); + }; + + if let Err(e) = muxer.write_sample(&output_sample, &mut output) { tracing::error!("Camera WriteSample failed: {e}"); } Ok(()) },
🧹 Nitpick comments (8)
crates/enc-mediafoundation/src/video/hevc.rs (1)
384-403: Consider clarifying the should_exit logic.The pattern of setting
should_exit = truethen conditionally resetting it tofalseworks correctly but is semantically unclear. The intent appears to be "exit the loop if we need input but cannot provide a frame."🔎 Clearer alternative
MediaFoundation::METransformNeedInput => { - should_exit = true; - if !should_stop.load(Ordering::SeqCst) - && let Some((texture, timestamp)) = get_frame()? - { + if should_stop.load(Ordering::SeqCst) { + should_exit = true; + } else if let Some((texture, timestamp)) = get_frame()? { self.video_processor.process_texture(&texture)?; let input_buffer = { MFCreateDXGISurfaceBuffer( &ID3D11Texture2D::IID, self.video_processor.output_texture(), 0, false, )? }; let mf_sample = MFCreateSample()?; mf_sample.AddBuffer(&input_buffer)?; mf_sample.SetSampleTime(timestamp.Duration)?; self.transform .ProcessInput(self.input_stream_id, &mf_sample, 0)?; - should_exit = false; + } else { + should_exit = true; } }crates/frame-converter/src/d3d11.rs (1)
217-264: Consider using Iterator combinators instead of unwrap.The GPU selection logic correctly prioritizes discrete hardware GPUs over integrated/software adapters. However, lines 244 and 261 use
.unwrap()after verifying the collection is non-empty. While safe, this pattern could be more idiomatic.🔎 Alternative using Iterator combinators
if !discrete_gpus.is_empty() { - let best = discrete_gpus + let best = *discrete_gpus .iter() .max_by_key(|g| g.dedicated_video_memory) - .unwrap(); + .expect("discrete_gpus is non-empty"); if discrete_gpus.len() > 1 || hardware_gpus.len() > 1 { tracing::info!( "Multi-GPU system detected ({} GPUs). Selected {} with {} MB VRAM for encoding.", gpus.len(), best.description, best.dedicated_video_memory / (1024 * 1024) ); } - return Some((*best).clone()); + return Some(best.clone()); } - let best = hardware_gpus + let best = *hardware_gpus .iter() .max_by_key(|g| g.dedicated_video_memory) - .unwrap(); + expect("hardware_gpus is non-empty"); Some((*best).clone())Alternatively, you could restructure to avoid the
.unwrap()entirely by usingcopied()or by combining the checks more directly.crates/scap-direct3d/src/lib.rs (4)
104-104: Consider handling mutex poisoning explicitly.The
.unwrap()will panic if the mutex is poisoned (another thread panicked while holding the lock). For better diagnostics and controlled failure, use.expect("staging pool mutex poisoned")or handle the poison error explicitly.🔎 Proposed fix
- let mut textures = self.textures.lock().unwrap(); + let mut textures = self.textures.lock().expect("staging pool mutex poisoned");
131-137: Simplify texture creation pattern.The pattern of using
OptionwithCreateTexture2Dfollowed by.unwrap()is indirect. Since the?operator on line 134 already propagates errors, the texture will always beSomeat line 137, making theunwrap()guaranteed safe but confusing.🔎 Clearer pattern
- let mut texture = None; - unsafe { - self.d3d_device - .CreateTexture2D(&texture_desc, None, Some(&mut texture))?; - }; - - let texture = texture.unwrap(); + let mut texture = None; + unsafe { + self.d3d_device + .CreateTexture2D(&texture_desc, None, Some(&mut texture))?; + }; + + let texture = texture.expect("CreateTexture2D returned None despite success");
344-348: Simplify context retrieval.The
Some(d3d_device).map(...).transpose()?.unwrap()pattern is unnecessarily complex. Sinced3d_deviceis always available at this point, directly callGetImmediateContextand handle theResult.🔎 Simplified approach
- let (d3d_device, d3d_context) = Some(d3d_device) - .map(|d| unsafe { d.GetImmediateContext() }.map(|v| (d, v))) - .transpose() - .map_err(NewCapturerError::Context)? - .unwrap(); + let d3d_context = unsafe { d3d_device.GetImmediateContext() } + .map_err(NewCapturerError::Context)?; + let (d3d_device, d3d_context) = (d3d_device, d3d_context);
366-369: Consider documenting the frame pool sizing formula.The formula
(fps / 30.0 * 2.0).ceil().clamp(2, 4)uses magic numbers without explanation. Documenting the rationale (e.g., "baseline of 2 buffers at 30 fps, scaled linearly") would aid future maintenance.crates/recording/src/output_pipeline/win_segmented_camera.rs (1)
584-586: Silent lock failure may hide issues.The code silently continues when the mutex lock fails without logging. Consider logging the failure to aid debugging, similar to the pattern used in the hardware encoder path.
🔎 Suggested improvement
- let Ok(mut output_guard) = output_clone.lock() else { - continue; - }; + let mut output_guard = match output_clone.lock() { + Ok(guard) => guard, + Err(e) => { + warn!("Failed to lock output mutex for software encoder: {e}"); + continue; + } + };crates/recording/src/output_pipeline/win.rs (1)
1035-1038: Consider making texture creation more explicit.The
.unwrap()on line 1038 assumes thatCreateTexture2Dalways sets the texture toSomewhen returningOk. While this is the Windows API contract, consider using a more explicit pattern for clarity.🔎 Suggested improvement
unsafe { let mut texture = None; device.CreateTexture2D(&texture_desc, Some(&subresource_data), Some(&mut texture))?; - Ok(texture.unwrap()) + texture.ok_or_else(|| windows::core::Error::from(windows::core::HRESULT(-1))) }
📜 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 (9)
.claude/settings.local.json(1 hunks)apps/desktop/src-tauri/src/general_settings.rs(2 hunks)crates/enc-mediafoundation/Cargo.toml(1 hunks)crates/enc-mediafoundation/src/video/hevc.rs(1 hunks)crates/frame-converter/src/d3d11.rs(21 hunks)crates/recording/src/output_pipeline/win.rs(9 hunks)crates/recording/src/output_pipeline/win_segmented_camera.rs(6 hunks)crates/rendering/src/decoder/mod.rs(5 hunks)crates/scap-direct3d/src/lib.rs(17 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/enc-mediafoundation/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/rendering/src/decoder/mod.rs
- apps/desktop/src-tauri/src/general_settings.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/recording/src/output_pipeline/win.rscrates/recording/src/output_pipeline/win_segmented_camera.rscrates/scap-direct3d/src/lib.rscrates/enc-mediafoundation/src/video/hevc.rscrates/frame-converter/src/d3d11.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/recording/src/output_pipeline/win.rscrates/recording/src/output_pipeline/win_segmented_camera.rscrates/scap-direct3d/src/lib.rscrates/enc-mediafoundation/src/video/hevc.rscrates/frame-converter/src/d3d11.rs
🧠 Learnings (8)
📚 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.rscrates/recording/src/output_pipeline/win_segmented_camera.rscrates/enc-mediafoundation/src/video/hevc.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/recording/src/output_pipeline/win.rscrates/frame-converter/src/d3d11.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:
crates/recording/src/output_pipeline/win.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.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 **/*.rs : Never use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Applied to files:
crates/enc-mediafoundation/src/video/hevc.rscrates/frame-converter/src/d3d11.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 use `dbg!()` macro in Rust code; use proper logging instead (Clippy: `dbg_macro` = deny)
Applied to files:
crates/enc-mediafoundation/src/video/hevc.rscrates/frame-converter/src/d3d11.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 : Always handle Result/Option or types marked #[must_use]; never ignore them
Applied to files:
crates/frame-converter/src/d3d11.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:
crates/frame-converter/src/d3d11.rs
🧬 Code graph analysis (4)
crates/recording/src/output_pipeline/win.rs (2)
crates/recording/src/output_pipeline/win_segmented_camera.rs (3)
new(66-71)new(97-103)default(163-169)crates/recording/src/capture_pipeline.rs (5)
new(34-38)default(27-29)output(115-115)output(119-119)output(122-123)
crates/recording/src/output_pipeline/win_segmented_camera.rs (1)
crates/recording/src/output_pipeline/win_segmented.rs (3)
new(69-74)new(97-103)duration_to_timespan(773-785)
crates/scap-direct3d/src/lib.rs (2)
crates/scap-direct3d/src/windows_version.rs (4)
std(61-61)detect(18-20)display_name(34-45)meets_minimum_requirements(22-24)crates/recording/src/capture_pipeline.rs (1)
create_d3d_device_with_type(347-370)
crates/enc-mediafoundation/src/video/hevc.rs (3)
crates/enc-mediafoundation/src/media.rs (2)
MFSetAttributeRatio(36-43)MFSetAttributeSize(26-33)crates/enc-mediafoundation/src/video/h264.rs (7)
new_with_scaled_output_with_flags(221-420)bitrate(496-498)calculate_bitrate(667-670)new(71-78)new(462-477)output_type(500-502)new_with_scaled_output(422-440)crates/enc-mediafoundation/src/mft.rs (1)
enumerate_with_flags(42-74)
⏰ 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 (21)
crates/enc-mediafoundation/src/video/hevc.rs (3)
78-278: LGTM: Constructor implementation is sound.The constructor correctly sets up the Media Foundation transform, handles stream ID negotiation including the E_NOTIMPL fallback, and properly configures input/output types. The transmute usage at line 190 follows the established pattern from h264.rs for passing the device manager to the transform.
280-352: LGTM: Convenience constructors provide clean API surface.The constructor variants appropriately delegate to the main implementation with different flags, providing hardware-accelerated and software fallback paths.
447-450: LGTM: Bitrate calculation is reasonable.The helper function applies a heuristic formula that accounts for resolution, frame rate, and a configurable multiplier. The frame rate factor adjustment and clamping are appropriate.
.claude/settings.local.json (1)
48-48: Verify the permission pattern matching behavior.This permission appears to target a specific PowerShell command for retrieving the Windows OS version, which aligns with the diagnostic features in this PR. However, ensure that the permission system's pattern matching behavior is exact and doesn't inadvertently allow variations or additional PowerShell commands beyond this specific diagnostic query.
crates/frame-converter/src/d3d11.rs (11)
4-11: LGTM: Import additions support new functionality.The additional imports (
OnceLockfor lazy initialization, atomics for timing,Instantfor measurements, and Windows handle types) are appropriate for the GPU detection, shared texture, and timing instrumentation features being added.
68-95: LGTM: GpuInfo extensions enhance GPU detection.The new fields (
adapter_index,is_software_adapter) and helper methods (is_basic_render_driver,is_warp,supports_hardware_encoding) provide clear semantics for multi-GPU systems and software renderer detection, which aligns with the PR's Windows optimization objectives.
111-215: LGTM: GPU enumeration with proper caching and error handling.The GPU detection functions use
OnceLockfor efficient lazy initialization and caching, which is appropriate since GPU configuration is static during execution. The enumeration logic properly handles DXGI factory creation failures and logs adapter details for diagnostics.
337-451: LGTM: Device creation with intelligent adapter selection.The
create_device_on_best_adaptermethod correctly iterates DXGI adapters, filters out software renderers (Microsoft Basic Render Driver, WARP), prioritizes discrete GPUs by VRAM, and creates the D3D11 device on the selected adapter. The use ofD3D_DRIVER_TYPE_UNKNOWNwith an explicit adapter is the correct pattern for multi-GPU systems.
453-602: LGTM: Converter initialization with shared texture support.The constructor properly integrates shared texture creation and logs the availability of shared handles, which is valuable for diagnosing encoding pipeline issues. The initialization sequence correctly establishes the GPU info, video device/context, processor, and textures.
608-658: LGTM: Well-designed accessor methods and format utilities.The new public methods provide clean access to shared handles, output texture, conversion timing, and format information. The
supported_input_formats()function returning a static slice is efficient, andis_format_supported()appropriately delegates to the existing format mapping logic.
660-850: LGTM: Non-invasive timing instrumentation for performance monitoring.The conversion timing logic using
InstantandAtomicU64is well-implemented and non-invasive. The periodic logging (every 300 frames) provides useful performance diagnostics without excessive output. The use ofOrdering::Relaxedfor statistics tracking is appropriate since exact ordering isn't critical for performance metrics.
852-873: LGTM: Format support extended for Windows encoding.The format mapping now includes BGRA, RGBA, and P010 formats, which aligns with the PR's Windows encoding optimizations. The
dxgi_format_namehelper (marked#[allow(dead_code)]) provides a useful utility for logging and diagnostics.
913-986: LGTM: Shared texture creation with NT handles and graceful fallback.The
create_shared_textureandextract_shared_handlefunctions properly create textures with NT-handle support (D3D11_RESOURCE_MISC_SHARED_NTHANDLE) for cross-process sharing. The fallback toNonewhen handle extraction fails (with appropriate logging) ensures the converter can still operate in staging mode.
988-1140: LGTM: Format-specific copy logic correctly implemented.The
copy_frame_to_mappedandcopy_mapped_to_framefunctions correctly handle all supported formats with appropriate bytes-per-pixel calculations:
- YUYV422: 2 bytes (16-bit packed YUV)
- BGRA/RGBA: 4 bytes (32-bit RGBA)
- P010LE: 2 bytes per component (10-bit in 16-bit container) with Y and UV planes
The use of
ptr::copy_nonoverlappingfor performance-critical memcpy operations is appropriate, and the stride-aware copying correctly handles texture alignment.
1145-1162: LGTM: Drop implementation properly handles shared handle cleanup.The
Dropimplementation correctly closes shared handles with proper error logging, addressing the previous review concern. The use of Rust's "let chains" syntax (stabilized in Rust 1.64) provides clean conditional logic that:
- Takes ownership with
.take()to prevent double-close- Validates the handle with
!handle.is_invalid()- Attempts
CloseHandleand logs any errorsThis approach is appropriate since errors cannot be propagated from
Drop, and logging provides valuable diagnostics for handle cleanup issues.crates/scap-direct3d/src/lib.rs (6)
293-322: LGTM: Robust version checking and capability detection.The Windows version and API capability checks provide clear error messages and prevent initialization on unsupported systems. The logging at lines 294-298 and 301-305 aids diagnostics.
500-504: LGTM: Appropriate logging for software fallback.The warning clearly informs users when hardware GPU acceleration is unavailable, aiding troubleshooting.
518-520: LGTM: Clean accessor for software rendering status.The method provides a clear way for callers to check if hardware acceleration is unavailable.
621-658: LGTM: Staging pool integration improves resource efficiency.The refactored
as_buffermethod uses the staging texture pool to avoid repeated allocations. The lifetime management is correct, withFrameBufferowning the staging texture andDropensuringUnmapis called (lines 671-676).
671-676: LGTM: Correct resource cleanup in Drop.The
Dropimplementation ensures the staging texture is properly unmapped, preventing resource leaks. SinceDropcannot propagate errors, silent failure onUnmapis acceptable here.
184-199: WARP fallback error handling is appropriate. WARP can be used as a software fallback for many kinds of applications, and DXGI_ERROR_UNSUPPORTED indicates the requested functionality is not supported by the device or driver. The code correctly triggers WARP fallback only on this error—other errors like E_INVALIDARG (feature-level mismatch) or E_FAIL (configuration issues) represent different failure categories that shouldn't fallback to WARP.
| "Bash(cargo clean:*)", | ||
| "Bash(cargo test:*)", | ||
| "Bash(powershell -Command \"[System.Environment]::OSVersion.Version.ToString()\")", | ||
| "Bash(cargo build:*)", | ||
| "Bash(gh api:*)", | ||
| "Bash(curl:*)", | ||
| "Bash(node -e:*)", | ||
| "Bash(findstr:*)" |
There was a problem hiding this comment.
Consider narrowing these broad permission patterns for better local development practice.
The permissions use wildcard patterns that accept any arguments (e.g., curl:*, node -e:*, gh api:*), which are overly permissive for a local development environment. Wildcards like Bash(*) can be overly permissive. While these settings are not checked in and useful for personal preferences and experimentation, narrowing these patterns would still be a good practice:
curl:*andnode -e:*(lines 51-52): Consider restricting to specific endpoints or use cases rather than allowing all argumentsgh api:*(line 50): Limit to read-only API operations if possiblecargo build:*andcargo test:*(lines 47, 49): These are reasonable for development but could exclude destructive variations if needed
This is optional for local development, but aligning with the principle of least privilege would make the configuration more maintainable long-term.
🤖 Prompt for AI Agents
In .claude/settings.local.json around lines 46 to 53 the Bash permission
patterns are overly broad (e.g., Bash(curl:*), Bash(node -e:*), Bash(gh api:*),
Bash(cargo test:*), Bash(cargo build:*)); narrow them by replacing wildcard
argument patterns with specific allowed commands or constrained argument
patterns — for curl and node -e restrict to explicit endpoints or scripts used
in local dev, for gh api limit to read-only endpoints or specific routes, and
for cargo build/test exclude destructive flags or limit to the project
workspace; update the lines to explicit, minimal patterns that follow
least-privilege practices.
A focus on enhanced system diagnostics, improved window sizing consistency, and a safer default for crash recovery. The most notable changes are the addition of a system diagnostics API (with UI integration), standardized window sizing (especially for Windows), and enabling crash recovery recording by default.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.