Introduce Shared General Decoder Options plus Specialization

[JimBobSquarePants]

Prerequisites

• [x] I have written a descriptive pull-request title
• [x] I have verified that there are no overlapping pull-requests open
• [x] I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules :cop:.
• [x] I have provided test coverage for my change (where applicable)

Description

Based on the discussion #2091 I've refactored our decoding API to do a few things.

• Introduced shared general decoder options
• Introduced specialized decoder specific options
• Introduced a general API for resizing images on decode.
• Introduced a new base ImageDecoder<T> class where T is ISpecializedDecoderOptions which allows external implementations to easily implement decoders using our new resize-on-decode strategy.
• Removed the ability to pass a decoder to Image.Load(...) as it was leading to too many user errors based on incorrect encoding assumptions.
• Removed all array overloads to our load APIs now that we have implicit ReadonlySpan<T> conversion

The general decoder options look like this.

/// <summary>
/// Provides general configuration options for decoding image formats.
/// </summary>
public sealed class DecoderOptions
{
    private static readonly Lazy<DecoderOptions> LazyOptions = new(() => new());

    private uint maxFrames = int.MaxValue;

    /// <summary>
    /// Gets the shared default general decoder options instance.
    /// </summary>
    public static DecoderOptions Default { get; } = LazyOptions.Value;

    /// <summary>
    /// Gets or sets a custom Configuration instance to be used by the image processing pipeline.
    /// </summary>
    public Configuration Configuration { get; set; } = Configuration.Default;

    /// <summary>
    /// Gets or sets the target size to decode the image into.
    /// </summary>
    public Size? TargetSize { get; set; } = null;

    /// <summary>
    /// Gets or sets a value indicating whether to ignore encoded metadata when decoding.
    /// </summary>
    public bool SkipMetadata { get; set; } = false;

    /// <summary>
    /// Gets or sets the maximum number of image frames to decode, inclusive.
    /// </summary>
    public uint MaxFrames { get => this.maxFrames; set => this.maxFrames = Math.Min(Math.Max(value, 1), int.MaxValue); }
}

The options replace Configuration as the primary parameter in all our decoding APIs

/// <summary>
/// Decode a new instance of the <see cref="Image"/> class from the given stream.
/// </summary>
/// <param name="options">The general decoder options.</param>
/// <param name="stream">The stream containing image information.</param>
/// <exception cref="ArgumentNullException">The options are null.</exception>
/// <exception cref="ArgumentNullException">The stream is null.</exception>
/// <exception cref="NotSupportedException">The stream is not readable or the image format is not supported.</exception>
/// <exception cref="UnknownImageFormatException">Image format not recognised.</exception>
/// <exception cref="InvalidImageContentException">Image contains invalid content.</exception>
/// <returns>A new <see cref="Image"/>.</returns>
public static Image Load(DecoderOptions options, Stream stream)

I've deliberately chosen not to allow passing ResizeOptions to the decoder as I want to leave a clear path between general and specialist pipelines. Default resize-on-decode uses ResizeMode.Max with the KnownResamplers.Box.

An example ISpecializedDecoderOptions implementation looks like the following.

/// <summary>
/// Configuration options for decoding Windows Bitmap images.
/// </summary>
public sealed class BmpDecoderOptions : ISpecializedDecoderOptions
{
    /// <inheritdoc/>
    public DecoderOptions GeneralOptions { get; set; } = new();

    /// <summary>
    /// Gets or sets the value indicating how to deal with skipped pixels,
    /// which can occur during decoding run length encoded bitmaps.
    /// </summary>
    public RleSkippedPixelHandling RleSkippedPixelHandling { get; set; }
}

Usage is allowed only on decoder instances via individual IImageDecoderSpecialized<T> implementations

/// <summary>
/// Decodes the image from the specified stream to an <see cref="Image{TPixel}"/> of a specific pixel type.
/// </summary>
/// <typeparam name="TPixel">The pixel format.</typeparam>
/// <param name="options">The specialized decoder options.</param>
/// <param name="stream">The <see cref="Stream"/> containing image data.</param>
/// <param name="cancellationToken">The token to monitor for cancellation requests.</param>
/// <returns>The <see cref="Image{TPixel}"/>.</returns>
/// <exception cref="ImageFormatException">Thrown if the encoded image contains errors.</exception>
public abstract Image<TPixel> DecodeSpecialized<TPixel>(T options, Stream stream, CancellationToken cancellationToken)
    where TPixel : unmanaged, IPixel<TPixel>;

/// <summary>
/// Decodes the image from the specified stream to an <see cref="Image"/> of a specific pixel type.
/// </summary>
/// <param name="options">The specialized decoder options.</param>
/// <param name="stream">The <see cref="Stream"/> containing image data.</param>
/// <param name="cancellationToken">The token to monitor for cancellation requests.</param>
/// <returns>The <see cref="Image{TPixel}"/>.</returns>
/// <exception cref="ImageFormatException">Thrown if the encoded image contains errors.</exception>
public abstract Image DecodeSpecialized(T options, Stream stream, CancellationToken cancellationToken);

All but the JpegDecoder utilize the same resizing strategy for specialized methods as the default loading behavior. JpegDecoder will only downscale to the nearest size that is supported by the decoder allowing users to apply their own resize behavior in advanced scenarios. CC @br3aker

[JimBobSquarePants]

I must have broken something with the last commit. I promise I had all tests passing! 😝

[br3aker]

Phenomenal, will try to review this week!

[antonfirsov]

There is a lot in there, have to pull down the code and do a proper review. Couldn't do it this last week, but will definitely take a look this week.

[JimBobSquarePants]

Thanks @antonfirsov for the thorough review. I'll make the default options internal for now but I want to discuss further before making other changes to be sure we're on the same page.

[antonfirsov]

One more concern:

We should think a bit more about the specialized decoder cases from user perspective. https://github.com/SixLabors/ImageSharp/discussions/2091#discussioncomment-3036607 considers it done with the following:

new JpegDecoder().DecodeSpecialized(JpegDecoderOptions ...)

However:

• DecodeSpecialized has a CancellationToken argument which is a weird thing to be expected from the user on a synchronous API. The only precedence is the unfamous/controversary sync HttpClient API. For clarity: I consider it OK as a compromise about how we implement decoders, but it's very weird on a public method signature that is expected to be invoked by the user.
• There is no async variant for Decode which can lead to complains similar to #272

I see two potential solutions to address this problem:

1. Keep ImageDecoderSpecialized<T> as proposed in https://github.com/SixLabors/ImageSharp/pull/2180#discussion_r935729010 and defining additional convenience and async overloads. Or equivalently: definine an IImageDecoderSpecialized<T> interface and a type containing extension methods.
2. Move back specialized options to stateful decoders, and let the existing Image.Load and Image.LoadAsync logic deal with the problem.

[JimBobSquarePants]

Move back specialized options to stateful decoders, and let the existing Image.Load and Image.LoadAsync logic deal with the problem.

Every time I look at stateful decoders mixed with general options it looks ugly. plus I really want to avoid passing a decoder to either of the Load/LoadAsync methods. It's a footgun for the inexperienced.

Regarding that cancellation token.... Yeah. Would be great to avoid it but we'd need a way to make Identify, Decode<T>, and Decode hidden from general consumption callable only by the methods on Image. This isn't a new problem though, it's possible today to spin up a decoder and decode an image using the Decode method directly with that token exposed.

One way to hide the methods of course is to remove the IImageDecoder and IImageInfoDetector interfaces and favor abstract base classes. That allows us to use protected.

[br3aker]

has a CancellationToken argument which is a weird thing to be expected from the user on a synchronous API

Why? From MSDN:

A CancellationToken enables cooperative cancellation between threads, thread pool work items, or Task objects.

Image decoding may be a very long-running operation. If user needs/wants to quit early - why not allow it rather than forcing users to wait for a result user no longer needs?

[antonfirsov]

A CancellationToken enables cooperative cancellation between threads, thread pool work items, or Task objects.

@br3aker this wording implies to me that CancellationToken was designed for multi-threaded / asynchronous scenarios. (When you do a sync call there aren't really any threads, thread pool work items or tasks involved, apart from the single thread that's doing the call.) There aren't any great examples where synchronous / blocking API-s take CancellationToken. When dotnet/runtime#32125 was designed, one of the concerns was the proposal being the precedence that unleashes the beast of synchronous API-s with a CancellationToken.

Image decoding may be a very long-running operation. If user needs/wants to quit early - why not allow it rather than forcing users to wait for a result user no longer needs?

This is totally reasonable in case of decoding yet it's atypical in .NET. Because of this, we don't have synchronous Image.Load methods accepting CancellationToken. We expect users to call LoadAsync instead, that way cancellation can also abort IO operations.

One way to hide the methods of course is to remove the IImageDecoder and IImageInfoDetector interfaces and favor abstract base classes.

@JimBobSquarePants I'm not sure if it's worth to bother with this. For me it's more important that we really don't want the cancellable sync methods to be the only overloads users can call. I would still consider exploring ways to expose facade methods similar to Image.Load and Image.LoadAsync, if we want to avoid the stateful decoders + general options mess. (The outcome might be worse than additional overloads right on the decoder or interface+extensions, but might worth a try IMO.)

[antonfirsov]

Every time I look at stateful decoders mixed with general options it looks ugly. plus I really want to avoid passing a decoder to either of the Load/LoadAsync methods. It's a footgun for the inexperienced.

We can also consider stuff like:

class Image
{
    public static Image DecodeBmp(BmpDecoderOptions options, ...);
}

[JimBobSquarePants]

class Image
{
    public static Image DecodeBmp(BmpDecoderOptions options, ...);
}

Unfortunately I don't think that's an improvement over passing a decoder to Image.Load(...).

I really want clean separation of advanced decoder operations from the general API. I've spotted more instances where people naïvely decode files because the extension is wrong than I can handle and I want to obscure that option so that experts only use it.

I'll have another crack at the API tomorrow night.

[br3aker]

This wording implies to me that CancellationToken was designed for multi-threaded / asynchronous scenarios.

I personally understand that wording as it's impossible to cancel synchronous operation without an additional thread (which is obvious of course). IMO it's sad that concept of cancellation tokens became tightly coupled with async calls over the years...

But I understand your point, most likely end users would prefer simplier sync API. Just got curious about the reasoning behind it.

[JimBobSquarePants]

To remove the cancellation token from the public API we need to remove IImageDecoder and use an abstract class. We'd do the same for encoders when we get there.

This gives us a ton more flexibility. We can make the default Decode methods protected abstract in the base class and only ever expose the specialized methods in the public API, funneling users always through Image, Image<T> for anything but those operations.

[antonfirsov]

I'm not suggesting to remove it from the Decoder API. That's the best way for us to implement the cancellation, so makes total sense to pass it to decoder implementations.

The concern I raised was simply:

• For a caller of our public API-s, that should not be they can interact with.
• Convenience overloads (or facades) and async variants are needed anyways.

[JimBobSquarePants]

@antonfirsov I've tried to follow your suggestions as closely as I can so please take another look. The API is a lot more sane now IMO. I still need to add tests though.

[antonfirsov]

Cool! Will do another round of review on Monday.

[antonfirsov]

Almost forgot the most important question regarding the proposed design:

What are our plans for the encoders? Do we plan to remove the disparity? If yes, can we put together a rough API proposal to make sure we are going to the right direction? If not: are we really OK decoder and encoder design being very different?

[JimBobSquarePants]

What are our plans for the encoders? Do we plan to remove the disparity? If yes, can we put together a rough API proposal to make sure we are going to the right direction? If not: are we really OK decoder and encoder design being very different?

I'm not sure yet. I think I can make them similar but I don't want to block the API improvements here figuring it out.

[JimBobSquarePants]

OK.... Added a bunch of extra tests.

Had another look at the encoder story and stateless with options looks like a sensible pattern we can adopt also. It allows us to ensure only registered encoders are used (currently it's possible to allow passing unregistered).

If I can get some eyes on this branch again please I'd be very grateful. I'm very keen to move on.

[JimBobSquarePants]

I'm gonna push on with this. Lot's to do.