perf(render): a picture that doesn't decode is decoded once, not at every draw (hayro-interpret patch 91)

The decoded-image cache stored a failed decode as an empty entry and read
an empty entry as a miss, so a picture or stencil mask that doesn't decode
(damaged data, a JPX or JBIG2 stream its decoder refuses) was decoded again
at every draw, and at every tile, preview and zoom of a render worker that
shares the cache.

The cache now keeps a failure for its key, with the warnings the decode
reported; a later draw reports the same warnings and gets nothing, as the
decode did. A failure counts 128 bytes towards the cache's 64 MiB budget.
A failure the page's JBIG2 allowance may have caused (ContentTruncated, or
the allowance spent) isn't kept, since the next page starts afresh.
Successful decodes are cached as before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pq7HyDBXygTf7jcqdsjcfn

crates/render/src/raster/tests.rs

@@ -8196,6 +8196,10 @@ fn decoded_images_serve_every_size_they_are_drawn_at() {
 // ICC profiles whose stream dictionaries are the same (vendored hayro-interpret patch 48).
 mod icc_profile_cache;
 
+// A picture that doesn't decode is decoded once however often it is drawn (vendored
+// hayro-interpret patch 91).
+mod failed_image_decodes;
+
 // A symbolic TrueType font draws through its (3, 0) cmap subtable first (#4156).
 mod symbolic_truetype_cmap;
 

crates/render/src/raster/tests/failed_image_decodes.rs

@@ -0,0 +1,216 @@
+//! A picture that doesn't decode is decoded once however often a page draws it (vendored
+//! hayro-interpret patch 91). The decoded-image cache kept only decodes that succeeded, so a
+//! damaged picture, or a JPX or JBIG2 stream its decoder refuses, was decoded again at every
+//! draw: a page drawing it a thousand times decoded it a thousand times. The failure is kept for
+//! the same key now, and every draw still reports it as it did.
+
+use super::image_cache_pdf;
+use hayro::hayro_interpret::font::Glyph;
+use hayro::hayro_interpret::hayro_syntax::Pdf;
+use hayro::hayro_interpret::{
+    BlendMode, ClipPath, Context, Device, GlyphDrawMode, Image, InterpreterCache, InterpreterSettings, InterpreterWarning, Paint, PathDrawMode,
+    SoftMask, TransformExt, interpret_page,
+};
+use kurbo::{Affine, BezPath, Rect};
+use std::io::Write;
+use std::sync::atomic::{AtomicUsize, Ordering};
+use std::sync::{Arc, Mutex};
+
+/// What a page handed its device: the raster images with their pixels, those of them with a
+/// mask, and the stencil masks with theirs. Every image is asked for at the size it is drawn
+/// at, as the renderer does.
+#[derive(Default)]
+struct Draws {
+    rasters: usize,
+    masked: usize,
+    stencils: usize,
+}
+
+impl<'a> Device<'a> for Draws {
+    fn set_soft_mask(&mut self, _: Option<SoftMask<'a>>) {}
+    fn set_blend_mode(&mut self, _: BlendMode) {}
+    fn draw_path(&mut self, _: &BezPath, _: Affine, _: &Paint<'a>, _: &PathDrawMode) {}
+    fn push_clip_path(&mut self, _: &ClipPath) {}
+    fn push_transparency_group(&mut self, _: f32, _: Option<SoftMask<'a>>, _: BlendMode) {}
+    fn draw_glyph(&mut self, _: &Glyph<'a>, _: Affine, _: Affine, _: &Paint<'a>, _: &GlyphDrawMode) {}
+    fn draw_image(&mut self, image: Image<'a, '_>, transform: Affine) {
+        let [a, b, c, d, ..] = transform.as_coeffs();
+        let target = ((a.hypot(b) * f64::from(image.width())).ceil() as u32, (c.hypot(d) * f64::from(image.height())).ceil() as u32);
+        match image {
+            Image::Raster(r) => r.with_rgba(
+                |_, alpha| {
+                    self.rasters += 1;
+                    self.masked += usize::from(alpha.is_some());
+                },
+                Some(target),
+            ),
+            Image::Stencil(s) => s.with_stencil(|_, _| self.stencils += 1, Some(target)),
+        }
+    }
+    fn pop_clip_path(&mut self) {}
+    fn pop_transparency_group(&mut self) {}
+}
+
+/// What drawing `/name` five times at one size, and then the picture that decodes (`/G`) once
+/// on the right, did: the decodes the cache ran, the image decode failures reported, and what
+/// the device was handed.
+struct Outcome {
+    decodes: usize,
+    failures: usize,
+    draws: Draws,
+}
+
+fn draw_five_times(name: &str) -> Outcome {
+    let flate = |data: &[u8]| {
+        let mut z = flate2::write::ZlibEncoder::new(Vec::new(), flate2::Compression::fast());
+        z.write_all(data).unwrap();
+        z.finish().unwrap()
+    };
+    let pixels = flate(&(0..64 * 64 * 3).map(|i| (i % 251) as u8).collect::<Vec<_>>());
+    let image = |filter: &str, extra: &str, len: usize| {
+        format!(
+            "<< /Type /XObject /Subtype /Image /Width 64 /Height 64 /ColorSpace /DeviceRGB /BitsPerComponent 8 /Filter /{filter} {extra}/Length {len} >>"
+        )
+    };
+    let good = image("FlateDecode", "", pixels.len());
+    // A JPEG 2000 picture whose codestream is four zero bytes.
+    let bad_jpx = image("JPXDecode", "", 4);
+    // A picture whose soft mask is such a JPEG 2000 stream.
+    let bad_mask = image("FlateDecode", "/SMask 9 0 R ", pixels.len());
+    // A stencil mask whose JBIG2 data is four zero bytes.
+    let bad_stencil = "<< /Type /XObject /Subtype /Image /Width 64 /Height 64 /ImageMask true /Filter /JBIG2Decode /Length 4 >>";
+    let jpx_mask =
+        "<< /Type /XObject /Subtype /Image /Width 64 /Height 64 /ColorSpace /DeviceGray /BitsPerComponent 8 /Filter /JPXDecode /Length 4 >>";
+    let content = [0, 24, 48, 72, 96].map(|x| format!("q 20 0 0 20 {x} 10 cm /{name} Do Q")).join(" ") + " q 10 0 0 10 118 10 cm /G Do Q";
+    let bytes = image_cache_pdf(
+        &content,
+        "<< /XObject << /G 5 0 R /J 6 0 R /S 7 0 R /M 8 0 R >> >>",
+        &[(&good, &pixels), (&bad_jpx, b"\0\0\0\0"), (&bad_mask, &pixels), (bad_stencil, b"\0\0\0\0"), (jpx_mask, b"\0\0\0\0")],
+    );
+    let parsed = Pdf::new(Arc::new(bytes)).expect("parses");
+    let page = &parsed.pages()[0];
+    let cache = InterpreterCache::new();
+    let failures = Arc::new(AtomicUsize::new(0));
+    let others = Arc::new(Mutex::new(Vec::new()));
+    let settings = InterpreterSettings {
+        warning_sink: {
+            let (failures, others) = (failures.clone(), others.clone());
+            Arc::new(move |warning| match warning {
+                InterpreterWarning::ImageDecodeFailure => {
+                    failures.fetch_add(1, Ordering::Relaxed);
+                }
+                other => others.lock().unwrap().push(format!("{other:?}")),
+            })
+        },
+        ..InterpreterSettings::default()
+    };
+    let initial = page.initial_transform(true).to_kurbo();
+    let mut ctx = Context::new(initial, Rect::new(0.0, 0.0, 130.0, 70.0), &cache, page.xref(), settings);
+    let mut draws = Draws::default();
+    interpret_page(page, &mut ctx, &mut draws);
+    assert!(others.lock().unwrap().is_empty(), "/{name} reported {:?}", others.lock().unwrap());
+    Outcome { decodes: cache.decoded_image_decodes(), failures: failures.load(Ordering::Relaxed), draws }
+}
+
+/// A JPEG 2000 picture that doesn't decode, drawn five times at one size, is decoded once, and
+/// each of the five draws still reports the failure.
+#[test]
+fn a_picture_that_does_not_decode_is_decoded_once_however_often_it_is_drawn() {
+    let out = draw_five_times("J");
+    assert_eq!(out.draws.rasters, 1, "only the picture after it drew");
+    assert_eq!(out.decodes, 2, "decoded again at every draw");
+    assert_eq!(out.failures, 5, "every draw reports the failure, as before");
+}
+
+/// A JBIG2 stencil mask that doesn't decode, drawn five times, is decoded once, and each draw
+/// still reports the failure: it isn't the page's JBIG2 allowance that refused it, which a
+/// later page would have afresh.
+#[test]
+fn a_stencil_mask_that_does_not_decode_is_decoded_once_however_often_it_is_drawn() {
+    let out = draw_five_times("M");
+    assert_eq!((out.draws.stencils, out.draws.rasters), (0, 1), "only the picture after it drew");
+    assert_eq!(out.decodes, 2, "decoded again at every draw");
+    assert_eq!(out.failures, 5, "every draw reports the failure, as before");
+}
+
+/// A picture whose soft mask doesn't decode draws without it at every draw, decoded once, and
+/// the mask's failure is reported once, as it was (the picture decodes; only its mask fails).
+#[test]
+fn a_picture_whose_soft_mask_does_not_decode_draws_unmasked_decoded_once() {
+    let out = draw_five_times("S");
+    assert_eq!((out.draws.rasters, out.draws.masked), (6, 0), "drawn five times without its mask, and the picture after it");
+    assert_eq!(out.decodes, 2);
+    assert_eq!(out.failures, 1);
+}
+
+/// A picture that decodes is still decoded once and handed to every draw.
+#[test]
+fn a_picture_that_decodes_is_still_decoded_once_and_drawn_every_time() {
+    let out = draw_five_times("G");
+    assert_eq!(out.draws.rasters, 6);
+    assert_eq!(out.decodes, 1);
+    assert_eq!(out.failures, 0);
+}
+
+/// A JBIG2 picture the page's allowance left out isn't kept as a failure: the next page drawing
+/// it starts with an allowance of its own and draws it, from the same cache.
+#[test]
+fn a_picture_left_out_for_the_pages_allowance_draws_on_the_next_page() {
+    use super::{RenderWarning, black_jbig2_page, jbig2_segment};
+    use crate::raster::{parse, render_page, take_warnings, warning_settings};
+    use crate::{RenderConfig, RenderRequest, RequestKind};
+    use hayro::RenderCache;
+
+    let image = |number: u8, id: u8| {
+        let mut stream = black_jbig2_page();
+        jbig2_segment(&mut stream, 1, 62, &[], &[0, 0, 0, 0, id]);
+        let mut object = format!(
+            "{number} 0 obj << /Type /XObject /Subtype /Image /Width 16 /Height 2 /BitsPerComponent 1 /ColorSpace /DeviceGray /Filter /JBIG2Decode /Length {} >> stream\n",
+            stream.len()
+        )
+        .into_bytes();
+        object.extend_from_slice(&stream);
+        object.extend_from_slice(b"\nendstream endobj\n");
+        object
+    };
+    let page = |number: u8, content: u8| {
+        format!("{number} 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 40 40] /Contents {content} 0 R /Resources << /XObject << /A 7 0 R /B 8 0 R >> >> >> endobj\n").into_bytes()
+    };
+    let content = |number: u8, text: &str| format!("{number} 0 obj << /Length {} >> stream\n{text}\nendstream endobj\n", text.len()).into_bytes();
+    let mut pdf =
+        b"%PDF-1.7\n1 0 obj << /Type /Catalog /Pages 2 0 R >> endobj\n2 0 obj << /Type /Pages /Kids [3 0 R 4 0 R] /Count 2 >> endobj\n".to_vec();
+    pdf.extend(page(3, 5));
+    pdf.extend(page(4, 6));
+    // The first page spends its allowance on A and has nothing left for B; the second draws B.
+    pdf.extend(content(5, "q 20 0 0 40 0 0 cm /A Do Q q 20 0 0 40 20 0 cm /B Do Q"));
+    pdf.extend(content(6, "q 20 0 0 40 20 0 cm /B Do Q"));
+    pdf.extend(image(7, 1));
+    pdf.extend(image(8, 2));
+    pdf.extend_from_slice(b"trailer << /Root 1 0 R >>\n%%EOF\n");
+    let pdf = parse(&Arc::new(pdf), None).expect("parses");
+    /// A page's pixels and the warnings it reported.
+    type Drawn = (Vec<u8>, Vec<RenderWarning>);
+    let black = |(rgba, _): &Drawn, x: usize| match &rgba[((20 * 40 + x) * 4)..][..4] {
+        [0, 0, 0, 255] => true,
+        [255, 255, 255, 255] => false,
+        other => panic!("neither an image nor the page: {other:?}"),
+    };
+    // Both pages through one cache, as a render worker keeps it.
+    let pages = |work: u64| {
+        let config = RenderConfig { jbig2_work: Some(work), ..RenderConfig::default() };
+        let cache = RenderCache::new();
+        [0, 1].map(|page| {
+            let warnings = Arc::new(Mutex::new(Vec::new()));
+            let settings = warning_settings(&config.settings(), warnings.clone());
+            let req = RenderRequest { page, kind: RequestKind::Pixels, tile: None, scale: 1.0, tag: 0, ..Default::default() };
+            let (_, _, rgba, _) = render_page(&pdf, &cache, &settings, &config, req).expect("renders");
+            (rgba[..].to_vec(), take_warnings(&warnings))
+        })
+    };
+    let one = (1..1_000).find(|work| black(&pages(*work)[0], 10)).expect("a small image is cheap");
+    let [first, second] = pages(one);
+    assert_eq!((black(&first, 10), black(&first, 30)), (true, false), "the first page had enough for A alone");
+    assert!(first.1.contains(&RenderWarning::ContentTruncated), "{:?}", first.1);
+    assert!(black(&second, 30), "B, left out of the first page, draws on the second");
+    assert!(second.1.is_empty(), "{:?}", second.1);
+}

vendor/hayro-interpret/src/cache.rs

@@ -1,10 +1,11 @@
 use crate::util::hash128;
+use crate::{InterpreterWarning, WarningSinkFn};
 use hayro_syntax::object::{Array, Dict, MaybeRef, Name, Null, ObjRef, Object, Stream};
 use kurbo::Affine;
 use std::any::Any;
 use std::collections::HashMap;
 use std::collections::hash_map::Entry;
-use std::sync::atomic::{AtomicUsize, Ordering};
+use std::sync::atomic::{AtomicU8, AtomicUsize, Ordering};
 use std::sync::{Arc, Mutex};
 
 type CacheMap = HashMap<u128, Option<Box<dyn Any + Send + Sync>>>;
@@ -69,6 +70,14 @@ impl Cache {
 /// them; when an insert would pass the budget the whole map is dropped (a fresh epoch), which
 /// only costs re-decoding, and an image larger than the whole budget is never kept. The budget
 /// caps memory per interpreter cache. A poisoned lock decodes without the cache.
+///
+/// PdfCraft patch (91): a decode that fails is kept too, as the warnings it reported, so an
+/// image drawn again and again that doesn't decode (damaged data, a JPX or JBIG2 stream its
+/// decoder refuses) isn't decoded again at every draw: a later draw reports the same warnings
+/// and gets nothing, as the decode did. A failure counts [`FAILED_DECODE_BYTES`] towards the
+/// budget, so failures can't grow the map without bound either. A failure that may come from
+/// what the page had left (its JBIG2 allowance, `ContentTruncated`) isn't kept: another page,
+/// or the same page drawn again, starts with a fresh allowance.
 #[derive(Clone)]
 pub(crate) struct DecodedImageCache {
     inner: Arc<Mutex<DecodedImageCacheInner>>,
@@ -110,9 +119,43 @@ pub(crate) struct DecodedImageKey {
     pub(crate) preview: bool,
 }
 
+/// PdfCraft patch (91): what the budget charges for a failed decode, about what its key and
+/// entry take in the map.
+const FAILED_DECODE_BYTES: usize = 128;
+
+/// PdfCraft patch (91): what a [`DecodedImageCache`] keeps for a key.
+enum DecodedEntry {
+    /// The decoded image.
+    Decoded(Box<dyn Any + Send + Sync>),
+    /// The decode failed, reporting these warnings ([`warning_bit`] flags).
+    Failed(u8),
+}
+
+/// PdfCraft patch (91): `warning` as a flag in a set of warnings.
+fn warning_bit(warning: InterpreterWarning) -> u8 {
+    match warning {
+        InterpreterWarning::UnsupportedFont => 1,
+        InterpreterWarning::ImageDecodeFailure => 2,
+        InterpreterWarning::ContentTruncated => 4,
+        InterpreterWarning::ColorFallback => 8,
+    }
+}
+
+/// PdfCraft patch (91): the warnings in a set of [`warning_bit`] flags.
+fn warnings_in(bits: u8) -> impl Iterator<Item = InterpreterWarning> {
+    [
+        InterpreterWarning::UnsupportedFont,
+        InterpreterWarning::ImageDecodeFailure,
+        InterpreterWarning::ContentTruncated,
+        InterpreterWarning::ColorFallback,
+    ]
+    .into_iter()
+    .filter(move |warning| bits & warning_bit(*warning) != 0)
+}
+
 #[derive(Default)]
 struct DecodedImageCacheInner {
-    map: HashMap<DecodedImageKey, Option<Box<dyn Any + Send + Sync>>>,
+    map: HashMap<DecodedImageKey, DecodedEntry>,
     bytes: usize,
     /// PdfCraft patch (32): how many keyed decodes ran (cache misses).
     decodes: usize,
@@ -140,44 +183,73 @@ impl DecodedImageCache {
         }
     }
 
-    /// The decoded image for `key`, from the cache or from `f`. `None` as the key decodes
-    /// without the cache.
+    /// The decoded image for `key`, from the cache or from `f`, which decodes reporting its
+    /// warnings to the sink it is handed. `None` as the key decodes without the cache.
+    /// PdfCraft patch (91): `sink` gets the warnings of the decode, or those of the failed
+    /// decode kept for `key`.
     pub(crate) fn get_or_insert_with<T: Clone + Send + Sync + 'static>(
         &self,
         key: Option<DecodedImageKey>,
         budget: usize,
         size_of: impl FnOnce(&T) -> usize,
-        f: impl FnOnce() -> Option<T>,
+        sink: &WarningSinkFn,
+        f: impl FnOnce(&WarningSinkFn) -> Option<T>,
     ) -> Option<T> {
-        let Some(key) = key else { return f() };
-        // Fast path: a hit clones the pixels without decoding.
+        let Some(key) = key else { return f(sink) };
+        // Fast path: a hit clones the pixels without decoding, and a kept failure reports its
+        // warnings again without decoding.
         let hit = match self.inner.lock() {
-            Ok(locked) => locked
-                .map
-                .get(&key)
-                .and_then(|val| val.as_ref())
-                .and_then(|val| val.downcast_ref::<T>().cloned()),
+            Ok(locked) => match locked.map.get(&key) {
+                Some(DecodedEntry::Decoded(val)) => val.downcast_ref::<T>().cloned().map(Ok),
+                Some(DecodedEntry::Failed(bits)) => Some(Err(*bits)),
+                None => None,
+            },
             // A thread panicked while holding the lock: decode without the cache.
-            Err(_) => return f(),
+            Err(_) => return f(sink),
         };
-        if let Some(hit) = hit {
-            return Some(hit);
+        match hit {
+            Some(Ok(hit)) => return Some(hit),
+            Some(Err(bits)) => {
+                warnings_in(bits).for_each(|warning| sink(warning));
+                return None;
+            }
+            None => {}
         }
-        // Miss: decode without holding the lock (decoding must never deadlock the cache).
-        let val = f();
+        // Miss: decode without holding the lock (decoding must never deadlock the cache),
+        // noting the warnings it reports.
+        let reported = Arc::new(AtomicU8::new(0));
+        let recording: WarningSinkFn = {
+            let (reported, sink) = (reported.clone(), sink.clone());
+            Arc::new(move |warning| {
+                reported.fetch_or(warning_bit(warning), Ordering::Relaxed);
+                sink(warning);
+            })
+        };
+        let val = f(&recording);
+        let reported = reported.load(Ordering::Relaxed);
         let Ok(mut locked) = self.inner.lock() else {
             return val;
         };
         locked.decodes = locked.decodes.saturating_add(1);
         // Another thread may have decoded it meanwhile: keep the first copy.
-        if let Some(existing) = locked.map.get(&key)
-            && let Some(existing) = existing.as_ref().and_then(|val| val.downcast_ref::<T>())
-        {
-            return Some(existing.clone());
+        match locked.map.get(&key) {
+            Some(DecodedEntry::Decoded(existing)) => {
+                if let Some(existing) = existing.downcast_ref::<T>() {
+                    return Some(existing.clone());
+                }
+            }
+            Some(DecodedEntry::Failed(_)) if val.is_none() => return None,
+            _ => {}
         }
         let size = match &val {
             Some(v) => size_of(v),
-            None => 0,
+            // A failure the page's JBIG2 allowance may have caused isn't the image's to keep.
+            None if reported & warning_bit(InterpreterWarning::ContentTruncated) != 0
+                || hayro_syntax::page_jbig2_work_spent() =>
+            {
+                return None;
+            }
+            None => FAILED_DECODE_BYTES,
         };
         if size > budget {
             return val;
@@ -186,12 +258,14 @@ impl DecodedImageCache {
             locked.map.clear();
             locked.bytes = 0;
         }
+        let entry = match &val {
+            Some(val) => DecodedEntry::Decoded(Box::new(val.clone())),
+            None => DecodedEntry::Failed(reported),
+        };
+        // An entry replaced (a failure another thread kept) isn't given back: the budget
+        // errs towards a fresh epoch.
         locked.bytes = locked.bytes.saturating_add(size);
-        locked.map.insert(
-            key,
-            val.clone()
-                .map(|val| Box::new(val) as Box<dyn Any + Send + Sync>),
-        );
+        locked.map.insert(key, entry);
         val
     }
 }

vendor/hayro-interpret/src/x_object.rs

@@ -578,7 +578,8 @@ impl<'a> ImageXObject<'a> {
             self.decode_key(cache, true, target_dimension),
             MAX_DECODED_IMAGE_BYTES,
             decoded_mask_size,
-            || decode_mask(self, target_dimension),
+            &self.warning_sink,
+            |sink| decode_mask(&self.reporting_to(sink), target_dimension),
         )
     }
 
@@ -598,7 +599,8 @@ impl<'a> ImageXObject<'a> {
             self.decode_key(cache, false, target_dimension),
             MAX_DECODED_IMAGE_BYTES,
             decoded_raster_size,
-            || decode_raster(self, target_dimension),
+            &self.warning_sink,
+            |sink| decode_raster(&self.reporting_to(sink), target_dimension),
         )
     }
 
@@ -649,6 +651,14 @@ impl<'a> ImageXObject<'a> {
             || dict.get::<Stream<'_>>(MASK).is_some_and(|s| uses_jpx(&s))
     }
 
+    /// PdfCraft patch (91): this image, reporting its warnings to `sink`.
+    fn reporting_to(&self, sink: &WarningSinkFn) -> Self {
+        Self {
+            warning_sink: sink.clone(),
+            ..self.clone()
+        }
+    }
+
     pub(crate) fn width(&self) -> u32 {
         self.width
     }

vendor/hayro-syntax/src/filter/jbig2.rs

@@ -28,7 +28,9 @@ pub fn set_page_jbig2_work(work: u64) {
 }
 
 /// Pays for reading `bytes` of a stream and its globals out of what the page has left, or `None`.
-/// An image that doesn't decode isn't kept, so a page drawing it again and again reads it again.
+/// An image that doesn't decode isn't always kept (not past the page's allowance, nor drawn under a
+/// transfer function: hayro-interpret patch 91), so a page drawing it again and again can read it
+/// again.
 #[cfg(feature = "std")]
 fn charge_page_reading(bytes: usize) -> Option<()> {
     let mut left = PAGE_WORK_LEFT.with(core::cell::Cell::get);