feat(png)!: preserve metadata across a re-encode, and name what could not come along - #550
Open
justin13888 wants to merge 14 commits into
Open
justin13888 wants to merge 14 commits into
justin13888 wants to merge 14 commits into
Conversation
… pairs the spec forbids `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.
…ncode 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.
…our pair 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.
`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.
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.
… called 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.
…y advises 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<MetadataNotice>`. `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.
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.
`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.
justin13888
changed the title
`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.
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.
This was referenced Sep 10, 2026
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.
…lour record `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.
…s a null drops 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the metadata gap in
gamut-pngand the two spec defects issue #483 names, establishedagainst the vendored PNG 3rd edition (
references/png/png-3.html) rather than the issue's text.Metadata preservation.
PngEncoder::with_metadata(&PngMetadata)andwith_metadata_from(&DecodedPng)carry a file's eXIf, iCCP, XMP, text annotations and colourchunks into the encoder that rewrites its pixels. Previously nothing connected the read side to
the write side, so every re-encode dropped all of it.
gamut convertnow uses this on the PNGoutput path, with
--strip-metadataas the opt-out. Preserve is the default: a stripped file issmaller, but dropping an ICC profile silently changes what a viewer paints, so the loss is the
thing that has to be asked for.
Identity, not just content.
TextChunk::kindrecords which of §11.3.3's three chunks carriedan annotation and whether its text was compressed, and a carry puts it back in the same one. The
XMP packet leaves the read side through its own field rather than as a
TextChunk, so the framingthat field cannot hold — §11.3.3.4's compression flag, language tag and translated keyword —
travels beside it in
XmpFraming. Without it the fixture's 354-byteiTXtcame back out at3 734 bytes where keeping the flag writes 352 — a factor of 10.6 — and the language tag and
translated keyword were a second, separate loss of the same defect. The packet is a single-value payload: setting it twice
sets it once, because a second
iTXtunder §11.3.3.1 Table 21's reserved keyword is one thiscrate's own reader discards.
What refuses, what is reported, and why the line is there. §15 gives the BCP 14 keywords force
"when, and only when, they appear in all capitals". Every statement §11.3.3.1 makes about a
keyword's shape is lowercase, and five of those 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. So the writer is no longer stricter than its own reader: those keywords are written
verbatim and reported. A keyword no chunk can hold (outside Latin-1, or outside the 1–79 bytes all
three chunks fix), a text string holding a null, a non-ASCII
iTXtlanguage tag and an XMP packetthat is not UTF-8 are dropped and reported. Only a null in a keyword — or in an
iTXttranslated keyword — refuses the encode, because those are the fields a null separator ends, so
the chunk would re-parse as a different annotation. A null in the text string re-frames
nothing (§11.3.3.2: "The text string is not null-terminated (the length of the chunk defines the
ending)") and this crate's own reader hands such a text back whole, so refusing it made the writer
stricter than its own reader; libpng truncates it at the null, so it is not written verbatim
either.
MetadataNotice::carried()tells a caller which of the two happened — derived fromwhether the annotation was emitted, so a "written, but…" notice is never reported for one nothing
was written for;
gamut convertprints both, worded differently.The
sRGB-beside-iCCPrefusal an earlier draft of this branch carried was removed on thesame argument: §5.6 Table 5 and §11.3.2.5 say only that the pair "should not" appear together —
lowercase — while §4.3 Table 1 presupposes it and ranks the chunks (cICP 1, iCCP 2, sRGB 3,
cHRM+gAMA 4). Both are written and a reader takes the one it can use; dropping either would throw
away colour information the source carried. The ranking is a colour-management decision, and it is
left to a CMM rather than pre-empted by the encoder.
Defect 1 —
tEXt/zTXtcarried UTF-8. §11.3.3.1 restricts a keyword to Latin-1 in all threetext chunks; §11.3.3.2 says a
tEXttext string "is interpreted according to the Latin-1character set"; §11.3.3.3 says inflating a
zTXt"yields Latin-1 text that is identical to thetext that would be stored in an equivalent
tEXtchunk".write_textpushed the RustString'sUTF-8 bytes into all of these, so every code point above U+007F was stored as mojibake —
é(U+00E9) became
C3 A9, which a conforming reader rendersé. Text and keyword are nowconverted to Latin-1 once, when the caller sets them, and the entry is stored in the byte form its
chunk carries, so the wrong encoding is unrepresentable. Text that Latin-1 cannot encode at all is
promoted to
iTXtexactly as §11.3.3.2 directs, keeping the caller's compression choice via§11.3.3.4's own flag.
Defect 2 —
iTXtlost its language tag and translated keyword, the two fields that make itinternational, and its compression flag. All three are now carried, for ordinary annotations and
for the XMP packet alike.
A specification contradiction, recorded rather than resolved silently. §11.3.3.1's closing
paragraph says
tEXt/zTXt"content is restricted to the printable Latin-1 character set plusU+000A LINE FEED (LF)". §11.3.3.2, which defines
tEXt, says one sentence after naming the samecharacter set: "The text string may contain any Latin-1 character." Both are quoted in
ancillary.rsand inSTATUS.md. The more specific and more permissive clause is taken, so aconforming annotation is no longer silently promoted to a different chunk type — which would have
contradicted the identity claim above.
with_cicp(§11.3.2.6) is added because without it preservation would silently drop thehighest-precedence colour chunk (§4.3 Table 1, priority 1) from any file that carries one. It
takes no matrix-coefficients argument: §11.3.2.6 states "RGB is currently the only supported color
model in PNG, and as such Matrix Coefficients shall be set to 0."
Validation
Run in the lane's worktree on
fix/483-png-metadata-preservation. Workspace-wide gates ran insidea memory-capped scope (
MemoryMax=16G,CARGO_BUILD_JOBS=2).mise run fmt-checkneeds__CARGO_TEST_ROOT=$(git rev-parse --show-toplevel)in a nested worktree, because the task loopsover every
tooling/*/Cargo.tomland at least one has no[workspace]table; this is anenvironment artefact, not a manifest change.
cargo test -p gamut-png --all-featurescargo clippy -p gamut-png -p gamut-cli --all-targets --all-features -- -D warnings__CARGO_TEST_ROOT=$(git rev-parse --show-toplevel) mise run fmt-checkmise run check-testsmise run check-commitsconvco check origin/feat/440-png-cabx..HEADmise run lint-D warningsmise run testGAMUT_MUTANTS_BASE=origin/feat/440-png-cabx mise run mutants-diffThe compressed-XMP defect, measured on
tests/preservation.rs's fixture: the sourceiTXtpayload is 354 bytes; before this change the carry rewrote it as 3 734 bytes (a factor of
10.6) with the language tag and translated keyword dropped; after it, 352 bytes with both
strings intact.
a_compressed_xmp_packet_goes_back_into_a_compressed_itxtmeasures the pairrather than a fixed threshold, so the claim is the inflation the flag prevents.
No
Cargo.tomlchanged, socheck-release-deps,check-ffi-featuresandcheck-ffi-headerwerenot required.
Round 5 (commits
ad178e05,a7f57fd3)cargo test -p gamut-png --all-featurescargo clippy -p gamut-png -p gamut-cli --all-targets --all-features -- -D warnings__CARGO_TEST_ROOT=$(git rev-parse --show-toplevel) mise run fmtthenmise run fmt-checkmise run check-testsconvco check origin/feat/440-png-cabx..HEADmise run lint-D warnings, in a 16 GiB scopemise run testGAMUT_MUTANTS_BASE=origin/feat/440-png-cabx mise run mutants-difforigin/feat/440-png-cabx(the stacked base, notmaster)Correction to the measurement stated above. The paragraph beginning "The compressed-XMP
defect, measured on
tests/preservation.rs's fixture" reads "the sourceiTXtpayload is 354bytes; before this change the carry rewrote it as 3 734 bytes (a factor of 10.6) with the
language tag and translated keyword dropped; after it, 352 bytes with both strings intact".
All three figures are real and re-measured this round, but the prose pairs them wrongly: 10.6 is
3 734 ÷ 352, not 3 734 ÷ 354 (which is 10.5), and the 3 734-byte figure is measured by the test
with the two strings present — it clears only the compression flag. The compression flag's cost
is 352 → 3 734 bytes; the language tag and translated keyword are a separate loss of the same
defect, pinned by a separate test. Re-measured on
b4fa5ada: source payload 354 bytes, carriedcompressed 352 bytes, same carry with the flag cleared 3 734 bytes.
The null-in-a-text-string defect, executed before and after (
b4fa5ada), on a source PNGbuilt chunk by chunk:
tEXtComment\0val\0ueComment, textval\0ueError::InvalidInput: "a keyword or text string may not contain a null character"TextStringNullreported, encode succeedsiTXtComment\0\0\0\0\0val\0uezTXtComment\0\0+ zlib(val\0ue)Round 5, continued (commit
af5cbdbc). Two inaccuracies this round's own repair introduced,closed in a commit of their own.
cargo test -p gamut-png --all-featurescargo clippy -p gamut-png -p gamut-cli --all-targets --all-features -- -D warningscargo doc -p gamut-png --no-deps --document-private-items --all-featuresBCP47,text_repertoire) are untouched and not from this change__CARGO_TEST_ROOT=$(git rev-parse --show-toplevel) mise run fmtthenmise run fmt-checkmise run check-testsconvco check origin/feat/440-png-cabx..HEADGAMUT_MUTANTS_BASE=origin/feat/440-png-cabx mise run mutants-difforigin/feat/440-png-cabxmise run lintandmise run testwere not re-run for this commit: it changes two doc comments andone user-visible string with no assertion on it, and
cargo clippyover both affected crates plusthe full
gamut-pngsuite and the mutation gate cover exactly what it touches. Stated as asubset, not as the whole.
Correction to the round-5 heading above. It reads "### Round 5 (commits
ad178e05,a7f57fd3)".a7f57fd3was amended before it was ever pushed and does not exist in thisrepository; the commit that heading means is
b4fa5ada, whose content is the same plus onere-wrapped
STATUS.mdline. The heading is left as written; this entry is the pointer.Coverage (test gate) was still
pending~35 minutes afterb4fa5adawas pushed, having beennon-terminal for over two and a half hours at round 4 with six concurrent runs queued. That is a
starved queue, not a failing job: every other required check — Format & Metadata, Clippy &
Doctests, Incremental (0–3) — is terminal and passing. It is recorded here as an observation, and
this round did not wait on it.
The
carried()defect, executed on the same head: a carry whoseTextChunkhas a keywordoutside Latin-1 and a language tag outside BCP 47 reported
[TextKeywordNotLatin1, ItxtLanguageTag]with zeroiTXtchunks written, andItxtLanguageTag.carried()istrue—so a CLI printed "carried with a caveat" for an annotation that was not written. After: only
TextKeywordNotLatin1is reported.Risks and rollout
DroppedMetadatais renamedMetadataNotice(six new variants,permanent append-only discriminants), and
PngEncoder::dropped_metadata() -> &[DroppedMetadata]becomes
metadata_notices() -> Vec<MetadataNotice>.PngMetadataandDecodedPnggain anxmp_framingfield; both are#[non_exhaustive], so only exhaustive construction inside thisworkspace is affected. Neither type had shipped in a release, but the marker stays: over-signalling
a version bump is recoverable, under-signalling is not.
repertoire, length or spacing rules, a non-ASCII
iTXtlanguage tag and a non-UTF-8 XMP packet nolonger return
Error::InvalidInput. A caller that relied on the refusal must readmetadata_notices()instead. The reverse — a caller who could not convert a file at all — is thecase this change exists for.
iTXttranslated keyword. A null in a text string, which earlier commits on this branch also refused,
now drops the annotation and reports
TextStringNullinstead — it was the last shape that made are-encode fail on a file this crate's own reader accepts.
tEXt/zTXtwhere it used to landas UTF-8 mojibake, and text Latin-1 cannot encode lands in
iTXt. Both are the spec's encoding;the previous bytes were unreadable by any conforming reader. A control character or U+00A0 in a
text string now stays in
tEXtrather than promoting, which changes the chunk type such anannotation is written to relative to the previous commit on this branch.
smaller than the previous commit on this branch produced, not larger.
with_metadatanever carries the C2PA manifest store: a store is signed over the exact bytes ofthe file it was made for, which is why
caBXis unsafe-to-copy (C2PA 2.4 §A.3.2). Re-sign theoutput and set it with
with_c2pa.pHYs,tIME,sBITandbKGDare not part ofPngMetadata/DecodedPng, so they cannot bepreserved. The XMP packet is emitted first among the annotations regardless of where its chunk
sat in the source, and its two read-side fields can be set inconsistently by a caller building a
PngMetadataby hand. Both are gamut-png: the XMP packet is an iTXt held outside the text list, so its framing needs a parallel field #600.gamut's own reader plus a decode the oracle accepts. tooling/libpng-oracle: expose bKGD/sBIT and a warning count so ancillary-chunk acceptance is oracle-testable #502, tooling/libpng-oracle: transcode text payloads to Latin-1 so tEXt/zTXt can be oracle-generated #571 and tooling/libpng-oracle: read back text and colour chunks so metadata preservation is oracle-testable #572 are what would make it
differential.
Issue
Refs #483. Two remainders this run declined are filed:
the text list, which would close the parallel-field, single-value and chunk-order seams together
but reshapes a public type on a crate about to be version-bumped;
tRNSas §11.3.2.1,which the vendored spec gives to
cHRM. Found while auditing this change's citations; the sitesare in the encoder, the reducer and two test suites, none of them on this change's surface;
iCCPprofile name has, word forword, the same §11.3.2.3 rules as a text keyword and gets none of this machinery: executed, a
non-Latin-1 name becomes mojibake and an empty, over-long or null-bearing name loses the profile
silently. Pre-existing, unreachable through the carry, and outside this manifest.
Stacked on
Based on
origin/feat/440-png-cabxate1dddb76, the head of PR #511 (open, not merged bythis run). #511 is itself stacked on PR #485 (
feat/224-png-encoder-efficiency). Review this diffagainst that base, not against
master.Decisions taken
No human approved this plan. This was an unattended run; the record below is what a human reads
afterwards. The user's own words declaring the run unattended:
The frozen record this lane was given, verbatim:
Appended by this lane:
Two entry points, not one.
with_metadata(&PngMetadata)alongside the record'swith_metadata_from(&DecodedPng), over one private borrowed view.Rejected: only
&DecodedPng, which forces a full pixel decode on a caller that used thepixel-free
metadata()entry point — which is precisely whatgamut convertdoes, since itdecodes through the shared RGBA path and never holds a
DecodedPng.Rejected: only
&PngMetadataplus a cloning accessor onDecodedPng, which copies everypayload twice.
Reverses: delete one method; the other keeps working.
with_cicpis added rather than the gap documented. The record's policy is "preserveeverything"; without a cICP setter,
with_metadatawould silently drop the highest-prioritycolour chunk (§4.3 Table 1) of any file that has one, which makes the policy untrue for
exactly the files it matters most for. Four bytes, fully specified by §11.3.2.6.
Rejected: document the hole and file it — a preservation feature whose headline gap is the
top-priority colour chunk is not the feature.
Reverses: remove the setter and skip the field in
with_metadata_view.A non-Latin-1 text keyword refuses the encode; it is not dropped and not transliterated.
§11.3.3.1 binds keywords in all three text chunks, so unlike the text — which §11.3.3.2 routes
to
iTXt— there is no chunk that can carry it.Rejected: drop the annotation silently, which is the same silent metadata loss this issue is
about.
Reverses: turn the
validatearm into a skip.The full §11.3.3.1 keyword syntax rules are not enforced — printable subset (0x20–0x7E,
0xA1–0xFF), no leading/trailing/consecutive spaces, no U+00A0, 1–79 bytes. The record scopes
this lane to Latin-1 representability, and enforcing the rest would make preservation fail on
real-world files whose keywords carry a trailing space. Filed as a follow-up.
Reverses: extend the
validatepredicate.The CLI carries metadata only PNG→PNG.
gamut convertreads the input's PNG metadata withgamut_png::metadata, which errors on any non-PNG input and is then skipped. MappingJPEG/WebP/JXL metadata into PNG chunks is a cross-format job with its own decisions and is
outside this lane's manifest (
convert.rsonly). Filed as a follow-up.Reverses: widen the sniff once the mapping exists.
Appended by this lane in round 3, after review:
A compressed XMP packet keeps its compression flag, language tag and translated keyword.
The packet leaves the read side through its own field rather than as a
TextChunk, soparse_itxtbound §11.3.3.4's three framing values and discarded them for that one keyword;the writer, with no chunk-kind to consult, always emitted flag 0 and two empty strings.
Measured on the fixture added with this change: a 354-byte
iTXtwas rewritten as 3 734 bytes,a factor of 10.6.
XmpFramingnow travels beside the packet on both read surfaces.Rejected: infer the flag from the packet's size, which guesses at what the file already stated.
Rejected: drop the framing and document it, which is what the previous round did without
noticing, and which makes the "identity, not just content" claim untrue for the largest payload
the path carries.
Reverses: -
Filed: -
The XMP packet is a single-value payload: a carry replaces one already set.
add_xmpappended, sowith_xmp(..).with_metadata(..)wrote twoiTXtchunks under§11.3.3.1 Table 21's one reserved keyword, and this crate's first-wins reader then discarded
the carried packet.
Rejected: leave it and document that a caller must not do both — a silent loss inside the
feature built to end silent loss.
Reverses: -
Filed: #600 — moving the packet into
textswould fix this, the parallel-field seam and the chunk-order seam together, but itreshapes a public type on a crate about to be version-bumped, so it is a decision of its own.
Only a null byte refuses an encode. §11.3.3.1's keyword clauses report instead.
§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
justified carrying
sRGBandiCCPtogether. Five keyword shapes this crate's readeraccepts and returns unchanged (leading space, trailing space, consecutive spaces, C0/C1
control, U+00A0) were refused on re-encode, so a conversion failed over a file whose pixels
were fine and the only escape discarded the file's ICC profile too. Those keywords are now
written verbatim and reported; a keyword no chunk can hold, a non-ASCII
iTXtlanguage tagand a non-UTF-8 XMP packet are dropped and reported. A null still refuses, because the chunk
would re-parse as a different annotation.
Rejected: keep the refusals and add an opt-out flag — a second knob for a clause the
specification does not bind.
Rejected: drop the offending annotation silently, which is the silent loss this issue is about.
Reverses: decision 6, which predicted exactly this failure ("enforcing the rest would make
preservation fail on real-world files whose keywords carry a trailing space") and was then
contradicted by the code without the record being amended.
Filed: -
The reporting channel is renamed, because it no longer reports only drops.
DroppedMetadatabecomesMetadataNoticeanddropped_metadata()becomesmetadata_notices(), withMetadataNotice::carried()separating a payload left behind fromone written with a caveat.
gamut convertwords the two cases differently.Rejected: keep the name and file
TextKeywordSpacingunderDroppedMetadatafor anannotation that is written — an enum whose name contradicts three of its variants.
Rejected: report only the drops and stay silent about the written-verbatim cases, which leaves
a caller unable to learn that its output carries a keyword some readers reject.
Reverses: -
Filed: -
A
tEXttext string may hold any Latin-1 character (§11.3.3.2), not only the printablesubset (§11.3.3.1's closing paragraph). The two clauses contradict each other one sentence
apart; the more specific and more permissive is taken, so a conforming annotation is no longer
silently promoted to a different chunk type — which contradicted the identity claim this work
makes. Both halves are quoted from the vendored text in
ancillary.rsandSTATUS.mdratherthan one being picked in silence. The keyword rule is unchanged, being specific to keywords
and shared by all three chunks.
Rejected: keep the tighter reading because promotion is lossless — it is lossless in
characters and not in chunk type, which is precisely what
TextChunkKindexists to preserve.Reverses: part of the reasoning behind decision 5's promotion rule, not its outcome.
Filed: -
The colour-chunk ranking stays with a colour-management module, not this encoder.
with_metadatawrites every colour chunk the source carried and resolves nothing. §4.3 Table 1ranks them for a reader; which one to honour depends on whether the reader has a CMM at all,
which an encoder cannot know. The carry's justification is therefore a claim about other
readers — that a conforming one will pick correctly — not about this crate, whose own reader
surfaces all of them without ranking.
Rejected: drop the lower-priority chunk at encode time, which throws away colour information
the source carried and bakes a reader's capability into a writer.
Reverses: -
Filed: -
The four behavioural findings land as one commit, not four. They are entangled in
ancillary.rsandencoder.rs— the notice channel must exist beforeadd_xmpcan reportthrough it, and
gamut-clicannot compile across the rename — so a four-commit series wouldneed reconstructed intermediate states that do not correspond to any state this work was ever
in, and two of the four would not build. The code lands as one commit whose body names each
defect and its mechanism; the record, the added parser tests and the colour-ranking note
follow as three commits of their own.
Rejected: four commits that individually fail to build, which breaks bisect for a cosmetic
gain.
Reverses: -
Filed: -
Only the citations on this change's own surface are corrected. Three read-surface doc
comments named the wrong clause against
references/png/png-3.html— cICP as §11.3.2.5(which is sRGB), sRGB as §11.3.2.4 (which is sBIT), eXIf as §11.3.4.4 (which is sPLT) — and
are fixed here. A fourth, wider error found in the same audit —
tRNScited as §11.3.2.1,which the spec gives to
cHRM, in six files across the encoder, the reducer and two testsuites — is filed rather than swept up.
Rejected: fix all of them, which turns a metadata-preservation review into a crate-wide
citation sweep across files this change does not otherwise touch.
Reverses: -
Filed: #604.
Appended by this lane in round 5, after review:
A null in a text string drops the annotation and reports it; it does not refuse the
encode. The two clauses cited for the refusal deny the mechanism given for it: §11.3.3.2
says "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". 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, as does one in an
iTXttranslated keyword. This crate's own readerhands such a text back whole (executed on
tEXt,iTXtand a compressedzTXt), so it wasthe last shape that made a re-encode fail on a file gamut itself accepts — the failure class
decision 10 exists to eliminate, under the rule decision 10 states.
Rejected: keep the refusal — it is the position the review refuted, and its stated mechanism
is false.
Rejected: write the text verbatim with a "written, but…" notice, as §11.3.3.1's advisory
keyword shapes are written. libpng truncates such a text at the null, so the chunk would hold
one annotation for this crate and a shorter one for libpng; a payload whose content depends on
who reads it is not preservation. Unlike the keyword shapes, this clause is a flat prohibition
on content, not a recommendation about form.
Reverses: decision 10's "a null still refuses" for the text string only; the keyword and
translated-keyword refusals stand.
Filed: -
carried()is derived from the entry's emit flag, not from the notice variant alone.itxt_entrypushes the language-tag notice aftertext_entryhas already cleared the emitflag for a keyword no chunk can hold, so
metadata_notices()reported a notice whosecarried()istruefor an annotation of which zero chunks were written (executed). Thevariant cannot know this — it crosses the C ABI as a plain integer and must stay a pure
function of itself — so
text_noticesfilters instead: an entry that was not emitted reportsonly the notices that explain the drop. That is the root, and it also closes the row the
review's mechanically derived keyword table omits.
Rejected: pair each notice with a
carriedbit in the returned type, which reshapes a publicAPI over a defect that is a reporting-order bug, and leaves
MetadataNotice::carried()eitherredundant or contradicting its neighbour.
Rejected: suppress the language-tag notice at the push site in
itxt_entry, which fixes theone shape the review found and leaves every future "written, but…" notice free to be recorded
on a dropped entry.
Reverses: -
Filed: -
The
iCCPprofile name is filed, not fixed here. §11.3.2.3 gives it word for word thekeyword's rules — 1–79 bytes, printable Latin-1, no leading/trailing/consecutive spaces — and
the writer applies none of them: executed, a non-Latin-1 name becomes mojibake and an empty,
over-long or null-bearing name loses the profile silently, with no notice. It is pre-existing,
unreachable through the carry (a name read out of a file has already passed the reader's 1–79
byte split), and outside this manifest.
Rejected: fix it here, which widens a metadata-preservation review into the colour-profile
writer on a branch already three rounds long.
Rejected: leave it unfiled, when the notice channel it needs now exists and the executed
evidence is in hand.
Reverses: -
Filed: #619, with the executed table.
A keyword outside §11.3.3.1's repertoire is reported as "non-conforming per §15.3.1", not as
something "another reader may reject". The review established that no real reader rejects a
deviant keyword — libtiff's and libpng's read paths never validate one — so the old wording was
both vaguer and less true than the conformance statement the specification supports: §15.3.1
requires that "All field values in the PNG datastream obey the relationships specified in this
specification".
Rejected: keep "another reader may reject it", which asserts about readers what the reviewer
disproved by reading them.
Reverses: -
Filed: -
with_cicp's "unlike thesRGB/iCCPpair" clause is deleted rather than qualified. Itis residue of a refusal this branch removed and it contradicts decision 13 and
with_srgb'sown doc two methods above; the sentence says everything it needs to without it.
Rejected: rewrite it to explain the pair, duplicating
with_srgb's paragraph at a methodabout a different chunk.
Reverses: -
Filed: -
TextStringNull's user-visible line names both payloads; the XML consequence goes in thevariant's doc. The XMP packet is carried as an
iTXtunder §11.3.3.1 Table 21's reservedkeyword, so it reaches the same notice as an ordinary annotation, and the line a caller reads
said "text annotation" for something that is not one. Of the two ways to close that, widening
the line is the one that reaches the caller — a doc comment reaches only whoever is reading
the source — so the line now names both, and the variant's doc carries what the second path
additionally means: XML 1.0 does not admit U+0000, so a packet that reaches this notice is
already not well-formed XML, not merely unwritable.
Rejected: a distinct notice variant per payload, which splits one clause across two
discriminants for a case only a corrupt packet reaches.
Rejected: document the XMP path and leave the line alone, which leaves the wrong payload named
in the one string a user actually sees.
Reverses: -
Filed: -
The companion fix in the same commit takes no fork:
TextEntry::noticesenumerated the noticekinds and decision 16 made that enumeration incomplete. It is not completed by hand — a copy of
an enum's variants is what went stale — so the field points at
MetadataNoticeas the list.Correction to decision 10. Decision 10's
Reverses:field names decision 6 only. It alsoreverses decision 5, whose outcome — "A non-Latin-1 text keyword refuses the encode; it is
not dropped and not transliterated" — decision 10 replaced with a drop and a notice; decision 5's
own
Rejected:line ("drop the annotation silently") is not what decision 10 does, since the dropis reported, but the outcome it states is no longer the code's. Decision 5 is left as written; this
entry is the pointer. Decision 16 above in turn reverses part of decision 10.
Unresolved review notes
(none)