feat(runtime): apply color grading in preview and render#1514
Conversation
miga-heygen
left a comment
There was a problem hiding this comment.
Approve. Well-structured WebGL shader pipeline with proper trilinear LUT interpolation and preview/render parity.
The shader correctly implements exposure/contrast/highlights/shadows/whites/blacks/temperature/tint/saturation adjustments with domain min/max clamping and division-by-zero protection. The lifecycle management is solid — destroyEntry properly cleans up WebGL resources, restores original opacity, and restores parent positioning.
Minor notes (non-blocking):
LUT_CACHEis module-level and never cleared ondestroy()— stale or errored entries persist across runtime lifecycles. Consider clearing indestroy().parseObjectPositionfor two-percentage values (30% 70%) processes both through thexValuebranch — second value overwritesxinstead of being assigned toy.ColorGradingTargettype inhyperframes-player.tsre-declares the same shape as the core type — consider re-exporting.MutationObservertriggersrefresh()on every attribute mutation — consider debouncing if many graded elements.
The SwiftShader enablement for headless CI WebGL testing is a nice touch.
— Miga
vanceingalls
left a comment
There was a problem hiding this comment.
Hello Ular — feat(runtime): apply color grading in preview and render.
Summary of intent
Layer 2 of the stack: a WebGL1 shader pipeline that overlays a graded canvas on each video / img carrying data-color-grading, with .cube LUT loading, before/after compare, MutationObserver-driven discovery, and a bridge surface for live updates from the inspector. Wires preview/player/render parity via __hf.colorGrading.redraw() and the videoFrameInjector hook.
Stack chain posture
- Base
feat/color-grading-coreat5329eb8d(= #1513 head). Head47a55279. Mergeable. - Stack-internal diff verified: #1513's contract files (
colorGrading.ts,colorLuts.ts) untouched in this PR — pure additive consumption. No silent reverter. - CI green (Build, Test, Render on windows-latest, Perf*, regression-shards 1-8, Typecheck).
Per-file findings
packages/core/src/runtime/colorGrading.ts (new, 1231 lines)
The substance of the PR. A few things I want to flag:
- Interpolation choice is trilinear, but not labeled. The fragment shader at
:236-260does an 8-corner mix (c000/c100/.../c111) and three rounds ofmix()— that's trilinear. The user-facing PR title and code don't name it, and tetrahedral is the higher-quality alternative for cube LUTs (closer to Resolve / Premiere behavior). Trilinear is reasonable for v1 and the standard for WebGL1, but please add a one-line// trilinear interpolationcomment aboveapplyLut(:236) so a future reader doesn't have to reverse-engineer the choice. Pattern #6 (small footgun, magic-choice without a name). MAX_LUT_SIZE = 64at:156is a second writer of the cap defined incolorLuts.ts:33(DEFAULT_MAX_SIZE). Two writers, no shared constant. If you bump one and not the other, the parser will accept a LUT the runtime can't handle (or vice-versa). Cheapest fix:export const DEFAULT_MAX_CUBE_LUT_SIZE = 64;fromcolorLuts.tsand import it here. Pattern #1 (duplicate validation at boundaries) cousin — small surface today, real drift risk over time.LUT_CACHEat:152is a module-levelMapthat never evicts. For v1 with manual uploads this is fine; long-lived studio sessions that churn through dozens of LUTs will accumulate. Not blocking — flag for aLRU(N)follow-up later.getCubeLutrejects cross-origin LUT URLs with the message "Remote LUT URLs are not supported" (:454-456), whichgetStatussurfaces to the inspector pill. Excellent UX — pattern #5 done well: user sees the actual reason, including parser line numbers when a malformed .cube is uploaded.picker.ts:71opacity bypass. The picker now treatsopacity <= 0.01as non-blocking when the element hasdata-hf-color-grading-source-hidden. This is correct (the graded canvas is what the user sees) but it's a cross-layer dependency: a runtime invariant downgraded for a specific reason owned by a different module. The attribute name encodes the contract, so it's not silent — defensible. Pattern #7 (defensive guard weakened) candidate, but the guard's purpose is exactly this case, so it's not actually weakened, just narrowed. Acceptable.- Shader luma uses BT.709 weights (
:229: vec3(0.2126, 0.7152, 0.0722)). MatchesHF_COLOR_GRADING_COLOR_SPACE = "rec709"from the schema. Good. drawEntryhides the source viaopacity:0 !importantand reinstates ondestroyEntry(:835-842,:978-993). The save/restore handshake tracks both inline value and priority. Solid.hideSourceElementis always called after a successful draw (:896), so the source flashes for one frame before the canvas paints. Practically invisible at 60Hz, mentioning only because Pattern #6: if the source has a slow-load image, the user may see one un-graded frame before the shader kicks in. Worth a sentence in the PR body about expected behavior; not a blocker.
packages/core/src/runtime/bridge.ts (+43)
set-color-grading and set-color-grading-compare actions added with target/payload coercion. readColorGradingTarget permissively coerces { id, hfId, selector, selectorIndex } and accepts a bare string. The inspector at #1515 builds the same shape (propertyPanelColorGradingSection.tsx:217-225), so the regime matches end-to-end.
packages/core/src/inline-scripts/runtimeContract.ts (+2)
Two new actions added to HYPERFRAME_CONTROL_ACTIONS. Forward-compat for the registry-based control discovery.
packages/core/src/runtime/init.ts (+24/-2)
createColorGradingRuntime instantiated once during sandbox init; colorGrading.redraw() wired into play/pause/seek/show-frame transitions. setSourceVisibility plugged into the timeline visibility loop at :1538-1540. Clean.
packages/engine/src/services/videoFrameInjector.ts (+20)
redrawRuntimeColorGrading(page) is a page.evaluate that calls window.__hf.colorGrading.redraw() after each frame injection. Errors swallowed silently with a comment. Pattern #4 candidate (defensive code catches its own throw) but the swallow is intentional — render layers are optional, and the engine shouldn't fail a render because color-grading wasn't initialized. Acceptable; the swallow is named (// Optional page-side shader layer.) so it's not pretending to be invisible.
packages/player/src/hyperframes-player.ts (+35)
Public setColorGrading, clearColorGrading, setColorGradingCompare, clearColorGradingCompare on the custom element. SDK-shaped API — good.
packages/studio/vite.browser.ts (+8/-1)
Added --enable-webgl --ignore-gpu-blocklist --use-gl=angle --use-angle=swiftshader --enable-unsafe-swiftshader to puppeteer args. The --enable-unsafe-swiftshader flag in particular is recent (Chrome 130+) and required for WebGL in headless tests on machines without a real GPU. Two concerns:
- Scope of this change is studio-wide, not color-grading-specific. Any future browser test in vite.browser will now run with WebGL enabled via SwiftShader, which changes execution semantics for any tests that previously assumed
--disable-gpu. Worth a sentence in the PR body that this PR's needs forced a global change to the studio browser test harness. Not blocking; would have caught this earlier in review. - The
--disable-gpuflag was removed from the args list. That's intentional (the new args supersede it via--use-gl=angle), but it's not commented. A line of context above the array would help future readers.
packages/producer/src/services/fileServer.ts:78 + packages/core/src/studio-api/routes/preview.ts:328
.cube → text/plain MIME entry in two places (Producer's file server and the Studio API's preview route). Same constant, two writers. Same drift risk as the parser/runtime constant — both small enough to defer, but consider centralizing the LUT MIME constant in @hyperframes/core so all three writers (this, this, plus htmlBundler.ts:204 from #1513) import it.
Multi-layer regime contract status
Schema → runtime: clean. The runtime imports NormalizedHfColorGrading directly from ../colorGrading and never re-defines the shape. setGrading calls normalizeHfColorGrading(rawGrading) and isHfColorGradingActive, which are the same predicates the schema layer exposes. The bridge's target coercion matches the schema's HfColorGradingTarget. No regime drift between layers 1 and 2.
Dispatch chain audit
Two new entries in HYPERFRAME_CONTROL_ACTIONS — verified no switch in mutate.ts or default branches in dispatch sites need updating beyond the bridge's own action handler. The bridge's installRuntimeControlBridge is an if-chain (not a switch), so unmatched actions fall through to the existing default-noop. Clean.
CI
Green across required checks (Build, CLI smoke, Test, Render on windows-latest, Tests on windows-latest, Perf parity, Perf drift, regression-shards 1-8, Typecheck, SDK contract).
Verdict
Clear-with-nits at 47a55279. The interpolation-naming comment is the cheapest concrete ask; the duplicate-constant fixes (MAX_LUT_SIZE, .cube MIME) are forward-looking pattern-#1 cousins that I'd hope to see closed before this stack lands as a follow-up if not in this PR. None block stamp. Re-verify if HEAD moves before stamp. Handing to Vai for stamp.
Review by Via
47a5527 to
8449708
Compare
5329eb8 to
5a68ade
Compare
8449708 to
d1162cd
Compare
5a68ade to
8aaaaf1
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Hello again Ular — R2 on feat(runtime): apply color grading in preview and render.
What I checked at the new SHA
- R1 was authored against
47a5527; head is nowd1162cd. Two new commits since R1:48fb42e(refactor: trim color grading helpers) andd1162cd(refactor: trim color grading api). Base remainsfeat/color-grading-core@8aaaaf1— stack chain still consistent with #1513's current head. - Regime contract re-verified across the new layer:
runtime/colorGrading.ts:8continues to importNormalizedHfColorGradingfrom@hyperframes/core(via the package barrel). The single-source property the R1 verdict leaned on still holds.
What did NOT move
All three R1 nits are unchanged at d1162cd:
-
Trilinear interpolation remains unlabeled. The eight-corner sample block at
packages/core/src/runtime/colorGrading.ts:237-244(c000/c100/c010/c110/c001/c101/c011/c111) is unmistakably trilinear, but nothing in the source — comment, named constant, or test name — declares it. A reviewer six months from now staring at this in an incident has to derive the algorithm from the sample lattice. A one-line// Trilinear interpolation across the 8 unit-cube cornersabove the block costs nothing and pays forever. -
LUT_CACHEis still unbounded.packages/core/src/runtime/colorGrading.ts:149declaresconst LUT_CACHE = new Map<string, LutCacheEntry>();and the only writes are unconditionalLUT_CACHE.set(...)calls at lines 469, 471, 472. There is nodelete, noclear, no size cap, no LRU. For the v1 preset-driven workflow this is plainly bounded in practice; for a long-running studio session where a user explores many custom.cubeuploads it is a slow memory creep. Same restatement as R1 — comment-level, but worth fixing before the LUT browser ships in earnest. -
vite.browser.tsSwiftShader flags remain studio-wide. Lines 33-38 ofpackages/studio/vite.browser.tsstill add--use-gl=angle,--use-angle=swiftshader,--enable-unsafe-swiftshader, and--enable-webglto the shared Puppeteer launcher used by all thumbnail generation, not just color-grading tests. Same scope creep flag — your change to enable GL in headless tests is correct, but the blast radius is the whole dev-server thumbnail pipeline. Either acknowledged scope creep (fine) or eventually scoped to a color-grading-only browser instance (better). Same severity.
Dispatch chain
parseCubeLut(text, { maxSize: MAX_LUT_SIZE })at line 466 —MAX_LUT_SIZE = 64in the runtime mirrorsDEFAULT_MAX_SIZE = 64incolorLuts.ts. The duplicate-constant nit is technically a #1513 finding; flagging here because it is the runtime side that imports the parser and would silently disagree on a future bump.readColorGradingAttribute→normalizeHfColorGrading→ shader uniforms: end-to-end traced, no orphan paths, no unreachable branches.
Verdict
Clear with the same nits restated. Handing back to @vaivia for the stamp; the stack-level call on whether to address the nits in this stack or as a follow-up is your read.
Review by Via
miga-heygen
left a comment
There was a problem hiding this comment.
R2 Approve. Refactor commits directly address the ponytail findings.
Addressed:
readColorGradingRawAttributeinlined (single-call wrapper)HfGlobalWithColorGradinginlined (single-use interface)parseObjectPositiontwo-percentage edge case fixed (index-aware disambiguation) + deadcenterbranch removedreadNumberandclampinlined intoclampNumberclosureclearGradingremoved (redundant —setGrading(target, null)handles it)await Promise.resolve(redraw())simplified to synchronousredraw()- Bridge helpers (
readColorGradingTarget,isRecord,readOptionalString,readOptionalIndex) removed per Miguel's inline request (-25 lines)
Remaining (non-blocking):
LUT_CACHEunbounded with noclear()ondestroy()- Trilinear interpolation in shader unlabeled (one-line comment)
MAX_LUT_SIZEduplicatesDEFAULT_MAX_SIZEfromcolorLuts.tsColorGradingTargettype duplication inhyperframes-player.ts
No React code in this PR — useEffect audit N/A. CI green at d1162cd.
— Miga
miga-heygen
left a comment
There was a problem hiding this comment.
Re-approve after rebase. HEAD is still d1162cd — no code changes since R2. All prior findings and approval stand.
— Miga
What
Applies color grading at runtime for media elements in preview/player/render.
This is PR 2 of 3 in the color grading stack. It is stacked on top of the core schema/LUT parsing PR.
Why
Users should be able to preview and render shader-based color adjustments and uploaded/project
.cubeLUTs without baking changes into the source media.How
window.__hf.colorGrading..cubeLUT loading.Test plan
Verification run:
bun run --filter @hyperframes/core test -- src/colorGrading.test.ts src/colorLuts.test.ts src/runtime/colorGrading.test.ts src/runtime/bridge.test.ts src/compiler/htmlBundler.test.tsbun test src/services/fileServer.test.tsfrompackages/producerbun run --filter @hyperframes/core build