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

High-level compression options API #503

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

Shnatsel
Copy link
Contributor

@Shnatsel Shnatsel commented Sep 25, 2024

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
Copy link
Contributor Author

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

@Shnatsel
Copy link
Contributor Author

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
Copy link
Contributor Author

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

src/common.rs Outdated Show resolved Hide resolved
@Shnatsel
Copy link
Contributor Author

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.

src/common.rs Outdated Show resolved Hide resolved
src/common.rs Outdated Show resolved Hide resolved
src/common.rs Outdated Show resolved Hide resolved
src/common.rs Outdated Show resolved Hide resolved
src/common.rs Show resolved Hide resolved
src/encoder.rs Outdated Show resolved Hide resolved
src/common.rs Show resolved Hide resolved
@Shnatsel
Copy link
Contributor Author

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.

Comment on lines +323 to +324
/// Significantly outperforms libpng and other popular encoders
/// by using a [specialized DEFLATE implementation tuned for PNG](https://crates.io/crates/fdeflate),
Copy link
Contributor

@okaneco okaneco Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A link backing up these claims would be nice here and in the new updated readme. It requires a bit of digging and research on the reader's part to confirm this.

src/filter.rs Outdated Show resolved Hide resolved
src/filter.rs Outdated Show resolved Hide resolved
@Shnatsel Shnatsel added this to the 0.18 milestone Dec 22, 2024
src/common.rs Outdated
///
/// Flate2 has several backends that make different trade-offs.
/// See the flate2 documentation for the available backends for more information.
Flate2(u32),
Copy link
Contributor

@fintelia fintelia Dec 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After more experimentation with adding higher compression levels to fdeflate, I'm pretty confident that we'd be able to expose "standard" compression levels ranging from 1 to 9 for any alternative we switch to.

One aspect is that the various different flate2 backends actually have a somewhat significant amount of variability about what each compression level means, so we wouldn't have to worry too much about exactly calibrating to match existing behavior. But additionally, these algorithms have like 5 to 10 different knobs to tune. So the challenge is really condensing them into a single number (which you want to do regardless, because users won't want to deal with setting all the parameters!)

Long story short, I think we should drop the Flate2 naming and just call this Level or something to that effect. And perhaps switch to a u8 given that values above 9 have no impact.

Copy link
Contributor Author

@Shnatsel Shnatsel Dec 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TL;DR: since this is the advanced API, not the high-level one, I'd prefer to keep it Flate2 and also keep the documentation about the way it's implemented. We would add a deprecation notice to it if we ever start emulating it like we did with Huffman and Rle.

I am going to calibrate wondermagick for specific compression ratios against a known reference with a specific flate2 backend, zlib-rs. If/when the underlying implementation changes away from flate2, I would much rather get a warning from the compiler and a deprecation message saying that it's now emulated so that I could re-evaluate the tagets for the new backend and adjust the mapping on my end, as opposed to the targets silently changing.

Knowing that flate2 is no longer used would also let me drop the direct flate2 dependency from my crate that I added to select the backend. Although perhaps png should just expose a zlib-rs feature and not have users add a direct flate2 dependency at all? And we would just make it a no-op feature if we ever move away from it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On u32 vs u8: originally I chose to stick to the flate2 API which used u32, but you're right - it makes no sense and just confuses people. I've switched it over to u8.

@fintelia
Copy link
Contributor

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

@Shnatsel
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Enabling uncompressed PNG support
4 participants