fix(core): stop a graded plate painting through an inactive clip - #3196
Conversation
A color-graded image or video inside a timed sub-composition kept painting after its clip window closed. The runtime hid the sub-composition wrapper with `visibility: hidden`, but the grading canvas carried an explicit inline `visibility: visible`, and an explicit value on a descendant escapes an ancestor's inherited `hidden`. The treated plate composited over whichever scene was actually on screen, in preview and in the encoded render alike. `drawEntry` only refreshed its cached view of the source's visibility inside `if (injectedFrameSource || !hiddenByColorGrading)`. That gate exists for opacity: `hideSourceElement` sets `opacity: 0 !important` on the source while grading is active, so mirroring the source's computed opacity onto the canvas would blank it. Visibility was swept into the same gate by accident. Grading never writes `visibility`, so the source's computed visibility always tracks the clip window — and because every graded source is hidden-by-grading, the mirror could never self-heal once it went stale. Split the two mirrors: opacity stays gated, visibility is re-read from computed style every frame. Deriving it from the source rather than from a notification means any way of hiding a clip works, including ones that do not exist yet.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Findings
No blockers. The fix is scoped, well-commented, and root-cause targeted: splitting the visibility mirror out from behind the opacity-hide gate is the right shape because grading owns opacity but never writes visibility, so visibility can safely re-read from computed style every frame. Preview and render share drawEntry, so parity is guaranteed by construction (confirmed by the byte-identical rendered frame in the PR body). All required checks green (regression, preview parity, Windows render, player perf: fps/scrub/drift/parity).
1. Test's failing-on-main property is implicit, not asserted — packages/core/src/runtime/colorGrading.test.ts:773-798 — the new test only reproduces the bug shape if hideSourceElement has already run by the time of runtime.redraw(), i.e. hiddenByColorGrading === true in drawEntry. If a future refactor lazy-defers the initial hide, the test would silently start passing on both main and the fix. Consider asserting image.style.getPropertyValue("opacity") === "0" and priority "important" before the scene.style.visibility = "hidden" toggle so the coverage stays honest under refactor.
2. Assumes source never carries inline visibility: visible — packages/core/src/runtime/colorGrading.ts:3079-3080 — computed.visibility on entry.element reflects the ancestor's hidden only because the source has no explicit inline visibility. Any future caller (or animation engine feature) that writes visibility: visible inline on a graded source resurrects the leak — CSS visible on the descendant escapes an inherited hidden, which is literally the sibling of the bug being fixed on the canvas side. Worth a comment right on the read (or an ancestor-walk fallback) so the next author doesn't have to re-derive the invariant.
3. CLI-feedback fingerprint missing from body — the PR body describes the macOS repro and hashes but doesn't cite the CLI feedback ts / fingerprint that triggered this fix. Other reviewers pulling this off the CLI feedback stream have to guess the linkage. One-line cross-reference in the "Why" section helps future triage — same nit I'd land on any CLI-driven fix.
Verdict
APPROVE. The fix is the right shape (root fix — derived from computed style, so any future hiding mechanism works), the regression test asserts the exact reported symptom on the exact code path, and renderer/preview parity is inherent to touching drawEntry. Adjacent primitives (ungraded <img>, <video>, image/solid plates) don't need the same guard because they render inline and inherit ancestor visibility naturally — the canvas overlay is what makes color grading unique here. Findings above are non-blockers; safe to merge.
— Via
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at c88e906e.
Read through both changed files, drawEntry's neighborhood, and the syncTimedElementVisibility seam over in init.ts. The fix looks right and I think the root-fix framing holds.
Splitting the mirrors is the right surface. entry.sourceVisibleForCanvas has one consumer (the canvas visibility mirror at colorGrading.ts:2624); sourceOpacityForCanvas has one consumer at :2623. Grading writes source-style opacity in exactly two places (:2929, :1954) and never writes visibility anywhere, so re-reading computed.visibility every frame is safe from grading's own writes — the value can only move by author/timed-sync, and both of those want to propagate.
The two visibility-sync paths stay consistent. syncTimedElementVisibility in init.ts:1969-1971 does BOTH rawNode.style.visibility = ... and setSourceVisibility(_, isVisibleNow). So for timed sources with their own data-start, the direct write to entry.sourceVisibleForCanvas at colorGrading.ts:3683 and the fix's re-read from computed will always agree on the next tick — the derived value matches the stamped value. No conflict. The bug case is exactly the untimed-descendant that syncTimedElementVisibility can't reach because the child has no data-start, and computed.visibility DOES reach it via CSS inheritance.
On the root-fix framing. Preferring the computed read over teaching syncTimedElementVisibility about untimed descendants IS the right call. Any future ancestor-hide mechanism — media query, author style, not-yet-invented sub-composition attribute — will propagate to computed.visibility for free and this fix will pick it up. Deriving from the DOM's own truth beats notifying through a callback that has to know about every hider.
One observation, not a change ask. The underlying pattern that the PR body describes — "the grading canvas carries an explicit inline visibility: visible, and an explicit value on a descendant escapes an ancestor's hidden" — is still active because :2624 writes the explicit value, and the mirror is what keeps it correct. An alternative for a future pass would be canvas.style.visibility = "" when visible, so the canvas naturally inherits from its actual parent and no mirror is needed at all. That's a bigger change with potential layout / z-order implications I haven't traced, and this PR is the right fix at the right level for now.
Nit — comment quality. colorGrading.ts:3069-3075 is exactly the right level; captures why opacity stays gated, why visibility can't be, and what the failure mode was. Future readers won't need to re-open this.
What I didn't verify. Did not walk every drawEntry-scheduling path to prove that the tick loop redraws still-image entries when only an ancestor's visibility toggles on the current tick. The reported repro exercises exactly that scenario and the fix works there, so I'm trusting the schedule.
Otherwise LGTM. Stamp routing per standing rule.
3 participants
What
A color-graded
<img>or<video>inside a timed sub-composition keeps painting after its clip window closes, compositing the treated plate over whichever scene is actually on screen. This makes the grading canvas respect its clip's visibility window.Why
Reproduced on macOS with a two-scene composition: scene A (0-3s, ungraded test chart) and scene B (3-6s, same chart with
data-color-grading). At t=1.5s, a second and a half before scene B starts, scene B's treatment is composited over scene A.The runtime hides the sub-composition wrapper correctly with
visibility: hidden. The grading canvas, however, carries an explicit inlinevisibility: visible, and an explicit value on a descendant escapes an ancestor's inheritedhidden.syncTimedElementVisibilityalready carries a comment warning about exactly this leak class, but its guard only covers timed descendants, and a graded image inside a sub-composition has nodata-startof its own.Preview and render agree here: the snapshot still and the rendered frame at the same timestamp are byte-identical. Both are wrong the same way, so this is a real defect rather than a preview-vs-render divergence.
How
drawEntryonly refreshed its cached view of the source's visibility insideif (injectedFrameSource || !hiddenByColorGrading). That gate exists for opacity:hideSourceElementsetsopacity: 0 !importanton the source while grading is active, so mirroring the source's computed opacity onto the canvas would blank it. Visibility got swept into the same gate by accident, and since every graded source is hidden-by-grading, the visibility mirror could never self-heal once it went stale.The two mirrors are now split. Opacity stays gated; visibility is re-read from computed style every frame. Grading never writes
visibility, so the source's computed visibility always tracks the clip window.Deriving the canvas's visibility from the source, rather than from a notification, is the reason this is a root fix: any way of hiding a clip now works, including ones that do not exist yet. The alternative — teaching
syncTimedElementVisibilityto also notify grading for untimed descendants — would leave the next hiding mechanism broken again.Not covered here: the
~20 msoffset between an audio clip's authoreddata-startand where it lands in the rendered mix, and the preview volume-envelope time base. Both are separate findings from the same investigation and are filed separately.Test plan
New regression test in
colorGrading.test.ts: a graded image inside a wrapper that goesvisibility: hiddenmust hide its canvas, and show it again when the wrapper comes back. Verified it fails onmainwith exactly the reported symptom (visiblewherehiddenwas expected) and passes with the fix.packages/coresuite: 1912 passed, 100 files.8c61b9d2…), where before the fix it differed (f036db6c…). Scene B still grades correctly in its own window.oxlintandoxfmt --checkclean on both changed files.