Introduce Shared General Decoder Options plus Specialization

[JimBobSquarePants]

Prerequisites

• I have written a descriptive pull-request title
• I have verified that there are no overlapping pull-requests open
• I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
• 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!

[gfoidl]

Just scrolled through the changes, some nits.
Didn't read the xml-comments and tests though.

[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.

[br3aker]

I suppose it's worth mentioning that quality of this mode is close to the 'Box' resampling/scaling.

[br3aker]

I believe we should make resampler type configurable to mimic MagicScaler functionality. What do you think?

[JimBobSquarePants]

I guess so, could easily add it to the general options. My original thinking though was that if they wanted more control they could use the processor.

[JimBobSquarePants]

I've added it.

[br3aker]

My original thinking though was that if they wanted more control they could use the processor.

Totally missed that possibility, need more sleep.
But I think all-in-one API may still be superior than two-step API.
Would like to hear Anton on this subject.

[antonfirsov]

The the target-size decoder is a memory-optimized version of the two step decode-resize workflow. In an ideal world I would feature-match them, so we don't force the user to choose between perf and flexibility. However I understand that it also makes sense to keep DecoderOptions as simple as possible. I think adding Resampler makes sense, we'll se if users need more.

[antonfirsov]

Approving the general direction, since I couldn't spot anything principally wrong after a quick look, but don't have time to do a full scale review now.

I'm leaving to a vacation tomorrow with no computer access at all, and will be back next Monday. Would prefer to do a final "nitpicking" review early next week if possible, we need to add tests anyways before merging. Sorry that it's going this slow ...

[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.