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 deflate rather than flate2 for deflate encoding. Fixes #2 #46

Merged
merged 1 commit into from
Jan 6, 2017
Merged

Use deflate rather than flate2 for deflate encoding. Fixes #2 #46

merged 1 commit into from
Jan 6, 2017

Conversation

oyvindln
Copy link
Contributor

@oyvindln oyvindln commented Jan 5, 2017

This changes the encoder to use deflate-rs, which is written in rust rather than flate2 which is wrapper to the c library miniz. This would fix #2.

I don't know if it should be merged yet, the roundtrip unit test seems to work fine, but it may need some more testing on other images. (There's a failing doc test, but it failed before I did any changes so I presume it's unrelated, as it complains about a missing file). I've tested the decoder on a fair number of different inputs, but there could still be bugs in it (but feel free to help out with the decoder if you find any). It's also going to be a bit slower than using flate2 for now.

My editor also ran rustfmt on the file I changed, so if that's an issue I'll change the PR to avoid that if you want.

Lastly, I wasn't sure if I should pin the full version, or if I should only depend on "0.7" to avoid having to update image-png in case of bugfixes to deflate.

@bvssvni
Copy link
Contributor

bvssvni commented Jan 6, 2017

Perhaps we could use a Cargo feature to enable flate2? This way people could test it in real projects.

@nwin
Copy link
Contributor

nwin commented Jan 6, 2017

I think we should just merge it like it is. The test we have passes. Encoders are usually less critical than decoders. We should have a look at the doctest nevertheless.

@oyvindln
Copy link
Contributor Author

oyvindln commented Jan 6, 2017

I suspect the failing doctest is due to this. Anyhow, that's a separate issue.

@bvssvni
Copy link
Contributor

bvssvni commented Jan 6, 2017

Nice! Merging.

@bvssvni bvssvni merged commit 62f92a5 into image-rs:master Jan 6, 2017
@eddyb
Copy link
Member

eddyb commented Jan 6, 2017

I mentioned this to @alexcrichton on IRC and he seems fine with adding a feature to flate2 to expose its API on top of these Rust implementations - that seems like it may scale better to other existing users.

@eddyb
Copy link
Member

eddyb commented Jan 6, 2017

@bvssvni LOL that timing.

@bvssvni
Copy link
Contributor

bvssvni commented Jan 6, 2017

@eddyb 😄

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.

Use pure Rust library instead of flate2
4 participants