Skip to content

Commit c988cbe

Browse files
committed
fix(engine): stop clobbering native <video> opacity in HDR pipeline
Four related bugs in the opacity pipeline that interact with HDR video compositing and GSAP-controlled fades: 1A. 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. We now use `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. queryVideoElementBounds parsed `style.opacity` and `style.zIndex` with `parseFloat(...) || N`, which silently coerces a real opacity of 0 into 1 (and zIndex 0 into 0 only by coincidence). Switched to explicit `Number.isNaN` checks so opacity 0 stays 0. 1C. 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. The opacity 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. Verified: - bun run --filter @hyperframes/engine typecheck (clean) - bun run --filter @hyperframes/engine test (308/308 passing) - bun run --filter @hyperframes/producer typecheck (clean) - oxlint + oxfmt --check on both touched files Next: regenerate hdr-regression window C (the opacity-fade window) and tighten its maxFrameFailures budget — done in a follow-up commit so the golden churn is reviewable separately from the code fix.
1 parent e56820c commit c988cbe

2 files changed

Lines changed: 24 additions & 28 deletions

File tree

packages/engine/src/services/screenshotService.ts

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -375,17 +375,12 @@ export async function injectVideoFramesBatch(
375375
let img = video.nextElementSibling as HTMLImageElement | null;
376376
const isNewImage = !img || !img.classList.contains("__render_frame__");
377377
const computedStyle = window.getComputedStyle(video);
378-
// GSAP seeks re-apply tween values during an active tween, but do not
379-
// re-apply tweens that have already completed. After an opacity fade-in
380-
// finishes, GSAP's last set value is overwritten on subsequent frames
381-
// by the `opacity: 0 !important` we apply at the bottom of this
382-
// function to hide the native <video>. That leaves `computedOpacity`
383-
// stuck at 0 even though the user's intent is opacity 1 (the tween's
384-
// end state). The `|| 1` fallback treats computedOpacity === 0 as a
385-
// hidden-native-video artifact and recovers opacity 1, matching the
386-
// final on-screen state for the vast majority of compositions.
387-
// For active tweens in the [0,1] exclusive range this is a no-op.
388-
const computedOpacity = parseFloat(computedStyle.opacity) || 1;
378+
// Read the GSAP-controlled opacity directly from the native <video>.
379+
// We hide the <video> below with `visibility: hidden` only (never
380+
// `opacity: 0`), so its computed opacity is preserved across seeks
381+
// and accurately reflects the user's intent on every frame.
382+
const opacityParsed = parseFloat(computedStyle.opacity);
383+
const computedOpacity = Number.isNaN(opacityParsed) ? 1 : opacityParsed;
389384
const sourceIsStatic = !computedStyle.position || computedStyle.position === "static";
390385

391386
if (isNewImage) {
@@ -454,8 +449,10 @@ export async function injectVideoFramesBatch(
454449
);
455450
img.style.opacity = String(computedOpacity);
456451
img.style.visibility = "visible";
452+
// Hide the native <video> with visibility only — never clobber inline
453+
// opacity, so subsequent reads (and queryElementStacking) see the real
454+
// GSAP-controlled value.
457455
video.style.setProperty("visibility", "hidden", "important");
458-
video.style.setProperty("opacity", "0", "important");
459456
video.style.setProperty("pointer-events", "none", "important");
460457
}
461458
if (pendingDecodes.length > 0) {
@@ -489,10 +486,10 @@ export async function syncVideoFrameVisibility(
489486
img.style.visibility = "visible";
490487
}
491488
} else {
492-
// Inactive video: hide both
489+
// Inactive video: hide both. Use visibility only (never opacity) so we
490+
// never clobber GSAP-controlled inline opacity.
493491
video.style.removeProperty("display");
494492
video.style.setProperty("visibility", "hidden", "important");
495-
video.style.setProperty("opacity", "0", "important");
496493
video.style.setProperty("pointer-events", "none", "important");
497494
if (hasImg) {
498495
img.style.visibility = "hidden";

packages/engine/src/services/videoFrameInjector.ts

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -149,8 +149,6 @@ export async function hideVideoElements(page: Page, videoIds: string[]): Promise
149149
const el = document.getElementById(id) as HTMLVideoElement | null;
150150
if (el) {
151151
el.style.setProperty("visibility", "hidden", "important");
152-
el.style.setProperty("opacity", "0", "important");
153-
// Also hide the injected render frame image if present
154152
const img = document.getElementById(`__render_frame_${id}__`);
155153
if (img) img.style.setProperty("visibility", "hidden", "important");
156154
}
@@ -168,7 +166,6 @@ export async function showVideoElements(page: Page, videoIds: string[]): Promise
168166
const el = document.getElementById(id) as HTMLVideoElement | null;
169167
if (el) {
170168
el.style.removeProperty("visibility");
171-
el.style.removeProperty("opacity");
172169
const img = document.getElementById(`__render_frame_${id}__`);
173170
if (img) img.style.removeProperty("visibility");
174171
}
@@ -203,8 +200,10 @@ export async function queryVideoElementBounds(
203200
}
204201
const rect = el.getBoundingClientRect();
205202
const style = window.getComputedStyle(el);
206-
const zIndex = parseInt(style.zIndex) || 0;
207-
const opacity = parseFloat(style.opacity) || 1;
203+
const zIndexParsed = parseInt(style.zIndex);
204+
const zIndex = Number.isNaN(zIndexParsed) ? 0 : zIndexParsed;
205+
const opacityParsed = parseFloat(style.opacity);
206+
const opacity = Number.isNaN(opacityParsed) ? 1 : opacityParsed;
208207
const transform = style.transform || "none";
209208
const visible =
210209
style.visibility !== "hidden" &&
@@ -320,12 +319,12 @@ export async function queryElementStacking(
320319
function resolveRadius(value: string, el: Element): number {
321320
if (value.includes("%")) {
322321
const pct = parseFloat(value) / 100;
323-
const htmlEl = el as HTMLElement;
324-
const w = htmlEl.offsetWidth || 0;
325-
const h = htmlEl.offsetHeight || 0;
322+
const w = el instanceof HTMLElement ? el.offsetWidth : 0;
323+
const h = el instanceof HTMLElement ? el.offsetHeight : 0;
326324
return pct * Math.min(w, h);
327325
}
328-
return parseFloat(value) || 0;
326+
const parsed = parseFloat(value);
327+
return Number.isNaN(parsed) ? 0 : parsed;
329328
}
330329

331330
// Check element itself (replaced elements clip to own border-radius)
@@ -435,12 +434,12 @@ export async function queryElementStacking(
435434
const rect = el.getBoundingClientRect();
436435
const style = window.getComputedStyle(el);
437436
const zIndex = getEffectiveZIndex(el);
438-
// For HDR video elements, the frame injector sets `opacity: 0 !important`
439-
// on the element itself. Start the opacity walk from the parent to get the
440-
// real GSAP-animated opacity from wrapper divs.
441437
const isHdrEl = hdrSet.has(id);
442-
const opacityStartNode = isHdrEl ? el.parentElement : el;
443-
const opacity = opacityStartNode ? getEffectiveOpacity(opacityStartNode) : 1;
438+
// The frame injector now uses `visibility: hidden` (without `opacity: 0`)
439+
// to hide native <video> elements, so the element's own computed opacity
440+
// remains the GSAP-controlled value. Walk from the element itself to
441+
// multiply through any ancestor opacity stacks.
442+
const opacity = getEffectiveOpacity(el);
444443
const visible =
445444
style.visibility !== "hidden" &&
446445
style.display !== "none" &&

0 commit comments

Comments
 (0)