Animator stopped notifying delegate to render when pipeline is not empty

shell/common/animator.cc

@@ -175,9 +175,21 @@ void Animator::Render(std::unique_ptr<flutter::LayerTree> layer_tree) {
   frame_timings_recorder_->RecordBuildEnd(fml::TimePoint::Now());
 
   // Commit the pending continuation.
-  bool result = producer_continuation_.Complete(std::move(layer_tree));
-  if (!result) {
+  PipelineProduceResult result =
+      producer_continuation_.Complete(std::move(layer_tree));
+
+  if (!result.success) {
+    frame_timings_recorder_.reset();
     FML_DLOG(INFO) << "No pending continuation to commit";
+    return;
+  }
+
+  if (!result.is_first_item) {
+    frame_timings_recorder_.reset();
+    // It has been successfully pushed to the pipeline but not as the first
+    // item. Eventually the 'Rasterizer' will consume it, so we don't need to
+    // notify the delegate.
+    return;
   }
 
   delegate_.OnAnimatorDraw(layer_tree_pipeline_,

shell/common/animator_unittests.cc

@@ -12,7 +12,9 @@
 
 #include "flutter/shell/common/shell_test.h"
 #include "flutter/shell/common/shell_test_platform_view.h"
+#include "flutter/testing/post_task_sync.h"
 #include "flutter/testing/testing.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 // CREATE_NATIVE_ENTRY is leaky by design
@@ -23,16 +25,17 @@ namespace testing {
 
 class FakeAnimatorDelegate : public Animator::Delegate {
  public:
-  void OnAnimatorBeginFrame(fml::TimePoint frame_target_time,
-                            uint64_t frame_number) override {}
+  MOCK_METHOD2(OnAnimatorBeginFrame,
+               void(fml::TimePoint frame_target_time, uint64_t frame_number));
 
   void OnAnimatorNotifyIdle(fml::TimePoint deadline) override {
     notify_idle_called_ = true;
   }
 
-  void OnAnimatorDraw(
-      std::shared_ptr<Pipeline<flutter::LayerTree>> pipeline,
-      std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder) override {}
+  MOCK_METHOD2(
+      OnAnimatorDraw,
+      void(std::shared_ptr<Pipeline<flutter::LayerTree>> pipeline,
+           std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder));
 
   void OnAnimatorDrawLastLayerTree(
       std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder) override {}
@@ -184,6 +187,63 @@ TEST_F(ShellTest, AnimatorDoesNotNotifyIdleBeforeRender) {
   latch.Wait();
 }
 
+TEST_F(ShellTest, AnimatorDoesNotNotifyDelegateIfPipelineIsNotEmpty) {
+  FakeAnimatorDelegate delegate;
+  TaskRunners task_runners = {
+      "test",
+      CreateNewThread(),  // platform
+      CreateNewThread(),  // raster
+      CreateNewThread(),  // ui
+      CreateNewThread()   // io
+  };
+
+  auto clock = std::make_shared<ShellTestVsyncClock>();
+  std::shared_ptr<Animator> animator;
+
+  auto flush_vsync_task = [&] {
+    fml::AutoResetWaitableEvent ui_latch;
+    task_runners.GetUITaskRunner()->PostTask([&] { ui_latch.Signal(); });
+    do {
+      clock->SimulateVSync();
+    } while (ui_latch.WaitWithTimeout(fml::TimeDelta::FromMilliseconds(1)));
+  };
+
+  // Create the animator on the UI task runner.
+  PostTaskSync(task_runners.GetUITaskRunner(), [&] {
+    auto vsync_waiter = static_cast<std::unique_ptr<VsyncWaiter>>(
+        std::make_unique<ShellTestVsyncWaiter>(task_runners, clock));
+    animator = std::make_unique<Animator>(delegate, task_runners,
+                                          std::move(vsync_waiter));
+  });
+
+  fml::AutoResetWaitableEvent begin_frame_latch;
+  EXPECT_CALL(delegate, OnAnimatorBeginFrame)
+      .WillRepeatedly(
+          [&](fml::TimePoint frame_target_time, uint64_t frame_number) {
+            begin_frame_latch.Signal();
+          });
+
+  // It will only be called once even though we call the method Animator::Render
+  // twice. because it will only be called when the pipeline is empty.
+  EXPECT_CALL(delegate, OnAnimatorDraw).Times(1);
+
+  for (int i = 0; i < 2; i++) {
+    task_runners.GetUITaskRunner()->PostTask([&] {
+      animator->RequestFrame();
+      task_runners.GetPlatformTaskRunner()->PostTask(flush_vsync_task);
+    });
+    begin_frame_latch.Wait();
+
+    PostTaskSync(task_runners.GetUITaskRunner(), [&] {
+      auto layer_tree =
+          std::make_unique<LayerTree>(SkISize::Make(600, 800), 1.0);
+      animator->Render(std::move(layer_tree));
+    });
+  }
+
+  PostTaskSync(task_runners.GetUITaskRunner(), [&] { animator.reset(); });
+}
+
 }  // namespace testing
 }  // namespace flutter
 

shell/common/pipeline.h

@@ -16,6 +16,14 @@
 
 namespace flutter {
 
+struct PipelineProduceResult {
+  // Whether the item was successfully pushed into the pipeline.
+  bool success = false;
+  // Whether it is the first item of the pipeline. Only valid when 'success' is
+  // 'true'.
+  bool is_first_item = false;
+};
+
 enum class PipelineConsumeResult {
   NoneAvailable,
   Done,
@@ -60,8 +68,8 @@ class Pipeline {
       }
     }
 
-    [[nodiscard]] bool Complete(ResourcePtr resource) {
-      bool result = false;
+    [[nodiscard]] PipelineProduceResult Complete(ResourcePtr resource) {
+      PipelineProduceResult result;
       if (continuation_) {
         result = continuation_(std::move(resource), trace_id_);
         continuation_ = nullptr;
@@ -75,7 +83,8 @@ class Pipeline {
 
    private:
     friend class Pipeline;
-    using Continuation = std::function<bool(ResourcePtr, size_t)>;
+    using Continuation =
+        std::function<PipelineProduceResult(ResourcePtr, size_t)>;
 
     Continuation continuation_;
     size_t trace_id_;
@@ -179,32 +188,35 @@ class Pipeline {
   std::mutex queue_mutex_;
   std::deque<std::pair<ResourcePtr, size_t>> queue_;
 
-  bool ProducerCommit(ResourcePtr resource, size_t trace_id) {
+  PipelineProduceResult ProducerCommit(ResourcePtr resource, size_t trace_id) {
+    bool is_first_item = false;
     {
       std::scoped_lock lock(queue_mutex_);
+      is_first_item = queue_.empty();
       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;
+    return {.success = true, .is_first_item = is_first_item};
   }
 
-  bool ProducerCommitIfEmpty(ResourcePtr resource, size_t trace_id) {
+  PipelineProduceResult 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;
+        return {.success = false, .is_first_item = 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;
+    return {.success = true, .is_first_item = true};
   }
 
   FML_DISALLOW_COPY_AND_ASSIGN(Pipeline);

shell/common/pipeline_unittests.cc

@@ -24,8 +24,10 @@ TEST(PipelineTest, ConsumeOneVal) {
   Continuation continuation = pipeline->Produce();
 
   const int test_val = 1;
-  bool result = continuation.Complete(std::make_unique<int>(test_val));
-  ASSERT_EQ(result, true);
+  PipelineProduceResult result =
+      continuation.Complete(std::make_unique<int>(test_val));
+  ASSERT_EQ(result.success, true);
+  ASSERT_EQ(result.is_first_item, true);
 
   PipelineConsumeResult consume_result = pipeline->Consume(
       [&test_val](std::unique_ptr<int> v) { ASSERT_EQ(*v, test_val); });
@@ -39,21 +41,23 @@ TEST(PipelineTest, ContinuationCanOnlyBeUsedOnce) {
   Continuation continuation = pipeline->Produce();
 
   const int test_val = 1;
-  bool result = continuation.Complete(std::make_unique<int>(test_val));
-  ASSERT_EQ(result, true);
+  PipelineProduceResult result =
+      continuation.Complete(std::make_unique<int>(test_val));
+  ASSERT_EQ(result.success, true);
+  ASSERT_EQ(result.is_first_item, true);
 
   PipelineConsumeResult consume_result_1 = pipeline->Consume(
       [&test_val](std::unique_ptr<int> v) { ASSERT_EQ(*v, test_val); });
 
   result = continuation.Complete(std::make_unique<int>(test_val));
-  ASSERT_EQ(result, false);
+  ASSERT_EQ(result.success, false);
   ASSERT_EQ(consume_result_1, PipelineConsumeResult::Done);
 
   PipelineConsumeResult consume_result_2 =
       pipeline->Consume([](std::unique_ptr<int> v) { FAIL(); });
 
   result = continuation.Complete(std::make_unique<int>(test_val));
-  ASSERT_EQ(result, false);
+  ASSERT_EQ(result.success, false);
   ASSERT_EQ(consume_result_2, PipelineConsumeResult::NoneAvailable);
 }
 
@@ -65,10 +69,12 @@ TEST(PipelineTest, PushingMoreThanDepthCompletesFirstSubmission) {
   Continuation continuation_2 = pipeline->Produce();
 
   const int test_val_1 = 1, test_val_2 = 2;
-  bool result = continuation_1.Complete(std::make_unique<int>(test_val_1));
-  ASSERT_EQ(result, true);
+  PipelineProduceResult result =
+      continuation_1.Complete(std::make_unique<int>(test_val_1));
+  ASSERT_EQ(result.success, true);
+  ASSERT_EQ(result.is_first_item, true);
   result = continuation_2.Complete(std::make_unique<int>(test_val_2));
-  ASSERT_EQ(result, false);
+  ASSERT_EQ(result.success, false);
 
   PipelineConsumeResult consume_result_1 = pipeline->Consume(
       [&test_val_1](std::unique_ptr<int> v) { ASSERT_EQ(*v, test_val_1); });
@@ -84,10 +90,13 @@ TEST(PipelineTest, PushingMultiProcessesInOrder) {
   Continuation continuation_2 = pipeline->Produce();
 
   const int test_val_1 = 1, test_val_2 = 2;
-  bool result = continuation_1.Complete(std::make_unique<int>(test_val_1));
-  ASSERT_EQ(result, true);
+  PipelineProduceResult result =
+      continuation_1.Complete(std::make_unique<int>(test_val_1));
+  ASSERT_EQ(result.success, true);
+  ASSERT_EQ(result.is_first_item, true);
   result = continuation_2.Complete(std::make_unique<int>(test_val_2));
-  ASSERT_EQ(result, true);
+  ASSERT_EQ(result.success, true);
+  ASSERT_EQ(result.is_first_item, false);
 
   PipelineConsumeResult consume_result_1 = pipeline->Consume(
       [&test_val_1](std::unique_ptr<int> v) { ASSERT_EQ(*v, test_val_1); });
@@ -106,10 +115,12 @@ TEST(PipelineTest, ProduceIfEmptyDoesNotConsumeWhenQueueIsNotEmpty) {
   Continuation continuation_2 = pipeline->ProduceIfEmpty();
 
   const int test_val_1 = 1, test_val_2 = 2;
-  bool result = continuation_1.Complete(std::make_unique<int>(test_val_1));
-  ASSERT_EQ(result, true);
+  PipelineProduceResult result =
+      continuation_1.Complete(std::make_unique<int>(test_val_1));
+  ASSERT_EQ(result.success, true);
+  ASSERT_EQ(result.is_first_item, true);
   result = continuation_2.Complete(std::make_unique<int>(test_val_2));
-  ASSERT_EQ(result, false);
+  ASSERT_EQ(result.success, false);
 
   PipelineConsumeResult consume_result_1 = pipeline->Consume(
       [&test_val_1](std::unique_ptr<int> v) { ASSERT_EQ(*v, test_val_1); });
@@ -123,8 +134,10 @@ TEST(PipelineTest, ProduceIfEmptySuccessfulIfQueueIsEmpty) {
   Continuation continuation_1 = pipeline->ProduceIfEmpty();
 
   const int test_val_1 = 1;
-  bool result = continuation_1.Complete(std::make_unique<int>(test_val_1));
-  ASSERT_EQ(result, true);
+  PipelineProduceResult result =
+      continuation_1.Complete(std::make_unique<int>(test_val_1));
+  ASSERT_EQ(result.success, true);
+  ASSERT_EQ(result.is_first_item, true);
 
   PipelineConsumeResult consume_result_1 = pipeline->Consume(
       [&test_val_1](std::unique_ptr<int> v) { ASSERT_EQ(*v, test_val_1); });

shell/common/rasterizer.cc

@@ -198,9 +198,9 @@ RasterStatus Rasterizer::Draw(
   bool should_resubmit_frame = ShouldResubmitFrame(raster_status);
   if (should_resubmit_frame) {
     auto front_continuation = pipeline->ProduceIfEmpty();
-    bool result =
+    PipelineProduceResult result =
         front_continuation.Complete(std::move(resubmitted_layer_tree_));
-    if (result) {
+    if (result.success) {
       consume_result = PipelineConsumeResult::MoreAvailable;
     }
   } else if (raster_status == RasterStatus::kEnqueuePipeline) {