Account for root surface transformation on the surfaces managed by the external view embedder. #11384

chinmaygarde · 2019-08-22T20:14:01Z

The earlier design speculated that embedders could affect the same
transformations on the layers post engine compositor presentation but before
final composition.

However, the linked issue points out that this design is not suitable for use
with hardware overlay planes. When rendering to the same, to affect the
transformation before composition, embedders would have to render to an
off-screen render target and then apply the transformation before presentation.
This patch negates the need for that off-screen render pass.

To be clear, the previous architecture is still fully viable. Embedders still
have full control over layer transformations before composition. This is an
optimization for the hardware overlay planes use-case.

Fixes b/139758641

chinmaygarde · 2019-08-22T20:15:25Z

cc @dnicoara

dnicoara · 2019-08-22T22:42:40Z

A couple of observations:

It looks like the FlutterCompositor APIs are now using the display coordinate system, rather than the surface coordinate system. Could this be documented in the API?
It looks like I also had to flip the image over x-axis. I'm not sure what caused this behavior. Any clues?

chinmaygarde · 2019-08-22T22:48:50Z

It looks like the FlutterCompositor APIs are now using the display coordinate system, rather than the surface coordinate system. Could this be documented in the API?

Sure. I will document this. But does this match your expectations? I wasn't sure if the least unexpected behavior for embedders would be for them to be in control of applying this transformation themselves.

It looks like I also had to flip the image over x-axis. I'm not sure what caused this behavior. Any clues?

The transformation is applied to the offset and the bounds of the layer. It is still embedder responsibility to render the transformed contents about the new offset. In fact, this is why the test is currently failing. My test compositor doesn't yet account for the transformation for the contents rendered by the embedder.

chinmaygarde · 2019-08-22T23:01:57Z

It looks like I also had to flip the image over x-axis. I'm not sure what caused this behavior. Any clues?

Ok, I am convinced, this is bogus behavior in this patch. I am fixing it so you wont have to apply the offset about the x-axis. You will still have to apply to the transformation to the contents though.

chinmaygarde · 2019-08-23T00:17:59Z

Fixed the math. You no longer have to apply the flip over the x-axis unnecessarily. Also documented the behavior that setting the root surface transformation will affect layer offsets and sizes in the present callback. PTAL.

amirh

LGTM modulo nits

amirh · 2019-08-23T18:03:39Z

shell/platform/embedder/embedder_external_view_embedder.cc

+  if (!surface_transformation_callback_) {
+    return SkMatrix{};
+  }
+


To make sure we apply the same transformation to the frame size and contents, I'd suggest make GetSurfaceTransformation return identity if !surface_transformation_callback_().rectStaysRect(). (since if that condition is false we're not applying a size transformation)

Disagree. We use the surface transformation to not only affect the size, but also the offset (of the root surface too). We will lose this information if we return identity here.

amirh · 2019-08-23T18:04:10Z

shell/platform/embedder/embedder_external_view_embedder.cc

+    // TODO(chinmaygarde): This has the opportunity to become super spammy. Move
+    // this to the begin frame call so that embedders get just one warning per
+    // frame.
+    FML_LOG(ERROR) << "Embedder supplied surface transformation does not map a "


if we apply the suggestion above this can be an assertion

On second thought, I think this block is incorrect. We can still return the bounding box of the transform box even when rect-ness is not maintained. Embedders can still apply transformations on their own.

amirh · 2019-08-23T18:05:08Z

shell/platform/embedder/embedder_external_view_embedder.cc

  FlutterLayer layer = {};

  layer.struct_size = sizeof(layer);
  layer.type = kFlutterLayerContentTypePlatformView;
  layer.platform_view = &platform_view;

-  layer.offset.x = params.offsetPixels.x();
-  layer.offset.y = params.offsetPixels.y();
+  const auto layer_bounds = SkRect::MakeXYWH(params.offsetPixels.x(),    //


nit: remove empty comment lines

I am actually using those to format the args in a column :/

amirh · 2019-08-23T19:46:40Z

shell/platform/embedder/tests/embedder_a11y_unittests.cc

@@ -15,6 +15,7 @@
 #include "flutter/shell/platform/embedder/embedder.h"
 #include "flutter/shell/platform/embedder/tests/embedder_config_builder.h"
 #include "flutter/testing/testing.h"
+#include "third_party/skia/include/core/SkMatrix.h"


why is this needed here?

Not necessary. Removed.

chinmaygarde · 2019-08-23T22:00:56Z

More unit-tests have been added because @dnicoara was mentioning that root surface transformations may not be consistent between the compositor and non-compositor use cases. They were consistent but now we have unit-tests to verify this.

chinmaygarde · 2019-09-10T01:42:30Z

This is good to go. The coordinate space for all OpenGL backing stores is bottom left origin while that for Flutter uses top-left origin. This is consistent with the previous behavior of treating the root OpenGL surface as having a bottom left origin.

dnicoara · 2019-09-10T15:46:27Z

Thanks, it did work as expected for the 270 degree clockwise rotation. Would it be possible to add tests for other rotations (90, 180)? For example, when I used a 90 degree rotation I didn't get the content upside-down, it was still right side-up (though it's possible I messed up and forgot to update something).

…e external view embedder. The earlier design speculated that embedders could affect the same transformations on the layers post engine compositor presentation but before final composition. However, the linked issue points out that this design is not suitable for use with hardware overlay planes. When rendering to the same, to affect the transformation before composition, embedders would have to render to an off-screen render target and then apply the transformation before presentation. This patch negates the need for that off-screen render pass. To be clear, the previous architecture is still fully viable. Embedders still have full control over layer transformations before composition. This is an optimization for the hardware overlay planes use-case. Fixes b/139758641

chinmaygarde · 2019-09-17T22:17:57Z

Fixes flutter/flutter#39339

…ed by the external view embedder. (flutter/engine#11384)

git@github.com:flutter/engine.git/compare/d1692d4cc703...6a96417 git log d1692d4..6a96417 --no-merges --oneline 2019-09-17 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from RRgw-... to F-g18... (flutter/engine#12326) 2019-09-17 chinmaygarde@google.com Account for root surface transformation on the surfaces managed by the external view embedder. (flutter/engine#11384) 2019-09-17 matthew-carroll@users.noreply.github.com Introduce FlutterFragmentActivity (flutter/engine#12305) 2019-09-17 chinmaygarde@google.com Shuffle test order and repeat test runs once. (flutter/engine#12275) 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 stuartmorgan@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

chinmaygarde requested review from cbracken, amirh and jason-simmons August 22, 2019 20:14

googlebot added the cla: yes label Aug 22, 2019

amirh approved these changes Aug 23, 2019

View reviewed changes

chinmaygarde force-pushed the embedder_rotation branch from c015c41 to a03db9d Compare August 23, 2019 21:50

chinmaygarde force-pushed the embedder_rotation branch 6 times, most recently from 6a18798 to 082abef Compare August 28, 2019 21:37

chinmaygarde force-pushed the embedder_rotation branch 2 times, most recently from c4af6c7 to 59819e0 Compare September 10, 2019 01:40

chinmaygarde force-pushed the embedder_rotation branch from 59819e0 to 2575be4 Compare September 13, 2019 19:00

chinmaygarde force-pushed the embedder_rotation branch from 2575be4 to 1afb79f Compare September 17, 2019 21:51

chinmaygarde merged commit 1c7300e into flutter:master Sep 17, 2019

chinmaygarde deleted the embedder_rotation branch September 17, 2019 22:17

chinmaygarde mentioned this pull request Sep 17, 2019

FlutterCompositor doesn't use the FlutterTransformation provided by the FlutterRendererConfig flutter/flutter#39339

Closed

engine-flutter-autoroll mentioned this pull request Sep 18, 2019

Roll engine d1692d4cc703..6a96417416b4 (4 commits) flutter/flutter#40721

Merged

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

1c7300e Account for root surface transformation on the surfaces manag…

80e1825

…ed by the external view embedder. (flutter/engine#11384)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account for root surface transformation on the surfaces managed by the external view embedder. #11384

Account for root surface transformation on the surfaces managed by the external view embedder. #11384

chinmaygarde commented Aug 22, 2019

chinmaygarde commented Aug 22, 2019

dnicoara commented Aug 22, 2019

chinmaygarde commented Aug 22, 2019 •

edited

Loading

chinmaygarde commented Aug 22, 2019

chinmaygarde commented Aug 23, 2019

amirh left a comment

amirh Aug 23, 2019

chinmaygarde Aug 23, 2019

amirh Aug 23, 2019

chinmaygarde Aug 23, 2019

chinmaygarde Aug 23, 2019

amirh Aug 23, 2019

chinmaygarde Aug 23, 2019

amirh Aug 23, 2019

chinmaygarde Aug 23, 2019

chinmaygarde commented Aug 23, 2019

chinmaygarde commented Sep 10, 2019

dnicoara commented Sep 10, 2019

chinmaygarde commented Sep 17, 2019

Account for root surface transformation on the surfaces managed by the external view embedder. #11384

Account for root surface transformation on the surfaces managed by the external view embedder. #11384

Conversation

chinmaygarde commented Aug 22, 2019

chinmaygarde commented Aug 22, 2019

dnicoara commented Aug 22, 2019

chinmaygarde commented Aug 22, 2019 • edited Loading

chinmaygarde commented Aug 22, 2019

chinmaygarde commented Aug 23, 2019

amirh 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

chinmaygarde commented Aug 23, 2019

chinmaygarde commented Sep 10, 2019

dnicoara commented Sep 10, 2019

chinmaygarde commented Sep 17, 2019

chinmaygarde commented Aug 22, 2019 •

edited

Loading