fix(compositor): live preview holds the current frame instead of consuming one per tick - #227
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesPTS-driven playback
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RenderLoop
participant Player
participant Decoder
participant CpuFrames
RenderLoop->>Player: step(comp, cfg, target_source_time)
Player->>Decoder: peek_next_time_sec()
Decoder-->>Player: next PTS or EOF
Player->>Decoder: commit_peek() when frame is due
Decoder->>CpuFrames: present committed frame
Player-->>RenderLoop: committed frame status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/compositor/src/live.rs (1)
397-430: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftRe-base the webcam target after the webcam loops at EOF.
target_webcam_tis derived from the screen time and keeps growing. When the webcam reaches EOF, Line 419 seeks it back to 0 but leaves the target unchanged. On the nextstep, the catch-up loop restarts from t=0 and commits every frame whose pts is below the unchanged, still-large target. The webcam file is therefore re-decoded from the start on every tick, bounded only byguard > 1000.The doc comment at Lines 372-374 states that a webcam shorter than the screen is an expected case, so this path is reachable in normal playback.
Wrap the target into the webcam duration, or hold the webcam on its last frame after EOF instead of seeking to 0.
♻️ Sketch: hold instead of restart
None => { - // Fin de la webcam avant l'écran : elle boucle SEULE — l'écran - // garde sa propre position, inchangée. - wf = self.wdec.seek_to(0.0)?; + // Fin de la webcam avant l'écran : on TIENT la dernière frame. + // Reseeker à 0 ici relancerait un rattrapage complet du fichier + // à chaque tick, la cible restant calée sur le temps écran. break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/compositor/src/live.rs` around lines 397 - 430, Update the webcam EOF handling in the catch-up loop within the frame-selection logic so it does not seek to 0 while retaining the ever-growing target_webcam_t. Either wrap target_webcam_t using the webcam duration before comparison, or hold the webcam on its final frame after EOF; preserve the screen’s independent playback position and ensure subsequent steps do not re-decode the webcam from the beginning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/compositor/src/linux_decode.rs`:
- Around line 266-293: Replace the release-unsafe precondition checks in
SwDecoder::commit_peek, Decoder::commit_peek in
crates/compositor/src/pipeline_macos.rs, and Decoder::commit_peek in
crates/compositor/src/pipeline_windows.rs with immediate bail!-style error
returns when has_peek is false, before swapping or presenting the frame;
preserve normal promotion behavior when a peek exists.
In `@crates/compositor/src/pipeline_macos.rs`:
- Around line 248-250: Invalidate pending peek state at the start of
Decoder::rewind in both crates/compositor/src/pipeline_macos.rs lines 248-250
and crates/compositor/src/pipeline_windows.rs lines 602-603 by resetting
has_peek before seeking and flushing buffers, matching the existing seek_to
behavior.
In `@crates/compositor/src/timeline_walk.rs`:
- Around line 51-60: The timeline walk must distinguish missing timestamps from
a valid 0.0 timestamp. Update peek-time handling used by advance_decoder_to and
live::Player::step so unknown PTS values are represented explicitly, are
excluded from the due-time comparison, and cause at most one frame to advance
before holding rather than streaming to EOF; preserve normal commit behavior for
usable timestamps.
---
Outside diff comments:
In `@crates/compositor/src/live.rs`:
- Around line 397-430: Update the webcam EOF handling in the catch-up loop
within the frame-selection logic so it does not seek to 0 while retaining the
ever-growing target_webcam_t. Either wrap target_webcam_t using the webcam
duration before comparison, or hold the webcam on its final frame after EOF;
preserve the screen’s independent playback position and ensure subsequent steps
do not re-decode the webcam from the beginning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e8b8b0f6-aec7-456e-8418-6a15429b8ea8
📒 Files selected for processing (7)
crates/compositor/src/linux_decode.rscrates/compositor/src/live.rscrates/compositor/src/pipeline_linux.rscrates/compositor/src/pipeline_macos.rscrates/compositor/src/pipeline_windows.rscrates/compositor/src/timeline_walk.rscrates/poc-d3d/src/app.rs
…uming one per tick Free-running preview playback (and the poc-d3d harness) decoded exactly one real frame per 1/60s tick, assuming a constant ~60fps source. ScreenCaptureKit (and equivalent screen captures) only emits a frame when the screen changes, so a recording with long static stretches could contain only a few hundred real frames over its whole duration. Consuming one frame per tick regardless exhausted the stream long before elapsed wall time reached the recording's duration, so the decoder hit EOF, looped back to the start, and the preview appeared to accelerate then jump back to the beginning. Adds a peek/commit lookahead (peek_next_time_sec / commit_peek) to each platform decoder (linux, macos, windows) so a frame is only adopted once its pts is actually due; otherwise the current frame is held. live::Player::step and timeline_walk::advance_decoder_to (already correct on the export path) now share this hold semantics, and render_thread's accumulator tracks source time actually consumed instead of a fixed 1/60s step per tick. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…test it Review follow-ups on the frame-hold fix — three from CodeRabbit, three from a second read. An unusable pts no longer reads as "due" (CodeRabbit). `peek_next_time_sec` returned `Some(0.0)` when `best_effort_timestamp` is `i64::MIN` or the time_base is zero, and `0.0` satisfies the commit condition against every target — so a stream with no usable pts was drained frame after frame to EOF, which is the exact failure the hold semantics exist to prevent, only worse. The three decoders now return a `NextFrameTime` that says `At`, `Unknown` or `Eof`, and `Unknown` advances exactly ONE frame before yielding — the pre-hold behaviour, restricted to the broken stream that warrants it instead of being the general rule. `commit_peek` enforces its precondition with `bail!` instead of `debug_assert!` (CodeRabbit). Compiled out in release, the assertion let a caller promote an `AVFrame` that was never filled, with an undefined `best_effort_timestamp`, all the way into the presentation path. `rewind()` clears the pending peek (CodeRabbit). `seek_to` already did; `rewind` ran the same `av_seek_frame` + `avcodec_flush_buffers` without it, so the next `next()` promoted a frame decoded at the pre-rewind position, carrying its old `cur_pts`. Windows and macOS both. The export's EOF behaviour is now stated rather than implied. `advance_decoder_to` returning `true` at EOF (it returned `false`) stops the clip loop from breaking early, so a clip whose declared window outruns its last real pts fills that window by holding its last frame. That is what the audio already assumes — `on_clip_end` reports the clip's frame count and the audio is stretched over the DECLARED duration — so a video that stopped short used to shift the next clip's junction. The trade is real and now documented: a genuinely truncated source freezes to the end of its window instead of stopping. `advance_decoder_to` also re-checks the frame it just adopted, restoring the per-iteration invariant the entry guard used to provide. And it has tests now, which the original change did not: the decision is extracted into a pure `frame_step`, and the accumulator arithmetic into `consume_acc`, so both are exercisable without ffmpeg or a file. They cover what the bug was actually about — a 24fps source adopting 24 frames per real second, not 60 — plus the sparse-source gap, the exact-pts boundary, the webcam offset, EOF hold, and the unusable-pts case. 122/122 pass.
de55697 to
fbb3826
Compare
|
Reviewed, measured, and pushed follow-up fixes to this branch. The diagnosis is right and the approach (drive by pts, not by frame count) is the correct one — this is close to ready. The bug is bigger than the description saysThe description frames this as a ScreenCaptureKit / sparse-capture issue. It is much broader: A second, simpler repro than the original one — a plain 24 fps CFR import, no missing frames, nothing sparse about it:
60/24 = 2.5 exactly. Measured headlessly through the addon's own API: free-run for 20 s of wall clock, pause, then locate the paused frame by seeking to candidate times and pixel-comparing. The "after" match is bit-exact. A perf win the PR doesn't claim: holding means far fewer composites — 480 instead of 1201 over 20 s, so ~60% less compose + readback work on a 24 fps source. What I pushedThree findings from CodeRabbit, all real, all confirmed against the head at the time:
Plus two from my own read:
One thing that is a decision, not a defect — worth a maintainer's eye
That changes RebaseThe base moved to Verification run on the rebased branch: |
…every tick Review follow-up: a webcam track shorter than the screen — the camera stopping before the capture does, which the doc already called a normal case — put the player into permanent full-speed decoding. `target_webcam_t` is derived from the screen clock and keeps growing. At the webcam's EOF the catch-up loop seeked back to 0 and left the target untouched, so the NEXT tick restarted the catch-up from t=0 against a target still tens of minutes away and committed frames until the 1000-frame guard cut it off. Every tick. The webcam file was re-decoded end to end, forever, to display a track that had nothing left to show. Holding its last frame is both the fix and the semantics this PR is built on. Once EOF is a hold, the webcam decision is exactly the screen's at export time, so the loop now calls `frame_step` directly instead of carrying its own copy of the four cases — and `frame_step`'s tests cover this path too. 122/122 pass.
|
All four CodeRabbit findings are addressed; the three inline threads are answered and resolved. The fourth had no thread to resolve (posted outside the diff range), so here it is. The webcam EOF one was the best catch of the four
Worth noting this one predates the PR: the old I took the hold option rather than wrapping the target. Two reasons. A camera that stopped early should freeze, not jump back in time and replay — wrapping would need a real seek on every wrap to avoid holding through most of the loop anyway. And hold is the semantics this whole PR is built on: once EOF is a hold, the webcam decision becomes identical to the screen's at export time, so the loop now calls Pushed as a8cb4f6. 122/122. Where this leaves the PREverything I can verify locally is green: 122/122 tests, One gap that is not mine to close: CI has never run on this branch's current code. The run sits at |
eb788be
into
getopenscreen:release/v1.8.0
2 participants
Summary
peek_next_time_sec/commit_peeklookahead to each platform decoder (Linux, macOS, Windows) so a frame is only adopted once its pts is actually due, otherwise the current frame is held.live::Player::stepandtimeline_walk::advance_decoder_to(already correct on the export path) now share this hold semantics;render_thread's accumulator tracks source time actually consumed instead of a fixed 1/60s step per tick.poc-d3d's harness follows the same pattern.Test plan
cargo check -p openscreen-compositor -p poc-d3d— clean build, no new warningscargo test -p openscreen-compositor --lib— 112/112 passing🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes