-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: Add camera preview timeout and improve camera cleanup #1436
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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. |
WalkthroughThe changes add first-frame startup timeout logic (5-second maximum) to camera initialization and refine state management in camera setup. Application state is pre-set when a camera ID is provided, with improved error handling and cleanup on initialization failure. Input removal logic on window close is expanded to consider active camera usage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/desktop/src-tauri/src/camera.rs (1)
500-503: First-frame timeout logic is sound; consider a small readability tweakThe startup timeout behaves correctly: you cap initial wait at 5 seconds using
saturating_sub, bail out cleanly on expiry, and lift the timeout after the first frame viareceived_first_frame. This integrates well with the existing select loop and avoids underflow.If you want to make intent even clearer and avoid scheduling a practically unbounded sleep, you could conditionally include the timeout branch only while
!received_first_frameinstead of usingDuration::MAX, but the current behavior is functionally fine.Also applies to: 506-515, 526-529, 534-535
apps/desktop/src-tauri/src/lib.rs (1)
459-464: Camera init retry/state handling looks correct; minor redundancy onlyThe new flow in
set_camera_inputforSome(id)is solid:
- You pre-set
selected_camera_id,camera_in_use, andcamera_cleanup_donebefore attempting initialization so other code can observe that the camera is being used.- The
init_resultloop retries up to three times with a clear error message and backoff, then rolls state back toNone/falseon failure and returns the error, avoiding a stuck “camera in use” flag on failed init.The only minor nit is redundancy: the trailing block later in the function reassigns
selected_camera_idandcamera_in_useagain on success. You could limit that block to handlingdisconnected_inputs/events (and perhaps just assert consistency) to reduce double writes, but it’s not functionally problematic.Also applies to: 467-496, 498-502
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src-tauri/src/camera.rs(3 hunks)apps/desktop/src-tauri/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:
apps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/camera.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Never add comments to code (
//,/* */,///,//!,#, etc.); code must be self-explanatory through naming, types, and structure
Files:
apps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/camera.rs
🧠 Learnings (1)
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
apps/desktop/src-tauri/src/lib.rs
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/lib.rs (1)
apps/desktop/src-tauri/src/windows.rs (9)
app(233-233)app(252-252)app(409-409)app(510-510)app(846-846)app(866-866)app(1073-1073)app(1085-1085)id(840-872)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
apps/desktop/src-tauri/src/camera.rs (1)
20-22: New imports are aligned with the timeout and logging changesBringing
Duration/Instantinto scope and narrowing thetracingimport to the used macros is appropriate and keeps the module tidy.apps/desktop/src-tauri/src/lib.rs (1)
2773-2779: Main-window destroy cleanup now respects camera usage and preview windowThe added
camera_window_opencheck and the!app_state.camera_in_useguard ensure that mic/camera inputs and selections are only torn down when:
- No recording is active or pending,
- No camera window is open, and
- The app state does not think the camera is in use.
This aligns with the new camera lifecycle behavior and avoids prematurely dropping camera input when the preview or recording is still active.
Improvements to camera initialization and error handling in the desktop application. The main focus is on making the camera startup process more robust by adding a timeout for receiving the first frame, improving error handling during camera setup, and ensuring the application state is updated correctly when camera initialization fails. Additionally, the logic for releasing microphone and camera resources has been refined.
Summary by CodeRabbit
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.