Share Android surface GrDirectContext #23798

xster · 2021-01-20T17:12:34Z

Does half of flutter/flutter#73597.
The Android side of #23634.

Make the AndroidContext store (but not create) GrDirectContext.
Give all AndroidSurfaces' CreateGPUSurface the AndroidContext's stored GrDirectContext if there is one.
Move AndroidSurfaceGL's reference storage of the AndroidContext up to the super AndroidSurface as a shared_ptr (since it now modifies it).
Let AndroidSurfaceGL lazy create the first GrDirectContext and save it in the AndroidContext.
Let AndroidSurfaceVulkan->GpuSurfaceVulkan->VulkanWindow have a constructor injectable GrDirectContext too.
Let AndroidSurfaceVulkan also lazy create the first GrDirectContext by building the GpuSurfaceVulkan, then pull it out to save another copy in the AndroidContext.

Manual tested and via the new Scenario App spawn test so far.

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.
The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2021-01-20T17:12:37Z

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.

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

xster · 2021-01-20T17:16:40Z

Writing tests now. Please take a look to see if the ownership scheme makes sense.

xster

Reading guide: most of this PR is just plumbing. There are just 2 spots that actually do real work.

xster · 2021-01-20T21:32:26Z

shell/platform/android/android_surface_gl.cc

 }

 std::unique_ptr<Surface> AndroidSurfaceGL::CreateGPUSurface(
    GrDirectContext* gr_context) {
  if (gr_context) {
    return std::make_unique<GPUSurfaceGL>(sk_ref_sp(gr_context), this, true);
+  } else {
+    sk_sp<GrDirectContext> main_skia_context =


This is more or less 1 of 2 of the actual interesting parts of this PR. When AndroidSurfaceGL needs to make a GPUSurfaceGL (which would then make a GrDirectContext implicitly), the AndroidSurfaceGL makes one explicitly, then stores it in the AndroidContext for reuse if more AndroidSurfaceGLs are made with the same AndroidContext.

xster · 2021-01-20T21:33:27Z

shell/platform/android/platform_view_android.cc

@@ -317,7 +317,8 @@ std::unique_ptr<Surface> PlatformViewAndroid::CreateRenderingSurface() {
  if (!android_surface_) {
    return nullptr;
  }
-  return android_surface_->CreateGPUSurface();
+  return android_surface_->CreateGPUSurface(
+      android_context_->GetMainSkiaContext().get());


This is the 2/2 of the actual active parts of this PR. If more GPUSurfaces are made with other AndroidSurfaces, re-use the same GrDirectContext from the AndroidContext.

xster · 2021-01-21T08:49:36Z

Since no one looked yet, I added Vulkan support to this one too. Unfortunately there are a lot of impediments to testing. For OpenGL, the main challenge is that the AndroidSurfaceGL needs a AndroidEGLSurface. It also needs an AndroidContextGL, where half of the cc code is interactions with <EGL/eglext.h>. This needs to pull in the Android ndk which we wouldn't want to do into host_debug_unopt where the unittests will be.

Possible approaches:

Use if(is_android) to carve out large swaths of AndroidContextGL so #include <EGL/eglext.h> can be carved out too. Though this is a bit unrealistic and creates a class under test that's getting far from the real one (since it is a GL bridging specific class).
Copy in a EGL header into our own repo for things that we use for mocks, then import the real files for Android builds. Adds more maintenance cost.

Then for AndroidSurfaceVulkan, the challenge is only engine builds with --enable-vulkan is built with the needed vulkan functions compiled. Host is never built with --enable-vulkan currently.

Possible approaches:

Remove the --enable-vulkan restriction for Android, then let LUCI build host_debug_unopt_vulkan everywhere instead of host_debug_unopt. Run unit tests from that engine build.

Maybe you guys have some ideas. Otherwise, I'd say this becomes a Q1/Q2 task of doing the long term solution of breaking all these classes up by composition as much as possible and only let the injectable bottom of indirections import Android (a bit like the JNI in java) so we can test the rest. And break up the big flutter_shell_native target.

gaaclarke

lgtm!

gaaclarke · 2021-01-21T21:21:38Z

shell/gpu/gpu_surface_vulkan.cc

@@ -12,7 +12,20 @@ GPUSurfaceVulkan::GPUSurfaceVulkan(
    GPUSurfaceVulkanDelegate* delegate,
    std::unique_ptr<vulkan::VulkanNativeSurface> native_surface,
    bool render_to_surface)
-    : window_(delegate->vk(), std::move(native_surface), render_to_surface),
+    : GPUSurfaceVulkan(nullptr,


nit: parameter comment would be nice for nullptr

thanks. Done here and in vulkan_window

gaaclarke · 2021-01-22T17:30:53Z

Heads up, my work on opengl had to be reverted. I'm investigating now (flutter/flutter#74502)

You probably don't want to land this until I remedy this problem.

xster · 2021-01-22T17:48:52Z

I'm actually guessing this PR fixes that issue :D. Since your PR changed GPUSurfaceGL for both platforms but might not have accounted for how Android uses it and this PR accounts for it. Let me run that test on this commit.

gaaclarke · 2021-01-22T17:59:43Z

shell/gpu/gpu_surface_vulkan.cc

@@ -12,7 +12,7 @@ GPUSurfaceVulkan::GPUSurfaceVulkan(
    GPUSurfaceVulkanDelegate* delegate,
    std::unique_ptr<vulkan::VulkanNativeSurface> native_surface,
    bool render_to_surface)
-    : GPUSurfaceVulkan(nullptr,
+    : GPUSurfaceVulkan(nullptr /* GrDirectContext */,


The proper way to comment these is with /*foobar=*/, where foobar is the name of the parameter according to the declaration.

https://google.github.io/styleguide/cppguide.html#Function_Argument_Comments

xster · 2021-01-22T19:11:52Z

Unfortunately my last testing path didn't quite go the way I wanted. I was thinking of just actually using the NDK and build 2 targets for Android. A normal jar for feeding into gradle and a separate executable for the unit test with the same deps upstream as the cpp files used to build the libflutter.so. It works and builds, except it's built with the NDK compiler for elf 64 rather than mach-o 64 to be able to run locally. A further slice might be pretty complicated to use mac's clang but import standard libraries from Android's NDK.

I think the proper modular, long term solution is to continue slicing and mocking the c++ classes like Emmanuel started in flutter/flutter#59031 for the rest of the dependencies.

xster · 2021-01-22T20:19:28Z

Spot checked a number of Android devicelab tests

* 004d8ad Roll Skia from cc6961b9ac5e to 6a272434c2b2 (3 revisions) (flutter/engine#23864) * 7c2da3b reland of flutter/engine#23634 (flutter/engine#23865) * a4f02b7 Share Android surface GrDirectContext (flutter/engine#23798) * 31ef1dd Roll Skia from 6a272434c2b2 to bfc9be0f773f (2 revisions) (flutter/engine#23867) * 6dec57f Remove workarounds now that type promotion accounts for local boolean variables. (flutter/engine#23862) * a787229 Roll Skia from bfc9be0f773f to 3193ff271628 (5 revisions) (flutter/engine#23872) * a242dd8 [web] Fix shadows for arbitrary paths on PhysicalShape (flutter/engine#23830) * 05b4bec Started using Dart_CreateInGroup when using spawn on a release build (flutter/engine#23782)

flutter-dashboard bot added platform-android platform-ios labels Jan 20, 2021

googlebot added the cla: yes label Jan 20, 2021

xster requested review from gaaclarke and jason-simmons January 20, 2021 17:16

xster force-pushed the android-surface-context branch from b4e821b to 7d79434 Compare January 20, 2021 17:36

xster commented Jan 20, 2021

View reviewed changes

xster changed the title ~~Android surface context~~ Share Android surface GrDirectContext Jan 21, 2021

jason-simmons approved these changes Jan 21, 2021

View reviewed changes

xster added 5 commits January 21, 2021 10:05

make AndroidSurfaceGL share GrDirectContext within an AndroidContext

8808370

fix existing unit tests

96ab5e0

fix more tests

32f1a18

hook up vulkan

f5cafe9

fix bug

60222a8

xster force-pushed the android-surface-context branch from 74a66c2 to 60222a8 Compare January 21, 2021 18:05

gaaclarke approved these changes Jan 21, 2021

View reviewed changes

review

bde08f9

gaaclarke reviewed Jan 22, 2021

View reviewed changes

xster added 2 commits January 22, 2021 11:13

nit

161f944

autoformat

274acae

xster 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 22, 2021

fluttergithubbot merged commit a4f02b7 into flutter:master Jan 22, 2021

engine-flutter-autoroll mentioned this pull request Jan 23, 2021

Roll Engine from fdddf8708039 to 05b4bec8b3c1 (8 revisions) flutter/flutter#74540

Merged

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

a4f02b7 Share Android surface GrDirectContext (flutter/engine#23798)

869f6b0

hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021

Share Android surface GrDirectContext (flutter#23798)

95144b9

jason-simmons mentioned this pull request Aug 17, 2021

libEGL : call to OpenGL ES API with no current context (logged once per thread) flutter/flutter#87937

Closed

This was referenced Sep 8, 2021

[WIP]Fix OpenGL leak on Android and iOS #28506

Closed

OpenGL leak on Android and iOS flutter/flutter#89658

Closed

gaaclarke mentioned this pull request Sep 20, 2021

Share the GrDirectContext across engine instances on Android flutter/flutter#90398

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Share Android surface GrDirectContext #23798

Share Android surface GrDirectContext #23798

Uh oh!

xster commented Jan 20, 2021 •

edited

Loading

Uh oh!

flutter-dashboard bot commented Jan 20, 2021

Uh oh!

xster commented Jan 20, 2021

Uh oh!

xster left a comment

Uh oh!

xster Jan 20, 2021

Uh oh!

xster Jan 20, 2021

Uh oh!

xster commented Jan 21, 2021

Uh oh!

gaaclarke left a comment

Uh oh!

gaaclarke Jan 21, 2021

Uh oh!

xster Jan 22, 2021

Uh oh!

gaaclarke commented Jan 22, 2021

Uh oh!

xster commented Jan 22, 2021

Uh oh!

gaaclarke Jan 22, 2021

Uh oh!

xster commented Jan 22, 2021

Uh oh!

xster commented Jan 22, 2021

Uh oh!

Uh oh!

Share Android surface GrDirectContext #23798

Share Android surface GrDirectContext #23798

Uh oh!

Conversation

xster commented Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Jan 20, 2021

Uh oh!

xster commented Jan 20, 2021

Uh oh!

xster left a comment

Choose a reason for hiding this comment

Uh oh!

xster Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

xster Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

xster commented Jan 21, 2021

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

xster Jan 22, 2021

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Jan 22, 2021

Uh oh!

xster commented Jan 22, 2021

Uh oh!

gaaclarke Jan 22, 2021

Choose a reason for hiding this comment

Uh oh!

xster commented Jan 22, 2021

Uh oh!

xster commented Jan 22, 2021

Uh oh!

Uh oh!

xster commented Jan 20, 2021 •

edited

Loading