fix: carry propagation drops bytes instead of zeroing them

[stephane-segning]

Fixes #191.

The bug

add_one_to_output propagates an arithmetic-coder carry backwards through already-written
0xFF bytes, turning each into 0x00 until it reaches one that can absorb the +1 — the
same rule as decimal 999 + 1 = 1000.

The current loop pops bytes and only pushes one back:

while let Some(value) = self.writer.pop() {
    if value < 255 {
        self.writer.push(value + 1);
        break;
    }
}

A popped 0xFF is not < 255, so it is discarded rather than written back as 0x00.
The output is shortened by however many 0xFF bytes were in the carry chain.

That is worse than a wrong byte. A decoder reading a stream that is short by N bytes does
not error — it silently reads everything after the carry shifted, so every symbol from that
point on is wrong. Which is why #191 presented as corrupted output at one specific quality
rather than as a failure.

Why it has stayed latent

It only fires when a carry actually has to cross a saturated byte, which needs a long enough
run of near-maximal bottom values. The encoder on main writes very little through this
path — a single fixed DC/DC luma mode, and mb_no_skip_coeff always disabled — so the
condition is essentially never met.

I hit it in a fork that added RD intra mode decision and skip signalling, which write far
more entropy-coded data per macroblock. It reproduced on kodim02 at lossy_quality = 85
and only there, which is what made it worth chasing rather than dismissing as noise.

So this is a real bug in main today, just one the current encoder rarely provokes. It gets
easier to hit as the lossy encoder does more.

The fix

Walk the buffer in place instead of popping:

for value in self.writer.iter_mut().rev() {
    if *value == 255 {
        *value = 0;
    } else {
        *value += 1;
        return;
    }
}

Length is preserved by construction.

Test

test_carry_propagates_through_saturated_bytes covers a carry crossing three saturated
bytes, a carry with no chain, and an all-saturated buffer where the carry falls off the
front (unreachable given the encoder's invariants, but it pins the property that the buffer
keeps its length rather than being emptied).

Verified it actually catches the bug — against the old loop it fails with:

assertion `left == right` failed
  left: [0, 11]
 right: [0, 11, 0, 0, 0]

Three bytes gone, which is the whole defect.

Checks

cargo test passes (31 + 31). cargo fmt --check is clean. cargo clippy --all-targets -- -D warnings reports 16 findings both before and after this change — all pre-existing,
none in the touched function.

Happy to adjust the comment length if it is more than you want inline; the mechanism seemed
worth recording next to the code given how long it went unnoticed.

[mstoeckl]

This change brings the code in line with the VP8 spec, and so the change to the logic looks reasonable to me. (not a maintainer of image-webp.)

https://www.rfc-editor.org/rfc/rfc6386.html#page-19

If the incoming boolean val to be encoded is false, we leave the left
interval endpoint bottom alone and reduce range, replacing it by
split. If the incoming val is true, we move up the left endpoint to
bottom + split, propagating any carry to the already-written value w
(this is where we need the ability to add 1 to w), and reduce range
to range - split.

https://www.rfc-editor.org/rfc/rfc6386.html#page-21

   /* Encoding very rarely produces a carry that must be propagated
      to the already-written output.  The arithmetic guarantees that
      the propagation will never go beyond the beginning of the
      output.  Put another way, the encoded value x is always less
      than one. */

   void add_one_to_output(uint8 *q)
   {
     while (*--q == 255)
       *q = 0;
     ++*q;
   }

Happy to adjust the comment length if it is more than you want inline; the mechanism seemed
worth recording next to the code given how long it went unnoticed.

I think the commit message suffices and the comment is actually safe to drop entirely. arithmetic_encoder.rs was documented as // currently just a direct translation of the encoder given in the vp8 specification, which is now correct; and anyone seeking to understand the logic (for example, to be certain that the carry never overflows past the first byte of self.writer) or optimize it further should look at the VP8 spec. The history of this bug probably won't help anyone make changes in the future.

Also, as for the tests: while these would identify a regression of the current very specific bug, I don't think they will catch the next one, and I can't see this change being reverted as likely. It's a typical issue seen in AI-generated tests. The most useful thing to catch regressions in the encoder or decoder logic in the future would perhaps be a fuzzer to verify the property that the arithmetic encoding output can always be decoded to the same series of decisions; but that's a more significant project.