address review: charge every copied node per tile; skip the image cache for named colour spaces

crates/pdf/src/build.rs

@@ -19,7 +19,6 @@ pub(crate) struct Builder {
     /// Which of the finished top-level nodes are optional-content layers.
     pub top_layers: Vec<bool>,
     pub skipped: Vec<String>,
-    pub shapes: usize,
 }
 
 pub(crate) fn group(name: &str) -> Group {
@@ -28,7 +27,7 @@ pub(crate) fn group(name: &str) -> Group {
 
 impl Builder {
     pub fn new() -> Builder {
-        Builder { stack: vec![(group("page"), Kind::Plain)], top_layers: vec![], skipped: vec![], shapes: 0 }
+        Builder { stack: vec![(group("page"), Kind::Plain)], top_layers: vec![], skipped: vec![] }
     }
 
     pub fn depth(&self) -> usize {
@@ -94,7 +93,6 @@ impl Builder {
         if path.elements().is_empty() || (fill.is_none() && stroke.is_none()) {
             return;
         }
-        self.shapes += 1;
         let (f, fo, rule) = match fill {
             Some((p, o, r)) => (Some(p), o, r),
             None => (None, 1.0, FillRule::NonZero),

crates/pdf/src/page.rs

@@ -809,7 +809,6 @@ impl<'a> Interp<'a> {
         self.gs = saved_gs;
         self.saved = saved_stack;
         self.path = saved_path;
-        let cell_shapes = cell_b.shapes;
         let (cell, _, sk) = cell_b.finish();
         for s in sk {
             self.b.skip(&s);
@@ -834,10 +833,19 @@ impl<'a> Interp<'a> {
             self.b.skip("tiling pattern (too many tiles)");
             return;
         }
-        // Every tile copies the cell: the shapes copied count as operations, so nested
-        // patterns cannot multiply a cell past the budget.
+        // Every tile copies the cell: the nodes copied (shapes, images and groups, with what the
+        // groups hold) count as operations, so nested patterns cannot multiply a cell past the
+        // budget.
         let tiles_n = ((i1 - i0 + 1.0) * (j1 - j0 + 1.0)).max(1.0) as usize;
-        let Some(cost) = tiles_n.checked_mul(cell_shapes.max(1)).filter(|c| *c <= self.budget) else {
+        let mut cell_nodes = 0usize;
+        let mut pending: Vec<&Node> = cell.iter().collect();
+        while let Some(n) = pending.pop() {
+            cell_nodes += 1;
+            if let Node::Group(g) = n {
+                pending.extend(g.children.iter());
+            }
+        }
+        let Some(cost) = tiles_n.checked_mul(cell_nodes.max(1)).filter(|c| *c <= self.budget) else {
             self.b.skip("tiling pattern (operation limit)");
             return;
         };
@@ -1046,7 +1054,11 @@ impl<'a> Interp<'a> {
             Fill::Color(c) => *c,
             _ => [0.0; 3],
         };
-        let cache_key = key.map(|k| (k, fill.map(|x| (x.clamp(0.0, 1.0) * 255.0).round() as u8)));
+        // A named colour space other than a device one is looked up in `res`, which differs
+        // between placements, so those images are decoded each time.
+        let named_space =
+            self.file.get(d, "ColorSpace").and_then(Obj::name).is_some_and(|n| !matches!(n, "DeviceGray" | "DeviceRGB" | "DeviceCMYK" | "G" | "RGB" | "CMYK"));
+        let cache_key = key.filter(|_| !named_space).map(|k| (k, fill.map(|x| (x.clamp(0.0, 1.0) * 255.0).round() as u8)));
         let decoded = match cache_key.as_ref().and_then(|k| self.images.get(k)) {
             Some(img) => img.clone(),
             None => {

crates/pdf/src/tests/import.rs

@@ -652,3 +652,50 @@ fn a_tiling_pattern_is_charged_for_every_tile_it_copies() {
     .unwrap();
     assert_eq!(doc.skipped, ["tiling pattern (operation limit)"]);
 }
+
+#[test]
+fn a_tiling_pattern_is_charged_for_images_it_copies() {
+    // The cell holds 8,000 image placements, which are not shapes: each tile still copies them.
+    let cell = "/Im1 Do ".repeat(8_000).into_bytes();
+    let doc = pdf_soon(page_pdf(
+        "/Pattern cs /P1 scn 0 0 50 50 re f",
+        "/Pattern << /P1 10 0 R >>",
+        vec![
+            (
+                10,
+                "<< /Type /Pattern /PatternType 1 /PaintType 1 /TilingType 1 /BBox [0 0 1 1] /XStep 1 /YStep 1 /Resources << /XObject << /Im1 11 0 R >> >> >>"
+                    .into(),
+                Some(cell),
+            ),
+            (11, "<< /Type /XObject /Subtype /Image /Width 1 /Height 1 /ColorSpace /DeviceGray /BitsPerComponent 8 >>".into(), Some(vec![255])),
+        ],
+    ))
+    .unwrap();
+    assert_eq!(doc.skipped, ["tiling pattern (operation limit)"]);
+}
+
+#[test]
+fn an_image_with_a_named_colour_space_is_not_shared_between_resource_dictionaries() {
+    fn images<'a>(g: &'a effectcraft_svg::Group, out: &mut Vec<&'a effectcraft_svg::Image>) {
+        for n in &g.children {
+            match n {
+                Node::Group(g) => images(g, out),
+                Node::Image(i) => out.push(i),
+                Node::Shape(_) => {}
+            }
+        }
+    }
+    let form = |space: &str| {
+        format!("<< /Type /XObject /Subtype /Form /BBox [0 0 200 100] /Resources << /XObject << /Im 10 0 R >> /ColorSpace << /CS1 /{space} >> >> >>")
+    };
+    let objs = vec![
+        (10, "<< /Type /XObject /Subtype /Image /Width 1 /Height 1 /ColorSpace /CS1 /BitsPerComponent 8 >>".into(), Some(vec![10, 20, 30])),
+        (11, form("DeviceGray"), Some(b"/Im Do".to_vec())),
+        (12, form("DeviceRGB"), Some(b"/Im Do".to_vec())),
+    ];
+    let doc = parse(&page_pdf("/FA Do /FB Do", "/XObject << /FA 11 0 R /FB 12 0 R >>", objs)).unwrap();
+    let mut found = vec![];
+    images(&doc.root, &mut found);
+    assert_eq!(found.len(), 2, "{:?}", doc.skipped);
+    assert_ne!(found[0].rgba, found[1].rgba, "each placement is decoded with its own colour space");
+}