feat(runtime): render media treatments deterministically#2752
Conversation
There was a problem hiding this comment.
Pull request overview
Adds deterministic, capture-safe media treatment support by extending the runtime color-grading/WebGL pipeline, integrating it with engine frame injection/capture hooks, and tightening lint/test coverage around the data-color-grading schema.
Changes:
- Extend the runtime color grading pipeline with multipass effects (blur/bloom/Kuwahara), palette-driven effects, animated property sampling, preview rendering, and LUT readiness waiting.
- Update Engine capture/injection services to coordinate with the runtime grading layer (redraw + visibility + LUT readiness).
- Add/expand lint rules and tests to validate structured
data-color-gradingJSON and GSAP parsing of custom properties.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/parsers/src/gsapParser.test.ts | Extends GSAP parser tests to include treatment CSS custom properties. |
| packages/lint/src/rules/media.ts | Adds structured validation for data-color-grading schema and media-only enforcement. |
| packages/lint/src/rules/media.test.ts | Adds test coverage for new data-color-grading lint validations. |
| packages/engine/src/services/videoFrameInjector.ts | Removes dedicated color-grading redraw hook (redraw now centralized in injection path). |
| packages/engine/src/services/screenshotService.ts | Triggers runtime redraw after frame injection and syncs grading visibility with injected-frame visibility. |
| packages/engine/src/services/screenshotService.test.ts | Adds assertions for redraw ordering and grading visibility sync behavior. |
| packages/engine/src/services/frameCapture.ts | Waits for runtime LUT readiness prior to deterministic capture. |
| packages/core/src/runtime/init.ts | Initializes/syncs visibility earlier and redraws animated grading only during playback ticks. |
| packages/core/src/runtime/init.test.ts | Adds tests for graded media allocation and playback-only redraw behavior. |
| packages/core/src/runtime/colorGrading.ts | Major runtime expansion: deterministic effect time, multipass effects, pooled renderers, previews, animated property sampling, and LUT readiness API. |
| packages/core/src/runtime/colorGrading.test.ts | Broad new coverage for DPR sizing, preview APIs, multipass effects, renderer pooling cleanup, and LUT readiness waiting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const waitForActiveLuts = async (): Promise<number> => { | ||
| const pending = new Set<Promise<CubeLut3D>>(); | ||
| for (const element of trackedElements) { | ||
| const lut = entries.get(element)?.grading.lut; | ||
| if (!lut?.src.trim() || (lut.intensity ?? 1) <= 0) continue; | ||
| const cached = getCubeLut(lut.src); | ||
| if (cached.state === "pending") pending.add(cached.promise); | ||
| } | ||
| if (pending.size > 0) await Promise.allSettled(pending); | ||
| redraw(); | ||
| return pending.size; | ||
| }; |
| const COLOR_GRADING_TOP_LEVEL_KEYS = new Set([ | ||
| "enabled", | ||
| "preset", | ||
| "intensity", | ||
| "adjust", | ||
| "details", | ||
| "effects", | ||
| "palette", | ||
| "lut", | ||
| "colorSpace", | ||
| ]); |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
[blocker] The new lint validator creates a second owner for the media-treatment contract. COLOR_GRADING_TOP_LEVEL_KEYS and every adjust/details/effects key are manually copied into packages/lint/src/rules/media.ts, while #2751 exports HF_COLOR_GRADING_ADJUST_KEYS, HF_COLOR_GRADING_DETAIL_KEYS, and HF_COLOR_GRADING_EFFECT_KEYS as the canonical registries. They match today, but there is no executable parity check; the “keeps every canonical grading key accepted by lint” test in #2751 is vacuous at that head because these color_grading_* findings do not exist until this PR. The next core control can therefore render correctly yet be rejected as an error by lint. Please establish one source of truth across the dependency boundary (for example, move the schema data into a lower-level shared module/package both core and lint can consume), or at minimum add an exhaustive parity contract generated from the canonical registry so drift fails CI.
Non-blocking while this integration file is open: packages/engine/src/services/screenshotService.ts still locally re-declares COLOR_GRADING_SOURCE_HIDDEN_ATTR even though core marks that runtime/consumer literal single-owner and the engine already imports adjacent grading constants. Importing it closes the same drift class in the capture-visibility handshake.
Audited: all 11 changed files at exact head a2094eb4; runtime lifecycle and pooled WebGL cleanup, deterministic time/grain inputs, multipass/LUT readiness, injected-frame visibility/redraw ordering, GSAP parsing, lint behavior, and core→engine API wiring.
Trusting: pixel-level shader fidelity and browser/GPU-specific output are covered by the project regression/preview/Windows matrix rather than reproduced manually here; regression shards were still running at review time.
Verdict: Request changes.
Reasoning: The runtime/capture implementation is thoughtfully structured and the focused tests cover the new paths, but the validator currently duplicates the very contract this stack introduces as canonical. That architectural split can turn future valid treatments into hard lint failures and should be closed before landing.
— Magi
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Additive review at a2094eb4 — @magi has CHANGES_REQUESTED (the lint rule copying the schema instead of consuming core's registries) and @Copilot commented (same duplication + a waitForActiveLuts/redraw perf note). I verified Magi's blocker and am layering the render-path / determinism delta on top rather than restacking a duplicate request.
Concur with @magi's blocker — here's the data behind "they match today." I diffed the lint rule's hard-coded sets against #2751's canonical registries at these heads:
- top-level keys: identical (9/9)
adjust: identical (10/10) ·details: identical (7/7) ·lut: identical (2/2)effects: 64/64 identical, zero divergence in either direction
So today's parity is exact but coincidental — nothing in CI asserts it across the lint→core boundary, and the next valid core control (effects is the growth axis) would become a hard media lint error with green CI, exactly as Magi describes. His fix direction (a shared lower-level owner, or an executable exhaustive parity contract importing HF_COLOR_GRADING_*_KEYS) is the right shape. Confirmed latent, not active.
Additive (render determinism) — the waitForActiveLuts() redraw is load-bearing; don't gate it. prepareFrameForCapture (engine frameCapture.ts:2356) awaits __hf.colorGrading.waitForActiveLuts() on every captured frame, and that function's trailing redraw() (colorGrading.ts:3258) is the capture-time paint that stamps the current post-seek / animated treatment onto the graded canvas before the screenshot. @Copilot's inline suggestion to skip the redraw() "when there are no pending LUT loads" would drop it for the common no-LUT / LUTs-already-cached case — leaving the graded canvas on the previous frame → stale / non-deterministic captures and a wrong first frame, the precise failure this PR exists to prevent. The current unconditional call is correct; recommend keeping it and marking it load-bearing with a one-line comment so it isn't "optimized" later. (Inline below.)
Additive (verify) — two clocks feed one draw; grain may not match preview↔render. In drawEntry, effectTime prefers the deterministic __player.getTime() (colorGrading.ts:2732, video.currentTime fallback), but grainSeed (2729-2731) is derived solely from entry.element.currentTime with no __player path. So tape / glitch / scanline animation rides the timeline clock while film-grain rides the video element's own currentTime. On the pre-extracted-frame render path (the <video> is displaced by injected __render_frame__ images and isn't necessarily seeked), video.currentTime may not track the timeline — grain would animate in browser preview (real playback) yet be effectively frozen in final capture. Reachable via shipped presets that set grain on video (home-movie-8mm grain 0.34, creator-camcorder 0.08). Tests pin u_effectTime deterministically but only assert u_grainSeed is any(Number), so parity isn't covered. Recommend confirming the injected-frame path seeks video.currentTime, or seeding grain from the same __player.getTime(). Flagging verify — a parallel reviewer read the grain inputs as fine, so this is the specific two-clock seam to rule in/out. (Inline below.)
Strengths (verified): resource lifecycle is clean — detachEntry (colorGrading.ts:3038) disconnects the per-entry ResizeObserver, runs all cleanup callbacks (incl. the style MutationObserver), removes the canvas, destroys the effect / Kuwahara GL targets, and restores the source element + parent position; the top-level destroy disconnects the attribute MutationObserver and tears down idle / preview renderers. No per-mount observer/listener leak. Multipass lanes (blur / bloom / kuwahara) match core's MULTIPASS_EFFECT_KEYS.
Verdict: COMMENT — I concur with @magi's CHANGES_REQUESTED (verified with the key diff above) rather than stacking a second one. The redraw note guards against a suggested regression; the grain seam is verify-level.
Review by Jerrai (hyperframes)
| if (cached.state === "pending") pending.add(cached.promise); | ||
| } | ||
| if (pending.size > 0) await Promise.allSettled(pending); | ||
| redraw(); |
There was a problem hiding this comment.
This redraw() is load-bearing as the capture-time paint: prepareFrameForCapture (engine frameCapture.ts:2356) awaits waitForActiveLuts() on every captured frame and relies on this call to stamp the current post-seek / animated treatment onto the graded canvas before the screenshot.
Copilot's suggestion to gate it on pending.size > 0 would skip it in the common no-LUT / LUTs-already-cached case → the graded canvas stays on the previous frame → stale / non-deterministic captures and a wrong first frame (the exact failure this PR prevents). Keep it unconditional; a one-line // load-bearing: capture-time repaint comment would stop a future "optimization."
| gl.bindTexture(gl.TEXTURE_2D, entry.effectTargets?.blur.texture ?? program.texture); | ||
| gl.activeTexture(gl.TEXTURE2); | ||
| gl.bindTexture(gl.TEXTURE_2D, program.lutTexture); | ||
| bindProgramTextures(gl, program, prepared); | ||
| const grainSeed = | ||
| entry.grainSeed + | ||
| (entry.element instanceof HTMLVideoElement ? Math.floor(entry.element.currentTime * 60) : 0); |
There was a problem hiding this comment.
grainSeed derives solely from entry.element.currentTime, but effectTime two lines below prefers the deterministic __player.getTime(). On the pre-extracted-frame render path the <video> is displaced by injected __render_frame__ images and may not be seeked, so grain-on-video could animate in preview yet freeze in final capture — reachable via home-movie-8mm / creator-camcorder (grain-on-video presets).
Consider seeding grain from __player.getTime() as well (matching effectTime), or confirm the injected-frame path seeks video.currentTime. Tests pin u_effectTime deterministically but only assert u_grainSeed is any(Number), so this parity isn't currently covered.
6703b7c to
7d7c18f
Compare
a2094eb to
f9bf4d0
Compare
What
Adds the realtime WebGL treatment runtime for real
<img>and<video>media, including grading, 3D.cubeLUTs, finishing controls, stylized effects, keyframed treatment values, and preview/render readiness.Why
Media treatments must produce the same pixels during playback, seek, screenshots, and final capture without iframe reloads or timing-dependent LUT application.
How
Uses one media shader pipeline with lazy multipass work for blur, bloom, and Kuwahara; deterministic timeline time; shared preview/render frame injection; explicit readiness polling; canvas lifecycle cleanup; and lint validation from the Core contract.
Test plan
Stack 2/6. Base: #2751. This runtime does not require the later CLI, Registry, or Studio PRs.