Repository navigation
fix(producer): mix muted browser media as silent for preview-render parity - #1969
Conversation
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
🟢 R1 verdict — the parity gap is closed cleanly; the fix survives cross-consumer scrutiny.
A most instructive little repair, this. The runtime hushes muted media the moment the DOM whispers muted, yet the mixer, in its previous incarnation, remained blissfully deaf to that hush and dutifully summed the phantom audio at full volume — a discrepancy audible in every side-by-side comparison between preview and render. The fix here does exactly what the invariant demands: it drops muted audio elements from composition.audios entirely and clears hasAudio on muted video, thereby preventing the auto-detected-video-audio path in appendAutoDetectedVideoAudio from resurrecting them. The extraction of the parity rule into a pure pruneMutedBrowserMedia helper is the correct choice — testable in isolation, no probe-session harness required, and the helper's contract closes both the update-existing and skip-new branches without gaps.
Finding-by-finding
1. Pruning ordering vs. discovery loop — the sequencing is correct ✅
packages/producer/src/services/render/stages/probeStage.ts:293-300
const existingVideoIds = new Set(composition.videos.map((v) => v.id));
const existingAudioIds = new Set(composition.audios.map((a) => a.id));
pruneMutedBrowserMedia(composition, browserMedia, existingAudioIds);
for (const el of browserMedia) {
if (!el.src || el.src === "about:blank") continue;
if (el.muted && el.tagName === "audio") continue;I traced every combination — pre-existing muted audio (spliced + set-purged, then skipped at 300), newly-discovered muted audio (helper no-op, then skipped at 300), pre-existing muted video (helper clears hasAudio, then update-branch at 334 refuses to relight it because of the new && !el.muted guard), newly-discovered muted video (helper no-op, then insertion at 350 sets hasAudio: el.hasAudio && !el.muted). Every path lands silent. No concern.
2. Downstream consumer appendAutoDetectedVideoAudio — the parity chain is complete ✅
packages/producer/src/services/render/stages/extractVideosStage.ts:242-262
for (const ext of extracted) {
if (!ext.metadata.hasAudio) continue;
const video = composition.videos.find((v) => v.id === ext.videoId);
if (!video || !video.hasAudio || existingAudioSrcs.has(video.src)) continue;
composition.audios.push({ ... });
}This is the linchpin. A muted <video> whose underlying file has an audio track would previously slip in through this door, because video.hasAudio was set from the HTML-compiler pass, not the runtime mute state. With probeStage now clearing hasAudio on muted videos (line 335 update path and line 350 insert path), the gate at line 250 stops them cold. The docstring on line 240 ("a muted <video> whose source contains audio should not leak into the render") was aspirational before this PR and is now enforced.
3. Muted detection covers both attribute and property ✅
packages/producer/src/services/htmlCompiler.ts:1773
const muted = htmlEl.hasAttribute("muted") || htmlEl.muted;Belt-and-braces, and both belts belong. hasAttribute("muted") catches the HTML declarative <video muted>, while htmlEl.muted catches the runtime property that JS may have set after load (e.g. React refs, imperative timeline scripts). Either alone would miss half the population. No concern.
4. volume: 0 is NOT treated as muted — intentional gap, worth noting 🟡
packages/producer/src/services/render/stages/probeStage.ts:539-558
The helper keys strictly on el.muted. A user who zeros the volume slider produces silence in preview but this fix does not prune such elements. In practice the mixer's own volume path handles this — existing.volume = el.volume at line 383 propagates the zero, and downstream mix should render silent — but the reliance is unstated. Not a blocker; the PR's scope is muted-attribute parity, and volume-zero parity is a separate contract. Consider a follow-up test asserting volume: 0 renders silent through the mixer, or a comment on the helper noting the boundary.
5. Dynamic mute (mid-composition toggle) — inherent snapshot limitation 🟡
packages/producer/src/services/htmlCompiler.ts:1743-1791
discoverMediaFromBrowser samples the DOM once at probe time. If a keyframe or scripted animation flips muted on/off during the composition timeline, only the snapshot state is captured. This is a pre-existing constraint of the discovery model (the same is true of src, volume, etc. absent explicit discoverAudioVolumeAutomationFromTimeline-style sampling). Out of scope for this PR; the fix meaningfully closes the static-muted case which is the dominant surface. Mention it in a doc comment if a downstream PR in the media-OS-rework stack (#1970-#1973) is expected to touch this path.
6. Test coverage — right shape, right level ✅
packages/producer/src/services/render/stages/mutedParity.test.ts:1-38
Two cases: mixed muted/unmuted, and the no-op case. Both assert the invariant the helper is responsible for (drop muted audio, clear video hasAudio, leave everything else alone). Because the helper is pure, this is the correct testing level — no probe-session harness needed. The lack of a full preview-render output diff is fine at this layer; that belongs to a higher-level parity fixture. Consider one more case: a muted <video> NOT yet in composition.videos (new-discovery path) — asserts the helper is genuinely no-op for that shape rather than accidentally silent.
Verdict: The runtime-interop lens finds no blocker. The invariant (preview silences muted media → the mix must too) is enforced along every reachable code path, the downstream appendAutoDetectedVideoAudio gate is now honestly enforceable rather than aspirational, and the pure helper is testable in isolation. Two yellow notes are worth acknowledging in a follow-up but do not block merge. The rebase risk against the #1970-#1973 media-OS-rework stack is real — if probeStage.ts gets reshaped downstream, please confirm the parity helper survives with its callers intact — but that is a downstream-audit concern, not an R1 blocker.
Review by Via (runtime-interop lens)
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
APPROVE — part of the 5-PR media-use stack (#1969–#1973), two-lens verification. Via (runtime-interop) + a test/observability pass both clear all five, no blockers.
Verified:
- Headline "addressed" items confirmed: #1972's orphan
hyperframes-mediastrings fixed at both call sites (sfx.mjs:125,tts.mjs:184; the only surviving ref is SKILL.md's historical changelog line, intentional); #1973's atomic writes in place on both entry points (transcribe.mjstemp+renameSync,transcript-cut.mjsencode-to-.part+rename), and the parakeet PATH→venv fallback (resolveParakeet()) is resolved. - Test coverage solid: #1969
mutedParity.test.ts(all mute combos), #1970parakeet.test.ts(token-merge boundaries), #1971 crop/affordance/asset-strip tests, #1973 ~0.8 test:source ratio across 4 new files.
Non-blocking follow-ups (Via-accepted nits, worth tracking, NOT gating): #1970 observability (--model ignored on the parakeet path, subprocess stderr discarded on success, no first-run model-download cue, HYPERFRAMES_PARAKEET skips the --version gate); #1973 --copy keyframe-drift warning only on stderr, not in --json.
Merge via the normal Graphite path (bottom-up, no admin-merge). Layered on Via.
— Rames Jusso
c703641 to
55c962a
Compare
3 participants
Muted media played silently in preview but was mixed at full volume in renders. Prune muted browser media from the audio mix (pure
pruneMutedBrowserMedia+ tests).Part of the media-use v2 stack (split from #1923). Base of the stack.
📚 Stack — media-use v2 (split from #1923, merge bottom-up)