Always composite PhysicalModels #28919

dnfield · 2019-03-06T00:43:00Z

Description

Currently, we only composite Physical layers for Fuchsia. This PR will enable compositing them on all platforms, which in turn will enable us to validate whether layers are being painted in the correct order or potentially paint with respect to elevation instead of the given painting order.

This PR should not land until flutter/engine#8044 has landed, which resolves an issue that would result in clipping shadows in this PR.

I'm not entirely sure how to test this - I don't believe the old fork in the logic was tested.

Related Issues

Fixes #27891
Helps #25673, #25226
Depends on flutter/engine#8044

Checklist

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.
My PR includes tests for all changed/updated/fixed behaviors (See Test Coverage).
All existing and new tests are passing.
I updated/added relevant documentation (doc comments with ///).
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes).
This PR will make it so that canvas operations we used to test for regarding shadows will no longer happen in the framework visible canvas - instead, changes will be found in the layer tree, and the engine side will do the canvas work.
No, this is not a breaking change.

mklim

LGTM, but @jason-simmons or @liyuqian should probably also review this before it's merged.

dnfield · 2019-03-07T02:10:08Z

As written right now, this is a breaking change. We'd no longer be calling canvas.drawShadow or clip*AndPaint, and we have tests that check a mock canvas for calls to these methods. The tests would have to be updated to check the layer tree instead, unless we were to make this opt in. I'm doing some performance investigation before trying to decide if this makes sense to have as opt in or not.

dnfield · 2019-03-07T04:17:38Z

This substantially improves Android benchmarks for gallery transitions perf, and slightly worsens them for worst frame time on iOS.

Android before and after on an S8:

"missed_transition_count": 0,
"average_frame_build_time_millis": 4.234034730538926 -> 2.987873549883989,
"90th_percentile_frame_build_time_millis": 8.53 -> 6.548,
"99th_percentile_frame_build_time_millis": 43.757 -> 25.773,
"worst_frame_build_time_millis": 92.395 -> 44.947, 
"missed_frame_build_budget_count": 47 -> 24,
"average_frame_rasterizer_time_millis": 6.817477326968965 -> 5.187597687861268,
"90th_percentile_frame_rasterizer_time_millis":  11.591 -> 7.675,
"99th_percentile_frame_rasterizer_time_millis": 22.694 -> 22.034,
"worst_frame_rasterizer_time_millis": 54.306 -> 50.733,
"missed_frame_rasterizer_budget_count": 32 -> 15,
"frame_count": 835 -> 862,

iOS before and after on an iPhone8+:

"missed_transition_count": 0 -> 0,
"average_frame_build_time_millis": 2.2832895277207355 -> 2.2099660493827167,
"90th_percentile_frame_build_time_millis":  3.779 -> 3.409,
"99th_percentile_frame_build_time_millis": 18.826 -> 18.897,
"worst_frame_build_time_millis": 57.393 -> 68.221,
"missed_frame_build_budget_count": 21 -> 21,
"average_frame_rasterizer_time_millis": 8.000753073770491 -> 6.932594871794868,
"90th_percentile_frame_rasterizer_time_millis": 16.571 -> 16.506,
"99th_percentile_frame_rasterizer_time_millis": 18.949 -> 17.271,
"worst_frame_rasterizer_time_millis": 54.931 -> 54.433,
"missed_frame_rasterizer_budget_count": 302 -> 272,
"frame_count":  974 -> 972,

I'm curious as to why there'd be such a difference between Android and iOS on this, but the improvement on Android is substantial.

dnfield · 2019-03-07T09:05:04Z

Adding @HansMuller since this required reworking tests for Material. This shouldn't actually be changing any behavior - although I was a bit puzzled by why OutlineButton sometimes seems to have two Materials in the mix.

dnfield · 2019-03-07T19:12:27Z

I'm looking into the golden failures.

dnfield · 2019-03-08T00:07:16Z

This results in nearly imperceptible changes to goldens. I've updated them as part of this PR.

…o composite_elevations

liyuqian

LGTM

liyuqian · 2019-03-12T21:19:47Z

packages/flutter/test/material/bottom_app_bar_test.dart

    await expectLater(
      find.byKey(key),
-      matchesGoldenFile('bottom_app_bar.custom_shape.1.png'),
+      matchesGoldenFile('bottom_app_bar.custom_shape.1.1.png'),


I guess that these new golden files are just temporary? Once this PR landed, we should be able to either replace the old png with this new png, or delete the old png?

We delete the old ones.

The instructions are to append a number to the end of the name - unfortunately for this file, it already has a number at the end of its name.

goderbauer

LGTM

Seems fine if performance is not negatively impacted :)

goderbauer · 2019-03-12T21:39:23Z

packages/flutter/test/widgets/nested_scroll_view_test.dart

    expect(find.text('Item 2'), findsOneWidget);
    expect(find.text('Item 18'), findsNothing);
-    expect(find.byType(NestedScrollView), isNot(paints..shadow()));
+    _checkPhysicalLayer(0);


nit: maybe make elevation a required named parameter so it's clear right here what the 0 is?

dnfield · 2019-03-12T22:49:51Z

I did another test on an iPhone 6, which is closer to what the devicelab uses if not exactly:

"missed_transition_count": 0 -> 0,
"average_frame_build_time_millis": 1.6932270833333336 -> 1.9984098018769552,
"90th_percentile_frame_build_time_millis": 2.866 -> 3.214,
"99th_percentile_frame_build_time_millis": 11.404 -> 16.865,
"worst_frame_build_time_millis": 49.945 -> 50.881,
"missed_frame_build_budget_count": 5 -> 12,
"average_frame_rasterizer_time_millis": 10.122177570093447 -> 7.59421413721414,
"90th_percentile_frame_rasterizer_time_millis": 16.735 -> 16.662,
"99th_percentile_frame_rasterizer_time_millis": 22.189 -> 22.699,
"worst_frame_rasterizer_time_millis": 118.773 -> 116.67,
"missed_frame_rasterizer_budget_count": 439 -> 288,
"frame_count": 960 -> 959,

It looks like the numbers go slightly up, but are still under our goals. That said, these numbers are still a little lower than devicelab so it may be on a less powerful 6 or 5 or something.

dnfield · 2019-03-12T23:33:51Z

Tree isn't actually red, landing this.

dnfield · 2019-03-13T02:28:14Z

complex_layout_scroll_perf__timeline_summary average_frame_rasterizer_time_millis went red on this PR. Everything else seems better or just noisy.

dnfield · 2019-03-13T02:28:41Z

@liyuqian can you help me look into that regression tomorrow?

dnfield · 2019-03-13T03:46:37Z

omplex_layout_ios__start_up timeToFirstFrameMicros also may have regressed, although that one has been noisy.

dnfield · 2019-03-13T04:06:46Z

Actually, I strongly suspect the regression on the rasterizer is expected. Rasterizer average time went up by ~.5ms, but frame build time went down by ~.5 ms. If I'm understanding correctly, this is because we moved some work from the framework to the engine.

liyuqian · 2019-03-13T17:28:20Z

Calling canvas.drawXXX in the UI thread before this PR or in the GPU thread after this PR shouldn't be too much a difference. In the end, everything goes to the GPU thread for SkCanvas::flush where the final costly rendering happens.

I suspect that this PR changes how the layer tree looks like and that affects our raster caching. For example, before this PR, the PhysicalShapeLayer is painted on the UI thread so everything is recorded in an SkPicture which is ready for raster cache. After this PR, it's no longer recorded in an SkPicture so no raster cache is done. It explains why the average raster time changes more than other metrics (e.g., 90th percentile).

I wouldn't worry about the 0.5ms regression here if this PR fixes some critical correctness issues.

Finally, I'm more curious why the MotoG4/iPhone6S in our device lab gave us very different performance changes than the Galaxy S8 and iPhone8+ as described in this PR. Maybe the older devices need more raster cache to be performant? It would be nice to confirm that in local tests with MotoG4.

To test whether it's the raster cache that's causing a difference, one can let IsPictureWorthRasterizing always return false.

liyuqian · 2019-03-13T17:31:06Z

@liyuqian can you help me look into that regression tomorrow?

Sure, I'm happy to help any time.

This reverts commit 58fb183.

Always composite PhysicalModels

8cf5006

dnfield requested review from jason-simmons, liyuqian and mklim March 6, 2019 00:43

googlebot added the cla: yes label Mar 6, 2019

goderbauer added the framework flutter/packages/flutter repository. See also f: labels. label Mar 6, 2019

mklim approved these changes Mar 6, 2019

View reviewed changes

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

c6fbad7

dnfield added the c: API break Backwards-incompatible API changes label Mar 7, 2019

dnfield added 2 commits March 7, 2019 01:01

rework tests

88c312f

revert unintentionally changed file

03c35b6

dnfield marked this pull request as ready for review March 7, 2019 09:03

dnfield requested review from HansMuller and goderbauer March 7, 2019 09:04

unused import

86a4f0a

dnfield force-pushed the composite_elevations branch from ee00a09 to 86a4f0a Compare March 7, 2019 16:44

dnfield added 3 commits March 7, 2019 09:37

skip golden test when !Linux

8c0ccbc

print failures

ed38df2

finish formatting!

018dfa2

update goldens

99b6b4f

dnfield added 2 commits March 7, 2019 17:01

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

dc62f86

Merge branch 'composite_elevations' of github.com:dnfield/flutter int…

e19a298

…o composite_elevations

This comment has been minimized.

Sign in to view

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

8fe2219

liyuqian approved these changes Mar 12, 2019

View reviewed changes

goderbauer reviewed Mar 12, 2019

View reviewed changes

dnfield added 2 commits March 12, 2019 14:54

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

325e3f1

Use named parameter

baf8708

dnfield merged commit 58fb183 into flutter:master Mar 12, 2019

dnfield deleted the composite_elevations branch March 12, 2019 23:34

dnfield added a commit that referenced this pull request Mar 13, 2019

Revert "Always composite PhysicalModels (#28919)"

e93a68a

This reverts commit 58fb183.

This was referenced Mar 13, 2019

Revert "Always composite PhysicalModels" #29314

Merged

Anti-Aliasing for shape layers flutter/engine#8157

Merged

willlarche mentioned this pull request Mar 14, 2019

[Engine] Restart Skia roller when safe #29268

Closed

willlarche pushed a commit that referenced this pull request Mar 14, 2019

Revert "Always composite PhysicalModels (#28919)" (#29314)

7ac2486

This reverts commit 58fb183.

dnfield mentioned this pull request Mar 18, 2019

Reland composite physical layers on all platforms #29532

Merged

10 tasks

github-actions bot locked as resolved and limited conversation to collaborators Aug 7, 2021

Always composite PhysicalModels #28919

Always composite PhysicalModels #28919

Uh oh!

Conversation

dnfield commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Breaking Change

Uh oh!

mklim left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfield commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfield commented Mar 7, 2019

Uh oh!

dnfield commented Mar 7, 2019

Uh oh!

dnfield commented Mar 8, 2019

Uh oh!

This comment has been minimized.

liyuqian left a comment

Choose a reason for hiding this comment

Uh oh!

liyuqian Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

dnfield Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

dnfield Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

goderbauer Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

dnfield Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

dnfield commented Mar 12, 2019

Uh oh!

dnfield commented Mar 12, 2019

Uh oh!

dnfield commented Mar 13, 2019

Uh oh!

dnfield commented Mar 13, 2019

Uh oh!

dnfield commented Mar 13, 2019

Uh oh!

dnfield commented Mar 13, 2019

Uh oh!

liyuqian commented Mar 13, 2019

Uh oh!

liyuqian commented Mar 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dnfield commented Mar 6, 2019 •

edited

Loading

dnfield commented Mar 7, 2019 •

edited

Loading

dnfield commented Mar 7, 2019 •

edited

Loading