From b321556dbe2f7479c5dbc65998a0f7afa0b4f24a Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sat, 22 Aug 2026 19:13:00 +0200 Subject: [PATCH] Sharpen the capture with a separable unsharp mask Capture sharpening as a two-pass unsharp mask in the detail stage: blur along x, then along y, each pass applying a one-dimensional high-pass to luminance so the composite preserves a flat field exactly and matches the textbook kernel on any locally one-dimensional edge. The radius is stated in source pixels and converted once per render, so a radius tuned on a fit view is the radius the exported file gets. Below one render pixel the operation declines to draw rather than showing sharpening the file will not contain, and emits a single pass-through that still carries the output transform. The develop session now renders through render_detailed, which is what lets an active neighbourhood operation reach the screen at all. Co-Authored-By: Claude Opus 5 (1M context) --- core/dr-gpu/tests/capture_sharpen.rs | 8 +- core/dr-pipeline/src/ops/capture_sharpen.rs | 156 +++++++++++++------- ui/dr-ui/src/develop.rs | 38 ++++- 3 files changed, 145 insertions(+), 57 deletions(-) diff --git a/core/dr-gpu/tests/capture_sharpen.rs b/core/dr-gpu/tests/capture_sharpen.rs index 4478e6c..1b101ee 100644 --- a/core/dr-gpu/tests/capture_sharpen.rs +++ b/core/dr-gpu/tests/capture_sharpen.rs @@ -217,10 +217,12 @@ fn a_proxy_and_an_export_sharpen_the_same_photograph() { let Some(ctx) = ctx() else { return }; const SOURCE: u32 = 128; let source = step_edge(&ctx, SOURCE, 90, 150); - // Four source pixels, so that even the half-size proxy has a two-pixel - // sigma and resolves the radius — the honest cut-off is tested in + // 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. - let graph = sharpened(100.0, 4.0, 0.0); + // 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 diff --git a/core/dr-pipeline/src/ops/capture_sharpen.rs b/core/dr-pipeline/src/ops/capture_sharpen.rs index 29a0944..c938fe3 100644 --- a/core/dr-pipeline/src/ops/capture_sharpen.rs +++ b/core/dr-pipeline/src/ops/capture_sharpen.rs @@ -494,7 +494,13 @@ let contrast = abs(high) / level; let knee = max(gate, 1e-5); let keep = select(1.0, smoothstep(knee * 0.5, knee, contrast), gate > 0.0); -let target = centre + amount * high * keep; +// `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 @@ -506,29 +512,32 @@ let target = centre + amount * high * keep; // 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(target, 0.0) / max(centre, 1e-5)); +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::detail::compose_detail; + use crate::EditGraph; use dr_types::ColourSpace; - /// The chain with the sharpener turned up, as the composer sees it. - fn sharpening(amount: f32, radius: f32) -> Vec> { - let mut ops = crate::ops::chain(); - for op in &mut ops { - if op.descriptor().id == ID { - op.set_param(AMOUNT, amount); - op.set_param(RADIUS, radius); - } - } - ops + /// 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(ops: &[Box], scale: RenderScale) -> crate::detail::ComposedDetail { - compose_detail(ops, scale, ColourSpace::Srgb) + fn chain_at(graph: &EditGraph, scale: RenderScale) -> crate::detail::ComposedDetail { + graph.compose_detail_for(scale, ColourSpace::Srgb) } #[test] @@ -552,7 +561,7 @@ mod tests { // 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(&crate::ops::chain(), RenderScale::full((512, 512))); + let composed = chain_at(&EditGraph::default_chain(), RenderScale::full((512, 512))); assert!(composed.is_empty()); assert_eq!(composed.radius(), 0); } @@ -636,53 +645,59 @@ mod tests { #[test] fn the_radius_is_a_sensor_length_not_a_viewport_one() { // TRACES: FR-DSP-1 — the decision this operation is most likely to get - // wrong, asserted directly. + // wrong, asserted directly. It is the test the brief asked for: the + // same edit at more than one render resolution has to sharpen the same + // photograph. // - // The same edit, composed at three resolutions of one 4000-pixel - // frame. A radius in source pixels must come out as the same number of - // *source* pixels every time, which means a different number of render - // pixels every time. Read the stored radius as render pixels instead - // and the third assertion below is the one that fails: the kernel - // would be four sigma wide at every size, so the proxy on screen would - // be sharpened four times as hard, relative to the picture, as the file - // that gets exported. - let ops = sharpening(80.0, 4.0); + // Three renders of one 4000-pixel frame: an export, a half-size proxy, + // and a 40% fit view. A radius in source pixels must come out as the + // same number of *source* pixels every time, which means a different + // number of render pixels every time. Read the stored radius as render + // pixels instead and the last three assertions are the ones that fail: + // the kernel would be three sigma wide at every size, so the proxy on + // screen would be sharpened two and a half times as hard, relative to + // the picture, as the file that gets exported. + // + // The radius is the slider's maximum rather than a round number past + // it, because `EditGraph::set_param` clamps and a test asserting on a + // radius the graph would have clipped would be asserting about a + // photograph nobody can produce. + let graph = sharpening(80.0, 3.0); let full = (4000u32, 4000u32); let in_source_pixels = |render: u32| -> f32 { let scale = RenderScale::new((render, render), full); - chain_at(&ops, scale).radius() as f32 / scale.ratio() + chain_at(&graph, scale).radius() as f32 / scale.ratio() }; - // Export, half-size proxy, quarter-size proxy. let export = in_source_pixels(4000); let half = in_source_pixels(2000); - let quarter = in_source_pixels(1000); + let fit = in_source_pixels(1600); - assert!((export - 12.0).abs() < 0.01, "three sigma of four pixels"); + 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 12 does not divide evenly by four. + // 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!( - (quarter - export).abs() <= 4.0, - "the same edit covers {quarter} source pixels on a quarter proxy \ - and {export} at export" + (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(&ops, RenderScale::new((render, render), full)).radius() - }; - assert_eq!(render_pixels(4000), 12); - assert_eq!(render_pixels(2000), 6); - assert_eq!(render_pixels(1000), 3); + 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] @@ -692,11 +707,11 @@ mod tests { // 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 ops = sharpening(100.0, 1.0); + let graph = sharpening(100.0, 1.0); let proxy = RenderScale::new((1000, 1000), (4000, 4000)); assert!(!proxy.resolves(1.0)); - let composed = chain_at(&ops, proxy); + 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. @@ -715,7 +730,7 @@ mod tests { // 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(&ops, one_to_one).len(), 2); + assert_eq!(chain_at(&graph, one_to_one).len(), 2); } #[test] @@ -742,13 +757,9 @@ mod tests { // 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 ops = sharpening(50.0, 1.0); - for op in &mut ops { - if op.descriptor().id == ID { - op.set_param(THRESHOLD, threshold); - } - } - let composed = chain_at(&ops, RenderScale::full((512, 512))); + 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); @@ -777,6 +788,51 @@ mod tests { } } + #[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 convolution 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 composer's own preamble + // declares `var output` and `let coord`, which naga accepts and + // which are not this test's business. + let body = pass + .source + .rsplit_once(" {\n") + .expect("the operation's block") + .1; + for keyword in RESERVED { + for form in [format!("let {keyword} "), format!("var {keyword} ")] { + assert!( + !body.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 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))