Fixed memory management issues in OpacityMaskView #459
Conversation
Event handlers in the HoverLight weren't being unsubscribed On Disconnected.
OpacityMaskView wasn't handling resources properly on unload, causing memory leaks and also changing ```csharp private static CompositionBrush GetVisualBrush(UIElement element) ``` to ```csharp private static CompositionSurfaceBrush GetVisualBrush(UIElement element) ``` improves performance.
This reverts commit 8cde6c8.
This reverts commit 972bd09.
OpacityMaskView wasn't handling resources properly on unload, causing memory leaks and also changing ```csharp private static CompositionBrush GetVisualBrush(UIElement element) ``` to ```csharp private static CompositionSurfaceBrush GetVisualBrush(UIElement element) ``` improves performance.
There was a problem hiding this comment.
Pull request overview
This PR addresses memory management issues in the OpacityMaskView control by adding proper disposal of composition resources. The changes introduce cleanup logic when the control unloads and track composition objects for explicit disposal.
Key Changes:
- Added CleanupComposition method and Unloaded event handler to dispose composition resources
- Changed GetVisualBrush return type from CompositionBrush to CompositionSurfaceBrush
- Added fields to track composition resources (_sourceBrush, _rootGrid, _redirectVisual) for proper cleanup
Comments suppressed due to low confidence (2)
AIDevGallery/Controls/OpacityMask.xaml.cs:45
- The XML documentation contains duplicate sentences with slightly different wording. The summary should only contain one clear description. Remove the redundant "Creates a new instance of the OpacityMaskView class." line.
/// Initializes a new instance of the <see cref="OpacityMaskView"/> class.
/// Creates a new instance of the <see cref="OpacityMaskView"/> class.
/// </summary>
AIDevGallery/Controls/OpacityMask.xaml.cs:112
- The GetVisualBrush method creates CompositionVisualSurface and ExpressionAnimation objects that are never disposed, creating a memory leak. These composition resources should be tracked as fields (similar to _sourceBrush) and disposed in CleanupComposition, or the method should return a tuple/wrapper that includes these resources so they can be disposed properly.
private static CompositionSurfaceBrush GetVisualBrush(UIElement element)
{
Visual visual = ElementCompositionPreview.GetElementVisual(element);
Compositor compositor = visual.Compositor;
CompositionVisualSurface visualSurface = compositor.CreateVisualSurface();
visualSurface.SourceVisual = visual;
ExpressionAnimation sourceSizeAnimation = compositor.CreateExpressionAnimation($"{nameof(visual)}.Size");
sourceSizeAnimation.SetReferenceParameter(nameof(visual), visual);
visualSurface.StartAnimation(nameof(visualSurface.SourceSize), sourceSizeAnimation);
CompositionSurfaceBrush brush = compositor.CreateSurfaceBrush(visualSurface);
visual.Opacity = 0;
return brush;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| DefaultStyleKey = typeof(OpacityMaskView); | ||
|
|
||
| // Ensure composition resources are cleaned up when control unloads | ||
| Unloaded += OpacityMaskView_Unloaded; |
There was a problem hiding this comment.
The Unloaded event handler is registered in the constructor but never unregistered. This creates a memory leak because the control instance will remain referenced by the event even after it should be garbage collected. Consider unregistering the handler in the Loaded event or implementing IDisposable, or accept that this is a lifecycle event that should remain registered for the control's lifetime.
| _sourceBrush?.Dispose(); | ||
| _sourceBrush = null; | ||
|
|
||
| // Dispose the mask brush instance if we created one |
There was a problem hiding this comment.
The comment incorrectly refers to "mask brush instance" when it should refer to "mask" or "mask visual brush". The _mask field holds the opacity mask visual brush, not the maskBrush (which is handled earlier). Consider updating the comment to clarify what _mask represents.
| // Dispose the mask brush instance if we created one | |
| // Dispose the mask (opacity mask visual brush) if we created one |
| private readonly Compositor _compositor = CompositionTarget.GetCompositorForCurrentThread(); | ||
|
|
||
| // Composition resources we create and need to tear down explicitly to avoid retaining composition references longer than necessary | ||
| private CompositionBrush? _mask; |
There was a problem hiding this comment.
The field _mask is declared as CompositionBrush? but GetVisualBrush now returns CompositionSurfaceBrush. This type mismatch prevents proper type checking. Change the type to CompositionSurfaceBrush? to match the return type of GetVisualBrush and ensure type safety.
| private CompositionBrush? _mask; | |
| private CompositionSurfaceBrush? _mask; |
3 participants
OpacityMaskView wasn't handling resources properly on unload, causing memory leaks and also changing
to
improves performance.