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

Encoding should use miniz_oxide as well #242

Closed
astei opened this issue Aug 2, 2020 · 8 comments
Closed

Encoding should use miniz_oxide as well #242

astei opened this issue Aug 2, 2020 · 8 comments

Comments

@astei
Copy link

astei commented Aug 2, 2020

#218 brought some welcome performance improvements by changing decoding to use miniz_oxide. I think it'd be good if encoding were also done with miniz_oxide as well, to both improve performance and decrease binary size (important to me as I deploy this in a WebAssembly module).

@scurest
Copy link
Contributor

scurest commented Jan 23, 2021

I made a branch's that replaces deflate with miniz_oxide to see what effect this would have.

I only tested a little. Initial observations are:

  • small compile time improvement
  • no significant binary size improvement
  • encode times appear to be comparable. It's somewhat difficult to compare since it depends on the compression settings. If I tweak the compression level to make my output PNG filesize close to master, the encode times are close too; miniz_oxide might actually be a little slower. (But that also might be because I've used it in a sub-optimal way....)

Anyway this experiment was not so successful as to convince me to pursue it further.

@Craig-Macomber
Copy link
Contributor

I can't see @scurest 's branch, but I made a similar one, this time replacing deflate with flate2 (which defaults to using miniz_oxide).

https://github.com/Craig-Macomber/image-png/tree/flate2 (I didn't know how to handle the huffman and rle compression levels, but otherwise the change was trivial)

This lead me to observe that flate2 depends on libc, even when using miniz_oxide, which is tracked by rust-lang/flate2-rs#274 . I'll take a look at that.

I think if that gets fixed, my changes might be a win as far as number of dependencies and build time goes. Until then, maybe its progress toward taking more actively maintained and popular dependencies (flate2 seems to be well supported, and in this case mostly used miniz_oxide which we already depend on).

An interesting approach might be to direct all access to miniz_oxide through flate2, which would allow the user to select a compression back-end via flate2's features.

@Craig-Macomber
Copy link
Contributor

flate2 no longer depends on libc making using it to reduce dependencies here actually make sense. I updated my branch accordingly.

@fintelia
Copy link
Contributor

fintelia commented Aug 3, 2022

@Craig-Macomber could you make a PR with your changes?

@Craig-Macomber
Copy link
Contributor

@Craig-Macomber could you make a PR with your changes?

Done. #350 350

@yisibl
Copy link

yisibl commented Oct 31, 2022

Hi, after I upgraded to 0.17.6, the size of the generated PNG file is much bigger, is there any way to fix it?

thx/resvg-js@325924a#diff-aceed9dd71f2a53f7ad24cb97fc3c387bd1726d239591f9b85a11d52ade9ea9c

@fintelia
Copy link
Contributor

@yisibl we were not anticipating compression to get worse. Could you create a new issue with more details including the compression settings you're using?

@AndrewKvalheim
Copy link

Also found this issue after updating and noticing a slight increase in IDAT size. In my case I’m encoding a few thousand small PNGs.

$ du --apparent-size --bytes --total $before/**/*.png | tail -n1
2198049	total
$ du --apparent-size --bytes --total $after/**/*.png | tail -n1
2457872	total
$ diff -u0 <(pngchunks $before/$sample) <(pngchunks $after/$sample)
--- /proc/self/fd/12	2022-12-19 14:12:44.668940342 -0800
+++ /proc/self/fd/13	2022-12-19 14:12:44.668940342 -0800
@@ -18 +18 @@
-Chunk: Data Length 5546 (max 2147483647), Type 1413563465 [IDAT]
+Chunk: Data Length 5638 (max 2147483647), Type 1413563465 [IDAT]
@@ -21 +21 @@
-  Chunk CRC: 1698423009
+  Chunk CRC: 1577502878

Doesn’t really matter for my use case, just FYI.

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

No branches or pull requests

6 participants