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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

* BitmapMemory: drop redundant Interlocked comment

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

* comments

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

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)
+        /// <summary>
+        /// Performs a row-by-row copy of pixels from a source buffer into a destination buffer.
+        /// </summary>
+        /// <remarks>
+        /// <paramref name="sourceRowBytes"/> is signed and may be negative: a negative value means the
+        /// source rows are laid out bottom-up, with <paramref name="sourceAddress"/> pointing at the
+        /// first (top) row. The destination <paramref name="stride"/> must be positive and at least the
+        /// tightly-packed row size. The caller is responsible for validating <paramref name="sourceRect"/>
+        /// against the source bounds (e.g. via <see cref="ValidateSourceRect"/>).
+        /// </remarks>
+        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);
             }
         }
 
+        /// <summary>
+        /// Validates <paramref name="sourceRect"/> against this bitmap and copies pixels out of the
+        /// given framebuffer. Self-contained convenience wrapper around the static
+        /// <see cref="CopyPixelsCore(PixelRect,IntPtr,int,PixelFormat,IntPtr,int,int)"/> for inheritors.
+        /// </summary>
+        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);
         }

src/Skia/Avalonia.Skia/ImmutableBitmap.cs

@@ -131,18 +131,26 @@ namespace Avalonia.Skia
         /// <param name="data">Data pixels.</param>
         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");
             }
 

tests/Avalonia.Skia.UnitTests/Avalonia.Skia.UnitTests.csproj

@@ -2,6 +2,7 @@
   <PropertyGroup>
     <TargetFramework>$(AvsCurrentTargetFramework)</TargetFramework>
     <OutputType>Exe</OutputType>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
   </PropertyGroup>
   <Import Project="..\..\build\UnitTests.NetCore.targets" />
   <Import Project="..\..\build\XUnit.props" />

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