From 107432829a855bb78f658ed36a9f72e31e32e58e Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Fri, 15 Jul 2022 09:14:10 -0400 Subject: [PATCH] Fix CompressionLevel.Optimal for Brotli The intent of Optimal is to be a balanced trade-off between compression ratio and speed, but whereas DeflateStream, GZipStream, and ZLibStream all treat Optimal as such (using zlib's default setting for such a balanced tradeoff), Brotli treats Optimal the same as maximum compression, which is very slow. Especially now that maximum compression is expressible as CompressionLevel.SmallestSize, it's even more valuable for Optimal to represent that balanced tradeoff. Based on a variety of sources around the net and some local testing, I've changed the Optimal value from 11 to 4. This is also more important now that we've fixed the argument validation bug that allowed arbitrary numerical values to be passed through unvalidated (DeflateStream, GZipStream, and ZLibStream all properly validate). --- .../src/System/IO/Compression/BrotliStream.cs | 9 +++++++++ .../src/System/IO/Compression/BrotliUtils.cs | 4 ++-- .../System/IO/Compression/enc/BrotliStream.Compress.cs | 1 + 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/BrotliStream.cs b/src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/BrotliStream.cs index 979d01175eedf..7e313a40b29a2 100644 --- a/src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/BrotliStream.cs +++ b/src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/BrotliStream.cs @@ -33,12 +33,21 @@ public BrotliStream(Stream stream, CompressionMode mode, bool leaveOpen) { case CompressionMode.Compress: if (!stream.CanWrite) + { throw new ArgumentException(SR.Stream_FalseCanWrite, nameof(stream)); + } + + _encoder.SetQuality(BrotliUtils.Quality_Default); + _encoder.SetWindow(BrotliUtils.WindowBits_Default); break; + case CompressionMode.Decompress: if (!stream.CanRead) + { throw new ArgumentException(SR.Stream_FalseCanRead, nameof(stream)); + } break; + default: throw new ArgumentException(SR.ArgumentOutOfRange_Enum, nameof(mode)); } diff --git a/src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/BrotliUtils.cs b/src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/BrotliUtils.cs index 52c5c39c42b6d..2e8e55a570ae3 100644 --- a/src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/BrotliUtils.cs +++ b/src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/BrotliUtils.cs @@ -9,16 +9,16 @@ internal static partial class BrotliUtils public const int WindowBits_Default = 22; public const int WindowBits_Max = 24; public const int Quality_Min = 0; - public const int Quality_Default = 11; + public const int Quality_Default = 4; public const int Quality_Max = 11; public const int MaxInputSize = int.MaxValue - 515; // 515 is the max compressed extra bytes internal static int GetQualityFromCompressionLevel(CompressionLevel compressionLevel) => compressionLevel switch { - CompressionLevel.Optimal => Quality_Default, CompressionLevel.NoCompression => Quality_Min, CompressionLevel.Fastest => 1, + CompressionLevel.Optimal => Quality_Default, CompressionLevel.SmallestSize => Quality_Max, _ => throw new ArgumentException(SR.ArgumentOutOfRange_Enum, nameof(compressionLevel)) }; diff --git a/src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/enc/BrotliStream.Compress.cs b/src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/enc/BrotliStream.Compress.cs index 559119945398e..9f2c84dfc0c55 100644 --- a/src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/enc/BrotliStream.Compress.cs +++ b/src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/enc/BrotliStream.Compress.cs @@ -17,6 +17,7 @@ public sealed partial class BrotliStream : Stream /// The stream to compress. /// One of the enumeration values that indicates whether to emphasize speed or compression efficiency when compressing the stream. public BrotliStream(Stream stream, CompressionLevel compressionLevel) : this(stream, compressionLevel, leaveOpen: false) { } + /// Initializes a new instance of the class by using the specified stream and compression level, and optionally leaves the stream open. /// The stream to compress. /// One of the enumeration values that indicates whether to emphasize speed or compression efficiency when compressing the stream.