Refactor AndroidContextGL, AndroidSurface and AndroidSurfaceGL #18670
Conversation
| namespace flutter { | ||
|
|
||
| class AndroidContextGL : public fml::RefCountedThreadSafe<AndroidContextGL> { | ||
| class AndroidContextGL : public AndroidContext { |
There was a problem hiding this comment.
This isn't a fml::RefCountedThreadSafe anymore. AndroidContextGL will be a shared pointer.
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| bool AndroidContext::IsValid() { | ||
| return true; |
There was a problem hiding this comment.
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.
| break; | ||
| } | ||
| FML_CHECK(context); | ||
| FML_CHECK(context->IsValid()) << "Could not create an Android context."; |
There was a problem hiding this comment.
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.
| context = std::make_shared<AndroidContextGL>( | ||
| rendering_api, fml::MakeRefCounted<AndroidEnvironmentGL>()); | ||
| break; | ||
| case AndroidRenderingAPI::kVulkan: |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
It shouldn't be a break the build AFAICT.
| bool IsValid(); | ||
|
|
||
| private: | ||
| AndroidRenderingAPI rendering_api_; |
There was a problem hiding this comment.
Make this const so its impossible to forget to initialize this ivar.
| const AndroidContextGL* share_context) | ||
| : environment_(env), | ||
| window_(nullptr), | ||
| AndroidContextGL::AndroidContextGL( |
| if (!eglQuerySurface(environment_->Display(), surface_, EGL_WIDTH, &width) || | ||
| !eglQuerySurface(environment_->Display(), surface_, EGL_HEIGHT, | ||
| &height)) { | ||
| if (!eglQuerySurface(environment_->Display(), surface, EGL_WIDTH, &width) || |
There was a problem hiding this comment.
If you are going to make EGLSurface an RAII object, it may make sense to put these behind accessors. Just a thought.
There was a problem hiding this comment.
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.
| namespace flutter { | ||
|
|
||
| class AndroidContextGL : public fml::RefCountedThreadSafe<AndroidContextGL> { | ||
| class AndroidContextGL : public AndroidContext { |
| bool CreatePBufferSurface(); | ||
| ~AndroidContextGL(); | ||
|
|
||
| EGLSurface CreateOnscreenSurface(fml::RefPtr<AndroidNativeWindow> window); |
|
PTAL |
| if (!eglQuerySurface(environment_->Display(), surface_, EGL_WIDTH, &width) || | ||
| !eglQuerySurface(environment_->Display(), surface_, EGL_HEIGHT, | ||
| &height)) { | ||
| if (!eglQuerySurface(environment_->Display(), surface, EGL_WIDTH, &width) || |
blasten
added
the
waiting for tree to go green
4 participants
This will make the relation between
AndroidContextandAndroidSurfacecloser toIOSContextandIOSSurface.With this change, every GL surface will own an onscreen and offscreen
EGLSurfaceas opposed to the Android context owning the surfaces.Fixes flutter/flutter#58269