Merge pull request #2604 from dotCooCoo/fix/decode-failure-count-race

Render: an export no longer warns about images another thread could not decode

crates/engine/src/cmd/fileio/tests_raster.rs

@@ -217,6 +217,53 @@ fn an_image_opens_at_the_size_its_resolution_declares() {
     }
 }
 
+#[test]
+fn a_raster_export_says_nothing_of_images_another_thread_could_not_decode() {
+    // While another thread keeps exporting a document whose image can't be decoded (two exports at
+    // once, or the canvas drawing such a document), PNG exports of art with no image report
+    // nothing, and every export on the other thread still reports its own image.
+    use std::sync::atomic::{AtomicBool, Ordering};
+    use vectorcraft_doc::{ImageBlob, ImageObject, Node, NodeKind};
+    let mut broken = vectorcraft_doc::Document::new(40.0, 20.0);
+    broken.images.insert("broken".into(), ImageBlob::new("image/png", b"\x89PNG\r\n\x1a\n".to_vec()));
+    let im = ImageObject {
+        key: "broken".into(),
+        width: 2,
+        height: 1,
+        xf: vectorcraft_geom::Affine::scale(20.0),
+        link: None,
+        placement: Default::default(),
+    };
+    let (id, layer) = (broken.alloc_id(), broken.layers[0].id);
+    broken.insert(Some(layer), 0, Node::new(id, NodeKind::Image(Box::new(im)))).unwrap();
+    let mut s = session(400.0, 400.0, 1);
+    s.execute("shape.ellipse", &json!({"x": 20, "y": 20, "width": 360, "height": 360})).unwrap();
+    let says_so = |w: &[Value]| w.iter().any(|w| w.as_str().is_some_and(|w| w.contains("couldn't be decoded")));
+    let stop = AtomicBool::new(false);
+    let (tx, rx) = std::sync::mpsc::channel();
+    // Nothing in the scope panics, so the other thread is always told to stop.
+    let (theirs, ours) = std::thread::scope(|scope| {
+        let other = scope.spawn(|| {
+            let mut warned = vec![];
+            while !stop.load(Ordering::Relaxed) {
+                let w = encode_with_warnings(&broken, "png", &json!({})).map(|(_, w)| w.into_iter().map(Value::from).collect::<Vec<_>>());
+                warned.push(w.is_ok_and(|w| says_so(&w)));
+                let _ = tx.send(());
+            }
+            warned
+        });
+        let started = rx.recv_timeout(std::time::Duration::from_secs(60)).is_ok();
+        let ours: Vec<Option<bool>> = (0..5)
+            .map(|_| s.execute("document.export", &json!({"format": "png"})).ok().map(|r| says_so(r["warnings"].as_array().map_or(&[], |w| w))))
+            .collect();
+        stop.store(true, Ordering::Relaxed);
+        (other.join().ok(), started.then_some(ours))
+    });
+    let theirs = theirs.expect("the other thread's exports");
+    assert!(!theirs.is_empty() && theirs.iter().all(|w| *w), "the other thread's exports: {theirs:?}");
+    assert_eq!(ours.expect("the other thread started"), vec![Some(false); 5], "exports with no image");
+}
+
 #[test]
 fn a_raster_export_says_when_an_image_could_not_be_decoded() {
     // A PNG whose header reads (so it places at its size) but whose pixel data is cut short:

crates/render/src/paint.rs

@@ -56,13 +56,17 @@ pub(crate) fn set_paint(ctx: &mut RenderContext, p: &Paint, bounds: Rect, f: &Fr
 /// 6614 x 59055 px, is 1.56 GB as RGBA), past the decoder's 512 MB default. The web keeps it.
 const MAX_DECODE_ALLOC: u64 = if cfg!(target_arch = "wasm32") { 512 << 20 } else { 2 << 30 };
 
-/// Images that couldn't be decoded (and so drew nothing), counted for exports to report.
-static DECODE_FAILURES: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0);
+thread_local! {
+    /// Images that couldn't be decoded (and so drew nothing) on this thread, counted for exports to
+    /// report.
+    static DECODE_FAILURES: std::cell::Cell<u64> = const { std::cell::Cell::new(0) };
+}
 
-/// How many images have failed to decode so far (process-wide): an export compares the count
-/// before and after to say it left images out.
+/// How many images have failed to decode on this thread so far: an export compares the count
+/// before and after it draws to say it left images out. Images that other threads (another export,
+/// the canvas, thumbnails) fail to decode meanwhile are not counted.
 pub fn decode_failures() -> u64 {
-    DECODE_FAILURES.load(std::sync::atomic::Ordering::Relaxed)
+    DECODE_FAILURES.with(std::cell::Cell::get)
 }
 
 /// `bytes` decoded within [`MAX_DECODE_ALLOC`] ([`decode_within_allowance`]); a failure is counted
@@ -70,7 +74,7 @@ pub fn decode_failures() -> u64 {
 fn decode(bytes: &[u8]) -> Option<image::DynamicImage> {
     let img = decode_within_allowance(bytes).ok();
     if img.is_none() {
-        DECODE_FAILURES.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
+        DECODE_FAILURES.with(|n| n.set(n.get() + 1));
     }
     img
 }

crates/render/src/tests.rs

@@ -776,6 +776,23 @@ fn invalid_dash_pattern_strokes_solid() {
     }
 }
 
+/// An image that can't be decoded is counted on the thread that tried to decode it: an export
+/// compares [`decode_failures`] before and after it draws, so another thread's failure meanwhile is
+/// not its own.
+#[test]
+fn decode_failures_counts_this_threads_images_only() {
+    let undecodable = || paint::decode_pixmap(b"\x89PNG\r\n\x1a\n").is_none();
+    let before = decode_failures();
+    let other = std::thread::spawn(move || {
+        let start = decode_failures();
+        undecodable() && decode_failures() == start + 1
+    });
+    assert!(other.join().unwrap(), "counted on the other thread");
+    assert_eq!(decode_failures(), before, "the other thread's failure");
+    assert!(undecodable());
+    assert_eq!(decode_failures(), before + 1, "this thread's failure");
+}
+
 /// An image drawn larger than its pixels samples smoothly, or with `smooth_images` off each screen
 /// pixel takes its nearest image pixel (Pixel Preview with File Handling › Display Bitmaps as
 /// Anti-aliased Images off, #394).