SFML.Net icon indicating copy to clipboard operation
SFML.Net copied to clipboard

Add an allocation-free alternative to Image.Pixels

Open Slko opened this issue 4 years ago • 1 comments

Currently, there are two ways to access the pixels of an Image.

The first one is Image.GetPixel(), which does a call to a native function on each invocation, which is pretty bad for performance if you do it millions of times per second.

The other one is Image.Pixels
It allocates a new pixel buffer on each getter access. Property accessors should never do anything heavy inside of them (according to common sense and the official design guidelines), you can easily shoot yourself in the foot by writing a naïve code like this:

for (int y = 0; y < image.Size.Y; y++)
{
    for (int x = 0; x < image.Size.X; x++)
    {
        // Do something with image.Pixels[(y * image.Size.X + x) * 4]
    }
}

Which seems faster than calling Image.GetPixel(), but is actually O(W * H) allocations of O(W * H) bytes!

This pull request adds a new allocation-free and more performant alternative, which copies the internal pixel buffer to a user-provided byte array, and is a safer alternative to #138.

And I would also recommend you to deprecate Image.Pixels for the reasons stated above.

Slko avatar Sep 12 '21 00:09 Slko

Upon further review (yes, this PR is 2 years old), I don't like how this is implemented.

Slko is correct that Pixels should not be allocating and then returning a byte[], but fixing this would be a breaking change. Ultimately, the Pixels shouldn't exist as a property. CA1819: Properties should not return arrays. There are no redeeming qualities to Pixels continuing to exist other than avoiding breaking changes.

I would rather see this as byte[] Image.GetPixels() which would allocate the correct size buffer based on the image size, and void Image.GetPixels(byte[]) that would fill the provided byte[] rather than allocating for itself.

There's also an argument to be made that these functions should be working with Color[] instead of byte[] and have separate methods (.GetPixelBytes() and .GetPixelBytes(byte[]) or similar) for working with pixels on raw level. .GetPixel(int,int) and .SetPixel(int,int,Color) already work with Color.

This can also all be simplified once we have access to Span<T> as we would avoid copying back and forth from managed to unmanaged memory.

DemoXinMC avatar Jan 03 '24 16:01 DemoXinMC