[Fuchsia] Move physical shape layer compositing to Flutter #17005

mikejurka · 2020-03-07T05:57:39Z

I split the change up into smaller CLs. I haven't independently tested the sub-CLs, but just the full stack. No known issues at this time (the one issue I ran into today turned out to be a Scenic/Escher issue).

flow/scene_update_context.cc

mikejurka · 2020-03-08T22:22:09Z

flow/scene_update_context.cc

+  // if there is a OpacityNode higher in the tree with opacity != 1. For now,
+  // clamp to a infinitesimally smaller value than 1, which does not cause visual
+  // problems in practice.
+  opacity_node_.SetOpacity(std::min(1 - FLT_EPSILON, opacity_ / 255.0f));


add Scenic bug for API for marking surfaces as transparent

liyuqian

It would be nice if there are some layer unit tests for these new changes. I believe @gw280 and @arbreng have done similar tests before.

flow/layers/child_scene_layer.cc

flow/layers/layer.h

flow/layers/physical_shape_layer.cc

liyuqian · 2020-03-09T19:44:47Z

flow/layers/physical_shape_layer.h

@@ -37,11 +37,18 @@ class PhysicalShapeLayer : public ContainerLayer {

 #if defined(OS_FUCHSIA)
  void UpdateScene(SceneUpdateContext& context) override;
+
+  void UpdateSceneChildren(SceneUpdateContext& context);


I just realized that UpdateSceneChildren is protected in other layers but public in here. Shall we make them consistent as I don't see why this needs to be public. Also, it might be nice to add some documentation to these function declarations. Although we lack the documentation traditionally (just like tests), we're pushing for better documentations and tests for all new code.

You can avoid overriding it entirely if you follow one of my suggestions above

Removed the separate method and moved inline, which removed a member variable at the cost of a little extra duplication.

flow/scene_update_context.cc

liyuqian · 2020-03-09T19:49:23Z

flow/scene_update_context.cc

@@ -222,6 +222,9 @@ SceneUpdateContext::ExecutePaintTasks(CompositorContext::ScopedFrame& frame) {
    surfaces_to_submit.emplace_back(std::move(task.surface));
  }
  paint_tasks_.clear();
+  alpha_ = 1.f;
+  topmost_global_scenic_elevation_ = 10.f;


I curious where this 10.f comes from, and whether it's configurable.

will pull out to constant

Nit: did you forget to use kScenicZElevationBetweenLayers here? I saw that you've changed other places.

flow/scene_update_context.cc

liyuqian · 2020-03-09T19:54:45Z

flow/scene_update_context.h

+
+  float GetGlobalElevationForNextScenicLayer() {
+    float elevation = topmost_global_scenic_elevation_;
+    topmost_global_scenic_elevation_ += 10.f;


same for the 10.f here.

liyuqian · 2020-03-09T19:56:36Z

shell/platform/fuchsia/flutter/vulkan_surface.h

@@ -134,9 +134,9 @@ class VulkanSurface final
  // |EntityNode| associated with the retained surface instead of using the
  // retained surface directly. Hence Flutter can't modify the surface during
  // retained rendering.
-  const scenic::EntityNode& GetRetainedNode() {
+  scenic::EntityNode* GetRetainedNode() {


The comment of this method says that we want to prevent Flutter from modifying the surface. Is changing const scenic::EntityNode& to scenic::EntityNode* breaking that promise?

Yes. Updating comment.

arbreng

I think this will be good once we do the simplifications and write a screenshot test

flow/layers/layer.h

flow/layers/child_scene_layer.cc

arbreng · 2020-03-10T22:29:00Z

flow/layers/physical_shape_layer.cc

+  // PhysicalShapeLayers that appear above the embedded content will be turned
+  // into their own Scenic layers.
+  if (child_layer_exists_below_) {
+    // Reset paint bounds, because shadows are not rendered in this case


Are you sure about this part? There is no comment (or changes at all for that matter) in Paint()

Actually, after reading forward I'm even more sure. If you follow my other comment below in this function, you can remove this

Look for "// TODO: Re-enable shadow drawing here." in Pain().

I think our offline discussion clarified why we need to do a check for child_layer_exists_below_; keeping it. Let me know if that wasn't the case.

flow/layers/physical_shape_layer.cc

arbreng · 2020-03-10T22:38:03Z

flow/layers/physical_shape_layer.h

 #endif  // defined(OS_FUCHSIA)

  float total_elevation() const { return total_elevation_; }

 private:
+#if defined(OS_FUCHSIA)
+  float local_scenic_elevation_ = 0.0f;


#2 and #3 in this list should be unnecesary

see comments above for child_layer_exists_below_

As far as children_need_system_compositing_ -- I think you are right, we can just get away with always calling UpdateSceneChildren in PhysicalShapeLayer::UpdateScene. I initially thought I needed it for correctness, but I think at this point it's just a tiny optimization. Getting rid of it will simplify things.

got rid of #1 and #3 as per our discussion.

arbreng · 2020-03-10T22:42:46Z

flow/layers/physical_shape_layer.cc

+
+    // Node: UpdateSceneChildren needs to be called here so that |frame| is still
+    // in scope (and therefore alive) while UpdateSceneChildren is being called.
+    if (children_need_system_compositing_) {


You shouldn't need this if, UpdateSceneChildren will check internally whether each child needs system compositing or not and skip over it if it doesnt

+1 I concur (see comment above)

arbreng · 2020-03-10T22:45:05Z

flow/layers/physical_shape_layer.cc

+
+void PhysicalShapeLayer::UpdateSceneChildren(SceneUpdateContext& context) {
+  float scenic_elevation = context.scenic_elevation();
+  context.set_scenic_elevation(scenic_elevation + local_scenic_elevation_);


If you do this line in PhysicalShapeLayer::UpdateScene instead, you won't need to store local_scenic_elevation_ as a member variable.

i moved this because UpdateSceneChildren is called in two different places and this removed some duplicated code. But I think I can put the elevation logic at the very top and very bottom of UpdateScene which would eliminate the duplication and need for a variable, at the cost of a slight reduction in understandability.

Removed local_scenic_elevation_.

arbreng · 2020-03-10T22:45:32Z

flow/layers/physical_shape_layer.h

@@ -37,11 +37,18 @@ class PhysicalShapeLayer : public ContainerLayer {

 #if defined(OS_FUCHSIA)
  void UpdateScene(SceneUpdateContext& context) override;
+
+  void UpdateSceneChildren(SceneUpdateContext& context);


You can avoid overriding it entirely if you follow one of my suggestions above

arbreng · 2020-03-10T22:45:59Z

flow/layers/physical_shape_layer.h

 #endif  // defined(OS_FUCHSIA)

  float total_elevation() const { return total_elevation_; }

 private:
+#if defined(OS_FUCHSIA)
+  float local_scenic_elevation_ = 0.0f;


Actually, #1 is also unnecesary if following one of my suggestions above

flow/layers/transform_layer.cc

mikejurka · 2020-03-15T23:40:46Z

Ok, added tests (see latest CL), and I also addressed comments in the last CL as well. Hope that's ok, I haven't found a productive workflow for going back and modifying the previous CLs.

See 9c629e4#diff-377431c448d255a96bd1943992182b3d and click "Load Diff" for the tests (not rendered by default since it's too big; file is fuchsia_layer_unittests.cc)

liyuqian · 2020-03-17T21:57:02Z

Thank you @mikejurka for adding the test! I wonder why all the presubmit tests are failing. Maybe you need to rebase against newest engine?

I also couldn't find my comment regarding flutter/flutter#18320 although that issue showed that I referenced it from this PR...

Anyway, it mostly looks good to me, and the only thing obvious to me is that you forget one more 10.f :)

liyuqian

Discussed with Mike offline about flutter/flutter#18320. This PR fixes that issue for PhysicalShapeLayer, but we'll still have to fix that issue for other layers such as PerformanceOverlayerLayer.

liyuqian · 2020-03-17T21:38:58Z

flow/scene_update_context.cc

@@ -222,6 +222,9 @@ SceneUpdateContext::ExecutePaintTasks(CompositorContext::ScopedFrame& frame) {
    surfaces_to_submit.emplace_back(std::move(task.surface));
  }
  paint_tasks_.clear();
+  alpha_ = 1.f;
+  topmost_global_scenic_elevation_ = 10.f;


Nit: did you forget to use kScenicZElevationBetweenLayers here? I saw that you've changed other places.

mikejurka · 2020-03-18T00:43:52Z

Replaced the stray 10.f with kScenicZElevationBetweenLayers

…need to float above child views. In that case, they will still need to be pulled into separate Scenic nodes to be composited on top of the child view[s].

Inspect commands going to Scenic and make sure they match what is expected. Also, restructure code to need less member variables, and other cleanups based on review feedback.

…lutter/engine#17005)

mikejurka requested review from iskakaushik, liyuqian and arbreng March 7, 2020 05:57

auto-assign bot requested a review from gaaclarke March 7, 2020 05:57

googlebot added the cla: yes label Mar 7, 2020

mikejurka mentioned this pull request Mar 7, 2020

[fuchsia] Avoid creating separate system layers for PhysicalShapeLayers. #16975

Closed

mikejurka requested review from gw280, gaaclarke and mklim and removed request for gaaclarke March 7, 2020 06:09

mikejurka assigned mikejurka and unassigned mikejurka Mar 8, 2020

mikejurka commented Mar 8, 2020

View reviewed changes

liyuqian reviewed Mar 9, 2020

View reviewed changes

arbreng reviewed Mar 10, 2020

View reviewed changes

mikejurka force-pushed the feb19_fuchsia_physical_shape_layer_compositing branch from 9c629e4 to 11f9263 Compare March 16, 2020 00:12

mikejurka requested review from liyuqian and arbreng March 16, 2020 00:13

mikejurka force-pushed the feb19_fuchsia_physical_shape_layer_compositing branch from 11f9263 to 0e03239 Compare March 16, 2020 00:42

arbreng approved these changes Mar 16, 2020

View reviewed changes

liyuqian approved these changes Mar 17, 2020

View reviewed changes

mikejurka force-pushed the feb19_fuchsia_physical_shape_layer_compositing branch 2 times, most recently from b1383a9 to 0792c63 Compare March 18, 2020 00:43

mikejurka force-pushed the feb19_fuchsia_physical_shape_layer_compositing branch 2 times, most recently from 6f14029 to 80f08eb Compare March 18, 2020 01:00

mikejurka added 2 commits March 18, 2020 11:24

[fuchsia] Add labels to Scenic nodes.

cc134bd

[fuchsia] Skip creating Scenic nodes for identity Transforms.

2dc66a3

mikejurka added 3 commits March 18, 2020 11:24

[fuchsia] Assign elevation to Scenic nodes based on paint order.

5b05181

[fuchsia] Create Scenic OpacityNodes at leaf nodes.

af31acd

[fuchsia] Composite PhysicalShapeLayers using Skia, except when they …

b2921e9

…need to float above child views. In that case, they will still need to be pulled into separate Scenic nodes to be composited on top of the child view[s].

mikejurka force-pushed the feb19_fuchsia_physical_shape_layer_compositing branch from 80f08eb to 8b4aedc Compare March 18, 2020 18:24

[fuchsia] Add tests for Fuchsia-specific layer behavior.

4c1b195

Inspect commands going to Scenic and make sure they match what is expected. Also, restructure code to need less member variables, and other cleanups based on review feedback.

mikejurka force-pushed the feb19_fuchsia_physical_shape_layer_compositing branch from 8b4aedc to 4c1b195 Compare March 18, 2020 18:58

mikejurka merged commit ad8ccf4 into flutter:master Mar 23, 2020

mikejurka deleted the feb19_fuchsia_physical_shape_layer_compositing branch March 23, 2020 23:38

engine-flutter-autoroll mentioned this pull request Mar 24, 2020

Roll engine 2d42c74a348d..d7ad50c96f05 (54 commits) flutter/flutter#53178

Closed

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

ad8ccf4 [Fuchsia] Move physical shape layer compositing to Flutter (f…

1f41a50

…lutter/engine#17005)

engine-flutter-autoroll mentioned this pull request Mar 24, 2020

Roll engine 2d42c74a348d..3a68f689996f (56 commits) flutter/flutter#53196

Closed

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

ad8ccf4 [Fuchsia] Move physical shape layer compositing to Flutter (f…

8a3519d

…lutter/engine#17005)

engine-flutter-autoroll mentioned this pull request Mar 25, 2020

Roll engine 2d42c74a348d..3a68f689996f (56 commits) flutter/flutter#53210

Merged

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

ad8ccf4 [Fuchsia] Move physical shape layer compositing to Flutter (f…

de08514

…lutter/engine#17005)

[Fuchsia] Move physical shape layer compositing to Flutter #17005

[Fuchsia] Move physical shape layer compositing to Flutter #17005

Conversation

mikejurka commented Mar 7, 2020

Choose a reason for hiding this comment

liyuqian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arbreng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikejurka commented Mar 15, 2020 • edited Loading

liyuqian commented Mar 17, 2020

liyuqian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikejurka commented Mar 18, 2020

mikejurka commented Mar 15, 2020 •

edited

Loading