diff --git a/crates/didbot-avatar/src/png.rs b/crates/didbot-avatar/src/png.rs index d86dc53b..1338fdfc 100644 --- a/crates/didbot-avatar/src/png.rs +++ b/crates/didbot-avatar/src/png.rs @@ -267,7 +267,10 @@ fn chunk(out: &mut Vec, kind: &[u8; 4], data: &[u8]) { /// PNG filters each row separately and says which filter it used in a leading /// byte. The heuristic is the specification's own: sum the filtered bytes as /// signed magnitudes and take the smallest, which stands in for how well the -/// row will compress. +/// row will compress. `min(v, -v)` on a `u8` is that magnitude — the byte +/// itself when it reads as positive, its negation when it reads as negative — +/// and getting it wrong does not corrupt anything, it just picks worse +/// filters, which is a cost paid on every avatar the deployment ever stores. fn filter_row(row: &[u8], above: &[u8], stride: usize, out: &mut Vec) { let mut best: Option<(u32, u8, Vec)> = None; for kind in 0u8..3 { @@ -283,7 +286,7 @@ fn filter_row(row: &[u8], above: &[u8], stride: usize, out: &mut Vec) { } let score: u32 = filtered .iter() - .map(|byte| u32::from(byte.wrapping_add(128).min(byte.wrapping_neg()))) + .map(|byte| u32::from((*byte).min(byte.wrapping_neg()))) .sum(); if best.as_ref().is_none_or(|(seen, _, _)| score < *seen) { best = Some((score, kind, filtered)); @@ -459,6 +462,27 @@ mod tests { assert!(flat.len() < 700, "a flat run compressed to {}", flat.len()); } + /// The filter heuristic has to compare true magnitudes. A version that + /// scored a residual of `+1` above one of `-3` would take the worse + /// filter every time a row's ink ran lighter than the row above it, and + /// nothing downstream would complain — the image still decodes, it is + /// just bigger. + #[test] + fn the_filter_heuristic_ranks_by_magnitude_rather_than_by_sign() { + // One pixel wide, so the choice is between leaving the row alone and + // subtracting the row above it. Untouched, the row is `+1` a channel; + // filtered against the row above, it is `-3` a channel. The first is + // the smaller residual and has to be the one taken. + let mut out = Vec::new(); + filter_row(&[1, 1, 1], &[4, 4, 4], 3, &mut out); + assert_eq!( + out[0], 0, + "took filter {} over the smaller residual", + out[0] + ); + assert_eq!(&out[1..], &[1, 1, 1]); + } + #[test] fn the_checksums_are_the_ones_the_formats_define() { // Both are pinned to values from outside this crate: adler32 of