oxipng icon indicating copy to clipboard operation
oxipng copied to clipboard

Probably regression in v9.0.0 compared to v8.0.0, compression became worse

Open aleksusklim opened this issue 1 year ago • 15 comments

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

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).

aleksusklim avatar Nov 16 '23 07:11 aleksusklim

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.

andrews05 avatar Nov 16 '23 19:11 andrews05

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.

AlexTMjugador avatar Nov 17 '23 14:11 AlexTMjugador

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.

andrews05 avatar Nov 17 '23 20:11 andrews05

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.

AlexTMjugador avatar Nov 18 '23 11:11 AlexTMjugador

Unrelated to the buffer changes by @Pr0methean ...?

ace-dent avatar Nov 18 '23 13:11 ace-dent

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.

andrews05 avatar Nov 18 '23 17:11 andrews05

@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 🤨

andrews05 avatar Nov 27 '23 07:11 andrews05

@AlexTMjugador Any ideas on how we can track down the cause of this?

We could locally patch the dependency on Zopfli to use a local checkout of its source code, and then git bisect that checkout to see if any recent Zopfli commit introduced the regression.

AlexTMjugador avatar Nov 27 '23 17:11 AlexTMjugador

@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.

andrews05 avatar Nov 28 '23 03:11 andrews05

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:

AlexTMjugador avatar Nov 28 '23 18:11 AlexTMjugador

Have anybody found a culprit code so far?

aleksusklim avatar Jan 28 '24 18:01 aleksusklim

Not yet as far as I know, @aleksusklim - I've clearly been pretty busy lately :sweat_smile:

AlexTMjugador avatar Feb 03 '24 15:02 AlexTMjugador

@aleksusklim Does https://github.com/shssoichiro/oxipng/releases/tag/v9.1.0 do anything to resolve this for your test cases? There is a lot contained therein, though I'm not sure I saw anything specific to this issue specifically.

TPS avatar Apr 23 '24 01:04 TPS

Version 9.1.0 still isn't released on crates.io, so anyone wanting to test it would currently have to use this ugly workaround:

oxipng = { git = "https://github.com/shssoichiro/oxipng/releases/tag/v9.1.0" }

Pr0methean avatar Apr 23 '24 02:04 Pr0methean

Nothing has changed in v9.1.x to address this, sorry.

andrews05 avatar Apr 23 '24 20:04 andrews05