fix(engine): make the AAC duration refinement safe, cancellable and LC-only - #2915
fix(engine): make the AAC duration refinement safe, cancellable and LC-only#2915vanceingalls wants to merge 1 commit into
Conversation
…C-only 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) <noreply@anthropic.com>
miga-heygen
left a comment
There was a problem hiding this comment.
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/catchwraps 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_nameis"aac"for all AAC variants; the SBR marker lives inprofile. The 1024 samples-per-packet assumption is AAC-LC specific. -
profileadded toFFProbeStreaminterface. - Signal forwarded to the second
runFfprobecall viaoptions?.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
left a comment
There was a problem hiding this comment.
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 alternativehe-?aaccorrectly 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 alternativeaac\s*(?:se?|v[12])\bmatches"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
ManagedChildProcessshort-circuits on an already-abortedsignalbefore 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
left a comment
There was a problem hiding this comment.
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.
Stack 4/6, on top of #2914. Pre-existing; unrelated to #2740.
The AAC packet-count probe is a refinement —
durationSecondsis already correct fromformat.durationbefore 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_packetsdemuxes the entire container againstrunFfprobe's fixed 30s deadline. A long AAC-bearing file on slow or network storage times out,ManagedChildProcessSIGTERMs, andextractAudioMetadatarejects.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:signal.aborted === trueaudioPadTrim.ts:413passes 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 inprofile, not the codec name. Under explicit signalling ffprobe reports the doubled outputsample_rate(44100) while each packet carries 2048 output samples, so:That value unconditionally replaced
format.durationwith no sanity check, and htmlCompiler used it as the element duration — truncating the audio at exactly half. Now gated onprofile, with the field added toFFProbeStream.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