search

Frommi / miniz_oxide

Rust replacement for miniz
MIT License
168 stars 48 forks source link

Allow disabling checksum test for zlib #102

Closed HeroicKatora closed 2 years ago

HeroicKatora commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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:

https://github.com/Frommi/miniz_oxide/blob/19782aa0833d201eed6bcbc847eefaa741bc2e32/miniz_oxide/src/inflate/stream.rs#L182

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 commented 2 years ago

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 commented 2 years ago

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.