Use Rec709 color space in cap_enc_avfoundation#1177
Conversation
WalkthroughTwo areas changed: error string formatting in desktop recording actor spawn paths, and AVFoundation MP4 encoder output settings gained explicit Rec.709 color properties with corresponding extern bindings updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/desktop/src-tauri/src/recording.rs (1)
488-490: Apply consistent error formatting to the instant recording branch.The studio recording branch now uses pretty Debug formatting (
{e:#}), but the instant recording branch at lines 523-526 still usese.to_string(). For consistency and better debugging output, apply the same formatting to both branches.Apply this diff to the instant recording branch:
.map_err(|e| { - error!("Failed to spawn instant recording actor: {e}"); - e.to_string() + error!("Failed to spawn instant recording actor: {e:#}"); + format!("{e:#}") })?;crates/enc-avfoundation/src/mp4.rs (1)
390-398: Consider using consistent type syntax for extern declarations.The color property keys (lines 391-393) use
ns::Stringwhile the color constants (lines 395-397) usecidre::ns::String. Since both resolve to the same type, prefer consistent syntax throughout the extern block.Apply this diff for consistency:
- static AVVideoTransferFunction_ITU_R_709_2: &'static cidre::ns::String; - static AVVideoColorPrimaries_ITU_R_709_2: &'static cidre::ns::String; - static AVVideoYCbCrMatrix_ITU_R_709_2: &'static cidre::ns::String; + static AVVideoTransferFunction_ITU_R_709_2: &'static ns::String; + static AVVideoColorPrimaries_ITU_R_709_2: &'static ns::String; + static AVVideoYCbCrMatrix_ITU_R_709_2: &'static ns::String;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src-tauri/src/recording.rs(1 hunks)crates/enc-avfoundation/src/mp4.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
apps/desktop/src-tauri/src/recording.rscrates/enc-avfoundation/src/mp4.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/recording.rscrates/enc-avfoundation/src/mp4.rs
crates/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
For desktop IPC, use tauri_specta derive/macros on Rust types/events; do not hand-roll bindings
Files:
crates/enc-avfoundation/src/mp4.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/enc-avfoundation/src/mp4.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: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
crates/enc-avfoundation/src/mp4.rs (1)
128-143: LGTM! Explicit Rec.709 color space configuration added.The addition of explicit Rec.709 color properties (transfer function, primaries, and YCbCr matrix) ensures correct color representation for HD/4K SDR video content. This is the standard color space for screen recordings and aligns with the PR objectives.
Summary by CodeRabbit