fix(compositor): live preview holds the current frame instead of consuming one per tick - #227
Open
eduumach wants to merge 2 commits into
Open
fix(compositor): live preview holds the current frame instead of consuming one per tick#227eduumach wants to merge 2 commits into
eduumach wants to merge 2 commits into
Conversation
…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>
📝 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 | ✅ 5✅ Passed checks (5 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
Comment on lines
+266
to
+293
| /// Décode la prochaine frame dans le buffer de lookahead et renvoie son temps (s) — | ||
| /// `None` à EOF. Cf. `pipeline_macos::Decoder::peek_next_time_sec`. | ||
| pub unsafe fn peek_next_time_sec(&mut self) -> Result<Option<f64>> { | ||
| if !self.has_peek { | ||
| if !self.receive_into(self.peek_frame)? { | ||
| return Ok(None); | ||
| } | ||
| self.has_peek = true; | ||
| } | ||
| let pts = (*self.peek_frame).best_effort_timestamp; | ||
| Ok(Some(if pts == i64::MIN { | ||
| 0.0 | ||
| } else { | ||
| pts as f64 * self.stream_timebase | ||
| })) | ||
| } | ||
|
|
||
| /// Promeut la frame de lookahead au rang de frame courante. Cf. | ||
| /// `pipeline_macos::Decoder::commit_peek`. | ||
| pub unsafe fn commit_peek(&mut self) -> Result<*mut AVFrame> { | ||
| debug_assert!(self.has_peek, "commit_peek sans peek_next_time_sec préalable"); | ||
| std::mem::swap(&mut self.frame, &mut self.peek_frame); | ||
| self.has_peek = false; | ||
| let pts = (*self.frame).best_effort_timestamp; | ||
| self.cur_pts = if pts == i64::MIN { None } else { Some(pts) }; | ||
| Ok(self.frame) | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
commit_peek protects its precondition with debug_assert! only. All three decoders expose commit_peek as a public promotion API and check has_peek with debug_assert!, which compiles out in release builds. Without a prior successful peek_next_time_sec, the pointer swap promotes an unfilled or stale AVFrame, sets cur_pts from an undefined best_effort_timestamp, and passes that frame to the presentation path.
crates/compositor/src/linux_decode.rs#L266-L293: replace thedebug_assert!inSwDecoder::commit_peekwith abail!whenhas_peekis false.crates/compositor/src/pipeline_macos.rs#L399-L412: replace thedebug_assert!inDecoder::commit_peekwith an error return beforecpu.present(self.frame)runs.crates/compositor/src/pipeline_windows.rs#L786-L798: replace thedebug_assert!inDecoder::commit_peekwith an error return beforecpu.present(self.frame)runs.
📍 Affects 3 files
crates/compositor/src/linux_decode.rs#L266-L293(this comment)crates/compositor/src/pipeline_macos.rs#L399-L412crates/compositor/src/pipeline_windows.rs#L786-L798
🤖 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/linux_decode.rs` around lines 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.
Comment on lines
+248
to
+250
| // Tout seek invalide un éventuel peek en attente : il portait sur "la frame après | ||
| // l'ancienne position courante", qui n'a plus de sens une fois qu'on a sauté ailleurs. | ||
| self.has_peek = false; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
rewind does not invalidate the pending peek. Both decoders document and apply the rule "any seek invalidates a pending peek" in seek_to, but rewind performs the same av_seek_frame plus avcodec_flush_buffers without clearing has_peek. After a rewind, the next next() call returns commit_peek() and promotes a frame decoded at the pre-rewind position, with its old cur_pts.
crates/compositor/src/pipeline_macos.rs#L248-L250: addself.has_peek = false;at the start ofDecoder::rewind, matching thisseek_toguard.crates/compositor/src/pipeline_windows.rs#L602-L603: addself.has_peek = false;at the start ofDecoder::rewind, matching thisseek_toguard.
📍 Affects 2 files
crates/compositor/src/pipeline_macos.rs#L248-L250(this comment)crates/compositor/src/pipeline_windows.rs#L602-L603
🤖 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/pipeline_macos.rs` around lines 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.
Comment on lines
51
to
60
| loop { | ||
| if decoder.cur_frame().is_null() { | ||
| return Ok(false); | ||
| } | ||
| if decoder.cur_time_sec() + timeline_offset_sec >= target_source_time { | ||
| return Ok(true); | ||
| } | ||
| if decoder.next()?.is_null() { | ||
| return Ok(false); | ||
| let next_time = match decoder.peek_next_time_sec()? { | ||
| Some(t) => t, | ||
| None => return Ok(true), // EOF : plus rien à décoder, on tient la dernière frame connue. | ||
| }; | ||
| if next_time + timeline_offset_sec > target_source_time { | ||
| return Ok(true); // la frame suivante n'est pas encore due : hold sur la courante. | ||
| } | ||
| decoder.commit_peek()?; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the sentinel and its consumers.
rg -nP -C 6 'i64::MIN' --type=rust crates/compositor/src
rg -nP -C 4 'peek_next_time_sec' --type=rust
# Check the index-based fallback that this path bypasses.
rg -nP -C 8 'fn cur_time_sec' --type=rust crates/compositor/srcRepository: getopenscreen/openscreen
Length of output: 17492
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== timeline_walk outline =="
ast-grep outline crates/compositor/src/timeline_walk.rs --view expanded || true
echo "== timeline_walk relevant lines =="
sed -n '1,140p' crates/compositor/src/timeline_walk.rs
echo "== live step relevant lines =="
sed -n '360,395p' crates/compositor/src/live.rs
echo "== Decoder trait/interface =="
rg -n -C 8 'trait Decoder|pub.*trait|peek_next_time_sec|commit_peek|cur_time_sec' crates/compositor/src --glob '*.rs'
echo "== timeline_walk callers =="
rg -n -C 5 'advance_decoder_to|timeline_walk' crates/compositor/src --glob '*.rs'Repository: getopenscreen/openscreen
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== timeline_walk output loop excerpt =="
sed -n '78,220p' crates/compositor/src/timeline_walk.rs
echo "== Decoder trait declaration =="
rg -n -C 10 '^pub (?:trait|enum) Decoder|^pub (?:struct|enum) Decoder' crates/compositor/src --glob '*.rs' | head -n 120
echo "== peek_next_time_sec only usages =="
rg -n -C 3 'peek_next_time_sec\(\)' crates/compositor/src --glob '*.rs'
echo "== cursor/last frame source references =="
rg -n -C 4 'last_frame|frame.*last|Frame|Frame|cur_frame\(\)|compose_frame' crates/compositor/src/compositor.rs crates/compositor/src/linux_decode.rs crates/compositor/src/pipeline_macos.rs crates/compositor/src/pipeline_windows.rs crates/compositor/src/timeline_walk.rs crates/compositor/src/live.rs --glob '*.rs' | head -n 220Repository: getopenscreen/openscreen
Length of output: 31856
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pipeline_windows peek_next_time_sec =="
sed -n '755,788p' crates/compositor/src/pipeline_windows.rs
echo "== pipeline_windows next and commit =="
sed -n '540,570p' crates/compositor/src/pipeline_windows.rs
sed -n '720,740p' crates/compositor/src/pipeline_windows.rs
echo "== timeline_walk remaining tail =="
sed -n '220,280p' crates/compositor/src/timeline_walk.rs
echo "== deterministic behavior probe for sentinel comparison and loop commit count =="
python3 - <<'PY'
from itertools import count
def advance_decoder_to_behavior(next_times, target_source_time, timeline_offset_sec):
commits = 0
while True:
try:
next_time = next_times.pop(0)
except IndexError:
return commits, "EOF"
if next_time + timeline_offset_sec > target_source_time:
return commits, "held"
commits += 1
cases = [
("unknown_sentinel_with_positive_target", [0.0, 0.0, 0.0], 10.0, 0.0),
("zero_timestamp_not_exceeding_nonzero_target", [0.0, 0.0, 0.0], 0.0, 0.0),
("zero_timestamp_not_exceeding_zero_target", [0.0, 0.0, 0.0], 0.0, 0.0),
("real_future_frames_held", [11.0, 12.0, 13.0], 5.0, 0.0),
("real_past_frames_consumed", [-1.0, -0.5, 0.4], 5.0, 0.0),
]
for name, times, target, offset in cases:
commits, reason = advance_decoder_to_behavior(times.copy(), target, offset)
print(f"{name} target={target} offset={offset}: commits={commits} reason={reason}")
PYRepository: getopenscreen/openscreen
Length of output: 4381
Keep unknown timestamps out of the peek-time comparison.
peek_next_time_sec() returns Some(0.0) when best_effort_timestamp == i64::MIN or when tb_sec <= 0.0 on macOS/Windows. In advance_decoder_to and live::Player::step, that value satisfies the commit condition for non-due targets, so those callers can stream through pending frames to EOF instead of holding the current frame. Return Option<Option<f64>> or another explicit sentinel for missing timestamps, and advance at most one frame when no usable PTS is available.
🤖 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/timeline_walk.rs` around lines 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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