From 8f97edb2b9257648f67a25aab0da64950f298644 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 28 Apr 2026 10:15:25 -0700 Subject: [PATCH] fix(hdr): filter zero-opacity elements and support overflow:hidden clip rects (#522) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(hdr): filter zero-opacity elements and support overflow:hidden clip rects in HDR compositor Two bugs in the HDR render pipeline: 1. Child data-start elements inside a parent with opacity:0 were still composited as independent layers, painting over content in later scenes. Fix: filter elements with effective opacity 0 before groupIntoLayers(). 2. CSS overflow:hidden on ancestor elements was ignored for HDR video layers, causing videos inside clipped containers (e.g. split-screen halves) to render full-frame. Fix: add clipRect to ElementStackingInfo, compute it from ancestor overflow:hidden in queryElementStacking(), and crop the source buffer to clip bounds before blitting in blitHdrVideoLayer(). Co-Authored-By: Claude Opus 4.6 (1M context) * fix(hdr): move opacity filter into blit loop to preserve hide-list correctness The previous approach filtered zero-opacity elements before groupIntoLayers(), which broke the DOM screenshot hide-list — invisible video elements' replacements weren't properly hidden from sibling layer screenshots, causing the vignelli-stacking regression. Fix: keep all elements in groupIntoLayers() for correct hide-list generation. Skip zero-opacity HDR elements only during the actual blit step with an early `continue` in the compositing loop. Co-Authored-By: Claude Opus 4.6 (1M context) * fix(hdr): route identity-matrix HDR elements through region blit for clip rect support parseTransformMatrix returns a valid matrix even for untransformed HDR elements (Chrome reports matrix(1,0,0,1,0,0)). This made the affine blit path always run, bypassing the region blit path which is the only one that applies clip rects from overflow:hidden ancestors. Fix: detect identity matrices and route them through the region path so the cropRgb48le clip logic is reachable for split-screen layouts. Co-Authored-By: Claude Opus 4.6 (1M context) * fix(hdr): handle translation-only matrices for clip rect support The previous isIdentity check only caught matrix(1,0,0,1,0,0). Elements with layout translation (e.g. right-half split at left:960px reporting matrix(1,0,0,1,960,0)) still routed through the affine path where clip rects are not applied. Fix: check for translation-only matrices (scale=1, rotation=0, any tx/ty) and route those through the region blit path. el.x/el.y from getBoundingClientRect already include the translation, so the region path handles positioning correctly. Co-Authored-By: Claude Opus 4.6 (1M context) * feat(render): auto-detect HDR from media probes, add --sdr flag Replace the --hdr opt-in model with automatic detection. When no flags are passed, the renderer probes all video/image sources and enables HDR output if any HDR color space is detected. Existing --hdr flag becomes a force override. New --sdr flag forces SDR output. Behavior matrix: (no flags) + HDR content → HDR output (no flags) + SDR content → SDR output --hdr → force HDR (defaults to HLG if no HDR sources) --sdr → force SDR (skips probing) --hdr --sdr → error Co-Authored-By: Claude Opus 4.6 (1M context) * Revert "feat(render): auto-detect HDR from media probes, add --sdr flag" This reverts commit 69fb52196f356632c3398cdc18bab394122d5eb8. * chore(hdr): simplify review fixes — remove redundant guard, add image clip warning - Remove redundant viewportMatrix.length >= 6 check (parseTransformMatrix always returns 6-element array or null) - Add clip rect warning log to blitHdrImageLayer for parity with video path Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: Claude Opus 4.6 (1M context) --- .../engine/src/services/videoFrameInjector.ts | 41 ++++++ .../engine/src/utils/hdrCompositing.test.ts | 130 ++++++++++++++++++ .../engine/src/utils/layerCompositor.test.ts | 11 +- .../src/services/renderOrchestrator.ts | 102 +++++++++++++- 4 files changed, 280 insertions(+), 4 deletions(-) create mode 100644 packages/engine/src/utils/hdrCompositing.test.ts diff --git a/packages/engine/src/services/videoFrameInjector.ts b/packages/engine/src/services/videoFrameInjector.ts index 10525a3f9..9223a87f1 100644 --- a/packages/engine/src/services/videoFrameInjector.ts +++ b/packages/engine/src/services/videoFrameInjector.ts @@ -255,6 +255,13 @@ export interface ElementStackingInfo { * Falls back to the CSS default `"50% 50%"` (center) when unset. */ objectPosition: string; + /** + * Clip rect from the nearest ancestor with `overflow: hidden` (or + * `clip`/`clip-path`). When set, the HDR compositor must scissor the + * element's blit to this viewport-relative rectangle. `null` means no + * clipping ancestor was found — render at full element bounds. + */ + clipRect: { x: number; y: number; width: number; height: number } | null; } /** @@ -357,6 +364,39 @@ export async function queryElementStacking( return [0, 0, 0, 0]; } + // Walk ancestors to find the tightest overflow:hidden clip rect. + // Returns null if no clipping ancestor exists. + function getClipRect( + node: Element, + ): { x: number; y: number; width: number; height: number } | null { + let current: Element | null = node.parentElement; + let clip: { x: number; y: number; width: number; height: number } | null = null; + while (current) { + const cs = window.getComputedStyle(current); + if (cs.overflow === "hidden" || cs.overflow === "clip") { + const r = current.getBoundingClientRect(); + const ancestor = { + x: Math.round(r.x), + y: Math.round(r.y), + width: Math.round(r.width), + height: Math.round(r.height), + }; + if (!clip) { + clip = ancestor; + } else { + // Intersect with existing clip + const x1 = Math.max(clip.x, ancestor.x); + const y1 = Math.max(clip.y, ancestor.y); + const x2 = Math.min(clip.x + clip.width, ancestor.x + ancestor.width); + const y2 = Math.min(clip.y + clip.height, ancestor.y + ancestor.height); + clip = { x: x1, y: y1, width: Math.max(0, x2 - x1), height: Math.max(0, y2 - y1) }; + } + } + current = current.parentElement; + } + return clip; + } + // Walk up the DOM multiplying each ancestor's opacity. GSAP animates // opacity on wrapper divs, not directly on the video element, so the // element's own opacity is often 1.0. Multiplying ancestors gives the @@ -472,6 +512,7 @@ export async function queryElementStacking( // can rely on a populated value. objectFit: style.objectFit || "fill", objectPosition: style.objectPosition || "50% 50%", + clipRect: isHdrEl ? getClipRect(el) : null, }); } return results; diff --git a/packages/engine/src/utils/hdrCompositing.test.ts b/packages/engine/src/utils/hdrCompositing.test.ts new file mode 100644 index 000000000..2962dec76 --- /dev/null +++ b/packages/engine/src/utils/hdrCompositing.test.ts @@ -0,0 +1,130 @@ +import { describe, expect, it } from "vitest"; +import { groupIntoLayers } from "./layerCompositor.js"; +import type { ElementStackingInfo } from "../services/videoFrameInjector.js"; + +function makeEl( + id: string, + zIndex: number, + isHdr: boolean, + overrides?: Partial, +): ElementStackingInfo { + return { + id, + zIndex, + x: 0, + y: 0, + width: 1920, + height: 1080, + layoutWidth: 1920, + layoutHeight: 1080, + opacity: 1, + visible: true, + isHdr, + transform: "none", + borderRadius: [0, 0, 0, 0], + objectFit: "cover", + objectPosition: "50% 50%", + clipRect: null, + ...overrides, + }; +} + +describe("HDR compositing — opacity filtering", () => { + it("zero-opacity elements remain in groupIntoLayers for hide-list correctness", () => { + const elements = [ + makeEl("bg", 0, false), + makeEl("v-hdr", 1, true), + makeEl("overlay", 2, false, { opacity: 0 }), + ]; + // Elements stay in layers for correct DOM screenshot hide-lists. + // The compositor skips zero-opacity HDR layers during blit. + const layers = groupIntoLayers(elements); + expect(layers).toHaveLength(3); + expect(layers[0]!.type).toBe("dom"); + expect(layers[1]!.type).toBe("hdr"); + expect(layers[2]!.type).toBe("dom"); + }); + + it("zero-opacity HDR element should be skipped during blit", () => { + const el = makeEl("v-hdr", 1, true, { opacity: 0 }); + // The compositor checks: if (layer.element.opacity <= 0) continue; + expect(el.opacity).toBe(0); + expect(el.opacity <= 0).toBe(true); + }); + + it("low but non-zero opacity HDR elements are NOT skipped", () => { + const el = makeEl("v-hdr", 1, true, { opacity: 0.1 }); + expect(el.opacity > 0).toBe(true); + }); + + it("child data-start element with parent opacity 0 has effective opacity 0", () => { + const childOverlay = makeEl("s6-text-wrap", 10, false, { opacity: 0 }); + expect(childOverlay.opacity).toBe(0); + }); + + it("DOM overlay above HDR video is in a separate layer when both visible", () => { + const elements = [makeEl("bg", 0, false), makeEl("v-hdr", 1, true), makeEl("badge", 10, false)]; + const layers = groupIntoLayers(elements); + expect(layers).toHaveLength(3); + expect(layers[0]!.type).toBe("dom"); + expect(layers[1]!.type).toBe("hdr"); + expect(layers[2]!.type).toBe("dom"); + if (layers[2]!.type === "dom") { + expect(layers[2]!.elementIds).toEqual(["badge"]); + } + }); +}); + +describe("HDR compositing — clip rect", () => { + it("clipRect is null when no overflow:hidden ancestor", () => { + const el = makeEl("video", 0, true); + expect(el.clipRect).toBeNull(); + }); + + it("clipRect constrains element bounds for split-screen", () => { + const el = makeEl("video-left", 0, true, { + x: 0, + y: 0, + width: 1920, + height: 1080, + clipRect: { x: 0, y: 0, width: 960, height: 1080 }, + }); + const cr = el.clipRect!; + const cx1 = Math.max(el.x, cr.x); + const cy1 = Math.max(el.y, cr.y); + const cx2 = Math.min(el.x + el.width, cr.x + cr.width); + const cy2 = Math.min(el.y + el.height, cr.y + cr.height); + expect(cx2 - cx1).toBe(960); + expect(cy2 - cy1).toBe(1080); + }); + + it("fully clipped element produces zero-size intersection", () => { + const el = makeEl("offscreen", 0, true, { + x: 1000, + y: 0, + width: 920, + height: 1080, + clipRect: { x: 0, y: 0, width: 960, height: 1080 }, + }); + const cr = el.clipRect!; + const cx2 = Math.min(el.x + el.width, cr.x + cr.width); + const cx1 = Math.max(el.x, cr.x); + expect(Math.max(0, cx2 - cx1)).toBe(0); + }); + + it("right-half clip produces correct source crop offset", () => { + const el = makeEl("video-right", 0, true, { + x: 960, + y: 0, + width: 1920, + height: 1080, + clipRect: { x: 960, y: 0, width: 960, height: 1080 }, + }); + const cr = el.clipRect!; + const cx1 = Math.max(el.x, cr.x); + const blitSrcX = cx1 - el.x; + expect(blitSrcX).toBe(0); + const blitW = Math.min(el.x + el.width, cr.x + cr.width) - cx1; + expect(blitW).toBe(960); + }); +}); diff --git a/packages/engine/src/utils/layerCompositor.test.ts b/packages/engine/src/utils/layerCompositor.test.ts index 013eb38a4..33e631d48 100644 --- a/packages/engine/src/utils/layerCompositor.test.ts +++ b/packages/engine/src/utils/layerCompositor.test.ts @@ -2,7 +2,12 @@ import { describe, expect, it } from "vitest"; import { groupIntoLayers } from "./layerCompositor.js"; import type { ElementStackingInfo } from "../services/videoFrameInjector.js"; -function makeEl(id: string, zIndex: number, isHdr: boolean): ElementStackingInfo { +function makeEl( + id: string, + zIndex: number, + isHdr: boolean, + overrides?: Partial, +): ElementStackingInfo { return { id, zIndex, @@ -17,6 +22,10 @@ function makeEl(id: string, zIndex: number, isHdr: boolean): ElementStackingInfo isHdr, transform: "none", borderRadius: [0, 0, 0, 0], + objectFit: "cover", + objectPosition: "50% 50%", + clipRect: null, + ...overrides, }; } diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index 834000d98..bee0a2b21 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -914,6 +914,32 @@ async function executeDiskCaptureWithAdaptiveRetry(options: { } } +/** + * Crop an rgb48le buffer to a sub-region. Returns a new Buffer containing + * only the cropped pixels. + */ +function cropRgb48le( + src: Buffer, + srcW: number, + srcH: number, + cropX: number, + cropY: number, + cropW: number, + cropH: number, +): Buffer { + const BPP = 6; + const dst = Buffer.alloc(cropW * cropH * BPP); + for (let row = 0; row < cropH; row++) { + const srcRow = cropY + row; + if (srcRow < 0 || srcRow >= srcH) continue; + const srcOff = (srcRow * srcW + cropX) * BPP; + const dstOff = row * cropW * BPP; + const copyLen = Math.min(cropW, srcW - cropX) * BPP; + if (copyLen > 0) src.copy(dst, dstOff, srcOff, srcOff + copyLen); + } + return dst; +} + /** * Blit a single HDR video layer onto an rgb48le canvas. * @@ -970,8 +996,56 @@ function blitHdrVideoLayer( const hasBorderRadius = br[0] > 0 || br[1] > 0 || br[2] > 0 || br[3] > 0; const borderRadiusParam = hasBorderRadius ? br : undefined; - if (viewportMatrix) { - // Use the full viewport transform (handles scale, rotation, translate) + // Apply ancestor overflow:hidden clip rect by constraining the blit + // bounds. For the no-transform (region) path, we crop the source + // image and adjust the destination position. For the affine path, + // clip rect support is not yet implemented (would require per-pixel + // scissor in the affine blit); log a warning and skip clipping. + let blitX = el.x; + let blitY = el.y; + let blitSrcX = 0; + let blitSrcY = 0; + let blitW = srcW; + let blitH = srcH; + let clipped = false; + + if (el.clipRect) { + const cr = el.clipRect; + const cx1 = Math.max(blitX, cr.x); + const cy1 = Math.max(blitY, cr.y); + const cx2 = Math.min(blitX + blitW, cr.x + cr.width); + const cy2 = Math.min(blitY + blitH, cr.y + cr.height); + if (cx2 <= cx1 || cy2 <= cy1) return; // fully clipped + blitSrcX = cx1 - blitX; + blitSrcY = cy1 - blitY; + blitW = cx2 - cx1; + blitH = cy2 - cy1; + blitX = cx1; + blitY = cy1; + clipped = true; + } + + // Detect translation-only matrix (no scale/rotation) — route through the + // region path which supports clip rects. Chrome reports a viewport matrix + // for all HDR elements, even untransformed ones or those with only layout + // translation (e.g. `left: 960px` → `matrix(1,0,0,1,960,0)`). The region + // blit handles translation via el.x/el.y, so we only need the affine path + // for actual scale/rotation transforms. + // parseTransformMatrix returns a 6-element array or null — length check unnecessary. + const isTranslationOnly = !!( + viewportMatrix && + Math.abs(viewportMatrix[0]! - 1) < 0.001 && + Math.abs(viewportMatrix[1]!) < 0.001 && + Math.abs(viewportMatrix[2]!) < 0.001 && + Math.abs(viewportMatrix[3]! - 1) < 0.001 + ); + + if (viewportMatrix && !isTranslationOnly) { + if (clipped && log) { + log.debug( + `HDR clip rect on affine-transformed element ${el.id} — clip not applied (affine scissor not yet supported)`, + ); + } blitRgb48leAffine( canvas, hdrRgb, @@ -983,8 +1057,22 @@ function blitHdrVideoLayer( el.opacity < 0.999 ? el.opacity : undefined, borderRadiusParam, ); + } else if (clipped) { + // Crop the source buffer to the clipped region before blitting + const croppedBuf = cropRgb48le(hdrRgb, srcW, srcH, blitSrcX, blitSrcY, blitW, blitH); + blitRgb48leRegion( + canvas, + croppedBuf, + blitX, + blitY, + blitW, + blitH, + width, + height, + el.opacity < 0.999 ? el.opacity : undefined, + borderRadiusParam, + ); } else { - // No transform — identity position, use fast region blit blitRgb48leRegion( canvas, hdrRgb, @@ -1040,6 +1128,9 @@ function blitHdrImageLayer( if (!buf) { return; } + if (el.clipRect && log) { + log.debug(`HDR clip rect on image element ${el.id} — clip not yet supported for images`); + } try { // The cache returns `buf.data` unchanged when no conversion is needed, @@ -1176,6 +1267,9 @@ async function compositeHdrFrame( ? fullStacking.filter((e) => elementFilter.has(e.id)) : fullStacking; + // Zero-opacity elements stay in the stacking for correct hide-list + // generation (their replacements must be hidden from sibling + // screenshots). The actual blit is skipped in the compositing loop below. const layers = groupIntoLayers(filteredStacking); const shouldLog = debugDumpEnabled && debugFrameIndex >= 0; @@ -1204,6 +1298,8 @@ async function compositeHdrFrame( for (const [layerIdx, layer] of layers.entries()) { if (layer.type === "hdr") { + // Skip zero-opacity HDR elements — their parent scene may have faded out. + if (layer.element.opacity <= 0) continue; const before = shouldLog ? countNonZeroRgb48(canvas) : 0; const isHdrImage = nativeHdrImageIds.has(layer.element.id); if (isHdrImage) {