API proposal: Pixel-agnostic Image base class

[antonfirsov]

This is an API proposal (+POC), communicated in the form of a pull request, not intended for merging. The target branch contains stripped ImageSharp source code, so we can focus on the important changes.

Please, feel free to review it, any feedback is welcome!

The problem

This proposal aims to solve the following issues with the current API:

• The majority of our users (eg. thumbnail makers) are not interested in the pixel type backing the Image<TPixel> instance. They just want to apply a processor, and resave the image. TPixel, Rgba32 is only a noise for them.
• Image.Load(...) is decoding images into into Image<Rgba32> in an arbitrary manner. This is sub-optimal in many cases. Eg. jpegs do not have an alpha-channel, therefore it's better to decode them into Image<Rgb24>. It would be benefitical to solve this in a way that's transparent to the user.
• It's not possible to make the current ImageProcessor implementations public without exposing a bunch of implementation details. This means that any refactor and optimization could introduce breaking changes in the future.
• From API perspective, it's not necessary for ImageProcessors and extension methods to be pixel-type specific.

The solution

• Making Image an abstract, pixel-agnostic base class for Image<TPixel>
• Image.Load(....) overloads return an Image instead of Image<Rgba32>
• Image.Load<TPixel>(....) overloads keep returning Image<TPixel>
• Introcuce a pixel-agnostic IImageProcessor and IImageProcessingContext interfaces
• Separate API from implementation in Image Processors. The "definition" class implements IImageProcessor, it's only responsible to encapsulate the parameters, and create the pixel specific IImageProcessor<TPixel> implementation.
• The majority of the extension methods could operate on the pixel-agnostic Image and IImageProcessingContext (See ResizeExtensions in the PR changes).
• For stuff like drawing, users should be aware of the pixel type.
• The "trick" is done by Double Dispatch, but it makes sense to follow the terminology defined by by the Visitor Pattern.

Impact

This is a breaking change, however it's scope is very limited, most of the users should be able to adapt the new API with minimal or no code changes:

• All users should recompile their project after package update
• Code using specific pixel types when loading an image should be unaffected:

using (Image<Rgb24> image = Image.Load<Rgb24>("foo.jpg"))
{
    image.Mutate(x => x
         .Resize(image.Width / 2, image.Height / 2);
    image.Save("bar.jpg");
}

• Processing code using var should be unaffected:

using (var image = Image.Load("foo.jpg"))
{
    image.Mutate(x => x
         .Resize(image.Width / 2, image.Height / 2);
    image.Save("bar.jpg");
}

• Code which used to specify the Image<Rgba32> class when using Image.Load(...) could be easily adapted by replacing Image<Rgba32> with Image:

using (Image image = Image.Load("foo.jpg"))
{
    image.Mutate(x => x
         .Resize(image.Width / 2, image.Height / 2);
    image.Save("bar.jpg");
}

[JimBobSquarePants]

Yeah mate, this is really nice. Very neat way to allow exposing the processors without the implementations.

I think we should go with it. 👍

[JimBobSquarePants]

private

[JimBobSquarePants]

Let's move this.

[JimBobSquarePants]

This will become Rgb24 yes?

[antonfirsov]

It will delegate into a method of JpegDecoderCore which can then decide what pixel type to use in the resulting Image<T>. For grayscale jpegs we can return Image<Gray8> (if worth).

Designing the code sharing between the specialized (generic) and the "native pixel type" (non-generic) methods is an interesting question. Probably even more challenging for PngDecoder.

[JimBobSquarePants]

That makes sense. Thinking about it shouldn’t be too bad for png either.

[JimBobSquarePants]

These properties can all be private fields now unless you're expecting inheritance.

[antonfirsov]

Cool, thanks for checking! This code is not production ready in any way, I just made the fastest possible route to make the POC work. I will start from scratch and/or rebase.

[antonfirsov]

@JimBobSquarePants I wonder whether I should keep the name ResizeProcessor<TPixel> instead. For certain processors there will be no pixel-agnostic variant, therefore it might be better to use a uniform naming.

[JimBobSquarePants]

I prefer that over Implementation. File name can be ResizeProcessorTPixel.cs I think and pass stylecop naming rules.