fix(cli): forward --hdr through Docker render + HDR docs#346
fix(cli): forward --hdr through Docker render + HDR docs#346vanceingalls merged 16 commits intomainfrom
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Staff engineer review
TL;DR
Fix itself is correct, minimal, and addresses a real user-facing bug (--hdr silently dropped through Docker). Test plan is solid with ffprobe-verifiable output. But the docs claim behavior that doesn't exist in the code, and the PR leaves two closely-related flags (--crf, --video-bitrate) silently dropped in the same function. Recommend a second pass before merge.
Blocking / should-fix
1. Non-existent engine exports in docs/packages/engine.mdx
The doc shows:
import { ... DEFAULT_HDR10_MASTERING } from '@hyperframes/engine';
import type { ... HdrMasteringMetadata } from '@hyperframes/engine';Neither is re-exported from packages/engine/src/index.ts — they live only in packages/engine/src/utils/hdr.ts. A user copy-pasting this snippet gets a compile error. Either add both to the barrel (consistent with HdrTransfer / HdrEncoderColorParams) or drop them from the doc.
2. Fabricated behavior in docs/guides/hdr.mdx
"If an HDR-tagged source has multiple frames (animated WebP, APNG), Hyperframes logs a warning and uses only the first frame."
No code implements this. HDR image decode is decodePngToRgb48le on a single PNG (renderOrchestrator.ts:429), and there's no corresponding log.warn. Either implement the warning path or delete the claim — asserting behavior that doesn't exist causes silent failures later.
3. Same-category latent bug left unfixed
renderDocker on main also silently drops options.crf and options.videoBitrate (confirmed by diff). The PR fixes --hdr but leaves --crf 15 --docker and --video-bitrate 10M --docker in exactly the failure mode you just wrote a changelog about. Either forward them in this PR (3 more lines, same idiom) or open a tracking issue and call it out in the PR body. Merging the one-flag fix alone will ship the same class of bug again.
Nits / follow-ups
4. Log level drift. Guide says --format mov|webm + --hdr "logs a warning". Code is log.info at renderOrchestrator.ts:982. Either bump it (arguably warn-worthy — user asked for HDR and silently didn't get it) or rewrite the doc as "logs a message".
5. Benchmark claim without a source. "roughly 5–10× the local render time" is a performance contract. If it's from the hdr-pq fixture on one machine, say so ("on the hdr-pq fixture we observed ~Nx") — otherwise it becomes the target users hold you to.
6. No regression test for the fix. The change is arg-array construction. Factoring the Docker arg build into a pure helper (buildDockerRunArgs(options)) + a snapshot test would prevent the next flag from silently dropping. Relevant even if you don't fix #3 here.
7. Minor: yt-dlp -f \"bestvideo[vcodec^=vp9.2]+bestaudio\" covers only VP9.2 HDR YouTube streams — for AV1-HDR it misses. bestvideo[dynamic_range=HDR]+bestaudio is more robust.
8. Made with [Cursor] footer — check with maintainers whether that gets stripped.
What I liked
- Surgical one-line fix; no drive-by refactors.
- The "detection, not force" framing is repeated in 3 places (guide, common-mistakes, CLI flag table) — defensive cross-linking that prevents the next support ticket.
- Producer
RenderConfig.hdrdoc matches the actual type atrenderOrchestrator.ts:190. - Test plan includes both positive (with-fix) and negative (without-fix) ffprobe output — that's how test plans should read.
- HDR10 metadata (
master-display,max-cll) called out explicitly as the thing that trips up QuickTime/YouTube — real operator knowledge, not boilerplate.
Recommendation
Address (1)–(3), then LGTM.
…level, docker arg helper)
- packages/engine/src/index.ts: export DEFAULT_HDR10_MASTERING and the
HdrMasteringMetadata type. docs/packages/engine.mdx imports both, but
neither was on the public surface, so the docs example failed at type-check.
- packages/cli/src/commands/render.ts + packages/cli/src/utils/dockerRunArgs.ts:
extract the docker run argv construction out of renderDocker into a pure
buildDockerRunArgs helper. The previous in-line array silently dropped --hdr
for months because every new render flag had to be remembered in two places;
a pure function lets the test suite snapshot the entire arg list and assert
flag forwarding directly.
- packages/cli/src/utils/dockerRunArgs.test.ts: snapshot tests for the default
argv and the all-flags-on argv, plus an explicit "must contain --hdr"
regression assertion so the failure message points straight at the bug class
the original PR was fixing.
- packages/producer/src/services/renderOrchestrator.ts: bump the
"HDR source detected but format is X" log from info to warn and rephrase it
to point at the fix ("Use --format mp4 for HDR10 output."). The HDR guide
documents this as a warning; the code was emitting info.
- docs/guides/hdr.mdx:
- Replace the fabricated "animated HDR images log a warning and use the
first frame" claim with a Note that HDR <img> support is limited to
16-bit PNG, with everything else falling back to SDR. The animated-image
branch never existed in code.
- Switch the yt-dlp example from -f bestvideo[vcodec=vp9.2] to
bestvideo[dynamic_range=HDR]+bestaudio so AV1-HDR streams from Shorts
and newer YouTube uploads are also picked up.
- Qualify the Docker render benchmark — the original "5–10× slower" number
was a single data point. The text now tells readers to measure on their
own composition.
Made-with: Cursor
The --hdr flag was silently dropped when combined with --docker because renderDocker did not include it in the args passed to the in-container hyperframes render invocation. Forwarding it produces the same HDR10 MP4 (HEVC, yuv420p10le, BT.2020 PQ, mastering display + max-cll) as a local render — verified end-to-end with the new hdr-pq regression fixture. This PR also fills in the documentation gaps from the HDR feature stack (#288, #289, #290, #265, #268), which each added a slice of HDR support without pulling it together into a single guide. Code change: - packages/cli/src/commands/render.ts: refactor renderDocker to use a new pure helper buildDockerRunArgs (packages/cli/src/utils/dockerRunArgs.ts) that explicitly threads every render flag — including --hdr, --gpu, --quiet — into the docker run argv. Snapshot-tested in dockerRunArgs.test.ts so any future flag drop fails loudly. Docs: - New docs/guides/hdr.mdx — top-level HDR guide: quickstart, how detection works, source media requirements (HDR video + 16-bit HDR PNG), output format constraints, ffprobe verification, Docker rendering, limitations, common pitfalls - docs/guides/common-mistakes.mdx — "Expected HDR but got SDR" entry; --hdr is detection, not force; notes Docker now supports it - docs/guides/rendering.mdx — --hdr row in flags table; HDR card under Next Steps - docs/packages/cli.mdx — expanded --hdr description with link to the guide - docs/packages/producer.mdx — HDR Output section under RenderConfig - docs/packages/engine.mdx — HDR APIs section (color-space utilities + WebGPU readback runtime); now compiles because DEFAULT_HDR10_MASTERING and HdrMasteringMetadata are re-exported from packages/engine/src/index.ts - docs/docs.json — register the new HDR guide in nav Review feedback from PR #346: - packages/engine/src/index.ts — export DEFAULT_HDR10_MASTERING and HdrMasteringMetadata so the engine docs example compiles - docs/guides/hdr.mdx — drop fabricated "animated HDR images log a warning and use first frame" claim (not implemented); clarify that HDR <img> is 16-bit PNG only and other formats fall back to SDR; broaden the yt-dlp example to bestvideo[dynamic_range=HDR]+bestaudio so AV1-HDR streams are also picked up; qualify the Docker render benchmark claim - packages/producer/src/services/renderOrchestrator.ts — bump the HDR + non-mp4 fallback from log.info to log.warn and make the message actionable Regression test (hdr-pq): - packages/producer/tests/hdr-regression — delete the broken sub-tree. The old hdr-pq composition relied on a CDN shader-transitions library and window.__hf injection with scenes defaulting to opacity:0, so when the library failed to load most of the render was invisible. The fixture was also nested two directories deep, so discoverTestSuites (which only scans direct children of tests/) never picked it up. - packages/producer/tests/hdr-pq — new top-level fixture: 5s of an HDR10 HEVC Main10 BT.2020 PQ source clip with a static SDR DOM overlay. Source is a 5s excerpt (1:18–1:23) from https://youtu.be/56hEFqjKG0s, downloaded via yt-dlp and re-encoded to HEVC Main10 / 1920x1080 / 30fps with HDR10 mastering metadata. Self-contained: no external CDNs, no custom fonts, no animation libraries. NOTICE.md captures attribution. - packages/producer/src/regression-harness.ts — add optional hdr boolean to TestMetadata.renderConfig, validate it, and forward it to createRenderJob so HDR fixtures actually run through the HDR pipeline. - .github/workflows/regression.yml — exclude hdr from the fast shard and add a dedicated hdr shard so the new fixture runs on CI. - .gitattributes — LFS-track packages/producer/tests/*/src/*.mp4 so source clips don't bloat the repo. - packages/engine/src/services/frameCapture.ts — inject a no-op __name shim into every page via evaluateOnNewDocument. tsx transforms the engine source on the fly with esbuild's keepNames, which wraps every named function in __name(fn, "name"). When page.evaluate() serializes a callback those calls would crash with "__name is not defined" because the helper only exists in Node. The shim makes the engine work whether it's imported from compiled dist or from source via tsx.
vanceingalls
force-pushed
the
feat/hdr-docker-and-docs
branch
from
b71ab32 to
7a695dd
Compare
Mirrors parseVideoElements for <img> tags: parses src, data-start, data-end/data-duration, generates stable hf-img-N IDs, and exports ImageElement type from the engine index. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Mirrors the existing video probe loop so 16-bit PNGs tagged BT.2020 PQ/HLG flow into analyzeCompositionHdr alongside videos. Image-only compositions can now trigger HDR output without any video sources.
…tacking Generalize the HDR pipeline to treat HDR videos and HDR images uniformly so image-only HDR compositions render natively instead of falling back to the SDR DOM screenshot path. - Combine nativeHdrVideoIds + nativeHdrImageIds into nativeHdrIds and gate hasHdrContent on the union so an image-only composition still triggers the HDR encode path. - Pass the combined nativeHdrIds set to queryElementStacking (renamed param in @hyperframes/engine) so the stacking query marks HDR images alongside HDR videos when the layer compositor groups elements by z-order. - Dispatch each HDR layer in compositeToBuffer (and the transition dual- scene loop) to blitHdrImageLayer for images and blitHdrVideoLayer for videos, using nativeHdrImageIds.has(id) as the discriminator. - Hide HDR images in the DOM screenshot via the hideIds filter (now keyed on nativeHdrIds) so the SDR fallback img doesn't overpaint the rgb48le buffer composited underneath. - Update HDR diagnostic logging to record kind: image|video and emit buffer dimensions / frame paths appropriate to each source type.
Mirror the HDR-video stacking-query logic for HDR images. Build a hdrImageStartTimes map from composition.images filtered to nativeHdrImageIds, and union those start times with hdrVideoStartTimes when computing uniqueStartTimes for the seek-and-query loop. Without this, an HDR image with `data-start > 0` would never have its layout dimensions recorded in hdrExtractionDims, because the stacking query only ran at HDR-video appearance times. Image-only compositions (no HDR videos) wouldn't query stacking at all.
HDR layered compositing uses captureAlphaPng (Page.captureScreenshot with a transparent background) for the SDR DOM overlay layer. That CDP call hangs indefinitely when Chrome runs with --enable-begin-frame-control (default on Linux/headless-shell) because the compositor is paused. The hdr-pq regression test was timing out in CI for this reason. Set cfg.forceScreenshot = true on the HDR path, mirroring the existing behavior for WebM/MOV alpha output. Made-with: Cursor
Covers the HDR still-image pipeline end-to-end with no HDR video
sources present:
- 16-bit BT.2020 PQ PNG (cICP-tagged) is detected via ffprobe,
- decoded once into rgb48le and blitted under the SDR DOM overlay,
- encoded as HEVC Main10 with HDR10 mastering display + content
light level metadata.
Includes a deterministic Python fixture generator (ffmpeg does not
embed cICP in PNGs, so we synthesize the bitmap and inject the chunk
manually) and wires the test into the existing `hdr` CI shard.
Made-with: Cursor
HDR layered compositing extracts HEVC video frames out-of-band via ffmpeg, so the in-DOM `<video>` element is only used for layout. On Linux headless-shell, Chromium ships without HEVC support and these elements never reach `readyState >= 1`, causing the per-page readiness poll to time out after 45s with "video metadata not ready". Add `skipReadinessVideoIds` to `CaptureOptions` and honor it in both screenshot and beginframe readiness polls. Wire `nativeHdrVideoIds` through every HDR-path `createCaptureSession`/`executeParallelCapture` call in `renderOrchestrator`. Made-with: Cursor
|
Since that review was posted, the main code path that still needs focused review is the HDR still-image support that landed afterward. What has been fixed since the original feedback:
What needs review based on code that changed after that feedback was posted:
Verification on the current branch:
So from a fresh review pass, I would focus on the PNG HDR metadata fallback logic, the resulting still-image HDR render path, and a quick pass over the new CLI flag forwarding rather than the older docs-only points. |
…d object-target tweens
`video_nested_in_timed_element` previously kept HTML5 void elements (e.g.
<img>, <br>) on its parent stack forever — they have no closing tag — so
any later <video data-start> was reported as nested inside the most recent
<img>. Skip void elements explicitly.
`overlapping_gsap_tweens` walked GSAP `.to/.from/.fromTo/.set/.add` regex
matches and indexed into `parsed.animations` from `parseGsapScript`. The
parser silently drops calls whose first argument is not a quoted selector
(object-target tweens like `tl.to({ _: 0 }, …)` used to anchor a timeline's
duration), but the regex still matched them — drifting the index by one
and making subsequent windows pick up the wrong selector and position.
Skip matches with non-quoted first arguments so both passes agree.
Both regressions surfaced while linting an HDR showcase composition that
mixes timed <img> HDR layers with <video> HDR layers and uses a phantom
duration tween in its GSAP timeline.
Made-with: Cursor
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Staff-engineer review — second pass
Prior review items are all addressed (engine exports, dropped fabricated claims, --crf/--video-bitrate forwarded, log.warn with actionable message, snapshot-tested buildDockerRunArgs). CI green including the new hdr shard. Approving — none of what's below blocks merge, but please file follow-ups; a few are real bugs I'd want on the tracker today.
Scope smell
Started as "forward --hdr through Docker + docs." Landed as: CLI flag forwarding, 5 docs files, ImageElement threaded through engine + producer, HDR image probe + native compositing (+292 lines in renderOrchestrator.ts), PNG cICP fallback, HDR readiness-skip, force-screenshot on HDR, __name dev-mode polyfill, 2 new regression fixtures + 3 deleted, and 2 unrelated lint bugfixes. Any revert is all-or-nothing. Next time, split after the first ~5 commits.
Substantive
-
PNG
cICPparser is permissive beyond spec —packages/engine/src/utils/ffprobe.ts:107-169.- No CRC32 validation — attacker who controls
<img src>can craft a fake cICP chunk and force HDR encode of non-HDR pixels. Low blast radius but ~10 lines to fix. - Spec: cICP length is exactly 4 and cICP must precede IDAT. We accept
>= 4and don't check ordering. - Test
ffprobe.test.ts:6-18passes only because the localffprobedoesn't read PNG color tags. An explicit ffprobe-unavailable mock test would pin down which branch is actually covered.
- No CRC32 validation — attacker who controls
-
Silent failure on HDR frame extraction —
renderOrchestrator.tsaround line 1354 in the diff. FFmpeg failure doeslog.warn(\"loop will fill with black\")then continues;blitHdrVideoLayerreturns early on missing frame path. Combined with unconditionalskipReadinessVideoIds, there is no page-level signal that native HDR decode is broken — a production render can complete successfully with black frames for a whole HDR video layer. At minimum emitlog.error+ a counter onjob.perfSummaryso the caller can detect it. -
compositeToBufferis a 200-line closure over ~15 variables insideexecuteRenderJob. Not unit-testable, and the diff between old HDR-video and new HDR-video+image paths is noisy because of it. Extract to a top-levelcompositeHdrFrame(ctx, canvas, time, stacking, filter)— closure variables become an explicit context object. Biggest maintainability risk in the PR. -
Per-layer DOM re-seek + re-screenshot is an uncaptured perf regression. For N z-index layers at 30fps over 10s = 300N screenshots. Given the prior 2-3x BeginFrame speedup work, this deserves a wall-clock measurement on
hdr-pq(old vs new). Nothing in the PR quantifies it. -
__namepolyfill ships dev-only workaround to prod —frameCapture.ts:87-100. Real fix is configuring tsx/esbuild to not wrap functions that cross Node↔Browser viapage.evaluate. Precedent ("bundler emits weird thing → patch runtime") is bad; also subtly claims ownership ofwindow.__nameon every captured page. -
{ ...captureOptions, skipReadinessVideoIds: ... }is pasted in 5 places. Factor tobuildHdrCaptureOptions()or the next path will forget one. -
HDR image buffer re-converts transfer per frame —
blitHdrImageLayerdoesBuffer.from(buf.data)+convertTransfer(...)on every frame even though neither source nor target transfer changes. Cache per(imageId, targetTransfer). -
extractVideoMetadata(imagePath)is cross-package semantic drift — the producer now relies on "call video-metadata extractor on a PNG, get back HDR color space." Works today. Rename toextractMediaMetadata, or add a namedextractImageMetadatawrapper in the engine so the contract is discoverable.
Tests / CI
-
Coverage deletion without replacement. Old
hdr-regression/mixed-sdr-hdrandsdr-baselineare gone. The replacements verify the specific bug but drop SDR→HDR color conversion, HDR + audio + captions, and SDR baseline stability under HDR code changes. -
hdr-pquses a 5s YouTube excerpt. Even withNOTICE.md, shipping a third-party YouTube clip as a fixture in an OSS repo is legal grey. Thehdr-image-onlyPython generator is the right model — apply it tohdr-pqtoo (synthesize a PQ HEVC clip via ffmpeg + test pattern). Worth a legal pass before the OSS repo surfaces it publicly. -
maxFrameFailures: 0over 100 checkpoints onhdr-image-only— any libx265/Chromium/ffmpeg version bump flakes the shard.maxFrameFailures: 2preserves signal and absorbs drift. -
hdr-image-only/src/hdr-photo.pngis not LFS-tracked despite the PR addingtests/*/src/*.mp4to LFS. Bump to 1920×1080 16-bit PNG and you get ~12MB in the pack file. Addtests/*/src/*.pngto.gitattributes.
Nits
dockerRunArgs.test.tssnapshot locks ordering — any neutral reorder fails. That's the intent; add a comment pointing reviewers at the test.DEFAULT_HDR10_MASTERINGis hardcoded; doesn't respectMaxCLL/MaxFALLfrom source metadata. File a TODO.extractVideoMetadatafor PNG returnsvideoCodec: \"png\",fps: 0,durationSeconds: 0. Fine but the type name is now misleading.- HDR docs sample uses self-closing
<img />— repo lintsself_closing_media_tagfor<video>/<audio>; worth a caveat so users don't carry the pattern across.
What landed well
- Every blocking item from the prior review addressed with a focused commit.
buildDockerRunArgs+ inline snapshot + explicit--hdrcontains-check is textbook regression-prevention.- Deterministic Python PNG generator is the right pattern for reproducible HDR fixtures.
- Two unrelated lint bugfixes each land with a focused test — good discipline.
- Warn level + actionable message on
--hdr+ non-mp4 fallback is exactly what was asked for. - The layered-compositing masking comment block in the DOM-layer branch is one of the clearest pieces of docs in the PR.
Follow-ups to file before merge
- CRC-validate PNG
cICPchunk; enforce length == 4 and cICP-before-IDAT. - HDR frame extraction failure should fail loudly (error + counter), not
log.warn → black frames. - Extract
compositeHdrFrameto a top-level function. - Measure
hdr-pqwall-clock before/after for the per-layer DOM re-screenshot perf impact. - Cache per-image transfer-converted HDR buffer.
- Replace
hdr-pqYouTube fixture with a synthesized source (same pattern ashdr-image-only). - Restore mixed-SDR-HDR + HDR+captions regression coverage.
- Investigate the real fix for
__name; remove the polyfill. - Relax
maxFrameFailures: 0 → 2on HDR fixtures.
LGTM — approving. 🚢
… images Previously blitHdrImageLayer rendered the source PNG at its native resolution, ignoring the CSS object-fit / object-position applied to the <img> element. With cover or contain rules, the image appeared as a small native-sized patch floating inside its layout box. Resample the decoded rgb48le buffer to the element's layout dimensions during the one-shot decode pass, applying object-fit semantics with a bilinear resampler. The downstream affine blit then treats the source as if it were already sized to the layout box, matching browser behavior. - Extend ElementStackingInfo with objectFit / objectPosition captured from getComputedStyle for HDR replaced elements. - Add resampleRgb48leObjectFit, normalizeObjectFit and helpers in alphaBlit (fill / cover / contain / none / scale-down with object-position support, opaque black slack fill). - Cache object-fit info per HDR image id and resample once during pre-decode to keep per-frame cost a memcpy + blit. Made-with: Cursor
2 participants
Summary
This PR ended up covering the full HDR Docker/docs follow-through plus the producer/engine work needed to make HDR still images render and regress correctly in CI.
The branch now does four things:
--hdrthrough the Docker render path in the CLIffprobedoes not expose PNG color tagsWhat changed
CLI and docs
hyperframes render --docker --hdrnow preserves--hdrwhen invoking the in-container CLIEngine and producer HDR image support
ImageElementsupport to the engine composition model and parsing pathHDR metadata robustness
extractVideoMetadata()to read PNGcICPmetadata directly whenffprobeomits color-space fields for PNGshdr-image-onlyfixture, where the render was falling back to SDR because the PNG was not being recognized as BT.2020 PQRegression coverage and fixture cleanup
hdr-image-only, a regression fixture that validates HDR still-image rendering end to endhdr-pq, a focused HDR PQ regression fixture for the video pathhdrshard with--sequential hdr-pq hdr-image-onlyhdr-regression/*fixture set in favor of the smaller targeted regressions used by CIWhy
The original PR description only covered the CLI flag forwarding and docs work. Since then, the branch also picked up the missing runtime support needed for HDR still images and the regression coverage to keep that path from breaking.
The practical issue this closes is:
hdr-image-onlyffprobein the CI Docker environmentcICPchunk directly makes HDR detection deterministic across environmentsTest plan
Local targeted checks
bunx oxlint packages/engine/src/utils/ffprobe.ts packages/engine/src/utils/ffprobe.test.ts bunx oxfmt packages/engine/src/utils/ffprobe.ts packages/engine/src/utils/ffprobe.test.ts bun --cwd packages/engine test src/utils/ffprobe.test.ts src/utils/hdr.test.tsProducer regression runs on host
Observed result:
fastshard: 7 passed, 0 failedhdrshard: 2 passed, 0 failedCI-equivalent Docker verification
Observed result:
hdr-image-only: passedhdr-pq: passedSpecific regression fixed
Before the PNG
cICPfallback, the Docker/CI run failedhdr-image-onlywith:"[Render] HDR source detected — output: PQ ..."log lineAfter the fallback, the same Docker path recognizes the PNG as HDR and the shard passes.