From cf55c18c782cdd8120ed130ea2d41d20783b1cbe Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Wed, 2 Sep 2020 19:18:53 -0700 Subject: [PATCH 1/3] Only clear GL context after changing the thread configuration --- fml/raster_thread_merger.cc | 23 +++++++++++++++++++---- fml/raster_thread_merger.h | 8 ++++++++ shell/common/rasterizer.cc | 16 ++++++++-------- 3 files changed, 35 insertions(+), 12 deletions(-) diff --git a/fml/raster_thread_merger.cc b/fml/raster_thread_merger.cc index 206d2157e874b..88b0b16e318b0 100644 --- a/fml/raster_thread_merger.cc +++ b/fml/raster_thread_merger.cc @@ -21,6 +21,10 @@ RasterThreadMerger::RasterThreadMerger(fml::TaskQueueId platform_queue_id, FML_CHECK(!task_queues_->Owns(platform_queue_id_, gpu_queue_id_)); } +void RasterThreadMerger::SetMergeUnmergeCallback(const fml::closure& callback) { + merge_unmerge_callback_ = callback; +} + void RasterThreadMerger::MergeWithLease(size_t lease_term) { std::scoped_lock lock(lease_term_mutex_); if (TaskQueuesAreSame()) { @@ -30,11 +34,19 @@ void RasterThreadMerger::MergeWithLease(size_t lease_term) { return; } FML_DCHECK(lease_term > 0) << "lease_term should be positive."; - if (!IsMergedUnSafe()) { - bool success = task_queues_->Merge(platform_queue_id_, gpu_queue_id_); - FML_CHECK(success) << "Unable to merge the raster and platform threads."; - lease_term_ = lease_term; + + if (IsMergedUnSafe()) { + merged_condition_.notify_one(); + return; + } + + bool success = task_queues_->Merge(platform_queue_id_, gpu_queue_id_); + if (success && merge_unmerge_callback_ != nullptr) { + merge_unmerge_callback_(); } + FML_CHECK(success) << "Unable to merge the raster and platform threads."; + lease_term_ = lease_term; + merged_condition_.notify_one(); } @@ -48,6 +60,9 @@ void RasterThreadMerger::UnMergeNow() { } lease_term_ = 0; bool success = task_queues_->Unmerge(platform_queue_id_); + if (success && merge_unmerge_callback_ != nullptr) { + merge_unmerge_callback_(); + } FML_CHECK(success) << "Unable to un-merge the raster and platform threads."; } diff --git a/fml/raster_thread_merger.h b/fml/raster_thread_merger.h index 785dd000b1857..dd464ca74fd43 100644 --- a/fml/raster_thread_merger.h +++ b/fml/raster_thread_merger.h @@ -85,6 +85,13 @@ class RasterThreadMerger // noop. bool IsEnabled(); + // Registers a callback that can be used to clean up global state right after + // the thread configuration has changed. + // + // For example, it can be used to clear the GL context so it can be used in + // the next task from a different thread. + void SetMergeUnmergeCallback(const fml::closure& callback); + private: static const int kLeaseNotSet; fml::TaskQueueId platform_queue_id_; @@ -93,6 +100,7 @@ class RasterThreadMerger std::atomic_int lease_term_; std::condition_variable merged_condition_; std::mutex lease_term_mutex_; + fml::closure merge_unmerge_callback_; bool enabled_; bool IsMergedUnSafe() const; diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index f80ab4a8edc1d..bd9cbb1cbc754 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -89,6 +89,14 @@ void Rasterizer::Setup(std::unique_ptr surface) { delegate_.GetTaskRunners().GetRasterTaskRunner()->GetTaskQueueId(); raster_thread_merger_ = fml::MakeRefCounted(platform_id, gpu_id); + + raster_thread_merger_->SetMergeUnmergeCallback( + [weak_this = weak_factory_.GetWeakPtr()]() { + // Clear the GL context after the thread configuration has changed. + if (weak_this && weak_this->surface_) { + weak_this->surface_->ClearRenderContext(); + } + }); } #endif } @@ -467,14 +475,6 @@ RasterStatus Rasterizer::DrawToSurface(flutter::LayerTree& layer_tree) { FireNextFrameCallbackIfPresent(); - // Clear the render context after submitting the frame. - // This ensures that the GL context is released after drawing to the - // surface. - // - // The GL context must be clear before performing Gr context deferred - // cleanup. - surface_->ClearRenderContext(); - if (surface_->GetContext()) { TRACE_EVENT0("flutter", "PerformDeferredSkiaCleanup"); surface_->GetContext()->performDeferredCleanup(kSkiaCleanupExpiration); From a68011ef85d8ccbb12b4797564c6147989793a50 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Wed, 2 Sep 2020 20:07:34 -0700 Subject: [PATCH 2/3] Test and fixes --- fml/raster_thread_merger_unittests.cc | 48 +++++++++++++++++++++++++++ shell/common/rasterizer.cc | 14 ++++---- 2 files changed, 54 insertions(+), 8 deletions(-) diff --git a/fml/raster_thread_merger_unittests.cc b/fml/raster_thread_merger_unittests.cc index 67d1da4d20576..76d76964d9860 100644 --- a/fml/raster_thread_merger_unittests.cc +++ b/fml/raster_thread_merger_unittests.cc @@ -579,5 +579,53 @@ TEST(RasterThreadMerger, RunExpiredTasksWhileFirstTaskUnMergesThreads) { thread_raster.join(); } +TEST(RasterThreadMerger, SetMergeUnmergeCallback) { + fml::MessageLoop* loop1 = nullptr; + fml::AutoResetWaitableEvent latch1; + fml::AutoResetWaitableEvent term1; + std::thread thread1([&loop1, &latch1, &term1]() { + fml::MessageLoop::EnsureInitializedForCurrentThread(); + loop1 = &fml::MessageLoop::GetCurrent(); + latch1.Signal(); + term1.Wait(); + }); + + fml::MessageLoop* loop2 = nullptr; + fml::AutoResetWaitableEvent latch2; + fml::AutoResetWaitableEvent term2; + std::thread thread2([&loop2, &latch2, &term2]() { + fml::MessageLoop::EnsureInitializedForCurrentThread(); + loop2 = &fml::MessageLoop::GetCurrent(); + latch2.Signal(); + term2.Wait(); + }); + + latch1.Wait(); + latch2.Wait(); + + fml::TaskQueueId qid1 = loop1->GetTaskRunner()->GetTaskQueueId(); + fml::TaskQueueId qid2 = loop2->GetTaskRunner()->GetTaskQueueId(); + + const auto raster_thread_merger = + fml::MakeRefCounted(qid1, qid2); + + int callbacks = 0; + raster_thread_merger->SetMergeUnmergeCallback( + [&callbacks]() { callbacks++; }); + + ASSERT_EQ(0, callbacks); + + raster_thread_merger->MergeWithLease(1); + ASSERT_EQ(1, callbacks); + + raster_thread_merger->DecrementLease(); + ASSERT_EQ(2, callbacks); + + term1.Signal(); + term2.Signal(); + thread1.join(); + thread2.join(); +} + } // namespace testing } // namespace fml diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index bd9cbb1cbc754..16f2c60843acd 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -89,14 +89,12 @@ void Rasterizer::Setup(std::unique_ptr surface) { delegate_.GetTaskRunners().GetRasterTaskRunner()->GetTaskQueueId(); raster_thread_merger_ = fml::MakeRefCounted(platform_id, gpu_id); - - raster_thread_merger_->SetMergeUnmergeCallback( - [weak_this = weak_factory_.GetWeakPtr()]() { - // Clear the GL context after the thread configuration has changed. - if (weak_this && weak_this->surface_) { - weak_this->surface_->ClearRenderContext(); - } - }); + raster_thread_merger_->SetMergeUnmergeCallback([=]() { + // Clear the GL context after the thread configuration has changed. + if (surface_) { + surface_->ClearRenderContext(); + } + }); } #endif } From 2893c3434cc1498bd592154a7fb7def9efe5e90c Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Wed, 2 Sep 2020 20:34:06 -0700 Subject: [PATCH 3/3] Clear callback in rasterizer teardown --- shell/common/rasterizer.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 16f2c60843acd..e5ed67dcd0935 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -108,6 +108,7 @@ void Rasterizer::Teardown() { raster_thread_merger_.get()->IsMerged()) { FML_DCHECK(raster_thread_merger_->IsEnabled()); raster_thread_merger_->UnMergeNow(); + raster_thread_merger_->SetMergeUnmergeCallback(nullptr); } }