Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Commit 7b5edec

Browse files
author
Emmanuel Garcia
authored
Reland: Teardown external view embedder prior to unmerging threads (#31122)
1 parent 4dd7eb7 commit 7b5edec

File tree

7 files changed

+85
-35
lines changed

7 files changed

+85
-35
lines changed

‎shell/common/rasterizer.cc‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,12 @@ void Rasterizer::Setup(std::unique_ptr<Surface> surface) {
7979
}
8080
}
8181

82+
void Rasterizer::TeardownExternalViewEmbedder() {
83+
if (external_view_embedder_) {
84+
external_view_embedder_->Teardown();
85+
}
86+
}
87+
8288
void Rasterizer::Teardown() {
8389
auto context_switch =
8490
surface_ ? surface_->MakeRenderContextCurrent() : nullptr;
@@ -95,10 +101,6 @@ void Rasterizer::Teardown() {
95101
raster_thread_merger_->UnMergeNowIfLastOne();
96102
raster_thread_merger_->SetMergeUnmergeCallback(nullptr);
97103
}
98-
99-
if (external_view_embedder_) {
100-
external_view_embedder_->Teardown();
101-
}
102104
}
103105

104106
void Rasterizer::EnableThreadMergerIfNeeded() {

‎shell/common/rasterizer.h‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,13 @@ class Rasterizer final : public SnapshotDelegate,
141141
///
142142
void Teardown();
143143

144+
//----------------------------------------------------------------------------
145+
/// @brief Releases any resource used by the external view embedder.
146+
/// For example, overlay surfaces or Android views.
147+
/// On Android, this method post a task to the platform thread,
148+
/// and waits until it completes.
149+
void TeardownExternalViewEmbedder();
150+
144151
//----------------------------------------------------------------------------
145152
/// @brief Notifies the rasterizer that there is a low memory situation
146153
/// and it must purge as many unnecessary resources as possible.

‎shell/common/shell.cc‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -869,6 +869,11 @@ void Shell::OnPlatformViewDestroyed() {
869869
fml::TaskRunner::RunNowOrPostTask(task_runners_.GetRasterTaskRunner(),
870870
raster_task);
871871
latch.Wait();
872+
// On Android, the external view embedder posts a task to the platform thread,
873+
// and waits until it completes.
874+
// As a result, the platform thread must not be blocked prior to calling
875+
// this method.
876+
rasterizer_->TeardownExternalViewEmbedder();
872877
}
873878

874879
// |PlatformView::Delegate|

‎shell/platform/android/external_view_embedder/external_view_embedder.cc‎

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44

55
#include "flutter/shell/platform/android/external_view_embedder/external_view_embedder.h"
66

7+
#include "flutter/fml/synchronization/waitable_event.h"
8+
#include "flutter/fml/task_runner.h"
79
#include "flutter/fml/trace_event.h"
810
#include "flutter/shell/platform/android/surface/android_surface.h"
911

@@ -12,12 +14,14 @@ namespace flutter {
1214
AndroidExternalViewEmbedder::AndroidExternalViewEmbedder(
1315
const AndroidContext& android_context,
1416
std::shared_ptr<PlatformViewAndroidJNI> jni_facade,
15-
std::shared_ptr<AndroidSurfaceFactory> surface_factory)
17+
std::shared_ptr<AndroidSurfaceFactory> surface_factory,
18+
TaskRunners task_runners)
1619
: ExternalViewEmbedder(),
1720
android_context_(android_context),
1821
jni_facade_(jni_facade),
1922
surface_factory_(surface_factory),
20-
surface_pool_(std::make_unique<SurfacePool>()) {}
23+
surface_pool_(std::make_unique<SurfacePool>()),
24+
task_runners_(task_runners) {}
2125

2226
// |ExternalViewEmbedder|
2327
void AndroidExternalViewEmbedder::PrerollCompositeEmbeddedView(
@@ -264,8 +268,8 @@ void AndroidExternalViewEmbedder::BeginFrame(
264268

265269
// The surface size changed. Therefore, destroy existing surfaces as
266270
// the existing surfaces in the pool can't be recycled.
267-
if (frame_size_ != frame_size && raster_thread_merger->IsOnPlatformThread()) {
268-
surface_pool_->DestroyLayers(jni_facade_);
271+
if (frame_size_ != frame_size) {
272+
DestroySurfaces();
269273
}
270274
surface_pool_->SetFrameSize(frame_size);
271275
// JNI method must be called on the platform thread.
@@ -300,7 +304,18 @@ bool AndroidExternalViewEmbedder::SupportsDynamicThreadMerging() {
300304

301305
// |ExternalViewEmbedder|
302306
void AndroidExternalViewEmbedder::Teardown() {
303-
surface_pool_->DestroyLayers(jni_facade_);
307+
DestroySurfaces();
308+
}
309+
310+
// |ExternalViewEmbedder|
311+
void AndroidExternalViewEmbedder::DestroySurfaces() {
312+
fml::AutoResetWaitableEvent latch;
313+
fml::TaskRunner::RunNowOrPostTask(task_runners_.GetPlatformTaskRunner(),
314+
[&]() {
315+
surface_pool_->DestroyLayers(jni_facade_);
316+
latch.Signal();
317+
});
318+
latch.Wait();
304319
}
305320

306321
} // namespace flutter

‎shell/platform/android/external_view_embedder/external_view_embedder.h‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
#include <unordered_map>
99

10+
#include "flutter/common/task_runners.h"
1011
#include "flutter/flow/embedded_views.h"
1112
#include "flutter/flow/rtree.h"
1213
#include "flutter/shell/platform/android/context/android_context.h"
@@ -32,7 +33,8 @@ class AndroidExternalViewEmbedder final : public ExternalViewEmbedder {
3233
AndroidExternalViewEmbedder(
3334
const AndroidContext& android_context,
3435
std::shared_ptr<PlatformViewAndroidJNI> jni_facade,
35-
std::shared_ptr<AndroidSurfaceFactory> surface_factory);
36+
std::shared_ptr<AndroidSurfaceFactory> surface_factory,
37+
TaskRunners task_runners);
3638

3739
// |ExternalViewEmbedder|
3840
void PrerollCompositeEmbeddedView(
@@ -99,6 +101,9 @@ class AndroidExternalViewEmbedder final : public ExternalViewEmbedder {
99101
// Holds surfaces. Allows to recycle surfaces or allocate new ones.
100102
const std::unique_ptr<SurfacePool> surface_pool_;
101103

104+
// The task runners.
105+
const TaskRunners task_runners_;
106+
102107
// The size of the root canvas.
103108
SkISize frame_size_;
104109

@@ -126,6 +131,11 @@ class AndroidExternalViewEmbedder final : public ExternalViewEmbedder {
126131
// The number of platform views in the previous frame.
127132
int64_t previous_frame_view_count_;
128133

134+
// Destroys the surfaces created from the surface factory.
135+
// This method schedules a task on the platform thread, and waits for
136+
// the task until it completes.
137+
void DestroySurfaces();
138+
129139
// Resets the state.
130140
void Reset();
131141

‎shell/platform/android/external_view_embedder/external_view_embedder_unittests.cc‎

Lines changed: 32 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22
// Use of this source code is governed by a BSD-style license that can be
33
// found in the LICENSE file.
44

5+
#define FML_USED_ON_EMBEDDER
6+
57
#include <memory>
68
#include "flutter/shell/platform/android/external_view_embedder/external_view_embedder.h"
79

@@ -87,12 +89,24 @@ fml::RefPtr<fml::RasterThreadMerger> GetThreadMergerFromRasterThread(
8789
rasterizer_queue_id);
8890
}
8991

92+
TaskRunners GetTaskRunnersForFixture() {
93+
fml::MessageLoop::EnsureInitializedForCurrentThread();
94+
auto& loop = fml::MessageLoop::GetCurrent();
95+
return {
96+
"test",
97+
loop.GetTaskRunner(), // platform
98+
loop.GetTaskRunner(), // raster
99+
loop.GetTaskRunner(), // ui
100+
loop.GetTaskRunner() // io
101+
};
102+
}
103+
90104
TEST(AndroidExternalViewEmbedder, GetCurrentCanvases) {
91105
auto jni_mock = std::make_shared<JNIMock>();
92106

93107
auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware);
94108
auto embedder = std::make_unique<AndroidExternalViewEmbedder>(
95-
android_context, jni_mock, nullptr);
109+
android_context, jni_mock, nullptr, GetTaskRunnersForFixture());
96110
fml::Thread rasterizer_thread("rasterizer");
97111
auto raster_thread_merger =
98112
GetThreadMergerFromPlatformThread(&rasterizer_thread);
@@ -117,7 +131,7 @@ TEST(AndroidExternalViewEmbedder, GetCurrentCanvasesCompositeOrder) {
117131

118132
auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware);
119133
auto embedder = std::make_unique<AndroidExternalViewEmbedder>(
120-
android_context, jni_mock, nullptr);
134+
android_context, jni_mock, nullptr, GetTaskRunnersForFixture());
121135
fml::Thread rasterizer_thread("rasterizer");
122136
auto raster_thread_merger =
123137
GetThreadMergerFromPlatformThread(&rasterizer_thread);
@@ -140,7 +154,7 @@ TEST(AndroidExternalViewEmbedder, GetCurrentCanvasesCompositeOrder) {
140154
TEST(AndroidExternalViewEmbedder, CompositeEmbeddedView) {
141155
auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware);
142156
auto embedder = std::make_unique<AndroidExternalViewEmbedder>(
143-
android_context, nullptr, nullptr);
157+
android_context, nullptr, nullptr, GetTaskRunnersForFixture());
144158

145159
ASSERT_EQ(nullptr, embedder->CompositeEmbeddedView(0));
146160
embedder->PrerollCompositeEmbeddedView(
@@ -156,7 +170,7 @@ TEST(AndroidExternalViewEmbedder, CompositeEmbeddedView) {
156170
TEST(AndroidExternalViewEmbedder, CancelFrame) {
157171
auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware);
158172
auto embedder = std::make_unique<AndroidExternalViewEmbedder>(
159-
android_context, nullptr, nullptr);
173+
android_context, nullptr, nullptr, GetTaskRunnersForFixture());
160174

161175
embedder->PrerollCompositeEmbeddedView(
162176
0, std::make_unique<EmbeddedViewParams>());
@@ -170,7 +184,7 @@ TEST(AndroidExternalViewEmbedder, RasterizerRunsOnPlatformThread) {
170184
auto jni_mock = std::make_shared<JNIMock>();
171185
auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware);
172186
auto embedder = std::make_unique<AndroidExternalViewEmbedder>(
173-
android_context, jni_mock, nullptr);
187+
android_context, jni_mock, nullptr, GetTaskRunnersForFixture());
174188

175189
fml::Thread rasterizer_thread("rasterizer");
176190
auto raster_thread_merger =
@@ -204,7 +218,7 @@ TEST(AndroidExternalViewEmbedder, RasterizerRunsOnRasterizerThread) {
204218
auto jni_mock = std::make_shared<JNIMock>();
205219
auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware);
206220
auto embedder = std::make_unique<AndroidExternalViewEmbedder>(
207-
android_context, jni_mock, nullptr);
221+
android_context, jni_mock, nullptr, GetTaskRunnersForFixture());
208222

209223
fml::Thread rasterizer_thread("rasterizer");
210224
auto raster_thread_merger =
@@ -225,7 +239,7 @@ TEST(AndroidExternalViewEmbedder, PlatformViewRect) {
225239

226240
auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware);
227241
auto embedder = std::make_unique<AndroidExternalViewEmbedder>(
228-
android_context, jni_mock, nullptr);
242+
android_context, jni_mock, nullptr, GetTaskRunnersForFixture());
229243
fml::Thread rasterizer_thread("rasterizer");
230244
auto raster_thread_merger =
231245
GetThreadMergerFromPlatformThread(&rasterizer_thread);
@@ -253,7 +267,7 @@ TEST(AndroidExternalViewEmbedder, PlatformViewRectChangedParams) {
253267

254268
auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware);
255269
auto embedder = std::make_unique<AndroidExternalViewEmbedder>(
256-
android_context, jni_mock, nullptr);
270+
android_context, jni_mock, nullptr, GetTaskRunnersForFixture());
257271
fml::Thread rasterizer_thread("rasterizer");
258272
auto raster_thread_merger =
259273
GetThreadMergerFromPlatformThread(&rasterizer_thread);
@@ -328,7 +342,7 @@ TEST(AndroidExternalViewEmbedder, SubmitFrame) {
328342
return android_surface_mock;
329343
});
330344
auto embedder = std::make_unique<AndroidExternalViewEmbedder>(
331-
*android_context, jni_mock, surface_factory);
345+
*android_context, jni_mock, surface_factory, GetTaskRunnersForFixture());
332346

333347
auto raster_thread_merger = GetThreadMergerFromPlatformThread();
334348

@@ -521,7 +535,7 @@ TEST(AndroidExternalViewEmbedder, SubmitFrameOverlayComposition) {
521535
return android_surface_mock;
522536
});
523537
auto embedder = std::make_unique<AndroidExternalViewEmbedder>(
524-
*android_context, jni_mock, surface_factory);
538+
*android_context, jni_mock, surface_factory, GetTaskRunnersForFixture());
525539

526540
auto raster_thread_merger = GetThreadMergerFromPlatformThread();
527541

@@ -623,7 +637,7 @@ TEST(AndroidExternalViewEmbedder, SubmitFramePlatformViewWithoutAnyOverlay) {
623637
return android_surface_mock;
624638
});
625639
auto embedder = std::make_unique<AndroidExternalViewEmbedder>(
626-
*android_context, jni_mock, surface_factory);
640+
*android_context, jni_mock, surface_factory, GetTaskRunnersForFixture());
627641

628642
auto raster_thread_merger = GetThreadMergerFromPlatformThread();
629643

@@ -662,7 +676,7 @@ TEST(AndroidExternalViewEmbedder, DoesNotCallJNIPlatformThreadOnlyMethods) {
662676

663677
auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware);
664678
auto embedder = std::make_unique<AndroidExternalViewEmbedder>(
665-
android_context, jni_mock, nullptr);
679+
android_context, jni_mock, nullptr, GetTaskRunnersForFixture());
666680

667681
// While on the raster thread, don't make JNI calls as these methods can only
668682
// run on the platform thread.
@@ -711,7 +725,7 @@ TEST(AndroidExternalViewEmbedder, DestroyOverlayLayersOnSizeChange) {
711725
});
712726

713727
auto embedder = std::make_unique<AndroidExternalViewEmbedder>(
714-
*android_context, jni_mock, surface_factory);
728+
*android_context, jni_mock, surface_factory, GetTaskRunnersForFixture());
715729
fml::Thread rasterizer_thread("rasterizer");
716730
auto raster_thread_merger =
717731
GetThreadMergerFromPlatformThread(&rasterizer_thread);
@@ -798,7 +812,7 @@ TEST(AndroidExternalViewEmbedder, DoesNotDestroyOverlayLayersOnSizeChange) {
798812
});
799813

800814
auto embedder = std::make_unique<AndroidExternalViewEmbedder>(
801-
*android_context, jni_mock, surface_factory);
815+
*android_context, jni_mock, surface_factory, GetTaskRunnersForFixture());
802816

803817
// ------------------ First frame ------------------ //
804818
{
@@ -843,9 +857,7 @@ TEST(AndroidExternalViewEmbedder, DoesNotDestroyOverlayLayersOnSizeChange) {
843857
embedder->EndFrame(/*should_resubmit_frame=*/false, raster_thread_merger);
844858
}
845859

846-
// Changing the frame size from the raster thread does not make JNI calls.
847-
848-
EXPECT_CALL(*jni_mock, FlutterViewDestroyOverlaySurfaces()).Times(0);
860+
EXPECT_CALL(*jni_mock, FlutterViewDestroyOverlaySurfaces()).Times(1);
849861
EXPECT_CALL(*jni_mock, FlutterViewBeginFrame()).Times(0);
850862

851863
fml::Thread platform_thread("platform");
@@ -857,15 +869,15 @@ TEST(AndroidExternalViewEmbedder, SupportsDynamicThreadMerging) {
857869
auto jni_mock = std::make_shared<JNIMock>();
858870
auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware);
859871
auto embedder = std::make_unique<AndroidExternalViewEmbedder>(
860-
android_context, jni_mock, nullptr);
872+
android_context, jni_mock, nullptr, GetTaskRunnersForFixture());
861873
ASSERT_TRUE(embedder->SupportsDynamicThreadMerging());
862874
}
863875

864876
TEST(AndroidExternalViewEmbedder, DisableThreadMerger) {
865877
auto jni_mock = std::make_shared<JNIMock>();
866878
auto android_context = AndroidContext(AndroidRenderingAPI::kSoftware);
867879
auto embedder = std::make_unique<AndroidExternalViewEmbedder>(
868-
android_context, jni_mock, nullptr);
880+
android_context, jni_mock, nullptr, GetTaskRunnersForFixture());
869881

870882
fml::Thread platform_thread("platform");
871883
auto raster_thread_merger = GetThreadMergerFromRasterThread(&platform_thread);
@@ -921,7 +933,7 @@ TEST(AndroidExternalViewEmbedder, Teardown) {
921933
});
922934

923935
auto embedder = std::make_unique<AndroidExternalViewEmbedder>(
924-
*android_context, jni_mock, surface_factory);
936+
*android_context, jni_mock, surface_factory, GetTaskRunnersForFixture());
925937
fml::Thread rasterizer_thread("rasterizer");
926938
auto raster_thread_merger =
927939
GetThreadMergerFromPlatformThread(&rasterizer_thread);

‎shell/platform/android/platform_view_android.cc‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,12 @@
1414
#include "flutter/shell/platform/android/android_external_texture_gl.h"
1515
#include "flutter/shell/platform/android/android_surface_gl.h"
1616
#include "flutter/shell/platform/android/android_surface_software.h"
17-
#include "flutter/shell/platform/android/external_view_embedder/external_view_embedder.h"
18-
#include "flutter/shell/platform/android/surface/android_surface.h"
19-
#include "flutter/shell/platform/android/surface/snapshot_surface_producer.h"
20-
2117
#include "flutter/shell/platform/android/context/android_context.h"
18+
#include "flutter/shell/platform/android/external_view_embedder/external_view_embedder.h"
2219
#include "flutter/shell/platform/android/jni/platform_view_android_jni.h"
2320
#include "flutter/shell/platform/android/platform_message_response_android.h"
21+
#include "flutter/shell/platform/android/surface/android_surface.h"
22+
#include "flutter/shell/platform/android/surface/snapshot_surface_producer.h"
2423
#include "flutter/shell/platform/android/vsync_waiter_android.h"
2524

2625
namespace flutter {
@@ -255,7 +254,7 @@ std::unique_ptr<Surface> PlatformViewAndroid::CreateRenderingSurface() {
255254
std::shared_ptr<ExternalViewEmbedder>
256255
PlatformViewAndroid::CreateExternalViewEmbedder() {
257256
return std::make_shared<AndroidExternalViewEmbedder>(
258-
*android_context_, jni_facade_, surface_factory_);
257+
*android_context_, jni_facade_, surface_factory_, task_runners_);
259258
}
260259

261260
// |PlatformView|

0 commit comments

Comments
 (0)