Avalonia icon indicating copy to clipboard operation
Avalonia copied to clipboard

Media Invalidation

Open TomEdwardsEnscape opened this issue 1 year ago • 7 comments

What does the pull request do?

This PR introduces Media Invalidation, a fast and low-maintenance system which consumes changes to AvaloniaObject instances which exist outside the visual tree, locates the Visual objects which depend on them for their rendered appearance, and calls the Visual.InvalidateVisual method on each one. Object nesting and collections are supported. These changes fix #8767.

Here is a scenario that Media Invalidation adds support for:

<Image>
  <DrawingImage>
    <DrawingGroup>
      <GeometryDrawing>
        <GeometryDrawing.Brush>
          <SolidColorBrush Color="Green"/>
        </GeometryDrawing.Brush>
        <RectangleGeometry Rect="0,0,120,120"/>
      </GeometryDrawing>
    </DrawingGroup>
  </DrawingImage>
</Image>

(See the MediaInvalidationTests class for real implementations similar to this sample.)

With Media Invalidation, changes to the value of SolidColorBrush.Color or RectangleGeometryRect both result in the Image control being invalidated and redrawing. Previously, the Image would never update unless manually forced to. Invalidation occurs even though the value that changed was deeply nested underneath the Image, and the path between the two passes through a DrawingGroup collection.

What is the current behavior?

Currently, invalidation of media objects is implemented in a very haphazard way. There are multiple bespoke systems in multiple classes, often involving complicated WeakEvent subscriptions. These systems are not recursive; links between parents and children must be explicitly made, and they are generally not.

One such system raises the event Pen.Invalidated. The event is not subscribed to, so I removed it and all the supporting code. The other systems I found do actually have effects, so I left them in place. They should be reviewed before being removed as some may also need to support recursive invalidation, and/or could execute without an associated AvaloniaProperty change.

Currently, tests which check for the old media invalidation systems are failing because they expect one invalidation but get two, due to both systems running at the same time. These failures will clear as the old systems are removed.

What is the updated/expected behavior with this PR?

Media Invalidation is an automated and low-maintenance solution for all Avalonia properties. Recursion is automatically handled if the property's value type is an AvaloniaObject. The only steps required are to register each relevant AvaloniaProperty with the Media Invalidation system, to ensure that property changed events are always raised (not guaranteed with DirectProperty) and to use collections which derive from MediaCollection<T>.

How was the solution implemented?

The core of the PR is the static class MediaInvalidation. This has a public method called AffectsMediaRender, which accepts a params array of AvaloniaProperty objects. It subscribes to changes on each property, and registers (with weak references, to prevent GC rooting) the property owner as a "media parent" of any AvaloniaObject which is assigned. Note that unlike objects in the visual and logical trees, a media object can have multiple parents, and can even have the same parent twice (e.g. Fill and Stroke being the same brush).

When the value of a registered media property changes, the media parents tree of the target object is recursively searched for all Visual objects, and each of these objects is visually invalidated.

A second important type is the abstract class MediaCollection<T>. This handles the invalidation of AvaloniaObject collections, by linking each collection member to each of the collection's parents. Everything is handled automatically once the the base type is implemented and assigned to a property registered with the media invalidation system.

Performance note: theme resources

A given AvaloniaObject can always change, so the Media Invalidation system must always track its parents. This can become a problem when a theme resource is frequently used, such as the standard Foreground brush. When I start the Avalonia control gallery, I see that a SolidColorBrush with the colour "Black" has over 1000 parents! This tree won't be searched unless the brush changes, but there is still a runtime and memory cost to all this book-keeping.

To avoid this situation, a new property called AvaloniaObject.IsSealed could be added. Changes to any AvaloniaProperty of a sealed object would be rejected, allowing MediaInvalidation to ignore it. This is exactly what WPF does to solve the same issue: media objects always derive from Freezable, which sets DependencyObject.IsSealed when its Freeze method is called. This happens automatically when such an object is inserted into a resource dictionary.

(Since opening the PR I have spotted #6387, which proposes transforming resources into immutable forms with a XAML directive. That would also resolve the problem.)

Benchmarks

There is a benchmark called SetPropertyThatAffectsRender, but it's not measuring anything on master. It sets and then clears the value of a Pen property on a test object, but because Pen does not implement IAffectsRender no subscriptions are made by Visual.AffectsRender. The only special behaviour being benchmarked is the InvalidateVisual call itself...which also does nothing, because the test object does not have a visual root.

So I fixed the benchmark locally by making it instead set and unset a Brush property. There is no need to commit this change as the changes in this PR cause the unmodified benchmark to become valid.

master

(After correcting the benchmark)

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
SetPropertyThatAffectsRender 1.059 us 0.0211 us 0.0187 us 0.0858 0.0067 0.0019 713 B

MediaInvalidation

(After correcting the benchmark and redirecting Visual.AffectsRender to MediaInvalidation.AffectsMediaRender)

Method Mean Error StdDev Gen 0 Allocated
SetPropertyThatAffectsRender 384.5 ns 0.59 ns 0.55 ns 0.0229 192 B

That's 62% faster, which is pretty good considering all the extra functionality that this PR adds! Instrumentation shows that after the InvalidateVisual call itself, the next biggest chunks of work are allocating and freeing weak GC handles, in that order.

Checklist

  • [x] Added unit tests
  • [x] Added XML documentation to any related classes
  • [ ] What happens to a property created with AddOwner?
  • [ ] Remove old notification systems (this will fix the failing tests)
  • [ ] Investigate #5813
  • [ ] Handle theme resources with many parents
  • [ ] Handle GlyphRun, which is mutable but not an AvaloniaObject

Event handlers to investigate

  • [ ] Visual.AffectsRender (turn into a pass-through)
  • [ ] Visual.RenderTransformChanged
  • [ ] Geometry.TransformChanged
  • [ ] Geometry.AffectsGeometryInvalidate (should this receive recursive events?)
  • [ ] GeometryCollection.Invalidate
  • [ ] PathGeometry.InvalidateGeometryFromSegments
  • [ ] Path.GeometryChangedHandler
  • [ ] GradientBrush.GradientStopsChanged / GradientBrush.GradientStopChanged

Fixed issues

Fixes #8767

TomEdwardsEnscape avatar Sep 05 '22 18:09 TomEdwardsEnscape

CLA assistant check
All CLA requirements met.

dnfadmin avatar Sep 05 '22 18:09 dnfadmin

The general plan was to move media invalidation to the render thread by introducing render-thread representations for media objects (the same way it's done in UWP). That would also remove the need to call Render and re-build the list of drawing commands.

kekekeks avatar Sep 06 '22 06:09 kekekeks

That sounds like a sensible long-term plan. In the immediate to medium term, this PR fixes bugs and substantially improves performance. It's also an opportunity to normalise everything into a single system which can be further adapted later.

Lastly, I'd vote for always having eventing on the CPU side, so that user code can respond to changes.

If this PR is something you would in principle accept into Avalonia, I will start working on removing the old invalidation events. :)

TomEdwardsEnscape avatar Sep 06 '22 07:09 TomEdwardsEnscape

A unified invalidating system like this is a really, really good idea! Like the detail in this PR too. Hope this gets in for 11.0

About the name: I would keep using the term 'Visual'. So VisualInvalidation.AffectsRender() for example. I don't think it is a good idea to introduce new terminology. Media is also a misnomer in some cases (Glyph run, etc.) and much less generic.

robloo avatar Sep 06 '22 12:09 robloo

MediaParentsBag and MediaCollection look like a good design in general, I think those can be adapted for use on the render thread.

However I'm not sure that we want to merge this PR as is if we are going to implement CompositionBrush, CompositionShape and friends, since in that case we'll have to remove the changes of this PR later.

I'd definitely would like to merge this if we won't get composition objects to work in time before the release.

The general roadmap is:

  • remove DeferrredRenderer/ImmediateRenderer, so that Compositor is always used
  • change Visual to synchronize properties instead of calling InvalidateVisual all the time.
  • implement media objects that live on the render thread

I hope to get that done before November this year.

BTW, there is also a similar problem with UI-thread animations: right now they don't track visual tree attachment at all, so if you have some brush with animation in resources, it would tick even if brush isn't used anywhere.

kekekeks avatar Sep 06 '22 13:09 kekekeks

Thanks for the positive feedback, everyone!

I'll leave this PR alone for now, because it sounds like the existing invalidation events are already due to be removed/refactored by people who understand them better than I do.

TomEdwardsEnscape avatar Sep 07 '22 10:09 TomEdwardsEnscape

I'll leave this PR alone for now, because it sounds like the existing invalidation events are already due to be removed/refactored

Personally, I'm sorry to hear that but it makes sense. Your approach here was very clean and thorough and is much better than the code we have now. Honestly, I'm not sure the other planned changes are going to be as good yet.

I certainly hope what was done here will be used where possible by @kekekeks. Even in a UWP-style composition system an invalidation system just like this PR is going to be required even if further composition primitives are added. In the end it might actually be better to finish this PR then modify it later with the composition-required changes. I don't think they are 1-and-1 equivalent.

robloo avatar Sep 07 '22 12:09 robloo