From b4ab88b7515e49efa6c4e219ac7fb8fa9e53c847 Mon Sep 17 00:00:00 2001 From: Alex Chambers Date: Sat, 10 Oct 2026 00:39:09 +0200 Subject: [PATCH 1/5] LR-EDIT-LIGHT-HIGHLIGHTS: clipped highlights stay neutral when darkened (#523) Sensor-clipped pixels are rebuilt at raw decode. The tone map rolls them off to white at Neutral, so any colour they carry shows only when Highlights, a negative Exposure or Whites darkens them. They took colour from the nearest unclipped pixel (dark foliage, lens fringes along branches, pixels demosaiced from clipped samples), left two clipped channels at their white-balance-scaled clip levels (red or pink), and were neutral only in camera RGB, not after the colour model (a look fitted to the camera's JPEG, a camera profile). - highlight::reconstruct_with: chromaticity only from bright unclipped content away from edges and from the clip, through a normalised push-pull pyramid; a neutral estimate blended in where reliable content is scarce at every scale up to a quarter of the long edge, applied only as far as the pixel lies past the nearest near-clip unclipped pixels (so a surface still runs into clipping without a step); clipped pairs keep the estimate's ratio at their clip floors; partly clipped pixels fade into the clipped neutral on their way from the first channel clipping to the last; fully clipped pixels become the colour model's neutral. Pixels without a clipped channel are left exactly as they were. - engine: the decode colour step as CameraColourModel, with clipped_white, which keeps the camera's neutral for nearly singular or implausible models. - Tests in raw (continuity along a ramp through every clip, also as lines 1 to 16 rows thick with no reliable colour, on canvases 32 to 1024 rows), engine (pipeline renders at Highlights -100, #523's edit and Exposure -2 through the first clip, over the same sweep; a CC0 ILCE-7RM4 corpus file added to RAW_SAMPLES) and the GPU equivalence suite. - Render cache version 27. Co-Authored-By: Claude Opus 5.5 --- Cargo.lock | 1 + crates/engine/src/files.rs | 289 ++++++++++- crates/engine/src/media.rs | 2 +- crates/gpu/Cargo.toml | 1 + crates/gpu/tests/equivalence.rs | 53 ++ crates/raw/src/highlight.rs | 832 +++++++++++++++++++++----------- docs/parity.md | 2 +- xtask/src/main.rs | 6 + 8 files changed, 892 insertions(+), 294 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 54f556b2d..cc2877aa5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3181,6 +3181,7 @@ dependencies = [ "lightcraft-geom", "lightcraft-pipeline", "lightcraft-raster", + "lightcraft-raw", "lightcraft-scenes", "log", "pollster 1.0.1", diff --git a/crates/engine/src/files.rs b/crates/engine/src/files.rs index db762f009..52177ae5f 100644 --- a/crates/engine/src/files.rs +++ b/crates/engine/src/files.rs @@ -200,6 +200,86 @@ fn preview_reason(bytes: &[u8], e: lightcraft_raw::RawError) -> Result { + /// White-balance multipliers (as shot). + pub wb: [f32; 3], + /// White-balanced camera RGB → linear Rec.2020: the camera matrix, or a look fitted to the camera's JPEG on top. + pub matrix: lightcraft_color::Mat3, + /// Baseline exposure, as a gain. + pub gain: f32, + /// A DNG profile's hue/saturation map and look table. + pub tables: Option<&'a lightcraft_raw::profile::ProfileTables>, + /// A fitted camera look's hue/saturation table. + pub hue_sat: Option<&'a crate::camera_preview::HueSat<'a>>, +} + +impl CameraColourModel<'_> { + /// Rebuild sensor-clipped channels; fully clipped highlights become [`clipped_white`] of the matrix, so they + /// come out of [`Self::apply`] neutral. + pub fn rebuild_highlights(&self, img: &mut Rgb32f) { + lightcraft_raw::highlight::reconstruct_with(img, self.wb, HIGHLIGHT_CLIP, clipped_white(&self.matrix)); + } + + /// White balance, matrix, tables and baseline exposure, in place. + pub fn apply(&self, img: &mut Rgb32f) { + let (wb, m, gain) = (self.wb, self.matrix.to_f32(), self.gain); + img.map_in_place(|p| { + let c = [p[0] * wb[0], p[1] * wb[1], p[2] * wb[2]]; + let rgb = [ + m[0][0] * c[0] + m[0][1] * c[1] + m[0][2] * c[2], + m[1][0] * c[0] + m[1][1] * c[1] + m[1][2] * c[2], + m[2][0] * c[0] + m[2][1] * c[1] + m[2][2] * c[2], + ]; + let rgb = match self.tables { + Some(tables) => tables.apply(rgb, gain), + None => rgb.map(|v| v * gain), + }; + self.hue_sat.map_or(rgb, |h| h.apply(rgb)).map(|v| v.max(0.0)) + }); + } +} + +/// The white-balanced camera RGB that `model` (white-balanced camera RGB → linear Rec.2020) renders +/// neutral, as bright as it renders `[1, 1, 1]`: what fully clipped highlights are rebuilt to, so they come out +/// neutral whatever the colour model does to the camera's own neutral (a look fitted to the camera's JPEG or a +/// camera profile often tints it by a few per cent, which any darkening of the highlights would show). +/// `[1, 1, 1]` (the camera's own neutral, as before this existed) when the model can't be trusted with it: not +/// invertible or nearly so, an answer far from the camera's neutral (a sane colour model moves white by a few per +/// cent), or one that isn't neutral through the `f32` matrix the decode applies. +pub(crate) fn clipped_white(model: &lightcraft_color::Mat3) -> [f32; 3] { + const CAMERA: [f32; 3] = [1.0; 3]; + /// Largest condition number (max row sum norm) of a model trusted for this. + const CONDITION: f64 = 1e3; + /// Largest factor between the answer and the camera's neutral, per channel. + const SPREAD: f64 = 4.0; + let norm = |m: &lightcraft_color::Mat3| m.0.iter().map(|row| row.iter().map(|v| v.abs()).sum::()).fold(0.0, f64::max); + let Some(inverse) = model.inverse() else { return CAMERA }; + let condition = norm(model) * norm(&inverse); + if !condition.is_finite() || condition > CONDITION { + return CAMERA; + } + let luma = |v: [f64; 3]| { + let c = model.apply(v); + (0..3).map(|i| c[i] * f64::from(lightcraft_color::LUMA_2020[i])).sum::() + }; + let n = inverse.apply([1.0; 3]); + let (target, own) = (luma([1.0; 3]), luma(n)); + if !(target > 0.0 && own > 0.0 && target.is_finite() && own.is_finite()) { + return CAMERA; + } + let white = n.map(|v| v * target / own); + if !white.iter().all(|v| v.is_finite() && (1.0 / SPREAD..=SPREAD).contains(v)) { + return CAMERA; + } + let white = white.map(|v| v as f32); + // neutral through the matrix as the decode applies it + let m = model.to_f32(); + let out: [f32; 3] = std::array::from_fn(|i| m[i][0] * white[0] + m[i][1] * white[1] + m[i][2] * white[2]); + let (lo, hi) = (out[0].min(out[1]).min(out[2]), out[0].max(out[1]).max(out[2])); + if lo > 0.0 && hi.is_finite() && hi - lo <= 1e-3 * hi { white } else { CAMERA } +} + /// The largest block size to bin a raw's mosaic by for a source of at most `max_edge` pixels: /// the binned image must keep at least 90 % of `max_edge` (a 16 MP sensor still bins 2× for the /// 2560 px preview). X-Trans can only bin 3× (its 6×6 pattern), and its full demosaic is ~5× the @@ -302,32 +382,24 @@ fn load_bytes_now( let twin = denoise.and_then(|spec| denoised_camera_rgb(&raw, spec, factor)).filter(|d| d.width == img.width && d.height == img.height); // the samples aren't needed any more (the colour model below reads only the tags) raw.data = lightcraft_raw::RawData::U16(Vec::new()); - let wb = t.wb; - let m = camera_look.as_ref().map(|p| p.matrix.mul(&t.matrix)).unwrap_or(t.matrix).to_f32(); let hue_sat = camera_look.as_ref().and_then(|p| p.hue_sat.as_ref()).and_then(crate::camera_preview::HueSat::new); - let gain = 2f32.powf(t.baseline_exposure as f32); // A DNG's own profile look (hue/saturation map, look table), DNG spec chapter 6. let tables = lightcraft_raw::profile::ProfileTables::new(&raw.color.profile, lightcraft_raw::color::illuminant_weight(&raw.color, xy)); + let colour = CameraColourModel { + wb: t.wb, + matrix: camera_look.as_ref().map(|p| p.matrix.mul(&t.matrix)).unwrap_or(t.matrix), + gain: 2f32.powf(t.baseline_exposure as f32), + tables: tables.as_ref(), + hue_sat: hue_sat.as_ref(), + }; // camera RGB → what the pipeline takes: clipped highlights rebuilt, white balance and the colour model applied, // fitted to `max_edge` and upright (the same for the plain and the denoised picture) let finish = |mut img: Rgb32f| { let mut stages = vec![("develop", t0.elapsed())]; stages.push(("transform", t0.elapsed())); - lightcraft_raw::highlight::reconstruct(&mut img, wb, HIGHLIGHT_CLIP); + colour.rebuild_highlights(&mut img); stages.push(("highlights", t0.elapsed())); - img.map_in_place(|p| { - let c = [p[0] * wb[0], p[1] * wb[1], p[2] * wb[2]]; - let rgb = [ - m[0][0] * c[0] + m[0][1] * c[1] + m[0][2] * c[2], - m[1][0] * c[0] + m[1][1] * c[1] + m[1][2] * c[2], - m[2][0] * c[0] + m[2][1] * c[1] + m[2][2] * c[2], - ]; - let rgb = match &tables { - Some(tables) => tables.apply(rgb, gain), - None => rgb.map(|v| v * gain), - }; - hue_sat.as_ref().map_or(rgb, |h| h.apply(rgb)).map(|v| v.max(0.0)) - }); + colour.apply(&mut img); stages.push(("colour", t0.elapsed())); let img = fit(&img, max_edge, max_edge, Filter::Box); stages.push(("fit", t0.elapsed())); @@ -868,6 +940,189 @@ mod tests { assert!((mean(&dim) / mean(&before) - 0.5).abs() < 0.02, "{} vs {}", mean(&dim), mean(&before)); } + /// A look fitted to a camera's JPEG, as `camera_preview` makes them: its rows don't sum to one, so the camera's + /// own white-balanced neutral comes out yellow (blue 12 % low). + fn yellowing_look() -> lightcraft_color::Mat3 { + lightcraft_color::Mat3([[0.42, 0.548, 0.04], [0.246, 0.728, 0.033], [-0.247, 0.679, 0.447]]) + } + + #[test] + fn clipped_white_renders_neutral_through_the_colour_model() { + use lightcraft_color::{LUMA_2020, Mat3}; + let look = yellowing_look(); + let white = clipped_white(&look); + let out = look.apply(white.map(f64::from)); + assert!(out.iter().all(|v| (v - out[1]).abs() < 1e-5), "{white:?} renders {out:?}"); + let luma = |c: [f64; 3]| (0..3).map(|i| c[i] * f64::from(LUMA_2020[i])).sum::(); + assert!((luma(out) - luma(look.apply([1.0; 3]))).abs() < 1e-5, "as bright as the camera's neutral"); + assert_eq!(clipped_white(&Mat3::IDENTITY), [1.0; 3]); + // a model that can't be inverted, or only just, keeps the camera's neutral + assert_eq!(clipped_white(&Mat3([[1.0, 0.0, 0.0], [1.0, 0.0, 0.0], [0.0, 0.0, 1.0]])), [1.0; 3]); + assert_eq!(clipped_white(&Mat3([[1.0, 1.0, 0.0], [1.0, 1.0 + 1e-8, 0.0], [0.0, 0.0, 1.0]])), [1.0; 3]); + // so does one whose white would overflow the rebuilt highlights (2.5e38 in red here) + assert_eq!(clipped_white(&Mat3([[3e-39, 0.0, 0.0], [0.0, 1.0, 0.0], [0.0, 0.0, 1.0]])), [1.0; 3]); + // or move the camera's neutral implausibly far + assert_eq!(clipped_white(&Mat3([[0.1, 0.0, 0.0], [0.0, 1.0, 0.0], [0.0, 0.0, 1.0]])), [1.0; 3]); + // what it answers renders neutral through the f32 matrix, and fully clipped highlights stay finite + for model in [look, Mat3([[1.9, -0.7, -0.2], [-0.3, 1.4, -0.1], [0.0, -0.4, 1.4]])] { + let w = clipped_white(&model); + let m = model.to_f32(); + let out: [f32; 3] = std::array::from_fn(|i| m[i][0] * w[0] + m[i][1] * w[1] + m[i][2] * w[2]); + assert!(out.iter().all(|v| (v - out[1]).abs() <= 1e-3 * out[1]), "{w:?} renders {out:?}"); + let colour = CameraColourModel { wb: [2.5, 1.0, 1.6], matrix: model, gain: 1.0, tables: None, hue_sat: None }; + let mut img = Rgb32f::filled(4, 4, [1.0; 3]); + colour.rebuild_highlights(&mut img); + colour.apply(&mut img); + assert!(img.data.iter().flatten().all(|v| v.is_finite()), "{:?}", img.get(0, 0)); + } + } + + /// Issue #523: fully clipped highlights are neutral in the white-balanced camera RGB, but a colour model that + /// doesn't map that neutral to neutral tinted them. The tone map's roll-off to white hid the tint at Neutral; + /// Highlights -100, or the issue's edit (Exposure -0.88, Highlights -75…), showed it. + #[test] + fn clipped_highlights_stay_neutral_when_darkened() { + use lightcraft_pipeline::{RenderRequest, render}; + let wb = [2.4f32, 1.0, 1.6]; + let colour = CameraColourModel { wb, matrix: yellowing_look(), gain: 1.0, tables: None, hue_sat: None }; + // a grey wall (camera neutral) with a fully clipped light and a light clipped in green only + let mut img = Rgb32f::from_fn(96, 64, |x, y| { + let d = |cx: f32| ((x as f32 - cx).powi(2) + (y as f32 - 32.0).powi(2)).sqrt(); + if d(28.0) < 14.0 { + [1.0; 3] + } else if d(70.0) < 12.0 { + [0.6 / wb[0], 1.0, 0.7 / wb[2]] + } else { + [0.3 / wb[0], 0.3, 0.3 / wb[2]] + } + }); + colour.rebuild_highlights(&mut img); + colour.apply(&mut img); + let info = SourceInfo { raw: true, ..Default::default() }; + let mut issue = lightcraft_develop::DevelopSettings::default(); + let l = &mut issue.light; + (l.exposure, l.contrast, l.highlights, l.shadows, l.whites, l.blacks) = (-0.88, 5.0, -75.0, 52.0, 6.0, -37.0); + let mut h100 = lightcraft_develop::DevelopSettings::default(); + h100.light.highlights = -100.0; + for (name, s) in [("Highlights -100", h100), ("#523's edit", issue)] { + let out = render(&img, &info, &s, &RenderRequest::fit(96, 64)).image; + let p = out.get(28, 32); + assert!(p[1] < 250, "{name}: the clipped light is darkened ({p:?})"); + let spread = p[0].max(p[1]).max(p[2]) - p[0].min(p[1]).min(p[2]); + assert!(spread <= 1, "{name}: clipped light {p:?} is not neutral"); + } + } + + /// A smooth pink gradient (`[0.9k, k, 0.8k]` in the sensor, k from 0.8 to 1.35, white balance `[2.5, 1, 1.6]`) + /// through the point where green clips, 1600 px long, in `rows` of a 1600 × `height` picture that is otherwise + /// near black, rebuilt and rendered at Exposure -2. Returns the rendered colour of the middle row's last + /// unclipped pixel and the next two, and the largest step between neighbouring pixels along that row. + fn darkened_ramp(rows: std::ops::Range, height: usize) -> ([[u8; 4]; 3], u8) { + use lightcraft_pipeline::{RenderRequest, render}; + let wb = [2.5f32, 1.0, 1.6]; + let colour = CameraColourModel { wb, matrix: lightcraft_color::Mat3::IDENTITY, gain: 1.0, tables: None, hue_sat: None }; + let w = 1600usize; + let k = |x: usize| 0.8 + 0.55 * x as f32 / (w - 1) as f32; + let sensor = + Rgb32f::from_fn(w, height, |x, y| if rows.contains(&y) { [0.9 * k(x), k(x), 0.8 * k(x)].map(|v| v.min(1.0)) } else { [0.01; 3] }); + let y = (rows.start + rows.end) / 2; + let first = (0..w).find(|&x| sensor.get(x, y)[1] >= HIGHLIGHT_CLIP).unwrap(); + let mut img = sensor.clone(); + colour.rebuild_highlights(&mut img); + colour.apply(&mut img); + let mut s = lightcraft_develop::DevelopSettings::default(); + s.light.exposure = -2.0; + let out = render(&img, &SourceInfo { raw: true, ..Default::default() }, &s, &RenderRequest::fit(w, height)).image; + let step = |x: usize| { + let (a, b) = (out.get(x - 1, y), out.get(x, y)); + (0..3).map(|c| a[c].abs_diff(b[c])).max().unwrap_or(0) + }; + ([out.get(first - 1, y), out.get(first, y), out.get(first + 1, y)], (1..w).map(step).max().unwrap_or(0)) + } + + /// The darkened pink gradient carries on across the point where green clips: before the fix the fade towards + /// the clipped neutral, driven by how close the other channels were to their clip, made a step of 37 levels + /// there. + #[test] + fn rendered_colour_is_continuous_where_the_first_channel_clips() { + let ([a, b, c], worst) = darkened_ramp(0..16, 16); + let step = |p: [u8; 4], q: [u8; 4]| (0..3).map(|i| p[i].abs_diff(q[i])).max().unwrap_or(0); + assert!(step(a, b) <= 1 && step(b, c) <= 1, "at the first clip: {a:?} {b:?} {c:?}"); + assert!(worst <= 3, "largest step between neighbouring pixels: {worst} levels"); + } + + /// The same gradient as a thin bright line on near black: every pixel of it is next to a strong edge, so there + /// is no reliable colour around it. It still carries on across the first clip, however thin the line and however + /// much dark background there is (a 4-row line on 32 rows used to jump from [225, 126, 153] to [198, 177, 163]; + /// on 512 rows, from [225, 126, 153] to [218, 145, 151]). + #[test] + fn rendered_thin_line_is_continuous_where_the_first_channel_clips() { + let step = |p: [u8; 4], q: [u8; 4]| (0..3).map(|i| p[i].abs_diff(q[i])).max().unwrap_or(0); + for height in [32usize, 128, 512, 1024] { + for width in [1usize, 2, 4, 16] { + let top = (height - width) / 2; + let ([a, b, c], worst) = darkened_ramp(top..top + width, height); + let case = format!("{width} rows of {height}"); + assert!(step(a, b) <= 1 && step(b, c) <= 1, "{case}, at the first clip: {a:?} {b:?} {c:?}"); + assert!(worst <= 3, "{case}: largest step between neighbouring pixels {worst} levels"); + } + } + } + + /// Issue #523 on a public ILCE-7RM4 raw (raw.pixls.us, CC0; skipped without the corpus): a dusk sky clipped in + /// green behind a poplar. Its rebuilt colour used to come from the tree's foliage and lens fringes, a green halo + /// on one side of the tree and a magenta one on the other (Highlights -100 or the issue's edit show them). + #[test] + fn corpus_clipped_sky_beside_a_tree_has_no_coloured_halo() { + let dir = std::env::var_os("LIGHTCRAFT_CORPUS") + .map(std::path::PathBuf::from) + .unwrap_or_else(|| std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("../../corpus")) + .join("raw"); + let Ok(bytes) = std::fs::read(dir.join("arw-sony-a7rm4-14bit-compressed.arw")) else { + eprintln!("skip: {} absent", dir.display()); + return; + }; + let (img, _) = load_bytes(&bytes, 1600).unwrap(); + let (w, h) = (img.width, img.height); + let lum = |p: [f32; 3]| lightcraft_color::luminance_2020(p); + let chroma = |p: [f32; 3]| { + let s = (p[0] + p[1] + p[2]).max(1e-6); + [p[0] / s, p[2] / s] + }; + // the poplar stands at about 30 % of the width, its top 25 to 34 % down the frame + let px = |f: f32, n: usize| (f * n as f32) as usize; + let (mut near, mut far) = ([[0f64; 2]; 2], [[0f64; 2]; 2]); + let mut rows = 0; + for y in px(0.24, h)..px(0.34, h) { + let row = &img.data[y * w..(y + 1) * w]; + let sky = lum(row[px(0.2, w)]); + let dark: Vec = (px(0.25, w)..px(0.35, w)).filter(|&x| lum(row[x]) < 0.5 * sky).collect(); + let (Some(&left), Some(&right)) = (dark.first(), dark.last()) else { continue }; + let k = w as f32 / 6000.0; + let mean = |a: usize, b: usize| { + let n = (b - a) as f64; + (a..b).map(|x| chroma(row[x])).fold([0f64; 2], |s, c| [s[0] + c[0] as f64 / n, s[1] + c[1] as f64 / n]) + }; + let (n0, n1, f0, f1) = (px(8.0 / 6000.0, w).max(2), px(40.0 / 6000.0, w), (200.0 * k) as usize, (320.0 * k) as usize); + for (side, (n, f)) in + [(mean(left - n1, left - n0), mean(left - f1, left - f0)), (mean(right + n0, right + n1), mean(right + f0, right + f1))] + .into_iter() + .enumerate() + { + for c in 0..2 { + near[side][c] += n[c]; + far[side][c] += f[c]; + } + } + rows += 1; + } + assert!(rows > 50, "the tree was not found ({rows} rows)"); + for side in 0..2 { + let d = (0..2).map(|c| ((near[side][c] - far[side][c]) / rows as f64).abs()).fold(0.0, f64::max); + assert!(d < 0.004, "{} of the tree the clipped sky's chromaticity is off by {d:.4}", ["left", "right"][side]); + } + } + /// iPhone ProRAW-style semantic masks drive the Sky mask: Apple's sky matte (here the right /// half of the sensor, i.e. the bottom of the photo once turned 90° clockwise — where the sky /// heuristic would never look) is used; a matte with an unknown name is ignored. diff --git a/crates/engine/src/media.rs b/crates/engine/src/media.rs index 1231221b9..b73d3f1de 100644 --- a/crates/engine/src/media.rs +++ b/crates/engine/src/media.rs @@ -62,7 +62,7 @@ impl SettingsHashes { } /// Bump when the pipeline's output changes, to invalidate cached thumbnails. -pub const RENDER_CACHE_VERSION: u64 = 22; +pub const RENDER_CACHE_VERSION: u64 = 27; /// Thumbnails render at one of these long edges (so window/cell size changes reuse the cache). pub const THUMB_SIZES: [usize; 4] = [128, 256, 384, 512]; diff --git a/crates/gpu/Cargo.toml b/crates/gpu/Cargo.toml index b19acf544..dc348ee88 100644 --- a/crates/gpu/Cargo.toml +++ b/crates/gpu/Cargo.toml @@ -30,3 +30,4 @@ denoise = ["dep:lightcraft-denoise", "lightcraft-denoise/runtime"] [dev-dependencies] lightcraft-scenes = { workspace = true } +lightcraft-raw = { workspace = true } diff --git a/crates/gpu/tests/equivalence.rs b/crates/gpu/tests/equivalence.rs index cb6e7c024..b10836c37 100644 --- a/crates/gpu/tests/equivalence.rs +++ b/crates/gpu/tests/equivalence.rs @@ -406,6 +406,59 @@ fn gpu_matches_cpu() { eprintln!("worst: mean {:.4} LSB, max {} LSB", worst.0, worst.1); } +/// Issue #523: a raw's clipped highlights rebuilt as the raw decode does it (fully clipped ones to the neutral of +/// the colour model applied after them, here a look whose rows don't sum to one) stay neutral when darkened, on +/// the GPU as on the CPU, and the two renders agree. +#[test] +fn clipped_highlights_stay_neutral_on_both() { + if !gpu() { + return; + } + let wb = [2.4f32, 1.0, 1.6]; + let look = lightcraft_color::Mat3([[0.42, 0.548, 0.04], [0.246, 0.728, 0.033], [-0.247, 0.679, 0.447]]); + let luma = |c: [f64; 3]| (0..3).map(|i| c[i] * f64::from(lightcraft_color::LUMA_2020[i])).sum::(); + let n = look.inverse().unwrap().apply([1.0; 3]); + let white = n.map(|v| (v * luma(look.apply([1.0; 3])) / luma(look.apply(n))) as f32); + // a grey wall with a fully clipped light, a light clipped in green only, and a dark bar with a purple fringe + // in front of the second + let mut cam = Rgb32f::from_fn(320, 240, |x, y| { + let d = |cx: f32| ((x as f32 - cx).powi(2) + (y as f32 - 120.0).powi(2)).sqrt(); + if d(90.0) < 50.0 { + [1.0; 3] + } else if (226..232).contains(&x) && (70..170).contains(&y) { + [0.02, 0.03, 0.02] + } else if (x == 225 || x == 232) && (70..170).contains(&y) { + [0.3, 0.25, 0.6] + } else if d(228.0) < 50.0 { + [0.6 / wb[0], 1.0, 0.7 / wb[2]] + } else { + [0.3 / wb[0], 0.3, 0.3 / wb[2]] + } + }); + lightcraft_raw::highlight::reconstruct_with(&mut cam, wb, 0.99, white); + let m = look.to_f32(); + let src = Arc::new(cam.map(|p| { + let c = [p[0] * wb[0], p[1] * wb[1], p[2] * wb[2]]; + std::array::from_fn(|i| (m[i][0] * c[0] + m[i][1] * c[1] + m[i][2] * c[2]).max(0.0)) + })); + let info = SourceInfo { raw: true, ..Default::default() }; + let req = RenderRequest::fit(320, 240); + let mut h100 = DevelopSettings::default(); + h100.light.highlights = -100.0; + let mut issue = DevelopSettings::default(); + let l = &mut issue.light; + (l.exposure, l.contrast, l.highlights, l.shadows, l.whites, l.blacks) = (-0.88, 5.0, -75.0, 52.0, 6.0, -37.0); + for (name, s) in [("clipped highlights -100", h100), ("clipped highlights #523", issue)] { + check(name, &src, &info, &s, &req); + let cpu = render(&src, &info, &s, &req).image; + let gpu = lightcraft_gpu::render(&src, &info, &s, &req, None).expect("gpu render").image; + for (who, img) in [("CPU", &cpu), ("GPU", &gpu)] { + let p = img.get(90, 120); + assert!(p[1] < 250 && p[0].max(p[1]).max(p[2]) - p[0].min(p[1]).min(p[2]) <= 1, "{name} {who}: clipped light {p:?}"); + } + } +} + #[test] fn rendered_sources_and_other_scenes() { if !gpu() { diff --git a/crates/raw/src/highlight.rs b/crates/raw/src/highlight.rs index 1731cb254..950be3afa 100644 --- a/crates/raw/src/highlight.rs +++ b/crates/raw/src/highlight.rs @@ -2,9 +2,18 @@ //! //! - [`clip_neutral`]: white-balance-aware clipping so that sensor-clipped areas render neutral instead of //! magenta/cyan (each channel limited to the smallest white-balanced clip level). -//! - [`reconstruct`]: where only some channels are clipped, rebuild them from the unclipped channels using the -//! local chromaticity of nearby unclipped pixels (diffused into the clipped region with a coarse-to-fine -//! normalised-convolution fill). Fully clipped pixels become neutral at the brightest plausible level. +//! - [`reconstruct`] / [`reconstruct_with`]: where only some channels are clipped, rebuild them from the +//! unclipped channels using the chromaticity of reliable unclipped content nearby (bright, away from edges and +//! from the clipped pixels themselves; diffused into the clipped region coarse to fine), and neutral where there +//! is none within reach. Fully clipped pixels become neutral at the brightest plausible level; partly clipped +//! ones fade into it on their way from the first channel clipping to the last. +//! +//! Why the care: the white-balance gains push clipped channels apart (green clips first, red and blue keep +//! rising), so whatever colour the rebuilt pixels carry is hidden at Neutral only by the tone map's roll-off to +//! white. Any darkening (Highlights, negative Exposure or Whites) brings it out. Two things used to leak colour +//! in: the chromaticity of whatever unclipped pixel happened to be nearest (dark foliage, purple and green +//! fringes along branches, pixels demosaiced from clipped neighbours), and a colour model that does not render +//! the camera's white-balanced neutral as neutral (a look fitted to the camera's JPEG, a camera profile). use lightcraft_raster::Rgb32f; use rayon::prelude::*; @@ -20,119 +29,335 @@ pub fn clip_neutral(img: &mut Rgb32f, wb: [f32; 3], clip: f32) { } } -/// Fill `values` (per-pixel vectors) where `valid` is false from valid neighbours, coarse to fine. -fn fill_invalid(w: usize, h: usize, values: &mut [[f32; 3]], valid: &mut [bool]) { - if w == 0 || h == 0 || valid.iter().all(|&v| v) || !valid.iter().any(|&v| v) { - return; - } - // Build a pyramid of weighted means, fill each level from the next coarser one. - let (cw, ch) = (w.div_ceil(2), h.div_ceil(2)); - let mut cv = vec![[0f32; 3]; cw * ch]; - let mut cval = vec![false; cw * ch]; - { - let (values, valid) = (&*values, &*valid); - downsample(w, h, &mut cv, &mut cval, |i| valid[i].then(|| values[i])); - } - if cw * ch < w * h { - fill_invalid(cw, ch, &mut cv, &mut cval); - } else { - // cannot shrink further (1×1): nothing valid anywhere handled above - return; - } - values.par_chunks_mut(w).zip(valid.par_chunks_mut(w)).enumerate().for_each(|(y, (vrow, okrow))| { - for x in 0..w { - if !okrow[x] { - vrow[x] = upsample(&cv, cw, ch, x, y); - okrow[x] = true; - } - } - }); +/// Unclipped pixels lend their chromaticity in proportion to how close they come to clipping (their largest +/// channel, relative to the clip level): from nothing below `SOURCE_LO` to fully above `SOURCE_HI`. Dark content +/// (foliage, branches, buildings against a clipped sky) says nothing about the sky's colour. +const SOURCE_LO: f32 = 0.25; +const SOURCE_HI: f32 = 0.6; +/// Sources on a strong edge (a 3 × 3 half-resolution neighbour brighter or darker by this many stops) are +/// left out, fading from `EDGE_LO` to `EDGE_HI`: edge pixels mix two surfaces and carry lens fringes. +const EDGE_LO: f32 = 0.15; +const EDGE_HI: f32 = 0.4; +/// Source cells within this many half-resolution cells of a clipped pixel are left out: their channels were +/// demosaiced partly from clipped samples. (At least 1: the same neighbourhood tells which cells clipped +/// pixels sample.) +const MARGIN: usize = 1; +/// Weight of the coarser estimate against a cell's own sources when the pyramid is pushed back down (in units +/// of source density: a cell half covered by full-weight sources counts 0.5). +const PRIOR: f32 = 0.01; +/// The rebuilt colour fades to neutral where reliable content covers less than `SUPPORT` of the surroundings at +/// every scale up to `REACH` of the long edge (so previews and exports agree), gradually, as the coverage of the +/// widest one falls: a clipped sky between bare branches is better white than the colour of a river far away. +const REACH: f32 = 1.0 / 4.0; +const SUPPORT: f32 = 0.05; +/// An unclipped channel above `FLOOR_LO` of its clip level counts more and more as the clip floor it is about to +/// become (see [`reconstruct_with`]). +const FLOOR_LO: f32 = 0.75; +/// Without reliable colour around, a clipped pixel is rebuilt as neutral only as far as it is brighter (or +/// otherwise unlike) the unclipped pixels nearest its clip: the "rim", unclipped pixels above `RIM_LO` of the +/// clip level, weighted towards the clip. Up to `NEAR_RIM` stops past the rim (the rim is an average, a little +/// below the clip) the clipped channels stay at their clip, and over the next `DEPTH` stops they go over to the +/// neutral guess, so a surface runs into clipping without a step whatever its colour; well past it, or where the +/// rim is another surface (its colour differs), the neutral guess applies. +const RIM_LO: f32 = 0.85; +const NEAR_RIM: f32 = 0.03; +const DEPTH: f32 = 0.2; + +#[inline] +fn smoothstep(e0: f32, e1: f32, x: f32) -> f32 { + let t = ((x - e0) / (e1 - e0)).clamp(0.0, 1.0); + t * t * (3.0 - 2.0 * t) } -/// One pyramid step: each `2 × 2` block of a `w × h` level becomes the mean of its valid -/// entries (`at(i)`: the value at index `i` when valid) in `cv`, `cval` (`⌈w/2⌉ × ⌈h/2⌉`). -fn downsample(w: usize, h: usize, cv: &mut [[f32; 3]], cval: &mut [bool], at: impl Fn(usize) -> Option<[f32; 3]> + Sync) { - let cw = w.div_ceil(2); - cv.par_chunks_mut(cw).zip(cval.par_chunks_mut(cw)).enumerate().for_each(|(y, (vrow, okrow))| { +/// One level of a pyramid, `w × h` cells of `N - 1` values and a weight. Pulled: per cell the weighted sums of +/// the values and the weight, as densities (per pixel of the cell). Pushed: the estimated values, and still the +/// weight. The chromaticity pyramid holds red and blue chromaticity, the rim pyramid white-balanced RGB. +struct Level { + w: usize, + h: usize, + cells: Vec<[f32; N]>, +} + +/// Bilinear sample of a level at the centre of pixel `(x, y)` of a grid `scale` times finer. +#[inline] +fn sample(l: &Level, scale: f32, x: usize, y: usize) -> [f32; N] { + let (cv, cw, ch) = (&l.cells, l.w, l.h); + let fx = ((x as f32 + 0.5) / scale - 0.5).clamp(0.0, cw.saturating_sub(1) as f32); + let fy = ((y as f32 + 0.5) / scale - 0.5).clamp(0.0, ch.saturating_sub(1) as f32); + let (x0, y0) = (fx.floor() as usize, fy.floor() as usize); + let (x1, y1) = ((x0 + 1).min(cw.saturating_sub(1)), (y0 + 1).min(ch.saturating_sub(1))); + let (tx, ty) = (fx - x0 as f32, fy - y0 as f32); + let g = |xx: usize, yy: usize| cv.get(yy * cw + xx).copied().unwrap_or([0.0; N]); + let (a, b, c, d) = (g(x0, y0), g(x1, y0), g(x0, y1), g(x1, y1)); + std::array::from_fn(|k| { + let top = a[k] + (b[k] - a[k]) * tx; + let bot = c[k] + (d[k] - c[k]) * tx; + top + (bot - top) * ty + }) +} + +/// The half-resolution source level of `img`: every `2 × 2` block's source-weighted chromaticity sums, with +/// blocks near clipped pixels or on strong edges left out. Also returns, per cell, whether a clipped pixel lies +/// in its row within [`MARGIN`] cells (the vertical half of that test is left to the readers). +fn sources(img: &Rgb32f, wb: [f32; 3], clip: f32) -> (Level<3>, Vec) { + let (w, h) = (img.width, img.height); + let (cw, ch) = (w.div_ceil(2), h.div_ceil(2)); + // per cell: [red sum, blue sum, weight] densities, luminance, whether it holds a clipped pixel + let mut cells = vec![[0f32; 3]; cw * ch]; + let mut lum = vec![0f32; cw * ch]; + let mut near = vec![false; cw * ch]; + cells.par_chunks_mut(cw).zip(lum.par_chunks_mut(cw)).zip(near.par_chunks_mut(cw)).enumerate().for_each(|(y, ((crow, lrow), nrow))| { + let mut touched = vec![false; cw]; + let row = |r: usize| img.data.get(r * w..(r + 1) * w).unwrap_or(&[]); + let (r0, r1) = (row(2 * y), row(2 * y + 1)); + let lo = SOURCE_LO * clip; for x in 0..cw { - let (mut s, mut n) = ([0f32; 3], 0); - for dy in 0..2 { - for dx in 0..2 { - let (sx, sy) = (2 * x + dx, 2 * y + dy); - if sx < w - && sy < h - && let Some(v) = at(sy * w + sx) - { - for c in 0..3 { - s[c] += v[c]; - } - n += 1; - } + let (mut a, mut sum, mut n, mut hit) = ([0f32; 3], 0f32, 0usize, false); + let span = 2 * x..(2 * x + 2).min(w); + for p in r0.get(span.clone()).unwrap_or(&[]).iter().chain(r1.get(span).unwrap_or(&[])) { + let q = [p[0] * wb[0], p[1] * wb[1], p[2] * wb[2]]; + let s = q[0].max(0.0) + q[1].max(0.0) + q[2].max(0.0); + sum += s; + n += 1; + let top = p[0].max(p[1]).max(p[2]); + if top >= clip { + hit = true; + continue; + } + if top > lo && s > 1e-4 { + let b = smoothstep(SOURCE_LO, SOURCE_HI, top / clip) / s; + a[0] += b * q[0].max(0.0); + a[1] += b * q[2].max(0.0); + a[2] += b * s; } } - if n > 0 { - vrow[x] = s.map(|v| v / n as f32); - okrow[x] = true; - } + let k = 1.0 / n.max(1) as f32; + crow[x] = a.map(|v| v * k); + lrow[x] = (sum * k).max(1e-6); + touched[x] = hit; + } + for (x, nr) in nrow.iter_mut().enumerate() { + *nr = touched.get(x.saturating_sub(MARGIN)..=(x + MARGIN).min(cw - 1)).is_some_and(|t| t.iter().any(|&t| t)); } }); + // leave out cells near clipped pixels and on strong edges (luminance ratios to the neighbours) + let (lum, near_ref) = (&lum, &near); + let (edge_lo, edge_hi) = (EDGE_LO.exp2(), EDGE_HI.exp2()); + cells.par_chunks_mut(cw).enumerate().for_each(|(y, row)| { + let (ya, yb) = (y.saturating_sub(MARGIN), (y + MARGIN).min(ch - 1)); + let (ea, eb) = (y.saturating_sub(1), (y + 1).min(ch - 1)); + for (x, cell) in row.iter_mut().enumerate() { + if cell[2] <= 0.0 { + continue; + } + if (ya..=yb).any(|yy| near_ref.get(yy * cw + x).copied().unwrap_or(false)) { + *cell = [0.0; 3]; + continue; + } + let l = lum.get(y * cw + x).copied().unwrap_or(1.0); + let (xa, xb) = (x.saturating_sub(1), (x + 1).min(cw - 1)); + let (mut lo, mut hi) = (l, l); + for yy in ea..=eb { + for &v in lum.get(yy * cw + xa..=yy * cw + xb).unwrap_or(&[]) { + lo = lo.min(v); + hi = hi.max(v); + } + } + let keep = 1.0 - smoothstep(edge_lo, edge_hi, (hi / l).max(l / lo)); + *cell = cell.map(|v| v * keep); + } + }); + (Level { w: cw, h: ch, cells }, near) } -/// Bilinear sample of the `cw × ch` coarse level at fine pixel `(x, y)` (twice the resolution). -fn upsample(cv: &[[f32; 3]], cw: usize, ch: usize, x: usize, y: usize) -> [f32; 3] { - let fx = ((x as f32 + 0.5) / 2.0 - 0.5).clamp(0.0, (cw - 1) as f32); - let fy = ((y as f32 + 0.5) / 2.0 - 0.5).clamp(0.0, (ch - 1) as f32); - let (x0, y0) = (fx.floor() as usize, fy.floor() as usize); - let (x1, y1) = ((x0 + 1).min(cw - 1), (y0 + 1).min(ch - 1)); - let (tx, ty) = (fx - x0 as f32, fy - y0 as f32); - let g = |xx: usize, yy: usize| cv[yy * cw + xx]; - let mut v = [0f32; 3]; - for c in 0..3 { - let top = g(x0, y0)[c] + (g(x1, y0)[c] - g(x0, y0)[c]) * tx; - let bot = g(x0, y1)[c] + (g(x1, y1)[c] - g(x0, y1)[c]) * tx; - v[c] = top + (bot - top) * ty; +/// The quarter-resolution rim level of `img`: every `4 × 4` block's white-balanced RGB of its unclipped pixels +/// above [`RIM_LO`] of the clip level, weighted (steeply) towards the clip, as densities. Pixels next to clipped +/// ones (`margin`, by half-resolution cell) are left out, like the colour sources: they were demosaiced partly from +/// clipped samples, and lens fringes along clipped content would pass for its colour. +fn rim(img: &Rgb32f, wb: [f32; 3], clip: f32, margin: &(dyn Fn(usize, usize) -> bool + Sync)) -> Level<4> { + let (w, h) = (img.width, img.height); + let (cw, ch) = (w.div_ceil(4), h.div_ceil(4)); + let mut cells = vec![[0f32; 4]; cw * ch]; + cells.par_chunks_mut(cw).enumerate().for_each(|(y, row)| { + for (x, cell) in row.iter_mut().enumerate() { + let (mut a, mut n) = ([0f32; 4], 0usize); + for sy in 4 * y..(4 * y + 4).min(h) { + for (i, p) in img.data.get(sy * w + 4 * x..sy * w + (4 * x + 4).min(w)).unwrap_or(&[]).iter().enumerate() { + n += 1; + let top = p[0].max(p[1]).max(p[2]) / clip; + if !(RIM_LO..1.0).contains(&top) || margin((4 * x + i) / 2, sy / 2) { + continue; + } + let k = ((top - RIM_LO) / (1.0 - RIM_LO)).powi(4); + for c in 0..3 { + a[c] += k * p[c] * wb[c]; + } + a[3] += k; + } + } + *cell = a.map(|v| v / n.max(1) as f32); + } + }); + Level { w: cw, h: ch, cells } +} + +/// The next coarser level: each `2 × 2` block's mean densities. +fn pull(l: &Level) -> Level { + let (w, h) = (l.w.div_ceil(2), l.h.div_ceil(2)); + let mut cells = vec![[0f32; N]; w * h]; + cells.par_chunks_mut(w).enumerate().for_each(|(y, row)| { + for (x, cell) in row.iter_mut().enumerate() { + let (mut s, mut n) = ([0f32; N], 0usize); + for (sx, sy) in [(2 * x, 2 * y), (2 * x + 1, 2 * y), (2 * x, 2 * y + 1), (2 * x + 1, 2 * y + 1)] { + if let Some(c) = (sx < l.w && sy < l.h).then(|| l.cells.get(sy * l.w + sx)).flatten() { + for k in 0..N { + s[k] += c[k]; + } + n += 1; + } + } + *cell = s.map(|v| v / n.max(1) as f32); + } + }); + Level { w, h, cells } +} + +/// The estimated values of the cells of `levels[0]` that `needed` marks (all of them when `None`), and of every +/// cell of the coarser levels: each level's own sources blended with the next coarser estimate. The top level is +/// its own sources' weighted mean (normalised: however sparse they are in a large empty picture, the empty part +/// never pulls the estimate anywhere), and `empty` (at zero weight) when it has none at all. +fn push(levels: &mut [Level], empty: [f32; N], needed: Option<&(dyn Fn(usize, usize) -> bool + Sync)>) { + let estimate = |cell: [f32; N], prior: [f32; N]| -> [f32; N] { + let weight = cell[N - 1]; + let d = weight + PRIOR; + std::array::from_fn(|k| if k + 1 == N { weight } else { (cell[k] + PRIOR * prior[k]) / d }) + }; + let Some(last) = levels.last_mut() else { return }; + last.cells.iter_mut().for_each(|c| { + let weight = c[N - 1]; + *c = if weight > 0.0 { std::array::from_fn(|k| if k + 1 == N { weight } else { c[k] / weight }) } else { empty }; + }); + for i in (0..levels.len().saturating_sub(1)).rev() { + let (fine, coarse) = levels.split_at_mut(i + 1); + let (Some(fine), Some(coarse)) = (fine.last_mut(), coarse.first()) else { continue }; + let w = fine.w; + let needed = needed.filter(|_| i == 0); + fine.cells.par_chunks_mut(w).enumerate().for_each(|(y, row)| { + for (x, cell) in row.iter_mut().enumerate() { + if needed.is_some_and(|f| !f(x, y)) { + continue; + } + *cell = estimate(*cell, sample(coarse, 2.0, x, y)); + } + }); } - v } -/// White-balanced chromaticity of an unclipped pixel (`None`: clipped or too dark to tell). -#[inline] -fn chroma_of(p: &[f32; 3], wb: [f32; 3], clip: f32) -> Option<[f32; 3]> { - if p[0] >= clip || p[1] >= clip || p[2] >= clip { +/// The white-balanced colour of a partly clipped pixel (sensor values `p`, white-balanced `q`, clipped channels +/// `cl`) whose colour is estimated at chromaticity `r`: clipped channels rebuilt, unclipped ones kept. `None` +/// when no unclipped channel can tell the level. +fn rebuild(p: [f32; 3], q: [f32; 3], cl: [bool; 3], r: [f32; 3], clip: f32, max_level: f32) -> Option<[f32; 3]> { + // The white-balanced channel sum the unclipped channels imply: their mean, except that a channel about to clip + // counts more and more as the floor it is about to become (continuous with the neighbours where it has). + let (mut sum, mut near_clip, mut k) = (0f32, 0f32, 0usize); + for c in 0..3 { + if !cl[c] && r[c] > 1e-3 { + let s = q[c] / r[c]; + sum += s; + near_clip = near_clip.max(s * smoothstep(FLOOR_LO, 1.0, p[c] / clip)); + k += 1; + } + } + if k == 0 { return None; } - let q = [p[0] * wb[0], p[1] * wb[1], p[2] * wb[2]]; - let s = q[0] + q[1] + q[2]; - (s > 1e-4).then(|| q.map(|v| v.max(0.0) / s)) + // Two clipped channels must not be left at their clip levels: those differ by the white-balance gains (green + // clips first), and the pair would show as magenta or red. They keep the estimate's ratio instead, at the level + // that lifts each to its clip (no channel above the clipped neutral). + let floor = (0..3).filter(|&c| cl[c]).map(|c| q[c] / r[c].max(1e-3)).fold(0.0f32, f32::max); + let top = r[0].max(r[1]).max(r[2]).max(1e-3); + let sum = (sum / k as f32).max(near_clip).max(floor.min(max_level / top)); + Some(std::array::from_fn(|c| if cl[c] { q[c].max(sum * r[c]) } else { q[c] })) +} + +/// Reconstruct partially clipped channels, with a neutral that is neutral in camera RGB. See +/// [`reconstruct_with`]. +pub fn reconstruct(img: &mut Rgb32f, wb: [f32; 3], clip: f32) -> usize { + reconstruct_with(img, wb, clip, [1.0; 3]) } /// Reconstruct partially clipped channels. `clip` is the sensor clip level in normalised units (use slightly -/// below 1.0, e.g. 0.99), `wb` the white-balance multipliers that will be applied afterwards. -/// Returns the number of pixels that had at least one clipped channel. -/// -/// The chromaticity field is diffused coarse to fine from the unclipped pixels. A clipped pixel is -/// never valid at full resolution, so its chromaticity is always the bilinear sample of the -/// half-resolution level: we start the pyramid there, straight from the image, and never build -/// the full-resolution field (same result, a quarter of the memory and far less work). -pub fn reconstruct(img: &mut Rgb32f, wb: [f32; 3], clip: f32) -> usize { +/// below 1.0, e.g. 0.99), `wb` the white-balance multipliers that will be applied afterwards, `white` the +/// white-balanced camera RGB that the colour model applied afterwards renders neutral (`[1, 1, 1]` for a model +/// that maps the white-balanced neutral to neutral), scaled to the brightness that `[1, 1, 1]` renders at. +/// Fully clipped pixels become `white` times the brightest plausible level; partly clipped ones take the +/// chromaticity of reliable unclipped content nearby, and that of `white` where there is none. +/// Returns the number of pixels that had at least one clipped channel; the others are left exactly as they were. +pub fn reconstruct_with(img: &mut Rgb32f, wb: [f32; 3], clip: f32, white: [f32; 3]) -> usize { let (w, h) = (img.width, img.height); let is_clipped = |p: &[f32; 3]| p[0] >= clip || p[1] >= clip || p[2] >= clip; let count = img.data.par_iter().filter(|p| is_clipped(p)).count(); - if count == 0 { + if count == 0 || w == 0 || h == 0 { return 0; } - // chromaticity (white-balanced ratios to the channel sum) of unclipped pixels, at half resolution + // a white far from the camera's neutral (or not a number) is a broken colour model: keep the camera's + let plausible = white.iter().all(|v| v.is_finite() && (1.0 / 16.0..=16.0).contains(v)); + let white = if plausible { white } else { [1.0; 3] }; + let total = white[0] + white[1] + white[2]; + let neutral = [white[0] / total, white[1] / total, white[2] / total]; + // A clipped pixel is never a source, so its chromaticity is always the bilinear sample of the + // half-resolution estimate: the pyramid starts there, straight from the image. + let (first, near) = sources(img, wb, clip); + let mut levels = vec![first]; + while let Some(l) = levels.last().filter(|l| l.w > 1 || l.h > 1) { + let next = pull(l); + levels.push(next); + } + // a clipped pixel samples the cells within one of its own, which are within one of a clipped cell let (cw, ch) = (w.div_ceil(2), h.div_ceil(2)); - let mut cv = vec![[1f32 / 3.0; 3]; cw * ch]; - let mut cval = vec![false; cw * ch]; - { - let data = &img.data; - downsample(w, h, &mut cv, &mut cval, |i| chroma_of(&data[i], wb, clip)); + let needed = |x: usize, y: usize| (y.saturating_sub(1)..=(y + 1).min(ch - 1)).any(|yy| near.get(yy * cw + x).copied().unwrap_or(false)); + push(&mut levels, [neutral[0], neutral[2], 0.0], Some(&needed)); + // How much reliable content lies around each needed cell: its density at any scale up to [`REACH`] (level + // i's cells span 2^i cells of level 0). Near reliable content some fine level is dense with it; far from it + // only the widest one may still hold a little, and the trust fades with that. Kept in the third entry of the + // level 0 estimates, which clipped pixels sample with their colour. + let reach = REACH * w.max(h) as f32 / 2.0; + let widest = (0..levels.len()).find(|&i| (1u64 << i.min(62)) as f32 >= reach).unwrap_or(levels.len().saturating_sub(1)); + if let Some((first, coarser)) = levels.split_first_mut() { + let support: Vec<(&Level<3>, f32)> = coarser.iter().take(widest).enumerate().map(|(i, l)| (l, (1u64 << (i + 1).min(62)) as f32)).collect(); + let fw = first.w; + first.cells.par_chunks_mut(fw).enumerate().for_each(|(y, row)| { + for (x, cell) in row.iter_mut().enumerate() { + if !needed(x, y) { + continue; + } + let mut t = 0f32; + for &(l, scale) in &support { + t = t.max(smoothstep(0.0, SUPPORT, sample(l, scale, x, y)[2])); + if t >= 1.0 { + break; + } + } + cell[2] = t; + } + }); } - if cw * ch < w * h { - fill_invalid(cw, ch, &mut cv, &mut cval); - } - let any_valid = cw * ch < w * h && cval.iter().any(|&v| v); + let Some(field) = levels.first() else { return count }; + // the rim, only where some clipped pixel lacks reliable colour (level 0 of its pyramid holds the estimate) + let untrusted = field.cells.par_iter().enumerate().any(|(i, c)| c[2] < 1.0 && needed(i % field.w, i / field.w)); + let rims = untrusted + .then(|| { + let mut levels = vec![rim(img, wb, clip, &needed)]; + while let Some(l) = levels.last().filter(|l| l.w > 1 || l.h > 1) { + let next = pull(l); + levels.push(next); + } + push(&mut levels, [0.0; 4], None); + levels + }) + // a rim anywhere at all (else the neutral guess applies throughout) + .filter(|levels| levels.last().is_some_and(|top| top.cells.iter().any(|c| c[3] > 0.0))); + let rim_field = rims.as_ref().and_then(|l| l.first()); let max_level = wb.iter().cloned().fold(0.0f32, f32::max) * clip; img.data.par_chunks_mut(w).enumerate().for_each(|(y, row)| { for (x, px) in row.iter_mut().enumerate() { @@ -141,27 +366,45 @@ pub fn reconstruct(img: &mut Rgb32f, wb: [f32; 3], clip: f32) -> usize { if !cl.iter().any(|&b| b) { continue; } - let r = if any_valid { upsample(&cv, cw, ch, x, y) } else { [1f32 / 3.0; 3] }; let q = [p[0] * wb[0], p[1] * wb[1], p[2] * wb[2]]; - // estimate the white-balanced channel sum from the unclipped channels - let (mut sum, mut k) = (0f32, 0usize); - for c in 0..3 { - if !cl[c] && r[c] > 1e-3 { - sum += q[c] / r[c]; - k += 1; - } - } - let mut out = q; - if k == 0 { - let v = q.iter().cloned().fold(0.0f32, f32::max).max(max_level); - out = [v; 3]; - } else { - let sum = sum / k as f32; - for c in 0..3 { - if cl[c] { - out[c] = q[c].max(sum * r[c]); - } + // rebuilt with the colour of the reliable content around, and with neutral, blended by how much of it + // there is (blending the results, not the colours, keeps the estimate consistent with the pixel's own + // unclipped channels all the way) + let e = sample(field, 2.0, x, y); + let trust = e[2].clamp(0.0, 1.0); + let local = (trust > 0.0).then(|| rebuild(p, q, cl, [e[0], 1.0 - e[0] - e[1], e[1]], clip, max_level)).flatten(); + let plain = (trust < 1.0).then(|| rebuild(p, q, cl, neutral, clip, max_level)).flatten().map(|b| { + // only as far as the pixel lies past the rim nearest its clip (in every unclipped channel) + let Some(rim) = rim_field else { return b }; + let edge = sample(rim, 4.0, x, y); + let past = (0..3) + .filter(|&c| !cl[c]) + .map(|c| if edge[c] > 1e-6 && q[c] > 1e-6 { (q[c] / edge[c]).log2().abs() } else { f32::INFINITY }) + .fold(0.0f32, f32::max); + let k = smoothstep(NEAR_RIM, NEAR_RIM + DEPTH, past); + std::array::from_fn(|c| q[c] + (b[c] - q[c]) * k) + }); + let mut out = match (local, plain) { + (Some(a), Some(b)) => std::array::from_fn(|c| b[c] + (a[c] - b[c]) * trust), + (Some(a), None) | (None, Some(a)) => a, + (None, None) => { + let v = q.iter().cloned().fold(0.0f32, f32::max).max(max_level); + white.map(|c| c * v) } + }; + // Fade into the fully clipped neutral on the way from the first channel clipping (no change: continuous + // with the unclipped neighbours) to the last one clipping (all of it: continuous with the fully clipped + // ones). The way is measured in stops: how far the rebuilt clipped channels lie above their clip, + // against how far the lowest unclipped channel still lies below its own. + let above = (0..3).filter(|&c| cl[c]).map(|c| (out[c] / q[c].max(1e-9)).max(1.0).log2()).fold(0.0f32, f32::max); + let lowest = (0..3).filter(|&c| !cl[c]).map(|c| p[c]).fold(f32::MAX, f32::min); + let below = (clip / lowest.max(1e-6)).log2().max(0.0); + let t = if above > 0.0 && lowest < clip { smoothstep(0.5, 1.0, above / (above + below)) } else { 0.0 }; + if t > 0.0 { + let level = (out[0] + out[1] + out[2]) / total; + let full = q.iter().cloned().fold(0.0f32, f32::max).max(max_level); + let v = level + (full - level) * t; + out = std::array::from_fn(|c| out[c] + (white[c] * v - out[c]) * t); } *px = [out[0] / wb[0], out[1] / wb[1], out[2] / wb[2]]; } @@ -173,154 +416,170 @@ pub fn reconstruct(img: &mut Rgb32f, wb: [f32; 3], clip: f32) -> usize { mod tests { use super::*; - // The previous full-resolution implementation, kept as the reference for `reconstruct`. - /// Fill `values` (per-pixel vectors) where `valid` is false from valid neighbours, coarse to fine. - fn fill_invalid_ref(w: usize, h: usize, values: &mut [[f32; 3]], valid: &mut [bool]) { - if w == 0 || h == 0 || valid.iter().all(|&v| v) || !valid.iter().any(|&v| v) { - return; - } - // Build a pyramid of weighted means, fill each level from the next coarser one. - let (cw, ch) = (w.div_ceil(2), h.div_ceil(2)); - let mut cv = vec![[0f32; 3]; cw * ch]; - let mut cval = vec![false; cw * ch]; - { - let (values, valid) = (&*values, &*valid); - cv.par_chunks_mut(cw).zip(cval.par_chunks_mut(cw)).enumerate().for_each(|(y, (vrow, okrow))| { - for x in 0..cw { - let (mut s, mut n) = ([0f32; 3], 0); - for dy in 0..2 { - for dx in 0..2 { - let (sx, sy) = (2 * x + dx, 2 * y + dy); - if sx < w && sy < h && valid[sy * w + sx] { - for c in 0..3 { - s[c] += values[sy * w + sx][c]; - } - n += 1; - } - } - } - if n > 0 { - vrow[x] = s.map(|v| v / n as f32); - okrow[x] = true; - } - } - }); - } - if cw * ch < w * h { - fill_invalid_ref(cw, ch, &mut cv, &mut cval); - } else { - // cannot shrink further (1×1): nothing valid anywhere handled above - return; - } - values.par_chunks_mut(w).zip(valid.par_chunks_mut(w)).enumerate().for_each(|(y, (vrow, okrow))| { - for x in 0..w { - if !okrow[x] { - // bilinear upsample of the coarse level - let fx = ((x as f32 + 0.5) / 2.0 - 0.5).clamp(0.0, (cw - 1) as f32); - let fy = ((y as f32 + 0.5) / 2.0 - 0.5).clamp(0.0, (ch - 1) as f32); - let (x0, y0) = (fx.floor() as usize, fy.floor() as usize); - let (x1, y1) = ((x0 + 1).min(cw - 1), (y0 + 1).min(ch - 1)); - let (tx, ty) = (fx - x0 as f32, fy - y0 as f32); - let g = |xx: usize, yy: usize| cv[yy * cw + xx]; - let mut v = [0f32; 3]; - for c in 0..3 { - let top = g(x0, y0)[c] + (g(x1, y0)[c] - g(x0, y0)[c]) * tx; - let bot = g(x0, y1)[c] + (g(x1, y1)[c] - g(x0, y1)[c]) * tx; - v[c] = top + (bot - top) * ty; - } - vrow[x] = v; - okrow[x] = true; - } - } - }); + /// Daylight-like multipliers: green clips first, red and blue keep rising. + const WB: [f32; 3] = [2.5, 1.0, 1.6]; + + fn balanced(p: [f32; 3]) -> [f32; 3] { + [p[0] * WB[0], p[1] * WB[1], p[2] * WB[2]] } - /// Reconstruct partially clipped channels. `clip` is the sensor clip level in normalised units (use slightly - /// below 1.0, e.g. 0.99), `wb` the white-balance multipliers that will be applied afterwards. - /// Returns the number of pixels that had at least one clipped channel. - fn reconstruct_ref(img: &mut Rgb32f, wb: [f32; 3], clip: f32) -> usize { - let (w, h) = (img.width, img.height); - let n = w * h; - let is_clipped = |p: &[f32; 3]| p[0] >= clip || p[1] >= clip || p[2] >= clip; - let count = img.data.par_iter().filter(|p| is_clipped(p)).count(); - if count == 0 { - return 0; - } - // chromaticity (white-balanced ratios to the channel mean) of unclipped pixels - let mut chroma = vec![[1f32 / 3.0; 3]; n]; - let mut valid = vec![false; n]; - chroma.par_iter_mut().zip(valid.par_iter_mut()).zip(img.data.par_iter()).for_each(|((r, v), p)| { - if is_clipped(p) { - return; - } - let q = [p[0] * wb[0], p[1] * wb[1], p[2] * wb[2]]; - let s = q[0] + q[1] + q[2]; - if s > 1e-4 { - *r = q.map(|v| v.max(0.0) / s); - *v = true; - } - }); - fill_invalid_ref(w, h, &mut chroma, &mut valid); - let max_level = wb.iter().cloned().fold(0.0f32, f32::max) * clip; - img.data.par_iter_mut().zip(chroma.par_iter()).for_each(|(px, r)| { - let p = *px; - let cl = [p[0] >= clip, p[1] >= clip, p[2] >= clip]; - if !cl.iter().any(|&b| b) { - return; - } - let q = [p[0] * wb[0], p[1] * wb[1], p[2] * wb[2]]; - // estimate the white-balanced channel sum from the unclipped channels - let (mut sum, mut k) = (0f32, 0usize); - for c in 0..3 { - if !cl[c] && r[c] > 1e-3 { - sum += q[c] / r[c]; - k += 1; - } - } - let mut out = q; - if k == 0 { - let v = q.iter().cloned().fold(0.0f32, f32::max).max(max_level); - out = [v; 3]; + fn sensor(q: [f32; 3]) -> [f32; 3] { + [q[0] / WB[0], q[1] / WB[1], q[2] / WB[2]].map(|v| v.min(1.0)) + } + + fn chroma(q: [f32; 3]) -> [f32; 3] { + let s = q[0] + q[1] + q[2]; + q.map(|v| v / s) + } + + /// A pale blue sky whose green channel clipped, an unclipped strip of the same sky along the top, and a dark + /// branch with purple fringes on both sides standing in the clipped part (white-balanced sky colour returned). + fn branch_scene() -> (Rgb32f, [f32; 3]) { + let sky = [1.10, 1.25, 1.45]; + let img = Rgb32f::from_fn(192, 128, |x, y| { + sensor(if y < 24 { + sky.map(|v| v * 0.7) + } else if (90..100).contains(&x) && y >= 40 { + [0.06, 0.05, 0.03] + } else if (x == 89 || x == 100) && y >= 40 { + [0.75, 0.25, 0.96] } else { - let sum = sum / k as f32; - for c in 0..3 { - if cl[c] { - out[c] = q[c].max(sum * r[c]); - } - } + sky + }) + }); + (img, sky) + } + + /// Issue #523: the clipped sky beside a branch took the colour of the branch's fringes and of the branch + /// itself (magenta, green), which any darkening of the highlights then showed. + #[test] + fn fringes_and_dark_branches_do_not_tint_a_clipped_sky() { + let (mut img, sky) = branch_scene(); + assert!(reconstruct(&mut img, WB, 0.99) > 0); + let want = chroma(sky); + let mut worst = 0f32; + for y in 40..128 { + for x in [84, 86, 88, 101, 103, 105] { + let got = chroma(balanced(img.get(x, y))); + worst = worst.max((0..3).map(|c| (got[c] - want[c]).abs()).fold(0.0, f32::max)); } - *px = [out[0] / wb[0], out[1] / wb[1], out[2] / wb[2]]; - }); - count + } + assert!(worst < 0.01, "clipped sky beside the branch is off the sky's colour by {worst}"); } - /// `cargo test --release -p lightcraft-raw --lib -- --ignored bench_reconstruct --nocapture` + /// A warm light with red and green clipped: left at their clip levels the pair reads red-magenta (the white + /// balance multiplies red's clip level by 2.5, green's by 1). #[test] - #[ignore] - fn bench_reconstruct() { - let img = Rgb32f::from_fn(6000, 4000, |x, y| { - let v = 0.4 + 0.8 * ((x as f32 * 0.003).sin() * (y as f32 * 0.002).cos()).abs(); - [v.min(1.0), (v * 0.7).min(1.0), (v * 0.5).min(1.0)] + fn two_clipped_channels_are_not_left_at_their_clip_levels() { + let light = [3.5, 2.6, 0.8]; + let mut img = Rgb32f::from_fn(64, 64, |x, y| { + let d = ((x as f32 - 32.0).powi(2) + (y as f32 - 32.0).powi(2)).sqrt(); + if d < 20.0 { sensor(light) } else { [0.02; 3] } }); - let wb = [2.0, 1.0, 1.5]; - let time = |f: &dyn Fn(&mut Rgb32f) -> usize| { - (0..3) - .map(|_| { - let mut i = img.clone(); - let t = std::time::Instant::now(); - f(&mut i); - t.elapsed().as_secs_f64() * 1e3 - }) - .fold(f64::MAX, f64::min) - }; - let new = time(&|i| reconstruct(i, wb, 0.99)); - let old = time(&|i| reconstruct_ref(i, wb, 0.99)); - eprintln!("reconstruct 24 MP: {new:.0} ms (full-resolution reference: {old:.0} ms)"); + reconstruct(&mut img, WB, 0.99); + let q = balanced(img.get(32, 32)); + assert!((q[2] - 0.8).abs() < 0.1, "the unclipped channel is about kept: {q:?}"); + let ratio = q[1] / q[0]; + assert!((0.6..=1.05).contains(&ratio), "green / red {ratio} ({q:?}): not a warm white"); } - /// The half-resolution start gives exactly the full-resolution algorithm's result. #[test] - fn matches_the_full_resolution_reference() { + fn fully_clipped_pixels_follow_the_colour_model_white() { + let white = [1.03, 0.98, 1.06]; + let mut img = Rgb32f::filled(8, 8, [1.0; 3]); + img.data[0] = [0.3, 0.6, 0.4]; + reconstruct_with(&mut img, WB, 0.99, white); + let q = balanced(img.get(4, 4)); + let k = q[1] / white[1]; + assert!((0..3).all(|c| (q[c] - white[c] * k).abs() < 1e-4 * k), "{q:?} is not along {white:?}"); + assert!(k >= 2.5 * 0.99 - 1e-4, "clipped neutral at least as bright as the highest clip level: {k}"); + assert_eq!(img.data[0], [0.3, 0.6, 0.4]); + // a white that cannot be right falls back to camera neutral, and nothing overflows + for bad in [[f32::NAN, 1.0, 1.0], [2.5e38, 0.74, 0.74], [1.0, 0.0, 1.0], [40.0, 1.0, 1.0]] { + let mut img = Rgb32f::filled(4, 4, [1.0; 3]); + img.data[0] = [1.0, 1.0, 0.5]; + reconstruct_with(&mut img, WB, 0.99, bad); + let q = balanced(img.get(1, 1)); + assert!((q[0] - q[1]).abs() < 1e-4 && (q[2] - q[1]).abs() < 1e-4, "{bad:?}: {q:?}"); + assert!(img.data.iter().flatten().all(|v| v.is_finite()), "{bad:?}"); + } + } + + /// Red rises through its clip level across a sky whose green and blue clipped: the rebuilt colour changes + /// smoothly (no edge where red clips too) and arrives at the clipped neutral. + #[test] + fn rebuilt_colour_is_continuous_as_more_channels_clip() { + let blue = [1.0, 1.3, 1.9]; + let img0 = Rgb32f::from_fn(240, 40, |x, y| { + if y < 8 { + return sensor(blue.map(|v| v * 0.5)); // unclipped sky: the source of colour + } + let level = 0.6 + 2.2 * x as f32 / 239.0; // red at 0.24 to 1.12 of its clip level + sensor([level * WB[0] * 0.4, 1.6, 2.0]) + }); + let mut img = img0.clone(); + reconstruct(&mut img, WB, 0.99); + let row: Vec<[f32; 3]> = (0..240).map(|x| chroma(balanced(img.get(x, 30)))).collect(); + let step = row.windows(2).map(|w| (0..3).map(|c| (w[1][c] - w[0][c]).abs()).fold(0.0, f32::max)).fold(0.0, f32::max); + assert!(step < 0.01, "largest chromaticity step between neighbours {step}"); + let end = row[239]; + assert!(end.iter().all(|v| (v - 1.0 / 3.0).abs() < 1e-3), "fully clipped end {end:?}"); + } + + /// A smooth pink ramp (`[0.9k, k, 0.8k]`, k from 0.8 to 1.35) whose green clips first, then red, then blue, + /// in `rows` of a `h`-row picture that is otherwise near black: the rebuilt middle row, and where green first + /// clips on it. + fn rebuilt_ramp(rows: std::ops::Range, h: usize) -> (Vec<[f32; 3]>, usize) { + let (w, k0, k1) = (1600usize, 0.8f32, 1.35f32); + let k = |x: usize| k0 + (k1 - k0) * x as f32 / (w - 1) as f32; + let img0 = Rgb32f::from_fn(w, h, |x, y| if rows.contains(&y) { [0.9 * k(x), k(x), 0.8 * k(x)].map(|v| v.min(1.0)) } else { [0.01; 3] }); + let mut img = img0.clone(); + reconstruct(&mut img, WB, 0.99); + let y = (rows.start + rows.end) / 2; + let row: Vec<[f32; 3]> = (0..w).map(|x| balanced(img.get(x, y))).collect(); + (row, (0..w).find(|&x| img0.get(x, y)[1] >= 0.99).unwrap_or(0)) + } + + /// No channel changes by more than `at_clip` (relative) where green first clips, nor by 3 % anywhere. + fn assert_continuous(row: &[[f32; 3]], first: usize, at_clip: f32, case: &str) { + // across the first clip the colour carries on as it was (the sensor's own colour, not a fade) + let (a, b) = (row[first - 1], row[first]); + assert!((0..3).all(|c| (b[c] / a[c] - 1.0).abs() < at_clip), "{case}, first clip: {a:?} then {b:?}"); + let worst = row.windows(2).map(|p| (0..3).map(|c| (p[1][c] / p[0][c] - 1.0).abs()).fold(0.0, f32::max)).fold(0.0, f32::max); + // (a smooth ramp moves 0.04 % per pixel here; the rebuilt colour turns faster where it leaves the colour of + // the unclipped part for neutral, but never by a step) + assert!(worst < 0.03, "{case}: largest relative step between neighbours {worst}"); + } + + /// The rebuilt colour has no step anywhere along the ramp: not where green first clips (the neutral fade starts + /// from nothing there), nor where red does, nor where the last channel clips (the fade ends at the fully + /// clipped neutral). + #[test] + fn rebuilt_ramp_is_continuous_from_the_first_clip_to_the_last() { + let (row, first) = rebuilt_ramp(0..32, 32); + assert_continuous(&row, first, 0.002, "full ramp"); + } + + /// The same ramp as a thin line on near black has no reliable colour around (every pixel of it is on a strong + /// edge), so its clipped part falls back to the neutral guess. That guess only applies as far as the line runs + /// past its own unclipped part: no step where it first clips, however thin the line and however much dark + /// background surrounds it. + #[test] + fn thin_line_without_reliable_colour_is_continuous_where_it_clips() { + for height in [32usize, 512, 1024] { + for width in [1usize, 4, 16] { + let top = (height - width) / 2; + let (row, first) = rebuilt_ramp(top..top + width, height); + assert_continuous(&row, first, 0.002, &format!("{width} rows of {height}")); + // and well past the first clip the neutral guess has taken over (green as high as red and blue imply) + let far = row[1500]; + assert!(far[1] > 0.95 * far[2], "{width} rows of {height}: {far:?}"); + } + } + } + + #[test] + fn unclipped_pixels_are_left_exactly() { let mut seed = 0x2545_f491_4f6c_dd1du64; let mut rnd = move || { seed ^= seed << 13; @@ -328,28 +587,28 @@ mod tests { seed ^= seed << 17; (seed >> 40) as f32 / (1u64 << 24) as f32 }; - for (w, h) in [(1, 1), (1, 2), (3, 1), (7, 5), (64, 48), (129, 77)] { - for density in [0.02f32, 0.3, 0.97] { - // smooth colour with clipped blobs, some dark pixels, some fully clipped ones - let noise: Vec = (0..w * h).map(|_| rnd()).collect(); - let img = Rgb32f::from_fn(w, h, |x, y| { - let t = ((x as f32 * 0.21).sin() + (y as f32 * 0.17).cos()) * 0.5; - let base = [0.5 + 0.4 * t, 0.45 - 0.2 * t, 0.3 + 0.1 * t]; - let r = noise[y * w + x]; - if r < density * 0.3 { - [1.0, 1.0, 1.0] - } else if r < density { - [1.0, base[1] * 1.6, base[2]] - } else if r > 0.995 { - [0.0, 0.0, 0.00001] - } else { - base - } - }); - let wb = [2.1, 1.0, 1.6]; - let (mut a, mut b) = (img.clone(), img.clone()); - assert_eq!(reconstruct(&mut a, wb, 0.99), reconstruct_ref(&mut b, wb, 0.99)); - assert!(a.data == b.data, "{w}×{h} at {density}"); + for (w, h) in [(1, 1), (3, 1), (7, 5), (64, 48), (129, 77)] { + let noise: Vec = (0..w * h).map(|_| rnd()).collect(); + let img = Rgb32f::from_fn(w, h, |x, y| { + let t = ((x as f32 * 0.21).sin() + (y as f32 * 0.17).cos()) * 0.5; + let base = [0.5 + 0.4 * t, 0.45 - 0.2 * t, 0.3 + 0.1 * t]; + match noise[y * w + x] { + r if r < 0.1 => [1.0; 3], + r if r < 0.4 => [1.0, base[1] * 1.6, base[2]], + r if r > 0.995 => [0.0, 0.0, 0.00001], + _ => base, + } + }); + let mut out = img.clone(); + let n = reconstruct_with(&mut out, [2.1, 1.0, 1.6], 0.99, [1.02, 0.99, 1.05]); + let clipped = img.data.iter().filter(|p| p.iter().any(|&v| v >= 0.99)).count(); + assert_eq!(n, clipped); + for (a, b) in img.data.iter().zip(&out.data) { + if a.iter().all(|&v| v < 0.99) { + assert_eq!(a, b, "{w}×{h}"); + } else { + assert!(b.iter().all(|v| v.is_finite() && *v >= 0.0), "{w}×{h}: {b:?}"); + } } } } @@ -368,19 +627,23 @@ mod tests { #[test] fn reconstruct_restores_clipped_channel() { - // a warm gradient whose red channel clips in the bright half - let truth = Rgb32f::from_fn(64, 16, |x, _| { - let v = 0.3 + 1.2 * x as f32 / 63.0; + // a warm gradient whose red channel clips at its bright end + let truth = Rgb32f::from_fn(256, 16, |x, _| { + let v = 0.3 + 1.0 * x as f32 / 255.0; [v * 1.0, v * 0.6, v * 0.35] }); let mut img = truth.map(|p| p.map(|v| v.min(1.0))); - let before: f32 = img.data.iter().zip(&truth.data).map(|(a, b)| (a[0] - b[0]).abs()).sum(); + let first = (0..256).find(|&x| img.get(x, 8)[0] >= 0.999).unwrap(); + // near the unclipped gradient (within the reach of its colour) red is rebuilt from it + let err = |img: &Rgb32f| -> f32 { (first..first + 12).map(|x| (img.get(x, 8)[0] - truth.get(x, 8)[0]).abs()).sum() }; + let before = err(&img); let n = reconstruct(&mut img, [1.0, 1.0, 1.0], 0.999); assert!(n > 0); - let after: f32 = img.data.iter().zip(&truth.data).map(|(a, b)| (a[0] - b[0]).abs()).sum(); + let after = err(&img); assert!(after < before * 0.2, "before {before} after {after}"); // unclipped pixels untouched assert_eq!(img.get(0, 0), truth.get(0, 0)); + assert_eq!(img.get(first - 1, 8), truth.get(first - 1, 8)); } #[test] @@ -394,4 +657,23 @@ mod tests { let mut empty = Rgb32f::new(0, 0); assert_eq!(reconstruct(&mut empty, [1.0; 3], 0.99), 0); } + + /// `cargo test --release -p lightcraft-raw --lib -- --ignored bench_reconstruct --nocapture` + #[test] + #[ignore] + fn bench_reconstruct() { + let img = Rgb32f::from_fn(6000, 4000, |x, y| { + let v = 0.4 + 0.8 * ((x as f32 * 0.003).sin() * (y as f32 * 0.002).cos()).abs(); + [v.min(1.0), (v * 0.7).min(1.0), (v * 0.5).min(1.0)] + }); + let ms = (0..3) + .map(|_| { + let mut i = img.clone(); + let t = std::time::Instant::now(); + reconstruct(&mut i, [2.0, 1.0, 1.5], 0.99); + t.elapsed().as_secs_f64() * 1e3 + }) + .fold(f64::MAX, f64::min); + eprintln!("reconstruct 24 MP: {ms:.0} ms"); + } } diff --git a/docs/parity.md b/docs/parity.md index 4c27755b5..8c243fd5f 100644 --- a/docs/parity.md +++ b/docs/parity.md @@ -241,7 +241,7 @@ under the UI command that wraps it (a no-op error); the UI command now wins. | LR-EDIT-HDR-MODE | HDR editing | P2 | ⬜ | | | | LR-EDIT-LIGHT-EXPOSURE | Exposure | P0 | ✅ | `ctl:light.exposure` | | | LR-EDIT-LIGHT-CONTRAST | Contrast | P0 | ✅ | `ctl:light.contrast` | | -| LR-EDIT-LIGHT-HIGHLIGHTS | Highlights | P0 | ✅ | `ctl:light.highlights` | | +| LR-EDIT-LIGHT-HIGHLIGHTS | Highlights | P0 | ✅ | `ctl:light.highlights`, `crates/raw/src/highlight.rs` | sensor-clipped highlights stay neutral when Highlights (or Exposure, Whites) darkens them: fully clipped content is rebuilt to the neutral of the colour model applied after it, partly clipped content takes its colour from reliable unclipped content nearby (not from dark objects or lens fringes beside it) and white where there is none (#523) | | LR-EDIT-LIGHT-SHADOWS | Shadows | P0 | ✅ | `ctl:light.shadows` | | | LR-EDIT-LIGHT-WHITES | Whites | P0 | ✅ | `ctl:light.whites` | | | LR-EDIT-LIGHT-BLACKS | Blacks | P0 | ✅ | `ctl:light.blacks` | | diff --git a/xtask/src/main.rs b/xtask/src/main.rs index cfdec92a3..4e6332930 100644 --- a/xtask/src/main.rs +++ b/xtask/src/main.rs @@ -340,6 +340,12 @@ const RAW_SAMPLES: &[(&str, &str, &str)] = &[ "https://raw.pixls.us/data/Sony/ILCE-7M4/ILCE-7M4_DSC06676_FullFrame-LossLess-Compressed-Small.ARW", "cbbd0930c7d8706dff84c68a2004454266e6fd0d8354f5f76a106b5d776e0223", ), + // a dusk sky clipped in green behind a poplar: the clipped-highlight colour of issue #523 + ( + "arw-sony-a7rm4-14bit-compressed.arw", + "https://raw.pixls.us/getfile.php/3480/nice/Sony%20-%20ILCE-7RM4%20-%2014bit%2014bit%20compressed%20%283:2%29.ARW", + "e6dafe42643f69ab9d1fd00414b7a1f5104df354bb589201600ac934b29b5e4a", + ), // pre-2017 bodies: white balance only in the enciphered maker note, black level only in the SR2SubIFD (#148) ( "arw-sony-rx100m3.arw", From 3cdf18ba6db99174a3b2ec53be79b6a58297f9ea Mon Sep 17 00:00:00 2001 From: hxw <530313@qq.com> Date: Sat, 10 Oct 2026 00:18:54 -0500 Subject: [PATCH 2/5] LR-VIEW-ZOOM: zoom windows cover a canvas wider than 2048 px (issue #652) The loupe read the GPU's texture limit from the frame's raw input (`i.raw.max_texture_side`). A native host (egui-winit) reports it in the first frame's input only, so from the second frame on the loupe fell back to 2048: every window render of a zoomed view was cut to 2048 x 2048 px, and on a canvas wider than that (HiDPI, 4K, a large window) the strips beside it showed the magnified whole-frame render. The whole-frame render and the second-window and compare renders were capped the same way. Read the limit egui keeps (`InputState::max_texture_side`) instead. The headless host reported the limit in every frame, which is why no test saw this: it now reports it once (and again when it changes), as a window does. Two regression tests: the limit outlives the frame it was reported in, and at 1:1 on a 2600 px canvas the window covers the canvas. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/headless.rs | 7 +++++- crates/ui-egui/src/panels/detail.rs | 26 ++++++++++++++++++++--- crates/ui-egui/src/tests_preview_limit.rs | 22 +++++++++++++++++++ 3 files changed, 51 insertions(+), 4 deletions(-) diff --git a/crates/ui-egui/src/headless.rs b/crates/ui-egui/src/headless.rs index e6929269f..fdf5ecb69 100644 --- a/crates/ui-egui/src/headless.rs +++ b/crates/ui-egui/src/headless.rs @@ -170,6 +170,9 @@ pub struct Headless { pub view: HeadlessView, /// The largest texture the pretend GPU takes (what a WebGL device may report: 2048). pub max_texture_side: usize, + /// The limit egui was last told. Like a native host (egui-winit) it is reported once, not in + /// every frame's input: code that reads the raw input sees `None` from then on (issue #652). + reported_texture_side: Option, /// Logical size (points) and scale. pub size: egui::Vec2, pub pixels_per_point: f32, @@ -201,6 +204,7 @@ impl Headless { app, view: HeadlessView::new(), max_texture_side: 16384, + reported_texture_side: None, size, pixels_per_point, time: 0.0, @@ -227,7 +231,8 @@ impl Headless { (self.size, self.pixels_per_point) = bounded_viewport(self.size, self.pixels_per_point); let mut raw = HeadlessView::raw_input(self.size, self.pixels_per_point, self.time, std::mem::take(&mut self.events)); raw.viewports.entry(ViewportId::ROOT).or_default().maximized = Some(self.window_maximized); - raw.max_texture_side = Some(self.max_texture_side); + raw.max_texture_side = (self.reported_texture_side != Some(self.max_texture_side)).then_some(self.max_texture_side); + self.reported_texture_side = Some(self.max_texture_side); self.app.raw_input_hook(&mut raw); let app = &mut self.app; let commands = self.view.run(raw, |ui| { diff --git a/crates/ui-egui/src/panels/detail.rs b/crates/ui-egui/src/panels/detail.rs index 2e708da66..cab305f0f 100644 --- a/crates/ui-egui/src/panels/detail.rs +++ b/crates/ui-egui/src/panels/detail.rs @@ -187,10 +187,14 @@ pub(crate) fn navigate_gesture(app: &mut LightcraftApp, ui: &mut egui::Ui, resp: true } -/// The largest texture the GPU behind `ctx` takes (egui reports it; 2048 when it doesn't, the +/// The largest texture the GPU behind `ctx` takes (egui reports it; 2048 until it does, the /// smallest limit WebGL devices have). +/// +/// The value egui keeps, not the frame's raw input: a native host (egui-winit) reports the limit +/// in the first frame's input only, so the raw value is `None` from the second frame on +/// (issue #652: every window render was cut to 2048 px, with blurry strips beside it). pub(crate) fn texture_side(ctx: &egui::Context) -> usize { - ctx.input(|i| i.raw.max_texture_side).unwrap_or(2048) + ctx.input(|i| i.max_texture_side) } /// The photo's own pixels as it is shown: its size after the crop and the user's rotation (what @@ -2050,7 +2054,23 @@ fn straighten_overlay(app: &mut LightcraftApp, ui: &mut egui::Ui, resp: &egui::R #[cfg(test)] mod tests { - use super::film_label; + use super::{film_label, texture_side}; + + /// Issue #652: a native host reports the GPU's texture limit in the first frame's input only. + /// The loupe must still know it in every later frame (it read the raw input, found nothing and + /// cut every window render to 2048 px). + #[test] + fn the_texture_limit_outlives_the_frame_it_was_reported_in() { + let ctx = egui::Context::default(); + assert_eq!(texture_side(&ctx), 2048, "until the host says: the smallest limit there is"); + let mut seen = Vec::new(); + for reported in [Some(8192), None, None] { + let raw = egui::RawInput { max_texture_side: reported, ..Default::default() }; + let mut out = ctx.run_ui(raw, |ui| seen.push(texture_side(ui.ctx()))); + out.textures_delta.clear(); + } + assert_eq!(seen, [8192, 8192, 8192]); + } #[test] fn film_labels_cut_on_characters_not_bytes() { diff --git a/crates/ui-egui/src/tests_preview_limit.rs b/crates/ui-egui/src/tests_preview_limit.rs index 4b15cd179..f48a2e1c3 100644 --- a/crates/ui-egui/src/tests_preview_limit.rs +++ b/crates/ui-egui/src/tests_preview_limit.rs @@ -352,6 +352,28 @@ mod in_the_loupe { } } + // Issue #652: given a canvas wider than 2048 px on a desktop GPU (which reports its texture + // limit to egui once, in the first frame), when I zoom to 1:1, then the window render covers + // the whole canvas: no strip of the magnified whole-frame render beside it + #[test] + fn a_window_covers_a_canvas_wider_than_2048_px() { + let (mut h, native) = detail_in([2600.0, 1200.0]); + assert!(native > 2600, "the photo is wider than the canvas at 1:1: {native}"); + h.request("engine.execute", json!({"command": "view.zoom100"}), T); + h.settle(SETTLE); + let canvas = h.app.canvas_rect.expect("the loupe was drawn"); + let image = h.app.image_rect.expect("the photo was drawn"); + let region = h.app.region_view.expect("a window"); + // the canvas in pixels of the window's frame (scale 1, at 100 %: a point is a pixel) + let to_px = region.full.0 as f32 / image.width(); + let (left, right) = (to_px * (canvas.left() - image.left()), to_px * (canvas.right() - image.left())); + assert!(right - left > 2048.0, "the canvas shows more than 2048 px: {left}..{right}"); + let (x0, x1) = (region.window.x as f32, (region.window.x + region.window.w) as f32); + assert!(x0 <= left + 0.5 && x1 >= right - 0.5, "the window {x0}..{x1} leaves part of the canvas {left}..{right} to the preview"); + let (w, _) = region_tile(&h).expect("the window rendered"); + assert_eq!(w, region.window.w, "and that is the texture drawn"); + } + // Given 400 % on a big photo, when a slider is dragged, then the work per frame follows the // canvas, not the zoom: a draft of the whole frame at canvas scale and a draft of the window #[test] From 41a61b74fcdba92b64e7cdbbf5f8ad3366eea493 Mon Sep 17 00:00:00 2001 From: Alex Chambers Date: Sat, 10 Oct 2026 07:40:18 +0200 Subject: [PATCH 3/5] LR-EDIT-LIGHT-HIGHLIGHTS: pale partly clipped skies turn white, not teal (#523) Testing the first version on ILCE-7RM4 photos found a partly clipped sky between bare branches leaning cyan to teal under #523's edit, where Lightroom shows white. Where only green clipped, red and blue say such a sky is a pale cyan-blue; the rebuild kept that colour until halfway to full clipping, and darkening and a camera profile showed it. Colour borrowed from another surface (bright foliage below the sky) could also push green up, with a small step where green first clips. - highlight::reconstruct_with: the colour of a pale partly clipped pixel (saturation relative to the colour model's white below 0.2, easing off by 0.4) fades to the clipped neutral from the first clip on instead of from halfway; brightness and strongly coloured pixels keep the old schedule. - Borrowed colour counts less and less as both the pixel's own channels and the rim's colour contradict it (0.15 to 0.4 stop); either alone can be wrong. - Right past the rim every estimate continues the rim's colour (it used to hold the clipped channels at their clip, for the neutral estimate only), so the first clip has no step whatever lent the estimate. The rim is built whenever there is clipping. - Nothing jumps where a later channel clips: in the rim distance and the contradiction, a clipped channel counts as the lower bound it is, and a channel goes over to that from 0.75 of its clip; a channel about to clip is a floor capped like the clip floor it becomes. - Tests: a pale sky above yellow-green foliage with thin dark branches in it (no step where green clips, no green from the foliage, the same beside the branches, neutral well past the clip); the fringe test compares the sky beside the branch with the sky away from it, on its way to neutral; a band whose red clips second next to a rim of another colour renders without a step, in six layouts. - docs/parity.md: the Highlights note. Render cache version 28. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/files.rs | 51 ++++++++ crates/engine/src/media.rs | 2 +- crates/raw/src/highlight.rs | 235 ++++++++++++++++++++++++++++-------- docs/parity.md | 2 +- 4 files changed, 239 insertions(+), 51 deletions(-) diff --git a/crates/engine/src/files.rs b/crates/engine/src/files.rs index 52177ae5f..989022d0b 100644 --- a/crates/engine/src/files.rs +++ b/crates/engine/src/files.rs @@ -1069,6 +1069,57 @@ mod tests { } } + /// A band clipped in green whose red rises through its own clip from left to right (sensor `[0.985 + 0.01 t, 1, + /// 0.8]`, t from 0 to 1 across 320 px), `band` rows at the bottom of the picture, below `strip` rows of a pale + /// surface just under the clip (`[0.4, 0.95, 0.8]`: the band's rim, of another colour) and above those `top` rows + /// of a strongly coloured reliable surface (`[0.8, 0.35, 0.3]`). Rebuilt and rendered at Exposure -2: the + /// rendered colour on either side of where red clips on the band's middle row, and the largest step between + /// neighbouring pixels along that row. + fn second_clip(top: usize, strip: usize, band: usize) -> ([[u8; 4]; 2], u8) { + use lightcraft_pipeline::{RenderRequest, render}; + let wb = [2.5f32, 1.0, 1.6]; + let colour = CameraColourModel { wb, matrix: lightcraft_color::Mat3::IDENTITY, gain: 1.0, tables: None, hue_sat: None }; + let (w, h) = (320usize, top + strip + band); + let sensor = Rgb32f::from_fn(w, h, |x, y| { + if y < top { + [0.8, 0.35, 0.3] + } else if y < top + strip { + [0.4, 0.95, 0.8] + } else { + [0.985 + 0.01 * x as f32 / (w - 1) as f32, 1.0, 0.8] + } + }); + let y = top + strip + band / 2; + let at = (0..w).find(|&x| sensor.get(x, y)[0] >= HIGHLIGHT_CLIP).unwrap(); + let mut img = sensor.clone(); + colour.rebuild_highlights(&mut img); + colour.apply(&mut img); + let mut s = lightcraft_develop::DevelopSettings::default(); + s.light.exposure = -2.0; + let out = render(&img, &SourceInfo { raw: true, ..Default::default() }, &s, &RenderRequest::fit(w, h)).image; + let step = |x: usize| { + let (a, b) = (out.get(x - 1, y), out.get(x, y)); + (0..3).map(|c| a[c].abs_diff(b[c])).max().unwrap_or(0) + }; + ([out.get(at - 1, y), out.get(at, y)], (1..w).map(step).max().unwrap_or(0)) + } + + /// Where a second channel clips within reach of the surface's own rim, the rebuilt colour carries on too. The + /// rim's comparison with the pixel used to drop each clipped channel, so a rim far from the pixel in red suddenly + /// matched it once red clipped (80 rows of colour, 16 of rim and 32 of band: [237, 124, 153] then + /// [205, 181, 168]); and a channel about to clip counted as an uncapped floor that was capped once it clipped + /// (without the coloured surface: [198, 195, 178] then [205, 181, 168]). + #[test] + fn rendered_colour_is_continuous_where_a_second_channel_clips() { + for (top, strip, band) in [(80, 16, 32), (0, 16, 32), (80, 16, 4), (200, 4, 8), (16, 64, 16), (8, 32, 24)] { + let ([a, b], worst) = second_clip(top, strip, band); + let case = format!("{top} rows of colour, {strip} of rim, {band} of band"); + let step = (0..3).map(|i| a[i].abs_diff(b[i])).max().unwrap_or(0); + assert!(step <= 1, "{case}: {a:?} then {b:?} where red clips"); + assert!(worst <= 3, "{case}: largest step between neighbouring pixels {worst} levels"); + } + } + /// Issue #523 on a public ILCE-7RM4 raw (raw.pixls.us, CC0; skipped without the corpus): a dusk sky clipped in /// green behind a poplar. Its rebuilt colour used to come from the tree's foliage and lens fringes, a green halo /// on one side of the tree and a magenta one on the other (Highlights -100 or the issue's edit show them). diff --git a/crates/engine/src/media.rs b/crates/engine/src/media.rs index b73d3f1de..9c09b5ec0 100644 --- a/crates/engine/src/media.rs +++ b/crates/engine/src/media.rs @@ -62,7 +62,7 @@ impl SettingsHashes { } /// Bump when the pipeline's output changes, to invalidate cached thumbnails. -pub const RENDER_CACHE_VERSION: u64 = 27; +pub const RENDER_CACHE_VERSION: u64 = 28; /// Thumbnails render at one of these long edges (so window/cell size changes reuse the cache). pub const THUMB_SIZES: [usize; 4] = [128, 256, 384, 512]; diff --git a/crates/raw/src/highlight.rs b/crates/raw/src/highlight.rs index 950be3afa..7fc056600 100644 --- a/crates/raw/src/highlight.rs +++ b/crates/raw/src/highlight.rs @@ -4,16 +4,20 @@ //! magenta/cyan (each channel limited to the smallest white-balanced clip level). //! - [`reconstruct`] / [`reconstruct_with`]: where only some channels are clipped, rebuild them from the //! unclipped channels using the chromaticity of reliable unclipped content nearby (bright, away from edges and -//! from the clipped pixels themselves; diffused into the clipped region coarse to fine), and neutral where there -//! is none within reach. Fully clipped pixels become neutral at the brightest plausible level; partly clipped -//! ones fade into it on their way from the first channel clipping to the last. +//! from the clipped pixels themselves; diffused into the clipped region coarse to fine) unless both the pixel's +//! own channels and the clipped surface's colour just below its clip (the rim) contradict it, and neutral where +//! there is none within reach. Right past the rim, the rim's colour carries on. Fully clipped pixels become +//! neutral at the brightest plausible level; partly clipped ones fade into it on their way from the first channel +//! clipping to the last, pale ones from the start. //! //! Why the care: the white-balance gains push clipped channels apart (green clips first, red and blue keep //! rising), so whatever colour the rebuilt pixels carry is hidden at Neutral only by the tone map's roll-off to -//! white. Any darkening (Highlights, negative Exposure or Whites) brings it out. Two things used to leak colour +//! white. Any darkening (Highlights, negative Exposure or Whites) brings it out. Three things used to leak colour //! in: the chromaticity of whatever unclipped pixel happened to be nearest (dark foliage, purple and green -//! fringes along branches, pixels demosaiced from clipped neighbours), and a colour model that does not render -//! the camera's white-balanced neutral as neutral (a look fitted to the camera's JPEG, a camera profile). +//! fringes along branches, pixels demosaiced from clipped neighbours); a colour model that does not render the +//! camera's white-balanced neutral as neutral (a look fitted to the camera's JPEG, a camera profile); and, in a +//! pale partly clipped sky, the colour of another reliable surface (bright foliage) and the sky's own faint +//! colour, which darkening and a camera profile turned teal. use lightcraft_raster::Rgb32f; use rayon::prelude::*; @@ -53,15 +57,28 @@ const SUPPORT: f32 = 0.05; /// An unclipped channel above `FLOOR_LO` of its clip level counts more and more as the clip floor it is about to /// become (see [`reconstruct_with`]). const FLOOR_LO: f32 = 0.75; -/// Without reliable colour around, a clipped pixel is rebuilt as neutral only as far as it is brighter (or -/// otherwise unlike) the unclipped pixels nearest its clip: the "rim", unclipped pixels above `RIM_LO` of the -/// clip level, weighted towards the clip. Up to `NEAR_RIM` stops past the rim (the rim is an average, a little -/// below the clip) the clipped channels stay at their clip, and over the next `DEPTH` stops they go over to the -/// neutral guess, so a surface runs into clipping without a step whatever its colour; well past it, or where the -/// rim is another surface (its colour differs), the neutral guess applies. +/// A clipped pixel takes its estimated colour (reliable colour around, or the neutral guess) only as far as it is +/// brighter (or otherwise unlike) the unclipped pixels nearest its clip: the "rim", unclipped pixels above `RIM_LO` +/// of the clip level, weighted towards the clip, the clipped surface's own colour just below it. Up to `NEAR_RIM` +/// stops past the rim (the rim is an average, a little below the clip) it is rebuilt with the rim's colour, and +/// over the next `DEPTH` stops it goes over to the estimate, so a surface runs into clipping without a step +/// whatever its colour and whatever lent the estimate; well past it, or where the rim is another surface (its +/// colour differs; a clipped channel differs by at least as much as it lies above the rim's), the estimate applies. const RIM_LO: f32 = 0.85; const NEAR_RIM: f32 = 0.03; const DEPTH: f32 = 0.2; +/// Reliable colour counts less and less as both the pixel's own channels (see [`contradiction`]) and the rim's +/// colour contradict it, by `AGREE.0` to `AGREE.1` stops: it is another surface's, like bright foliage below a pale +/// sky, whose green would turn the sky teal. (Either alone can be wrong: a pixel's own channels are noisy and +/// can't speak for the channels that clipped, and the rim can be another surface, like a white window frame +/// around a bright view.) +const AGREE: (f32, f32) = (0.15, 0.4); +/// On the way from the first channel clipping to the last, a partly clipped pixel's brightness goes over to the +/// clipped neutral's in the second half, and so does the colour of a strongly coloured one (saturation relative to +/// the white above `PALE.1`). A pale one's colour (below `PALE.0`) goes over from the start: the few per cent of +/// colour left in a pale sky once a channel clipped is as much the estimate's error as the sky's, and darkening +/// shows it. +const PALE: (f32, f32) = (0.2, 0.4); #[inline] fn smoothstep(e0: f32, e1: f32, x: f32) -> f32 { @@ -253,6 +270,35 @@ fn push(levels: &mut [Level], empty: [f32; N], needed: Option } } +/// How many stops two chromaticities differ by in any channel ratio. +fn unlike(a: [f32; 3], b: [f32; 3]) -> f32 { + let ratio = |i: usize, j: usize| (a[i].max(1e-6) * b[j].max(1e-6)) / (a[j].max(1e-6) * b[i].max(1e-6)); + [(0, 1), (0, 2), (1, 2)].iter().map(|&(i, j)| ratio(i, j).log2().abs()).fold(0.0f32, f32::max) +} + +/// How far channel `c` of a pixel (sensor values `p`, clipped channels `cl`) only bounds its true value from below: +/// fully once clipped, and increasingly from `FLOOR_LO` of the clip on, so that nothing compared with it jumps where +/// it clips. +fn bound(p: [f32; 3], cl: [bool; 3], clip: f32, c: usize) -> f32 { + if cl[c] { 1.0 } else { smoothstep(FLOOR_LO, 1.0, p[c] / clip) } +} + +/// How many stops the channels of a partly clipped pixel (sensor values `p`, white-balanced `q`, clipped channels +/// `cl`) contradict chromaticity `r`: how far the levels they imply spread, where a clipped channel's only bounds the +/// level from below (see [`bound`]). 0 for the pixel's own colour, whatever its level. +fn contradiction(p: [f32; 3], q: [f32; 3], cl: [bool; 3], r: [f32; 3], clip: f32) -> f32 { + let (mut lo, mut hi) = (f32::INFINITY, 0f32); + for c in 0..3 { + let level = q[c] / r[c].max(1e-3); + hi = hi.max(level); + let w = bound(p, cl, clip, c); + if r[c] > 1e-3 && w < 1.0 { + lo = lo.min(level / (1.0 - w)); + } + } + if hi > 0.0 && lo > 0.0 && lo.is_finite() { (hi / lo).log2().max(0.0) } else { 0.0 } +} + /// The white-balanced colour of a partly clipped pixel (sensor values `p`, white-balanced `q`, clipped channels /// `cl`) whose colour is estimated at chromaticity `r`: clipped channels rebuilt, unclipped ones kept. `None` /// when no unclipped channel can tell the level. @@ -273,10 +319,11 @@ fn rebuild(p: [f32; 3], q: [f32; 3], cl: [bool; 3], r: [f32; 3], clip: f32, max_ } // Two clipped channels must not be left at their clip levels: those differ by the white-balance gains (green // clips first), and the pair would show as magenta or red. They keep the estimate's ratio instead, at the level - // that lifts each to its clip (no channel above the clipped neutral). + // that lifts each to its clip (no channel above the clipped neutral), and so does a channel about to clip (the + // same floor, so nothing jumps where it clips). let floor = (0..3).filter(|&c| cl[c]).map(|c| q[c] / r[c].max(1e-3)).fold(0.0f32, f32::max); let top = r[0].max(r[1]).max(r[2]).max(1e-3); - let sum = (sum / k as f32).max(near_clip).max(floor.min(max_level / top)); + let sum = (sum / k as f32).max(near_clip.max(floor).min(max_level / top)); Some(std::array::from_fn(|c| if cl[c] { q[c].max(sum * r[c]) } else { q[c] })) } @@ -343,20 +390,16 @@ pub fn reconstruct_with(img: &mut Rgb32f, wb: [f32; 3], clip: f32, white: [f32; }); } let Some(field) = levels.first() else { return count }; - // the rim, only where some clipped pixel lacks reliable colour (level 0 of its pyramid holds the estimate) - let untrusted = field.cells.par_iter().enumerate().any(|(i, c)| c[2] < 1.0 && needed(i % field.w, i / field.w)); - let rims = untrusted - .then(|| { - let mut levels = vec![rim(img, wb, clip, &needed)]; - while let Some(l) = levels.last().filter(|l| l.w > 1 || l.h > 1) { - let next = pull(l); - levels.push(next); - } - push(&mut levels, [0.0; 4], None); - levels - }) - // a rim anywhere at all (else the neutral guess applies throughout) - .filter(|levels| levels.last().is_some_and(|top| top.cells.iter().any(|c| c[3] > 0.0))); + // the rim (level 0 of its pyramid holds the estimate), if there is one anywhere at all + let rims = { + let mut levels = vec![rim(img, wb, clip, &needed)]; + while let Some(l) = levels.last().filter(|l| l.w > 1 || l.h > 1) { + let next = pull(l); + levels.push(next); + } + push(&mut levels, [0.0; 4], None); + Some(levels).filter(|levels| levels.last().is_some_and(|top| top.cells.iter().any(|c| c[3] > 0.0))) + }; let rim_field = rims.as_ref().and_then(|l| l.first()); let max_level = wb.iter().cloned().fold(0.0f32, f32::max) * clip; img.data.par_chunks_mut(w).enumerate().for_each(|(y, row)| { @@ -368,22 +411,51 @@ pub fn reconstruct_with(img: &mut Rgb32f, wb: [f32; 3], clip: f32, white: [f32; } let q = [p[0] * wb[0], p[1] * wb[1], p[2] * wb[2]]; // rebuilt with the colour of the reliable content around, and with neutral, blended by how much of it - // there is (blending the results, not the colours, keeps the estimate consistent with the pixel's own - // unclipped channels all the way) + // there is and how well the pixel agrees with it (blending the results, not the colours, keeps the + // estimate consistent with the pixel's own unclipped channels all the way) let e = sample(field, 2.0, x, y); - let trust = e[2].clamp(0.0, 1.0); - let local = (trust > 0.0).then(|| rebuild(p, q, cl, [e[0], 1.0 - e[0] - e[1], e[1]], clip, max_level)).flatten(); - let plain = (trust < 1.0).then(|| rebuild(p, q, cl, neutral, clip, max_level)).flatten().map(|b| { - // only as far as the pixel lies past the rim nearest its clip (in every unclipped channel) - let Some(rim) = rim_field else { return b }; - let edge = sample(rim, 4.0, x, y); - let past = (0..3) - .filter(|&c| !cl[c]) - .map(|c| if edge[c] > 1e-6 && q[c] > 1e-6 { (q[c] / edge[c]).log2().abs() } else { f32::INFINITY }) - .fold(0.0f32, f32::max); + let borrowed = [e[0], 1.0 - e[0] - e[1], e[1]]; + let edge = rim_field.map(|rim| sample(rim, 4.0, x, y)); + let mut trust = e[2].clamp(0.0, 1.0); + if trust > 0.0 { + let mut off = contradiction(p, q, cl, borrowed, clip); + if off > AGREE.0 { + off = off.min(edge.map_or(0.0, |edge| { + let s = edge[0] + edge[1] + edge[2]; + if s > 1e-6 { unlike([edge[0] / s, edge[1] / s, edge[2] / s], borrowed) } else { 0.0 } + })); + } + trust *= 1.0 - smoothstep(AGREE.0, AGREE.1, off); + } + // either applies only as far as the pixel lies past the rim nearest its clip (in any channel); nearer, the + // pixel is rebuilt with the rim's colour + let at_rim = edge.and_then(|edge| { + // stops between pixel and rim in each channel; a clipped channel's true value lies at least as far + // above the rim's as its clip, and no further below (see [`bound`]) + let ratio = (0..3) + .map(|c| { + let (a, b) = (q[c].max(1e-12), edge[c].max(1e-12)); + if a >= b { + return a / b; + } + match bound(p, cl, clip, c) { + w if w <= 0.0 => b / a, + w if w >= 1.0 => 1.0, + w => (b / a).powf(1.0 - w), + } + }) + .fold(1.0f32, f32::max); + let past = ratio.log2().min(64.0); let k = smoothstep(NEAR_RIM, NEAR_RIM + DEPTH, past); - std::array::from_fn(|c| q[c] + (b[c] - q[c]) * k) + let s = edge[0] + edge[1] + edge[2]; + (k < 1.0 && s > 1e-6).then(|| rebuild(p, q, cl, [edge[0] / s, edge[1] / s, edge[2] / s], clip, max_level)).flatten().map(|a| (a, k)) }); + let past_rim = |b: [f32; 3]| -> [f32; 3] { + let Some((a, k)) = at_rim else { return b }; + std::array::from_fn(|c| a[c] + (b[c] - a[c]) * k) + }; + let local = (trust > 0.0).then(|| rebuild(p, q, cl, borrowed, clip, max_level)).flatten().map(past_rim); + let plain = (trust < 1.0).then(|| rebuild(p, q, cl, neutral, clip, max_level)).flatten().map(past_rim); let mut out = match (local, plain) { (Some(a), Some(b)) => std::array::from_fn(|c| b[c] + (a[c] - b[c]) * trust), (Some(a), None) | (None, Some(a)) => a, @@ -399,12 +471,19 @@ pub fn reconstruct_with(img: &mut Rgb32f, wb: [f32; 3], clip: f32, white: [f32; let above = (0..3).filter(|&c| cl[c]).map(|c| (out[c] / q[c].max(1e-9)).max(1.0).log2()).fold(0.0f32, f32::max); let lowest = (0..3).filter(|&c| !cl[c]).map(|c| p[c]).fold(f32::MAX, f32::min); let below = (clip / lowest.max(1e-6)).log2().max(0.0); - let t = if above > 0.0 && lowest < clip { smoothstep(0.5, 1.0, above / (above + below)) } else { 0.0 }; - if t > 0.0 { + let way = if above > 0.0 && lowest < clip { above / (above + below) } else { 0.0 }; + // The brightness goes over in the second half of the way, and so does the colour of a strongly coloured + // pixel; a pale one's goes over from the start ([`PALE`]). + let u = [out[0] / white[0], out[1] / white[1], out[2] / white[2]]; + let (lo, hi) = (u[0].min(u[1]).min(u[2]), u[0].max(u[1]).max(u[2])); + let saturation = if hi > 0.0 && hi.is_finite() { 1.0 - lo.max(0.0) / hi } else { 1.0 }; + let start = 0.5 * smoothstep(PALE.0, PALE.1, saturation); + let (t_level, t_colour) = (smoothstep(0.5, 1.0, way), smoothstep(start, start + 0.5, way)); + if t_colour > 0.0 { let level = (out[0] + out[1] + out[2]) / total; let full = q.iter().cloned().fold(0.0f32, f32::max).max(max_level); - let v = level + (full - level) * t; - out = std::array::from_fn(|c| out[c] + (white[c] * v - out[c]) * t); + let v = level + (full - level) * t_level; + out = std::array::from_fn(|c| out[c] + (white[c] * v - out[c]) * t_colour); } *px = [out[0] / wb[0], out[1] / wb[1], out[2] / wb[2]]; } @@ -450,21 +529,34 @@ mod tests { (img, sky) } + /// How far chromaticity `got` lies off the way from `from` to neutral (largest channel difference to the + /// nearest point of that way). + fn off_the_way_to_neutral(got: [f32; 3], from: [f32; 3]) -> f32 { + let d: [f32; 3] = std::array::from_fn(|c| 1.0 / 3.0 - from[c]); + let len = d.iter().map(|v| v * v).sum::(); + let t = if len > 0.0 { ((0..3).map(|c| (got[c] - from[c]) * d[c]).sum::() / len).clamp(0.0, 1.0) } else { 0.0 }; + (0..3).map(|c| (got[c] - (from[c] + d[c] * t)).abs()).fold(0.0, f32::max) + } + /// Issue #523: the clipped sky beside a branch took the colour of the branch's fringes and of the branch - /// itself (magenta, green), which any darkening of the highlights then showed. + /// itself (magenta, green), which any darkening of the highlights then showed. Beside the branch it is now the + /// clipped sky away from it: the sky's colour, on its way to neutral. #[test] fn fringes_and_dark_branches_do_not_tint_a_clipped_sky() { let (mut img, sky) = branch_scene(); assert!(reconstruct(&mut img, WB, 0.99) > 0); - let want = chroma(sky); let mut worst = 0f32; for y in 40..128 { + let away = chroma(balanced(img.get(30, y))); + assert!(away == chroma(balanced(img.get(170, y))), "row {y}"); + let off = off_the_way_to_neutral(away, chroma(sky)); + assert!(off < 0.005, "row {y}: the clipped sky {away:?} is not the sky's colour on its way to neutral ({off})"); for x in [84, 86, 88, 101, 103, 105] { let got = chroma(balanced(img.get(x, y))); - worst = worst.max((0..3).map(|c| (got[c] - want[c]).abs()).fold(0.0, f32::max)); + worst = worst.max((0..3).map(|c| (got[c] - away[c]).abs()).fold(0.0, f32::max)); } } - assert!(worst < 0.01, "clipped sky beside the branch is off the sky's colour by {worst}"); + assert!(worst < 0.01, "clipped sky beside the branch is off the sky away from it by {worst}"); } /// A warm light with red and green clipped: left at their clip levels the pair reads red-magenta (the white @@ -578,6 +670,51 @@ mod tests { } } + /// Issue #523 (a river photo darkened by Highlights -75): a pale cyan-blue sky (`[1, 1.07, 1.2]` white-balanced) + /// brightening from left to right through the point where green clips (and later blue), above bright + /// yellow-green foliage, with thin dark branches standing in its lower half. Its rebuilt colour used to lean + /// green to teal: the foliage lent its colour to the sky (green rebuilt from it), with a step where green first + /// clips, and the sky kept its own pale colour where Lightroom shows white. + #[test] + fn pale_sky_by_dark_branches_and_foliage_is_not_tinted() { + let sky = [1.0, 1.07, 1.2]; + let (w, h) = (320usize, 96usize); + let k = |x: usize| 0.7 * (1.2 * x as f32 / (w - 1) as f32).exp2(); + let branches = [150usize, 200, 250]; + let on_branch = |x: usize| branches.iter().any(|&b| (b.saturating_sub(1)..=b + 2).contains(&x)); + let img0 = Rgb32f::from_fn(w, h, |x, y| { + sensor(if y >= 72 { + [0.5, 0.6, 0.22] + } else if y >= 40 && branches.iter().any(|&b| x == b || x == b + 1) { + [0.1, 0.09, 0.04] + } else { + sky.map(|v| v * k(x)) + }) + }); + let mut img = img0.clone(); + reconstruct(&mut img, WB, 0.99); + let at = |x: usize, y: usize| chroma(balanced(img.get(x, y))); + let first = (0..w).find(|&x| img0.get(x, 20)[1] >= 0.99).unwrap_or(0); + assert!(first > 0 && first < 150); + let diff = |a: [f32; 3], b: [f32; 3]| (0..3).map(|c| (a[c] - b[c]).abs()).fold(0.0, f32::max); + for y in [20, 50] { + // no step where green clips + let step = diff(at(first - 1, y), at(first, y)); + assert!(step < 0.001, "row {y}: step of {step} where green clips"); + for x in first..w { + // nothing but the sky's colour on its way to neutral (no green from the foliage)... + let off = off_the_way_to_neutral(at(x, y), chroma(sky)); + assert!(on_branch(x) || off < 0.005, "row {y}, x {x}: {:?} is off the sky's way to neutral by {off}", at(x, y)); + // ... the same beside the branches as above them ... + let beside = diff(at(x, y), at(x, 20)); + assert!(on_branch(x) || beside < 0.005, "row {y}, x {x}: {:?} beside a branch, {:?} above", at(x, y), at(x, 20)); + } + // ... and neutral well before the last channel clips (the sky is 0.7 stops past green's clip there) + let end = at(w - 1, y); + assert!(end.iter().all(|v| (v - 1.0 / 3.0).abs() < 0.002), "row {y}: {end:?} at the end"); + } + } + #[test] fn unclipped_pixels_are_left_exactly() { let mut seed = 0x2545_f491_4f6c_dd1du64; diff --git a/docs/parity.md b/docs/parity.md index 8c243fd5f..29e460519 100644 --- a/docs/parity.md +++ b/docs/parity.md @@ -241,7 +241,7 @@ under the UI command that wraps it (a no-op error); the UI command now wins. | LR-EDIT-HDR-MODE | HDR editing | P2 | ⬜ | | | | LR-EDIT-LIGHT-EXPOSURE | Exposure | P0 | ✅ | `ctl:light.exposure` | | | LR-EDIT-LIGHT-CONTRAST | Contrast | P0 | ✅ | `ctl:light.contrast` | | -| LR-EDIT-LIGHT-HIGHLIGHTS | Highlights | P0 | ✅ | `ctl:light.highlights`, `crates/raw/src/highlight.rs` | sensor-clipped highlights stay neutral when Highlights (or Exposure, Whites) darkens them: fully clipped content is rebuilt to the neutral of the colour model applied after it, partly clipped content takes its colour from reliable unclipped content nearby (not from dark objects or lens fringes beside it) and white where there is none (#523) | +| LR-EDIT-LIGHT-HIGHLIGHTS | Highlights | P0 | ✅ | `ctl:light.highlights`, `crates/raw/src/highlight.rs` | sensor-clipped highlights stay neutral when Highlights (or Exposure, Whites) darkens them: fully clipped content is rebuilt to the neutral of the colour model applied after it, partly clipped content takes its colour from reliable unclipped content nearby (not from dark objects, lens fringes or another surface beside it) and white where there is none, and pale partly clipped content (a white sky) turns white on its way to clipping (#523) | | LR-EDIT-LIGHT-SHADOWS | Shadows | P0 | ✅ | `ctl:light.shadows` | | | LR-EDIT-LIGHT-WHITES | Whites | P0 | ✅ | `ctl:light.whites` | | | LR-EDIT-LIGHT-BLACKS | Blacks | P0 | ✅ | `ctl:light.blacks` | | From 5a537d5d39b69dfac601ac8f9c99784a5f398e98 Mon Sep 17 00:00:00 2001 From: hxw <530313@qq.com> Date: Sat, 10 Oct 2026 00:46:24 -0500 Subject: [PATCH 4/5] LR-BEHAV-GPU: no render is inside the GPU driver when the app quits (issue #620) A render still running on a worker thread while the process exits crashes in the driver (SIGSEGV in the NVIDIA Vulkan driver on Linux), where neither the panic hook nor catch_unwind helps: pool workers were detached, the devices are statics that are never dropped, and nothing on the way out waited for GPU work. - gpu: every entry into a device (render, device creation, pool trim, denoise runner and tiles) is counted as work in flight (exit.rs). begin_shutdown() lets nothing new in; wait_idle(timeout) waits for what is inside. - preview: JobPool keeps its workers. shutdown(timeout) and Drop discard the queued jobs and wait, bounded, for the running ones (2 s on drop); never for the calling thread, and not twice. - engine: a render job caught by the shutdown gives up instead of rendering on the CPU, so quitting doesn't wait for it. - app: on_exit ends with LightcraftApp::shutdown (2 s for everything), after settings and library are saved; the macOS menu-bar Quit goes through on_exit. Not changed: the export, merge, denoise and import threads are still detached; their GPU work goes through the gate, their CPU work is cut off at exit as before. Tests: pool drop waits / is bounded / survives a panicked job and its own job dropping it; the gate; GPU renders refused and the device not created after begin_shutdown; the app's shutdown leaves no render worker. The crash itself needs Linux + NVIDIA and was not reproduced here (Windows, DX12): 9 of 9 quits of the app mid-render exit with code 0 in under 0.8 s. Co-Authored-By: Claude Opus 5.5 --- apps/lightcraft/src/main.rs | 8 ++ crates/engine/src/media.rs | 9 +- crates/gpu/src/exit.rs | 132 ++++++++++++++++++++++++ crates/gpu/src/lib.rs | 53 +++++++++- crates/gpu/src/nn.rs | 11 ++ crates/gpu/tests/shutdown.rs | 58 +++++++++++ crates/gpu/tests/shutdown_first.rs | 25 +++++ crates/preview/src/pool.rs | 160 ++++++++++++++++++++++++++--- crates/preview/src/tests.rs | 113 ++++++++++++++++++++ crates/ui-egui/src/lib.rs | 17 +++ crates/ui-egui/src/render.rs | 13 +++ crates/ui-egui/tests/shutdown.rs | 40 ++++++++ docs/gpu-pipeline.md | 11 ++ docs/parity.md | 2 +- 14 files changed, 634 insertions(+), 18 deletions(-) create mode 100644 crates/gpu/src/exit.rs create mode 100644 crates/gpu/tests/shutdown.rs create mode 100644 crates/gpu/tests/shutdown_first.rs create mode 100644 crates/ui-egui/tests/shutdown.rs diff --git a/apps/lightcraft/src/main.rs b/apps/lightcraft/src/main.rs index 832b158b3..8d33b881e 100644 --- a/apps/lightcraft/src/main.rs +++ b/apps/lightcraft/src/main.rs @@ -78,9 +78,17 @@ impl eframe::App for App { if let Err(e) = self.0.session.close_library() { log::error!("saving the library failed: {e}"); } + // last, with everything saved: no render may be inside the GPU driver when the process ends + if !self.0.shutdown(EXIT_WAIT) { + log::warn!("quit: background work was still running after {EXIT_WAIT:?}"); + } } } +/// How long quitting waits for renders that are running (issue #620). A GPU render takes well under +/// a second; anything slower gives way to quitting. +const EXIT_WAIT: std::time::Duration = std::time::Duration::from_secs(2); + /// The window's renderer (egui-wgpu): eframe's defaults, with LightCraft's backend choice — DX12 /// alone on Windows unless `LIGHTCRAFT_GPU_BACKEND` / `WGPU_BACKEND` say otherwise (issue #136: /// with Vulkan in the set, wgpu loads the Vulkan driver even when it then picks DX12) — and DX12 diff --git a/crates/engine/src/media.rs b/crates/engine/src/media.rs index 1231221b9..a67bdcaac 100644 --- a/crates/engine/src/media.rs +++ b/crates/engine/src/media.rs @@ -752,7 +752,14 @@ impl RenderJob { }; } let was_loaded = matches!(self.source, SourceRef::Loaded(_)); - match self.source.load_source() { + // The app is closing (issue #620): nobody is left to show this, and quitting waits for the + // jobs that are running. Without this, one that lost the GPU mid-way would render on the CPU. + let closing = || lightcraft_gpu::shutting_down().then(|| "LightCraft is closing".to_string()); + let source = match closing() { + Some(e) => Err(e), + None => self.source.load_source().and_then(|s| closing().map_or(Ok(s), Err)), + }; + match source { Ok(source) => { // the plain picture, or (Denoise amount above 0 and a denoised picture cached) the mix the amount asks for let src = &source.image_for(self.settings.denoise_amount()); diff --git a/crates/gpu/src/exit.rs b/crates/gpu/src/exit.rs new file mode 100644 index 000000000..79f114ef1 --- /dev/null +++ b/crates/gpu/src/exit.rs @@ -0,0 +1,132 @@ +//! Leaving: no GPU work may be in flight when the process ends (issue #620). +//! +//! The devices are statics that are never dropped, and the threads that render (preview workers, +//! export, denoise, the device warm-up) are not the one that ends the process. Exiting runs the +//! driver's own teardown while those threads keep running: a render that is inside the driver at +//! that moment crashes there (seen as a SIGSEGV in the NVIDIA Vulkan driver), where neither the +//! panic hook nor `catch_unwind` can help. +//! +//! So every entry into a device goes through [`enter`], and the host ends the process with +//! [`crate::begin_shutdown`] (nothing new enters) and [`crate::wait_idle`] (what is in flight +//! finishes, up to a deadline). + +use std::sync::{Condvar, Mutex, MutexGuard}; +use std::time::{Duration, Instant}; + +struct State { + closed: bool, + /// [`Work`] in flight, on any thread. + busy: usize, +} + +pub(crate) struct Gate { + state: Mutex, + idle: Condvar, +} + +/// GPU work in flight (until dropped). +pub(crate) struct Work<'a>(&'a Gate); + +static GATE: Gate = Gate::new(); + +/// Start GPU work, or `None` when the process is ending: the caller then does without the GPU. +/// Work may nest (a render that creates the device). +pub(crate) fn enter() -> Option> { + GATE.enter() +} + +pub(crate) fn begin_shutdown() { + GATE.close(); +} + +pub(crate) fn shutting_down() -> bool { + GATE.lock().closed +} + +pub(crate) fn wait_idle(timeout: Duration) -> bool { + GATE.wait_idle(timeout) +} + +impl Gate { + pub const fn new() -> Gate { + Gate { state: Mutex::new(State { closed: false, busy: 0 }), idle: Condvar::new() } + } + + fn lock(&self) -> MutexGuard<'_, State> { + self.state.lock().unwrap_or_else(|e| e.into_inner()) + } + + pub fn enter(&self) -> Option> { + let mut s = self.lock(); + if s.closed { + return None; + } + s.busy = s.busy.saturating_add(1); + Some(Work(self)) + } + + /// Nothing enters from now on. + pub fn close(&self) { + self.lock().closed = true; + } + + /// Wait up to `timeout` until no work is in flight; `false` when some still is. + pub fn wait_idle(&self, timeout: Duration) -> bool { + let t0 = Instant::now(); + let mut s = self.lock(); + while s.busy > 0 { + let Some(left) = timeout.checked_sub(t0.elapsed()).filter(|d| !d.is_zero()) else { return false }; + s = self.idle.wait_timeout(s, left).unwrap_or_else(|e| e.into_inner()).0; + } + true + } +} + +impl Drop for Work<'_> { + fn drop(&mut self) { + let mut s = self.0.lock(); + s.busy = s.busy.saturating_sub(1); + if s.busy == 0 { + drop(s); + self.0.idle.notify_all(); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn closed_gate_lets_nothing_in_and_waits_for_what_is_inside() { + let gate = Gate::new(); + assert!(gate.wait_idle(Duration::ZERO), "idle: no wait"); + let outer = gate.enter().unwrap(); + let inner = gate.enter().unwrap(); + gate.close(); + assert!(gate.enter().is_none(), "nothing new once closed"); + drop(inner); + let t0 = Instant::now(); + assert!(!gate.wait_idle(Duration::from_millis(50)), "the outer work is still in flight"); + assert!(t0.elapsed() >= Duration::from_millis(50)); + std::thread::scope(|s| { + s.spawn(move || { + std::thread::sleep(Duration::from_millis(100)); + drop(outer); + }); + assert!(gate.wait_idle(Duration::from_secs(30)), "woken when the work ends"); + }); + assert!(gate.enter().is_none()); + } + + #[test] + fn work_that_panics_still_leaves() { + let gate = Gate::new(); + let r = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let _work = gate.enter(); + panic!("a render panics (expected in this test)"); + })); + assert!(r.is_err()); + assert!(gate.wait_idle(Duration::ZERO)); + } +} diff --git a/crates/gpu/src/lib.rs b/crates/gpu/src/lib.rs index 9c2349643..6c3a2ebe3 100644 --- a/crates/gpu/src/lib.rs +++ b/crates/gpu/src/lib.rs @@ -15,6 +15,8 @@ //! and [`last_fallback`] say why (`ui.inspect` → `perf.gpuReason` / `perf.gpuFallback`). //! The browser build has no GPU path yet (WebGPU device creation is asynchronous): everything here //! compiles to the CPU fallback on wasm32. +//! +//! Before the process ends: [`begin_shutdown`], then [`wait_idle`] (issue #620, see `exit.rs`). #![forbid(unsafe_code)] #![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] @@ -29,6 +31,8 @@ use lightcraft_raster::Rgb32f; pub mod backend; #[cfg(not(target_arch = "wasm32"))] mod ctx; +#[cfg(not(target_arch = "wasm32"))] +mod exit; #[cfg(all(feature = "denoise", not(target_arch = "wasm32")))] pub mod nn; #[cfg(not(target_arch = "wasm32"))] @@ -77,6 +81,9 @@ pub fn unavailable_reason() -> Option { if !ENABLED.load(Ordering::Relaxed) { return Some("disabled by the GPU rendering preference (app.gpu)".into()); } + if shutting_down() { + return Some("LightCraft is closing".into()); + } if BROKEN.load(Ordering::Relaxed) { let r = BROKEN_REASON.lock().unwrap_or_else(|e| e.into_inner()).clone().unwrap_or_else(|| "device error".into()); return Some(format!("stopped after a GPU failure: {r}")); @@ -147,6 +154,41 @@ pub fn enabled() -> bool { ENABLED.load(Ordering::Relaxed) && !BROKEN.load(Ordering::Relaxed) && !env_disabled() } +/// The process is about to end: no GPU work starts from now on, on any thread. [`render`] returns +/// `None` (callers render on the CPU), the devices are not created and denoise tiles fail over to +/// the CPU. There is no way back. Follow it with [`wait_idle`]. +pub fn begin_shutdown() { + #[cfg(not(target_arch = "wasm32"))] + exit::begin_shutdown(); +} + +/// Whether [`begin_shutdown`] was called. +pub fn shutting_down() -> bool { + #[cfg(not(target_arch = "wasm32"))] + { + exit::shutting_down() + } + #[cfg(target_arch = "wasm32")] + { + false + } +} + +/// Wait up to `timeout` for the GPU work in flight on every thread to end. `true`: none is left, +/// and after [`begin_shutdown`] none can start, so the process may end; `false`: the deadline +/// passed with a render (or a device being created) still inside the driver. +pub fn wait_idle(timeout: std::time::Duration) -> bool { + #[cfg(not(target_arch = "wasm32"))] + { + exit::wait_idle(timeout) + } + #[cfg(target_arch = "wasm32")] + { + let _ = timeout; + true + } +} + /// Why GPU compute that has a device of its own (denoise) is switched off by the user's settings, or `None`: the /// environment switches and the rendering preference, but not a render-device failure (the devices are separate). #[cfg(all(feature = "denoise", not(target_arch = "wasm32")))] @@ -177,6 +219,11 @@ pub(crate) fn device() -> Option<&'static ctx::Gpu> { if env_disabled() || !ENABLED.load(Ordering::Relaxed) { return existing_device(); } + if let Some(g) = GPU.get() { + return g.as_ref().ok(); + } + // creating the device is a long call into the driver: not once the process is ending + let _work = exit::enter()?; GPU.get_or_init(|| { let Some(backends) = backend::compute_backends() else { return Err("disabled by LIGHTCRAFT_GPU_BACKEND=off".into()) }; backend::with_init_marker(backends, || { @@ -281,7 +328,9 @@ pub fn set_pool_limit(bytes: u64) { /// Free recycled buffers until at most `keep` bytes stay pooled (e.g. when the app goes idle). pub fn trim_pool(keep: u64) { #[cfg(not(target_arch = "wasm32"))] - if let Some(g) = existing_device() { + if let Some(g) = existing_device() + && let Some(_work) = exit::enter() + { if keep == 0 { render::release_shared_source(); } @@ -340,6 +389,8 @@ pub fn render(src: &Arc, info: &SourceInfo, s: &DevelopSettings, req: &R if !enabled() || req.depth != lightcraft_pipeline::OutputDepth::U8 || req.proof.is_some() { return None; } + // in flight until this returns; `None` once the process is ending (issue #620) + let _work = exit::enter()?; let gpu = device()?; let ext = stages.map(|c| c.extension::()); let fault = take_fault(); diff --git a/crates/gpu/src/nn.rs b/crates/gpu/src/nn.rs index bb9d5eea7..fb4a4f780 100644 --- a/crates/gpu/src/nn.rs +++ b/crates/gpu/src/nn.rs @@ -63,9 +63,17 @@ fn dev() -> Result<&'static Dev, String> { if let Some(why) = crate::switched_off() { return Err(why); } + // creating the device is GPU work too: not once the process is ending (issue #620) + let _work = match DEV.get() { + Some(_) => None, + None => Some(crate::exit::enter().ok_or(CLOSING)?), + }; DEV.get_or_init(create_device).as_ref().map_err(|e| e.clone()) } +/// Why nothing runs on the GPU once [`crate::begin_shutdown`] was called. +const CLOSING: &str = "LightCraft is closing"; + fn create_device() -> Result { let Some(backends) = crate::backend::compute_backends() else { return Err("disabled by LIGHTCRAFT_GPU_BACKEND=off".into()) }; crate::backend::with_init_marker(backends, || { @@ -654,6 +662,7 @@ pub struct NetRunner { /// Put `net` on the GPU for `tile × tile` input cells, or say why it cannot be (no GPU, a layer or a size the kernels /// do not do, the device refusing the buffers). Creates the device on first use and compiles the kernels. pub fn runner(net: &Net, tile: usize) -> Result { + let _work = crate::exit::enter().ok_or(CLOSING)?; let d = dev()?; let max_binding = d.limits.max_storage_buffer_binding_size.min(d.limits.max_buffer_size).min(MAX_BUFFER); let (layout, layers) = plan(net, tile, max_binding)?; @@ -840,6 +849,8 @@ impl TileRunner for NetRunner { if input.len() != self.shared.layout.in_floats { return Err(Error::Input("the tile is not the size the model wants".into())); } + // one tile is one piece of GPU work: a photo's worth would outlast any wait at exit + let Some(_work) = crate::exit::enter() else { return Err(Error::Runtime(CLOSING.into())) }; let w = self.take()?; let r = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| self.execute(&w, input))); match r { diff --git a/crates/gpu/tests/shutdown.rs b/crates/gpu/tests/shutdown.rs new file mode 100644 index 000000000..b827a0711 --- /dev/null +++ b/crates/gpu/tests/shutdown.rs @@ -0,0 +1,58 @@ +//! Issue #620: a render still inside the GPU driver while the process exits crashes there (a +//! SIGSEGV no panic hook catches). Once `begin_shutdown` is called nothing starts on the GPU, and +//! `wait_idle` returns when the renders in flight on other threads have ended. +//! One test: the shutdown holds for the whole process. Without a GPU adapter only the CPU side of +//! the contract is checked. + +use std::sync::Arc; +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use std::time::Duration; + +use lightcraft_develop::DevelopSettings; +use lightcraft_pipeline::{RenderRequest, SourceInfo}; + +#[test] +fn nothing_renders_on_the_gpu_once_the_process_is_ending() { + let (w, h) = (600, 400); + let src = Arc::new(lightcraft_scenes::demo_library()[0].render(w, h)); + let info = SourceInfo { raw: true, ..Default::default() }; + let mut s = DevelopSettings::default(); + s.light.exposure = 0.3; + let req = RenderRequest::fit(w, h); + let has_gpu = lightcraft_gpu::available(); + if !has_gpu { + eprintln!("no GPU adapter ({:?}): only the CPU side is checked", lightcraft_gpu::unavailable_reason()); + } + assert!(!lightcraft_gpu::shutting_down()); + assert!(lightcraft_gpu::wait_idle(Duration::ZERO), "nothing is in flight yet"); + + // a worker that renders until the GPU refuses, as the preview workers do while the app quits + let (rendered, refused) = (Arc::new(AtomicUsize::new(0)), Arc::new(AtomicBool::new(false))); + let worker = { + let (src, info, s) = (src.clone(), info.clone(), s.clone()); + let (rendered, refused) = (rendered.clone(), refused.clone()); + std::thread::spawn(move || { + while lightcraft_gpu::render(&src, &info, &s, &req, None).is_some() { + rendered.fetch_add(1, Ordering::SeqCst); + } + refused.store(true, Ordering::SeqCst); + }) + }; + if has_gpu { + while rendered.load(Ordering::SeqCst) == 0 && !refused.load(Ordering::SeqCst) { + std::thread::yield_now(); + } + assert!(rendered.load(Ordering::SeqCst) > 0, "the GPU renders before the shutdown ({:?})", lightcraft_gpu::last_fallback()); + } + + lightcraft_gpu::begin_shutdown(); + assert!(lightcraft_gpu::shutting_down()); + assert!(lightcraft_gpu::wait_idle(Duration::from_secs(60)), "the render in flight ends"); + worker.join().unwrap(); + assert!(refused.load(Ordering::SeqCst)); + assert!(lightcraft_gpu::render(&src, &info, &s, &req, None).is_none(), "no GPU render once the process is ending"); + assert!(lightcraft_gpu::wait_idle(Duration::ZERO)); + if has_gpu { + assert_eq!(lightcraft_gpu::unavailable_reason().as_deref(), Some("LightCraft is closing")); + } +} diff --git a/crates/gpu/tests/shutdown_first.rs b/crates/gpu/tests/shutdown_first.rs new file mode 100644 index 000000000..3810762e6 --- /dev/null +++ b/crates/gpu/tests/shutdown_first.rs @@ -0,0 +1,25 @@ +//! Issue #620: quitting right after the start must not leave the device being created (a long +//! call into the driver, on the `lc-gpu-init` thread) while the process exits. Once +//! `begin_shutdown` is called the device is not created at all. +//! One test in its own process: the shutdown comes before the first use of the GPU. + +use std::sync::Arc; +use std::time::Duration; + +use lightcraft_develop::DevelopSettings; +use lightcraft_pipeline::{RenderRequest, SourceInfo}; + +#[test] +fn the_device_is_not_created_once_the_process_is_ending() { + lightcraft_gpu::begin_shutdown(); + lightcraft_gpu::warm_up(); + assert!(!lightcraft_gpu::available()); + assert_eq!(lightcraft_gpu::adapter_name(), None); + let src = Arc::new(lightcraft_scenes::demo_library()[0].render(300, 200)); + let rendered = lightcraft_gpu::render(&src, &SourceInfo::default(), &DevelopSettings::default(), &RenderRequest::fit(300, 200), None); + assert!(rendered.is_none()); + assert!(lightcraft_gpu::wait_idle(Duration::from_secs(60)), "the warm-up thread found the gate closed"); + // `ready()` is "device creation finished" (or the GPU is switched off by the environment) + let switched_off = lightcraft_gpu::unavailable_reason().is_some_and(|r| r.starts_with("disabled by LIGHTCRAFT_GPU")); + assert!(switched_off || !lightcraft_gpu::ready(), "no device was created"); +} diff --git a/crates/preview/src/pool.rs b/crates/preview/src/pool.rs index a0ea185f2..4dbd35661 100644 --- a/crates/preview/src/pool.rs +++ b/crates/preview/src/pool.rs @@ -7,11 +7,16 @@ //! //! Native: a fixed set of worker threads, started on first use. wasm32 (no threads): the host //! calls [`JobPool::run_inline`] once per frame. +//! +//! No worker outlives its pool by more than a deadline (issue #620: a render still inside the GPU +//! driver while the process exits crashes there): [`JobPool::shutdown`] and dropping the pool +//! discard the queued jobs and wait, bounded, for the running ones. use std::hash::Hash; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::mpsc::{Receiver, Sender, channel}; use std::sync::{Arc, Condvar, Mutex}; +use std::time::Duration; pub type Job = Box R + Send>; @@ -27,8 +32,16 @@ struct Shared { queue: Mutex>>, cv: Condvar, shutdown: AtomicBool, + /// The worker threads that have not ended yet, and the signal that one did. + #[cfg(not(target_arch = "wasm32"))] + live: Mutex>, + #[cfg(not(target_arch = "wasm32"))] + ended: Condvar, } +/// How long dropping a pool waits for the jobs still running (see [`JobPool::shutdown`]). +const DROP_WAIT: Duration = Duration::from_secs(2); + /// A finished job. pub struct Done { pub slot: S, @@ -47,6 +60,10 @@ pub struct JobPool { threads: usize, started: bool, seq: u64, + #[cfg(not(target_arch = "wasm32"))] + workers: Vec>, + /// [`JobPool::shutdown`] ran: dropping the pool doesn't wait a second time. + stopped: bool, } impl JobPool { @@ -54,12 +71,23 @@ impl JobPool { pub fn new(threads: usize) -> Self { let (tx, rx) = channel(); JobPool { - shared: Arc::new(Shared { queue: Mutex::new(Vec::new()), cv: Condvar::new(), shutdown: AtomicBool::new(false) }), + shared: Arc::new(Shared { + queue: Mutex::new(Vec::new()), + cv: Condvar::new(), + shutdown: AtomicBool::new(false), + #[cfg(not(target_arch = "wasm32"))] + live: Mutex::new(Vec::new()), + #[cfg(not(target_arch = "wasm32"))] + ended: Condvar::new(), + }), tx, rx, threads, started: false, seq: 0, + #[cfg(not(target_arch = "wasm32"))] + workers: Vec::new(), + stopped: false, } } @@ -81,24 +109,69 @@ impl JobPool { } self.started = true; #[cfg(not(target_arch = "wasm32"))] - for i in 0..self.threads { - let shared = self.shared.clone(); - let tx = self.tx.clone(); - let _ = std::thread::Builder::new().name(format!("lc-job-{i}")).spawn(move || { - while let Some(e) = take(&shared, true) { - let t0 = std::time::Instant::now(); - let result = (e.job)(); - let done = Done { slot: e.slot, key: e.key, result, ms: t0.elapsed().as_secs_f64() * 1000.0 }; - if tx.send(done).is_err() { - break; + { + // held while spawning: a worker that ends at once still finds its id listed + let mut live = self.shared.live.lock().unwrap_or_else(|e| e.into_inner()); + for i in 0..self.threads { + let shared = self.shared.clone(); + let tx = self.tx.clone(); + let spawned = std::thread::Builder::new().name(format!("lc-job-{i}")).spawn(move || { + let _ended = Ended(&shared); + while let Some(e) = take(&shared, true) { + let t0 = std::time::Instant::now(); + let result = (e.job)(); + let done = Done { slot: e.slot, key: e.key, result, ms: t0.elapsed().as_secs_f64() * 1000.0 }; + if tx.send(done).is_err() { + break; + } } + }); + match spawned { + Ok(h) => { + live.push(h.thread().id()); + self.workers.push(h); + } + Err(e) => log::error!("job pool: worker {i} could not start: {e}"), } - }); + } + } + } + + /// Stop the pool: queued jobs are dropped, no new one starts, and the call waits up to + /// `timeout` for the jobs that are running. Returns whether every worker has ended; one that + /// hasn't is left to finish on its own. For owners that are about to end the process, where + /// dropping the pool may come too late or never (`std::process::exit`). + pub fn shutdown(&mut self, timeout: Duration) -> bool { + self.stopped = true; + #[cfg(not(target_arch = "wasm32"))] + { + stop(&self.shared, &mut self.workers, timeout) + } + #[cfg(target_arch = "wasm32")] + { + let _ = timeout; + close(&self.shared); + true + } + } + + /// Worker threads that have not ended (0 before the first job, and on wasm). + pub fn live_workers(&self) -> usize { + #[cfg(not(target_arch = "wasm32"))] + { + self.shared.live.lock().map(|l| l.len()).unwrap_or(0) + } + #[cfg(target_arch = "wasm32")] + { + 0 } } /// Queue `job` for `slot`, replacing a queued (not yet running) job for the same slot. pub fn submit(&mut self, slot: S, key: u64, priority: u32, job: Job) { + if self.stopped { + return; // nothing runs it any more + } self.start(); self.seq += 1; let (lock, cv) = (&self.shared.queue, &self.shared.cv); @@ -172,9 +245,66 @@ fn take(shared: &Shared, wait: bool) -> Option> { } } -impl Drop for JobPool { +/// Takes a worker off the live list when its thread ends (also when a job panicked). +#[cfg(not(target_arch = "wasm32"))] +struct Ended<'a, S, R>(&'a Shared); + +#[cfg(not(target_arch = "wasm32"))] +impl Drop for Ended<'_, S, R> { fn drop(&mut self) { - self.shared.shutdown.store(true, Ordering::Relaxed); - self.shared.cv.notify_all(); + let me = std::thread::current().id(); + self.0.live.lock().unwrap_or_else(|e| e.into_inner()).retain(|id| *id != me); + self.0.ended.notify_all(); + } +} + +/// Tell the workers to stop and take the queued jobs away from them. +fn close(shared: &Shared) { + // the flag changes under the queue lock: a worker is either before its check or waiting + let queued = { + let mut q = shared.queue.lock().unwrap_or_else(|e| e.into_inner()); + shared.shutdown.store(true, Ordering::Relaxed); + std::mem::take(&mut *q) + }; + shared.cv.notify_all(); + drop(queued); // outside the lock: a job may own a lot +} + +/// [`JobPool::shutdown`]: wait up to `timeout` for the workers to end, without holding the queue +/// lock, and never for the calling thread (a pool dropped by one of its own jobs). +#[cfg(not(target_arch = "wasm32"))] +fn stop(shared: &Shared, workers: &mut Vec>, timeout: Duration) -> bool { + close(shared); + let me = std::thread::current().id(); + let t0 = std::time::Instant::now(); + let mut live = shared.live.lock().unwrap_or_else(|e| e.into_inner()); + while live.iter().any(|id| *id != me) { + let Some(left) = timeout.checked_sub(t0.elapsed()).filter(|d| !d.is_zero()) else { break }; + live = shared.ended.wait_timeout(live, left).unwrap_or_else(|e| e.into_inner()).0; + } + let running: Vec = live.iter().copied().filter(|id| *id != me).collect(); + drop(live); + for h in workers.drain(..) { + let id = h.thread().id(); + // a worker off the list is past its last job: joining it only waits for the thread to unwind + if id != me && !running.contains(&id) { + let _ = h.join(); + } + } + if !running.is_empty() { + log::warn!("job pool: {} job(s) still running after {timeout:?}; not waiting for them", running.len()); + } + running.is_empty() +} + +impl Drop for JobPool { + fn drop(&mut self) { + if self.stopped { + return; + } + #[cfg(not(target_arch = "wasm32"))] + stop(&self.shared, &mut self.workers, DROP_WAIT); + #[cfg(target_arch = "wasm32")] + close(&self.shared); } } diff --git a/crates/preview/src/tests.rs b/crates/preview/src/tests.rs index 3d6340032..d733ad829 100644 --- a/crates/preview/src/tests.rs +++ b/crates/preview/src/tests.rs @@ -201,6 +201,119 @@ fn pool_runs_inline() { assert_eq!(p.try_recv().map(|d| d.result), Some(1)); } +/// A pool with one worker that is inside a job until the returned sender is used or dropped. +fn pool_with_a_running_job() -> (JobPool, std::sync::mpsc::Sender<()>) { + let mut p: JobPool = JobPool::new(1); + let (started, has_started) = std::sync::mpsc::channel(); + let (release, released) = std::sync::mpsc::channel::<()>(); + p.submit( + 1, + 0, + 1, + Box::new(move || { + let _ = started.send(()); + let _ = released.recv_timeout(std::time::Duration::from_secs(60)); + }), + ); + has_started.recv().unwrap(); + (p, release) +} + +#[test] +fn pool_drop_waits_for_running_job() { + // issue #620: a job that outlives its pool can still be inside the GPU driver when the process exits + use std::sync::atomic::{AtomicBool, Ordering}; + let mut p: JobPool = JobPool::new(1); + let (started, has_started) = std::sync::mpsc::channel(); + let (finished, queued_ran) = (Arc::new(AtomicBool::new(false)), Arc::new(AtomicBool::new(false))); + let f = finished.clone(); + p.submit( + 1, + 0, + 9, + Box::new(move || { + let _ = started.send(()); + std::thread::sleep(std::time::Duration::from_millis(200)); + f.store(true, Ordering::SeqCst); + }), + ); + has_started.recv().unwrap(); + let q = queued_ran.clone(); + p.submit(2, 0, 1, Box::new(move || q.store(true, Ordering::SeqCst))); + drop(p); + assert!(finished.load(Ordering::SeqCst), "drop returned while the job was still running"); + assert!(!queued_ran.load(Ordering::SeqCst), "a job that had not started is discarded"); +} + +#[test] +fn pool_shutdown_reports_workers_and_drop_does_not_wait_again() { + let (mut p, release) = pool_with_a_running_job(); + assert_eq!(p.live_workers(), 1); + let t0 = std::time::Instant::now(); + assert!(!p.shutdown(std::time::Duration::from_millis(100)), "the job is still running"); + let waited = t0.elapsed(); + assert!(waited >= std::time::Duration::from_millis(100) && waited < std::time::Duration::from_secs(30), "{waited:?}"); + assert_eq!(p.live_workers(), 1); + // stopped: nothing new is queued, and the job that ends is noticed + p.submit(2, 0, 1, Box::new(|| ())); + assert_eq!(p.queued(), 0); + drop(release); + assert!(p.shutdown(std::time::Duration::from_secs(30))); + assert_eq!(p.live_workers(), 0); + + let (mut p, _release) = pool_with_a_running_job(); + assert!(!p.shutdown(std::time::Duration::ZERO)); + let t0 = std::time::Instant::now(); + drop(p); + assert!(t0.elapsed() < std::time::Duration::from_secs(1), "dropped after a shutdown: no second wait"); +} + +#[test] +fn pool_drop_is_bounded_when_a_job_never_ends() { + // a job blocked on something its owner holds must not hang the owner (it would hang quitting) + let (p, _release) = pool_with_a_running_job(); + let t0 = std::time::Instant::now(); + drop(p); + let waited = t0.elapsed(); + assert!(waited >= std::time::Duration::from_secs(1) && waited < std::time::Duration::from_secs(30), "{waited:?}"); +} + +#[test] +fn pool_stops_after_a_job_panicked_and_when_dropped_by_its_own_job() { + // a worker that unwound is not waited for + let mut p: JobPool = JobPool::new(1); + p.submit(1, 0, 1, Box::new(|| panic!("a job panics (expected in this test)"))); + while p.live_workers() > 0 { + std::thread::yield_now(); + } + assert!(p.shutdown(std::time::Duration::from_secs(30))); + + // the last owner of a pool is one of its jobs: the worker must not wait for (or join) itself + let slot: Arc>>> = Arc::new(std::sync::Mutex::new(None)); + let (dropped, has_dropped) = std::sync::mpsc::channel(); + let mut p: JobPool = JobPool::new(1); + let s = slot.clone(); + p.submit( + 1, + 0, + 1, + Box::new(move || { + let own = loop { + if let Some(p) = s.lock().unwrap().take() { + break p; + } + std::thread::yield_now(); + }; + let t0 = std::time::Instant::now(); + drop(own); + let _ = dropped.send(t0.elapsed()); + }), + ); + *slot.lock().unwrap() = Some(p); + let waited = has_dropped.recv_timeout(std::time::Duration::from_secs(30)).unwrap(); + assert!(waited < std::time::Duration::from_secs(1), "{waited:?}"); +} + #[test] fn disk_cache_never_touches_foreign_files() { // issue #98: a library opened on a folder that already has a `thumbs/` folder diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 705818911..3c55333c8 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -685,6 +685,23 @@ impl LightcraftApp { } } + /// The process is about to end (settings and library are saved by then): make sure nothing is + /// inside the GPU driver when it does (issue #620: a render running on a worker thread while + /// the process exits crashes in the driver). No GPU work starts from here on; the render + /// workers get up to `timeout` to finish their jobs, and what is left of it goes to GPU work + /// on other threads (export, denoise, the device warm-up). `false`: the deadline passed first + /// (the host exits anyway). The app renders nothing afterwards. + pub fn shutdown(&mut self, timeout: std::time::Duration) -> bool { + lightcraft_engine::gpu::begin_shutdown(); + #[cfg(not(target_arch = "wasm32"))] + let t0 = std::time::Instant::now(); + let stopped = self.renderer.shutdown(timeout); + #[cfg(not(target_arch = "wasm32"))] + let timeout = timeout.saturating_sub(t0.elapsed()); + let idle = lightcraft_engine::gpu::wait_idle(timeout); + stopped && idle + } + /// Per-frame logic before layout (control channel, renders, shortcuts, drops). pub fn logic(&mut self, ctx: &egui::Context) { i18n::set_language(self.ui.language); diff --git a/crates/ui-egui/src/render.rs b/crates/ui-egui/src/render.rs index f46436941..5bbd4516e 100644 --- a/crates/ui-egui/src/render.rs +++ b/crates/ui-egui/src/render.rs @@ -415,6 +415,19 @@ impl Renderer { self.pending.contains_key(&slot) } + /// The app is closing: drop the jobs that have not started and wait up to `timeout` for the + /// ones running on the worker threads. Returns whether all of them have ended. Nothing is + /// rendered afterwards. + pub fn shutdown(&mut self, timeout: std::time::Duration) -> bool { + self.queue.clear(); + self.pool.shutdown(timeout) + } + + /// Worker threads that are still alive (0 before the first render). + pub fn live_workers(&self) -> usize { + self.pool.live_workers() + } + /// The texture to show for a grid/filmstrip thumbnail: the rendered one, else its stand-in. pub fn thumb(&self, id: PhotoId) -> Option<&Tex> { self.textures.get(&Slot::Thumb(id)).or_else(|| self.textures.get(&Slot::ThumbQuick(id))) diff --git a/crates/ui-egui/tests/shutdown.rs b/crates/ui-egui/tests/shutdown.rs new file mode 100644 index 000000000..1a601bc78 --- /dev/null +++ b/crates/ui-egui/tests/shutdown.rs @@ -0,0 +1,40 @@ +//! Issue #620: quitting while previews render must not leave a render worker running (on Linux +//! with the NVIDIA driver, one inside the GPU when the process exits is a SIGSEGV). +//! `LightcraftApp::shutdown` stops the render workers and closes the GPU for the whole process, +//! hence one test in a binary of its own. + +use std::time::{Duration, Instant}; + +use lightcraft_ui_egui::headless::Headless; +use lightcraft_ui_egui::{LightcraftApp, Services}; + +#[test] +fn shutdown_leaves_no_render_worker_running() { + let services = Services { png: None, ..Default::default() }; + let mut h = Headless::new(LightcraftApp::new(lightcraft_engine::Session::with_demo(), services), [1200.0, 760.0], 1.0); + // frames until the grid's thumbnails are being rendered on the workers + let t0 = Instant::now(); + while h.app.renderer.live_workers() == 0 || h.app.renderer.in_flight() == 0 { + assert!(t0.elapsed() < Duration::from_secs(120), "no render started"); + h.step(); + } + assert!(!lightcraft_engine::gpu::shutting_down()); + + assert!(h.app.shutdown(Duration::from_secs(120)), "the jobs that were running ended in time"); + assert_eq!(h.app.renderer.live_workers(), 0); + assert_eq!(h.app.renderer.queued(), 0, "jobs that had not started are dropped"); + assert!(lightcraft_engine::gpu::shutting_down()); + assert!(lightcraft_engine::gpu::wait_idle(Duration::ZERO), "nothing is inside the GPU"); + + // frames after it (the window's last ones) ask for renders that are not started any more + for _ in 0..3 { + h.step(); + } + assert_eq!(h.app.renderer.live_workers(), 0); + assert_eq!(h.app.renderer.queued(), 0); + + // a job that was caught before its render gives up: quitting doesn't wait for a CPU render + let id = h.app.session.active().or_else(|| h.app.session.catalog.photos().next().map(|p| p.id)).expect("a demo photo"); + let job = h.app.session.render_job(id, 300, 200, false, true).expect("a render job"); + assert_eq!(job.run().rendered.err().as_deref(), Some("LightCraft is closing")); +} diff --git a/docs/gpu-pipeline.md b/docs/gpu-pipeline.md index c843d5a06..8d7472352 100644 --- a/docs/gpu-pipeline.md +++ b/docs/gpu-pipeline.md @@ -60,6 +60,17 @@ up (`gpu::warm_up` from the first frame's settings), and never while GPU renderi Settings ▸ Performance ▸ *Use the GPU for rendering* unchecked (applied before the window opens), `LIGHTCRAFT_GPU=0` or `LIGHTCRAFT_GPU_BACKEND=off`, no GPU driver is loaded for rendering at all. +**Quitting.** The devices are never dropped, and renders run on worker threads: a render still inside +the driver while the process exits crashes there (a SIGSEGV in the NVIDIA Vulkan driver on Linux, +issue #620), which no panic hook catches. So every entry into a device counts as work in flight +(`crates/gpu/src/exit.rs`), and the desktop app ends with `LightcraftApp::shutdown` (from `on_exit`, +after settings and library are saved): `gpu::begin_shutdown()` — from then on `gpu::render` returns +`None`, no device is created, denoise tiles fail over and a render job gives up instead of rendering +on the CPU — then the render pool stops (queued jobs dropped, running ones waited for) and +`gpu::wait_idle` waits for GPU work on other threads (export, denoise, the device warm-up). One +deadline of 2 s covers it all; past it the app quits anyway. A job pool that is dropped (headless +hosts, tests) waits for its running jobs the same way (`JobPool::shutdown`, `crates/preview/src/pool.rs`). + **Crash sentinel.** The desktop app writes `gpu-init.marker` into its settings folder (next to `ui.json`: `%APPDATA%\LightCraft`, `~/Library/Application Support/LightCraft`, `~/.config/lightcraft`) just before the compute device is created and removes it as soon as creation diff --git a/docs/parity.md b/docs/parity.md index 4c27755b5..4f64fdcab 100644 --- a/docs/parity.md +++ b/docs/parity.md @@ -517,7 +517,7 @@ under the UI command that wraps it (a no-op error); the UI command now wins. | LR-BEHAV-BG-TASKS | Background tasks | P0 | ✅ | `crates/preview/src/lib.rs` (`JobPool`), `crates/ui-egui/src/export_task.rs`, `crates/ui-egui/src/import.rs` | renders off the UI thread; exports started from the UI run on a worker thread with a progress panel and Cancel (`ui.inspect` → `export`); imports show a progress window. No combined activity centre | | LR-BEHAV-OFFLINE | Offline editing | P1 | ✅ | | local-first: everything works offline | | LR-BEHAV-WEB-SAFETY | Library safety in the browser build | P1 | 🟡 | `cmd:file.backupLibrary`, `cmd:file.restoreLibrary`, `apps/lightcraft-web/src/safety.rs`, `apps/lightcraft-web/src/backup.rs`, `apps/lightcraft-web/src/files.rs` | web only (experimental): Back Up Library (zip of the catalog and every stored original) and a non-destructive Restore; failed saves show as unsaved and are retried; a photo that can't be stored isn't added; one tab per library (Web Locks); notices for storage that isn't persistent and for a library that can't open; `?reset` asks first; a panic shows a message. Missing: `?safe` start, deleting originals of removed photos, zip64 (backups > 4 GB) | -| LR-BEHAV-GPU | GPU acceleration | P0 | ✅ | `crates/gpu/src/render.rs`, `cmd:app.gpu`, `docs/gpu-pipeline.md` | CPU fallback per render on device limits, out of memory, device errors / loss, incomplete or black results (`perf.gpuReason`, `perf.gpuFallback`); short submissions for slow iGPUs; backend choice (DX12 only on Windows, `LIGHTCRAFT_GPU_BACKEND` / `WGPU_BACKEND`), device created after the window opens, crash sentinel (issue #136); no tiling beyond the buffer limit (CPU) | +| LR-BEHAV-GPU | GPU acceleration | P0 | ✅ | `crates/gpu/src/render.rs`, `cmd:app.gpu`, `docs/gpu-pipeline.md` | CPU fallback per render on device limits, out of memory, device errors / loss, incomplete or black results (`perf.gpuReason`, `perf.gpuFallback`); short submissions for slow iGPUs; backend choice (DX12 only on Windows, `LIGHTCRAFT_GPU_BACKEND` / `WGPU_BACKEND`), device created after the window opens, crash sentinel (issue #136); quitting waits (up to 2 s) for renders in flight and starts no new GPU work, so none is inside the driver when the process exits (`crates/gpu/src/exit.rs`, issue #620; seen crashing only on Linux + NVIDIA, other drivers unverified); no tiling beyond the buffer limit (CPU) | | LR-BEHAV-RENDER-FIDELITY | Rendering matches Lightroom | P1 | 🟡 | `crates/pipeline/src/lib.rs`, `crates/pipeline/src/tone.rs` | every slider exists and works, but the character of the result (tone curve shape, highlight recovery, texture / clarity / dehaze, noise reduction, sharpening, default look) is tuned by eye; there is no systematic side-by-side comparison against Lightroom on the same CC0 raws. Needs a fidelity suite: Lightroom reference renders kept only in the local `plan/` (never committed), compared per slider and preset with a perceptual metric | | LR-BEHAV-DRAGDROP | Drag and drop | P1 | ✅ | `crates/ui-egui/src/lib.rs`, `crates/ui-egui/src/panels/grid.rs` (`drag_feedback`), `crates/ui-egui/src/panels/left.rs` (`drop_target`, `album_drag_over`, `top_level_drop_target`, `drag_auto_scroll`) | files → app (import); grid photos → an album row (adds the selection, with a count badge while dragging); sidebar album / folder rows → a folder (moves it; its name follows the pointer); a drag of albums or grid photos near the top or bottom edge of the sidebar scrolls it (faster nearer the edge, only while the pointer is over the sidebar) | | LR-BEHAV-TOAST | Toast notifications | P1 | ✅ | `crates/ui-egui/src/panels/mod.rs` | | From aa6dfe3b30012ddf8ae7b8c7369f7bb26e349069 Mon Sep 17 00:00:00 2001 From: Alex Chambers Date: Sat, 10 Oct 2026 07:40:18 +0200 Subject: [PATCH 5/5] LR-EDIT-LIGHT-HIGHLIGHTS: pale partly clipped skies turn white, not teal (#523) Testing the first version on ILCE-7RM4 photos found a partly clipped sky between bare branches leaning cyan to teal under #523's edit, where Lightroom shows white. Where only green clipped, red and blue say such a sky is a pale cyan-blue; the rebuild kept that colour until halfway to full clipping, and darkening and a camera profile showed it. Colour borrowed from another surface (bright foliage below the sky) could also push green up, with a small step where green first clips. - highlight::reconstruct_with: the colour of a pale partly clipped pixel (saturation relative to the colour model's white below 0.2, easing off by 0.4) fades to the clipped neutral from the first clip on instead of from halfway; brightness and strongly coloured pixels keep the old schedule. - Borrowed colour counts less and less as both the pixel's own channels and the rim's colour contradict it (0.15 to 0.4 stop); either alone can be wrong. - Right past the rim every estimate continues the rim's colour (it used to hold the clipped channels at their clip, for the neutral estimate only), so the first clip has no step whatever lent the estimate. The rim is built whenever there is clipping. - Nothing jumps where a later channel clips: in the rim distance and the contradiction, a clipped channel counts as the lower bound it is, and a channel goes over to that from 0.75 of its clip; a channel about to clip is a floor capped like the clip floor it becomes. - Tests: a pale sky above yellow-green foliage with thin dark branches in it (no step where green clips, no green from the foliage, the same beside the branches, neutral well past the clip); the fringe test compares the sky beside the branch with the sky away from it, on its way to neutral; a band whose red clips second next to a rim of another colour renders without a step, in six layouts. - docs/parity.md: the Highlights note. Render cache version 28. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/files.rs | 51 ++++++++ crates/engine/src/media.rs | 2 +- crates/raw/src/highlight.rs | 235 ++++++++++++++++++++++++++++-------- docs/parity.md | 2 +- 4 files changed, 239 insertions(+), 51 deletions(-) diff --git a/crates/engine/src/files.rs b/crates/engine/src/files.rs index d6294b80c..e1c54b342 100644 --- a/crates/engine/src/files.rs +++ b/crates/engine/src/files.rs @@ -1084,6 +1084,57 @@ mod tests { } } + /// A band clipped in green whose red rises through its own clip from left to right (sensor `[0.985 + 0.01 t, 1, + /// 0.8]`, t from 0 to 1 across 320 px), `band` rows at the bottom of the picture, below `strip` rows of a pale + /// surface just under the clip (`[0.4, 0.95, 0.8]`: the band's rim, of another colour) and above those `top` rows + /// of a strongly coloured reliable surface (`[0.8, 0.35, 0.3]`). Rebuilt and rendered at Exposure -2: the + /// rendered colour on either side of where red clips on the band's middle row, and the largest step between + /// neighbouring pixels along that row. + fn second_clip(top: usize, strip: usize, band: usize) -> ([[u8; 4]; 2], u8) { + use lightcraft_pipeline::{RenderRequest, render}; + let wb = [2.5f32, 1.0, 1.6]; + let colour = CameraColourModel { wb, matrix: lightcraft_color::Mat3::IDENTITY, gain: 1.0, tables: None, hue_sat: None }; + let (w, h) = (320usize, top + strip + band); + let sensor = Rgb32f::from_fn(w, h, |x, y| { + if y < top { + [0.8, 0.35, 0.3] + } else if y < top + strip { + [0.4, 0.95, 0.8] + } else { + [0.985 + 0.01 * x as f32 / (w - 1) as f32, 1.0, 0.8] + } + }); + let y = top + strip + band / 2; + let at = (0..w).find(|&x| sensor.get(x, y)[0] >= HIGHLIGHT_CLIP).unwrap(); + let mut img = sensor.clone(); + colour.rebuild_highlights(&mut img); + colour.apply(&mut img); + let mut s = lightcraft_develop::DevelopSettings::default(); + s.light.exposure = -2.0; + let out = render(&img, &SourceInfo { raw: true, ..Default::default() }, &s, &RenderRequest::fit(w, h)).image; + let step = |x: usize| { + let (a, b) = (out.get(x - 1, y), out.get(x, y)); + (0..3).map(|c| a[c].abs_diff(b[c])).max().unwrap_or(0) + }; + ([out.get(at - 1, y), out.get(at, y)], (1..w).map(step).max().unwrap_or(0)) + } + + /// Where a second channel clips within reach of the surface's own rim, the rebuilt colour carries on too. The + /// rim's comparison with the pixel used to drop each clipped channel, so a rim far from the pixel in red suddenly + /// matched it once red clipped (80 rows of colour, 16 of rim and 32 of band: [237, 124, 153] then + /// [205, 181, 168]); and a channel about to clip counted as an uncapped floor that was capped once it clipped + /// (without the coloured surface: [198, 195, 178] then [205, 181, 168]). + #[test] + fn rendered_colour_is_continuous_where_a_second_channel_clips() { + for (top, strip, band) in [(80, 16, 32), (0, 16, 32), (80, 16, 4), (200, 4, 8), (16, 64, 16), (8, 32, 24)] { + let ([a, b], worst) = second_clip(top, strip, band); + let case = format!("{top} rows of colour, {strip} of rim, {band} of band"); + let step = (0..3).map(|i| a[i].abs_diff(b[i])).max().unwrap_or(0); + assert!(step <= 1, "{case}: {a:?} then {b:?} where red clips"); + assert!(worst <= 3, "{case}: largest step between neighbouring pixels {worst} levels"); + } + } + /// Issue #523 on a public ILCE-7RM4 raw (raw.pixls.us, CC0; skipped without the corpus): a dusk sky clipped in /// green behind a poplar. Its rebuilt colour used to come from the tree's foliage and lens fringes, a green halo /// on one side of the tree and a magenta one on the other (Highlights -100 or the issue's edit show them). diff --git a/crates/engine/src/media.rs b/crates/engine/src/media.rs index cfb2c6743..e6ef4b32b 100644 --- a/crates/engine/src/media.rs +++ b/crates/engine/src/media.rs @@ -62,7 +62,7 @@ impl SettingsHashes { } /// Bump when the pipeline's output changes, to invalidate cached thumbnails. -pub const RENDER_CACHE_VERSION: u64 = 36; +pub const RENDER_CACHE_VERSION: u64 = 37; /// Thumbnails render at one of these long edges (so window/cell size changes reuse the cache). pub const THUMB_SIZES: [usize; 4] = [128, 256, 384, 512]; diff --git a/crates/raw/src/highlight.rs b/crates/raw/src/highlight.rs index 950be3afa..7fc056600 100644 --- a/crates/raw/src/highlight.rs +++ b/crates/raw/src/highlight.rs @@ -4,16 +4,20 @@ //! magenta/cyan (each channel limited to the smallest white-balanced clip level). //! - [`reconstruct`] / [`reconstruct_with`]: where only some channels are clipped, rebuild them from the //! unclipped channels using the chromaticity of reliable unclipped content nearby (bright, away from edges and -//! from the clipped pixels themselves; diffused into the clipped region coarse to fine), and neutral where there -//! is none within reach. Fully clipped pixels become neutral at the brightest plausible level; partly clipped -//! ones fade into it on their way from the first channel clipping to the last. +//! from the clipped pixels themselves; diffused into the clipped region coarse to fine) unless both the pixel's +//! own channels and the clipped surface's colour just below its clip (the rim) contradict it, and neutral where +//! there is none within reach. Right past the rim, the rim's colour carries on. Fully clipped pixels become +//! neutral at the brightest plausible level; partly clipped ones fade into it on their way from the first channel +//! clipping to the last, pale ones from the start. //! //! Why the care: the white-balance gains push clipped channels apart (green clips first, red and blue keep //! rising), so whatever colour the rebuilt pixels carry is hidden at Neutral only by the tone map's roll-off to -//! white. Any darkening (Highlights, negative Exposure or Whites) brings it out. Two things used to leak colour +//! white. Any darkening (Highlights, negative Exposure or Whites) brings it out. Three things used to leak colour //! in: the chromaticity of whatever unclipped pixel happened to be nearest (dark foliage, purple and green -//! fringes along branches, pixels demosaiced from clipped neighbours), and a colour model that does not render -//! the camera's white-balanced neutral as neutral (a look fitted to the camera's JPEG, a camera profile). +//! fringes along branches, pixels demosaiced from clipped neighbours); a colour model that does not render the +//! camera's white-balanced neutral as neutral (a look fitted to the camera's JPEG, a camera profile); and, in a +//! pale partly clipped sky, the colour of another reliable surface (bright foliage) and the sky's own faint +//! colour, which darkening and a camera profile turned teal. use lightcraft_raster::Rgb32f; use rayon::prelude::*; @@ -53,15 +57,28 @@ const SUPPORT: f32 = 0.05; /// An unclipped channel above `FLOOR_LO` of its clip level counts more and more as the clip floor it is about to /// become (see [`reconstruct_with`]). const FLOOR_LO: f32 = 0.75; -/// Without reliable colour around, a clipped pixel is rebuilt as neutral only as far as it is brighter (or -/// otherwise unlike) the unclipped pixels nearest its clip: the "rim", unclipped pixels above `RIM_LO` of the -/// clip level, weighted towards the clip. Up to `NEAR_RIM` stops past the rim (the rim is an average, a little -/// below the clip) the clipped channels stay at their clip, and over the next `DEPTH` stops they go over to the -/// neutral guess, so a surface runs into clipping without a step whatever its colour; well past it, or where the -/// rim is another surface (its colour differs), the neutral guess applies. +/// A clipped pixel takes its estimated colour (reliable colour around, or the neutral guess) only as far as it is +/// brighter (or otherwise unlike) the unclipped pixels nearest its clip: the "rim", unclipped pixels above `RIM_LO` +/// of the clip level, weighted towards the clip, the clipped surface's own colour just below it. Up to `NEAR_RIM` +/// stops past the rim (the rim is an average, a little below the clip) it is rebuilt with the rim's colour, and +/// over the next `DEPTH` stops it goes over to the estimate, so a surface runs into clipping without a step +/// whatever its colour and whatever lent the estimate; well past it, or where the rim is another surface (its +/// colour differs; a clipped channel differs by at least as much as it lies above the rim's), the estimate applies. const RIM_LO: f32 = 0.85; const NEAR_RIM: f32 = 0.03; const DEPTH: f32 = 0.2; +/// Reliable colour counts less and less as both the pixel's own channels (see [`contradiction`]) and the rim's +/// colour contradict it, by `AGREE.0` to `AGREE.1` stops: it is another surface's, like bright foliage below a pale +/// sky, whose green would turn the sky teal. (Either alone can be wrong: a pixel's own channels are noisy and +/// can't speak for the channels that clipped, and the rim can be another surface, like a white window frame +/// around a bright view.) +const AGREE: (f32, f32) = (0.15, 0.4); +/// On the way from the first channel clipping to the last, a partly clipped pixel's brightness goes over to the +/// clipped neutral's in the second half, and so does the colour of a strongly coloured one (saturation relative to +/// the white above `PALE.1`). A pale one's colour (below `PALE.0`) goes over from the start: the few per cent of +/// colour left in a pale sky once a channel clipped is as much the estimate's error as the sky's, and darkening +/// shows it. +const PALE: (f32, f32) = (0.2, 0.4); #[inline] fn smoothstep(e0: f32, e1: f32, x: f32) -> f32 { @@ -253,6 +270,35 @@ fn push(levels: &mut [Level], empty: [f32; N], needed: Option } } +/// How many stops two chromaticities differ by in any channel ratio. +fn unlike(a: [f32; 3], b: [f32; 3]) -> f32 { + let ratio = |i: usize, j: usize| (a[i].max(1e-6) * b[j].max(1e-6)) / (a[j].max(1e-6) * b[i].max(1e-6)); + [(0, 1), (0, 2), (1, 2)].iter().map(|&(i, j)| ratio(i, j).log2().abs()).fold(0.0f32, f32::max) +} + +/// How far channel `c` of a pixel (sensor values `p`, clipped channels `cl`) only bounds its true value from below: +/// fully once clipped, and increasingly from `FLOOR_LO` of the clip on, so that nothing compared with it jumps where +/// it clips. +fn bound(p: [f32; 3], cl: [bool; 3], clip: f32, c: usize) -> f32 { + if cl[c] { 1.0 } else { smoothstep(FLOOR_LO, 1.0, p[c] / clip) } +} + +/// How many stops the channels of a partly clipped pixel (sensor values `p`, white-balanced `q`, clipped channels +/// `cl`) contradict chromaticity `r`: how far the levels they imply spread, where a clipped channel's only bounds the +/// level from below (see [`bound`]). 0 for the pixel's own colour, whatever its level. +fn contradiction(p: [f32; 3], q: [f32; 3], cl: [bool; 3], r: [f32; 3], clip: f32) -> f32 { + let (mut lo, mut hi) = (f32::INFINITY, 0f32); + for c in 0..3 { + let level = q[c] / r[c].max(1e-3); + hi = hi.max(level); + let w = bound(p, cl, clip, c); + if r[c] > 1e-3 && w < 1.0 { + lo = lo.min(level / (1.0 - w)); + } + } + if hi > 0.0 && lo > 0.0 && lo.is_finite() { (hi / lo).log2().max(0.0) } else { 0.0 } +} + /// The white-balanced colour of a partly clipped pixel (sensor values `p`, white-balanced `q`, clipped channels /// `cl`) whose colour is estimated at chromaticity `r`: clipped channels rebuilt, unclipped ones kept. `None` /// when no unclipped channel can tell the level. @@ -273,10 +319,11 @@ fn rebuild(p: [f32; 3], q: [f32; 3], cl: [bool; 3], r: [f32; 3], clip: f32, max_ } // Two clipped channels must not be left at their clip levels: those differ by the white-balance gains (green // clips first), and the pair would show as magenta or red. They keep the estimate's ratio instead, at the level - // that lifts each to its clip (no channel above the clipped neutral). + // that lifts each to its clip (no channel above the clipped neutral), and so does a channel about to clip (the + // same floor, so nothing jumps where it clips). let floor = (0..3).filter(|&c| cl[c]).map(|c| q[c] / r[c].max(1e-3)).fold(0.0f32, f32::max); let top = r[0].max(r[1]).max(r[2]).max(1e-3); - let sum = (sum / k as f32).max(near_clip).max(floor.min(max_level / top)); + let sum = (sum / k as f32).max(near_clip.max(floor).min(max_level / top)); Some(std::array::from_fn(|c| if cl[c] { q[c].max(sum * r[c]) } else { q[c] })) } @@ -343,20 +390,16 @@ pub fn reconstruct_with(img: &mut Rgb32f, wb: [f32; 3], clip: f32, white: [f32; }); } let Some(field) = levels.first() else { return count }; - // the rim, only where some clipped pixel lacks reliable colour (level 0 of its pyramid holds the estimate) - let untrusted = field.cells.par_iter().enumerate().any(|(i, c)| c[2] < 1.0 && needed(i % field.w, i / field.w)); - let rims = untrusted - .then(|| { - let mut levels = vec![rim(img, wb, clip, &needed)]; - while let Some(l) = levels.last().filter(|l| l.w > 1 || l.h > 1) { - let next = pull(l); - levels.push(next); - } - push(&mut levels, [0.0; 4], None); - levels - }) - // a rim anywhere at all (else the neutral guess applies throughout) - .filter(|levels| levels.last().is_some_and(|top| top.cells.iter().any(|c| c[3] > 0.0))); + // the rim (level 0 of its pyramid holds the estimate), if there is one anywhere at all + let rims = { + let mut levels = vec![rim(img, wb, clip, &needed)]; + while let Some(l) = levels.last().filter(|l| l.w > 1 || l.h > 1) { + let next = pull(l); + levels.push(next); + } + push(&mut levels, [0.0; 4], None); + Some(levels).filter(|levels| levels.last().is_some_and(|top| top.cells.iter().any(|c| c[3] > 0.0))) + }; let rim_field = rims.as_ref().and_then(|l| l.first()); let max_level = wb.iter().cloned().fold(0.0f32, f32::max) * clip; img.data.par_chunks_mut(w).enumerate().for_each(|(y, row)| { @@ -368,22 +411,51 @@ pub fn reconstruct_with(img: &mut Rgb32f, wb: [f32; 3], clip: f32, white: [f32; } let q = [p[0] * wb[0], p[1] * wb[1], p[2] * wb[2]]; // rebuilt with the colour of the reliable content around, and with neutral, blended by how much of it - // there is (blending the results, not the colours, keeps the estimate consistent with the pixel's own - // unclipped channels all the way) + // there is and how well the pixel agrees with it (blending the results, not the colours, keeps the + // estimate consistent with the pixel's own unclipped channels all the way) let e = sample(field, 2.0, x, y); - let trust = e[2].clamp(0.0, 1.0); - let local = (trust > 0.0).then(|| rebuild(p, q, cl, [e[0], 1.0 - e[0] - e[1], e[1]], clip, max_level)).flatten(); - let plain = (trust < 1.0).then(|| rebuild(p, q, cl, neutral, clip, max_level)).flatten().map(|b| { - // only as far as the pixel lies past the rim nearest its clip (in every unclipped channel) - let Some(rim) = rim_field else { return b }; - let edge = sample(rim, 4.0, x, y); - let past = (0..3) - .filter(|&c| !cl[c]) - .map(|c| if edge[c] > 1e-6 && q[c] > 1e-6 { (q[c] / edge[c]).log2().abs() } else { f32::INFINITY }) - .fold(0.0f32, f32::max); + let borrowed = [e[0], 1.0 - e[0] - e[1], e[1]]; + let edge = rim_field.map(|rim| sample(rim, 4.0, x, y)); + let mut trust = e[2].clamp(0.0, 1.0); + if trust > 0.0 { + let mut off = contradiction(p, q, cl, borrowed, clip); + if off > AGREE.0 { + off = off.min(edge.map_or(0.0, |edge| { + let s = edge[0] + edge[1] + edge[2]; + if s > 1e-6 { unlike([edge[0] / s, edge[1] / s, edge[2] / s], borrowed) } else { 0.0 } + })); + } + trust *= 1.0 - smoothstep(AGREE.0, AGREE.1, off); + } + // either applies only as far as the pixel lies past the rim nearest its clip (in any channel); nearer, the + // pixel is rebuilt with the rim's colour + let at_rim = edge.and_then(|edge| { + // stops between pixel and rim in each channel; a clipped channel's true value lies at least as far + // above the rim's as its clip, and no further below (see [`bound`]) + let ratio = (0..3) + .map(|c| { + let (a, b) = (q[c].max(1e-12), edge[c].max(1e-12)); + if a >= b { + return a / b; + } + match bound(p, cl, clip, c) { + w if w <= 0.0 => b / a, + w if w >= 1.0 => 1.0, + w => (b / a).powf(1.0 - w), + } + }) + .fold(1.0f32, f32::max); + let past = ratio.log2().min(64.0); let k = smoothstep(NEAR_RIM, NEAR_RIM + DEPTH, past); - std::array::from_fn(|c| q[c] + (b[c] - q[c]) * k) + let s = edge[0] + edge[1] + edge[2]; + (k < 1.0 && s > 1e-6).then(|| rebuild(p, q, cl, [edge[0] / s, edge[1] / s, edge[2] / s], clip, max_level)).flatten().map(|a| (a, k)) }); + let past_rim = |b: [f32; 3]| -> [f32; 3] { + let Some((a, k)) = at_rim else { return b }; + std::array::from_fn(|c| a[c] + (b[c] - a[c]) * k) + }; + let local = (trust > 0.0).then(|| rebuild(p, q, cl, borrowed, clip, max_level)).flatten().map(past_rim); + let plain = (trust < 1.0).then(|| rebuild(p, q, cl, neutral, clip, max_level)).flatten().map(past_rim); let mut out = match (local, plain) { (Some(a), Some(b)) => std::array::from_fn(|c| b[c] + (a[c] - b[c]) * trust), (Some(a), None) | (None, Some(a)) => a, @@ -399,12 +471,19 @@ pub fn reconstruct_with(img: &mut Rgb32f, wb: [f32; 3], clip: f32, white: [f32; let above = (0..3).filter(|&c| cl[c]).map(|c| (out[c] / q[c].max(1e-9)).max(1.0).log2()).fold(0.0f32, f32::max); let lowest = (0..3).filter(|&c| !cl[c]).map(|c| p[c]).fold(f32::MAX, f32::min); let below = (clip / lowest.max(1e-6)).log2().max(0.0); - let t = if above > 0.0 && lowest < clip { smoothstep(0.5, 1.0, above / (above + below)) } else { 0.0 }; - if t > 0.0 { + let way = if above > 0.0 && lowest < clip { above / (above + below) } else { 0.0 }; + // The brightness goes over in the second half of the way, and so does the colour of a strongly coloured + // pixel; a pale one's goes over from the start ([`PALE`]). + let u = [out[0] / white[0], out[1] / white[1], out[2] / white[2]]; + let (lo, hi) = (u[0].min(u[1]).min(u[2]), u[0].max(u[1]).max(u[2])); + let saturation = if hi > 0.0 && hi.is_finite() { 1.0 - lo.max(0.0) / hi } else { 1.0 }; + let start = 0.5 * smoothstep(PALE.0, PALE.1, saturation); + let (t_level, t_colour) = (smoothstep(0.5, 1.0, way), smoothstep(start, start + 0.5, way)); + if t_colour > 0.0 { let level = (out[0] + out[1] + out[2]) / total; let full = q.iter().cloned().fold(0.0f32, f32::max).max(max_level); - let v = level + (full - level) * t; - out = std::array::from_fn(|c| out[c] + (white[c] * v - out[c]) * t); + let v = level + (full - level) * t_level; + out = std::array::from_fn(|c| out[c] + (white[c] * v - out[c]) * t_colour); } *px = [out[0] / wb[0], out[1] / wb[1], out[2] / wb[2]]; } @@ -450,21 +529,34 @@ mod tests { (img, sky) } + /// How far chromaticity `got` lies off the way from `from` to neutral (largest channel difference to the + /// nearest point of that way). + fn off_the_way_to_neutral(got: [f32; 3], from: [f32; 3]) -> f32 { + let d: [f32; 3] = std::array::from_fn(|c| 1.0 / 3.0 - from[c]); + let len = d.iter().map(|v| v * v).sum::(); + let t = if len > 0.0 { ((0..3).map(|c| (got[c] - from[c]) * d[c]).sum::() / len).clamp(0.0, 1.0) } else { 0.0 }; + (0..3).map(|c| (got[c] - (from[c] + d[c] * t)).abs()).fold(0.0, f32::max) + } + /// Issue #523: the clipped sky beside a branch took the colour of the branch's fringes and of the branch - /// itself (magenta, green), which any darkening of the highlights then showed. + /// itself (magenta, green), which any darkening of the highlights then showed. Beside the branch it is now the + /// clipped sky away from it: the sky's colour, on its way to neutral. #[test] fn fringes_and_dark_branches_do_not_tint_a_clipped_sky() { let (mut img, sky) = branch_scene(); assert!(reconstruct(&mut img, WB, 0.99) > 0); - let want = chroma(sky); let mut worst = 0f32; for y in 40..128 { + let away = chroma(balanced(img.get(30, y))); + assert!(away == chroma(balanced(img.get(170, y))), "row {y}"); + let off = off_the_way_to_neutral(away, chroma(sky)); + assert!(off < 0.005, "row {y}: the clipped sky {away:?} is not the sky's colour on its way to neutral ({off})"); for x in [84, 86, 88, 101, 103, 105] { let got = chroma(balanced(img.get(x, y))); - worst = worst.max((0..3).map(|c| (got[c] - want[c]).abs()).fold(0.0, f32::max)); + worst = worst.max((0..3).map(|c| (got[c] - away[c]).abs()).fold(0.0, f32::max)); } } - assert!(worst < 0.01, "clipped sky beside the branch is off the sky's colour by {worst}"); + assert!(worst < 0.01, "clipped sky beside the branch is off the sky away from it by {worst}"); } /// A warm light with red and green clipped: left at their clip levels the pair reads red-magenta (the white @@ -578,6 +670,51 @@ mod tests { } } + /// Issue #523 (a river photo darkened by Highlights -75): a pale cyan-blue sky (`[1, 1.07, 1.2]` white-balanced) + /// brightening from left to right through the point where green clips (and later blue), above bright + /// yellow-green foliage, with thin dark branches standing in its lower half. Its rebuilt colour used to lean + /// green to teal: the foliage lent its colour to the sky (green rebuilt from it), with a step where green first + /// clips, and the sky kept its own pale colour where Lightroom shows white. + #[test] + fn pale_sky_by_dark_branches_and_foliage_is_not_tinted() { + let sky = [1.0, 1.07, 1.2]; + let (w, h) = (320usize, 96usize); + let k = |x: usize| 0.7 * (1.2 * x as f32 / (w - 1) as f32).exp2(); + let branches = [150usize, 200, 250]; + let on_branch = |x: usize| branches.iter().any(|&b| (b.saturating_sub(1)..=b + 2).contains(&x)); + let img0 = Rgb32f::from_fn(w, h, |x, y| { + sensor(if y >= 72 { + [0.5, 0.6, 0.22] + } else if y >= 40 && branches.iter().any(|&b| x == b || x == b + 1) { + [0.1, 0.09, 0.04] + } else { + sky.map(|v| v * k(x)) + }) + }); + let mut img = img0.clone(); + reconstruct(&mut img, WB, 0.99); + let at = |x: usize, y: usize| chroma(balanced(img.get(x, y))); + let first = (0..w).find(|&x| img0.get(x, 20)[1] >= 0.99).unwrap_or(0); + assert!(first > 0 && first < 150); + let diff = |a: [f32; 3], b: [f32; 3]| (0..3).map(|c| (a[c] - b[c]).abs()).fold(0.0, f32::max); + for y in [20, 50] { + // no step where green clips + let step = diff(at(first - 1, y), at(first, y)); + assert!(step < 0.001, "row {y}: step of {step} where green clips"); + for x in first..w { + // nothing but the sky's colour on its way to neutral (no green from the foliage)... + let off = off_the_way_to_neutral(at(x, y), chroma(sky)); + assert!(on_branch(x) || off < 0.005, "row {y}, x {x}: {:?} is off the sky's way to neutral by {off}", at(x, y)); + // ... the same beside the branches as above them ... + let beside = diff(at(x, y), at(x, 20)); + assert!(on_branch(x) || beside < 0.005, "row {y}, x {x}: {:?} beside a branch, {:?} above", at(x, y), at(x, 20)); + } + // ... and neutral well before the last channel clips (the sky is 0.7 stops past green's clip there) + let end = at(w - 1, y); + assert!(end.iter().all(|v| (v - 1.0 / 3.0).abs() < 0.002), "row {y}: {end:?} at the end"); + } + } + #[test] fn unclipped_pixels_are_left_exactly() { let mut seed = 0x2545_f491_4f6c_dd1du64; diff --git a/docs/parity.md b/docs/parity.md index 3d09ef1a8..ff7587b3c 100644 --- a/docs/parity.md +++ b/docs/parity.md @@ -242,7 +242,7 @@ under the UI command that wraps it (a no-op error); the UI command now wins. | LR-EDIT-HDR-MODE | HDR editing | P2 | ✅ | `cmd:develop.hdr`, `ctl:hdr.maxEv`, `crates/ui-egui/src/panels/edit.rs` (`hdr_row`) | HDR switch in the Light panel; see Q. HDR | | LR-EDIT-LIGHT-EXPOSURE | Exposure | P0 | ✅ | `ctl:light.exposure` | | | LR-EDIT-LIGHT-CONTRAST | Contrast | P0 | ✅ | `ctl:light.contrast` | | -| LR-EDIT-LIGHT-HIGHLIGHTS | Highlights | P0 | ✅ | `ctl:light.highlights`, `crates/raw/src/highlight.rs` | sensor-clipped highlights stay neutral when Highlights (or Exposure, Whites) darkens them: fully clipped content is rebuilt to the neutral of the colour model applied after it, partly clipped content takes its colour from reliable unclipped content nearby (not from dark objects or lens fringes beside it) and white where there is none (#523) | +| LR-EDIT-LIGHT-HIGHLIGHTS | Highlights | P0 | ✅ | `ctl:light.highlights`, `crates/raw/src/highlight.rs` | sensor-clipped highlights stay neutral when Highlights (or Exposure, Whites) darkens them: fully clipped content is rebuilt to the neutral of the colour model applied after it, partly clipped content takes its colour from reliable unclipped content nearby (not from dark objects, lens fringes or another surface beside it) and white where there is none, and pale partly clipped content (a white sky) turns white on its way to clipping (#523) | | LR-EDIT-LIGHT-SHADOWS | Shadows | P0 | ✅ | `ctl:light.shadows` | | | LR-EDIT-LIGHT-WHITES | Whites | P0 | ✅ | `ctl:light.whites` | | | LR-EDIT-LIGHT-BLACKS | Blacks | P0 | ✅ | `ctl:light.blacks` | |