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", () => {