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

gaaclarke · 2023-10-10T22:09:43Z

issue: flutter/flutter#135245

This is a first step. Next we'll implement a retry.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
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.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

jonahwilliams

LGTM with some nits

jonahwilliams · 2023-10-11T21:27:22Z

fml/status_or.h

+
+namespace fml {
+
+/// TODO(): Replace with absl::StatusOr.


In place (or in addition to) a todo, it would be nice to document or link to documents on how this should be used.

jonahwilliams · 2023-10-11T21:28:42Z

lib/ui/painting/image_encoding_impeller.cc

  auto texture = dl_image->impeller_texture();

  if (impeller_context == nullptr) {
    FML_LOG(ERROR) << "Impeller context was null.";


Do we need the FML_LOG(ERROR) if we're surfacing the exact error message now?

jonahwilliams · 2023-10-11T21:30:03Z

lib/ui/painting.dart

+      if (encoded != null) {
+        completer.complete(encoded.buffer.asByteData());
+      } else {
+        completer.completeError(Exception(error ?? 'unknown error'));


ubernit: unknown exception

jonahwilliams · 2023-10-11T21:34:22Z

lib/ui/painting/image_encoding_impeller.cc

-          .SetIfTrue([&encode_task] { encode_task(nullptr); })
+          .SetIfTrue([&encode_task] {
+            encode_task(
+                fml::Status(fml::StatusCode::kUnavailable, "gpu unavailable"));


nit: "Gpu unavailable." . Most of the other errors seem formatted this way.

gaaclarke · 2023-10-12T15:43:12Z

rebase to flush out fuchsia infra failures now that the tree is green

gaaclarke · 2023-10-12T23:35:09Z

@jonahwilliams I had to tweak painting.dart to get the tests passing. There was some wild stuff going on with zones. Let me know if you want to take another look.

jonahwilliams · 2023-10-13T00:09:05Z

With apologies to @bdero

jonahwilliams

Still LGTM

…ata fails (flutter/engine#46738)

…136546) flutter/engine@60968c8...93f02f7 2023-10-13 [email protected] [Impeller] Started throwing errors if dart:ui/Image.toByteData fails (flutter/engine#46738) 2023-10-13 [email protected] Roll Skia from e566db061ce8 to 3acf82dcc479 (2 revisions) (flutter/engine#46883) 2023-10-13 [email protected] Fix forward declare and some deprecated enums (flutter/engine#46882) 2023-10-13 [email protected] Roll Skia from c640eeed2695 to e566db061ce8 (1 revision) (flutter/engine#46881) 2023-10-13 [email protected] Update to use GrDirectContexts::MakeVulkan (flutter/engine#46878) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

jiahaog · 2023-10-19T00:27:32Z

fml/status_or.h

@@ -0,0 +1,81 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


Passer-by comment from updating the internal build - since this is a new file, were we supposed to update BUILD.gn to specify it?

I'm seeing build errors related to being unable to find status_or.h. sponge2/34a90d3c-29af-45ca-952f-6e9733c86612

I've filed an issue: flutter/flutter#136858

…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].  [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

gaaclarke force-pushed the retry-tobytedata branch 2 times, most recently from fef13eb to 3da3a9c Compare October 11, 2023 16:19

gaaclarke changed the title ~~[Impeller] started retrying toByteData if the gpu isn't available.~~ [Impeller] Started throwing errors if dart:ui/Image.toByteData fails Oct 11, 2023

gaaclarke force-pushed the retry-tobytedata branch from f07be40 to 55a833b Compare October 11, 2023 17:42

gaaclarke marked this pull request as ready for review October 11, 2023 17:45

gaaclarke requested review from dnfield and jonahwilliams October 11, 2023 17:45

jonahwilliams approved these changes Oct 11, 2023

View reviewed changes

gaaclarke added 7 commits October 12, 2023 08:40

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

49eceff

added test

8f5d893

license update

8038a2e

tidy fix (maybe crash fix)

c472696

turned off test for platforms other than macos

98f59d4

added missing include

0c1a1b7

jonah feedback

50b344b

gaaclarke force-pushed the retry-tobytedata branch from b7f9812 to 50b344b Compare October 12, 2023 15:40

gaaclarke mentioned this pull request Oct 12, 2023

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

Merged

8 tasks

chinmaygarde assigned gaaclarke Oct 12, 2023

chinmaygarde added the e: impeller label Oct 12, 2023

gaaclarke mentioned this pull request Oct 12, 2023

Linux Framework Smoke Tests continually timing out flutter/flutter#136483

Closed

Introduced _futurizeWithError

7c89a87

github-actions bot removed the e: impeller label Oct 12, 2023

jonahwilliams approved these changes Oct 13, 2023

View reviewed changes

gaaclarke merged commit 93f02f7 into flutter:main Oct 13, 2023

engine-flutter-autoroll mentioned this pull request Oct 13, 2023

Roll Flutter Engine from 60968c892649 to 93f02f7237db (5 revisions) flutter/flutter#136546

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 13, 2023

93f02f723 [Impeller] Started throwing errors if dart:ui/Image.toByteD…

24bb175

…ata fails (flutter/engine#46738)

jiahaog reviewed Oct 19, 2023

View reviewed changes

dkwingsmt mentioned this pull request Oct 19, 2023

Internal build failure: 'status_or.h' file not found flutter/flutter#136858

Closed

denrase mentioned this pull request Oct 24, 2023

Add attachScreenshotOnlyWhenResumed to options getsentry/sentry-dart#1700

Merged

6 tasks

		@@ -0,0 +1,81 @@
		// Copyright 2013 The Flutter Authors. All rights reserved.

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

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

Uh oh!

Conversation

gaaclarke commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Oct 12, 2023

Uh oh!

gaaclarke commented Oct 12, 2023

Uh oh!

jonahwilliams commented Oct 13, 2023

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gaaclarke commented Oct 10, 2023 •

edited

Loading

dkwingsmt Oct 19, 2023 •

edited

Loading