Multiview pipeline Pt. 2: Split out EndFrame

lib/ui/painting/image_dispose_unittests.cc

@@ -5,6 +5,7 @@
 #define FML_USED_ON_EMBEDDER
 
 #include "flutter/common/task_runners.h"
+#include "flutter/fml/synchronization/count_down_latch.h"
 #include "flutter/fml/synchronization/waitable_event.h"
 #include "flutter/lib/ui/painting/canvas.h"
 #include "flutter/lib/ui/painting/image.h"
@@ -57,6 +58,10 @@ TEST_F(ImageDisposeTest, ImageReleasedAfterFrameAndDisposePictureAndLayer) {
   };
 
   Settings settings = CreateSettingsForFixture();
+  fml::CountDownLatch frame_latch{2};
+  settings.frame_rasterized_callback = [&frame_latch](const FrameTiming& t) {
+    frame_latch.CountDown();
+  };
   auto task_runner = CreateNewThread();
   TaskRunners task_runners("test",                  // label
                            GetCurrentTaskRunner(),  // platform
@@ -83,12 +88,15 @@ TEST_F(ImageDisposeTest, ImageReleasedAfterFrameAndDisposePictureAndLayer) {
   shell->RunEngine(std::move(configuration), [&](auto result) {
     ASSERT_EQ(result, Engine::RunStatus::Success);
   });
-
   message_latch_.Wait();
 
   ASSERT_TRUE(current_display_list_);
   ASSERT_TRUE(current_image_);
 
+  // Wait for 2 frames to be rasterized. The 2nd frame releases resources of the
+  // 1st frame.
+  frame_latch.Wait();
+
   // Force a drain the SkiaUnrefQueue. The engine does this normally as frames
   // pump, but we force it here to make the test more deterministic.
   message_latch_.Reset();

shell/common/animator.cc

@@ -60,6 +60,10 @@ void Animator::EnqueueTraceFlowId(uint64_t trace_flow_id) {
 
 void Animator::BeginFrame(
     std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder) {
+  // Both frame_timings_recorder_ and layer_trees_tasks_ must be empty if not
+  // between BeginFrame and EndFrame.
+  FML_DCHECK(frame_timings_recorder_ == nullptr);
+  FML_DCHECK(layer_trees_tasks_.empty());
   TRACE_EVENT_ASYNC_END0("flutter", "Frame Request Pending",
                          frame_request_number_);
   frame_request_number_++;
@@ -112,6 +116,33 @@ void Animator::BeginFrame(
   dart_frame_deadline_ = frame_target_time.ToEpochDelta();
   uint64_t frame_number = frame_timings_recorder_->GetFrameNumber();
   delegate_.OnAnimatorBeginFrame(frame_target_time, frame_number);
+}
+
+void Animator::EndFrame() {
+  FML_CHECK(frame_timings_recorder_ != nullptr);
+  if (!layer_trees_tasks_.empty()) {
+    // The build is completed in OnAnimatorBeginFrame.
+    frame_timings_recorder_->RecordBuildEnd(fml::TimePoint::Now());
+
+    delegate_.OnAnimatorUpdateLatestFrameTargetTime(
+        frame_timings_recorder_->GetVsyncTargetTime());
+
+    // Commit the pending continuation.
+    PipelineProduceResult result =
+        producer_continuation_.Complete(std::make_unique<FrameItem>(
+            std::move(layer_trees_tasks_), std::move(frame_timings_recorder_)));
+
+    if (!result.success) {
+      FML_DLOG(INFO) << "Failed to commit to the pipeline";
+    } else if (!result.is_first_item) {
+      // Do nothing. 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.
+    } else {
+      delegate_.OnAnimatorDraw(layer_tree_pipeline_);
+    }
+  }
+  frame_timings_recorder_ = nullptr;  // Ensure it's cleared.
 
   if (!frame_scheduled_ && has_rendered_) {
     // Wait a tad more than 3 60hz frames before reporting a big idle period.
@@ -139,6 +170,10 @@ void Animator::BeginFrame(
         },
         kNotifyIdleTaskWaitTime);
   }
+  // Both frame_timings_recorder_ and layer_trees_tasks_ must be empty if not
+  // between BeginFrame and EndFrame.
+  FML_DCHECK(layer_trees_tasks_.empty());
+  FML_DCHECK(frame_timings_recorder_ == nullptr);
 }
 
 void Animator::Render(std::unique_ptr<flutter::LayerTree> layer_tree,
@@ -150,38 +185,15 @@ void Animator::Render(std::unique_ptr<flutter::LayerTree> layer_tree,
   }
   has_rendered_ = true;
 
+  // TODO(dkwingsmt): Use a real view ID when Animator supports multiple views.
+  int64_t view_id = kFlutterImplicitViewId;
+
   TRACE_EVENT_WITH_FRAME_NUMBER(frame_timings_recorder_, "flutter",
                                 "Animator::Render", /*flow_id_count=*/0,
                                 /*flow_ids=*/nullptr);
-  frame_timings_recorder_->RecordBuildEnd(fml::TimePoint::Now());
 
-  delegate_.OnAnimatorUpdateLatestFrameTargetTime(
-      frame_timings_recorder_->GetVsyncTargetTime());
-
-  // TODO(dkwingsmt): Currently only supports a single window.
-  // See https://github.com/flutter/flutter/issues/135530, item 2.
-  int64_t view_id = kFlutterImplicitViewId;
-  std::vector<std::unique_ptr<LayerTreeTask>> layer_trees_tasks;
-  layer_trees_tasks.push_back(std::make_unique<LayerTreeTask>(
+  layer_trees_tasks_.push_back(std::make_unique<LayerTreeTask>(
       view_id, std::move(layer_tree), device_pixel_ratio));
-  // Commit the pending continuation.
-  PipelineProduceResult result =
-      producer_continuation_.Complete(std::make_unique<FrameItem>(
-          std::move(layer_trees_tasks), std::move(frame_timings_recorder_)));
-
-  if (!result.success) {
-    FML_DLOG(INFO) << "No pending continuation to commit";
-    return;
-  }
-
-  if (!result.is_first_item) {
-    // 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_);
 }
 
 const std::weak_ptr<VsyncWaiter> Animator::GetVsyncWaiter() const {
@@ -253,6 +265,7 @@ void Animator::AwaitVSync() {
             self->DrawLastLayerTrees(std::move(frame_timings_recorder));
           } else {
             self->BeginFrame(std::move(frame_timings_recorder));
+            self->EndFrame();
           }
         }
       });

shell/common/animator.h

@@ -90,7 +90,13 @@ class Animator final {
   void EnqueueTraceFlowId(uint64_t trace_flow_id);
 
  private:
+  // Animator's work during a vsync is split into two methods, BeginFrame and
+  // EndFrame. The two methods should be called synchronously back-to-back to
+  // avoid being interrupted by a regular vsync. The reason to split them is to
+  // allow ShellTest::PumpOneFrame to insert a Render in between.
+
   void BeginFrame(std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder);
+  void EndFrame();
 
   bool CanReuseLastLayerTrees();
 
@@ -107,6 +113,7 @@ class Animator final {
   std::shared_ptr<VsyncWaiter> waiter_;
 
   std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder_;
+  std::vector<std::unique_ptr<LayerTreeTask>> layer_trees_tasks_;
   uint64_t frame_request_number_ = 1;
   fml::TimeDelta dart_frame_deadline_;
   std::shared_ptr<FramePipeline> layer_tree_pipeline_;

shell/common/input_events_unittests.cc

@@ -127,11 +127,11 @@ static void TestSimulatedInputEvents(
         ShellTest::DispatchFakePointerData(shell.get());
         i += 1;
       }
-      ShellTest::VSyncFlush(shell.get(), will_draw_new_frame);
+      ShellTest::VSyncFlush(shell.get(), &will_draw_new_frame);
     }
     // Finally, issue a vsync for the pending event that may be generated duing
     // the last vsync.
-    ShellTest::VSyncFlush(shell.get(), will_draw_new_frame);
+    ShellTest::VSyncFlush(shell.get(), &will_draw_new_frame);
   });
 
   simulation.wait();
@@ -346,7 +346,7 @@ TEST_F(ShellTest, CanCorrectlyPipePointerPacket) {
   packet->SetPointerData(5, data);
   ShellTest::DispatchPointerData(shell.get(), std::move(packet));
   bool will_draw_new_frame;
-  ShellTest::VSyncFlush(shell.get(), will_draw_new_frame);
+  ShellTest::VSyncFlush(shell.get(), &will_draw_new_frame);
 
   reportLatch.Wait();
   size_t expect_length = 6;
@@ -408,7 +408,7 @@ TEST_F(ShellTest, CanCorrectlySynthesizePointerPacket) {
   packet->SetPointerData(3, data);
   ShellTest::DispatchPointerData(shell.get(), std::move(packet));
   bool will_draw_new_frame;
-  ShellTest::VSyncFlush(shell.get(), will_draw_new_frame);
+  ShellTest::VSyncFlush(shell.get(), &will_draw_new_frame);
 
   reportLatch.Wait();
   size_t expect_length = 6;

shell/common/shell_test.cc

@@ -22,6 +22,39 @@ namespace testing {
 
 constexpr int64_t kImplicitViewId = 0;
 
+FrameContent ViewContent::NoViews() {
+  return std::map<int64_t, ViewContent>();
+}
+
+FrameContent ViewContent::DummyView(double width, double height) {
+  FrameContent result;
+  result[kImplicitViewId] = ViewContent{
+      .viewport_metrics = {1.0, width, height, 22, 0},
+      .builder = {},
+  };
+  return result;
+}
+
+FrameContent ViewContent::DummyView(flutter::ViewportMetrics viewport_metrics) {
+  FrameContent result;
+  result[kImplicitViewId] = ViewContent{
+      .viewport_metrics = std::move(viewport_metrics),
+      .builder = {},
+  };
+  return result;
+}
+
+FrameContent ViewContent::ImplicitView(double width,
+                                       double height,
+                                       LayerTreeBuilder builder) {
+  FrameContent result;
+  result[kImplicitViewId] = ViewContent{
+      .viewport_metrics = {1.0, width, height, 22, 0},
+      .builder = std::move(builder),
+  };
+  return result;
+}
+
 ShellTest::ShellTest()
     : thread_host_("io.flutter.test." + GetCurrentTestName() + ".",
                    ThreadHost::Type::kPlatform | ThreadHost::Type::kIo |
@@ -92,16 +125,18 @@ void ShellTest::RestartEngine(Shell* shell, RunConfiguration configuration) {
   ASSERT_TRUE(restarted.get_future().get());
 }
 
-void ShellTest::VSyncFlush(Shell* shell, bool& will_draw_new_frame) {
+void ShellTest::VSyncFlush(Shell* shell, bool* will_draw_new_frame) {
   fml::AutoResetWaitableEvent latch;
   fml::TaskRunner::RunNowOrPostTask(
       shell->GetTaskRunners().GetPlatformTaskRunner(),
-      [shell, &will_draw_new_frame, &latch] {
+      [shell, will_draw_new_frame, &latch] {
         // The following UI task ensures that all previous UI tasks are flushed.
         fml::AutoResetWaitableEvent ui_latch;
         shell->GetTaskRunners().GetUITaskRunner()->PostTask(
-            [&ui_latch, &will_draw_new_frame]() {
-              will_draw_new_frame = true;
+            [&ui_latch, will_draw_new_frame]() {
+              if (will_draw_new_frame != nullptr) {
+                *will_draw_new_frame = true;
+              }
               ui_latch.Signal();
             });
 
@@ -154,6 +189,7 @@ void ShellTest::SetViewportMetrics(Shell* shell, double width, double height) {
               std::make_unique<FrameTimingsRecorder>();
           recorder->RecordVsync(frame_begin_time, frame_end_time);
           engine->animator_->BeginFrame(std::move(recorder));
+          engine->animator_->EndFrame();
         }
         latch.Signal();
       });
@@ -172,52 +208,57 @@ void ShellTest::NotifyIdle(Shell* shell, fml::TimeDelta deadline) {
   latch.Wait();
 }
 
-void ShellTest::PumpOneFrame(Shell* shell,
-                             double width,
-                             double height,
-                             LayerTreeBuilder builder) {
-  PumpOneFrame(shell, {1.0, width, height, 22, 0}, std::move(builder));
+void ShellTest::PumpOneFrame(Shell* shell) {
+  PumpOneFrame(shell, ViewContent::DummyView());
 }
 
-void ShellTest::PumpOneFrame(Shell* shell,
-                             const flutter::ViewportMetrics& viewport_metrics,
-                             LayerTreeBuilder builder) {
+void ShellTest::PumpOneFrame(Shell* shell, FrameContent frame_content) {
   // Set viewport to nonempty, and call Animator::BeginFrame to make the layer
   // tree pipeline nonempty. Without either of this, the layer tree below
   // won't be rasterized.
   fml::AutoResetWaitableEvent latch;
+  fml::WeakPtr<RuntimeDelegate> runtime_delegate = shell->weak_engine_;
   shell->GetTaskRunners().GetUITaskRunner()->PostTask(
-      [&latch, engine = shell->weak_engine_, viewport_metrics]() {
-        engine->SetViewportMetrics(kImplicitViewId, viewport_metrics);
+      [&latch, engine = shell->weak_engine_, &frame_content,
+       runtime_delegate]() {
+        for (auto& [view_id, view_content] : frame_content) {
+          engine->SetViewportMetrics(view_id, view_content.viewport_metrics);
+        }
         const auto frame_begin_time = fml::TimePoint::Now();
         const auto frame_end_time =
             frame_begin_time + fml::TimeDelta::FromSecondsF(1.0 / 60.0);
         std::unique_ptr<FrameTimingsRecorder> recorder =
             std::make_unique<FrameTimingsRecorder>();
         recorder->RecordVsync(frame_begin_time, frame_end_time);
         engine->animator_->BeginFrame(std::move(recorder));
-        latch.Signal();
-      });
-  latch.Wait();
 
-  latch.Reset();
-  // Call |Render| to rasterize a layer tree and trigger |OnFrameRasterized|
-  fml::WeakPtr<RuntimeDelegate> runtime_delegate = shell->weak_engine_;
-  shell->GetTaskRunners().GetUITaskRunner()->PostTask(
-      [&latch, runtime_delegate, &builder, viewport_metrics]() {
-        SkMatrix identity;
-        identity.setIdentity();
-        auto root_layer = std::make_shared<TransformLayer>(identity);
-        auto layer_tree = std::make_unique<LayerTree>(
-            LayerTree::Config{.root_layer = root_layer},
-            SkISize::Make(viewport_metrics.physical_width,
-                          viewport_metrics.physical_height));
-        float device_pixel_ratio =
-            static_cast<float>(viewport_metrics.device_pixel_ratio);
-        if (builder) {
-          builder(root_layer);
+        // The BeginFrame phase and the EndFrame phase must be performed in a
+        // single task, otherwise a normal vsync might be inserted in between,
+        // causing flaky assertion errors.
+
+        // TODO(dkwingsmt): The rendering system only supports single view for
+        // now. Remove the following two `FML_DCHECK`s and add view ID to
+        // runtime_delegate->Render after the rendering system supports multiple
+        // views.
+        FML_DCHECK(frame_content.size() <= 1);
+
+        for (auto& [view_id, view_content] : frame_content) {
+          FML_DCHECK(view_id == kImplicitViewId);
+          SkMatrix identity;
+          identity.setIdentity();
+          auto root_layer = std::make_shared<TransformLayer>(identity);
+          auto layer_tree = std::make_unique<LayerTree>(
+              LayerTree::Config{.root_layer = root_layer},
+              SkISize::Make(view_content.viewport_metrics.physical_width,
+                            view_content.viewport_metrics.physical_height));
+          float device_pixel_ratio = static_cast<float>(
+              view_content.viewport_metrics.device_pixel_ratio);
+          if (view_content.builder) {
+            view_content.builder(root_layer);
+          }
+          runtime_delegate->Render(std::move(layer_tree), device_pixel_ratio);
         }
-        runtime_delegate->Render(std::move(layer_tree), device_pixel_ratio);
+        engine->animator_->EndFrame();
         latch.Signal();
       });
   latch.Wait();