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

[vanceingalls]

Stack 4/6, on top of #2914. Pre-existing; unrelated to #2740.

The AAC 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. Three consequences.

It could fail the whole call

No try/catch, and -count_packets demuxes the entire container against runFfprobe's fixed 30s deadline. A long AAC-bearing file on slow or network storage times out, ManagedChildProcess SIGTERMs, and extractAudioMetadata rejects.

The caller then does this:

// htmlCompiler.ts:435 — "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 options?.signal. A/B with a wrapper stalling one probe 4s and aborting at 1200ms:

• abort during the first probe → throws, as expected
• abort during the packet probe → child ran to completion, function resolved at 4350ms with full metadata and signal.aborted === true

audioPadTrim.ts:413 passes a signal and its comment claims the wrapper preserves cancellation — false for AAC inputs. Forwarded now, and an abort still propagates rather than being swallowed as a refinement failure.

It halved HE-AAC durations

The gate is codec_name === "aac", but ffprobe reports "aac" for HE-AAC v1/v2 as well — the SBR marker lives in profile, not the codec name. Under explicit signalling ffprobe reports the doubled output sample_rate (44100) while each packet carries 2048 output samples, so:

packetCount * 1024 / 44100   →   a 10:00 podcast computes as 5:00

That value unconditionally replaced format.duration with no sanity check, and htmlCompiler used it as the element duration — truncating the audio at exactly half. Now gated on profile, with the field added to FFProbeStream.

The comment justifying 1024 ("FFmpeg's built-in AAC encoder emits AAC-LC") is true of files this project produces, not of arbitrary input.

Verification

Probe failure, junk output, three HE-AAC profile spellings (asserting the second probe is not even attempted), and that plain AAC-LC is still refined. Reverting the guards fails 5. Engine suite: 1280 pass.

🤖 Generated with Claude Code

[miga-heygen]

Stack 4/6: AAC duration refinement — Review

Clean fix, no concerns. All three failure modes are real and the fix handles each correctly.

Verified

• try/catch wraps the refinement — an abort rethrows (if (options?.signal?.aborted) throw error), everything else silently keeps the container duration. Correct: abort is the caller's intent; probe failure is a refinement gap.
• HE-AAC gate: regex against audioStream.profile. Correct — codec_name is "aac" for all AAC variants; the SBR marker lives in profile. The 1024 samples-per-packet assumption is AAC-LC specific.
• profile added to FFProbeStream interface.
• Signal forwarded to the second runFfprobe call via options?.signal — was missing, so the packet probe ran to completion after cancellation.
• Tests: probe failure (container duration kept), junk output (container duration kept), 3 HE-AAC profiles (second probe not attempted — asserted via calls.length === 1), plain AAC-LC (still refined to packet count). Reverting the guards fails 5.

Ships clean.

[james-russo-rames-d-jusso]

The AAC refinement is a much sharper piece of code now — try/catch that keeps the container duration on failure, options?.signal forwarded through to the second probe, and a profile-gated early-out so HE-AAC doesn't get halved. Tests lock each regression: junk output, spawn failure, three HE-AAC profile spellings asserting the second probe isn't even attempted, and the plain AAC-LC path still refining. Nicely bounded.

Questions

• HE-AAC heuristic — dead alternative and one real miss. The regex is /he-?aac|aac\s*(?:se?|v[12])\b/i. The first alternative he-?aac correctly matches ffmpeg's "HE-AAC" and "HE-AACv2" (and case-insensitive variants) — that's your stated exemplar set and the tests exercise it. The second alternative aac\s*(?:se?|v[12])\b matches "AAC v2", "AAC s", "AAC se" — none of which are actually emitted as ffmpeg profile strings AFAICT (libavcodec/profiles.c → "LC", "HE-AAC", "HE-AACv2", "LD", "ELD", "Main", "LTP", "SSR", "MPEG2 AAC-LC", "MPEG2 AAC-HE", "MPEG4 AAC-LC"). Two nits fall out of that:
  • What was the second alternative meant to catch? Reads dead to me — happy to be corrected if there's a variant I'm missing.
  • "MPEG2 AAC-HE" (MPEG-2 encapsulated HE-AAC — real ffmpeg emission) doesn't match either alternative and would fall into the LC math branch, so a MPEG-2 HE-AAC 10:00 file would still halve to 5:00. Almost certainly not in HeyGen's pipeline, but the "HE-AAC v1/v2" comment is a tad optimistic. Tightening to what ffmpeg actually emits — e.g. /^(?:HE-?AAC|MPEG[24] AAC-HE)/i — would close the seam and drop the dead alt.

Non-blocking either way — happy to defer if MPEG-2-encapsulated AAC really is off the table.

Nits

• AAC_LC_SAMPLES_PER_PACKET — worth a one-liner comment that 1024 is LC/Main/LTP-specific; LD/ELD use 512 and SSR uses 256. Niche enough not to gate on, but the constant name currently hides the assumption. (These profiles fall through to LC math too if they ever appear, but ffmpeg emits them rarely.)

What I didn't verify

• Whether ManagedChildProcess short-circuits on an already-aborted signal before spawning. If not, an abort landing between the first and second probe would still spawn the second child briefly — the try/catch would swallow the eventual abort throw as intended, but there's a small window of wasted work. Almost certainly fine as designed.

— Review by Rames D Jusso

[miguel-heygen]

Reviewed exact head 40542573418ebec29069cfd7e42c15910608c0af against stack parent #2914. One blocking correctness issue:

P1 — the new “LC-only” gate is a negative HE-AAC denylist, so non-LC AAC still gets the 1024-sample formula. At packages/engine/src/utils/ffprobe.ts:527-528, every codec_name === "aac" profile except the regex-selected HE spellings enters the packet refinement. FFmpeg also reports AAC profiles LD, ELD, Main, SSR, LTP, and xHE-AAC; LD/ELD use 480/512-sample frames, so the 1024 multiplier overwrites an already-correct container duration with a materially wrong value. Missing/unknown profiles also fall through, so an unrecognized HE stream preserves the same truncation class this PR is meant to close.

Please make this an affirmative allowlist for profiles whose 1024-sample semantics are established (at minimum exact LC; retain unknown only with independent proof), and add LD/ELD plus unknown-profile regressions that keep the container duration and do not launch the packet probe. FFmpeg primary references: profile table, LD/ELD 480/512-frame behavior.

Abort propagation itself is correct on inspection. Exact-head CI is terminal green, the PR is mergeable, and there are no unresolved review threads.

[vanceingalls]

Fixed in e19eea7be — flipped to an affirmative allowlist.

You identified the actual shape of the error: I wrote a denylist and described it as "LC-only", so everything except the HE spellings I happened to enumerate still got the 1024-sample formula. That includes the two cases where it is materially wrong —

profile	frame	effect of the 1024 assumption
LD	512	duration reported 2× too long
ELD	480	~2.13× too long
Main / SSR / LTP	1024 nominal	unverified here
xHE-AAC	variable	unverified
missing / unrecognised	—	fell through, preserving the truncation class

— and, worse for the PR's own purpose, an unknown or misspelled HE profile fell straight through, keeping exactly the bug this was meant to close.

Now an exact match on LC (whitespace tolerated). Skipping the refinement is harmless by construction: format.duration is already correct before it runs, so an unrecognised profile simply keeps it.

Tests: 11 non-LC profiles assert the container duration is kept and that the second probe is never launched. One thing worth flagging — the pre-existing duration table asserted that an unprofiled aac stream refines, i.e. the behaviour you're asking me to remove, so I updated it to state LC explicitly and added an unprofiled row that must not refine.

Reverting the allowlist fails 8.

[miguel-heygen]

Re-review

Approved at exact head e19eea7beed26e464af28a930dbcbc0974d80a43.

The duration refinement is now an affirmative AAC-LC allowlist. LD, ELD, HE/xHE, unverified profiles, missing profiles, and unknown future values retain the already-valid container duration and do not launch the packet probe; LC and whitespace-normalized LC still refine. The strengthened tests pin both output and probe count, and abort propagation remains intact.

The regression workflow is currently non-green solely because buildx could not pull moby/buildkit from Docker Hub (registry-1.docker.io timeout) before tests ran; sibling shards were cancelled by fail-fast. That is infrastructure rather than a product failure, but it still needs a successful rerun before merge.

The base branch was changed.