diff --git a/crates/engine/src/denoise.rs b/crates/engine/src/denoise.rs index 30df142ca..2d39d15d2 100644 --- a/crates/engine/src/denoise.rs +++ b/crates/engine/src/denoise.rs @@ -1008,16 +1008,18 @@ impl Session { } /// Make the index of ready photos from the products on disk (a directory listing and a hash per photo: no file is - /// opened). Pictures that appeared or vanished drop the decoded sources they affect. + /// opened). Pictures that appeared or vanished drop the decoded sources they affect. The photo being made is its + /// job's to report (see [`Self::denoise_check_photo`]). pub(crate) fn denoise_reindex(&mut self) { self.denoise.dirty = false; self.denoise.reindexed = Some((self.catalog.revision, web_time::Instant::now())); + let running = self.denoise.running.as_ref().map(|r| r.photo); let mut now: HashMap = HashMap::new(); if let (Some(dir), Some(active)) = (self.denoise_products_dir(), self.denoise.active.as_ref()) { let names: HashSet = products_in(&dir).into_iter().filter_map(|(p, _, _)| p.file_stem().and_then(|s| s.to_str()).map(str::to_string)).collect(); if !names.is_empty() { - for p in self.catalog.photos().filter(|p| eligible(p)) { + for p in self.catalog.photos().filter(|p| eligible(p) && running != Some(p.id)) { let key = product_key(&active.fingerprint, p); if names.contains(&key) { now.insert(p.id, DenoiseSpec { product: product_path(&dir, &key), key, make: None }); @@ -1046,7 +1048,14 @@ impl Session { } /// A photo whose file changed (its content key is not the one its index entry was made for) has no picture. + /// + /// The photo whose picture is being made is left to its job: the worker saves the picture before it reports, so the + /// file is on disk while the job still counts as running. Finding it here would make the photo Ready while the session + /// is busy with it (#719); [`Self::denoise_poll`] makes it Ready in the pump that takes the job's result instead. fn denoise_check_photo(&mut self, id: PhotoId) { + if self.denoise.running.as_ref().is_some_and(|r| r.photo == id) { + return; + } let Some(p) = self.catalog.photo(id).cloned() else { return }; let Some((spec, _)) = self.denoise_spec(&p) else { return }; match self.media.denoise.spec(id) { @@ -1399,6 +1408,12 @@ impl Session { self.denoise.made } + /// The running job's product path and key (tests stand in for the worker's save). + #[cfg(test)] + pub(crate) fn denoise_running_product(&self) -> Option<(PathBuf, String)> { + self.denoise.running.as_ref().map(|r| (r.product.clone(), r.key.clone())) + } + /// What an export of photo `id` with `settings` needs for its Denoise amount: its denoised picture, made now when the /// queue has not got to it. `Ok(None)` when denoise does not apply (amount 0, not a Bayer raw), `Err` when it should /// have and cannot (no model chosen, nowhere to keep the picture, the model failing). diff --git a/crates/engine/src/tests_denoise.rs b/crates/engine/src/tests_denoise.rs index a7d1c7c77..ae202f192 100644 --- a/crates/engine/src/tests_denoise.rs +++ b/crates/engine/src/tests_denoise.rs @@ -720,6 +720,52 @@ fn a_photo_waits_for_its_turn_unless_pictures_are_not_made_on_their_own_or_have_ let _ = std::fs::remove_dir_all(&x.dir); } +#[test] +fn a_picture_that_appears_while_its_job_runs_is_ready_only_when_the_job_reports() { + // #719: the worker saves the picture, then tears down its thread pool before it reports. A pump in that window + // used to find the file and mark the photo Ready while the session still counted the job as running: Ready and + // busy at once, and the activity row stayed up for a photo that said it was done. + use std::sync::{Mutex, mpsc}; + use std::time::Duration; + let mut x = setup("ready-while-running", true); + let id = photo(&x.s); + set_amount(&mut x.s, 80.0); + // the job loads the model afresh and waits in the loader until it is released: a worker that is at work + *x.s.denoise.active.as_ref().unwrap().model.lock().unwrap() = None; + let (entered_tx, entered_rx) = mpsc::channel(); + let (release_tx, release_rx) = mpsc::channel::<()>(); + let release_rx = Mutex::new(release_rx); + let runs = x.runs.clone(); + x.s.denoise.loader = Arc::new(move |_, _| { + entered_tx.send(()).map_err(|e| e.to_string())?; + release_rx.lock().unwrap().recv_timeout(Duration::from_secs(10)).map_err(|e| e.to_string())?; + Ok(Arc::new(Dim { runs: runs.clone() }) as Arc) + }); + x.s.execute("denoise.pump", &json!({"pace": "full"})).unwrap(); + entered_rx.recv_timeout(Duration::from_secs(10)).unwrap(); + assert!(matches!(x.s.denoise_photo_state(id), PhotoState::Running { .. })); + assert!(x.s.denoise_busy()); + // the picture lands on disk before the worker says so (as it does between its save and its report) + let (product, key) = x.s.denoise_running_product().unwrap(); + let mut rgb = lightcraft_raster::Rgb32f::new(192, 160); + rgb.data.fill([0.25, 0.25, 0.25]); + lightcraft_denoise::product::write(&product, &rgb, &key).unwrap(); + assert!(lightcraft_denoise::product::is_current(&product, &key)); + x.s.denoise.touch(); // the index is also rebuilt from the folder in this window + let r = x.s.execute("denoise.pump", &json!({"pace": "full"})).unwrap(); + assert!(matches!(x.s.denoise_photo_state(id), PhotoState::Running { .. }), "the job reports its own picture"); + assert_eq!(r["ready"], 0, "{r}"); + assert!(x.s.denoise_busy()); + // the worker reports: ready and idle in the same pump + release_tx.send(()).unwrap(); + let r = pump_until_ready(&mut x.s, 1); + assert_eq!(x.s.denoise_photo_state(id), PhotoState::Ready); + assert_eq!(r["running"], Value::Null, "{r}"); + assert!(!x.s.denoise_busy()); + drop(x.s); + let _ = std::fs::remove_dir_all(x.dir); +} + #[test] fn the_denoise_switch_validates_arguments_and_accepts_explicit_targets_without_selection() { let mut x = setup("toggle", false);