More convenient ways to work with MemoryManager<T>?

[antonfirsov]

More a discussion than anything else.

Update:
Might make sense to repurpose this discussion around accessibility of non-array backed Memory<T>. See #44412 (comment)

---

In ImageSharp we faced the following situation:

• We expose overloads Image.WrapMemory<TPixel>(Memory<TPixel>) and Image.WrapMemory<TPixel>(IMemoryOwner<TPixel>) to enable users going high-perf by wrapping existing memory buffers as Image<TPixel> so they can for example run edge detection on a video buffer, copy-free
• UWP developer wants to utilize this feature, but (despite their knowledge on pointers and other low-level stuff) has no knowledge about how to implement a MemoryManager<T> around the buffers held by SoftwareBitmap (understandably IMHO)
  • Note: a proper implementation should wrap LockBuffer and the release of BitmapBuffer within it's pin/unpin concept
• Community demands an unsafe Image.WrapMemory<TPixel>(void* ptr, int width, int height) overload , using a MemoryManager implementation that - in my opinion - goes against best practices by pushing the lifecycle management concerns out of scope.

I really don't know what is the right resolution of this situation.

/cc @GrabYourPitchforks

EDIT:
It's important to note that the issue is not specific to UWP interop. Applies to other imaging API's having a lock/release concept, for example: System.Drawing.Bitmap.LockBits, and I assume other areas outside of imaging.

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[Sergio0694]

Just for reference, regarding the original issue, just to add a couple more notes:

• That's not necessarily about developers not knowing how to implement a MemoryManager<T>, but also about how to avoid all that extra boilerplate code when dealing with these scenarios, ie. not having to reimplement that type manually every time.
• The way I viewed the custom memory manager in this case was more like an internal implementation detail. As in, as far as the user is concerned, the Image<TPixel> instance is just directly wrapping some native buffer pointing at a given void* target. The fact that it internally does so by using a thin custom MemoryWrapper<T> is really just a detail.

[GrabYourPitchforks]

Sorry - not quite sure I follow. There's not really a bulletproof way to remove the lifetime concerns whenever MemoryManager<T> is involved. Whoever holds the IMemoryOwner reference is seen as the one true owner of the memory block, but it's still up to them to attest that nobody is using the Memory<T> when they call IMemoryOwner.Dispose. The runtime doesn't really provide guardrails here.

One way to mitigate this would be never to expose a Memory<T> instance to third-party code, but instead to expose a Span<T> instance to them. The third-party plugin would implement an interface that somewhat resembles the following.

public interface IThirdPartyExtension
{
    void ProcessData(Span<byte> imageData, ImageMetadata anyAdditionalMetadataYouNeed, CancellationToken cancellationToken);
}

The caller creates a span around the native buffer then passes that span through this interface. Because the data is provided as a span, there's no way (short of using unsafe code) for the callee to hold on to the span after the method returns. Patterns like this provide a pretty solid guarantee that third-party extensions aren't still holding on to the buffer at the time the caller discards it.

[Sergio0694]

@GrabYourPitchforks Anton's concern regarding lifetime stemmed from this example I provided in the linked issue, which is the solution I wrote to help out that UWP developer that was trying to use ImageSharp to interoperate with a Win2D frame buffer. In particular, I used this approach so that the image processing logic would only work in-place, with no extra buffers:

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);
}

The issue is that here I'm essentially (ab)using MemoryManager<T> with that custom UnmanagedMemoryManager<T> class that literally just wraps a void* pointer and exposes the underlying data as a Memory<T>, so that I can pass that to ImageSharp's APIs. The proposal was to essentially hide that part and just expose an Image.WrapMemory API that would take a void* pointer and only use that memory manager internally, but the issue remains that if users forgot to dispose the image and/or tried to modify that outside of this scope, they'd like AV or cause other kind of crashes.

My idea here was that in general you'd expect developers working with code like this to be comfortable enough with manually managing lifetime of buffers and whatnot, but I can indeed agree that the API needs extreme care to avoid nasty errors, so I can understand how they're being very careful about integrating that into ImageSharp in general.

Though now that you mention it, ImageSharp already exposes wrapping APIs that take non-owned buffers (either byte[], or a Memory<T>), so I guess a very similar issue is already there anyway with respect to the library dealing with lifetimes 🤔

@antonfirsov That suggestion from Levi though gave me a potentially related idea, what if we completely flipped this proposal on its head and came up with something completely static instead, taking a Span<T> as suggested?

Just a random idea, something like (plus additional overloads):

public static unsafe void Mutate<TPixel>(
    Span<TPixel> span,
    int width,
    int height,
    Action<IImageProcessingContext> operation)
    where TPixel : unmanaged, IPixel<TPixel>
{
    fixed (TPixel* p = span)
    {
        using var manager = new UnmanagedMemoryManager<TPixel>(p, span.Length);
        using var image = Image.WrapMemory(manager.Memory, width, height);

        image.Mutate(operation);
    }
}

This would be a safer and flexible way for developers to statically use ImageSharp as an image processing library for working with arbitrary buffers, in-place? This could be applied to all those cases like in the original proposal, would be even more compact (no need to even declare the image at all), and would allow working on managed buffers as well, as a bonus. 😄

Otherwise, I'm not sure I see another solution outside of either this static approach, or the one proposed in #1419.

[GrabYourPitchforks]

The issue is that here I'm essentially (ab)using MemoryManager<T> with that custom UnmanagedMemoryManager<T> class that literally just wraps a void* pointer and exposes the underlying data as a Memory<T>, so that I can pass that to ImageSharp's APIs.

IMHO that's a legitimate use case. We have an internal type PointerMemoryManager<T> that we use for exactly this scenario. It'd be nice if we had a first-class way of creating a Memory<T> around an arbitrary pointer, but doing that would be difficult without increasing the size of each Memory<T> instance in 64-bit apps, and we're not sure if it's the right tradeoff.

[antonfirsov]

IMHO that's a legitimate use case.

@GrabYourPitchforks if it's legitimate, it should be made more accessible to developers. Currently it's very complicated to use Memory<T> backed by non-array data. The way we enable this today is practically invisible, I doubt anyone uses MemoryManager<T> except the .NET team itself and a few library developers in the wild.

It'd be nice if we had a first-class way of creating a Memory<T> around an arbitrary pointer, but doing that would be difficult without increasing the size of each Memory<T> instance in 64-bit apps, and we're not sure if it's the right tradeoff.

A Memory(void*, int) constructor that creates an internal PointerMemoryManager<T> (or similar) would solve the problem API-wise. I don't think first-class runtime support is that important, optimizations can land later.

[Sergio0694]

@antonfirsov Just to make sure I understand, here you're talking about two separate issues, right?

As in, even if .NET 6 added some new Memory(void*, int) constructor that internally created a PointerMemoryManager<T> to power that, the actual public API we'd have in ImageSharp would still have the same exact "unsafe" element to it due to scoping and lifetime of the target memory that the one in the PR I opened would have today, correct?

By that I mean, regardless of when/if the BCL will get built-in support for this new constructor in the future, if @GrabYourPitchforks says using that workaround is a legitimate usage of the MemoryManager<T> class, and also considering that the manager would be internal to ImageSharp and transparent to consumers anyway, is the proposed API surface in #1419 fine then? 🤔

Of course, with the remaining point of future consideration being to eventually swap our internal manager to create the Memory<T> to wrap with just the new API from the BCL if that was added in the future, but leaving the public API the same anyway?

[GrabYourPitchforks]

@antonfirsov We make pretty strong guarantees that creating a Memory<T> is a non-allocating operation. I wouldn't feel comfortable adding a publicly accessible ctor or other factory which allocates a PointerMemoryManager<T> under the covers. We've prototyped ways around this, but they're very hacky.

[antonfirsov]

@GrabYourPitchforks I'm closing this, since my original question about PointerMemoryManager<T> and "pushing ownership out of scope" seems to be answered now.

I'm still unhappy though that Memory<T> is practically useless for 3P library authors who need to expose lower-level memory API-s for interop scenarios. I want to open (a more specific) feature request about this later.