A CMYK JPEG is not labelled JFIF

A CMYK JPEG came out with JFIF's APP0 segment at its head and four components
in the frame:

  APP0/JFIF  APP14/Adobe  APP2[ICC_PROFILE 1/4 … 4/4]  SOF0: 4 components

JFIF describes one- and three-component images, greyscale and YCbCr, so an APP0
segment in front of a four-component frame says two different things about the
file's colours: a reader that trusts JFIF decodes nonsense, and one that checks
refuses the file. Adobe's own CMYK JPEGs carry the APP14 marker the encoder
already writes, and no JFIF. `jpeg-encoder` writes JFIF unconditionally, so the
segment is replaced here.

The resolution JFIF carried moves to an EXIF APP1 segment -- `XResolution`,
`YResolution` and `ResolutionUnit` -- which is where `cmd::fileio::ppi` already
looks when a file has no JFIF, so nothing is lost; with no resolution asked for
the segment is simply dropped. RGB and greyscale files keep JFIF, which is what
it is for.

Found while looking at #3084, whose default RGB exports are valid here; this is
the file that is not.

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

@@ -220,6 +220,21 @@ mod tests {
         assert!(close(resolution(&jpg).unwrap(), (254.0, 240.0 * 2.54)));
     }
 
+    /// A CMYK JPEG carries no JFIF (it describes one- and three-component images), so its
+    /// resolution is written as EXIF instead and this reader still finds it (#3084).
+    #[test]
+    fn a_cmyk_jpeg_keeps_its_resolution_without_jfif() {
+        use vectorcraft_render::encode::jpeg::{self, ColorModel, JpegOptions};
+        let o = JpegOptions { color_model: ColorModel::Cmyk, ..Default::default() };
+        let jpg = jpeg::encode(&[0; 4 * 4], 2, 2, 90, Some(240.0), &o).unwrap();
+        assert!(!jpg.windows(5).any(|w| w == b"JFIF\0"), "no JFIF on a CMYK file");
+        assert_eq!(resolution(&jpg), Some((240.0, 240.0)));
+        // An RGB file still says it in JFIF, as it always did.
+        let rgb = jpeg::encode(&[0; 3 * 4], 2, 2, 90, Some(240.0), &JpegOptions::default()).unwrap();
+        assert!(rgb.windows(5).any(|w| w == b"JFIF\0"));
+        assert_eq!(resolution(&rgb), Some((240.0, 240.0)));
+    }
+
     /// A little-endian TIFF/EXIF block declaring `x`/`y` resolution in `unit`.
     fn exif_block(x: u32, y: u32, unit: u16) -> Vec<u8> {
         let mut b = b"II*\0".to_vec();

crates/render/src/encode/jpeg.rs

@@ -155,9 +155,68 @@ pub fn encode(px: &[u8], width: u32, height: u32, quality: u8, ppi: Option<f64>,
         ColorModel::Gray => ColorType::Luma,
     };
     enc.encode(px, w, h, ty).map_err(fail)?;
+    // JFIF describes one- and three-component images (greyscale and YCbCr), so its APP0 segment
+    // must not head a four-component CMYK frame: the file then says two different things about
+    // its colours, and a strict reader rejects it rather than guessing. Adobe's own CMYK JPEGs
+    // carry the APP14 marker the encoder already writes, and no JFIF (#3084).
+    if o.color_model == ColorModel::Cmyk {
+        unlabel_jfif(&mut buf, ppi);
+    }
     Ok(buf)
 }
 
+/// Replace the leading APP0/JFIF segment, which the encoder always writes, with an EXIF segment
+/// carrying the same resolution (TIFF `XResolution`, `YResolution` and `ResolutionUnit`, which is
+/// where a reader looks when there is no JFIF). Without a resolution the segment is simply
+/// dropped.
+fn unlabel_jfif(buf: &mut Vec<u8>, ppi: Option<f64>) {
+    // SOI, then APP0 with its two length bytes and the tag it is identified by.
+    let Some(head) = buf.get(..13) else { return };
+    if head[..4] != [0xFF, 0xD8, 0xFF, 0xE0] || &head[6..11] != b"JFIF\0" {
+        return;
+    }
+    let len = usize::from(u16::from_be_bytes([head[4], head[5]]));
+    if 4 + len > buf.len() {
+        return;
+    }
+    let exif = ppi.filter(|p| p.is_finite() && *p >= 1.0).map(|p| exif_resolution(p.round() as u32)).unwrap_or_default();
+    buf.splice(2..4 + len, exif);
+}
+
+/// An APP1 EXIF segment holding `dpi` as IFD0's resolution: big-endian TIFF, three entries, each
+/// rational pointing past the directory.
+fn exif_resolution(dpi: u32) -> Vec<u8> {
+    let mut tiff = Vec::new();
+    tiff.extend_from_slice(b"MM\0*"); // big-endian, TIFF magic
+    tiff.extend_from_slice(&8u32.to_be_bytes()); // IFD0 right after the header
+    tiff.extend_from_slice(&3u16.to_be_bytes()); // three entries
+    // Entry: tag, type (5 = rational, 3 = short), count, then the value or its offset.
+    let entry = |tag: u16, ty: u16, value: [u8; 4]| {
+        let mut e = Vec::with_capacity(12);
+        e.extend_from_slice(&tag.to_be_bytes());
+        e.extend_from_slice(&ty.to_be_bytes());
+        e.extend_from_slice(&1u32.to_be_bytes());
+        e.extend_from_slice(&value);
+        e
+    };
+    // The two rationals sit after the directory: header 8 + count 2 + 3 entries + next-IFD 4.
+    let after = 8 + 2 + 3 * 12 + 4;
+    tiff.extend(entry(282, 5, (after as u32).to_be_bytes())); // XResolution
+    tiff.extend(entry(283, 5, (after as u32 + 8).to_be_bytes())); // YResolution
+    tiff.extend(entry(296, 3, [0, 2, 0, 0])); // ResolutionUnit: inches
+    tiff.extend_from_slice(&0u32.to_be_bytes()); // no next IFD
+    for _ in 0..2 {
+        tiff.extend_from_slice(&dpi.to_be_bytes());
+        tiff.extend_from_slice(&1u32.to_be_bytes());
+    }
+    let mut seg = vec![0xFF, 0xE1];
+    let body_len = 6 + tiff.len() + 2;
+    seg.extend_from_slice(&u16::try_from(body_len).unwrap_or(u16::MAX).to_be_bytes());
+    seg.extend_from_slice(b"Exif\0\0");
+    seg.extend_from_slice(&tiff);
+    seg
+}
+
 /// `img` flattened on white as RGB, or as grey with each colour's lightness.
 pub(crate) fn screen_pixels(img: &Rendered, gray: bool) -> Vec<u8> {
     let px = img.to_straight();

crates/render/src/encode/tests_jpeg.rs

@@ -48,6 +48,44 @@ fn noise(w: u32, h: u32, channels: usize) -> Vec<u8> {
         .collect()
 }
 
+/// JFIF describes one- and three-component images, so its APP0 segment must not head a CMYK
+/// frame: the file would say two different things about its colours and a strict reader rejects
+/// it. A CMYK JPEG carries the Adobe APP14 marker instead, as Adobe's own do (#3084).
+#[test]
+fn a_cmyk_jpeg_is_not_labelled_jfif() {
+    let px = noise(16, 16, 4);
+    let o = JpegOptions { color_model: ColorModel::Cmyk, ..Default::default() };
+    let file = jpeg::encode(&px, 16, 16, 90, Some(300.0), &o).unwrap();
+    let markers: Vec<u8> = segments(&file).into_iter().map(|(m, _)| m).collect();
+    assert_eq!(frame(&file).1, 4, "four components");
+    assert!(!markers.contains(&0xE0), "no APP0/JFIF: {markers:02x?}");
+    assert!(markers.contains(&0xEE), "the Adobe APP14 marker names the colours: {markers:02x?}");
+    assert!(icc(&file).is_some(), "the CMYK profile is still embedded");
+    assert_eq!(&file[..2], &[0xFF, 0xD8], "the file still starts at SOI");
+    // The resolution JFIF carried moves to EXIF, where a reader looks when there is no JFIF.
+    let exif = segments(&file).into_iter().find(|(m, _)| *m == 0xE1).expect("APP1/EXIF").1;
+    assert!(exif.starts_with(b"Exif\0\0"), "{:02x?}", &exif[..8.min(exif.len())]);
+    // Big-endian TIFF with XResolution (282), YResolution (283) and ResolutionUnit (296).
+    assert_eq!(&exif[6..10], b"MM\0*");
+    // "Exif\0\0" then the TIFF header (8 bytes) and the entry count (2), so entries start at 16.
+    let tags: Vec<u16> = (0..3).map(|i| u16::from_be_bytes([exif[16 + i * 12], exif[17 + i * 12]])).collect();
+    assert_eq!(tags, [282, 283, 296], "{tags:?}");
+    // No resolution asked for: nothing stands in for the dropped segment.
+    let plain = jpeg::encode(&px, 16, 16, 90, None, &o).unwrap();
+    let markers: Vec<u8> = segments(&plain).into_iter().map(|(m, _)| m).collect();
+    assert!(!markers.contains(&0xE0) && !markers.contains(&0xE1), "{markers:02x?}");
+
+    // Three- and one-component files keep JFIF, which is what it is for, and their resolution.
+    for (model, channels) in [(ColorModel::Rgb, 3), (ColorModel::Gray, 1)] {
+        let px = noise(16, 16, channels);
+        let o = JpegOptions { color_model: model, ..Default::default() };
+        let file = jpeg::encode(&px, 16, 16, 90, Some(300.0), &o).unwrap();
+        let markers: Vec<u8> = segments(&file).into_iter().map(|(m, _)| m).collect();
+        assert!(markers.contains(&0xE0), "{model:?} keeps JFIF: {markers:02x?}");
+        assert_eq!(frame(&file).1, channels as u8);
+    }
+}
+
 #[test]
 fn size_falls_with_quality() {
     let px = noise(64, 48, 3);
@@ -85,9 +123,18 @@ fn colour_models_write_their_components_and_profiles() {
         assert_eq!(frame(&file).1, comps, "{model:?}");
         let p = IccProfile::from_bytes(None, &icc(&file).expect("APP2 ICC")).unwrap();
         assert_eq!(p.kind, kind, "{model:?}");
-        let app0 = segments(&file).into_iter().find(|(m, _)| *m == 0xE0).unwrap().1;
-        assert_eq!(&app0[..5], b"JFIF\0");
-        assert_eq!(app0[7..12], [1, 0, 150, 0, 150], "dots per inch");
+        // JFIF describes one- and three-component images, so a CMYK file says its resolution in
+        // EXIF instead (see `a_cmyk_jpeg_is_not_labelled_jfif`).
+        if model == ColorModel::Cmyk {
+            let exif = segments(&file).into_iter().find(|(m, _)| *m == 0xE1).expect("APP1/EXIF").1;
+            assert!(exif.starts_with(b"Exif\0\0"));
+            // "Exif\0\0" (6) + TIFF header (8) + count (2) + three entries (36) + next IFD (4).
+            assert_eq!(&exif[56..64], &[0, 0, 0, 150, 0, 0, 0, 1], "150 dots per inch, as a rational");
+        } else {
+            let app0 = segments(&file).into_iter().find(|(m, _)| *m == 0xE0).unwrap().1;
+            assert_eq!(&app0[..5], b"JFIF\0");
+            assert_eq!(app0[7..12], [1, 0, 150, 0, 150], "dots per inch");
+        }
         let none = jpeg::encode(&noise(16, 8, model.channels()), 16, 8, 90, None, &JpegOptions { embed_icc: false, ..o }).unwrap();
         assert!(icc(&none).is_none());
     }