From e69b433156f6263819ba488e0767c28d35604185 Mon Sep 17 00:00:00 2001 From: mtlcl410 Date: Sat, 7 Nov 2026 11:56:24 +0800 Subject: [PATCH 1/4] M5: comments without an appearance read style values given by reference, and a callout without /RD is drawn (view.render-annotation-appearances) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Any value may be an indirect object (ISO 32000-2 §7.3.10). The appearance builder read only direct values, so a border style dictionary given as `/BS 5 0 R` (Table 168) was ignored and the border drawn 1 pt solid. The display copy, a redraw, the properties and a new width now read the annotation with its style references followed (bounded in depth and in copied values; links such as /P and /Popup and streams stay references). A new width keeps an indirect /BS's other entries instead of dropping them. /RD is optional (Table 177): a FreeText callout without it now has its text box at /Rect and is drawn, where it was not drawn at all. PDFBOX-4515 (annotations-2.0-without-AP). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pq7HyDBXygTf7jcqdsjcfn --- crates/annot/src/appearance.rs | 7 ++- crates/annot/src/draw.rs | 85 +++++++++++++++++++++++++++-- crates/annot/src/info.rs | 3 ++ crates/annot/src/modify.rs | 10 ++-- crates/annot/src/tests.rs | 98 ++++++++++++++++++++++++++++++++++ parity/acrobat-features.toml | 4 +- 6 files changed, 198 insertions(+), 9 deletions(-) diff --git a/crates/annot/src/appearance.rs b/crates/annot/src/appearance.rs index 7148e9fc2e..8f2a5ca4ac 100644 --- a/crates/annot/src/appearance.rs +++ b/crates/annot/src/appearance.rs @@ -5,6 +5,7 @@ //! `/BBox = /Rect` (identity matrix), except note icons, which are drawn in a 20 × 20 box. //! It returns `None` for anything it cannot draw faithfully (cloudy borders, unknown line //! endings, indirect geometry), so callers never replace an appearance with a worse one. +//! Callers that hold the document follow references first (`draw::with_direct_values`). //! //! The note icons are PdfCraft's own drawings (AGENTS.md §1). Text boxes use the standard //! Helvetica font with WinAnsi encoding; line breaking uses [`text_width`], an approximation of @@ -1019,7 +1020,11 @@ fn draw_free_text(context: &AppearanceContext<'_>, mut drawing: AppearanceDrawin let mut rect = rect; if d.contains(b"CL") { let cl = nums(d, b"CL").filter(|l| l.len() == 4 || l.len() == 6)?; - let rd = nums(d, b"RD").filter(|r| r.len() == 4 && r.iter().all(|x| *x >= 0.0))?; + // `/RD` is optional (Table 177; null is absent, §7.3.9): without it the text box is `/Rect`. + let rd = match d.get(b"RD") { + None | Some(Object::Null) => vec![0.0; 4], + Some(_) => nums(d, b"RD").filter(|r| r.len() == 4 && r.iter().all(|x| *x >= 0.0))?, + }; rect = [full[0] + rd[0], full[1] + rd[1], full[2] - rd[2], full[3] - rd[3]]; if rect[2] - rect[0] < 1.0 || rect[3] - rect[1] < 1.0 { return None; diff --git a/crates/annot/src/draw.rs b/crates/annot/src/draw.rs index 5e698332da..27f38f43f6 100644 --- a/crates/annot/src/draw.rs +++ b/crates/annot/src/draw.rs @@ -40,7 +40,7 @@ pub(crate) fn draw_appearance(doc: &mut Document, r: ObjRef) -> Result<(), Annot /// Draw the appearance for an annotation dictionary: the expensive half of [`set_appearance`]. /// Pure from the dictionary and the objects it points at, so [`AppearanceCache`] can reuse it. fn build_appearance(doc: &Document, d: &Dict) -> Option { - let mut stream = appearance::build(d)?; + let mut stream = appearance::build(&with_direct_values(doc, d))?; // Picture-stamp restyling must retain the placement's page-axis correction. if d.get(b"PCPicture").is_some() { let matrix = d.get(b"AP").and_then(|ap| { @@ -56,10 +56,89 @@ fn build_appearance(doc: &Document, d: &Dict) -> Option { Some(stream) } +/// Entries that link the annotation to other objects rather than describe how it looks: left as +/// references by [`with_direct_values`]. +const LINK_KEYS: [&[u8]; 5] = [b"P", b"Popup", b"IRT", b"Parent", b"AP"]; + +/// How deep inside an entry [`with_direct_values`] follows references: `/InkList` is an array of +/// arrays, `/Border` may end in a dash array. +const INLINE_DEPTH: usize = 4; + +/// Values [`with_direct_values`] copies out of referenced arrays and dictionaries for one +/// annotation, so a file can't have a large object copied for every entry that points at it. +const INLINE_BUDGET: usize = 1 << 16; + +/// `d` with the references among the values [`appearance::build`] reads followed. Any value may be +/// given by reference (ISO 32000-2 §7.3.10): a border style dictionary (`/BS 5 0 R`, Table 168), a +/// colour, `/RD`, `/CL`, a `/W` inside `/BS`. The builder reads only direct values, so it draws +/// from this copy. The entries in [`LINK_KEYS`] and streams stay references; past +/// [`INLINE_DEPTH`] levels or [`INLINE_BUDGET`] copied values a reference is left as it is, which +/// the builder treats as unreadable. `d` itself when nothing is a reference. +pub(crate) fn with_direct_values<'d>(doc: &Document, d: &'d Dict) -> std::borrow::Cow<'d, Dict> { + let mut budget = INLINE_BUDGET; + let mut out: Option = None; + for (key, value) in d.iter() { + if LINK_KEYS.contains(&key.as_slice()) { + continue; + } + if let Some(direct) = direct_value(doc, value, INLINE_DEPTH, &mut budget) { + out.get_or_insert_with(|| d.clone()).set(key.clone(), direct); + } + } + out.map_or(std::borrow::Cow::Borrowed(d), std::borrow::Cow::Owned) +} + +/// `o` with references followed `depth` levels into arrays and dictionaries, or `None` when there +/// is nothing to follow (or it may not be: a stream, an over-budget object, a chain of references). +fn direct_value(doc: &Document, o: &Object, depth: usize, budget: &mut usize) -> Option { + let held; + let (base, followed) = match o { + Object::Ref(_) => { + held = doc.resolve(o); + let cost = match &*held { + Object::Stream(_) | Object::Ref(_) => return None, + Object::Array(a) => a.len().max(1), + Object::Dict(d) => d.len().max(1), + _ => 1, + }; + *budget = budget.checked_sub(cost)?; + (&*held, true) + } + other => (other, false), + }; + let Some(next) = depth.checked_sub(1) else { return followed.then(|| base.clone()) }; + match base { + Object::Array(items) => { + let mut changed: Option> = None; + for (i, item) in items.iter().enumerate() { + if let Some(direct) = direct_value(doc, item, next, budget) { + let copy = changed.get_or_insert_with(|| items.clone()); + if let Some(slot) = copy.get_mut(i) { + *slot = direct; + } + } + } + changed.map(Object::Array).or_else(|| followed.then(|| base.clone())) + } + Object::Dict(entries) => { + let mut changed: Option = None; + for (key, value) in entries.iter() { + if let Some(direct) = direct_value(doc, value, next, budget) { + changed.get_or_insert_with(|| entries.clone()).set(key.clone(), direct); + } + } + changed.map(Object::Dict).or_else(|| followed.then(|| base.clone())) + } + _ => followed.then(|| base.clone()), + } +} + /// Store a built appearance on the annotation: the cheap half of [`set_appearance`]. fn apply_appearance(doc: &mut Document, r: ObjRef, d: &Dict, stream: Stream) -> Result<(), AnnotError> { // Characters the standard fonts can't draw came out as `?`: mark the annotation with the first. - let missing = appearance::undrawable_text(d).map(|(_, c)| c); + // Read like the builder read it, references followed. + let direct = with_direct_values(doc, d); + let missing = appearance::undrawable_text(&direct).map(|(_, c)| c); let ap = doc.add(Object::Stream(stream)); // A copy (a shared /AP is left alone) that keeps unknown entries; the old down and rollover // appearances would show the previous look on press or hover, so they go with the old /N. @@ -71,7 +150,7 @@ fn apply_appearance(doc: &mut Document, r: ObjRef, d: &Dict, stream: Stream) -> // The appearance's box is mapped onto `/Rect`, so any other `/Rect` (a large one, or one with // no width, as LaTeX `pdfcomment` writes) would stretch the icon or shrink it to nothing. let icon_rect = matches!(d.name(b"Subtype"), Some(b"Text" | b"FileAttachment")) - .then(|| d.get(b"Rect")?.as_array()?.iter().map(|o| o.as_f64()).collect::>>()) + .then(|| direct.get(b"Rect")?.as_array()?.iter().map(|o| o.as_f64()).collect::>>()) .flatten() .filter(|q| q.len() == 4 && finite(q)) .map(|q| { diff --git a/crates/annot/src/info.rs b/crates/annot/src/info.rs index ba181d7504..e451e3009a 100644 --- a/crates/annot/src/info.rs +++ b/crates/annot/src/info.rs @@ -238,6 +238,9 @@ pub fn props(doc: &Document, page: usize, index: usize) -> Option { let c: Vec = d.get(b"C").and_then(|o| o.as_array().map(|a| a.iter().filter_map(|x| x.as_f64()).collect())).unwrap_or_default(); let color = if subtype == "FreeText" { Some(appearance::parse_da(d).0) } else { (c.len() == 3).then(|| [c[0], c[1], c[2]]) }; let width = matches!(subtype.as_str(), "Square" | "Circle" | "Line" | "Ink" | "Polygon" | "PolyLine" | "FreeText").then(|| { + // A border style given by reference (`/BS 5 0 R`) reads as the display draws it. + let direct = crate::draw::with_direct_values(doc, d); + let d = direct.as_ref(); if subtype == "FreeText" { if !d.contains(b"BS") && !d.contains(b"Border") { 0.0 } else { appearance::border_width(d) } } else { diff --git a/crates/annot/src/modify.rs b/crates/annot/src/modify.rs index c7d87be090..54636d1841 100644 --- a/crates/annot/src/modify.rs +++ b/crates/annot/src/modify.rs @@ -730,8 +730,12 @@ pub fn set_style(doc: &mut Document, page: usize, index: usize, change: StyleCha // So does a text box inside an oval or a circle, which goes around it again. let shape = if free_text { BorderShape::of(&d) } else { BorderShape::Rectangle }; let typewriter = free_text && (d.name(b"IT") == Some(b"FreeTextTypeWriter") || shape != BorderShape::Rectangle) && !d.contains(b"CL"); + // A border style given by reference (`/BS 5 0 R`, ISO 32000-2 §7.3.10): its width and other + // entries, which a new width keeps. + let direct = crate::draw::with_direct_values(doc, &d); + let (old_width, old_bs) = (border_width_of(&direct), direct.get(b"BS").and_then(Object::as_dict).cloned()); let regrow = width.filter(|_| typewriter).and_then(|w| { - let by = w.max(0.0) - if d.contains(b"BS") || d.contains(b"Border") { border_width_of(&d) } else { 0.0 }; + let by = w.max(0.0) - if d.contains(b"BS") || d.contains(b"Border") { old_width } else { 0.0 }; let rect = grow(text_box_of(&d)?, by); (by != 0.0 && finite(&rect) && rect[2] - rect[0] >= 1.0 && rect[3] - rect[1] >= 1.0).then_some((rect, w.max(0.0))) }); @@ -755,9 +759,9 @@ pub fn set_style(doc: &mut Document, page: usize, index: usize, change: StyleCha set_part_keys(d, appearance::PartOpacity { line: o, fill: o, text: o }); } if let Some(w) = width { - let old_w = border_width_of(d); + let old_w = old_width; let w = w.max(0.0); - let mut bs = d.get(b"BS").and_then(|b| b.as_dict()).cloned().unwrap_or_default(); + let mut bs = old_bs.unwrap_or_default(); bs.set(b"W".to_vec(), Object::Real(w)); d.set(b"BS".to_vec(), Object::Dict(bs)); d.remove(b"Border"); diff --git a/crates/annot/src/tests.rs b/crates/annot/src/tests.rs index ff7f8925f0..6acb8ce742 100644 --- a/crates/annot/src/tests.rs +++ b/crates/annot/src/tests.rs @@ -1461,6 +1461,104 @@ fn malformed_optional_interior_color_does_not_hide_shape_strokes() { assert!(list(&doc, 0)[0].get(b"AP").is_none(), "display generation leaves the source document unchanged"); } +/// Any value may be given by reference (ISO 32000-2 §7.3.10). A Square whose border style +/// dictionary (Table 168), colour and dash array are indirect objects is drawn with them, not +/// with the 1 pt solid default; a reference cycle inside `/BS` is not followed for ever. +#[test] +fn indirect_border_style_and_colour_are_drawn() { + let doc = document_of(&[ + b"<< /Type /Catalog /Pages 2 0 R >>", + b"<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 200 200] >>", + b"<< /Type /Page /Parent 2 0 R /Annots [4 0 R] >>", + b"<< /Type /Annot /Subtype /Square /P 3 0 R /Rect [20 20 180 180] /BS 5 0 R /C 6 0 R >>", + b"<< /Type /Border /W 7 0 R /S /D /D 8 0 R /Self 5 0 R >>", + b"[0 0 1]", + b"6", + b"[4 2]", + ]); + let display = with_missing_appearances(&doc).expect("the square gets a display appearance"); + let shown = list(&display, 0).remove(0); + let content = ap_content(&display, &shown); + assert!(content.contains("0 0 1 RG\n6 w\n[4 2] 0 d\n"), "the indirect /BS, /W, /D and /C are used: {content}"); + assert!(content.contains("23 23 154 154 re"), "the 6 pt border is inset by half its width: {content}"); + assert_eq!(shown.get(b"BS"), Some(&Object::Ref(ObjRef::new(5, 0))), "the display copy keeps the reference"); + assert_eq!(shown.get(b"P"), Some(&Object::Ref(ObjRef::new(3, 0)))); + let props = props(&doc, 0, 0).expect("props"); + assert_eq!(props.width, Some(6.0), "the properties show the width drawn"); +} + +/// Following references copies a bounded number of values: an indirect array past the budget +/// stays a reference (and the comment is not drawn from it), a chain that never ends is not +/// followed, and links to other objects are never copied. +#[test] +fn direct_values_are_bounded() { + let mut doc = fixture(); + let huge = doc.add(Object::Array(vec![Object::Int(0); (1 << 16) + 1])); + let chain = doc.add(Object::Null); + doc.set(chain, Object::Ref(chain)); + let mut d = Dict::new(); + d.set(b"Subtype".to_vec(), Object::name("Square")); + d.set(b"Rect".to_vec(), Object::Ref(doc.add(num_array(&[0.0, 0.0, 10.0, 10.0])))); + d.set(b"Vertices".to_vec(), Object::Ref(huge)); + d.set(b"IC".to_vec(), Object::Ref(chain)); + d.set(b"Popup".to_vec(), Object::Ref(huge)); + let direct = crate::draw::with_direct_values(&doc, &d); + assert_eq!(direct.get(b"Rect").and_then(Object::as_array).map(Vec::len), Some(4), "a small array is followed"); + assert_eq!(direct.get(b"Vertices"), Some(&Object::Ref(huge)), "over the budget: left as a reference"); + assert_eq!(direct.get(b"IC"), Some(&Object::Ref(chain)), "a chain of references is left alone"); + assert_eq!(direct.get(b"Popup"), Some(&Object::Ref(huge)), "a link is not copied"); + let mut plain = Dict::new(); + plain.set(b"W".to_vec(), Object::Int(2)); + assert!(matches!(crate::draw::with_direct_values(&doc, &plain), std::borrow::Cow::Borrowed(_)), "nothing to follow: no copy"); +} + +/// A new width for a comment whose border style is an indirect object (§7.3.10) keeps that +/// dictionary's other entries (a dashed style stays dashed) and leaves the shared object alone. +#[test] +fn new_width_keeps_an_indirect_border_style() { + let mut doc = document_of(&[ + b"<< /Type /Catalog /Pages 2 0 R >>", + b"<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 200 200] >>", + b"<< /Type /Page /Parent 2 0 R /Annots [4 0 R] >>", + b"<< /Type /Annot /Subtype /Square /Rect [20 20 180 180] /BS 5 0 R /C [1 0 0] >>", + b"<< /W 6 /S /D /D [4 2] >>", + ]); + set_style(&mut doc, 0, 0, StyleChange::new(None, None, Some(3.0), None), &meta("")).unwrap(); + let doc = reopen(&doc); + let d = list(&doc, 0).remove(0); + let bs = d.get(b"BS").and_then(Object::as_dict).expect("a direct /BS"); + assert_eq!(bs.get(b"W").and_then(Object::as_f64), Some(3.0)); + assert_eq!(bs.name(b"S"), Some(b"D".as_slice()), "the dashed style is kept"); + assert!(ap_content(&doc, &d).contains("3 w\n[4 2] 0 d\n")); + let shared = doc.get(ObjRef::new(5, 0)); + assert_eq!(shared.as_dict().and_then(|b| b.get(b"W")).and_then(Object::as_f64), Some(6.0), "the shared object is unchanged"); +} + +/// `/RD` is optional (ISO 32000-2 Table 177): a callout without it has its text box at `/Rect` +/// and is drawn, box, text and leader line. A callout line and `/RD` given by reference are read too. +#[test] +fn callout_without_rd_is_drawn() { + let doc = document_of(&[ + b"<< /Type /Catalog /Pages 2 0 R >>", + b"<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 400 400] >>", + b"<< /Type /Page /Parent 2 0 R /Annots [4 0 R 5 0 R 6 0 R] >>", + b"<< /Type /Annot /Subtype /FreeText /IT /FreeTextCallout /Rect [100 100 300 160] /CL [20 20 60 60 100 130] /LE /OpenArrow /C [1 1 0] /DA (0 g /Helv 12 Tf) /Contents (callout) >>", + b"<< /Type /Annot /Subtype /FreeText /IT /FreeTextCallout /Rect [10 200 300 300] /CL 7 0 R /RD 8 0 R /C [1 1 0] /DA (0 g /Helv 12 Tf) /Contents (indirect) >>", + b"<< /Type /Annot /Subtype /FreeText /IT /FreeTextCallout /Rect [10 300 300 390] /CL [5 305 10 350] /RD [1 2] /C [1 1 0] /DA (0 g /Helv 12 Tf) /Contents (bad) >>", + b"[15 205 40 250 100 250]", + b"[90 0 0 0]", + ]); + let display = with_missing_appearances(&doc).expect("the callouts get a display appearance"); + let shown = list(&display, 0); + let plain = ap_content(&display, &shown[0]); + assert!(plain.contains("1 1 0 rg\n100 100 200 60 re f"), "the yellow box fills /Rect: {plain}"); + assert!(plain.contains("20 20 m\n60 60 l\n100 130 l\nS"), "the leader line: {plain}"); + assert!(plain.contains("(callout) Tj"), "the text: {plain}"); + let indirect = ap_content(&display, &shown[1]); + assert!(indirect.contains("100 200 200 100 re f") && indirect.contains("15 205 m"), "indirect /CL and /RD: {indirect}"); + assert!(shown[2].get(b"AP").is_none(), "a malformed /RD is still not guessed at"); +} + /// #3175: viewers draw a note without an appearance as a fixed-size icon at the upper-left corner /// of its `/Rect`. The display copy must not stretch the icon over a large `/Rect` or shrink it /// to nothing in one with no width. diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index 10b040e09f..b60fbe4179 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -818,8 +818,8 @@ milestone = "M2" status = "shipped" commands = [] tools = [] -evidence = ["crates/render/src/raster/tests.rs::widget_appearance_states", "crates/render/src/raster/tests.rs::highlights_without_an_appearance_are_drawn", "crates/engine/src/tests.rs::comments_without_appearances_are_drawn_but_not_saved", "crates/engine/src/tests.rs::cli_display_adds_missing_appearances_without_changing_the_source", "crates/engine/src/tests.rs::fields_left_to_the_viewer_are_displayed_from_their_values", "crates/forms/src/tests.rs::need_appearances_draws_every_text_and_choice_field_in_the_display_copy", "crates/forms/src/tests.rs::the_field_appearance_cache_sees_every_input", "crates/engine/src/tests.rs::buttons_and_combs_left_to_the_viewer_are_displayed", "crates/forms/src/tests.rs::check_boxes_without_an_appearance_show_their_caption_symbol", "crates/forms/src/tests.rs::push_buttons_without_an_appearance_show_their_caption_and_frame", "crates/forms/src/tests.rs::need_appearances_draws_check_boxes_and_radio_buttons_from_their_captions", "crates/engine/src/tests.rs::need_appearances_buttons_are_displayed_from_their_captions", "crates/render/src/raster/tests.rs::invisible_unknown_annotations_and_popups_paint_nothing", "crates/print/src/tests.rs::invisible_annotations_of_unknown_types_do_not_print", "crates/engine/src/tests.rs::stamps_without_appearances_draw_the_stamp_they_name", "crates/automation/tests/it/automation.rs::page_render_draws_stamps_without_an_appearance"] -notes = "comments without /AP (FreeText, Ink, notes, stamps, shapes, markup) are drawn from their dictionaries in the page view, page_render and page images, including the CLI render command, never saved; a stamp from another program draws the stamp its /Name names (the fourteen standard names of ISO 32000-2 Table 184, Draft when there is none) or, for any other name, a grey frame with both diagonals; text and choice fields are drawn from their values when the form sets /NeedAppearances (every one) or a widget has no /N (#3470), also display-only and cached per widget (a field edit on a 1000-field /NeedAppearances form takes ~13 ms against ~7 ms on a form that needs none) (a read-only document still gets the renderer's Highlight fallback only); check boxes and radio buttons with no appearance for their /AS are drawn display-only from /MK (background, border, and when on the /MK /CA ZapfDingbats symbol in the /DA colour, a check or a dot by default), and when the form sets /NeedAppearances so is every one that has an /MK, over the appearance the file carries (ISO 32000-2 Table 224; one without /MK keeps its own), and push buttons with none from /MK and /BS with their caption; Link borders without /AP and printing are not covered yet; an annotation of a non-standard type with the Invisible flag (ISO 32000-2 Table 167) is neither drawn nor printed, and a popup's own /AP is never painted on the page (its parent's text shows in the popup window, §12.5.6.14)" +evidence = ["crates/render/src/raster/tests.rs::widget_appearance_states", "crates/render/src/raster/tests.rs::highlights_without_an_appearance_are_drawn", "crates/engine/src/tests.rs::comments_without_appearances_are_drawn_but_not_saved", "crates/engine/src/tests.rs::cli_display_adds_missing_appearances_without_changing_the_source", "crates/engine/src/tests.rs::fields_left_to_the_viewer_are_displayed_from_their_values", "crates/forms/src/tests.rs::need_appearances_draws_every_text_and_choice_field_in_the_display_copy", "crates/forms/src/tests.rs::the_field_appearance_cache_sees_every_input", "crates/engine/src/tests.rs::buttons_and_combs_left_to_the_viewer_are_displayed", "crates/forms/src/tests.rs::check_boxes_without_an_appearance_show_their_caption_symbol", "crates/forms/src/tests.rs::push_buttons_without_an_appearance_show_their_caption_and_frame", "crates/forms/src/tests.rs::need_appearances_draws_check_boxes_and_radio_buttons_from_their_captions", "crates/engine/src/tests.rs::need_appearances_buttons_are_displayed_from_their_captions", "crates/render/src/raster/tests.rs::invisible_unknown_annotations_and_popups_paint_nothing", "crates/print/src/tests.rs::invisible_annotations_of_unknown_types_do_not_print", "crates/engine/src/tests.rs::stamps_without_appearances_draw_the_stamp_they_name", "crates/automation/tests/it/automation.rs::page_render_draws_stamps_without_an_appearance", "crates/annot/src/tests.rs::indirect_border_style_and_colour_are_drawn", "crates/annot/src/tests.rs::callout_without_rd_is_drawn"] +notes = "comments without /AP (FreeText, Ink, notes, stamps, shapes, markup) are drawn from their dictionaries in the page view, page_render and page images, including the CLI render command, never saved, with style values given by reference (a /BS, colour, /RD or /CL as an indirect object, ISO 32000-2 §7.3.10) followed and a callout without /RD (optional, Table 177) boxed at its /Rect; a stamp from another program draws the stamp its /Name names (the fourteen standard names of ISO 32000-2 Table 184, Draft when there is none) or, for any other name, a grey frame with both diagonals; text and choice fields are drawn from their values when the form sets /NeedAppearances (every one) or a widget has no /N (#3470), also display-only and cached per widget (a field edit on a 1000-field /NeedAppearances form takes ~13 ms against ~7 ms on a form that needs none) (a read-only document still gets the renderer's Highlight fallback only); check boxes and radio buttons with no appearance for their /AS are drawn display-only from /MK (background, border, and when on the /MK /CA ZapfDingbats symbol in the /DA colour, a check or a dot by default), and when the form sets /NeedAppearances so is every one that has an /MK, over the appearance the file carries (ISO 32000-2 Table 224; one without /MK keeps its own), and push buttons with none from /MK and /BS with their caption; Link borders without /AP and printing are not covered yet; an annotation of a non-standard type with the Invisible flag (ISO 32000-2 Table 167) is neither drawn nor printed, and a popup's own /AP is never painted on the page (its parent's text shows in the popup window, §12.5.6.14)" [[feature]] id = "view.render-error-isolation" From 4bbd3a2626cf7226166058248db5899877d911c4 Mon Sep 17 00:00:00 2001 From: mtlcl410 Date: Sat, 7 Nov 2026 12:01:00 +0800 Subject: [PATCH 2/4] M5: an indirect value the builder can't draw keeps the plain drawing (view.render-annotation-appearances) Following references can turn up a value the appearance builder leaves alone, such as a cloudy /BE on a square. The comment then keeps the drawing made without the references followed, instead of none. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pq7HyDBXygTf7jcqdsjcfn --- crates/annot/src/draw.rs | 10 +++++++++- crates/annot/src/tests.rs | 16 ++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/crates/annot/src/draw.rs b/crates/annot/src/draw.rs index 27f38f43f6..e36bf3a534 100644 --- a/crates/annot/src/draw.rs +++ b/crates/annot/src/draw.rs @@ -40,7 +40,15 @@ pub(crate) fn draw_appearance(doc: &mut Document, r: ObjRef) -> Result<(), Annot /// Draw the appearance for an annotation dictionary: the expensive half of [`set_appearance`]. /// Pure from the dictionary and the objects it points at, so [`AppearanceCache`] can reuse it. fn build_appearance(doc: &Document, d: &Dict) -> Option { - let mut stream = appearance::build(&with_direct_values(doc, d))?; + // References followed first. Where that turns up something the builder doesn't draw (a + // cloudy `/BE` on a square), the drawing made without them stays, rather than none. + let direct = with_direct_values(doc, d); + let built = appearance::build(&direct); + let mut stream = match (built, direct) { + (Some(stream), _) => stream, + (None, std::borrow::Cow::Owned(_)) => appearance::build(d)?, + (None, std::borrow::Cow::Borrowed(_)) => return None, + }; // Picture-stamp restyling must retain the placement's page-axis correction. if d.get(b"PCPicture").is_some() { let matrix = d.get(b"AP").and_then(|ap| { diff --git a/crates/annot/src/tests.rs b/crates/annot/src/tests.rs index 6acb8ce742..28ab7e8fa7 100644 --- a/crates/annot/src/tests.rs +++ b/crates/annot/src/tests.rs @@ -1487,6 +1487,22 @@ fn indirect_border_style_and_colour_are_drawn() { assert_eq!(props.width, Some(6.0), "the properties show the width drawn"); } +/// A followed reference the builder can't draw (a cloudy border effect on a square, which it +/// leaves alone) keeps the drawing made without it instead of none. +#[test] +fn an_undrawable_indirect_value_keeps_the_plain_drawing() { + let doc = document_of(&[ + b"<< /Type /Catalog /Pages 2 0 R >>", + b"<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 200 200] >>", + b"<< /Type /Page /Parent 2 0 R /Annots [4 0 R] >>", + b"<< /Type /Annot /Subtype /Square /Rect [20 20 180 180] /BE 5 0 R /C [1 0 0] >>", + b"<< /S /C /I 1 >>", + ]); + let display = with_missing_appearances(&doc).expect("the square is still drawn"); + let shown = list(&display, 0).remove(0); + assert!(ap_content(&display, &shown).contains("1 0 0 RG\n1 w\n")); +} + /// Following references copies a bounded number of values: an indirect array past the budget /// stays a reference (and the comment is not drawn from it), a chain that never ends is not /// followed, and links to other objects are never copied. From b5e3de67fbd39ac458bd9cf62fae56af2ead6839 Mon Sep 17 00:00:00 2001 From: mtlcl410 Date: Sat, 7 Nov 2026 12:51:22 +0800 Subject: [PATCH 3/4] M5: properties and edits read referenced colours as drawn; following references is bounded per display pass (view.render-annotation-appearances) props() reads the whole annotation with references followed, so an indirect /C or /IC (ISO 32000-2 7.3.10) shows its colour, and "restylable" and the set_style / set_fill / set_part_opacity checks use the same build and fallback as the display (draw::can_draw), so such a comment can be restyled. Following references now counts every value copied, inner arrays and dictionaries included, and one display pass copies at most 4M values in all, so an object many annotations point at is not copied for each of them without bound. A cached appearance keeps what storing it needs (the undrawable character and a note's /Rect), so a cache hit follows no references at all. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pq7HyDBXygTf7jcqdsjcfn --- crates/annot/src/draw.rs | 148 ++++++++++++++++++++++++++----------- crates/annot/src/info.rs | 10 +-- crates/annot/src/modify.rs | 6 +- crates/annot/src/tests.rs | 43 +++++++++++ 4 files changed, 157 insertions(+), 50 deletions(-) diff --git a/crates/annot/src/draw.rs b/crates/annot/src/draw.rs index e36bf3a534..85eab4a487 100644 --- a/crates/annot/src/draw.rs +++ b/crates/annot/src/draw.rs @@ -33,21 +33,28 @@ pub(crate) fn redraw_or_restore(doc: &mut Document, r: ObjRef, before: Dict) -> pub(crate) fn draw_appearance(doc: &mut Document, r: ObjRef) -> Result<(), AnnotError> { let d = annot_dict(doc, r); let subtype = String::from_utf8_lossy(d.name(b"Subtype").unwrap_or_default()).into_owned(); - let Some(stream) = build_appearance(doc, &d) else { return Err(AnnotError::Unsupported(subtype)) }; - apply_appearance(doc, r, &d, stream) + let direct = with_direct_values(doc, &d); + let Some(stream) = build_appearance(doc, &d, &direct) else { return Err(AnnotError::Unsupported(subtype)) }; + let built = Built::new(stream, &direct); + apply_appearance(doc, r, &d, built) +} + +/// Whether [`set_appearance`] can draw the annotation `d`: read as the display reads it, with +/// references followed and the same fallback. Edits check this before changing anything. +pub(crate) fn can_draw(doc: &Document, d: &Dict) -> bool { + build_appearance(doc, d, &with_direct_values(doc, d)).is_some() } /// Draw the appearance for an annotation dictionary: the expensive half of [`set_appearance`]. -/// Pure from the dictionary and the objects it points at, so [`AppearanceCache`] can reuse it. -fn build_appearance(doc: &Document, d: &Dict) -> Option { +/// `direct` is `d` with references followed ([`with_direct_values`]). Pure from the dictionary and +/// the objects it points at, so [`AppearanceCache`] can reuse it. +fn build_appearance(doc: &Document, d: &Dict, direct: &Dict) -> Option { // References followed first. Where that turns up something the builder doesn't draw (a // cloudy `/BE` on a square), the drawing made without them stays, rather than none. - let direct = with_direct_values(doc, d); - let built = appearance::build(&direct); - let mut stream = match (built, direct) { - (Some(stream), _) => stream, - (None, std::borrow::Cow::Owned(_)) => appearance::build(d)?, - (None, std::borrow::Cow::Borrowed(_)) => return None, + let mut stream = match appearance::build(direct) { + Some(stream) => stream, + None if !std::ptr::eq(direct, d) => appearance::build(d)?, + None => return None, }; // Picture-stamp restyling must retain the placement's page-axis correction. if d.get(b"PCPicture").is_some() { @@ -76,6 +83,11 @@ const INLINE_DEPTH: usize = 4; /// annotation, so a file can't have a large object copied for every entry that points at it. const INLINE_BUDGET: usize = 1 << 16; +/// Values copied for all the annotations one display pass draws +/// ([`with_missing_appearances_cached`]), so a large object that many annotations point at is +/// not copied for each of them without bound. Past it, references are left as they are. +pub(crate) const PASS_BUDGET: usize = 1 << 22; + /// `d` with the references among the values [`appearance::build`] reads followed. Any value may be /// given by reference (ISO 32000-2 §7.3.10): a border style dictionary (`/BS 5 0 R`, Table 168), a /// colour, `/RD`, `/CL`, a `/W` inside `/BS`. The builder reads only direct values, so it draws @@ -83,7 +95,14 @@ const INLINE_BUDGET: usize = 1 << 16; /// [`INLINE_DEPTH`] levels or [`INLINE_BUDGET`] copied values a reference is left as it is, which /// the builder treats as unreadable. `d` itself when nothing is a reference. pub(crate) fn with_direct_values<'d>(doc: &Document, d: &'d Dict) -> std::borrow::Cow<'d, Dict> { - let mut budget = INLINE_BUDGET; + with_direct_values_within(doc, d, &mut INLINE_BUDGET.clone()) +} + +/// [`with_direct_values`], copying at most [`INLINE_BUDGET`] values and no more than `pass` has +/// left, which is reduced by what was copied. +pub(crate) fn with_direct_values_within<'d>(doc: &Document, d: &'d Dict, pass: &mut usize) -> std::borrow::Cow<'d, Dict> { + let allowed = INLINE_BUDGET.min(*pass); + let mut budget = allowed; let mut out: Option = None; for (key, value) in d.iter() { if LINK_KEYS.contains(&key.as_slice()) { @@ -93,9 +112,36 @@ pub(crate) fn with_direct_values<'d>(doc: &Document, d: &'d Dict) -> std::borrow out.get_or_insert_with(|| d.clone()).set(key.clone(), direct); } } + *pass = pass.saturating_sub(allowed.saturating_sub(budget)); out.map_or(std::borrow::Cow::Borrowed(d), std::borrow::Cow::Owned) } +/// The number of values in `o`, counting into its arrays and dictionaries (not through +/// references, which are charged when followed), or `None` past `limit`. Walks at most `limit` +/// values and holds no more than `limit` waiting. +fn weight(o: &Object, limit: usize) -> Option { + let mut count = 0usize; + let mut stack = vec![o]; + while let Some(o) = stack.pop() { + count = count.checked_add(1).filter(|c| *c <= limit)?; + let inner = match o { + Object::Array(items) => items.len(), + Object::Dict(entries) => entries.len(), + _ => continue, + }; + // Every value waiting is counted later, so more than `limit` in all is over it already. + if count.saturating_add(stack.len()).saturating_add(inner) > limit { + return None; + } + match o { + Object::Array(items) => stack.extend(items.iter()), + Object::Dict(entries) => stack.extend(entries.iter().map(|(_, v)| v)), + _ => {} + } + } + Some(count) +} + /// `o` with references followed `depth` levels into arrays and dictionaries, or `None` when there /// is nothing to follow (or it may not be: a stream, an over-budget object, a chain of references). fn direct_value(doc: &Document, o: &Object, depth: usize, budget: &mut usize) -> Option { @@ -103,13 +149,11 @@ fn direct_value(doc: &Document, o: &Object, depth: usize, budget: &mut usize) -> let (base, followed) = match o { Object::Ref(_) => { held = doc.resolve(o); - let cost = match &*held { - Object::Stream(_) | Object::Ref(_) => return None, - Object::Array(a) => a.len().max(1), - Object::Dict(d) => d.len().max(1), - _ => 1, - }; - *budget = budget.checked_sub(cost)?; + if matches!(&*held, Object::Stream(_) | Object::Ref(_)) { + return None; + } + // Everything copied counts, inner arrays and dictionaries too. + *budget = budget.checked_sub(weight(&held, *budget)?)?; (&*held, true) } other => (other, false), @@ -141,12 +185,32 @@ fn direct_value(doc: &Document, o: &Object, depth: usize, budget: &mut usize) -> } } +/// A built appearance and what storing it needs from the dictionary it was drawn from, read +/// with references followed, so a cached one is stored without following them again. +#[derive(Clone, Debug)] +struct Built { + stream: Stream, + /// The first character the standard fonts can't draw (it came out as `?`). + missing: Option, + /// A note's or attachment's `/Rect`, as written. + rect: Option>, +} + +impl Built { + fn new(stream: Stream, direct: &Dict) -> Self { + let missing = appearance::undrawable_text(direct).map(|(_, c)| c); + let rect = matches!(direct.name(b"Subtype"), Some(b"Text" | b"FileAttachment")) + .then(|| direct.get(b"Rect")?.as_array()?.iter().map(|o| o.as_f64()).collect::>>()) + .flatten(); + Built { stream, missing, rect } + } +} + /// Store a built appearance on the annotation: the cheap half of [`set_appearance`]. -fn apply_appearance(doc: &mut Document, r: ObjRef, d: &Dict, stream: Stream) -> Result<(), AnnotError> { - // Characters the standard fonts can't draw came out as `?`: mark the annotation with the first. - // Read like the builder read it, references followed. - let direct = with_direct_values(doc, d); - let missing = appearance::undrawable_text(&direct).map(|(_, c)| c); +fn apply_appearance(doc: &mut Document, r: ObjRef, d: &Dict, built: Built) -> Result<(), AnnotError> { + // `missing`: characters the standard fonts can't draw came out as `?`; the annotation is + // marked with the first. + let Built { stream, missing, rect } = built; let ap = doc.add(Object::Stream(stream)); // A copy (a shared /AP is left alone) that keeps unknown entries; the old down and rollover // appearances would show the previous look on press or hover, so they go with the old /N. @@ -157,14 +221,10 @@ fn apply_appearance(doc: &mut Document, r: ObjRef, d: &Dict, stream: Stream) -> // A note icon has a fixed size (`NOTE_SIZE`); viewers draw it at the upper-left corner of `/Rect`. // The appearance's box is mapped onto `/Rect`, so any other `/Rect` (a large one, or one with // no width, as LaTeX `pdfcomment` writes) would stretch the icon or shrink it to nothing. - let icon_rect = matches!(d.name(b"Subtype"), Some(b"Text" | b"FileAttachment")) - .then(|| direct.get(b"Rect")?.as_array()?.iter().map(|o| o.as_f64()).collect::>>()) - .flatten() - .filter(|q| q.len() == 4 && finite(q)) - .map(|q| { - let (x, top) = (q[0].min(q[2]), q[1].max(q[3])); - [x, top - NOTE_SIZE, x + NOTE_SIZE, top] - }); + let icon_rect = rect.filter(|q| q.len() == 4 && finite(q)).map(|q| { + let (x, top) = (q[0].min(q[2]), q[1].max(q[3])); + [x, top - NOTE_SIZE, x + NOTE_SIZE, top] + }); doc.update_dict(r, |d| { d.set(b"AP".to_vec(), Object::Dict(apd)); if let Some(rect) = icon_rect { @@ -188,7 +248,7 @@ fn apply_appearance(doc: &mut Document, r: ObjRef, d: &Dict, stream: Stream) -> /// refreshes) and keep it for content-only ones. #[derive(Clone, Debug, Default)] pub struct AppearanceCache { - entries: std::collections::HashMap, + entries: std::collections::HashMap, bytes: usize, hits: u64, builds: u64, @@ -203,20 +263,21 @@ impl AppearanceCache { *self = Self::default(); } - fn get(&mut self, r: ObjRef) -> Option { - let stream = self.entries.get(&r)?.clone(); + fn get(&mut self, r: ObjRef) -> Option { + let built = self.entries.get(&r)?.clone(); self.hits += 1; - Some(stream) + Some(built) } - fn insert(&mut self, r: ObjRef, stream: &Stream) { + fn insert(&mut self, r: ObjRef, built: &Built) { + let stream = &built.stream; self.builds += 1; if self.entries.len() >= 1_000_000 { self.clear(); } if self.bytes.saturating_add(stream.raw.len()) <= Self::MAX_BYTES { self.bytes += stream.raw.len(); - self.entries.insert(r, stream.clone()); + self.entries.insert(r, built.clone()); } // Else over budget: keep serving the entries already cached instead of growing. } @@ -250,6 +311,7 @@ pub fn with_missing_appearances_cached(doc: &Document, cache: &mut AppearanceCac // reads `doc`, which never gets the appearance, so every repeat used to add another stream to // the copy, and the display bytes grew with the number of listings. let mut seen = std::collections::HashSet::new(); + let mut budget = PASS_BUDGET; for page in page_refs(doc).ok()? { for r in annots(doc, page).iter().filter_map(Object::as_ref) { if !seen.insert(r) { @@ -263,9 +325,9 @@ pub fn with_missing_appearances_cached(doc: &Document, cache: &mut AppearanceCac if has_normal { continue; } - if let Some(stream) = cache.get(r) { + if let Some(built) = cache.get(r) { // Fails only when `r` isn't a dictionary, which the cached build ruled out. - apply_appearance(copy.get_or_insert_with(|| doc.clone()), r, &d, stream).ok(); + apply_appearance(copy.get_or_insert_with(|| doc.clone()), r, &d, built).ok(); continue; } // Another program's stamp gets a drawing for display only (ISO 32000-2 Table 184). Its @@ -275,10 +337,12 @@ pub fn with_missing_appearances_cached(doc: &Document, cache: &mut AppearanceCac bare.remove(b"AP"); appearance::display_stamp(&bare) }; - let Some(stream) = build_appearance(doc, &d).or_else(other_stamp) else { continue }; - cache.insert(r, &stream); + let direct = with_direct_values_within(doc, &d, &mut budget); + let Some(stream) = build_appearance(doc, &d, &direct).or_else(other_stamp) else { continue }; + let built = Built::new(stream, &direct); + cache.insert(r, &built); // Fails only when `r` isn't a dictionary, which the build just ruled out. - apply_appearance(copy.get_or_insert_with(|| doc.clone()), r, &d, stream).ok(); + apply_appearance(copy.get_or_insert_with(|| doc.clone()), r, &d, built).ok(); } } copy diff --git a/crates/annot/src/info.rs b/crates/annot/src/info.rs index e451e3009a..3d54140abf 100644 --- a/crates/annot/src/info.rs +++ b/crates/annot/src/info.rs @@ -233,14 +233,14 @@ pub fn props(doc: &Document, page: usize, index: usize) -> Option { let p = page_ref(doc, page).ok()?; let entry = annots(doc, p).get(index).cloned()?; let obj = doc.resolve(&entry); - let d = obj.as_dict()?; + // Values given by reference (`/C 7 0 R`, `/BS 5 0 R`, ISO 32000-2 §7.3.10) read as the display draws them. + let raw = obj.as_dict()?; + let direct = crate::draw::with_direct_values(doc, raw); + let d = direct.as_ref(); let subtype = String::from_utf8_lossy(d.name(b"Subtype")?).into_owned(); let c: Vec = d.get(b"C").and_then(|o| o.as_array().map(|a| a.iter().filter_map(|x| x.as_f64()).collect())).unwrap_or_default(); let color = if subtype == "FreeText" { Some(appearance::parse_da(d).0) } else { (c.len() == 3).then(|| [c[0], c[1], c[2]]) }; let width = matches!(subtype.as_str(), "Square" | "Circle" | "Line" | "Ink" | "Polygon" | "PolyLine" | "FreeText").then(|| { - // A border style given by reference (`/BS 5 0 R`) reads as the display draws it. - let direct = crate::draw::with_direct_values(doc, d); - let d = direct.as_ref(); if subtype == "FreeText" { if !d.contains(b"BS") && !d.contains(b"Border") { 0.0 } else { appearance::border_width(d) } } else { @@ -255,7 +255,7 @@ pub fn props(doc: &Document, page: usize, index: usize) -> Option { width, icon: (subtype == "Text").then(|| d.name(b"Name").and_then(|n| NoteIcon::from_name(&String::from_utf8_lossy(n))).unwrap_or(NoteIcon::Note)), modified: text_value(doc, d, b"M"), - restylable: appearance::build(d).is_some(), + restylable: crate::draw::can_draw(doc, raw), locked: annotation_flags(doc, d) & FLAG_LOCKED != 0, fillable: fillable(&subtype), fill: fill_of(d), diff --git a/crates/annot/src/modify.rs b/crates/annot/src/modify.rs index 54636d1841..4b454239c2 100644 --- a/crates/annot/src/modify.rs +++ b/crates/annot/src/modify.rs @@ -710,7 +710,7 @@ pub fn set_style(doc: &mut Document, page: usize, index: usize, change: StyleCha let d = annot_dict(doc, r); let subtype = String::from_utf8_lossy(d.name(b"Subtype").unwrap_or_default()).into_owned(); // Check before changing anything: a stale appearance would contradict the new style. - if appearance::build(&d).is_none() { + if !crate::draw::can_draw(doc, &d) { return Err(AnnotError::Unsupported(subtype.clone())); } if color.is_some_and(|c| !finite(&c)) || opacity.is_some_and(|o| !o.is_finite()) || width.is_some_and(|w| !w.is_finite()) { @@ -875,7 +875,7 @@ pub fn set_fill(doc: &mut Document, page: usize, index: usize, fill: Option return Err(AnnotError::Invalid("only rectangles, ovals, polygons and text boxes can be filled".into())); } // Check before changing anything: a stale appearance would contradict the new fill. - if appearance::build(&d).is_none() { + if !crate::draw::can_draw(doc, &d) { return Err(AnnotError::Unsupported(subtype)); } if fill.is_some_and(|c| !finite(&c)) { @@ -916,7 +916,7 @@ pub fn set_part_opacity(doc: &mut Document, page: usize, index: usize, fill: Opt return Err(AnnotError::Invalid("invalid opacity".into())); } // Check before changing anything: a stale appearance would contradict the new opacity. - if appearance::build(&d).is_none() { + if !crate::draw::can_draw(doc, &d) { return Err(AnnotError::Unsupported(subtype)); } refuse_foreign_text(doc, &d)?; diff --git a/crates/annot/src/tests.rs b/crates/annot/src/tests.rs index 28ab7e8fa7..132a69bb02 100644 --- a/crates/annot/src/tests.rs +++ b/crates/annot/src/tests.rs @@ -1550,6 +1550,49 @@ fn new_width_keeps_an_indirect_border_style() { assert_eq!(shared.as_dict().and_then(|b| b.get(b"W")).and_then(Object::as_f64), Some(6.0), "the shared object is unchanged"); } +/// A colour given by reference (`/C 6 0 R`, §7.3.10) is read by the properties and the edits as +/// the display draws it: the properties show it, the appearance can be restyled and filled. +#[test] +fn an_indirect_colour_reads_and_restyles_as_drawn() { + let mut doc = document_of(&[ + b"<< /Type /Catalog /Pages 2 0 R >>", + b"<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 200 200] >>", + b"<< /Type /Page /Parent 2 0 R /Annots [4 0 R] >>", + b"<< /Type /Annot /Subtype /Square /Rect [20 20 180 180] /C 5 0 R /IC 6 0 R >>", + b"[0 0 1]", + b"[1 1 0]", + ]); + let p = props(&doc, 0, 0).expect("props"); + assert_eq!((p.color, p.fill, p.restylable), (Some([0.0, 0.0, 1.0]), Some([1.0, 1.0, 0.0]), true)); + set_style(&mut doc, 0, 0, StyleChange::new(None, None, Some(3.0), None), &meta("")).expect("restylable"); + set_fill(&mut doc, 0, 0, Some([0.0, 1.0, 0.0]), &meta("")).expect("fillable"); + let d = list(&doc, 0).remove(0); + let content = ap_content(&doc, &d); + assert!(content.contains("0 0 1 RG") && content.contains("0 1 0 rg") && content.contains("3 w"), "{content}"); +} + +/// References are followed into everything they point at, and all of it is counted: a referenced +/// array of two arrays of 40,000 numbers is over the per-annotation budget. One display pass +/// copies at most [`crate::draw::PASS_BUDGET`] values in all, so an array many annotations point +/// at is not copied again for each of them. +#[test] +fn following_references_is_bounded_per_value_and_per_pass() { + let mut doc = fixture(); + let inner = Object::Array(vec![Object::Int(0); 40_000]); + let nested = doc.add(Object::Array(vec![inner.clone(), inner])); + let mut d = Dict::new(); + d.set(b"Subtype".to_vec(), Object::name("Square")); + d.set(b"Junk".to_vec(), Object::Ref(nested)); + let direct = crate::draw::with_direct_values(&doc, &d); + assert!(direct.get(b"Junk") == Some(&Object::Ref(nested)), "80,003 values: left as a reference"); + + let shared = doc.add(Object::Array(vec![Object::Int(0); 60_000])); + d.set(b"Junk".to_vec(), Object::Ref(shared)); + let mut pass = crate::draw::PASS_BUDGET; + let copies = (0..100).filter(|_| matches!(crate::draw::with_direct_values_within(&doc, &d, &mut pass), std::borrow::Cow::Owned(_))).count(); + assert_eq!(copies, crate::draw::PASS_BUDGET / 60_001, "copies stop when the pass budget is spent"); +} + /// `/RD` is optional (ISO 32000-2 Table 177): a callout without it has its text box at `/Rect` /// and is drawn, box, text and leader line. A callout line and `/RD` given by reference are read too. #[test] From 6a238f9fd7563d8b2ce4f1f90652284fc7e76447 Mon Sep 17 00:00:00 2001 From: mtlcl410 Date: Sat, 7 Nov 2026 14:01:34 +0800 Subject: [PATCH 4/4] M5: finding a referenced object too large to copy is charged to the budgets (view.render-annotation-appearances) A referenced object rejected as over the per-annotation budget was walked up to that budget and charged nothing, so many annotations (or many references in one annotation) pointing at the same oversized nested array each walked it again, and PASS_BUDGET did not bound the work. The walk is now charged to the annotation's budget and through it to the pass, and a spent budget follows no further references. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pq7HyDBXygTf7jcqdsjcfn --- crates/annot/src/draw.rs | 45 ++++++++++++++++++++++++++++----------- crates/annot/src/tests.rs | 27 +++++++++++++++++++++++ 2 files changed, 59 insertions(+), 13 deletions(-) diff --git a/crates/annot/src/draw.rs b/crates/annot/src/draw.rs index 85eab4a487..cda0e6992b 100644 --- a/crates/annot/src/draw.rs +++ b/crates/annot/src/draw.rs @@ -79,13 +79,15 @@ const LINK_KEYS: [&[u8]; 5] = [b"P", b"Popup", b"IRT", b"Parent", b"AP"]; /// arrays, `/Border` may end in a dash array. const INLINE_DEPTH: usize = 4; -/// Values [`with_direct_values`] copies out of referenced arrays and dictionaries for one -/// annotation, so a file can't have a large object copied for every entry that points at it. -const INLINE_BUDGET: usize = 1 << 16; +/// Values [`with_direct_values`] copies out of referenced arrays and dictionaries, or walks +/// before finding one too large to copy, for one annotation, so a file can't have a large object +/// copied or scanned for every entry that points at it. +pub(crate) const INLINE_BUDGET: usize = 1 << 16; -/// Values copied for all the annotations one display pass draws +/// Values copied or walked for all the annotations one display pass draws /// ([`with_missing_appearances_cached`]), so a large object that many annotations point at is -/// not copied for each of them without bound. Past it, references are left as they are. +/// not copied, or scanned and found too large, for each of them without bound. Past it, +/// references are left as they are. pub(crate) const PASS_BUDGET: usize = 1 << 22; /// `d` with the references among the values [`appearance::build`] reads followed. Any value may be @@ -117,13 +119,18 @@ pub(crate) fn with_direct_values_within<'d>(doc: &Document, d: &'d Dict, pass: & } /// The number of values in `o`, counting into its arrays and dictionaries (not through -/// references, which are charged when followed), or `None` past `limit`. Walks at most `limit` -/// values and holds no more than `limit` waiting. -fn weight(o: &Object, limit: usize) -> Option { +/// references, which are charged when followed), or, past `limit`, `Err` with the values it +/// visited or queued before finding that out (at least one), so a rejected walk is charged too. +/// Walks at most `limit` values and holds no more than `limit` waiting. +fn weight(o: &Object, limit: usize) -> Result { let mut count = 0usize; let mut stack = vec![o]; while let Some(o) = stack.pop() { - count = count.checked_add(1).filter(|c| *c <= limit)?; + let spent = count.saturating_add(stack.len()).max(1); + count = match count.checked_add(1).filter(|c| *c <= limit) { + Some(c) => c, + None => return Err(spent), + }; let inner = match o { Object::Array(items) => items.len(), Object::Dict(entries) => entries.len(), @@ -131,7 +138,7 @@ fn weight(o: &Object, limit: usize) -> Option { }; // Every value waiting is counted later, so more than `limit` in all is over it already. if count.saturating_add(stack.len()).saturating_add(inner) > limit { - return None; + return Err(spent.saturating_add(1)); } match o { Object::Array(items) => stack.extend(items.iter()), @@ -139,7 +146,7 @@ fn weight(o: &Object, limit: usize) -> Option { _ => {} } } - Some(count) + Ok(count) } /// `o` with references followed `depth` levels into arrays and dictionaries, or `None` when there @@ -148,12 +155,24 @@ fn direct_value(doc: &Document, o: &Object, depth: usize, budget: &mut usize) -> let held; let (base, followed) = match o { Object::Ref(_) => { + // A spent budget follows nothing more, not even to find out what is there. + if *budget == 0 { + return None; + } held = doc.resolve(o); if matches!(&*held, Object::Stream(_) | Object::Ref(_)) { return None; } - // Everything copied counts, inner arrays and dictionaries too. - *budget = budget.checked_sub(weight(&held, *budget)?)?; + // Everything copied counts, inner arrays and dictionaries too, and so does the walk + // that finds an object too large: many references to one would otherwise each walk + // it again for free. + match weight(&held, *budget) { + Ok(n) => *budget = budget.checked_sub(n)?, + Err(spent) => { + *budget = budget.saturating_sub(spent); + return None; + } + } (&*held, true) } other => (other, false), diff --git a/crates/annot/src/tests.rs b/crates/annot/src/tests.rs index 132a69bb02..2bc8121612 100644 --- a/crates/annot/src/tests.rs +++ b/crates/annot/src/tests.rs @@ -1593,6 +1593,33 @@ fn following_references_is_bounded_per_value_and_per_pass() { assert_eq!(copies, crate::draw::PASS_BUDGET / 60_001, "copies stop when the pass budget is spent"); } +/// Finding an object too large to copy costs the walk that found it out: an array of two arrays +/// of 40,000 numbers that many annotations point at is walked about 40,000 values deep before it +/// is rejected, and that is charged to the annotation and to the pass, so after a bounded number +/// of such walks the pass has no room left to walk it again. A reference to it repeated in one annotation +/// is not walked again in full, and a small referenced colour after it is still read. +#[test] +fn a_rejected_large_object_is_charged_to_the_budgets() { + let mut doc = fixture(); + let inner = Object::Array(vec![Object::Int(0); 40_000]); + let nested = doc.add(Object::Array(vec![inner.clone(), inner])); + let red = doc.add(Object::Array(vec![Object::Int(1), Object::Int(0), Object::Int(0)])); + let mut d = Dict::new(); + d.set(b"Subtype".to_vec(), Object::name("Square")); + d.set(b"Junk".to_vec(), Object::Array(vec![Object::Ref(nested); 1_000])); + d.set(b"C".to_vec(), Object::Ref(red)); + let mut pass = crate::draw::PASS_BUDGET; + let direct = crate::draw::with_direct_values_within(&doc, &d, &mut pass); + assert_eq!(direct.get(b"C"), Some(&Object::Array(vec![Object::Int(1), Object::Int(0), Object::Int(0)]))); + let charged = crate::draw::PASS_BUDGET - pass; + assert!((40_000..=crate::draw::INLINE_BUDGET).contains(&charged), "the rejected walk is charged once: {charged}"); + d.set(b"Junk".to_vec(), Object::Ref(nested)); + for _ in 0..crate::draw::PASS_BUDGET / 40_000 { + crate::draw::with_direct_values_within(&doc, &d, &mut pass); + } + assert!(pass < 40_000, "after {} annotations the pass has no room for another full walk: {pass}", crate::draw::PASS_BUDGET / 40_000 + 1); +} + /// `/RD` is optional (ISO 32000-2 Table 177): a callout without it has its text box at `/Rect` /// and is drawn, box, text and leader line. A callout line and `/RD` given by reference are read too. #[test]