Refactor AndroidContextGL, AndroidSurface and AndroidSurfaceGL by blasten · Pull Request #18670 · flutter/engine

blasten · 2020-05-29T20:53:33Z

This will make the relation between AndroidContext and AndroidSurface closer to IOSContext and IOSSurface.

With this change, every GL surface will own an onscreen and offscreen EGLSurface as opposed to the Android context owning the surfaces.

Fixes flutter/flutter#58269

blasten · 2020-05-29T20:57:45Z

shell/platform/android/android_context_gl.h

 namespace flutter {

-class AndroidContextGL : public fml::RefCountedThreadSafe<AndroidContextGL> {
+class AndroidContextGL : public AndroidContext {


This isn't a fml::RefCountedThreadSafe anymore. AndroidContextGL will be a shared pointer.

Sound good.

chinmaygarde

Sound direction and thanks for the separation of the surface and the context. Just a few nits and the one comment about making EGLSurface an RAII object.

chinmaygarde · 2020-06-01T19:11:31Z

shell/platform/android/android_context.cc

+}
+
+bool AndroidContext::IsValid() {
+  return true;


What is the point of this method if it is always true? It seems you are not returning a non-valid context from the factory. If you don't plan on adding to this, I'd get rid of this.

Done. This has been removed.

chinmaygarde · 2020-06-01T19:14:31Z

shell/platform/android/android_context.cc

+      break;
+  }
+  FML_CHECK(context);
+  FML_CHECK(context->IsValid()) << "Could not create an Android context.";


IsValid is not virtual and always returns true. So this check is redundant. I suggest making IsValid a const pure virtual so that AndroidContext itself is abstract. Then in the IsValid override for each, you can return if the context is actually valid.

chinmaygarde · 2020-06-01T19:16:14Z

shell/platform/android/android_context.cc

+      context = std::make_shared<AndroidContextGL>(
+          rendering_api, fml::MakeRefCounted<AndroidEnvironmentGL>());
+      break;
+    case AndroidRenderingAPI::kVulkan:


This breaking Vulkan builds right? (its been quite a long time since I have checked those builds BTW, if maintaining those is getting in the way, we should get rid of them).

It shouldn't be a break the build AFAICT.

chinmaygarde · 2020-06-01T19:16:49Z

shell/platform/android/android_context.h

+  bool IsValid();
+
+ private:
+  AndroidRenderingAPI rendering_api_;


Make this const so its impossible to forget to initialize this ivar.

chinmaygarde · 2020-06-01T19:17:22Z

shell/platform/android/android_context_gl.cc

-                                   const AndroidContextGL* share_context)
-    : environment_(env),
-      window_(nullptr),
+AndroidContextGL::AndroidContextGL(


Much cleaner. Thanks!

chinmaygarde · 2020-06-01T19:23:42Z

shell/platform/android/android_context_gl.cc

-  if (!eglQuerySurface(environment_->Display(), surface_, EGL_WIDTH, &width) ||
-      !eglQuerySurface(environment_->Display(), surface_, EGL_HEIGHT,
-                       &height)) {
+  if (!eglQuerySurface(environment_->Display(), surface, EGL_WIDTH, &width) ||


If you are going to make EGLSurface an RAII object, it may make sense to put these behind accessors. Just a thought.

I tried to use fml/unique_object.h, but I realized it was a bit tricky for one reason:
To destroy an EGLSurface, an instance of EGLDisplay is required. This is currently held by AndroidEnvironmentGL. While we currently just use EGL_DEFAULT_DISPLAY, I shouldn't assume that in a Free(EGLSurface* surface) method.

Discussed this via chat.

chinmaygarde · 2020-06-01T19:23:51Z

shell/platform/android/android_context_gl.h

 namespace flutter {

-class AndroidContextGL : public fml::RefCountedThreadSafe<AndroidContextGL> {
+class AndroidContextGL : public AndroidContext {


Sound good.

chinmaygarde · 2020-06-01T19:24:10Z

shell/platform/android/android_context_gl.h

-  bool CreatePBufferSurface();
+  ~AndroidContextGL();
+
+  EGLSurface CreateOnscreenSurface(fml::RefPtr<AndroidNativeWindow> window);


These should be const IMO.

shell/platform/android/android_context_gl.h

blasten · 2020-06-04T02:37:22Z

PTAL

chinmaygarde · 2020-06-05T01:08:11Z

shell/platform/android/android_context_gl.cc

-  if (!eglQuerySurface(environment_->Display(), surface_, EGL_WIDTH, &width) ||
-      !eglQuerySurface(environment_->Display(), surface_, EGL_HEIGHT,
-                       &height)) {
+  if (!eglQuerySurface(environment_->Display(), surface, EGL_WIDTH, &width) ||


Discussed this via chat.

flutter/engine#18670)

Emmanuel Garcia added 4 commits May 29, 2020 10:35

Refactor AndroidContextGL and AndroidSurface

e8f5d87

Add new files to license file

3a05a1a

clean up

b06b098

Capital letter

464c9dc

googlebot added the cla: yes label May 29, 2020

blasten commented May 29, 2020

View reviewed changes

blasten marked this pull request as ready for review May 29, 2020 21:02

auto-assign bot requested a review from jason-simmons May 29, 2020 21:03

blasten requested a review from chinmaygarde June 1, 2020 18:03

chinmaygarde reviewed Jun 1, 2020

View reviewed changes

Emmanuel Garcia added 3 commits June 2, 2020 17:20

Feedback

000f410

Merge remote-tracking branch 'upstream/master' into android_context

9216f49

Const methods

e3d99d0

blasten requested a review from chinmaygarde June 3, 2020 00:34

chinmaygarde approved these changes Jun 5, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into android_context

ba0923c

blasten 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 Jun 5, 2020

fluttergithubbot merged commit fafccf8 into flutter:master Jun 5, 2020

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 5, 2020

fafccf8 Refactor AndroidContextGL, AndroidSurface and AndroidSurfaceGL (

7878df1

flutter/engine#18670)

engine-flutter-autoroll mentioned this pull request Jun 5, 2020

Roll Engine from e87a05fbba78 to fafccf8a3a6c (1 revision) flutter/flutter#58809

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 5, 2020

fafccf8 Refactor AndroidContextGL, AndroidSurface and AndroidSurfaceGL (

857e5c8

flutter/engine#18670)

This was referenced Jun 5, 2020

Roll Engine from e87a05fbba78 to 5356def7d3fd (2 revisions) flutter/flutter#58818

Closed

Roll Engine from e87a05fbba78 to dbb57f1874fd (6 revisions) flutter/flutter#58826

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 5, 2020

fafccf8 Refactor AndroidContextGL, AndroidSurface and AndroidSurfaceGL (

3b7adae

flutter/engine#18670)

blasten deleted the android_context branch June 10, 2020 18:35

zanderso mentioned this pull request Aug 12, 2020

Engine crashes due to 'Could not make the context current, EGL Error: EGL_BAD_ACCESS (12290)' flutter/flutter#61158

Closed

matanlurey mentioned this pull request Nov 2, 2023

[Proposal] Refactor AndroidRenderingAPI to encapsulate Impeller/Skia config flutter/flutter#137798

Closed

Conversation

blasten commented May 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde 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

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!

Uh oh!

Uh oh!

blasten commented Jun 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants