Turn on RasterCache based on view hierarchy #13360

mklim · 2019-10-25T20:13:01Z

Previously the cache was disabled on whether or not PlatformViews were
globally enabled. Instead track their existence in the view hierarchy
and only disable RasterCache if a PlatformView is actually present.

Fixes flutter/flutter#38903

mklim · 2019-10-25T20:13:31Z

flow/layers/layer_tree.cc

    root_layer_->Preroll(&preroll_context, root_surface_transformation);
    // The needs painting flag may be set after the preroll. So check it after.
    if (root_layer_->needs_painting()) {
+      paint_context.has_platform_view = preroll_context.has_platform_view;


Is this necessary? None of the other variables from preroll_context are copied over here. I wasn't sure if this root_layer_ was guaranteed to be a single node with no children at this point or not.

Previously the cache was disabled on whether or not PlatformViews were globally enabled. Instead track their existence in the view hierarchy and only disable RasterCache if a PlatformView is actually present.

amirh · 2019-10-25T22:31:26Z

IIUC the current code splits the tree into 2 parts (before and after hitting the first platform view) and raster cache is disabled for the later part?

I'm thinking we should probably set has_platform_view to false before visiting each child here

engine/flow/layers/container_layer.cc

Line 30 in 87b00d3

layer->Preroll(context, child_matrix);

and at the end of the loop set it to true if it was true after prerolling one or more children?

mklim · 2019-10-25T23:19:59Z

I'm thinking we should probably set has_platform_view to false before visiting each child here

Good catch, I tested this out with a demo app and a sibling of a PlatformView was also having its RasterCache disabled. This should be fixed now. I didn't set it to false but instead set it to whatever the ContainerLayer already had. My reasoning was that if the ContainerLayer itself was somehow a child of a PlatformView, then all its children should have it disabled.

amirh · 2019-10-25T23:30:46Z

I'm thinking we should probably set has_platform_view to false before visiting each child here

Good catch, I tested this out with a demo app and a sibling of a PlatformView was also having its RasterCache disabled. This should be fixed now. I didn't set it to false but instead set it to whatever the ContainerLayer already had. My reasoning was that if the ContainerLayer itself was somehow a child of a PlatformView, then all its children should have it disabled.

Thanks. Not sure it matters as a platform view layer has no children flow layers.

chinmaygarde · 2019-10-28T20:01:28Z

flow/layers/layer.h

    TextureRegistry& texture_registry;
    const RasterCache* raster_cache;
    const bool checkerboard_offscreen_layers;
+    bool has_platform_view;


Not sure why this new field is necessary. If the raster cache needs to be disabled, the raster_cache pointer can just not be supplied in the paint context. There is nothing more to do.

It's not necessary, removed.

amirh · 2019-10-28T23:35:56Z

flow/layers/container_layer.cc

 void ContainerLayer::PrerollChildren(PrerollContext* context,
                                     const SkMatrix& child_matrix,
                                     SkRect* child_paint_bounds) {
+  const bool had_platform_view = context->has_platform_view;


nit: I'd keep the simpler version of this where we assume platform view has no children (it is a leaf layer)

Done. I did put in a debug assert just to try and catch in case this assumption ever changes in the future.

flow/layers/opacity_layer.cc

amirh · 2019-10-28T23:45:07Z

I'm thinking we should probably set has_platform_view to false before visiting each child here

Good catch, I tested this out with a demo app and a sibling of a PlatformView was also having its RasterCache disabled. This should be fixed now. I didn't set it to false but instead set it to whatever the ContainerLayer already had. My reasoning was that if the ContainerLayer itself was somehow a child of a PlatformView, then all its children should have it disabled.

We should probably add a test for the case we're using raster cache after a platform view

- Test for platform view layer as a sibling of layers that need to be cached to make sure the context is being reset between children. - Minor code cleanup.

mklim · 2019-10-29T21:45:39Z

We should probably add a test for the case we're using raster cache after a platform view

Done. I converted the previous test case instead of adding an entirely separate one. I think testing that the platform view layer isn't cached but sibling layers are also covers the case of checking that platform views aren't cached in general.

amirh

LGTM

(assuming that Chinmay approves the changes to shell and embedder_engine)

amirh · 2019-10-29T22:27:01Z

flow/raster_cache.cc

-                                layer->Paint(paintContext);
-                              }
-                            });
+    entry.image = Rasterize(


I guess the formatting of this file is going to depend on the last editor? 😝
@chinmaygarde

[not blocking this PR]

The actual change here is that paintContext.rasterCache is getting set to context->has_platform_view ? nullptr : context->raster_cache instead of just context->raster_cache. That statement is so long that clang-format decided to move this whole block further to the left to make room for it. I'm not sure why it was moved further to the right to start with, but it should be consistent between editors and CI.

chinmaygarde

Some nits but looks good overall. Thanks!

flow/raster_cache.h

shell/common/shell.cc

chinmaygarde · 2019-10-29T23:38:53Z

shell/platform/embedder/tests/embedder_unittests.cc

+
+  setup.Wait();
+  const flutter::Shell& shell =
+      reinterpret_cast<flutter::EmbedderEngine*>(engine.get())->shell();


This reinterpret cast depends on an implementation detail in embedder.cc. Please add a method to any file other than embedder.h that basically does just this cast. The implmentation of the method itself can be in embedder.cc though.

Moved to embedder_assertions.h with the other utility conversion methods.

shell/platform/embedder/embedder_engine.h

This caused EmbedderTest.VerifyB143464703 to fail after merging into master. ``` ../../flutter/shell/platform/embedder/tests/embedder_unittests.cc:3111: Failure Value of: ImageMatchesFixture("verifyb143464703.png", renderered_scene) Actual: false Expected: true [ FAILED ] EmbedderTest.VerifyB143464703 (2507 ms) ``` This reverts commit 3ad3bc7.

…3360)

[email protected]:flutter/engine.git/compare/5051bef17bdc...e609577 git log 5051bef..e609577 --no-merges --oneline 2019-10-30 [email protected] Revert "Turn on RasterCache based on view hierarchy (#13360)" (flutter/engine#13442) 2019-10-30 [email protected] Turn on RasterCache based on view hierarchy (flutter/engine#13360) 2019-10-30 [email protected] Roll src/third_party/dart d3a5b82355..f30b494035 (9 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 [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

This is a duplicate of #13360 with the test switched to use the software backend instead of the GL backend. After some debugging and testing on another GL embedder I think the issue with the test is some bug having to do with the GL implementation in the test harness specifically. Fixes flutter/flutter#38903

[email protected]:flutter/engine.git/compare/5051bef17bdc...e609577 git log 5051bef..e609577 --no-merges --oneline 2019-10-30 [email protected] Revert "Turn on RasterCache based on view hierarchy (flutter#13360)" (flutter/engine#13442) 2019-10-30 [email protected] Turn on RasterCache based on view hierarchy (flutter/engine#13360) 2019-10-30 [email protected] Roll src/third_party/dart d3a5b82355..f30b494035 (9 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 [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

mklim requested review from chinmaygarde and iskakaushik October 25, 2019 20:13

googlebot added the cla: yes label Oct 25, 2019

mklim commented Oct 25, 2019

View reviewed changes

Turn on RasterCache based on view hierarchy

de23efd

Previously the cache was disabled on whether or not PlatformViews were globally enabled. Instead track their existence in the view hierarchy and only disable RasterCache if a PlatformView is actually present.

mklim requested a review from amirh October 25, 2019 21:56

Fix siblings being incorrectly also disabled

5031137

chinmaygarde reviewed Oct 28, 2019

View reviewed changes

Remove extraneous PaintContext variable

b40a245

amirh reviewed Oct 28, 2019

View reviewed changes

More review feedback

fbb9ce6

- Test for platform view layer as a sibling of layers that need to be cached to make sure the context is being reset between children. - Minor code cleanup.

amirh approved these changes Oct 29, 2019

View reviewed changes

chinmaygarde reviewed Oct 29, 2019

View reviewed changes

Review feedback

3a7f595

mklim merged commit 3ad3bc7 into flutter:master Oct 30, 2019

mklim deleted the raster_cache branch October 30, 2019 17:45

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

3ad3bc7 Turn on RasterCache based on view hierarchy (flutter/engine#1…

526a9b5

…3360)

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

Roll engine 5051bef17bdc..e609577d1201 (3 commits) flutter/flutter#43847

Closed

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

3ad3bc7 Turn on RasterCache based on view hierarchy (flutter/engine#1…

8c615aa

…3360)

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

Roll engine 5051bef17bdc..e609577d1201 (3 commits) flutter/flutter#43851

Closed

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

3ad3bc7 Turn on RasterCache based on view hierarchy (flutter/engine#1…

a413385

…3360)

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

Roll engine 5051bef17bdc..e609577d1201 (3 commits) flutter/flutter#43854

Merged

mklim mentioned this pull request Oct 30, 2019

Use RasterCache where possible even when we have embedded views enabled. flutter/flutter#38903

Closed

mklim mentioned this pull request Nov 8, 2019

Turn on RasterCache based on view hierarchy #13762

Merged

Turn on RasterCache based on view hierarchy #13360

Turn on RasterCache based on view hierarchy #13360

Uh oh!

Conversation

mklim commented Oct 25, 2019

Uh oh!

mklim Oct 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirh commented Oct 25, 2019

Uh oh!

mklim commented Oct 25, 2019

Uh oh!

amirh commented Oct 25, 2019

Uh oh!

chinmaygarde Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

mklim Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

amirh Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

mklim Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amirh commented Oct 28, 2019

Uh oh!

mklim commented Oct 29, 2019

Uh oh!

amirh left a comment

Choose a reason for hiding this comment

Uh oh!

amirh Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

mklim Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chinmaygarde Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

mklim Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mklim Oct 25, 2019 •

edited

Loading

mklim Oct 29, 2019 •

edited

Loading