From 5f0d84341bd929552100a7fd30fd847cf9eeffa0 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Thu, 27 Jan 2022 11:24:52 -0800 Subject: [PATCH 1/2] Revert "Revert "Teardown external view embedder prior to unmerging threads (#30893)" (#31108)" This reverts commit 55420ec8f49e4238d0c0728f62808563013340bc. --- .../external_view_embedder.cc | 25 +++++++-- .../external_view_embedder.h | 12 ++++- .../external_view_embedder_unittests.cc | 52 ++++++++++++------- .../platform/android/platform_view_android.cc | 10 ++-- 4 files changed, 68 insertions(+), 31 deletions(-) diff --git a/shell/platform/android/external_view_embedder/external_view_embedder.cc b/shell/platform/android/external_view_embedder/external_view_embedder.cc index 0c326eb950b03..84f2ab9378125 100644 --- a/shell/platform/android/external_view_embedder/external_view_embedder.cc +++ b/shell/platform/android/external_view_embedder/external_view_embedder.cc @@ -4,6 +4,8 @@ #include "flutter/shell/platform/android/external_view_embedder/external_view_embedder.h" +#include "flutter/fml/synchronization/waitable_event.h" +#include "flutter/fml/task_runner.h" #include "flutter/fml/trace_event.h" #include "flutter/shell/platform/android/surface/android_surface.h" @@ -12,12 +14,14 @@ namespace flutter { AndroidExternalViewEmbedder::AndroidExternalViewEmbedder( const AndroidContext& android_context, std::shared_ptr jni_facade, - std::shared_ptr surface_factory) + std::shared_ptr surface_factory, + TaskRunners task_runners) : ExternalViewEmbedder(), android_context_(android_context), jni_facade_(jni_facade), surface_factory_(surface_factory), - surface_pool_(std::make_unique()) {} + surface_pool_(std::make_unique()), + task_runners_(task_runners) {} // |ExternalViewEmbedder| void AndroidExternalViewEmbedder::PrerollCompositeEmbeddedView( @@ -264,8 +268,8 @@ void AndroidExternalViewEmbedder::BeginFrame( // The surface size changed. Therefore, destroy existing surfaces as // the existing surfaces in the pool can't be recycled. - if (frame_size_ != frame_size && raster_thread_merger->IsOnPlatformThread()) { - surface_pool_->DestroyLayers(jni_facade_); + if (frame_size_ != frame_size) { + DestroySurfaces(); } surface_pool_->SetFrameSize(frame_size); // JNI method must be called on the platform thread. @@ -300,7 +304,18 @@ bool AndroidExternalViewEmbedder::SupportsDynamicThreadMerging() { // |ExternalViewEmbedder| void AndroidExternalViewEmbedder::Teardown() { - surface_pool_->DestroyLayers(jni_facade_); + DestroySurfaces(); +} + +// |ExternalViewEmbedder| +void AndroidExternalViewEmbedder::DestroySurfaces() { + fml::AutoResetWaitableEvent latch; + fml::TaskRunner::RunNowOrPostTask(task_runners_.GetPlatformTaskRunner(), + [&]() { + surface_pool_->DestroyLayers(jni_facade_); + latch.Signal(); + }); + latch.Wait(); } } // namespace flutter diff --git a/shell/platform/android/external_view_embedder/external_view_embedder.h b/shell/platform/android/external_view_embedder/external_view_embedder.h index 278d7693e7661..2ec13297bb9ad 100644 --- a/shell/platform/android/external_view_embedder/external_view_embedder.h +++ b/shell/platform/android/external_view_embedder/external_view_embedder.h @@ -7,6 +7,7 @@ #include +#include "flutter/common/task_runners.h" #include "flutter/flow/embedded_views.h" #include "flutter/flow/rtree.h" #include "flutter/shell/platform/android/context/android_context.h" @@ -32,7 +33,8 @@ class AndroidExternalViewEmbedder final : public ExternalViewEmbedder { AndroidExternalViewEmbedder( const AndroidContext& android_context, std::shared_ptr jni_facade, - std::shared_ptr surface_factory); + std::shared_ptr surface_factory, + TaskRunners task_runners); // |ExternalViewEmbedder| void PrerollCompositeEmbeddedView( @@ -99,6 +101,9 @@ class AndroidExternalViewEmbedder final : public ExternalViewEmbedder { // Holds surfaces. Allows to recycle surfaces or allocate new ones. const std::unique_ptr surface_pool_; + // The task runners. + const TaskRunners task_runners_; + // The size of the root canvas. SkISize frame_size_; @@ -126,6 +131,11 @@ class AndroidExternalViewEmbedder final : public ExternalViewEmbedder { // The number of platform views in the previous frame. int64_t previous_frame_view_count_; + // Destroys the surfaces created from the surface factory. + // This method schedules a task on the platform thread, and waits for + // the task until it completes. + void DestroySurfaces(); + // Resets the state. void Reset(); diff --git a/shell/platform/android/external_view_embedder/external_view_embedder_unittests.cc b/shell/platform/android/external_view_embedder/external_view_embedder_unittests.cc index c5a3c19701a3f..57966440d0d05 100644 --- a/shell/platform/android/external_view_embedder/external_view_embedder_unittests.cc +++ b/shell/platform/android/external_view_embedder/external_view_embedder_unittests.cc @@ -2,6 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#define FML_USED_ON_EMBEDDER + #include #include "flutter/shell/platform/android/external_view_embedder/external_view_embedder.h" @@ -87,12 +89,24 @@ fml::RefPtr GetThreadMergerFromRasterThread( rasterizer_queue_id); } +TaskRunners GetTaskRunnersForFixture() { + fml::MessageLoop::EnsureInitializedForCurrentThread(); + auto& loop = fml::MessageLoop::GetCurrent(); + return { + "test", + loop.GetTaskRunner(), // platform + loop.GetTaskRunner(), // raster + loop.GetTaskRunner(), // ui + loop.GetTaskRunner() // io + }; +} + TEST(AndroidExternalViewEmbedder, GetCurrentCanvases) { auto jni_mock = std::make_shared(); auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware); auto embedder = std::make_unique( - android_context, jni_mock, nullptr); + android_context, jni_mock, nullptr, GetTaskRunnersForFixture()); fml::Thread rasterizer_thread("rasterizer"); auto raster_thread_merger = GetThreadMergerFromPlatformThread(&rasterizer_thread); @@ -117,7 +131,7 @@ TEST(AndroidExternalViewEmbedder, GetCurrentCanvasesCompositeOrder) { auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware); auto embedder = std::make_unique( - android_context, jni_mock, nullptr); + android_context, jni_mock, nullptr, GetTaskRunnersForFixture()); fml::Thread rasterizer_thread("rasterizer"); auto raster_thread_merger = GetThreadMergerFromPlatformThread(&rasterizer_thread); @@ -140,7 +154,7 @@ TEST(AndroidExternalViewEmbedder, GetCurrentCanvasesCompositeOrder) { TEST(AndroidExternalViewEmbedder, CompositeEmbeddedView) { auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware); auto embedder = std::make_unique( - android_context, nullptr, nullptr); + android_context, nullptr, nullptr, GetTaskRunnersForFixture()); ASSERT_EQ(nullptr, embedder->CompositeEmbeddedView(0)); embedder->PrerollCompositeEmbeddedView( @@ -156,7 +170,7 @@ TEST(AndroidExternalViewEmbedder, CompositeEmbeddedView) { TEST(AndroidExternalViewEmbedder, CancelFrame) { auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware); auto embedder = std::make_unique( - android_context, nullptr, nullptr); + android_context, nullptr, nullptr, GetTaskRunnersForFixture()); embedder->PrerollCompositeEmbeddedView( 0, std::make_unique()); @@ -170,7 +184,7 @@ TEST(AndroidExternalViewEmbedder, RasterizerRunsOnPlatformThread) { auto jni_mock = std::make_shared(); auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware); auto embedder = std::make_unique( - android_context, jni_mock, nullptr); + android_context, jni_mock, nullptr, GetTaskRunnersForFixture()); fml::Thread rasterizer_thread("rasterizer"); auto raster_thread_merger = @@ -204,7 +218,7 @@ TEST(AndroidExternalViewEmbedder, RasterizerRunsOnRasterizerThread) { auto jni_mock = std::make_shared(); auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware); auto embedder = std::make_unique( - android_context, jni_mock, nullptr); + android_context, jni_mock, nullptr, GetTaskRunnersForFixture()); fml::Thread rasterizer_thread("rasterizer"); auto raster_thread_merger = @@ -225,7 +239,7 @@ TEST(AndroidExternalViewEmbedder, PlatformViewRect) { auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware); auto embedder = std::make_unique( - android_context, jni_mock, nullptr); + android_context, jni_mock, nullptr, GetTaskRunnersForFixture()); fml::Thread rasterizer_thread("rasterizer"); auto raster_thread_merger = GetThreadMergerFromPlatformThread(&rasterizer_thread); @@ -253,7 +267,7 @@ TEST(AndroidExternalViewEmbedder, PlatformViewRectChangedParams) { auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware); auto embedder = std::make_unique( - android_context, jni_mock, nullptr); + android_context, jni_mock, nullptr, GetTaskRunnersForFixture()); fml::Thread rasterizer_thread("rasterizer"); auto raster_thread_merger = GetThreadMergerFromPlatformThread(&rasterizer_thread); @@ -328,7 +342,7 @@ TEST(AndroidExternalViewEmbedder, SubmitFrame) { return android_surface_mock; }); auto embedder = std::make_unique( - *android_context, jni_mock, surface_factory); + *android_context, jni_mock, surface_factory, GetTaskRunnersForFixture()); auto raster_thread_merger = GetThreadMergerFromPlatformThread(); @@ -521,7 +535,7 @@ TEST(AndroidExternalViewEmbedder, SubmitFrameOverlayComposition) { return android_surface_mock; }); auto embedder = std::make_unique( - *android_context, jni_mock, surface_factory); + *android_context, jni_mock, surface_factory, GetTaskRunnersForFixture()); auto raster_thread_merger = GetThreadMergerFromPlatformThread(); @@ -623,7 +637,7 @@ TEST(AndroidExternalViewEmbedder, SubmitFramePlatformViewWithoutAnyOverlay) { return android_surface_mock; }); auto embedder = std::make_unique( - *android_context, jni_mock, surface_factory); + *android_context, jni_mock, surface_factory, GetTaskRunnersForFixture()); auto raster_thread_merger = GetThreadMergerFromPlatformThread(); @@ -662,7 +676,7 @@ TEST(AndroidExternalViewEmbedder, DoesNotCallJNIPlatformThreadOnlyMethods) { auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware); auto embedder = std::make_unique( - android_context, jni_mock, nullptr); + android_context, jni_mock, nullptr, GetTaskRunnersForFixture()); // While on the raster thread, don't make JNI calls as these methods can only // run on the platform thread. @@ -711,7 +725,7 @@ TEST(AndroidExternalViewEmbedder, DestroyOverlayLayersOnSizeChange) { }); auto embedder = std::make_unique( - *android_context, jni_mock, surface_factory); + *android_context, jni_mock, surface_factory, GetTaskRunnersForFixture()); fml::Thread rasterizer_thread("rasterizer"); auto raster_thread_merger = GetThreadMergerFromPlatformThread(&rasterizer_thread); @@ -798,7 +812,7 @@ TEST(AndroidExternalViewEmbedder, DoesNotDestroyOverlayLayersOnSizeChange) { }); auto embedder = std::make_unique( - *android_context, jni_mock, surface_factory); + *android_context, jni_mock, surface_factory, GetTaskRunnersForFixture()); // ------------------ First frame ------------------ // { @@ -843,9 +857,7 @@ TEST(AndroidExternalViewEmbedder, DoesNotDestroyOverlayLayersOnSizeChange) { embedder->EndFrame(/*should_resubmit_frame=*/false, raster_thread_merger); } - // Changing the frame size from the raster thread does not make JNI calls. - - EXPECT_CALL(*jni_mock, FlutterViewDestroyOverlaySurfaces()).Times(0); + EXPECT_CALL(*jni_mock, FlutterViewDestroyOverlaySurfaces()).Times(1); EXPECT_CALL(*jni_mock, FlutterViewBeginFrame()).Times(0); fml::Thread platform_thread("platform"); @@ -857,7 +869,7 @@ TEST(AndroidExternalViewEmbedder, SupportsDynamicThreadMerging) { auto jni_mock = std::make_shared(); auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware); auto embedder = std::make_unique( - android_context, jni_mock, nullptr); + android_context, jni_mock, nullptr, GetTaskRunnersForFixture()); ASSERT_TRUE(embedder->SupportsDynamicThreadMerging()); } @@ -865,7 +877,7 @@ TEST(AndroidExternalViewEmbedder, DisableThreadMerger) { auto jni_mock = std::make_shared(); auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware); auto embedder = std::make_unique( - android_context, jni_mock, nullptr); + android_context, jni_mock, nullptr, GetTaskRunnersForFixture()); fml::Thread platform_thread("platform"); auto raster_thread_merger = GetThreadMergerFromRasterThread(&platform_thread); @@ -921,7 +933,7 @@ TEST(AndroidExternalViewEmbedder, Teardown) { }); auto embedder = std::make_unique( - *android_context, jni_mock, surface_factory); + *android_context, jni_mock, surface_factory, GetTaskRunnersForFixture()); fml::Thread rasterizer_thread("rasterizer"); auto raster_thread_merger = GetThreadMergerFromPlatformThread(&rasterizer_thread); diff --git a/shell/platform/android/platform_view_android.cc b/shell/platform/android/platform_view_android.cc index c3d4c696628f4..97c58de4c3298 100644 --- a/shell/platform/android/platform_view_android.cc +++ b/shell/platform/android/platform_view_android.cc @@ -14,13 +14,12 @@ #include "flutter/shell/platform/android/android_external_texture_gl.h" #include "flutter/shell/platform/android/android_surface_gl.h" #include "flutter/shell/platform/android/android_surface_software.h" -#include "flutter/shell/platform/android/external_view_embedder/external_view_embedder.h" -#include "flutter/shell/platform/android/surface/android_surface.h" -#include "flutter/shell/platform/android/surface/snapshot_surface_producer.h" - #include "flutter/shell/platform/android/context/android_context.h" +#include "flutter/shell/platform/android/external_view_embedder/external_view_embedder.h" #include "flutter/shell/platform/android/jni/platform_view_android_jni.h" #include "flutter/shell/platform/android/platform_message_response_android.h" +#include "flutter/shell/platform/android/surface/android_surface.h" +#include "flutter/shell/platform/android/surface/snapshot_surface_producer.h" #include "flutter/shell/platform/android/vsync_waiter_android.h" namespace flutter { @@ -255,7 +254,8 @@ std::unique_ptr PlatformViewAndroid::CreateRenderingSurface() { std::shared_ptr PlatformViewAndroid::CreateExternalViewEmbedder() { return std::make_shared( - *android_context_, jni_facade_, surface_factory_); + *android_context_, jni_facade_, surface_factory_, + std::move(task_runners_)); } // |PlatformView| From a57f0792a04d8afc2071f8766506e5ecb42c65fe Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Thu, 27 Jan 2022 16:15:21 -0800 Subject: [PATCH 2/2] Fix deadlock --- shell/common/rasterizer.cc | 10 ++++++---- shell/common/rasterizer.h | 7 +++++++ shell/common/shell.cc | 5 +++++ shell/platform/android/platform_view_android.cc | 3 +-- 4 files changed, 19 insertions(+), 6 deletions(-) diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 2fec73ab23793..dc22eb83f4903 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -81,6 +81,12 @@ void Rasterizer::Setup(std::unique_ptr surface) { } } +void Rasterizer::TeardownExternalViewEmbedder() { + if (external_view_embedder_) { + external_view_embedder_->Teardown(); + } +} + void Rasterizer::Teardown() { auto context_switch = surface_ ? surface_->MakeRenderContextCurrent() : nullptr; @@ -97,10 +103,6 @@ void Rasterizer::Teardown() { raster_thread_merger_->UnMergeNowIfLastOne(); raster_thread_merger_->SetMergeUnmergeCallback(nullptr); } - - if (external_view_embedder_) { - external_view_embedder_->Teardown(); - } } void Rasterizer::EnableThreadMergerIfNeeded() { diff --git a/shell/common/rasterizer.h b/shell/common/rasterizer.h index fb381cc0f9af0..8f0959d8390a3 100644 --- a/shell/common/rasterizer.h +++ b/shell/common/rasterizer.h @@ -140,6 +140,13 @@ class Rasterizer final : public SnapshotDelegate { /// void Teardown(); + //---------------------------------------------------------------------------- + /// @brief Releases any resource used by the external view embedder. + /// For example, overlay surfaces or Android views. + /// On Android, this method post a task to the platform thread, + /// and waits until it completes. + void TeardownExternalViewEmbedder(); + //---------------------------------------------------------------------------- /// @brief Notifies the rasterizer that there is a low memory situation /// and it must purge as many unnecessary resources as possible. diff --git a/shell/common/shell.cc b/shell/common/shell.cc index bc664f34ac605..8cb3dbf613403 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -885,6 +885,11 @@ void Shell::OnPlatformViewDestroyed() { fml::TaskRunner::RunNowOrPostTask(task_runners_.GetRasterTaskRunner(), raster_task); latch.Wait(); + // On Android, the external view embedder posts a task to the platform thread, + // and waits until it completes. + // As a result, the platform thread must not be blocked prior to calling + // this method. + rasterizer_->TeardownExternalViewEmbedder(); } // |PlatformView::Delegate| diff --git a/shell/platform/android/platform_view_android.cc b/shell/platform/android/platform_view_android.cc index 97c58de4c3298..f259c73b49525 100644 --- a/shell/platform/android/platform_view_android.cc +++ b/shell/platform/android/platform_view_android.cc @@ -254,8 +254,7 @@ std::unique_ptr PlatformViewAndroid::CreateRenderingSurface() { std::shared_ptr PlatformViewAndroid::CreateExternalViewEmbedder() { return std::make_shared( - *android_context_, jni_facade_, surface_factory_, - std::move(task_runners_)); + *android_context_, jni_facade_, surface_factory_, task_runners_); } // |PlatformView|