Prevent app from accessing the GPU in the background in EncodeImage #28369

ColdPaleLight · 2021-08-30T17:40:50Z

about test：
(edit ColdPaleLight: I added the new test as #28369 (comment))
~~(edit ColdPaleLight: I added the test #28369 (comment))~~
~~I tried to add a test like before, but this is a bit difficult, maybe I need some help~~

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 Hixie said 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.

flutter-dashboard · 2021-08-30T17:40:52Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

gaaclarke

I'm not sure about testing. One not great idea would be to pull ConvertToRasterUsingResourceContext into a header and making templated so you can pass in a mock SyncSwitch. You could go as high as ConvertImageToRaster. Since it's just a file local function it shouldn't change much. You might want to try @chinmaygarde for better ideas.

template<typename Switch>
sk_sp<SkImage> ConvertToRasterUsingResourceContext(
    sk_sp<SkImage> image,
    GrDirectContext* resource_context
    const std::shared_ptr<Switch>& sync_switch) {
  ...
}

gaaclarke · 2021-08-30T22:27:13Z

lib/ui/painting/image_encoding.cc

@@ -132,19 +143,20 @@ void ConvertImageToRaster(sk_sp<SkImage> image,
  // to prevent concurrent usage of the image on both the IO and raster threads.
  raster_task_runner->PostTask([image, encode_task = std::move(encode_task),
                                resource_context, snapshot_delegate,
-                                io_task_runner]() {
+                                io_task_runner, &gpu_disable_sync_switch]() {


This should be capture a copy of the shared_ptr, not a reference to the shared_ptr.

gaaclarke · 2021-08-30T22:27:44Z

lib/ui/painting/image_encoding.cc

    sk_sp<SkImage> raster_image =
        snapshot_delegate->ConvertToRasterImage(image);

    io_task_runner->PostTask([image, encode_task = std::move(encode_task),
                              raster_image = std::move(raster_image),
-                              resource_context]() mutable {
+                              resource_context,
+                              &gpu_disable_sync_switch]() mutable {


Same here, capture a copy.

ColdPaleLight · 2021-08-31T12:52:29Z

I'm not sure about testing. One not great idea would be to pull ConvertToRasterUsingResourceContext into a header and making templated so you can pass in a mock SyncSwitch. You could go as high as ConvertImageToRaster. Since it's just a file local function it shouldn't change much. You might want to try @chinmaygarde for better ideas.
template<typename Switch>
sk_sp<SkImage> ConvertToRasterUsingResourceContext(
    sk_sp<SkImage> image,
    GrDirectContext* resource_context
    const std::shared_ptr<Switch>& sync_switch) {
  ...
}

I briefly tried your suggestion, and I think the change is still a bit big.Since existing tests can ensure that EncodeImage works as expected. In addition, this patch should not be easy to be accidentally revert back, so is it possible not to add additional tests?

gaaclarke

The code looks good now minus a naming nit. The existing tests don't assert that the new code is actually doing anything. A low level test that tests that the sync switch at least is being exercised is better than nothing. I wouldn't worry about how easy something is to revert.

I personally think the imperfect test is better than no test. If you strongly disagree we can defer the decision about no tests to someone else on the team.

(edit: To be clear, my request for changes has to do with the name change. The testing question I can defer to someone else on the team if you feel strongly against the tests)

gaaclarke · 2021-08-31T18:14:51Z

lib/ui/painting/image_encoding.cc

@@ -65,15 +65,24 @@ void InvokeDataCallback(std::unique_ptr<DartPersistentValue> callback,

 sk_sp<SkImage> ConvertToRasterUsingResourceContext(
    sk_sp<SkImage> image,
-    GrDirectContext* resource_context) {
+    fml::WeakPtr<GrDirectContext> resource_context,
+    const std::shared_ptr<const fml::SyncSwitch>& gpu_disable_sync_switch) {


s/gpu_disable_sync_switch/is_gpu_disabled_sync_switch

That way the "SetIfTrue" makes sense.

zanderso · 2021-08-31T18:26:34Z

I agree with @gaaclarke that this PR needs at least a simple unit test. Among other reasons, without a test, it can't be considered for a cherry-pick to the beta or stable branches if one is needed. The test @gaaclarke describes sounds adequate to me.

ColdPaleLight · 2021-09-01T07:46:30Z

Hi, @gaaclarke @zanderso
I think what you said is very reasonable, I will add a test to this PR.

(editing ColdPaleLight : I added the test: #28369 (comment) )
In addition, I have an idea, I don’t know if it is feasible.In fact, in this use case, we just want to test whether SyncSwitch::Execute has been executed. Can we change some code of SyncSwitch to make it better for testing?

For example, if SyncSwitch has a Listener, if this Listener is registered, then the Listener can be notified every time when SyncSwitch::SetSwitch or SyncSwitch::Execute is called. If SyncSwitch adds this feature, the problem of testing whether SyncSwitch::Execute is called will be able to easily resolved, I believe there will be such scenarios in the future.

Or there is a State in the SyncSwitch, and there is an execute_count_ in the State to record the number of operations. Each time SyncSwitch::Execute will increase the execute_count_. We can check whether SyncSwitch::Execute is executed by execute_count

~~In addition, I want to know whether Engine has predefined macros in the test environment. If so, can we add additional code to some classes based on the predefined macros to make it easier to test?~~

Anyway, I will add a test to this PR.

Thanks

ColdPaleLight · 2021-09-02T07:12:53Z

Hi, @gaaclarke @zanderso
I added the unit tests for this PR. I used the predefined macro FLUTTER_RELEASE to distinguish the following two cases：

If FLUTTER_RELEASE is not defined, it means that it is currently in debug or profile mode. I did the following
a. Added WasExecuted and ClearWasExecutedFlag methods to SyncSwitch to help test whether the SyncSwitch::Execute method was called, and I also added a test for new method.
b. Add a flutter::testing::ConvertToRasterUsingResourceContextForTest to image_encoding.cc, specifically for testing.
c. Add test case TEST_F(ShellTest, EncodeImageAccessesSyncSwitch)
If FLUTTER_RELEASE is defined, it means that it is currently in release mode.
Everything remains the same as before.

This Idea comes from : #25868

gaaclarke

I don't particularly like this test. I'm out-of-office but I'd like to spend some time trying to write a test for you when the holiday weekend is over. I probably won't approve something like this until you've decided this is the best you could come up with and one of us tries to come up with an alternative. If @zanderso, feels different that's fine with me.

ColdPaleLight · 2021-09-03T21:19:16Z

Hi, @gaaclarke
Thanks for your reply. In fact, I am not sure whether the current test is good or not, so your opinion is very important to me. May I ask why you don’t like this test?

You are more experienced in this field than me, so I will follow the advice you provided before to tweak the test.

Let me talk about my initial thoughts. I thought the advantages of this test are these two:

There is no change if FLUTTER_RELEASE is defined
Minimize the changes of EncodeImage, where the image_encoding.h file has no changes, and the image_encoding.cc file only adds a test method.

The disadvantage of this test is that these two： ( I guess this may be the reason you don’t like this test )

FLUTTER_RELEASE macro appears in multiple places in the code
SyncSwitch has been changed: Two methods that have nothing to do with its core functions have been added.

Then I saw that FLUTTER_RELEASE is also used in the project, so I decided to try to write this test first.

ColdPaleLight · 2021-09-04T13:37:32Z

lib/ui/painting/image_encoding.h

+#include "flutter/lib/ui/ui_dart_state.h"
+#include "third_party/skia/include/core/SkCanvas.h"
+#include "third_party/skia/include/core/SkImage.h"
+#include "third_party/skia/include/core/SkSurface.h"


Nit: This is where I am a little hesitant to use this approach. We have included more header files than before

Just pull ConvertToRasterUsingResourceContext into its own header file and include it in image_encoding.cc and image_encoding_unittests.cc. Name it something like image_encoding_impl.h. It doesn't need to be publicly visible from image_encoding.h

ColdPaleLight · 2021-09-04T14:06:08Z

Hi, @gaaclarke
I am very grateful that you are willing to spend some time writing tests for me. But I still want to try again by myself, because I think I can make it better.

I revert the previous test and wrote a new test based on your suggestion:

Pull ConvertToRasterUsingResourceContext into a header and making templated.
Add a MockSyncSwitch for test.
Add test case TEST_F(ShellTest, EncodeImageAccessesSyncSwitch)

Would you mind reviewing it again？

Thanks

gaaclarke

Nice! I like this approach much better, thanks. Sorry I didn't explain myself better last time. Your last approach relied on conditional compilation which is difficult to maintain. That's why this approach is preferred.

gaaclarke · 2021-09-07T17:03:42Z

lib/ui/painting/image_encoding_unittests.cc

+      const std::shared_ptr<const MockSyncSwitch>& is_gpu_disabled_sync_switch =
+          std::make_shared<MockSyncSwitch>();


This variable shouldn't be a reference. You can just use auto here auto is_gpu_disabled_sync_switch = std::make_shared<MockSyncSwitch>(); since the type is in the right hand expression.

gaaclarke · 2021-09-07T17:11:08Z

lib/ui/painting/image_encoding.h

+#include "flutter/lib/ui/ui_dart_state.h"
+#include "third_party/skia/include/core/SkCanvas.h"
+#include "third_party/skia/include/core/SkImage.h"
+#include "third_party/skia/include/core/SkSurface.h"


Just pull ConvertToRasterUsingResourceContext into its own header file and include it in image_encoding.cc and image_encoding_unittests.cc. Name it something like image_encoding_impl.h. It doesn't need to be publicly visible from image_encoding.h

gaaclarke · 2021-09-07T17:13:29Z

lib/ui/painting/image_encoding_unittests.cc

+                           CreateNewThread()        // io
+  );
+
+  auto nativeEncodeImage = [&](Dart_NativeArguments args) {


s/nativeEncodeImage/native_encode_image

gaaclarke · 2021-09-07T17:20:41Z

lib/ui/painting/image_encoding.cc

      if (!raster_image) {
        // The rasterizer was unable to render the cross-context image
        // (presumably because it does not have a GrContext).  In that case,
        // convert the image on the IO thread using the resource context.
-        raster_image =
-            ConvertToRasterUsingResourceContext(image, resource_context);
+        raster_image = ConvertToRasterUsingResourceContext<fml::SyncSwitch>(


Remove <fml::SyncSwitch>, the type system will do that for you.

gaaclarke

Nice work @ColdPaleLight! Nice find, nice fix, and nice followup on your PR. Thanks =)

This PR was a bit weird because it was hard to test, I think this is a good approach though considering the circumstances.

gaaclarke · 2021-09-08T16:43:01Z

lib/ui/painting/image_encoding_unittests.cc

+          .WillOnce([](const MockSyncSwitch::Handlers& handlers) {
+            handlers.true_handler();
+          });
+      ConvertToRasterUsingResourceContext<MockSyncSwitch>(


nit: remove <MockSyncSwitch>

ColdPaleLight · 2021-09-08T16:59:50Z

Nice work @ColdPaleLight! Nice find, nice fix, and nice followup on your PR. Thanks =)

This PR was a bit weird because it was hard to test, I think this is a good approach though considering the circumstances.

Thank you, @gaaclarke , I have learned a lot from you in this process. : )

This reverts commit 2ecfeb5.

This reverts commit b1278fa.

…header and making templated

… tweak the test

gaaclarke · 2021-09-08T20:39:26Z

(rebasing onto master to try to address the CI flakes)

…eImage (flutter/engine#28369)

dodatw · 2022-01-11T06:59:14Z

Is this fixing in flutter 2.8 ? It still have MTLGetWarningMode crash in flutter 2.8

ColdPaleLight · 2022-01-11T07:21:46Z

Is this fixing in flutter 2.8 ? It still have MTLGetWarningMode crash in flutter 2.8

This patch is in 2.8. I think the problem you encountered should be related to flutter/flutter#89171, you can pay attention to the 2.9 or higher version.

…lutter#28369)

flutter-dashboard bot added the needs tests label Aug 30, 2021

google-cla bot added the cla: yes label Aug 30, 2021

ColdPaleLight requested a review from gaaclarke August 30, 2021 22:22

gaaclarke requested changes Aug 30, 2021

View reviewed changes

ColdPaleLight requested a review from gaaclarke August 31, 2021 12:52

gaaclarke requested changes Aug 31, 2021

View reviewed changes

ColdPaleLight removed the needs tests label Sep 2, 2021

ColdPaleLight requested a review from gaaclarke September 2, 2021 07:16

gaaclarke reviewed Sep 3, 2021

View reviewed changes

ColdPaleLight commented Sep 4, 2021

View reviewed changes

ColdPaleLight requested a review from gaaclarke September 4, 2021 14:09

gaaclarke requested changes Sep 7, 2021

View reviewed changes

gaaclarke approved these changes Sep 8, 2021

View reviewed changes

ColdPaleLight added 9 commits September 8, 2021 13:38

Prevent app from accessing the GPU in the background in EncodeImage

ce6b36b

Capture a copy of the shared_ptr

01f9cba

Replace gpu_disable_sync_switch with is_gpu_disabled_sync_switch

21f802a

Add unit tests

7c35b13

Add Comments

fd8b731

Revert "Add Comments"

6bd913c

This reverts commit 2ecfeb5.

Revert "Add unit tests"

6b8611d

This reverts commit b1278fa.

Add unit tests by pulling ConvertToRasterUsingResourceContext into a …

ee59737

…header and making templated

Pull ConvertToRasterUsingResourceContext into its own header file and…

52a9c03

… tweak the test

gaaclarke force-pushed the image_encoding_gpu branch from ab24644 to 52a9c03 Compare September 8, 2021 20:39

Tweak the image encoding unit tests

b36f9e0

gaaclarke added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Sep 9, 2021

fluttergithubbot merged commit 00f0d00 into flutter:master Sep 9, 2021

engine-flutter-autoroll mentioned this pull request Sep 9, 2021

Roll Engine from 6a3b04632b57 to 00f0d0082241 (24 revisions) flutter/flutter#89767

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2021

00f0d00 Prevent app from accessing the GPU in the background in Encod…

b50329e

…eImage (flutter/engine#28369)

engine-flutter-autoroll mentioned this pull request Sep 9, 2021

Roll Engine from 6a3b04632b57 to d8749fa5f493 (26 revisions) flutter/flutter#89772

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2021

00f0d00 Prevent app from accessing the GPU in the background in Encod…

4eaf275

…eImage (flutter/engine#28369)

engine-flutter-autoroll mentioned this pull request Sep 9, 2021

Roll Engine from 6a3b04632b57 to 011c0f5f8ed6 (27 revisions) flutter/flutter#89778

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2021

00f0d00 Prevent app from accessing the GPU in the background in Encod…

48ec34f

…eImage (flutter/engine#28369)

engine-flutter-autoroll mentioned this pull request Sep 9, 2021

Roll Engine from 6a3b04632b57 to fd9e8d18b8d4 (28 revisions) flutter/flutter#89790

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2021

00f0d00 Prevent app from accessing the GPU in the background in Encod…

8776d3d

…eImage (flutter/engine#28369)

engine-flutter-autoroll mentioned this pull request Sep 9, 2021

Roll Engine from 6a3b04632b57 to 012730808d69 (30 revisions) flutter/flutter#89800

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2021

00f0d00 Prevent app from accessing the GPU in the background in Encod…

d158f96

…eImage (flutter/engine#28369)

engine-flutter-autoroll mentioned this pull request Sep 9, 2021

Roll Engine from 6a3b04632b57 to 65f1ca53d3af (33 revisions) flutter/flutter#89804

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 9, 2021

00f0d00 Prevent app from accessing the GPU in the background in Encod…

e2a0b85

…eImage (flutter/engine#28369)

engine-flutter-autoroll mentioned this pull request Sep 10, 2021

Roll Engine from 6a3b04632b57 to 57bc7414b894 (35 revisions) flutter/flutter#89807

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 10, 2021

00f0d00 Prevent app from accessing the GPU in the background in Encod…

7aff484

…eImage (flutter/engine#28369)

ColdPaleLight mentioned this pull request Oct 21, 2021

SIGABRT ABORT MTLGetWarningMode - CRASH flutter/flutter#76772

Closed

ColdPaleLight added the customer: alibaba label Jan 6, 2022

yzhtracy pushed a commit to BuildAndRelease/engine that referenced this pull request Feb 18, 2022

Prevent app from accessing the GPU in the background in EncodeImage (f…

27b5d23

…lutter#28369)

subzero911 mentioned this pull request Apr 25, 2022

SIGABRT ABORT MTLGetWarningMode - CRASH (recurring) flutter/flutter#102497

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent app from accessing the GPU in the background in EncodeImage #28369

Prevent app from accessing the GPU in the background in EncodeImage #28369

ColdPaleLight commented Aug 30, 2021 •

edited

Loading

flutter-dashboard bot commented Aug 30, 2021

gaaclarke left a comment

gaaclarke Aug 30, 2021

ColdPaleLight Aug 31, 2021

gaaclarke Aug 30, 2021

ColdPaleLight Aug 31, 2021

ColdPaleLight commented Aug 31, 2021 •

edited

Loading

gaaclarke left a comment •

edited

Loading

gaaclarke Aug 31, 2021

ColdPaleLight Sep 2, 2021

zanderso commented Aug 31, 2021

ColdPaleLight commented Sep 1, 2021 •

edited

Loading

ColdPaleLight commented Sep 2, 2021 •

edited

Loading

gaaclarke left a comment •

edited

Loading

ColdPaleLight commented Sep 3, 2021 •

edited

Loading

ColdPaleLight Sep 4, 2021 •

edited

Loading

gaaclarke Sep 7, 2021

ColdPaleLight Sep 8, 2021

ColdPaleLight commented Sep 4, 2021 •

edited

Loading

gaaclarke left a comment

gaaclarke Sep 7, 2021

ColdPaleLight Sep 8, 2021

gaaclarke Sep 7, 2021

gaaclarke Sep 7, 2021

ColdPaleLight Sep 8, 2021

gaaclarke Sep 7, 2021

ColdPaleLight Sep 8, 2021

gaaclarke left a comment

gaaclarke Sep 8, 2021

ColdPaleLight Sep 9, 2021

ColdPaleLight commented Sep 8, 2021

gaaclarke commented Sep 8, 2021

dodatw commented Jan 11, 2022

ColdPaleLight commented Jan 11, 2022

		const std::shared_ptr<const MockSyncSwitch>& is_gpu_disabled_sync_switch =
		std::make_shared<MockSyncSwitch>();

Prevent app from accessing the GPU in the background in EncodeImage #28369

Prevent app from accessing the GPU in the background in EncodeImage #28369

Conversation

ColdPaleLight commented Aug 30, 2021 • edited Loading

Pre-launch Checklist

flutter-dashboard bot commented Aug 30, 2021

gaaclarke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ColdPaleLight commented Aug 31, 2021 • edited Loading

gaaclarke left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanderso commented Aug 31, 2021

ColdPaleLight commented Sep 1, 2021 • edited Loading

ColdPaleLight commented Sep 2, 2021 • edited Loading

gaaclarke left a comment • edited Loading

Choose a reason for hiding this comment

ColdPaleLight commented Sep 3, 2021 • edited Loading

ColdPaleLight Sep 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ColdPaleLight commented Sep 4, 2021 • edited Loading

gaaclarke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ColdPaleLight commented Sep 8, 2021

gaaclarke commented Sep 8, 2021

dodatw commented Jan 11, 2022

ColdPaleLight commented Jan 11, 2022

ColdPaleLight commented Aug 30, 2021 •

edited

Loading

ColdPaleLight commented Aug 31, 2021 •

edited

Loading

gaaclarke left a comment •

edited

Loading

ColdPaleLight commented Sep 1, 2021 •

edited

Loading

ColdPaleLight commented Sep 2, 2021 •

edited

Loading

gaaclarke left a comment •

edited

Loading

ColdPaleLight commented Sep 3, 2021 •

edited

Loading

ColdPaleLight Sep 4, 2021 •

edited

Loading

ColdPaleLight commented Sep 4, 2021 •

edited

Loading