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

Remove duplicated crc32 algorithm #127

Closed
martinlindhe opened this issue May 4, 2019 · 10 comments · Fixed by #131
Closed

Remove duplicated crc32 algorithm #127

martinlindhe opened this issue May 4, 2019 · 10 comments · Fixed by #131
Labels

Comments

@martinlindhe
Copy link
Member

martinlindhe commented May 4, 2019

There's a crc32 implementation in src/crc.rs

A duplicate also lives in tests/check_testimages.rs

This issue is for reusing the code from src/crc.rs and deleting the duplicated code in tests/check_testimages.rs

martinlindhe added a commit to martinlindhe/image-png that referenced this issue May 4, 2019
@martinlindhe
Copy link
Member Author

As mentioned in #128, another option could be to instead depend on a external crate for crc32. Any opinions?

@HeroicKatora
Copy link
Member

HeroicKatora commented May 4, 2019

I'd only depend on an external crate if it doesn't have dependencies or if it offers direct benefits.

The crc crate seems a bit to immature for me, has too wide of a scope and for some reason generates its tables a build time?

Another criterion could be performance. crc32fast offers –according to its own benchmark–six times the throughput at 1499 MB/s and up to 7314 MB/s on sse platforms. That could be worth a great deal of tradeoff, for example in additional dependencies and would lower our maintenance burder. Edit: Its interface looks also much like our own, which is also close to the standard Hasher interface except with u32 output.

@martinlindhe
Copy link
Member Author

I'll make a PR using crc32fast and we can take it from there

@martinlindhe
Copy link
Member Author

I have a crc32fast branch working, but it currently depends on srijs/rust-crc32fast#8 in order to work.

I'm awaiting follow up on that PR before I can go ahead and open a PR for this.
Meanwhile my branch lives in https://github.com/martinlindhe/image-png/tree/crc32fast

@HeroicKatora
Copy link
Member

HeroicKatora commented May 4, 2019

I think State implements Clone, so self.current_chunk.0.clone().finalize() could work as well.

@martinlindhe
Copy link
Member Author

Thanks, I'll have a look at that

@martinlindhe
Copy link
Member Author

I think State implements Clone,

It does not. I added Clone to State & U32Value but same issue.

so self.current_chunk.0.finalize() could work as well.

This is what the code's already doing and is the issue resolved by srijs/rust-crc32fast#8

@HeroicKatora
Copy link
Member

Oops, that should have been self.current_chunk.0.clone().finalize(). Corrected it above. But rust-crc32fast does implement it for State. I'm a bit confused, why is that not accessible?

@martinlindhe
Copy link
Member Author

Just me being confused too :-) I was looking at this State: https://github.com/image-rs/image-png/blob/master/src/decoder/stream.rs#L29

@martinlindhe
Copy link
Member Author

Oops, that should have been self.current_chunk.0.clone().finalize(). Corrected it above.

Yep, this works! Rebasing etc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants