image-png icon indicating copy to clipboard operation
image-png copied to clipboard

High-level compression options API

Open Shnatsel opened this issue 1 year ago • 5 comments

Cleans up the high-level compression ratio API and makes it easier to use.

  • Makes setting Compression also automatically select the filtering mode for you
  • Removes obsolete Huffman and Rle compression options
  • Makes the Compression enum non-exhaustive so we could add e.g. Moderate for fdeflate's new mode and Extreme for Zopfli in the future
  • Renames Best to High since it isn't actually best, as its documentation already states. Especially if we add Zopfli later.
  • Renames Default to Balanced and makes it the default

Builds upon #502, so the diff includes changes from #502 when compared with main. Diff without the changes from #502 can be found here: https://github.com/Shnatsel/image-png/compare/advanced-compression...Shnatsel:image-png:high-level-compression-control?expand=1

Supersedes #490, fixes #349 and #505

Unlike #502, this is a semver-breaking change.

The exact choice of filters for each mode is debatable, but I think this is a reasonable default. I'm happy to change the mapping from the high-level Compression modes based on your input - I'm sure you have more experience with these than I do.

Update: also implements #505 in the same PR, as requested

Shnatsel avatar Sep 25 '24 16:09 Shnatsel

Looks like the expanded roundtrip fuzzing that now also covers adaptive filtering has found a bug, so that's cool

Shnatsel avatar Sep 25 '24 16:09 Shnatsel

I cannot reproduce the crash locally just by fuzzing, and I don't have the permissions to access the testcase that fuzzing has found. @HeroicKatora do you have access? If so, could you post the file on the issue tracker so I could investigate what went wrong? I'm pretty sure this is a bug that also applies to the current version, since I didn't change much about the algorithms, so it could be a significant real-world issue in existing versions.

Shnatsel avatar Sep 25 '24 18:09 Shnatsel

I left the fuzzer running locally overnight, but it failed to rediscover the issue after 1 billion executions.

Shnatsel avatar Sep 26 '24 10:09 Shnatsel

TODO: finish making adaptive filtering the default and document it as such. Needs #506 to be merged first because I don't want to create editing conflicts with myself.

Shnatsel avatar Sep 26 '24 23:09 Shnatsel

I think I've addressed all the feedback so far (good calls btw!) and also added the implementation of #505 to this PR, as you requested.

Shnatsel avatar Sep 27 '24 15:09 Shnatsel

Would this API be compatible with adding logic to automatically detect whether an image contains a small number of colors and automatically perform a palette transform for it?

That's a trick that libwebp uses to improve compression by a bit. Before encoding, they count whether the image contains 257 or more colors. If not, the consider encoding using a palette transform. According to their code comments, it is nearly always a benefit if the palette indexes fit in 4-bits (so they do it unconditionally) and is often beneficial for 8-bit palettes (so they compare the compression ratio both with and without).

fintelia avatar Dec 22 '24 22:12 fintelia

As far as the configuration goes (ignoring streaming and such not touched by this PR) - yes, absolutely.

The high-level compression options can transparently configure whether we're trying to palettize the image depending on the compression level, just like they already select the filtering mode. We can also expose an explicit control for it, separate from the advanced compression enum.

I think that would fit into this configuration API structure very well.

Shnatsel avatar Dec 23 '24 10:12 Shnatsel

I resolved the merge conflicts that arose from the API-breaking changes in master.

Now that we're doing breaking changes anyway, I'd appreciate getting this merged before the branches diverge once again.

Shnatsel avatar Dec 29 '24 18:12 Shnatsel

Thanks @Shnatsel, @kornelski, @okaneco, and @fintelia for the PR, reviews, and for merging!

Any chance that you could please push a new release/version to https://crates.io/crates/png/versions?

A new official version would help me absorb some latest functional fixes (like ExiF support) into Chromium, but I would also like to experiment a little bit with the additional compression levels - there is a weird bug (see https://crbug.com/396176046#comment11 and https://crbug.com/396176046#comment15) where Rust PNG is having trouble sticking in Chromium (a prerequisite for Dev channel field trials) because it slows down tests that run debug binaries (yes, debug! not release!).

anforowicz avatar Feb 14 '25 02:02 anforowicz

@anforowicz You can adjust your build system to enable optimizations for PNG crate even in debug mode. In Cargo you would add this to the Cargo.toml:

[profile.dev]
opt-level = 3

(opt levels 1 and 2 may also be sufficient, check if the trade-off is worth it)

Or if you have a bigger Cargo project that depends on png and you want to enable optimizations only for the PNG crate, that would be

[profile.dev.package.png]
opt-level = 3

If you're using bazel/blaze then there are probably ways to pass -C opt-level=3 to the compiler even in debug mode, but I'm not familiar with those build systems so I cannot provide the exact recipe.

Shnatsel avatar Feb 14 '25 07:02 Shnatsel