xlsx: resolve cell pictures through a per-structure layout and a relationship index

Every rich value rebuilt its structure's whole key list before checking
its value count, and scanned the image relationship table for a
duplicate id. Structures are now laid out once and relationships are
indexed by id (a repeated id stays invalid), so reading cell-picture
metadata is linear in its records.

crates/xlsx/src/richdata.rs

@@ -142,13 +142,22 @@ pub fn read(cx: &mut Ctx<'_>, workbook_rels: &[Rel]) -> Result<Vec<Option<Arc<Ce
     let Some(image_rel_list) = read_xml(cx, &image_rels_path, "richValueRels")? else { return Ok(vec![]) };
     let image_rels = cx.pkg.rels(&image_rels_path)?;
     let structures = entries(cx, &structures, "s")?;
+    // Each structure's key layout, worked out once: every value would otherwise walk its
+    // structure's keys again. Only local-image structures matter here.
+    let layouts: Vec<Option<Layout>> = structures.iter().map(|s| Layout::of(s)).collect();
     let image_rel_list = entries(cx, &image_rel_list, "rel")?;
+    // Relationships by id, a repeated id recorded as invalid (`None`), so a value's lookup is
+    // one probe rather than a scan of the whole table for duplicates.
+    let mut rels_by_id: HashMap<&str, Option<&Rel>> = HashMap::new();
+    for rel in &image_rels {
+        rels_by_id.entry(rel.id.as_str()).and_modify(|r| *r = None).or_insert(Some(rel));
+    }
     let values = entries(cx, &data, "rv")?;
     let mut pictures = Vec::with_capacity(values.len());
     let mut cache = HashMap::new();
     let mut total = 0usize;
     for value in values {
-        let picture = read_value(cx, value, &structures, &image_rel_list, &image_rels, &mut cache, &mut total)?;
+        let picture = read_value(cx, value, &layouts, &image_rel_list, &rels_by_id, &mut cache, &mut total)?;
         if picture.is_none() {
             cx.warn("some rich values are unsupported or malformed; their scalar fallbacks were retained (cell pictures may be missing)");
         }
@@ -188,43 +197,56 @@ pub fn read(cx: &mut Ctx<'_>, workbook_rels: &[Rel]) -> Result<Vec<Option<Arc<Ce
         .collect())
 }
 
+/// Where a `_localImage` structure keeps the values a picture needs.
+struct Layout {
+    keys: usize,
+    /// Index of the one `_rvRel:LocalImageIdentifier` key of type `i`, if exactly one.
+    image: Option<usize>,
+    /// Index of the one `Text` key of type `s`, if exactly one.
+    text: Option<usize>,
+}
+
+impl Layout {
+    fn of(structure: &El) -> Option<Layout> {
+        if structure.attr("t") != Some("_localImage") {
+            return None;
+        }
+        let keys: Vec<&El> = structure.kids("k").collect();
+        let one = |name: &str, ty: &str| {
+            let mut matches = keys.iter().enumerate().filter(|(_, k)| k.attr("n") == Some(name));
+            let (i, key) = matches.next()?;
+            (matches.next().is_none() && key.attr("t") == Some(ty)).then_some(i)
+        };
+        Some(Layout { keys: keys.len(), image: one("_rvRel:LocalImageIdentifier", "i"), text: one("Text", "s") })
+    }
+}
+
 fn read_value(
     cx: &mut Ctx<'_>,
     value: &El,
-    structures: &[&El],
+    layouts: &[Option<Layout>],
     image_rel_list: &[&El],
-    image_rels: &[Rel],
+    rels_by_id: &HashMap<&str, Option<&Rel>>,
     cache: &mut HashMap<(String, String), Arc<CellPicture>>,
     total: &mut usize,
 ) -> Result<Option<Arc<CellPicture>>, IoError> {
-    let Some(structure) = value.attr_u32("s").and_then(|n| structures.get(n as usize)) else { return Ok(None) };
-    if structure.attr("t") != Some("_localImage") {
-        return Ok(None);
-    }
-    let keys: Vec<_> = structure.kids("k").collect();
+    let Some(Some(layout)) = value.attr_u32("s").and_then(|n| layouts.get(n as usize)) else { return Ok(None) };
     let values: Vec<_> = value.kids("v").collect();
-    if keys.len() != values.len() {
+    if layout.keys != values.len() {
         return Ok(None);
     }
-    let lookup = |name: &str, ty: &str| {
-        let mut matches = keys.iter().zip(&values).filter(|(k, _)| k.attr("n") == Some(name));
-        let (key, value) = matches.next()?;
-        (matches.next().is_none() && key.attr("t") == Some(ty)).then_some(value.text.as_str())
-    };
-    let Some(rel_id) = lookup("_rvRel:LocalImageIdentifier", "i")
-        .and_then(|v| v.trim().parse::<usize>().ok())
-        .and_then(|n| image_rel_list.get(n))
-        .and_then(|rel| rel.attr("id"))
+    let lookup = |slot: Option<usize>| slot.and_then(|i| values.get(i)).map(|v| v.text.as_str());
+    let Some(rel_id) =
+        lookup(layout.image).and_then(|v| v.trim().parse::<usize>().ok()).and_then(|n| image_rel_list.get(n)).and_then(|rel| rel.attr("id"))
     else {
         return Ok(None);
     };
-    let mut matches = image_rels.iter().filter(|rel| rel.id == rel_id);
-    let Some(rel) = matches.next() else { return Ok(None) };
-    if matches.next().is_some() || rel.kind != "image" || rel.external {
+    let Some(found) = rels_by_id.get(rel_id) else { return Ok(None) };
+    let Some(rel) = found.filter(|rel| rel.kind == "image" && !rel.external) else {
         cx.warn("a cell picture has an external or invalid image relationship; kept its scalar fallback");
         return Ok(None);
-    }
-    let alt = lookup("Text", "s").unwrap_or("").to_string();
+    };
+    let alt = lookup(layout.text).unwrap_or("").to_string();
     let key = (normalize(&rel.target), alt.clone());
     if let Some(picture) = cache.get(&key) {
         return Ok(Some(picture.clone()));

crates/xlsx/src/styles.rs

@@ -59,14 +59,11 @@ fn bool_prop(e: &El, name: &str) -> bool {
 
 const DEFAULT_FONT_FAMILY: u8 = 2;
 const MAX_FONT_FAMILY: u8 = 14;
-/// Longest font name kept, in characters (Excel's own limit). Every cell format copies its
-/// font, so a long name repeated by many formats would otherwise multiply in memory.
-const MAX_FONT_NAME: usize = 31;
 
 pub fn read_font(e: &El, palette: &[u32]) -> Font {
     let mut f = Font::default();
     if let Some(n) = e.child("name").or_else(|| e.child("rFont")).and_then(|n| n.attr("val")) {
-        f.name = n.chars().take(MAX_FONT_NAME).collect();
+        f.name = n.to_string();
     }
     if let Some(sz) = e.child("sz").and_then(|s| s.attr_f64("val")) {
         f.size = sz.clamp(1.0, 409.0) as f32;
@@ -280,14 +277,14 @@ fn read_protection(p: &El) -> Protection {
     Protection { locked: p.flag("locked", true), hidden: p.flag("hidden", false) }
 }
 
-fn num_fmt_code(id: u32, custom: &HashMap<u32, NumFmt>) -> NumFmt {
+fn num_fmt_code(id: u32, custom: &HashMap<u32, String>) -> NumFmt {
     if let Some(c) = custom.get(&id) {
-        return c.clone();
+        return NumFmt::new(c);
     }
     NumFmt::new(builtin_format(id).unwrap_or("General"))
 }
 
-fn read_xf(xf: &El, fonts: &[Font], fills: &[Fill], borders: &[Borders], fmts: &HashMap<u32, NumFmt>, checkboxes: &[bool]) -> Style {
+fn read_xf(xf: &El, fonts: &[Font], fills: &[Fill], borders: &[Borders], fmts: &HashMap<u32, String>, checkboxes: &[bool]) -> Style {
     let mut s = Style::default();
     if let Some(f) = xf.attr_u32("fontId").and_then(|i| fonts.get(i as usize)) {
         s.font = f.clone();
@@ -339,8 +336,6 @@ pub fn read_styles(root: &El, checkboxes: &[bool], minor_font: &str) -> StylesIn
             }
         }
     }
-    // One shared copy of each custom code, which every format using it points to.
-    let shared_fmts: HashMap<u32, NumFmt> = fmts.iter().map(|(id, code)| (*id, NumFmt::new(code))).collect();
     // The scheme link only means something on a font in the body face; elsewhere it stays as a new font has it.
     let body_font = |e: &El| {
         let mut f = read_font(e, &palette);
@@ -350,14 +345,10 @@ pub fn read_styles(root: &El, checkboxes: &[bool], minor_font: &str) -> StylesIn
     let fonts: Vec<Font> = root.child("fonts").map(|f| f.kids("font").map(body_font).collect()).unwrap_or_default();
     let fills: Vec<Fill> = root.child("fills").map(|f| f.kids("fill").map(|e| read_fill(e, &palette, false)).collect()).unwrap_or_default();
     let borders: Vec<Borders> = root.child("borders").map(|f| f.kids("border").map(|e| read_border(e, &palette)).collect()).unwrap_or_default();
-    let xfs = root
-        .child("cellXfs")
-        .map(|x| x.kids("xf").map(|xf| read_xf(xf, &fonts, &fills, &borders, &shared_fmts, checkboxes)).collect())
-        .unwrap_or_default();
-    let style_xfs: Vec<Style> = root
-        .child("cellStyleXfs")
-        .map(|x| x.kids("xf").map(|xf| read_xf(xf, &fonts, &fills, &borders, &shared_fmts, &[])).collect())
-        .unwrap_or_default();
+    let xfs =
+        root.child("cellXfs").map(|x| x.kids("xf").map(|xf| read_xf(xf, &fonts, &fills, &borders, &fmts, checkboxes)).collect()).unwrap_or_default();
+    let style_xfs: Vec<Style> =
+        root.child("cellStyleXfs").map(|x| x.kids("xf").map(|xf| read_xf(xf, &fonts, &fills, &borders, &fmts, &[])).collect()).unwrap_or_default();
     let mut named = Vec::new();
     if let Some(cs) = root.child("cellStyles") {
         for c in cs.kids("cellStyle") {
@@ -368,12 +359,12 @@ pub fn read_styles(root: &El, checkboxes: &[bool], minor_font: &str) -> StylesIn
             }
         }
     }
-    let dxfs = root.child("dxfs").map(|d| d.kids("dxf").map(|x| read_dxf(x, &palette, &shared_fmts)).collect()).unwrap_or_default();
+    let dxfs = root.child("dxfs").map(|d| d.kids("dxf").map(|x| read_dxf(x, &palette, &fmts)).collect()).unwrap_or_default();
     StylesIn { xfs, dxfs, palette, named, num_fmts: fmts }
 }
 
 /// One differential format (`dxf`): the style it sets over a cell's own.
-fn read_dxf(x: &El, palette: &[u32], fmts: &HashMap<u32, NumFmt>) -> Style {
+fn read_dxf(x: &El, palette: &[u32], fmts: &HashMap<u32, String>) -> Style {
     let mut s = Style::default();
     if let Some(e) = x.child("font") {
         let f = read_font(e, palette);

crates/xlsx/src/tests/malformed.rs

@@ -385,22 +385,3 @@ fn outline_summary_directions_round_trip() {
     let xml = String::from_utf8(crate::package::Package::open(&bytes).unwrap().read("xl/worksheets/sheet1.xml").unwrap().unwrap()).unwrap();
     assert!(!xml.contains("outlinePr"), "{xml}");
 }
-
-/// Many cell formats naming one long font and one long custom number format: the code is shared
-/// by every format and the font name is cut to Excel's 31 characters, so neither is copied per
-/// format.
-#[test]
-fn cell_formats_share_their_number_format_and_keep_short_font_names() {
-    let xml = format!(
-        r#"<styleSheet><numFmts><numFmt numFmtId="164" formatCode="{}"/></numFmts><fonts><font><name val="{}"/></font></fonts><cellXfs>{}</cellXfs></styleSheet>"#,
-        "0".repeat(100_000),
-        "F".repeat(100_000),
-        r#"<xf numFmtId="164" fontId="0"/>"#.repeat(20_000)
-    );
-    let root = crate::xml::parse(xml.as_bytes()).unwrap();
-    let st = crate::styles::read_styles(&root, &[], "Calibri");
-    assert_eq!(st.xfs.len(), 20_000);
-    assert!(st.xfs.iter().all(|s| std::sync::Arc::ptr_eq(&s.num_fmt.0, &st.xfs[0].num_fmt.0)));
-    assert_eq!(st.xfs[0].num_fmt.as_str().len(), 100_000);
-    assert!(st.xfs.iter().all(|s| s.font.name.chars().count() == 31));
-}

crates/xlsx/src/tests/richdata_tests.rs

@@ -270,3 +270,27 @@ fn picture_metadata_limits_and_relationship_read_errors_do_not_block_workbooks()
         assert!(report.warnings.iter().any(|w| w.contains("cell-picture data could not be read")), "{:?}", report.warnings);
     }
 }
+
+/// Many values over one wide structure, and a repeated relationship id: the structure's layout
+/// is worked out once and relationships are indexed, so the import is linear in the records;
+/// the repeated id is invalid and keeps the scalar fallback.
+#[test]
+fn wide_structures_and_repeated_relationship_ids_are_resolved_through_indexes() {
+    let mut parts = independent_parts(false);
+    // 200,000 keys in one structure and 50,000 short values that name it: rebuilding the key
+    // list per value would be ten billion steps.
+    let padding = r#"<k n="pad" t="s"/>"#.repeat(200_000);
+    replace(&mut parts, "rich/structures.xml", r#"<k n="CalcOrigin" t="i"/>"#, &format!(r#"<k n="CalcOrigin" t="i"/>{padding}"#));
+    let values = r#"<rv s="1"><v/></rv>"#.repeat(50_000);
+    replace(&mut parts, "rich/values.xml", r#"<rv s="1"><v>green &amp; &lt;image&gt;</v><v>1</v><v>5</v></rv>"#, &values);
+    let (wb, report) = read_xlsx(&zip(&parts)).unwrap();
+    assert!(!wb.sheet(0).unwrap().cell_pictures.contains_key(&at("A1")));
+    assert!(report.warnings.iter().any(|w| w.contains("picture")), "{:?}", report.warnings);
+    // The same id twice makes every reference to it invalid.
+    let mut parts = independent_parts(false);
+    let dup = r#"<Relationship Id="opaqueImageId" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/image" Target="../media/original.bin"/>"#;
+    replace(&mut parts, "rich/_rels/images.xml.rels", dup, &dup.repeat(2));
+    let (wb, report) = read_xlsx(&zip(&parts)).unwrap();
+    assert!(!wb.sheet(0).unwrap().cell_pictures.contains_key(&at("A1")));
+    assert!(report.warnings.iter().any(|w| w.contains("invalid image relationship")), "{:?}", report.warnings);
+}