Repository navigation
fix(core): key the preview volume envelope to the clip, not the timeline - #3198
Conversation
A GSAP volume fade on an audio clip that starts after t=0 left the preview silent for the clip's whole length while the encoded render was correct. `mediaVolumeEnvelope` is meant to keep preview and render on one envelope, and its contract is "normalise, then read with track-relative seconds". The preview skipped both halves. `probeElementVolumeKeyframes` stamps each keyframe with the TIMELINE seek time it sampled at, and `normaliseEnvelope` — the function that rebases those onto the track — had exactly one caller, the renderer's PCM baker. The preview handed the raw keyframes to `interpolateVolumeGain` along with `relTime`, so for a clip at t=2 every lookup fell two seconds before the first keyframe and clamped to its volume: 0 for a fade-in. Rebase once, where the cache is filled, so the cached envelope has a single documented time base. Read it with elapsed-time-in-clip rather than `relTime`, which is a position inside the media SOURCE — it carries `mediaStart` and the playback rate, and only coincides with the envelope's time base for an untrimmed clip playing at 1x from zero. That second half also fixes a latent sibling: a trimmed clip read the wrong envelope point even when it started at 0.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Findings
-
Test coverage is asymmetric to the fix. The regression test at
packages/core/src/runtime/mediaVolumeEnvelope.test.ts:150-186locks in the cache-write half (envelope stored track-relative), but never drives the read-side change atpackages/core/src/runtime/media.ts:279-282. For the untrimmed / 1x case in the test,relTime === elapsedInClip— so reverting themedia.tsarm (elapsedInClip→relTime) still passes this test. The PR body explicitly claims the read-side change also fixes a trimmed sibling (data-playback-start) and, by extension,clip.playbackRate !== 1— neither is covered. Non-blocker, but the two axes the PR itself cites deserve their own assertion (e.g. a clip withmediaStart=5orplaybackRate=2att=3, asserting the read matches the envelope's track-relative point rather than themediaStart-carryingrelTime). -
CLI-feedback linkage not cited. The body describes an author-reproduced case but doesn't include a Slack
ts=/ fingerprint for the CLI-feedback signal that prompted the fix. Adding a permalink helps future repeat-signal correlation and closes the loop for the reporter. Non-blocker. -
Pre-existing cache-staleness surface (noted for awareness, not this PR).
probeAndCacheVolumeKeyframesinpackages/core/src/runtime/init.ts:1864-1876early-returns when the element is already cached. If a fade is edited in-place on the same DOM element, the stored envelope goes stale. Not introduced here — the new normalisation just means the stale envelope is now stale on the correct axis. Fine to defer.
Verdict
Diagnosis is clean and the mechanism trace is exact: two probes stamp timeline time, the renderer already rebased at its single reader, the preview didn't, so a clip after t=0 clamps to the fade's first sample. Rebasing once at the cache-fill point matches the header contract ("normalise, then read with track-relative seconds") and collapses two time bases into one. The read-side switch from relTime to elapsedInClip also fixes the trimmed / playback-rate variants the author calls out. Renderer path untouched — encoded audio is byte-identical, verified in the test plan. All 40+ required checks green including preview parity, Windows render, and cross-shard regression. Named exports, import type clean, oxfmt/oxlint clean. Cache is a per-runtime WeakMap<HTMLMediaElement>, so no cross-clip identity aliasing. Ship it; fold the trimmed / rate-≠-1 test case into the next touch.
— Via
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 629b7f90.
Read through the three changed files, the renderer's PCM baker (packages/engine/src/services/audioVolumeEnvelope.ts), the producer's own timeline probe (htmlCompiler.ts:2196+), and the read site in media.ts. The fix is right and the "rebase once, read with elapsed-in-clip" framing is the correct shape.
Two independent probe paths and both now agree on the time base. The preview runtime's cache (via probeAndCacheElementVolume) is now normalised at write; interpolateVolumeGain(_, elapsedInClip) reads it correctly. The producer's audio-mix pipeline runs a byte-forked probe (discoverAudioVolumeAutomationFromTimeline in htmlCompiler.ts) that emits raw timeline-stamped keyframes, ships them through the plan, and applyVolumeEnvelopeToWav normalises at read via normaliseEnvelope(keyframes, trackStart, baseVolume). Two different code paths, two different normalise sites, one consistent time base at every read. No renderer-side breakage — the PCM baker's input contract is untouched.
Playback rate and trimming come out right. GSAP authors envelopes in TIMELINE time, so elapsedInClip = timeSeconds - clip.start is the correct indexer regardless of playback rate: a 2× clip with a fade authored at timeline [t=2, t=2.05] fades in 0.05s of preview wall-clock, matching the mixer's baked output. The data-playback-start (trimmed) sibling the PR body mentions is closed for the same reason — relTime's mediaStart term never enters the envelope index anymore.
Reader stays inside the active guard. elapsedInClip = params.timeSeconds - clip.start could go negative in principle, but the read at media.ts:273 sits inside the isActive branch (:259) which requires params.timeSeconds >= clip.start (:255), so it never does. interpolateVolumeGain also anchors envelope[0] at time=0 via normaliseEnvelope's prepend, so any tiny floating-point-negative elapsed clamps cleanly to the baseVolume.
Test locks the fingerprint case. mediaVolumeEnvelope.test.ts:147+ asserts envelope[0].time === 0 for a clip at start=2, which is the specific bug's tell (before: {time: 2, ...}; after: {time: 0, ...}). The direct interpolateVolumeGain checks at t=0/0.05/0.5/1 exactly express the preview's expected 0 → 0.667 → 1 → 1 → 1 read across the fade window.
One nit — semantic drift on the clip.volumeKeyframes type. After this fix, clip.volumeKeyframes at media.ts:64 is semantically a normalised envelope, but the type is still VolumeKeyframe[] with no marker. If a future consumer in the runtime treats it as raw timeline-stamped (like the producer's serialised audio.volumeKeyframes in the plan / element.volumeKeyframes in audioMixer.ts), they'll be silently wrong. A type brand (NormalisedVolumeEnvelope = VolumeKeyframe[] & { __normalised: true }) or at minimum a // NORMALISED, indexed by elapsed-in-clip seconds docstring on the field would close that gap. Not blocking.
One observation — duplicate probe code. probeElementVolumeKeyframes in packages/core and discoverAudioVolumeAutomationFromTimeline in packages/producer are structurally the same sampler with byte-forked keyframe compression. Not this PR's concern — flagging as a future consolidation opportunity so preview + render can't drift on sampling semantics if either side gets touched.
Sub-composition fringe (pre-existing, not this PR). resolveVolumeProbeWindow reads raw el.dataset.start, which is relative to a sub-composition parent, but the timeline probed is the root. So a clip inside a sub-composition never gets its automation window sampled and takes the fallback path. Same behaviour before and after this PR — flagging so we don't confuse it later with a regression from this change.
What I didn't verify. Did not stand up an end-to-end preview+render locally to reproduce the 0 → 0.667 → 1 → 1 → 1 timing. Reading matches the test and Miguel's manual measurement in the PR body.
Otherwise LGTM. Stamp routing per standing rule.
What
A GSAP volume fade on an audio clip that starts after
t=0leaves the preview silent for the clip's entire length, while the encoded render is correct. This puts the preview back on the same envelope the renderer uses.Why
Reproduced on macOS with one
<audio data-start="2" data-duration="3" data-volume="1">and a 0.05sfromTovolume fade at the clip's start. Probingel.volumethrough the preview:The encoded render was already right: full volume reached just after the fade and held to the clip's end. So the same composition sounded silent while authoring and correct once rendered, which is the worst version of this to debug.
mediaVolumeEnvelopeexists to keep preview and render on one envelope — its own header comment says so — and its contract is "normalise, then read with track-relative seconds". The preview skipped both halves:probeElementVolumeKeyframesstamps each keyframe with the timeline seek time it sampled at.normaliseEnvelope, which rebases those onto the track, had exactly one caller: the renderer's PCM baker inaudioVolumeEnvelope.ts.interpolateVolumeGainalong withrelTime.For a clip at
t=2, every preview lookup landed two seconds before the first keyframe and clamped to its volume — 0 for a fade-in. Two controls confirm the mechanism: the same clip with no fade previews at a steady 1, and the same fade atdata-start="0"previews correctly, because there timeline time and track time coincide.How
Rebase once, at the single point that fills the cache, so the cached envelope has one documented time base instead of one per reader.
Read it with elapsed-time-in-clip instead of
relTime.relTimeis a position inside the media source: it carriesmediaStartand the playback rate, so it only coincides with the envelope's time base for an untrimmed clip playing at 1x from zero. That second half also fixes a latent sibling — a trimmed clip (data-playback-start) read the wrong envelope point even when it started at 0.The renderer's path is untouched; it already normalised.
Not covered here: audio clips land about 20 ms after their authored
data-startin the rendered mix. That reproduces with and without volume automation, is not encoder priming (two codecs agree, and a hand-built reference through the same ffmpeg lands exactly on time), and is a separate defect in the assemble stage rather than in this envelope. Filed separately.Test plan
New regression test in
mediaVolumeEnvelope.test.ts: a clip starting att=2with a 0.05s fade must cache an envelope whose first keyframe is attime: 0, and reading it must give 0 at the clip's start, 1 once the fade completes, and 1 held to the end. Verified it fails onmainwith{ time: 2 }where{ time: 0 }was expected, and passes with the fix.packages/coresuite: 1912 passed, 100 files.packages/engineaudio mixer and volume-envelope suites: 30 passed.oxlintandoxfmtclean on all three changed files.