Discard wrong size layer tree instead of rendering it #21179

knopp · 2020-09-15T15:32:41Z

Description

This PR supersedes #21108.

This PR will ensure that after viewport change metrics event is received, layer tree with previous (wrong size) are discarded and not rasterized. This is required for smooth resizing on both Mac and Windows.

Important: This will potentially affect iOS and Android embedders. We need to make sure this doesn't cause any issues.

Related Issues

flutter/flutter#44136

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.

If this callback returns true for given layer tree, the layer tree will be discarded and not rasterized This is useful after the resize event, when layer tree with certain size is expected, but there is a tree with previous size still in pipeline

flutter-dashboard · 2020-09-15T15:32:43Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

iskakaushik · 2020-09-15T15:39:46Z

Looks good overall but requires tests as mentioned in this comment: #21108 (review). Let me know if you run into issues writing tests.

knopp · 2020-09-15T20:58:27Z

I added test for this:
d01b804
642e383

iskakaushik

Left some improvements to the test, we should be good to merge this PR once those are fixed.

iskakaushik · 2020-09-15T22:58:20Z

shell/common/shell_unittests.cc

+          auto layer_tree =
+              std::make_unique<LayerTree>(size, static_cast<float>(1));
+          SkMatrix identity;
+          identity.setIdentity();


nit: I thought SkMatrix was default initialized to identiy.

I copied that snippet from another test, but you're right. Unlike other types SkMatrix actually seems to have sane default value.

iskakaushik · 2020-09-15T22:59:09Z

shell/common/shell_unittests.cc

+      shell->GetTaskRunners().GetPlatformTaskRunner(),
+      [&shell, &expected_size]() {
+        shell->GetPlatformView()->SetViewportMetrics(
+            {1.0, double(expected_size.width()),


nit: can these be static_cast<double>?

iskakaushik · 2020-09-15T23:00:28Z

shell/common/shell_unittests.cc

+    shell->GetTaskRunners().GetUITaskRunner()->PostTask(
+        [runtime_delegate, size]() {
+          auto layer_tree =
+              std::make_unique<LayerTree>(size, static_cast<float>(1));


nit: this can be 1.0f.

Indeed. Not sure why it was written like this.

shell/common/shell_unittests.cc

iskakaushik

Is this still a WIP or do you think we can land this once the presubmits pass?

If you think it's ready, please remove WIP from the title of the PR.

iskakaushik · 2020-09-16T15:14:41Z

shell/common/shell_test_external_view_embedder.cc

    GrDirectContext* context,
    std::unique_ptr<SurfaceFrame> frame) {
  frame->Submit();
+  if (frame && frame->SkiaSurface()) {


Windows toolchain doesn't seem to be liking this:

C:\b\s\w\ir\kitchen-checkout\depot_tools\win_toolchain\vs_files\418b3076791776573a815eb298c8aa590307af63\win_sdk\bin\..\..\VC\Tools\MSVC\14.16.27023\include\atomic(620,2): error: static_assert failed due to requirement 'alignof(SkISize) >= sizeof(unsigned long long)' "You've instantiated std::atomic<T> with sizeof(T) equal to 2/4/8 and alignof(T) < sizeof(T). Before VS 2015 Update 2, this would have misbehaved at runtime. VS 2015 Update 2 was fixed to handle this correctly, but the fix inherently changes layout and breaks binary compatibility. Please define _ENABLE_ATOMIC_ALIGNMENT_FIX to acknowledge that you understand this, and that everything you're linking has been compiled with VS 2015 Update 2 (or later)." static_assert(alignof(_Ty) >= sizeof(_My_int), ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ C:\b\s\w\ir\kitchen-checkout\depot_tools\win_toolchain\vs_files\418b3076791776573a815eb298c8aa590307af63\win_sdk\bin\..\..\VC\Tools\MSVC\14.16.27023\include\atomic(635,5): note: in instantiation of template class 'std::_Atomic_base<SkISize, 8>' requested here : _Atomic_base<_Ty, sizeof (_Ty)> ^ ../../flutter/shell/common/shell_test_external_view_embedder.h(86,24): note: in instantiation of template class 'std::atomic<SkISize>' requested here std::atomic<SkISize> last_submitted_frame_size_; ^ 1 error generated.

See https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket.appspot.com/8869006746431748192/+/steps/build_host_debug/0/stdout for full error.

This isn't going to be an issue in future, in the meanwhile perhaps we could define _ENABLE_ATOMIC_ALIGNMENT_FIX. There's nothing wrong with the code, it's just a windows STL quirk.

iskakaushik · 2020-09-16T15:15:36Z

shell/common/shell_unittests.cc

+    end_frame_latch.Signal();
+  };
+
+  external_view_embedder = std::make_shared<ShellTestExternalViewEmbedder>(


nit: declare this variable here rather than line:2020. See https://google.github.io/styleguide/cppguide.html#Local_Variables

Yeah. It was accessed from the callback before. I'll move it.

shell/common/shell_unittests.cc

iskakaushik · 2020-09-17T20:25:06Z

@knopp I still see two errors only on windows:

C:\b\s\w\ir\kitchen-checkout\depot_tools\win_toolchain\vs_files\418b3076791776573a815eb298c8aa590307af63\win_sdk\bin\..\..\VC\Tools\MSVC\14.16.27023\include\atomic(620,2): error: static_assert failed due to requirement 'alignof(SkISize) >= sizeof(unsigned long long)' "You've instantiated std::atomic<T> with sizeof(T) equal to 2/4/8 and alignof(T) < sizeof(T). Before VS 2015 Update 2, this would have misbehaved at runtime. VS 2015 Update 2 was fixed to handle this correctly, but the fix inherently changes layout and breaks binary compatibility. Please define _ENABLE_ATOMIC_ALIGNMENT_FIX to acknowledge that you understand this, and that everything you're linking has been compiled with VS 2015 Update 2 (or later)."
        static_assert(alignof(_Ty) >= sizeof(_My_int),
        ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:\b\s\w\ir\kitchen-checkout\depot_tools\win_toolchain\vs_files\418b3076791776573a815eb298c8aa590307af63\win_sdk\bin\..\..\VC\Tools\MSVC\14.16.27023\include\atomic(635,5): note: in instantiation of template class 'std::_Atomic_base<SkISize, 8>' requested here
                : _Atomic_base<_Ty, sizeof (_Ty)>
                  ^
../..\flutter/shell/common/shell_test_external_view_embedder.h(91,24): note: in instantiation of template class 'std::atomic<SkISize>' requested here
  std::atomic<SkISize> last_submitted_frame_size_;
                       ^
1 error generated.

and

In file included from ../../third_party/skia\include/core/SkRefCnt.h:13:
C:\b\s\w\ir\kitchen-checkout\depot_tools\win_toolchain\vs_files\418b3076791776573a815eb298c8aa590307af63\win_sdk\bin\..\..\VC\Tools\MSVC\14.16.27023\include\atomic(620,2): error: static_assert failed due to requirement 'alignof(SkISize) >= sizeof(unsigned long long)' "You've instantiated std::atomic<T> with sizeof(T) equal to 2/4/8 and alignof(T) < sizeof(T). Before VS 2015 Update 2, this would have misbehaved at runtime. VS 2015 Update 2 was fixed to handle this correctly, but the fix inherently changes layout and breaks binary compatibility. Please define _ENABLE_ATOMIC_ALIGNMENT_FIX to acknowledge that you understand this, and that everything you're linking has been compiled with VS 2015 Update 2 (or later)."
        static_assert(alignof(_Ty) >= sizeof(_My_int),
        ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:\b\s\w\ir\kitchen-checkout\depot_tools\win_toolchain\vs_files\418b3076791776573a815eb298c8aa590307af63\win_sdk\bin\..\..\VC\Tools\MSVC\14.16.27023\include\atomic(635,5): note: in instantiation of template class 'std::_Atomic_base<SkISize, 8>' requested here
                : _Atomic_base<_Ty, sizeof (_Ty)>
                  ^
../..\flutter/shell/common/shell_test_external_view_embedder.h(91,24): note: in instantiation of template class 'std::atomic<SkISize>' requested here
  std::atomic<SkISize> last_submitted_frame_size_;
                       ^
../../flutter/shell/common/shell_test_platform_view_gl.cc(78,10): error: cannot initialize return object of type 'flutter::ExternalViewEmbedder *' with an rvalue of type 'std::_Ptr_base<flutter::ShellTestExternalViewEmbedder>::element_type *' (aka 'flutter::ShellTestExternalViewEmbedder *')
  return shell_test_external_view_embedder_.get();
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.

Still seem to be artifacts of STL on windows?

knopp · 2020-09-17T20:55:22Z

It is artifact of windows STL, but the define on top of the file should have taken care of it. I'm building now on windows locally.

iskakaushik · 2020-09-17T21:36:24Z

seems like formatting is off in BUILD.gn.

iskakaushik

LGTM

…r/engine#21179)

googlebot added the cla: yes label Sep 15, 2020

knopp mentioned this pull request Sep 15, 2020

WIP: Discard wrong size layer tree instead of rendering it #21108

Closed

12 tasks

Do not rasterize wrong size layer tree after viewport metrics change

9dab1f2

knopp force-pushed the discard_wrong_size_tree_wip_pr2 branch from 981a8f5 to 9dab1f2 Compare September 15, 2020 15:54

cbracken added the Work in progress (WIP) Not ready (yet) for review! label Sep 15, 2020

iskakaushik mentioned this pull request Sep 15, 2020

LUCI infra failure on "Linux Android Scenarios" try job for flutter engine PRs flutter/flutter#65899

Closed

iskakaushik reviewed Sep 15, 2020

View reviewed changes

knopp force-pushed the discard_wrong_size_tree_wip_pr2 branch from 642e383 to 58f1b71 Compare September 16, 2020 10:44

iskakaushik reviewed Sep 16, 2020

View reviewed changes

knopp force-pushed the discard_wrong_size_tree_wip_pr2 branch 2 times, most recently from 1a57215 to 146a855 Compare September 16, 2020 17:18

knopp force-pushed the discard_wrong_size_tree_wip_pr2 branch from a78fe6f to 19dad88 Compare September 17, 2020 21:08

knopp force-pushed the discard_wrong_size_tree_wip_pr2 branch 3 times, most recently from 08257a4 to 1e524f5 Compare September 17, 2020 22:29

knopp added 2 commits September 18, 2020 00:31

Provide frame size of last submitted frame

8370a9e

Add ShellTest.DiscardLayerTreeOnResize

9fccda2

knopp force-pushed the discard_wrong_size_tree_wip_pr2 branch from 1e524f5 to 9fccda2 Compare September 17, 2020 22:31

knopp changed the title ~~WIP: Discard wrong size layer tree instead of rendering it~~ Discard wrong size layer tree instead of rendering it Sep 18, 2020

iskakaushik removed the Work in progress (WIP) Not ready (yet) for review! label Sep 18, 2020

iskakaushik approved these changes Sep 18, 2020

View reviewed changes

iskakaushik added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Sep 18, 2020

fluttergithubbot merged commit 5c9dddc into flutter:master Sep 18, 2020

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 22, 2020

5c9dddc Discard wrong size layer tree instead of rendering it (flutte…

eeba639

…r/engine#21179)

engine-flutter-autoroll mentioned this pull request Sep 22, 2020

Roll Engine from 4b8477d11573 to 1ee3364df1c0 (24 revisions) flutter/flutter#66329

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 22, 2020

5c9dddc Discard wrong size layer tree instead of rendering it (flutte…

5ac02db

…r/engine#21179)

engine-flutter-autoroll mentioned this pull request Sep 22, 2020

Roll Engine from 4b8477d11573 to ecedc854c473 (27 revisions) flutter/flutter#66337

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 22, 2020

5c9dddc Discard wrong size layer tree instead of rendering it (flutte…

8c5863a

…r/engine#21179)

engine-flutter-autoroll mentioned this pull request Sep 22, 2020

Roll Engine from 4b8477d11573 to c73732616b0b (28 revisions) flutter/flutter#66340

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 22, 2020

5c9dddc Discard wrong size layer tree instead of rendering it (flutte…

3001258

…r/engine#21179)

engine-flutter-autoroll mentioned this pull request Sep 22, 2020

Roll Engine from 4b8477d11573 to 98cdb7d82d35 (29 revisions) flutter/flutter#66346

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 22, 2020

5c9dddc Discard wrong size layer tree instead of rendering it (flutte…

033afe1

…r/engine#21179)

engine-flutter-autoroll mentioned this pull request Sep 22, 2020

Roll Engine from 4b8477d11573 to 83c9d103edd4 (30 revisions) flutter/flutter#66353

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 22, 2020

5c9dddc Discard wrong size layer tree instead of rendering it (flutte…

ce90460

…r/engine#21179)

engine-flutter-autoroll mentioned this pull request Sep 22, 2020

Roll Engine from 4b8477d11573 to 24df09ddd126 (31 revisions) flutter/flutter#66362

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 22, 2020

5c9dddc Discard wrong size layer tree instead of rendering it (flutte…

7b7ddd1

…r/engine#21179)

engine-flutter-autoroll mentioned this pull request Sep 22, 2020

Roll Engine from 4b8477d11573 to fcb56ec20309 (32 revisions) flutter/flutter#66365

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 22, 2020

5c9dddc Discard wrong size layer tree instead of rendering it (flutte…

117f1ee

…r/engine#21179)

engine-flutter-autoroll mentioned this pull request Sep 22, 2020

Roll Engine from 4b8477d11573 to 88e53777d1aa (34 revisions) flutter/flutter#66371

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 22, 2020

5c9dddc Discard wrong size layer tree instead of rendering it (flutte…

334b328

…r/engine#21179)

engine-flutter-autoroll mentioned this pull request Sep 22, 2020

Roll Engine from 4b8477d11573 to c0aa47e7fe60 (35 revisions) flutter/flutter#66378

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 22, 2020

5c9dddc Discard wrong size layer tree instead of rendering it (flutte…

48bf2ec

…r/engine#21179)

engine-flutter-autoroll mentioned this pull request Sep 22, 2020

Roll Engine from 4b8477d11573 to 84d50d71d20b (36 revisions) flutter/flutter#66388

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 22, 2020

5c9dddc Discard wrong size layer tree instead of rendering it (flutte…

49a2ee5

…r/engine#21179)

engine-flutter-autoroll mentioned this pull request Sep 22, 2020

Roll Engine from 4b8477d11573 to 69e24105951e (40 revisions) flutter/flutter#66396

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 22, 2020

5c9dddc Discard wrong size layer tree instead of rendering it (flutte…

e2003c2

…r/engine#21179)

liyuqian added perf: speed Performance issues related to (mostly rendering) speed severe: performance Relates to speed or footprint issues. labels Sep 22, 2020

engine-flutter-autoroll mentioned this pull request Sep 22, 2020

Roll Engine from 4b8477d11573 to c62d6652992b (44 revisions) flutter/flutter#66398

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 22, 2020

5c9dddc Discard wrong size layer tree instead of rendering it (flutte…

a25b82a

…r/engine#21179)

knopp deleted the discard_wrong_size_tree_wip_pr2 branch July 17, 2021 09:26

This was referenced Aug 31, 2021

Started providing the GPU sync switch to Rasterizer.DrawToSurface() #28383

Merged

Discard the resubmitted layer tree of the wrong size instead of rendering it #28474

Merged

Discard wrong size layer tree instead of rendering it #21179

Discard wrong size layer tree instead of rendering it #21179

Uh oh!

Conversation

knopp commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Breaking Change

Uh oh!

flutter-dashboard bot commented Sep 15, 2020

Uh oh!

iskakaushik commented Sep 15, 2020

Uh oh!

knopp commented Sep 15, 2020

Uh oh!

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

Uh oh!

iskakaushik left a comment • 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!

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!

Uh oh!

iskakaushik commented Sep 17, 2020

Uh oh!

knopp commented Sep 17, 2020

Uh oh!

iskakaushik commented Sep 17, 2020

Uh oh!

iskakaushik left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

knopp commented Sep 15, 2020 •

edited

Loading

iskakaushik left a comment •

edited

Loading