Skip to content

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

Open
vanceingalls wants to merge 1 commit into
ffprobe-3-frameratefrom
ffprobe-4-audio
Open

fix(engine): make the AAC duration refinement safe, cancellable and LC-only#2915
vanceingalls wants to merge 1 commit into
ffprobe-3-frameratefrom
ffprobe-4-audio

Conversation

@vanceingalls

Copy link
Copy Markdown
Collaborator

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

The AAC packet-count probe is a refinementdurationSeconds 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

…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 miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants