pptx: read a notes or comments part for the first slide that names it

crates/media/src/audio.rs

@@ -81,7 +81,7 @@ pub fn decode(bytes: &Bytes) -> Result<Pcm> {
             Some(r) => r,
             None => contained(|| decode_symphonia(bytes, "mkv")),
         },
-        _ if !crate::mp4guard::probe_fits(bytes) => Err(MediaError::Corrupt("the MP4's sample tables do not fit the file".into())),
+        Container::Mp4 if !crate::mp4guard::tables_fit(bytes) => Err(MediaError::Corrupt("the MP4's sample tables do not fit the file".into())),
         c => contained(|| decode_symphonia(bytes, c.name())),
     }
 }

crates/media/src/mp4guard.rs

@@ -12,20 +12,6 @@ pub(crate) fn tables_fit(b: &[u8]) -> bool {
     boxes_fit(b, b.len() as u64, 0)
 }
 
-/// Bytes symphonia's probe scans for a format marker before giving up.
-const PROBE_SEARCH_LIMIT: usize = 1024 * 1024;
-
-/// Whether symphonia can be given `b`, whatever the sniffer made of it: its probe takes the first
-/// `ftyp` within its search window as the start of an MP4 (the size field 4 bytes before it), so
-/// the table check must start where that parser will, not at offset 0.
-pub(crate) fn probe_fits(b: &[u8]) -> bool {
-    let head = &b[..b.len().min(PROBE_SEARCH_LIMIT + 16)];
-    match head.windows(4).position(|w| w == b"ftyp") {
-        Some(p) => tables_fit(&b[p.saturating_sub(4)..]),
-        None => true,
-    }
-}
-
 fn be32(b: &[u8], at: usize) -> Option<u64> {
     b.get(at..at.checked_add(4)?).map(|s| u32::from_be_bytes([s[0], s[1], s[2], s[3]]) as u64)
 }
@@ -88,29 +74,3 @@ fn table_fits(kind: &[u8], body: &[u8], file_len: u64) -> bool {
     };
     be32(body, 4).is_some_and(|count| count <= body.len().saturating_sub(8) as u64 / entry)
 }
-
-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    /// An MP4 whose `stsc` count asks for far more entries than the file holds.
-    fn mp4_with_huge_stsc() -> Vec<u8> {
-        let stsc = [&24u32.to_be_bytes()[..], b"stsc", &[0, 0, 0, 0], &u32::MAX.to_be_bytes(), &[0; 8]].concat();
-        let stbl = [&(stsc.len() as u32 + 8).to_be_bytes()[..], b"stbl", &stsc].concat();
-        let moov = [&(stbl.len() as u32 + 8).to_be_bytes()[..], b"moov", &stbl].concat();
-        [&16u32.to_be_bytes()[..], b"ftyp", b"isom", &[0; 4], &moov].concat()
-    }
-
-    #[test]
-    fn a_prefixed_mp4_is_checked_where_symphonia_starts_it() {
-        let mp4 = mp4_with_huge_stsc();
-        assert!(!tables_fit(&mp4));
-        assert!(!probe_fits(&mp4));
-        let prefixed = [&b"junk before the file "[..], &mp4].concat();
-        assert_eq!(crate::Container::sniff(&prefixed), crate::Container::Unknown);
-        assert!(tables_fit(&prefixed), "offset 0 sees no MP4 boxes");
-        assert!(!probe_fits(&prefixed));
-        let far = [&vec![0u8; PROBE_SEARCH_LIMIT + 16][..], &mp4].concat();
-        assert!(probe_fits(&far), "past the probe window symphonia never reads it as an MP4");
-    }
-}

crates/media/src/probe.rs

@@ -71,7 +71,7 @@ pub fn probe_headers(bytes: &Bytes) -> Result<MediaInfo> {
     };
     if info.audio.is_none() && info.video.is_none() || info.audio.as_ref().is_some_and(|a| a.sample_rate == 0) {
         // Plain audio files (and anything our demuxers didn't describe): symphonia's probe.
-        if !crate::mp4guard::probe_fits(bytes) {
+        if c == Container::Mp4 && !crate::mp4guard::tables_fit(bytes) {
             return Err(MediaError::Corrupt("the MP4's sample tables do not fit the file".into()));
         }
         let (codec, rate, channels, frames) = std::panic::catch_unwind(|| crate::audio::probe_symphonia(bytes, c.name()))

crates/pptx/src/read.rs

@@ -663,42 +663,46 @@ fn read_slides(
     let pkg = imp.pkg;
     let fallback_layout = p.masters.first().and_then(|m| m.layouts.first()).map(|l| l.id).unwrap_or_default();
     let mut file_slide_ids: HashMap<u32, SlideId> = HashMap::new();
-    let mut notes_by_target: HashMap<String, TextBody> = HashMap::new();
+    // A notes or comments part belongs to one slide; one shared by many must not be read (and
+    // copied) once per slide that names it.
+    let mut owned_parts: HashSet<String> = HashSet::new();
     for (file_id, path, id) in slide_ids {
         let Some(doc) = docs.get(&opc::norm(path)) else { continue };
         let part = Part::new(pkg, path, doc);
         let layout = part.rels.first_of("slideLayout").and_then(|r| layout_by_part.get(&opc::norm(&r.target)).copied()).unwrap_or(fallback_layout);
         let mut slide = read_slide(imp, &part, doc, *id, layout);
-        if let Some(notes) = slide_notes(imp, &part, &mut notes_by_target) {
+        if let Some(notes) = slide_notes(imp, &part, &mut owned_parts) {
             slide.notes = notes;
         }
-        read_slide_comments(pkg, pres_part, &part, &mut slide);
+        read_slide_comments(pkg, pres_part, &part, &mut slide, &mut owned_parts);
         file_slide_ids.insert(*file_id, *id);
         p.slides.push(Arc::new(slide));
     }
     file_slide_ids
 }
 
-/// The slide's notes text, read once per notes part.
-fn slide_notes(imp: &mut Imp, part: &Part, cache: &mut HashMap<String, TextBody>) -> Option<TextBody> {
+/// The slide's notes text. A notes part is the notes of the first slide that names it.
+fn slide_notes(imp: &mut Imp, part: &Part, owned: &mut HashSet<String>) -> Option<TextBody> {
     let n = part.rels.first_of("notesSlide")?;
-    let key = opc::norm(&n.target);
-    if let Some(cached) = cache.get(&key) {
-        return Some(cached.clone());
+    if !owned.insert(opc::norm(&n.target)) {
+        log::warn!("pptx: notes part {} is named by more than one slide; kept on the first", n.target);
+        return None;
     }
     let pkg = imp.pkg;
     let nd = load(pkg, &n.target)?;
     let np = Part::new(pkg, &n.target, &nd);
-    let notes = read_notes(imp, &np, &nd.root);
-    cache.insert(key, notes.clone());
-    Some(notes)
+    Some(read_notes(imp, &np, &nd.root))
 }
 
 /// Comments. A modern part and the legacy part can describe the same threads (PowerPoint
 /// writes both): the modern one wins, its flattened legacy copy is skipped.
-fn read_slide_comments(pkg: &Package, pres_part: &Part, part: &Part, slide: &mut Slide) {
+/// Each comments part is read once, for the first slide that names it.
+fn read_slide_comments(pkg: &Package, pres_part: &Part, part: &Part, slide: &mut Slide, owned: &mut HashSet<String>) {
     let mut parts: Vec<(Doc, bool)> = vec![];
     for c in part.rels.all_of("comments") {
+        if !owned.insert(opc::norm(&c.target)) {
+            continue;
+        }
         if let Some(cd) = pkg.xml(&c.target) {
             // The content type names the modern part; a package that types it only through a
             // Default entry instead of an Override is still modern when its root is p188.

crates/pptx/tests/it/malformed.rs

@@ -287,3 +287,34 @@ fn unreadable_slide_is_reported() {
     assert_eq!(lost, vec!["ppt/slides/slide2.xml".to_string()]);
     assert_eq!(deckcraft_pptx::import(&broken).expect("import").slides.len(), 1);
 }
+
+/// Many slides naming one notes part: it is read for the first slide only, not copied onto every
+/// slide that names it.
+#[test]
+fn a_shared_notes_part_is_read_once() {
+    use std::sync::Arc;
+    let mut p = defaults::new_presentation(None);
+    let layout = p.slides[0].layout;
+    for _ in 0..3 {
+        let s = defaults::new_slide(&mut p, layout).expect("slide");
+        p.slides.push(Arc::new(s));
+    }
+    for (i, s) in p.slides.iter_mut().enumerate() {
+        Arc::make_mut(s).notes = deckcraft_model::text::TextBody::from_text(&format!("notes {i}"));
+    }
+    let mut es = entries(&deckcraft_pptx::export(&p).expect("export"));
+    for (n, d) in es.iter_mut() {
+        if n.starts_with("ppt/slides/_rels/") {
+            let x = String::from_utf8(d.clone()).expect("utf8");
+            let x = x
+                .replace("notesSlide2.xml", "notesSlide1.xml")
+                .replace("notesSlide3.xml", "notesSlide1.xml")
+                .replace("notesSlide4.xml", "notesSlide1.xml");
+            *d = x.into_bytes();
+        }
+    }
+    let q = deckcraft_pptx::import(&rezip(&es)).expect("import");
+    assert_eq!(q.slides.len(), 4);
+    assert_eq!(q.slides[0].notes.text(), "notes 0");
+    assert!(q.slides[1..].iter().all(|s| s.notes.text().is_empty()), "a shared notes part is not copied");
+}