address review: exempt guide-free geometries from the space cap

crates/metafile/src/canvas.rs

@@ -369,11 +369,6 @@ impl Canvas {
     }
 
     /// True once the item cap is hit; the readers stop there and keep what they have.
-    /// Bytes the canvas has retained so far.
-    pub fn used(&self) -> usize {
-        self.used
-    }
-
     pub fn full(&self) -> bool {
         self.full
     }
@@ -556,10 +551,6 @@ impl Canvas {
     /// of at most [`SHADE_RES`] cells on the longer side, laid out so that its one repeat covers
     /// the shape.
     pub fn shade_shape(&mut self, ops: &[Op], shade: &dyn Fn(P) -> [u8; 4]) {
-        // A full canvas keeps nothing more, so do not sample a tile for it.
-        if self.full {
-            return;
-        }
         let dev: Vec<Op> = ops.iter().map(|o| self.map_op(*o)).collect();
         let Some([l, t, r, b]) = crate::cliprect::ops_bbox(&dev) else { return };
         let side = (r - l).max(b - t);

crates/metafile/src/emfplus.rs

@@ -289,7 +289,7 @@ impl Plus {
             return None;
         }
         match typ {
-            0x4008 => self.object(flags, r, cv.used()),
+            0x4008 => self.object(flags, r),
             0x4009 | 0x4013 | 0x401A..=0x401C | 0x4036 => self.record_image_text_region(cv, typ, flags, r),
             0x4025..=0x4035 => self.record_state(cv, typ, flags, r),
             0x4010..=0x4012 | 0x4016..=0x4019 => self.record_curve(cv, typ, flags, r),
@@ -559,7 +559,7 @@ impl Plus {
         Some(())
     }
 
-    fn object(&mut self, flags: u16, r: Bytes, canvas_used: usize) -> Option<()> {
+    fn object(&mut self, flags: u16, r: Bytes) -> Option<()> {
         let id = usize::from(flags & 0xFF);
         let kind = (flags >> 8) & 0x7F;
         if flags & 0x8000 != 0 {
@@ -575,7 +575,7 @@ impl Plus {
             p.data.extend_from_slice(&chunk[..chunk.len().min(room)]);
             if p.data.len() >= p.total {
                 let obj = parse_object(p.kind, Bytes(&p.data));
-                self.store(p.id, obj, canvas_used);
+                self.store(p.id, obj);
             } else {
                 self.partial = Some(p);
             }
@@ -589,23 +589,22 @@ impl Plus {
             let room = p.total.saturating_sub(p.data.len());
             p.data.extend_from_slice(&r.0[..r.0.len().min(room)]);
             let obj = parse_object(kind, Bytes(&p.data));
-            self.store(id, obj, canvas_used);
+            self.store(id, obj);
             return Some(());
         }
         let obj = parse_object(kind, r);
-        self.store(id, obj, canvas_used);
+        self.store(id, obj);
         Some(())
     }
 
-    /// `canvas_used` is what the canvas already retains: objects and items share one budget.
-    fn store(&mut self, id: usize, obj: Option<Obj>, canvas_used: usize) {
+    fn store(&mut self, id: usize, obj: Option<Obj>) {
         let Some(slot) = self.objects.get_mut(id) else { return };
         if let Some(old) = slot.take() {
             self.object_bytes = self.object_bytes.saturating_sub(Self::obj_cost(&old));
         }
         if let Some(new_obj) = obj {
             let cost = Self::obj_cost(&new_obj);
-            if self.object_bytes.saturating_add(cost).saturating_add(canvas_used) <= crate::MAX_BYTES {
+            if self.object_bytes.saturating_add(cost) <= crate::MAX_BYTES {
                 self.object_bytes += cost;
                 *slot = Some(new_obj);
             }
@@ -616,7 +615,7 @@ impl Plus {
         match obj {
             Obj::Image(img) => match img.as_ref() {
                 ImageData::Bitmap(rgba) => rgba.px.len(),
-                ImageData::Meta(pic) => picture_cost(&pic.items),
+                ImageData::Meta(pic) => pic.items.len().saturating_mul(128),
             },
             Obj::Brush(Fill::Texture { img, .. }) => img.px.len(),
             Obj::Path(ops) => ops.len().saturating_mul(std::mem::size_of::<Op>()),
@@ -1277,33 +1276,6 @@ fn metafile(r: Bytes) -> Option<Picture> {
     pic.filter(|p| p.width > 0.0 && p.height > 0.0)
 }
 
-/// Bytes the items of a nested picture own: path segments, clip rectangles, pixels and text, with
-/// each shared pattern tile counted once.
-fn picture_cost(items: &[crate::Item]) -> usize {
-    use crate::Item;
-    let region = |r: &Option<Vec<[f32; 4]>>| r.as_ref().map_or(0, |v| v.len().saturating_mul(16));
-    let mut tiles = std::collections::HashSet::new();
-    let mut total = 0usize;
-    for item in items {
-        let own = match item {
-            Item::Path { segs, region: r, .. } => segs.len().saturating_mul(std::mem::size_of::<crate::Seg>()).saturating_add(region(r)),
-            Item::Bitmap { rgba, region: r, .. } => rgba.len().saturating_add(region(r)),
-            Item::Pattern { segs, tile, region: r, .. } => {
-                let tile_bytes = if tiles.insert(Arc::as_ptr(tile)) { tile.rgba.len() } else { 0 };
-                segs.len().saturating_mul(std::mem::size_of::<crate::Seg>()).saturating_add(tile_bytes).saturating_add(region(r))
-            }
-            Item::Text(t) => t
-                .text
-                .len()
-                .saturating_add(t.dx.len().saturating_mul(std::mem::size_of::<f32>()))
-                .saturating_add(t.font.family.len())
-                .saturating_add(region(&t.region)),
-        };
-        total = total.saturating_add(128).saturating_add(own);
-    }
-    total
-}
-
 fn bitmap(r: Bytes) -> Option<Rgba> {
     let (w, h, stride) = (r.u32(8)? as i32, r.u32(12)? as i32, r.u32(16)? as i32);
     let format = r.u32(20)?;
@@ -1581,17 +1553,3 @@ fn wrap_line(line: &[u16], width: f64, em: f64) -> Vec<Vec<u16>> {
     out.push(cur);
     out
 }
-
-#[cfg(test)]
-mod cost_tests {
-    use super::*;
-    use crate::{Item, Seg, Tile};
-
-    #[test]
-    fn a_shared_pattern_tile_is_charged_once() {
-        let tile = Arc::new(Tile { width: 512, height: 512, rgba: vec![0; 1 << 20], cell: [1.0, 1.0], origin: [0.0, 0.0] });
-        let item = || Item::Pattern { segs: vec![Seg::Move(0.0, 0.0), Seg::Line(1.0, 1.0)], even_odd: false, tile: tile.clone(), region: None };
-        let items: Vec<Item> = (0..257).map(|_| item()).collect();
-        assert!(picture_cost(&items) < 2 << 20, "{}", picture_cost(&items));
-    }
-}

crates/metafile/src/place.rs

@@ -58,7 +58,6 @@ impl Picture {
         let k = (c[0].abs() + c[3].abs()) / 2.0;
         let mut out = Vec::with_capacity(self.items.len());
         let mut total_elements = 0usize;
-        let region_elements = std::cell::Cell::new(0usize);
         let mut cached_region: Option<(*const [f32; 4], usize, Option<Option<Arc<BezPath>>>)> = None;
         let mut get_region = |reg: &Option<Vec<[f32; 4]>>| -> Option<Option<Arc<BezPath>>> {
             let (ptr, len) = match reg {
@@ -72,15 +71,12 @@ impl Picture {
                 return res.clone();
             }
             let res = placed_region(&tf, reg);
-            // Each distinct clip region is expanded once; its elements count against the placement budget.
-            let n = res.as_ref().and_then(Option::as_ref).map_or(0, |p| p.elements().len());
-            region_elements.set(region_elements.get().saturating_add(n));
             cached_region = Some((ptr, len, res.clone()));
             res
         };
 
         for (index, item) in self.items.iter().enumerate() {
-            if total_elements.saturating_add(region_elements.get()) >= MAX_PLACED_ELEMENTS {
+            if total_elements >= MAX_PLACED_ELEMENTS {
                 break;
             }
             match item {
@@ -88,10 +84,6 @@ impl Picture {
                     let path = tf * bez(segs);
                     let Some(region) = get_region(region) else { continue };
                     if in_range(path.bounding_box()) {
-                        // This item's own region is expanded by now: it counts before it is kept.
-                        if total_elements.saturating_add(path.elements().len()).saturating_add(region_elements.get()) > MAX_PLACED_ELEMENTS {
-                            break;
-                        }
                         total_elements = total_elements.saturating_add(path.elements().len());
                         let stroke = stroke.map(|(c, w)| (c, f64::from(w) * k));
                         out.push(PlacedItem::Path { path, fill: *fill, stroke, even_odd: *even_odd, region });
@@ -105,9 +97,6 @@ impl Picture {
                         * Affine::scale_non_uniform(f64::from(t.cell[0]), f64::from(t.cell[1]));
                     let det = tile.determinant();
                     if in_range(path.bounding_box()) && det.is_finite() && det.abs() > 1e-12 {
-                        if total_elements.saturating_add(path.elements().len()).saturating_add(region_elements.get()) > MAX_PLACED_ELEMENTS {
-                            break;
-                        }
                         total_elements = total_elements.saturating_add(path.elements().len());
                         out.push(PlacedItem::Pattern { index, path, even_odd: *even_odd, tile, region });
                     }

crates/metafile/src/tests.rs

@@ -897,24 +897,6 @@ fn place_fits_the_frame_scales_strokes_and_drops_far_paths() {
     assert_eq!(stroke.map(|s| s.1), Some(8.0));
 }
 
-#[test]
-fn place_counts_the_items_own_clip_region_before_keeping_it() {
-    // A path of 499,999 elements nearly fills the 500,000 budget; a second one whose new clip
-    // region adds elements goes past it and is left out.
-    let mut segs = vec![Seg::Move(0.0, 0.0)];
-    segs.extend((0..499_998).map(|i| Seg::Line(1.0 + (i % 50) as f32, 1.0)));
-    let big = Item::Path { segs, fill: None, stroke: None, even_odd: false, region: None };
-    let small = Item::Path {
-        segs: vec![Seg::Move(0.0, 0.0), Seg::Line(1.0, 1.0)],
-        fill: None,
-        stroke: None,
-        even_odd: false,
-        region: Some(vec![[0.0, 0.0, 50.0, 25.0], [10.0, 10.0, 20.0, 20.0]]),
-    };
-    let pic = crate::Picture { width: 100.0, height: 50.0, items: vec![big, small] };
-    assert_eq!(pic.place(kurbo::Rect::new(0.0, 0.0, 100.0, 50.0), [0.0; 4]).len(), 1);
-}
-
 #[test]
 fn place_keeps_only_finite_bounded_bitmaps_with_area() {
     let bitmap = |rect: [f32; 4]| Item::Bitmap { rect, width: 1, height: 1, rgba: vec![0; 4], region: None };
@@ -2092,15 +2074,3 @@ fn clip_polygon_work_bound() {
     let clipped = crate::cliprect::clip_polygon(&poly, &clip);
     assert!(clipped.is_empty(), "excessive polygon clip product rejected");
 }
-
-#[test]
-fn full_canvas_does_not_sample_gradient_tiles() {
-    let mut cv = Canvas::new();
-    let big = || Rgba { w: 1, h: 1, bottom_up: false, px: vec![0u8; MAX_BYTES / 2] };
-    for _ in 0..3 {
-        cv.bitmap([0.0, 0.0, 1.0, 1.0], big());
-    }
-    assert!(cv.full());
-    cv.shade_shape(&crate::geom::rect(0.0, 0.0, 10.0, 10.0), &|_| panic!("sampled a full canvas"));
-    assert_eq!(cv.items.len(), 2);
-}

crates/pptx/src/custgeom.rs

@@ -15,6 +15,10 @@ use crate::xml::{A, El, W};
 
 const DEG: f64 = 60_000.0;
 
+/// Distinct path coordinate spaces whose guides are evaluated for one custom geometry; paths in
+/// further spaces are left out.
+const MAX_SPACES: usize = 16;
+
 /// Guide variables for one shape.
 pub struct Guides {
     vars: HashMap<String, f64>,
@@ -151,14 +155,26 @@ fn pt(g: &Guides, e: &El) -> (f64, f64) {
 pub fn read_cust_geom(cg: &El, w_emu: f64, h_emu: f64) -> Vec<CustomPath> {
     let mut out = vec![];
     let Some(pl) = cg.child("pathLst") else { return out };
+    let mut spaces: HashMap<(u64, u64), Guides> = HashMap::new();
+    // The cap on cached spaces saves guide evaluation: a geometry without guides has none to save.
+    let guided = ["avLst", "gdLst"].iter().any(|n| cg.child(n).is_some_and(|l| l.elements().next().is_some()));
     for p in pl.children_named("path").take(1000) {
         let pw = p.f64("w").unwrap_or(0.0);
         let ph = p.f64("h").unwrap_or(0.0);
         // Guides are evaluated in the path's own space when it has one.
         let (gw, gh) = if pw > 0.0 || ph > 0.0 { (pw, ph) } else { (w_emu, h_emu) };
-        let mut g = Guides::new(gw, gh);
-        g.eval_list(cg.child("avLst"));
-        g.eval_list(cg.child("gdLst"));
+        // The guide lists are evaluated once per coordinate space, not once per path.
+        let key = (gw.to_bits(), gh.to_bits());
+        if !spaces.contains_key(&key) {
+            if guided && spaces.len() >= MAX_SPACES {
+                continue;
+            }
+            let mut g = Guides::new(gw, gh);
+            g.eval_list(cg.child("avLst"));
+            g.eval_list(cg.child("gdLst"));
+            spaces.insert(key, g);
+        }
+        let Some(g) = spaces.get(&key) else { continue };
         let mut d = String::new();
         let mut cur = (0.0, 0.0);
         let mut start = (0.0, 0.0);
@@ -166,7 +182,7 @@ pub fn read_cust_geom(cg: &El, w_emu: f64, h_emu: f64) -> Vec<CustomPath> {
             match c.local() {
                 "moveTo" => {
                     if let Some(q) = c.child("pt") {
-                        cur = pt(&g, q);
+                        cur = pt(g, q);
                         start = cur;
                         d.push_str("M ");
                         num(&mut d, cur.0);
@@ -177,7 +193,7 @@ pub fn read_cust_geom(cg: &El, w_emu: f64, h_emu: f64) -> Vec<CustomPath> {
                 }
                 "lnTo" => {
                     if let Some(q) = c.child("pt") {
-                        cur = pt(&g, q);
+                        cur = pt(g, q);
                         d.push_str("L ");
                         num(&mut d, cur.0);
                         d.push(' ');
@@ -186,7 +202,7 @@ pub fn read_cust_geom(cg: &El, w_emu: f64, h_emu: f64) -> Vec<CustomPath> {
                     }
                 }
                 "cubicBezTo" => {
-                    let pts: Vec<(f64, f64)> = c.children_named("pt").map(|q| pt(&g, q)).collect();
+                    let pts: Vec<(f64, f64)> = c.children_named("pt").map(|q| pt(g, q)).collect();
                     if let [a, b, e] = pts.as_slice() {
                         d.push_str("C ");
                         for (x, y) in [a, b, e] {
@@ -199,7 +215,7 @@ pub fn read_cust_geom(cg: &El, w_emu: f64, h_emu: f64) -> Vec<CustomPath> {
                     }
                 }
                 "quadBezTo" => {
-                    let pts: Vec<(f64, f64)> = c.children_named("pt").map(|q| pt(&g, q)).collect();
+                    let pts: Vec<(f64, f64)> = c.children_named("pt").map(|q| pt(g, q)).collect();
                     if let [a, e] = pts.as_slice() {
                         d.push_str("Q ");
                         for (x, y) in [a, e] {
@@ -470,6 +486,29 @@ mod tests {
         assert_eq!(paths[1].d, "M 1 1 Q 2 2 3 1");
     }
 
+    #[test]
+    fn paths_share_guides_and_distinct_spaces_are_capped() {
+        let path = |w: usize| format!(r#"<a:path w="{w}" h="10"><a:moveTo><a:pt x="mid" y="1"/></a:moveTo></a:path>"#);
+        let paths: String = (1..=40).map(|i| path(10 * i)).collect();
+        let x = format!(
+            r#"<a:custGeom xmlns:a="x"><a:gdLst><a:gd name="mid" fmla="*/ w 1 2"/></a:gdLst><a:pathLst>{paths}{}</a:pathLst></a:custGeom>"#,
+            path(10)
+        );
+        let d = parse(x.as_bytes()).unwrap();
+        let out = read_cust_geom(&d.root, 1.0, 1.0);
+        assert_eq!(out.len(), MAX_SPACES + 1, "16 spaces plus the repeat of the first");
+        assert!(out[0].d.starts_with("M 5 1"), "{}", out[0].d);
+        assert!(out[MAX_SPACES].d.starts_with("M 5 1"), "{}", out[MAX_SPACES].d);
+    }
+
+    #[test]
+    fn guide_free_geometries_are_not_capped_by_space() {
+        let paths: String = (1..=40).map(|i| format!(r#"<a:path w="{}" h="10"><a:moveTo><a:pt x="1" y="1"/></a:moveTo></a:path>"#, 10 * i)).collect();
+        let x = format!(r#"<a:custGeom xmlns:a="x"><a:avLst/><a:gdLst/><a:pathLst>{paths}</a:pathLst></a:custGeom>"#);
+        let d = parse(x.as_bytes()).unwrap();
+        assert_eq!(read_cust_geom(&d.root, 1.0, 1.0).len(), 40);
+    }
+
     #[test]
     fn write_then_read_path() {
         let p = CustomPath { w: 100.0, h: 50.0, d: "M 0 0 L 100 0 C 100 10 90 50 50 50 Q 0 50 0 0 Z".into(), fill: FillMode::Norm, stroke: true };