fix(engine): write the audio mix filter graph to a file, not the command line#1890
Conversation
…and line mixAudioTracks built the ffmpeg -filter_complex argument as one inline string scaling linearly with track count. Reported in the wild at 146 timed audio clips: the resulting command line exceeded the OS length limit and spawn failed with ENAMETOOLONG, dropping audio entirely until the user manually consolidated clips to reduce the count. FFmpeg supports -filter_complex_script specifically for this - the same filter graph read from a file instead of inlined as an argument. The -i pairs for each track still scale with count but stay short and fixed-size each, so the one component that actually grew unbounded (the filter string) no longer sits on the command line at all. The temp file is cleaned up immediately after ffmpeg exits, matching the existing sibling temp-file convention in audioVolumeEnvelope.ts. Verified end-to-end against a real ffmpeg binary (not just mocked): a two-track mix produced correct output audio with no leftover temp files.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 23b0fdd.
🟢 Correct fix using the exact FFmpeg feature designed for this problem (-filter_complex_script <file> vs inline -filter_complex <string>). Sidesteps the argv length limit for the one component that actually grew unbounded (the filter graph); inputs (-i pairs) still scale linearly but each is short and fixed-size, and the args-array total stays well under 20KB even at 150 tracks (verified in the new regression test).
Verification notes:
- End-to-end verification against real ffmpeg (not just mocked) called out in the PR body — appreciated, this is the shape of verification I want to see on wire-format changes to a subprocess boundary.
- Temp-file lifecycle:
mkdtempSyncfor the script dir,openSync(..., "wx", 0o600)for exclusive create + owner-only perms,rmSync(scriptDir, { recursive: true, force: true })in.finally()covers both success and failure paths. Matches the sibling convention inaudioVolumeEnvelope.ts. - Test-mock retrofit is careful — the
capturedFilterScriptsside array is index-aligned with call order, and the twomockImplementationOnceoverrides in the automation-fallback test explicitly push placeholders to keep alignment. Non-obvious enough to warrant the comment; author added it.
Concerns:
- 🟡
scriptDiris created inoutputDir(=dirname(outputPath)). If a downstream render pipeline globsoutputDirfor output artifacts (e.g.ls outputDir/*.m4a, or a manifest-diff scan), the.filter-complex-*/temp dirs are transient enough that they only exist for the ffmpeg process lifetime — but if the process is force-killed mid-run (SIGKILL bypasses.finally()), you'll leak a.filter-complex-<random>/graph.txtsitting next to the output. Considertmpdir()instead ofoutputDiras the parent, or a cleanup sweep in the caller. Low-severity; the leak is a few KB per crash. - 🟡 The automation-fallback retry path (
runMix(true)) creates a SECOND script dir + file. Each retry writes a new temp; if the first mix hangs and gets timed out, both dirs exist briefly..finally()on each individually is correct, just noting the pattern for future readers.
Nits:
- 🟡
writeFileSync(fd, ...)afteropenSync(...)— thetry/finallycorrectlycloseSyncs the fd, but ifwriteFileSyncthrows (disk full, etc.), the empty file + dir persists briefly until process exit. The.finally()on the outerrunFfmpegchain handles this. Acceptable.
Full package test pass (841 tests, no regressions) + real-ffmpeg verification is strong evidence. Nothing blocking.
— Rames D Jusso
vanceingalls
left a comment
There was a problem hiding this comment.
🟢 R1 verdict — LGTM (runtime-interop lens)
Reported real-world ENAMETOOLONG at 146 tracks. Fix pushes the one unbounded component (the filter graph string) off argv and into a temp file, keeping the -i pairs (fixed-size each) on the command line. Sound minimal fix.
Finding-by-finding
1. Race safety on parallel runMix invocations — 🟢
packages/engine/src/services/audioMixer.ts:433-436
const scriptDir = mkdtempSync(join(outputDir, ".filter-complex-"));
const scriptPath = join(scriptDir, "graph.txt");
const fd = openSync(scriptPath, "wx", 0o600);Two things to like here after the second commit:
mkdtempSyncguarantees a unique per-mix directory, so two concurrentmixAudioTrackscalls sharing anoutputDirdon't stomp each other'sgraph.txt.openSync(..., "wx", 0o600)fails fast on any existing file and forbids other-user read of the graph content while it's on disk. Belt-and-braces on top of the unique dir; not necessary but not harmful.
The .finally(() => rmSync(scriptDir, { recursive: true, force: true })) cleans up on both the initial mix and the automation-degraded retry, and force: true means a missing dir isn't an error — safe against a signal-abort mid-mix that already deleted it. LGTM.
2. Downstream consumer sweep — 🟢
mixAudioTracks is called only from processCompositionAudio in the same file (verified via repo-wide code search). No external export, no shape change to worry about. Blast radius zero.
3. Reachability under prod defaults — 🟢
The new runMix path replaces the old inline-filter path unconditionally — every existing call site hits the new code. No conditional gate, no feature flag. Prod defaults use it.
4. Test integrity — 🟢 (out of lens but worth flagging positively)
The capturedFilterScripts side-array with an index-aligned placeholder pushed from the two mockImplementationOnce calls in the degraded-automation test (line ~114-126) is the right approach — the file is unlinked before Vitest's assertions run, so reading it after processCompositionAudio resolves would ENOENT. The mock captures it synchronously while it still exists on disk. Clean.
No blockers.
Review by Via (runtime-interop lens)
Summary
mixAudioTracksbuilt the ffmpeg-filter_complexargument as one inline string that scales linearly with track count. Reported in the wild at 146 timed audio clips: the resulting command line exceeded the OS argument-length limit andspawnfailed withENAMETOOLONG, dropping audio entirely until the user manually consolidated clips to reduce the count.Fix
FFmpeg supports
-filter_complex_script <file>specifically for this — the same filter graph read from a file instead of inlined as a command-line argument.-i <path>pairs for each track still scale with count, but each is short and fixed-size; the one component that actually grew unbounded (the filter graph string) is off the command line entirely.The temp script file is written right before the
runFfmpegcall and cleaned up immediately after it resolves (success or failure, via.finally()), matching the existing sibling temp-file convention already used inaudioVolumeEnvelope.ts.Test plan
bunx vitest run packages/engine/src/services/audioMixer.test.ts— 7 tests pass (6 existing, updated to read the filter graph from the mock's captured file content instead of the now-removed inline-filter_complexstring, + 1 new regression test)-filter_complex_script(not-filter_complex), and the captured filter script's content still contains a correctamix=inputs=150and oneatrim=segment per trackbunx vitest run packages/engine/— full package, 841 tests pass, no regressionsprocessCompositionAudiounmocked, confirmed correct output audio (right duration, right codec viaffprobe) and no leftover temp files after completion