feat(png): clean a caller-supplied palette before writing it - #614
Open
justin13888 wants to merge 98 commits into
Open
justin13888 wants to merge 98 commits into
justin13888 wants to merge 98 commits into
Conversation
`gamut_png::deconstruct` classifies every byte of a PNG into a typed `Segment` and reports the figures an encoder-efficiency comparison is built from: bits per pixel, what the DEFLATE stage achieved in isolation, how many bytes went to chunk framing, and which scanline filter each row chose. It works on any PNG, whichever encoder wrote it, which is the point: the same numbers can be read off libpng's, oxipng's or zopflipng's output and compared directly. Issue #224 asks for BPP efficiency and parity, and neither is answerable from a total byte count alone -- a size difference has to be attributable to a stage before it can be acted on. Shape follows `gamut_tiff::deconstruct` / `gamut_dng::deconstruct` for the entry point and verdict method, and `gamut_isobmff::segments` for the `Segment { range, kind }` tiling. gamut-png does not and must not depend on gamut-isobmff, and that walk is box-structured anyway, so PNG needs its own -- but the names are deliberately identical. Owned rather than borrowed, unlike the ISOBMFF one. Its segments borrow because they are the only route to an unknown box's bytes; PNG already has `metadata()` for payloads, so the report carries only counts and ranges and can be `Clone + PartialEq + Eq` and stored across a bench corpus without pinning every input buffer alive. Deliberately more tolerant than `metadata()`, which rejects an unknown critical chunk: a measurement tool that refuses to measure is useless. Unknown chunks of either criticality, CRC mismatches, a missing IEND, trailing bytes and a truncated tail are reported, not errored -- `gamut_dng::deconstruct`'s contract verbatim. Only a file with no header to report on fails. The filter histogram is the one part that costs work and can fail, so it is `Option`. The inflation bound needs no policy: PNG's filtered length is *exactly* determined by IHDR, so `max_out` is that length and a zlib bomb cannot exceed it by a byte; a hostile IHDR is handled by declining to inflate past the decoder's existing 64 MiB image budget. Everything else in the report comes from framing and IHDR, so it survives a corrupt, truncated or oversized stream. `RawChunk` gains its own `range`, taken from the offset `ChunkReader` already advances, so byte accounting cannot drift from framing arithmetic; the reader gains an `offset()` so a caller can bound a malformed tail. `PngHeader` gains `PartialEq, Eq` -- additive, and a plain `Copy` header should be comparable. Tests are the byte-accounting law, the family `docs/testing.md` names after `gamut-avif`/`gamut-heic`'s `tests/accounting.rs`. `assert_covers` re-derives the tiling rather than trusting `is_fully_classified`, which is the thing under test. Fixtures come from libpng wherever the claim is about reading a foreign file: interlaced streams, forced filters and sub-byte depths are all things `PngEncoder` cannot write, and a histogram checked against gamut's own filter choice would be self-consistent rather than correct. Two findings from writing them, both recorded in the code: * A trailer counts against `is_intact` even though §13.2 lets a decoder ignore trailing bytes. `bits_per_pixel` divides the whole file by the pixel count, so bytes outside the datastream inflate the headline figure and a size comparison has to know they are there. * The CRC fixture corrupts a stored CRC, not a payload. Corrupting IHDR's payload makes the header unparsable, which is a hard error and a different claim entirely. Refs #224
A `benches/` target compiles as a separate crate, so it can only reach `pub` items -- and every encoder stage is crate-private. Timing them one at a time needs a seam. `src/stages.rs` is that seam, and it is re-exports and nothing else. No wrapper bodies: a wrapper would be an executable line no gate ever runs, since bench targets carry `test = false` and neither `cargo test`, `cargo llvm-cov` nor `cargo mutants` reach them. It would drag the coverage floor and generate mutants no test could kill. `.cargo/mutants.toml` already states the rule this follows, in its `crates/gamut/**` entry: "pure feature-gated re-exports (no function bodies), so it carries no logic of its own to mutate." So this needs no new exclusion. The stage items become `pub` inside their still-private modules, which changes no effective visibility -- a `pub` item in a private module is unreachable. With the feature off the crate's public API is byte-identical to before. `test-support` follows the convention gamut-core, gamut-ifd and gamut-tonemap use for their `invariants` modules: additive, `doc(hidden)`, no SemVer guarantee, and never enabled by the `gamut` umbrella, so the shipped surface and `mise run check-ffi-features` are unaffected (both verified). `Crc32::new` gains an `expect(clippy::new_without_default)` rather than a `Default` impl. Nothing in the crate would call such an impl, so it would be an uncovered region and an unkillable mutant -- dead delegation added only to satisfy a lint. Refs #224
gamut-png was one of the few codec crates with no `benches/` directory, and
both `README.md` and `STATUS.md` claimed "output size is benchmarked against
libpng at maximum compression" -- a claim no code backed. This is that
benchmark.
Two tables print before the divan run, following gamut-deflate's and
gamut-dng's shape: output size and bits-per-pixel against libpng at zlib
level 9, then where the bytes went stage by stage. Every column of both comes
from `gamut_png::deconstruct` reading the encoded file back, so the libpng
column is a like-for-like measurement rather than two encoders' self-reports,
and a size difference can be attributed to filtering, to the colour-type
choice, or to DEFLATE.
libpng gets the *same source layout* gamut gets, with no `palette` option even
for palettisable rows -- handing it a palette would hand it gamut's own
reduction and the comparison would stop measuring anything. Its default
adaptive filtering is left alone: that is the honest baseline.
The measured baseline, recorded here so the next change has something to be
judged against (one machine; read the ratios, not the times):
input raw default best libpng-9 best/lp9
gradient_rgb8 196608 2831 2272 2393 -5.1%
photo_rgb8 196608 29885 20293 27467 -26.1%
noise_rgb8 196608 196983 196983 197280 -0.2%
grey_as_rgb8 196608 721 370 566 -34.6%
palette64_rgba8 262144 1274 715 1102 -35.1%
sprite_rgba8 262144 4181 3729 3889 -4.1%
flat_rgba8 262144 821 103 664 -84.5%
tiny_rgb8 768 136 135 138 -2.2%
gamut is smaller than libpng-9 on every row. The stage table shows why, and
where it is not: `sprite_rgba8` -- binary alpha over invisible colour noise --
stays TruecolorAlpha where the reduce cascade should reach it, which is
exactly the tRNS-colour-key and dirty-alpha gaps this issue is about.
Corpus notes, both of which cost a fixture rewrite to get right:
* 256x256 is the floor that means anything. RGB at that size is 192 KiB,
roughly six times the 32 KiB DEFLATE window, so LZ77 match behaviour is
real; a 64x64 image fits *inside* the window and would flatter both
encoders equally.
* The "incompressible" row is a full avalanche mix, not the plain
`i * 2654435761 >> 24` gamut-deflate's bench uses. Over a dense index that
top byte changes only once every few hundred `i`, so the first version of
this row compressed 97x and measured nothing at all. It now expands
slightly, as any lossless codec must on random data.
Per-stage rows sit behind `test-support` and are skipped without it, so plain
`cargo bench -p gamut-png` and `mise run bench` still work. No
`required-features` on the target: `mise run bench` passes no features, and
the whole bench would silently never run.
Refs #224, #149
`README.md` and `STATUS.md` have long claimed "output size is benchmarked
against libpng at maximum compression". The previous commit prints that
comparison, but a bench asserts nothing and does not run in the per-PR gate.
This makes the claim enforceable: a regression in the crate's reason to exist
fails the build, the same mechanism gamut-deflate's ratio contract and
gamut-webp/tests/effort.rs use.
Every budget carries its own written justification naming the stage that
spends the bytes, in the shape of gamut-cmm's precision-budget table, and
records what the row measured when the budget was set so drift shows up in
review rather than as a surprise red build. Measured at 128x128 -- half the
bench's side, so this stays fast enough for the coverage and mutation lanes.
row gamut libpng-9 ratio budget
gradient_rgb8 703 749 0.939 0.98
photo_rgb8 5843 7768 0.752 0.85
noise_rgb8 49348 49435 0.998 1.01
grey_as_rgb8 146 251 0.582 0.70
flat_rgba8 96 299 0.321 0.45
sprite_rgba8 1669 1733 0.963 1.00
palette64_rgba8 451 405 1.114 1.15
The last row is the finding, and the budget records it rather than hiding it.
gamut auto-palettises where libpng writes RGBA: at 256x256 that wins by 35%,
at 128x128 it loses by 11%. Measured with `deconstruct` across four sizes:
side gamut IDAT PLTE+tRNS libpng-9
128 451 121 273 405
160 511 181 273 572
192 564 234 273 707
256 715 385 273 1102
The cause is not that `reduce::analyze8` ignores the palette chunks -- it
counts them, estimating 280 bytes against an actual 273. It is that the model
compares *raw* sizes, and raw size does not predict compressed size when one
candidate's bytes are incompressible and the other's are not. Those 273 bytes
survive DEFLATE intact while the RGBA alternative compresses roughly 160x, so
the estimate sees 16 664 against 65 536 and picks palette by a 4x margin that
does not survive compression. The crossover sits near 160x160. Filed
separately; a cost model that weighs incompressible overhead against
compressible pixels is what tightens that budget.
Four tests, each failing for one reason: the budget table, a strictly-smaller
assertion for the rows that claim a structural win, an attribution test, and
determinism. The winning set is listed explicitly rather than derived from
`max_ratio < 1.0` -- a budget loosened past 1.0 during a regression would
otherwise drop out of that test silently, which is exactly when it should
fail. Not hypothetical: palette64 was in the derived set before it was
measured.
The attribution test is why `deconstruct` is a dependency here. Where both
encoders land on the same colour type and depth the filtered stream is
identical by construction, so comparing the *compressed* streams isolates
DEFLATE from filtering and from the colour-type choice.
The corpus moves to `tests/common/corpus.rs` and the bench includes it by
path. Budgets are only meaningful measured on the same pixels the table
reports, and two copies would drift invisibly -- a budget that no longer
describes the row it names.
libpng gets the same source layout with no palette hint and its own default
adaptive filtering. Handing it a palette would hand it gamut's reduction.
Refs #224
At `alpha == 0` the colour channels are invisible by definition, but the
source's bytes are still stored and still cost. `with_transparent_cleanup`
zeroes them. Off by default, and deliberately separate from
`with_auto_reduce`: every other reduction in this crate is exactly reversible,
and this one is only reversible in what you can see.
It pays three compounding ways -- transparent pixels become identical so a run
filters to zeros; `analyze8` keys its palette on the whole RGBA quad, so
invisible pixels that differ only in unseen colour stop costing an entry each;
and it is the precondition for a tRNS colour key, which needs one colour to
stand for "transparent".
One constant, not the neighbouring pixel's colour, and that was measured
rather than assumed. Inheriting the predecessor flattens a run just as well,
but leaves every invisible pixel a distinct RGBA quad, so the palette and tRNS
benefits both vanish: on a fixture alternating visible and invisible pixels it
collapsed nothing and saved exactly zero bytes (378 vs 378). Zeroing collapses
them to one entry.
Two halves to the claim, so two techniques. That nothing visible changes is
differential: libpng decodes both files and every pixel with non-zero alpha
must be byte-identical, with alpha itself identical everywhere. That it pays
is a size assertion against the same image encoded without it.
Measured, and the interaction is worth stating plainly -- on the 256x256
sprite this makes the file *larger*:
side clean total colour type IDAT
64 false 859 TruecolorAlpha 802
64 true 817 Indexed/8 549
128 false 1669 TruecolorAlpha 1612
128 true 1925 Indexed/8 1477
256 false 3729 TruecolorAlpha 3672
256 true 4589 Indexed/8 3781
The cleanup is not what regresses: its IDAT is smaller at every size. What
happens is that collapsing the invisible colours drops the image under the
256-colour cliff, so `analyze8` now offers a palette -- and the raw-size cost
model then picks it, exactly as it wrongly picks it for `palette64_rgba8` in
the previous commit. Same defect, second independent witness, and cleaning
makes it reachable on more images. The next commit fixes the model; this one
would have been a regression shipped alone.
Refs #224
`reduce::analyze8` chooses by comparing **raw** sizes, and raw size does not
predict compressed size when one candidate's bytes are incompressible and the
other's are not. A palette carries PLTE (and often tRNS) that DEFLATE cannot
touch, while the pixels it replaces may compress by two orders of magnitude.
Two independent measurements from the previous commits:
* `palette64_rgba8` at 128x128: PLTE + tRNS is a flat 273 bytes, the indexed
pixel data compresses to 121, and the RGBA alternative compresses to 405
in total. The estimate sees 16 664 against 65 536 and picks the palette by
4x. Finished files: 451 against libpng-9's 405 -- the only corpus row
where gamut lost.
* The sprite, once transparent-colour cleanup collapses its invisible pixels
under the 256-colour cliff, becomes palettisable and is then chosen at
every size: 817 vs 859 at 64x64, but 1925 vs 1669 at 128 and 4589 vs 3729
at 256.
Same defect, and cleaning made it reachable on more images.
Rather than guess a correction factor, `write_reduced_or_native` encodes both
candidates and keeps the smaller. That is exactly what
`FilterStrategy::BruteForce` already does for filters, it needs no tuned
constant, and it cannot be worse than either candidate alone. A tie keeps the
palette, which decodes with less work.
Only palette reductions pay for the second encode. Greyscale, alpha-drop and
16->8 demotion add no chunks, so for them the raw comparison is already sound
and the function returns immediately.
Measured after:
row before after
palette64_rgba8 @128 451 390 (libpng-9: 405, now a win)
sprite_rgba8 +clean @256 4589 2619 (uncleaned best: 3729)
The sprite is the striking one: cleanup was a 23% regression and is now a 30%
improvement, because the race stops the analysis's mistake from landing.
Two oracle tests changed, and the reason is worth stating rather than burying.
Both pinned a *colour type* as a proxy for "a reduction happened", and the
race decouples those: the analysis still offers a palette, the encoder now
declines it when it would cost bytes. On 32x32 fixtures with a handful of
repeating colours the unreduced stream genuinely wins, so the old expectations
were asserting the defect. They now assert the contract that matters -- the
pixels survive, and the smaller file is kept -- and a new
`a_palette_is_chosen_when_it_actually_wins` covers the other side of the race
at 192x192, where the fixed cost is amortised. Without it the palette encoding
path would only ever be exercised where it loses. The analysis contract itself
stays pinned by `reduce`'s own unit tests, which is where it belongs.
Refs #224
Both hot loops the new benchmark exposed, neither needing any `unsafe` in
gamut. Output is byte-identical: every row of the size table is unchanged, and
the oracle, determinism and size-contract suites all still pass. This buys
time, not bytes.
before after
crc32 420.8 MB/s 8.996 GB/s 21x
filter_image None 497.9 MB/s 16.26 GB/s 33x
filter_image Paeth 277.1 MB/s 1.202 GB/s 4.3x
filter_image MSA 46.7 MB/s 265.8 MB/s 5.7x
choose_min_sum_abs 68.0 MB/s 308.4 MB/s 4.5x
CRC-32 moves to `crc32fast`, which dispatches to PCLMULQDQ/AVX-512 on x86-64
and the `crc32` instructions on aarch64, with a table fallback elsewhere
including wasm32. Its `unsafe` stays inside that crate; gamut-png remains 100%
safe Rust, which is why this needed no policy change. The two existing unit
tests stay exactly as they were, now as a drift guard: they pin the polynomial
this module's doc claims, so a backend computing a different CRC-32 variant
fails here rather than silently producing files no decoder accepts.
The filter loops needed no dependency at all. Three structural pessimisations
were blocking the vectoriser, and removing them is most of the win:
* The `i >= bpp` test choosing between a real left-neighbour and an implicit
zero is loop-invariant. The row now splits into a `bpp`-long prologue
where `a` and `c` are zero and a body where they are not. That collapses
Sub to a copy in the prologue and, less obviously, Paeth to Up, because
`paeth(0, b, 0) == b` for every `b` -- at `b == 0` all three distances tie
and the spec's order picks `a`, which is also zero.
* The body reads five equal-length subslices, so the bounds checks fold away
instead of being re-proved per index.
* The filter is matched once outside the loop instead of once per byte, and
`out` is sized once instead of a capacity check per `push`.
Separately, `MinSumAbs` was filtering each scanline **six** times, not five:
`choose_min_sum_abs` computed all five candidates, returned only which one
won, and `filter_image` then recomputed exactly those bytes. It now hands back
the winning buffer, trading a `memcpy` per improvement for a full filter pass
per row.
`unfilter_row` is deliberately untouched. Forward filtering has no serial
dependency, so all five kernels vectorise; reconstruction reads
`row[i - bpp]` after writing it, so only `Up` would benefit and this is an
encoder-first crate.
Refs #224
…onvention
`gamut-png`'s STATUS gains an Efficiency section: the size table against
libpng-9, the throughput before/after, a per-axis scorecard of the nine things
a PNG encoder competes on, and the measured explanation of why the palette
choice is now a race rather than an estimate. Every number is reproduced by
`cargo bench -p gamut-png` and gated by `tests/size_contract.rs`.
Its README and STATUS both claimed "output size is benchmarked against libpng
at maximum compression" while no code did either. They now say what is true:
measured by the bench, enforced by the contract.
`docs/benchmarking.md` is new, and takes an owner for something that had none.
`docs/testing.md` disclaimed benchmarks by name, and `docs/README.md` makes
anything unlisted there "descriptive, not binding" -- so the conventions every
bench in the workspace already follows were binding on nobody. It is normative
for where a benchmark lives, what a size or ratio table must record, and where
a measured number is kept, and it hands the enforcement question back to
`testing.md` explicitly. The rule it turns on:
A benchmark reports. A test asserts. Only the test can fail a build.
It also records what CI actually does now, which changed under this branch:
`mise run lint`'s `--all-targets` compiles every bench on every PR, and the
Extended lane's `mise run bench-test` runs each once. Neither gates a number,
and the document says why that is still open rather than implying benches are
ungated.
Both normative documents change here because `docs/README.md` requires it: a
`docs/` file that contradicts another is a change to both.
Seven follow-ups filed with their measured evidence rather than left as prose:
#478 gamut-deflate: 8-byte-at-a-time longest_match -- the dominant cost of
every encode in the workspace, safe Rust, byte-identical output
#479 gamut-deflate: relax each length at its own nearest distance
#480 gamut-png: entropy and bigram heuristics, pruned two-tier trials
#481 gamut-png: tRNS colour key for grey and truecolour
#482 gamut-png: palette ordering and caller-supplied palette cleanup
#483 gamut-png: metadata policy, and the CLI's silent drop
#484 gamut-png: parallel filter trials, and a composed effort dial
Refs #224
CI's diff-scoped mutation run surfaced ten survivors across the four shards.
None was noise: each one names a claim the new code makes that nothing
actually checked.
Three needed only a fixture that could tell the difference:
* `is_fully_classified`'s `||` and its whole body. `deconstruct` cannot
produce a malformed tiling -- it is correct by construction -- so every
negative case has to be built by hand. Inline tests now assemble reports
with a gap, an empty segment, an overlap, a late start and an early end,
each isolating one half of the predicate.
* `ChunkStats`'s `count += 1` and `payload_bytes += len`. Every fixture
carried at most one chunk of each type, so the accumulate arm never ran
and `count` sat at the 1 it is inserted with. Two tests now cover it: a
hand-built file with two `tEXt` chunks, and a real multi-IDAT encode that
also ties the chunk table back to `idat_compressed`.
* `filter_histogram`'s `at += 1 + row_bytes`. Mutated to `*=` the cursor
stays at 0 and every row's filter byte is read from the same offset --
indistinguishable while every histogram test forced a *single* filter for
the whole image, because both report `height` of it. A fixture whose rows
genuinely choose differently now pins that at least two buckets are
non-empty.
Three were untestable where they stood, and moved rather than being papered
over:
* The inflation budget (`filtered_len == 0 || filtered_len > MAX`). Reaching
the boundary through `deconstruct` would need a real 64 MiB stream either
side of the cap, and a hostile IHDR cannot separate `>` from `>=` or `==`
because an over-budget file is rejected a second time when the inflated
length fails to match. Now `within_inflation_budget`, tested at 0, 1, the
cap and one past it.
* The palette-vs-native tie-break. Engineering two encodings of one image to
land on exactly equal lengths is not something a fixture can do reliably,
so `prefers_native` carries the comparison and a unit test pins the
documented rule: a tie keeps the palette.
* `clean_transparent`'s "is there anything to do" check. Mutated to `!=` it
returns `Some(unchanged copy)` for a fully opaque image instead of `None`,
which the encoder cannot see -- the bytes are identical either way. The
distinction is that the encoder must be able to tell "no work" from "work
that changed nothing", or it allocates a whole image for nothing, so the
test is on the function.
And one was an equivalent mutant, removed rather than tested: the
`start < png.len()` guard before pushing a `Truncated` segment can never be
false, because `next_chunk` returns `Ok(None)` when nothing is left and only
errors with bytes remaining. It was dead code wearing a safety net's clothes;
a `debug_assert` records why.
Refs #224
`gamut inspect` already answered "did every byte get accounted for?" for TIFF
and DNG. For PNG the same walk answers a second question -- where did the
bytes go? -- which is what makes an encoder comparison possible from the
command line, on files this crate did not write.
PNG prints on its own path rather than being flattened into `Summary`. It has
no IFD tree and no tag vocabulary, but it carries compression figures the
others have no equivalent for, and forcing both through one shape would lose
the half that matters.
Verified end to end on libpng's own `pngtest.png` -- Adam7 interlaced, 18
chunk types including five this crate does not recognise (`sTER`, `vpAg`,
`oFFs`, `pCAL`, `sCAL`):
image: 91x69 TruecolorAlpha depth 8, Adam7 interlaced
size: 8759 bytes (11.160 bits/pixel)
IDAT: 8119 bytes compressed from 25247 filtered (32.2%)
overhead: 640 bytes, of which 216 is chunk framing
filters: None 21 / Sub 15 / Up 52 / Average 10 / Paeth 33 (131 scanlines)
classified: yes
intact: yes
Every byte of a foreign file classified, and the filter distribution counted
across seven Adam7 passes. Truncating it to 4000 bytes reports
`truncated from offset 342 (3658 bytes)`, keeps every framing- and
IHDR-derived figure, drops only the histogram, and exits non-zero.
`Crc32::new`'s lint suppression changes from `expect` to `allow`, and the
reason is worth recording: `clippy::new_without_default` only fires when
`test-support` re-exports the type through `crate::stages`, so an `expect` is
*unfulfilled* in a default-feature build and fails there instead. That is
`expect` working correctly -- it caught its own obsolescence in one of two
configurations -- but a feature-dependent lint wants `allow`.
Refs #224
The one lawful PNG representation this encoder could not write. The crate said
so itself, at `decoder.rs:1327`: "the encoder cannot write interlaced files or
greyscale/truecolour tRNS colour keys". The decoder has always read them, so
only the encoder half was missing.
Three conditions, all necessary, because §11.3.2.1 gives a decoder exactly one
transparent colour and not a mask: every alpha is 0 or 255; at least one pixel
is transparent; and every transparent pixel shares one colour that no opaque
pixel uses. That last one is why `with_transparent_cleanup` pairs with this --
it collapses every invisible pixel to one colour, which is precisely what a
key needs.
Two passes, not one: the candidate is unknown until the first transparent
pixel is seen, so proving no *earlier* opaque pixel used it needs a second
look. The second only runs once the first has found a candidate.
The measurement changed the design twice, and both are recorded in the code
because neither is guessable:
* **It is worth ~7-9%, not the 25% the raw-byte arithmetic suggests.**
Dropping a channel removes 25% of the samples, but the alpha plane is
usually the most compressible plane in the image, so most of that is
already free. On a 128x128 sprite: 863 bytes keyed against 926 plain.
* **Only on a contiguous transparent region.** With the transparency
scattered by a hash instead, the invisible colour interleaves with the
visible gradient and wrecks the RGB channels' compressibility: `RGB+tRNS`
came out at 14 886 bytes against plain RGBA's 14 319, and the race
correctly declined the key. The first version of the fixture here was
scattered, and the tests failed until the shape matched what real sprites
and icons actually look like.
So keyed encodings join `Indexed` in `write_reduced_or_native`'s race rather
than being taken on the estimate. A `tRNS` chunk is incompressible in exactly
the way a `PLTE` is, and the same raw-size blind spot applies: at 32x32 and
64x64 the analysis offers a key and the race is right to refuse it.
Tests go through libpng in every case rather than round-tripping gamut against
itself: gamut writes the key and libpng interprets it, so a round trip could
agree on a wrong convention and prove nothing. That includes pinning the
payload bytes, since §11.3.2.1 wants three *16-bit big-endian* samples and a
decoder reading them as three bytes would key on the wrong colour.
Refs #224. Closes #481.
Axis 3 moves to done, with the measured figure rather than the raw-byte one: ~7-9% on a contiguous transparent region, because the alpha plane a key removes is usually the most compressible plane in the image. Refs #224
Palette index order is not free. It decides the `tRNS` chunk's length, and it
decides what the row filters see, because a filtered index stream is the
*difference* between neighbouring indices. Discovery order -- raster scan --
optimises neither.
Two rules. Transparent entries first, so the trailing-opaque `tRNS` trim cuts
as much as §11.3.2.1 allows; one late transparent entry used to pin the whole
chunk to full length. Then by Rec. 601 luma, so neighbouring indices are
neighbouring brightnesses and a smoothly shaded image produces small index
deltas rather than the arbitrary jumps discovery order gives.
Measured by disabling the ordering alone, so the figure is not confounded with
the colour key landing in the same branch:
row unordered ordered
sprite_rgba8 +clean 2619 2235 -14.7%
palette64_rgba8 715 726 +1.5%
A real trade, and worth stating rather than rounding to "it helps". The
sprite's gain is 35x the palette64 loss, and palette64's colours are synthetic
ramps whose discovery order already correlates with index adjacency -- the
case luma sorting is least able to improve and most able to disturb. The full
modified-Zeng ordering oxipng uses remains #482.
The rest of this commit closes the mutation gaps CI found in the previous
commit's colour key. All seven were in the cost estimate -- the guard deciding
whether to look for a key, the match on `all_gray`, and the arithmetic in both
arms -- and they share one cause worth recording, because it will recur:
**`write_reduced_or_native` makes the estimate much less observable.** A
mutated cost still produces a keyed candidate, which still races the unreduced
encoding, and the smaller still wins. So perturbing the estimate usually
changes which candidate is *offered* without changing the bytes that finally
win. That is the race doing its job -- it is exactly why the estimate stopped
being load-bearing -- but it means an estimate can no longer be tested through
the encoder.
So the arithmetic moves into `may_have_colour_key` and `keyed_size`, tested
directly, with the chunk costs as named constants derived from the spec
(2 + 12 for greyscale, 6 + 12 for truecolour) rather than as literals. Same
treatment the inflation budget and the palette tie-break already got.
Refs #224. Closes #482.
The sprite row's cleaned figure moves 2619 -> 2235 and palette64's 715 -> 726, which is the trade the ordering commit measured. Axis 4 moves to partial: ordering landed, modified-Zeng and the caller-supplied palette path remain. Refs #224
Sum-of-absolutes asks "are these bytes small?". DEFLATE asks "are these bytes
repetitive?". Those are different questions, and a row alternating 0 and 200
answers the first badly and the second beautifully -- which is why oxipng
dropped libpng's MinSum from every preset except its cheapest and its most
expensive.
That is a preset table, not published byte counts, so gamut measured it on its
own corpus. IDAT bytes at `Level::Best`, each heuristic alone:
input MinSumAbs Entropy Bigrams winner
gradient_rgb8 2215 2215 1505 Bigrams
photo_rgb8 25364 22427 19513 Bigrams
noise_rgb8 196890 196890 196890 tie
grey_as_rgb8 475 506 506 MinSumAbs
palette64_rgba8 990 899 770 Bigrams
sprite_rgba8 3672 3857 4062 MinSumAbs
flat_rgba8 573 573 605 MinSumAbs
tiny_rgb8 79 79 62 Bigrams
Bigrams wins four rows by 22-32%; MinSumAbs wins three by 5-6%. Neither
dominates and the margins run the wrong way to drop either, so both are in the
brute-force set -- which is also the shape of oxipng's own presets.
**Entropy is never the unique winner, and that is recorded as a negative
result rather than quietly merged.** It beats MinSumAbs on the photographic
and palette rows but loses to Bigrams on both, and ties MinSumAbs elsewhere.
The brute-force set resolves by taking the smallest, so a candidate dominated
everywhere costs a full filter pass and a full DEFLATE for nothing. It is not
in that set. It stays selectable, because eight images is a corpus and not a
proof, and `docs/benchmarking.md` asks for the negative result to be written
down so nobody re-derives it.
End to end, with Bigrams in the brute-force set:
row before after
gradient_rgb8 2272 1562 -31.2% (vs libpng-9: -5.1% -> -34.7%)
tiny_rgb8 135 119 -11.9% (vs libpng-9: -2.2% -> -13.8%)
photo_rgb8 20293 19570 -3.6% (vs libpng-9: -26.1% -> -28.8%)
The scorers share one `Scratch` allocated per image, not per scanline: the
bigram set is 8 KiB of bitset and rebuilding it per row would dominate the
very measurement it exists to make cheap. A test pins that the scratch does
not leak state between rows, because a stale one would silently score every
row after the first against the previous row's data.
`tests/backends.rs`'s `rgb8_best_bruteforce` golden is re-captured: Bigrams
wins on that fixture and takes its IDAT from 36 bytes to 21. That pin exists
to prove the *codec-abi seam* is inert, not to freeze the encoder, so the
comment there now records the re-capture and why -- an encoder change making
output *larger* would look identical at that assertion and would be a
regression.
Refs #224, #480.
`choose_by` seeded `best_score` with `u64::MAX` and improved on a strict `<`, so a row whose five candidates all scored `u64::MAX` left `best_bytes` untouched. `filter_image` hoists that buffer out of the row loop, so such a row was emitted under a filter byte of 0 carrying the *previous* row's residuals -- or, on the first row, nothing at all. `Score::Entropy` reached that sentinel whenever no byte value repeated in the filtered row, which is ordinary for narrow images. A 2x1 Gray8 `[1, 3]` encoded to a PNG whose IDAT is shorter than its image; a 2x2 `[0, 0, 0, 1]` encoded to a structurally valid PNG decoding to `[0, 0, 0, 0]` -- silent corruption, no error anywhere. Two independent fixes, because one is a class and the other an instance. `best_score` becomes `Option<u64>`, so "nothing chosen yet" is unrepresentable as a score and the first candidate is taken whatever any scorer returns; a future scorer cannot reintroduce this. And the entropy score is restated as `sum c*log2(n/c)`, the quantity its doc already claimed, which is non-negative and bounded by `8n*256` -- so it can no longer collide with a sentinel at all. The tie-break is unchanged: the only comparison is still a strict `<` over candidates 2..5, and candidate 1 is `FilterType::None`, first in the documented None/Sub/Up/Average/Paeth order. No pinned bytes move, because `sum_abs` and `Bigrams` are bounded far below `u64::MAX` and so always wrote on their first candidate already -- the two paths are bit-identical for every strategy in `BRUTE_FORCE_STRATEGIES`, and `MinEntropy` is not in that set. `tests/oracle.rs` gains the end-to-end sweep whose absence hid this: `MinEntropy` was scored by unit tests but never encoded with.
`(a << 8) | b` over two `u8`s is spelling out `u16::from_be_bytes`, and it costs two operators that carry no meaning of their own. One of them has no behavioural variant at all: the low byte of `a << 8` is zero, so `|` and `^` compute the same index, and no test can ever tell them apart. `.cargo/mutants.toml` would accept a line-scoped exclusion with that argument written out. Restructuring is better and the file already prefers it -- `deconstruct.rs` twice shapes code so an equivalent mutant is never generated rather than excluding one after the fact. Reading the pair as the big-endian `u16` it is leaves no operator to mutate. The bigram vectors gain the case none of them covered: (1,3), (3,2), (2,3) is three distinct pairs over two distinct second bytes, so an index that dropped the high byte would report two. Every existing vector happens to have as many pairs as second bytes.
`analyze8` reached its colour-key branch through `key.expect(...)` -- the only `expect` outside `#[cfg(test)]` in the crate's `src/`, which the house rule forbids in library code paths. Fold the option into the guard with a let-chain, as the palette scan at the top of the function already does. Behaviour is identical: when no key was found `keyed_size` is `usize::MAX`, and `best` has already been proven smaller than `input_size`, so `best == keyed_size` could never hold. `colour_key` carried the same shape one level down. Its `any_transparent` flag was assigned in exactly the arm that assigns `candidate`, so `!any_transparent` was a spelling of `candidate.is_none()` that the following `candidate?` discharges again -- an unkillable mutant in a file `.cargo/mutants.toml` does not exclude. Drop the flag and record in the doc why condition 2 needs no check of its own, including the caller gate (`may_have_colour_key` requires `!all_opaque`) that makes the `?` itself unreachable in practice.
`ordered_palette` was untested as a function: every palette fixture in the crate happens to have discovery order equal to sorted order, so none of them could tell it from the identity. The three Rec. 601 weights survived mutation to additions for exactly that reason. Pin the luma order on a five-entry fixture chosen so collapsing any one weight to an addition returns a different sequence, and tabulate the four columns in the doc comment so the choice of entries is auditable. Pin rule 1 separately, through `build_indexed`, on a palette whose transparent entry is discovered last -- the case first-appearance order gets wrong. In discovery order the `tRNS` alphas are `[255, 255, 0]` and the trailing-opaque trim cannot shorten them at all; sorted transparent-first they are `[0, 255, 255]` and the trim cuts two of three.
A PNG chunk type is four unvalidated bytes and the deconstruct walk never drops a chunk, so a hostile file chooses how many *distinct* types it carries: one per 12-byte chunk. Accumulating the per-type totals with a linear scan over the types seen so far was therefore quadratic in the file length, reachable from `gamut inspect` on an untrusted file — 4.8 MB of empty chunks took 40.9 s. A private `ChunkTally` keeps a `HashMap<[u8; 4], usize>` beside the stats vector, so each chunk costs O(1) and the public `Vec<ChunkStats>` keeps the first-appearance order it documents. The map is dropped at the end of the walk and never surfaced; `ChunkStats` stays `Copy` and `#[non_exhaustive]`. Hashing attacker-chosen keys is safe only because the default hasher is SipHash-1-3 with a per-process seed, so that is recorded on the type: a faster unseeded hasher would reopen the blow-up by a different route. `PngReport::chunk` stays a linear scan — O(distinct types) per call, not quadratic — and now documents that cost, and that summarising every type means iterating `chunks` once rather than calling it per type. The regression test asserts a self-calibrating ratio rather than a wall-clock ceiling, which would be flaky under `llvm-cov` and parallel test binaries: two files of equal byte length and equal chunk count, one distinct type per chunk against one repeated type, deconstructed back to back in one process. Measured 3–5x with the index and 1488x without it (18.0 s against 12.1 ms), so the 20x bound has ~4x of headroom above the fix and ~75x below the defect.
`with_transparent_cleanup` documented "no effect on an image with no fully transparent pixel, or on a layout with no alpha channel", but `cleaned_samples` was only reached from `EncodeImage<Rgba8>` and `EncodeImage<GrayAlpha8>`. `Rgba16` and `GrayAlpha16` carry an alpha channel and can carry fully transparent pixels, so a caller enabling the knob on a 16-bit sprite got the documented behaviour's opposite: silently nothing. `reduce::clean_transparent` cannot serve those layouts — it reads one-byte samples on a one-byte stride, whereas a 16-bit pixel is invisible only when its whole alpha sample is zero, and clearing a colour sample must clear all sixteen bits. Add `clean_transparent16`, its `u16` twin, beside the encoder. Working on the samples rather than on the big-endian bytes `encode_16bit` serialises keeps the ordering identical to the 8-bit paths: cleanup runs first, so `reduce::analyze16` sees the collapsed invisible pixels. `encode_16bit` therefore takes dimensions plus samples instead of the `ImageRef`, so the alpha layouts can hand it a cleaned buffer. The inline tests pin the two things the byte-wise reading would get wrong: an alpha sample of `0x0001` is visible (its high byte is zero), and every cleared colour sample is cleared in both bytes. `tests/transparent_cleanup.rs` adds the end-to-end halves for both layouts against libpng — `decode` rather than `decode_rgba8`, which would scale 16-bit samples down to 8 and hide exactly that low byte — plus the size claim and the byte-identical no-op on an opaque image. Correct the doc to describe what is now true.
Four corrections that this branch's new bench, size contract and golden re-capture made due. `benchmarking.md`'s counter table said "per-pixel or per-sample kernel -> ItemsCount", which reads as a rule `gamut-png`'s stage benches break: they count `BytesCount` over `crc32`, `pack_scanlines`, `filter_image` and `analyze8/16`. They do not break it. Those are byte-oriented stages of a codec pipeline whose natural item *is* a byte, and counting items would put their figures in a different unit from the crate's own encode benchmark and its size table, which are the figures a stage row exists to be read against. The workspace's actual `ItemsCount` users are all kernels whose item is not a byte -- `gamut-dsp` counts transform coefficients, `gamut-tonemap` `f32` samples, `gamut-color` `f64` samples and pixels, `gamut-bitstream` coded symbols, `gamut-cmm` transformed pixels -- and bytes per second would say nothing about any of them. So amend the rule rather than the bench: add the byte-oriented-stage row and sharpen the existing one to name the distinction it was always making. `testing.md`'s per-crate authority row for `gamut-png` named only "differential + conformance", omitting the size contract this branch adds, while `gamut-webp` names its own. Mirror it, and cite `crates/gamut-png/tests/size_contract.rs` from the technique table beside `gamut-webp/tests/effort.rs`. `mise.toml`'s `bench-test` comment says why `--benches` is passed and counts the workspace's benches to make the point; `gamut-png`'s is the sixteenth. (The "all 15 crates" at the top of the file is about `tooling/` and is a separate claim.) `gamut-png/tests/backends.rs`'s header says the goldens were captured before the seam existed, which the per-row note directly below it already contradicts for `rgb8_best_bruteforce`. State the exception in the header instead of leaving the two to disagree; no golden byte moves.
The report walk capped the *filtered* stream at 64 MiB while documenting that cap as matching the decoder's image budget. The decoder budgets the *decoded* buffer instead, and the two differ by exactly one filter byte per scanline: a 4096x4096 RGBA8 image is 67 108 864 native bytes, which decodes on the default budget, and 67 112 960 filtered, which the walk declined — so `deconstruct` reported an undamaged file as damaged and `gamut inspect` exited non-zero on it. Two constants asserted to agree had drifted, so make the agreement structural. `ihdr::native_bytes` is now the single definition of the quantity; `PngDecoder::check_limits` reads it (byte-identical behaviour, pinned by `byte_budget_is_exact`), and `MAX_FILTERED_BYTES` / `within_inflation_budget` give way to `fits_decode_budget(header, max_image_bytes)`. The budget is a parameter, so the inclusive boundary is reachable from a unit test without a 64 MiB fixture. Inflation stays bounded: a file that passes inflates to at most the native bytes plus one per scanline. Kept, against the plan: `idat_ratio`'s `filtered_len == 0` guard. It was to be deleted as unreachable, but it is reachable in thirteen header bytes. §11.2.1 admits 2^31-1 square, which at RGBA16 implies 2^65 filtered bytes; `adam7::expected_stream_len` refuses to wrap and `deconstruct` reports such a file rather than erroring, leaving `filtered_len` zero. `gamut inspect` prints the ratio for every file it reads, so replacing the guard with a `debug_assert!` would have put a panic on a hostile-input path. The branch is pinned by a new accounting test instead, which is what makes it killable rather than equivalent.
`Reduced::GrayKeyed` is reachable and correct, but nothing in the suite produced one, so neither `analyze8`'s `all_gray` split inside the keyed arm nor the encoder's arm for it had a test that could see them. Two tests, at the two scopes the placement rule forces. `Reduced` is private, so the analysis side is pinned inline: grey with binary alpha, 64 opaque levels, and a 65-entry palette that keeps the palette estimate (540 bytes) out of a race the key wins at 270. The encoder side needs libpng, and is pinned in `colour_key.rs` as the greyscale twin of the existing truecolour differential: colour type grey at depth 8, a two-byte `tRNS`, and an exact round trip. The key is grey 7 rather than 0 in both, so the `tRNS` sample's byte order is observable -- written little-endian it would read `[7, 0]`, which a key of 0 could not distinguish from the correct `[0, 7]`. The greyscale win is thinner than truecolour's, since dropping the alpha plane saves one byte per pixel rather than three against the same flat 14-byte chunk. Measured, it wins anyway at every square from 32 to 256: 499 bytes against 626 at 128, about 20%, so the fixture needs no size threshold.
`write_reduced_or_native` races a chunk-carrying reduction against the unreduced encoding, and its `carries_chunks` set decides which reductions enter that race. The palette member had both sides covered; the keyed members had only the winning one. The three existing negative tests here all stay RGBA because no key was ever *offered* -- partial alpha, two invisible colours, a collision with a visible pixel -- not because a valid key lost on size, so dropping `Rgb8Keyed` from the set would have gone unnoticed. Add the losing side at 32x32 on the existing fixture, reconstructing the candidate that lost: the encoder's `Rgb8Keyed` arm is the RGB stream through the same configuration plus one 18-byte `tRNS`, so the test can assert the declined encoding really was the larger one (279 bytes against RGBA's 274) rather than merely that RGBA survived. Parameterise the fixture by side to do it, and correct the module doc while it is in hand: the crossover was measured at 32, not below 128 as the `SIDE` comment claimed -- at 48 the key already wins, 347 against 353.
Two halves of one gap. The off-grid grey case had been weakened from an exact colour-type assertion to `COLOR_GRAY || COLOR_PALETTE`; that fixture produces grey at depth 8, so the palette arm was a branch no input could take. Assert the colour type exactly again and say in the comment where the palette case is covered instead. It is covered here. `a_palette_is_chosen_when_it_actually_wins` needs 64 colours before the race takes the palette at all, and 64 entries is depth 8, so the encoder's `depth < 8` path into `pack::pack_scanlines` and `index_bit_depth`'s `3..=4 => 2` arm were only ever reached by inputs whose palette was then declined. Four colours at 192x192, arranged by a finalizer-quality hash of the pixel index rather than in blocks: blocked, the RGBA stream compresses away and the race keeps it, which is why the 64-colour fixture needed 64 colours. Scattered, both streams sit near their entropy and the 2-bit packing is the whole difference -- 9500 bytes indexed (9216 of payload) against 19 135 as RGBA. A cheaper mix was tried first and rejected: one multiply and a shift is periodic in x, DEFLATE finds the period, and the same fixture came out at 272 bytes.
`PngReport::filters` was `Option<FilterHistogram>`, so "no histogram" conflated a file this reader declined to inflate with one whose compressed data is broken — and `is_intact` treated both as damage. Now that the walk budgets what the decoder budgets, that conflation is the last thing standing between a large sound PNG and an intact verdict. `FilterScan` is `Counted(FilterHistogram)` or `Skipped(SkippedFilterScan)`, the reason being `#[repr(u8)]` plain data with explicit, permanent, append-only discriminants: `OverBudget`, `CorruptStream`, `LengthMismatch`, `UndefinedFilterCode`. `SkippedFilterScan::is_damage` is the single source of truth for the grading question — only `OverBudget` is not damage, since it describes the reader's budget rather than the file — and `is_intact` narrows its conjunct to `!filters.is_damage()` rather than dropping it, because a corrupt zlib payload under a valid CRC is damage nothing else in the report can see. `PngReport::native_bytes` exposes the budgeted quantity, so a caller can tell what an `OverBudget` verdict was measured against. `gamut inspect` prints the reason through a `filter_skip_label` with a wildcard arm, and pushes a damage-bearing skip into the findings list before printing it — the exit message used to read "0 finding(s)" while exiting non-zero on a file whose only defect was its IDAT stream.
The module doc said the command exits non-zero when the file "is not fully accounted for" without saying what that is, and the three formats name it differently: TIFF and DNG gate on `is_fully_accounted()`, PNG on `is_intact()`. They are the same strength, which is worth writing down — PNG's `is_fully_classified()` is printed but is not the gate, being true by construction for every file `deconstruct` accepts, so gating on it would exit 0 on a truncated PNG. Also records that an over-budget filter scan is not a finding, and moves the stray `/// The display name of a format.` off `inspect_png` and back onto `format_name`.
It is dead in the shipped crate — the encoder calls `choose_by` directly, and the wrapper carried `allow(dead_code)` off the `test-support` feature to say so. What it added on top of `choose_by` was a fresh 9 KiB `Scratch` per call, which `Score::SumAbs` never reads: the bench row it existed to serve was therefore measuring a per-scanline allocation the encoder never performs, and its question — what the sum-of-absolute-residuals heuristic costs per row — is already answered by the `filter_image / MinSumAbs` row. It was also a wrapper body in a seam whose own module doc forbids them: `stages` is "re-exports and nothing else", because bench targets are reached by no gate, so a body there drags the coverage floor and generates mutants nothing can kill. Its one test moves to `choose_by(Score::SumAbs, ...)`, the call the encoder actually makes, and keeps its teeth: inverting `choose_by`'s comparison still fails it.
The table's ratios were chosen by hand, so nothing said what a budget meant or when it should move. Each `max_ratio` is now `measured` times a stated headroom, rounded up to two decimals, and `Budget::max_ratio` carries the procedure for refreshing the whole table after an encoder change. The refresh also adds the three rows the bench reported and nothing gated: both `+clean` columns and `tiny_rgb8`. `Budget` grows `fixture`, `side` and `cleanup` so a cleaned row shares its twin's pixels instead of duplicating them. Two rows take less than the default 5%. `sprite_rgba8` measures 0.963, where 5% rounds past 1.00 and would surrender the claim the row exists to make, so it takes 2%. `palette64_rgba8 +clean` takes 2% because there is nothing to protect: cleaning *costs* bytes there, 403 against the uncleaned 364. That last row's justification had it backwards -- it predicted shorter PLTE and tRNS and therefore a smaller file. Both halves of that are true and the file still grows, because collapsing the transparent entries rewrites pixels that were compressing well and at 128x128 the second effect wins. `with_transparent_cleanup` is a canonicalisation, not an optimisation. The row now says so, which is the drift this refresh exists to catch. The gradient and photo rows move on their own: 0.939 to 0.772 and 0.752 to 0.731, from this branch's encoder work. Refs #224
… into feat/440-png-cabx
`find_c2pa` walked to end of input, unlike every other chunk walk in the crate (`parse_stream`, `walk_metadata_chunks` and `deconstruct` all stop at IEND). Latent today, since its only caller is the encoder reading back its own fresh output, but it made the "the encoder's report and the file's report cannot disagree" claim false for any later caller: a `caBX` appended after IEND — bytes §13.2 calls a trailer, outside the datastream — would have been reported as the file's manifest store. The walk now stops at the first IDAT or at IEND, whichever comes first, which states the store rule in one place: the first CRC-valid `caBX` before the first IDAT. Stopping at IDAT is C2PA 2.4 §A.3.2's placement — the store precedes IDAT and data after it is bad-form — and it is what keeps a chunk appended to a finished file from being taken as a store the file does not carry.
`fill_c2pa(&mut png, &span, store)` writes a finished manifest store into the `caBX` chunk a span names, rewriting the payload and the chunk CRC and nothing else. It is the second half of the reserve-then-fill flow C2PA 2.4 §18.5 describes, and the shape that flow actually needs: a signer hashes the reserved file with the chunk's span excluded, then fills it. Until now the only way to fill a reservation was to encode again with `with_c2pa`. That reaches the same bytes but costs a second full encode — at `Level::Best` with `FilterStrategy::BruteForce`, the whole brute-force set again — and it makes the signature depend on the encoder reproducing its output byte for byte. Filling in place is O(store) and depends on nothing but the chunk's own bytes, and it is the only route at all for a file gamut did not write. Every argument is validated before the first byte is written, with a distinct typed error each: the span must lie inside the image, frame a chunk (payload exactly `chunk.start + 8 .. chunk.end - 4`), name a `caBX`, and receive a store of exactly the reserved length. A store of the wrong length is rejected rather than resized: resizing would move every byte after the chunk and invalidate the hash the signer signed.
C2PA 2.4 §A.3.2 places the manifest store before IDAT and calls data after it bad-form, but the decode surfaced the first CRC-valid `caBX` wherever it sat. That let anyone append a `caBX` to a finished PNG and have it read back as that file's provenance — including into a file that carries no store at all, where the appended chunk became the only answer. The encoder never writes there, so nothing gamut produces was affected; the exposure was on read. The store is now the first CRC-valid `caBX` before the first IDAT, in all three readers that answer the question — `decode`, both `metadata` entry points, and `PngReport::c2pa` — with the addition done in one place so they cannot drift apart. An ignored chunk stays visible rather than being silently dropped: the counter now covers both reasons a `caBX` is not the store, a later one and one after IDAT, so `c2pa == None` with a non-zero count is exactly the shape of an appended store. It is renamed `c2pa_ignored` to say that, since "duplicates" is false when the file carries no original, and it becomes a `usize`: as a saturating `u8` a file with 300 ignored chunks reported 255, a number the file does not contain.
`walk_metadata_chunks` grew a second result — the count of `caBX` chunks found after IDAT — and the bare tuple tripped `clippy::type_complexity`. The alias carries the explanation the tuple could not: which chunks come back, and why the post-IDAT ones are a number rather than chunks.
The span docs claimed the report names the chunk the decode surfaces as its payload. That is false under a budget: `PngReport::c2pa` has no byte budget, so a store past `with_max_metadata_bytes` is still spanned while `decode().c2pa` is `None` — a caller gating on the report could get a `None` it had been told could not happen. The same block claimed the ignored count agrees with `chunk(b"caBX")`, which it deliberately does not, since that counts CRC-invalid and post-IDAT chunks too. Both now say what is true: the report answers *where the bytes are* and has no budget, the decode answers *what was admitted*, and each count answers its own question. Exclude the span from a hash; read the payload from the decode. Also states two things the code already did but the docs did not. The reserve-then-fill flow's step 3 is `fill_c2pa`, with re-encoding named as the costlier alternative rather than the route. And "last chunk before IDAT" is this writer's guarantee about files it produces, not a property of the format: PNG §14.3.2 says ordering relative to other *ancillary* chunks is never assumable and an editor may insert one after ours, so readers assume only "before IDAT" — which is exactly what they do.
The mutation gate caught the gap: `span.chunk.end > png.len()` could be weakened to `>=` and every test still passed, because each fixture put an IDAT and an IEND after the store, so the chunk never ended at the buffer's end. `fill_c2pa` takes a `&mut [u8]`, so the exact fit is a legitimate call — the prefix of a file up to the end of its store, or a file whose store is its last chunk — and under `>=` every such call is refused.
`fill_c2pa` validated the span against itself and against the buffer's bounds, then read exactly four bytes of `png` — the chunk type — and wrote. It never compared the span with the length the chunk itself declares, so a span naming a longer chunk than the one actually there passed every guard: the payload and the recomputed CRC went over whatever followed, in practice the IDAT, and the call returned `Ok(())`. The mirror case, a span shorter than the chunk declares, planted the CRC inside the real payload, leaving a chunk `find_c2pa` then skips — the store silently not carried. Reaching it needs only a mismatched span, which the API allows by construction: `C2paSpan`'s fields are `pub`, `#[non_exhaustive]` blocks literal construction but not field assignment, and both hand-out points return owned values. Taking a span from one file and filling another is supported on purpose — the exact-fit case does it — which is precisely why the declared length has to be checked rather than assumed. No panic and no memory-safety issue: every write stayed inside the slice. But the documented contract says a span whose bytes are not a `caBX` chunk is rejected, and these bytes are not one.
`c2pa_ignored`'s docs claimed that `c2pa == None` with a non-zero count meant someone had appended a store to a file carrying none. The unit test forty lines below asserted the counter-example: two `caBX` chunks both *before* IDAT, the first over the metadata budget, gives exactly that pair with nothing appended. The converse failed too — the canonical append is a `caBX` after IEND, which is a trailer neither walk reaches, so it counted zero. A caller gating injection detection on the field got both a false positive and a false negative on the one case the sentence named. The count now covers every CRC-valid `caBX` in the datastream that was not surfaced as the store, which adds the budget-skipped store-position chunk `collect` already saw and previously passed over. First-wins is unchanged: the first chunk still claims the store position whether or not it is admitted, so an oversized store cannot be substituted by a smaller one after it. The docs — the two struct fields, STATUS.md and the README — now state the three cases it counts, that it does not distinguish them, and that a chunk after IEND is outside the datastream and therefore outside the count, with `deconstruct`'s trailer segment named as where that shape is visible instead. Both claims are pinned by tests.
The declared-length check read the length field through four indexed reads, `start`, `start + 1`, `start + 2`, `start + 3`. The mutation gate found the first of those offsets unkillable: for any payload under 64 KiB the top two length bytes are both zero, so reading one in place of the other changes nothing a fixture of that size can observe. The offsets are gone rather than papered over with a 64 KiB fixture. The eight header bytes are taken as one borrow and split where §5.3 splits them, and the length field is compared as bytes against the span's own payload length in network order — so there is no offset arithmetic left to get wrong, and a wrong split fails the type check that follows. The test gains the case the byte comparison earns: a 300-byte store, where a span agreeing only in the low byte (44) is rejected, which no single-byte length could have shown.
… 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.
`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.
The rule that a tRNS chunk may omit its trailing opaque entries (§11.3.2.1) lived inside reduce.rs's build_indexed, where only the encoder-derived palette could reach it. It is a fact about tRNS, not about that path, and a second palette path is about to need it. Move it to palette.rs as trim_trailing_opaque, beside the OPAQUE constant that names the value the rule is about, and have build_indexed call it. Behaviour is unchanged; the loop is the same loop.
`encode_indexed8` wrote the caller's palette verbatim. Unlike the palette `reduce.rs` builds, which cannot hold either by construction, a caller's may hold entries nothing in the file names and entries naming a colour an earlier entry already names. Both go into an incompressible `PLTE`, and the count of them picks the index bit depth -- a 256-entry palette holding four colours cost 768 `PLTE` bytes and pinned every pixel to 8 bits where 2 would do. `PngPalette::cleaned` drops an entry no pixel and no in-range `bKGD` index marks, merges a later entry holding the same RGB *and* the same alpha as an earlier one, trims the trailing opaque `tRNS` bytes §11.3.2.1 lets a chunk omit, and returns the old-index -> new-index map. `encode_indexed8` derives the depth from what survives, remaps the image's indices, and moves a `bKGD` palette index with the entry it names, so the background still resolves to the colour the caller chose. An entry named only by that background survives with it; an index already out of range stays out of range rather than being renumbered back in. Alpha is part of an entry's identity: two entries sharing an RGB triple but not an alpha are different colours and both survive, and an entry `tRNS` omits compares as opaque rather than as absent. Surviving entries keep the caller's relative order -- reordering is a heuristic question, filed as #612. Nothing is reported, because nothing is observable: every surviving entry keeps its bytes and the map sends each old index to the entry holding the colour it named. libpng resolving a wholly redundant palette to the caller's exact RGBA is the test of that, rather than a round trip through our own decoder, which would resolve the file through the very palette the encoder wrote. Refs #482
Axis 4 said caller-supplied palette cleanup remained; it no longer does. Give it its own section beside the cost model: what is dropped, merged, trimmed and renumbered, that the `bKGD` index moves with its entry, and that the whole pass is silent because it is lossless. The measurement is a 64x64 four-colour picture handed a full 256-entry palette: 1194 bytes before, 162 after, against 164/162 for the tight palette holding the same four colours. Both "before" figures are measured on this branch's base. The two "after" figures are equal because after cleaning the two palettes *are* the same palette, which the encoder suite pins as a byte-for-byte file equality rather than as a size; the tight palette's own 2 bytes are the `tRNS` trim this path did not previously apply. The remainder axis 4 still names is ordering -- modified-Zeng for the derived palette, and any ordering of a caller's -- which is a heuristic chosen by measurement rather than a rule the specification states. Point it at #612, which holds that question, instead of at the umbrella issue. Refs #482
`trim_trailing_opaque` popped while `alphas.last() == Some(&OPAQUE)`. Invert that comparison and the loop never ends: an emptied vector answers `None`, and `None != Some(&OPAQUE)` holds forever, so it pops an empty vector for as long as anything is willing to wait. The diff mutation gate found it — the mutant did not survive, it timed out, which is a different fact and needs the opposite repair. An exclusion would have recorded the hang instead of removing it. Compute the length to keep instead: find the last entry that is not opaque, keep everything up to and including it, truncate. Same result, no loop, and every mutant of the new form changes the length a test already asserts. Refs #482
`encode_indexed8` marked an entry used for the pixels and for a one-byte `bKGD` payload only, with a comment asserting the index was "the one background form that survives this path". It is not: `bkgd_for` also resolves a two-byte grey sample and a six-byte RGB triple against the palette under an indexed colour type, and the builders promise exactly that conversion. So an RGB or grey background naming an otherwise-unused entry lost the entry, and the chunk vanished with it. Sharper: where the triple appears both opaque and transparent, the resolver prefers the opaque entry -- if no pixel names it, cleaning dropped it and the chunk was still written, now pointing at the transparent twin. An opaque background silently turned see-through, with nothing missing from the file to show for it. Which entry a payload names is now one function, `background_entry`, asked by both callers instead of restated beside one of them: `bkgd_for` converts the chunk for the header being written, and `encode_indexed8` marks the entry it names as used. The chunk is then written as that entry's index in the cleaned palette, so the resolution is taken once, against the palette the caller supplied, rather than a second time against the cleaned one -- which keeps the answer independent of the order cleaning leaves the survivors in. Cleaning is lossless again, so it stays silent.
… once The claim corrected is the one the previous commit made false: the encoder's own doc, `STATUS.md` and the comment in `encode_indexed8` all said an *index* was the background form palette cleaning had to keep. All three now say what holds -- every form names an entry of the caller's palette, one rule with one owner, and the entry each names survives. `palette.rs` introduced `OPAQUE` for the tRNS trim while two methods two screens above still carried the literal 255 for the same §11.3.2.1 fact. They take the constant.
The diff mutation gate reported one survivor: replacing the match guard `self.ancillary.bkgd.as_deref() != Some([index].as_slice())` with `true`. It is not a gap in the tests. The guard only decided whether to skip a clone of the encoder's chunk state when the renumbered chunk would carry the bytes it already carries, so both sides of it write the same file and no assertion can tell them apart. Removing the branch removes the mutant, which is better than tolerating it: what is left is one clone on the one path that has a background at all, against a branch that bought nothing a reader could observe.
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
gamut-png'sencode_indexed8wrote the palette the caller handed it verbatim. Unlike the palettereduce.rsbuilds — which can hold neither by construction, because it is built from the pixels —a caller's palette may hold entries nothing in the file names and entries naming a colour an earlier
entry already names. Both are written into an incompressible
PLTE, and the count of them decidesthe index bit depth.
This branch cleans it.
PngPalette::cleaneddrops an entry no pixel and nobKGDbackgroundnames, merges a later entry holding the same RGB and the same alpha as an earlier one, trims the
trailing opaque
tRNSbytes §11.3.2.1 lets a chunk omit, and hands back the old-index → new-indexmap.
encode_indexed8derives the bit depth from what survives, remaps the image's indices, andmoves the
bKGDbackground onto the entry it names — in whichever of its three forms it wasset (§11.3.5.1: a palette index, a grey sample, an RGB triple), because all three name an entry of
the palette the caller supplied.
Measured on a 64×64 four-colour picture handed a full 256-entry palette (four colours repeated
sixty-four times, one of them transparent):
−86.4% on the first row. Both "before" figures were measured on this branch's base
(
9243d2e4) by encoding the same two fixtures there; the "after" figures are what the suite assertstoday. The two rows now agree because after cleaning the two palettes are the same palette, and
the suite pins that as a byte-for-byte file equality rather than as a size. The tight palette's own
two bytes are the
tRNStrim this path did not previously apply. What is bought is rarely thePLTEbytes alone: 252 dropped entries also take the index stream from 8 bits per pixel to 2.This pull request is stacked on #550 (
fix/483-png-metadata-preservation), which is unmerged.It targets
master, so GitHub's diff shows #550's commits underneath this branch's three. Read onlythe three commits above
9243d2e4; everything else belongs to #550 and must be reviewed there.The commits
refactor(png): give the trailing-opaque tRNS trim one owner— moves the §11.3.2.1 rule out ofreduce.rs'sbuild_indexed, where only the derived palette could reach it, intopalette.rsbeside the constant that names the value it is about. Behaviour unchanged; the loop is the same
loop.
feat(png): clean a caller-supplied palette before writing it—PngPalette::cleanedand itswiring into
encode_indexed8.docs(png): record what cleaning a caller's palette buys—STATUS.mdaxis 4 and a new sectionbeside the cost model.
fix(png): trim the trailing opaque alphas without a loop that can hang— the repair themutation gate's timeout asked for; see Validation.
fix(png): keep the palette entry a colour-form background names— the round-2 review finding:the used-mark asked only about the one-byte
bKGDform, so a grey or RGB background lost itsentry. Which entry a payload names is now one function,
ancillary::background_entry, asked byboth callers rather than restated beside one of them.
docs(png): say which background forms cleaning keeps, and name OPAQUE once— the three claimscommit 5 made false, and the constant's one owner.
refactor(png): renumber the background unconditionally— removes a match guard whosetruemutant survived because both sides of it write the same file.
docs(png): rewrap the bKGD paragraph to the file's column— Markdown only.What is deliberately not here
Ordering. Surviving entries keep the caller's relative order. Reordering a palette — by
modified-Zeng or by anything else — is a heuristic chosen by encoding a corpus and comparing sizes,
which is experimentation this run's own invocation excludes. Filed as
#612, which also carries the remaining
modified-Zeng work for the derived palette. Hence
Refs #482, notCloses.A notice. Cleaning reports nothing. The notice channel #550 added names what could not come
along; a merged palette entry did not fail to come along, it arrived under another index. Every
surviving entry keeps its RGB and its alpha byte for byte, and the map sends each old index to the
entry holding the colour it named, so the pixels a decoder resolves are the ones the caller supplied.
The
bKGDbackground is the case that had to be made lossless before that could stand, and round 2did: the entry the background names is kept whatever form named it, and the chunk is written as that
entry's index in the cleaned palette. Two things can still leave a background out of the file, and
neither is cleaning's doing — a colour no entry of the caller's palette holds, and an index already
past the palette's end — both of which
ancillary::bkgd_foromits against the palette as supplied,before a single entry is dropped. Nothing is left for a notice to carry.
Validation
Every command was run in this branch's worktree. Workspace-wide gates ran inside
systemd-run --user --scope --slice=agents.slice -p MemoryMax=16G -p MemorySwapMax=0withCARGO_BUILD_JOBS=2andulimit -v 12000000.mise run fmt/fmt-checkcarry the__CARGO_TEST_ROOT=$(git rev-parse --show-toplevel)prefix, without whichfmt-tooling-checkexits101 on an untouched tree in a nested worktree — an environment artefact of cargo walking past the
worktree root, not a property of this change.
cargo test -p gamut-png --all-featurescargo clippy -p gamut-png --all-targets --all-features -- -D warnings__CARGO_TEST_ROOT=… mise run fmtthen… mise run fmt-checkmise run check-testsconvco check 9243d2e4..HEADmise run lintmise run testGAMUT_MUTANTS_BASE=origin/fix/483-png-metadata-preservation mise run mutants-diffmise run fetch-av1-oracleswas run first:third_party/aomandthird_party/dav1dareupdate = none, so a recursive submodule update leaves them empty and the workspace-wide gatescannot build the AV1/AVIF oracles without them.
Every gate above was re-run in full after the fourth commit; the table is the second run, not the
first.
The mutation gate, and the base it used
Base:
origin/fix/483-png-metadata-preservation(9243d2e4), notorigin/master. This branchis stacked, and with the default base the selection folds in every mutant belonging to #550, which
is not evidence about this diff. The run prints the base it used:
mutants: selection diff base:origin/fix/483-png-metadata-preservation.The first run of that gate did not pass, and the repair is the fourth commit. It reported
18 mutants tested in 11m: 14 caught, 3 unviable, 1 timeouts, exit code 3 — and a timeout is not asurvivor; the two need opposite repairs. The mutant was
palette.rs: replace == with != in trim_trailing_opaque, and it did not time out because the suiteis slow.
while alphas.last() == Some(&OPAQUE) { alphas.pop(); }inverted becomeswhile alphas.last() != Some(&OPAQUE), and an emptied vector answersNone:None != Some(&OPAQUE)holds forever, so the loop pops an empty vector indefinitely. The loop's shape could express a
hang.
Excluding it would have recorded the hang rather than removed it. The function now computes the
length to keep — find the last non-opaque entry, keep through it, truncate — which has no loop at
all. Same result, and every mutant of the new form changes a length the existing test already
asserts. The re-run: 20 mutants, 17 caught, 3 unviable, none surviving, none timing out, in 3
minutes rather than 11.
One consequence worth naming for a reviewer: CI's own "Incremental (PR diff)" shards use the
pull request's base ref, which is
master, so they select this branch's mutants and #550's. Afailure there attributable to a file this branch does not touch belongs to #550.
No
Cargo.tomlwas touched, socheck-release-deps,check-ffi-featuresandcheck-ffi-headerare not implicated.
mise run coverageis not run: no new module was added, and every new functionis exercised by the crate's own suite.
The measurement in the table
The two "before" figures are not an estimate. The base tree was materialised from
9243d2e4, thetwo fixtures encoded there, and the sizes read off:
padded=1194 tight=164. The 2-byte gap betweenthe tight palette's before and after is exactly the two trailing opaque
tRNSentries the trim nowremoves, which is the arithmetic agreeing with the measurement.
What the gates cannot see
STATUS.md. Nothing holds a documentation sentence but thearithmetic it restates, which is why the sizes it publishes are the sizes a test asserts
(
a_redundant_palette_costs_what_the_tight_one_costspins 162) rather than numbers written onceand left to drift.
each other, so
tests/roundtrip.rswould pass through a defect symmetric across the two. Theclaim that cleaning is unobservable is therefore tested against libpng
(
tests/oracle.rs::a_cleaned_palette_still_resolves_to_the_colours_the_caller_supplied), whichresolves the file through the palette that was actually written.
indexed_round_trips_at_every_auto_depthhad to grow. Its 21×9 fixture did not name everyentry of its larger palettes, so cleaning would have dropped them and turned a bit-depth test into
a palette-cleaning test. It is now 23×13 — at least 256 pixels, so the cycling indices name every
entry, with an odd width that keeps the sub-byte depths padding their rows. The test still names
one thing.
Round 2 — after the background repair (head
ac3aba92)The table above is round 1's record and stands as written; it is not a statement about this head.
Every gate below was re-run over the repair, in the lane worktree, workspace-wide ones inside
systemd-run --user --scope --slice=agents.slice -p MemoryMax=16G -p MemorySwapMax=0withCARGO_BUILD_JOBS=2andulimit -v 12000000.cargo test -p gamut-png --all-featurescargo clippy -p gamut-png --all-targets --all-features -- -D warnings__CARGO_TEST_ROOT=… mise run fmtthen… mise run fmt-checkmise run check-testsconvco check 9243d2e4..HEADmise run lintmise run testGAMUT_MUTANTS_BASE=origin/fix/483-png-metadata-preservation mise run mutants-diffThe mutation base and the cap. Base
origin/fix/483-png-metadata-preservation, whose merge basewith this head is
9243d2e4— the run printsdiff base:origin/fix/483-png-metadata-preservation.The runner caps address space at
ulimit -v 8 GiBper process; a mutant that loops foreverallocating aborts under that cap and is scored
caught, where CI, which has no such cap, wouldreport a TIMEOUT. So this count is not by itself evidence that CI's mutation shards are green. It is
evidence about survivors, and the repair introduces no loop: the one loop this branch ever had was
removed in commit 4, and commit 7 removes a branch rather than adding one.
The first round-2 run was not green:
37 → 40 mutants, 1 missed, the survivor beingreplace match guard self.ancillary.bkgd.as_deref() != Some([index].as_slice()) with true. It wasnot a hole in the tests. The guard decided only whether to skip a clone of the encoder's chunk state
in the case where the renumbered chunk would carry the bytes it already carries, so both sides of it
write the same file and no assertion can separate them. Commit 7 deletes the branch, which removes
the mutant instead of tolerating it; the re-run is the 37/34/3 above.
No
Cargo.tomlwas touched in round 2 either, and no public type changed (background_entryispub(crate)), socheck-release-deps,check-ffi-featuresandcheck-ffi-headerremainun-implicated.
mise run coveragewas again not run: no new module, and both new tests sit in thecrate's own suite.
Added after the table: the head advanced from
ac3aba92tobfb5b26a, a Markdown-only rewrap ofone
STATUS.mdparagraph.git diff ac3aba92..bfb5b26a -- '*.rs' Cargo.toml Cargo.lockis empty, sothe Rust tree the table above was measured on is byte-for-byte the tree at the current head, and the
compiled gates carry over unchanged. Re-run over the new head all the same:
mise run check-tests(pass),
__CARGO_TEST_ROOT=… mise run fmt-check(pass),convco check 9243d2e4..HEAD— "no errorsin 8 commits". The whole-workspace
lintandtestdid run and are in the table;coverageisthe one leg of the triple this round did not take, and is reported as unverified rather than claimed.
What round 2's gates cannot see
bKGD.tooling/libpng-oracleexposes the decoded samples and theIHDR fields, not
png_get_bKGD, so the oracle cannot say which entry the background chunk names.The oracle test therefore carries the part libpng can settle — the entry survives, the depth
follows it, and all 256 pixels still resolve to the caller's own RGBA through the palette that
grew — while the chunk's own bytes are asserted beside the encoder against §11.3.5.1's read rule
(
PLTE[index], andtRNS[index]for its alpha, opaque past its end per §11.3.2.1). Extending theoracle would move that assertion; it is recorded below rather than taken, because the oracle crate
is outside this change's bounds.
pixel-level claim however hard it pushed. The remap and the surviving palette are both derived
from the same
usedset, so dropping the background's entry never disagrees with the indiceswritten beside it. The observable is the chunk, and only the chunk.
Risks and rollout
encode_indexed8now emits a different byte stream for aredundant palette: fewer
PLTEbytes, possibly a smaller bit depth, renumbered indices. Thedecoded image is unchanged, which is the claim libpng tests. A caller that asserted on the exact
encoded bytes of a redundant palette, or that assumed the written index equals the index it
supplied, will see the difference. It is not an API break and no signature changed.
bKGDis the sharp edge, and round 2's finding was on it. The chunk names a palette entry inthree forms, so an index left in the caller's numbering would silently repaint the background, and
an entry dropped from under any of the three would name a colour the file no longer holds — or, if
a transparent twin of the triple outlived it, be re-resolved onto the twin and turn an opaque
background see-through. All of it is covered
(
a_background_index_names_a_surviving_entry_after_cleaning,a_colour_background_names_an_entry_holding_its_colour,a_colour_background_keeps_its_entry_in_the_palette_libpng_resolves), as is the inverse: an index alreadyout of range must stay omitted rather than be renumbered into range by a remap that sends unmarked
indices to 0. That test starts at
palette.len()itself, the smallest out-of-range index, becausethe off-by-one that would let it through is invisible to any index further out.
survivors before it. Only the index remap walks the image — one pass, one byte per pixel, beside
the filter candidates the encoder already deflates.
documentation.
Issue
Refs #482— the issue also asks for palette ordering, which is deliberately left undone.Filed as #612: "gamut-png: order the palette by
a measured heuristic (modified-Zeng), including a caller's".
Stacked on #550; do not merge before it.
No human approved this plan. This branch was produced by an unattended run, and this record is
what a human reads afterwards.
Decisions taken
Unresolved review notes
Four questions this round did not take. None is round 2's finding; each is recorded here for a human
to decide, and none is filed as an issue, because filing one would commit the repository to work
nobody has chosen.
them.
PngPalette::cleanedbuilds aVecof surviving entries and aVecof their alphas,encode_indexed8builds a freshVec<u8>of remapped indices, and round 2 adds one more: thecaller's
PLTEis flattened a second time to resolve the background against it. All are bounded— three by 256 entries, one by the pixel count, one taken only when a background is set — and sit
beside filter candidates the encoder already deflates. What a repair would buy is fewer
allocations on a path whose cost is dominated by DEFLATE; what it would cost is the clarity of
cleanedreturning a palette rather than mutating one.palette index as an identifier outside the file — one rewriting an existing
PLTE, say — cannotask for its numbering back byte for byte. What an opt-out would buy is exactly that; what it costs
is a second code path and a builder flag on a method that has none.
PngPalette::cleanedconstructsSelfdirectly rather than throughwith_transparency, theconstructor that enforces the 1..=256-entry invariant. The invariant does hold, and
cleaned'sdoc argues why (at most 256 in, at most 256 out; the one caller marks every pixel's index, and
ImageRefrefuses an empty image). Routing it through the checked twin would buy the invariantholding by construction instead of by a paragraph, at the cost of an error path that cannot fire.
bKGD(see "What round 2's gates cannot see"). Addingpng_get_bKGDtotooling/libpng-oracle— libpng fills the returnedpng_color_16from thepalette entry for an indexed file — would let the background's resolved colour be asserted through
the reference reader rather than by reading the chunk. It would not reach the alpha, which lives
in
tRNS. The oracle crate is outside this change's bounds, so it was not attempted.