Multiview ExternalViewEmbedder #46169

dkwingsmt · 2023-09-21T23:14:37Z

This PR adds multiview support for ExternalViewEmbedder.

Nomenclature

The term view can be ambiguous in ExternalViewEmbedder, and therefore the following terms are used:

A native view refers to the final drawing surface that to composite layers to. It is the "view" used in other places of the engine, such as Shell::AddView.
A platform view refers a platform view, a layer that holds content to be embedded.

Change

The lifecycle of ExternalViewEmbedder is changed:

Before PR	After PR	How it's called
BeginFrame	BeginFrame	Once per frame
BeginFrame	PrepareFlutterView	Once per flutter view
SubmitFrame	SubmitFlutterView (renamed)	Once per flutter view
EndFrame	EndFrame (unchanged)	Once per frame

BeginFrame should perform per-frame actions, such as merge-unmerging threads.
PrepareView should perform per-native-view preparations, such as recording the view ID and view size.

This change is necessary because some actions in PrepareView needs to be refreshed at the beginning of drawing every native view.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flow/embedded_views.h

loic-sharma · 2023-11-08T18:10:10Z

flow/embedded_views.h

+//
+//   1. At the start of a frame, call |BeginFrame|, then |SetUsedThisFrame| to
+//      true.
+//   2. For each view to be drawn, call |PrepareView|, then |SubmitView|.


View versus platform view is confusing. Would it be better to refer to views that back Flutter content as "Flutter views"?

The status is:

There are "native views". As defined above, each native view corresponds to a window (simply put).

The content for each native view is composited from a layer tree.

Layer trees have types, platform view layer being one of them.

I'm not sure if a "flutter view" is the complement for "platform view".

I would be tempted to use the term Flutter View and Platform View explicitly where there's any possibility for confusion.

I changed the names to PrepareFlutterView SubmitFlutterView flutter_view_id and platform_view_id.

loic-sharma · 2023-11-08T18:15:02Z

shell/platform/embedder/embedder_external_view_embedder.cc

+                                               SkISize frame_size,
+                                               double device_pixel_ratio) {
+  // TODO(dkwingsmt): This class only supports rendering into the implicit view.
+  // Properly support multi-view in the future.


Do we have a tracking issue for this? I'm assuming this affects macOS, so we'll want to fix this as part of the desktop multi-view project.

(In theory we should have a tracking issue for IOSExternalViewEmbedder::PrepareView too but I don't feel as strongly about that one!)

Yeah I'll link flutter/flutter#135530 item 4 to it. (Although I'll submit the PR for embedder API that removes this TODO.)

loic-sharma

These changes LGTM but I'm not familiar enough with this area to approve. Please loop in reviewers from the engine team too

I meant to hit comment

…raster-multiview

dkwingsmt · 2023-11-21T01:10:56Z

shell/common/rasterizer.cc

        (!raster_thread_merger_ || raster_thread_merger_->IsMerged())) {
      FML_DCHECK(!frame->IsSubmitted());
-      external_view_embedder_->SubmitFrame(
+      external_view_embedder_->SubmitView(


Oh, I thought this frame was like the "frame" as in "144 frames per second".

cbracken · 2023-11-21T01:11:19Z

flow/embedded_views.cc

 }

-void ExternalViewEmbedder::SubmitFrame(
+void ExternalViewEmbedder::SubmitView(


I wonder if there's a clearer name for this that isn't overly long -- SubmitViewFrame for example?

I wonder how much we need to mention "frame" in this context, since of course a view is part of a frame, and the relationship is also explained in the lifecycle described in ExternalViewEmbedder's doc.

If we really need it, maybe FrameSubmitView is better, since it shows that the view is part of a frame. What do you think?

dkwingsmt · 2023-11-21T20:10:20Z

I also added a paragraph to ExternalViewEmbedder's doc defining the two view IDs.

cbracken

Overall this seems like a reasonable refactoring to decouple frame lifecycle from the individual surface work in terms of the approach that was proposed to this.

Please also request a review from @chinmaygarde or another relevant person from the core engine team for feedback.

dnfield · 2023-12-01T19:36:47Z

shell/platform/fuchsia/flutter/external_view_embedder.cc

-    double device_pixel_ratio,
    const fml::RefPtr<fml::RasterThreadMerger>& raster_thread_merger) {
  TRACE_EVENT0("flutter", "ExternalViewEmbedder::BeginFrame");
+}


Remove the trace event - it's pure overhead at this point, measuring an empty function.

dnfield · 2023-12-01T19:37:36Z

shell/platform/darwin/ios/ios_external_view_embedder.mm

-    double device_pixel_ratio,
    const fml::RefPtr<fml::RasterThreadMerger>& raster_thread_merger) {
  TRACE_EVENT0("flutter", "IOSExternalViewEmbedder::BeginFrame");
+}


Ditto - remove this trace event, this method should just be empty now.

dnfield

Please remove the tracing on empty method bodies, otherwise LGTM

…139507) flutter/engine@c6395d9...de0ba84 2023-12-04 skia-flutter-autoroll@skia.org Roll Skia from 0fe76d9ce79e to cbd2cf40d63b (1 revision) (flutter/engine#48638) 2023-12-04 dnfield@google.com Remove some trivial forward declares from Impeller (flutter/engine#48635) 2023-12-04 737941+loic-sharma@users.noreply.github.com [Windows] Decouple the GL context from the view (flutter/engine#48636) 2023-12-04 dkwingsmt@users.noreply.github.com Multiview ExternalViewEmbedder (flutter/engine#46169) 2023-12-04 skia-flutter-autoroll@skia.org Roll Dart SDK from 67f7a2c2559a to a1b67665b3a3 (1 revision) (flutter/engine#48637) 2023-12-04 skia-flutter-autoroll@skia.org Roll Skia from db4a29e0689e to 0fe76d9ce79e (5 revisions) (flutter/engine#48634) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC matanl@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

This PR connects the view ID provided by the rasterizer to `EmbedderExternalViewEmbedder`'s presenting call (see `embedder_external_view_embedder.cc`). This PR also contains a refactor (which actually takes the majority of LOC) that moves the specification of view ID from `PrepareFlutterView` to `SubmitFlutterView`. The `flutter_view_id` parameter in `PrepareFlutterView` was added in #46169. It turns out that we don't need the view ID throughout the preparation phase, so we can delay the specification to submission, which makes it feel cleaner. ### Tests Existing tests are changed to verify that `SubmitFlutterView` receive the correct view ID. The feature that `EmbedderExternalViewEmbedder` sends the view ID to the presenting API is not tested for now, because `EmbedderExternalViewEmbedder` is typically tested in `embedder_unittests`, but it requires the embedder API `AddView`. It will be tested in later changes to `EmbedderExternalViewEmbedder`. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

github-actions bot added platform-android platform-fuchsia platform-ios labels Sep 21, 2023

dkwingsmt force-pushed the mv-raster-multiview branch from 09a6e77 to e095f1f Compare November 2, 2023 07:22

dkwingsmt added 2 commits November 2, 2023 14:01

Cleanup impl

324cc1c

Format

c7d5022

dkwingsmt force-pushed the mv-raster-multiview branch from 08e8ff1 to c7d5022 Compare November 5, 2023 22:15

dkwingsmt added 3 commits November 6, 2023 10:59

Merge remote-tracking branch 'origin/main' into mv-raster-multiview

ec459ab

Fix lint

da8694e

Add doc

a715ed5

dkwingsmt changed the title ~~WIP: Multi-view rasterizer with full multi-view support~~ Multiview ExternalViewEmbedder Nov 6, 2023

dkwingsmt marked this pull request as ready for review November 6, 2023 22:36

dkwingsmt added 2 commits November 6, 2023 16:08

Add a test

f483d8c

Format

0d2be4e

dkwingsmt requested a review from loic-sharma November 7, 2023 00:22

cbracken self-requested a review November 8, 2023 00:30

loic-sharma reviewed Nov 8, 2023

View reviewed changes

flow/embedded_views.h Show resolved Hide resolved

loic-sharma reviewed Nov 8, 2023

View reviewed changes

loic-sharma previously approved these changes Nov 8, 2023

View reviewed changes

loic-sharma self-requested a review November 8, 2023 18:17

dkwingsmt and others added 8 commits November 9, 2023 00:07

Add TODO

89eceb1

Merge remote-tracking branch 'origin/main' into mv-raster-multiview

109cdea

Merge branch 'main' into mv-raster-multiview

90107b5

Merge remote-tracking branch 'origin/main' into mv-raster-multiview

0260384

Merge remote-tracking branch 'dkwingsmt/mv-raster-multiview' into mv-…

b6431b5

…raster-multiview

Compile

cadb2f3

fuchsia compile

cba9869

Fix fuchsia compile

17293b0

dkwingsmt commented Nov 21, 2023

View reviewed changes

cbracken reviewed Nov 21, 2023

View reviewed changes

dkwingsmt added 2 commits November 21, 2023 12:05

Explain the two IDs

94cff61

Prepare/SubmitFlutterView

068793b

dkwingsmt and others added 4 commits November 27, 2023 10:10

Merge remote-tracking branch 'origin/main' into mv-raster-multiview

18ee8b3

Fix lint error

de5f205

Fix fuchsia

559a043

Merge branch 'main' into mv-raster-multiview

86af1c4

cbracken approved these changes Nov 28, 2023

View reviewed changes

dkwingsmt requested a review from chinmaygarde November 28, 2023 01:07

Merge branch 'main' into mv-raster-multiview

dfeae47

jonahwilliams requested a review from dnfield November 30, 2023 21:03

dnfield reviewed Dec 1, 2023

View reviewed changes

dkwingsmt added 3 commits December 4, 2023 11:52

Merge remote-tracking branch 'origin/main' into mv-raster-multiview

973a85d

ignore deprecated member on hashvalues

5ad5661

Remove empty trace

830ad4c

github-actions bot added the platform-web Code specifically for the web engine label Dec 4, 2023

dkwingsmt added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 4, 2023

auto-submit bot merged commit e532e4b into flutter:main Dec 4, 2023

engine-flutter-autoroll mentioned this pull request Dec 4, 2023

Roll Flutter Engine from c6395d920786 to 3e93395d3cc4 (5 revisions) flutter/flutter#139504

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 4, 2023

e532e4b7a Multiview ExternalViewEmbedder (flutter/engine#46169)

ce3a27f

engine-flutter-autoroll mentioned this pull request Dec 4, 2023

Roll Flutter Engine from c6395d920786 to de0ba840339c (6 revisions) flutter/flutter#139507

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 4, 2023

e532e4b7a Multiview ExternalViewEmbedder (flutter/engine#46169)

c44946e

dkwingsmt mentioned this pull request Mar 27, 2024

Make EmbedderExternalViewEmbedder present to the correct view #51719

Merged

8 tasks

dkwingsmt mentioned this pull request Apr 4, 2024

Support multi-view rendering flutter/flutter#135530

Closed

5 tasks

Multiview ExternalViewEmbedder #46169

Multiview ExternalViewEmbedder #46169

Uh oh!

Conversation

dkwingsmt commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Nomenclature

Change

Pre-launch Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loic-sharma left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt commented Nov 21, 2023

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dkwingsmt commented Sep 21, 2023 •

edited

Loading

dkwingsmt Nov 9, 2023 •

edited

Loading

loic-sharma left a comment •

edited

Loading