Android Embedder Migration to Embedder API

[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]

Just curious, why is this needed? I would think we could remove this dependency as well. I believe desktop platforms do not depend on this library. Not a big deal either ways, asking for my understanding.

[mboetger]

It's not actually needed - I just added it since it was considered optional - will make it more clear.

[loic-sharma]

For the primary Flutter layer (platform_views_count == 0), AndroidCompositor::CreateBackingStore returns the main surface framebuffer (GetFBO()).

Does the prototype already do this? I'd be curious how this works. This seems like a good optimization, but AFAICT FlutterBackingStoreConfig does not provide enough information in the create_backing_store_callback to determine if you can render to the main surface framebuffer directly.

[mboetger]

I did try doing this - but ended up adding an is_overlay to the FlutterBackingStoreConfig to get things to actually work, so this is no longer true - you're right that there wasn't enough info.

I'll rewrite step 1 to propose an is_root_layer field on FlutterBackingStoreConfig (false unless the layer is the bottom one with no platform view beneath it), require avoid_backing_store_cache = true, and fall back to an offscreen store plus a blit when the field is missing. I'd imagine the iOS RFC needs the same info - wdyt @cbracken ?

[loic-sharma]

Here's the similar conversation on the iOS RFC: #27 (comment)

FYI here's a concern I bubbled up on the iOS design:

One thing that's not clear to me: how will resizing work with the drawable-root approach? Today, embedders that use FlutterCompositor resize during FlutterCompositor.present_view_callback. I imagine with the drawable-root approach, the embedder will instead need to resize when the engine requests the root layer's backing store? Assuming that's correct, this might require migrating existing embedders that use FlutterCompositor.

If Android uses the main surface framebuffer as the backing store, we'll need to resize step to happen when the engine requests the backing store, instead of when the engine presents (which is what all embedders that use FlutterCompositor do today).

[loic-sharma]

... single atomic platform-thread frame transaction...

Could you expand on this? I don't follow this bit. Is this the synchronization_fence_fd mechanism described below, or something else?

[mboetger]

Something else, and "transaction" was the wrong word next to SurfaceControl.Transaction. synchronization_fence_fd only exists on the HCPP path (API 34+, Vulkan); HC never uses it. What I meant: after the raster thread swaps frame N into the ImageReaders, it posts one platform-thread task that runs onBeginFrame, every onDisplayPlatformView and onDisplayOverlaySurface, and onEndFrame, so no Android traversal can draw a half-applied frame; today's HCPP embedder already does this with a single PostTask (external_view_embedder_2.cc). For HC the raster thread also waits for that task, because onEndFrame calls acquireLatestImage() (PlatformViewsController.java), which would otherwise show frame N+1's pixels with frame N's view positions; the legacy SubmitFlutterView got this ordering for free by rendering and making the JNI calls on the merged platform thread. I'll rewrite the section in those terms, drop "atomic", and describe the two steps that still need a synchronous platform-thread hop mid-frame: creating an overlay surface, and the first HC frame's switch to FlutterImageView.

[loic-sharma]

These LGTM, but we should get these new members reviewed by @chunhtai and @hannah-hyj

[chunhtai]

We also need SemanticsRole

[mboetger]

Added SemanticRole and tried to align more with what Android currently uses. Dropped traversal_parent - since I guess that's just web. PTAL

[loic-sharma]

Does this need to be a pointer?

[loic-sharma]

A potential alternative might be adding a new renderer type that is a callback that produces a renderer config.

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

This approach should remove the need for FlutterEngineLaunchShell, FlutterEngineNotifyCreated, FlutterEngineNotifyDestroyed. I suspect we might be able to remove defer_surface_creation by deferring PlatformView::NotifyCreated until the deferred renderer config is resolved.

Opus's feedback:

1. External texture registration would need to be updated since that depends on the renderer type.
2. Today, FlutterEngineInitialize validates the renderer config at startup. This would now happen in a background thread.

[loic-sharma]

I imagine the engine should never cache the default framebuffer / next swapchain image? Should we make a prerequisite for this being true that avoid_backing_store_cache is true? Otherwise, the engine would never be able to cache the root layer's backing store.

[loic-sharma]

This section seems unnecessarily dense. Could we simplify it?

[loic-sharma]

This seems like a major shift in how HC platform views work.

From my understanding, the Android embedder checks at startup whether it can use HC or HCPP. If HC is picked, we merge the raster thread whenever there are platform views. Did we consider an alternative where the embedder API has an opt-in flag to merge the raster thread if there are platform views, and the Android embedder turns on this flag if it detects the current device does not support HCPP?

If we do stick with this approach, this seems like something we complete in phase 1.

[loic-sharma]

One concern is that an embedder might break if they receive a new mutation type they don't know how to handle. We could add a kFlutterPlatformViewMutationCapabilityClipPath capability, and if the embedder didn't opt-in, fallback to a clip rect mutation. Am I being too paranoid though? I lean towards skipping this and keeping it simple.

cc @cbracken for thoughts.

[loic-sharma]

Looks like this one doesn't exist:

https://github.com/flutter/flutter/blob/5f5027d7b388695f1598ef80697c9d439811d3f1/engine/src/flutter/lib/ui/semantics.dart#L549-L561

[loic-sharma]

I assume this isn't used with the OpenGL renderer?

[loic-sharma]

When the Dart VM finishes loading and drops its references to both data and instruction buffers, ...

It's not clear to me this is correct. The Dart embedder API does not specify the lifetime of these buffers: https://github.com/dart-lang/sdk/blob/7b69ded4350b8f3b02883ccdb34ac4d48c43e71d/runtime/include/dart_api.h#L3607-L3618

The Flutter engine appears to attempt to keep these buffers alive while it is alive: https://github.com/flutter/flutter/blob/d60340594eb2ab394bd1198c2f950d066dd679c6/engine/src/flutter/runtime/dart_isolate.cc#L571

I suspect the embedder will need to keep the buffers in memory until the engine is shutdown.

[loic-sharma]

It looks like tilt and orientation need to be added to the embedder API:

https://github.com/flutter/flutter/blob/1bc0d733e022a685ef50ff6abbbf0dbd534c9c44/engine/src/flutter/shell/platform/embedder/embedder.h#L1400-L1447

[loic-sharma]

It looks like we'll need to add FlutterPointerEvent.embedder_id to the embedder API:

See:

• https://github.com/flutter/flutter/blob/1bc0d733e022a685ef50ff6abbbf0dbd534c9c44/engine/src/flutter/shell/platform/android/io/flutter/embedding/android/AndroidTouchProcessor.java#L372
• https://github.com/flutter/flutter/blob/1bc0d733e022a685ef50ff6abbbf0dbd534c9c44/engine/src/flutter/shell/platform/embedder/embedder.cc#L2895-L2896
• https://github.com/flutter/flutter/blob/1bc0d733e022a685ef50ff6abbbf0dbd534c9c44/engine/src/flutter/shell/platform/android/io/flutter/plugin/platform/PlatformViewsController.java#L732-L734

[loic-sharma]

It's weird that some of the members on FlutterProjectArgs are repeated on FlutterEngineSpawnInfo. For example, the entrypoint arguments are on both structs.

Is it possible to remove the project args and renderer config? Can the new engine reuse the parent engine's configuration? This would let us avoid weird things like attempting to use different rendering configs.

[loic-sharma]

From my understanding, the reason the embedder API's Skia Vulkan renderer config didn't need this is because the engine performed the host sync before invoking the present callback:

https://github.com/flutter/flutter/blob/b304cf18b6864d141cf08e26a07898901cddb4b5/engine/src/flutter/shell/platform/embedder/embedder.h#L1061-L1063

Can/should we reuse this approach for HCPP?

---

If we do decide to keep this fence, I suspect we should make this an opt-in feature. Perhaps something like FlutterVulkanRendererConfig.enable_manual_present_host_sync.

[loic-sharma]

Should we add the initial route to FlutterProjectArgs instead? It looks like you can use set the initial route without multi-engine: https://docs.flutter.dev/add-to-app/android/add-flutter-fragment#run-flutter-with-a-specified-initial-route

val flutterFragment = FlutterFragment.withNewEngine()
    .initialRoute("myInitialRoute/")
    .build()

EDIT: Never mind this suggestion. Android/iOS set the initial engine's initial route by sending a message immediately at startup (link). This is gross, but we can fix that later.

[loic-sharma]

I would add more information to this section. I wasn't familiar with the callback cache and how one uses it. Consider linking to this blog post: https://flutter.dev/blog/executing-dart-in-the-background-with-flutter-plugins-and-geofencing

[loic-sharma]

Could we document which threads each of these callbacks are invoked from?

[loic-sharma]

Could you expand how you picked the name "image generator" instead of "image decoder"? The latter seems more common.

[loic-sharma]

Could you explain how an image with multiple fames (eg. GIFs) is decoded? Does the engine call get_pixels for each frame or something? Or does the engine create a generator per frame?

[loic-sharma]

Should we add a FlutterImageGeneratorColorSpace enum that today only has a kFlutterImageGeneratorColorSpaceSRGB value? This would make it easier for us to add wide gamut support in the future.

[loic-sharma]

Could you add docs on this? I assume the engine picks these in descending order, 0 is the engine's built-in priority, -1 means prefer engine built-in implementation, etc?