diff --git a/crates/engine/src/cmd/face_recognize.rs b/crates/engine/src/cmd/face_recognize.rs index e099f13ec..e6e850119 100644 --- a/crates/engine/src/cmd/face_recognize.rs +++ b/crates/engine/src/cmd/face_recognize.rs @@ -275,7 +275,7 @@ mod imp { } let job = s.preview_job(id, EDGE, EDGE, false, &DevelopSettings::default())?; let preview = s.scan_preview_of(&photo); - Some(Prepared { tag, epoch: s.faces.epoch, id, job, preview, todo, detect, detector }) + Some(Prepared { tag, epoch: s.faces.epoch, id, content: crate::media::content_key(&photo), job, preview, todo, detect, detector }) } /// The faces the detector found in a photo that had none become its face regions: unnamed, marked as detected (so a @@ -299,13 +299,16 @@ mod imp { } } - /// Take a finished photo into the index (ignored when it was made for another model or another library). Returns - /// (embedded, failed). + /// Take a finished photo into the index (ignored when it was made for another model or another library, or from + /// a file the photo no longer has: issue #1524). Returns (embedded, failed). fn ingest(s: &mut Session, d: Done) -> (usize, usize) { s.faces.in_flight.remove(&d.id); if d.epoch != s.faces.epoch || d.tag != s.faces.index.tag { return (0, 0); } + if s.catalog.photo(d.id).is_none_or(|p| crate::media::content_key(p) != d.content) { + return (0, 0); + } if d.detect_wanted { match &d.found { Some(found) => { @@ -830,7 +833,8 @@ mod imp { } fn finished(s: &Session, id: PhotoId, found: Option>, results: Vec<(Rect, Option>, Rect)>) -> Done { - Done { tag: s.faces.index.tag.clone(), epoch: s.faces.epoch, id, results, detect_wanted: true, found } + let content = s.catalog.photo(id).map(|p| crate::media::content_key(p)).unwrap_or_default(); + Done { tag: s.faces.index.tag.clone(), epoch: s.faces.epoch, id, content, results, detect_wanted: true, found } } #[test] @@ -915,6 +919,34 @@ mod imp { let _ = std::fs::remove_dir_all(&d); } + /// A Reload that finds the file replaced (an outside edit) gives the photo other pixels: what a worker made from + /// the old file is dropped when it comes back, the faces already embedded from it are forgotten and the photo is + /// searched again (issue #1524). + #[test] + fn faces_made_from_a_replaced_file_are_dropped() { + let (mut s, d) = loaded("replaced"); + let (a, b) = two_photos(&s); + let rect = region(0.2, None).rect; + s.faces.index.insert(a.0, &rect, unit(&[(0, 1.0)])); + s.faces.scanned.insert(a.0); + s.faces.index.insert(b.0, &rect, unit(&[(1, 1.0)])); + s.faces.scanned.insert(b.0); + let old = finished(&s, a, Some(vec![rect]), vec![(rect, Some(unit(&[(2, 1.0)])), rect)]); + let (width, height) = s.catalog.photo(a).map(|p| (p.width, p.height)).unwrap(); + let replace = Op::SetContent { id: a, width, height, file_size: 1234, content_hash: Some("edited".into()), preview_only: None }; + s.commit("Reload Changed Files", replace).unwrap(); + assert!(s.faces.index.get(a.0, &rect).is_none() && !s.faces.scanned.contains(a.0), "the old file's faces are forgotten"); + assert!(needs_search(&s, s.catalog.photo(a).unwrap()), "and the photo is searched again"); + assert!(s.faces.index.get(b.0, &rect).is_some() && s.faces.scanned.contains(b.0), "other photos keep theirs"); + assert_eq!(ingest(&mut s, old), (0, 0), "a result made from the old file is dropped"); + assert!(s.catalog.photo(a).unwrap().meta.regions.is_empty() && !s.faces.scanned.contains(a.0)); + // what is made from the new file is taken in + let new = finished(&s, a, Some(vec![rect]), vec![(rect, Some(unit(&[(3, 1.0)])), rect)]); + assert_eq!(ingest(&mut s, new), (1, 0)); + assert!(s.faces.scanned.contains(a.0) && s.faces.index.get(a.0, &rect).is_some()); + let _ = std::fs::remove_dir_all(&d); + } + #[test] fn the_unnamed_faces_that_look_like_a_person_come_best_first() { let (mut s, d) = loaded("similar"); diff --git a/crates/engine/src/faces_index.rs b/crates/engine/src/faces_index.rs index fd4d9f2e7..83e86b099 100644 --- a/crates/engine/src/faces_index.rs +++ b/crates/engine/src/faces_index.rs @@ -55,6 +55,8 @@ pub(crate) struct Index { skipped: std::collections::HashSet, /// The cache file on disk belongs to this model, so new records can be appended to it. file_matches: bool, + /// Records were dropped ([`Index::forget_photo`]): the cache file is written fresh at the next save. + rewrite: bool, pub path: Option, } @@ -87,6 +89,16 @@ impl Index { self.skipped.insert((photo, region_key(rect))); } + /// Drop every face of `photo` (its file was replaced, so they were made from other pixels): they are embedded + /// again, and the cache file is rewritten without them at the next save. + pub fn forget_photo(&mut self, photo: u64) { + let before = self.entries.len(); + self.entries.retain(|k, _| k.0 != photo); + self.unsaved.retain(|(k, _)| k.0 != photo); + self.skipped.retain(|k| k.0 != photo); + self.rewrite |= self.entries.len() != before; + } + #[cfg(test)] pub fn insert(&mut self, photo: u64, rect: &Rect, embedding: Vec) { self.insert_with_view(photo, rect, embedding, rect); @@ -102,7 +114,7 @@ impl Index { /// Whether there are embeddings not yet written to the cache file. pub fn has_unsaved(&self) -> bool { - !self.unsaved.is_empty() + !self.unsaved.is_empty() || self.rewrite } /// Start over for `tag` (`dim` numbers per face), loading what the cache file at `path` holds for it. What the @@ -116,6 +128,7 @@ impl Index { self.unsaved.clear(); self.skipped.clear(); self.file_matches = false; + self.rewrite = false; if let Some(p) = self.path.clone() { self.load(&p); } @@ -175,17 +188,19 @@ impl Index { r } - /// Write what was added since the last save: appended when the file already belongs to this model, else the - /// whole index is written fresh (atomically). A failure is returned, the records stay pending. + /// Write what was added since the last save: appended when the file already belongs to this model, else (or when + /// records were dropped) the whole index is written fresh (atomically). A failure is returned, the records stay + /// pending. pub fn save(&mut self) -> std::io::Result<()> { let Some(path) = self.path.clone() else { self.unsaved.clear(); + self.rewrite = false; return Ok(()); }; - if self.unsaved.is_empty() { + if self.unsaved.is_empty() && !self.rewrite { return Ok(()); } - if self.file_matches { + if self.file_matches && !self.rewrite { let mut f = std::fs::OpenOptions::new().append(true).open(&path)?; let mut buf = Vec::new(); for (k, e) in &self.unsaved { @@ -201,6 +216,7 @@ impl Index { std::fs::write(&part, &buf)?; std::fs::rename(&part, &path)?; self.file_matches = true; + self.rewrite = false; } self.unsaved.clear(); Ok(()) @@ -228,6 +244,8 @@ pub(crate) struct Scanned { unsaved: Vec, path: Option, file_matches: bool, + /// A photo was taken out ([`Scanned::remove`]): the record is written fresh at the next save. + rewrite: bool, } impl Scanned { @@ -249,8 +267,16 @@ impl Scanned { } } + /// Search `photo` again (its file was replaced). + pub fn remove(&mut self, photo: u64) { + if self.set.remove(&photo) { + self.unsaved.retain(|p| *p != photo); + self.rewrite = true; + } + } + pub fn has_unsaved(&self) -> bool { - !self.unsaved.is_empty() + !self.unsaved.is_empty() || self.rewrite } /// Start over with the record at `path` (written first: what was not yet saved is not lost). @@ -259,6 +285,7 @@ impl Scanned { self.set.clear(); self.unsaved.clear(); self.file_matches = false; + self.rewrite = false; self.path = path; if let Some(p) = self.path.clone() { self.load(&p); @@ -292,12 +319,13 @@ impl Scanned { pub fn save(&mut self) -> std::io::Result<()> { let Some(path) = self.path.clone() else { self.unsaved.clear(); + self.rewrite = false; return Ok(()); }; - if self.unsaved.is_empty() { + if self.unsaved.is_empty() && !self.rewrite { return Ok(()); } - if self.file_matches { + if self.file_matches && !self.rewrite { let mut f = std::fs::OpenOptions::new().append(true).open(&path)?; let buf: Vec = self.unsaved.iter().flat_map(|id| id.to_le_bytes()).collect(); f.write_all(&buf)?; @@ -308,6 +336,7 @@ impl Scanned { std::fs::write(&part, &buf)?; std::fs::rename(&part, &path)?; self.file_matches = true; + self.rewrite = false; } self.unsaved.clear(); Ok(()) @@ -406,6 +435,43 @@ mod tests { assert_eq!(b.len(), 0, "a non-finite vector is not loaded back"); let _ = std::fs::remove_file(path); } + + /// The faces and the search of a photo whose file was replaced are dropped from the files too, not only from memory: + /// they don't come back at the next launch (issue #1524). + #[test] + fn a_forgotten_photo_stays_forgotten_after_reloading() { + let dir = std::env::temp_dir().join(format!("lc-faces-forget-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let (emb, scan) = (dir.join("face-embeddings.bin"), dir.join("face-scanned.bin")); + let mut a = Index::default(); + a.reset("m@abc", 2, Some(emb.clone())); + a.insert(1, &rect(0.1), vec![1.0, 0.0]); + a.insert(2, &rect(0.1), vec![0.0, 1.0]); + a.save().unwrap(); + a.forget_photo(1); + assert!(a.has_unsaved()); + a.save().unwrap(); + a.insert(3, &rect(0.1), vec![1.0, 1.0]); + a.save().unwrap(); + let mut b = Index::default(); + b.reset("m@abc", 2, Some(emb)); + assert!(!b.contains(1, &rect(0.1)) && b.contains(2, &rect(0.1)) && b.contains(3, &rect(0.1))); + let mut s = Scanned::default(); + s.reset(Some(scan.clone())); + s.insert(1); + s.insert(2); + s.save().unwrap(); + s.remove(1); + assert!(s.has_unsaved()); + s.save().unwrap(); + s.insert(3); + s.save().unwrap(); + let mut t = Scanned::default(); + t.reset(Some(scan)); + assert!(!t.contains(1) && t.contains(2) && t.contains(3)); + let _ = std::fs::remove_dir_all(&dir); + } } /// What the recognition commands keep between calls: the loaded model and the embeddings made with it. @@ -456,4 +522,12 @@ impl FacesState { self.saved_at = None; self.epoch = self.epoch.wrapping_add(1); } + + /// Photo `id`'s file was replaced by other content: the faces made from the old pixels are dropped and the photo is + /// searched again (issue #1524). What a worker is still making from the old file is dropped when it comes back. + pub fn content_replaced(&mut self, id: lightcraft_catalog::PhotoId) { + self.index.forget_photo(id.0); + self.scanned.remove(id.0); + self.scan_failed.remove(&id.0); + } } diff --git a/crates/engine/src/faces_worker.rs b/crates/engine/src/faces_worker.rs index 81ff6adae..96bd4f124 100644 --- a/crates/engine/src/faces_worker.rs +++ b/crates/engine/src/faces_worker.rs @@ -134,6 +134,9 @@ pub(crate) struct Prepared { /// The session's face epoch when this was prepared: another library has been opened since if it differs. pub epoch: u64, pub id: PhotoId, + /// The photo's content key ([`crate::media::content_key`]) when this was prepared: its file was replaced (Reload + /// after an outside edit) since if it differs. + pub content: String, pub job: RenderJob, /// A raw's embedded preview, tried before the render job (much faster than decoding the raw). pub preview: Option<(String, PreviewLoader)>, @@ -150,6 +153,7 @@ pub(crate) struct Done { pub tag: String, pub epoch: u64, pub id: PhotoId, + pub content: String, /// Each face's region, its embedding (`None` when it could not be made) and the box to show it by. pub results: Vec<(Rect, Option>, Rect)>, /// Faces were to be looked for in this photo. @@ -177,7 +181,7 @@ fn face_rect(f: &Face) -> Option { /// Do the work for one photo. Never panics: whatever goes wrong, its faces come back as `None`. pub(crate) fn process(p: Prepared, embedder: &Embedder) -> Done { - let Prepared { tag, epoch, id, job, preview, todo, detect, detector } = p; + let Prepared { tag, epoch, id, content, job, preview, todo, detect, detector } = p; let fallback = |todo: Vec| todo.into_iter().map(|r| (r, None, r)).collect::>(); let rects = todo.clone(); let run = catch_unwind(AssertUnwindSafe(|| { @@ -238,7 +242,7 @@ pub(crate) fn process(p: Prepared, embedder: &Embedder) -> Done { (results, new_faces) })); let (results, found) = run.unwrap_or_else(|_| (fallback(rects), None)); - Done { tag, epoch, id, results, detect_wanted: detect, found } + Done { tag, epoch, id, content, results, detect_wanted: detect, found } } type Job = (Arc, Prepared); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 191e2ee07..84d85b3ad 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -558,10 +558,16 @@ impl Session { // Retain only cache IDs while the catalog consumes the operation. Cloning an import // batch here also cloned every photo, although commit already keeps the journal copy. let mut invalidated = Vec::new(); + // photos whose file was replaced: the faces made from the old pixels go too (issue #1524) + let mut replaced = Vec::new(); let mut pending = vec![&op]; while let Some(op) = pending.pop() { match op { - Op::SetFile { id, .. } | Op::Relink { id, .. } | Op::SetContent { id, .. } | Op::RemovePhoto { id } => invalidated.push(*id), + Op::SetContent { id, .. } => { + invalidated.push(*id); + replaced.push(*id); + } + Op::SetFile { id, .. } | Op::Relink { id, .. } | Op::RemovePhoto { id } => invalidated.push(*id), Op::AddPhoto { photo } => invalidated.push(photo.id), Op::Batch { ops } => pending.extend(ops), _ => {} @@ -571,6 +577,12 @@ impl Session { for id in invalidated { self.media.forget(id); } + #[cfg(not(target_arch = "wasm32"))] + for id in replaced { + self.faces.content_replaced(id); + } + #[cfg(target_arch = "wasm32")] + let _ = replaced; Ok(inv) } diff --git a/docs/whats-new.md b/docs/whats-new.md index eff45fb6d..411907029 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -34,6 +34,11 @@ name (`catalog.snap.tmp`, `model.onnx.part`): the link is removed and a new file made in its place, so the file it pointed to is never changed (issue #1502). +### Reliability +- A photo whose file was replaced (Reload Changed Files after an outside edit) no longer keeps the faces recognition + made from the old pixels: they are made again from the new file, and a scan still busy with the old file when it + changed doesn't put its faces on the new one (issue #1524). + ### Hard-edged Heal and Clone spots - A spot with Feather at 0 no longer draws black pixels exactly on its radius. This also fixes spots at 0% Opacity changing pixels along their edge. The hard edge now leaves pixels on the radius unchanged.