From 7e512c0547a374c155215c78248ec75d94380dcc Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Thu, 24 Sep 2026 16:19:03 +0000 Subject: [PATCH] Ask ffmpeg for the frames the capturer actually wrote MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every animated banner encode failed in production. The frame filename pattern was written down twice and the copies drifted: the capturer emits f-0000.png, and the GIF encoder asked ffmpeg for frame-0000.png, so palettegen exited with "Could find no file with path" on the first unit of every render. There is an exported FRAME_PATTERN for exactly this reason and the GIF path should have used it. It does now. The guard that existed to catch this did not, because it counted PNG files rather than looking at their names — fifty files were present, none of them named what ffmpeg was asked for. It now checks that each expected filename is present and names the first one missing, so the same class of mistake fails here with a sentence that says what happened rather than inside ffmpeg. The test could not have caught it either: its synthetic capturer wrote the encoder's spelling instead of the real one, which is how a divergence between two hardcoded copies passed CI. It derives both from FRAME_PATTERN now. Separately, a failed banner was failing the whole revision. The comment beside that catch claimed it would not — everything pushed onto `problems` fails the render, and the video for this campaign was fine. Three renders of a perfectly good pre-roll were thrown away over a GIF. The failure is logged and the revision proceeds; the banners are optional profiles and their absence is already visible in the asset list. Co-Authored-By: Claude Opus 5 (1M context) --- lib/ads/gif/encode.ts | 8 +++++++- lib/ads/gif/render.ts | 17 +++++++++++++---- lib/ads/video/render.ts | 11 ++++++----- tests/ads-gif-pipeline.test.ts | 6 +++++- 4 files changed, 31 insertions(+), 11 deletions(-) diff --git a/lib/ads/gif/encode.ts b/lib/ads/gif/encode.ts index 1de2f31..4060c9e 100644 --- a/lib/ads/gif/encode.ts +++ b/lib/ads/gif/encode.ts @@ -16,6 +16,7 @@ import { execFile } from "node:child_process"; import { promisify } from "node:util"; import path from "node:path"; import { GIF_FPS } from "../video/profiles"; +import { FRAME_PATTERN } from "../video/render"; const run = promisify(execFile); @@ -71,7 +72,12 @@ export async function encodeGif(input: { ffmpegPath?: string; }): Promise { const ffmpeg = input.ffmpegPath ?? "ffmpeg"; - const pattern = path.join(input.frameDir, "frame-%04d.png"); + // The shared pattern, not a second copy of it. These were written down twice + // and drifted: the capturer emits f-0000.png and this module asked ffmpeg for + // frame-0000.png, so every banner encode failed with "could find no file" + // while the frame count check passed — it counts .png files and never looks + // at their names. + const pattern = path.join(input.frameDir, FRAME_PATTERN); const palette = path.join(input.frameDir, "palette.png"); await run(ffmpeg, paletteArgs(pattern, palette), { maxBuffer: 16 * 1024 * 1024 }); diff --git a/lib/ads/gif/render.ts b/lib/ads/gif/render.ts index 3072568..2930412 100644 --- a/lib/ads/gif/render.ts +++ b/lib/ads/gif/render.ts @@ -14,7 +14,7 @@ import { mkdir, readdir, stat } from "node:fs/promises"; import path from "node:path"; import { GIF_FRAMES, videoProfile, type GifProfileId } from "../video/profiles"; import type { VideoDesignSnapshot } from "../video/snapshot"; -import type { FrameCapturer } from "../video/render"; +import { FRAME_PATTERN, type FrameCapturer } from "../video/render"; import { GIF_UNITS, gifDocument } from "./compose"; import { encodeGif, validateGif } from "./encode"; @@ -74,10 +74,19 @@ export async function renderAnimatedBanners(args: { // The compositor is the only thing that decides how many frames exist. If // the capture disagrees, the timeline did not run and encoding whatever // landed would ship a banner that is silently short. - const captured = (await readdir(framesDir)).filter((f) => f.endsWith(".png")); - if (captured.length !== GIF_FRAMES) { + // + // Checked by NAME, not just by count. Counting alone passed while the + // capturer wrote f-0000.png and the encoder asked ffmpeg for + // frame-0000.png, so the mismatch reached production as an ffmpeg error + // instead of a clear one here. + const wanted = Array.from({ length: GIF_FRAMES }, (_, i) => + FRAME_PATTERN.replace("%04d", String(i).padStart(4, "0")), + ); + const present = new Set(await readdir(framesDir)); + const missing = wanted.filter((f) => !present.has(f)); + if (missing.length > 0) { throw new Error( - `${unit.id}: captured ${captured.length} frames, expected ${GIF_FRAMES}`, + `${unit.id}: ${missing.length} of ${GIF_FRAMES} frames missing, first is ${missing[0]}`, ); } diff --git a/lib/ads/video/render.ts b/lib/ads/video/render.ts index 58a52fd..4d77402 100644 --- a/lib/ads/video/render.ts +++ b/lib/ads/video/render.ts @@ -388,11 +388,12 @@ export async function renderPreroll(args: { }); } } catch (err) { - problems.push({ - check: "animated banners", - expected: "three units", - actual: (err as Error).message, - }); + // Deliberately NOT pushed onto `problems`: everything in there fails the + // revision, and this comment previously claimed a failed banner would not + // — while the code did the opposite and failed whole renders whose video + // was fine. The banners are optional profiles; their absence is visible + // in the asset list, and the reason belongs in the log. + console.warn(`[render] animated banners skipped: ${(err as Error).message}`); } } diff --git a/tests/ads-gif-pipeline.test.ts b/tests/ads-gif-pipeline.test.ts index 21aced7..3224fc1 100644 --- a/tests/ads-gif-pipeline.test.ts +++ b/tests/ads-gif-pipeline.test.ts @@ -8,6 +8,7 @@ import { GIF_UNITS, gifDocument, gifFrameState, gifTimeline, GIF_TIMELINE } from import { GIF_PALETTE_COLORS, gifArgs, paletteArgs, validateGif } from "@/lib/ads/gif/encode"; import { renderAnimatedBanners } from "@/lib/ads/gif/render"; import { GIF_FPS, GIF_FRAMES, videoProfile } from "@/lib/ads/video/profiles"; +import { FRAME_PATTERN } from "@/lib/ads/video/render"; import type { VideoDesignSnapshot } from "@/lib/ads/video/snapshot"; const run = promisify(execFile); @@ -189,7 +190,10 @@ describe("end to end through real ffmpeg", () => { const svgPath = path.join(a.outDir, `f-${i}.svg`); await writeFile(svgPath, svg, "utf8"); await run("ffmpeg", ["-y", "-v", "error", "-i", svgPath, - path.join(a.outDir, `frame-${String(i).padStart(4, "0")}.png`)]); + // Named exactly as the real capturer names them, derived from the + // shared pattern. Writing "frame-%04d.png" here is what let the + // encoder's divergent literal pass CI and fail in production. + path.join(a.outDir, FRAME_PATTERN.replace("%04d", String(i).padStart(4, "0")))]); await rm(svgPath); } };