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

Provide BufferedZopfliDeflater and allow user to pass in a custom Deflater #530

Merged
merged 18 commits into from
Jul 8, 2023

Conversation

shssoichiro
Copy link
Owner

No description provided.

@shssoichiro shssoichiro merged commit 2a59419 into master Jul 8, 2023
@shssoichiro shssoichiro deleted the Pr0methean branch July 8, 2023 02:35
@andrews05
Copy link
Collaborator

andrews05 commented Jul 8, 2023

Heh, so I guess it's a little late now, but thought I should comment here anyway as I have some concerns with this PR.
@Pr0methean

  • As noted in the comments, part of the code is "forked" from zopfli-rs. I'm not sure what's different, but surely it would be better to get the required changes submitted to the zopfli library? It doesn't seem like a great idea to keep partially forked code here.
  • There's a notable speed regression at level 0 as it no longer skips the final (redundant) compression trial.
  • The API seems a little confusing/more difficult now. You set the deflater in the options yet you must also pass in an additional deflater to the optimize function. Although this is only done for the raw API - what about the memory/file APIs?
  • Unsure of the actual benefits of this? You said the intention was to improve performance yet it didn't actually achieve that, right? The flexibility of zopfli could certainly be improved, I just think it could do with some more consideration.

@shssoichiro Hoping you see this too.

@shssoichiro
Copy link
Owner Author

If it's particularly problematic I can always revert it back out, although I think it's better if we can fix the issues with it here. Sorry, it's rough trying to keep up with all the changes coming in. Everyone's working too hard 😂

@andrews05
Copy link
Collaborator

andrews05 commented Jul 9, 2023

Hm, I do think it would be best to back it out for now. It may take some time to work through the concerns, especially if some of the changes need to happen in the zopfli library, and I don't think it should remain like this for the next release.

shssoichiro added a commit that referenced this pull request Jul 9, 2023
shssoichiro added a commit that referenced this pull request Jul 9, 2023
Pr0methean added a commit to Pr0methean/oxipng that referenced this pull request Dec 1, 2023
…later (shssoichiro#530)

* Add .whitesource configuration file

* Experimental: allow Zopfli to use any size BufWriter

* Allow user to specify the output buffer size as well

* Allow user to specify maximum block splits

* Reformat and fix warnings

* Use deflater on iCCP chunk as well

* Bug fix: need to implement Zlib format

* Make functions const when possible

* Switch to using zopfli::Options in prep for zopfli-rs/zopfli#21

* Switch to using zopfli::Options in prep for zopfli-rs/zopfli#21

* Cargo fmt

* Fix compilation

* Fix tests

* Fix more lints

* Fix more lints

* Fix compilation more

---------

Co-authored-by: mend-bolt-for-github[bot] <42819689+mend-bolt-for-github[bot]@users.noreply.github.com>
Co-authored-by: Chris Hennick <hennickc@amazon.com>
Co-authored-by: Chris Hennick <4961925+Pr0methean@users.noreply.github.com>
Pr0methean pushed a commit to Pr0methean/oxipng that referenced this pull request Dec 1, 2023
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.

3 participants