New Image.WrapMemory<TPixel>(void*) overloads

[Sergio0694]

Background and Motivation

Following the conversation with @JimBobSquarePants in the C# Discord server, this proposal is about adding some new Image.WrapMemory overloads to make it easy to interoperate with native memory buffers. In particular, having this feature built-in will allow users to avoid having to roll their own custom memory manager every time they need to do this, vastly reducing the boilerplate code they need to use ImageSharp in these situations.

Proposed API

namespace SixLabors.ImageSharp
{
    public abstract class Image
    {
        public static unsafe Image<TPixel> WrapMemory<TPixel>(
            Configuration configuration,
            void* pointer,
            int width,
            int height,
            ImageMetadata metadata)
            where TPixel : unmanaged, IPixel<TPixel>;

        public static unsafe Image<TPixel> WrapMemory<TPixel>(
            Configuration configuration,
            void* pointer,
            int width,
            int height)
            where TPixel : unmanaged, IPixel<TPixel>;
            
        public static unsafe Image<TPixel> WrapMemory<TPixel>(
            void* pointer,
            int width,
            int height)
            where TPixel : unmanaged, IPixel<TPixel>;
    }
}

Usage Examples

The idea for this proposal was a developer that asked for help about how to interoperate between the BitmapBuffer type and ImageSharp, working in UWP. In particular, they wanted to apply the DetectEdges processor to a graphics buffer stored in native memory (retrieved through some WinRT APIs), without incurring in unnecessary allocations. They were particularly struggling with this since ImageSharp had no built-in way to support wrapping native buffers. Here's the solution I came up with for them, which involved using a custom UnmanagedMemoryManager<T> type (the same as the one I included in this PR, essentially). Here's the code with my solution:

public static unsafe void ProcessFrame(ProcessVideoFrameContext context)
{
    using BitmapBuffer sourceBuffer = context.InputFrame.SoftwareBitmap.LockBuffer(BitmapBufferAccessMode.Read);
    using BitmapBuffer targetBuffer = context.OutputFrame.SoftwareBitmap.LockBuffer(BitmapBufferAccessMode.Write);
    using IMemoryBufferReference sourceReference = sourceBuffer.CreateReference();
    using IMemoryBufferReference targetReference = targetBuffer.CreateReference();

    ((IMemoryBufferByteAccess)sourceReference).GetBuffer(out byte* sourcePtr, out uint sourceCapacity);
    ((IMemoryBufferByteAccess)targetReference).GetBuffer(out byte* targetPtr, out _);

    BitmapPlaneDescription bufferLayout = sourceBuffer.GetPlaneDescription(0);
    int length = bufferLayout.Height * bufferLayout.Width;

    var memoryManager = new UnmanagedMemoryManager<Rgba32>((Rgba32*)sourcePtr, length);
    using (Image<Rgba32> image = Image.WrapMemory(memoryManager.Memory, bufferLayout.Width, bufferLayout.Height))
    {
        image.Mutate(x => x.DetectEdges());
    }

    Buffer.MemoryCopy(sourcePtr, targetPtr, sourceCapacity, sourceCapacity);
}

You can see how having a built-in API to wrap a void* would make these cases much easier to work with. Such an API would allow for easy interoperation with a variety of buffer types, regardless of where they're coming from (eg. in this case, some WinRT API returning a COM interface wrapping a native buffer).

Alternative Designs

We might want to call the new APIs DangerousWrapMemory if we wanted to highlight the "unsafe" nature of these APIs. Not really sure that's necessary though since the fact an input parameter is void* already means the API could only be used when a developer is already within an unsafe context anyway.

Risks

This method is technically "not safe" since a user might accidentally use the Image instance afterwards, causing segfaults, but I'd argue that's not really a concern since in these scenarios the devs would already be dealing with pretty unsafe stuff such as COM interfaces, native buffers on their end as well, etc. It's only expected that they should also be aware of the lifetime of types they interact with in the first place.

Open questions

These APIs all act as a non-owned buffer. Should we also have some owning overloads? If so, thoughts on the signature?
For instance, we could take:

• Just a MemoryHandle
• A void* pointer + an IPinnable instance
• A void* pointer + an IDisposable instance
• A void* pointer + an Action that disposes
• A void* pointer + a custom ReleaseMemoryCallback delegate that receives the void* pointer as input
• Something else?

I'm thinking we might want to have something like:

• void*: takes just a pointer, the image doesn't perform any kind of lifetime validation on the underlying memory. Users need to ensure the image is not used after the underlying memory is unpinned or freed.
• MemoryHandle: this let's the image take ownership over pinning. That is, when the image is disposed, the handle will be released.
• void* + ReleaseMemoryCallback: full ownership, the delegate is invoked when the image is disposed, and it can do whatever logic it needs to free the buffer (it received the input pointer as input, so no need to capture it)

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 matches 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)

[john-h-k]

A void* pointer + an IPinnable instance

Can kill 2 birds with one stone by using a MemoryHandle, which can be either an IPinnable, GChandle or neither.

[codecov]

Codecov Report

Merging #1419 (0b9e4dc) into master (5601559) will decrease coverage by 0.00%.
The diff coverage is 81.25%.

@@            Coverage Diff             @@
##           master    #1419      +/-   ##
==========================================
- Coverage   83.48%   83.48%   -0.01%     
==========================================
  Files         740      741       +1     
  Lines       32559    32575      +16     
  Branches     3652     3652              
==========================================
+ Hits        27181    27194      +13     
- Misses       4665     4668       +3     
  Partials      713      713              

Flag	Coverage Δ
unittests	83.48% <81.25%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/ImageSharp/Memory/ByteMemoryManager{T}.cs	57.14% <ø> (ø)
src/ImageSharp/Memory/UnmanagedMemoryManager{T}.cs	62.50% <62.50%> (ø)
src/ImageSharp/Image.WrapMemory.cs	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f84d525...0b9e4dc. Read the comment docs.

[antonfirsov]

@Sergio0694 only concern here is that this is a very unsafe API (inherently). We should double check everything that goes on this front to make sure we choose the right thing from all alternatives.

I think there is a reason why there is no Memory(void*,int) constructor in the BCL. This doesn't necessarily mean we should apply exactly the same strict principles, but I'd really like to see more discussion and proof before we go further.

Can you fill a BCL-style API proposal for this, showing motivation, alternatives and some examples on how real life interop code would interact with such an API?

So far I only played with System.Drawing interop, and based on that experience having API that interacts with high level classes like Bitmap seemed to be a better way to go than giving the user the responsibility to fish out the pointers.

[Sergio0694]

@antonfirsov Updated the OP with a full proposal using the template from the runtime, let me know what you think! 😄

[antonfirsov]

@Sergio0694 in your example there is a lifecycle issue (locking and unlocking buffers) similar to the one in System.Drawing.Bitmap .

I believe that adding dumb Image.WrapMemory overloads is the wrong way to solve this problem. I was actually thinking about them before, but decided to stop because of all the open questions around lifecycle.

The solution should be rather a smart combination of the following approaches:

1. Helper utilities that also deal with the lifecycle. (MemoryManager base class? Static methods that also consume delegates for locking releasing the memory? Some other smart thing?)
2. Code samples in https://github.com/SixLabors/Samples demonstrating the correct way of utilizing these utils
3. Separate interop libraries for UWP, System.Drawing, Xamarin etc. building on top of the utilis in 1. Eg. something like:

public static class ImageSharpUwpExtensions
{
    // image.Dispose() will unlock the buffer
    public static Image<TPixel> LockBufferAsImage<TPixel>(this SoftwareBitmap bitmap);
}

[Sergio0694]

I agree that such an API would be considered unsafe (I mean, it also literally is unsafe for the C# compiler), but I'm not sure I understand why that would necessarily be a bad thing here. I mean, even without considering ImageSharp in particular, it's not uncommon for devs to have to do a number of unsafe things anyway. Consider that same snippet from above, we have:

• Creation of two IMemoryBufferReference instance, which has to be done while the previous object is still not disposed. We still have to managed lifetime there anyway, having to do so for ImageSharp too doesn't really change much.
• Cast to a COM interface (IMemoryBufferByteAccess). I would argue devs doing that would know what they're doing, so having to also be careful about the lifetime of an Image instance wouldn't be an issue (especially so because they're likely to be doing much worse already in the rest of the codebase, as doing COM interop from C# is already messy anyway).
• Working with pointers already (from that GetBuffer API), which already needs to only be used within the scope of the source buffer reference, otherwise you'd get the same possible segfault that ImageSharp would cause if you misuse the API.

My point is that in this scenario, I'm not sure I see how the ImageSharp API would make the code any more error prone.
I agree that having specific extensions would be nice but to me that sounds like an uphill battle. Like, you'd have to add helpers for System.Drawing. What about the UWP ones? Let's add those too. There are a number of different ways to do so though, what UWP APIs are we using? What if someone uses WinRT APIs from outside UWP? That's a separate target too. My point is that this feels like an ever increasing API surface to maintain that will inevitably still result in some devs being left out, instead of just offering a general purpose API that anyone doing interop could work with, also without the need of extra packages.

Similar issue with things such as delegates, eg. in this case you'd just be creating an unnecessary closure that you don't really need anyway. That'd likely just make the code even more complex/verbose compared to just doing the above. To recap, if we added these new overloads to ImageSharp, the original code would become as simple and straightforward as this:

public static unsafe void ProcessFrame(ProcessVideoFrameContext context)
{
    using BitmapBuffer sourceBuffer = context.InputFrame.SoftwareBitmap.LockBuffer(BitmapBufferAccessMode.Read);
    using BitmapBuffer targetBuffer = context.OutputFrame.SoftwareBitmap.LockBuffer(BitmapBufferAccessMode.Write);
    using IMemoryBufferReference sourceReference = sourceBuffer.CreateReference();
    using IMemoryBufferReference targetReference = targetBuffer.CreateReference();

    ((IMemoryBufferByteAccess)sourceReference).GetBuffer(out byte* sourcePtr, out uint sourceCapacity);
    ((IMemoryBufferByteAccess)targetReference).GetBuffer(out byte* targetPtr, out _);

    BitmapPlaneDescription bufferLayout = sourceBuffer.GetPlaneDescription(0);
    int length = bufferLayout.Height * bufferLayout.Width;

    using (Image<Rgba32> image = Image.WrapMemory<Rgba32>(sourcePtr, bufferLayout.Width, bufferLayout.Height))
    {
        image.Mutate(x => x.DetectEdges());
    }

    Buffer.MemoryCopy(sourcePtr, targetPtr, sourceCapacity, sourceCapacity);
}

I'll admit I'm a bit biased here and might be particularly lax with respect to having "dangerous APIs" in a codebase, but I do feel like in this case there are valid arguments that would make having such an API worth to have? 🤔

Thoughts? 😄

EDIT: to be clear, I have no problems to also offer an overload taking a MemoryHandle to control pinning, and one taking a delegate to also control the release/unlocking of the memory. I just think that there's value in also having a "raw" one 😊

[antonfirsov]

My main concern is the bad integration with MemoryManager. It's being IDisposable exactly for the use-cases where there is a lifecycle issue around acquiring/releasing a pointer. Working it around by implementing a thin UnmanagedMemoryManager that pushes these lock/release concerns out of scope seems like a wrong precedent.

On the other hand, I understand the concerns about how difficult and time consuming it is to provide a proper ways of integration with the current API-s, opened dotnet/runtime#44412, let's see reactions of Levi or others.

[Sergio0694]

@antonfirsov Might want to call out that this doesn't just apply to UWP in particular, but to any similar scenario too?
I fear that by just saying this somehow relates to UWP that'd automatically make the issue go way down in priority 🤣

Also note, the link to this issue gives a 404, you accidentally used https://github.com/dotnet/runtime/issues/SixLabors/ImageSharp#1419 so that failed.

[antonfirsov]

I fear that by just saying this somehow relates to UWP that'd automatically make the issue go way down in priority 🤣

Ah yeah totally valid concern, updating my OP.

[antonfirsov]

@Sergio0694 sorry, I'm not ignoring you in dotnet/runtime#44412, just want to keep it focused on the BCL stuff.

I want to point out that in your original example you are modifying context.InputFrame.SoftwareBitmap in a destructive way. Are you sure this is OK?

Based on your dotnet/runtime#44412 (comment), and #1422, I'm wondering if this is what we actually need:

public class Image
{
    public static void ProcessAsImage<TPixel>(ReadOnlySpan<TPixel> input, Span<TPixel> output, Action<IImageProcessingContext> operations);
}

(Although not possible to implement yet.)

[Sergio0694]

@antonfirsov Ahahah no worries, that makes sense!
I should've replied here actually, sorry for adding clutter there! 😅

To answer your question, yes that was by design, I was thinking the API would just work destructively on the input buffer just like normal Image APIs would when using WrapMemory. The consumer would just need to first copy the memory to the target buffer, and then work in-place over that one. I mean you'd need to still do one memory copy either way, the difference is just the ordering, really. That said I'm not 100% convinced about that static API after what Levi said in the other thread 🤔

Like, he did make a good point about our other WrapMemory APIs already technically are not safe with respect to lifetime, since they take input buffers that don't retain ownership. That is, if eg. you use WrapMemory from an input Memory that was passed to you from a caller that actually got it from MemoryPool, then that caller disposed that memory, you could end up with an Image working on a buffer that has been returned to the pool and possibly rented by someone else, causing all sorts of possible issues. Not saying that's bad, I mean, you do need to use the APIs properly, I'm just saying in light of that I'm not seeing how doing fundamentally the same with the void* overload would be that much worse? Because I think in exchange for that, having access to a full blown Image instance would give you plenty more flexibility over a single static ProcessImage API. Like, what if you wanted to save that image to disk? What if you wanted to clone it in a different pixel format? Etc.

Having an actual Image instance would give you access to the full suite of ImageSharp APIs, which that static method wouldn't.
Thoughts?

P.S.: by the way, really enjoying these API brainstorming sessions! 😄

[antonfirsov]

@Sergio0694 feel free to proceed on this. I'm happy to merge the 3 proposed variants now, since based on the feedback in the runtime issue, there is really no better way to achieve this today.

Note that many devs may end up misusing this API thanks to the fact that Memory<T> is a mystery for newcomers. By not knowing that byte[] is actually Memory<byte>, they may end up pinning a byte* unnecessarily. (#1423 (reply in thread) might be such a case, although not 100%)

As for the overloads taking IDisposable, IPinnable, ReleaseMemoryCallback etc:
I'd prefer to them in a separate issue, since it opens another can of design questions. Not being optimistic whether we can answer them.

[Sergio0694]

Note that many devs may end up misusing this API thanks to the fact that Memory is a mystery for newcomers. By not knowing that byte[] is actually Memory, they may end up pinning a byte* unnecessarily. (#1423 (reply in thread) might be such a case, although not 100%)

@antonfirsov That's... Actually concerning. And not just for this API, but also thinking about other developers that in general might be thrown off by those Memory<byte> overloads and think they can't use them with their byte[] arrays. Though I guess they could just try and VS should just show them that the code is fine. Not sure how much we can do on our end though 🤔

I guess at the very least I can add more info in the XML summary for these overloads? Like, with some additional notes on lifetime, and about checking whether one of the other overloads wouldn't work first before trying these new ones?

[antonfirsov]

I guess at the very least I can add more info in the XML summary for these overloads? Like, with some additional notes on lifetime, and about checking whether one of the other overloads wouldn't work first before trying these new ones

That would be really appreciated 👍

[JimBobSquarePants]

/// <summary>
/// <para>
/// Wraps an existing contiguous memory area of 'width' x 'height' pixels allowing viewing/manipulation as
/// an <see cref="Image{TPixel}"/> instance.
/// </para>
/// <para>
/// Please Note: Using this method does not transfer the ownership of the underlying buffer of the input <see cref="Memory{T}"/>
/// to the new <see cref="Image{TPixel}"/> instance. This means that consumers of this method must ensure that the input buffer
/// is either self-contained, (for example, a <see cref="Memory{T}"/> instance wrapping a new array that was
/// created), or that the owning object is not disposed until the returned <see cref="Image{TPixel}"/> is disposed.
/// </para>
/// <para>
/// If the input <see cref="Memory{T}"/> instance is one retrieved from an <see cref="IMemoryOwner{T}"/> instance
/// rented from a memory pool (such as <see cref="MemoryPool{T}"/>), and that owning instance is disposed while the image is still
/// in use, this will lead to undefined behavior and possibly runtime crashes (as the same buffer might then be modified by other
/// consumers while the returned image is still working on it). Make sure to control the lifetime of the input buffers appropriately.
/// </para>
/// </summary>

[JimBobSquarePants]

/// <summary>
/// <para>
/// Wraps an existing contiguous memory area of 'width' x 'height' pixels allowing viewing/manipulation as
/// an <see cref="Image{TPixel}"/> instance.
/// </para>
/// <para>
/// Please Note: This method relies on callers to carefully manage the target memory area being referenced by the
/// pointer and that the lifetime of such a memory area is at least equal to that of the returned
/// <see cref="Image{TPixel}"/> instance. For example, if the input pointer references an unmanaged memory area,
/// callers must ensure that the memory area is not freed as long as the returned <see cref="Image{TPixel}"/> is
/// in use and not disposed. The same applies if the input memory area points to a pinned managed object, as callers
/// must ensure that objects will remain pinned as long as the <see cref="Image{TPixel}"/> instance is in use.
/// Failing to do so constitutes undefined behavior and will likely lead to memory corruption and runtime crashes.
/// <para>
/// Note also that if you have a <see cref="Memory{T}"/> or an array (which can be cast to <see cref="Memory{T}"/>) of
/// either <see cref="byte"/> or <typeparamref name="TPixel"/> values, it is highly recommended to use one of the other
/// available overloads of this method instead (such as <see cref="WrapMemory{TPixel}(Configuration, Memory{byte}, int, int)"/>
/// or <see cref="WrapMemory{TPixel}(Configuration, Memory{TPixel}, int, int)"/>, to make the resulting code less error
/// prone and avoid having to pin the underlying memory buffer in use. This method is primarily meant to be used when
/// doing interop or working with buffers that are located in unmanaged memory.
/// </para>
/// </summary>

[JimBobSquarePants]

LGTM.... Well I mean, scares the crap out of me but the code is good 😛

[antonfirsov]

Product code LGTM, one nit in tests.

[antonfirsov]

Suggested change

	public unsafe void WrapSystemDrawingBitmap_FromPointer_WhenObserved()
	public unsafe void WrapSystemDrawingBitmap_FromPointer()

Nit: "observation" is a concept related to Memory<T> and IMemoryOwner, not applicable here.

Unless it ends up with a terrible code bloat, I would also try to inline the locking from BitmapMemoryManager to make it more clear what are we testing.

[Sergio0694]

What do you mean with "inline the locking"? I mean, I thought the point here was to showcase how you could have a self-contained memory manager wrapping a System.Drawing image and controlling its lifetime 🤔

[antonfirsov]

I meant inlining the logic. Today was a day of terrible typos.

[antonfirsov]

I mean: this is the case when you dont have MemoryManager. If you had one, you could just pass it (or the Memory).

[antonfirsov]

But this is really just a nit.

[Sergio0694]

Well I got busy with the other optimizations and James merged this PR before I could do this 😆
Let me know if you want me to create another PR to refactor this bit! 🙂

[JimBobSquarePants]

Feel free to but only if you have the time.