revise docs and address CR comments

Author: Kaushik Iska

ci/licenses_golden/licenses_flutter

@@ -576,6 +576,8 @@ FILE: ../../../flutter/shell/common/fixtures/shelltest_screenshot.png
 FILE: ../../../flutter/shell/common/input_events_unittests.cc
 FILE: ../../../flutter/shell/common/isolate_configuration.cc
 FILE: ../../../flutter/shell/common/isolate_configuration.h
+FILE: ../../../flutter/shell/common/layer_tree_holder.cc
+FILE: ../../../flutter/shell/common/layer_tree_holder.h
 FILE: ../../../flutter/shell/common/persistent_cache.cc
 FILE: ../../../flutter/shell/common/persistent_cache.h
 FILE: ../../../flutter/shell/common/persistent_cache_unittests.cc

flow/compositor_context.h

@@ -27,10 +27,6 @@ enum class RasterStatus {
   // Frame needs to be resubmitted for rasterization. This is
   // currently only called when thread configuration change occurs.
   kResubmit,
-  // Frame has been successfully rasterized, but "there are additional items in
-  // the pipeline waiting to be consumed. This is currently
-  // only called when thread configuration change occurs.
-  kEnqueuePipeline,
   // Failed to rasterize the frame.
   kFailed
 };

shell/common/engine.h

@@ -406,25 +406,18 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate {
   ///             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);
 
   //----------------------------------------------------------------------------

shell/common/layer_tree_holder.cc

@@ -11,12 +11,12 @@ std::unique_ptr<LayerTree> LayerTreeHolder::Get() {
   return std::move(layer_tree_);
 }
 
-void LayerTreeHolder::ReplaceIfNewer(std::unique_ptr<LayerTree> layer_tree) {
+void LayerTreeHolder::ReplaceIfNewer(
+    std::unique_ptr<LayerTree> proposed_layer_tree) {
   std::scoped_lock lock(layer_tree_mutex);
-  if (IsEmpty()) {
-    layer_tree_ = std::move(layer_tree);
-  } else if (layer_tree_->target_time() < layer_tree->target_time()) {
-    layer_tree_ = std::move(layer_tree);
+  if (IsEmpty() ||
+      layer_tree_->target_time() < proposed_layer_tree->target_time()) {
+    layer_tree_ = std::move(proposed_layer_tree);
   }
 }
 

shell/common/layer_tree_holder.h

@@ -21,7 +21,7 @@ class LayerTreeHolder {
 
   std::unique_ptr<LayerTree> Get();
 
-  void ReplaceIfNewer(std::unique_ptr<LayerTree> layer_tree);
+  void ReplaceIfNewer(std::unique_ptr<LayerTree> proposed_layer_tree);
 
  private:
   mutable std::mutex layer_tree_mutex;

shell/common/rasterizer.cc

@@ -121,29 +121,24 @@ void Rasterizer::Draw(std::shared_ptr<LayerTreeHolder> layer_tree_holder) {
   FML_DCHECK(task_runners_.GetRasterTaskRunner()->RunsTasksOnCurrentThread());
 
   std::unique_ptr<LayerTree> layer_tree = layer_tree_holder->Get();
-  RasterStatus raster_status = RasterStatus::kFailed;
-  if (layer_tree) {
-    raster_status = DoDraw(std::move(layer_tree));
-  }
-
-  // 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.
-  if (raster_status == RasterStatus::kResubmit) {
-    layer_tree_holder->ReplaceIfNewer(std::move(resubmitted_layer_tree_));
-  }
+  RasterStatus raster_status =
+      layer_tree ? DoDraw(std::move(layer_tree)) : RasterStatus::kFailed;
 
   // Merging the thread as we know the next `Draw` should be run on the platform
   // thread.
   if (raster_status == RasterStatus::kResubmit) {
+    layer_tree_holder->ReplaceIfNewer(std::move(resubmitted_layer_tree_));
     auto* external_view_embedder = surface_->GetExternalViewEmbedder();
-    // We know only the `external_view_embedder` can
-    // causes|RasterStatus::kResubmit|. Check to make sure.
-    FML_DCHECK(external_view_embedder != nullptr);
+    FML_DCHECK(external_view_embedder != nullptr)
+        << "kResubmit is an invalid raster status without external view "
+           "embedder.";
     external_view_embedder->EndFrame(raster_thread_merger_);
   }
 
-  // Consume as many pipeline items as possible. But yield the event loop
+  // Consume as many layer trees as possible. But yield the event loop
   // between successive tries.
+  // 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()) {
     task_runners_.GetRasterTaskRunner()->PostTask(
         [weak_this = weak_factory_.GetWeakPtr(), layer_tree_holder]() {
@@ -299,29 +294,6 @@ RasterStatus Rasterizer::DoDraw(
     );
   }
 
-  // Pipeline pressure is applied from a couple of places:
-  // rasterizer: When there are more items as of the time of Consume.
-  // animator (via shell): Frame gets produces every vsync.
-  // Enqueing here is to account for the following scenario:
-  // T = 1
-  //  - one item (A) in the pipeline
-  //  - rasterizer starts (and merges the threads)
-  //  - pipeline consume result says no items to process
-  // T = 2
-  //  - animator produces (B) to the pipeline
-  //  - applies pipeline pressure via platform thread.
-  // T = 3
-  //   - rasterizes finished (and un-merges the threads)
-  //   - |Draw| for B yields as its on the wrong thread.
-  // This enqueue ensures that we attempt to consume from the right
-  // thread one more time after un-merge.
-  if (raster_thread_merger_) {
-    if (raster_thread_merger_->DecrementLease() ==
-        fml::RasterThreadStatus::kUnmergedNow) {
-      return RasterStatus::kEnqueuePipeline;
-    }
-  }
-
   return raster_status;
 }
 

shell/common/rasterizer.h

@@ -230,34 +230,26 @@ class Rasterizer final : public SnapshotDelegate {
   flutter::TextureRegistry* GetTextureRegistry();
 
   //----------------------------------------------------------------------------
-  /// @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 raster 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.
-  /// TODO(kaushikiska) fix docs whereever pipeline is mentioned.
+  /// @param[in]  layer_tree_holder  The layer tree holder to take the latest
+  ///                                layer tree to render from.
   void Draw(std::shared_ptr<LayerTreeHolder> layer_tree_holder);
 
   //----------------------------------------------------------------------------
@@ -425,7 +417,8 @@ class Rasterizer final : public SnapshotDelegate {
   std::unique_ptr<flutter::LayerTree> 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<flutter::LayerTree> resubmitted_layer_tree_;
   fml::closure next_frame_callback_;
   bool user_override_resource_cache_bytes_;

shell/common/shell_test.cc

@@ -136,7 +136,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(