[Impeller] Avoid encoding metal commands while the GPU is unavailable when decoding images. #42349

dnfield · 2023-05-26T15:47:24Z

This disables device private upload on iOS when backgrounded, and disables mipmap generation when backgrounded.

We don't have a good way to test the core problem in this repo because it only reproduces on physical iOS hardware - simulators don't really care if you do this stuff in the background.

I'll write a devicelab test in the framework to capture this. In the mean time it can be reviewed.

We could consider making the IOManager a shared_ptr instead of having an fml::WeakPtr and that'd clean up some of the extra arguments to engine construction - or we could consider vending the sync switch from impeller::Context unconditionally, but it's pretty iOS specific...

… when decoding images.

gaaclarke

Looks good for the most part. The only thing that should get changed is removing the conditional compilation. I think we should treat sync_switch as a nonnull parameter instead. Also, I want clarity in for the true branches of the syncswitches. We should have comments that at least explain their omission.

gaaclarke · 2023-05-26T15:55:24Z

lib/ui/painting/image_decoder_impeller.cc

-      FML_DLOG(ERROR) << decode_error;
-      return std::make_pair(nullptr, decode_error);
+    std::optional<std::string> decode_error;
+#if FML_OS_IOS


Other platforms also have a sync switch that is never switched off, so there's no need to introduce conditional compilation.

Sounds good, that will actually make this more testable too.

I can refactor UploadToPrivate to work more nicely if we're on iOS.

gaaclarke · 2023-05-26T16:01:09Z

lib/ui/painting/image_decoder_impeller.cc

-      return std::make_pair(nullptr, decode_error);
+    std::optional<std::string> decode_error;
+#if FML_OS_IOS
+    gpu_disabled_switch->Execute(fml::SyncSwitch::Handlers().SetIfFalse(


Should the true branch be setting decode_error?

No - added a comment. We only need to do this unconditionally for GL,and we'll always unconditionally do it for GL.

We ripped out opengl for iOS right?

gaaclarke · 2023-05-26T16:02:59Z

lib/ui/painting/image_decoder.h

      std::shared_ptr<fml::ConcurrentTaskRunner> concurrent_task_runner,
-      fml::WeakPtr<IOManager> io_manager);
+      fml::WeakPtr<IOManager> io_manager,
+      const std::shared_ptr<fml::SyncSwitch>& gpu_disabled_switch = nullptr);


Everyone should have a syncswitch so there's no need to support nullptr.

gaaclarke · 2023-05-26T16:03:33Z

lib/ui/painting/image_decoder_impeller.cc

+                [&needs_upload, &image, &decode_error, &context,
+                 &bitmap_result] {
+                  needs_upload = false;
+                  FML_LOG(ERROR) << "Yay";


(removed the one below too)

gaaclarke · 2023-05-26T16:05:18Z

lib/ui/painting/image_decoder_impeller.cc

-#endif
+          FML_DCHECK(gpu_disabled_switch);
+          if (gpu_disabled_switch) {
+            gpu_disabled_switch->Execute(fml::SyncSwitch::Handlers().SetIfFalse(


It's worth adding a comment about the omission of a true branch.

This got moved and there's no longer an omission.

dnfield · 2023-05-26T17:02:33Z

Added a test.

Ideally this would get a screenshot based test on a real device, but that will probably take some more time to write. For now the test I added validates that if the sync switch is disabled we do not create any command buffers.

lib/ui/painting/image_decoder_impeller.cc

gaaclarke · 2023-05-26T17:34:20Z

lib/ui/painting/image_decoder_impeller.cc

+            result = UnsafeUploadTextureToPrivate(context, buffer, image_info);
+          })
+          .SetIfTrue([&result, context, bitmap, gpu_disabled_switch] {
+            // create_mips is false because we already know the GPU is disabled.


Can't this result in suboptimal rendering based on when the gpu is disabled? Are we going to get bug reports about blurry images that we can't source back to coming from backgrounded apps?

It can result in suboptimal rendering. We could choose to build mips when the GPU is available again, but that's going to be a more invasive/risky change. I'd like this to be cherry pickable.

gaaclarke · 2023-05-26T17:35:03Z

lib/ui/painting/image_decoder_impeller.cc

-      return std::make_pair(nullptr, decode_error);
+    std::optional<std::string> decode_error;
+#if FML_OS_IOS
+    gpu_disabled_switch->Execute(fml::SyncSwitch::Handlers().SetIfFalse(


We ripped out opengl for iOS right?

lib/ui/painting/image_decoder_impeller.cc

gaaclarke · 2023-05-26T17:41:51Z

shell/common/engine_unittests.cc

        /*font_collection=*/std::make_shared<FontCollection>(),
-        /*runtime_controller=*/std::move(runtime_controller_));
+        /*runtime_controller=*/std::move(runtime_controller_),
+        /*gpu_disabled_switch=*/nullptr);


nit: make syncswitches here since technically it isn't supposed to be null. We could also add a dcheck that it isn't null.

bdero · 2023-05-26T17:46:25Z

lib/ui/painting/image_decoder_impeller.cc

+            return;
+          }
+          command_buffer->WaitUntilScheduled();
+        }));


This isn't something we should solve now, but we'll eventually need a solution to defer generating mipmaps. Not generating mipmaps will cause min filtered images to continue appearing more blurry than Skia once we start properly setting the mip bias.

Filed flutter/flutter#127697

bdero · 2023-05-26T17:48:22Z

lib/ui/painting/image_decoder_impeller.cc

+            return;
+          }
+          blit_pass->SetLabel("Mipmap Blit Pass");
+          blit_pass->GenerateMipmap(texture);


The texture.NeedsMipmapGeneration() check also needs to be removed from render_pass_mtl.mm.

Should we change that to a DLOG on iOS?

I ifdef'd it out for IOS.

I think we need to rethink these safety checks in general and have the HAL be less opinionated about it. While it's unsafe for GLES and probably worth tracking in some way, fully mipmapped textures can be uploaded and/or single mip levels can be replaced individually (albeit, Impeller doesn't support this yet). One example is storing radiance maps for image based lighting.

Co-authored-by: gaaclarke <[email protected]>

bdero

LGTM

…navailable when decoding images. (flutter/engine#42349)

…127702) flutter/engine@858d975...eed12f3 2023-05-26 [email protected] Started executing vulkan unit tests with validation on macos (flutter/engine#42337) 2023-05-26 [email protected] [Impeller] Avoid encoding metal commands while the GPU is unavailable when decoding images. (flutter/engine#42349) 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Issue: flutter/flutter#126878 Picks in #42349 and #42175 The reason for the second is that the first creates many more conflicts without it, and resolving them was a lot easier. The second PR is also very low risk and improves error messaging.

[Impeller] Avoid encoding metal commands while the GPU is unavailable…

b14952e

… when decoding images.

dnfield requested review from bdero, gaaclarke and jonahwilliams May 26, 2023 15:47

gaaclarke suggested changes May 26, 2023

View reviewed changes

bdero mentioned this pull request May 26, 2023

[Impeller] Switch back to shared buffer UI image uploading on iOS #42333

Closed

dnfield added 2 commits May 26, 2023 09:32

Aaron review

86e0411

test

1cbd4cf

dnfield requested a review from gaaclarke May 26, 2023 17:22

gaaclarke approved these changes May 26, 2023

View reviewed changes

bdero suggested changes May 26, 2023

View reviewed changes

dnfield and others added 2 commits May 26, 2023 10:57

Update lib/ui/painting/image_decoder_impeller.cc

4ca1ed1

Co-authored-by: gaaclarke <[email protected]>

Update lib/ui/painting/image_decoder_impeller.cc

020469e

Co-authored-by: gaaclarke <[email protected]>

dnfield mentioned this pull request May 26, 2023

[Impeller] Generate mipmaps when GPU becomes available. flutter/flutter#127697

Open

bdero/gaaclarke review

3f456de

dnfield requested a review from bdero May 26, 2023 18:03

bdero approved these changes May 26, 2023

View reviewed changes

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label May 26, 2023

auto-submit bot merged commit 882703b into flutter:main May 26, 2023

engine-flutter-autoroll mentioned this pull request May 26, 2023

Roll Flutter Engine from 858d9753453c to eed12f36f595 (2 revisions) flutter/flutter#127702

Merged

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

882703b7f [Impeller] Avoid encoding metal commands while the GPU is u…

de34196

…navailable when decoding images. (flutter/engine#42349)

dnfield deleted the decode_safe branch May 26, 2023 20:16

dnfield mentioned this pull request May 26, 2023

Cherry picks for image decoding in Impeller #42362

Merged

[Impeller] Avoid encoding metal commands while the GPU is unavailable when decoding images. #42349

[Impeller] Avoid encoding metal commands while the GPU is unavailable when decoding images. #42349

Uh oh!

Conversation

dnfield commented May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaaclarke 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!

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!

dnfield commented May 26, 2023

Uh oh!

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!

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!

bdero May 26, 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!

bdero left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dnfield commented May 26, 2023 •

edited

Loading

bdero May 26, 2023 •

edited

Loading