Add support for more pixel formats and GPU converters #1450
Conversation
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on January 8. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
|
Warning Rate limit exceeded@richiemcilroy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 9 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 ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughAdds GPU compute converters and WGSL shaders, broadens platform pixel-format support (macOS/Windows), adds macOS swscale fallback and Windows H.264 decoding, introduces BGRA→RGBA runtime conversions, tweaks MP4 format constant and local allowlist, and updates workspace dependencies. Changes
Sequence Diagram(s)Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
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: 4
🧹 Nitpick comments (5)
crates/rendering/src/decoder/frame_converter.rs (1)
93-110: LGTM! Correct BGRA→RGBA channel swapping.The pixel reordering logic correctly maps BGRA to RGBA. The structure mirrors
copy_rgba_planeappropriately.For potential performance improvement on large frames, consider using
extendwith an array instead of individualpushcalls:for pixel in row[..row_len].chunks_exact(4) { - frame_buffer.push(pixel[2]); - frame_buffer.push(pixel[1]); - frame_buffer.push(pixel[0]); - frame_buffer.push(pixel[3]); + frame_buffer.extend_from_slice(&[pixel[2], pixel[1], pixel[0], pixel[3]]); }crates/gpu-converters/src/yuyv.rs (1)
3-9: Consider validating input data size.The function doesn't validate that
data.len()matches the expected texture size (width * height * 2bytes for YUYV). If the caller passes mismatched data,create_texture_with_datamay panic or produce undefined behavior.pub fn create_input_texture( device: &wgpu::Device, queue: &wgpu::Queue, data: &[u8], width: u32, height: u32, ) -> wgpu::Texture { + let expected_size = (width * height * 2) as usize; + assert_eq!( + data.len(), + expected_size, + "YUYV data size mismatch: expected {}, got {}", + expected_size, + data.len() + ); device.create_texture_with_data(crates/gpu-converters/src/yuyv_rgba/mod.rs (1)
14-29: GPU initialization uses unwrap() without fallback.Both
request_adapterandrequest_deviceuse.unwrap(), which will panic if no GPU adapter is available (e.g., headless servers, CI environments). Consider returningResultorOptionfromnew()to allow callers to handle GPU unavailability gracefully.- pub async fn new() -> Self { + pub async fn new() -> Option<Self> { let instance = wgpu::Instance::new(&wgpu::InstanceDescriptor::default()); let adapter = instance .request_adapter(&wgpu::RequestAdapterOptions { power_preference: wgpu::PowerPreference::HighPerformance, force_fallback_adapter: false, compatible_surface: None, }) - .await - .unwrap(); + .await?; let (device, queue) = adapter .request_device(&wgpu::DeviceDescriptor::default()) - .await - .unwrap(); + .await + .ok()?; // ... rest unchanged ... - Self { + Some(Self { device, queue, pipeline, bind_group_layout, - } + }) }crates/gpu-converters/src/bgra_rgba/mod.rs (1)
13-28: GPU initialization uses unwrap() without fallback.Same issue as
YUYVToRGBA: bothrequest_adapterandrequest_deviceuse.unwrap(), which will panic if no GPU is available. Consider returningOption<Self>orResult<Self, _>to allow graceful degradation.crates/camera-windows/src/lib.rs (1)
533-535: DirectShow NV21 uses Media Foundation GUID constant.The DirectShow path uses
MF_VIDEO_FORMAT_NV21(a Media Foundation constant) for NV21 detection. While these GUIDs are typically identical across APIs, consider defining a separateMEDIASUBTYPE_NV21constant for clarity and to avoid potential confusion.+const MEDIASUBTYPE_NV21: GUID = GUID::from_u128(0x3132564e_0000_0010_8000_00aa00389b71); + // In DSPixelFormat::new: - t if t == MF_VIDEO_FORMAT_NV21 => PixelFormat::NV21, + t if t == MEDIASUBTYPE_NV21 => PixelFormat::NV21,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.claude/settings.local.json(1 hunks)crates/camera-ffmpeg/Cargo.toml(1 hunks)crates/camera-ffmpeg/src/macos.rs(5 hunks)crates/camera-ffmpeg/src/windows.rs(3 hunks)crates/camera-windows/src/lib.rs(4 hunks)crates/enc-ffmpeg/src/mux/mp4.rs(1 hunks)crates/gpu-converters/src/bgra_rgba/mod.rs(1 hunks)crates/gpu-converters/src/bgra_rgba/shader.wgsl(1 hunks)crates/gpu-converters/src/lib.rs(1 hunks)crates/gpu-converters/src/yuyv.rs(1 hunks)crates/gpu-converters/src/yuyv_nv12/mod.rs(1 hunks)crates/gpu-converters/src/yuyv_nv12/shader.wgsl(1 hunks)crates/gpu-converters/src/yuyv_rgba/mod.rs(1 hunks)crates/media-info/src/lib.rs(2 hunks)crates/rendering/src/decoder/avassetreader.rs(2 hunks)crates/rendering/src/decoder/frame_converter.rs(1 hunks)crates/video-decode/src/avassetreader.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Use duration.saturating_sub(other) instead of duration - other to avoid panics on underflow
Merge nested if statements: write 'if a && b { }' instead of 'if a { if b { } }'
Don't call .clone() on Copy types (integers, bools, etc.); copy them directly
Use function references directly: iter.map(foo) instead of iter.map(|x| foo(x))
Accept &[T] or &str instead of &Vec or &String in function parameters for flexibility
Use .is_empty() instead of .len() == 0 or .len() > 0 / .len() != 0
Don't assign () to a variable: write foo(); instead of let _ = foo(); or let x = foo(); when return is unit
Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Use 'for item in &collection' or 'for (i, item) in collection.iter().enumerate()' instead of 'for i in 0..collection.len()'
Use value.clamp(min, max) instead of manual if chains or .min(max).max(min) patterns
Always handle Result/Option or types marked #[must_use]; never ignore them
**/*.rs: Userustfmtand workspace clippy lints for Rust code formatting and linting
Use snake_case for Rust module names and kebab-case for crate names
Never usedbg!()macro in Rust code; use proper logging instead (Clippy:dbg_macro= deny)
Always handleResult/Optionor types marked#[must_use]; never ignore them (Rust compiler lint:unused_must_use= deny)
Never writelet _ = async_fn()which silently drops futures; await or explicitly handle them (Clippy:let_underscore_future= deny)
Usesaturating_subinstead of-forDurationto avoid panics (Clippy:unchecked_duration_subtraction= deny)
Merge nestedifstatements: useif a && b { }instead ofif a { if b { } }(Clippy:collapsible_if= deny)
Don't call.clone()onCopytypes; just copy them directly (Clippy:clone_on_copy= deny)
U...
Files:
crates/enc-ffmpeg/src/mux/mp4.rscrates/video-decode/src/avassetreader.rscrates/gpu-converters/src/yuyv_rgba/mod.rscrates/rendering/src/decoder/frame_converter.rscrates/rendering/src/decoder/avassetreader.rscrates/media-info/src/lib.rscrates/gpu-converters/src/yuyv_nv12/mod.rscrates/gpu-converters/src/bgra_rgba/mod.rscrates/gpu-converters/src/lib.rscrates/gpu-converters/src/yuyv.rscrates/camera-windows/src/lib.rscrates/camera-ffmpeg/src/windows.rscrates/camera-ffmpeg/src/macos.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-ffmpeg/src/mux/mp4.rscrates/video-decode/src/avassetreader.rscrates/gpu-converters/src/yuyv_rgba/mod.rscrates/rendering/src/decoder/frame_converter.rscrates/rendering/src/decoder/avassetreader.rscrates/media-info/src/lib.rscrates/gpu-converters/src/yuyv_nv12/mod.rscrates/gpu-converters/src/bgra_rgba/mod.rscrates/gpu-converters/src/lib.rscrates/gpu-converters/src/yuyv.rscrates/camera-windows/src/lib.rscrates/camera-ffmpeg/src/windows.rscrates/camera-ffmpeg/src/macos.rs
🧠 Learnings (2)
📚 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/enc-ffmpeg/src/mux/mp4.rs
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/enc-ffmpeg/src/mux/mp4.rscrates/camera-ffmpeg/src/macos.rs
🧬 Code graph analysis (5)
crates/video-decode/src/avassetreader.rs (1)
crates/rendering/src/decoder/mod.rs (1)
format(152-154)
crates/gpu-converters/src/yuyv_rgba/mod.rs (2)
crates/gpu-converters/src/util.rs (2)
copy_texture_to_buffer_command(17-55)read_buffer_to_vec(1-15)crates/gpu-converters/src/yuyv.rs (1)
create_input_texture(3-29)
crates/rendering/src/decoder/avassetreader.rs (1)
crates/rendering/src/decoder/frame_converter.rs (2)
copy_bgra_to_rgba(94-110)copy_rgba_plane(81-92)
crates/gpu-converters/src/yuyv_nv12/mod.rs (2)
crates/gpu-converters/src/util.rs (1)
read_buffer_to_vec(1-15)crates/gpu-converters/src/yuyv.rs (1)
create_input_texture(3-29)
crates/gpu-converters/src/bgra_rgba/mod.rs (1)
crates/gpu-converters/src/util.rs (2)
copy_texture_to_buffer_command(17-55)read_buffer_to_vec(1-15)
⏰ 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 (27)
crates/camera-ffmpeg/Cargo.toml (1)
10-10: LGTM!Adding the
tracingdependency is appropriate for improving observability and error diagnostics in the camera format handling code.crates/video-decode/src/avassetreader.rs (1)
162-178: LGTM! Symmetric BGRA mapping with reasonable fallback.The BGRA↔BGRA mapping is correctly symmetric. The fallback to RGBA for unknown formats is a reasonable graceful degradation, though unsupported formats will be silently converted which may produce incorrect colors. Consider whether a warning log would be appropriate for the fallback paths to aid debugging when unexpected formats are encountered.
crates/rendering/src/decoder/avassetreader.rs (2)
20-21: Import addition is appropriate.The new import for
copy_bgra_to_rgbais correctly scoped alongside the existingcopy_rgba_planeimport.
107-121: BGRA branch correctly mirrors RGBA handling with appropriate conversion.The implementation correctly extracts BGRA data and converts it to RGBA using the new utility function. The output format is correctly set to
PixelFormat::Rgbasince the conversion happens in-place. The structure appropriately mirrors the existing RGBA branch..claude/settings.local.json (1)
44-45: Clarify the relationship to PR objectives.This configuration change adds a web fetch permission for
gix.github.io, which appears unrelated to the PR's stated objectives of adding pixel format support and GPU converters.Please clarify:
- Why is this permission needed for the pixel format work?
- Is this an accidental inclusion from separate development work?
- Should this be in a separate PR for better change isolation?
crates/gpu-converters/src/lib.rs (1)
1-16: LGTM!The module declarations and public re-exports follow the established pattern of existing converters (UYVY variants). The API surface is consistent.
crates/gpu-converters/src/yuyv_rgba/mod.rs (1)
87-152: LGTM!The
convertmethod correctly creates input/output textures, dispatches the compute shader with appropriate workgroup counts for the halved YUYV texture width, and uses the shared utility functions for buffer readback.crates/gpu-converters/src/bgra_rgba/mod.rs (1)
86-168: LGTM!The
convertmethod correctly sets up input/output textures with appropriate formats (Bgra8Unorm input, Rgba8Unorm output), dispatches workgroups covering the full image dimensions, and performs buffer readback using shared utilities.crates/gpu-converters/src/bgra_rgba/shader.wgsl (1)
15-17: The swizzle logic is incorrect for sampled texture input.WebGPU automatically converts formats when reading and writing sampled textures, meaning a
texture_2d<f32>bound to a Bgra8Unorm texture is already swizzled to RGBA order by the hardware. The current swizzlevec4<f32>(bgra.b, bgra.g, bgra.r, bgra.a)reverses R and B channels again, producing BGRA output instead of RGBA.Remove the swizzle:
- let rgba = vec4<f32>(bgra.b, bgra.g, bgra.r, bgra.a); + let rgba = bgra;Likely an incorrect or invalid review comment.
crates/gpu-converters/src/yuyv_nv12/mod.rs (1)
184-184: Verify workgroup dispatch matches shader's thread-to-pixel mapping.The dispatch uses
(width / 2).div_ceil(8)for x-dimension, which corresponds to the YUYV texture width (since each YUYV texel contains 2 pixels). This appears correct given the shader processes one YUYV texel per invocation.crates/gpu-converters/src/yuyv_nv12/shader.wgsl (2)
31-36: UV subsampling logic is correct for NV12.The condition
(y & 1u) == 0ucorrectly processes only even rows for 4:2:0 chroma subsampling, and the interleaved U,V write pattern matches NV12 expectations.
10-14: Bounds check is correctly implemented.Using
textureDimensionsfor the bounds check rather than the uniform dimensions is correct since the texture has width/2 due to YUYV packing.crates/enc-ffmpeg/src/mux/mp4.rs (1)
83-85: LGTM!The format change from YUYV420 to Yuv420p is appropriate for MP4/H.264 encoding, as YUV420P is the standard planar format expected by most H.264 encoders.
crates/media-info/src/lib.rs (2)
205-220: LGTM!The expanded
RawVideoFormatenum provides comprehensive coverage for the new pixel formats. The variants are well-organized and follow consistent naming conventions.
240-249: LGTM!The pixel format mappings are correct. H264 mapping to YUV420P is appropriate since that's the typical output format from H264 decoders.
crates/camera-windows/src/lib.rs (3)
233-250: LGTM!The new
PixelFormatvariants are consistent with the broader format expansion in this PR and align with theRawVideoFormatadditions in the media-info crate.
479-484: LGTM!The Media Foundation format mappings correctly use the defined GUID constants and map to appropriate
PixelFormatvariants.
15-21: Verify GUID constant naming and values against Media Foundation documentation.The GUID constants have been reviewed against official sources. Most values are correctly based on their FOURCC/D3DFORMAT equivalents (L8=0x50, RGB565=0x17, NV21='NV21', P010='P010'). However, verify that
MF_VIDEO_FORMAT_L16with value0x00000051is the correct encoding for L16 format—the official D3DFORMAT documentation shows D3DFMT_A8L8 = 51, so confirmation is needed that this correctly represents L16 in Media Foundation. Additionally, ensure thatMEDIASUBTYPE_Y800follows the same GUID template as the MF formats (appears to use Y800='Y800' FOURCC).crates/camera-ffmpeg/src/windows.rs (3)
261-288: LGTM!The GRAY8 and GRAY16 handlers correctly handle stride differences and perform proper per-row copying with appropriate bytes-per-pixel calculations.
289-313: NV21 to NV12 conversion is correct.The UV byte swapping correctly converts NV21 (VU interleaved) to NV12 (UV interleaved) format by swapping adjacent chroma bytes.
314-351: LGTM!RGB565 and P010 handlers correctly handle their respective stride calculations and plane layouts. RGB565 applies vertical flip consistent with other RGB formats, and P010 properly handles both the 16-bit Y plane and UV plane.
crates/camera-ffmpeg/src/macos.rs (6)
25-50: LGTM!The
FourccInfostructure andfourcc_to_pixel_formatmapping provide a clean abstraction for handling additional formats via the swscale fallback path. The bytes_per_pixel values are correct for each format.
52-52: LGTM!The one-time warning mechanism using
AtomicBoolwithOrdering::Relaxedis appropriate for this use case where strict ordering isn't required.
198-220: LGTM!The RGBA format handler follows the established pattern and correctly handles stride differences during per-row copying.
244-266: LGTM!The "24RG" (RGB24) format handler correctly maps to
Pixel::RGB24and handles the 3 bytes-per-pixel stride calculation.
302-324: LGTM!The grayscale format handler correctly supports both "L008" and "GRAY" fourcc codes, mapping them to
Pixel::GRAY8with appropriate stride handling.
325-378: Well-designed swscale fallback mechanism.The fallback path provides graceful degradation for less common formats with:
- One-time performance warning
- Appropriate use of
FAST_BILINEARfor real-time capture- Comprehensive error handling with descriptive messages
| @group(0) @binding(1) var<storage, read_write> y_plane: array<u32>; | ||
| @group(0) @binding(2) var<storage, read_write> uv_plane: array<u32>; |
There was a problem hiding this comment.
Critical: Storage type mismatch - outputs should pack bytes into u32.
array<u32> means each element is 4 bytes, but Y and UV values are single bytes. The current code writes one byte-sized value per u32 element, wasting 3 bytes per write and requiring 4x more buffer space than allocated in Rust.
For correct NV12 output with byte-sized buffers, pack 4 bytes per u32:
-@group(0) @binding(1) var<storage, read_write> y_plane: array<u32>;
-@group(0) @binding(2) var<storage, read_write> uv_plane: array<u32>;
+@group(0) @binding(1) var<storage, read_write> y_plane: array<u32>; // packed bytes
+@group(0) @binding(2) var<storage, read_write> uv_plane: array<u32>; // packed bytesThen modify writes to pack values:
let y_index = (y * width + x * 2u) / 4u;
let y_offset = ((y * width + x * 2u) % 4u) * 8u;
// Use atomicOr or restructure to write 4 pixels at onceAlternatively, process 4 horizontal pixels per invocation and pack them into one u32 write.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/frame-converter/src/d3d11.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Use duration.saturating_sub(other) instead of duration - other to avoid panics on underflow
Merge nested if statements: write 'if a && b { }' instead of 'if a { if b { } }'
Don't call .clone() on Copy types (integers, bools, etc.); copy them directly
Use function references directly: iter.map(foo) instead of iter.map(|x| foo(x))
Accept &[T] or &str instead of &Vec or &String in function parameters for flexibility
Use .is_empty() instead of .len() == 0 or .len() > 0 / .len() != 0
Don't assign () to a variable: write foo(); instead of let _ = foo(); or let x = foo(); when return is unit
Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Use 'for item in &collection' or 'for (i, item) in collection.iter().enumerate()' instead of 'for i in 0..collection.len()'
Use value.clamp(min, max) instead of manual if chains or .min(max).max(min) patterns
Always handle Result/Option or types marked #[must_use]; never ignore them
**/*.rs: Userustfmtand workspace clippy lints for Rust code formatting and linting
Use snake_case for Rust module names and kebab-case for crate names
Never usedbg!()macro in Rust code; use proper logging instead (Clippy:dbg_macro= deny)
Always handleResult/Optionor types marked#[must_use]; never ignore them (Rust compiler lint:unused_must_use= deny)
Never writelet _ = async_fn()which silently drops futures; await or explicitly handle them (Clippy:let_underscore_future= deny)
Usesaturating_subinstead of-forDurationto avoid panics (Clippy:unchecked_duration_subtraction= deny)
Merge nestedifstatements: useif a && b { }instead ofif a { if b { } }(Clippy:collapsible_if= deny)
Don't call.clone()onCopytypes; just copy them directly (Clippy:clone_on_copy= deny)
U...
Files:
crates/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/frame-converter/src/d3d11.rs
⏰ 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 (2)
crates/frame-converter/src/d3d11.rs (2)
544-578: LGTM: Simplified copy operations.The removal of redundant
unsafeblocks is appropriate since the entire function is already markedunsafe. The copy logic for both NV12 and packed 4:2:2 formats remains correct.
580-615: LGTM: Simplified copy operations.The consolidation of YUYV422/UYVY422 handling and removal of redundant
unsafeblocks improves code clarity without changing functionality. The copy logic remains correct for both NV12 and packed 4:2:2 formats.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/recording/src/output_pipeline/win.rs (2)
724-738: LGTM: Conversion logic is correct.The byte-swapping logic correctly transforms UYVY422 (U₀Y₀V₀Y₁) to YUYV (Y₀U₀Y₁V₀) format, and bounds checking is properly implemented to prevent out-of-bounds access.
For high-resolution video at high frame rates, consider reusing a pre-allocated buffer instead of allocating a new Vec on every frame. This would reduce allocation overhead, though it adds complexity around buffer lifecycle management.
763-775: Consider restructuring for clarity.The current pattern of declaring
Option<Vec<u8>>, conditionally assigning it, then immediately unwrapping works correctly but is somewhat indirect.A clearer alternative would be:
- let original_data = &*lock; - - let converted_buffer: Option<Vec<u8>>; - let data: &[u8] = if frame.pixel_format == cap_camera_windows::PixelFormat::UYVY422 { - converted_buffer = Some(convert_uyvy_to_yuyv( - original_data, - frame.width, - frame.height, - )); - converted_buffer.as_ref().unwrap() - } else { - original_data - }; + let converted_buffer; + let data: &[u8] = if frame.pixel_format == cap_camera_windows::PixelFormat::UYVY422 { + converted_buffer = convert_uyvy_to_yuyv(&*lock, frame.width, frame.height); + &converted_buffer + } else { + &*lock + };This removes the Option wrapper and makes the data flow more direct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/frame-converter/src/d3d11.rs(1 hunks)crates/recording/src/output_pipeline/win.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/frame-converter/src/d3d11.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.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.rs
⏰ 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)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/camera-windows/src/lib.rs (1)
534-534: Consider defining a DirectShow-specific constant for clarity.Using the Media Foundation constant
MF_VIDEO_FORMAT_NV21in the DirectShow format detection context is confusing. While the underlying GUID value may be standardized across both APIs, mixing namespace-specific constants reduces code clarity.Consider defining a DirectShow-specific constant:
+const MEDIASUBTYPE_NV21: GUID = GUID::from_u128(0x3132564e_0000_0010_8000_00aa00389b71);Then update the mapping:
- t if t == MF_VIDEO_FORMAT_NV21 => PixelFormat::NV21, + t if t == MEDIASUBTYPE_NV21 => PixelFormat::NV21,
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/camera-windows/src/lib.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Use duration.saturating_sub(other) instead of duration - other to avoid panics on underflow
Merge nested if statements: write 'if a && b { }' instead of 'if a { if b { } }'
Don't call .clone() on Copy types (integers, bools, etc.); copy them directly
Use function references directly: iter.map(foo) instead of iter.map(|x| foo(x))
Accept &[T] or &str instead of &Vec or &String in function parameters for flexibility
Use .is_empty() instead of .len() == 0 or .len() > 0 / .len() != 0
Don't assign () to a variable: write foo(); instead of let _ = foo(); or let x = foo(); when return is unit
Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Use 'for item in &collection' or 'for (i, item) in collection.iter().enumerate()' instead of 'for i in 0..collection.len()'
Use value.clamp(min, max) instead of manual if chains or .min(max).max(min) patterns
Always handle Result/Option or types marked #[must_use]; never ignore them
**/*.rs: Userustfmtand workspace clippy lints for Rust code formatting and linting
Use snake_case for Rust module names and kebab-case for crate names
Never usedbg!()macro in Rust code; use proper logging instead (Clippy:dbg_macro= deny)
Always handleResult/Optionor types marked#[must_use]; never ignore them (Rust compiler lint:unused_must_use= deny)
Never writelet _ = async_fn()which silently drops futures; await or explicitly handle them (Clippy:let_underscore_future= deny)
Usesaturating_subinstead of-forDurationto avoid panics (Clippy:unchecked_duration_subtraction= deny)
Merge nestedifstatements: useif a && b { }instead ofif a { if b { } }(Clippy:collapsible_if= deny)
Don't call.clone()onCopytypes; just copy them directly (Clippy:clone_on_copy= deny)
U...
Files:
crates/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/camera-windows/src/lib.rs
⏰ 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 (4)
crates/camera-windows/src/lib.rs (4)
232-232: LGTM!Adding
PartialEqto thePixelFormatenum enables comparison operations, which is a sensible enhancement for working with pixel formats.
239-249: LGTM!The new pixel format variants appropriately expand camera format support for grayscale, additional YUV variants, RGB565, P010 (10-bit), and H.264 encoded streams.
479-484: LGTM!The Media Foundation format mappings follow the established pattern and correctly map the new GUID constants to their corresponding
PixelFormatvariants. The implementation is clean and consistent.Note: The correctness of these mappings depends on the GUID constant values being verified as flagged in the earlier comment.
15-21: GUID constants follow correct Media Foundation format.The constants use the standard DEFINE_MEDIATYPE_GUID pattern with FOURCC values in the first DWORD and the fixed suffix 0x0000_0010_8000_00aa00389b71. All formats (L8, L16, RGB565, P010) are officially supported Media Foundation formats. MEDIASUBTYPE_Y800 matches the standard DirectShow GUID value. No changes required.
| t if t == MEDIASUBTYPE_NV12 => PixelFormat::NV12, | ||
| t if t == MEDIASUBTYPE_MJPG => PixelFormat::MJPEG, | ||
| t if t == MEDIASUBTYPE_YV12 => PixelFormat::YV12, | ||
| t if t == MEDIASUBTYPE_Y800 || t == MEDIASUBTYPE_RGB8 => PixelFormat::GRAY8, |
There was a problem hiding this comment.
Clarify non-standard MEDIASUBTYPE_RGB8 grayscale handling.
MEDIASUBTYPE_RGB8 is officially an 8-bit palettized RGB format, but some camera manufacturers use it for black and white cameras. Document why the mapping to PixelFormat::GRAY8 assumes this non-standard behavior, and confirm target cameras actually produce grayscale data.
🤖 Prompt for AI Agents
In crates/camera-windows/src/lib.rs around line 533, the mapping of
MEDIASUBTYPE_RGB8 to PixelFormat::GRAY8 relies on a non-standard manufacturer
behavior; update the code by adding a clear inline comment explaining that
MEDIASUBTYPE_RGB8 is officially an 8-bit palettized RGB format but some target
cameras emit raw grayscale bytes so we map it to GRAY8 for those devices, cite
the relevant spec or note the vendor behavior, and add a runtime verification
and fallback: sample incoming frames or inspect the palette to detect whether
values are grayscale (R==G==B) and if not, log a warning and treat as palettized
RGB (or expose a configurable option to force GRAY8). Ensure the comment
documents the assumption and the runtime check to avoid silently misinterpreting
color frames.
| t if t == MEDIASUBTYPE_YV12 => PixelFormat::YV12, | ||
| t if t == MEDIASUBTYPE_Y800 || t == MEDIASUBTYPE_RGB8 => PixelFormat::GRAY8, | ||
| t if t == MF_VIDEO_FORMAT_NV21 => PixelFormat::NV21, | ||
| t if t == MEDIASUBTYPE_RGB565 => PixelFormat::RGB565, |
There was a problem hiding this comment.
Verify MEDIASUBTYPE_RGB565 constant availability.
Ensure that MEDIASUBTYPE_RGB565 is defined in the DirectShow API or imported dependencies. If this constant is not available, this code will fail to compile.
Run the following script to verify the constant is defined:
#!/bin/bash
# Search for MEDIASUBTYPE_RGB565 definition in dependencies
rg -n "MEDIASUBTYPE_RGB565" --type rust
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
crates/gpu-converters/src/yuyv_nv12/mod.rs (1)
122-124: Consider propagating the error instead of using.expect().Same pattern as in
yuyv_rgba: while validation should prevent failure, using.expect()introduces a panic path. Consider proper error propagation for robustness.
🧹 Nitpick comments (4)
crates/gpu-converters/src/yuyv_rgba/mod.rs (1)
104-106: Consider propagating the error instead of using.expect().While the validation above should prevent
create_input_texturefrom failing, using.expect()introduces a panic path. For consistency with the error-handling pattern elsewhere in this function, consider mapping the error:let yuyv_texture = yuyv::create_input_texture(&self.device, &self.queue, yuyv_data, width, height) - .expect("YUYV input validation passed above"); + .map_err(|e| ConvertError::BufferSizeMismatch { + expected: expected_size, + actual: yuyv_data.len(), + })?;Alternatively, add a dedicated
ConvertErrorvariant for texture creation failures. Based on learnings,Result/Optiontypes should always be handled properly.crates/gpu-converters/src/lib.rs (1)
26-63: Consider usingthiserrorforConvertErrorfor consistency.
GpuConverterErrorusesthiserror::Errorderive, butConvertErrorhas manualDisplayandErrorimplementations. Usingthiserrorfor both would reduce boilerplate and maintain consistency:-#[derive(Debug)] +#[derive(Debug, thiserror::Error)] pub enum ConvertError { + #[error("YUYV format requires even width, got {width}")] OddWidth { width: u32 }, + #[error("buffer size mismatch: expected {expected} bytes, got {actual}")] BufferSizeMismatch { expected: usize, actual: usize }, + #[error("GPU poll error: {0}")] Poll(wgpu::PollError), } - -impl std::fmt::Display for ConvertError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - Self::OddWidth { width } => { - write!(f, "YUYV format requires even width, got {width}") - } - Self::BufferSizeMismatch { expected, actual } => { - write!( - f, - "buffer size mismatch: expected {expected} bytes, got {actual}" - ) - } - Self::Poll(e) => write!(f, "GPU poll error: {e}"), - } - } -} - -impl std::error::Error for ConvertError { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - match self { - Self::Poll(e) => Some(e), - _ => None, - } - } -}The
From<wgpu::PollError>impl can remain as-is or use#[from]attribute on thePollvariant.crates/camera-ffmpeg/src/windows.rs (2)
182-205: Thread-local decoder caching is a sound design.Using thread-local storage for the H264 decoder is appropriate here, as it provides persistent decoder state for each thread while avoiding the need for explicit synchronization. The lazy initialization pattern is correct.
Minor: Line 194 could be more idiomatic:
- let decoder = decoder_opt.as_mut().unwrap(); - decoder - .decode(bytes)? + decoder_opt + .as_mut() + .unwrap() + .decode(bytes)? .ok_or(AsFFmpegError::H264NeedMoreData)Or use
expect()with a descriptive message since we know it's Some at this point.
429-453: NV21 conversion is correct but could be optimized.The implementation correctly converts NV21 (VU interleaved) to NV12 (UV interleaved) by swapping the chroma bytes. However, the byte-by-byte swap in the loop (lines 446-449) could be more efficient for large frames.
For better performance, consider using
chunks_exact_mut(2)or SIMD operations if this becomes a hot path:for y in 0..height / 2 { let row_width = width; let src_row = &src_uv[y * row_width..y * row_width + row_width]; let dest_row = &mut ff_frame.data_mut(1)[y * stride..y * stride + row_width]; for i in (0..row_width).step_by(2) { dest_row[i] = src_row[i + 1]; // V dest_row[i + 1] = src_row[i]; // U } }But the current implementation is clear and correct, so this optimization is optional.
📜 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 (7)
crates/camera-ffmpeg/src/windows.rs(4 hunks)crates/gpu-converters/Cargo.toml(1 hunks)crates/gpu-converters/src/lib.rs(1 hunks)crates/gpu-converters/src/yuyv.rs(1 hunks)crates/gpu-converters/src/yuyv_nv12/mod.rs(1 hunks)crates/gpu-converters/src/yuyv_rgba/mod.rs(1 hunks)crates/video-decode/src/avassetreader.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Use duration.saturating_sub(other) instead of duration - other to avoid panics on underflow
Merge nested if statements: write 'if a && b { }' instead of 'if a { if b { } }'
Don't call .clone() on Copy types (integers, bools, etc.); copy them directly
Use function references directly: iter.map(foo) instead of iter.map(|x| foo(x))
Accept &[T] or &str instead of &Vec or &String in function parameters for flexibility
Use .is_empty() instead of .len() == 0 or .len() > 0 / .len() != 0
Don't assign () to a variable: write foo(); instead of let _ = foo(); or let x = foo(); when return is unit
Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Use 'for item in &collection' or 'for (i, item) in collection.iter().enumerate()' instead of 'for i in 0..collection.len()'
Use value.clamp(min, max) instead of manual if chains or .min(max).max(min) patterns
Always handle Result/Option or types marked #[must_use]; never ignore them
**/*.rs: Userustfmtand workspace clippy lints for Rust code formatting and linting
Use snake_case for Rust module names and kebab-case for crate names
Never usedbg!()macro in Rust code; use proper logging instead (Clippy:dbg_macro= deny)
Always handleResult/Optionor types marked#[must_use]; never ignore them (Rust compiler lint:unused_must_use= deny)
Never writelet _ = async_fn()which silently drops futures; await or explicitly handle them (Clippy:let_underscore_future= deny)
Usesaturating_subinstead of-forDurationto avoid panics (Clippy:unchecked_duration_subtraction= deny)
Merge nestedifstatements: useif a && b { }instead ofif a { if b { } }(Clippy:collapsible_if= deny)
Don't call.clone()onCopytypes; just copy them directly (Clippy:clone_on_copy= deny)
U...
Files:
crates/gpu-converters/src/yuyv_rgba/mod.rscrates/video-decode/src/avassetreader.rscrates/gpu-converters/src/yuyv.rscrates/gpu-converters/src/lib.rscrates/gpu-converters/src/yuyv_nv12/mod.rscrates/camera-ffmpeg/src/windows.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/gpu-converters/src/yuyv_rgba/mod.rscrates/video-decode/src/avassetreader.rscrates/gpu-converters/src/yuyv.rscrates/gpu-converters/src/lib.rscrates/gpu-converters/src/yuyv_nv12/mod.rscrates/camera-ffmpeg/src/windows.rs
🧠 Learnings (5)
📚 Learning: 2025-12-07T14:29:40.743Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.743Z
Learning: Applies to **/*.rs : Use `.unwrap_or(val)` instead of `.unwrap_or_else(|| val)` for cheap values (Clippy: `unnecessary_lazy_evaluations` = deny)
Applied to files:
crates/gpu-converters/src/yuyv_nv12/mod.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/gpu-converters/src/yuyv_nv12/mod.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/gpu-converters/src/yuyv_nv12/mod.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/gpu-converters/src/yuyv_nv12/mod.rs
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/camera-ffmpeg/src/windows.rs
🧬 Code graph analysis (2)
crates/video-decode/src/avassetreader.rs (3)
crates/video-decode/src/ffmpeg.rs (2)
decoder(103-105)decoder(122-124)crates/camera-windows/src/lib.rs (1)
pixel_format(372-374)crates/rendering/src/decoder/mod.rs (1)
format(152-154)
crates/camera-ffmpeg/src/windows.rs (2)
crates/camera-windows/src/lib.rs (4)
new(467-493)new(519-544)width(360-362)height(364-366)crates/camera/src/lib.rs (2)
width(57-59)height(61-63)
⏰ 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 (14)
crates/gpu-converters/Cargo.toml (1)
7-7: Workspace dependency is correctly configured and actively used.The addition of
thiserror.workspace = trueis valid. Verification confirms:
- The workspace root defines
thiserror = "1.0"in[workspace.dependencies]- The gpu-converters crate correctly references it via workspace inheritance
- The dependency is actively used in
src/lib.rsline 18:#[derive(Debug, thiserror::Error)]No issues found.
crates/gpu-converters/src/yuyv.rs (1)
1-48: LGTM!The validation logic correctly enforces YUYV constraints (non-zero dimensions, even width, expected data length), and the texture creation properly uses
width / 2since YUYV packs two pixels into four bytes (matching the RGBA8Uint format). The function provides clear, informative error messages.crates/gpu-converters/src/yuyv_rgba/mod.rs (1)
14-84: LGTM!The async constructor properly returns
Result<Self, GpuConverterError>and uses the?operator for fallible GPU adapter and device requests, addressing the error-handling pattern correctly.crates/gpu-converters/src/lib.rs (1)
1-24: LGTM!The module organization is clean, with well-structured public re-exports and a properly defined
GpuConverterErrorenum usingthiserrorfor ergonomic error handling.crates/gpu-converters/src/yuyv_nv12/mod.rs (1)
12-102: LGTM!The async constructor properly returns
Result<Self, GpuConverterError>and uses the?operator for adapter and device requests, correctly addressing error handling requirements. This aligns with the pattern established across the other converters.crates/video-decode/src/avassetreader.rs (2)
41-41: LGTM! Proper error propagation.The use of the
?operator to propagate theResultfrompixel_to_pixel_formatis idiomatic and ensures that unsupported pixel formats are caught early during decoder initialization.
162-179: Excellent error handling for unsupported pixel formats.The explicit error handling with
tracing::error!and descriptive error messages is well-implemented. This approach ensures that unsupported formats are caught early and provides clear guidance on which formats are supported.crates/camera-ffmpeg/src/windows.rs (7)
17-20: Good addition of H264-specific error variants.The new error variants follow the existing pattern and appropriately distinguish between decode errors and the non-fatal "need more data" state that can occur with streaming H.264 decoding.
119-122: Verify that discarding buffered frames in reset() is intentional.The
reset()method recreates the decoder by callingSelf::new(), which discards any frames currently inframe_buffer. If these buffered frames are important, they should either be drained first or this behavior should be documented.If discarding frames is intentional (e.g., for stream boundary resets), consider adding a method-level doc comment explaining this behavior. Otherwise, modify reset to preserve or drain buffered frames.
47-174: Excellent - persistent decoder addresses previous review concern!The new
H264Decoderstruct maintains decoder state across frames (including keyframe tracking and frame buffering), which correctly resolves the critical issue flagged in the previous review where a new decoder was created per frame. This implementation properly handles:
- H.264 reference frame dependencies via persistent decoder state
- EAGAIN backpressure from FFmpeg
- Frame reordering via the internal buffer
- Keyframe detection to ensure proper stream start
401-428: LGTM - GRAY8 and GRAY16 implementations are correct.Both grayscale format conversions properly handle the pixel data layout. Using
GRAY16LEis appropriate for Windows little-endian architecture.
454-467: LGTM - RGB565 implementation correctly handles endianness and flip.The RGB565 conversion properly uses
RGB565LEfor little-endian and applies the vertical flip expected for Windows bitmap formats.
468-491: LGTM - P010 implementation correctly handles 10-bit YUV 4:2:0.The P010 format conversion properly handles both the Y plane and the interleaved UV plane with 2-byte samples (10-bit values in 16-bit containers).
492-492: LGTM - H264 format integration is clean.The H264 format now routes through the persistent decoder, completing the integration of H.264 support into the Windows camera pipeline.
| if !self.received_keyframe && !Self::contains_keyframe(bytes) { | ||
| return Ok(None); | ||
| } | ||
|
|
||
| if Self::contains_keyframe(bytes) { | ||
| self.received_keyframe = true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Eliminate redundant keyframe detection call.
The contains_keyframe(bytes) method is called twice when a keyframe hasn't been received yet. This performs the NAL unit parsing twice unnecessarily.
Apply this diff to check once:
- if !self.received_keyframe && !Self::contains_keyframe(bytes) {
- return Ok(None);
- }
-
- if Self::contains_keyframe(bytes) {
+ if !self.received_keyframe {
+ if !Self::contains_keyframe(bytes) {
+ return Ok(None);
+ }
self.received_keyframe = true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !self.received_keyframe && !Self::contains_keyframe(bytes) { | |
| return Ok(None); | |
| } | |
| if Self::contains_keyframe(bytes) { | |
| self.received_keyframe = true; | |
| } | |
| let has_keyframe = Self::contains_keyframe(bytes); | |
| if !self.received_keyframe && !has_keyframe { | |
| return Ok(None); | |
| } | |
| if !self.received_keyframe && has_keyframe { | |
| self.received_keyframe = true; | |
| } |
| if !self.received_keyframe && !Self::contains_keyframe(bytes) { | |
| return Ok(None); | |
| } | |
| if Self::contains_keyframe(bytes) { | |
| self.received_keyframe = true; | |
| } | |
| if !self.received_keyframe { | |
| if !Self::contains_keyframe(bytes) { | |
| return Ok(None); | |
| } else { | |
| self.received_keyframe = true; | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/camera-ffmpeg/src/windows.rs around lines 76 to 82, the code calls
Self::contains_keyframe(bytes) twice causing redundant NAL parsing; fix by
calling contains_keyframe(bytes) once, store the result in a local boolean
(e.g., let has_keyframe = Self::contains_keyframe(bytes)? or without ? depending
on return type), use that boolean in the early-return check and to set
self.received_keyframe = true if true, and remove the second contains_keyframe
call so parsing happens only once.
| let y_plane_size = (width * height) as u64; | ||
| let uv_plane_size = (width * height / 2) as u64; | ||
|
|
||
| let y_write_buffer = self.device.create_buffer(&wgpu::BufferDescriptor { | ||
| label: Some("YUYV to NV12 Y Plane Buffer"), | ||
| size: y_plane_size, | ||
| usage: wgpu::BufferUsages::STORAGE | wgpu::BufferUsages::COPY_SRC, | ||
| mapped_at_creation: false, | ||
| }); | ||
|
|
||
| let uv_write_buffer = self.device.create_buffer(&wgpu::BufferDescriptor { | ||
| label: Some("YUYV to NV12 UV Plane Buffer"), | ||
| size: uv_plane_size, | ||
| usage: wgpu::BufferUsages::STORAGE | wgpu::BufferUsages::COPY_SRC, | ||
| mapped_at_creation: false, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find crates/gpu-converters/src/yuyv_nv12 -name "*.wgsl" -type fRepository: CapSoftware/Cap
Length of output: 106
🏁 Script executed:
cat crates/gpu-converters/src/yuyv_nv12/shader.wgslRepository: CapSoftware/Cap
Length of output: 1031
Buffer sizes are 4x too small for u32 storage arrays.
The shader declares y_plane: array<u32> and uv_plane: array<u32>, where each u32 element is 4 bytes. The Rust code allocates only width * height and width * height / 2 bytes respectively, but the shader requires 4x this size. This will cause buffer overflow and data corruption.
Fix by either:
- Multiplying buffer sizes by 4:
y_plane_size = (width * height * 4) as u64anduv_plane_size = (width * height * 2) as u64 - Changing shader to use
array<u8>for byte-level storage
🤖 Prompt for AI Agents
In crates/gpu-converters/src/yuyv_nv12/mod.rs around lines 126 to 141, the
allocated buffer sizes are too small because the shader uses array<u32> (4 bytes
per element) but the Rust code uses width*height and width*height/2 bytes;
update the allocations so y_plane_size = (width * height * 4) as u64 and
uv_plane_size = (width * height / 2 * 4) as u64 (i.e., multiply both sizes by 4)
OR alternatively change the shader to use array<u8> and keep the current
byte-sized allocations; apply the chosen fix consistently for both
y_write_buffer and uv_write_buffer and update any related comments/labels.
| pub fn pixel_format_to_pixel(format: cv::PixelFormat) -> avformat::Pixel { | ||
| match format { | ||
| cv::PixelFormat::_420V => avformat::Pixel::NV12, | ||
| cv::PixelFormat::_32_RGBA => avformat::Pixel::RGBA, | ||
| _ => todo!(), | ||
| cv::PixelFormat::_32_BGRA => avformat::Pixel::BGRA, | ||
| _ => avformat::Pixel::RGBA, | ||
| } | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling: silent fallback masks unknown formats.
The wildcard pattern _ => avformat::Pixel::RGBA silently converts any unknown cv::PixelFormat to RGBA without logging or error reporting. This is inconsistent with pixel_to_pixel_format (lines 168-177), which explicitly errors on unsupported formats with tracing and a descriptive message.
This silent fallback could mask bugs when new formats are added, especially problematic given this PR's goal of expanding format support. Consider one of these approaches:
- Preferred: Return
Result<avformat::Pixel, String>and error on unknown formats for consistency - Alternative: Add
tracing::warn!when the fallback is used to at least log the occurrence
Option 1 (Preferred): Return Result for consistency
-pub fn pixel_format_to_pixel(format: cv::PixelFormat) -> avformat::Pixel {
+pub fn pixel_format_to_pixel(format: cv::PixelFormat) -> Result<avformat::Pixel, String> {
match format {
- cv::PixelFormat::_420V => avformat::Pixel::NV12,
- cv::PixelFormat::_32_RGBA => avformat::Pixel::RGBA,
- cv::PixelFormat::_32_BGRA => avformat::Pixel::BGRA,
- _ => avformat::Pixel::RGBA,
+ cv::PixelFormat::_420V => Ok(avformat::Pixel::NV12),
+ cv::PixelFormat::_32_RGBA => Ok(avformat::Pixel::RGBA),
+ cv::PixelFormat::_32_BGRA => Ok(avformat::Pixel::BGRA),
+ other => {
+ tracing::error!(
+ cv_pixel_format = ?other,
+ "Unhandled cv::PixelFormat - no mapping to avformat::Pixel available"
+ );
+ Err(format!(
+ "Unsupported cv::PixelFormat: {:?}. Supported formats: _420V, _32_RGBA, _32_BGRA",
+ other
+ ))
+ }
}
}Option 2 (Alternative): Add warning logging
pub fn pixel_format_to_pixel(format: cv::PixelFormat) -> avformat::Pixel {
match format {
cv::PixelFormat::_420V => avformat::Pixel::NV12,
cv::PixelFormat::_32_RGBA => avformat::Pixel::RGBA,
cv::PixelFormat::_32_BGRA => avformat::Pixel::BGRA,
- _ => avformat::Pixel::RGBA,
+ other => {
+ tracing::warn!(
+ cv_pixel_format = ?other,
+ "Unknown cv::PixelFormat, falling back to RGBA"
+ );
+ avformat::Pixel::RGBA
+ }
}
}🤖 Prompt for AI Agents
In crates/video-decode/src/avassetreader.rs around lines 181-188, change
pixel_format_to_pixel to return Result<avformat::Pixel, String> (matching
pixel_to_pixel_format) instead of silently falling back to RGBA; match the known
cv::PixelFormat variants and return Ok(avformat::Pixel::... ) for each, and for
the wildcard arm log the issue with tracing::error! (include the unknown format
value) and return Err with a descriptive message (e.g. "unsupported
cv::PixelFormat: ...") so callers can handle unsupported formats consistently.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
crates/gpu-converters/src/yuyv_nv12/mod.rs (1)
126-141: Buffer sizes may be incorrect for shader storage array type.The buffer sizes are calculated as raw byte counts for NV12 planes (
width * heightfor Y,width * height / 2for UV). This is correct for the output data format, but if the shader declaresy_plane: array<u32>anduv_plane: array<u32>, GPU storage buffer alignment requirements may cause issues.Please verify the shader at
shader.wgsluses byte-level writes compatible with these buffer sizes:#!/bin/bash cat crates/gpu-converters/src/yuyv_nv12/shader.wgslcrates/camera-ffmpeg/src/windows.rs (2)
76-82: Eliminate redundant keyframe detection call.The
contains_keyframe(bytes)method is called twice when a keyframe hasn't been received yet, performing NAL unit parsing twice unnecessarily.Apply this diff to check once:
- if !self.received_keyframe && !Self::contains_keyframe(bytes) { - return Ok(None); - } - - if Self::contains_keyframe(bytes) { + let has_keyframe = Self::contains_keyframe(bytes); + + if !self.received_keyframe { + if !has_keyframe { + return Ok(None); + } self.received_keyframe = true; }
176-180: Default implementation can panic - use a fallible constructor pattern instead.The
Defaulttrait implementation usesexpect(), which will panic at runtime if the H264 codec is not available.Defaultimplementations should not panic.Apply this diff to remove the problematic Default implementation:
-impl Default for H264Decoder { - fn default() -> Self { - Self::new().expect("Failed to create H264Decoder") - } -}Then update any call sites that rely on
H264Decoder::default()to callH264Decoder::new()and handle theResult.
🧹 Nitpick comments (2)
crates/gpu-converters/src/yuyv_rgba/mod.rs (1)
96-96: Consider using checked arithmetic for buffer size calculation.While unlikely in practice for valid texture dimensions, the multiplication could theoretically overflow on 32-bit systems or with extremely large dimensions.
- let expected_size = (width as usize) * (height as usize) * 2; + let expected_size = (width as usize) + .checked_mul(height as usize) + .and_then(|v| v.checked_mul(2)) + .ok_or_else(|| ConvertError::BufferSizeMismatch { + expected: usize::MAX, + actual: yuyv_data.len(), + })?;crates/camera-ffmpeg/src/windows.rs (1)
401-491: Consider adding buffer size validation for pixel format conversions.The pixel format conversion code assumes the input buffer has the correct size for the given format and dimensions. If the camera provides a malformed frame with an undersized buffer, array indexing will panic at runtime.
While panicking on malformed frames may be acceptable, explicit size validation with descriptive errors would improve debuggability:
// Example for GRAY8: let expected_size = width * height; if bytes.len() < expected_size { return Err(AsFFmpegError::FailedToGetBytes( windows_core::Error::from_hresult(windows_core::HRESULT(-1)) )); }This pattern could be applied to each format handler for more graceful error handling.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/camera-ffmpeg/src/windows.rs(4 hunks)crates/gpu-converters/src/yuyv.rs(1 hunks)crates/gpu-converters/src/yuyv_nv12/mod.rs(1 hunks)crates/gpu-converters/src/yuyv_rgba/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Use duration.saturating_sub(other) instead of duration - other to avoid panics on underflow
Merge nested if statements: write 'if a && b { }' instead of 'if a { if b { } }'
Don't call .clone() on Copy types (integers, bools, etc.); copy them directly
Use function references directly: iter.map(foo) instead of iter.map(|x| foo(x))
Accept &[T] or &str instead of &Vec or &String in function parameters for flexibility
Use .is_empty() instead of .len() == 0 or .len() > 0 / .len() != 0
Don't assign () to a variable: write foo(); instead of let _ = foo(); or let x = foo(); when return is unit
Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Use 'for item in &collection' or 'for (i, item) in collection.iter().enumerate()' instead of 'for i in 0..collection.len()'
Use value.clamp(min, max) instead of manual if chains or .min(max).max(min) patterns
Always handle Result/Option or types marked #[must_use]; never ignore them
**/*.rs: Userustfmtand workspace clippy lints for Rust code formatting and linting
Use snake_case for Rust module names and kebab-case for crate names
Never usedbg!()macro in Rust code; use proper logging instead (Clippy:dbg_macro= deny)
Always handleResult/Optionor types marked#[must_use]; never ignore them (Rust compiler lint:unused_must_use= deny)
Never writelet _ = async_fn()which silently drops futures; await or explicitly handle them (Clippy:let_underscore_future= deny)
Usesaturating_subinstead of-forDurationto avoid panics (Clippy:unchecked_duration_subtraction= deny)
Merge nestedifstatements: useif a && b { }instead ofif a { if b { } }(Clippy:collapsible_if= deny)
Don't call.clone()onCopytypes; just copy them directly (Clippy:clone_on_copy= deny)
U...
Files:
crates/gpu-converters/src/yuyv.rscrates/gpu-converters/src/yuyv_nv12/mod.rscrates/camera-ffmpeg/src/windows.rscrates/gpu-converters/src/yuyv_rgba/mod.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/gpu-converters/src/yuyv.rscrates/gpu-converters/src/yuyv_nv12/mod.rscrates/camera-ffmpeg/src/windows.rscrates/gpu-converters/src/yuyv_rgba/mod.rs
🧠 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 `.unwrap_or(val)` instead of `.unwrap_or_else(|| val)` for cheap values (Clippy: `unnecessary_lazy_evaluations` = deny)
Applied to files:
crates/gpu-converters/src/yuyv_nv12/mod.rs
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/camera-ffmpeg/src/windows.rs
📚 Learning: 2025-12-07T14:29:19.180Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T14:29:19.180Z
Learning: Applies to **/*.rs : Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Applied to files:
crates/camera-ffmpeg/src/windows.rs
🧬 Code graph analysis (2)
crates/gpu-converters/src/yuyv_nv12/mod.rs (2)
crates/gpu-converters/src/util.rs (1)
read_buffer_to_vec(1-15)crates/gpu-converters/src/yuyv.rs (1)
create_input_texture(3-48)
crates/gpu-converters/src/yuyv_rgba/mod.rs (2)
crates/gpu-converters/src/util.rs (2)
copy_texture_to_buffer_command(17-55)read_buffer_to_vec(1-15)crates/gpu-converters/src/yuyv.rs (1)
create_input_texture(3-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
crates/gpu-converters/src/yuyv_nv12/mod.rs (3)
1-10: LGTM!The imports are appropriate and the struct fields correctly capture the GPU context needed for the compute pipeline.
12-102: LGTM!The constructor now properly returns
Result<Self, GpuConverterError>and uses the?operator for fallible operations, addressing the previous review feedback. The GPU context initialization and pipeline setup are correct.
182-227: LGTM!The compute pass dispatch, buffer copy operations, and readback logic are correctly implemented. Error handling properly propagates
PollErrorthrough theConvertError::Pollvariant.crates/gpu-converters/src/yuyv.rs (1)
3-48: LGTM!The validation logic is comprehensive and correct for YUYV format constraints. The texture width is appropriately halved since YUYV packs two pixels into four bytes (Y₀U Y₁V), and storing this as Rgba8Uint treats each macropixel as a single texel. Error messages clearly explain validation failures.
crates/gpu-converters/src/yuyv_rgba/mod.rs (2)
15-84: LGTM!The WGPU initialization follows standard patterns with appropriate error propagation. The bind group layout correctly defines the input texture (Rgba8Uint for packed YUYV data) and output storage texture (Rgba8Unorm for final RGBA).
108-164: LGTM!The conversion logic properly sets up the compute pipeline, dispatches workgroups with correct dimensions (accounting for the 8×8 workgroup size), and handles the texture-to-buffer copy and readback. Error propagation is appropriate throughout.
crates/camera-ffmpeg/src/windows.rs (2)
492-492: The callers ofas_ffmpeg()handleH264NeedMoreDataappropriately in production code. The main call site incrates/recording/src/feeds/camera.rsuseslet Ok(...) else { return; }which gracefully handles this non-fatal error by skipping the frame, rather than panicking or ignoring it. The error type itself is documented as "(non-fatal)" in the enum definition, and error propagation patterns across the codebase are consistent.
182-199: The thread-local H264 decoder is correct and intentional.Camera frames arrive on a dedicated capture thread via callback, and
as_ffmpeg()is invoked directly in that callback. This means all frames from the same camera stream are processed sequentially on a single thread and share the same thread-local decoder instance, properly maintaining stateful H264 decoding state (reference frames, SPS/PPS context). Multiple cameras run on different threads with independent decoder instances, preventing data races. No changes needed.
| if !width.is_multiple_of(2) { | ||
| return Err(ConvertError::OddWidth { width }); | ||
| } | ||
|
|
||
| let expected_size = (width as usize) * (height as usize) * 2; | ||
| if yuyv_data.len() != expected_size { | ||
| return Err(ConvertError::BufferSizeMismatch { | ||
| expected: expected_size, | ||
| actual: yuyv_data.len(), | ||
| }); | ||
| } | ||
|
|
||
| let yuyv_texture = | ||
| yuyv::create_input_texture(&self.device, &self.queue, yuyv_data, width, height) | ||
| .expect("YUYV input validation passed above"); |
There was a problem hiding this comment.
Missing validation for zero dimensions will cause a panic.
The yuyv::create_input_texture function validates that width and height are non-zero, but this code only validates that width is even. If width == 0 or height == 0, the .expect() will panic.
if !width.is_multiple_of(2) {
return Err(ConvertError::OddWidth { width });
}
+
+ if width == 0 || height == 0 {
+ return Err(ConvertError::ZeroDimension { width, height });
+ }
let expected_size = (width as usize) * (height as usize) * 2;This requires adding a ZeroDimension variant to ConvertError in lib.rs, or alternatively changing the .expect() to proper error handling with ? or .map_err().
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/gpu-converters/src/yuyv_nv12/mod.rs around lines 110–124, add
validation to return an error when width == 0 or height == 0 before computing
expected_size and calling yuyv::create_input_texture; update lib.rs to add a
ConvertError::ZeroDimension { width: u32, height: u32 } variant (or similar) and
return that variant here, and remove the .expect() by propagating errors from
create_input_texture with ? or map_err to a ConvertError if create_input_texture
can fail for other reasons.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crates/camera-ffmpeg/src/windows.rs (1)
76-82: Eliminate redundant keyframe detection call.The
contains_keyframe(bytes)method is called twice when a keyframe hasn't been received yet, performing NAL unit parsing twice unnecessarily.Apply this diff to check once:
- if !self.received_keyframe && !Self::contains_keyframe(bytes) { - return Ok(None); - } - - if Self::contains_keyframe(bytes) { + let has_keyframe = Self::contains_keyframe(bytes); + + if !self.received_keyframe { + if !has_keyframe { + return Ok(None); + } self.received_keyframe = true; }crates/gpu-converters/src/yuyv_nv12/mod.rs (1)
126-143: Multiply buffer sizes by 4 to match shader's u32 array declarations.The shader declares
y_plane: array<u32>anduv_plane: array<u32>. Since each u32 is 4 bytes, the buffer sizes must account for this. Currently,y_plane_size = width * heightanduv_plane_size = width * height / 2allocate bytes, but should be multiplied by 4 to allocate for u32 elements. Update lines 128-129:- let y_plane_size = width_u64 * height_u64; - let uv_plane_size = (width_u64 * height_u64) / 2; + let y_plane_size = width_u64 * height_u64 * 4; + let uv_plane_size = (width_u64 * height_u64) / 2 * 4;
🧹 Nitpick comments (3)
crates/camera-ffmpeg/src/windows.rs (2)
119-122: Consider optimizing reset by flushing instead of recreating the decoder.Creating a new decoder context is expensive. Consider flushing the existing decoder and resetting state flags instead:
pub fn reset(&mut self) -> Result<(), AsFFmpegError> { - *self = Self::new()?; + let _ = self.flush(); + self.received_keyframe = false; Ok(()) }
423-447: Consider optimizing NV21→NV12 conversion.The manual byte-swapping loop is correct but could be more efficient using chunk-based swapping:
for y in 0..height / 2 { let row_width = width; let src_row = &src_uv[y * row_width..]; let dest_row = &mut ff_frame.data_mut(1)[y * stride..]; - for x in 0..width / 2 { - dest_row[x * 2] = src_row[x * 2 + 1]; - dest_row[x * 2 + 1] = src_row[x * 2]; + for x in 0..width / 2 { + dest_row[x * 2..x * 2 + 2].copy_from_slice(&[src_row[x * 2 + 1], src_row[x * 2]]); } }Or use
chunks_exact(2)for better vectorization:for y in 0..height / 2 { let row_width = width; let src_row = &src_uv[y * row_width..]; let dest_row = &mut ff_frame.data_mut(1)[y * stride..]; - for x in 0..width / 2 { - dest_row[x * 2] = src_row[x * 2 + 1]; - dest_row[x * 2 + 1] = src_row[x * 2]; + for (i, chunk) in src_row[..width].chunks_exact(2).enumerate() { + dest_row[i * 2] = chunk[1]; + dest_row[i * 2 + 1] = chunk[0]; } }crates/gpu-converters/src/yuyv_rgba/mod.rs (1)
92-106: Zero-dimension validation is now handled via texture creation error path.The
.map_err(ConvertError::TextureCreation)on line 106 addresses the previous review concern -yuyv::create_input_texturevalidates zero dimensions and returns an error that's now properly propagated instead of panicking.For more explicit error messages, consider adding upfront validation:
+ if width == 0 || height == 0 { + return Err(ConvertError::BufferSizeMismatch { + expected: 1, + actual: 0, + }); + } + if !width.is_multiple_of(2) {This is optional since the current code is functionally correct.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
crates/camera-ffmpeg/src/windows.rs(4 hunks)crates/camera-windows/src/lib.rs(4 hunks)crates/gpu-converters/src/bgra_rgba/mod.rs(1 hunks)crates/gpu-converters/src/lib.rs(1 hunks)crates/gpu-converters/src/yuyv.rs(1 hunks)crates/gpu-converters/src/yuyv_nv12/mod.rs(1 hunks)crates/gpu-converters/src/yuyv_rgba/mod.rs(1 hunks)crates/media-info/src/lib.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/camera-windows/src/lib.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/gpu-converters/src/yuyv.rscrates/gpu-converters/src/yuyv_nv12/mod.rscrates/media-info/src/lib.rscrates/gpu-converters/src/yuyv_rgba/mod.rscrates/gpu-converters/src/bgra_rgba/mod.rscrates/camera-ffmpeg/src/windows.rscrates/gpu-converters/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/gpu-converters/src/yuyv.rscrates/gpu-converters/src/yuyv_nv12/mod.rscrates/media-info/src/lib.rscrates/gpu-converters/src/yuyv_rgba/mod.rscrates/gpu-converters/src/bgra_rgba/mod.rscrates/camera-ffmpeg/src/windows.rscrates/gpu-converters/src/lib.rs
🧠 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 `.unwrap_or(val)` instead of `.unwrap_or_else(|| val)` for cheap values (Clippy: `unnecessary_lazy_evaluations` = deny)
Applied to files:
crates/gpu-converters/src/yuyv_nv12/mod.rs
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/camera-ffmpeg/src/windows.rs
📚 Learning: 2025-12-07T14:29:19.180Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T14:29:19.180Z
Learning: Applies to **/*.rs : Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Applied to files:
crates/camera-ffmpeg/src/windows.rs
🧬 Code graph analysis (1)
crates/gpu-converters/src/bgra_rgba/mod.rs (1)
crates/gpu-converters/src/util.rs (2)
copy_texture_to_buffer_command(17-55)read_buffer_to_vec(1-15)
⏰ 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 (17)
crates/media-info/src/lib.rs (2)
211-218: LGTM! Enum variants properly extend format support.The new
RawVideoFormatvariants expand the range of supported pixel formats, which aligns with the PR objectives to improve compatibility with more video sources.
239-246: No issues identified. All FFmpegPixelformat constants are valid and available in theffmpeg-nextcrate being used. The mappings fromRawVideoFormatvariants to their correspondingPixeltypes are correct and consistent with existing usage in the codebase (e.g.,Pixel::P010LEis already successfully used in camera-ffmpeg).crates/camera-ffmpeg/src/windows.rs (7)
1-2: LGTM!The new imports are necessary for the thread-local H264 decoder state management and frame buffering.
17-20: LGTM!The new error variants appropriately distinguish between fatal decode errors and the non-fatal "need more data" condition for incremental H264 decoding.
395-408: LGTM!GRAY8 pixel format handling is correct. Single-plane 8-bit grayscale with proper stride handling.
409-422: LGTM!GRAY16 pixel format handling is correct. Single-plane 16-bit little-endian grayscale with proper stride handling.
448-461: LGTM!RGB565 pixel format handling is correct. Properly handles little-endian 16-bit RGB with vertical flip.
462-485: LGTM!P010 (10-bit 4:2:0 YUV) pixel format handling is correct. Properly handles both Y and interleaved UV planes with 2-byte samples.
486-486: H264 integration is correct.The integration properly delegates to
decode_h264()which uses the thread-local decoder. Note the thread-local sharing concern raised in the earlier comment about lines 180-193.crates/gpu-converters/src/yuyv_rgba/mod.rs (1)
14-84: Well-structured GPU initialization with proper error handling.The async constructor properly propagates errors using
?with theGpuConverterErrortype, and the GPU pipeline setup follows wgpu best practices.crates/gpu-converters/src/yuyv.rs (2)
3-32: Thorough input validation with overflow protection.Good defensive programming: validates zero dimensions, even width requirement, and uses
checked_multo prevent overflow on large dimensions. This provides the safety net that callers rely on.
34-52: Correct YUYV texture packing.The texture width is halved because each YUYV macro-pixel (4 bytes encoding 2 pixels) maps to one
Rgba8Uinttexel. The shader can access Y0/U/Y1/V as the R/G/B/A channels.crates/gpu-converters/src/bgra_rgba/mod.rs (1)
15-85: Constructor follows the established pattern.The async initialization with proper error propagation via
GpuConverterErroris consistent with other converters.crates/gpu-converters/src/yuyv_nv12/mod.rs (2)
110-124: Validation follows established pattern.Consistent with
YUYVToRGBA, zero dimensions are caught byyuyv::create_input_textureand properly mapped toConvertError::TextureCreation.
12-102: Constructor and pipeline setup look correct.Properly handles GPU initialization errors and sets up the compute pipeline with appropriate bind group layout for texture input, two storage buffers (Y and UV planes), and a uniform dimensions buffer.
crates/gpu-converters/src/lib.rs (2)
18-36: Well-designed error types using thiserror.Good use of
#[from]for automatic conversions from wgpu errors. TheTextureCreation(String)variant provides flexibility for various texture creation failures.
1-16: Clean module organization and public API.The new converters are properly organized into separate modules and re-exported for public use. Naming is consistent across all converter types.
| fn decode_h264(bytes: &[u8]) -> Result<FFVideo, AsFFmpegError> { | ||
| H264_DECODER.with(|decoder_cell| { | ||
| let mut decoder_opt = decoder_cell.borrow_mut(); | ||
|
|
||
| if decoder_opt.is_none() { | ||
| *decoder_opt = Some(H264Decoder::new()?); | ||
| } | ||
|
|
||
| let decoder = decoder_opt.as_mut().unwrap(); | ||
| decoder | ||
| .decode(bytes)? | ||
| .ok_or(AsFFmpegError::H264NeedMoreData) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Thread-local decoder may cause issues with multiple camera streams.
Using a single thread-local decoder means that if multiple camera instances are used on the same thread (e.g., switching cameras), they will share decoder state, potentially causing decode failures or frame corruption.
Consider one of these alternatives:
- Store the decoder per camera instance instead of thread-local
- Add stream identification and auto-reset when stream changes
- Document that
reset_h264_decoder()must be called when switching cameras
Additionally, the unwrap at line 188 is safe but could be more idiomatic:
- if decoder_opt.is_none() {
- *decoder_opt = Some(H264Decoder::new()?);
- }
-
- let decoder = decoder_opt.as_mut().unwrap();
+ let decoder = match decoder_opt.as_mut() {
+ Some(d) => d,
+ None => {
+ *decoder_opt = Some(H264Decoder::new()?);
+ decoder_opt.as_mut().unwrap()
+ }
+ };| pub fn convert( | ||
| &self, | ||
| bgra_data: &[u8], | ||
| width: u32, | ||
| height: u32, | ||
| ) -> Result<Vec<u8>, wgpu::PollError> { | ||
| let input_texture = self.device.create_texture_with_data( | ||
| &self.queue, | ||
| &wgpu::TextureDescriptor { | ||
| label: Some("BGRA Input Texture"), | ||
| size: wgpu::Extent3d { | ||
| width, | ||
| height, | ||
| depth_or_array_layers: 1, | ||
| }, | ||
| mip_level_count: 1, | ||
| sample_count: 1, | ||
| dimension: wgpu::TextureDimension::D2, | ||
| format: wgpu::TextureFormat::Bgra8Unorm, | ||
| usage: wgpu::TextureUsages::TEXTURE_BINDING | wgpu::TextureUsages::COPY_DST, | ||
| view_formats: &[], | ||
| }, | ||
| wgpu::util::TextureDataOrder::MipMajor, | ||
| bgra_data, | ||
| ); |
There was a problem hiding this comment.
Missing input validation and inconsistent error type.
Unlike YUYVToRGBA::convert, this method lacks validation for:
- Zero dimensions (
width == 0 || height == 0) - Buffer size (
bgra_data.len() != width * height * 4)
Additionally, the return type Result<Vec<u8>, wgpu::PollError> is inconsistent with other converters that use ConvertError.
pub fn convert(
&self,
bgra_data: &[u8],
width: u32,
height: u32,
- ) -> Result<Vec<u8>, wgpu::PollError> {
+ ) -> Result<Vec<u8>, ConvertError> {
+ if width == 0 || height == 0 {
+ return Err(ConvertError::BufferSizeMismatch {
+ expected: 1,
+ actual: 0,
+ });
+ }
+
+ let expected_size = (width as usize) * (height as usize) * 4;
+ if bgra_data.len() != expected_size {
+ return Err(ConvertError::BufferSizeMismatch {
+ expected: expected_size,
+ actual: bgra_data.len(),
+ });
+ }
+
let input_texture = self.device.create_texture_with_data(Also update line 168 to map the error:
- read_buffer_to_vec(&output_buffer, &self.device)
+ read_buffer_to_vec(&output_buffer, &self.device).map_err(ConvertError::Poll)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/gpu-converters/src/bgra_rgba/mod.rs around lines 87 to 111, add the
same input validation as YUYVToRGBA::convert: return an
Err(ConvertError::InvalidDimensions) if width == 0 || height == 0 and return
Err(ConvertError::InvalidBufferSize) if bgra_data.len() != (width as usize) *
(height as usize) * 4; change the function signature to return Result<Vec<u8>,
ConvertError> (matching other converters) and update the
create_texture_with_data call to map any wgpu::PollError into ConvertError (see
line 168 and replace or wrap the existing mapping so the PollError is converted
into ConvertError::GpuError or similar appropriate ConvertError variant). Ensure
all new error variants used exist in ConvertError or add them consistently.
Adds support for additional pixel formats for both macOS and Windows camera backends, improves error handling, and enhances compatibility with more video sources. The changes primarily expand the range of supported formats, add fallback mechanisms for less common formats.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.