Repository navigation
fix(media): iPhone clips preview and export with their AAC voice - #5745
miguel-heygen wants to merge 3 commits into
Conversation
Edit accuracy: accurate 271 (base branch 271), smooth 157 of thoseThe gate passes. Quarantined, measured but not gated (0) |
terencecho
left a comment
There was a problem hiding this comment.
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.
| async function audioTrackMap(file: string): Promise<string[]> { | ||
| const stream = await probeAudio(file).catch(() => null); |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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:audioTrackMappasses the preparation signal and a 10 s timeout toprobeAudio(audio/analysis/decode.ts:23-42). An abort rethrows the reason rather than falling back to ffmpeg's own pick. BothextractAudioandprepareAudiopass the signal;sherpa.ts:449reaches it throughprepareWav.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 60probe child is left afterwards. - With both source files reverted to
d3a9147e, it times out at 20 s and leaves thesleep 60child running. - Reverting only
transcribe.ts, or onlydecode.ts, also times out.
- At head it passes, and no
- 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.level4/4, coreaudibleVideo13/13. - Spot checks of failing first, against the merge-base:
proxyTranscoder.tsat base fails "proxies an iPhone clip with its AAC voice…", andtranscribe.tsat 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 bevideo/*). - The transcode probes the canonical path (
proxyTranscoder.ts:157,165→runFfmpeg(source.sourcePath)→probeMediaMetadata(sourcePath)at:423). - For a target without a video extension,
probeMediaMetadatareturns{ kind: "unknown" }(mediaMetadata.ts:207-210) with noprobeErrorand noaudioTrack.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>/bloband<proj>/clip.mov -> blob, callingresolveProxy(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
audioTrackis 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, haveprobeMediaMetadataprobe 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:27is 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 inaudioTrackMapstill passes the new test: the already-aborted signal makesrunFfmpeg'sstoppableExecFilethrow before spawning. - The rethrow still matters: without it,
getMediaDurationSecondsruns 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
|
Thanks, all three addressed at
Linux, with real ffmpeg: studio-server |
terencecho
left a comment
There was a problem hiding this comment.
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. streamMapsnow 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 recipev6changes 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.
|
On the waveform-cache question: rather than settle the premise, I closed it at |
cdc4807 to
bfe228c
Compare
|
Rebased onto current main at bfe228c. The requested repairs remain present and were re-verified:
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. |
bfe228c to
72da22e
Compare
|
Rebased onto current main at |
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'.
72da22e to
75b8426
Compare
There was a problem hiding this comment.
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'sstreamMapsrecipe 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 usesplayableAudioTrack(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 recipev6… 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.
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:0can choose the unrecognized stream and fail to decode, even when an AAC voice is available.The fix
playableAudioTrackin core owns the policy: select the first audio-relative stream whose codec name is present and notunknown.clip.movsymlink whose canonical target has no extension.v6, waveformv3, and peak-mappeaks-v3rebuild 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:
audibleVideo.test.tsmediaMetadata.test.tsproxyTranscoder.test.tspeakMap.test.tswaveform.test.tsloudness.test.tsdecode.test.tstranscribe.test.tstranscribe.signal.test.tsaudioMixer.level.test.tsformat=durationafter cancellation. Restoring it passes.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.