feat(producer): add codec knob to DistributedRenderConfig#850
Conversation
5e00c8f to
3495815
Compare
a44bcfb to
271dcda
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed as part of the fixture stack. LGTM.
miguel-heygen
left a comment
There was a problem hiding this comment.
Stack Review: Distributed fixture PRs (#845, #847, #848, #850, #851, #852)
Well-structured test stack. Fixtures cover the important format matrix (H.264, H.265, ProRes, png-sequence) and per-adapter chunk-boundary determinism.
#850 (codec knob): resolveEncoderTriple replacing the format-encoder table is clean — enables validation at the call site. One note: verify "libx265-software" is in the LockedRenderConfig["encoder"] union (the type constraint is implicitly relied upon).
#852 (chunk-boundary fixtures): Strongest PR. Using png-sequence for byte-equality testing avoids mp4 keyframe placement noise. Covering all 6 first-party adapters (GSAP, Anime, Three, Lottie, CSS, WAAPI) is the right regression vector.
#851 (H.265): Cross-codec PSNR baseline (H.264 in-process vs H.265 distributed, 48dB observed) is clever but could get fragile with complex visuals. The 30dB threshold has headroom for now.
#847 (png-sequence): Minor type concern: VIDEO_EXT is Record<"mp4" | "mov" | "webm", string> but outputFormat includes "png-sequence". The runtime ternary guards it, but TypeScript won't narrow through the ternary — VIDEO_EXT[outputFormat] could type-error. Consider an explicit cast.
#848 (ProRes): meta.json includes minAudioCorrelation / maxAudioLagWindows despite no audio in the composition — harmless but confusing for future fixture authors.
All approved. No blocking issues.
vanceingalls
left a comment
There was a problem hiding this comment.
Codec knob threads cleanly through plan → encoder.json → renderChunk → buildEncoderArgs. libx265-software was already a member of LockedRenderConfig["encoder"] (freezePlan.ts:39), and the closed-GOP h265 params in chunkEncoder.ts:207-228 fire automatically because lockGopForChunkConcat: true is already set at renderChunk.ts:603. planHash incorporates the encoder JSON (freezePlan.ts:309-310), so h264 vs h265 produces distinct hashes — replay correctness preserved.
Additive to @miguel-heygen's stack approval (his union-type concern verified satisfied — no need to re-raise).
Strengths
resolveEncoderTriple(plan.ts:478-501) — pulling the table out into a function is the right move once the (format, codec) cross-product enters the picture. The "throw when codec is set on a non-mp4 format" branch is the load-bearing bit: silently ignoring the field would produce a planHash that recordscodec: "h265"but anencoder: "prores-software"worker, which is exactly the kind of mismatch that's invisible until production. Failing at plan-time is correct.renderChunk.ts:556-562— the override comment ("getEncoderPresetonly returns h265 in HDR paths today") names the precise reason the spread-override exists, so the next person who tries to "clean this up" by deleting it has the context to not break SDR h265.- Test for the rejection path (
plan.test.ts:256-272) uses@ts-expect-errorto force the runtime check — that's the right shape for a defense-in-depth guard.
Findings
important
- Unknown codec strings silently fall through to libx264 —
plan.ts:483-488. The branch isif (codec === "h265") { libx265 } return libx264, so any non-"h265" string (typo, future "av1", a JS caller passingcodec: "H265") maps to h264 without a warning. The PR rightly rejects bad (format, codec) combos at the boundary; this is the symmetric gap. Concretely: a Lambda caller building config from JSON who passescodec: "h266"(typo) gets h264 output with no signal. Suggested fix: explicitif (codec !== "h264" && codec !== "h265") throw ...so the failure mode matches the non-mp4-format case. Cheap to add now, awkward later if the codec union grows. file:packages/producer/src/services/distributed/plan.ts:483-488. - No unit coverage on the
renderChunk.tsoverride —renderChunk.ts:561-562flipspreset.codecbased on the locked encoder discriminant. Zero direct test asserts that alibx265-softwareencoder.json yields a preset withcodec: "h265"after the override. The end-to-end fixture in #851 will catch a regression, but a fast unit test (synthesize aLockedRenderConfigwithencoder: "libx265-software", assert the post-override preset) would catch it independently of the heavyweight Docker fixture. Without this, someone refactoring the override (e.g. moving it intogetEncoderPresetitself) won't see a fast-test signal. file:packages/producer/src/services/distributed/renderChunk.ts:555-562.
nit
- Field name
codecis mp4-specific by construction but reads as generic —plan.ts:84-93. ProRes422 vs ProRes4444 is a plausible future "codec for mov" choice, and a field literally namedcodecwill be the obvious place to put it; the current name pre-commits the namespace to mp4. If you expect any future codec selection for mov / png,mp4Codecwould future-proof the surface. Pure-naming nit — not worth churn if the type docstring is the contract. - Test on
plan.test.ts:257: the matcher/codec.*only valid for format="mp4"/would silently break if the wording ever changes to e.g. "must be omitted for format=mov" — a tighter pin on the error class shape (expect(caught).toBeInstanceOf(Error)+ acodefield, like theFormatNotSupportedInDistributedErrorpattern this PR doesn't extend) would be more durable. Not a blocker — the regex is fine for now.
notes
- CI: required regression-shards are pending (not failed). The Graphite ❌ on the stack is the downstack-PR-open mergeability check + earlier-run CANCELLED states being re-superseded — not a code failure. Worth a final check before stack-merge.
planFormatBanlist.test.tswas intentionally not updated — codec validation lives inplan.test.tsalongside the codec knob, which is the right colocation. Mentioning so the absence isn't read as a miss.
Verdict: COMMENT
Reasoning: No correctness blocker — the knob threads through cleanly, planHash captures the choice, the closed-GOP h265 params already exist downstream. The unknown-codec silent fall-through is a real boundary-validation gap worth fixing before #851 ships traffic, but it's a narrow caller-input case, not a current-correctness issue. Author's call on whether to fold the renderChunk unit test + symmetric throw into this PR or land them as a follow-up.
— Vai
271dcda to
967ebe6
Compare
…-override helper Address @vanceingalls review on #850: 1. Unknown codec strings (typos like 'H265', future additions like 'av1') silently fell through to libx264 in resolveEncoderTriple. Add an explicit throw symmetric to the non-mp4-format branch already there. A JS caller building config from JSON who passes 'codec: "h266"' now gets a clear error at plan time instead of unflagged h264 output. 2. The preset.codec override in renderChunk had no fast unit coverage — only the heavyweight Docker fixture in #851 would catch a regression if someone refactored the spread (e.g. moved it into getEncoderPreset itself). Extract resolvePresetForLockedEncoder() and add 4 fast unit tests pinning the four encoder shapes (libx265/libx264/prores/png-seq). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
3495815 to
e7be7c8
Compare
|
Thanks @vanceingalls, @miguel-heygen — both
@miguel-heygen — The |
Same autoformat as PR #850; carrying it here so this branch's Format check passes once it triggers. No behavior change. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-stamping after rebase. LGTM.
…-override helper Address @vanceingalls review on #850: 1. Unknown codec strings (typos like 'H265', future additions like 'av1') silently fell through to libx264 in resolveEncoderTriple. Add an explicit throw symmetric to the non-mp4-format branch already there. A JS caller building config from JSON who passes 'codec: "h266"' now gets a clear error at plan time instead of unflagged h264 output. 2. The preset.codec override in renderChunk had no fast unit coverage — only the heavyweight Docker fixture in #851 would catch a regression if someone refactored the spread (e.g. moved it into getEncoderPreset itself). Extract resolvePresetForLockedEncoder() and add 4 fast unit tests pinning the four encoder shapes (libx265/libx264/prores/png-seq). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
The generic parameter constraint exceeded oxfmt's line width, so the formatter wraps the type-param list onto its own line. Applies the same formatting locally that CI's 'Format' job would have produced via 'bun run format:check' — no behavior change. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
b5d31a5 to
38e08e8
Compare
Merge activity
|
## Description Phase 4 of the distributed rendering plan: test fixtures (see DISTRIBUTED-RENDERING-PLAN.md §11 Phase 4 + §10 test strategy). This is PR 4.3 of the Phase 4 remainder, stacked on PR 4.3-pre (#850) which added the codec knob to `DistributedRenderConfig`. This PR adds the mp4 H.265 SDR fixture (`tests/distributed/mp4-h265-sdr/`) plus the small harness plumbing that exposes the codec knob through `meta.json`. Composition mirrors the H.264 fixture: 2 seconds (60 frames) at 30fps with text, a crossfade transition straddling the frame-30 chunk seam, and a continuously rotating SVG icon. `renderConfig.format: "mp4"` + `renderConfig.codec: "h265"` + `chunkSize: 15` routes the distributed pipeline through libx265 with closed-GOP keyint params (`min-keyint=N:scenecut=0:open-gop=0:repeat-headers=1`) so concat-copy at assemble time round-trips losslessly. **Cross-codec PSNR assertion**: the in-process renderer doesn't expose a codec hint (its RenderConfig only switches codec via HDR mode), so the in-process baseline for this fixture is rendered as h264. The harness's PSNR comparison therefore measures "libx265 chunked + concat" against "libx264 single-pass" on the same source frames. At "high" quality both encoders are near-lossless on simple vector+text content; observed PSNR is ~48dB across all 100 checkpoints — well above the 30dB threshold. This catches gross codec/encoder failures (e.g. libx265 emitting wrong bit depth or losing the IDR-at-chunk-seam contract) while accepting normal cross-codec PSNR drift. The in-process arm renders h264 vs the h264 baseline byte-identically. Harness extensions: 1. **`TestMetadata.renderConfig.codec`** field accepted by `validateMetadata`. Rejected with format ∉ {mp4} for symmetry with the `DistributedRenderConfig` runtime check from 4.3-pre. 2. **`RunDistributedSimulatedInput.codec`** plumbed through to `plan()`. The non-mp4 plan-config branch keeps the field structurally absent (so byte-identical to pre-codec planDirs for mov/png-sequence) rather than passing `undefined`, which would surface in JSON. ## Testing - `bun run --cwd packages/producer docker:test:update mp4-h265-sdr` — baseline rendered inside `Dockerfile.test` (h264 mp4 from in-process, used as the cross-codec reference) - `bun run --cwd packages/producer docker:test mp4-h265-sdr` — in-process passes (renders h264, byte-identical against h264 baseline) - `bun run --cwd packages/producer docker:test:distributed mp4-h265-sdr` — distributed-simulated passes (h265 mp4, ~48dB PSNR across all 100 checkpoints vs h264 baseline) - `bun run --cwd packages/producer docker:test:distributed font-variant-numeric many-cuts gsap-letters-render-compat style-1-prod sub-composition-video mp4-h264-sdr png-sequence mov-prores mp4-h265-sdr -- --sequential` — full smoke set + all 4 stacked fixtures pass (9/9) - `bunx oxlint` + `bunx oxfmt --check` clean - `bunx tsc --noEmit` (producer package) clean 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…852) ## Description Phase 4 of the distributed rendering plan: test fixtures (see DISTRIBUTED-RENDERING-PLAN.md §11 Phase 4 + §10 test strategy). This is PR 4.7 of the Phase 4 remainder — the final fixture PR. This PR adds six per-adapter chunk-boundary fixtures under `tests/distributed/{gsap,anime,three,lottie,css,waapi}-boundary/` plus a single `bun:test` driver (`chunkBoundary.test.ts`) that exercises each fixture's seek-determinism contract. Each fixture is a 60-frame composition (2s @ 30fps, 320×180) that drives the named adapter through the HyperFrames runtime's seek hook. The test renders each at `chunkSize=60` (N=1 chunk, no seams) and `chunkSize=15` (N=4 chunks, three seams at frames 15/30/45), then asserts every PNG frame is byte-identical across the two runs. **Why png-sequence**: mp4 bitstreams encode keyframe placement directly. At `chunkSize=60` libx264 emits 1 IDR; at `chunkSize=15` it emits 4 IDRs at frames 0/15/30/45. Those are legitimately different bytes even when the captured pixels are identical. png-sequence's assemble path merges chunk frame directories with no re-encode, so per-frame byte equality is exactly pixel equality — the strongest contract a distributed render can satisfy. **Fixture design** (each is ~60 lines of HTML): - `gsap-boundary` — single GSAP `tl.to(...)` driving translateX + rotation linearly across 2s. - `anime-boundary` — anime.js v4 timeline registered via `window.__hfAnime`. - `three-boundary` — minimal Three.js scene; cube rotation derived from `window.__hfThreeTime`. - `lottie-boundary` — inline Lottie JSON (rectangle layer animating position+rotation) loaded via `lottie-web` and registered via `window.__hfLottie`. - `css-boundary` — pure `@keyframes` animation; the HyperFrames CSS adapter seeks via `animation-delay`. - `waapi-boundary` — `element.animate()` with linear keyframes; runtime sets `currentTime` per frame. The fixtures intentionally omit `meta.json` so the regression-harness discovery skips them with a clear `missing meta.json` log (they're driven exclusively by `chunkBoundary.test.ts`). The test passes `rejectOnSystemFonts: false` because some adapter bundles (notably anime.js's IIFE) embed CSS-shaped strings inside their JS source — `font-family: ui-monospace, monospace` for internal devtools styling — which `validateNoSystemFonts`'s document-wide regex would otherwise false-positive on every adapter fixture that loads such a bundle. The fixtures display no text, so the relaxed font validation doesn't affect the contract under test. The 7th test case is a layout sanity check that asserts every expected `*-boundary` fixture directory exists. ## Testing - `bun test packages/producer/src/services/distributed/chunkBoundary.test.ts` — 7 tests pass on host (6 adapters × byte-identical N=1 vs N=4 + the layout check, 41.6s) - `bun test packages/producer/src/services/distributed/` — all 49 distributed unit tests pass (43.6s) - `bun run --cwd packages/producer docker:test:distributed font-variant-numeric many-cuts gsap-letters-render-compat style-1-prod sub-composition-video mp4-h264-sdr png-sequence mov-prores mp4-h265-sdr -- --sequential` — full smoke set + all four prior stacked fixtures pass (9/9) - `bunx oxlint` + `bunx oxfmt --check` clean - `bunx tsc --noEmit` (producer package) clean ## After this stack lands The Phase 4 fixture set is complete: PR 4.6 (#844) pins cross-worker idempotency; 4.2/4.3/4.4/4.5 (#845/#851/#847/#848) prove each format produces correct chunked output at the fixture's `minPsnr`; 4.3-pre (#850) added the codec knob H.265 needed; and this PR proves each first-party adapter's seek-determinism survives chunk seams. Phase 5 (CLI surface for `hyperframes plan/chunk/assemble`) and Phase 6 (AWS Lambda turnkey) are unblocked. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Description
Phase 4 prerequisite for PR 4.3 (the mp4 H.265 SDR distributed fixture). Splits the codec selection out of
DistributedRenderConfig.formatso callers can ask for libx265 without changing the format.Surface change (
@hyperframes/producer/distributed):DistributedRenderConfig.codec?: "h264" | "h265"— defaults to"h264", ignored for non-mp4 formats. Passingcodecwithformat !== "mp4"throws at plan time with a clear error so caller mistakes surface immediately rather than producing a silently-wrong planDir.FORMAT_ENCODER_TABLEis replaced byresolveEncoderTriple(config)— a small function that switches on(format, codec). mp4 + h265 →{encoder: "libx265-software", pixelFormat: "yuv420p"}. mov and png-sequence are unchanged.Plumbing through
renderChunk:The chunk worker reads
LockedRenderConfig.encoderfrommeta/encoder.json. When that's"libx265-software", the worker overridesgetEncoderPreset(quality, "mp4")'s defaultcodec: "h264"with"h265"sorunEncodeStageinvokes libx265 with the closed-GOP keyint params (min-keyint=N:scenecut=0:open-gop=0:repeat-headers=1) that survive concat-copy at assemble time. The engine layer (packages/engine/src/services/chunkEncoder.ts) already supports both codecs — this PR is purely the distributed config surface.Bit depth: SDR-only, 8-bit yuv420p for both codecs. h265 + 10-bit yuv420p10le is HDR territory and lives in v1.5 (see plan §12).
Testing
bun test packages/producer/src/services/distributed/plan.test.ts— 14 tests pass including 3 new codec cases (codecdefaults to h264,codec: "h265"maps to libx265-software, non-mp4 + codec throws)bun test packages/producer/src/services/distributed/— all 42 distributed unit tests passbunx oxlint+bunx oxfmt --checkcleanbunx tsc --noEmit(producer package) cleanThe H.265 fixture that exercises this end-to-end inside
Dockerfile.testlands in the follow-up PR 4.3.🤖 Generated with Claude Code