light: the tone base is a real blur, not nine point samples of one — the ring aliased the luma and the ramp painted the alias back as mottle
The base layer shipped last commit was nine POINT SAMPLES of the child, one ring out at TONE_BASE_RADIUS. A ring is not an average, and on a frame with texture at the ring's own scale it is worse than one: the nine lumas differ, the sample pattern beats against the texture, and the base field comes out aliased. The gain the pixel then rides, o(base)/base, is a function of the base with a kink at every knot — so each alias of the base becomes an alias of the gain, and the reconstruction paints it straight back over the detail it was supposed to leave standing. Reported on a waterfall: grey patches loose on a mountainside, a smear across the face of the falls, and plateaus in cloud and sky (the pass runs whole-frame, so its base is read in the bright end too). Measured, 1160x774 at SHADOW +100, the high-frequency part of the gain field (9px high-pass, where a real base can hold none): 0.0627 for the ring, 0.0048 for a blur of the same radius. Thirteen times. The fix is not more taps — a 7x7 at a third of the step is still point samples, only smaller — it is to stop sampling: the caller blurs. So the base is now a second CHILD of the tone pass, the frame blurred by Skia's own MakeBlur (blurredBase in exportEngine.ts, the draw drawBlurred already made for the sharpening pass) at TONE_BASE_RADIUS of the frame width and sigma TONE_BASE_SIGMA of that radius — a box's equivalent at the radius, so the neighbourhood is the one the radius always named and the cuts are smooth instead of hard. The shader reads it ONCE per pixel and baseLuma loses its loop and its `bx`. That is also the cheaper pass: nine child evals walked the exposure/matrix chain nine times, one eval does not. A caller with no frame to blur hands in the image it is already shading as the base. The tap then lands exactly on t and the ramp is the global move again — a mask's degenerate bx of zero, spelled as a child that IS the source, which is what gradientMask.ts's own call (base == t) already meant. Checks: tone-base-check.mjs is new — SkSL is only compiled at runtime and nothing here compiled TONE_SKSL whole, so the pass is compiled and rendered for real, with the frame and the base held at two flat values a little apart: the pixel has to land on the value the ramp over THAT base predicts (171 for a 128 pixel over a 76 base), and a base equal to the pixel has to be the identity. highlight-knee-check now pins the one tap, the absence of `bx`, the blur, and the two-child wiring. 9/9 pass, build clean. Skipped: no guided filter proper — the base is a plain blur, so a strong edge is no longer held out of it the way the range weight held it (a dark rock a blur's width from the water reads a lifted base and keeps its own darkness). Add when a frame shows the halo; the doc asks for a plain blur and this is one.
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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`,
|
||||
);
|
||||
Reference in New Issue
Block a user