-
Notifications
You must be signed in to change notification settings - Fork 142
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
base: master
Are you sure you want to change the base?
High-level compression options API #503
Conversation
… can be evolved to set both
Looks like the expanded roundtrip fuzzing that now also covers adaptive filtering has found a bug, so that's cool |
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. |
I left the fuzzer running locally overnight, but it failed to rediscover the issue after 1 billion executions. |
/// Flate2 has several backends that make different trade-offs. | ||
/// See the flate2 documentation for the available backends for more information. | ||
Flate2(u32), | ||
// TODO: Zopfli? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be dependent on feature flags?
If they exist unconditionally, then they can either fail at run time if a dependency is not added, or we'll have to have all the deps included all the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the modes listed right now are unconditional dependencies.
I'm really not a fan of things failing at runtime due to improper build time configuration, so for Zopfli I was going to gate that enum variant behind a feature. That way we could let the API user decide whether want it and enable the feature, decide they don't want it and disable the feature, or pass on the optionality of this by exposing their own feature, which would affect this enum.
I'm happy to hear your thoughts on how an optional Zopfli dependency could be added in the future, and how can we adjust this API to make that easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are even nice rustdoc annotations to make it clear that something is only available when a certain feature is enabled.
…reproducible fuzzer errors
This reverts commit 6b06728.
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. |
…lter now that it's possible
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. |
/// Significantly outperforms libpng and other popular encoders | ||
/// by using a [specialized DEFLATE implementation tuned for PNG](https://crates.io/crates/fdeflate), |
There was a problem hiding this comment.
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.
Cleans up the high-level compression ratio API and makes it easier to use.
Compression
also automatically select the filtering mode for youHuffman
andRle
compression optionsCompression
enum non-exhaustive so we could add e.g.Moderate
for fdeflate's new mode andExtreme
for Zopfli in the futureBest
toHigh
since it isn't actually best, as its documentation already states. Especially if we add Zopfli later.Default
toBalanced
and makes it the defaultBuilds 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