From 7a0c75bb69925205eacd109433fb45ff4385bcae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Tue, 27 Oct 2026 14:48:54 +0000 Subject: [PATCH] fix(cli): a snapshot clip whose frame fails is named with ffmpeg's reason (#6564) --- packages/cli/src/commands/snapshot.test.ts | 77 +++++++++++++--------- packages/cli/src/commands/snapshot.ts | 34 ++++++---- 2 files changed, 69 insertions(+), 42 deletions(-) diff --git a/packages/cli/src/commands/snapshot.test.ts b/packages/cli/src/commands/snapshot.test.ts index 07135aeb56..1d01a7d93c 100644 --- a/packages/cli/src/commands/snapshot.test.ts +++ b/packages/cli/src/commands/snapshot.test.ts @@ -17,6 +17,7 @@ const snapshotState = vi.hoisted(() => ({ settle: vi.fn(async (_page: unknown, _options?: unknown) => ({ pending: [], failed: [] })), closeServer: vi.fn(async () => undefined), ffmpegTimesOut: false, + ffmpegFails: false, noFfmpeg: false, })); @@ -38,7 +39,13 @@ vi.mock("../capture/captureCompositionFrame.js", async (importOriginal) => { runFfmpegOnce: (...args: Parameters) => snapshotState.ffmpegTimesOut ? Promise.resolve({ code: null, timedOut: true, stderr: "" }) - : actual.runFfmpegOnce(...args), + : snapshotState.ffmpegFails + ? Promise.resolve({ + code: 1, + timedOut: false, + stderr: "\nError while decoding stream #0:0\n", + }) + : actual.runFfmpegOnce(...args), }; }); @@ -126,7 +133,7 @@ describe("transparent snapshot capture", () => { it("pairs every frame with the frame-exact reference frame under --against", () => { const source = readFileSync(new URL("./snapshot.ts", import.meta.url), "utf8"); expect(source).toContain("against: {"); - expect(source).toContain("extractVideoFrameToBuffer(opts.against, time)"); + expect(source).toContain("extractVideoFrameOrWarn(warnings, opts.against, time)"); expect(source).toContain('labels: ["render", "reference"]'); }); @@ -650,22 +657,37 @@ describe("extractVideoFrameToBuffer", () => { }, ); + const oneSecondClip = (dir: string) => { + 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", clip], + ]); + return clip; + }; + + it.skipIf(noProbe)( + "records the clip, time and FFmpeg's reason when the extract fails", + async () => { + const dir = mkdtempSync(join(tmpdir(), "hf-snapshot-fails-")); + snapshotState.ffmpegFails = true; + try { + const warnings: string[] = []; + expect(await extractVideoFrameOrWarn(warnings, oneSecondClip(dir), 0.5)).toBeNull(); + expect(warnings).toEqual([ + expect.stringMatching(/clip\.mp4 at 0\.50s: Error while decoding stream #0:0$/), + ]); + } finally { + snapshotState.ffmpegFails = false; + rmSync(dir, { recursive: true, force: true }); + } + }, + ); + it.skipIf(noProbe)("names the clip and time when FFmpeg times out", async () => { const dir = mkdtempSync(join(tmpdir(), "hf-snapshot-timeout-")); 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", - clip, - ]); + const clip = oneSecondClip(dir); snapshotState.ffmpegTimesOut = true; await expect(extractVideoFrameToBuffer(clip, 0.5)).rejects.toThrow(/clip\.mp4 at 0\.50s/); } finally { @@ -679,19 +701,7 @@ describe("extractVideoFrameToBuffer", () => { async () => { const dir = mkdtempSync(join(tmpdir(), "hf-snapshot-finding-")); 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", - clip, - ]); + const clip = oneSecondClip(dir); snapshotState.ffmpegTimesOut = true; const warnings: string[] = []; expect(await extractVideoFrameOrWarn(warnings, clip, 0.5)).toBeNull(); @@ -761,14 +771,19 @@ describe("extractVideoFrameToBuffer", () => { ); it.skipIf(noProbe)( - "leaves an unreadable video blank instead of failing the snapshot", + "leaves an unreadable video blank, naming it and why, instead of failing the snapshot", async () => { const dir = mkdtempSync(join(tmpdir(), "hf-snapshot-unreadable-")); try { const garbage = join(dir, "garbage.mp4"); writeFileSync(garbage, "not a video"); - expect(await extractVideoFrameToBuffer(join(dir, "missing.mp4"), 1)).toBeNull(); - expect(await extractVideoFrameToBuffer(garbage, 1)).toBeNull(); + const warnings: string[] = []; + expect(await extractVideoFrameOrWarn(warnings, join(dir, "missing.mp4"), 1)).toBeNull(); + expect(await extractVideoFrameOrWarn(warnings, garbage, 1)).toBeNull(); + expect(warnings).toEqual([ + expect.stringMatching(/FFprobe could not read .*missing\.mp4: .*No such file/), + expect.stringMatching(/FFprobe could not read .*garbage\.mp4: .*Invalid data found/), + ]); } finally { rmSync(dir, { recursive: true, force: true }); } diff --git a/packages/cli/src/commands/snapshot.ts b/packages/cli/src/commands/snapshot.ts index 1422e098f8..fe1abf8829 100644 --- a/packages/cli/src/commands/snapshot.ts +++ b/packages/cli/src/commands/snapshot.ts @@ -319,6 +319,14 @@ export function containingSourceFrameIndex(timestamps: readonly number[], time: return index; } +class FrameExtractError extends Error {} + +const lastLine = (text: string) => text.trim().split("\n").at(-1) ?? ""; +const processStderr = (error: unknown) => + error instanceof Error && "stderr" in error && typeof error.stderr === "string" + ? error.stderr + : ""; + /** Keyframe seek plus the decoded-time threshold that selects the frame containing `time`, or null. */ export async function probeContainingSourceFrameSeek( videoPath: string, @@ -346,9 +354,10 @@ export async function probeContainingSourceFrameSeek( return typeof parsed === "object" && parsed !== null ? (parsed as Record) : null; - } catch { - // An unreadable source blanks this one video, as a failed ffmpeg extraction does. - return null; + } catch (error) { + throw new FrameExtractError( + `FFprobe could not read ${videoPath}: ${lastLine(processStderr(error) || String(error))}`, + ); } }; const data = await probe("%+#1", "stream=time_base,start_pts:format=start_time,duration"); @@ -473,8 +482,6 @@ function warnNoHdrFiltersOnce(): void { ); } -class FrameExtractTimeoutError extends Error {} - export async function extractVideoFrameToBuffer( videoPath: string, timeSeconds: number, @@ -536,12 +543,17 @@ export async function extractVideoFrameToBuffer( warnNoHdrFiltersOnce(); result = await runFfmpegOnce(ffmpegPath, frameArgs(null), FFMPEG_EXTRACT_TIMEOUT_MS); } + const frameOf = `the frame of ${videoPath} at ${timeSeconds.toFixed(2)}s`; if (result.timedOut) { - throw new FrameExtractTimeoutError( - `FFmpeg timed out after ${FFMPEG_EXTRACT_TIMEOUT_MS / 1000}s extracting the frame of ${videoPath} at ${timeSeconds.toFixed(2)}s`, + throw new FrameExtractError( + `FFmpeg timed out after ${FFMPEG_EXTRACT_TIMEOUT_MS / 1000}s extracting ${frameOf}`, + ); + } + if (result.code !== 0 || !existsSync(outPath)) { + throw new FrameExtractError( + `FFmpeg could not extract ${frameOf}: ${lastLine(result.stderr) || `exit code ${result.code}`}`, ); } - if (result.code !== 0 || !existsSync(outPath)) return null; return readFileSync(outPath); } finally { try { @@ -552,7 +564,7 @@ export async function extractVideoFrameToBuffer( } } -/** A timed-out clip becomes a finding naming it, so the other frames of the snapshot still land. */ +/** A clip whose frame fails becomes a finding naming it and why, so the other frames of the snapshot still land. */ export async function extractVideoFrameOrWarn( warnings: string[], ...args: Parameters @@ -560,7 +572,7 @@ export async function extractVideoFrameOrWarn( try { return await extractVideoFrameToBuffer(...args); } catch (error) { - if (!(error instanceof FrameExtractTimeoutError)) throw error; + if (!(error instanceof FrameExtractError)) throw error; warnings.push(error.message); console.warn(` ${c.warn("⚠")} ${error.message}`); return null; @@ -1133,7 +1145,7 @@ async function captureSnapshots( if (opts.against) { // Frame-exact reference frame beside the render, so a rebuild can be // checked against its footage without hand-rolled ffmpeg + montage. - const refPng = await extractVideoFrameToBuffer(opts.against, time); + const refPng = await extractVideoFrameOrWarn(warnings, opts.against, time); if (!refPng) { console.error( ` ${c.warn("⚠")} --against has no frame at ${timeLabel} — reference pair skipped`,