DNG 1.7 deinterleave - fix per review

crates/raw/src/dng.rs

@@ -20,34 +20,57 @@ pub(crate) fn raw_ifd(tiff: &Tiff) -> Option<&Ifd> {
 }
 
 /// Undo the DNG 1.7 row/column interleave, which reorders the stored raster without changing its
-/// size: with `RowInterleaveFactor = n` the rows are stored as `n` consecutive groups, group `g`
+/// size: with `RowInterleaveFactor = n` the rows are stored as `n` consecutive fields, field `g`
 /// holding rows `g, g+n, g+2n, …` of the image (and likewise for columns).
 ///
-/// Left alone, a file like this decodes as `n × m` half-scale copies of the scene, one per
-/// quadrant, with the CFA pattern then read across permuted samples — so it is not merely
-/// misplaced, it is the wrong colour as well (`RowInterleaveFactor = ColumnInterleaveFactor = 2`
-/// is what DNG 1.7 JPEG XL files from Adobe's converter carry).
+/// The tag counts fields, not a divisor of the raster — see [`field_map`] — so a dimension the
+/// factor does not divide is decoded, with the trailing fields short, rather than refused.
+///
+/// Left alone, a file like this decodes as `n × m` scaled-down copies of the scene, one per
+/// field, with the CFA pattern then read across permuted samples — so it is not merely misplaced,
+/// it is the wrong colour as well (`RowInterleaveFactor = ColumnInterleaveFactor = 2` is what DNG
+/// 1.7 JPEG XL files from Adobe's converter carry).
 fn deinterleave(data: &mut RawData, w: usize, h: usize, cpp: usize, rows: usize, cols: usize) -> Result<()> {
     if rows <= 1 && cols <= 1 {
         return Ok(());
     }
+    // The spec sets no bound on the value (it is only "the number of interleaved fields"); this
+    // caps the field table we build and the permutation, and rejects a hostile tag.
     if !(1..=16).contains(&rows) || !(1..=16).contains(&cols) {
         return Err(RawError::Unsupported(format!("interleave factors {rows} × {cols}")));
     }
-    if !h.is_multiple_of(rows) || !w.is_multiple_of(cols) {
-        return Err(RawError::Corrupt(format!("interleave {rows}×{cols} does not divide {w}×{h}")));
-    }
-    // Stored row `r` belongs to group `r / (h / rows)` at position `r % (h / rows)` within it, and
-    // that position holds image row `group + rows * position`. Same for columns.
-    let (gh, gw) = (h / rows, w / cols);
-    let at = |i: usize, n: usize, g: usize| (i % g) * n + i / g;
-    let dest = |r: usize, c: usize| at(r, rows, gh) * w + at(c, cols, gw);
+    let (map_r, map_c) = (field_map(h, rows), field_map(w, cols));
+    let dest = |r: usize, c: usize| map_r[r] * w + map_c[c];
     match data {
         RawData::U16(v) => permute(v, w, h, cpp, dest),
         RawData::F32(v) => permute(v, w, h, cpp, dest),
     }
 }
 
+/// The stored-line → image-line map for one axis: a factor of `n` splits `len` lines into `n`
+/// interleaved fields, field `g` holding lines `g, g+n, g+2n, …`. The fields are stored
+/// consecutively, so field `g` starts at stored line `offs[g]` and its `k`-th line is image line
+/// `g + n*k`. Field sizes follow from the round-robin split — field `g` has `ceil((len - g) / n)`
+/// lines — so when `n` does not divide `len` the trailing fields are short, and when `n` exceeds
+/// `len` the trailing fields are empty.
+fn field_map(len: usize, n: usize) -> Vec<usize> {
+    let mut offs = Vec::with_capacity(n + 1);
+    let mut acc = 0usize;
+    for g in 0..=n {
+        offs.push(acc);
+        if g < n && g < len {
+            acc += (len - 1 - g) / n + 1;
+        }
+    }
+    let mut map = vec![0usize; len];
+    for g in 0..n {
+        for (k, i) in (offs[g]..offs[g + 1]).enumerate() {
+            map[i] = g + n * k;
+        }
+    }
+    map
+}
+
 /// Move every `cpp`-sample pixel of `v` from its stored position to `dest(r, c)`, the image
 /// position it belongs at. The permutation is not its own inverse, so it needs a destination.
 fn permute<T: Copy + Default, F: Fn(usize, usize) -> usize>(v: &mut Vec<T>, w: usize, h: usize, cpp: usize, dest: F) -> Result<()> {
@@ -260,7 +283,8 @@ mod tests {
 
     /// Store `image` the way DNG 1.7 interleaves it: row `r` of the image is written to stored row
     /// `(r % rows) * (h / rows) + r / rows`, and columns likewise. This is the inverse of
-    /// [`deinterleave`]'s destination map, i.e. what a writer produces.
+    /// [`deinterleave`]'s destination map for a raster the factor divides, i.e. what a writer
+    /// produces (the short-trailing-field case is covered by the hand-written fixtures below).
     fn interleave(image: &[u16], w: usize, h: usize, rows: usize, cols: usize) -> Vec<u16> {
         let (gh, gw) = (h / rows, w / cols);
         let mut out = vec![0u16; w * h];
@@ -279,6 +303,19 @@ mod tests {
         }
     }
 
+    /// The image `image` laid out in stored order, given the image line each stored row and column
+    /// holds. Written out by hand from the tag's plain reading, so it does not share a formula with
+    /// the code under test.
+    fn store(image: &[u16], w: usize, rows_img: &[usize], cols_img: &[usize]) -> Vec<u16> {
+        let mut out = vec![0u16; image.len()];
+        for (sr, &ir) in rows_img.iter().enumerate() {
+            for (sc, &ic) in cols_img.iter().enumerate() {
+                out[sr * w + sc] = image[ir * w + ic];
+            }
+        }
+        out
+    }
+
     #[test]
     fn deinterleaves_an_interleaved_raster() {
         // 3×3 rather than 2×2 on purpose: the 2×2 permutation is its own inverse, so a 2×2 fixture
@@ -302,16 +339,43 @@ mod tests {
     }
 
     #[test]
-    fn deinterleave_is_a_no_op_at_one_and_refuses_a_bad_factor() {
+    fn deinterleave_pins_the_field_direction() {
+        // Hand-computed ground truth for the tag's plain reading ("field g holds lines g, g+n,
+        // g+2n, …"). The 3×3 test above round-trips against a helper written from the same reading
+        // of the tag, so it fixes consistency but not the direction; and a 2×2 fixture cannot tell
+        // the two directions apart, being its own inverse. These maps are written out literally so
+        // a reversed permutation fails here. 3 row fields of h = 9 start at stored rows 0, 3, 6 and
+        // hold image rows [0,3,6,1,4,7,2,5,8]; 2 column fields of w = 4 give [0,2,1,3].
+        let (w, h) = (4, 9);
+        let image: Vec<u16> = (0..(w * h) as u16).collect();
+        let mut data = RawData::U16(store(&image, w, &[0, 3, 6, 1, 4, 7, 2, 5, 8], &[0, 2, 1, 3]));
+        assert_ne!(samples(&data), image, "the fixture has to actually be permuted");
+        deinterleave(&mut data, w, h, 1, 3, 2).unwrap();
+        assert_eq!(samples(&data), image);
+    }
+
+    #[test]
+    fn deinterleaves_a_raster_the_factor_does_not_divide() {
+        // The spec fixes the number of fields, not that the size is a multiple of it: h = 7 over 3
+        // row fields is 3 + 2 + 2 (the round-robin split), so field 0 starts at stored row 0 holding
+        // image rows 0, 3, 6; field 1 at stored row 3 holding 1, 4; field 2 at stored row 5 holding
+        // 2, 5. Columns: w = 5 over 2 fields is 3 + 2 → image columns [0,2,4,1,3].
+        let (w, h) = (5, 7);
+        let image: Vec<u16> = (0..(w * h) as u16).collect();
+        let mut data = RawData::U16(store(&image, w, &[0, 3, 6, 1, 4, 2, 5], &[0, 2, 4, 1, 3]));
+        assert_ne!(samples(&data), image, "the fixture has to actually be permuted");
+        deinterleave(&mut data, w, h, 1, 3, 2).unwrap();
+        assert_eq!(samples(&data), image);
+    }
+
+    #[test]
+    fn deinterleave_is_a_no_op_at_one_and_refuses_a_hostile_factor() {
         let image: Vec<u16> = (0..48).collect();
         let mut plain = RawData::U16(image.clone());
         deinterleave(&mut plain, 8, 6, 1, 1, 1).unwrap();
         assert_eq!(samples(&plain), image);
 
-        // A factor that does not divide the raster is an error, not a scrambled image.
-        let mut bad = RawData::U16(image.clone());
-        assert!(deinterleave(&mut bad, 8, 6, 1, 4, 1).is_err());
-        // so is a hostile factor
+        // A hostile factor is an error, not a scrambled image.
         let mut hostile = RawData::U16(image);
         assert!(deinterleave(&mut hostile, 8, 6, 1, 1 << 20, 1).is_err());
     }

docs/parity.md

@@ -142,7 +142,7 @@ under the UI command that wraps it (a no-op error); the UI command now wins.
 | LR-IMP-PROFILES | Import profiles & presets | P1 | ✅ | `cmd:file.importPresets`, `cmd:preset.import`, `cmd:profile.import`, `crates/engine/src/preset_import.rs`, `crates/engine/src/preset_luminar.rs` | presets: .lcpreset, XMP, classic .lrtemplate, photos carrying edits ("DNG presets"), Luminar looks (.lmp, .mplumpack collections; sliders with a counterpart), .zip bundles, folders (folder → group), drag & drop; masks carried over; unmapped settings reported (fields that only describe the preset or photo, e.g. `Cluster`, `SortName`, `SupportsAmount2`, the as-shot white, empty Point Color slots, are not); profiles: .cube 3D LUTs; Adobe profile formats deliberately unsupported |
 | LR-IMP-LOCAL | Work on files in place | P0 | ✅ | `cmd:library.browse`, `cmd:photo.addToLibrary`, `cmd:library.import` (mode add), `cmd:local.addRoot`, `cmd:local.hide`, `cmd:local.restoreHidden`, `crates/engine/src/cmd/browse.rs`, `crates/ui-egui/src/panels/left.rs` (`local_section`), `crates/ui-egui/src/panels/grid.rs` (`folder_header`) | Local: browse Pictures / Desktop / Downloads / Home or any folder without adding it (breadcrumb, Include subfolders, Add N to My Photos); browsed photos stay out of All Photos, albums and counts; folders picked with Browse Folder… (or Keep in Local / Add to Local on a folder row) stay listed across restarts, and browsing a folder inside a listed one highlights it inside that tree (opened down to it, siblings reachable) instead of adding a root; edits go to XMP sidecars; importing promotes them; right-click a top-level location → Remove from Local hides the shortcut only (nothing on disk or in the catalog changes), saved with the UI state, and “Show N hidden locations” (or Browse Folder… on it) brings it back; a folder is one location however its path is spelled (`/` or `\`, trailing separator, `.`/`..`, drive-letter case; `folder_key` in `crates/catalog/src/query.rs`); browsed photos nobody changed are forgotten once their folder has not been browsed for 30 days (Settings → Performance → Local folders, 0 = never; checked when the library opens, or `cmd:library.forgetLocal` with `dryRun`; files and sidecars stay, browsing again brings them back; see `crates/catalog/src/local.rs` for what counts as a change) |
 | LR-IMP-SIDECAR-SPLIT | Separate XMP sidecar variants | P2 | ⬜ | `cmd:library.xmpPreferences` | sidecar naming option exists (stem/full), no split sidecars |
-| LR-IMP-FORMATS | Supported formats | P0 | 🟡 | `crates/codecs/src/lib.rs`, `crates/raw/src/lib.rs`, `crates/raw/src/jxl.rs` | JPEG, PNG, TIFF (1- to 64-bit integer, 16- to 64-bit float, incl. bilevel and CCITT fax), WebP, JXL, PSD, GIF, BMP; DNG (incl. lossy / Smart Preview DNG and DNG 1.7 JPEG XL tiles/strips: lossless ones decode bit-exactly on synthetic CFA and LinearRaw files, lossy ones decode but no real *lossy* JPEG XL DNG has verified them yet, and DNG 1.7 row/column-interleaved rasters are undone before the mosaic is read — `RowInterleaveFactor` / `ColumnInterleaveFactor` reorder the stored raster without changing its size, so without that step a file from Adobe's converter decodes as `n × m` half-scale copies of the scene, one per quadrant, in the wrong colours; verified on 4 real ILCE-6400 JPEG XL DNGs rendering bit-identical to their non-interleaved twins, with the permutation unit-tested at a factor whose inverse differs from itself; a single-chunk JPEG XL preview IFD is offered as the embedded preview), CR2 (colour-filter layout from the file's `CR2CFAPattern` tag, issue #85), ARW (pre-2017 bodies: white balance and black level from enciphered maker-note / `SR2SubIFD` data, issue #148; downsized Sony lossless YCbCr 4:2:0 / 4:2:2 tiles decoded to linear RGB, verified on private ILCE-7M4 3:2 & 4:3 examples including 16-bit full-resolution export), NEF (uncompressed, lossless and lossy compressed 12/14-bit, `crates/raw/src/vendor/nefc.rs`), RAF (uncompressed, lossless/lossy compressed Bayer and X-Trans; `crates/raw/src/vendor/rafc.rs`, full sensor verification on 13 bodies, `docs/raf-compression.md`), RW2 / Leica RWL / Panasonic RAW in every raw format (compressed 4 and 6, the prefix-coded strips of format 8 from the GH6 on, packed 2/5/7, the 16-bit words of the 2005–2007 bodies; black levels, in-camera aspect crops and mapped-out defect pixels; 178 CC0 files from 118 bodies checked, `crates/raw/src/vendor/rw2.rs`), PEF, ORF (uncompressed; colour-filter layout from the file's Exif `CFAPattern`). 12-bit NEF black level: maker note `0x003d` is in 14-bit units (D750, D780, D850, D7500, Z 50 checked). Lossy type 2 NEF curves of version `0x44 0x40` (D780, D6, Z 5, Z 6, Z 7, Z 7_2) span a quarter of the sample range, read from the table's own point spacing. CR3 lossless CRX Bayer and version 0x100/0x200 C-RAW decodes to sensor pixels with timed white balance, black/white levels and file crop (the shot's aspect ratio from the maker note's `AspectInfo`); full sensor samples match the reference on M50/R100/R8 (`crates/raw/tests/cr3_corpus.rs`, `docs/cr3.md`). Unsupported coding variants retain metadata and use the embedded JPEG when available; HEVC preview decoding remains unsupported. Missing: CR3 unverified CRX variants, compressed ORF, NEFs labelled compressed (34713) but stored uncompressed without a `0x0096` table (Z 6 packed 14-bit, D850 12-bit uncompressed), NEF "lossy after split" (preview only: imported with `preview_only` = the decoder's reason, rendered as a rendered JPEG, flagged by a grid badge and loupe/Edit/Info notices, `previewOnly` in `catalog.query`; Reload clears it once the file decodes), TIFF-based raws whose raw data sits in a private block (Phase One / Leaf IIQ, Canon EOS-1D / 1Ds and Kodak DCS TIFFs: IFD0 is only a reduced RGB image next to a much larger declared picture) are recognised from that structure and import as `preview_only` with the reason, instead of opening as a thumbnail-sized ordinary image (`thumbnail_shell` in `crates/raw/src/lib.rs`), Recognised raws LightCraft cannot decode open as `preview_only` with any usable embedded JPEG, also Canon CRW, Minolta MRW and Sigma X3F (containers recognised by magic, preview found by a structural JPEG scan), Kodak KDC (IFD0 without `ImageWidth`), Leaf MOS and Epson ERF (scan), and any decode failure of a recognised raw, not only an unsupported variant; ones with no JPEG still fail with "without an embedded preview", AVIF decode (HEIC decodes only behind the non-default `heif` feature: `crates/heif`, heic-rs, same policy as PhotoCraft; 8/10-bit, alpha, irot/imir/clap, ICC/EXIF/XMP), palette (indexed-colour) TIFF (refused by the `tiff` crate) |
+| LR-IMP-FORMATS | Supported formats | P0 | 🟡 | `crates/codecs/src/lib.rs`, `crates/raw/src/lib.rs`, `crates/raw/src/jxl.rs` | JPEG, PNG, TIFF (1- to 64-bit integer, 16- to 64-bit float, incl. bilevel and CCITT fax), WebP, JXL, PSD, GIF, BMP; DNG (incl. lossy / Smart Preview DNG and DNG 1.7 JPEG XL tiles/strips: lossless ones decode bit-exactly on synthetic CFA and LinearRaw files, lossy ones decode but no real *lossy* JPEG XL DNG has verified them yet, and DNG 1.7 row/column-interleaved rasters are undone before the mosaic is read — `RowInterleaveFactor` / `ColumnInterleaveFactor` reorder the stored raster without changing its size, so without that step a file from Adobe's converter (whose 2×2 interleave makes four half-scale quadrants) decodes as `n × m` scaled-down copies of the scene in the wrong colours; the fields follow the tag's count, so a dimension the factor does not divide decodes with short trailing fields rather than being refused; verified on 4 real ILCE-6400 JPEG XL DNGs rendering bit-identical to their non-interleaved twins, with the permutation unit-tested against a hand-written stored→image map at a factor whose inverse differs from itself (factors above 2 rest on the plain reading of the tag — no real file with one has turned up); a single-chunk JPEG XL preview IFD is offered as the embedded preview), CR2 (colour-filter layout from the file's `CR2CFAPattern` tag, issue #85), ARW (pre-2017 bodies: white balance and black level from enciphered maker-note / `SR2SubIFD` data, issue #148; downsized Sony lossless YCbCr 4:2:0 / 4:2:2 tiles decoded to linear RGB, verified on private ILCE-7M4 3:2 & 4:3 examples including 16-bit full-resolution export), NEF (uncompressed, lossless and lossy compressed 12/14-bit, `crates/raw/src/vendor/nefc.rs`), RAF (uncompressed, lossless/lossy compressed Bayer and X-Trans; `crates/raw/src/vendor/rafc.rs`, full sensor verification on 13 bodies, `docs/raf-compression.md`), RW2 / Leica RWL / Panasonic RAW in every raw format (compressed 4 and 6, the prefix-coded strips of format 8 from the GH6 on, packed 2/5/7, the 16-bit words of the 2005–2007 bodies; black levels, in-camera aspect crops and mapped-out defect pixels; 178 CC0 files from 118 bodies checked, `crates/raw/src/vendor/rw2.rs`), PEF, ORF (uncompressed; colour-filter layout from the file's Exif `CFAPattern`). 12-bit NEF black level: maker note `0x003d` is in 14-bit units (D750, D780, D850, D7500, Z 50 checked). Lossy type 2 NEF curves of version `0x44 0x40` (D780, D6, Z 5, Z 6, Z 7, Z 7_2) span a quarter of the sample range, read from the table's own point spacing. CR3 lossless CRX Bayer and version 0x100/0x200 C-RAW decodes to sensor pixels with timed white balance, black/white levels and file crop (the shot's aspect ratio from the maker note's `AspectInfo`); full sensor samples match the reference on M50/R100/R8 (`crates/raw/tests/cr3_corpus.rs`, `docs/cr3.md`). Unsupported coding variants retain metadata and use the embedded JPEG when available; HEVC preview decoding remains unsupported. Missing: CR3 unverified CRX variants, compressed ORF, NEFs labelled compressed (34713) but stored uncompressed without a `0x0096` table (Z 6 packed 14-bit, D850 12-bit uncompressed), NEF "lossy after split" (preview only: imported with `preview_only` = the decoder's reason, rendered as a rendered JPEG, flagged by a grid badge and loupe/Edit/Info notices, `previewOnly` in `catalog.query`; Reload clears it once the file decodes), TIFF-based raws whose raw data sits in a private block (Phase One / Leaf IIQ, Canon EOS-1D / 1Ds and Kodak DCS TIFFs: IFD0 is only a reduced RGB image next to a much larger declared picture) are recognised from that structure and import as `preview_only` with the reason, instead of opening as a thumbnail-sized ordinary image (`thumbnail_shell` in `crates/raw/src/lib.rs`), Recognised raws LightCraft cannot decode open as `preview_only` with any usable embedded JPEG, also Canon CRW, Minolta MRW and Sigma X3F (containers recognised by magic, preview found by a structural JPEG scan), Kodak KDC (IFD0 without `ImageWidth`), Leaf MOS and Epson ERF (scan), and any decode failure of a recognised raw, not only an unsupported variant; ones with no JPEG still fail with "without an embedded preview", AVIF decode (HEIC decodes only behind the non-default `heif` feature: `crates/heif`, heic-rs, same policy as PhotoCraft; 8/10-bit, alpha, irot/imir/clap, ICC/EXIF/XMP), palette (indexed-colour) TIFF (refused by the `tiff` crate) |
 | LR-IMP-CAMERA-COVERAGE | Camera coverage, verified per model | P0 | 🟡 | `crates/raw/tests/corpus.rs`, `xtask/src/main.rs` | a supported container is not the same as every camera that writes it decoding correctly: ~55 CC0 corpus files plus 6 exact-sensor CR3 cases (plus ~60 NEFs checked by hand and 178 Panasonic / Leica RW2, RWL and RAW files from 118 bodies, checked against each file's own JPEG) against the >1,000 models Lightroom lists; per-model differences still surface (e.g. the CR2 colour-filter layout differed by model until it was read from the file's own tag, issue #85, checked on 35 CR2 bodies; pre-2017 Sony ARWs opened green until their scrambled white balance / black level were read, issue #148, checked on 16 ARWs from 15 Sony bodies; Canon PowerShot and EOS M CR2s whose `ColorData` is stored as bytes opened green until the bytes were read as 16-bit words, 25 files from 25 bodies checked). Compressed RAF sensor arrays verified on 13 Fujifilm bodies (`docs/raf-compression.md`). Grow the CC0 corpus per model and test each sample decodes with plausible colour. Nikon CropArea and untagged-preview colour-space regression tests; private Z 8 crop verified. |
 | LR-IMP-CULL-AT-IMPORT | Culling analysis at import | P2 | 🟡 | `cmd:photo.analyze` | run Assisted Culling on the imported photos (they're selected after an import); not automatic |
 | LR-IMP-MOVE | Move on import [Classic] | P1 | ✅ | `cmd:library.import` (`mode` move), `crates/engine/src/import_move.rs`, `crates/engine/src/tests_import_move.rs`, `crates/ui-egui/src/import.rs` | Transfer ▸ Move with Copy's destination, folders (day / month / one folder / custom template), rename template and example destination; XMP sidecars (both namings) move along; each source is removed only after its destination is written (hard link on the same volume, else copied, synced and compared byte for byte) and its catalog record is saved; failed, duplicate, unchecked files keep their sources, taken names get -1, -2…, never overwritten; files already in the destination/library are added in place; sources that can't be removed (read-only card) are kept and reported (`kept`). Undo removes the photos from the library but leaves the moved files at the destination. No Copy as DNG while moving |