diff --git a/crates/organize/src/dedupe.rs b/crates/organize/src/dedupe.rs index 020c0df22..fe5cb0749 100644 --- a/crates/organize/src/dedupe.rs +++ b/crates/organize/src/dedupe.rs @@ -10,7 +10,10 @@ //! encoding dictionaries, and colour-space arrays. Objects whose *identity* matters (pages, //! annotations, form fields, layers, outline items, structure elements) are never merged. //! - Two objects are equal when they are structurally identical with references compared after -//! merging, so a font dictionary merges once its font file has. This runs to a fixpoint. +//! merging, so a font dictionary merges once its font file has. This runs to a fixpoint. A +//! reference to a plain value (a name, number, boolean or string, such as ImageMagick's +//! `/ColorSpace 10 0 R` → `/DeviceRGB`) compares as that value; the value objects themselves +//! are never merged. //! - Only `candidates` (objects created by the operation) are ever replaced or rewritten. Existing //! objects can serve as the merge target but are not changed, so an incremental save stays //! small. @@ -84,7 +87,14 @@ fn canon(map: &HashMap, mut r: ObjRef) -> ObjRef { r } -fn hash_obj(o: &Object, map: &HashMap, h: &mut impl Hasher) { +/// The value behind `r` when it is a plain value (name, number, boolean or string): it then +/// means that value wherever it is used, whichever object holds it (#878). +fn plain_value(doc: &Document, r: ObjRef) -> Option> { + let o = doc.get(r); + matches!(*o, Object::Bool(_) | Object::Int(_) | Object::Real(_) | Object::Name(_) | Object::String(_)).then_some(o) +} + +fn hash_obj(o: &Object, doc: &Document, map: &HashMap, h: &mut impl Hasher) { match o { Object::Null => 0u8.hash(h), Object::Bool(b) => (1u8, b).hash(h), @@ -95,7 +105,7 @@ fn hash_obj(o: &Object, map: &HashMap, h: &mut impl Hasher) { Object::Array(a) => { (6u8, a.len()).hash(h); for x in a { - hash_obj(x, map, h); + hash_obj(x, doc, map, h); } } Object::Dict(d) => { @@ -104,28 +114,39 @@ fn hash_obj(o: &Object, map: &HashMap, h: &mut impl Hasher) { for (k, v) in d.iter() { let mut eh = std::collections::hash_map::DefaultHasher::new(); k.hash(&mut eh); - hash_obj(v, map, &mut eh); + hash_obj(v, doc, map, &mut eh); acc = acc.wrapping_add(eh.finish()); } (7u8, d.len(), acc).hash(h); } Object::Stream(s) => { 8u8.hash(h); - hash_obj(&Object::Dict(s.dict.clone()), map, h); + hash_obj(&Object::Dict(s.dict.clone()), doc, map, h); s.raw[..].hash(h); } Object::Ref(r) => { let c = canon(map, *r); - (9u8, c.num, c.generation).hash(h); + match plain_value(doc, c) { + Some(v) => hash_obj(&v, doc, map, h), + None => (9u8, c.num, c.generation).hash(h), + } } } } -fn eq_obj(a: &Object, b: &Object, map: &HashMap) -> bool { +fn eq_obj(a: &Object, b: &Object, doc: &Document, map: &HashMap) -> bool { + // Plain values behind references compare as the values (as `hash_obj` hashes them). + let plain = |o: &Object| if let Object::Ref(r) = o { plain_value(doc, canon(map, *r)) } else { None }; + match (plain(a), plain(b)) { + (Some(x), Some(y)) => return eq_obj(&x, &y, doc, map), + (Some(x), None) => return eq_obj(&x, b, doc, map), + (None, Some(y)) => return eq_obj(a, &y, doc, map), + (None, None) => {} + } match (a, b) { - (Object::Array(x), Object::Array(y)) => x.len() == y.len() && x.iter().zip(y).all(|(p, q)| eq_obj(p, q, map)), - (Object::Dict(x), Object::Dict(y)) => x.len() == y.len() && x.iter().all(|(k, v)| y.get(k).is_some_and(|w| eq_obj(v, w, map))), - (Object::Stream(x), Object::Stream(y)) => x.raw == y.raw && eq_obj(&Object::Dict(x.dict.clone()), &Object::Dict(y.dict.clone()), map), + (Object::Array(x), Object::Array(y)) => x.len() == y.len() && x.iter().zip(y).all(|(p, q)| eq_obj(p, q, doc, map)), + (Object::Dict(x), Object::Dict(y)) => x.len() == y.len() && x.iter().all(|(k, v)| y.get(k).is_some_and(|w| eq_obj(v, w, doc, map))), + (Object::Stream(x), Object::Stream(y)) => x.raw == y.raw && eq_obj(&Object::Dict(x.dict.clone()), &Object::Dict(y.dict.clone()), doc, map), (Object::Ref(x), Object::Ref(y)) => canon(map, *x) == canon(map, *y), (Object::Real(x), Object::Real(y)) => x.to_bits() == y.to_bits(), (Object::String(x), Object::String(y)) => x.bytes == y.bytes, @@ -201,9 +222,9 @@ pub fn dedupe_resources(doc: &mut Document, candidates: &[ObjRef], index_existin continue; } let mut h = std::collections::hash_map::DefaultHasher::new(); - hash_obj(o, &map, &mut h); + hash_obj(o, doc, &map, &mut h); let bucket = buckets.entry(h.finish()).or_default(); - let target = bucket.iter().map(|j| &universe[*j]).find(|(_, other)| eq_obj(o, other, &map)).map(|(t, _)| *t); + let target = bucket.iter().map(|j| &universe[*j]).find(|(_, other)| eq_obj(o, other, doc, &map)).map(|(t, _)| *t); match target { Some(t) if cand.contains(r) => { map.insert(*r, t); @@ -304,6 +325,35 @@ mod tests { assert_eq!(doc.get(p1), doc.get(p2), "both pages now use the one graphics state"); } + fn image(doc: &mut Document, colour_space: Object) -> ObjRef { + let mut d = Dict::new(); + d.set(b"Subtype".to_vec(), Object::name("Image")); + d.set(b"ColorSpace".to_vec(), colour_space); + doc.add(Object::Stream(Stream { dict: d, raw: b"pixels".to_vec().into() })) + } + + #[test] + fn references_to_plain_values_compare_as_the_values() { + // ImageMagick writes `/ColorSpace 10 0 R` with `10 0 obj /DeviceRGB`, one per file (#878). + let mut doc = Document::new_empty(); + let rgb1 = doc.add(Object::name("DeviceRGB")); + let rgb2 = doc.add(Object::name("DeviceRGB")); + let grey = doc.add(Object::name("DeviceGray")); + let a = image(&mut doc, Object::Ref(rgb1)); + let b = image(&mut doc, Object::Ref(rgb2)); + let c = image(&mut doc, Object::name("DeviceRGB")); // written directly: the same image + let d = image(&mut doc, Object::Ref(grey)); // same pixels, another colour space + let mut page = Dict::new(); + page.set(b"Images".to_vec(), Object::Array([a, b, c, d].map(Object::Ref).to_vec())); + let p = doc.add(Object::Dict(page)); + assert_eq!(dedupe_resources(&mut doc, &[rgb1, rgb2, grey, a, b, c, d, p], false), 2); + let page = doc.get(p); + let images = page.as_dict().unwrap().get(b"Images").unwrap().as_array().unwrap().clone(); + assert_eq!(images, [a, a, a, d].map(Object::Ref).to_vec()); + let nums = doc.object_numbers(); + assert!(nums.contains(&rgb1.num) && nums.contains(&rgb2.num), "the values themselves are never merged"); + } + #[test] fn existing_objects_are_targets_but_never_changed() { let mut doc = Document::new_empty(); diff --git a/crates/organize/src/tests.rs b/crates/organize/src/tests.rs index a6d4d978a..baa070998 100644 --- a/crates/organize/src/tests.rs +++ b/crates/organize/src/tests.rs @@ -734,6 +734,57 @@ fn combine_and_insert_share_identical_resources() { assert_eq!(labels(&d), ["A1", "A2", "A3", "A1", "A2"]); } +/// A one-page document drawing one image, written the way ImageMagick writes it: the colour +/// space is an indirect `/DeviceRGB` object of its own (#878). +fn indirect_colour_space_doc() -> Document { + let objs: Vec> = vec![ + b"<< /Type /Catalog /Pages 2 0 R >>".to_vec(), + b"<< /Type /Pages /Kids [3 0 R] /Count 1 >>".to_vec(), + b"<< /Type /Page /Parent 2 0 R /MediaBox [0 0 100 100] /Resources << /XObject << /Im0 5 0 R >> >> /Contents 4 0 R >>".to_vec(), + stream(b"q 100 0 0 100 0 0 cm /Im0 Do Q"), + [ + b"<< /Type /XObject /Subtype /Image /Width 2 /Height 1 /ColorSpace 6 0 R /BitsPerComponent 8 /Length 6 >>\nstream\n".as_slice(), + &[255, 0, 0, 0, 0, 255], + b"\nendstream", + ] + .concat(), + b"/DeviceRGB".to_vec(), + ]; + let mut out = b"%PDF-1.7\n".to_vec(); + let mut offsets = Vec::new(); + for (i, o) in objs.iter().enumerate() { + offsets.push(out.len()); + out.extend_from_slice(format!("{} 0 obj\n", i + 1).as_bytes()); + out.extend_from_slice(o); + out.extend_from_slice(b"\nendobj\n"); + } + let xref = out.len(); + out.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", objs.len() + 1).as_bytes()); + for o in offsets { + out.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes()); + } + out.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n", objs.len() + 1).as_bytes()); + open(out) +} + +#[test] +fn combine_stores_an_image_once_when_its_colour_space_is_an_indirect_name() { + let images = |d: &Document| { + d.object_numbers() + .into_iter() + .filter(|n| matches!(&*d.get(ObjRef::new(*n, d.generation(*n))), Object::Stream(s) if s.dict.name(b"Subtype") == Some(b"Image"))) + .count() + }; + let src = indirect_colour_space_doc(); + let out = full_roundtrip(&crate::combine(&[("one", &src), ("two", &src), ("three", &src)]).unwrap()); + assert_eq!(images(&out), 1); + assert_eq!(pages(&out).unwrap().len(), 3); + // Inserting the same pages again reuses the image already there. + let mut d = indirect_colour_space_doc(); + crate::import_pages(&mut d, &indirect_colour_space_doc(), &[0], 1).unwrap(); + assert_eq!(images(&full_roundtrip(&d)), 1); +} + // ---- bookmarks (M4.6) -------------------------------------------------------------------------- fn titles(b: &[crate::Bookmark]) -> Vec {