Add a base Win2D-compatible canvas effect type

[Sergio0694]

Note: this is a follow up to #417.

Description

This issue tracks designing and adding a base type that would allow developers to "package" their own custom Win2D-compatible effects. This is crucial to allow consumers of those effect to benefit from an easy to use API surface that's abstracted from the internal implementation details of the effect. For instance, consumers shouldn't need to see or directly use transform mappings or constant buffers. Instead, the packaged effect would expose the correct properties to modify the effect-specific state in a nice way.

Proposed API (work in progress)

namespace ComputeSharp.D2D1.Uwp;

public abstract partial class CanvasEffect : ICanvasImage // ICanvasImageInterop
{
    protected abstract ICanvasImage CreateCanvasImage();
    protected abstract void ConfigureCanvasImage();

    protected void InvalidateCanvasImage(InvalidationType invalidationType = InvalidationType.Update);
    protected void SetAndInvalidateCanvasImage<T>([NotNullIfNotNull("value")] ref T storage, T value, InvalidationType invalidationType = InvalidationType.Update);

    protected enum InvalidationType
    {
        Creation,
        Update
    }
}

A few key points:

• The GetBounds methods are simply forwarding the logic to the wrapped ICanvasImage object
• The ICanvasImageInterop is internal and explicitly implemented, as it's a COM interface and not a public API.
• CreateCanvasImage is used to create the effect graph from the implementing type. CanvasEffect takes care of caching this.
• InvalidateCnavasImage is used by derived types to signal when the effect graph is no longer valid, and the cached value should be discarded. This will cause CanvasEffect to call CreateCanvasImage again the next time an image is needed.

Example use

Here's a very simple wrapper packaged effect:

public sealed partial class SampleEffect : CanvasEffect
{
    private GaussianBlurEffect? blurEffect;

    protected override ICanvasImage CreateCanvasImage()
    {
        this.blurEffect?.Dispose();

        this.blurEffect = new GaussianBlurEffect();

        return this.blurEffect;
    }

    protected override void ConfigureCanvasImage()
    {
        this.blurEffect.BlurAmount = this.blurAmount;
    }

    private float blurAmount;

    public float BlurAmount
    {
        get => blurAmount;
        set => SetAndInvalidateCanvasImage(ref blurAmount, value);
    }
}

[rickbrew]

I think this

using var _ = GetCanvasImageLock();

is an anti-pattern: it's way too easy to forget to do this, and also very easy to get confused as to when it's necessary. What race conditions are you worried about? These things are generally only used single-threaded, and if the D2D factory is created multithreaded then you're generally fine. And if you are using a CanvasEffect from multiple threads then you kinda deserve what you get.

It might be better to establish a pattern like with I use in Paint.NET's GpuImageEffect. Translated to CanvasEffect, you would have 2 invalidation methods: one that invalidates the entirety of the wrapped ICanvasImage (dispose it, recreate it), and another that just gives the opportunity to update the ICanvasImage. Or maybe just have 1 method with an enum.

public enum CanvasEffectInvalidationType // or, you know, some better name could be chosen
{
    Recreate,
    Update
}

public abstract unsafe class CanvasEffect : ICanvasImage // ICanvasImageInterop
{
    ...
    protected abstract ICanvasImage CreateCanvasImage();
    protected virtual void UpdateCanvasImage();

    protected void InvalidateCanvasImage(CanvasEffectInvalidationType invalidationType = CanvasEffectInvalidationType.Recreate);
    ...
}

When creating the ICanvasImage for the first time, or when responding to CanvasEffectInvalidationType.Recreate, the base class would call CreateCanvasImage and then immediately call UpdateCanvasImage(). When only updating it, only UpdateCanvasImage() would be called.

Then when implementing the derived sample effect, you'd have it like this:

public sealed partial class SampleEffect : CanvasEffect
{
    private GaussianBlurEffect? blurEffect;

    protected override ICanvasImage CreateCanvasImage()
    {
        this.blurEffect = new GaussianBlurEffect();
        return this.blurEffect;
    }

    protected override void UpdateCanvasImage()
    {
        this.blurEffect.BlurAmount = this.blurAmount;
    }

    private float blurAmount;

    public float BlurAmount
    {
        get => this.blurAmount;

        set
        {
            this.blurAmount = value;
            InvalidateCanvasImage(CanvasEffectInvalidationType.Update);
        }
    }
}

This is much simpler to work with. You don't have to worry about checking whether this.blurEffect is null in every property setter. Trust me this would become arduous and error prone when you have many properties.

This pattern is also much more performant when you have a complicated effect graph. Recreating the graph every time one little property changes would be very expensive. And the pattern from your code above adds a little complexity to every location that wants to modify the ICanvasImage. Seems better to centralize all that into one method.

[rickbrew]

Also, a derived class does not have to implement UpdateCanvasImage. It can always invalidate the whole canvas image and recreate it from scratch every time. This provides a good implementation path from "I just need to get it to work" to "and now I want it to be optimal."

[Sergio0694]

Right. Yeah maybe the thread-safety thing was not actually needed 🤔
I like the idea of the two methods and how much simpler they'd make the code. I think we can actually make that even simpler with more helpers. First though: I'm concerned about the invalidate image method, wouldn't that just refresh all parameters every single time even just one is changed? I guess it's just not enough of a performance concern?

Also I'm thinking it'd be nice to have a SetAndInvalidateCanvasImage method, that sets the backing field and then automatically invalidates the image only if the value has changed (effectively like SetProperty in MVVM scenarios, which is widely used).

What about something like this:

public enum CanvasEffectInvalidationType
{
    Creation,
    Update
}

public abstract unsafe class CanvasEffect : ICanvasImage // ICanvasImageInterop
{
    protected abstract ICanvasImage CreateCanvasImage();
    protected virtual void UpdateCanvasImage();

    protected void InvalidateCanvasImage(CanvasEffectInvalidationType invalidationType = CanvasEffectInvalidationType.Update);
    protected void SetAndInvalidateCanvasImage<T>(ref T storage, T value, CanvasEffectInvalidationType invalidationType = CanvasEffectInvalidationType.Update);
}

Then you could do:

public sealed partial class SampleEffect : CanvasEffect
{
    private GaussianBlurEffect? blurEffect;

    protected override ICanvasImage CreateCanvasImage()
    {
        this.blurEffect = new GaussianBlurEffect();
        return this.blurEffect;
    }

    protected override void UpdateCanvasImage()
    {
        this.blurEffect.BlurAmount = this.blurAmount;
    }

    private float blurAmount;

    public float BlurAmount
    {
        get => this.blurAmount;
        set => SetAndInvalidateCanvasImage(ref this.blurAmount, value);
    }
}

And then I guess next year once C# 12 lands:

public sealed partial class SampleEffect : CanvasEffect
{
    private GaussianBlurEffect? blurEffect;

    protected override ICanvasImage CreateCanvasImage()
    {
        this.blurEffect = new GaussianBlurEffect();
        return this.blurEffect;
    }

    protected override void UpdateCanvasImage()
    {
        this.blurEffect.BlurAmount = this.blurAmount;
    }

    public float BlurAmount
    {
        get => field;
        set => SetAndInvalidateCanvasImage(ref field, value);
    }
}

Which is very very nice 😄

What do you think?