From 242a42f6c94d5cc780477ed94c7938448639f67f Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Fri, 31 Jul 2026 00:06:13 -0700 Subject: [PATCH] fix(engine): make the AAC duration refinement safe, cancellable and LC-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The packet-count probe is a refinement — durationSeconds is already correct from format.duration before it runs — but it was written as if it were load-bearing. It could fail the whole call. No try/catch, and `-count_packets` demuxes the entire container against runFfprobe's fixed 30s deadline, so a long AAC file on slow or network storage timed out and extractAudioMetadata rejected. htmlCompiler catches that under the comment "Source file has no audio stream", returns duration 0, drops the audio element, and the render ships silent with no warning. Now caught, keeping the container duration. It ignored the caller's AbortSignal. Only the first probe received it, so aborting during the packet probe let the child run to completion and the call resolved with full metadata after cancellation — while audioPadTrim's comment claims the wrapper preserves cancellation. The signal is forwarded, and an abort still propagates rather than being swallowed as a refinement failure. It halved HE-AAC durations. ffprobe reports codec_name "aac" for HE-AAC v1/v2 as well — the marker is in the profile field — and with SBR each packet carries 2048 output samples against the doubled output sample_rate, so the 1024 assumption computed exactly half. A 10:00 podcast became 5:00 and htmlCompiler truncated the audio there. Gated on profile, with `profile` added to FFProbeStream. Tests: probe failure, junk output, three HE-AAC profile spellings (which also assert the second probe is not attempted), and that plain AAC-LC is still refined. Reverting the guards fails 5. Co-Authored-By: Claude Opus 5 (1M context) --- packages/engine/src/utils/ffprobe.test.ts | 78 +++++++++++++++++++++++ packages/engine/src/utils/ffprobe.ts | 59 +++++++++++++---- 2 files changed, 123 insertions(+), 14 deletions(-) diff --git a/packages/engine/src/utils/ffprobe.test.ts b/packages/engine/src/utils/ffprobe.test.ts index 40997518b..062662ee4 100644 --- a/packages/engine/src/utils/ffprobe.test.ts +++ b/packages/engine/src/utils/ffprobe.test.ts @@ -797,3 +797,81 @@ 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"])( + "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("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 23e8619a7..ca6893943 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,48 @@ 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 not apply to HE-AAC. ffprobe reports codec_name "aac" for + // HE-AAC v1/v2 as well — the marker is in the profile — and with SBR + // each packet carries 2048 output samples against the doubled output + // sample_rate, so assuming 1024 halves the duration. A 10:00 podcast + // became 5:00, truncating the audio at exactly half. + const isHeAac = /he-?aac|aac\s*(?:se?|v[12])\b/i.test(audioStream.profile ?? ""); + if (audioCodec === "aac" && !isHeAac && 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; } }