perf(producer): compute regression PSNR in one ffmpeg pass (#2813)

* perf(producer): compute regression PSNR in one ffmpeg pass

* fix(producer): fail loudly when one PSNR input runs out of frames

packages/producer/scripts/test-classification.mjs

@@ -9,6 +9,7 @@ export const PRODUCER_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), ".
 // or local sockets. Keep the list explicit so a filename-only rename does not
 // make Git/fallow re-audit thousands of unchanged test lines as new code.
 const INTEGRATION_TEST_FILES = new Set([
+  "src/regression-harness-psnr.test.ts",
   "src/services/coreRuntimeBrowser.test.ts",
   "src/services/deterministicFonts-systemCapture.test.ts",
   "src/services/distributed/assemble.test.ts",

packages/producer/src/regression-harness-psnr.test.ts

@@ -0,0 +1,272 @@
+// Equivalence tests for the regression harness's PSNR comparison.
+//
+// `psnrPerFrame()` replaced a loop that spawned one ffmpeg per checkpoint with
+// `select='eq(n,N)'`. That filter has no index, so every spawn re-decoded from
+// frame 0 and the comparison phase grew quadratically with checkpoint count.
+// These tests pin the replacement to the behaviour of the method it replaced:
+// the same frame must yield the same PSNR, within the two-decimal precision
+// that ffmpeg's `stats_file` reports.
+//
+// Lives in the integration lane (see scripts/test-classification.mjs) because
+// it shells out to a real ffmpeg.
+
+import { afterAll, beforeAll, describe, expect, it } from "bun:test";
+import { spawnSync } from "node:child_process";
+import { mkdtempSync, rmSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { frameIndexForCheckpoint, psnrAtCheckpoint, psnrAtFrames } from "./regression-harness.js";
+
+const FPS = 30;
+const TOTAL_FRAMES = 60;
+const SAMPLE_FRAMES = [0, 1, 15, 30, 45, 59];
+
+let workDir: string;
+let referenceVideo: string;
+let degradedVideo: string;
+/** Same pixels as `degradedVideo`, but a timeline framesync cannot align. */
+let rebasedVideo: string;
+/** Half the frames of `referenceVideo`, for one-sided end-of-stream cases. */
+let halfLengthVideo: string;
+
+/**
+ * The pre-existing per-checkpoint implementation, kept here as the oracle.
+ * Any divergence between this and `psnrPerFrame()` is a behaviour change in
+ * the regression suite's pass/fail decisions.
+ */
+function legacyPsnrAtFrame(videoA: string, videoB: string, frameIndex: number): number {
+  const filter =
+    `[0:v]select='eq(n\\,${frameIndex})',setpts=PTS-STARTPTS[rv];` +
+    `[1:v]select='eq(n\\,${frameIndex})',setpts=PTS-STARTPTS[gv];[rv][gv]psnr`;
+  const result = spawnSync(
+    "ffmpeg",
+    [
+      "-hide_banner",
+      "-loglevel",
+      "info",
+      "-i",
+      videoA,
+      "-i",
+      videoB,
+      "-filter_complex",
+      filter,
+      "-frames:v",
+      "1",
+      "-f",
+      "null",
+      "-",
+    ],
+    { encoding: "utf-8" },
+  );
+  const match = result.stderr.match(/average:\s*(\S+)/i);
+  if (!match) throw new Error(`legacy PSNR parse failed at frame ${frameIndex}`);
+  const raw = (match[1] ?? "").trim().toLowerCase();
+  return raw === "inf" || raw === "infinite" ? Number.POSITIVE_INFINITY : Number(raw);
+}
+
+function ffmpeg(args: string[]): void {
+  const result = spawnSync("ffmpeg", args, { encoding: "utf-8" });
+  if (result.status !== 0) throw new Error(`ffmpeg fixture setup failed: ${result.stderr}`);
+}
+
+beforeAll(() => {
+  workDir = mkdtempSync(join(tmpdir(), "hf-psnr-test-"));
+  referenceVideo = join(workDir, "reference.mp4");
+  degradedVideo = join(workDir, "degraded.mp4");
+
+  // testsrc2 is deterministic, so these fixtures are reproducible across hosts.
+  ffmpeg([
+    "-hide_banner",
+    "-loglevel",
+    "error",
+    "-f",
+    "lavfi",
+    "-i",
+    `testsrc2=size=320x240:rate=${FPS}:duration=${TOTAL_FRAMES / FPS}`,
+    "-c:v",
+    "libx264",
+    "-crf",
+    "18",
+    "-pix_fmt",
+    "yuv420p",
+    "-y",
+    referenceVideo,
+  ]);
+  // Re-encode hard enough to land well inside the PSNR range fixtures use.
+  ffmpeg([
+    "-hide_banner",
+    "-loglevel",
+    "error",
+    "-i",
+    referenceVideo,
+    "-c:v",
+    "libx264",
+    "-crf",
+    "40",
+    "-pix_fmt",
+    "yuv420p",
+    "-y",
+    degradedVideo,
+  ]);
+
+  // Identical frames to degradedVideo, re-timed to a different rate. Rendered
+  // output does not carry its baseline's timestamps, and comparing the two
+  // streams directly lets framesync pair frames by PTS instead of by index —
+  // the bug that made style-3-prod fail 10 checkpoints. Frame N here must
+  // still compare against frame N of the reference.
+  rebasedVideo = join(workDir, "rebased.mp4");
+  ffmpeg([
+    "-hide_banner",
+    "-loglevel",
+    "error",
+    "-i",
+    degradedVideo,
+    "-vf",
+    "setpts=N/25/TB",
+    "-r",
+    "25",
+    "-c:v",
+    "libx264",
+    "-crf",
+    "40",
+    "-pix_fmt",
+    "yuv420p",
+    "-y",
+    rebasedVideo,
+  ]);
+
+  // Exactly half the frames, same rate. Used for one-sided EOF: requesting an
+  // index this video does not have must fail rather than silently pair against
+  // its repeated last frame.
+  halfLengthVideo = join(workDir, "half.mp4");
+  ffmpeg([
+    "-hide_banner",
+    "-loglevel",
+    "error",
+    "-f",
+    "lavfi",
+    "-i",
+    `testsrc2=size=320x240:rate=${FPS}:duration=${TOTAL_FRAMES / 2 / FPS}`,
+    "-c:v",
+    "libx264",
+    "-crf",
+    "18",
+    "-pix_fmt",
+    "yuv420p",
+    "-y",
+    halfLengthVideo,
+  ]);
+});
+
+afterAll(() => {
+  rmSync(workDir, { recursive: true, force: true });
+});
+
+describe("psnrAtFrames()", () => {
+  it("returns exactly the frames asked for", () => {
+    const byFrame = psnrAtFrames(referenceVideo, degradedVideo, SAMPLE_FRAMES);
+    expect([...byFrame.keys()].sort((a, b) => a - b)).toEqual(SAMPLE_FRAMES);
+  });
+
+  it("matches the per-checkpoint ffmpeg method it replaced", () => {
+    const byFrame = psnrAtFrames(referenceVideo, degradedVideo, SAMPLE_FRAMES);
+    // Sample across the whole range: the old method's cost grew with frame
+    // index, so late frames are exactly where a regression would hide.
+    for (const frameIndex of SAMPLE_FRAMES) {
+      const actual = byFrame.get(frameIndex);
+      const expected = legacyPsnrAtFrame(referenceVideo, degradedVideo, frameIndex);
+      expect(actual).toBeDefined();
+      // stats_file reports two decimals; the legacy stderr parse was full float.
+      expect(Math.abs((actual as number) - expected)).toBeLessThanOrEqual(0.01);
+    }
+  });
+
+  it("pairs by frame index even when the two videos carry different timelines", () => {
+    // The regression this guards: a rendered output whose PTS cadence differs
+    // from the baseline's must still be compared frame-for-frame. Letting
+    // framesync align by timestamp silently compares unrelated frames.
+    const byFrame = psnrAtFrames(referenceVideo, rebasedVideo, SAMPLE_FRAMES);
+    expect([...byFrame.keys()].sort((a, b) => a - b)).toEqual(SAMPLE_FRAMES);
+    for (const frameIndex of SAMPLE_FRAMES) {
+      const expected = legacyPsnrAtFrame(referenceVideo, rebasedVideo, frameIndex);
+      expect(Math.abs((byFrame.get(frameIndex) as number) - expected)).toBeLessThanOrEqual(0.01);
+    }
+  });
+
+  it("deduplicates repeated frame indices without losing alignment", () => {
+    // Fixtures shorter than 100 frames map several checkpoints onto one frame.
+    // `select` emits that frame once, so the row-to-frame mapping has to
+    // account for it or every later value shifts.
+    const withRepeats = [0, 0, 15, 15, 15, 59];
+    const byFrame = psnrAtFrames(referenceVideo, degradedVideo, withRepeats);
+    expect([...byFrame.keys()].sort((a, b) => a - b)).toEqual([0, 15, 59]);
+    for (const frameIndex of [0, 15, 59]) {
+      const expected = legacyPsnrAtFrame(referenceVideo, degradedVideo, frameIndex);
+      expect(Math.abs((byFrame.get(frameIndex) as number) - expected)).toBeLessThanOrEqual(0.01);
+    }
+  });
+
+  it("is insensitive to the order frame indices are supplied in", () => {
+    const ascending = psnrAtFrames(referenceVideo, degradedVideo, [1, 30, 59]);
+    const shuffled = psnrAtFrames(referenceVideo, degradedVideo, [59, 1, 30]);
+    expect([...shuffled.entries()].sort()).toEqual([...ascending.entries()].sort());
+  });
+
+  it("reports identical video as infinite PSNR, as the legacy method did", () => {
+    const byFrame = psnrAtFrames(referenceVideo, referenceVideo, SAMPLE_FRAMES);
+    expect([...byFrame.values()].every((value) => value === Number.POSITIVE_INFINITY)).toBe(true);
+    expect(legacyPsnrAtFrame(referenceVideo, referenceVideo, 10)).toBe(Number.POSITIVE_INFINITY);
+  });
+
+  it("throws rather than mis-pairing when a frame is past the end of both videos", () => {
+    expect(() => psnrAtFrames(referenceVideo, degradedVideo, [0, TOTAL_FRAMES + 50])).toThrow(
+      /differ in frame count|Expected PSNR/,
+    );
+  });
+
+  it("throws when only one input runs out of frames", () => {
+    // The asymmetric case, and the one the test above cannot reach: there both
+    // inputs end together, so framesync ends too. With only one side short,
+    // framesync's repeatlast default holds that side's last selected frame and
+    // pads the pairing to full length — ffmpeg then writes one row per
+    // requested frame and the count guard sees nothing wrong, while the tail
+    // rows silently compare against a stale frame.
+    expect(() => psnrAtFrames(referenceVideo, halfLengthVideo, [0, 15, 45])).toThrow(
+      /differ in frame count|Expected PSNR/,
+    );
+  });
+
+  it("still compares every requested frame when one input is merely shorter than the last index", () => {
+    // Guard against over-correcting: truncation must fail, but a shorter input
+    // that still covers every requested index has to keep working.
+    const byFrame = psnrAtFrames(referenceVideo, halfLengthVideo, [0, 10, 29]);
+    expect([...byFrame.keys()].sort((a, b) => a - b)).toEqual([0, 10, 29]);
+  });
+
+  it("returns an empty map when asked for nothing", () => {
+    expect(psnrAtFrames(referenceVideo, degradedVideo, []).size).toBe(0);
+  });
+});
+
+describe("psnrAtCheckpoint()", () => {
+  it("maps a checkpoint time to the same frame index the legacy code used", () => {
+    const times = [0, 0.5, 1, 1.9];
+    const byFrame = psnrAtFrames(
+      referenceVideo,
+      degradedVideo,
+      times.map((time) => frameIndexForCheckpoint(time, FPS)),
+    );
+    for (const time of times) {
+      // Math.round(time * fps) is the mapping the per-checkpoint code used.
+      expect(frameIndexForCheckpoint(time, FPS)).toBe(Math.round(time * FPS));
+      expect(psnrAtCheckpoint(byFrame, time, FPS)).toBe(
+        byFrame.get(Math.round(time * FPS)) as number,
+      );
+    }
+  });
+
+  it("throws when the requested frame was not among those compared", () => {
+    const byFrame = psnrAtFrames(referenceVideo, degradedVideo, [0, 15]);
+    expect(() => psnrAtCheckpoint(byFrame, 1.5, FPS)).toThrow(/Unable to parse PSNR output/);
+  });
+});

packages/producer/src/regression-harness.ts

@@ -6,6 +6,7 @@ import {
   readFileSync,
   writeFileSync,
   copyFileSync,
+  mkdtempSync,
   rmSync,
   statSync,
   cpSync,
@@ -576,42 +577,135 @@ function extractFrameAsImage(
   );
 }
 
-function psnrAtCheckpoint(
+/** The frame a checkpoint time samples. Shared so selection and lookup agree. */
+export function frameIndexForCheckpoint(checkpointSec: number, fps: number): number {
+  return Math.max(0, Math.round(checkpointSec * fps));
+}
+
+/**
+ * PSNR for a set of frame indices, in a single ffmpeg pass.
+ *
+ * The original implementation spawned one ffmpeg per checkpoint, each
+ * selecting its frame with `select='eq(n,N)'`. That filter has no index, so
+ * ffmpeg decoded from frame 0 every time — checkpoint 99 decoded 99% of both
+ * videos to read a single frame. Across 100 checkpoints that is roughly 50
+ * full decodes of each video, and it dominated regression runtime (27% of
+ * total suite work; 60-80% on short fixtures).
+ *
+ * This keeps the original `select` semantics exactly and only collapses the
+ * spawns: both inputs are filtered to the same frame indices, chosen **by
+ * decode index, independently per input**, then compared pairwise.
+ *
+ * Selecting by index is load-bearing, not incidental. Handing the streams to
+ * `psnr` directly (`[0:v][1:v]psnr`) instead makes ffmpeg's framesync align
+ * them by presentation timestamp, and rendered output does not carry the same
+ * PTS as its golden baseline. That pairs frames which do not correspond: on
+ * style-3-prod it moved 80 of 100 checkpoints by more than 2 dB and turned
+ * three exactly-identical frames into 82/38/51 dB.
+ *
+ * `settb=1/1,setpts=N` after each `select` renumbers both selected streams to
+ * the same synthetic one-tick-per-frame timeline, so framesync pairs the Nth
+ * selected frame of one input with the Nth of the other. The timebase is
+ * pinned rather than derived (`setpts=N/FRAME_RATE/TB` is not enough) because
+ * `FRAME_RATE` is per-input: if the two videos report different rates, that
+ * form hands framesync two different timelines again and it silently emits a
+ * different number of rows than frames requested.
+ *
+ * Returns a 0-based frame index -> PSNR map. `stats_file` reports `psnr_avg`
+ * to two decimals where the old stderr parse had full float precision;
+ * thresholds are integers and fixtures pass with dB of margin, so the 0.005 dB
+ * rounding is not material.
+ */
+export function psnrAtFrames(
   renderedVideo: string,
   snapshotVideo: string,
+  frameIndices: number[],
+): Map<number, number> {
+  // Several checkpoints land on one frame when a fixture has fewer frames than
+  // checkpoints, and `select` emits such a frame once. Deduplicate so the
+  // filter output length is predictable and position-addressable.
+  const wanted = [...new Set(frameIndices)].sort((left, right) => left - right);
+  if (wanted.length === 0) return new Map();
+
+  const statsDir = mkdtempSync(join(tmpdir(), "hf-psnr-"));
+  const statsFile = join(statsDir, "psnr.log");
+  try {
+    // ffmpeg treats `:` and `\` in filter option values as syntax, so a temp
+    // path containing either would break the filtergraph. mkdtemp under
+    // tmpdir() does not produce those on POSIX, but escape defensively.
+    const escaped = statsFile.replace(/\\/g, "\\\\").replace(/:/g, "\\:");
+    const selectExpr = wanted.map((frame) => `eq(n\\,${frame})`).join("+");
+    const stream = (index: number, label: string) =>
+      `[${index}:v]select='${selectExpr}',settb=1/1,setpts=N[${label}]`;
+    runFfmpeg(
+      [
+        "-hide_banner",
+        "-loglevel",
+        "error",
+        "-i",
+        renderedVideo,
+        "-i",
+        snapshotVideo,
+        "-filter_complex",
+        // shortest=1:repeatlast=0 makes framesync stop at the first stream to
+        // end instead of holding its last frame. Without them, an input that
+        // runs out of selected frames has its final frame repeated to pad the
+        // pairing, so ffmpeg still writes one row per requested frame and the
+        // count check below cannot tell that the tail rows compare a stale
+        // frame. Verified: 60-frame vs 30-frame inputs asking for frames
+        // [0,15,45] emit 3 rows under the defaults (row 3 comparing frame 45
+        // against a repeated frame 15, 16.65 dB) and 2 rows with these set.
+        `${stream(0, "rv")};${stream(1, "gv")};` +
+          `[rv][gv]psnr=shortest=1:repeatlast=0:stats_file=${escaped}`,
+        "-f",
+        "null",
+        "-",
+      ],
+      "Checkpoint PSNR",
+    );
+
+    const values: number[] = [];
+    for (const line of readFileSync(statsFile, "utf-8").split("\n")) {
+      const psnrMatch = line.match(/(?:^|\s)psnr_avg:(\S+)/);
+      if (!psnrMatch) continue;
+      const raw = (psnrMatch[1] ?? "").trim().toLowerCase();
+      if (raw === "inf" || raw === "infinite") {
+        values.push(Number.POSITIVE_INFINITY);
+        continue;
+      }
+      const parsed = Number(raw);
+      if (!Number.isFinite(parsed)) {
+        throw new Error(`Invalid PSNR value in ffmpeg stats output: ${psnrMatch[1]}`);
+      }
+      values.push(parsed);
+    }
+
+    // A short count means an input ran out of frames, so every later pairing
+    // would be silently offset. The per-checkpoint implementation also failed
+    // loudly here; keep it that way rather than reporting PSNR for frames that
+    // were never compared.
+    if (values.length !== wanted.length) {
+      throw new Error(
+        `Expected PSNR for ${wanted.length} frames but ffmpeg reported ${values.length}. ` +
+          "The rendered output and baseline likely differ in frame count.",
+      );
+    }
+
+    return new Map(wanted.map((frame, position) => [frame, values[position] as number]));
+  } finally {
+    rmSync(statsDir, { recursive: true, force: true });
+  }
+}
+
+export function psnrAtCheckpoint(
+  psnrByFrame: Map<number, number>,
   checkpointSec: number,
   fps: number,
 ): number {
-  const frameIndex = Math.max(0, Math.round(checkpointSec * fps));
-  const filter = `[0:v]select='eq(n\\,${frameIndex})',setpts=PTS-STARTPTS[rv];[1:v]select='eq(n\\,${frameIndex})',setpts=PTS-STARTPTS[gv];[rv][gv]psnr`;
-  const args = [
-    "-hide_banner",
-    "-loglevel",
-    "info",
-    "-i",
-    renderedVideo,
-    "-i",
-    snapshotVideo,
-    "-filter_complex",
-    filter,
-    "-frames:v",
-    "1",
-    "-f",
-    "null",
-    "-",
-  ];
-  const { stderr } = runFfmpeg(args, `Frame PSNR at ${checkpointSec}s`);
-  const match = stderr.match(/average:\s*([^\s]+)/i);
-  if (!match) {
-    throw new Error(`Unable to parse PSNR output at ${checkpointSec}s`);
-  }
-  const rawValue = (match[1] ?? "").trim().toLowerCase();
-  if (rawValue === "inf" || rawValue === "infinite") {
-    return Number.POSITIVE_INFINITY;
-  }
-  const parsedValue = Number(rawValue);
-  if (!Number.isFinite(parsedValue)) {
-    throw new Error(`Invalid PSNR value at ${checkpointSec}s: ${match[1]}`);
+  const frameIndex = frameIndexForCheckpoint(checkpointSec, fps);
+  const parsedValue = psnrByFrame.get(frameIndex);
+  if (parsedValue === undefined) {
+    throw new Error(`Unable to parse PSNR output at ${checkpointSec}s (frame ${frameIndex})`);
   }
   return parsedValue;
 }
@@ -1211,9 +1305,15 @@ async function runTestSuite(
       const sampleDuration = Math.max(0, videoDuration - 1 / fps);
 
       const minPsnrForMode = resolveMinPsnrForMode(options.mode, suite.meta.minPsnr);
+      const checkpointTimes = Array.from({ length: 100 }, (_, i) => (sampleDuration * i) / 100);
+      const psnrByFrame = psnrAtFrames(
+        renderedOutputPath,
+        snapshotVideoPath,
+        checkpointTimes.map((time) => frameIndexForCheckpoint(time, fps)),
+      );
       for (let i = 0; i < 100; i++) {
-        const time = (sampleDuration * i) / 100;
-        const psnr = psnrAtCheckpoint(renderedOutputPath, snapshotVideoPath, time, fps);
+        const time = checkpointTimes[i] as number;
+        const psnr = psnrAtCheckpoint(psnrByFrame, time, fps);
         visualCheckpoints.push({
           time,
           psnr,