Platform View implemenation for Metal #11070

dnfield · 2019-08-17T01:20:02Z

I've refactored bits and pieces to try to make more sense of this - in particular, some of the different methods for GL vs not GL have been merged and may do nothing on Software.

Fixes flutter/flutter#33487

dnfield · 2019-08-18T06:12:37Z

This is ready for review.

It still needs a test, which I'll work on as we go through review.

dnfield · 2019-08-18T06:35:08Z

shell/common/animator.cc

@@ -27,22 +27,27 @@ Animator::Animator(Delegate& delegate,
      waiter_(std::move(waiter)),
      last_begin_frame_time_(),
      dart_frame_deadline_(0),
+#if FLUTTER_SHELL_ENABLE_METAL
+      layer_tree_pipeline_(fml::MakeRefCounted<LayerTreePipeline>(2)),


This isn't an issue on Metal, I believe because we're controlling when we actually present the frame so that it always happens before the transaction is committed.

Can we do that on OpenGL?

I don't think the issue was identified when using OpenGL. IIRC this was a speculative fix for the keyboard lag.

Yes - but the keyboard lag doesn't happen on metal. Without the wait added in this PR for metal, similar issues are seen but much worse.

chinmaygarde · 2019-08-19T00:16:01Z

shell/gpu/gpu_surface_metal.mm

    [command_buffer.get() commit];
+    if (!wait) {


IIRC, presentDrawable is just waiting (asynchronously) for the command buffer to be scheduled and the and then presenting the same. Since you want the wait to happen now, the following code should be equivalent and more readable.

if (wait) { [command_buffer waitUntilScheduled]; } [command_buffer presentDrawable:drawable];

Nit: Just a suggestion that I found more readable.

I'm a little concerned that presentDrawable could end up unexpectedly yielding - the documentation on presentsWithTransaction says to use present instead of presentDrawable for this reason.

Just had to update this - the current formulation doesn't work, we have to call the presentDrawable before we commit otherwise it throws an exception sometimes.

chinmaygarde · 2019-08-19T00:39:47Z

shell/platform/darwin/ios/ios_surface.h

@@ -33,6 +33,10 @@ class IOSSurface {

  virtual std::unique_ptr<Surface> CreateGPUSurface() = 0;

+  // Creates a secondary surface using the supplied GrContext, if supported.
+  // Otherwise, the same as CreateGPUSurface.
+  virtual std::unique_ptr<Surface> CreateGPUSurface(GrContext* gr_context) = 0;


Can we just remove the no argument CreateGPUSurface and make the argument nullable?

chinmaygarde · 2019-08-19T00:41:03Z

shell/common/animator.cc

      pending_frame_semaphore_(1),
      frame_number_(1),
      paused_(false),
      regenerate_layer_tree_(false),
      frame_scheduled_(false),
      notify_idle_task_id_(0),
      dimension_change_pending_(false),
-      weak_factory_(this) {}
+      weak_factory_(this) {


Nit: This may require a clang-format pass.

This is how clang-format is putting it - maybe it's confused by the preprocessor macros?

chinmaygarde

Some nits. All minor. Otherwise looks good. Thanks!

amirh · 2019-08-20T18:35:39Z

shell/gpu/gpu_surface_metal.mm

+  // https://developer.apple.com/documentation/quartzcore/cametallayer/1478157-presentswithtransaction
+  // TODO(dnfield): only do this if transactions are actually being used.
+  // https://github.com/flutter/flutter/issues/24133
+  bool wait = delegate_->GetExternalViewEmbedder() != nullptr;


nit: may be clearer if we call this hasExternalViewEmbedder and move the comment above the if.

amirh · 2019-08-20T18:40:13Z

shell/gpu/gpu_surface_metal.mm

+      [command_buffer.get() commit];
+    } else {
+      [command_buffer.get() commit];
+      [command_buffer.get() waitUntilScheduled];


Would it make sense to not block here but instead finish rasterizing, and then do all the waits, present, and commit the transaction?

[Not asking to do it in this PR, just wondering whether it is possible and whether we think it can be more performant]

We should do as much work as we possibly can before getting the next drawable - guidelines from Apple say to hold it for as little time as possible. But if we use transactions, we have to block before presenting so that we are sure to present in the transaction. Alternatively, we have to make sure to not commit until we've presented in a callback.

amirh · 2019-08-20T18:41:14Z

shell/platform/darwin/ios/framework/Source/FlutterOverlayView.mm

+  } else if ([self.layer isKindOfClass:[CAMetalLayer class]]) {
+    fml::scoped_nsobject<CAMetalLayer> metalLayer(
+        reinterpret_cast<CAMetalLayer*>([self.layer retain]));
+    if (@available(iOS 8.0, *)) {


amirh · 2019-08-20T18:41:41Z

shell/platform/darwin/ios/framework/Source/FlutterOverlayView.mm

+  } else if ([self.layer isKindOfClass:[CAMetalLayer class]]) {
+    fml::scoped_nsobject<CAMetalLayer> metalLayer(
+        reinterpret_cast<CAMetalLayer*>([self.layer retain]));
+    if (@available(iOS 8.0, *)) {


Do we need a guard for iOS 8 features?

Probably not, but it doesn't hurt either right? I can remove if you want.

amirh · 2019-08-20T19:08:24Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

  overlays_[overlay_id] = std::make_unique<FlutterPlatformViewLayer>(
-      fml::scoped_nsobject<UIView>(overlay_view), std::move(ios_surface), std::move(surface));
+      std::move(overlay_view), std::move(ios_surface), std::move(surface));


Reading this code again I realize we have a bug here (we had it before this change): If we have multiple overlays and the gr_context is changed we only update it for the first overlay (as we immediately follow by updating overlays_gr_context_). We should probably instead update overlays_gr_context after iterating on composition_order_.

amirh · 2019-08-20T19:10:57Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

+  FML_DCHECK(flutter_view_);
+
+  auto overlay_it = overlays_.find(overlay_id);
+


The flow for this function is getting a little confusing, how about we do:

EnsureGrContextIsCurrent(overlay_id, gr_context); if (overlays_.count(overlay_id) != 0) { return; } // single copy of the overlay view creation code.

We create the overlay view differently if there is or is not a GrContext, and in the software backend there is none.

amirh · 2019-08-20T20:25:36Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h

@@ -63,6 +63,7 @@ struct FlutterPlatformViewLayer {
  fml::scoped_nsobject<UIView> overlay_view;
  std::unique_ptr<IOSSurface> ios_surface;
  std::unique_ptr<Surface> surface;
+  void UpdateSurface();


Forgot to implement?

Removed - I thought I'd need this and didn't.

amirh · 2019-08-20T20:30:19Z

shell/platform/darwin/ios/ios_surface.h

@@ -31,7 +31,10 @@ class IOSSurface {

  virtual void UpdateStorageSizeIfNecessary() = 0;

-  virtual std::unique_ptr<Surface> CreateGPUSurface() = 0;
+  // Creates a secondary surface using the supplied GrContext, if supported.
+  // The gr_context argument may be nullptr.


nit: mention that it creates a new GrContext if gr_context is null

Updated this comment to make more sense.

git@github.com:flutter/engine.git/compare/dfd281bcaefe...00b72a1 git log dfd281b..00b72a1 --no-merges --oneline 2019-08-23 50503328+flar@users.noreply.github.com Add tracing of the number of frames in flight in the pipeline. (flutter/engine#11423) 2019-08-23 dnfield@google.com Platform View implemenation for Metal (flutter/engine#11070) 2019-08-23 bkonyi@google.com Roll src/third_party/dart 2e4c89aa73..ef07c76302 (5 commits) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff (liyuqian@google.com), and stop the roller if necessary.

Having 2 or more platform views simultaneously in the layer tree was crashing immediately on iOS with GL backend. This regressed in #11070 which passed gl_context to a function in a loop using std::move (which meant on the second iteration the caller is no longer the owner of the field). I added a scenarios_app test, though this test doesn't run on a physical device on CI so it would have only caught the problem when running locally (flutter/flutter#43852).

…13449) Having 2 or more platform views simultaneously in the layer tree was crashing immediately on iOS with GL backend. This regressed in flutter#11070 which passed gl_context to a function in a loop using std::move (which meant on the second iteration the caller is no longer the owner of the field). I added a scenarios_app test, though this test doesn't run on a physical device on CI so it would have only caught the problem when running locally (flutter/flutter#43852).

dnfield added 5 commits May 28, 2019 19:55

partial

c322ee9

Merge remote-tracking branch 'upstream/master' into metal_pview

051504f

temp

98d3a08

Merge remote-tracking branch 'upstream/master' into metal_pview

62d52a5

It compiles!

388c6e0

dnfield requested review from chinmaygarde, iskakaushik and amirh August 17, 2019 01:20

googlebot added the cla: yes label Aug 17, 2019

dnfield added 6 commits August 16, 2019 23:13

missing code - still not working

0fe03a7

Merge remote-tracking branch 'upstream/master' into metal_pview

0644a99

Add upstream change so it displays

5be14d4

format

d0a15e0

format gn

2a87e08

Working!

d672d53

dnfield marked this pull request as ready for review August 18, 2019 06:12

dnfield mentioned this pull request Aug 18, 2019

Switch to the Metal rendering backend on iOS. flutter/flutter#33079

Closed

5 tasks

add TODO

edb557b

dnfield commented Aug 18, 2019

View reviewed changes

chinmaygarde reviewed Aug 19, 2019

View reviewed changes

chinmaygarde approved these changes Aug 19, 2019

View reviewed changes

dnfield added 5 commits August 18, 2019 20:58

default to nullptr

0664f40

Merge remote-tracking branch 'upstream/master' into metal_pview

64dea80

format

9f465ec

fix license

a1be9dc

Fix scheduling

57d78a3

amirh reviewed Aug 20, 2019

View reviewed changes

dnfield mentioned this pull request Aug 20, 2019

Add tests for platform views #11319

Merged

review

90e9e71

dnfield mentioned this pull request Aug 21, 2019

Firebase test for Platform Views on iOS #11350

Merged

dnfield merged commit 66636ea into flutter:master Aug 23, 2019

dnfield deleted the metal_pview branch August 23, 2019 23:15

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 24, 2019

66636ea Platform View implemenation for Metal (flutter/engine#11070)

bf779b3

engine-flutter-autoroll mentioned this pull request Aug 24, 2019

Roll engine dfd281bcaefe..00b72a14758d (3 commits) flutter/flutter#39163

Merged

bparrishMines mentioned this pull request Aug 27, 2019

Add check to enable metal for import #11537

Merged

amirh mentioned this pull request Oct 30, 2019

Fix iOS crash when multiple platform views are in the scene #13449

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Platform View implemenation for Metal #11070

Platform View implemenation for Metal #11070

dnfield commented Aug 17, 2019 •

edited

Loading

dnfield commented Aug 18, 2019

dnfield Aug 18, 2019

chinmaygarde Aug 19, 2019 •

edited

Loading

dnfield Aug 19, 2019

chinmaygarde Aug 19, 2019 •

edited

Loading

dnfield Aug 19, 2019

dnfield Aug 19, 2019

chinmaygarde Aug 19, 2019

dnfield Aug 19, 2019

chinmaygarde Aug 19, 2019

dnfield Aug 19, 2019

chinmaygarde left a comment

amirh Aug 20, 2019

dnfield Aug 20, 2019

amirh Aug 20, 2019

dnfield Aug 20, 2019

amirh Aug 20, 2019

amirh Aug 20, 2019

dnfield Aug 20, 2019

amirh Aug 20, 2019

amirh Aug 20, 2019

dnfield Aug 20, 2019

amirh Aug 20, 2019

dnfield Aug 20, 2019

amirh Aug 20, 2019

dnfield Aug 20, 2019

		FML_DCHECK(flutter_view_);

		auto overlay_it = overlays_.find(overlay_id);

Platform View implemenation for Metal #11070

Platform View implemenation for Metal #11070

Conversation

dnfield commented Aug 17, 2019 • edited Loading

dnfield commented Aug 18, 2019

Choose a reason for hiding this comment

chinmaygarde Aug 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmaygarde Aug 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmaygarde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield commented Aug 17, 2019 •

edited

Loading

chinmaygarde Aug 19, 2019 •

edited

Loading

chinmaygarde Aug 19, 2019 •

edited

Loading