From 073b098e21aa006571b60c446711c6dff57a2eac Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Sat, 22 Aug 2026 04:23:34 -0700 Subject: [PATCH] feat(engine): preflight psnr filter availability and force-fallback to screenshot on missing (#3418) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What Adds a one-shot ffmpeg-psnr filter probe at drawElement session bootstrap. When the resident ffmpeg is missing or lacks libpostproc (no `psnr` filter), the capture-session router now force-fallbacks to the screenshot capture path and emits a `de_gate_reason = "ffmpeg_no_psnr_filter"` telemetry signal via the existing `render_complete` breakdown. Also tightens `psnrForDiskSample`'s catch: infrastructure-class ffmpeg failures (ENOENT, "No such filter") no longer silently skip the sample — they abort the render so the safety net cannot fail-open post-preflight. ## Why The drawElement self-verify safety net (parallelCoordinator's `psnrForDiskSample` → `psnrDb`) shells to `ffmpeg -lavfi psnr`. If ffmpeg is missing, or was compiled without libpostproc (so the `psnr` filter is absent), every per-sample compare throws. The existing catch swallows the error and returns `null` — callers treat that as "skip this sample" and the render completes with the safety net inoperative. Field signal (⭐ 9/10 CLI feedback, Slack ts=1787380767.210079, hyperframes 0.8.7, darwin/arm64, tid=93ff9910-2207-45c2-bc1f-54c0b347d4fe): > "host ffmpeg lacked psnr filter used by drawElement self-verification, > but render completed." The user's frames happened to be byte-identical so no visual damage shipped — but the safety net silently wasn't running. Any future compositor-damage bug on that host would have shipped straight through. ## How Two-part fix, both in `packages/engine`: 1. New `utils/psnrFilterAvailability.ts` — cached probe that runs `ffmpeg -hide_banner -filters` once per process and word-boundary- matches `psnr` in the output. Any failure (ENOENT, non-zero exit, timeout, unparseable output) returns `false`; never rejects. 2. Wired into `services/frameCapture.ts` `initDrawElementOrTransparentBackground` right after the Chrome capability probe: when useDrawElement resolves true and the preflight returns false, set `session.deGateReason = "ffmpeg_no_psnr_filter"` (same low-cardinality bucket every other DE gate uses; flows through `getCapturePerfSummary` → `render_complete.de_gate_reason` in PostHog), emit a stderr warning naming what's missing, and call `routeToFallback()` — the same fail-graceful shape as the SwiftShader / CSS-effect / at-risk-timeline gates. Skipped under `HF_FORCE_DRAWELEMENT=1` (matches the diagnostic knob's policy of bypassing every other gate). Belt-and-braces: `psnrForDiskSample` now discriminates infrastructure- class failures (ENOENT / "No such filter" / "Unknown filter") from per-sample noise (readFile races, transient EPERM). Only the former re-throw — per-sample noise still returns `null` (skipped sample). The preflight normally catches this at bootstrap; the re-throw covers ffmpeg-swapped-mid-render. ## Test plan - [x] Unit tests added: `packages/engine/src/utils/psnrFilterAvailability.test.ts` — mocked `execFile` covers: `psnr` present → true; `psnr` absent → false; ENOENT → false; non-zero exit → false; result memoized + reset works; substring-not-word-boundary → false. - [x] Unit tests added: `isFfmpegInfrastructureFailure` in `packages/engine/src/services/parallelCoordinator.test.ts` covers ENOENT, "No such filter", "Unknown filter", per-sample EACCES, parse errors, null/non-object. - [x] `bun run test` — `packages/engine/src/utils/psnrFilterAvailability.test.ts` (6 tests) + `packages/engine/src/services/parallelCoordinator.test.ts` (50 tests) + `frameCapture.test.ts` (26 tests) all pass. Pre-existing ffprobe test failures (4) on the base commit are unrelated (missing PNG fixture bytes — the file is 129 B on disk, likely LFS-stored). - [x] `bunx tsc --noEmit -p packages/engine/tsconfig.json` — clean. - [x] `bunx oxlint ` — 0 warnings, 0 errors. - [x] `bunx oxfmt --check ` — clean. Not covered here: an integration test that boots `initDrawElementOrTransparentBackground` end-to-end. That path is Puppeteer-driven and has no unit-scale bootstrap harness in the repository — the pure preflight + pure discriminator coverage above are what this PR can prove at the vitest layer. Co-authored-by: Claude Opus 4.7 --- packages/engine/src/services/frameCapture.ts | 23 +++ .../src/services/parallelCoordinator.test.ts | 58 ++++++ .../src/services/parallelCoordinator.ts | 43 ++++- .../src/utils/psnrFilterAvailability.test.ts | 172 ++++++++++++++++++ .../src/utils/psnrFilterAvailability.ts | 58 ++++++ 5 files changed, 351 insertions(+), 3 deletions(-) create mode 100644 packages/engine/src/utils/psnrFilterAvailability.test.ts create mode 100644 packages/engine/src/utils/psnrFilterAvailability.ts diff --git a/packages/engine/src/services/frameCapture.ts b/packages/engine/src/services/frameCapture.ts index c9991a20a..aefd3ea12 100644 --- a/packages/engine/src/services/frameCapture.ts +++ b/packages/engine/src/services/frameCapture.ts @@ -46,6 +46,7 @@ import { produceDrawElementFrameBatch, } from "./drawElementService.js"; import { initThreeDProjection, detectCssEffectRisk } from "./threeDProjection.js"; +import { isPsnrFilterAvailable } from "../utils/psnrFilterAvailability.js"; import { DEFAULT_CONFIG, applyConcreteGpuScreenshotClamp, type EngineConfig } from "../config.js"; import type { CaptureOptions, @@ -902,6 +903,28 @@ async function initDrawElementOrTransparentBackground( await routeToFallback(); return; } + // ffmpeg-psnr preflight: the disk-sample self-verify path + // (parallelCoordinator's psnrForDiskSample → psnrDb) shells to + // `ffmpeg -lavfi psnr`. When the resident ffmpeg is missing or was built + // without libpostproc, every per-sample compare throws and + // psnrForDiskSample swallows the error — the safety net silently fails + // open. Force-fallback to the reliable capture path so the safety net + // for drawElement isn't the one thing standing between a compositor bug + // and a shipped video. Skipped under HF_FORCE_DRAWELEMENT (matches the + // policy of every other gate below). + if (!forceDE && !(await isPsnrFilterAvailable())) { + session.deGateReason = "ffmpeg_no_psnr_filter"; + session.deFallbackTrigger = "ffmpeg_no_psnr_filter"; + console.warn( + `[engine] fast capture: falling back to ${session.launchCaptureMode} capture — ` + + "host ffmpeg is missing or was built without the `psnr` filter " + + "(libpostproc), so drawElement self-verification cannot run. Install " + + "an ffmpeg build that includes libpostproc (or set HYPERFRAMES_FFMPEG_PATH " + + "to one) to re-enable fast capture.", + ); + await routeToFallback(); + return; + } // SwiftShader gate: drawElement's only advantage is skipping the GPU→CPU // screenshot-readback IPC. On a software rasterizer (Docker/CI, no GPU) both // paths block on identical software raster, so drawElement is parity-or-slower diff --git a/packages/engine/src/services/parallelCoordinator.test.ts b/packages/engine/src/services/parallelCoordinator.test.ts index 43d4386d7..08b08ac99 100644 --- a/packages/engine/src/services/parallelCoordinator.test.ts +++ b/packages/engine/src/services/parallelCoordinator.test.ts @@ -6,6 +6,7 @@ import { expectedFramesForTask, flagSilentWorkerExits, formatWorkerFailure, + isFfmpegInfrastructureFailure, selectVerifySampleIndicesForTask, selectWorkerDiagnostics, shouldDisableBrowserPoolForParallelWorker, @@ -425,3 +426,60 @@ describe("resolveParallelDeVerifySamples", () => { expect(resolveParallelDeVerifySamples(2, 3)).toBe(2); }); }); + +describe("isFfmpegInfrastructureFailure", () => { + it("matches an execFile ENOENT (missing ffmpeg binary)", () => { + const err = Object.assign(new Error("spawn ffmpeg ENOENT"), { code: "ENOENT" }); + expect(isFfmpegInfrastructureFailure(err)).toBe(true); + }); + + it('matches ffmpeg\'s "No such filter" stderr (libpostproc-less build)', () => { + const err = Object.assign(new Error("Command failed"), { + stderr: + "Error initializing filter 'psnr' with args ''\n" + + " No such filter: 'psnr'\n" + + "Error opening filters!", + }); + expect(isFfmpegInfrastructureFailure(err)).toBe(true); + }); + + it("matches the older `Unknown filter 'psnr'` wording (ffmpeg <=5)", () => { + const err = Object.assign(new Error("Command failed"), { + stderr: "Unknown filter 'psnr'", + }); + expect(isFfmpegInfrastructureFailure(err)).toBe(true); + }); + + it("does not match per-sample noise (readFile race, transient EPERM)", () => { + const eperm = Object.assign(new Error("EACCES: permission denied, open '/tmp/…'"), { + code: "EACCES", + }); + expect(isFfmpegInfrastructureFailure(eperm)).toBe(false); + + const parseErr = new Error("psnr parse failed: average="); + expect(isFfmpegInfrastructureFailure(parseErr)).toBe(false); + + const enoentFile = Object.assign( + new Error("ENOENT: no such file or directory, open '/tmp/frame_000042.jpg'"), + { + code: "ENOENT", + }, + ); + // ⚠ known aliasing edge: an execFile ENOENT and an fs ENOENT reading the + // sample frame share the same code. The discriminator errs toward the + // infrastructure classification — a spurious per-sample fs ENOENT + // (impossible for a frame that was just written by the worker before this + // verify call) would abort the render, which is acceptable given how + // rarely that shape appears vs. how important the infra-fail signal is. + // Documented here so a future maintainer sees why the assertion below + // reads "true": this is the deliberate false-positive on collision. + expect(isFfmpegInfrastructureFailure(enoentFile)).toBe(true); + }); + + it("returns false for null / non-object errors", () => { + expect(isFfmpegInfrastructureFailure(null)).toBe(false); + expect(isFfmpegInfrastructureFailure(undefined)).toBe(false); + expect(isFfmpegInfrastructureFailure("psnr broken")).toBe(false); + expect(isFfmpegInfrastructureFailure(42)).toBe(false); + }); +}); diff --git a/packages/engine/src/services/parallelCoordinator.ts b/packages/engine/src/services/parallelCoordinator.ts index 3ee1bb637..35cb06d39 100644 --- a/packages/engine/src/services/parallelCoordinator.ts +++ b/packages/engine/src/services/parallelCoordinator.ts @@ -619,11 +619,37 @@ function assertDiskSampleAboveFloor( ); } +/** + * Distinguishes infrastructure-class ffmpeg failures (spawn ENOENT, missing + * `psnr` filter) from per-sample noise (readFile races, transient tmpdir + * EPERM). Only the infrastructure class should terminate the render — the + * ffmpeg preflight in `initDrawElementOrTransparentBackground` catches these + * at bootstrap, so surfacing them here means the preflight was bypassed or + * the host ffmpeg changed mid-render. + * + * Exported for testing; the discriminator is a pure error-shape read. + */ +export function isFfmpegInfrastructureFailure(err: unknown): boolean { + if (!err || typeof err !== "object") return false; + const record = err as { code?: unknown; message?: unknown; stderr?: unknown }; + if (record.code === "ENOENT") return true; + const message = typeof record.message === "string" ? record.message : ""; + const stderr = typeof record.stderr === "string" ? record.stderr : ""; + const text = `${message}\n${stderr}`; + // Spawn-side failures ("spawn ffmpeg ENOENT") and filter-side failures + // ("No such filter: 'psnr'", ffmpeg <=5 emits "Unknown filter 'psnr'"). + return /\bENOENT\b|No such filter|Unknown filter/i.test(text); +} + /** * Compare one captured frame file against its ground truth. Returns the - * PSNR, or null on infrastructure failure (missing file already surfaces - * via the frame completeness check; ffmpeg spawn/tmpdir here) — a skipped - * sample is not damage evidence and must not fail the capture. + * PSNR, or null on per-sample noise (readFile races, transient EPERM, + * unparseable ffmpeg output on a single sample) — a skipped sample is not + * damage evidence and must not fail the capture. Re-throws when the error + * shape indicates the ffmpeg install itself is broken (missing binary or + * missing `psnr` filter): the drawElement self-verify safety net cannot + * possibly run in that state, and continuing would silently ship every + * remaining frame unverified. */ async function psnrForDiskSample( framePath: string, @@ -634,6 +660,17 @@ async function psnrForDiskSample( try { return await psnrDb(await readFile(framePath), truth); } catch (err) { + if (isFfmpegInfrastructureFailure(err)) { + const detail = err instanceof Error ? err.message : String(err); + throw new Error( + `[Parallel] drawElement disk self-verify aborted (worker ${workerId}, frame ${idx}): ` + + `ffmpeg or the \`psnr\` filter is unavailable — ${detail}. The preflight in ` + + "initDrawElementOrTransparentBackground normally catches this at bootstrap; if you " + + "hit this after a successful preflight, ffmpeg was replaced mid-render or " + + "HYPERFRAMES_FFMPEG_PATH now points at a different binary.", + { cause: err }, + ); + } console.warn( `[Parallel] drawElement disk self-verify sample skipped (worker ${workerId}, ` + `frame ${idx}): ${err instanceof Error ? err.message : String(err)}`, diff --git a/packages/engine/src/utils/psnrFilterAvailability.test.ts b/packages/engine/src/utils/psnrFilterAvailability.test.ts new file mode 100644 index 000000000..53fb9ae0f --- /dev/null +++ b/packages/engine/src/utils/psnrFilterAvailability.test.ts @@ -0,0 +1,172 @@ +import { promisify } from "node:util"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +interface ExecFileCall { + file: string; + args: readonly string[]; +} + +type ExecFileOutcome = + | { kind: "ok"; stdout: string; stderr?: string } + | { kind: "exit_nonzero"; code: number; stdout?: string; stderr?: string } + | { kind: "enoent" }; + +// Node's built-in `child_process.execFile` carries a `util.promisify.custom` +// implementation that resolves to `{stdout, stderr}`. A plain-callback mock +// without that Symbol would be promisified as a single-result function, so +// `{stdout} = await execFileP(...)` would silently destructure to `undefined` +// — the exact hazard psnr.ts documents. Stamp the custom impl on the mock so +// promisify keeps the `{stdout, stderr}` shape. +function createExecFileSpy(outcome: ExecFileOutcome): { + execFile: ( + file: string, + args: readonly string[], + options: unknown, + callback: (err: Error | null, stdout?: string, stderr?: string) => void, + ) => void; + calls: ExecFileCall[]; +} { + const calls: ExecFileCall[] = []; + + async function run( + file: string, + args: readonly string[], + ): Promise<{ stdout: string; stderr: string }> { + calls.push({ file, args }); + if (outcome.kind === "enoent") { + const err = new Error("spawn ffmpeg ENOENT") as NodeJS.ErrnoException; + err.code = "ENOENT"; + throw err; + } + if (outcome.kind === "exit_nonzero") { + const err = new Error(`Command failed: ffmpeg (exit ${outcome.code})`) as Error & { + code: number; + stdout?: string; + stderr?: string; + }; + err.code = outcome.code; + err.stdout = outcome.stdout ?? ""; + err.stderr = outcome.stderr ?? ""; + throw err; + } + return { stdout: outcome.stdout, stderr: outcome.stderr ?? "" }; + } + + const execFile = (( + file: string, + args: readonly string[], + _options: unknown, + callback: (err: Error | null, stdout?: string, stderr?: string) => void, + ) => { + run(file, args).then( + ({ stdout, stderr }) => process.nextTick(() => callback(null, stdout, stderr)), + (err: Error) => process.nextTick(() => callback(err)), + ); + }) as (( + file: string, + args: readonly string[], + options: unknown, + callback: (err: Error | null, stdout?: string, stderr?: string) => void, + ) => void) & { [key: symbol]: unknown }; + (execFile as { [k: symbol]: unknown })[promisify.custom] = ( + file: string, + args: readonly string[], + ) => run(file, args); + + return { execFile, calls }; +} + +beforeEach(() => { + vi.resetModules(); +}); + +afterEach(() => { + vi.doUnmock("node:child_process"); +}); + +describe("isPsnrFilterAvailable", () => { + it("returns true when `ffmpeg -filters` output lists the psnr filter", async () => { + const { execFile } = createExecFileSpy({ + kind: "ok", + stdout: [ + "Filters:", + " T.. overlay VV->V Overlay a video source on top of the input.", + " T.. psnr VV->V Calculate the PSNR between two video streams.", + " ... yadif V->V Deinterlace the input image.", + ].join("\n"), + }); + vi.doMock("node:child_process", () => ({ execFile })); + + const { isPsnrFilterAvailable } = await import("./psnrFilterAvailability.js"); + await expect(isPsnrFilterAvailable()).resolves.toBe(true); + }); + + it("returns false when `ffmpeg -filters` output omits the psnr filter", async () => { + const { execFile } = createExecFileSpy({ + kind: "ok", + stdout: [ + "Filters:", + " T.. overlay VV->V Overlay a video source on top of the input.", + " ... yadif V->V Deinterlace the input image.", + ].join("\n"), + }); + vi.doMock("node:child_process", () => ({ execFile })); + + const { isPsnrFilterAvailable } = await import("./psnrFilterAvailability.js"); + await expect(isPsnrFilterAvailable()).resolves.toBe(false); + }); + + it("returns false when the ffmpeg binary is missing (ENOENT from execFile)", async () => { + const { execFile } = createExecFileSpy({ kind: "enoent" }); + vi.doMock("node:child_process", () => ({ execFile })); + + const { isPsnrFilterAvailable } = await import("./psnrFilterAvailability.js"); + await expect(isPsnrFilterAvailable()).resolves.toBe(false); + }); + + it("returns false on a non-zero exit from `ffmpeg -filters`", async () => { + const { execFile } = createExecFileSpy({ + kind: "exit_nonzero", + code: 1, + stderr: "Unrecognized option '-filters'.", + }); + vi.doMock("node:child_process", () => ({ execFile })); + + const { isPsnrFilterAvailable } = await import("./psnrFilterAvailability.js"); + await expect(isPsnrFilterAvailable()).resolves.toBe(false); + }); + + it("memoizes the probe across calls and re-probes after resetPsnrFilterAvailabilityCache", async () => { + const { execFile, calls } = createExecFileSpy({ + kind: "ok", + stdout: " T.. psnr VV->V Calculate the PSNR", + }); + vi.doMock("node:child_process", () => ({ execFile })); + + const { isPsnrFilterAvailable, resetPsnrFilterAvailabilityCache } = + await import("./psnrFilterAvailability.js"); + + await isPsnrFilterAvailable(); + await isPsnrFilterAvailable(); + await isPsnrFilterAvailable(); + expect(calls.length).toBe(1); + + resetPsnrFilterAvailabilityCache(); + await isPsnrFilterAvailable(); + expect(calls.length).toBe(2); + }); + + it("does not treat a whole-string 'psnr' inside another word as the filter", async () => { + const { execFile } = createExecFileSpy({ + kind: "ok", + stdout: [ + "Filters:", + " T.. bpsnrx V->V (hypothetical extended filter, not the real psnr)", + ].join("\n"), + }); + vi.doMock("node:child_process", () => ({ execFile })); + + const { isPsnrFilterAvailable } = await import("./psnrFilterAvailability.js"); + await expect(isPsnrFilterAvailable()).resolves.toBe(false); + }); +}); diff --git a/packages/engine/src/utils/psnrFilterAvailability.ts b/packages/engine/src/utils/psnrFilterAvailability.ts new file mode 100644 index 000000000..c252b4566 --- /dev/null +++ b/packages/engine/src/utils/psnrFilterAvailability.ts @@ -0,0 +1,58 @@ +import { execFile } from "node:child_process"; +import { promisify } from "node:util"; +import { getFfmpegBinary } from "./ffmpegBinaries.js"; + +/** + * Preflight for the ffmpeg `psnr` filter used by drawElement self-verify + * (see `psnr.ts`). Some host ffmpeg builds ship without `libpostproc` and + * silently omit the filter — every downstream `psnrDb()` call then throws + * mid-render and the disk-sample verifier swallows it (fail-open safety net). + * A cached one-shot probe surfaces the shape once, at bootstrap, so the + * capture-session router can force-fallback to the reliable screenshot path + * instead of arming a drawElement render whose safety net cannot run. + * + * Cache lifetime is the current process: an operator's ffmpeg install does + * not change across renders within the same CLI invocation, and re-probing + * per session would burn ~50-100ms of subprocess spawn per capture worker. + */ +let cached: Promise | null = null; + +/** + * Returns true when the resident ffmpeg exposes the `psnr` filter. False on + * any probe failure — missing binary (ENOENT), non-zero exit, timeout, + * unparseable output — because in every case the drawElement self-verify + * path cannot function. Never rejects. + * + * Result is memoized per process; call {@link resetPsnrFilterAvailabilityCache} + * from tests that need to re-probe. + */ +export function isPsnrFilterAvailable(): Promise { + if (cached === null) cached = probe(); + return cached; +} + +/** Test-only: drop the memoized probe result. */ +export function resetPsnrFilterAvailabilityCache(): void { + cached = null; +} + +async function probe(): Promise { + // Match `psnr.ts`: promisify lazily so a partial `node:child_process` mock + // (test that omits `execFile`) doesn't crash at module load — it fails at + // call time instead, and the try/catch below converts that to `false`. + const execFileP = promisify(execFile); + try { + const { stdout } = await execFileP(getFfmpegBinary(), ["-hide_banner", "-filters"], { + maxBuffer: 4 * 1024 * 1024, + timeout: 5_000, + }); + // ffmpeg's `-filters` output lists one filter per line, e.g. + // " T.. psnr VV->V Calculate the PSNR between two video streams." + // A whole-word match keeps `multi-psnr` (hypothetical) from masquerading + // as the real filter, and dodges the banner text that mentions PSNR in + // prose on some builds. + return /(^|\s)psnr(\s|$)/m.test(stdout); + } catch { + return false; + } +}