Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
aleksusklim opened this issue Nov 16, 2023 · 15 comments · May be fixed by #595
Open

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

aleksusklim opened this issue Nov 16, 2023 · 15 comments · May be fixed by #595
Labels
T-Compression Relates to improving the compression ratio of images

Comments

@aleksusklim
Copy link

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

@AlexTMjugador AlexTMjugador added the T-Compression Relates to improving the compression ratio of images label Nov 16, 2023
@andrews05
Copy link
Collaborator

andrews05 commented Nov 16, 2023

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.

@AlexTMjugador
Copy link
Collaborator

AlexTMjugador commented Nov 17, 2023

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.

@andrews05
Copy link
Collaborator

andrews05 commented Nov 17, 2023

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.

@AlexTMjugador
Copy link
Collaborator

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

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.

@ace-dent
Copy link

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

@andrews05
Copy link
Collaborator

andrews05 commented Nov 18, 2023

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
Copy link
Collaborator

@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
Copy link
Collaborator

AlexTMjugador commented Nov 27, 2023

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

@andrews05
Copy link
Collaborator

@AlexTMjugador Narrowed it down to this commit: zopfli-rs/zopfli@90cfdd4
On rust 1.72 it doesn't have any impact on output. But on rust 1.74 it's somehow different.

@AlexTMjugador
Copy link
Collaborator

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 😂

@aleksusklim
Copy link
Author

Have anybody found a culprit code so far?

@AlexTMjugador
Copy link
Collaborator

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

@TPS
Copy link

TPS commented Apr 23, 2024

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

@Pr0methean
Copy link
Contributor

Pr0methean commented Apr 23, 2024

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" }

@andrews05
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Compression Relates to improving the compression ratio of images
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants