diff --git a/crates/pipeline/src/tests.rs b/crates/pipeline/src/tests.rs index 910705497..d7949315e 100644 --- a/crates/pipeline/src/tests.rs +++ b/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 { diff --git a/crates/pipeline/src/tone.rs b/crates/pipeline/src/tone.rs index d4d2d129a..120698501 100644 --- a/crates/pipeline/src/tone.rs +++ b/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.2); + assert!(o < 1.0 && 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); diff --git a/docs/process-versions.md b/docs/process-versions.md index ea31d3950..1da9367a9 100644 --- a/docs/process-versions.md +++ b/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