Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Skia] RenderTargetBitmap improvements #14955

Closed
Youssef1313 opened this issue Jan 3, 2024 · 1 comment · Fixed by #14959
Closed

[Skia] RenderTargetBitmap improvements #14955

Youssef1313 opened this issue Jan 3, 2024 · 1 comment · Fixed by #14959
Assignees
Labels
area/performance 📈 Categorizes an issue or PR as relevant to performance area/skia/stability ✏️ area/skia ✏️ Categorizes an issue or PR as relevant to Skia difficulty/medium 🤔 Categorizes an issue for which the difficulty level is reachable with a good understanding of WinUI kind/enhancement New feature or request

Comments

@Youssef1313
Copy link
Member

Youssef1313 commented Jan 3, 2024

Current situation

  • RenderTargetBitmap is IDisposable in Uno while it's not in WinUI. We should maintain parity with WinUI.
  • RenderTargetBitmap is using ArrayPool for renting the array. Generally, RenderTargetBitmap will tend to ask for a very large array that the .NET ArrayPool implementation will just always allocate a new array. So, effectively we are always allocating a large array and not using anything from the pool.
  • The allocated array ends up in the Large Object Heap (which might be a performance issue)

Two solutions come to mind:

Solution 1: Avoid _buffer field

Currently, in RenderAsBgra8_Premul, we get an SKBitmap and copy its pixels to _buffer field.

Then, we dispose the SKBitmap, and we use the _buffer array for two purposes:

  1. Creating an SKBitmap back again, if needed (e.g, if RenderTargetBitmap is used as Image.Source)
  2. In GetPixelsAsync

Both purposes can still be achieved with SKBitmap instead of a _buffer array.

The remaining part will be disposing the SKBitmap, which we will have to do in the destructor (unfortunately)

Note that this approach is Skia-specific. We might find similar ideas for other platforms, but then this will introduce more platform-specific code to RenderTargetBitmap.

EDIT: I tried this approach, but it's problematic as we need the array anyways in GetPixelsAsync (because Buffer needs either an array of bytes or Memory<byte>, but we have a Span<byte> that we can't convert to Memory<byte> as far as I know.

Solution 2: Using SegmentedArrays

We can port SegmentedArray implementation from Roslyn and use it instead of a regular array. This will help with LOH allocations.

EDIT: This too couldn't work, as SkiaSharp needs a pointer to contiguous memory.


Some profiling will be needed to know the performance impact of the changes. This will be most notable in snapshot tests.

@Youssef1313 Youssef1313 added kind/enhancement New feature or request triage/untriaged Indicates an issue requires triaging or verification difficulty/tbd Categorizes an issue for which the difficulty level needs to be defined. area/skia ✏️ Categorizes an issue or PR as relevant to Skia area/performance 📈 Categorizes an issue or PR as relevant to performance labels Jan 3, 2024
@Youssef1313 Youssef1313 self-assigned this Jan 3, 2024
@Youssef1313
Copy link
Member Author

Both approaches didn't work. I'm experimenting now with an unmanaged memory approach (suggested by @ramezgerges)

@MartinZikmund MartinZikmund changed the title [Skia] RenderTargetBitmap improvements [Skia] RenderTargetBitmap improvements Jan 29, 2024
@MartinZikmund MartinZikmund added difficulty/medium 🤔 Categorizes an issue for which the difficulty level is reachable with a good understanding of WinUI area/skia/stability ✏️ and removed triage/untriaged Indicates an issue requires triaging or verification difficulty/tbd Categorizes an issue for which the difficulty level needs to be defined. labels Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/performance 📈 Categorizes an issue or PR as relevant to performance area/skia/stability ✏️ area/skia ✏️ Categorizes an issue or PR as relevant to Skia difficulty/medium 🤔 Categorizes an issue for which the difficulty level is reachable with a good understanding of WinUI kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants