Skip to content

Screen stream (WebRTC): skip unchanged frames (Petal's dirty-rect gate), 1 s keep-alive - #36065

Closed
teamleaderleo wants to merge 8 commits into
feat-cmux-nextfrom
lane/stream-webrtc-tune-dirty-skip
Closed

teamleaderleo wants to merge 8 commits into
feat-cmux-nextfrom
lane/stream-webrtc-tune-dirty-skip

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

Leo, 2026-11-05: "honestly i'd rather us look at lot more at Petal and stuff like we ought to be able to get a lot more efficient and start going towards crazy stuff... after being able to do the basics end to end and adding eval measurements, ofc", and on the Petal list: "obv do all these and more".

HQ#7146 item 3 of the Petal ports (WebRTC path, CmuxVNCWebRTC): the static-screen half. The low-QP or lossless still once the screen goes static is a follow-up.

What changes

  • Dirty-rect skip gate (ScreenCaptureBackend). A complete ScreenCaptureKit frame whose dirtyRects attachment is empty, at the same size and with the pointer where it was, no longer goes to the encoder. A missing attachment counts as changed (Petal's "unknown is always pushed"), and so do the first frame after a capture start, a size change and a pointer move (the cursor is drawn into the picture and may not count as dirty). Ported from Petal (https://github.com/kiruna-labs/petal @ 7ac436a, Apache-2.0, apps/desktop/src-tauri/src/session/share.rs:2060-2290).
  • Keep-alive once a second, not twice (ScreenStreamHost). The idle refresh re-sent the last frame every 500 ms so a lost keyframe or a quality step had a frame to encode; it now does so after 1 s without a frame (Petal's static refresh floor).
  • The host's 3 s video: log line adds unchanged-skipped N fps, so a run shows how often the gate fires.

Numbers

Before and after from screen-stream-g0's harness (scripts/measure/screen-stream-eval.sh, webrtc target; idle kbps and fps are the ones this should move, typing/scroll/latency should not get worse): pending. The harness is cmux#36043, not yet on feat-cmux-next. This PR stays a draft until the numbers are in.

Tests

FrameRateTests.unchangedFramesStayOutOfTheEncoder (new).

Observed / Not run

  • Prepush gate: no new violations.
  • Not run yet: the package tests on the fleet, and the harness.

Changelog

  • Changed: a still Mac screen streamed to the iPhone sends fewer frames.

🤖 Generated with Claude Code
Agent session: agent session


Autofix with [code]smith
Open in [code]smith · Need help on this PR? Tag @codesmith-bot with what you need.


Summary by cubic

Skips unchanged frames in the WebRTC screen stream so a still Mac screen stops sending redundant frames, and refreshes it just enough to keep text sharp: every 250 ms for the first 2 s after the screen settles, then once a second (the 1 s floor alone blurred text, 17.8 % vs 11.2 % OCR error).

  • Complete frames with zero dirty rects, the same size, and an unchanged pointer are dropped before the encoder; unknown dirty rects, the first frame after a capture start, size changes, pointer moves, and the frame after the pointer comes to rest still go through.
  • Only the installed stream's frames pass; installing a stream resets the gate and held frame, so a new stream's first frame is never compared with an earlier one, and frames a stopping stream sends while it winds down are dropped.
  • The backend keeps the newest skipped frame and the keep-alive sends it, so a cursor the gate missed shows instead of the last frame sent.
  • The 3 s video log line now reports how many frames were skipped.

Written for commit a6718c6. Summary will update on new commits.

View guided diff Turn on auto-fix

…eep-alive

Petal's dirty-rect skip gate (session/share.rs, Apache-2.0): a complete
SCK frame with zero dirty rects, the same size and the pointer where it
was stays out of the encoder; unknown dirty rects count as changed. The
idle refresh that re-sent the last frame every 500 ms now does so once a
second. The host's 3 s video log line counts the skipped frames.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Session: https://cmux.com/deeplink/agent?id=aid-29da0cfaa02255d6b3d5
…et before a capture starts

Review of #36065: the pointer is read as a frame arrives, so the frame
where the cursor came to rest could be skipped and the viewer kept it a
few pixels off. A frame now also goes out when the pointer differs from
the frame just before. The first-frame reset moves before startCapture,
so a new stream's first frame is never compared with the old one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Session: https://cmux.com/deeplink/agent?id=aid-29da0cfaa02255d6b3d5
…er the old stream stops

Review at 5fbe987: a frame captured mid-move but read after the pointer
settled let the gate skip the frame that showed the cursor at rest, and the
keep-alive resent the stale one. The backend now keeps the newest skipped
frame and the keep-alive sends it, so a missed cursor shows within a second.
A display switch also clears the gate again once the old stream stops.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Session: https://cmux.com/deeplink/agent?id=aid-29da0cfaa02255d6b3d5

@teamleaderleo teamleaderleo left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review (claude-opus-5-5): SHIP at 34ecf05

Independent review subagent (Claude Opus 5.5, medium): SHIP at c92fb1b after two rounds (stale-cursor finding fixed by sending the newest held frame on the keep-alive; display-switch reset after the old stream stops; held buffer pins at most one SCK surface of 4–6; locks never nested). 34ecf05 only moves two doc-comment lines onto takeSkippedUnchangedFrames (delta read directly). Compile check CmuxVNCWebRTC at c92fb1b running; previous heads passed.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Review (codex gpt-6-astra high, router-review): BLOCK at 34ecf05

BLOCK at 34ecf05

Visual verdict: no images; Leo tries the build

  • [P1] [blocking] [category:tooling] [finding:F1] [outcome:pending] [ScreenStream.swift:473] keepAliveMilliseconds is declared as UInt64, but Duration.milliseconds accepts signed integer or floating-point values. Passing it directly prevents the package from compiling; convert it to Int64 or make the constant an Int64.

  • [P1] [blocking] [category:reliability] [finding:F2] [outcome:pending] [ScreenCaptureBackend.swift:256] The callback ignores its stream argument while updating the shared dirty-frame state. A queued callback from the old stream can run after the post-stop reset, seed lastDelivered, and cause the new stream’s first zero-dirty frame (same size and pointer) to be skipped. During same-sized display switches this can leave the viewer on the old display until the one-second keep-alive; reject callbacks from non-current streams or associate state with the capture generation.

Codex review at 34ecf05: a frame the previous stream sent while it stopped
could seed lastDelivered after the reset and make the new display's first
unchanged frame skip. The backend now keeps the stream it delivers from and
drops frames from any other (restored to the running one when a start fails).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Session: https://cmux.com/deeplink/agent?id=aid-29da0cfaa02255d6b3d5
@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Replies to the router review at 34ecf05:

  • F1 (Duration.milliseconds with a UInt64): this isn't a defect. Duration.milliseconds(_:) has a generic BinaryInteger overload, so a UInt64 argument compiles, and the package compile check passed at c92fb1b with this exact line.
  • F2 (callbacks from the old stream): fixed in 50be71a. The backend now records the stream it delivers from (delivering, set before startCapture()) and drops every frame from any other stream. A frame the previous stream sends while it stops can no longer reach the encoder or seed lastDelivered/held. If a start fails, delivering returns to the running stream, and the post-stop reset is no longer needed.

@blacksmith-sh

blacksmith-sh Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

Found 3 test failures on Blacksmith runners:

Failures

Test View Logs
nightly cmux-tui resolve/nightly cmux-tui resolve View Logs
test_every_compile_job_names_its_warm_sources (main.ThisTree.test_every_compile_job
_names_its_warm_sources)/
test_every_compile_job_names_its_warm_sources (main.ThisTree.test_every_compile_job
_names_its_warm_sources)
View Logs
the six-hour schedule must allow 90 minutes for a cold cache build and use the matching
runner, Xcode, and visible timing output/
the six-hour schedule must allow 90 minutes for a cold cache build and use the matching
runner, Xcode, and visible timing output
View Logs

Fix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

@github-actions

github-actions Bot commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

cmux status (one comment, edited in place; each line is updated by the job that owns it)

web and size checks: green

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

For the owning lane (stream-webrtc-tune): this branch predates #36043, so its head has no scripts/measure/screen-stream-eval.sh. The screen-stream eval is now admitted at PR heads, but at this head the fleet step is refused with "script not found". To get the after numbers, bring feat-cmux-next into this branch. The script then matches the reviewed feat-cmux-next copy and the eval can run here. This PR doesn't touch the eval harness, so no conflict is expected there.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Review (codex gpt-6-astra high, router-review): BLOCK at 50be71a

BLOCK at 50be71a

Visual verdict: no images; Leo tries the build

  • [P1] [blocking] [category:tooling] [finding:F1] [outcome:pending] Packages/Shared/CmuxVNCWebRTC/Sources/CmuxVNCWebRTC/ScreenStream.swift:473: keepAliveMilliseconds is UInt64, but Duration.milliseconds requires an Int64 argument. This does not compile; use a compatible integer type or cast explicitly.

  • [P1] [blocking] [category:reliability] [finding:F2] [outcome:pending] Packages/Shared/CmuxVNCWebRTC/Sources/CmuxVNCWebRTC/ScreenCaptureBackend.swift:237: If capture B fails while an earlier capture A is still starting, B restores delivering to the old active stream. A can then become the installed stream while its callbacks are rejected by the stale token, freezing the stream until another restart. Restore the token only if no newer capture has claimed it.

…sleep

Codex review at 50be71a: a capture that failed while an earlier one was
still starting restored the running stream's token, so the earlier one,
once installed, had its frames dropped. Installing a stream now makes it the
delivering one (the gate starts over when that moves it). The keep-alive's
Duration.milliseconds also takes an explicit Int64.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Session: https://cmux.com/deeplink/agent?id=aid-29da0cfaa02255d6b3d5
@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Review (codex, router-review): SHIP at 4bb1350

Reviewer: codex gpt-6.1-sol xhigh, fast tier.

Follow-ups: 1. They land as checklist items in this PR's one follow-up tracking issue in this repo (Follow-ups: #N ..., label follow-up; add to an open one before filing, never one issue per finding), linking this receipt; the lane that opened the PR fixes them forward.

SHIP at 4bb1350

Visual verdict: no images; Leo tries the build

  • [P2] [follow-up] [category:tests] [finding:F1] [outcome:pending] [FrameRateTests.swift:154] The tests cover only the pure helper functions; they do not exercise sample-buffer attachment handling, stream-token resets, held-frame consumption, or the host’s one-second keep-alive timing. Add coverage for the actual capture/host path or suitable test seams.

Follow-ups:

  • [finding:F1] Add integration or seam-based coverage for the new capture and keep-alive behavior.

…n once a second

The G0 eval (cmux-austin-mini-0, feat-cmux-next 94aa138, three runs each) showed
the 1 s keep-alive cost text sharpness: OCR error 17.8 % against 11.2 % before,
while idle fell from 76 to 63 kbps. The encoder sharpens a still picture with
each frame, so the picture now goes again every 250 ms for the first 2 s after
capture last sent one, then once a second.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Session: https://cmux.com/deeplink/agent?id=aid-29da0cfaa02255d6b3d5
@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Numbers, and why this closes

From the G0 screen-stream eval (webrtc target), on one measurement Mac (M4, 1920×1080), with host and viewer on the same machine. Every run is at feat-cmux-next 94aa138, or this branch merged with it. The Mac drifted about 6 ms in latency over the day, so the table pairs each variant with the baselines that ran near it.

Variant (runs) Idle kbps (fps) OCR error Freezes Key / click p50
Baseline, early (3) 76.1 (1.9) 7.9–11.3 % 17 39.9 / 39.5 ms
Skip gate + 1 s keep-alive, this PR at 4bb1350 (3) 63.2 (1.0) 16.2–19.7 % 17–18 40.5 / 39.1 ms
+ 250 ms settle refresh for 2 s, a6718c6 (4) 61.9 (1.1) 10.3–15.7 % 26–28 46.9 / 45.9 ms (late baseline 46.3 / 48.2)
Skip gate alone, old 500 ms keep-alive (2) 79.4 (1.9) 8.7–9.0 % 15–16 45.0 / 46.2 ms (late baseline 46.3 / 48.2)

What this shows:

  • The dirty-rect gate does nothing measurable here. On its own it gives the same idle traffic, frame rate, text and latency as the baseline. ScreenCaptureKit already sends no complete frame for a still screen; it marks those frames .idle, which the backend already drops. So the baseline's 1.9 fps while idle is just the 500 ms keep-alive.
  • All of the idle saving came from the slower keep-alive, and it costs text sharpness. The encoder sharpens a still picture with each frame it gets. Fewer refreshes leave text blurrier (OCR error 16–20 % against 8–11 %). The settle refresh got most of the sharpness back, but added about 10 freezes per run.
  • The saving is about 15 kbps while idle. That is too small to buy with blurrier text or more freezes.

So nothing here is worth landing: the gate has no measured effect and the keep-alive change is a net loss. A low-QP or lossless refresh once the screen goes static (Petal's next step) is the better route to sharp, cheap idle frames. It needs encoder control that the Objective-C SDK doesn't expose, so it stays a note on cmuxterm-hq#7146 for now. The branch stays as it is in case a capture source turns up that sends complete frames with no dirty rects.

This branch was previously deployed

1 inactive deployment
ci-routing-agent — a6718c67 Deployed Nov 5, 2026 by teamleaderleo via cmux-tui tree preflight #117039
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk:normal Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant