Skip to content

fix: hold last frames across source restarts — the placeholder never flashes on scene switches - #244

Open
TheOrcDev wants to merge 1 commit into
mainfrom
fix/hold-frames-across-source-restarts
Open

TheOrcDev wants to merge 1 commit into
mainfrom
fix/hold-frames-across-source-restarts

Conversation

@TheOrcDev

@TheOrcDev TheOrcDev commented Aug 20, 2026

Copy link
Copy Markdown
Owner

The orange flash (owner screenshots, camera ↔ screen+camera)

The "colors" are finally identified — all three rounds of them. They were never buffer corruption: they're the compositor's own missing-source placeholders. missing_source_placeholder_bgra paints a 64px dark texture with an accent border + marching diagonal — accent BGRA [0,160,255] = orange for Screen/Window, [255,0,255] = magenta for Camera. Scaled to canvas size, that's exactly the blocky orange staircase in the owner's screenshots (and the magenta band from the 0.9.63 report).

Why it flashed on switches

CompositorLiveSources cleared its held last frame whenever a source slot went empty — and a slot goes empty during every stop/restart interval: the screen source is retired while in camera-only and restarted on the way back; the camera slot empties during geometry restarts. The warm gate genuinely waits for frame evidence before committing the scene, but the render loop re-adopts sources via try-lock and can lag several ticks behind the commit. With the held frame already thrown away, those ticks rendered the placeholder into the preview, stream, and recording. This silently defeated the documented contract — "the compositor holds the last frame across the restart, so the swap is a brief hold, not a blank slot" — and retroactively explains the magenta camera episodes too.

Fix

adopt_camera_source / adopt_screen_source now remember the key of the source that produced the held frame:

  • Empty slot → HOLD. The last real picture bridges restart/retire intervals.
  • Different key → drop. Switching to another display/camera never shows the old device's picture.
  • The placeholder still renders for sources that have never delivered a frame — it stays an honest diagnostic, it just can't flash transiently anymore.
  • Camera staleness semantics unchanged (frames past the cutoff still refuse to render or count as recording-barrier evidence; screen frames render at any age — that was already the doctrine, holding across retire matches it).

Gates

  • cargo test: 1513 pass (2 new tests pin the hold contract: empty-slot hold + same-key restart keeps the bridge; different-key drops)
  • clippy -D warnings · fmt --check: clean
  • smoke:recording-matrix: 12/12 · smoke:live-layout-switch-recording (transitions on): PASS
  • smoke:layout-source-loop fails identically on clean main (verified via stash) — pre-existing, tracked separately.

Owner acceptance

Switch camera ↔ screen+camera repeatedly (motion on and off, idle and while recording): the screen region must always show a real desktop picture — worst case a briefly frozen one — never the orange pattern; the camera must never flash magenta.

Summary by CodeRabbit

  • Bug Fixes

    • Live camera and screen previews now retain the last available frame during temporary interruptions, including stop and restart intervals.
    • Retained frames are cleared when the source identity changes, preventing outdated content from being displayed.
  • Tests

    • Added coverage for frame retention across restarts and clearing frames when sources change.

…e placeholder flashes on scene switches

Owner-reported (third round of "those colors"): switching camera <->
screen+camera sometimes flashed a blocky orange pattern over the screen
region of the live output.

Root cause, finally unified: the "colors" were never buffer corruption —
they are the compositor's animated missing-source placeholders
(missing_source_placeholder_bgra: accent BGRA [0,160,255] = ORANGE for
screen/window, [255,0,255] = MAGENTA for camera; a 64px texture whose
border + marching diagonal scale up into exactly the blocky pattern in
the owner's screenshots).

Why they showed: CompositorLiveSources cleared its held last frame
whenever a source slot went EMPTY — and a slot goes empty during every
stop/restart interval (camera geometry restarts; the screen source is
retired on camera-only and restarted on the way back). The warm gate
does wait for real frame evidence before committing, but the render
loop re-adopts sources via try-lock and can lag by several ticks — and
with the held frame thrown away, those ticks rendered the placeholder
straight into the preview, the stream, and the recording. This defeated
the documented "the compositor holds the last frame across the restart"
contract and explains the earlier magenta episodes too.

Fix: adopt_camera_source/adopt_screen_source remember the key of the
source that produced the held frame. An empty slot HOLDS the frame (the
bridge picture); only a source with a genuinely DIFFERENT key drops it
(switching displays never shows the old display's picture). The
missing-source placeholder still renders for sources that never
delivered a frame — it remains an honest diagnostic, just never a
transient flash. Camera staleness semantics are unchanged (frames past
the cutoff still refuse to render or count as barrier evidence).

Tests: two new unit tests pin the hold contract (empty slot holds
across the interval + same-key restart keeps the bridge; a different
display key drops it). Test-only frame-source constructors added.

Gates: cargo 1513 + clippy + fmt; smoke:recording-matrix 12/12;
smoke:live-layout-switch-recording PASS (transitions on).
(smoke:layout-source-loop fails identically on clean main —
pre-existing, verified via stash.)
@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 78ec42c3-6665-42c2-898e-55bd19d62d66

📥 Commits

Reviewing files that changed from the base of the PR and between dc765d5 and 8a5149a.

📒 Files selected for processing (3)
  • crates/videorc-backend/src/compositor.rs
  • crates/videorc-backend/src/preview_camera.rs
  • crates/videorc-backend/src/preview_screen.rs

Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The compositor now tracks camera and screen source keys. It retains the last frame during temporary source unavailability and same-key restarts. It clears the held frame when a different source key appears. Test-only constructors support regression coverage without live source slots.

Changes

Source frame retention

Layer / File(s) Summary
Source identity and adoption
crates/videorc-backend/src/compositor.rs
CompositorLiveSources tracks camera and screen source keys. Dedicated adoption methods preserve frames for empty slots and same-key restarts, and clear frames for different source keys. Obsolete comparison helpers were removed.
Retention test support and regression coverage
crates/videorc-backend/src/preview_camera.rs, crates/videorc-backend/src/preview_screen.rs, crates/videorc-backend/src/compositor.rs
Test-only constructors create keyed preview sources without live slots. Regression tests cover camera and screen frame retention and clearing after source replacement.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 8a514

The change preserves the last real frame across source restarts while dropping frames from different devices, preventing transient placeholders during switches; no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant SourceSlot
  participant Compositor
  participant CompositorLiveSources
  participant FrameStore
  SourceSlot->>Compositor: report source key or empty slot
  Compositor->>CompositorLiveSources: adopt camera or screen source
  CompositorLiveSources->>FrameStore: retain frame for empty or same-key source
  CompositorLiveSources->>FrameStore: clear frame for different source key
Loading

Suggested reviewers: petercr

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 46.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: retaining frames across source restarts to prevent placeholder flashes during scene switches.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/hold-frames-across-source-restarts

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

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant