fix(engine): skip video frame injection when a visual ancestor is hidden#1028
fix(engine): skip video frame injection when a visual ancestor is hidden#1028lirian-su-opus wants to merge 2 commits into
Conversation
`injectVideoFramesBatch` and `syncVideoFrameVisibility` iterate every `video[data-start]` whose raw time window covers the current seek. Inner `<video>` elements inside `[data-composition-src]` sub-compositions get `data-start="0"` auto-injected by `compileTimingAttrs` and probed-duration cover the entire timeline, so they look "active" even when their host has not yet started. When the runtime then hides the host with `visibility: hidden` (its out-of-window lifecycle), the inner video inherits hidden via the CSS cascade — but our injector responded by painting a replacement `<img class="__render_frame__" style="visibility: visible">` next to the video. `visibility: visible` on the descendant defeats the parent `visibility: hidden` cascade, and because the host has not been morphed by GSAP yet the video's bounding box is its CSS default (usually full-bleed). The result is one full-bleed frame per inactive sub-comp painted over whichever moment is *actually* visible — the overlay symptom the upstream agentic-finecut project saw. Walk ancestors in both functions; if any has `display: none` or `visibility: hidden`, skip the inject and hide any stale `__render_frame__` sibling. The render is now correctly empty for hidden hosts, which is what the surrounding CSS cascade already intends. Tests: - `screenshotService.test.ts`: cover the new guard for both visibility:hidden and display:none hosts, both for the fresh-img and the stale-img paths, plus `syncVideoFrameVisibility` for the case where the time window calls a video "active" but a hidden ancestor still requires its frame to stay hidden. Each test fails against pre-fix `screenshotService.ts`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The screenshotService.test.ts regression-suite comment pointed at the author's fork branch as backstory. Strip the line so upstream code doesn't carry a fork-relative reference; the surrounding paragraph already explains the bug end-to-end without it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
miguel-heygen
left a comment
There was a problem hiding this comment.
style-9-prod regression — root cause found
The isVisualAncestorHidden check is too broad. It catches a legitimate rendering scenario in style-9-prod where the runtime's [data-start] lifecycle hides a composition container whose GSAP timeline is shorter than its authored data-duration.
What happens
In style-9-prod, the #aroll-comp element:
- Has
data-composition-id="aroll-layer",data-start="0",data-duration="16.04" - But its GSAP timeline only tweens up to 9.88s
- The runtime truncates visible duration to
Math.min(16.04, 9.88) = 9.88, settingvisibility: hiddenafter t=9.88s
Before this PR: The replacement <img> had visibility: visible which correctly overrides ancestor visibility: hidden (CSS spec: child visibility: visible overrides parent visibility: hidden). The video held its final GSAP state.
After this PR: The ancestor walk finds #aroll-comp with visibility: hidden and skips injection entirely → 38 blank frames from t=9.95s to end (PSNR drops from ~50 to ~11).
Fix
The check should only trigger on sub-composition hosts ([data-composition-src] or [data-composition-file]) — which was the original intent of the PR. Regular [data-start] containers with visibility: hidden are handled correctly by CSS visibility inheritance (the <img>'s explicit visibility: visible overrides the ancestor).
Replace:
const isVisualAncestorHidden = (el: HTMLElement): boolean => {
let parent = el.parentElement;
while (parent !== null && parent !== document.documentElement) {
const computed = window.getComputedStyle(parent);
if (computed.display === "none" || computed.visibility === "hidden") return true;
parent = parent.parentElement;
}
return false;
};With:
const isVisualAncestorHidden = (el: HTMLElement): boolean => {
let parent = el.parentElement;
while (parent !== null && parent !== document.documentElement) {
const computed = window.getComputedStyle(parent);
if (computed.display === "none") return true;
// Only treat visibility:hidden as a skip signal on sub-composition
// hosts. For regular [data-start] containers the replacement <img>'s
// explicit `visibility: visible` correctly overrides the ancestor per
// CSS spec — we must NOT skip injection for those.
if (
computed.visibility === "hidden" &&
(parent.hasAttribute("data-composition-src") || parent.hasAttribute("data-composition-file"))
) return true;
parent = parent.parentElement;
}
return false;
};Both copies of isVisualAncestorHidden (in injectVideoFramesBatch and syncVideoFrameVisibility) need this same change.
What
In
packages/engine/src/services/screenshotService.ts, teachinjectVideoFramesBatchandsyncVideoFrameVisibilityto walk the video's ancestors and treat anydisplay: none/visibility: hiddenhost as a signal to skip painting the replacement<img class="__render_frame__">. Stale__render_frame__siblings left from a previous seek are explicitly flipped tovisibility: hiddenso they don't bleed onto the visible host. Add a regression suite inscreenshotService.test.ts.Why
The frame injector iterates every
video[data-start]whose raw[data-start, data-start + data-duration)time window covers the current seek and overlays the extracted source-video frame on each one. That contract works for top-level<video>clips but is wrong for<video>elements nested insidedata-composition-srcsub-compositions:compileTimingAttrsauto-injectsdata-start="0"and adata-hf-auto-startmarker on any<video>without timing attrs. The duration prober then resolvesdata-endto the full source duration. So an inner PIP<video>covers the entire timeline in the active check, regardless of which moment its host actually belongs to.[data-start]lifecycle hides out-of-window hosts withvisibility: hidden. The cascade hides the nested<video>'s rendered output — but the injector still creates an<img>next to it and explicitly setsimg.style.visibility = "visible", which under CSS rules defeats the parentvisibility: hidden.<video>'s used box is the CSS default — typically full-bleed.The net effect: every inactive sub-comp with a nested
<video>paints one full-bleed replacement frame on top of whichever moment is currently visible. In the agentic-finecut consumer this looked like the speaker chip stretched fullscreen over Zuckerberg + Meta artwork, which is exactly the symptom that initially looked like the runtime had failed to seek at all. The runtime had in fact seeked correctly; the page-side painter was clobbering the viewport.How
Walk parents from the
<video>up todocument.documentElementand checkgetComputedStyle(parent).display === "none"or=== "hidden". Reading the video's own computed visibility is unreliable because both functions writevisibility: hidden !importantto the<video>themselves, so we deliberately start the walk atparentElement.If any ancestor is hidden:
injectVideoFramesBatchshort-circuits per video: hide a stale__render_frame__sibling if one exists, thencontinue. Don't create a new<img>, don't read computed style, don't paint anything.syncVideoFrameVisibilityadds the same check inside theactive.has(video.id)branch and falls through to the inactive arm when the ancestor is hidden. The<img>lands onvisibility: hidden, matching what CSS already implies for the rest of the subtree.The helper is inlined in both functions instead of hoisted to a shared module because the bodies run inside
page.evaluateand we don't want to maintain a cross-file string-serialized helper. Both copies are identical; if they ever drift we'd catch it in the regression tests, which exercise both paths.A few alternatives I rejected:
snapshot.ts,producer/services/videoFrameInjector.ts). Two equivalent implementations would have to stay in sync, and any future caller would have to remember to filter.data-start/data-endand recompute active per-video. Couples the injector to the timing convention; doesn't generalize to hosts hidden for other authoring reasons (modal overlays, accessibility toggles, etc.).<video>elements when the host is out of window. Same outcome but a much bigger blast radius — the injector is the layer that knows it's about to override the cascade, so it's the natural enforcement point.Test plan
Added a
video-frame injection respects ancestor visibilityblock toscreenshotService.test.tswith four cases:visibility: hiddenhost →injectVideoFramesBatchcreates no<img>next to the video.display: nonehost → same.__render_frame__<img>left over from a previous seek when the host was visible →injectVideoFramesBatchflips it tovisibility: hidden.syncVideoFrameVisibilitycalled with the video in theactiveset but the ancestor hidden → the existing<img>stays hidden, not "visible".Each test follows the existing
injectVideoFramesBatch replacement layoutpattern: linkedom DOM, a per-elementgetComputedStylestub backed by aMap<Element, StyleLike>, and a pass-throughpage.evaluatethat runs the implementation directly under Node.I verified every new test fails against pre-fix
screenshotService.tsby stashing the source change and re-running — 4 failures, 5 existing tests still green. With the fix re-applied, all 9 tests pass.bun run --filter @hyperframes/engine teston this branch:613/616 passed. The three failures are inffprobe.test.ts(HDR PNG cICP metadata parsing) and reproduce identically onmain— they are unrelated to this change.hyperframes renderrun, confirmed the visible host's content shows through after the fix; verifiedbunx oxlint/bunx oxfmt --checkand the engine test suite minus the pre-existing failures)