From 49eceffed3719e09097dd07dd333e00a6dc676f2 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 10 Oct 2023 15:02:14 -0700 Subject: [PATCH 1/8] [Impeller] Started throwing errors if dart:ui/Image.toByteData fails --- fml/status_or.h | 66 +++++++++++++++++++++ lib/ui/painting.dart | 17 ++++-- lib/ui/painting/image_encoding.cc | 45 ++++++++------ lib/ui/painting/image_encoding_impeller.cc | 27 +++++---- lib/ui/painting/image_encoding_impeller.h | 5 +- lib/ui/painting/image_encoding_unittests.cc | 26 ++++---- 6 files changed, 140 insertions(+), 46 deletions(-) create mode 100644 fml/status_or.h diff --git a/fml/status_or.h b/fml/status_or.h new file mode 100644 index 0000000000000..80c39d01dddd9 --- /dev/null +++ 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 + +#include "flutter/fml/status.h" + +namespace fml { + +/// TODO(): Replace with absl::StatusOr. +template +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 value_; +}; + +} // namespace fml + +#endif diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index cfb5e2348f06f..5b325bdfac87f 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -1936,16 +1936,23 @@ base class _Image extends NativeFieldWrapperClass1 { external int get height; Future toByteData({ImageByteFormat format = ImageByteFormat.rawRgba}) { - return _futurize((_Callback callback) { - return _toByteData(format.index, (Uint8List? encoded) { - callback(encoded!.buffer.asByteData()); - }); + final Completer completer = Completer(); + 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, Int32, Handle)>(symbol: 'Image::toByteData') - external String? _toByteData(int format, _Callback 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 index dfd6daf130c61..07bc22194df0f 100644 --- 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 callback, - sk_sp buffer) { + fml::StatusOr>&& buffer) { std::shared_ptr 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(buffer->data()); - const intptr_t length = buffer->size(); - void* peer = reinterpret_cast(buffer.release()); + void* bytes = const_cast(buffer.value()->data()); + const intptr_t length = buffer.value()->size(); + void* peer = reinterpret_cast(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 CopyImageByteData(const sk_sp& raster_image, @@ -110,21 +112,30 @@ void EncodeImageAndInvokeDataCallback( const std::shared_ptr& is_gpu_disabled_sync_switch, const std::shared_ptr& impeller_context, bool is_impeller_enabled) { - auto callback_task = fml::MakeCopyable( - [callback = std::move(callback)](sk_sp encoded) mutable { + auto callback_task = + fml::MakeCopyable([callback = std::move(callback)]( + fml::StatusOr>&& 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& raster_image) { - sk_sp 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>& raster_image) { + if (raster_image.ok()) { + sk_sp 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 index 445bc2bfa5eb2..524f2b79f283a 100644 --- a/lib/ui/painting/image_encoding_impeller.cc +++ b/lib/ui/painting/image_encoding_impeller.cc @@ -58,12 +58,15 @@ sk_sp ConvertBufferToSkImage( void DoConvertImageToRasterImpeller( const sk_sp& dl_image, - std::function)> encode_task, + std::function>)> encode_task, const std::shared_ptr& is_gpu_disabled_sync_switch, const std::shared_ptr& 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& dl_image, - std::function)> encode_task, + std::function>)> encode_task, const std::shared_ptr& 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& dl_image, - std::function)> encode_task, + std::function>)> encode_task, const fml::RefPtr& raster_task_runner, const fml::RefPtr& io_task_runner, const std::shared_ptr& is_gpu_disabled_sync_switch, const std::shared_ptr& 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 image) mutable { + io_task_runner](fml::StatusOr> 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 index 18fc3d90650bf..84894407854a3 100644 --- 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& dl_image, - std::function)> encode_task, + std::function>)> encode_task, const std::shared_ptr& impeller_context); /// Converts a DlImage to a SkImage. /// `encode_task` is executed with the resulting `SkImage`. static void ConvertImageToRaster( const sk_sp& dl_image, - std::function)> encode_task, + std::function>)> encode_task, const fml::RefPtr& raster_task_runner, const fml::RefPtr& io_task_runner, const std::shared_ptr& is_gpu_disabled_sync_switch, diff --git a/lib/ui/painting/image_encoding_unittests.cc b/lib/ui/painting/image_encoding_unittests.cc index 46c1e15ef4dfe..d3299a5e4f25d 100644 --- 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& image) { + [&did_call](const fml::StatusOr>& 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& image) { + [&did_call](const fml::StatusOr>& 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); From 8f5d893ed66ce650941e29d19c298d85319565bf Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Wed, 11 Oct 2023 10:33:12 -0700 Subject: [PATCH 2/8] added test --- lib/ui/fixtures/ui_test.dart | 28 ++++++++++++- lib/ui/painting/image_encoding_unittests.cc | 44 +++++++++++++++++++++ shell/common/shell_test.cc | 4 ++ shell/common/shell_test.h | 2 + 4 files changed, 76 insertions(+), 2 deletions(-) diff --git a/lib/ui/fixtures/ui_test.dart b/lib/ui/fixtures/ui_test.dart index 3cc93a746921a..a4b8ada735b2c 100644 --- a/lib/ui/fixtures/ui_test.dart +++ b/lib/ui/fixtures/ui_test.dart @@ -310,7 +310,7 @@ Future encodeImageProducesExternalUint8List() async { canvas.drawCircle(c, 25.0, paint); final Picture picture = pictureRecorder.endRecording(); final Image image = await picture.toImage(100, 100); - _encodeImage(image, ImageByteFormat.png.index, (Uint8List result) { + _encodeImage(image, ImageByteFormat.png.index, (Uint8List result, String? error) { // The buffer should be non-null and writable. result[0] = 0; // The buffer should be external typed data. @@ -319,9 +319,33 @@ Future encodeImageProducesExternalUint8List() async { } @pragma('vm:external-name', 'EncodeImage') -external void _encodeImage(Image i, int format, void Function(Uint8List result)); +external void _encodeImage(Image i, int format, void Function(Uint8List result, String? error)); @pragma('vm:external-name', 'ValidateExternal') external void _validateExternal(Uint8List result); +@pragma('vm:external-name', 'ValidateError') +external void _validateError(String? error); +@pragma('vm:external-name', 'TurnOffGPU') +external void _turnOffGPU(); + +@pragma('vm:entry-point') +Future toByteDataWithoutGPU() 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(); + try { + ByteData? byteData = await image.toByteData(); + _validateError(null); + } catch (ex) { + _validateError(ex.toString()); + } +} @pragma('vm:entry-point') Future pumpImage() async { diff --git a/lib/ui/painting/image_encoding_unittests.cc b/lib/ui/painting/image_encoding_unittests.cc index d3299a5e4f25d..754cdcb86d5bc 100644 --- a/lib/ui/painting/image_encoding_unittests.cc +++ b/lib/ui/painting/image_encoding_unittests.cc @@ -224,6 +224,50 @@ std::shared_ptr MakeConvertDlImageToSkImageContext( } } // namespace +TEST_F(ShellTest, EncodeImageFailsWithoutGPUImpeller) { + Settings settings = CreateSettingsForFixture(); + settings.enable_impeller = true; + TaskRunners task_runners("test", // label + GetCurrentTaskRunner(), // platform + CreateNewThread(), // raster + CreateNewThread(), // ui + CreateNewThread() // io + ); + + auto native_validate_error = [&](Dart_NativeArguments args) { + auto handle = Dart_GetNativeArgument(args, 0); + + EXPECT_FALSE(Dart_IsNull(handle)); + + message_latch.Signal(); + }; + + AddNativeCallback("ValidateError", + CREATE_NATIVE_ENTRY(native_validate_error)); + + std::unique_ptr shell = CreateShell({ + .settings = settings, + .task_runners = std::move(task_runners), + }); + + auto turn_off_gpu = [&](Dart_NativeArguments args) { + TurnOffGPU(shell.get()); + }; + + AddNativeCallback("TurnOffGPU", CREATE_NATIVE_ENTRY(turn_off_gpu)); + + ASSERT_TRUE(shell->IsSetup()); + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("toByteDataWithoutGPU"); + + shell->RunEngine(std::move(configuration), [&](auto result) { + ASSERT_EQ(result, Engine::RunStatus::Success); + }); + + message_latch.Wait(); + DestroyShell(std::move(shell), task_runners); +} + TEST(ImageEncodingImpellerTest, ConvertDlImageToSkImage16Float) { sk_sp image(new MockDlImage()); EXPECT_CALL(*image, dimensions) diff --git a/shell/common/shell_test.cc b/shell/common/shell_test.cc index 1ea8c7f0b3248..f9f59f7366de8 100644 --- a/shell/common/shell_test.cc +++ b/shell/common/shell_test.cc @@ -382,5 +382,9 @@ size_t ShellTest::GetLiveTrackedPathCount( }); } +void ShellTest::TurnOffGPU(Shell* shell) { + shell->is_gpu_disabled_sync_switch_->SetSwitch(true); +} + } // namespace testing } // namespace flutter diff --git a/shell/common/shell_test.h b/shell/common/shell_test.h index 1fd588e3b8447..dfcc22223e5ac 100644 --- a/shell/common/shell_test.h +++ b/shell/common/shell_test.h @@ -135,6 +135,8 @@ class ShellTest : public FixtureTest { static size_t GetLiveTrackedPathCount( const std::shared_ptr& tracker); + static void TurnOffGPU(Shell* shell); + private: ThreadHost thread_host_; From 8038a2eb6ee6368bb6a06a372b63ffce700e06ab Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Wed, 11 Oct 2023 10:41:38 -0700 Subject: [PATCH 3/8] license update --- ci/licenses_golden/licenses_flutter | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index 589e36106dd78..a6e3a6f30a1a3 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -1617,6 +1617,7 @@ ORIGIN: ../../../flutter/fml/shared_thread_merger.cc + ../../../flutter/LICENSE ORIGIN: ../../../flutter/fml/shared_thread_merger.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/fml/size.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/fml/status.h + ../../../flutter/LICENSE +ORIGIN: ../../../flutter/fml/status_or.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/fml/string_conversion.cc + ../../../flutter/LICENSE ORIGIN: ../../../flutter/fml/string_conversion.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/fml/synchronization/atomic_object.h + ../../../flutter/LICENSE @@ -4377,6 +4378,7 @@ FILE: ../../../flutter/fml/shared_thread_merger.cc FILE: ../../../flutter/fml/shared_thread_merger.h FILE: ../../../flutter/fml/size.h FILE: ../../../flutter/fml/status.h +FILE: ../../../flutter/fml/status_or.h FILE: ../../../flutter/fml/string_conversion.cc FILE: ../../../flutter/fml/string_conversion.h FILE: ../../../flutter/fml/synchronization/atomic_object.h From c4726961853389aa0f422982a2d6c3f0de46bb05 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Wed, 11 Oct 2023 11:23:06 -0700 Subject: [PATCH 4/8] tidy fix (maybe crash fix) --- lib/ui/painting/image_encoding_unittests.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ui/painting/image_encoding_unittests.cc b/lib/ui/painting/image_encoding_unittests.cc index 754cdcb86d5bc..61f712bd7e28b 100644 --- a/lib/ui/painting/image_encoding_unittests.cc +++ b/lib/ui/painting/image_encoding_unittests.cc @@ -247,7 +247,7 @@ TEST_F(ShellTest, EncodeImageFailsWithoutGPUImpeller) { std::unique_ptr shell = CreateShell({ .settings = settings, - .task_runners = std::move(task_runners), + .task_runners = task_runners, }); auto turn_off_gpu = [&](Dart_NativeArguments args) { From 98f59d4ee7f5d5a034e0c3ba49b183d431178d6d Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Wed, 11 Oct 2023 13:24:31 -0700 Subject: [PATCH 5/8] turned off test for platforms other than macos --- lib/ui/painting/image_encoding_unittests.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/ui/painting/image_encoding_unittests.cc b/lib/ui/painting/image_encoding_unittests.cc index 61f712bd7e28b..cf8abf76b2f4f 100644 --- a/lib/ui/painting/image_encoding_unittests.cc +++ b/lib/ui/painting/image_encoding_unittests.cc @@ -23,6 +23,8 @@ // CREATE_NATIVE_ENTRY is leaky by design // NOLINTBEGIN(clang-analyzer-core.StackAddressEscape) +#pragma GCC diagnostic ignored "-Wunreachable-code" + namespace flutter { namespace testing { @@ -225,6 +227,10 @@ std::shared_ptr MakeConvertDlImageToSkImageContext( } // namespace TEST_F(ShellTest, EncodeImageFailsWithoutGPUImpeller) { +#ifndef FML_OS_MACOSX + // Only works on macos currently. + GTEST_SKIP(); +#endif Settings settings = CreateSettingsForFixture(); settings.enable_impeller = true; TaskRunners task_runners("test", // label From 0c1a1b70e3b577f79b469b12f4b0129a35e5813c Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Wed, 11 Oct 2023 14:19:53 -0700 Subject: [PATCH 6/8] added missing include --- lib/ui/painting/image_encoding.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/ui/painting/image_encoding.cc b/lib/ui/painting/image_encoding.cc index 07bc22194df0f..69d3bd38f87ba 100644 --- a/lib/ui/painting/image_encoding.cc +++ b/lib/ui/painting/image_encoding.cc @@ -11,6 +11,7 @@ #include "flutter/common/task_runners.h" #include "flutter/fml/build_config.h" #include "flutter/fml/make_copyable.h" +#include "flutter/fml/status_or.h" #include "flutter/fml/trace_event.h" #include "flutter/lib/ui/painting/image.h" #if IMPELLER_SUPPORTS_RENDERING From 50b344bbc6279949d7ca1ca052856e651d75fcf9 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Wed, 11 Oct 2023 14:44:50 -0700 Subject: [PATCH 7/8] jonah feedback --- fml/status_or.h | 17 ++++++++++++++++- lib/ui/painting.dart | 2 +- lib/ui/painting/image_encoding_impeller.cc | 6 +----- 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/fml/status_or.h b/fml/status_or.h index 80c39d01dddd9..33718da74f126 100644 --- a/fml/status_or.h +++ b/fml/status_or.h @@ -11,7 +11,22 @@ namespace fml { -/// TODO(): Replace with absl::StatusOr. +// TODO(https://github.com/flutter/flutter/issues/134741): Replace with +// absl::StatusOr. +/// Represents a union type of an object of type `T` and an fml::Status. +/// +/// This is often used as a replacement for C++ exceptions where a function that +/// could fail may return an error or a result. These are typically used for +/// errors that are meant to be recovered from. If there is no recovery +/// available `FML_CHECK` is more appropriate. +/// +/// Example: +/// StatusOr div(int n, int d) { +/// if (d == 0) { +/// return Status(StatusCode::kFailedPrecondition, "div by zero"); +/// } +/// return n / d; +/// } template class StatusOr { public: diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index 5b325bdfac87f..28eb159b58747 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -1941,7 +1941,7 @@ base class _Image extends NativeFieldWrapperClass1 { if (encoded != null) { completer.complete(encoded.buffer.asByteData()); } else { - completer.completeError(Exception(error ?? 'unknown error')); + completer.completeError(Exception(error ?? 'Unknown exception.')); } }); if (syncError != null) { diff --git a/lib/ui/painting/image_encoding_impeller.cc b/lib/ui/painting/image_encoding_impeller.cc index 524f2b79f283a..4e403d5d3f2bf 100644 --- a/lib/ui/painting/image_encoding_impeller.cc +++ b/lib/ui/painting/image_encoding_impeller.cc @@ -65,7 +65,7 @@ void DoConvertImageToRasterImpeller( fml::SyncSwitch::Handlers() .SetIfTrue([&encode_task] { encode_task( - fml::Status(fml::StatusCode::kUnavailable, "gpu unavailable")); + fml::Status(fml::StatusCode::kUnavailable, "GPU unavailable.")); }) .SetIfFalse([&dl_image, &encode_task, &impeller_context] { ImageEncodingImpeller::ConvertDlImageToSkImage( @@ -82,14 +82,12 @@ void ImageEncodingImpeller::ConvertDlImageToSkImage( auto texture = dl_image->impeller_texture(); if (impeller_context == nullptr) { - FML_LOG(ERROR) << "Impeller context was null."; encode_task(fml::Status(fml::StatusCode::kFailedPrecondition, "Impeller context was null.")); return; } if (texture == nullptr) { - FML_LOG(ERROR) << "Image was null."; encode_task( fml::Status(fml::StatusCode::kFailedPrecondition, "Image was null.")); return; @@ -99,14 +97,12 @@ void ImageEncodingImpeller::ConvertDlImageToSkImage( auto color_type = ToSkColorType(texture->GetTextureDescriptor().format); if (dimensions.isEmpty()) { - FML_LOG(ERROR) << "Image dimensions were empty."; 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(fml::Status(fml::StatusCode::kUnimplemented, "Failed to get color type from pixel format.")); return; From 7c89a87779e85e37ba5a6839623266b50f002ba6 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Thu, 12 Oct 2023 15:39:47 -0700 Subject: [PATCH 8/8] Introduced _futurizeWithError --- lib/ui/painting.dart | 51 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 40 insertions(+), 11 deletions(-) diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index 28eb159b58747..1fbf1cae9fdd0 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -1936,18 +1936,15 @@ base class _Image extends NativeFieldWrapperClass1 { external int get height; Future toByteData({ImageByteFormat format = ImageByteFormat.rawRgba}) { - final Completer completer = Completer(); - final String? syncError = _toByteData(format.index, (Uint8List? encoded, String? error) { - if (encoded != null) { - completer.complete(encoded.buffer.asByteData()); - } else { - completer.completeError(Exception(error ?? 'Unknown exception.')); - } + return _futurizeWithError((_CallbackWithError callback) { + return _toByteData(format.index, (Uint8List? encoded, String? error) { + if (error == null && encoded != null) { + callback(encoded.buffer.asByteData(), null); + } else { + callback(null, error); + } + }); }); - if (syncError != null) { - completer.completeError(Exception(syncError)); - } - return completer.future; } /// Returns an error message on failure, null on success. @@ -6891,12 +6888,19 @@ base class _NativeImageDescriptor extends NativeFieldWrapperClass1 implements Im /// Generic callback signature, used by [_futurize]. typedef _Callback = void Function(T result); +/// Generic callback signature, used by [_futurizeWithError]. +typedef _CallbackWithError = void Function(T result, String? error); + /// Signature for a method that receives a [_Callback]. /// /// Return value should be null on success, and a string error message on /// failure. typedef _Callbacker = String? Function(_Callback callback); +/// Signature for a method that receives a [_CallbackWithError]. +/// See also: [_Callbacker] +typedef _CallbackerWithError = String? Function(_CallbackWithError callback); + // Converts a method that receives a value-returning callback to a method that // returns a Future. // @@ -6948,6 +6952,31 @@ Future _futurize(_Callbacker callbacker) { return completer.future; } +/// A variant of `_futurize` that can communicate specific errors. +Future _futurizeWithError(_CallbackerWithError callbacker) { + final Completer completer = Completer.sync(); + // If the callback synchronously throws an error, then synchronously + // rethrow that error instead of adding it to the completer. This + // prevents the Zone from receiving an uncaught exception. + bool isSync = true; + final String? error = callbacker((T? t, String? error) { + if (t != null) { + completer.complete(t); + } else { + if (isSync) { + throw Exception(error ?? 'operation failed'); + } else { + completer.completeError(Exception(error ?? 'operation failed')); + } + } + }); + isSync = false; + if (error != null) { + throw Exception(error); + } + return completer.future; +} + /// An exception thrown by [Canvas.drawImage] and related methods when drawing /// an [Image] created via [Picture.toImageSync] that is in an invalid state. ///