fix: prevent nested composition videos from autoplaying on seek#477
fix: prevent nested composition videos from autoplaying on seek#477miguel-heygen merged 2 commits intomainfrom
Conversation
75201a4 to
97f888b
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Verdict: approve — bug reproduced on main, fix cleanly closes the ownership split
Local verification
Bug reproduced on main (6b21ead7). Cherry-picked the new init.test.ts case onto main and ran via vitest:
FAIL src/runtime/init.test.ts > initSandboxRuntimeModular > pauses nested media that is outside the timed-media cache after a seek
AssertionError: expected false to be true // Object.is equality
expect(video.paused).toBe(true);
That's the real autoplay-on-seek: player.seek(29) leaves the nested <video> at paused: false because the pre-PR refreshRuntimeMediaCache only tracked video[data-start] / audio[data-start], so an inner media element without its own data-start was invisible to the sync pass.
Fix verified on 97f888be via vitest:
bun run test -- src/runtime/init.test.ts src/runtime/media.test.ts src/runtime/player.test.ts→ 74/74 pass (the new regression case + 2 pre-existinginit.test.ts+ 42media.test.ts+ 29player.test.ts)bun run --filter @hyperframes/core typecheck→ clean
Staff review — architecture + correctness
The PR identifies a single conceptual split and closes it at the right abstraction layer. Prior contract: "media ownership requires data-start." That's correct for root-level timed media, wrong for media that inherits its timing from an enclosing composition host. The fix extends refreshRuntimeMediaCache with explicit ownership predicates (shouldIncludeElement, resolveDurationSeconds) so the media helper stays generic and the init.ts caller owns the policy. Clean.
The inheritance math is right for the current pipeline:
shouldIncludeElement:hasAttribute("data-start") || closest("[data-composition-id]"). Every<video>/<audio>inside any composition host now participates.resolveStartSeconds:resolveStartForElement(element, inheritedStart ?? 0). The fallback only kicks in when the element has nodata-startof its own — in which case we pick up the host's start.resolveDurationSeconds: computes bothsourceDuration = element.duration - mediaStartandhostRemaining = inheritedStart + inheritedDuration - start, then returnsmin(sourceDuration, hostRemaining). Source-longer-than-host clamps to the host window; source-shorter-than-host keeps the natural duration. Correct in both directions.
Why the "global-data-start" assumption holds. For a nested <video> WITH its own data-start: the Hyperframes compiler already offsets scene-local data-start values to global during sub-composition inlining (that's the whole point of parseSubCompositions + the offset logic, recently reinforced by the hf#476 template-unwrap fix for the same data flow). So by the time the runtime sees the HTML, resolveStartForElement returns a global start even when the author wrote a scene-local one. The fallback-to-inherited in this PR only ever fires for media that legitimately had no data-start at all.
Transport-state sync. The paused transition flows through syncMediaForCurrentState → syncRuntimeMedia (the existing path), and the new elements get included in mediaClips with correct start/end bounds. The regression test proves the paused-after-seek path works; the playing path is covered by the same sync logic (just with a non-paused transport) so it should work by construction.
Non-blocking observations
-
Test gap — positive play-state path. The new regression test covers paused-after-seek. A complementary test for "transport playing, seek into nested composition, expect nested video playing + advancing" would guard against a future change that silently re-breaks the play path. The current single test is necessary but not sufficient coverage. Not blocking since the shared
syncRuntimeMediahandles both transport states. -
Test gap — source-longer-than-host clamp.
resolveDurationSecondsreturnsmin(sourceDuration, hostRemaining). In the regression fixture,element.duration = 20andhostRemaining = 16(host at[20, 36], media starts at20→ remaining16), so the clamp fires (min(20, 16) = 16), but the test assertscurrentTime === 9which doesn't exercise the end-of-window behavior. A positive test that seeks past the host edge and verifies the nested video stops would validate the clamp. Not blocking. -
includeAuthoredTimingAttrs: truehere vs.falsein hf#464's visibility path. This file'sresolveMediaCompositionContextusestrueto resolve the host's duration, meaning the AUTHOREDdata-hf-authored-durationwins over the live child timeline's.duration(). In hf#464's9754c3b3the composition-host visibility path was flipped tofalseto prefer the live timeline. The two paths diverge intentionally — visibility needs live truth (so a shrinking sub-composition hides its children early); media sync needs authored stability (so a short child timeline doesn't clamp the media window smaller than the author intended). Correct asymmetry, but a one-line comment at this call site noting "we intentionally use authored duration here because media window is stable, unlike visibility cutoff" would save future archaeology. -
Semantic broadening of "timed media."
shouldIncludeElementnow brings every<video>/<audio>inside any[data-composition-id]element under transport sync, not just those with explicitdata-start. That aligns with the framework contract ("the framework manages media playback") and matches the PR's stated intent. If the codebase ever grows a legitimate use case for a composition-local video that's meant to play independently of the transport (e.g., an ambient loop), the newshouldIncludeElementlambda gives a clean extension point to exclude it. For now, no such case exists. -
Performance.
refreshRuntimeMediaCachenow runsclosest("[data-composition-id]")+resolveStartForElement+resolveDurationForElementper media element per sync tick. At Studio preview scale (tens of media elements, seeks a few times per second) this is unmeasurable. At scale (hundreds of media elements in a large composition) it could become a noticeable fraction of a sync tick. Future optimization: memoize composition context per-element, invalidate on DOM mutation. Flag only.
CI state
On 97f888be — green where completed: Format / Typecheck / Lint / Test / Build / Test: runtime contract / Smoke / CodeQL / Perf (fps/scrub/drift/load). In-progress: Perf: parity, Tests on windows-latest, Render on windows-latest, all 10 regression-shards. No blockers; hold merge on those settling.
Ship once CI finishes.
Review by Rames Jusso
Merge activity
|
Problem
Studio seek could still wake nested composition media even when the transport itself stayed paused.
In the real repro from
apple-presentation, scrubbing to0:29without pressing play lands on theslide-translationcomposition. That composition containsMultilingual_Journey.mp4inside the composition host. On the broken path:That was especially confusing because the seek was otherwise correct: the timeline moved to the right point, but the nested media stopped obeying the paused transport state.
What this fixes
Nested media now participates in runtime media sync
video[data-start]/audio[data-start]are relevantdata-startNested media timing is resolved in the host composition window
0Paused seeks land on the right frame and stay paused
Regression coverage
data-startplayer.seek(29)leaves the nested video paused while landing it at the expectedcurrentTimeRoot cause
The bug came from a mismatch between deterministic timeline seeking and media ownership.
1. The runtime only managed media with direct timing attrs
refreshRuntimeMediaCache()only collectedvideo[data-start]andaudio[data-start]. That works for root-level timed media, but not for media embedded inside a composition host where timing is inherited from the host composition rather than duplicated onto the inner media node.2. Nested composition seek could still advance inner media
The runtime intentionally rearms sibling timelines during deterministic seek so nested timelines land on the right local offsets. That part is necessary and correct.
But because the nested video was not part of the managed media cache, it could advance during that seek path without being brought back under the paused transport state afterward.
3. The runtime had no way to reconcile the two
So the system had an inconsistent split:
The fix closes that split by resolving nested media start/duration from the enclosing composition context and running it through the same sync logic as other managed media.
Verification
Local checks
bun run --filter @hyperframes/core typecheckbun run test -- src/runtime/init.test.ts src/runtime/media.test.ts src/runtime/player.test.tsinpackages/corebunx oxlint packages/core/src/runtime/media.ts packages/core/src/runtime/init.ts packages/core/src/runtime/init.test.tsbunx oxfmt --check packages/core/src/runtime/media.ts packages/core/src/runtime/init.ts packages/core/src/runtime/init.test.tsBrowser verification
Verified against a repo-backed local Studio preview of
apple-presentation:http://127.0.0.1:3014/#project/apple-presentation0:29without pressing playslide-translationMultilingual_Journey.mp4landed at a non-zerocurrentTime(3.067in the verified run)pausedand itscurrentTimeremained stable across a follow-up check instead of autoplayingNotes
qa-artifacts/autoplay-seek/are verification-only and are not part of this PR