From bd21d00b13601d0d6b421fd117c8cdc962abcfd2 Mon Sep 17 00:00:00 2001 From: James Date: Thu, 14 May 2026 23:02:33 +0000 Subject: [PATCH] refactor(producer): /simplify Phase 4 distributed-rendering changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address findings from a three-agent code-review pass over the Phase 4 stack: - regression-harness: hoist `readdirSync` out of the per-checkpoint failure-extraction loop (was running 20 redundant syscalls on every failing png-sequence test). Drop redundant `existsSync` guards before `mkdirSync(recursive: true)` and `rmSync(force: true)`. Replace the three-deep ternary that built the output filename suffix with a single `Record` lookup. - regression-harness-distributed: flatten the `format === "mp4" ? {...} : {...}` branching in the `plan()` call into a single config object with a conditional spread. `plan()` already accepts `codec: undefined` for non-mp4 formats, so the duplicate object was unnecessary. - chunkBoundary.test: rename the stale "byte-identical mp4" test title to "byte-identical frames" (the test now uses png-sequence). Trim the 10-line comment justifying `rejectOnSystemFonts: false` to the essential WHY. - renderChunk / plan.test / regression-harness: drop trailing-edge comment phrases that pinned the prose to the PR's calendar context ("today", "v1.5", "pre-codec-knob output", section-numbered cross- references to the planning doc). No behavior change. All 49 distributed unit tests pass. Smoke + four distributed format fixtures pass in --mode=distributed-simulated. ๐Ÿค– Generated with [Claude Code](https://claude.com/claude-code) --- .../src/regression-harness-distributed.ts | 47 +++++------- packages/producer/src/regression-harness.ts | 71 +++++++++---------- .../distributed/chunkBoundary.test.ts | 49 +++++-------- .../src/services/distributed/plan.test.ts | 5 +- 4 files changed, 71 insertions(+), 101 deletions(-) diff --git a/packages/producer/src/regression-harness-distributed.ts b/packages/producer/src/regression-harness-distributed.ts index c62435b23..61123aa78 100644 --- a/packages/producer/src/regression-harness-distributed.ts +++ b/packages/producer/src/regression-harness-distributed.ts @@ -153,38 +153,25 @@ export async function runDistributedSimulatedRender( mkdirSync(planDir, { recursive: true }); mkdirSync(chunksDir, { recursive: true }); - // Step A: plan. `codec` is only forwarded when the format actually - // accepts it โ€” `plan()` throws if codec is set for a non-mp4 format, - // and a caller passing `format: "mov", codec: undefined` would still - // surface that field in the resulting object. We omit it conditionally - // to keep the off-path planDir identical to pre-codec-knob output. + // Step A: plan. `plan()` throws when `codec` is set with a non-mp4 format, + // but `codec: undefined` is a no-op โ€” so we forward it directly for mp4 + // and elide it for the others rather than branching the entire config. + // hdrMode is pinned to force-sdr so the harness's behavior is independent + // of any future auto-detect changes. const planResult = await plan( input.projectDir, - input.format === "mp4" - ? { - fps: input.fps, - width: 1920, - height: 1080, - format: "mp4", - codec: input.codec, - chunkSize: input.chunkSize, - maxParallelChunks: input.maxParallelChunks, - hdrMode: "force-sdr", - } - : { - fps: input.fps, - // Required-by-type but overridden by the composition's own attrs; - // see docstring above. Any positive integer works. - width: 1920, - height: 1080, - format: input.format, - chunkSize: input.chunkSize, - maxParallelChunks: input.maxParallelChunks, - // Force the SDR path explicitly โ€” `auto` would still resolve to - // force-sdr in distributed mode, but pinning it here keeps the - // harness's behavior independent of any future auto-detect changes. - hdrMode: "force-sdr", - }, + { + fps: input.fps, + // Required-by-type but overridden by the composition's `data-width` / + // `data-height` attrs; any positive integer works. + width: 1920, + height: 1080, + format: input.format, + ...(input.format === "mp4" && input.codec !== undefined ? { codec: input.codec } : {}), + chunkSize: input.chunkSize, + maxParallelChunks: input.maxParallelChunks, + hdrMode: "force-sdr", + }, planDir, ); diff --git a/packages/producer/src/regression-harness.ts b/packages/producer/src/regression-harness.ts index 776d14145..cff48b16d 100644 --- a/packages/producer/src/regression-harness.ts +++ b/packages/producer/src/regression-harness.ts @@ -372,12 +372,10 @@ function discoverTestSuites( if (!statSync(dir).isDirectory()) continue; if (entry === "node_modules" || entry.startsWith(".")) continue; - // `tests/distributed//` is the home for fixtures authored - // specifically for the distributed pipeline (see tests/README.md and - // DISTRIBUTED-RENDERING-PLAN.md ยง10.2). Recurse one level deeper so - // each `` becomes a first-class fixture ID (`mp4-h264-sdr`, - // `mov-prores`, โ€ฆ) the user can target on the CLI without their - // namespace prefix. + // `tests/distributed//` holds fixtures authored for the + // distributed pipeline. Recurse one level deeper so each `` + // becomes a first-class fixture ID the user can target on the CLI + // without a namespace prefix. if (entry === "distributed") { for (const sub of readdirSync(dir)) { const subDir = join(dir, sub); @@ -547,9 +545,7 @@ function saveFailureDetails( snapshotHtml?: string, ): void { const failuresDir = join(suite.dir, "failures"); - if (!existsSync(failuresDir)) { - mkdirSync(failuresDir, { recursive: true }); - } + mkdirSync(failuresDir, { recursive: true }); // Save compilation failures if (result.compilation && !result.compilation.passed) { @@ -608,30 +604,36 @@ function saveFailureDetails( const framesToExtract = failedCheckpoints.slice(0, 10); if (framesToExtract.length > 0) { const framesDir = join(failuresDir, "frames"); - if (!existsSync(framesDir)) { - mkdirSync(framesDir, { recursive: true }); - } + mkdirSync(framesDir, { recursive: true }); const renderedIsDir = existsSync(renderedVideoPath) && statSync(renderedVideoPath).isDirectory(); logPretty(`Extracting ${framesToExtract.length} failed frames...`, "๐Ÿ“ธ"); + // For directory output, sort both frame lists once โ€” they're static for + // the duration of the failure-extraction loop, so the per-checkpoint + // readdir+filter+sort the loop did before was wasted syscalls. + const renderedDirFrames = renderedIsDir + ? readdirSync(renderedVideoPath) + .filter((n) => n.toLowerCase().endsWith(".png")) + .sort() + : null; + const snapshotDirFrames = renderedIsDir + ? readdirSync(snapshotVideoPath) + .filter((n) => n.toLowerCase().endsWith(".png")) + .sort() + : null; + for (const checkpoint of framesToExtract) { const timeStr = checkpoint.time.toFixed(2).replace(".", "_"); try { - if (renderedIsDir) { + if (renderedDirFrames && snapshotDirFrames) { const frameIndex = Math.max( 0, Math.round(checkpoint.time * fpsToNumber(suite.meta.renderConfig.fps)), ); - const renderedFrames = readdirSync(renderedVideoPath) - .filter((n) => n.toLowerCase().endsWith(".png")) - .sort(); - const snapshotFrames = readdirSync(snapshotVideoPath) - .filter((n) => n.toLowerCase().endsWith(".png")) - .sort(); - const renderedFrame = renderedFrames[frameIndex]; - const snapshotFrame = snapshotFrames[frameIndex]; + const renderedFrame = renderedDirFrames[frameIndex]; + const snapshotFrame = snapshotDirFrames[frameIndex]; if (renderedFrame !== undefined) { copyFileSync( join(renderedVideoPath, renderedFrame), @@ -717,17 +719,15 @@ async function runTestSuite( const tempDownloadDir = join(tempRoot, "downloads"); const outputFormat = suite.meta.renderConfig.format ?? "mp4"; const isPngSequence = outputFormat === "png-sequence"; - // png-sequence output is a directory; encoded video outputs (mp4/mov/webm) - // are single files. `outputSuffix` is appended to the in-temp + baseline - // names so both shapes round-trip cleanly. - const outputSuffix = isPngSequence - ? "" - : outputFormat === "mp4" - ? ".mp4" - : outputFormat === "mov" - ? ".mov" - : ".webm"; - const outputBasename = isPngSequence ? "frames" : `output${outputSuffix}`; + // png-sequence output is a directory (basename = "frames"); encoded video + // formats produce a single file (basename = "output."). One lookup + // covers both shapes for the in-temp render and the on-disk baseline. + const VIDEO_EXT: Record<"mp4" | "mov" | "webm", string> = { + mp4: ".mp4", + mov: ".mov", + webm: ".webm", + }; + const outputBasename = isPngSequence ? "frames" : `output${VIDEO_EXT[outputFormat]}`; const renderedOutputPath = join(tempRoot, outputBasename); // Snapshot files stored in test's output/ directory. For png-sequence the @@ -882,10 +882,9 @@ async function runTestSuite( } if (isPngSequence) { // Frames directory โ€” recursive copy so every PNG lands at - // `/frames/.png`. - if (existsSync(snapshotVideoPath)) { - rmSync(snapshotVideoPath, { recursive: true, force: true }); - } + // `/frames/.png`. `rmSync(..., force: true)` + // tolerates a missing path, so the prior existsSync gate was redundant. + rmSync(snapshotVideoPath, { recursive: true, force: true }); cpSync(renderedOutputPath, snapshotVideoPath, { recursive: true }); } else { copyFileSync(renderedOutputPath, snapshotVideoPath); diff --git a/packages/producer/src/services/distributed/chunkBoundary.test.ts b/packages/producer/src/services/distributed/chunkBoundary.test.ts index 7e5829caa..5eba0b09e 100644 --- a/packages/producer/src/services/distributed/chunkBoundary.test.ts +++ b/packages/producer/src/services/distributed/chunkBoundary.test.ts @@ -1,34 +1,23 @@ /** * Per-adapter chunk-boundary contract: rendering the same composition at * chunkSize=N (single chunk, no seams) vs chunkSize=N/4 (four chunks, three - * seams at frames 15, 30, 45) MUST produce byte-identical *frames*. This - * is the strongest contract a distributed render can satisfy โ€” anything + * seams at frames 15, 30, 45) MUST produce byte-identical *frames*. Anything * weaker means the worker's seek-determinism leaks across chunk boundaries. * - * Output format is png-sequence rather than mp4 because mp4 bitstreams - * encode keyframe placement directly: chunkSize=60 emits 1 IDR; chunkSize=15 - * emits 4 IDRs at frames 0/15/30/45. Those are legitimately different bytes - * even when the captured pixels are identical. The png-sequence assemble - * path merges chunk frame directories with no re-encode, so per-frame - * byte equality round-trips a pixel-level contract. + * Output is png-sequence rather than mp4 because mp4 bitstreams encode + * keyframe placement directly: chunkSize=60 emits 1 IDR; chunkSize=15 emits + * 4 IDRs at frames 0/15/30/45. Those are legitimately different bytes even + * when the captured pixels are identical. The png-sequence assemble path + * merges chunk frame directories with no re-encode, so per-frame byte + * equality is exactly pixel equality. * * For each first-party adapter (GSAP, Anime.js, Three.js, Lottie, CSS, * WAAPI), `tests/distributed/-boundary/src/index.html` is a * 60-frame composition that drives the adapter through its registered seek - * hook. The test: - * - * 1. plan() + renderChunk() ร— N + assemble() at chunkSize=60 โ†’ N=1 chunk. - * 2. Same at chunkSize=15 โ†’ N=4 chunks. - * 3. Per-frame `Buffer.equals` across the two output frame directories. - * - * Fixtures with no checked-in baseline aren't compared by the regression - * harness โ€” they're driven from here via `bun test`. CI exercises them - * through the same `bun test` step inside `Dockerfile.test`. - * - * Soft-skip behavior matches `renderChunk.test.ts`: if the host's - * `chrome-headless-shell` can't render (no SwiftShader, missing GL stack), - * the test logs a warning and returns. The Docker harness covers the real - * contract against a known-good image. + * hook. The fixtures intentionally lack a `meta.json` so they're invisible + * to the regression harness; this test owns them. On hosts whose + * chrome-headless-shell can't render (no SwiftShader / missing GL stack), + * each subtest soft-skips and the Docker harness covers the contract. */ import { afterAll, beforeAll, describe, expect, it } from "bun:test"; @@ -86,15 +75,11 @@ async function planAndAssemble(input: { // affects keyframe placement in the bitstream. format: "png-sequence", chunkSize: input.chunkSize, - // Some adapter bundles (notably anime.js's IIFE) embed CSS-shaped - // strings inside their JS โ€” `font-family: ui-monospace, monospace` - // for internal devtools styling. `validateNoSystemFonts` scans the - // entire compiled HTML and matches those JS string literals, which - // would false-positive every chunk-boundary fixture that loads - // such a bundle. Disable the check for this test only; the fixtures - // never display text and the byte-identity contract is independent - // of which fonts the page would resolve. This is the documented - // escape hatch for the option. + // anime.js's IIFE bundle embeds `font-family: ui-monospace, monospace` + // as a string literal inside its JS, which `validateNoSystemFonts`'s + // document-wide regex false-positives. These fixtures display no text, + // so disabling the check (the documented escape hatch on this flag) is + // safe. rejectOnSystemFonts: false, }, planDir, @@ -122,7 +107,7 @@ describe("per-adapter chunk-boundary byte equality", () => { for (const adapter of ADAPTERS) { it( - `${adapter}: chunkSize=60 (N=1) vs chunkSize=15 (N=4) produces byte-identical mp4`, + `${adapter}: chunkSize=60 (N=1) vs chunkSize=15 (N=4) produces byte-identical frames`, async () => { const fixtureDir = join(testsDistributedDir, `${adapter}-boundary`); if (!existsSync(join(fixtureDir, "src", "index.html"))) { diff --git a/packages/producer/src/services/distributed/plan.test.ts b/packages/producer/src/services/distributed/plan.test.ts index 42efb383b..36043d58d 100644 --- a/packages/producer/src/services/distributed/plan.test.ts +++ b/packages/producer/src/services/distributed/plan.test.ts @@ -242,9 +242,8 @@ describe("plan() โ€” codec knob", () => { readFileSync(join(planDir, "meta", "encoder.json"), "utf-8"), ) as Record; expect(encoder.encoder).toBe("libx265-software"); - // SDR 8-bit yuv420p, same as h264. Distributed mode is SDR-only โ€” - // anyone reading this and tempted to bump to 10-bit, that's HDR - // territory and lives in v1.5. + // SDR 8-bit yuv420p, same as h264 โ€” distributed mode is SDR-only and + // 10-bit / HDR pixelFormat selection is not exposed on this surface. expect(encoder.pixelFormat).toBe("yuv420p"); }, TIMEOUT_MS,