Revert "Use a Pbuffer surface when the onscreen surface is not available for snapshotting on Android (#27141)" (#27607)

This reverts commit a1180b1.

Author: dnfield

BUILD.gn

@@ -121,11 +121,6 @@ group("flutter") {
     public_deps += [ "//flutter/testing/scenario_app" ]
   }
 
-  if (is_android && flutter_runtime_mode == "profile" &&
-      current_cpu == "arm64") {
-    public_deps += [ "//flutter/testing/android_background_image" ]
-  }
-
   # Compile all unittests targets if enabled.
   if (enable_unittests) {
     public_deps += [

ci/firebase_testlab.py

@@ -19,28 +19,28 @@
 
 
 def RunFirebaseTest(apk, results_dir):
-  # game-loop tests are meant for OpenGL apps.
-  # This type of test will give the application a handle to a file, and
-  # we'll write the timeline JSON to that file.
-  # See https://firebase.google.com/docs/test-lab/android/game-loop
-  # Pixel 4. As of this commit, this is a highly available device in FTL.
-  process = subprocess.Popen(
-    [
-      'gcloud',
-      '--project', 'flutter-infra',
-      'firebase', 'test', 'android', 'run',
-      '--type', 'game-loop',
-      '--app', apk,
-      '--timeout', '2m',
-      '--results-bucket', bucket,
-      '--results-dir', results_dir,
-      '--device', 'model=flame,version=29',
-    ],
-    stdout=subprocess.PIPE,
-    stderr=subprocess.STDOUT,
-    universal_newlines=True,
-  )
-  return process
+  try:
+    # game-loop tests are meant for OpenGL apps.
+    # This type of test will give the application a handle to a file, and
+    # we'll write the timeline JSON to that file.
+    # See https://firebase.google.com/docs/test-lab/android/game-loop
+    # Pixel 4. As of this commit, this is a highly available device in FTL.
+    subprocess.check_output([
+        'gcloud',
+        '--project', 'flutter-infra',
+        'firebase', 'test', 'android', 'run',
+        '--type', 'game-loop',
+        '--app', apk,
+        '--timeout', '2m',
+        '--results-bucket', bucket,
+        '--results-dir', results_dir,
+        '--device', 'model=flame,version=29',
+    ])
+  except subprocess.CalledProcessError as ex:
+    print(ex.output)
+    # Recipe will retry return codes from firebase that indicate an infra
+    # failure.
+    sys.exit(ex.returncode)
 
 
 def CheckLogcat(results_dir):
@@ -87,25 +87,13 @@ def main():
   git_revision = subprocess.check_output(
       ['git', 'rev-parse', 'HEAD'], cwd=script_dir).strip()
 
-  results = []
   for apk in apks:
     results_dir = '%s/%s/%s' % (os.path.basename(apk), git_revision, args.build_id)
-    process = RunFirebaseTest(apk, results_dir)
-    results.append((results_dir, process))
-
-  for results_dir, process in results:
-    for line in iter(process.stdout.readline, ""):
-      print(line.strip())
-    return_code = process.wait()
-    if return_code != 0:
-      print('Firebase test failed ' + returncode)
-      sys.exit(process.returncode)
-
-    print('Checking logcat for %s' % results_dir)
+
+    RunFirebaseTest(apk, results_dir)
     CheckLogcat(results_dir)
     # scenario_app produces a timeline, but the android image test does not.
     if 'scenario' in apk:
-      print('Checking timeline for %s' % results_dir)
       CheckTimeline(results_dir)
 
   return 0

ci/licenses_golden/licenses_flutter

@@ -725,7 +725,6 @@ FILE: ../../../flutter/shell/common/shell_unittests.cc
 FILE: ../../../flutter/shell/common/skia_event_tracer_impl.cc
 FILE: ../../../flutter/shell/common/skia_event_tracer_impl.h
 FILE: ../../../flutter/shell/common/skp_shader_warmup_unittests.cc
-FILE: ../../../flutter/shell/common/snapshot_surface_producer.h
 FILE: ../../../flutter/shell/common/switches.cc
 FILE: ../../../flutter/shell/common/switches.h
 FILE: ../../../flutter/shell/common/thread_host.cc
@@ -933,8 +932,6 @@ FILE: ../../../flutter/shell/platform/android/surface/android_surface.cc
 FILE: ../../../flutter/shell/platform/android/surface/android_surface.h
 FILE: ../../../flutter/shell/platform/android/surface/android_surface_mock.cc
 FILE: ../../../flutter/shell/platform/android/surface/android_surface_mock.h
-FILE: ../../../flutter/shell/platform/android/surface/snapshot_surface_producer.cc
-FILE: ../../../flutter/shell/platform/android/surface/snapshot_surface_producer.h
 FILE: ../../../flutter/shell/platform/android/vsync_waiter_android.cc
 FILE: ../../../flutter/shell/platform/android/vsync_waiter_android.h
 FILE: ../../../flutter/shell/platform/common/accessibility_bridge.cc

shell/common/BUILD.gn

@@ -89,7 +89,6 @@ source_set("common") {
     "shell_io_manager.h",
     "skia_event_tracer_impl.cc",
     "skia_event_tracer_impl.h",
-    "snapshot_surface_producer.h",
     "switches.cc",
     "switches.h",
     "thread_host.cc",

shell/common/platform_view.cc

@@ -67,6 +67,7 @@ void PlatformView::SetViewportMetrics(const ViewportMetrics& metrics) {
 
 void PlatformView::NotifyCreated() {
   std::unique_ptr<Surface> surface;
+
   // Threading: We want to use the platform view on the non-platform thread.
   // Using the weak pointer is illegal. But, we are going to introduce a latch
   // so that the platform view is not collected till the surface is obtained.
@@ -181,9 +182,4 @@ void PlatformView::UpdateAssetResolverByType(
   delegate_.UpdateAssetResolverByType(std::move(updated_asset_resolver), type);
 }
 
-std::unique_ptr<SnapshotSurfaceProducer>
-PlatformView::CreateSnapshotSurfaceProducer() {
-  return nullptr;
-}
-
 }  // namespace flutter

shell/common/platform_view.h

@@ -62,7 +62,7 @@ class PlatformView {
     ///             Metal, Vulkan) specific. This is usually a sign to the
     ///             rasterizer to set up and begin rendering to that surface.
     ///
-    /// @param[in]  surface           The surface
+    /// @param[in]  surface  The surface
     ///
     virtual void OnPlatformViewCreated(std::unique_ptr<Surface> surface) = 0;
 
@@ -796,22 +796,6 @@ class PlatformView {
       std::unique_ptr<AssetResolver> updated_asset_resolver,
       AssetResolver::AssetResolverType type);
 
-  //--------------------------------------------------------------------------
-  /// @brief      Creates an object that produces surfaces suitable for raster
-  ///             snapshotting. The rasterizer will request this surface if no
-  ///             on screen surface is currently available when an application
-  ///             requests a snapshot, e.g. if `Scene.toImage` or
-  ///             `Picture.toImage` are called while the application is in the
-  ///             background.
-  ///
-  ///             Not all backends support this kind of surface usage, and the
-  ///             default implementation returns nullptr. Platforms should
-  ///             override this if they can support GPU operations in the
-  ///             background and support GPU resource context usage.
-  ///
-  virtual std::unique_ptr<SnapshotSurfaceProducer>
-  CreateSnapshotSurfaceProducer();
-
  protected:
   PlatformView::Delegate& delegate_;
   const TaskRunners task_runners_;
@@ -820,7 +804,8 @@ class PlatformView {
   SkISize size_;
   fml::WeakPtrFactory<PlatformView> weak_factory_;
 
-  // This is the only method called on the raster task runner.
+  // Unlike all other methods on the platform view, this is called on the
+  // raster task runner.
   virtual std::unique_ptr<Surface> CreateRenderingSurface();
 
  private:

shell/common/rasterizer.cc

@@ -258,22 +258,11 @@ sk_sp<SkImage> Rasterizer::DoMakeRasterSnapshot(
   sk_sp<SkImage> result;
   SkImageInfo image_info = SkImageInfo::MakeN32Premul(
       size.width(), size.height(), SkColorSpace::MakeSRGB());
-
-  std::unique_ptr<Surface> pbuffer_surface;
-  Surface* snapshot_surface = nullptr;
-  if (surface_ && surface_->GetContext()) {
-    snapshot_surface = surface_.get();
-  } else if (snapshot_surface_producer_) {
-    pbuffer_surface = snapshot_surface_producer_->CreateSnapshotSurface();
-    if (pbuffer_surface && pbuffer_surface->GetContext())
-      snapshot_surface = pbuffer_surface.get();
-  }
-
-  if (!snapshot_surface) {
+  if (surface_ == nullptr || surface_->GetContext() == nullptr) {
     // Raster surface is fine if there is no on screen surface. This might
     // happen in case of software rendering.
-    sk_sp<SkSurface> sk_surface = SkSurface::MakeRaster(image_info);
-    result = DrawSnapshot(sk_surface, draw_callback);
+    sk_sp<SkSurface> surface = SkSurface::MakeRaster(image_info);
+    result = DrawSnapshot(surface, draw_callback);
   } else {
     delegate_.GetIsGpuDisabledSyncSwitch()->Execute(
         fml::SyncSwitch::Handlers()
@@ -282,14 +271,12 @@ sk_sp<SkImage> Rasterizer::DoMakeRasterSnapshot(
               result = DrawSnapshot(surface, draw_callback);
             })
             .SetIfFalse([&] {
-              FML_DCHECK(snapshot_surface);
-              auto context_switch =
-                  snapshot_surface->MakeRenderContextCurrent();
+              auto context_switch = surface_->MakeRenderContextCurrent();
               if (!context_switch->GetResult()) {
                 return;
               }
 
-              GrRecordingContext* context = snapshot_surface->GetContext();
+              GrRecordingContext* context = surface_->GetContext();
               auto max_size = context->maxRenderTargetSize();
               double scale_factor = std::min(
                   1.0, static_cast<double>(max_size) /
@@ -307,19 +294,19 @@ sk_sp<SkImage> Rasterizer::DoMakeRasterSnapshot(
 
               // When there is an on screen surface, we need a render target
               // SkSurface because we want to access texture backed images.
-              sk_sp<SkSurface> sk_surface =
+              sk_sp<SkSurface> surface =
                   SkSurface::MakeRenderTarget(context,          // context
                                               SkBudgeted::kNo,  // budgeted
                                               image_info        // image info
                   );
-              if (!sk_surface) {
+              if (!surface) {
                 FML_LOG(ERROR)
                     << "DoMakeRasterSnapshot can not create GPU render target";
                 return;
               }
 
-              sk_surface->getCanvas()->scale(scale_factor, scale_factor);
-              result = DrawSnapshot(sk_surface, draw_callback);
+              surface->getCanvas()->scale(scale_factor, scale_factor);
+              result = DrawSnapshot(surface, draw_callback);
             }));
   }
 
@@ -713,11 +700,6 @@ void Rasterizer::SetExternalViewEmbedder(
   external_view_embedder_ = view_embedder;
 }
 
-void Rasterizer::SetSnapshotSurfaceProducer(
-    std::unique_ptr<SnapshotSurfaceProducer> producer) {
-  snapshot_surface_producer_ = std::move(producer);
-}
-
 void Rasterizer::FireNextFrameCallbackIfPresent() {
   if (!next_frame_callback_) {
     return;

shell/common/rasterizer.h

@@ -24,7 +24,6 @@
 #include "flutter/fml/time/time_point.h"
 #include "flutter/lib/ui/snapshot_delegate.h"
 #include "flutter/shell/common/pipeline.h"
-#include "flutter/shell/common/snapshot_surface_producer.h"
 
 namespace flutter {
 
@@ -99,7 +98,7 @@ class Rasterizer final : public SnapshotDelegate {
   ///             currently only created by the shell (which also sets itself up
   ///             as the rasterizer delegate).
   ///
-  /// @param[in]  delegate                   The rasterizer delegate.
+  /// @param[in]  delegate            The rasterizer delegate.
   ///
   Rasterizer(Delegate& delegate);
 
@@ -350,17 +349,6 @@ class Rasterizer final : public SnapshotDelegate {
   void SetExternalViewEmbedder(
       const std::shared_ptr<ExternalViewEmbedder>& view_embedder);
 
-  //----------------------------------------------------------------------------
-  /// @brief Set the snapshot surface producer. This is done on shell
-  ///        initialization. This is non-null on platforms that support taking
-  ///        GPU accelerated raster snapshots in the background.
-  ///
-  /// @param[in]  producer  A surface producer for raster snapshotting when the
-  ///                       onscreen surface is not available.
-  ///
-  void SetSnapshotSurfaceProducer(
-      std::unique_ptr<SnapshotSurfaceProducer> producer);
-
   //----------------------------------------------------------------------------
   /// @brief      Returns a pointer to the compositor context used by this
   ///             rasterizer. This pointer will never be `nullptr`.
@@ -446,7 +434,6 @@ class Rasterizer final : public SnapshotDelegate {
  private:
   Delegate& delegate_;
   std::unique_ptr<Surface> surface_;
-  std::unique_ptr<SnapshotSurfaceProducer> snapshot_surface_producer_;
   std::unique_ptr<flutter::CompositorContext> compositor_context_;
   // This is the last successfully rasterized layer tree.
   std::unique_ptr<flutter::LayerTree> last_layer_tree_;

shell/common/rasterizer_unittests.cc

@@ -30,7 +30,6 @@ class MockDelegate : public Rasterizer::Delegate {
   MOCK_CONST_METHOD0(GetTaskRunners, const TaskRunners&());
   MOCK_CONST_METHOD0(GetIsGpuDisabledSyncSwitch,
                      std::shared_ptr<const fml::SyncSwitch>());
-  MOCK_METHOD0(CreateSnapshotSurface, std::unique_ptr<Surface>());
 };
 
 class MockSurface : public Surface {

shell/common/shell.cc

@@ -634,8 +634,6 @@ bool Shell::Setup(std::unique_ptr<PlatformView> platform_view,
   // Set the external view embedder for the rasterizer.
   auto view_embedder = platform_view_->CreateExternalViewEmbedder();
   rasterizer_->SetExternalViewEmbedder(view_embedder);
-  rasterizer_->SetSnapshotSurfaceProducer(
-      platform_view_->CreateSnapshotSurfaceProducer());
 
   // The weak ptr must be generated in the platform thread which owns the unique
   // ptr.