Skip to content

feat(runtime): render media treatments deterministically#2752

Merged
ukimsanov merged 1 commit into
mainfrom
feat/media-treatment-runtime
Jul 24, 2026
Merged

feat(runtime): render media treatments deterministically#2752
ukimsanov merged 1 commit into
mainfrom
feat/media-treatment-runtime

Conversation

@ukimsanov

Copy link
Copy Markdown
Collaborator

What

Adds the realtime WebGL treatment runtime for real <img> and <video> media, including grading, 3D .cube LUTs, 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

  • Core runtime/init tests: 105/105
  • Engine screenshot tests: 31/31
  • Lint media tests: 29/29; parser regressions: 201/201
  • Core, Engine, Lint, and Parser typechecks pass
  • Exact six-PR stack passes the complete workspace build
  • Documentation updated (added in stacked PR 3)

Stack 2/6. Base: #2751. This runtime does not require the later CLI, Registry, or Studio PRs.

@ukimsanov
ukimsanov marked this pull request as ready for review July 24, 2026 03:42
Copilot AI review requested due to automatic review settings July 24, 2026 03:42

Copilot AI 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.

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-grading JSON 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.

Comment thread packages/core/src/runtime/colorGrading.ts
Comment thread packages/lint/src/rules/media.ts Outdated

@miguel-heygen miguel-heygen 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.

[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 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.

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 lintcore 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)

Comment thread packages/core/src/runtime/colorGrading.ts
Comment thread packages/core/src/runtime/colorGrading.ts Outdated
@ukimsanov
ukimsanov force-pushed the feat/media-treatment-schema branch from 6703b7c to 7d7c18f Compare July 24, 2026 06:30
@ukimsanov
ukimsanov force-pushed the feat/media-treatment-runtime branch from a2094eb to f9bf4d0 Compare July 24, 2026 06:30
@ukimsanov
ukimsanov force-pushed the feat/media-treatment-schema branch from 7d7c18f to 963e1ea Compare July 24, 2026 15:20
@ukimsanov
ukimsanov force-pushed the feat/media-treatment-runtime branch from f9bf4d0 to f0ee0db Compare July 24, 2026 15:21

@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.

Re-review at f0ee0db9 (was a2094eb4). @magi deferred to my verdict; standalone re-review.

All previously-raised concerns resolved.

  • Magi's blocker (the lint rule was a second owner of the schema): packages/lint/src/rules/media.ts no longer copies any keys — it imports validateColorGradingContract from the new @hyperframes/parsers/color-grading-contract and calls it (:292). Both lint and core now consume that single lower-level owner, so they can't drift from each other. This is the structural fix Magi asked for.
  • My finding #1 (waitForActiveLuts redraw() is the load-bearing capture paint): kept unconditional and now documented (runtime/colorGrading.ts:3253"this paint is required even when no LUT is pending so the screenshot receives the current frame"). Copilot's gate-on-pending-LUTs suggestion correctly not taken.
  • My finding #2 (grain/effect two-clock seam): fixed exactly as recommended (drawEntry, ~L2726). A single frameTime is now derived from the deterministic __player.getTime() (fallback video.currentTime), and grainSeed keys off that same frameTime instead of video.currentTime. Grain-on-video is now deterministic between preview and final capture.
  • Magi's non-blocking cleanup (engine re-declared the source-hidden attr): screenshotService.ts now imports COLOR_GRADING_SOURCE_HIDDEN_ATTR from @hyperframes/core/color-grading instead of re-declaring the literal.

#2752's own diff is ready on the merits. The one open item is not in this PR — it's the base: #2751 removes four released preset IDs that break the media-use skill (details in my REQUEST_CHANGES on #2751). Since #2752 is stacked on #2751 it inherits that gate. Also note @magi's CHANGES_REQUESTED from a2094eb4 is now stale (its lint blocker is resolved) but still formally on record — since Magi is deferring and won't re-review, it'll need a dismissal before merge.

CI: no failing checks at f0ee0db9; perf + regression shards still running at review time.

Verdict: COMMENT — #2752's changes fully resolve every prior concern and are ready on the merits; it's gated only by #2751's preset-removal blocker (the stack base) and the stale Magi review. Not stamping — deferring the approve to you.

Review by Jerrai (hyperframes)

@ukimsanov
ukimsanov force-pushed the feat/media-treatment-schema branch from 963e1ea to 70213c5 Compare July 24, 2026 15:56
@ukimsanov
ukimsanov force-pushed the feat/media-treatment-runtime branch from f0ee0db to 7a911c3 Compare July 24, 2026 16:01

@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.

Re-review at 7a911c37. The only delta since my last pass is the stacked #2751 fix (skill / core key-type guard / docs / tests) — #2752's own runtime / lint / engine diff is unchanged and still ready on the merits (every prior concern stayed resolved: shared parsers contract consumed by lint, unified deterministic clock, documented capture redraw, canonical source-hidden attr).

With #2751's preset-removal blocker now resolved, #2752 is gated only on: #2751 landing, my #2751 approve (pending @jrusso1020's go), and a dismissal of @magi's now-stale CHANGES_REQUESTED here (its lint blocker was fixed two revisions ago; Magi is deferring and won't re-review). No new issues.

Verdict: COMMENT — ready on the merits; deferring the stamp.

Review by Jerrai (hyperframes)

jrusso1020
jrusso1020 previously approved these changes Jul 24, 2026

@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.

Per @jrusso1020's go — APPROVE at 7a911c37. Every prior concern is resolved across the review rounds: the lint rule now consumes the shared @hyperframes/parsers/color-grading-contract (no key duplication), the grain/effect clock is unified on the deterministic __player.getTime(), the capture-time redraw() stays unconditional and documented, and the engine imports the canonical source-hidden attribute from core. Rides the #2751 stack. (@magi is separately clearing its now-stale change-request.)

Review by Jerrai (hyperframes)

miguel-heygen
miguel-heygen previously approved these changes Jul 24, 2026

@miguel-heygen miguel-heygen 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.

Exact-head re-review at 7a911c37cd0c4bc5ef08e15a50c723db8f99661c, superseding my stale change-request on a2094eb4.

The blocker is structurally resolved: lint now delegates the full grading payload to validateColorGradingContract from the shared parsers contract instead of maintaining copied key sets. The adjacent consumer drift is closed too: screenshotService.ts imports COLOR_GRADING_SOURCE_HIDDEN_ATTR from core. I also rechecked the render-seam fixes: capture redraws after frame injection, waitForActiveLuts() redraws unconditionally after every seek, and the grading runtime uses the deterministic player clock for animated grain/effects.

No additive blocker found. The exact-head non-regression checks are green; regression shards were still running at review time with no failures.

Verdict: APPROVE
Reasoning: The validator/runtime/capture stack now consumes one shared contract and preserves deterministic redraw behavior; my previous schema-ownership gate no longer applies.

— Magi

Base automatically changed from feat/media-treatment-schema to main July 24, 2026 16:58
@ukimsanov
ukimsanov dismissed stale reviews from miguel-heygen and jrusso1020 July 24, 2026 16:58

The base branch was changed.

@miguel-heygen miguel-heygen 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.

Exact-head re-review at 7a911c37cd0c4bc5ef08e15a50c723db8f99661c after the base retarget to 9e7ddb0188182a92469600dec62396f452519c00.

The code-merit verdict is unchanged: no additive source blocker found. The previously raised issues remain resolved—the lint layer consumes the shared parsers contract, grain and effect animation share the deterministic player clock, capture-time redraw remains unconditional and documented, and Engine imports the canonical source-hidden attribute.

I cannot reapply the approval to the current remote state because the required regression gate is red on this exact head. Shard 1 has a real style-5-prod visual-parity failure (61 failed frames); shards 2–8 were canceled by fail-fast. The branch is also still the old SHA rather than a newly published rebased commit. The remaining required checks are green.

Verdict: COMMENT
Reasoning: The implementation is clean on re-review, but an exact-head approval would misrepresent a known required regression failure and would be dismissed again when the actual rebased SHA is pushed. Re-review/stamp the new head once published and green.

— Magi

@ukimsanov
ukimsanov force-pushed the feat/media-treatment-runtime branch from 7a911c3 to 944640c Compare July 24, 2026 18:17

@miguel-heygen miguel-heygen 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.

Freshness re-review at exact head 944640c3, superseding my prior green-code/comment verdict at 7a911c37.

The rebase delta is clean and addresses the stale-head visual failure rather than carrying it forward:

  • packages/core/src/runtime/init.ts:1796-1812 keeps the normal all-timed-node visibility sync as the default contract, while packages/core/src/runtime/init.ts:2179-2184 scopes only the pre-color-grading initialization pass to timed <video>/<img> nodes. That allocates grading only for active media without prematurely mutating future composition containers.
  • packages/core/src/runtime/init.test.ts:1323-1367 pins both sides: future timed media is hidden before grading allocation, while a future non-media composition retains its authored display/visibility until the normal timeline seek owns it.
  • The prior blockers remain resolved: lint still consumes @hyperframes/parsers/color-grading-contract; animated grain/effects still share the deterministic player clock; capture redraw remains unconditional; Engine still imports the canonical source-hidden attribute.

Audited: full range-diff from previously approved 7a911c37 to 944640c3, the complete additive init.ts/init.test.ts delta, resolved review threads, and exact-head check runs.

Trusting: the previously audited runtime/shader implementation that is patch-equivalent across the rebase; pixel/GPU behavior is covered by the now-green preview parity, Windows render, perf matrix, and all 8 regression shards.

Exact-head CI is fully green, including every required check, CodeQL, CLI/global install, Windows, preview parity, perf, and all regression shards. The PR is mergeable; the only remaining GitHub gate was review.

Verdict: APPROVE
Reasoning: The published rebase preserves the reviewed implementation, adds a narrowly tested initialization fix for the prior visual-parity failure, and is green across the complete exact-head matrix.

— Magi

@ukimsanov
ukimsanov merged commit 7cf6416 into main Jul 24, 2026
53 checks passed
@ukimsanov
ukimsanov deleted the feat/media-treatment-runtime branch July 24, 2026 19:05

@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.

Re-applying my approval at the rebased head 944640c3 (my earlier APPROVE at 7a911c37 was mechanically dismissed when #2751 merged and this retargeted onto main). Per @jrusso1020's earlier go on #2752.

I verified the rebase rather than assuming it: the only new #2752 content vs the patch I already approved is a targeted fix in runtime/init.ts — the initial pre-grading syncTimedElementVisibility call is now scoped to video[data-start], img[data-start] (via a new optional visibilityNodes param that defaults to the full [data-start] set, so playback-time syncs are unchanged), with a regression test proving non-media composition containers aren't hidden prematurely. That's what closes the stale-head style-5-prod visual-parity failure (61 frames), and all 8 regression shards + the full required suite are now green at this SHA. The proxyTranscoder/preview diffs relative to my old head are main-inherited base movement, not part of this PR (absent from the main...944640c3 diff). The previously-reviewed runtime/lint/engine changes are untouched.

Review by Jerrai (hyperframes)

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.

4 participants