pptx: take nested math text once and check an object's part for relationships once

crates/pptx/src/read.rs

@@ -46,6 +46,9 @@ pub struct Imp<'a> {
     pub tag_bytes: usize,
     /// The tag budget ran out and the warning was given.
     pub tags_capped: bool,
+    /// Whether a part has relationships of its own, by normalized part name: looked up once
+    /// however many embedded objects point at the part.
+    pub has_rels: HashMap<String, bool>,
 }
 
 impl Imp<'_> {
@@ -100,7 +103,8 @@ impl Part {
     }
     /// Like [`Part::keep`], also declaring prefixes that only an enclosing wrapper binds.
     pub fn keep_inner(&self, el: &El) -> String {
-        keep_with(el, &[&self.ns, &self.inner_ns])
+        let ns: Vec<(String, String)> = self.ns.iter().chain(&self.inner_ns).cloned().collect();
+        keep_xml(el, &ns)
     }
 }
 
@@ -135,58 +139,36 @@ fn inner_decls(root: &El, ns: &[(String, String)]) -> Vec<(String, String)> {
     out
 }
 
-/// Longest namespace name copied onto a kept fragment or a resolved `mc` branch. Real ones are
-/// URIs of well under 100 bytes; a longer one is not copied, so one declaration cannot be
-/// multiplied by every fragment that uses its prefix.
-const MAX_NS_VALUE: usize = 2048;
-
-/// Prefixes `el` and its descendants use in element and attribute names, and those listed in
-/// `mc:Ignorable` (which must stay declared for the markup-compatibility rules to hold).
-fn used_prefixes(el: &El) -> HashSet<&str> {
-    fn rec<'a>(e: &'a El, used: &mut HashSet<&'a str>, depth: usize) {
+/// Serialize `el`, declaring every prefix it uses that the source root declared.
+pub fn keep_xml(el: &El, ns: &[(String, String)]) -> String {
+    let mut used = HashSet::new();
+    fn rec(e: &El, used: &mut HashSet<String>, depth: usize) {
         if depth > crate::xml::MAX_DEPTH {
             return;
         }
         if let Some((p, _)) = e.name.split_once(':') {
-            used.insert(p);
+            used.insert(p.to_string());
         }
-        for (k, v) in &e.attrs {
-            if let Some((p, local)) = k.split_once(':')
+        for (k, _) in &e.attrs {
+            if let Some((p, _)) = k.split_once(':')
                 && p != "xmlns"
                 && p != "xml"
             {
-                used.insert(p);
-                if local == "Ignorable" {
-                    used.extend(v.split_whitespace());
-                }
+                used.insert(p.to_string());
             }
         }
         for c in e.elements() {
             rec(c, used, depth + 1);
         }
     }
-    let mut used = HashSet::new();
     rec(el, &mut used, 0);
-    used
-}
-
-/// Serialize `el`, declaring every prefix it uses that the source root declared.
-pub fn keep_xml(el: &El, ns: &[(String, String)]) -> String {
-    keep_with(el, &[ns])
-}
-
-fn keep_with(el: &El, decls: &[&[(String, String)]]) -> String {
-    let used = used_prefixes(el);
-    let mut have: HashSet<&str> = el.attrs.iter().map(|(a, _)| a.as_str()).collect();
-    let mut extra = Vec::new();
-    for (k, v) in decls.iter().flat_map(|d| d.iter()) {
+    let mut el = el.clone();
+    for (k, v) in ns {
         let p = k.strip_prefix("xmlns:").unwrap_or("");
-        if !p.is_empty() && used.contains(p) && v.len() <= MAX_NS_VALUE && have.insert(k) {
-            extra.push((k.clone(), v.clone()));
+        if !p.is_empty() && used.contains(p) && !el.attrs.iter().any(|(a, _)| a == k) {
+            el.attrs.push((k.clone(), v.clone()));
         }
     }
-    let mut el = el.clone();
-    el.attrs.extend(extra);
     el.to_xml()
 }
 
@@ -336,12 +318,9 @@ fn kept_parts(pkg: &Package, pres_path: &str) -> Vec<deckcraft_model::KeptPart>
             let rp = opc::rels_path(&name);
             if let Some(rels_xml) = text(&rp) {
                 for sub in pkg.rels(&name).items.iter().filter(|s| !s.external && keepable(&s.target)) {
-                    // Checked before the part is read: repeated relationships to one large part
-                    // must not copy it again each time.
-                    if out.len() >= 400 || out.iter().any(|k| k.name.eq_ignore_ascii_case(&sub.target)) {
-                        continue;
-                    }
-                    if let Some(x) = text(&sub.target) {
+                    if let Some(x) = text(&sub.target)
+                        && !out.iter().any(|k| k.name.eq_ignore_ascii_case(&sub.target))
+                    {
                         out.push(KeptPart {
                             content_type: pkg.content_type(&sub.target).unwrap_or_default(),
                             name: sub.target.clone(),
@@ -389,19 +368,13 @@ fn resolve_mc_rels(el: &mut El, depth: usize, rels: Option<&Rels>) {
                     if let Some(b) = branch {
                         for n in &b.children {
                             if let Node::El(e) = n {
-                                // Carry the namespace declarations from the branch that its content
-                                // uses (all of them per child made children x declarations copies).
-                                let used = used_prefixes(e);
-                                let mut have: HashSet<&str> = e.attrs.iter().map(|(a, _)| a.as_str()).collect();
-                                let mut extra = Vec::new();
+                                let mut e = e.clone();
+                                // Carry namespace declarations from the branch onto its content.
                                 for (k, v) in b.attrs.iter().chain(ac.attrs.iter()) {
-                                    let needed = k == "xmlns" || k.strip_prefix("xmlns:").is_some_and(|p| used.contains(p));
-                                    if needed && v.len() <= MAX_NS_VALUE && have.insert(k) {
-                                        extra.push((k.clone(), v.clone()));
+                                    if k.starts_with("xmlns") && !e.attrs.iter().any(|(a, _)| a == k) {
+                                        e.attrs.push((k.clone(), v.clone()));
                                     }
                                 }
-                                let mut e = e.clone();
-                                e.attrs.extend(extra);
                                 out.push(Node::El(e));
                             }
                         }
@@ -476,6 +449,7 @@ pub fn import_theme(bytes: &[u8]) -> Result<(deckcraft_model::theme::Theme, Vec<
         tags_by_part: HashMap::new(),
         tag_bytes: 0,
         tags_capped: false,
+        has_rels: HashMap::new(),
     };
     let part = Part::new(&pkg, &theme_path, &doc);
     let theme = theme::read_theme(&mut imp, &part, &doc.root);
@@ -517,6 +491,7 @@ pub fn import_checked(bytes: &[u8]) -> Result<(Presentation, Vec<String>), PptxE
         tags_by_part: HashMap::new(),
         tag_bytes: 0,
         tags_capped: false,
+        has_rels: HashMap::new(),
     };
     // Slide ids first so hyperlinks anywhere can point at slides.
     let slide_ids = register_slides(&mut imp, &docs, &slide_entries, &mut lost_slides);
@@ -1444,36 +1419,3 @@ fn header_footer(p: &Presentation) -> HeaderFooter {
     }
     hf
 }
-
-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    #[test]
-    fn mc_branches_carry_only_the_declarations_their_content_uses() {
-        // 2,000 declarations on the branch and 2,000 children: each child gets the one it uses
-        // (and the default), not all 2,000.
-        let decls: String = (0..2000).map(|i| format!(r#" xmlns:n{i}="urn:n{i}""#)).collect();
-        let kids: String = (0..2000).map(|i| format!("<n{i}:x/>")).collect();
-        let x = format!(
-            r#"<r xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"><mc:AlternateContent><mc:Fallback xmlns="urn:d"{decls}>{kids}</mc:Fallback></mc:AlternateContent></r>"#
-        );
-        let mut d = crate::xml::parse(x.as_bytes()).unwrap();
-        resolve_mc(&mut d.root, 0);
-        let kids: Vec<&El> = d.root.elements().collect();
-        assert_eq!(kids.len(), 2000);
-        assert_eq!(kids[7].attrs, vec![("xmlns".to_string(), "urn:d".to_string()), ("xmlns:n7".to_string(), "urn:n7".to_string())]);
-    }
-
-    #[test]
-    fn kept_fragments_skip_oversized_namespace_names() {
-        let big = "u".repeat(MAX_NS_VALUE + 1);
-        let ns = vec![("xmlns:a".to_string(), "urn:a".to_string()), ("xmlns:b".to_string(), big)];
-        let d = crate::xml::parse(br#"<a:x xmlns:a="urn:a" xmlns:b="urn:b"><b:y/></a:x>"#).unwrap();
-        let mut el = d.root.clone();
-        el.attrs.clear();
-        let out = keep_xml(&el, &ns);
-        assert!(out.contains(r#"xmlns:a="urn:a""#), "{out}");
-        assert!(!out.contains("uuuu"), "the oversized declaration is not copied");
-    }
-}

crates/pptx/src/read/dml.rs

@@ -627,12 +627,22 @@ fn push_text_runs(para: &mut Paragraph, props: RunProps, text: &str) {
 /// Appends an Office Math (a14:m) run: its plain text and its OMML.
 fn push_math_run(para: &mut Paragraph, part: &Part, e: &El) {
     if para.runs.len() < MAX_PARAGRAPH_RUNS {
-        let mut text = String::new();
-        let mut ts = vec![];
-        e.find_all("t", &mut ts);
-        for t in ts {
-            text.push_str(&t.text());
+        // The text of the outermost `t` elements: a `t` nested in another is already part of
+        // the outer one's text, and taking it again would copy it once per nesting level.
+        fn outer_t(e: &El, depth: usize, text: &mut String) {
+            if depth > crate::xml::MAX_DEPTH {
+                return;
+            }
+            for c in e.elements() {
+                if c.is("t") {
+                    text.push_str(&c.text());
+                } else {
+                    outer_t(c, depth + 1, text);
+                }
+            }
         }
+        let mut text = String::new();
+        outer_t(e, 0, &mut text);
         let omml = e.elements().map(|c| part.keep_inner(c)).collect::<String>();
         para.runs.push(Run { text, props: RunProps::default(), kind: RunKind::Math { omml } });
     }
@@ -677,6 +687,17 @@ mod tests {
     use super::*;
     use crate::xml::parse;
 
+    #[test]
+    fn nested_math_text_is_taken_once() {
+        // 200 `m:t` nested in each other around one word: the word, not 200 copies of it.
+        let x = format!(r#"<a14:m xmlns:a14="a" xmlns:m="m"><m:oMath>{}word{}</m:oMath></a14:m>"#, "<m:t>".repeat(200), "</m:t>".repeat(200));
+        let d = parse(x.as_bytes()).unwrap();
+        let part = Part { name: "ppt/slides/slide1.xml".into(), rels: Default::default(), ns: vec![], inner_ns: vec![] };
+        let mut para = Paragraph::default();
+        push_math_run(&mut para, &part, &d.root);
+        assert_eq!(para.runs[0].text, "word");
+    }
+
     #[test]
     fn colors_and_transforms() {
         let d = parse(

crates/pptx/src/read/shapes.rs

@@ -475,7 +475,8 @@ fn frame(imp: &mut Imp, part: &Part, ctx: &mut IdCtx, e: &El, depth: usize) -> S
         .into_iter()
         .map(|id| {
             let rel = part.rels.get(&id).filter(|r| !r.external)?;
-            if !imp.pkg.rels(&rel.target).items.is_empty() {
+            let pkg = imp.pkg;
+            if *imp.has_rels.entry(crate::opc::norm(&rel.target)).or_insert_with(|| !pkg.rels(&rel.target).items.is_empty()) {
                 return None;
             }
             let (kind, target) = (rel.ty.clone(), rel.target.clone());