feat(core): add professional color grading controls

[ukimsanov]

What

• Add three-way tonal wheels, master/RGB curves, hue-selective curves, and up to four bypassable HSL secondaries to the shared color-grading contract.
• Compile the advanced controls into deterministic GPU lookup data used by the existing media-treatment shader.

Why

HyperFrames has primary correction, looks, effects, and user-provided LUTs, but it lacks the selective controls expected from a real SDR color-grading workflow.

How

• Keep one canonical versioned contract in parsers/core.
• Preserve the existing single media-treatment pass and process: primary correction -> wheels -> RGB curves -> hue curves -> HSL secondaries -> 3D LUT.
• Use one signature-cached RGBA8 lookup texture, with no extra canvas, WebGL context, or render pass.
• Preserve existing Effects, Palette, overlay, canvas-position, and compositing behavior.
• Scope this contract to the current Rec.709/SDR workflow; it does not claim native HDR, LOG transforms, ACES, or OCIO.

Test plan

• Unit tests added/updated
• Parsers: 899 passing
• Core/runtime: 1,409 passing
• Typecheck, build, oxlint, oxfmt, Fallow, and repository hooks
• Browser/video/software-producer pixel and fallback checks

Stack: 1/3. Base: main. Next: #2798.

[Copilot]

Pull request overview

Adds “pro” SDR/Rec.709 color-grading controls (tonal wheels, RGB & hue curves, and up to 4 HSL secondaries) to HyperFrames’ shared grading contract, and compiles them into deterministic GPU lookup data consumed by the existing single-pass media-treatment shader.

Changes:

• Bumped the parser-side color grading contract to v2 and added validation for wheels/curves/hue-curves/secondaries.
• Added curve compilation utilities and expanded core normalization/serialization/capabilities to include advanced grading controls.
• Extended the runtime WebGL shader + uniform/texture plumbing to apply advanced grading in-order, with signature-cached lookup uploads and serialized preview rendering.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
packages/parsers/src/colorGradingContract.ts	Contract v2 keys/constants and structural validation for advanced grading sections.
packages/parsers/src/colorGradingContract.test.ts	New parser tests covering acceptance/rejection/variable passthrough for advanced sections.
packages/core/src/runtime/colorGrading.ts	Runtime shader/uniform/texture changes to apply advanced grading using a cached lookup texture and serialized preview batches.
packages/core/src/runtime/colorGrading.test.ts	Added tests asserting shader integration, advanced texture upload gating/caching, and preview serialization behavior.
packages/core/src/index.ts	Re-exported new curve helpers/types and grading capability helpers.
packages/core/src/colorGradingCurves.ts	New curve sampling/compilation utilities for RGB and periodic hue curves.
packages/core/src/colorGradingCurves.test.ts	Unit tests for curve compilation behavior and validation.
packages/core/src/colorGrading.ts	Core contract/types expanded; normalization/serialization/capabilities updated for advanced controls + authored-value detection.
packages/core/src/colorGrading.test.ts	New tests for capabilities v2, advanced normalization/serialization, variable resolution in arrays, and authored-value behavior.

Comments suppressed due to low confidence (1)

packages/core/src/runtime/colorGrading.ts:993

• advancedConfig() also hard-codes the LUT width and the row center (0.8333333). It should be derived from HF_COLOR_CURVE_LUT_SIZE and ADVANCED_TEXTURE_HEIGHT to avoid silent breakage if the advanced texture layout changes.

  "vec4 advancedConfig(float index){",
  "  return texture2D(u_advanced, vec2((index + 0.5) / 1024.0, 0.8333333));",
  "}",

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[miguel-heygen]

Findings

• Important — remove the unchecked non-null assertion cluster from the new curve compiler. packages/core/src/colorGradingCurves.ts:29-50 and :122-132 use array[index]! throughout ordinary index arithmetic, and packages/core/src/colorGrading.ts:1149-1150 repeats the pattern. CONTRIBUTING.md:55 explicitly forbids non-null assertions outside post-if-checked paths. These assertions also make the interpolation invariants harder to audit: the validation happens downstream of pointSlopes, while the helper itself advertises no minimum-length precondition. Validate/guard once at the owning boundary, then iterate with checked pairs or hoist checked neighbors so the compiler and reader share the same proof.

• The existing unresolved Copilot comment on packages/core/src/runtime/colorGrading.ts:988 remains valid and is part of this verdict: the GLSL embeds 1024/1023/3 and the row center independently from HF_COLOR_CURVE_LUT_SIZE and ADVANCED_TEXTURE_HEIGHT. That duplicates the texture-layout contract across JS and shader source, so a later layout change can silently sample the wrong texels. Generate those shader literals from the canonical constants or pass dimensions as uniforms.

The overall shape is good: the parser owns the public key registry, core centralizes normalization/serialization, PCHIP sampling is isolated and tested, and the runtime keeps one cached texture and one pass. Focused verification passed for the new curve/parser tests, all 51 runtime WebGL tests, core/parser typechecks, oxlint, oxfmt, and diff check.

Verdict: Request changes.
Reasoning: The implementation is behaviorally well tested, but it adds a documented type-safety violation in the core interpolation path and leaves the GPU texture layout with two sources of truth. Both are small, local fixes that should land before this becomes the public advanced-grading contract.

— Magi

[jrusso1020]

Reviewed with the lens you asked for: duplication, prior art, and whether each new abstraction earns its keep. I read the full changed files rather than the diff, and grepped for existing helpers before accepting anything new as necessary.

Additive only — prior coverage I'm deliberately not restating: Copilot and @miguel-heygen both flagged the GLSL hard-coding the advanced-texture layout (1024/1023/3/0.8333333) instead of deriving from HF_COLOR_CURVE_LUT_SIZE / ADVANCED_TEXTURE_HEIGHT; my finding 4 below is the JS-side sibling of that same class, not a repeat. @miguel-heygen also flagged the non-null-assertion cluster against CONTRIBUTING.md:55 — I confirmed it independently (22 bracket-index ! in a 187-line file) and agree it's the right call, so I'll only add the remedy angle: most of that density is pointSlopes/compileCurveSamples doing manual index arithmetic where iterating pairs directly would remove the indexing (and the assertions) rather than just guarding it. Everything below is a gap neither of them raised.

Audited: colorGradingCurves.ts, colorGrading.ts (normalization + capabilities), parsers/colorGradingContract.ts, runtime/colorGrading.ts (shader chain, texture/uniform plumbing, draw path) — read end-to-end.
Trusting: the four test files (read for coverage of the findings below, not line-by-line), and the aggregate suite counts in the body — I verified the pieces I could run cheaply and leaned on CI's green Test/Typecheck at this head for the 899/1,409 figures rather than claiming I reran them.

What's genuinely well done

• The stated pipeline order is real. I verified it against the body's claim rather than taking it: applyColorGrade (:1147) → applyPrimaryGrade (:1148) → applyAdvancedGrade (:1084, which is wheels :1085 → RGB curves :1086 → hue curves :1087 → secondaries :1088-1091) → applyLut (:1150). Matches "primary correction → wheels → RGB curves → hue curves → HSL secondaries → 3D LUT" exactly.
• Single pass genuinely preserved. The advanced data rides one extra NEAREST RGBA8 sampler on TEXTURE5 (:1653, :2764) — no new framebuffer, render pass, canvas, or WebGL context. The claim holds structurally.
• compileHfHueCurve gets periodicity right, which is the easy thing to botch here. It pads two points below (hue - 360) and two above (hue + 360) (colorGradingCurves.ts:178-179) and samples index / size * 360 rather than size - 1 (:183), so the 0/360 texel isn't duplicated. The padded sequence is also provably strictly increasing (h[n-1] - 360 < 0 ≤ h[0]), so validateCurvePoints' monotonicity requirement can't trip on the synthetic points.
• Overshoot control is the right call and is pinned. The Fritsch–Carlson-style tangent limiting in endpointSlope/pointSlopes (:9-56) keeps grading curves from ringing, and there's a test holding it ("does not overshoot a non-monotone curve").
• No new lint waivers. I checked: 7 fallow-ignore on main and 7 at this head — the advanced work was added without widening the suppression list, even though it extends the already-waived applyUniforms (71 → 88 lines).

Findings

1. The same numeric bounds now live in three layers, with nothing keeping them in sync — important

The identical constraints are independently expressed in:

• packages/parsers/src/colorGradingContract.ts:505-513, 536, 492 — what validation rejects (center 0 up-to-360, range 0-180, softness 0-180, range + softness ≤ 180, hueShift ±180, soft-range softness 0-0.5).
• packages/core/src/colorGrading.ts:1008-1026 — what the capability contract tells agents is legal (maxExclusive: 360, rangePlusSoftnessMax: 180, …).
• packages/core/src/colorGrading.ts:1233-1249 — what the runtime actually clamps to (max: 180, max: 180 - range, hueShift ±180).

To be clear, I checked and all three agree today — including the boundary I expected to catch them out. hue: 360 is rejected by parsers (inclusiveMax = false at :452/:505), declared out of range by capabilities (maxExclusive: 360), and wrapped to 0 by wrapDegrees — and capabilities honestly advertises that with wrap: true. So this is not a live bug, and the design is coherent.

The problem is that nothing pins them together, and these are precisely the three things that must never disagree: what an agent is told is legal, what validation rejects, and what the runtime silently clamps. Note the asymmetry that makes drift quiet: variable refs skip validation entirely (validateNumericField early-returns on isColorGradingVariableRef), so a variable resolving out of range only ever meets the clamp — no issue is raised. This is the same surface the effectSpecs drift came from on #2755, and the fix chosen there was to derive from getHfColorGradingCapabilities() instead of re-declaring. Either derive the parsers NumericLimit entries and the core clamp limits from one table, or add a test asserting capability bounds == validation bounds == clamp bounds.

2. ensureAdvancedTexture builds a JSON.stringify cache key on the per-frame draw path — nit, but hot

runtime/colorGrading.ts:2757 sits inside applyUniforms (:2840), which sits inside drawEntry (:3019) — so it runs once per graded element per frame.

I measured it rather than guess, on a worst-case payload (4 RGB curves + 3 hue curves at the 16-point max, 4 secondaries): 4,581-byte string, ~23 µs per call. That's ~0.07% of a 30 fps frame for a single element, so I'm explicitly not grading it a blocker. What makes it worth a line: it's ~2.7 MB/s of throwaway string garbage at 10 graded elements @ 60 fps, and it's the only JSON.stringify in this 3,781-line file — the neighbouring cache guard in the same file (geometrySignature, :3357) uses a cheap template string of a few scalars. A revision counter bumped on grading mutation, or hashing the handful of scalars, buys the same guard for near-free.

Minor sub-point: the signature covers all of secondaries while buildAdvancedTextureData only consumes .slice(0, 4) (:2745), so editing a 5th secondary invalidates the cache and re-uploads a byte-identical texture. Wasted work only.

3. Byte quantization is now written a third time, with different edge semantics — nit

:2762 inlines Math.round(Math.min(1, Math.max(0, value)) * 255). packages/core/src/colorLuts.ts already has exactly this as toByte (:202) over clampUnit (:197). Both are module-private, so this is a missed extraction rather than a missed import — but note the two copies disagree on non-finite input: clampUnit maps Infinity → 0, the inline version maps Infinity → 255. buildAdvancedTextureData can't currently emit non-finite values, so nothing is broken; two copies of the same quantization with divergent edge behaviour is just the kind of thing that bites a year later.

4. .slice(0, 4) hardcodes what a constant already names, and is redundant besides — nit

:2745 uses a literal 4 where COLOR_GRADING_MAX_SECONDARIES exists — and normalizeSecondaries (colorGrading.ts:1273) already caps to that same constant before this code ever runs, so the slice is a no-op in practice. Same magic-number class Copilot flagged in the shader, on the JS side.

5. Redundant guard inside one call path — nit

compileHfColorCurve checks points.length < 2 (colorGradingCurves.ts:144), then delegates to compileCurveSamples → validateCurvePoints, which checks points.length < 2 again (:59) with a different message. One of the two is unreachable from this entry point.

6. Two new public exports have no consumer outside packages/core — nit

compileHfColorCurve / compileHfHueCurve are re-exported from packages/core/src/index.ts, but the only callers anywhere are colorGrading.ts and runtime/colorGrading.ts; nothing in cli, studio, engine, producer, skills, or docs references them. That commits @hyperframes/core's published API to curve-compilation internals (default LUT size, Float32Array return type, RangeError message shapes) for no current gain. In fairness I checked the house precedent before raising it: the accompanying HF_COLOR_GRADING_*_KEYS exports match existing style — EFFECT_KEYS/DETAIL_KEYS/ADJUST_KEYS are already exported with no out-of-package consumers either — so I'm only flagging the two functions, not the constants.

On CI, because the UI is actively misleading here

The PR shows a red regression, but both failing regression check-runs belong to cancelled workflow runs (30189181236, 30189215281) that were superseded by rapid re-pushes. The live run (30189247148) was still in_progress when I looked, with all 8 shards pending. So there is no confirmed red gate — and equally, the required suite hasn't gone green yet, so this isn't ready to merge on that basis alone. Worth a re-check at whatever the final head is.

Scope note

At 1,968 added lines this is past the point where the repo norm suggests stacking rather than one PR. It is stacked (1/3), which covers most of it — but the three units inside this one PR are separable and would each review faster on their own: the parsers contract bump, the curve compiler (colorGradingCurves.ts is self-contained and independently testable), and the runtime shader/GPU plumbing. Not asking you to re-cut it at this stage; flagging for the next one.

Verdict: COMMENT (additive to @miguel-heygen's REQUEST_CHANGES, which stands on its own merits)
Reasoning: I found no correctness defect of my own — the pipeline order, single-pass claim, and hue periodicity all verified true, and the bounds triplication I expected to be a live inconsistency turned out consistent when I actually checked the boundary. My findings are maintainability and SSOT, with finding 1 the one worth a decision before this contract hardens into three places that must agree forever. Not approving: the required suite hasn't finished at this head, @miguel-heygen has changes outstanding, and approval isn't mine to give here regardless.

— hyperframes

[jrusso1020]

Re-review at 9a515858. Every finding from my f99fc4e5 pass is addressed, and two of them landed the stronger option rather than the cheap one. No new correctness defect found.

Additive only. @miguel-heygen is posting its own re-review in parallel; I'm not restating its confirmations. What's mine below is the verification depth on the two fixes that could plausibly have introduced a new bug (the cache memo and the moved validation call), plus one duplication the fix left behind.

Audited: colorGradingCurves.ts, colorGrading.ts, colorGradingContract.ts, runtime/colorGrading.ts, colorLuts.ts, index.ts — diffed f99fc4e5..9a515858 and read the changed regions in full.
Verified by running (at this head, in a clean worktree): colorGrading.test.ts + colorGradingCurves.test.ts 42/42, colorGradingContract.test.ts 6/6, oxlint . 0 warnings / 0 errors, tsc --noEmit clean on core and parsers.

The two fixes I checked hardest, because they could have broken something

The WeakMap signature memo is sound — I verified the two conditions it depends on

advancedTextureSignature (runtime/colorGrading.ts:2761-2780) replaces the per-frame JSON.stringify with an identity-keyed memo. That's the right shape, but it trades a fresh-every-frame comparison for one that assumes identity implies unchanged contents — so a stale signature would mean ensureAdvancedTexture early-returns and the texture never re-uploads. Two things had to hold, and both do:

1. No in-place mutation. I grepped curves / hueCurves / secondaries repo-wide for property writes, indexed writes, and mutating array methods (push/splice/sort/fill/…). Zero hits in JS — the only match was the GLSL string at :1037. Normalizers build fresh objects, and :3476 replaces the whole grading rather than mutating it.
2. Identity is actually stable per-frame, or the memo would be a no-op that still pays the stringify. readAnimatedGrading (:2997-3017) either returns grading untouched on the fast path or shallow-spreads it — a shallow spread doesn't override those three keys, so identity survives. So the memo hits, and the animated path can't stale it.

Worth knowing for later: this is now load-bearing. Any future code that mutates a normalized grading in place instead of replacing it will silently serve a stale texture, and the failure is visual, not a crash. A one-line comment on ADVANCED_SIGNATURES saying "identity-keyed: normalized grading must be replaced, never mutated" would make that contract visible to the next person.

Moving validateCurvePoints out of compileCurveSamples didn't drop validation anywhere

The redundant guard is gone the right way — validateCurvePoints moved out of the shared helper and into both entry points. I audited every caller rather than assume two: compileCurveSamples has exactly two callsites repo-wide (:167, :195), and both now validate explicitly, so no path lost its check. The compileHfHueCurve side validates the padded periodic array, matching the old behaviour.

Two details that make this safe rather than lucky: the <2-points message lives inside validateCurvePoints with identical text, so the pinned toThrow("at least two points") still holds; and validateCurvePoints deliberately doesn't check max-points, so padding four synthetic points past the 16-point limit can't trip a spurious error. Only observable change is error precedence — input that is both over-length and non-monotone now reports monotonicity first. Nothing depends on it.

valueAt plus the pair-iteration rewrite of pointSlopes is more than the assertion cluster needed: it removes the index arithmetic rather than guarding it, which is the version I'd hoped for.

Confirmed fixed (compactly, since @miguel-heygen is also re-checking)

• Bounds triplication → derived, not just tested. COLOR_GRADING_ADVANCED_LIMITS (colorGradingContract.ts:5-14) is now the single source, consumed by all three layers: validation (:459-472, :506-517, :525-551, :572-580), capabilities (:980-1042), and the runtime clamps (:1235-1272). Derivation was the stronger of the two options I offered, and there are now tests pinning it from both sides (colorGrading.test.ts:181-197, colorGradingContract.test.ts:24-29). The inclusiveMax: false flag keeps the exclusive-360 semantics encoded once instead of re-asserted per callsite.
• Quantization → toByte exported as unitFloatToByte (colorLuts.ts:202) and consumed by the runtime (:2793). The edge divergence I flagged is resolved in the safer direction: non-finite now maps to 0 via clampUnit, not 255.
• .slice(0, 4) → replaced by an enabled-filter loop, and it's safe to drop the cap: normalizeSecondaries still bounds to COLOR_GRADING_MAX_SECONDARIES (colorGrading.ts:1292) and the shader reads at most four (:1094-1097). Texture contents are equivalent — enabled secondaries at compacted indices, same source order.
• Dead public exports → both curve compilers removed from index.ts.
• CI: retracting my prior note. All six workflow runs at this head are success; the red regression was the cancelled-run artifact I described and it's gone.

One duplication the fix left next to itself — nit

EFFECT_LIMIT_OVERRIDES is still declared independently in both packages, byte-identical in all four entries (asciiStyle 0-7, bloom 0-3, bloomRadius 1-100, monoScreenShape 0-4) — colorGradingContract.ts and colorGrading.ts. It agrees today, and it's pre-existing rather than introduced here, so I'm not asking you to gate on it. But it's the same cross-package literal-duplication class the new table just eliminated, sitting in the same two files, and it's the surface that already drifted once on #2755. Now that COLOR_GRADING_ADVANCED_LIMITS exists and core already imports it, folding these in is a couple of lines. Same for DETAIL_LIMITS, where the {min:0,max:1} / {min:-1,max:1} literals could read .unit / .signedUnit — core-only, so no cross-package drift risk there, purely consistency.

Verdict: COMMENT — no blocking findings from me; ready on the merits at this head.
Reasoning: All six of my prior findings are fixed, two with the better option, and the two riskier fixes verified sound under a mutation-and-callsite audit rather than on inspection. Not approving: @miguel-heygen's changes-requested is still the gate, and a stamp here is James's call, not mine.

— hyperframes

[miguel-heygen]

Re-review at exact head 9a515858c9c65b749f1a976b3a72c5f4868a8256.

Both findings from my prior change request are resolved:

• The interpolation path no longer relies on unchecked indexed non-null assertions. The shared valueAt boundary and pair-oriented iteration make the proof explicit and comply with CONTRIBUTING.md:55.
• The shader's LUT dimensions and row coordinates are generated from HF_COLOR_CURVE_LUT_SIZE and the canonical advanced-texture row constants; the JS/GLSL layout no longer has independent magic numbers.

The follow-up also improves the design beyond the minimum fix: parser validation, capability metadata, and runtime clamps now derive from one COLOR_GRADING_ADVANCED_LIMITS table; byte quantization is shared; redundant guards and unneeded public compiler exports are removed; and the per-frame signature uses an identity memo without any in-place mutation path that could stale it.

Fresh exact-head verification: 48/48 focused core/parser tests, core and parser typechecks, diff check, and the complete GitHub CI/regression matrix are green.

Verdict: APPROVE
Reasoning: The documented type-safety violation and GPU-layout SSOT defect are fixed at their owning boundaries, the maintainability follow-ups are sound, and exact-head verification is fully green.

— Magi

[ukimsanov]

Addressed the remaining review requests in e446de602: effect and finishing limits now derive from the canonical parser-owned contract; Core exposes the shared secondary-mask calculation used for Studio/runtime parity; and saturation/luma softness tests exercise the real RGBA8 pack/decode path. Focused parser/Core tests pass (6/6 and 94/94), both typechecks pass, and lint/format are clean.

[miguel-heygen]

Restamp at exact head e446de60234b837672100334fd169d2f2b547218.

The delta from Magi's last approved head is the single fix(core): align secondary mask contracts commit. It takes the stronger SSOT route:

• parser validation, core capabilities, and runtime clamps derive from COLOR_GRADING_ADVANCED_LIMITS;
• the product secondary qualifier is centralized as calculateHfColorGradingSecondaryMask;
• saturation/luma softness is pinned against the runtime texture pack/decode convention.

This delta was also exercised as the downstack contract during #2797's exact-head review. Fresh verification on the current stack: Core color-grading/parity tests 39/39, parser contract tests 6/6, Core and parser typechecks pass, diff check is clean, and all 56 exact-head check runs are terminal green.

Verdict: APPROVE
Reasoning: The stale approval delta reduces duplicated contracts, is regression-covered, and introduces no new correctness or design blocker.

— Magi