Place: an unusable EXIF resolution falls back to the JFIF density

A JPEG whose EXIF resolution is 0 ppi or outside 1 to 100,000 ppi now
takes its size from its JFIF density instead of one point per pixel.

Tests place an 11 x 17 in image (330 x 510 px at 30 ppi) with a click,
check that it comes in at 792 x 1224 pt and keeps that size through a
save and reopen, and that an image with no stored resolution comes in
at 72 ppi.

crates/engine/src/cmd/file/place.rs

@@ -589,10 +589,8 @@ pub(super) mod image_place_tests {
         }
     }
 
-    #[test]
-    fn placed_photo_honors_its_embedded_resolution() {
-        // #1991: a placed graphic's embedded density sets its size (600 dpi: 40 px place
-        // 4.8 pt wide), instead of a pixel always becoming a point.
+    /// A `width` x `height` PNG with a pHYs chunk of `ppm` pixels per meter on both axes, or none.
+    fn png_with_density(width: u32, height: u32, ppm: Option<u32>) -> std::io::Result<tempfile::NamedTempFile> {
         fn crc32(data: &[u8]) -> u32 {
             let mut crc = 0xFFFF_FFFFu32;
             for &b in data {
@@ -603,22 +601,33 @@ pub(super) mod image_place_tests {
             }
             !crc
         }
-        let png = designcraft_render::Rendered { width: 40, height: 20, pixels: [30, 90, 180, 255].repeat(800) }.to_png();
+        let pixels = [30, 90, 180, 255].repeat((width * height) as usize);
+        let png = designcraft_render::Rendered { width, height, pixels }.to_png();
         assert_eq!(&png[12..16], b"IHDR");
-        let mut phys = Vec::new();
-        phys.extend_from_slice(&23622u32.to_be_bytes());
-        phys.extend_from_slice(&23622u32.to_be_bytes());
-        phys.push(1);
         let mut bytes = png[..33].to_vec();
-        bytes.extend_from_slice(&(phys.len() as u32).to_be_bytes());
-        bytes.extend_from_slice(b"pHYs");
-        bytes.extend_from_slice(&phys);
-        let mut c = b"pHYs".to_vec();
-        c.extend_from_slice(&phys);
-        bytes.extend_from_slice(&crc32(&c).to_be_bytes());
+        if let Some(ppm) = ppm {
+            let mut phys = Vec::new();
+            phys.extend_from_slice(&ppm.to_be_bytes());
+            phys.extend_from_slice(&ppm.to_be_bytes());
+            phys.push(1);
+            bytes.extend_from_slice(&(phys.len() as u32).to_be_bytes());
+            bytes.extend_from_slice(b"pHYs");
+            bytes.extend_from_slice(&phys);
+            let mut c = b"pHYs".to_vec();
+            c.extend_from_slice(&phys);
+            bytes.extend_from_slice(&crc32(&c).to_be_bytes());
+        }
         bytes.extend_from_slice(&png[33..]);
-        let file = tempfile::Builder::new().suffix(".png").tempfile().unwrap();
-        std::fs::write(file.path(), bytes).unwrap();
+        let file = tempfile::Builder::new().suffix(".png").tempfile()?;
+        std::fs::write(file.path(), bytes)?;
+        Ok(file)
+    }
+
+    #[test]
+    fn placed_photo_honors_its_embedded_resolution() {
+        // #1991: a placed graphic's embedded density sets its size (600 dpi: 40 px place
+        // 4.8 pt wide), instead of a pixel always becoming a point.
+        let file = png_with_density(40, 20, Some(23622)).unwrap();
         let mut session = Session::new();
         session.execute("file.new", &json!({})).unwrap();
         let placed = session.execute("file.place", &json!({"path": file.path(), "x": 10.0, "y": 20.0})).unwrap();
@@ -629,6 +638,47 @@ pub(super) mod image_place_tests {
         assert!((b.width() - 4.8).abs() < 0.01 && (b.height() - 2.4).abs() < 0.01, "{b:?}");
     }
 
+    /// File › Place, then a click: an 11 x 17 in image (330 x 510 px at 30 ppi) comes in at
+    /// 792 x 1224 pt, keeps that size through a save and reopen, and an image with no stored
+    /// resolution comes in at 72 ppi (330 x 510 pt).
+    #[test]
+    fn a_clicked_image_keeps_its_print_size_through_save_and_reopen() {
+        // 1181 pixels per meter is 29.9974 ppi.
+        let (w, h) = (330.0 * 72.0 / (1181.0 * 0.0254), 510.0 * 72.0 / (1181.0 * 0.0254));
+        for (ppm, (ew, eh)) in [(Some(1181), (w, h)), (None, (330.0, 510.0))] {
+            let image = png_with_density(330, 510, ppm).unwrap();
+            let mut session = Session::new();
+            session.execute("file.new", &json!({})).unwrap();
+            session.execute("file.place", &json!({"path": image.path(), "loadCursor": true})).unwrap();
+            session.execute("place.drop", &json!({"x": 0, "y": 0})).unwrap();
+            // The bounds and natural size of the document's one placed graphic.
+            let shown = |session: &Session| {
+                let doc = &session.doc().unwrap().doc;
+                let graphics: Vec<_> = doc
+                    .all_items()
+                    .into_iter()
+                    .filter_map(|id| doc.item(id))
+                    .filter_map(|item| match &item.content {
+                        Content::Graphic(g) => Some((item.bounds(), g.size)),
+                        _ => None,
+                    })
+                    .collect();
+                assert_eq!(graphics.len(), 1, "{graphics:?}");
+                graphics[0]
+            };
+            let (b, size) = shown(&session);
+            assert!((b.width() - ew).abs() < 1e-6 && (b.height() - eh).abs() < 1e-6, "{ppm:?}: {b:?}");
+            assert!((size.0 - ew).abs() < 1e-6 && (size.1 - eh).abs() < 1e-6, "{ppm:?}: {size:?}");
+
+            let dir = tempfile::tempdir().unwrap();
+            let path = dir.path().join("placed.designcraft");
+            session.execute("file.save", &json!({"path": path.to_string_lossy()})).unwrap();
+            let mut reopened = Session::new();
+            reopened.execute("file.open", &json!({"path": path.to_string_lossy()})).unwrap();
+            assert_eq!(shown(&reopened), (b, size), "{ppm:?}");
+        }
+    }
+
     #[test]
     fn explicit_frame_wins_over_placement_and_selection() {
         let image = image_file().unwrap();

crates/images/src/lib.rs

@@ -374,8 +374,8 @@ pub fn natural_size(bytes: &[u8]) -> Option<(f64, f64)> {
         return Some((bb[2] - bb[0], bb[3] - bb[1])).filter(|(w, h)| w.is_finite() && h.is_finite() && *w > 0.0 && *h > 0.0);
     }
     let (w, h) = pixel_size(bytes)?;
-    // Honor the file's density (#1991): a 600 px photo saved at 600 dpi places one inch wide,
-    // as in InDesign. Absurd metadata falls back to a pixel per point.
+    // Honor the file's density (#1991): a 600 px photo saved at 600 dpi places one inch wide.
+    // Absurd metadata falls back to a pixel per point.
     match raster_resolution(bytes) {
         Some(dpi) if resolution::sane_resolution(dpi) => Some((w as f64 * 72.0 / dpi.0, h as f64 * 72.0 / dpi.1)),
         _ => Some((w as f64, h as f64)),

crates/images/src/resolution.rs

@@ -1,6 +1,6 @@
 //! Embedded raster resolution: the density a placed graphic was saved with, so placing honors
-//! it instead of assuming 72 ppi (#1991: a 600 px photo saved at 600 dpi places one inch wide,
-//! as in InDesign). Reads JFIF/EXIF from JPEG (and EXIF from WebP), pHYs from PNG, the resolution
+//! it instead of assuming 72 ppi (#1991: a 600 px photo saved at 600 dpi places one inch wide).
+//! Reads JFIF/EXIF from JPEG (and EXIF from WebP), pHYs from PNG, the resolution
 //! tags from TIFF, and the header density from BMP. GIF, PSD and files without (usable) metadata
 //! report nothing and keep the pixel-is-a-point size.
 
@@ -78,10 +78,10 @@ fn jpeg_resolution(bytes: &[u8]) -> Option<(f64, f64)> {
         if m == 0xE0 && jfif.is_none() {
             jfif = jfif_density(payload);
         } else if m == 0xE1
-            && let Some(dpi) = exif_resolution(payload)
+            && let Some(dpi) = exif_resolution(payload).filter(|dpi| sane_resolution(*dpi))
         {
-            // A non-EXIF APP1 (XMP, ICC, ...) parses to nothing: keep scanning, since a
-            // later APP1 can still carry the EXIF resolution.
+            // A non-EXIF APP1 (XMP, ICC, ...) or an unusable EXIF density parses to nothing:
+            // keep scanning, so a later APP1 or the JFIF density still sets the size.
             return Some(dpi);
         }
         pos += len;

crates/images/src/tests.rs

@@ -537,6 +537,19 @@ fn jpeg_exif_resolution_beats_jfif() {
     assert_eq!(raster_resolution(&zero), None);
 }
 
+/// An EXIF density of 0 ppi (or any value outside the usable range) does not hide the JFIF
+/// density: the 600x900 px photo at a JFIF 300 dpi still places 144 x 216 pt.
+#[test]
+fn unusable_exif_density_keeps_the_jfif_density() {
+    for exif in [(0, 1), (1, 4), (1_000_000, 1)] {
+        let mut app = jfif_app(1, 300, 300);
+        app.extend_from_slice(&exif_app(&tiff_bytes(exif, exif, Some(2))));
+        let jpg = jpeg_with(&app);
+        assert_eq!(raster_resolution(&jpg), Some((300.0, 300.0)), "EXIF {exif:?}");
+        assert_eq!(natural_size(&jpg), Some((144.0, 216.0)), "EXIF {exif:?}");
+    }
+}
+
 /// Big-endian EXIF parses too.
 #[test]
 fn jpeg_exif_big_endian_parses() {

crates/render/src/decode.rs

@@ -87,7 +87,7 @@ pub fn image_size(bytes: &[u8]) -> Option<(u32, u32)> {
 }
 
 /// Natural size of a placed graphic, unrounded: a PDF page (crop box) and an SVG in points, a raster
-/// in pixels, an EPS its bounding box independently of preview pixels. Placed graphics are drawn at
+/// at its stored resolution (72 ppi without one), an EPS its bounding box independently of preview pixels. Placed graphics are drawn at
 /// this size; `image_size` rounds it to whole pixels for display.
 pub fn natural_size(bytes: &[u8], page: usize) -> Option<(f64, f64)> {
     if is_pdf(bytes) {