Expose non-nullable configuration to remove AOT limiting null check #2514
Conversation
JimBobSquarePants
changed the title
JimBobSquarePants
requested review from
tocsoft,
antonfirsov,
dlemstra and
brianpopow
There was a problem hiding this comment.
seems fine... only concern is the unnesorcery breaking of the public API. (AdvancedImageExtensions.GetConfiguration(...) overloads.
yes we are no longer dependent on them, but they are public api and some else probably is, anyone with custom encoders/decoders for example.
| /// </summary> | ||
| /// <param name="source">The source image.</param> | ||
| /// <returns>Returns the configuration.</returns> | ||
| public static Configuration GetConfiguration(this Image source) |
There was a problem hiding this comment.
we should probably leave these in despite then no longer being needed for backwards compatability, anyone with a dependency on this public API will break otherwise, shoudl mark them Obsolete instead
There was a problem hiding this comment.
There are several breaking changes coming to 3.1 already without obsoletion eg. #2455 or #2485. I see little point using Obsolete selectively.
What is worrying me is that the net amount of breaking changes between 3.0.* -> 3.1.0 is much bigger than between 2.0.* -> 2.1.0. Maybe we should rename 3.1 to 4.0 to avoid confusion?
There was a problem hiding this comment.
My plan with major versions has always been to support latest the .NET LTS version. There are breaking changes (unfortunately more than I'd like), but they typically only affect scenarios involving advanced behavior.
|
@MichalStrehovsky I finally bit the bullet and installed the .NET 8 RC so I could use Sizoscope. With these changes and a tweak to the calling code from the sample project in #2486 we achieve much better results. /// <summary>
/// Saves the given buffer using the filename extension as format.
/// </summary>
/// <param name="filename">The full pathname for the filename being written</param>
/// <param name="buffer">The image data stored as RGB buffer.</param>
/// <param name="width">The image width in pixels.</param>
/// <param name="height">The image height in pixels.</param>
public static void SaveImage(string filename, ReadOnlySpan<byte> buffer, int width, int height)
{
// Pass custom Configuration to avoid loading all default formats.
using var image = Image.LoadPixelData<Rgb24>(new(), buffer, width, height);
using var stream = new FileStream(filename, FileMode.Create);
var pngEncoder = new PngEncoder()
{
ColorType = PngColorType.Rgb
};
image.SaveAsPng(stream, pngEncoder);
}
We could probably drop the size by another 40-45kb if we refactored the way the quantizer is loaded in the png encoder, for some reason the compiler cannot trim out the construction of that type but that's beyond the scope of this PR. |
Nice! How big is the resulting sample project? I'm guessing this is a ~30% size reduction? IIRC this app was about 5 MB before. |
The executable? Less than half if I'm following the Twitter thread correctly. 
|
Better than expected! Looks like C#/.NET matches the Go size now! |
|
Fantastic stuff! I'll be looking at Sizoscope a lot for v4 to see if we can design the library internals to be as trimmable as possible. 😁 |
Prerequisites
Description
Fixes #2486
As discussed in the linked issue
AdvancedImageExtensions.GetConfiguration()causes the AOT compiler to include all image unused formats due to the null check and assignment ofConfiguration.Default.Since that null check is unavoidable without an API change, I decided to make it by making
IConfigurationProviderpublic and exposing the property directly.While developers should be careful to not edit the configuration singleton, I do not consider accessing the configuration an advanced scenario since we already provide means to construct and pass custom configuration to our types.