Skip empty platform view overlays. by amirh · Pull Request #11427 · flutter/engine

amirh · 2019-08-24T01:47:13Z

This change sets up a "spying canvas" to try and detect empty canvases.
When using platform views with a custom embedder, if a platform view
overlay canvas is known to be empty we skip creating a compositor layer
for that overlay.

This change sets up a "spying canvas" to try and detect empty canvases. When using platform views with a custom embedder, if a platform view overlay canvas is known to be empty we skip creating a compositor layer for that overlay.

chinmaygarde

Other than a test to make sure we don't ask the embedder for a backing store unnecessarily, everything else looks good. All other comments are nits.

chinmaygarde · 2019-08-24T21:45:02Z

shell/common/canvas_spy.h

+// This is used to determine whether anything was drawn into
+// a canvas so it is possible to implement optimizations that
+// are specific to empty canvases.
+class CanvasSpy {


The comments won't end up in doxygen generated docs. See shell.h for a prototype.

Ditto for all other comment blocks.

chinmaygarde · 2019-08-24T21:49:26Z

shell/common/canvas_spy_unittests.cc

+  SkPixmap actual;
+  SkPixmap expected;
+
+  actual_surface->peekPixels(&actual);


These calls can fail. ASSERT_TRUE on the result.

chinmaygarde · 2019-08-24T21:49:40Z

shell/common/canvas_spy_unittests.cc

+  actual_surface->peekPixels(&actual);
+  expected_surface->peekPixels(&expected);
+
+  const auto size = actual.rowBytes() * actual.height();


ASSERT_NE(size, 0) to make sure earlier failures on the peek don't result in this test passing.

chinmaygarde · 2019-08-24T21:54:15Z

shell/common/canvas_spy.cc

+  return n_way_canvas_.get();
+}
+
+DidDrawCanvas::DidDrawCanvas(int width, int height) : INHERITED(width, height) {


We don't use Skia's internal non-namespaced macros or the typedef pattern. Please use the superclass name directly instead of using INHERITED.

chinmaygarde · 2019-08-24T21:56:04Z

shell/common/canvas_spy.cc

+}
+
+DidDrawCanvas::DidDrawCanvas(int width, int height) : INHERITED(width, height) {
+  did_draw_ = false;


Nit: Prefer defaulting this to false in the header and get rid of this. That way, if/when we add more ctors, we don't have to remember to default all scalars.

chinmaygarde · 2019-08-24T22:00:32Z

shell/common/canvas_spy.h

+  ~DidDrawCanvas() override;
+  bool DidDrawIntoCanvas();
+
+ protected:


We are not going to subclass this further. Please make this private and mark the class final. This will also make it a compiler error if the base class adds new pure virtual methods that we haven't overriden.

chinmaygarde · 2019-08-24T22:01:38Z

shell/common/canvas_spy.h

+  bool onDoSaveBehind(const SkRect*) override;
+  void willRestore() override;
+
+  void didConcat(const SkMatrix&) override;


We mention the class/interface that defines the method being overriden. So, // | SkCanvasVirtualEnforcer<SkNoDrawCanvas>| here and in the implementation for all overrides.

chinmaygarde · 2019-08-24T22:06:33Z

shell/platform/embedder/embedder_external_view_embedder.cc

+    presented_layers.push_back(
+        MakeLayer(params, presented_platform_views.at(view_id)));
+
+    if (!pending_canvas_spies_[view_id]->DidDrawIntoCanvas()) {


Use .at() instead of .[] when you know you don't want to insert a default constructed map element. We know this is safe because of the DCHECK added above.

chinmaygarde · 2019-08-24T22:09:34Z

shell/platform/embedder/embedder_external_view_embedder.cc

+    if (!pending_canvas_spies_[view_id]->DidDrawIntoCanvas()) {
+      // Nothing was drawn into the overlay canvas, we don't need to composite
+      // it.
+      continue;


Test this continue. We should upgrade the embedder test compositor to make sure we track the number of backing stores being requested of the the embedder. Then assert that we don't ask for extra backing stores. If we forgot or later moved this continue, no test would fail.

Added an assertion for the number of backing stores in the test.

Side note that EmbedderTest.CompositorMustBeAbleToRenderWithPlatformLayerOnBottom would have failed if we removed this continue.

amirh

Thanks, PTAL.

amirh · 2019-08-26T16:43:32Z

shell/common/canvas_spy.cc

+  return n_way_canvas_.get();
+}
+
+DidDrawCanvas::DidDrawCanvas(int width, int height) : INHERITED(width, height) {


amirh · 2019-08-26T16:44:14Z

shell/common/canvas_spy.cc

+}
+
+DidDrawCanvas::DidDrawCanvas(int width, int height) : INHERITED(width, height) {
+  did_draw_ = false;


amirh · 2019-08-26T16:46:48Z

shell/common/canvas_spy.h

+// This is used to determine whether anything was drawn into
+// a canvas so it is possible to implement optimizations that
+// are specific to empty canvases.
+class CanvasSpy {


amirh · 2019-08-26T16:50:17Z

shell/common/canvas_spy.h

+  ~DidDrawCanvas() override;
+  bool DidDrawIntoCanvas();
+
+ protected:


amirh · 2019-08-26T16:53:36Z

shell/common/canvas_spy.h

+  bool onDoSaveBehind(const SkRect*) override;
+  void willRestore() override;
+
+  void didConcat(const SkMatrix&) override;


amirh · 2019-08-26T16:55:09Z

shell/common/canvas_spy_unittests.cc

+  SkPixmap actual;
+  SkPixmap expected;
+
+  actual_surface->peekPixels(&actual);


amirh · 2019-08-26T16:55:44Z

shell/common/canvas_spy_unittests.cc

+  actual_surface->peekPixels(&actual);
+  expected_surface->peekPixels(&expected);
+
+  const auto size = actual.rowBytes() * actual.height();


amirh · 2019-08-26T17:01:22Z

shell/platform/embedder/embedder_external_view_embedder.cc

+    presented_layers.push_back(
+        MakeLayer(params, presented_platform_views.at(view_id)));
+
+    if (!pending_canvas_spies_[view_id]->DidDrawIntoCanvas()) {


amirh · 2019-08-26T18:16:55Z

shell/platform/embedder/embedder_external_view_embedder.cc

+    if (!pending_canvas_spies_[view_id]->DidDrawIntoCanvas()) {
+      // Nothing was drawn into the overlay canvas, we don't need to composite
+      // it.
+      continue;


Added an assertion for the number of backing stores in the test.

Side note that EmbedderTest.CompositorMustBeAbleToRenderWithPlatformLayerOnBottom would have failed if we removed this continue.

chinmaygarde · 2019-08-26T19:06:55Z

I am going to close b/139737705.

[email protected]:flutter/engine.git/compare/11e3ade157db...ba1a303 git log 11e3ade..ba1a303 --no-merges --oneline 2019-08-26 [email protected] Patch buttons for chromebook touchpad (flutter/engine#11420) 2019-08-26 [email protected] Skip empty platform view overlays. (flutter/engine#11427) 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 [email protected] 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

liyuqian · 2019-08-31T20:57:01Z

@amirh : this looks like a performance optimization. Is there a known benchmark that covers this performance? Or is there a manual performance test process that's documented somewhere?

chinmaygarde · 2019-09-03T01:56:06Z

This optimization is guaranteed by the engine and made available to the embedder. This is tested in the the CanvasSpyUnittests Amir added in this patch and enabled in EmbedderTest.CompositorMustBeAbleToRenderWithPlatformLayerOnBottom.

amirh requested a review from chinmaygarde August 24, 2019 01:47

googlebot added the cla: yes label Aug 24, 2019

Skip empty platform view overlays.

fb66d03

This change sets up a "spying canvas" to try and detect empty canvases. When using platform views with a custom embedder, if a platform view overlay canvas is known to be empty we skip creating a compositor layer for that overlay.

amirh force-pushed the skip_empty_overlays branch from d67d1fd to fb66d03 Compare August 24, 2019 01:49

chinmaygarde reviewed Aug 24, 2019

View reviewed changes

amirh added 2 commits August 26, 2019 09:41

licenses

bb72055

review comments followup

ca806ff

amirh commented Aug 26, 2019

View reviewed changes

chinmaygarde approved these changes Aug 26, 2019

View reviewed changes

amirh merged commit a34f9a8 into flutter:master Aug 26, 2019

amirh deleted the skip_empty_overlays branch August 26, 2019 18:40

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

a34f9a8 Skip empty platform view overlays. (flutter/engine#11427)

e48dbf3

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

Roll engine 11e3ade157db..ba1a303ae2d7 (2 commits) flutter/flutter#39265

Merged

Conversation

amirh commented Aug 24, 2019

Uh oh!

chinmaygarde 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!

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!

Choose a reason for hiding this comment

Uh oh!

amirh 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!

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!

chinmaygarde commented Aug 26, 2019

Uh oh!

liyuqian commented Aug 31, 2019

Uh oh!

chinmaygarde commented Sep 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants