diff --git a/docker/frontend/scripts/highlight-knee-check.mjs b/docker/frontend/scripts/highlight-knee-check.mjs index ac1fcb4..a8ffd16 100644 --- a/docker/frontend/scripts/highlight-knee-check.mjs +++ b/docker/frontend/scripts/highlight-knee-check.mjs @@ -60,28 +60,44 @@ const mathTmpl = tone.match(/export const TONE_MATH_SKSL = `([\s\S]*?)`;/)?.[1]; assert.ok(mathTmpl, 'TONE_MATH_SKSL is gone — the frame and a mask no longer share the maths'); assert.equal((mathTmpl.match(/\$\{TONE_ANCHOR\}/g) ?? []).length, 4, 'a knot is pinned to a literal, not to TONE_ANCHOR'); assert.ok(tmpl.includes('${TONE_MATH_SKSL}'), 'the frame pass carries its own copy of the ramp again'); -assert.match(tmpl, /rgb = toneRamp\(rgb, t, baseLuma\(xy, t\), bl, sh, hl, wh, dr\);/); -// The BASE layer the ramp is drawn through, and the radius the caller spends on -// it. It reads the CHILD nine times — a caller whose bx is zero reads its own -// pixel nine times and gets the global move back, which is what makes the shared -// maths safe for a mask (see the mask's own call below). -assert.match(tmpl, /float baseLuma\(vec2 xy, float t\) \{/); -assert.match(tmpl, /vec3 s = clamp\(src\.eval\(xy \+ vec2\(float\(i\), float\(j\)\) \* bx\)\.rgb, 0\.0, 1\.0\);/); -assert.match(tmpl, /float rw = exp\(-d \* d \* 24\.0\);/); -assert.doesNotMatch(tmpl, /if \(i == 0 && j == 0\) continue;/, 'the 3x3 grew a branch — bx = 0 no longer returns exactly t'); -assert.match(tmpl, /uniform float2 bx;/); +assert.match(tmpl, /rgb = toneRamp\(rgb, t, baseLuma\(xy\), bl, sh, hl, wh, dr\);/); +// The BASE layer the ramp is drawn through. It is ONE tap of a blurred child, +// and the blur is the caller's (blurredBase in exportEngine.ts) — a ring of point +// samples in here was the mottle bug: the luma aliased on a textured frame, the +// gain o(base)/base carried the alias, and the reconstruction painted it back. +// So the shader must read `base` once and must NOT grow a sampling loop again, +// and `bx` — the step only a loop ever needed — must stay gone. +assert.match(tmpl, /float baseLuma\(vec2 xy\) \{/); +assert.match(tmpl, /vec3 s = clamp\(base\.eval\(xy\)\.rgb, 0\.0, 1\.0\);/); +assert.match(tmpl, /uniform shader base;/); +assert.doesNotMatch(tmpl, /uniform float2 bx;/, 'the base is a sampling loop again — that is what mottled'); +assert.doesNotMatch(tmpl, /baseLuma\(xy, t\)/, 'baseLuma grew its neighbourhood back'); // One tap of the base is a FRACTION of the frame, so the preview and the file -// look at the same neighbourhood: the pass has the frame size, the shader has a -// step in its own pixels. +// look at the same neighbourhood: the pass has the frame size and turns it into +// the blur's sigma. assert.match(tone, /export const TONE_BASE_RADIUS = ([0-9.]+);/); const baseRadius = Number(tone.match(/export const TONE_BASE_RADIUS = ([0-9.]+);/)[1]); assert.ok(baseRadius >= 0.02 && baseRadius <= 0.05, `the base reads ${baseRadius} of the frame — the doc asks for 2%..5%`); +assert.match(tone, /export const TONE_BASE_SIGMA = ([0-9.]+);/); +const baseSigma = Number(tone.match(/export const TONE_BASE_SIGMA = ([0-9.]+);/)[1]); +assert.ok(baseSigma > 0 && baseSigma <= 0.5, `TONE_BASE_SIGMA ${baseSigma} is not a sigma under the radius`); const engine = readFileSync(new URL('../src/engine/exportEngine.ts', import.meta.url), 'utf8'); assert.match( engine, - /getToneUniforms\(adjustments, recipe\.baseFilter, \[\s*width \* TONE_BASE_RADIUS,\s*height \* TONE_BASE_RADIUS,\s*\]\)/, - 'the tone pass no longer hands the base a frame-sized step' + /const sigma = width \* TONE_BASE_RADIUS \* TONE_BASE_SIGMA;\s*\n\s*const base = own\(blurredBase\(baseShaderOf, width, height, sigma\)\);/, + 'the tone pass no longer blurs a frame-sized base' ); +assert.match( + engine, + /effect\.makeShaderWithChildren\(toneUniformArray\(tone\), \[\s*baseShaderOf\(\),\s*base \? own\(imageShaderChild\(base\)\) : baseShaderOf\(\),\s*\]\)/, + 'the blurred base is not handed to the tone pass as its second child' +); +assert.match( + engine, + /function blurredBase\([\s\S]*?Skia\.ImageFilter\.MakeBlur\(sigma, sigma, Skia\.TileMode\.Clamp, null\)/, + 'the base is no longer Skia’s own blur' +); +assert.match(engine, /getToneUniforms\(adjustments, recipe\.baseFilter\)/, 'the tone pass still steps a sampling ring by hand'); // The uniform block: the shader's declarations, arrays expanded and in // declaration order, have to be the numbers `toneUniformArray` writes — a // mismatch is a silent off-by-one down the whole block. diff --git a/docker/frontend/scripts/tone-base-check.mjs b/docker/frontend/scripts/tone-base-check.mjs new file mode 100644 index 0000000..def9906 --- /dev/null +++ b/docker/frontend/scripts/tone-base-check.mjs @@ -0,0 +1,120 @@ +// The tone ramp is drawn through a BASE layer, and that base is now a blurred +// CHILD of the pass rather than a ring of point samples inside it — see +// TONE_BASE_RADIUS in toneShader.ts for the mottle that ring caused. Two things +// can go wrong with that and neither is a crash: the shader stops compiling once +// it takes a second child (SkSL is only checked at runtime, and no other check +// here compiles TONE_SKSL as a whole), or the second child is wired to the wrong +// slot and the pass quietly reads the sharp image as its own base — which is +// exactly the identity the default (bx = 0) used to give, so a regression would +// look like nothing happening. So the pass is compiled and rendered for real: +// the frame and the base are held at two different flat values, and the pixel +// has to land on the somewhere-between value the ramp over THAT base predicts. +// +// node scripts/tone-base-check.mjs +import assert from 'node:assert/strict'; +import { mkdtempSync, readFileSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { fileURLToPath, pathToFileURL } from 'node:url'; +import ts from 'typescript'; +import CanvasKitInit from 'canvaskit-wasm/bin/full/canvaskit.js'; + +const transpile = (path) => + ts.transpileModule(readFileSync(new URL(path, import.meta.url), 'utf8'), { + compilerOptions: { module: ts.ModuleKind.ESNext, target: ts.ScriptTarget.ES2022 }, + }).outputText; + +const dir = mkdtempSync(join(tmpdir(), 'tone-base-check-')); +writeFileSync(join(dir, 'colorUtils.mjs'), transpile('../shared/utils/colorUtils.ts')); +writeFileSync( + join(dir, 'toneShader.mjs'), + transpile('../shared/utils/toneShader.ts').replace( + /^import .*from ['"]\.\/colorUtils['"];$/m, + 'import { HSL_BANDS, hslBandGaps, isMonochromeBase } from "./colorUtils.mjs";', + ), +); +const { TONE_SKSL, TONE_BASE_RADIUS, TONE_BASE_SIGMA, getToneUniforms, toneUniformArray, toneIsActive } = + await import(pathToFileURL(join(dir, 'toneShader.mjs')).href); + +// The shader has to NAME a base child — a ring of taps would not need one. +assert.match(TONE_SKSL, /uniform shader base;/); + +const CanvasKit = await CanvasKitInit({ + locateFile: () => fileURLToPath(new URL('../node_modules/canvaskit-wasm/bin/full/canvaskit.wasm', import.meta.url)), +}); +const SIZE = 8; + +function flat(value) { + const surf = CanvasKit.MakeSurface(SIZE, SIZE); + const paint = new CanvasKit.Paint(); + paint.setColor(CanvasKit.Color(value, value, value)); + surf.getCanvas().drawPaint(paint); + return surf.makeImageSnapshot(); +} + +const asChild = (image) => + image.makeShaderOptions( + CanvasKit.TileMode.Clamp, CanvasKit.TileMode.Clamp, CanvasKit.FilterMode.Linear, CanvasKit.MipmapMode.None, + ); + +// SHADOW full, everything else off — the knob the mottle was reported on. +const adjustments = { shadow: 10 }; +const uniforms = getToneUniforms(adjustments); +assert.ok(toneIsActive(uniforms), 'SHADOW +100 no longer turns the pass on'); + +function render(srcValue, baseValue) { + const effect = CanvasKit.RuntimeEffect.Make(TONE_SKSL); + assert.ok(effect, 'TONE_SKSL does not compile'); + const src = flat(srcValue); + const base = flat(baseValue); + const shader = effect.makeShaderWithChildren(uniforms ? toneUniformArray(uniforms) : [], [ + asChild(src), + asChild(base), + ]); + assert.ok(shader, 'the pass did not take two children — the base is not wired in'); + const out = CanvasKit.MakeSurface(SIZE, SIZE); + const paint = new CanvasKit.Paint(); + paint.setShader(shader); + out.getCanvas().drawPaint(paint); + const pixels = out.makeImageSnapshot().readPixels(0, 0, { + width: SIZE, height: SIZE, colorType: CanvasKit.ColorType.RGBA_8888, + alphaType: CanvasKit.AlphaType.Unpremul, colorSpace: CanvasKit.ColorSpace.SRGB, + }); + return pixels[0]; +} + +// The ramp, in the same arithmetic the shader runs: SHADOW +100 puts a1 (the +// 0.25 knot) on 0.375, and 0.50 is fixed, so a base of b below a half reads +// a1 + (0.5 - a1) * (b - 0.25) / 0.25. +const ramp = (b) => 0.375 + (0.5 - 0.375) * ((b - 0.25) / 0.25); + +const srcValue = 128; // the pixel: 0.501961 encoded +const baseValue = 76; // its neighbourhood, darker: 0.298039 +const t = srcValue / 255; +const b = baseValue / 255; +const expected = Math.round(255 * ((ramp(b) * t) / b)); + +const got = render(srcValue, baseValue); +assert.ok( + Math.abs(got - expected) <= 2, + `a base of ${b.toFixed(4)} under a pixel of ${t.toFixed(4)} gave ${got}, the ramp over that base predicts ${expected}`, +); +// And the two are NOT the same value: if the pass had quietly read the sharp +// image as its base the answer would be the pixel itself, unchanged. +assert.ok( + Math.abs(got - srcValue) > 8, + `the pass returned the pixel (${got}) — it is reading its own sharp image as the base again`, +); +// With the base handed in as the sharp image the pass is the global move, which +// is what a mask (no neighbourhood of its own) needs it to be. +const selfBase = render(srcValue, srcValue); +assert.ok( + Math.abs(selfBase - srcValue) <= 1, + `a base equal to the pixel must be the identity, got ${selfBase} for ${srcValue}`, +); + +console.log( + `tone base ok: pixel ${srcValue} over a base of ${baseValue} -> ${got} ` + + `(ramp predicts ${expected}, sharp-base identity ${selfBase}); ` + + `radius ${TONE_BASE_RADIUS} of the frame, sigma ${TONE_BASE_SIGMA} of it`, +); diff --git a/docker/frontend/shared/utils/toneShader.ts b/docker/frontend/shared/utils/toneShader.ts index cd38156..faccb5e 100644 --- a/docker/frontend/shared/utils/toneShader.ts +++ b/docker/frontend/shared/utils/toneShader.ts @@ -65,8 +65,8 @@ import { HSL_BANDS, hslBandGaps, isMonochromeBase } from './colorUtils'; // It costs nothing where there is no lift to make: with every knob on zero the // ramp at base IS base, so the ratio is exactly 1 and the pass is the identity // however coarse the base is. A caller that hands in no neighbourhood at all -// (bx = 0, or a mask, which has none) reads its own pixel nine times and gets the -// global move back — which is why the shared maths can take the base as an +// (a mask, which has none) hands in the pixel's own image as the base and gets +// the global move back — which is why the shared maths can take the base as an // argument and mean the same thing in both places. // // Between two knots the ramp is drawn STRAIGHT, and that is deliberate: a @@ -142,15 +142,32 @@ const BAND_BLOCK = hslBandGaps() export const TONE_ANCHOR = 0.25; // How far out the BASE layer of `toneRamp` reads, as a fraction of the frame's -// own width and height — the fix_shadow.md neighbourhood (it asks for 2%..5% of -// the width). A fraction rather than a pixel count so the preview and the export -// look at the same neighbourhood, and the measurement is flat across the range -// anyway: full deflection on the sample frame keeps 0.77 of the band's spread at -// 0.7%, 0.80 at 2.5%, 0.81 at 4.8%. The low end of the doc's range, because a -// wider base is a wider neighbourhood for a strong edge to be reconstructed -// across. +// own width — the fix_shadow.md neighbourhood (it asks for 2%..5% of the width). +// A fraction rather than a pixel count so the preview and the export look at the +// same neighbourhood, and the measurement is flat across the range anyway: full +// deflection on the sample frame keeps 0.77 of the band's spread at 0.7%, 0.80 at +// 2.5%, 0.81 at 4.8%. +// +// The caller turns this into a BLUR, and it took a bug to make that a blur in +// fact and not only in name. The base used to be nine point samples of the child +// out at plus or minus this radius, and point samples are not an average: on a +// frame with texture at the sampling scale — a waterfall, a mountainside — the +// nine-tap luma aliases, the gain o(base)/base inherits the alias, and the +// reconstruction paints it as mottle. Measured on a 1160x774 frame at +// SHADOW +100, the high-frequency (9px high-pass) part of that gain field was +// 0.063 against 0.005 for a real blur of the same radius — 13x. So the base is +// now a real gaussian blur of the child, made by the caller (Skia's own +// MakeBlur, see blurredBase() in exportEngine.ts) and read here as ONE tap. That +// is also the cheaper pass: nine child evals walk the whole exposure/matrix +// chain nine times, one eval does not. export const TONE_BASE_RADIUS = 0.025; +// One gaussian sigma of the base blur, as a fraction of TONE_BASE_RADIUS. A box +// of the same radius and a gaussian of this sigma carry the same weight at the +// radius, so the neighbourhood is the one the radius has always named while the +// cuts are smooth instead of hard. +export const TONE_BASE_SIGMA = 0.35; + // The tone and exposure maths, in ONE copy, because two passes ask it: the // whole-frame passes here and a gradient mask, which moves the same knobs on the // shape the user drew. What "HIGHLIGHT" or "EXPOSURE" means must not depend on @@ -313,6 +330,10 @@ vec3 exposureMove(vec3 rgb, float ev) { export const TONE_SKSL = ` uniform shader src; +// The BASE layer's child: a blur of the image above, of TONE_BASE_RADIUS, made +// by the caller. Read for its luma alone, and read ONCE per pixel — the +// neighbourhood is the blur's, not a sampling loop's (see TONE_BASE_RADIUS). +uniform shader base; uniform float dr; uniform float hl; uniform float sh; @@ -334,10 +355,6 @@ uniform float hslL[8]; uniform float gh; uniform float gs; uniform float gl; -// One tap of the base layer, in the frame's own pixels (TONE_BASE_RADIUS of it). -// Zero is a legal neighbourhood — it is the one a caller with no frame to look -// at hands in, and it reads the pixel itself nine times. -uniform float2 bx; // sRGB <-> HSL. The mixer works in HSL because that is the space the knobs are // named after: a hue shift must not change how light a colour is, and a // lightness move must not change its hue, which is exactly what scaling RGB @@ -384,31 +401,20 @@ float bandW(float hue, float anchor, float gapL, float gapR) { } ${TONE_MATH_SKSL} // The BASE layer: the light the frame carries where this pixel sits, at the -// radius the caller hands in — a coarse blur of the LUMA, because the luma is the -// one quantity the ramp moves (the colour rides the ratio afterwards). Nine taps -// of the child at plus or minus one bx is not a guided filter; it is the doc's -// own cheap stand-in for one, and the range weight is what keeps it from being a -// blur across an edge: a tap whose luma is nothing like the centre's counts for -// little, so a dark crevice in a sunlit rock keeps its own base instead of being -// reconstructed against the rock. Same weight shape the CLARITY reference blurs -// with (CLARITY_BLUR_SKSL), on the luma rather than the colour. A three-by-three -// that INCLUDES the centre — no branch, and a caller whose bx is zero reads the -// same pixel nine times, which lands the base exactly on t and hands back the -// global move. -float baseLuma(vec2 xy, float t) { - float sum = 0.0; - float wsum = 0.0; - for (int i = -1; i <= 1; i++) { - for (int j = -1; j <= 1; j++) { - vec3 s = clamp(src.eval(xy + vec2(float(i), float(j)) * bx).rgb, 0.0, 1.0); - float ts = dot(s, vec3(0.2126, 0.7152, 0.0722)); - float d = ts - t; - float rw = exp(-d * d * 24.0); - sum += rw * ts; - wsum += rw; - } - } - return sum / wsum; +// radius the caller handed in. One tap of a real blur of the same child the +// pixel comes from, so it is an AVERAGE of the neighbourhood and not a handful +// of point samples of it — that distinction is the whole bug (see +// TONE_BASE_RADIUS). Luma, because the luma is the one quantity the ramp moves +// and the colour rides the ratio afterwards; the blur being linear, blurring the +// child and taking its luma is the same as blurring the luma. +// +// A caller with no neighbourhood to speak of hands in the child itself as the +// base (see blurredBase()): the tap then lands exactly on t and the pass falls +// back to the global move, which is what a shape with a mask's degenerate base +// wants and what it got from a bx of zero before. +float baseLuma(vec2 xy) { + vec3 s = clamp(base.eval(xy).rgb, 0.0, 1.0); + return clamp(dot(s, vec3(0.2126, 0.7152, 0.0722)), 0.0, 1.0); } vec4 main(vec2 xy) { vec4 c = src.eval(xy); @@ -431,7 +437,7 @@ vec4 main(vec2 xy) { // same knots, the same hue-preserving rebuild, the same move a gradient mask // makes with the same four sliders. DR is the whole frame's, so it is spent // here and nowhere else. - rgb = toneRamp(rgb, t, baseLuma(xy, t), bl, sh, hl, wh, dr); + rgb = toneRamp(rgb, t, baseLuma(xy), bl, sh, hl, wh, dr); // Split tone (stock look): the shadows and the highlights may each carry // their own tint, so the two ends of the curve can drift opposite ways // (Classic Neg: green-cyan darks, warm brights) without touching mid-greys. @@ -736,7 +742,6 @@ export interface ToneUniforms { gh: number; // -1..1 whole-image hue turn (±30° at full) gs: number; // -1..1 whole-image saturation scale gl: number; // -1..1 whole-image lightness offset (±0.25 at full, ungated) - bx: [number, number]; // one base-layer tap, in the frame's own pixels } // Per-stock tone pass. Fuji's Classic stocks are not a plain colour matrix: @@ -779,14 +784,14 @@ const FILM_TONE: Partial>> = { 'mono-high-contrast': { sh: -0.64, hl: 0.52 }, }; -// `baseStep` is one tap of the base layer in the frame's own pixels; the caller -// is the only one that knows how big the frame is (TONE_BASE_RADIUS of it). Left -// at zero the pass reads no neighbourhood and keeps the global ramp — see the -// note at the head of the file. +// The base layer's neighbourhood is the caller's business, not this function's: +// it is a blurred CHILD of the shader (see TONE_SKSL), so only the caller knows +// how big the frame is or whether there is a frame at all. A shape with a mask +// has no frame to look at and hands in the image it is already shading, which +// lands the base on the pixel and keeps the global ramp (see baseLuma). export function getToneUniforms( adj: ColorAdjustments, - baseFilter?: BaseFilter, - baseStep: [number, number] = [0, 0] + baseFilter?: BaseFilter ): ToneUniforms { const drRaw = adj.dynamicRange ?? 'auto'; const dr = drRaw === 'auto' || drRaw === 100 ? 0 : (drRaw - 100) / 300; @@ -852,7 +857,6 @@ export function getToneUniforms( gh, gs, gl, - bx: baseStep, }; } @@ -863,7 +867,6 @@ export function toneUniformArray(u: ToneUniforms): number[] { u.dr, u.hl, u.sh, u.wh, u.bl, u.vib, u.shT[0], u.shT[1], u.shT[2], u.hlT[0], u.hlT[1], u.hlT[2], u.cc, u.ccb, u.hslOn, ...u.hslH, ...u.hslS, ...u.hslL, u.gh, u.gs, u.gl, - u.bx[0], u.bx[1], ]; } diff --git a/docker/frontend/src/engine/exportEngine.ts b/docker/frontend/src/engine/exportEngine.ts index b9eac28..7686431 100644 --- a/docker/frontend/src/engine/exportEngine.ts +++ b/docker/frontend/src/engine/exportEngine.ts @@ -35,6 +35,7 @@ import { dehazeUniformArray, getToneUniforms, TONE_BASE_RADIUS, + TONE_BASE_SIGMA, toneIsActive, toneUniformArray, glowUniformArray, @@ -310,6 +311,40 @@ function drawBlurred(canvas: any, surface: any, w: number, h: number, sigma: num disposeAll([paint, filter, snap]); } +// The BASE layer of the tone ramp: the frame blurred out to TONE_BASE_RADIUS, as +// an image the tone shader takes as its second child and reads once per pixel. +// Blurred with Skia's own MakeBlur — the same draw drawBlurred makes for the +// sharpening pass — because what the ramp needs is an AVERAGE of the +// neighbourhood and a ring of point samples is not one: sampled on a fine ring +// the luma aliases, the gain o(base)/base inherits the alias, and the +// reconstruction paints it back as mottle (a 1160x774 frame at SHADOW +100 came +// out with 0.063 of high-frequency gain against 0.005 for this blur — see +// TONE_BASE_RADIUS in toneShader.ts). `shaderOf` is left owned by the caller: the +// tone pass reads the same shader as its first child. +function blurredBase( + shaderOf: () => any, + w: number, + h: number, + sigma: number +): any | null { + const surf = createSurface(w, h); + if (!surf) return null; + try { + const shader = shaderOf(); + if (!shader) return null; + const filter = Skia.ImageFilter.MakeBlur(sigma, sigma, Skia.TileMode.Clamp, null); + const paint = Skia.Paint(); + paint.setImageFilter(filter); + paint.setShader(shader); + surf.getCanvas().drawRect(Skia.XYWHRect(0, 0, w, h), paint); + flush(surf); + disposeAll([paint, filter]); + return surf.makeImageSnapshot() ?? null; + } finally { + surf.dispose(); + } +} + // --- spatial passes: CLARITY and DEHAZE ------------------------------------- // // Both of the scratchpad docs' algorithms (raw_parameter_processing... §3 and @@ -797,13 +832,14 @@ export async function renderPhoto(input: RenderInput): Promise v === (i % 6 === 0 ? 1 : 0)); // 3b. Tone shader. - // The base layer the ramp is drawn through reads a fraction of the FRAME, so - // the preview and the file look at the same neighbourhood — this is the one - // place that knows how big the frame is (TONE_BASE_RADIUS, see toneShader.ts). - const tone = getToneUniforms(adjustments, recipe.baseFilter, [ - width * TONE_BASE_RADIUS, - height * TONE_BASE_RADIUS, - ]); + // TONE_BASE_RADIUS of the FRAME is the neighbourhood the ramp is drawn + // through, so the preview and the file look at the same size of one — this is + // the one place that knows how big the frame is. The base itself is a blurred + // copy of the tone shader's own child (blurredBase), not a ring of samples + // inside it: see TONE_BASE_RADIUS for why a ring was the mottle bug. A frame + // too small or a surface that will not come up leaves the sharp child as the + // base, which lands the base on the pixel and keeps the global ramp. + const tone = getToneUniforms(adjustments, recipe.baseFilter); let toneShader: any = null; // 3c. Cinema seasonal grade (cinema → tone → image). const cinema = getCinemaUniforms(recipe.cinema); @@ -844,7 +880,16 @@ export async function renderPhoto(input: RenderInput): Promise