diff --git a/crates/engine/src/cmd/query.rs b/crates/engine/src/cmd/query.rs index fbe4608f0..e92c5aef5 100644 --- a/crates/engine/src/cmd/query.rs +++ b/crates/engine/src/cmd/query.rs @@ -139,8 +139,11 @@ pub fn specs() -> Vec { .into_iter() .map(|r| json!({"group": r.group, "tag": r.tag, "name": r.name, "value": r.value})) .collect(); - // XMP: the sidecar if there is one, else the file's own packet - let packet = crate::sidecar::read_packet(path, ph.kind, s.sidecar_naming(ph.id)).map(|(x, _)| x).or_else(|| lightcraft_meta::embedded(&bytes).xmp); + // Keep sidecar precedence; reuse the loaded bytes for embedded XMP. + let packet = match crate::sidecar::find_sidecar(path, s.sidecar_naming(ph.id)) { + Some(sc) => std::fs::read_to_string(sc).ok(), + None => lightcraft_meta::embedded(&bytes).xmp, + }; let xmp: Vec = packet .and_then(|x| lightcraft_meta::parse_xmp(&x).ok()) .map(|d| d.properties.into_iter().filter(|(k, _)| !k.starts_with("lc:")).map(|(k, v)| json!({"name": k, "value": v.join("; ")})).collect()) diff --git a/crates/engine/src/cmd/xmp.rs b/crates/engine/src/cmd/xmp.rs index fb0ec9883..d48ca3cd2 100644 --- a/crates/engine/src/cmd/xmp.rs +++ b/crates/engine/src/cmd/xmp.rs @@ -95,7 +95,7 @@ pub fn specs() -> Vec { "Read Metadata from File", ["Photo"], None, - "{ids?} — reads each photo's XMP sidecar (or a raw/DNG's embedded XMP): metadata and develop settings, one undo step; never writes the sidecar, even with auto-write on → {read, failed, warnings?: [{path, code, profile?, mode?, fields?}]}", + "{ids?} — reads each photo's XMP sidecar, or embedded XMP in raw/DNG and JPEG files when no sidecar exists: metadata and develop settings, one undo step; never writes the sidecar, even with auto-write on; JPEG uses the first complete standard XMP APP1 before SOS/EOI, within 16 MiB of headers and 4096 marker/fill steps; an unreadable sidecar fails without embedded fallback → {read, failed, warnings?: [{path, code, profile?, mode?, fields?}]}", has_selection, read ), diff --git a/crates/engine/src/sidecar.rs b/crates/engine/src/sidecar.rs index ea06d69a7..f3ae190c9 100644 --- a/crates/engine/src/sidecar.rs +++ b/crates/engine/src/sidecar.rs @@ -22,8 +22,10 @@ //! else `photoshop:DateCreated`, else `xmp:CreateDate`) fills in only when the file has none; develop //! settings are restored from `lc:settings` when present, else mapped from interoperable `crs:` //! fields ([`crate::crs`]). For raw/DNG files without a sidecar the file's embedded XMP is used. +//! Manual reads also accept the first complete standard XMP APP1 in a bounded JPEG header (see `read_jpeg_xmp`). use std::collections::HashMap; +use std::io::{Read, Seek, SeekFrom}; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -498,18 +500,77 @@ fn file_path(p: &Photo) -> Option<&str> { } } -/// The XMP packet that describes a photo on disk: its sidecar, else (raw/DNG) the embedded XMP. +/// The XMP packet that describes a photo on disk: its sidecar, else embedded raw/DNG or JPEG XMP. pub fn read_packet(original: &str, kind: MediaKind, naming: SidecarNaming) -> Option<(String, PathBuf)> { - if let Some(sc) = find_sidecar(original, naming) - && let Ok(s) = std::fs::read_to_string(&sc) - { - return Some((s, sc)); + if let Some(sc) = find_sidecar(original, naming) { + // A discovered sidecar keeps precedence even when reading it fails. + return std::fs::read_to_string(&sc).ok().map(|s| (s, sc)); } if kind == MediaKind::Raw { let bytes = std::fs::read(original).ok()?; let x = lightcraft_meta::embedded(&bytes).xmp?; return Some((x, PathBuf::from(original))); } + if kind == MediaKind::Image { + let mut file = std::fs::File::open(original).ok()?; + let x = read_jpeg_xmp(&mut file, JPEG_XMP_HEADER_SPAN, JPEG_XMP_HEADER_STEPS)?; + return Some((x, PathBuf::from(original))); + } + None +} + +const JPEG_XMP_HEADER_SPAN: u64 = 16 * 1024 * 1024; +const JPEG_XMP_HEADER_STEPS: usize = 4096; + +/// Read from SOI at the current position, stopping at the first standard XMP packet or SOS/EOI. +/// The span includes SOI, segment headers and skipped payloads; each marker/fill step after SOI +/// consumes one iteration. Only a complete APP1 payload (at most 65533 bytes) reaches the shared +/// parser, in a single SOI+APP1 wrapper (at most 65539 bytes). This does not validate the whole JPEG. +fn read_jpeg_xmp(reader: &mut (impl Read + Seek), header_span: u64, marker_steps: usize) -> Option { + let mut header = reader.take(header_span); + let mut soi = [0; 2]; + header.read_exact(&mut soi).ok()?; + if soi != [0xff, 0xd8] { + return None; + } + let mut filled = false; + for _ in 0..marker_steps { + if !filled { + let mut prefix = [0]; + header.read_exact(&mut prefix).ok()?; + if prefix != [0xff] { + return None; + } + } + let mut code = [0]; + header.read_exact(&mut code).ok()?; + let [marker] = code; + filled = marker == 0xff; + match marker { + 0xff | 0xd8 | 0xd0..=0xd7 | 0x01 => continue, + 0xd9 | 0xda => return None, + _ => {} + } + let mut length = [0; 2]; + header.read_exact(&mut length).ok()?; + let payload_len = u16::from_be_bytes(length).checked_sub(2)?; + let remaining = header.limit().checked_sub(u64::from(payload_len))?; + if marker != 0xe1 { + // Seeking must spend the same span budget as reading, before the seek is attempted. + header.set_limit(remaining); + header.get_mut().seek(SeekFrom::Current(i64::from(payload_len))).ok()?; + continue; + } + let segment_len = 6usize.checked_add(usize::from(payload_len))?; + let [hi, lo] = length; + let mut segment = Vec::with_capacity(segment_len); + segment.extend_from_slice(&[0xff, 0xd8, 0xff, 0xe1, hi, lo]); + segment.resize(segment_len, 0); + header.read_exact(segment.get_mut(6..)?).ok()?; + if let Some(xmp) = lightcraft_meta::jpeg_segments(&segment).xmp { + return Some(xmp); + } + } None } @@ -899,3 +960,220 @@ mod tests_repair_provenance; #[cfg(test)] #[path = "sidecar/tests_stem_owners.rs"] mod tests_stem_owners; + +#[cfg(test)] +mod jpeg_header_tests { + use super::{JPEG_XMP_HEADER_SPAN, JPEG_XMP_HEADER_STEPS, read_jpeg_xmp}; + use std::io::{self, Cursor, Read, Seek, SeekFrom}; + + struct CountedReader { + cursor: Cursor>, + bytes_read: usize, + largest_read: usize, + seeks: usize, + chunk: usize, + read_error_at: Option, + seek_error: bool, + } + + impl CountedReader { + fn new(bytes: Vec) -> Self { + Self { cursor: Cursor::new(bytes), bytes_read: 0, largest_read: 0, seeks: 0, chunk: usize::MAX, read_error_at: None, seek_error: false } + } + } + + impl Read for CountedReader { + fn read(&mut self, buf: &mut [u8]) -> io::Result { + self.largest_read = self.largest_read.max(buf.len()); + if self.read_error_at.is_some_and(|at| self.cursor.position() >= at) { + return Err(io::Error::other("injected header read failure")); + } + let len = buf.len().min(self.chunk); + let read = self.cursor.read(&mut buf[..len])?; + self.bytes_read += read; + Ok(read) + } + } + + impl Seek for CountedReader { + fn seek(&mut self, pos: SeekFrom) -> io::Result { + self.seeks += 1; + if self.seek_error { + return Err(io::Error::other("injected header seek failure")); + } + self.cursor.seek(pos) + } + } + + fn segment(marker: u8, payload: &[u8]) -> Vec { + let len = u16::try_from(payload.len() + 2).unwrap(); + let mut bytes = vec![0xff, marker]; + bytes.extend_from_slice(&len.to_be_bytes()); + bytes.extend_from_slice(payload); + bytes + } + + fn packet(rating: i8) -> String { + lightcraft_meta::write_xmp(&lightcraft_meta::Metadata { rating: Some(rating), ..Default::default() }, None) + } + + fn xmp_segment(packet: &str) -> Vec { + let mut payload = b"http://ns.adobe.com/xap/1.0/\0".to_vec(); + payload.extend_from_slice(packet.as_bytes()); + segment(0xe1, &payload) + } + + fn jpeg(packet: &str) -> Vec { + let mut bytes = vec![0xff, 0xd8]; + bytes.extend(xmp_segment(packet)); + bytes + } + + #[test] + fn first_standard_packet_skips_other_segments_and_stops_before_tail() { + let first = packet(2); + let first_payload_len = xmp_segment(&first).len() - 4; + let mut bytes = vec![0xff, 0xd8]; + bytes.extend(segment(0xe0, &[7; 17])); + bytes.extend(segment(0xe1, b"Exif\0\0tiny")); + let mut extended = b"http://ns.adobe.com/xmp/extension/\0".to_vec(); + extended.extend_from_slice(&[0; 40]); + extended.extend_from_slice(b"not a standard packet"); + bytes.extend(segment(0xe1, &extended)); + bytes.extend(xmp_segment(&first)); + let end = bytes.len(); + bytes.extend(xmp_segment(&packet(5))); + bytes.extend_from_slice(&[0xff, 0xda, 0, 2]); + bytes.extend_from_slice(&[3; 128]); + let mut reader = CountedReader::new(bytes); + reader.read_error_at = Some(end as u64); + assert_eq!(read_jpeg_xmp(&mut reader, JPEG_XMP_HEADER_SPAN, JPEG_XMP_HEADER_STEPS), Some(first)); + assert_eq!(reader.cursor.position(), end as u64, "later packets and image bytes are not read"); + assert_eq!(reader.bytes_read, end - 17, "APP0 payload is skipped, not buffered"); + assert_eq!(reader.seeks, 1); + assert_eq!(reader.largest_read, first_payload_len, "reads at most one complete APP1 payload at a time"); + } + + #[test] + fn stops_at_sos_or_eoi_without_reading_later_xmp() { + for stop in [0xda, 0xd9] { + let mut bytes = vec![0xff, 0xd8, 0xff, stop]; + bytes.extend(xmp_segment(&packet(3))); + let mut reader = CountedReader::new(bytes); + reader.read_error_at = Some(4); + assert_eq!(read_jpeg_xmp(&mut reader, JPEG_XMP_HEADER_SPAN, JPEG_XMP_HEADER_STEPS), None); + assert_eq!((reader.bytes_read, reader.cursor.position(), reader.seeks), (4, 4, 0)); + } + } + + #[test] + fn rejects_incomplete_app1_even_when_partial_xmp_parses() { + let expected = packet(4); + let mut bytes = jpeg(&expected); + let declared = u16::from_be_bytes([bytes[4], bytes[5]]) + 7; + bytes[4..6].copy_from_slice(&declared.to_be_bytes()); + let partial = lightcraft_meta::jpeg_segments(&bytes).xmp.unwrap(); + assert_eq!(partial, expected, "the shared extractor tolerates a truncated declared payload"); + assert_eq!(super::parse_sidecar(&partial, crate::crs::Target::Rendered).unwrap().rating, Some(4)); + let len = bytes.len(); + let mut reader = CountedReader::new(bytes); + assert_eq!(read_jpeg_xmp(&mut reader, JPEG_XMP_HEADER_SPAN, JPEG_XMP_HEADER_STEPS), None); + assert_eq!(reader.bytes_read, len); + } + + #[test] + fn span_budget_counts_soi_headers_and_skipped_payloads() { + let expected = packet(3); + let mut bytes = vec![0xff, 0xd8]; + bytes.extend(segment(0xe0, &[9; 23])); + bytes.extend(xmp_segment(&expected)); + let end = bytes.len(); + let mut exact = CountedReader::new(bytes.clone()); + assert_eq!(read_jpeg_xmp(&mut exact, end as u64, JPEG_XMP_HEADER_STEPS), Some(expected)); + assert_eq!(exact.cursor.position(), end as u64, "a complete early APP1 does not require EOI"); + assert_eq!(exact.bytes_read, end - 23); + let mut short = CountedReader::new(bytes); + assert_eq!(read_jpeg_xmp(&mut short, (end - 1) as u64, JPEG_XMP_HEADER_STEPS), None); + assert_eq!((short.bytes_read, short.cursor.position(), short.seeks), (10, 33, 1), "reject before APP1 payload read"); + assert_eq!(short.largest_read, 2); + } + + #[test] + fn span_budget_rejects_skip_before_seeking() { + let mut bytes = vec![0xff, 0xd8]; + bytes.extend(segment(0xe0, &[8; 18])); + bytes.extend(xmp_segment(&packet(1))); + let mut reader = CountedReader::new(bytes); + assert_eq!(read_jpeg_xmp(&mut reader, 6, JPEG_XMP_HEADER_STEPS), None); + assert_eq!((reader.bytes_read, reader.cursor.position(), reader.seeks), (6, 6, 0)); + } + + #[test] + fn marker_budget_counts_fill_and_standalone_markers() { + let expected = packet(1); + // Two fill steps, SOI, TEM, RST3, then APP1: six steps after the initial SOI. + let mut bytes = vec![0xff, 0xd8, 0xff, 0xff, 0xff, 0xd8, 0xff, 0x01, 0xff, 0xd3]; + bytes.extend(xmp_segment(&expected)); + for (steps, end) in [(0, 2), (5, 10)] { + let mut reader = CountedReader::new(bytes.clone()); + assert_eq!(read_jpeg_xmp(&mut reader, JPEG_XMP_HEADER_SPAN, steps), None); + assert_eq!((reader.bytes_read, reader.cursor.position()), (end, end as u64)); + } + let mut reader = CountedReader::new(bytes); + assert_eq!(read_jpeg_xmp(&mut reader, JPEG_XMP_HEADER_SPAN, 6), Some(expected)); + } + + #[test] + fn rejects_invalid_headers_and_segment_lengths() { + for bytes in [ + vec![], + vec![0xff], + b"\x89PNG\r\n\x1a\n".to_vec(), + vec![0xff, 0xd8, 0, 0xe1, 0, 2], + vec![0xff, 0xd8, 0xff, 0xe1, 0], + vec![0xff, 0xd8, 0xff, 0xe1, 0, 0], + vec![0xff, 0xd8, 0xff, 0xe1, 0, 1], + vec![0xff, 0xd8, 0xff, 0xe1, 0, 3], + ] { + let len = bytes.len(); + let mut reader = CountedReader::new(bytes); + assert_eq!(read_jpeg_xmp(&mut reader, JPEG_XMP_HEADER_SPAN, JPEG_XMP_HEADER_STEPS), None); + assert!(reader.bytes_read <= len); + } + } + + #[test] + fn propagates_read_and_seek_failures() { + for at in [0, 6] { + let mut reader = CountedReader::new(jpeg(&packet(4))); + reader.read_error_at = Some(at); + assert_eq!(read_jpeg_xmp(&mut reader, JPEG_XMP_HEADER_SPAN, JPEG_XMP_HEADER_STEPS), None); + assert_eq!(reader.bytes_read as u64, at); + } + let mut bytes = vec![0xff, 0xd8]; + bytes.extend(segment(0xe0, &[1])); + bytes.extend(xmp_segment(&packet(4))); + let mut reader = CountedReader::new(bytes); + reader.seek_error = true; + assert_eq!(read_jpeg_xmp(&mut reader, JPEG_XMP_HEADER_SPAN, JPEG_XMP_HEADER_STEPS), None); + assert_eq!((reader.bytes_read, reader.seeks), (6, 1)); + } + + #[test] + fn handles_short_reads_without_bypassing_span_limit() { + let expected = packet(2); + let bytes = jpeg(&expected); + let end = bytes.len(); + let mut exact = CountedReader::new(bytes.clone()); + exact.chunk = 1; + assert_eq!(read_jpeg_xmp(&mut exact, end as u64, JPEG_XMP_HEADER_STEPS), Some(expected)); + assert_eq!(exact.bytes_read, end); + for (span, read) in [(0, 0), (1, 1), (end - 1, 6)] { + let mut reader = CountedReader::new(bytes.clone()); + reader.chunk = 1; + assert_eq!(read_jpeg_xmp(&mut reader, span as u64, JPEG_XMP_HEADER_STEPS), None); + assert_eq!(reader.bytes_read, read); + assert!(reader.cursor.position() <= span as u64); + } + } +} diff --git a/crates/engine/src/tests_xmp.rs b/crates/engine/src/tests_xmp.rs index 231a7bee3..1a4d86fa3 100644 --- a/crates/engine/src/tests_xmp.rs +++ b/crates/engine/src/tests_xmp.rs @@ -621,6 +621,214 @@ fn dng_embedded_crs_settings_are_read_on_import() { let _ = std::fs::remove_dir_all(&src); } +fn jpeg_xmp_packet(rating: i8, title: &str) -> String { + lightcraft_meta::write_xmp(&lightcraft_meta::Metadata { rating: Some(rating), title: Some(title.into()), ..Default::default() }, None) +} + +fn write_jpeg_xmp(path: &Path, packet: &str) -> Vec { + let img = lightcraft_raster::Rgba8::from_fn(32, 24, |x, y| [(x * 7) as u8, (y * 9) as u8, 90, 255]); + let meta = lightcraft_codecs::EncodeMeta { xmp: Some(packet), ..Default::default() }; + let bytes = lightcraft_codecs::encode_jpeg(&lightcraft_codecs::EncodeImage::rgba8(&img), 90, Default::default(), &meta).unwrap(); + assert_eq!(lightcraft_meta::embedded(&bytes).xmp.as_deref(), Some(packet), "the procedural JPEG carries the requested packet"); + std::fs::write(path, &bytes).unwrap(); + bytes +} + +#[test] +fn embedded_jpeg_xmp_reread_refreshes_metadata_and_undoes_once() { + let src = temp_dir("jpeg-xmp-refresh"); + let file = src.join("photo.jpg"); + let initial = write_jpeg_xmp(&file, &jpeg_xmp_packet(1, "Initial embedded title")); + let mut s = Session::new().with_fs(); + let imported = s.execute("library.import", &json!({"paths": [file.to_string_lossy()]})).unwrap(); + assert_eq!(imported["imported"].as_array().unwrap().len(), 1, "{imported}"); + let id = only(&s); + assert_eq!(s.catalog.photo(id).unwrap().kind, lightcraft_catalog::MediaKind::Image); + s.execute("library.select", &json!({"ids": [id.0]})).unwrap(); + s.execute("photo.rate", &json!({"rating": 2})).unwrap(); + s.execute("photo.setMeta", &json!({"title": "Catalog title"})).unwrap(); + let before = s.catalog.photo(id).unwrap().clone(); + assert!(!s.xmp.auto_write); + + // Change the packet after import: rereading must use the file, not the imported metadata. + let changed = write_jpeg_xmp(&file, &jpeg_xmp_packet(4, "Changed embedded title")); + assert_ne!(initial, changed); + assert!(!src.join("photo.xmp").exists() && !src.join("photo.jpg.xmp").exists()); + let read = s.execute("photo.readMetadataFromFile", &json!({"ids": [id.0]})).unwrap(); + assert_eq!(read["read"], json!([{"id": id.0, "from": file.to_string_lossy()}]), "embedded JPEG XMP must be reread: {read}"); + assert_eq!(read["failed"], json!([]), "{read}"); + let photo = s.catalog.photo(id).unwrap(); + assert_eq!((photo.rating, photo.meta.title.as_str()), (4, "Changed embedded title")); + assert_eq!(photo.develop, before.develop); + + s.execute("edit.undo", &json!({})).unwrap(); + let photo = s.catalog.photo(id).unwrap(); + assert_eq!((photo.rating, &photo.meta.title), (before.rating, &before.meta.title), "one undo restores both fields"); + assert_eq!(std::fs::read(&file).unwrap(), changed, "reading and undo never rewrite the original"); + assert!(!src.join("photo.xmp").exists() && !src.join("photo.jpg.xmp").exists()); + let _ = std::fs::remove_dir_all(&src); +} + +#[test] +fn embedded_jpeg_xmp_reread_keeps_sidecar_precedence() { + let src = temp_dir("jpeg-xmp-sidecar"); + let file = src.join("photo.jpg"); + let original = write_jpeg_xmp(&file, &jpeg_xmp_packet(4, "Embedded title")); + let mut s = Session::new().with_fs(); + s.execute("library.import", &json!({"paths": [file.to_string_lossy()]})).unwrap(); + let id = only(&s); + let sidecar = src.join("photo.xmp"); + let packet = jpeg_xmp_packet(5, "Sidecar title"); + std::fs::write(&sidecar, &packet).unwrap(); + let read = s.execute("photo.readMetadataFromFile", &json!({"ids": [id.0]})).unwrap(); + assert_eq!(read["read"], json!([{"id": id.0, "from": sidecar.to_string_lossy()}]), "{read}"); + assert_eq!(read["failed"], json!([]), "{read}"); + let photo = s.catalog.photo(id).unwrap(); + assert_eq!((photo.rating, photo.meta.title.as_str()), (5, "Sidecar title")); + assert_eq!(std::fs::read_to_string(&sidecar).unwrap(), packet); + + // A malformed sidecar must report its failure, not fall back to the valid embedded packet. + let malformed = "".repeat(1000); + assert!(lightcraft_meta::parse_xmp(&malformed).is_err()); + std::fs::write(&sidecar, &malformed).unwrap(); + let read = s.execute("photo.readMetadataFromFile", &json!({"ids": [id.0]})).unwrap(); + assert_eq!(read["read"], json!([]), "{read}"); + assert_eq!(read["failed"].as_array().unwrap().len(), 1, "{read}"); + assert_eq!(read["failed"][0]["id"], id.0); + assert!(!read["failed"][0]["error"].as_str().unwrap().is_empty()); + let photo = s.catalog.photo(id).unwrap(); + assert_eq!((photo.rating, photo.meta.title.as_str()), (5, "Sidecar title")); + assert_eq!(std::fs::read_to_string(&sidecar).unwrap(), malformed); + assert_eq!(std::fs::read(&file).unwrap(), original); + let _ = std::fs::remove_dir_all(&src); +} + +#[test] +fn embedded_jpeg_xmp_reread_malformed_input_preserves_state_and_undo() { + let src = temp_dir("jpeg-xmp-malformed"); + let file = src.join("photo.jpg"); + write_jpeg_xmp(&file, &jpeg_xmp_packet(1, "Original title")); + let mut s = Session::new().with_fs(); + s.execute("library.import", &json!({"paths": [file.to_string_lossy()]})).unwrap(); + let id = only(&s); + s.execute("library.select", &json!({"ids": [id.0]})).unwrap(); + s.execute("photo.rate", &json!({"rating": 2})).unwrap(); + let previous_title = s.catalog.photo(id).unwrap().meta.title.clone(); + s.execute("photo.setMeta", &json!({"title": "Catalog title"})).unwrap(); + let before = s.catalog.photo(id).unwrap().clone(); + let malformed = "".repeat(1000); + assert!(lightcraft_meta::parse_xmp(&malformed).is_err()); + let broken_packet = write_jpeg_xmp(&file, &malformed); + let truncated_container = vec![0xff, 0xd8, 0xff, 0xe1, 0, 40, b'x']; + for bytes in [broken_packet, truncated_container] { + std::fs::write(&file, &bytes).unwrap(); + let read = s.execute("photo.readMetadataFromFile", &json!({"ids": [id.0]})).unwrap(); + assert_eq!(read["read"], json!([]), "{read}"); + assert_eq!(read["failed"].as_array().unwrap().len(), 1, "{read}"); + assert_eq!(read["failed"][0]["id"], id.0); + assert!(!read["failed"][0]["error"].as_str().unwrap().is_empty()); + let photo = s.catalog.photo(id).unwrap(); + assert_eq!((photo.rating, &photo.meta.title), (before.rating, &before.meta.title)); + assert_eq!(photo.develop, before.develop); + assert_eq!(std::fs::read(&file).unwrap(), bytes); + assert!(!src.join("photo.xmp").exists() && !src.join("photo.jpg.xmp").exists()); + } + // Failed reads do not add an undo entry: the last successful title edit is still next. + s.execute("edit.undo", &json!({})).unwrap(); + let photo = s.catalog.photo(id).unwrap(); + assert_eq!((photo.rating, &photo.meta.title), (2, &previous_title)); + let _ = std::fs::remove_dir_all(&src); +} + +#[test] +fn embedded_jpeg_xmp_reread_non_utf8_sidecar_preserves_state_and_undo() { + let src = temp_dir("jpeg-xmp-non-utf8-sidecar"); + let file = src.join("photo.jpg"); + let original = write_jpeg_xmp(&file, &jpeg_xmp_packet(4, "Embedded title")); + let mut s = Session::new().with_fs(); + s.execute("library.import", &json!({"paths": [file.to_string_lossy()]})).unwrap(); + let id = only(&s); + assert!(!s.xmp.auto_write); + s.execute("library.select", &json!({"ids": [id.0]})).unwrap(); + s.execute("photo.rate", &json!({"rating": 2})).unwrap(); + let previous_title = s.catalog.photo(id).unwrap().meta.title.clone(); + s.execute("photo.setMeta", &json!({"title": "Catalog title"})).unwrap(); + let before = s.catalog.photo(id).unwrap().clone(); + let sidecar = src.join("photo.xmp"); + let invalid = [0xff, 0xfe, 0x80]; + std::fs::write(&sidecar, invalid).unwrap(); + assert_eq!(std::fs::read_to_string(&sidecar).unwrap_err().kind(), std::io::ErrorKind::InvalidData); + assert_eq!(crate::sidecar::find_sidecar(file.to_str().unwrap(), s.sidecar_naming(id)), Some(sidecar.clone())); + + let read = s.execute("photo.readMetadataFromFile", &json!({"ids": [id.0]})).unwrap(); + assert_eq!(read["read"], json!([]), "an existing non-UTF-8 sidecar must not fall back to embedded JPEG XMP: {read}"); + assert_eq!(read["failed"].as_array().unwrap().len(), 1, "{read}"); + assert_eq!(read["failed"][0]["id"], id.0); + assert!(!read["failed"][0]["error"].as_str().unwrap().is_empty()); + let photo = s.catalog.photo(id).unwrap(); + assert_eq!((photo.rating, &photo.meta.title), (before.rating, &before.meta.title)); + assert_eq!(photo.develop, before.develop); + + s.execute("edit.undo", &json!({})).unwrap(); + let photo = s.catalog.photo(id).unwrap(); + assert_eq!((photo.rating, &photo.meta.title), (2, &previous_title), "failed reads must not add an undo entry"); + assert_eq!(photo.develop, before.develop); + assert_eq!(std::fs::read(&file).unwrap(), original); + assert_eq!(std::fs::read(&sidecar).unwrap(), invalid); + assert!(!src.join("photo.jpg.xmp").exists()); + let _ = std::fs::remove_dir_all(&src); +} + +#[test] +fn embedded_jpeg_xmp_reread_all_metadata_uses_supplied_bytes() { + use std::sync::Arc; + use std::sync::atomic::{AtomicUsize, Ordering}; + + let src = temp_dir("jpeg-xmp-all-metadata"); + let file = src.join("photo.jpg"); + let original = write_jpeg_xmp(&file, &jpeg_xmp_packet(1, "On-disk title")); + let mut s = Session::new().with_fs(); + s.execute("library.import", &json!({"paths": [file.to_string_lossy()]})).unwrap(); + let id = only(&s); + assert!(!s.xmp.auto_write); + let before = s.catalog.photo(id).unwrap().clone(); + let supplied = write_jpeg_xmp(&src.join("supplied.jpg"), &jpeg_xmp_packet(4, "Supplied title")); + let reads = Arc::new(AtomicUsize::new(0)); + let counted = reads.clone(); + let expected_path = file.to_string_lossy().into_owned(); + s.media.file_bytes = Some(Arc::new(move |path| { + assert_eq!(path, expected_path); + counted.fetch_add(1, Ordering::SeqCst); + Ok(supplied.clone()) + })); + + let query = s.execute("photo.allMetadata", &json!({"id": id.0})).unwrap(); + assert_eq!(reads.load(Ordering::SeqCst), 1); + let xmp = query["xmp"].as_array().unwrap(); + assert!( + xmp.iter().any(|x| x["name"] == "dc:title" && x["value"] == "Supplied title"), + "All Metadata must use the bytes already supplied: {query}" + ); + assert!(xmp.iter().any(|x| x["name"] == "xmp:Rating" && x["value"] == "4"), "{query}"); + assert!(!xmp.iter().any(|x| x["value"] == "On-disk title"), "{query}"); + + let sidecar = src.join("photo.xmp"); + let packet = jpeg_xmp_packet(5, "Sidecar title"); + std::fs::write(&sidecar, &packet).unwrap(); + let query = s.execute("photo.allMetadata", &json!({"id": id.0})).unwrap(); + assert_eq!(reads.load(Ordering::SeqCst), 2); + let xmp = query["xmp"].as_array().unwrap(); + assert!(xmp.iter().any(|x| x["name"] == "dc:title" && x["value"] == "Sidecar title"), "{query}"); + assert!(xmp.iter().any(|x| x["name"] == "xmp:Rating" && x["value"] == "5"), "{query}"); + assert!(!xmp.iter().any(|x| x["value"] == "Supplied title"), "{query}"); + let photo = s.catalog.photo(id).unwrap(); + assert_eq!((photo.rating, &photo.meta.title), (before.rating, &before.meta.title)); + assert_eq!(photo.develop, before.develop); + assert_eq!(std::fs::read(&file).unwrap(), original); + assert_eq!(std::fs::read_to_string(&sidecar).unwrap(), packet); + let _ = std::fs::remove_dir_all(&src); +} + #[test] fn demo_photos_have_no_sidecar() { let mut s = Session::with_demo(); diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 95dcd2aef..7610eea56 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -170,8 +170,8 @@ differences and missing bindings are in [ui-parity.md](ui-parity.md#shortcuts). | LR-META-COPYRIGHT-DEFAULT | Default copyright on import | P1 | ✅ | `cmd:library.preferences` (`import.copyright`, `import.creator`), `crates/ui-egui/src/panels/settings.rs` | Settings → Import → Metadata; fills only photos without their own | | LR-META-LOCATION | Location editing | P2 | 🟡 | `cmd:photo.setMeta` (`location`, `city`, `state`, `country`, `gps`), `cmd:metadata.reverseGeocode` | place fields and GPS (decimal or degrees / minutes / seconds) edited in Info, read/written as IPTC Core / Photoshop / EXIF XMP; Show on Map opens OpenStreetMap; Fill Location fills empty City / State / Country from GPS offline (GeoNames, downloaded on request: `cmd:metadata.downloadPlaceNames`); no embedded map | | LR-META-COPYPASTE | Copy / paste metadata | P2 | ✅ | `cmd:photo.copyMetadata`, `cmd:photo.pasteMetadata` | title, caption, alt text, extended description, copyright (notice, status, usage terms, info URL), creator, place fields, keywords; `fields` picks a subset | -| LR-META-XMP | XMP read/write | P0 | ✅ | `cmd:photo.saveMetadataToFile`, `cmd:photo.readMetadataFromFile`, `cmd:library.xmpPreferences`, `crates/engine/src/sidecar.rs`, `docs/xmp-interop.md` | sidecar wins for metadata; a sidecar capture time (`exif:DateTimeOriginal` / `photoshop:DateCreated` / `xmp:CreateDate`) fills in only when the file has none; saving merges into an existing sidecar (`crates/meta/src/xmp_merge.rs`): another app's `crs:`, `xmpMM:History` and unknown namespaces are kept byte for byte, an unreadable sidecar is backed up first; with stem naming, files sharing a stem (raw + JPEG) get separate sidecars (`.xmp` for all but the raw); a `crs:` custom white balance on a relative-WB raw is shifted from `crs:AsShotTemperature` rather than read as Kelvin, or kept As Shot and reported when the packet has no as-shot white (issue #510); heal and clone spots (`crs:RetouchAreas`, legacy `crs:RetouchInfo`) are read into our spots (`crates/engine/src/crs_masks.rs`, see `docs/xmp-interop.md` → Spot removal); radial masks (`Mask/CircularGradient`) keep Lightroom's inside / outside: `crs:Flipped="true"` is the effect-inside flag of current masks and of the legacy Radial Filter, `crs:MaskInverted` the inversion, checked against Lightroom Classic's renders (issue #1815); Read Metadata from File never writes the sidecar back, even with auto-write on, so another app's later `crs:` edits are still read (issue #934) | -| LR-META-EXIF-FULL | Full EXIF/IPTC [Classic] | P1 | ✅ | `cmd:photo.allMetadata`, `cmd:dialog.allMetadata`, `crates/meta/src/tags.rs`, `crates/meta/src/exif.rs`, `crates/meta/src/iptc.rs` | read and written on export; Info ▸ All Metadata…: every TIFF / EXIF / GPS / interop tag (named, common values spelled out) and the XMP fields, searchable; metadata presets | +| LR-META-XMP | XMP read/write | P0 | ✅ | `cmd:photo.saveMetadataToFile`, `cmd:photo.readMetadataFromFile`, `cmd:library.xmpPreferences`, `crates/engine/src/sidecar.rs`, `docs/xmp-interop.md` | manual rereading also reads sidecar-free JPEG embedded XMP (first complete standard APP1 before SOS/EOI, within 16 MiB of headers and 4096 marker/fill steps; no extended assembly or between-scan packets, not strict whole-JPEG validation; reader cases in `crates/engine/src/sidecar.rs`); a discovered unreadable sidecar fails without embedded fallback; procedural refresh/undo, non-UTF-8 sidecar and two explicit malformed fixtures in `crates/engine/src/tests_xmp.rs` (file-write checks use auto-write off); sidecar wins for metadata; a sidecar capture time (`exif:DateTimeOriginal` / `photoshop:DateCreated` / `xmp:CreateDate`) fills in only when the file has none; saving merges into an existing sidecar (`crates/meta/src/xmp_merge.rs`): another app's `crs:`, `xmpMM:History` and unknown namespaces are kept byte for byte, an unreadable sidecar is backed up first; with stem naming, files sharing a stem (raw + JPEG) get separate sidecars (`.xmp` for all but the raw); a `crs:` custom white balance on a relative-WB raw is shifted from `crs:AsShotTemperature` rather than read as Kelvin, or kept As Shot and reported when the packet has no as-shot white (issue #510); heal and clone spots (`crs:RetouchAreas`, legacy `crs:RetouchInfo`) are read into our spots (`crates/engine/src/crs_masks.rs`, see `docs/xmp-interop.md` → Spot removal); radial masks (`Mask/CircularGradient`) keep Lightroom's inside / outside: `crs:Flipped="true"` is the effect-inside flag of current masks and of the legacy Radial Filter, `crs:MaskInverted` the inversion, checked against Lightroom Classic's renders (issue #1815); Read Metadata from File never writes the sidecar back, even with auto-write on, so another app's later `crs:` edits are still read (issue #934) | +| LR-META-EXIF-FULL | Full EXIF/IPTC [Classic] | P1 | ✅ | `cmd:photo.allMetadata`, `cmd:dialog.allMetadata`, `crates/meta/src/tags.rs`, `crates/meta/src/exif.rs`, `crates/meta/src/iptc.rs` | `photo.allMetadata` reuses loaded original bytes for embedded XMP and keeps sidecar precedence (`crates/engine/src/tests_xmp.rs`); read and written on export; Info ▸ All Metadata…: every TIFF / EXIF / GPS / interop tag (named, common values spelled out) and the XMP fields, searchable; metadata presets | ## F. Edit panel — global adjustments (EDIT) @@ -828,6 +828,7 @@ Latin stays upright. Advanced Japanese composition remains open. Regression cove | 2026-10-13 | minor | LR-IMP-DEVICE: Settings → Import → Cameras and Cards can open the import review when a camera or card is connected; status unchanged (✅) | | 2026-10-13 | minor | KEY-SLIDER-SCRUB: drag a slider's value sideways to scrub it, as in Lightroom (new row) | | 2026-10-13 | minor | KEY-SLIDER-RESET: hovering a slider's name or thumb says a double-click resets it; a double-click on the thumb is one undo step, not two; a disabled slider's name doesn't reset it (issue #1436); status unchanged (✅) | +| 2026-10-12 | minor | LR-META-XMP: bounded complete-APP1 manual JPEG reread, fail-closed sidecar precedence and scoped regression coverage; LR-META-EXIF-FULL: reuse loaded bytes; statuses unchanged | | 2026-10-11 | minor | LR-MERGE-HDR: HDR and panorama merges of raws without colour matrices (CR2, NEF, ARW…) write the colour they open with as DNG tags (issue #965); status unchanged | | 2026-10-12 | minor | LR-META-XMP: Read Metadata from File no longer auto-writes the sidecar it has just read, which added `lc:settings` and hid another app's later `crs:` edits (#934); status unchanged (✅) | | 2026-10-12 | patch | LR-IMP-CAMERA-COVERAGE: CR2 white level from the active area, not the saturated masked rows above it (issue #1272); status unchanged (🟡) | diff --git a/docs/xmp-interop.md b/docs/xmp-interop.md index c467a471a..314444318 100644 --- a/docs/xmp-interop.md +++ b/docs/xmp-interop.md @@ -10,7 +10,7 @@ other tools), and let LightCraft pick up edits made elsewhere. |---|---| | Name | `.xmp` by default (`IMG_0001.CR3` → `IMG_0001.xmp`); `.xmp` (`IMG_0001.CR3.xmp`) with `naming: "full"`. Reading accepts either (preferred first) and `.XMP`. With stem naming, files sharing a stem don't share a sidecar: see *Shared names* below. | | Write | `photo.saveMetadataToFile {ids?}` (Photo ▸ Save Metadata to File, ⌘S), or automatically after every change with `library.xmpPreferences {autoWrite: true}` (File ▸ Automatically Write Changes into XMP). Slider drags are written once, when the drag ends; undo/redo rewrite the sidecar. An existing sidecar is **merged into, never replaced** (see *Saving into an existing sidecar*). Written atomically (temp file, fsync, rename). The result lists `written`, `merged` and `backups`. | -| Read | On import (`library.import`, the report counts `sidecars`), and `photo.readMetadataFromFile {ids?}` (one undo step). For raw/DNG files without a sidecar, the XMP embedded in the file is used. `photo.readMetadataFromFile` never writes the sidecar, even with auto-write on: writing back what it has just read would add `lc:settings`, which later reads prefer over another app's `crs:` edits (issue #934). | +| Read | On import (`library.import`, the report counts `sidecars`), and `photo.readMetadataFromFile {ids?}` (one undo step). Manual rereading uses embedded XMP in raw/DNG and JPEG files when no sidecar exists. A discovered sidecar that cannot be read or parsed is reported in `failed`, without applying embedded values. `photo.readMetadataFromFile` never writes the sidecar, even with auto-write on: writing back what it has just read would add `lc:settings`, which later reads prefer over another app's `crs:` edits (issue #934). | | Preferences | `library.xmpPreferences {autoWrite?, naming?: stem\|full}`, stored in the library's `prefs.json`. | What we write (standard namespaces, so other tools can read the metadata): @@ -41,6 +41,29 @@ When foreign `crs:` settings are only partly transferable, `library.import` incl report. Warnings identify unsupported profile names, saved Upright geometry that will be recalculated, and other unmapped adjustments; they do not change the source packet or become catalog edits. +Manual rereading and undo leave the JPEG original unchanged. Manual rereading also leaves sidecar files unchanged, +even with automatic XMP writing on. With automatic XMP writing off (the default), undo/redo leave sidecars unchanged; +with it enabled, subsequent edits and undo/redo can trigger the existing sidecar writer. `photo.allMetadata` uses +its already loaded file bytes for embedded XMP, with an existing sidecar taking precedence. + +Manual JPEG rereading uses the first standard XMP APP1 before the first SOS (scan) or EOI (end) marker. +Scanning is limited to a 16 MiB header span, including SOI, segment headers and skipped payloads, and 4096 +marker/fill steps after SOI. Non-APP1 payloads are skipped without loading them. Each APP1 payload must be +complete and is at most 65,533 bytes. Header extraction succeeds for the first complete +standard XMP packet within both limits, including a packet ending exactly at either limit; otherwise it +fails without a whole-file fallback, including on I/O errors or an incomplete APP1 whose available bytes +form parseable XMP. A valid JPEG with +XMP beyond those limits therefore cannot be manually reread through this path. Extended XMP assembly and +packets between scans are not supported by manual JPEG rereading. + +Packet completeness is stricter than the shared best-effort JPEG extractor; it is not strict whole-JPEG +validation. A complete early XMP packet can be returned without examining the remaining file. Import and +`photo.allMetadata` retain their existing extraction paths. Command regressions in +`crates/engine/src/tests_xmp.rs` cover metadata/undo and sidecar precedence, an over-nested XMP packet and a +truncated APP1 without an XMP signature. Separate reader cases in `sidecar::jpeg_header_tests` use small +counted readers and injected limits for span/step boundaries, complete versus truncated APP1, early stopping, +short reads and I/O errors. + ## Saving into an existing sidecar A sidecar may already hold another application's data — e.g. its `crs:` develop settings and `xmpMM:History` — often