fix(engine): support current FFmpeg filter scripts#2324
Conversation
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 05fd570.
Clean two-attempt fallback that preserves backward-compatibility with FFmpeg 4.2 while adding forward-compatibility with nightlies that removed the legacy alias. legacyFilterScriptOptionIsUnsupported's regex correctly narrows to the specific "option gone" failure mode (filter_complex_script name + "unrecognized option" / "option not found" phrasing), so unrelated ffmpeg errors don't burn a spurious retry. The in-place option swap on the argv copy is safe — there's exactly one -filter_complex_script occurrence and the file path immediately following is unchanged. Cleanup via try/finally maintains the same guarantee the old .finally() chain had. Miguel verified both directions empirically (FFmpeg 4.2 rejects -/filter_complex, nightly N-125551 rejects the legacy spelling).
What I didn't verify
- Whether an intermediate FFmpeg version (post-deprecation but pre-removal) emits a warning to stderr about
filter_complex_scriptwhile still accepting it. The regex would false-positive that message, but only if the run also failed for another reason — in which case a-/filter_complexretry is harmless (both would fail). Non-issue in practice.
vanceingalls
left a comment
There was a problem hiding this comment.
R1 by Via — posted after Rames-D's parallel review; independent verification.
Peer-lens with Rames-D: convergent LGTM. Same-lens finding on the narrow retry classifier regex (Rames flagged the intermediate-version warning case; my review adds three additional non-blocking nits — observability gap on the still-present video-only fallback in renderOrchestrator.ts:1870, two-spawn cost on newer FFmpeg with no memoization, and negative-path test coverage).
PR #2324 — fix(engine): support current FFmpeg filter scripts
Verdict: 🟢 LGTM
Head: 05fd570d on main
Status: all CI green (Lint, Typecheck, Tests on windows-latest, Render on windows-latest, regression + 8 regression shards, CodeQL, SDK unit+contract+smoke, Studio load smoke, CLI smoke, Test: runtime contract, preview-regression, player-perf, Smoke: global install); mergeable_state=blocked (branch protection — needs formal reviews).
Summary of change
Two-file diff in packages/engine/src/services/audioMixer.ts + its test. mixAudioTracks now attempts -filter_complex_script <path> (legacy) first; on failure, if stderr matches /filter_complex_script/i AND /unrecognized option|option (?:was )?not found/i, retries once with -/filter_complex <path> (the current file-valued syntax) using the same temp graph file. Cleanup moved into finally so both attempts share one temp-dir lifetime. New test drives the retry path via mockImplementationOnce returning the exact nightly stderr; runFfmpegMock extended to read the graph file whether either option name is present.
Findings
-
🟢 Core claim holds.
packages/engine/src/services/audioMixer.ts:449-459legacy-first + narrow-stderr-classification retry. Pre-fix path traced end-to-end:mixAudioTracksfailure →audioResult.success=falseinpackages/producer/src/services/render/stages/audioStage.ts:72→hasAudio=falseinrenderOrchestrator.ts:1867→log.warn("[Render] Audio mix failed — output will be video-only")atrenderOrchestrator.ts:1870, render then continues to composite video-only. Before this PR every mix on nightly N-125551+ would silently degrade with just a warn line; Magi's characterization is accurate. -
🟢 Backward compatibility preserved. Legacy
-filter_complex_scriptstill the first attempt, so older FFmpeg (verified against 4.2 per PR body) never touches the new spelling. No regression risk on the older-build path. -
🟢 Blast radius is contained.
-filter_complex_script(file-valued) appears only inaudioMixer.ts+ its test. Four other producer callers (packages/producer/src/{utils/audioRegression.ts:187, services/audioExtractor.ts:223, parity-harness.ts:115, regression-harness.ts:577}) use inline-filter_complex, which FFmpeg has NOT deprecated — they're unaffected. No parallel branch still using the removed flag. -
🟢 Same temp file reused across retry.
currentArgs[currentArgs.indexOf("-filter_complex_script")] = "-/filter_complex"(line 455) swaps the option name only; the adjacent path index still points at the on-disk graph.finally(line 457) deletesscriptDirafter both attempts — no leaked temp dirs. -
🟢 Volume-automation fallback still lives on top of the new retry. The outer
runMix(false) → runMix(true)degraded-automation path (lines 462-477) sits aboverunMix, so the retry logic composes correctly with the pre-existing "drop keyframes rather than drop the track" defense. -
🟢 Test not aspirational.
audioMixer.test.ts:353-402calls the realprocessCompositionAudioexport and assertsrunFfmpegMockwas invoked 3× with the correct arg-shape transition (legacy → current) between call 2 and 3. Production dispatch verified. -
🟡 NIT/O2 — observability gap on the video-only fallback.
renderOrchestrator.ts:1870still justlog.warns when audio drops out; no counter / no funnel event. Fleet-wide visibility on "how often does audio drop and why" isn't improved by this PR — you fix the primary failure mode but the still-latent fallback stays observable only via log grep. Sibling of the telemetry-gap-on-parallel-branch pattern. Out of scope for this bug fix, but worth a follow-up ticket while it's fresh — Magi's own "silent audio loss is especially dangerous" framing suggests a metric is warranted. -
🟡 NIT/O2 — retry classifier narrow to two error phrasings. The stderr regex matches "Unrecognized option" (nightly N-125551 wording, verified) and "option (was) not found". An intermediate FFmpeg build that phrases the same failure differently (e.g. "invalid option", "no such option") would silently skip the retry and revert to the video-only fallback. Acceptable trade-off given the PR's explicit test matrix, but a bug tracker note might be useful if similar reports surface.
-
🟡 NIT/O2 — two-spawn cost per mix on newer FFmpeg with no memoization. Every mix on a build without the legacy alias pays for two ffmpeg spawns (legacy fails, current succeeds). Correctness is fine, but a module-level flag after first successful
-/filter_complexwould drop the cost from 2 to 1 spawn on subsequent renders. Perf-only, out of scope. -
🟡 NIT/O2 — negative-path tests missing. No test asserts (a) legacy failure with non-matching stderr does NOT retry, (b) both attempts failing surfaces a useful error. Coverage gap, not a correctness bug — the code paths are simple enough to be safe.
Adversarial refute attempts
- Could the retry reuse a deleted temp file? No —
finallyruns after BOTH attempts (rmSync moved outsidereturn), and the retry swaps only the option name, not the path arg. - Could a false-positive stderr match (e.g. filter script content that mentions "filter_complex_script" plus an unrelated "not found" error) trigger a wasted retry? Yes, technically — but the retry uses the same file with a different option name; if the real error is elsewhere it just fails the same way. Wasted work, not incorrect behavior.
- Does older FFmpeg (4.2) ever hit the
-/filter_complexpath? Only if legacy fails with the target stderr. Older FFmpeg supports-filter_complex_script, so first attempt succeeds — retry never fires. No regression path. - Producer callers still using the removed flag? Grep across the repo shows
-filter_complex_scriptonly inaudioMixer.ts+ its test. All other-filter_complexsites are inline (unaffected by the deprecation). - Test index alignment? Verified — mock #1 (prepareAudioTrack) pushes
""at index 0; mock #2 (legacy mix) pushes""at index 1; default mock (current mix) reads the graph file at index 2.capturedFilterScripts[2]correctly containsamix=inputs=1.
Cross-checks
- Head SHA current: yes (
05fd570dedb1068671575e227bfeda2465e4e273) - Prior reviewers on record: none
- Mergeability:
blocked(branch protection — needs formal reviews) - CI lanes: all required lanes green, no failing checks (only
skippedare optional Mintlify/Perf/Preview-parity/CLI-npx-shim/Skills lanes) - Fallback path (silent audio loss) explicitly gated/removed: no — the
renderOrchestrator.ts:1870"video-only"log.warnfallback is unchanged; the fix removes the primary failure trigger, not the fallback itself - Version detection for FFmpeg: no explicit version probe; stderr-classification retry serves the same purpose without paying a probe-spawn cost
- Every ffmpeg call site uses the new path: yes —
-filter_complex_scriptused only inaudioMixer.ts; other producer sites use inline-filter_complexwhich is not deprecated
R1 by Via
jrusso1020
left a comment
There was a problem hiding this comment.
Stamped — verified before approving: CI green (40 pass) at 05fd570d, author miguel-heygen, no standing CHANGES_REQUESTED. Confirmed at source: two-attempt fallback tries -filter_complex_script first (broad compat with stable FFmpeg), retries -/filter_complex only when legacyFilterScriptOptionIsUnsupported(stderr) matches (filter_complex_script + unrecognized/not-found) — so unrelated ffmpeg errors don't burn a spurious retry. scriptDir cleanup moved into finally so it covers both attempt paths.
— Rames Jusso
What
Audio mixing now works on current FFmpeg nightlies without giving up compatibility with older stable installations or reintroducing Windows command-line overflow for large track counts.
Why
FFmpeg removed the deprecated
-filter_complex_scriptalias from nightly builds. HyperFrames therefore emitted a video-only fallback even though the same filter graph is valid through FFmpeg's replacement-/filter_complex <file>syntax. Older FFmpeg builds do not recognize that replacement, so switching unconditionally would break existing users.How
The mixer keeps the legacy file-valued option as its first attempt and retries with
-/filter_complexonly when stderr explicitly reports thatfilter_complex_scriptis unavailable. Both paths use the same temporary graph file, keeping 100+ track graphs off argv, and cleanup still runs after either attempt.Test plan
Unrecognized option 'filter_complex_script'on FFmpeg nightlyN-125551-/filter_complexsucceeds on the identical nightly graph-/filter_complex, exercising the backward-compatibility requirementbun run --cwd packages/engine test -- audioMixer.test.ts(12/12)bun run --cwd packages/engine typecheckbunx oxlintandbunx oxfmt --checkon changed files