Repository navigation
Skia: avoid SKBitmap.Copy when creating ImmutableBitmap from pixels - #21675
Merged
Merged
Conversation
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)
kekekeks
added
area-perf
backport-candidate-12.0.x
Contributor
|
Why aren't we using https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Base/Media/Imaging/BitmapMemory.cs for this change? |
|
You can test this PR using the following package version. |
Member
Author
|
To skip awkwardness with per-row copying, BitmapMemory enforces tight layout. I guess it would technically be a better approach, yes. |
…-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)
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)
Member
Author
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)
* 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)
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)
Co-Authored-By: Claude Opus 4.8 (1M context)
kekekeks
marked this pull request as draft
June 30, 2026 18:12
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)
kekekeks
marked this pull request as ready for review
June 30, 2026 18:18
|
You can test this PR using the following package version. |
MrJul
added this pull request to the merge queue
Jul 2, 2026
MrJul
removed
the
backport-candidate-12.0.x
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Copy pixel data into manually allocated buffer instead of calling SKBitmap.Copy() which internally uses SKCanvas for some reason.
Also introduces a fast-path for CopyPixels and fixes a buffer size check to account for potential overflow.