From 0a7e66517f38994f4e6cb4abe018b9ef8a91f3cb Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 00:36:01 -0400 Subject: [PATCH 01/14] feat(png): preserve metadata across a re-encode, and refuse the chunk pairs the spec forbids MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `PngEncoder::with_metadata` / `with_metadata_from` carry a decoded file's eXIf, iCCP, XMP, text and colour chunks into the encoder that rewrites its pixels, so a re-encode no longer drops every one of them. Two defects the spec settles are fixed on the way: * sRGB beside iCCP. PNG 3rd ed. §5.6 Table 5 states the constraint on both rows, and §11.3.2.5 repeats it: the two should not appear together. Both were written whenever both were set. The encode is now refused with `InvalidInput`, and `with_metadata` resolves the pair by §4.3 Table 1's colour-chunk priority (iCCP 2 outranks sRGB 3) so a file carrying both is still re-encodable. * tEXt/zTXt carried UTF-8. §11.3.3.2 interprets a tEXt text string as Latin-1 and §11.3.3.3 says an inflated zTXt is identical to it, while §11.3.3.1 restricts every keyword to Latin-1. Pushing a Rust `String`'s bytes stored mojibake for every code point above U+007F. Text is now converted once, at the setter, and a non-Latin-1 text is promoted to iTXt as §11.3.3.2 directs; a keyword no chunk can carry refuses the encode. Adds `with_cicp` (§11.3.2.6), without which preservation would silently drop the highest-precedence colour chunk a file carries. --- crates/gamut-png/src/ancillary.rs | 187 ++++++++++++++++++++++++---- crates/gamut-png/src/decoder.rs | 18 ++- crates/gamut-png/src/encoder.rs | 182 ++++++++++++++++++++++++++- crates/gamut-png/tests/c2pa.rs | 7 +- crates/gamut-png/tests/metadata.rs | 58 +++++++-- crates/gamut-png/tests/roundtrip.rs | 5 +- 6 files changed, 410 insertions(+), 47 deletions(-) diff --git a/crates/gamut-png/src/ancillary.rs b/crates/gamut-png/src/ancillary.rs index da7a0cd6..b3b1815d 100644 --- a/crates/gamut-png/src/ancillary.rs +++ b/crates/gamut-png/src/ancillary.rs @@ -34,6 +34,7 @@ //! sample inside the written range keeps its input-depth value. That is issue #501, not this //! module's claim. +use gamut_core::{Error, Result}; use gamut_deflate::{DeflateEncoder, Level}; use crate::{ColorType, chunk}; @@ -101,15 +102,45 @@ enum TextKind { Compressed, /// `iTXt`: uncompressed UTF-8. International, + /// `iTXt` with the compression flag set: zlib-compressed UTF-8. + InternationalCompressed, } +/// One accumulated text annotation, already **in the byte form its chunk carries**. +/// +/// The distinction is the whole point of holding bytes rather than `String`s. PNG's three text +/// chunks do not share a character set: §11.3.3.1 restricts a keyword to Latin-1 +/// ([ISO_8859-1]) in *every* one of them, §11.3.3.2 says a `tEXt` text string "is interpreted +/// according to the Latin-1 character set" (and §11.3.3.3 that inflating a `zTXt` "yields +/// Latin-1 text that is identical to the text that would be stored in an equivalent `tEXt` +/// chunk"), while §11.3.3.4 gives `iTXt` UTF-8. A Rust `String` is UTF-8, so writing its bytes +/// into a `tEXt` chunk stores mojibake for every code point above U+007F — `é` (U+00E9) becomes +/// the two bytes `C3 A9`, which a conforming reader shows as `é`. Converting once, at the point +/// the caller sets the text, makes that unrepresentable: an entry exists only if its bytes are +/// already right for its `kind`. #[derive(Debug, Clone)] struct TextEntry { - keyword: String, - text: String, + /// The keyword, Latin-1 (§11.3.3.1). + keyword: Vec, + /// The text: Latin-1 for `tEXt`/`zTXt`, UTF-8 for `iTXt`. + text: Vec, + /// The `iTXt` language tag (§11.3.3.4, BCP 47); empty for the other kinds and for an + /// unspecified language. + language: Vec, + /// The `iTXt` translated keyword (UTF-8, §11.3.3.4); empty for the other kinds. + translated: Vec, kind: TextKind, } +/// The Latin-1 bytes of `s`, or `None` when a character has no Latin-1 encoding. +/// +/// Latin-1 is the first 256 Unicode code points, so the encoding is `u8::try_from` on each +/// `char` — the exact inverse of the decoder's `latin1`, which maps byte *n* to U+00*nn*. A +/// string that came out of this crate's decoder therefore always converts back. +fn latin1_bytes(s: &str) -> Option> { + s.chars().map(|c| u8::try_from(u32::from(c)).ok()).collect() +} + /// Accumulated ancillary metadata to emit alongside the image. #[derive(Debug, Clone, Default)] pub(crate) struct Ancillary { @@ -119,6 +150,9 @@ pub(crate) struct Ancillary { pub chrm: Option<[u32; 8]>, /// sRGB: rendering-intent code. pub srgb: Option, + /// cICP: (colour primaries, transfer function, video full-range flag). The matrix + /// coefficients byte is not carried because §11.3.2.6 fixes it at 0 for PNG. + pub cicp: Option<(u8, u8, bool)>, /// sBIT: significant bits per channel (1–4 values, matching the colour type). pub sbit: Option>, /// bKGD: background colour, pre-serialised to its colour-type-specific bytes. @@ -136,6 +170,13 @@ pub(crate) struct Ancillary { pub c2pa: Option>, /// tEXt / zTXt / iTXt entries, emitted in insertion order. texts: Vec, + /// Whether a caller set a text annotation whose **keyword** has no Latin-1 encoding. + /// + /// §11.3.3.1 restricts a keyword to Latin-1 in all three text chunks, so — unlike the text, + /// which `iTXt` carries in UTF-8 — there is no chunk such a keyword fits. The entry is + /// dropped at the setter and the encode is refused by [`Self::validate`], rather than + /// silently writing a keyword no reader can match. + unencodable_keyword: bool, } impl Ancillary { @@ -164,18 +205,108 @@ impl Ancillary { self.push_text(keyword, text, TextKind::International); } + /// Adds an `iTXt` entry keeping its language tag and translated keyword (§11.3.3.4), which + /// [`add_text_international`](Self::add_text_international) leaves empty. Used only to carry + /// a decoded annotation forward, so that re-encoding a file does not silently drop the two + /// fields that make `iTXt` international. + pub(crate) fn add_text_international_tagged( + &mut self, + keyword: &str, + language: &str, + translated: &str, + text: &str, + ) { + if let Some(mut entry) = self.text_entry(keyword, text, TextKind::International) { + entry.language = language.as_bytes().to_vec(); + entry.translated = translated.as_bytes().to_vec(); + self.texts.push(entry); + } + } + fn push_text(&mut self, keyword: &str, text: &str, kind: TextKind) { - self.texts.push(TextEntry { - keyword: keyword.to_string(), - text: text.to_string(), + if let Some(entry) = self.text_entry(keyword, text, kind) { + self.texts.push(entry); + } + } + + /// Builds the entry for one text annotation, choosing the chunk that can actually carry it. + /// + /// The caller's `kind` is a *preference*, not a guarantee: §11.3.3.2 says outright that "text + /// containing characters outside the repertoire of ISO/IEC 8859-1 should be encoded using the + /// `iTXt` chunk", so a `tEXt`/`zTXt` request whose text is not Latin-1 is promoted to `iTXt` + /// rather than written as UTF-8 bytes a Latin-1 reader mis-renders. The promotion keeps the + /// caller's *other* choice — compression — because §11.3.3.4 gives `iTXt` a compression flag + /// of its own; only the character set changes. + /// + /// `None` (the entry is dropped, and [`Self::validate`] then refuses the encode) is reserved + /// for the one case no chunk can express: a keyword outside Latin-1. + fn text_entry(&mut self, keyword: &str, text: &str, kind: TextKind) -> Option { + let Some(keyword) = latin1_bytes(keyword) else { + self.unencodable_keyword = true; + return None; + }; + let (kind, text) = match (kind, latin1_bytes(text)) { + (TextKind::Latin1, Some(latin1)) => (TextKind::Latin1, latin1), + (TextKind::Compressed, Some(latin1)) => (TextKind::Compressed, latin1), + (TextKind::Latin1, None) => (TextKind::International, text.as_bytes().to_vec()), + (TextKind::Compressed, None) => { + (TextKind::InternationalCompressed, text.as_bytes().to_vec()) + } + (kind, _) => (kind, text.as_bytes().to_vec()), + }; + Some(TextEntry { + keyword, + text, + language: Vec::new(), + translated: Vec::new(), kind, - }); + }) + } + + /// Refuses an accumulation the spec says must not be written, before any byte is emitted. + /// + /// Two cases, both of which the caller stated explicitly and neither of which this encoder + /// may silently resolve for it: + /// + /// - **`sRGB` together with `iCCP`.** §5.6 Table 5 records the constraint on both rows — "if + /// the `iCCP` chunk is present, the `sRGB` chunk should not be present" and its converse — + /// and §11.3.2.5 repeats it ("it is recommended that the `sRGB` and `iCCP` chunks do not + /// appear simultaneously in a PNG datastream"). Emitting both is not undefined, because + /// §4.3 Table 1 ranks the colour chunks and a reader takes the lowest priority number + /// (`iCCP` 2 over `sRGB` 3) — but it *is* a datastream the standard tells encoders not to + /// produce, and which of the two the caller meant is not something this crate can guess. + /// Dropping one silently would lose colour information the caller supplied, so the encode + /// is refused. To carry both forward from a decoded file, use + /// [`PngEncoder::with_metadata`](crate::PngEncoder::with_metadata), which applies Table 1 + /// itself. + /// - **A text keyword outside Latin-1** (§11.3.3.1), which no text chunk can carry. + pub(crate) fn validate(&self) -> Result<()> { + if self.srgb.is_some() && self.iccp.is_some() { + return Err(Error::invalid_input( + env!("CARGO_PKG_NAME"), + "PNG: sRGB and iCCP must not both be written (spec §5.6 Table 5, §11.3.2.5); \ + set one", + )); + } + if self.unencodable_keyword { + return Err(Error::invalid_input( + env!("CARGO_PKG_NAME"), + "PNG: a text keyword must be Latin-1 (spec §11.3.3.1)", + )); + } + Ok(()) } /// Emits the colour-space chunks that must precede `PLTE` (PNG Table 7). `effort` is the /// encoder's [`Level::Best`] budget, applied to the compressed `iCCP` payload; `written` is /// the IHDR these chunks sit under, which `sBIT` must agree with. pub(crate) fn write_pre_plte(&self, out: &mut Vec, effort: u8, written: WrittenHeader<'_>) { + if let Some((primaries, transfer, full_range)) = self.cicp { + // §11.3.2.6 Table 18: primaries, transfer function, matrix coefficients, full-range + // flag — one byte each, the matrix fixed at 0 because "RGB is currently the only + // supported color model in PNG, and as such Matrix Coefficients shall be set to 0". + chunk::write_chunk(out, *b"cICP", &[primaries, transfer, 0, u8::from(full_range)]); + } if let Some(chrm) = self.chrm { let mut data = [0u8; 32]; for (slot, value) in chrm.iter().enumerate() { @@ -437,32 +568,42 @@ pub(crate) fn sbit_for(sbit: &[u8], color: ColorType, bit_depth: u8) -> Option, entry: &TextEntry, effort: u8) { + let compress = |payload: &[u8], data: &mut Vec| { + DeflateEncoder::new() + .with_level(Level::Best) + .with_effort(effort) + .zlib_compress(payload, data); + }; + let mut data = entry.keyword.clone(); + data.push(0); // null separator match entry.kind { TextKind::Latin1 => { - let mut data = entry.keyword.clone().into_bytes(); - data.push(0); // null separator - data.extend_from_slice(entry.text.as_bytes()); + data.extend_from_slice(&entry.text); chunk::write_chunk(out, *b"tEXt", &data); } TextKind::Compressed => { - let mut data = entry.keyword.clone().into_bytes(); - data.push(0); // null separator data.push(0); // compression method: 0 = zlib/deflate - DeflateEncoder::new() - .with_level(Level::Best) - .with_effort(effort) - .zlib_compress(entry.text.as_bytes(), &mut data); + compress(&entry.text, &mut data); chunk::write_chunk(out, *b"zTXt", &data); } - TextKind::International => { - let mut data = entry.keyword.clone().into_bytes(); - data.push(0); // null separator - data.push(0); // compression flag: 0 = uncompressed - data.push(0); // compression method - data.push(0); // empty language tag, then null - data.push(0); // empty translated keyword, then null - data.extend_from_slice(entry.text.as_bytes()); // UTF-8 text + TextKind::International | TextKind::InternationalCompressed => { + let compressed = entry.kind == TextKind::InternationalCompressed; + data.push(u8::from(compressed)); // compression flag + data.push(0); // compression method: 0 = zlib/deflate + data.extend_from_slice(&entry.language); + data.push(0); // language tag terminator + data.extend_from_slice(&entry.translated); + data.push(0); // translated keyword terminator + if compressed { + compress(&entry.text, &mut data); + } else { + data.extend_from_slice(&entry.text); + } chunk::write_chunk(out, *b"iTXt", &data); } } diff --git a/crates/gamut-png/src/decoder.rs b/crates/gamut-png/src/decoder.rs index 8a595eb4..562b4641 100644 --- a/crates/gamut-png/src/decoder.rs +++ b/crates/gamut-png/src/decoder.rs @@ -1636,7 +1636,6 @@ mod tests { #[test] fn rich_decode_surfaces_metadata_and_native_image() { - use crate::SrgbIntent; use crate::decoded::PngImage; let (w, h) = (6u32, 4u32); @@ -1647,7 +1646,9 @@ mod tests { let mut png = Vec::new(); PngEncoder::new() .with_gamma(1.0 / 2.2) - .with_srgb(SrgbIntent::Perceptual) + // cICP, not sRGB: the encoder refuses sRGB beside the iCCP this fixture needs + // (§5.6 Table 5, §11.3.2.5), while cICP is legal alongside it (§4.3 Table 1). + .with_cicp(9, 16, true) .with_chromaticities((0.3127, 0.3290), (0.64, 0.33), (0.30, 0.60), (0.15, 0.06)) .with_exif(&exif) .with_icc_profile("prof", b"not-a-real-profile-but-bytes") @@ -1670,7 +1671,7 @@ mod tests { other => panic!("expected Rgb8, got {other:?}"), } assert_eq!(decoded.gamma, Some(45455)); - assert_eq!(decoded.srgb, Some(SrgbIntent::Perceptual)); + assert!(decoded.srgb.is_none()); let chrm = decoded.chromaticities.unwrap(); assert_eq!(chrm.white, (31270, 32900)); assert_eq!(chrm.blue, (15000, 6000)); @@ -1690,7 +1691,16 @@ mod tests { assert_eq!(decoded.texts[1].text, comment); assert!(decoded.palette.is_none()); assert!(decoded.transparency.is_none()); - assert!(decoded.cicp.is_none()); + let cicp = decoded.cicp.expect("cICP present"); + assert_eq!( + ( + cicp.color_primaries, + cicp.transfer_function, + cicp.matrix_coefficients, + cicp.full_range + ), + (9, 16, 0, true) + ); } #[test] diff --git a/crates/gamut-png/src/encoder.rs b/crates/gamut-png/src/encoder.rs index b855978b..fd261996 100644 --- a/crates/gamut-png/src/encoder.rs +++ b/crates/gamut-png/src/encoder.rs @@ -31,6 +31,7 @@ use crate::ancillary::{ use crate::backend::{IdatDeflater, IdatInfo, Registry, run_deflaters}; use crate::chunk::{self, C2paSpan, SIGNATURE}; use crate::color::ColorType; +use crate::decoded::{Chromaticities, Cicp, DecodedPng, IccProfile, PngMetadata, TextChunk}; use crate::filter::{self, FilterStrategy, FilterType}; use crate::palette::PngPalette; use crate::reduce::{self, Reduced, Reductions}; @@ -74,6 +75,23 @@ pub struct PngEncodeReport { pub c2pa: Option, } +/// The metadata fields [`PngMetadata`] and [`DecodedPng`] both carry, borrowed. +/// +/// The two read surfaces agree field for field on purpose (one reads the pixels, one does not), +/// so [`PngEncoder::with_metadata`] and [`PngEncoder::with_metadata_from`] are the same function +/// over two shapes. Borrowing rather than cloning into a `PngMetadata` keeps a large ICC profile +/// or EXIF block from being copied twice on the way into the encoder. +struct MetadataView<'a> { + exif: Option<&'a [u8]>, + icc_profile: Option<&'a IccProfile>, + xmp: Option<&'a [u8]>, + texts: &'a [TextChunk], + gamma: Option, + chromaticities: Option, + srgb: Option, + cicp: Option, +} + /// A reusable PNG encoder. #[derive(Debug, Clone)] pub struct PngEncoder { @@ -208,12 +226,39 @@ impl PngEncoder { } /// Records the standard colour-space rendering intent (sRGB chunk). + /// + /// Mutually exclusive with [`with_icc_profile`](Self::with_icc_profile): PNG §5.6 Table 5 and + /// §11.3.2.5 both say the two chunks should not appear together, so setting both makes the + /// encode fail with [`Error::InvalidInput`] rather than write a file the standard tells + /// encoders not to produce. [`with_metadata`](Self::with_metadata) resolves the pair for you. #[must_use] pub fn with_srgb(mut self, intent: SrgbIntent) -> Self { self.ancillary.set_srgb(intent); self } + /// Records the video-signal colour space by its ITU-T H.273 code points (cICP chunk, + /// §11.3.2.6): the colour primaries, the transfer function, and whether the samples use the + /// full value range. + /// + /// There is no matrix-coefficients parameter because §11.3.2.6 fixes it: "RGB is currently + /// the only supported color model in PNG, and as such Matrix Coefficients shall be set to 0." + /// + /// cICP is the **highest-precedence** colour chunk (§4.3 Table 1, priority 1), so a reader + /// that understands it ignores any `iCCP`, `sRGB`, `gAMA` and `cHRM` in the same file. Those + /// stay legal alongside it — unlike the `sRGB`/`iCCP` pair — and are worth keeping as a + /// fallback for readers that do not. + #[must_use] + pub fn with_cicp( + mut self, + color_primaries: u8, + transfer_function: u8, + full_range: bool, + ) -> Self { + self.ancillary.cicp = Some((color_primaries, transfer_function, full_range)); + self + } + /// Records the white point and RGB primary chromaticities (cHRM chunk), each as `(x, y)`. #[must_use] pub fn with_chromaticities( @@ -351,8 +396,12 @@ impl PngEncoder { } /// Embeds an ICC colour profile (iCCP chunk), zlib-compressed. `profile` is the raw ICC profile - /// — for example the bytes produced by `gamut-icc`. (Mutually exclusive with [`Self::with_srgb`] - /// per the spec; set only one.) + /// — for example the bytes produced by `gamut-icc`. + /// + /// Mutually exclusive with [`with_srgb`](Self::with_srgb): PNG §5.6 Table 5 and §11.3.2.5 both + /// say the two chunks should not appear together, so setting both makes the encode fail with + /// [`Error::InvalidInput`] rather than write a file the standard tells encoders not to + /// produce. [`with_metadata`](Self::with_metadata) resolves the pair for you. #[must_use] pub fn with_icc_profile(mut self, name: &str, profile: &[u8]) -> Self { self.ancillary.iccp = Some((name.to_string(), profile.to_vec())); @@ -368,6 +417,131 @@ impl PngEncoder { self } + /// Carries every metadata chunk a [`PngMetadata`] holds into this encoder, so that + /// re-encoding a file keeps its EXIF, ICC profile, XMP packet, text annotations and colour + /// chunks instead of dropping them. + /// + /// This is the write-side counterpart of [`metadata`](crate::metadata): read a file's + /// metadata without touching its pixels, then hand it to the encoder that rewrites them. + /// [`with_metadata_from`](Self::with_metadata_from) is the same thing for a full + /// [`DecodedPng`]. + /// + /// # What it carries, and what it deliberately does not + /// + /// Everything the read side surfaces is set, with three spec-driven adjustments: + /// + /// - **`iCCP` and `sRGB` are resolved, not both written.** §4.3 Table 1 ranks the colour + /// chunks and a reader takes the lowest priority number, so the ICC profile (priority 2) + /// wins over the rendering intent (priority 3) and the `sRGB` chunk is dropped — which is + /// exactly the chunk a conforming reader would have ignored. Writing both is refused (§5.6 + /// Table 5, §11.3.2.5); this method is how a file carrying both is re-encoded at all. + /// - **A `cICP` whose matrix coefficients are not 0 is dropped.** §11.3.2.6 requires 0 for + /// PNG, so such a chunk is not conforming and copying it forward would reproduce the defect. + /// - **The C2PA manifest store is never carried.** A store is signed over the exact bytes of + /// the file it was made for, so copying it into a re-encode invalidates it by construction + /// — which is why `caBX` is *unsafe to copy* (C2PA 2.4 §A.3.2). Re-sign the output and set + /// it with [`with_c2pa`](Self::with_c2pa). + /// + /// Two further limits are the read side's, not this method's: `pHYs`, `tIME`, `sBIT` and + /// `bKGD` are not part of [`PngMetadata`], so they cannot be carried here (set them with + /// their own builder methods); and a `zTXt` is indistinguishable from a `tEXt` once decoded, + /// so a compressed annotation is rewritten uncompressed. Neither loses any text. + #[must_use] + pub fn with_metadata(self, metadata: &PngMetadata) -> Self { + self.with_metadata_view(MetadataView { + exif: metadata.exif.as_deref(), + icc_profile: metadata.icc_profile.as_ref(), + xmp: metadata.xmp.as_deref(), + texts: &metadata.texts, + gamma: metadata.gamma, + chromaticities: metadata.chromaticities, + srgb: metadata.srgb, + cicp: metadata.cicp, + }) + } + + /// Carries the metadata of a decoded file into this encoder: the [`DecodedPng`] twin of + /// [`with_metadata`](Self::with_metadata), which documents exactly what is and is not carried. + /// + /// Use this when you already decoded the pixels; use `with_metadata` when + /// [`metadata`](crate::metadata) read the file without them. + #[must_use] + pub fn with_metadata_from(self, decoded: &DecodedPng) -> Self { + self.with_metadata_view(MetadataView { + exif: decoded.exif.as_deref(), + icc_profile: decoded.icc_profile.as_ref(), + xmp: decoded.xmp.as_deref(), + texts: &decoded.texts, + gamma: decoded.gamma, + chromaticities: decoded.chromaticities, + srgb: decoded.srgb, + cicp: decoded.cicp, + }) + } + + /// The one implementation behind [`with_metadata`](Self::with_metadata) and + /// [`with_metadata_from`](Self::with_metadata_from). + fn with_metadata_view(mut self, meta: MetadataView<'_>) -> Self { + if let Some(exif) = meta.exif { + self = self.with_exif(exif); + } + // §4.3 Table 1: the reader honours the lowest priority number, iCCP (2) over sRGB (3). + // Writing both is what `Ancillary::validate` refuses, so pick the one that would have + // been honoured rather than hand the caller an error it cannot act on. + match (meta.icc_profile, meta.srgb) { + (Some(icc), _) => self = self.with_icc_profile(&icc.name, &icc.profile), + (None, Some(intent)) => self = self.with_srgb(intent), + (None, None) => {} + } + // §11.3.2.6: "Matrix Coefficients shall be set to 0". A source chunk that says otherwise + // is not a conforming cICP; carrying it forward would put the same defect in the output. + if let Some(cicp) = meta.cicp.filter(|cicp| cicp.matrix_coefficients == 0) { + self = self.with_cicp( + cicp.color_primaries, + cicp.transfer_function, + cicp.full_range, + ); + } + // Set in the stored ×100 000 fixed-point units rather than through `with_gamma` / + // `with_chromaticities`, whose `f64` arguments would round-trip the value through a + // division and a `round()`: preservation must be byte-exact. + if let Some(gamma) = meta.gamma { + self.ancillary.gamma = Some(gamma); + } + if let Some(chrm) = meta.chromaticities { + self.ancillary.chrm = Some([ + chrm.white.0, + chrm.white.1, + chrm.red.0, + chrm.red.1, + chrm.green.0, + chrm.green.1, + chrm.blue.0, + chrm.blue.1, + ]); + } + // The XMP packet is UTF-8 by §11.3.3.4; bytes that are not are not a packet this encoder + // can frame, and are dropped rather than written as an invalid iTXt. + if let Some(xmp) = meta.xmp.and_then(|bytes| str::from_utf8(bytes).ok()) { + self = self.with_xmp(xmp); + } + for text in meta.texts { + match (&text.language, &text.translated_keyword) { + // Neither field set: the annotation came from a tEXt/zTXt, or from an iTXt whose + // two optional fields were empty. Offer it as Latin-1 — which is byte-exact for + // the first case — and let `Ancillary` promote it to iTXt if the text needs it. + (None, None) => self.ancillary.add_text_latin1(&text.keyword, &text.text), + (language, translated) => self.ancillary.add_text_international_tagged( + &text.keyword, + language.as_deref().unwrap_or_default(), + translated.as_deref().unwrap_or_default(), + &text.text, + ), + } + } + self + } + /// Embeds a C2PA manifest store (the `caBX` chunk, C2PA 2.4 §A.3.2), verbatim and /// uncompressed, as the last chunk before `IDAT`. /// @@ -677,6 +851,10 @@ impl PngEncoder { pre_idat: F, out: &mut Vec, ) -> Result { + // Refuse an accumulation the spec says must not be written before emitting a byte, so a + // caller never receives a half-written buffer for a chunk set it chose (see + // [`Ancillary::validate`]). Every encode path funnels through here. + self.ancillary.validate()?; let (color, bit_depth) = (written.color, written.bit_depth); // Stride in bytes per pixel (≥1, even for sub-byte depths) and the padded row length. let bits_per_pixel = color.channels() * bit_depth as usize; diff --git a/crates/gamut-png/tests/c2pa.rs b/crates/gamut-png/tests/c2pa.rs index fc58e84f..0c577f0c 100644 --- a/crates/gamut-png/tests/c2pa.rs +++ b/crates/gamut-png/tests/c2pa.rs @@ -15,7 +15,7 @@ use common::{ }; use gamut_core::{DecodeImage, Dimensions, EncodeImage, ImageBuf, ImageRef, Indexed8, Rgb8, Rgba8}; use gamut_png::{ - PhysicalUnit, PngDecoder, PngEncoder, PngPalette, SegmentKind, SrgbIntent, deconstruct, + PhysicalUnit, PngDecoder, PngEncoder, PngPalette, SegmentKind, deconstruct, fill_c2pa, }; @@ -65,7 +65,10 @@ fn rgb_source() -> (Vec, Dimensions) { fn everything_else() -> PngEncoder { PngEncoder::new() .with_gamma(1.0 / 2.2) - .with_srgb(SrgbIntent::Perceptual) + // cICP rather than sRGB: §5.6 Table 5 and §11.3.2.5 say sRGB and iCCP must not both + // be written, and iCCP is the one whose payload has a size the store's placement depends + // on. cICP is legal alongside it (§4.3 Table 1 only ranks them). + .with_cicp(9, 16, true) .with_chromaticities((0.3127, 0.3290), (0.64, 0.33), (0.30, 0.60), (0.15, 0.06)) .with_icc_profile("Tiny", &tiny_icc_profile()) .with_significant_bits(&[8, 8, 8, 8]) diff --git a/crates/gamut-png/tests/metadata.rs b/crates/gamut-png/tests/metadata.rs index e63c8d94..dd8946d1 100644 --- a/crates/gamut-png/tests/metadata.rs +++ b/crates/gamut-png/tests/metadata.rs @@ -11,7 +11,7 @@ use common::{ chunk, ihdr_payload, minimal_png, png_from_chunks, tiny_exif, tiny_icc_profile, zlib, }; use gamut_core::{Dimensions, EncodeImage, ErrorKind, ImageRef, Rgb8}; -use gamut_png::{PngDecoder, PngEncoder, PngMetadata, SrgbIntent}; +use gamut_png::{PngDecoder, PngEncoder, PngMetadata}; /// A 2×2 RGB8 source for the encoder-driven tests. fn source() -> Vec { @@ -50,7 +50,9 @@ fn every_carrier_round_trips_byte_exact() { .with_compressed_text("Comment", "compressed comment") .with_international_text("Title", "international title") .with_gamma(1.0 / 2.2) - .with_srgb(SrgbIntent::RelativeColorimetric) + // cICP rather than sRGB, which §5.6 Table 5 and §11.3.2.5 forbid beside the iCCP + // this file also carries; sRGB's own carriage is pinned by `roundtrip.rs`. + .with_cicp(9, 16, true) .with_chromaticities( (0.3127, 0.3290), (0.6400, 0.3300), @@ -67,7 +69,16 @@ fn every_carrier_round_trips_byte_exact() { assert_eq!(meta.xmp.as_deref(), Some(xmp.as_bytes())); assert_eq!(meta.c2pa.as_deref(), Some(&c2pa[..])); assert_eq!(meta.gamma, Some(45_455)); - assert_eq!(meta.srgb, Some(SrgbIntent::RelativeColorimetric)); + let cicp = meta.cicp.expect("cICP present"); + assert_eq!( + ( + cicp.color_primaries, + cicp.transfer_function, + cicp.matrix_coefficients, + cicp.full_range + ), + (9, 16, 0, true) + ); let chrm = meta.chromaticities.expect("cHRM present"); assert_eq!(chrm.white, (31_270, 32_900)); assert_eq!(chrm.red, (64_000, 33_000)); @@ -84,17 +95,32 @@ fn every_carrier_round_trips_byte_exact() { /// and not the other fails here. #[test] fn metadata_agrees_with_decode_field_for_field() { + // Built chunk by chunk rather than by the encoder, so that *every* field is populated: the + // encoder refuses sRGB beside iCCP (§5.6 Table 5, §11.3.2.5), and a comparison of two `None`s + // would not see a chunk wired into one walk and not the other. A reader still meets such a + // file, and §13.1 says an ancillary chunk it cannot use is skipped, not fatal. let exif = tiny_exif(); let icc = tiny_icc_profile(); - let png = encode(|e| { - e.with_exif(&exif) - .with_icc_profile("Tiny", &icc) - .with_xmp("") - .with_c2pa(b"\0\0\0\x10jumbc2pa") - .with_text("Author", "nobody") - .with_gamma(1.0 / 2.2) - .with_srgb(SrgbIntent::Perceptual) - }); + let mut iccp = b"Tiny\0\0".to_vec(); + iccp.extend_from_slice(&zlib(&icc)); + let mut chrm = Vec::new(); + for coord in [31_270u32, 32_900, 64_000, 33_000, 30_000, 60_000, 15_000, 6_000] { + chrm.extend_from_slice(&coord.to_be_bytes()); + } + let png = png_from_chunks(&[ + chunk(b"IHDR", &ihdr_payload(3, 2, 8, 2, 0)), + chunk(b"eXIf", &exif), + chunk(b"iCCP", &iccp), + chunk(b"sRGB", &[1]), + chunk(b"cICP", &[1, 13, 0, 1]), + chunk(b"gAMA", &45_455u32.to_be_bytes()), + chunk(b"cHRM", &chrm), + chunk(b"tEXt", b"Author\0nobody"), + chunk(b"iTXt", b"XML:com.adobe.xmp\0\0\0\0\0"), + chunk(b"caBX", b"\0\0\0\x10jumbc2pa"), + chunk(b"IDAT", &zlib(&[0u8; 20])), + chunk(b"IEND", &[]), + ]); let meta = gamut_png::metadata(&png).unwrap(); let decoded = PngDecoder::new().decode(&png).unwrap(); @@ -109,10 +135,16 @@ fn metadata_agrees_with_decode_field_for_field() { assert_eq!(meta.chromaticities, decoded.chromaticities); assert_eq!(meta.srgb, decoded.srgb); assert_eq!(meta.cicp, decoded.cicp); + // A `None` on both sides would pass every comparison above, so pin that the file really did + // carry each field. + assert!(meta.exif.is_some() && meta.icc_profile.is_some() && meta.xmp.is_some()); + assert!(meta.c2pa.is_some() && !meta.texts.is_empty()); + assert!(meta.gamma.is_some() && meta.chromaticities.is_some()); + assert!(meta.srgb.is_some() && meta.cicp.is_some()); } /// The probe case from #379: cICP is uncompressed, so a colour-space probe costs a chunk walk and -/// nothing more. The encoder cannot write cICP, so the chunk is built by hand. +/// nothing more. Built by hand so the assertion reads the walk, not the encoder's own chunk. #[test] fn cicp_is_read_without_inflating_anything() { // BT.2020 primaries (9), PQ transfer (16), RGB matrix (0), full range. diff --git a/crates/gamut-png/tests/roundtrip.rs b/crates/gamut-png/tests/roundtrip.rs index f49aa436..75d810e8 100644 --- a/crates/gamut-png/tests/roundtrip.rs +++ b/crates/gamut-png/tests/roundtrip.rs @@ -258,7 +258,8 @@ fn ancillary_pile_survives_decode() { let (w, h) = (16u32, 16u32); let src = noise((w * h * 3) as usize, 9); let exif = tiny_exif(); - let icc = tiny_icc_profile(); + // No iCCP: it is the one chunk the encoder refuses beside the sRGB this pile carries (§5.6 + // Table 5, §11.3.2.5), and its carriage is pinned by `tests/metadata.rs`. let xmp = r#""#; let mut png = Vec::new(); PngEncoder::new() @@ -273,7 +274,6 @@ fn ancillary_pile_survives_decode() { .with_compressed_text("Comment", &"squeeze ".repeat(40)) .with_international_text("Author", "gämut") .with_exif(&exif) - .with_icc_profile("prof", &icc) .with_xmp(xmp) .encode_image( ImageRef::::new(&src, Dimensions::new(w, h).unwrap()).unwrap(), @@ -289,7 +289,6 @@ fn ancillary_pile_survives_decode() { assert_eq!(decoded.srgb, Some(SrgbIntent::RelativeColorimetric)); assert!(decoded.chromaticities.is_some()); assert_eq!(decoded.exif.as_deref(), Some(exif.as_slice())); - assert_eq!(decoded.icc_profile.unwrap().profile, icc); assert_eq!(decoded.xmp.as_deref(), Some(xmp.as_bytes())); assert_eq!(decoded.texts.len(), 3); } From d8c2ffdf4e859ec72311abc2af599d8c3aa8df73 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 00:44:55 -0400 Subject: [PATCH 02/14] test(png): pin Latin-1 text, the refused chunk pairs, and what a re-encode carries Inline in `ancillary.rs` where the assertion reads a non-pub item (`text_entry`, `validate`, `write_text`), and in `tests/preservation.rs` for the public `with_metadata` pair. Each names the function whose mutation it kills. Also wires `gamut convert` to carry the input's metadata on the PNG path, with `--strip-metadata` as the opt-out, pinned by a binary-driving test because `gamut-cli` is outside the mutation globs and the coverage regex. --- crates/gamut-cli/src/commands/convert.rs | 36 ++++- crates/gamut-cli/tests/convert_metadata.rs | 91 +++++++++++ crates/gamut-png/STATUS.md | 52 ++++++ crates/gamut-png/src/ancillary.rs | 154 +++++++++++++++++- crates/gamut-png/tests/c2pa.rs | 3 +- crates/gamut-png/tests/metadata.rs | 4 +- crates/gamut-png/tests/preservation.rs | 178 +++++++++++++++++++++ 7 files changed, 512 insertions(+), 6 deletions(-) create mode 100644 crates/gamut-cli/tests/convert_metadata.rs create mode 100644 crates/gamut-png/tests/preservation.rs diff --git a/crates/gamut-cli/src/commands/convert.rs b/crates/gamut-cli/src/commands/convert.rs index 7e7970aa..de5e6412 100644 --- a/crates/gamut-cli/src/commands/convert.rs +++ b/crates/gamut-cli/src/commands/convert.rs @@ -1,6 +1,6 @@ //! `gamut convert` — decode an image and re-encode it with a gamut codec. -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use clap::{Args, ValueEnum}; use gamut::avif::AvifEncoder; @@ -86,6 +86,14 @@ pub(crate) struct ConvertArgs { /// for other output formats. #[arg(long)] jxl_container: bool, + /// Drop the input's metadata instead of carrying it into the output. By default a PNG input + /// re-encoded to PNG keeps its EXIF, ICC profile, XMP packet, text annotations and colour + /// chunks; a stripped file is smaller, an unstripped one is colour-accurate, so the default + /// is the one that loses nothing. The C2PA manifest store is never carried either way (it is + /// signed over the bytes of the file it was made for). Currently applies only to the PNG + /// output path with a PNG input; every other pair drops metadata regardless. + #[arg(long)] + strip_metadata: bool, } /// Output container/codec for `gamut convert`. @@ -242,6 +250,22 @@ pub(crate) fn run(args: &ConvertArgs) -> Result<(), CliError> { if let Some(effort) = args.png_effort { encoder = encoder.with_effort(effort); } + // Carry the input's metadata rather than dropping it (issue #483). `png_metadata` + // reads the file a second time — cheaply: the walk skips IDAT by length and never + // inflates a pixel — and yields nothing for an input that is not a PNG. + let metadata = (!args.strip_metadata) + .then(|| png_metadata(&args.input)) + .flatten(); + if let Some(metadata) = &metadata { + tracing::info!( + texts = metadata.texts.len(), + exif = metadata.exif.is_some(), + icc = metadata.icc_profile.is_some(), + xmp = metadata.xmp.is_some(), + "carrying input metadata" + ); + encoder = encoder.with_metadata(metadata); + } encoder.encode_image(ImageRef::::new(&rgba, dims)?, &mut out)?; (rgba.len(), dims) } @@ -324,6 +348,16 @@ pub(crate) fn run(args: &ConvertArgs) -> Result<(), CliError> { Ok(()) } +/// The metadata `path` carries, or `None` when it is not a PNG or cannot be read. +/// +/// Deliberately total: the input has already been decoded successfully by the time this is +/// called, so an error here means the file is simply not a PNG — a JPEG or WebP input has +/// metadata of its own, but mapping that into PNG chunks is a cross-format job this command does +/// not do yet. Failing to *read* metadata must never fail a conversion whose pixels are fine. +fn png_metadata(path: &Path) -> Option { + gamut::png::metadata(&std::fs::read(path).ok()?).ok() +} + /// Picks the output format from `--format`, falling back to the output file's extension. fn resolve_format(args: &ConvertArgs) -> Result { if let Some(format) = args.format { diff --git a/crates/gamut-cli/tests/convert_metadata.rs b/crates/gamut-cli/tests/convert_metadata.rs new file mode 100644 index 00000000..947ceb2d --- /dev/null +++ b/crates/gamut-cli/tests/convert_metadata.rs @@ -0,0 +1,91 @@ +//! End-to-end tests for what `gamut convert` does with the input's metadata on the PNG path +//! (issue #483): carried by default, dropped under `--strip-metadata`. +//! +//! These drive the built `gamut` binary (`CARGO_BIN_EXE_gamut`) rather than calling the command +//! function, because `crates/gamut-cli` is outside both the mutation globs and the coverage +//! regex — behaviour pinned only by a unit test here is pinned nowhere the gates can see. The +//! encoder-side claims are pinned in `gamut-png`; what this file adds is that the CLI wires them +//! up at all, which is exactly the gap the issue reported (0% metadata round-trip). + +use std::path::PathBuf; +use std::process::Command; + +use gamut::core::{Dimensions, EncodeImage, ImageRef, Rgba8}; +use gamut::png::{PngEncoder, PngMetadata, SrgbIntent}; + +/// A 2×2 PNG carrying an EXIF block, a text annotation and a rendering intent. +fn png_with_metadata() -> Vec { + let rgba = vec![255u8; 4 * 4]; + let dims = Dimensions { + width: 2, + height: 2, + }; + let image = ImageRef::::new(&rgba, dims).unwrap(); + PngEncoder::new() + .with_exif(&[0x49, 0x49, 0x2A, 0x00, 0x08, 0x00, 0x00, 0x00]) + .with_text("Author", "nobody") + .with_srgb(SrgbIntent::Perceptual) + .encode_to_vec(image) + .unwrap() +} + +/// Writes `png` to a temp file, converts it to PNG with `extra` flags, and returns the output's +/// metadata. Both temp files are removed before the assertion runs. +fn convert(name: &str, png: &[u8], extra: &[&str]) -> PngMetadata { + let dir = std::env::temp_dir(); + let input = dir.join(format!( + "gamut-convert-{}-{name}-in.png", + std::process::id() + )); + let output: PathBuf = dir.join(format!( + "gamut-convert-{}-{name}-out.png", + std::process::id() + )); + std::fs::write(&input, png).unwrap(); + + let status = Command::new(env!("CARGO_BIN_EXE_gamut")) + .arg("convert") + .arg(&input) + .arg(&output) + .args(extra) + .output() + .expect("run gamut convert"); + let encoded = std::fs::read(&output).ok(); + let _ = std::fs::remove_file(&input); + let _ = std::fs::remove_file(&output); + + assert!( + status.status.success(), + "stderr: {}", + String::from_utf8_lossy(&status.stderr) + ); + gamut::png::metadata(&encoded.expect("output written")).expect("read back") +} + +/// The issue's headline: `gamut convert` used to decode to raw RGBA and encode with a bare +/// builder, so every EXIF, ICC, XMP and text chunk was lost with no warning. +#[test] +fn png_to_png_carries_the_input_metadata_by_default() { + let meta = convert("default", &png_with_metadata(), &[]); + + assert_eq!( + meta.exif.as_deref(), + Some(&[0x49, 0x49, 0x2A, 0x00, 0x08, 0x00, 0x00, 0x00][..]) + ); + assert_eq!(meta.srgb, Some(SrgbIntent::Perceptual)); + let texts: Vec<(&str, &str)> = meta + .texts + .iter() + .map(|t| (t.keyword.as_str(), t.text.as_str())) + .collect(); + assert_eq!(texts, [("Author", "nobody")]); +} + +/// The opt-out: a stripped file is smaller, which is why the flag exists, but it has to be asked +/// for — the default may not silently discard colour information. +#[test] +fn strip_metadata_drops_it_all() { + let meta = convert("stripped", &png_with_metadata(), &["--strip-metadata"]); + + assert_eq!(meta, PngMetadata::default()); +} diff --git a/crates/gamut-png/STATUS.md b/crates/gamut-png/STATUS.md index 260f098a..7e033458 100644 --- a/crates/gamut-png/STATUS.md +++ b/crates/gamut-png/STATUS.md @@ -41,6 +41,8 @@ opts into narrowing. That is distinct from the encoder's *lossless* auto-reduce | E1 | #224 | **Efficiency:** `deconstruct` byte accounting; divan size/bpp + per-stage bench; libpng-9 size contract; opt-in transparent cleanup; palette-vs-native race; `crc32fast` and restructured filter kernels (see [Efficiency](#efficiency-issue-224)) | ✅ done | | C1 | C2PA 2.4 §A.3.2, §18.5.4 | **C2PA carriage** (#440): the `caBX` manifest store — raw decode surface (`c2pa`; first CRC-valid chunk before `IDAT` wins, ignored ones counted, under the metadata budget); `with_c2pa` / `with_c2pa_reserved` as the last chunk before `IDAT`; the whole-chunk exclusion span from `encode_with_report` and `PngReport::c2pa`, filled in place by `fill_c2pa` (see [C2PA](#c2pa-manifest-store-issue-440)) | ✅ done | +| M1 | §4.3, §5.6, §11.3.2.6, §11.3.3 | **Metadata preservation** (#483): `with_metadata` / `with_metadata_from` carry a read file's eXIf/iCCP/XMP/text/colour chunks into a re-encode (`gamut convert` uses it; `--strip-metadata` opts out); `with_cicp`; `sRGB` beside `iCCP` refused and resolved by colour-chunk priority; `tEXt`/`zTXt` written as Latin-1 with promotion to `iTXt` (see [Metadata preservation](#metadata-preservation-issue-483)) | ✅ done | + ## Decoder phases (issue #249) | Phase | Spec | Scope | Status | @@ -135,6 +137,56 @@ hash assertion can be checked over the excluded span) is issue #447. of any kind. `gamut convert` does not carry a store across a re-encode (that is the facade's `C2paPolicy` law, and the CLI's own path is #448/#483). +## Metadata preservation (issue #483) + +The read side has surfaced every metadata payload since D5, and the write side has accepted every +one since P8, but nothing joined them: a re-encode dropped all of it, so `gamut convert`'s PNG +path round-tripped 0% of a file's metadata. + +`PngEncoder::with_metadata(&PngMetadata)` and `with_metadata_from(&DecodedPng)` are that join — +one private borrowed view behind two entry points, so the pixel-free `metadata()` walk and a full +`decode()` reach it without copying a large ICC profile twice. `gamut convert` uses it on the PNG +output path; `--strip-metadata` is the opt-out. **Preserve is the default**: a stripped file is +smaller, but dropping an ICC profile silently changes what a viewer paints, so the loss is the +thing that has to be asked for. + +**Three spec-driven adjustments** on the way through, none of them a policy choice: + +- `iCCP` and `sRGB` are **resolved, not both written**. §5.6 Table 5 records the constraint on both + rows and §11.3.2.5 repeats it; §4.3 Table 1 then ranks the colour chunks (cICP 1, iCCP 2, sRGB 3, + cHRM+gAMA 4) and a reader honours the lowest number. So the `iCCP` is carried and the `sRGB` + dropped — the chunk a conforming reader was already ignoring. +- A `cICP` whose matrix coefficients are not 0 is dropped: §11.3.2.6 requires 0 for PNG, so such a + chunk is not conforming and carrying it forward would reproduce the defect. +- The **C2PA manifest store is never carried**. A store is signed over the exact bytes of the file + it was made for — the reason `caBX` is unsafe to copy (C2PA 2.4 §A.3.2) — so a copy is invalid by + construction. Re-sign the output and set it with `with_c2pa`. + +**Two spec defects** the same issue found, both in the writer: + +- *`sRGB` beside `iCCP` was written whenever both were set*, warned about only in a doc comment. + Now `Ancillary::validate` refuses the encode with `InvalidInput` at the one chokepoint every + encode path funnels through. Refusing rather than dropping one is the point: which the caller + meant is not guessable, and `with_metadata` exists for the case where §4.3 answers it. +- *`tEXt`/`zTXt` carried UTF-8.* §11.3.3.2 interprets a `tEXt` text string as Latin-1, §11.3.3.3 + makes an inflated `zTXt` identical to it, and §11.3.3.1 binds every keyword to Latin-1 — but the + writer pushed the Rust `String`'s bytes, storing `C3 A9` where `é` belongs. Text and keyword are + now converted once at the setter and the entry holds the bytes its chunk carries, so the wrong + encoding is unrepresentable rather than merely avoided. A text outside Latin-1 is promoted to + `iTXt` exactly as §11.3.3.2 directs, keeping the caller's compression via §11.3.3.4's flag; a + *keyword* outside it has no chunk at all, so it refuses the encode. + +`with_cicp` (§11.3.2.6) was added with this work — without it, preservation would silently drop the +highest-precedence colour chunk of any file that carries one. It takes no matrix argument: PNG +fixes that byte at 0. + +**Not done.** `pHYs`, `tIME`, `sBIT` and `bKGD` are not part of `PngMetadata`/`DecodedPng`, so they +cannot be carried (set them with their own builder methods). A `zTXt` is indistinguishable from a +`tEXt` once decoded, so a compressed annotation is rewritten uncompressed — no text is lost, only +bytes. §11.3.3.1's keyword *syntax* rules beyond Latin-1 (the printable subset, the space rules, +the 1–79-byte bound) are not enforced. `gamut convert` carries metadata only PNG→PNG; mapping a +JPEG/WebP/JXL input's metadata into PNG chunks is a cross-format job of its own. + ## Efficiency (issue #224) Correctness was settled long before efficiency was measured. This section is the measured state: diff --git a/crates/gamut-png/src/ancillary.rs b/crates/gamut-png/src/ancillary.rs index b3b1815d..23015801 100644 --- a/crates/gamut-png/src/ancillary.rs +++ b/crates/gamut-png/src/ancillary.rs @@ -305,7 +305,11 @@ impl Ancillary { // §11.3.2.6 Table 18: primaries, transfer function, matrix coefficients, full-range // flag — one byte each, the matrix fixed at 0 because "RGB is currently the only // supported color model in PNG, and as such Matrix Coefficients shall be set to 0". - chunk::write_chunk(out, *b"cICP", &[primaries, transfer, 0, u8::from(full_range)]); + chunk::write_chunk( + out, + *b"cICP", + &[primaries, transfer, 0, u8::from(full_range)], + ); } if let Some(chrm) = self.chrm { let mut data = [0u8; 32]; @@ -567,7 +571,6 @@ pub(crate) fn sbit_for(sbit: &[u8], color: ColorType, bit_depth: u8) -> Option]) -> Vec { + let mut iccp = b"Tiny\0\0".to_vec(); + iccp.extend_from_slice(&zlib(&tiny_icc_profile())); + let mut chrm = Vec::new(); + for coord in CHRM { + chrm.extend_from_slice(&coord.to_be_bytes()); + } + let mut chunks = vec![ + chunk(b"IHDR", &ihdr_payload(3, 2, 8, 2, 0)), + chunk(b"eXIf", &tiny_exif()), + chunk(b"iCCP", &iccp), + chunk(b"gAMA", &45_455u32.to_be_bytes()), + chunk(b"cHRM", &chrm), + chunk(b"tEXt", b"Author\0caf\xE9"), + chunk(b"iTXt", b"Note\0\0\0de\0Notiz\0g\xC3\xA4mut"), + chunk(b"iTXt", b"XML:com.adobe.xmp\0\0\0\0\0"), + chunk(b"caBX", b"\0\0\0\x10jumbc2pa"), + ]; + chunks.extend_from_slice(extra); + chunks.push(chunk(b"IDAT", &zlib(&[0u8; 20]))); + chunks.push(chunk(b"IEND", &[])); + png_from_chunks(&chunks) +} + +/// Re-encodes a 2×2 image under `build`, and reads back what the output carries. +fn re_encoded(build: impl FnOnce(PngEncoder) -> PngEncoder) -> PngMetadata { + let pixels = vec![0u8; 3 * 4]; + let image = ImageRef::::new(&pixels, Dimensions::new(2, 2).unwrap()).unwrap(); + let png = build(PngEncoder::new()) + .encode_to_vec(image) + .expect("re-encode"); + gamut_png::metadata(&png).expect("read back") +} + +/// The headline claim of #483: nothing the read side surfaced is dropped on the way back out. +/// Before this, `gamut convert`'s PNG path round-tripped 0% of it. +#[test] +fn every_carried_chunk_survives_a_re_encode() { + let meta = gamut_png::metadata(&source(&[])).unwrap(); + let re = re_encoded(|e| e.with_metadata(&meta)); + + assert_eq!(re.exif, meta.exif); + assert_eq!(re.icc_profile, meta.icc_profile); + assert_eq!(re.xmp, meta.xmp); + assert_eq!(re.gamma, Some(45_455)); + let chrm = re.chromaticities.expect("cHRM carried"); + assert_eq!( + (chrm.white, chrm.blue), + ((CHRM[0], CHRM[1]), (CHRM[6], CHRM[7])) + ); + // Both text annotations, in file order, with the Latin-1 `é` intact. + let texts: Vec<(&str, &str)> = re + .texts + .iter() + .map(|t| (t.keyword.as_str(), t.text.as_str())) + .collect(); + assert_eq!(texts, [("Author", "café"), ("Note", "gämut")]); +} + +/// §11.3.3.4's language tag and translated keyword are what make an `iTXt` international; a +/// re-encode that reduced every annotation to a bare keyword and text would silently strip them. +#[test] +fn an_itxt_keeps_its_language_and_translated_keyword() { + let meta = gamut_png::metadata(&source(&[])).unwrap(); + let re = re_encoded(|e| e.with_metadata(&meta)); + + let note = re.texts.iter().find(|t| t.keyword == "Note").expect("Note"); + assert_eq!(note.language.as_deref(), Some("de")); + assert_eq!(note.translated_keyword.as_deref(), Some("Notiz")); +} + +/// §4.3 Table 1 ranks the colour chunks and a reader honours the lowest priority number, so of a +/// source carrying both the `iCCP` (2) is the chunk that was being used and the `sRGB` (3) the +/// chunk that was being ignored. Carrying both would be the pair §5.6 Table 5 and §11.3.2.5 +/// refuse, and would make the file unencodable. +#[test] +fn srgb_gives_way_to_an_icc_profile_from_the_same_file() { + let meta = gamut_png::metadata(&source(&[chunk(b"sRGB", &[1])])).unwrap(); + assert_eq!(meta.srgb, Some(SrgbIntent::RelativeColorimetric)); + assert!(meta.icc_profile.is_some(), "the source carries both"); + + let re = re_encoded(|e| e.with_metadata(&meta)); + assert!(re.icc_profile.is_some(), "the ICC profile is kept"); + assert!(re.srgb.is_none(), "the lower-priority sRGB is dropped"); +} + +/// The converse: with no ICC profile to outrank it, the rendering intent is the colour +/// information the file has, and dropping it would lose it. +#[test] +fn srgb_is_carried_when_no_icc_profile_outranks_it() { + let png = png_from_chunks(&[ + chunk(b"IHDR", &ihdr_payload(3, 2, 8, 2, 0)), + chunk(b"sRGB", &[2]), + chunk(b"IDAT", &zlib(&[0u8; 20])), + chunk(b"IEND", &[]), + ]); + let meta = gamut_png::metadata(&png).unwrap(); + + let re = re_encoded(|e| e.with_metadata(&meta)); + assert_eq!(re.srgb, Some(SrgbIntent::Saturation)); +} + +/// §11.3.2.6: "RGB is currently the only supported color model in PNG, and as such Matrix +/// Coefficients shall be set to 0." A source chunk that says otherwise is not a conforming cICP, +/// so it is dropped rather than reproduced — while a conforming one is carried, which matters +/// because §4.3 Table 1 makes cICP the *highest*-precedence colour chunk. +#[test] +fn a_cicp_is_carried_only_when_its_matrix_coefficients_are_zero() { + let conforming = gamut_png::metadata(&source(&[chunk(b"cICP", &[9, 16, 0, 1])])).unwrap(); + let carried = re_encoded(|e| e.with_metadata(&conforming)) + .cicp + .expect("cICP carried"); + assert_eq!( + ( + carried.color_primaries, + carried.transfer_function, + carried.matrix_coefficients, + carried.full_range + ), + (9, 16, 0, true) + ); + + let non_rgb = gamut_png::metadata(&source(&[chunk(b"cICP", &[9, 16, 1, 1])])).unwrap(); + assert!(non_rgb.cicp.is_some(), "the source carries it"); + assert!(re_encoded(|e| e.with_metadata(&non_rgb)).cicp.is_none()); +} + +/// Drift guard. A C2PA manifest store is signed over the exact bytes of the file it was made for, +/// which is why C2PA 2.4 §A.3.2 marks `caBX` unsafe to copy: carried into a re-encode it is +/// invalid by construction, and a validator would report a tampered file rather than an unsigned +/// one. This asserts the omission is deliberate, because adding one line would undo it silently. +#[test] +fn the_c2pa_manifest_store_is_never_carried_forward() { + let meta = gamut_png::metadata(&source(&[])).unwrap(); + assert!(meta.c2pa.is_some(), "the source carries a store"); + + assert!(re_encoded(|e| e.with_metadata(&meta)).c2pa.is_none()); +} + +/// The two entry points differ only in which read surface they take, so a field wired into one +/// and not the other is a bug this catches — the same anti-drift shape `tests/metadata.rs` uses +/// for the two *read* walks. +#[test] +fn with_metadata_from_agrees_with_with_metadata() { + let png = source(&[chunk(b"cICP", &[9, 16, 0, 1])]); + let decoded = PngDecoder::new().decode(&png).unwrap(); + let meta = gamut_png::metadata(&png).unwrap(); + + let from_decoded = re_encoded(|e| e.with_metadata_from(&decoded)); + let from_metadata = re_encoded(|e| e.with_metadata(&meta)); + assert_eq!(from_decoded, from_metadata); + // A pair of empty results would satisfy the comparison above. + assert!(from_decoded.icc_profile.is_some() && from_decoded.cicp.is_some()); + assert!(!from_decoded.texts.is_empty() && from_decoded.exif.is_some()); +} From d7868bc39c067767e71a7d9782cd68dbf2d108e6 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 01:35:28 -0400 Subject: [PATCH 03/14] =?UTF-8?q?fix(png)!:=20implement=20=C2=A711.3.3's?= =?UTF-8?q?=20text=20clauses=20and=20stop=20refusing=20the=20colour=20pair?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The keyword rule shipped as "code point under 256", which is neither of the clauses PNG states. §11.3.3.1 binds a keyword to code points 0x20-0x7E and 0xA1-0xFF, 1 to 79 bytes, with no leading, trailing or consecutive space and expressly not U+00A0; §11.3.3.1's closing paragraph restricts a tEXt/zTXt text string to that repertoire plus U+000A. Both are now implemented as written, so an empty keyword, a 200-byte one, U+00A0, 0x7F and 0x9F no longer pass, and a control character promotes to iTXt with everything else outside the repertoire rather than being written with no defined meaning. A null was accepted anywhere. It is the field separator, so `Auth\0or` does not merely offend the grammar — the chunk re-parses as a *different* annotation. §11.3.3.2 forbids it in a tEXt keyword and text string and §11.3.3.4 in an iTXt's text and translated keyword; all four are refused, as is a language tag outside BCP 47's subtag characters and an XMP packet that is not UTF-8. The refusal names the annotation's index and its keyword through the owned-context error channel, so a caller can act on it. The sRGB-beside-iCCP refusal goes. §5.6 Table 5 and §11.3.2.5 say only "should not" and "it is recommended", and §15 gives the BCP 14 keywords force "when, and only when, they appear in all capitals"; §4.3 Table 1 presupposes the pair and defines the outcome by ranking the chunks. libpng reads a file carrying both and returns the same pixels, which `tests/oracle.rs` now pins — so the four in-repo fixtures that had to be rewritten around the refusal are restored. BREAKING CHANGE: a text annotation whose keyword or text breaks §11.3.3 now fails the encode with `Error::InvalidInput` instead of being written. Keywords that were accepted before and are not now: empty, longer than 79 bytes, containing a null, a control character or U+00A0, and any with a leading, trailing or consecutive space. --- crates/gamut-png/src/ancillary.rs | 613 ++++++++++++++++++++----- crates/gamut-png/src/decoded.rs | 47 +- crates/gamut-png/src/decoder.rs | 6 +- crates/gamut-png/src/encoder.rs | 219 ++++++--- crates/gamut-png/src/lib.rs | 3 +- crates/gamut-png/tests/c2pa.rs | 8 +- crates/gamut-png/tests/metadata.rs | 51 +- crates/gamut-png/tests/oracle.rs | 41 ++ crates/gamut-png/tests/preservation.rs | 192 ++++++-- crates/gamut-png/tests/roundtrip.rs | 5 +- 10 files changed, 924 insertions(+), 261 deletions(-) diff --git a/crates/gamut-png/src/ancillary.rs b/crates/gamut-png/src/ancillary.rs index 23015801..19e179ed 100644 --- a/crates/gamut-png/src/ancillary.rs +++ b/crates/gamut-png/src/ancillary.rs @@ -37,9 +37,10 @@ use gamut_core::{Error, Result}; use gamut_deflate::{DeflateEncoder, Level}; +use crate::decoded::XMP_KEYWORD; use crate::{ColorType, chunk}; -/// The rendering intent for an `sRGB` chunk (PNG spec §11.3.3.5). +/// The rendering intent for an `sRGB` chunk (PNG spec §11.3.2.5). #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum SrgbIntent { /// Perceptual (intent code 0). @@ -106,6 +107,20 @@ enum TextKind { InternationalCompressed, } +impl TextKind { + /// The `iTXt` kind that carries the same compression choice. + /// + /// §11.3.3.2 sends text outside Latin-1's repertoire to `iTXt`, and §11.3.3.4 gives `iTXt` a + /// compression flag of its own, so a promotion changes the character set and nothing else — + /// a compressed annotation stays compressed. + fn international(self) -> Self { + match self { + Self::Latin1 | Self::International => Self::International, + Self::Compressed | Self::InternationalCompressed => Self::InternationalCompressed, + } + } +} + /// One accumulated text annotation, already **in the byte form its chunk carries**. /// /// The distinction is the whole point of holding bytes rather than `String`s. PNG's three text @@ -116,11 +131,12 @@ enum TextKind { /// chunk"), while §11.3.3.4 gives `iTXt` UTF-8. A Rust `String` is UTF-8, so writing its bytes /// into a `tEXt` chunk stores mojibake for every code point above U+007F — `é` (U+00E9) becomes /// the two bytes `C3 A9`, which a conforming reader shows as `é`. Converting once, at the point -/// the caller sets the text, makes that unrepresentable: an entry exists only if its bytes are -/// already right for its `kind`. +/// the caller sets the text, makes that unrepresentable: an entry's bytes are always already +/// right for its `kind`, or it carries the [`fault`](Self::fault) that stops it being written. #[derive(Debug, Clone)] struct TextEntry { - /// The keyword, Latin-1 (§11.3.3.1). + /// The keyword, Latin-1 (§11.3.3.1). Empty when [`fault`](Self::fault) is set, because such + /// an entry is never written — [`Ancillary::validate`] refuses the encode first. keyword: Vec, /// The text: Latin-1 for `tEXt`/`zTXt`, UTF-8 for `iTXt`. text: Vec, @@ -130,15 +146,116 @@ struct TextEntry { /// The `iTXt` translated keyword (UTF-8, §11.3.3.4); empty for the other kinds. translated: Vec, kind: TextKind, + /// Whether this entry came from [`Ancillary::begin_carry`] rather than a direct setter, so a + /// second carry can replace exactly what the first contributed. + carried: bool, + /// Why this annotation must not be written, if it must not. Recorded here rather than + /// returned from the setter because the setters sit behind `#[must_use]` builder methods + /// that have no error channel; [`Ancillary::validate`] reports it at the encode chokepoint. + fault: Option, +} + +/// Why one accumulated text annotation cannot be written, and which annotation it was. +#[derive(Debug, Clone)] +struct TextFault { + /// The keyword exactly as the caller gave it, for the refusal message — including a keyword + /// that is itself the fault. + keyword: String, + /// The clause the annotation breaks, phrased for the caller. + reason: &'static str, +} + +/// §11.3.3.1: "Keywords are restricted to 1 to 79 bytes in length." +const KEYWORD_LENGTH: &str = "a keyword is restricted to 1 to 79 bytes (§11.3.3.1)"; +/// §11.3.3.1: "Keywords shall contain only printable Latin-1 [ISO_8859-1] characters and spaces; +/// that is, only code points 0x20-7E and 0xA1-FF are allowed", and expressly "nor is U+00A0 +/// NON-BREAKING SPACE". A null is outside it too, which is also §11.3.3.2's "Neither the keyword +/// nor the text string may contain a null character". +const KEYWORD_REPERTOIRE: &str = "a keyword may hold only code points 0x20-0x7E and 0xA1-0xFF \ + — no null, no control character, not U+00A0 (§11.3.3.1)"; +/// §11.3.3.1: "leading spaces, trailing spaces, and consecutive spaces are not permitted in +/// keywords". +const KEYWORD_SPACES: &str = + "a keyword may not have a leading, trailing or consecutive space (§11.3.3.1)"; +/// §11.3.3.2 for `tEXt`/`zTXt` ("Neither the keyword nor the text string may contain a null +/// character") and §11.3.3.4 for `iTXt` ("neither shall contain a zero byte"). The null is the +/// field separator, so an embedded one does not merely offend the grammar — the chunk re-parses +/// as a *different* annotation. +const TEXT_NUL: &str = "a text string may not contain a null character (§11.3.3.2, §11.3.3.4)"; +/// §11.3.3.4: "The language tag is a well-formed language tag defined by [BCP47]", whose subtags +/// are ASCII letters and digits joined by hyphens. Anything else is neither well-formed nor +/// (being written as UTF-8 and read back as Latin-1) byte-exact. +const LANGUAGE_TAG: &str = + "an iTXt language tag may hold only ASCII letters, digits and '-' (§11.3.3.4, BCP 47)"; +/// §11.3.3.4: "The translated keyword and text both use the UTF-8 encoding, and neither shall +/// contain a zero byte (null character)." +const TRANSLATED_NUL: &str = + "an iTXt translated keyword may not contain a null character (§11.3.3.4)"; +/// §11.3.3.4 gives the `iTXt` text field UTF-8 and no other encoding, so a packet that is not +/// UTF-8 has no chunk to go in. Dropping it silently is the loss this crate refuses to make. +const XMP_NOT_UTF8: &str = + "the XMP packet is not UTF-8, and an iTXt text string must be (§11.3.3.4)"; + +/// Whether `c` is a printable Latin-1 character or a space, the repertoire §11.3.3.1 spells out +/// as "only code points 0x20-7E and 0xA1-FF". +fn printable_latin1(c: char) -> bool { + matches!(u32::from(c), 0x20..=0x7E | 0xA1..=0xFF) +} + +/// Whether `c` may appear in a `tEXt`/`zTXt` **text string**: §11.3.3.1's closing paragraph +/// restricts their content to "the printable Latin-1 character set plus U+000A LINE FEED (LF)". +/// +/// §11.3.3.2 says more loosely that the text "may contain any Latin-1 character", which would +/// admit the C0/C1 controls and U+00A0. The tighter reading costs nothing to take: a character +/// outside this set is not rejected, it is *promoted* to `iTXt` — exactly what §11.3.3.2's own +/// "Text containing characters outside the repertoire of ISO/IEC 8859-1 should be encoded using +/// the iTXt chunk" directs — so the character always survives and only the chunk changes. +fn text_repertoire(c: char) -> bool { + c == '\n' || printable_latin1(c) +} + +/// The Latin-1 byte of `c`: Latin-1 is the first 256 Unicode code points, so the encoding is +/// `u8::try_from` — the exact inverse of the decoder's `latin1`, which maps byte *n* to U+00*nn*. +fn latin1_byte(c: char) -> Option { + u8::try_from(u32::from(c)).ok() } -/// The Latin-1 bytes of `s`, or `None` when a character has no Latin-1 encoding. +/// The Latin-1 bytes of a keyword, or the §11.3.3.1 clause it breaks. /// -/// Latin-1 is the first 256 Unicode code points, so the encoding is `u8::try_from` on each -/// `char` — the exact inverse of the decoder's `latin1`, which maps byte *n* to U+00*nn*. A -/// string that came out of this crate's decoder therefore always converts back. -fn latin1_bytes(s: &str) -> Option> { - s.chars().map(|c| u8::try_from(u32::from(c)).ok()).collect() +/// The repertoire is checked before the length so that the length bound counts *stored* bytes: +/// every character that passes is one Latin-1 byte, which a UTF-8 `str::len` is not. +fn keyword_bytes(keyword: &str) -> core::result::Result, &'static str> { + let bytes: Option> = keyword + .chars() + .map(|c| latin1_byte(c).filter(|_| printable_latin1(c))) + .collect(); + let bytes = bytes.ok_or(KEYWORD_REPERTOIRE)?; + if bytes.is_empty() || bytes.len() > 79 { + return Err(KEYWORD_LENGTH); + } + if keyword.starts_with(' ') || keyword.ends_with(' ') || keyword.contains(" ") { + return Err(KEYWORD_SPACES); + } + Ok(bytes) +} + +/// The Latin-1 bytes of a `tEXt`/`zTXt` text string, or `None` when a character is outside +/// [`text_repertoire`] — the signal to promote the annotation to `iTXt`. +fn text_bytes(text: &str) -> Option> { + text.chars() + .map(|c| latin1_byte(c).filter(|_| text_repertoire(c))) + .collect() +} + +/// The §11.3.3.4 clause an `iTXt`'s language tag or translated keyword breaks, if any. +fn itxt_field_fault(language: &str, translated: &str) -> Option<&'static str> { + if !language + .bytes() + .all(|b| b.is_ascii_alphanumeric() || b == b'-') + { + return Some(LANGUAGE_TAG); + } + translated.contains('\0').then_some(TRANSLATED_NUL) } /// Accumulated ancillary metadata to emit alongside the image. @@ -170,13 +287,10 @@ pub(crate) struct Ancillary { pub c2pa: Option>, /// tEXt / zTXt / iTXt entries, emitted in insertion order. texts: Vec, - /// Whether a caller set a text annotation whose **keyword** has no Latin-1 encoding. - /// - /// §11.3.3.1 restricts a keyword to Latin-1 in all three text chunks, so — unlike the text, - /// which `iTXt` carries in UTF-8 — there is no chunk such a keyword fits. The entry is - /// dropped at the setter and the encode is refused by [`Self::validate`], rather than - /// silently writing a keyword no reader can match. - unencodable_keyword: bool, + /// Whether the entries being pushed right now come from a metadata carry, so that a second + /// carry can replace exactly what the first contributed. Set between [`Self::begin_carry`] + /// and [`Self::end_carry`]. + carrying: bool, } impl Ancillary { @@ -206,93 +320,147 @@ impl Ancillary { } /// Adds an `iTXt` entry keeping its language tag and translated keyword (§11.3.3.4), which - /// [`add_text_international`](Self::add_text_international) leaves empty. Used only to carry - /// a decoded annotation forward, so that re-encoding a file does not silently drop the two - /// fields that make `iTXt` international. + /// [`add_text_international`](Self::add_text_international) leaves empty, and its compression + /// flag. Used to carry a decoded annotation forward without changing its identity: neither + /// the two fields that make `iTXt` international nor the flag that keeps a 40-byte payload + /// from being rewritten as 1600 uncompressed bytes. pub(crate) fn add_text_international_tagged( &mut self, keyword: &str, language: &str, translated: &str, text: &str, + compressed: bool, ) { - if let Some(mut entry) = self.text_entry(keyword, text, TextKind::International) { - entry.language = language.as_bytes().to_vec(); - entry.translated = translated.as_bytes().to_vec(); - self.texts.push(entry); + let kind = if compressed { + TextKind::InternationalCompressed + } else { + TextKind::International + }; + let mut entry = self.text_entry(keyword, text, kind); + if entry.fault.is_none() { + entry.fault = itxt_field_fault(language, translated).map(|reason| TextFault { + keyword: keyword.to_string(), + reason, + }); } + entry.language = language.as_bytes().to_vec(); + entry.translated = translated.as_bytes().to_vec(); + self.texts.push(entry); } - fn push_text(&mut self, keyword: &str, text: &str, kind: TextKind) { - if let Some(entry) = self.text_entry(keyword, text, kind) { - self.texts.push(entry); + /// Adds an XMP packet as the `iTXt` §11.3.3.1 Table 21 reserves for it. + /// + /// Takes bytes rather than a `&str` because that is what the read side surfaces: a file's + /// packet is whatever bytes its chunk held. §11.3.3.4 gives the `iTXt` text field UTF-8 and + /// no alternative, so bytes that are not UTF-8 have no chunk to go in — and are recorded as + /// a refusal rather than discarded, because a caller that handed this encoder a packet is + /// entitled to learn it did not come out the other side. + pub(crate) fn add_xmp(&mut self, packet: &[u8]) { + match str::from_utf8(packet) { + Ok(text) => self.add_text_international(XMP_KEYWORD, text), + Err(_) => { + let mut entry = self.text_entry(XMP_KEYWORD, "", TextKind::International); + entry.fault = Some(TextFault { + keyword: XMP_KEYWORD.to_string(), + reason: XMP_NOT_UTF8, + }); + self.texts.push(entry); + } } } - /// Builds the entry for one text annotation, choosing the chunk that can actually carry it. + /// Starts carrying a read file's metadata, discarding whatever a previous carry contributed. + /// + /// This is what makes [`PngEncoder::with_metadata`](crate::PngEncoder::with_metadata) + /// idempotent for text. The single-value slots — `gamma`, `iccp`, `srgb`, … — are idempotent + /// already because a second write overwrites the first; the text list is the one place where + /// "set it again" would otherwise mean "append it again", duplicating every annotation. + pub(crate) fn begin_carry(&mut self) { + self.texts.retain(|entry| !entry.carried); + self.carrying = true; + } + + /// Ends the carry started by [`begin_carry`](Self::begin_carry), so later direct setters push + /// entries a subsequent carry will not remove. + pub(crate) fn end_carry(&mut self) { + self.carrying = false; + } + + fn push_text(&mut self, keyword: &str, text: &str, kind: TextKind) { + let entry = self.text_entry(keyword, text, kind); + self.texts.push(entry); + } + + /// Builds the entry for one text annotation, choosing the chunk that can actually carry it + /// and recording the clause it breaks if no chunk can. /// /// The caller's `kind` is a *preference*, not a guarantee: §11.3.3.2 says outright that "text /// containing characters outside the repertoire of ISO/IEC 8859-1 should be encoded using the - /// `iTXt` chunk", so a `tEXt`/`zTXt` request whose text is not Latin-1 is promoted to `iTXt` - /// rather than written as UTF-8 bytes a Latin-1 reader mis-renders. The promotion keeps the - /// caller's *other* choice — compression — because §11.3.3.4 gives `iTXt` a compression flag - /// of its own; only the character set changes. + /// `iTXt` chunk", so a `tEXt`/`zTXt` request whose text leaves [`text_repertoire`] is + /// promoted rather than written as bytes a Latin-1 reader mis-renders. The promotion keeps + /// the caller's *other* choice, compression, because §11.3.3.4 gives `iTXt` a flag of its own. /// - /// `None` (the entry is dropped, and [`Self::validate`] then refuses the encode) is reserved - /// for the one case no chunk can express: a keyword outside Latin-1. - fn text_entry(&mut self, keyword: &str, text: &str, kind: TextKind) -> Option { - let Some(keyword) = latin1_bytes(keyword) else { - self.unencodable_keyword = true; - return None; + /// A null in the text is the one thing promotion cannot fix — §11.3.3.2 and §11.3.3.4 both + /// forbid it, and it is the field separator, so the chunk would re-parse as a different + /// annotation — and neither can a keyword outside §11.3.3.1's repertoire, length or spacing + /// rules. Those become a [`TextFault`] the entry carries to [`Self::validate`]. + fn text_entry(&self, keyword: &str, text: &str, kind: TextKind) -> TextEntry { + let (keyword_bytes, keyword_fault) = match keyword_bytes(keyword) { + Ok(bytes) => (bytes, None), + Err(reason) => (Vec::new(), Some(reason)), }; - let (kind, text) = match (kind, latin1_bytes(text)) { - (TextKind::Latin1, Some(latin1)) => (TextKind::Latin1, latin1), - (TextKind::Compressed, Some(latin1)) => (TextKind::Compressed, latin1), - (TextKind::Latin1, None) => (TextKind::International, text.as_bytes().to_vec()), - (TextKind::Compressed, None) => { - (TextKind::InternationalCompressed, text.as_bytes().to_vec()) - } - (kind, _) => (kind, text.as_bytes().to_vec()), + let reason = keyword_fault.or_else(|| text.contains('\0').then_some(TEXT_NUL)); + // An iTXt was asked for as UTF-8 and stays UTF-8; only a Latin-1 request has a + // repertoire to leave. + let latin1 = match kind { + TextKind::Latin1 | TextKind::Compressed => text_bytes(text), + TextKind::International | TextKind::InternationalCompressed => None, + }; + let (kind, text_bytes) = match latin1 { + Some(bytes) => (kind, bytes), + None => (kind.international(), text.as_bytes().to_vec()), }; - Some(TextEntry { - keyword, - text, + TextEntry { + keyword: keyword_bytes, + text: text_bytes, language: Vec::new(), translated: Vec::new(), kind, - }) + carried: self.carrying, + fault: reason.map(|reason| TextFault { + keyword: keyword.to_string(), + reason, + }), + } } - /// Refuses an accumulation the spec says must not be written, before any byte is emitted. + /// Refuses an accumulation the spec forbids, before any byte is emitted. /// - /// Two cases, both of which the caller stated explicitly and neither of which this encoder - /// may silently resolve for it: + /// Only the text chunks are refusable here, and only where a clause is a requirement rather + /// than a recommendation: a keyword outside §11.3.3.1's repertoire, length or spacing rules; + /// a null in a text string (§11.3.3.2, §11.3.3.4); a language tag or translated keyword + /// §11.3.3.4 rules out; a non-UTF-8 XMP packet. Each is a chunk that would be *read back as + /// something else* — the null re-frames the annotation outright — so writing it is a silent + /// corruption, and dropping it is a silent loss. /// - /// - **`sRGB` together with `iCCP`.** §5.6 Table 5 records the constraint on both rows — "if - /// the `iCCP` chunk is present, the `sRGB` chunk should not be present" and its converse — - /// and §11.3.2.5 repeats it ("it is recommended that the `sRGB` and `iCCP` chunks do not - /// appear simultaneously in a PNG datastream"). Emitting both is not undefined, because - /// §4.3 Table 1 ranks the colour chunks and a reader takes the lowest priority number - /// (`iCCP` 2 over `sRGB` 3) — but it *is* a datastream the standard tells encoders not to - /// produce, and which of the two the caller meant is not something this crate can guess. - /// Dropping one silently would lose colour information the caller supplied, so the encode - /// is refused. To carry both forward from a decoded file, use - /// [`PngEncoder::with_metadata`](crate::PngEncoder::with_metadata), which applies Table 1 - /// itself. - /// - **A text keyword outside Latin-1** (§11.3.3.1), which no text chunk can carry. + /// The colour chunks are deliberately **not** policed. §5.6 Table 5 and §11.3.2.5 say only + /// that `sRGB` and `iCCP` "should not" appear together, and §15 gives the BCP 14 keywords + /// force "when, and only when, they appear in all capitals"; §4.3 Table 1 then *presupposes* + /// the co-occurrence and defines the outcome by ranking the chunks. Both are written, and a + /// reader takes the highest-priority one. pub(crate) fn validate(&self) -> Result<()> { - if self.srgb.is_some() && self.iccp.is_some() { - return Err(Error::invalid_input( - env!("CARGO_PKG_NAME"), - "PNG: sRGB and iCCP must not both be written (spec §5.6 Table 5, §11.3.2.5); \ - set one", - )); - } - if self.unencodable_keyword { - return Err(Error::invalid_input( - env!("CARGO_PKG_NAME"), - "PNG: a text keyword must be Latin-1 (spec §11.3.3.1)", - )); + for (index, entry) in self.texts.iter().enumerate() { + if let Some(fault) = &entry.fault { + return Err(Error::invalid_input( + env!("CARGO_PKG_NAME"), + "PNG: a text annotation breaks the clause of the chunk that would carry it", + ) + .with_detail(format!( + "text annotation {index} (keyword {:?}): {}", + fault.keyword, fault.reason + ))); + } } Ok(()) } @@ -969,21 +1137,32 @@ mod tests { assert_eq!(find_chunk(&post, b"bKGD"), None); } + /// Encodes `a`'s post-PLTE chunks and returns the buffer, so a claim can read the bytes a + /// text annotation actually becomes. + fn post_plte(a: &Ancillary) -> Vec { + let mut out = vec![0u8; 8]; + a.write_post_plte(&mut out, DeflateEncoder::DEFAULT_EFFORT, RGB8); + out + } + + /// The refusal `validate` gives, rendered — including the owned detail naming the annotation. + fn refusal(a: &Ancillary) -> String { + a.validate().expect_err("the encode is refused").to_string() + } + /// A `tEXt` text string "is interpreted according to the Latin-1 character set" (§11.3.3.2), /// so a character above U+007F is **one** byte, not its UTF-8 pair. /// - /// Kills a mutant of [`Ancillary::text_entry`] that keeps the caller's `String` bytes: `é` - /// would be stored as `C3 A9`, which a conforming reader renders `é`. Asserted on the chunk - /// payload rather than through a decode, because this crate's decoder maps Latin-1 back + /// Kills a mutant of [`text_bytes`] that keeps the caller's `String` bytes: `é` would be + /// stored as `C3 A9`, which a conforming reader renders `é`. Asserted on the chunk payload + /// rather than through a decode, because this crate's decoder maps Latin-1 back /// code-point-for-code-point and would agree with the encoder either way. #[test] fn latin1_text_is_written_one_byte_per_character() { let mut a = Ancillary::default(); a.add_text_latin1("Author", "café ÿ"); - let mut out = vec![0u8; 8]; - a.write_post_plte(&mut out, DeflateEncoder::DEFAULT_EFFORT, RGB8); assert_eq!( - find_chunk(&out, b"tEXt"), + find_chunk(&post_plte(&a), b"tEXt"), Some(b"Author\0caf\xE9 \xFF".to_vec()) ); } @@ -992,14 +1171,13 @@ mod tests { /// encoded using the iTXt chunk." A `tEXt` request whose text has no Latin-1 encoding is /// therefore promoted rather than mangled or dropped. /// - /// Kills the `(TextKind::Latin1, None)` arm of [`Ancillary::text_entry`]. The keyword stays - /// Latin-1 either way (§11.3.3.1 binds it in every text chunk). + /// Kills the `None` arm of [`Ancillary::text_entry`]'s promotion. The keyword stays Latin-1 + /// either way (§11.3.3.1 binds it in every text chunk). #[test] fn text_outside_latin1_is_promoted_to_itxt() { let mut a = Ancillary::default(); a.add_text_latin1("Title", "字"); - let mut out = vec![0u8; 8]; - a.write_post_plte(&mut out, DeflateEncoder::DEFAULT_EFFORT, RGB8); + let out = post_plte(&a); assert_eq!(find_chunk(&out, b"tEXt"), None); // keyword, NUL, compression flag 0, method 0, empty language, empty translated keyword, // then the UTF-8 text (§11.3.3.4). @@ -1009,19 +1187,55 @@ mod tests { ); } + /// §11.3.3.1 restricts a `tEXt`/`zTXt` text string to "the printable Latin-1 character set + /// plus U+000A LINE FEED (LF)", and a control character is outside it — so it promotes, for + /// the same reason a Han character does. The character survives either way; only the chunk + /// that can define it changes. + /// + /// Kills [`text_repertoire`] mutated to accept everything Latin-1 can hold, which the looser + /// wording of §11.3.3.2 ("may contain any Latin-1 character") would otherwise excuse. 0x7F + /// DELETE is Latin-1-encodable and still not printable. + #[test] + fn a_control_character_promotes_the_annotation_to_itxt() { + let mut a = Ancillary::default(); + a.add_text_latin1("Title", "one\u{7F}two"); + let out = post_plte(&a); + assert_eq!(find_chunk(&out, b"tEXt"), None); + assert_eq!( + find_chunk(&out, b"iTXt"), + Some(b"Title\0\0\0\0\0one\x7Ftwo".to_vec()) + ); + } + + /// The other side of the same boundary: a line feed and the top of Latin-1 are *inside* the + /// repertoire §11.3.3.1 grants `tEXt`, so neither promotes. + /// + /// Kills [`text_repertoire`] mutated to drop its `'\n'` case or to stop at 0xFE, either of + /// which would push an ordinary multi-line Latin-1 note into an `iTXt`. + #[test] + fn a_line_feed_and_the_top_of_latin1_stay_in_a_text_chunk() { + let mut a = Ancillary::default(); + a.add_text_latin1("Description", "line\nÿ"); + let out = post_plte(&a); + assert_eq!(find_chunk(&out, b"iTXt"), None); + assert_eq!( + find_chunk(&out, b"tEXt"), + Some(b"Description\0line\n\xFF".to_vec()) + ); + } + /// Promoting a `zTXt` keeps the caller's *compression*, because §11.3.3.4 gives `iTXt` a /// compression flag of its own — only the character set had to change. /// - /// Kills the `(TextKind::Compressed, None)` arm of [`Ancillary::text_entry`] and the - /// compression-flag byte in [`write_text`]: a mutant that promotes to plain `International` - /// leaves the flag at 0 and the body uncompressed. + /// Kills the `Compressed` arm of [`TextKind::international`] and the compression-flag byte in + /// [`write_text`]: a mutant that promotes to plain `International` leaves the flag at 0 and + /// the body uncompressed. #[test] fn compressed_text_outside_latin1_stays_compressed_in_itxt() { let body = "字".repeat(200); let mut a = Ancillary::default(); a.add_text_compressed("Comment", &body); - let mut out = vec![0u8; 8]; - a.write_post_plte(&mut out, DeflateEncoder::DEFAULT_EFFORT, RGB8); + let out = post_plte(&a); assert_eq!(find_chunk(&out, b"zTXt"), None); let itxt = find_chunk(&out, b"iTXt").expect("promoted to iTXt"); assert_eq!(&itxt[..12], b"Comment\0\x01\0\0\0"); @@ -1032,49 +1246,190 @@ mod tests { ); } - /// §5.6 Table 5 states it on both rows — "If the iCCP chunk is present, the sRGB chunk should - /// not be present" and its converse — and §11.3.2.5 repeats it. Setting both is a question - /// only the caller can answer, so the encode is refused rather than one chunk silently - /// dropped. + /// §5.6 Table 5 and §11.3.2.5 say only that the two chunks "should not" appear together — + /// lowercase, and §15 gives the BCP 14 keywords force "when, and only when, they appear in + /// all capitals" — while §4.3 Table 1 presupposes the pair and ranks it. Both are written, so + /// no colour information the caller supplied is thrown away. /// - /// Kills the first guard of [`Ancillary::validate`]. Asserts the message, not `is_err`: the - /// second guard also rejects, so `is_err` alone would survive removing this one. + /// Kills a mutant that reinstates a refusal or drops one of the two chunks. #[test] - fn srgb_beside_iccp_is_refused() { + fn a_profile_and_a_rendering_intent_are_both_written() { let mut a = Ancillary::default(); a.set_srgb(SrgbIntent::Perceptual); - assert!(a.validate().is_ok(), "sRGB alone is fine"); - a.iccp = Some(("prof".to_string(), vec![0u8; 4])); - let error = a.validate().expect_err("sRGB beside iCCP"); + assert!(a.validate().is_ok(), "the pair is legal"); + + let mut out = vec![0u8; 8]; + a.write_pre_plte(&mut out, DeflateEncoder::DEFAULT_EFFORT, RGB8); + assert_eq!(find_chunk(&out, b"sRGB"), Some(vec![0])); assert!( - error.to_string().contains("sRGB and iCCP must not both"), - "{error}" + find_chunk(&out, b"iCCP").is_some(), + "the profile is written" ); + } + + /// §11.3.3.1: "Keywords are restricted to 1 to 79 bytes in length." Both edges, because an + /// empty keyword makes a third-party reader drop the whole annotation and an over-long one is + /// a chunk no conforming reader has to accept. + /// + /// Kills the length guard in [`keyword_bytes`], including a mutant that shifts either bound + /// by one. + #[test] + fn a_keyword_outside_one_to_seventy_nine_bytes_is_refused() { + let mut ok = Ancillary::default(); + ok.add_text_latin1(&"k".repeat(79), "body"); + ok.add_text_latin1("k", "body"); + assert!(ok.validate().is_ok(), "79 bytes and 1 byte are inside"); + + for keyword in ["", &"k".repeat(80)] { + let mut a = Ancillary::default(); + a.add_text_latin1(keyword, "body"); + assert!( + refusal(&a).contains("restricted to 1 to 79 bytes"), + "keyword of {} bytes", + keyword.len() + ); + } + } + + /// §11.3.3.1: "only code points 0x20-7E and 0xA1-FF are allowed", and expressly "nor is + /// U+00A0 NON-BREAKING SPACE since it is visually indistinguishable from an ordinary space". + /// The null is the same clause read through §11.3.3.2 — and the one that *corrupts* rather + /// than merely offends, because it is the field separator: `Auth\0or` re-parses as the + /// annotation `Auth`. + /// + /// Kills the repertoire guard in [`keyword_bytes`] and each edge of [`printable_latin1`]. + #[test] + fn a_keyword_outside_the_printable_latin1_repertoire_is_refused() { + for keyword in [ + "Auth\0or", // the field separator itself + "Auth\u{7F}", // DELETE + "Auth\u{9F}", // C1 control + "Auth\u{A0}", // NON-BREAKING SPACE, named by the clause + "题", // outside Latin-1 altogether + ] { + let mut a = Ancillary::default(); + a.add_text_latin1(keyword, "body"); + assert!( + refusal(&a).contains("code points 0x20-0x7E and 0xA1-0xFF"), + "keyword {keyword:?}" + ); + } - a.srgb = None; - assert!(a.validate().is_ok(), "iCCP alone is fine"); + let mut edges = Ancillary::default(); + edges.add_text_latin1("a\u{20}b\u{7E}\u{A1}\u{FF}", "body"); + assert!(edges.validate().is_ok(), "0x20, 0x7E, 0xA1 and 0xFF are in"); } - /// §11.3.3.1 binds the keyword to Latin-1 in all three text chunks, so — unlike the text, - /// which §11.3.3.2 routes to `iTXt` — a keyword outside it has no chunk at all. The entry is - /// not written, and the encode is refused rather than the annotation quietly disappearing. + /// §11.3.3.1: "leading spaces, trailing spaces, and consecutive spaces are not permitted in + /// keywords", so that a keyword cannot be misread as another. /// - /// Kills the keyword arm of [`Ancillary::text_entry`] and the second guard of - /// [`Ancillary::validate`]. Asserts the message for the same reason as the sRGB test. + /// Kills the spacing guard in [`keyword_bytes`], one condition at a time. #[test] - fn a_text_keyword_outside_latin1_is_refused() { + fn a_keyword_with_a_leading_trailing_or_consecutive_space_is_refused() { + for keyword in [" Author", "Author ", "Two Words"] { + let mut a = Ancillary::default(); + a.add_text_latin1(keyword, "body"); + assert!( + refusal(&a).contains("leading, trailing or consecutive space"), + "keyword {keyword:?}" + ); + } + + let mut ok = Ancillary::default(); + ok.add_text_latin1("Two Words", "body"); + assert!(ok.validate().is_ok(), "a single interior space is allowed"); + } + + /// §11.3.3.2: "Neither the keyword nor the text string may contain a null character", and + /// §11.3.3.4 the same for `iTXt`. This is corruption, not pedantry: the null is the field + /// separator, so `note\0Author\0other` written as a `tEXt` body re-parses as a *different* + /// annotation. Promotion cannot rescue it, because `iTXt` forbids it too. + /// + /// Kills the null guard in [`Ancillary::text_entry`], in both the Latin-1 and the UTF-8 + /// request — a mutant that checks only one leaves the other writing the corrupt chunk. + #[test] + fn a_null_in_a_text_string_is_refused() { + let mut latin1 = Ancillary::default(); + latin1.add_text_latin1("Note", "before\0after"); + assert!(refusal(&latin1).contains("may not contain a null character")); + + let mut utf8 = Ancillary::default(); + utf8.add_text_international("Note", "before\0after"); + assert!(refusal(&utf8).contains("may not contain a null character")); + } + + /// §11.3.3.4: "The translated keyword and text both use the UTF-8 encoding, and neither shall + /// contain a zero byte (null character)" — the translated keyword is null-terminated too, so + /// an embedded null re-frames everything after it. + /// + /// Kills the translated-keyword arm of [`itxt_field_fault`]. + #[test] + fn a_null_in_a_translated_keyword_is_refused() { let mut a = Ancillary::default(); - a.add_text_latin1("题", "body"); - assert!(a.texts.is_empty(), "the entry is not written"); + a.add_text_international_tagged("Note", "de", "No\0tiz", "body", false); + assert!(refusal(&a).contains("translated keyword may not contain a null")); + } + + /// §11.3.3.4: "The language tag is a well-formed language tag defined by [BCP47]", whose + /// subtags are ASCII letters and digits joined by hyphens. Anything else is not a tag, and — + /// written as UTF-8 into a field a reader takes as Latin-1 — would not even survive the trip. + /// + /// Kills the language arm of [`itxt_field_fault`], and the empty case pins that "unspecified" + /// stays legal. + #[test] + fn a_language_tag_outside_bcp_47_is_refused() { + for language in ["de\0DE", "zh_Hans", "dé"] { + let mut a = Ancillary::default(); + a.add_text_international_tagged("Note", language, "", "body", false); + assert!( + refusal(&a).contains("ASCII letters, digits and '-'"), + "language {language:?}" + ); + } - let error = a.validate().expect_err("keyword outside Latin-1"); + let mut ok = Ancillary::default(); + ok.add_text_international_tagged("Note", "", "", "body", false); + ok.add_text_international_tagged("Note", "ar-AE-u-nu-latn", "", "body", false); assert!( - error.to_string().contains("keyword must be Latin-1"), - "{error}" + ok.validate().is_ok(), + "empty and a full BCP 47 tag are fine" ); } + /// §11.3.3.4 gives the `iTXt` text field UTF-8 and no alternative, so a packet that is not + /// UTF-8 has no chunk to go in. It is refused rather than quietly discarded: the read side + /// surfaces a packet as raw bytes, and a caller that handed those bytes back is entitled to + /// learn they did not come out the other side. + /// + /// Kills the `Err` arm of [`Ancillary::add_xmp`] — with it gone the packet vanishes silently. + #[test] + fn a_non_utf8_xmp_packet_is_refused() { + let mut a = Ancillary::default(); + a.add_xmp(b""); + assert!(refusal(&a).contains("XMP packet is not UTF-8")); + + let mut valid = Ancillary::default(); + valid.add_xmp(b""); + assert!(valid.validate().is_ok(), "a UTF-8 packet is carried"); + assert!(find_chunk(&post_plte(&valid), b"iTXt").is_some()); + } + + /// A refusal a caller cannot act on is barely better than a silent drop, so it names *which* + /// annotation offended — its position and its keyword, escaped so a null shows up. + /// + /// Kills the `enumerate` and the owned detail in [`Ancillary::validate`]: with either gone + /// the message is the same for every annotation in the file. + #[test] + fn the_refusal_names_the_annotation_and_its_keyword() { + let mut a = Ancillary::default(); + a.add_text_latin1("Title", "fine"); + a.add_text_latin1("Author", "bad\0body"); + let message = refusal(&a); + assert!(message.contains("text annotation 1"), "{message}"); + assert!(message.contains(r#""Author""#), "{message}"); + } + /// §11.3.3.4's language tag and translated keyword survive, so carrying a decoded `iTXt` /// forward does not strip the two fields that make it international. /// @@ -1083,15 +1438,31 @@ mod tests { #[test] fn a_tagged_itxt_keeps_its_language_and_translated_keyword() { let mut a = Ancillary::default(); - a.add_text_international_tagged("Author", "de", "Autor", "gämut"); - let mut out = vec![0u8; 8]; - a.write_post_plte(&mut out, DeflateEncoder::DEFAULT_EFFORT, RGB8); + a.add_text_international_tagged("Author", "de", "Autor", "gämut", false); assert_eq!( - find_chunk(&out, b"iTXt"), + find_chunk(&post_plte(&a), b"iTXt"), Some(b"Author\0\0\0de\0Autor\0g\xC3\xA4mut".to_vec()) ); } + /// A carry replaces what an earlier carry contributed instead of appending a second copy, so + /// `with_metadata` is idempotent for text the way the single-value colour slots already are. + /// + /// Kills the `retain` in [`Ancillary::begin_carry`] (two copies of every annotation) and the + /// `carried` flag's `end_carry` reset (a carry that also eats the caller's own annotations). + #[test] + fn a_second_carry_replaces_the_first_and_spares_direct_setters() { + let mut a = Ancillary::default(); + a.add_text_latin1("Mine", "kept"); + for _ in 0..2 { + a.begin_carry(); + a.add_text_latin1("Carried", "once"); + a.end_carry(); + } + let keywords: Vec<&[u8]> = a.texts.iter().map(|e| e.keyword.as_slice()).collect(); + assert_eq!(keywords, [b"Mine".as_slice(), b"Carried".as_slice()]); + } + /// §11.3.2.6 Table 18 orders the payload primaries, transfer function, matrix coefficients, /// full-range flag — and fixes the matrix at 0 for PNG, so the setter has no argument for it. /// diff --git a/crates/gamut-png/src/decoded.rs b/crates/gamut-png/src/decoded.rs index 0b41153a..06551367 100644 --- a/crates/gamut-png/src/decoded.rs +++ b/crates/gamut-png/src/decoded.rs @@ -103,11 +103,34 @@ pub struct Cicp { pub full_range: bool, } +/// Which of §11.3.3's three chunks carried an annotation, and whether its text was compressed. +/// +/// The four combinations are the whole space PNG defines, so this enum is closed. It exists so a +/// re-encode can put an annotation back in the chunk it came out of: without it a `zTXt` is +/// indistinguishable from a `tEXt` once decoded, and rewriting a compressed 40-byte payload as an +/// uncompressed one can inflate it fortyfold — preservation that does not preserve. +/// +/// `#[repr(u8)]` with explicit, permanent discriminants: the value crosses the C ABI as a plain +/// integer. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +#[repr(u8)] +pub enum TextChunkKind { + /// `tEXt`: uncompressed Latin-1 (§11.3.3.2). + Text = 0, + /// `zTXt`: zlib-compressed Latin-1 (§11.3.3.3). + CompressedText = 1, + /// `iTXt` with the compression flag clear: uncompressed UTF-8 (§11.3.3.4). + International = 2, + /// `iTXt` with the compression flag set: zlib-compressed UTF-8 (§11.3.3.4). + CompressedInternational = 3, +} + /// One text annotation (tEXt/zTXt/iTXt, §11.3.3), decompressed where stored compressed. /// /// tEXt/zTXt hold Latin-1, mapped code-point-for-code-point into the `String` (lossless); -/// iTXt holds UTF-8. The XMP packet (`XML:com.adobe.xmp`) is surfaced as [`DecodedPng::xmp`], -/// not repeated here. +/// iTXt holds UTF-8. [`kind`](Self::kind) records which chunk it was, so a re-encode can put it +/// back in the same one. The XMP packet (`XML:com.adobe.xmp`) is surfaced as +/// [`DecodedPng::xmp`], not repeated here. #[derive(Debug, Clone, PartialEq, Eq)] #[non_exhaustive] pub struct TextChunk { @@ -119,6 +142,8 @@ pub struct TextChunk { pub language: Option, /// The iTXt translated keyword, if the chunk carried one. pub translated_keyword: Option, + /// The chunk this annotation was stored in, and whether its text was compressed. + pub kind: TextChunkKind, } /// Everything a PNG carries: the pixels in their native layout plus the ancillary payloads. @@ -340,8 +365,9 @@ pub(crate) fn collect(chunks: &[([u8; 4], &[u8])], budget: usize) -> PngMetadata meta } -/// The standard iTXt keyword carrying an XMP packet (XMP Specification Part 3). -const XMP_KEYWORD: &str = "XML:com.adobe.xmp"; +/// The standard iTXt keyword carrying an XMP packet (XMP Specification Part 3), reserved for it +/// by §11.3.3.1 Table 21. Shared with the encoder so the two sides cannot disagree on it. +pub(crate) const XMP_KEYWORD: &str = "XML:com.adobe.xmp"; /// A parsed iTXt: either the XMP packet or an ordinary text annotation. enum ITxt { @@ -403,7 +429,7 @@ fn parse_chrm(data: &[u8]) -> Option { }) } -/// tEXt (§11.3.3.3): keyword, NUL, Latin-1 text. +/// tEXt (§11.3.3.2): keyword, NUL, Latin-1 text. fn parse_text(data: &[u8]) -> Option { let (keyword, text) = split_keyword(data)?; Some(TextChunk { @@ -411,10 +437,11 @@ fn parse_text(data: &[u8]) -> Option { text: latin1(text), language: None, translated_keyword: None, + kind: TextChunkKind::Text, }) } -/// zTXt (§11.3.3.4): keyword, NUL, compression method 0, deflated Latin-1 text. +/// zTXt (§11.3.3.3): keyword, NUL, compression method 0, deflated Latin-1 text. fn parse_ztxt(data: &[u8], budget: &mut usize) -> Option { let (keyword, rest) = split_keyword(data)?; let (&method, compressed) = rest.split_first()?; @@ -427,10 +454,11 @@ fn parse_ztxt(data: &[u8], budget: &mut usize) -> Option { text: latin1(&text), language: None, translated_keyword: None, + kind: TextChunkKind::CompressedText, }) } -/// iTXt (§11.3.3.5): keyword, NUL, compression flag, compression method, language tag, NUL, +/// iTXt (§11.3.3.4): keyword, NUL, compression flag, compression method, language tag, NUL, /// translated keyword, NUL, UTF-8 text (deflated when the flag is 1). fn parse_itxt(data: &[u8], budget: &mut usize) -> Option { let (keyword, rest) = split_keyword(data)?; @@ -454,6 +482,11 @@ fn parse_itxt(data: &[u8], budget: &mut usize) -> Option { text: String::from_utf8(text_bytes).ok()?, language: Some(language).filter(|l| !l.is_empty()), translated_keyword: Some(translated).filter(|t| !t.is_empty()), + kind: if flag == 1 { + TextChunkKind::CompressedInternational + } else { + TextChunkKind::International + }, })) } diff --git a/crates/gamut-png/src/decoder.rs b/crates/gamut-png/src/decoder.rs index 562b4641..cc7cb071 100644 --- a/crates/gamut-png/src/decoder.rs +++ b/crates/gamut-png/src/decoder.rs @@ -1636,6 +1636,7 @@ mod tests { #[test] fn rich_decode_surfaces_metadata_and_native_image() { + use crate::SrgbIntent; use crate::decoded::PngImage; let (w, h) = (6u32, 4u32); @@ -1646,8 +1647,7 @@ mod tests { let mut png = Vec::new(); PngEncoder::new() .with_gamma(1.0 / 2.2) - // cICP, not sRGB: the encoder refuses sRGB beside the iCCP this fixture needs - // (§5.6 Table 5, §11.3.2.5), while cICP is legal alongside it (§4.3 Table 1). + .with_srgb(SrgbIntent::Perceptual) .with_cicp(9, 16, true) .with_chromaticities((0.3127, 0.3290), (0.64, 0.33), (0.30, 0.60), (0.15, 0.06)) .with_exif(&exif) @@ -1671,7 +1671,7 @@ mod tests { other => panic!("expected Rgb8, got {other:?}"), } assert_eq!(decoded.gamma, Some(45455)); - assert!(decoded.srgb.is_none()); + assert_eq!(decoded.srgb, Some(SrgbIntent::Perceptual)); let chrm = decoded.chromaticities.unwrap(); assert_eq!(chrm.white, (31270, 32900)); assert_eq!(chrm.blue, (15000, 6000)); diff --git a/crates/gamut-png/src/encoder.rs b/crates/gamut-png/src/encoder.rs index fd261996..716b839b 100644 --- a/crates/gamut-png/src/encoder.rs +++ b/crates/gamut-png/src/encoder.rs @@ -31,7 +31,9 @@ use crate::ancillary::{ use crate::backend::{IdatDeflater, IdatInfo, Registry, run_deflaters}; use crate::chunk::{self, C2paSpan, SIGNATURE}; use crate::color::ColorType; -use crate::decoded::{Chromaticities, Cicp, DecodedPng, IccProfile, PngMetadata, TextChunk}; +use crate::decoded::{ + Chromaticities, Cicp, DecodedPng, IccProfile, PngMetadata, TextChunk, TextChunkKind, +}; use crate::filter::{self, FilterStrategy, FilterType}; use crate::palette::PngPalette; use crate::reduce::{self, Reduced, Reductions}; @@ -90,6 +92,56 @@ struct MetadataView<'a> { chromaticities: Option, srgb: Option, cicp: Option, + /// Whether the source carried a C2PA manifest store. Only the presence is needed: a store is + /// never carried, but a caller has to be told it was left behind. + c2pa: bool, +} + +/// A metadata payload [`PngEncoder::with_metadata`] could not carry into the output. +/// +/// Preservation exists to stop metadata disappearing quietly, so the two payloads a carry cannot +/// take are named rather than dropped in silence. Read them back with +/// [`PngEncoder::dropped_metadata`] and tell the user — `gamut convert` does. +/// +/// `#[repr(u8)]` with explicit discriminants, which are permanent and append-only: the value +/// crosses the C ABI as a plain integer. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +#[repr(u8)] +#[non_exhaustive] +pub enum DroppedMetadata { + /// A `cICP` whose matrix coefficients are not 0. §11.3.2.6 requires 0 for PNG — "RGB is + /// currently the only supported color model in PNG, and as such Matrix Coefficients shall be + /// set to 0" — so the source chunk is not conforming and copying it forward would reproduce + /// the defect in a file this encoder signed off on. + NonRgbCicp = 0, + /// The C2PA manifest store (`caBX`). A store is signed over the exact bytes of the file it + /// was made for, which is why C2PA 2.4 §A.3.2 marks the chunk unsafe to copy: carried into a + /// re-encode it is invalid by construction, and a validator reports a *tampered* file rather + /// than an unsigned one. Re-sign the output and set it with + /// [`with_c2pa`](PngEncoder::with_c2pa). + C2paManifestStore = 1, +} + +impl DroppedMetadata { + /// One line naming what was left behind and why, fit to show a user. + #[must_use] + pub fn reason(self) -> &'static str { + match self { + Self::NonRgbCicp => { + "cICP: its matrix coefficients are not 0, which PNG requires (§11.3.2.6)" + } + Self::C2paManifestStore => { + "C2PA manifest store: signed over the source bytes, so a copy would be invalid \ + (C2PA 2.4 §A.3.2) — re-sign the output" + } + } + } +} + +impl core::fmt::Display for DroppedMetadata { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + f.write_str(self.reason()) + } } /// A reusable PNG encoder. @@ -102,6 +154,9 @@ pub struct PngEncoder { auto_reduce: bool, clean_transparent: bool, backends: Registry, + /// What the last metadata carry could not take, in the order it was found. Reset by each + /// [`Self::with_metadata`] / [`Self::with_metadata_from`] call, so it describes that call. + dropped: Vec, } impl Default for PngEncoder { @@ -123,6 +178,7 @@ impl PngEncoder { auto_reduce: false, clean_transparent: false, backends: Registry::default(), + dropped: Vec::new(), } } @@ -225,12 +281,14 @@ impl PngEncoder { self } - /// Records the standard colour-space rendering intent (sRGB chunk). + /// Records the standard colour-space rendering intent (sRGB chunk, §11.3.2.5). /// - /// Mutually exclusive with [`with_icc_profile`](Self::with_icc_profile): PNG §5.6 Table 5 and - /// §11.3.2.5 both say the two chunks should not appear together, so setting both makes the - /// encode fail with [`Error::InvalidInput`] rather than write a file the standard tells - /// encoders not to produce. [`with_metadata`](Self::with_metadata) resolves the pair for you. + /// May be combined with [`with_icc_profile`](Self::with_icc_profile). §5.6 Table 5 and + /// §11.3.2.5 say only that the two "should not" appear together — lowercase, and §15 gives + /// the BCP 14 keywords force "when, and only when, they appear in all capitals" — while §4.3 + /// Table 1 presupposes the pair and settles it, ranking `iCCP` (priority 2) above `sRGB` + /// (3). Both are written; a reader honours the profile and treats the intent as the fallback + /// for readers that cannot apply one. #[must_use] pub fn with_srgb(mut self, intent: SrgbIntent) -> Self { self.ancillary.set_srgb(intent); @@ -395,13 +453,11 @@ impl PngEncoder { self } - /// Embeds an ICC colour profile (iCCP chunk), zlib-compressed. `profile` is the raw ICC profile - /// — for example the bytes produced by `gamut-icc`. + /// Embeds an ICC colour profile (iCCP chunk, §11.3.2.3), zlib-compressed. `profile` is the + /// raw ICC profile — for example the bytes produced by `gamut-icc`. /// - /// Mutually exclusive with [`with_srgb`](Self::with_srgb): PNG §5.6 Table 5 and §11.3.2.5 both - /// say the two chunks should not appear together, so setting both makes the encode fail with - /// [`Error::InvalidInput`] rather than write a file the standard tells encoders not to - /// produce. [`with_metadata`](Self::with_metadata) resolves the pair for you. + /// May be combined with [`with_srgb`](Self::with_srgb); see there for why the pair is + /// written rather than refused, and which chunk a reader honours. #[must_use] pub fn with_icc_profile(mut self, name: &str, profile: &[u8]) -> Self { self.ancillary.iccp = Some((name.to_string(), profile.to_vec())); @@ -412,8 +468,7 @@ impl PngEncoder { /// the XMP/RDF document — for example the bytes produced by `gamut-xmp`. #[must_use] pub fn with_xmp(mut self, xmp: &str) -> Self { - self.ancillary - .add_text_international("XML:com.adobe.xmp", xmp); + self.ancillary.add_xmp(xmp.as_bytes()); self } @@ -426,26 +481,32 @@ impl PngEncoder { /// [`with_metadata_from`](Self::with_metadata_from) is the same thing for a full /// [`DecodedPng`]. /// + /// Calling it twice with the same metadata is the same as calling it once: a later carry + /// replaces what an earlier one contributed rather than appending a second copy of every + /// annotation. + /// /// # What it carries, and what it deliberately does not /// - /// Everything the read side surfaces is set, with three spec-driven adjustments: + /// Everything the read side surfaces is set, including a `cICP`, an `sRGB` and an `iCCP` + /// together — §4.3 Table 1 ranks the colour chunks precisely so a file may carry more than + /// one, and a reader honours the lowest priority number. Each text annotation goes back into + /// the chunk it came out of, compressed if it was compressed + /// ([`TextChunkKind`](crate::TextChunkKind)). + /// + /// Two payloads cannot be carried, and both are **named** rather than dropped in silence — + /// read them back with [`dropped_metadata`](Self::dropped_metadata): /// - /// - **`iCCP` and `sRGB` are resolved, not both written.** §4.3 Table 1 ranks the colour - /// chunks and a reader takes the lowest priority number, so the ICC profile (priority 2) - /// wins over the rendering intent (priority 3) and the `sRGB` chunk is dropped — which is - /// exactly the chunk a conforming reader would have ignored. Writing both is refused (§5.6 - /// Table 5, §11.3.2.5); this method is how a file carrying both is re-encoded at all. - /// - **A `cICP` whose matrix coefficients are not 0 is dropped.** §11.3.2.6 requires 0 for - /// PNG, so such a chunk is not conforming and copying it forward would reproduce the defect. - /// - **The C2PA manifest store is never carried.** A store is signed over the exact bytes of - /// the file it was made for, so copying it into a re-encode invalidates it by construction - /// — which is why `caBX` is *unsafe to copy* (C2PA 2.4 §A.3.2). Re-sign the output and set - /// it with [`with_c2pa`](Self::with_c2pa). + /// - a **`cICP` whose matrix coefficients are not 0**, which §11.3.2.6 does not allow in PNG; + /// - the **C2PA manifest store**, signed over the bytes of the file it was made for. /// - /// Two further limits are the read side's, not this method's: `pHYs`, `tIME`, `sBIT` and - /// `bKGD` are not part of [`PngMetadata`], so they cannot be carried here (set them with - /// their own builder methods); and a `zTXt` is indistinguishable from a `tEXt` once decoded, - /// so a compressed annotation is rewritten uncompressed. Neither loses any text. + /// Anything that would be *corrupted* rather than lost — a keyword outside §11.3.3.1's + /// repertoire, a null inside a text string, an XMP packet that is not UTF-8 — makes the + /// encode fail with [`Error::InvalidInput`] naming the annotation, rather than being written + /// as something a reader reads back differently. + /// + /// One further limit is the read side's, not this method's: `pHYs`, `tIME`, `sBIT` and `bKGD` + /// are not part of [`PngMetadata`], so they cannot be carried here (set them with their own + /// builder methods). #[must_use] pub fn with_metadata(self, metadata: &PngMetadata) -> Self { self.with_metadata_view(MetadataView { @@ -457,6 +518,7 @@ impl PngEncoder { chromaticities: metadata.chromaticities, srgb: metadata.srgb, cicp: metadata.cicp, + c2pa: metadata.c2pa.is_some(), }) } @@ -476,31 +538,58 @@ impl PngEncoder { chromaticities: decoded.chromaticities, srgb: decoded.srgb, cicp: decoded.cicp, + c2pa: decoded.c2pa.is_some(), }) } + /// What the last [`with_metadata`](Self::with_metadata) / + /// [`with_metadata_from`](Self::with_metadata_from) call could not carry, in the order it was + /// found — empty when it carried everything, and reset by each call. + /// + /// Surface this to whoever asked for the re-encode. Losing metadata without saying so is the + /// defect the preservation path exists to remove; losing it *with* an explanation is a + /// choice the spec forces. + #[must_use] + pub fn dropped_metadata(&self) -> &[DroppedMetadata] { + &self.dropped + } + /// The one implementation behind [`with_metadata`](Self::with_metadata) and /// [`with_metadata_from`](Self::with_metadata_from). fn with_metadata_view(mut self, meta: MetadataView<'_>) -> Self { + self.dropped.clear(); + self.ancillary.begin_carry(); if let Some(exif) = meta.exif { self = self.with_exif(exif); } - // §4.3 Table 1: the reader honours the lowest priority number, iCCP (2) over sRGB (3). - // Writing both is what `Ancillary::validate` refuses, so pick the one that would have - // been honoured rather than hand the caller an error it cannot act on. - match (meta.icc_profile, meta.srgb) { - (Some(icc), _) => self = self.with_icc_profile(&icc.name, &icc.profile), - (None, Some(intent)) => self = self.with_srgb(intent), - (None, None) => {} + // Both colour statements are carried. §5.6 Table 5 and §11.3.2.5 only *recommend* against + // the pair, and §4.3 Table 1 exists to resolve it: `iCCP` outranks `sRGB`, so the profile + // is what a reader applies and the intent is what a reader without a CMM falls back on. + // Dropping either would throw away colour information the source carried. + if let Some(icc) = meta.icc_profile { + self = self.with_icc_profile(&icc.name, &icc.profile); } - // §11.3.2.6: "Matrix Coefficients shall be set to 0". A source chunk that says otherwise - // is not a conforming cICP; carrying it forward would put the same defect in the output. - if let Some(cicp) = meta.cicp.filter(|cicp| cicp.matrix_coefficients == 0) { - self = self.with_cicp( - cicp.color_primaries, - cicp.transfer_function, - cicp.full_range, - ); + if let Some(intent) = meta.srgb { + self = self.with_srgb(intent); + } + match meta.cicp { + // §11.3.2.6: "Matrix Coefficients shall be set to 0". A source chunk that says + // otherwise is not a conforming cICP; carrying it forward would put the same defect + // in the output. + Some(cicp) if cicp.matrix_coefficients != 0 => { + self.dropped.push(DroppedMetadata::NonRgbCicp); + } + Some(cicp) => { + self = self.with_cicp( + cicp.color_primaries, + cicp.transfer_function, + cicp.full_range, + ); + } + None => {} + } + if meta.c2pa { + self.dropped.push(DroppedMetadata::C2paManifestStore); } // Set in the stored ×100 000 fixed-point units rather than through `with_gamma` / // `with_chromaticities`, whose `f64` arguments would round-trip the value through a @@ -520,25 +609,41 @@ impl PngEncoder { chrm.blue.1, ]); } - // The XMP packet is UTF-8 by §11.3.3.4; bytes that are not are not a packet this encoder - // can frame, and are dropped rather than written as an invalid iTXt. - if let Some(xmp) = meta.xmp.and_then(|bytes| str::from_utf8(bytes).ok()) { - self = self.with_xmp(xmp); + // Handed over as bytes, because that is what the chunk held. §11.3.3.4 requires UTF-8, so + // a packet that is not gets a refusal at `encode` naming it — never a silent drop. + if let Some(xmp) = meta.xmp { + self.ancillary.add_xmp(xmp); } for text in meta.texts { - match (&text.language, &text.translated_keyword) { - // Neither field set: the annotation came from a tEXt/zTXt, or from an iTXt whose - // two optional fields were empty. Offer it as Latin-1 — which is byte-exact for - // the first case — and let `Ancillary` promote it to iTXt if the text needs it. - (None, None) => self.ancillary.add_text_latin1(&text.keyword, &text.text), - (language, translated) => self.ancillary.add_text_international_tagged( + let (language, translated) = ( + text.language.as_deref().unwrap_or_default(), + text.translated_keyword.as_deref().unwrap_or_default(), + ); + match text.kind { + TextChunkKind::Text => self.ancillary.add_text_latin1(&text.keyword, &text.text), + TextChunkKind::CompressedText => { + self.ancillary + .add_text_compressed(&text.keyword, &text.text); + } + TextChunkKind::International => self.ancillary.add_text_international_tagged( &text.keyword, - language.as_deref().unwrap_or_default(), - translated.as_deref().unwrap_or_default(), + language, + translated, &text.text, + false, ), + TextChunkKind::CompressedInternational => { + self.ancillary.add_text_international_tagged( + &text.keyword, + language, + translated, + &text.text, + true, + ); + } } } + self.ancillary.end_carry(); self } diff --git a/crates/gamut-png/src/lib.rs b/crates/gamut-png/src/lib.rs index 9ddb73a7..ab7fdce5 100644 --- a/crates/gamut-png/src/lib.rs +++ b/crates/gamut-png/src/lib.rs @@ -91,13 +91,14 @@ pub use chunk::{C2paSpan, fill_c2pa}; pub use color::ColorType; pub use decoded::{ Chromaticities, Cicp, DecodedPng, IccProfile, PngHeader, PngImage, PngMetadata, TextChunk, + TextChunkKind, }; pub use decoder::{PngDecoder, TransparencyKey, metadata}; pub use deconstruct::{ ChunkStats, DEFAULT_MAX_CHUNKS, DeconstructLimits, FilterHistogram, FilterScan, PassStats, PngReport, Segment, SegmentKind, SkippedFilterScan, deconstruct, deconstruct_with_limits, }; -pub use encoder::{PngEncodeReport, PngEncoder}; +pub use encoder::{DroppedMetadata, PngEncodeReport, PngEncoder}; pub use filter::{FilterStrategy, FilterType}; /// The DEFLATE compression level, accepted by [`PngEncoder::with_compression`]. pub use gamut_deflate::Level; diff --git a/crates/gamut-png/tests/c2pa.rs b/crates/gamut-png/tests/c2pa.rs index 86d18e99..fc58e84f 100644 --- a/crates/gamut-png/tests/c2pa.rs +++ b/crates/gamut-png/tests/c2pa.rs @@ -15,7 +15,8 @@ use common::{ }; use gamut_core::{DecodeImage, Dimensions, EncodeImage, ImageBuf, ImageRef, Indexed8, Rgb8, Rgba8}; use gamut_png::{ - PhysicalUnit, PngDecoder, PngEncoder, PngPalette, SegmentKind, deconstruct, fill_c2pa, + PhysicalUnit, PngDecoder, PngEncoder, PngPalette, SegmentKind, SrgbIntent, deconstruct, + fill_c2pa, }; /// A stand-in manifest store of `len` bytes: not all zero, no two runs alike, so a fill is @@ -64,10 +65,7 @@ fn rgb_source() -> (Vec, Dimensions) { fn everything_else() -> PngEncoder { PngEncoder::new() .with_gamma(1.0 / 2.2) - // cICP rather than sRGB: §5.6 Table 5 and §11.3.2.5 say sRGB and iCCP must not both - // be written, and iCCP is the one whose payload has a size the store's placement depends - // on. cICP is legal alongside it (§4.3 Table 1 only ranks them). - .with_cicp(9, 16, true) + .with_srgb(SrgbIntent::Perceptual) .with_chromaticities((0.3127, 0.3290), (0.64, 0.33), (0.30, 0.60), (0.15, 0.06)) .with_icc_profile("Tiny", &tiny_icc_profile()) .with_significant_bits(&[8, 8, 8, 8]) diff --git a/crates/gamut-png/tests/metadata.rs b/crates/gamut-png/tests/metadata.rs index d276a32b..501498a7 100644 --- a/crates/gamut-png/tests/metadata.rs +++ b/crates/gamut-png/tests/metadata.rs @@ -11,7 +11,7 @@ use common::{ chunk, ihdr_payload, minimal_png, png_from_chunks, tiny_exif, tiny_icc_profile, zlib, }; use gamut_core::{Dimensions, EncodeImage, ErrorKind, ImageRef, Rgb8}; -use gamut_png::{PngDecoder, PngEncoder, PngMetadata}; +use gamut_png::{PngDecoder, PngEncoder, PngMetadata, SrgbIntent}; /// A 2×2 RGB8 source for the encoder-driven tests. fn source() -> Vec { @@ -50,8 +50,7 @@ fn every_carrier_round_trips_byte_exact() { .with_compressed_text("Comment", "compressed comment") .with_international_text("Title", "international title") .with_gamma(1.0 / 2.2) - // cICP rather than sRGB, which §5.6 Table 5 and §11.3.2.5 forbid beside the iCCP - // this file also carries; sRGB's own carriage is pinned by `roundtrip.rs`. + .with_srgb(SrgbIntent::RelativeColorimetric) .with_cicp(9, 16, true) .with_chromaticities( (0.3127, 0.3290), @@ -69,6 +68,7 @@ fn every_carrier_round_trips_byte_exact() { assert_eq!(meta.xmp.as_deref(), Some(xmp.as_bytes())); assert_eq!(meta.c2pa.as_deref(), Some(&c2pa[..])); assert_eq!(meta.gamma, Some(45_455)); + assert_eq!(meta.srgb, Some(SrgbIntent::RelativeColorimetric)); let cicp = meta.cicp.expect("cICP present"); assert_eq!( ( @@ -95,34 +95,27 @@ fn every_carrier_round_trips_byte_exact() { /// and not the other fails here. #[test] fn metadata_agrees_with_decode_field_for_field() { - // Built chunk by chunk rather than by the encoder, so that *every* field is populated: the - // encoder refuses sRGB beside iCCP (§5.6 Table 5, §11.3.2.5), and a comparison of two `None`s - // would not see a chunk wired into one walk and not the other. A reader still meets such a - // file, and §13.1 says an ancillary chunk it cannot use is skipped, not fatal. let exif = tiny_exif(); let icc = tiny_icc_profile(); - let mut iccp = b"Tiny\0\0".to_vec(); - iccp.extend_from_slice(&zlib(&icc)); - let mut chrm = Vec::new(); - for coord in [ - 31_270u32, 32_900, 64_000, 33_000, 30_000, 60_000, 15_000, 6_000, - ] { - chrm.extend_from_slice(&coord.to_be_bytes()); - } - let png = png_from_chunks(&[ - chunk(b"IHDR", &ihdr_payload(3, 2, 8, 2, 0)), - chunk(b"eXIf", &exif), - chunk(b"iCCP", &iccp), - chunk(b"sRGB", &[1]), - chunk(b"cICP", &[1, 13, 0, 1]), - chunk(b"gAMA", &45_455u32.to_be_bytes()), - chunk(b"cHRM", &chrm), - chunk(b"tEXt", b"Author\0nobody"), - chunk(b"iTXt", b"XML:com.adobe.xmp\0\0\0\0\0"), - chunk(b"caBX", b"\0\0\0\x10jumbc2pa"), - chunk(b"IDAT", &zlib(&[0u8; 20])), - chunk(b"IEND", &[]), - ]); + let png = encode(|e| { + e.with_exif(&exif) + .with_icc_profile("Tiny", &icc) + .with_xmp("") + .with_c2pa(b"\0\0\0\x10jumbc2pa") + .with_text("Author", "nobody") + .with_gamma(1.0 / 2.2) + .with_chromaticities( + (0.3127, 0.3290), + (0.6400, 0.3300), + (0.3000, 0.6000), + (0.1500, 0.0600), + ) + // Every colour chunk at once, including the sRGB/iCCP pair §4.3 Table 1 ranks: a + // comparison of two `None`s would not see a chunk wired into one walk and not the + // other. + .with_srgb(SrgbIntent::Perceptual) + .with_cicp(1, 13, true) + }); let meta = gamut_png::metadata(&png).unwrap(); let decoded = PngDecoder::new().decode(&png).unwrap(); diff --git a/crates/gamut-png/tests/oracle.rs b/crates/gamut-png/tests/oracle.rs index f2f8b478..fe6d2a57 100644 --- a/crates/gamut-png/tests/oracle.rs +++ b/crates/gamut-png/tests/oracle.rs @@ -322,6 +322,47 @@ fn ancillary_chunks_are_accepted_by_libpng() { assert_eq!(dec.pixels, src); } +/// The reference reader is the arbiter of whether a file carrying **both** colour chunks is a +/// file at all. §5.6 Table 5 and §11.3.2.5 say only that `sRGB` "should not" appear beside +/// `iCCP` — lowercase, and §15 gives the BCP 14 keywords force "when, and only when, they appear +/// in all capitals" — while §4.3 Table 1 presupposes the pair and ranks it. libpng reads the +/// datastream and returns the same pixels, so `PngEncoder::with_metadata` carrying both loses a +/// caller nothing. +/// +/// Note the oracle's own limit: `libpng_oracle::decode` sets `png_set_benign_errors` and drops +/// warnings, so what this pins is that the pair is not a *critical* error and the image survives +/// it, not that libpng raised no warning (issue #502), and it reads no chunk back (issue #572). +#[test] +fn a_profile_beside_a_rendering_intent_is_accepted_by_libpng() { + let (w, h) = (12u32, 12u32); + let src = rgb_pattern(w, h); + let dims = Dimensions::new(w, h).unwrap(); + let mut icc = vec![0u8; 132]; + icc[0..4].copy_from_slice(&132u32.to_be_bytes()); + icc[8..12].copy_from_slice(&0x0210_0000u32.to_be_bytes()); + icc[12..16].copy_from_slice(b"mntr"); + icc[16..20].copy_from_slice(b"RGB "); + icc[20..24].copy_from_slice(b"XYZ "); + icc[36..40].copy_from_slice(b"acsp"); + + let mut png = Vec::new(); + PngEncoder::new() + .with_icc_profile("both", &icc) + .with_srgb(SrgbIntent::Perceptual) + .encode_image(ImageRef::::new(&src, dims).unwrap(), &mut png) + .expect("encode"); + + assert!(contains_chunk(&png, b"iCCP"), "iCCP present"); + assert!(contains_chunk(&png, b"sRGB"), "sRGB present"); + assert_eq!(libpng_oracle::decode(&png).pixels, src); + + // gamut's own reader sees both too, which is what makes carrying them preservation rather + // than duplication. + let meta = gamut_png::metadata(&png).expect("read back"); + assert_eq!(meta.srgb, Some(SrgbIntent::Perceptual)); + assert_eq!(meta.icc_profile.expect("profile").profile, icc); +} + #[test] fn metadata_chunks_embed_and_image_survives() { let (w, h) = (12u32, 12u32); diff --git a/crates/gamut-png/tests/preservation.rs b/crates/gamut-png/tests/preservation.rs index 4e32fb15..79472e2c 100644 --- a/crates/gamut-png/tests/preservation.rs +++ b/crates/gamut-png/tests/preservation.rs @@ -1,16 +1,16 @@ //! `PngEncoder::with_metadata` / `with_metadata_from` (issue #483): what a re-encode carries //! forward from the file it rewrites, and what it deliberately does not. //! -//! Example and drift-guard level. No oracle: the claim is about gamut's own read→write seam, and -//! the source files are built chunk by chunk from `common` so a fixture can carry combinations -//! this encoder refuses to write — notably `sRGB` beside `iCCP`, which §5.6 Table 5 and §11.3.2.5 -//! tell encoders not to produce but which a reader still meets. +//! Example and drift-guard level, over gamut's own read→write seam. The source files are built +//! chunk by chunk from `common` so a fixture can carry exactly the combination each claim is +//! about, without the encoder's own choices standing in the way. That a re-encode's output is a +//! file the *reference* reader accepts is `tests/oracle.rs`'s job, not this file's. mod common; use common::{chunk, ihdr_payload, png_from_chunks, tiny_exif, tiny_icc_profile, zlib}; -use gamut_core::{Dimensions, EncodeImage, ImageRef, Rgb8}; -use gamut_png::{PngDecoder, PngEncoder, PngMetadata, SrgbIntent}; +use gamut_core::{Dimensions, EncodeImage, ErrorKind, ImageRef, Rgb8}; +use gamut_png::{DroppedMetadata, PngDecoder, PngEncoder, PngMetadata, SrgbIntent}; /// The `cHRM` payload for the sRGB primaries, in the ×100 000 units §11.3.2.1 stores. const CHRM: [u32; 8] = [ @@ -45,14 +45,42 @@ fn source(extra: &[Vec]) -> Vec { png_from_chunks(&chunks) } -/// Re-encodes a 2×2 image under `build`, and reads back what the output carries. -fn re_encoded(build: impl FnOnce(PngEncoder) -> PngEncoder) -> PngMetadata { +/// A source carrying only `extra` between the header and the image data — for a claim about one +/// annotation, which the full [`source`] pile would confuse with its own. +fn minimal_source(extra: &[Vec]) -> Vec { + let mut chunks = vec![chunk(b"IHDR", &ihdr_payload(3, 2, 8, 2, 0))]; + chunks.extend_from_slice(extra); + chunks.push(chunk(b"IDAT", &zlib(&[0u8; 20]))); + chunks.push(chunk(b"IEND", &[])); + png_from_chunks(&chunks) +} + +/// Re-encodes a 2×2 image under `build`, returning the output bytes. +fn re_encoded_bytes(build: impl FnOnce(PngEncoder) -> PngEncoder) -> Vec { let pixels = vec![0u8; 3 * 4]; let image = ImageRef::::new(&pixels, Dimensions::new(2, 2).unwrap()).unwrap(); - let png = build(PngEncoder::new()) + build(PngEncoder::new()) .encode_to_vec(image) - .expect("re-encode"); - gamut_png::metadata(&png).expect("read back") + .expect("re-encode") +} + +/// Re-encodes a 2×2 image under `build`, and reads back what the output carries. +fn re_encoded(build: impl FnOnce(PngEncoder) -> PngEncoder) -> PngMetadata { + gamut_png::metadata(&re_encoded_bytes(build)).expect("read back") +} + +/// The payload of the first chunk of type `ty`, for a claim about which *chunk* carries an +/// annotation rather than what text it holds — the distinction a decode erases. +fn chunk_payload(png: &[u8], ty: &[u8; 4]) -> Option> { + let mut i = 8; // past the signature + while i + 12 <= png.len() { + let len = u32::from_be_bytes([png[i], png[i + 1], png[i + 2], png[i + 3]]) as usize; + if &png[i + 4..i + 8] == ty { + return Some(png[i + 8..i + 8 + len].to_vec()); + } + i += 12 + len; + } + None } /// The headline claim of #483: nothing the read side surfaced is dropped on the way back out. @@ -92,35 +120,23 @@ fn an_itxt_keeps_its_language_and_translated_keyword() { assert_eq!(note.translated_keyword.as_deref(), Some("Notiz")); } -/// §4.3 Table 1 ranks the colour chunks and a reader honours the lowest priority number, so of a -/// source carrying both the `iCCP` (2) is the chunk that was being used and the `sRGB` (3) the -/// chunk that was being ignored. Carrying both would be the pair §5.6 Table 5 and §11.3.2.5 -/// refuse, and would make the file unencodable. +/// A source may legally carry both, and both are kept. §5.6 Table 5 and §11.3.2.5 say only that +/// `sRGB` and `iCCP` "should not" appear together — lowercase, and §15 gives the BCP 14 keywords +/// force "when, and only when, they appear in all capitals" — while §4.3 Table 1 presupposes the +/// pair and ranks it, `iCCP` (2) over `sRGB` (3). Dropping either would lose colour information +/// the source carried, which is exactly what this preservation path exists to stop. +/// +/// That the result is a file the reference reader accepts is pinned against libpng in +/// `tests/oracle.rs`. #[test] -fn srgb_gives_way_to_an_icc_profile_from_the_same_file() { +fn a_profile_and_a_rendering_intent_are_both_carried() { let meta = gamut_png::metadata(&source(&[chunk(b"sRGB", &[1])])).unwrap(); assert_eq!(meta.srgb, Some(SrgbIntent::RelativeColorimetric)); assert!(meta.icc_profile.is_some(), "the source carries both"); let re = re_encoded(|e| e.with_metadata(&meta)); - assert!(re.icc_profile.is_some(), "the ICC profile is kept"); - assert!(re.srgb.is_none(), "the lower-priority sRGB is dropped"); -} - -/// The converse: with no ICC profile to outrank it, the rendering intent is the colour -/// information the file has, and dropping it would lose it. -#[test] -fn srgb_is_carried_when_no_icc_profile_outranks_it() { - let png = png_from_chunks(&[ - chunk(b"IHDR", &ihdr_payload(3, 2, 8, 2, 0)), - chunk(b"sRGB", &[2]), - chunk(b"IDAT", &zlib(&[0u8; 20])), - chunk(b"IEND", &[]), - ]); - let meta = gamut_png::metadata(&png).unwrap(); - - let re = re_encoded(|e| e.with_metadata(&meta)); - assert_eq!(re.srgb, Some(SrgbIntent::Saturation)); + assert_eq!(re.icc_profile, meta.icc_profile); + assert_eq!(re.srgb, meta.srgb); } /// §11.3.2.6: "RGB is currently the only supported color model in PNG, and as such Matrix @@ -145,7 +161,16 @@ fn a_cicp_is_carried_only_when_its_matrix_coefficients_are_zero() { let non_rgb = gamut_png::metadata(&source(&[chunk(b"cICP", &[9, 16, 1, 1])])).unwrap(); assert!(non_rgb.cicp.is_some(), "the source carries it"); - assert!(re_encoded(|e| e.with_metadata(&non_rgb)).cicp.is_none()); + let encoder = PngEncoder::new().with_metadata(&non_rgb); + assert!(re_encoded(|_| encoder.clone()).cicp.is_none()); + // Dropped, but not in silence: the caller can say so. + assert!( + encoder + .dropped_metadata() + .contains(&DroppedMetadata::NonRgbCicp), + "{:?}", + encoder.dropped_metadata() + ); } /// Drift guard. A C2PA manifest store is signed over the exact bytes of the file it was made for, @@ -157,7 +182,12 @@ fn the_c2pa_manifest_store_is_never_carried_forward() { let meta = gamut_png::metadata(&source(&[])).unwrap(); assert!(meta.c2pa.is_some(), "the source carries a store"); - assert!(re_encoded(|e| e.with_metadata(&meta)).c2pa.is_none()); + let encoder = PngEncoder::new().with_metadata(&meta); + assert!(re_encoded(|_| encoder.clone()).c2pa.is_none()); + assert_eq!( + encoder.dropped_metadata(), + [DroppedMetadata::C2paManifestStore] + ); } /// The two entry points differ only in which read surface they take, so a field wired into one @@ -176,3 +206,93 @@ fn with_metadata_from_agrees_with_with_metadata() { assert!(from_decoded.icc_profile.is_some() && from_decoded.cicp.is_some()); assert!(!from_decoded.texts.is_empty() && from_decoded.exif.is_some()); } + +/// §11.3.3.3 makes a `zTXt` "semantically equivalent" to a `tEXt`, so a decode that keeps only the +/// text loses no *words* — but rewriting a compressed annotation uncompressed is still not +/// preservation: the fixture's 1 600-byte body is a 40-byte chunk in the source, and a re-encode +/// that forgets which chunk it came from writes it back forty times larger. +/// +/// Kills the `CompressedText` arm of `with_metadata_view`'s routing, and any mutant that collapses +/// [`TextChunkKind`](gamut_png::TextChunkKind) to one value. +#[test] +fn a_compressed_annotation_goes_back_into_a_compressed_chunk() { + let body = "the quick brown fox ".repeat(80); + let mut ztxt = b"Comment\0\0".to_vec(); + ztxt.extend_from_slice(&zlib(body.as_bytes())); + let meta = gamut_png::metadata(&minimal_source(&[chunk(b"zTXt", &ztxt)])).unwrap(); + + let out = re_encoded_bytes(|e| e.with_metadata(&meta)); + let carried = chunk_payload(&out, b"zTXt").expect("carried as zTXt"); + assert!( + chunk_payload(&out, b"tEXt").is_none(), + "not inflated to tEXt" + ); + assert!( + carried.len() < body.len() / 4, + "still compressed: {} bytes for a {}-byte body", + carried.len(), + body.len() + ); +} + +/// The same claim for the compression flag §11.3.3.4 gives `iTXt`: a compressed international +/// annotation stays compressed, and keeps the language tag and translated keyword that a plain +/// `iTXt` rewrite would have kept but a `tEXt` rewrite would have dropped. +/// +/// Kills the `CompressedInternational` arm of `with_metadata_view`'s routing. +#[test] +fn a_compressed_itxt_goes_back_into_a_compressed_itxt() { + let body = "gämut ".repeat(200); + let mut itxt = b"Note\0\x01\0de\0Notiz\0".to_vec(); + itxt.extend_from_slice(&zlib(body.as_bytes())); + let meta = gamut_png::metadata(&minimal_source(&[chunk(b"iTXt", &itxt)])).unwrap(); + + let out = re_encoded_bytes(|e| e.with_metadata(&meta)); + let note = chunk_payload(&out, b"iTXt").expect("the Note annotation"); + // keyword, NUL, compression flag 1, method 0, language, NUL, translated keyword, NUL. + assert!(note.starts_with(b"Note\0\x01\0de\0Notiz\0"), "{note:?}"); + assert!( + note.len() < body.len() / 4, + "still compressed: {} bytes", + note.len() + ); +} + +/// Carrying the same metadata twice is carrying it once. The single-value slots are idempotent +/// because a second write overwrites the first; the text list is the one place where a second +/// call would otherwise append a duplicate of every annotation — which is what a caller that +/// builds an encoder in a loop, or reuses one across files, would get. +#[test] +fn carrying_the_same_metadata_twice_carries_it_once() { + let meta = gamut_png::metadata(&source(&[])).unwrap(); + + let once = re_encoded(|e| e.with_metadata(&meta)); + let twice = re_encoded(|e| e.with_metadata(&meta).with_metadata(&meta)); + assert_eq!(once, twice); + assert_eq!(once.texts.len(), 2, "the fixture carries two annotations"); +} + +/// §11.3.3.4 gives the `iTXt` text field UTF-8 and no alternative, so a packet that is not UTF-8 +/// has no chunk this encoder can frame. The read side hands it over as raw bytes regardless — it +/// reports what the file held — so the write side is where it has to be said out loud. Refusing +/// is the point: the alternative is a caller who asked for preservation and got a file with the +/// packet missing and nothing to read about it. +#[test] +fn a_non_utf8_xmp_packet_refuses_the_re_encode() { + let mut itxt = b"XML:com.adobe.xmp\0\0\0\0\0".to_vec(); + itxt.extend_from_slice(b""); + let meta = gamut_png::metadata(&minimal_source(&[chunk(b"iTXt", &itxt)])).unwrap(); + assert!(meta.xmp.is_some(), "the read side surfaces the raw packet"); + + let pixels = vec![0u8; 3 * 4]; + let image = ImageRef::::new(&pixels, Dimensions::new(2, 2).unwrap()).unwrap(); + let error = PngEncoder::new() + .with_metadata(&meta) + .encode_to_vec(image) + .expect_err("refused"); + assert_eq!(error.kind(), ErrorKind::InvalidInput); + assert!( + error.to_string().contains("XMP packet is not UTF-8"), + "{error}" + ); +} diff --git a/crates/gamut-png/tests/roundtrip.rs b/crates/gamut-png/tests/roundtrip.rs index 75d810e8..f49aa436 100644 --- a/crates/gamut-png/tests/roundtrip.rs +++ b/crates/gamut-png/tests/roundtrip.rs @@ -258,8 +258,7 @@ fn ancillary_pile_survives_decode() { let (w, h) = (16u32, 16u32); let src = noise((w * h * 3) as usize, 9); let exif = tiny_exif(); - // No iCCP: it is the one chunk the encoder refuses beside the sRGB this pile carries (§5.6 - // Table 5, §11.3.2.5), and its carriage is pinned by `tests/metadata.rs`. + let icc = tiny_icc_profile(); let xmp = r#""#; let mut png = Vec::new(); PngEncoder::new() @@ -274,6 +273,7 @@ fn ancillary_pile_survives_decode() { .with_compressed_text("Comment", &"squeeze ".repeat(40)) .with_international_text("Author", "gämut") .with_exif(&exif) + .with_icc_profile("prof", &icc) .with_xmp(xmp) .encode_image( ImageRef::::new(&src, Dimensions::new(w, h).unwrap()).unwrap(), @@ -289,6 +289,7 @@ fn ancillary_pile_survives_decode() { assert_eq!(decoded.srgb, Some(SrgbIntent::RelativeColorimetric)); assert!(decoded.chromaticities.is_some()); assert_eq!(decoded.exif.as_deref(), Some(exif.as_slice())); + assert_eq!(decoded.icc_profile.unwrap().profile, icc); assert_eq!(decoded.xmp.as_deref(), Some(xmp.as_bytes())); assert_eq!(decoded.texts.len(), 3); } From 47b42c36e4cf2af517e83627197e1c1e19b98944 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 01:35:37 -0400 Subject: [PATCH 04/14] feat(cli): say what metadata a conversion could not carry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gamut convert` carried a PNG input's metadata and said nothing about the payloads it could not: a C2PA manifest store, signed over the bytes of the file it was made for, and a cICP whose matrix coefficients PNG does not allow. Silent loss is the defect class this path exists to remove, so both are now warned about on stderr, which the default verbosity shows. Also corrects the claim about the second read's cost: the metadata walk is cheap — it skips IDAT by length and never inflates a pixel — but reading the file from disk again is not, and that is what taking a path rather than the already-loaded bytes costs. --- crates/gamut-cli/src/commands/convert.rs | 19 +++++++++---- crates/gamut-cli/tests/convert_metadata.rs | 33 ++++++++++++++++++---- 2 files changed, 41 insertions(+), 11 deletions(-) diff --git a/crates/gamut-cli/src/commands/convert.rs b/crates/gamut-cli/src/commands/convert.rs index de5e6412..7e9aadba 100644 --- a/crates/gamut-cli/src/commands/convert.rs +++ b/crates/gamut-cli/src/commands/convert.rs @@ -89,9 +89,10 @@ pub(crate) struct ConvertArgs { /// Drop the input's metadata instead of carrying it into the output. By default a PNG input /// re-encoded to PNG keeps its EXIF, ICC profile, XMP packet, text annotations and colour /// chunks; a stripped file is smaller, an unstripped one is colour-accurate, so the default - /// is the one that loses nothing. The C2PA manifest store is never carried either way (it is - /// signed over the bytes of the file it was made for). Currently applies only to the PNG - /// output path with a PNG input; every other pair drops metadata regardless. + /// is the one that loses nothing. Anything that cannot be carried — the C2PA manifest store, + /// signed over the bytes of the file it was made for — is reported on stderr rather than + /// dropped in silence. Currently applies only to the PNG output path with a PNG input; every + /// other pair drops metadata regardless. #[arg(long)] strip_metadata: bool, } @@ -251,8 +252,10 @@ pub(crate) fn run(args: &ConvertArgs) -> Result<(), CliError> { encoder = encoder.with_effort(effort); } // Carry the input's metadata rather than dropping it (issue #483). `png_metadata` - // reads the file a second time — cheaply: the walk skips IDAT by length and never - // inflates a pixel — and yields nothing for an input that is not a PNG. + // reads the file from disk a second time; the *walk* is cheap (it skips IDAT by + // length and never inflates a pixel), the second read is not, and it is what the + // convenience of taking a path rather than the already-loaded bytes costs. It yields + // nothing for an input that is not a PNG. let metadata = (!args.strip_metadata) .then(|| png_metadata(&args.input)) .flatten(); @@ -265,6 +268,12 @@ pub(crate) fn run(args: &ConvertArgs) -> Result<(), CliError> { "carrying input metadata" ); encoder = encoder.with_metadata(metadata); + // Say what could not come along. Silent loss is the defect this path exists to + // remove, and a payload the spec forbids carrying is still a payload the caller + // had. + for dropped in encoder.dropped_metadata() { + tracing::warn!("input metadata not carried — {dropped}"); + } } encoder.encode_image(ImageRef::::new(&rgba, dims)?, &mut out)?; (rgba.len(), dims) diff --git a/crates/gamut-cli/tests/convert_metadata.rs b/crates/gamut-cli/tests/convert_metadata.rs index 947ceb2d..1dd64a6f 100644 --- a/crates/gamut-cli/tests/convert_metadata.rs +++ b/crates/gamut-cli/tests/convert_metadata.rs @@ -13,7 +13,8 @@ use std::process::Command; use gamut::core::{Dimensions, EncodeImage, ImageRef, Rgba8}; use gamut::png::{PngEncoder, PngMetadata, SrgbIntent}; -/// A 2×2 PNG carrying an EXIF block, a text annotation and a rendering intent. +/// A 2×2 PNG carrying an EXIF block, a text annotation, a rendering intent and a C2PA manifest +/// store — the last being the one payload a re-encode may not carry. fn png_with_metadata() -> Vec { let rgba = vec![255u8; 4 * 4]; let dims = Dimensions { @@ -25,13 +26,15 @@ fn png_with_metadata() -> Vec { .with_exif(&[0x49, 0x49, 0x2A, 0x00, 0x08, 0x00, 0x00, 0x00]) .with_text("Author", "nobody") .with_srgb(SrgbIntent::Perceptual) + .with_c2pa(b"\0\0\0\x10jumbc2pa") .encode_to_vec(image) .unwrap() } /// Writes `png` to a temp file, converts it to PNG with `extra` flags, and returns the output's -/// metadata. Both temp files are removed before the assertion runs. -fn convert(name: &str, png: &[u8], extra: &[&str]) -> PngMetadata { +/// metadata together with what the command said on stderr. Both temp files are removed before +/// the assertion runs. +fn convert(name: &str, png: &[u8], extra: &[&str]) -> (PngMetadata, String) { let dir = std::env::temp_dir(); let input = dir.join(format!( "gamut-convert-{}-{name}-in.png", @@ -59,14 +62,17 @@ fn convert(name: &str, png: &[u8], extra: &[&str]) -> PngMetadata { "stderr: {}", String::from_utf8_lossy(&status.stderr) ); - gamut::png::metadata(&encoded.expect("output written")).expect("read back") + ( + gamut::png::metadata(&encoded.expect("output written")).expect("read back"), + String::from_utf8_lossy(&status.stderr).into_owned(), + ) } /// The issue's headline: `gamut convert` used to decode to raw RGBA and encode with a bare /// builder, so every EXIF, ICC, XMP and text chunk was lost with no warning. #[test] fn png_to_png_carries_the_input_metadata_by_default() { - let meta = convert("default", &png_with_metadata(), &[]); + let (meta, _) = convert("default", &png_with_metadata(), &[]); assert_eq!( meta.exif.as_deref(), @@ -85,7 +91,22 @@ fn png_to_png_carries_the_input_metadata_by_default() { /// for — the default may not silently discard colour information. #[test] fn strip_metadata_drops_it_all() { - let meta = convert("stripped", &png_with_metadata(), &["--strip-metadata"]); + let (meta, _) = convert("stripped", &png_with_metadata(), &["--strip-metadata"]); assert_eq!(meta, PngMetadata::default()); } + +/// A payload the command could not carry is *said*, not swallowed. A C2PA manifest store is +/// signed over the bytes of the file it was made for (C2PA 2.4 §A.3.2), so a copy would be +/// invalid — but the caller asked for preservation and is entitled to know their provenance did +/// not survive. Warnings reach stderr at the default verbosity, so this needs no `-v`. +#[test] +fn a_payload_that_cannot_be_carried_is_reported_on_stderr() { + let (meta, stderr) = convert("dropped", &png_with_metadata(), &[]); + + assert!(meta.c2pa.is_none(), "the store is not carried"); + assert!( + stderr.contains("C2PA manifest store"), + "stderr said nothing about the store: {stderr}" + ); +} From 0a78c0f4333a03d1b21e7ac1725e50fcc7d29264 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 01:35:37 -0400 Subject: [PATCH 05/14] docs(png): record the clauses metadata preservation implements The M1 row sat behind a blank line, so it rendered as a table of its own rather than a row of the phase table. Attach it, and rewrite the section to state the repertoire of each field as its own clause gives it, what a carry drops and names, why both colour chunks are written, and which oracle gaps stop the claim being differential today. --- crates/gamut-png/STATUS.md | 99 +++++++++++++++++++++++++------------- 1 file changed, 65 insertions(+), 34 deletions(-) diff --git a/crates/gamut-png/STATUS.md b/crates/gamut-png/STATUS.md index 7e033458..dedef130 100644 --- a/crates/gamut-png/STATUS.md +++ b/crates/gamut-png/STATUS.md @@ -40,8 +40,7 @@ opts into narrowing. That is distinct from the encoder's *lossless* auto-reduce | P10 | — | CLI `gamut convert → .png`; umbrella `png` feature; final API review | ✅ done | | E1 | #224 | **Efficiency:** `deconstruct` byte accounting; divan size/bpp + per-stage bench; libpng-9 size contract; opt-in transparent cleanup; palette-vs-native race; `crc32fast` and restructured filter kernels (see [Efficiency](#efficiency-issue-224)) | ✅ done | | C1 | C2PA 2.4 §A.3.2, §18.5.4 | **C2PA carriage** (#440): the `caBX` manifest store — raw decode surface (`c2pa`; first CRC-valid chunk before `IDAT` wins, ignored ones counted, under the metadata budget); `with_c2pa` / `with_c2pa_reserved` as the last chunk before `IDAT`; the whole-chunk exclusion span from `encode_with_report` and `PngReport::c2pa`, filled in place by `fill_c2pa` (see [C2PA](#c2pa-manifest-store-issue-440)) | ✅ done | - -| M1 | §4.3, §5.6, §11.3.2.6, §11.3.3 | **Metadata preservation** (#483): `with_metadata` / `with_metadata_from` carry a read file's eXIf/iCCP/XMP/text/colour chunks into a re-encode (`gamut convert` uses it; `--strip-metadata` opts out); `with_cicp`; `sRGB` beside `iCCP` refused and resolved by colour-chunk priority; `tEXt`/`zTXt` written as Latin-1 with promotion to `iTXt` (see [Metadata preservation](#metadata-preservation-issue-483)) | ✅ done | +| M1 | §4.3, §11.3.2.6, §11.3.3 | **Metadata preservation** (#483): `with_metadata` / `with_metadata_from` carry a read file's eXIf/iCCP/sRGB/cICP/gAMA/cHRM/XMP/text chunks into a re-encode, each annotation back into the chunk it came from (`gamut convert` uses it; `--strip-metadata` opts out; what cannot be carried is named by `dropped_metadata`); `with_cicp`; §11.3.3.1's keyword rules and §11.3.3.2/§11.3.3.4's null prohibition enforced, with promotion to `iTXt` for text outside Latin-1 (see [Metadata preservation](#metadata-preservation-issue-483)) | ✅ done | ## Decoder phases (issue #249) @@ -148,44 +147,76 @@ one private borrowed view behind two entry points, so the pixel-free `metadata() `decode()` reach it without copying a large ICC profile twice. `gamut convert` uses it on the PNG output path; `--strip-metadata` is the opt-out. **Preserve is the default**: a stripped file is smaller, but dropping an ICC profile silently changes what a viewer paints, so the loss is the -thing that has to be asked for. - -**Three spec-driven adjustments** on the way through, none of them a policy choice: - -- `iCCP` and `sRGB` are **resolved, not both written**. §5.6 Table 5 records the constraint on both - rows and §11.3.2.5 repeats it; §4.3 Table 1 then ranks the colour chunks (cICP 1, iCCP 2, sRGB 3, - cHRM+gAMA 4) and a reader honours the lowest number. So the `iCCP` is carried and the `sRGB` - dropped — the chunk a conforming reader was already ignoring. -- A `cICP` whose matrix coefficients are not 0 is dropped: §11.3.2.6 requires 0 for PNG, so such a - chunk is not conforming and carrying it forward would reproduce the defect. -- The **C2PA manifest store is never carried**. A store is signed over the exact bytes of the file - it was made for — the reason `caBX` is unsafe to copy (C2PA 2.4 §A.3.2) — so a copy is invalid by - construction. Re-sign the output and set it with `with_c2pa`. - -**Two spec defects** the same issue found, both in the writer: - -- *`sRGB` beside `iCCP` was written whenever both were set*, warned about only in a doc comment. - Now `Ancillary::validate` refuses the encode with `InvalidInput` at the one chokepoint every - encode path funnels through. Refusing rather than dropping one is the point: which the caller - meant is not guessable, and `with_metadata` exists for the case where §4.3 answers it. -- *`tEXt`/`zTXt` carried UTF-8.* §11.3.3.2 interprets a `tEXt` text string as Latin-1, §11.3.3.3 - makes an inflated `zTXt` identical to it, and §11.3.3.1 binds every keyword to Latin-1 — but the - writer pushed the Rust `String`'s bytes, storing `C3 A9` where `é` belongs. Text and keyword are - now converted once at the setter and the entry holds the bytes its chunk carries, so the wrong - encoding is unrepresentable rather than merely avoided. A text outside Latin-1 is promoted to - `iTXt` exactly as §11.3.3.2 directs, keeping the caller's compression via §11.3.3.4's flag; a - *keyword* outside it has no chunk at all, so it refuses the encode. +thing that has to be asked for. Carrying the same metadata twice carries it once — the text list +is replaced, not appended to, so the single-value colour slots and the annotations are idempotent +alike. + +**Identity, not just content.** `TextChunk::kind` records which of §11.3.3's three chunks carried +an annotation and whether its text was compressed, and a carry puts it back in the same one. +Without it a `zTXt` is indistinguishable from a `tEXt` once decoded, and a compressed 40-byte +payload comes back out as 1 600 uncompressed bytes — no words lost, but not preservation either. + +**Two payloads cannot be carried, and neither is dropped in silence.** `dropped_metadata()` names +them and `gamut convert` prints them: + +- a `cICP` whose matrix coefficients are not 0 — §11.3.2.6 requires 0 for PNG, so the source chunk + is not conforming and carrying it forward would reproduce the defect; +- the **C2PA manifest store**, signed over the exact bytes of the file it was made for, which is + why `caBX` is unsafe to copy (C2PA 2.4 §A.3.2). Re-sign the output and set it with `with_c2pa`. + +**The colour chunks are carried together, not resolved.** §5.6 Table 5 and §11.3.2.5 say only that +`sRGB` and `iCCP` "should not" appear together — lowercase, and §15 gives the BCP 14 keywords +force "when, and only when, they appear in all capitals" — while §4.3 Table 1 *presupposes* the +co-occurrence and defines the outcome by ranking the chunks (cICP 1, iCCP 2, sRGB 3, cHRM+gAMA 4). +libpng reads a file carrying both and returns the same pixels (`tests/oracle.rs`). So both are +written: dropping either would throw away colour information the source carried, and a reader +takes the one it can use. + +**The text clauses are enforced, because breaking them corrupts rather than merely offends.** +§11.3.3.1 and §11.3.3.2/§11.3.3.4 are different clauses with different repertoires, and both are +implemented as written: + +| Field | Repertoire | Clause | +| --- | --- | --- | +| Keyword (all three chunks) | code points `0x20`–`0x7E` and `0xA1`–`0xFF`; 1–79 bytes; no leading, trailing or consecutive space; expressly not U+00A0 | §11.3.3.1 | +| `tEXt`/`zTXt` text string | the keyword repertoire plus U+000A LINE FEED | §11.3.3.1 closing ¶, §11.3.3.2 | +| `iTXt` text and translated keyword | UTF-8, no null byte | §11.3.3.4 | +| `iTXt` language tag | ASCII letters, digits and `-` (BCP 47 subtags) | §11.3.3.4 | + +Text outside the `tEXt`/`zTXt` repertoire is **promoted** to `iTXt`, which is what §11.3.3.2 +directs ("Text containing characters outside the repertoire of ISO/IEC 8859-1 should be encoded +using the iTXt chunk"), keeping the caller's compression via §11.3.3.4's own flag. Because +promotion is lossless — the character survives, only the chunk changes — the tighter of §11.3.3.1's +and §11.3.3.2's two readings of "Latin-1" is taken, so a control character promotes rather than +being written with no defined meaning. + +Anything **no** chunk can carry refuses the encode with `InvalidInput`, naming the annotation's +index and keyword: a null anywhere in a keyword or text string (it is the field separator, so the +chunk re-parses as a *different* annotation), a keyword outside §11.3.3.1, an XMP packet that is +not UTF-8. A refusal is not a policy choice here — the alternative is a file that reads back as +something else, or a payload that vanishes with nothing said. + +**Two spec defects** the same issue found, both in the writer, both fixed: + +- *`tEXt`/`zTXt` carried UTF-8.* §11.3.3.2 interprets a `tEXt` text string as Latin-1 and + §11.3.3.3 makes an inflated `zTXt` identical to it, but the writer pushed the Rust `String`'s + bytes, storing `C3 A9` where `é` belongs. Text and keyword are now converted once at the setter + and the entry holds the bytes its chunk carries, so the wrong encoding is unrepresentable rather + than merely avoided. +- *`iTXt` lost its language tag and translated keyword*, the two fields that make it + international, and its compression flag. `with_cicp` (§11.3.2.6) was added with this work — without it, preservation would silently drop the highest-precedence colour chunk of any file that carries one. It takes no matrix argument: PNG fixes that byte at 0. **Not done.** `pHYs`, `tIME`, `sBIT` and `bKGD` are not part of `PngMetadata`/`DecodedPng`, so they -cannot be carried (set them with their own builder methods). A `zTXt` is indistinguishable from a -`tEXt` once decoded, so a compressed annotation is rewritten uncompressed — no text is lost, only -bytes. §11.3.3.1's keyword *syntax* rules beyond Latin-1 (the printable subset, the space rules, -the 1–79-byte bound) are not enforced. `gamut convert` carries metadata only PNG→PNG; mapping a -JPEG/WebP/JXL input's metadata into PNG chunks is a cross-format job of its own. +cannot be carried (set them with their own builder methods). The `iTXt` language tag is checked for +its character set, not for full BCP 47 well-formedness (subtag order, registry membership). +`gamut convert` carries metadata only PNG→PNG; mapping a JPEG/WebP/JXL input's metadata into PNG +chunks is a cross-format job of its own. The libpng oracle reads no chunk back and drops warnings, +so preservation is pinned against gamut's own reader plus a decode the oracle accepts — #502, #571 +and #572 are what would make it differential. ## Efficiency (issue #224) From 0314208ae256444808b03c1a0a609909bc7a4ee7 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 02:02:47 -0400 Subject: [PATCH 06/14] test(png): pin what end_carry separates and what a dropped payload is called MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two mutants the diff gate reached and no test killed. `end_carry` could be replaced with nothing: the idempotence test set its own annotation *before* the carries, where the flag's state makes no difference, so it now sets one after a carry too — the case where mistaking a direct setter for part of the carry eats it on the next one. `DroppedMetadata::reason` and its `Display` could return an empty string. The lines they produce are the whole of what a user learns about metadata that did not survive, and the test that reads them drives the `gamut` binary from `gamut-cli`, which the mutation gate cannot see. Pin the words in gamut-png's own suite. --- crates/gamut-png/src/ancillary.rs | 26 +++++++++++++++++++------- crates/gamut-png/tests/preservation.rs | 23 +++++++++++++++++++++++ 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/crates/gamut-png/src/ancillary.rs b/crates/gamut-png/src/ancillary.rs index 19e179ed..0459215d 100644 --- a/crates/gamut-png/src/ancillary.rs +++ b/crates/gamut-png/src/ancillary.rs @@ -1453,14 +1453,26 @@ mod tests { #[test] fn a_second_carry_replaces_the_first_and_spares_direct_setters() { let mut a = Ancillary::default(); - a.add_text_latin1("Mine", "kept"); - for _ in 0..2 { - a.begin_carry(); - a.add_text_latin1("Carried", "once"); - a.end_carry(); - } + a.add_text_latin1("Before", "kept"); + a.begin_carry(); + a.add_text_latin1("Carried", "once"); + a.end_carry(); + // Set *after* the carry ended: it must not be mistaken for part of it, which is what + // `end_carry` is for and what a mutant that skips it would get wrong. + a.add_text_latin1("After", "kept"); + a.begin_carry(); + a.add_text_latin1("Carried", "once"); + a.end_carry(); + let keywords: Vec<&[u8]> = a.texts.iter().map(|e| e.keyword.as_slice()).collect(); - assert_eq!(keywords, [b"Mine".as_slice(), b"Carried".as_slice()]); + assert_eq!( + keywords, + [ + b"Before".as_slice(), + b"After".as_slice(), + b"Carried".as_slice() + ] + ); } /// §11.3.2.6 Table 18 orders the payload primaries, transfer function, matrix coefficients, diff --git a/crates/gamut-png/tests/preservation.rs b/crates/gamut-png/tests/preservation.rs index 79472e2c..c39f39f2 100644 --- a/crates/gamut-png/tests/preservation.rs +++ b/crates/gamut-png/tests/preservation.rs @@ -296,3 +296,26 @@ fn a_non_utf8_xmp_packet_refuses_the_re_encode() { "{error}" ); } + +/// Naming a dropped payload is only useful if the name says something. `gamut convert` prints +/// these lines and they are the whole of what a user learns about metadata that did not survive, +/// so each has to identify the payload and give the reason it could not come along. +/// +/// Pinned here rather than in `gamut-cli`, whose tests the mutation gate cannot see: a mutant +/// that empties [`DroppedMetadata::reason`] or its `Display` would otherwise leave the command +/// printing nothing at all. +#[test] +fn a_dropped_payload_is_named_in_words() { + let store = DroppedMetadata::C2paManifestStore.to_string(); + assert!(store.contains("C2PA manifest store"), "{store}"); + assert!(store.contains("re-sign"), "{store}"); + + let cicp = DroppedMetadata::NonRgbCicp.to_string(); + assert!(cicp.contains("cICP"), "{cicp}"); + assert!(cicp.contains("matrix coefficients"), "{cicp}"); + assert_eq!( + cicp, + DroppedMetadata::NonRgbCicp.reason(), + "Display is the reason" + ); +} From cc52efddbe298c6d11b0a926989b374451373c1a Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 07:17:16 -0400 Subject: [PATCH 07/14] =?UTF-8?q?fix(png)!:=20carry=20an=20XMP=20packet's?= =?UTF-8?q?=20framing,=20and=20report=20what=20=C2=A711.3.3=20only=20advis?= =?UTF-8?q?es?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four defects in the preservation path, all of them the same mistake in two directions: the writer was stricter than its own reader about clauses the specification does not bind, and looser than the file about the one field that carries a packet's identity. **A compressed XMP packet was rewritten uncompressed.** The packet leaves the read side through its own field rather than as a `TextChunk`, so `parse_itxt` bound §11.3.3.4's compression flag, language tag and translated keyword and then discarded all three for that one keyword; the writer, having no chunk-kind to consult, always emitted flag 0 with both strings empty. Measured on the fixture this commit adds: a 354-byte `iTXt` came back out as 3 734 bytes, a factor of 10.6, with the tag and translated keyword gone. `XmpFraming` now travels beside the packet on both read surfaces, and `with_xmp` — which has no source file to take framing from — takes the one §11.3.3.1 Table 21 recommends. **Setting the packet and then carrying one wrote two chunks.** A PNG carries one XMP packet under one reserved keyword, so `add_xmp` replaces rather than appends, like every other single-value payload. Appending left this crate's own first-wins reader discarding the carried packet: a silent loss inside the feature built to end silent loss. **Five keyword shapes this crate reads perfectly were refused on re-encode.** §15 gives the BCP 14 keywords force "when, and only when, they appear in all capitals", and every statement §11.3.3.1 makes about a keyword's shape is lowercase — the same argument that lets `sRGB` and `iCCP` be carried together. A leading space, a trailing space, consecutive spaces, a C0/C1 control and U+00A0 all round-trip through this crate's reader unchanged, so refusing to write them back failed a conversion over a file whose pixels are fine, and the only escape discarded the file's ICC profile too. They are now written verbatim and reported. A keyword no chunk can hold — outside Latin-1, or outside the 1–79 bytes all three chunks fix — is dropped and reported, as are a language tag outside §11.3.3.4's ASCII shape and an XMP packet that is not UTF-8. **Only a null byte still refuses**, because it is the field separator and the chunk would re-parse as a different annotation. `DroppedMetadata` becomes `MetadataNotice` and `dropped_metadata` becomes `metadata_notices`, because the channel now reports payloads that reached the output as well as payloads that did not; `MetadataNotice::carried` separates them, and `gamut convert` words the two cases differently. **§11.3.3.1 and §11.3.3.2 contradict each other about a `tEXt` text string.** §11.3.3.1's closing paragraph restricts `tEXt`/`zTXt` content to "the printable Latin-1 character set plus U+000A LINE FEED (LF)"; §11.3.3.2, which defines `tEXt`, says one sentence later that "The text string may contain any Latin-1 character". The more specific and more permissive clause is taken, so a conforming annotation is no longer silently promoted to a different chunk type. The keyword rule stays as written, being specific to keywords. BREAKING CHANGE: `DroppedMetadata` is renamed `MetadataNotice` and gains six variants; `PngEncoder::dropped_metadata() -> &[DroppedMetadata]` becomes `metadata_notices() -> Vec`. `PngMetadata` and `DecodedPng` gain an `xmp_framing` field. An encode that carried a keyword outside §11.3.3.1's repertoire, length or spacing rules, a non-ASCII `iTXt` language tag, or an XMP packet that is not UTF-8 no longer fails; read `metadata_notices()` instead. Refs #483. Refs #600. --- crates/gamut-cli/src/commands/convert.rs | 21 +- crates/gamut-png/src/ancillary.rs | 496 +++++++++++++++-------- crates/gamut-png/src/decoded.rs | 56 ++- crates/gamut-png/src/decoder.rs | 1 + crates/gamut-png/src/encoder.rs | 190 +++++++-- crates/gamut-png/src/lib.rs | 4 +- crates/gamut-png/tests/preservation.rs | 261 +++++++++++- 7 files changed, 780 insertions(+), 249 deletions(-) diff --git a/crates/gamut-cli/src/commands/convert.rs b/crates/gamut-cli/src/commands/convert.rs index 7e9aadba..9df5c53f 100644 --- a/crates/gamut-cli/src/commands/convert.rs +++ b/crates/gamut-cli/src/commands/convert.rs @@ -90,9 +90,10 @@ pub(crate) struct ConvertArgs { /// re-encoded to PNG keeps its EXIF, ICC profile, XMP packet, text annotations and colour /// chunks; a stripped file is smaller, an unstripped one is colour-accurate, so the default /// is the one that loses nothing. Anything that cannot be carried — the C2PA manifest store, - /// signed over the bytes of the file it was made for — is reported on stderr rather than - /// dropped in silence. Currently applies only to the PNG output path with a PNG input; every - /// other pair drops metadata regardless. + /// signed over the bytes of the file it was made for — and anything carried in a shape the + /// PNG specification does not endorse is reported on stderr rather than passed over in + /// silence. Currently applies only to the PNG output path with a PNG input; every other pair + /// drops metadata regardless. #[arg(long)] strip_metadata: bool, } @@ -268,11 +269,15 @@ pub(crate) fn run(args: &ConvertArgs) -> Result<(), CliError> { "carrying input metadata" ); encoder = encoder.with_metadata(metadata); - // Say what could not come along. Silent loss is the defect this path exists to - // remove, and a payload the spec forbids carrying is still a payload the caller - // had. - for dropped in encoder.dropped_metadata() { - tracing::warn!("input metadata not carried — {dropped}"); + // Say what could not come along, and what came along with a caveat. Silent loss + // is the defect this path exists to remove, and a payload the spec forbids + // carrying is still a payload the caller had. + for notice in encoder.metadata_notices() { + if notice.carried() { + tracing::warn!("input metadata carried with a caveat — {notice}"); + } else { + tracing::warn!("input metadata not carried — {notice}"); + } } } encoder.encode_image(ImageRef::::new(&rgba, dims)?, &mut out)?; diff --git a/crates/gamut-png/src/ancillary.rs b/crates/gamut-png/src/ancillary.rs index 0459215d..da7883ef 100644 --- a/crates/gamut-png/src/ancillary.rs +++ b/crates/gamut-png/src/ancillary.rs @@ -38,6 +38,7 @@ use gamut_core::{Error, Result}; use gamut_deflate::{DeflateEncoder, Level}; use crate::decoded::XMP_KEYWORD; +use crate::encoder::MetadataNotice; use crate::{ColorType, chunk}; /// The rendering intent for an `sRGB` chunk (PNG spec §11.3.2.5). @@ -135,8 +136,8 @@ impl TextKind { /// right for its `kind`, or it carries the [`fault`](Self::fault) that stops it being written. #[derive(Debug, Clone)] struct TextEntry { - /// The keyword, Latin-1 (§11.3.3.1). Empty when [`fault`](Self::fault) is set, because such - /// an entry is never written — [`Ancillary::validate`] refuses the encode first. + /// The keyword, Latin-1 (§11.3.3.1). Empty when the keyword had no Latin-1 encoding at all, + /// which is also when [`emit`](Self::emit) is clear. keyword: Vec, /// The text: Latin-1 for `tEXt`/`zTXt`, UTF-8 for `iTXt`. text: Vec, @@ -149,9 +150,21 @@ struct TextEntry { /// Whether this entry came from [`Ancillary::begin_carry`] rather than a direct setter, so a /// second carry can replace exactly what the first contributed. carried: bool, - /// Why this annotation must not be written, if it must not. Recorded here rather than - /// returned from the setter because the setters sit behind `#[must_use]` builder methods - /// that have no error channel; [`Ancillary::validate`] reports it at the encode chokepoint. + /// Whether this entry is the XMP packet (§11.3.3.1 Table 21's reserved keyword). A file + /// carries one packet, so setting it again replaces this entry rather than adding a second. + xmp: bool, + /// Whether the entry is written at all. A cleared flag keeps the entry in the list purely to + /// carry its [`notices`](Self::notices) — a payload dropped in silence is the defect this + /// module exists to remove. + emit: bool, + /// What §11.3.3 says about this annotation that the caller has to hear: a keyword no chunk + /// can hold, or one written verbatim that deviates from a recommendation. Surfaced by + /// [`PngEncoder::metadata_notices`](crate::PngEncoder::metadata_notices). + notices: Vec, + /// Why this annotation must not be written *at all*, if it must not — the null byte, and + /// only the null byte. Recorded here rather than returned from the setter because the + /// setters sit behind `#[must_use]` builder methods that have no error channel; + /// [`Ancillary::validate`] reports it at the encode chokepoint. fault: Option, } @@ -165,36 +178,18 @@ struct TextFault { reason: &'static str, } -/// §11.3.3.1: "Keywords are restricted to 1 to 79 bytes in length." -const KEYWORD_LENGTH: &str = "a keyword is restricted to 1 to 79 bytes (§11.3.3.1)"; -/// §11.3.3.1: "Keywords shall contain only printable Latin-1 [ISO_8859-1] characters and spaces; -/// that is, only code points 0x20-7E and 0xA1-FF are allowed", and expressly "nor is U+00A0 -/// NON-BREAKING SPACE". A null is outside it too, which is also §11.3.3.2's "Neither the keyword -/// nor the text string may contain a null character". -const KEYWORD_REPERTOIRE: &str = "a keyword may hold only code points 0x20-0x7E and 0xA1-0xFF \ - — no null, no control character, not U+00A0 (§11.3.3.1)"; -/// §11.3.3.1: "leading spaces, trailing spaces, and consecutive spaces are not permitted in -/// keywords". -const KEYWORD_SPACES: &str = - "a keyword may not have a leading, trailing or consecutive space (§11.3.3.1)"; /// §11.3.3.2 for `tEXt`/`zTXt` ("Neither the keyword nor the text string may contain a null /// character") and §11.3.3.4 for `iTXt` ("neither shall contain a zero byte"). The null is the /// field separator, so an embedded one does not merely offend the grammar — the chunk re-parses -/// as a *different* annotation. -const TEXT_NUL: &str = "a text string may not contain a null character (§11.3.3.2, §11.3.3.4)"; -/// §11.3.3.4: "The language tag is a well-formed language tag defined by [BCP47]", whose subtags -/// are ASCII letters and digits joined by hyphens. Anything else is neither well-formed nor -/// (being written as UTF-8 and read back as Latin-1) byte-exact. -const LANGUAGE_TAG: &str = - "an iTXt language tag may hold only ASCII letters, digits and '-' (§11.3.3.4, BCP 47)"; +/// as a *different* annotation. It is the one thing here that makes a file **mean** something +/// else, and so the one thing that refuses the encode. +const TEXT_NUL: &str = + "a keyword or text string may not contain a null character (§11.3.3.2, §11.3.3.4)"; /// §11.3.3.4: "The translated keyword and text both use the UTF-8 encoding, and neither shall -/// contain a zero byte (null character)." +/// contain a zero byte (null character)." Null-terminated like the language tag, so an embedded +/// one re-frames every field after it. const TRANSLATED_NUL: &str = "an iTXt translated keyword may not contain a null character (§11.3.3.4)"; -/// §11.3.3.4 gives the `iTXt` text field UTF-8 and no other encoding, so a packet that is not -/// UTF-8 has no chunk to go in. Dropping it silently is the loss this crate refuses to make. -const XMP_NOT_UTF8: &str = - "the XMP packet is not UTF-8, and an iTXt text string must be (§11.3.3.4)"; /// Whether `c` is a printable Latin-1 character or a space, the repertoire §11.3.3.1 spells out /// as "only code points 0x20-7E and 0xA1-FF". @@ -202,60 +197,91 @@ fn printable_latin1(c: char) -> bool { matches!(u32::from(c), 0x20..=0x7E | 0xA1..=0xFF) } -/// Whether `c` may appear in a `tEXt`/`zTXt` **text string**: §11.3.3.1's closing paragraph -/// restricts their content to "the printable Latin-1 character set plus U+000A LINE FEED (LF)". -/// -/// §11.3.3.2 says more loosely that the text "may contain any Latin-1 character", which would -/// admit the C0/C1 controls and U+00A0. The tighter reading costs nothing to take: a character -/// outside this set is not rejected, it is *promoted* to `iTXt` — exactly what §11.3.3.2's own -/// "Text containing characters outside the repertoire of ISO/IEC 8859-1 should be encoded using -/// the iTXt chunk" directs — so the character always survives and only the chunk changes. -fn text_repertoire(c: char) -> bool { - c == '\n' || printable_latin1(c) -} - /// The Latin-1 byte of `c`: Latin-1 is the first 256 Unicode code points, so the encoding is /// `u8::try_from` — the exact inverse of the decoder's `latin1`, which maps byte *n* to U+00*nn*. fn latin1_byte(c: char) -> Option { u8::try_from(u32::from(c)).ok() } -/// The Latin-1 bytes of a keyword, or the §11.3.3.1 clause it breaks. +/// What §11.3.3.1 has to say about one keyword, resolved into what the writer does with it. +/// +/// Three outcomes, because the clause mixes three kinds of statement and §15 gives them different +/// force ("when, and only when, they appear in all capitals"). Everything §11.3.3.1 says about a +/// keyword's *shape* is lowercase — "Keywords shall contain only printable Latin-1", "leading +/// spaces, trailing spaces, and consecutive spaces are not permitted", "Keywords are restricted +/// to 1 to 79 bytes" — so none of it is binding, and this crate's own reader accepts every shape +/// of keyword the length allows. What separates the outcomes is therefore not the wording but +/// the consequence: +/// +/// - a **null** is the field separator, so the chunk re-parses as a different annotation. Refuse; +/// - a keyword **no chunk can hold** — outside Latin-1, or outside the 1–79 bytes all three +/// chunks fix — is one this crate's reader and libpng both *drop*, so writing it loses the +/// annotation with nothing said. Drop it here instead, and say so; +/// - anything else round-trips through this crate's reader byte for byte, so the keyword is +/// written exactly as it arrived and the deviation is reported. Refusing it would fail a +/// conversion over a file whose pixels are fine, and the only escape would be discarding all +/// of its metadata. +enum Keyword { + /// Write these Latin-1 bytes, reporting the recommendation the keyword does not meet. + Write(Vec, Option), + /// Do not write the annotation; report why. + Drop(MetadataNotice), + /// Refuse the encode: the keyword holds the field separator. + Refuse, +} + +/// Resolves `keyword` against §11.3.3.1. /// -/// The repertoire is checked before the length so that the length bound counts *stored* bytes: -/// every character that passes is one Latin-1 byte, which a UTF-8 `str::len` is not. -fn keyword_bytes(keyword: &str) -> core::result::Result, &'static str> { - let bytes: Option> = keyword +/// Latin-1 representability is settled before the length so that the bound counts *stored* +/// bytes: every character that passes is one Latin-1 byte, which a UTF-8 `str::len` is not. +fn keyword_verdict(keyword: &str) -> Keyword { + if keyword.contains('\0') { + return Keyword::Refuse; + } + let Some(bytes) = keyword .chars() - .map(|c| latin1_byte(c).filter(|_| printable_latin1(c))) - .collect(); - let bytes = bytes.ok_or(KEYWORD_REPERTOIRE)?; + .map(latin1_byte) + .collect::>>() + else { + return Keyword::Drop(MetadataNotice::TextKeywordNotLatin1); + }; if bytes.is_empty() || bytes.len() > 79 { - return Err(KEYWORD_LENGTH); + return Keyword::Drop(MetadataNotice::TextKeywordLength); } - if keyword.starts_with(' ') || keyword.ends_with(' ') || keyword.contains(" ") { - return Err(KEYWORD_SPACES); + if !keyword.chars().all(printable_latin1) { + return Keyword::Write(bytes, Some(MetadataNotice::TextKeywordRepertoire)); } - Ok(bytes) + let spacing = keyword.starts_with(' ') || keyword.ends_with(' ') || keyword.contains(" "); + Keyword::Write(bytes, spacing.then_some(MetadataNotice::TextKeywordSpacing)) } -/// The Latin-1 bytes of a `tEXt`/`zTXt` text string, or `None` when a character is outside -/// [`text_repertoire`] — the signal to promote the annotation to `iTXt`. +/// The Latin-1 bytes of a `tEXt`/`zTXt` text string, or `None` when a character has no Latin-1 +/// encoding at all — the signal to promote the annotation to `iTXt`. +/// +/// **The specification contradicts itself here, and the more specific clause wins.** +/// §11.3.3.1's closing paragraph says of `tEXt`/`zTXt` that "There are also tEXt and zTXt chunks, +/// whose content is restricted to the printable Latin-1 character set plus U+000A LINE FEED +/// (LF)". §11.3.3.2, the clause that defines `tEXt` itself, says the opposite one sentence after +/// naming the same character set: "Text is interpreted according to the Latin-1 character set +/// [ISO_8859-1]. The text string may contain any Latin-1 character." — adding only that +/// "Characters other than those defined in Latin-1 plus the linefeed character have no defined +/// meaning in tEXt chunks", which is a statement about characters *outside* Latin-1, not inside +/// it. §11.3.3.2 is the more specific and the more permissive of the two, so it is the one taken: +/// every Latin-1 character is written into the chunk that already interprets it as Latin-1, and +/// only a character Latin-1 cannot encode promotes to `iTXt` — which is what §11.3.3.2 itself +/// directs ("Text containing characters outside the repertoire of ISO/IEC 8859-1 should be +/// encoded using the iTXt chunk"). fn text_bytes(text: &str) -> Option> { - text.chars() - .map(|c| latin1_byte(c).filter(|_| text_repertoire(c))) - .collect() + text.chars().map(latin1_byte).collect() } -/// The §11.3.3.4 clause an `iTXt`'s language tag or translated keyword breaks, if any. -fn itxt_field_fault(language: &str, translated: &str) -> Option<&'static str> { - if !language +/// Whether `language` has the shape §11.3.3.4 requires: "The language tag is a well-formed +/// language tag defined by [BCP47]", whose subtags are ASCII letters and digits joined by +/// hyphens. This checks the character set, not full BCP 47 well-formedness. +fn well_formed_language(language: &str) -> bool { + language .bytes() .all(|b| b.is_ascii_alphanumeric() || b == b'-') - { - return Some(LANGUAGE_TAG); - } - translated.contains('\0').then_some(TRANSLATED_NUL) } /// Accumulated ancillary metadata to emit alongside the image. @@ -332,42 +358,87 @@ impl Ancillary { text: &str, compressed: bool, ) { + let entry = self.itxt_entry(keyword, language, translated, text, compressed); + self.texts.push(entry); + } + + /// Builds one `iTXt` entry with its §11.3.3.4 fields, shared by the tagged text setter and + /// the XMP packet. + /// + /// A language tag outside §11.3.3.4's ASCII shape is **dropped, not refused**: written as + /// UTF-8 into a field a reader takes as Latin-1 it would not survive the trip, but the + /// annotation itself would, and an unspecified language is what §11.3.3.4 already means by + /// an empty tag. A null in the translated keyword is a different thing — it re-frames every + /// field after it — so it refuses, like every other null. + fn itxt_entry( + &self, + keyword: &str, + language: &str, + translated: &str, + text: &str, + compressed: bool, + ) -> TextEntry { let kind = if compressed { TextKind::InternationalCompressed } else { TextKind::International }; let mut entry = self.text_entry(keyword, text, kind); - if entry.fault.is_none() { - entry.fault = itxt_field_fault(language, translated).map(|reason| TextFault { + if entry.fault.is_none() && translated.contains('\0') { + entry.fault = Some(TextFault { keyword: keyword.to_string(), - reason, + reason: TRANSLATED_NUL, }); } - entry.language = language.as_bytes().to_vec(); + if well_formed_language(language) { + entry.language = language.as_bytes().to_vec(); + } else { + entry.notices.push(MetadataNotice::ItxtLanguageTag); + } entry.translated = translated.as_bytes().to_vec(); - self.texts.push(entry); + entry } - /// Adds an XMP packet as the `iTXt` §11.3.3.1 Table 21 reserves for it. + /// Adds an XMP packet as the `iTXt` §11.3.3.1 Table 21 reserves for it, framed the way the + /// file that carried it framed it (§11.3.3.4's compression flag, language tag and translated + /// keyword). + /// + /// Replaces any packet already accumulated rather than adding a second: a PNG carries one + /// XMP packet, so this is a single-value payload like `iCCP` or `eXIf`, and two `iTXt` chunks + /// under the same reserved keyword would leave a reader to pick — this crate's own reader + /// keeps the first and discards the rest. /// /// Takes bytes rather than a `&str` because that is what the read side surfaces: a file's /// packet is whatever bytes its chunk held. §11.3.3.4 gives the `iTXt` text field UTF-8 and - /// no alternative, so bytes that are not UTF-8 have no chunk to go in — and are recorded as - /// a refusal rather than discarded, because a caller that handed this encoder a packet is - /// entitled to learn it did not come out the other side. - pub(crate) fn add_xmp(&mut self, packet: &[u8]) { - match str::from_utf8(packet) { - Ok(text) => self.add_text_international(XMP_KEYWORD, text), + /// no alternative, so bytes that are not UTF-8 have no chunk to go in — and are reported + /// rather than discarded, because a caller that handed this encoder a packet is entitled to + /// learn it did not come out the other side. + pub(crate) fn add_xmp( + &mut self, + packet: &[u8], + language: &str, + translated: &str, + compressed: bool, + ) { + self.texts.retain(|entry| !entry.xmp); + let mut entry = match str::from_utf8(packet) { + Ok(text) => self.itxt_entry(XMP_KEYWORD, language, translated, text, compressed), Err(_) => { let mut entry = self.text_entry(XMP_KEYWORD, "", TextKind::International); - entry.fault = Some(TextFault { - keyword: XMP_KEYWORD.to_string(), - reason: XMP_NOT_UTF8, - }); - self.texts.push(entry); + entry.emit = false; + entry.notices.push(MetadataNotice::XmpNotUtf8); + entry } - } + }; + entry.xmp = true; + self.texts.push(entry); + } + + /// Every §11.3.3 deviation the accumulated annotations carry, in insertion order. + pub(crate) fn text_notices(&self) -> impl Iterator + '_ { + self.texts + .iter() + .flat_map(|entry| entry.notices.iter().copied()) } /// Starts carrying a read file's metadata, discarding whatever a previous carry contributed. @@ -401,16 +472,18 @@ impl Ancillary { /// promoted rather than written as bytes a Latin-1 reader mis-renders. The promotion keeps /// the caller's *other* choice, compression, because §11.3.3.4 gives `iTXt` a flag of its own. /// - /// A null in the text is the one thing promotion cannot fix — §11.3.3.2 and §11.3.3.4 both - /// forbid it, and it is the field separator, so the chunk would re-parse as a different - /// annotation — and neither can a keyword outside §11.3.3.1's repertoire, length or spacing - /// rules. Those become a [`TextFault`] the entry carries to [`Self::validate`]. + /// A null anywhere in the keyword or the text is the one thing neither promotion nor a + /// notice can fix — §11.3.3.2 and §11.3.3.4 both forbid it, and it is the field separator, so + /// the chunk would re-parse as a different annotation. It becomes a [`TextFault`] the entry + /// carries to [`Self::validate`]. Every *other* way a keyword can fall short of §11.3.3.1 is + /// a [`MetadataNotice`] instead: see [`Keyword`] for why the line is drawn there. fn text_entry(&self, keyword: &str, text: &str, kind: TextKind) -> TextEntry { - let (keyword_bytes, keyword_fault) = match keyword_bytes(keyword) { - Ok(bytes) => (bytes, None), - Err(reason) => (Vec::new(), Some(reason)), + let (keyword_bytes, emit, notice, keyword_nul) = match keyword_verdict(keyword) { + Keyword::Write(bytes, notice) => (bytes, true, notice, false), + Keyword::Drop(notice) => (Vec::new(), false, Some(notice), false), + Keyword::Refuse => (Vec::new(), true, None, true), }; - let reason = keyword_fault.or_else(|| text.contains('\0').then_some(TEXT_NUL)); + let refused = keyword_nul || text.contains('\0'); // An iTXt was asked for as UTF-8 and stays UTF-8; only a Latin-1 request has a // repertoire to leave. let latin1 = match kind { @@ -428,21 +501,27 @@ impl Ancillary { translated: Vec::new(), kind, carried: self.carrying, - fault: reason.map(|reason| TextFault { + xmp: false, + emit, + notices: notice.into_iter().collect(), + fault: refused.then(|| TextFault { keyword: keyword.to_string(), - reason, + reason: TEXT_NUL, }), } } /// Refuses an accumulation the spec forbids, before any byte is emitted. /// - /// Only the text chunks are refusable here, and only where a clause is a requirement rather - /// than a recommendation: a keyword outside §11.3.3.1's repertoire, length or spacing rules; - /// a null in a text string (§11.3.3.2, §11.3.3.4); a language tag or translated keyword - /// §11.3.3.4 rules out; a non-UTF-8 XMP packet. Each is a chunk that would be *read back as - /// something else* — the null re-frames the annotation outright — so writing it is a silent - /// corruption, and dropping it is a silent loss. + /// **Only a null byte gets here.** A null in a keyword, a text string or an `iTXt` + /// translated keyword (§11.3.3.2, §11.3.3.4) is the field separator, so a chunk carrying one + /// re-parses as a *different* annotation: the file would mean something other than what the + /// caller supplied, and no notice can undo that. Everything else §11.3.3 asks of a text + /// chunk — the keyword's repertoire, length and spacing, the `iTXt` language tag's shape, an + /// XMP packet that is not UTF-8 — is reported through + /// [`PngEncoder::metadata_notices`](crate::PngEncoder::metadata_notices) and the encode + /// proceeds. Refusing those would fail a conversion over a file whose pixels are fine, and + /// leave the caller no way out but to discard all of its metadata, colour profile included. /// /// The colour chunks are deliberately **not** policed. §5.6 Table 5 and §11.3.2.5 say only /// that `sRGB` and `iCCP` "should not" appear together, and §15 gives the BCP 14 keywords @@ -541,7 +620,8 @@ impl Ancillary { if let Some(time) = self.time { chunk::write_chunk(out, *b"tIME", &time); } - for entry in &self.texts { + // An entry with `emit` clear is a placeholder holding its notice, not a chunk. + for entry in self.texts.iter().filter(|entry| entry.emit) { write_text(out, entry, effort); } // Last, so nothing whose size could shift the store follows it: a reservation filled by @@ -1150,6 +1230,11 @@ mod tests { a.validate().expect_err("the encode is refused").to_string() } + /// The notices `a` has accumulated, in order. + fn notices(a: &Ancillary) -> Vec { + a.text_notices().collect() + } + /// A `tEXt` text string "is interpreted according to the Latin-1 character set" (§11.3.3.2), /// so a character above U+007F is **one** byte, not its UTF-8 pair. /// @@ -1187,40 +1272,25 @@ mod tests { ); } - /// §11.3.3.1 restricts a `tEXt`/`zTXt` text string to "the printable Latin-1 character set - /// plus U+000A LINE FEED (LF)", and a control character is outside it — so it promotes, for - /// the same reason a Han character does. The character survives either way; only the chunk - /// that can define it changes. + /// The specification contradicts itself about a `tEXt` text string, and the more specific and + /// more permissive clause is the one taken. §11.3.3.1's closing paragraph says `tEXt`/`zTXt` + /// "content is restricted to the printable Latin-1 character set plus U+000A LINE FEED (LF)"; + /// §11.3.3.2, which *defines* `tEXt`, says "The text string may contain any Latin-1 + /// character". A control character, a line feed and the top of Latin-1 are therefore all + /// written into the chunk that already interprets its bytes as Latin-1. /// - /// Kills [`text_repertoire`] mutated to accept everything Latin-1 can hold, which the looser - /// wording of §11.3.3.2 ("may contain any Latin-1 character") would otherwise excuse. 0x7F - /// DELETE is Latin-1-encodable and still not printable. + /// Kills [`text_bytes`] mutated to filter its characters against a narrower repertoire, which + /// would promote a conforming annotation to a different chunk type — changing the file's + /// shape over a clause the spec itself contradicts. #[test] - fn a_control_character_promotes_the_annotation_to_itxt() { + fn every_latin1_character_stays_in_a_text_chunk() { let mut a = Ancillary::default(); - a.add_text_latin1("Title", "one\u{7F}two"); - let out = post_plte(&a); - assert_eq!(find_chunk(&out, b"tEXt"), None); - assert_eq!( - find_chunk(&out, b"iTXt"), - Some(b"Title\0\0\0\0\0one\x7Ftwo".to_vec()) - ); - } - - /// The other side of the same boundary: a line feed and the top of Latin-1 are *inside* the - /// repertoire §11.3.3.1 grants `tEXt`, so neither promotes. - /// - /// Kills [`text_repertoire`] mutated to drop its `'\n'` case or to stop at 0xFE, either of - /// which would push an ordinary multi-line Latin-1 note into an `iTXt`. - #[test] - fn a_line_feed_and_the_top_of_latin1_stay_in_a_text_chunk() { - let mut a = Ancillary::default(); - a.add_text_latin1("Description", "line\nÿ"); + a.add_text_latin1("Description", "one\u{7F}two\nÿ\u{A0}"); let out = post_plte(&a); assert_eq!(find_chunk(&out, b"iTXt"), None); assert_eq!( find_chunk(&out, b"tEXt"), - Some(b"Description\0line\n\xFF".to_vec()) + Some(b"Description\0one\x7Ftwo\n\xFF\xA0".to_vec()) ); } @@ -1269,76 +1339,124 @@ mod tests { } /// §11.3.3.1: "Keywords are restricted to 1 to 79 bytes in length." Both edges, because an - /// empty keyword makes a third-party reader drop the whole annotation and an over-long one is - /// a chunk no conforming reader has to accept. + /// empty keyword makes a reader drop the whole annotation and an over-long one is a chunk no + /// conforming reader has to accept — including this crate's own, which splits a payload at + /// its first null and refuses a keyword field outside 1–79 bytes. Writing such a chunk would + /// therefore lose the annotation without a word, so it is dropped here and reported. /// - /// Kills the length guard in [`keyword_bytes`], including a mutant that shifts either bound - /// by one. + /// Kills the length guard in [`keyword_verdict`], including a mutant that shifts either bound + /// by one, and the `Drop` arm of [`Ancillary::text_entry`] that keeps the chunk out. #[test] - fn a_keyword_outside_one_to_seventy_nine_bytes_is_refused() { + fn a_keyword_outside_one_to_seventy_nine_bytes_is_dropped_with_a_notice() { let mut ok = Ancillary::default(); ok.add_text_latin1(&"k".repeat(79), "body"); ok.add_text_latin1("k", "body"); - assert!(ok.validate().is_ok(), "79 bytes and 1 byte are inside"); + assert!(notices(&ok).is_empty(), "79 bytes and 1 byte are inside"); + assert!(find_chunk(&post_plte(&ok), b"tEXt").is_some()); for keyword in ["", &"k".repeat(80)] { let mut a = Ancillary::default(); a.add_text_latin1(keyword, "body"); - assert!( - refusal(&a).contains("restricted to 1 to 79 bytes"), + assert_eq!( + notices(&a), + [MetadataNotice::TextKeywordLength], "keyword of {} bytes", keyword.len() ); + assert!(a.validate().is_ok(), "reported, not refused"); + assert_eq!(find_chunk(&post_plte(&a), b"tEXt"), None); } } + /// §11.3.3.1 binds a keyword to Latin-1 in all three text chunks, so a character Latin-1 + /// cannot encode has no chunk to go in — unlike a *text string*, which §11.3.3.2 routes to + /// `iTXt`. The annotation is dropped and reported rather than transliterated. + /// + /// Kills the `Drop(TextKeywordNotLatin1)` arm of [`keyword_verdict`]: with it gone the + /// keyword's UTF-8 bytes reach a field a reader takes as Latin-1. + #[test] + fn a_keyword_outside_latin1_is_dropped_with_a_notice() { + let mut a = Ancillary::default(); + a.add_text_latin1("题", "body"); + assert_eq!(notices(&a), [MetadataNotice::TextKeywordNotLatin1]); + assert!(a.validate().is_ok(), "reported, not refused"); + assert_eq!(find_chunk(&post_plte(&a), b"tEXt"), None); + } + /// §11.3.3.1: "only code points 0x20-7E and 0xA1-FF are allowed", and expressly "nor is /// U+00A0 NON-BREAKING SPACE since it is visually indistinguishable from an ordinary space". - /// The null is the same clause read through §11.3.3.2 — and the one that *corrupts* rather - /// than merely offends, because it is the field separator: `Auth\0or` re-parses as the - /// annotation `Auth`. + /// Lowercase "shall", so §15 makes it advisory, and this crate's reader returns such a + /// keyword unchanged — so the keyword is **written verbatim** and the deviation reported. /// - /// Kills the repertoire guard in [`keyword_bytes`] and each edge of [`printable_latin1`]. + /// Kills each edge of [`printable_latin1`] and the `Write(_, Some(..))` arm of + /// [`keyword_verdict`]: a mutant that stops noticing leaves the caller unwarned, and one that + /// drops the annotation loses metadata the file had. #[test] - fn a_keyword_outside_the_printable_latin1_repertoire_is_refused() { + fn a_keyword_outside_the_printable_latin1_repertoire_is_written_with_a_notice() { for keyword in [ - "Auth\0or", // the field separator itself - "Auth\u{7F}", // DELETE - "Auth\u{9F}", // C1 control - "Auth\u{A0}", // NON-BREAKING SPACE, named by the clause - "题", // outside Latin-1 altogether + "Auth\u{7F}or", // DELETE + "Auth\u{9F}or", // C1 control + "Auth\u{A0}or", // NON-BREAKING SPACE, named by the clause ] { let mut a = Ancillary::default(); a.add_text_latin1(keyword, "body"); - assert!( - refusal(&a).contains("code points 0x20-0x7E and 0xA1-0xFF"), + assert_eq!( + notices(&a), + [MetadataNotice::TextKeywordRepertoire], "keyword {keyword:?}" ); + let mut expected = keyword.chars().map(|c| c as u8).collect::>(); + expected.extend_from_slice(b"\0body"); + assert_eq!(find_chunk(&post_plte(&a), b"tEXt"), Some(expected)); } let mut edges = Ancillary::default(); edges.add_text_latin1("a\u{20}b\u{7E}\u{A1}\u{FF}", "body"); - assert!(edges.validate().is_ok(), "0x20, 0x7E, 0xA1 and 0xFF are in"); + assert!( + notices(&edges).is_empty(), + "0x20, 0x7E, 0xA1 and 0xFF are in" + ); } /// §11.3.3.1: "leading spaces, trailing spaces, and consecutive spaces are not permitted in - /// keywords", so that a keyword cannot be misread as another. + /// keywords", so that a keyword cannot be misread as another. Lowercase again, and again a + /// keyword this crate's reader hands back unchanged, so it is written and reported. /// - /// Kills the spacing guard in [`keyword_bytes`], one condition at a time. + /// Kills the spacing guard in [`keyword_verdict`], one condition at a time. #[test] - fn a_keyword_with_a_leading_trailing_or_consecutive_space_is_refused() { + fn a_keyword_with_a_leading_trailing_or_consecutive_space_is_written_with_a_notice() { for keyword in [" Author", "Author ", "Two Words"] { let mut a = Ancillary::default(); a.add_text_latin1(keyword, "body"); - assert!( - refusal(&a).contains("leading, trailing or consecutive space"), + assert_eq!( + notices(&a), + [MetadataNotice::TextKeywordSpacing], "keyword {keyword:?}" ); + let mut expected = keyword.as_bytes().to_vec(); + expected.extend_from_slice(b"\0body"); + assert_eq!(find_chunk(&post_plte(&a), b"tEXt"), Some(expected)); } let mut ok = Ancillary::default(); ok.add_text_latin1("Two Words", "body"); - assert!(ok.validate().is_ok(), "a single interior space is allowed"); + assert!( + notices(&ok).is_empty(), + "a single interior space is allowed" + ); + } + + /// A null in the *keyword* is the field separator, so `Auth\0or` re-parses as the annotation + /// `Auth` with `or` for its text: the chunk means something the caller never wrote. That — + /// and only that — still refuses, which is the line between what this module reports and what + /// it rejects. + /// + /// Kills the `Refuse` arm of [`keyword_verdict`], which no notice test can reach. + #[test] + fn a_null_in_a_keyword_is_refused() { + let mut a = Ancillary::default(); + a.add_text_latin1("Auth\0or", "body"); + assert!(refusal(&a).contains("may not contain a null character")); } /// §11.3.3.2: "Neither the keyword nor the text string may contain a null character", and @@ -1372,18 +1490,28 @@ mod tests { } /// §11.3.3.4: "The language tag is a well-formed language tag defined by [BCP47]", whose - /// subtags are ASCII letters and digits joined by hyphens. Anything else is not a tag, and — - /// written as UTF-8 into a field a reader takes as Latin-1 — would not even survive the trip. + /// subtags are ASCII letters and digits joined by hyphens. Anything else, written as UTF-8 + /// into a field a reader takes as Latin-1, would not survive the trip — so the **tag** goes + /// and the annotation stays, an empty tag being §11.3.3.4's own way of saying the language is + /// unspecified. /// - /// Kills the language arm of [`itxt_field_fault`], and the empty case pins that "unspecified" - /// stays legal. + /// Kills the language arm of [`Ancillary::itxt_entry`]; the empty case pins that + /// "unspecified" is not itself a deviation. #[test] - fn a_language_tag_outside_bcp_47_is_refused() { + fn a_language_tag_outside_bcp_47_is_dropped_with_a_notice() { for language in ["de\0DE", "zh_Hans", "dé"] { let mut a = Ancillary::default(); a.add_text_international_tagged("Note", language, "", "body", false); - assert!( - refusal(&a).contains("ASCII letters, digits and '-'"), + assert_eq!( + notices(&a), + [MetadataNotice::ItxtLanguageTag], + "language {language:?}" + ); + assert!(a.validate().is_ok(), "reported, not refused"); + // keyword, NUL, flag, method, *empty* language, NUL, empty translated keyword, NUL. + assert_eq!( + find_chunk(&post_plte(&a), b"iTXt"), + Some(b"Note\0\0\0\0\0body".to_vec()), "language {language:?}" ); } @@ -1392,29 +1520,57 @@ mod tests { ok.add_text_international_tagged("Note", "", "", "body", false); ok.add_text_international_tagged("Note", "ar-AE-u-nu-latn", "", "body", false); assert!( - ok.validate().is_ok(), + notices(&ok).is_empty(), "empty and a full BCP 47 tag are fine" ); } /// §11.3.3.4 gives the `iTXt` text field UTF-8 and no alternative, so a packet that is not - /// UTF-8 has no chunk to go in. It is refused rather than quietly discarded: the read side + /// UTF-8 has no chunk to go in. It is reported rather than quietly discarded: the read side /// surfaces a packet as raw bytes, and a caller that handed those bytes back is entitled to - /// learn they did not come out the other side. + /// learn they did not come out the other side. It does not refuse, because the rest of the + /// file — pixels, colour profile — is fine. /// - /// Kills the `Err` arm of [`Ancillary::add_xmp`] — with it gone the packet vanishes silently. + /// Kills the `Err` arm of [`Ancillary::add_xmp`] — with it gone the packet vanishes silently + /// — and its `emit` flag, without which the packet's *keyword* is written with no packet. #[test] - fn a_non_utf8_xmp_packet_is_refused() { + fn a_non_utf8_xmp_packet_is_reported_not_written() { let mut a = Ancillary::default(); - a.add_xmp(b""); - assert!(refusal(&a).contains("XMP packet is not UTF-8")); + a.add_xmp(b"", "", "", false); + assert_eq!(notices(&a), [MetadataNotice::XmpNotUtf8]); + assert!(a.validate().is_ok(), "reported, not refused"); + assert_eq!(find_chunk(&post_plte(&a), b"iTXt"), None); let mut valid = Ancillary::default(); - valid.add_xmp(b""); - assert!(valid.validate().is_ok(), "a UTF-8 packet is carried"); + valid.add_xmp(b"", "", "", false); + assert!(notices(&valid).is_empty(), "a UTF-8 packet is carried"); assert!(find_chunk(&post_plte(&valid), b"iTXt").is_some()); } + /// A PNG carries one XMP packet, and §11.3.3.1 Table 21 reserves one keyword for it, so the + /// encoder's packet is a single-value payload: setting it again replaces it. Appending would + /// write two `iTXt` chunks under that keyword, and this crate's reader keeps the first — so + /// the packet set *last* would be the one silently discarded. + /// + /// Kills the `retain` in [`Ancillary::add_xmp`]. Asserted on the written chunk rather than on + /// the entry list because it is the chunk count a reader sees. + #[test] + fn setting_an_xmp_packet_twice_writes_one_chunk() { + let mut a = Ancillary::default(); + a.add_xmp(b"", "", "", false); + a.add_xmp(b"", "", "", false); + let out = post_plte(&a); + assert_eq!( + find_chunk(&out, b"iTXt"), + Some(b"XML:com.adobe.xmp\0\0\0\0\0".to_vec()) + ); + assert_eq!( + out.windows(4).filter(|w| *w == b"iTXt").count(), + 1, + "one chunk, not two" + ); + } + /// A refusal a caller cannot act on is barely better than a silent drop, so it names *which* /// annotation offended — its position and its keyword, escaped so a null shows up. /// diff --git a/crates/gamut-png/src/decoded.rs b/crates/gamut-png/src/decoded.rs index 06551367..220aeb0f 100644 --- a/crates/gamut-png/src/decoded.rs +++ b/crates/gamut-png/src/decoded.rs @@ -125,6 +125,32 @@ pub enum TextChunkKind { CompressedInternational = 3, } +/// How a file framed its XMP packet inside the `iTXt` chunk §11.3.3.1 Table 21 reserves for it. +/// +/// The packet itself is [`DecodedPng::xmp`] / [`PngMetadata::xmp`]; this is everything *else* the +/// chunk carried, and it is `Some` exactly when the packet is. It exists for the same reason +/// [`TextChunkKind`] does — a re-encode has to put the packet back the way it came out — but the +/// packet is surfaced as its own field rather than as a [`TextChunk`], so the framing needs its +/// own home. Table 21 *recommends* the null framing (`compressed` clear, both strings empty) for +/// XMP compliance; it does not require it, and a file that frames it otherwise is still a file +/// whose bytes have to survive a re-encode. +/// +/// Marked `#[non_exhaustive]`: consolidating the packet into [`PngMetadata::texts`] would retire +/// this type, and that is a decision of its own (issue #600). Until then the pairing is a +/// convention, not a type: a caller assembling a [`PngMetadata`] by hand can set one field +/// without the other, and the encoder then takes this type's [`Default`] framing. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +#[non_exhaustive] +pub struct XmpFraming { + /// The chunk's language tag (§11.3.3.4), if it carried a non-empty one. + pub language: Option, + /// The chunk's translated keyword (§11.3.3.4), if it carried a non-empty one. + pub translated_keyword: Option, + /// Whether the packet was stored zlib-compressed (§11.3.3.4's compression flag). A packet + /// stored compressed and rewritten uncompressed is the same words at many times the size. + pub compressed: bool, +} + /// One text annotation (tEXt/zTXt/iTXt, §11.3.3), decompressed where stored compressed. /// /// tEXt/zTXt hold Latin-1, mapped code-point-for-code-point into the `String` (lossless); @@ -167,9 +193,12 @@ pub struct DecodedPng { pub exif: Option>, /// The embedded ICC profile (iCCP), inflated. Feed as `MetadataBlock::Icc`. pub icc_profile: Option, - /// The XMP packet (the `XML:com.adobe.xmp` iTXt, §11.3.3.2), decompressed if stored + /// The XMP packet (the `XML:com.adobe.xmp` iTXt, §11.3.3.4), decompressed if stored /// compressed. Feed as `MetadataBlock::Xmp`. pub xmp: Option>, + /// How the chunk that carried [`xmp`](Self::xmp) framed it: its compression flag, language + /// tag and translated keyword (§11.3.3.4). `Some` exactly when `xmp` is. + pub xmp_framing: Option, /// The C2PA manifest store (the `caBX` chunk, C2PA 2.4 §A.3.2) verbatim: the JUMBF bytes, /// uncompressed, exactly as the chunk carries them — opaque here, never parsed or judged. /// Feed as `MetadataBlock::C2pa`. The first CRC-valid `caBX` before the first `IDAT`, and @@ -243,9 +272,12 @@ pub struct PngMetadata { pub exif: Option>, /// The embedded ICC profile (iCCP), inflated. Feed as `MetadataBlock::Icc`. pub icc_profile: Option, - /// The XMP packet (the `XML:com.adobe.xmp` iTXt, §11.3.3.2), decompressed if stored + /// The XMP packet (the `XML:com.adobe.xmp` iTXt, §11.3.3.4), decompressed if stored /// compressed. Feed as `MetadataBlock::Xmp`. pub xmp: Option>, + /// How the chunk that carried [`xmp`](Self::xmp) framed it: its compression flag, language + /// tag and translated keyword (§11.3.3.4). `Some` exactly when `xmp` is. + pub xmp_framing: Option, /// The C2PA manifest store (the `caBX` chunk, C2PA 2.4 §A.3.2) verbatim and uncompressed — /// opaque bytes, never parsed or judged. Feed as `MetadataBlock::C2pa`. The first CRC-valid /// `caBX` before the first `IDAT`, and only when it fits the metadata budget; see @@ -351,9 +383,10 @@ pub(crate) fn collect(chunks: &[([u8; 4], &[u8])], budget: usize) -> PngMetadata } } b"iTXt" => match parse_itxt(data, &mut budget) { - Some(ITxt::Xmp(packet)) => { + Some(ITxt::Xmp(packet, framing)) => { if meta.xmp.is_none() { meta.xmp = Some(packet); + meta.xmp_framing = Some(framing); } } Some(ITxt::Text(text)) => meta.texts.push(text), @@ -369,9 +402,10 @@ pub(crate) fn collect(chunks: &[([u8; 4], &[u8])], budget: usize) -> PngMetadata /// by §11.3.3.1 Table 21. Shared with the encoder so the two sides cannot disagree on it. pub(crate) const XMP_KEYWORD: &str = "XML:com.adobe.xmp"; -/// A parsed iTXt: either the XMP packet or an ordinary text annotation. +/// A parsed iTXt: either the XMP packet and how its chunk framed it, or an ordinary text +/// annotation. enum ITxt { - Xmp(Vec), + Xmp(Vec, XmpFraming), Text(TextChunk), } @@ -475,7 +509,17 @@ fn parse_itxt(data: &[u8], budget: &mut usize) -> Option { _ => return None, }; if keyword == XMP_KEYWORD { - return Some(ITxt::Xmp(text_bytes)); + // The packet leaves by its own field, so everything the chunk framed it with — the + // compression flag above all — leaves beside it rather than with the annotation list. + // Without the flag a 71-byte chunk is rewritten as thousands of uncompressed bytes. + return Some(ITxt::Xmp( + text_bytes, + XmpFraming { + language: Some(language).filter(|l| !l.is_empty()), + translated_keyword: Some(translated).filter(|t| !t.is_empty()), + compressed: flag == 1, + }, + )); } Some(ITxt::Text(TextChunk { keyword, diff --git a/crates/gamut-png/src/decoder.rs b/crates/gamut-png/src/decoder.rs index cc7cb071..1ffc18f2 100644 --- a/crates/gamut-png/src/decoder.rs +++ b/crates/gamut-png/src/decoder.rs @@ -436,6 +436,7 @@ impl PngDecoder { exif: meta.exif, icc_profile: meta.icc_profile, xmp: meta.xmp, + xmp_framing: meta.xmp_framing, c2pa: meta.c2pa, c2pa_ignored: meta.c2pa_ignored, texts: meta.texts, diff --git a/crates/gamut-png/src/encoder.rs b/crates/gamut-png/src/encoder.rs index 716b839b..1484d879 100644 --- a/crates/gamut-png/src/encoder.rs +++ b/crates/gamut-png/src/encoder.rs @@ -32,7 +32,7 @@ use crate::backend::{IdatDeflater, IdatInfo, Registry, run_deflaters}; use crate::chunk::{self, C2paSpan, SIGNATURE}; use crate::color::ColorType; use crate::decoded::{ - Chromaticities, Cicp, DecodedPng, IccProfile, PngMetadata, TextChunk, TextChunkKind, + Chromaticities, Cicp, DecodedPng, IccProfile, PngMetadata, TextChunk, TextChunkKind, XmpFraming, }; use crate::filter::{self, FilterStrategy, FilterType}; use crate::palette::PngPalette; @@ -87,6 +87,9 @@ struct MetadataView<'a> { exif: Option<&'a [u8]>, icc_profile: Option<&'a IccProfile>, xmp: Option<&'a [u8]>, + /// How the source framed its XMP packet (§11.3.3.4): compression flag, language tag, + /// translated keyword. Carried beside the packet because the packet has its own field. + xmp_framing: Option<&'a XmpFraming>, texts: &'a [TextChunk], gamma: Option, chromaticities: Option, @@ -97,33 +100,66 @@ struct MetadataView<'a> { c2pa: bool, } -/// A metadata payload [`PngEncoder::with_metadata`] could not carry into the output. +/// Something [`PngEncoder::with_metadata`] could not do faithfully with a payload it was given. /// -/// Preservation exists to stop metadata disappearing quietly, so the two payloads a carry cannot -/// take are named rather than dropped in silence. Read them back with -/// [`PngEncoder::dropped_metadata`] and tell the user — `gamut convert` does. +/// Preservation exists to stop metadata disappearing quietly, so anything a carry cannot take — +/// and anything it takes only by writing bytes the specification does not endorse — is named +/// rather than passed over. Read them back with [`PngEncoder::metadata_notices`] and tell the +/// user; `gamut convert` does. [`carried`](Self::carried) separates the two cases: a payload +/// left behind from one that reached the output with a caveat on it. +/// +/// This is deliberately **not** an error channel. The only thing that stops an encode is a null +/// byte in a text field, which makes the chunk re-parse as a different annotation; everything +/// here is something a caller has to *know*, not something that should fail a conversion whose +/// pixels are fine. /// /// `#[repr(u8)]` with explicit discriminants, which are permanent and append-only: the value /// crosses the C ABI as a plain integer. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] #[repr(u8)] #[non_exhaustive] -pub enum DroppedMetadata { - /// A `cICP` whose matrix coefficients are not 0. §11.3.2.6 requires 0 for PNG — "RGB is - /// currently the only supported color model in PNG, and as such Matrix Coefficients shall be - /// set to 0" — so the source chunk is not conforming and copying it forward would reproduce - /// the defect in a file this encoder signed off on. +pub enum MetadataNotice { + /// A `cICP` whose matrix coefficients are not 0, left behind. §11.3.2.6 requires 0 for PNG — + /// "RGB is currently the only supported color model in PNG, and as such Matrix Coefficients + /// shall be set to 0" — so the source chunk is not conforming and copying it forward would + /// reproduce the defect in a file this encoder signed off on. NonRgbCicp = 0, - /// The C2PA manifest store (`caBX`). A store is signed over the exact bytes of the file it - /// was made for, which is why C2PA 2.4 §A.3.2 marks the chunk unsafe to copy: carried into a - /// re-encode it is invalid by construction, and a validator reports a *tampered* file rather - /// than an unsigned one. Re-sign the output and set it with + /// The C2PA manifest store (`caBX`), left behind. A store is signed over the exact bytes of + /// the file it was made for, which is why C2PA 2.4 §A.3.2 marks the chunk unsafe to copy: + /// carried into a re-encode it is invalid by construction, and a validator reports a + /// *tampered* file rather than an unsigned one. Re-sign the output and set it with /// [`with_c2pa`](PngEncoder::with_c2pa). C2paManifestStore = 1, + /// A text annotation left behind because its keyword holds a character Latin-1 cannot + /// encode. §11.3.3.1 binds the keyword to Latin-1 in *all three* text chunks, so unlike the + /// text — which §11.3.3.2 routes to `iTXt` — there is no chunk that could carry it. + TextKeywordNotLatin1 = 2, + /// A text annotation left behind because its keyword is empty or longer than the 79 bytes + /// §11.3.3.1 allows. All three chunks fix that field at 1–79 bytes, so a reader — this + /// crate's own included — drops the whole chunk rather than reading a longer one. + TextKeywordLength = 3, + /// A text annotation **written**, whose keyword leaves the repertoire §11.3.3.1 recommends + /// ("only code points 0x20-7E and 0xA1-FF are allowed", and expressly "nor is U+00A0 + /// NON-BREAKING SPACE"). The keyword is written exactly as it arrived — this crate reads it + /// back unchanged — but another reader need not be so forgiving. + TextKeywordRepertoire = 4, + /// A text annotation **written**, whose keyword has a leading, trailing or consecutive + /// space, which §11.3.3.1 says are "not permitted in keywords" so that one keyword cannot be + /// misread as another. Written as it arrived, for the same reason as + /// [`TextKeywordRepertoire`](Self::TextKeywordRepertoire). + TextKeywordSpacing = 5, + /// A text annotation **written without its `iTXt` language tag**, because the tag was not + /// the ASCII shape §11.3.3.4 requires ("a well-formed language tag defined by [BCP47]"). + /// Written as UTF-8 into a field a reader takes as Latin-1 the tag would not survive the + /// trip; an empty tag is §11.3.3.4's own way of saying the language is unspecified. + ItxtLanguageTag = 6, + /// An XMP packet left behind because it is not UTF-8. §11.3.3.4 gives the `iTXt` text field + /// UTF-8 and no alternative, so there is no chunk to frame it in. + XmpNotUtf8 = 7, } -impl DroppedMetadata { - /// One line naming what was left behind and why, fit to show a user. +impl MetadataNotice { + /// One line naming the payload and what happened to it, fit to show a user. #[must_use] pub fn reason(self) -> &'static str { match self { @@ -134,11 +170,48 @@ impl DroppedMetadata { "C2PA manifest store: signed over the source bytes, so a copy would be invalid \ (C2PA 2.4 §A.3.2) — re-sign the output" } + Self::TextKeywordNotLatin1 => { + "text annotation: its keyword is not Latin-1, which every text chunk requires \ + (§11.3.3.1)" + } + Self::TextKeywordLength => { + "text annotation: its keyword is not 1 to 79 bytes, the length every text chunk \ + fixes (§11.3.3.1)" + } + Self::TextKeywordRepertoire => { + "text annotation: written, but its keyword leaves the code points 0x20-0x7E and \ + 0xA1-0xFF §11.3.3.1 recommends — another reader may reject it" + } + Self::TextKeywordSpacing => { + "text annotation: written, but its keyword has a leading, trailing or \ + consecutive space, which §11.3.3.1 does not permit" + } + Self::ItxtLanguageTag => { + "text annotation: written without its language tag, which was not the BCP 47 \ + shape §11.3.3.4 requires" + } + Self::XmpNotUtf8 => { + "XMP packet: not UTF-8, and an iTXt text string must be (§11.3.3.4)" + } } } + + /// Whether the payload still reached the output. + /// + /// `false` means it was left behind entirely; `true` means it was written, with the caveat + /// [`reason`](Self::reason) gives. A caller showing these to a user needs the difference — + /// "this did not come along" and "this came along in a form some readers dislike" call for + /// different action. + #[must_use] + pub fn carried(self) -> bool { + matches!( + self, + Self::TextKeywordRepertoire | Self::TextKeywordSpacing | Self::ItxtLanguageTag + ) + } } -impl core::fmt::Display for DroppedMetadata { +impl core::fmt::Display for MetadataNotice { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { f.write_str(self.reason()) } @@ -154,9 +227,11 @@ pub struct PngEncoder { auto_reduce: bool, clean_transparent: bool, backends: Registry, - /// What the last metadata carry could not take, in the order it was found. Reset by each - /// [`Self::with_metadata`] / [`Self::with_metadata_from`] call, so it describes that call. - dropped: Vec, + /// What the last metadata carry could not take *as a whole payload*, in the order it was + /// found. Reset by each [`Self::with_metadata`] / [`Self::with_metadata_from`] call, so it + /// describes that call. Per-annotation notices live with their annotation instead, so that a + /// second carry replaces them exactly as it replaces the annotations themselves. + carry_notices: Vec, } impl Default for PngEncoder { @@ -178,7 +253,7 @@ impl PngEncoder { auto_reduce: false, clean_transparent: false, backends: Registry::default(), - dropped: Vec::new(), + carry_notices: Vec::new(), } } @@ -468,7 +543,11 @@ impl PngEncoder { /// the XMP/RDF document — for example the bytes produced by `gamut-xmp`. #[must_use] pub fn with_xmp(mut self, xmp: &str) -> Self { - self.ancillary.add_xmp(xmp.as_bytes()); + // §11.3.3.1 Table 21: "The use of iTXt, with Compression Flag set to 0, and both Language + // Tag and Translated Keyword set to the null string, are recommended for XMP compliance." + // A packet read out of a file that framed it otherwise keeps its framing; this entry + // point has no framing to keep, so it takes the recommended one. + self.ancillary.add_xmp(xmp.as_bytes(), "", "", false); self } @@ -491,18 +570,24 @@ impl PngEncoder { /// together — §4.3 Table 1 ranks the colour chunks precisely so a file may carry more than /// one, and a reader honours the lowest priority number. Each text annotation goes back into /// the chunk it came out of, compressed if it was compressed - /// ([`TextChunkKind`](crate::TextChunkKind)). + /// ([`TextChunkKind`](crate::TextChunkKind)); so does the XMP packet, whose own framing — + /// compression flag, language tag, translated keyword — rides in + /// [`XmpFraming`](crate::XmpFraming). /// - /// Two payloads cannot be carried, and both are **named** rather than dropped in silence — - /// read them back with [`dropped_metadata`](Self::dropped_metadata): + /// Two payloads cannot be carried at all, and neither is dropped in silence — read them back + /// with [`metadata_notices`](Self::metadata_notices): /// /// - a **`cICP` whose matrix coefficients are not 0**, which §11.3.2.6 does not allow in PNG; /// - the **C2PA manifest store**, signed over the bytes of the file it was made for. /// - /// Anything that would be *corrupted* rather than lost — a keyword outside §11.3.3.1's - /// repertoire, a null inside a text string, an XMP packet that is not UTF-8 — makes the - /// encode fail with [`Error::InvalidInput`] naming the annotation, rather than being written - /// as something a reader reads back differently. + /// A text annotation whose keyword or XMP packet §11.3.3 does not endorse is reported through + /// the same channel rather than failing the carry: a keyword outside §11.3.3.1's repertoire + /// or spacing rules is written as it arrived, a keyword no chunk can hold and an XMP packet + /// that is not UTF-8 are left behind, and + /// [`MetadataNotice::carried`](MetadataNotice::carried) says which happened. **Only a null** + /// in a keyword or text string fails the encode with [`Error::InvalidInput`] naming the + /// annotation — the null is the field separator, so the chunk would be read back as a + /// *different* annotation, which no notice can undo. /// /// One further limit is the read side's, not this method's: `pHYs`, `tIME`, `sBIT` and `bKGD` /// are not part of [`PngMetadata`], so they cannot be carried here (set them with their own @@ -513,6 +598,7 @@ impl PngEncoder { exif: metadata.exif.as_deref(), icc_profile: metadata.icc_profile.as_ref(), xmp: metadata.xmp.as_deref(), + xmp_framing: metadata.xmp_framing.as_ref(), texts: &metadata.texts, gamma: metadata.gamma, chromaticities: metadata.chromaticities, @@ -533,6 +619,7 @@ impl PngEncoder { exif: decoded.exif.as_deref(), icc_profile: decoded.icc_profile.as_ref(), xmp: decoded.xmp.as_deref(), + xmp_framing: decoded.xmp_framing.as_ref(), texts: &decoded.texts, gamma: decoded.gamma, chromaticities: decoded.chromaticities, @@ -542,22 +629,30 @@ impl PngEncoder { }) } - /// What the last [`with_metadata`](Self::with_metadata) / - /// [`with_metadata_from`](Self::with_metadata_from) call could not carry, in the order it was - /// found — empty when it carried everything, and reset by each call. + /// What this encoder could not carry faithfully: whole payloads left behind, then the + /// per-annotation notices, in the order they were found — empty when everything came along + /// intact. /// /// Surface this to whoever asked for the re-encode. Losing metadata without saying so is the - /// defect the preservation path exists to remove; losing it *with* an explanation is a - /// choice the spec forces. + /// defect the preservation path exists to remove; losing it — or bending it — *with* an + /// explanation is a choice the spec forces. Use + /// [`MetadataNotice::carried`](MetadataNotice::carried) to tell the two apart. + /// + /// The payload-level notices describe the last [`with_metadata`](Self::with_metadata) / + /// [`with_metadata_from`](Self::with_metadata_from) call and are reset by each; the + /// per-annotation notices belong to the annotations still accumulated, so they follow the + /// same replace-not-append rule a carry gives the text list. #[must_use] - pub fn dropped_metadata(&self) -> &[DroppedMetadata] { - &self.dropped + pub fn metadata_notices(&self) -> Vec { + let mut notices = self.carry_notices.clone(); + notices.extend(self.ancillary.text_notices()); + notices } /// The one implementation behind [`with_metadata`](Self::with_metadata) and /// [`with_metadata_from`](Self::with_metadata_from). fn with_metadata_view(mut self, meta: MetadataView<'_>) -> Self { - self.dropped.clear(); + self.carry_notices.clear(); self.ancillary.begin_carry(); if let Some(exif) = meta.exif { self = self.with_exif(exif); @@ -577,7 +672,7 @@ impl PngEncoder { // otherwise is not a conforming cICP; carrying it forward would put the same defect // in the output. Some(cicp) if cicp.matrix_coefficients != 0 => { - self.dropped.push(DroppedMetadata::NonRgbCicp); + self.carry_notices.push(MetadataNotice::NonRgbCicp); } Some(cicp) => { self = self.with_cicp( @@ -589,7 +684,7 @@ impl PngEncoder { None => {} } if meta.c2pa { - self.dropped.push(DroppedMetadata::C2paManifestStore); + self.carry_notices.push(MetadataNotice::C2paManifestStore); } // Set in the stored ×100 000 fixed-point units rather than through `with_gamma` / // `with_chromaticities`, whose `f64` arguments would round-trip the value through a @@ -609,10 +704,21 @@ impl PngEncoder { chrm.blue.1, ]); } - // Handed over as bytes, because that is what the chunk held. §11.3.3.4 requires UTF-8, so - // a packet that is not gets a refusal at `encode` naming it — never a silent drop. + // Handed over as bytes, because that is what the chunk held, and with the framing its + // chunk gave it — above all §11.3.3.4's compression flag, without which a packet stored + // as 71 compressed bytes is rewritten as the 4 045 it inflates to. §11.3.3.4 requires + // UTF-8, so a packet that is not is reported by `metadata_notices` — never a silent drop. if let Some(xmp) = meta.xmp { - self.ancillary.add_xmp(xmp); + let (language, translated, compressed) = + meta.xmp_framing.map_or(("", "", false), |f| { + ( + f.language.as_deref().unwrap_or_default(), + f.translated_keyword.as_deref().unwrap_or_default(), + f.compressed, + ) + }); + self.ancillary + .add_xmp(xmp, language, translated, compressed); } for text in meta.texts { let (language, translated) = ( diff --git a/crates/gamut-png/src/lib.rs b/crates/gamut-png/src/lib.rs index ab7fdce5..ba4cab1c 100644 --- a/crates/gamut-png/src/lib.rs +++ b/crates/gamut-png/src/lib.rs @@ -91,14 +91,14 @@ pub use chunk::{C2paSpan, fill_c2pa}; pub use color::ColorType; pub use decoded::{ Chromaticities, Cicp, DecodedPng, IccProfile, PngHeader, PngImage, PngMetadata, TextChunk, - TextChunkKind, + TextChunkKind, XmpFraming, }; pub use decoder::{PngDecoder, TransparencyKey, metadata}; pub use deconstruct::{ ChunkStats, DEFAULT_MAX_CHUNKS, DeconstructLimits, FilterHistogram, FilterScan, PassStats, PngReport, Segment, SegmentKind, SkippedFilterScan, deconstruct, deconstruct_with_limits, }; -pub use encoder::{DroppedMetadata, PngEncodeReport, PngEncoder}; +pub use encoder::{MetadataNotice, PngEncodeReport, PngEncoder}; pub use filter::{FilterStrategy, FilterType}; /// The DEFLATE compression level, accepted by [`PngEncoder::with_compression`]. pub use gamut_deflate::Level; diff --git a/crates/gamut-png/tests/preservation.rs b/crates/gamut-png/tests/preservation.rs index c39f39f2..1592df7b 100644 --- a/crates/gamut-png/tests/preservation.rs +++ b/crates/gamut-png/tests/preservation.rs @@ -10,7 +10,7 @@ mod common; use common::{chunk, ihdr_payload, png_from_chunks, tiny_exif, tiny_icc_profile, zlib}; use gamut_core::{Dimensions, EncodeImage, ErrorKind, ImageRef, Rgb8}; -use gamut_png::{DroppedMetadata, PngDecoder, PngEncoder, PngMetadata, SrgbIntent}; +use gamut_png::{MetadataNotice, PngDecoder, PngEncoder, PngMetadata, SrgbIntent}; /// The `cHRM` payload for the sRGB primaries, in the ×100 000 units §11.3.2.1 stores. const CHRM: [u32; 8] = [ @@ -36,7 +36,7 @@ fn source(extra: &[Vec]) -> Vec { chunk(b"cHRM", &chrm), chunk(b"tEXt", b"Author\0caf\xE9"), chunk(b"iTXt", b"Note\0\0\0de\0Notiz\0g\xC3\xA4mut"), - chunk(b"iTXt", b"XML:com.adobe.xmp\0\0\0\0\0"), + chunk(b"iTXt", &compressed_xmp()), chunk(b"caBX", b"\0\0\0\x10jumbc2pa"), ]; chunks.extend_from_slice(extra); @@ -45,6 +45,42 @@ fn source(extra: &[Vec]) -> Vec { png_from_chunks(&chunks) } +/// The `iTXt` payload for a **compressed** XMP packet carrying both §11.3.3.4 fields. +/// +/// §11.3.3.1 Table 21 recommends the null framing for XMP compliance — flag 0, both strings +/// empty — but recommends is all it does, and a provenance packet is exactly the payload a +/// writer compresses. The uncompressed fixture that stood here could not see the flag being +/// dropped, which is how a 57× inflation went unnoticed. +fn compressed_xmp() -> Vec { + let mut itxt = b"XML:com.adobe.xmp\0\x01\0en\0Metadata\0".to_vec(); + itxt.extend_from_slice(&zlib(&xmp_packet())); + itxt +} + +/// A realistic XMP packet: repetitive RDF followed by the whitespace padding XMP Part 3 +/// recommends so an in-place update can grow without rewriting the file. That padding is exactly +/// why a real packet is stored compressed, and exactly what a writer that loses the compression +/// flag puts back in full. +fn xmp_packet() -> Vec { + let mut packet = XMP_RDF.as_bytes().to_vec(); + packet.resize(packet.len() + 3_072, b' '); + packet.extend_from_slice(b""); + packet +} + +/// The RDF body of [`xmp_packet`]. +const XMP_RDF: &str = concat!( + "", + "", + "", + "a title", + "a creator", + "a notice", + "a description", + "", +); + /// A source carrying only `extra` between the header and the image data — for a claim about one /// annotation, which the full [`source`] pile would confuse with its own. fn minimal_source(extra: &[Vec]) -> Vec { @@ -93,6 +129,7 @@ fn every_carried_chunk_survives_a_re_encode() { assert_eq!(re.exif, meta.exif); assert_eq!(re.icc_profile, meta.icc_profile); assert_eq!(re.xmp, meta.xmp); + assert_eq!(re.xmp_framing, meta.xmp_framing); assert_eq!(re.gamma, Some(45_455)); let chrm = re.chromaticities.expect("cHRM carried"); assert_eq!( @@ -166,10 +203,10 @@ fn a_cicp_is_carried_only_when_its_matrix_coefficients_are_zero() { // Dropped, but not in silence: the caller can say so. assert!( encoder - .dropped_metadata() - .contains(&DroppedMetadata::NonRgbCicp), + .metadata_notices() + .contains(&MetadataNotice::NonRgbCicp), "{:?}", - encoder.dropped_metadata() + encoder.metadata_notices() ); } @@ -185,8 +222,8 @@ fn the_c2pa_manifest_store_is_never_carried_forward() { let encoder = PngEncoder::new().with_metadata(&meta); assert!(re_encoded(|_| encoder.clone()).c2pa.is_none()); assert_eq!( - encoder.dropped_metadata(), - [DroppedMetadata::C2paManifestStore] + encoder.metadata_notices(), + [MetadataNotice::C2paManifestStore] ); } @@ -258,6 +295,144 @@ fn a_compressed_itxt_goes_back_into_a_compressed_itxt() { ); } +/// The same claim for the XMP packet, which is where it was untrue: the packet leaves the read +/// side through its own field, so the `iTXt` framing that field does *not* hold — §11.3.3.4's +/// compression flag above all — has to travel beside it or be invented at the writer. +/// +/// A provenance packet is exactly the payload a writer compresses, and rewriting one +/// uncompressed inflates it by a factor a user notices. Kills a mutant that ignores +/// [`XmpFraming::compressed`](gamut_png::XmpFraming::compressed). +#[test] +fn a_compressed_xmp_packet_goes_back_into_a_compressed_itxt() { + let meta = gamut_png::metadata(&minimal_source(&[chunk(b"iTXt", &compressed_xmp())])).unwrap(); + assert_eq!(meta.xmp, Some(xmp_packet())); + + let out = re_encoded_bytes(|e| e.with_metadata(&meta)); + let carried = chunk_payload(&out, b"iTXt").expect("the packet"); + // keyword, NUL, then §11.3.3.4's compression flag. + assert_eq!(carried[18], 1, "the flag is set: {carried:?}"); + + // Measured against the same carry with the flag cleared, so the claim is the inflation the + // flag prevents rather than a threshold that happens to hold for this packet. + let mut flat = meta.clone(); + flat.xmp_framing.as_mut().expect("framed").compressed = false; + let flat_out = re_encoded_bytes(|e| e.with_metadata(&flat)); + let inflated = chunk_payload(&flat_out, b"iTXt").expect("the packet"); + assert!( + carried.len() * 2 < inflated.len(), + "{} bytes compressed against {} uncompressed", + carried.len(), + inflated.len() + ); +} + +/// §11.3.3.4's language tag and translated keyword are as much a part of the XMP chunk as of any +/// other `iTXt`, and §11.3.3.1 Table 21 only *recommends* leaving them empty. A file that fills +/// them is a file whose bytes have to come back. +/// +/// Separate from the compression claim above because a writer can keep the flag and still drop +/// the two strings — the defect this pins was exactly that pair going missing together. +#[test] +fn an_xmp_packet_keeps_its_language_and_translated_keyword() { + let meta = gamut_png::metadata(&source(&[])).unwrap(); + let framing = meta.xmp_framing.clone().expect("the source frames it"); + assert_eq!(framing.language.as_deref(), Some("en")); + assert_eq!(framing.translated_keyword.as_deref(), Some("Metadata")); + assert!(framing.compressed); + + let re = re_encoded(|e| e.with_metadata(&meta)); + assert_eq!(re.xmp_framing, Some(framing)); +} + +/// A PNG carries one XMP packet, so the encoder's packet is a single-value payload like `eXIf` or +/// `iCCP`: setting it again replaces it. Appending instead wrote two `iTXt` chunks under the one +/// keyword §11.3.3.1 Table 21 reserves, and this crate's reader keeps the *first* — so the packet +/// a caller carried in was the one silently discarded, inside the feature built to end silent +/// discarding. +#[test] +fn carrying_an_xmp_packet_replaces_one_already_set() { + let meta = gamut_png::metadata(&source(&[])).unwrap(); + let out = re_encoded_bytes(|e| { + e.with_xmp("") + .with_metadata(&meta) + }); + + let mut packets = 0; + let mut i = 8; + while i + 12 <= out.len() { + let len = u32::from_be_bytes([out[i], out[i + 1], out[i + 2], out[i + 3]]) as usize; + if &out[i + 4..i + 8] == b"iTXt" && out[i + 8..].starts_with(b"XML:com.adobe.xmp\0") { + packets += 1; + } + i += 12 + len; + } + assert_eq!(packets, 1, "one keyword, one chunk"); + assert_eq!( + gamut_png::metadata(&out).unwrap().xmp, + meta.xmp, + "and it is the carried packet, not the one it replaced" + ); +} + +/// §11.3.3.1's keyword *shape* rules are lowercase throughout — "Keywords shall contain only +/// printable Latin-1", "leading spaces, trailing spaces, and consecutive spaces are not +/// permitted" — and §15 gives the BCP 14 keywords force "when, and only when, they appear in all +/// capitals". This crate's reader accepts every one of these keywords and returns them +/// unchanged, so refusing to write them back would fail a conversion over a file whose pixels +/// are fine, leaving no escape but to discard the file's metadata entirely. +/// +/// So they are written verbatim and reported. Kills a mutant that turns any of these back into a +/// refusal, or that drops the annotation instead of writing it. +#[test] +fn a_keyword_the_reader_accepts_survives_the_re_encode_with_a_notice() { + for (keyword, notice) in [ + (" Author", MetadataNotice::TextKeywordSpacing), + ("Author ", MetadataNotice::TextKeywordSpacing), + ("Two Words", MetadataNotice::TextKeywordSpacing), + ("Auth\u{7F}or", MetadataNotice::TextKeywordRepertoire), + ("Auth\u{A0}or", MetadataNotice::TextKeywordRepertoire), + ] { + let mut text = keyword.as_bytes().to_vec(); + text.extend_from_slice(b"\0body"); + let png = minimal_source(&[chunk(b"tEXt", &text)]); + let meta = gamut_png::metadata(&png).unwrap(); + assert_eq!(meta.texts.len(), 1, "the reader accepts {keyword:?}"); + + let encoder = PngEncoder::new().with_metadata(&meta); + assert_eq!(encoder.metadata_notices(), [notice], "keyword {keyword:?}"); + let re = re_encoded(|_| encoder.clone()); + assert_eq!(re.texts, meta.texts, "keyword {keyword:?} came back whole"); + } +} + +/// The other half of the same line: a keyword no chunk can hold is left behind rather than +/// written, because all three text chunks fix that field at 1–79 Latin-1 bytes and a reader — +/// this crate's own included — drops a chunk whose keyword busts it. Writing it would be the +/// silent loss, so the annotation goes and the notice stays. +/// +/// Driven through the setters, because the reader will not produce such a keyword from a file. +#[test] +fn a_keyword_no_chunk_can_hold_is_left_behind_with_a_notice() { + for (keyword, notice) in [ + ("", MetadataNotice::TextKeywordLength), + (&"k".repeat(80), MetadataNotice::TextKeywordLength), + ("题", MetadataNotice::TextKeywordNotLatin1), + ] { + let encoder = PngEncoder::new().with_text(keyword, "body"); + assert_eq!( + encoder.metadata_notices(), + [notice], + "keyword of {} chars", + keyword.chars().count() + ); + assert!( + re_encoded(|_| encoder.clone()).texts.is_empty(), + "keyword of {} chars was not written", + keyword.chars().count() + ); + } +} + /// Carrying the same metadata twice is carrying it once. The single-value slots are idempotent /// because a second write overwrites the first; the text list is the one place where a second /// call would otherwise append a duplicate of every annotation — which is what a caller that @@ -274,48 +449,92 @@ fn carrying_the_same_metadata_twice_carries_it_once() { /// §11.3.3.4 gives the `iTXt` text field UTF-8 and no alternative, so a packet that is not UTF-8 /// has no chunk this encoder can frame. The read side hands it over as raw bytes regardless — it -/// reports what the file held — so the write side is where it has to be said out loud. Refusing -/// is the point: the alternative is a caller who asked for preservation and got a file with the -/// packet missing and nothing to read about it. +/// reports what the file held — so the write side is where it has to be said out loud. It is +/// **reported, not refused**: the pixels of such a file are fine, and failing the whole encode +/// would leave a caller no way to convert it but to discard its ICC profile too. #[test] -fn a_non_utf8_xmp_packet_refuses_the_re_encode() { +fn a_non_utf8_xmp_packet_is_reported_and_the_re_encode_proceeds() { let mut itxt = b"XML:com.adobe.xmp\0\0\0\0\0".to_vec(); itxt.extend_from_slice(b""); let meta = gamut_png::metadata(&minimal_source(&[chunk(b"iTXt", &itxt)])).unwrap(); assert!(meta.xmp.is_some(), "the read side surfaces the raw packet"); + let encoder = PngEncoder::new().with_metadata(&meta); + assert_eq!(encoder.metadata_notices(), [MetadataNotice::XmpNotUtf8]); + assert!( + !MetadataNotice::XmpNotUtf8.carried(), + "the packet is left behind, not written" + ); + assert!(re_encoded(|_| encoder.clone()).xmp.is_none()); +} + +/// A null byte is the one thing that still refuses, because it is the field separator: a `tEXt` +/// carrying `Note\0Author\0other` re-parses as a *different* annotation, so writing it would make +/// the file mean something the caller never supplied. No notice can undo that. +#[test] +fn a_null_in_a_carried_text_string_refuses_the_re_encode() { + // Built through the setter rather than a fixture: the reader splits a chunk at its first + // null, so no file can hand a null to the carry — only a caller can. let pixels = vec![0u8; 3 * 4]; let image = ImageRef::::new(&pixels, Dimensions::new(2, 2).unwrap()).unwrap(); let error = PngEncoder::new() - .with_metadata(&meta) + .with_text("Note", "before\0after") .encode_to_vec(image) .expect_err("refused"); assert_eq!(error.kind(), ErrorKind::InvalidInput); assert!( - error.to_string().contains("XMP packet is not UTF-8"), + error + .to_string() + .contains("may not contain a null character"), "{error}" ); } -/// Naming a dropped payload is only useful if the name says something. `gamut convert` prints -/// these lines and they are the whole of what a user learns about metadata that did not survive, -/// so each has to identify the payload and give the reason it could not come along. +/// Naming a payload is only useful if the name says something. `gamut convert` prints these +/// lines and they are the whole of what a user learns about metadata that did not survive +/// intact, so each has to identify the payload and give the reason. /// /// Pinned here rather than in `gamut-cli`, whose tests the mutation gate cannot see: a mutant -/// that empties [`DroppedMetadata::reason`] or its `Display` would otherwise leave the command +/// that empties [`MetadataNotice::reason`] or its `Display` would otherwise leave the command /// printing nothing at all. #[test] -fn a_dropped_payload_is_named_in_words() { - let store = DroppedMetadata::C2paManifestStore.to_string(); +fn a_notice_names_its_payload_in_words() { + let store = MetadataNotice::C2paManifestStore.to_string(); assert!(store.contains("C2PA manifest store"), "{store}"); assert!(store.contains("re-sign"), "{store}"); - let cicp = DroppedMetadata::NonRgbCicp.to_string(); + let cicp = MetadataNotice::NonRgbCicp.to_string(); assert!(cicp.contains("cICP"), "{cicp}"); assert!(cicp.contains("matrix coefficients"), "{cicp}"); assert_eq!( cicp, - DroppedMetadata::NonRgbCicp.reason(), + MetadataNotice::NonRgbCicp.reason(), "Display is the reason" ); } + +/// The whole point of the channel is that "it did not come along" and "it came along bent" are +/// different news for a user, so [`MetadataNotice::carried`] has to separate them — and it is +/// the only thing that does. +/// +/// Kills a mutant that makes `carried` constant either way, which would have `gamut convert` +/// telling a user their ICC profile was dropped when it was not. +#[test] +fn a_notice_says_whether_the_payload_reached_the_output() { + for carried in [ + MetadataNotice::TextKeywordRepertoire, + MetadataNotice::TextKeywordSpacing, + MetadataNotice::ItxtLanguageTag, + ] { + assert!(carried.carried(), "{carried:?}"); + } + for lost in [ + MetadataNotice::NonRgbCicp, + MetadataNotice::C2paManifestStore, + MetadataNotice::TextKeywordNotLatin1, + MetadataNotice::TextKeywordLength, + MetadataNotice::XmpNotUtf8, + ] { + assert!(!lost.carried(), "{lost:?}"); + } +} From 5d714256167647ac953a3b80b96f830bba6c9e65 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 07:17:27 -0400 Subject: [PATCH 08/14] docs(png): correct what preservation carries and what it only reports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The metadata-preservation section claimed identity was preserved under a heading about identity, while the XMP packet — the largest payload the path carries — lost its compression flag, language tag and translated keyword. It also listed §11.3.3.1's keyword rules as enforced, when enforcing them refused five keyword shapes this crate's own reader accepts. Records instead: what the XMP packet's framing costs when it is lost (a 354-byte `iTXt` rewritten as 3 734, measured on the fixture); the three-way split between what refuses the encode, what is dropped and reported, and what is written verbatim and reported, with the §15 argument for the line; and the §11.3.3.1/§11.3.3.2 contradiction about a `tEXt` text string, quoting both halves from the vendored text rather than picking one silently. The "not done" list gains the seams #600 would close — the packet's parallel fields and its position among the annotations — and the efficiency table's metadata-hygiene axis no longer says `gamut convert` drops metadata on the PNG path, which this work made untrue. Refs #483. Refs #600. --- crates/gamut-png/STATUS.md | 92 ++++++++++++++++++++++++++------------ 1 file changed, 63 insertions(+), 29 deletions(-) diff --git a/crates/gamut-png/STATUS.md b/crates/gamut-png/STATUS.md index dedef130..3c34b64a 100644 --- a/crates/gamut-png/STATUS.md +++ b/crates/gamut-png/STATUS.md @@ -40,7 +40,7 @@ opts into narrowing. That is distinct from the encoder's *lossless* auto-reduce | P10 | — | CLI `gamut convert → .png`; umbrella `png` feature; final API review | ✅ done | | E1 | #224 | **Efficiency:** `deconstruct` byte accounting; divan size/bpp + per-stage bench; libpng-9 size contract; opt-in transparent cleanup; palette-vs-native race; `crc32fast` and restructured filter kernels (see [Efficiency](#efficiency-issue-224)) | ✅ done | | C1 | C2PA 2.4 §A.3.2, §18.5.4 | **C2PA carriage** (#440): the `caBX` manifest store — raw decode surface (`c2pa`; first CRC-valid chunk before `IDAT` wins, ignored ones counted, under the metadata budget); `with_c2pa` / `with_c2pa_reserved` as the last chunk before `IDAT`; the whole-chunk exclusion span from `encode_with_report` and `PngReport::c2pa`, filled in place by `fill_c2pa` (see [C2PA](#c2pa-manifest-store-issue-440)) | ✅ done | -| M1 | §4.3, §11.3.2.6, §11.3.3 | **Metadata preservation** (#483): `with_metadata` / `with_metadata_from` carry a read file's eXIf/iCCP/sRGB/cICP/gAMA/cHRM/XMP/text chunks into a re-encode, each annotation back into the chunk it came from (`gamut convert` uses it; `--strip-metadata` opts out; what cannot be carried is named by `dropped_metadata`); `with_cicp`; §11.3.3.1's keyword rules and §11.3.3.2/§11.3.3.4's null prohibition enforced, with promotion to `iTXt` for text outside Latin-1 (see [Metadata preservation](#metadata-preservation-issue-483)) | ✅ done | +| M1 | §4.3, §11.3.2.6, §11.3.3 | **Metadata preservation** (#483): `with_metadata` / `with_metadata_from` carry a read file's eXIf/iCCP/sRGB/cICP/gAMA/cHRM/XMP/text chunks into a re-encode, each annotation back into the chunk it came from and the XMP packet back into the framing its `iTXt` gave it (`gamut convert` uses it; `--strip-metadata` opts out; what could not be carried faithfully is named by `metadata_notices`); `with_cicp`; §11.3.3.2/§11.3.3.4's null prohibition refuses the encode and §11.3.3.1's advisory keyword rules report through the notice channel, with promotion to `iTXt` for text outside Latin-1 (see [Metadata preservation](#metadata-preservation-issue-483)) | ✅ done | ## Decoder phases (issue #249) @@ -156,7 +156,23 @@ an annotation and whether its text was compressed, and a carry puts it back in t Without it a `zTXt` is indistinguishable from a `tEXt` once decoded, and a compressed 40-byte payload comes back out as 1 600 uncompressed bytes — no words lost, but not preservation either. -**Two payloads cannot be carried, and neither is dropped in silence.** `dropped_metadata()` names +The **XMP packet leaves the read side through its own field**, not through `texts`, so the framing +that field does not hold travels beside it in `XmpFraming`: §11.3.3.4's compression flag, language +tag and translated keyword. §11.3.3.1 Table 21 recommends the null framing for XMP compliance +("with Compression Flag set to 0, and both Language Tag and Translated Keyword set to the null +string") — recommends, not requires, and a provenance packet is exactly the payload a writer +compresses. The measured cost of getting this wrong, on the fixture in `tests/preservation.rs`: a +354-byte `iTXt` rewritten as 3 734 bytes, a factor of 10.6, with the language tag and translated +keyword gone as well. `with_xmp` — which has no source file to take framing from — takes Table 21's +recommended framing. The packet is a **single-value payload** like `iCCP` or `eXIf`: setting it +again replaces it, because a second `iTXt` under the reserved keyword is one this crate's own +reader discards. + +Consolidating the packet into `texts` would retire `XmpFraming` and put the packet back in its +file position rather than first among the annotations; it reshapes a public type, so it is +[#600](https://github.com/visualcommons/gamut/issues/600), not this work. + +**Two payloads cannot be carried, and neither is dropped in silence.** `metadata_notices()` names them and `gamut convert` prints them: - a `cICP` whose matrix coefficients are not 0 — §11.3.2.6 requires 0 for PNG, so the source chunk @@ -172,29 +188,43 @@ libpng reads a file carrying both and returns the same pixels (`tests/oracle.rs` written: dropping either would throw away colour information the source carried, and a reader takes the one it can use. -**The text clauses are enforced, because breaking them corrupts rather than merely offends.** -§11.3.3.1 and §11.3.3.2/§11.3.3.4 are different clauses with different repertoires, and both are -implemented as written: +**Only the null byte refuses the encode. Everything else §11.3.3 asks for is a notice.** +§15 gives the BCP 14 keywords force "when, and only when, they appear in all capitals", and every +statement §11.3.3.1 makes about a keyword's shape is lowercase — "Keywords shall contain only +printable Latin-1", "leading spaces, trailing spaces, and consecutive spaces are not permitted", +"Keywords are restricted to 1 to 79 bytes in length". The same argument that lets `sRGB` and +`iCCP` be carried together applies here, so what separates the outcomes is the *consequence*, not +the wording: -| Field | Repertoire | Clause | +| Field | Clause | Outcome | | --- | --- | --- | -| Keyword (all three chunks) | code points `0x20`–`0x7E` and `0xA1`–`0xFF`; 1–79 bytes; no leading, trailing or consecutive space; expressly not U+00A0 | §11.3.3.1 | -| `tEXt`/`zTXt` text string | the keyword repertoire plus U+000A LINE FEED | §11.3.3.1 closing ¶, §11.3.3.2 | -| `iTXt` text and translated keyword | UTF-8, no null byte | §11.3.3.4 | -| `iTXt` language tag | ASCII letters, digits and `-` (BCP 47 subtags) | §11.3.3.4 | - -Text outside the `tEXt`/`zTXt` repertoire is **promoted** to `iTXt`, which is what §11.3.3.2 -directs ("Text containing characters outside the repertoire of ISO/IEC 8859-1 should be encoded -using the iTXt chunk"), keeping the caller's compression via §11.3.3.4's own flag. Because -promotion is lossless — the character survives, only the chunk changes — the tighter of §11.3.3.1's -and §11.3.3.2's two readings of "Latin-1" is taken, so a control character promotes rather than -being written with no defined meaning. - -Anything **no** chunk can carry refuses the encode with `InvalidInput`, naming the annotation's -index and keyword: a null anywhere in a keyword or text string (it is the field separator, so the -chunk re-parses as a *different* annotation), a keyword outside §11.3.3.1, an XMP packet that is -not UTF-8. A refusal is not a policy choice here — the alternative is a file that reads back as -something else, or a payload that vanishes with nothing said. +| A null in a keyword or text string | §11.3.3.2, §11.3.3.4 | **refuses the encode** — the null is the field separator, so the chunk re-parses as a *different* annotation | +| Keyword outside Latin-1, or outside 1–79 bytes | §11.3.3.1 | annotation **dropped**, `TextKeywordNotLatin1` / `TextKeywordLength` — no chunk can hold it, and this crate's own reader drops one that tries | +| Keyword outside `0x20`–`0x7E` / `0xA1`–`0xFF`, or with a leading, trailing or consecutive space | §11.3.3.1 | **written verbatim**, `TextKeywordRepertoire` / `TextKeywordSpacing` | +| `iTXt` language tag outside ASCII letters, digits and `-` | §11.3.3.4 | tag **dropped**, annotation written, `ItxtLanguageTag` | +| XMP packet that is not UTF-8 | §11.3.3.4 | packet **dropped**, `XmpNotUtf8` | + +The written-verbatim row is the important one, and it is where an earlier draft of this work got +it wrong. Five keyword shapes — a leading space, a trailing space, consecutive spaces, a C0/C1 +control, U+00A0 — are ones this crate's *reader* accepts and returns unchanged. Refusing to write +them back made a re-encode fail on a file whose pixels are fine, and the only escape was +`--strip-metadata`, which discards the ICC profile too. A writer must not be stricter than its own +reader about a clause that is advisory in the first place; `MetadataNotice::carried()` tells a +caller which of these reached the output. + +**The specification contradicts itself about a `tEXt` text string, and the more specific clause +wins.** §11.3.3.1's closing paragraph: "There are also tEXt and zTXt chunks, whose content is +restricted to the printable Latin-1 character set plus U+000A LINE FEED (LF)." §11.3.3.2, which +defines `tEXt`: "Text is interpreted according to the Latin-1 character set [ISO_8859-1]. The text +string may contain any Latin-1 character." Both are in `references/png/png-3.html`. §11.3.3.2 is +the more specific and the more permissive, so it is taken: every Latin-1 character is written into +the chunk that already interprets its bytes as Latin-1, and only a character Latin-1 cannot encode +**promotes** to `iTXt` — which is what §11.3.3.2 itself directs ("Text containing characters +outside the repertoire of ISO/IEC 8859-1 should be encoded using the iTXt chunk"), keeping the +caller's compression via §11.3.3.4's own flag. Taking the tighter reading silently changed a +conforming annotation's chunk *type*, which contradicts the identity claim above. The keyword rule +stays as §11.3.3.1 writes it, because that clause is specific to keywords and all three chunks +share it. **Two spec defects** the same issue found, both in the writer, both fixed: @@ -212,11 +242,15 @@ fixes that byte at 0. **Not done.** `pHYs`, `tIME`, `sBIT` and `bKGD` are not part of `PngMetadata`/`DecodedPng`, so they cannot be carried (set them with their own builder methods). The `iTXt` language tag is checked for -its character set, not for full BCP 47 well-formedness (subtag order, registry membership). -`gamut convert` carries metadata only PNG→PNG; mapping a JPEG/WebP/JXL input's metadata into PNG -chunks is a cross-format job of its own. The libpng oracle reads no chunk back and drops warnings, -so preservation is pinned against gamut's own reader plus a decode the oracle accepts — #502, #571 -and #572 are what would make it differential. +its character set, not for full BCP 47 well-formedness (subtag order, registry membership). The XMP +packet rides in its own field beside `XmpFraming` rather than in `texts`, so a carry emits it +**first** among the annotations regardless of where it sat in the source, and the two fields can be +set inconsistently by a caller building a `PngMetadata` by hand — #600. `sPLT` and `hIST` are +surfaced by neither read walk, so they are not carried either. `gamut convert` carries metadata +only PNG→PNG; mapping a JPEG/WebP/JXL input's metadata into PNG chunks is a cross-format job of its +own. The libpng oracle reads no chunk back and drops warnings, so preservation is pinned against +gamut's own reader plus a decode the oracle accepts — #502, #571 and #572 are what would make it +differential. ## Efficiency (issue #224) @@ -302,7 +336,7 @@ byte) plus removing a sixth redundant filter pass per scanline. | 3 | Smallest lawful representation | **partial** — every reduction is implemented (grey, alpha-drop, ≤256 palette, 16→8, sub-byte, and a `tRNS` colour key for grey/truecolour) and the key is worth ~7–9% on a contiguous transparent region, *not* the 25% the raw-byte arithmetic suggests: the alpha plane it removes is usually the most compressible plane in the image. What is not done is the **selection**. `reduce::analyze8` still resolves *some* candidates on the raw estimate alone, and a raw estimate cannot see DEFLATE (below). Until the three-candidate race below it resolved all of them, and the eliminated runner-up was often the one that won the finished file: an opaque RGBA image with ≤256 colours kept an alpha channel that was 255 everywhere (349 bytes against 317), and a 16-bit image whose samples are all `k·257` kept all sixteen bits (220 against 172). The estimate now hands the best **chunk-free** candidate over beside the chunk-carrying one and `write_reduced_or_native` measures both, which closes that whole family — the chunk-free gates are mutually exclusive, so at most one such candidate ever exists. The remainder is the *pair* that both carry a chunk: where a palette and a `tRNS` colour key are both lawful, only the raw-smaller one is ever encoded. | | 4 | Palette optimization | **partial** — trailing-opaque `tRNS` trim, plus ordering: transparent entries first (so that trim cuts as far as §11.3.2.1 allows) then by luma. Worth −14.7% on the sprite row against +1.5% on `palette64`. Modified-Zeng ordering and caller-supplied palette cleanup remain. [#482] | | 5 | Cleaning invisible data | **done** — `with_transparent_cleanup`, opt-in, on every alpha-carrying layout at 8 and 16 bits. It is the crate's **one lossy knob**: it rewrites stored samples no decoder renders, where every other reduction here is byte-exact, which is why it is off by default and separate from `with_auto_reduce`. Worth **40.1%** on the sprite row, and it is what makes a colour key reachable at all on a source whose invisible pixels carry different unseen colours. It is a *transform*, not a reduction, so it is **raced** rather than assumed: on `palette64_rgba8` cleaning measured −2.3% at 32×32, **+10.7% at 128×128** and −5.2% at 256×256, because zeroing invisible pixels that carry structure destroys bytes DEFLATE was compressing. `cleaned_or_plain` encodes both and keeps the smaller, so the knob can never cost bytes. A tie keeps the **plain** encoding: cleaning buys its rewritten samples with a size win, and where there is no win there is nothing to buy them with. | -| 6 | Metadata hygiene | **no policy** — the encoder emits exactly what the caller set, and `gamut convert` drops metadata on the PNG path. The one exception is shape, not policy: `bKGD` and `sBIT` are resolved against the header actually written (see [Chunks that follow the race](#the-cost-model-and-why-it-is-a-race)). [#483] | +| 6 | Metadata hygiene | **preserve, never strip** — the encoder emits exactly what the caller set, and `gamut convert` carries a PNG input's metadata into a PNG output unless `--strip-metadata` asks otherwise (see [Metadata preservation](#metadata-preservation-issue-483)). Preserving costs bytes, and that is the trade this axis takes: a smaller file that silently lost a colour profile is not a better one. The one exception is shape, not policy: `bKGD` and `sBIT` are resolved against the header actually written (see [Chunks that follow the race](#the-cost-model-and-why-it-is-a-race)). [#483] | | 7 | Interlacing | **correctly none.** Adam7 costs 5–20%; out of scope by declaration. | | 8 | Effort / speed / determinism | Output is byte-reproducible (no time, no randomness, and the one `HashMap` is never iterated). Three independent knobs, no composed dial. No parallelism. [#484] | | 9 | Correctness / robustness | **covered** — 16-bit, odd dimensions, 1×1, CRC policy, malformed input. | From beda77436b079ebc32b8aadfe5a33744d3122418 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 07:21:09 -0400 Subject: [PATCH 09/14] test(png): pin both readings of an XMP chunk's iTXt framing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `parse_itxt` binds §11.3.3.4's compression flag, language tag and translated keyword and now hands all three to `XmpFraming`. The integration suite pins the framed case; nothing pinned the unframed one, so a parser that reported every packet compressed, or that kept an empty tag as `Some("")`, would have rewritten a chunk conforming to §11.3.3.1 Table 21's recommended framing as something else with no test failing. Both directions are asserted here, inline, because `collect` is not public. --- crates/gamut-png/src/decoded.rs | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/crates/gamut-png/src/decoded.rs b/crates/gamut-png/src/decoded.rs index 220aeb0f..c9df2bc0 100644 --- a/crates/gamut-png/src/decoded.rs +++ b/crates/gamut-png/src/decoded.rs @@ -620,6 +620,38 @@ mod tests { assert!(meta.texts.is_empty()); } + /// The framing §11.3.3.1 Table 21 recommends — "Compression Flag set to 0, and both Language + /// Tag and Translated Keyword set to the null string" — reads back as exactly that, so a + /// re-encode reproduces it rather than inventing one. + /// + /// Kills the framing arm of [`parse_itxt`] read the other way from + /// `xmp_framing_carries_the_compression_flag`: a mutant that reports every packet compressed, + /// or that keeps an empty tag as `Some("")`, would rewrite a Table 21-conforming chunk as + /// something else. + #[test] + fn an_unframed_xmp_packet_reads_back_unframed() { + let itxt = b"XML:com.adobe.xmp\0\0\0\0\0"; + let meta = collect(&[(*b"iTXt", itxt)], 1024); + assert_eq!(meta.xmp_framing, Some(XmpFraming::default())); + } + + /// §11.3.3.4's compression flag, language tag and translated keyword belong to the XMP chunk + /// as much as to any other `iTXt`, and the packet's own field cannot hold them. Losing the + /// flag alone rewrites a compressed packet at many times its size. + /// + /// Kills each field of the `ITxt::Xmp` arm of [`parse_itxt`]. + #[test] + fn xmp_framing_carries_the_compression_flag() { + let mut itxt = b"XML:com.adobe.xmp\0\x01\0en-GB\0Metadata\0".to_vec(); + itxt.extend_from_slice(&deflated(b"")); + let meta = collect(&[(*b"iTXt", &itxt)], 1024); + assert_eq!(meta.xmp.as_deref(), Some(&b""[..])); + let framing = meta.xmp_framing.expect("framed"); + assert!(framing.compressed); + assert_eq!(framing.language.as_deref(), Some("en-GB")); + assert_eq!(framing.translated_keyword.as_deref(), Some("Metadata")); + } + #[test] fn metadata_budget_is_cumulative_and_skips_busting_chunks() { let body = vec![b'a'; 600]; From 64a61f49166064cec3dc9d2af2baffabddca9eb3 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 07:22:10 -0400 Subject: [PATCH 10/14] docs(png): say whose job the colour-chunk ranking is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `with_metadata` carries `cICP`, `iCCP` and `sRGB` together and justifies it with §4.3 Table 1's Color Chunk Priority — but Table 1 ranks the chunks for a *reader*, and which one to honour depends on whether that reader has a colour-management module. gamut-png's own reader surfaces all of them and ranks none, so the justification is a claim about other readers, not about this crate. Resolving a profile against a rendering intent is `gamut-cmm`'s work (epic #323), and this encoder deliberately does not pre-empt it. Refs #483. --- crates/gamut-png/STATUS.md | 6 ++++++ crates/gamut-png/src/encoder.rs | 5 ++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/crates/gamut-png/STATUS.md b/crates/gamut-png/STATUS.md index 3c34b64a..7ffee8a5 100644 --- a/crates/gamut-png/STATUS.md +++ b/crates/gamut-png/STATUS.md @@ -188,6 +188,12 @@ libpng reads a file carrying both and returns the same pixels (`tests/oracle.rs` written: dropping either would throw away colour information the source carried, and a reader takes the one it can use. +That last clause is a claim about **other** readers, not about this crate. Table 1 ranks the chunks +for a reader, and which one to honour depends on whether the reader has a CMM at all — which an +encoder cannot know. gamut-png's own reader surfaces `cICP`, `iCCP`, `sRGB`, `cHRM` and `gAMA` side +by side and ranks none of them; resolving a profile against an intent is `gamut-cmm`'s work +(epic #323), and this encoder deliberately does not pre-empt it. + **Only the null byte refuses the encode. Everything else §11.3.3 asks for is a notice.** §15 gives the BCP 14 keywords force "when, and only when, they appear in all capitals", and every statement §11.3.3.1 makes about a keyword's shape is lowercase — "Keywords shall contain only diff --git a/crates/gamut-png/src/encoder.rs b/crates/gamut-png/src/encoder.rs index 1484d879..18c7041c 100644 --- a/crates/gamut-png/src/encoder.rs +++ b/crates/gamut-png/src/encoder.rs @@ -568,7 +568,10 @@ impl PngEncoder { /// /// Everything the read side surfaces is set, including a `cICP`, an `sRGB` and an `iCCP` /// together — §4.3 Table 1 ranks the colour chunks precisely so a file may carry more than - /// one, and a reader honours the lowest priority number. Each text annotation goes back into + /// one, and a reader honours the lowest priority number. That is a claim about *other* + /// readers: this crate's own reader surfaces all of them and ranks none, because which chunk + /// to honour depends on whether the reader has a colour-management module, which an encoder + /// cannot know. Resolving a profile against an intent belongs to `gamut-cmm`. Each text annotation goes back into /// the chunk it came out of, compressed if it was compressed /// ([`TextChunkKind`](crate::TextChunkKind)); so does the XMP packet, whose own framing — /// compression flag, language tag, translated keyword — rides in From 9243d2e41389e693583830acbeee3786b0677da1 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 07:24:44 -0400 Subject: [PATCH 11/14] docs(png): cite the sections the vendored spec actually numbers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three chunk citations on the read surface named the wrong clause, checked against `references/png/png-3.html`: cICP is §11.3.2.6 (§11.3.2.5 is sRGB), sRGB is §11.3.2.5 (§11.3.2.4 is sBIT), and eXIf is §11.3.4.5 (§11.3.4.4 is sPLT). A reader following one of these lands on a different chunk's clause, which is worse than no citation at all in a crate whose rule is that the specification is the source of truth. The crate also cites tRNS as §11.3.2.1 in six files, where the vendored text numbers it §11.3.1.1 and gives §11.3.2.1 to cHRM. That is outside this change's surface and is filed separately. Refs #483. --- crates/gamut-png/src/decoded.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/gamut-png/src/decoded.rs b/crates/gamut-png/src/decoded.rs index c9df2bc0..5b93a061 100644 --- a/crates/gamut-png/src/decoded.rs +++ b/crates/gamut-png/src/decoded.rs @@ -89,7 +89,7 @@ pub struct Chromaticities { pub blue: (u32, u32), } -/// Coding-independent code points (cICP, §11.3.2.5) identifying the video-signal colour space. +/// Coding-independent code points (cICP, §11.3.2.6) identifying the video-signal colour space. #[derive(Debug, Clone, Copy, PartialEq, Eq)] #[non_exhaustive] pub struct Cicp { @@ -188,7 +188,7 @@ pub struct DecodedPng { pub palette: Option, /// The tRNS colour key of a greyscale/truecolour image, in native (unscaled) sample units. pub transparency: Option, - /// The eXIf payload verbatim: a TIFF stream starting with `II`/`MM` (§11.3.4.4). Feed as + /// The eXIf payload verbatim: a TIFF stream starting with `II`/`MM` (§11.3.4.5). Feed as /// `gamut_metadata::MetadataBlock::Exif`. pub exif: Option>, /// The embedded ICC profile (iCCP), inflated. Feed as `MetadataBlock::Icc`. @@ -224,7 +224,7 @@ pub struct DecodedPng { pub gamma: Option, /// cHRM chromaticities, each coordinate × 100 000. pub chromaticities: Option, - /// sRGB rendering intent (§11.3.2.4). + /// sRGB rendering intent (§11.3.2.5). pub srgb: Option, /// cICP video-signal code points. pub cicp: Option, @@ -267,7 +267,7 @@ pub struct DecodedPng { #[derive(Debug, Clone, Default, PartialEq, Eq)] #[non_exhaustive] pub struct PngMetadata { - /// The eXIf payload verbatim: a TIFF stream starting with `II`/`MM` (§11.3.4.4). Feed as + /// The eXIf payload verbatim: a TIFF stream starting with `II`/`MM` (§11.3.4.5). Feed as /// `gamut_metadata::MetadataBlock::Exif`. pub exif: Option>, /// The embedded ICC profile (iCCP), inflated. Feed as `MetadataBlock::Icc`. @@ -303,7 +303,7 @@ pub struct PngMetadata { pub gamma: Option, /// cHRM chromaticities, each coordinate × 100 000. pub chromaticities: Option, - /// sRGB rendering intent (§11.3.2.4). + /// sRGB rendering intent (§11.3.2.5). pub srgb: Option, /// cICP video-signal code points. pub cicp: Option, From ad178e05cfe4c0ef4f6f9642513e289a21838de0 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 10:40:40 -0400 Subject: [PATCH 12/14] fix(png): drop a text string's null instead of refusing the encode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A null in a text *string* refused the whole encode, on a file this crate's own reader accepts: `split_keyword` stops at the keyword's null and hands everything after it back as the text, nulls included, so `Comment\0val\0ue` read as the annotation `Comment` = `val\0ue` and then failed to re-encode. Executed on all three chunk types, including a zTXt whose inflated payload holds one. The mechanism the refusal was justified by is denied by the two clauses it cited. §11.3.3.2: "The text string is not null-terminated (the length of the chunk defines the ending)". §11.3.3.4: "The text, unlike other textual data in this chunk, is not null-terminated; its length is derived from the chunk length". The text is last in all three chunks, so a null in it re-frames nothing — unlike a null in the keyword, which is the field a null separator ends and which still refuses. This was the only shape left that made a re-encode fail on a file gamut's own reader accepts, under the rule this work is built on: a writer must not be stricter than its own reader. It is not written verbatim either. libpng truncates such a text at the null, so the chunk would hold one annotation for this crate and a shorter one for libpng. The annotation is dropped and named instead, through the notice channel the rest of §11.3.3's advisory rules already use: `MetadataNotice::TextStringNull`. Separately, `carried()` claimed `true` for an annotation dropped entirely: `itxt_entry` pushes the language-tag notice after `text_entry` has already cleared the emit flag for a keyword no chunk can hold, so a caller was told its annotation came along with a caveat while zero chunks were written. `text_notices` now derives what it reports from the entry's emit flag: an annotation nothing was written for reports only why it was dropped. --- crates/gamut-png/STATUS.md | 18 +++- crates/gamut-png/src/ancillary.rs | 137 ++++++++++++++++++------- crates/gamut-png/src/encoder.rs | 33 ++++-- crates/gamut-png/tests/preservation.rs | 46 +++++++-- 4 files changed, 176 insertions(+), 58 deletions(-) diff --git a/crates/gamut-png/STATUS.md b/crates/gamut-png/STATUS.md index 7ffee8a5..36c116cb 100644 --- a/crates/gamut-png/STATUS.md +++ b/crates/gamut-png/STATUS.md @@ -40,7 +40,7 @@ opts into narrowing. That is distinct from the encoder's *lossless* auto-reduce | P10 | — | CLI `gamut convert → .png`; umbrella `png` feature; final API review | ✅ done | | E1 | #224 | **Efficiency:** `deconstruct` byte accounting; divan size/bpp + per-stage bench; libpng-9 size contract; opt-in transparent cleanup; palette-vs-native race; `crc32fast` and restructured filter kernels (see [Efficiency](#efficiency-issue-224)) | ✅ done | | C1 | C2PA 2.4 §A.3.2, §18.5.4 | **C2PA carriage** (#440): the `caBX` manifest store — raw decode surface (`c2pa`; first CRC-valid chunk before `IDAT` wins, ignored ones counted, under the metadata budget); `with_c2pa` / `with_c2pa_reserved` as the last chunk before `IDAT`; the whole-chunk exclusion span from `encode_with_report` and `PngReport::c2pa`, filled in place by `fill_c2pa` (see [C2PA](#c2pa-manifest-store-issue-440)) | ✅ done | -| M1 | §4.3, §11.3.2.6, §11.3.3 | **Metadata preservation** (#483): `with_metadata` / `with_metadata_from` carry a read file's eXIf/iCCP/sRGB/cICP/gAMA/cHRM/XMP/text chunks into a re-encode, each annotation back into the chunk it came from and the XMP packet back into the framing its `iTXt` gave it (`gamut convert` uses it; `--strip-metadata` opts out; what could not be carried faithfully is named by `metadata_notices`); `with_cicp`; §11.3.3.2/§11.3.3.4's null prohibition refuses the encode and §11.3.3.1's advisory keyword rules report through the notice channel, with promotion to `iTXt` for text outside Latin-1 (see [Metadata preservation](#metadata-preservation-issue-483)) | ✅ done | +| M1 | §4.3, §11.3.2.6, §11.3.3 | **Metadata preservation** (#483): `with_metadata` / `with_metadata_from` carry a read file's eXIf/iCCP/sRGB/cICP/gAMA/cHRM/XMP/text chunks into a re-encode, each annotation back into the chunk it came from and the XMP packet back into the framing its `iTXt` gave it (`gamut convert` uses it; `--strip-metadata` opts out; what could not be carried faithfully is named by `metadata_notices`); `with_cicp`; a null in a keyword refuses the encode, a null in the length-delimited text string drops the annotation, and §11.3.3.1's advisory keyword rules report through the notice channel, with promotion to `iTXt` for text outside Latin-1 (see [Metadata preservation](#metadata-preservation-issue-483)) | ✅ done | ## Decoder phases (issue #249) @@ -194,7 +194,7 @@ encoder cannot know. gamut-png's own reader surfaces `cICP`, `iCCP`, `sRGB`, `cH by side and ranks none of them; resolving a profile against an intent is `gamut-cmm`'s work (epic #323), and this encoder deliberately does not pre-empt it. -**Only the null byte refuses the encode. Everything else §11.3.3 asks for is a notice.** +**Only a null in a keyword refuses the encode. Everything else §11.3.3 asks for is a notice.** §15 gives the BCP 14 keywords force "when, and only when, they appear in all capitals", and every statement §11.3.3.1 makes about a keyword's shape is lowercase — "Keywords shall contain only printable Latin-1", "leading spaces, trailing spaces, and consecutive spaces are not permitted", @@ -204,7 +204,8 @@ the wording: | Field | Clause | Outcome | | --- | --- | --- | -| A null in a keyword or text string | §11.3.3.2, §11.3.3.4 | **refuses the encode** — the null is the field separator, so the chunk re-parses as a *different* annotation | +| A null in a keyword, or in an `iTXt` translated keyword | §11.3.3.2, §11.3.3.4 | **refuses the encode** — those fields end at their first null, so the chunk re-parses as a *different* annotation | +| A null in a text string | §11.3.3.2, §11.3.3.4 | annotation **dropped**, `TextStringNull` — the text is last and "not null-terminated (the length of the chunk defines the ending)", so it re-frames nothing and this crate's reader hands it back whole; but libpng truncates it at the null, so writing it would put a chunk two readers read differently into a file this encoder signed off on | | Keyword outside Latin-1, or outside 1–79 bytes | §11.3.3.1 | annotation **dropped**, `TextKeywordNotLatin1` / `TextKeywordLength` — no chunk can hold it, and this crate's own reader drops one that tries | | Keyword outside `0x20`–`0x7E` / `0xA1`–`0xFF`, or with a leading, trailing or consecutive space | §11.3.3.1 | **written verbatim**, `TextKeywordRepertoire` / `TextKeywordSpacing` | | `iTXt` language tag outside ASCII letters, digits and `-` | §11.3.3.4 | tag **dropped**, annotation written, `ItxtLanguageTag` | @@ -216,7 +217,16 @@ control, U+00A0 — are ones this crate's *reader* accepts and returns unchanged them back made a re-encode fail on a file whose pixels are fine, and the only escape was `--strip-metadata`, which discards the ICC profile too. A writer must not be stricter than its own reader about a clause that is advisory in the first place; `MetadataNotice::carried()` tells a -caller which of these reached the output. +caller which of these reached the output. A notice that says "written, but…" is suppressed for an +annotation nothing was written for, so `carried()` never claims a payload came along when the +entry carrying the deviation was dropped for another reason. + +The same rule settles the text string's null. §11.3.3.2 and §11.3.3.4 forbid it in words, but +neither field is *framed* by it, and this crate's reader returns such a text whole — so a refusal +would again be a writer stricter than its own reader, on a file the reader accepted. What stops it +being written verbatim is not the clause but the disagreement: libpng truncates the text at the +null, so the chunk would hold one annotation for this crate and a shorter one for libpng. Dropped +and named is the only outcome that is the same everywhere. **The specification contradicts itself about a `tEXt` text string, and the more specific clause wins.** §11.3.3.1's closing paragraph: "There are also tEXt and zTXt chunks, whose content is diff --git a/crates/gamut-png/src/ancillary.rs b/crates/gamut-png/src/ancillary.rs index da7883ef..c5e88e31 100644 --- a/crates/gamut-png/src/ancillary.rs +++ b/crates/gamut-png/src/ancillary.rs @@ -161,8 +161,9 @@ struct TextEntry { /// can hold, or one written verbatim that deviates from a recommendation. Surfaced by /// [`PngEncoder::metadata_notices`](crate::PngEncoder::metadata_notices). notices: Vec, - /// Why this annotation must not be written *at all*, if it must not — the null byte, and - /// only the null byte. Recorded here rather than returned from the setter because the + /// Why this annotation must not be written *at all*, if it must not — a null in the keyword + /// or in the `iTXt` translated keyword, the two fields a null separator ends, and nothing + /// else. Recorded here rather than returned from the setter because the /// setters sit behind `#[must_use]` builder methods that have no error channel; /// [`Ancillary::validate`] reports it at the encode chokepoint. fault: Option, @@ -178,13 +179,18 @@ struct TextFault { reason: &'static str, } -/// §11.3.3.2 for `tEXt`/`zTXt` ("Neither the keyword nor the text string may contain a null -/// character") and §11.3.3.4 for `iTXt` ("neither shall contain a zero byte"). The null is the -/// field separator, so an embedded one does not merely offend the grammar — the chunk re-parses -/// as a *different* annotation. It is the one thing here that makes a file **mean** something -/// else, and so the one thing that refuses the encode. -const TEXT_NUL: &str = - "a keyword or text string may not contain a null character (§11.3.3.2, §11.3.3.4)"; +/// §11.3.3.2 and §11.3.3.4 lay all three text chunks out as "Keyword … Null separator … ", so +/// the keyword is the field that *ends* at its first zero byte. An embedded null there does not +/// merely offend the grammar — the chunk re-parses as a *different* annotation, `Auth\0or` +/// becoming the keyword `Auth` with `or` for its text. It is the one thing here that makes a +/// file **mean** something else, and so the one thing that refuses the encode. +/// +/// The *text string* is the opposite case and is **not** covered by this: §11.3.3.2 says of it +/// "The text string is not null-terminated (the length of the chunk defines the ending)", and +/// §11.3.3.4 "The text, unlike other textual data in this chunk, is not null-terminated; its +/// length is derived from the chunk length". A null there re-frames nothing, so it is reported +/// as [`MetadataNotice::TextStringNull`] instead. +const KEYWORD_NUL: &str = "a keyword may not contain a null character (§11.3.3.2, §11.3.3.4)"; /// §11.3.3.4: "The translated keyword and text both use the UTF-8 encoding, and neither shall /// contain a zero byte (null character)." Null-terminated like the language tag, so an embedded /// one re-frames every field after it. @@ -435,10 +441,21 @@ impl Ancillary { } /// Every §11.3.3 deviation the accumulated annotations carry, in insertion order. + /// + /// An entry that is **not emitted** reports only the notices that explain the drop. + /// [`MetadataNotice::carried`](crate::MetadataNotice::carried) is fixed by the variant, so + /// the entry's [`emit`](TextEntry::emit) flag is the authority on whether anything reached + /// the output: one entry can record both a keyword no chunk can hold and a language tag that + /// did not survive, and surfacing the second unfiltered would tell a caller its annotation + /// came along with a caveat when no chunk was written at all. pub(crate) fn text_notices(&self) -> impl Iterator + '_ { - self.texts - .iter() - .flat_map(|entry| entry.notices.iter().copied()) + self.texts.iter().flat_map(|entry| { + entry + .notices + .iter() + .copied() + .filter(move |notice| entry.emit || !notice.carried()) + }) } /// Starts carrying a read file's metadata, discarding whatever a previous carry contributed. @@ -472,18 +489,28 @@ impl Ancillary { /// promoted rather than written as bytes a Latin-1 reader mis-renders. The promotion keeps /// the caller's *other* choice, compression, because §11.3.3.4 gives `iTXt` a flag of its own. /// - /// A null anywhere in the keyword or the text is the one thing neither promotion nor a - /// notice can fix — §11.3.3.2 and §11.3.3.4 both forbid it, and it is the field separator, so - /// the chunk would re-parse as a different annotation. It becomes a [`TextFault`] the entry - /// carries to [`Self::validate`]. Every *other* way a keyword can fall short of §11.3.3.1 is - /// a [`MetadataNotice`] instead: see [`Keyword`] for why the line is drawn there. + /// A null is forbidden in both fields (§11.3.3.2, §11.3.3.4), and the two are not the same + /// kind of forbidden. The **keyword** ends at its first null, so a null there makes the chunk + /// re-parse as a *different* annotation and nothing can undo it: it becomes a [`TextFault`] + /// the entry carries to [`Self::validate`], and the encode refuses. The **text string** is + /// last and length-delimited — see [`KEYWORD_NUL`] for both clauses — so a null there + /// re-frames nothing, and this crate's own reader hands such a text back intact; refusing it + /// would make the writer stricter than its own reader over a file the reader accepts. It is + /// not written either, because readers disagree about what the chunk then holds (libpng + /// truncates the text at the null), so the annotation is dropped and reported as + /// [`MetadataNotice::TextStringNull`]. Every *other* way a keyword can fall short of + /// §11.3.3.1 is a [`MetadataNotice`] too: see [`Keyword`] for where that line falls. fn text_entry(&self, keyword: &str, text: &str, kind: TextKind) -> TextEntry { - let (keyword_bytes, emit, notice, keyword_nul) = match keyword_verdict(keyword) { + let (keyword_bytes, mut emit, notice, keyword_nul) = match keyword_verdict(keyword) { Keyword::Write(bytes, notice) => (bytes, true, notice, false), Keyword::Drop(notice) => (Vec::new(), false, Some(notice), false), Keyword::Refuse => (Vec::new(), true, None, true), }; - let refused = keyword_nul || text.contains('\0'); + let mut notices: Vec = notice.into_iter().collect(); + if text.contains('\0') { + emit = false; + notices.push(MetadataNotice::TextStringNull); + } // An iTXt was asked for as UTF-8 and stays UTF-8; only a Latin-1 request has a // repertoire to leave. let latin1 = match kind { @@ -503,21 +530,22 @@ impl Ancillary { carried: self.carrying, xmp: false, emit, - notices: notice.into_iter().collect(), - fault: refused.then(|| TextFault { + notices, + fault: keyword_nul.then(|| TextFault { keyword: keyword.to_string(), - reason: TEXT_NUL, + reason: KEYWORD_NUL, }), } } /// Refuses an accumulation the spec forbids, before any byte is emitted. /// - /// **Only a null byte gets here.** A null in a keyword, a text string or an `iTXt` - /// translated keyword (§11.3.3.2, §11.3.3.4) is the field separator, so a chunk carrying one - /// re-parses as a *different* annotation: the file would mean something other than what the - /// caller supplied, and no notice can undo that. Everything else §11.3.3 asks of a text - /// chunk — the keyword's repertoire, length and spacing, the `iTXt` language tag's shape, an + /// **Only a null in a keyword gets here.** A null in a text chunk's keyword or in an `iTXt` + /// translated keyword (§11.3.3.2, §11.3.3.4) sits in a field a null separator *ends*, so a + /// chunk carrying one re-parses as a *different* annotation: the file would mean something + /// other than what the caller supplied, and no notice can undo that. Everything else §11.3.3 + /// asks of a text chunk — the keyword's repertoire, length and spacing, a null in the + /// length-delimited text string, the `iTXt` language tag's shape, an /// XMP packet that is not UTF-8 — is reported through /// [`PngEncoder::metadata_notices`](crate::PngEncoder::metadata_notices) and the encode /// proceeds. Refusing those would fail a conversion over a file whose pixels are fine, and @@ -1459,22 +1487,55 @@ mod tests { assert!(refusal(&a).contains("may not contain a null character")); } - /// §11.3.3.2: "Neither the keyword nor the text string may contain a null character", and - /// §11.3.3.4 the same for `iTXt`. This is corruption, not pedantry: the null is the field - /// separator, so `note\0Author\0other` written as a `tEXt` body re-parses as a *different* - /// annotation. Promotion cannot rescue it, because `iTXt` forbids it too. + /// §11.3.3.2 forbids a null in the text string ("Neither the keyword nor the text string may + /// contain a null character") and §11.3.3.4 says the same for `iTXt` — but neither field is + /// *framed* by a null: "The text string is not null-terminated (the length of the chunk + /// defines the ending)". So the annotation is **dropped and reported**, not refused: this + /// crate's own reader hands such a text back whole, and a writer must not fail on a file its + /// own reader accepts. It is not written either, because libpng truncates such a text at the + /// null, so the chunk would mean different things to different readers. /// - /// Kills the null guard in [`Ancillary::text_entry`], in both the Latin-1 and the UTF-8 - /// request — a mutant that checks only one leaves the other writing the corrupt chunk. + /// Kills the text-null guard in [`Ancillary::text_entry`], in both the Latin-1 and the UTF-8 + /// request — a mutant that checks only one leaves the other writing the disputed chunk — and + /// a mutant that turns the drop back into a refusal. #[test] - fn a_null_in_a_text_string_is_refused() { + fn a_null_in_a_text_string_is_dropped_with_a_notice() { let mut latin1 = Ancillary::default(); latin1.add_text_latin1("Note", "before\0after"); - assert!(refusal(&latin1).contains("may not contain a null character")); + assert_eq!(notices(&latin1), [MetadataNotice::TextStringNull]); + assert!(latin1.validate().is_ok(), "reported, not refused"); + assert_eq!(find_chunk(&post_plte(&latin1), b"tEXt"), None); let mut utf8 = Ancillary::default(); utf8.add_text_international("Note", "before\0after"); - assert!(refusal(&utf8).contains("may not contain a null character")); + assert_eq!(notices(&utf8), [MetadataNotice::TextStringNull]); + assert!(utf8.validate().is_ok(), "reported, not refused"); + assert_eq!(find_chunk(&post_plte(&utf8), b"iTXt"), None); + } + + /// A notice that says the annotation *was written* has no business being reported for one no + /// chunk carries. [`MetadataNotice::carried`] is fixed by the variant, so the entry's `emit` + /// flag is what decides: an `iTXt` whose keyword no chunk can hold records the language tag's + /// deviation on the same entry, and reporting that unfiltered tells a caller its annotation + /// came along with a caveat when zero chunks were written. + /// + /// Kills the `emit` filter in [`Ancillary::text_notices`]; the second half pins that the + /// filter does not swallow the same notice when the annotation *is* written. + #[test] + fn a_dropped_annotation_reports_only_why_it_was_dropped() { + let mut dropped = Ancillary::default(); + dropped.add_text_international_tagged("\u{153}kw", "zh_Hans", "", "body", false); + assert_eq!( + notices(&dropped), + [MetadataNotice::TextKeywordNotLatin1], + "the language tag of an annotation nobody wrote is not news" + ); + assert_eq!(find_chunk(&post_plte(&dropped), b"iTXt"), None); + + let mut written = Ancillary::default(); + written.add_text_international_tagged("kw", "zh_Hans", "", "body", false); + assert_eq!(notices(&written), [MetadataNotice::ItxtLanguageTag]); + assert!(find_chunk(&post_plte(&written), b"iTXt").is_some()); } /// §11.3.3.4: "The translated keyword and text both use the UTF-8 encoding, and neither shall @@ -1580,10 +1641,10 @@ mod tests { fn the_refusal_names_the_annotation_and_its_keyword() { let mut a = Ancillary::default(); a.add_text_latin1("Title", "fine"); - a.add_text_latin1("Author", "bad\0body"); + a.add_text_latin1("Auth\0or", "body"); let message = refusal(&a); assert!(message.contains("text annotation 1"), "{message}"); - assert!(message.contains(r#""Author""#), "{message}"); + assert!(message.contains(r#""Auth\0or""#), "{message}"); } /// §11.3.3.4's language tag and translated keyword survive, so carrying a decoded `iTXt` diff --git a/crates/gamut-png/src/encoder.rs b/crates/gamut-png/src/encoder.rs index 18c7041c..70a91b1b 100644 --- a/crates/gamut-png/src/encoder.rs +++ b/crates/gamut-png/src/encoder.rs @@ -109,9 +109,9 @@ struct MetadataView<'a> { /// left behind from one that reached the output with a caveat on it. /// /// This is deliberately **not** an error channel. The only thing that stops an encode is a null -/// byte in a text field, which makes the chunk re-parse as a different annotation; everything -/// here is something a caller has to *know*, not something that should fail a conversion whose -/// pixels are fine. +/// byte in a *keyword* — a field a null separator ends, so the chunk would re-parse as a +/// different annotation; everything here is something a caller has to *know*, not something that +/// should fail a conversion whose pixels are fine. /// /// `#[repr(u8)]` with explicit discriminants, which are permanent and append-only: the value /// crosses the C ABI as a plain integer. @@ -156,6 +156,17 @@ pub enum MetadataNotice { /// An XMP packet left behind because it is not UTF-8. §11.3.3.4 gives the `iTXt` text field /// UTF-8 and no alternative, so there is no chunk to frame it in. XmpNotUtf8 = 7, + /// A text annotation left behind because its **text string** holds a null character, which + /// §11.3.3.2 ("Neither the keyword nor the text string may contain a null character") and + /// §11.3.3.4 ("neither shall contain a zero byte") both forbid. + /// + /// Unlike a null in a keyword this re-frames nothing — the text is last and "not + /// null-terminated (the length of the chunk defines the ending)" — so it does not fail the + /// encode. It is not written either: readers disagree about what such a chunk holds, libpng + /// truncating the text at the null where this crate's reader returns it whole, so the + /// annotation is dropped rather than written into a file whose meaning depends on who reads + /// it. + TextStringNull = 8, } impl MetadataNotice { @@ -193,6 +204,10 @@ impl MetadataNotice { Self::XmpNotUtf8 => { "XMP packet: not UTF-8, and an iTXt text string must be (§11.3.3.4)" } + Self::TextStringNull => { + "text annotation: its text string contains a null character, which no text chunk \ + may hold (§11.3.3.2, §11.3.3.4)" + } } } @@ -585,12 +600,12 @@ impl PngEncoder { /// /// A text annotation whose keyword or XMP packet §11.3.3 does not endorse is reported through /// the same channel rather than failing the carry: a keyword outside §11.3.3.1's repertoire - /// or spacing rules is written as it arrived, a keyword no chunk can hold and an XMP packet - /// that is not UTF-8 are left behind, and - /// [`MetadataNotice::carried`](MetadataNotice::carried) says which happened. **Only a null** - /// in a keyword or text string fails the encode with [`Error::InvalidInput`] naming the - /// annotation — the null is the field separator, so the chunk would be read back as a - /// *different* annotation, which no notice can undo. + /// or spacing rules is written as it arrived, while a keyword no chunk can hold, a text + /// string holding a null and an XMP packet that is not UTF-8 are left behind, and + /// [`MetadataNotice::carried`](MetadataNotice::carried) says which happened. **Only a null in + /// a keyword** — or in an `iTXt` translated keyword — fails the encode with + /// [`Error::InvalidInput`] naming the annotation: those fields end at their first null, so + /// the chunk would be read back as a *different* annotation, which no notice can undo. /// /// One further limit is the read side's, not this method's: `pHYs`, `tIME`, `sBIT` and `bKGD` /// are not part of [`PngMetadata`], so they cannot be carried here (set them with their own diff --git a/crates/gamut-png/tests/preservation.rs b/crates/gamut-png/tests/preservation.rs index 1592df7b..5f290404 100644 --- a/crates/gamut-png/tests/preservation.rs +++ b/crates/gamut-png/tests/preservation.rs @@ -468,17 +468,48 @@ fn a_non_utf8_xmp_packet_is_reported_and_the_re_encode_proceeds() { assert!(re_encoded(|_| encoder.clone()).xmp.is_none()); } -/// A null byte is the one thing that still refuses, because it is the field separator: a `tEXt` -/// carrying `Note\0Author\0other` re-parses as a *different* annotation, so writing it would make -/// the file mean something the caller never supplied. No notice can undo that. +/// A null in a text *string* is a shape this crate's own reader hands back: §11.3.3.2 makes the +/// text last and length-delimited ("The text string is not null-terminated (the length of the +/// chunk defines the ending)"), so the reader stops at the keyword's null and everything after +/// it — later nulls included — is the text. Refusing to write it back would fail a re-encode on +/// a file this crate decoded without complaint, which is the failure the notice channel exists to +/// end. It is not written either: libpng truncates such a text at the null, so the chunk would +/// hold different annotations for different readers. Dropped, and named. #[test] -fn a_null_in_a_carried_text_string_refuses_the_re_encode() { - // Built through the setter rather than a fixture: the reader splits a chunk at its first - // null, so no file can hand a null to the carry — only a caller can. +fn a_null_in_a_carried_text_string_is_dropped_with_a_notice() { + let png = minimal_source(&[chunk(b"tEXt", b"Comment\0val\0ue")]); + let meta = gamut_png::metadata(&png).unwrap(); + assert_eq!( + meta.texts.first().map(|t| t.text.as_str()), + Some("val\0ue"), + "the reader hands the null back" + ); + + let encoder = PngEncoder::new().with_metadata(&meta); + assert_eq!( + encoder.metadata_notices(), + [MetadataNotice::TextStringNull], + "named, not refused" + ); + assert!( + re_encoded(|_| encoder.clone()).texts.is_empty(), + "and not written" + ); +} + +/// The null that still refuses is the one in a *keyword*: all three chunks are framed "Keyword … +/// Null separator …", so `Auth\0or` re-parses as the annotation `Auth` with `or` for its text and +/// the file means something the caller never supplied. +/// +/// Built through the setter, which is the only way in — the reader splits a chunk at its first +/// null and never returns a keyword holding one. Kills a mutant that drops the accumulated +/// annotations' validation from the encode path. +#[test] +fn a_null_in_a_carried_keyword_refuses_the_re_encode() { let pixels = vec![0u8; 3 * 4]; let image = ImageRef::::new(&pixels, Dimensions::new(2, 2).unwrap()).unwrap(); let error = PngEncoder::new() - .with_text("Note", "before\0after") + .with_text("Auth\0or", "body") .encode_to_vec(image) .expect_err("refused"); assert_eq!(error.kind(), ErrorKind::InvalidInput); @@ -534,6 +565,7 @@ fn a_notice_says_whether_the_payload_reached_the_output() { MetadataNotice::TextKeywordNotLatin1, MetadataNotice::TextKeywordLength, MetadataNotice::XmpNotUtf8, + MetadataNotice::TextStringNull, ] { assert!(!lost.carried(), "{lost:?}"); } From b4fa5adafec6f8f7a38b5d7b9958db28aff5fb9f Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 10:41:14 -0400 Subject: [PATCH 13/14] docs(png): name the conformance clause, and stop contradicting the colour record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `with_cicp` said the fallback colour chunks "stay legal alongside it — unlike the `sRGB`/`iCCP` pair". That is residue of a refusal this branch removed, and it contradicts `with_srgb` two methods above and the record entry that settles the point: §4.3 Table 1 presupposes the pair and ranks it, so both are written and the ranking is a colour-management decision, not this encoder's. `TextKeywordRepertoire` said a keyword outside §11.3.3.1's repertoire is one "another reader may reject". No real reader does — libtiff's and libpng's read paths never validate a keyword — so the statement is both vaguer and less true than the one the specification supports: §15.3.1 requires that "All field values in the PNG datastream obey the relationships specified in this specification", so the datastream is non-conforming. Precision here costs nothing. The three direct text setters now each say where a §11.3.3 deviation is reported and which single case still fails the encode, so a caller reaching `with_text` first does not have to find `with_metadata` to learn it. The XMP measurement paired the source payload's 354 bytes with a factor computed from the 352-byte carried chunk, and attached "both strings gone" to a figure measured with both strings present. Both figures are real; they are now stated as the test measures them — 352 bytes with the compression flag kept against 3 734 with it cleared — with the two lost strings named as the separate loss they are. --- crates/gamut-png/STATUS.md | 12 +++++++----- crates/gamut-png/src/encoder.rs | 24 ++++++++++++++++++++---- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/crates/gamut-png/STATUS.md b/crates/gamut-png/STATUS.md index 36c116cb..ffac0c1a 100644 --- a/crates/gamut-png/STATUS.md +++ b/crates/gamut-png/STATUS.md @@ -161,10 +161,12 @@ that field does not hold travels beside it in `XmpFraming`: §11.3.3.4's compres tag and translated keyword. §11.3.3.1 Table 21 recommends the null framing for XMP compliance ("with Compression Flag set to 0, and both Language Tag and Translated Keyword set to the null string") — recommends, not requires, and a provenance packet is exactly the payload a writer -compresses. The measured cost of getting this wrong, on the fixture in `tests/preservation.rs`: a -354-byte `iTXt` rewritten as 3 734 bytes, a factor of 10.6, with the language tag and translated -keyword gone as well. `with_xmp` — which has no source file to take framing from — takes Table 21's -recommended framing. The packet is a **single-value payload** like `iCCP` or `eXIf`: setting it +compresses. The measured cost of getting this wrong, on the fixture in `tests/preservation.rs`: +the source `iTXt` payload is 354 bytes, the carry that keeps the flag writes 352, and the same +carry with the flag cleared writes **3 734** — a factor of 10.6 against the 352 it should have +been. The language tag and translated keyword were a second, separate loss of the same defect, +pinned by a test of their own. `with_xmp` — which has no source file to take framing from — takes +Table 21's recommended framing. The packet is a **single-value payload** like `iCCP` or `eXIf`: setting it again replaces it, because a second `iTXt` under the reserved keyword is one this crate's own reader discards. @@ -207,7 +209,7 @@ the wording: | A null in a keyword, or in an `iTXt` translated keyword | §11.3.3.2, §11.3.3.4 | **refuses the encode** — those fields end at their first null, so the chunk re-parses as a *different* annotation | | A null in a text string | §11.3.3.2, §11.3.3.4 | annotation **dropped**, `TextStringNull` — the text is last and "not null-terminated (the length of the chunk defines the ending)", so it re-frames nothing and this crate's reader hands it back whole; but libpng truncates it at the null, so writing it would put a chunk two readers read differently into a file this encoder signed off on | | Keyword outside Latin-1, or outside 1–79 bytes | §11.3.3.1 | annotation **dropped**, `TextKeywordNotLatin1` / `TextKeywordLength` — no chunk can hold it, and this crate's own reader drops one that tries | -| Keyword outside `0x20`–`0x7E` / `0xA1`–`0xFF`, or with a leading, trailing or consecutive space | §11.3.3.1 | **written verbatim**, `TextKeywordRepertoire` / `TextKeywordSpacing` | +| Keyword outside `0x20`–`0x7E` / `0xA1`–`0xFF`, or with a leading, trailing or consecutive space | §11.3.3.1 | **written verbatim**, `TextKeywordRepertoire` / `TextKeywordSpacing` — the datastream is then non-conforming per §15.3.1 | | `iTXt` language tag outside ASCII letters, digits and `-` | §11.3.3.4 | tag **dropped**, annotation written, `ItxtLanguageTag` | | XMP packet that is not UTF-8 | §11.3.3.4 | packet **dropped**, `XmpNotUtf8` | diff --git a/crates/gamut-png/src/encoder.rs b/crates/gamut-png/src/encoder.rs index 70a91b1b..8d691157 100644 --- a/crates/gamut-png/src/encoder.rs +++ b/crates/gamut-png/src/encoder.rs @@ -141,7 +141,9 @@ pub enum MetadataNotice { /// A text annotation **written**, whose keyword leaves the repertoire §11.3.3.1 recommends /// ("only code points 0x20-7E and 0xA1-FF are allowed", and expressly "nor is U+00A0 /// NON-BREAKING SPACE"). The keyword is written exactly as it arrived — this crate reads it - /// back unchanged — but another reader need not be so forgiving. + /// back unchanged — but the datastream is then non-conforming per §15.3.1, which requires + /// that "All field values in the PNG datastream obey the relationships specified in this + /// specification". TextKeywordRepertoire = 4, /// A text annotation **written**, whose keyword has a leading, trailing or consecutive /// space, which §11.3.3.1 says are "not permitted in keywords" so that one keyword cannot be @@ -191,7 +193,8 @@ impl MetadataNotice { } Self::TextKeywordRepertoire => { "text annotation: written, but its keyword leaves the code points 0x20-0x7E and \ - 0xA1-0xFF §11.3.3.1 recommends — another reader may reject it" + 0xA1-0xFF §11.3.3.1 recommends, so the datastream is non-conforming per \ + §15.3.1" } Self::TextKeywordSpacing => { "text annotation: written, but its keyword has a leading, trailing or \ @@ -394,8 +397,7 @@ impl PngEncoder { /// /// cICP is the **highest-precedence** colour chunk (§4.3 Table 1, priority 1), so a reader /// that understands it ignores any `iCCP`, `sRGB`, `gAMA` and `cHRM` in the same file. Those - /// stay legal alongside it — unlike the `sRGB`/`iCCP` pair — and are worth keeping as a - /// fallback for readers that do not. + /// are worth keeping alongside it as a fallback for readers that do not. #[must_use] pub fn with_cicp( mut self, @@ -515,6 +517,12 @@ impl PngEncoder { } /// Adds an uncompressed Latin-1 text annotation (tEXt chunk). + /// + /// Almost nothing here fails: what §11.3.3 does not endorse — a keyword outside §11.3.3.1's + /// repertoire, length or spacing, a text string holding a null — is reported through + /// [`metadata_notices`](Self::metadata_notices), which also says whether the annotation was + /// written. The one exception is a null in the *keyword*, which fails the encode, because a + /// keyword ends at its first null and the chunk would re-parse as a different annotation. #[must_use] pub fn with_text(mut self, keyword: &str, text: &str) -> Self { self.ancillary.add_text_latin1(keyword, text); @@ -522,6 +530,10 @@ impl PngEncoder { } /// Adds a zlib-compressed Latin-1 text annotation (zTXt chunk). + /// + /// §11.3.3's rules reach this annotation exactly as they reach + /// [`with_text`](Self::with_text): everything but a null in the keyword is reported through + /// [`metadata_notices`](Self::metadata_notices) rather than failing the encode. #[must_use] pub fn with_compressed_text(mut self, keyword: &str, text: &str) -> Self { self.ancillary.add_text_compressed(keyword, text); @@ -529,6 +541,10 @@ impl PngEncoder { } /// Adds an uncompressed UTF-8 text annotation (iTXt chunk). + /// + /// §11.3.3's rules reach this annotation exactly as they reach + /// [`with_text`](Self::with_text): everything but a null in the keyword is reported through + /// [`metadata_notices`](Self::metadata_notices) rather than failing the encode. #[must_use] pub fn with_international_text(mut self, keyword: &str, text: &str) -> Self { self.ancillary.add_text_international(keyword, text); From af5cbdbc49138330a0d1f9434aab5a989c159079 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Thu, 10 Sep 2026 11:27:00 -0400 Subject: [PATCH 14/14] docs(png): let the notice list stay the enum's, and name both payloads a null drops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two inaccuracies this round's own repair introduced. `TextEntry::notices` enumerated what a notice can be — "a keyword no chunk can hold, or one written verbatim that deviates from a recommendation" — and adding `TextStringNull` made that list incomplete. It is not completed by hand: a copy of an enum's variants written out once is exactly what goes stale, so the field now points at `MetadataNotice` as the list and says only what is true of every member of it, plus which flag decides whether this entry reached the output when the variant and the entry disagree. `TextStringNull`'s user-visible line said "text annotation", but the XMP packet is carried as an `iTXt` too and so can reach the same notice; a caller was told the wrong payload had been dropped. The line now names both. The variant's own doc records what that second path means: XML 1.0 does not admit U+0000 in a document, so a packet that reaches this notice is not merely unwritable, it is already not well-formed XML. --- crates/gamut-png/src/ancillary.rs | 11 +++++++++-- crates/gamut-png/src/encoder.rs | 14 +++++++++----- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/crates/gamut-png/src/ancillary.rs b/crates/gamut-png/src/ancillary.rs index c5e88e31..6da64a5f 100644 --- a/crates/gamut-png/src/ancillary.rs +++ b/crates/gamut-png/src/ancillary.rs @@ -157,8 +157,15 @@ struct TextEntry { /// carry its [`notices`](Self::notices) — a payload dropped in silence is the defect this /// module exists to remove. emit: bool, - /// What §11.3.3 says about this annotation that the caller has to hear: a keyword no chunk - /// can hold, or one written verbatim that deviates from a recommendation. Surfaced by + /// What §11.3.3 says about this annotation that the caller has to hear. + /// + /// Which things those are is [`MetadataNotice`]'s own list of variants, deliberately not + /// copied here: a copy written out once is what goes stale when a variant is added. An entry + /// records every notice its fields earn — some meaning the annotation was left behind, some + /// that it was written with a deviation on it — and + /// [`MetadataNotice::carried`](crate::MetadataNotice::carried) is which of the two a variant + /// means. Whether *this* entry reached the output is [`emit`](Self::emit), which + /// [`Ancillary::text_notices`] takes as the authority when the two disagree. Surfaced by /// [`PngEncoder::metadata_notices`](crate::PngEncoder::metadata_notices). notices: Vec, /// Why this annotation must not be written *at all*, if it must not — a null in the keyword diff --git a/crates/gamut-png/src/encoder.rs b/crates/gamut-png/src/encoder.rs index 8d691157..9443f164 100644 --- a/crates/gamut-png/src/encoder.rs +++ b/crates/gamut-png/src/encoder.rs @@ -158,7 +158,7 @@ pub enum MetadataNotice { /// An XMP packet left behind because it is not UTF-8. §11.3.3.4 gives the `iTXt` text field /// UTF-8 and no alternative, so there is no chunk to frame it in. XmpNotUtf8 = 7, - /// A text annotation left behind because its **text string** holds a null character, which + /// A text payload left behind because its **text string** holds a null character, which /// §11.3.3.2 ("Neither the keyword nor the text string may contain a null character") and /// §11.3.3.4 ("neither shall contain a zero byte") both forbid. /// @@ -166,8 +166,12 @@ pub enum MetadataNotice { /// null-terminated (the length of the chunk defines the ending)" — so it does not fail the /// encode. It is not written either: readers disagree about what such a chunk holds, libpng /// truncating the text at the null where this crate's reader returns it whole, so the - /// annotation is dropped rather than written into a file whose meaning depends on who reads - /// it. + /// payload is dropped rather than written into a file whose meaning depends on who reads it. + /// + /// The payload is usually a text annotation, but the XMP packet goes into an `iTXt` too and + /// so can land here. That case is worth reading twice: XML 1.0 does not admit U+0000 in a + /// document at all, so a packet that reaches this notice is not merely unwritable — it is + /// already not well-formed XML, whatever produced it. TextStringNull = 8, } @@ -208,8 +212,8 @@ impl MetadataNotice { "XMP packet: not UTF-8, and an iTXt text string must be (§11.3.3.4)" } Self::TextStringNull => { - "text annotation: its text string contains a null character, which no text chunk \ - may hold (§11.3.3.2, §11.3.3.4)" + "text annotation or XMP packet: its text string contains a null character, \ + which no text chunk may hold (§11.3.3.2, §11.3.3.4)" } } }