Skip to content

fix(engine): supersample screenshot capture to honor deviceScaleFactor#666

Merged
jrusso1020 merged 2 commits intomainfrom
05-07-fix_engine_supersample_screenshot_capture_to_honor_devicescalefactor
May 7, 2026
Merged

fix(engine): supersample screenshot capture to honor deviceScaleFactor#666
jrusso1020 merged 2 commits intomainfrom
05-07-fix_engine_supersample_screenshot_capture_to_honor_devicescalefactor

Conversation

@jrusso1020
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 commented May 7, 2026

What

Fixes the actual end-to-end 4K render path. The previous PRs in this stack (#660#664) wired --resolution 4k from CLI flag → RenderConfig.outputResolutionresolveDeviceScaleFactorCaptureOptions.deviceScaleFactorpage.setViewport({ deviceScaleFactor }). The page correctly received window.devicePixelRatio === 2, but the captured screenshot still came out at 1920×1080, so the encoded MP4 was 1080p despite all the orchestrator logging saying "Supersampling via deviceScaleFactor".

This PR makes the screenshot path actually emit device-pixel-sized frames, which produces a real 3840×2160 MP4 at the encoder.

Stacked on #665.

Why

Verified end-to-end with a fresh hyperframes init project + hyperframes render --resolution 4k:

Before this PR:

$ ffprobe out-4k.mp4
width=1920
height=1080

The orchestrator logged the supersample, the page reported devicePixelRatio=2, but every captured JPEG was 1920×1080.

After this PR:

$ ffprobe out-4k.mp4
width=3840
height=2160

Captured frames are 3840×2160 native, text renders crisply at 4K (vector, not upscaled).

Two underlying issues:

  1. Page.captureScreenshot ignores viewport DPR by default. Even with Emulation.setDeviceMetricsOverride setting deviceScaleFactor: 2, calling Page.captureScreenshot without an explicit clip.scale returns at CSS dimensions (1920×1080), not device dimensions (3840×2160). The viewport DPR affects the layout / window.devicePixelRatio but not the screenshot bitmap.

  2. HeadlessExperimental.beginFrame screenshot ignores DPR entirely. BeginFrame's ScreenshotParams type has no clip / scale / viewport field — it captures the compositor surface sized by the OS window in CSS pixels regardless of deviceScaleFactor. There's no way to ask BeginFrame for a higher-resolution screenshot.

How

Two surgical fixes in packages/engine/src/services/:

  1. screenshotService.ts:pageScreenshotCapture — when deviceScaleFactor > 1, pass an explicit clip parameter { x: 0, y: 0, width, height, scale: dpr } to Page.captureScreenshot. Chrome then honors the scale and emits a width × height × dpr bitmap. No effect when dpr === 1 (default render path is byte-identical).

  2. frameCapture.ts:createCaptureSession — when deviceScaleFactor > 1, force the screenshot capture mode (skip BeginFrame) since BeginFrame can't supersample. The supersample render is already significantly slower than 1080p (4× pixels), and BeginFrame's perf advantage doesn't matter on a path the user explicitly opted into for higher quality.

Test plan

  • End-to-end manual:
    • Scaffolded hyperframes init 4k-test --example blank, edited the composition to display "4K TEST" centered text
    • hyperframes render --resolution 4k --output out-4k.mp4ffprobe reports width=3840 height=2160, JPEG sample frame is 3840×2160, text renders crisply at native 4K (vector, not upscaled)
    • hyperframes render --output out-1080p.mp4 (no --resolution) — still produces 1920×1080 (no regression on the default path)
  • Existing engine tests pass (541/541)
  • Format / lint / typecheck clean

Limitations / future work

  • BeginFrame is bypassed for any supersample render. This loses BeginFrame's per-frame perf advantage (~2–3× faster capture) at higher resolutions. A follow-up could explore using Emulation.setDeviceMetricsOverride with width: outputWidth, height: outputHeight, deviceScaleFactor: 1 (so the layout viewport matches device pixels) — but that breaks composition CSS layouts that assume 1920×1080. The current trade-off favors correctness.
  • Not unit-tested in this PR — the fix lives in code paths that require a real Chrome instance to validate (puppeteer + headless-shell). The verification is the end-to-end manual run above. A follow-up could add a Docker-built golden test at 4K (per CLAUDE.md's baseline-in-Docker rule).

vanceingalls
vanceingalls previously approved these changes May 7, 2026
Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

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

Verdict: Approve. Two surgical fixes that close out the actual end-to-end 4K render path. Last piece of the #660#665 stack: makes --resolution 4k produce real 3840×2160 pixels at the encoder rather than 1080p frames stretched to a 4K MP4 container. Diff is 15/-1 across two files, narrowly scoped, and the body documents both root causes (CDP Page.captureScreenshot ignoring viewport DPR without clip.scale; HeadlessExperimental.beginFrame having no DPR knob at all). Verified the underlying CDP behavior matches the description; both fixes are byte-identical on the dpr === 1 default path.

Note: stack still has open feedback I left on #662 (cache-self-eviction blocker) and #664 (Vite adapter dropping outputResolution) — neither is in scope for this PR, but worth landing them before declaring the 4K stack done.


Blockers

None.

Important

  • importantpackages/engine/src/services/screenshotService.ts:135-137 — alpha capture at >1 DPR is unverified. captureAlphaPng and captureScreenshotWithAlpha always pass scale: 1 and ignore deviceScaleFactor, so an alpha-format render (webm/mov/png-sequence) combined with --resolution 4k will silently produce 1080p alpha frames — the same class of bug this PR is fixing for the opaque path. resolveDeviceScaleFactor rejects HDR + supersampling but does not reject alpha + supersampling (packages/producer/src/services/renderOrchestrator.ts:592), so this combination is reachable today. Not a regression introduced by this PR (the path was already broken), but a near-miss given the fix is right next door. Either (a) extend the same clip.scale = dpr treatment to the alpha helpers, or (b) reject the combination in resolveDeviceScaleFactor until it's wired up. Pick one and flag explicitly so users don't get a silent 1080p alpha render.

  • important — testing. The PR body acknowledges no unit tests; the verification is one manual ffprobe run. Given how subtle the two-bug interaction was (orchestrator log says "supersampling", page reports devicePixelRatio === 2, and the bitmap still came out at CSS dimensions), this is exactly the path that needs a regression test — silent 1080p output with all the right log lines is the precise failure mode that will reappear. The Docker baseline-at-4K follow-up mentioned in "Limitations" is the right shape; please file a follow-up issue and link it here so it doesn't get lost. A cheap intermediate: a unit test that asserts pageScreenshotCapture sends clip: { …, scale: dpr } to client.send when deviceScaleFactor > 1 and omits clip when it's 1. That's a 20-line test using a fake CDPSession and would have caught the original silent regression.

Nits

  • nitpackages/engine/src/services/screenshotService.ts:138-145 — the clip is built conditionally then spread conditionally:

    const clip = dpr > 1 ? { x: 0, y: 0, width: ..., height: ..., scale: dpr } : undefined;
    // ...
    ...(clip ? { clip } : {}),

    Simpler:

    ...(dpr > 1 && { clip: { x: 0, y: 0, width: options.width, height: options.height, scale: dpr } }),

    or assign clip unconditionally and pass it to CDP unconditionally — Page.captureScreenshot accepts clip with scale: 1 and that produces an identical bitmap to omitting clip (both return CSS-sized capture). The double conditional reads as defensive against a behavior that doesn't actually exist. Style only.

  • nitpackages/engine/src/services/frameCapture.ts:120 — naming. supersampling reads as a verb-ish flag describing the render mode; the actual condition is "DPR > 1". A comment is fine, but dpr > 1 inline at the use site would also be readable and skip the named binding. Style only.

  • nit — comment in frameCapture.ts:115-119 is correct but slightly buries the lede. The reason BeginFrame is bypassed is "BeginFrame's ScreenshotParams has no clip/scale field" — that's the load-bearing fact. Worth promoting it to the first sentence.

Praise

  • The PR body is excellent — both root causes explained, the fact that the orchestrator/log layer was lying ("supersampling…" when no supersampling was actually happening) called out, before/after ffprobe evidence, and the explicit perf trade-off (BeginFrame bypassed at high DPR) named with the rationale. This is what staff-level fix writeups look like.

  • Surgical scope. 15 added lines, 1 removed, two files, no drive-by refactors. Trivial to revert.

  • Good defensive choice on the clip path: dpr === 1 produces a byte-identical screenshot call, so the default 1080p path can't regress from this change. That's the right shape for a fix that lands behind a flag the user opts into.

  • Honest about limitations — the BeginFrame perf regression at 4K is named, the Docker golden test is named as a follow-up, and the trade-off (correctness > perf) is justified rather than hand-waved.

— Vai

miguel-heygen
miguel-heygen previously approved these changes May 7, 2026
Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen left a comment

Choose a reason for hiding this comment

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

Approving this fix. This PR addresses the core end-to-end bug in the stack: DPR was wired through, but screenshots still came out at CSS-pixel dimensions.

What I verified on the live head badfce67c579e0bd47c22042bcf5f153cd5986d4:

  • inspected the diff in frameCapture.ts and screenshotService.ts
  • bun install --frozen-lockfile
  • bun run build:hyperframes-runtime
  • bun run --filter @hyperframes/engine test -- src/services/frameCapture.test.ts
  • bun run --filter @hyperframes/engine typecheck
  • bunx oxlint packages/engine/src/services/frameCapture.ts packages/engine/src/services/screenshotService.ts
  • bunx oxfmt --check packages/engine/src/services/frameCapture.ts packages/engine/src/services/screenshotService.ts

I also ran a small real render/ffprobe proof on this PR head:

  • hyperframes render --resolution 4k --quality draft on a 1s blank scaffold produced 3840x2160
  • the same scaffold without --resolution produced 1920x1080

That proves the screenshot path is now emitting device-pixel-sized frames for the 4K preset while preserving the default 1080p path.

Caveats: #666 CI still had regression shards pending when I rechecked, and the broader stack still has unresolved lower-layer issues I requested changes on (#661, #663, #664). This approval is for the engine supersampling screenshot fix itself.

@jrusso1020 jrusso1020 force-pushed the 05-07-refactor_dedupe_resolution_presets_and_clean_up_4k_stack branch from 1753b49 to 994d013 Compare May 7, 2026 05:05
@jrusso1020 jrusso1020 force-pushed the 05-07-fix_engine_supersample_screenshot_capture_to_honor_devicescalefactor branch from badfce6 to 1a59dea Compare May 7, 2026 05:05
@jrusso1020 jrusso1020 force-pushed the 05-07-refactor_dedupe_resolution_presets_and_clean_up_4k_stack branch from 994d013 to b39bb8d Compare May 7, 2026 05:48
@jrusso1020 jrusso1020 force-pushed the 05-07-fix_engine_supersample_screenshot_capture_to_honor_devicescalefactor branch from 1a59dea to 26f1a4c Compare May 7, 2026 05:48
Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

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

Re-review — already APPROVED, posting status on follow-ups

Reference: prior review #666 (review) (APPROVED)

This PR was already approved. The two follow-ups I'd called out are now both addressed in commit 26f1a4c7:

Addressed since prior approval (commit 26f1a4c7)

  • Follow-up — alpha capture at DPR>1 silently produced composition-resolution frames: packages/producer/src/services/renderOrchestrator.tsresolveDeviceScaleFactor now takes alphaRequested and throws when both alpha + outputResolution are set. The error message is explicit about why: "The alpha screenshot path does not yet apply deviceScaleFactor and would silently produce composition-resolution frames." Hard-rejection is the right call — silent dimension fallback would be the worst failure mode here.
    • Test: renderOrchestrator.test.ts:799rejects alpha + outputResolution (the alpha capture path doesn't apply DPR yet).
  • Follow-up — unit test for the silent-failure mode this PR fixes: packages/engine/src/services/screenshotService.test.ts (new file) — 4 cases covering the clip+scale plumbing:
    • omits clip when deviceScaleFactor is undefined or 1
    • passes clip: {x:0, y:0, width, height, scale: 2} at DPR=2
    • propagates non-2 supersample factor (e.g. 720p → 4K = 3×)
    • These directly pin the supersample contract that PR #666's original fix established. Future changes to pageScreenshotCapture can't silently drop the clip.
  • Bonus — frameCapture forces screenshot mode at DPR>1: packages/engine/src/services/frameCapture.ts:117supersampling = (options.deviceScaleFactor ?? 1) > 1; preMode = … && !supersampling ? "beginframe" : "screenshot". Comment explains BeginFrame ignores deviceScaleFactor so the screenshot path is required for any supersampled render. This is the systemic fix for the silent-fallback class of bug, not just a test for the original symptom.

— Vai

@jrusso1020 jrusso1020 force-pushed the 05-07-fix_engine_supersample_screenshot_capture_to_honor_devicescalefactor branch from 7617f4c to 453bd6e Compare May 7, 2026 16:58
@jrusso1020 jrusso1020 force-pushed the 05-07-refactor_dedupe_resolution_presets_and_clean_up_4k_stack branch from 2a097c4 to 0eaa54e Compare May 7, 2026 16:58
Base automatically changed from 05-07-refactor_dedupe_resolution_presets_and_clean_up_4k_stack to main May 7, 2026 18:47
@jrusso1020 jrusso1020 dismissed stale reviews from miguel-heygen and vanceingalls May 7, 2026 18:47

The base branch was changed.

@jrusso1020 jrusso1020 merged commit 7bf30d5 into main May 7, 2026
48 of 52 checks passed
@jrusso1020 jrusso1020 deleted the 05-07-fix_engine_supersample_screenshot_capture_to_honor_devicescalefactor branch May 7, 2026 19:08
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.

3 participants