Repository: shssoichiro/oxipng (language: rust)

Issue:
Probably regression in v9.0.0 compared to v8.0.0, compression became worse
This is the command that was used to convert an album of anime screenshots:
`oxipng.exe --verbose --threads 8 --alpha --force --interlace 1 -f 0-9 --zopfli --strip safe --preserve *.png`

Here is the output from new `oxipng-9.0.0-x86_64-pc-windows-msvc.zip`:

```
Processing: HenSuki.png
    1920x2290 pixels, PNG format
    8-bit RGB, non-interlaced
    IDAT size = 9382252 bytes
    File size = 9384075 bytes
Reducing image to 8-bit RGB, interlaced
Trying: Brute
Found better combination:
    zc = zopfli  f = Brute     6534426 bytes
    IDAT size = 6534426 bytes (2847826 bytes decrease)
    file size = 6534517 bytes (2849558 bytes = 30.37% decrease)
6534517 bytes (30.37% smaller): HenSuki.png
```

And here is the output from old `oxipng-8.0.0-x86_64-pc-windows-msvc.zip`
```
Processing: HenSuki.png
preserving metadata: Some(Metadata { file_type: FileType(FileType { attributes: 32, reparse_tag: 0 }), is_dir: false, is_file: true, permissions: Permissions(FilePermissions { attrs: 32 }), modified: Ok(SystemTime { intervals: 133445895491882136 }), accessed: Ok(SystemTime { intervals: 133445907278923468 }), created: Ok(SystemTime { intervals: 133445907278873930 }), .. })
    1920x2290 pixels, PNG format
    3x8 bits/pixel, RGB (non-interlaced)
    IDAT size = 9382252 bytes
    File size = 9384075 bytes
Reducing image to 3x8 bits/pixel, RGB (interlaced)
Evaluating: 10 filters
Trying: Brute
    zc = 0  f = Brute     6468101 bytes
Found better combination:
    zc = 0  f = Brute     6468101 bytes
    IDAT size = 6468101 bytes (2914151 bytes decrease)
    file size = 6468192 bytes (2915883 bytes = 31.07% decrease)
attempting to set file times: atime: FileTime { seconds: 13344590727, nanos: 892346800 }, mtime: FileTime { seconds: 13344589549, nanos: 188213600 }
Output: HenSuki.png
```
I am attaching this image along with its compressed versions in zip here:
[HenSuki.zip](https://github.com/shssoichiro/oxipng/files/13373412/HenSuki.zip)


This problem is happening with other similar images too, which were not specifically crafted to trick oxipng. Actually, the new version was checked against images that were already compressed by the old version to see if it would improve the compression ratio, but instead of `0 bytes decrease`/`0.00% decrease` it showed `larger`/`increase`.

You can find the full album compressed with the old version here:
https://telegra.ph/HenSuki-03-08
The problem happens on all images except for one (that depicts a letter and has large similarly-colored areas).


Discussion / hints:
Hi @aleksusklim 

This seems quite strange, I ran that same command on oxipng 9 on aarch64-apple-darwin and it came to 6468192, the same as you showed for v8.
@AlexTMjugador Any idea what might cause a regression in just the windows build?

[edit] Scratch that, I downloaded the v9 binary from releases (instead of building myself) and got the larger 6534517 result. Something strange may be happening in the build workflow? Seems to be specific to zopfli - using libdeflate I get the same result from both my own build and the release binary.
I can reproduce the issue when generating and running the v9 binary on Linux with `git checkout e1db84f && cargo run --release -- --verbose --threads 8 --alpha --force --interlace 1 -f 0-9 --zopfli --strip safe --preserve HenSuki.png` (`e1db84f` is the commit the release was made from). It does not seem related to CI modifying anything about the executable. I get the same results as OP reported.

The latest Zopfli update has been careful to not change how the same input with the same configuration parameters is compressed. Given that there were no other relevant changes since `e1db84f`, my guess is that OxiPNG happened to hand-off Zopfli a PNG datastream that somehow compressed better. Digging deeper into this will probably require some bisection and detailed inspection.
Weird, I just ran that exact command (now on x86_64-apple-darwin, 1.66.0-nightly) and got the smaller 6468192 result.

[edit] Aha! Upgrading to latest rust now gives me the larger result. pngcheck shows the filters are identical, so it seems the toolchain is somehow affecting Zopfli's behaviour.
> Aha! Upgrading to latest rust now gives me the larger result. pngcheck shows the filters are identical, so it seems the toolchain is somehow affecting Zopfli's behaviour.

Wow, I really hope Zopfli is not relying on some sort of undefined behavior then... :sweat_smile: 

Do you remember what Rust version did not exhibit this problem? Both me and CI used nightly Rust to compile binaries, so that may explain why I was not able to reproduce it.
Unrelated to the buffer changes by @Pr0methean ...?
> Do you remember what Rust version did not exhibit this problem? Both me and CI used nightly Rust to compile binaries, so that may explain why I was not able to reproduce it.

I was on 1.66 nightly before I upgraded. (I think my arm machine was slightly newer, but can’t check right now)

[edit] The change appears to be in 1.74 which gives the larger result, vs 1.72 which gives the smaller.
@AlexTMjugador Any ideas on how we can track down the cause of this?

I found a smaller (faster) test case: issue-29.png also shows a small increase between 1.72 and 1.74.
Some files such issue-553.png actually got smaller 🤨
> @AlexTMjugador Any ideas on how we can track down the cause of this?

We could locally [patch the dependency](https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html#the-patch-section) on Zopfli to use a local checkout of its source code, and then [`git bisect`](https://git-scm.com/docs/git-bisect) that checkout to see if any recent Zopfli commit introduced the regression.
@AlexTMjugador Narrowed it down to this commit: https://github.com/zopfli-rs/zopfli/commit/90cfdd4c085bb87d55d3cb54b0ed0bdd21dde7cd
On rust 1.72 it doesn't have any impact on output. But on rust 1.74 it's somehow different.
Thanks for bisecting @andrews05! I hope I'm able to take a more in-depth look at what's going on with that commit later this week. I totally did not expect safe code for this straightforward change to cause this regression :joy:
Have anybody found a culprit code so far?
Not yet as far as I know, @aleksusklim - I've clearly been pretty busy lately :sweat_smile: 

Fix patch:
diff --git a/src/deflate/zopfli_oxipng.rs b/src/deflate/zopfli_oxipng.rs
index cb6e6eb4..b77e3164 100644
--- a/src/deflate/zopfli_oxipng.rs
+++ b/src/deflate/zopfli_oxipng.rs
@@ -8,7 +8,9 @@ pub fn deflate(data: &[u8], iterations: NonZeroU8) -> PngResult<Vec<u8>> {
         iteration_count: iterations.into(),
         ..Default::default()
     };
-    match zopfli::compress(options, zopfli::Format::Zlib, data, &mut output) {
+    // Since Rust v1.74, passing &[u8] directly into zopfli causes a regression in compressed size
+    // for some files. Wrapping the slice in another Read implementer such as Box fixes it for now.
+    match zopfli::compress(options, zopfli::Format::Zlib, Box::new(data), &mut output) {
         Ok(_) => (),
         Err(_) => return Err(PngError::new("Failed to compress in zopfli")),
     };


Test patch: