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) <noreply@anthropic.com>

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
+    // The widest radius the slider offers, so that even the half-size proxy
-    // sigma and resolves the radius — the honest cut-off is tested in
+    // 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

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.
+    /// The develop chain with the sharpener turned up.
-    fn sharpening(amount: f32, radius: f32) -> Vec<Box<dyn Operation>> {
+    ///
-        let mut ops = crate::ops::chain();
+    /// Built through [`EditGraph`] rather than by reaching into `ops::chain()`
-        for op in &mut ops {
+    /// directly, so that every value these tests use passes the same clamp a
-            if op.descriptor().id == ID {
+    /// slider's would. A test asserting an exact kernel for a radius the graph
-                op.set_param(AMOUNT, amount);
+    /// would have clipped on the way in is a test of a configuration the
-                op.set_param(RADIUS, radius);
+    /// 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 {
-        ops
+        let mut graph = EditGraph::default_chain();
+        graph.set_param(ID, AMOUNT, amount);
+        graph.set_param(ID, RADIUS, radius);
+        graph
     }
 
-    fn chain_at(ops: &[Box<dyn Operation>], scale: RenderScale) -> crate::detail::ComposedDetail {
+    fn chain_at(graph: &EditGraph, scale: RenderScale) -> crate::detail::ComposedDetail {
-        compose_detail(ops, scale, ColourSpace::Srgb)
+        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
+        // Three renders of one 4000-pixel frame: an export, a half-size proxy,
-        // frame. A radius in source pixels must come out as the same number of
+        // and a 40% fit view. A radius in source pixels must come out as the
-        // *source* pixels every time, which means a different number of render
+        // same number of *source* pixels every time, which means a different
-        // pixels every time. Read the stored radius as render pixels instead
+        // number of render pixels every time. Read the stored radius as render
-        // and the third assertion below is the one that fails: the kernel
+        // pixels instead and the last three assertions are the ones that fail:
-        // would be four sigma wide at every size, so the proxy on screen would
+        // the kernel would be three sigma wide at every size, so the proxy on
-        // be sharpened four times as hard, relative to the picture, as the file
+        // screen would be sharpened two and a half times as hard, relative to
-        // that gets exported.
+        // the picture, as the file that gets exported.
-        let ops = sharpening(80.0, 4.0);
+        //
+        // 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,
+            (fit - export).abs() <= 2.5,
-            "the same edit covers {quarter} source pixels on a quarter proxy \
+            "the same edit covers {fit} source pixels on a 40% view and \
-             and {export} at export"
+             {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| {
+        let render_pixels =
-            chain_at(&ops, RenderScale::new((render, render), full)).radius()
+            |render: u32| chain_at(&graph, RenderScale::new((render, render), full)).radius();
-        };
+        assert_eq!(render_pixels(4000), 9);
-        assert_eq!(render_pixels(4000), 12);
+        assert_eq!(render_pixels(2000), 5, "ceil(3 sigma of 1.5)");
-        assert_eq!(render_pixels(2000), 6);
+        assert_eq!(render_pixels(1600), 4, "ceil(3 sigma of 1.2)");
-        assert_eq!(render_pixels(1000), 3);
     }
 
     #[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);
+            let mut graph = sharpening(50.0, 1.0);
-            for op in &mut ops {
+            graph.set_param(ID, THRESHOLD, threshold);
-                if op.descriptor().id == ID {
+            let composed = chain_at(&graph, RenderScale::full((512, 512)));
-                    op.set_param(THRESHOLD, threshold);
-                }
-            }
-            let composed = chain_at(&ops, 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

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))