Skip to content

Commit c4fe259

Browse files
vanceingallsclaude
andcommitted
feat(engine): preflight psnr filter availability and force-fallback to screenshot on missing
## 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 <files>` — 0 warnings, 0 errors. - [x] `bunx oxfmt --check <files>` — 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 <noreply@anthropic.com>
1 parent 718bf5e commit c4fe259

5 files changed

Lines changed: 351 additions & 3 deletions

File tree

packages/engine/src/services/frameCapture.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ import {
4646
produceDrawElementFrameBatch,
4747
} from "./drawElementService.js";
4848
import { initThreeDProjection, detectCssEffectRisk } from "./threeDProjection.js";
49+
import { isPsnrFilterAvailable } from "../utils/psnrFilterAvailability.js";
4950
import { DEFAULT_CONFIG, applyConcreteGpuScreenshotClamp, type EngineConfig } from "../config.js";
5051
import type {
5152
CaptureOptions,
@@ -902,6 +903,28 @@ async function initDrawElementOrTransparentBackground(
902903
await routeToFallback();
903904
return;
904905
}
906+
// ffmpeg-psnr preflight: the disk-sample self-verify path
907+
// (parallelCoordinator's psnrForDiskSample → psnrDb) shells to
908+
// `ffmpeg -lavfi psnr`. When the resident ffmpeg is missing or was built
909+
// without libpostproc, every per-sample compare throws and
910+
// psnrForDiskSample swallows the error — the safety net silently fails
911+
// open. Force-fallback to the reliable capture path so the safety net
912+
// for drawElement isn't the one thing standing between a compositor bug
913+
// and a shipped video. Skipped under HF_FORCE_DRAWELEMENT (matches the
914+
// policy of every other gate below).
915+
if (!forceDE && !(await isPsnrFilterAvailable())) {
916+
session.deGateReason = "ffmpeg_no_psnr_filter";
917+
session.deFallbackTrigger = "ffmpeg_no_psnr_filter";
918+
console.warn(
919+
`[engine] fast capture: falling back to ${session.launchCaptureMode} capture — ` +
920+
"host ffmpeg is missing or was built without the `psnr` filter " +
921+
"(libpostproc), so drawElement self-verification cannot run. Install " +
922+
"an ffmpeg build that includes libpostproc (or set HYPERFRAMES_FFMPEG_PATH " +
923+
"to one) to re-enable fast capture.",
924+
);
925+
await routeToFallback();
926+
return;
927+
}
905928
// SwiftShader gate: drawElement's only advantage is skipping the GPU→CPU
906929
// screenshot-readback IPC. On a software rasterizer (Docker/CI, no GPU) both
907930
// paths block on identical software raster, so drawElement is parity-or-slower

packages/engine/src/services/parallelCoordinator.test.ts

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
expectedFramesForTask,
77
flagSilentWorkerExits,
88
formatWorkerFailure,
9+
isFfmpegInfrastructureFailure,
910
selectVerifySampleIndicesForTask,
1011
selectWorkerDiagnostics,
1112
shouldDisableBrowserPoolForParallelWorker,
@@ -425,3 +426,60 @@ describe("resolveParallelDeVerifySamples", () => {
425426
expect(resolveParallelDeVerifySamples(2, 3)).toBe(2);
426427
});
427428
});
429+
430+
describe("isFfmpegInfrastructureFailure", () => {
431+
it("matches an execFile ENOENT (missing ffmpeg binary)", () => {
432+
const err = Object.assign(new Error("spawn ffmpeg ENOENT"), { code: "ENOENT" });
433+
expect(isFfmpegInfrastructureFailure(err)).toBe(true);
434+
});
435+
436+
it('matches ffmpeg\'s "No such filter" stderr (libpostproc-less build)', () => {
437+
const err = Object.assign(new Error("Command failed"), {
438+
stderr:
439+
"Error initializing filter 'psnr' with args ''\n" +
440+
" No such filter: 'psnr'\n" +
441+
"Error opening filters!",
442+
});
443+
expect(isFfmpegInfrastructureFailure(err)).toBe(true);
444+
});
445+
446+
it("matches the older `Unknown filter 'psnr'` wording (ffmpeg <=5)", () => {
447+
const err = Object.assign(new Error("Command failed"), {
448+
stderr: "Unknown filter 'psnr'",
449+
});
450+
expect(isFfmpegInfrastructureFailure(err)).toBe(true);
451+
});
452+
453+
it("does not match per-sample noise (readFile race, transient EPERM)", () => {
454+
const eperm = Object.assign(new Error("EACCES: permission denied, open '/tmp/…'"), {
455+
code: "EACCES",
456+
});
457+
expect(isFfmpegInfrastructureFailure(eperm)).toBe(false);
458+
459+
const parseErr = new Error("psnr parse failed: average=<truncated>");
460+
expect(isFfmpegInfrastructureFailure(parseErr)).toBe(false);
461+
462+
const enoentFile = Object.assign(
463+
new Error("ENOENT: no such file or directory, open '/tmp/frame_000042.jpg'"),
464+
{
465+
code: "ENOENT",
466+
},
467+
);
468+
// ⚠ known aliasing edge: an execFile ENOENT and an fs ENOENT reading the
469+
// sample frame share the same code. The discriminator errs toward the
470+
// infrastructure classification — a spurious per-sample fs ENOENT
471+
// (impossible for a frame that was just written by the worker before this
472+
// verify call) would abort the render, which is acceptable given how
473+
// rarely that shape appears vs. how important the infra-fail signal is.
474+
// Documented here so a future maintainer sees why the assertion below
475+
// reads "true": this is the deliberate false-positive on collision.
476+
expect(isFfmpegInfrastructureFailure(enoentFile)).toBe(true);
477+
});
478+
479+
it("returns false for null / non-object errors", () => {
480+
expect(isFfmpegInfrastructureFailure(null)).toBe(false);
481+
expect(isFfmpegInfrastructureFailure(undefined)).toBe(false);
482+
expect(isFfmpegInfrastructureFailure("psnr broken")).toBe(false);
483+
expect(isFfmpegInfrastructureFailure(42)).toBe(false);
484+
});
485+
});

packages/engine/src/services/parallelCoordinator.ts

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -619,11 +619,37 @@ function assertDiskSampleAboveFloor(
619619
);
620620
}
621621

622+
/**
623+
* Distinguishes infrastructure-class ffmpeg failures (spawn ENOENT, missing
624+
* `psnr` filter) from per-sample noise (readFile races, transient tmpdir
625+
* EPERM). Only the infrastructure class should terminate the render — the
626+
* ffmpeg preflight in `initDrawElementOrTransparentBackground` catches these
627+
* at bootstrap, so surfacing them here means the preflight was bypassed or
628+
* the host ffmpeg changed mid-render.
629+
*
630+
* Exported for testing; the discriminator is a pure error-shape read.
631+
*/
632+
export function isFfmpegInfrastructureFailure(err: unknown): boolean {
633+
if (!err || typeof err !== "object") return false;
634+
const record = err as { code?: unknown; message?: unknown; stderr?: unknown };
635+
if (record.code === "ENOENT") return true;
636+
const message = typeof record.message === "string" ? record.message : "";
637+
const stderr = typeof record.stderr === "string" ? record.stderr : "";
638+
const text = `${message}\n${stderr}`;
639+
// Spawn-side failures ("spawn ffmpeg ENOENT") and filter-side failures
640+
// ("No such filter: 'psnr'", ffmpeg <=5 emits "Unknown filter 'psnr'").
641+
return /\bENOENT\b|No such filter|Unknown filter/i.test(text);
642+
}
643+
622644
/**
623645
* Compare one captured frame file against its ground truth. Returns the
624-
* PSNR, or null on infrastructure failure (missing file already surfaces
625-
* via the frame completeness check; ffmpeg spawn/tmpdir here) — a skipped
626-
* sample is not damage evidence and must not fail the capture.
646+
* PSNR, or null on per-sample noise (readFile races, transient EPERM,
647+
* unparseable ffmpeg output on a single sample) — a skipped sample is not
648+
* damage evidence and must not fail the capture. Re-throws when the error
649+
* shape indicates the ffmpeg install itself is broken (missing binary or
650+
* missing `psnr` filter): the drawElement self-verify safety net cannot
651+
* possibly run in that state, and continuing would silently ship every
652+
* remaining frame unverified.
627653
*/
628654
async function psnrForDiskSample(
629655
framePath: string,
@@ -634,6 +660,17 @@ async function psnrForDiskSample(
634660
try {
635661
return await psnrDb(await readFile(framePath), truth);
636662
} catch (err) {
663+
if (isFfmpegInfrastructureFailure(err)) {
664+
const detail = err instanceof Error ? err.message : String(err);
665+
throw new Error(
666+
`[Parallel] drawElement disk self-verify aborted (worker ${workerId}, frame ${idx}): ` +
667+
`ffmpeg or the \`psnr\` filter is unavailable — ${detail}. The preflight in ` +
668+
"initDrawElementOrTransparentBackground normally catches this at bootstrap; if you " +
669+
"hit this after a successful preflight, ffmpeg was replaced mid-render or " +
670+
"HYPERFRAMES_FFMPEG_PATH now points at a different binary.",
671+
{ cause: err },
672+
);
673+
}
637674
console.warn(
638675
`[Parallel] drawElement disk self-verify sample skipped (worker ${workerId}, ` +
639676
`frame ${idx}): ${err instanceof Error ? err.message : String(err)}`,
Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
import { promisify } from "node:util";
2+
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
3+
4+
interface ExecFileCall {
5+
file: string;
6+
args: readonly string[];
7+
}
8+
9+
type ExecFileOutcome =
10+
| { kind: "ok"; stdout: string; stderr?: string }
11+
| { kind: "exit_nonzero"; code: number; stdout?: string; stderr?: string }
12+
| { kind: "enoent" };
13+
14+
// Node's built-in `child_process.execFile` carries a `util.promisify.custom`
15+
// implementation that resolves to `{stdout, stderr}`. A plain-callback mock
16+
// without that Symbol would be promisified as a single-result function, so
17+
// `{stdout} = await execFileP(...)` would silently destructure to `undefined`
18+
// — the exact hazard psnr.ts documents. Stamp the custom impl on the mock so
19+
// promisify keeps the `{stdout, stderr}` shape.
20+
function createExecFileSpy(outcome: ExecFileOutcome): {
21+
execFile: (
22+
file: string,
23+
args: readonly string[],
24+
options: unknown,
25+
callback: (err: Error | null, stdout?: string, stderr?: string) => void,
26+
) => void;
27+
calls: ExecFileCall[];
28+
} {
29+
const calls: ExecFileCall[] = [];
30+
31+
async function run(
32+
file: string,
33+
args: readonly string[],
34+
): Promise<{ stdout: string; stderr: string }> {
35+
calls.push({ file, args });
36+
if (outcome.kind === "enoent") {
37+
const err = new Error("spawn ffmpeg ENOENT") as NodeJS.ErrnoException;
38+
err.code = "ENOENT";
39+
throw err;
40+
}
41+
if (outcome.kind === "exit_nonzero") {
42+
const err = new Error(`Command failed: ffmpeg (exit ${outcome.code})`) as Error & {
43+
code: number;
44+
stdout?: string;
45+
stderr?: string;
46+
};
47+
err.code = outcome.code;
48+
err.stdout = outcome.stdout ?? "";
49+
err.stderr = outcome.stderr ?? "";
50+
throw err;
51+
}
52+
return { stdout: outcome.stdout, stderr: outcome.stderr ?? "" };
53+
}
54+
55+
const execFile = ((
56+
file: string,
57+
args: readonly string[],
58+
_options: unknown,
59+
callback: (err: Error | null, stdout?: string, stderr?: string) => void,
60+
) => {
61+
run(file, args).then(
62+
({ stdout, stderr }) => process.nextTick(() => callback(null, stdout, stderr)),
63+
(err: Error) => process.nextTick(() => callback(err)),
64+
);
65+
}) as ((
66+
file: string,
67+
args: readonly string[],
68+
options: unknown,
69+
callback: (err: Error | null, stdout?: string, stderr?: string) => void,
70+
) => void) & { [key: symbol]: unknown };
71+
(execFile as { [k: symbol]: unknown })[promisify.custom] = (
72+
file: string,
73+
args: readonly string[],
74+
) => run(file, args);
75+
76+
return { execFile, calls };
77+
}
78+
79+
beforeEach(() => {
80+
vi.resetModules();
81+
});
82+
83+
afterEach(() => {
84+
vi.doUnmock("node:child_process");
85+
});
86+
87+
describe("isPsnrFilterAvailable", () => {
88+
it("returns true when `ffmpeg -filters` output lists the psnr filter", async () => {
89+
const { execFile } = createExecFileSpy({
90+
kind: "ok",
91+
stdout: [
92+
"Filters:",
93+
" T.. overlay VV->V Overlay a video source on top of the input.",
94+
" T.. psnr VV->V Calculate the PSNR between two video streams.",
95+
" ... yadif V->V Deinterlace the input image.",
96+
].join("\n"),
97+
});
98+
vi.doMock("node:child_process", () => ({ execFile }));
99+
100+
const { isPsnrFilterAvailable } = await import("./psnrFilterAvailability.js");
101+
await expect(isPsnrFilterAvailable()).resolves.toBe(true);
102+
});
103+
104+
it("returns false when `ffmpeg -filters` output omits the psnr filter", async () => {
105+
const { execFile } = createExecFileSpy({
106+
kind: "ok",
107+
stdout: [
108+
"Filters:",
109+
" T.. overlay VV->V Overlay a video source on top of the input.",
110+
" ... yadif V->V Deinterlace the input image.",
111+
].join("\n"),
112+
});
113+
vi.doMock("node:child_process", () => ({ execFile }));
114+
115+
const { isPsnrFilterAvailable } = await import("./psnrFilterAvailability.js");
116+
await expect(isPsnrFilterAvailable()).resolves.toBe(false);
117+
});
118+
119+
it("returns false when the ffmpeg binary is missing (ENOENT from execFile)", async () => {
120+
const { execFile } = createExecFileSpy({ kind: "enoent" });
121+
vi.doMock("node:child_process", () => ({ execFile }));
122+
123+
const { isPsnrFilterAvailable } = await import("./psnrFilterAvailability.js");
124+
await expect(isPsnrFilterAvailable()).resolves.toBe(false);
125+
});
126+
127+
it("returns false on a non-zero exit from `ffmpeg -filters`", async () => {
128+
const { execFile } = createExecFileSpy({
129+
kind: "exit_nonzero",
130+
code: 1,
131+
stderr: "Unrecognized option '-filters'.",
132+
});
133+
vi.doMock("node:child_process", () => ({ execFile }));
134+
135+
const { isPsnrFilterAvailable } = await import("./psnrFilterAvailability.js");
136+
await expect(isPsnrFilterAvailable()).resolves.toBe(false);
137+
});
138+
139+
it("memoizes the probe across calls and re-probes after resetPsnrFilterAvailabilityCache", async () => {
140+
const { execFile, calls } = createExecFileSpy({
141+
kind: "ok",
142+
stdout: " T.. psnr VV->V Calculate the PSNR",
143+
});
144+
vi.doMock("node:child_process", () => ({ execFile }));
145+
146+
const { isPsnrFilterAvailable, resetPsnrFilterAvailabilityCache } =
147+
await import("./psnrFilterAvailability.js");
148+
149+
await isPsnrFilterAvailable();
150+
await isPsnrFilterAvailable();
151+
await isPsnrFilterAvailable();
152+
expect(calls.length).toBe(1);
153+
154+
resetPsnrFilterAvailabilityCache();
155+
await isPsnrFilterAvailable();
156+
expect(calls.length).toBe(2);
157+
});
158+
159+
it("does not treat a whole-string 'psnr' inside another word as the filter", async () => {
160+
const { execFile } = createExecFileSpy({
161+
kind: "ok",
162+
stdout: [
163+
"Filters:",
164+
" T.. bpsnrx V->V (hypothetical extended filter, not the real psnr)",
165+
].join("\n"),
166+
});
167+
vi.doMock("node:child_process", () => ({ execFile }));
168+
169+
const { isPsnrFilterAvailable } = await import("./psnrFilterAvailability.js");
170+
await expect(isPsnrFilterAvailable()).resolves.toBe(false);
171+
});
172+
});
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
import { execFile } from "node:child_process";
2+
import { promisify } from "node:util";
3+
import { getFfmpegBinary } from "./ffmpegBinaries.js";
4+
5+
/**
6+
* Preflight for the ffmpeg `psnr` filter used by drawElement self-verify
7+
* (see `psnr.ts`). Some host ffmpeg builds ship without `libpostproc` and
8+
* silently omit the filter — every downstream `psnrDb()` call then throws
9+
* mid-render and the disk-sample verifier swallows it (fail-open safety net).
10+
* A cached one-shot probe surfaces the shape once, at bootstrap, so the
11+
* capture-session router can force-fallback to the reliable screenshot path
12+
* instead of arming a drawElement render whose safety net cannot run.
13+
*
14+
* Cache lifetime is the current process: an operator's ffmpeg install does
15+
* not change across renders within the same CLI invocation, and re-probing
16+
* per session would burn ~50-100ms of subprocess spawn per capture worker.
17+
*/
18+
let cached: Promise<boolean> | null = null;
19+
20+
/**
21+
* Returns true when the resident ffmpeg exposes the `psnr` filter. False on
22+
* any probe failure — missing binary (ENOENT), non-zero exit, timeout,
23+
* unparseable output — because in every case the drawElement self-verify
24+
* path cannot function. Never rejects.
25+
*
26+
* Result is memoized per process; call {@link resetPsnrFilterAvailabilityCache}
27+
* from tests that need to re-probe.
28+
*/
29+
export function isPsnrFilterAvailable(): Promise<boolean> {
30+
if (cached === null) cached = probe();
31+
return cached;
32+
}
33+
34+
/** Test-only: drop the memoized probe result. */
35+
export function resetPsnrFilterAvailabilityCache(): void {
36+
cached = null;
37+
}
38+
39+
async function probe(): Promise<boolean> {
40+
// Match `psnr.ts`: promisify lazily so a partial `node:child_process` mock
41+
// (test that omits `execFile`) doesn't crash at module load — it fails at
42+
// call time instead, and the try/catch below converts that to `false`.
43+
const execFileP = promisify(execFile);
44+
try {
45+
const { stdout } = await execFileP(getFfmpegBinary(), ["-hide_banner", "-filters"], {
46+
maxBuffer: 4 * 1024 * 1024,
47+
timeout: 5_000,
48+
});
49+
// ffmpeg's `-filters` output lists one filter per line, e.g.
50+
// " T.. psnr VV->V Calculate the PSNR between two video streams."
51+
// A whole-word match keeps `multi-psnr` (hypothetical) from masquerading
52+
// as the real filter, and dodges the banner text that mentions PSNR in
53+
// prose on some builds.
54+
return /(^|\s)psnr(\s|$)/m.test(stdout);
55+
} catch {
56+
return false;
57+
}
58+
}

0 commit comments

Comments
 (0)