remove hardcoded min update interval

[Brendonovich]

Summary by CodeRabbit

• New Features

  • Added configurable FPS limiting for Windows screen capture with early frame dropping to maintain target frame rates.
  • Improved timestamp handling for more consistent synchronization.
  • Introduced drop-rate monitoring with logging and an auto-stop safeguard when drops exceed a threshold.

• Chores

  • Example CLI now runs without camera or system audio by default and includes a longer startup delay for stability.

[coderabbitai]

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title "remove hardcoded min update interval" accurately and concisely summarizes the primary change: removal of the hardcoded min_update_interval and introduction of timestamp-based per-frame handling and FPS control in the Windows screen-capture code. This is reflected in crates/recording/src/sources/screen_capture/windows.rs where min_update_interval was removed and NewFrame/timestamp logic was updated. The title is specific and on-point even though the changeset also includes related example and Actor-builder tweaks that need not be listed in the title.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-min-update-interval

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (4)

crates/recording/examples/recording-cli.rs (2)

61-61: Possible typo in sleep duration

The sleep duration remains at 10ms but the comment suggests this may have been intended to change based on the summary mentioning "timing adjustment before join: initial sleep increased from 10 ms to 10 seconds".

Based on line 77 which has a 10-second sleep, this appears to be unchanged from the original. If the intention was to increase this initial sleep as well, apply:

-    tokio::time::sleep(Duration::from_millis(10)).await;
+    tokio::time::sleep(Duration::from_secs(10)).await;

---

69-72: Consider configuration flexibility for camera/audio features

System audio and camera feed configuration are commented out. If these features might be needed in the future, consider using command-line arguments or configuration flags to enable/disable them rather than commenting out code.

Example approach:

let builder = instant_recording::Actor::builder(
    dir.path().into(),
    ScreenCaptureTarget::Display {
        id: Display::primary().id(),
    },
);

// Conditionally add features based on CLI args or config
let builder = if args.enable_system_audio {
    builder.with_system_audio(true)
} else {
    builder
};

crates/recording/src/sources/screen_capture/windows.rs (2)

132-138: Consider error handling for timestamp extraction

The code silently returns when SystemRelativeTime() fails. Consider logging this error condition for debugging purposes.

-        let Ok(timestamp) = msg.0.inner().SystemRelativeTime() else {
+        let Ok(timestamp) = msg.0.inner().SystemRelativeTime() else {
+            trace!("Failed to extract SystemRelativeTime from frame");
             return;
         };

---

404-404: Consider documenting the NewFrame struct change

The NewFrame struct has been simplified from containing both frame and display_time to just wrapping the frame. This is a breaking change in the internal API that should be documented.

Add documentation:

/// Represents a new frame captured from the screen.
/// The timestamp is now extracted from the frame's SystemRelativeTime.
pub struct NewFrame(pub scap_direct3d::Frame);

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d130069 and 4d2b25f.

📒 Files selected for processing (2)

• crates/recording/examples/recording-cli.rs (2 hunks)
• crates/recording/src/sources/screen_capture/windows.rs (6 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

{crates,apps/cli,apps/desktop/src-tauri}/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

{crates,apps/cli,apps/desktop/src-tauri}/**/*.rs: Rust code must be formatted with rustfmt and adhere to workspace clippy lints
Rust module files should use snake_case names

Files:

• crates/recording/src/sources/screen_capture/windows.rs
• crates/recording/examples/recording-cli.rs

{crates,apps/cli,apps/desktop/src-tauri}/**/{src,tests}/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Rust tests should live in src (inline/unit) or tests (integration) directories

Files:

• crates/recording/src/sources/screen_capture/windows.rs

🧬 Code graph analysis (2)

crates/recording/src/sources/screen_capture/windows.rs (2)

crates/timestamp/src/win.rs (1)

• now (35-39)

crates/timestamp/src/lib.rs (1)

• now (103-112)

crates/recording/examples/recording-cli.rs (4)

crates/recording/src/feeds/camera.rs (10)

• handle (326-400)
• handle (406-422)
• handle (428-430)
• handle (436-453)
• handle (459-465)
• handle (471-487)
• handle (503-539)
• handle (545-569)
• handle (575-591)
• handle (597-610)

crates/recording/src/studio_recording.rs (1)

• builder (129-131)

crates/recording/src/instant_recording.rs (1)

• builder (154-156)

crates/scap-targets/src/lib.rs (1)

• primary (20-22)

⏰ 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/examples/recording-cli.rs (2)

28-42: Ensure camera code removal is intentional

The camera initialization and setup code has been commented out. If this is temporary for testing, consider adding a TODO comment explaining when it should be restored. If permanent, consider removing the commented code entirely to avoid confusion.

---

63-68: LGTM! Clean switch to instant recording

The change from studio_recording::Actor::builder to instant_recording::Actor::builder is clean and aligns with the PR's objective to simplify the recording pipeline.

crates/recording/src/sources/screen_capture/windows.rs (5)

55-57: LGTM! Well-structured FPS control fields

The addition of target_fps, last_timestamp, and timestamps fields provides a clean foundation for implementing FPS limiting without relying on hardcoded intervals.

---

140-151: Verify FPS limiter tolerance calculation

The FPS limiter uses an 80% tolerance (20% early arrival). This means frames can arrive up to 20% earlier than the target interval. Ensure this tolerance aligns with your requirements - it might be too permissive for precise frame rate control.

Consider if the tolerance should be configurable or if a tighter tolerance (e.g., 5-10%) would be more appropriate for consistent frame rates.

---

254-256: LGTM! Proper initialization of FPS control state

The FPS control fields are correctly initialized using the configured FPS value and current timestamps.

---

285-288: Remove obsolete min_update_interval setting

Good job removing the hardcoded min_update_interval in favor of the new FPS limiter implementation. This aligns perfectly with the PR objective.

---

431-431: LGTM! Clean message passing with new NewFrame structure

The simplified NewFrame structure is correctly used in the frame handler callback.