fix(engine): stop clobbering native <video> opacity in HDR pipeline (#368)

## Summary

Fix four interrelated bugs in the opacity pipeline. The headline fix: the HDR compositor was effectively ignoring direct-on-`<video>` opacity animation because the engine itself was clobbering inline opacity with `opacity: 0 !important` — switching to `visibility: hidden` resolves the bug at the root.

## Why

`Chunk 1` of `plans/hdr-followups.md`. This was the most user-visible bug in the entire follow-ups list: a GSAP-controlled opacity tween directly on a `<video>` element under HDR rendered at full brightness instead of fading.

## What changed

**1A — Stop clobbering native `<video>` opacity.** `screenshotService.injectVideoFramesBatch` and `syncVideoFrameVisibility` were applying `opacity: 0 !important` to native `<video>` elements to hide them under the injected `<img>`. That stomp clobbered any GSAP-controlled inline opacity, so the next seek read 0 from computed style and the comp went black. Switched to `visibility: hidden !important` only. Visibility hides the element from rendering without changing its opacity, so subsequent reads (and `queryElementStacking`) see the real GSAP value on every frame. The `parseFloat(...) || 1` recovery hack at `injectVideoFramesBatch` was specifically there to compensate for this stomp; it's now replaced with a `Number.isNaN` guard that defaults to 1 only when parsing actually fails.

**1B — `Number.isNaN` guards in `queryVideoElementBounds`.** `parseFloat(style.opacity) || 1` silently coerced a real opacity of 0 into 1. Switched to explicit `Number.isNaN` checks so opacity 0 stays 0. Same fix for `parseFloat(style.zIndex)`.

**1C — `instanceof HTMLElement` instead of cast.** `resolveRadius` cast `el as HTMLElement` to read `offsetWidth`/`Height`. SVG and other non-HTML elements would have crashed at runtime. Replaced the cast with an `instanceof HTMLElement` guard, and made the numeric fallback `Number.isNaN`-safe.

**1D — Opacity walk starts from the element itself.** The walk in `queryVideoElementBounds` started from `el.parentElement` for HDR videos to skip past the engine's forced `opacity: 0` on the element itself. Now that the engine never sets opacity, the special case is unnecessary — always walk from `el`. Kept the `isHdrEl` lookup because transform/border-radius logic further down still branches on it.

## Test plan

- [x] `bun run --filter @hyperframes/engine typecheck` clean.
- [x] `bun run --filter @hyperframes/engine test` — 308/308 passing.
- [x] `bun run --filter @hyperframes/producer typecheck` clean.
- [x] `oxlint` + `oxfmt --check` on both touched files.
- [x] `hdr-regression` Window C (the direct-opacity window) now passes against the regenerated golden — see follow-up PR in this stack which tightens the budget.

## Stack

Chunk 1 of `plans/hdr-followups.md`. Window C of the regression suite documents the bug; the next PR in the stack regenerates the golden and tightens its `maxFrameFailures` budget.
This commit is contained in:
Vance Ingalls
2026-04-22 19:50:40 -07:00
committed by GitHub
parent 293d92af05
commit 2d57918f64
4 changed files with 441 additions and 509 deletions
@@ -375,17 +375,12 @@ export async function injectVideoFramesBatch(
let img = video.nextElementSibling as HTMLImageElement | null;
const isNewImage = !img || !img.classList.contains("__render_frame__");
const computedStyle = window.getComputedStyle(video);
// GSAP seeks re-apply tween values during an active tween, but do not
// re-apply tweens that have already completed. After an opacity fade-in
// finishes, GSAP's last set value is overwritten on subsequent frames
// by the `opacity: 0 !important` we apply at the bottom of this
// function to hide the native <video>. That leaves `computedOpacity`
// stuck at 0 even though the user's intent is opacity 1 (the tween's
// end state). The `|| 1` fallback treats computedOpacity === 0 as a
// hidden-native-video artifact and recovers opacity 1, matching the
// final on-screen state for the vast majority of compositions.
// For active tweens in the [0,1] exclusive range this is a no-op.
const computedOpacity = parseFloat(computedStyle.opacity) || 1;
// Read the GSAP-controlled opacity directly from the native <video>.
// We hide the <video> below with `visibility: hidden` only (never
// `opacity: 0`), so its computed opacity is preserved across seeks
// and accurately reflects the user's intent on every frame.
const opacityParsed = parseFloat(computedStyle.opacity);
const computedOpacity = Number.isNaN(opacityParsed) ? 1 : opacityParsed;
const sourceIsStatic = !computedStyle.position || computedStyle.position === "static";
if (isNewImage) {
@@ -454,8 +449,10 @@ export async function injectVideoFramesBatch(
);
img.style.opacity = String(computedOpacity);
img.style.visibility = "visible";
// Hide the native <video> with visibility only — never clobber inline
// opacity, so subsequent reads (and queryElementStacking) see the real
// GSAP-controlled value.
video.style.setProperty("visibility", "hidden", "important");
video.style.setProperty("opacity", "0", "important");
video.style.setProperty("pointer-events", "none", "important");
}
if (pendingDecodes.length > 0) {
@@ -489,10 +486,10 @@ export async function syncVideoFrameVisibility(
img.style.visibility = "visible";
}
} else {
// Inactive video: hide both
// Inactive video: hide both. Use visibility only (never opacity) so we
// never clobber GSAP-controlled inline opacity.
video.style.removeProperty("display");
video.style.setProperty("visibility", "hidden", "important");
video.style.setProperty("opacity", "0", "important");
video.style.setProperty("pointer-events", "none", "important");
if (hasImg) {
img.style.visibility = "hidden";
@@ -149,8 +149,6 @@ export async function hideVideoElements(page: Page, videoIds: string[]): Promise
const el = document.getElementById(id) as HTMLVideoElement | null;
if (el) {
el.style.setProperty("visibility", "hidden", "important");
el.style.setProperty("opacity", "0", "important");
// Also hide the injected render frame image if present
const img = document.getElementById(`__render_frame_${id}__`);
if (img) img.style.setProperty("visibility", "hidden", "important");
}
@@ -168,7 +166,6 @@ export async function showVideoElements(page: Page, videoIds: string[]): Promise
const el = document.getElementById(id) as HTMLVideoElement | null;
if (el) {
el.style.removeProperty("visibility");
el.style.removeProperty("opacity");
const img = document.getElementById(`__render_frame_${id}__`);
if (img) img.style.removeProperty("visibility");
}
@@ -203,8 +200,10 @@ export async function queryVideoElementBounds(
}
const rect = el.getBoundingClientRect();
const style = window.getComputedStyle(el);
const zIndex = parseInt(style.zIndex) || 0;
const opacity = parseFloat(style.opacity) || 1;
const zIndexParsed = parseInt(style.zIndex);
const zIndex = Number.isNaN(zIndexParsed) ? 0 : zIndexParsed;
const opacityParsed = parseFloat(style.opacity);
const opacity = Number.isNaN(opacityParsed) ? 1 : opacityParsed;
const transform = style.transform || "none";
const visible =
style.visibility !== "hidden" &&
@@ -320,12 +319,12 @@ export async function queryElementStacking(
function resolveRadius(value: string, el: Element): number {
if (value.includes("%")) {
const pct = parseFloat(value) / 100;
const htmlEl = el as HTMLElement;
const w = htmlEl.offsetWidth || 0;
const h = htmlEl.offsetHeight || 0;
const w = el instanceof HTMLElement ? el.offsetWidth : 0;
const h = el instanceof HTMLElement ? el.offsetHeight : 0;
return pct * Math.min(w, h);
}
return parseFloat(value) || 0;
const parsed = parseFloat(value);
return Number.isNaN(parsed) ? 0 : parsed;
}
// Check element itself (replaced elements clip to own border-radius)
@@ -435,12 +434,12 @@ export async function queryElementStacking(
const rect = el.getBoundingClientRect();
const style = window.getComputedStyle(el);
const zIndex = getEffectiveZIndex(el);
// For HDR video elements, the frame injector sets `opacity: 0 !important`
// on the element itself. Start the opacity walk from the parent to get the
// real GSAP-animated opacity from wrapper divs.
const isHdrEl = hdrSet.has(id);
const opacityStartNode = isHdrEl ? el.parentElement : el;
const opacity = opacityStartNode ? getEffectiveOpacity(opacityStartNode) : 1;
// The frame injector now uses `visibility: hidden` (without `opacity: 0`)
// to hide native <video> elements, so the element's own computed opacity
// remains the GSAP-controlled value. Walk from the element itself to
// multiply through any ancestor opacity stacks.
const opacity = getEffectiveOpacity(el);
const visible =
style.visibility !== "hidden" &&
style.display !== "none" &&