Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iOS platform views render the entire scene multiple times per frame #114712

Closed
jonahwilliams opened this issue Nov 4, 2022 · 17 comments
Closed
Assignees
Labels
engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list platform-ios iOS applications specifically

Comments

@jonahwilliams
Copy link
Member

When running an application on iOS with one platform view, its expect that we have to break the flutter frame into foreground/background in order to correctly place the platform view within the visual hierarchy. Locally I've observed that when targeting impeller, both the background and foreground frame seem to do exactly the same amount of work. Furthermore, If I add an expensive widget that does not intersect the PlatformView at all - I would expect one of the flutter render passes to slow down. Instead, both slow down.

This seems to me like we are clipping or splitting incorrectly, and might be doing far too much work. Potentially doing most of the work to draw most of the tree at least twice.

There don't seem to be any major rendering artifacts, besides the scrolling issue, so it could be that whatever clipping we use to break apart the scene happens too late in the Impeller backend.

Example app used was flutter/plugins/packages/google_maps_flutter

Apply the following diff:

diff --git a/packages/google_maps_flutter/google_maps_flutter/example/lib/map_ui.dart b/packages/google_maps_flutter/google_maps_flutter/example/lib/map_ui.dart
index 3f56f4079..e935db23f 100644
--- a/packages/google_maps_flutter/google_maps_flutter/example/lib/map_ui.dart
+++ b/packages/google_maps_flutter/google_maps_flutter/example/lib/map_ui.dart
@@ -4,6 +4,7 @@
 
 // ignore_for_file: public_member_api_docs
 
+import 'dart:math' as math;
 import 'package:flutter/material.dart';
 import 'package:flutter/services.dart' show rootBundle;
 import 'package:google_maps_flutter/google_maps_flutter.dart';
@@ -308,6 +309,10 @@ class MapUiBodyState extends State<MapUiBody> {
         Expanded(
           child: ListView(
             children: <Widget>[
+              CustomPaint(
+                size: const Size(400, 400),
+                painter: QRPainter(),
+              ),
               Text('camera bearing: ${_position.bearing}'),
               Text(
                   'camera target: ${_position.target.latitude.toStringAsFixed(4)},'
@@ -354,3 +359,35 @@ class MapUiBodyState extends State<MapUiBody> {
     });
   }
 }
+
+// Draw a "QR" like-code with 400x400 squarees.
+class QRPainter extends CustomPainter {
+  static final math.Random _random = math.Random(12455);
+
+  @override
+  void paint(Canvas canvas, Size size) {
+    final double boxWidth = size.width / 400;
+    final double boxHeight = size.height / 400;
+    for (int i = 0; i < 400; i++) {
+      for (int j = 0; j < 400; j++) {
+        final Rect rect = Rect.fromLTWH(i * boxWidth, j * boxHeight, boxWidth, boxHeight);
+        if (_random.nextBool()) {
+          canvas.drawRect(rect, Paint()
+            ..style = PaintingStyle.fill
+            ..color = Colors.red
+          );
+        } else {
+          canvas.drawCircle(rect.center, 1, Paint()
+            ..style = PaintingStyle.fill
+            ..color = Colors.blue
+          );
+        }
+      }
+    }
+  }
+
+  @override
+  bool shouldRepaint(covariant CustomPainter oldDelegate) {
+    return false;
+  }
+}

And run the app, selecting the first option.

Notice the timeline:

image (5)

@cyanglaz @flar

From casual observation, it seems odd to me that we use both a display list builder and skcanvas in https://github.com/flutter/engine/blob/main/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm#L597-L598 . Unsure if that is related.

@jonahwilliams jonahwilliams added platform-ios iOS applications specifically engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list e: impeller Impeller rendering backend issues and features requests labels Nov 4, 2022
@jonahwilliams
Copy link
Member Author

@zanderso

@flar
Copy link
Contributor

flar commented Nov 7, 2022

Is this an issue related to why I'm creating LayerStateStack? Basically, when we have the before/after canvas we execute state operations on both even though only one of them is reponsible for rendering at any particular time. As a result, the foreground canvas will get a bunch of "state changes, renderings, state changes, renderings, etc." calls, then we hit the PlatformView and then all it sees after that is "state changes, state changes, state changes" all of which have no content in them. The background canvas gets the opposite - only state changes until we get to the PlatformView and then a mix of state changes and rendering after that.

This leads us to have a bunch of NOP saveLayers and clip/transform changes on the tail end of the foreground recording and at the leading end of the background recording. Does Impeller ignore those as well as SkCanvas does?

With LayerStateStack, we will only communicate state changes to the active recorder and when we hit a PlatformView we will transfer all outstanding state to the new recorder and stop talking to the old recorder. This may prevent the issue. You can try running this on any of the flutter commits where the LayerStateStack was landed to see if the problem is mitigated.

The 2 times it landed were:

The next outstanding "reland" PR is:

Run any version of Flutter that has those engine commits rolled into it and see if there is a different performance profile.

@flar
Copy link
Contributor

flar commented Nov 7, 2022

Another question is whether Impeller will automatically ignore saveLayer requests that have no content in them?

@zanderso
Copy link
Member

zanderso commented Nov 7, 2022

cc @bdero

@flar
Copy link
Contributor

flar commented Nov 8, 2022

@jonahwilliams does your thumbs-up mean that you ran it against LayerStateStack and saw an improvement? Or just that you like the direction that is going in?

@jonahwilliams
Copy link
Member Author

I'm going to try out with the layer state stack, but haven't yet

@bdero
Copy link
Member

bdero commented Nov 9, 2022

Another question is whether Impeller will automatically ignore saveLayer requests that have no content in them?

Yes. At render time, Impeller skips SaveLayers with empty coverage. And the coverage ends up empty when the blend mode is non-destructive and the SaveLayers contain nothing that modifies the layer's color attachment.

@chinmaygarde chinmaygarde added P1 High-priority issues at the top of the work list and removed P2 Important issues not at the top of the work list labels Nov 9, 2022
@bdero
Copy link
Member

bdero commented Nov 11, 2022

I seem to be seeing the same double-rendering behavior with Skia. Is there any evidence of an issue here that's Impeller-specific?

Skia:
image

Impeller:
image

@jonahwilliams
Copy link
Member Author

Perhaps impeller is just significantly slower for certain scenes? I'd still expect to see two drawFrames for a platform view, so that Skia trace is reasonable by itself

@bdero
Copy link
Member

bdero commented Nov 11, 2022

That seems likely. We'll need to repro the stuff that's slower. With the diff, I'm actually getting worse results with Skia (11 seconds per frame) than Impeller (7 seconds per frame).

@jonahwilliams
Copy link
Member Author

Besides impeller vs Skia, I think we'll need to find a smarter way to slice up the scenes for platform views.

@zanderso do you know what engine commit the g3 app was on? Maybe that was before I landed the trivial glyph atlas caching. That would blow up time on Impeller compared to Skia if we were creating 2x glyph atlas per frame

@zanderso
Copy link
Member

Yes. It was last week, so prior to cl/486796593, which has the glyph atlas caching change. I will try again today.

@bdero bdero changed the title [Impeller] iOS platform views performance issues on Impeller iOS platform views performance issues on Impeller Nov 11, 2022
@bdero bdero removed the e: impeller Impeller rendering backend issues and features requests label Nov 11, 2022
@bdero bdero changed the title iOS platform views performance issues on Impeller iOS platform views render the entire scene multiple times per frame Nov 11, 2022
@bdero
Copy link
Member

bdero commented Nov 11, 2022

I renamed the bug to "iOS platform views render the entire scene multiple times per frame", but this may be wrong as I haven't actually verified that there isn't a setup problem in the example app causing the overlap rectangle to be too large/encapsulate the custom painter with the 160k draw calls.

@jonahwilliams jonahwilliams added P2 Important issues not at the top of the work list and removed P1 High-priority issues at the top of the work list labels Nov 14, 2022
@jonahwilliams
Copy link
Member Author

Bumping this down the P4 because there is no near-term fix that we are looking at, based on my understanding

@flar
Copy link
Contributor

flar commented Dec 1, 2022

It looks like this is actually the issue that was documented in #114784 but the cause for that issue wasn't well described. What appears to be happening is this:

  • Embedders break a scene into multiple layers representing what Flutter draws below, between, and above each PlatformView
  • When rendering each between/above layer, the rendering is broken into pieces so that we restrict the pixels we touch that are above the View so that iOS doesn't think of that area as "owned by Flutter's content" for event distribution semantics.
  • Due to the need to minimize rendering above a view, we break the layers that are above any view into one piece per view that only renders the stuff that intersects that view (once per view), and a final pass that renders the rest of that layer with all of the view rectangles punched out of its clip (using clipRect(kDifference)).

It's that final step where we render each layer multiple times. Because of a missing clipRect in the code that creates the pixels for each "covering a view" piece, we render the full frame for each of those pieces. The issue above is asking to add in that clipRect to minimize the rendering.

Adding that clipRect significantly reduces the amount of work done when rendering around embedders, but we could improve the savings by implementing some sort of up-front rendering op rejection based on the bounds of the op and the current clip. SkPicture rendering already does something like this, but we don't have as efficient mechanisms in DisplayList/Impeller. This rendering op culling is described in #92740

@chinmaygarde
Copy link
Member

The quick fix for this issue landed in flutter/engine#37107 and the ask for adding such optimizations in the DisplayList/Impeller layer are tracked in #92740. Closing this as completed. Let me know if there is context here that is not already captured elsewhere.

@github-actions
Copy link

github-actions bot commented Mar 5, 2023

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list platform-ios iOS applications specifically
Projects
No open projects
Archived in project
Development

No branches or pull requests

5 participants