diff --git a/packages/engine/src/utils/ffprobe.test.ts b/packages/engine/src/utils/ffprobe.test.ts index 40997518bb..9c6de657e3 100644 --- a/packages/engine/src/utils/ffprobe.test.ts +++ b/packages/engine/src/utils/ffprobe.test.ts @@ -212,40 +212,81 @@ describe("ffprobe missing-binary fallback", () => { expect(calls[0]?.args.slice(0, 2)).toEqual(["-v", "error"]); }); + // `profile` matters now: the packet refinement is an allowlist on AAC-LC, + // because the 1024-sample formula is wrong for LD/ELD/HE and unverified for + // the rest. An unprofiled "aac" stream deliberately keeps its container + // duration rather than being refined on an assumption. it.each([ - { name: "non-AAC metadata", codec: "mp3", packets: undefined, expected: 1.25, calls: 1 }, - { name: "valid AAC packet count", codec: "aac", packets: "783", expected: 16.704, calls: 2 }, + { + name: "non-AAC metadata", + codec: "mp3", + profile: undefined, + packets: undefined, + expected: 1.25, + calls: 1, + }, + { + name: "unprofiled AAC", + codec: "aac", + profile: undefined, + packets: "783", + expected: 1.25, + calls: 1, + }, + { + name: "valid AAC-LC packet count", + codec: "aac", + profile: "LC", + packets: "783", + expected: 16.704, + calls: 2, + }, { name: "missing AAC packet count", codec: "aac", + profile: "LC", packets: undefined, expected: 1.25, calls: 2, }, - { name: "zero AAC packet count", codec: "aac", packets: "0", expected: 1.25, calls: 2 }, + { + name: "zero AAC packet count", + codec: "aac", + profile: "LC", + packets: "0", + expected: 1.25, + calls: 2, + }, { name: "invalid AAC packet count", codec: "aac", + profile: "LC", packets: "invalid", expected: 1.25, calls: 2, }, ])( "derives audio duration for $name", - async ({ codec, packets, expected, calls: expectedCalls }) => { + async ({ codec, profile, packets, expected, calls: expectedCalls }) => { const outcomes: SpawnOutcome[] = [ { kind: "exit", code: 0, stdout: JSON.stringify({ streams: [ - { codec_type: "audio", codec_name: codec, sample_rate: "48000", channels: 2 }, + { + codec_type: "audio", + codec_name: codec, + sample_rate: "48000", + channels: 2, + profile, + }, ], format: { duration: "1.25", bit_rate: "128000" }, }), }, ]; - if (codec === "aac") { + if (codec === "aac" && profile === "LC") { outcomes.push({ kind: "exit", code: 0, @@ -553,7 +594,17 @@ describe("ffprobe option separator", () => { kind: "exit", code: 0, stdout: JSON.stringify({ - streams: [{ codec_type: "audio", codec_name: "aac", sample_rate: "48000", channels: 2 }], + streams: [ + { + codec_type: "audio", + codec_name: "aac", + // LC, so the packet-count refinement actually runs and its + // argv is covered here too. + profile: "LC", + sample_rate: "48000", + channels: 2, + }, + ], format: { duration: "1.25" }, }), }, @@ -574,8 +625,14 @@ describe("ffprobe option separator", () => { await extractAudioMetadata("/tmp/-audio.wav"); await analyzeKeyframeIntervals("/tmp/-video.mp4"); - const args = calls.flatMap((call) => [...(call.args ?? [])]); - expect(args.filter((arg) => arg === "--")).toHaveLength(3); + // Per call, not a flattened count. A total of 3 is satisfied by one call + // emitting three `--` and two emitting none — i.e. it cannot fail for + // misplacement, which is the shape of bug this exists to catch. + expect(calls.map((call) => (call.args ?? []).slice(-2))).toEqual([ + ["--", "/tmp/-audio.wav"], + ["--", "/tmp/-audio.wav"], + ["--", "/tmp/-video.mp4"], + ]); }); }); @@ -797,3 +854,110 @@ describe("pix_fmt alpha detection", () => { it.each(ALPHA)("detects alpha in %s", (fmt) => expect(pixelFormatHasAlpha(fmt)).toBe(true)); it.each(OPAQUE)("reports %s as opaque", (fmt) => expect(pixelFormatHasAlpha(fmt)).toBe(false)); }); + +describe("AAC duration refinement must never fail or distort the call", () => { + afterEach(() => { + vi.resetModules(); + vi.doUnmock("child_process"); + }); + + const aacStream = (profile?: string) => + JSON.stringify({ + streams: [ + { codec_type: "audio", codec_name: "aac", sample_rate: "44100", channels: 2, profile }, + ], + format: { duration: "600", bit_rate: "128000" }, + }); + + async function probe(outcomes: SpawnOutcome[], file: string) { + const { spawn, calls } = createSpawnSpy(outcomes); + vi.resetModules(); + vi.doMock("child_process", () => ({ spawn })); + const { extractAudioMetadata } = await import("./ffprobe.js"); + return { meta: await extractAudioMetadata(file), calls }; + } + + // Regression: the refinement had no try/catch, so its failure rejected a + // call whose duration was already correct. htmlCompiler catches that as + // "no audio stream", returns 0, and the render ships silent. + it("keeps the container duration when the packet probe fails", async () => { + const { meta } = await probe( + [ + { kind: "exit", code: 0, stdout: aacStream("LC") }, + { kind: "exit", code: 1, stdout: "", stderr: "ffprobe exploded" }, + ], + "/tmp/aac-packet-probe-fails.m4a", + ); + expect(meta.durationSeconds).toBe(600); + }); + + it("keeps the container duration when the packet probe returns junk", async () => { + const { meta } = await probe( + [ + { kind: "exit", code: 0, stdout: aacStream("LC") }, + { kind: "exit", code: 0, stdout: "not json at all" }, + ], + "/tmp/aac-packet-probe-junk.m4a", + ); + expect(meta.durationSeconds).toBe(600); + }); + + // Regression: codec_name is "aac" for HE-AAC too, but its packets carry + // 2048 output samples — assuming 1024 halved a 10:00 podcast to 5:00. + it.each([ + "HE-AAC", + "HE-AACv2", + "he-aac", + // 512- and 480-sample framing: the 1024 multiplier overstates these by + // 2x and ~2.13x, overwriting an already-correct container duration. + "LD", + "ELD", + // Not verified for this maths, so not allowlisted. + "Main", + "SSR", + "LTP", + "xHE-AAC", + // Missing or unrecognised profile must NOT fall through to the formula — + // that is how an unknown HE spelling kept the truncation bug. + "", + "SomethingNew", + ])("does not apply the LC packet maths to profile %s", async (profile) => { + const { meta, calls } = await probe( + [{ kind: "exit", code: 0, stdout: aacStream(profile) }], + `/tmp/heaac-${profile}.m4a`, + ); + expect(meta.durationSeconds).toBe(600); + // The second probe is not even attempted. + expect(calls).toHaveLength(1); + }); + + it.each(["LC", " lc "])("still refines AAC profile %s", async (profile) => { + const { meta } = await probe( + [ + { kind: "exit", code: 0, stdout: aacStream(profile) }, + { + kind: "exit", + code: 0, + stdout: JSON.stringify({ streams: [{ nb_read_packets: "861" }], format: {} }), + }, + ], + `/tmp/aac-lc-${profile.trim()}.m4a`, + ); + expect(meta.durationSeconds).toBeCloseTo((861 * 1024) / 44100, 5); + }); + + it("still refines a plain AAC-LC stream", async () => { + const { meta } = await probe( + [ + { kind: "exit", code: 0, stdout: aacStream("LC") }, + { + kind: "exit", + code: 0, + stdout: JSON.stringify({ streams: [{ nb_read_packets: "861" }], format: {} }), + }, + ], + "/tmp/aac-lc-refined.m4a", + ); + expect(meta.durationSeconds).toBeCloseTo((861 * 1024) / 44100, 5); + }); +}); diff --git a/packages/engine/src/utils/ffprobe.ts b/packages/engine/src/utils/ffprobe.ts index 23e8619a70..acc47bfb98 100644 --- a/packages/engine/src/utils/ffprobe.ts +++ b/packages/engine/src/utils/ffprobe.ts @@ -136,6 +136,9 @@ export interface AudioMetadata { interface FFProbeStream { codec_type: string; codec_name?: string; + /** e.g. "LC", "HE-AAC", "HE-AACv2" — where the SBR marker lives, since + * codec_name is plain "aac" for all of them. */ + profile?: string; width?: number; height?: number; duration?: string; @@ -538,20 +541,59 @@ export async function extractAudioMetadata( const streamDuration = audioStream.duration ? parseFloat(audioStream.duration) : undefined; const sampleRate = audioStream.sample_rate ? parseInt(audioStream.sample_rate) : 44100; const audioCodec = audioStream.codec_name || "unknown"; - if (audioCodec === "aac" && sampleRate > 0) { - const packetStdout = await runFfprobe(filePath, [ - "-select_streams", - "a:0", - "-count_packets", - "-show_entries", - "stream=nb_read_packets", - "-print_format", - "json", - ]); - const packetOutput = parseProbeJson(packetStdout); - const packetCount = Number(packetOutput.streams[0]?.nb_read_packets); - if (Number.isFinite(packetCount) && packetCount > 0) { - durationSeconds = (packetCount * AAC_LC_SAMPLES_PER_PACKET) / sampleRate; + // AAC-LC container durations are often slightly wrong, so the packet + // count gives a better one. Three constraints on that refinement: + // + // 1. It must never fail the call. durationSeconds is ALREADY correct from + // format.duration at this point. `-count_packets` demuxes the whole + // container against runFfprobe's fixed 30s deadline, so a long file on + // slow or network storage times out — and the caller in htmlCompiler + // catches that under "Source file has no audio stream", returns + // duration 0, drops the audio element and ships a silent render. + // 2. It must honour the caller's AbortSignal. Only the first probe + // received it, so aborting during this one was ignored and the call + // resolved with full metadata long after cancellation. + // 3. It must apply ONLY to profiles whose 1024-sample framing is + // established. ffprobe reports codec_name "aac" for every AAC + // variant — the framing lives in the profile: + // + // LC 1024 samples/frame <- the only one this maths fits + // HE-AAC v1/v2 2048 output samples against a doubled sample_rate + // LD 512 + // ELD 480 + // Main/SSR/LTP 1024 nominally, but not verified here + // xHE-AAC (USAC) variable + // + // An ALLOWLIST, not a HE-AAC denylist. The denylist form let LD/ELD + // through (halving to a third of the true duration), and let an + // unknown or missing profile through too — so an unrecognised HE + // spelling preserved the exact truncation this is meant to close. + // A skipped refinement is harmless: format.duration is already correct. + const isAacLc = /^\s*LC\s*$/i.test(audioStream.profile ?? ""); + if (audioCodec === "aac" && isAacLc && sampleRate > 0) { + try { + const packetStdout = await runFfprobe( + filePath, + [ + "-select_streams", + "a:0", + "-count_packets", + "-show_entries", + "stream=nb_read_packets", + "-print_format", + "json", + ], + options?.signal, + ); + const packetOutput = parseProbeJson(packetStdout); + const packetCount = Number(packetOutput.streams[0]?.nb_read_packets); + if (Number.isFinite(packetCount) && packetCount > 0) { + durationSeconds = (packetCount * AAC_LC_SAMPLES_PER_PACKET) / sampleRate; + } + } catch (error) { + // An abort is the caller's intent, not a refinement failure — let it + // through. Anything else keeps the container duration we already have. + if (options?.signal?.aborted) throw error; } }