From 37f14fdcb12aeea8ec39cc6bce8c6a338ceca54f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Mon, 26 Oct 2026 17:27:13 +0000 Subject: [PATCH] fix(snapshot): video frames keep the colours render gives the same footage (#6425) * fix(snapshot): video frames keep the colours render gives the same footage * fix(snapshot): say so once when video colours cannot be matched to render * test(snapshot): cut the HEVC fixture's encode time so the flake gate stays inside its timeout * style(snapshot): format the HEVC fixture line --- packages/cli/src/commands/snapshot.test.ts | 71 +++++++++++++++---- packages/cli/src/commands/snapshot.ts | 15 ++++ packages/engine/src/index.ts | 1 + .../src/services/videoFrameExtractor.ts | 12 ++-- 4 files changed, 81 insertions(+), 18 deletions(-) diff --git a/packages/cli/src/commands/snapshot.test.ts b/packages/cli/src/commands/snapshot.test.ts index 1cba9e52c0..320cb1a3c9 100644 --- a/packages/cli/src/commands/snapshot.test.ts +++ b/packages/cli/src/commands/snapshot.test.ts @@ -446,6 +446,20 @@ describe("containingSourceFrameIndex", () => { }); }); +/** The pixel data of a PNG: the colour-tag chunks a retag adds are not part of the picture. */ +function samePixels(actual: Buffer | null | undefined, expected: Buffer): boolean { + const idat = (png: Buffer): Buffer => { + const chunks: Buffer[] = []; + for (let at = 8; at < png.length; at += 12 + png.readUInt32BE(at)) { + if (png.toString("latin1", at + 4, at + 8) === "IDAT") { + chunks.push(png.subarray(at + 8, at + 8 + png.readUInt32BE(at))); + } + } + return Buffer.concat(chunks); + }; + return !!actual && idat(actual).equals(idat(expected)); +} + describe("extractVideoFrameToBuffer", () => { const ffmpeg = findFFmpeg(); const ffprobe = findFFprobe(); @@ -491,7 +505,7 @@ describe("extractVideoFrameToBuffer", () => { expected, ]); const actual = await extractVideoFrameToBuffer(clip, time); - expect(actual?.equals(readFileSync(expected))).toBe(true); + expect(samePixels(actual, readFileSync(expected))).toBe(true); } } finally { rmSync(dir, { recursive: true, force: true }); @@ -532,7 +546,7 @@ describe("extractVideoFrameToBuffer", () => { "-y", expected, ]); - expect((await extractVideoFrameToBuffer(clip, 0.3))?.equals(readFileSync(expected))).toBe( + expect(samePixels(await extractVideoFrameToBuffer(clip, 0.3), readFileSync(expected))).toBe( true, ); } finally { @@ -576,7 +590,7 @@ describe("extractVideoFrameToBuffer", () => { ...["-i", clip, "-vf", `select=eq(n\\,${index})`, "-frames:v", "1", "-y", expected], ]); const actual = await extractVideoFrameToBuffer(clip, (index + 0.5) / 30); - expect(actual?.equals(readFileSync(expected)), `frame ${index}`).toBe(true); + expect(samePixels(actual, readFileSync(expected)), `frame ${index}`).toBe(true); } } finally { rmSync(dir, { recursive: true, force: true }); @@ -594,7 +608,17 @@ describe("extractVideoFrameToBuffer", () => { execFileSync(ffmpeg!, [ ...quiet, ...["-f", "lavfi", "-i", "testsrc2=d=70:r=30:s=160x90", "-pix_fmt", "yuv420p"], - ...["-c:v", "libx265", "-x265-params", x265, "-tag:v", "hvc1", source], + ...[ + "-c:v", + "libx265", + "-preset", + "ultrafast", + "-x265-params", + x265, + "-tag:v", + "hvc1", + source, + ], ]); const clip = join(dir, "cut.mov"); execFileSync(ffmpeg!, [...quiet, ...["-ss", "31.77", "-i", source, "-c", "copy", clip]]); @@ -608,7 +632,7 @@ describe("extractVideoFrameToBuffer", () => { ...["-i", clip, "-vf", "select=eq(n\\,1056)", "-frames:v", "1", "-y", expected], ]); const actual = await extractVideoFrameToBuffer(clip, 1056.5 / 30); - expect(actual?.equals(readFileSync(expected))).toBe(true); + expect(samePixels(actual, readFileSync(expected))).toBe(true); } finally { rmSync(dir, { recursive: true, force: true }); } @@ -716,9 +740,9 @@ describe("extractVideoFrameToBuffer", () => { "-y", expected, ]); - expect((await extractVideoFrameToBuffer(clip, time))?.equals(readFileSync(expected))).toBe( - true, - ); + expect( + samePixels(await extractVideoFrameToBuffer(clip, time), readFileSync(expected)), + ).toBe(true); } finally { rmSync(dir, { recursive: true, force: true }); } @@ -760,7 +784,7 @@ describe("extractVideoFrameToBuffer", () => { ...["-i", clip, "-vf", `select=eq(n\\,${index})`, "-frames:v", "1", "-y", expected], ]); const actual = await extractVideoFrameToBuffer(clip, index / 24); - expect(actual?.equals(readFileSync(expected)), `frame ${index}`).toBe(true); + expect(samePixels(actual, readFileSync(expected)), `frame ${index}`).toBe(true); } } finally { rmSync(dir, { recursive: true, force: true }); @@ -790,7 +814,7 @@ describe("extractVideoFrameToBuffer", () => { ...["-i", clip, "-vf", `select=eq(n\\,${index})`, "-frames:v", "1", "-y", expected], ]); const actual = await extractVideoFrameToBuffer(clip, index / 24); - expect(actual?.equals(readFileSync(expected)), `frame ${index}`).toBe(true); + expect(samePixels(actual, readFileSync(expected)), `frame ${index}`).toBe(true); } } finally { rmSync(dir, { recursive: true, force: true }); @@ -843,7 +867,7 @@ describe("extractVideoFrameToBuffer", () => { ...["-frames:v", "1", "-y", expected], ]); const actual = await extractVideoFrameToBuffer(clip, time); - expect(actual?.equals(readFileSync(expected)), `t=${time}`).toBe(true); + expect(samePixels(actual, readFileSync(expected)), `t=${time}`).toBe(true); } } finally { rmSync(dir, { recursive: true, force: true }); @@ -851,6 +875,7 @@ describe("extractVideoFrameToBuffer", () => { }); it.skipIf(noProbe)("selects frames in a TS whose timestamps start below zero", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); const dir = mkdtempSync(join(tmpdir(), "hf-snapshot-negative-")); try { const clip = join(dir, "clip.ts"); @@ -875,7 +900,7 @@ describe("extractVideoFrameToBuffer", () => { ...["-i", clip, "-vf", `select=eq(n\\,${index})`, "-frames:v", "1", "-y", expected], ]); const actual = await extractVideoFrameToBuffer(clip, time); - expect(actual?.equals(readFileSync(expected)), `t=${time}`).toBe(true); + expect(samePixels(actual, readFileSync(expected)), `t=${time}`).toBe(true); }; for (const [time, index] of [ [0, 0], @@ -894,7 +919,10 @@ describe("extractVideoFrameToBuffer", () => { [1, 30], ] as const) await frameAt(time, index); + // Colours cannot be matched to render without ffprobe: said once, not per frame. + expect(warn.mock.calls.filter(([m]) => /colours may differ/.test(String(m)))).toHaveLength(1); } finally { + warn.mockRestore(); vi.unstubAllEnvs(); rmSync(dir, { recursive: true, force: true }); } @@ -924,13 +952,28 @@ describe("extractVideoFrameToBuffer", () => { ...["-i", clip, "-vf", `select=eq(n\\,${index})`, "-frames:v", "1", "-y", expected], ]); const actual = await extractVideoFrameToBuffer(clip, time); - expect(actual?.equals(readFileSync(expected)), `t=${time}`).toBe(true); + expect(samePixels(actual, readFileSync(expected)), `t=${time}`).toBe(true); } } finally { rmSync(dir, { recursive: true, force: true }); } }); + it.skipIf(noProbe)("retags SDR footage as sRGB, as render does", async () => { + const dir = mkdtempSync(join(tmpdir(), "hf-snapshot-retag-")); + try { + const clip = join(dir, "clip.mp4"); + execFileSync(ffmpeg!, [ + ...["-hide_banner", "-loglevel", "error", "-f", "lavfi", "-i", "testsrc=d=1:r=24:s=160x90"], + ...["-pix_fmt", "yuv420p", "-colorspace", "bt709", "-color_trc", "bt709", clip], + ]); + const png = await extractVideoFrameToBuffer(clip, 0.5); + expect(png?.includes("sRGB", 0, "latin1")).toBe(true); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + it.skipIf(!ffmpeg)("extracts without ffprobe, as before frame selection needed it", async () => { const dir = mkdtempSync(join(tmpdir(), "hf-snapshot-no-ffprobe-")); vi.stubEnv("HYPERFRAMES_FFPROBE_PATH", join(dir, "missing-ffprobe")); @@ -945,7 +988,7 @@ describe("extractVideoFrameToBuffer", () => { ...["-hide_banner", "-loglevel", "error", "-i", clip, "-vf", "select=eq(n\\,12)"], ...["-frames:v", "1", "-y", expected], ]); - expect((await extractVideoFrameToBuffer(clip, 0.5))?.equals(readFileSync(expected))).toBe( + expect(samePixels(await extractVideoFrameToBuffer(clip, 0.5), readFileSync(expected))).toBe( true, ); } finally { diff --git a/packages/cli/src/commands/snapshot.ts b/packages/cli/src/commands/snapshot.ts index 4360a92e8f..5d2851865d 100644 --- a/packages/cli/src/commands/snapshot.ts +++ b/packages/cli/src/commands/snapshot.ts @@ -2,6 +2,8 @@ import { DEFAULT_CONFIG, buildZeroDurationDiagnostic, evaluateHfDiagnostic, + extractMediaMetadata, + sdrCanvasColourFilters, } from "@hyperframes/engine"; import type { Page } from "puppeteer-core"; import { reportArtifacts } from "../utils/toolContext.js"; @@ -394,6 +396,16 @@ async function probeSeekLanding( } } +let warnedColourUnknown = false; + +function warnColourUnknownOnce(): void { + if (warnedColourUnknown) return; + warnedColourUnknown = true; + console.warn( + ` ${c.warn("⚠")} snapshot colours may differ from render: install ffmpeg/ffprobe so video colours can be matched`, + ); +} + class FrameExtractTimeoutError extends Error {} export async function extractVideoFrameToBuffer( @@ -419,6 +431,9 @@ export async function extractVideoFrameToBuffer( args.push("-i", videoPath, "-map", "0:v:0"); const filters: string[] = []; if (seek.select !== undefined) filters.push(`select=gte(t-start_t\\,${seek.select})`); + const metadata = findFFprobe() ? await extractMediaMetadata(videoPath) : null; + if (metadata) filters.push(...sdrCanvasColourFilters(metadata)); + else warnColourUnknownOnce(); // Shrink so the short side still covers the displayed box at any rotation or fit; never enlarge. if (displayedPixels > 0) { filters.push( diff --git a/packages/engine/src/index.ts b/packages/engine/src/index.ts index c2cb887f2c..f0e0c1ae8a 100644 --- a/packages/engine/src/index.ts +++ b/packages/engine/src/index.ts @@ -253,6 +253,7 @@ export { VIDEO_FRAME_FORMATS, isVideoFrameFormat, EXTRACT_CACHE_MIN_AGE_MS, + sdrCanvasColourFilters, } from "./services/videoFrameExtractor.js"; export { isSelfOrAncestorHidden } from "./services/mediaHidden.js"; export { directorySizeBytes, gcExtractionCache } from "./services/extractionCache.js"; diff --git a/packages/engine/src/services/videoFrameExtractor.ts b/packages/engine/src/services/videoFrameExtractor.ts index 867e3c76cf..326af9736e 100644 --- a/packages/engine/src/services/videoFrameExtractor.ts +++ b/packages/engine/src/services/videoFrameExtractor.ts @@ -907,6 +907,13 @@ function chromeGuessForUntaggedMatrix(metadata: VideoMetadata): string[] { return [`setparams=colorspace=${hd ? "bt709" : "smpte170m"}`]; } +export function sdrCanvasColourFilters(metadata: VideoMetadata, toneMappedToSdr = false): string[] { + if (!isHdrColorSpaceUtil(metadata.colorSpace)) { + return [...chromeGuessForUntaggedMatrix(metadata), SDR_CANVAS_PASSTHROUGH_FILTER]; + } + return toneMappedToSdr ? [SDR_CANVAS_PASSTHROUGH_FILTER] : []; +} + /** * Longest stretch of output one ffmpeg process extracts; longer ranges split into segments * that run side by side. Segments sample the same absolute times as a single pass, so their @@ -1153,10 +1160,7 @@ async function extractVideoFramesRangeUnredacted( vfFilters.push(hdrToSdrToneMapFilter(metadata.colorSpace ?? {}, frameColour)); } if (toneMappedToSdr || (!isHdr && !options.sdrToHdrTransfer)) { - vfFilters.push( - ...(isHdr ? [] : chromeGuessForUntaggedMatrix(metadata)), - SDR_CANVAS_PASSTHROUGH_FILTER, - ); + vfFilters.push(...sdrCanvasColourFilters(metadata, toneMappedToSdr)); if (format === "jpg") { if (options.shownBox) vfFilters.push(halveToCoverScaleFilter(options.shownBox)); vfFilters.push(SDR_JPEG_AS_BT601_FULL_RANGE_FILTER);