fix(core): preserve playhead during volume probing#2143
Merged
Conversation
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
LGTM — clean surgical fix. Leaving as a comment; stamp is James's call.
What I checked at 4c79bf2f:
- Root cause matches the fix.
probeAndCacheElementVolumeruns during runtime init (called frominit.ts:1641viaprobeAndCacheVolumeKeyframes, which fires frombindMediaMetadataListeners). Its inner loop inprobeElementVolumeKeyframesseeks the timeline fromsampleStarttosampleEnd(bounded bycompositionDuration) — for a 3-second composition that's ~180 sequential seeks ending at the composition tail. Read-then-restore around the probe is the right shape. - Signature change is correct. Making
totalTime?/seek?'stoptional ((t?: number, ...)) mirrors GSAP's dual getter/setter API —.totalTime()returns the current playhead,.totalTime(x, true)seeks.Number(...)+Number.isFiniteguard protects against non-numeric mock returns. - No parallel bug on the render side.
probeElementVolumeKeyframeshas only the one caller (the wrapper), and the engine'saudioVolumeEnvelope.ts(offline PCM baking) receives pre-collected keyframes rather than seeking a live timeline, so it doesn't need the same fix. - Test asserts the right thing. Initial playhead 0.75, composition duration 1, mocked
totalTimedual mode — after the probe, the test verifies playhead is back to 0.75 andaudio.volumeis restored, plus that avolume: 0keyframe was captured (probe ran, restoration didn't obliterate the sampling). Solid unit-level coverage for the fix. - CI: initial run all SUCCESS across the CI/Windows/Player-perf/Preview/CodeQL/SDK/CLI/regression matrix; a newer run is churning on the same SHA (functionally clean).
Nits (non-blocking):
- The
Number(timeline.totalTime())getter path returnsunknownunder theRuntimeTimelineReftype. TheNumber.isFinite(originalTime)gate correctly prevents a bogus restore, but if a caller ever passes a timeline whose getter yields a non-number (chained ref, etc.), the probe still perturbs the playhead and the restore silently no-ops. Not a real concern for GSAP, just something worth knowing. - The regression test covers the
totalTimegetter/setter path but not theseek-fallback branch. A one-line secondit()withseek(next?)would round out coverage for theelse ifbranch on lines 151–152 / 164–165. Optional.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2141
Summary
Reproduction (HyperFrames#2141)
Using
hyperframes@0.7.48, a composition withdata-var-text="title", a GSAP opacity tween, and a lateraudiovolume tween renders the default title when invoked with--variables '{"title":"OVERRIDE"}'. Removing only the volume tween rendersOVERRIDE. The volume probe seeks the live timeline through the full duration and leaves it at the final sample, perturbing runtime initialization before capture.Root cause
probeAndCacheElementVolumesampled the live timeline but did not restore its existing playhead. The probe therefore changed the timeline state as a side effect of metadata binding. This was our runtime behavior, not an invalid caller input.Fix / verification
The probe now reads and restores the playhead after sampling. Added a jsdom regression test asserting the timeline position and media volume are restored while automation is still cached. Verified with
bun run test -- src/runtime/mediaVolumeEnvelope.test.tsandbun run buildinpackages/core.