fix(export): mix all source audio tracks so the mic isn't dropped - #109
Conversation
Native macOS recordings write system audio and the microphone as two separate AAC tracks in the screen recording, and both are flagged `default`. The exporter decoded audio through web-demuxer's bare "audio" selector, which resolves to the single stream FFmpeg's `av_find_best_stream` picks — the first (system-audio) track. When nothing was playing, that track is silent, so the exported video had no audible audio even though the mic was recorded fine. The browser recorder already blends system + mic into one track; the native path never did. Decode every audio stream and mix them into one timeline before encoding, mirroring the browser recorder: - `mixPlanarSources` (pure, unit-tested) sums each decoded source, downmixed to the target channels and aligned at its source-time offset, clamped to [-1, 1]. - Per-stream decode via `readAVPacket(streamIndex)` targets each track by container index instead of the best-stream heuristic. - The trim-only and offline (speed) export paths both mix multi-track sources; multi-track speed projects are routed to the offline path because the real-time <audio> capture can only play one track. - The source-copy fast path is disabled for multi-track sources, since copying verbatim carries both tracks over and players fall back to the silent first one. Single-track recordings keep the original fast path unchanged. Adds a real-browser regression test (fixture: silent track + 440 Hz tone) that asserts the exported audio carries the tone through both mixing paths. Closes getopenscreen#108 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughMulti-track audio export now decodes all audio streams, aligns and mixes their planar PCM, supports trim and speed-region paths, and disables source-copy export when mixing is required. Unit and browser tests validate mixed output and audible exports. ChangesMulti-track audio export
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant VideoExporter
participant AudioProcessor
participant WebDemuxer
participant mixPlanarSources
VideoExporter->>AudioProcessor: process source audio
AudioProcessor->>WebDemuxer: enumerate and decode audio streams
WebDemuxer-->>AudioProcessor: planar PCM with timeline offsets
AudioProcessor->>mixPlanarSources: align and mix streams
mixPlanarSources-->>AudioProcessor: mixed planar timeline
AudioProcessor-->>VideoExporter: encoded export audio
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/exporter/audioEncoder.ts`:
- Around line 330-459: Replace the retained AudioData[] and full-track
planar-array workflow in decodeAudioStreamToPlanes and decodeMixedAudioPlanes
with bounded-window processing that decodes, mixes, and forwards audio
incrementally to the encoder/WSOLA pipeline. Avoid accumulating complete tracks
or duplicating samples in memory; preserve stream timeline offsets, channel
handling, cancellation, and sample-rate validation while ensuring long
recordings use bounded memory.
- Around line 437-439: Update mixPlanarSources() to return the sole decodable
source instead of null when sources.length is 1, while preserving the null
result for zero sources. Ensure the related caller paths around the
source-selection logic also retain and use this single-source result rather than
falling back to the demuxer’s best stream.
- Around line 388-399: Update the frame-alignment logic around startFrame in the
audio encoding flow to preserve the signed source timestamp; remove the clamp
that forces startFrame to zero. Keep mixPlanarSources() responsible for
discarding samples before frame zero so AAC preroll and timestamp-zero frames
retain their correct relative offsets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 72ae16b3-89b6-4900-95d1-2691e7a3bdba
⛔ Files ignored due to path filters (1)
tests/fixtures/sample-dual-audio.mp4is excluded by!**/*.mp4
📒 Files selected for processing (6)
.gitignoresrc/lib/exporter/audioEncoder.test.tssrc/lib/exporter/audioEncoder.tssrc/lib/exporter/audioMixExport.browser.test.tssrc/lib/exporter/streamingDecoder.tssrc/lib/exporter/videoExporter.ts
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
Review — REQUEST CHANGES
Substantial fix: enumerates every audio stream (instead of av_find_best_stream's first match), sums per-track PCM into a single timeline, hard-clamps to [-1, 1], and re-encodes. Touches all three export paths and adds a audioStreamCount blocker to the source-copy fast path so multi-track sources no longer silently bypass the mixer. Unit-test coverage on mixPlanarSources is exemplary (silent-track recovery pins the #108 regression).
A few gaps remain — most importantly, this conflicts with PR #86 which fixes the same issue with a different (and partially complementary) shape. See "Cross-PR" below; the recommendation is to land this PR and port PR #86's two unique features over as follow-ups rather than merging both.
Open issues
- Pitch-preserved path (speed ≤ 16x) still drops the mic.
renderPitchPreservedTimelineAudio(line ~791) creates<audio>+createMediaElementSource+createMediaStreamDestination, but never callsenableAllMediaAudioTracks(media). A multi-track project with ≤ 16× speed edits therefore still exports silent audio on this path. PR #86 handles this — please port. - Cross-rate streams fall back to silent.
decodeMixedAudioPlanescheckssources.some((s) => s.sampleRate !== sampleRate)at line 442 and returnsnull, taking the single-stream path which picks the default track. The author's note says "never the case for native captures, which are all 48 kHz" — true for the current repro, but at minimum log a per-tracksample_ratefor diagnosis, or port PR #86's linear resampler. - Browser-test threshold too loose.
expect(rms).toBeGreaterThan(0.01)at line 62 will pass on noise floor. PR #86 usespeak > 0.1; recommend tightening. - No test pins the source-copy fast-path blocker. The
audioStreamCount > 1check is added but not exercised invideoExporter.test.ts. Add a regression.
Cross-PR comparison vs #86
Both modify processTrimOnlyAudio and conflict on overlapping hunks. #109 is the broader superset: it covers trim-only, routes every multi-track speed export through the offline mixer, blocks source-copy, and adds WSOLA support. #86 uniquely handles the pitch-preserved path via enableAllMediaAudioTracks and adds linear resampling for cross-rate sources. Recommendation: merge #109, then port those two features as follow-ups.
Smaller notes (inline)
Naive hard-clip instead of gain reduction — fine for a screen recorder, but document the trade-off. Memory: a 10-minute multi-track holds ~230 MB Float32 at once. The new comments are good but quite dense; AGENTS.md says "no comments unless asked" — please confirm with maintainers.
|
@EtienneLescot I didn't see that other PR otherwise I would have just used that one. I'm no great Javascript developer so my PR was all Claude. Do you want to be make fixes to my PR or close in favour of #86 ? |
|
@barnaclebarnes It seems like your PR still has value. The only thing is that it should be merged after the other one because it is broader (even if yours was made before) |
- Preserve the signed startFrame so AAC preroll (negative timestamp) no longer collides with the timestamp-zero frame; mixPlanarSources already discards pre-zero frames. - Keep the sole decodable stream instead of returning null, so a partial decode failure can't fall back to the demuxer's best stream and export silence. - Log per-track sample rates when a cross-rate mix is skipped. - Document the hard-clip trade-off in mixPlanarSources. - Tests: pin the source-copy audioStreamCount>1 blocker, cover the signed startFrame case, and tighten the browser RMS thresholds (0.01 -> 0.05). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Plan / sequencing: this PR is the broader superset (trim-only mixing, offline WSOLA path, source-copy Pushed |
…tion Compares origin/main against origin/feat/ai-edition directly from their divergence point (68d3a68, 2026-07-15), using the descriptions of the 15 PRs merged into main since, rather than a commit-level diff. Two findings drove the spec: - main produced no new product feature since the divergence, only fixes and release tooling -- but five of those fixes cover bugs still live in ai-edition (black video on odd-dimension window capture, recording that never stops under the software encoder, Wayland capture, HUD drag drift). - ancestry is not wiring. Several capabilities exist in ai-edition's code but were never connected to the V4 shell (speed capped at 3x while 100x is implemented, no zoom auto/manual toggle, only text annotations creatable). The audit also relocates PR #109: the "mic dropped from the export" bug is alive in our native path (poc-d3d/src/audio.rs picks a single audio stream via av_find_best_stream), while main's fix lives in the browser exporter the app no longer uses -- so it must be reimplemented in Rust, not cherry-picked.
…survives The native macOS recorder writes system audio and the microphone as two separate AAC tracks, both flagged default. `decode_clip_audio` picked a single stream through `av_find_best_stream`, which returns the first one -- silent whenever nothing was playing through the system. The microphone was therefore dropped from every export of such a recording. This is issue #108. PR #109 fixed it in the browser exporter, but ExportDialog now goes through exportMultiNative/exportNative, so that fix never runs: the defect is alive in the Rust path, which is the only one the app uses. Hence a reimplementation here rather than a cherry-pick. - Enumerate every audio stream instead of asking for the "best" one, and run one decoder + resampler per track off a single demux pass, routing each packet by stream index. One seek only -- it repositions the whole container -- calibrated on the first audio track, then all decoders are flushed. - Mix by summing. Each track is already recropped onto the same source window (zero-padded at the front when its first decoded packet lands after the window start, prefetch trimmed when the seek fell back to an earlier frame), so inter-track start offsets are absorbed by that alignment and the mix itself needs nothing more than an add. Extracted as `mix_aligned_tracks` so the logic is testable without ffmpeg. - Clamp to [-1, 1] only when more than one track is present, which keeps single-track sources behaving exactly as before. - An audio track that cannot be opened is skipped rather than fatal: before this change only one track was ever opened, so an exotic extra track could not break an export, and it still cannot. But if audio streams exist and none is decodable we now fail loudly instead of silently exporting mute -- silent audio loss is the very defect being fixed. `AudioTrackDecoder` frees its codec context on Drop, so the `?` paths no longer leak one context per track. Verification: cargo check clean, cargo test --lib 25/25 pass, including 7 new tests -- among them the #108 case itself (silent first track + microphone second yields the microphone), single-track passthrough, the no-clamp promise, and both alignment directions. Not yet verified end-to-end on a real multi-track macOS recording; that needs such a file to export.
2 participants
Problem
Native macOS recordings write system audio and the microphone as two separate AAC tracks in the screen recording, both flagged
default. The exporter decoded audio through web-demuxer's bare"audio"selector, which resolves to the single stream FFmpeg'sav_find_best_streampicks — the first (system-audio) track. When nothing was playing through the system, that track is silent, so the exported video had no audible audio even though the microphone was recorded fine.The browser/MediaRecorder path already blends system + mic into one track (
audioMix.ts); the native macOS path never did, and the exporter assumed a single audio track.Repro (from a real affected recording): the second track holds the voice, but
av_find_best_streamselects the silent first track:Fix
Decode every audio stream and mix them into one timeline before encoding, mirroring the browser recorder:
mixPlanarSources(pure, unit-tested) — sums each decoded source, downmixed to the target channels and aligned at its source-time offset, clamped to[-1, 1].readAVPacket(streamIndex)targets each track by container index instead of the best-stream heuristic.<audio>capture can only play one track.Testing
mixPlanarSourcesunit tests (sum, silent-track recovery, start-offset alignment, mono→stereo upmix, clamping).tsc, and Biome.Closes #108
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests / Chores
.gitignorefor Vitest attachments.