From 842937ddad8378a92d48bb15d63bd9b75e17fe2c Mon Sep 17 00:00:00 2001 From: Nikita Tsukanov Date: Thu, 2 Jul 2026 18:40:24 +0500 Subject: [PATCH] Skia: avoid SKBitmap.Copy when creating ImmutableBitmap from pixels (#21675) * Skia: avoid SKBitmap.Copy when creating ImmutableBitmap from pixels The pixel-data ImmutableBitmap constructor used by LoadBitmap(IntPtr) wrapped the caller's buffer with InstallPixels and then called SKBitmap.Copy() to get an owned copy. SKBitmap.Copy() internally spins up an SKCanvas and draws the source via an SKPaint shader, which is far more expensive than a memory blit. Instead, allocate the destination with Marshal.AllocHGlobal, blit the pixels span-to-span, and hand ownership to Skia via InstallPixels with a release proc that frees the buffer. This is dramatically faster (~6x at 4096x4096, 50-80x at small/medium sizes) and allocates less managed memory. Co-Authored-By: Claude Opus 4.8 (1M context) * Skia: back ImmutableBitmap-from-pixels with BitmapMemory + shared per-row copy Follow-up to the previous change. Two corrections/improvements: * Use BitmapMemory as the backing storage instead of a bare Marshal.AllocHGlobal buffer, so allocation/lifetime (incl. GC memory pressure) is managed in one place. Ownership is handed to Skia via InstallPixels with a release proc that disposes the BitmapMemory. * The previous single contiguous blit was incorrect: the source stride is allowed to be negative (bottom-up layouts). Copy row by row instead, which also handles the backing store's row alignment differing from the source stride. To avoid duplicating the row-copy logic, Bitmap.CopyPixelsCore is now an internal static helper taking the source as raw (address, rowBytes, format), reused by both Bitmap/WriteableBitmap.CopyPixels and ImmutableBitmap. Source-rect validation moves to the (instance) call sites. Co-Authored-By: Claude Opus 4.8 (1M context) * Skia: keep a self-contained instance CopyPixelsCore for inheritors Re-add the instance CopyPixelsCore(..., ILockedFramebuffer fb) overload that validates the source rect and delegates to the static raw-buffer core. Inheritors (WriteableBitmap) and the framebuffer-based CopyPixels paths use this self-contained overload again, while ImmutableBitmap keeps using the static raw overload directly. Co-Authored-By: Claude Opus 4.8 (1M context) * Bitmap: add contiguous fast-path to CopyPixelsCore When the source and destination layouts are identical, tightly-packed and forward (no row padding, no X offset, positive stride == minStride), the whole region is contiguous in both buffers, so copy it with a single blit instead of the per-row loop. Measured up to ~5x faster for small images and ~30% for large ones; requiring stride == minStride also guarantees we never read past the source's last row. Negative strides, sub-rect copies and padded layouts still take the per-row path. Co-Authored-By: Claude Opus 4.8 (1M context) * Address code-review findings on the ImmutableBitmap pixel path * BitmapMemory: make ReleaseUnmanagedResources idempotent (zero Address after FreeHGlobal). SKBitmap.InstallPixels invokes the release proc even when it returns false, so the failure branch in ImmutableBitmap could dispose the same BitmapMemory twice -> double free / heap corruption. Idempotency also guards any other native-handoff consumer that double-disposes. * Bitmap.CopyPixelsCore: compute minBufferSize in 64-bit so a very large stride*height can't overflow the buffer-size guard and let the new contiguous blit (or the per-row loop) run past the buffers. * ImmutableBitmap: dispose the SKBitmap if SKImage.FromBitmap returns null so the backing memory is freed promptly via the release proc instead of waiting for the finalizer. Co-Authored-By: Claude Opus 4.8 (1M context) * BitmapMemory: free the buffer atomically with Interlocked.Exchange Back Address with a field and claim it via Interlocked.Exchange in ReleaseUnmanagedResources, so the buffer is freed exactly once even if an explicit Dispose() races with the finalizer or a native release callback running on another thread. Co-Authored-By: Claude Opus 4.8 (1M context) * BitmapMemory: drop redundant Interlocked comment Co-Authored-By: Claude Opus 4.8 (1M context) * Skia: let SKBitmap own its pixels in ImmutableBitmap, drop BitmapMemory Since CopyPixelsCore copies into the destination, ImmutableBitmap can simply TryAllocPixels a Skia-owned SKBitmap and blit into it, instead of allocating a separate BitmapMemory and handing it to InstallPixels with a release proc. This removes the manual unmanaged-memory lifetime management (and the release-proc/double-dispose hazard along with it). SKBitmap frees its own pixels on Dispose. Reverts the now-unneeded BitmapMemory idempotency/Interlocked change too, since that code path no longer exists. Co-Authored-By: Claude Opus 4.8 (1M context) * comments --------- Co-authored-by: Claude Opus 4.8 (1M context) --- src/Avalonia.Base/Media/Imaging/Bitmap.cs | 55 +++++++++++---- src/Skia/Avalonia.Skia/ImmutableBitmap.cs | 18 +++-- .../Avalonia.Skia.UnitTests.csproj | 1 + .../Media/ImmutableBitmapTests.cs | 68 +++++++++++++++++++ 4 files changed, 125 insertions(+), 17 deletions(-) create mode 100644 tests/Avalonia.Skia.UnitTests/Media/ImmutableBitmapTests.cs diff --git a/src/Avalonia.Base/Media/Imaging/Bitmap.cs b/src/Avalonia.Base/Media/Imaging/Bitmap.cs index dc1541414b..87ef0a234f 100644 --- a/src/Avalonia.Base/Media/Imaging/Bitmap.cs +++ b/src/Avalonia.Base/Media/Imaging/Bitmap.cs @@ -196,32 +196,63 @@ namespace Avalonia.Media.Imaging return sourceRect; } - private protected unsafe void CopyPixelsCore(PixelRect sourceRect, IntPtr buffer, int bufferSize, int stride, - ILockedFramebuffer fb) + /// + /// Performs a row-by-row copy of pixels from a source buffer into a destination buffer. + /// + /// + /// is signed and may be negative: a negative value means the + /// source rows are laid out bottom-up, with pointing at the + /// first (top) row. The destination must be positive and at least the + /// tightly-packed row size. The caller is responsible for validating + /// against the source bounds (e.g. via ). + /// + internal static unsafe void CopyPixelsCore(PixelRect sourceRect, IntPtr sourceAddress, int sourceRowBytes, + PixelFormat sourceFormat, IntPtr buffer, int bufferSize, int stride) { - if (Format == null) - throw new NotSupportedException("CopyPixels is not supported for this bitmap type"); - - sourceRect = ValidateSourceRect(sourceRect); - - int minStride = checked(((sourceRect.Width * fb.Format.BitsPerPixel) + 7) / 8); + int minStride = checked(((sourceRect.Width * sourceFormat.BitsPerPixel) + 7) / 8); if (stride < minStride) throw new ArgumentOutOfRangeException(nameof(stride)); - var minBufferSize = stride * sourceRect.Height; + // 64-bit to avoid overflowing the guard for very large strides/heights, which would + // otherwise let an oversized contiguous blit/loop run past the buffers. + var minBufferSize = (long)stride * sourceRect.Height; if (minBufferSize > bufferSize) throw new ArgumentOutOfRangeException(nameof(bufferSize)); - var offsetX = checked(((sourceRect.X * Format.Value.BitsPerPixel) + 7) / 8); + var offsetX = checked(((sourceRect.X * sourceFormat.BitsPerPixel) + 7) / 8); + + // Fast-path: when the source and destination layouts are identical, tightly-packed and + // forward (no row padding, no X offset, positive stride), the whole region is contiguous in + // both buffers and can be copied with a single blit. This is meaningfully faster than the + // per-row loop (up to ~5x for small images, ~30% for large ones). Requiring stride == minStride + // also guarantees we don't read past the source's last row. + if (offsetX == 0 && sourceRowBytes == stride && stride == minStride) + { + Unsafe.CopyBlock(buffer.ToPointer(), + (sourceAddress + sourceRowBytes * sourceRect.Y).ToPointer(), (uint)minBufferSize); + return; + } for (var y = 0; y < sourceRect.Height; y++) { - var srcAddress = fb.Address + fb.RowBytes * (sourceRect.Y + y) + offsetX; + var srcAddress = sourceAddress + sourceRowBytes * (sourceRect.Y + y) + offsetX; var dstAddress = buffer + stride * y; Unsafe.CopyBlock(dstAddress.ToPointer(), srcAddress.ToPointer(), (uint)minStride); } } + /// + /// Validates against this bitmap and copies pixels out of the + /// given framebuffer. Self-contained convenience wrapper around the static + /// for inheritors. + /// + private protected void CopyPixelsCore(PixelRect sourceRect, IntPtr buffer, int bufferSize, int stride, + ILockedFramebuffer fb) + { + sourceRect = ValidateSourceRect(sourceRect); + CopyPixelsCore(sourceRect, fb.Address, fb.RowBytes, fb.Format, buffer, bufferSize, stride); + } + public virtual void CopyPixels(PixelRect sourceRect, IntPtr buffer, int bufferSize, int stride) { if ( @@ -235,7 +266,7 @@ namespace Avalonia.Media.Imaging if (_isTranscoded) throw new NotSupportedException("CopyPixels is not supported for transcoded bitmaps"); - + using (var fb = readable.Lock()) CopyPixelsCore(sourceRect, buffer, bufferSize, stride, fb); } diff --git a/src/Skia/Avalonia.Skia/ImmutableBitmap.cs b/src/Skia/Avalonia.Skia/ImmutableBitmap.cs index 44408a5333..c168394768 100644 --- a/src/Skia/Avalonia.Skia/ImmutableBitmap.cs +++ b/src/Skia/Avalonia.Skia/ImmutableBitmap.cs @@ -131,18 +131,26 @@ namespace Avalonia.Skia /// Data pixels. public ImmutableBitmap(PixelSize size, Vector dpi, int stride, PixelFormat format, AlphaFormat alphaFormat, IntPtr data) { - using (var tmp = new SKBitmap()) + var info = new SKImageInfo(size.Width, size.Height, format.ToSkColorType(), alphaFormat.ToSkAlphaType()); + + _bitmap = new SKBitmap(); + if (!_bitmap.TryAllocPixels(info)) { - tmp.InstallPixels( - new SKImageInfo(size.Width, size.Height, format.ToSkColorType(), alphaFormat.ToSkAlphaType()), - data, stride); - _bitmap = tmp.Copy(); + _bitmap.Dispose(); + throw new ArgumentException("Unable to create bitmap from provided data"); } + + // Our CopyPixels is 6-15x faster than SKBitmap.Copy(), which internally spins up an + // SKCanvas and draws the source bitmap by assigning it as a shader on an SKPaint + Bitmap.CopyPixelsCore(new PixelRect(size), data, stride, format, _bitmap.GetPixels(), + _bitmap.RowBytes * size.Height, _bitmap.RowBytes); + _bitmap.SetImmutable(); _image = SKImage.FromBitmap(_bitmap); if (_image == null) { + _bitmap.Dispose(); throw new ArgumentException("Unable to create bitmap from provided data"); } diff --git a/tests/Avalonia.Skia.UnitTests/Avalonia.Skia.UnitTests.csproj b/tests/Avalonia.Skia.UnitTests/Avalonia.Skia.UnitTests.csproj index 1edcbfa5aa..5ee6d2b787 100644 --- a/tests/Avalonia.Skia.UnitTests/Avalonia.Skia.UnitTests.csproj +++ b/tests/Avalonia.Skia.UnitTests/Avalonia.Skia.UnitTests.csproj @@ -2,6 +2,7 @@ $(AvsCurrentTargetFramework) Exe + true diff --git a/tests/Avalonia.Skia.UnitTests/Media/ImmutableBitmapTests.cs b/tests/Avalonia.Skia.UnitTests/Media/ImmutableBitmapTests.cs new file mode 100644 index 0000000000..34fc7dadd5 --- /dev/null +++ b/tests/Avalonia.Skia.UnitTests/Media/ImmutableBitmapTests.cs @@ -0,0 +1,68 @@ +using System; +using System.Runtime.InteropServices; +using Avalonia.Platform; +using Xunit; + +namespace Avalonia.Skia.UnitTests.Media +{ + public class ImmutableBitmapTests + { + [Theory] + [InlineData(1, 1, false)] + [InlineData(3, 5, false)] + [InlineData(64, 64, false)] + [InlineData(1, 1, true)] + [InlineData(3, 5, true)] + [InlineData(64, 64, true)] + public unsafe void Constructor_From_Pixels_Copies_Source_Data(int width, int height, bool negativeStride) + { + var size = new PixelSize(width, height); + var rowBytes = width * 4; + var absStride = rowBytes; + var byteSize = absStride * height; + + // Logical pixel byte: deterministic function of (row, byteIndexWithinRow). + byte Expected(int row, int x) => (byte)((row * rowBytes + x) * 7 + 1); + + var source = Marshal.AllocHGlobal(byteSize); + try + { + // Lay the logical rows out in physical memory. For a negative stride the rows are stored + // bottom-up and the data pointer addresses the first (top) logical row, which sits at the + // highest address. + var buffer = new Span((void*)source, byteSize); + for (var row = 0; row < height; row++) + { + var physicalRow = negativeStride ? height - 1 - row : row; + for (var x = 0; x < rowBytes; x++) + buffer[physicalRow * absStride + x] = Expected(row, x); + } + + var stride = negativeStride ? -absStride : absStride; + var data = negativeStride ? source + absStride * (height - 1) : source; + + using var bitmap = new ImmutableBitmap( + size, new Vector(96, 96), stride, + PixelFormat.Bgra8888, AlphaFormat.Premul, data); + + // The constructor must take its own copy: corrupting (and freeing) the source + // afterwards must not affect the bitmap's pixels. + buffer.Fill(0xCD); + + Assert.Equal(size, bitmap.PixelSize); + + using var locked = bitmap.Lock(); + Assert.Equal(size, locked.Size); + + var dst = new ReadOnlySpan((void*)locked.Address, locked.RowBytes * height); + for (var row = 0; row < height; row++) + for (var x = 0; x < rowBytes; x++) + Assert.Equal(Expected(row, x), dst[row * locked.RowBytes + x]); + } + finally + { + Marshal.FreeHGlobal(source); + } + } + } +}