What's the fastest method to copy pixels to/from a surface

[midora]

Assuming there is a pixel array containing all pixels of the image in BGRA top-left to bottom-right order. There are many surface methods (let's exclude the obsolete one) which may be helpful. So what's the proposed way for filetype plugins.

 

Is unsafe code acceptable (like the following).

 

Spoiler

        unsafe void SurfaceFromRasterBGRA(Surface surface, byte[] rasterBGRA)
        {
            int width = surface.Width;
            int height = surface.Height;
            fixed (byte* rasterPtr = rasterBGRA)
            {
                int row = 0;
                var srcPtr = (uint*)rasterPtr;
                var srcRastercEndPtr = srcPtr + width * height;
                while (srcPtr < srcRastercEndPtr)
                {
                    var srcRowEndPtr = srcPtr + width;
                    var dstPtr = (uint*)surface.GetRowPointerUnchecked(row);
                    while (srcPtr < srcRowEndPtr)
                    {
                        *dstPtr++ = *srcPtr++;
                    }
                    row++;
                }
            }

        }

 

is the surface data locked during the operation or is there the potential risk of a crash.

 

I would prefer to use a paint.net method to copy from/to a raster but I didn't find a good method.

 

 

 

 

[MJW]

I'm not sure I know what I'm talking about, but I think you might want to use the PDN method BufferUtil.Copy():

public unsafe static void Copy(void* dst, void* src, int byteCount)

 

I'm pretty sure the surface memory is already locked down. I believe the memory for PDN Surfaces is always pinned, or whatever it's called.

[null54]

3 hours ago, midora said:

is the surface data locked during the operation or is there the potential risk of a crash.

 

The surface data is always locked.

 

The following code uses Buffer.MemoryCopy to copy an entire row at once.

 

Spoiler

unsafe void SurfaceFromRasterBGRA(Surface surface, byte[] rasterBGRA)
{
    int width = surface.Width;
    int height = surface.Height;
    
    fixed (byte* rasterPtr = rasterBGRA)
    {
        int srcStride = width * 4;
        ulong length = (ulong)srcStride;
    
        for (int row = 0; row < height; row++)
        {
            var srcPtr = rasterPtr + (row * srcStride);
            var dstPtr = surface.GetRowPointerUnchecked(row);
            
            System.Buffer.MemoryCopy(srcPtr, dstPtr, length, length);
        }
    }
}

 

 

[MJW]

26 minutes ago, null54 said:

The following code uses Buffer.MemoryCopy to copy an entire row at once.

 

Why just a row at a time? BGRA format doesn't have any padding between rows, so why not copy the whole buffer at once?

[Rick Brewster]

Surfaces don't have a concept of locking. Their location is fixed in memory and it's always completely accessible via the Scan0 pointer.

 

You can't just copy width*height elements -- you're not taking stride into account. There can be space between the rows, either due to the Surface being a window into another Surface, or for alignment reasons, or anything. Your current code will potentially cause corrupt/garbled pixels.

 

A while-loop like that is the slowest way to copy data from one buffer to another.

 

Fastest way would be Buffer.MemoryCopy, and use Surface.GetRowPointer() for each row.

[Rick Brewster]

37 minutes ago, MJW said:

 

Why just a row at a time? BGRA format doesn't have any padding between rows, so why not copy the whole buffer at once?

 

Surface has the Stride property, which defines the number of bytes between the start of each row. If you don't respect this value, your code is wrong and will do bad things.

[MJW]

5 hours ago, Rick Brewster said:

Surface has the Stride property, which defines the number of bytes between the start of each row. If you don't respect this value, your code is wrong and will do bad things.

 

I was aware of the stride, but according to Microsoft: "The stride is the width of a single row of pixels (a scan line), rounded up to a four-byte boundary. If the stride is positive, the bitmap is top-down. If the stride is negative, the bitmap is bottom-up." So for a bitmap with 32-bit pixels, the stride equals the width. I realize a Surface isn't strictly speaking a Bitmap, but in order to be aliased to a bitmap, it has to have the same format.

 

(Perhaps I'm missing some finer point, but I at least had a reasonable basis for my original statement that "BGRA format doesn't have any padding between rows.")

[midora]

No complains about unsafe code in plugins? 😉

 

I put the code in an extension method for Surfaces.

 

Spoiler

        public unsafe static void FromRasterBGRA(this Surface surface, byte[] rasterBGRA)
        {
            int width = surface.Width;
            int height = surface.Height;
            fixed (byte* rasterPtr = rasterBGRA)
            {
                int row = 0;
                long srcStride = width * 4;
                var srcPtr = rasterPtr;
                var srcRasterEndPtr = srcPtr + srcStride * height;
                while (srcPtr < srcRasterEndPtr)
                {
                    var dstPtr = surface.GetRowPointerUnchecked(row);
                    System.Buffer.MemoryCopy(srcPtr, dstPtr, srcStride, srcStride);
                    srcPtr += srcStride;
                    row++;
                }
            }
        }

 

 

Pointer operations are fast compared to for loops. If the stride is small then they are beating method calls because of the overhead.

Sometimes your raster is not in BGRA order. This can be handled easily using pointers.

 

Still I would like to avoid unsafe code at this level.

So surface methods to copy from/to part of a row would be great. I.e.

 

void CopyToRow(byte[] src, long srcOffset, int row, int column, int length);

void CopyFromRow(byte[] dst, long dstOffset, int row, int column, int length);

 

void CopyToRow(uint[] src, long srcOffset, int row, int column, int columns);

void CopyFromRow(uin[] dst, long dstOffset, int row, int column, int columns);

 

or for a whole raster

 

void CopyToImage(byte[] src, int srcStride);

void CopyFromImage(byte[] dst, int dstStride);

 

void CopyToImage(uint[] src, int srcStride);

void CopyFromImage(uint[] dst, int dstStride);

 

Sorry that I have to use spoilers instead of code sections but I'm tired to wait that it opens here (and most of the time it fails).

 

[NinthDesertDude]

1 hour ago, MJW said:

 

I was aware of the stride, but according to Microsoft: "The stride is the width of a single row of pixels (a scan line), rounded up to a four-byte boundary. If the stride is positive, the bitmap is top-down. If the stride is negative, the bitmap is bottom-up." So for a bitmap with 32-bit pixels, the stride equals the width. I realize a Surface isn't strictly speaking a Bitmap, but in order to be aliased to a bitmap, it has to have the same format.

 

(Perhaps I'm missing some finer point, but I at least had a reasonable basis for my original statement that "BGRA format doesn't have any padding between rows.")

You could be clever with assumptions that are correct and likely never to change, but why bake in an assumption? Do you really lose anything by using Stride? If in the future, paint.net adds overloads to deal with lower-bit data or something like that, the image format and therefore stride may be different. But since the API is already exposed with the expectation that you're using Stride, there will be no accommodation for your plugin when those changes roll around; it will simply stop working at that time.

You only lose the time spent resolving a variable, which is fractions of a single millisecond (at best)

[NinthDesertDude]

Also consider the use of Parallel.For() if you're copying a large bitmap and you're not delegating this to a GPU yourself (which I imagine you're not in your plugin)

[Rick Brewster]

8 hours ago, MJW said:

 

I was aware of the stride, but according to Microsoft: "The stride is the width of a single row of pixels (a scan line), rounded up to a four-byte boundary. If the stride is positive, the bitmap is top-down. If the stride is negative, the bitmap is bottom-up." So for a bitmap with 32-bit pixels, the stride equals the width. I realize a Surface isn't strictly speaking a Bitmap, but in order to be aliased to a bitmap, it has to have the same format.

 

(Perhaps I'm missing some finer point, but I at least had a reasonable basis for my original statement that "BGRA format doesn't have any padding between rows.")

 

That's Microsoft's definition of stride for System.Drawing.BitmapData, and is scoped to the way that they employ it for users of their library.

Paint.NET uses stride to go further, employing it for purposes like clipping. I can take a surface and carve out a rectangular region of it by setting newSurface.Scan0 = originalSurface.GetPointPointer(x,y), newSurface.Stride = originalSurface.Stride, etc. And then draw only within that rectangular region. No special clipping logic necessary beyond respecting width, height, and stride.

 

As another example, Paint.NET internally uses a recycling Surface allocator, and will often satisfy a request for (e.g.) a 180x180 surface by returning a 256x256 surface with a wrapper that says Width=180, Height=180, Stride=256*sizeof(ColorBgra).

 

Quote

No complains about unsafe code in plugins? 😉

 

I don't care if you use unsafe code. However, if you don't know exactly what you're doing, and why, do not use the Unchecked methods. I see you're using GetRowPointerUnchecked. If you have even the slightest doubt of your code's correctness, don't use that. Just use GetRowPointer.

If your plugin causes crashes then I will just block that version of it.

 

6 hours ago, NinthDesertDude said:

Also consider the use of Parallel.For() if you're copying a large bitmap and you're not delegating this to a GPU yourself (which I imagine you're not in your plugin)

 

Parallel.For() is unlikely to be a good choice because, 1) effects are already multithreaded, but also very much 2) memory copy operations are already fast and rows are generally short enough that this probably adds more overhead and does not improve performance. However, that said, benchmark it. Don't just parallelize it because it's neat and easy, but don't avoid it just because I'm skeptical. Benchmark it and use the faster code.

 

Honestly a lot of this discussion is, in my opinion, premature. Unless your code is running way too slow, I wouldn't dive into unsafe code with pointers. Just loop through your source buffer and set pixels using the indexer, e.g. surface[x,y] = buffer[i]. Copying into a surface like that at the end of a file loading operation is not going to be a bottleneck unless the image is absolutely enormous.

[Rick Brewster]

7 hours ago, midora said:

Still I would like to avoid unsafe code at this level.

So surface methods to copy from/to part of a row would be great. I.e.

 

void CopyToRow(byte[] src, long srcOffset, int row, int column, int length);

void CopyFromRow(byte[] dst, long dstOffset, int row, int column, int length);

 

void CopyToRow(uint[] src, long srcOffset, int row, int column, int columns);

void CopyFromRow(uin[] dst, long dstOffset, int row, int column, int columns);

 

or for a whole raster

 

void CopyToImage(byte[] src, int srcStride);

void CopyFromImage(byte[] dst, int dstStride);

 

void CopyToImage(uint[] src, int srcStride);

void CopyFromImage(uint[] dst, int dstStride);

 

 

FYI new methods such as these would likely not use arrays, instead preferring System.Span<T>, which can wrap either an array or a {T*, int length}, both in a very efficient way that the JIT is highly optimized for.

[midora]

17 minutes ago, Rick Brewster said:

 

FYI new methods such as these would likely not use arrays, instead preferring System.Span<T>, which can wrap either an array or a {T*, int length}, both in a very efficient way that the JIT is highly optimized for.

 

You are right that would be better. Plus Span<T> is pinable and can be used in a fixed context.