[Impeller] Started throwing errors if dart:ui/Image.toByteData fails

gaaclarke · gaaclarke · commit 3da3a9c9de5e · 2023-10-11T09:18:28.000-07:00
diff --git a/fml/status_or.h b/fml/status_or.h
@@ -0,0 +1,66 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef FLUTTER_FML_STATUS_OR_H_
+#define FLUTTER_FML_STATUS_OR_H_
+
+#include <optional>
+
+#include "flutter/fml/status.h"
+
+namespace fml {
+
+/// TODO(): Replace with absl::StatusOr.
+template <typename T>
+class StatusOr {
+ public:
+  StatusOr(const T& value) : status_(), value_(value) {}
+  StatusOr(const Status& status) : status_(status), value_() {}
+
+  StatusOr(const StatusOr&) = default;
+  StatusOr(StatusOr&&) = default;
+
+  StatusOr& operator=(const StatusOr&) = default;
+  StatusOr& operator=(StatusOr&&) = default;
+
+  StatusOr& operator=(const T& value) {
+    status_ = Status();
+    value_ = value;
+    return *this;
+  }
+
+  StatusOr& operator=(const T&& value) {
+    status_ = Status();
+    value_ = std::move(value);
+    return *this;
+  }
+
+  StatusOr& operator=(const Status& value) {
+    status_ = value;
+    value_ = std::nullopt;
+    return *this;
+  }
+
+  const Status& status() const { return status_; }
+
+  bool ok() const { return status_.ok(); }
+
+  const T& value() const {
+    FML_CHECK(status_.ok());
+    return value_.value();
+  }
+
+  T& value() {
+    FML_CHECK(status_.ok());
+    return value_.value();
+  }
+
+ private:
+  Status status_;
+  std::optional<T> value_;
+};
+
+}  // namespace fml
+
+#endif
diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart
@@ -1956,16 +1956,23 @@ base class _Image extends NativeFieldWrapperClass1 {
   external int get height;
 
   Future<ByteData?> toByteData({ImageByteFormat format = ImageByteFormat.rawRgba}) {
-    return _futurize((_Callback<ByteData> callback) {
-      return _toByteData(format.index, (Uint8List? encoded) {
-        callback(encoded!.buffer.asByteData());
-      });
+    final Completer<ByteData?> completer = Completer<ByteData?>();
+    final String? syncError = _toByteData(format.index, (Uint8List? encoded, String? error) {
+      if (encoded != null) {
+        completer.complete(encoded.buffer.asByteData());
+      } else {
+        completer.completeError(Exception(error ?? 'unknown error'));
+      }
     });
+    if (syncError != null) {
+      completer.completeError(Exception(syncError));
+    }
+    return completer.future;
   }
 
   /// Returns an error message on failure, null on success.
   @Native<Handle Function(Pointer<Void>, Int32, Handle)>(symbol: 'Image::toByteData')
-  external String? _toByteData(int format, _Callback<Uint8List?> callback);
+  external String? _toByteData(int format, void Function(Uint8List?, String?) callback);
 
   bool _disposed = false;
   void dispose() {
diff --git a/lib/ui/painting/image_encoding.cc b/lib/ui/painting/image_encoding.cc
@@ -40,25 +40,27 @@ void FinalizeSkData(void* isolate_callback_data, void* peer) {
 }
 
 void InvokeDataCallback(std::unique_ptr<DartPersistentValue> callback,
-                        sk_sp<SkData> buffer) {
+                        fml::StatusOr<sk_sp<SkData>>&& buffer) {
   std::shared_ptr<tonic::DartState> dart_state = callback->dart_state().lock();
   if (!dart_state) {
     return;
   }
   tonic::DartState::Scope scope(dart_state);
-  if (!buffer) {
-    DartInvoke(callback->value(), {Dart_Null()});
+  if (!buffer.ok()) {
+    std::string error_copy(buffer.status().message());
+    Dart_Handle dart_string = ToDart(error_copy);
+    DartInvoke(callback->value(), {Dart_Null(), dart_string});
     return;
   }
   // Skia will not modify the buffer, and it is backed by memory that is
   // read/write, so Dart can be given direct access to the buffer through an
   // external Uint8List.
-  void* bytes = const_cast<void*>(buffer->data());
-  const intptr_t length = buffer->size();
-  void* peer = reinterpret_cast<void*>(buffer.release());
+  void* bytes = const_cast<void*>(buffer.value()->data());
+  const intptr_t length = buffer.value()->size();
+  void* peer = reinterpret_cast<void*>(buffer.value().release());
   Dart_Handle dart_data = Dart_NewExternalTypedDataWithFinalizer(
       Dart_TypedData_kUint8, bytes, length, peer, length, FinalizeSkData);
-  DartInvoke(callback->value(), {dart_data});
+  DartInvoke(callback->value(), {dart_data, Dart_Null()});
 }
 
 sk_sp<SkData> CopyImageByteData(const sk_sp<SkImage>& raster_image,
@@ -110,21 +112,30 @@ void EncodeImageAndInvokeDataCallback(
     const std::shared_ptr<const fml::SyncSwitch>& is_gpu_disabled_sync_switch,
     const std::shared_ptr<impeller::Context>& impeller_context,
     bool is_impeller_enabled) {
-  auto callback_task = fml::MakeCopyable(
-      [callback = std::move(callback)](sk_sp<SkData> encoded) mutable {
+  auto callback_task =
+      fml::MakeCopyable([callback = std::move(callback)](
+                            fml::StatusOr<sk_sp<SkData>>&& encoded) mutable {
         InvokeDataCallback(std::move(callback), std::move(encoded));
       });
   // The static leak checker gets confused by the use of fml::MakeCopyable in
   // EncodeImage.
   // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
-  auto encode_task = [callback_task = std::move(callback_task), format,
-                      ui_task_runner](const sk_sp<SkImage>& raster_image) {
-    sk_sp<SkData> encoded = EncodeImage(raster_image, format);
-    ui_task_runner->PostTask([callback_task = callback_task,
-                              encoded = std::move(encoded)]() mutable {
-      callback_task(std::move(encoded));
-    });
-  };
+  auto encode_task =
+      [callback_task = std::move(callback_task), format,
+       ui_task_runner](const fml::StatusOr<sk_sp<SkImage>>& raster_image) {
+        if (raster_image.ok()) {
+          sk_sp<SkData> encoded = EncodeImage(raster_image.value(), format);
+          ui_task_runner->PostTask([callback_task = callback_task,
+                                    encoded = std::move(encoded)]() mutable {
+            callback_task(std::move(encoded));
+          });
+        } else {
+          ui_task_runner->PostTask([callback_task = callback_task,
+                                    raster_image = raster_image]() mutable {
+            callback_task(raster_image.status());
+          });
+        }
+      };
 
   FML_DCHECK(image);
 #if IMPELLER_SUPPORTS_RENDERING
diff --git a/lib/ui/painting/image_encoding_impeller.cc b/lib/ui/painting/image_encoding_impeller.cc
@@ -58,12 +58,15 @@ sk_sp<SkImage> ConvertBufferToSkImage(
 
 void DoConvertImageToRasterImpeller(
     const sk_sp<DlImage>& dl_image,
-    std::function<void(sk_sp<SkImage>)> encode_task,
+    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) {
   is_gpu_disabled_sync_switch->Execute(
       fml::SyncSwitch::Handlers()
-          .SetIfTrue([&encode_task] { encode_task(nullptr); })
+          .SetIfTrue([&encode_task] {
+            encode_task(
+                fml::Status(fml::StatusCode::kUnavailable, "gpu unavailable"));
+          })
           .SetIfFalse([&dl_image, &encode_task, &impeller_context] {
             ImageEncodingImpeller::ConvertDlImageToSkImage(
                 dl_image, std::move(encode_task), impeller_context);
@@ -74,19 +77,21 @@ void DoConvertImageToRasterImpeller(
 
 void ImageEncodingImpeller::ConvertDlImageToSkImage(
     const sk_sp<DlImage>& dl_image,
-    std::function<void(sk_sp<SkImage>)> encode_task,
+    std::function<void(fml::StatusOr<sk_sp<SkImage>>)> encode_task,
     const std::shared_ptr<impeller::Context>& impeller_context) {
   auto texture = dl_image->impeller_texture();
 
   if (impeller_context == nullptr) {
     FML_LOG(ERROR) << "Impeller context was null.";
-    encode_task(nullptr);
+    encode_task(fml::Status(fml::StatusCode::kFailedPrecondition,
+                            "Impeller context was null."));
     return;
   }
 
   if (texture == nullptr) {
     FML_LOG(ERROR) << "Image was null.";
-    encode_task(nullptr);
+    encode_task(
+        fml::Status(fml::StatusCode::kFailedPrecondition, "Image was null."));
     return;
   }
 
@@ -95,13 +100,15 @@ void ImageEncodingImpeller::ConvertDlImageToSkImage(
 
   if (dimensions.isEmpty()) {
     FML_LOG(ERROR) << "Image dimensions were empty.";
-    encode_task(nullptr);
+    encode_task(fml::Status(fml::StatusCode::kFailedPrecondition,
+                            "Image dimensions were empty."));
     return;
   }
 
   if (!color_type.has_value()) {
     FML_LOG(ERROR) << "Failed to get color type from pixel format.";
-    encode_task(nullptr);
+    encode_task(fml::Status(fml::StatusCode::kUnimplemented,
+                            "Failed to get color type from pixel format."));
     return;
   }
 
@@ -121,7 +128,7 @@ void ImageEncodingImpeller::ConvertDlImageToSkImage(
                      encode_task = std::move(encode_task)](
                         impeller::CommandBuffer::Status status) {
     if (status != impeller::CommandBuffer::Status::kCompleted) {
-      encode_task(nullptr);
+      encode_task(fml::Status(fml::StatusCode::kUnknown, ""));
       return;
     }
     auto sk_image = ConvertBufferToSkImage(buffer, color_type, dimensions);
@@ -135,14 +142,14 @@ void ImageEncodingImpeller::ConvertDlImageToSkImage(
 
 void ImageEncodingImpeller::ConvertImageToRaster(
     const sk_sp<DlImage>& dl_image,
-    std::function<void(sk_sp<SkImage>)> encode_task,
+    std::function<void(fml::StatusOr<sk_sp<SkImage>>)> encode_task,
     const fml::RefPtr<fml::TaskRunner>& raster_task_runner,
     const fml::RefPtr<fml::TaskRunner>& io_task_runner,
     const std::shared_ptr<const fml::SyncSwitch>& is_gpu_disabled_sync_switch,
     const std::shared_ptr<impeller::Context>& impeller_context) {
   auto original_encode_task = std::move(encode_task);
   encode_task = [original_encode_task = std::move(original_encode_task),
-                 io_task_runner](sk_sp<SkImage> image) mutable {
+                 io_task_runner](fml::StatusOr<sk_sp<SkImage>> image) mutable {
     fml::TaskRunner::RunNowOrPostTask(
         io_task_runner,
         [original_encode_task = std::move(original_encode_task),
diff --git a/lib/ui/painting/image_encoding_impeller.h b/lib/ui/painting/image_encoding_impeller.h
@@ -7,6 +7,7 @@
 
 #include "flutter/common/task_runners.h"
 #include "flutter/display_list/image/dl_image.h"
+#include "flutter/fml/status_or.h"
 #include "flutter/fml/synchronization/sync_switch.h"
 
 namespace impeller {
@@ -26,14 +27,14 @@ class ImageEncodingImpeller {
   /// Visible for testing.
   static void ConvertDlImageToSkImage(
       const sk_sp<DlImage>& dl_image,
-      std::function<void(sk_sp<SkImage>)> encode_task,
+      std::function<void(fml::StatusOr<sk_sp<SkImage>>)> encode_task,
       const std::shared_ptr<impeller::Context>& impeller_context);
 
   /// Converts a DlImage to a SkImage.
   /// `encode_task` is executed with the resulting `SkImage`.
   static void ConvertImageToRaster(
       const sk_sp<DlImage>& dl_image,
-      std::function<void(sk_sp<SkImage>)> encode_task,
+      std::function<void(fml::StatusOr<sk_sp<SkImage>>)> encode_task,
       const fml::RefPtr<fml::TaskRunner>& raster_task_runner,
       const fml::RefPtr<fml::TaskRunner>& io_task_runner,
       const std::shared_ptr<const fml::SyncSwitch>& is_gpu_disabled_sync_switch,
diff --git a/lib/ui/painting/image_encoding_unittests.cc b/lib/ui/painting/image_encoding_unittests.cc
@@ -238,13 +238,14 @@ TEST(ImageEncodingImpellerTest, ConvertDlImageToSkImage16Float) {
   bool did_call = false;
   ImageEncodingImpeller::ConvertDlImageToSkImage(
       image,
-      [&did_call](const sk_sp<SkImage>& image) {
+      [&did_call](const fml::StatusOr<sk_sp<SkImage>>& image) {
         did_call = true;
-        ASSERT_TRUE(image);
-        EXPECT_EQ(100, image->width());
-        EXPECT_EQ(100, image->height());
-        EXPECT_EQ(kRGBA_F16_SkColorType, image->colorType());
-        EXPECT_EQ(nullptr, image->colorSpace());
+        ASSERT_TRUE(image.ok());
+        ASSERT_TRUE(image.value());
+        EXPECT_EQ(100, image.value()->width());
+        EXPECT_EQ(100, image.value()->height());
+        EXPECT_EQ(kRGBA_F16_SkColorType, image.value()->colorType());
+        EXPECT_EQ(nullptr, image.value()->colorSpace());
       },
       context);
   EXPECT_TRUE(did_call);
@@ -264,13 +265,14 @@ TEST(ImageEncodingImpellerTest, ConvertDlImageToSkImage10XR) {
   bool did_call = false;
   ImageEncodingImpeller::ConvertDlImageToSkImage(
       image,
-      [&did_call](const sk_sp<SkImage>& image) {
+      [&did_call](const fml::StatusOr<sk_sp<SkImage>>& image) {
         did_call = true;
-        ASSERT_TRUE(image);
-        EXPECT_EQ(100, image->width());
-        EXPECT_EQ(100, image->height());
-        EXPECT_EQ(kBGR_101010x_XR_SkColorType, image->colorType());
-        EXPECT_EQ(nullptr, image->colorSpace());
+        ASSERT_TRUE(image.ok());
+        ASSERT_TRUE(image.value());
+        EXPECT_EQ(100, image.value()->width());
+        EXPECT_EQ(100, image.value()->height());
+        EXPECT_EQ(kBGR_101010x_XR_SkColorType, image.value()->colorType());
+        EXPECT_EQ(nullptr, image.value()->colorSpace());
       },
       context);
   EXPECT_TRUE(did_call);
