recording: move display+crop conversion out of capture sources #1257
Conversation
WalkthroughThis PR replaces the async create_screen_capture path with a new target_to_display_and_crop function that resolves a ScreenCaptureTarget into a scap_targets::Display and optional platform-specific CropBounds, and updates ScreenCaptureConfig::init and callers to accept the resolved display + crop. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Instant/Studio Caller
participant Pipeline as capture_pipeline::target_to_display_and_crop
participant Config as ScreenCaptureConfig::init
participant Source as screen_capture source
rect #EBF5FF
note over Caller,Pipeline: Resolve ScreenCaptureTarget -> (Display, Option<CropBounds>)
Caller->>Pipeline: target_to_display_and_crop(target)
Pipeline->>Pipeline: lookup Display\ncompute optional CropBounds (OS-specific)
Pipeline-->>Caller: (Display, Option<CropBounds>)
end
rect #F0FFF0
note over Caller,Config: Initialize capture with resolved context
Caller->>Config: init(display, crop_bounds, show_cursor, max_fps, ...)
Config->>Source: configure capture using crop_bounds/output_size
Config-->>Caller: ScreenCaptureConfig
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
crates/*/src/**/*📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (1)crates/recording/src/capture_pipeline.rs (3)
⏰ 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)
🔇 Additional comments (5)
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: 2
🧹 Nitpick comments (4)
crates/recording/src/studio_recording.rs (1)
753-763: Misleading context labelThis block builds the system audio pipeline but uses context("microphone pipeline setup"). Rename for clarity.
- .context("microphone pipeline setup")?; + .context("system audio pipeline setup")?;crates/recording/src/instant_recording.rs (1)
302-316: Avoid double-retain on macOS shareable_contentinit() already retains shareable_content internally. Passing inputs.shareable_content (not .retained()) avoids an unnecessary retain and matches studio_recording.
- #[cfg(target_os = "macos")] - inputs.shareable_content.retained(), + #[cfg(target_os = "macos")] + inputs.shareable_content,crates/recording/src/sources/screen_capture/mod.rs (1)
301-315: Avoid cloning OptionUse as_ref() to prevent an unnecessary clone; keeps identical behavior.
- let output_size = crop_bounds - .clone() - .and_then(|b| { + let output_size = crop_bounds + .as_ref() + .and_then(|b| {crates/recording/src/capture_pipeline.rs (1)
9-9: Unused/OS‑mismatched importTop‑level use scap_targets::bounds::LogicalBounds is redundant (function imports bounds::*). On Windows it may trigger unused import warnings.
-use scap_targets::bounds::LogicalBounds; +// (remove — rely on function‑scoped import)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/recording/src/capture_pipeline.rs(2 hunks)crates/recording/src/instant_recording.rs(2 hunks)crates/recording/src/sources/screen_capture/mod.rs(3 hunks)crates/recording/src/studio_recording.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
crates/recording/src/studio_recording.rscrates/recording/src/instant_recording.rscrates/recording/src/sources/screen_capture/mod.rscrates/recording/src/capture_pipeline.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/recording/src/studio_recording.rscrates/recording/src/instant_recording.rscrates/recording/src/sources/screen_capture/mod.rscrates/recording/src/capture_pipeline.rs
🧬 Code graph analysis (4)
crates/recording/src/studio_recording.rs (3)
crates/recording/src/capture_pipeline.rs (1)
target_to_display_and_crop(129-220)crates/recording/src/cursor.rs (1)
spawn_cursor_recorder(41-188)crates/recording/src/sources/screen_capture/mod.rs (3)
d3d_device(345-347)display(67-73)init(285-342)
crates/recording/src/instant_recording.rs (2)
crates/recording/src/capture_pipeline.rs (1)
target_to_display_and_crop(129-220)crates/recording/src/sources/screen_capture/mod.rs (2)
display(67-73)init(285-342)
crates/recording/src/sources/screen_capture/mod.rs (3)
crates/scap-targets/src/platform/win.rs (1)
display(1044-1051)crates/scap-targets/src/platform/macos.rs (1)
display(519-539)crates/scap-targets/src/lib.rs (1)
display(146-148)
crates/recording/src/capture_pipeline.rs (3)
apps/desktop/src/utils/tauri.ts (4)
ScreenCaptureTarget(459-459)LogicalBounds(415-415)LogicalPosition(416-416)PhysicalSize(430-430)crates/recording/src/sources/screen_capture/mod.rs (2)
display(67-73)window(75-80)crates/scap-targets/src/platform/macos.rs (4)
display(519-539)from_id(48-51)id(49-49)id(302-304)
⏰ 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 (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (7)
crates/recording/src/studio_recording.rs (2)
686-690: Cursor crop derivation: LGTMUsing ScreenCaptureTarget::cursor_crop() keeps cursor recorder aligned with the visual crop. Good.
764-779: Passing cursor_crop_bounds: LGTMPlumbing the precomputed cursor crop ensures correct cursor normalization post‑refactor.
crates/recording/src/instant_recording.rs (2)
299-301: Display/crop derivation: LGTMUsing target_to_display_and_crop with Context yields actionable errors. Good.
10-10: Import Context: LGTMBringing anyhow::Context as _ is consistent with usage below.
crates/recording/src/sources/screen_capture/mod.rs (2)
254-260: Config and CropBounds aliasing: LGTMSingle CropBounds alias per‑OS simplifies the surface and keeps Config minimal.
Also applies to: 261-266
286-295: Updated init signature: LGTMinit(display, crop_bounds, …) aligns with the new separation of concerns.
crates/recording/src/capture_pipeline.rs (1)
129-220: No fixes needed; verification confirms legitimate uses.The
ScreenCaptureInitError::NoBoundsvariant exists in the enum definition, and both uses incapture_pipeline.rs(lines 196, 199) are valid. The reference at line 317 inscreen_capture/mod.rsis in a different context with the appropriate return type. No stray references detected.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/recording/src/capture_pipeline.rs (1)
194-200: Previously flagged error-type mismatch is resolvedSwapping ScreenCaptureInitError for anyhow here aligns the function’s Result type. LGTM.
🧹 Nitpick comments (7)
crates/recording/src/capture_pipeline.rs (7)
9-9: Remove/guard unused LogicalBounds importOn non-macOS builds this is unused and may trip clippy. Since the function re-imports bounds types, the top-level import can go.
-use scap_targets::bounds::LogicalBounds;
132-132: Avoid wildcard imports inside functionWildcard imports often fail clippy::wildcard_imports. Import only what’s used.
- use scap_targets::{bounds::*, *}; + use scap_targets::{Display, Window}; + use scap_targets::bounds::{LogicalBounds, PhysicalBounds, LogicalPosition, PhysicalPosition, PhysicalSize};
134-142: Enrich errors with anyhow::Context (include variant/id)Add context so callers see which target/id failed. Apply similarly for window lookups.
- let display = target - .display() - .ok_or_else(|| anyhow!("Display not found"))?; + use anyhow::Context; + let display = target + .display() + .with_context(|| format!("Display not found for target {:?}", target))?;- let window = Window::from_id(id).ok_or_else(|| anyhow!("Window not found"))?; + let window = Window::from_id(id) + .with_context(|| format!("Window not found: {:?}", id))?;Based on learnings.
201-214: Compute scale once and guard against divide‑by‑zeroMinor cleanup: derive scale_x/scale_y once from display sizes; optionally assert they’re > 0 to avoid edge cases.
Example (conceptual):
- scale_x = raw.width / logical.width
- scale_y = raw.height / logical.height
- position_px = relative.position * scale
- size_px = relative.size * scale
This reduces repeated divisions and clarifies intent. If scap_targets uses integer physical units, consider rounding to nearest pixel before constructing PhysicalPosition/PhysicalSize.
Would you confirm the concrete number types of PhysicalPosition/PhysicalSize in scap_targets so we can choose between exact float math vs rounding?
145-161: Clamp crop rect to display bounds (optional hardening)If a window/area straddles edges, ensure the computed crop stays within the target display to avoid downstream API errors.
I can propose a small helper like clamp_to_display(display_bounds, crop_bounds) if desired.
Also applies to: 165-181
129-131: Add a brief doc commentDocument coordinate systems per platform (macOS=Logical, Windows=Physical) and invariants (crop relative to returned display).
Example:
/// Maps a ScreenCaptureTarget to (Display, optional crop).
/// macOS: CropBounds=LogicalBounds; Windows: CropBounds=PhysicalBounds.
/// Crop rectangle is relative to the returned Display’s origin.
129-220: Add unit tests for platform-specific bounds transformationsThe function
target_to_display_and_croplacks test coverage. Consider adding light tests for:
- Window on same display (offset math)
- Area on Windows with a non-1.0 scale factor
- macOS logical passthrough for Area
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/recording/src/capture_pipeline.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
crates/recording/src/capture_pipeline.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/recording/src/capture_pipeline.rs
🧬 Code graph analysis (1)
crates/recording/src/capture_pipeline.rs (2)
apps/desktop/src/utils/tauri.ts (3)
ScreenCaptureTarget(459-459)LogicalBounds(415-415)LogicalPosition(416-416)crates/recording/src/sources/screen_capture/mod.rs (2)
display(67-73)window(75-80)
⏰ 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 (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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 (2)
crates/recording/src/instant_recording.rs(3 hunks)crates/recording/src/studio_recording.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/recording/src/instant_recording.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
crates/recording/src/studio_recording.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/recording/src/studio_recording.rs
🧬 Code graph analysis (1)
crates/recording/src/studio_recording.rs (3)
crates/recording/src/capture_pipeline.rs (1)
target_to_display_and_crop(129-220)crates/recording/src/cursor.rs (1)
spawn_cursor_recorder(41-188)crates/recording/src/sources/screen_capture/mod.rs (3)
d3d_device(345-347)display(67-73)init(285-342)
⏰ 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 (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
crates/recording/src/studio_recording.rs (1)
694-712: LGTM: Error propagation fixed and new display/crop derivation approach is clean.The refactoring properly addresses the past review concern about using
.unwrap()after.awaitonScreenCaptureConfig::init. The code now correctly propagates errors using.context("screen capture init")?.The new approach of deriving
displayandcropviatarget_to_display_and_cropbefore passing them toScreenCaptureConfig::initis cleaner and separates concerns appropriately.Based on past review comments.
Summary by CodeRabbit