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

Use Flate2 for encoding #350

Merged
merged 5 commits into from
Aug 5, 2022
Merged

Conversation

Craig-Macomber
Copy link
Contributor

Thus uses flat2 for encoding instead of the deflate crate.

Since flat2 defaults to using miniz_oxide, this shares the same underlying compression library as the decoding.

See issue #242

I'm not sure this is the right solution, and it does not full handle all the compression options (Huffman and Rle just use "fast" mode).

Cargo.toml Outdated Show resolved Hide resolved
src/encoder.rs Outdated Show resolved Hide resolved
@fintelia fintelia requested a review from HeroicKatora August 5, 2022 05:03
Copy link
Member

@HeroicKatora HeroicKatora left a comment

Choose a reason for hiding this comment

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

Looks okay. I don't particularly like the deprecation but it seems harmless enough. (Note: if we had a compression level 'plaintext'—or as deflate calls it's unimplemented option: _ForceStored—and this would have removed it, I would have vetoed; but alas, we do not). Definitely a maintenance win.

@HeroicKatora HeroicKatora merged commit fccaf2e into image-rs:master Aug 5, 2022
@fintelia
Copy link
Contributor

fintelia commented Aug 5, 2022

In fact, this change actually allows us to add a plaintext compression level because flate2 does support it!

@oyvindln
Copy link
Contributor

oyvindln commented Aug 5, 2022

Looks okay. I don't particularly like the deprecation but it seems harmless enough. (Note: if we had a compression level 'plaintext'—or as deflate calls it's unimplemented option: _ForceStored—and this would have removed it, I would have vetoed; but alas, we do not). Definitely a maintenance win.

If there is a need for RLE and huffman only it should be pretty easy to add support for it to flate2 as it's already supported by the back-ends I think. (RLE maybe via a window size 1 if there is no explicit option for it). Not sure where one would want to use the huffman only variant though, it doesn't have much purpose, RLE may have a slight use for compression speed I suppose.

@HeroicKatora
Copy link
Member

Don't feel compelled to do so based on this singular data point, I don't quite see their purpose either. No users wants RLE-only (unless they targeted non-compliant decoders for some reason?)—they want speed. If anything then the encoder should just switch to rle-only based on the speed setting. If it's actually so much faster.

@fintelia
Copy link
Contributor

fintelia commented Aug 6, 2022

The fastest approach is probably going to be fpnge's strategy of using specially constructed Huffman tables so that the resulting codes are conducive to vectorized encoding. However, I think it does bake in some assumptions about the kind of filters that PNG uses, so it might not be suited to a generic compression library.

yisibl added a commit to thx/resvg-js that referenced this pull request Oct 31, 2022
Upgrading to 0.16.0 will cause the generated PNG files to become larger.
See image-rs/image-png#350
yisibl added a commit to thx/resvg-js that referenced this pull request Oct 31, 2022
Upgrading to 0.17.6 will cause the generated PNG files to become larger.
See image-rs/image-png#350
yisibl added a commit to thx/resvg-js that referenced this pull request Oct 31, 2022
* feat: upgrade napi-rs to 2.10.0 and Node.js v18

* chore: lock the png crate version to 0.17.5

Upgrading to 0.17.6 will cause the generated PNG files to become larger.
See image-rs/image-png#350

* chore: build wasm
AndrewKvalheim added a commit to AndrewKvalheim/little-a-map that referenced this pull request Dec 20, 2022
Slight regression in PNG size following image-rs/image-png#350:

    $ du --apparent-size --bytes --total $before/**/*.png | tail -n1
    2198049	total
    $ du --apparent-size --bytes --total $after/**/*.png | tail -n1
    2457872	total
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.

4 participants