address review: count all chart vectors and relationship strings; reject a chart that crosses the presentation limit

crates/pptx/src/read.rs

@@ -38,6 +38,8 @@ pub struct Imp<'a> {
     pub vml_previews: HashMap<String, HashMap<String, String>>,
     /// Aggregate byte count of retained chart sidecars across the presentation.
     pub kept_sidecar_bytes: usize,
+    /// Data slots of the charts decoded so far (see `chart::MAX_PRESENTATION_CHART_POINTS`).
+    pub chart_points: usize,
     /// Cached kept sidecars by normalized chart part name.
     pub kept_charts: HashMap<String, deckcraft_model::chart::ChartKept>,
     /// Add-in tags parts, parsed once each, by normalized part name.
@@ -442,6 +444,7 @@ pub fn import_theme(bytes: &[u8]) -> Result<(deckcraft_model::theme::Theme, Vec<
         table_style_ids: Vec::new(),
         vml_previews: HashMap::new(),
         kept_sidecar_bytes: 0,
+        chart_points: 0,
         kept_charts: HashMap::new(),
         tags_by_part: HashMap::new(),
         tag_bytes: 0,
@@ -483,6 +486,7 @@ pub fn import_checked(bytes: &[u8]) -> Result<(Presentation, Vec<String>), PptxE
         table_style_ids: Vec::new(),
         vml_previews: HashMap::new(),
         kept_sidecar_bytes: 0,
+        chart_points: 0,
         kept_charts: HashMap::new(),
         tags_by_part: HashMap::new(),
         tag_bytes: 0,

crates/pptx/src/read/chart.rs

@@ -39,6 +39,24 @@ pub(crate) fn rich_text(t: &El) -> Option<String> {
 /// Most category/value/size slots one chart may decode across all its series and caches, so
 /// sparse `ptCount`/`idx` declarations can't expand far beyond the XML they came from.
 pub(crate) const MAX_CHART_POINTS: usize = 1_000_000;
+/// Data slots (values and categories) the charts of one presentation may hold in all; a chart past
+/// it is read as a placeholder frame. Each frame that references a chart decodes its own copy.
+pub(crate) const MAX_PRESENTATION_CHART_POINTS: usize = 4_000_000;
+
+/// Slots a decoded chart holds, counting the chart itself as at least one.
+pub(crate) fn point_count(c: &Chart) -> usize {
+    let series = |s: &deckcraft_model::chart::Series| {
+        let bars = s.error_bars.as_ref().map_or(0, |e| e.plus.len().saturating_add(e.minus.len()));
+        s.values.len().saturating_add(s.x_values.len()).saturating_add(s.sizes.len()).saturating_add(bars)
+    };
+    let levels: usize = c.category_levels.iter().map(Vec::len).sum();
+    1usize.saturating_add(c.categories.len()).saturating_add(levels).saturating_add(c.series.iter().map(series).fold(0, usize::saturating_add))
+}
+
+/// Bytes of the strings a kept relationship owns.
+fn rel_bytes(r: &deckcraft_model::chart::ChartRel) -> usize {
+    r.id.len() + r.rel_type.len() + r.target.len() + r.part.len()
+}
 
 /// Most slots one cache may hold.
 const MAX_CACHE_POINTS: usize = 100_000;
@@ -466,6 +484,13 @@ pub(crate) fn capture_kept(imp: &mut super::Imp, chart_part: &str) -> deckcraft_
 
     let key = crate::opc::norm(chart_part);
     if let Some(cached) = imp.kept_charts.get(&key) {
+        // Every frame that shows this chart gets its own copy, so each copy is charged like the first.
+        let bytes: usize = cached.parts.iter().map(|p| p.bytes.len()).sum::<usize>() + cached.rels.iter().map(rel_bytes).sum::<usize>();
+        if imp.kept_sidecar_bytes.saturating_add(bytes) > MAX_KEPT_TOTAL_BYTES {
+            log::warn!("pptx: chart sidecar retained byte budget exceeded for {chart_part}");
+            return ChartKept::default();
+        }
+        imp.kept_sidecar_bytes += bytes;
         return cached.clone();
     }
 
@@ -485,6 +510,13 @@ pub(crate) fn capture_kept(imp: &mut super::Imp, chart_part: &str) -> deckcraft_
             kept.rels.push(ChartRel { id: r.id, rel_type: r.ty, target: r.target_raw, part: r.target, external: false });
         }
     }
+    // The first capture's relationship strings are retained too (the parts were charged as copied).
+    let rel_total: usize = kept.rels.iter().map(rel_bytes).sum();
+    if imp.kept_sidecar_bytes.saturating_add(rel_total) > MAX_KEPT_TOTAL_BYTES {
+        log::warn!("pptx: chart sidecar retained byte budget exceeded for {chart_part}");
+        return ChartKept::default();
+    }
+    imp.kept_sidecar_bytes += rel_total;
     imp.kept_charts.insert(key, kept.clone());
     kept
 }
@@ -590,6 +622,16 @@ fn text_size(e: Option<&El>) -> Option<f64> {
 mod tests {
     use super::*;
 
+    #[test]
+    fn point_count_charges_every_retained_vector() {
+        let mut c = Chart { categories: vec!["a".into()], category_levels: vec![vec!["x".into(); 2], vec!["y".into(); 3]], ..Default::default() };
+        let mut s =
+            deckcraft_model::chart::Series { values: vec![Some(1.0); 4], x_values: vec![None; 5], sizes: vec![None; 6], ..Default::default() };
+        s.error_bars = Some(deckcraft_model::chart::ErrorBars { plus: vec![None; 7], minus: vec![None; 8], ..Default::default() });
+        c.series = vec![s];
+        assert_eq!(point_count(&c), 1 + 1 + 5 + 4 + 5 + 6 + 15);
+    }
+
     #[test]
     fn sparse_caches_share_one_chart_wide_budget() {
         // Every series declares 100000 points but writes one: without a chart-wide budget each

crates/pptx/src/read/shapes.rs

@@ -428,10 +428,14 @@ fn frame(imp: &mut Imp, part: &Part, ctx: &mut IdCtx, e: &El, depth: usize) -> S
             return s;
         }
         if let Some(c) = gd.child("chart")
+            && imp.chart_points < super::chart::MAX_PRESENTATION_CHART_POINTS
             && let Some(rel) = part.rels.get(c.attr("r:id").unwrap_or(""))
             && let Some(bytes) = imp.pkg.xml_bytes(&rel.target)
             && let Some(mut chart) = if uri.ends_with("/chartex") { super::read_chartex_xml(bytes) } else { super::read_chart_xml(bytes) }
+            // A chart that would cross the presentation limit is read as the placeholder frame instead.
+            && imp.chart_points.saturating_add(super::chart::point_count(&chart)) <= super::chart::MAX_PRESENTATION_CHART_POINTS
         {
+            imp.chart_points = imp.chart_points.saturating_add(super::chart::point_count(&chart));
             chart.kept = super::chart::capture_kept(imp, &rel.target);
             s.kind = ShapeKind::Chart(Box::new(chart));
             return s;

crates/ui-egui/src/media.rs

@@ -12,9 +12,6 @@ use egui::{Align2, Color32, CornerRadius, Pos2, Rect, Sense, Stroke, TextureHand
 
 use crate::icons::{self, Icon};
 
-/// Video shapes that get a decoder at once; more play their sound without a picture.
-const MAX_VIDEO_FEEDS: usize = 16;
-
 /// Who started a voice: the editor's control bar stops when you leave the slide, the slide show
 /// manages its own.
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
@@ -230,21 +227,12 @@ impl MediaHost {
             audio: clip.volume > 0.0,
         };
         let source_changed = self.owners.get(&id).is_some_and(|o| o.source != media_key);
-        // A shape whose feed could not be had (the cap, a decode error) tries again on each play.
-        let feedless = self.videos.get(&id).is_some_and(|v| v.feed.is_none());
-        if clip.video && (source_changed || feedless || !self.videos.contains_key(&id)) {
-            // One decoder and frame queue per video shape: past the cap a shape plays its sound only.
-            let live = self.videos.iter().filter(|(vid, v)| **vid != id && v.feed.is_some()).count();
-            let feed = if live >= MAX_VIDEO_FEEDS {
-                log::info!("video {id}: more than {MAX_VIDEO_FEEDS} videos playing; showing no picture");
-                None
-            } else {
-                match VideoFeed::new(bytes.clone()) {
-                    Ok(f) => Some(f),
-                    Err(e) => {
-                        log::info!("video {id}: {e}");
-                        None
-                    }
+        if clip.video && (source_changed || !self.videos.contains_key(&id)) {
+            let feed = match VideoFeed::new(bytes.clone()) {
+                Ok(f) => Some(f),
+                Err(e) => {
+                    log::info!("video {id}: {e}");
+                    None
                 }
             };
             self.videos.insert(id, Video { feed, frame: None, tex: None });

crates/ui-egui/src/show/render.rs

@@ -196,40 +196,18 @@ pub(super) fn part_textures(ctx: &egui::Context, doc: &Presentation, from: Optio
     ]
 }
 
-/// Width of a slide-show grid thumbnail, in pixels.
-const THUMB_WIDTH: f64 = 320.0;
-/// Tallest grid thumbnail, so an extreme portrait slide cannot ask for a huge raster.
-const THUMB_MAX_HEIGHT: f64 = 2048.0;
-
-/// The scale and pixel size of a grid thumbnail of a `width` x `height` slide.
-fn thumb_size(width: f64, height: f64) -> (f64, f64, f64) {
-    let (sw, sh) = (width.max(1.0), height.max(1.0));
-    let scale = (THUMB_WIDTH / sw).min(THUMB_MAX_HEIGHT / sh);
-    (scale, (sw * scale).round().max(1.0), (sh * scale).round().max(1.0))
-}
-
 pub(super) fn render_thumb(ctx: &egui::Context, show: &mut Show, doc: &Presentation, i: usize) -> Option<TextureHandle> {
     if show.thumbs.len() >= GRID_CAP {
         return None;
     }
-    // 320 wide, unless the slide is so tall that this would pass the height limit: fit that box.
-    let (scale, w, h) = thumb_size(doc.slide_size.width, doc.slide_size.height);
-    let img = deckcraft_render::render_slide(doc, i, &RenderOpts { scale, size: Some((w as u32, h as u32)), ..Default::default() });
+    let w = 320.0_f64;
+    let h = (w * doc.slide_size.height / doc.slide_size.width.max(1.0)).round().max(1.0);
+    let img = deckcraft_render::render_slide(
+        doc,
+        i,
+        &RenderOpts { scale: w / doc.slide_size.width.max(1.0), size: Some((w as u32, h as u32)), ..Default::default() },
+    );
     let tex = crate::textures::upload(ctx, format!("show-grid-{i}"), crate::textures::to_color_image(&img, false));
     show.thumbs.push((i, tex.clone()));
     Some(tex)
 }
-
-#[cfg(test)]
-mod thumb_size_tests {
-    use super::*;
-
-    #[test]
-    fn ordinary_slides_are_320_wide_and_extreme_ones_stay_in_the_height_limit() {
-        let (scale, w, h) = thumb_size(960.0, 540.0);
-        assert_eq!((w, h), (320.0, 180.0));
-        assert!((scale - 320.0 / 960.0).abs() < 1e-12);
-        let (_, w, h) = thumb_size(1.0, 1.0e9);
-        assert!(h <= THUMB_MAX_HEIGHT && w >= 1.0);
-    }
-}