Delay metal drawable acquisition till frame submission. #13367

chinmaygarde · 2019-10-26T00:30:33Z

Uses the new SkSurface::MakeFromCAMetalLayer Skia API. Fixes flutter/flutter#41829

Uses the new `SkSurface::MakeFromCAMetalLayer` Skia API.

dnfield · 2019-10-26T16:49:27Z

shell/gpu/gpu_surface_metal.mm

-                                            bounds.height * scale,  // height
-                                            1,                      // sample count
-                                            metal_texture_info      // metal texture info
+  auto surface = SkSurface::MakeFromCAMetalLayer(context_.get(),            // context


Does next_drawable_ get set synchronously here?

No. This will defer the next drawable acquisition till much later. In fact, it is almost guaranteed to not be set after this call. I would have captured it in one of the scoped wrappers otherwise.

What happens if the next_drawable_ isn't set until after we check if it's null in the callback?

The next GPUSurfaceMetal::AcquireFrame will collect it if there is a next frame. GPUSurfaceMetal::~GPUSurfaceMetal will collect it via ReleaseUnusedDrawableIfNecessary if there isn't.

I should mention that either case is extremely unlikely because the surface frame itself is collected immediately after submission.

dnfield · 2019-10-26T16:49:29Z

shell/gpu/gpu_surface_metal.mm

    canvas->flush();
-    if (!hasExternalViewEmbedder) {
+
+    if (next_drawable_ == nullptr) {


Is it possible that this callback gets invoked between lines 101 and 104 above?

No. These are on the same thread and tasks are posted serially.

dnfield · 2019-10-26T16:50:19Z

shell/gpu/gpu_surface_metal.mm

  // External views need to present with transaction. When presenting with
  // transaction, we have to block, otherwise we risk presenting the drawable
  // after the CATransaction has completed.
  // See:
  // 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


nit: maybe we should move this comment down to line 143 now, since we're no longer getting whether it's an external view embedder here.

dnfield

Do the platform view tests pass on the metal backend with this? We only run them on OpenGL on CI AFAIK.

chinmaygarde

Addressed all comments. I also missed a release of the drawable when the surface itself is collected without pending surface frame submission.

Do the platform view tests pass on the metal backend with this?

Yes, but this needs to be automated. Tracking in flutter/flutter#43649

chinmaygarde · 2019-10-28T18:28:36Z

shell/gpu/gpu_surface_metal.mm

-                                            bounds.height * scale,  // height
-                                            1,                      // sample count
-                                            metal_texture_info      // metal texture info
+  auto surface = SkSurface::MakeFromCAMetalLayer(context_.get(),            // context


No. This will defer the next drawable acquisition till much later. In fact, it is almost guaranteed to not be set after this call. I would have captured it in one of the scoped wrappers otherwise.

chinmaygarde · 2019-10-28T18:29:34Z

shell/gpu/gpu_surface_metal.mm

  // External views need to present with transaction. When presenting with
  // transaction, we have to block, otherwise we risk presenting the drawable
  // after the CATransaction has completed.
  // See:
  // 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


chinmaygarde · 2019-10-28T18:30:09Z

shell/gpu/gpu_surface_metal.mm

    canvas->flush();
-    if (!hasExternalViewEmbedder) {
+
+    if (next_drawable_ == nullptr) {


No. These are on the same thread and tasks are posted serially.

dnfield

LGTM

…ter/engine#13367)

git@github.com:flutter/engine.git/compare/49971e21402e...25563a1 git log 49971e2..25563a1 --no-merges --oneline 2019-10-29 iska.kaushik@gmail.com Update recipe_changelog to account for dart_aot runner and clobbering 2019-10-28 iska.kaushik@gmail.com Revert "[flutter_runner] Don't build far files twice (#13397)" (flutter/engine#13400) 2019-10-28 matthew-carroll@users.noreply.github.com Remove multiplexed Flutter Android Lifecycle. (#43663) (flutter/engine#13394) 2019-10-28 iska.kaushik@gmail.com [flutter_runner] Don't build far files twice (flutter/engine#13397) 2019-10-28 chinmaygarde@google.com Delay metal drawable acquisition till frame submission. (flutter/engine#13367) 2019-10-28 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from 9MM-e... to mc3jR... (flutter/engine#13390) 2019-10-28 bkonyi@google.com Roll src/third_party/dart 5e39817ec7..e1fce75301 (2 commits) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC aaclarke@google.com on the revert to ensure that a human is aware of the problem. 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/+/master/autoroll/README.md

git@github.com:flutter/engine.git/compare/49971e21402e...25563a1 git log 49971e2..25563a1 --no-merges --oneline 2019-10-29 iska.kaushik@gmail.com Update recipe_changelog to account for dart_aot runner and clobbering 2019-10-28 iska.kaushik@gmail.com Revert "[flutter_runner] Don't build far files twice (flutter#13397)" (flutter/engine#13400) 2019-10-28 matthew-carroll@users.noreply.github.com Remove multiplexed Flutter Android Lifecycle. (flutter#43663) (flutter/engine#13394) 2019-10-28 iska.kaushik@gmail.com [flutter_runner] Don't build far files twice (flutter/engine#13397) 2019-10-28 chinmaygarde@google.com Delay metal drawable acquisition till frame submission. (flutter/engine#13367) 2019-10-28 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from 9MM-e... to mc3jR... (flutter/engine#13390) 2019-10-28 bkonyi@google.com Roll src/third_party/dart 5e39817ec7..e1fce75301 (2 commits) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC aaclarke@google.com on the revert to ensure that a human is aware of the problem. 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/+/master/autoroll/README.md

Delay metal drawable acquisition till frame submission.

3ca952c

Uses the new `SkSurface::MakeFromCAMetalLayer` Skia API.

chinmaygarde requested review from dnfield and jason-simmons October 26, 2019 00:30

chinmaygarde added this to In Progress in Metal Renderer via automation Oct 26, 2019

googlebot added the cla: yes label Oct 26, 2019

dnfield reviewed Oct 26, 2019

View reviewed changes

PR comments.

e4d8626

chinmaygarde commented Oct 28, 2019

View reviewed changes

dnfield approved these changes Oct 28, 2019

View reviewed changes

chinmaygarde merged commit 4044876 into flutter:master Oct 28, 2019

Metal Renderer automation moved this from In Progress to Done Oct 28, 2019

chinmaygarde deleted the metal_surface branch October 28, 2019 20:24

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

4044876 Delay metal drawable acquisition till frame submission. (flut…

91c803b

…ter/engine#13367)

engine-flutter-autoroll mentioned this pull request Oct 29, 2019

Roll engine 49971e21402e..4044876a7800 (3 commits) flutter/flutter#43678

Closed

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

4044876 Delay metal drawable acquisition till frame submission. (flut…

1d1f6da

…ter/engine#13367)

engine-flutter-autoroll mentioned this pull request Oct 29, 2019

Roll engine 49971e21402e..979baa078a9a (6 commits) flutter/flutter#43683

Closed

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

4044876 Delay metal drawable acquisition till frame submission. (flut…

106a212

…ter/engine#13367)

engine-flutter-autoroll mentioned this pull request Oct 29, 2019

Roll engine 49971e21402e..979baa078a9a (6 commits) flutter/flutter#43687

Closed

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

4044876 Delay metal drawable acquisition till frame submission. (flut…

0782541

…ter/engine#13367)

engine-flutter-autoroll mentioned this pull request Oct 29, 2019

Roll engine 49971e21402e..979baa078a9a (6 commits) flutter/flutter#43690

Closed

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

4044876 Delay metal drawable acquisition till frame submission. (flut…

a65a5be

…ter/engine#13367)

engine-flutter-autoroll mentioned this pull request Oct 29, 2019

Roll engine 49971e21402e..25563a12cab1 (7 commits) flutter/flutter#43692

Closed

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

4044876 Delay metal drawable acquisition till frame submission. (flut…

c8300ab

…ter/engine#13367)

engine-flutter-autoroll mentioned this pull request Oct 29, 2019

Roll engine 49971e21402e..25563a12cab1 (7 commits) flutter/flutter#43695

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delay metal drawable acquisition till frame submission. #13367

Delay metal drawable acquisition till frame submission. #13367

chinmaygarde commented Oct 26, 2019

dnfield Oct 26, 2019

chinmaygarde Oct 28, 2019

dnfield Oct 28, 2019

chinmaygarde Oct 28, 2019

chinmaygarde Oct 28, 2019

dnfield Oct 26, 2019

chinmaygarde Oct 28, 2019

dnfield Oct 26, 2019

chinmaygarde Oct 28, 2019

dnfield left a comment

chinmaygarde left a comment

chinmaygarde Oct 28, 2019

chinmaygarde Oct 28, 2019

chinmaygarde Oct 28, 2019

dnfield left a comment

Delay metal drawable acquisition till frame submission. #13367

Delay metal drawable acquisition till frame submission. #13367

Conversation

chinmaygarde commented Oct 26, 2019

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 left a 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

dnfield left a comment

Choose a reason for hiding this comment