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

gaaclarke · 2023-10-12T17:36:13Z

Design doc: link
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:

This keeps that functionality in one location
It makes writing tests much easier
There's no need of conditional compilation between macos and ios
There is no need to add an objc class

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 · 2023-10-13T21:39:45Z

impeller/renderer/backend/metal/context_mtl.mm


+void ContextMTL::StoreTaskForGPU(std::function<void()> task) {
+  tasks_awaiting_gpu_.emplace_back(std::move(task));
+  while (tasks_awaiting_gpu_.size() > kMaxTasksAwaitingGPU) {


Why do we need a maximum limit of tasks? If we do have a limit, do we need a mechanism to drop them gracefully?

For example, if there is a Future pending from toByteData, I'd think that dropping the task should cause an error to be thrown?

EncodeImageFailsWithoutGPUImpeller tests this, it's handled gracefully since you have to check for the GPU in the callback and we never retry more than once. Getting dropped is just like the behavior we had before we implemented the retry.

Oh, so when we exceed the size we execute the task, and then the task sees the GPU is still unavailable and errors? makes sense. I would document that a bit.

Added that info to the Context::StoreTaskForGPU docstring.

jonahwilliams · 2023-10-13T21:45:35Z

lib/ui/painting/image_encoding_impeller.cc

+  if (!status.ok()) {
+    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()) {
+                encode_task(retry_status);
+              }
+            };
+            if (retry_runner) {
+              retry_runner->PostTask(retry_task);
+            } else {
+              retry_task();
+            }
+          });
+    } else {
+      encode_task(status);


This code is a little hard to read. Are we dropping the status here in the event its not OK and the code isn't unavailable, or does that percolate up elsewhere?

I added some comments. Let me know if you think this is the right amount and if it is helpful.

gaaclarke · 2023-10-13T22:22:22Z

impeller/renderer/context.h


+  /// The maximum number of tasks that should ever be stored for
+  /// `StoreTaskForGPU`.
+  static constexpr int32_t kMaxTasksAwaitingGPU = 10;


Oh by the way. I'm open to increasing this. I just pulled this number out of nowhere but I knew I didn't want it to be completely unbounded.

I would leave a note that this was arbitrarily chosen so we're not afraid to increase it in the future.

jonahwilliams

LGTM

gaaclarke · 2023-10-16T18:02:55Z

Turns out some tests were using nullptr for the syncswitch when it shouldn't. I've updated the test. Those tests are a pain to debug.

…oByteData. (flutter/engine#46840)

…136665) flutter/engine@ca8cf6b...c24f303 2023-10-16 skia-flutter-autoroll@skia.org Roll Skia from 675f088b9ac4 to 68de6e352585 (1 revision) (flutter/engine#46956) 2023-10-16 30870216+gaaclarke@users.noreply.github.com [Impeller] implements a retry mechanism for dart:ui/Image.toByteData. (flutter/engine#46840) 2023-10-16 jason-simmons@users.noreply.github.com [Impeller] Adjust clip coverage operations to handle per-pass clip stacks (flutter/engine#46912) 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 jsimmons@google.com,rmistry@google.com,zra@google.com 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

…#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

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

chinmaygarde assigned gaaclarke Oct 12, 2023

gaaclarke force-pushed the retry-tobytedata2 branch 2 times, most recently from 556afdd to 234fcd7 Compare October 13, 2023 16:18

gaaclarke marked this pull request as ready for review October 13, 2023 20:18

gaaclarke requested review from dnfield and jonahwilliams October 13, 2023 21:29

jonahwilliams reviewed Oct 13, 2023

View reviewed changes

gaaclarke added 8 commits October 13, 2023 15:03

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

dc8231f

removed the app_state_notifier, opting for observers on the sync switch

de67844

fixed the existing test

ecff455

added test for retrying

de82a23

factored out shared code

33f2ae6

quick cleanup

4d6722c

tidy

0edb946

Added comments

cd38f20

gaaclarke force-pushed the retry-tobytedata2 branch from 46f0dde to cd38f20 Compare October 13, 2023 22:03

updated docstring

56298e9

gaaclarke commented Oct 13, 2023

View reviewed changes

more docstrings

551f6ee

jonahwilliams approved these changes Oct 13, 2023

View reviewed changes

fixed platform tests to have a syncswitch

3cd40b3

github-actions bot added the platform-ios label Oct 16, 2023

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 16, 2023

auto-submit bot merged commit 6b262a0 into flutter:main Oct 16, 2023

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

Roll Flutter Engine from ca8cf6bc256d to c24f30343cf6 (3 revisions) flutter/flutter#136665

Merged

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

6b262a052 [Impeller] implements a retry mechanism for dart:ui/Image.t…

2ef0e8b

…oByteData. (flutter/engine#46840)

dnfield mentioned this pull request Nov 17, 2023

Reland "[Impeller] Fail if software backend is chosen and Impeller is enabled on iOS." #46275

Merged

gaaclarke mentioned this pull request Apr 22, 2024

[Impeller] Picture/Layer toImage doesn't work when application is backgrounded on iOS. flutter/flutter#146990

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

gaaclarke commented Oct 12, 2023 •

edited

Loading

Uh oh!

jonahwilliams Oct 13, 2023

Uh oh!

gaaclarke Oct 13, 2023

Uh oh!

jonahwilliams Oct 13, 2023

Uh oh!

gaaclarke Oct 13, 2023 •

edited

Loading

Uh oh!

jonahwilliams Oct 13, 2023

Uh oh!

gaaclarke Oct 13, 2023

Uh oh!

gaaclarke Oct 13, 2023

Uh oh!

jonahwilliams Oct 13, 2023

Uh oh!

gaaclarke Oct 13, 2023

Uh oh!

jonahwilliams left a comment

Uh oh!

gaaclarke commented Oct 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

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

Uh oh!

Conversation

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

Uh oh!

Pre-launch Checklist

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 Oct 13, 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!

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!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Oct 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gaaclarke commented Oct 12, 2023 •

edited

Loading

gaaclarke Oct 13, 2023 •

edited

Loading