diff --git a/core/dr-gpu/src/adjust.rs b/core/dr-gpu/src/adjust.rs index df7c9d1..f9a261a 100644 --- a/core/dr-gpu/src/adjust.rs +++ b/core/dr-gpu/src/adjust.rs @@ -1063,12 +1063,32 @@ 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 - ) - }); + + // 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. + // + // 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. + 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 + ) + }); } } } @@ -1423,23 +1443,44 @@ 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 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 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" + // 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" ); assert!( shader.source.contains("---- framing ----"), "framing must reach the shader alongside the colour operations" ); - // 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) + // Both halves, from the one graph: with a detail stage present the + // fused pass stops at linear working values and the last detail pass + // performs the output transform, so rendering only the first half is + // not a smaller test — it is a texture format the driver rejects. + 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"); } diff --git a/core/dr-gpu/tests/capture_sharpen.rs b/core/dr-gpu/tests/capture_sharpen.rs new file mode 100644 index 0000000..1b101ee --- /dev/null +++ b/core/dr-gpu/tests/capture_sharpen.rs @@ -0,0 +1,505 @@ +//! Capture sharpening, end to end on a real device. +//! +//! `dr-pipeline`'s own tests assert what the composer *generates* — the kernel +//! extent, the uniforms, which pass encodes. None of them can tell whether the +//! generated WGSL compiles, whether the second pass is handed what the first +//! one wrote, or whether the result is sharpening rather than a shader that +//! silently produced the input again. Those are questions only a GPU answers. +//! +//! # Reading the expected values +//! +//! The source is uploaded through `DemosaicedImage::from_rgba8`, which flags it +//! non-linear, so the generated shader decodes sRGB before any operation runs +//! and a black/white step reaches the detail stage as linear 0.0 and 1.0 +//! exactly. The last detail pass re-encodes. So a byte read back here is +//! `srgb_encode(whatever the kernel produced in linear light)`, and an +//! overshoot — the bright fringe an unsharp mask puts on the light side of an +//! edge — cannot show above 255 on the white side of a full-scale step, and the +//! undershoot on the dark side of one clips to black long before the halo has +//! been drawn. The tests therefore use a **grey** step, from byte 90 to byte +//! 150, which at 100% amount leaves the whole halo inside the representable +//! range at both ends. Every expected value below is arithmetic on that step, +//! not a number read off a previous run. + +use dr_gpu::{AdjustPass, DemosaicedImage, GpuContext}; +use dr_pipeline::descriptor::ParamId; +use dr_pipeline::ops::capture_sharpen::{AMOUNT, ID, RADIUS, THRESHOLD}; +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 + } + } +} + +/// A vertical step from `low` to `high`, changing at the middle column. +/// +/// The one image whose sharpening is worth checking by hand: an unsharp mask +/// must darken the last few columns before the step and brighten the first few +/// after it, and leave everything further away exactly where it was. A gradient +/// would blur to itself and hide a kernel that does nothing at all. +fn step_edge(ctx: &GpuContext, size: u32, low: u8, high: u8) -> DemosaicedImage { + let data: Vec = (0..size * size) + .flat_map(|i| { + let v = if (i % size) < size / 2 { low } else { high }; + [v, v, v, 255] + }) + .collect(); + DemosaicedImage::from_rgba8(ctx, &data, size, size).expect("upload") +} + +/// A flat field of one value. +fn flat(ctx: &GpuContext, size: u32, value: u8) -> DemosaicedImage { + let data: Vec = (0..size * size) + .flat_map(|_| [value, value, value, 255]) + .collect(); + DemosaicedImage::from_rgba8(ctx, &data, size, size).expect("upload") +} + +/// One row of the rendered image, red channel, as bytes. +fn row(pixels: &[u8], width: u32, y: u32) -> Vec { + (0..width) + .map(|x| pixels[((y * width + x) * 4) as usize]) + .collect() +} + +/// The develop chain with capture sharpening set. +fn sharpened(amount: f32, radius: f32, threshold: f32) -> EditGraph { + let mut graph = EditGraph::default_chain(); + graph.set_param(ID, AMOUNT, amount); + graph.set_param(ID, RADIUS, radius); + graph.set_param(ID, THRESHOLD, threshold); + graph +} + +/// Render one graph, with its detail stage, and read the pixels back. +/// +/// The whole calling convention a frontend adopts, in five lines: compose both +/// halves from one graph at one output space, ask the graph for the scale, and +/// pass the invalidation key through. +fn render( + pass: &mut AdjustPass, + graph: &EditGraph, + source: &DemosaicedImage, + out: u32, +) -> Vec { + let shader = graph.compose_for(ColourSpace::Srgb); + let scale = graph.render_scale(source.size(), (out, out)); + let detail = graph.compose_detail_for(scale, ColourSpace::Srgb); + let key = graph.invalidation().through(Affects::Colour); + pass.render_detailed(source, &shader, out, out, None, &detail, key) + .expect("render"); + pass.export_pixels().expect("readback").0 +} + +#[test] +fn an_unsharp_mask_puts_a_halo_on_the_edge_and_leaves_the_rest_alone() { + // What sharpening *is*, asserted as pixels rather than as "something + // changed": an undershoot immediately before the transition, an overshoot + // immediately after it, the step itself steeper than it was, and the flat + // ground at either end untouched. A shader that ran the blur and forgot to + // add the difference back would pass a "the image changed" test and fail + // every one of these. + let Some(ctx) = ctx() else { return }; + const SIZE: u32 = 64; + let source = step_edge(&ctx, SIZE, 90, 150); + + let plain = render( + &mut AdjustPass::new(&ctx), + &EditGraph::default_chain(), + &source, + SIZE, + ); + let sharp = render( + &mut AdjustPass::new(&ctx), + &sharpened(100.0, 2.0, 0.0), + &source, + SIZE, + ); + + let before = row(&plain, SIZE, SIZE / 2); + let after = row(&sharp, SIZE, SIZE / 2); + let edge = (SIZE / 2) as usize; + + // The dark side of the transition is driven darker and the light side + // lighter — the halo. Two pixels in, where a two-pixel-sigma kernel has + // most of its response. + assert!( + after[edge - 2] < before[edge - 2], + "the dark side of the edge should be pushed down: {} -> {}", + before[edge - 2], + after[edge - 2] + ); + assert!( + after[edge + 1] > before[edge + 1], + "the light side of the edge should be pushed up: {} -> {}", + before[edge + 1], + after[edge + 1] + ); + + // And the transition really is steeper across the same two columns. + let slope = |r: &[u8]| r[edge] as i32 - r[edge - 1] as i32; + assert!( + slope(&after) > slope(&before), + "sharpening must steepen the edge: {} -> {}", + slope(&before), + slope(&after) + ); + + // Far from the edge there is nothing to sharpen, so nothing may move. This + // is the property a kernel that forgot to normalise its weights breaks, + // and it breaks it as a brightness shift over the whole photograph. + for x in [0usize, 4, 8, SIZE as usize - 1] { + assert!( + after[x].abs_diff(before[x]) <= 1, + "column {x} is flat ground and moved: {} -> {}", + before[x], + after[x] + ); + } +} + +#[test] +fn a_flat_field_survives_any_amount_of_sharpening() { + // The kernel sums to one — `(1 + a)` of the pixel minus `a` of its blur — + // so a sky must come through bit for bit however far the slider is pushed. + // The border is the part that is easy to get wrong: `tap` clamps, and a + // kernel that normalised by an analytic integral instead of by the weights + // it actually summed would draw a band around the whole frame. + let Some(ctx) = ctx() else { return }; + const SIZE: u32 = 48; + let source = flat(&ctx, SIZE, 128); + + let plain = render( + &mut AdjustPass::new(&ctx), + &EditGraph::default_chain(), + &source, + SIZE, + ); + let sharp = render( + &mut AdjustPass::new(&ctx), + &sharpened(100.0, 3.0, 0.0), + &source, + SIZE, + ); + + for (i, (a, b)) in sharp.iter().zip(&plain).enumerate() { + assert!( + a.abs_diff(*b) <= 1, + "pixel {} of a flat field moved: {b} -> {a}", + i / 4 + ); + } +} + +#[test] +fn a_proxy_and_an_export_sharpen_the_same_photograph() { + // TRACES: FR-DSP-1 — the decision this operation is most likely to get + // wrong, and the one that is invisible until an export comes back wrong. + // + // The same edit, rendered at two resolutions of one source. The radius is + // in source pixels, so the halo must cover the same *proportion of the + // picture* at both: a fringe four source pixels wide is four source pixels + // wide whether it was drawn on a half-size proxy or at full size. + // + // Read the radius as render pixels instead and the proxy's halo would be + // twice as wide relative to the frame and roughly twice as strong, so what + // was tuned on screen would not be what landed in the file. That is the + // failure this catches, and it is a large one: the widths would differ by a + // factor of two, not by a rounding. + let Some(ctx) = ctx() else { return }; + const SOURCE: u32 = 128; + let source = step_edge(&ctx, SOURCE, 90, 150); + // The widest radius the slider offers, so that even the half-size proxy + // has a 1.5-pixel sigma and resolves it — the honest cut-off is tested in + // `dr-pipeline`, and this test is about the case where both renders draw. + // Asking for more would be asking for a photograph nobody can produce: + // `EditGraph::set_param` clamps to the descriptor on the way in. + let graph = sharpened(100.0, 3.0, 0.0); + + // The halo, measured against the same edit with no sharpening at the same + // size: how far from the transition the picture is still disturbed, as a + // fraction of the frame, and how much deviation the halo carries in total. + let measure = |out: u32| -> (f32, f32) { + let plain = render( + &mut AdjustPass::new(&ctx), + &EditGraph::default_chain(), + &source, + out, + ); + let sharp = render(&mut AdjustPass::new(&ctx), &graph, &source, out); + let (a, b) = (row(&plain, out, out / 2), row(&sharp, out, out / 2)); + + let disturbed: Vec = (0..out as usize) + .filter(|&x| b[x].abs_diff(a[x]) > 3) + .collect(); + let first = *disturbed.first().expect("a halo"); + let last = *disturbed.last().expect("a halo"); + + // The halo's strength as an *area* — the sum of the deviations, scaled + // by the width of a render pixel — rather than as its peak. A peak is + // one sample of a smooth curve, and the two renders do not sample it at + // the same place: the pixel next to the transition sits half a render + // pixel from it, which is half a source pixel at export and a whole one + // on the proxy, so their peaks would legitimately differ by more than + // the property under test. An integral over the same curve does not + // care where the samples fell. + let area: f32 = (0..out as usize) + .map(|x| b[x].abs_diff(a[x]) as f32) + .sum::() + / out as f32; + ((last - first) as f32 / out as f32, area) + }; + + let (proxy_width, proxy_area) = measure(SOURCE / 2); + let (export_width, export_area) = measure(SOURCE); + + assert!( + (proxy_width - export_width).abs() < 0.06, + "the halo covers {proxy_width:.3} of the proxy and {export_width:.3} \ + of the export; a radius tuned on screen must land in the file" + ); + // The strength has to agree too. A viewport-scaled kernel would not only + // be wider on the proxy, it would push the fringe further, because a wider + // blur takes more away for the high-pass to add back — so the areas would + // differ by considerably more than the sampling slack allowed here. + let ratio = proxy_area / export_area; + assert!( + (0.75..1.35).contains(&ratio), + "the halo carries {proxy_area:.2} on the proxy and {export_area:.2} at \ + export, a ratio of {ratio:.2}" + ); + // And both are a real halo rather than two flat images agreeing. + assert!( + proxy_width > 0.05 && export_width > 0.05, + "{proxy_width:.3} / {export_width:.3}" + ); + assert!(proxy_area > 1.0 && export_area > 1.0, "{proxy_area} / {export_area}"); +} + +#[test] +fn the_threshold_leaves_shallow_modulation_where_it_found_it() { + // What the threshold is for: sensor noise is shallow, and sharpening it is + // the fastest way to make a clean frame look worse. Two images, one with a + // strong edge and one with a shallow ripple, through the same gate — the + // edge must still sharpen and the ripple must not. + let Some(ctx) = ctx() else { return }; + const SIZE: u32 = 64; + + // A four-code ripple: about 4% local contrast at this level, which is the + // order of magnitude read noise reaches on a well-exposed frame — and well + // under the 12.5% at which the gate below starts letting detail through. + let ripple: Vec = (0..SIZE * SIZE) + .flat_map(|i| { + let v = if (i % SIZE) % 2 == 0 { 128u8 } else { 132 }; + [v, v, v, 255] + }) + .collect(); + let ripple = DemosaicedImage::from_rgba8(&ctx, &ripple, SIZE, SIZE).expect("upload"); + let edge = step_edge(&ctx, SIZE, 90, 150); + + let gated = sharpened(100.0, 1.0, 1.0); + let ungated = sharpened(100.0, 1.0, 0.0); + + let spread = |graph: &EditGraph, source: &DemosaicedImage| -> u8 { + let pixels = render(&mut AdjustPass::new(&ctx), graph, source, SIZE); + let line = row(&pixels, SIZE, SIZE / 2); + // Peak-to-peak over the middle of the row, away from the border. + let window = &line[8..24]; + window.iter().max().unwrap() - window.iter().min().unwrap() + }; + + let ripple_open = spread(&ungated, &ripple); + let ripple_gated = spread(&gated, &ripple); + assert!( + ripple_gated < ripple_open, + "the gate must hold shallow modulation back: {ripple_open} -> \ + {ripple_gated}" + ); + + // The edge is deep modulation and must come through the same gate + // sharpened — a threshold that flattens everything is not a threshold. + let plain_edge = { + let pixels = render( + &mut AdjustPass::new(&ctx), + &EditGraph::default_chain(), + &edge, + SIZE, + ); + row(&pixels, SIZE, SIZE / 2) + }; + let gated_edge = { + let pixels = render(&mut AdjustPass::new(&ctx), &gated, &edge, SIZE); + row(&pixels, SIZE, SIZE / 2) + }; + let mid = (SIZE / 2) as usize; + assert!( + gated_edge[mid - 1] < plain_edge[mid - 1], + "a real edge must still sharpen through the gate: {} -> {}", + plain_edge[mid - 1], + gated_edge[mid - 1] + ); +} + +#[test] +fn sharpening_an_edge_does_not_change_its_colour() { + // The reason the high-pass is applied as a gain on the three channels + // rather than as an offset. An offset moves a saturated colour towards + // grey as it brightens it, so a sharpened red roof gets a pink fringe — + // which reads as chromatic aberration and gets blamed on the lens. + let Some(ctx) = ctx() else { return }; + const SIZE: u32 = 64; + + // A step between two saturated reds of different brightness: the ratios + // between the channels are the colour, and they must survive the halo. + let data: Vec = (0..SIZE * SIZE) + .flat_map(|i| { + if (i % SIZE) < SIZE / 2 { + [80u8, 30, 30, 255] + } else { + [200, 75, 75, 255] + } + }) + .collect(); + let source = DemosaicedImage::from_rgba8(&ctx, &data, SIZE, SIZE).expect("upload"); + + let pixels = render( + &mut AdjustPass::new(&ctx), + &sharpened(60.0, 2.0, 0.0), + &source, + SIZE, + ); + + // Sampled inside the halo, where an additive sharpener would have washed + // the colour out most. + let y = SIZE / 2; + for x in [SIZE / 2 - 2, SIZE / 2 + 1] { + let i = ((y * SIZE + x) * 4) as usize; + let (r, g, b) = (pixels[i] as f32, pixels[i + 1] as f32, pixels[i + 2] as f32); + assert!(r > g && r > b, "the fringe lost its hue at column {x}"); + // Green and blue started equal and must stay equal: an offset would + // keep them equal too, but the *ratio* to red is what moves, and this + // is the assertion that it did not. + let saturation = (r - g) / r; + assert!( + saturation > 0.55, + "column {x} washed out: rgb {r} {g} {b}, saturation {saturation:.3}" + ); + } +} + +#[test] +fn dragging_the_amount_recompiles_nothing_and_reallocates_nothing() { + // The two costs that are ruinous per frame and invisible in the output. + // A sharpening slider is dragged continuously, so this is the difference + // between a control that tracks the mouse and one that stutters. + let Some(ctx) = ctx() else { return }; + const SIZE: u32 = 48; + let source = step_edge(&ctx, SIZE, 90, 150); + let mut pass = AdjustPass::new(&ctx); + let mut graph = sharpened(40.0, 1.0, 0.0); + + render(&mut pass, &graph, &source, SIZE); + let pipelines = pass.cached_detail_pipelines(); + let allocations = pass.detail_allocations(); + assert_eq!(pipelines, 2, "one per axis of the separable mask"); + assert_eq!(allocations, 2, "the colour result, and one hand-off"); + assert_eq!(pass.detail_dispatches(), 2); + assert_eq!(pass.colour_dispatches(), 1); + + for amount in [50.0, 60.0, 70.0, 80.0] { + graph.set_param(ID, AMOUNT, amount); + render(&mut pass, &graph, &source, 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" + ); + // TRACES: FR-DEV-3d — and the operational point of `Affects::Detail`: + // sharpening is downstream of every fused operation, so dragging it must + // not re-run them. + assert_eq!( + pass.colour_dispatches(), + 1, + "the fused colour pass re-ran for a change it does not depend on" + ); + + // The radius is also only a uniform, even though it changes the kernel + // extent — the loop bound is read from the uniform block rather than + // baked into the source, which is what keeps a drag off the compiler. + graph.set_param(ID, RADIUS, 2.5); + render(&mut pass, &graph, &source, SIZE); + assert_eq!(pass.cached_detail_pipelines(), pipelines); + graph.set_param(ID, THRESHOLD, 0.3); + render(&mut pass, &graph, &source, SIZE); + assert_eq!(pass.cached_detail_pipelines(), pipelines); +} + +#[test] +fn a_render_too_coarse_for_the_radius_still_reaches_the_screen() { + // The failure mode that the pass-through exists to prevent, proved on a + // device rather than argued about. With the radius finer than a render + // pixel the operation declines to sharpen — but it is still active, so the + // fused pass has already been composed to hand on unclipped linear values, + // and something must still perform the output transform. An empty chain + // here would not be a soft preview: it would be a hard error out of + // `render_detailed`, on the most ordinary develop view there is. + let Some(ctx) = ctx() else { return }; + const SOURCE: u32 = 128; + const RENDER: u32 = 32; // a quarter scale, as a fit view of a large frame + let source = step_edge(&ctx, SOURCE, 90, 150); + + let graph = sharpened(100.0, 1.0, 0.0); + let scale = graph.render_scale((SOURCE, SOURCE), (RENDER, RENDER)); + assert!(!scale.resolves(1.0), "the premise of this test"); + + let mut pass = AdjustPass::new(&ctx); + let sharp = render(&mut pass, &graph, &source, RENDER); + assert_eq!(pass.detail_dispatches(), 1, "one pass, and it only encodes"); + + // And what reaches the screen is the unsharpened picture, not a black + // frame, a linear one, or a guess. + let plain = render( + &mut AdjustPass::new(&ctx), + &EditGraph::default_chain(), + &source, + RENDER, + ); + for (i, (a, b)) in sharp.iter().zip(&plain).enumerate() { + assert!( + a.abs_diff(*b) <= 1, + "pixel {} differs from the unsharpened render: {b} -> {a}", + i / 4 + ); + } +} + +#[test] +fn the_operation_is_reachable_by_the_ids_a_frontend_will_use() { + // FR-DEV-3c: adding an operation needs no UI change, which is only true if + // the panel can find it through the capability list. A typo between the + // declaration's `id:` and the descriptor's would place it in the chain + // under one name and address it under another. + let graph = EditGraph::default_chain(); + let cap = graph + .capabilities() + .into_iter() + .find(|c| c.id == ID) + .expect("capture sharpening is in the default chain"); + let names: Vec = cap.params.iter().map(|p| p.id).collect(); + assert_eq!(names, vec![AMOUNT, RADIUS, THRESHOLD]); + assert!(!cap.active, "a fresh chain is not sharpening anything"); +} diff --git a/core/dr-pipeline/ops/README.md b/core/dr-pipeline/ops/README.md index aa27c1a..73a4acc 100644 --- a/core/dr-pipeline/ops/README.md +++ b/core/dr-pipeline/ops/README.md @@ -212,7 +212,9 @@ 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). `vignetting` is hand-written too but is not in the develop chain — it +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 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. @@ -240,6 +242,9 @@ rather than a convenience. A node of this kind: each with a WGSL body, its uniforms, and **its kernel radius in render pixels**, which the tile scheduler needs and nothing can infer. +`capture_sharpen` is the worked example: two passes, one per axis, and a radius +converted from source pixels once per render. + The `order:` still belongs here, and still orders the node — among the other detail nodes. Detail runs as a group after every point operation, so an `order:` that interleaves one with exposure would be a lie the chain cannot tell. diff --git a/core/dr-pipeline/ops/capture_sharpen.yaml b/core/dr-pipeline/ops/capture_sharpen.yaml new file mode 100644 index 0000000..f135e7c --- /dev/null +++ b/core/dr-pipeline/ops/capture_sharpen.yaml @@ -0,0 +1,33 @@ +# A hand-written node, and a neighbourhood one: it reads the pixels around the +# one it is writing, so it runs in the detail stage rather than as a fragment +# in the fused pass. See `../src/detail.rs` for why that stage exists and +# `README.md`'s "Nodes that read their neighbours" for the contract. +# +# As with every `rust:` node, its descriptor, parameters and behaviour come +# from the type; this file exists so that `ops/` remains the one place the +# pipeline's order is written down. +id: capture_sharpen +order: 110 + +attributes: [detail] +rust: CaptureSharpen + +why_rust: | + A convolution, not a point function. The schema in `README.md` describes an + operation handed a colour with no way back to a coordinate, which is exactly + what a kernel cannot work with — and stretching it to cover taps, kernel + extents and a per-render conversion from source pixels to render pixels + would produce a worse language than Rust aimed at one caller. + +placement: | + First among the detail nodes, because capture sharpening is a correction to + the capture: it recovers the acutance the anti-aliasing filter, the lens's + circle of confusion and the demosaic interpolation each took out, and it is + meaningful before any effect built on top of it. The compositional detail + controls — texture, clarity — reasonably follow it, since they are about the + picture rather than about the sensor. + + Being in the detail group at all is what places it after every tonal and + chromatic operation: an amount tuned before a tone curve is amplified by + whatever slope that curve happens to have, so the amount that looked right + stops looking right the moment the curve moves. diff --git a/core/dr-pipeline/src/lib.rs b/core/dr-pipeline/src/lib.rs index a763ef9..68545e9 100644 --- a/core/dr-pipeline/src/lib.rs +++ b/core/dr-pipeline/src/lib.rs @@ -107,13 +107,45 @@ 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. + + // A neighbourhood operation contributes no fused fragment. That is not + // an omission: it reads pixels it is not writing, the fused contract + // hands a fragment a colour with no way back to a coordinate, and + // `compose_full` filters it out rather than emitting an empty block + // that would read as an operation doing nothing (see `crate::detail`). + // + // So the count is against the operations that *can* be fused, and the + // ones that cannot are named by the detail chain rather than by a list + // written here — which is what keeps this test correct as sharpening, + // noise reduction and clarity arrive, rather than weakening it into + // "most of them appear". + // + // Composed at source resolution deliberately: an acutance operation's + // radius is in source pixels, and on a proxy it may honestly decline + // to draw at all (`RenderScale::resolves`), which would leave it out + // of both halves and make this count agree for the wrong reason. + let detail = g.compose_detail(crate::detail::RenderScale::full((4096, 4096))); + let neighbourhood: std::collections::BTreeSet<&str> = detail + .passes + .iter() + .map(|p| p.label.split('/').next().expect("/")) + .collect(); + assert_eq!( shader.source.matches("---- ").count(), - g.descriptors().len(), - "every operation in the chain should appear" + g.descriptors().len() - neighbourhood.len(), + "every operation that can be a fused fragment should appear" ); + + // And each neighbourhood operation is genuinely absent from the fused + // shader rather than merely uncounted — the arithmetic above would be + // satisfied just as well by two errors that cancelled. + for id in &neighbourhood { + assert!( + !shader.source.contains(&format!("---- {id} ----")), + "{id} reads its neighbours and cannot be a fused fragment" + ); + } } #[test] diff --git a/core/dr-pipeline/src/mask.rs b/core/dr-pipeline/src/mask.rs index 76c7d9c..2428cc6 100644 --- a/core/dr-pipeline/src/mask.rs +++ b/core/dr-pipeline/src/mask.rs @@ -934,9 +934,19 @@ impl MaskLayer { /// the output's dimensions, so it is a property of the photograph and not /// of a region within it. There is no such thing as cropping part of an /// image. + /// + /// The neighbourhood operations are absent for the same reason, and this + /// filter is the visible half of the one [`Self::active_ops`] already + /// applies. A layer's chain is fused into the point-operation pass; the + /// detail stage runs once, afterwards, over the whole frame, so there is + /// no seam through which a mask could reach it (see [`crate::detail`]). + /// Offering the controls anyway would put a sharpening slider on a mask + /// that moves and does nothing — which is worse than the control being + /// absent, because absence is legible and a dead slider is not. pub fn capabilities(&self) -> Vec { self.ops .iter() + .filter(|op| op.detail().is_none()) .map(|op| { let desc = op.descriptor(); crate::graph::OpCapability { @@ -1625,4 +1635,38 @@ mod tests { let order: Vec<&str> = stack.layers().iter().map(|l| l.id.as_str()).collect(); assert_eq!(order, ["m3", "m1", "m2"]); } + + #[test] + fn a_layer_offers_no_control_it_cannot_honour() { + // A layer's chain is fused into the point-operation pass, and the + // detail stage runs once afterwards over the whole frame — so a + // neighbourhood operation inside a mask has nowhere to run. + // `active_ops` has always dropped them; this is the other half, which + // stops the panel drawing a sharpening slider on a mask that would + // move and change nothing. + let layer = lit_layer("m1", 1.0); + let global: Vec<&str> = crate::EditGraph::default_chain() + .capabilities() + .iter() + .map(|c| c.id.0) + .collect(); + let scoped: Vec<&str> = layer.capabilities().iter().map(|c| c.id.0).collect(); + + let detail: Vec<&str> = crate::ops::chain() + .iter() + .filter(|o| o.detail().is_some()) + .map(|o| o.descriptor().id.0) + .collect(); + assert!( + !detail.is_empty(), + "the chain has neighbourhood operations, or this proves nothing" + ); + for id in detail { + assert!(global.contains(&id), "{id} is missing from the chain"); + assert!( + !scoped.contains(&id), + "{id} cannot run inside a mask and must not be offered there" + ); + } + } } diff --git a/core/dr-pipeline/src/ops/capture_sharpen.rs b/core/dr-pipeline/src/ops/capture_sharpen.rs new file mode 100644 index 0000000..d727a19 --- /dev/null +++ b/core/dr-pipeline/src/ops/capture_sharpen.rs @@ -0,0 +1,864 @@ +//! TRACES: FR-DEV-3 | FR-DSP-1 +//! Capture sharpening — an unsharp mask against the sensor. +//! +//! Every raw file arrives softer than the scene was. The anti-aliasing filter +//! spreads a point over more than one photosite on purpose, the lens's circle +//! of confusion spreads it further, and the demosaic interpolates two of every +//! three colour samples at each site from its neighbours. None of that is a +//! mistake to be corrected in the developed *picture*; it is a property of how +//! the frame was recorded, and capture sharpening is the step that undoes as +//! much of it as the data supports before anything else is built on top. +//! +//! That distinction is the whole reason this operation exists separately from +//! the output sharpening in `dr-export` (FR-EXP-4). Output sharpening is aimed +//! at a size and a medium — a 900-pixel web image and a matte A2 print want +//! different treatment of the same edit. Capture sharpening is aimed at the +//! sensor, and its answer does not change because the file is going somewhere +//! else. +//! +//! # Unsharp mask, and why the plainest one +//! +//! Blur a copy, subtract it from the original, and add back some multiple of +//! the difference. The difference is everything the blur threw away — the +//! high-frequency content — so adding it back steepens exactly the transitions +//! that the capture chain flattened, and leaves flat areas alone because the +//! blur of a flat area is the area itself. +//! +//! Deconvolution would in principle do better, since the thing being undone +//! really is a convolution with a roughly known kernel. It is also iterative, +//! needs a per-body point-spread estimate this codebase does not have, and +//! amplifies noise in a way that needs its own regularisation. An unsharp mask +//! is the baseline every editor ships and the one a photographer's hands +//! already know; a deconvolution mode can be added later behind the same three +//! parameters without changing what those parameters mean. +//! +//! # Why two passes and not one kernel +//! +//! A Gaussian is separable: blurring along x and then along y gives exactly +//! the same result as a single two-dimensional kernel, at 2(2R+1) taps per +//! pixel instead of (2R+1)². At the radii this operation reaches when zoomed +//! in — a 3-source-pixel radius at 400% is a kernel extent of 36 render pixels +//! — that is 146 taps against 5 329, and it is the difference between a +//! sharpening slider that tracks the mouse and one that does not. +//! +//! [`crate::detail::DetailStage::passes`] returns a *list* precisely so this +//! is expressible: the stage ping-pongs between intermediates, so the second +//! pass is handed what the first one wrote with no plumbing here. +//! +//! ## What the chain can and cannot do, exactly +//! +//! A detail pass reads exactly one texture — whatever ran before it. So the +//! textbook arrangement, "blur in two passes and then subtract the result from +//! the original", is not available: by the time the blur is finished the +//! original is two dispatches behind and nothing is holding it. That is not a +//! gap to be worked around with a third pass either, and it is worth writing +//! down why, because it looks like it should be. +//! +//! Write `Gx`, `Gy` for the two one-dimensional blurs and `Hx = I - Gx`, +//! `Hy = I - Gy` for the high-passes they define. A second pass can form only +//! `α·t + β·Gy(t)` from what the first pass left it in `t`. The result wanted +//! is `(1+a)c - a·GyGx(c)`, whose only occurrence of `c` is under two blurs; +//! matching the `GyGx` term needs `β ≠ 0`, and then the stray `Gy(c)` term +//! that comes with it can only be cancelled by `α·t` if `t` contains `c` +//! unblurred, which the `Gx` in the same expression rules out. No number of +//! extra passes changes this: each one only adds another blur in front. +//! +//! So the two passes each apply a *one-dimensional* unsharp mask, and the +//! composite is the product of the two one-dimensional kernels: +//! +//! ```text +//! (I + a·Hy)(I + a·Hx) = I + a·(Hx + Hy) + a²·HxHy +//! true unsharp = I + a·(Hx + Hy) - a ·HxHy +//! ``` +//! +//! They differ in one term, and that term is worth understanding rather than +//! apologising for. `HxHy` responds only to structure that curves in both +//! directions at once: on any locally one-dimensional feature — which is what +//! an edge is — one of the two factors is zero and **the two agree exactly**. +//! Run this on a vertical edge and it produces the textbook unsharp mask to +//! the last bit. They part company only at corners and at fine two-dimensional +//! texture, where this arrangement sharpens slightly harder, by `a(1+a)` times +//! a quantity that is itself second-order small. +//! +//! Both preserve a flat field exactly: each one-dimensional kernel sums to +//! `(1+a) - a = 1`, so their product does too, and no amount of sharpening +//! shifts the brightness of a sky. +//! +//! # The radius is in source pixels, and that is the decision to check +//! +//! [`crate::detail::RenderScale`] offers two units and the choice between them +//! is the one thing about a neighbourhood operation that is easy to get wrong +//! and invisible when it is. `frame_fraction` is for lengths that are a +//! property of the *composition* — clarity, texture, dehaze, a mask feather — +//! where "one percent of the frame" is what the photographer meant. This +//! radius is not one of those. It stands for the spread of a point across +//! *photosites*, and a body with a stronger anti-aliasing filter needs a +//! larger one at the same framing, so it is stated in source pixels and +//! converted with [`RenderScale::source_pixels`] once per render. +//! +//! Read as render pixels instead, the slider would mean a different photograph +//! at every size: the develop view renders at whatever the viewport needs +//! (FR-DSP-1), so a 60 MP frame in a 2 000 px panel would be sharpened with a +//! kernel nine times too wide relative to the picture, and the export — the +//! only render that is ever kept — would be the one that looked nothing like +//! what was tuned. `the_radius_is_a_sensor_length_not_a_viewport_one` below +//! and `a_proxy_and_an_export_sharpen_the_same_photograph` in `dr-gpu` are the +//! two halves of the proof that it does not. +//! +//! # Where the honest answer is "not at this size" +//! +//! Converting into render pixels does not conjure detail back. On a proxy at +//! one-third scale a one-source-pixel radius is a third of a render pixel, and +//! the frequencies it would act on were destroyed by the downscale before this +//! stage ran. [`RenderScale::resolves`] is the predicate for that condition and +//! this operation obeys it: below one render pixel it stops, rather than +//! drawing a plausible-looking sharpening that the exported file will not +//! contain. That is why every editor tells the photographer to judge +//! sharpening at 1:1 — and zooming to 1:1 is enough, because the framing's +//! view rect shrinks while the render target keeps its size and the ratio +//! climbs back to one. +//! +//! A softer roll-off, fading the amount out as the kernel approaches a pixel +//! rather than stopping at it, would look better while zooming. It is not done +//! because it needs a second threshold that no requirement supplies and that +//! would be a guess dressed as a number; `resolves` is the line the stage +//! already draws, and drawing it in two places differently is worse than a +//! visible step. + +use crate::descriptor::{ + Attribute, LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Scale, Unit, +}; +use crate::detail::{DetailPass, DetailStage, RenderScale}; +use crate::operation::{Affects, Helper, Operation, Uniform}; +use crate::ops::helpers; + +pub const ID: OpId = OpId("capture_sharpen"); + +pub const AMOUNT: ParamId = ParamId("amount"); +pub const RADIUS: ParamId = ParamId("radius"); +pub const THRESHOLD: ParamId = ParamId("threshold"); + +/// How far out the Gaussian is walked, in standard deviations. +/// +/// Three: beyond that a Gaussian carries under 1.2% of its weight, and the +/// taps cost more than they change. The kernel is normalised by the weights +/// actually summed rather than by an analytic integral, so truncating here +/// costs a slightly narrower effective blur and *not* a brightness shift. +const KERNEL_SIGMAS: f32 = 3.0; + +/// The largest kernel extent, in render pixels, that will be dispatched. +/// +/// Only reachable by zooming past about 16:1, where the ratio climbs above one +/// and a source-pixel radius becomes many render pixels. The cap exists so +/// that a magnification nobody judges sharpening at cannot quietly turn a +/// slider drag into a 200-tap convolution per pass; the price is a Gaussian +/// truncated inside three sigma at those magnifications, which is a slightly +/// tighter blur and nothing else. +const MAX_KERNEL: f32 = 48.0; + +/// The default radius, in source pixels. +/// +/// One photosite. It is what an anti-aliasing filter and a demosaic between +/// them spread a point over on a conventional Bayer sensor, and it is where +/// every editor's capture sharpening starts. +const DEFAULT_RADIUS: f32 = 1.0; + +static DESCRIPTOR: OpDescriptor = OpDescriptor { + id: ID, + label: LocalizedKey("op.capture_sharpen"), + params: &[ + // Amount carries the neutral, which is why it is first: the operation + // is off when this is zero regardless of the other two, so a reset is + // one control and the panel's ordering matches the way it is used. + ParamDescriptor::amount("amount", "param.amount"), + // In **source pixels** — see the module documentation. Half a photosite + // is the smallest radius that means anything on a Bayer sensor, and + // three is already past the point where an unsharp mask is sharpening + // rather than adding local contrast; a photographer wanting the latter + // wants clarity, which is a different operation with a different unit. + ParamDescriptor::scalar( + "radius", + "param.radius", + 0.5, + 3.0, + DEFAULT_RADIUS, + Unit::None, + Scale::Linear, + 2, + ), + // A fraction, but declared as a scalar rather than through + // `ParamDescriptor::fraction` for its precision alone: four decimal + // places on a control whose whole useful travel is a dozen steps + // reads as noise, and invites fiddling with digits that do nothing. + ParamDescriptor::scalar( + "threshold", + "param.threshold", + 0.0, + 1.0, + 0.0, + Unit::None, + Scale::Linear, + 2, + ), + ], + attributes: &[Attribute::Detail], +}; + +/// TRACES: FR-DEV-3 +/// Capture sharpening: a separable unsharp mask with a contrast threshold. +#[derive(Debug, Clone, Copy, PartialEq)] +pub struct CaptureSharpen { + /// −100…100. Negative softens, which is a real request: a lens that + /// out-resolves the sensor, or a frame with moiré, is better served by + /// backing off the capture chain's acutance than by sharpening it. + amount: f32, + /// The Gaussian's standard deviation, **in source pixels**. + radius: f32, + /// Local contrast below which detail is left alone, 0…1. + threshold: f32, +} + +impl Default for CaptureSharpen { + /// Neutral, and a radius already set to something usable. + /// + /// The radius does not start at its minimum, and this is not the usual + /// "neutral means every parameter at zero" rule being broken: neutrality + /// here is `amount == 0`, and a radius has no neutral value at all — a + /// blur of zero width is not the identity, it is a kernel that does not + /// exist. Starting it at one photosite means dragging the amount up gives + /// a sensible result immediately rather than whatever the low end of the + /// slider happens to be. + fn default() -> Self { + Self { + amount: 0.0, + radius: DEFAULT_RADIUS, + threshold: 0.0, + } + } +} + +impl CaptureSharpen { + pub fn new() -> Self { + Self::default() + } + + /// The Gaussian's standard deviation at `scale`, in **render** pixels. + /// + /// The single place the unit conversion happens, and the reason it is a + /// method rather than a line inside [`Self::passes`]: the tests assert + /// against it, and a test that recomputed the conversion would agree with + /// a bug in it. + pub fn sigma(&self, scale: RenderScale) -> f32 { + scale.source_pixels(self.radius) + } + + /// The kernel extent at `scale`, in render pixels — the halo each pass + /// reads, and what [`DetailPass::radius`] has to state. + /// + /// At least one whenever the pass runs at all: a kernel of extent zero + /// reads one tap, its "blur" is the pixel itself, and the high-pass it + /// produces is identically zero. That would be a dispatch that copies the + /// image, which is not what "sharpen a little" should mean. + pub fn kernel(&self, scale: RenderScale) -> u32 { + let extent = (self.sigma(scale) * KERNEL_SIGMAS).ceil(); + extent.clamp(1.0, MAX_KERNEL) as u32 + } + + /// Whether this render is fine enough to show the radius that was chosen. + /// + /// Delegates to [`RenderScale::resolves`] rather than restating the + /// comparison, so that the line between "sharpened" and "not at this size" + /// is drawn in exactly one place in the codebase. + pub fn resolves(&self, scale: RenderScale) -> bool { + scale.resolves(self.radius) + } +} + +impl Operation for CaptureSharpen { + fn descriptor(&self) -> &'static OpDescriptor { + &DESCRIPTOR + } + + fn set_param(&mut self, id: ParamId, value: f32) { + if id == AMOUNT { + self.amount = value; + } else if id == RADIUS { + self.radius = value; + } else if id == THRESHOLD { + self.threshold = value; + } else { + log::warn!("capture_sharpen: unknown parameter {id}"); + } + } + + fn param(&self, id: ParamId) -> f32 { + if id == AMOUNT { + self.amount + } else if id == RADIUS { + self.radius + } else if id == THRESHOLD { + self.threshold + } else { + 0.0 + } + } + + /// Neutral is `amount == 0`, not "every parameter at its default". + /// + /// A radius and a threshold describe *how* to sharpen and say nothing + /// about whether to; moving either one with the amount at zero must leave + /// the photograph untouched and must not make the edit non-neutral, or an + /// unedited file would open reporting itself modified as soon as anyone + /// brushed the radius slider. + fn is_active(&self) -> bool { + self.amount != 0.0 + } + + /// Never called: a detail operation contributes no fused fragment, and + /// [`crate::operation::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 Rec. 709 luminance, which in this stage is not the + /// approximation its own documentation warns about: `_helpers.yaml` notes + /// that the weights are only approximate on camera-space values, and the + /// detail stage runs *after* the camera matrix, in linear sRGB, where they + /// are the definition. + fn helpers(&self) -> &'static [Helper] { + &[helpers::LUMINANCE] + } +} + +impl DetailStage for CaptureSharpen { + fn passes(&self, scale: RenderScale) -> Vec { + if !self.resolves(scale) { + return vec![nothing_to_sharpen()]; + } + + let extent = self.kernel(scale); + // Both halves are the same body and the same uniforms, differing only + // in the axis they walk — and the axis is read from the pass index the + // composer already writes into the base uniform block, so there is one + // kernel here rather than two that can drift apart. + ["horizontal", "vertical"] + .into_iter() + .map(|label| DetailPass { + label, + radius: extent, + uniforms: vec![ + Uniform { + // −100…100 as a gain around zero. A hundred percent is + // a strong capture sharpen and not the ceiling of what + // is useful, which is why the control is the familiar + // photographic amount rather than a 0…1 fraction. + name: "amount", + value: self.amount / 100.0, + }, + Uniform { + name: "sigma", + value: self.sigma(scale), + }, + Uniform { + name: "taps", + value: extent as f32, + }, + Uniform { + // The threshold as a local-contrast fraction. A quarter + // at the top of the slider: past about 25% modulation + // the gate has stopped rejecting noise and started + // rejecting the edges the operation exists to sharpen. + name: "gate", + value: self.threshold * 0.25, + }, + ], + wgsl: BODY.to_string(), + }) + .collect() + } +} + +/// The pass emitted when the radius is finer than a render pixel. +/// +/// One dispatch that changes nothing, rather than an empty chain, and the +/// difference is not stylistic. [`crate::operation::compose_full`] decides +/// from the *operations* — before any resolution is known — that an active +/// detail operation means the fused pass hands on unclipped linear values +/// instead of encoding its own output. If this returned no passes at all, +/// that decision would still stand and nothing downstream would ever perform +/// the output transform: `dr-gpu` would be handed a linear-working shader +/// with an empty chain and refuse it. +/// +/// So the honest "nothing survives at this scale" still has to carry the +/// encode, and one pass that does only that is exactly the resolve step the +/// stage would otherwise need. It costs a single copy of a proxy-sized +/// texture, which is a rounding error against the dispatches around it. +fn nothing_to_sharpen() -> DetailPass { + DetailPass { + label: "unresolved", + // Reads only the pixel it writes, so a tile needs no halo at all. + radius: 0, + uniforms: Vec::new(), + wgsl: "// The chosen radius is finer than one pixel of this render, so the detail +// it would act on is not in this texture — it was lost to the downscale +// before this stage ran (FR-DSP-1). Guessing at it would put sharpening on +// screen that the exported file will not contain, so this pass passes the +// colour through unchanged and the interface is free to say `zoom to 1:1`. +// +// `c` already holds this pixel; leaving it alone is the whole body." + .to_string(), + } +} + +/// One axis of the separable unsharp mask. +/// +/// Emitted verbatim for both passes — see [`DetailStage::passes`] for why the +/// axis is read from a uniform the composer already writes rather than from a +/// second copy of this kernel. +const BODY: &str = r#"// One axis of a separable unsharp mask, applied to luminance. +// +// The composer writes this pass's index into the base uniform block's fourth +// lane precisely so that a two-pass operation need not carry a uniform of its +// own to say which half it is in. Pass 0 walks x, pass 1 walks y. +let axis = select(vec2(0, 1), vec2(1, 0), u.detail_base.w < 0.5); + +// The Gaussian is evaluated here rather than uploaded as a weight table. The +// kernel changes size with the zoom — the radius is in source pixels and the +// ratio is not fixed — so a table would have to be a fixed-length array padded +// to the widest kernel the slider can reach, uploaded per frame, to save an +// `exp` that the hardware does in one instruction. +let extent = i32(taps); +let falloff = 1.0 / (2.0 * sigma * sigma); + +// Sharpening acts on luminance alone. Adding the high-pass to the three +// channels independently sharpens chroma noise into coloured speckle at every +// edge, which is the classic way an unsharp mask ruins a high-ISO frame; and +// the demosaic's interpolation error — the thing being corrected — is a +// luminance error, because that is the channel the CFA samples most densely. +let centre = luminance(c); + +var weighted = 0.0; +var total = 0.0; +for (var i = -extent; i <= extent; i = i + 1) { + let d = f32(i); + let w = exp(-d * d * falloff); + weighted = weighted + w * luminance(tap(coord, axis * i)); + total = total + w; +} + +// Normalised by the weights actually summed, never by an analytic integral. +// The kernel is truncated at three sigma and `tap` clamps at the border, so +// the two disagree — by a fraction of a percent in the middle of the frame and +// by far more along its edge. Dividing by the wrong one would put a bright or +// dark band around the whole photograph, which is invisible on a test pattern +// and perfectly visible on a sky. +let blurred = weighted / total; + +// Everything this axis's blur threw away. Zero on a flat field, so a sky comes +// through untouched at any amount, and the kernel as a whole still sums to one. +let high = centre - blurred; + +// The threshold, as *local contrast* rather than as an absolute difference. +// +// A photographer setting this is saying "modulation this shallow is noise, not +// detail", and that judgement is about the ratio between the detail and the +// tone it sits on: the same sensor noise is a hundred times smaller in linear +// units in a shadow than in a highlight, so an absolute gate calibrated on a +// midtone would leave shadow noise fully sharpened and flatten highlight +// texture. A ratio also makes the control survive the exposure slider, which +// an absolute one would not. +// +// The floor keeps the ratio finite as the local level approaches black. Below +// roughly nine stops down there is nothing but read noise anyway, and without +// it a noise-sized difference divided by a noise-sized level would read as a +// hard edge and be sharpened hardest exactly where it is least wanted. +let level = max(blurred, 0.005); +let contrast = abs(high) / level; + +// A soft knee rather than a step: gating on a comparison would sharpen one +// pixel fully and its neighbour not at all, and the boundary between them is +// itself an edge — visible as a crawling outline around every gently graded +// region. Full suppression below half the gate, full effect above it. +let knee = max(gate, 1e-5); +let keep = select(1.0, smoothstep(knee * 0.5, knee, contrast), gate > 0.0); + +// `sharpened` rather than the obvious `target`: `target` is a WGSL reserved +// keyword, and a fragment that declares one fails to compile against +// *generated* source, so the error names a file nobody wrote. The pipeline +// has been bitten by exactly this once already — see +// `no_fragment_declares_a_wgsl_reserved_keyword` in `lib.rs`, which was added +// the day `let target` broke the contrast fragment. +let sharpened = centre + amount * high * keep; + +// Applied as a gain on all three channels rather than as an offset, so that +// steepening an edge does not drag its colour towards grey: the channel ratios +// are the hue and the saturation, and multiplying leaves them exactly where +// they were. Negative luminance is not a colour, so undershoot stops at black +// — the only clamp in this stage, and it is on the scalar, not on the channels, +// which stay unclipped above one for the output transform to deal with. +// +// Below a nearly-black luminance the ratio stops carrying information — the +// three channels are all noise there and the divisor is meaningless — so the +// pixel is handed on untouched rather than multiplied by whatever fell out. +let scaled = c * (max(sharpened, 0.0) / max(centre, 1e-5)); +c = select(c, scaled, centre > 1e-5);"#; + +#[cfg(test)] +mod tests { + use super::*; + use crate::EditGraph; + use dr_types::ColourSpace; + + /// The develop chain with the sharpener turned up. + /// + /// Built through [`EditGraph`] rather than by reaching into `ops::chain()` + /// directly, so that every value these tests use passes the same clamp a + /// slider's would. A test asserting an exact kernel for a radius the graph + /// would have clipped on the way in is a test of a configuration the + /// photographer cannot reach — the mistake `build.rs` refuses outright for + /// declared nodes, and which nothing catches for a hand-written one. + fn sharpening(amount: f32, radius: f32) -> EditGraph { + let mut graph = EditGraph::default_chain(); + graph.set_param(ID, AMOUNT, amount); + graph.set_param(ID, RADIUS, radius); + graph + } + + fn chain_at(graph: &EditGraph, scale: RenderScale) -> crate::detail::ComposedDetail { + graph.compose_detail_for(scale, ColourSpace::Srgb) + } + + #[test] + fn a_radius_alone_is_not_an_edit() { + // The neutral rule this operation states differently from most: two of + // its three parameters describe *how* to sharpen, and moving them with + // the amount at zero must leave the file unmodified. Otherwise opening + // an image and brushing the radius slider would mark it edited and + // write a sidecar for a photograph nobody changed. + let mut op = CaptureSharpen::new(); + assert!(!op.is_active()); + op.set_param(RADIUS, 3.0); + op.set_param(THRESHOLD, 1.0); + assert!(!op.is_active(), "a radius is not a decision to sharpen"); + op.set_param(AMOUNT, 25.0); + assert!(op.is_active()); + } + + #[test] + fn a_neutral_sharpener_composes_no_passes_at_all() { + // The rule the whole pipeline rests on: an operation at its defaults + // costs nothing. Almost every photograph in a library is unsharpened, + // and none of them should pay a dispatch for it. + let composed = chain_at(&EditGraph::default_chain(), RenderScale::full((512, 512))); + assert!(composed.is_empty()); + assert_eq!(composed.radius(), 0); + } + + #[test] + fn sharpening_is_two_passes_and_only_the_last_one_encodes() { + // Separability, as it reaches the GPU. The first pass writes a linear + // intermediate and the second writes the display texture, so the + // output transform happens exactly once (FR-DEV-2) at the end of the + // chain rather than in the middle of a convolution. + let composed = chain_at(&sharpening(50.0, 1.0), RenderScale::full((512, 512))); + assert_eq!(composed.len(), 2); + + let (first, last) = (&composed.passes[0], &composed.passes[1]); + assert_eq!(first.label, "capture_sharpen/horizontal"); + assert_eq!(last.label, "capture_sharpen/vertical"); + + assert!(!first.writes_output); + assert!(first.source.contains("texture_storage_2d f32 { + let scale = RenderScale::new((render, render), full); + chain_at(&graph, scale).radius() as f32 / scale.ratio() + }; + + let export = in_source_pixels(4000); + let half = in_source_pixels(2000); + let fit = in_source_pixels(1600); + + assert!((export - 9.0).abs() < 0.01, "three sigma of three pixels"); + // Within the rounding of one render pixel back through the ratio, + // which is the whole of the permitted error: the kernel is an integer + // count of render pixels, and one of those is two source pixels on the + // half proxy and two and a half on the fit view. + assert!( + (half - export).abs() <= 2.0, + "the same edit covers {half} source pixels on a half proxy and \ + {export} at export" + ); + assert!( + (fit - export).abs() <= 2.5, + "the same edit covers {fit} source pixels on a 40% view and \ + {export} at export" + ); + + // The other half of the statement, and the one that fails if the unit + // is misread: the kernel in *render* pixels must shrink with the + // render, because that is what keeps it the same size on the picture. + let render_pixels = + |render: u32| chain_at(&graph, RenderScale::new((render, render), full)).radius(); + assert_eq!(render_pixels(4000), 9); + assert_eq!(render_pixels(2000), 5, "ceil(3 sigma of 1.5)"); + assert_eq!(render_pixels(1600), 4, "ceil(3 sigma of 1.2)"); + } + + #[test] + fn a_render_too_coarse_for_the_radius_stops_rather_than_guesses() { + // A one-pixel radius on a quarter-scale proxy is a quarter of a render + // pixel, and no kernel represents that — the frequencies it would act + // on went out with the downscale. `RenderScale::resolves` reports the + // condition and this obeys it, because a preview that shows sharpening + // the exported file will not contain is worse than one that shows none. + let graph = sharpening(100.0, 1.0); + let proxy = RenderScale::new((1000, 1000), (4000, 4000)); + assert!(!proxy.resolves(1.0)); + + let composed = chain_at(&graph, proxy); + // Not empty, though. See `nothing_to_sharpen`: the fused pass has + // already been composed to hand on linear values, so *something* must + // still perform the output transform. + assert_eq!(composed.len(), 1); + assert_eq!(composed.radius(), 0, "it reads no neighbours"); + let pass = &composed.passes[0]; + assert_eq!(pass.label, "capture_sharpen/unresolved"); + assert!(pass.writes_output); + assert!(pass.source.contains("fn encode_output")); + assert!( + !pass.source.contains("for (var i ="), + "the pass-through must not walk a kernel it has decided not to run" + ); + + // Zooming to 1:1 is what brings it back — the view rect shrinks while + // the render target keeps its size — so there is no separate + // full-resolution preview path for a photographer to wait on. + let one_to_one = RenderScale::new((1000, 1000), (1000, 1000)); + assert_eq!(chain_at(&graph, one_to_one).len(), 2); + } + + #[test] + fn the_amount_reaches_the_shader_as_a_gain_and_keeps_its_sign() { + // Negative is not a mistake to be clamped away: a lens that + // out-resolves the sensor, or a frame with moiré, wants the capture + // chain's acutance backed off rather than lifted, and the same kernel + // with a negative gain is exactly that. + let amount_of = |value: f32| -> f32 { + let composed = chain_at(&sharpening(value, 1.0), RenderScale::full((512, 512))); + // Base block first and fixed, then this pass's own, in the order + // `passes` declared them. + composed.passes[0].uniforms[crate::detail::DETAIL_BASE_UNIFORM_FIELDS] + }; + assert!((amount_of(100.0) - 1.0).abs() < 1e-6); + assert!((amount_of(50.0) - 0.5).abs() < 1e-6); + assert!((amount_of(-40.0) + 0.4).abs() < 1e-6); + } + + #[test] + fn the_threshold_is_off_when_it_is_at_zero() { + // The gate is a `smoothstep`, and a `smoothstep` whose two edges meet + // is undefined. The body guards it with a `select` on this uniform + // being positive, so a photographer who never touches the threshold + // gets the plain unsharp mask and not a NaN. + let gate_of = |threshold: f32| -> f32 { + let mut graph = sharpening(50.0, 1.0); + graph.set_param(ID, THRESHOLD, threshold); + let composed = chain_at(&graph, RenderScale::full((512, 512))); + *composed.passes[0].uniforms.last().expect("a gate") + }; + assert_eq!(gate_of(0.0), 0.0); + assert!((gate_of(1.0) - 0.25).abs() < 1e-6); + assert!(chain_at(&sharpening(50.0, 1.0), RenderScale::full((512, 512))).passes[0] + .source + .contains("gate > 0.0")); + } + + #[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 compilation error against source nobody wrote. Both + // shapes this operation emits have to satisfy it — the sharpening pair + // and the pass-through, which carries no uniforms of its own at all. + let scales = [ + RenderScale::full((512, 512)), + RenderScale::new((256, 256), (4096, 4096)), + ]; + for scale in scales { + for pass in chain_at(&sharpening(75.0, 1.0), scale).passes { + assert_eq!(pass.uniforms.len() % 4, 0, "{}", pass.label); + assert!(pass.uniforms.iter().all(|v| v.is_finite()), "{}", pass.label); + } + } + } + + #[test] + fn the_kernel_declares_no_wgsl_reserved_keyword() { + // `lib.rs` runs this check over the fused fragments and cannot reach + // here: a detail pass is a separate shader, composed at a resolution + // that `compose()` never sees. It is worth repeating rather than + // skipping, because the failure it catches is the least legible one in + // this crate — `let target = ...` in the contrast fragment once failed + // with "name `target` is a reserved keyword", pointing at generated + // source rather than at the operation that wrote it, and this body + // reaches for exactly that word. + // + // Not the full reserved list; the words a kernel would plausibly pick + // for a local. The same list `no_fragment_declares_a_wgsl_reserved_keyword` uses, + // kept identical on purpose: two lists that drift apart would let a + // word be safe in one stage and not in the other, which is the sort of + // difference nobody discovers until a shader fails to compile. + const RESERVED: &[&str] = &[ + "target", "sample", "filter", "texture", "buffer", "binding", "const", "enum", "mat", + "vec", "ptr", "ref", "shared", "static", "typedef", "union", "unless", "handle", + "layout", "packed", "premerge", "regardless", "active", "do", "input", "output", + "private", "resource", "restrict", "self", "std", "where", + ]; + + for pass in chain_at(&sharpening(100.0, 2.0), RenderScale::full((512, 512))).passes { + // Only what this operation wrote — the block the composer wraps + // the body in. Its preamble declares `var output` and its tail + // writes through it, and neither is this test's business; scanning + // the whole file would fail on generated code nobody here can fix. + let block = pass + .source + .rsplit_once(" {\n") + .expect("the operation's block opens") + .1; + let body = block + .split_once("\n }\n") + .map_or(block, |(inside, _)| inside); + + // Comments are not declarations, and the kernel deliberately names + // `target` in one to say why it does not use it. Stripping them + // keeps this on the code, so that editing the prose can never fail + // a test about what compiles. + let code: Vec<&str> = body + .lines() + .filter(|l| !l.trim_start().starts_with("//")) + .collect(); + let code = code.join("\n"); + + for keyword in RESERVED { + for form in [format!("let {keyword} "), format!("var {keyword} ")] { + assert!( + !code.contains(&form), + "{} declares `{keyword}`, which is a WGSL reserved keyword", + pass.label + ); + } + } + } + } + + #[test] + fn a_deep_zoom_cannot_turn_a_slider_into_an_unbounded_convolution() { + // Zooming past about 16:1 pushes the ratio above one, and a radius in + // source pixels becomes many render pixels. The cap keeps the cost of + // a magnification nobody judges sharpening at from growing without + // limit; what it costs is a Gaussian truncated inside three sigma, + // which is a slightly tighter blur and no other artefact. + let mut op = CaptureSharpen::new(); + op.set_param(AMOUNT, 100.0); + op.set_param(RADIUS, 3.0); + let deep = RenderScale::new((2000, 2000), (25, 25)); + assert!(deep.ratio() > 16.0); + assert_eq!(op.kernel(deep), MAX_KERNEL as u32); + } +} diff --git a/core/dr-pipeline/src/ops/mod.rs b/core/dr-pipeline/src/ops/mod.rs index 34362cb..413bce3 100644 --- a/core/dr-pipeline/src/ops/mod.rs +++ b/core/dr-pipeline/src/ops/mod.rs @@ -22,10 +22,20 @@ //! reason rather than for want of migrating. The tone curve interpolates //! between five points and its neutral is a *relationship* between them; the //! colour mixer generates thirty-six faceted parameters from twelve computed -//! hue bands; [`vignetting`] carries lens-profile coefficients that are not +//! hue bands; [`capture_sharpen`] is a convolution, and the schema describes a +//! fragment handed a colour with no way back to a coordinate; +//! [`vignetting`] carries lens-profile coefficients that are not //! parameters at all. A schema stretched to cover those would be a worse //! language than Rust, aimed at one caller each. //! +//! # 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. +//! //! 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 @@ -41,12 +51,14 @@ // Hand-written nodes. Each is listed in `ops/` with `rust:`, which is what // places it in the chain; these are the implementations that entry points at. pub mod aberration; +pub mod capture_sharpen; pub mod colour_mixer; pub mod curve; pub mod distortion; pub mod vignetting; pub use aberration::Aberration; +pub use capture_sharpen::CaptureSharpen; pub use colour_mixer::ColourMixer; pub use curve::ToneCurve; pub use distortion::Distortion; diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index 8820974..4ba5b2a 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -1055,11 +1055,20 @@ 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. + /// + /// `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. 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); @@ -1069,8 +1078,29 @@ impl DevelopSession { .then(|| self.masks.as_ref().and_then(|p| p.array())) .flatten(); + // The neighbourhood stage, composed at the size actually being drawn. + // + // It has to be composed *per render* rather than cached with the edit, + // because a kernel is the one thing in this pipeline that is not + // 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. + // + // 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); + 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()) } @@ -1769,7 +1799,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 @@ -1893,7 +1923,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()) @@ -1920,7 +1950,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)) diff --git a/ui/dr-ui/src/labels.rs b/ui/dr-ui/src/labels.rs index f40c076..67ea829 100644 --- a/ui/dr-ui/src/labels.rs +++ b/ui/dr-ui/src/labels.rs @@ -30,6 +30,13 @@ pub fn resolve(key: &str) -> String { "op.vibrance" => "Vibrance".into(), "op.saturation" => "Saturation".into(), "op.colour_mixer" => "Colour Mixer".into(), + // "Sharpening" rather than what `derive` would make of the id. The id + // says *capture* sharpening to separate it from the output sharpening + // an export applies (FR-EXP-4), which is a distinction about where in + // the pipeline it sits; in the develop panel there is only one, and + // "Capture Sharpen" would name a distinction the photographer cannot + // see from there. + "op.capture_sharpen" => "Sharpening".into(), "op.framing" => "Crop & Rotate".into(), // Parameters @@ -134,6 +141,16 @@ mod tests { fn catalogued_keys_resolve_to_their_label() { assert_eq!(resolve("op.white_balance"), "White Balance"); assert_eq!(resolve("param.highlights"), "Highlights"); + // Catalogued precisely because `derive` would get it wrong: the id + // carries a distinction ("capture", as against an export's output + // sharpening) that belongs in the pipeline and not on a panel. + assert_eq!(resolve("op.capture_sharpen"), "Sharpening"); + // Its parameters are the opposite case — the derived words are the + // right words, so they are left uncatalogued and shared with whatever + // asks for an amount or a radius next. + assert_eq!(resolve("param.amount"), "Amount"); + assert_eq!(resolve("param.radius"), "Radius"); + assert_eq!(resolve("param.threshold"), "Threshold"); } #[test]