Merge PR #694

# Conflicts:
#	crates/gpu/src/backend.rs
#	docs/parity-checklist.md

crates/gpu/src/backend.rs

@@ -172,29 +172,71 @@ pub(crate) fn env_off() -> bool {
 }
 
 static MARKER: Mutex<Option<PathBuf>> = Mutex::new(None);
+/// Scopes of driver work (device creation, shader and pipeline compilation) running now, so
+/// that overlapping ones (the window-side warm-up and a first denoise) share one marker file.
+static ACTIVE: Mutex<u32> = Mutex::new(0);
 
-/// Write this file before the compute device is created and remove it once creation returned
-/// (successfully or not). A file still there at the next launch means the process died inside
-/// the driver: see [`take_init_marker`]. `None` (the default) writes nothing.
+/// Write this file before the compute device is created or any shader / pipeline is compiled
+/// (the kernels, the denoise network's), and remove it once that returned (successfully or not).
+/// A file still there at the next launch means the process died inside the driver — e.g. Apple's
+/// Metal compiler (`MTLCompilerService`) aborting on a kernel, issue #250: see
+/// [`take_init_marker`]. `None` (the default) writes nothing.
 pub fn set_init_marker(path: Option<PathBuf>) {
     *MARKER.lock().unwrap_or_else(|e| e.into_inner()) = path;
 }
 
-/// Run device creation `f` between writing and removing the init marker (if one is set).
-pub(crate) fn with_init_marker<T>(backends: Backends, f: impl FnOnce() -> T) -> T {
-    let marker = MARKER.lock().unwrap_or_else(|e| e.into_inner()).clone();
-    if let Some(m) = &marker {
-        if let Some(d) = m.parent() {
+/// The marker's text: what started, and when.
+fn marker_text(what: &str, secs: u64) -> String {
+    format!("GPU {what} started (unix time {secs})\n")
+}
+
+/// Enter a driver-work scope on `marker`: the first one writes the file.
+fn arm(marker: &std::path::Path, active: &Mutex<u32>, what: &str) {
+    let mut n = active.lock().unwrap_or_else(|e| e.into_inner());
+    if *n == 0 {
+        if let Some(d) = marker.parent() {
             let _ = std::fs::create_dir_all(d);
         }
         let secs = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).map(|d| d.as_secs()).unwrap_or(0);
-        let _ = std::fs::write(m, format!("GPU device creation started (backends {backends:?}, unix time {secs})\n"));
+        let _ = std::fs::write(marker, marker_text(what, secs));
     }
-    let r = f();
+    *n += 1;
+}
+
+/// Leave a scope: the last one removes the file.
+fn disarm(marker: &std::path::Path, active: &Mutex<u32>) {
+    let mut n = active.lock().unwrap_or_else(|e| e.into_inner());
+    *n = n.saturating_sub(1);
+    if *n == 0 {
+        let _ = std::fs::remove_file(marker);
+    }
+}
+
+/// Removes the marker on the way out, a panic included (the process survived).
+struct Armed(Option<PathBuf>);
+
+impl Drop for Armed {
+    fn drop(&mut self) {
+        if let Some(m) = &self.0 {
+            disarm(m, &ACTIVE);
+        }
+    }
+}
+
+/// Run driver work `f` (`what`: "device creation", "kernel compilation"…) between writing and
+/// removing the init marker (if one is set).
+pub(crate) fn with_marker<T>(what: &str, f: impl FnOnce() -> T) -> T {
+    let marker = MARKER.lock().unwrap_or_else(|e| e.into_inner()).clone();
     if let Some(m) = &marker {
-        let _ = std::fs::remove_file(m);
+        arm(m, &ACTIVE, what);
     }
-    r
+    let _armed = Armed(marker);
+    f()
+}
+
+/// Run device creation (and the kernel build that follows it) `f` under the init marker.
+pub(crate) fn with_init_marker<T>(backends: Backends, f: impl FnOnce() -> T) -> T {
+    with_marker(&format!("device and kernel creation (backends {backends:?})"), f)
 }
 
 /// If the init marker `path` is present — the last process died while creating the GPU device —
@@ -310,6 +352,9 @@ mod tests {
     }
     use super::*;
 
+    /// The tests that set the process-wide marker take turns.
+    static GLOBAL: Mutex<()> = Mutex::new(());
+
     #[test]
     fn parses_backend_names() {
         assert_eq!(parse_backends("dx12"), Some(BackendChoice::Use(Backends::DX12)));
@@ -376,6 +421,7 @@ mod tests {
 
     #[test]
     fn init_marker_is_written_during_creation_and_removed_after() {
+        let _turn = GLOBAL.lock().unwrap_or_else(|e| e.into_inner());
         let dir = std::env::temp_dir().join(format!("lc-gpu-marker-{}", std::process::id()));
         let m = dir.join("gpu-init.marker");
         let _ = std::fs::remove_dir_all(&dir);
@@ -385,7 +431,7 @@ mod tests {
         assert!(seen, "the marker exists while the device is created");
         assert!(!m.exists(), "and is removed afterwards");
         // a marker left behind by a crashed process is reported once; reading it leaves it in place
-        std::fs::write(&m, "GPU device creation started (backends DX12)").unwrap();
+        std::fs::write(&m, "GPU device and kernel creation started (backends DX12)").unwrap();
         assert!(read_init_marker(&m).unwrap().contains("DX12"));
         assert!(m.exists(), "read_init_marker leaves the marker for the next launch");
         assert!(take_init_marker(&m).unwrap().contains("DX12"));
@@ -433,4 +479,41 @@ mod tests {
         // `glow` only concerns the window: compute keeps its default, without a warning
         assert_eq!(parse_backends("glow"), Some(BackendChoice::Auto));
     }
+
+    #[test]
+    fn overlapping_scopes_share_one_marker_until_the_last_ends() {
+        let dir = std::env::temp_dir().join(format!("lc-gpu-scopes-{}", std::process::id()));
+        let m = dir.join("gpu-init.marker");
+        let _ = std::fs::remove_dir_all(&dir);
+        let active = Mutex::new(0);
+        arm(&m, &active, "device creation");
+        assert!(read_init_marker(&m).unwrap().contains("device creation"));
+        // a second scope (a denoise network's kernels) keeps the first's text and the file
+        arm(&m, &active, "denoise kernel compilation");
+        assert!(read_init_marker(&m).unwrap().contains("device creation"));
+        disarm(&m, &active);
+        assert!(m.exists(), "the first scope is still running");
+        disarm(&m, &active);
+        assert!(!m.exists(), "the last scope removes it");
+        // a stray extra disarm does not underflow or break the next scope
+        disarm(&m, &active);
+        arm(&m, &active, "kernel compilation");
+        assert!(m.exists());
+        disarm(&m, &active);
+        let _ = std::fs::remove_dir_all(&dir);
+    }
+
+    #[test]
+    fn a_panic_inside_the_scope_still_removes_the_marker() {
+        let _turn = GLOBAL.lock().unwrap_or_else(|e| e.into_inner());
+        let dir = std::env::temp_dir().join(format!("lc-gpu-panic-{}", std::process::id()));
+        let m = dir.join("gpu-init.marker");
+        let _ = std::fs::remove_dir_all(&dir);
+        set_init_marker(Some(m.clone()));
+        let r = std::panic::catch_unwind(|| with_marker("kernel compilation", || panic!("compile")));
+        set_init_marker(None);
+        assert!(r.is_err());
+        assert!(!m.exists(), "the process survived, so nothing is left to report");
+        let _ = std::fs::remove_dir_all(&dir);
+    }
 }

crates/gpu/src/ctx.rs

@@ -323,7 +323,8 @@ impl Gpu {
             log::error!("gpu: device lost ({reason:?}): {msg}");
             crate::mark_broken(&format!("device lost ({reason:?}): {msg}"));
         });
-        let scope = device.push_error_scope(wgpu::ErrorFilter::Validation);
+        // a kernel the driver's compiler refuses is a validation or an internal error: no GPU, not a half-built one
+        let scopes = [device.push_error_scope(wgpu::ErrorFilter::Validation), device.push_error_scope(wgpu::ErrorFilter::Internal)];
         let consts = constants();
         let mut kernels = HashMap::new();
         for m in MODULES {
@@ -349,7 +350,9 @@ impl Gpu {
                 kernels.insert(*e, Kernel { pipeline, layout: layout.clone(), nbuf: m.bindings.len() });
             }
         }
-        if let Some(e) = pollster::block_on(scope.pop()) {
+        let [validation, internal] = scopes;
+        let (internal, validation) = (pollster::block_on(internal.pop()), pollster::block_on(validation.pop()));
+        if let Some(e) = internal.or(validation) {
             log::error!("gpu: kernels failed to build on {}: {e}", info.name);
             return Err(format!("{}: kernels failed to build ({e})", info.name));
         }

crates/gpu/src/lib.rs

@@ -226,9 +226,13 @@ pub(crate) fn device() -> Option<&'static ctx::Gpu> {
     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, || {
+        let r = backend::with_init_marker(backends, || {
             std::panic::catch_unwind(|| ctx::Gpu::new(backends)).unwrap_or_else(|_| Err("device creation panicked".into()))
-        })
+        });
+        if let Err(why) = &r {
+            log::warn!("gpu: rendering on the CPU: {why}");
+        }
+        r
     })
     .as_ref()
     .ok()

crates/gpu/src/nn.rs

@@ -672,7 +672,8 @@ pub fn runner(net: &Net, tile: usize) -> Result<NetRunner, String> {
         d.device.push_error_scope(wgpu::ErrorFilter::Validation),
         d.device.push_error_scope(wgpu::ErrorFilter::Internal),
     ];
-    let pipes = build_pipelines(d, &layout);
+    // the driver compiles the kernels here: a crash in it is reported at the next launch (issue #250)
+    let pipes = crate::backend::with_marker("denoise kernel compilation", || build_pipelines(d, &layout));
     let weights: Vec<(wgpu::Buffer, wgpu::Buffer)> = layers
         .iter()
         .map(|l| {

docs/gpu-pipeline.md

@@ -73,16 +73,20 @@ hosts, tests) waits for its running jobs the same way (`JobPool::shutdown`, `cra
 
 **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
-returns, successfully or not. If the marker is still there at the next launch, the process died inside
+`~/.config/lightcraft`) just before the compute device is created and its kernels are compiled (and, on the first
+AI denoise, before that network's kernels are) and removes it as soon as that returns, successfully or
+not (issue #250: Apple's Metal compiler, `MTLCompilerService`, aborting on a kernel on an older Intel
+Mac). A kernel the driver refuses without crashing is caught by wgpu error scopes: the log says why
+(`gpu: rendering on the CPU: …`) and renders stay on the CPU. `LIGHTCRAFT_GPU=0` (or Settings ▸
+uncheck *Use the GPU for rendering*) forces the CPU path from the start. If the marker is still there at the next launch, the process died inside
 the driver: LightCraft starts with GPU rendering off (the preference is saved unchecked), removes the
 marker and says so in a notice. Checking *Use the GPU for rendering* again tries the GPU once more
 (and re-arms the sentinel). Not with `LIGHTCRAFT_NO_PREFS` (tests, scripts), and not in a `--memory` session,
 which writes nothing: it neither arms the sentinel nor removes a marker it finds (it still starts with GPU
 rendering off when one is there, and the next ordinary launch reports and clears it). Killing the app during
 the ~0.3 s of device creation, or two instances starting at the same moment, can trip it falsely —
-harmless: rendering is then on the CPU until the box is checked again. The sentinel only covers the
-compute device; a crash while the window's renderer starts is avoided by the backend defaults above
+harmless: rendering is then on the CPU until the box is checked again. The sentinel covers the compute
+device and every kernel build; a crash while the window's renderer starts is avoided by the backend defaults above
 or worked around with `LIGHTCRAFT_GPU_BACKEND`.
 
 **The window's renderer and its fallback (Windows).** Before the window opens, each renderer is probed in

docs/parity-checklist.md

@@ -426,7 +426,7 @@ differences and missing bindings are in [ui-parity.md](ui-parity.md#shortcuts).
 | LR-PREF-ACCOUNT | Account | OOS | 🚫 | | |
 | LR-PREF-INTERFACE | Interface options | P1 | ✅ | `cmd:app.settings` | filmstrip names/badges, grid badges (auto/always/never), square-grid names, navigator, info overlay |
 | LR-PREF-DISPLAY | Monitor (display) colour management | P1 | 🟡 | `cmd:app.displayProfile`, `crates/codecs/src/display.rs`, `crates/engine/src/display.rs`, `crates/ui-egui/src/panels/settings.rs`, `docs/display-profiles.md` | Settings ▸ Display: a monitor ICC profile (matrix/TRC exactly, LUT-based through the CMS) chosen by hand; the loupe, Before, Compare, hover previews and the second window render into the display's primaries (its full gamut, CPU and GPU), thumbnails and stand-ins are converted from sRGB; histogram, preview caches and exports stay sRGB; soft proofing's display gamut warning uses the display's gamut. Missing: picking up the profile the OS assigns (X11 `_ICC_PROFILE`, colord, ColorSync, WCS), per-monitor profiles |
-| LR-PREF-PERFORMANCE | GPU / performance | P1 | ✅ | `cmd:app.gpu`, `cmd:app.memoryBudget`, `cmd:app.settings` | GPU on/off, preview size (Automatic = the whole-frame loupe render at canvas size, up to 2560 px; or a 1600–5120 px size for it; zoomed views add a window at up to 100 %, issue #323), memory budget (a quarter of it is the idle per-view stage caches' share; the open photo's views are counted and reported, and cut only past four times that, the before side's and the zoom window's first — a Before/After at 1:1 on a 2800 px canvas holds about 1 GB of GPU stages, plus the one 288 MB copy of the original, which `ui.inspect` reports as `sharedSourceBytes`), thumbnail cache size in Settings |
+| LR-PREF-PERFORMANCE | GPU / performance | P1 | ✅ | `cmd:app.gpu`, `cmd:app.memoryBudget`, `cmd:app.settings` | GPU on/off (a crash while the GPU device or its kernels are built, issue #250, turns it off for the next launch), preview size (Automatic = the whole-frame loupe render at canvas size, up to 2560 px; or a 1600–5120 px size for it; zoomed views add a window at up to 100 %, issue #323), memory budget (a quarter of it is the idle per-view stage caches' share; the open photo's views are counted and reported, and cut only past four times that, the before side's and the zoom window's first — a Before/After at 1:1 on a 2800 px canvas holds about 1 GB of GPU stages, plus the one 288 MB copy of the original, which `ui.inspect` reports as `sharedSourceBytes`), thumbnail cache size in Settings |
 | LR-PREF-PEOPLE | Face recognition | P2 | 🟡 | `cmd:faces.enable`, `cmd:faces.models.list`, `cmd:faces.models.inspect`, `cmd:faces.models.install`, `cmd:faces.models.download`, `cmd:faces.models.downloads`, `cmd:faces.models.downloadCancel`, `cmd:faces.models.remove`, `cmd:faces.models.select`, `cmd:dialog.faceModel`, `crates/faces/src/lib.rs`, `crates/ui-egui/src/panels/faces.rs` | Settings ▸ Faces: an off-by-default toggle and the face model list. Models are opt-in and never bundled, the 232 KB YuNet detector included. Press Download (terms first; then a pinned address, size and SHA-256 checked, fetched over pure-Rust https with `lightcraft-fetch`), or add any `.onnx` by picking or dropping it: it is recognised by hash or described from its shape, shows its licence terms and training-data note, runs a self-test, and installs only after an "I accept" box; an installed model is chosen and recognition switched on at once (the newest wins, each model keeps its own embedding cache). Non-commercial models are add-by-file only, never offered for download. Suggestions of names run on the shared pure-Rust CPU interpreter (`crates/faces/src/net.rs`, `crates/faces/src/runtime.rs`; bounded im2col/GEMM, checked ONNX shapes/operators; no external inference runtime; 40 public crops agree within 2.1e-6, one-thread SFace 34–36 ms / AuraFace 233–252 ms, see docs/faces.md) in a background worker and are never applied automatically. Thresholds are starting points calibrated on busts, not yet on live faces; the Recognise faces switch is off until a recognition model is chosen and installed (`faces.enable {enabled: true}` refuses otherwise; removing the last recogniser switches it off), issue #678 |
 | LR-PREF-WATERMARK | Watermark settings | P1 | ✅ | `cmd:export.savePreset`, `crates/ui-egui/src/panels/dialogs.rs` | set in the Export dialog; kept with Export with Previous and in saved export presets |
 | LR-PREF-SHORTCUTS | Shortcut customisation | — | ✅ | `cmd:app.setShortcut`, `cmd:app.resetShortcuts`, `crates/ui-egui/src/panels/keymap.rs` | an extra (the reference app has no keymap editor): Help ▸ Keyboard Shortcuts (⌘/) lists every command; click a shortcut and press the new keys (Esc cancels), × removes, ↺ restores, Reset All; a key another command had moves over; saved in `ui.json`; native menu accelerators follow. Rating/label digits and secondary keys are listed read-only and yield to a key given to a command |