Platform views have CreateExternalViewEmbedder #22214

iskakaushik · 2020-10-29T23:54:26Z

This is the last change before we can remove GetExternalViewEmbedder from the Surface APIs.

This furthers the effort to share the surfaces between macOS and iOS.

flutter-dashboard · 2020-10-29T23:54:28Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

chinmaygarde

I'd prefer we stayed away from shared pointers unless there are components that are shared across threads. Shared ownership of the object makes its lifetime hard to ascertain and the object has to make sure we its destruction is safe on any thread. We usually don't have components that do that.

In this case, it looks like the external view embedder is only used on the raster thread. So maybe the platform call to create the external view embedder can return a shared pointer instead.

On a related note, the CreateExternalViewEmbedder method makes it seem like a new instance will be allocated, but platform views are free to return a reference to an existing instance. That confused me a bit. Not sure what else to call it though. Maybe just document this? If you avoid shared pointers, this problem goes away too.

iskakaushik · 2020-10-30T21:37:34Z

So maybe the platform call to create the external view embedder can return a shared pointer instead.

Did you mean a unique_ptr instead? My plan is to go to a world where we have only one instance of the external view embedder. And that is held by the rasterizer and briefly passed to the compositor context as a raw pointer to acquire frame, etc.

In the current world it's still shared with the surface. Though they are still accessed on the raster thread. But as I mentioned earlier, this will soon be simplified.

iskakaushik · 2020-10-30T21:39:41Z

Also, create is a misleading name. I agree, I initially wanted to call this GetExternalViewEmbedder as all the platforms typically have this created and just return an instance. There were name conflicts where some of the platform views were themselves extending the GpuSurfaceDelegate interface and lead to name conflicts.

iskakaushik · 2020-11-03T14:09:50Z

@chinmaygarde friendly ping on this.

gaaclarke

Looks good for the most part my 2 big concerns are:

The added method says CreateExternalViewEmbedder but none of the implementations actually create anything.
It's worth seeing if we can make the external view embedder a constructor argument to the rasterizer. I'm not sure it's easily possible, please take a look.

gaaclarke · 2020-11-05T18:07:36Z

shell/common/rasterizer.cc

-  if (surface_ != nullptr && surface_->GetExternalViewEmbedder() != nullptr) {
-    surface_->GetExternalViewEmbedder()->EndFrame(should_resubmit_frame,
-                                                  raster_thread_merger_);
+  if (surface_ && external_view_embedder_) {


Do we still want to predicate this on surface_? Looks like it should work without one.

gaaclarke · 2020-11-05T18:09:08Z

shell/common/rasterizer.cc

+void Rasterizer::SetExternalViewEmbedder(
+    const std::shared_ptr<ExternalViewEmbedder>& view_embedder) {
+  external_view_embedder_ = view_embedder;
+}
+


Could we make the external_view_embedder_ a parameter to the constructor instead of setting it later? That would be safer.

gaaclarke · 2020-11-05T18:11:42Z

shell/platform/android/platform_view_android.cc

+// |PlatformView|
+std::shared_ptr<ExternalViewEmbedder>
+PlatformViewAndroid::CreateExternalViewEmbedder() {
+  return external_view_embedder_;
+}
+


The name of this method is CreateExternalViewEmbedder, but this method is not creating one, it's just returning one that is already created. We should change the name of the method, or change the implementation.

@gaaclarke I agree with this, please look at #22214 (comment). I will rename this method to GetExternalViewEmbedder once #22272 lands.

Edit: I agree with your #22214 (comment). Once we don't have to share the external view embedder with surfaces, I will make this a unique_ptr and make rasterizer the sole owner. CreateExternalViewEmbedder makes sense in that world for sure!

gaaclarke · 2020-11-05T18:17:52Z

If you avoid shared pointers, this problem goes away too.

I just read Chinmay's feedback. It seems like the sole owner for the external view embedder should be the rasterizer, then you can keep the semantics of CreateExternalViewEmbedder. That sounds like a better solution and eliminates the shared_ptr.

iskakaushik · 2020-11-05T18:54:51Z

@gaaclarke I've looked through passing ExternalViewEmbedder (EVE) as a ctor param for the rasterizer. Across most of the platforms EVE is owned as a shared_ptr by the platform view. There needs to be a refactor that moves the creation to engine or the equivalent and passed to on_create_rasterizer.

That being said, I do agree that rasterizer being the sole-owner for EVE is a world that we should go towards, and I will get to it as part of: flutter/flutter#69905

gaaclarke

LGTM with the promise that the shared_ptr will get removed and the method CreateExternalViewController will be reconsidered after the other PR lands.

gaaclarke · 2020-11-05T19:11:45Z

I wish github did dependent PRs better so we could approve all your steps before they land. It makes me a bit nervous landing code that we know we want to change right away.

fluttergithubbot · 2020-11-05T19:59:02Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite build_and_test_linux_unopt_debug has failed. Please fix the issues identified (or deflake) before re-applying this label.

chinmaygarde · 2020-11-05T22:37:41Z

This might need to be rebased to make the failing presubmit pass.

This is the last change before we can remove GetExternalViewEmbedder from the Surface APIs. This furthers the effort to share the surfaces between macOS and iOS.

…e#22214)

iskakaushik requested a review from chinmaygarde October 29, 2020 23:54

flutter-dashboard bot added platform-android platform-ios labels Oct 29, 2020

googlebot added the cla: yes label Oct 29, 2020

iskakaushik force-pushed the pv_c_ext_embe-2 branch 2 times, most recently from 37247cc to b16d861 Compare October 30, 2020 00:06

iskakaushik changed the title ~~Platform views have GetExternalViewEmbedder~~ Platform views have CreateExternalViewEmbedder Oct 30, 2020

iskakaushik force-pushed the pv_c_ext_embe-2 branch from d05ba89 to 47871c3 Compare October 30, 2020 17:27

chinmaygarde reviewed Oct 30, 2020

View reviewed changes

iskakaushik mentioned this pull request Nov 3, 2020

Remove GetExternalViewEmbedder from surface #22272

Merged

iskakaushik requested a review from gaaclarke November 5, 2020 15:50

gaaclarke suggested changes Nov 5, 2020

View reviewed changes

iskakaushik mentioned this pull request Nov 5, 2020

ExternalViewEmbedder should be solely owned by Rasterizer flutter/flutter#69905

Open

gaaclarke approved these changes Nov 5, 2020

View reviewed changes

iskakaushik added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Nov 5, 2020

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Nov 5, 2020

Platform views have GetExternalViewEmbedder

731180a

This is the last change before we can remove GetExternalViewEmbedder from the Surface APIs. This furthers the effort to share the surfaces between macOS and iOS.

iskakaushik force-pushed the pv_c_ext_embe-2 branch from 8dbd0f4 to 731180a Compare November 6, 2020 12:48

iskakaushik added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Nov 6, 2020

iskakaushik merged commit 6e95e00 into flutter:master Nov 6, 2020

iskakaushik deleted the pv_c_ext_embe-2 branch November 6, 2020 13:30

engine-flutter-autoroll mentioned this pull request Nov 6, 2020

Roll Engine from 0693ee04d130 to 6e95e005f1b2 (21 revisions) flutter/flutter#69962

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 6, 2020

6e95e00 Platform views have CreateExternalViewEmbedder (flutter/engin…

8ea2b0b

…e#22214)

engine-flutter-autoroll mentioned this pull request Nov 6, 2020

Roll Engine from 0693ee04d130 to 40408629770c (23 revisions) flutter/flutter#69965

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 6, 2020

6e95e00 Platform views have CreateExternalViewEmbedder (flutter/engin…

193eaa3

…e#22214)

iskakaushik mentioned this pull request Nov 6, 2020

Rasterizer is initialized with an external view embedder #22348

Merged

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

Platform views have CreateExternalViewEmbedder (flutter#22214)

93f9ef4

Platform views have CreateExternalViewEmbedder #22214

Platform views have CreateExternalViewEmbedder #22214

Uh oh!

Conversation

iskakaushik commented Oct 29, 2020

Uh oh!

flutter-dashboard bot commented Oct 29, 2020

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

iskakaushik commented Oct 30, 2020

Uh oh!

iskakaushik commented Oct 30, 2020

Uh oh!

iskakaushik commented Nov 3, 2020

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

iskakaushik Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

gaaclarke Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

gaaclarke Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

iskakaushik Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

iskakaushik Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Nov 5, 2020

Uh oh!

iskakaushik commented Nov 5, 2020

Uh oh!

gaaclarke 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!

gaaclarke commented Nov 5, 2020

Uh oh!

fluttergithubbot commented Nov 5, 2020

Uh oh!

chinmaygarde commented Nov 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gaaclarke left a comment •

edited

Loading