Started providing the GPU sync switch to Rasterizer.DrawToSurface() #28383

ColdPaleLight · 2021-08-31T09:47:31Z

Update

Add new method virtual bool AllowsDrawingWhenGpuDisabled() const to flutter:Surface to distinguish whether Rasterizer::DrawToSurface needs to use sync switch
Rasterizer::DrawToSurface uses sync switch on iOS's OpenGLES and Metal implementation. If Rasterizer::DrawToSurface is called in the background, this frame will be discarded.
Remove the sync switch from the external view embedders by reverting started providing the GPU sync switch to external view embedders #22302
Add some rasterizer unit tests;

Origin

Add new method virtual bool IsAllowDrawingToSurfaceWhenGpuDisabled() const to flutter:Surface to distinguish whether Rasterizer.Draw needs to use gpu_disable_sync_switch
Rasterizer.Draw uses gpu_disable_sync_switch on iOS's OpenGLES and Metal implementation. If Rasterizer.Draw is called in the background, this frame will be discarded.
Make SyncSwitch recursive

Fixes issue:
flutter/flutter#89171

~~maybe can fix: flutter/flutter#89204~~ (edit gaaclarke: we have evidence that it's a different issue).

about test:
If we confirm to use this patch, I can add tests

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-31T09:47:34Z

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 think you are right that this is the best approach to fix these bugs for now. You make a compelling argument that this could fix the linked issue but we don't 100% know. We do know it does fix some issues.

In the future we should look into implementing a solution that doesn't require a mutex lock every frame.

gaaclarke · 2021-08-31T20:13:32Z

fml/synchronization/sync_switch.h


 private:
-  mutable std::mutex mutex_;
+  mutable std::recursive_mutex mutex_;


Is there a reason this is recursive? I'd rather not make it recursive. I don't think anyone under DoDraw should be executing the sync switch, no?

Replaced here with std::recursive_mutex is to let Scenario App Integration Tests pass，This is the test result before the replacement：https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8837385340239046353/+/u/Scenario_App_Integration_Tests/stdout?format=raw.
The reason is that external view embedders also use SyncSwitch. #22302
So there are two options.

Use std::recursive_mutex instead of std::mutex. This solution has a small workload but may have poor performance

Remove the SyncSwitch used by external view embedders. This solution may have some workload

Please let me know if you prefer the second option, my original plan was to land this PR first, and then see if we can remove the SyncSwitch in the external view embedders.

I think we should do the second thing, remove the sync switch from the external view embedders, this lock supersedes that one.

Rasterizer::Draw -> Rasterizer::DoDraw -> Rasterizer::DrawToSurface -> FlutterPlatformViewsController::SubmitFrame -> SyncSwitch::Execute

However look at Rasterizer::DrawLastLayerTree, potentially that is using the sync switch since that calls Rasterizer::DrawToSurface outside of Rasterizer::Draw

Rasterizer::DrawLastLayerTree -> Rasterizer::DrawToSurface -> FlutterPlatformViewsController::SubmitFrame -> SyncSwitch::Execute

That means we should use the sync switch in Rasterizer::DrawToSurface instead of Rasterizer::Draw. Instead of removing or adding a sync switch, we can think of this as just moving the existing one in FlutterPlatformViewsController up one more layer. From the stack traces we've seen this should be a good place to have it still.

Currently, if we are in the background, the RasterStatus we return is RasterStatus::kDiscarded.I can be sure that there is no problem, because the following code also returns RasterStatus::kDiscarded in Draw.

if (discardCallback(*layer_tree.get())) { raster_status = RasterStatus::kDiscarded; }

But if we move sync switch to Rasterizer::DrawToSurface, and in the background in iOS we retuns RasterStatus::kDiscarded from Rasterizer::DrawToSurface, how should Rasterizer::DoDraw handle it?
https://github.com/flutter/engine/blob/master/shell/common/rasterizer.cc#L380-L388

RasterStatus::kDiscarded is Added by #21179.

I'd return kDiscarded from DrawToSurface and in DoDraw expand the branch that checks return codes to early out like so:

RasterStatus raster_status = DrawToSurface(*frame_timings_recorder, *layer_tree); if (raster_status == RasterStatus::kSuccess) { last_layer_tree_ = std::move(layer_tree); } else if (raster_status == RasterStatus::kResubmit || raster_status == RasterStatus::kSkipAndRetry) { resubmitted_layer_tree_ = std::move(layer_tree); return raster_status; } else if (raster_status = RasterStatus::kDiscarded) { return raster_status; }

I think that should be good.

I got it. Another problem is that we need to wrap a method outside DrawToSurface to handle sync switch (such as DrawToSurfaceWrapper), DoDraw and DrawLastLayerTree should call DrawToSurfaceWrapper instead of DrawToSurface . Here I need a good method name because I am not a native speaker.

"Wrapper" isn't that good since it doesn't tell you anything interesting about the code's semantics, just how it is structured.

How about his:

RasterStatus DrawToSurface() { RasterStatus result; ... IsGpuDisabledSyncSwitch().execute(Handlers() .SetIfTrue([]{result = kDiscarded;}) .SetIfFalse([]{result = DrawToSurfaceUnSafe()}); ... }

We use the "UnSafe" postfix in other code in the engine to denote the context with which it should be used is enforced higher in the call stack.

Is that was you were asking about?

Is that was you were asking about?

Yes, I got it

flow/surface.h

gaaclarke · 2021-08-31T20:26:06Z

shell/common/rasterizer.cc

        } else {
-          raster_status =
-              DoDraw(std::move(frame_timings_recorder), std::move(layer_tree));
+          if (!layer_tree || !surface_) {


Leave this to DoDraw, this creates an early return that didn't exist previously.

The early return here is used to ensure that surface_ is not null pointer, because we need to use it right away

if (surface_->AllowsDrawingWhenGpuDisabled()) { raster_status = DoDraw(std::move(frame_timings_recorder), std::move(layer_tree));

Good call, lets change it to:

if (!layer_tree || !surface_) { raster_status = RasterStatus::kFailed; } else if (surface_->AllowsDrawingWhenGpuDisabled()) { raster_status = DoDraw(...); } else { ... }

This removes the new early out. You can remove the check inside of DoDraw too now that it is here (replace it with asserts is probably better).

Done. I’m not sure if assertions can be used here, so I’ll just keep the original logic unchanged.

(although this becomes moot if we make the change I just suggested in the other comment, moving the sync switch to DrawToSurface)

gaaclarke · 2021-08-31T20:33:00Z

@ColdPaleLight let me know if you need any help with this PR. There are some internal customers that are interested in the fix and I'm able to take over if you don't have the time to work on this. You've been really helpful in the past, I just wanted to offer to help if you need it.

gaaclarke · 2021-08-31T21:05:38Z

Can you also make sure that "Allow edits by maintainers" is enabled. That way I can tweak the PR quickly instead of waiting for your time zone, as long as you are ok with that.

ColdPaleLight · 2021-08-31T21:24:38Z

Hi, @gaaclarke
I am going to make changes according to your request now.

…GpuDisabled

ColdPaleLight · 2021-08-31T22:05:03Z

@ColdPaleLight let me know if you need any help with this PR. There are some internal customers that are interested in the fix and I'm able to take over if you don't have the time to work on this. You've been really helpful in the past, I just wanted to offer to help if you need it.

I have plenty of time, and I will try my best to land the PR as soon as possible

ColdPaleLight · 2021-08-31T22:07:32Z

Can you also make sure that "Allow edits by maintainers" is enabled. That way I can tweak the PR quickly instead of waiting for your time zone, as long as you are ok with that.

"Allow edits by maintainers" is enable. You can directly modify the code if you need.

…ers (flutter#22302)" This reverts commit 1c3bc02.

This reverts commit d1875bd.

ColdPaleLight · 2021-09-01T11:16:16Z

I added a few unit tests to RasterizerTest, so the "needs tests" label can be removed

…h by default

ColdPaleLight · 2021-09-01T13:02:02Z

Hi, @gaaclarke
I have completed the work we planned：

move sync switch toRasterizer::DrawToSurface
Remove the sync switch from the external view embedders (Revert https://github.com/flutter/engine/pull/22302）
Revert std::recursive_mutex to std::mutex in SyncSwitch
Add 4 unit tests for the PR

You can review this PR again. By the way, my time zone is GMT + 8，You can directly edit the code yourself if you can’t contact me.

…umentation

gaaclarke

Code looks good. I updated the tests and added some comments. I'll approve this once my changes pass CI. Thanks again for all your help! =)

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

shell/common/rasterizer_unittests.cc

+  bool AllowsDrawingWhenGpuDisabled() const override {
+    return allows_drawing_when_gpu_disabled_;
+  }


This should be a mock method too.

gaaclarke · 2021-09-01T16:36:24Z

shell/common/rasterizer_unittests.cc

+    EXPECT_TRUE(result);
+    auto no_discard = [](LayerTree&) { return false; };


All of these tests have the same asserts. They aren't asserting any different behavior for the different variables.

gaaclarke · 2021-09-01T16:42:06Z

shell/common/rasterizer.cc

+  return raster_status;
+}
+
+RasterStatus Rasterizer::DrawToSurfaceUnsafe(


Add a comment about why it is unsafe.

gaaclarke

LGTM!

…face() (flutter/engine#28383)

…lutter#28383) Co-authored-by: Aaron Clarke <gaaclarke>

* Roll Skia cherrypicks * Revert "Reland enable DisplayList by default (#27892)" (#28308) * Started providing the GPU sync switch to Rasterizer.DrawToSurface() (#28383) Co-authored-by: Aaron Clarke <gaaclarke> * Update licenses_skia * update .ci.yaml Co-authored-by: Jim Graham <flar@google.com> Co-authored-by: ColdPaleLight <31977171+ColdPaleLight@users.noreply.github.com>

* Started providing the GPU sync switch to Rasterizer.DrawToSurface() (#28383) Co-authored-by: Aaron Clarke <gaaclarke> * 'add branch flutter-2.6-candidate.3 to enabled_branches in .ci.yaml' * Roll skia cherrypicks * update licenses_skia Co-authored-by: ColdPaleLight <31977171+ColdPaleLight@users.noreply.github.com>

d6638219 · 2021-09-15T06:32:35Z

@gaaclarke @christopherfujino flutter 2.0.6 requests to merge the code. flutter-1.26-candidate.17

Started providing the GPU sync switch to Rasterizer.Draw()

4fd366f

flutter-dashboard bot added the platform-ios label Aug 31, 2021

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

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

ColdPaleLight marked this pull request as draft August 31, 2021 09:47

ColdPaleLight requested review from chinmaygarde and gaaclarke August 31, 2021 09:48

ColdPaleLight mentioned this pull request Aug 31, 2021

Crash in Metal from MTLReleaseAssertionFailure flutter/flutter#89171

Closed

ColdPaleLight added 2 commits August 31, 2021 19:32

Return early when raster status is failed

b85350e

Make SyncSwitch recursive

d1875bd

ColdPaleLight marked this pull request as ready for review August 31, 2021 15:33

gaaclarke suggested changes Aug 31, 2021

View reviewed changes

Replace IsAllowDrawingToSurfaceWhenGpuDisabled with AllowsDrawingWhen…

4816654

…GpuDisabled

ColdPaleLight requested a review from gaaclarke August 31, 2021 22:07

ColdPaleLight added 7 commits September 1, 2021 06:16

Remove early return in DoDraw

01ee198

Move sync switch to DrawToSurface

290836f

Revert "started providing the GPU sync switch to external view embedd…

4d1b9b5

…ers (flutter#22302)" This reverts commit 1c3bc02.

Add rasterizer unit tests

93eaa37

Update FlatlandExternalViewEmbedder

314e22f

Format rasterizer_unittests.cc

cfd563f

Revert "Make SyncSwitch recursive"

7c1f729

This reverts commit d1875bd.

ColdPaleLight removed the needs tests label Sep 1, 2021

Let GetIsGpuDisabledSyncSwitch will return is_gpu_disabled_sync_switc…

d4b500c

…h by default

Delete unused code

3422430

flutter-dashboard bot added embedder Related to the embedder API platform-android platform-fuchsia labels Sep 1, 2021

ColdPaleLight changed the title ~~Started providing the GPU sync switch to Rasterizer.Draw()~~ Started providing the GPU sync switch to Rasterizer.DrawToSurface() Sep 1, 2021

Tweaked the tests to assert the frame is discarded and added some doc…

60260aa

…umentation

gaaclarke reviewed Sep 1, 2021

View reviewed changes

gaaclarke approved these changes Sep 1, 2021

View reviewed changes

gaaclarke merged commit be8467e into flutter:master Sep 1, 2021

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

Roll Engine from d149231127c0 to be8467ea6113 (9 revisions) flutter/flutter#89316

Merged

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

be8467e Started providing the GPU sync switch to Rasterizer.DrawToSur…

8f35509

…face() (flutter/engine#28383)

christopherfujino mentioned this pull request Sep 7, 2021

apply patches to flutter-2.5-candidate.8 flutter/flutter#89598

Closed

gaaclarke pushed a commit to gaaclarke/engine that referenced this pull request Sep 8, 2021

Started providing the GPU sync switch to Rasterizer.DrawToSurface() (f…

e12d3d2

…lutter#28383) Co-authored-by: Aaron Clarke <gaaclarke>

christopherfujino mentioned this pull request Sep 8, 2021

[flutter_releases] apply patches to flutter-2.5-candidate.8 #28520

Merged

christopherfujino mentioned this pull request Sep 9, 2021

Apply engine patches to flutter-2.6-candidate.3 flutter/flutter#89766

Closed

d6638219 mentioned this pull request Sep 15, 2021

Flutter 1.26 rasterizer gpu disable #28633

Closed

gaaclarke mentioned this pull request Oct 11, 2021

crash always in iOS 15 (19A346) flutter/flutter#90743

Closed

This was referenced Oct 21, 2021

Metal: -[MTLIOAccelCommandBuffer commandBufferResourceInfo] + 12 Crash flutter/flutter#74386

Closed

iOS crash, EXC_BAD_ACCESS KERN_INVALID_ADDRESS 0x1b29f9dac -[MTLIOAccelCommandBuffer commandBufferResourceInfo] crash flutter/flutter#90785

Closed

ColdPaleLight added the customer: alibaba label Jan 6, 2022

ColdPaleLight mentioned this pull request Jan 21, 2022

Engine should still notify framework to draw frame if frame is scheduled when lifecycle state paused flutter/flutter#45131

Closed

ColdPaleLight mentioned this pull request Feb 23, 2022

Stopped calling 'ExternalViewEmbedder::EndFrame' when pipeline is empty #31546

Merged

8 tasks

zjrwdl mentioned this pull request May 27, 2022

GrMtlCommandBuffer::getRenderCommandEncoder(MTLRenderPassDescriptor*, GrMtlOpsRenderPass*) (in Flutter) (GrMtlCommandBuffer.mm:152) flutter/flutter#104810

Closed

		EXPECT_TRUE(result);
		auto no_discard = [](LayerTree&) { return false; };

Started providing the GPU sync switch to Rasterizer.DrawToSurface() #28383

Started providing the GPU sync switch to Rasterizer.DrawToSurface() #28383

Uh oh!

Conversation

ColdPaleLight commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Update

Origin

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Aug 31, 2021

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ColdPaleLight Aug 31, 2021 • 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!

ColdPaleLight Aug 31, 2021 • 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!

gaaclarke Aug 31, 2021 • 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!

ColdPaleLight Aug 31, 2021 • 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!

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 Aug 31, 2021

Uh oh!

gaaclarke commented Aug 31, 2021

Uh oh!

ColdPaleLight commented Aug 31, 2021

Uh oh!

ColdPaleLight commented Aug 31, 2021

Uh oh!

ColdPaleLight commented Aug 31, 2021

Uh oh!

ColdPaleLight commented Sep 1, 2021

Uh oh!

ColdPaleLight commented Sep 1, 2021

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

ColdPaleLight commented Aug 31, 2021 •

edited

Loading

ColdPaleLight Aug 31, 2021 •

edited

Loading

ColdPaleLight Aug 31, 2021 •

edited

Loading

gaaclarke Aug 31, 2021 •

edited

Loading

ColdPaleLight Aug 31, 2021 •

edited

Loading

d6638219 commented Sep 15, 2021 •

edited

Loading