Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion packages/engine/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
14 changes: 14 additions & 0 deletions packages/engine/src/services/frameCapture.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
});
});
21 changes: 16 additions & 5 deletions packages/engine/src/services/frameCapture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
}
}

Expand Down Expand Up @@ -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;
Expand Down
34 changes: 34 additions & 0 deletions packages/engine/src/services/parallelCoordinator.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { describe, it, expect, vi } from "vitest";
import {
assertDiskSampleVerified,
calculateOptimalWorkers,
computeWorkerSizing,
distributeFrames,
Expand All @@ -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", () => {
Expand Down Expand Up @@ -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();
});
});
76 changes: 54 additions & 22 deletions packages/engine/src/services/parallelCoordinator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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)`,
);
}

Expand Down Expand Up @@ -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<number | null> {
): 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);
Expand All @@ -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,
Expand All @@ -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);
}
}

Expand Down
Loading
Loading