flutter · blasten · Sep 11, 2020 · Sep 9, 2020 · Sep 9, 2020 · Sep 10, 2020
diff --git a/flow/compositor_context.cc b/flow/compositor_context.cc
@@ -82,6 +82,9 @@ RasterStatus CompositorContext::ScopedFrame::Raster(
   if (post_preroll_result == PostPrerollResult::kResubmitFrame) {
     return RasterStatus::kResubmit;
   }
+  if (post_preroll_result == PostPrerollResult::kSkipAndRetryFrame) {
+    return RasterStatus::kSkipAndRetry;
+  }
   // Clearing canvas after preroll reduces one render target switch when preroll
   // paints some raster cache.
   if (canvas()) {

diff --git a/flow/compositor_context.h b/flow/compositor_context.h
@@ -24,12 +24,25 @@ class LayerTree;
 enum class RasterStatus {
   // Frame has successfully rasterized.
   kSuccess,
-  // Frame needs to be resubmitted for rasterization. This is
-  // currently only called when thread configuration change occurs.
+  // Frame is submitted twice. This is only used on Android when
+  // switching the background surface to FlutterImageView.
+  //
+  // On Android, the first frame doesn't make the image available
+  // to the ImageReader right away. The second frame does.
+  //
+  // TODO(egarciad): https://github.com/flutter/flutter/issues/65652
   kResubmit,
+  // Frame is dropped and a new frame with the same layer tree is
+  // attempted.
+  //
+  // This is currently used to wait for the thread merger to merge
+  // the raster and platform threads.
+  //
+  // Since the thread merger may be disabled,
+  kSkipAndRetry,
   // 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.
+  // only used when thread configuration change occurs.
   kEnqueuePipeline,
   // Failed to rasterize the frame.
   kFailed

diff --git a/flow/embedded_views.h b/flow/embedded_views.h
@@ -242,7 +242,17 @@ class EmbeddedViewParams {
   SkRect final_bounding_rect_;
 };
 
-enum class PostPrerollResult { kResubmitFrame, kSuccess };
+enum class PostPrerollResult {
+  // Frame has successfully rasterized.
+  kSuccess,
+  // Frame is submitted twice. This is currently only used when
+  // thread configuration change occurs.
+  kResubmitFrame,
+  // Frame is dropped and a new frame with the same layer tree is
+  // attempted. This is currently only used when thread configuration
+  // change occurs.
+  kSkipAndRetryFrame
+};
 
 // Facilitates embedding of platform views within the flow layer tree.
 //

diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc
@@ -172,7 +172,10 @@ void Rasterizer::Draw(fml::RefPtr<Pipeline<flutter::LayerTree>> pipeline) {
   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.
-  if (raster_status == RasterStatus::kResubmit) {
+
+  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_));
@@ -186,7 +189,6 @@ void Rasterizer::Draw(fml::RefPtr<Pipeline<flutter::LayerTree>> pipeline) {
   // Merging the thread as we know the next `Draw` should be run on the platform
   // thread.
   if (surface_ != nullptr && surface_->GetExternalViewEmbedder() != nullptr) {
-    auto should_resubmit_frame = raster_status == RasterStatus::kResubmit;
     surface_->GetExternalViewEmbedder()->EndFrame(should_resubmit_frame,
                                                   raster_thread_merger_);
   }
@@ -332,7 +334,8 @@ RasterStatus Rasterizer::DoDraw(
   RasterStatus raster_status = DrawToSurface(*layer_tree);
   if (raster_status == RasterStatus::kSuccess) {
     last_layer_tree_ = std::move(layer_tree);
-  } else if (raster_status == RasterStatus::kResubmit) {
+  } else if (raster_status == RasterStatus::kResubmit ||
+             raster_status == RasterStatus::kSkipAndRetry) {
     resubmitted_layer_tree_ = std::move(layer_tree);
     return raster_status;
   }
@@ -457,7 +460,8 @@ RasterStatus Rasterizer::DrawToSurface(flutter::LayerTree& layer_tree) {
 
   if (compositor_frame) {
     RasterStatus raster_status = compositor_frame->Raster(layer_tree, false);
-    if (raster_status == RasterStatus::kFailed) {
+    if (raster_status == RasterStatus::kFailed ||
+        raster_status == RasterStatus::kSkipAndRetry) {
       return raster_status;
     }
     if (external_view_embedder != nullptr) {

diff --git a/shell/common/shell_test_external_view_embedder.cc b/shell/common/shell_test_external_view_embedder.cc
@@ -12,17 +12,16 @@ ShellTestExternalViewEmbedder::ShellTestExternalViewEmbedder(
     bool support_thread_merging)
     : end_frame_call_back_(end_frame_call_back),
       post_preroll_result_(post_preroll_result),
-      support_thread_merging_(support_thread_merging) {
-  resubmit_once_ = false;
-}
+      support_thread_merging_(support_thread_merging),
+      submitted_frame_count_(0) {}
 
 void ShellTestExternalViewEmbedder::UpdatePostPrerollResult(
     PostPrerollResult post_preroll_result) {
   post_preroll_result_ = post_preroll_result;
 }
 
-void ShellTestExternalViewEmbedder::SetResubmitOnce() {
-  resubmit_once_ = true;
+int ShellTestExternalViewEmbedder::GetSubmittedFrameCount() {
+  return submitted_frame_count_;
 }
 
 // |ExternalViewEmbedder|
@@ -44,10 +43,6 @@ void ShellTestExternalViewEmbedder::PrerollCompositeEmbeddedView(
 PostPrerollResult ShellTestExternalViewEmbedder::PostPrerollAction(
     fml::RefPtr<fml::RasterThreadMerger> raster_thread_merger) {
   FML_DCHECK(raster_thread_merger);
-  if (resubmit_once_) {
-    resubmit_once_ = false;
-    return PostPrerollResult::kResubmitFrame;
-  }
   return post_preroll_result_;
 }
 
@@ -66,6 +61,7 @@ void ShellTestExternalViewEmbedder::SubmitFrame(
     GrDirectContext* context,
     std::unique_ptr<SurfaceFrame> frame) {
   frame->Submit();
+  submitted_frame_count_++;
 }
 
 // |ExternalViewEmbedder|

diff --git a/shell/common/shell_test_external_view_embedder.h b/shell/common/shell_test_external_view_embedder.h
@@ -28,9 +28,9 @@ class ShellTestExternalViewEmbedder final : public ExternalViewEmbedder {
   // returns the new `post_preroll_result`.
   void UpdatePostPrerollResult(PostPrerollResult post_preroll_result);
 
-  // Updates the post preroll result to `PostPrerollResult::kResubmitFrame` for
-  // only the next frame.
-  void SetResubmitOnce();
+  // Gets the number of times the SubmitFrame method has been called in
+  // the external view embedder.
+  int GetSubmittedFrameCount();
 
  private:
   // |ExternalViewEmbedder|
@@ -74,11 +74,13 @@ class ShellTestExternalViewEmbedder final : public ExternalViewEmbedder {
   bool SupportsDynamicThreadMerging() override;
 
   const EndFrameCallBack end_frame_call_back_;
+
   PostPrerollResult post_preroll_result_;
-  bool resubmit_once_;
 
   bool support_thread_merging_;
 
+  std::atomic<int> submitted_frame_count_;
+
   FML_DISALLOW_COPY_AND_ASSIGN(ShellTestExternalViewEmbedder);
 };
 

diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc
@@ -617,8 +617,7 @@ TEST_F(ShellTest,
       };
   auto external_view_embedder = std::make_shared<ShellTestExternalViewEmbedder>(
       end_frame_callback, PostPrerollResult::kSuccess, true);
-  // Set resubmit once to trigger thread merging.
-  external_view_embedder->SetResubmitOnce();
+
   auto shell = CreateShell(std::move(settings), GetTaskRunnersForFixture(),
                            false, external_view_embedder);
 
@@ -674,18 +673,25 @@ TEST_F(ShellTest,
   const size_t ThreadMergingLease = 10;
   auto settings = CreateSettingsForFixture();
   fml::AutoResetWaitableEvent end_frame_latch;
+  std::shared_ptr<ShellTestExternalViewEmbedder> external_view_embedder;
+
   auto end_frame_callback =
       [&](bool should_resubmit_frame,
           fml::RefPtr<fml::RasterThreadMerger> raster_thread_merger) {
         if (should_resubmit_frame && !raster_thread_merger->IsMerged()) {
           raster_thread_merger->MergeWithLease(ThreadMergingLease);
+
+          ASSERT_TRUE(raster_thread_merger->IsMerged());
+          external_view_embedder->UpdatePostPrerollResult(
+              PostPrerollResult::kSuccess);
         }
         end_frame_latch.Signal();
       };
-  auto external_view_embedder = std::make_shared<ShellTestExternalViewEmbedder>(
+  external_view_embedder = std::make_shared<ShellTestExternalViewEmbedder>(
       end_frame_callback, PostPrerollResult::kSuccess, true);
   // Set resubmit once to trigger thread merging.
-  external_view_embedder->SetResubmitOnce();
+  external_view_embedder->UpdatePostPrerollResult(
+      PostPrerollResult::kResubmitFrame);
   auto shell = CreateShell(std::move(settings), GetTaskRunnersForFixture(),
                            false, external_view_embedder);
 
@@ -796,7 +802,8 @@ TEST_F(ShellTest,
 
   // Pump a frame with `PostPrerollResult::kResubmitFrame` to start merging
   // threads
-  external_view_embedder->SetResubmitOnce();
+  external_view_embedder->UpdatePostPrerollResult(
+      PostPrerollResult::kResubmitFrame);
   PumpOneFrame(shell.get(), 100, 100, builder);
 
   // Now destroy the platform view immediately.
@@ -988,6 +995,96 @@ TEST_F(ShellTest,
   DestroyShell(std::move(shell), std::move(task_runners));
 }
 
+// TODO(https://github.com/flutter/flutter/issues/59816): Enable on fuchsia.
+TEST_F(ShellTest,
+#if defined(OS_FUCHSIA)
+       DISABLED_SkipAndSubmitFrame
+#else
+       SkipAndSubmitFrame
+#endif
+) {
+  auto settings = CreateSettingsForFixture();
+  fml::AutoResetWaitableEvent end_frame_latch;
+  std::shared_ptr<ShellTestExternalViewEmbedder> external_view_embedder;
+
+  auto end_frame_callback =
+      [&](bool should_resubmit_frame,
+          fml::RefPtr<fml::RasterThreadMerger> raster_thread_merger) {
+        external_view_embedder->UpdatePostPrerollResult(
+            PostPrerollResult::kSuccess);
+        end_frame_latch.Signal();
+      };
+  external_view_embedder = std::make_shared<ShellTestExternalViewEmbedder>(
+      end_frame_callback, PostPrerollResult::kSkipAndRetryFrame, true);
+
+  auto shell = CreateShell(std::move(settings), GetTaskRunnersForFixture(),
+                           false, external_view_embedder);
+
+  PlatformViewNotifyCreated(shell.get());
+
+  auto configuration = RunConfiguration::InferFromSettings(settings);
+  configuration.SetEntrypoint("emptyMain");
+  RunEngine(shell.get(), std::move(configuration));
+
+  ASSERT_EQ(0, external_view_embedder->GetSubmittedFrameCount());
+
+  PumpOneFrame(shell.get());
+
+  // `EndFrame` changed the post preroll result to `kSuccess`.
+  end_frame_latch.Wait();
+  ASSERT_EQ(0, external_view_embedder->GetSubmittedFrameCount());
+
+  PumpOneFrame(shell.get());
+  end_frame_latch.Wait();
+  ASSERT_EQ(1, external_view_embedder->GetSubmittedFrameCount());
+
+  DestroyShell(std::move(shell));
+}
+
+// TODO(https://github.com/flutter/flutter/issues/59816): Enable on fuchsia.
+TEST_F(ShellTest,
+#if defined(OS_FUCHSIA)
+       DISABLED_ResubmitFrame
+#else
+       ResubmitFrame
+#endif
+) {
+  auto settings = CreateSettingsForFixture();
+  fml::AutoResetWaitableEvent end_frame_latch;
+  std::shared_ptr<ShellTestExternalViewEmbedder> external_view_embedder;
+
+  auto end_frame_callback =
+      [&](bool should_resubmit_frame,
+          fml::RefPtr<fml::RasterThreadMerger> raster_thread_merger) {
+        external_view_embedder->UpdatePostPrerollResult(
+            PostPrerollResult::kSuccess);
+        end_frame_latch.Signal();
+      };
+  external_view_embedder = std::make_shared<ShellTestExternalViewEmbedder>(
+      end_frame_callback, PostPrerollResult::kResubmitFrame, true);
+
+  auto shell = CreateShell(std::move(settings), GetTaskRunnersForFixture(),
+                           false, external_view_embedder);
+
+  PlatformViewNotifyCreated(shell.get());
+
+  auto configuration = RunConfiguration::InferFromSettings(settings);
+  configuration.SetEntrypoint("emptyMain");
+  RunEngine(shell.get(), std::move(configuration));
+
+  ASSERT_EQ(0, external_view_embedder->GetSubmittedFrameCount());
+
+  PumpOneFrame(shell.get());
+  // `EndFrame` changed the post preroll result to `kSuccess`.
+  end_frame_latch.Wait();
+  ASSERT_EQ(1, external_view_embedder->GetSubmittedFrameCount());
+
+  end_frame_latch.Wait();
+  ASSERT_EQ(2, external_view_embedder->GetSubmittedFrameCount());
+
+  DestroyShell(std::move(shell));
+}
+
 TEST(SettingsTest, FrameTimingSetsAndGetsProperly) {
   // Ensure that all phases are in kPhases.
   ASSERT_EQ(sizeof(FrameTiming::kPhases),

diff --git a/shell/platform/android/external_view_embedder/external_view_embedder.cc b/shell/platform/android/external_view_embedder/external_view_embedder.cc
@@ -77,10 +77,6 @@ void AndroidExternalViewEmbedder::SubmitFrame(
     std::unique_ptr<SurfaceFrame> frame) {
   TRACE_EVENT0("flutter", "AndroidExternalViewEmbedder::SubmitFrame");
 
-  if (should_run_rasterizer_on_platform_thread_) {
-    // Don't submit the current frame if the frame will be resubmitted.
-    return;
-  }
   if (!FrameHasPlatformLayers()) {
     frame->Submit();
     return;
@@ -217,27 +213,25 @@ AndroidExternalViewEmbedder::CreateSurfaceIfNeeded(GrDirectContext* context,
 // |ExternalViewEmbedder|
 PostPrerollResult AndroidExternalViewEmbedder::PostPrerollAction(
     fml::RefPtr<fml::RasterThreadMerger> raster_thread_merger) {
-  // This frame may remove existing platform views that aren't contained
-  // in `composition_order_`.
-  //
-  // If this frame doesn't have platform views, it's still required to keep
-  // the rasterizer running on the platform thread for at least one more
-  // frame.
-  //
-  // To keep the rasterizer running on the platform thread one more frame,
-  // `kDefaultMergedLeaseDuration` must be at least `1`.
   if (!FrameHasPlatformLayers()) {
     return PostPrerollResult::kSuccess;
   }
-  if (raster_thread_merger->IsMerged()) {
-    raster_thread_merger->ExtendLeaseTo(kDefaultMergedLeaseDuration);
-  } else {
-    // Merge the raster and platform threads in `EndFrame`.
-    should_run_rasterizer_on_platform_thread_ = true;
+  if (!raster_thread_merger->IsMerged()) {
+    // The raster thread merger may be disabled if the rasterizer is being
+    // created or teared down.
+    //
+    // In such cases, the current frame is dropped, and a new frame is attempted
+    // with the same layer tree.
+    //
+    // Eventually, the frame is submitted once this method returns `kSuccess`.
+    // At that point, the raster tasks are handled on the platform thread.
+    raster_thread_merger->MergeWithLease(kDefaultMergedLeaseDuration);
     CancelFrame();
-    return PostPrerollResult::kResubmitFrame;
+    return PostPrerollResult::kSkipAndRetryFrame;
   }
+  raster_thread_merger->ExtendLeaseTo(kDefaultMergedLeaseDuration);
   // Surface switch requires to resubmit the frame.
+  // TODO(egarciad): https://github.com/flutter/flutter/issues/65652
   if (previous_frame_view_count_ == 0) {
     return PostPrerollResult::kResubmitFrame;
   }
@@ -291,17 +285,6 @@ void AndroidExternalViewEmbedder::CancelFrame() {
 void AndroidExternalViewEmbedder::EndFrame(
     bool should_resubmit_frame,
     fml::RefPtr<fml::RasterThreadMerger> raster_thread_merger) {
-  if (should_resubmit_frame && should_run_rasterizer_on_platform_thread_) {
-    raster_thread_merger->MergeWithLease(kDefaultMergedLeaseDuration);
-    if (raster_thread_merger->IsMerged()) {
-      // The raster thread merger may be disabled if the rasterizer is being
-      // teared down.
-      //
-      // Therefore, set `should_run_rasterizer_on_platform_thread_` to `false`
-      // only if the thread merger was able to set the lease.
-      should_run_rasterizer_on_platform_thread_ = false;
-    }
-  }
   surface_pool_->RecycleLayers();
   // JNI method must be called on the platform thread.
   if (raster_thread_merger->IsOnPlatformThread()) {

diff --git a/shell/platform/android/external_view_embedder/external_view_embedder.h b/shell/platform/android/external_view_embedder/external_view_embedder.h
@@ -98,10 +98,6 @@ class AndroidExternalViewEmbedder final : public ExternalViewEmbedder {
   // Holds surfaces. Allows to recycle surfaces or allocate new ones.
   const std::unique_ptr<SurfacePool> surface_pool_;
 
-  // Whether the rasterizer task runner should run on the platform thread.
-  // When this is true, the current frame is cancelled and resubmitted.
-  bool should_run_rasterizer_on_platform_thread_ = false;
-
   // The size of the root canvas.
   SkISize frame_size_;