From 8cf3b70d8f98c47391aca2348340d587d1f67a0b Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 23:11:54 +0000 Subject: [PATCH] address review: count shared tiles once, include text advances, check clip region before keeping an item --- crates/metafile/src/emfplus.rs | 47 +++++++++++++++++++++++++++------- crates/metafile/src/place.rs | 7 +++++ crates/metafile/src/tests.rs | 18 +++++++++++++ 3 files changed, 63 insertions(+), 9 deletions(-) diff --git a/crates/metafile/src/emfplus.rs b/crates/metafile/src/emfplus.rs index 6125799e7..da27d346c 100644 --- a/crates/metafile/src/emfplus.rs +++ b/crates/metafile/src/emfplus.rs @@ -616,7 +616,7 @@ impl Plus { match obj { Obj::Image(img) => match img.as_ref() { ImageData::Bitmap(rgba) => rgba.px.len(), - ImageData::Meta(pic) => pic.items.iter().map(item_cost).sum(), + ImageData::Meta(pic) => picture_cost(&pic.items), }, Obj::Brush(Fill::Texture { img, .. }) => img.px.len(), Obj::Path(ops) => ops.len().saturating_mul(std::mem::size_of::()), @@ -1277,16 +1277,31 @@ fn metafile(r: Bytes) -> Option { pic.filter(|p| p.width > 0.0 && p.height > 0.0) } -/// Bytes an item of a nested picture owns: its path segments, clip rectangles, pixels and text. -fn item_cost(item: &crate::Item) -> usize { +/// Bytes the items of a nested picture own: path segments, clip rectangles, pixels and text, with +/// each shared pattern tile counted once. +fn picture_cost(items: &[crate::Item]) -> usize { use crate::Item; - let region = |r: &Option>| r.as_ref().map_or(0, |v| v.len() * 16); - 128 + match item { - Item::Path { segs, region: r, .. } => segs.len() * std::mem::size_of::() + region(r), - Item::Bitmap { rgba, region: r, .. } => rgba.len() + region(r), - Item::Pattern { segs, tile, region: r, .. } => segs.len() * std::mem::size_of::() + tile.rgba.len() + region(r), - Item::Text(t) => t.text.len() + region(&t.region), + let region = |r: &Option>| r.as_ref().map_or(0, |v| v.len().saturating_mul(16)); + let mut tiles = std::collections::HashSet::new(); + let mut total = 0usize; + for item in items { + let own = match item { + Item::Path { segs, region: r, .. } => segs.len().saturating_mul(std::mem::size_of::()).saturating_add(region(r)), + Item::Bitmap { rgba, region: r, .. } => rgba.len().saturating_add(region(r)), + Item::Pattern { segs, tile, region: r, .. } => { + let tile_bytes = if tiles.insert(Arc::as_ptr(tile)) { tile.rgba.len() } else { 0 }; + segs.len().saturating_mul(std::mem::size_of::()).saturating_add(tile_bytes).saturating_add(region(r)) + } + Item::Text(t) => t + .text + .len() + .saturating_add(t.dx.len().saturating_mul(std::mem::size_of::())) + .saturating_add(t.font.family.len()) + .saturating_add(region(&t.region)), + }; + total = total.saturating_add(128).saturating_add(own); } + total } fn bitmap(r: Bytes) -> Option { @@ -1566,3 +1581,17 @@ fn wrap_line(line: &[u16], width: f64, em: f64) -> Vec> { out.push(cur); out } + +#[cfg(test)] +mod cost_tests { + use super::*; + use crate::{Item, Seg, Tile}; + + #[test] + fn a_shared_pattern_tile_is_charged_once() { + let tile = Arc::new(Tile { width: 512, height: 512, rgba: vec![0; 1 << 20], cell: [1.0, 1.0], origin: [0.0, 0.0] }); + let item = || Item::Pattern { segs: vec![Seg::Move(0.0, 0.0), Seg::Line(1.0, 1.0)], even_odd: false, tile: tile.clone(), region: None }; + let items: Vec = (0..257).map(|_| item()).collect(); + assert!(picture_cost(&items) < 2 << 20, "{}", picture_cost(&items)); + } +} diff --git a/crates/metafile/src/place.rs b/crates/metafile/src/place.rs index 13cac6766..78ffc4b0a 100644 --- a/crates/metafile/src/place.rs +++ b/crates/metafile/src/place.rs @@ -88,6 +88,10 @@ impl Picture { let path = tf * bez(segs); let Some(region) = get_region(region) else { continue }; if in_range(path.bounding_box()) { + // This item's own region is expanded by now: it counts before it is kept. + if total_elements.saturating_add(path.elements().len()).saturating_add(region_elements.get()) > MAX_PLACED_ELEMENTS { + break; + } total_elements = total_elements.saturating_add(path.elements().len()); let stroke = stroke.map(|(c, w)| (c, f64::from(w) * k)); out.push(PlacedItem::Path { path, fill: *fill, stroke, even_odd: *even_odd, region }); @@ -101,6 +105,9 @@ impl Picture { * Affine::scale_non_uniform(f64::from(t.cell[0]), f64::from(t.cell[1])); let det = tile.determinant(); if in_range(path.bounding_box()) && det.is_finite() && det.abs() > 1e-12 { + if total_elements.saturating_add(path.elements().len()).saturating_add(region_elements.get()) > MAX_PLACED_ELEMENTS { + break; + } total_elements = total_elements.saturating_add(path.elements().len()); out.push(PlacedItem::Pattern { index, path, even_odd: *even_odd, tile, region }); } diff --git a/crates/metafile/src/tests.rs b/crates/metafile/src/tests.rs index 4fdf0ed5f..1b750ffcf 100644 --- a/crates/metafile/src/tests.rs +++ b/crates/metafile/src/tests.rs @@ -897,6 +897,24 @@ fn place_fits_the_frame_scales_strokes_and_drops_far_paths() { assert_eq!(stroke.map(|s| s.1), Some(8.0)); } +#[test] +fn place_counts_the_items_own_clip_region_before_keeping_it() { + // A path of 499,999 elements nearly fills the 500,000 budget; a second one whose new clip + // region adds elements goes past it and is left out. + let mut segs = vec![Seg::Move(0.0, 0.0)]; + segs.extend((0..499_998).map(|i| Seg::Line(1.0 + (i % 50) as f32, 1.0))); + let big = Item::Path { segs, fill: None, stroke: None, even_odd: false, region: None }; + let small = Item::Path { + segs: vec![Seg::Move(0.0, 0.0), Seg::Line(1.0, 1.0)], + fill: None, + stroke: None, + even_odd: false, + region: Some(vec![[0.0, 0.0, 50.0, 25.0], [10.0, 10.0, 20.0, 20.0]]), + }; + let pic = crate::Picture { width: 100.0, height: 50.0, items: vec![big, small] }; + assert_eq!(pic.place(kurbo::Rect::new(0.0, 0.0, 100.0, 50.0), [0.0; 4]).len(), 1); +} + #[test] fn place_keeps_only_finite_bounded_bitmaps_with_area() { let bitmap = |rect: [f32; 4]| Item::Bitmap { rect, width: 1, height: 1, rgba: vec![0; 4], region: None };