Pixel processing breaking changes & API discussion

@saucecontrol

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

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.

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 :/

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

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.

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?

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. 😉

@ptasev

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.

@br3aker

@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
}

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

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.

@Sergio0694

I'm still unable to overcome my main dilemma around how to move on with the "enforce contiguous image buffers" feature. I believe that maintaining our flexibility is a very important strategic requirement, so we can't just bake this setting into MemoryAllocator which normally has to be a singleton, controlling every buffer instantiation in an app, thus not only the images which have to be contiguous. Therefore, the enforcement of contiguous image buffers should be controlled by contextual settings.

This leaves us two options:

Extend Configuration with buffer options like PreferContiguousImageBuffers or settings to enforce buffer padding for advanced GPU interop scenarios
Define a separate BufferSettings type to carry these options as described in Pixel processing breaking changes & API discussion #1739 (comment)

The argument against doing 1. is that currently it's quite inconvenient to create custom Configuration instances. What I mean is that an ideal API usage would look like this:

using var image = new Image<Rgba32>(4096, 4096, new Configuration { PreferContiguousImageBuffers = true });

This is currently not possible, since the Configuration constructor will return a dumb configuration instance without the necessary codec setup.

This leaves us two sub-options for the case if we extend Configuration:

A: The default constructor should fill all the codec settings the way we currently do in our internal CreateDefaultInstance method.
B: Create an overload of Clone that provides an easy way for overriding the settings, so one could do stuff like this:

public class Configuration {
    public bool PreferContiguousImageBuffers { get; set; }
    
    // This is an idea for implementing padding/stride for advanced GPU scenarios.
    // Not sure if vertical padding is needed or not.
    public Func<Size, Size> DetermineImageBufferPaddingCallback { get; set; }
    
    public Configuration Clone(Action<Configuration> configureClone);
}

using var image = new Image<Rgba32>(4096, 4096, Configuration.Default.Clone(c => { 
    c.PreferContiguousImageBuffers = true;
    c.DetermineImageBufferStrideCallback = size => RoundToNearestPowerOf2(size);
}));

@Sergio0694 @DaZombieKiller thoughts as potential users?

The argument against doing 1. is that currently it's quite inconvenient to create custom Configuration instances. What I mean is that an ideal API usage would look like this:
using var image = new Image<Rgba32>(4096, 4096, new Configuration { PreferContiguousImageBuffers = true });

Do you think it would make sense to offer a "copy constructor" for Configuration? This would result in something like:

using var image = new Image<Rgba32>(4096, 4096, new Configuration(Configuration.Default)
{
    PreferContiguousImageBuffers       = true,
    DetermineImageBufferStrideCallback = RoundToNearestPowerOf2
}));

Though it might come across as awkward since (as far as I know) no other API in ImageSharp employs this technique. I also think it might not be as "obvious" to use compared to Configuration.Default.Clone, but I suppose both approaches suffer from this when new Configuration() is available.

This leads me to think that maybe changing the parameterless constructor to assign the default settings as you suggest in sub-option A would be worthwhile. The problem is then determining whether that will be a problem for any existing code that's just doing new Configuration(), and what a replacement API for "create an empty Configuration" would look like.

Define a separate BufferSettings type to carry these options

I'm torn on this one. I feel like it complicates the API a bit too much (you now need to keep track of a whole new slew of overloads when you want control over buffering). My gut reaction is that if there were a BufferSettings type, I'd prefer something like:

using var image = new Image<Rgba32>(4096, 4096, Configuration.Default.WithBufferSettings(new()
{
    PreferContiguousImageBuffers       = true,
    DetermineImageBufferStrideCallback = RoundToNearestPowerOf2
}));

Which just looks like a variant of option 1.

I would add the BufferSettings for now. It gives us some room to refactor in the future without introducing something that works differently to the current API.

For V3 we can reevaluate our configuration APIs to best fit common patterns from MS.

After doing some initial prototyping on BufferSettings and seeing the API blowup, I switched sides. We should add the property to Configuration and ask users to override it in a copy of Configuration.Default:

Configuration configuration = Configuration.Default.Clone();
configuration.PreferContiguousImageBuffers  = true;
using var image = new Image<Rgba32>(configuration, 4096, 4096);

✔️ The code above though not perfect, still looks acceptable for users of PreferContiguousImageBuffers.
✔️ Keeps things much simpler for us internally.
✔️ Better aligned with the existing Configuration design where we try to flatten all settings into a single Configuration object.

Ok.... Let's stick with the simple approach for now. I want to revisit configuration in V3 anyway to ensure we're using patterns that match the MS way of things.

@JimBobSquarePants

@JimBobSquarePants coming from here, I think we also need an API like:

public class Image<TPixel>
{
    public void CopyPixelDataTo(Span<TPixel> destination);
    public TPixel[] CopyPixelData();
}

// same for ImageFrame

Although allocating new arrays to copy pixels to is not ideal for perf, it might be good enough or even necessary for many users.

@antonfirsov

@antonfirsov
CopyPixelDataTo is pseudo-reasonable but CopyPixelData is a footgun. We'll get blamed for allocations when users inevitably misuse it.

I have major concerns with both because both can fail with a large enough image.

We'll get blamed for allocations when users inevitably misuse it.

The typical case for this is when users forget to Dispose(). When user code is suboptimal in general, that's typically not a library-specific issue. In #1807 for example, the extra allocation also happens in the legacy System.Drawing path.

What we see in that issue, that a user new to graphics programming is desperately trying to update some legacy code to migrate away from System.Drawing. We have a strong interest helping these users by delivering a smooth migration experience.

Copying pixel data can be also important in many interop scenarios, when users can't WrapMemory() or PreferContiguousImageBuffers, so I believe that CopyPixelDataTo(span) should be an accessible method.

Accessibility of TPixel[] CopyPixelData() is less critical though, we can sacrifice it for promoting good practices. I'm fine if we update our example instead of exposing a method directly.

@antonfirsov

@antonfirsov if you're so concerned about users forgetting to call Dispose, specially now that you're about to introduce an unmanaged memory manager, I would suggest not only making the API more robust, but also introducing tools to help find memory leaks.

This is a pattern I sometimes use to find objects that were not disposed:

static class Diagnostics
{
    public static bool EnableStrictDispose {get;set;}
}

class AnyDisposableClass : IDisposable
{
    public void Dispose()
    { 
        GC.SupressFinalize(this);
    }

    ~AnyDisposableClass()
    {
        if (Diagnostics.EnableStrictDispose) throw new Exception("You forgot to dispose of me");
    }
}

I think this would have saved lots of hadaches.... because, whenever I see ImageSharp's users code around, 4 out of 5 times, I see code creating and manipulating Image objects without any sense of memory management; not usings, neither disposes.

The standard practice to support diagnostics is to implement EventCounters that can be monitored with the dotnet-counters tool.

I wouldn't say that these tools are easy to use, or that it's a trivial thing to implement and document the event counters in ImageSharp, so a class that exposes diagnostic tools right from C# might be useful:

namespace SixLabors.ImageSharp.Diagnostics
{
    public static class MemoryTracker
    {
        // doesn't count the memory that's returned to pools
        public static long TotalOutstandingUnmanagedMemory { get; }

        // Throws in finalizers
        public static bool EnableStrictDisposeWatcher { get; set; }
    }
}

This doesn't mean that we can get away without exposing some event counters the standard way, but we need to draw the line for 2.0 somewhere.

Navigation Menu

Repository navigation

Pixel processing breaking changes & API discussion #1739

Why breaking changes again?

Proposed API changes

Processing pixel rows using `Span<T>`

Sample code

Processing multiple images simultaneously

Processing pixel rows using `Memory<T>`

Getting a single pixel span to `Image<TPixel>`

API usage

Initializing `MemoryAllocator`

Limiting pool size

Feedback is welcome!

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Pixel processing breaking changes & API discussion #1739

Description

Why breaking changes again?

Proposed API changes

Processing pixel rows using Span<T>

Sample code

Processing multiple images simultaneously

Processing pixel rows using Memory<T>

Getting a single pixel span to Image<TPixel>

API usage

Initializing MemoryAllocator

Limiting pool size

Feedback is welcome!

Activity

saucecontrol commented on Aug 14, 2021

br3aker commented on Aug 14, 2021

ptasev commented on Aug 15, 2021

saucecontrol commented on Aug 15, 2021

ptasev commented on Aug 15, 2021

saucecontrol commented on Aug 15, 2021

antonfirsov commented on Aug 16, 2021

antonfirsov commented on Aug 16, 2021

JimBobSquarePants commented on Aug 17, 2021

antonfirsov commented on Aug 17, 2021

antonfirsov commented on Oct 16, 2021

DaZombieKiller commented on Oct 16, 2021

JimBobSquarePants commented on Oct 18, 2021

antonfirsov commented on Oct 28, 2021

JimBobSquarePants commented on Oct 28, 2021

antonfirsov commented on Nov 4, 2021

JimBobSquarePants commented on Nov 5, 2021

antonfirsov commented on Nov 5, 2021

vpenades commented on Dec 2, 2021

antonfirsov commented on Dec 2, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Processing pixel rows using `Span<T>`

Processing pixel rows using `Memory<T>`

Getting a single pixel span to `Image<TPixel>`

Initializing `MemoryAllocator`