[framework] elide ImageFilter layers when animation is stopped#101731
[framework] elide ImageFilter layers when animation is stopped#101731fluttergithubbot merged 5 commits intoflutter:masterfrom
Conversation
flutter-dashboard
bot
added
the
framework
flar
left a comment
flar
left a comment
There was a problem hiding this comment.
Very succinct way to solve this. Do you have any data on whether this helps the original issue yet?
|
I ran the new gallery transition perf benchmark on a Pixel 4. I compared the base zoom transition with the zoom transition using FilterQuality.none and this patch. current: patched: Not super scientific, but we could always switch it over in a separate PR and then use skia perf to track regressions/improvements |
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| /// The ImageFilter layer created by setting filterQuality will introduce |
| /// The ImageFilter layer created by setting filterQuality will introduce | ||
| /// a saveLayer call. This is usually worthwhile when animating the layer, | ||
| /// but leaving it in the layer tree before the animation has started or after | ||
| /// it has finished significantly hurts performance. |
There was a problem hiding this comment.
Should we leave a note about this behavior in the doc comment on filterQuality?
| /// a saveLayer call. This is usually worthwhile when animating the layer, | ||
| /// but leaving it in the layer tree before the animation has started or after | ||
| /// it has finished significantly hurts performance. | ||
| final bool skipfilterQuality = scale.isCompleted || scale.isDismissed; |
There was a problem hiding this comment.
we prefer a switch/case on Animation.status over this on the off-chance that we ever need to add another status value to get the analyzer's help for all usages that need to be considered.
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| /// The ImageFilter layer created by setting filterQuality will introduce |
There was a problem hiding this comment.
All three comments from above also apply here :)
| // a saveLayer call. This is usually worthwhile when animating the layer, | ||
| // but leaving it in the layer tree before the animation has started or after | ||
| // it has finished significantly hurts performance. | ||
| final bool skipfilterQuality; |
There was a problem hiding this comment.
A nit. This is a negative name so that we have to do something if it is not set. I tend to prefer positive names that indicate something needs to be done if the flag is set. We're focused on "skipping" the filter quality because that is the improvement we want to see, but this flag is really indicating whether to use the filter quality. Does that make any semantic sense?
So, would "useFilterQuality" and swap all of the true/false/?: logic read better in this code?
There was a problem hiding this comment.
The exceptional and uncommon condition here is that we are in the process of animating, so the default is to not forward the filterQuality and the condition to call out are the cases when we do want to use it...?
There was a problem hiding this comment.
Yeah that reads better to me. Updated.
4 participants
See also #100972 (comment) for more information.
Using a ScaleTransition or a RotationTransition with a FilterQuality today incurs an extra cost, even when these widgets are not currently being animated. For the reasons outlined in the discussion above, the engine does not skip any work even if the ImageFilter used would be a no-op (for instance, the identity matrix). This results in extra save layers / render target switches.
Since the framework has more knowledge about when it is appropriate to add/remove these layers, the animated widgets should drop Filter Quality if the animation isn't running.
Note that this is not the cause of the regression in the linked issue. Instead, this performance issue is preventing us from switching to using the ImageFilter even though in theory it should perform better compared to the Transform layer.
#100972