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

Allow disabling checksum test for zlib #102

Closed
HeroicKatora opened this issue Apr 15, 2021 · 4 comments
Closed

Allow disabling checksum test for zlib #102

HeroicKatora opened this issue Apr 15, 2021 · 4 comments

Comments

@HeroicKatora
Copy link
Contributor

HeroicKatora commented Apr 15, 2021

This is a feature request, motivated by the request to parse a png with incorrect checksum

libpng warning: IDAT: incorrect data check

The png library also wants to accept this image as it obviously has correct image data and the need for actually checksumming data chunks at rest is dubious at best. It seems that miniz_oxide currently always computes and tests the checksum when the flag TINFL_FLAG_PARSE_ZLIB_HEADER is active.

https://docs.rs/miniz_oxide/0.4.4/src/miniz_oxide/inflate/core.rs.html#1619

I had previously assumed that not setting TINFL_FLAG_COMPUTE_ADLER32 would work but counter intuitively it seems that it the flag is overwritten by the zlib request.

@oyvindln
Copy link
Collaborator

Yeah this seems reasonable. png actually has double checksums for the sections I think? Using both the zlib one and a CRC checksum in the png format itself, so one could probably save some processing by skipping the zlib one.

@AlanRace
Copy link

I am not sure whether this is related, or would be better suited in its own issue, but I am a little unsure why the checksum is always turned on in the inflate function:

let mut decomp_flags = inflate_flags::TINFL_FLAG_COMPUTE_ADLER32;

I thought from looking at the code, that this should depend on the InflateState? As far as I can tell there is no way to turn off the calculation of the checksum in cases where I just want fast decompression - but perhaps I have missed something?

Manually removing this flag (i.e. by let mut decomp_flags = 0;) and recompiling results in a ~13% speed increase for decompressing in my use case.

@oyvindln
Copy link
Collaborator

oyvindln commented Oct 27, 2021

It's the same thing. Should be easy enough to add an option for it. There isn't any other reason that it it just not being implemented. (EDIT: And it was not an option to ignore it in original C miniz it seems either.)

@oyvindln
Copy link
Collaborator

Ok, adding a parameter to the InflateState constructors would require some breakage, although it could be done with only very minor breakage by adding an extra variant to DataFormat (which in practice I doubt would break any code using this crate.) That should work fine for now I think. Planning a version bump to raise MSRV #106 in any case so those tho could be done together. Changing the InflateState constructors etc is probably better done together with other larger API changes.

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

3 participants