From 30fea8d34c95faf2af95523e05f5501fa9b014e2 Mon Sep 17 00:00:00 2001 From: Matthew James Sivak Date: Tue, 20 Oct 2026 13:14:16 +0200 Subject: [PATCH] LR-VIEW-CLIPPING: profiled diagnostic views keep the sRGB histogram; GPU equivalence for the clipping overlays On a profiled display the Grey and Sharpening-mask views rendered in the display's primaries, so their histogram counted display-primary pixels, not the photo's sRGB ones. They now render in sRGB like the clipping views (`Overlay::renders_in_srgb`) and are converted for the display after; they show no photo colour, so nothing is lost. The GPU equivalence test now covers Grey (mean/max LSB limits) and the shadow/highlight clipping views (share of differing pixels, as for Visualize Spots: both renderers draw them with the same `visualize::apply`, and a 1-LSB difference at 0/255 flips a pixel). Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/display.rs | 22 +++++++++++++++++----- crates/engine/src/media.rs | 10 +++++----- crates/gpu/tests/equivalence.rs | 21 +++++++++++++++++++++ crates/pipeline/src/visualize.rs | 9 +++++---- 4 files changed, 48 insertions(+), 14 deletions(-) diff --git a/crates/engine/src/display.rs b/crates/engine/src/display.rs index 8a91a6644..518ce54aa 100644 --- a/crates/engine/src/display.rs +++ b/crates/engine/src/display.rs @@ -152,19 +152,31 @@ mod tests { } } - /// ⌥-drag views on a profiled display (issue #937): the histogram stays the photo's (not the - /// view's), and the clipping view is tested and drawn in sRGB, so it stays black and white. + /// ⌥-drag views on a profiled display (issue #937): the histogram stays the photo's sRGB one + /// (not the view's, nor the display-primary pixels'), and the clipping view is tested and + /// drawn in sRGB, so it stays black and white. #[test] fn diagnostic_views_keep_the_photos_histogram() { use lightcraft_pipeline::Overlay; let mut session = crate::Session::with_demo(); let id = session.visible_cloned()[0]; let d = std::sync::Arc::new(p3()); - let mut run = |o| session.loupe_job(id, 320, 240, true).unwrap().with_display(Some(d.clone())).with_overlay(o).run().rendered.unwrap(); - let plain = run(Overlay::None); - let clip = run(Overlay::ShadowClip); + let mut run = |o, d: Option>| { + session.loupe_job(id, 320, 240, true).unwrap().with_display(d).with_overlay(o).run().rendered.unwrap() + }; + let plain = run(Overlay::None, Some(d.clone())); + let clip = run(Overlay::ShadowClip, Some(d.clone())); let (a, b) = (plain.histogram.mean_luma(), clip.histogram.mean_luma()); assert!((a - b).abs() < 0.02, "the photo's histogram: {a} vs {b}"); + // each channel's mean level: the photo's, as a render without a display profile counts it + let means = |h: &Histogram| { + [&h.r, &h.g, &h.b].map(|c| c.iter().enumerate().map(|(i, n)| i as f64 * f64::from(*n)).sum::() / f64::from(h.total.max(1))) + }; + let srgb = means(&run(Overlay::None, None).histogram); + for o in [Overlay::ShadowClip, Overlay::HighlightClip, Overlay::Grey, Overlay::SharpenMask] { + let v = means(&run(o, Some(d.clone())).histogram); + assert!(srgb.iter().zip(v).all(|(p, v)| (p - v).abs() < 0.05), "{o:?}: the photo's sRGB histogram {srgb:?}, got {v:?}"); + } let px = &clip.image.data; // (the profile may round white to 254) let binary = px.iter().filter(|p| p[..3].iter().all(|v| *v <= 2 || *v >= 253)).count(); diff --git a/crates/engine/src/media.rs b/crates/engine/src/media.rs index d04132e0b..e25bd0232 100644 --- a/crates/engine/src/media.rs +++ b/crates/engine/src/media.rs @@ -968,10 +968,10 @@ impl RenderJob { // Thumbnails (many small jobs side by side) stay on the CPU; views and exports use // the GPU when there is one. let gpu = self.cache.is_none(); - // a clipping view tests sRGB values, like the histogram: rendered in sRGB, then - // converted for the display below - let clip_view = self.request.overlay.is_clipping(); - let request = if clip_view { RenderRequest { display: None, ..self.request } } else { self.request }; + // a clipping or grey view: rendered in sRGB (the clipping test and the histogram + // read sRGB values), then converted for the display below + let srgb_view = self.request.overlay.renders_in_srgb(); + let request = if srgb_view { RenderRequest { display: None, ..self.request } } else { self.request }; let (mut rendered, gpu_fallback) = develop_reporting(src, &info, &self.settings, &request, self.stages.as_deref(), gpu); // for a display profile: the image goes to the display's values, the histogram // and the cached view preview stay sRGB @@ -980,7 +980,7 @@ impl RenderJob { // a diagnostic view: the histogram stays the photo's (taken before the overlay // was drawn), and nothing goes to the view cache (Some(d), Some(_)) if self.request.overlay != lightcraft_pipeline::Overlay::None => { - let converted = if clip_view { d.profile.from_srgb(&mut rendered.image) } else { d.profile.correct(&mut rendered.image) }; + let converted = if srgb_view { d.profile.from_srgb(&mut rendered.image) } else { d.profile.correct(&mut rendered.image) }; if let Err(e) = converted { log::warn!("display profile: {e}"); } diff --git a/crates/gpu/tests/equivalence.rs b/crates/gpu/tests/equivalence.rs index c877e3d9d..150df5b9c 100644 --- a/crates/gpu/tests/equivalence.rs +++ b/crates/gpu/tests/equivalence.rs @@ -680,6 +680,27 @@ fn overlays_match() { let req = RenderRequest { overlay: Overlay::Mask { id, view, color: [230, 30, 40], opacity: 50 }, ..RenderRequest::fit(640, 640) }; check(&format!("mask {id} overlay {}", view.name()), &src, &raw, &s, &req); } + // The ⌥-drag views (issue #937): clipping and grey, on an edit that clips both ends. + let mut s = DevelopSettings::default(); + typical(&mut s); + s.light.whites = 60.0; + s.light.blacks = -60.0; + check("grey overlay", &src, &raw, &s, &RenderRequest { overlay: Overlay::Grey, ..RenderRequest::fit(640, 640) }); + // The clipping views test each channel for exactly 0 / 255 (both renderers draw them with + // `visualize::apply` on their 8-bit image): like Visualize Spots, a 1-LSB difference at the + // threshold flips a pixel, so compare the share of differing pixels, and that the views agree + // on what clips. + for (overlay, edge) in [(Overlay::ShadowClip, 0u8), (Overlay::HighlightClip, 255)] { + let req = RenderRequest { overlay, ..RenderRequest::fit(640, 640) }; + let plain = render(&src, &raw, &s, &RenderRequest::fit(640, 640)).image; + let cpu = render(&src, &raw, &s, &req).image; + let gpu = lightcraft_gpu::render(&src, &raw, &s, &req, None).expect("gpu").image; + let differ = cpu.data.iter().zip(&gpu.data).filter(|(a, b)| a != b).count() as f64 / cpu.data.len() as f64; + let clipped = plain.data.iter().filter(|p| p[..3].contains(&edge)).count() as f64 / plain.data.len() as f64; + eprintln!("{overlay:<28?} clipped {:.3}% differing {:.4}%", clipped * 100.0, differ * 100.0); + assert!(clipped > 0.001, "{overlay:?}: the edit clips nothing to compare"); + assert!(differ < 0.002, "{overlay:?}: {differ}"); + } } #[test] diff --git a/crates/pipeline/src/visualize.rs b/crates/pipeline/src/visualize.rs index 3f1ab19fc..af5c2c290 100644 --- a/crates/pipeline/src/visualize.rs +++ b/crates/pipeline/src/visualize.rs @@ -190,10 +190,11 @@ impl Overlay { } } - /// The ⌥-drag clipping views, which test the sRGB values the histogram counts (a display - /// render draws them in sRGB, then converts). - pub fn is_clipping(self) -> bool { - matches!(self, Overlay::ShadowClip | Overlay::HighlightClip) + /// The ⌥-drag views that show none of the photo's colours: the clipping views (which test the + /// sRGB values the histogram counts) and the grey ones. A display render draws them in sRGB, + /// then converts, so their histogram (taken before the overlay) is the photo's sRGB one. + pub fn renders_in_srgb(self) -> bool { + matches!(self, Overlay::ShadowClip | Overlay::HighlightClip | Overlay::SharpenMask | Overlay::Grey) } /// The mask an [`Overlay::Mask`] shows (if it exists in `s` and has components).