Pixel processing breaking changes & API discussion

[antonfirsov]

Why breaking changes again?

ImageSharp 1.0 suffers from serious memory management issues under high parallel load.

#1730 is here to deal with this, but we realized we cannot guarantee the safety of .GetPixelRowSpan(y) API-s after switching to unmanaged memory.

Although well written written user code is not at risk, missing or buggy Image<T> lifetime management can lead to memory corruption under certain circumstances. Consider the following code:

var image = new Image<Rgba32>(w, h); // missing using statement
Span<Rgba32> span = image.GetPixelRowSpan(0); // last use of image, GC is free to collect it

// GC happens here and finalizers have enough time to finish
// and potentially return image's underlying memory to the OS

span[0] = default;  // memory corruption

We cannot let a silent package update to turn performance issues and minor bugs into serious security problems, and we don't want to maintain safe-looking API-s which are in fact inherently dangerous, therefore we have to replace existing Span<TPixel> methods with delegate-based variants, which can be implemented safely.

We are about to deliver tons of bugfixes and performance improvements in our next NuGet release. We believe this will justify the breaking changes, so we decided to bump our major version number.

ImageSharp 2.0 is coming!

Proposed API changes

Processing pixel rows using Span<T>

The following methods are about to be deleted:

public class Image<TPixel>
{
-	public Span<TPixel> GetPixelRowSpan(int rowIndex);
}

public class ImageFrame<TPixel>
{
-	public Span<TPixel> GetPixelRowSpan(int rowIndex);	
}

public class Buffer2D<TPixel>
{
-	public Span<TPixel> GetRowSpan(int rowIndex);	
}

... and replaced by delegate based variants:

public readonly ref struct PixelAccessor<TPixel> where TPixel : unmanaged, IPixel<TPixel>
{
    // We expose the dimensions of the backing image
    // to make it easy to use PixelAccessor<TPixel> with static delegates.
    public int Width { get; }
    public int Height { get; }
    public Span<TPixel> GetRowSpan(int rowIndex);
}

public delegate void PixelAccessorAction<TPixel>(PixelAccessor<TPixel> pixelAccessor) where TPixel : unmanaged, IPixel<TPixel>;

public class Image<TPixel> where TPixel : unmanaged, IPixel<TPixel>
{
	public void ProcessPixelRows(PixelAccessorAction<TPixel> processPixels);
}

public class ImageFrame<TPixel> where TPixel : unmanaged, IPixel<TPixel>
{
	public void ProcessPixelRows(PixelAccessorAction<TPixel> processPixels);
}

public class Buffer2D<T> where T : unmanaged
{
	// This is an advanced API, mostly used internally.
	// We don't expect many users to interact with it.
	// Regardless, adding the Dangerous prefix seems reasonable.
 	public Span<T> DangerousGetRowSpan(int rowIndex);	
}

The delegate is responsible to make sure, that necessary bookkeeping is invoked around pixel processing (increasing SafeHandle ref counts). PixelAccessor<TPixel> has to be a ref struct to make sure that no-one saves it to the heap escaping the bookkeeping mechanisms.

Sample code

using var image = new Image<Rgb24>(256, 256);
image.ProcessPixelRows(static pixelAccessor =>
{
    for (int y = 0; y < pixelAccessor.Height; y++)
    {
        Span<Rgb24> row = pixelAccessor.GetRowSpan(y);

        // Using row.Length helps JIT to eliminate bounds checks when accessing row[x].
        for (int x = 0; x < row.Length; x++)
        {
            row[x] = new Rgb24((byte)x, (byte)y, 0);
        }
    }
});

Processing multiple images simultaneously

Things get tricky with use cases like #1666. The following code using nested delegates won't work, because PixelAccessor<TPixel> is a ref struct and can not be captured by a delegate:

- static Image<Rgba32> Extract(Image<Rgba32> sourceImage, Rectangle sourceArea)
- {
-     Image<Rgba32> targetImage = new (sourceArea.Width, sourceArea.Height);
-     int height = sourceArea.Height;
-     sourceImage.ProcessPixelRows(sourceAccessor =>
-     {
-         targetImage.ProcessPixelRows(targetAccessor =>
-         {
-             for (int i = 0; i < height; i++)
-             {
-                 // [CS1628] Cannot use ref, out, or in parameter 'sourceAccessor' inside an anonymous method, lambda expression, query expression, or local function
-                 Span<Rgba32> sourceRow = sourceAccessor.GetRowSpan(sourceArea.Y + i);
-                 Span<Rgba32> targetRow = targetAccessor.GetRowSpan(i);
- 
-                 sourceRow.Slice(sourceArea.X, sourceArea.Width).CopyTo(targetRow);
-             }
-         });
-     });
- 
-     return targetImage;
- }

To deal with this, we need to expose additional delegates and ProcessPixelRows overloads that work with multiple PixelProcessors<TPixel>-s simultaneously.

public delegate void PixelAccessorAction<TPixel1, TPixel2>(PixelAccessor<TPixel1> pixelAccessor1, PixelAccessor<TPixel2> pixelAccessor2)
        where TPixel1 : unmanaged, IPixel<TPixel1>
        where TPixel2 : unmanaged, IPixel<TPixel2>;

public delegate void PixelAccessorAction<TPixel1, TPixel2, TPixel3>(PixelAccessor<TPixel1> pixelAccessor1, PixelAccessor<TPixel2> pixelAccessor2, PixelAccessor<TPixel2> pixelAccessor3)
        where TPixel1 : unmanaged, IPixel<TPixel1>
        where TPixel2 : unmanaged, IPixel<TPixel2>
        where TPixel3 : unmanaged, IPixel<TPixel3>;

public class Image<TPixel> where TPixel : unmanaged, IPixel<TPixel>
{
	public void ProcessPixelRows<TPixel2>(Image<TPixel2> image2,
		PixelAccessorAction<TPixel> processPixels1, PixelAccessorAction<TPixel> processPixels2)
		where TPixel2 : unmanaged, IPixel<TPixel2>;

	public void ProcessPixelRows<TPixel2, TPixel3>(Image<TPixel2> image2, Image<TPixel3> image3,
		PixelAccessorAction<TPixel> processPixels1, PixelAccessorAction<TPixel> processPixels2, PixelAccessorAction<TPixel> processPixels3)
		where TPixel2 : unmanaged, IPixel<TPixel2>
	    where TPixel3 : unmanaged, IPixel<TPixel3>;	
}

// Also expose the same overloads on ImageFrame<TPixel>

Now we can implement the Extract method:

private static Image<Rgba32> Extract(Image<Rgba32> sourceImage, Rectangle sourceArea)
{
    Image<Rgba32> targetImage = new (sourceArea.Width, sourceArea.Height);
    int height = sourceArea.Height;
    sourceImage.ProcessPixelRows(targetImage, (sourceAccessor, targetAccessor) =>
    {
        for (int i = 0; i < height; i++)
        {
            Span<Rgba32> sourceRow = sourceAccessor.GetRowSpan(sourceArea.Y + i);
            Span<Rgba32> targetRow = targetAccessor.GetRowSpan(i);

            sourceRow.Slice(sourceArea.X, sourceArea.Width).CopyTo(targetRow);
        }
    });

    return targetImage;
}

Processing pixel rows using Memory<T>

There are some less-known overloads in SixLabors.ImageSharp.Advanced.AdvancedImageExtensions that allow accessing image rows through Memory<T>. We should consider adding the Dangerous prefix to them because accessing Span<TPixel> through image.GetPixelRowMemory(y).Span suffers from the same issues as image.GetPixelRowSpan(y).

public static class AdvancedImageExtensions
{
-	public static Memory<TPixel> GetPixelRowMemory<TPixel>(this Image<TPixel> source, int rowIndex);
-	public static Memory<TPixel> GetPixelRowMemory<TPixel>(this ImageFrame<TPixel> source, int rowIndex)
+	public static Memory<TPixel> DangerousGetPixelRowMemory<TPixel>(this Image<TPixel> source, int rowIndex);
+	public static Memory<TPixel> DangerousGetPixelRowMemory<TPixel>(this ImageFrame<TPixel> source, int rowIndex)
}

Although these methods are alredy hidden behind the *.Advanced namespace, I'm in favor of this change, since users can abuse them to acquire a pixel row span with image.GetPixelRowMemory(y).Span instead of the ProcessPixelRows delegate API-s.

Getting a single pixel span to Image<TPixel>

The method Image.TryGetSinglePixelSpan(out Span<TPixel>) is mostly used for interop scenarios, to acquire a Span<TPixel> that could be pinned so users can pass the image's pointer to the GPU or other image processing libraries. Not surprisingly, we have to replace this method:

public class Image<TPixel>
{
-	public bool TryGetSinglePixelSpan(out Span<TPixel> span)
+   public bool DangerousTryGetSinglePixelMemory(out Memory<TPixel> memory);
}

API usage

using var image = new Image<Rgba32>(4096, 4096);

if (!image.DangerousTryGetSinglePixelMemory(out Memory<Rgba32> memory))
{
    throw new Exception("Make sure to initialize MemoryAllocator.Default!");
}

using (MemoryHandle imageMemoryHandle = memory.Pin())
{
    IntPtr resourceHandle = Interop.AllocateSomeUnmanagedResourceToWorkWithPixelData(
        (IntPtr)imageMemoryHandle.Pointer,
        image.Width * image.Height);
    Interop.WorkWithUnmanagedResource(resourceHandle);
    Interop.FreeUnmanagedResource(resourceHandle);
}

Initializing MemoryAllocator

Update: The proposal to guarantee contiguous memory at alloactor level has been replaced by Configuration.PreferContiguousImageBuffers: #1739 (comment)

After #1730, an image of 4096 x 4096 pixels will no longer fit into a single contiguous memory buffer by default.
The following initialization code has to be executed once during startup, to make sure that an image of this size is contiguous:

Limiting pool size

MemoryAllocator.Default = MemoryAllocator.Create(
	new MemoryAllocatorOptions()
    {
        MaximumPoolSizeMegabytes= 8
    });

Feedback is welcome!

/cc @saucecontrol @Sergio0694 @br3aker @alistaircarscadden @ThomasMiz @Veriara @DaZombieKiller @TanukiSharp @ptasev

[saucecontrol]

Nice work on this! I followed the discussions you had on discord around the buffer lifetime issue, and PixelAccessor looks like a good solution. The perf improvements are certainly worth an API break, and these should be relatively niche scenarios anyway.

I question the value of DangerousTryGetSinglePixelMemory since it requires not only a contiguous memory block but also that the row padding ImageSharp chooses would have to be exactly compatible with whatever the interop target is. And of course changing MinimumContiguousBlockSizeBytes globally will have negative perf side effects throughout the library. One could use ProcessPixelRows to copy row-by-row to a compatible contiguous buffer if necessary, so there are options I suppose. Could just be a documentation issue.

Edit: Thinking through the use case of DangerousTryGetSinglePixelMemory a bit more, if someone really wants that single span and calls that method, what do you expect they'll do if it fails? They could change MinimumContiguousBlockSizeBytes, or they could allocate a contiguous buffer themselves and copy into it, or they could implement their own MemoryAllocator, etc -- but those are choices that will be difficult to explain and easy to get wrong. And what if you want to change the internal default contiguous buffer size such that a call that always succeeds today would fail tomorrow? Users will still consider that a break, even if the contract never guaranteed the success initially. And again, if the call succeeds, how would a user know whether the row padding/stride is what they need it to be? And what if you wanted to change that padding for perf reasons (think 64-byte alignment for AVX-512 pixel mashing in .NET 8 or 9 or 10 or whenever)?

I wonder if a LockBits-style API would be better for that use case. A method that takes a stride (and optionally an area of interest) would allow you to return the existing Memory if it's already a compatible single span or to allocate and copy if it's not, exactly as GDI+ does. That's easier for the user and gives you more freedom to change the internals in future, up to and including making the materialization of pixels completely lazy.

[br3aker]

First of all, awesome stuff!

I think that pixel frame width should be exposed explicitly. Let's say we have some watermark applience or general 'draw image over image' code. What if 'base' image is smaller than the one we are drawing over? While we can check it before actually calling any accessor-related methods I think it would promote ugly code.

Instead of doing something like this:

image.ProcessPixelRows(Operation);

public static void Operation(PixelAccessor<TPixel> pixelAccessor) 
{
    // some checks and/or size-related code
    
    // actual operation
}

Users would need to do it like this:

// some checks and/or size-related code
image.ProcessPixelRows(Operation);

public static void Operation(PixelAccessor<TPixel> pixelAccessor) 
{
    // actual operation
}

So we either leak operation-related checks/code to outer scope (we would also need to copy-paste those everywhere operation is used) or wrap ProcessPixelRows() call with another delegate.

We also can do int width = sourceAccessor.GetRowSpan(0).Length; but that is just strange :/

[ptasev]

My knowledge is limited in some respects here so forgive me if my suggestion is not valid. It's a shame that ease of use has to be reduced for the library to protect against code with missing/buggy lifetime management.

If PixelAccessor<TPixel> is made to hold a private reference to the underlying Image<TPixel>, then wouldn't that be enough to tell the GC not to dispose the image? Then the PixelAccessor can be returned directly. Especially since PixelAccessor is a ref struct then it acts very similarly to Span, but makes sure that the image is alive. Please correct me if I'm misunderstanding something.

In the case that image still gets disposed by something else during access to PixelAccessor and row Span, then I'm assuming PixelAccessor can manage the necessary bookkeeping such that it keeps reference to the unmanaged memory alive as mentioned in OP that the delegates would do. This is probably the part I understand the least though. I'm assuming this is the tough part. How to actually implement the bookkeeping in PixelAccessor? Maybe that doesn't make sense given API considerations that need to be made to make it possible.

public readonly ref struct PixelAccessor<TPixel> where TPixel : unmanaged, IPixel<TPixel>
{
    // holds private reference to underlying image
    public int Height { get; }
    public Span<TPixel> GetRowSpan(int rowIndex);
}

public class Image<TPixel>
{
    public PixelAccessor<TPixel> GetPixelAccessor();
}

[saucecontrol]

That would have the same issue, as the last use of PixelAccessor may come well before the last use of the Span it gives out. This could be mitigated somewhat by making PixelAccessor implement Dispose, but then a missing using would re-introduce the problem, so nothing is gained.

var image = new Image<Rgba32>(w, h);      // missing using statement
var accessor = image.GetPixelAccessor(0); // missing using statement
Span<Rgba32> span = accessor.GetPixelRowSpan(0); // last use of accessor, and therefore its Image field

// GC happens here and finalizers have enough time to finish
// and potentially return image's underlying memory to the OS

span[0] = default;  // memory corruption

To be clear, if the Span were holding a reference to managed memory, that would be sufficient to keep that managed memory alive until the last use of the Span. But when a Span is backed by a native pointer, there must be a corresponding managed reference held somewhere to keep the memory wrapper alive. It's that managed wrapper that is meeting an early demise in this scenario, leaving the Span (which in this case is no better than a naked pointer) abandoned.

[ptasev]

Ah right, seems I missed the obvious that the accessor could be cleaned early too. Does the fact that it is also a ref struct change this though? Would the GC still clean it up early given that it's allocated on the stack? Can anything be exploited about that to make this work?

[saucecontrol]

Yeah, it's a bit of a brain twister. A ref struct instance is still a local variable, so it has the same lifetime rules that a local Image variable would.

The only way to prolong the PixelAccessor's life sufficiently would be to move its last access to be later than the last access to the Span it hands out. A using would do this with its implicit call to PixelAccessor.Dispose at the end of the variable's scope, but if we could count on that, we could count on Image.Dispose and skip the whole mess. 😉

[antonfirsov]

Thanks everyone for the valuable input! I will answer one by one.

@ptasev the implementation of ProcessPixelRows() could look more ore less like this:

public void ProcessPixelRows(PixelAccessorAction<TPixel> processPixels)
{
    var accessor = new PixelAccessor<TPixel>(this);
    SafeHandle[] handles = this.CollectUnderlyingMemoryHandles();

    IncreaseSafeHandleReferenceCounters(handles);

    processPixels(accessor);

    DecreaseSafeHandleReferenceCounters(handles);
}

This should protect against memory corruption even if the user does something like this:

image.ProcessPixelRows(a => {
  var span = a.GetRowSpan(0);
  image.Dispose(); // Dispose will not free the memory handle here, it can only happen after leaving ProcessPixelRows() (this is where SafeHandle refcount can reach 0)
  span[0] = default; // saves your ass from hard memory corruption, although (thanks to pooling) this can be a write  to some unrelated (but ImageSharp controlled) memory area
});

Unlike with other approaches, with the delegate trick there is no way to escape the bookkeeping. @saucecontrol hope I'm getting this right and not missing some tiny detail.

[antonfirsov]

@br3aker I updated the proposal to also include PixelAccessor<T>.Width, thanks for the ideas!

@saucecontrol regarding single pixel Span/Memory feature:
although your concerns on padding are valid, there is some proof that people actually use it in it's current form (#1307 #1375 for example). Not providing an equivalent solution would be feature-removal, which is no-go for me.

Note that #1487 briefly touches on the topic of padding already. @DaZombieKiller @Sergio0694 any thoughts on the usability of the current (non-padded) single span extraction, and future options to enable padding?

LockBits is an interesting idea, but it seems to come with a performance penalty similar to other options, and also with unnecessarily complicated API and implementation.

What we really need here is locality. ("Just do this for this particular image!") I'm considering something like this now:

public class BufferSettings
{
    // With the current pool implementation, there will be no pooling for image buffers when this is set to true.
    // A future multi-bucket pool can transparently fix this.
    public bool PreferContiguousImageBuffers { get; set; }
    
    // Add padding / alignment constraints in future releases.
}

public class Configuration
{
    public bool BufferSettings { get; set; } // So that an Image can carry around the requested BufferSettings in Image.Configuration
}

public class Image<T>
{
    public Image(int width, int height, BufferSettings bufferSettings); // Copy Configuration.Default, but override BufferSettings
}

public class Image
{
    public static Image<TPixel> Load<TPixel>(Stream stream, BufferSettings bufferSettings); // Copy Configuration.Default, but override BufferSettings
}

[JimBobSquarePants]

I wonder whether we should simply implement a two flavors of the allocator rather than additional configuration options?

[antonfirsov]

I wonder whether we should simply implement a two flavors of the allocator rather than additional configuration options?

This is what my original MemoryAllocator.Create proposal does by defining buffer constraints on (the preferably global) MemoryAllocator.

I'm trying to figure out what is the most future-proof API, considering possible future multi-bucket optimizations, padding/alignment options, #1487 and similar. Not every buffer allocated by Allocate2D<T>(...) has to be contiguous / padded in non-default way etc. BufferSettings could give us the the freedom to limit these kind of constraints only to image buffers where it has been explicitly requested.

On the other hand, switching out the global allocator might be just good enough for (mostly sequential) desktop applications working with the GPU.