-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Add WindowSize property to BrotliCompressionOptions #121786
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
base: main
Are you sure you want to change the base?
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-io-compression |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a WindowSize property to the BrotliCompressionOptions class, allowing developers to configure the Brotli compression window size. The window size controls the sliding window size in bits for the LZ77 algorithm, with valid values ranging from 10 to 24 and a default of 22.
- Added
WindowSizeproperty toBrotliCompressionOptionswith validation (10-24 range) - Integrated window size configuration into
BrotliStreamcompression constructor - Added comprehensive test coverage for window size validation
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/enc/BrotliCompressionOptions.cs | Added WindowSize property with validation using BrotliUtils constants |
| src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/enc/BrotliStream.Compress.cs | Applied window size configuration to encoder when using compression options |
| src/libraries/System.IO.Compression.Brotli/ref/System.IO.Compression.Brotli.cs | Updated public API reference to include WindowSize property |
| src/libraries/System.IO.Compression.Brotli/tests/CompressionStreamUnitTests.Brotli.cs | Added test to validate window size property including default value and boundary conditions |
Comments suppressed due to low confidence (1)
src/libraries/System.IO.Compression.Brotli/src/System/IO/Compression/enc/BrotliCompressionOptions.cs:27
- The Quality property validation uses hardcoded literals (0, 11) instead of BrotliUtils constants, unlike the WindowSize property which uses BrotliUtils.WindowBits_Min and BrotliUtils.WindowBits_Max. For consistency and maintainability, consider using BrotliUtils.Quality_Min and BrotliUtils.Quality_Max here as well.
ArgumentOutOfRangeException.ThrowIfLessThan(value, 0, nameof(value));
ArgumentOutOfRangeException.ThrowIfGreaterThan(value, 11, nameof(value));
| /// <remarks> | ||
| /// The window size is the sliding window size in bits used by the LZ77 algorithm. Larger window sizes can improve compression ratio but use more memory. Range is from 10 to 24. The default value is 22. | ||
| /// </remarks> | ||
| public int WindowSize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bartonjs I just realized, the Zstandard proposal named similar property Window, and the existing ctor parameter on BrotliEncoder is also called window. I think we should make sure the names are consistent.
Unfortunately, we are already somewhat inconsistent because DeflateStream parameters are called windowBits, but we'd better not add a third name for the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like Window is the more naturally aligned name, so it's probably fine to go ahead and use it. Send a blurb to FXDC to see if anyone wants to object.
This PR adds a
WindowSizeproperty toBrotliCompressionOptionsto allow developers to configure the Brotli compression window size (valid range: 10-24, default: 22). The window size controls the maximum distance for back-references in the LZ77 algorithm, where larger windows can improve compression for data with long-distance repetitions. Includes corresponding validation tests following the existing pattern established for theQualityproperty.Fixes #116227