From 3077cf3a2d2984bf0d927f5161888b6eb8a037b7 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Mon, 17 May 2021 17:22:45 -0700 Subject: [PATCH] Capture the layer tree pipeline as a weak pointer in the OnAnimatorDraw task This prevents the OnAnimatorDraw lambda from extending the pipeline's lifetime beyond the lifetime of the animator and rasterizer. Fixes https://github.com/flutter/flutter/issues/82353 --- shell/common/animator.cc | 4 ++-- shell/common/animator.h | 4 ++-- shell/common/pipeline.h | 2 +- shell/common/pipeline_unittests.cc | 12 ++++++------ shell/common/rasterizer.cc | 2 +- shell/common/rasterizer.h | 2 +- shell/common/rasterizer_unittests.cc | 10 +++++----- shell/common/shell.cc | 12 ++++++++---- shell/common/shell.h | 2 +- 9 files changed, 27 insertions(+), 23 deletions(-) diff --git a/shell/common/animator.cc b/shell/common/animator.cc index 2fbd6fa772f1e..5e3b2d17fa2cd 100644 --- a/shell/common/animator.cc +++ b/shell/common/animator.cc @@ -29,12 +29,12 @@ Animator::Animator(Delegate& delegate, waiter_(std::move(waiter)), dart_frame_deadline_(0), #if SHELL_ENABLE_METAL - layer_tree_pipeline_(fml::MakeRefCounted(2)), + layer_tree_pipeline_(std::make_shared(2)), #else // SHELL_ENABLE_METAL // TODO(dnfield): We should remove this logic and set the pipeline depth // back to 2 in this case. See // https://github.com/flutter/engine/pull/9132 for discussion. - layer_tree_pipeline_(fml::MakeRefCounted( + layer_tree_pipeline_(std::make_shared( task_runners.GetPlatformTaskRunner() == task_runners.GetRasterTaskRunner() ? 1 diff --git a/shell/common/animator.h b/shell/common/animator.h index 058b1bdc32f47..53dd2e275feec 100644 --- a/shell/common/animator.h +++ b/shell/common/animator.h @@ -36,7 +36,7 @@ class Animator final { virtual void OnAnimatorNotifyIdle(int64_t deadline) = 0; virtual void OnAnimatorDraw( - fml::RefPtr> pipeline, + std::shared_ptr> pipeline, std::unique_ptr frame_timings_recorder) = 0; virtual void OnAnimatorDrawLastLayerTree( @@ -105,7 +105,7 @@ class Animator final { std::unique_ptr frame_timings_recorder_; int64_t dart_frame_deadline_; - fml::RefPtr layer_tree_pipeline_; + std::shared_ptr layer_tree_pipeline_; fml::Semaphore pending_frame_semaphore_; LayerTreePipeline::ProducerContinuation producer_continuation_; bool paused_; diff --git a/shell/common/pipeline.h b/shell/common/pipeline.h index d05e0f50612c0..f51f41e942812 100644 --- a/shell/common/pipeline.h +++ b/shell/common/pipeline.h @@ -27,7 +27,7 @@ size_t GetNextPipelineTraceID(); /// A thread-safe queue of resources for a single consumer and a single /// producer. template -class Pipeline : public fml::RefCountedThreadSafe> { +class Pipeline { public: using Resource = R; using ResourcePtr = std::unique_ptr; diff --git a/shell/common/pipeline_unittests.cc b/shell/common/pipeline_unittests.cc index 0327042cc6c0e..159022ee53f12 100644 --- a/shell/common/pipeline_unittests.cc +++ b/shell/common/pipeline_unittests.cc @@ -19,7 +19,7 @@ using IntPipeline = Pipeline; using Continuation = IntPipeline::ProducerContinuation; TEST(PipelineTest, ConsumeOneVal) { - fml::RefPtr pipeline = fml::MakeRefCounted(2); + std::shared_ptr pipeline = std::make_shared(2); Continuation continuation = pipeline->Produce(); @@ -34,7 +34,7 @@ TEST(PipelineTest, ConsumeOneVal) { } TEST(PipelineTest, ContinuationCanOnlyBeUsedOnce) { - fml::RefPtr pipeline = fml::MakeRefCounted(2); + std::shared_ptr pipeline = std::make_shared(2); Continuation continuation = pipeline->Produce(); @@ -59,7 +59,7 @@ TEST(PipelineTest, ContinuationCanOnlyBeUsedOnce) { TEST(PipelineTest, PushingMoreThanDepthCompletesFirstSubmission) { const int depth = 1; - fml::RefPtr pipeline = fml::MakeRefCounted(depth); + std::shared_ptr pipeline = std::make_shared(depth); Continuation continuation_1 = pipeline->Produce(); Continuation continuation_2 = pipeline->Produce(); @@ -78,7 +78,7 @@ TEST(PipelineTest, PushingMoreThanDepthCompletesFirstSubmission) { TEST(PipelineTest, PushingMultiProcessesInOrder) { const int depth = 2; - fml::RefPtr pipeline = fml::MakeRefCounted(depth); + std::shared_ptr pipeline = std::make_shared(depth); Continuation continuation_1 = pipeline->Produce(); Continuation continuation_2 = pipeline->Produce(); @@ -100,7 +100,7 @@ TEST(PipelineTest, PushingMultiProcessesInOrder) { TEST(PipelineTest, ProduceIfEmptyDoesNotConsumeWhenQueueIsNotEmpty) { const int depth = 2; - fml::RefPtr pipeline = fml::MakeRefCounted(depth); + std::shared_ptr pipeline = std::make_shared(depth); Continuation continuation_1 = pipeline->Produce(); Continuation continuation_2 = pipeline->ProduceIfEmpty(); @@ -118,7 +118,7 @@ TEST(PipelineTest, ProduceIfEmptyDoesNotConsumeWhenQueueIsNotEmpty) { TEST(PipelineTest, ProduceIfEmptySuccessfulIfQueueIsEmpty) { const int depth = 1; - fml::RefPtr pipeline = fml::MakeRefCounted(depth); + std::shared_ptr pipeline = std::make_shared(depth); Continuation continuation_1 = pipeline->ProduceIfEmpty(); diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 7281f5f29dddc..b78c0c17f3fd7 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -158,7 +158,7 @@ void Rasterizer::DrawLastLayerTree( void Rasterizer::Draw( std::unique_ptr frame_timings_recorder, - fml::RefPtr> pipeline, + std::shared_ptr> pipeline, LayerTreeDiscardCallback discardCallback) { TRACE_EVENT0("flutter", "GPURasterizer::Draw"); if (raster_thread_merger_ && diff --git a/shell/common/rasterizer.h b/shell/common/rasterizer.h index 1f1559cbb9466..b4fb8bb02128b 100644 --- a/shell/common/rasterizer.h +++ b/shell/common/rasterizer.h @@ -244,7 +244,7 @@ class Rasterizer final : public SnapshotDelegate { /// is discarded instead of being rendered /// void Draw(std::unique_ptr frame_timings_recorder, - fml::RefPtr> pipeline, + std::shared_ptr> pipeline, LayerTreeDiscardCallback discardCallback = NoDiscard); //---------------------------------------------------------------------------- diff --git a/shell/common/rasterizer_unittests.cc b/shell/common/rasterizer_unittests.cc index 9effc8ae98c0d..6e4a64aa87f9c 100644 --- a/shell/common/rasterizer_unittests.cc +++ b/shell/common/rasterizer_unittests.cc @@ -105,7 +105,7 @@ TEST(RasterizerTest, drawEmptyPipeline) { rasterizer->Setup(std::move(surface)); fml::AutoResetWaitableEvent latch; thread_host.raster_thread->GetTaskRunner()->PostTask([&] { - auto pipeline = fml::AdoptRef(new Pipeline(/*depth=*/10)); + auto pipeline = std::make_shared>(/*depth=*/10); rasterizer->Draw(CreateFinishedBuildRecorder(), pipeline, nullptr); latch.Signal(); }); @@ -157,7 +157,7 @@ TEST(RasterizerTest, rasterizer->Setup(std::move(surface)); fml::AutoResetWaitableEvent latch; thread_host.raster_thread->GetTaskRunner()->PostTask([&] { - auto pipeline = fml::AdoptRef(new Pipeline(/*depth=*/10)); + auto pipeline = std::make_shared>(/*depth=*/10); auto layer_tree = std::make_unique(/*frame_size=*/SkISize(), /*device_pixel_ratio=*/2.0f); bool result = pipeline->Produce().Complete(std::move(layer_tree)); @@ -211,7 +211,7 @@ TEST( rasterizer->Setup(std::move(surface)); fml::AutoResetWaitableEvent latch; thread_host.raster_thread->GetTaskRunner()->PostTask([&] { - auto pipeline = fml::AdoptRef(new Pipeline(/*depth=*/10)); + auto pipeline = std::make_shared>(/*depth=*/10); auto layer_tree = std::make_unique(/*frame_size=*/SkISize(), /*device_pixel_ratio=*/2.0f); bool result = pipeline->Produce().Complete(std::move(layer_tree)); @@ -270,7 +270,7 @@ TEST( rasterizer->Setup(std::move(surface)); - auto pipeline = fml::AdoptRef(new Pipeline(/*depth=*/10)); + auto pipeline = std::make_shared>(/*depth=*/10); auto layer_tree = std::make_unique(/*frame_size=*/SkISize(), /*device_pixel_ratio=*/2.0f); bool result = pipeline->Produce().Complete(std::move(layer_tree)); @@ -307,7 +307,7 @@ TEST(RasterizerTest, externalViewEmbedderDoesntEndFrameWhenNoSurfaceIsSet) { fml::AutoResetWaitableEvent latch; thread_host.raster_thread->GetTaskRunner()->PostTask([&] { - auto pipeline = fml::AdoptRef(new Pipeline(/*depth=*/10)); + auto pipeline = std::make_shared>(/*depth=*/10); auto no_discard = [](LayerTree&) { return false; }; rasterizer->Draw(CreateFinishedBuildRecorder(), pipeline, no_discard); latch.Signal(); diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 8c37afa183eff..50ef20a25bec8 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1128,7 +1128,7 @@ void Shell::OnAnimatorNotifyIdle(int64_t deadline) { // |Animator::Delegate| void Shell::OnAnimatorDraw( - fml::RefPtr> pipeline, + std::shared_ptr> pipeline, std::unique_ptr frame_timings_recorder) { FML_DCHECK(is_setup_); @@ -1153,12 +1153,16 @@ void Shell::OnAnimatorDraw( task_runners_.GetRasterTaskRunner()->PostTask(fml::MakeCopyable( [&waiting_for_first_frame = waiting_for_first_frame_, &waiting_for_first_frame_condition = waiting_for_first_frame_condition_, - rasterizer = rasterizer_->GetWeakPtr(), pipeline = std::move(pipeline), + rasterizer = rasterizer_->GetWeakPtr(), + weak_pipeline = std::weak_ptr>(pipeline), discard_callback = std::move(discard_callback), frame_timings_recorder = std::move(frame_timings_recorder)]() mutable { if (rasterizer) { - rasterizer->Draw(std::move(frame_timings_recorder), pipeline, - std::move(discard_callback)); + std::shared_ptr> pipeline = weak_pipeline.lock(); + if (pipeline) { + rasterizer->Draw(std::move(frame_timings_recorder), + std::move(pipeline), std::move(discard_callback)); + } if (waiting_for_first_frame.load()) { waiting_for_first_frame.store(false); diff --git a/shell/common/shell.h b/shell/common/shell.h index ebc04263145ed..e39f78d633f43 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -539,7 +539,7 @@ class Shell final : public PlatformView::Delegate, // |Animator::Delegate| void OnAnimatorDraw( - fml::RefPtr> pipeline, + std::shared_ptr> pipeline, std::unique_ptr frame_timings_recorder) override; // |Animator::Delegate|