feat(cli): add --resolution flag to hyperframes render for one-line 4k#663
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: approve. This is the right shape: keep the composition pristine, lift output dims via Chrome DPR, validate the constraints up-front. The aspect / integer-scale / no-downsample / no-HDR guards are all the right rejections to fail loudly on. Tests cover each branch. Docs guide is genuinely useful.
Important
-
important —
packages/producer/src/services/renderOrchestrator.ts:619-624— The aspect-ratio check useswidthRatio !== heightRatiowith float division. For all the canonical preset pairs inCANVAS_DIMENSIONSthe ratios are exact integers (so===holds), but if anyone adds a non-power-of-2 preset later (e.g. cinema 4K 4096×2160 with a 1.8963 ratio), the check becomes float-fragile. Usetarget.width * input.compositionHeight === target.height * input.compositionWidth(cross-multiplication is integer-safe). Also: when the check fails, the error message lists "aspect ratio" but the actual cause might be "non-integer scale" if both dims are off. Today's code lumps both into the aspect message via short-circuit ordering — a 1500×844 → 4K case throws "aspect ratio mismatch" rather than the more helpful "non-integer scale." Test at line 822 acknowledges this with/aspect ratio|non-integer/. Worth ordering the checks integer-scale-first, OR being explicit in the message that "aspect-or-non-integer-scale rejection." -
important —
packages/cli/src/commands/render.ts:240-248— The flag-validation path is before--compositionresolution and--workersvalidation. Good. But: there's no validation that the user hasn't combined--resolutionwith--hdrat the CLI level. Today the rejection happens insideresolveDeviceScaleFactordeep in the orchestrator, which means by the time the user sees the error, ffmpeg may have been spun up, work directories may have been created, and the error path is a thrown exception bubbling up rather than the friendlyerrorBoxyou use for other invalid combinations. Add the HDR-incompatibility check next to the resolution validation. -
important —
packages/cli/src/commands/render.ts:494-532—outputResolutionis threaded intoRenderOptionsand forwarded torenderLocalandrenderDockerseparately. Two places to update if anything changes. The dockerRunArgs test catches the docker case — good. The localcreateRenderJobpath is not covered by an end-to-end test that proves the orchestrator actually receives it. The 7 unit tests coverresolveDeviceScaleFactorin isolation, but not "CLI flag → orchestrator config." Add at minimum a test that assertscreateRenderJobreceivesoutputResolution: "landscape-4k"when the flag is--resolution 4k. -
important —
docs/guides/4k-rendering.mdx:104-108— Doc claim: "ffmpeg auto-detects the dimensions from the screenshot stream and encodes at 4K." This is correct forimage2pipewith PNG/JPEG, but worth verifying the encoder isn't being passed an explicit-s 1920x1080somewhere in the ffmpeg invocation. If it is, the encoded video downscales silently and the user gets a 1080p file labeled "4k.mp4". I'd want to see one end-to-end test that actually runsffprobeon the output (the doc shows this as a verification step — it should be part of CI). Without that, "renders 4K" is a doc claim, not a tested guarantee. -
important —
packages/cli/src/commands/render.ts:343-345— The render plan banner prints "Output resolution: landscape-4k (supersampled via DPR)" but only ifoutputResolutionis set. For users who pass--resolution 4kagainst a native 4K composition (where DPR ends up as 1, no supersampling), the banner is misleading. Either suppress the "(supersampled via DPR)" suffix when the composition already matches, or print "(no-op — composition is already 4K)". Minor UX paper cut, but the wrong information is worse than no information.
Nits
-
nit —
packages/producer/src/services/renderOrchestrator.ts:603-619—resolveDeviceScaleFactorJSDoc still says "Throws on: HDR + outputResolution combination" — accurate. The comment about non-integer "(e.g. 720p composition, 4K output → 3× height but the width ratio is also 3× ✓; 1080p portrait → 4K landscape would mismatch)" is confusing because 720p → 4K is an integer scale (3×) and the example accidentally argues the opposite. #665 fixes this. Fine to leave as-is in this PR since #665 is queued. -
nit —
packages/cli/src/commands/render.ts:60-77—RENDER_RESOLUTION_ALIASESlacks"portrait-4k": "portrait-4k"(which init.ts has, also needlessly). The asymmetry between init.ts and render.ts alias maps is exactly the smell #665 fixes. Same nit — drop in a future PR (or wait for #665 to clean it). -
nit —
docs/guides/4k-rendering.mdx:130-138— "Performance" section gives multiplicative estimates (3-4×, 2-3×, 3-5×). These are believable but uncited; if there's a benchmark we can link to, that's better than orders-of-magnitude estimates that may drift. -
nit —
packages/producer/src/services/renderOrchestrator.test.ts:822-832— The non-integer test comment notes the regex/aspect ratio|non-integer/is permissive because the orchestrator throws aspect first. Once you re-order checks (see "important" above), tighten to/non-integer/. -
nit —
docs/guides/4k-rendering.mdx:103— Three tildes/apostrophes in "Steps" component — verify that mintlify renders this correctly. (Not a code review concern; just flagging for the docs reviewer.)
Cross-PR
- Consumes
CanvasResolutionfrom #660 — clean. - Consumes the
frameDataUriCacheBytesLimitMbfrom #662 implicitly: 4K renders at scale will exercise that budget. The combo is the actual prod risk. End-to-end test of "render at 4K with default config, prove cache eviction stats are sane" would close the loop on the whole stack. - The
buildDockerRunArgstest pattern (assert flag forwarded + assert flag absent) is the right tripwire for a feature that fans out to a docker arg — well done.
Praise
resolveDeviceScaleFactor as a pure, testable, exported helper is exactly the right shape — the orchestrator has too many call sites for inline validation to be safe. The four guards (HDR, aspect, downsample, non-integer) are the correct set of "fail loudly" rejections. Centralizing the supersample log + perf summary at one site is clean. Good docs guide — concrete examples, ffprobe verification, and the "two paths" framing (author at 4K vs supersample) makes the UX legible.
— Vai
miguel-heygen
left a comment
There was a problem hiding this comment.
Requesting changes for the HDR boundary around --resolution.
The new guard only passes hdrRequested: job.config.hdrMode === "force-hdr" into resolveDeviceScaleFactor(). That blocks --hdr --resolution 4k, but it does not block the default auto-HDR path. Auto-detected HDR is decided later from extracted video/image color spaces, so an HDR-source composition rendered with --resolution 4k can still enter the HDR layered compositor.
That path is not just a docs mismatch. The DOM capture path receives the scaled deviceScaleFactor, but the HDR video encoder/compositor setup still uses the authored width / height. So the render can either hit mismatched layer dimensions or produce a non-4K HDR output while the earlier log/perf metadata says the output is scaled.
Please reject outputResolution after effectiveHdr is known, or fully scale the HDR layered compositor path. Either way, add a regression test for auto-detected HDR plus outputResolution; forced HDR coverage alone does not catch this.
I rechecked the live head before posting: 00da353e21ac88862820ee60ad9f8e7fe81e59fe.
00da353 to
41dc94a
Compare

What
Adds a
--resolutionflag tohyperframes render. Pass--resolution 4kto render any composition (1080p or otherwise) at 4K — ChromedeviceScaleFactordoes the supersampling, the composition HTML is never touched.This is the missing piece for the simple 4K UX. PRs #660–#662 added the type system, scaffold flag, and memory safety; this one delivers the user-facing one-liner for existing projects.
Why
After PRs #660–#662 a brand-new project can be 4K via
init --resolution 4k, and the engine no longer OOMs at 4K. But for someone with an existing 1080p project, the only path to 4K was hand-editing five locations in the composition (data-width,data-height,data-resolution,#stageCSS,<meta viewport>) and re-rendering. That's not "simple UX."This PR closes the gap. Now every existing user gets:
How
Orchestrator (
renderOrchestrator.ts):RenderConfig.outputResolution?: CanvasResolution— acceptslandscape | portrait | landscape-4k | portrait-4k.resolveDeviceScaleFactor()helper takes composition dims + outputResolution + HDR flag and returns the integer DPR. Centralizes the validation:deviceScaleFactoris wired into bothCaptureOptionsconstruction sites (probe + render).RenderPerfSummary.resolutionreports the output dims, not composition dims, so telemetry stays correct.image2pipeto ffmpeg, which auto-detects dimensions from the image bytes, so the encoder needs zero changes.CLI (
render.ts):--resolutionflag with the same alias map asinit(1080p,4k,uhd, etc.).Output resolution: landscape-4k (supersampled via DPR)so users see what's about to happen.RenderOptions→createRenderJob(local) andbuildDockerRunArgs(docker).Docker arg builder (
dockerRunArgs.ts):outputResolution?: stringfield, forwarded as--resolution <preset>to the in-container CLI. Two new tests indockerRunArgs.test.tscover present + absent cases — the test file's existing tripwire pattern guards against silent flag drops.Docs (
docs/guides/4k-rendering.mdx):init, supersample at render). Quickstart, presets table, howdeviceScaleFactorworks, all four constraints with example error messages, performance notes (capture/encode/memory), and a pointer to upcoming Studio support.docs/packages/cli.mdx's render flag table and registered indocs/docs.jsonnext to the HDR guide.Test plan
resolveDeviceScaleFactor(default = 1, 1080p → 4K = 2, portrait → portrait-4k, native 4K = 1, HDR rejection, aspect mismatch, downsample, non-integer).dockerRunArgstests (forwards--resolutionwhen set, omits when not). Uses the existing tripwire pattern.bun run --cwd packages/cli test— 283/283 passbunx vitest run packages/producer/src/services/renderOrchestrator.test.ts— 50/51 pass; the 1 failing test (rejects a maliciously crafted key…) also fails on cleanmainand is unrelated to this change.docs/guides/4k-rendering.mdx, render flag table updated indocs/packages/cli.mdx, navigation registered indocs/docs.json.