diff --git a/core/dr-gpu/src/adjust.rs b/core/dr-gpu/src/adjust.rs index f9a261a..85ed9f8 100644 --- a/core/dr-gpu/src/adjust.rs +++ b/core/dr-gpu/src/adjust.rs @@ -1064,21 +1064,28 @@ mod tests { g.set_param(cap.id, p.id, value); let shader = g.compose(); - // Through the detail stage rather than through `render`, - // because a neighbourhood operation contributes no fused - // fragment: its WGSL is generated per resolution and lives - // in dispatches of its own. Compiling only the fused half - // would leave every kernel in the chain untested here — - // and worse, `render` refuses a shader composed to hand on - // linear working values, so the omission would arrive as - // "invalid WGSL" against a shader that is perfectly valid. + // 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 scale comes from the graph, so the kernel really is - // converted the way a render converts it. The image is - // 16×16 and so is the target, which puts the ratio at 1.0 - // and keeps an acutance operation from declining to draw - // (`RenderScale::resolves`) and compiling its pass-through - // instead of the kernel this test exists to check. + // Compiling only the fused half would leave every kernel + // untested here — and worse, `render` refuses a shader + // composed to hand on linear working values, so the + // omission would arrive as "invalid WGSL" against a shader + // that is perfectly valid. + // + // The scale comes from the graph, so the kernel is + // converted the way a real render converts it. Mind the + // size: a radius stated in source pixels can decide there + // is nothing to draw at sixteen pixels + // (`RenderScale::resolves`) and compile its pass-through + // instead of the kernel under test. 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); @@ -1442,38 +1449,50 @@ mod tests { } } - let shader = g.compose(); - // 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); - // A neighbourhood operation is active and yet emits no fused block: it - // reads pixels it is not writing, so it is a dispatch of its own. The - // ones that are come from the detail chain rather than from a list - // here, which keeps the count exact as sharpening, noise reduction and - // clarity arrive instead of loosening it to an inequality. - let neighbourhood: std::collections::BTreeSet<&str> = detail - .passes - .iter() - .map(|p| p.label.split('/').next().expect("/")) - .collect(); - - assert_eq!( - shader.source.matches("---- ").count(), - // Every fusable operation, plus framing — which emits a stage of - // its own rather than an operation block, and is not in - // `descriptors` — less the ones that run after this shader. - g.descriptors().len() + 1 - neighbourhood.len(), - "every fusable operation and the framing should be active" + // 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!( 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" + ); // Both halves, from the one graph: with a detail stage present the // fused pass stops at linear working values and the last detail pass diff --git a/core/dr-gpu/tests/noise_reduction.rs b/core/dr-gpu/tests/noise_reduction.rs new file mode 100644 index 0000000..8ebf2f3 --- /dev/null +++ b/core/dr-gpu/tests/noise_reduction.rs @@ -0,0 +1,549 @@ +//! 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.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 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, + "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: 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 +/// 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; + 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 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 +/// 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. +/// +/// `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 value = sample(linear(pixels, width, x, row)); + let sign = if (x / half_period) % 2 == 0 { 1.0 } else { -1.0 }; + total += value * sign; + count += 1.0; + } + total / count +} + +/// 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() { + // 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. + // + // 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, 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, 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}" + ); +} + +#[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 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 + // 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, 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, chroma_r); + + // Both must be doing something: two flat images would agree perfectly and + // 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, chroma_r) + }; + assert!( + export_amp < untouched * 0.8, + "the denoiser did nothing: {export_amp} of {untouched}" + ); + + assert!( + (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" + ); + + // 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, chroma_r); + assert!( + wrong_amp < export_amp * 0.8, + "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 73a4acc..e4a10b0 100644 --- a/core/dr-pipeline/ops/README.md +++ b/core/dr-pipeline/ops/README.md @@ -212,9 +212,10 @@ half in Rust would be worse than either alone. Currently hand-written: `tone_curve` (one widget over four curves of five interpolated points — master, red, green, blue — each reaching the shader only when it has been moved), `colour_mixer` (thirty-six faceted parameters from -twelve computed hue bands), `capture_sharpen` (a separable convolution, which -is the other reason a node is Rust — see the next section). `vignetting` is -hand-written too but is not in the develop chain — it +twelve computed hue bands), `capture_sharpen` (a separable convolution) and +`noise_reduction` (a kernel, and one that decides how many dispatches to emit +at each resolution) — the last two for the reason the next section gives. +`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/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 = detail - .passes - .iter() - .map(|p| p.label.split('/').next().expect("/")) - .collect(); - + // Composed at 1:1 deliberately. An acutance operation's radius is in + // source pixels, so on a proxy it may honestly decline to draw at all + // (`RenderScale::resolves`) — which would put it in neither half and + // make the assertion fail for a reason that is not a defect. + 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() - neighbourhood.len(), - "every operation that can be a fused fragment should appear" + fused_blocks, + "the fused shader carries a block nothing in the chain asked for" ); // And each neighbourhood operation is genuinely absent from the fused diff --git a/core/dr-pipeline/src/mask.rs b/core/dr-pipeline/src/mask.rs index 2428cc6..e77e963 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(), } } @@ -1286,6 +1311,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); diff --git a/core/dr-pipeline/src/ops/mod.rs b/core/dr-pipeline/src/ops/mod.rs index 413bce3..b2a6c13 100644 --- a/core/dr-pipeline/src/ops/mod.rs +++ b/core/dr-pipeline/src/ops/mod.rs @@ -30,11 +30,18 @@ //! //! # The neighbourhood nodes //! -//! [`capture_sharpen`] reads the pixels around the one it writes, so it runs -//! in [`crate::detail`]'s stage after the fused pass rather than as a fragment -//! within it. It is an ordinary [`Operation`](crate::Operation) in every other -//! respect — descriptor, parameters, sidecar, history — which is what lets the -//! panel, the presets and the undo stack carry it with no special case. +//! [`capture_sharpen`] and [`noise_reduction`] read the pixels around the one +//! they write, so they run in [`crate::detail`]'s stage after the fused pass +//! rather than as fragments within it. They are ordinary +//! [`Operation`](crate::Operation)s in every other respect — descriptor, +//! parameters, sidecar, history — which is what lets the panel, the presets +//! and the undo stack carry them with no special case. +//! +//! [`noise_reduction`] shows why the declarative schema cannot express one at +//! all: a declared node's `wgsl:` is handed a colour with no way back to a +//! coordinate. A kernel decides at each render how many dispatches to emit, +//! and converts a radius stated in sensor pixels into the render pixels this +//! frame is actually being drawn at. //! //! Both publish the same [`crate::descriptor::OpDescriptor`], so nothing //! downstream can tell them apart. A hand-written node still declares its @@ -55,6 +62,7 @@ pub mod capture_sharpen; pub mod colour_mixer; pub mod curve; pub mod distortion; +pub mod noise_reduction; pub mod vignetting; pub use aberration::Aberration; @@ -62,6 +70,7 @@ pub use capture_sharpen::CaptureSharpen; 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 4ba5b2a..95851c6 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -1056,13 +1056,23 @@ impl DevelopSession { /// 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. /// - /// `space` is the output space `shader` was composed for, and it has to be - /// passed rather than assumed because the **detail stage** is composed - /// here too and the two halves must agree. When an edit has an active - /// neighbourhood operation the fused pass stops at unclipped linear - /// working values and the last detail pass performs the output transform; - /// composing the fused half for Display P3 and the detail half for sRGB - /// would encode the export in the wrong space, with nothing to notice it. + /// **And the detail stage with it.** The neighbourhood operations — noise + /// reduction, capture sharpening, 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, @@ -1085,15 +1095,18 @@ impl DevelopSession { // scale-free: a sharpening radius is stated in source pixels and the // develop view renders at whatever the viewport needs (FR-DSP-1), so // the conversion is different for the canvas, the thumbnail and the - // export. `render_scale` works the ratio out from the framing, which - // is also what makes zooming to 1:1 restore an exact preview with no - // second render path to maintain. + // export. `render_scale` works the ratio out from the framing, so a + // crop and a zoom are already accounted for, and zooming to 1:1 + // restores an exact preview with no second render path to maintain. // // Empty for every edit with no active neighbourhood operation — which // is almost all of them — and `render_detailed` then falls straight // through to the single masked dispatch this used to call. 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()