test(producer): add mp4 H.265 SDR distributed fixture#851
Conversation
jrusso1020
force-pushed
the
05-14-feat_producer_add_codec_knob_to_distributedrenderconfig
branch
from
5e00c8f to
3495815
Compare
jrusso1020
force-pushed
the
05-14-test_producer_add_mp4_h.265_sdr_distributed_fixture
branch
from
dadb453 to
32c5f88
Compare
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed as part of the fixture stack. LGTM.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
H.265 SDR distributed fixture — surface and harness wiring look correct, codec gating matches the contract introduced in #850, and the PSNR-floor reasoning is honestly stated in the meta description. Independent review from Vai, additive to @miguel-heygen's approval.
Calibrated strengths
regression-harness.ts:271-275— surfacing thecodec∉ {h264, h265} andcodec⊗ non-mp4 errors at fixture-load time (not run time) is exactly the right validation site. Caller mistakes surface immediately and the error message names the offending value.meta.jsondescription (tests/distributed/mp4-h265-sdr/meta.json) — honest about the cross-codec PSNR pattern (h265 chunked + concatvsh264 single-pass) instead of papering over it. The "~30-35 dB normal drift, observed ~48 dB" framing makes the 18 dB floor margin defensible.- Closed-GOP claim in the PR body is verified —
chunkEncoder.ts:235-236emitskeyint=N:min-keyint=N:scenecut=0:open-gop=0:repeat-headers=1exactly whenrenderChunk.ts:603passeslockGopForChunkConcat: true+gopSize: framesInChunk. Concat-copy at assemble time should round-trip losslessly.
Findings
nit — over-engineered conditional plan-config branch. regression-harness-distributed.ts:156-184 builds two parallel config literals to keep codec "structurally absent" from the non-mp4 plan input. The stated goal is byte-identity of the off-path planDir against pre-codec-knob output — but codec never enters LockedRenderConfig (see freezePlan.ts:21-49), only the resolved encoder string does. plan() doesn't serialize the raw input config into the planDir, so passing codec: undefined for a non-mp4 format would produce a byte-identical planDir anyway, and plan() already throws when codec !== undefined && format !== "mp4" (plan.ts:485-490). The conditional protects a property already guaranteed elsewhere. Consider collapsing to a single object literal with codec: input.format === "mp4" ? input.codec : undefined, or just codec: input.codec and let plan()'s validator catch caller errors. The comment block on lines 156-160 also overstates the determinism risk.
nit — explanatory comments dropped from the mp4 branch. The non-mp4 branch in the same conditional retains the // Required-by-type but overridden by the composition's own attrs; and // Force the SDR path explicitly — … comments, but the mp4 branch (lines 162-173) drops both. The mp4 branch is the path you actually expect callers to exercise; doc context shouldn't live exclusively on the legacy path. Either lift the comments to the function docstring or carry them on both branches.
nit — fixture HTML is a near-duplicate of #845 with the inline rationale comments stripped. tests/distributed/mp4-h265-sdr/src/index.html is structurally identical to tests/distributed/mp4-h264-sdr/src/index.html except for label text ("CODEC"/"HEVC ONE" vs "CHUNK"/"PHASE ONE") and the loss of three inline comments: the chunk-seam-mapping comment, the icon-rotation continuity comment, and the audio-omission rationale. The meta.json description carries some of this, but the per-line "why" — especially the audio-omission rationale, which is non-obvious and easy to regress in a future copy — should not live in only one of two sibling fixtures. Recommend either (a) carrying the same comments in this fixture for parity, or (b) factoring the shared bits into a tests/distributed/README.md note that both fixtures point at. Two fixtures isn't yet "extract shared scaffolding" territory, but the comments-only divergence is.
important (cross-PR, flag for the chunk-boundary fixture in #852) — codec coverage gap is intentional but worth pinning. Because lockGopForChunkConcat: true forces -bf 0 for h265 (chunkEncoder.ts:207-212), this fixture exercises libx265 only with B-frames disabled. H.265's distinctive inter-frame ref structure is not exercised at all. That matches the production contract (distributed mode is closed-GOP, no B-frames) and so is correct — but it means the fixture's value is "did libx265 produce a parseable + comparable mp4 with the same composition," not "did the H.265 chunk-boundary contract hold under typical H.265 ref structure." If #852 introduces chunk-boundary fixtures per adapter, the H.265 variant there should explicitly call out the no-B-frame assumption so a future reader doesn't assume the test catches B-frame-related regressions.
nit — validateMetadata codec/format check is asymmetric. regression-harness.ts:272 checks rc.codec !== undefined && rc.format !== undefined && rc.format !== "mp4". When format is omitted, it defaults to mp4 — so codec is legal — which is functionally right but relies on the reader knowing the default. Consider either normalizing rc.format ??= "mp4" before the check or rephrasing the condition as (rc.format ?? "mp4") !== "mp4" to remove the silent default. Behavior identical; intent clearer.
Audited
packages/producer/src/regression-harness-distributed.ts, packages/producer/src/regression-harness.ts, packages/producer/tests/distributed/mp4-h265-sdr/{meta.json,src/index.html} (read end-to-end), cross-referenced with #845 + #850 + services/distributed/plan.ts + services/distributed/renderChunk.ts + services/render/stages/freezePlan.ts + engine/services/chunkEncoder.ts on the stack branch. output/output.mp4 not byte-inspected (binary LFS).
Trusting
The PR-body claim that bun run --cwd packages/producer docker:test:distributed mp4-h265-sdr produces ~48 dB across all 100 checkpoints — not reproduced locally; relying on the author's reported numbers and the 18 dB floor margin.
Verdict: COMMENT (would APPROVE — nothing blocks merge; the nits above are non-gating)
Reasoning: Surface and harness wiring are correct, codec gating + closed-GOP claim verify against the actual chunkEncoder code on the stack branch, and the cross-codec PSNR pattern is honestly framed in the meta description. Posting as COMMENT to avoid the self-stamp guard given Miguel's already-merged APPROVE; the H.264 sibling fixture lives one PR upstack and inherits the same comments-divergence nit.
Review by Vai
…-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)
jrusso1020
force-pushed
the
05-14-feat_producer_add_codec_knob_to_distributedrenderconfig
branch
from
3495815 to
e7be7c8
Compare
…ale to mp4-h265-sdr fixture Address @vanceingalls review on #851: 1. validateMetadata's codec/format check read 'rc.codec !== undefined && rc.format !== undefined && rc.format !== "mp4"'. The behavior was correct (omitted format defaults to mp4 downstream so codec is legal) but relied on the reader knowing that default. Normalize 'effectiveFormat = rc.format ?? "mp4"' before the comparison so the intent reads directly. 2. The mp4-h264-sdr sibling carries inline rationale for the no-audio choice (AAC frame quantization extends container.duration past nb_frames/fps and trips the harness PSNR sampler) and the chunk-seam mapping (crossfade window 0.9-1.1s straddles frame 30). mp4-h265-sdr stripped both. Carry them back so the two fixtures stay parallel. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
jrusso1020
force-pushed
the
05-14-test_producer_add_mp4_h.265_sdr_distributed_fixture
branch
from
32c5f88 to
ee4fc8c
Compare
|
Thanks @vanceingalls — addressed in commit
On the over-engineered conditional plan-config branch — that was already collapsed during the The H.265 no-B-frame coverage note is a real forward-looking call-out — flagged for the H.265 entry of any future chunk-boundary fixture matrix. |
jrusso1020
changed the base branch from
05-14-feat_producer_add_codec_knob_to_distributedrenderconfig
to
graphite-base/851
…-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)
…ale to mp4-h265-sdr fixture Address @vanceingalls review on #851: 1. validateMetadata's codec/format check read 'rc.codec !== undefined && rc.format !== undefined && rc.format !== "mp4"'. The behavior was correct (omitted format defaults to mp4 downstream so codec is legal) but relied on the reader knowing that default. Normalize 'effectiveFormat = rc.format ?? "mp4"' before the comparison so the intent reads directly. 2. The mp4-h264-sdr sibling carries inline rationale for the no-audio choice (AAC frame quantization extends container.duration past nb_frames/fps and trips the harness PSNR sampler) and the chunk-seam mapping (crossfade window 0.9-1.1s straddles frame 30). mp4-h265-sdr stripped both. Carry them back so the two fixtures stay parallel. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
jrusso1020
force-pushed
the
graphite-base/851
branch
from
e7be7c8 to
2886c2b
Compare
jrusso1020
force-pushed
the
05-14-test_producer_add_mp4_h.265_sdr_distributed_fixture
branch
from
1869e8b to
fb7a0a4
Compare
graphite-app
Bot
dismissed
miguel-heygen’s stale review
The base branch was changed.
…ale to mp4-h265-sdr fixture Address @vanceingalls review on #851: 1. validateMetadata's codec/format check read 'rc.codec !== undefined && rc.format !== undefined && rc.format !== "mp4"'. The behavior was correct (omitted format defaults to mp4 downstream so codec is legal) but relied on the reader knowing that default. Normalize 'effectiveFormat = rc.format ?? "mp4"' before the comparison so the intent reads directly. 2. The mp4-h264-sdr sibling carries inline rationale for the no-audio choice (AAC frame quantization extends container.duration past nb_frames/fps and trips the harness PSNR sampler) and the chunk-seam mapping (crossfade window 0.9-1.1s straddles frame 30). mp4-h265-sdr stripped both. Carry them back so the two fixtures stay parallel. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
jrusso1020
force-pushed
the
05-14-test_producer_add_mp4_h.265_sdr_distributed_fixture
branch
from
fb7a0a4 to
6dbdac8
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-stamping after rebase. LGTM.
Merge activity
|
jrusso1020
deleted the
05-14-test_producer_add_mp4_h.265_sdr_distributed_fixture
branch
…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)
4 participants
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 throughmeta.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: 15routes 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:
TestMetadata.renderConfig.codecfield accepted byvalidateMetadata. Rejected with format ∉ {mp4} for symmetry with theDistributedRenderConfigruntime check from 4.3-pre.RunDistributedSimulatedInput.codecplumbed through toplan(). The non-mp4 plan-config branch keeps the field structurally absent (so byte-identical to pre-codec planDirs for mov/png-sequence) rather than passingundefined, which would surface in JSON.Testing
bun run --cwd packages/producer docker:test:update mp4-h265-sdr— baseline rendered insideDockerfile.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 --checkcleanbunx tsc --noEmit(producer package) clean🤖 Generated with Claude Code