feat: Numerous camera fixes (pausing, stopping, UX bits, etc) #1391
Conversation
WalkthroughThe changes refactor input management by extracting common logic into centralized helper functions, implement automatic input restoration from stored settings during recording startup and window initialization, add robust camera frame forwarding with reattachment capability, enforce monotonic video timestamps, and restructure the recording UI to support interactive area bounds tracking for controls. Changes
Sequence DiagramssequenceDiagram
participant App
participant RecordingStartup as Recording Start
participant StateStore as Settings Store
participant InputApply as apply_mic/camera_input
participant AppState
RecordingStartup->>RecordingStartup: Check if recording active
alt No recording & inputs missing
RecordingStartup->>StateStore: Load RecordingSettingsStore
StateStore-->>RecordingStartup: Settings loaded
RecordingStartup->>InputApply: apply_mic_input(state, label)
InputApply->>AppState: Update mic state
InputApply-->>RecordingStartup: Result
RecordingStartup->>InputApply: apply_camera_input(app, state, id)
InputApply->>AppState: Update camera state & show preview
InputApply-->>RecordingStartup: Result
Note over RecordingStartup: Log warnings on failure, continue
else Recording active or inputs present
RecordingStartup->>RecordingStartup: Skip restoration
end
RecordingStartup->>RecordingStartup: Proceed with recording
sequenceDiagram
participant CameraTask as Camera Forwarding Task
participant FrameReceiver as Frame Receiver
participant VideoEncoder as Video Encoder
participant FeedLock as Feed Lock
loop Continuous Frame Loop
CameraTask->>FrameReceiver: Await frame
alt Frame received
FrameReceiver-->>CameraTask: Frame data
CameraTask->>VideoEncoder: Send frame
VideoEncoder-->>CameraTask: Ack
else Receiver error (channel closed)
FrameReceiver-->>CameraTask: Error
CameraTask->>CameraTask: Create new bounded channel
CameraTask->>FeedLock: Send AddSender (reattach)
FeedLock-->>CameraTask: Result (log warning if fails)
CameraTask->>CameraTask: Switch to new receiver
Note over CameraTask: Continue forwarding with new channel
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
crates/recording/src/output_pipeline/ffmpeg.rs (1)
106-126: Consider logging timestamp adjustments for debugging.The timestamp adjustment logic correctly enforces non-decreasing timestamps by advancing out-of-order frames to
last_ts + frame_duration. However, these corrections happen silently, which could make it harder to diagnose issues with the video source.Consider adding a debug-level log when timestamps are adjusted:
if let Some(last_ts) = self.last_video_ts { if timestamp <= last_ts { + tracing::debug!( + "Adjusted video timestamp from {:?} to {:?}", + timestamp, + last_ts + frame_duration + ); timestamp = last_ts + frame_duration; } }crates/recording/src/sources/camera.rs (1)
33-65: Forwarding loop is robust; consider usingSetupCtx::spawnfor supervisionThe reattachment logic around
flumereceiver errors and the error/warn logging look solid and should make the camera pipeline more resilient.One thing to consider is wiring this task through
SetupCtx::spawninstead of a baretokio::spawn, so it participates in the output pipeline’s lifecycle/logging (like the other tasks spawned fromSetupCtx). That would avoid an extra detached task and give you consistent tracing of task start/finish.apps/desktop/src-tauri/src/deeplink_actions.rs (1)
9-12: Confirm semantics when deeplinks explicitly passNonefor camera/micRouting the deeplink path through
apply_camera_input/apply_mic_inputis good for consistency.One subtle edge case:
start_recordingnow callsrestore_inputs_from_store_if_missing, which restores mic/camera fromRecordingSettingsStorewheneverselected_*isNone. If a deeplink explicitly passescamera: nullormic_label: null,apply_*_inputwill set the selection toNone, and the subsequent restore step may bring back stored inputs, effectively ignoring the deeplink’s “no camera/mic” intent.If “
Nonemeans restore last-used” is not what you want for deeplinks, you may want to either:
- Skip
restore_inputs_from_store_if_missingfor deeplink-started recordings, or- Pass a flag into
start_recording/ the restore helper to indicate that inputs were explicitly chosen for this invocation and should not be auto-restored.Also applies to: 122-124
apps/desktop/src-tauri/src/windows.rs (1)
23-30: Idle restoration helper is sound; double‑check when it should runThe new
restore_recording_inputs_if_idlehelper looks well-contained:
- It tolerates missing/invalid
RecordingSettingsStoreentries and just logs awarn!.- It skips work when both
mic_nameandcamera_idareNone.- It checks
is_recording_active_or_pending()before touching inputs, and applies mic/camera via the centralizedapply_*_inputhelpers with warnings on failure.Two small points to confirm:
When to run:
ShowCapWindow::Maincallsrestore_recording_inputs_if_idle(app)unconditionally (regardless ofenable_new_recording_flow). If the intent was to only auto-restore inputs for the new recording flow, you may want to gate this call onnew_recording_flow.Possible duplicate restoration: There is now both this idle-time restoration and the
restore_inputs_from_store_if_missingcall at the top ofstart_recording. In practice it’s harmless (the helpers are idempotent), but if you only want one of these behaviors, it might be cleaner to pick a single restoration point.Also applies to: 281-286, 802-842
apps/desktop/src-tauri/src/recording.rs (1)
50-60: Input restoration helper is reasonable; clarify “missing vs explicitly disabled” semanticsThe restoration flow is generally solid:
- It first checks
recording_stateand bails if a recording is active.- It only does work when
selected_mic_labelorselected_camera_idisNone.- It handles
RecordingSettingsStore::getfailures with awarn!and early return.- Mic/camera restoration runs through the centralized
apply_*_inputhelpers, so state and preview windows stay in sync.One behavior to be aware of: treating
selected_mic_label.is_none()/selected_camera_id.is_none()as “needs restore” means that any caller who deliberately sets these toNoneright beforestart_recording(e.g. a deeplink that intends “no camera”) can have that choice silently overridden by stored settings.If you need to distinguish “unset/missing on startup” from “explicitly disabled for this recording”, it might be worth:
- Threading an explicit “don’t restore inputs for this call” flag into
restore_inputs_from_store_if_missing, or- Having the caller pass in which inputs it wants restored vs left alone, rather than inferring purely from
selected_*beingNone.Otherwise, the implementation looks correct and should behave well in the general “restore last-known inputs on startup” case.
Also applies to: 354-389, 399-400
apps/desktop/src/routes/in-progress-recording.tsx (1)
200-222: Effect and cleanup correctly manage fake-window lifecycle; consider optional throttlingThe effect defensively removes the fake window when the ref disappears, skips zero-sized bounds, and reapplies bounds when the interactive area moves/resizes, with
onCleanupensuring final teardown. This is functionally sound.If you ever see backend chatter from frequent
setFakeWindowBoundscalls (e.g., during window drags or resizes), consider tracking the last{left, top, width, height}in a local signal/memo and early-return when unchanged to avoid redundant TAURI invocations. No change needed unless profiling shows this as hot.apps/desktop/src-tauri/src/lib.rs (1)
2561-2563: Don’t silently ignore apply_ failures before starting a recording*In the
RequestStartRecordinghandler, failures fromapply_mic_input/apply_camera_inputare currently discarded, so a recording can start with misconfigured inputs without any log signal. This isn’t fatal but makes diagnosing device issues harder.Consider logging these errors before proceeding to
start_recording:- let settings = RecordingSettingsStore::get(&app) + let settings = RecordingSettingsStore::get(&app) .ok() .flatten() .unwrap_or_default(); - let _ = apply_mic_input(app.state(), settings.mic_name).await; - let _ = apply_camera_input(app.clone(), app.state(), settings.camera_id).await; + if let Err(err) = apply_mic_input(app.state(), settings.mic_name).await { + warn!("Failed to apply mic input from settings: {err}"); + } + if let Err(err) = + apply_camera_input(app.clone(), app.state(), settings.camera_id).await + { + warn!("Failed to apply camera input from settings: {err}"); + } let _ = start_recording(app.clone(), app.state(), {This keeps behavior the same (recording still starts) but surfaces configuration problems via tracing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/desktop/src-tauri/src/deeplink_actions.rs(2 hunks)apps/desktop/src-tauri/src/lib.rs(3 hunks)apps/desktop/src-tauri/src/recording.rs(4 hunks)apps/desktop/src-tauri/src/windows.rs(3 hunks)apps/desktop/src/routes/in-progress-recording.tsx(6 hunks)crates/recording/examples/camera_stream.rs(1 hunks)crates/recording/src/output_pipeline/core.rs(2 hunks)crates/recording/src/output_pipeline/ffmpeg.rs(4 hunks)crates/recording/src/sources/camera.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/sources/camera.rscrates/recording/src/output_pipeline/ffmpeg.rscrates/recording/src/output_pipeline/core.rsapps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/deeplink_actions.rsapps/desktop/src-tauri/src/recording.rsapps/desktop/src-tauri/src/windows.rscrates/recording/examples/camera_stream.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/sources/camera.rscrates/recording/src/output_pipeline/ffmpeg.rscrates/recording/src/output_pipeline/core.rs
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/desktop/src/routes/in-progress-recording.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/routes/in-progress-recording.tsx
🧠 Learnings (2)
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/recording/src/sources/camera.rscrates/recording/src/output_pipeline/ffmpeg.rscrates/recording/src/output_pipeline/core.rsapps/desktop/src-tauri/src/deeplink_actions.rscrates/recording/examples/camera_stream.rs
📚 Learning: 2025-10-17T05:58:22.586Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1219
File: crates/enc-avfoundation/src/mp4.rs:350-373
Timestamp: 2025-10-17T05:58:22.586Z
Learning: In crates/enc-avfoundation/src/mp4.rs, the `finish()` method intentionally skips video extension when `is_paused` is true. This is correct behavior because if recording is paused, the video should not be extended beyond the pause point—the pause is user-initiated, unlike the case where ScreenCaptureKit stops providing frames during static content.
Applied to files:
crates/recording/src/output_pipeline/ffmpeg.rscrates/recording/src/output_pipeline/core.rs
🧬 Code graph analysis (7)
crates/recording/src/sources/camera.rs (2)
crates/recording/src/output_pipeline/core.rs (1)
spawn(122-142)apps/desktop/src-tauri/src/camera_legacy.rs (2)
flume(8-8)flume(9-9)
crates/recording/src/output_pipeline/ffmpeg.rs (3)
crates/enc-avfoundation/src/mp4.rs (1)
video_frame_duration(398-411)crates/recording/src/sources/screen_capture/mod.rs (2)
config(348-350)info(352-354)crates/enc-ffmpeg/src/video/h264.rs (1)
builder(234-236)
crates/recording/src/output_pipeline/core.rs (7)
crates/recording/src/output_pipeline/ffmpeg.rs (2)
timestamp(22-24)send_video_frame(106-126)crates/recording/src/sources/screen_capture/windows.rs (2)
timestamp(63-65)std(157-157)crates/recording/src/sources/screen_capture/macos.rs (1)
timestamp(61-63)crates/recording/src/output_pipeline/macos.rs (1)
send_video_frame(70-103)crates/recording/src/output_pipeline/win.rs (1)
send_video_frame(342-352)crates/timestamp/src/macos.rs (1)
duration_since(22-31)crates/timestamp/src/win.rs (1)
duration_since(30-47)
apps/desktop/src-tauri/src/deeplink_actions.rs (1)
apps/desktop/src-tauri/src/lib.rs (8)
apply_camera_input(428-505)apply_mic_input(337-408)app(1537-1538)app(2597-2597)app(2628-2628)app(2692-2692)app(2698-2698)app(2922-2923)
apps/desktop/src-tauri/src/recording.rs (2)
apps/desktop/src-tauri/src/lib.rs (9)
apply_camera_input(428-505)apply_mic_input(337-408)app(1537-1538)app(2597-2597)app(2628-2628)app(2692-2692)app(2698-2698)app(2922-2923)None(3061-3061)apps/desktop/src-tauri/src/recording_settings.rs (1)
get(29-40)
apps/desktop/src-tauri/src/windows.rs (2)
apps/desktop/src-tauri/src/lib.rs (12)
apply_camera_input(428-505)apply_mic_input(337-408)app(1537-1538)app(2597-2597)app(2628-2628)app(2692-2692)app(2698-2698)app(2922-2923)None(3061-3061)app_handle(509-509)app_handle(550-550)app_handle(594-594)apps/desktop/src-tauri/src/recording_settings.rs (1)
get(29-40)
crates/recording/examples/camera_stream.rs (1)
crates/recording/src/feeds/camera.rs (2)
camera_info(157-159)from_info(189-193)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (10)
crates/recording/src/output_pipeline/core.rs (2)
432-432: LGTM: Parameter mutability removed.The receiver is now converted to a fused stream within the task (line 461), so mutability at the parameter level is no longer needed.
453-500: Well-structured cancellation-aware video muxing.The refactored task now handles video stream closure gracefully:
- Uses
tokio::select!to handle both cancellation signals and incoming frames- If the video stream ends before cancellation (line 483-490), it logs a warning and cancels the entire pipeline via
stop_token_on_close- Ensures proper cleanup by calling
muxer.lock().await.stop()after the loopThis is a significant behavioral improvement that prevents incomplete recordings when the video source stops unexpectedly.
crates/recording/src/output_pipeline/ffmpeg.rs (3)
31-32: LGTM: Timestamp tracking fields added.These fields support the non-decreasing timestamp enforcement logic in
send_video_frame. TheOptiontypes correctly handle scenarios where video encoding is disabled.
51-60: LGTM: Frame duration computed during setup.Computing the frame duration once during encoder initialization is efficient. The tuple return pattern cleanly handles both the encoder and duration together.
129-138: Verify handling of invalid frame rates.The
frame_durationcalculation uses.max(1)guards to prevent division by zero. However, consider whether additional validation is needed for edge cases:Run this script to check if
VideoInfo.frame_ratecan have zero or negative values:If zero or negative frame rates are possible, consider adding an early return with a sensible default (as done in the AVFoundation implementation at
crates/enc-avfoundation/src/mp4.rs:397-410):fn frame_duration(info: &VideoInfo) -> Duration { let num = info.frame_rate.numerator(); let den = info.frame_rate.denominator(); if num <= 0 || den <= 0 { return Duration::from_millis(33); // ~30 fps fallback } let nanos = ((den as u128 * 1_000_000_000u128) / num as u128).max(1); Duration::from_nanos(nanos as u64) }crates/recording/examples/camera_stream.rs (1)
1-56: Camera example looks correct and idiomaticThe example wires up
CameraFeed, appliesSetInput, acquires a lock, and uses a bounded channel to count frames over 5 seconds with sensible error handling. Nothing blocking here from a correctness perspective.apps/desktop/src/routes/in-progress-recording.tsx (2)
55-89: Interactive area ref and bounds wiring look solidUsing a dedicated
FAKE_WINDOW_BOUNDS_NAMEplusinteractiveAreaRefand derived bounds is clear and keeps the interactive area concerns nicely scoped to this component. No functional issues stand out here.Please just confirm visually that the fake-window overlay aligns with this container across DPI scaling / multi-display setups.
510-672: New interactive area layout and drag-region placement are coherentWrapping the issue panel, controls, and drag handle in the interactive area (with the handle as the primary
data-tauri-drag-region) matches the fake-window bounds logic and keeps the draggable affordance explicit. The mic/issue/settings controls and ARIA attributes still look consistent with prior behavior.Given the drag-region is now mainly the right-hand handle (and the mic-off icon when no mic), double-check on Windows/macOS that window dragging still feels discoverable and that the fake-window hitbox matches the visible control bar.
apps/desktop/src-tauri/src/lib.rs (2)
334-408: Mic input helper centralization looks correct and avoids holding locks across awaits
apply_mic_inputcleanly factors out the mic-handling logic: it snapshotsmic_feedand optional studio handle under a read lock, avoids unnecessary work when the requested label matches the current one, detaches the studio mic feed while reconfiguring the actor, then updates App state and emitsInputRestoredonly when the disconnected flag was actually set. This matches the existing disconnect/restoration model without obvious regressions.Given this now runs both from the Tauri command and from startup flows, please ensure you have a test/manual path that covers:
- Switching from one mic to another mid-recording (including pause/resume), and
- Selecting “No Microphone” after an
InputLostevent,
to confirmdisconnected_inputsand the frontend issue-state behave as expected.
420-505: Camera input helper mirrors mic logic and integrates preview window behavior correctly
apply_camera_inputfollows the same pattern as the mic helper: it snapshotscamera_feedand optional studio handle, no-ops when the same camera is already in-use, detaches from the studio handle while updating, removes or sets the camera feed accordingly, shows the camera preview when a camera is selected, then updatesselected_camera_id,camera_in_use, and conditionally emitsInputRestored. This keeps the recording state and device watcher invariants intact.Please sanity-check:
- Switching camera devices during a studio recording (including recovering from
InputLost), and- Turning the camera off (None) while the preview window is open,
to ensure the preview window andcamera_in_useflag stay in sync with the new helper logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/routes/in-progress-recording.tsx (1)
212-230: Improve bounds validation before setting fake window.The effect uses nullish coalescing with
0as fallback for bounds that might beundefined, but only checks for zero width/height. Ifleftortopareundefined, they'll become0, which might not represent the actual position during initial render.Consider validating that all bounds properties are defined:
const left = interactiveBounds.left ?? 0; const top = interactiveBounds.top ?? 0; const width = interactiveBounds.width ?? 0; const height = interactiveBounds.height ?? 0; -if (width === 0 || height === 0) return; +if (width === 0 || height === 0 || interactiveBounds.left == null || interactiveBounds.top == null) return; void commands.setFakeWindowBounds(FAKE_WINDOW_BOUNDS_NAME, {This ensures fake window bounds are only set when the element is fully positioned.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/in-progress-recording.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/desktop/src/routes/in-progress-recording.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/routes/in-progress-recording.tsx
⏰ 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). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
apps/desktop/src/routes/in-progress-recording.tsx (5)
1-1: LGTM! Clear setup for interactive bounds tracking.The imports and constant for managing the interactive area bounds are well-named and appropriate for the new functionality.
Also applies to: 21-21, 57-57
86-88: LGTM! Proper reactive bounds tracking.The signal and bounds tracking setup correctly follows Solid.js patterns for observing DOM element dimensions.
232-234: LGTM! Proper cleanup on unmount.The cleanup correctly removes the fake window when the component is destroyed, preventing resource leaks.
522-686: LGTM! Well-structured UI improvements.The UI restructure successfully:
- Wraps controls in a tracked interactive area for bounds management
- Adds a dismiss button to the issue panel for better UX (lines 532-539)
- Maintains all existing functionality and accessibility features
- Preserves the drag region for window movement
The nested structure is clear and the interactive area tracking integrates cleanly with the recording controls.
93-101: Verify platform-specific pause capability logic aligns with backend support.The frontend restricts pause to studio mode, macOS, or Windows+instant mode (lines 93-101). However, the backend has no equivalent platform or mode checks—
pause_recording()unconditionally callsrecording.pause()for bothInstantandStudiotypes without guards.Verify one of the following:
- The pause capability restrictions in the underlying
instant_recordingandstudio_recordingimplementations justify the frontend constraints, OR- The frontend restrictions should be relaxed if the backend genuinely supports pause across all platforms/modes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/recording/src/output_pipeline/ffmpeg.rs (1)
109-120: Good timestamp monotonicity enforcement.The logic correctly ensures strictly monotonic timestamps by adjusting any frame with
timestamp <= last_tstolast_ts + frame_duration. This prevents muxer issues with non-monotonic or duplicate timestamps.Consider adding debug logging when timestamp adjustments occur to aid future debugging:
if let Some(frame_duration) = self.video_frame_duration { if let Some(last_ts) = self.last_video_ts && timestamp <= last_ts { + tracing::debug!( + "Adjusting timestamp from {:?} to {:?} (last_ts: {:?}, frame_duration: {:?})", + timestamp, last_ts + frame_duration, last_ts, frame_duration + ); timestamp = last_ts + frame_duration; } self.last_video_ts = Some(timestamp); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/recording/src/output_pipeline/ffmpeg.rs(4 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/output_pipeline/ffmpeg.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/output_pipeline/ffmpeg.rs
🧠 Learnings (2)
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/recording/src/output_pipeline/ffmpeg.rs
📚 Learning: 2025-10-17T05:58:22.586Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1219
File: crates/enc-avfoundation/src/mp4.rs:350-373
Timestamp: 2025-10-17T05:58:22.586Z
Learning: In crates/enc-avfoundation/src/mp4.rs, the `finish()` method intentionally skips video extension when `is_paused` is true. This is correct behavior because if recording is paused, the video should not be extended beyond the pause point—the pause is user-initiated, unlike the case where ScreenCaptureKit stops providing frames during static content.
Applied to files:
crates/recording/src/output_pipeline/ffmpeg.rs
🧬 Code graph analysis (1)
crates/recording/src/output_pipeline/ffmpeg.rs (2)
crates/enc-avfoundation/src/mp4.rs (1)
video_frame_duration(398-411)crates/enc-ffmpeg/src/video/h264.rs (1)
builder(234-236)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
crates/recording/src/output_pipeline/ffmpeg.rs (4)
31-32: LGTM! Fields support timestamp monotonicity enforcement.The new fields appropriately track frame timing state to enable the timestamp adjustment logic in
send_video_frame. UsingOptiontypes is correct since video encoding may not always be configured.
51-60: LGTM! Frame duration computation is properly integrated into setup.The setup logic cleanly handles both configured and unconfigured video scenarios. Computing frame duration at setup time is appropriate since the frame rate should remain constant throughout the recording.
73-74: LGTM! Proper field initialization.Setting
last_video_tstoNoneis correct since no frames have been sent yet. The first frame will establish the timestamp baseline.
130-138: LGTM! Frame duration calculation is correct.The calculation matches the reference implementation in
crates/enc-avfoundation/src/mp4.rs:397-410. Using.max(1)on both numerator and denominator prevents division by zero while keeping the code simple.Example verification:
- 30 FPS: (1 × 1e9 / 30) = 33,333,333 ns ≈ 33.33 ms ✓
- 60 FPS: (1 × 1e9 / 60) = 16,666,666 ns ≈ 16.67 ms ✓
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/desktop/src-tauri/src/recording.rs (1)
354-387: Input restoration helper is sound; consider minor cleanups and a semantics checkThe overall flow looks good: you avoid restoring while a recording is active, bail when both inputs are already populated, and handle settings/errors defensively.
A couple of refinements:
- The single‑arm matches on the
apply_*calls are a bit clippy‑unfriendly; something like:- if let Some(mic) = settings.mic_name.clone().filter(|_| needs_mic) { - match apply_mic_input(app.state(), Some(mic)).await { - Err(err) => warn!(%err, "Failed to restore microphone input"), - Ok(_) => {} - } - } + if let Some(mic) = settings.mic_name.clone().filter(|_| needs_mic) { + if let Err(err) = apply_mic_input(app.state(), Some(mic)).await { + warn!(%err, "Failed to restore microphone input"); + } + }(and similarly for camera) will likely keep clippy quieter and simplify the code. As per coding guidelines.
- Optional: you can avoid cloning when restoration is not needed by checking
needs_mic/needs_camerafirst, then cloning only in that branch, which may help if these IDs ever become heavier types.Also worth double‑checking that
apply_mic_input’s early return onlabel == current_labelwon’t prevent restoration in cases where the label is already set but the underlying feed was removed (e.g., afterhandle_recording_endcallsRemoveInput). If that scenario matters for this helper, you may need to revisit the guard inapply_mic_input.apps/desktop/src-tauri/src/windows.rs (2)
246-302: Hooking idle input restoration from the Main window is sensible; verify scopeCalling
restore_recording_inputs_if_idle(app)right after constructing the Main window ties input restoration to the primary UI entry point, which should feel natural to users.Note that this runs regardless of
enable_new_recording_flow; if the intent was to limit this behavior to the new flow only, you might want to gate the call behindif new_recording_flow { ... }. If it’s desirable for both flows, current placement is fine.
802-842: Idle input restoration logic is reasonable; small refactors could improve clarityThe helper correctly:
- Loads
RecordingSettingsStoreand logs on error.- Short‑circuits when both stored inputs are
None.- Spawns an async task that first checks
is_recording_active_or_pending()to avoid interfering with active/pending recordings.- Uses the centralized
apply_mic_input/apply_camera_inputhelpers and logs restoration failures.A few targeted tweaks to consider:
- As in
recording.rs, the single‑armmatchon theapply_*results could be simplified toif let Err(err) = ...to keep clippy happy and reduce noise:- if let Some(mic) = mic_name { - match apply_mic_input(app_handle.state(), Some(mic)).await { - Err(err) => warn!(%err, "Failed to restore microphone input"), - Ok(_) => {} - } - } + if let Some(mic) = mic_name { + if let Err(err) = apply_mic_input(app_handle.state(), Some(mic)).await { + warn!(%err, "Failed to restore microphone input"); + } + }(and similarly for the camera case). As per coding guidelines.
You already guard on
is_recording_active_or_pending()once inside the spawned task. If you find in practice that recordings are occasionally started in the brief window after this check, and that causes unwanted re‑attachment while recording, you may want a slightly stronger guard (e.g., rechecking just before eachapply_*or havingapply_*itself be aware of the “active/pending” concept).As with the other helper, it’s worth confirming
apply_mic_input’slabel == current_labelearly return doesn’t prevent re‑attaching a mic that was previously removed but has the same label, since this helper always uses the stored label.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src-tauri/src/recording.rs(4 hunks)apps/desktop/src-tauri/src/windows.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/windows.rsapps/desktop/src-tauri/src/recording.rs
🧬 Code graph analysis (2)
apps/desktop/src-tauri/src/windows.rs (2)
apps/desktop/src-tauri/src/lib.rs (12)
apply_camera_input(428-505)apply_mic_input(337-408)app(1537-1538)app(2597-2597)app(2628-2628)app(2692-2692)app(2698-2698)app(2922-2923)None(3061-3061)app_handle(509-509)app_handle(550-550)app_handle(594-594)apps/desktop/src-tauri/src/recording_settings.rs (1)
get(29-40)
apps/desktop/src-tauri/src/recording.rs (2)
apps/desktop/src-tauri/src/lib.rs (9)
apply_camera_input(428-505)apply_mic_input(337-408)app(1537-1538)app(2597-2597)app(2628-2628)app(2692-2692)app(2698-2698)app(2922-2923)None(3061-3061)apps/desktop/src-tauri/src/recording_settings.rs (1)
get(29-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
apps/desktop/src-tauri/src/recording.rs (3)
45-66: New imports align with helper usageThe added imports for
apply_camera_input,apply_mic_input, andRecordingSettingsStoreare consistent with their usage below and keep responsibilities centralized in the helpers. No issues here.
392-398: Restoring inputs before recording start is reasonableCalling
restore_inputs_from_store_if_missingat the top ofstart_recordingensures missing mic/camera selections are pulled from the store before any further state checks. Given the helper already short‑circuits when a recording is active, this is safe and keeps the command idempotent.
403-417: Camera preview gating for instant recordings looks correctThe new
should_open_camera_previewlogic:
- Only runs in
Instantmode.- Requires a selected camera.
- Skips if the camera window is already open (so it cooperates with
apply_camera_input’s ownShowCapWindow::Camerabehavior).The error handling via
map_err(|err| error!(...)).ok()also keeps failures from breaking the recording flow while still logging. No functional issues spotted.apps/desktop/src-tauri/src/windows.rs (1)
23-30: Import updates correctly reflect new responsibilitiesBringing in
apply_camera_input,apply_mic_input,RecordingSettingsStore, andRecordingTargetModematches the new logic added later in this file. Everything appears consistent with how these symbols are used.
Summary by CodeRabbit
New Features
Improvements