[fuchsia] Wire up OpacityLayer to Scenic #11322

arbreng · 2019-08-20T23:17:49Z

[fuchsia] Wire up OpacityLayer to Scenic

On Fuchsia, add a build flag for compositing OpacityLayers using the system
compositor vs Skia, which exposes a fastpath for opacity via Scenic.
This will only work under certain circumstances, in particular nested
OpacityLayers will not render correctly!

On Fuchsia, add a build flag for compositing PhysicalShapeLayers using
the system compositor vs Skia. Set to off by default, which restores
performant shadows on Fuchsia.

Remove the opacity exposed from ChildView, as that was added mistakenly.

Finally, we centralize the logic for switching between the
system-composited and in-process-composited paths inside of
ContainerLayer. We also centralize the logic for computing elevation
there. This allows the removal of many OS_FUCHSIA-specific code-paths.

Test: Ran workstation on Fuchsia; benchmarked before and after
Bug: 23711
Bug: 24163

arbreng · 2019-08-20T23:20:34Z

flow/layers/physical_shape_layer.cc

    set_paint_bounds(path_.getBounds());
  } else {
 #if defined(OS_FUCHSIA)
-    // Let the system compositor draw all shadows for us.


Sorry this diff is so terrible. I copy-pasted all of this shadow code as-is, I just added an if() else around it

Do you have some benchmarks that compare the performance between kRenderPhysicalShapeUsingSystemCompositor is true and false?

I focused on redoing the CL for your other comments. I will work on getting benchmarks now

In addition to Fuchsia's performance benchmarks, it would also be nice to attach some performance numbers from transition_perf driver tests in the framework by cd ${FLUTTER_FRAMEWORK_REPO}/examples/flutter_gallery/ && flutter drive --profile --local-engine=android_profile test_driver/transitions_perf.dart.

It would be much easier to catch any performance regression early than to let the engine roll find the regression and then roll back.

flow/layers/opacity_layer.cc

flow/scene_update_context.h

flow/layers/opacity_layer.cc

flow/layers/child_scene_layer.cc

flow/layers/layer_tree.cc

flow/layers/opacity_layer.cc

flow/layers/opacity_layer.h

flow/layers/opacity_layer.cc

flow/layers/physical_shape_layer.cc

flow/scene_update_context.cc

flow/layers/child_scene_layer.cc

mikejurka · 2019-09-06T21:09:05Z

+1

liyuqian

I saved a comment yesterday and I can't find it today on Github (probably due to new commits)... However, there's a "1" in the "Review changes" button, so I'll try to submit the review to surface my comment yesterday...

flow/layers/container_layer.cc

liyuqian

Commit 91658cb looks good to me! Will resume reviewing more commits tomorrow.

amott9 · 2019-09-11T18:59:04Z

flow/scene_update_context.cc

-    // Clamp the local z coordinate at our max bound. Take into account the
-    // parent z position here to fix clamping in cases where the child is
-    // overflowing because of its parents.
-    const float parent_elevation = world_elevation - local_elevation;


This code is still necessary to make sure that elevations are clamped within their view bounds (z bound). Otherwise, on Fuchsia, layers can be clipped by the scenic view bounds, and never show on the screen.
@mklim to verify

Yeah, correct that we still need it. At a glance it looks like this functionality has been moved into ClampElevation in this PR. From what I can tell it seems like that should be called and clamping -elevation before it gets to this call, but it's hard to tell for sure and I'd be worried that down the line someone would modify elevation here without realizing that it shouldn't be moved post-clamp. I'll add this to my queue to review more thoroughly.

Yep, I moved this logic into ClampElevation, because we are trying to reduce Fuchsia-specific code paths. A future PR is going to make these objects like Frame stateless, so we can make elevation const here if you want.

Yep! I moved this into ClampElevation because we are trying to reduce the number of Fuchsia-specific code paths. My next PR is going to make these objects like SceneUpdateContext::Frame stateless so we can make elevation const here if you want

Just got a chance to go through this in more detail. I have one question but other than that the elevation changes LG.

I don't think const would be super useful here since it's generally used as a language feature to make the variable itself immutable. It's not the elevation param specifically that matters here but more the larger context of where this frame is elevated shouldn't be altered at this point. I'm not sure the right way to convey that. It may be helpful to change the param name in this function from elevation to z_position and have UpdateScene pass in the -elevation itself, so that this function isn't expected to use a modified version of this value at all. Or leave a comment. But I don't think it's a huge risk either way.

amott9 · 2019-09-11T19:02:41Z

flow/layers/layer_tree.cc

+  SceneUpdateContext::Transform transform(context,                  // context
+                                          1.0f / metrics->scale_x,  // X
+                                          1.0f / metrics->scale_y,  // Y
+                                          1.0f / metrics->scale_z   // Z


Are these all guaranteed to be non-zero?

Good catch; they don't seem to be. I'll add some defensive DCHECKS.

Nope, nothing is checking them right now, good catch. I'll add some DCHECKs

liyuqian

Reviewed the second commit. Will resume tomorrow.

flow/scene_update_context.h

lib/ui/compositing/scene.cc

liyuqian · 2019-09-13T21:26:21Z

flow/layers/clip_rect_layer.h

  void Paint(PaintContext& context) const override;
-
-#if defined(OS_FUCHSIA)
  void UpdateScene(SceneUpdateContext& context) override;


@chinmaygarde : what's your take on UpdateScene and SceneUpdateContext in the future?

mklim · 2019-09-16T20:10:56Z

flow/layers/container_layer.cc

+  set_paint_bounds(child_paint_bounds);
+
+  // Restore the elevation for our parent.
+  context->total_elevation = parent_elevation_;


Why are we subtracting this entity's elevation from the total here?

The context tracks the total elevation in a stack-like fashion as we are walking the LayerTree depth first. Since we've finished computing elevations for the children of this ContainerLayer, we want to "pop" them off the stack so their elevations won't be incorrectly added into the total for subsequent "branches" of the LayerTree.

For example:

Stack(children: [
PhysicalShape(elevation: 1.0f, child: PhysicalShape(elevation: 1.0f)),
PhysicalShape(elevation: 1.0f)
]

would be incorrect without this line of code. The 2nd PhysicalShape in the Stack would end up with an elevation of 3, instead of 1.

Got, thank you! Makes sense. May be worth adding this explanation as a comment inline.

mklim · 2019-09-16T20:16:24Z

flow/scene_update_context.cc

-    // Clamp the local z coordinate at our max bound. Take into account the
-    // parent z position here to fix clamping in cases where the child is
-    // overflowing because of its parents.
-    const float parent_elevation = world_elevation - local_elevation;


Just got a chance to go through this in more detail. I have one question but other than that the elevation changes LG.

I don't think const would be super useful here since it's generally used as a language feature to make the variable itself immutable. It's not the elevation param specifically that matters here but more the larger context of where this frame is elevated shouldn't be altered at this point. I'm not sure the right way to convey that. It may be helpful to change the param name in this function from elevation to z_position and have UpdateScene pass in the -elevation itself, so that this function isn't expected to use a modified version of this value at all. Or leave a comment. But I don't think it's a huge risk either way.

liyuqian

Thanks @arbreng for your persistent improvements over this change! It's now not only much cleaner compared to your first version, but also much better than many of our existing code.

I've left several more nits and concerns. The major one is that the system-composited layer seems to have much code interleaved with the non-system-composited case. It may be difficult for our future improvements and maintenance if there's some performance regression, or if we choose to do something else with SceneUpdateContext.

I think this is good to land as soon as that's addressed.

flow/layers/clip_path_layer.cc

flow/layers/clip_rect_layer.cc

flow/layers/clip_rrect_layer.cc

liyuqian · 2019-09-16T22:25:39Z

flow/layers/container_layer.h


 private:
  std::vector<std::shared_ptr<Layer>> layers_;
+  std::shared_ptr<ContainerLayer> single_child_;


It seems that layers_ and single_child_ cannot be used simultaneously. Can we make two subclasses MultiChildContainerLayer, SingleChildContainerLayer that inherits the abstract class ContainerLayer where MultiChildContainerLayer only has the vector, while SingleChildContainerLayer only has the single child pointer?

liyuqian · 2019-09-16T22:30:13Z

flow/layers/container_layer.h

+  SkColor frame_color_;
+  float parent_elevation_ = 0.0f;
+  float elevation_ = 0.0f;
+  float clamped_elevation_ = 0.0f;


We're adding many fields, and their related computations (e.g., elevation processing) to all ContainerLayers. Considering that we're going to discuss future SceneUpdateContext changes, maybe it's a nice idea to encapsulate all these fields and computations in some struct or class, so we can quickly find them and do some surgery. Also, for performance optimizations, it might be nice to have a simple if to short-circuit all those computations by default (outside of Fuchsia) so runtime branch prediction can make the overhead almost zero.

liyuqian · 2019-09-16T22:39:40Z

flow/layers/physical_shape_layer.cc

    set_paint_bounds(path_.getBounds());
  } else {
 #if defined(OS_FUCHSIA)
-    // Let the system compositor draw all shadows for us.


In addition to Fuchsia's performance benchmarks, it would also be nice to attach some performance numbers from transition_perf driver tests in the framework by cd ${FLUTTER_FRAMEWORK_REPO}/examples/flutter_gallery/ && flutter drive --profile --local-engine=android_profile test_driver/transitions_perf.dart.

It would be much easier to catch any performance regression early than to let the engine roll find the regression and then roll back.

flow/layers/opacity_layer.cc

liyuqian · 2019-09-16T22:53:19Z

flow/layers/opacity_layer.cc

-    SkMatrix ctm = child_matrix;
+  // When using the system compositor, do not include the offset or use the
+  // raster cache, since we are rendering as a separate piece of geometry.
+  if (kRenderOpacityUsingSystemCompositor) {


Instead of having a single OpacityLayer and having an if branch in both Preroll and Paint, I wonder if it makes sense to create two subclasses FlutterCompositedOpacityLayer and SystemCompositedOpacityLayer so the logic are clearly separated. Also, in case that one class needs more fields, they won't invade the other class. CC @chinmaygarde for his opinion too.

flow/layers/physical_shape_layer.cc

flow/scene_update_context.cc

cbracken · 2019-09-23T18:20:36Z

@arbreng have have you had a chance to review @liyuqian 's feedback?

arbreng · 2019-09-23T18:24:53Z

Yeah, my latest commit should address the majority of it -- I'm working on the last few changes today.

liyuqian

commit 7b3d201 looks good except one question below.

liyuqian · 2019-09-23T19:45:49Z

flow/layers/physical_shape_layer.cc

    paint.setBlendMode(SkBlendMode::kSrc);
-    context.leaf_nodes_canvas->drawRect(paint_bounds(), paint);
-
+    context.internal_nodes_canvas->drawRect(paint_bounds(), paint);


Apologize if I forget something that we discussed before: why is this using internal_nodes_canvas instead of leaf_nodes_canvas?

Amir mentioned that ContainerLayer's render into internal_nodes_canvas when I spoke to him about how composition works. However, that is only used for iOS so this should probably go to leaf_node_canvas.

(didn't review the entire PR so I may be missing some context).
Generally draws should always be done on the leaf_nodes_canvas, the internal_nodes_canvas forwards the operation to all canvases for all composited surfaces.

liyuqian · 2019-09-24T00:30:39Z

I got the following benchmarks on Moto G4, and there doesn't seem to be any regressions compared to our current device lab data. Hence the performance LGTM.

flutter_gallery__transition_perf

  "average_frame_build_time_millis": 5.625441213653615,
  "90th_percentile_frame_build_time_millis": 8.143,
  "99th_percentile_frame_build_time_millis": 48.726,
  "worst_frame_build_time_millis": 126.007,
  "missed_frame_build_budget_count": 59,
  "average_frame_rasterizer_time_millis": 8.768480404551203,
  "90th_percentile_frame_rasterizer_time_millis": 17.009,
  "99th_percentile_frame_rasterizer_time_millis": 35.909,
  "worst_frame_rasterizer_time_millis": 97.285,

complex_layout_scroll_perf__timeline_summary

  "average_frame_build_time_millis": 2.2124489795918354,
  "90th_percentile_frame_build_time_millis": 3.416,
  "99th_percentile_frame_build_time_millis": 22.292,
  "worst_frame_build_time_millis": 25.599,
  "missed_frame_build_budget_count": 5,
  "average_frame_rasterizer_time_millis": 4.849117346938776,
  "90th_percentile_frame_rasterizer_time_millis": 6.372,
  "99th_percentile_frame_rasterizer_time_millis": 8.842,
  "worst_frame_rasterizer_time_millis": 15.342,

liyuqian

LGTM (specifically on performance) except the internal_nodes_canvas vs leaf_nodes_canvas issue. That's probably why this PR currently fails tests.

Move all of this to ContainerLayer

git@github.com:flutter/engine.git/compare/48ad8a08e45f...0bfca37 git log 48ad8a0..0bfca37 --no-merges --oneline 2019-09-25 yjbanov@google.com Force exit felt tool on sigint, sigterm (flutter/engine#12443) 2019-09-25 skia-flutter-autoroll@skia.org Roll src/third_party/skia 6abaff3041a3..559ffe4a23ce (2 commits) (flutter/engine#12444) 2019-09-25 dworsham@google.com [fuchsia] Wire up OpacityLayer to Scenic (flutter/engine#11322) 2019-09-25 yjbanov@google.com delete golden files; switch to flutter/goldens (flutter/engine#12434) 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

This reverts commit fcc4ab3. Reason: flutter/flutter#41394 and other related correctness issues.

@arbreng

This reverts commit fcc4ab3. Fixes flutter/flutter#41394 and other related correctness issues. TBR: @arbreng @jason-simmons @mehmetf

git@github.com:flutter/engine.git/compare/48ad8a08e45f...0bfca37 git log 48ad8a0..0bfca37 --no-merges --oneline 2019-09-25 yjbanov@google.com Force exit felt tool on sigint, sigterm (flutter/engine#12443) 2019-09-25 skia-flutter-autoroll@skia.org Roll src/third_party/skia 6abaff3041a3..559ffe4a23ce (2 commits) (flutter/engine#12444) 2019-09-25 dworsham@google.com [fuchsia] Wire up OpacityLayer to Scenic (flutter/engine#11322) 2019-09-25 yjbanov@google.com delete golden files; switch to flutter/goldens (flutter/engine#12434) 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

arbreng requested a review from liyuqian August 20, 2019 23:17

googlebot added the cla: yes label Aug 20, 2019

arbreng requested review from chinmaygarde and mikejurka August 20, 2019 23:18

arbreng commented Aug 20, 2019

View reviewed changes

iskakaushik self-requested a review August 21, 2019 04:46

liyuqian reviewed Aug 23, 2019

View reviewed changes

flow/layers/opacity_layer.cc Outdated Show resolved Hide resolved

liyuqian reviewed Aug 23, 2019

View reviewed changes

flow/layers/opacity_layer.cc Outdated Show resolved Hide resolved

liyuqian reviewed Aug 23, 2019

View reviewed changes

flow/scene_update_context.h Show resolved Hide resolved