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

[vanceingalls]

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

• 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.
• 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.

[vanceingalls]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• docs(engine): document __name polyfill and add regression test #385
• perf(producer): cache transfer-converted hdr image buffers per render job #384
• perf(producer): gate per-frame debug meta via optional isLevelEnabled #383
• perf(producer): hdr benchmark harness — --tags filter, peak heap/RSS tracking, bench:hdr script #382
• test(producer): extract frameDirMaxIndexCache to its own module and pin cross-job isolation #381
• test(engine): cover spawnStreamingEncoder lifecycle and cleanup paths #380
• test(engine): add ffprobe-unavailable fallback regression tests #379
• test(shader-transitions): add midpoint (p=0.5) regression invariants for all shaders #378
• test(engine): lock down sRGB→BT.2020 LUT with byte-exact reference values #377
• build(lfs): track tests/*/src/*.png via Git LFS #376
• test(hdr-regression): tighten Window F maxFrameFailures budget after Chunk 4 fix #375
• fix(engine,shader): handle matrix3d transforms and hide non-first scenes #374
• refactor(producer): extract HDR compositing helpers and rename media metadata #373
• fix(producer): wire --crf and --video-bitrate CLI overrides into encoders #372
• fix(producer): tighten resource lifecycle and harden file server #371
• feat(engine): wire options.hdr through chunkEncoder + dynamic SDR→HDR transfer #370
• test(hdr-regression): tighten Window C maxFrameFailures budget after Chunk 1 fix #369
• fix(engine): stop clobbering native <video> opacity in HDR pipeline #368 👈 (View in Graphite)
• refactor(shader-transitions): extract DEFAULT_DURATION and DEFAULT_EASE constants #367
• refactor(types): tighten type safety, dedupe HfTransitionMeta, prune dead LUT export #366
• test(producer): add hdr-regression and hdr-hlg-regression test suites #365
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[jrusso1020]

Fix is at the right layer and the root-cause analysis in the PR body is what I'd expect from a staff-eng writeup. Three things I want to call out explicitly:

Fix is structural, not a workaround. Hiding <video> via visibility: hidden instead of opacity: 0 !important is the correct primitive — visibility removes the element from the render tree without touching its computed opacity, so GSAP-controlled opacity stays readable on every frame. The previous code was compensating for its own stomp with parseFloat(...) || 1, which silently coerced legitimate opacity 0 into 1 and masked the bug for anyone not specifically testing direct-on-<video> fades. Replacing that compensator with Number.isNaN-guarded defaults (1B) is the right tightening — it makes the code tell the truth about its inputs instead of papering over them.

1C is a real bug too. resolveRadius casting el as HTMLElement would have runtime-crashed on <svg> or any non-HTML element in the element stack. instanceof HTMLElement is the right shape because it doesn't just narrow the type, it checks the actual prototype chain. Good defensive fix.

1D is correctly downstream of 1A. The walk-from-parent in queryVideoElementBounds existed only to route around the engine's own opacity stomp. Now that the stomp is gone, walking from the element itself is what you'd naively expect from the start. Leaving the isHdrEl lookup intact is the right call — the transform/border-radius branches below still need it for unrelated reasons.

Non-blocking things worth eyeballing:

1. CI shows 4 failures including regression-shards (fast) and styles-b. Worth checking whether any of those are caused by this change — a test that was passing only because opacity: 0 happened to hide some element would now fail because visibility: hidden preserves layout-box but removes from paint. Specifically: any composition where a hidden <video>'s zero-opacity was previously masking pointer-events, stacking ambiguity, or sibling visibility expectations could flip. If those failures are in compositions without native HDR video, the suspicion deserves one more trace.

2. Equivalence caveat: visibility: hidden and opacity: 0 differ on one thing that matters for us here — visibility doesn't create a new stacking context, opacity: 0 does (when opacity < 1 and the element is positioned). For a native <video> that's opacity-1 → 0 tweened by GSAP, you're now relying on the DOM's natural stacking order without the opacity-induced stacking context. Worth mentally verifying that nothing in queryElementStacking was implicitly depending on the video element being its own stacking-context parent.

3. Tests: the PR body notes 308/308 engine tests pass, but the producer-side hdr-regression Window C golden is regenerated in the next PR (#369). I'd have liked to see at least one inline test here for queryVideoElementBounds that pins the new opacity-walk-from-self behavior directly — the regression suite is great at catching pixel-level regressions but a unit test is faster for bisecting future breakage. Not blocking.

Approved.

— Rames Jusso

[vanceingalls]

Thanks. Three notes addressed:

1. CI failures. fast and styles-b finished after you reviewed and both passed; only regression-shards (hdr) is red, which is exactly the Window C re-baseline this PR documents and #369 lands. So the failure cascade is bounded to the HDR golden, not a side-effect of visibility: hidden flipping a non-HDR composition's stacking/pointer-events expectations.

2. Stacking context implications of visibility: hidden. Verified — queryElementStacking does not depend on the video element being its own stacking-context parent. The getEffectiveZIndex walk only considers explicit z-index on positioned (non-static) ancestors and explicitly does NOT detect implicit stacking contexts from opacity < 1, transform, filter, will-change, isolation, or mix-blend-mode (see the long comment block at videoFrameInjector.ts:283-298). The compositor was never relying on the opacity: 0-induced stacking context to anchor the video — it was working in spite of it. Switching to visibility: hidden is strictly cleaner: same hide-from-paint, no stacking-context side effect, and the GSAP-controlled opacity now reads back through getEffectiveOpacity (which walks from el itself, multiplying ancestor opacities) on every frame.

3. Inline unit test for queryVideoElementBounds. Fair call. The function runs inside page.evaluate against a live Puppeteer DOM, so a true unit test needs JSDOM-with-DOMMatrix or a thin Puppeteer harness — non-trivial setup that's why the existing engine suite leans on the regression goldens for this layer. Tracking it in plans/hdr-followups.md as a follow-up rather than blocking on it here. The behavioral pin you wanted (opacity-walk-from-self preserving GSAP-controlled <video> opacity) is now caught end-to-end by the regenerated Window C in #369 — slower to bisect than a unit test, but correct.

Approving observations are non-blocking, so landing as-is.