Skip to content

fix(producer): HDR exports keep overlays above HDR video and apply CSS filters - #5728

Closed
miguel-heygen wants to merge 12 commits into
mainfrom
fix/hdr-export-overlays-and-filters
Closed

miguel-heygen wants to merge 12 commits into
mainfrom
fix/hdr-export-overlays-and-filters

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

What

HDR exports of a sub-composition that mixes HLG/PQ clips with SDR clips, HTML overlays and CSS filters now match the preview:

  • Overlays stay on top. Untimed content that paints above an HDR video (a later sibling of it or of an ancestor, or one with a higher z-index) is now its own layer above that video. Before, it stayed in its timed host's DOM layer, which sorted below the HDR video, so the full-frame HDR blit painted over every overlay.
  • CSS filters apply. The HDR blit can't apply CSS filters. So when a filter applies to an HDR video or any ancestor, that video now renders through the SDR path: its frames are tone-mapped to SDR, and the filter applies in the page as in the preview. The render logs one line, for example: HDR video "hf-video-0" renders through the SDR path: CSS filter brightness(0.5) applies to it. The export stays HDR.

Before this change, such an export showed only raw video frames and reported success.

How

  • queryElementStacking (engine) adds a stacking entry for each untimed element that paints above an HDR element, placed right after it. It uses the compositor's existing rule: higher effective z-index, or equal and later in tree order. Injected render-frame images and color-grading canvases are excluded. The entry is addressed by a data-hf-layer-unit attribute, so author ids are never rewritten. applyDomLayerMask can show or hide those entries like any other layer element.
  • The HDR capture stage seeks to each native HDR video's start and reads the computed filter of the video and its ancestors (findCssFilteredVideos). Filtered videos are re-extracted tone-mapped (toneMapVideosToSdr, which reuses the extract stage's options) and swapped into the frame lookup. They are then left out of the native HDR set before HDR resources are planned.
  • The frame injector's skip-if-unchanged cache now compares frame paths, not frame indexes, so swapped frames are always injected.

Verification

fails without the fix: hdr-overlay-filter-regression (100 of 100 frames on main) and screenshotService.test.ts > applyDomLayerMask keeps the render frame of an SDR video shown over an HDR video under a hidden ancestor (red on the old mask loop, green now).

  • New regression fixture hdr-overlay-filter-regression: a generated 10-bit HLG clip and an SDR clip in a sub-composition, an untimed title, a logo <img>, a progress bar, and brightness(0.5) on the video container. Golden generated in Docker; frames checked by eye: overlays on top, the HLG clip dimmed, progress at 1/4 and 3/4.
    • On main it fails: 100 of 100 frames, PSNR 15.18.
    • With the filter check disabled it fails on the HLG half: 50 frames, PSNR 19.17.
    • With this branch it passes.
  • New videoFrameInjector.stacking.browser.test.ts (real Chrome): the overlay becomes a layer above the HDR video, and the mask shows it only in that layer. Both tests fail on main.
  • engine and producer tsc are clean, as are oxlint, oxfmt and the comment ratchet. Injector and stacking engine tests pass 3 runs in a row (49 tests).
  • End to end with the built CLI on a 4K-free version of the reported project shape: overlays visible at 1 s (HLG) and 5 s (SDR), filter applied, output still hevc / yuv420p10le / arib-std-b67.

@github-actions

github-actions Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 271 (base branch 271), smooth 110 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

Unstable (1)

  • seqplainkeys-none-pct-r0-root-z100: tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo false, teleport true / tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true / tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes at 776c5697. The overlay layers and the filter path are sound outside transitions, but during a shader transition the new overlay layers are hidden.

Blocking: during a shader transition, the overlay units are masked out of both scene captures.

  • Cause: captureHdrStage.ts builds sceneElements once, before the frame loop, from the [id] elements under each .scene. The new stacking entries get generated ids (hf-layer-unit-N, from queryElementStacking), so they are never in sceneIds. In captureHdrFrameShared.ts, captureSceneIntoBuffer hides every stacking id where !sceneIds.has(id) || nativeHdrIds.has(id), and applyDomLayerMask then sets inline visibility:hidden !important on the overlay.
  • Before this PR: the same overlay was a plain descendant of its timed host. It was shown through #host * and painted on top of the scene's HDR blit.
  • When it breaks: a sub-composition host with class="scene" that holds an HDR clip and an untimed title, which is the shape this PR targets, plus a shader transition. On every transition frame, the title's own box and text disappear. Its child elements stay visible, because inline-hidden overrides the stylesheet only on the element itself. That cascade was confirmed in headless Chrome: unit hidden, child <span>/<img> visible. No full render was run.
  • Fix: count a unit as part of a scene when its element is inside that scene. Either record unit ids per scene, or check scene.contains(unitEl) when building the masks.
  • Why the suite misses it: hdr-regression window D doesn't catch this, because its labels inside scenes are timed.

Non-blocking

  • The new end-to-end fixture never exercises the overlay-layer code. Its only HDR clip has brightness(0.5), so it moves to the SDR path, the native HDR set is empty, and no units are created. Overlay ordering is covered only by videoFrameInjector.stacking.browser.test.ts. A fixture with an unfiltered HDR clip under an overlay would cover the end-to-end path.
  • The tone-mapped clip's colour may not match its SDR neighbours. In a mixed HDR timeline, SDR clips go through colorspace=all=bt2020:iall=bt709. The re-extracted filtered clip gets only the tone-map and the setparams passthrough, with no primaries conversion, so its saturation may differ from the SDR clips beside it. A golden generated by the same code wouldn't catch that.
  • The filter check runs only at the clip's start time (findCssFilteredVideos). A filter that GSAP adds later is missed, and a filter removed later still forces the whole clip to SDR. HDR images with CSS filters aren't covered. A comment or a log line would help.
  • The children of an overlay are painted twice. They still match #host * in the host's layer and are drawn again in their own layer, so semi-transparent edges outside the video blend twice. The same leak already exists for children of timed elements.

Checked and fine

  • The frame injector's skip cache is now keyed by frame path. That's required, because FrameLookupTable.addVideo replaces entries by videoId, and the SDR frames go to their own __hyperframes_video_frames_sdr directory.
  • toneMapVideosToSdr reuses extractionOptions, overriding only toneMapHdrToSdr and outputDir. Setting hdrToSdrTransformKey turns off deferred range extraction, so nothing is left undriven. It keeps the same failure policy.
  • planHdrResources now runs after the filtered clips leave the native set. hasHdrContent still uses the probed set, so the output stays HDR.
  • Units are spliced right after their HDR entry. groupIntoLayers is a stable z-index sort, so equal-z units stay above the video. Injected frames and grading canvases are excluded, and so are timed siblings.
  • The SDR path is unchanged. applyDomLayerMask only adds a third lookup, [data-hf-layer-unit=…], for elements that carry that attribute.

CI is the only run. Not armed or merged.

— Rames

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Source review at 776c56975d9b9a1e7babde1d61b2d15fd8e12c7b against effective base b1cf6e3c517e227d1dca1b212e7637c19e502a64.

The path-based injection cache (videoFrameInjector.ts:227) and moving HDR resource planning after demotion (captureHdrStage.ts:231–240) are sensible. There is one additional source-selection bug below.

Additional blocker [P2]: packages/producer/src/services/render/stages/extractVideosStage.ts:567–569 rematerializes the SDR replacement into the original extraction's staging directory when the external extraction cache is enabled (the default). The unchanged materializer uses only compiledDir/__hyperframes_video_frames/<videoId> as its destination, skips staging if it already exists, then unconditionally remaps the new result's frame paths there (render/shared.ts:646–668). The first extraction has already populated that destination. Although the second extraction has a distinct tone-map cache key, frameLookup.addVideo therefore installs paths to the original, non-tone-mapped extraction, while the capture stage removes this video from native HDR compositing. Use a transform-specific staging destination or safely replace the staged source before remapping. Cover two cache-backed extractions of the same video ID with different transforms. Cache-disabled extraction remains inside the dedicated SDR directory and avoids this collision; comparing paths in the injector cannot fix a remap that collapses them back to the original paths.

Existing blocker: I independently verified the synthetic-overlay/transition membership mismatch already reported by Rames in review 5555154773; no duplicate inline finding. Attribute-only unit IDs are absent from the scene's authored-ID snapshot and explicitly hidden during scene capture. Both follow-up commits leave this predicate and the staging collision intact.

Audited: all eight changed existing BASE files end-to-end before the full diff; complete HEAD extraction stage, HDR capture stage, injector and new browser-test source; changed fixtures/schedule and integration hunks; bound cache/materializer/lookup/mask/transition producer-consumer paths. Checked interactions among synthetic units, masks, filter demotion, re-extraction and path caching.
Trusting / limits: remaining HEAD context reviewed through full BASE plus complete diff, not a second full direct HEAD read; unchanged integration sources read in relevant ranges. LFS pointers only, not media/golden payloads. Source-only: no renderer/browser/native execution, tests or CI queries; no claim of measured pixel output.

— tai

Verdict: REQUEST CHANGES
Reasoning: The default cache-backed path can serve the old extraction after SDR demotion, and the independently confirmed transition-overlay blocker remains unresolved.

throwIfEncoderInterrupted(result);
const failure = applyVideoExtractionFailurePolicy(result, extractionPolicy, log);
if (failure) throw failure;
materializeExtractedFramesForCompiledDir(result.extracted, compiledDir, {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Keep the SDR replacement's staged source distinct. With the default external extraction cache, the initial extraction has already staged __hyperframes_video_frames/<videoId>. materializeExtractedFramesForCompiledDir skips an existing destination but still rewrites this new cache result's paths to it (render/shared.ts:646–668). frameLookup.addVideo then serves the original non-tone-mapped files even though captureHdrStage demotes the video out of native HDR. The _sdr outputDir only avoids this when extraction is not returned from the external cache. Use a transform-specific destination or safely replace/repoint the staged source, and cover two cache-backed extractions for the same video ID with distinct transforms.

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Both blockers fixed at 82591ef2.

Overlay units in shader transitions (Rames, also confirmed by tai). Unit ids are minted per frame, after the scene map is read, so captureSceneIntoBuffer now adds the units inside the scene before it builds the mask. The engine owns the lookup (layerUnitsInside(page, ids), next to the mask code), since it owns data-hf-layer-unit. A unit counts for a scene when it, or an ancestor, is one of that scene's elements. Tests:

  • the stacking browser test checks the unit is found under host and not under hdr;
  • captureHdrFrameShared.dom-mask.test.ts checks a unit in scene A is shown in A's mask and not hidden. It fails on the previous head.

SDR re-extraction served the cached original (tai). Agreed: the materializer staged by video id alone, so a second extraction of the same clip was remapped onto the first one's folder. materializeExtractedFramesForCompiledDir now takes a frameRootName. The SDR pass stages under __hyperframes_video_frames_sdr, the same root its cache-off output already uses (one constant for both). New test extractVideosStage.sdrRestage.test.ts runs the stage with real folders: an original extraction, then the tone-map pass of the same id, through both the symlink and the copy path. It expects the lookup to serve the tone-mapped frame. On the previous head it gets 'original'.

Non-blocking

  • Fixture never reaches the overlay path: right. The unit path is covered by the browser test today. I'll add an unfiltered HDR clip under an overlay to the regression fixture in this PR.
  • Tone-mapped clip vs SDR neighbours: I agree with the reading. SDR clips in an HDR render get the BT.709 to BT.2020 remap at extraction; the tone-mapped clip doesn't. I'll measure it on a colour-bar fixture (same content as SDR and as HLG) before changing it, and follow up here with the numbers.
  • Filter checked at the clip's start only: true, and a known limit. A GSAP-added filter is missed, and a filter removed later keeps the whole clip on the SDR path. Each demotion is logged (renders through the SDR path: CSS filter … applies to it). I'll add "at its start" to that line. HDR images with filters are out of scope here.
  • Overlay children painted twice: agreed. It's the same leak timed elements already have (#host * in the host layer), so it needs one fix in the mask for both. I'll raise it separately rather than widen this PR.

Devbox: each new test fails on the previous head; engine (39) and producer render (669) suites pass 3 runs in a row; tsc, the comment ratchet and format:check are clean.

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Follow-up on the colour note, at 541f8a77: measured, confirmed, fixed.

Measurement. One HLG render holds three copies of the same red/green bars, sampled as raw BT.2020 code values (16-bit):

  1. an SDR BT.709 clip;
  2. the HLG version of the same bars with a no-op filter: saturate(1), so it takes the tone-map path;
  3. the HLG version unfiltered.
red patch SDR clip tone-mapped clip native HLG
previous head 60449 28472 11173 61201 10149 0 45961 21093 11426
this head 60449 28472 11173 51699 26915 16205 45961 21093 11426

The tone-mapped clip carried BT.709 values unconverted into the BT.2020 output, so it was visibly oversaturated next to its SDR neighbour. Green showed the same.

Fix. In an HDR render, the tone-map pass now takes the same BT.709 to BT.2020 step the SDR clips get, applied after the tone-map:

  • toneMapVideosToSdr passes the render's transfer;
  • the extractor records it through sdrToHdrTransfers, the one array both the cache key and the extraction read, so these frames get their own cache entry.

The two filters swap order (tone-map, then remap); they never combined before. Forced-SDR renders are unchanged.

Test. videoFrameExtractor.test.ts "gives a tone-mapped clip in an HDR render the BT.2020 primaries of its SDR neighbours" extracts the same colour as SDR and as HLG together for an HLG render.

  • With the fix, the tone-mapped frame's saturation is within 0.03 of the SDR clip's (0.366 vs 0.368).
  • On the previous head the gap is 0.08.

Engine extractor (202) and producer render (380) suites pass 3 runs in a row. tsc, the comment ratchet, format:check and the Fallow audit are clean.

The demotion log line now says the filter applies "at its start". The unfiltered-HDR-under-overlay regression fixture is next, in this PR.

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Regression shard 3 is red on hdr-overlay-filter-regression only (visual; the other fixtures in the shard pass). That is expected: the fixture's HDR clip is the filtered one, and 541f8a77 now gives it the BT.2020 conversion its SDR neighbours get, so its golden changes. I'll regenerate the golden in the test image and push it to this PR.

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Two more commits, at e896c1b2:

Timed overlay without an id dropped from HDR exports (found by another contributor, item 137).

  • The cause: queryElementStacking's main loop gives an element an entry only when it has an id. The new sibling rule skipped every data-start element, assuming it already had an entry. So a timed overlay with no id got neither and fell into its host's layer, under the HDR video.
  • The fix: the main loop records the elements it describes, and the sibling rule skips exactly those.
  • The test: a browser stacking case where a class="label" data-start="0" overlay with no id must get its own layer above the HDR video. It fails on 541f8a77.

Goldens. Both are regenerated in the test image:

  • hdr-overlay-filter-regression: the filtered clip now gets the BT.2020 conversion. It passes 3 runs against the new golden.
  • hdr-hlg-regression: its fixture labels are exactly the id-less timed overlays above, and the old golden had them missing. The new golden shows them; for example, "A · HLG baseline + DOM overlay" is now visible at 1 s.

The stacking and injector browser suites pass 3 runs in a row. tsc, the comment ratchet, format:check and the Fallow audit are clean.

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Merge-order note: #5749 changes how SDR white sits in HLG exports, and hdr-hlg-regression's labels are SDR white over HLG. Whichever of #5728 and #5749 merges second must regenerate that golden on top of the other. If #5749 lands first, I'll rebase this PR and regenerate it.

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Source re-review at e896c1b2b8b3fe4ad78e9e9e6800559613ec04d3 against effective base b1cf6e3c517e227d1dca1b212e7637c19e502a64.

The earlier transition-membership blocker is repaired by the current ancestry expansion (captureHdrFrameShared.ts:199–202, introduced in ab67955b). The cached-original staging blocker is also repaired: the SDR pass now uses the same distinct root for extraction and materialization (extractVideosStage.ts:567–578, introduced in 82591ef2). I am not carrying either historical blocker forward.

Remaining correctness blocker [P2]: promoted overlay children still paint in two layers

Confirming the overlay-child issue already reported by Rames in review 5555154773, rather than posting a duplicate inline. I consider it blocking for this change because the BASE differential makes it an introduced rendering regression for previously untimed overlays, not just an unchanged timed-element leak.

Use an active, identified timed host with opacity 1, a smaller unfiltered native-HDR video, and a later positioned untimed overlay containing a visible child with fractional alpha. Put part of that child outside the video's bounds; ordinary equal-z sibling ordering suffices.

  • BASE: the untimed overlay has no separate stacking entry. The child is captured once in the host DOM layer. The smaller native-video blit does not overwrite its outside-video pixels.
  • HEAD: packages/engine/src/services/videoFrameInjector.ts:811–821 adds the overlay as a separate unit, producing host DOM → HDR video → overlay DOM. During the host capture, applyDomLayerMask explicitly shows #host * (screenshotService.ts:493–506), but excluding the overlay only hides its root (:460, :517–520). Its child therefore remains explicitly visible. During the later unit capture, the unit and its descendants are shown again (:485–487). hdrCompositor.ts:657–706 captures/blends both DOM passes; no native blit replaces the selected outside-video pixel between them.

The child consequently contributes once before this PR and twice after it. With fractional alpha and a contrasting background, that changes the exported output. The hidden-timed-descendant preservation does not cover a visible untimed child, and the new browser-test source checks root visibility rather than descendant exclusion. Please make layer ownership exclude the promoted subtree from the lower host capture while preserving legitimately shown nested layers, and cover this smaller-video/translucent-child case. Blindly hiding the host through opacity would break the documented nested-layer contract.

Audited: all 16 existing changed effective-BASE versions in full before all 26 changed HEAD versions in full; all eight actual historical commit patches; complete bound layerCompositor, hdrCompositor, sequential-loop and hybrid-loop integration sources. Followed producers/callers/consumers through demotion, transform/cache identity, served staging, mutable lookup, injection, masks, scenes and cleanup; assessed all 36 unordered pairs of nine behavioral groups. Both independent review passes completed; the descendant finding was additionally checked against BASE rather than inherited from its earlier public severity.

Trusting / limits: LFS pointer text only, not media/golden payloads. Source-only reasoning, not a browser/render reproduction or measured pixel result; no tests, FFmpeg/native execution, CI queries or live-service access. Start-only filter detection and filtered-HDR-image support remain stated limitations. The macOS/no-zscale plus target-transfer branch remains an unanswered native-contract question; I am not asserting a second defect without that evidence.

— Review by tai (pr-review)

Verdict: REQUEST CHANGES
Reasoning: The old scene and staging blockers are fixed, but the new overlay split still double-composites translucent descendants outside native-video coverage on a previously single-painted untimed input.

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST CHANGES · 1 blocker · 0 important · 0 nits

Re-review at 8f7e984c. Hiding the promoted overlay's children fixes the double paint that @terencecho flagged. The new loop runs for every hide id, though, not just layer units, and it now also hides the injected frame of an SDR video in a layer above an HDR video. Details are inline.

Scope

Checked the merge's own changes with git show --remerge-diff. Both conflict resolutions are correct. In videoFrameExtractor.ts, the tone-map still runs after sampling and before the BT.709→BT.2020 remap, and the signal argument was dropped to match main's unsignalled readFirstFrameColour. In extractVideosStage.ts, the shared extractionOptions gains urlByAbsentLocalPath, so the SDR re-extraction receives it too. I ran the blocker repro in vitest, where it fails at this head and passes with e896c1b2's screenshotService.ts. CI (CI, regression, Windows, CodeQL) was still running when I posted this.

// The show rules reach `#host *`, so a child would stay painted under its hidden root;
// a layer shown on its own, or inside one, keeps its place.
for (const child of el.querySelectorAll<HTMLElement>("*")) {
const shownInside = shownElements.some(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[blocker] This hides the render-frame <img> and grading canvas of a shown video whenever an ancestor of that video is in hide. Those are siblings of the <video>, not its descendants, so s.contains(child) is false for them. They then get inline visibility:hidden !important, which overrides the #__render_frame_<id>__ show rule.

How to trigger it: #main[data-start] contains an HDR video and an SDR video stacked above it (for example a PiP, or a filtered HDR clip that this PR demotes to SDR). The layers are [main] → [hdr] → [sdr]. When the compositor captures the SDR layer, hideIds includes main, and the SDR clip's frame vanishes from the export. Before this commit only #main itself was hidden.

Repro: in this describe block, <div id="root" data-start="0"><video id="hdr" data-start="0"></video><video id="pip" data-start="0"></video><img id="__render_frame_pip__"></div> with applyDomLayerMask(page, ["pip"], ["root", "hdr"]) leaves the img hidden. The same test passes on e896c1b2.

Fix: add each shown id's render frame (document.getElementById(renderFrameId(id))) and its ${canvasIdPrefix}${id} canvas to shownElements. Pin that with the case above.

@miguel-heygen
miguel-heygen force-pushed the fix/hdr-export-overlays-and-filters branch from 8f7e984 to 720a9ae Compare October 28, 2026 09:21
@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main (no conflicts) and pushed.

  • Blocker (SDR video layered over an HDR video lost its frame): applyDomLayerMask now counts each shown id's render frame and grading canvas as shown, so they stay painted under a hidden ancestor. Test: applyDomLayerMask keeps the render frame of an SDR video shown over an HDR video under a hidden ancestor (fails on the old code with expected 'hidden' to be '', passes now).
  • Rebase fallout I fixed: main now skips the SDR-to-HDR conversion when toneMapHdrToSdr is set, which left an SDR clip beside a tone-mapped one on BT.709 primaries. That preflight now also runs when hdrTransferAfterToneMap is set; the existing gives a tone-mapped clip in an HDR render the BT.2020 primaries of its SDR neighbours test caught it (0.088 saturation gap against a 0.03 limit) and passes again.
  • P2 (staged SDR source with an external cache): I could not reproduce it. SDR re-extraction stages under its own __hyperframes_video_frames_sdr/<id> root, extractVideosStage.sdrRestage.test.ts already covers cache-backed restage serving SDR frames, and tone-mapping runs once per render.

Ran: videoFrameExtractor, screenshotService and videoFrameInjector test files (280 passed). Not run here: the full producer and regression suites; CI covers those.

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST CHANGES · 2 blockers · 0 important · 0 nits · head 720a9aee

  • The required regression check fails on the PR's own fixture. In shard 9 (run 40663571358, attempt 2), hdr-overlay-filter-regression fails on 100 of 100 frames at PSNR 16.77. The body reports 15.18 for main, so at this head the golden looks no better than main. Attempt 1 failed on a 503 from the docker mirror and I re-ran it; this failure is a real render mismatch. The likely cause is 720a9aee: it re-enables the SDR→BT.2020 preflight for tone-mapped renders after the goldens were refreshed in 18fae9ce. Either the golden is stale or the output regressed. Pull the regression-failures-shard-9 artifact to tell which.
  • No fails without the fix: line. This is a fix(...) PR, and the lane requires a line in the body, starting fails without the fix:, that names a test failing without the fix. The new screenshotService.test.ts case for the SDR-over-HDR blocker would do.

The SDR-over-HDR blocker from my last review is fixed in 1ca64016: a shown video's render frame and grading canvas now count as shown, and the new jsdom case covers it. @terencecho noted that the cached-staging thread (extractVideosStage.ts:587) was already repaired at e896c1b2.

Scope

Re-reviewed the commits after my 8f7e984c review: 1ca64016 (mask fix and test) and 720a9aee (preflight guard, covered by the existing "BT.2020 primaries of its SDR neighbours" extractor test). CI, Windows, CodeQL, Comments and PR captures are green.

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Both blockers answered at ad626d58.

  • fails without the fix: line: added to the PR body, naming hdr-overlay-filter-regression and the applyDomLayerMask SDR-over-HDR test.
  • hdr-overlay-filter-regression failing at 16.77: reproduced in the CI test image. It is not caused by 720a9aee or the rebase: the PR's original head 8f7e984c fails with the same PSNR, and e896c1b2 (where CI passed on 17 Oct) still passes in today's image. The difference is main's change to alphaBlit.ts that puts SDR white at the BT.2408 reference level (75% HLG signal) instead of peak: every SDR-path pixel in this fixture (the overlays, and the filtered video) renders at that level now, so a golden made before it is stale. A probe render of the fixture with the brightness(0.5) removed showed the same overlay level, so the filter is not leaking.
  • Golden refreshed on the current code, and I checked the new frames by eye: at 0.16 s and 2.6 s the title, logo and progress bar sit above the video in both the HLG and the SDR half, and the video is dimmed by the filter while the overlays are not.

Not run here: the other regression shards; CI covers those.

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE: 2 resolved, 0 open, 0 new · 720a9aee → ad626d58. The golden was stale: the rebase brought in #5749 (SDR at BT.2408 reference white), and this commit only refreshes that fixture's output. All 10 regression shards now pass, along with CI, Windows, CodeQL and Comments. The body now has the fails without the fix: line. This approval covers ad626d58 only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants