feat(png): measure the encoder end to end, then move what it exposed - #485
Open
justin13888 wants to merge 45 commits into
Open
feat(png): measure the encoder end to end, then move what it exposed#485justin13888 wants to merge 45 commits into
justin13888 wants to merge 45 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
`with_transparent_cleanup` committed to the transform on the assumption that collapsing invisible pixels to one colour can only help. Measured on `palette64_rgba8`, it does not: cleaning is worth -2.3% at 32x32, **+10.7% at 128x128** and -5.2% at 256x256, with both candidates landing on the same colour type throughout. The sign is a property of the image, not of any reduction. The mechanism is that cleaning is a *transform*, not a reduction. It rewrites bytes DEFLATE was already compressing. Where the invisible pixels carry noise -- a sprite -- zeroing them is worth ~31%. Where they carry structure that continues under the transparent region, zeroing inserts a discontinuity that costs more than the collapsed palette saves. This is the same failure `6b31ab9` fixed one axis over for palettes, and it takes the same fix: encode both candidates and keep the smaller, with no tuned constant. `cleaned_or_plain` mirrors `write_reduced_or_native`, and the two per-buffer encodes are factored into `encode_alpha8`/`encode_alpha16` so all four alpha-carrying layouts race identically at both bit depths. A tie keeps the cleaned encoding, which carries less unseen data. `with_transparent_cleanup` now means "clean where it pays" and can never cost bytes. `cleanup_never_costs_bytes_on_any_corpus_row` pins that as a law over every corpus row -- it needs no constant and would have failed before this change -- and `palette64_rgba8 +clean` is the row that exercises the declining side, now measuring exactly what its uncleaned twin does.
"Everything here is produced by `cargo bench` and gated by `tests/size_contract.rs`" was true of neither half. The size table is now gated in full -- every row including `tiny_rgb8` and both `+clean` columns -- while the throughput and per-heuristic tables are reported only, because a timing assertion cannot fail a build without making it flaky, which is why CI runs the benches for compile rot alone (#437). Saying so is the point: a reader deciding whether a number is load-bearing should not have to open the test. Axis 5 was stale in both directions. Cleanup is worth 40.1% on the sprite row, not the 30% recorded before palette ordering landed, and it now applies to every alpha-carrying layout at 8 and 16 bits. It is also raced rather than assumed: on `palette64_rgba8` cleaning measures -2.3% at 32x32, +10.7% at 128x128 and -5.2% at 256x256, so the axis is only "done" because `cleaned_or_plain` keeps whichever encoding is smaller. The `choose_min_sum_abs` throughput row is dropped with the function. Its 4.5x measured a per-scanline 9 KiB `Scratch` allocation the encoder never performs, so the figure described the benchmark rather than the codec. The size and per-heuristic tables are re-measured at this revision and unchanged, which is the result worth recording for the entropy score's restatement: it is ranking-equivalent on every corpus row. The bench gains `docs/benchmarking.md`'s house phrase, naming the axes it deliberately does not measure.
real conflict and one behavioural collision, both in the palette path. **The `tRNS` trim.** master removed the `alphas.len() > 1` guard, arguing the loop cannot empty the vector because the arm only runs when some entry's alpha is not 255. This branch had rewritten the same lines to iterate the *ordered* palette. Both are kept: ordering makes master's argument stronger rather than weaker, since the entry that halts the loop is now at index 0, so it stops with at least one element left. **`indexed_at_depth_eight_is_not_bit_packed`.** master added it pinning `ColorType::Indexed` on 32 colours over 200 pixels, which this branch's `write_reduced_or_native` race declines -- correctly. That fixture's RGB stream compresses to an 82-byte file and its `PLTE` alone is 96 bytes, so the palette cannot win before a single index is written; pinning `Indexed` there asserts the defect the race exists to fix. The fixture moves to 1024 pseudo-random pixels, where the palette wins on measurement (479 against 525) and the test's actual claim -- depth 8, unpacked, 32 entries -- is preserved. This is the third test in this series to pin a colour type as a proxy for "a reduction happened". master's `PngPalette::is_empty` mutants exclusion is unrelated to the survivors closed here and carries over untouched.
The two survivors CI reported against the last push, both in code this series added. `cleaned_or_plain` inlined its comparison, so nothing pinned the tie its doc promises. `write_reduced_or_native` already had this problem and already solved it: `prefers_native` exists because two encodings of the same image cannot be made to land on exactly equal lengths by any fixture, so the tie is only assertable at the boundary. `prefers_plain` is its twin, and it keeps the cleaned encoding on a tie -- less unseen data for the same bytes. The entropy weighting needed a fixture no existing row provided. Replacing `c * log2(n/c)` with `c + log2(n/c)` leaves a score that mostly counts distinct symbols, and every vector in the suite happens to rank the same way under both. The new pair inverts: sixteen bytes split evenly between two symbols carry a full bit each, while fourteen of one symbol plus two singletons carry less information despite having *more* distinct symbols. Weighted, the concentrated row scores lower; unweighted it scores higher, because it has three log terms against two. Both verified by hand-applying the exact mutation and running the package suite. No `.cargo/mutants.toml` exclusions.
The walk took two attacker-chosen quantities on trust and conflated two different verdicts. `DeconstructLimits` makes both ceilings the caller's. `max_image_bytes` was hard-coded to the decoder's default, so "a report never allocates more than a decode would" held only against a default-configured decoder; it is now a parameter, with `deconstruct_with_limits` beside `deconstruct` and builder methods matching `PngDecoder::with_max_image_bytes`. `max_chunks` is new: a chunk costs 12 bytes of input and buys a `Segment`, plus a `ChunkStats` and an index entry for a type not seen before, so an unbounded chunk count is unbounded heap at roughly an order of magnitude over the file size -- and the chunk type is four unvalidated bytes, so the distinct-type count is chosen by the input too. Every other attacker-driven quantity in this crate already has a documented cap; this one had none. `is_verified` separates "this file was read" from `is_intact`'s "nothing is known against this file". They are not the same claim: a file whose IDAT was never inflated satisfies `is_intact` vacuously, and a corrupt zlib payload under a valid CRC is damage only the scan can see. `FilterScan::is_counted` answers the narrow question both rest on. `is_intact` keeps its meaning, which is the one a report wants; a gate wants the other. `pass_stats` now checks its running total the way `adam7::expected_stream_len` does. Bailing out only per pass let an interlaced header whose seven passes each fit `usize` but whose sum does not report all seven passes against a `filtered_len` saturated to 0 -- and `idat_ratio` then printed `0.0%` as though it were a measurement.
…dict Four cases the suite could not see. `UndefinedFilterCode` was the only skip reason with no fixture: the variant appeared in an `is_damage` assertion and a discriminant pin, but nothing drove `scan_filters` into it. Delete the `FilterType::from_code` guard and a hostile file's undefined code is counted as `None` under a bogus histogram, with every other assertion still passing. `is_verified` needs the case that separates it from `is_intact` -- an over-budget file, where nothing is known to be wrong and nothing was read. The chunk ceiling is asserted from both sides, so the cap cannot degenerate into a refusal to measure. The interlaced overflow twin covers where the two checks disagree: seven passes that each fit `usize` while their sum does not.
`gamut inspect` sells itself as an archival CI gate and exited 0 on any file whose IDAT it never inflated. At the decoder's 64 MiB budget that was every PNG past 4096x4096 RGBA8 -- an ordinary photograph -- reported `intact: yes` whatever the compressed stream contained. Chunk CRCs do not cover it: a corrupt-but-CRC-valid IDAT is exactly the damage only the scan can see. Two changes, because there were two faults. The walk's budget here is now a gigabyte rather than the decoder's default: a decoder's budget guards a decode against hostile input, while reading the file is this command's whole job, and past any real image is the right place for that line. And the gate is `is_verified`, so a file that still could not be read exits non-zero saying it was not verified, distinctly from a damaged one. `intact:` is still printed and still true -- nothing is held against such a file -- but it is no longer mistaken for a verification. Measured on a 4100x4100 RGBA8 image, past the old budget: sound, it now counts all 4100 scanlines and exits 0; with its IDAT corrupted under a valid chunk CRC, it now exits 1. Both exited 0 before. The per-chunk-type table also bypassed `MAX_LIST`, so 400k distinct types in a 4.8 MB file printed 23.6 MB of stdout, and the findings list materialized one `String` per damaged chunk before truncating at print. Both are now built under the bound they are printed under, with the true total still reported.
… dirtied `FilterStrategy` is public, re-exported through the umbrella, and gained two variants this branch -- which breaks any downstream exhaustive `match`. At 0.1.0 a minor bump is Cargo's breaking slot so nothing breaks today, and `#[non_exhaustive]` is free now and not later. It is also already the house style: the workspace uses it in 212 places, `SkippedFilterScan` and `PngReport` included. The bigram scorer wiped its whole 8 KiB bitset per candidate -- 40 KiB of memset per scanline at five candidates, independent of row length, which for an ordinary row is more work than the scoring it makes possible. `MinBigrams` is in `BRUTE_FORCE_STRATEGIES`, so `BruteForce` paid it too. The set now records the words it dirtied and clears only those: a row of n bytes touches at most n-1 of them. Byte-identical output; the `Scratch` doc no longer claims hoisting saves a cost that hoisting does not touch.
`ordered_palette` sorted by `(c[3] == 255, c[3], luma)`. The first component is monotone non-decreasing in the second over 0..=255, so it orders every pair the way `c[3]` alone already does and can never change the result -- 255 being the maximum is exactly why ordering by alpha *is* "opaque last". A tuple component no input can make load-bearing is the kind of branch this repository's mutation policy exists to keep out.
`a_greyscale_colour_key_drops_the_alpha_channel_losslessly` proves `GrayKeyed` is reachable, but its fixture wins at every size, so dropping `GrayKeyed` from `write_reduced_or_native`'s `carries_chunks` set -- emitting the keyed file without racing it -- would not change its result. Nothing else in the suite could see that member. Losing needs a thinner saving than truecolour's: the `tRNS` costs a flat 14 bytes while dropping the alpha plane saves one byte per pixel, so a mostly-opaque image is where the fixed cost wins. A quarter-width transparent border at 16x16 measures 88 bytes as `GrayAlpha8` against 97 for the key, and the encoder must emit the 88.
The cost-model table was a pre-race snapshot presented as current. Its `gamut` column (451/511/564/715) matches the shipped encoder at no size -- measured totals are 364/465/563/726 -- and it reported a flat 273-byte `PLTE`+`tRNS` at every row when a palette is emitted at only one of them. 273 is itself pre-ordering: this branch's own transparent-first ordering took the `tRNS` from 57 alphas to 8, so the palette candidate's fixed cost is 224. Worse, the 273 was repeated as the written justification for the `palette64_rgba8` budget, in the file the branch presents as carrying a measured reason per case. Retabulated from measurement, and restated to say what the three palette-less rows actually show: the raw estimate picks the palette at every one of these sizes, and the finished files disagree until 256, which is the argument for racing rather than estimating. `the_deflate_stage_accounts_for_the_residual_gap` is renamed to what it asserts. Landing on the same colour type makes `filtered_len` identical -- it is a function of IHDR alone -- but not the filtered bytes: gamut runs BruteForce while libpng runs its own heuristic, so the two compress different inputs and the ratio never isolated DEFLATE. The "smaller on every row" claim is qualified where it is a 0.2% near-tie on incompressible input, which is also the one row whose budget sits above parity and is excluded from the win assertion. The bench can now print `tie`, which `STATUS.md` recorded and the winner chain could not produce; and the module doc no longer tells the reader to pass a `--features test-support` flag that the crate's dev-dependency on itself already enables.
The rule is "maintainer-approved external crates", and the approval for this one lived nowhere outside the diff that added it. Recorded where the rule is, with what it buys and why it does not cost the crate its safety posture.
The incremental mutation gate found five survivors in the previous commits, all of them gaps in the tests rather than in the code. `is_counted` and `is_verified` were pinned only by their negative cases -- an over-budget file, which satisfies every assertion those made even when both predicates are hardcoded `false`. A verdict a gate depends on was one that could always have said no. Both now have the positive case as well. `with_max_image_bytes` was never exercised: the ceiling test only ever set `max_chunks`, so replacing the setter with `Default::default()` changed nothing, and `deconstruct_with_limits` was `deconstruct` with extra steps. A one-byte budget over an ordinary file now makes the caller's choice observable. The chunk ceiling was asserted far past the boundary, where `>`, `>=` and `==` are indistinguishable -- any file well over the limit is refused by all three. It now asserts the exact count: a file of precisely the ceiling's size is admitted, and one more is refused. Each of the five was re-applied by hand against this suite to confirm it now fails.
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.
Closes #224.
gamut-pngwas correct but unmeasured. ItsREADME.mdandSTATUS.mdboth claimed "output sizeis benchmarked against libpng at maximum compression" — and no code did either. It was one of
the few codec crates with no
benches/directory, and the encoder's only observable output was atotal byte count.
This measures every stage, records the baseline where a future change can be diffed against it,
turns the size claim into a gate, and then moves every axis the measurement exposed.
Result
gamut is smaller than libpng-9 on every corpus row, and the two hot loops got 4–33×.
gradient_rgb8photo_rgb8noise_rgb8grey_as_rgb8palette64_rgba8sprite_rgba8(+cleanup)flat_rgba8tiny_rgb8crc32filter_image/ Nonefilter_image/Fixed(Paeth)filter_image/MinSumAbsOutput is byte-identical across the speed work. No
unsafein any gamut crate.Measurement first
Commits 1–4 build the harness; every later commit quotes its own before/after from it. That
ordering is why this is one PR.
gamut_png::deconstruct— types every byte of any PNG and reports bpp, the DEFLATEstage's ratio in isolation, framing overhead, and the per-scanline filter histogram. Works on
libpng's and oxipng's files, which is what makes the comparison a measurement rather than two
encoders' self-reports. Verified on libpng's own interlaced
pngtest.png, 18 chunk typesincluding five this crate doesn't recognise.
test-supportstage seam — re-exports only, no wrapper bodies, so no coverage regions andno mutants (the rule
.cargo/mutants.tomlalready states forcrates/gamut/**).benches/encode.rs— size/bpp, where-the-bytes-went, and per-heuristic tables, plusper-stage throughput. Every column read back through
deconstruct.tests/size_contract.rs— the gate. Per-case budgets each carrying a written justification.Three findings the measurement produced
The cost model was comparing the wrong thing.
reduce::analyze8chose by raw size, whichcan't predict compressed size when one candidate's bytes are incompressible and the other's
aren't.
palette64_rgba8'sPLTE+tRNSis a flat 273 bytes DEFLATE can't touch, so gamut lostto libpng at 128×128 and won at 256×256. Two independent witnesses — the second appeared when
cleanup made the sprite palettisable, turning a 30% win into a 23% regression.
write_reduced_or_nativenow encodes both candidates and keeps the smaller, asBruteForcealready does for filters. No tuned constant.
A colour key is worth ~7–9%, not 25%. Dropping the alpha channel removes 25% of the samples,
but that plane is usually the most compressible one in the image. And it only pays on a
contiguous transparent region: with transparency scattered,
RGB+tRNSmeasured 14 886 bytesagainst plain RGBA's 14 319 and the race correctly refused it.
Entropy is never the unique winner. Bigrams wins four corpus rows by 22–32%, MinSumAbs three
by 5–6%, and Entropy none — it beats MinSumAbs twice but loses to Bigrams both times. Recorded as
a negative result in
STATUS.mdperdocs/benchmarking.md, and kept out of the brute-force setwhere it would cost a filter pass and a DEFLATE for nothing.
Worth a reviewer's attention
with_transparent_cleanupcould make files larger:on
palette64_rgba8, cleaning measured −2.3% at 32×32, +10.7% at 128×128 and −5.2% at256×256, with both candidates on the same colour type throughout. Cleaning is a transform, not
a reduction — it rewrites bytes DEFLATE was already compressing, and where the invisible pixels
carry structure rather than noise, destroying it costs more than the collapsed palette saves.
This is the same failure the cost model had, so it takes the same fix:
cleaned_or_plainencodesboth and keeps the smaller. The knob now means "clean where it pays" and can never cost bytes,
pinned by
cleanup_never_costs_bytes_on_any_corpus_row.choose_byseededbest_scorewithu64::MAXandimproved on a strict
<, so a row whose five candidates all scoredu64::MAXleftbest_bytesunwritten — and
filter_imagereuses that buffer across scanlines.Score::Entropyhit thesentinel whenever no byte repeated, which is ordinary for narrow images: a 2×1 Gray8
[1,3]encoded to a PNG whose IDAT is shorter than its image, and a 2×2
[0,0,0,1]to a valid PNGdecoding to
[0,0,0,0]. Fixed twice over —Option<u64>makeschoose_bytotal whatever anyscorer returns, and the entropy score is restated as
Σ c·log2(n/c), bounded by8n·256, so itcannot reach a sentinel at all. The restatement is ranking-equivalent: the per-heuristic table
re-measures byte-identically.
deconstruct. Per-chunk-type stats accumulated by linear scan, andchunk types are four unvalidated bytes — one distinct type per 12 bytes of hostile input made the
walk quadratic (4.8 MB → 40.9 s, reachable from
gamut inspect). Now a hash index beside thefirst-appearance-ordered
Vec. Separately, the filter-scan budget bounded the filtered streamwhile claiming to match the decoder's decoded budget; the two differ by one byte per scanline,
so a 4096×4096 RGBA8 image that decodes fine was reported damaged.
ihdr::native_bytesis nowthe single definition both sides read, so the claim holds by construction.
filtersis now a typedFilterScan.Option<FilterHistogram>collapsed four causes intoone
None, andis_intact()counted "we declined to measure" as damage.SkippedFilterScannames the reason; only
OverBudgetis not damage.which the race decouples. On 32×32 fixtures the unreduced stream genuinely wins, so the old
expectations were asserting the defect. The relaxed disjunction has since been tightened back to
an exact pin — its
COLOR_PALETTEarm was unreachable — and sub-byte indexed auto-reduce, whichlost its only cover in that edit, is re-pinned by a new 192×192 four-colour case.
rgb8_best_bruteforce's IDAT went 36 → 21 bytes whenBigrams joined the candidate set. That pin proves the codec-abi seam is inert, not that the
encoder is frozen; the change is documented in place, because an encoder change making output
larger would look identical there and would be a regression.
bigram index is read as one
u16::from_be_bytes, leaving no operator to mutate, and the entropyfilter's
c > 0follows from the restated algebra. No.cargo/mutants.tomlexclusions areadded.
FilterScan/SkippedFilterScanandPngReport::native_bytes()are new exports.PngHeadergainsPartialEq, Eq— requiredtransitively by
PngReport's own derives, not incidental.choose_min_sum_absis removed: itwas dead in the shipped crate and a wrapper body in a seam whose module doc forbids them, and its
reported 4.5× measured a per-scanline 9 KiB allocation the encoder never performs.
Axis scorecard
AtomicMinpruning and a two-tier cheap trial remain — #480Docs
docs/benchmarking.mdis new because benchmarks had no normative owner —docs/testing.mddisclaimed them by name and
docs/README.mdmakes anything unlisted non-binding. Both updated tomatch.
gamut inspectgains PNG so the accounting is reachable without writing Rust, and the rootREADME.mdtask table gains themise run bench/bench-testrows that document points at.docs/benchmarking.md's counter rule gains a row for byte-oriented pipeline stages, which iswhat the new per-stage benches are;
docs/testing.md's authority table now names the sizecontract on the
gamut-pngrow.Validation
mise run test·fmt-check·lint·check-tests·check-commits·check-ffi-features·check-release-deps— green locally, lint clean in both feature configurations.Depends on nothing. #477 (the
unsafepolicy) is independent — nothing here needed it, whichis itself a finding: since Rust 1.87,
#[target_feature]on safe fns means most SIMD needs nounsafe, andcrc32fastkeeps its own.Decisions taken
A follow-up pass (commits
7593fe5..332af8d) acted on a merge review. No human has approvedthis section — it is a record to read, amend, or revert, not an approval that was collected.
Each entry names the fork, what was taken, what was rejected and why, and the edit that reverses it.
1.
gamut inspectexited 0 on a file it never read.is_intact()includes!filters.is_damage(), and an over-budget skip is not damage — so at thedecoder's 64 MiB budget, every PNG past 4096×4096 RGBA8 was reported
intact: yesand exited 0whatever its IDAT held. Chunk CRCs do not cover this; a corrupt-but-CRC-valid IDAT is precisely the
damage the module doc says only the scan can see.
Taken: raise the walk's budget here to 1 GiB (past any real image) and gate the exit on a new
PngReport::is_verified()=is_intact()+ the scan ran.intact:is still printed and stilltrue. Rejected: leaving exit 0 (the tool advertises itself as an archival gate, so "did not look"
must not read as "looked and found nothing"); exit 1 on any skip (fails sound very large PNGs);
a third exit code (only PNG could produce it, breaking the deliberate TIFF/DNG symmetry).
Measured: a 4100×4100 RGBA8 image now counts all 4100 scanlines and exits 0; the same image with
its IDAT corrupted under a valid CRC now exits 1. Both exited 0 before.
Reverses: in
inspect.rs, gate onreport.is_intact()and drop thelimitsbinding.2.
deconstructhard-coded the decoder's budget.So "a report never allocates more than a decode would" held only against a default-configured
decoder, while
PngDecoderlets a caller change its own.Taken:
DeconstructLimits+deconstruct_with_limits, with builder methods mirroringPngDecoder::with_max_image_bytes.deconstructkeeps its signature and its defaults.Rejected: a bare
usizeparameter (no room for the second ceiling below); struct-literalconstruction (
#[non_exhaustive]forbids it across crates, which is why the builders exist).Reverses: delete the type and inline
DEFAULT_MAX_IMAGE_BYTESatscan_filters.3. Nothing capped the chunk count.
A chunk costs 12 input bytes and buys a
Segment— measured ~11× amplification (48 MB input →522 MB RSS) — in a crate that caps every other attacker-chosen quantity.
Taken:
max_chunks, default 2²⁰; past it the walk returnsInvalidInput. A PNG at the ceilingcarries ≥12 MiB of pure framing, which no real file does.
Rejected: no cap (the growth being linear is not a defence when the constant is 11×); truncating
the walk instead of erroring (it would break the every-byte tiling law, which is the report's
headline invariant).
Reverses: drop the
segments.len() > limits.max_chunkscheck.4.
crc32fasthad no approval on record. — the user approved this dependency explicitly.Recorded in
AGENTS.mdwhere the "maintainer-approved external crates" rule lives.Reverses: revert
332af8d.5.
FilterStrategywas public, non-sealed, and gained two variants.That breaks a downstream exhaustive
match. At0.1.0a minor bump is Cargo's breaking slot, sonothing breaks today — and
#[non_exhaustive]is free now and not after 1.0.Taken: seal it; it is already the house style (212 uses in-workspace).
Rejected: leaving it open and recording a post-1.0 posture — the fix costs nothing now.
Reverses: drop the attribute.
6.
Closes #482closed an issue this PR leaves two-thirds undone.#482 names palette ordering (done here),
encode_indexed8caller-path dedupe/unused-entryremoval/depth re-derivation, and
PngPalette::trns()trimming on the caller path. The last two areuntouched, as the scorecard itself says.
Taken: demote to
refs #482so the issue stays open carrying its remainder. No new issue isfiled, because #482 already tracks exactly that scope — a second one would duplicate it.
Rejected: implementing the caller path (well beyond this PR's scope); closing it and filing the
remainder (needlessly splits one tracked axis in two).
Reverses: restore
closes #482in the axis-4 row.Findings fixed alongside
pass_statsbailed out only per pass whileadam7::expected_stream_lenalso fails on the seven-pass sum, so a header whose passes each fit
usizereported seven passesagainst a
filtered_lensaturated to 0 — printed as a0.0%ratio.row length. It now clears only the words the row dirtied. Byte-identical output.
c[3] == 255) that is monotone in the component after it and so cannever change the order.
SkippedFilterScan::UndefinedFilterCodehad no fixture drivingscan_filtersinto it; the
GrayKeyedmember ofcarries_chunkshad no case where the key loses. Both arenow pinned, the second at a measured 88-vs-97 bytes.
the_deflate_stage_accounts_for_the_residual_gapnever isolated DEFLATE — same colour typemakes
filtered_lenequal, not the filtered bytes, since the two encoders choose differentfilters. Renamed to what it asserts.
gamutcolumn matchedthe encoder at no size (451/511/564/715 against a measured 364/465/563/726), and its flat
"273-byte
PLTE+tRNS" was a pre-ordering figure — this branch's own ordering made it 224 — thatwas also the written justification for the
palette64_rgba8budget. The bench can now print thetieSTATUS.md recorded, and the "smaller on every row" claim is qualified on the incompressiblerow, which is the one budget deliberately set above parity.
Repair pass (
97567f5)The incremental mutation gate rejected the first push with five survivors, all in the code these
commits added — test gaps, not code defects, and each fixed by a test rather than by weakening
anything:
FilterScan::is_countedandPngReport::is_verifiedwere pinned only by their negative cases.An over-budget file satisfies every assertion those made even with both predicates hardcoded
false, so the verdict the CLI gate depends on could always have said no.DeconstructLimits::with_max_image_byteswas never exercised — the ceiling test only ever setmax_chunks— so replacing the setter withDefault::default()changed nothing.>,>=and==all refuse alike. It now asserts the exact count: a file of precisely theceiling's size is admitted, one more is refused.
Each of the five was re-applied by hand against the suite to confirm it now fails, rather than
assuming a new test was sufficient.
Validation
mise run test(whole workspace, 0 failed) ·fmt-check·lint·check-tests·check-ffi-features·check-release-deps·convco check— all green locally.CI green at
97567f5: Format & Metadata, Clippy & Doctests, Coverage (test gate), and all fourIncremental (PR diff)mutation shards.Full workspaceis skipped on pull requests by design andis informational only.
Not run:
check-crossandcheck-msrv, which the Extended workflow runs post-merge on master bydesign — worth knowing because this branch adds a runtime dependency (
crc32fast, MSRV 1.63against the workspace's 1.92) and a new bench, which CI compiles here but first executes on
master.