fix(runtime): resolve media clip start once for playback, manifest, and audio

[miguel-heygen]

What a user could not do before

Put a video or audio clip inside a scene, and the editor's timeline could draw it in the wrong place. Playback was right; the drawing was wrong. In the worst case the clip was pushed past the end of the composition and vanished from the timeline entirely, while still playing fine.

A nested clip's volume fade also started at the wrong moment, for the same reason.

Why it happened

Two pieces of code worked out "when does this clip start" from the same HTML attributes, each in its own way. One drew the timeline; one played the video. Only the playback side knew about the marker that says a clip's start time is already measured from the beginning of the whole video rather than from its scene.

Neither side was testable against the other, so both test suites were green for as long as the bug existed.

The change

There is now one function that answers that question, resolveMediaStartForElement, and everything asks it:

Caller	What it does	Before
clip manifest, media window scan	sizes the composition	read the raw attribute, ignored the scene offset entirely
clip manifest, per-clip start	draws the timeline	added the scene offset even when the value was already absolute
visibility pass	shows/hides a clip	already correct
media cache	drives playback	already correct
clock audio attach, hard resync, Web Audio scheduling	schedules audio	read the raw attribute
volume automation probe	rebases a fade envelope	read the raw attribute

Two duplicated helpers also collapsed into one: resolveMediaElementDurationSeconds had identical copies in the manifest and the runtime.

startResolver now imports readElementPlaybackRate straight from playbackRate rather than through media's re-export, which removes an import cycle the volume-probe caller would otherwise have created.

Regression coverage

Two tests build a real composition, initialise the runtime, and assert that the manifest's clip start equals the start the runtime plays at. Neither test supplies the expected number itself, so it cannot pass vacuously. On the unfixed code:

FAIL > reports the same media start in the clip manifest as the runtime plays at
AssertionError: expected undefined to be close to 45.4, received difference is NaN

FAIL > keeps a composition-local media clip in the manifest at the time it plays
AssertionError: expected undefined to be close to 12, received difference is NaN

undefined because at the wrong start the clip was clamped out of the manifest altogether.

A third test covers the volume envelope origin for a nested clip; reverting that one line alone gives:

× uses the clip's absolute start, not its composition-local data-start
  → expected +0 to be close to 1

Verification

• bun run test:hyperframe-runtime-ci — exit 0, 50 files / 1083 tests passed
• bun run lint, bun run format:check — exit 0
• packages/core typecheck (both projects) — exit 0

Known follow-ups, not in this PR

• media.ts refreshRuntimeMediaCache and adapters/css.ts createCssAdapter each keep a Number.parseFloat(data-start) fallback for callers that pass no resolver. The only production caller passes one, so these are unreachable defaults, but the tests exercise them, which means the tests construct the unit differently from production. Removing them touches ~36 test call sites and belongs in its own change.
• clipTree.ts resolveDuration derives a third duration with different fallback semantics (it substitutes the remaining root window). Used only to filter zero-duration decorative elements, so it does not currently disagree in a user-visible way.
• The render pipeline has its own Node-side start resolution over a parsed document. It honours the same rule today, but nothing structurally ties the two together.

Conflicts

Touches the same region of init.ts as two open PRs that rework how resolvers are constructed there. This change keeps the existing three-wrapper shape, so it should rebase onto either without a semantic conflict, but the hunks overlap.

[vanceingalls]

Verdict: Approve

Verified against source at bd8044c (not just the diff summary) — traced all 5 consolidated call sites against pre-PR code, hand-traced the multi-level-nesting math, and actually ran the touched test files (built @hyperframes/parsers first, then vitest run on startResolver.test.ts, mediaVolumeEnvelope.test.ts, init.test.ts, timeline.test.ts).

What I checked

1. The resolver is a genuine consolidation, not a rename. resolveMediaStartForElement (packages/core/src/runtime/startResolver.ts:200) is new in this PR. init.ts's resolveAbsoluteMediaStartSeconds (used by clock-audio-attach, hard-resync, and Web Audio scheduling), timeline.ts's manifest start/window-scan, and mediaVolumeEnvelope.ts's resolveVolumeProbeWindow all previously read data-start (or Number.parseFloat(el.dataset.start)) directly and now call through the shared resolver — I diffed each site against main to confirm the old inline math is actually deleted, not left dead alongside a wrapper.

2. Multi-level nesting composes correctly. resolveMediaStartForElement's hostStart comes from resolveStartForElementInternal on the nearest ancestor [data-composition-id], which recurses through the pre-existing (untouched by this PR) resolveHostOffsetForElement for every ancestor composition root up the tree, with cycle protection (visiting set) and memoization (WeakMap cache). I hand-traced a hypothetical 3-level case (root → sceneA → sceneB → clip) and the offsets compose correctly (0 + sceneA's own resolved absolute start + sceneB's own resolved absolute start). This recursive core already has 3-level coverage via sibling-reference chaining (startResolver.test.ts:241, slide-1→slide-2→slide-3), just not through a nested-composition fixture specifically. Nit: worth adding one explicit 3-level nested-composition regression test through resolveMediaStartForElement itself, since this is precisely the bug class being fixed — cheap insurance, not a blocker.

3. Tests actually pass. 201/202 in the 4 touched files. The 1 failure (TypeError: readFileSync is not a function in init.test.ts) reproduces identically on an untouched, unrelated file (compositionAssembly.test.ts) — confirmed it's a local sandbox/jsdom node:fs polyfill quirk, not a regression from this change (matches a known flake pattern seen on prior PRs in this stack). CI's own Test job is green.

4. hardSyncAllMedia (init.ts:3179) correctly adds if (!el.hasAttribute("data-start")) continue;. This site queries plain video, audio (unlike the other consolidated sites, which already query audio[data-start]/video[data-start]), and unlike the old Number.parseFloat behavior — which returned NaN and got skipped by !Number.isFinite(start) — the new resolver returns a real fallback value for an element with no data-start. Verified this is the only site that actually needed the new guard.

5. Volume-fade probe now samples the same absolute start the transport plays at, and the probeKeyframesInWindow split also fixes a latent double-computation (the window used to get resolved twice per element for probe + normalize). New test exercises exactly the nested-composition-offset scenario from the bug report.

6. Scope is honestly disclosed. The three related-but-unconsolidated readers called out in the PR description (media.ts/adapters/css.ts data-start fallbacks, clipTree.ts resolveDuration, the Node-side render pipeline) — I checked directly: the two runtime fallbacks are genuinely unreachable in production (their only real callers already pass a resolver via resolveStartSeconds/resolveMediaStartForElement), so the "safe to leave" claim holds up, it isn't hand-waved.

7. No missed 6th call site. Grepped packages/core/src/runtime/* for other independent data-start math; nothing found beyond what's disclosed. Broader repo grep for data-start turns up ~70 hits in studio/producer/engine, but those are a different layer (React studio state, Node render pipeline) that the PR explicitly scopes out.

8. No timeline-UI round-trip risk. Manifest generation is stateless — it recomputes from raw data-start on each call; editing only ever writes the raw attribute, never a resolved value, so there's no double-offset/feedback-loop risk from repeated resolve→write→resolve cycles.

No blockers. One low-cost nit (a 3-level nested-composition test through resolveMediaStartForElement) but nothing that should hold up merge.

— Vai

[terencecho]

APPROVE — concurring with Vance's stamp at this head. The "5 parallel restatements → one resolver" migration is correct, complete, and the just-added commit 1ba54717… resolves the per-call resolver-allocation non-blocker my diff-review turned up. All axes clean.

Head + freshness. 1ba547179b74b574dde2d2160b06a2e336a1236c, three commits all authored by miguel-heygen. mergeable=MERGEABLE. mergeStateStatus=BLOCKED reflects the fresh CI run kicked off by the rebase — Test / Typecheck / Player-perf / regression shards / Windows render / CodeQL still IN_PROGRESS at time of stamp. Branch protection will hold on CI settle.

1. Five parallel restatements — all retired.
Six read paths in packages/core/src/runtime migrated (Miguel's PR body lists five plus "media cache, already correct"). All confirmed:

• timeline.ts:186 — resolveMediaWindowEndSeconds: was Math.max(0, Number(getAttribute("data-start") ?? 0) || 0); now startResolver.resolveMediaStartForElement(mediaNode).
• timeline.ts:346 — manifest per-clip start: media branch routes to resolveMediaStartForElement, non-media stays on resolveStartForElement.
• init.ts — clock-audio-attach fallback loop: was Number.parseFloat(rawEl.dataset.start ?? ""); now resolveAbsoluteMediaStartSeconds(rawEl).
• init.ts — hardSyncAllMedia: now hasAttribute("data-start")-guarded + resolver.
• init.ts — scheduleWebAudioForActiveClips: now resolveAbsoluteMediaStartSeconds(rawEl).
• mediaVolumeEnvelope.ts — resolveVolumeProbeWindow: was parseStrictFiniteTimingNumber(el.dataset.start) ?? 0; now via the resolver.

No sixth restatement in runtime code. Full-repo grep at head for dataset.start reads across packages/core/src/runtime returns only the two Miguel-declared "unreachable defaults" (media.ts:80, adapters/css.ts:26), both guarded behind a resolveStartSeconds callback that both production sites pass. packages/core/src/generators/hyperframes.ts:838 also reads dataset.start but that's the Node-side HTML generator emitting a standalone <script> sync loop — separate emitted-code path Miguel flagged in his third follow-up. Out of scope.

2. Resolver math correct, incl. two-level nesting.
resolveMediaStartForElement in startResolver.ts:190-207:

• 2-level nesting (outer 10, inner 5, clip 2): closest([data-composition-id]) → inner scene; resolveHostOffsetForElement walks to outer via parentComposition recursion → 10 + 5 = 15; clip = 15 + 2 = 17. Correct for arbitrarily-deep nesting.
• Legacy data-hf-media-start-basis="global" bypasses host-add and returns authoredStart verbatim. New test at init.test.ts:~2213 pins this (host 45.4, video 45.4 global-basis → 45.4, not 90.8).
• Flat composition (root host at 0 or absent): hostStart <= 0 gate falls to resolveStartForElementInternal(element, hostStart) — bit-for-bit equivalent to the old parseFloat(dataset.start) for the common case.

3. Test coverage.
Three new tests, all newly added, all fail against pre-fix (asserts manifestClip?.start toBeCloseTo(runtimeStart!) — expected value IS the runtime's own answer, so cannot pass vacuously):

• init.test.ts:~2209 — nested clip with legacy global basis, manifest ≟ runtime.
• init.test.ts:~2251 — nested clip composition-local, manifest ≟ runtime.
• mediaVolumeEnvelope.test.ts:~204 — nested clip volume-fade probe window origin.

Manifest + volume-probe directly covered end-to-end. Audio-scheduling / hardSync / media-cache covered indirectly via the shared resolveAbsoluteMediaStartSeconds closure the manifest test's window.__hfResolveMediaStartSeconds handle exercises. Not a blocker — closure is trivially delegated.

4. Non-nested behavior parity. Flat-composition case: resolveMediaStartForElement with hostStart <= 0 falls through to resolveStartForElementInternal, collapsing to the absolute value. Same as old parseFloat(dataset.start). Semantic change for hostStart > 0 AND authoredStart == 0 (composition-local zero): old raw parse returned 0 unconditionally (ignoring host offset — this IS the manifest bug); new returns hostStart. Intentional and correct.

5. Compensating-quirk delta between the 5 impls and the resolver.
Enumerated each removed-hunk's edge-branch vs. the resolver:

• Missing data-start: OLD parseFloat("") = NaN → caller's Number.isFinite skips. Every affected loop already scopes via querySelectorAll("audio[data-start]") or explicit hasAttribute("data-start") guard, so the missing-attr case is unreachable at these five sites. NEW resolver would fall to hostStart — behavior change moot because the case can't occur.
• Non-numeric attribute (data-start="foo"): OLD → NaN → skip. NEW parses as expression; if resolves to a data-composition-id or id, becomes a reference resolution and returns a real number; otherwise falls to hostStart or 0. Silent behavior change on invalid input, but no production composition would author invalid data-start attributes.
• Negative data-start ("-2"): OLD raw parse returned -2 (unclamped); NEW clamps to 0 via Math.max(0, expression.value) in resolveStartForElementInternal. Semantic tightening, no regression.
• Expression-syntax ("intro + 2"): OLD → NaN → skipped by audio loops; NEW resolves the reference correctly. Semantic improvement — audio can now use expression starts.
• Float precision: no site did rounding; all use raw parseFloat/Number. NEW resolver adds and clamps but doesn't round. No delta.
• Duplicated resolveMediaElementDurationSeconds helpers: two identical copies (media.ts, timeline.ts inline), verified byte-for-byte identical before this PR. Consolidation into playbackRate.ts:47 is safe.

No pre-existing test was relying on OLD compensating quirks — CI was fully green on the pre-rebase head across 9 regression shards + preview-parity + Windows + Player-perf. A test passing on parseFloat("intro + 2") = NaN → skip would have flipped; none did.

6. Consumer-invariant match per new consumer.
resolveMediaStartForElement returns number (never null) — signature at startResolver.ts:28. Each new consumer verified:

• Manifest generation (timeline.ts:346): computes end = start + duration, then maxEnd = Math.max(maxEnd, end). Any finite number fine.
• Manifest media-window-end (timeline.ts:186): guards if (!Number.isFinite(start)) continue.
• Audio-scheduling ×2 (init.ts 3088, 3210): both guard if (Number.isFinite(...)).
• Volume-fade probe (mediaVolumeEnvelope.ts): new test pins nested-clip envelope origin.
• hardSyncAllMedia (init.ts 3179): NEW code adds if (!el.hasAttribute("data-start")) continue BEFORE the resolver — same effect as OLD implicit parseFloat("") = NaN skip.
• Media cache (init.ts 2143 via refreshRuntimeMediaCache): closure passed as resolveStartSeconds param; consumer already lived with numeric returns.

No consumer relies on a null sentinel the resolver never emits. No crash surface introduced.

7. Commit 3 — 1ba54717… addresses the one non-blocker.
The pre-rebase review flagged resolveAbsoluteMediaStartSeconds allocating a fresh createRuntimeStartTimeResolver({...}) on every call — per-element-per-tick allocation in hot loops (hardSyncAllMedia, scheduleWebAudioForActiveClips). Commit 3 (+3/-10, init.ts only) replaces the per-call construction with timingResolverFor(true).resolveMediaStartForElement(element), routing through the pass-scoped resolver. Consistent with #3831's shared-pass pattern (already merged). No new surface — pure allocation-reduction fix.

8. Scope. 7 files, +234/-64 pre-rebase + commit 3's +3/-10. All in packages/core/src/runtime. No dependency on #3824 beyond sharing the runtime module; the commit-3 fix explicitly aligns with #3831's timingResolverFor API which is now merged.

9. CI at new head. Fresh run kicked off by the rebase. Many jobs IN_PROGRESS at time of stamp (Test, Typecheck, Player-perf ×5, all 9 regression shards, Windows render, CodeQL). Pre-rebase head was fully green across the same lanes. Branch protection will hold merge until this run settles.

10. Concurrent reviews. vanceingalls APPROVED (5161977716) at this head at 02:19:40Z — concurring here.

— tai

[vanceingalls]

Re-verification at new head 1ba547179 (post-rebase onto #3824)

My original approval was verified against bd8044c (pre-rebase). Confirmed startResolver.ts, timeline.ts, mediaVolumeEnvelope.ts, and playbackRate.ts are byte-identical between bd8044c and 1ba547179 — only init.ts changed, to route through #3824's pass-scoped withTimingResolver/timingResolverFor instead of constructing its own resolver inline. Re-verified the following against the new head directly.

Rebase-specific question: does reuse of the pass-scoped resolver introduce reentrancy/staleness?

Traced every call site of the new resolveAbsoluteMediaStartSeconds (now timingResolverFor(true).resolveMediaStartForElement(element)):

• The transportTick audio-attach loop (init.ts:3254), play()'s hardSyncAllMedia/scheduleWebAudioForActiveClips (init.ts:2670,2675), and pause()'s hardSyncAllMedia (init.ts:2690) all run outside any withTimingResolver scope. activeTimingResolver is null there, so each call constructs its own fresh resolver — the same "construct-per-call" behavior #3824's own comment says is preserved for out-of-scope callers. No staleness risk (nothing is shared across elements).
• The one reentrant case: applyTimedElementVisibility (Scope 2) calls scheduleWebAudioForActiveClips() synchronously when hiddenAudioDirty && clock.isPlaying() (init.ts:2290-2292). This reuses Scope 2's active resolver. I checked whether anything in scheduleWebAudioForActiveClips's synchronous body (up to its .then() continuations) mutates data-start/data-duration/data-composition-id/etc. — it doesn't; classifyWebAudioMediaRoute/reportWebAudioMediaRoute are pure reads plus a WeakSet diagnostic latch. The .then() continuations run as microtasks after the scope has already closed (withTimingResolver's finally runs synchronously when applyTimedElementVisibility returns), and none of them call a resolve function — they close over already-computed compStart/mediaStart locals. So the reuse is safe: #3824's invariant ("nothing in this body calls el.load() or awaits") still holds transitively into the reentrant call.

No violation of #3824's scoping assumptions found.

Via's point 1 — byte-for-byte delta per former call site

• parseStrictFiniteTimingNumber (used by the new resolver) is a direct passthrough alias of parseNumeric from startExpression.ts (playbackRate.ts:6-8) — the exact same function the old init.ts code called directly. No parsing divergence.
• Both the old resolveAbsoluteMediaStartSeconds (init.ts) and the new resolveMediaStartForElement (startResolver.ts) are structurally guaranteed to return a finite number, never NaN/null — traced every branch (host-offset fallback chain in resolveStartForElementInternal, and the two-armed basis logic in resolveAbsoluteMediaStartSeconds/mediaTiming.ts, all clamp or default to a finite value).
• One real, but pre-existing and intentional, divergence: timeline.ts's manifest/window-scan previously did Math.max(0, Number(data-start)) for media — clamping to 0 and ignoring the scene offset entirely (the bug). The new shared resolver composes hostStart + authoredStart without clamping the final sum to ≥0. For a deliberately negative nested data-start smaller in magnitude than its scene's own offset, this could theoretically produce a small negative composed start where the old manifest code produced 0. However, this exact unclamped composition is what the "already correct" playback/media-cache/visibility-pass call sites have used all along (same resolveAbsoluteMediaStartSeconds math in mediaTiming.ts, pre-existing, untouched) — so this PR is making the manifest consistent with playback's long-standing, tested behavior, not introducing new unclamped math. Flagging for visibility, not a blocker.
• Ran the full packages/core/src/runtime/ suite (not just the touched files) against the new head: 1094/1095 passing. The 1 failure, and 1 additional suite-collection failure in deckAudio.test.ts (resolve is not a function from node:path), are the same class of sandbox-local node:fs/node:path polyfill quirk seen before — reproduces on files this PR never touched. No test failed from a "compensating quirk" in an old implementation being removed.

Via's point 2 — consumer-invariant mismatch (null/missing handling)

Checked all 4 non-"already correct" consumers for Number.isFinite/null-guard preservation:

• init.ts clock-audio-attach, hardSyncAllMedia, scheduleWebAudioForActiveClips: all keep (or, for hardSyncAllMedia, gained) an explicit finite/attribute guard.
• timeline.ts manifest per-clip loop: no Number.isFinite(start) check, same as before — and correctly so, since (as above) the resolver structurally cannot return non-finite here, matching the old resolveStartForElement's same guarantee. Verified this isn't a newly-missing check, it was never needed.
• timeline.ts window-scan: explicit if (!Number.isFinite(start)) continue preserved.
• mediaVolumeEnvelope.ts probe: no nullable return to guard against; both old (?? 0 fallback) and new resolver always produce a real number.

No crash-on-null/NaN risk found in any of the 4 consumers from this swap.

Verdict unchanged: Approve. The rebase's resolver-reuse mechanism doesn't violate #3824's scoping invariants, and Via's two concerns don't surface a real behavior regression — only one pre-existing (not new) unclamped-negative-start edge case worth keeping in mind, not blocking.

— Vai