Use a Pbuffer surface when the onscreen surface is not available for snapshotting on Android

[dnfield]

When rasterizing a Picture or Scene, we normally try to use the on screen surface. If it's not available, we go down a raster based path. However, this won't work if the image contains images that were uploaded via the resource context - Skia won't draw the pixels into the resulting picture.

We can't use the resource context from the raster thread either, and rasterizing the image on the IO thread has been attempted but does not work.

This patch adds a 1x1 pbuffer for use if the on screen surface is not available, and rasterizes using that. It only applies to Android since it is GL and background specific - on the iOS GL backend it would be illegal to use the GPU from the background anyway.

Fixes flutter/flutter#73675
Fixes flutter/flutter#86680

Need to figure out the right way to add this test case: flutter/flutter#73675 (comment)

[dnfield]

Something's wrong here - in the midst of refacotring/cleaning up I broke the test I have.

[dnfield]

Changes here are to run multiple firebase testlab tests in parallel. Now that we're running two this makes a difference. The new test executes pretty quickly but still requires setup/upload/results processing time.

[dnfield]

This patch is ready for review.

[dnfield]

Sorry for the size, a substantial number of files are testing support related files :\

[jason-simmons]

Can the redundancy of surface_ versus pbuffer_surface be reduced?

e.g. with something like

Surface* snapshot_surface = nullptr;
if (surface_ && surface_->GetContext()) {
  snapshot_surface = surface_.get();
} else {
  pbuffer_surface = delegate_.CreateSnapshotSurface();
  if (pbuffer_surface && pbuffer_surface->GetContext())
    snapshot_surface = pbuffer_surface.get();
}

The rest of the method can then only use snapshot_surface

[dnfield]

I'd ahve to manually delete the surface if it came from CreateSnapshotSurface then though. like pbuffer_surface.release() and then a delete at the end if it's not nullptr and not the surface_.get(). Is there some way I'm missing?

[jason-simmons]

pbuffer_surface would be a unique_ptr declared at the same scope as snapshot_surface. If pbuffer_surface is assigned then it will be deleted when the function exits.

[dnfield]

Ahh I get it now.

[dnfield]

Done

[jason-simmons]

Is there a way to avoid having a method on PlatformView which is a special case that is not called on the platform thread?

One potential option would be creating another interface similar to ExternalViewEmbedder that is instantiated by the PlatformView and then given to the Rasterizer by the Shell.

Like AndroidExternalViewEmbedder, this object could hold a reference to the AndroidContext and use that to create the pbuffer surface.

[dnfield]

I like this idea. I'll try to do this - I think it'll get rid of some of the funky threading concerns around accessing the platform view.

[dnfield]

Ok, latest commit has these changes. This gets rid of the threading concern/note that I had in platform view before. @chinmaygarde FYI

[dnfield]

Build failure is because the gradlew script is missing from the project dir. Currently it's gitignored. @blasten is there a better way to handle this than checking in gradlew?

[blasten]

If the intention is to build with Gradle, then there are two options, but really just one (see 2).

1. Run with the installed version of gradle gradle <task-name>. Unfortunately, this version must match the version required by the project, which makes a direct dependency on the environment, and doesn't scale if you later check in another project, and it requires a different version.

2. Check in gradlew. Generate the wrapper with gradle wrapper assuming you have the right version locally.

[dnfield]

@blasten - looks like gradlew is unhappy with buildtools?

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket.appspot.com/8841170477406727600/+/u/build_android_profile_x64__default_clang_x64_gen_snapshot_/build_android_profile_x64_default_clang_x64_gen_snapshot/stdout#L8363_8

[3880/3895] ACTION //flutter/testing/scenario_app/android:android_lint(//build/toolchain/android:clang_x64)
 FAILED: scenario_app/build/reports 
 python ../../flutter/testing/rules/run_gradle.py /b/s/w/ir/cache/builder/src/flutter/testing/scenario_app/android lint --no-daemon -Pflutter_jar=/b/s/w/ir/cache/builder/src/out/android_profile_x64/flutter.jar -Pout_dir=/b/s/w/ir/cache/builder/src/out/android_profile_x64/scenario_app --project-cache-dir=\"/b/s/w/ir/cache/builder/src/out/android_profile_x64/scenario_app/.gradle\" --gradle-user-home=\"/b/s/w/ir/cache/builder/src/out/android_profile_x64/scenario_app/.gradle\" -Plibapp=/b/s/w/ir/cache/builder/src/out/android_profile_x64/gen/flutter/testing/scenario_app/android/libs
 FAILURE: Build failed with an exception.
 * What went wrong:
 Could not determine the dependencies of task ':app:compileReleaseJavaWithJavac'.
 > Installed Build Tools revision 29.0.2 is corrupted. Remove and install again using the SDK Manager.

[dnfield]

Checking if it's a flake...

[dnfield]

AFAICT gradle is trying to download buildtools 30.0.2 and CI doesn't like it.

I don't have permissions to update the CIPD packages to add 30.0.2 and roll us to that. @blasten @godofredoc

[jason-simmons]

I think this should hold a reference to the AndroidSurface instead of converting AndroidSurface into a shared pointer.
(similar to the AndroidContext& held by the AndroidExternalViewEmbedder)

I don't think it's safe to delete the last AndroidSurface pointer on a thread other than the platform thread.

Normally this would not happen because the Shell would delete the Rasterizer before the PlatformView, which would ensure that the PlatformView's pointer to the AndroidSurface would be the last to go. But if the PlatformView must be responsible for deleting the AndroidSurface, then I'd prefer to formalize that by having the PlatformView hold a unique_ptr.

Alternatively, CreateSnapshotSurface could be moved out of AndroidSurface and into AndroidContext. The AndroidSurfaceGL::CreatePbufferSurface implementation is delegating to AndroidContextGL and does not use any of AndroidSurfaceGL's state.

[dnfield]

We do need an AndroidSurfaceGL to create a GPUSurfaceGL which is what the rasterizer expects to interact with. I'll ook at using the ref.

[dnfield]

Using a ref is less than ideal because it access a bunch of non-const methods.

I'm using a weak_ptr which I think should be safe w.r.t. your concerns.

[dnfield]

Ok - weak_ptr doesn't actually solve this problem, because we'd still potentially release the shared_ptr on the wrong thread. changed to a non-const reference.