Revisit BrotliStream CompressionLevel values

[stephentoub]

Two independent-but-related issues:

1. The ctor:

public BrotliStream(Stream stream, CompressionLevel compressionLevel)

passes the compressionLevel to the function:

        internal static int GetQualityFromCompressionLevel(CompressionLevel level) =>
            level switch
            {
                CompressionLevel.Optimal => Quality_Default,
                CompressionLevel.NoCompression => Quality_Min,
                CompressionLevel.Fastest => 1,
                CompressionLevel.SmallestSize => Quality_Max,
                _ => (int)level,
            };

with no argument validation. If an arbitrary level is provided, that then gets passed through as-is to the underlying library, resulting in inconsistent and potentially unexpected behavior. We should decide whether we're ok with this behavior or whether we should take a breaking change to correct it and be more like DeflateStream, which throws for an unknown value.

2. The quality values are set as:

        public const int Quality_Min = 0;
        public const int Quality_Default = 11;
        public const int Quality_Max = 11;

With the introduction of SmallestSize (#1549 (comment)) and the rationale employed for it that Optimal should really be a good balance between speed and size, we should define it closer to 6 rather than 11. The docs will also need to be updated.

[GSPP]

Users are definitely going to take a dependency on this. It will become harder and harder to change this.

[carlossanlop]

Triage:

We should decide whether we're ok with this behavior or whether we should take a breaking change to correct it

1. I think we should be slightly more strict with validation: The user should still be able to pass an arbitrary value, but only if it's between the acceptable ranges of the underlying algorithm.

we should define it closer to 6 rather than 11

2. Makes sense. We should make the change. I think the docs are already correct in triple slash for both the SmallestSize and Optimal enum values, so we need to make sure the MS Docs texts are updated.

This could become a breaking change for customers already using Optimal, so we need to make sure to report this.

[stephentoub]

I think we should be slightly more strict with validation: The user should still be able to pass an arbitrary value, but only if it's between the acceptable ranges of the underlying algorithm.

To be clear, this means that if someone passes in the values 0, 1, 2, or 3 it's interpreted as the corresponding value from CompressionLevel, but if someone passes in 4 through 11, it's interpreted completely differently as the underlying compression level setting? (Also, the validation you propose is already done.)

[i3arnon]

I went ahead with a PR that validates that values are between 0 and 11: #57440

[carlossanlop]

To be clear, this means that if someone passes in the values 0, 1, 2, or 3 it's interpreted as the corresponding value from CompressionLevel, but if someone passes in 4 through 11, it's interpreted completely differently as the underlying compression level setting?

I see what you mean. My idea would be confusing to users.

What about a constructor that takes an integer between 0 and 11, to allow users to freely specify a valid compression level value accepted by the underlying algorithm, without using a CompressionLevel enum value? We could consider it an advanced API.

[stephentoub]

What about a constructor that takes an integer between 0 and 11, to allow users to freely specify a valid compression level value accepted by the underlying algorithm, without using a CompressionLevel enum value? We could consider it an advanced API.

That's what BrotliEncoder provides, what #1556 proposes for BrotliStream, and what #42820 discusses standardizing for all of compression.

[stephentoub]

@carlossanlop, this was closed by #57561, but it only addressed half of this issue. Was closing it intentional? The two halves should really be addressed together.