Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Commit d3c310d

Browse files
gaaclarkeharryterkelsen
authored andcommitted
[Impeller] Started throwing errors if dart:ui/Image.toByteData fails (#46738)
issue: flutter/flutter#135245 This is a first step. Next we'll implement a retry. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
1 parent 1d243fa commit d3c310d

File tree

10 files changed

+268
-51
lines changed

10 files changed

+268
-51
lines changed

ci/licenses_golden/licenses_flutter

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1617,6 +1617,7 @@ ORIGIN: ../../../flutter/fml/shared_thread_merger.cc + ../../../flutter/LICENSE
16171617
ORIGIN: ../../../flutter/fml/shared_thread_merger.h + ../../../flutter/LICENSE
16181618
ORIGIN: ../../../flutter/fml/size.h + ../../../flutter/LICENSE
16191619
ORIGIN: ../../../flutter/fml/status.h + ../../../flutter/LICENSE
1620+
ORIGIN: ../../../flutter/fml/status_or.h + ../../../flutter/LICENSE
16201621
ORIGIN: ../../../flutter/fml/string_conversion.cc + ../../../flutter/LICENSE
16211622
ORIGIN: ../../../flutter/fml/string_conversion.h + ../../../flutter/LICENSE
16221623
ORIGIN: ../../../flutter/fml/synchronization/atomic_object.h + ../../../flutter/LICENSE
@@ -4378,6 +4379,7 @@ FILE: ../../../flutter/fml/shared_thread_merger.cc
43784379
FILE: ../../../flutter/fml/shared_thread_merger.h
43794380
FILE: ../../../flutter/fml/size.h
43804381
FILE: ../../../flutter/fml/status.h
4382+
FILE: ../../../flutter/fml/status_or.h
43814383
FILE: ../../../flutter/fml/string_conversion.cc
43824384
FILE: ../../../flutter/fml/string_conversion.h
43834385
FILE: ../../../flutter/fml/synchronization/atomic_object.h

fml/status_or.h

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
// Copyright 2013 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
#ifndef FLUTTER_FML_STATUS_OR_H_
6+
#define FLUTTER_FML_STATUS_OR_H_
7+
8+
#include <optional>
9+
10+
#include "flutter/fml/status.h"
11+
12+
namespace fml {
13+
14+
// TODO(https://github.com/flutter/flutter/issues/134741): Replace with
15+
// absl::StatusOr.
16+
/// Represents a union type of an object of type `T` and an fml::Status.
17+
///
18+
/// This is often used as a replacement for C++ exceptions where a function that
19+
/// could fail may return an error or a result. These are typically used for
20+
/// errors that are meant to be recovered from. If there is no recovery
21+
/// available `FML_CHECK` is more appropriate.
22+
///
23+
/// Example:
24+
/// StatusOr<int> div(int n, int d) {
25+
/// if (d == 0) {
26+
/// return Status(StatusCode::kFailedPrecondition, "div by zero");
27+
/// }
28+
/// return n / d;
29+
/// }
30+
template <typename T>
31+
class StatusOr {
32+
public:
33+
StatusOr(const T& value) : status_(), value_(value) {}
34+
StatusOr(const Status& status) : status_(status), value_() {}
35+
36+
StatusOr(const StatusOr&) = default;
37+
StatusOr(StatusOr&&) = default;
38+
39+
StatusOr& operator=(const StatusOr&) = default;
40+
StatusOr& operator=(StatusOr&&) = default;
41+
42+
StatusOr& operator=(const T& value) {
43+
status_ = Status();
44+
value_ = value;
45+
return *this;
46+
}
47+
48+
StatusOr& operator=(const T&& value) {
49+
status_ = Status();
50+
value_ = std::move(value);
51+
return *this;
52+
}
53+
54+
StatusOr& operator=(const Status& value) {
55+
status_ = value;
56+
value_ = std::nullopt;
57+
return *this;
58+
}
59+
60+
const Status& status() const { return status_; }
61+
62+
bool ok() const { return status_.ok(); }
63+
64+
const T& value() const {
65+
FML_CHECK(status_.ok());
66+
return value_.value();
67+
}
68+
69+
T& value() {
70+
FML_CHECK(status_.ok());
71+
return value_.value();
72+
}
73+
74+
private:
75+
Status status_;
76+
std::optional<T> value_;
77+
};
78+
79+
} // namespace fml
80+
81+
#endif

lib/ui/fixtures/ui_test.dart

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -310,7 +310,7 @@ Future<void> encodeImageProducesExternalUint8List() async {
310310
canvas.drawCircle(c, 25.0, paint);
311311
final Picture picture = pictureRecorder.endRecording();
312312
final Image image = await picture.toImage(100, 100);
313-
_encodeImage(image, ImageByteFormat.png.index, (Uint8List result) {
313+
_encodeImage(image, ImageByteFormat.png.index, (Uint8List result, String? error) {
314314
// The buffer should be non-null and writable.
315315
result[0] = 0;
316316
// The buffer should be external typed data.
@@ -319,9 +319,33 @@ Future<void> encodeImageProducesExternalUint8List() async {
319319
}
320320

321321
@pragma('vm:external-name', 'EncodeImage')
322-
external void _encodeImage(Image i, int format, void Function(Uint8List result));
322+
external void _encodeImage(Image i, int format, void Function(Uint8List result, String? error));
323323
@pragma('vm:external-name', 'ValidateExternal')
324324
external void _validateExternal(Uint8List result);
325+
@pragma('vm:external-name', 'ValidateError')
326+
external void _validateError(String? error);
327+
@pragma('vm:external-name', 'TurnOffGPU')
328+
external void _turnOffGPU();
329+
330+
@pragma('vm:entry-point')
331+
Future<void> toByteDataWithoutGPU() async {
332+
final PictureRecorder pictureRecorder = PictureRecorder();
333+
final Canvas canvas = Canvas(pictureRecorder);
334+
final Paint paint = Paint()
335+
..color = Color.fromRGBO(255, 255, 255, 1.0)
336+
..style = PaintingStyle.fill;
337+
final Offset c = Offset(50.0, 50.0);
338+
canvas.drawCircle(c, 25.0, paint);
339+
final Picture picture = pictureRecorder.endRecording();
340+
final Image image = await picture.toImage(100, 100);
341+
_turnOffGPU();
342+
try {
343+
ByteData? byteData = await image.toByteData();
344+
_validateError(null);
345+
} catch (ex) {
346+
_validateError(ex.toString());
347+
}
348+
}
325349

326350
@pragma('vm:entry-point')
327351
Future<void> pumpImage() async {

lib/ui/painting.dart

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1936,16 +1936,20 @@ base class _Image extends NativeFieldWrapperClass1 {
19361936
external int get height;
19371937

19381938
Future<ByteData?> toByteData({ImageByteFormat format = ImageByteFormat.rawRgba}) {
1939-
return _futurize((_Callback<ByteData> callback) {
1940-
return _toByteData(format.index, (Uint8List? encoded) {
1941-
callback(encoded!.buffer.asByteData());
1939+
return _futurizeWithError((_CallbackWithError<ByteData?> callback) {
1940+
return _toByteData(format.index, (Uint8List? encoded, String? error) {
1941+
if (error == null && encoded != null) {
1942+
callback(encoded.buffer.asByteData(), null);
1943+
} else {
1944+
callback(null, error);
1945+
}
19421946
});
19431947
});
19441948
}
19451949

19461950
/// Returns an error message on failure, null on success.
19471951
@Native<Handle Function(Pointer<Void>, Int32, Handle)>(symbol: 'Image::toByteData')
1948-
external String? _toByteData(int format, _Callback<Uint8List?> callback);
1952+
external String? _toByteData(int format, void Function(Uint8List?, String?) callback);
19491953

19501954
bool _disposed = false;
19511955
void dispose() {
@@ -6884,12 +6888,19 @@ base class _NativeImageDescriptor extends NativeFieldWrapperClass1 implements Im
68846888
/// Generic callback signature, used by [_futurize].
68856889
typedef _Callback<T> = void Function(T result);
68866890

6891+
/// Generic callback signature, used by [_futurizeWithError].
6892+
typedef _CallbackWithError<T> = void Function(T result, String? error);
6893+
68876894
/// Signature for a method that receives a [_Callback].
68886895
///
68896896
/// Return value should be null on success, and a string error message on
68906897
/// failure.
68916898
typedef _Callbacker<T> = String? Function(_Callback<T?> callback);
68926899

6900+
/// Signature for a method that receives a [_CallbackWithError].
6901+
/// See also: [_Callbacker]
6902+
typedef _CallbackerWithError<T> = String? Function(_CallbackWithError<T?> callback);
6903+
68936904
// Converts a method that receives a value-returning callback to a method that
68946905
// returns a Future.
68956906
//
@@ -6941,6 +6952,31 @@ Future<T> _futurize<T>(_Callbacker<T> callbacker) {
69416952
return completer.future;
69426953
}
69436954

6955+
/// A variant of `_futurize` that can communicate specific errors.
6956+
Future<T> _futurizeWithError<T>(_CallbackerWithError<T> callbacker) {
6957+
final Completer<T> completer = Completer<T>.sync();
6958+
// If the callback synchronously throws an error, then synchronously
6959+
// rethrow that error instead of adding it to the completer. This
6960+
// prevents the Zone from receiving an uncaught exception.
6961+
bool isSync = true;
6962+
final String? error = callbacker((T? t, String? error) {
6963+
if (t != null) {
6964+
completer.complete(t);
6965+
} else {
6966+
if (isSync) {
6967+
throw Exception(error ?? 'operation failed');
6968+
} else {
6969+
completer.completeError(Exception(error ?? 'operation failed'));
6970+
}
6971+
}
6972+
});
6973+
isSync = false;
6974+
if (error != null) {
6975+
throw Exception(error);
6976+
}
6977+
return completer.future;
6978+
}
6979+
69446980
/// An exception thrown by [Canvas.drawImage] and related methods when drawing
69456981
/// an [Image] created via [Picture.toImageSync] that is in an invalid state.
69466982
///

lib/ui/painting/image_encoding.cc

Lines changed: 29 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
#include "flutter/common/task_runners.h"
1212
#include "flutter/fml/build_config.h"
1313
#include "flutter/fml/make_copyable.h"
14+
#include "flutter/fml/status_or.h"
1415
#include "flutter/fml/trace_event.h"
1516
#include "flutter/lib/ui/painting/image.h"
1617
#if IMPELLER_SUPPORTS_RENDERING
@@ -40,25 +41,27 @@ void FinalizeSkData(void* isolate_callback_data, void* peer) {
4041
}
4142

4243
void InvokeDataCallback(std::unique_ptr<DartPersistentValue> callback,
43-
sk_sp<SkData> buffer) {
44+
fml::StatusOr<sk_sp<SkData>>&& buffer) {
4445
std::shared_ptr<tonic::DartState> dart_state = callback->dart_state().lock();
4546
if (!dart_state) {
4647
return;
4748
}
4849
tonic::DartState::Scope scope(dart_state);
49-
if (!buffer) {
50-
DartInvoke(callback->value(), {Dart_Null()});
50+
if (!buffer.ok()) {
51+
std::string error_copy(buffer.status().message());
52+
Dart_Handle dart_string = ToDart(error_copy);
53+
DartInvoke(callback->value(), {Dart_Null(), dart_string});
5154
return;
5255
}
5356
// Skia will not modify the buffer, and it is backed by memory that is
5457
// read/write, so Dart can be given direct access to the buffer through an
5558
// external Uint8List.
56-
void* bytes = const_cast<void*>(buffer->data());
57-
const intptr_t length = buffer->size();
58-
void* peer = reinterpret_cast<void*>(buffer.release());
59+
void* bytes = const_cast<void*>(buffer.value()->data());
60+
const intptr_t length = buffer.value()->size();
61+
void* peer = reinterpret_cast<void*>(buffer.value().release());
5962
Dart_Handle dart_data = Dart_NewExternalTypedDataWithFinalizer(
6063
Dart_TypedData_kUint8, bytes, length, peer, length, FinalizeSkData);
61-
DartInvoke(callback->value(), {dart_data});
64+
DartInvoke(callback->value(), {dart_data, Dart_Null()});
6265
}
6366

6467
sk_sp<SkData> CopyImageByteData(const sk_sp<SkImage>& raster_image,
@@ -110,21 +113,30 @@ void EncodeImageAndInvokeDataCallback(
110113
const std::shared_ptr<const fml::SyncSwitch>& is_gpu_disabled_sync_switch,
111114
const std::shared_ptr<impeller::Context>& impeller_context,
112115
bool is_impeller_enabled) {
113-
auto callback_task = fml::MakeCopyable(
114-
[callback = std::move(callback)](sk_sp<SkData> encoded) mutable {
116+
auto callback_task =
117+
fml::MakeCopyable([callback = std::move(callback)](
118+
fml::StatusOr<sk_sp<SkData>>&& encoded) mutable {
115119
InvokeDataCallback(std::move(callback), std::move(encoded));
116120
});
117121
// The static leak checker gets confused by the use of fml::MakeCopyable in
118122
// EncodeImage.
119123
// NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
120-
auto encode_task = [callback_task = std::move(callback_task), format,
121-
ui_task_runner](const sk_sp<SkImage>& raster_image) {
122-
sk_sp<SkData> encoded = EncodeImage(raster_image, format);
123-
ui_task_runner->PostTask([callback_task = callback_task,
124-
encoded = std::move(encoded)]() mutable {
125-
callback_task(std::move(encoded));
126-
});
127-
};
124+
auto encode_task =
125+
[callback_task = std::move(callback_task), format,
126+
ui_task_runner](const fml::StatusOr<sk_sp<SkImage>>& raster_image) {
127+
if (raster_image.ok()) {
128+
sk_sp<SkData> encoded = EncodeImage(raster_image.value(), format);
129+
ui_task_runner->PostTask([callback_task = callback_task,
130+
encoded = std::move(encoded)]() mutable {
131+
callback_task(std::move(encoded));
132+
});
133+
} else {
134+
ui_task_runner->PostTask([callback_task = callback_task,
135+
raster_image = raster_image]() mutable {
136+
callback_task(raster_image.status());
137+
});
138+
}
139+
};
128140

129141
FML_DCHECK(image);
130142
#if IMPELLER_SUPPORTS_RENDERING

lib/ui/painting/image_encoding_impeller.cc

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -58,12 +58,15 @@ sk_sp<SkImage> ConvertBufferToSkImage(
5858

5959
void DoConvertImageToRasterImpeller(
6060
const sk_sp<DlImage>& dl_image,
61-
std::function<void(sk_sp<SkImage>)> encode_task,
61+
std::function<void(fml::StatusOr<sk_sp<SkImage>>)> encode_task,
6262
const std::shared_ptr<const fml::SyncSwitch>& is_gpu_disabled_sync_switch,
6363
const std::shared_ptr<impeller::Context>& impeller_context) {
6464
is_gpu_disabled_sync_switch->Execute(
6565
fml::SyncSwitch::Handlers()
66-
.SetIfTrue([&encode_task] { encode_task(nullptr); })
66+
.SetIfTrue([&encode_task] {
67+
encode_task(
68+
fml::Status(fml::StatusCode::kUnavailable, "GPU unavailable."));
69+
})
6770
.SetIfFalse([&dl_image, &encode_task, &impeller_context] {
6871
ImageEncodingImpeller::ConvertDlImageToSkImage(
6972
dl_image, std::move(encode_task), impeller_context);
@@ -74,34 +77,34 @@ void DoConvertImageToRasterImpeller(
7477

7578
void ImageEncodingImpeller::ConvertDlImageToSkImage(
7679
const sk_sp<DlImage>& dl_image,
77-
std::function<void(sk_sp<SkImage>)> encode_task,
80+
std::function<void(fml::StatusOr<sk_sp<SkImage>>)> encode_task,
7881
const std::shared_ptr<impeller::Context>& impeller_context) {
7982
auto texture = dl_image->impeller_texture();
8083

8184
if (impeller_context == nullptr) {
82-
FML_LOG(ERROR) << "Impeller context was null.";
83-
encode_task(nullptr);
85+
encode_task(fml::Status(fml::StatusCode::kFailedPrecondition,
86+
"Impeller context was null."));
8487
return;
8588
}
8689

8790
if (texture == nullptr) {
88-
FML_LOG(ERROR) << "Image was null.";
89-
encode_task(nullptr);
91+
encode_task(
92+
fml::Status(fml::StatusCode::kFailedPrecondition, "Image was null."));
9093
return;
9194
}
9295

9396
auto dimensions = dl_image->dimensions();
9497
auto color_type = ToSkColorType(texture->GetTextureDescriptor().format);
9598

9699
if (dimensions.isEmpty()) {
97-
FML_LOG(ERROR) << "Image dimensions were empty.";
98-
encode_task(nullptr);
100+
encode_task(fml::Status(fml::StatusCode::kFailedPrecondition,
101+
"Image dimensions were empty."));
99102
return;
100103
}
101104

102105
if (!color_type.has_value()) {
103-
FML_LOG(ERROR) << "Failed to get color type from pixel format.";
104-
encode_task(nullptr);
106+
encode_task(fml::Status(fml::StatusCode::kUnimplemented,
107+
"Failed to get color type from pixel format."));
105108
return;
106109
}
107110

@@ -121,7 +124,7 @@ void ImageEncodingImpeller::ConvertDlImageToSkImage(
121124
encode_task = std::move(encode_task)](
122125
impeller::CommandBuffer::Status status) {
123126
if (status != impeller::CommandBuffer::Status::kCompleted) {
124-
encode_task(nullptr);
127+
encode_task(fml::Status(fml::StatusCode::kUnknown, ""));
125128
return;
126129
}
127130
auto sk_image = ConvertBufferToSkImage(buffer, color_type, dimensions);
@@ -135,14 +138,14 @@ void ImageEncodingImpeller::ConvertDlImageToSkImage(
135138

136139
void ImageEncodingImpeller::ConvertImageToRaster(
137140
const sk_sp<DlImage>& dl_image,
138-
std::function<void(sk_sp<SkImage>)> encode_task,
141+
std::function<void(fml::StatusOr<sk_sp<SkImage>>)> encode_task,
139142
const fml::RefPtr<fml::TaskRunner>& raster_task_runner,
140143
const fml::RefPtr<fml::TaskRunner>& io_task_runner,
141144
const std::shared_ptr<const fml::SyncSwitch>& is_gpu_disabled_sync_switch,
142145
const std::shared_ptr<impeller::Context>& impeller_context) {
143146
auto original_encode_task = std::move(encode_task);
144147
encode_task = [original_encode_task = std::move(original_encode_task),
145-
io_task_runner](sk_sp<SkImage> image) mutable {
148+
io_task_runner](fml::StatusOr<sk_sp<SkImage>> image) mutable {
146149
fml::TaskRunner::RunNowOrPostTask(
147150
io_task_runner,
148151
[original_encode_task = std::move(original_encode_task),

0 commit comments

Comments
 (0)