Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow embedder controlled composition of Flutter layers. #10195

Merged
merged 14 commits into from Aug 13, 2019

Conversation

@chinmaygarde
Copy link
Member

commented Jul 27, 2019

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

Copy link
Contributor

left a comment

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.

namespace fml {

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

class AutoFireClosure {

This comment has been minimized.

Copy link
@stuartmorgan

stuartmorgan Jul 29, 2019

Contributor

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Jul 30, 2019

Author Member

Done

private:
fml::closure closure_;

FML_DISALLOW_COPY_AND_ASSIGN(AutoFireClosure);

This comment has been minimized.

Copy link
@stuartmorgan

stuartmorgan Jul 29, 2019

Contributor

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Jul 29, 2019

Author Member

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.

This comment has been minimized.

Copy link
@stuartmorgan

stuartmorgan Aug 6, 2019

Contributor

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.

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 12, 2019

Author Member

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.


namespace flutter {

class GPUSurfaceSoftwareDelegate {

This comment has been minimized.

Copy link
@stuartmorgan

stuartmorgan Jul 29, 2019

Contributor

Comment class and methods.

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Jul 31, 2019

Author Member

Done.

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

This comment has been minimized.

Copy link
@stuartmorgan

stuartmorgan Jul 29, 2019

Contributor

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Jul 31, 2019

Author Member

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.

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 2, 2019

Author Member

I was wrong #10480

return surface;
}

static sk_sp<SkSurface> WrapPlatformRenderTarget(

This comment has been minimized.

Copy link
@stuartmorgan

stuartmorgan Jul 29, 2019

Contributor

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

@@ -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;

This comment has been minimized.

Copy link
@stuartmorgan

stuartmorgan Jul 29, 2019

Contributor

Comment.

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 1, 2019

Author Member

Done.

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

This comment has been minimized.

Copy link
@stuartmorgan

stuartmorgan Jul 29, 2019

Contributor

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)?

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Jul 30, 2019

Author Member

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?

This comment has been minimized.

Copy link
@stuartmorgan

stuartmorgan Aug 6, 2019

Contributor

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 12, 2019

Author Member

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.


namespace flutter {

class EmbedderExternalViewEmbedder : public ExternalViewEmbedder {

This comment has been minimized.

Copy link
@stuartmorgan

stuartmorgan Jul 29, 2019

Contributor

Comment

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 1, 2019

Author Member

Done.


namespace flutter {

class EmbedderRenderTarget final {

This comment has been minimized.

Copy link
@stuartmorgan

stuartmorgan Jul 29, 2019

Contributor

Comment class and methods.

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 1, 2019

Author Member

Done.


namespace flutter {

class EmbedderRenderTargetRegistry {

This comment has been minimized.

Copy link
@stuartmorgan

stuartmorgan Jul 29, 2019

Contributor

Comment.

Copy link
Contributor

left a comment

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).

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;

This comment has been minimized.

Copy link
@amirh

amirh Jul 29, 2019

Contributor

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.

This comment has been minimized.

Copy link
@amirh

amirh Jul 29, 2019

Contributor

Or at least, wasn't that the plan?

// 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.

This comment has been minimized.

Copy link
@amirh

amirh Jul 29, 2019

Contributor

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Jul 31, 2019

Author Member

Updated.

void* user_data);

typedef bool (*FlutterCompositorLayersPresentCallback)(
const FlutterCompositorLayer** layers,

This comment has been minimized.

Copy link
@amirh

amirh Jul 29, 2019

Contributor

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?

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

This comment has been minimized.

Copy link
@amirh

amirh Jul 29, 2019

Contributor

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 2, 2019

Author Member

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

Copy link
Member Author

left a comment

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.

namespace fml {

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

class AutoFireClosure {

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Jul 30, 2019

Author Member

Done

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

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

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Jul 30, 2019

Author Member

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

@chinmaygarde chinmaygarde requested a review from franciscojma86 Jul 30, 2019
@chinmaygarde

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

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 chinmaygarde force-pushed the chinmaygarde:embedder_layer branch 2 times, most recently from 99c6bbd to 1a451be Jul 31, 2019
@chinmaygarde

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

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

Copy link
Contributor

left a comment

reviewed everything but tests

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

This comment has been minimized.

Copy link
@amirh

amirh Aug 2, 2019

Contributor

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)

This comment has been minimized.

Copy link
@amirh

amirh Aug 2, 2019

Contributor

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 2, 2019

Author Member

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.

// 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

This comment has been minimized.

Copy link
@amirh

amirh Aug 2, 2019

Contributor

nit: to composite

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 2, 2019

Author Member

Done.

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

This comment has been minimized.

Copy link
@amirh

amirh Aug 2, 2019

Contributor

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 2, 2019

Author Member

Clarified.

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

This comment has been minimized.

Copy link
@amirh

amirh Aug 2, 2019

Contributor

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?

This comment has been minimized.

Copy link
@amirh

amirh Aug 2, 2019

Contributor

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)

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 2, 2019

Author Member

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?

This comment has been minimized.

Copy link
@stuartmorgan

stuartmorgan Aug 7, 2019

Contributor

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

This comment has been minimized.

Copy link
@amirh

amirh Aug 8, 2019

Contributor

ScopedCleanupClosure sgtm

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 12, 2019

Author Member

Done.


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

This comment has been minimized.

Copy link
@amirh

amirh Aug 2, 2019

Contributor

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 2, 2019

Author Member

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).

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

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

This comment has been minimized.

Copy link
@amirh

amirh Aug 2, 2019

Contributor

Why not keep this default implementation in GPUSurfaceGLDelegate?

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 2, 2019

Author Member

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).

This comment has been minimized.

Copy link
@amirh

amirh Aug 2, 2019

Contributor

sgtm

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

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

This comment has been minimized.

Copy link
@amirh

amirh Aug 2, 2019

Contributor

Why not keep this in GPUSurfaceSoftwareDelegate?

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 2, 2019

Author Member

Similar reasoning as above.


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

This comment has been minimized.

Copy link
@amirh

amirh Aug 2, 2019

Contributor

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 2, 2019

Author Member

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.

This comment has been minimized.

Copy link
@amirh

amirh Aug 8, 2019

Contributor

Right 😳


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

This comment has been minimized.

Copy link
@amirh

amirh Aug 2, 2019

Contributor

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 2, 2019

Author Member

Done.

render_canvas->flush();

// Indicate a layer for the platform view.
presented_platform_views[view_id] = MakePlatformView(view_id);

This comment has been minimized.

Copy link
@amirh

amirh Aug 2, 2019

Contributor

What is presented_platform_views used for?

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 2, 2019

Author Member

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.

This comment has been minimized.

Copy link
@amirh

amirh Aug 8, 2019

Contributor

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?

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 12, 2019

Author Member

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

This comment has been minimized.

Copy link
@amirh

amirh Aug 13, 2019

Contributor

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.

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 13, 2019

Author Member

Done.

@chinmaygarde

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

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

@chinmaygarde chinmaygarde force-pushed the chinmaygarde:embedder_layer branch from 492def4 to f1017ad Aug 5, 2019
@chinmaygarde

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2019

@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

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2019

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

@stuartmorgan

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

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

Copy link
Contributor

left a comment

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.

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

This comment has been minimized.

Copy link
@stuartmorgan

stuartmorgan Aug 7, 2019

Contributor

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

};
} FlutterOpenGLBackingStore;

typedef struct {

This comment has been minimized.

Copy link
@stuartmorgan

stuartmorgan Aug 7, 2019

Contributor

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 12, 2019

Author Member

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

// 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

This comment has been minimized.

Copy link
@stuartmorgan

stuartmorgan Aug 7, 2019

Contributor

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 12, 2019

Author Member

Done.

Copy link
Contributor

left a comment

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.

render_canvas->flush();

// Indicate a layer for the platform view.
presented_platform_views[view_id] = MakePlatformView(view_id);

This comment has been minimized.

Copy link
@amirh

amirh Aug 8, 2019

Contributor

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?

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

virtual ~ExternalViewEmbedder() = default;

virtual sk_sp<SkSurface> GetRootSurface();

This comment has been minimized.

Copy link
@amirh

amirh Aug 8, 2019

Contributor

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.

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 12, 2019

Author Member

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

}

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

This comment has been minimized.

Copy link
@amirh

amirh Aug 8, 2019

Contributor

nit: s/can be affected/is set/

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 12, 2019

Author Member

Done.

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

This comment has been minimized.

Copy link
@amirh

amirh Aug 8, 2019

Contributor

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?

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 12, 2019

Author Member

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.

This comment has been minimized.

Copy link
@cyanglaz

cyanglaz Aug 13, 2019

Contributor

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

namespace flutter {
namespace testing {

//

This comment has been minimized.

Copy link
@amirh

amirh Aug 8, 2019

Contributor

Do we need this?

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 12, 2019

Author Member

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

@chinmaygarde chinmaygarde force-pushed the chinmaygarde:embedder_layer branch 2 times, most recently from 357474a to f36c0f6 Aug 9, 2019
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 chinmaygarde force-pushed the chinmaygarde:embedder_layer branch from 58118a0 to 9ac4d51 Aug 12, 2019
@chinmaygarde

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

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.

Copy link
Contributor

left a comment

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

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

This comment has been minimized.

Copy link
@amirh

amirh Aug 13, 2019

Contributor

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.

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 13, 2019

Author Member

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

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

if (!render_to_surface_) {

This comment has been minimized.

Copy link
@amirh

amirh Aug 13, 2019

Contributor

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?

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 13, 2019

Author Member

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

render_canvas->flush();

// Indicate a layer for the platform view.
presented_platform_views[view_id] = MakePlatformView(view_id);

This comment has been minimized.

Copy link
@amirh

amirh Aug 13, 2019

Contributor

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.

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

This comment has been minimized.

Copy link
@amirh

amirh Aug 13, 2019

Contributor

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 13, 2019

Author Member

Done.

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

This comment has been minimized.

Copy link
@amirh

amirh Aug 13, 2019

Contributor

nit: unknown platform view

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 13, 2019

Author Member

Done.

Color blue = Color.fromARGB(127, 0, 0, 255);
Color gray = Color.fromARGB(127, 127, 127, 127);

Size size = Size(100.0, 100.0);

This comment has been minimized.

Copy link
@amirh

amirh Aug 13, 2019

Contributor

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 13, 2019

Author Member

Roger. Update fixtures and expectations.

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

This comment has been minimized.

Copy link
@amirh

amirh Aug 13, 2019

Contributor

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.

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 13, 2019

Author Member

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 13, 2019

Author Member

Fixed. I also updated the render size.

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

This comment has been minimized.

Copy link
@amirh

amirh Aug 13, 2019

Contributor

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

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 13, 2019

Author Member

Roger. Ditto about the size being wrong too.

This comment has been minimized.

Copy link
@chinmaygarde

chinmaygarde Aug 13, 2019

Author Member

Fixed. I also updated the render size.

PR
PR
PR
PR
@amirh
amirh approved these changes Aug 13, 2019
Copy link
Contributor

left a comment

LGTM!

@chinmaygarde chinmaygarde merged commit e8f9544 into flutter:master Aug 13, 2019
14 checks passed
14 checks passed
WIP Ready for review
Details
build_and_test_android_profile_app Task Summary
Details
build_and_test_android_profile_app
Details
build_and_test_android_unopt_debug Task Summary
Details
build_and_test_android_unopt_debug
Details
build_and_test_linux_unopt_debug Task Summary
Details
build_and_test_linux_unopt_debug
Details
build_windows_opt_debug Task Summary
Details
build_windows_opt_debug
Details
build_windows_unopt_debug Task Summary
Details
build_windows_unopt_debug
Details
format_and_dart_test Task Summary
Details
format_and_dart_test
Details
luci-engine
Details
@chinmaygarde chinmaygarde deleted the chinmaygarde:embedder_layer branch Aug 13, 2019
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Aug 14, 2019
git@github.com:flutter/engine.git/compare/be4c8338a6ab...4b6b4afd6492

git log be4c8338a6ab..4b6b4afd6492 --no-merges --oneline
2019-08-13 skia-flutter-autoroll@skia.org Roll src/third_party/skia cd8b6d5c1cb8..f75996469d02 (5 commits) (flutter/engine#10984)
2019-08-13 chinmaygarde@google.com 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 (stuartmorgan@google.com), and stop
the roller if necessary.
@liyuqian

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

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

@chinmaygarde

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.