Allow pixel-specific processors

[Sergio0694]

NOTE: follow up from Discord, pinging @JimBobSquarePants, @antonfirsov, @john-h-k, @DaZombieKiller.

Overview

Opening this discussion to propose allowing 3rd party to properly implement pixel-specific pixel processors.
This would primarily involve the following changes:

• Exposing IImageProcessingContext<TPixel>
• Expose Clone and Mutate extensions taking Action<IImageProcessingContext<TPixel>>
• Expose Clone and Mutate extensions taking params IImageProcessor<TPixel>[]

The reason for this is that right now consumers of the library wanting to implement custom image processors are pretty much forced into necessarily writing pixel-agnostic ones, which is not always ideal, or doable at all without big compromises. This is particularly an issue in cases such as when doing interop with the GPU (eg. like I'm doing in ComputeSharp), because not all pixel formats are supported by the GPU.

Right now this means even when implementing a GPU-based image processor, I have to still do the pixel conversion to Vector4 on the CPU, which is much slower and completely prevents me from just passing the image on the GPU and using the implicit mapping (aka Texture2D<unorm float4> in HLSL), which would mean the GPU would take care of doing conversions for me. This also means I end up having to allocate much bigger buffers on the GPU (as in, Vector4 ones instead of just packed uint).

Pixel-format agnostic processors are absolutely great, but I do think they're not necessarily the best approach in all situations, and the current APIs are effectively causing consumers to lose type information when they're actually working with pixel-specific images. This is not ideal and can be a problem in scenarios like the one mentioned above. I feel like it'd actually make ImageSharp more flexible in general and usable in scenarios where it's not really ideal today.

Practical example

Consider this example of doing image processing with ComputeSharp, from an image loaded with System.Drawing:

Bitmap bitmap = new("cat.jpg");
BitmapData data = bitmap.LockBits(
    new Rectangle(default, bitmap.Size),
    ImageLockMode.ReadWrite,
    PixelFormat.Format32bppArgb);
Span<Bgra32> span = new(data.Scan0.ToPointer(), bitmap.Width * bitmap.Height);

try
{
    using ReadWriteTexture2D<Bgra32, Vector4> texture =
        Gpu.Default.AllocateReadWriteTexture2D<Bgra32, Vector4>(span, bitmap.Width, bitmap.Height);

    // Process the image on the GPU...

    texture.GetData(span);
}
finally
{
    bitmap.UnlockBits(data);
}

bitmap.Save("cat_processed.jpg");

You can see here that since I know for a fact the image will be loaded as BGRA32, I can just directly copy that (packed) data to GPU memory, and then have my shader work on it directly mapping those values to Vector4 values. The GPU will automatically perform the pixel conversion whenever the shader reads/writes texture elements. The CPU is doing no work at all.

This is not really doable with ImageSharp, unfortunately.
Imagine I wanted to write a custom processor to work the same way, like this:

public sealed class ComputeSharpProcessor : ImageProcessor<Rgba32>
{
    public ComputeSharpProcessor(Configuration configuration, Image<Rgba32> source, Rectangle sourceRectangle)
        : base(configuration, source, sourceRectangle)
    {
    }

    protected override void OnFrameApply(ImageFrame<Rgba32> source)
    {
        _ = source.TryGetSinglePixelSpan(out Span<Rgba32> span);

        using ReadWriteTexture2D<Rgba32, Vector4> texture =
            Gpu.Default.AllocateReadWriteTexture2D<Rgba32, Vector4>(span, source.Width, source.Height);

        // Process the image on the GPU...

        texture.GetData(span);
    }
}

And then use it like so:

using Image<Rgba32> image = Image.Load("cat.jpg");

image.Mutate(c => c.ApplyProcessor(new ComputeSharpProcessor()));

image.Save("cat_processed.jpg");

This is just not possible at all today, because all the Clone and Mutate extensions completely lose type information on the image in use (even if the image is explicitly a generic version), so you just can't insert your pixel-specific processor in there.

Summary

I think this would be a relaativey easy to implement but powerful feature to allow ImageSharp to be more flexible in these more advanced scenarios. I would be happy to help with the actual implementation if we decide to do this, of course 😄

[JimBobSquarePants]

current APIs are effectively causing consumers to lose type information when they're actually working with pixel-specific images.

What type information? Nothing is lost ever. If you want to decode to a specific pixel format, use that format.

 if (typeof(TPixel) == typeof(Rgba32)) { ... }
 else { ... }

[JimBobSquarePants]

Actually. Cloning processors work also.

public sealed class ComputeSharpCloningProcessor : CloningImageProcessor<Rgba32>, ICloningImageProcessor
{
    public ComputeSharpCloningProcessor(Image<Rgba32> source)
        : this(source.GetConfiguration(), source, source.Bounds())
    {
        // This constructor only exists to satisfy the base class
        // and ensure that the class usage is type safe.
    }

    private ComputeSharpCloningProcessor(
        Configuration configuration,
        Image<Rgba32> source,
        Rectangle sourceRectangle)
        : base(configuration, source, sourceRectangle)
    {
    }

    public ICloningImageProcessor<TPixel> CreatePixelSpecificCloningProcessor<TPixel>(
        Configuration configuration,
        Image<TPixel> source,
        Rectangle sourceRectangle) where TPixel : unmanaged, IPixel<TPixel>
    {
        if (typeof(TPixel) != typeof(Rgba32))
        {
            throw new NotSupportedException();
        }

        // We return a new instance since the source and rectangle are actually
        // the destination following cloning and chaining.
        return (ICloningImageProcessor<TPixel>)
            (object)(new ComputeSharpCloningProcessor(configuration, (Image<Rgba32>)(object)source, sourceRectangle));
    }

    IImageProcessor<TPixel> IImageProcessor.CreatePixelSpecificProcessor<TPixel>(
        Configuration configuration,
        Image<TPixel> source,
        Rectangle sourceRectangle)
        => this.CreatePixelSpecificCloningProcessor(configuration, source, sourceRectangle);

    protected override Size GetDestinationSize()
        => new Size(this.Source.Width / 2, this.Source.Height / 2);

    protected override void OnFrameApply(ImageFrame<Rgba32> source, ImageFrame<Rgba32> destination)
    {
        throw new NotImplementedException();
    }
}

class Program
{
    static void Main(string[] args)
    {
        Console.WriteLine("Hello World!");

        using var image = new Image<Rgba32>(100, 100);
        image.Mutate(x => x.ApplyProcessor(new ComputeSharpCloningProcessor(image)));
    }
}

[DaZombieKiller]

IImageProcessingContext is already public.

I think that's a typo in the OP (based on the loose `), I believe it's supposed to say:

Expose IImageProcessingContext<TPixel>

[JimBobSquarePants]

That should work for both Clone and Mutate

[Sergio0694]

What type information? Nothing is lost ever.

What I meant is that regardless of if you staart with an Image or an Image<TPixel>, when you do Mutate you always just get the untyped IImageProcessingContext, so the type information is "lost" there. You can't add pixel-specific processors in a pipeline.

Regarding your workaround, yeah I do agree that there are ways to make it work already, but that's clunky at best.
You have an empty constructor just to get build-time checks, you allocate a closure because you're capturing the image, you need to have those ugly (T)(object) casts to bypass the C# restrictions, you need to have runtime type checks and throw exceptions, you have that weird double implementation of CreatePixelSpecificProcessor, etc.
I just feel like having built-in support for this would be cleaner, less error prone and easier to implement for consumers 🤔

[JimBobSquarePants]

Oh definitely ugly as sin! I just wanted to prove it wasn’t impossible 😄

[antonfirsov]

@Sergio0694 what's the user value on utilizing the Mutate/Clone infra for this? According to your example, you most likely only want to mutate and never clone. Why don't you just define an extension method over Image<Rgba32> within your library?

In cases you want to clone the image you can implement your extension method to return a new Image<Rgba32>. I would assume you want strong control over your memory anyways, so it shouldn't be an issue.

[JimBobSquarePants]

Yeah, let's see what @antonfirsov comes up with. That's a LOT of extensions to update across both here and Drawing!!

[antonfirsov]

Well, my "idea" is quite dumb actually. I still don't think introducing pixel-specific processors is the right way to solve this.
I believe, that in this area our first priority should be to maintain simplicity and clarity, and supporting advanced scenarios only comes as second. Currently Image Processor is an abstraction for an operation that can be executed on any image regardless of the pixel type. I think it would serve our users the best if we keep this concept as-is.

My point is that - even with the above constraints - there is still space to support shader-based processors (actually in a better way IMO) by implementing multiple kernels for different pixel types and and having automatic conversion for the unknown ones :

public sealed class ComputeSharpProcessor<TPixel> : ImageProcessor<TPixel>
{
    public ComputeSharpProcessor(Configuration configuration, Image<TPixel> source, Rectangle sourceRectangle)
        : base(configuration, source, sourceRectangle)
    {
    }

    protected override void OnFrameApply(ImageFrame<TPixel> source)
    {
        _ = specific.TryGetSinglePixelSpan(out Span<TPixel> genericSpan);

        if (typeof(TPixel) == typeof(Rgba32))
        {
            Span<Rgba32> span = MemoryMarshal.Cast<TPixel, Rgba32>(genericSpan);
            Texture<Rgba32> texture = Gpu.CreateTexture(span, source.Width, source.Height);
            Kernels.MyKernelRgba32(texture);
            texture.GetData(span);
        }
        else if (typeof(TPixel) == typeof(Rgb24))
        {
            Span<Rgb24> span = MemoryMarshal.Cast<TPixel, Rgb24>(genericSpan);
            Texture<Rgb24> texture = Gpu.CreateTexture(span, source.Width, source.Height);
            
            // This kernel could be a slight alteration of "MyKernelRgba32" that starts by concerting the input pixel, and reuses the core logic
            Kernels.MyKernelRgb24(texture);
            texture.GetData(span);
        }
        // else if ...
        else
        {
            // Ideally, this won't be called often, but note that most conversions should be very fast since #1404

            using var tempBuffer = this.Configuration.MemoryAllocator.Allocate<Rgba32>(genericSpan.Length);
            Span<Rgba32> span = tempBuffer.Span;
            PixelOperations<TPixel>.ToRgba32(this.Configuration, genericSpan, span);

            Texture<Rgba32> texture = Gpu.CreateTexture(span, source.Width, source.Height);
            Kernels.MyKernelRgba32(texture);
            texture.GetData(span);

            PixelOperations<TPixel>.ToRgba32(this.Configuration, span, genericSpan);
        }
    }
}

[antonfirsov]

Then we could add new overloads to the various extensions, like this for Crop (including existing extensions) [...]

I really don't think we should ever do this. This pollutes API for users a lot, and creates maintainability hell for us.

[JimBobSquarePants]

This is the way.

Yep totally agree. This would have been my first choice approach and I only put together the class I demonstrated to prove a point.

[Sergio0694]

Right, yeah those are all very good points, I have to agree that doing this would be pretty complex 😄

I have to say I'm still not a fan of hiding the type information though - while I do understand that having a non-generic interface for pixel-format agnostic processor is super nice, I don't particularly like forcing this on consumers. Admittedly though, I might be a bit biased on this because lately I've been working quite a bit with Roslyn APIs in my project, and those are really all-in with generics everywhere, and they have this aspect of "not losing type information" all over the place, as tons and tons of methods are generic specifically to be able to return the same type as the input (when used in a fluent chain call like in ImageSharp).

As mentioned though, yeah I can see how that'd require a big number of new overloads for a feature that'd probably be used just by a small subset of users, that's fair. Still very glad we could have this conversation though! 😊