fix(engine): restore fast screenshot path for viewport captures

[miguel-heygen]

Problem

Issue #1653 is still reproducible after the metric attribution fix: the reporter's new 0.7.3 summary shows setup is tiny (captureSetupMs: 64) while actual frame capture is slow (captureFrameMs: 15641, captureAvgMs: 174) on a pure DOM/GSAP waveform composition with no video, audio, or images.

Local reproduction matched that shape. On the reporter fixture, 0.6.42 captured 90 frames in ~11.0s locally while 0.7.3 captured the same frames in ~17.3s with workers: 4. A temporary timing probe showed seek stayed ~1-2ms/frame; the extra time was in Page.captureScreenshot.

What this fixes

This restores Chrome's faster viewport-bound screenshot path for ordinary viewport-sized captures by defaulting pageScreenshotCapture back to captureBeyondViewport: false.

The slower captureBeyondViewport: true path is now explicit on CaptureOptions and producer opts into it when a composition contains <video> elements. Distributed chunks use the same rule from the serialized video plan. That keeps the previous portrait video edge-clipping protection scoped to the class of renders it was added for instead of applying it to pure DOM/GSAP renders.

The chosen path is now also surfaced in render observability: capture_strategy checkpoints and render-level capture summaries include captureBeyondViewport, so future reports can tell whether a render used the fast viewport-bound path or the slower beyond-viewport compositor path.

Root cause

PR #1094 changed all CDP screenshot call sites to captureBeyondViewport: true to avoid bottom/right clipping on tall portrait video renders under multi-tab load. That fixed the edge case, but it also moved every normal screenshot capture onto Chrome's slower beyond-viewport compositor path even when the explicit clip rect exactly matched the page viewport.

For pure DOM/GSAP compositions like the #1653 waveform fixture, there is no video compositor surface edge case to protect. The global flag was the regression: it made the screenshot phase substantially more expensive while leaving the seek/runtime phase essentially unchanged.

This specifically targets the screenshot-capture cohort: macOS/Windows screenshot mode, and Linux renders that are forced onto screenshot capture. BeginFrame renders are unchanged because HeadlessExperimental.beginFrame does not use this CDP flag.

Verification

Local checks

• bun run --filter @hyperframes/engine test -- src/services/screenshotService.test.ts
• bun test packages/producer/src/services/render/observability.test.ts
• bun run build:producer
• Reporter fixture render, workers: 1: captureFrameMs: 13184, captureAvgMs: 146, totalFrames: 90
• Reporter fixture render, workers: 4: captureFrameMs: 14495, captureAvgMs: 161, totalFrames: 90
• bun run --filter @hyperframes/engine typecheck
• bun run --filter @hyperframes/producer typecheck
• bunx oxlint packages/engine/src/types.ts packages/engine/src/services/screenshotService.ts packages/engine/src/services/screenshotService.test.ts packages/producer/src/services/renderOrchestrator.ts packages/producer/src/services/distributed/renderChunk.ts
• bunx oxfmt --check packages/engine/src/types.ts packages/engine/src/services/screenshotService.ts packages/engine/src/services/screenshotService.test.ts packages/producer/src/services/renderOrchestrator.ts packages/producer/src/services/distributed/renderChunk.ts
• Follow-up review changes:
  • bunx oxlint packages/engine/src/services/screenshotService.ts packages/producer/src/services/render/observability.ts packages/producer/src/services/renderOrchestrator.ts
  • bunx oxfmt --check packages/engine/src/services/screenshotService.ts packages/producer/src/services/render/observability.ts packages/producer/src/services/renderOrchestrator.ts
• lefthook pre-commit: lint, format, fallow, typecheck, commitlint

Browser verification

• Rendered /tmp/hf-1653-audio-wave-fix-w4.mp4 from the real producer path.
• Opened the rendered MP4 with agent-browser.
• Screenshot: /tmp/hf-1653-browser-proof/rendered-waveform.png
• Recording: /tmp/hf-1653-browser-proof/rendered-waveform.webm

Review follow-up

• Re-ran portrait-edge-bleed on clean origin/main (20d7200b8) in /Users/miguel07code/dev/hyperframes-oss/.worktrees/repro-1653-main-portrait.
• Main produced the same local macOS/Chrome 151 failure as this PR: PSNR 26.736581 for all checkpoints, below the fixture's minPsnr: 30.
• That disambiguates the local portrait fixture result as pre-existing macOS/Chrome-151 golden drift, not a regression introduced by this PR. CI's Linux regression shards remain the canonical guard and passed.

Notes

• bun test packages/engine/src/services/screenshotService.test.ts runs the file under Bun's test runner and fails two pre-existing vi spy assertions. The package's Vitest command passes the same file.
• captureScreenshotWithAlpha and captureAlphaPng intentionally keep captureBeyondViewport: true for HDR alpha captures to preserve the fix(engine): use captureBeyondViewport on all CDP screenshot paths #1094 tall-portrait edge-clipping guard. Extending the opt-in plumbing to HDR alpha captures can be a follow-up, but this PR keeps that smaller correctness-sensitive cohort unchanged.
• This does not implement the reporter's original init-overhead hypotheses (GSAP proxy drain, __renderReady polling, static-dedup verification, image decode readiness). The new split timing shows their current reproduction is dominated by frame screenshot time, not setup time.

Addresses #1653.

[james-russo-rames-d-jusso]

R1 review @ HEAD 8be3fda1 — blind, no prior reviews present.

Issue #1653 closure check

The fix addresses the dominant per-frame symptom the reporter observed (slow Page.captureScreenshot on workers >1 / forceScreenshot paths), but not the reporter's filed theory of init-overhead items 1–4 (GSAP proxy drain via rAF, __renderReady polling delay, armStaticDedup verification, pollImagesReady+decodeAllImages). The PR body's "captureSetupMs: 64 / captureFrameMs: 15641" reproduction shape correctly inverts the diagnosis (frame capture, not init), but the issue body's four items remain unaddressed. Suggest clarifying that in the close-out comment so the issue isn't read as fully resolving the reporter's enumerated diagnoses.

The fix only applies on the pageScreenshotCapture path — i.e. macOS/Windows always, plus Linux when forceScreenshot=true is set (multi-worker distributed chunks, lowMemoryMode, alpha output, usePageSideCompositing, applyRenderModeHints auto-select). The reporter's workers: 4 distributed render IS on this path, so they get the fix. BeginFrame-mode renders (default Linux + single worker + headless-shell) are untouched — beginFrameCapture never passes captureBeyondViewport to CDP, only screenshot.format/quality/optimizeForSpeed. Worth surfacing in the PR body so future bisects don't chase the wrong path.

Findings

	file:line	finding
🟡	PR body, "Notes"	The portrait-edge-bleed regression test added in #1094 specifically to catch the bug this PR opts out of for non-video viewports is reported as PSNR 26.736581 for all checkpoints on macOS/Chrome 151 vs the fixture's minPsnr: 30 threshold (packages/producer/tests/portrait-edge-bleed/meta.json:6). CI's shard-6 run passed on Linux — so this is either (a) pre-existing macOS/Chrome-151 golden drift unrelated to this PR, or (b) the fast path on macOS allowing the same compositor edge clip #1094 was added to catch. Please confirm: does main HEAD (without this PR) also produce ~PSNR 26.7 on the same local macOS/Chrome 151? If yes → 🟢 pre-existing baseline drift, fine. If no → 🛑 the fast path is reintroducing the #1009 portrait-edge clip on macOS for the videoCount: 1 cohort and the predicate composition.videos.length > 0 isn't enough on that platform. A quick re-run on the merge-base would disambiguate.
🟡	packages/engine/src/services/screenshotService.ts:174,239	captureScreenshotWithAlpha and captureAlphaPng still hardcode captureBeyondViewport: true. These were the other two of #1094's three CDP screenshot call sites, used by the HDR layered compositor (hdrCompositor.ts:648, captureHdrFrameShared.ts:246). Each DOM layer captured in the HDR layered path still pays the slow tax even when that layer has no video-surface edge to protect. Scope-incomplete vs the PR's stated thesis ("restore Chrome's faster viewport-bound screenshot path for ordinary viewport-sized captures"). Likely an acceptable defer since HDR layered is a smaller cohort, but the comment at line 127 (Keeping the fast path for opaque jpeg captures is fine) implies a deliberate scoping choice — worth noting in the PR body so the alpha-path scope omission is intentional rather than a miss. Same opt-in plumbing (CaptureOptions.captureBeyondViewport threaded through) would extend cleanly to those two if/when wanted.
🟡	packages/producer/src/services/renderOrchestrator.ts:1215, distributed/renderChunk.ts:510	No observability signal differentiates fast vs slow path. Per the feedback_observability_pr_failure_path_coverage rubric on capture-perf PRs: if a 0.7.4 user reports "captureAvgMs is 174ms again," oncall has no way to tell from logs/captureObservability whether this PR's predicate dropped them on slow path (a video snuck in somewhere) or whether they're hitting a different bottleneck. Suggest adding captureBeyondViewport: boolean to the capture_strategy observability checkpoint at renderOrchestrator.ts:1438-1444 so the chosen path is queryable. One line, makes the rollback signal trivial.
🟡	packages/producer/src/services/distributed/renderChunk.ts:510 vs renderOrchestrator.ts:1215	The two predicates use slightly different shapes (planVideos?.videos.length ?? 0 vs composition.videos.length). Both correctly resolve to "≥1 video" but they're sourced from different artifacts (videos.json on disk vs in-process CompiledComposition). The producer extracts both from compiled.videos at compile time (compileStage.ts:156 → plan.ts:979), so they agree on intent. Not a finding — flagging for awareness that any future bug where the two diverge would silently break this PR's invariant.
🟢	screenshotService.ts:141 + types.ts:92-101	Predicate + type are sound. Default ?? false cleanly preserves the legacy fast-path contract for callers that don't opt in. JSDoc on CaptureOptions.captureBeyondViewport explains both branches. Test pinning at screenshotService.test.ts:44,59-68 covers both the new default-off and the opt-in path — canonical defensive bug-pinning shape per feedback_defensive_bug_pinning_tests_fp_suppression.
🟢	HF#1656 page-side compositing	usePageSideCompositingForTransitions forces forceScreenshot=true and routes through pageScreenshotCapture. When the source composition contains <video> elements, composition.videos.length > 0 still triggers captureBeyondViewport: true, so the page-side compositing path is unaffected for video-bearing renders. For pure-shader-transition DOM-only compositions (no <video>), it now takes fast path, which is the desired behavior. The frameCapture.ts:1293 1×1-px micro-screenshot trick that drives page-side composite resolution doesn't pass captureBeyondViewport (defaults false), which is fine — it's a throwaway.
🟢	CI	All 44 required checks pass, including all 8 regression shards. portrait-edge-bleed runs in shard-6 (Linux) and passed.

Verdict

Sound diagnosis + sound fix shape for the targeted cohort. The portrait-edge-bleed PSNR-on-macOS observation is the only item I'd want disambiguated before stamping — if it's pre-existing baseline drift, this PR is good to land. The HDR-alpha scope omission and observability gap are non-blocking but would round it out.

Stamp routing: not stamping — flagging <@U0ARJFN5S6Q> (Rames Jusso, OG) to land after the macOS PSNR question is resolved one way or the other. If James prefers to skip the macOS confirmation and ship to unblock the dominant Linux/workers>1 cohort, that's a reasonable call too — the CI regression-shard is the canonical guard and it passed.

— Rames D Jusso

[vanceingalls]

Verdict: LGTM, scoped concur with Rames's findings. HEAD 8be3fda1, base main, standalone. +35/-5 across 5 files, 1 commit.

Posting after Rames's R1 (at 8be3fda1). HEAD unchanged. My read agrees with the disposition — none of Rames's flagged items block this from landing once the macOS PSNR question is disambiguated; the rest are either deferred-scope, observability follow-ups, or pre-existing baselines. Adding my own verification + a couple of independent angles below.

Independent verification

Discriminator correctness. Fast path is the default; slow captureBeyondViewport: true is opt-in on CaptureOptions (packages/engine/src/types.ts:95-101) and consumed at the single CDP call site (packages/engine/src/services/screenshotService.ts:140). Two opt-in points cover the two render entry paths:

• In-process executeRenderJob (packages/producer/src/services/renderOrchestrator.ts:1215): composition.videos.length > 0.
• Distributed renderChunk (packages/producer/src/services/distributed/renderChunk.ts:510): (planVideos?.videos.length ?? 0) > 0.

Cross-checked both shapes are VideoElement[] and that composition.videos aggregates main + sub-composition videos (parseSubCompositions flow in htmlCompiler.ts:303,1357,1891 with id-dedupe). Sub-composition <video> elements are covered. Concur with Rames that the two predicates source from different artifacts (compiled.videos in-process vs meta/videos.json on disk) but agree on intent — both are written from the same compile-time pass.

Single consumer of the option. pageScreenshotCapture is the only CDP screenshot path that reads the new flag; the two callers in packages/engine/src/services/frameCapture.ts:1517,1698 both forward the session-level options. beginFrameCapture never reads captureBeyondViewport (the CDP HeadlessExperimental.beginFrame API doesn't accept it), so BeginFrame-mode renders are untouched — concur with Rames that this scopes the fix to the screenshot-capture cohort (macOS/Windows always; Linux when forceScreenshot=true: lowMemoryMode, multi-worker calibration timeout, alpha output, page-side compositing, hint-driven mode select).

#1094 protection preserved. Any composition containing a <video> element still routes through the beyond-viewport path. The checked-in portrait-edge-bleed regression fixture (packages/producer/tests/portrait-edge-bleed/) contains sample.mp4, so videos.length > 0 and the slow path stays armed. On CI, the regression workflow's portrait-edge shard passed at 8be3fda1. (Note: per Miguel's PR body the regression fixture lives in shard-3 of the renamed regression shards in the head SHA's CI run, not shard-6 as Rames's review states. Both observations are consistent — shard-3 (...css-spinner-render-compat web...) and shard-6 (overlay-montage-prod style-12-prod chat missing-host-comp-id png-sequ...) both completed SUCCESS at this HEAD. Doesn't change the conclusion.)

No interaction with #1656 / #1618. captureForceScreenshot (the #1656 knob, renderOrchestrator.ts:1413) selects screenshot vs BeginFrame; the new captureBeyondViewport is orthogonal — a CDP flag inside the screenshot path. The #1618 page-side compositor handshake (HF_PAGE_SIDE_COMPOSITING_STUB + evaluateOnNewDocument) is injected upstream of the CDP capture and is not gated on captureBeyondViewport. Customer cohorts from #1618, #1656, and #1662 (descendant-hide in init.ts, page-side runtime, orthogonal to capture path) all remain covered.

Concurring with Rames's flags

• 🟡 portrait-edge-bleed macOS PSNR 26.7 (Miguel's note). Worth disambiguating on main HEAD pre-merge per Rames. My independent angle: this PR sets captureBeyondViewport: true for that fixture (it has a video), so the predicate keeps the slow path armed there. If PSNR drops on macOS at HEAD, it would point to either (a) pre-existing macOS/Chrome-151 baseline drift unrelated to this PR, or (b) the slow path itself regressing on the newer Chrome — neither caused by this PR's scoping. A 30-second git stash + bun run --filter @hyperframes/producer test -- portrait-edge-bleed on main would close this.
• 🟡 HDR alpha scope (captureScreenshotWithAlpha, captureAlphaPng). Agreed — these still hardcode captureBeyondViewport: true. Defer is reasonable since HDR layered is a smaller cohort and the alpha path has additional correctness constraints (optimizeForSpeed: false already, Emulation.setDefaultBackgroundColorOverride ping). The new CaptureOptions plumbing would extend cleanly when wanted. Non-blocking; worth a PR-body line so the omission is read as intentional.
• 🟡 Observability gap. Strong agreement — adding captureBeyondViewport: boolean to capture_strategy checkpoint at renderOrchestrator.ts:1438-1444 would make the next rollback signal trivially queryable. One-line change, ideally in this PR.

One additional nit (mine)

• History anchoring. The new comment in screenshotService.ts:138-140 reads cleanly but the hardcoded captureBeyondViewport: true on captureScreenshotWithAlpha / captureAlphaPng says // see pageScreenshotCapture for rationale — and the rationale comment at pageScreenshotCapture no longer mentions #1094 (it's now the "fast by default, opt-in slow" comment). Next person doing a code-spelunk on those two true literals will hit a dead reference. Either re-anchor the alpha-path comments to // portrait video edge-clipping protection — see #1094 directly, or extend the new pageScreenshotCapture comment to retain the #1094 anchor for the back-reference. Cosmetic.

Customer-cohort regression summary

• #1094 (portrait video edge clipping): covered wherever composition has any <video>.
• #1618 (page-side compositor handshake): orthogonal, covered.
• #1656 (page-side compositing screenshot route): orthogonal, covered. For pure-shader-transition DOM-only compositions, page-side compositing now takes fast path — this is the desired behavior, no <video> surface to protect.
• #1662 (descendant-hide ancestor walk): runtime-side, orthogonal to capture path. Covered.

CI

All 44 required checks pass at HEAD 8be3fda1 (CI: Test, Typecheck, Lint, Fallow audit, regression shards 1-8, Windows render, preview-regression, CodeQL).

Review by Via