[mboetger]

Android Embedder Migration RFC

Tracking issue: flutter/flutter#193309

Pre-launch Checklist

• I read [RFC 000.0001: Taxonomy] and followed the file and path naming and metadata standards.
• I read [RFC 000.0002: Process] and confirmed this proposal meets the threshold for a full RFC.
• I read and agree to the [Code of Conduct].
• I signed the [CLA].
• I have linked an issue from flutter/flutter with the label design doc.
• All existing and new tests are passing.
• I have enabled "Allow edits from maintainers" on this PR so the bot can automatically assign an RFC number (or I will run dart run bin/assign_rfc_number.dart locally when instructed).

[loic-sharma]

Instead of having this negotiation back-and-forth between the embedder and engine, could we move all checks to the embedder? Android would be fully responsible for picking between Vulkan and OpenGL. The engine would error if Android picks Vulkan but a Vulkan context cannot be created.

I think this model would let us have a simplified deferred renderer config:

Simplified API...

typedef enum {
  ...

  /// The renderer is chosen lazily by the embedder after the engine
  /// has been initialized. See |FlutterDeferredRendererConfig|.
  kDeferred,
} FlutterRendererType;

struct FlutterRendererConfig {
  FlutterRendererType type;
  union {
    FlutterOpenGLRendererConfig open_gl;
    FlutterSoftwareRendererConfig software;
    FlutterMetalRendererConfig metal;
    FlutterVulkanRendererConfig vulkan;
    FlutterDeferredRendererConfig deferred;
  };
};

/// Callback invoked by the engine to resolve a deferred renderer
/// configuration into a concrete one.
///
/// @param[in]  user_data   The `user_data` passed to
///                         `FlutterEngineInitialize`
///                         or `FlutterEngineRun`.
/// @param[out] config_out  Engine-owned storage. The embedder must set
///                         `config_out->type` to `kOpenGL`, `kSoftware`,
///                         `kMetal`, or `kVulkan` and populate the
///                         corresponding union member. `kDeferred` is not
///                         allowed. All memory referenced by the
///                         configuration must remain valid until
///                         `FlutterEngineDeinitilaize` or
///                         `FlutterEngineShutdown` returns.
///
/// @return     On error, `false`.
typedef bool (*FlutterRendererConfigResolveCallback)(
    void* /* user_data */,
    FlutterRendererConfig* /* config_out */);

typedef struct {
  size_t struct_size;
  /// Invoked exactly once by the engine to obtain the concrete renderer
  /// configuration. Required.
  ///
  /// The callback is invoked once on the raster thread.
  /// Platform thread work that requires a GPU context blocks until this
  /// callback returns.
  FlutterRendererConfigResolveCallback resolve_callback;
} FlutterDeferredRendererConfig;

[mboetger]

Moved the checks to the Embedder and highlighted that code be moved out of Impeller.

[andywolff]

Wait, I don't think we want the engine to error out if Android picks Vulkan but then fails to create a context. We want it to fall back to openGLES in that case, right? Vulkan context creation can fail for reasons that are hard to predict ahead of time, like a bad driver update or something.

[mboetger]

Yes, we want it to fall back.

[mboetger]

So here's the problem. If we move ALL the checks into flutter/common and have the embedder do this - then some embedders won't get the benefit of the Vulkan capability checks. If we only move some of the checks to embedder we still need a way of handling the fall back to OpenGL (and another embedder API). WDYT @loic-sharma

[gaaclarke]

I didn't make it through the whole doc yet. I have to put a pin in it for now and come back so here's some general feedback to go with my notes so far.

I think there is a lot of awesome work that went into this. This scares the hell out of me. I'm glad there is a section that lays out the implementation plan since that is my biggest concern, "how do we implement this huge change carefully and iteratively". I'm looking forward to that. Also I'm glad you've included a testing section since we are by virtue of expanding embedder.h, signing ourselves up for maintaining a new API, which should be tested.

I do find this document hard to navigate. I don't have a good hierarchy of it in my head (maybe this is the failings of my markdown preview). Maybe it's big enough there should be something like a TOC at the beginning?

I think leading with diagrams would be helpful. I often found myself reading code in prose form, trying to follow along and then finding the diagram and not liking that I had just wasted time trying to juggle the picture in my mind. I don't really find "code in prose" much help. Pseudocode or sequence diagrams are better.

When writing newspaper articles what they try to do is lead with the important information first so that the most important information is conveyed first to accommodate someone being able to abort reading a section and still coming away with a general understanding of the section. The concept should be communicated first and each line should provide further details. I think that sort of ordering can help make this easier to digest. Someone shouldn't have to read a whole section to get the gist.

The diagrams are following an ad hoc language. I'd find it more helpful if you use standardized diagrams (like UML class, UML sequence).

I have a lot of meetings tomorrow but I'll try to come back to this ASAP.

(note to self left off at Alternative Considered: Splitting Startup into Separate Shell and Isolate Launch APIs)

[gaaclarke]

This chart would be more clear if it used standardized iconography like a UML class diagram.

[mboetger]

Honestly it made it less readable to me.

[gaaclarke]

It makes sense that you'd author it the way that was most comfortable for you. Being less readable is likely a familiarity thing though that will adjust over time. It's nice to have a standard instead of coming into each doc and trying to decipher a new design language =)

[gaaclarke]

nit: It would be helpful if standard sequence diagram iconography was used here too (UML sequence diagram). In particular participants not being associated with instances is a bit of a twist, but abstract participants for threads is clear.

[gaaclarke]

I have misgivings about making "Spawn" a public API. It's not very friendly. What we've done previously is made FlutterEngineGroup the public API to this concept, which uses spawn under the covers. I think that's a better approach.

[mboetger]

THe android embedder does provide FlutterEngineGroup as a public API - I guess we could match the same API and just forward to the embedder API. Will take a look

[gaaclarke]

Do we need to maintain an alternate benchmark for legacy? Can't we just watch the existing benchmark as we start to transition?

[mboetger]

It's behind a flag - so we need to be able to test before switching the flag on by default.

[mboetger]

@gaaclarke - I took a massive hatchet to the context provided on how things work and why and left only what was necessary. I think this makes the doc more manageable. PTAL.

[andywolff]

Is AndroidSurfaceControl the best place for the bad driver checks when considering other embedders?

The checks look mostly android-focused, but some of the checks refer to hardware like PowerVR and Adreno, which can exist on non-Android embedded systems. It would be unfortunate if a new custom embedder had to start from scratch or duplicate the checks because we moved them to an android embedder specific location.

[mboetger]

I've gone back and forth a couple times with @loic-sharma and @gaaclarke about this - it simplifies the Embedder API such that we don't need to add a "Driver Check" API. But I agree other embedders will lose out. Perhaps moving it to the common library makes more sense.

[andywolff]

Wait, I don't think we want the engine to error out if Android picks Vulkan but then fails to create a context. We want it to fall back to openGLES in that case, right? Vulkan context creation can fail for reasons that are hard to predict ahead of time, like a bad driver update or something.

[andywolff]

Are we conflating a caching policy with information about topology here? It's weird to me that a field called is_root_layer would ever be false for the root layer. Do we need a different channel to communicate whether a backingstore should be cached or not? Maybe an is_cacheable field on FlutterBackingStore, or swapchain info

[loic-sharma]

DlClipOp is engine internals. I assume any higher-Z platform views would have FlutterPlatformViewMutations?

[mboetger]

This happens in the engine internals - embedder_external_view_embedder.cc.

[loic-sharma]

Sure, but the embedder API is externally-facing. The embedder API's docs should prefer the external names for concepts over the internal names.

[loic-sharma]

I wonder if we should make this an engine flag instead, similar other engine flags like --enable-hcpp-and-surface-control.

This would let us remove raster thread merging from the embedder API if/when we remove HC or we update HC to use latches instead of thread merging.

[loic-sharma]

I believe to opt-in to merged threads you need to pass a non-NULL FlutterCustomTaskRunners.platform_task_runner (which matches the ui_task_runner). If so, can we remove this requirement?

[knopp]

Is raster thread merging absolutely necessary? This is one of the most convoluted parts of the engine and it's only used by the android embedder right now. "traditionally" platform views were mutated in preroll, so the thread merging was in a way necessary, but the embedder api records mutations so we only need to synchronize platform and raster thread at a single spot (schedule mutation on platform thread and latch the raster thread). Both macOS and iOS embedders do this. Does this not work for android embedder? I'd very much like to not expose dynamic thread merging in the embedder API.

[gaaclarke]

Thanks Matt, the doc is looking way better layout in the presentation. Here are a few questions that came to my mind during it:

1) What about SDFs?

I recommend we try to ignore this as long as possible. Ideally in the future, everyone just uses the SDF rendering. If a customer asks for it we can consider engine flags.

2) What about vulkan validation layers?

What does the user story look like for the embedder that wants to enable vulkan validation layers? Do we care to support that?

3) What about kMergeAfterLaunch?

How do we want to surface its in the embedder, if at all? Right now flutter engine groups don't work with it. So we need to probably opt out of it, or allow it to be selectable if we want to expose flutter engine group apis.

[loic-sharma]

Could we put this in //flutter/shell/platform/common? That's where we generally put code that's shared across embedders, instead of //flutter/common.