Support decoding into an existing Memory<T>

[DaZombieKiller]

The Problem

There is currently no built-in API that enables a decoder to decode directly into a Memory<T> instance. This can complicate matters when you want to use ImageSharp to load images into a third party API (such as the Unity game engine's Texture2D).

For additional context, consider the following example:

static Texture2D LoadImage<TPixel>(Configuration configuration, Stream stream)
    where TPixel : unmanaged, IPixel<TPixel>
{
    // Identify the image without loading it so we can create the Texture2D with an appropriate size.
    var startPosition = stream.Position;
    IImageInfo info   = Image.Identify(configuration, stream);
    stream.Position   = startPosition;
    var texture       = new Texture2D(info.Width, info.Height, GetTextureFormat<TPixel>(), mipChain: false);

    // Unity allows access to the pixel data using GetPixelData<T>, which can
    // then be wrapped into a Memory<T> instance using a custom MemoryManager.
    NativeArray<TPixel> pixels = texture.GetPixelData<TPixel>(mipLevel: 0);

    // As there is no API to either re-use an existing Image or decode directly
    // into a Memory<T>, we are required to allocate an entirely new Image and
    // copy it into another Image that wraps our memory, which is expensive.
    using var srcImage = Image.Load<TPixel>(configuration, stream);
    using var dstImage = Image.WrapMemory(configuration, pixels.AsMemory(), info.Width, info.Height, info.Metadata);

    if (srcImage.TryGetSinglePixelSpan(out Span<TPixel> src) &&
        dstImage.TryGetSinglePixelSpan(out Span<TPixel> dst))
    {
        src.CopyTo(dst);
        texture.Apply();
    }

    return texture;
}

The third party API offers no control over the memory that is allocated for the pixel data, which forces the code to incur the cost of a copy operation.

The Proposal

A possible solution for this could be to expose an ITargetImageDecoder interface, which allows a decoder to expose an API for decoding directly into an existing Image<T> instance.

namespace SixLabors.ImageSharp.Formats
{
    public interface ITargetImageDecoder
    {
        void Decode<TPixel>(Stream stream, Image<TPixel> image, Rectangle? region)
            where TPixel : unmanaged, IPixel<TPixel>;

        Task DecodeAsync<TPixel>(Stream stream, Image<TPixel> image, Rectangle? region, CancellationToken cancellationToken)
            where TPixel : unmanaged, IPixel<TPixel>;
    }
}

namespace SixLabors.ImageSharp.Advanced
{
    public partial static class AdvancedImageExtensions
    {
        public static void Load<TPixel>(this Image<TPixel> image, Stream stream)
            where TPixel : unmanaged, IPixel<TPixel>;

        public static void Load<TPixel>(this Image<TPixel> image, Stream stream, out IImageFormat format)
            where TPixel : unmanaged, IPixel<TPixel>;

        public static void Load<TPixel>(this Image<TPixel> image, Stream stream, ITargetImageDecoder decoder)
            where TPixel : unmanaged, IPixel<TPixel>;

        public static Task LoadAsync<TPixel>(this Image<TPixel> image, Stream stream, CancellationToken cancellationToken = default)
            where TPixel : unmanaged, IPixel<TPixel>;

        public static Task LoadAsync<TPixel>(this Image<TPixel> image, Stream stream, ITargetImageDecoder decoder, CancellationToken cancellationToken = default)
            where TPixel : unmanaged, IPixel<TPixel>;

        public static Task<IImageFormat> LoadWithFormatAsync(this Image<TPixel> image, Stream stream, CancellationToken cancellationToken = default)
            where TPixel : unmanaged, IPixel<TPixel>;
    }
}

Open Questions

• What should happen if a decoder does not implement ITargetImageDecoder?
  • Ideally the decoder would take a slow path and copy.
• What happens if the load operation changes the image's size?
  • In the PR it was suggested that it should throw if the Image<TPixel> is too small for the new data.
  • This case is not relevant to me, so I don't have much of an opinion on it.
• Should animated/multi-frame image formats be supported?
  • GIFs would result in NotSupportedException in the PR implementation.
• It would be very useful for this API to support decoding into a region of an image. What would be the best way to expose this?
  • It was suggested previously to add a Point? or Rectangle? parameter to Decode/DecodeAsync, with initial implementations throwing when it's non-null. Would this be sufficient?
• I'm still not certain of what to name the interface. Other potential names could be:
  • IImageDecoder
  • IImageInstanceDecoder
  • IImageTargetDecoder
  • IAdvancedImageDecoder

[JimBobSquarePants]

I don't like this.

Basically the premise is backwards. If you want a texture you should be creating that from the decoded image not the other way around.

You have access to the decoded image pixel buffer so why can you not use that?

[DaZombieKiller]

@JimBobSquarePants I may not have been clear enough with my description in the OP. This is primarily about avoiding the need to perform a copy when I could just decode directly into the pixel buffer that's been allocated by the third party API.

[JimBobSquarePants]

That's exactly what I am saying is backwards. There should be a way to create a texture from the decoded image not the other way around.

[DaZombieKiller]

I do agree that it seems backwards, but unfortunately there's no API for that offered by Unity. It allocates its own buffers and merely gives you access to them. If there's no chance of this getting into ImageSharp then I can make a feature request to Unity instead, although I fear that'd be far less likely to happen.

[DaZombieKiller]

I decided to take one last look and it appears that there is actually a SetPixelData method in Unity's Texture2D class that I can use to do this. I must have skimmed over it before, as it was only added pretty recently according to documentation. With that method I can now write the following:

unsafe static Texture2D LoadImage<TPixel>(Configuration configuration, Stream stream)
    where TPixel : unmanaged, IPixel<TPixel>
{
    using var image = Image.Load<TPixel>(configuration, stream);
    var texture     = new Texture2D(image.Width, image.Height, GetTextureFormat<TPixel>(), mipChain: false);

    if (image.TryGetSinglePixelSpan(out Span<TPixel> span))
    {
        fixed (TPixel* address = span)
        {
            var array = NativeArrayUnsafeUtility.ConvertExistingDataToNativeArray<TPixel>(address, span.Length, Allocator.None);
        #if ENABLE_UNITY_COLLECTIONS_CHECKS
            NativeArrayUnsafeUtility.SetAtomicSafetyHandle(ref array, AtomicSafetyHandle.Create());
        #endif
            texture.SetPixelData(array, mipLevel: 0);
            texture.Apply();
        }
    }

    return texture;
}

I apologize for any time wasted on this!

[JimBobSquarePants]

Aha! Fantastic!

That's great news and absolutely no need to apologize for any time wasted!

On a side note SetAtomicSafetyHandle may be by favorite method name ever. Reads like something from a 1950s sci-fi 😄

[Sergio0694]

I was actually just about to open this same exact discussion, then I saw this 😄
@JimBobSquarePants I think we absolutely should expose APIs for this, and the solution found by @DaZombieKiller doesn't actually solvee the issue at all. The code in the last comment is still performing an unnecessary copy, which can (and should) be avoided.

The way I see it, there are two main reasons why such an API should be public:

• It's conceptually the symmetric equivalent for Image.WrapMemory. We exposed that, so it just seems logical to expose this too.
• It allows users to directly decode an image into a 3rd party buffer, with no extra copy.

I have this issue in ComputeSharp, but really the same applies to many scenarios. In my case, since I'm working with DirectX 12 buffer types, I have the following scenario when loading from disk to a GPU resource, or when going back from GPU resource to disk:

• To GPU: image.jpg -> Image.Load -> (1) copy to temporary GPU buffer (upload) -> (2) copy to target GPU buffer
• From GPU: GPU buffer -> (1) copy to temporary buffer (readback) -> Image.WrapMemory -> image.jpg

NOTE: you can't directly write to GPU buffers that are shaders visible. You need to first go through either an "upload" buffer when copying data to GPU, or a "readback" buffer when reading data back from GPU. These temporary buffers can then be CPU mapped.

As you can see, I have just one copy when reading back, as I can do Image.WrapMemory directly over the CPU mapped readback buffer, and then save that image to disk. But, not when writing data to CPU. Right now you have to first load the image, then copy (1) to the temporary GPU buffer, then from there copy (2) to the target GPU buffer. This is an entire extra copy of the whole image, which especially when working with large (eg. 4K) images is far from ideal. With the proposed APIs instead, we could do this:

• To GPU: create temporary GPU buffer -> Image.WrapMemory -> Image.Load (instance) -> (1) copy to target GPU buffer

This would completely skip one copy of the entire image, as ImageSharp would be effectively decoding the image directly over the temporary GPU buffer (since we can use Image.WrapMemory to create an Image instance wrapping that buffer). This would also avoid renting temporary buffers for the intermediate image entirely as well, since we'd never actually need that at all.

All in all, this would be a greatly useful API for consumers working with 3rd party APIs and interested in high performance, that would fix a problem that cannot be otherwise fixed with the currently available APIs. Furthermore, it also feels like a missing API anyway, due to the fact that its symmetric equivalent Image.WrapMemory already exists. I think we definitely should do this, and I'd be happy to help 😄

[JimBobSquarePants]

Just be aware that there are API limitations that come with this. Any operations that require the buffer to be swapped (anything implementing ICloningImageProcessor<TPixel> or any convolution based processors) will throw an InvalidMemoryOperationException since the memory is not owned by us.

[Sergio0694]

or any convolution based processors

@JimBobSquarePants Wait, that's not actually true though, no? 🤔
If you create an image with Image.WrapMemory, convolution effects (eg. the bokeh blur) still work just fine, even though the buffer in use is not owned, but just consumed. Were you thinking of something else or am I missing something here?

[JimBobSquarePants]

I’m sure some of them call the Buffer2D swap method. I’d have to double check.

[antonfirsov]

The trick is in the SwapOrCopyContent method:

ImageSharp/src/ImageSharp/Memory/DiscontiguousBuffers/MemoryGroup{T}.cs

Lines 174 to 179 in a1784a6

	/// <summary>
	/// Swaps the contents of 'target' with 'source' if the buffers are allocated (1),
	/// copies the contents of 'source' to 'target' otherwise (2).
	/// Groups should be of same TotalLength in case 2.
	/// </summary>
	public static bool SwapOrCopyContent(MemoryGroup<T> target, MemoryGroup<T> source)

Cloning processors will do a copy in case if memory is not allocated by MemoryAllocator. If the processor is altering the size of the image, an exception will be thrown:

ImageSharp/src/ImageSharp/Memory/DiscontiguousBuffers/MemoryGroup{T}.cs

Lines 189 to 193 in a1784a6

	if (target.TotalLength != source.TotalLength)
	{
	throw new InvalidMemoryOperationException(
	"Trying to copy/swap incompatible buffers. This is most likely caused by applying an unsupported processor to wrapped-memory images.");
	}

Which will be eventually wrapped into an ImageProcessingException :

ImageSharp/src/ImageSharp/Processing/Processors/CloningImageProcessor{TPixel}.cs

Lines 76 to 78 in a1784a6

	catch (Exception ex)
	{
	throw new ImageProcessingException($"An error occurred when processing the image using {this.GetType().Name}. See the inner exception for more detail.", ex);

[JimBobSquarePants]

Thanks @antonfirsov !

[Sergio0694]

@antonfirsov Following up there from your comments on the PR 🙂
Note: that PR was just work in progress and I just did it yesterday as a proof of concept to see what the API surface could look like. I'm aware there are details left to iron out, and I wasn't proposing that PR as ready to review and merge, it was really just an investigation 😄

I like the idea of having a Load(Async) overloads that work on an existing Image instance over the static variants in the original proposal, since they separate the concerns of wrapping the memory and loading the image. This reduces the number of overloads we need to create and maintain to NumOf(WrapVariants) + NumOf(LoadVariants) from NumOf(WrapVariants) * NumOf(LoadVariants). (The numbers will likely keep growing.) On the other hand, the user needs to know the dimensions in advance, I'm not sure if it's always possible. @DaZombieKiller? (Also see my notes in the very end.)

Not sure on this point, I could be convinced either way. The main reasons why I went for static methods was:

• Consistency with Image.WrapMemory, which as mentioned is the opposite equivalent of this API
• Making them less visible to inexperience users, as they'd not be instance methods in IntelliSense.

If the stream image size is bigger, we clearly need to throw. For other cases, I see 3 options here:

• The user wants to change dimensions (mutate)
• The user wants to decode to a sub rectangle of the target image
• The user is expecting an exception if sizes mismatch (current behavior)

We were discussing this in the Discord server as well, and point number 2 would actually be pretty handy especially when working with GPU textures. The main issue is that those are row-aligned (both in DX12 and Vulkan), so the memory isn't guaranteed to be contiguous. This means you'd necessarily have to have basically 3 copies to load an image and put it into a 2D texture:

• Load/Decode -> Copy into GPU staging buffer -> Copy to GPU

If we went with "The user wants to decode to a sub rectangle of the target image" instead, it would be possible to do:

• Identify an image and get its size
• Calculate the staging buffer size after row padding
• Create the staging buffer already, and CPU-map it
• Use Image.WrapMemory to create an image directly over the staging buffer
• Load/Decode the image over that buffer. The image would be decode aligned to the left (so ignoring padding)
• Copy to GPU

This means we'd now have:

• Load/Decode directory to GPU staging -> Copy to GPU

So effectively removing one copy operation entirely.
This seems like it could be a very powerful and flexible feature. Just not entirely sure about the API design here, as in, would we need to also return to the user the effective size of the decoded area? Or indicate that somewhere?

Thoughts? 🙂

[JimBobSquarePants]

Are you expecting to be able to write from a slice? I.E a left offset of the original source image?

If so, that could be a significant amount of refactoring.

[DaZombieKiller]

That's not a requirement for my current use case, but I do have other parts of my code that would benefit from it (texture atlasing code, mostly). If it's too significant of a refactor it might be best to leave it out of the initial implementation, I think.

[Sergio0694]

Are you expecting to be able to write from a slice? I.E a left offset of the original source image?

Not really, I was thinking we could just always put padding on the right, since that's the standard anyway for all kinds of GPU processing and whatnot (this is the same on both Vulkan and DX11/DX12 APIs, OpenGL, CUDA, etc.). If people need to actually load an image into a sub-region, at that point they might just use ImageSharp.Drawing 😄
Defaulting to padding on the right should be relatively easy to do, right?

[JimBobSquarePants]

Yeah I think so. We’d just have to double check we’re slicing everything correctly.

[antonfirsov]

We have Buffer2DRegion<T> to support (whatever) padding. The simplest would be to refactor decoders to query GetRowSpan from the region instead of Image<T> or Buffer2D<T>.

This is a non-trivial refactor better carried out incrementally (Jpeg & Png first). I would add a default (copying) implementation for the rest.

[DaZombieKiller]

I noticed that a while back, the pull request associated with this (#1490) was closed. Has this been implemented elsewhere or was it rejected in the end? Assuming it's still an accepted proposal, I'd be willing to contribute the changes for the feature once I have time to (in the event that nobody else takes it up).

[DaZombieKiller]

@JimBobSquarePants
I've updated the OP with the latest proposal and information sprinkled through the discussion.

[antonfirsov]

@DaZombieKiller how about the target rectangle, shouldn't ITargetImageDecoder be future-proof to enable that?

[DaZombieKiller]

@antonfirsov I had it in the "open questions" section in case there were still any qualms about how to expose that functionality, but I'll add it to the methods in the proposal too

[sunkin351]

Whatever happened to this btw? The ability to decode into a specified buffer would be reason enough to add this library to a few projects of mine.

[JimBobSquarePants]

Nobody has picked it up. As ever, we have a case of too much work to do with too few participants.