From 84c185088542086cb789a0781d83bd951caf67db Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Thu, 24 Sep 2026 20:32:44 +0000 Subject: [PATCH] Stop -frames:v from cutting the audio short MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every narrated render failed validation with audio around 3.3 of 5 seconds, and the cause was not the padding. -frames:v ends the whole output the moment the video stream reaches its limit, which cuts the audio wherever the encoder happened to have flushed to. That is why none of the obvious fixes worked. apad, apad=whole_dur=5, atrim=0:5 with asetpts, -shortest: all of them left the audio short, and an input already padded to exactly 5.000s still came out at 3.264s. The padding was fine the whole time; the output was being terminated early. An encode that carries sound is now bounded by duration instead. The frame count is still exactly 150, because 150 frames at 30fps is five seconds — the guarantee now comes from arithmetic rather than from a flag, and validation counts the frames either way. Silent encodes keep -frames:v, since they have no audio to truncate and a frame count cannot be satisfied by a timebase rounding error the way a duration can. -shortest goes from the music path as well: with both streams bounded to the same explicit length there is no shorter one to find. Worth recording why this took so long. The behaviour is version dependent: on ffmpeg 8, which is what this repo's test suite runs against locally, the narrated encode produces a correct 5.000s track and the test I added passes. On ffmpeg 4.4.2, which is what ships in the image, the same arguments produce 3.306s. A test that runs only on the newer binary cannot catch this, so it was reproduced by running ffmpeg inside the deployed container. Co-Authored-By: Claude Opus 5 (1M context) --- lib/ads/video/encode.ts | 32 +++++++++++++++++------ tests/ads-video-compose-validate.test.ts | 33 ++++++++++++++++++++---- 2 files changed, 52 insertions(+), 13 deletions(-) diff --git a/lib/ads/video/encode.ts b/lib/ads/video/encode.ts index 929e249..1a79b6f 100644 --- a/lib/ads/video/encode.ts +++ b/lib/ads/video/encode.ts @@ -64,10 +64,20 @@ export type Mp4EncodeOptions = { /** * Frames in, one MP4 out. * - * `-frames:v 150` rather than `-t 5`: the contract is a frame count, and a - * duration flag lets a timebase rounding error produce 149 or 151 frames that - * still measure "5.0 seconds". Counting frames is the check that actually - * catches a dropped one. + * `-frames:v 150` rather than `-t 5` for a silent encode: the contract is a + * frame count, and a duration flag lets a timebase rounding error produce 149 + * or 151 frames that still measure "5.0 seconds". Counting frames is the check + * that actually catches a dropped one. + * + * With an audio track that flag has to go. `-frames:v` ends the whole output + * the moment the video stream hits its limit, which cuts the audio wherever the + * encoder happened to have flushed to — around 3.3 of 5 seconds here. That is + * not a padding problem and no amount of `apad`, `apad=whole_dur`, `atrim` or + * `-shortest` fixes it: an input already padded to exactly 5.000s still came + * out at 3.264s. Bounding the output by duration instead produces both a + * 5.000s audio track and exactly 150 video frames, because 150 frames at 30fps + * IS five seconds — the frame count is still guaranteed, just by arithmetic + * rather than by a flag, and validation counts the frames either way. */ export function mp4Args(o: Mp4EncodeOptions): string[] { const p = videoProfile(o.profile); @@ -86,9 +96,16 @@ export function mp4Args(o: Mp4EncodeOptions): string[] { const withMusic = Boolean(o.audioPath && o.musicPath); if (withMusic) args.push("-stream_loop", "-1", "-i", o.musicPath!); + // See the note above: -frames:v truncates the audio, so an encode that + // carries sound is bounded by duration instead. + const carriesAudio = Boolean(o.musicPath) || Boolean(o.audioPath); + if (carriesAudio) { + args.push("-t", String(PREROLL_MS / 1000)); + } else { + args.push("-frames:v", String(PREROLL_FRAMES)); + } + args.push( - "-frames:v", - String(PREROLL_FRAMES), "-r", String(PREROLL_FPS), "-c:v", @@ -136,7 +153,7 @@ export function mp4Args(o: Mp4EncodeOptions): string[] { "-map", "0:v", "-map", "[a]", ); - args.push("-c:a", "aac", "-profile:a", "aac_low", "-ar", String(AAC_SAMPLE_RATE), "-b:a", "128k", "-ac", "2", "-shortest"); + args.push("-c:a", "aac", "-profile:a", "aac_low", "-ar", String(AAC_SAMPLE_RATE), "-b:a", "128k", "-ac", "2"); } else if (o.audioPath) { // `apad` extends the audio with silence indefinitely, and `-shortest` then // trims the result to the video's five seconds — so the track is exactly as @@ -156,7 +173,6 @@ export function mp4Args(o: Mp4EncodeOptions): string[] { // once padded, dropped the stream entirely. An explicit duration is not a // workaround — it is the thing actually being asserted. args.push("-af", "apad"); - args.push("-t", String(PREROLL_MS / 1000)); args.push("-c:a", "aac", "-profile:a", "aac_low", "-ar", String(AAC_SAMPLE_RATE), "-b:a", "128k", "-ac", "2"); } else { args.push("-an"); diff --git a/tests/ads-video-compose-validate.test.ts b/tests/ads-video-compose-validate.test.ts index 7efde71..3949861 100644 --- a/tests/ads-video-compose-validate.test.ts +++ b/tests/ads-video-compose-validate.test.ts @@ -187,6 +187,21 @@ describe("encoder arguments carry the media contract", () => { expect(narrated[narrated.indexOf("-ar") + 1]).toBe("48000"); }); + it("keeps the frame-count contract for a silent encode", () => { + // Silent encodes have no audio to truncate, so they keep the stronger + // guarantee: a frame count cannot be satisfied by a timebase rounding + // error the way a duration can. + const silent = mp4Args({ + framePattern: "f-%04d.png", + audioPath: null, + outPath: "/tmp/out.mp4", + profile: "master_1080p", + videoKbps: 4000, + }).join(" "); + expect(silent).toContain("-frames:v 150"); + expect(silent).not.toContain("-t 5"); + }); + it("pads a narration to the length of the picture", () => { // Every narration is shorter than its ad — a five-second read is about two // and a half seconds of speech. Without the pad the audio stream ended @@ -200,11 +215,12 @@ describe("encoder arguments carry the media contract", () => { videoKbps: 4000, }).join(" "); expect(a).toContain("-af apad"); - // apad alone runs forever, and -shortest does NOT trim it: in ffmpeg 4.x - // that flag keys off input durations, so the padded stream either kept the - // original 2.3s of speech or vanished entirely. The output duration is - // stated outright instead. + // Bounded by duration, and crucially WITHOUT -frames:v. That flag ends the + // whole output when the video stream hits its limit, cutting the audio + // wherever the encoder had flushed to — an input already padded to exactly + // 5.000s still came out at 3.264s. No padding filter can fix that. expect(a).toContain("-t 5"); + expect(a).not.toContain("-frames:v"); expect(a).not.toContain("-shortest"); }); @@ -434,7 +450,14 @@ describe("a music bed under the narration", () => { it("trims both to the length of the picture", () => { const f = withBed().join(" "); expect(f).toContain("atrim=0:5"); - expect(withBed()).toContain("-shortest"); + // Bounded by an explicit output duration rather than -shortest, and with + // no -frames:v. That flag ends the output when the video stream hits its + // limit and cut the audio to ~3.3 of 5 seconds no matter how it was + // padded; -shortest is then redundant, since both streams are already + // bounded to the same length. + expect(f).toContain("-t 5"); + expect(withBed()).not.toContain("-shortest"); + expect(withBed()).not.toContain("-frames:v"); }); it("a bed without a voice is ignored, since that is just music", () => {