perf(raster): mode and depth conversion keep shared tiles shared (#4531) (#4725)

Surface::convert / try_convert and map_color converted tile by
coordinate, and the ICC path (color_cmds::convert_surface) staged and
converted every coordinate's tile. A tile shared across the grid, such
as a filled background (one Arc for all of it), therefore came out as
one fresh tile per coordinate: a white 2048×1024 background went from
1 to 32 tile allocations, a 24 MP one to 384.

Each distinct tile is now converted once and the result shared by
every coordinate that shared the original. Pixels are unchanged; tests
compare against a per-pixel conversion and check the sharing for
convert, try_convert, map_color and Image › Mode › CMYK.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Brandon Thomas <echelon@gmail.com>

crates/engine/src/color_cmds.rs

@@ -571,20 +571,41 @@ pub fn convert_surface(s: &Surface, from: ColorMode, to: PixelFormat, t: &Transf
     let mut dpo = vec![0.0f32; ds];
     t.convert_f32(&dp, ss, &mut dpo, ds, true);
     let mut out = Surface::with_default(to, &dpo);
-    // Convert all tiles in one parallel pass into a staging buffer, then store them.
-    let tiles: Vec<_> = s.tiles().map(|(c, t)| (*c, t.clone())).collect();
+    // Convert every distinct tile once, in one parallel pass into a staging buffer, then store
+    // them. Coordinates that share a tile (a filled background) share the converted one (#4531).
+    let mut distinct: Vec<std::sync::Arc<photocraft_raster::Tile>> = Vec::new();
+    let mut slot: std::collections::HashMap<*const photocraft_raster::Tile, usize> = std::collections::HashMap::new();
+    let tiles: Vec<(photocraft_geom::TileCoord, usize)> = s
+        .tiles()
+        .map(|(c, tile)| {
+            let i = *slot.entry(std::sync::Arc::as_ptr(tile)).or_insert_with(|| {
+                distinct.push(tile.clone());
+                distinct.len() - 1
+            });
+            (*c, i)
+        })
+        .collect();
     let tile_px = (photocraft_geom::TILE_SIZE * photocraft_geom::TILE_SIZE) as usize;
     let dst_len = tile_px * to.bytes_per_pixel();
-    let mut staging = vec![0u8; dst_len * tiles.len()];
-    let jobs: Vec<(&[u8], &mut [u8])> = tiles.iter().map(|(_, t)| t.bytes()).zip(staging.chunks_mut(dst_len.max(1))).collect();
+    let mut staging = vec![0u8; dst_len * distinct.len()];
+    let jobs: Vec<(&[u8], &mut [u8])> = distinct.iter().map(|t| t.bytes()).zip(staging.chunks_mut(dst_len.max(1))).collect();
     let kind = match sf.sample {
         SampleType::U8 => SampleKind::U8,
         SampleType::U16 => SampleKind::U16,
         SampleType::F32 => SampleKind::F32,
     };
     t.convert_bytes_many(kind, jobs, ss, ds, true);
-    for ((c, _), bytes) in tiles.iter().zip(staging.chunks(dst_len.max(1))) {
-        out.tile_mut(*c).bytes_mut().copy_from_slice(bytes);
+    let mut converted: Vec<Option<std::sync::Arc<photocraft_raster::Tile>>> = vec![None; distinct.len()];
+    for (c, i) in tiles {
+        if let Some(Some(tile)) = converted.get(i) {
+            out.put_tiles([(c, tile.clone())]);
+            continue;
+        }
+        let Some(bytes) = staging.chunks(dst_len.max(1)).nth(i) else { continue };
+        out.tile_mut(c).bytes_mut().copy_from_slice(bytes);
+        if let Some(slot) = converted.get_mut(i) {
+            *slot = out.tile(c).cloned();
+        }
     }
     out
 }
@@ -1125,6 +1146,29 @@ mod tests {
     use super::*;
     use photocraft_geom::Rect;
 
+    /// Image › Mode to CMYK keeps a filled background's shared tile shared (#4531): one
+    /// converted tile for the whole grid, the same colour everywhere.
+    #[test]
+    fn mode_conversion_keeps_shared_tiles_shared() {
+        let mut s = Session::new();
+        s.execute("file.new", json!({"width": 2048, "height": 1024, "background": "white"})).unwrap();
+        let distinct = |s: &Session| {
+            let doc = &s.active().unwrap().doc;
+            let surf = doc.layers[0].surface().unwrap();
+            let mut seen: Vec<_> = surf.tiles().map(|(_, t)| std::sync::Arc::as_ptr(t)).collect();
+            seen.sort();
+            seen.dedup();
+            (seen.len(), surf.tile_count())
+        };
+        let (before, count) = distinct(&s);
+        s.execute("image.mode.cmyk", json!({})).unwrap();
+        let (after, count_after) = distinct(&s);
+        assert_eq!((after, count_after), (before, count), "{before} distinct of {count} before");
+        let doc = &s.active().unwrap().doc;
+        let surf = doc.layers[0].surface().unwrap();
+        assert_eq!(surf.pixel(5, 5), surf.pixel(2000, 1000), "the same converted white everywhere");
+    }
+
     fn session(mode: &str, depth: u32) -> Session {
         let mut s = Session::new();
         s.execute("file.new", json!({"width": 64, "height": 32, "mode": mode, "depth": depth})).unwrap();

crates/raster/src/lib.rs

@@ -813,7 +813,16 @@ impl Surface {
         encode_pixel(&format, &dp, &mut self.default_pixel);
         let coords: Vec<TileCoord> = self.tiles.keys().copied().collect();
         let px_count = (TILE_SIZE * TILE_SIZE) as usize;
+        // A tile shared by several coordinates (a filled background) is mapped once and stays
+        // shared (#4531). Keys are the original tiles, all alive until their coordinates are
+        // reached, so a freed address can't be mistaken for another original.
+        let mut done: std::collections::HashMap<*const Tile, Arc<Tile>> = std::collections::HashMap::new();
         for c in coords {
+            let Some(key) = self.tiles.get(&c).map(Arc::as_ptr) else { continue };
+            if let Some(mapped) = done.get(&key) {
+                self.tiles.insert(c, mapped.clone());
+                continue;
+            }
             let tile = self.tile_mut(c);
             for i in 0..px_count {
                 for k in 0..color {
@@ -822,6 +831,9 @@ impl Surface {
                     write_sample(&mut tile.data, format.sample, at, f(v));
                 }
             }
+            if let Some(mapped) = self.tiles.get(&c) {
+                done.insert(key, mapped.clone());
+            }
         }
     }
 
@@ -830,7 +842,13 @@ impl Surface {
         let mut out = Surface::with_default(to, &convert_pixel(&self.format, &to, &self.default_pixel()));
         let from_n = self.channels();
         let mut src = vec![0.0f32; from_n];
+        // Each distinct tile converts once; coordinates sharing it share the result (#4531).
+        let mut done: std::collections::HashMap<*const Tile, Arc<Tile>> = std::collections::HashMap::new();
         for (c, t) in &self.tiles {
+            if let Some(converted) = done.get(&Arc::as_ptr(t)) {
+                out.tiles.insert(*c, converted.clone());
+                continue;
+            }
             let dst = out.tile_mut(*c);
             let px_count = (TILE_SIZE * TILE_SIZE) as usize;
             for i in 0..px_count {
@@ -842,6 +860,9 @@ impl Surface {
                     write_sample(&mut dst.data, to.sample, i * to.channels() + k, *v);
                 }
             }
+            if let Some(converted) = out.tiles.get(c) {
+                done.insert(Arc::as_ptr(t), converted.clone());
+            }
         }
         out
     }
@@ -853,7 +874,12 @@ impl Surface {
         let mut src = Vec::new();
         src.try_reserve_exact(from_n).map_err(|_| AllocationError::NotEnoughMemory { bytes: from_n * std::mem::size_of::<f32>() })?;
         src.resize(from_n, 0.0f32);
+        let mut done: std::collections::HashMap<*const Tile, Arc<Tile>> = std::collections::HashMap::new();
         for (c, t) in &self.tiles {
+            if let Some(converted) = done.get(&Arc::as_ptr(t)) {
+                out.tiles.insert(*c, converted.clone());
+                continue;
+            }
             let dst = out.try_tile_mut(*c)?;
             let px_count = (TILE_SIZE * TILE_SIZE) as usize;
             for i in 0..px_count {
@@ -865,6 +891,9 @@ impl Surface {
                     write_sample(&mut dst.data, to.sample, i * to.channels() + k, *v);
                 }
             }
+            if let Some(converted) = out.tiles.get(c) {
+                done.insert(Arc::as_ptr(t), converted.clone());
+            }
         }
         Ok(out)
     }
@@ -984,6 +1013,43 @@ pub fn is_hdr(sample: SampleType) -> bool {
 
 #[cfg(test)]
 mod tests {
+    /// Distinct tile allocations in `s`.
+    fn distinct_tiles(s: &Surface) -> usize {
+        let mut seen: Vec<*const Tile> = s.tiles().map(|(_, t)| Arc::as_ptr(t)).collect();
+        seen.sort();
+        seen.dedup();
+        seen.len()
+    }
+
+    /// Converting or tone-mapping a surface keeps a shared tile shared (#4531) and gives the
+    /// same pixels as converting each pixel: a filled background is one tile across the grid,
+    /// plus some distinct tiles.
+    #[test]
+    fn conversion_keeps_shared_tiles_shared() {
+        let mut s = Surface::new(PixelFormat::RGBA8);
+        s.fill_rect(Rect::new(0, 0, 2048, 1024), &[0.9, 0.5, 0.2, 1.0]);
+        s.fill_rect(Rect::new(10, 10, 20, 20), &[0.1, 0.2, 0.3, 0.5]);
+        let shared_before = distinct_tiles(&s);
+        assert!(shared_before <= 2 && s.tile_count() == 32, "{shared_before} distinct of {}", s.tile_count());
+        let r = Rect::new(0, 0, 2048, 1024);
+        for to in [PixelFormat::RGBA16, PixelFormat::RGBA32F] {
+            for out in [s.convert(to), s.try_convert(to).expect("memory")] {
+                assert_eq!(distinct_tiles(&out), shared_before, "{to:?}: still shared");
+                assert_eq!(out.tile_count(), 32);
+                let (a, b) = (out.read_region(r), s.read_region(r));
+                for (pa, pb) in a.as_chunks::<4>().0.iter().zip(b.as_chunks::<4>().0) {
+                    let want = convert_pixel(&s.format(), &to, pb);
+                    assert!(pa.iter().zip(&want).all(|(x, y)| (x - y).abs() < 1e-6), "{to:?}: {pa:?} vs {want:?}");
+                }
+            }
+        }
+        let mut m = s.clone();
+        m.map_color(|v| v * 0.5);
+        assert_eq!(distinct_tiles(&m), shared_before, "map_color keeps it shared");
+        assert!((m.pixel(1000, 500)[0] - s.pixel(1000, 500)[0] * 0.5).abs() < 1.0 / 255.0);
+        assert_eq!(s.pixel(1000, 500), vec![s.pixel(1000, 500)[0], s.pixel(1000, 500)[1], s.pixel(1000, 500)[2], 1.0], "the source is untouched");
+    }
+
     use super::*;
     use photocraft_color::PixelFormat;
     use proptest::prelude::*;