M14: keep structural page order through inspection (#1182)

* M14: keep structural page IDs in traversal order

* M14: reuse structural page order during inspection

crates/render/src/inspect.rs

@@ -466,6 +466,7 @@ fn inspect_structure(info: &mut DocInfo, fill: impl FnOnce(&mut DocInfo) -> Resu
 
 struct Inspector<'a> {
     doc: &'a dyn Structure,
+    page_ids: Vec<ObjectId>,
     page_index: HashMap<ObjectId, usize>,
     /// Named destinations (`/Names /Dests` tree), keyed by raw string bytes, built once.
     /// Looking each name up by walking the tree was O(links × names): minutes on manuals.
@@ -474,8 +475,9 @@ struct Inspector<'a> {
 
 impl<'a> Inspector<'a> {
     fn new(doc: &'a dyn Structure) -> Self {
-        let page_index = doc.get_pages().into_iter().map(|(n, id)| (id, n as usize - 1)).collect();
-        let mut me = Self { doc, page_index, named: HashMap::new() };
+        let page_ids = doc.page_ids();
+        let page_index = page_ids.iter().copied().enumerate().map(|(index, id)| (id, index)).collect();
+        let mut me = Self { doc, page_ids, page_index, named: HashMap::new() };
         let mut named = HashMap::new();
         if let Some(tree) =
             doc.catalog().ok().and_then(|c| c.get(b"Names").ok()).and_then(|o| me.dict(o)).and_then(|n| n.get(b"Dests").ok()).and_then(|o| me.dict(o))
@@ -523,9 +525,7 @@ impl<'a> Inspector<'a> {
         }
         // Widgets no `/Fields` entry reaches are fields too (Acrobat and the browsers fill them);
         // each is adopted through its topmost unlisted `/Parent`, so a split field stays one.
-        let mut pages: Vec<_> = self.page_index.iter().collect();
-        pages.sort_by_key(|(_, i)| **i);
-        for (&pid, _) in pages {
+        for &pid in &self.page_ids {
             let Ok(page_dict) = self.doc.get_dictionary(pid) else { continue };
             let Ok(Object::Array(annots)) = page_dict.get(b"Annots").map(|o| self.resolve(o)) else { continue };
             for a in annots {
@@ -1185,9 +1185,7 @@ impl<'a> Inspector<'a> {
 
     fn fonts(&self, out: &mut Vec<FontInfo>) {
         let mut seen = HashSet::new();
-        let mut pages: Vec<_> = self.page_index.iter().collect();
-        pages.sort_by_key(|(_, i)| **i);
-        for (&pid, _) in pages {
+        for &pid in &self.page_ids {
             let Ok(fonts) = self.doc.get_page_fonts(pid) else { continue };
             for (_, f) in fonts {
                 let base = f

crates/render/src/structure.rs

@@ -37,8 +37,8 @@ pub(crate) trait Structure {
         self.trailer().get(b"Root").and_then(Object::as_reference).and_then(|id| self.get_dictionary(id))
     }
 
-    fn get_pages(&self) -> BTreeMap<u32, ObjectId> {
-        let mut pages = BTreeMap::new();
+    fn page_ids(&self) -> Vec<ObjectId> {
+        let mut pages = Vec::new();
         let Ok(root) = self.catalog().and_then(|c| c.get(b"Pages")).and_then(Object::as_reference) else { return pages };
         let mut pending = vec![(root, 0usize)];
         let mut seen = HashSet::new();
@@ -49,9 +49,11 @@ pub(crate) trait Structure {
             let Ok(dict) = self.get_dictionary(id) else { continue };
             match dict.get_type() {
                 Ok(b"Page") => {
-                    // Page numbers are 1-based u32s; a tree past u32::MAX pages stops there.
-                    let Some(number) = u32::try_from(pages.len()).ok().and_then(|n| n.checked_add(1)) else { return pages };
-                    pages.insert(number, id);
+                    // Keep the effective u32 page-count bound without trusting /Count.
+                    if u32::try_from(pages.len()).ok().and_then(|n| n.checked_add(1)).is_none() {
+                        return pages;
+                    }
+                    pages.push(id);
                 }
                 Ok(b"Pages") => {
                     if let Ok(kids) = dict.get(b"Kids").and_then(|o| self.dereference(o)).and_then(|(_, o)| o.as_array()) {
@@ -115,7 +117,7 @@ impl Structure for lopdf::Document {
     fn encrypted(&self) -> bool {
         self.is_encrypted() || self.was_encrypted() || self.trailer.has(b"Encrypt")
     }
-    // `get_pages` is not overridden: lopdf's own page-tree walk trusts /Count and pre-allocates
+    // `page_ids` is not overridden: lopdf's own page-tree walk trusts /Count and pre-allocates
     // by it, so a hostile /Count (a fuzz finding: 4294967295) aborted the process with a 48 GiB
     // allocation. The trait's bounded default walk counts the real pages instead.
     fn get_page_fonts(&self, page: ObjectId) -> Result<BTreeMap<Vec<u8>, &Dictionary>> {
@@ -226,8 +228,62 @@ trailer
 << /Size 4 /Root 1 0 R >>
 %%EOF";
         let doc = lopdf::Document::load_mem(&pdf[..]).unwrap();
-        let pages = Structure::get_pages(&doc);
-        assert_eq!(pages.len(), 1, "the walk counts the one real page, not /Count");
+        let pages = Structure::page_ids(&doc);
+        assert_eq!(pages, [(3, 0)], "the walk counts the one real page, not /Count");
+    }
+
+    fn nested_sparse_page_tree(repeated_and_cyclic: bool) -> lopdf::Document {
+        let mut doc = lopdf::Document::with_version("1.7");
+        let mut catalog = Dictionary::new();
+        catalog.set("Type", Object::Name(b"Catalog".to_vec()));
+        catalog.set("Pages", Object::Reference((5, 0)));
+        doc.objects.insert((17, 0), Object::Dictionary(catalog));
+
+        let mut pages = Dictionary::new();
+        pages.set("Type", Object::Name(b"Pages".to_vec()));
+        pages.set("Kids", vec![Object::Reference((31, 0)), Object::Reference((88, 0))]);
+        pages.set("Count", Object::Integer(if repeated_and_cyclic { 5 } else { 3 }));
+        doc.objects.insert((5, 0), Object::Dictionary(pages));
+
+        let mut nested = Dictionary::new();
+        nested.set("Type", Object::Name(b"Pages".to_vec()));
+        nested.set("Parent", Object::Reference((5, 0)));
+        let kids = if repeated_and_cyclic {
+            vec![Object::Reference((12, 0)), Object::Reference((5, 0)), Object::Reference((31, 0)), Object::Reference((42, 0))]
+        } else {
+            vec![Object::Reference((12, 0)), Object::Reference((42, 0))]
+        };
+        nested.set("Kids", kids);
+        nested.set("Count", Object::Integer(if repeated_and_cyclic { 4 } else { 2 }));
+        doc.objects.insert((88, 0), Object::Dictionary(nested));
+
+        for (id, parent) in [(31, 5), (12, 88), (42, 88)] {
+            let mut page = Dictionary::new();
+            page.set("Type", Object::Name(b"Page".to_vec()));
+            page.set("Parent", Object::Reference((parent, 0)));
+            doc.objects.insert((id, 0), Object::Dictionary(page));
+        }
+        doc.max_id = 88;
+        doc.trailer.set("Root", Object::Reference((17, 0)));
+        doc
+    }
+
+    #[test]
+    fn page_walk_keeps_nested_sparse_order_and_first_visit_semantics() {
+        let doc = nested_sparse_page_tree(true);
+        assert_eq!(Structure::page_ids(&doc), [(31, 0), (12, 0), (42, 0)]);
+    }
+
+    #[test]
+    fn lazy_and_compatibility_walks_keep_nested_sparse_order() {
+        let mut source = nested_sparse_page_tree(false);
+        let mut bytes = Vec::new();
+        source.save_to(&mut bytes).unwrap();
+        let expected = [(31, 0), (12, 0), (42, 0)];
+        let eager = lopdf::Document::load_mem(&bytes).unwrap();
+        let lazy = LazyStructure::new(Arc::new(bytes), None).unwrap();
+        assert_eq!(Structure::page_ids(&eager), expected);
+        assert_eq!(Structure::page_ids(&lazy), expected);
     }
 
     #[test]
@@ -246,10 +302,11 @@ trailer
         source.save_to(&mut bytes).unwrap();
         let cos = Document::open(Arc::new(bytes)).unwrap();
         let compressed = pdfcraft_cos::write_full(&cos, &pdfcraft_cos::SaveOptions::default()).unwrap();
-        let expected = lopdf::Document::load_mem(&compressed).unwrap().get_pages();
+        let eager = lopdf::Document::load_mem(&compressed).unwrap();
+        let expected = Structure::page_ids(&eager);
         let view = LazyStructure::new(Arc::new(compressed), None).unwrap();
-        assert_eq!(view.get_pages().len(), 1);
-        assert_eq!(view.get_pages(), expected);
+        assert_eq!(view.page_ids().len(), 1);
+        assert_eq!(view.page_ids(), expected);
         assert!(!view.failed());
         assert!(
             view.objects.iter().filter(|(_, slot)| slot.get().is_some()).count() <= 3,