Repository navigation
fix(producer): HDR exports keep overlays above HDR video and apply CSS filters - #5728
miguel-heygen wants to merge 12 commits into
Conversation
Edit accuracy: accurate 271 (base branch 271), smooth 110 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
jrusso1020
left a comment
There was a problem hiding this comment.
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.tsbuildssceneElementsonce, before the frame loop, from the[id]elements under each.scene. The new stacking entries get generated ids (hf-layer-unit-N, fromqueryElementStacking), so they are never insceneIds. IncaptureHdrFrameShared.ts,captureSceneIntoBufferhides every stacking id where!sceneIds.has(id) || nativeHdrIds.has(id), andapplyDomLayerMaskthen sets inlinevisibility:hidden !importanton 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-regressionwindow 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 byvideoFrameInjector.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 thesetparamspassthrough, 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.addVideoreplaces entries byvideoId, and the SDR frames go to their own__hyperframes_video_frames_sdrdirectory. toneMapVideosToSdrreusesextractionOptions, overriding onlytoneMapHdrToSdrandoutputDir. SettinghdrToSdrTransformKeyturns off deferred range extraction, so nothing is left undriven. It keeps the same failure policy.planHdrResourcesnow runs after the filtered clips leave the native set.hasHdrContentstill uses the probed set, so the output stays HDR.- Units are spliced right after their HDR entry.
groupIntoLayersis 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.
applyDomLayerMaskonly 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
left a comment
There was a problem hiding this comment.
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, { |
There was a problem hiding this comment.
[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.
|
Both blockers fixed at Overlay units in shader transitions (Rames, also confirmed by tai). Unit ids are minted per frame, after the scene map is read, so
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. Non-blocking
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 |
|
Follow-up on the colour note, at Measurement. One HLG render holds three copies of the same red/green bars, sampled as raw BT.2020 code values (16-bit):
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:
The two filters swap order (tone-map, then remap); they never combined before. Forced-SDR renders are unchanged. Test.
Engine extractor (202) and producer render (380) suites pass 3 runs in a row. tsc, the comment ratchet, The demotion log line now says the filter applies "at its start". The unfiltered-HDR-under-overlay regression fixture is next, in this PR. |
|
Regression shard 3 is red on |
|
Two more commits, at Timed overlay without an id dropped from HDR exports (found by another contributor, item 137).
Goldens. Both are regenerated in the test image:
The stacking and injector browser suites pass 3 runs in a row. tsc, the comment ratchet, |
terencecho
left a comment
There was a problem hiding this comment.
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–821adds the overlay as a separate unit, producing host DOM → HDR video → overlay DOM. During the host capture,applyDomLayerMaskexplicitly 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–706captures/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.
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
[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.
…S filters # Conflicts: # packages/engine/src/services/videoFrameInjector.ts # packages/producer/src/services/render/stages/extractVideosStage.ts
…er a hidden ancestor
… the HDR primaries
8f7e984 to
720a9ae
Compare
|
Rebased onto current main (no conflicts) and pushed.
Ran: |
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 2 blockers · 0 important · 0 nits · head 720a9aee
- The required
regressioncheck fails on the PR's own fixture. In shard 9 (run 40663571358, attempt 2),hdr-overlay-filter-regressionfails 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 is720a9aee: it re-enables the SDR→BT.2020 preflight for tone-mapped renders after the goldens were refreshed in18fae9ce. Either the golden is stale or the output regressed. Pull theregression-failures-shard-9artifact to tell which. - No
fails without the fix:line. This is afix(...)PR, and the lane requires a line in the body, startingfails without the fix:, that names a test failing without the fix. The newscreenshotService.test.tscase 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.
…t BT.2408 reference white
|
Both blockers answered at
Not run here: the other regression shards; CI covers those. |
There was a problem hiding this comment.
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.
What
HDR exports of a sub-composition that mixes HLG/PQ clips with SDR clips, HTML overlays and CSS filters now match the preview:
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.filterapplies 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 effectivez-index, or equal and later in tree order. Injected render-frame images and color-grading canvases are excluded. The entry is addressed by adata-hf-layer-unitattribute, so author ids are never rewritten.applyDomLayerMaskcan show or hide those entries like any other layer element.filterof 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.Verification
fails without the fix:
hdr-overlay-filter-regression(100 of 100 frames on main) andscreenshotService.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).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, andbrightness(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.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.tscare clean, as are oxlint, oxfmt and the comment ratchet. Injector and stacking engine tests pass 3 runs in a row (49 tests).hevc / yuv420p10le / arib-std-b67.