Skip to content

fix(engine): make the AAC duration refinement safe, cancellable and LC-only #2915

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
vanceingalls merged 2 commits into from
Aug 1, 2026
Merged

fix(engine): make the AAC duration refinement safe, cancellable and LC-only #2915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
242a42f
fix(engine): make the AAC duration refinement safe, cancellable and L…
vanceingalls Jul 31, 2026
361fd49
fix(engine): allowlist AAC-LC for the packet refinement, not deny HE-AAC
vanceingalls Jul 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
182 changes: 173 additions & 9 deletions packages/engine/src/utils/ffprobe.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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" },
}),
},
Expand All @@ -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"],
]);
});
});

Expand Down Expand Up @@ -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);
});
});
70 changes: 56 additions & 14 deletions packages/engine/src/utils/ffprobe.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
}

Expand Down
Loading