From 910b7ed0dfffc5b0e355231fc1165904fd71a432 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Wed, 24 Jun 2020 14:02:48 -0700 Subject: [PATCH] EndFrame should be always called by rasterizer (flutter/engine#19257) --- engine/src/flutter/flow/embedded_views.h | 6 +++--- engine/src/flutter/shell/common/rasterizer.cc | 10 ++++------ .../shell/common/shell_test_external_view_embedder.cc | 3 ++- .../shell/common/shell_test_external_view_embedder.h | 3 ++- engine/src/flutter/shell/common/shell_unittests.cc | 3 ++- .../external_view_embedder/external_view_embedder.cc | 3 ++- .../external_view_embedder/external_view_embedder.h | 1 + .../external_view_embedder_unittests.cc | 10 +++++----- .../ios/framework/Source/FlutterPlatformViews.mm | 3 ++- .../framework/Source/FlutterPlatformViews_Internal.h | 3 ++- .../flutter/shell/platform/darwin/ios/ios_surface.h | 3 ++- .../flutter/shell/platform/darwin/ios/ios_surface.mm | 5 +++-- 12 files changed, 30 insertions(+), 23 deletions(-) diff --git a/engine/src/flutter/flow/embedded_views.h b/engine/src/flutter/flow/embedded_views.h index 3493f02be74..ddc2153ec6e 100644 --- a/engine/src/flutter/flow/embedded_views.h +++ b/engine/src/flutter/flow/embedded_views.h @@ -287,16 +287,16 @@ class ExternalViewEmbedder { virtual bool SubmitFrame(GrContext* context, std::unique_ptr frame); - // This should only be called after |SubmitFrame|. // This method provides the embedder a way to do additional tasks after - // |SubmitFrame|. After invoking this method, the current task on the - // TaskRunner should end immediately. + // |SubmitFrame|. For example, merge task runners if `should_resubmit_frame` + // is true. // // For example on the iOS embedder, threads are merged in this call. // A new frame on the platform thread starts immediately. If the GPU thread // still has some task running, there could be two frames being rendered // concurrently, which causes undefined behaviors. virtual void EndFrame( + bool should_resubmit_frame, fml::RefPtr raster_thread_merger) {} FML_DISALLOW_COPY_AND_ASSIGN(ExternalViewEmbedder); diff --git a/engine/src/flutter/shell/common/rasterizer.cc b/engine/src/flutter/shell/common/rasterizer.cc index afcce2960a6..ca69a7b2abd 100644 --- a/engine/src/flutter/shell/common/rasterizer.cc +++ b/engine/src/flutter/shell/common/rasterizer.cc @@ -151,12 +151,10 @@ void Rasterizer::Draw(fml::RefPtr> pipeline) { // Merging the thread as we know the next `Draw` should be run on the platform // thread. - if (raster_status == RasterStatus::kResubmit) { - auto* external_view_embedder = surface_->GetExternalViewEmbedder(); - // We know only the `external_view_embedder` can - // causes|RasterStatus::kResubmit|. Check to make sure. - FML_DCHECK(external_view_embedder != nullptr); - external_view_embedder->EndFrame(raster_thread_merger_); + if (surface_ != nullptr && surface_->GetExternalViewEmbedder() != nullptr) { + auto should_resubmit_frame = raster_status == RasterStatus::kResubmit; + surface_->GetExternalViewEmbedder()->EndFrame(should_resubmit_frame, + raster_thread_merger_); } // Consume as many pipeline items as possible. But yield the event loop diff --git a/engine/src/flutter/shell/common/shell_test_external_view_embedder.cc b/engine/src/flutter/shell/common/shell_test_external_view_embedder.cc index 306c1b0016b..ba7aa22cb22 100644 --- a/engine/src/flutter/shell/common/shell_test_external_view_embedder.cc +++ b/engine/src/flutter/shell/common/shell_test_external_view_embedder.cc @@ -43,8 +43,9 @@ bool ShellTestExternalViewEmbedder::SubmitFrame( // |ExternalViewEmbedder| void ShellTestExternalViewEmbedder::EndFrame( + bool should_resubmit_frame, fml::RefPtr raster_thread_merger) { - end_frame_call_back_(); + end_frame_call_back_(should_resubmit_frame); } // |ExternalViewEmbedder| diff --git a/engine/src/flutter/shell/common/shell_test_external_view_embedder.h b/engine/src/flutter/shell/common/shell_test_external_view_embedder.h index da96503a9bb..26072331eae 100644 --- a/engine/src/flutter/shell/common/shell_test_external_view_embedder.h +++ b/engine/src/flutter/shell/common/shell_test_external_view_embedder.h @@ -15,7 +15,7 @@ namespace flutter { /// class ShellTestExternalViewEmbedder final : public ExternalViewEmbedder { public: - using EndFrameCallBack = std::function; + using EndFrameCallBack = std::function; ShellTestExternalViewEmbedder(const EndFrameCallBack& end_frame_call_back, PostPrerollResult post_preroll_result) @@ -56,6 +56,7 @@ class ShellTestExternalViewEmbedder final : public ExternalViewEmbedder { // |ExternalViewEmbedder| void EndFrame( + bool should_resubmit_frame, fml::RefPtr raster_thread_merger) override; // |ExternalViewEmbedder| diff --git a/engine/src/flutter/shell/common/shell_unittests.cc b/engine/src/flutter/shell/common/shell_unittests.cc index 9b3da6833f6..b9ba1e09dd1 100644 --- a/engine/src/flutter/shell/common/shell_unittests.cc +++ b/engine/src/flutter/shell/common/shell_unittests.cc @@ -474,7 +474,8 @@ TEST_F(ShellTest, auto settings = CreateSettingsForFixture(); fml::AutoResetWaitableEvent endFrameLatch; bool end_frame_called = false; - auto end_frame_callback = [&] { + auto end_frame_callback = [&](bool should_resubmit_frame) { + ASSERT_TRUE(should_resubmit_frame); end_frame_called = true; endFrameLatch.Signal(); }; diff --git a/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder.cc b/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder.cc index 048e96295f6..bec7d7c799d 100644 --- a/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder.cc +++ b/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder.cc @@ -254,8 +254,9 @@ void AndroidExternalViewEmbedder::CancelFrame() { // |ExternalViewEmbedder| void AndroidExternalViewEmbedder::EndFrame( + bool should_resubmit_frame, fml::RefPtr raster_thread_merger) { - if (should_run_rasterizer_on_platform_thread_) { + if (should_resubmit_frame && should_run_rasterizer_on_platform_thread_) { raster_thread_merger->MergeWithLease(kDefaultMergedLeaseDuration); should_run_rasterizer_on_platform_thread_ = false; } diff --git a/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder.h b/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder.h index 41b6e68f115..2f0562bdbf6 100644 --- a/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder.h +++ b/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder.h @@ -65,6 +65,7 @@ class AndroidExternalViewEmbedder final : public ExternalViewEmbedder { // |ExternalViewEmbedder| void EndFrame( + bool should_resubmit_frame, fml::RefPtr raster_thread_merger) override; // Gets the rect based on the device pixel ratio of a platform view displayed diff --git a/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_unittests.cc b/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_unittests.cc index a0551a0a39d..8bac045e1b9 100644 --- a/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_unittests.cc +++ b/engine/src/flutter/shell/platform/android/external_view_embedder/external_view_embedder_unittests.cc @@ -158,7 +158,7 @@ TEST(AndroidExternalViewEmbedder, RasterizerRunsOnPlatformThread) { ASSERT_TRUE(embedder->SubmitFrame(nullptr, nullptr)); EXPECT_CALL(*jni_mock, FlutterViewEndFrame()); - embedder->EndFrame(raster_thread_merger); + embedder->EndFrame(/*should_resubmit_frame=*/true, raster_thread_merger); ASSERT_TRUE(raster_thread_merger->IsMerged()); @@ -182,7 +182,7 @@ TEST(AndroidExternalViewEmbedder, RasterizerRunsOnRasterizerThread) { ASSERT_EQ(PostPrerollResult::kSuccess, result); EXPECT_CALL(*jni_mock, FlutterViewEndFrame()); - embedder->EndFrame(raster_thread_merger); + embedder->EndFrame(/*should_resubmit_frame=*/true, raster_thread_merger); ASSERT_FALSE(raster_thread_merger->IsMerged()); } @@ -332,7 +332,7 @@ TEST(AndroidExternalViewEmbedder, SubmitFrame__RecycleSurfaces) { embedder->SubmitFrame(gr_context.get(), std::move(surface_frame)); EXPECT_CALL(*jni_mock, FlutterViewEndFrame()); - embedder->EndFrame(raster_thread_merger); + embedder->EndFrame(/*should_resubmit_frame=*/false, raster_thread_merger); } // ------------------ Second frame ------------------ // @@ -380,7 +380,7 @@ TEST(AndroidExternalViewEmbedder, SubmitFrame__RecycleSurfaces) { embedder->SubmitFrame(gr_context.get(), std::move(surface_frame)); EXPECT_CALL(*jni_mock, FlutterViewEndFrame()); - embedder->EndFrame(raster_thread_merger); + embedder->EndFrame(/*should_resubmit_frame=*/false, raster_thread_merger); } } @@ -399,7 +399,7 @@ TEST(AndroidExternalViewEmbedder, DoesNotCallJNIPlatformThreadOnlyMethods) { raster_thread_merger); EXPECT_CALL(*jni_mock, FlutterViewEndFrame()).Times(0); - embedder->EndFrame(raster_thread_merger); + embedder->EndFrame(/*should_resubmit_frame=*/false, raster_thread_merger); } } // namespace testing diff --git a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm b/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm index 474e9f084c1..1b021bc6081 100644 --- a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm +++ b/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm @@ -604,8 +604,9 @@ void FlutterPlatformViewsController::BringLayersIntoView(LayersMap layer_map) { } void FlutterPlatformViewsController::EndFrame( + bool should_resubmit_frame, fml::RefPtr raster_thread_merger) { - if (merge_threads_) { + if (should_resubmit_frame && merge_threads_) { raster_thread_merger->MergeWithLease(kDefaultMergedLeaseDuration); merge_threads_ = false; } diff --git a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h b/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h index 26984400c2e..8c570cd97cb 100644 --- a/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h +++ b/engine/src/flutter/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h @@ -168,7 +168,8 @@ class FlutterPlatformViewsController { // Invoked at the very end of a frame. // After invoking this method, nothing should happen on the current TaskRunner during the same // frame. - void EndFrame(fml::RefPtr raster_thread_merger); + void EndFrame(bool should_resubmit_frame, + fml::RefPtr raster_thread_merger); void OnMethodCall(FlutterMethodCall* call, FlutterResult& result); diff --git a/engine/src/flutter/shell/platform/darwin/ios/ios_surface.h b/engine/src/flutter/shell/platform/darwin/ios/ios_surface.h index 5fb6f78f6d7..5e58ce2c6af 100644 --- a/engine/src/flutter/shell/platform/darwin/ios/ios_surface.h +++ b/engine/src/flutter/shell/platform/darwin/ios/ios_surface.h @@ -85,7 +85,8 @@ class IOSSurface : public ExternalViewEmbedder { bool SubmitFrame(GrContext* context, std::unique_ptr frame) override; // |ExternalViewEmbedder| - void EndFrame(fml::RefPtr raster_thread_merger) override; + void EndFrame(bool should_resubmit_frame, + fml::RefPtr raster_thread_merger) override; public: FML_DISALLOW_COPY_AND_ASSIGN(IOSSurface); diff --git a/engine/src/flutter/shell/platform/darwin/ios/ios_surface.mm b/engine/src/flutter/shell/platform/darwin/ios/ios_surface.mm index 26eca6cf26b..30b08c298c5 100644 --- a/engine/src/flutter/shell/platform/darwin/ios/ios_surface.mm +++ b/engine/src/flutter/shell/platform/darwin/ios/ios_surface.mm @@ -149,10 +149,11 @@ bool IOSSurface::SubmitFrame(GrContext* context, std::unique_ptr f } // |ExternalViewEmbedder| -void IOSSurface::EndFrame(fml::RefPtr raster_thread_merger) { +void IOSSurface::EndFrame(bool should_resubmit_frame, + fml::RefPtr raster_thread_merger) { TRACE_EVENT0("flutter", "IOSSurface::EndFrame"); FML_CHECK(platform_views_controller_ != nullptr); - return platform_views_controller_->EndFrame(raster_thread_merger); + return platform_views_controller_->EndFrame(should_resubmit_frame, raster_thread_merger); } } // namespace flutter