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

[ZlibStream] ArgumentOutOfRangeException when passing in compression levels past level 3 (cast from manual enum defining all of the native compression levels). #58742

Closed
AraHaan opened this issue Sep 7, 2021 · 10 comments
Labels
area-System.IO.Compression untriaged New issue has not been triaged by the area owner

Comments

@AraHaan
Copy link
Member

AraHaan commented Sep 7, 2021

Description

I wanted to pass in the native zlib compression levels (raw 0~9) onto the Native zlib that used by DeflateStream in my code. However doing such results in an ArgumentOutOfRangeException when I explicitly cast my enum down to the one that DeflateStream expects (which are the proper values that are supported in the native zlib implementation).

This branch on my zlib repository should be a minimal reproducible code sample, also the steps to run it are dotnet test (implies build). The problem lies on this constructor.

Configuration

.NET: 6.0.100-rc.1.21417.19
OS: MacOS v12 Beta
Arch: x64
I do not think it's specific to this configuration.

Regression?

I have no clue, I just expected that the cast from my enum to the CompressionLevel enum would be ok since the native zlib implementation has those levels implemented already (for zlib it should check to see if compression level is less than -1 or greater than 9).

But then again perhaps there should be a version of DeflateStream that accepts int for compression levels instead for those who directly want to pass specific compression levels directly to the native implementation through that stream (if they want to avoid the CompressionLevel enum).

Other information

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.IO.Compression untriaged New issue has not been triaged by the area owner labels Sep 7, 2021
@filipnavara
Copy link
Member

That won't work (by design). The enumeration is converted into the native values and it's not a simple cast:

switch (compressionLevel)
{
// See the note in ZLibNative.CompressionLevel for the recommended combinations.
case CompressionLevel.Optimal:
zlibCompressionLevel = ZLibNative.CompressionLevel.DefaultCompression;
memLevel = ZLibNative.Deflate_DefaultMemLevel;
break;
case CompressionLevel.Fastest:
zlibCompressionLevel = ZLibNative.CompressionLevel.BestSpeed;
memLevel = ZLibNative.Deflate_DefaultMemLevel;
break;
case CompressionLevel.NoCompression:
zlibCompressionLevel = ZLibNative.CompressionLevel.NoCompression;
memLevel = ZLibNative.Deflate_NoCompressionMemLevel;
break;
case CompressionLevel.SmallestSize:
zlibCompressionLevel = ZLibNative.CompressionLevel.BestCompression;
memLevel = ZLibNative.Deflate_DefaultMemLevel;
break;
default:
throw new ArgumentOutOfRangeException(nameof(compressionLevel));
}

@AraHaan
Copy link
Member Author

AraHaan commented Sep 7, 2021

Actually I think that default case should be changed to:

     default: 
         zlibCompressionLevel = (ZLibNative.CompressionLevel)compressionLevel; 
         memLevel = ZLibNative.Deflate_DefaultMemLevel; 
         break;

As most people like me want to use compression levels between the ones that are provided and so they expect to be able to cast it to CompressionLevel. I feel like that should not break anything at all really.

Also it would also allow for all of the compression levels that the native implementation supports to be used as well from .NET (Currently this implementation does not allow it due to that default case throwing which I feel like should be a bug as those are valid compression levels on the native side).

@filipnavara
Copy link
Member

filipnavara commented Sep 7, 2021

Actually I think that default case should be changed to

That would not work. The values of the enum overlap with the values of the native compression levels.

@AraHaan
Copy link
Member Author

AraHaan commented Sep 7, 2021

Well there should be some way to allow one to use all of the compression levels that the native one supports from within .NET, perhaps a ctor needs to be made that allows one to pass it in as an int instead of the CompressonLevel enum for the compression levels (even for the Deflater)?

@filipnavara
Copy link
Member

filipnavara commented Sep 7, 2021

There's not even a guarantee that all native values are supported or that the native zlib is used. In fact, it was not used in early .NET Framework versions and it's probably still not used for Browser implementation (https://github.com/dotnet/runtime/tree/main/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateManaged). For most platforms .NET actually uses a modified Intel Zlib fork and not the original library.

I don't necessarily see a use case for supporting more compression levels but if that was to be done it would have to be a new API that explicitly accepts the 0-9 int value.

Note that there are managed forks of the original Zlib, like https://github.com/SixLabors/ZlibStream, that support the full range of the compression levels. It essentially has the same heritage like the fork you linked in the original post.

@AraHaan
Copy link
Member Author

AraHaan commented Sep 7, 2021

Also note that their ZlibStream inflater is broken: image

As such I thought I could redo the stream based on DeflateStream, but it looks like I will have to instead copy and paste the code inside of it and all the dependent code to it on over to my project and remove the usage of System.IO.Compression.CompressionLevel enum entirely (turn it into an int param) and then look into all the other things deep down in the implementation to ensure everything works properly on all of those compression levels.

@FiniteReality
Copy link

Actually I think that default case should be changed to

That would not work. The values of the enum overlap with the values of the native compression levels.

A trick used by some other APIs is to map the known values to negative numbers, and then allow somebody to pass a positive value which gets mapped as-is. Since CompressionLevel already uses positive numbers, the opposite could instead be done: negative numbers would get mapped to their absolute value.

Personally, though, I think the best solution would be to expose a ZLibEncoder/Decoder (see this API request comment for inspiration) which takes the encoder/decoder-specific options, and leave the Stream types to take CompressionLevel. After all, new applications should (IMO) be using pipelines which can be wrapped as streams when interfacing with older software.

@AraHaan
Copy link
Member Author

AraHaan commented Sep 7, 2021

But what if one wants to construct those streams using an int value for compression levels?

@FiniteReality
Copy link

But what if one wants to construct those streams using an int value for compression levels?

You don't.

@stephentoub
Copy link
Member

Closing as a dup of #42820

@ghost ghost locked as resolved and limited conversation to collaborators Oct 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO.Compression untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

No branches or pull requests

4 participants