Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an allocation-free alternative to Image.Pixels #202

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Slko
Copy link
Contributor

@Slko Slko commented Sep 12, 2021

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.

@eXpl0it3r eXpl0it3r added the Feature New functionality or extending of an existing one label Sep 15, 2021
Copy link
Contributor

@DemoXinMC DemoXinMC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything that we can use as a stop-gap until we transition to having access to Span strikes me as a positive.

@DemoXinMC
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New functionality or extending of an existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants