From 346777e51ee13ee9ba27c2426dba1fbdd0bb1be3 Mon Sep 17 00:00:00 2001 From: Santhi Prakash Date: Mon, 14 Sep 2026 18:09:55 +0000 Subject: [PATCH] fix(engine): catch low-contrast drawElement self-verify misses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Whole-frame magnitude-weighted PSNR cannot see a fully-missing low-contrast element: the issue-3345 repro (a ~4/255 band over ~15% of the frame) scores 44dB, 12dB clear of the 32dB floor, and ships a wrong MP4 with exit code 0. Add regionShift(): one ffmpeg pass (grainextract + area downscale) reporting the worst ~96px region's mean signed channel shift in 1/255 units. Encoder noise cancels inside a region while an absent element leaves its whole footprint shifted by its own delta, so a missing region can no longer be diluted by a correct background. Both the streaming drain guard and the parallel disk-path verify now require PSNR >= HF_DE_VERIFY_MIN_DB AND region shift <= HF_DE_VERIFY_MAX_SHIFT (default 3/255). Either breach throws DrawElementVerificationError (new kind "shift"), which the orchestrator already converts into the never-wrong screenshot fallback — matching the videoFrameCoverage fail-closed precedent. Closes #3345 --- packages/engine/src/index.ts | 7 +- .../engine/src/services/frameCapture.test.ts | 14 ++ packages/engine/src/services/frameCapture.ts | 21 +- .../src/services/parallelCoordinator.test.ts | 34 +++ .../src/services/parallelCoordinator.ts | 76 ++++-- packages/engine/src/utils/psnr.test.ts | 225 ++++++++++++++++++ packages/engine/src/utils/psnr.ts | 80 ++++++- .../src/services/render/observability.ts | 21 +- .../stages/captureStreamingStage.test.ts | 68 ++++++ .../render/stages/captureStreamingStage.ts | 36 ++- .../src/services/renderOrchestrator.ts | 4 +- 11 files changed, 539 insertions(+), 47 deletions(-) create mode 100644 packages/engine/src/utils/psnr.test.ts diff --git a/packages/engine/src/index.ts b/packages/engine/src/index.ts index a592d6487b..3e25dd0434 100644 --- a/packages/engine/src/index.ts +++ b/packages/engine/src/index.ts @@ -342,7 +342,12 @@ export { trackChildProcess, killTrackedProcesses } from "./utils/processTracker. // drawElement self-verify comparison — shared by the streaming drain // (producer) and the parallel disk-path verify (parallelCoordinator). -export { psnrDb, resolveDeVerifyMinDb } from "./utils/psnr.js"; +export { + psnrDb, + regionShift, + resolveDeVerifyMaxShift, + resolveDeVerifyMinDb, +} from "./utils/psnr.js"; export { decodePng, diff --git a/packages/engine/src/services/frameCapture.test.ts b/packages/engine/src/services/frameCapture.test.ts index 05cd4fb52c..6bd29b4509 100644 --- a/packages/engine/src/services/frameCapture.test.ts +++ b/packages/engine/src/services/frameCapture.test.ts @@ -356,4 +356,18 @@ describe("DrawElementVerificationError details", () => { ); expect(getDrawElementVerificationDetails(adversarial)?.kind).toBe("psnr"); }); + + it("carries kind='shift' with its own shift fields, not the dB pair", () => { + const err = new DrawElementVerificationError( + "drawElement self-verify failed at frame 9: region shift 13/255 > 3/255", + { kind: "shift", frameIndex: 9, failedShift: 13, verifyMaxShift: 3 }, + ); + expect(isDrawElementVerificationError(err)).toBe(true); + expect(getDrawElementVerificationDetails(err)).toEqual({ + kind: "shift", + frameIndex: 9, + failedShift: 13, + verifyMaxShift: 3, + }); + }); }); diff --git a/packages/engine/src/services/frameCapture.ts b/packages/engine/src/services/frameCapture.ts index eff6b50331..db23672317 100644 --- a/packages/engine/src/services/frameCapture.ts +++ b/packages/engine/src/services/frameCapture.ts @@ -277,20 +277,27 @@ export interface CaptureSession { * exists to close — review finding: message wording, translation, or a * cross-module/serialized error must never be able to flip the reported * kind). All fields but `kind` are optional: a blank-frame trip has no PSNR - * score, so `failedDb`/`verifyThresholdDb` are omitted for that throw site. + * score, so `failedDb`/`verifyThresholdDb` are omitted for that throw site; + * a region-shift trip carries `failedShift`/`verifyMaxShift` instead. */ export interface DrawElementVerificationDetails { - kind: "blank" | "psnr"; + kind: "blank" | "psnr" | "shift"; frameIndex?: number; failedDb?: number; verifyThresholdDb?: number; + /** Worst-region mean signed shift observed (1/255 units), when kind="shift". */ + failedShift?: number; + /** The HF_DE_VERIFY_MAX_SHIFT ceiling that shift breached, when kind="shift". */ + verifyMaxShift?: number; } export class DrawElementVerificationError extends Error { - readonly kind: "blank" | "psnr"; + readonly kind: "blank" | "psnr" | "shift"; readonly frameIndex?: number; readonly failedDb?: number; readonly verifyThresholdDb?: number; + readonly failedShift?: number; + readonly verifyMaxShift?: number; constructor(message: string, details: DrawElementVerificationDetails) { super(message); @@ -303,6 +310,8 @@ export class DrawElementVerificationError extends Error { this.frameIndex = details.frameIndex; this.failedDb = details.failedDb; this.verifyThresholdDb = details.verifyThresholdDb; + this.failedShift = details.failedShift; + this.verifyMaxShift = details.verifyMaxShift; } } @@ -331,16 +340,18 @@ export function getDrawElementVerificationDetails( if (rec.deVerificationFailure === true) { // Every construction path sets `kind` (required on the constructor), so // this only defends against a malformed cross-module-instance shape — - // treat anything other than exactly "blank" as "psnr", the same + // treat anything other than exactly "blank"/"shift" as "psnr", the same // fallback polarity the old message regex had, but driven by a // structural field instead of parsing text. const details: DrawElementVerificationDetails = { - kind: rec.kind === "blank" ? "blank" : "psnr", + kind: rec.kind === "blank" ? "blank" : rec.kind === "shift" ? "shift" : "psnr", }; if (typeof rec.frameIndex === "number") details.frameIndex = rec.frameIndex; if (typeof rec.failedDb === "number") details.failedDb = rec.failedDb; if (typeof rec.verifyThresholdDb === "number") details.verifyThresholdDb = rec.verifyThresholdDb; + if (typeof rec.failedShift === "number") details.failedShift = rec.failedShift; + if (typeof rec.verifyMaxShift === "number") details.verifyMaxShift = rec.verifyMaxShift; return details; } e = (e as { cause?: unknown }).cause; diff --git a/packages/engine/src/services/parallelCoordinator.test.ts b/packages/engine/src/services/parallelCoordinator.test.ts index 9cc2c832f9..7b18c27006 100644 --- a/packages/engine/src/services/parallelCoordinator.test.ts +++ b/packages/engine/src/services/parallelCoordinator.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect, vi } from "vitest"; import { + assertDiskSampleVerified, calculateOptimalWorkers, computeWorkerSizing, distributeFrames, @@ -16,6 +17,7 @@ import { resolveParallelDeVerifySamples, type WorkerResult, } from "./parallelCoordinator.js"; +import { getDrawElementVerificationDetails } from "./frameCapture.js"; import type { EngineConfig } from "../config.js"; describe("parallel worker phase deadline", () => { @@ -512,3 +514,35 @@ describe("isFfmpegInfrastructureFailure", () => { expect(isFfmpegInfrastructureFailure(42)).toBe(false); }); }); + +describe("assertDiskSampleVerified", () => { + it("throws kind='shift' when a low-contrast region is missing but PSNR passes", () => { + // The #3345 failure shape on the disk path: whole-frame PSNR clears the + // floor while a region is fully absent. + let caught: unknown; + try { + assertDiskSampleVerified({ db: 44, shift: 13 }, 32, 3, 7, 1); + } catch (err) { + caught = err; + } + expect(caught).toBeInstanceOf(Error); + expect((caught as Error).message).toContain("drawElement self-verify failed at frame 7"); + expect((caught as Error).message).toContain("region shift 13/255 > 3/255"); + expect(getDrawElementVerificationDetails(caught)).toEqual({ + kind: "shift", + frameIndex: 7, + failedShift: 13, + verifyMaxShift: 3, + }); + }); + + it("still throws kind='psnr' below the dB floor even with zero shift", () => { + expect(() => assertDiskSampleVerified({ db: 20, shift: 0 }, 32, 3, 7, 1)).toThrow( + /20\.0dB < 32dB/, + ); + }); + + it("passes a sample that clears both gates", () => { + expect(() => assertDiskSampleVerified({ db: 48, shift: 2 }, 32, 3, 7, 1)).not.toThrow(); + }); +}); diff --git a/packages/engine/src/services/parallelCoordinator.ts b/packages/engine/src/services/parallelCoordinator.ts index 45098d2eab..4234bceef8 100644 --- a/packages/engine/src/services/parallelCoordinator.ts +++ b/packages/engine/src/services/parallelCoordinator.ts @@ -25,7 +25,12 @@ import { type CapturePerfSummary, type BeforeCaptureHook, } from "./frameCapture.js"; -import { psnrDb, resolveDeVerifyMinDb } from "../utils/psnr.js"; +import { + psnrDb, + regionShift, + resolveDeVerifyMaxShift, + resolveDeVerifyMinDb, +} from "../utils/psnr.js"; import { DEFAULT_CONFIG, type EngineConfig } from "../config.js"; import { assertSwiftShader } from "../utils/assertSwiftShader.js"; import { readWebGlVendorInfoFromCanvas } from "../utils/readWebGlVendorInfoFromCanvas.js"; @@ -709,27 +714,51 @@ function logParDebug(message: () => string): void { } /** - * Throw the verification error for a sample below the PSNR floor; log the - * pass otherwise. Split from the sampling loop for the complexity gate. + * Throw the verification error for a sample that breaches either gate — below + * the PSNR floor (magnitude damage) or above the region-shift ceiling + * (low-contrast content missing, issue #3345); log the pass otherwise. Split + * from the sampling loop for the complexity gate. + * + * Exported for testing; the gate decision is a pure metrics read. */ -function assertDiskSampleAboveFloor( - db: number, +export function assertDiskSampleVerified( + metrics: { db: number; shift: number }, verifyMinDb: number, + verifyMaxShift: number, idx: number, workerId: number, ): void { - if (db < verifyMinDb) { + if (metrics.db < verifyMinDb) { // Message keeps the contiguous "drawElement self-verify" phrase — // captureFailure's VERIFICATION_ERROR_PATTERNS classifies on it. throw new DrawElementVerificationError( `drawElement self-verify failed at frame ${idx} (disk path, worker ${workerId}): ` + - `${db.toFixed(1)}dB < ${verifyMinDb}dB vs pre-injection screenshot`, - { kind: "psnr", frameIndex: idx, failedDb: db, verifyThresholdDb: verifyMinDb }, + `${metrics.db.toFixed(1)}dB < ${verifyMinDb}dB vs pre-injection screenshot`, + { + kind: "psnr", + frameIndex: idx, + failedDb: metrics.db, + verifyThresholdDb: verifyMinDb, + }, + ); + } + if (metrics.shift > verifyMaxShift) { + throw new DrawElementVerificationError( + `drawElement self-verify failed at frame ${idx} (disk path, worker ${workerId}): ` + + `region shift ${metrics.shift}/255 > ${verifyMaxShift}/255 vs pre-injection screenshot — ` + + "a low-contrast region is absent from the drawElement frame", + { + kind: "shift", + frameIndex: idx, + failedShift: metrics.shift, + verifyMaxShift, + }, ); } console.log( `[Parallel] drawElement disk self-verify passed (worker ${workerId}, frame ${idx}, ` + - `${db === Infinity ? "inf" : db.toFixed(1)}dB)`, + `${metrics.db === Infinity ? "inf" : metrics.db.toFixed(1)}dB, ` + + `shift ${metrics.shift}/255)`, ); } @@ -757,22 +786,24 @@ export function isFfmpegInfrastructureFailure(err: unknown): boolean { /** * Compare one captured frame file against its ground truth. Returns the - * 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 + * PSNR + worst-region shift, 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 a missing 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( +async function metricsForDiskSample( framePath: string, truth: Buffer, workerId: number, idx: number, -): Promise { +): Promise<{ db: number; shift: number } | null> { try { - return await psnrDb(await readFile(framePath), truth); + const buf = await readFile(framePath); + const [db, shift] = await Promise.all([psnrDb(buf, truth), regionShift(buf, truth)]); + return { db, shift }; } catch (err) { if (isFfmpegInfrastructureFailure(err)) { const detail = err instanceof Error ? err.message : String(err); @@ -794,8 +825,8 @@ async function psnrForDiskSample( } // Branches are the gate conditions themselves (mode/armed/streaming guards + -// per-sample skip/breach) — already decomposed into psnrForDiskSample + -// assertDiskSampleAboveFloor; further splitting obscures the check. +// per-sample skip/breach) — already decomposed into metricsForDiskSample + +// assertDiskSampleVerified; further splitting obscures the check. // fallow-ignore-next-line complexity export async function verifyDiskDrawElementSamples( session: CaptureSession, @@ -807,15 +838,16 @@ export async function verifyDiskDrawElementSamples( const truths = session.deVerifyFrames; if (!truths || truths.size === 0) return; const verifyMinDb = resolveDeVerifyMinDb(); + const verifyMaxShift = resolveDeVerifyMaxShift(); const ext = session.options.format === "png" ? "png" : "jpg"; const offset = task.outputFrameOffset ?? 0; for (const idx of selectVerifySampleIndicesForTask(truths.keys(), task)) { const truth = truths.get(idx); if (!truth) continue; const framePath = join(task.outputDir, `frame_${String(idx - offset).padStart(6, "0")}.${ext}`); - const db = await psnrForDiskSample(framePath, truth, task.workerId, idx); - if (db === null) continue; - assertDiskSampleAboveFloor(db, verifyMinDb, idx, task.workerId); + const metrics = await metricsForDiskSample(framePath, truth, task.workerId, idx); + if (metrics === null) continue; + assertDiskSampleVerified(metrics, verifyMinDb, verifyMaxShift, idx, task.workerId); } } diff --git a/packages/engine/src/utils/psnr.test.ts b/packages/engine/src/utils/psnr.test.ts new file mode 100644 index 0000000000..b84a063c6e --- /dev/null +++ b/packages/engine/src/utils/psnr.test.ts @@ -0,0 +1,225 @@ +// fallow-ignore-file code-duplication +import { execFileSync, spawnSync } from "node:child_process"; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { promisify } from "node:util"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { getFfmpegBinary } from "./ffmpegBinaries.js"; + +/** Same probe the ffmpeg-dependent suites use: ask the binary, don't assume it. */ +const HAS_FFMPEG = spawnSync(getFfmpegBinary(), ["-version"], { encoding: "utf-8" }).status === 0; + +interface ExecFileCall { + file: string; + args: readonly string[]; +} + +/** + * 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 — the + * exact hazard psnr.ts documents. Stamp the custom impl on the mock so + * promisify keeps the `{stdout, stderr}` shape. + */ +function createExecFileSpy(handler: (args: readonly string[]) => void): { + 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 }); + handler(args); + return { stdout: "", 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 }; +} + +const envBackup: Record = {}; + +function setEnv(key: string, value: string | undefined): void { + if (!(key in envBackup)) envBackup[key] = process.env[key]; + if (value === undefined) delete process.env[key]; + else process.env[key] = value; +} + +beforeEach(() => { + vi.resetModules(); +}); + +afterEach(() => { + vi.doUnmock("node:child_process"); + for (const [key, value] of Object.entries(envBackup)) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + delete envBackup[key]; + } +}); + +describe("resolveDeVerifyMinDb", () => { + it("defaults to 32 when unset and honors an in-range override", async () => { + const { resolveDeVerifyMinDb } = await import("./psnr.js"); + setEnv("HF_DE_VERIFY_MIN_DB", undefined); + expect(resolveDeVerifyMinDb()).toBe(32); + setEnv("HF_DE_VERIFY_MIN_DB", "45"); + expect(resolveDeVerifyMinDb()).toBe(45); + }); + + it("falls back to 32 for out-of-range or malformed values", async () => { + const { resolveDeVerifyMinDb } = await import("./psnr.js"); + for (const bad of ["5", "61", "banana", ""]) { + setEnv("HF_DE_VERIFY_MIN_DB", bad); + expect(resolveDeVerifyMinDb()).toBe(32); + } + }); +}); + +describe("resolveDeVerifyMaxShift", () => { + it("defaults to 3 when unset and honors an in-range override", async () => { + const { resolveDeVerifyMaxShift } = await import("./psnr.js"); + setEnv("HF_DE_VERIFY_MAX_SHIFT", undefined); + expect(resolveDeVerifyMaxShift()).toBe(3); + setEnv("HF_DE_VERIFY_MAX_SHIFT", "8"); + expect(resolveDeVerifyMaxShift()).toBe(8); + }); + + it("falls back to 3 for out-of-range or malformed values", async () => { + const { resolveDeVerifyMaxShift } = await import("./psnr.js"); + for (const bad of ["0", "65", "banana", ""]) { + setEnv("HF_DE_VERIFY_MAX_SHIFT", bad); + expect(resolveDeVerifyMaxShift()).toBe(3); + } + }); +}); + +describe("regionShift", () => { + it("returns the max |cell-128| of the downscaled signed-diff frame", async () => { + // The mock writes the canned rawvideo cell payload to the output path + // (last argv), so the real readFile + scan path runs end to end. + const cells = Buffer.from([128, 128, 128, 137, 128, 128, 90, 128, 128]); + const { execFile, calls } = createExecFileSpy((args) => { + const outPath = args[args.length - 1]; + if (typeof outPath !== "string") throw new Error("no outpath in argv"); + writeFileSync(outPath, cells); + }); + vi.doMock("node:child_process", () => ({ execFile })); + + const { regionShift } = await import("./psnr.js"); + await expect(regionShift(Buffer.from("a"), Buffer.from("b"))).resolves.toBe(38); + const argv = (calls[0]?.args ?? []).join(" "); + expect(argv).toContain("grainextract"); + expect(argv).toContain("flags=area"); + expect(argv).toContain("rawvideo"); + }); + + it("throws on empty rawvideo output", async () => { + const { execFile } = createExecFileSpy((args) => { + const outPath = args[args.length - 1]; + if (typeof outPath !== "string") throw new Error("no outpath in argv"); + writeFileSync(outPath, Buffer.alloc(0)); + }); + vi.doMock("node:child_process", () => ({ execFile })); + + const { regionShift } = await import("./psnr.js"); + await expect(regionShift(Buffer.from("a"), Buffer.from("b"))).rejects.toThrow(/empty rawvideo/); + }); +}); + +describe.skipIf(!HAS_FFMPEG)("regionShift + psnrDb against real ffmpeg (issue #3345 repro)", () => { + let dir: string; + let truth: Buffer; + let lowContrastMissing: Buffer; + let highContrastMissing: Buffer; + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), "hf-psnr-test-")); + const ff = getFfmpegBinary(); + const mk = (out: string, drawbox?: string) => { + const src = "color=c=0x12122E:s=1920x1080:d=1"; + const args = drawbox + ? ["-v", "error", "-y", "-f", "lavfi", "-i", src, "-vf", drawbox, "-frames:v", "1", out] + : ["-v", "error", "-y", "-f", "lavfi", "-i", src, "-frames:v", "1", out]; + execFileSync(ff, args); + }; + // The reporter's synthetic pair, at 1080p: a ~4/255-luma band over ~15% + // of the frame — invisible to a whole-frame magnitude average. + const truthPath = join(dir, "truth.jpg"); + const lowPath = join(dir, "low.jpg"); + const highPath = join(dir, "high.jpg"); + mk(truthPath); + mk(lowPath, "drawbox=x=0:y=450:w=1920:h=160:color=0x161632:t=fill"); + mk(highPath, "drawbox=x=0:y=450:w=1920:h=160:color=0xFFFFFF:t=fill"); + truth = readFileSync(truthPath); + lowContrastMissing = readFileSync(lowPath); + highContrastMissing = readFileSync(highPath); + }); + + afterEach(() => { + rmSync(dir, { recursive: true, force: true }); + }); + + it("catches the missing low-contrast element that the 32dB PSNR gate passes", async () => { + const { psnrDb, regionShift } = await import("./psnr.js"); + // Sabotage assertion: the pre-fix gate genuinely passes this damage. + const db = await psnrDb(lowContrastMissing, truth); + expect(db).toBeGreaterThan(32); + // The new presence check reads the region's full ~4/255 delta. + const shift = await regionShift(lowContrastMissing, truth); + expect(shift).toBeGreaterThanOrEqual(3); + }); + + it("still reads saturated damage far above the ceiling", async () => { + const { regionShift } = await import("./psnr.js"); + expect(await regionShift(highContrastMissing, truth)).toBeGreaterThan(64); + }); + + it("reports ~0 on an identical pair and stays under the ceiling on pure encoder noise", async () => { + const { regionShift } = await import("./psnr.js"); + expect(await regionShift(truth, truth)).toBe(0); + // Re-encode the truth at a different jpeg quality: the difference is pure + // encoder noise with no missing content — must not trip the gate. + const noisyPath = join(dir, "noisy.jpg"); + execFileSync(getFfmpegBinary(), [ + "-v", + "error", + "-y", + "-i", + join(dir, "truth.jpg"), + "-q:v", + "12", + noisyPath, + ]); + expect(await regionShift(readFileSync(noisyPath), truth)).toBeLessThanOrEqual(2); + }); +}); diff --git a/packages/engine/src/utils/psnr.ts b/packages/engine/src/utils/psnr.ts index 675594ec47..88b67ac6b3 100644 --- a/packages/engine/src/utils/psnr.ts +++ b/packages/engine/src/utils/psnr.ts @@ -1,5 +1,5 @@ import { execFile } from "node:child_process"; -import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { promisify } from "node:util"; @@ -43,3 +43,81 @@ export function resolveDeVerifyMinDb(): number { const raw = Number(process.env.HF_DE_VERIFY_MIN_DB ?? "32"); return Number.isFinite(raw) && raw >= 10 && raw <= 60 ? raw : 32; } + +/** + * Worst-region mean signed channel shift between two same-dimension encoded + * images, in 1/255 units (0 = no regional difference; 127 = saturated). + * + * `psnrDb` answers "how far off are the pixels on average" — a magnitude + * average a correct background dilutes arbitrarily, so a fully-missing + * low-contrast element can score 44dB+ and ship with a clean exit code. The + * self-verify gate's real question is "did any content go missing", so this + * instead averages the SIGNED per-pixel difference inside ~96px regions and + * reports the worst one: random encoder noise cancels inside a region while + * an absent element leaves its whole footprint shifted by its own delta — + * a wrong region can no longer be averaged away by a correct background. + * Measured on the issue-3345 repro: the weakest reported miss (a 4/255 luma + * band over ~15% of a 4K frame, PSNR 44dB) reads ~5/255 here, and even an + * adversarial independent-noise pair stays ≤2/255. + * + * One ffmpeg pass: `grainextract` centers the signed A−B on mid-gray (128), + * the area-averaged downscale makes each region one cell whose distance from + * 128 IS its mean shift, and the max |cell−128| over the rgb24 output is the + * return value. ~96px regions are large enough to cancel encoder noise and + * small enough that an element worth gating dominates at least one cell. + */ +export async function regionShift(a: Buffer, b: Buffer): Promise { + // Lazy promisify, same reason as psnrDb above. + const execFileP = promisify(execFile); + const dir = await mkdtemp(join(tmpdir(), "hf-de-verify-")); + try { + const pa = join(dir, "a.jpg"); + const pb = join(dir, "b.jpg"); + const outPath = join(dir, "shift.raw"); + await Promise.all([writeFile(pa, a), writeFile(pb, b)]); + await execFileP( + getFfmpegBinary(), + [ + "-hide_banner", + "-i", + pa, + "-i", + pb, + "-lavfi", + "[0:v]format=rgb24[ga];[1:v]format=rgb24[gb];" + + "[ga][gb]blend=all_mode=grainextract," + + "scale='ceil(iw/96)':'ceil(ih/96)':flags=area", + "-f", + "rawvideo", + "-pix_fmt", + "rgb24", + "-y", + outPath, + ], + { maxBuffer: 4 * 1024 * 1024 }, + ); + const raw = await readFile(outPath); + if (raw.length === 0) { + throw new Error("regionShift parse failed: empty rawvideo output"); + } + let max = 0; + for (const byte of raw) { + const shift = Math.abs(byte - 128); + if (shift > max) max = shift; + } + return max; + } finally { + await rm(dir, { recursive: true, force: true }).catch(() => {}); + } +} + +/** + * The drawElement self-verify region-shift ceiling (1/255 channel units). + * HF_DE_VERIFY_MAX_SHIFT overrides, clamped to [1, 64]; out-of-range or unset + * falls back to 3. Below ~1 the gate sits inside measured encoder noise; the + * ceiling keeps an override able to weaken (not silently remove) the check. + */ +export function resolveDeVerifyMaxShift(): number { + const raw = Number(process.env.HF_DE_VERIFY_MAX_SHIFT ?? "3"); + return Number.isFinite(raw) && raw >= 1 && raw <= 64 ? raw : 3; +} diff --git a/packages/producer/src/services/render/observability.ts b/packages/producer/src/services/render/observability.ts index 0b77548363..aa2835e33d 100644 --- a/packages/producer/src/services/render/observability.ts +++ b/packages/producer/src/services/render/observability.ts @@ -41,18 +41,19 @@ export interface RenderCaptureObservability { hasHdrContent?: boolean; browserGpuMode?: string; /** - * drawElement per-render SELF-VERIFICATION tripped (blank/PSNR) → whole - * render re-ran via screenshot. NARROWED semantics since the pinned-fallback - * retry was widened (review): OOM- and generic-capture-error-triggered - * fallbacks report FALSE here, with `deFallbackReason` ∈ {oom, - * capture_error}. The "any fallback fired" signal is `deFallbackReason` - * being set, NOT this flag — dashboards keyed on `de_self_verify_fallback = - * true` as any-fallback must migrate to `de_fallback_reason IS NOT NULL`. + * drawElement per-render SELF-VERIFICATION tripped (blank/PSNR/region-shift) + * → whole render re-ran via screenshot. NARROWED semantics since the + * pinned-fallback retry was widened (review): OOM- and + * generic-capture-error-triggered fallbacks report FALSE here, with + * `deFallbackReason` ∈ {oom, capture_error}. The "any fallback fired" + * signal is `deFallbackReason` being set, NOT this flag — dashboards keyed + * on `de_self_verify_fallback = true` as any-fallback must migrate to + * `de_fallback_reason IS NOT NULL`. */ deSelfVerifyFallback?: boolean; /** * Why the capture-stage retry (self-verify OR the pinned-worker-count - * fallback) fired: "blank"/"psnr" for a real self-verify trip, + * fallback) fired: "blank"/"psnr"/"shift" for a real self-verify trip, * "oom"/"capture_error" for the widened generic-failure retry. Set * whenever a fallback is attempted, independent of whether that retry * itself later succeeds — so a render that fails AFTER a fallback attempt @@ -60,9 +61,9 @@ export interface RenderCaptureObservability { * telemetry from one that never attempted any fallback. */ deFallbackReason?: string; - /** The failing PSNR (dB) when `deFallbackReason === "psnr"`; undefined for blank/oom/capture_error (no score exists). */ + /** The failing PSNR (dB) when `deFallbackReason === "psnr"`; undefined for shift/blank/oom/capture_error (no dB score exists). */ deFallbackFailedDb?: number; - /** Frame index the verification failure was detected at; set for both "psnr" and "blank" fallback reasons. */ + /** Frame index the verification failure was detected at; set for "psnr", "shift", and "blank" fallback reasons. */ deFallbackFrameIndex?: number; /** The HF_DE_VERIFY_MIN_DB threshold the failing dB breached; only set alongside deFallbackFailedDb (psnr reason). */ deFallbackThresholdDb?: number; diff --git a/packages/producer/src/services/render/stages/captureStreamingStage.test.ts b/packages/producer/src/services/render/stages/captureStreamingStage.test.ts index 66dca5fbc3..a64ae05970 100644 --- a/packages/producer/src/services/render/stages/captureStreamingStage.test.ts +++ b/packages/producer/src/services/render/stages/captureStreamingStage.test.ts @@ -30,6 +30,9 @@ let hangParallelUntilAbort = false; let hangSequentialUntilStall = false; let sessionWorkerEncodeEnabled = false; let captureSessionMode: "drawelement" | "screenshot" = "drawelement"; +let deVerifyFramesMap: Map | undefined; +let psnrDbValue = Infinity; +let regionShiftValue = 0; let failPrepareCaptureSessionForReuse = false; let initializeSessionErrorMessage = "initialize failed"; const browserConsoleBuffer = ["[FrameCapture:ERROR] page.goto failed"]; @@ -69,6 +72,7 @@ mock.module("@hyperframes/engine", () => ({ options: { captureBeyondViewport: false }, workerEncodeEnabled: sessionWorkerEncodeEnabled, captureMode: captureSessionMode, + deVerifyFrames: deVerifyFramesMap, }), createFrameReorderBuffer: () => ({ waitForFrame: async () => {}, @@ -135,6 +139,8 @@ mock.module("@hyperframes/engine", () => ({ throw new Error("prepare reuse failed: ENOSPC"); } }, + psnrDb: async () => psnrDbValue, + regionShift: async () => regionShiftValue, recaptureDrawElementFrameForVerify: async () => Buffer.from("frame"), spawnStreamingEncoder, writeCapturedFrame: async () => {}, @@ -481,6 +487,68 @@ describe("runCaptureStreamingStage", () => { expect((caught as Error).message).not.toContain("stalled"); expect((caught as Error).message).toContain("aborted"); }); + + it("rejects a verified frame when the region-shift gate trips while PSNR passes", async () => { + // The #3345 failure shape: a low-contrast region is absent from the + // drawElement frame but the whole-frame PSNR still clears the 32dB floor. + sessionWorkerEncodeEnabled = true; + deVerifyFramesMap = new Map([[0, Buffer.from("truth")]]); + psnrDbValue = 44; + regionShiftValue = 13; + const prevBatch = process.env.HF_DE_BATCH; + process.env.HF_DE_BATCH = "1"; + const { runCaptureStreamingStage } = await import("./captureStreamingStage.js"); + const cfg = { forceScreenshot: false, ffmpegStreamingTimeout: 3_600_000 }; + const input = { ...createInput(cfg), totalFrames: 1, workerCount: 1 }; + + let caught: unknown; + try { + await runCaptureStreamingStage(input); + } catch (error) { + caught = error; + } finally { + sessionWorkerEncodeEnabled = false; + deVerifyFramesMap = undefined; + psnrDbValue = Infinity; + regionShiftValue = 0; + if (prevBatch === undefined) delete process.env.HF_DE_BATCH; + else process.env.HF_DE_BATCH = prevBatch; + } + + expect(caught).toBeInstanceOf(Error); + expect((caught as Error).message).toContain("drawElement self-verify failed at frame 0"); + expect((caught as Error).message).toContain("region shift 13/255 > 3/255"); + }); + + it("accepts a verified frame when both gates clear", async () => { + sessionWorkerEncodeEnabled = true; + deVerifyFramesMap = new Map([[0, Buffer.from("truth")]]); + psnrDbValue = 48; + regionShiftValue = 1; + const prevBatch = process.env.HF_DE_BATCH; + process.env.HF_DE_BATCH = "1"; + const { runCaptureStreamingStage } = await import("./captureStreamingStage.js"); + const cfg = { forceScreenshot: false, ffmpegStreamingTimeout: 3_600_000 }; + const input = { ...createInput(cfg), totalFrames: 1, workerCount: 1 }; + + let result: { success?: boolean } | undefined; + let caught: unknown; + try { + result = await runCaptureStreamingStage(input); + } catch (error) { + caught = error; + } finally { + sessionWorkerEncodeEnabled = false; + deVerifyFramesMap = undefined; + psnrDbValue = Infinity; + regionShiftValue = 0; + if (prevBatch === undefined) delete process.env.HF_DE_BATCH; + else process.env.HF_DE_BATCH = prevBatch; + } + + expect(caught).toBeUndefined(); + expect(result?.success).toBe(true); + }); }); describe("runCaptureStage", () => { diff --git a/packages/producer/src/services/render/stages/captureStreamingStage.ts b/packages/producer/src/services/render/stages/captureStreamingStage.ts index 0dc8e76f54..ad545e604a 100644 --- a/packages/producer/src/services/render/stages/captureStreamingStage.ts +++ b/packages/producer/src/services/render/stages/captureStreamingStage.ts @@ -63,6 +63,8 @@ import { executeParallelCapture, getCapturePerfSummary, psnrDb, + regionShift, + resolveDeVerifyMaxShift, resolveDeVerifyMinDb, recaptureDrawElementFrameForVerify, completeDeferredDrawElementInit, @@ -286,8 +288,15 @@ function createDrainFrameGuard(args: { // no stable median yet. // 2. Self-verification: at K sampled indices, compare the DE frame against // its pre-injection screenshot ground truth (session.deVerifyFrames). - // PSNR below HF_DE_VERIFY_MIN_DB (default 32; natural DE-vs-screenshot - // agreement measures ≥45, damage <30) → verification error. + // Two checks, both fail-closed into DrawElementVerificationError: + // a. PSNR below HF_DE_VERIFY_MIN_DB (default 32; natural DE-vs-screenshot + // agreement measures ≥45, damage <30) — magnitude-weighted over the + // whole frame, so it cannot see a fully-missing LOW-CONTRAST element + // (issue #3345: a 4/255 band over 15% of a 4K frame scores 44dB). + // b. Region shift above HF_DE_VERIFY_MAX_SHIFT (default 3, 1/255 units) — + // the worst ~96px region's mean signed shift. Encoder noise cancels + // inside a region; an absent element leaves its whole footprint + // shifted by its own delta, which no correct background can dilute. // A DrawElementVerificationError propagates to the orchestrator, which // re-renders the whole job via the screenshot path (never-wrong fallback). // Clamp to a defensible band: below ~10dB even severe damage passes (the @@ -298,12 +307,19 @@ function createDrainFrameGuard(args: { // never apply different PSNR floors to the same composition. The warn stays // here because only this path has a logger in scope. const verifyMinDb = resolveDeVerifyMinDb(); + const verifyMaxShift = resolveDeVerifyMaxShift(); const rawEnv = process.env.HF_DE_VERIFY_MIN_DB; if (rawEnv !== undefined && Number(rawEnv) !== verifyMinDb) { log.warn(`[Render] HF_DE_VERIFY_MIN_DB out of range [10,60]; using ${verifyMinDb}`, { raw: rawEnv, }); } + const rawShiftEnv = process.env.HF_DE_VERIFY_MAX_SHIFT; + if (rawShiftEnv !== undefined && Number(rawShiftEnv) !== verifyMaxShift) { + log.warn(`[Render] HF_DE_VERIFY_MAX_SHIFT out of range [1,64]; using ${verifyMaxShift}`, { + raw: rawShiftEnv, + }); + } const sizes: number[] = []; // Absolute floor lowers once a small frame proves deterministic (dark / // low-detail content), so a dark stretch doesn't re-capture every frame. @@ -373,8 +389,9 @@ function createDrainFrameGuard(args: { const truth = session.deVerifyFrames?.get(idx); if (truth) { let db: number; + let shift: number; try { - db = await psnrDb(buf, truth); + [db, shift] = await Promise.all([psnrDb(buf, truth), regionShift(buf, truth)]); } catch (err) { // Infrastructure failure (ffmpeg spawn/parse/tmpdir), not evidence of // damage — skip this sample rather than failing or falling back. @@ -384,7 +401,7 @@ function createDrainFrameGuard(args: { }); return buf; } - if (db < verifyMinDb) { + if (db < verifyMinDb || shift > verifyMaxShift) { // Keep the mismatched pair for diagnosis (tmpdir; OS-reaped). const dumpDir = await mkdtemp(join(tmpdir(), "hf-de-verify-fail-")).catch(() => null); if (dumpDir) { @@ -393,9 +410,15 @@ function createDrainFrameGuard(args: { writeFile(join(dumpDir, `frame-${idx}-truth.jpg`), truth), ]).catch(() => {}); } + if (db < verifyMinDb) { + throw new DrawElementVerificationError( + `drawElement self-verify failed at frame ${idx}: ${db.toFixed(1)}dB < ${verifyMinDb}dB vs pre-injection screenshot${dumpDir ? ` (pair: ${dumpDir})` : ""}`, + { kind: "psnr", frameIndex: idx, failedDb: db, verifyThresholdDb: verifyMinDb }, + ); + } throw new DrawElementVerificationError( - `drawElement self-verify failed at frame ${idx}: ${db.toFixed(1)}dB < ${verifyMinDb}dB vs pre-injection screenshot${dumpDir ? ` (pair: ${dumpDir})` : ""}`, - { kind: "psnr", frameIndex: idx, failedDb: db, verifyThresholdDb: verifyMinDb }, + `drawElement self-verify failed at frame ${idx}: region shift ${shift}/255 > ${verifyMaxShift}/255 vs pre-injection screenshot — a low-contrast region is absent from the drawElement frame${dumpDir ? ` (pair: ${dumpDir})` : ""}`, + { kind: "shift", frameIndex: idx, failedShift: shift, verifyMaxShift }, ); } stats.verifyChecked += 1; @@ -403,6 +426,7 @@ function createDrainFrameGuard(args: { log.info("[Render] drawElement self-verify passed", { frame: idx, psnrDb: db === Infinity ? "inf" : Number(db.toFixed(1)), + regionShift: shift, }); } return buf; diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index bc45ff2067..c595aab202 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -575,7 +575,7 @@ export interface RenderPerfSummary { * `fallbackReason` being set is the "any fallback fired" signal. */ selfVerifyFallback: boolean; - /** What tripped the fallback retry: psnr | blank | oom | de_renderer_stall | capture_error. */ + /** What tripped the fallback retry: psnr | shift | blank | oom | de_renderer_stall | capture_error. */ fallbackReason?: string; /** The failing PSNR (dB) when `fallbackReason === "psnr"`; undefined for every other reason (no score exists). */ fallbackFailedDb?: number; @@ -2215,7 +2215,7 @@ export function extractStandaloneEntryFromIndex( * cross-module-serialized error can't flip "blank" into "psnr". */ function deVerifyFallbackTelemetry(err: unknown): { - reason: "psnr" | "blank"; + reason: "psnr" | "blank" | "shift"; failedDb?: number; frameIndex?: number; thresholdDb?: number;