Move Rasterizer::Draw's discard_callback to Delegate (flutter#44813)

This PR `Rasterizer::Draw`'s `discard_callback` parameter, which is assigned by `Shell`, to `Delegate`, which is also implemented by `Shell`. This refactory makes the API cleaner. ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I signed the [CLA]. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
izouxv · Aug 23, 2023 · 48a5226 · 48a5226
1 parent f48bdba
commit 48a5226
Show file tree

Hide file tree

Showing 5 changed files with 62 additions and 62 deletions.
diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc
@@ -195,8 +195,7 @@ void Rasterizer::DrawLastLayerTree(
 }
 
 RasterStatus Rasterizer::Draw(
-    const std::shared_ptr<LayerTreePipeline>& pipeline,
-    LayerTreeDiscardCallback discard_callback) {
+    const std::shared_ptr<LayerTreePipeline>& pipeline) {
   TRACE_EVENT0("flutter", "GPURasterizer::Draw");
   if (raster_thread_merger_ &&
       !raster_thread_merger_->IsOnRasterizingThread()) {
@@ -209,15 +208,16 @@ RasterStatus Rasterizer::Draw(
 
   RasterStatus raster_status = RasterStatus::kFailed;
   LayerTreePipeline::Consumer consumer =
-      [&](std::unique_ptr<LayerTreeItem> item) {
+      [&raster_status, this,
+       &delegate = delegate_](std::unique_ptr<LayerTreeItem> item) {
         // TODO(dkwingsmt): Use a proper view ID when Rasterizer supports
         // multi-view.
         int64_t view_id = kFlutterImplicitViewId;
         std::unique_ptr<LayerTree> layer_tree = std::move(item->layer_tree);
         std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder =
             std::move(item->frame_timings_recorder);
         float device_pixel_ratio = item->device_pixel_ratio;
-        if (discard_callback(view_id, *layer_tree.get())) {
+        if (delegate.ShouldDiscardLayerTree(view_id, *layer_tree.get())) {
           raster_status = RasterStatus::kDiscarded;
         } else {
           raster_status = DoDraw(std::move(frame_timings_recorder),
@@ -259,13 +259,11 @@ RasterStatus Rasterizer::Draw(
   switch (consume_result) {
     case PipelineConsumeResult::MoreAvailable: {
       delegate_.GetTaskRunners().GetRasterTaskRunner()->PostTask(
-          fml::MakeCopyable(
-              [weak_this = weak_factory_.GetWeakPtr(), pipeline,
-               discard_callback = std::move(discard_callback)]() mutable {
-                if (weak_this) {
-                  weak_this->Draw(pipeline, std::move(discard_callback));
-                }
-              }));
+          [weak_this = weak_factory_.GetWeakPtr(), pipeline]() {
+            if (weak_this) {
+              weak_this->Draw(pipeline);
+            }
+          });
       break;
     }
     default:

diff --git a/shell/common/rasterizer.h b/shell/common/rasterizer.h
@@ -119,6 +119,9 @@ class Rasterizer final : public SnapshotDelegate,
         const = 0;
 
     virtual const Settings& GetSettings() const = 0;
+
+    virtual bool ShouldDiscardLayerTree(int64_t view_id,
+                                        const flutter::LayerTree& tree) = 0;
   };
 
   //----------------------------------------------------------------------------
@@ -254,9 +257,6 @@ class Rasterizer final : public SnapshotDelegate,
 
   std::shared_ptr<flutter::TextureRegistry> GetTextureRegistry() override;
 
-  using LayerTreeDiscardCallback =
-      std::function<bool(int64_t, flutter::LayerTree&)>;
-
   //----------------------------------------------------------------------------
   /// @brief      Takes the next item from the layer tree pipeline and executes
   ///             the raster thread frame workload for that pipeline item to
@@ -285,11 +285,8 @@ class Rasterizer final : public SnapshotDelegate,
   ///
   /// @param[in]  pipeline  The layer tree pipeline to take the next layer tree
   ///                       to render from.
-  /// @param[in]  discard_callback if specified and returns true, the layer tree
-  ///                             is discarded instead of being rendered
   ///
-  RasterStatus Draw(const std::shared_ptr<LayerTreePipeline>& pipeline,
-                    LayerTreeDiscardCallback discard_callback = NoDiscard);
+  RasterStatus Draw(const std::shared_ptr<LayerTreePipeline>& pipeline);
 
   //----------------------------------------------------------------------------
   /// @brief      The type of the screenshot to obtain of the previously
@@ -585,9 +582,6 @@ class Rasterizer final : public SnapshotDelegate,
 
   void FireNextFrameCallbackIfPresent();
 
-  static bool NoDiscard(int64_t view_id, const flutter::LayerTree& layer_tree) {
-    return false;
-  }
   static bool ShouldResubmitFrame(const RasterStatus& raster_status);
 
   Delegate& delegate_;

diff --git a/shell/common/rasterizer_unittests.cc b/shell/common/rasterizer_unittests.cc
@@ -43,6 +43,8 @@ class MockDelegate : public Rasterizer::Delegate {
   MOCK_CONST_METHOD0(GetIsGpuDisabledSyncSwitch,
                      std::shared_ptr<const fml::SyncSwitch>());
   MOCK_CONST_METHOD0(GetSettings, const Settings&());
+  MOCK_METHOD2(ShouldDiscardLayerTree,
+               bool(int64_t, const flutter::LayerTree&));
 };
 
 class MockSurface : public Surface {
@@ -129,7 +131,8 @@ TEST(RasterizerTest, drawEmptyPipeline) {
   fml::AutoResetWaitableEvent latch;
   thread_host.raster_thread->GetTaskRunner()->PostTask([&] {
     auto pipeline = std::make_shared<LayerTreePipeline>(/*depth=*/10);
-    rasterizer->Draw(pipeline, nullptr);
+    ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false));
+    rasterizer->Draw(pipeline);
     latch.Signal();
   });
   latch.Wait();
@@ -199,8 +202,8 @@ TEST(RasterizerTest,
     PipelineProduceResult result =
         pipeline->Produce().Complete(std::move(layer_tree_item));
     EXPECT_TRUE(result.success);
-    auto no_discard = [](int64_t, LayerTree&) { return false; };
-    rasterizer->Draw(pipeline, no_discard);
+    ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false));
+    rasterizer->Draw(pipeline);
     latch.Signal();
   });
   latch.Wait();
@@ -265,8 +268,8 @@ TEST(
     PipelineProduceResult result =
         pipeline->Produce().Complete(std::move(layer_tree_item));
     EXPECT_TRUE(result.success);
-    auto no_discard = [](int64_t, LayerTree&) { return false; };
-    rasterizer->Draw(pipeline, no_discard);
+    ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false));
+    rasterizer->Draw(pipeline);
     latch.Signal();
   });
   latch.Wait();
@@ -336,8 +339,8 @@ TEST(
   PipelineProduceResult result =
       pipeline->Produce().Complete(std::move(layer_tree_item));
   EXPECT_TRUE(result.success);
-  auto no_discard = [](int64_t, LayerTree&) { return false; };
-  rasterizer->Draw(pipeline, no_discard);
+  ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false));
+  rasterizer->Draw(pipeline);
 }
 
 TEST(RasterizerTest,
@@ -410,11 +413,11 @@ TEST(RasterizerTest,
   PipelineProduceResult result =
       pipeline->Produce().Complete(std::move(layer_tree_item));
   EXPECT_TRUE(result.success);
-  auto no_discard = [](int64_t, LayerTree&) { return false; };
 
   // The Draw() will respectively call BeginFrame(), SubmitFrame() and
   // EndFrame() one time.
-  rasterizer->Draw(pipeline, no_discard);
+  ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false));
+  rasterizer->Draw(pipeline);
 
   // The DrawLastLayerTree() will respectively call BeginFrame(), SubmitFrame()
   // and EndFrame() one more time, totally 2 times.
@@ -460,8 +463,8 @@ TEST(RasterizerTest, externalViewEmbedderDoesntEndFrameWhenNoSurfaceIsSet) {
     PipelineProduceResult result =
         pipeline->Produce().Complete(std::move(layer_tree_item));
     EXPECT_TRUE(result.success);
-    auto no_discard = [](int64_t, LayerTree&) { return false; };
-    rasterizer->Draw(pipeline, no_discard);
+    ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false));
+    rasterizer->Draw(pipeline);
     latch.Signal();
   });
   latch.Wait();
@@ -517,8 +520,8 @@ TEST(RasterizerTest, externalViewEmbedderDoesntEndFrameWhenNotUsedThisFrame) {
         pipeline->Produce().Complete(std::move(layer_tree_item));
     EXPECT_TRUE(result.success);
     // Always discard the layer tree.
-    auto discard_callback = [](int64_t, LayerTree&) { return true; };
-    RasterStatus status = rasterizer->Draw(pipeline, discard_callback);
+    ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(true));
+    RasterStatus status = rasterizer->Draw(pipeline);
     EXPECT_EQ(status, RasterStatus::kDiscarded);
     latch.Signal();
   });
@@ -561,8 +564,8 @@ TEST(RasterizerTest, externalViewEmbedderDoesntEndFrameWhenPipelineIsEmpty) {
   fml::AutoResetWaitableEvent latch;
   thread_host.raster_thread->GetTaskRunner()->PostTask([&] {
     auto pipeline = std::make_shared<LayerTreePipeline>(/*depth=*/10);
-    auto no_discard = [](int64_t, LayerTree&) { return false; };
-    RasterStatus status = rasterizer->Draw(pipeline, no_discard);
+    ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false));
+    RasterStatus status = rasterizer->Draw(pipeline);
     EXPECT_EQ(status, RasterStatus::kFailed);
     latch.Signal();
   });
@@ -619,8 +622,8 @@ TEST(RasterizerTest,
     PipelineProduceResult result =
         pipeline->Produce().Complete(std::move(layer_tree_item));
     EXPECT_TRUE(result.success);
-    auto no_discard = [](int64_t, LayerTree&) { return false; };
-    rasterizer->Draw(pipeline, no_discard);
+    ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false));
+    rasterizer->Draw(pipeline);
     latch.Signal();
   });
   latch.Wait();
@@ -677,8 +680,8 @@ TEST(
     PipelineProduceResult result =
         pipeline->Produce().Complete(std::move(layer_tree_item));
     EXPECT_TRUE(result.success);
-    auto no_discard = [](int64_t, LayerTree&) { return false; };
-    RasterStatus status = rasterizer->Draw(pipeline, no_discard);
+    ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false));
+    RasterStatus status = rasterizer->Draw(pipeline);
     EXPECT_EQ(status, RasterStatus::kSuccess);
     latch.Signal();
   });
@@ -735,8 +738,8 @@ TEST(
     PipelineProduceResult result =
         pipeline->Produce().Complete(std::move(layer_tree_item));
     EXPECT_TRUE(result.success);
-    auto no_discard = [](int64_t, LayerTree&) { return false; };
-    RasterStatus status = rasterizer->Draw(pipeline, no_discard);
+    ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false));
+    RasterStatus status = rasterizer->Draw(pipeline);
     EXPECT_EQ(status, RasterStatus::kSuccess);
     latch.Signal();
   });
@@ -792,8 +795,8 @@ TEST(
     PipelineProduceResult result =
         pipeline->Produce().Complete(std::move(layer_tree_item));
     EXPECT_TRUE(result.success);
-    auto no_discard = [](int64_t, LayerTree&) { return false; };
-    RasterStatus status = rasterizer->Draw(pipeline, no_discard);
+    ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false));
+    RasterStatus status = rasterizer->Draw(pipeline);
     EXPECT_EQ(status, RasterStatus::kDiscarded);
     latch.Signal();
   });
@@ -848,8 +851,8 @@ TEST(
     PipelineProduceResult result =
         pipeline->Produce().Complete(std::move(layer_tree_item));
     EXPECT_TRUE(result.success);
-    auto no_discard = [](int64_t, LayerTree&) { return false; };
-    RasterStatus status = rasterizer->Draw(pipeline, no_discard);
+    ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false));
+    RasterStatus status = rasterizer->Draw(pipeline);
     EXPECT_EQ(status, RasterStatus::kFailed);
     latch.Signal();
   });
@@ -929,10 +932,10 @@ TEST(RasterizerTest,
       EXPECT_TRUE(result.success);
       EXPECT_EQ(result.is_first_item, i == 0);
     }
-    auto no_discard = [](int64_t, LayerTree&) { return false; };
     // Although we only call 'Rasterizer::Draw' once, it will be called twice
     // finally because there are two items in the pipeline.
-    rasterizer->Draw(pipeline, no_discard);
+    ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false));
+    rasterizer->Draw(pipeline);
   });
   count_down_latch.Wait();
   thread_host.raster_thread->GetTaskRunner()->PostTask([&] {
@@ -1100,10 +1103,10 @@ TEST(RasterizerTest, presentationTimeSetWhenVsyncTargetInFuture) {
       EXPECT_TRUE(result.success);
       EXPECT_EQ(result.is_first_item, i == 0);
     }
-    auto no_discard = [](int64_t, LayerTree&) { return false; };
     // Although we only call 'Rasterizer::Draw' once, it will be called twice
     // finally because there are two items in the pipeline.
-    rasterizer->Draw(pipeline, no_discard);
+    ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false));
+    rasterizer->Draw(pipeline);
   });
 
   submit_latch.Wait();
@@ -1181,8 +1184,8 @@ TEST(RasterizerTest, presentationTimeNotSetWhenVsyncTargetInPast) {
         pipeline->Produce().Complete(std::move(layer_tree_item));
     EXPECT_TRUE(result.success);
     EXPECT_EQ(result.is_first_item, true);
-    auto no_discard = [](int64_t, LayerTree&) { return false; };
-    rasterizer->Draw(pipeline, no_discard);
+    ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false));
+    rasterizer->Draw(pipeline);
   });
 
   submit_latch.Wait();

diff --git a/shell/common/shell.cc b/shell/common/shell.cc
@@ -1218,23 +1218,15 @@ void Shell::OnAnimatorUpdateLatestFrameTargetTime(
 void Shell::OnAnimatorDraw(std::shared_ptr<LayerTreePipeline> pipeline) {
   FML_DCHECK(is_set_up_);
 
-  auto discard_callback = [this](int64_t view_id, flutter::LayerTree& tree) {
-    std::scoped_lock<std::mutex> lock(resize_mutex_);
-    auto expected_frame_size = ExpectedFrameSize(view_id);
-    return !expected_frame_size.isEmpty() &&
-           tree.frame_size() != expected_frame_size;
-  };
-
   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(),
-       weak_pipeline = std::weak_ptr<LayerTreePipeline>(pipeline),
-       discard_callback = std::move(discard_callback)]() mutable {
+       weak_pipeline = std::weak_ptr<LayerTreePipeline>(pipeline)]() mutable {
         if (rasterizer) {
           std::shared_ptr<LayerTreePipeline> pipeline = weak_pipeline.lock();
           if (pipeline) {
-            rasterizer->Draw(pipeline, std::move(discard_callback));
+            rasterizer->Draw(pipeline);
           }
 
           if (waiting_for_first_frame.load()) {
@@ -1591,6 +1583,15 @@ fml::TimePoint Shell::GetLatestFrameTargetTime() const {
   return latest_frame_target_time_.value();
 }
 
+// |Rasterizer::Delegate|
+bool Shell::ShouldDiscardLayerTree(int64_t view_id,
+                                   const flutter::LayerTree& tree) {
+  std::scoped_lock<std::mutex> lock(resize_mutex_);
+  auto expected_frame_size = ExpectedFrameSize(view_id);
+  return !expected_frame_size.isEmpty() &&
+         tree.frame_size() != expected_frame_size;
+}
+
 // |ServiceProtocol::Handler|
 fml::RefPtr<fml::TaskRunner> Shell::GetServiceProtocolHandlerTaskRunner(
     std::string_view method) const {

diff --git a/shell/common/shell.h b/shell/common/shell.h
@@ -693,6 +693,10 @@ class Shell final : public PlatformView::Delegate,
   // |Rasterizer::Delegate|
   fml::TimePoint GetLatestFrameTargetTime() const override;
 
+  // |Rasterizer::Delegate|
+  bool ShouldDiscardLayerTree(int64_t view_id,
+                              const flutter::LayerTree& tree) override;
+
   // |ServiceProtocol::Handler|
   fml::RefPtr<fml::TaskRunner> GetServiceProtocolHandlerTaskRunner(
       std::string_view method) const override;