Skip to content

Commit 3d943a8

Browse files
fix(export): address review feedback on multi-track audio mixing
- 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>
1 parent 9920be7 commit 3d943a8

4 files changed

Lines changed: 60 additions & 11 deletions

File tree

src/lib/exporter/audioEncoder.test.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,21 @@ describe("mixPlanarSources", () => {
132132
]);
133133
});
134134

135+
it("discards samples before frame zero from a negative startFrame (AAC preroll)", () => {
136+
// AAC preroll gives the first decoded frame a negative timestamp, so the source's
137+
// startFrame is signed. The preroll samples (timeline < 0) must be dropped while
138+
// the real timestamp-zero sample keeps its true offset.
139+
const withPreroll = {
140+
planes: [new Float32Array([0.9, 0.5, 0.5])],
141+
startFrame: -2,
142+
};
143+
144+
const mixed = mixPlanarSources([withPreroll], 1, 2);
145+
146+
// Frames -2 and -1 (0.9, 0.5) are discarded; frame 0 keeps the real sample.
147+
expect(Array.from(mixed[0])).toEqual([expect.closeTo(0.5, 5), expect.closeTo(0, 5)]);
148+
});
149+
135150
it("upmixes a mono source to stereo before mixing", () => {
136151
const mono = { planes: [new Float32Array([0.3, 0.3])], startFrame: 0 };
137152
const stereo = {

src/lib/exporter/audioEncoder.ts

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -153,8 +153,11 @@ export interface PlanarAudioSource {
153153
* into one track before encoding.
154154
*
155155
* Each source is first downmixed to `targetChannels`, then summed sample-aligned at
156-
* its `startFrame`. The result is clamped to [-1, 1] so a loud overlap can't wrap or
157-
* overflow the encoder. Sources of differing lengths and offsets are supported;
156+
* its `startFrame`. The result is hard-clamped to [-1, 1] so a loud overlap can't wrap
157+
* or overflow the encoder. Hard clipping (rather than a 1/N gain pre-sum or soft-knee
158+
* limiter) can add harmonic distortion when two sources are simultaneously near unity —
159+
* an acceptable trade-off for a screen recorder, where keeping both tracks at full level
160+
* matters more than headroom. Sources of differing lengths and offsets are supported;
158161
* samples past `totalFrames` are dropped.
159162
*/
160163
export function mixPlanarSources(
@@ -384,8 +387,11 @@ export class AudioProcessor {
384387
const numberOfChannels = decoded[0].numberOfChannels;
385388
// Absolute timeline position of this stream's first sample (mic tracks often
386389
// start slightly after the video, e.g. start_time ~0.17s), so mixing keeps
387-
// every track locked to the same source-time origin the video uses.
388-
const startFrame = Math.max(0, Math.round((decoded[0].timestamp / 1_000_000) * sampleRate));
390+
// every track locked to the same source-time origin the video uses. Kept signed:
391+
// AAC preroll carries a negative timestamp, and clamping it to zero would land the
392+
// preroll and the real timestamp-zero frame on the same offset. mixPlanarSources()
393+
// discards frames before zero, so the signed origin stays correct.
394+
const startFrame = Math.round((decoded[0].timestamp / 1_000_000) * sampleRate);
389395

390396
let totalFrames = 0;
391397
for (const d of decoded) {
@@ -435,12 +441,17 @@ export class AudioProcessor {
435441
}
436442

437443
if (sources.length === 0) return null;
438-
// A single decodable stream can't be "mixed" — fall back to the normal path.
439-
if (sources.length === 1) return null;
444+
// If only one of several streams decoded, keep it rather than falling back to the
445+
// demuxer's best stream — that fallback can re-select the stream that just failed
446+
// to decode and export silence. mixPlanarSources() handles a single source fine.
440447

441448
const sampleRate = sources[0].sampleRate;
442449
if (sources.some((s) => s.sampleRate !== sampleRate)) {
443-
console.warn("[AudioProcessor] Audio streams differ in sample rate; skipping mix");
450+
console.warn(
451+
`[AudioProcessor] Audio streams differ in sample rate; skipping mix (rates: ${sources
452+
.map((s) => s.sampleRate)
453+
.join(", ")}Hz)`,
454+
);
444455
return null;
445456
}
446457

src/lib/exporter/audioMixExport.browser.test.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,9 @@ describe("Multi-track audio export (real browser)", () => {
5757
const { rms, hasAudio } = await measureAudioRms(result.blob!);
5858
expect(hasAudio).toBe(true);
5959
// Silence (the first track alone) sits near 0; the mixed-in 440 Hz tone lifts
60-
// RMS well above the noise floor. A comfortable threshold below the tone's
61-
// real level (~0.08) but far above silence.
62-
expect(rms).toBeGreaterThan(0.01);
60+
// RMS to ~0.08. Threshold sits below the tone's real level but well above the
61+
// noise floor, so a near-silent fallback (the old #108 bug) still fails.
62+
expect(rms).toBeGreaterThan(0.05);
6363
});
6464

6565
it("mixes both audio tracks through the speed-region (offline) path too", async () => {
@@ -85,6 +85,6 @@ describe("Multi-track audio export (real browser)", () => {
8585

8686
const { rms, hasAudio } = await measureAudioRms(result.blob!);
8787
expect(hasAudio).toBe(true);
88-
expect(rms).toBeGreaterThan(0.01);
88+
expect(rms).toBeGreaterThan(0.05);
8989
});
9090
});

src/lib/exporter/videoExporter.test.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,29 @@ describe("getSourceCopyFastPathBlockers", () => {
129129
}),
130130
).toContain("output-size 1920x1080 differs from source 1920x1032");
131131
});
132+
133+
// A verbatim source copy would carry over every audio track; most players play only
134+
// the first, which on native macOS captures is often the silent system track — the
135+
// #108 bug. Multi-track sources must be blocked from the copy path so they get mixed.
136+
it("blocks the copy path when the source has multiple audio tracks (#108)", () => {
137+
expect(
138+
getSourceCopyFastPathBlockers(createConfig(), {
139+
width: 1920,
140+
height: 1080,
141+
audioStreamCount: 2,
142+
}),
143+
).toContain("source has multiple audio tracks (must be mixed)");
144+
});
145+
146+
it("does not report the audio-track blocker for a single-track source", () => {
147+
expect(
148+
getSourceCopyFastPathBlockers(createConfig(), {
149+
width: 1920,
150+
height: 1080,
151+
audioStreamCount: 1,
152+
}),
153+
).not.toContain("source has multiple audio tracks (must be mixed)");
154+
});
132155
});
133156

134157
// The original bug measured the timeout from the encoder's last *output* event

0 commit comments

Comments
 (0)