Only creates 'onscreen_surface_' when it's not available in 'AndroidSurfaceGL::CreateSnapshotSurface' #30590

ColdPaleLight · 2021-12-30T07:49:49Z

There will be a situation where AndroidSurfaceGL::on_screen_surface_ is set, but Rasterizer::surface_ has not been set. So when'AndroidSurfaceGL::CreatePbufferSurface' is called, AndroidSurfaceGL::on_screen_surface_ may already exist. At this time, we should not generate a new one, but should directly use the existing one.

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-12-30T07:49:51Z

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.

dnfield · 2021-12-30T14:16:24Z

shell/platform/android/android_surface_gl.cc


 std::unique_ptr<Surface> AndroidSurfaceGL::CreatePbufferSurface() {
-  onscreen_surface_ = GLContextPtr()->CreatePbufferSurface();
+  if (!onscreen_surface_ || !onscreen_surface_->IsValid()) {


Maybe just an early return instead? This needs a test too.

If we early return, then Rasterizer::DoMakeRasterSnapshot will execute the following logic, which will change from hardware rasterization to software rasterization. The performance will be a little bit worse, but the logic should be correct. If you think an early return is more reasonable, I'm happy to tweak the code.

engine/shell/common/rasterizer.cc

Lines 292 to 297 in 30f048d

if (!snapshot_surface) {

// Raster surface is fine if there is no on screen surface. This might

// happen in case of software rendering.

sk_sp<SkSurface> sk_surface = SkSurface::MakeRaster(image_info);

result = DrawSnapshot(sk_surface, draw_callback);

} else {

And about test, I'm not sure how to test this, would you mind giving me some suggestions ? Thank you :) .

I thought about it again, the early return will introduce a new problem. If Rasterizer::DoMakeRasterSnapshot is executed multiple times when the app is in the background. The first time there is no problem, and AndroidSurfaceGL::on_screen_surface_ which size is 1x1 is created, but the second time, because AndroidSurfaceGL::on_screen_surface_ already exists, it is returned early. As a result, Rasterizer::DoMakeRasterSnapshot has changed from hardware rasterization to software rasterization.

I think the part I'm not clear on is how we'd get here.

I think the part I'm not clear on is how we'd get here.

Although I can't provide a reproducible sample, it appeared many times in my application, and I finally found the reason for the problem by printing the Log, and the problem was triggered by this line of code.
https://github.com/flutter/flutter/blob/019f03374c5b106c3fde05c8eee930cd26b8ffff/packages/flutter/lib/src/painting/shader_warm_up.dart#L95

I think this is a combination of a few things:

The platform thread creates the onscreen surface

It then calls Shell::NotifyCreated

NotifyCreated fire and forgets the UI task runner, which might schedule an image decode (this is a recent change, from November or Decemberish)

The rasterizer setup is called, but now there's a race where the UI thread might have asked the rasterizer to make a snapshot before it has been setup

If the UI thread beats rasterizer setup, the rasterizer will say it doesn't have a surface to work with and ask for a snapshotting surface. The AndroidContextGL creates a pbuffer surface for this and sets it as the onscreen_surface_, which destroys the one we actually want.

So what I think should happen here is the following:

Rename AndroidSurfaceGL::CreatePbufferSurface to something like AndroidSurfaceGL::GetOrCreateSnapshotSurface.

Have that renamed method only create the Pbuffer surface if onscreen_surface_ is null/invalid. Otherwise, return onscreen_surface_.

Write a test that exercises this, e.g. latch the raster task runner before NotifyCreated is called, run some Dart code that calls and completes toImage, unlatch the raster task runner, make sure the rasterizer surface is the right one.

Have that renamed method only create the Pbuffer surface if onscreen_surface_ is null/invalid. Otherwise, return onscreen_surface_.

What happens if the buffer isn't a Pbuffer at this time? Does it make sense to destroy the existing surface and then create the actual Pbuffer?

No. We just want to make sure that we have a GPU backed surface and don't end up in the software backend code.

So what I think should happen here is the following:

Rename AndroidSurfaceGL::CreatePbufferSurface to something like AndroidSurfaceGL::GetOrCreateSnapshotSurface.

Have that renamed method only create the Pbuffer surface if onscreen_surface_ is null/invalid. Otherwise, return onscreen_surface_.

Write a test that exercises this, e.g. latch the raster task runner before NotifyCreated is called, run some Dart code that calls and completes toImage, unlatch the raster task runner, make sure the rasterizer surface is the right one.

@dnfield
We can't return onscreen_surface_ because the type of the return value is std::unique_ptr<Surface>, but the type of onscreen_surface_ is std::unique_ptr<AndroidEGLSurface>. So in either case we have to create a new std::unique_ptr<Surface>. so I renamed it to AndroidSurfaceGL::CreateSnapshotSurface. I also added test cases as you said in discord.

…urfaceGL::CreatePbufferSurface'

dnfield · 2022-01-13T17:14:51Z

shell/platform/android/android_surface_gl.h

  // |GPUSurfaceGLDelegate|
  sk_sp<const GrGLInterface> GetGLInterface() const override;

+  AndroidEGLSurface* GetOnscreenSurface() const {


Please add a doc comment, probably explaining that this is only intended for tests.

e.g.

// Obtain a raw pointer to the AndroidEGLSurface. This method is intended for use in tests. Callers must not delete this pointer.

(I went ahead and made this change so we can just get this landed, rather than wait for another time zone cycle :)

dnfield

LGTM with doc nit

…AndroidSurfaceGL::CreateSnapshotSurface' (flutter/engine#30590)

…urfaceGL::CreateSnapshotSurface' (flutter#30590) * Only creates 'onscreen_surface_' when it's not available in 'AndroidSurfaceGL::CreatePbufferSurface' * Rename the function and add some unit tests * Update android_surface_gl.h Co-authored-by: Dan Field <[email protected]>

flutter-dashboard bot added the platform-android label Dec 30, 2021

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

ColdPaleLight requested a review from dnfield December 30, 2021 09:12

dnfield reviewed Dec 30, 2021

View reviewed changes

ColdPaleLight added the customer: alibaba label Jan 6, 2022

Only creates 'onscreen_surface_' when it's not available in 'AndroidS…

499f597

…urfaceGL::CreatePbufferSurface'

AlexV525 mentioned this pull request Jan 13, 2022

[Android] Running an app with debug scene when the device is locked cause a black screen on Android flutter/flutter#94921

Closed

Rename the function and add some unit tests

822d4af

ColdPaleLight force-pushed the android_pbuffer branch from 754f1c4 to 822d4af Compare January 13, 2022 13:54

ColdPaleLight requested a review from dnfield January 13, 2022 14:09

ColdPaleLight changed the title ~~Only creates 'onscreen_surface_' when it's not available in 'AndroidSurfaceGL::CreatePbufferSurface'~~ Only creates 'onscreen_surface_' when it's not available in 'AndroidSurfaceGL::CreateSnapshotSurface' Jan 13, 2022

dnfield reviewed Jan 13, 2022

View reviewed changes

dnfield approved these changes Jan 13, 2022

View reviewed changes

Update android_surface_gl.h

87b3d63

dnfield 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 Jan 13, 2022

dnfield merged commit fab1982 into flutter:main Jan 13, 2022

dnfield mentioned this pull request Jan 13, 2022

[Android] App shows black screen after being in the background. flutter/flutter#93276

Closed

engine-flutter-autoroll mentioned this pull request Jan 13, 2022

Roll Engine from e29aad31a30d to fab19829949d (5 revisions) flutter/flutter#96620

Merged

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

fab1982 Only creates 'onscreen_surface_' when it's not available in '…

f065f8a

…AndroidSurfaceGL::CreateSnapshotSurface' (flutter/engine#30590)

	if (!snapshot_surface) {
	// Raster surface is fine if there is no on screen surface. This might
	// happen in case of software rendering.
	sk_sp<SkSurface> sk_surface = SkSurface::MakeRaster(image_info);
	result = DrawSnapshot(sk_surface, draw_callback);
	} else {

Only creates 'onscreen_surface_' when it's not available in 'AndroidSurfaceGL::CreateSnapshotSurface' #30590

Only creates 'onscreen_surface_' when it's not available in 'AndroidSurfaceGL::CreateSnapshotSurface' #30590

Uh oh!

Conversation

ColdPaleLight commented Dec 30, 2021 • edited by dnfield Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Dec 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ColdPaleLight Dec 30, 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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ColdPaleLight commented Dec 30, 2021 •

edited by dnfield

Loading

ColdPaleLight Dec 30, 2021 •

edited

Loading