diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index 9292b5aa78057..8c108ca8a8aa2 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -638,10 +638,10 @@ FILE: ../../../flutter/shell/common/engine_unittests.cc FILE: ../../../flutter/shell/common/fixtures/shell_test.dart FILE: ../../../flutter/shell/common/fixtures/shelltest_screenshot.png FILE: ../../../flutter/shell/common/input_events_unittests.cc +FILE: ../../../flutter/shell/common/layer_tree_holder.cc +FILE: ../../../flutter/shell/common/layer_tree_holder.h +FILE: ../../../flutter/shell/common/layer_tree_holder_unittests.cc FILE: ../../../flutter/shell/common/persistent_cache_unittests.cc -FILE: ../../../flutter/shell/common/pipeline.cc -FILE: ../../../flutter/shell/common/pipeline.h -FILE: ../../../flutter/shell/common/pipeline_unittests.cc FILE: ../../../flutter/shell/common/platform_view.cc FILE: ../../../flutter/shell/common/platform_view.h FILE: ../../../flutter/shell/common/pointer_data_dispatcher.cc diff --git a/shell/common/BUILD.gn b/shell/common/BUILD.gn index ef390e5d0649e..de891fd1e34a3 100644 --- a/shell/common/BUILD.gn +++ b/shell/common/BUILD.gn @@ -69,8 +69,8 @@ source_set("common") { "display_manager.h", "engine.cc", "engine.h", - "pipeline.cc", - "pipeline.h", + "layer_tree_holder.cc", + "layer_tree_holder.h", "platform_view.cc", "platform_view.h", "pointer_data_dispatcher.cc", @@ -242,8 +242,8 @@ if (enable_unittests) { "canvas_spy_unittests.cc", "engine_unittests.cc", "input_events_unittests.cc", + "layer_tree_holder_unittests.cc", "persistent_cache_unittests.cc", - "pipeline_unittests.cc", "rasterizer_unittests.cc", "shell_unittests.cc", "skp_shader_warmup_unittests.cc", diff --git a/shell/common/animator.cc b/shell/common/animator.cc index cacac38c12063..3fc5857dfd209 100644 --- a/shell/common/animator.cc +++ b/shell/common/animator.cc @@ -29,18 +29,7 @@ Animator::Animator(Delegate& delegate, last_vsync_start_time_(), last_frame_target_time_(), dart_frame_deadline_(0), -#if SHELL_ENABLE_METAL - layer_tree_pipeline_(fml::MakeRefCounted(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( - task_runners.GetPlatformTaskRunner() == - task_runners.GetRasterTaskRunner() - ? 1 - : 2)), -#endif // SHELL_ENABLE_METAL + layer_tree_holder_(std::make_shared()), pending_frame_semaphore_(1), frame_number_(1), paused_(false), @@ -48,8 +37,7 @@ Animator::Animator(Delegate& delegate, frame_scheduled_(false), notify_idle_task_id_(0), dimension_change_pending_(false), - weak_factory_(this) { -} + weak_factory_(this) {} Animator::~Animator() = default; @@ -111,25 +99,6 @@ void Animator::BeginFrame(fml::TimePoint vsync_start_time, regenerate_layer_tree_ = false; pending_frame_semaphore_.Signal(); - if (!producer_continuation_) { - // We may already have a valid pipeline continuation in case a previous - // begin frame did not result in an Animation::Render. Simply reuse that - // instead of asking the pipeline for a fresh continuation. - producer_continuation_ = layer_tree_pipeline_->Produce(); - - if (!producer_continuation_) { - // If we still don't have valid continuation, the pipeline is currently - // full because the consumer is being too slow. Try again at the next - // frame interval. - RequestFrame(); - return; - } - } - - // We have acquired a valid continuation from the pipeline and are ready - // to service potential frame. - FML_DCHECK(producer_continuation_); - last_frame_begin_time_ = fml::TimePoint::Now(); last_vsync_start_time_ = vsync_start_time; fml::tracing::TraceEventAsyncComplete("flutter", "VsyncSchedulingOverhead", @@ -183,13 +152,8 @@ void Animator::Render(std::unique_ptr layer_tree) { layer_tree->RecordBuildTime(last_vsync_start_time_, last_frame_begin_time_, last_frame_target_time_); - // Commit the pending continuation. - bool result = producer_continuation_.Complete(std::move(layer_tree)); - if (!result) { - FML_DLOG(INFO) << "No pending continuation to commit"; - } - - delegate_.OnAnimatorDraw(layer_tree_pipeline_, last_frame_target_time_); + layer_tree_holder_->PushIfNewer(std::move(layer_tree)); + delegate_.OnAnimatorDraw(layer_tree_holder_, last_frame_target_time_); } bool Animator::CanReuseLastLayerTree() { diff --git a/shell/common/animator.h b/shell/common/animator.h index cd2fff454e277..faba71cb15387 100644 --- a/shell/common/animator.h +++ b/shell/common/animator.h @@ -6,13 +6,14 @@ #define FLUTTER_SHELL_COMMON_ANIMATOR_H_ #include +#include #include "flutter/common/task_runners.h" #include "flutter/fml/memory/ref_ptr.h" #include "flutter/fml/memory/weak_ptr.h" #include "flutter/fml/synchronization/semaphore.h" #include "flutter/fml/time/time_point.h" -#include "flutter/shell/common/pipeline.h" +#include "flutter/shell/common/layer_tree_holder.h" #include "flutter/shell/common/rasterizer.h" #include "flutter/shell/common/vsync_waiter.h" @@ -35,7 +36,7 @@ class Animator final { virtual void OnAnimatorNotifyIdle(int64_t deadline) = 0; virtual void OnAnimatorDraw( - fml::RefPtr> pipeline, + std::shared_ptr layer_tree_holder, fml::TimePoint frame_target_time) = 0; virtual void OnAnimatorDrawLastLayerTree() = 0; @@ -79,8 +80,6 @@ class Animator final { void EnqueueTraceFlowId(uint64_t trace_flow_id); private: - using LayerTreePipeline = Pipeline; - void BeginFrame(fml::TimePoint frame_start_time, fml::TimePoint frame_target_time); @@ -99,9 +98,8 @@ class Animator final { fml::TimePoint last_vsync_start_time_; fml::TimePoint last_frame_target_time_; int64_t dart_frame_deadline_; - fml::RefPtr layer_tree_pipeline_; + std::shared_ptr layer_tree_holder_; fml::Semaphore pending_frame_semaphore_; - LayerTreePipeline::ProducerContinuation producer_continuation_; int64_t frame_number_; bool paused_; bool regenerate_layer_tree_; diff --git a/shell/common/engine.h b/shell/common/engine.h index ec5778f99073c..d70ec0e7c9c27 100644 --- a/shell/common/engine.h +++ b/shell/common/engine.h @@ -476,25 +476,18 @@ class Engine final : public RuntimeDelegate, /// will cause the jank in the Flutter application: /// * The time taken by this method to create a layer-tree exceeds /// on frame interval (for example, 16.66 ms on a 60Hz display). - /// * The time take by this method to generate a new layer-tree - /// causes the current layer-tree pipeline depth to change. To - /// illustrate this point, note that maximum pipeline depth used - /// by layer tree in the engine is 2. If both the UI and GPU - /// task runner tasks finish within one frame interval, the - /// pipeline depth is one. If the UI thread happens to be - /// working on a frame when the raster thread is still not done - /// with the previous frame, the pipeline depth is 2. When the - /// pipeline depth changes from 1 to 2, animations and UI - /// interactions that cause the generation of the new layer tree - /// appropriate for (frame_time + one frame interval) will - /// actually end up at (frame_time + two frame intervals). This - /// is not what code running on the UI thread expected would - /// happen. This causes perceptible jank. + /// * A new layer-tree produced by this method replaces a stale + /// layer tree in `LayerTreeHolder`. See: + /// `LayerTreeHolder::ReplaceIfNewer`. This could happen if + /// rasterizer takes more than one frame interval to rasterize a + /// layer tree. This would cause some frames to be skipped and + /// could result in perceptible jank. /// /// @param[in] frame_time The point at which the current frame interval /// began. May be used by animation interpolators, /// physics simulations, etc.. /// + /// @see `LayerTreeHolder::ReplaceIfNewer` void BeginFrame(fml::TimePoint frame_time); // |HintFreedDelegate| diff --git a/shell/common/layer_tree_holder.cc b/shell/common/layer_tree_holder.cc new file mode 100644 index 0000000000000..c329b37e47efe --- /dev/null +++ b/shell/common/layer_tree_holder.cc @@ -0,0 +1,32 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "flutter/shell/common/layer_tree_holder.h" + +namespace flutter { + +LayerTreeHolder::LayerTreeHolder() = default; + +LayerTreeHolder::~LayerTreeHolder() = default; + +std::unique_ptr LayerTreeHolder::Pop() { + std::scoped_lock lock(layer_tree_mutex); + return std::move(layer_tree_); +} + +void LayerTreeHolder::PushIfNewer( + std::unique_ptr proposed_layer_tree) { + std::scoped_lock lock(layer_tree_mutex); + if (!layer_tree_ || + layer_tree_->target_time() < proposed_layer_tree->target_time()) { + layer_tree_ = std::move(proposed_layer_tree); + } +} + +bool LayerTreeHolder::IsEmpty() const { + std::scoped_lock lock(layer_tree_mutex); + return !layer_tree_; +} + +}; // namespace flutter diff --git a/shell/common/layer_tree_holder.h b/shell/common/layer_tree_holder.h new file mode 100644 index 0000000000000..c1baf9fc1ebd6 --- /dev/null +++ b/shell/common/layer_tree_holder.h @@ -0,0 +1,55 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef FLUTTER_SHELL_COMMON_LAYER_TREE_HOLDER_H_ +#define FLUTTER_SHELL_COMMON_LAYER_TREE_HOLDER_H_ + +#include + +#include "flow/layers/layer_tree.h" + +namespace flutter { + +/** + * @brief Holds the next `flutter::LayerTree` that needs to be rasterized. The + * accesses to `LayerTreeHolder` are thread safe. This is important as this + * component is accessed from both the UI and the Raster threads. + * + * A typical flow of events through this component would be: + * 1. `flutter::Animator` pushed a layer tree to be rendered during each + * `Animator::Render` call. + * 2. `flutter::Rasterizer::Draw` consumes the pushed layer tree via `Pop`. + * + * It is important to note that if a layer tree held by this class is yet to be + * consumed, it can be overriden by a newer layer tree produced by the + * `Animator`. The newness of the layer tree is determined by the target time. + */ +class LayerTreeHolder { + public: + LayerTreeHolder(); + + ~LayerTreeHolder(); + + /** + * @brief Checks if a layer tree is currently held. + * + * @return true is no layer tree is held. + * @return false if there is a layer tree waiting to be consumed. + */ + bool IsEmpty() const; + + [[nodiscard]] std::unique_ptr Pop(); + + void PushIfNewer(std::unique_ptr proposed_layer_tree); + + private: + mutable std::mutex layer_tree_mutex; + std::unique_ptr layer_tree_; + + FML_DISALLOW_COPY_AND_ASSIGN(LayerTreeHolder); +}; + +}; // namespace flutter + +#endif // FLUTTER_SHELL_COMMON_LAYER_TREE_HOLDER_H_ diff --git a/shell/common/layer_tree_holder_unittests.cc b/shell/common/layer_tree_holder_unittests.cc new file mode 100644 index 0000000000000..35e13ad45bf12 --- /dev/null +++ b/shell/common/layer_tree_holder_unittests.cc @@ -0,0 +1,76 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// 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 +#include + +#include "flutter/shell/common/layer_tree_holder.h" +#include "gtest/gtest.h" + +namespace flutter { +namespace testing { + +TEST(LayerTreeHolder, EmptyOnInit) { + const LayerTreeHolder layer_tree_holder; + ASSERT_TRUE(layer_tree_holder.IsEmpty()); +} + +TEST(LayerTreeHolder, PutOneAndGet) { + LayerTreeHolder layer_tree_holder; + const auto frame_size = SkISize::Make(64, 64); + auto layer_tree = std::make_unique(frame_size, 1.0f); + layer_tree_holder.PushIfNewer(std::move(layer_tree)); + ASSERT_FALSE(layer_tree_holder.IsEmpty()); + const auto stored = layer_tree_holder.Pop(); + ASSERT_EQ(stored->frame_size(), frame_size); + ASSERT_TRUE(layer_tree_holder.IsEmpty()); +} + +TEST(LayerTreeHolder, PutMultiGetsLatest) { + const auto build_begin = fml::TimePoint::Now(); + const auto target_time_1 = build_begin + fml::TimeDelta::FromSeconds(2); + const auto target_time_2 = build_begin + fml::TimeDelta::FromSeconds(5); + + LayerTreeHolder layer_tree_holder; + const auto frame_size_1 = SkISize::Make(64, 64); + auto layer_tree_1 = std::make_unique(frame_size_1, 1.0f); + layer_tree_1->RecordBuildTime(build_begin, build_begin, target_time_1); + layer_tree_holder.PushIfNewer(std::move(layer_tree_1)); + + const auto frame_size_2 = SkISize::Make(128, 128); + auto layer_tree_2 = std::make_unique(frame_size_2, 1.0f); + layer_tree_2->RecordBuildTime(build_begin, build_begin, target_time_2); + layer_tree_holder.PushIfNewer(std::move(layer_tree_2)); + + const auto stored = layer_tree_holder.Pop(); + ASSERT_EQ(stored->frame_size(), frame_size_2); + ASSERT_TRUE(layer_tree_holder.IsEmpty()); +} + +TEST(LayerTreeHolder, RetainsOlderIfNewerFrameHasEarlierTargetTime) { + const auto build_begin = fml::TimePoint::Now(); + const auto target_time_1 = build_begin + fml::TimeDelta::FromSeconds(5); + const auto target_time_2 = build_begin + fml::TimeDelta::FromSeconds(2); + + LayerTreeHolder layer_tree_holder; + const auto frame_size_1 = SkISize::Make(64, 64); + auto layer_tree_1 = std::make_unique(frame_size_1, 1.0f); + layer_tree_1->RecordBuildTime(build_begin, build_begin, target_time_1); + layer_tree_holder.PushIfNewer(std::move(layer_tree_1)); + + const auto frame_size_2 = SkISize::Make(128, 128); + auto layer_tree_2 = std::make_unique(frame_size_2, 1.0f); + layer_tree_2->RecordBuildTime(build_begin, build_begin, target_time_2); + layer_tree_holder.PushIfNewer(std::move(layer_tree_2)); + + const auto stored = layer_tree_holder.Pop(); + ASSERT_EQ(stored->frame_size(), frame_size_1); + ASSERT_TRUE(layer_tree_holder.IsEmpty()); +} + +} // namespace testing +} // namespace flutter diff --git a/shell/common/pipeline.cc b/shell/common/pipeline.cc deleted file mode 100644 index fb80c187181fd..0000000000000 --- a/shell/common/pipeline.cc +++ /dev/null @@ -1,14 +0,0 @@ -// Copyright 2013 The Flutter Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "flutter/shell/common/pipeline.h" - -namespace flutter { - -size_t GetNextPipelineTraceID() { - static std::atomic_size_t PipelineLastTraceID = {0}; - return ++PipelineLastTraceID; -} - -} // namespace flutter diff --git a/shell/common/pipeline.h b/shell/common/pipeline.h deleted file mode 100644 index d05e0f50612c0..0000000000000 --- a/shell/common/pipeline.h +++ /dev/null @@ -1,215 +0,0 @@ -// Copyright 2013 The Flutter Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef FLUTTER_SHELL_COMMON_PIPELINE_H_ -#define FLUTTER_SHELL_COMMON_PIPELINE_H_ - -#include -#include -#include - -#include "flutter/fml/macros.h" -#include "flutter/fml/memory/ref_counted.h" -#include "flutter/fml/synchronization/semaphore.h" -#include "flutter/fml/trace_event.h" - -namespace flutter { - -enum class PipelineConsumeResult { - NoneAvailable, - Done, - MoreAvailable, -}; - -size_t GetNextPipelineTraceID(); - -/// A thread-safe queue of resources for a single consumer and a single -/// producer. -template -class Pipeline : public fml::RefCountedThreadSafe> { - public: - using Resource = R; - using ResourcePtr = std::unique_ptr; - - /// Denotes a spot in the pipeline reserved for the producer to finish - /// preparing a completed pipeline resource. - class ProducerContinuation { - public: - ProducerContinuation() : trace_id_(0) {} - - ProducerContinuation(ProducerContinuation&& other) - : continuation_(other.continuation_), trace_id_(other.trace_id_) { - other.continuation_ = nullptr; - other.trace_id_ = 0; - } - - ProducerContinuation& operator=(ProducerContinuation&& other) { - std::swap(continuation_, other.continuation_); - std::swap(trace_id_, other.trace_id_); - return *this; - } - - ~ProducerContinuation() { - if (continuation_) { - continuation_(nullptr, trace_id_); - TRACE_EVENT_ASYNC_END0("flutter", "PipelineProduce", trace_id_); - // The continuation is being dropped on the floor. End the flow. - TRACE_FLOW_END("flutter", "PipelineItem", trace_id_); - TRACE_EVENT_ASYNC_END0("flutter", "PipelineItem", trace_id_); - } - } - - [[nodiscard]] bool Complete(ResourcePtr resource) { - bool result = false; - if (continuation_) { - result = continuation_(std::move(resource), trace_id_); - continuation_ = nullptr; - TRACE_EVENT_ASYNC_END0("flutter", "PipelineProduce", trace_id_); - TRACE_FLOW_STEP("flutter", "PipelineItem", trace_id_); - } - return result; - } - - operator bool() const { return continuation_ != nullptr; } - - private: - friend class Pipeline; - using Continuation = std::function; - - Continuation continuation_; - size_t trace_id_; - - ProducerContinuation(const Continuation& continuation, size_t trace_id) - : continuation_(continuation), trace_id_(trace_id) { - TRACE_FLOW_BEGIN("flutter", "PipelineItem", trace_id_); - TRACE_EVENT_ASYNC_BEGIN0("flutter", "PipelineItem", trace_id_); - TRACE_EVENT_ASYNC_BEGIN0("flutter", "PipelineProduce", trace_id_); - } - - FML_DISALLOW_COPY_AND_ASSIGN(ProducerContinuation); - }; - - explicit Pipeline(uint32_t depth) - : depth_(depth), empty_(depth), available_(0), inflight_(0) {} - - ~Pipeline() = default; - - bool IsValid() const { return empty_.IsValid() && available_.IsValid(); } - - ProducerContinuation Produce() { - if (!empty_.TryWait()) { - return {}; - } - ++inflight_; - FML_TRACE_COUNTER("flutter", "Pipeline Depth", - reinterpret_cast(this), // - "frames in flight", inflight_.load() // - ); - - return ProducerContinuation{ - std::bind(&Pipeline::ProducerCommit, this, std::placeholders::_1, - std::placeholders::_2), // continuation - GetNextPipelineTraceID()}; // trace id - } - - // Create a `ProducerContinuation` that will only push the task if the queue - // is empty. - // Prefer using |Produce|. ProducerContinuation returned by this method - // doesn't guarantee that the frame will be rendered. - ProducerContinuation ProduceIfEmpty() { - if (!empty_.TryWait()) { - return {}; - } - ++inflight_; - FML_TRACE_COUNTER("flutter", "Pipeline Depth", - reinterpret_cast(this), // - "frames in flight", inflight_.load() // - ); - - return ProducerContinuation{ - std::bind(&Pipeline::ProducerCommitIfEmpty, this, std::placeholders::_1, - std::placeholders::_2), // continuation - GetNextPipelineTraceID()}; // trace id - } - - using Consumer = std::function; - - /// @note Procedure doesn't copy all closures. - [[nodiscard]] PipelineConsumeResult Consume(const Consumer& consumer) { - if (consumer == nullptr) { - return PipelineConsumeResult::NoneAvailable; - } - - if (!available_.TryWait()) { - return PipelineConsumeResult::NoneAvailable; - } - - ResourcePtr resource; - size_t trace_id = 0; - size_t items_count = 0; - - { - std::scoped_lock lock(queue_mutex_); - std::tie(resource, trace_id) = std::move(queue_.front()); - queue_.pop_front(); - items_count = queue_.size(); - } - - { - TRACE_EVENT0("flutter", "PipelineConsume"); - consumer(std::move(resource)); - } - - empty_.Signal(); - --inflight_; - - TRACE_FLOW_END("flutter", "PipelineItem", trace_id); - TRACE_EVENT_ASYNC_END0("flutter", "PipelineItem", trace_id); - - return items_count > 0 ? PipelineConsumeResult::MoreAvailable - : PipelineConsumeResult::Done; - } - - private: - const uint32_t depth_; - fml::Semaphore empty_; - fml::Semaphore available_; - std::atomic inflight_; - std::mutex queue_mutex_; - std::deque> queue_; - - bool ProducerCommit(ResourcePtr resource, size_t trace_id) { - { - std::scoped_lock lock(queue_mutex_); - queue_.emplace_back(std::move(resource), trace_id); - } - - // Ensure the queue mutex is not held as that would be a pessimization. - available_.Signal(); - return true; - } - - bool ProducerCommitIfEmpty(ResourcePtr resource, size_t trace_id) { - { - std::scoped_lock lock(queue_mutex_); - if (!queue_.empty()) { - // Bail if the queue is not empty, opens up spaces to produce other - // frames. - empty_.Signal(); - return false; - } - queue_.emplace_back(std::move(resource), trace_id); - } - - // Ensure the queue mutex is not held as that would be a pessimization. - available_.Signal(); - return true; - } - - FML_DISALLOW_COPY_AND_ASSIGN(Pipeline); -}; - -} // namespace flutter - -#endif // FLUTTER_SHELL_COMMON_PIPELINE_H_ diff --git a/shell/common/pipeline_unittests.cc b/shell/common/pipeline_unittests.cc deleted file mode 100644 index 0327042cc6c0e..0000000000000 --- a/shell/common/pipeline_unittests.cc +++ /dev/null @@ -1,135 +0,0 @@ -// Copyright 2013 The Flutter Authors. All rights reserved. -// 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 "flutter/shell/common/pipeline.h" - -#include -#include -#include - -#include "gtest/gtest.h" - -namespace flutter { -namespace testing { - -using IntPipeline = Pipeline; -using Continuation = IntPipeline::ProducerContinuation; - -TEST(PipelineTest, ConsumeOneVal) { - fml::RefPtr pipeline = fml::MakeRefCounted(2); - - Continuation continuation = pipeline->Produce(); - - const int test_val = 1; - bool result = continuation.Complete(std::make_unique(test_val)); - ASSERT_EQ(result, true); - - PipelineConsumeResult consume_result = pipeline->Consume( - [&test_val](std::unique_ptr v) { ASSERT_EQ(*v, test_val); }); - - ASSERT_EQ(consume_result, PipelineConsumeResult::Done); -} - -TEST(PipelineTest, ContinuationCanOnlyBeUsedOnce) { - fml::RefPtr pipeline = fml::MakeRefCounted(2); - - Continuation continuation = pipeline->Produce(); - - const int test_val = 1; - bool result = continuation.Complete(std::make_unique(test_val)); - ASSERT_EQ(result, true); - - PipelineConsumeResult consume_result_1 = pipeline->Consume( - [&test_val](std::unique_ptr v) { ASSERT_EQ(*v, test_val); }); - - result = continuation.Complete(std::make_unique(test_val)); - ASSERT_EQ(result, false); - ASSERT_EQ(consume_result_1, PipelineConsumeResult::Done); - - PipelineConsumeResult consume_result_2 = - pipeline->Consume([](std::unique_ptr v) { FAIL(); }); - - result = continuation.Complete(std::make_unique(test_val)); - ASSERT_EQ(result, false); - ASSERT_EQ(consume_result_2, PipelineConsumeResult::NoneAvailable); -} - -TEST(PipelineTest, PushingMoreThanDepthCompletesFirstSubmission) { - const int depth = 1; - fml::RefPtr pipeline = fml::MakeRefCounted(depth); - - Continuation continuation_1 = pipeline->Produce(); - Continuation continuation_2 = pipeline->Produce(); - - const int test_val_1 = 1, test_val_2 = 2; - bool result = continuation_1.Complete(std::make_unique(test_val_1)); - ASSERT_EQ(result, true); - result = continuation_2.Complete(std::make_unique(test_val_2)); - ASSERT_EQ(result, false); - - PipelineConsumeResult consume_result_1 = pipeline->Consume( - [&test_val_1](std::unique_ptr v) { ASSERT_EQ(*v, test_val_1); }); - - ASSERT_EQ(consume_result_1, PipelineConsumeResult::Done); -} - -TEST(PipelineTest, PushingMultiProcessesInOrder) { - const int depth = 2; - fml::RefPtr pipeline = fml::MakeRefCounted(depth); - - Continuation continuation_1 = pipeline->Produce(); - Continuation continuation_2 = pipeline->Produce(); - - const int test_val_1 = 1, test_val_2 = 2; - bool result = continuation_1.Complete(std::make_unique(test_val_1)); - ASSERT_EQ(result, true); - result = continuation_2.Complete(std::make_unique(test_val_2)); - ASSERT_EQ(result, true); - - PipelineConsumeResult consume_result_1 = pipeline->Consume( - [&test_val_1](std::unique_ptr v) { ASSERT_EQ(*v, test_val_1); }); - ASSERT_EQ(consume_result_1, PipelineConsumeResult::MoreAvailable); - - PipelineConsumeResult consume_result_2 = pipeline->Consume( - [&test_val_2](std::unique_ptr v) { ASSERT_EQ(*v, test_val_2); }); - ASSERT_EQ(consume_result_2, PipelineConsumeResult::Done); -} - -TEST(PipelineTest, ProduceIfEmptyDoesNotConsumeWhenQueueIsNotEmpty) { - const int depth = 2; - fml::RefPtr pipeline = fml::MakeRefCounted(depth); - - Continuation continuation_1 = pipeline->Produce(); - Continuation continuation_2 = pipeline->ProduceIfEmpty(); - - const int test_val_1 = 1, test_val_2 = 2; - bool result = continuation_1.Complete(std::make_unique(test_val_1)); - ASSERT_EQ(result, true); - result = continuation_2.Complete(std::make_unique(test_val_2)); - ASSERT_EQ(result, false); - - PipelineConsumeResult consume_result_1 = pipeline->Consume( - [&test_val_1](std::unique_ptr v) { ASSERT_EQ(*v, test_val_1); }); - ASSERT_EQ(consume_result_1, PipelineConsumeResult::Done); -} - -TEST(PipelineTest, ProduceIfEmptySuccessfulIfQueueIsEmpty) { - const int depth = 1; - fml::RefPtr pipeline = fml::MakeRefCounted(depth); - - Continuation continuation_1 = pipeline->ProduceIfEmpty(); - - const int test_val_1 = 1; - bool result = continuation_1.Complete(std::make_unique(test_val_1)); - ASSERT_EQ(result, true); - - PipelineConsumeResult consume_result_1 = pipeline->Consume( - [&test_val_1](std::unique_ptr v) { ASSERT_EQ(*v, test_val_1); }); - ASSERT_EQ(consume_result_1, PipelineConsumeResult::Done); -} - -} // namespace testing -} // namespace flutter diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 0c671b8534efa..01a287db4c3b7 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -151,8 +151,12 @@ void Rasterizer::DrawLastLayerTree() { DrawToSurface(*last_layer_tree_); } -void Rasterizer::Draw(fml::RefPtr> pipeline, +void Rasterizer::Draw(std::shared_ptr layer_tree_holder, LayerTreeDiscardCallback discardCallback) { + if (layer_tree_holder->IsEmpty()) { + // We do not have any frame to raster. + return; + } TRACE_EVENT0("flutter", "GPURasterizer::Draw"); if (raster_thread_merger_ && !raster_thread_merger_->IsOnRasterizingThread()) { @@ -163,53 +167,44 @@ void Rasterizer::Draw(fml::RefPtr> pipeline, .GetRasterTaskRunner() ->RunsTasksOnCurrentThread()); - RasterStatus raster_status = RasterStatus::kFailed; - Pipeline::Consumer consumer = - [&](std::unique_ptr layer_tree) { - if (discardCallback(*layer_tree.get())) { - raster_status = RasterStatus::kDiscarded; - } else { - raster_status = DoDraw(std::move(layer_tree)); - } - }; + std::unique_ptr layer_tree = layer_tree_holder->Pop(); - PipelineConsumeResult consume_result = pipeline->Consume(consumer); - // if the raster status is to resubmit the frame, we push the frame to the - // front of the queue and also change the consume status to more available. + RasterStatus raster_status; + if (layer_tree) { + if (discardCallback(*layer_tree.get())) { + raster_status = RasterStatus::kDiscarded; + } else { + raster_status = DoDraw(std::move(layer_tree)); + } + } else { + raster_status = RasterStatus::kFailed; + } + // Merging the thread as we know the next `Draw` should be run on the + // platform thread. auto should_resubmit_frame = raster_status == RasterStatus::kResubmit || raster_status == RasterStatus::kSkipAndRetry; if (should_resubmit_frame) { - auto front_continuation = pipeline->ProduceIfEmpty(); - bool result = - front_continuation.Complete(std::move(resubmitted_layer_tree_)); - if (result) { - consume_result = PipelineConsumeResult::MoreAvailable; - } - } else if (raster_status == RasterStatus::kEnqueuePipeline) { - consume_result = PipelineConsumeResult::MoreAvailable; + layer_tree_holder->PushIfNewer(std::move(resubmitted_layer_tree_)); + FML_DCHECK(external_view_embedder_ != nullptr) + << "kResubmit is an invalid raster status without external view " + "embedder."; } - // EndFrame should perform cleanups for the external_view_embedder. if (surface_ && external_view_embedder_) { external_view_embedder_->EndFrame(should_resubmit_frame, raster_thread_merger_); } - // Consume as many pipeline items as possible. But yield the event loop - // between successive tries. - switch (consume_result) { - case PipelineConsumeResult::MoreAvailable: { - delegate_.GetTaskRunners().GetRasterTaskRunner()->PostTask( - [weak_this = weak_factory_.GetWeakPtr(), pipeline]() { - if (weak_this) { - weak_this->Draw(pipeline); - } - }); - break; - } - default: - break; + // Note: This behaviour is left as-is to be inline with the pipeline + // semantics. TODO(kaushikiska): explore removing this block. + if (!layer_tree_holder->IsEmpty()) { + delegate_.GetTaskRunners().GetRasterTaskRunner()->PostTask( + [weak_this = weak_factory_.GetWeakPtr(), layer_tree_holder]() { + if (weak_this) { + weak_this->Draw(layer_tree_holder); + } + }); } } @@ -295,9 +290,9 @@ sk_sp Rasterizer::MakeRasterSnapshot(sk_sp picture, sk_sp Rasterizer::ConvertToRasterImage(sk_sp image) { TRACE_EVENT0("flutter", __FUNCTION__); - // If the rasterizer does not have a surface with a GrContext, then it will - // be unable to render a cross-context SkImage. The caller will need to - // create the raster image on the IO thread. + // If the rasterizer does not have a surface with a GrContext, then it + // will be unable to render a cross-context SkImage. The caller will need + // to create the raster image on the IO thread. if (surface_ == nullptr || surface_->GetContext() == nullptr) { return nullptr; } @@ -352,8 +347,8 @@ RasterStatus Rasterizer::DoDraw( } // TODO(liyuqian): in Fuchsia, the rasterization doesn't finish when - // Rasterizer::DoDraw finishes. Future work is needed to adapt the timestamp - // for Fuchsia to capture SceneUpdateContext::ExecutePaintTasks. + // Rasterizer::DoDraw finishes. Future work is needed to adapt the + // timestamp for Fuchsia to capture SceneUpdateContext::ExecutePaintTasks. const auto raster_finish_time = fml::TimePoint::Now(); timing.Set(FrameTiming::kRasterFinish, raster_finish_time); delegate_.OnFrameRasterized(timing); @@ -431,18 +426,20 @@ RasterStatus Rasterizer::DrawToSurface(flutter::LayerTree& layer_tree) { embedder_root_canvas = external_view_embedder_->GetRootCanvas(); } - // On Android, the external view embedder deletes surfaces in `BeginFrame`. + // On Android, the external view embedder deletes surfaces in + // `BeginFrame`. // // Deleting a surface also clears the GL context. Therefore, acquire the - // frame after calling `BeginFrame` as this operation resets the GL context. + // frame after calling `BeginFrame` as this operation resets the GL + // context. auto frame = surface_->AcquireFrame(layer_tree.frame_size()); if (frame == nullptr) { return RasterStatus::kFailed; } - // If the external view embedder has specified an optional root surface, the - // root surface transformation is set by the embedder instead of + // If the external view embedder has specified an optional root surface, + // the root surface transformation is set by the embedder instead of // having to apply it here. SkMatrix root_surface_transformation = embedder_root_canvas ? SkMatrix{} : surface_->GetRootTransformation(); @@ -470,7 +467,8 @@ RasterStatus Rasterizer::DrawToSurface(flutter::LayerTree& layer_tree) { raster_thread_merger_->IsMerged()) { // TODO(73620): Remove when platform views are accounted for. FML_LOG(ERROR) - << "Error: Thread merging not implemented for engines with shared " + << "Error: Thread merging not implemented for engines with " + "shared " "components.\n\n" "This is likely a result of using platform views with enigne " "groups. See " @@ -535,8 +533,8 @@ static sk_sp CreateSnapshotSurface(GrDirectContext* surface_context, const auto image_info = SkImageInfo::MakeN32Premul( size.width(), size.height(), SkColorSpace::MakeSRGB()); if (surface_context) { - // There is a rendering surface that may contain textures that are going to - // be referenced in the layer tree about to be drawn. + // There is a rendering surface that may contain textures that are going + // to be referenced in the layer tree about to be drawn. return SkSurface::MakeRenderTarget(surface_context, // SkBudgeted::kNo, // image_info // @@ -553,8 +551,8 @@ sk_sp Rasterizer::ScreenshotLayerTreeAsImage( flutter::CompositorContext& compositor_context, GrDirectContext* surface_context, bool compressed) { - // Attempt to create a snapshot surface depending on whether we have access to - // a valid GPU rendering context. + // Attempt to create a snapshot surface depending on whether we have + // access to a valid GPU rendering context. auto snapshot_surface = CreateSnapshotSurface(surface_context, tree->frame_size()); if (snapshot_surface == nullptr) { @@ -565,15 +563,15 @@ sk_sp Rasterizer::ScreenshotLayerTreeAsImage( // Draw the current layer tree into the snapshot surface. auto* canvas = snapshot_surface->getCanvas(); - // There is no root surface transformation for the screenshot layer. Reset the - // matrix to identity. + // There is no root surface transformation for the screenshot layer. Reset + // the matrix to identity. SkMatrix root_surface_transformation; root_surface_transformation.reset(); - // snapshot_surface->makeImageSnapshot needs the GL context to be set if the - // render context is GL. frame->Raster() pops the gl context in platforms that - // gl context switching are used. (For example, older iOS that uses GL) We - // reset the GL context using the context switch. + // snapshot_surface->makeImageSnapshot needs the GL context to be set if + // the render context is GL. frame->Raster() pops the gl context in + // platforms that gl context switching are used. (For example, older iOS + // that uses GL) We reset the GL context using the context switch. auto context_switch = surface_->MakeRenderContextCurrent(); if (!context_switch->GetResult()) { FML_LOG(ERROR) << "Screenshot: unable to make image screenshot"; @@ -587,7 +585,8 @@ sk_sp Rasterizer::ScreenshotLayerTreeAsImage( frame->Raster(*tree, true); canvas->flush(); - // Prepare an image from the surface, this image may potentially be on th GPU. + // Prepare an image from the surface, this image may potentially be on th + // GPU. auto potentially_gpu_snapshot = snapshot_surface->makeImageSnapshot(); if (!potentially_gpu_snapshot) { FML_LOG(ERROR) << "Screenshot: unable to make image screenshot"; @@ -601,8 +600,8 @@ sk_sp Rasterizer::ScreenshotLayerTreeAsImage( return nullptr; } - // If the caller want the pixels to be compressed, there is a Skia utility to - // compress to PNG. Use that. + // If the caller want the pixels to be compressed, there is a Skia utility + // to compress to PNG. Use that. if (compressed) { return cpu_snapshot->encodeToData(); } diff --git a/shell/common/rasterizer.h b/shell/common/rasterizer.h index b853e6628e4f8..ad869f2cbaca6 100644 --- a/shell/common/rasterizer.h +++ b/shell/common/rasterizer.h @@ -22,7 +22,7 @@ #include "flutter/fml/time/time_delta.h" #include "flutter/fml/time/time_point.h" #include "flutter/lib/ui/snapshot_delegate.h" -#include "flutter/shell/common/pipeline.h" +#include "flutter/shell/common/layer_tree_holder.h" namespace flutter { @@ -211,37 +211,30 @@ class Rasterizer final : public SnapshotDelegate { using LayerTreeDiscardCallback = std::function; //---------------------------------------------------------------------------- - /// @brief Takes the next item from the layer tree pipeline and executes - /// the raster thread frame workload for that pipeline item to - /// render a frame on the on-screen surface. + /// @brief Takes the latest item from the layer tree holder and executes + /// the raster thread frame workload for that item to render a + /// frame on the on-screen surface. /// - /// Why does the draw call take a layer tree pipeline and not the + /// Why does the draw call take a layer tree holder and not the /// layer tree directly? /// - /// The pipeline is the way book-keeping of frame workloads - /// distributed across the multiple threads is managed. The - /// rasterizer deals with the pipelines directly (instead of layer - /// trees which is what it actually renders) because the pipeline - /// consumer's workload must be accounted for within the pipeline - /// itself. If the rasterizer took the layer tree directly, it - /// would have to be taken out of the pipeline. That would signal - /// the end of the frame workload and the pipeline would be ready - /// for new frames. But the last frame has not been rendered by - /// the frame yet! On the other hand, the pipeline must own the - /// layer tree it renders because it keeps a reference to the last - /// layer tree around till a new frame is rendered. So a simple - /// reference wont work either. The `Rasterizer::DoDraw` method - /// actually performs the GPU operations within the layer tree - /// pipeline. + /// The layer tree holder is a thread safe way to produce frame + /// workloads from the UI thread and rasterize them on the raster + /// thread. To account for scenarious where the UI thread + /// continues to produce the frames while a raster task is queued, + /// `Rasterizer::DoDraw` that gets executed on the raster thread + /// must pick up the newest layer tree produced by the UI thread. + /// If we were to pass the layer tree as opposed to the holder, it + /// would result in stale frames being rendered. /// /// @see `Rasterizer::DoDraw` /// - /// @param[in] pipeline The layer tree pipeline to take the next layer tree - /// to render from. + /// @param[in] layer_tree_holder The layer tree holder to take the latest + /// layer tree to render from. /// @param[in] discardCallback if specified and returns true, the layer tree /// is discarded instead of being rendered /// - void Draw(fml::RefPtr> pipeline, + void Draw(std::shared_ptr layer_tree_holder, LayerTreeDiscardCallback discardCallback = NoDiscard); //---------------------------------------------------------------------------- @@ -451,7 +444,8 @@ class Rasterizer final : public SnapshotDelegate { std::unique_ptr last_layer_tree_; // Set when we need attempt to rasterize the layer tree again. This layer_tree // has not successfully rasterized. This can happen due to the change in the - // thread configuration. This will be inserted to the front of the pipeline. + // thread configuration. This layer tree could be rasterized again if there + // are no newer ones. std::unique_ptr resubmitted_layer_tree_; fml::closure next_frame_callback_; bool user_override_resource_cache_bytes_; diff --git a/shell/common/rasterizer_unittests.cc b/shell/common/rasterizer_unittests.cc index 3098a19256533..2b1e851091182 100644 --- a/shell/common/rasterizer_unittests.cc +++ b/shell/common/rasterizer_unittests.cc @@ -6,6 +6,7 @@ #include "flutter/shell/common/rasterizer.h" +#include "flutter/shell/common/layer_tree_holder.h" #include "flutter/shell/common/thread_host.h" #include "flutter/testing/testing.h" #include "gmock/gmock.h" @@ -90,8 +91,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)); - rasterizer->Draw(pipeline, nullptr); + rasterizer->Draw(std::make_unique(), nullptr); latch.Signal(); }); latch.Wait(); @@ -142,13 +142,12 @@ 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 layer_tree = std::make_unique(/*frame_size=*/SkISize(), /*device_pixel_ratio=*/2.0f); - bool result = pipeline->Produce().Complete(std::move(layer_tree)); - EXPECT_TRUE(result); + auto layer_tree_holder = std::make_unique(); + layer_tree_holder->PushIfNewer(std::move(layer_tree)); auto no_discard = [](LayerTree&) { return false; }; - rasterizer->Draw(pipeline, no_discard); + rasterizer->Draw(std::move(layer_tree_holder), no_discard); latch.Signal(); }); latch.Wait(); @@ -196,13 +195,12 @@ TEST( rasterizer->Setup(std::move(surface)); fml::AutoResetWaitableEvent latch; thread_host.raster_thread->GetTaskRunner()->PostTask([&] { - auto pipeline = fml::AdoptRef(new Pipeline(/*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)); - EXPECT_TRUE(result); + auto layer_tree_holder = std::make_unique(); + layer_tree_holder->PushIfNewer(std::move(std::move(layer_tree))); auto no_discard = [](LayerTree&) { return false; }; - rasterizer->Draw(pipeline, no_discard); + rasterizer->Draw(std::move(layer_tree_holder), no_discard); latch.Signal(); }); latch.Wait(); @@ -255,13 +253,12 @@ TEST( rasterizer->Setup(std::move(surface)); - auto pipeline = fml::AdoptRef(new Pipeline(/*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)); - EXPECT_TRUE(result); + auto layer_tree_holder = std::make_unique(); + layer_tree_holder->PushIfNewer(std::move(std::move(layer_tree))); auto no_discard = [](LayerTree&) { return false; }; - rasterizer->Draw(pipeline, no_discard); + rasterizer->Draw(std::move(layer_tree_holder), no_discard); } TEST(RasterizerTest, externalViewEmbedderDoesntEndFrameWhenNoSurfaceIsSet) { @@ -292,9 +289,8 @@ TEST(RasterizerTest, externalViewEmbedderDoesntEndFrameWhenNoSurfaceIsSet) { fml::AutoResetWaitableEvent latch; thread_host.raster_thread->GetTaskRunner()->PostTask([&] { - auto pipeline = fml::AdoptRef(new Pipeline(/*depth=*/10)); auto no_discard = [](LayerTree&) { return false; }; - rasterizer->Draw(pipeline, no_discard); + rasterizer->Draw(std::make_unique(), no_discard); latch.Signal(); }); latch.Wait(); diff --git a/shell/common/shell.cc b/shell/common/shell.cc index e64573a45bb61..a0106f503dffc 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1123,7 +1123,7 @@ void Shell::OnAnimatorNotifyIdle(int64_t deadline) { } // |Animator::Delegate| -void Shell::OnAnimatorDraw(fml::RefPtr> pipeline, +void Shell::OnAnimatorDraw(std::shared_ptr layer_tree_holder, fml::TimePoint frame_target_time) { FML_DCHECK(is_setup_); @@ -1146,11 +1146,12 @@ void Shell::OnAnimatorDraw(fml::RefPtr> pipeline, task_runners_.GetRasterTaskRunner()->PostTask( [&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), - discard_callback = std::move(discard_callback)]() { + rasterizer = rasterizer_->GetWeakPtr(), + discard_callback = std::move(discard_callback), + layer_tree_holder = std::move(layer_tree_holder)]() { if (rasterizer) { - rasterizer->Draw(pipeline, std::move(discard_callback)); - + rasterizer->Draw(std::move(layer_tree_holder), + std::move(discard_callback)); if (waiting_for_first_frame.load()) { waiting_for_first_frame.store(false); waiting_for_first_frame_condition.notify_all(); diff --git a/shell/common/shell.h b/shell/common/shell.h index a26c6cf53f653..8aeb5fbb72382 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -35,6 +35,7 @@ #include "flutter/shell/common/animator.h" #include "flutter/shell/common/display_manager.h" #include "flutter/shell/common/engine.h" +#include "flutter/shell/common/layer_tree_holder.h" #include "flutter/shell/common/platform_view.h" #include "flutter/shell/common/rasterizer.h" #include "flutter/shell/common/shell_io_manager.h" @@ -511,7 +512,7 @@ class Shell final : public PlatformView::Delegate, void OnAnimatorNotifyIdle(int64_t deadline) override; // |Animator::Delegate| - void OnAnimatorDraw(fml::RefPtr> pipeline, + void OnAnimatorDraw(std::shared_ptr layer_tree_holder, fml::TimePoint frame_target_time) override; // |Animator::Delegate| diff --git a/shell/common/shell_test.cc b/shell/common/shell_test.cc index 0c753f523032f..691e891c28b5d 100644 --- a/shell/common/shell_test.cc +++ b/shell/common/shell_test.cc @@ -166,7 +166,7 @@ void ShellTest::PumpOneFrame(Shell* shell, flutter::ViewportMetrics viewport_metrics, LayerTreeBuilder builder) { // Set viewport to nonempty, and call Animator::BeginFrame to make the layer - // tree pipeline nonempty. Without either of this, the layer tree below + // tree holder nonempty. Without either of this, the layer tree below // won't be rasterized. fml::AutoResetWaitableEvent latch; shell->GetTaskRunners().GetUITaskRunner()->PostTask(