Allow embedder controlled composition of Flutter layers. #10195

chinmaygarde · 2019-07-27T02:49:33Z

This patch allows embedders to split the Flutter layer tree into multiple chunks. These chunks are meant to be composed one on top of another. This gives embedders a chance to interleave their own contents between these chunks.

The Flutter embedder API already provides hooks for the specification of textures for the Flutter engine to compose within its own hierarchy (for camera feeds, video, etc..). However, not all embedders can render the contents of such sources into textures the Flutter engine can accept. Moreover, this composition model may have overheads that are non-trivial for certain use cases. In such cases, the embedder may choose to specify multiple render target for Flutter to render into instead of just one.

The use of this API allows embedders to perform composition very similar to the iOS embedder. This composition model is used on that platform for the embedding of UIKit view such and web view and map views within the Flutter hierarchy. However, do note that iOS also has threading configurations that are currently not available to custom embedders.

The embedder API updates in this patch are ABI stable and existing embedders will continue to work are normal. For embedders that want to enable this composition mode, the API is designed to make it easy to opt into the same in an incremental manner.

Rendering of contents into the “root” rendering surface remains unchanged. However, now the application can push “platform views” via a scene builder. These platform views need to handled by a FlutterCompositor specified in a new field at the end of the FlutterProjectArgs struct.

When a new platform view in introduced within the layer tree, the compositor will ask the embedder to create a new render target for that platform view. Render targets can currently be OpenGL framebuffers, OpenGL textures or software buffers. The type of the render target returned by the embedder must be compatible with the root render surface. That is, if the root render surface is an OpenGL framebuffer, the render target for each platform view must either be a texture or a framebuffer in the same OpenGL context. New render target types as well as root renderers for newer APIs like Metal & Vulkan can and will be added in the future. The addition of these APIs will be done in an ABI & API stable manner.

As Flutter renders frames, it gives the embedder a callback with information about the position of the various platform views in the effective hierarchy. The embedder is then meant to put the contents of the render targets that it setup and had previously given to the engine onto the screen (of course, interleaving the contents of the platform views).

Unit-tests have been added that test not only the structure and properties of layer hierarchy given to the compositor, but also the contents of the texels rendered by a test compositor using both the OpenGL and software rendering backends.

Fixes b/132812775
Fixes flutter/flutter#35410

stuartmorgan

I did a quick initial pass (although didn't to the test code). Is there a document somewhere that provides a high-level overview of how this all fits together? Building up and holding the whole state in my head while reviewing (especially without comments on many of the classes explaining their role in the flow) is quite difficult without already having a lot of context about how the entire rendering pipeline works.

stuartmorgan · 2019-07-29T15:49:27Z

fml/closure.h

 namespace fml {

 using closure = std::function<void()>;

+class AutoFireClosure {


This needs a class-level comment, per style guide.

stuartmorgan · 2019-07-29T15:50:40Z

fml/closure.h

+ private:
+  fml::closure closure_;
+
+  FML_DISALLOW_COPY_AND_ASSIGN(AutoFireClosure);


Since macros like this are now strongly discouraged by the style guide, it would be better to use delete directly in all new code.

Can you link to this? I cannot find or rationalize the guidance w.r.t the macros in the style guide. The guidance "... move-only class should explicitly declare the move operations, and a non-copyable/movable class should explicitly delete the copy operations. Explicitly declaring or deleting all four copy/move operations is permitted, but not required." is adequately satisfied by the macros.

https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros
"Do not use macros to define pieces of a C++ API."
I misremembered; they are forbidden now, not just discouraged.

See also:
https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types
"a non-copyable/movable class should explicitly delete the copy operations"

I can also point you to internal discussion of exactly this macro construction if you'd like.

Yes. Please. I'll also get rid of the macro and all usages in a separate patch. It is just best to stick to the guide.

stuartmorgan · 2019-07-29T15:51:42Z

shell/gpu/gpu_surface_software_delegate.h

+
+namespace flutter {
+
+class GPUSurfaceSoftwareDelegate {


Comment class and methods.

stuartmorgan · 2019-07-29T15:53:52Z

shell/platform/embedder/embedder.cc

-  };
+  return fml::MakeCopyable(
+      [gl_dispatch_table, fbo_reset_after_present, platform_dispatch_table,
+       external_view_embedder =


It seems like this would be clearer if you used a new variable name instead of shadowing. (Same comment in the similar block below.)

There is no variable shadowing in this case because there is no implicit capture of the external view embedder. Also, I think -Wshadow which is on by default would have caught this form of shadowing.

I was wrong #10480

stuartmorgan · 2019-07-29T15:56:23Z

shell/platform/embedder/embedder.cc

+  return surface;
+}
+
+static sk_sp<SkSurface> WrapPlatformRenderTarget(


These helpers are all non-trivial, and need declaration-style comments explaining what they do.

stuartmorgan · 2019-07-29T16:05:05Z

shell/platform/embedder/embedder.h

@@ -685,6 +815,8 @@ typedef struct {
  // optional argument allows for the specification of task runner interfaces to
  // event loops managed by the embedder on threads it creates.
  const FlutterCustomTaskRunners* custom_task_runners;
+
+  const FlutterCompositor* compositor;


stuartmorgan · 2019-07-29T16:07:35Z

shell/platform/embedder/embedder.h

+typedef struct {
+  double x;
+  double y;
+} FlutterPoint;


The inconsistency with existing APIs taking points and sizes is unfortunate. Have you thought about maybe doing a follow-up to add updated versions of the relevant APIs (window metrics, pointer)?

I am not too excited about making that breaking API change (though the ABI would be stable). Maybe I should just place these inline in the struct instead of adding these additional structs for consistency?

I was actually thinking you could freeze the old APIs, comment them as deprecated, and make new ones that used the new structs.

We can do that too. Though, I don't think this patch needs to include that change. But if we agree that this is the direction we want to go, I'll keep these structs around.

stuartmorgan · 2019-07-29T16:09:28Z

shell/platform/embedder/embedder_external_view_embedder.h

+
+namespace flutter {
+
+class EmbedderExternalViewEmbedder : public ExternalViewEmbedder {


stuartmorgan · 2019-07-29T16:10:21Z

shell/platform/embedder/embedder_render_target.h

+
+namespace flutter {
+
+class EmbedderRenderTarget final {


Comment class and methods.

stuartmorgan · 2019-07-29T16:10:52Z

shell/platform/embedder/embedder_render_target_registry.h

+
+namespace flutter {
+
+class EmbedderRenderTargetRegistry {


amirh

Wow awesome super fast progress!

Took a first crack, left a few comments, mainly I'm concerned about baking an assumption that each platform view has an associated overlay into the public API.

Side note - this PR is pretty big, it may be more productive to review in smaller chunks (I'm ok to continue with current PR if this is still your preference).

amirh · 2019-07-29T17:09:57Z

shell/platform/embedder/embedder.h

+  FlutterCompositorLayerRendererCollectCallback collect_layer_renderer_callback;
+  // Callback invoked by the engine to composite the contents of each platform
+  // view into the specified render target.
+  FlutterCompositorLayersPresentCallback present_layers_callback;


The comment should mention that when this is used present won't be called for the root FlutterOpenGLRendererConfig/FlutterSoftwareRendererConfig and that the implementation should present the root surface as well.

Or at least, wasn't that the plan?

amirh · 2019-07-29T17:11:42Z

shell/platform/embedder/embedder.h

+  // their resources in the user_data field of that struct.
+  FlutterCompositorLayerRendererCollectCallback collect_layer_renderer_callback;
+  // Callback invoked by the engine to composite the contents of each platform
+  // view into the specified render target.


nit: I'm not sure I understand what's the specified render target into which all the platform views (layers?) are rendered?

amirh · 2019-07-29T17:17:13Z

shell/platform/embedder/embedder.h

+    void* user_data);
+
+typedef bool (*FlutterCompositorLayersPresentCallback)(
+    const FlutterCompositorLayer** layers,


This bakes in an assumption that each platform view has an overlay surface, which is likely to not be true in the future.

What do you think about passing in a single list of layers, and have each layer refer to either a platform view or to an overlay?

amirh · 2019-07-29T17:44:02Z

shell/common/rasterizer.cc

    if (external_view_embedder != nullptr) {
      external_view_embedder->SubmitFrame(surface_->GetContext());
    }
+    frame->Submit();


This submit will now happen after the CATransaction is committed on iOS (it's currently committed in the external view embedder SubmitFrame)

Yeah, this is still to be fixed. Won't commit before addressing the same.

chinmaygarde

Took a stab at reworking the API to think of layers having backing stores instead of introducing a layer at an interleaving level. Still need to work though (most) all comments made so far.

chinmaygarde · 2019-07-29T22:30:04Z

fml/closure.h

 namespace fml {

 using closure = std::function<void()>;

+class AutoFireClosure {


chinmaygarde · 2019-07-29T23:33:09Z

shell/platform/embedder/embedder.h

@@ -193,6 +193,15 @@ typedef struct {

 typedef void (*VoidCallback)(void* /* user data */);

+typedef enum {
+  //    Specifies an OpenGL texture target type. Textures are specified using


I though so too. But that was the pattern followed in this file already. See FlutterTransformation above.

chinmaygarde · 2019-07-30T23:49:31Z

The API has been updated. Instead of thinking of the layers as being introduced at interleaving levels, a new FlutterLayer is now represented by either a backing store that Flutter engine renders into, or, a platform view that the embedder gets to composite. This also addresses the extensibility concerns expressed in the earlier iteration of the patch. Unit-tests have been updated.

chinmaygarde · 2019-08-01T00:28:26Z

The updated API has the implementation, tests and documentation. PTAL.

amirh

reviewed everything but tests

amirh · 2019-08-02T16:36:48Z

shell/platform/embedder/embedder.h

+    // the framebuffer is complete.
+    FlutterOpenGLFramebuffer framebuffer;
+  };
+} FlutterOpenGLBackingStore;


I'm wondering whether we need these 2 levels of hierarchy (software/gl, texture/fbo) vs. flattening the hierarchy (which will reduce some boilerplate in this file)

Seems like flattening the hierarchy will save the nested switch-case in CreateBackingStore

Hmm, I see your point. But I am on the fence on this one. Since the addition of render targets for Vulkan & Metal will increase the backing store types, I'd rather just filter away entire target type classes in once shot in the future instead of adding client rendering API/target type combinations to this single enum.

amirh · 2019-08-02T17:06:23Z

shell/platform/embedder/embedder.h

+  // call. When this happens, the Flutter rasterizer divides the effective view
+  // hierarchy into multiple layers. Each layer gets its own backing store and
+  // Flutter renders into the same. Once the layers contents have been
+  // fulfilled, the embedder is asked to compositor these layers on-screen. At


nit: to composite

amirh · 2019-08-02T17:08:31Z

shell/platform/embedder/embedder.h

+  };
+  // The offset of this layer relative to the top left of the root surface used
+  // by the engine.
+  FlutterPoint offset;


we should say what's the unit for the offset and the size

amirh · 2019-08-02T17:29:50Z

fml/closure.h

+///             automatically at the end of the scope. This covers the cases
+///             where there are early returns as well.
+///
+class AutoFireClosure {


First running into this name I didn't know when to expect that closure will be fired, and I didn't guess it will fire on destruction. Maybe we can come up with a name that better conveys what this thing is doing?
InvokeOnDestruction?

Coming back here after reviewing embedder.cc (which I believe is the only user of this), I'd reconsider whether the overhead of introducing this class is justified (see my comment on embedder.cc)

I already introduced a resource leak on error as I was refactoring the initial version of the implementation. Since it is so easy to introduce such resource leaks in C and C++ interop code (in error cases no less), I'd rather make this a pattern we follow.

About the name, maybe ScopedFireClosure is more immediately descriptive?

I like Scoped, since Scoped* is a common naming pattern. What about ScopedCleanupClosure, which more clearly indicates that it fires on destruction?

ScopedCleanupClosure sgtm

amirh · 2019-08-02T17:51:52Z

fml/mapping.cc

+
+NonOwnedMapping::~NonOwnedMapping() {
+  if (release_proc_) {
+    release_proc_(data_, size_);


Doesn't it make it an owned mapping once we have a release proc?

I guess so. The argument could also be made that this object does not own the mapping but gives the closure to collect it (so whoever passes in the closure to this owns the mapping).

amirh · 2019-08-02T18:53:13Z

shell/platform/android/android_surface_gl.cc

@@ -125,4 +125,9 @@ intptr_t AndroidSurfaceGL::GLContextFBO() const {
  return 0;
 }

+// |GPUSurfaceGLDelegate|
+ExternalViewEmbedder* AndroidSurfaceGL::GetExternalViewEmbedder() {


Why not keep this default implementation in GPUSurfaceGLDelegate?

Since not all platforms implement platform views like iOS and embedders do, it was not immediately clear from looking at just one implementation if the technique was used on that platform (Android in this case). I believe this explicitness makes it more clear from looking at the Android implementation. Also, this happened to be the only function in the delegate that had a default implementation in the base class (the acquire and present calls on the backing store were pure virtual).

amirh · 2019-08-02T18:53:42Z

shell/platform/android/android_surface_software.cc

@@ -134,6 +134,11 @@ bool AndroidSurfaceSoftware::PresentBackingStore(
  return true;
 }

+// |GPUSurfaceSoftwareDelegate|
+ExternalViewEmbedder* AndroidSurfaceSoftware::GetExternalViewEmbedder() {


Why not keep this in GPUSurfaceSoftwareDelegate?

Similar reasoning as above.

amirh · 2019-08-02T18:57:39Z

shell/platform/embedder/embedder_external_view_embedder.h

+
+    struct Hash {
+      constexpr std::size_t operator()(RegistryKey const& key) const {
+        return key.view_identifier;


feels a bit dodgy to have the same hash for 2 values that are unequal (have different sizes)

A hash function maps values from an arbitrarily large set into a finite set. Per that definition, how is this dodgy. The equality is implemented separately below as well.

amirh · 2019-08-02T19:00:47Z

shell/platform/embedder/embedder_external_view_embedder.cc

+
+// |ExternalViewEmbedder|
+bool EmbedderExternalViewEmbedder::HasPendingViewOperations() {
+  return false;


This is semantically incorrect (e.g we may have "pending view operations"), I'd leave a comment explaining why we return false.

amirh · 2019-08-02T19:06:54Z

shell/platform/embedder/embedder_external_view_embedder.cc

+    render_canvas->flush();
+
+    // Indicate a layer for the platform view.
+    presented_platform_views[view_id] = MakePlatformView(view_id);


What is presented_platform_views used for?

presented_layers contain pointers to objects in presented_platform_views (see the comment on line 144). These structs must be alive till the present_callback_ is invoked.

wow I really missed that, do you mind clarifying on the presented_layers that it's sole purpose is to release the layers created below when we quit the method?

That is why I added the comment on line 165. Were you referring to something else?

I guess that comment wasn't explicit enough for me 😄
I'd suggest adding a comment here, saying that we add to presented_platform_views in order to keep at allocated just for the scope of the current method.

chinmaygarde · 2019-08-02T22:14:01Z

Did another pass over new comments. PTAL. Only the move of the Submit on the rasterizer remains unaddressed.

chinmaygarde · 2019-08-05T19:20:28Z

@amirh: When you do your next review pass, please pay close attention to the offset of the Fluter backing store backed layers in the unit-tests. I am not sure the current behavior is as intuitive as it could be.

chinmaygarde · 2019-08-05T23:55:55Z

All comments and API suggestions and been incorporated. I believe this is good to go.

stuartmorgan · 2019-08-06T18:56:05Z

I'll do another review pass by EOD, sorry for the delay.

stuartmorgan

Left a few more minor comments, but it's definitely easier to follow with comments. I still think it would be extremely helpful to have a doc/wiki page with a high-level overview of this part of the system though.

I'll leave more substantive review to others who have more familiarity with the details of the rendering pipeline.

stuartmorgan · 2019-08-07T20:05:06Z

fml/closure.h

+///             automatically at the end of the scope. This covers the cases
+///             where there are early returns as well.
+///
+class AutoFireClosure {


I like Scoped, since Scoped* is a common naming pattern. What about ScopedCleanupClosure, which more clearly indicates that it fires on destruction?

stuartmorgan · 2019-08-07T20:11:16Z

shell/platform/embedder/embedder.h

+  };
+} FlutterOpenGLBackingStore;
+
+typedef struct {


Should this have a size field, or are you 100% confident that it won't grow over time?

I was trying to make this consistent with the FlutterOpenGLTexture and FlutterOpenGLFramebuffer structs. No, I am not 100% confident.

stuartmorgan · 2019-08-07T20:17:09Z

shell/platform/embedder/embedder_external_view_embedder.cc

+  // Decide if we want to discard the previous root render target.
+  if (root_render_target_) {
+    auto surface = root_render_target_->GetRenderSurface();
+    // This is unlikely to happen but the embedder could have given us a render


Please be more specific about 'us', 'we', etc. (go/avoidwe)

amirh

Left a few nits. Overall looks good.
Per offline discussion lets make sure we don't invoke both the fbo callback and create backing store for the root surface.

amirh · 2019-08-08T21:16:19Z

shell/platform/embedder/embedder_external_view_embedder.cc

+    render_canvas->flush();
+
+    // Indicate a layer for the platform view.
+    presented_platform_views[view_id] = MakePlatformView(view_id);


wow I really missed that, do you mind clarifying on the presented_layers that it's sole purpose is to release the layers created below when we quit the method?

amirh · 2019-08-08T21:28:14Z

flow/embedded_views.h

@@ -191,6 +191,10 @@ class ExternalViewEmbedder {
 public:
  ExternalViewEmbedder() = default;

+  virtual ~ExternalViewEmbedder() = default;
+
+  virtual sk_sp<SkSurface> GetRootSurface();


Let's document that it's open and explain the behaviors when it's null and non null.

Also - what do you think about leaving a TODO to implement this for iOS as well and then treat it as non-optional.

Good idea. Done. Also documented the protocol and the iOS implementation that returns nullptr.

amirh · 2019-08-08T21:56:28Z

shell/common/rasterizer.cc

  }

+  // If the external view embedder has specified an optional root surface, the
+  // root surface transformation can be affected by the embedder instead of


nit: s/can be affected/is set/

amirh · 2019-08-08T21:58:03Z

shell/common/rasterizer.cc

  auto compositor_frame = compositor_context_->AcquireFrame(
-      surface_->GetContext(), canvas, external_view_embedder,
-      surface_->GetRootTransformation(), true);
+      surface_->GetContext(),  // skia GrContext


something is off with the auto formatter, I recall being surprised by the reformatting here when @cyanglaz applied it. It seems like CI accepts both formats. @cyanglaz what did you use for formatting locally?

I think it is the formatter trying to "columnize" the argument and their docstrings but the embedder_root_surface specification causing a wrap. Agreed that it was looking goofy however. I moved that into its own variable and now it is neatly arranged.

Sorry, just saw this comment. I used /engine/src/flutter/ci/format.sh for formatting if you still want to know.

amirh · 2019-08-08T22:05:35Z

testing/assertions_skia.h

+namespace flutter {
+namespace testing {
+
+//


Do we need this?

No. It was for assertions that were later moved to embedder_assertions.h. Removed.

This patch allows embedders to split the Flutter layer tree into multiple chunks. These chunks are meant to be composed one on top of another. This gives embedders a chance to interleave their own contents between these chunks. The Flutter embedder API already provides hooks for the specification of textures for the Flutter engine to compose within its own hierarchy (for camera feeds, video, etc..). However, not all embedders can render the contents of such sources into textures the Flutter engine can accept. Moreover, this composition model may have overheads that are non-trivial for certain use cases. In such cases, the embedder may choose to specify multiple render target for Flutter to render into instead of just one. The use of this API allows embedders to perform composition very similar to the iOS embedder. This composition model is used on that platform for the embedding of UIKit view such and web view and map views within the Flutter hierarchy. However, do note that iOS also has threading configurations that are currently not available to custom embedders. The embedder API updates in this patch are ABI stable and existing embedders will continue to work are normal. For embedders that want to enable this composition mode, the API is designed to make it easy to opt into the same in an incremental manner. Rendering of contents into the “root” rendering surface remains unchanged. However, now the application can push “platform views” via a scene builder. These platform views need to handled by a FlutterCompositor specified in a new field at the end of the FlutterProjectArgs struct. When a new platform view in introduced within the layer tree, the compositor will ask the embedder to create a new render target for that platform view. Render targets can currently be OpenGL framebuffers, OpenGL textures or software buffers. The type of the render target returned by the embedder must be compatible with the root render surface. That is, if the root render surface is an OpenGL framebuffer, the render target for each platform view must either be a texture or a framebuffer in the same OpenGL context. New render target types as well as root renderers for newer APIs like Metal & Vulkan can and will be added in the future. The addition of these APIs will be done in an ABI & API stable manner. As Flutter renders frames, it gives the embedder a callback with information about the position of the various platform views in the effective hierarchy. The embedder is then meant to put the contents of the render targets that it setup and had previously given to the engine onto the screen (of course interleaving the contents of the platform views). Unit-tests have been added that test not only the structure and properties of layer hierarchy given to the compositor, but also the contents of the texels rendered by a test compositor using both the OpenGL and software rendering backends. Fixes b/132812775 Fixes flutter/flutter#35410

chinmaygarde · 2019-08-13T01:02:23Z

Ok, All comments have been addressed. The presubmits are having a rough time today but I'll keep patching targets as and when the bots encounter them.

amirh

Left a few nit, almost looks good, I just want to understand whether I'm misreading the test code first (left a comment)

amirh · 2019-08-13T16:57:55Z

shell/gpu/gpu_surface_gl.h

-  GPUSurfaceGL(sk_sp<GrContext> gr_context, GPUSurfaceGLDelegate* delegate);
+  GPUSurfaceGL(sk_sp<GrContext> gr_context,
+               GPUSurfaceGLDelegate* delegate,
+               bool render_to_surface);


The meaning of render_to_surface wasn't immediately clear to me (A GPU surface that does not render to surface?), maybe worth documenting it.

I documented the ivar and filed a bug to get rid of this hack.

amirh · 2019-08-13T16:59:02Z

shell/gpu/gpu_surface_gl.cc

@@ -236,6 +244,13 @@ std::unique_ptr<SurfaceFrame> GPUSurfaceGL::AcquireFrame(const SkISize& size) {
    return nullptr;
  }

+  if (!render_to_surface_) {


FWIW I feel like we've reached a point where we're applying concepts that doesn't align well with the higher level surface API and we should probably revisit it at some point. What do you think about leaving filing an issue saying we should refactor and leaving a TODO here describing the reason for this workaround and saying that we should clean it up in a coming refactoring?

Right, added a TODO and filed a bug flutter/flutter#38466

amirh · 2019-08-13T17:06:51Z

shell/platform/embedder/embedder_external_view_embedder.cc

+    render_canvas->flush();
+
+    // Indicate a layer for the platform view.
+    presented_platform_views[view_id] = MakePlatformView(view_id);


I guess that comment wasn't explicit enough for me 😄
I'd suggest adding a comment here, saying that we add to presented_platform_views in order to keep at allocated just for the scope of the current method.

amirh · 2019-08-13T17:07:54Z

shell/platform/embedder/embedder_render_target.cc

+      render_surface_(std::move(render_surface)),
+      on_release_(on_release) {
+  // The optimization to elide backing store updates between frames has not been
+  // implemented yet.


Let's file an issue describing what has to be done and link it from a TODO here.

amirh · 2019-08-13T17:23:59Z

shell/platform/embedder/tests/embedder_unittests.cc

+          } break;
+          default:
+            // Asked to render an unknown layer.
+            FML_CHECK(false) << "Test was asked for unknown layer";


nit: unknown platform view

amirh · 2019-08-13T17:26:32Z

shell/platform/embedder/fixtures/main.dart

+    Color blue = Color.fromARGB(127, 0, 0, 255);
+    Color gray = Color.fromARGB(127, 127, 127, 127);
+
+    Size size = Size(100.0, 100.0);


nit: I'd make width != height to catch a possible faulty swap of these parameters.

Roger. Update fixtures and expectations.

amirh · 2019-08-13T17:30:30Z

shell/platform/embedder/tests/embedder_unittests.cc

+          layer.type = kFlutterLayerContentTypeBackingStore;
+          layer.backing_store = &backing_store;
+          layer.size = FlutterSizeMake(100.0, 100.0);
+          layer.offset = FlutterPointMake(20.0, 20.0);


I'm probably confused but shouldn't this be 0,0?

The test code is:

SceneBuilder builder = SceneBuilder(); builder.pushOffset(0.0, 0.0); // 10 builder.addPicture(Offset(10.0, 10.0), CreateColoredBox(red, size)); // red - flutter // 20 builder.pushOffset(20.0, 20.0); builder.addPlatformView(1, width: size.width, height:size.height); // green - platform builder.pop(); // 30 builder.addPicture(Offset(30.0, 30.0), CreateColoredBox(blue, size)); // blue - flutter

the [20,20] offset is popped just before adding the layer with index 2.

You're right. But, isn't the size wrong too? Shouldn't it be the frame size?

Fixed. I also updated the render size.

amirh · 2019-08-13T17:31:31Z

shell/platform/embedder/tests/embedder_unittests.cc

+          layer.type = kFlutterLayerContentTypeBackingStore;
+          layer.backing_store = &backing_store;
+          layer.size = FlutterSizeMake(100.0, 100.0);
+          layer.offset = FlutterPointMake(40.0, 40.0);


Same here, shouldn't it be 0,0 ?

Roger. Ditto about the size being wrong too.

Fixed. I also updated the render size.

amirh

LGTM!

…tter/engine#10195)

[email protected]:flutter/engine.git/compare/be4c8338a6ab...4b6b4af git log be4c833..4b6b4af --no-merges --oneline 2019-08-13 [email protected] Roll src/third_party/skia cd8b6d5c1cb8..f75996469d02 (5 commits) (flutter/engine#10984) 2019-08-13 [email protected] Allow embedder controlled composition of Flutter layers. (flutter/engine#10195) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.

liyuqian · 2019-08-26T17:41:39Z

@chinmaygarde : will this be used for Fuchsia in the future so we can retire all #ifdef OS_FUCHSIA?

chinmaygarde · 2019-08-26T18:53:40Z

Yes. the plan is to use the Embedder API for the Fuchsia runner eventually.

chinmaygarde requested review from stuartmorgan, cbracken, jason-simmons and liyuqian July 27, 2019 02:49

googlebot added the cla: yes label Jul 27, 2019

chinmaygarde mentioned this pull request Jul 27, 2019

Enable Flutter compositor layers flutter/flutter#35410

Closed

chinmaygarde requested a review from amirh July 27, 2019 02:52

stuartmorgan reviewed Jul 29, 2019

View reviewed changes

amirh reviewed Jul 29, 2019

View reviewed changes

chinmaygarde commented Jul 30, 2019

View reviewed changes

chinmaygarde requested a review from franciscojma86 July 30, 2019 21:31

chinmaygarde force-pushed the embedder_layer branch 2 times, most recently from 99c6bbd to 1a451be Compare July 31, 2019 19:20

amirh reviewed Aug 2, 2019

View reviewed changes

chinmaygarde force-pushed the embedder_layer branch from 492def4 to f1017ad Compare August 5, 2019 19:14

stuartmorgan reviewed Aug 7, 2019

View reviewed changes

amirh reviewed Aug 8, 2019

View reviewed changes

chinmaygarde force-pushed the embedder_layer branch 2 times, most recently from 357474a to f36c0f6 Compare August 12, 2019 20:57

chinmaygarde force-pushed the embedder_layer branch from 58118a0 to 9ac4d51 Compare August 12, 2019 23:12

chinmaygarde added 3 commits August 12, 2019 16:26

Document tests.

2761522

FlutterBackingStore::did_update

29791ff

Optionally render to surface

8e160ba

chinmaygarde added 2 commits August 12, 2019 18:07

Fix iOS & Android

91a3d39

Windows fix

9b8c091

amirh reviewed Aug 13, 2019

View reviewed changes

chinmaygarde added 8 commits August 13, 2019 11:47

Sizes

9f14706

PR

786ae19

PR

4c5b365

PR

c09cc95

Document ivar.

fa25174

PR

04b0955

Comment

6f88bc9

Width != Height

976b965

amirh approved these changes Aug 13, 2019

View reviewed changes

chinmaygarde merged commit e8f9544 into flutter:master Aug 13, 2019

chinmaygarde deleted the embedder_layer branch August 13, 2019 21:53

engine-flutter-autoroll mentioned this pull request Aug 13, 2019

Roll engine be4c8338a6ab..4b6b4afd6492 (2 commits) flutter/flutter#38498

Merged

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

e8f9544 Allow embedder controlled composition of Flutter layers. (flu…

b8e5c71

…tter/engine#10195)


		namespace flutter {

		class EmbedderExternalViewEmbedder : public ExternalViewEmbedder {

Allow embedder controlled composition of Flutter layers. #10195

Allow embedder controlled composition of Flutter layers. #10195

Conversation

chinmaygarde commented Jul 27, 2019

stuartmorgan 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

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

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

chinmaygarde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmaygarde commented Jul 30, 2019

chinmaygarde commented Aug 1, 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

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

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 2, 2019

chinmaygarde commented Aug 5, 2019

chinmaygarde commented Aug 5, 2019