[Impeller] implements a retry mechanism for dart:ui/Image.toByteData. (#46840)

Design doc: [link](https://docs.google.com/document/d/1Uuiw3pdQxNFTA8OQuZ-kuvYg1NB42XgccQCZeqr4oII/edit#heading=h.hn6wreyrz6fm)
fixes: flutter/flutter#135245

One slight deviation from the design doc is that I decided to make ContextMTL respond to changes to the SyncSwitch instead of having it observe the app state directly.  The benefits are:
1) This keeps that functionality in one location
1) It makes writing tests much easier
1) There's no need of conditional compilation between macos and ios
1) There is no need to add an objc class

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

Author: gaaclarke

fml/synchronization/sync_switch.cc

@@ -32,8 +32,26 @@ void SyncSwitch::Execute(const SyncSwitch::Handlers& handlers) const {
 }
 
 void SyncSwitch::SetSwitch(bool value) {
+  {
+    fml::UniqueLock lock(*mutex_);
+    value_ = value;
+  }
+  for (Observer* observer : observers_) {
+    observer->OnSyncSwitchUpdate(value);
+  }
+}
+
+void SyncSwitch::AddObserver(Observer* observer) const {
   fml::UniqueLock lock(*mutex_);
-  value_ = value;
+  if (std::find(observers_.begin(), observers_.end(), observer) ==
+      observers_.end()) {
+    observers_.push_back(observer);
+  }
 }
 
+void SyncSwitch::RemoveObserver(Observer* observer) const {
+  fml::UniqueLock lock(*mutex_);
+  observers_.erase(std::remove(observers_.begin(), observers_.end(), observer),
+                   observers_.end());
+}
 }  // namespace fml

fml/synchronization/sync_switch.h

@@ -21,6 +21,16 @@ namespace fml {
 /// at a time.
 class SyncSwitch {
  public:
+  /// Observes changes to the SyncSwitch.
+  class Observer {
+   public:
+    virtual ~Observer() = default;
+    /// `new_is_disabled` not guaranteed to be the value of the SyncSwitch
+    /// during execution, it should be checked with calls to
+    /// SyncSwitch::Execute.
+    virtual void OnSyncSwitchUpdate(bool new_is_disabled) = 0;
+  };
+
   /// Represents the 2 code paths available when calling |SyncSwitch::Execute|.
   struct Handlers {
     /// Sets the handler that will be executed if the |SyncSwitch| is true.
@@ -53,8 +63,15 @@ class SyncSwitch {
   /// @param[in]  value  New value for the |SyncSwitch|.
   void SetSwitch(bool value);
 
+  /// Threadsafe.
+  void AddObserver(Observer* observer) const;
+
+  /// Threadsafe.
+  void RemoveObserver(Observer* observer) const;
+
  private:
   mutable std::unique_ptr<fml::SharedMutex> mutex_;
+  mutable std::vector<Observer*> observers_;
   bool value_;
 
   FML_DISALLOW_COPY_AND_ASSIGN(SyncSwitch);

impeller/renderer/backend/metal/context_mtl.h

@@ -6,8 +6,8 @@
 
 #include <Metal/Metal.h>
 
+#include <deque>
 #include <string>
-#include <vector>
 
 #include "flutter/fml/concurrent_message_loop.h"
 #include "flutter/fml/macros.h"
@@ -93,7 +93,20 @@ class ContextMTL final : public Context,
 
   std::shared_ptr<const fml::SyncSwitch> GetIsGpuDisabledSyncSwitch() const;
 
+  // |Context|
+  void StoreTaskForGPU(std::function<void()> task) override;
+
  private:
+  class SyncSwitchObserver : public fml::SyncSwitch::Observer {
+   public:
+    SyncSwitchObserver(ContextMTL& parent);
+    virtual ~SyncSwitchObserver() = default;
+    void OnSyncSwitchUpdate(bool new_value) override;
+
+   private:
+    ContextMTL& parent_;
+  };
+
   id<MTLDevice> device_ = nullptr;
   id<MTLCommandQueue> command_queue_ = nullptr;
   std::shared_ptr<ShaderLibraryMTL> shader_library_;
@@ -103,6 +116,8 @@ class ContextMTL final : public Context,
   std::shared_ptr<const Capabilities> device_capabilities_;
   std::shared_ptr<fml::ConcurrentMessageLoop> raster_message_loop_;
   std::shared_ptr<const fml::SyncSwitch> is_gpu_disabled_sync_switch_;
+  std::deque<std::function<void()>> tasks_awaiting_gpu_;
+  std::unique_ptr<SyncSwitchObserver> sync_switch_observer_;
   bool is_valid_ = false;
 
   ContextMTL(
@@ -114,6 +129,8 @@ class ContextMTL final : public Context,
   std::shared_ptr<CommandBuffer> CreateCommandBufferInQueue(
       id<MTLCommandQueue> queue) const;
 
+  void FlushTasksAwaitingGPU();
+
   FML_DISALLOW_COPY_AND_ASSIGN(ContextMTL);
 };
 

impeller/renderer/backend/metal/context_mtl.mm

@@ -83,6 +83,9 @@ static bool DeviceSupportsComputeSubgroups(id<MTLDevice> device) {
     return;
   }
 
+  sync_switch_observer_.reset(new SyncSwitchObserver(*this));
+  is_gpu_disabled_sync_switch_->AddObserver(sync_switch_observer_.get());
+
   // Worker task runner.
   {
     raster_message_loop_ = fml::ConcurrentMessageLoop::Create(
@@ -284,7 +287,9 @@ new ContextMTL(device, command_queue,
   return context;
 }
 
-ContextMTL::~ContextMTL() = default;
+ContextMTL::~ContextMTL() {
+  is_gpu_disabled_sync_switch_->RemoveObserver(sync_switch_observer_.get());
+}
 
 Context::BackendType ContextMTL::GetBackendType() const {
   return Context::BackendType::kMetal;
@@ -376,4 +381,28 @@ new ContextMTL(device, command_queue,
   return buffer;
 }
 
+void ContextMTL::StoreTaskForGPU(std::function<void()> task) {
+  tasks_awaiting_gpu_.emplace_back(std::move(task));
+  while (tasks_awaiting_gpu_.size() > kMaxTasksAwaitingGPU) {
+    tasks_awaiting_gpu_.front()();
+    tasks_awaiting_gpu_.pop_front();
+  }
+}
+
+void ContextMTL::FlushTasksAwaitingGPU() {
+  for (const auto& task : tasks_awaiting_gpu_) {
+    task();
+  }
+  tasks_awaiting_gpu_.clear();
+}
+
+ContextMTL::SyncSwitchObserver::SyncSwitchObserver(ContextMTL& parent)
+    : parent_(parent) {}
+
+void ContextMTL::SyncSwitchObserver::OnSyncSwitchUpdate(bool new_is_disabled) {
+  if (!new_is_disabled) {
+    parent_.FlushTasksAwaitingGPU();
+  }
+}
+
 }  // namespace impeller

impeller/renderer/context.h

@@ -52,6 +52,14 @@ class Context {
     kVulkan,
   };
 
+  /// The maximum number of tasks that should ever be stored for
+  /// `StoreTaskForGPU`.
+  ///
+  /// This number was arbitrarily chosen. The idea is that this is a somewhat
+  /// rare situation where tasks happen to get executed in that tiny amount of
+  /// time while an app is being backgrounded but still executing.
+  static constexpr int32_t kMaxTasksAwaitingGPU = 10;
+
   //----------------------------------------------------------------------------
   /// @brief      Destroys an Impeller context.
   ///
@@ -176,6 +184,19 @@ class Context {
 
   CaptureContext capture;
 
+  /// Stores a task on the `ContextMTL` that is awaiting access for the GPU.
+  ///
+  /// The task will be executed in the event that the GPU access has changed to
+  /// being available or that the task has been canceled. The task should
+  /// operate with the `SyncSwitch` to make sure the GPU is accessible.
+  ///
+  /// Threadsafe.
+  ///
+  /// `task` will be executed on the platform thread.
+  virtual void StoreTaskForGPU(std::function<void()> task) {
+    FML_CHECK(false && "not supported in this context");
+  }
+
  protected:
   Context();
 

lib/ui/fixtures/ui_test.dart

@@ -325,7 +325,11 @@ external void _validateExternal(Uint8List result);
 @pragma('vm:external-name', 'ValidateError')
 external void _validateError(String? error);
 @pragma('vm:external-name', 'TurnOffGPU')
-external void _turnOffGPU();
+external void _turnOffGPU(bool value);
+@pragma('vm:external-name', 'FlushGpuAwaitingTasks')
+external void _flushGpuAwaitingTasks();
+@pragma('vm:external-name', 'ValidateNotNull')
+external void _validateNotNull(Object? object);
 
 @pragma('vm:entry-point')
 Future<void> toByteDataWithoutGPU() async {
@@ -338,12 +342,40 @@ Future<void> toByteDataWithoutGPU() async {
   canvas.drawCircle(c, 25.0, paint);
   final Picture picture = pictureRecorder.endRecording();
   final Image image = await picture.toImage(100, 100);
-  _turnOffGPU();
+  _turnOffGPU(true);
+  Timer flusher = Timer.periodic(Duration(milliseconds: 1), (timer) {
+    _flushGpuAwaitingTasks();
+  });
   try {
     ByteData? byteData = await image.toByteData();
     _validateError(null);
-  } catch (ex) {
-    _validateError(ex.toString());
+  } catch (error) {
+    _validateError(error.toString());
+  } finally {
+    flusher.cancel();
+  }
+}
+
+@pragma('vm:entry-point')
+Future<void> toByteDataRetries() async {
+  final PictureRecorder pictureRecorder = PictureRecorder();
+  final Canvas canvas = Canvas(pictureRecorder);
+  final Paint paint = Paint()
+    ..color = Color.fromRGBO(255, 255, 255, 1.0)
+    ..style = PaintingStyle.fill;
+  final Offset c = Offset(50.0, 50.0);
+  canvas.drawCircle(c, 25.0, paint);
+  final Picture picture = pictureRecorder.endRecording();
+  final Image image = await picture.toImage(100, 100);
+  _turnOffGPU(true);
+  Future<void>.delayed(Duration(milliseconds: 10), () {
+    _turnOffGPU(false);
+  });
+  try {
+    ByteData? byteData = await image.toByteData();
+    _validateNotNull(byteData);
+  } catch (error) {
+    _validateNotNull(null);
   }
 }
 

lib/ui/painting/image_encoding_impeller.cc

@@ -56,21 +56,68 @@ sk_sp<SkImage> ConvertBufferToSkImage(
   return raster_image;
 }
 
-void DoConvertImageToRasterImpeller(
+[[nodiscard]] fml::Status DoConvertImageToRasterImpeller(
     const sk_sp<DlImage>& dl_image,
-    std::function<void(fml::StatusOr<sk_sp<SkImage>>)> encode_task,
+    const std::function<void(fml::StatusOr<sk_sp<SkImage>>)>& encode_task,
     const std::shared_ptr<const fml::SyncSwitch>& is_gpu_disabled_sync_switch,
     const std::shared_ptr<impeller::Context>& impeller_context) {
+  fml::Status result;
   is_gpu_disabled_sync_switch->Execute(
       fml::SyncSwitch::Handlers()
-          .SetIfTrue([&encode_task] {
-            encode_task(
-                fml::Status(fml::StatusCode::kUnavailable, "GPU unavailable."));
+          .SetIfTrue([&result] {
+            result =
+                fml::Status(fml::StatusCode::kUnavailable, "GPU unavailable.");
           })
           .SetIfFalse([&dl_image, &encode_task, &impeller_context] {
             ImageEncodingImpeller::ConvertDlImageToSkImage(
-                dl_image, std::move(encode_task), impeller_context);
+                dl_image, encode_task, impeller_context);
           }));
+  return result;
+}
+
+/// Same as `DoConvertImageToRasterImpeller` but it will attempt to retry the
+/// operation if `DoConvertImageToRasterImpeller` returns kUnavailable when the
+/// GPU becomes available again.
+void DoConvertImageToRasterImpellerWithRetry(
+    const sk_sp<DlImage>& dl_image,
+    std::function<void(fml::StatusOr<sk_sp<SkImage>>)>&& encode_task,
+    const std::shared_ptr<const fml::SyncSwitch>& is_gpu_disabled_sync_switch,
+    const std::shared_ptr<impeller::Context>& impeller_context,
+    const fml::RefPtr<fml::TaskRunner>& retry_runner) {
+  fml::Status status = DoConvertImageToRasterImpeller(
+      dl_image, encode_task, is_gpu_disabled_sync_switch, impeller_context);
+  if (!status.ok()) {
+    // If the conversion failed because of the GPU is unavailable, store the
+    // task on the Context so it can be executed when the GPU becomes available.
+    if (status.code() == fml::StatusCode::kUnavailable) {
+      impeller_context->StoreTaskForGPU(
+          [dl_image, encode_task = std::move(encode_task),
+           is_gpu_disabled_sync_switch, impeller_context,
+           retry_runner]() mutable {
+            auto retry_task = [dl_image, encode_task = std::move(encode_task),
+                               is_gpu_disabled_sync_switch, impeller_context] {
+              fml::Status retry_status = DoConvertImageToRasterImpeller(
+                  dl_image, encode_task, is_gpu_disabled_sync_switch,
+                  impeller_context);
+              if (!retry_status.ok()) {
+                // The retry failed for some reason, maybe the GPU became
+                // unavailable again. Don't retry again, just fail in this case.
+                encode_task(retry_status);
+              }
+            };
+            // If a `retry_runner` is specified, post the retry to it, otherwise
+            // execute it directly.
+            if (retry_runner) {
+              retry_runner->PostTask(retry_task);
+            } else {
+              retry_task();
+            }
+          });
+    } else {
+      // Pass on errors that are not `kUnavailable`.
+      encode_task(status);
+    }
+  }
 }
 
 }  // namespace
@@ -153,18 +200,20 @@ void ImageEncodingImpeller::ConvertImageToRaster(
   };
 
   if (dl_image->owning_context() != DlImage::OwningContext::kRaster) {
-    DoConvertImageToRasterImpeller(dl_image, std::move(encode_task),
-                                   is_gpu_disabled_sync_switch,
-                                   impeller_context);
+    DoConvertImageToRasterImpellerWithRetry(dl_image, std::move(encode_task),
+                                            is_gpu_disabled_sync_switch,
+                                            impeller_context,
+                                            /*retry_runner=*/nullptr);
     return;
   }
 
   raster_task_runner->PostTask([dl_image, encode_task = std::move(encode_task),
                                 io_task_runner, is_gpu_disabled_sync_switch,
-                                impeller_context]() mutable {
-    DoConvertImageToRasterImpeller(dl_image, std::move(encode_task),
-                                   is_gpu_disabled_sync_switch,
-                                   impeller_context);
+                                impeller_context,
+                                raster_task_runner]() mutable {
+    DoConvertImageToRasterImpellerWithRetry(
+        dl_image, std::move(encode_task), is_gpu_disabled_sync_switch,
+        impeller_context, raster_task_runner);
   });
 }