From 1bd786090ddb2e130b19bd34a2b4be963880e7ee Mon Sep 17 00:00:00 2001 From: Benedikt Stebner Date: Tue, 28 Jul 2026 14:07:37 +0200 Subject: [PATCH] CompositionBrush corrections (#21873) * Add failing tests for composition gradient brush change tracking Three defects in the unreleased CompositionBrush surface: - Replacing GradientStops after the first commit never reaches the server: the hand-written property registers nothing for serialization, so the change only ships once an unrelated tracked property dirties the brush. - SpreadMethod has the same gap. - A mutable Media.GradientStop crosses the batch by reference and the render thread then reads a UI-thread object at replay time. - A CompositionBrush resolves its server object for any compositor, so a foreign compositor ends up sharing a resource across render loops. * Track composition gradient brush changes and respect compositor affinity - GradientStops and SpreadMethod now register for serialization when assigned, so changes made after the first commit reach the server without waiting for an unrelated tracked property to dirty the brush. In-place list mutation stays untracked and is documented as requiring re-assignment. - Non-composition gradient stops are snapshotted to ImmutableGradientStop at serialization time instead of crossing the batch by reference, since the render thread reads the server list at replay time. - Resolving a CompositionBrush for a different compositor now throws instead of silently wiring one server resource into two render loops. Transient contexts without a compositor keep receiving the client brush unchanged. --- .../Composition/Brushes/CompositionBrush.cs | 47 +++++++++- .../Drawing/ServerResourceHelperExtensions.cs | 11 +++ .../Composition/CompositionBrushTests.cs | 94 +++++++++++++++++++ 3 files changed, 149 insertions(+), 3 deletions(-) create mode 100644 tests/Avalonia.Base.UnitTests/Composition/CompositionBrushTests.cs diff --git a/src/Avalonia.Base/Rendering/Composition/Brushes/CompositionBrush.cs b/src/Avalonia.Base/Rendering/Composition/Brushes/CompositionBrush.cs index bb356bab81..4cc67b4cee 100644 --- a/src/Avalonia.Base/Rendering/Composition/Brushes/CompositionBrush.cs +++ b/src/Avalonia.Base/Rendering/Composition/Brushes/CompositionBrush.cs @@ -1,5 +1,6 @@ using System.Collections.Generic; using Avalonia.Media; +using Avalonia.Media.Immutable; using Avalonia.Rendering.Composition; using Avalonia.Rendering.Composition.Server; using Avalonia.Rendering.Composition.Transport; @@ -42,10 +43,47 @@ partial class CompositionConicGradientBrush : IConicGradientBrush public abstract partial class CompositionGradientBrush : CompositionBrush, IGradientBrush { + private List _gradientStops = []; + private GradientSpreadMethod _spreadMethod; + internal new ServerCompositionGradientBrush Server { get; } - public List GradientStops { get; set; } = []; + + /// + /// The gradient stops. Mutations of the list itself are not tracked - assign + /// the property to ship in-place edits made after a commit. Stops created via + /// stay live + /// on the server and animate individually; other stops are snapshotted at + /// serialization time. + /// + public List GradientStops + { + get => _gradientStops; + set + { + if (ReferenceEquals(_gradientStops, value)) + return; + _gradientStops = value; + RegisterForSerialization(); + } + } + IReadOnlyList IGradientBrush.GradientStops => GradientStops; - public GradientSpreadMethod SpreadMethod { get; set; } + + /// + /// How the gradient repeats outside the stop range. + /// + public GradientSpreadMethod SpreadMethod + { + get => _spreadMethod; + set + { + if (_spreadMethod == value) + return; + _spreadMethod = value; + RegisterForSerialization(); + } + } + partial void OnRootChanged(); partial void OnRootChanging(); @@ -63,7 +101,10 @@ public abstract partial class CompositionGradientBrush : CompositionBrush, IGrad if (stop is CompositionGradientStop comp) writer.WriteObject(comp.Server); else - writer.WriteObject(stop); + // A mutable UI-thread stop must not cross to the render thread + // by reference; ship its current values instead. + writer.WriteObject(stop as ImmutableGradientStop + ?? new ImmutableGradientStop(stop.Offset, stop.Color)); } } } diff --git a/src/Avalonia.Base/Rendering/Composition/Drawing/ServerResourceHelperExtensions.cs b/src/Avalonia.Base/Rendering/Composition/Drawing/ServerResourceHelperExtensions.cs index bde92bcb94..4cb1ec181a 100644 --- a/src/Avalonia.Base/Rendering/Composition/Drawing/ServerResourceHelperExtensions.cs +++ b/src/Avalonia.Base/Rendering/Composition/Drawing/ServerResourceHelperExtensions.cs @@ -20,7 +20,14 @@ static class ServerResourceHelperExtensions if (brush is ICompositionRenderResource resource) return resource.GetForCompositor(compositor); if (brush is CompositionBrush compositionBrush) + { + // The server object belongs to its own compositor's render loop; + // handing it to another compositor would share one resource across + // two render threads. + if (compositionBrush.Compositor != compositor) + ThrowForeignCompositor(compositionBrush); return compositionBrush.Server; + } ThrowNotCompatible(brush); return null; } @@ -42,6 +49,10 @@ static class ServerResourceHelperExtensions [MethodImpl(MethodImplOptions.NoInlining), DoesNotReturn] static void ThrowNotCompatible(object o) => throw new InvalidOperationException(o.GetType() + " is not compatible with composition"); + + [MethodImpl(MethodImplOptions.NoInlining), DoesNotReturn] + static void ThrowForeignCompositor(CompositionObject o) => + throw new InvalidOperationException(o.GetType() + " belongs to a different compositor"); public static ITransform? GetServer(this ITransform? transform, Compositor? compositor) { diff --git a/tests/Avalonia.Base.UnitTests/Composition/CompositionBrushTests.cs b/tests/Avalonia.Base.UnitTests/Composition/CompositionBrushTests.cs new file mode 100644 index 0000000000..c26c77199c --- /dev/null +++ b/tests/Avalonia.Base.UnitTests/Composition/CompositionBrushTests.cs @@ -0,0 +1,94 @@ +using System; +using System.Collections.Generic; +using Avalonia.Media; +using Avalonia.Media.Immutable; +using Avalonia.Rendering; +using Avalonia.Rendering.Composition; +using Avalonia.Rendering.Composition.Drawing; +using Avalonia.Threading; +using Avalonia.UnitTests; +using Xunit; + +namespace Avalonia.Base.UnitTests.Composition; + +public class CompositionBrushTests : ScopedTestBase +{ + [Fact] + public void Replacing_The_Gradient_Stop_List_After_A_Commit_Should_Reach_The_Server() + { + using var services = new CompositorTestServices(); + var compositor = services.Compositor; + + var brush = compositor.CreateLinearGradientBrush(); + brush.GradientStops.Add(compositor.CreateGradientStop(0, Colors.Red)); + services.RunJobs(); + + Assert.Single(brush.Server.GradientStops); + + brush.GradientStops = new List + { + compositor.CreateGradientStop(0, Colors.Red), + compositor.CreateGradientStop(1, Colors.Blue), + }; + services.RunJobs(); + + Assert.Equal(2, brush.Server.GradientStops.Count); + } + + [Fact] + public void Changing_SpreadMethod_After_A_Commit_Should_Reach_The_Server() + { + using var services = new CompositorTestServices(); + var compositor = services.Compositor; + + var brush = compositor.CreateLinearGradientBrush(); + brush.GradientStops.Add(compositor.CreateGradientStop(0, Colors.Red)); + services.RunJobs(); + + Assert.Equal(GradientSpreadMethod.Pad, brush.Server.SpreadMethod); + + brush.SpreadMethod = GradientSpreadMethod.Repeat; + services.RunJobs(); + + Assert.Equal(GradientSpreadMethod.Repeat, brush.Server.SpreadMethod); + } + + [Fact] + public void Mutable_Gradient_Stops_Should_Be_Snapshotted_For_The_Server() + { + using var services = new CompositorTestServices(); + var compositor = services.Compositor; + + var mutableStop = new GradientStop(Colors.Red, 0); + var brush = compositor.CreateLinearGradientBrush(); + brush.GradientStops.Add(mutableStop); + services.RunJobs(); + + // The render thread reads the server list at replay time, so a mutable + // UI-thread stop must not cross the batch by reference. + var serverStop = Assert.Single(brush.Server.GradientStops); + Assert.NotSame(mutableStop, serverStop); + Assert.Equal(Colors.Red, serverStop.Color); + Assert.Equal(0, serverStop.Offset); + } + + [Fact] + public void Using_A_Composition_Brush_With_A_Foreign_Compositor_Should_Throw() + { + using var services = new CompositorTestServices(); + + var brush = services.Compositor.CreateSolidColorBrush(Colors.Red); + + var foreign = new Compositor(RenderLoop.FromTimer(services.Timer), null, + true, new DispatcherCompositorScheduler(), true, Dispatcher.UIThread); + + // A composition brush's server object belongs to its own compositor's + // render loop; handing it to another compositor's stream would let two + // render threads race over one resource. + Assert.Throws(() => brush.GetServer(foreign)); + + // A transient context without a compositor keeps the client brush and + // draws its static values, so no affinity applies there. + Assert.Same(brush, brush.GetServer(null)); + } +}