Avoid WriteableBitmap misuse in several places, avoid UnmanagedBlod disposal from finalizer #14181

maxkatz6 · 2024-01-12T08:28:46Z

What does the pull request do?

Disposes WriteableBitmap used in Win32Icon for TrayIcon. Previously it was garbage collected and disposed on a wrong thread. Not a big issue, but better to dispose it as early as we don't need it anymore.
Replaces WriteableBitmap with Bitmap. This one is pretty simple. We don't really need a WriteableBitmap, as we never actually write into a bitmap anything. We only need to create a Bitmap from a pinned array of bytes.
Extra: I took a liberty to replace ArrayList with PooledList. For the most part it's just a type change, without any logic changes. As a result, ColorPicker now uses pooled lists, which should reduce memory consumption.

Fixed issues

Technically #9092, but that issue explains an actual bug somewhere in WriteableBitmap code, which is a different issue to solve.
Double fixes #9051

cc @robloo

avaloniaui-bot · 2024-01-12T08:48:33Z

You can test this PR using the following package version. 11.1.999-cibuild0043516-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

robloo · 2024-01-13T16:39:32Z

Replaces WriteableBitmap with Bitmap. This one is pretty simple. We don't really need a WriteableBitmap, as we never actually write into a bitmap anything. We only need to create a Bitmap from a pinned array of bytes.

I would argue Avalonia doesn't properly implement WriteableBitmap to begin with ;) Also note that the original ColorPicker code is copied from WinUI and then a C#/.NET port from a few years ago that uses the original WriteableBitmap. So it's using a WriteableBitmap because that is what original code did and what you need to do in other XAML frameworks. Avalonia is just... different.

I'll comment more specifically on the changes. In general though, I don't have any major objection to the change itself. You know more about this than me. However, I do think Avalonia should stop using unsafe code like this (at the control level) and use/implement WriteableBitmaps like everyone else.

Extra: I took a liberty to replace ArrayList with PooledList. For the most part it's just a type change, without any logic changes. As a result, ColorPicker now uses pooled lists, which should reduce memory consumption.

I never liked ArrayList to begin with and only wrote it to avoid 1 extra copy. If we have a PooledList that can do this already that's great. I did not know about this Avalonia type.

src/Avalonia.Controls.ColorPicker/Helpers/ColorPickerHelpers.cs

robloo · 2024-01-13T16:49:34Z

src/Avalonia.Controls.ColorPicker/ColorSpectrum/ColorSpectrum.cs

@@ -1187,7 +1171,7 @@ private async void CreateBitmapsAndColorMap()
                }
            });

-            Dispatcher.UIThread.Post(() =>
+            await Dispatcher.UIThread.InvokeAsync(() =>


Why switch from Post to InvokeAsync? The original code didn't have any await and was done completely async and the bitmaps came in a bit later after the control loads.

This method is a scope for pooled arrays. Without awaiting for Dispatcher call, pooled arrays are going to be disposed way too early.

Hm, I need to test this then. As long as we aren't blocking loading everything else and displaying the control awaiting the bitmaps to load it's fine. But bitmaps must be completely async -- especially on WASM they take a long time to load.

robloo · 2024-01-13T16:51:26Z

src/Avalonia.Controls.ColorPicker/ColorSpectrum/ColorSpectrum.cs

                        _valueBitmap = _maxBitmap;
                        break;
                    case ColorSpectrumComponents.ValueSaturation:
                    case ColorSpectrumComponents.SaturationValue:
+                        _hueRedBitmap?.Dispose();


Hm... I don't have anything against using a type other than ArrayList. However, Dispose() implies more native resource usage and shouldn't really be needed in a managed environment. I don't like the dispose calls everywhere and these things are easy to forget about in other code paths...

It should, really. We can leave bitmap to float in the memory, as they will be garbage collected anyway. But from our experience we know that skia objects finalizers are pretty slow, especially when there are many of them. Since we can dispose old value with not much issues - it should be done.

robloo · 2024-01-13T16:56:33Z

src/Avalonia.Controls.ColorPicker/ColorSpectrum/ColorSpectrum.cs

+                    ? pixelDataSize : 0;
+
+            var newHsvValues = new List<Hsv>(pixelCount);
+            using var bgraMinPixelData = new PooledList<byte>(pixelDataSize, ClearMode.Never);


Again, I'm not a fan of using a type for simple array data that requires disposing.

Array pool data would require disposing either way, as array needs to be returned to the pool at some point.

Well, I guess PooledList is overkill for this scenario. We only need an array with an .Add method for usability with existing code. Then we can re-use the same array reference as long as the bitmap size doesn't change. Just keep overwriting the data. No disposal needed.

I just don't like to see Dispose() much in high-level managed code... so it's just an observation. Then again, I also don't like the old ArrayList class either. Hopefully this all goes away in the future with composition brushes.

maxkatz6 · 2024-01-14T02:53:37Z

I would argue Avalonia doesn't properly implement WriteableBitmap to begin with ;)

As discussed with @kekekeks, potential problem with previous attempts might be caused by the fact that previous WriteableBitmap instance was still used as a value for Image.Source property. It shouldn't raise any exceptions, if Image.Source value was replaced before disposing old bitmap.
Saying this, I haven't tried this hack. As in my opinion Bitmap is fits better for this control.

Even better would be to use shaders and render spectrum on the GPU directly, but we don't have that yet. Using skia directly would greatly limit the control.

robloo · 2024-01-14T04:44:50Z

Even better would be to use shaders and render spectrum on the GPU directly, but we don't have that yet. Using skia directly would greatly limit the control.

Yes, I had a later thought too. Upstream uses a custom composition brush for this on newer versions of Windows. We should do the same eventually. So I can accept that what we have now is a temporary solution solving a few things including performance. But it should be replaced in a few years when we have better options.

kekekeks · 2024-01-28T20:10:16Z

src/Avalonia.Controls.ColorPicker/ColorSlider/ColorSlider.cs

-                ArrayList<byte> bgraPixelData = await ColorPickerHelpers.CreateComponentBitmapAsync(
+                // siteToCapacity = true, because CreateComponentBitmapAsync sets bytes via indexer over pre-allocated buffer. 
+                using var bgraPixelData = new PooledList<byte>(pixelWidth * pixelHeight * 4, ClearMode.Never, true);
+                await ColorPickerHelpers.CreateComponentBitmapAsync(


BTW, are we canceling previously queued operations when UpdateBackground is called again? Is there even a queue or do we have async operations executed on the thread pool in random order?

Also, why is it event a list in the first place, could have been just some buffer rented from array pool

BTW, are we canceling previously queued operations when UpdateBackground is called again?

No

Is there even a queue

No

do we have async operations executed on the thread pool in random order?

Theoretically, yes. In practice, no. The background bitmaps for both component sliders and the color spectrum DO NOT matter for the function of the color picker itself. Even if these bitmaps are completely wrong or missing the control will still work without an issue. They are visual-only and therefore it isn't considered important enough to worry about this. In practice I have never seen any issues with bitmaps rendering out of order and being noticeably different to the end user.

This is also why I originally had code calculating them completely async so the control can load and function while the bitmaps load (which can take a while on some platforms).

could have been just some buffer rented from array pool

The code follows the ideas of upstream WinUI which uses shared_ptr<vector<::byte>> usage. I certainly wasn't aware you had a special PooledList at the time and didn't think to use a pooled array either. That said, a lot of the code relies on using .Add()-type ideas where we don't need to track an index in the buffer. That means an array can't be used directly without modifying some code I really didn't/don't want to modify.

Task.Run possibly can be removed, as there is none in WinRT code. Especially when we reduced all the allocations.

Task.Run possibly can be removed, as there is none in WinRT code. Especially when we reduced all the allocations.

I'm fine with having this as a separate discussion along with: #14181 (comment). I just want to keep bitmap generation async. Maybe the solution for that is also composition brushes though running on another thread.

kekekeks

LGTM in general, probably need to investigate Task.Run usage separately

avaloniaui-bot · 2024-01-29T00:23:02Z

You can test this PR using the following package version. 11.1.999-cibuild0044098-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

…isposal from finalizer (#14181) * Fix TrayIconImpl not disposing WriteableBitmap when it should * Replace WriteableBitmap with Bitmap in ColorPicker * Replace ArrayList with PooledList to avoid extra allocations

maxkatz6 added 3 commits January 11, 2024 23:44

Fix TrayIconImpl not disposing WriteableBitmap when it should

95742a3

Replace WriteableBitmap with Bitmap in ColorPicker

baa5ee9

Replace ArrayList with PooledList to avoid extra allocations

72d6c3c

maxkatz6 added the backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch label Jan 12, 2024

robloo reviewed Jan 13, 2024

View reviewed changes

src/Avalonia.Controls.ColorPicker/Helpers/ColorPickerHelpers.cs Show resolved Hide resolved

robloo reviewed Jan 13, 2024

View reviewed changes

kekekeks reviewed Jan 28, 2024

View reviewed changes

kekekeks approved these changes Jan 28, 2024

View reviewed changes

grokys added this pull request to the merge queue Jan 28, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 28, 2024

Merge branch 'master' into writeable-bitmap-misuse-fixes

2f44076

maxkatz6 enabled auto-merge January 28, 2024 23:58

maxkatz6 disabled auto-merge January 29, 2024 06:23

maxkatz6 merged commit fc7339c into master Jan 29, 2024
5 of 7 checks passed

maxkatz6 deleted the writeable-bitmap-misuse-fixes branch January 29, 2024 06:23

maxkatz6 added backported-11.0.x and removed backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch labels Feb 8, 2024

robloo mentioned this pull request Sep 5, 2024

WriteableBitmap is too bare bones #16944

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid WriteableBitmap misuse in several places, avoid UnmanagedBlod disposal from finalizer #14181

Avoid WriteableBitmap misuse in several places, avoid UnmanagedBlod disposal from finalizer #14181

maxkatz6 commented Jan 12, 2024

avaloniaui-bot commented Jan 12, 2024

robloo commented Jan 13, 2024

robloo Jan 13, 2024

maxkatz6 Jan 14, 2024

robloo Jan 14, 2024

robloo Jan 13, 2024

maxkatz6 Jan 14, 2024

robloo Jan 13, 2024

maxkatz6 Jan 14, 2024

robloo Jan 14, 2024

maxkatz6 commented Jan 14, 2024

robloo commented Jan 14, 2024

kekekeks Jan 28, 2024

kekekeks Jan 28, 2024

robloo Jan 28, 2024

maxkatz6 Jan 28, 2024

robloo Jan 28, 2024

kekekeks left a comment

avaloniaui-bot commented Jan 29, 2024

Avoid WriteableBitmap misuse in several places, avoid UnmanagedBlod disposal from finalizer #14181

Avoid WriteableBitmap misuse in several places, avoid UnmanagedBlod disposal from finalizer #14181

Conversation

maxkatz6 commented Jan 12, 2024

What does the pull request do?

Fixed issues

avaloniaui-bot commented Jan 12, 2024

robloo commented Jan 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxkatz6 commented Jan 14, 2024

robloo commented Jan 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kekekeks left a comment

Choose a reason for hiding this comment

avaloniaui-bot commented Jan 29, 2024