Avalonia

Branch: main

Commit Graph

Author	SHA1	Message	Date
Nikita Tsukanov	842937ddad	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 strideheight 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>	3 months ago

Author

SHA1

Message

Date

Nikita Tsukanov

842937ddad

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>

3 months ago

1 Commits (main)