Skip to content

fix(preview): improve browser recording quality - #217

Merged
rynfar merged 2 commits into
pylonfrom
upstream/2026-08-31-preview-recording
Aug 31, 2026
Merged

rynfar merged 2 commits into
pylonfrom
upstream/2026-08-31-preview-recording

Conversation

@rynfar

@rynfar rynfar commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Preview browser recordings serialized JPEG screenshots over IPC at a fixed 12 fps
and re-encoded them through a canvas. The result was low, uneven frame rates —
poor evidence for exactly the UI and motion work that recordings exist to capture.

Recording now uses Electron's native tab media stream: the desktop side returns a
DesktopPreviewRecordingSource (source id plus measured dimensions), and the
renderer acquires that stream and encodes with MediaRecorder, negotiating the best
available codec and saving the recorder's actual output format. Frame rate is a new
client-local setting (30 or 60 fps, default 30) under Settings → Integrations →
Browser. Hidden tabs stay offscreen and still record, so multiple tabs can capture
without opening their threads; native stream acquisition is bounded to five seconds
and disposes streams that resolve late.

Adopted from upstream pingdotgg/t3code#8839 (3958111057), cherry-picked clean.
The setting is client-local for the same reason the other preview defaults are: the
Chromium guest being captured belongs to the desktop app.

Verified: vp test run --dir apps/desktop apps/desktop/src/preview/Manager.test.ts
(71 passed); vp test run --dir apps/web apps/web/src/browser/browserRecording.test.ts apps/web/src/components/settings/SettingsPanels.logic.test.ts apps/web/src/components/settings/settingsSearch.test.ts
(56 passed); vp test run --dir packages/contracts packages/contracts/src/settings.test.ts
(65 passed); typecheck clean on @t3tools/contracts, @t3tools/web, @t3tools/desktop.

Not verified by making a real recording. This is a full pipeline swap with an
IPC return-type change (startScreencast now returns a source instead of void)
and new timeout and lifecycle failure modes, so it deserves a hands-on capture in
the desktop app before merge. Say the word and I'll run one.

Reviewed and integrated with Claude Opus 5 in Claude Code.


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

Co-authored-by: maria-rcks <254055478+maria-rcks@users.noreply.github.com>
(cherry picked from commit 3958111057c10c10350dd9c20ec2a2df00f504be)
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XL labels Aug 31, 2026
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 13.6 KiB 13.7 KiB +173 B (+1.2%) 15.1 KiB
Codex Thread snapshot wire 6.9 KiB 6.9 KiB +3 B (+0.0%) 7.3 KiB
Codex Live turn WebSocket wire 6.6 KiB 6.8 KiB +170 B (+2.5%) 7.8 KiB
Codex Live turn WebSocket decoded 57.2 KiB 58.7 KiB +1.5 KiB (+2.6%) 66.4 KiB
Codex Live turn messages 10 11 +1 (+10.0%) 21
Claude Total thread wire 13.6 KiB 13.6 KiB +10 B (+0.1%) 15.1 KiB
Claude Thread snapshot wire 6.9 KiB 6.9 KiB −8 B (−0.1%) 7.3 KiB
Claude Live turn WebSocket wire 6.6 KiB 6.6 KiB +18 B (+0.3%) 7.8 KiB
Claude Live turn WebSocket decoded 58.1 KiB 58.1 KiB 0 B (0.0%) 66.4 KiB
Claude Live turn messages 10 10 0 (0.0%) 21

Baseline: 9ade80d · PR result: e16f93e · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 109.5 KiB
  • Claude decoded thread snapshot: 110.2 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

Review follow-ups on the #8839 port.

startRecording holds the per-tab lifecycle lock across executeJavaScript and
capturePage, neither of which was bounded. A guest with a wedged main thread —
infinite loop, modal dialog, attached debugger — never settles them, so the
permit was never released and stopRecording and closeTab would block forever.
closeTab claims the tab id before taking the lock, so every later close became
a silent no-op and the webContents leaked for the life of the app. Both calls
now time out; a timed-out measure reports the existing unavailable-size error.

The encoder list also ranked av1 first. Chromium reports it supported but
encodes it in software, which at the new 60 fps option drops frames and burns
CPU on a machine already running agents. Hardware-encodable formats now win,
av1 stays as a fallback, and the explicit bitrate the old canvas path carried
is restored and scaled with frame rate.

Finally, the startup-cancellation check between stream acquisition and recorder
construction threw without releasing the capture, unlike every other failure
branch. It now goes through cleanupFailedRecordingStart.
@rynfar

rynfar commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

Adversarial review found three defects in the adopted code. All are upstream's, but they land here, so they're fixed on this branch.

1. Recording startup could permanently wedge a tab (serious). startRecording holds the per-tab lifecycle lock across wc.executeJavaScript(...) and wc.capturePage(), neither bounded. A guest with a blocked main thread — infinite loop, alert(), attached debugger; — never settles them, so the permit is never released and both stopRecording and closeTab block forever. Worse, closeTab claims closingTabIdsRef before acquiring the lock, so every subsequent close returns early as a silent no-op and the tab plus its webContents leak for the life of the app. Before this change startRecording took no lock at all, so this is a new failure mode. Both calls are now bounded; a timed-out measure falls through to the existing PreviewRecordingSourceSizeUnavailableError.

2. Software AV1 at 60 fps. preferredMimeTypes ranked video/webm;codecs=av1 first. Chromium reports it supported but encodes it in software, so at the new 60 fps option it drops frames and burns CPU on a machine already running agents — the opposite of what a recording is for. Hardware-encodable H.264 now wins, AV1 stays as a fallback. The explicit videoBitsPerSecond: 4_000_000 the old canvas path carried was also dropped, leaving Chromium's ~2.5 Mbps default at up to 5× the frame rate; restored and scaled with frame rate.

3. Stream leak on a cancellation path. The throwIfStartupCancelled() between stream acquisition and recorder construction threw without stopMediaStream, unlike every other failure branch. Narrow reachability, but it left a live tab capture with no owner. Now routed through cleanupFailedRecordingStart.

Re-verified: Manager.test.ts 71 passed, browserRecording.test.ts 21 passed (encoder tests updated for the new ordering), @t3tools/web and @t3tools/desktop typecheck clean.

Two things noted and deliberately not changed, for your call:

  • The old recording-frame IPC pipeline is now unreachable (subscribeRecordingFrames, PREVIEW_RECORDING_FRAME_CHANNEL, preload's recording.onFrame, the Page.screencastFrame CDP handler). Zero runtime cost; worth deleting in a follow-up rather than widening this PR.
  • Capture resolution is pinned to CSS pixels, ignoring devicePixelRatio, so HiDPI recordings are downsampled. Not a regression — the old canvas was also DIP-sized — but multiplying by DPR would be a cheap quality win given the PR's title.

The hands-on capture caveat still stands: getMediaSourceId with chromeMediaSource: "tab" against an offscreen guest is the one thing static review can't settle.

@rynfar
rynfar merged commit b5c0ef7 into pylon Aug 31, 2026
19 checks passed
@rynfar
rynfar deleted the upstream/2026-08-31-preview-recording branch August 31, 2026 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XL vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants