Merge PR #1972: Tone: neutral display tone map is the identity up to white (#1945)

crates/pipeline/src/tests.rs

@@ -137,7 +137,8 @@ fn perf_report() {
 
 #[test]
 fn unedited_rendered_source_is_passthrough() {
-    // An sRGB ramp decoded to linear Rec.2020 must render back to (almost) the same 8-bit values.
+    // An sRGB ramp decoded to linear Rec.2020 must render back to the same 8-bit values, white
+    // included (issue #1945: neutral 255, 254 and 253 came back one level darker).
     let src8 = lightcraft_raster::Rgba8::from_fn(256, 4, |x, y| [x as u8, (x as u8).wrapping_add(y as u8 * 40), 255 - x as u8, 255]);
     let m = lightcraft_color::SRGB.to_space(&lightcraft_color::REC2020);
     let src = src8.to_linear().map(|c| m.apply_f32(c));
@@ -148,7 +149,24 @@ fn unedited_rendered_source_is_passthrough() {
             worst = worst.max((a[c] as i32 - b[c] as i32).abs());
         }
     }
-    assert!(worst <= 3, "max error {worst}");
+    assert_eq!(worst, 0, "max error {worst}");
+}
+
+#[test]
+fn unedited_neutral_near_whites_are_passthrough() {
+    // Issue #1945: neutral 255, 254 and 253 rendered one level darker (the display tone map's
+    // shoulder ran at neutral settings), so a white background never exported as 255.
+    let vals = [255u8, 254, 253, 252, 250, 240, 200, 128, 0];
+    let src8 = lightcraft_raster::Rgba8::from_fn(vals.len(), 1, |x, _| {
+        let v = vals[x];
+        [v, v, v, 255]
+    });
+    let m = lightcraft_color::SRGB.to_space(&lightcraft_color::REC2020);
+    let src = src8.to_linear().map(|c| m.apply_f32(c));
+    let r = render(&src, &SourceInfo::default(), &DevelopSettings::default(), &RenderRequest::fit(vals.len(), 1)).image;
+    let got: Vec<[u8; 3]> = r.data.iter().map(|p| [p[0], p[1], p[2]]).collect();
+    let want: Vec<[u8; 3]> = vals.iter().map(|&v| [v, v, v]).collect();
+    assert_eq!(got, want);
 }
 
 fn typical_edits() -> DevelopSettings {

crates/pipeline/src/tone.rs

@@ -186,6 +186,9 @@ impl ToneMap {
         let w = (whites / 100.0) as f32;
         let b = (blacks / 100.0) as f32;
         let m = GREY.powf(1.0 / 2.2);
+        // Neutral sliders are the identity up to white (issue #1945): the SDR shoulder below would
+        // pull 1.0 down to 0.9875 (8-bit 255 to 254), so it only shapes an edited curve.
+        let neutral = c == 0.0 && w == 0.0 && b == 0.0;
         let lut = (0..LUT_N)
             .map(|i| {
                 let ev = LUT_MIN_EV + (LUT_MAX_EV - LUT_MIN_EV) * i as f32 / (LUT_N - 1) as f32;
@@ -203,12 +206,12 @@ impl ToneMap {
                     p += w * 0.12 * 0.5;
                 }
                 let mut o = p.max(0.0).powf(2.2);
-                // short shoulder: slope 1 at 0.95, reaching 1.0 at 1.05
+                // short shoulder (SDR, edited curves): slope 1 at 0.95, reaching 1.0 at 1.05
                 if peak > 1.0 && o > 0.95 {
                     // exponential shoulder: slope 1 at 0.95, asymptote at `peak`
                     let r = peak - 0.95;
                     o = 0.95 + r * (1.0 - (-(o - 0.95) / r).exp());
-                } else if o > 0.95 {
+                } else if o > 0.95 && !neutral {
                     let d = (o - 0.95).min(0.1);
                     o = 0.95 + d - d * d / 0.2;
                 }
@@ -411,10 +414,13 @@ mod tests {
     #[test]
     fn display_identity_and_monotone() {
         let t = ToneMap::display(0.0, 0.0, 0.0);
-        for i in 1..=95 {
+        // up to and including white (issue #1945: 1.0 came out as 0.9875, 8-bit 255 as 254); the
+        // tolerance is the LUT's interpolation next to the clip at 1.0, far below an 8-bit level
+        for i in 1..=100 {
             let y = i as f32 / 100.0;
-            assert!((t.apply(y) - y).abs() < 2e-3, "{y} -> {}", t.apply(y));
+            assert!((t.apply(y) - y).abs() < 1e-3, "{y} -> {}", t.apply(y));
         }
+        assert_eq!(t.apply(4.0), 1.0, "neutral values pushed above white clip");
         for (c, w, b) in [(100.0, 100.0, -100.0), (-100.0, -100.0, 100.0), (60.0, -40.0, 30.0)] {
             let t = ToneMap::display(c, w, b);
             let mut prev = -1.0;
@@ -428,6 +434,17 @@ mod tests {
         assert!(c.apply(0.05) < 0.05 && c.apply(0.7) > 0.7);
     }
 
+    #[test]
+    fn display_shoulder_stays_on_edited_curves() {
+        // The short shoulder is part of the V1 look of edited curves (docs/process-versions.md);
+        // only neutral settings are the identity.
+        for (c, w, b) in [(20.0, 0.0, 0.0), (0.0, 10.0, 0.0), (0.0, 0.0, -5.0)] {
+            let t = ToneMap::display(c, w, b);
+            let o = t.apply(1.0);
+            assert!(o < 0.999 && o > 0.95, "{c} {w} {b}: {o}");
+        }
+    }
+
     #[test]
     fn grey_stays_near_grey_and_highlights_roll_off() {
         let t = ToneMap::new(0.0, 0.0, 0.0);

docs/process-versions.md

@@ -22,6 +22,11 @@ stronger Light sliders, issue #196) without changing a single photo someone has
 - Changes to how a file is decoded or which camera colour it gets change the source, not how settings
   are interpreted; the process doesn't reach that stage today.
 
+**Neutral display tone map** (JPEG, PNG and other display-referred sources with Contrast, Whites and
+Blacks at 0) is the identity up to white: the SDR shoulder no longer pulls 1.0 down to 0.9875 (8-bit
+255 to 254, issue #1945). It landed in place as a bug fix, since the tone map promised identity at
+neutral settings; edited curves keep the shoulder and their V1 look.
+
 ## Adding a process
 
 1. Add a variant to `Process` (`V2`), append it to `Process::ALL` (oldest first), give it its number in