From 0853497b4c6105d7066554bd898df98faf77e4fb Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Fri, 5 Jun 2020 10:55:12 -0700 Subject: [PATCH 1/3] Add RAII wrapper for EGLSurface --- shell/platform/android/android_context_gl.cc | 51 +++++++++++++------- shell/platform/android/android_context_gl.h | 28 ++++++++--- shell/platform/android/android_surface_gl.cc | 22 ++++----- shell/platform/android/android_surface_gl.h | 4 +- 4 files changed, 67 insertions(+), 38 deletions(-) diff --git a/shell/platform/android/android_context_gl.cc b/shell/platform/android/android_context_gl.cc index c1ac748e07729..8042d0a3593a4 100644 --- a/shell/platform/android/android_context_gl.cc +++ b/shell/platform/android/android_context_gl.cc @@ -105,6 +105,10 @@ static bool TeardownContext(EGLDisplay display, EGLContext context) { return true; } +AndroidEGLSurface::AndroidEGLSurface(EGLSurface surface) : surface(surface) {} + +AndroidEGLSurface::~AndroidEGLSurface() = default; + AndroidContextGL::AndroidContextGL( AndroidRenderingAPI rendering_api, fml::RefPtr environment) @@ -161,25 +165,28 @@ AndroidContextGL::~AndroidContextGL() { } } -EGLSurface AndroidContextGL::CreateOnscreenSurface( +std::unique_ptr AndroidContextGL::CreateOnscreenSurface( fml::RefPtr window) const { EGLDisplay display = environment_->Display(); const EGLint attribs[] = {EGL_NONE}; - return eglCreateWindowSurface( + EGLSurface surface = eglCreateWindowSurface( display, config_, reinterpret_cast(window->handle()), attribs); + return std::make_unique(surface); } -EGLSurface AndroidContextGL::CreateOffscreenSurface() const { +std::unique_ptr AndroidContextGL::CreateOffscreenSurface() + const { // We only ever create pbuffer surfaces for background resource loading // contexts. We never bind the pbuffer to anything. EGLDisplay display = environment_->Display(); const EGLint attribs[] = {EGL_WIDTH, 1, EGL_HEIGHT, 1, EGL_NONE}; - return eglCreatePbufferSurface(display, config_, attribs); + EGLSurface surface = eglCreatePbufferSurface(display, config_, attribs); + return std::make_unique(surface); } fml::RefPtr AndroidContextGL::Environment() const { @@ -190,9 +197,10 @@ bool AndroidContextGL::IsValid() const { return valid_; } -bool AndroidContextGL::MakeCurrent(EGLSurface& surface) { - if (eglMakeCurrent(environment_->Display(), surface, surface, context_) != - EGL_TRUE) { +bool AndroidContextGL::MakeCurrent( + std::unique_ptr surface_wrapper) { + if (eglMakeCurrent(environment_->Display(), surface_wrapper->surface, + surface_wrapper->surface, context_) != EGL_TRUE) { FML_LOG(ERROR) << "Could not make the context current"; LogLastEGLError(); return false; @@ -200,9 +208,10 @@ bool AndroidContextGL::MakeCurrent(EGLSurface& surface) { return true; } -bool AndroidContextGL::ResourceMakeCurrent(EGLSurface& surface) { - if (eglMakeCurrent(environment_->Display(), surface, surface, - resource_context_) != EGL_TRUE) { +bool AndroidContextGL::ResourceMakeCurrent( + std::unique_ptr surface_wrapper) { + if (eglMakeCurrent(environment_->Display(), surface_wrapper->surface, + surface_wrapper->surface, resource_context_) != EGL_TRUE) { FML_LOG(ERROR) << "Could not make the context current"; LogLastEGLError(); return false; @@ -223,17 +232,21 @@ bool AndroidContextGL::ClearCurrent() { return true; } -bool AndroidContextGL::SwapBuffers(EGLSurface& surface) { +bool AndroidContextGL::SwapBuffers( + std::unique_ptr surface_wrapper) { TRACE_EVENT0("flutter", "AndroidContextGL::SwapBuffers"); - return eglSwapBuffers(environment_->Display(), surface); + return eglSwapBuffers(environment_->Display(), surface_wrapper->surface); } -SkISize AndroidContextGL::GetSize(EGLSurface& surface) { +SkISize AndroidContextGL::GetSize( + std::unique_ptr surface_wrapper) { EGLint width = 0; EGLint height = 0; - if (!eglQuerySurface(environment_->Display(), surface, EGL_WIDTH, &width) || - !eglQuerySurface(environment_->Display(), surface, EGL_HEIGHT, &height)) { + if (!eglQuerySurface(environment_->Display(), surface_wrapper->surface, + EGL_WIDTH, &width) || + !eglQuerySurface(environment_->Display(), surface_wrapper->surface, + EGL_HEIGHT, &height)) { FML_LOG(ERROR) << "Unable to query EGL surface size"; LogLastEGLError(); return SkISize::Make(0, 0); @@ -241,9 +254,11 @@ SkISize AndroidContextGL::GetSize(EGLSurface& surface) { return SkISize::Make(width, height); } -bool AndroidContextGL::TeardownSurface(EGLSurface& surface) { - if (surface != EGL_NO_SURFACE) { - return eglDestroySurface(environment_->Display(), surface) == EGL_TRUE; +bool AndroidContextGL::TeardownSurface( + std::unique_ptr surface_wrapper) { + if (surface_wrapper->surface != EGL_NO_SURFACE) { + return eglDestroySurface(environment_->Display(), + surface_wrapper->surface) == EGL_TRUE; } return true; diff --git a/shell/platform/android/android_context_gl.h b/shell/platform/android/android_context_gl.h index 606ffe829444b..ad2f68093841f 100644 --- a/shell/platform/android/android_context_gl.h +++ b/shell/platform/android/android_context_gl.h @@ -16,6 +16,20 @@ namespace flutter { +//------------------------------------------------------------------------------ +/// Holds a `EGLSurface` reference. +/// +/// +/// This can be used in conjuction to unique_ptr to provide better guarantees +/// about the lifespam of the `EGLSurface` object. +/// +struct AndroidEGLSurface { + AndroidEGLSurface(EGLSurface surface); + ~AndroidEGLSurface(); + + const EGLSurface surface; +}; + //------------------------------------------------------------------------------ /// The Android context is used by `AndroidSurfaceGL` to create and manage /// EGL surfaces. @@ -39,7 +53,7 @@ class AndroidContextGL : public AndroidContext { /// /// @return The window surface. /// - EGLSurface CreateOnscreenSurface( + std::unique_ptr CreateOnscreenSurface( fml::RefPtr window) const; //---------------------------------------------------------------------------- @@ -51,7 +65,7 @@ class AndroidContextGL : public AndroidContext { /// /// @return The pbuffer surface. /// - EGLSurface CreateOffscreenSurface() const; + std::unique_ptr CreateOffscreenSurface() const; //---------------------------------------------------------------------------- /// @return The Android environment that contains a reference to the @@ -77,7 +91,7 @@ class AndroidContextGL : public AndroidContext { /// /// @return Whether the surface was made current. /// - bool MakeCurrent(EGLSurface& surface); + bool MakeCurrent(std::unique_ptr surface_wrapper); //---------------------------------------------------------------------------- /// @brief Binds the resource EGLContext context to the current rendering @@ -85,7 +99,7 @@ class AndroidContextGL : public AndroidContext { /// /// @return Whether the surface was made current. /// - bool ResourceMakeCurrent(EGLSurface& surface); + bool ResourceMakeCurrent(std::unique_ptr surface_wrapper); //---------------------------------------------------------------------------- /// @brief This only applies to on-screen surfaces such as those created @@ -93,19 +107,19 @@ class AndroidContextGL : public AndroidContext { /// /// @return Whether the EGL surface color buffer was swapped. /// - bool SwapBuffers(EGLSurface& surface); + bool SwapBuffers(std::unique_ptr surface_wrapper); //---------------------------------------------------------------------------- /// @return The size of an `EGLSurface`. /// - SkISize GetSize(EGLSurface& surface); + SkISize GetSize(std::unique_ptr surface_wrapper); //---------------------------------------------------------------------------- /// @brief Destroys an `EGLSurface`. /// /// @return Whether the surface was destroyed. /// - bool TeardownSurface(EGLSurface& surface); + bool TeardownSurface(std::unique_ptr surface_wrapper); private: fml::RefPtr environment_; diff --git a/shell/platform/android/android_surface_gl.cc b/shell/platform/android/android_surface_gl.cc index 59e5eacffac2b..51d3d0a0c62f8 100644 --- a/shell/platform/android/android_surface_gl.cc +++ b/shell/platform/android/android_surface_gl.cc @@ -20,7 +20,7 @@ AndroidSurfaceGL::AndroidSurfaceGL( std::static_pointer_cast(android_context); // Acquire the offscreen surface. offscreen_surface_ = android_context_->CreateOffscreenSurface(); - if (offscreen_surface_ == EGL_NO_SURFACE) { + if (offscreen_surface_->surface == EGL_NO_SURFACE) { offscreen_surface_ = nullptr; } external_view_embedder_ = std::make_unique(); @@ -28,11 +28,11 @@ AndroidSurfaceGL::AndroidSurfaceGL( AndroidSurfaceGL::~AndroidSurfaceGL() { if (offscreen_surface_) { - android_context_->TeardownSurface(offscreen_surface_); + android_context_->TeardownSurface(std::move(offscreen_surface_)); offscreen_surface_ = nullptr; } if (onscreen_surface_) { - android_context_->TeardownSurface(onscreen_surface_); + android_context_->TeardownSurface(std::move(onscreen_surface_)); onscreen_surface_ = nullptr; } } @@ -58,25 +58,25 @@ bool AndroidSurfaceGL::OnScreenSurfaceResize(const SkISize& size) { FML_DCHECK(onscreen_surface_); FML_DCHECK(native_window_); - if (size == android_context_->GetSize(onscreen_surface_)) { + if (size == android_context_->GetSize(std::move(onscreen_surface_))) { return true; } android_context_->ClearCurrent(); - android_context_->TeardownSurface(onscreen_surface_); + android_context_->TeardownSurface(std::move(onscreen_surface_)); onscreen_surface_ = android_context_->CreateOnscreenSurface(native_window_); - if (!onscreen_surface_ || onscreen_surface_ == EGL_NO_SURFACE) { + if (onscreen_surface_->surface == EGL_NO_SURFACE) { FML_LOG(ERROR) << "Unable to create EGL window surface on resize."; return false; } - android_context_->MakeCurrent(onscreen_surface_); + android_context_->MakeCurrent(std::move(onscreen_surface_)); return true; } bool AndroidSurfaceGL::ResourceContextMakeCurrent() { FML_DCHECK(IsValid()); - return android_context_->ResourceMakeCurrent(offscreen_surface_); + return android_context_->ResourceMakeCurrent(std::move(offscreen_surface_)); } bool AndroidSurfaceGL::ResourceContextClearCurrent() { @@ -91,7 +91,7 @@ bool AndroidSurfaceGL::SetNativeWindow( native_window_ = window; // Create the onscreen surface. onscreen_surface_ = android_context_->CreateOnscreenSurface(window); - if (onscreen_surface_ == EGL_NO_SURFACE) { + if (onscreen_surface_->surface == EGL_NO_SURFACE) { return false; } return true; @@ -101,7 +101,7 @@ std::unique_ptr AndroidSurfaceGL::GLContextMakeCurrent() { FML_DCHECK(IsValid()); FML_DCHECK(onscreen_surface_); auto default_context_result = std::make_unique( - android_context_->MakeCurrent(onscreen_surface_)); + android_context_->MakeCurrent(std::move(onscreen_surface_))); return std::move(default_context_result); } @@ -113,7 +113,7 @@ bool AndroidSurfaceGL::GLContextClearCurrent() { bool AndroidSurfaceGL::GLContextPresent() { FML_DCHECK(IsValid()); FML_DCHECK(onscreen_surface_); - return android_context_->SwapBuffers(onscreen_surface_); + return android_context_->SwapBuffers(std::move(onscreen_surface_)); } intptr_t AndroidSurfaceGL::GLContextFBO() const { diff --git a/shell/platform/android/android_surface_gl.h b/shell/platform/android/android_surface_gl.h index 58188630fb178..1db19d115c489 100644 --- a/shell/platform/android/android_surface_gl.h +++ b/shell/platform/android/android_surface_gl.h @@ -64,8 +64,8 @@ class AndroidSurfaceGL final : public GPUSurfaceGLDelegate, fml::RefPtr native_window_; std::unique_ptr external_view_embedder_; std::shared_ptr android_context_; - EGLSurface onscreen_surface_; - EGLSurface offscreen_surface_; + std::unique_ptr onscreen_surface_; + std::unique_ptr offscreen_surface_; FML_DISALLOW_COPY_AND_ASSIGN(AndroidSurfaceGL); }; From 289bef3944f8b82535ebe33ffed8faadf75753d5 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Fri, 5 Jun 2020 10:58:36 -0700 Subject: [PATCH 2/3] grammar --- shell/platform/android/android_context_gl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shell/platform/android/android_context_gl.h b/shell/platform/android/android_context_gl.h index ad2f68093841f..f5e89501e84bd 100644 --- a/shell/platform/android/android_context_gl.h +++ b/shell/platform/android/android_context_gl.h @@ -17,7 +17,7 @@ namespace flutter { //------------------------------------------------------------------------------ -/// Holds a `EGLSurface` reference. +/// Holds an `EGLSurface` reference. /// /// /// This can be used in conjuction to unique_ptr to provide better guarantees From a2d846ff9568d5a10a9b4f4964dffca3814d6e45 Mon Sep 17 00:00:00 2001 From: Emmanuel Garcia Date: Tue, 9 Jun 2020 17:05:41 -0700 Subject: [PATCH 3/3] Destroy surface in destructor --- shell/platform/android/android_context_gl.cc | 22 +++++++------------- shell/platform/android/android_context_gl.h | 18 ++++------------ shell/platform/android/android_surface_gl.cc | 12 +---------- 3 files changed, 13 insertions(+), 39 deletions(-) diff --git a/shell/platform/android/android_context_gl.cc b/shell/platform/android/android_context_gl.cc index 8042d0a3593a4..740c3e328ebdb 100644 --- a/shell/platform/android/android_context_gl.cc +++ b/shell/platform/android/android_context_gl.cc @@ -105,9 +105,13 @@ static bool TeardownContext(EGLDisplay display, EGLContext context) { return true; } -AndroidEGLSurface::AndroidEGLSurface(EGLSurface surface) : surface(surface) {} +AndroidEGLSurface::AndroidEGLSurface(EGLSurface surface, EGLDisplay display) + : surface(surface), display_(display) {} -AndroidEGLSurface::~AndroidEGLSurface() = default; +AndroidEGLSurface::~AndroidEGLSurface() { + auto result = eglDestroySurface(display_, surface); + FML_DCHECK(result == EGL_TRUE); +} AndroidContextGL::AndroidContextGL( AndroidRenderingAPI rendering_api, @@ -174,7 +178,7 @@ std::unique_ptr AndroidContextGL::CreateOnscreenSurface( EGLSurface surface = eglCreateWindowSurface( display, config_, reinterpret_cast(window->handle()), attribs); - return std::make_unique(surface); + return std::make_unique(surface, display); } std::unique_ptr AndroidContextGL::CreateOffscreenSurface() @@ -186,7 +190,7 @@ std::unique_ptr AndroidContextGL::CreateOffscreenSurface() const EGLint attribs[] = {EGL_WIDTH, 1, EGL_HEIGHT, 1, EGL_NONE}; EGLSurface surface = eglCreatePbufferSurface(display, config_, attribs); - return std::make_unique(surface); + return std::make_unique(surface, display); } fml::RefPtr AndroidContextGL::Environment() const { @@ -254,14 +258,4 @@ SkISize AndroidContextGL::GetSize( return SkISize::Make(width, height); } -bool AndroidContextGL::TeardownSurface( - std::unique_ptr surface_wrapper) { - if (surface_wrapper->surface != EGL_NO_SURFACE) { - return eglDestroySurface(environment_->Display(), - surface_wrapper->surface) == EGL_TRUE; - } - - return true; -} - } // namespace flutter diff --git a/shell/platform/android/android_context_gl.h b/shell/platform/android/android_context_gl.h index f5e89501e84bd..450a792163177 100644 --- a/shell/platform/android/android_context_gl.h +++ b/shell/platform/android/android_context_gl.h @@ -24,10 +24,13 @@ namespace flutter { /// about the lifespam of the `EGLSurface` object. /// struct AndroidEGLSurface { - AndroidEGLSurface(EGLSurface surface); + AndroidEGLSurface(EGLSurface surface, EGLDisplay display); ~AndroidEGLSurface(); const EGLSurface surface; + + private: + const EGLDisplay display_; }; //------------------------------------------------------------------------------ @@ -48,9 +51,6 @@ class AndroidContextGL : public AndroidContext { /// @brief Allocates an new EGL window surface that is used for on-screen /// pixels. /// - /// @attention Consumers must tear down the surface by calling - /// `AndroidContextGL::TeardownSurface`. - /// /// @return The window surface. /// std::unique_ptr CreateOnscreenSurface( @@ -60,9 +60,6 @@ class AndroidContextGL : public AndroidContext { /// @brief Allocates an 1x1 pbuffer surface that is used for making the /// offscreen current for texture uploads. /// - /// @attention Consumers must tear down the surface by calling - /// `AndroidContextGL::TeardownSurface`. - /// /// @return The pbuffer surface. /// std::unique_ptr CreateOffscreenSurface() const; @@ -114,13 +111,6 @@ class AndroidContextGL : public AndroidContext { /// SkISize GetSize(std::unique_ptr surface_wrapper); - //---------------------------------------------------------------------------- - /// @brief Destroys an `EGLSurface`. - /// - /// @return Whether the surface was destroyed. - /// - bool TeardownSurface(std::unique_ptr surface_wrapper); - private: fml::RefPtr environment_; EGLConfig config_; diff --git a/shell/platform/android/android_surface_gl.cc b/shell/platform/android/android_surface_gl.cc index 51d3d0a0c62f8..449b7e411c19b 100644 --- a/shell/platform/android/android_surface_gl.cc +++ b/shell/platform/android/android_surface_gl.cc @@ -26,16 +26,7 @@ AndroidSurfaceGL::AndroidSurfaceGL( external_view_embedder_ = std::make_unique(); } -AndroidSurfaceGL::~AndroidSurfaceGL() { - if (offscreen_surface_) { - android_context_->TeardownSurface(std::move(offscreen_surface_)); - offscreen_surface_ = nullptr; - } - if (onscreen_surface_) { - android_context_->TeardownSurface(std::move(onscreen_surface_)); - onscreen_surface_ = nullptr; - } -} +AndroidSurfaceGL::~AndroidSurfaceGL() = default; void AndroidSurfaceGL::TeardownOnScreenContext() { android_context_->ClearCurrent(); @@ -63,7 +54,6 @@ bool AndroidSurfaceGL::OnScreenSurfaceResize(const SkISize& size) { } android_context_->ClearCurrent(); - android_context_->TeardownSurface(std::move(onscreen_surface_)); onscreen_surface_ = android_context_->CreateOnscreenSurface(native_window_); if (onscreen_surface_->surface == EGL_NO_SURFACE) {