[cupertino] improve cupertino picker performance by using at most one opacity layer #124719

jonahwilliams · 2023-04-12T19:51:42Z

Rather than applying an opacity layer per picker item, apply them to all partially opaque picker items at once. This dramatically improves performance for impeller, by reducing the number of required subpasses and texture allocations signficantly.

Before:
Doesn't finish, 100s of passes

After:

See also:

#124658

jonahwilliams · 2023-04-12T19:52:42Z

Letting this bake to see if any existing tests fail 😄

jonahwilliams · 2023-04-12T20:18:10Z

Previously with a cupertino picker containing ~N items, we would end up with N opacity layers. Now we only have 1 for all offcenter widgets and potentially 2 more for partially offcenter items - max of 3.

jonahwilliams · 2023-04-12T20:30:36Z

Actually we can go further and combine all partially opaque shapes together, this gives us a single opacity layer regardless of the number of items in the picker.

jonahwilliams · 2023-04-12T20:33:04Z

flutter-dashboard · 2023-04-12T21:24:13Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #124719 at sha 47a840b

jonahwilliams · 2023-04-12T23:21:37Z

I've looked through the goldens and in general they look OK to me. The change in color is easily explained by different behavior for clipping then opacity versus opacity then clipping. The only concern would be the scale of g3 impact.

goderbauer · 2023-04-13T16:34:45Z

packages/flutter/lib/src/rendering/list_wheel_viewport.dart

+      RenderBox? childToPaint = firstChild;
+      while (childToPaint != null) {
+        final ListWheelParentData childParentData = childToPaint.parentData! as ListWheelParentData;
+        _paintTransformedChild(childToPaint, context, offset, childParentData.offset, center: false);
+        childToPaint = childAfter(childToPaint);
+      }


Wondering if we can refactor this into a method (with center as a configurable argument) and call that here, above, and below to de-dup this a little and show that it is essentially doing the same thing?

goderbauer · 2023-04-13T16:36:34Z

packages/flutter/lib/src/rendering/list_wheel_viewport.dart

+  //
+  // If [center] is true, only children that overlap with the center section
+  // will be painted. If [center] is false, only children that do not overlap
+  // will be painted. Opacity is only applied to the partially overlapping


Is this comment about Opacity here actually true? My understanding was that opacity is fully controlled from outside this method now? I.e. it is up to the caller to apply opacity where necessary.

goderbauer · 2023-04-13T16:40:02Z

packages/flutter/lib/src/rendering/list_wheel_viewport.dart

@@ -828,22 +828,51 @@ class RenderListWheelViewport

  /// Paints all children visible in the current viewport.
  void _paintVisibleChildren(PaintingContext context, Offset offset) {
+    if (overAndUnderCenterOpacity >= 1) {


should we also go down this code path if useMagnifier is false? Seems like in that case the other code path would paint some children twice since _paintTransformedChild ignores the value of center in that case?

see line 907

No, the magnifier cannot be disabled if there is opacity.

Is that enforced anywhere in the API that useMagnifier must be true if overAndUnderCenterOpacity is less than 1? Or is it if there is opacity the value of useMagnifier is just (silently) ignored?

This is confusing, but I guess not a problem of this PR...

the value of use magnifier is just ignored

goderbauer · 2023-04-13T16:43:23Z

packages/flutter/lib/src/rendering/list_wheel_viewport.dart

@@ -876,7 +905,7 @@ class RenderListWheelViewport

    final bool shouldApplyOffCenterDim = overAndUnderCenterOpacity < 1;
    if (useMagnifier || shouldApplyOffCenterDim) {
-      _paintChildWithMagnifier(context, offset, child, transform, offsetToCenter, untransformedPaintingCoordinates);
+      _paintChildWithMagnifier(context, offset, child, transform, offsetToCenter, untransformedPaintingCoordinates, center: center);
    } else {
      _paintChildCylindrically(context, offset, child, transform, offsetToCenter);


should we assert that center is null if we go down the else branch?

Actually, the fact that this branch ignores center silently makes the code kinda hard to follow (and I think it is also the source of a bug in the current implementation, see my comment about useMagnifier above). Maybe we need to refactor this somehow to make it easier to follow.

should we assert that center is null if we go down the else branch?

What about this one? Since this is already pretty confusing it may be worthwhile to put a guardrail in place for when this gets refactored to make the caller instantly aware of the fact that their specification of center is not doing what they may think it is doing?

I added the assert. I'm not in love with this code but I don't see a nice way to re-arrange it all...

goderbauer · 2023-04-13T16:45:00Z

packages/flutter/lib/src/rendering/list_wheel_viewport.dart

+  // will be painted. If [center] is false, only children that do not overlap
+  // will be painted. Opacity is only applied to the partially overlapping
+  // children painted when [center] is true.
+  // If center is `null` this logic is ignored entirely.


Maybe clearer: if center is null all children will be painted?

jonahwilliams · 2023-04-13T19:25:17Z

From some investigation on my part, its not currently possible to disable the magnifier if there is any opacity. This change keeps that behavior, though it is a bit confusing.

I've tried to clarify the comments

goderbauer

LGTM

goderbauer · 2023-04-14T16:18:20Z

packages/flutter/lib/src/rendering/list_wheel_viewport.dart

@@ -828,22 +828,51 @@ class RenderListWheelViewport

  /// Paints all children visible in the current viewport.
  void _paintVisibleChildren(PaintingContext context, Offset offset) {
+    if (overAndUnderCenterOpacity >= 1) {


Is that enforced anywhere in the API that useMagnifier must be true if overAndUnderCenterOpacity is less than 1? Or is it if there is opacity the value of useMagnifier is just (silently) ignored?

This is confusing, but I guess not a problem of this PR...

goderbauer · 2023-04-14T16:32:22Z

packages/flutter/lib/src/rendering/list_wheel_viewport.dart

@@ -828,22 +828,42 @@ class RenderListWheelViewport

  /// Paints all children visible in the current viewport.
  void _paintVisibleChildren(PaintingContext context, Offset offset) {
+    // Note that the magnifier cannot be turned off if the opacity is less than


nit: per style guide just remove the "note that" at the beginning here.

goderbauer · 2023-04-14T16:36:56Z

packages/flutter/lib/src/rendering/list_wheel_viewport.dart

@@ -876,7 +905,7 @@ class RenderListWheelViewport

    final bool shouldApplyOffCenterDim = overAndUnderCenterOpacity < 1;
    if (useMagnifier || shouldApplyOffCenterDim) {
-      _paintChildWithMagnifier(context, offset, child, transform, offsetToCenter, untransformedPaintingCoordinates);
+      _paintChildWithMagnifier(context, offset, child, transform, offsetToCenter, untransformedPaintingCoordinates, center: center);
    } else {
      _paintChildCylindrically(context, offset, child, transform, offsetToCenter);


should we assert that center is null if we go down the else branch?

What about this one? Since this is already pretty confusing it may be worthwhile to put a guardrail in place for when this gets refactored to make the caller instantly aware of the fact that their specification of center is not doing what they may think it is doing?

…most one opacity layer (flutter/flutter#124719)

flutter/flutter@00171b0...50171bb 2023-04-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from da0805a9cf03 to 4a998b73a2df (5 revisions) (flutter/flutter#124944) 2023-04-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from feb476b7b991 to da0805a9cf03 (4 revisions) (flutter/flutter#124933) 2023-04-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from ee66f0d6f2c9 to feb476b7b991 (2 revisions) (flutter/flutter#124929) 2023-04-15 jonahwilliams@google.com Revert "[framework] use shader tiling instead of repeated calls to drawImage" (flutter/flutter#124640) 2023-04-15 jonahwilliams@google.com [cupertino] improve cupertino picker performance by using at most one opacity layer (flutter/flutter#124719) 2023-04-15 andrewrkolos@gmail.com [flutter_tools] Remove `Version.unknown` (flutter/flutter#124771) 2023-04-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from 41c1ea9a8283 to ee66f0d6f2c9 (2 revisions) (flutter/flutter#124923) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

… opacity layer (flutter#124719) Fixes flutter#124703 Rather than applying an opacity layer per picker item, apply them to all partially opaque picker items at once. This dramatically improves performance for impeller, by reducing the number of required subpasses and texture allocations signficantly. Before: Doesn't finish, 100s of passes After: ![image](https://user-images.githubusercontent.com/8975114/231569280-91f55c9a-53a5-4b75-8728-59a4dceebe81.png) ![image](https://user-images.githubusercontent.com/8975114/231569309-7c82e5ff-46c7-4f00-80f0-9b4096aa4b14.png) See also: flutter#124658

flutter/flutter@00171b0...50171bb 2023-04-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from da0805a9cf03 to 4a998b73a2df (5 revisions) (flutter/flutter#124944) 2023-04-16 engine-flutter-autoroll@skia.org Roll Flutter Engine from feb476b7b991 to da0805a9cf03 (4 revisions) (flutter/flutter#124933) 2023-04-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from ee66f0d6f2c9 to feb476b7b991 (2 revisions) (flutter/flutter#124929) 2023-04-15 jonahwilliams@google.com Revert "[framework] use shader tiling instead of repeated calls to drawImage" (flutter/flutter#124640) 2023-04-15 jonahwilliams@google.com [cupertino] improve cupertino picker performance by using at most one opacity layer (flutter/flutter#124719) 2023-04-15 andrewrkolos@gmail.com [flutter_tools] Remove `Version.unknown` (flutter/flutter#124771) 2023-04-15 engine-flutter-autoroll@skia.org Roll Flutter Engine from 41c1ea9a8283 to ee66f0d6f2c9 (2 revisions) (flutter/flutter#124923) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

[cupertino] improve cupertino picker performance

02692b6

flutter-dashboard bot added f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Apr 12, 2023

++

25711db

++

06eb67a

jonahwilliams changed the title ~~[cupertino] improve cupertino picker performance~~ [cupertino] improve cupertino picker performance by using at most one opacity layer Apr 12, 2023

++

47a840b

jonahwilliams marked this pull request as ready for review April 12, 2023 20:46

jonahwilliams requested review from goderbauer and dnfield April 12, 2023 20:46

flutter-dashboard bot added the will affect goldens Changes to golden files label Apr 12, 2023

goderbauer reviewed Apr 13, 2023

View reviewed changes

cleanup comments

3db1b0d

jonahwilliams added 2 commits April 13, 2023 12:31

helper

721fe24

default

dd5d163

goderbauer approved these changes Apr 14, 2023

View reviewed changes

jonahwilliams added 3 commits April 14, 2023 10:02

Add assert

697a4db

Merge branch 'master' into fix_cupertino_picker

e8ae26b

Merge branch 'master' into fix_cupertino_picker

3215f96

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 15, 2023

auto-submit bot merged commit d1d426e into flutter:master Apr 15, 2023

engine-flutter-autoroll mentioned this pull request Apr 16, 2023

Roll Flutter from 00171b07130e to 2d57261069bc (6 revisions) flutter/packages#3722

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 16, 2023

d1d426e [cupertino] improve cupertino picker performance by using at …

cc3b414

…most one opacity layer (flutter/flutter#124719)

engine-flutter-autoroll mentioned this pull request Apr 16, 2023

Roll Flutter from 00171b07130e to 50171bbae357 (7 revisions) flutter/packages#3723

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 16, 2023

d1d426e [cupertino] improve cupertino picker performance by using at …

ad34a90

…most one opacity layer (flutter/flutter#124719)

HansMuller mentioned this pull request Apr 20, 2023

Flutter Framework Team: Notable Commits #121415

Closed

[cupertino] improve cupertino picker performance by using at most one opacity layer #124719

[cupertino] improve cupertino picker performance by using at most one opacity layer #124719

Uh oh!

Conversation

jonahwilliams commented Apr 12, 2023

Uh oh!

jonahwilliams commented Apr 12, 2023

Uh oh!

jonahwilliams commented Apr 12, 2023

Uh oh!

jonahwilliams commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented Apr 12, 2023

Uh oh!

flutter-dashboard bot commented Apr 12, 2023

Uh oh!

jonahwilliams commented Apr 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Apr 13, 2023

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonahwilliams commented Apr 12, 2023 •

edited

Loading