From b4e55b47c11cd456ed4ae04a75837d19f72bf8ac Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sat, 22 Aug 2026 19:01:18 +0200 Subject: [PATCH 1/6] WIP: noise reduction Checkpoint committed by the coordinator, not by the authoring agent: the session hit its API limit mid-task and left this work uncommitted. Committed so it survives, NOT because it is finished - expect failing tests and half-applied changes. The agent resumes from here. --- core/dr-gpu/tests/noise_reduction.rs | 471 ++++++++++ core/dr-pipeline/ops/README.md | 4 +- core/dr-pipeline/ops/noise_reduction.yaml | 30 + core/dr-pipeline/src/ops/mod.rs | 8 + core/dr-pipeline/src/ops/noise_reduction.rs | 912 ++++++++++++++++++++ ui/dr-ui/src/develop.rs | 49 +- 6 files changed, 1469 insertions(+), 5 deletions(-) create mode 100644 core/dr-gpu/tests/noise_reduction.rs create mode 100644 core/dr-pipeline/ops/noise_reduction.yaml create mode 100644 core/dr-pipeline/src/ops/noise_reduction.rs diff --git a/core/dr-gpu/tests/noise_reduction.rs b/core/dr-gpu/tests/noise_reduction.rs new file mode 100644 index 0000000..92ba008 --- /dev/null +++ b/core/dr-gpu/tests/noise_reduction.rs @@ -0,0 +1,471 @@ +//! Noise reduction, end to end on a real device. +//! +//! `dr-pipeline`'s own tests assert what the operation *composes* — how many +//! passes, what radius, in what unit. None of them can tell whether the WGSL +//! compiles, whether the filter actually preserves an edge, or whether the +//! luminance and chroma halves stay out of each other's way once real floats +//! run through them. Those are questions only a GPU answers. +//! +//! # Reading the expected values +//! +//! Sources are uploaded through `DemosaicedImage::from_rgba8`, which flags +//! them non-linear, so the generated shader decodes sRGB before any operation +//! runs and the detail stage sees linear values. The last detail pass +//! re-encodes. So every assertion here decodes the readback back to linear +//! before comparing — comparing 8-bit code values directly would fold the +//! transfer function's varying slope into every tolerance. +//! +//! Almost everything is measured **against a baseline render of the same +//! image with the amount at zero**, rather than against an absolute +//! expectation. That is deliberate: it isolates what noise reduction did from +//! everything else the pipeline does to a pixel, and it stays correct if a +//! later change to the chain moves the values this stage is handed. + +use dr_gpu::{AdjustPass, DemosaicedImage, GpuContext}; +use dr_pipeline::detail::RenderScale; +use dr_pipeline::ops::noise_reduction::{CHROMA, ID, LUMINANCE}; +use dr_pipeline::{Affects, EditGraph}; +use dr_types::ColourSpace; + +fn ctx() -> Option { + // CI runners and headless machines may have no usable adapter. Skip rather + // than fail, exactly as the rest of this crate's device tests do. + match pollster::block_on(GpuContext::new_headless()) { + Ok(c) => Some(c), + Err(e) => { + eprintln!("skipping: no GPU adapter ({e})"); + None + } + } +} + +fn graph_with(luminance: f32, chroma: f32) -> EditGraph { + let mut graph = EditGraph::default_chain(); + graph.set_param(ID, LUMINANCE, luminance); + graph.set_param(ID, CHROMA, chroma); + graph +} + +/// Render one graph and read the pixels back, at the scale the graph itself +/// works out — which is what a frontend does. +fn render( + ctx: &GpuContext, + pass: &mut AdjustPass, + graph: &EditGraph, + source: &DemosaicedImage, + out: (u32, u32), +) -> Vec { + let scale = graph.render_scale(source.size(), out); + render_at(ctx, pass, graph, source, out, scale) +} + +/// Render with an explicitly chosen [`RenderScale`]. +/// +/// Split out for one test only — the one that needs to compose the detail +/// stage at the *wrong* scale on purpose, to show that the conversion from +/// source pixels to render pixels is load-bearing rather than decorative. +fn render_at( + _ctx: &GpuContext, + pass: &mut AdjustPass, + graph: &EditGraph, + source: &DemosaicedImage, + out: (u32, u32), + scale: RenderScale, +) -> Vec { + let shader = graph.compose_for(ColourSpace::Srgb); + let detail = graph.compose_detail_for(scale, ColourSpace::Srgb); + let key = graph.invalidation().through(Affects::Colour); + pass.render_detailed(source, &shader, out.0, out.1, None, &detail, key) + .expect("render"); + pass.export_pixels().expect("readback").0 +} + +fn srgb_decode(byte: u8) -> f32 { + let e = byte as f32 / 255.0; + if e <= 0.040_45 { + e / 12.92 + } else { + ((e + 0.055) / 1.055).powf(2.4) + } +} + +fn luminance(c: [f32; 3]) -> f32 { + 0.2126 * c[0] + 0.7152 * c[1] + 0.0722 * c[2] +} + +/// One pixel of a readback, as linear RGB. +fn linear(pixels: &[u8], width: u32, x: u32, y: u32) -> [f32; 3] { + let i = ((y * width + x) * 4) as usize; + [ + srgb_decode(pixels[i]), + srgb_decode(pixels[i + 1]), + srgb_decode(pixels[i + 2]), + ] +} + +/// A pixel split the way the operation itself splits it: a luminance, and a +/// colour difference whose own luminance is zero. +fn split(pixels: &[u8], width: u32, x: u32, y: u32) -> (f32, [f32; 3]) { + let c = linear(pixels, width, x, y); + let y0 = luminance(c); + (y0, [c[0] - y0, c[1] - y0, c[2] - y0]) +} + +/// Upload an image built from a per-pixel closure. +fn upload( + ctx: &GpuContext, + size: u32, + f: impl Fn(u32, u32) -> [u8; 3], +) -> DemosaicedImage { + let data: Vec = (0..size * size) + .flat_map(|i| { + let (x, y) = (i % size, i / size); + let [r, g, b] = f(x, y); + [r, g, b, 255] + }) + .collect(); + DemosaicedImage::from_rgba8(ctx, &data, size, size).expect("upload") +} + +/// A vertical step edge of a chosen height, centred on the frame. +fn step_edge(ctx: &GpuContext, size: u32, low: u8, high: u8) -> DemosaicedImage { + upload(ctx, size, move |x, _| { + let v = if x < size / 2 { low } else { high }; + [v, v, v] + }) +} + +#[test] +fn a_difference_below_the_threshold_is_averaged_and_one_above_it_is_not() { + // The defining property, and the reason this is a bilateral rather than a + // Gaussian. Both images are step edges and the filter is identical; the + // only thing that differs is how tall the step is relative to the noise + // threshold. A Gaussian would soften both by exactly the same amount, and + // that indiscriminate softening is what "denoised" pictures look like. + let Some(ctx) = ctx() else { return }; + const SIZE: u32 = 64; + let mid = SIZE / 2; + let row = SIZE / 2; + + let measure = |source: &DemosaicedImage| -> f32 { + let mut off = AdjustPass::new(&ctx); + let plain = render(&ctx, &mut off, &graph_with(0.0, 0.0), source, (SIZE, SIZE)); + let mut on = AdjustPass::new(&ctx); + let denoised = render(&ctx, &mut on, &graph_with(100.0, 0.0), source, (SIZE, SIZE)); + // How far the pixel just inside the bright side moved, in linear + // luminance. An averaging filter pulls it down towards the dark half; + // an edge-preserving one leaves it where it was. + let (before, _) = split(&plain, SIZE, mid, row); + let (after, _) = split(&denoised, SIZE, mid, row); + before - after + }; + + // A step of eight code values around mid-grey is about 0.028 in linear + // luminance, against a threshold of roughly 0.034 at full amount: within + // the range where the filter is meant to treat a difference as noise. + let quiet = measure(&step_edge(&ctx, SIZE, 120, 128)); + assert!( + quiet > 0.004, + "a difference below the threshold was left alone: moved {quiet}" + ); + + // Black to white is thirty times the threshold. It has to survive intact + // — an edge that softens here is a halo in every high-contrast picture. + let loud = measure(&step_edge(&ctx, SIZE, 0, 255)); + assert!( + loud.abs() < 0.004, + "an edge far above the threshold was smoothed: moved {loud}" + ); + assert!( + quiet > loud.abs() * 3.0, + "the filter did not distinguish noise from an edge: {quiet} vs {loud}" + ); +} + +/// A fine chroma pattern on a constant grey: colour that alternates every +/// `half_period` pixels with the lightness very nearly fixed. +/// +/// This is what chroma noise looks like to the filter — a colour difference +/// with almost no luminance difference under it — and it is the one pattern +/// that can tell the two halves of this operation apart. +fn chroma_pattern(ctx: &GpuContext, size: u32, half_period: u32, swing: i32) -> DemosaicedImage { + upload(ctx, size, move |x, _| { + let on = (x / half_period) % 2 == 0; + let d = if on { swing } else { -swing }; + [(128 + d) as u8, 128, (128 - d) as u8] + }) +} + +/// How much of a known alternating pattern survived, as the correlation of one +/// linear channel of the middle row against the pattern's own sign. +/// +/// A matched filter rather than a peak-to-peak reading. The readback is eight +/// bits, and the modulation these tests work with is only a handful of code +/// values — deliberately, because a larger one would read as a real colour +/// boundary and the filter would refuse to touch it. Correlating over a whole +/// number of periods averages the quantisation down instead of letting it set +/// the noise floor of the measurement. +/// +/// `margin` covers a whole number of periods too, so the window is unbiased by +/// the row's mean, and it keeps the measurement clear of the borders where a +/// clamped kernel legitimately behaves differently. +fn modulation(pixels: &[u8], width: u32, half_period: u32, channel: usize) -> f32 { + let row = width / 2; + let margin = half_period * 4; + let mut total = 0.0; + let mut count = 0.0; + for x in margin..(width - margin) { + let sample = match channel { + usize::MAX => luminance(linear(pixels, width, x, row)), + c => linear(pixels, width, x, row)[c], + }; + let sign = if (x / half_period) % 2 == 0 { 1.0 } else { -1.0 }; + total += sample * sign; + count += 1.0; + } + total / count +} + +/// Correlate against luminance rather than a channel. +const AS_LUMINANCE: usize = usize::MAX; + +#[test] +fn chroma_noise_reduction_never_moves_lightness() { + // Half of the claim the two-slider design rests on. The split is into a + // luminance and a colour difference whose own luminance is zero, so the + // chroma passes reconstruct with the lightness this pixel arrived with — + // exactly, not approximately. + let Some(ctx) = ctx() else { return }; + const SIZE: u32 = 64; + + // The strongest available form of the assertion, on an image that has no + // colour to filter: every colour difference is zero, so the filter is the + // identity and the output must be the *same bytes*. A reconstruction that + // used a filtered luminance instead of this pixel's own would soften the + // step and show up here immediately. + let grey = step_edge(&ctx, SIZE, 90, 110); + let mut off = AdjustPass::new(&ctx); + let plain = render(&ctx, &mut off, &graph_with(0.0, 0.0), &grey, (SIZE, SIZE)); + let mut on = AdjustPass::new(&ctx); + let denoised = render(&ctx, &mut on, &graph_with(0.0, 100.0), &grey, (SIZE, SIZE)); + assert_eq!( + plain, denoised, + "chroma noise reduction altered an image with no colour in it" + ); + + // And on an image that does have colour to filter, where the two halves + // could actually interfere: the colour modulation must fall while the + // luminance modulation under it stays where it was. The tolerance is wide + // because both readings pass through an eight-bit readback twice over; the + // failure it guards against is not a drift of a few percent but a + // collapse, which is what a leak between the two components would be. + let source = chroma_pattern(&ctx, SIZE, 4, 12); + let mut off = AdjustPass::new(&ctx); + let plain = render(&ctx, &mut off, &graph_with(0.0, 0.0), &source, (SIZE, SIZE)); + let mut on = AdjustPass::new(&ctx); + let chroma = render(&ctx, &mut on, &graph_with(0.0, 100.0), &source, (SIZE, SIZE)); + + let colour_before = modulation(&plain, SIZE, 4, 0); + let colour_after = modulation(&chroma, SIZE, 4, 0); + assert!( + colour_after < colour_before * 0.7, + "chroma noise reduction did not reduce the colour swing: \ + {colour_after} of {colour_before}" + ); + + let light_before = modulation(&plain, SIZE, 4, AS_LUMINANCE); + let light_after = modulation(&chroma, SIZE, 4, AS_LUMINANCE); + assert!( + (light_after - light_before).abs() < light_before.abs() * 0.3, + "chroma noise reduction moved lightness: {light_after} was {light_before}" + ); +} + +#[test] +fn luminance_noise_reduction_never_moves_colour() { + // The other half. The luminance pass adds the *change* in lightness back + // to the colour it was given, so the colour difference passes through + // untouched however hard the luminance is filtered. Written the obvious + // way instead — filtering the three channels and calling it a luminance + // filter — the colour would desaturate as the amount rose, and the chroma + // slider would stop meaning anything on its own. + let Some(ctx) = ctx() else { return }; + const SIZE: u32 = 64; + let source = chroma_pattern(&ctx, SIZE, 4, 12); + + let mut off = AdjustPass::new(&ctx); + let plain = render(&ctx, &mut off, &graph_with(0.0, 0.0), &source, (SIZE, SIZE)); + let mut on = AdjustPass::new(&ctx); + let luma = render(&ctx, &mut on, &graph_with(100.0, 0.0), &source, (SIZE, SIZE)); + + // The colour difference — not the raw channel, which follows lightness. + let row = SIZE / 2; + let interior = 16..(SIZE - 16); + let mut worst = 0.0f32; + for x in interior { + let (_, before) = split(&plain, SIZE, x, row); + let (_, after) = split(&luma, SIZE, x, row); + worst = worst.max((after[0] - before[0]).abs()); + } + // A code value at this brightness, doubled for the two readbacks the + // comparison passes through. The leak this guards against would be a + // sizeable fraction of the pattern's own 0.037 swing, not a rounding. + let quantum = srgb_decode(129) - srgb_decode(128); + assert!( + worst < quantum * 3.0, + "luminance noise reduction moved colour by {worst} \ + (one code value is {quantum})" + ); +} + +#[test] +fn the_same_edit_denoises_the_same_at_two_resolutions() { + // TRACES: FR-DSP-1 — the thing this operation is most likely to get wrong. + // + // A radius is stored in *source* pixels and converted to render pixels at + // every render, because noise is made by photosites. Get that conversion + // wrong and the develop view and the exported file are different + // photographs: tune the slider on a half-size proxy and the export is + // denoised at half the strength, or twice it. + // + // The subject is a chroma square wave with a period that is a power of two + // and aligned to the frame, so halving the render resolution decimates it + // exactly — the proxy sees the same pattern at half the period, with no + // resampling of its own to confuse the measurement. + let Some(ctx) = ctx() else { return }; + const SOURCE: u32 = 256; + const HALF_PERIOD: u32 = 8; // in source pixels + // Six code values of swing. Small on purpose: the colour difference has to + // land near the filter's threshold, because a larger one is a colour + // boundary and the whole point of a bilateral is that it refuses to cross + // those. There would be nothing to measure at either resolution. + let source = chroma_pattern(&ctx, SOURCE, HALF_PERIOD, 6); + + // Sixty percent is an eight-source-pixel radius, which halves to exactly + // four render pixels on a half-size proxy — so the rounding to an integer + // kernel is not what this test is measuring. + let graph = graph_with(0.0, 60.0); + + let mut export_pass = AdjustPass::new(&ctx); + let export = render(&ctx, &mut export_pass, &graph, &source, (SOURCE, SOURCE)); + let export_amp = modulation(&export, SOURCE, HALF_PERIOD, 0); + + let proxy_size = SOURCE / 2; + let mut proxy_pass = AdjustPass::new(&ctx); + let proxy = render(&ctx, &mut proxy_pass, &graph, &source, (proxy_size, proxy_size)); + let proxy_amp = modulation(&proxy, proxy_size, HALF_PERIOD / 2, 0); + + // Both must be doing something: two flat images would agree perfectly and + // prove nothing. + let untouched = { + let mut pass = AdjustPass::new(&ctx); + let plain = render(&ctx, &mut pass, &graph_with(0.0, 0.0), &source, (SOURCE, SOURCE)); + modulation(&plain, SOURCE, HALF_PERIOD, 0) + }; + assert!( + export_amp < untouched * 0.8, + "the denoiser did nothing: {export_amp} of {untouched}" + ); + + assert!( + (proxy_amp - export_amp).abs() < export_amp * 0.2, + "the same edit left {proxy_amp} of the pattern on the proxy and \ + {export_amp} on the export" + ); + + // The control, and the reason the tolerance above means something. Compose + // the detail stage as though the proxy were a full-resolution render — + // which is exactly the bug of storing a radius in render pixels — and the + // kernel is twice as wide in source terms. If the conversion were not + // load-bearing, this would land in the same place as the other two. + let mut wrong_pass = AdjustPass::new(&ctx); + let wrong = render_at( + &ctx, + &mut wrong_pass, + &graph, + &source, + (proxy_size, proxy_size), + RenderScale::full((proxy_size, proxy_size)), + ); + let wrong_amp = modulation(&wrong, proxy_size, HALF_PERIOD / 2, 0); + assert!( + wrong_amp < export_amp * 0.7, + "an unconverted radius was indistinguishable from a converted one: \ + {wrong_amp} against {export_amp}" + ); +} + +#[test] +fn each_amount_costs_only_the_dispatches_it_needs() { + // The cost story, which is invisible in the picture and therefore has to + // be asserted on a counter. Luminance is one exact two-dimensional pass; + // chroma is two, because at its radius the exact form is quadratic and + // unaffordable. An edit using neither must pay for neither — and must + // produce pixels identical to a chain that has no denoiser in it at all. + let Some(ctx) = ctx() else { return }; + const SIZE: u32 = 48; + let source = step_edge(&ctx, SIZE, 40, 200); + + for (luminance, chroma, expected) in [(60.0, 0.0, 1), (0.0, 60.0, 2), (60.0, 60.0, 3)] { + let mut pass = AdjustPass::new(&ctx); + render(&ctx, &mut pass, &graph_with(luminance, chroma), &source, (SIZE, SIZE)); + assert_eq!( + pass.detail_dispatches(), + expected, + "luminance {luminance}, chroma {chroma}" + ); + assert_eq!(pass.colour_dispatches(), 1); + } + + let mut neutral = AdjustPass::new(&ctx); + let a = render(&ctx, &mut neutral, &graph_with(0.0, 0.0), &source, (SIZE, SIZE)); + assert_eq!(neutral.detail_dispatches(), 0); + assert_eq!(neutral.detail_allocations(), 0, "nothing was allocated"); + + // Byte-identical, not merely close: an operation at its defaults must not + // touch the image, and a stage that ran and wrote back the same values + // would still have quantised twice. + let mut absent = AdjustPass::new(&ctx); + let b = render(&ctx, &mut absent, &EditGraph::default_chain(), &source, (SIZE, SIZE)); + assert_eq!(a, b, "a neutral denoiser changed the picture"); +} + +#[test] +fn dragging_either_slider_recompiles_nothing_and_reallocates_nothing() { + // TRACES: FR-DEV-3d. Both of these are ruinous per frame and invisible in + // the output, which is why they need a counter rather than an eye. A + // radius rides in a uniform buffer, so moving a slider re-runs the detail + // dispatches against the pipelines already compiled — and does not re-run + // the colour pass at all, since nothing it depends on moved. + let Some(ctx) = ctx() else { return }; + const SIZE: u32 = 48; + let source = step_edge(&ctx, SIZE, 40, 200); + let mut pass = AdjustPass::new(&ctx); + + let mut graph = graph_with(50.0, 50.0); + render(&ctx, &mut pass, &graph, &source, (SIZE, SIZE)); + let pipelines = pass.cached_detail_pipelines(); + let allocations = pass.detail_allocations(); + assert_eq!(pipelines, 3, "one per pass: luminance, then two for chroma"); + + for amount in [55.0, 60.0, 65.0, 70.0] { + graph.set_param(ID, LUMINANCE, amount); + graph.set_param(ID, CHROMA, amount); + render(&ctx, &mut pass, &graph, &source, (SIZE, SIZE)); + } + assert_eq!( + pass.cached_detail_pipelines(), + pipelines, + "an amount is a uniform, not a shader" + ); + assert_eq!( + pass.detail_allocations(), + allocations, + "a steady viewport must allocate nothing" + ); + assert_eq!( + pass.colour_dispatches(), + 1, + "the fused colour pass re-ran for a change it does not depend on" + ); +} diff --git a/core/dr-pipeline/ops/README.md b/core/dr-pipeline/ops/README.md index dfb8ec8..8128817 100644 --- a/core/dr-pipeline/ops/README.md +++ b/core/dr-pipeline/ops/README.md @@ -211,7 +211,9 @@ half in Rust would be worse than either alone. Currently hand-written: `tone_curve` (a curve widget over five interpolated points), `colour_mixer` (thirty-six faceted parameters from twelve computed hue -bands). `vignetting` is hand-written too but is not in the develop chain — it +bands), and `noise_reduction` (a kernel, and one that decides how many +dispatches to emit at each resolution — see the next section). +`vignetting` is hand-written too but is not in the develop chain — it carries lens-profile coefficients that are not parameters. `distortion` and `aberration` are `Warp`s rather than operations: they rewrite coordinates before sampling rather than transforming a colour after it. diff --git a/core/dr-pipeline/ops/noise_reduction.yaml b/core/dr-pipeline/ops/noise_reduction.yaml new file mode 100644 index 0000000..20722f0 --- /dev/null +++ b/core/dr-pipeline/ops/noise_reduction.yaml @@ -0,0 +1,30 @@ +# A hand-written node. `rust:` names the type in `crate::ops` that implements +# `Operation`; its descriptor, its parameters and its passes come from that +# type rather than from this file. +# +# It appears here anyway so that `ops/` lists the whole pipeline in order — +# including the neighbourhood operations, which run as a group after every +# point operation but are still ordered among themselves. +id: noise_reduction +order: 110 + +rust: NoiseReduction + +why_rust: | + A kernel, not four facts. The declarative schema hands a fragment a colour + and no coordinate, which is precisely what a denoiser cannot work with — it + is defined by what the neighbouring pixels are doing. It is therefore a + `DetailStage` (see `../src/detail.rs`), which means deciding at every render + how many dispatches to emit, converting a radius stated in *source* pixels + into the render pixels this frame is being drawn at, and declaring the halo + the tile scheduler needs. None of that is expressible as a uniform + expression, and stretching the schema to cover it would produce a worse + language than Rust aimed at one caller. + +placement: | + First among the detail operations, because denoising is a repair and + everything else in this stage is an enhancement: sharpening or adding + clarity to a noisy frame amplifies the grain along with the detail, and no + later pass can separate them again. Its position relative to the point + operations is not this number's to decide — the whole detail stage runs + after the fused pass, in linear light, before the output transform. diff --git a/core/dr-pipeline/src/ops/mod.rs b/core/dr-pipeline/src/ops/mod.rs index 34362cb..2edb12f 100644 --- a/core/dr-pipeline/src/ops/mod.rs +++ b/core/dr-pipeline/src/ops/mod.rs @@ -26,6 +26,12 @@ //! parameters at all. A schema stretched to cover those would be a worse //! language than Rust, aimed at one caller each. //! +//! [`noise_reduction`] is an exception for a different reason again: it is a +//! *kernel*, and a declared node's `wgsl:` is handed a colour with no way back +//! to a coordinate. It runs in [`crate::detail`] instead, deciding at each +//! render how many dispatches to emit and converting a radius stated in sensor +//! pixels into the render pixels this frame is being drawn at. +//! //! Both publish the same [`crate::descriptor::OpDescriptor`], so nothing //! downstream can tell them apart. A hand-written node still declares its //! place in the chain in `ops/.yaml` with `rust:`, so the directory @@ -44,12 +50,14 @@ pub mod aberration; pub mod colour_mixer; pub mod curve; pub mod distortion; +pub mod noise_reduction; pub mod vignetting; pub use aberration::Aberration; pub use colour_mixer::ColourMixer; pub use curve::ToneCurve; pub use distortion::Distortion; +pub use noise_reduction::NoiseReduction; pub use vignetting::Vignetting; // The declared nodes, plus `helpers` and `chain`. Generated into OUT_DIR by diff --git a/core/dr-pipeline/src/ops/noise_reduction.rs b/core/dr-pipeline/src/ops/noise_reduction.rs new file mode 100644 index 0000000..c26c4ca --- /dev/null +++ b/core/dr-pipeline/src/ops/noise_reduction.rs @@ -0,0 +1,912 @@ +//! TRACES: FR-DEV-3 +//! Noise reduction — luminance and chroma, as two independent amounts. +//! +//! # Why two controls and not one +//! +//! Sensor noise arrives as two quite different faults, and a photographer +//! treats them differently because they cost different things to remove. +//! +//! **Luminance noise** is fine, high-frequency grain in lightness. It sits at +//! the same spatial frequency as real detail — eyelashes, fabric weave, tree +//! bark — so the eye cannot be given more smoothing without also being given +//! less texture. The radius that helps is one or two photosites, and past +//! about three the picture stops looking like a photograph and starts looking +//! like a painting. Many photographers deliberately leave some. +//! +//! **Chroma noise** is coarse, blotchy and low-frequency: magenta and green +//! patches tens of pixels across, produced by the demosaic interpolating +//! between colour-filtered sites that disagree. Nothing in a photograph looks +//! like it, so it can be smoothed hard — and it has to be, because a radius +//! of two pixels does not touch a blotch of twenty. Human spatial acuity for +//! colour is roughly a quarter of that for lightness, which is why a +//! chroma-only blur that would be obvious in luminance is invisible here, and +//! is the same fact JPEG chroma subsampling has exploited since 1992. +//! +//! So the radius that is *correct* differs between the two by roughly an +//! order of magnitude. That is the reason these are two amounts rather than +//! one: a single slider would either under-treat the colour blotches or +//! destroy the detail, and there is no setting at which it does neither. +//! +//! # The split, and why it makes the two amounts genuinely independent +//! +//! Each pass decomposes the linear sRGB colour into a luminance and a colour +//! difference: +//! +//! ```text +//! y = luminance(c) Rec. 709 weights, exact in this space +//! d = c - vec3(y) luminance(d) == 0, by construction +//! c = vec3(y) + d exactly, up to floating-point rounding +//! ``` +//! +//! `d` carries no lightness at all: the weights sum to one, so subtracting a +//! grey of the same luminance leaves a vector whose own luminance is zero. +//! The luminance pass therefore replaces `y` and returns `d` untouched, and +//! the chroma passes replace `d` and return `y` untouched. Neither can leak +//! into the other, which is what lets a photographer set the two sliders +//! independently and get what they say rather than their product. +//! +//! This is also why the split is taken *here* rather than in the fused pass: +//! the detail stage runs after the camera matrix, where the working space is +//! linear sRGB and a Rec. 709 luminance is a luminance rather than a weighted +//! sum of whatever the colour filter array's dyes happened to pass. +//! +//! # The filter: a bilateral, in two different arrangements +//! +//! A plain Gaussian is not an option. Denoising is exactly the problem of +//! averaging pixels that differ only by noise while refusing to average +//! pixels that differ because the scene does, and a Gaussian cannot tell the +//! difference — it removes grain and edges in the same proportion, which is +//! the smeared look that makes noise reduction recognisable at a glance. +//! +//! A **bilateral filter** multiplies the spatial weight by a *range* weight +//! that falls off with how different the neighbour's value is, so a neighbour +//! across an edge contributes almost nothing and the edge survives the +//! average that removes the grain either side of it. It is the conventional +//! edge-preserving choice; it needs neither a guide image nor the per-window +//! statistics a guided filter accumulates, and it is one expression per tap — +//! which matters, because this runs on the frame path (ARCH §6.1). +//! +//! It is also, in its exact form, quadratic: a radius *r* costs `(2r+1)²` +//! taps. That is affordable at the luminance radius and ruinous at the chroma +//! radius, and the two are arranged differently in consequence: +//! +//! | | radius (source px) | arrangement | taps / pixel | dispatches | +//! |---|---|---|---|---| +//! | luminance | 1.0 … 2.5 | exact 2-D bilateral | 9 … 49 | 1 | +//! | chroma | 2.0 … 12.0 | separable bilateral | 10 … 50 | 2 | +//! +//! The worst case with both at full strength is **99 taps per pixel across +//! three dispatches**, against 49 + 625 = 674 for the exact 2-D form of both. +//! The chroma pass is where all of that saving is. +//! +//! **The luminance pass is exact rather than separable** because at these +//! radii the separable form is not actually cheaper in the way that matters: +//! r = 2 is 25 taps in one dispatch against 20 taps in two, and the second +//! dispatch costs a full-frame `rgba16float` write and read that the taps +//! saved do not pay for. It is also the higher-quality answer, with none of +//! the axis-aligned streaking the approximation can show. +//! +//! **The chroma pass is separable** — a 1-D bilateral along x, then along y, +//! the approximation Pham and van Vliet published in 2005. It is an +//! approximation and not an identity: a bilateral's range weights make the +//! two-dimensional kernel non-separable in principle, and the residual shows +//! as faint axis-aligned structure along strong diagonal edges. That is +//! acceptable here for the same reason the large radius is acceptable — it is +//! in chroma, where the eye's spatial acuity is four times lower — and the +//! alternative, 625 taps a pixel at 4K, is roughly five gigataps a frame and +//! not a frame path at all. Where the approximation *would* be visible, in +//! luminance, it is not used. +//! +//! # The threshold, and what it is a fraction of +//! +//! A bilateral needs to know how large a difference counts as noise. A single +//! absolute number in linear light cannot say: linear light puts middle grey +//! at 0.18, so a threshold tuned for a highlight is roughly a hundred times +//! too coarse for a shadow and would flatten it completely. +//! +//! The threshold is therefore proportional to the square root of the signal: +//! +//! ```text +//! sigma(y) = k * sqrt(max(y, 0) + NOISE_FLOOR) +//! ``` +//! +//! which is the photon-noise law — the arrival of light is Poisson, so its +//! variance equals its mean and its standard deviation goes as the square +//! root. `NOISE_FLOOR` stands in for the sensor's read noise, which does not +//! vanish at black, and keeps `sigma` finite there instead of collapsing to +//! zero and switching the filter off exactly where noise is worst. +//! +//! **The honest limitation.** By the time this stage runs, exposure, the tone +//! curve and the recovery controls have already moved these values, so they +//! are no longer proportional to photon counts and the law is an +//! approximation rather than a measurement. It is kept because it is a far +//! better approximation than a constant — the tone mapping is monotone and +//! only gently compressive, so the ordering and the rough scaling survive it +//! — and because the thing that *would* be exact is FR-DEV-3g's learned +//! denoiser, operating in the raw domain where the noise model still holds. +//! This is the conventional path that degrades to when no model is present, +//! and it is not trying to be it. +//! +//! # Radius units +//! +//! Both radii are stated in **source pixels** and converted through +//! [`RenderScale::source_pixels`] at every render. Noise is a property of the +//! sensor and of the demosaic: its grain is about one photosite across +//! because photosites are what recorded it, and that stays true regardless of +//! how large the frame is drawn on screen or how many megapixels the body +//! has. [`RenderScale::frame_fraction`], the other unit, would say the +//! opposite — that grain covers a fixed proportion of the *picture* — so the +//! same body's files would need different settings as their pixel count +//! changed, and a crop would need different settings from the frame it came +//! out of. +//! +//! The consequence is the one [`crate::detail`] documents: on a heavy proxy a +//! luminance radius of one source pixel is a fraction of a render pixel, the +//! information it would act on was thrown away by the downscale, and this +//! operation emits no luminance pass at all rather than drawing a plausible +//! lie. The chroma radius, ten times larger, still resolves — which is also +//! true of the fault it treats, since a blotch twenty pixels across survives +//! being halved. + +use crate::descriptor::{ + Attribute, LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Scale, Unit, +}; +use crate::detail::{DetailPass, DetailStage, RenderScale}; +use crate::operation::{Affects, Helper, Operation, Uniform}; + +pub const ID: OpId = OpId("noise_reduction"); +pub const LUMINANCE: ParamId = ParamId("luminance"); +pub const CHROMA: ParamId = ParamId("chroma"); + +static DESCRIPTOR: OpDescriptor = OpDescriptor { + id: ID, + label: LocalizedKey("op.noise_reduction"), + attributes: &[Attribute::Detail], + // Zero to a hundred rather than the symmetric `amount` shape the tonal + // controls use. There is no meaningful negative: "minus fifty noise + // reduction" would be adding grain, which is a look rather than a repair + // and belongs to a different operation carrying `Attribute::Effect`. A + // control whose left half does nothing is worse than one that stops. + params: &[ + ParamDescriptor::scalar( + "luminance", + "param.noise_reduction.luminance", + 0.0, + 100.0, + 0.0, + Unit::None, + Scale::Linear, + 0, + ), + ParamDescriptor::scalar( + "chroma", + "param.noise_reduction.chroma", + 0.0, + 100.0, + 0.0, + Unit::None, + Scale::Linear, + 0, + ), + ], +}; + +/// The luminance radius at the lowest and the highest amount, in **source** +/// pixels. +/// +/// It starts at one rather than at zero because a kernel smaller than a pixel +/// is not a kernel; the amount fades the *threshold* in from zero instead, so +/// the control is still continuous at its neutral. It stops at 2.5 because +/// past roughly three source pixels a luminance average stops removing grain +/// and starts removing the subject — the point at which every editor's +/// luminance slider gets described as watercolour. +const LUMA_RADIUS: (f32, f32) = (1.0, 2.5); + +/// The chroma radius at the lowest and the highest amount, in **source** +/// pixels. +/// +/// An order of magnitude larger, because the fault is an order of magnitude +/// larger: demosaic-born colour blotches are tens of pixels across and a +/// two-pixel average does not see them. +const CHROMA_RADIUS: (f32, f32) = (2.0, 12.0); + +/// Hard ceilings on the kernel actually dispatched, in **render** pixels. +/// +/// Necessary because [`RenderScale::ratio`] exceeds one when the view is +/// zoomed past 1:1 — the render target keeps its size while the region it +/// covers shrinks — so a radius in source pixels can ask for an arbitrarily +/// large kernel at high magnification. Without a cap, zooming to 800% would +/// quietly turn a twelve-pixel chroma radius into a ninety-six-pixel one and +/// cost sixty-four times the taps, at exactly the moment the user is +/// inspecting the result closely and most wants the view to stay responsive. +/// Clamping instead means the effect stops growing past the point where more +/// of it would be visible anyway. +const LUMA_KERNEL_CAP: u32 = 3; +const CHROMA_KERNEL_CAP: u32 = 16; + +/// The luminance range threshold at full amount, as a coefficient on +/// `sqrt(signal)`. +/// +/// At middle grey this is `0.075 * sqrt(0.18) ≈ 0.032`, about three percent +/// of full scale — roughly eight 8-bit code values, which is the grain of a +/// high-ISO frame. Much larger and it would start treating real texture as +/// noise. +const LUMA_SIGMA: f32 = 0.075; + +/// The chroma range threshold at full amount, on the length of the colour +/// difference vector. +/// +/// Far larger than the luminance threshold because it is allowed to be: a +/// genuine colour boundary separates colours by much more than this — a +/// saturated red sits about 0.84 from grey — while chroma noise is a few +/// hundredths. That gap is the whole reason chroma can be filtered hard +/// without visible bleeding. +const CHROMA_SIGMA: f32 = 0.20; + +/// How tightly the chroma passes are steered by luminance, at the lowest and +/// the highest amount. +/// +/// The chroma filter weights a neighbour by *both* how far its colour is and +/// how far its lightness is. The lightness term is a cross-bilateral guide in +/// the ordinary sense — the cleaner channel steering the noisier one — and it +/// is what stops colour crossing a boundary the colour channel itself cannot +/// see: a dark object against a light background of the same hue. +/// +/// It does not start at zero. A guide with a zero threshold rejects every +/// neighbour, and the filter would do nothing however far the colour +/// threshold was opened. It widens with the amount because a photographer +/// asking for more chroma denoising has a noisier frame, whose luminance — +/// the guide itself — is also noisier and would otherwise break the weights. +const CHROMA_GUIDE_SIGMA: (f32, f32) = (0.06, 0.16); + +/// The read-noise floor, in linear working units. +/// +/// Keeps `sigma` finite at black. Roughly 1/400 of full scale, about where a +/// deep shadow sits after a normal rendering: small enough not to affect a +/// midtone, large enough that the filter does not switch itself off in the +/// shadows. +const NOISE_FLOOR: f32 = 0.0025; + +/// TRACES: FR-DEV-3 +/// Luminance and chroma noise reduction, as two independent amounts. +#[derive(Debug, Clone, Copy, Default)] +pub struct NoiseReduction { + luminance: f32, + chroma: f32, +} + +impl NoiseReduction { + pub fn new() -> Self { + Self::default() + } + + /// Both amounts at once, for tests and for a preset applying the pair. + pub fn with_amounts(luminance: f32, chroma: f32) -> Self { + Self { luminance, chroma } + } + + /// The luminance radius in **source** pixels, or `None` at neutral. + pub fn luminance_radius(&self) -> Option { + (self.luminance > 0.0).then(|| lerp(LUMA_RADIUS, self.luminance / 100.0)) + } + + /// The chroma radius in **source** pixels, or `None` at neutral. + pub fn chroma_radius(&self) -> Option { + (self.chroma > 0.0).then(|| lerp(CHROMA_RADIUS, self.chroma / 100.0)) + } + + /// The luminance kernel this render would dispatch, in render pixels. + /// + /// Zero means "not at this resolution": either the control is neutral, or + /// the radius is smaller than a render pixel and the detail it would act + /// on is not present in this render at all (see [`RenderScale::resolves`]). + /// + /// Exposed so a test — and, in time, an interface offering to zoom to 1:1 + /// — can state the expected kernel without repeating the rounding rule, + /// which is how a test comes to agree with a bug. + pub fn luminance_kernel(&self, scale: RenderScale) -> u32 { + kernel(self.luminance_radius(), scale, LUMA_KERNEL_CAP) + } + + /// The chroma kernel this render would dispatch, in render pixels. + pub fn chroma_kernel(&self, scale: RenderScale) -> u32 { + kernel(self.chroma_radius(), scale, CHROMA_KERNEL_CAP) + } + + /// The luminance range threshold coefficient — the `k` in + /// `sigma = k * sqrt(signal + floor)`. + fn luma_sigma(&self) -> f32 { + LUMA_SIGMA * (self.luminance / 100.0) + } + + /// The chroma passes' two thresholds: the luminance guide, then the + /// colour difference. + fn chroma_sigmas(&self) -> (f32, f32) { + let t = self.chroma / 100.0; + (lerp(CHROMA_GUIDE_SIGMA, t), CHROMA_SIGMA * t) + } +} + +fn lerp((low, high): (f32, f32), t: f32) -> f32 { + low + (high - low) * t.clamp(0.0, 1.0) +} + +/// A radius in source pixels, as the kernel to walk in render pixels. +/// +/// Zero when [`RenderScale::resolves`] says the radius does not survive this +/// render. That check rather than the rounding, because the two disagree +/// exactly where it matters: 0.6 of a render pixel *rounds* to one, and a +/// one-pixel kernel would then be dispatched to remove grain that the +/// downscale averaged away before this stage ran. It would cost a dispatch to +/// draw something that is not in the picture, and — worse — it would look +/// like an effect, so a photographer would tune against it. +/// +/// Otherwise rounded rather than truncated, so a 1.4-pixel radius is one pixel +/// and a 1.6-pixel radius is two; and capped, so that zooming past 1:1 cannot +/// make the cost of a frame grow without bound. +fn kernel(radius: Option, scale: RenderScale, cap: u32) -> u32 { + let Some(radius) = radius else { return 0 }; + if !scale.resolves(radius) { + return 0; + } + (scale.source_pixels(radius).round().max(1.0) as u32).min(cap) +} + +/// The Gaussian spatial falloff for a kernel of this radius, as the +/// `1 / (2 * sigma^2)` the shader multiplies a squared distance by. +/// +/// `sigma` is half the radius, which puts the weight at the rim of the kernel +/// at `exp(-2)`, about 0.135 — small enough that the kernel has no visible +/// hard edge, large enough that the outermost taps are still doing work +/// rather than being paid for and discarded. +fn inv_spatial(kernel: u32) -> f32 { + let sigma = (kernel as f32 * 0.5).max(0.5); + 1.0 / (2.0 * sigma * sigma) +} + +impl Operation for NoiseReduction { + fn descriptor(&self) -> &'static OpDescriptor { + &DESCRIPTOR + } + + fn set_param(&mut self, id: ParamId, value: f32) { + match id { + LUMINANCE => self.luminance = value, + CHROMA => self.chroma = value, + _ => log::warn!("noise_reduction: unknown parameter {id}"), + } + } + + fn param(&self, id: ParamId) -> f32 { + match id { + LUMINANCE => self.luminance, + CHROMA => self.chroma, + _ => 0.0, + } + } + + fn is_active(&self) -> bool { + self.luminance > 0.0 || self.chroma > 0.0 + } + + /// Never called: a detail operation contributes no fused fragment, and + /// `compose_full` filters it out before asking. + fn wgsl_body(&self) -> String { + String::new() + } + + fn uniforms(&self) -> Vec { + Vec::new() + } + + fn affects(&self) -> Affects { + Affects::Detail + } + + fn detail(&self) -> Option<&dyn DetailStage> { + Some(self) + } + + /// The shared `luminance` helper, which both kernels call. + /// + /// Taken from `ops/_helpers.yaml` rather than defined here, so that + /// lightness means one thing across the whole pipeline. Its own + /// documentation calls the Rec. 709 weights an approximation, which they + /// are in camera space — but the detail stage runs after the camera + /// matrix, in linear sRGB, where they are exactly the right weights. + fn helpers(&self) -> &'static [Helper] { + HELPERS + } +} + +static HELPERS: &[Helper] = &[crate::ops::helpers::LUMINANCE]; + +impl DetailStage for NoiseReduction { + fn passes(&self, scale: RenderScale) -> Vec { + let mut passes = Vec::new(); + + // Luminance first, and the order is not arbitrary: the chroma passes + // are steered by luminance, and a luminance that has already been + // denoised is a cleaner guide than a noisy one. Doing it the other way + // round would make the chroma weights noisier for no gain anywhere. + // When the luminance control is neutral the guide is simply the + // luminance as it arrived, which is the honest fallback. + let luma = self.luminance_kernel(scale); + if luma > 0 { + passes.push(DetailPass { + label: "luminance", + radius: luma, + uniforms: vec![ + Uniform { + name: "radius", + value: luma as f32, + }, + Uniform { + name: "inv_spatial", + value: inv_spatial(luma), + }, + Uniform { + name: "sigma_k", + value: self.luma_sigma(), + }, + Uniform { + name: "noise_floor", + value: NOISE_FLOOR, + }, + ], + wgsl: LUMA_WGSL.to_string(), + }); + } + + let chroma = self.chroma_kernel(scale); + if chroma > 0 { + let (guide, colour) = self.chroma_sigmas(); + // One body, dispatched twice with the step vector rotated. Writing + // it as two passes over one kernel rather than as two kernels is + // what keeps the two halves of a separable filter from drifting + // apart — the classic way an axis ends up filtered differently + // from the other and the result acquires a diagonal bias. + for (index, (sx, sy)) in [(1.0, 0.0), (0.0, 1.0)].into_iter().enumerate() { + passes.push(DetailPass { + label: if index == 0 { + "chroma-horizontal" + } else { + "chroma-vertical" + }, + radius: chroma, + uniforms: vec![ + Uniform { + name: "radius", + value: chroma as f32, + }, + Uniform { + name: "step_x", + value: sx, + }, + Uniform { + name: "step_y", + value: sy, + }, + Uniform { + name: "inv_spatial", + value: inv_spatial(chroma), + }, + Uniform { + name: "guide_k", + value: guide, + }, + Uniform { + name: "chroma_k", + value: colour, + }, + Uniform { + name: "noise_floor", + value: NOISE_FLOOR, + }, + ], + wgsl: CHROMA_WGSL.to_string(), + }); + } + } + + passes + } +} + +/// The exact two-dimensional bilateral, acting on luminance alone. +const LUMA_WGSL: &str = "\ +// A bilateral filter over luminance: the spatial Gaussian every blur has, +// multiplied by a range term that falls off with how different the +// neighbour's lightness is. That second factor is the entire difference +// between denoising and smearing — a neighbour on the far side of an edge +// contributes essentially nothing, so the edge survives the average that +// removes the grain either side of it. +// +// Exact rather than separable. At the one-to-three-pixel radii a luminance +// kernel is allowed, the two-pass approximation saves a handful of taps and +// costs a whole extra full-frame write and read, and it can leave axis-aligned +// streaking in the one channel the eye reads sharpest. +let r = i32(radius); +let y0 = luminance(c); + +// Photon noise: the standard deviation of a signal goes as its square root, so +// the threshold has to as well. A constant would be a hundred times too coarse +// in the shadows relative to the highlights and would flatten them. +// `noise_floor` stands for read noise and keeps this finite at black. +let sigma = max(sigma_k * sqrt(max(y0, 0.0) + noise_floor), 1e-5); +let inv_range = 1.0 / (2.0 * sigma * sigma); + +var weight_sum = 0.0; +var luma_sum = 0.0; +for (var dy = -r; dy <= r; dy = dy + 1) { + for (var dx = -r; dx <= r; dx = dx + 1) { + let n = luminance(tap(coord, vec2(dx, dy))); + let dl = n - y0; + let distance2 = f32(dx * dx + dy * dy); + let w = exp(-(distance2 * inv_spatial + dl * dl * inv_range)); + weight_sum = weight_sum + w; + luma_sum = luma_sum + n * w; + } +} + +// Substitute the filtered luminance and leave the colour difference exactly as +// it arrived. `c - vec3(y0)` has zero luminance by construction, so adding the +// change in lightness back changes lightness and nothing else — which is what +// keeps this control independent of the chroma one. +c = c + vec3(luma_sum / weight_sum - y0);"; + +/// One axis of the separable cross-bilateral, acting on chroma alone. +const CHROMA_WGSL: &str = "\ +// One axis of a separable bilateral over the colour difference. +// +// Separable because the radius is an order of magnitude larger than the +// luminance one and the exact form is quadratic: at twelve source pixels that +// is 625 taps a pixel, which is not a frame path. Two one-dimensional passes +// are fifty, and the axis-aligned residual the approximation leaves is in +// chroma, where the eye resolves about a quarter of what it resolves in +// lightness. +// +// The weight has two range terms, not one. The colour term is what the filter +// is for. The luminance term is a *guide*: it stops colour crossing a boundary +// the colour channel itself cannot see — a dark object against a light +// background of the same hue — by letting the cleaner channel steer the +// noisier one. +let r = i32(radius); +let step = vec2(i32(step_x), i32(step_y)); + +let y0 = luminance(c); +let d0 = c - vec3(y0); + +// Both thresholds follow the same square-root-of-signal law, for the reason +// the luminance pass states. +let level = sqrt(max(y0, 0.0) + noise_floor); +let guide = max(guide_k * level, 1e-5); +let colour = max(chroma_k * level, 1e-5); +let inv_guide = 1.0 / (2.0 * guide * guide); +let inv_colour = 1.0 / (2.0 * colour * colour); + +var weight_sum = 0.0; +var chroma_sum = vec3(0.0); +for (var i = -r; i <= r; i = i + 1) { + let n = tap(coord, step * i); + let yn = luminance(n); + let dn = n - vec3(yn); + let dl = yn - y0; + let dc = dn - d0; + let w = exp(-(f32(i * i) * inv_spatial + dl * dl * inv_guide + dot(dc, dc) * inv_colour)); + weight_sum = weight_sum + w; + chroma_sum = chroma_sum + dn * w; +} + +// This pixel's own luminance, unchanged, plus the filtered colour difference. +// Every `dn` has zero luminance, so their weighted mean does too and the +// reconstructed colour keeps exactly the lightness it arrived with. +c = vec3(y0) + chroma_sum / weight_sum;"; + +#[cfg(test)] +mod tests { + use super::*; + use dr_types::ColourSpace; + + /// A 24 MP frame, and the panel a develop view might show it in. + const FULL: (u32, u32) = (6000, 4000); + + fn nr(luminance: f32, chroma: f32) -> NoiseReduction { + NoiseReduction::with_amounts(luminance, chroma) + } + + fn chain_with(op: NoiseReduction) -> Vec> { + vec![Box::new(op)] + } + + fn compose(op: NoiseReduction, scale: RenderScale) -> crate::detail::ComposedDetail { + crate::detail::compose_detail(&chain_with(op), scale, ColourSpace::Srgb) + } + + #[test] + fn neutral_costs_the_edit_nothing() { + // The rule the whole pipeline rests on. An unedited photograph must + // not pay for a denoiser it is not using — no pass, no dispatch, and + // the fused shader ends exactly as it always did. + let op = nr(0.0, 0.0); + assert!(!op.is_active()); + assert!(compose(op, RenderScale::full(FULL)).is_empty()); + } + + #[test] + fn each_amount_reaches_the_shader_on_its_own() { + // The point of two controls: either alone must produce its own passes + // and nothing of the other's. A denoiser that emitted the chroma + // dispatches whenever luminance was on would cost two thirds of the + // stage for an effect the user did not ask for — and would be + // invisible in the picture, because at a zero threshold the chroma + // filter is very nearly the identity. + let scale = RenderScale::full(FULL); + + let labels = |op| { + compose(op, scale) + .passes + .iter() + .map(|p| p.label.clone()) + .collect::>() + }; + + assert_eq!(labels(nr(50.0, 0.0)), ["noise_reduction/luminance"]); + assert_eq!( + labels(nr(0.0, 50.0)), + [ + "noise_reduction/chroma-horizontal", + "noise_reduction/chroma-vertical" + ] + ); + + let both = compose(nr(50.0, 50.0), scale); + assert_eq!(both.len(), 3); + // The luminance pass runs first, so the chroma guide is the denoised + // luminance rather than the raw one. + assert_eq!(both.passes[0].label, "noise_reduction/luminance"); + // And only the last pass in the whole chain performs the output + // transform, whichever pass that happens to be. + assert!(!both.passes[0].writes_output); + assert!(!both.passes[1].writes_output); + assert!(both.passes[2].writes_output); + } + + #[test] + fn chroma_always_reaches_further_than_luminance() { + // The reason these are two controls rather than one. Colour blotches + // are tens of pixels across and grain is one or two, so no single + // radius treats both — and if this ever inverted, the chroma slider + // would have become an expensive second luminance slider. + for amount in [1.0, 25.0, 50.0, 75.0, 100.0] { + let op = nr(amount, amount); + let l = op.luminance_radius().expect("active"); + let c = op.chroma_radius().expect("active"); + assert!(c > l * 2.0, "at {amount}: chroma {c} vs luminance {l}"); + } + } + + #[test] + fn a_radius_is_a_count_of_sensor_pixels_not_a_fraction_of_the_frame() { + // TRACES: FR-DSP-1 — the decision this operation is most likely to + // get wrong, stated as the property that distinguishes the two units. + // + // Noise is made by photosites, so its grain is the same size in + // *source* pixels however the frame is being rendered. Convert with + // `source_pixels` and the kernel in render pixels tracks the scale; + // convert with `frame_fraction` and it would instead be constant for a + // constant render size, which is a different — and wrong — claim. + let op = nr(100.0, 100.0); + let radius = op.chroma_radius().expect("active"); + assert!((radius - 12.0).abs() < 1e-6); + + // The same photograph at three sizes. Measured back in source pixels, + // the kernel is the same length every time. + for render in [(1500u32, 1000u32), (3000, 2000), (6000, 4000)] { + let scale = RenderScale::new(render, FULL); + let in_source_pixels = op.chroma_kernel(scale) as f32 / scale.ratio(); + assert!( + (in_source_pixels - radius).abs() < 0.5, + "{render:?} denoised {in_source_pixels} source pixels, not {radius}" + ); + } + + // And the distinguishing case: one panel, two cameras. A 24 MP file + // and a 96 MP file shown at the same size have the same *frame + // fraction* per render pixel but four times the photosites, so the + // sensor-pixel radius covers a quarter as much of the picture in the + // second. A frame-fraction radius would have given both the same + // kernel, which would mean the 96 MP body needed a different setting + // to remove the same grain. + let render = (1500, 1000); + let small = op.chroma_kernel(RenderScale::new(render, (6000, 4000))); + let large = op.chroma_kernel(RenderScale::new(render, (12000, 8000))); + assert!( + small > large, + "denser sensor, same panel: {small} vs {large} render pixels" + ); + } + + #[test] + fn a_luminance_radius_too_small_to_draw_is_not_drawn() { + // The honest limit `RenderScale` exists to report. At a quarter-size + // proxy a 2.5-source-pixel luminance radius is 0.6 render pixels: the + // grain it would remove was averaged away by the downscale before this + // stage ran, and there is no kernel that represents a fraction of a + // pixel. Emitting a pass anyway would burn a dispatch to draw a guess. + let proxy = RenderScale::new((1500, 1000), FULL); + let op = nr(100.0, 100.0); + assert_eq!(op.luminance_kernel(proxy), 0); + assert!(!proxy.resolves(op.luminance_radius().expect("active"))); + + // Chroma is a different case at the same scale, and the difference is + // real rather than a rounding accident: a blotch twenty pixels across + // is still ten pixels across in a half-size proxy, so it both survives + // the downscale and can still be removed. + assert_eq!(op.chroma_kernel(proxy), 3); + let composed = compose(op, proxy); + assert_eq!(composed.len(), 2, "chroma alone survives a heavy proxy"); + } + + #[test] + fn zooming_past_one_to_one_does_not_let_the_kernel_run_away() { + // A 1:1 view already renders one render pixel per source pixel; at + // 800% there are eight. Without the cap the chroma kernel would be + // ninety-six render pixels — sixty-four times the taps — precisely + // when the user is looking closely and least tolerant of a stall. + let magnified = RenderScale::new((2000, 2000), (250, 250)); + assert!((magnified.ratio() - 8.0).abs() < 1e-6); + let op = nr(100.0, 100.0); + assert_eq!(op.chroma_kernel(magnified), CHROMA_KERNEL_CAP); + assert_eq!(op.luminance_kernel(magnified), LUMA_KERNEL_CAP); + } + + #[test] + fn the_declared_halo_is_the_kernel_the_shader_walks() { + // ARCH §5.3 grows a tile by the declared radius before scheduling it. + // An understated radius shows as a seam at every tile boundary, which + // looks like a driver bug rather than like an arithmetic error — so + // the number handed to the scheduler and the number the loop counts to + // must be the same number, not two that happen to agree today. + let scale = RenderScale::full(FULL); + let op = nr(100.0, 100.0); + for pass in compose(op, scale).passes { + let declared = pass.radius as f32; + let walked = pass.uniforms[crate::detail::DETAIL_BASE_UNIFORM_FIELDS]; + assert_eq!(declared, walked, "{}", pass.label); + } + assert_eq!(compose(op, scale).radius(), op.chroma_kernel(scale)); + } + + #[test] + fn every_pass_declares_a_uniform_block_the_gpu_will_accept() { + // A uniform struct whose size is not a multiple of sixteen is rejected + // outright by the WGSL uniform address space rules, and the failure + // arrives as a compile error against generated source a long way from + // here. + for pass in compose(nr(60.0, 60.0), RenderScale::full(FULL)).passes { + assert_eq!(pass.uniforms.len() % 4, 0, "{}", pass.label); + assert!( + pass.uniforms.iter().all(|v| v.is_finite()), + "{} uploaded a non-finite uniform", + pass.label + ); + } + } + + #[test] + fn the_two_chroma_passes_are_one_kernel_along_two_axes() { + // A separable filter is only separable if both halves are the same + // filter. The bodies must be identical and the step vectors must be + // perpendicular unit steps; anything else is two different blurs whose + // composition is not the two-dimensional one intended. + let passes = nr(0.0, 80.0).passes(RenderScale::full(FULL)); + assert_eq!(passes.len(), 2); + assert_eq!(passes[0].wgsl, passes[1].wgsl); + + let step = |p: &DetailPass| { + let get = |name| { + p.uniforms + .iter() + .find(|u| u.name == name) + .expect("declared") + .value + }; + (get("step_x"), get("step_y")) + }; + assert_eq!(step(&passes[0]), (1.0, 0.0)); + assert_eq!(step(&passes[1]), (0.0, 1.0)); + + // Everything else about the two must match, or one axis is filtered + // harder than the other and a round blotch comes out oval. + assert_eq!(passes[0].radius, passes[1].radius); + for name in ["radius", "inv_spatial", "guide_k", "chroma_k", "noise_floor"] { + let of = |p: &DetailPass| { + p.uniforms + .iter() + .find(|u| u.name == name) + .expect("declared") + .value + }; + assert_eq!(of(&passes[0]), of(&passes[1]), "{name}"); + } + } + + #[test] + fn the_threshold_opens_with_the_amount_and_closes_at_neutral() { + // The amount is a *threshold* as much as a radius: it decides how + // large a difference the filter is willing to call noise. If it did + // not reach zero at the neutral end the control would be + // discontinuous, and the first pixel of travel on the slider would + // visibly flatten the image. + let mut previous = 0.0; + for amount in [1.0, 10.0, 50.0, 100.0] { + let sigma = nr(amount, 0.0).luma_sigma(); + assert!(sigma > previous, "at {amount}: {sigma} <= {previous}"); + previous = sigma; + } + assert_eq!(nr(0.0, 0.0).luma_sigma(), 0.0); + + // The chroma guide is the exception, and deliberately so: a guide with + // a zero threshold rejects every neighbour, so the filter would do + // nothing at all at low amounts however wide the colour threshold was. + let (guide, colour) = nr(0.0, 1.0).chroma_sigmas(); + assert!(guide > 0.0, "a zero guide would reject every neighbour"); + assert!(colour > 0.0); + } + + #[test] + fn a_chroma_threshold_is_far_wider_than_a_luminance_one() { + // Not a tuning detail but the reason the two are separable problems. + // Chroma noise is a few hundredths from grey and a real colour + // boundary is most of the way to a primary, so the gap between them is + // wide enough to filter hard through. Luminance has no such gap, which + // is why its threshold has to stay tight. + let (_, colour) = nr(100.0, 100.0).chroma_sigmas(); + assert!(colour > nr(100.0, 100.0).luma_sigma() * 2.0); + } + + #[test] + fn parameters_round_trip_and_an_unknown_one_is_ignored() { + // What the sidecar, the history and the preset system all rely on. + let mut op = NoiseReduction::new(); + op.set_param(LUMINANCE, 40.0); + op.set_param(CHROMA, 70.0); + assert_eq!(op.param(LUMINANCE), 40.0); + assert_eq!(op.param(CHROMA), 70.0); + op.set_param(ParamId("sharpness"), 99.0); + assert_eq!(op.param(LUMINANCE), 40.0); + assert_eq!(op.param(ParamId("sharpness")), 0.0); + } + + #[test] + fn the_generated_wgsl_addresses_its_own_uniforms() { + // The composer prefixes each uniform with the operation id and the + // pass index, so two operations may both call a uniform `radius` and + // neither has to know. A body that slipped through unrewritten would + // fail to compile against the generated struct. + let composed = compose(nr(50.0, 50.0), RenderScale::full(FULL)); + assert!(composed.passes[0] + .source + .contains("noise_reduction_0_sigma_k: f32,")); + assert!(composed.passes[1] + .source + .contains("noise_reduction_1_chroma_k: f32,")); + assert!(composed.passes[2] + .source + .contains("noise_reduction_2_chroma_k: f32,")); + // The shared luminance helper reaches every pass that calls it. + for pass in &composed.passes { + assert!( + pass.source.contains("fn luminance(c: vec3)"), + "{} calls luminance without defining it", + pass.label + ); + } + // Three passes of one operation are three shaders, and must not share + // a pipeline-cache entry. + let hashes: std::collections::BTreeSet = + composed.passes.iter().map(|p| p.structure_hash).collect(); + assert_eq!(hashes.len(), 3); + } +} diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index 6863a65..5b1a6b7 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -891,11 +891,30 @@ impl DevelopSession { /// The mask array is rasterised in source space at proxy size and sampled /// through the framing map, so one array is correct at every output size: /// a 256px thumbnail and a 24 MP export bind the same texture. + /// + /// **And the detail stage with it.** The neighbourhood operations — noise + /// reduction, and the rest of FR-DEV-3's kernels — cannot be fused into + /// the single dispatch, so an edit using one composes a fused pass that + /// hands on *linear* values and a chain of passes that finishes the job + /// (see `dr_pipeline::detail`). Those two halves must be composed from one + /// graph and dispatched together, or the fused shader's storage format + /// does not match the texture bound to it; going through + /// `render_detailed` here is what makes that true of every path at once. + /// It falls through to the plain render when the chain is empty, which is + /// almost every edit, so this costs nothing to the frames that do not + /// need it. + /// + /// `space` has to be the space `shader` was composed for. It is the last + /// pass of the detail chain that performs the output transform when there + /// is one, so the two would otherwise be free to disagree about which + /// primaries the file is in — and the result would be a correctly + /// labelled file with the wrong colours in it (FR-EXP-2). fn render_with_masks( &mut self, shader: &dr_pipeline::operation::ComposedShader, w: u32, h: u32, + space: dr_types::ColourSpace, ) -> Result<(), String> { let ctx = self.ctx.clone(); self.ensure_subject_fields(&ctx); @@ -905,8 +924,30 @@ impl DevelopSession { .then(|| self.masks.as_ref().and_then(|p| p.array())) .flatten(); + // The scale a kernel's radius is converted through. Worked out from + // the framing, so a crop and a zoom are already accounted for: what + // matters to a sensor-sized radius is how many source pixels one + // render pixel stands for, here and now (FR-DSP-1). + let scale = self.graph.render_scale(self.demosaiced.size(), (w, h)); + let detail = self.graph.compose_detail_for(scale, space); + // Detail passes read what the colour pass wrote, so the key they are + // cached against is the colour key: moving a sharpening slider re-runs + // this stage and not the fused one (FR-DEV-3d). + let colour_key = self + .graph + .invalidation() + .through(dr_pipeline::Affects::Colour); + self.adjust - .render_masked(&self.demosaiced, shader, w, h, masks) + .render_detailed( + &self.demosaiced, + shader, + w, + h, + masks, + &detail, + colour_key, + ) .map(|_| ()) .map_err(|e| e.to_string()) } @@ -1605,7 +1646,7 @@ impl DevelopSession { // Rasterise the masks first: the shader addresses array slices by // index, so the array has to describe *this* stack before it is bound. - self.render_with_masks(&shader, w, h)?; + self.render_with_masks(&shader, w, h, dr_types::ColourSpace::Srgb)?; let texture = self.adjust.output().ok_or("nothing was rendered")?; // The import is fallible on format and usage only, and both are fixed @@ -1729,7 +1770,7 @@ impl DevelopSession { let (w, h) = self.graph.output_size(sw, sh); let shader = self.graph.compose_for(space); - self.render_with_masks(&shader, w, h)?; + self.render_with_masks(&shader, w, h, space)?; let (pixels, rw, rh) = self.adjust.export_pixels().map_err(|e| e.to_string())?; dr_export::Frame::in_space(rw, rh, pixels, space).map_err(|e| e.to_string()) @@ -1756,7 +1797,7 @@ impl DevelopSession { let (w, h) = fit(fw, fh, edge.max(1), edge.max(1)); let shader = self.graph.compose_for(dr_types::ColourSpace::Srgb); - self.render_with_masks(&shader, w, h)?; + self.render_with_masks(&shader, w, h, dr_types::ColourSpace::Srgb)?; let (pixels, rw, rh) = self.adjust.export_pixels().map_err(|e| e.to_string())?; Ok((rw, rh, pixels)) From df3fe660e582bb07ecc877ca1c028cd9176704d1 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sat, 22 Aug 2026 19:14:53 +0200 Subject: [PATCH 2/6] Finish the render when a kernel is too small to draw An active detail operation may emit no pass at a given render scale - the honest answer for a sensor-sized radius on a heavy proxy. The fused composer cannot see that, having no resolution to consult, so it had already stopped short of the output transform and the frame died on a storage-format mismatch. Compose a bodyless resolve pass in that case so the output transform still happens exactly once. --- core/dr-pipeline/src/detail.rs | 93 ++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/core/dr-pipeline/src/detail.rs b/core/dr-pipeline/src/detail.rs index cccc909..6226a8d 100644 --- a/core/dr-pipeline/src/detail.rs +++ b/core/dr-pipeline/src/detail.rs @@ -449,6 +449,48 @@ pub fn compose_detail( } } + // An active detail operation that emitted nothing at this scale. + // + // Legal, and the honest answer for an acutance operation on a heavy proxy + // — a one-source-pixel radius is a third of a render pixel there and no + // kernel represents a third of a pixel (see [`RenderScale`]). But it opens + // a hole between the two halves of the composition: [`compose_full`] + // decides to hand on linear working values from the *operations*, which it + // must, having no scale to consult, so the fused pass has already stopped + // short of the output transform. Returning an empty chain here would leave + // that transform undone and bind an `rgba16float` shader to an + // `rgba8unorm` target, which surfaces as a wgpu validation failure a long + // way from the cause. + // + // So the chain is never empty when the fused pass is expecting one: a + // single pass with no body, which reads the intermediate and performs the + // output transform the fused pass skipped. One dispatch, in the uncommon + // case where a photographer has a kernel switched on at a scale that + // cannot draw it — against the alternative of the preview failing outright + // or `compose_full` growing a resolution argument it has no other use for. + if planned.is_empty() + && ops + .iter() + .any(|o| o.is_active() && o.detail().is_some()) + { + return ComposedDetail { + passes: vec![compose_one( + RESOLVE_ID, + &[], + &DetailPass { + label: "resolve", + radius: 0, + wgsl: String::new(), + uniforms: Vec::new(), + }, + 0, + scale, + output, + true, + )], + }; + } + let last = planned.len().saturating_sub(1); let passes = planned .into_iter() @@ -461,6 +503,14 @@ pub fn compose_detail( ComposedDetail { passes } } +/// The operation id the resolve pass is labelled with. +/// +/// Not an operation: no `ops/*.yaml` declares it and nothing in the chain +/// answers to it. It exists so the generated label reads `detail/resolve` +/// rather than borrowing the id of whichever operation happened to fall +/// through, which would send a reader looking for a bug in that operation. +const RESOLVE_ID: &str = "detail"; + #[allow(clippy::too_many_arguments)] fn compose_one( id: &str, @@ -880,6 +930,49 @@ mod tests { } } + #[test] + fn an_active_operation_that_draws_nothing_still_finishes_the_render() { + // The seam between the two composers, and the one case where they + // cannot see each other. `compose_full` decides to hand on linear + // working values from the *operations* — it has no resolution to + // consult — while this composer converts a radius and can legitimately + // decide there is nothing to draw at this size. An empty chain would + // then leave the output transform undone: the fused pass writes + // `rgba16float` and the frontend binds an `rgba8unorm` target to it. + // + // A photographer meets this by turning on capture sharpening or + // luminance noise reduction while the develop view is fitted to a + // large file, which is the normal way to work, so it is not an edge + // case that can be left to fail. + let ops = with_blur(0.001); + let scale = RenderScale::full((400, 400)); + assert!(ops.last().expect("the blur").is_active()); + assert_eq!( + BoxBlur::with_radius(0.001).passes(scale).len(), + 0, + "the premise: a radius too small to draw emits no pass" + ); + + let composed = compose_detail(&ops, scale, dr_types::ColourSpace::Srgb); + assert_eq!(composed.len(), 1, "the chain must not be empty here"); + assert_eq!(composed.radius(), 0, "it reads only the pixel it writes"); + + let resolve = &composed.passes[0]; + assert_eq!(resolve.label, "detail/resolve"); + assert!(resolve.writes_output); + assert!(resolve.source.contains("texture_storage_2d Date: Sat, 22 Aug 2026 19:16:43 +0200 Subject: [PATCH 3/6] Measure the chroma tests on colour difference, not on red A colour square wave built from equal, opposite swings of red and blue is not a pure colour pattern: Rec. 709 weights them 0.2126 and 0.0722, so it carries a luminance square wave of about a seventh of the swing underneath. Correlating the raw red channel therefore reads a constant floor that the chroma filter is not meant to remove, which compressed every ratio towards one - enough that the resolution test could no longer tell a correct kernel from one twice the size. Correlate the colour difference instead, and write the derivation of each expected value into the test. --- core/dr-gpu/tests/noise_reduction.rs | 146 ++++++++++++++++++++------- 1 file changed, 112 insertions(+), 34 deletions(-) diff --git a/core/dr-gpu/tests/noise_reduction.rs b/core/dr-gpu/tests/noise_reduction.rs index 92ba008..8ebf2f3 100644 --- a/core/dr-gpu/tests/noise_reduction.rs +++ b/core/dr-gpu/tests/noise_reduction.rs @@ -161,16 +161,28 @@ fn a_difference_below_the_threshold_is_averaged_and_one_above_it_is_not() { }; // A step of eight code values around mid-grey is about 0.028 in linear - // luminance, against a threshold of roughly 0.034 at full amount: within + // luminance, against a threshold of roughly 0.035 at full amount: within // the range where the filter is meant to treat a difference as noise. + // + // Worked out on paper, since it cannot be run here: at amount 100 the + // kernel is 3 render pixels and sigma_k is 0.075, so at y = 0.2159 the + // range sigma is 0.075·sqrt(0.2159 + 0.0025) = 0.0350 and a neighbour + // 0.0280 away is weighted exp(-0.320) = 0.726. Summing the 7×7 kernel's + // spatial weights over the four bright columns and the three dark ones + // gives a filtered luminance of 0.2076 against 0.2159 — a move of 0.0082, + // which survives the 8-bit readback as about **0.0072**. The threshold + // below is set well under that rather than at it: what would be a bug is + // the filter declining to average at all. let quiet = measure(&step_edge(&ctx, SIZE, 120, 128)); assert!( quiet > 0.004, "a difference below the threshold was left alone: moved {quiet}" ); - // Black to white is thirty times the threshold. It has to survive intact - // — an edge that softens here is a halo in every high-contrast picture. + // Black to white is thirteen times the threshold — 1.0 against a sigma of + // 0.075·sqrt(1.0025) = 0.0751 — so a neighbour across it is weighted + // exp(-88), which is zero in any arithmetic. The edge has to survive + // intact; one that softens here is a halo in every high-contrast picture. let loud = measure(&step_edge(&ctx, SIZE, 0, 255)); assert!( loud.abs() < 0.004, @@ -182,12 +194,18 @@ fn a_difference_below_the_threshold_is_averaged_and_one_above_it_is_not() { ); } -/// A fine chroma pattern on a constant grey: colour that alternates every -/// `half_period` pixels with the lightness very nearly fixed. +/// A fine chroma pattern: red and blue swung in opposite directions by the +/// same number of code values, green held. /// /// This is what chroma noise looks like to the filter — a colour difference -/// with almost no luminance difference under it — and it is the one pattern -/// that can tell the two halves of this operation apart. +/// alternating over a few pixels — and it is the one pattern that can tell the +/// two halves of this operation apart. +/// +/// It is not a *pure* colour pattern, and the tests must not assume it is. +/// Rec. 709 weights red at 0.2126 and blue at 0.0722, so swinging one up and +/// the other down by equal amounts moves lightness by about a seventh of the +/// swing. That residue is real, it is not the chroma filter's to remove, and +/// [`chroma_r`] is what keeps it out of the measurements. fn chroma_pattern(ctx: &GpuContext, size: u32, half_period: u32, swing: i32) -> DemosaicedImage { upload(ctx, size, move |x, _| { let on = (x / half_period) % 2 == 0; @@ -196,8 +214,8 @@ fn chroma_pattern(ctx: &GpuContext, size: u32, half_period: u32, swing: i32) -> }) } -/// How much of a known alternating pattern survived, as the correlation of one -/// linear channel of the middle row against the pattern's own sign. +/// How much of a known alternating pattern survived, as the correlation of a +/// chosen measurement of the middle row against the pattern's own sign. /// /// A matched filter rather than a peak-to-peak reading. The readback is eight /// bits, and the modulation these tests work with is only a handful of code @@ -209,25 +227,39 @@ fn chroma_pattern(ctx: &GpuContext, size: u32, half_period: u32, swing: i32) -> /// `margin` covers a whole number of periods too, so the window is unbiased by /// the row's mean, and it keeps the measurement clear of the borders where a /// clamped kernel legitimately behaves differently. -fn modulation(pixels: &[u8], width: u32, half_period: u32, channel: usize) -> f32 { +/// +/// `sample` is what to measure. Passing [`chroma_r`] rather than the raw red +/// channel matters more than it looks: a colour square wave built from 8-bit +/// code values carries a *luminance* square wave under it — Rec. 709 does not +/// weight red and blue equally, so swinging one up and the other down moves +/// lightness too — and that component is not the chroma filter's to remove. +/// Left in the measurement it is a constant floor under every reading, which +/// compresses every ratio this file asserts towards one and would leave the +/// tests unable to tell a correct kernel from one twice the size. +fn modulation( + pixels: &[u8], + width: u32, + half_period: u32, + sample: impl Fn([f32; 3]) -> f32, +) -> f32 { let row = width / 2; let margin = half_period * 4; let mut total = 0.0; let mut count = 0.0; for x in margin..(width - margin) { - let sample = match channel { - usize::MAX => luminance(linear(pixels, width, x, row)), - c => linear(pixels, width, x, row)[c], - }; + let value = sample(linear(pixels, width, x, row)); let sign = if (x / half_period) % 2 == 0 { 1.0 } else { -1.0 }; - total += sample * sign; + total += value * sign; count += 1.0; } total / count } -/// Correlate against luminance rather than a channel. -const AS_LUMINANCE: usize = usize::MAX; +/// The red component of the colour difference — red with its lightness taken +/// out, which is the quantity the chroma passes actually filter. +fn chroma_r(c: [f32; 3]) -> f32 { + c[0] - luminance(c) +} #[test] fn chroma_noise_reduction_never_moves_lightness() { @@ -255,26 +287,36 @@ fn chroma_noise_reduction_never_moves_lightness() { // And on an image that does have colour to filter, where the two halves // could actually interfere: the colour modulation must fall while the - // luminance modulation under it stays where it was. The tolerance is wide - // because both readings pass through an eight-bit readback twice over; the - // failure it guards against is not a drift of a few percent but a - // collapse, which is what a leak between the two components would be. + // luminance modulation under it stays where it was. + // + // On paper, at amount 100 over a half-period of 4: the kernel is 12 render + // pixels, both chroma thresholds are 0.16·sqrt(y + floor) ≈ 0.076 and + // 0.20·… ≈ 0.095, so an opposite-coloured neighbour is weighted 0.410, and + // summing the separable kernel over the eight phases leaves about **0.42** + // of the chroma amplitude — 0.0158 of 0.0377. The luminance amplitude must + // not move at all: every colour difference the pass averages has zero + // luminance by construction, so their weighted mean does too. + // + // Both tolerances are wide of those numbers because both readings pass + // through an eight-bit readback twice over; the failure they guard against + // is not a drift of a few percent but a collapse, which is what a leak + // between the two components would be. let source = chroma_pattern(&ctx, SIZE, 4, 12); let mut off = AdjustPass::new(&ctx); let plain = render(&ctx, &mut off, &graph_with(0.0, 0.0), &source, (SIZE, SIZE)); let mut on = AdjustPass::new(&ctx); let chroma = render(&ctx, &mut on, &graph_with(0.0, 100.0), &source, (SIZE, SIZE)); - let colour_before = modulation(&plain, SIZE, 4, 0); - let colour_after = modulation(&chroma, SIZE, 4, 0); + let colour_before = modulation(&plain, SIZE, 4, chroma_r); + let colour_after = modulation(&chroma, SIZE, 4, chroma_r); assert!( colour_after < colour_before * 0.7, "chroma noise reduction did not reduce the colour swing: \ {colour_after} of {colour_before}" ); - let light_before = modulation(&plain, SIZE, 4, AS_LUMINANCE); - let light_after = modulation(&chroma, SIZE, 4, AS_LUMINANCE); + let light_before = modulation(&plain, SIZE, 4, luminance); + let light_after = modulation(&chroma, SIZE, 4, luminance); assert!( (light_after - light_before).abs() < light_before.abs() * 0.3, "chroma noise reduction moved lightness: {light_after} was {light_before}" @@ -330,8 +372,44 @@ fn the_same_edit_denoises_the_same_at_two_resolutions() { // // The subject is a chroma square wave with a period that is a power of two // and aligned to the frame, so halving the render resolution decimates it - // exactly — the proxy sees the same pattern at half the period, with no - // resampling of its own to confuse the measurement. + // exactly. The fused pass loads the nearest source pixel when the framing + // is unrotated, so output column `x` reads source column `2x` — always the + // same half of an eight-wide block as `2x + 1` — and the proxy sees the + // same two colours at half the period, with no resampling of its own to + // confuse the measurement. + // + // # The expected numbers + // + // Worked out on paper, because the tolerances below are meaningless + // without knowing what they are tolerances *around*. + // + // The two colours are (134, 128, 122) and (122, 128, 134), which decode to + // linear (0.2384, 0.2159, 0.1946) and its mirror. Their colour differences + // are ±(0.0193, -0.0033, -0.0245), so the pattern's chroma amplitude — + // what `modulation` with `chroma_r` reads — is 0.0188 before filtering. + // + // At amount 60 both chroma thresholds are 0.12·sqrt(y + floor) ≈ 0.0565, + // so a neighbour of the opposite colour is weighted + // `exp(-(dl²/2σ_g² + |dc|²/2σ_c²)) ≈ exp(-0.624) ≈ 0.536`: attenuated, but + // far from rejected, which is the regime where the kernel's *width* is + // what decides the answer. That is the point — a test where the range + // weights dominated would pass whatever the radius conversion did. + // + // Summing the separable kernel's spatial weights over the eight phases of + // the pattern gives a mean surviving fraction of **0.521** at export (a + // radius of 8 render pixels over a half-period of 8) and **0.521** on the + // proxy (4 over 4) — the two arrangements are the same filter sampled at + // two rates, and they agree to three decimal places. So both amplitudes + // land at about 0.0098. + // + // The control lands at **0.305**, about 0.0057: a kernel of 8 render + // pixels over a half-period of 4 is twice as wide in the terms that + // matter. The tolerances are set wide of those numbers rather than tight + // to them, because the readback is eight bits and each of the handful of + // distinct output values carries up to half a code value of rounding — a + // floor of a few percent on any of these readings that no amount of + // averaging over a larger frame removes, since the error is periodic + // rather than random. let Some(ctx) = ctx() else { return }; const SOURCE: u32 = 256; const HALF_PERIOD: u32 = 8; // in source pixels @@ -348,19 +426,19 @@ fn the_same_edit_denoises_the_same_at_two_resolutions() { let mut export_pass = AdjustPass::new(&ctx); let export = render(&ctx, &mut export_pass, &graph, &source, (SOURCE, SOURCE)); - let export_amp = modulation(&export, SOURCE, HALF_PERIOD, 0); + let export_amp = modulation(&export, SOURCE, HALF_PERIOD, chroma_r); let proxy_size = SOURCE / 2; let mut proxy_pass = AdjustPass::new(&ctx); let proxy = render(&ctx, &mut proxy_pass, &graph, &source, (proxy_size, proxy_size)); - let proxy_amp = modulation(&proxy, proxy_size, HALF_PERIOD / 2, 0); + let proxy_amp = modulation(&proxy, proxy_size, HALF_PERIOD / 2, chroma_r); // Both must be doing something: two flat images would agree perfectly and - // prove nothing. + // prove nothing. About 0.0098 of 0.0188, by the derivation above. let untouched = { let mut pass = AdjustPass::new(&ctx); let plain = render(&ctx, &mut pass, &graph_with(0.0, 0.0), &source, (SOURCE, SOURCE)); - modulation(&plain, SOURCE, HALF_PERIOD, 0) + modulation(&plain, SOURCE, HALF_PERIOD, chroma_r) }; assert!( export_amp < untouched * 0.8, @@ -368,7 +446,7 @@ fn the_same_edit_denoises_the_same_at_two_resolutions() { ); assert!( - (proxy_amp - export_amp).abs() < export_amp * 0.2, + (proxy_amp - export_amp).abs() < export_amp * 0.25, "the same edit left {proxy_amp} of the pattern on the proxy and \ {export_amp} on the export" ); @@ -387,9 +465,9 @@ fn the_same_edit_denoises_the_same_at_two_resolutions() { (proxy_size, proxy_size), RenderScale::full((proxy_size, proxy_size)), ); - let wrong_amp = modulation(&wrong, proxy_size, HALF_PERIOD / 2, 0); + let wrong_amp = modulation(&wrong, proxy_size, HALF_PERIOD / 2, chroma_r); assert!( - wrong_amp < export_amp * 0.7, + wrong_amp < export_amp * 0.8, "an unconverted radius was indistinguishable from a converted one: \ {wrong_amp} against {export_amp}" ); From eb229051ef8f12ec22a2736fb383018f26f299f6 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sat, 22 Aug 2026 19:20:22 +0200 Subject: [PATCH 4/6] Teach the whole-chain GPU tests about neighbourhood operations Both tests composed only the fused half and rendered it through the plain path. That was correct while every operation was a point operation; with a kernel in the chain the fused pass stops short of the output transform, so the render was rejected and the operation-block count was one too high. Compose both halves and dispatch them together, and assert that each operation reaches exactly one of the two stages rather than counting blocks - so the next kernel added extends the coverage instead of breaking it. --- core/dr-gpu/src/adjust.rs | 85 ++++++++++++++++++++++++++++++++------- 1 file changed, 70 insertions(+), 15 deletions(-) diff --git a/core/dr-gpu/src/adjust.rs b/core/dr-gpu/src/adjust.rs index df7c9d1..a47e85d 100644 --- a/core/dr-gpu/src/adjust.rs +++ b/core/dr-gpu/src/adjust.rs @@ -1063,12 +1063,31 @@ mod tests { let mut g = EditGraph::default_chain(); g.set_param(cap.id, p.id, value); let shader = g.compose(); - pass.render(&img, &shader, 16, 16).unwrap_or_else(|e| { - panic!( - "{}.{} at {value} generated invalid WGSL:\n{e}", - cap.id, p.id - ) - }); + + // A neighbourhood operation compiles as a *chain*, not as + // a fragment: it contributes nothing to the fused pass, + // and the fused pass in turn stops short of the output + // transform so the last detail pass can perform it. Going + // through `render_detailed` covers both kinds with one + // loop, which is the property that makes this test extend + // itself when an operation is added — the whole reason it + // is derived from the chain rather than hand-written. + // + // Composed at the size actually being rendered, because a + // radius stated in source pixels can decide there is + // nothing to draw at sixteen pixels (FR-DSP-1); the chain + // still carries the resolve pass that finishes the render, + // and that generated source is worth compiling too. + let scale = g.render_scale(img.size(), (16, 16)); + let detail = g.compose_detail(scale); + let key = g.invalidation().through(dr_pipeline::Affects::Colour); + pass.render_detailed(&img, &shader, 16, 16, None, &detail, key) + .unwrap_or_else(|e| { + panic!( + "{}.{} at {value} generated invalid WGSL:\n{e}", + cap.id, p.id + ) + }); } } } @@ -1422,24 +1441,60 @@ mod tests { } } + // Cropped, so the render is against an output size that is not the + // source size — the case where a wrong dispatch or a wrong texture + // allocation would show up. + let (w, h) = g.output_size(32, 32); let shader = g.compose(); + let scale = g.render_scale(img.size(), (w, h)); + let detail = g.compose_detail(scale); + + // Every operation has to reach the pipeline, but they do not all reach + // the same half of it, and which half is not this test's business to + // know: a point operation is a block in the fused shader, and a + // neighbourhood operation is one or more passes of the detail chain + // (`dr_pipeline::detail`) and contributes *no* fused block, because a + // fused fragment is handed a colour with no way back to a coordinate. + // + // Asserted as an exclusive or over the chain rather than as a count, + // so that adding either kind of operation extends this test on its own + // — and so that an operation which somehow managed both, or neither, + // is named rather than showing up as an arithmetic mismatch. + let mut fused_blocks = 0; + for desc in g.descriptors() { + let id = desc.id.0; + let point = shader.source.contains(&format!("---- {id} ----")); + let neighbourhood = detail + .passes + .iter() + .any(|p| p.label.starts_with(&format!("{id}/"))); + assert!( + point ^ neighbourhood, + "{id} reaches {} of the two stages; every active operation \ + belongs to exactly one", + if point { "both" } else { "neither" } + ); + fused_blocks += usize::from(point); + } + assert_eq!( shader.source.matches("---- ").count(), - // Every operation, plus framing — which emits a stage of its own - // rather than an operation block, and is not in `descriptors`. - g.descriptors().len() + 1, - "every operation and the framing should be active" + // The fused operations, plus framing — which emits a stage of its + // own rather than an operation block, and is not in `descriptors`. + fused_blocks + 1, + "the fused shader carries a block nothing in the chain asked for" ); assert!( shader.source.contains("---- framing ----"), "framing must reach the shader alongside the colour operations" ); + assert!( + !detail.is_empty(), + "with every operation active the detail stage must run" + ); - // Cropped, so the render is against an output size that is not the - // source size — the case where a wrong dispatch or a wrong texture - // allocation would show up. - let (w, h) = g.output_size(32, 32); - pass.render(&img, &shader, w, h) + let key = g.invalidation().through(dr_pipeline::Affects::Colour); + pass.render_detailed(&img, &shader, w, h, None, &detail, key) .expect("the full chain must compile"); } From 690e76a51fdca3b636fd0326dd8890fc850b6765 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sat, 22 Aug 2026 19:21:59 +0200 Subject: [PATCH 5/6] Let the fully-active chain test account for both stages Activating every operation now activates a kernel too, and a kernel emits no block in the fused shader. Assert that each operation reaches exactly one of the fused pass and the detail chain, rather than counting fused blocks against the length of the chain. --- core/dr-pipeline/src/lib.rs | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/core/dr-pipeline/src/lib.rs b/core/dr-pipeline/src/lib.rs index a763ef9..a651c3a 100644 --- a/core/dr-pipeline/src/lib.rs +++ b/core/dr-pipeline/src/lib.rs @@ -107,12 +107,38 @@ mod tests { let g = fully_active(); assert!(!g.is_neutral()); let shader = g.compose(); - // Counted against the chain rather than a literal, so adding an - // operation does not require editing this test. + + // The chain has two kinds of operation in it and they arrive in + // different places: a point operation is a block in the fused shader, + // while a neighbourhood operation is a pass of the detail chain and + // contributes no fused block at all — it reads pixels it is not + // writing, and a fused fragment is handed a colour with no coordinate. + // + // So the assertion is that each operation reaches exactly one of the + // two, checked against the chain rather than a literal, and phrased so + // that adding either kind extends it without an edit here. + let scale = g.render_scale((4000, 3000), (4000, 3000)); + let detail = g.compose_detail(scale); + let mut fused_blocks = 0; + for desc in g.descriptors() { + let id = desc.id.0; + let point = shader.source.contains(&format!("---- {id} ----")); + let neighbourhood = detail + .passes + .iter() + .any(|p| p.label.starts_with(&format!("{id}/"))); + assert!( + point ^ neighbourhood, + "{id} reaches {} of the two stages; an active operation \ + belongs to exactly one", + if point { "both" } else { "neither" } + ); + fused_blocks += usize::from(point); + } assert_eq!( shader.source.matches("---- ").count(), - g.descriptors().len(), - "every operation in the chain should appear" + fused_blocks, + "the fused shader carries a block nothing in the chain asked for" ); } From 7031352e85d7ca44fa957c897e1a29d98ca97cde Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sat, 22 Aug 2026 19:23:43 +0200 Subject: [PATCH 6/6] Keep neighbourhood operations out of a mask layer's chain A layer holds a full chain and fuses it into the colour dispatch, so the panel - which names no operation - would have offered a noise reduction slider inside a local adjustment. It could not have worked: the detail stage is its own dispatch, running after the masks are already applied, with nowhere to be handed one layer's mask. The control would have moved and done nothing. Filter the layer's chain to the operations that can honour it. --- core/dr-pipeline/src/mask.rs | 72 +++++++++++++++++++++++++++++++++++- 1 file changed, 70 insertions(+), 2 deletions(-) diff --git a/core/dr-pipeline/src/mask.rs b/core/dr-pipeline/src/mask.rs index 76c7d9c..b35b4f2 100644 --- a/core/dr-pipeline/src/mask.rs +++ b/core/dr-pipeline/src/mask.rs @@ -660,12 +660,37 @@ pub struct MaskLayer { pub ops: Vec>, } +/// The chain a mask layer holds: every point operation, and none of the +/// neighbourhood ones. +/// +/// A layer's adjustments are fused into the colour dispatch and multiplied by +/// the mask afterwards, which is exactly why a layer needs no per-operation +/// support — the composer already knows how to turn a chain into WGSL. A +/// neighbourhood operation cannot go through that path at all: it runs as its +/// own dispatch in [`crate::detail`], after the fused pass and after the masks +/// have already been applied, and there is nowhere in that arrangement for it +/// to be given one layer's mask. +/// +/// Left in, it would be worse than absent. `Operation::wgsl_body` returns an +/// empty string for a detail operation, so the layer would emit an empty block +/// and the panel — which builds itself from [`MaskLayer::capabilities`] and +/// names no operation — would offer a slider that moved and did nothing. +/// Filtering here means a local sharpening or denoise control simply does not +/// appear until there is a stage that can honour it, which is the honest +/// state of affairs. +fn layer_chain() -> Vec> { + ops::chain() + .into_iter() + .filter(|o| o.detail().is_none()) + .collect() +} + impl Clone for MaskLayer { /// Cloned by *value*, not by handle: the ops are trait objects, so this /// rebuilds a fresh chain and copies the parameters across. Needed because /// the UI edits a layer speculatively and the history stores snapshots. fn clone(&self) -> Self { - let mut ops = ops::chain(); + let mut ops = layer_chain(); for (dst, src) in ops.iter_mut().zip(&self.ops) { for p in src.descriptor().params { dst.set_param(p.id, src.param(p.id)); @@ -738,7 +763,7 @@ impl MaskLayer { falloff: Falloff::default(), morphology: Morphology::default(), morph_radius: 0.0, - ops: ops::chain(), + ops: layer_chain(), } } @@ -1276,6 +1301,49 @@ mod tests { assert_eq!(stack.len(), 1, "but they are not deleted"); } + #[test] + fn a_layer_offers_only_the_operations_it_can_actually_apply() { + // A layer's adjustments are fused into the colour dispatch and then + // multiplied by the mask. A neighbourhood operation cannot take that + // route: it is a dispatch of its own, run after the fused pass and + // after the masks are already applied, so there is nowhere to hand it + // one layer's mask. + // + // The panel builds itself from `capabilities()` and names no + // operation, so anything left in this chain becomes a control. One + // that cannot work is worse than one that is missing: it moves, the + // picture does not change, and nothing says why. + let layer = lit_layer("m1", 1.0); + let ids: Vec<&str> = layer.capabilities().iter().map(|c| c.id.0).collect(); + + let global = crate::ops::chain(); + for op in &global { + let id = op.descriptor().id.0; + assert_eq!( + ids.contains(&id), + op.detail().is_none(), + "{id} is offered as a local adjustment but cannot be one, \ + or is a point operation and has gone missing from a layer" + ); + } + assert!( + ids.len() < global.len() || global.iter().all(|o| o.detail().is_none()), + "the filter dropped nothing, so either it is not running or the \ + chain has no neighbourhood operation left to drop" + ); + + // And a clone must rebuild the same chain: it copies parameters across + // by position, so a chain built one way and rebuilt another would + // silently apply each value to the wrong operation. + let cloned: Vec<&str> = layer + .clone() + .capabilities() + .iter() + .map(|c| c.id.0) + .collect(); + assert_eq!(ids, cloned); + } + #[test] fn zero_opacity_is_inactive() { let mut layer = lit_layer("m1", 1.0);