Skip to content

fix(core): read CSS animation opacity through color grading's own hide - #3507

Open
miga-heygen wants to merge 1 commit into
mainfrom
fix/color-grading-entrance-opacity
Open

fix(core): read CSS animation opacity through color grading's own hide#3507
miga-heygen wants to merge 1 commit into
mainfrom
fix/color-grading-entrance-opacity

Conversation

@miga-heygen

Copy link
Copy Markdown
Contributor

Summary

Fixes color-graded elements with CSS entrance animations (e.g. opacity: 0→1) rendering as solid white (mp4) or black (prores) frames for the entire output.

Root cause: On the first drawEntry(), the animation's initial opacity: 0 is copied to sourceOpacityForCanvas. Then hideSourceElement() sets opacity: 0 !important on the source. On subsequent frames, the hiddenByColorGrading guard correctly prevents reading back grading's own hide — but also prevents updating the canvas opacity as the animation progresses. Both source and canvas stay at opacity: 0 for the entire render.

Fix: When the source is hidden by color grading, temporarily restore the authored inline opacity before reading getComputedStyle, so the CSS animation's current value shows through. The restore→read→rehide is synchronous within a single JS task, so no repaint occurs between the style writes.

Closes #3329

— Miga

#3329)

When a color-graded element has a CSS entrance animation (e.g. opacity:
0→1), the first drawEntry() copies the animation's initial opacity "0"
to sourceOpacityForCanvas, then hideSourceElement() sets opacity:0
!important on the source. On subsequent frames the hiddenByColorGrading
guard correctly prevents reading back grading's own hide — but also
prevents updating the canvas opacity as the animation progresses,
freezing both source and canvas at opacity 0 for the entire render.

Fix: when the source is hidden by color grading, temporarily restore the
authored inline opacity before reading getComputedStyle, so the CSS
animation's current value shows through. The restore–read–rehide is
synchronous, so no repaint occurs between the style writes.

Co-Authored-By: Miga <miguel.sierra_miga@heygen.com>

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Miguel — traced the fix end-to-end against drawEntry and hideSourceElement at the new HEAD. The core mechanic is sound: lift authored inline opacity → read getComputedStyle → rehide 0 !important — all inside a single synchronous JS task, so no repaint slips between the writes and MutationObservers see a coalesced final state. The scoping is per-entry.element, so two <canvas> elements on-screen won't leak hide-state between each other. One load-bearing concern below, then some smaller items.

Load-bearing concern — the fallback capture path in hideSourceElement silently defeats the fix.

packages/core/src/runtime/colorGrading.ts:2918-2925 — when the parse-time data-hf-authored-opacity attribute is present (the authored !== null branch at :2919-2921), the fix works as advertised: the attribute holds the pre-animation authored value (empty or the declared opacity) and the lift on drawEntry :3076-3086 restores that. But when the attribute is absent, hideSourceElement falls back at :2922-2925 to entry.element.style.getPropertyValue("opacity") — and if the CSS entrance animation has already sampled its opacity: 0 initial keyframe by the time the first hide runs, that captured sourceInlineOpacity is "0".

On subsequent frames the lift branch at :3077-3082 then restores opacity to "0", getComputedStyle at :3087 reads "0", and sourceOpacityForCanvas at :3090 is set to "0" — the exact frozen-at-initial-value failure mode #3329 was meant to fix. The comment at :2912-2917 ("Fall back to the live inline value for documents loaded without the capture installed") is essentially the acknowledgement.

Two questions:

  • Which surfaces install the parse-time capture, and is it guaranteed for every production render path (producer render, studio preview, iframe embed)? If any surface can render color grading without the attribute stamped before first hide, #3329 still reproduces there — silently, no error, same solid frame.
  • If the fallback path is a known-degraded mode (i.e. accepted risk), can we at least log a swallow-level breadcrumb from the fallback branch so support can grep for it when someone reports a repro?

No new test exercises the fallback path — every hit of data-hf-authored-opacity in colorGrading.test.ts (:259, :275, :807) sets the attribute. A test that omits the attribute, sets video.style.opacity = "0" before createColorGradingRuntime(), then asserts canvas.style.opacity !== "0" on runtime.redraw() would either prove the fallback is safe or turn this concern into a blocker.

Non-blocker concerns.

  1. packages/core/src/runtime/colorGrading.test.ts:802-830 — the new #3329 test only covers the sourceInlineOpacity === null branch of the lift (removeProperty at :3084). The other branch — setProperty(..., sourceInlineOpacityPriority) at colorGrading.ts:3078-3082 — has no per-frame coverage. The destroy-time restore of "0.75" at test.ts:255-271 predates this PR and doesn't exercise drawEntry's new lift/rehide dance. Suggest adding a case that stamps data-hf-authored-opacity="0.5" and asserts canvas opacity reflects the restored value across runtime.redraw(), with source rehidden at 0 !important after each frame.

  2. colorGrading.test.ts:825-826 — the second runtime.redraw() doesn't simulate an animation progressing; jsdom has no CSS-animation engine, so both getComputedStyle reads return "" and hit the || "1" normalization at colorGrading.ts:3090. The test proves "the code path no longer freezes at 0", not "a real browser reads the interpolated animation value." That verification necessarily lives in the visual/regression shards — the preview-regression and regression-shards shards were pending at review time; recommend confirming at least one existing fixture in those shards exercises an opacity keyframe on a graded element, otherwise consider a targeted CSS-animation-opacity fixture add. (The producer unit-test claim of "all 53 tests pass" is real, but by construction it can't catch the animation-progression regression.)

  3. colorGrading.ts:3076-3092 — no try/finally wrapping the lift-read-rehide. The only realistic throw surface between the lift and the rehide is window.getComputedStyle on :3087 (which throws in jsdom for detached elements, spec'd not to throw in browsers). If it did throw — element detached concurrently mid-frame — the source stays temporarily with its authored inline opacity and sourceHidden remains true, so the DOM/flag state diverges until the next drawEntry re-hides (or restoreSourceElement at :1948-1958 skips the branch because opacity !== "0 !important" and leaves it as-is). Wrapping the lift and rehide in try/finally would harden this at zero real cost. Nit-level.

  4. colorGrading.ts:3087-3092 — the lift/rehide now runs every frame for every hidden-by-grading entry, adding two setProperty mutations and one attribute-mutation observer callback per entry per frame. Opacity is a composited-only property so no forced layout, and the observer at :3416-3427 bails on the geometrySignature check without triggering a redraw. Not a real perf concern at HF scale, but if we ever need to compose ~50 graded entries in one composition it's worth remembering this hot-path cost was added.

What I didn't verify.

  • Real-browser getComputedStyle().opacity semantics while a @keyframes animation is playing — the fix's premise is that the browser returns the current interpolated value once the inline 0 !important is out of the cascade. I trust that against the spec but did not open a browser to confirm.
  • The preview-regression and regression-shards shards were still pending at review time; if a fixture there covers this exact repro, that's the real validation for #3329.
  • Whether heygen-com/hyperframes-internal is a surface where the parse-time authored-opacity capture might not run — outside my read scope, worth an internal check.

Peer state + CI at HEAD d3ae41fd9a4.

gh api /pulls/3507/reviews[]. No peer reviews. Author is miga-heygen (bot); merge is your call, not a peer-bot approval. Static gates green at review time; heavy shards (regression-shards, Perf: *, Preview parity, Windows render, Producer: unit tests) all in-progress. Producer: unit tests is where the new colorGrading test lands — hold merge on that shard's green.

Review by Rames D Jusso

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.

Bug: data-color-grading renders solid white (mp4) / black (prores) frames — any grading, 0.7.109 & 0.8.2

3 participants