fix: stop avfoundation mp4 writes when paused

[richiemcilroy]

• ensure the MP4 encoder clamps the final timestamp when finishing during an active pause so we don’t enqueue a synthetic frame past the pause gap
• reset pause state before flushing the asset writer to keep shutdown paths consistent

Summary by CodeRabbit

• Bug Fixes
  • Improved video finalization with enhanced timestamp handling when pause states are active. The encoder now accurately computes final timestamps and applies stricter validation before queuing the last frame, ensuring more precise video recording completion.

[coderabbitai]

Walkthrough

The finish method in the MP4 encoder is modified to compute a finish_timestamp by considering both the provided timestamp and any existing pause state. The pause state is cleared after obtaining the most recent frame, and the final frame extension logic uses the computed timestamp to conditionally queue the last frame only when the time difference exceeds 500 milliseconds.

Changes

Cohort / File(s)	Change Summary
MP4 encoder finish method crates/enc-avfoundation/src/mp4.rs	Modified finish method to compute finish_timestamp by clamping the provided timestamp to the pause point if one exists, clear pause state after retrieving the most recent frame, and conditionally extend the video by queuing the last frame only when the time difference from the most recent frame exceeds 500ms

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

• Single-file, focused modification to the finish method
• Timestamp computation logic with conditional frame queueing
• State management changes (clearing is_paused and pause_timestamp) are straightforward
• Worth confirming the timestamp clamping logic and the 500ms threshold are intentional

Possibly related PRs

• feat: Ensure monotonic PTS for video and audio frames (fixes instant mode pausing) #1240: Overlaps on pause/timestamp state handling in the same finish method, modifying how pause timestamps are tracked and cleared during finalization
• desktop: account for screen capture not providing frames #1219: Also modifies the finish method's timestamp computation and conditional frame queueing logic with similar 500ms threshold checks

Poem

🐰 A rabbit hops through frames so fine,
Pausing time at just the right line,
Timestamps dance, five hundred milliseconds wait,
Clear the pause, let the final frame create,
bink bink ✨ — the video's fate!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.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 "fix: stop avfoundation mp4 writes when paused" directly addresses the main objective of the changeset, which is to prevent the MP4 encoder from writing synthetic frames past the pause point by clamping the final timestamp when paused. The title is specific and descriptive, mentioning the affected component (avfoundation), the type of operation (mp4 writes), and the key condition (when paused). It is concise, clear enough for a teammate scanning history to understand the primary change, and avoids vague or misleading language.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-timestamp

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 612ef9e and f5fcca4.

📒 Files selected for processing (1)

• crates/enc-avfoundation/src/mp4.rs (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Format Rust code using rustfmt and 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/enc-avfoundation/src/mp4.rs

crates/*/src/**/*

📄 CodeRabbit inference engine (AGENTS.md)

Rust crates should place tests within the src/ and/or a sibling tests/ directory for each crate inside crates/*.

Files:

• crates/enc-avfoundation/src/mp4.rs

🧠 Learnings (2)

📓 Common learnings

Learnt from: Brendonovich
PR: CapSoftware/Cap#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.

📚 Learning: 2025-10-17T05:58:22.586Z

Learnt from: Brendonovich
PR: CapSoftware/Cap#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/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 (3)

crates/enc-avfoundation/src/mp4.rs (3)

455-462: LGTM! Timestamp clamping logic is correct.

The finish_timestamp computation correctly ensures that when finishing during an active pause, the timestamp is clamped to the pause point. The logic handles all cases appropriately:

• When both timestamps exist, it uses the minimum
• When only pause_timestamp exists, it becomes the finish timestamp
• When not paused, the original timestamp is used unchanged

This aligns perfectly with the PR objective to prevent synthetic frames from being enqueued past the pause gap.

---

469-470: LGTM! Pause state reset is correctly positioned.

Clearing the pause state at this point is correct and ensures consistent shutdown behavior. The placement is deliberate:

• After computing finish_timestamp (which may have used pause_timestamp)
• Before the potential queue_video_frame() call, preventing the early return on line 223 from blocking the final frame
• Guarantees clean state regardless of whether the extension is performed

This fulfills the PR objective to "reset pause state before flushing the asset writer."

---

473-485: LGTM! Using finish_timestamp correctly prevents extension past pause.

This is the crucial change that ensures proper handling of the final frame. By using finish_timestamp (which may have been clamped to pause_timestamp), the extension logic now respects the pause boundary.

The approach is sound:

• If paused, extension is limited to the pause point due to earlier clamping
• If not paused, behaves as before
• The 500ms threshold check still applies appropriately

This is consistent with the retrieved learning that "the video should not be extended beyond the pause point" while improving the implementation to handle edge cases more robustly.

Based on learnings

---

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.}