desktop: account for screen capture not providing frames #1219
Conversation
WalkthroughMP4Encoder now stores the most recent video frame alongside its timestamp and accepts owned frame values. queue_video_frame and finish signatures changed; finish can take an optional timestamp and may queue a final frame. The Muxer trait and its implementations were updated to accept a timestamp passed through the recording pipeline. Changes
Sequence Diagram(s)sequenceDiagram
participant Recorder
participant Pipeline
participant Muxer
participant Encoder as MP4Encoder
Recorder->>Pipeline: queue_video_frame(frame)
activate Pipeline
Pipeline->>Muxer: queue_video_frame(frame)
activate Muxer
Muxer->>Encoder: queue_video_frame(frame: arc::R<SampleBuf>, timestamp)
Encoder->>Encoder: most_recent_frame = Some((frame.clone(), timestamp))
deactivate Muxer
deactivate Pipeline
Recorder->>Pipeline: finish()
activate Pipeline
Note over Pipeline: elapsed = timestamps.instant().elapsed()
Pipeline->>Muxer: finish(elapsed)
activate Muxer
Muxer->>Encoder: finish(Some(elapsed))
activate Encoder
Note over Encoder: diff = elapsed - most_recent_frame.timestamp
alt diff > 500ms
Encoder->>Encoder: attempt to queue final frame at elapsed
alt queue success
Encoder->>Encoder: update most_recent_frame with queued timestamp
else queue error
Note right of Encoder: log error
end
end
Encoder->>Encoder: compute end_time = most_recent_frame.1 - offset
deactivate Encoder
deactivate Muxer
deactivate Pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
crates/*/src/**/*📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📓 Common learnings🧬 Code graph analysis (1)crates/recording/src/output_pipeline/win.rs (4)
⏰ 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 (1)
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: 3
🧹 Nitpick comments (1)
crates/enc-avfoundation/src/mp4.rs (1)
377-379: End time based on most_recent_frame.1 minus offset — consistentMatches the timebase used at session start (ms). Consider documenting the 1000 time scale choice for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/enc-avfoundation/src/mp4.rs(8 hunks)crates/recording/src/output_pipeline/core.rs(2 hunks)crates/recording/src/output_pipeline/ffmpeg.rs(2 hunks)crates/recording/src/output_pipeline/macos.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/output_pipeline/ffmpeg.rscrates/recording/src/output_pipeline/core.rscrates/recording/src/output_pipeline/macos.rscrates/enc-avfoundation/src/mp4.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.rscrates/recording/src/output_pipeline/core.rscrates/recording/src/output_pipeline/macos.rscrates/enc-avfoundation/src/mp4.rs
🧬 Code graph analysis (4)
crates/recording/src/output_pipeline/ffmpeg.rs (6)
crates/enc-avfoundation/src/mp4.rs (1)
finish(350-394)crates/recording/src/output_pipeline/core.rs (1)
finish(769-769)crates/recording/src/output_pipeline/macos.rs (1)
finish(52-58)crates/enc-ffmpeg/src/mux/mp4.rs (1)
finish(98-118)crates/recording/src/output_pipeline/win.rs (1)
finish(212-218)crates/enc-ffmpeg/src/mux/ogg.rs (1)
finish(40-46)
crates/recording/src/output_pipeline/core.rs (4)
crates/enc-avfoundation/src/mp4.rs (1)
finish(350-394)crates/recording/src/output_pipeline/ffmpeg.rs (3)
finish(68-80)finish(134-137)timestamp(22-24)crates/recording/src/output_pipeline/macos.rs (1)
finish(52-58)crates/recording/src/output_pipeline/win.rs (1)
finish(212-218)
crates/recording/src/output_pipeline/macos.rs (3)
crates/enc-avfoundation/src/mp4.rs (1)
finish(350-394)crates/recording/src/output_pipeline/core.rs (2)
finish(769-769)timestamp(750-750)crates/recording/src/output_pipeline/ffmpeg.rs (3)
finish(68-80)finish(134-137)timestamp(22-24)
crates/enc-avfoundation/src/mp4.rs (2)
crates/recording/src/output_pipeline/core.rs (2)
timestamp(750-750)finish(769-769)crates/recording/src/output_pipeline/macos.rs (1)
finish(52-58)
⏰ 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
- 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 (9)
crates/recording/src/output_pipeline/ffmpeg.rs (2)
68-80: OK to ignore timestamp in FFmpeg MP4 pathAccepting and ignoring the Duration is fine since the encoder already flushes and trailer writes finalize duration. No change requested.
Please confirm that cap_enc_ffmpeg cannot benefit from duplicating the last frame when input stalls (unlike ScreenCaptureKit). If needed later, the timestamp is now available to implement parity.
134-137: OggMuxer finish signature updated — behavior unchangedSignature aligned; finish delegates to encoder.finish() and writes trailer as before. Looks good.
crates/recording/src/output_pipeline/macos.rs (1)
77-79: Pass-by-value is correct and aligns with new encoder APISending frame.sample_buf by value matches MP4Encoder::queue_video_frame(arc::R<...>). This avoids extra borrows and fits the (frame, timestamp) caching.
crates/enc-avfoundation/src/mp4.rs (5)
19-19: Cache (frame, timestamp) — good foundation for tail extensionStoring the last frame alongside its timestamp enables safe duplication on finish. Looks good.
214-216: API change: take owned SampleBufTaking arc::Rcm::SampleBuf is appropriate; internal code clones once to store for reuse. Call sites updated accordingly.
227-227: Update most_recent_frame on every enqueueCorrectly tracks the last frame to allow duplication later.
334-340: Pause stores timestamp from last frame — OKDeriving pause point from most_recent_frame ensures accurate offset accounting across pauses.
399-400: Drop calls finish(None) — safe, idempotent cleanupPrevents duplicate work if user forgets to finish; good.
crates/recording/src/output_pipeline/core.rs (1)
352-352: Timestamp origins are consistent; no inconsistency foundVerification confirms both code paths use the same timestamp reference. Line 352 calls
timestamps.instant().elapsed()(measuring from the stored Instant to now), while lines 427 and 502 callduration_since(timestamps)(measuring from the same stored Instant to the current frame/timestamp). Both reference the sametimestamps.instantas their origin, so there is no over/under-extension risk from clock mismatch.The secondary consideration about handling pause-near-shutdown scenarios is an architectural design choice (whether to use elapsed time vs. last frame timestamp for finish), not a timestamp consistency problem.
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 (1)
crates/recording/src/output_pipeline/ffmpeg.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/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
🧬 Code graph analysis (1)
crates/recording/src/output_pipeline/ffmpeg.rs (6)
crates/recording/src/output_pipeline/macos.rs (1)
finish(52-58)crates/enc-avfoundation/src/mp4.rs (1)
finish(350-394)crates/recording/src/output_pipeline/core.rs (1)
finish(769-769)crates/enc-ffmpeg/src/mux/mp4.rs (1)
finish(98-118)crates/recording/src/output_pipeline/win.rs (1)
finish(212-218)crates/enc-ffmpeg/src/mux/ogg.rs (1)
finish(40-46)
⏰ 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 (2)
crates/recording/src/output_pipeline/ffmpeg.rs (2)
42-42: LGTM: Idiomatic unused parameter naming.Renaming the TaskPool parameter to
_clearly indicates it's unused in this implementation while still satisfying the trait signature.
68-80: LGTM: Correctly implements updated trait signature.The Duration parameter is appropriately ignored here since the underlying FFmpeg encoders don't support the timestamp-based frame extension feature implemented in the AVFoundation encoder. The frame extension to handle ScreenCaptureKit gaps is macOS-specific.
1 participant
On macOS, ScreenCaptureKit sometimes won't continue to output frames if there's only static content on screen. This accounts for that in the encoder by reusing the last received frame if the end time is > 0.5s greater than the most recent frame's timestamp. I'm not sure if this will fix issues of recordings being cut short but it will account for an edge case we're not handling.
Summary by CodeRabbit