Skip to content

fix(media): iPhone clips preview and export with their AAC voice - #5745

Open
miguel-heygen wants to merge 3 commits into
mainfrom
fix/proxy-picks-playable-audio-track
Open

miguel-heygen wants to merge 3 commits into
mainfrom
fix/proxy-picks-playable-audio-track

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 17, 2026 •

Copy link
Copy Markdown
Collaborator

What a user can do now

Import an iPhone-shaped video containing an unrecognized spatial-audio track before its AAC voice: preview proxies and exports retain the AAC audio. Waveforms, peak maps, loudness matching, CLI audio analysis and transcription select that same track.

Why

The media contains more than one audio stream. Implicit selection or always reading 0:a:0 can choose the unrecognized stream and fail to decode, even when an AAC voice is available.

The fix

playableAudioTrack in core owns the policy: select the first audio-relative stream whose codec name is present and not unknown.

  • Preview: metadata supplies the selected track; the proxy explicitly maps video and that audio. An unknown media kind or failed probe preserves automatic selection. This keeps audio for a clip.mov symlink whose canonical target has no extension.
  • Export: metadata, decoded-duration probing, constant-speed extraction and speed-ramp filter graphs all use the selected audio-relative index.
  • Analysis: peak maps use the selected track's channel count; waveform, loudness, CLI decode and transcription map the same track.
  • Cancellation: transcription's preflight receives its caller's signal and a 10-second timeout. Cancellation rethrows before a fallback duration probe or conversion can start.
  • Caches: proxy recipe v6, waveform v3, and peak-map peaks-v3 rebuild cached results made under the previous selection policy.

For media with multiple named audio codecs, the first now wins. This is a named-codec selection rule, not a guarantee that every installed decoder supports every named codec.

Tests

The synthetic media fixture is generated with native ffmpeg: a four-channel PCM sample entry is retagged as apac, followed by an AAC sine. No user media or binary fixture is added.

Three serial runs of each file on Linux, every run exit 0 with no skips:

Package File Passed per run
core audibleVideo.test.ts 13
studio-server mediaMetadata.test.ts 9
studio-server proxyTranscoder.test.ts 47
studio-server peakMap.test.ts 7
studio-server waveform.test.ts 6
studio-server loudness.test.ts 36
cli decode.test.ts 1
cli transcribe.test.ts 59
cli transcribe.signal.test.ts 3
engine audioMixer.level.test.ts 4
  • Native proxy tests assert H.264 + AAC output. Native export tests check audible sine output at constant speed and through a rate lane.
  • Removing only the cancellation rethrow produces exit 1, 1 failed / 2 passed: the fake probe log contains format=duration after cancellation. Restoring it passes.
  • Removing only the media-kind condition produces exit 1: the extensionless-target proxy contains H.264 without AAC. Restoring it passes.
  • Neighbor checks passed: ffprobe 119, mixer order 3, mixer 72, chunk encoder 103, peak decode failure 1, server child-process flags 2, CLI init child-process flags 1; each file exit 0.
  • Core, studio-server, engine and CLI typechecks passed. Formatting, lint, comment citations and comment ratchet passed.
  • The latest rebase changes none of the seven existing commit patches, verified with git range-diff.

Limits

Verification used generated MOV fixtures and native Linux ffmpeg, not a newly recorded physical iPhone clip or a physical audio device. Standalone media scripts under skills/ retain their existing track selection. The shared Studio metadata helper only identifies tracks for recognized video paths; audio-only waveform/loudness fallback behavior is unchanged.

@github-actions

github-actions Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 271 (base branch 271), smooth 157 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

@terencecho terencecho 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.

Reviewed head d3a9147e6e10113c81f17ad611022d39c2905a0d against effective merge-base 4e6ee6d2300290b638042ec88ef5afb7a0c27408 (API base: eab6ca15b1af7ec7bca0bbeb4cd0330e677467a5).

The shared audio-relative selector is a useful consolidation, and the engine carries the selected stream through both metadata/decoded-duration probing and constant/ramp extraction rather than choosing independently at each step (packages/engine/src/utils/ffprobe.ts:950–1104, packages/engine/src/services/audioMixer.ts:115–172). One introduced cancellation regression needs fixing; see the inline P2 at packages/cli/src/whisper/transcribe.ts:338–339.

Audited: All 24 existing changed effective-base source versions in full before reading all 26 changed head versions in full, plus every effective diff and the four intermediate commit patch collections. Traced the real public transcription signal/preparation chain, Studio metadata-to-timeline insertion and write callback, preview serving/cache boundary, CLI analysis/beats consumers, and selected engine/producer integration points. Assessed all 55 unordered interactions across the 11 behavioral groups: selector, Studio metadata/audible drop, proxy, engine metadata/duration, constant extraction, ramp extraction, peaks, waveform, loudness, CLI analysis, and transcription. The last two commits remove comments and repair the init test's partial child-process mock respectively; neither repairs the retained cancellation gap. No prior public blocking review to carry forward.

Trusting / unverified: Source-only review; no repository/dependency execution, tests, native FFmpeg/media/browser runs, or CI/check/log queries. Actual iPhone playback, decoder availability, browser default-stream semantics, and the reported native/CI results remain unverified. Additional integration files were read at the specific relevant ranges, not as a whole-repository audit. Unchanged cache-version questions were not promoted without a concrete valid old-content/new-content witness.

— Review by tai (pr-review)

Verdict: REQUEST CHANGES
Reasoning: Track identity is propagated coherently through the audited media paths, but the newly inserted transcription probe sits outside preparation's existing cancellation and timeout ownership. An abort while that probe is pending cannot settle the request until the unbounded probe finishes.

Comment thread packages/cli/src/whisper/transcribe.ts Outdated
Comment on lines +338 to +339
async function audioTrackMap(file: string): Promise<string[]> {
const stream = await probeAudio(file).catch(() => null);

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.

[P2] Keep the new track probe inside the preparation cancellation boundary

Both extractAudio and prepareAudio now await this helper before reaching runFfmpeg. Its probeAudio call uses promisified execFile with neither a signal nor a timeout (audio/analysis/decode.ts:23–39). For a real runTranscribe({ signal, ... }) call, if the caller aborts while that new ffprobe child is still pending, preparation and the public API remain awaiting it: neither owns an independent abort race, and the cancellable conversion has not started. Previously this preflight did not exist; preparation entered the existing signal-owned, timeout-capped conversion after its bounded synchronous setup. This therefore adds an unbounded non-cancellable phase to an API that explicitly promises cancellation during audio preparation (api/transcribe.ts:81–86). Please forward the preparation signal and a finite probe deadline, and propagate cancellation rather than swallowing it as ordinary probe fallback. This is established from the source/control flow, not an executed native-hang reproduction.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 452794f. probeAudio now takes the preparation's signal and a 10 s timeout (forwarded to execFile), and audioTrackMap rethrows when the signal aborted instead of falling back, so an abort during the probe rejects prepareWav with the abort reason. A probe that only times out still falls back to ffmpeg's own track pick.

Regression test: transcribe.signal.test.ts "stops on abort while the audio track probe is still running" points ffprobe at a script that never answers, aborts while it runs, and expects the abort reason. With this commit's two source changes reverted it times out (20 s); with them it passes, three runs in a row, alongside the other transcribe, decode and analysis tests (91 of 91). CLI typecheck and oxlint clean.

@vanceingalls vanceingalls 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.

R2 at 452794f70b02fc775829e49890801bd0f48509cb. The Oct 17 cancellation finding is resolved. One regression in the preview proxy still blocks.

Oct 17 CR (tai, review 5557830756): resolved

  • packages/cli/src/whisper/transcribe.ts:338-344: audioTrackMap passes the preparation signal and a 10 s timeout to probeAudio (audio/analysis/decode.ts:23-42). An abort rethrows the reason rather than falling back to ffmpeg's own pick. Both extractAudio and prepareAudio pass the signal; sherpa.ts:449 reaches it through prepareWav.
  • transcribe.signal.test.ts "stops on abort while the audio track probe is still running", which I ran:
    • At head it passes, and no sleep 60 probe child is left afterwards.
    • With both source files reverted to d3a9147e, it times out at 20 s and leaves the sleep 60 child running.
    • Reverting only transcribe.ts, or only decode.ts, also times out.
  • Calls with no signal now have a 10 s limit.

Original iPhone fix: re-verified

  • I ran the new real-ffmpeg tests with CI's pinned static ffmpeg (n8.0.1, sha256-checked): cli decode/transcribe/signal 71/71, studio-server proxy/mediaMetadata/peakMap/waveform/loudness 101/101, engine audioMixer.level 4/4, core audibleVideo 13/13.
  • Spot checks of failing first, against the merge-base: proxyTranscoder.ts at base fails "proxies an iPhone clip with its AAC voice…", and transcribe.ts at base fails "extracts an iPhone clip's AAC voice…".
  • Required CI at this head: Build, Render on windows-latest, Semantic PR title, Test, Test: runtime contract, Tests on windows-latest, Typecheck and regression are all green.

Blocker: a symlinked video with an extensionless target loses its audio in the preview proxy
This confirms the P2 tai found at this head but could not post.

  • The preview gate decides on the requested name (routes/preview.ts:661: getMimeType(subPath) must be video/*).
  • The transcode probes the canonical path (proxyTranscoder.ts:157,165 → runFfmpeg(source.sourcePath) → probeMediaMetadata(sourcePath) at :423).
  • For a target without a video extension, probeMediaMetadata returns { kind: "unknown" } (mediaMetadata.ts:207-210) with no probeError and no audioTrack. streamMaps (proxyTranscoder.ts:411-415) reads that as "a video with no playable audio" and emits only -map 0:V:0?.
  • The proxy code supports symlinked sources on purpose (:167-168, the cache-identity comment for external targets). Extensionless targets are what content-addressed stores produce, such as DVC's cache or git-annex's non-E backends.
  • Reproduced with a real h264+aac file at <proj>/blob and <proj>/clip.mov -> blob, calling resolveProxy(dir, clip.mov, "h264"):
    base 4e6ee6d2: SOURCE=video,audio  PROXY=video,audio
    head 452794f7: SOURCE=video,audio  PROXY=video
    
  • On main this preview has sound; at this head it is silent, and nothing flags the change. Waveform and loudness do not regress, because they fall back when audioTrack is undefined. Only the proxy reads "undefined" as "silent".
  • Fix: only map when the probe actually described a video, e.g. if (metadata.probeError || metadata.kind !== "video") return [];. Alternatively, have probeMediaMetadata probe the content instead of inferring the kind from the canonical extension. Please add a test for a symlink whose target is extensionless.

Important: bump PROXY_PARAMS_VERSION

  • proxyTranscoder.ts:27 is still "v5", the same as at the merge-base, but the recipe changed.
  • A proxy already cached for a clip with two decodable audio tracks keeps ffmpeg's old most-channels pick. The export now uses the first playable track (audioMixer), so preview and export disagree until the source's mtime or size changes.
  • Before this PR both used ffmpeg's default, so they matched. A version bump ("v6") invalidates those caches cleanly. iPhone clips are unaffected: their old transcodes failed, so nothing was cached.

Nit

  • Removing only the signal?.throwIfAborted() line in audioTrackMap still passes the new test: the already-aborted signal makes runFfmpeg's stoppableExecFile throw before spawning.
  • The rethrow still matters: without it, getMediaDurationSeconds runs a synchronous ffprobe after the abort. The test just does not pin it. An assertion that ffprobe ran once, or that no duration probe ran, would cover it.

Verdict: REQUEST CHANGES
Reasoning: The cancellation CR is fixed and its test fails first. The iPhone fix holds under real ffmpeg, and CI is green. The proxy's new stream maps silently drop audio for a symlinked source with an extensionless target, which plays with sound on main. The fix is one line plus a test; with that and the cache bump, this is approvable.

— Via

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Thanks, all three addressed at f083701b:

  • Blocker, symlinked clip with an extensionless target: streamMaps now returns no maps unless the probe described a video (metadata.probeError || metadata.kind !== "video"). An unknown kind keeps ffmpeg's own pick, as on main. New real-ffmpeg test "keeps the sound of a symlinked clip whose target has no extension": an h264+aac file written as blob, and clip.mov -> blob. The proxy has h264,aac. With the guard reverted to probeError only, it fails (video only).
  • PROXY_PARAMS_VERSION → v6, so copies cached under the old most-channels pick are rebuilt and match the export.
  • Nit: the fake ffprobe in transcribe.signal.test.ts now logs its arguments, and the test asserts no format=duration probe ran after the abort. With the signal?.throwIfAborted() line removed, it fails: the duration probe runs and the test times out.

Linux, with real ffmpeg: studio-server proxyTranscoder, mediaMetadata, peakMap, waveform (66) and cli transcribe.signal, transcribe, decode (71) pass three runs in a row. The symlink test ran, not skipped. Typecheck for both packages, oxlint, the comment ratchet and fallow are clean.

@terencecho terencecho 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.

Source re-review of f083701b2c0390ea87fe5b06a02096d059a77d74 against effective BASE 4e6ee6d2300290b638042ec88ef5afb7a0c27408 (API BASE eab6ca15b1af7ec7bca0bbeb4cd0330e677467a5). The previously raised repairs are present; I cannot give a confident approval or prove a new blocker within this review's evidence boundary.

Verified repairs:

  • The transcription preflight forwards the preparation signal and a 10-second timeout, and rethrows cancellation before ordinary fallback (packages/cli/src/whisper/transcribe.ts:338–344, packages/cli/src/audio/analysis/decode.ts:23–42). This source-addresses my earlier cancellation finding; it is not a claim about native descendant settlement.
  • streamMaps now leaves automatic selection enabled unless metadata actually describes a video (packages/studio-server/src/helpers/proxyTranscoder.ts:411–415), addressing the dropped-audio case in Via's review. The new extensionless-target symlink test uses the real metadata helper (proxyTranscoder.test.ts:517–557); I read it, not ran it. Proxy recipe v6 changes both disk keys and the imported preview ETag salt (proxyTranscoder.ts:27,176–187; helpers/mediaProxyPreview.ts:66–68). I am not withdrawing another reviewer's review.

Unresolved compatibility question — not a confirmed finding: Waveform decoding now explicitly selects the named-codec audio track (packages/studio-server/src/helpers/waveform.ts:62–71), but its cache remains v2, keyed by path/size/mtime (:18,34–41). The real route returns a valid warm cached vector before running the new decoder (packages/studio-server/src/routes/waveform.ts:30–43), and generateWaveformCache also skips that warm key (helpers/waveform.ts:106–115). For a canonical recognized-video input whose current proxy/export paths both select first track A, if the previous automatic waveform recipe selected later decodable track B with a different temporal envelope, an old B waveform would survive while current playback/export use A. Normalizing peaks cannot erase a difference in when their peaks occur. I verified the unchanged-key/early-return control flow, but did not independently establish the actual configured selection/output premise or a valid differing BASE waveform. A hand-transcribed cache model is not that proof. The precise open question is whether a supported input produces differing old/new waveform content under the configured selection rules; authoritative pinned selection semantics plus a concrete supported witness could establish it without runtime execution. An unchanged version alone is not a proved blocker. The original supporting-source budget is exhausted, so I did not acquire new dependency source to close that question.

Audited: All 25 existing changed BASE versions in full before current changed HEAD semantics, and all 27 changed HEAD versions in full: 49 previously read exact-blob versions were identity-checked at this pin; three were freshly read in full. Complete trees/changed-leaf union, current effective diff, full public history and canonical rubric were checked. Used a 55-pair cross-surface checklist covering selector, Studio metadata, proxy, engine metadata/duration, constant extraction, ramp extraction, peaks, waveform, loudness, CLI analysis and transcription; this is not proof of every higher-order interaction. Separate source reviewers agreed on the repairs; the code-adversarial lane had full-production/partial-test coverage, omitting two large HEAD test files. Their divergence on whether the conditional cache witness proves a defect led me to re-judge the whole review, not promote the missing proof into a P2. Post-compact parent-state metadata was exposed to both lanes, so I do not claim perfectly blind isolation.

Trusting / unverified: Source-only: no repository/dependency execution, tests, native FFmpeg/media/browser runs or CI queries/triggers. Author/peer native and CI reports are not my results. Named-codec selection is a heuristic, not universal decoder availability. Canonical unknown-kind proxy fallback does not prove first-track parity. Supporting files were read at their recorded scope, not as a whole-repository caller audit; native child/descendant settlement remains unverified. I did not establish the PR's separate claim that every render pad-trim/plan-parity/regression-harness caller accepts only renderer-produced files; that scope limit is not a new finding. No merge/build/release/deployment action.

— Review by tai (pr-review)

Verdict: COMMENT — incomplete; no approval or new request-changes finding.
Reasoning: The cancellation, extensionless-target audio-loss and proxy-cache repairs are source-addressed. The separate waveform-cache compatibility question has an unverified selection/output premise, so I am neither presenting it as a proved defect nor claiming the evidence supports an unconditional approval.

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

On the waveform-cache question: rather than settle the premise, I closed it at cdc4807f. WAVEFORM_CACHE_VERSION goes v2 → v3, and PEAK_MAP_CACHE_VERSION goes peaks-v2 → peaks-v3. Both helpers switched to the playable-track choice in this PR, the same reason the proxy recipe went to v6. A clip with two decodable tracks whose old cached waveform or peak map came from ffmpeg's default pick is now rebuilt from the track the preview and export play. Loudness keeps no cache. The peak-map key test is updated. Waveform and peak-map tests pass three runs in a row on Linux, and studio-server typecheck and the comment ratchet are clean.

@miguel-heygen
miguel-heygen force-pushed the fix/proxy-picks-playable-audio-track branch from cdc4807 to bfe228c Compare October 22, 2026 18:18
@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main at bfe228c. git range-diff confirms that all seven existing commits are patch-identical after this rebase.

The requested repairs remain present and were re-verified:

  • Removing only signal?.throwIfAborted() fails transcribe.signal.test.ts with exit 1, 1 failed / 2 passed. The assertion catches format=duration in the probe log after cancellation, rather than relying on a conversion failure or test timeout.
  • Removing only metadata.kind !== "video" fails the real symlink test: the extensionless target's proxy contains video without AAC. With the guard restored it contains both.
  • Proxy, waveform and peak-map recipe versions are v6, v3, and peaks-v3.

All ten selected audio test files passed three serial Linux runs, 185 tests per round, each file exit 0 and no skips. This includes native ffmpeg proxy, AAC decode, waveform, loudness, transcription and normal/ramped export paths. Neighbor checks add 301 passing tests. Core, studio-server, engine and CLI typechecks, lint, format, comment citations and comment ratchet pass.

An initial ffprobe neighbor run had 4 PNG failures because the fresh checkout held an LFS pointer. Restoring the exact repository fixture, checked against its recorded SHA-256, produced 119/119 passing tests. No source or expectation was changed to resolve that environment gap.

The PR body now separates the named-codec heuristic from actual decoder support and removes unverified claims about browser stream selection and audio-only container contents. No new physical-iPhone or audio-device audition is claimed. Ready for a fresh review at this head.

@miguel-heygen
miguel-heygen force-pushed the fix/proxy-picks-playable-audio-track branch from bfe228c to 72da22e Compare October 22, 2026 22:24
@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main at 72da22e348 (patch-identical, no conflicts). Every earlier finding stays addressed: the cancellation race, the extensionless-symlink audio drop, and the waveform/peak-map cache versions. On the rebased tree all 16 changed test files pass three serial runs (cli 64, core 13, engine 184, studio-server 108), with workspace packages built first. Ready for re-review.

An iPhone video carries a spatial-audio track ffmpeg cannot decode next to its AAC track. Preview proxies, waveforms, peak maps, loudness, audio extraction and transcription now use the first audio track ffprobe can name, not ffmpeg's own pick (most channels), which failed with 'Error opening output files: Invalid argument'.
@miguel-heygen
miguel-heygen force-pushed the fix/proxy-picks-playable-audio-track branch from 72da22e to 75b8426 Compare October 25, 2026 23:17

@heygengenesis heygengenesis Bot 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.

Verdict: REQUEST CHANGES · 1 blocker · 0 important · 0 nits · head e711c128

  • blocker: the rebase lost the proxy cache bump. PROXY_PARAMS_VERSION (packages/studio-server/src/helpers/proxyTranscoder.ts:38) is "v6" on both main and this head. Main got to "v6" separately in ada9932 (#6307, Oct 25), so this PR's streamMaps recipe change no longer changes the disk key or the preview ETag salt (mediaProxyPreview.ts:68). Failure: take a clip with two decodable audio tracks whose v6 proxy main already cached with ffmpeg's default pick (most channels). After merge, preview keeps serving that stale proxy, while export uses playableAudioTrack (first named codec). Preview and export then play different audio until the source's mtime or size changes. @vanceingalls raised this at 452794f as a condition for approval, and the PR body still says "proxy recipe v6… rebuild cached results". Fix: change it to "v7".
Scope

Read the full source diff against main@6f4d46ad. Still fixed at this head: the transcription cancel and timeout (@terencecho), the extensionless symlink → auto-select (streamMaps kind !== "video"), waveform v3 and peaks-v3 (both still v2 on main, so those bumps are real). Engine metadata, decoded duration, constant-speed and ramp extraction all share one audioTrack. CI on this head passes, all 8 workflows. Apart from the version string, I found nothing else blocking.

This branch has not been deployed

No deployments
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.

3 participants