feat(png): measure the encoder end to end, then move what it exposed - #485
Open
justin13888 wants to merge 65 commits into
Open
feat(png): measure the encoder end to end, then move what it exposed#485justin13888 wants to merge 65 commits into
justin13888 wants to merge 65 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
`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.
`write_png` emitted the `Ancillary` bag verbatim whatever colour type it wrote, and auto-reduce can write a different one from the input's: the palette and colour-key candidates are raced against the unreduced encoding on compressed size, so which colour type lands is not knowable when `with_background_index` or `with_significant_bits` is called. A one-byte `bKGD` under colour type 6, or a four-entry `sBIT` under colour type 2, is a chunk libpng rejects (`png_handle_bKGD` / `png_handle_sBIT`: the length must match the colour type, an index must be inside the palette, every value must fit the depth) and silently drops. Both chunks are now resolved against the header actually written, in `ancillary::bkgd_for` and `ancillary::sbit_for`: a lossless conversion where one exists — RGBA `sBIT` loses its alpha entry, an RGB or grey background under a palette becomes the index of the entry holding it, a grey RGB triple collapses to one grey sample, and the reverse where the channels agree — and omission otherwise, including a sample or bit count the written depth cannot hold. `write_png` takes a `WrittenHeader` (colour type, depth, palette) so both writers see the same header. The pre-existing encoder test pinned a grey `bKGD` of 0x1234 under an 8-bit image — a chunk libpng drops — and an index under a truecolour file it never referred to; it now pins the same builders on colours the written file can carry, through `encode_indexed8` for the index. The libpng oracle exposes neither chunk nor a warning count, so the new integration tests assert the emitted payload against libpng's acceptance rules and decode every file through libpng; the conversion rules themselves are pinned inline.
`scan_filters` budgeted the *image* the header describes against `max_image_bytes` and then handed that figure to `inflate_zlib` as the output cap. `gamut inspect` raises the budget to a gigabyte so a 16k×16k photograph is read, and at that budget a one-megabyte PNG declaring 16384×16384 RGBA8 over a zlib stream of zeros inflates to about a gigabyte before a single filter byte is read. The walk now refuses, before inflating, a stream that would inflate to more than sixty-four times its own length — but only once the image is past the decoder's default budget, so every file the decoder inflates by default is still scanned whatever its ratio (a flat 4096×4096 RGBA8 image compresses thousands-fold and is a real PNG). The floor is stated over the header, like the budget, not over the filtered length: the two differ by one filter byte per scanline, and an image exactly at the default budget must scan. The refusal is the existing `SkippedFilterScan::OverBudget`, a statement about the reader, so `is_intact` still holds for such a file. The end-to-end test discriminates by reason: without the bound the tiny stream inflates completely and the walk reports the file's `LengthMismatch`; with it, `OverBudget` and no inflation.
The ceiling compared `segments.len()` against `DeconstructLimits::max_chunks`, and `segments` holds the signature segment too, so a file of N chunks needed `max_chunks >= N + 1` — one more than the field's own documentation says. The walk now counts the chunks materialized so far, and the boundary test in `tests/accounting.rs` pins a ten-chunk file admitted at a ceiling of ten and refused at nine, where it previously encoded the off-by-one as eleven segments.
`the_chunk_tally_does_not_slow_down_when_every_type_is_distinct` asserted a wall-clock ratio between two `deconstruct` runs inside the blocking test gate. Timing is what the gate must not depend on: under `llvm-cov` instrumentation and parallel test binaries a 20x ratio is a property of the machine's load, not of the code. The algorithmic claim — a type is found through the tally's index, never by scanning the stats — is now asserted structurally where the index is visible, inline in `deconstruct.rs`: after a mixed sequence of records the index holds exactly one entry per distinct type, each at the position of its stats entry, in first-appearance order, with the counts both arms of `record` produce. The public-side test keeps its content assertions at scale (262 144 distinct types against the same bytes with one type), drops the two `Instant` measurements, and is renamed for what it now pins. Timing belongs to `benches/`.
- `decoder.rs`: the fixture builder's note said the encoder cannot write greyscale/truecolour tRNS colour keys; it can since the colour-key reduction landed. It cannot write interlaced files, which is the reason the fixture is hand-built, and the hand-built key keeps the decoder's claim independent of `reduce`'s. - `STATUS.md`: the worst-case pass count of the nested races — 7 brute-force strategies × the palette/colour-key race × the cleanup race = 28 filter-plus-DEFLATE passes for one file — recorded against the 7 of `BruteForce` alone, with the cost-model remainder pointed at #480; the transparent cleanup named as the crate's one lossy knob; and the `bKGD`/`sBIT` resolution against the written header, cross-referenced from the metadata axis. - `deconstruct.rs`: `DeconstructLimits::max_image_bytes` and `SkippedFilterScan::OverBudget` say that a budget past the decoder's default admits larger images, not larger inflations from small files.
The module doc said the TIFF/DNG and PNG gates are "deliberately the same strength" without saying where they differ: a TIFF or DNG walk reads directories and never pixel data, so nothing in it can be declined and its verdict never depends on the reader's budget, while a PNG's verification is an inflation that can be. PNG alone therefore has a third outcome — not damaged, not verified — and exits non-zero for it distinctly. The doc now says so, records why gating on `is_intact` would make the formats symmetric in wording and asymmetric in strength, and notes that the gigabyte budget bounds the image rather than what a small file may inflate to.
This branch marked `FilterStrategy` `#[non_exhaustive]` so that a heuristic — a measurement result — can be added as the corpus grows. That is a breaking change for any downstream exhaustive `match`, and the commit that made it did not say so; `STATUS.md` now records it on the filter-selection axis, and this message carries the marker the release tooling reads. BREAKING CHANGE: FilterStrategy is #[non_exhaustive]; downstream exhaustive matches must add a wildcard arm
This was referenced Sep 6, 2026
The in-diff mutation run left one survivor: `&&` → `||` in `sbit_for`'s grey test. The negative case pinned a triple where no adjacent pair agrees, which both operators reject alike; a triple with exactly one agreeing pair separates them, so two are added — one under `Grayscale`, one under `GrayscaleAlpha`.
…d origin Two re-review findings on `bkgd_for`'s palette arm. An RGB or grey background was mapped to the *first* PLTE entry holding its triple. The encoder orders transparent entries first, and transparent cleanup zeroes every invisible pixel to (0, 0, 0, 0), so an image with opaque black carries two [0, 0, 0] entries with the transparent one ahead — and a black background named the entry a viewer never sees. `WrittenPalette` now carries the `tRNS` payload beside `PLTE`, and `index_of` prefers an entry with alpha 255, falling back to the first match only when no opaque twin exists (its RGB is still what a compositing reader paints). A caller's `with_background_index` was kept whenever the written palette held that many entries. Under auto-reduce the palette is the encoder's, in an order the caller never saw, so the index named an arbitrary entry. `WrittenPalette` now records its `PaletteOrigin`: an index is kept only on the `encode_indexed8` path, whose palette is the caller's, and omitted under a derived palette. Both pinned end to end in `tests/ancillary_colour_type.rs` — the black-on- transparent sprite reproduces the first (index 0, the transparent twin, before this change) and the two-colour checkerboard the second — and by unit tests on `bkgd_for`.
The structural test pinned the index's content — one entry per type, each at its stats position — which a `record` that scans `stats` linearly and also maintains the index satisfies unchanged, so it did not falsify the quadratic walk the index replaced. `ChunkTally` now carries a `#[cfg(test)]` probe counter that `record` increments once per entry examined (one for a hash lookup; a linear scan would have to account one per entry compared), and a new inline test asserts N chunks cost exactly N probes both with every type distinct and with a single type — the O(N) claim by count rather than by clock. Two comments corrected: the structural test's doc no longer claims a linear scan would fail it, and the at-scale public test no longer says the fixture "would not complete under the defect" — it took about 17 s; the probe count, not that test's duration, separates the two.
…mb still costs - `with_significant_bits`, `with_background_gray/rgb/index`: say that the chunk is emitted for the colour type actually written, converted where lossless and omitted without error where the written header cannot carry it, and that an index survives only against the caller's own palette. - `ancillary` module doc and `STATUS.md`: the "converted or omitted" contract holds across colour types; on the depth axis a `bKGD` sample is range-checked but not rescaled with a 16→8 demotion or sub-byte packing, which is #501. - `INFLATION_RATIO`: the worst case a few-kilobyte file can still cost, numerically — the decoder's own default exposure of 64 MiB plus one byte per scanline (64 MiB + 4 KiB for 4096×4096 RGBA8, 128 MiB for a one-pixel- wide column) — so the ratio's job is stated as stopping a raised budget, not shrinking the default one.
This was referenced Sep 6, 2026
`reduce::analyze8` collapsed five reduction candidates to one by raw estimated size, and `write_reduced_or_native` then raced only that single winner against the fully unreduced encoding. So whenever a palette won on raw bytes and lost the finished file to `PLTE`'s incompressible payload, the runner-up it had eliminated -- an alpha drop, a greyscale collapse, `analyze16`'s 16->8 demotion -- was never encoded at all and the encoder fell all the way back to no reduction. Measured at this revision: a 128x128 opaque RGBA image with 256 colours emitted 349 bytes with an alpha channel that was 255 everywhere, against 317 for the same pixels as RGB; a 64x64 RGB16 image whose every sample is `k*257` emitted 220 bytes at depth 16, against 172 for the plain demotion. `analyze8`/`analyze16` now return a `Reductions` that names the family of the estimate's winner, and hands over the best chunk-free candidate alongside a chunk-carrying one. `write_reduced_or_native` races all three -- chunk-carrying, chunk-free, unreduced -- and keeps the smallest. Ties resolve toward the earlier of `chunked > chunk-free > native`: the existing `prefers_native` tie-break (a tie keeps the palette) is unchanged, and the new `prefers_chunk_free` keeps the candidate the estimate ranked first, so an equal-length runner-up changes no output. A chunk-free *winner* still needs no race and is written straight out; only a chunk-free *runner-up* is measured, because the candidate that beat it carries a chunk. The corpus had no opaque-RGBA-with-few-colours row and no 16-bit row, which is why no gate could see this. `opaque256_rgba8` and `demotable_rgb16` are those two cases, budgeted against libpng-9 at 0.78 and 0.68 against measured 0.741 and 0.644; both would breach their budgets at the pre-fix sizes. STATUS.md's axis 3 said "done" and the cost model said "never worse than either candidate alone". Neither was true of the selection, so axis 3 is now **partial** -- what remains is the pair that both carry a chunk, a palette and a `tRNS` colour key, still resolved by the raw estimate alone -- and the worst-case pass count is 7 x 3 x 2 = 42, not 28.
`prefers_plain` gave an exact size tie to the *cleaned* encoding. `with_transparent_cleanup` is the crate's one knob that alters stored samples -- every other reduction here is byte-exact -- and it is opt-in for a size win. Where the race finds no size win there is nothing to trade that exactness for, so the tie now keeps the plain encoding, the candidate that changed no sample. `prefers_native` is untouched: a tie there still keeps the palette, which decodes with less work for the same bytes.
The counter was incremented once at the head of `record`, outside the lookup, so it read one per call whatever the lookup did with it: the inline probe test passed identically under the quadratic `stats` scan the index replaced, and the timing assertion that used to catch that is gone. Two doc comments claimed the opposite. Move the charge into a single `lookup` method, made where an entry is actually examined. A hash lookup charges one; the linear scan charges one per comparison, which takes the same test to 2 096 128 probes against the 2 048 it asserts.
262 144 chunks built two ~3.1 MB PNGs to assert a per-entry content claim that holds at any count past a handful. The complexity half of the claim belongs to the inline probe count, as this test's own doc concedes, and the fixture never failed under the quadratic walk anyway — it took about 17 s and completed. 1024 clears `synthetic_type`'s 26 and 676 rollovers, so three of the four type bytes still vary, and costs ~12 KB per half. Renamed: the size was the only thing "at scale" named.
Both the image exceeding the caller's budget and the ratio guard refusing a suspected zlib bomb reported `OverBudget`, so `gamut inspect` told a valid flat 16384x16384 RGBA8 PNG — exactly the gigabyte it budgets, not one byte over — that it was larger than the reader's byte budget, and exited 1 blaming a limit the file meets. Append `ImplausibleInflation = 4` (the discriminants are permanent and append-only; the enum is non_exhaustive) and give the CLI the accurate message. Neither refusal is damage, so `is_damage` keeps its meaning with both excluded.
IHDR is pushed before the loop and the ceiling was only checked inside it, so `with_max_chunks(N)` admitted N + 1 chunks in that one respect and `with_max_chunks(0)` admitted a whole one-chunk file. Check the ceiling where every chunk enters the report instead, so the count means what its name says at every N. The hard error stays: a file at the default ceiling is 12 MiB of pure framing.
`print_lines` delegates as `print_lines_of(label, lines, lines.len())`, and the notice fired on `total > lines.len()` — always false for that caller. A TIFF or DNG with fifty unknown tags printed the header, twenty lines, and no sign the list had been cut; the PNG caller passes a pre-truncated list with a separate true total, so it was unaffected and nothing saw the regression. Count the hidden entries from what is printed instead, which is right for both callers, and pin that in `hidden_entries`.
The contract lived only in `inspect.rs`'s module doc, where a caller scripting the command cannot find it. State it in `docs/`, indexed and normative for one thing: the two exit codes, the gate each format is judged by, PNG's third outcome and why only PNG has one, the gibibyte walk budget against the decoder's 64 MiB, and every filter-scan skip reason with whether it is damage. The module doc keeps the reasoning and points at it.
Nightly rustfmt's own layout for the two statements the previous commit introduced. No behaviour change.
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.
Refs #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.
Repair pass (repair-1,
9d7f770..fac39dc)An unattended run acted on a read-only review of
97567f5. No human approved this plan — therecord below is what a human reads afterwards. It merged
origin/master(6a75ec4) by mergecommit, then repaired exactly the five findings the review raised and nothing else.
Summary
bKGD/sBITemitted for a colour type the race did not write(
49189a6).write_pngemitted theAncillarybag verbatim, and the palette/colour-key racedecides the colour type after those chunks were set; a one-byte
bKGDunder colour type 6 or afour-entry
sBITunder colour type 2 is a chunk libpng rejects and drops. Both are now resolvedagainst the header actually written (
ancillary::bkgd_for/sbit_for): a lossless conversionwhere one exists (RGBA
sBITdrops its alpha entry; an RGB or grey background under a palettebecomes the index of its entry; a grey RGB triple collapses to one grey sample; and the reverse
where the channels agree), omission otherwise — including a sample or bit count the written depth
cannot hold.
write_pngtakes aWrittenHeaderso both writers see the same header. Sevenend-to-end tests in
tests/ancillary_colour_type.rs(six reproduced the defect at97567f5,including the reviewer's exact case; the seventh is the unchanged-colour-type control) plus the
conversion rules pinned inline.
inspect's 1 GiB budget inflated a zlib bomb (5e2807c).scan_filtershanded the caller's image budget toinflate_zlibas the cap. Past the decoder'sdefault budget the walk now refuses, before inflating, a stream that would grow to more than 64×
its own length (
SkippedFilterScan::OverBudget, sois_intactstill holds). The floor is statedover the header, not the filtered length, so the 4096×4096 RGBA8 boundary image this branch
already fixed still scans. The end-to-end test discriminates by reason:
LengthMismatchat97567f5(the tiny stream was inflated completely),OverBudgetnow.max_chunkscounted the signature segment (1851bb0). A file of Nchunks needed
max_chunks ≥ N+1. The walk counts chunks; the boundary test pins ten chunksadmitted at ten and refused at nine.
90ff376). Replaced by a structuralinline test that the tally's index names every recorded type at its
statsposition (theproperty that makes the walk O(N)); the public test keeps its content assertions at scale and
loses its
Instants.decoder.rs:1329(4517679) no longer says the encoder cannot write colourkeys.
Plus the design questions the record answers: the worst-case pass count (28) and the one lossy knob
recorded in
STATUS.md(4517679);inspect.rs's module doc states why its verification gate isPNG-only (
2c08480);docs(png)!carries theBREAKING CHANGEfooter for#[non_exhaustive] FilterStrategy(eabb0bd); one mutation survivor closed by a test (5f8e71b).Re-review of
97567f5..5f8e71bconfirmed the five findings closed (no Critical/High) andreturned three Low findings and four design questions, decided by the orchestrator (decision 19):
bkgd_for's palette arm (589df4f). An RGB background was mapped to the firstPLTE entry holding its triple; under the encoder's transparent-first ordering, transparent
cleanup gives an image with opaque black two
[0,0,0]entries with the transparent one ahead,so a black background named the entry a viewer never sees.
WrittenPalettenow carriestRNSand prefers an opaque entry. And a caller's
with_background_indexsurvived under anencoder-derived palette whose order the caller never saw; it is now kept only on the
encode_indexed8path. Both reproduced end to end before the change.e1e39b3). A#[cfg(test)]probe counter onChunkTally, incremented once per entryrecordexamines; N chunks cost exactly N probes withevery type distinct and with one type. The structural test's doc no longer claims a linear scan
would fail it, and the at-scale comment no longer says the fixture "would not complete" (it took
about 17 s).
fac39dc). The "converted or omitted" contract is stated as holdingacross colour types, with the depth axis named as gamut-png: bKGD sample values are not rescaled when auto-reduce demotes 16→8 or packs sub-byte grey #501 (
ancillary.rs,STATUS.md); thesetters say the chunk is emitted for the written colour type and omitted without error where it
cannot carry it;
INFLATION_RATIOstates the worst case numerically (the decoder's own defaultexposure: 64 MiB + one byte per scanline).
Validation (repair pass)
Run from the lane's worktree at the head named; every workspace-wide gate inside a
systemd-run --user --scope -p MemoryMax=16GwithCARGO_BUILD_JOBS=2.cargo test -p gamut-png --all-features --no-fail-fasteabb0bdcargo clippy -p gamut-png --all-targets --all-features -- -D warningseabb0bdmise run linteabb0bdmise run testeabb0bd__CARGO_TEST_ROOT=$(git rev-parse --show-toplevel) mise run fmt-checkeabb0bdmise run check-testseabb0bdmise run check-commits/convco check 97567f5..HEAD5f8e71bGAMUT_MUTANTS_BASE=9d7f770 ./tooling/mutants/run.sh --diff --budget 16eabb0bdsbit_for&&→||)cargo test -p gamut-png --all-features --lib -- ancillary5f8e71bGAMUT_MUTANTS_BASE=9d7f770 ./tooling/mutants/run.sh --diff --budget 165f8e71bcargo test -p gamut-png --all-features --no-fail-fast --lib --test ancillary_colour_type --test accountingfac39dccargo clippy -p gamut-png --all-targets --all-features -- -D warningsfac39dc__CARGO_TEST_ROOT=$(git rev-parse --show-toplevel) mise run fmt-checkfac39dcmise run check-tests·convco check 97567f5..HEADfac39dcmise run lintfac39dcmise run testfac39dcGAMUT_MUTANTS_BASE=9d7f770 ./tooling/mutants/run.sh --diff --budget 16fac39dcRUSTDOCFLAGS='-D warnings' cargo doc -p gamut-png --no-deps --all-featuresreports twopre-existing unresolved links (
backend.rs:40,deconstruct.rs:259, neither touched by thispass; CI's doctest job is green with them) and nothing from the new docs.
Not run:
check-release-deps/check-ffi-features(noCargo.tomltouched);coverage(no newmodule with little reach — the new test file and inline tests cover every added function).
Risks and rollout
bKGD/sBIToutput changes only where the previous output was a chunk readers drop: same colourtype, in-range values → byte-identical. A caller who set an index background on an image auto-reduce
does not palettise now gets no
bKGDinstead of an invalid one.IDAT is under a 64th of its filtered length (a flat 16k×16k image). It is reported
OverBudget, notdamage;
gamut inspectexits non-zero saying "not verified", as it already did for any skipped scan.max_chunksadmits one more chunk than before at a given ceiling — the documented count.docs(png)!commit makes release-plz bumpgamut-png0.1 → 0.2 — Cargo's breaking slot fora 0.x crate — for the already-pushed
#[non_exhaustive]; that is the intent.with_background_indexunder auto-reduce now yields nobKGDeven when a palette iswritten (it was an index into a palette the caller never saw); set the background as a colour to
have it resolved against whatever is written.
Issue
Refs #224 (the PR's own subject; unchanged). Filed from this pass: #500 (gamut-core byte-tiling
primitive, design question 9), #501 (bKGD values under 16→8 demotion / sub-byte packing — the depth
axis of finding 1), #502 (libpng oracle: expose bKGD/sBIT and a warning count).
Decisions taken
Appended in the same shape by the repair pass:
Unresolved review notes
crates/gamut-png/tests/oracle.rs(auto-reduce cases). Claim: the grey palette(an
Indexedwrite from aGray8input) has no end-to-end libpng pin after the earliertightening; only the 192×192 four-colour RGB case re-pins sub-byte indexed auto-reduce. Not
repaired: outside the five findings that put this entry in the order (decision 1).
crates/gamut-png/tests/size_contract.rs. Claim: the per-row byte budgets are setby judgement rather than derived from a stated model. Not repaired: decision 1 / question 7 —
accepted as measured.
crates/gamut-png/src/ancillary.rs::bkgd_for. Claim: abKGDsampleinside the written depth's range is not rescaled when auto-reduce demoted the samples (16→8,
sub-byte grey). Not repaired: the depth axis of finding 1, pre-dating the race; filed as gamut-png: bKGD sample values are not rescaled when auto-reduce demotes 16→8 or packs sub-byte grey #501.
crates/gamut-png/tests/ancillary_colour_type.rs. Claim: libpng's acceptance ofthe converted chunk is asserted from its rules, not observed through the oracle. Not repaired:
needs
tooling/libpng-oracle, outside the manifest; filed as tooling/libpng-oracle: expose bKGD/sBIT and a warning count so ancillary-chunk acceptance is oracle-testable #502.crates/gamut-png/benches/. Claim: the distinct-chunk-typesdeconstructcase(262 144 types against one) has no benchmark now that its timing left the test gate; the O(N)
claim is pinned by probe count inline, and the wall-clock figure belongs in
benches/. Notrepaired:
benches/is outside this lane's manifest.Maintainer sanction
The two sections above each state that no human approved the decisions they record. That is no
longer true as of this review; the entries below were put to the maintainer with their evidence
and answered. Everything not listed here stands as recorded.
deconstruct's 12 unconditional exportsgamut inspect's 1 GiB budget,is_verified()gate and PNG-only asymmetrydocs/#[non_exhaustive] FilterStrategyand the 0.1 → 0.2 bump it drivescrc32fast(decision 4)OverBudgetconflating the image-size and inflation-ratio refusalsSkippedFilterScanvariantmax_chunkserroring rather than truncatingbKGD/sBITomitted silently;with_background_indexdropped under a derived paletteChunkTallyprobe counterCloses #224Refs #224; axes 1, 2, 4, 6 and 8 remain open under itCloses #481is unchanged: its stated acceptance test — tighteningsprite_rgba8past theplaceholder
1.00— is met at0.99against a measured0.963.