Add to constructor of the BrotliStream class ability to specify quality

[Taritsyn]

It would be nice to have the following API:

public partial class BrotliStream : System.IO.Stream
{
    …
    public BrotliStream(System.IO.Stream stream, int quality);
    public BrotliStream(System.IO.Stream stream, int quality, bool leaveOpen);
    public BrotliStream(System.IO.Stream stream, int quality, bool leaveOpen, int bufferSize);
    …
}

From the “Introducing Support for Brotli Compression” article and the “Brotli defaults to quality 11” discussion I know how to pass an integer value to the constructor as a compression level, but at the moment it leads to errors. To demonstrate these errors, create a simple console application:

using System;
using System.IO;
using System.IO.Compression;
using System.Net.Http;
using System.Threading.Tasks;
using static System.Console;

namespace TestBrotli
{
    class Program
    {
        static readonly string s_URL = "https://www.dotnetfoundation.org/";

        static async Task Main(string[] args)
        {
            var client = new HttpClient();
            var response = await client.GetAsync(s_URL, HttpCompletionOption.ResponseHeadersRead);
            var bytes = await response.Content.ReadAsByteArrayAsync();

            WriteLine($"Compression levels (quality):");
            WriteLine();

            for (int quality = 0; quality <= 11; quality++)
            {
                long compressedSize = CalculateCompressedSize(bytes, quality);

                WriteLine($"{quality}: {compressedSize}");
            }
        }

        private static long CalculateCompressedSize(byte[] bytes, int quality)
        {
            using (var compressedStream = new MemoryStream())
            {
                using (var compressionStream = new BrotliStream(compressedStream, (CompressionLevel)quality, leaveOpen: true))
                {
                    compressionStream.Write(bytes, 0, bytes.Length);
                }

                return compressedStream.Length;
            }
        }
    }
}

Running it we get the following result:

Compression levels (quality):

0: 8408
1: 11690
2: 12042
3: 10843
4: 10152
5: 9518
6: 9458
7: 9435
8: 9423
9: 9404
10: 8640
11: 8408

The results show that first three compression levels were interpreted as the CompressionLevel enumeration values:

• 0 - Optimal
• 1 - Fastest
• 2 - NoCompression

I. e. first three levels of compression actually work in reverse order. To fix this error I use the following patch:

private static long CalculateCompressedSize(byte[] bytes, int quality)
{
    CompressionLevel compressionLevel;

    switch (quality)
    {
        case 0:
            compressionLevel = CompressionLevel.NoCompression;
            break;
        case 1:
        case 2:
            compressionLevel = CompressionLevel.Fastest;
            break;
        case int q when q >= 3 && q <= 11:
            compressionLevel = (CompressionLevel)q;
            break;
        default:
            throw new NotSupportedException();
    }

    using (var compressedStream = new MemoryStream())
    {
        using (var compressionStream = new BrotliStream(compressedStream, compressionLevel, leaveOpen: true))
        {
            compressionStream.Write(bytes, 0, bytes.Length);
        }

        return compressedStream.Length;
    }
}

And I get the following result:

0: 12042
1: 11690
2: 11690
3: 10843
4: 10152
5: 9518
6: 9458
7: 9435
8: 9423
9: 9404
10: 8640
11: 8408

This result looks better, but we don't have the ability to use real level 2.

[carlossanlop]

Triage:
Being able to pass an integer casted as CompressionLevel does not seem to be something we should allow. We should continue the conversation to determine if we want to add new overloads that expose the Brotli compression level, probably as advanced APIs.

[Genbox]

I would agree with Carlos on this one. The enum is a better choice, even if it is a little-known fact that Enums can contain arbitrary values e.g. (CompressionLevel)11.

Combined with #42820 such that it is exposed as a configuration property, and combined with the new SmallestSize that gives ordinary users a better idea of what CompressionLevel gives smallest or fastest results, it makes the integer input proposal less of a good argument.

That being said, it still feels like CompressionLevel is shoehorning the range of available values. I'm going to propose a set of values in #62658 to remedy that issue.

[m-gasser]

Will this API be added in .NET 7? We currently use the workaround where we cast our desired compression level to the CompressionLevel enum. This workaround is no longer possible now that #57561 is merged and none of the default CompressionLevels fit our use case.

[adamsitnik]

@bartonjs how do you feel about adding a new BrotliStream ctor that allows the user to specify quality (values in range of 0-11) and window (10-22) as integers (not enums)?

public BrotliStream(Stream stream, bool leaveOpen, int quality, int window) 

[bartonjs]

how do you feel about adding a new BrotliStream ctor that allows the user to specify quality (values in range of 0-11) and window (10-22) as integers (not enums)?

That almost no one will understand what values are valid or what anything means. I'd personally think it was better if it was a static method with some ugly and unapproachable name, since that'll keep it out of the faces of people who just know they were told to "use brotli". public static BrotliStream [BrotliStream::]CreateFromSpecificationValues(Stream stream, int quality, int window, bool leaveOpen=true,false,whatever).

[stephentoub]

This overlaps with #42820. We shouldn't do something specific for BrotliStream without also being consistent on Deflate/ZLib/GZipStream.

[m-gasser]

This overlaps with #42820. We shouldn't do something specific for BrotliStream without also being consistent on Deflate/ZLib/GZipStream.

While I agree with this, the consequence for us is, we either have to resort to an even uglier hack using Reflection, or we have to find another compression library. It is not too bad for us, because only a single use-case is affected. However this might look different for other developers currently using this workaround. I guess many will only stumble over this after .NET 7 is released. Do you think it would be possible to fast track #42820 so it will be included in .NET 7?

Also I tried to run our biggest code base on the latest .NET 7 preview yesterday (triggered by https://devblogs.microsoft.com/dotnet/performance_improvements_in_net_7/, thanks for the awesome blog post!). I stumbled over another issue with BrotliStream. Decompression throws an exception because of truncation detection. Is it possible that the same change was made as for DeflateStream but only the DeflateStream change was reverted in #72726? I this is the case, the same reasoning applies for BrotliStream and that change should probably be reverted for all compression streams.

[stephentoub]

Do you think it would be possible to fast track #42820 so it will be included in .NET 7?

No, we're not adding new APIs to 7 at this point. Just high priority bug fixes. It's almost done. I'd hoped we'd get to it in 7, but too many other priorities took precedence; I would like to see us address it for good in 8 (@jeffhandley).

I stumbled over another issue with BrotliStream.

Can you please open a separate issue for that, with a repro? Thanks.

[m-gasser]

I stumbled over another issue with BrotliStream.

Can you please open a separate issue for that, with a repro? Thanks.

It was actually a bug in our side. After properly closing the stream it works now. I guess we just got lucky that it worked in .NET 6.

[CumpsD]

Just bumped on this breaking change crashing our code since we pass in 5 as a compression level.

Taritsyn/WebMarkupMin#150

[ericstj]

@buyaa-n @stephentoub is this now being covered with #42820?

[stephentoub]

@buyaa-n @stephentoub is this now being covered with #42820?

It should be, yes.

[stephentoub]

Deduplicating with #42820