feat(studio): audio level meters at the timeline's right edge - #4193
Conversation
a6d9387 to
b2ba94d
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at a07357246a2d32d0d8d5f069ea1838cf62a7fbf1. Read every changed file in full plus the surrounding graph (webAudioTransport chain construction, scheduleWebAudioForActiveClips, resolveGroupElement, the Timeline container's own class contract), and ran the core suite in a worktree at this head.
The shape is good: the tap is a genuine side branch, nothing is created until start(), the rAF loop re-reads the hook off the live preview window instead of caching it, and the bars are written straight to the DOM. One blocking defect, one limitation I think the PR body overstates, and a small perf note.
Blocking — Master and the group meters sit on opposite sides of the monitor fader
createLevelTap(ctx, this._masterGain) hangs the master analyser off _masterGain's output (webAudioTransport.ts:526-530), and _masterGain.gain.value is the preview's monitor control: applyMasterGain() writes this._masterMuted ? 0 : this._masterVolume (:809-810), fed by setVolume / setMuted (:775, :804), which are wired to the Studio player's audioVolume / audioMuted — a per-user UI preference in localStorage (playerStore.ts:263-264, 409-415), reachable with the M key (usePlaybackKeyboard.ts:131).
The group taps hang off output (:500), which is upstream of _masterGain (output.connect(this._masterGain), :435).
Measured at this head, with your own mock context (probe appended to webAudioTransport.test.ts, one group vo playing):
masterGain.connect targets: ["tap-mix"]
groupOutput.connect targets: ["masterGain","tap-mix"]
masterGain.gain.value at unity: 1
after setVolume(0.5): 0.5
after setMuted(true): 0
group output gain untouched: 1
So the monitor fader sits between the two sets of taps:
- Preview volume at 50% → the Master bar reads ~6 dB under the groups that feed it. The dB scale printed next to it is then wrong relative to the strip beside it.
- Press
M→ Master flatlines while every group meter keeps bouncing.
That second one is the reason I'm calling this blocking rather than a taste question. Under "the meter shows the program" the Master strip is wrong; under "the meter shows what you're hearing" (which is how I read "moving with the real playback level in the preview") the group strips are wrong. There's no reading of the widget where both halves are right, and the divergence is one keystroke away in the default configuration.
Smallest fix I can see that keeps your "audible path is unchanged" property intact: split the two roles that _masterGain is currently playing. Add a monitor gain downstream — everything keeps connecting to _masterGain exactly as it does now, _masterGain.connect(_monitorGain), _monitorGain.connect(destination), and applyMasterGain() writes to _monitorGain. The master tap then stays exactly where it is and becomes a program tap, the group taps already are one, and nothing else in the file changes its connection target. One extra unity gain node in the chain.
(The other direction — moving the master tap upstream of the fader — needs a summing node anyway, because ungrouped clips connect straight to _masterGain, so it lands in the same place.)
Important — the meters measure the WebAudio graph, not the audible output
scheduleWebAudioForActiveClips selects document.querySelectorAll("audio[data-start]") (init.ts:4054, and the paused-side sibling at :3915). A <video>'s soundtrack never enters the graph — it plays natively, with onSetVolume setting el.volume directly (:4212-4229). So it never reaches _masterGain and never reaches a tap.
Consequences:
- A composition mixing audio clips with a video that has sound: the strip renders (there's an
<audio>, sohasProjectAudiois true) and Master under-reports by exactly the video's contribution, silently. - Same for any
<audio>clip that falls back to native playback — thedecode-onlyroute that fails to decode stays audible but is invisible to the tap.
Worth noting the flip side, because I think it's right: hiding the strip entirely for a video-only project (hasProjectAudio using isAudioTimelineElement, which excludes video) is the correct call here, not an oversight — showing a strip that could only ever read silence would be worse. It's the mixed case that misleads.
I'd either scope the PR-body claim ("the real playback level" → the audio-clip and group busses) or say the limitation out loud in the levelTap doc comment. This matters more once the faders land: someone will set a level against a Master reading that's missing a track.
Nit — readLevels() re-scans the preview document once per group, per frame
isPresent() calls resolveGroupElement(doc, groupId) (:468), which runs a full-document querySelectorAll("hf-audio-group[data-hf-render-id]") before falling back to getElementById (audioGroups.ts:100-104). readLevels() calls it for every tapped group, and the rAF loop calls readLevels() every frame with no idle path — it runs at 60 fps while the strip is visible even when the transport is paused and every bar is at rest.
Measured with 3 groups:
one readLevels(): {"querySelectorAll":3,"getElementById":3}
60 readLevels() (=1s at 60fps): {"querySelectorAll":180,"getElementById":180}
That's 180 full-document scans per second inside the preview iframe — the same document that's rendering playback. And it's redundant for the only consumer: useStrips already derives the strip list from the player store, so Studio never asks for a group it doesn't have an element for. Resolving presence when the group set changes (or memoising it for the current read) would drop it to zero per frame.
Smaller things
useMeterLoop'sstatemap never drops entries for strips that disappear (AudioMeterStrip.tsx:78). Bounded by the group count, so it's housekeeping, not a leak that matters.startMetering/stopMeteringaren't ref-counted. Fine with exactly one consumer; worth remembering when the fader PR adds a second reader of the same hook.
Verified negatives
Things I went looking for and did not find, so nobody re-derives them:
- The render path is untouched.
init.ts:2746-2750assigns three closures and nothing else;attachGroupTapis guarded by_metering(:533), which onlystartMetering()sets, and only Studio calls it. A headless render creates no analyser, no splitter and no extraconnect. Theplayer-perfgreen at this head is consistent, but the guard is the actual reason. - The
window.__hfglobal needs no readiness protocol here. The loop re-reads the hook every frame andstart()s on identity change (AudioMeterStrip.tsx:81-94), andinitSandboxRuntimeModularassigns a fresh object per run while teardown+re-init builds a new transport (init.ts:284-286,:297) — so a preview that mounts late, reloads, or is swapped is picked up on the next frame. This is the one case where the window-flag pattern is fine, because nothing is cached across the boundary. - The group id spaces line up. Studio writes
audioGroup: groupIdwheregroupIdis thehf-audio-groupelement's id (timelineAudioGroupCreate.ts:251); the transport keys on the same id viaaudioGroupOf/resolveGroupElement. Strip keys andread().groupskeys match. - The layout wrapper preserves the Timeline's height contract. The Timeline root is
h-full(timelineProviderStateBuilders.ts:82); the newmin-w-0 flex-1wrapper is a stretched item in aflex min-h-0 flex-1row, so its height is definite andheight:100%still resolves. Before, the root was a column flex item shrinking to fit — same final height, and the strip returningnullleaves the row with a single child. - rAF lifecycle is correct.
rafis reassigned at the top oftickbefore the body, so the cleanup'scancelAnimationFrame(raf)always holds the pending handle; unmount also stops the live hook. No listeners or timers added anywhere in the diff. - PR-body test claim reproduces. I ran the core suite in a worktree at this head:
webAudioTransport.test.ts77 passed, andaudioMeterMath.test.ts15 passed. (I couldn't run the component suite in my borrowed rig —cnand@base-ui/reactaren't in the snapshot I was linking against. That's my install, not your PR.)
Per the standing rule I didn't treat CI as a gate; the blocker above is a code finding, not a red check — everything is green at this head.
Happy to re-review as soon as the master tap moves. The rest of this is genuinely nice work — the side-branch design and the identity-keyed hook follow are both the right shape.
— Rames
|
fc14441 addresses the monitor-fader finding: |
jrusso1020
left a comment
There was a problem hiding this comment.
Re-reviewed at 38e2549249f93db769e8af3430591f787a8eabef. The delta over the head I blocked (a07357246) is exactly two commits and two files: fc1444118 plus the oxfmt wrap.
The blocker is resolved. You split the two roles instead of moving the tap, which is the version that leaves every existing connect alone.
I measured it two ways rather than reading it, because the injected-mock tests bypass init() entirely and so can't see the chain they're asserting about.
1 — the real init(), against a stubbed AudioContext (metering started and setVolume(0.5) called before init(), so the ordering at :192 is exercised too):
gain#0 = _masterGain gain 1 connects -> gain#1, gain#2
gain#1 = _monitorGain gain 0.5 connects -> destination
gain#2 = tap mix gain 1 connects -> splitter#3 -> analyser#4, analyser#5
after init (volume 0.5 set pre-init) _masterGain 1 _monitorGain 0.5
setMuted(true) _masterGain 1 _monitorGain 0
setMuted(false) _masterGain 1 _monitorGain 0.5
setVolume(99) _masterGain 1 _monitorGain 1 <- spec clamp
2 — the group case, same probe I ran in round 1 (one group vo playing), so the two rounds are directly comparable:
round 1 a07357246 |
round 2 38e254924 |
|
|---|---|---|
masterGain.connect targets |
["tap-mix"] |
["tap-mix"] |
groupOutput.connect targets |
["masterGain","tap-mix"] |
["masterGain","tap-mix"] |
masterGain.gain at unity |
1 | 1 |
…after setVolume(0.5) |
0.5 | 1 |
…after setMuted(true) |
0 | 1 |
monitorGain.gain (unity / 0.5 / muted) |
— | 1 / 0.5 / 0 |
groupOutput.gain |
1 | 1 |
Both taps now hang off nodes that sit at unity no matter where the preview fader is. M no longer flatlines Master while the group bars keep bouncing, and the audible product is unchanged — the fader changed node, not value.
Two places this shape usually leaks, both clear:
- Nothing bypasses the new node. Every
connectin the file is:190,:191,:361,:439,:450,:459,:651, and:191is the only one that touches_ctx.destination. So_monitorGainstill gates 100% of what the graph carries — mute mutes exactly what it muted before. - Nothing else writes the tapped node.
_masterGainis assigned once (:188) and never written again;applyMasterGainis the only writer of either fader value, and no file outsidewebAudioTransport.tsreferencesmasterGainat all. There's no second writer that could put a level back onto the program tap.
keeps program taps off the monitor fader is a real pin, not a vacuous one: revert the split and applyMasterGain writes _masterGain.gain.value = 0, so expect(mock.masterGain.gain.value).toBe(1) fails.
Still open from round 1 — all non-blocking, none of it gates this
- The PR body claim. This now needs a slightly different edit than I asked for last round, because the fix deliberately decoupled the meters from the monitor fader: "moving with the real playback level in the preview" describes something the widget now intentionally does not do — pull the preview volume down and the bars don't move. Something like "moving with the program level, ahead of the preview volume control" is both accurate and a selling point. The original half stands too:
scheduleWebAudioForActiveClipsonly takesaudio[data-start], so a<video>'s soundtrack never enters the graph and a mixed composition under-reports on Master silently. readLevels()re-scans the preview document once per group per frame — 180querySelectorAllper second at 3 groups, inside the document that's rendering playback, with no idle path.useMeterLoop'sstatemap never drops vanished strips, andstartMetering/stopMeteringaren't ref-counted. Both are fine with one consumer; the second one is worth remembering when the fader PR adds a second reader of the same hook.
Nit
keeps the user's master volume spec-clamped seeds the monitor mock at 1 and expects 1 after setVolume(99), so it passes whether or not setVolume wrote anything. Seed it at a non-unity value — { gain: { value: 0.25 } } — and it distinguishes "clamped to 1" from "never written". The clamp itself is correct; measured 0.5 → setVolume(99) → 1 above. (Separately, applyMasterGain writes the monitor node now — applyMonitorGain would say what it does. Entirely optional.)
What I ran
webAudioTransport.test.ts at this head in a worktree against a borrowed install: 78 passed, which is your 77 plus the new program-tap test. The PR body's "77 passed" is stale by exactly that one.
Per the standing rule I didn't treat CI as a gate — everything above is the code plus the two probes.
Approving. Nice fix: one extra unity node, three lines of production change, and the widget now has a single consistent reading.
— Rames
What
Studio gets audio level meters at the right edge of the timeline: one stereo meter with peak hold and a dB scale (0, -3, -6, -12, -24) for each audio group, plus Master, moving with the real playback level in the preview. A speaker button in the timeline toolbar hides and shows the strip and the choice is remembered. A project with no audio gets no strip and no button.
Read-only in this PR. Faders (each strip's volume through the existing group volume write path) follow in the next PR.
How
window.__hf.audioMeter = { start, stop, read }(levelTap.ts,webAudioTransport.ts).start()adds a stereo analyser branch after each group bus and after master;read()returns the linear peak per channel;stop()disconnects everything. Taps are side branches, so the audible path is unchanged, and nothing exists untilstart().AudioMeterStripruns one rAF loop that re-reads the hook off the live preview window each frame, so a reloaded preview is followed and its taps started. Bars are written straight to the DOM; no React state per frame.read().Before
After
Captured in Studio on this head, from a fixture built of generated tones in two audio groups plus Master. After is playing; the bars are filled, not an idle rail.
Checks
origin/main. The previousregressionred was the aggregate job of a cancelled shard (One or more regression shards failed); shard-6 was cancelled during Docker image build and never ran the fixtures.webAudioTransporttests 77 passed, exit 0 (includes taps created only on start, side-branch wiring, idempotent start, group added or removed mid-session, metering started before audio context init).tsc --noEmitclean for studio.