fix(core): prevent double-offset on pip video visibility and media sync#1086
Conversation
…y loop For video and audio elements, data-start is authored in global (composition-root) time — the same contract used by the render pipeline's discoverMediaFromBrowser, which reads the raw attribute directly. Previously, the visibility loop called resolveStartForElement which adds the nearest ancestor composition's global start on top, causing a double-offset that kept pip-wired media permanently hidden when the host composition did not start at t=0. Example: a pip video with data-start="45.40" inside a host composition that also starts at data-start="45.40" resolved to 90.80, so the video was always hidden during its actual [45.40, 52.46] window. Non-media elements (divs, sections, etc.) continue to use the accumulating resolver because their data-start values are local to their composition. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Narrow the raw data-start read to media elements without data-hf-auto-start (explicitly authored global coordinates). Elements with auto-injected data-start="0" remain composition-local via the resolver. Apply consistently across all three consumers: - visibility loop (init.ts) - refreshRuntimeMediaCache start/duration (init.ts) - resolveMediaWindowEndSeconds (timeline.ts) Add regression test for auto-injected data-start="0" inside a late-starting host to prove it doesn't regress.
Renders a 6s composition with a pip video inside a sub-composition host starting at t=3. Verifies the video is visible during the host window and not double-offset to t=6. Baseline generated in Docker.
Fallow audit reportFound 62 findings. Duplication (41)
Health (21)
Generated by fallow. |
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Read the full diff + enumerated every resolveStartForElement call site in init.ts to audit intra-file parity. The math in the tests checks out (host[45.40, 52.46], pip[45.40, 52.46]: visible@46 ✓, hidden@53 ✓, hidden@44 ✓), and the render-preview parity restoration to match discoverMediaFromBrowser is the right call. But one parallel media-window site in init.ts looks unfixed.
Potential miss — resolveMediaWindowDurationSeconds in init.ts
This function has the same shape as collectRuntimeTimelinePayload in timeline.ts (the third site the PR did fix):
const resolveMediaWindowDurationSeconds = (): number | null => {
const mediaNodes = Array.from(
document.querySelectorAll("video[data-start], audio[data-start]"),
) as HTMLMediaElement[];
...
for (const node of mediaNodes) {
const start = resolveStartForElement(node, 0); // ← unguarded
...
maxWindowEndSeconds = Math.max(maxWindowEndSeconds, Math.max(0, start) + duration);
}
...
};It feeds an active code path:
resolveMediaWindowDurationSeconds
→ resolveMediaDurationFloorSeconds
→ resolveTimelineDurationSeconds // returns Math.max(timelineDuration, durationFloor, fallbackDuration)
With the bug shape (pip video data-start="45.40" inside host data-start="45.40"), start = 90.80, so:
maxWindowEndSeconds = 90.80 + 7.06 = 97.86(should be52.46)durationFloorbecomes97.86safeDuration = Math.max(authoredTimeline, 97.86, ...) = 97.86if the authored timeline isn't longer
So the timeline duration floor is inflated by the offset amount — the player would think the comp is ~98s long instead of ~52s. Wouldn't show up in the visibility test (which seeks to specific times and asserts a class), but would surface as wrong timeline length in the scrubber / metadata. Per the PR body, the fix was applied to "all three consumers" of resolveStartForElement on media — this is a fourth, parallel to the one that got fixed in timeline.ts.
Suggested patch — same guard shape as the other three sites:
const start = !node.hasAttribute("data-hf-auto-start")
? Math.max(0, Number(node.getAttribute("data-start") ?? 0) || 0)
: resolveStartForElement(node, 0);(The selector video[data-start], audio[data-start] already filters to media; no tag check needed inside the loop.)
Soft observation — CSS adapter on media elements
createCssAdapter({ resolveStartSeconds: (element) => resolveStartForElement(element, 0) }) also uses the unguarded resolver. If a pip video has a CSS animation attached (rare but legal — e.g. a fade-in keyframe), the CSS adapter would resolve its start time with the same double-offset. Probably an edge case rarely hit, but if you're patching resolveMediaWindowDurationSeconds anyway, worth checking whether the CSS adapter needs the same media/non-media discrimination. Not blocking.
Positive findings
- Three patched sites + new test coverage matches the PR body. Math in both new tests checks out.
- Test 2 (auto-start guard) is the load-bearing assertion that the
data-hf-auto-startmarker discriminator is what's discriminating. Without it, all media would skip the resolver and the auto-injected case would break. - Producer regression test
pip-video-late-host+ 100/100 visual checkpoints adds end-to-end coverage. Good closed-loop. - Render-preview parity is restored — preview now matches
discoverMediaFromBrowser's global-coords assumption. This is exactly the kind of parity gap thereference_preview_render_parity_checklens watches for. ✓
Non-blocking observations
- Four-site (would-be-five with
resolveMediaWindowDurationSeconds) duplication of the same guard expression is a refactor candidate —resolveMediaStartSeconds(element, fallbackContext)helper would consolidate. Not for this PR; if more media-time call sites get added, the duplication grows linearly. - Fallow audit red but not acknowledged in the PR body. If it's the inherited duplication carried over, fine; if it's new (e.g. the four-site guard duplication tripping a new threshold), worth a sentence in the body.
Verdict
The fix is correct on the three patched sites and tests pin the contract well. The unfixed resolveMediaWindowDurationSeconds is a real-but-non-visibility-critical leftover from the same bug class — would recommend patching before merge for a clean fix, or filing as a follow-up if scope-bound.
— Rames Jusso (hyperframes)
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Code review
Strengths
init.test.ts:477pins the exact regression numbers (45.40 + 45.40 → 90.80) as a unit test — the comment is the best kind of commit message. Having the arithmetic of the bug visible in the test makes future archeology obvious.init.test.ts:542(auto-start guard test) correctly pins the opposite branch: auto-injecteddata-start=\"0\"+data-hf-auto-startstill usesresolveStartForElementand resolves at host time, not t=0. That's the right second test to have here.- The producer regression (
pip-video-late-host/) is exactly the right kind of coverage for a visibility/sync fix — frame-level checkpoints will catch any backslide.
Findings
blocker — init.ts:460 — resolveMediaWindowDurationSeconds has the same double-offset, untouched
The PR fixes three consumers of resolveStartForElement on media elements:
resolveStartSecondsinrefreshRuntimeMediaCache(hunk at@@ -1278)resolveDurationSecondsinrefreshRuntimeMediaCache(hunk at@@ -1285)- visibility loop (hunk at
@@ -1329)
A fourth consumer is not patched:
// init.ts:453
const resolveMediaWindowDurationSeconds = (): number | null => {
const mediaNodes = Array.from(
document.querySelectorAll("video[data-start], audio[data-start]"),
) as HTMLMediaElement[];
...
for (const node of mediaNodes) {
const start = resolveStartForElement(node, 0); // ← double-offset for pip media
...
maxWindowEndSeconds = Math.max(maxWindowEndSeconds, Math.max(0, start) + duration);
}
return maxWindowEndSeconds ...;
};This function queries the same video[data-start] set, calls resolveStartForElement unguarded, and feeds its result into resolveSafeDurationForTimeline via resolveMediaDurationFloorSeconds. For the bug's canonical case (pip video at data-start=45.40 inside host at 45.40, duration 7.06 s), this site would compute floor = 90.80 + 7.06 = 97.86 s — inflating the root timeline's calculated duration floor well past the actual 52.46 s composition end. That makes timeline duration resolution incorrect in the exact scenario the PR claims to fix.
Same contract violation, different code path. Per Rule 2 in the review rubric, a sibling site with the same failure mode is a blocker, not a follow-up.
Fix is the same guard pattern already established in this PR:
const isGlobalMediaStart = !node.hasAttribute("data-hf-auto-start");
const start = isGlobalMediaStart
? Math.max(0, Number(node.getAttribute("data-start") ?? 0) || 0)
: resolveStartForElement(node, 0);important — CSS adapter at init.ts:1632 — same unguarded resolveStartForElement for any element with CSS animations
createCssAdapter({
resolveStartSeconds: (element) => resolveStartForElement(element, 0),
}),createCssAdapter discovers ALL elements with CSS animations (document.querySelectorAll("*")). A pip <video> with a CSS animation and data-start in a sub-composition would get the double-offset start applied to its CSS animation seek. This is narrow (pip videos with authored CSS animations are uncommon) but follows the same contract. Either apply the same guard here, or add a comment explicitly documenting why this path doesn't need it (e.g. "CSS animations on media elements are authoring-time-only and don't appear inside sub-compositions").
important — Helper extraction would have prevented the missed site
The !hasAttribute("data-hf-auto-start") && hasAttribute("data-start") ? Math.max(0, Number(...) || 0) : resolveStartForElement(...) pattern is inlined at four sites across two files after this PR. A named helper — something like resolveMediaElementStartSeconds(el, resolveStartForElement) — would make the contract explicit and make it hard to add a fifth site without the guard. The blocker above is the direct consequence of not having it.
nit — Fallow audit is failing
The new resolveMediaWindowEndSeconds in timeline.ts just ticked over the CRAP-score threshold (31.6 vs. 30.0 ceiling). Not a required check gate, but the signal is real — this function now has both cyclomatic complexity and low test coverage. If you add the fix for the blocker above, consider pulling the guard into a helper to keep the function complexity flat.
Verdict: REQUEST CHANGES
Reasoning: resolveMediaWindowDurationSeconds (init.ts:460) satisfies the same contract as the three patched sites — it queries pip-wired media elements and calls resolveStartForElement unguarded — meaning the double-offset bug survives in root timeline duration computation for the exact case the PR claims to fix. All other CI checks pass (BLOCKED is reviewer-gate only).
— Vai
Fix the 4th unguarded resolveStartForElement call site in resolveMediaWindowDurationSeconds that inflated the timeline duration floor for pip compositions. Extract resolveMediaStartSeconds helper to consolidate the data-hf-auto-start guard across all call sites.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-reviewed ca39687 ("patch resolveMediaWindowDurationSeconds + extract helper"). Vai's blocker + my consolidation nit both addressed.
Verified
resolveMediaStartSecondshelper extracted atinit.ts:387— single source of truth for thedata-hf-auto-startguard.- Applied at 4 sites in
init.ts:resolveMediaWindowDurationSeconds(the blocker Vai caught) ✓resolveStartSecondscallback inrefreshRuntimeMediaCache✓resolveDurationSecondscallback in same ✓- Visibility loop, with
tag === "video" || tag === "audio"predicate before calling the helper ✓
- The visibility-loop tag predicate is the right shape — non-media elements (divs etc.) skip the helper and stay on the composition-local resolver.
- Build / Lint / Typecheck / Test / Studio smoke / CLI smoke / Perf / Preview parity all green on this commit; regression-shards + Windows still in flight but those don't usually go red post-typecheck.
Two carry-over items (non-blocking, follow-ups)
-
timeline.tscollectRuntimeTimelinePayloadstill has the inline guard, not the helper. Same logic, just not consolidated through the new helper. Cosmetic; the helper lives ininit.tsso an export + cross-file import would be needed to share. Not worth a round-trip on this PR. -
createCssAdapteratinit.ts:1632— still callsresolveStartForElement(element, 0)directly. Vai flagged this as "important" (CSS animations on pip media elements would get the double-offset applied to their animation seek). Narrow case (pip video + CSS animation is a rare combination) but if anyone wants a clean five-sites story, this is the fifth site. Easy follow-up.
Verdict
The primary fix is correct and the helper extraction is clean. The unaddressed CSS adapter case is the only remaining same-bug-class site, but it's a different surface (animation timing, not visibility/duration) and Vai's "important" grade was already non-blocking. Fine to ship as-is; CSS adapter can be a separate one-line follow-up.
— Rames Jusso (hyperframes)
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Re-reviewed after push. Both items from the prior review addressed cleanly.
Blocker resolved — resolveMediaWindowDurationSeconds (init.ts:460)
The resolveMediaStartSeconds helper introduced at init.ts:391 is the right extraction: single guard for the data-hf-auto-start / data-start pattern, now called at all four media-start consumer sites. The prior review's blocker (double-offset in resolveMediaWindowDurationSeconds) is confirmed fixed via line 467. James's re-review already verified all four application sites.
Nit resolved — helper extracted
Guard is no longer inlined 4× — resolveMediaStartSeconds is the single definition. Good.
Carry-over — CSS adapter (init.ts:1675-1677)
createCssAdapter({ resolveStartSeconds: (element) => resolveStartForElement(element, 0) }) still passes the unguarded resolver. A pip <video> with CSS animations would still get the double-offset on its animation seek. This was "important" in the prior review, not a blocker — narrow case (pip video + CSS animation), and James's re-review explicitly called it out as a clean follow-up one-liner. Deferring is fine; just want it tracked and not dropped.
CI
All required checks pass. Fallow audit fail is pre-existing complexity on the large init.ts functions, not introduced by this PR. Regression shards pending but not required.
Approving. The primary correctness fix is solid.
— Vai
Merge activity
|
4 participants
Summary
Fixes pip-wired media elements being permanently hidden in preview when their host sub-composition starts at a non-zero time. Applies the fix consistently across all three start-time consumers and adds a producer regression test.
Supersedes #1078 — based on @JamesXiaoFF's root cause analysis. Pushed to origin because the fork couldn't accept LFS objects for the regression baseline.
Root cause
resolveStartForElementadds the host composition's global start offset on top of the media element's owndata-start. When a pip video hasdata-start="45.40"inside a host atdata-start="45.40", the resolved start becomes 90.80 — well past the host's window, keeping the video permanently hidden.Media elements authored with explicit
data-startuse global coordinates (matching the render pipeline'sdiscoverMediaFromBrowserwhich reads the raw attribute). The resolver's local-to-global accumulation double-counts the offset for these elements.Fix
For media elements (video/audio) without the
data-hf-auto-startmarker, readdata-startdirectly as a global timestamp. Elements withdata-hf-auto-start(compiler-injecteddata-start="0") continue using the resolver since their timing is composition-local.Applied to all three consumers:
init.tsrefreshRuntimeMediaCachestart/duration resolution ininit.tsresolveMediaWindowEndSecondsintimeline.tsTests
Unit tests (
init.test.ts):data-start="45.40"inside host at45.40— visible at t=46, hidden at t=53 and t=44data-start="0"+data-hf-auto-startinside host at10— visible at t=12, hidden at t=5 and t=16Producer regression test (
pip-video-late-host):