Fix a crash in ExternalTextureGL (#81)

* Fix a crash in ExternalTextureGL * Abandon ownership of tbm surface in PopulateTextureWithIdentifier * This patch includes temporary fix for invalid tmb_surface Signed-off-by: Boram Bae <[email protected]> * Include the texture id in the log Signed-off-by: Boram Bae <[email protected]> * Lock the mutex in the external texture related API * Use find instead of operator[] that can cause unintended insertion Signed-off-by: Boram Bae <[email protected]>
flutter-tizen · May 7, 2021 · ad1fd5b · ad1fd5b
1 parent 9dfe48b
commit ad1fd5b
Show file tree

Hide file tree

Showing 5 changed files with 79 additions and 65 deletions.
diff --git a/shell/platform/tizen/external_texture_gl.cc b/shell/platform/tizen/external_texture_gl.cc
@@ -30,69 +30,90 @@ struct ExternalTextureGLState {
 
 static std::atomic_long nextTextureId = {1};
 
+static void MarkTbmSurfaceToUse(void* surface) {
+#ifndef WEARABLE_PROFILE
+  FT_ASSERT(surface);
+  tbm_surface_h tbm_surface = (tbm_surface_h)surface;
+  tbm_surface_internal_ref(tbm_surface);
+#endif
+}
+
+static void UnmarkTbmSurfaceToUse(void* surface) {
+  FT_ASSERT(surface);
+  tbm_surface_h tbm_surface = (tbm_surface_h)surface;
+#ifndef WEARABLE_PROFILE
+  tbm_surface_internal_unref(tbm_surface);
+#else
+  tbm_surface_destroy(tbm_surface);
+#endif
+}
+
 ExternalTextureGL::ExternalTextureGL()
     : state_(std::make_unique<ExternalTextureGLState>()),
-      texture_tbm_surface_(NULL),
+      available_tbm_surface_(nullptr),
       texture_id_(nextTextureId++) {}
 
 ExternalTextureGL::~ExternalTextureGL() {
-  mutex_.lock();
   if (state_->gl_texture != 0) {
     glDeleteTextures(1, &state_->gl_texture);
   }
+
+  // If there is a available_tbm_surface_ that is not populated, remove it
+  if (available_tbm_surface_) {
+    UnmarkTbmSurfaceToUse(available_tbm_surface_);
+  }
+
   state_.release();
-  DestructionTbmSurface();
-  mutex_.unlock();
 }
 
 bool ExternalTextureGL::OnFrameAvailable(tbm_surface_h tbm_surface) {
-  mutex_.lock();
   if (!tbm_surface) {
-    FT_LOGE("tbm_surface is null");
-    mutex_.unlock();
+    FT_LOGE("[texture id:%ld] tbm_surface is null", texture_id_);
     return false;
   }
-  if (texture_tbm_surface_) {
-    FT_LOGD("texture_tbm_surface_ does not destruction, discard");
-    mutex_.unlock();
+
+  if (available_tbm_surface_) {
+    FT_LOGD(
+        "[texture id:%ld] Discard! an available tbm surface that has not yet "
+        "been used exists",
+        texture_id_);
     return false;
   }
+
   tbm_surface_info_s info;
   if (tbm_surface_get_info(tbm_surface, &info) != TBM_SURFACE_ERROR_NONE) {
-    FT_LOGD("tbm_surface not valid, pass");
-    mutex_.unlock();
+    FT_LOGD("[texture id:%ld] tbm_surface not valid, pass", texture_id_);
     return false;
   }
-  texture_tbm_surface_ = tbm_surface;
-  mutex_.unlock();
+
+  available_tbm_surface_ = tbm_surface;
+  MarkTbmSurfaceToUse(available_tbm_surface_);
+
   return true;
 }
 
 bool ExternalTextureGL::PopulateTextureWithIdentifier(
     size_t width, size_t height, FlutterOpenGLTexture* opengl_texture) {
-  mutex_.lock();
-  if (!texture_tbm_surface_) {
-    FT_LOGD("texture_tbm_surface_ is NULL");
-    mutex_.unlock();
+  if (!available_tbm_surface_) {
+    FT_LOGD("[texture id:%ld] available_tbm_surface_ is null", texture_id_);
     return false;
   }
   tbm_surface_info_s info;
-  if (tbm_surface_get_info(texture_tbm_surface_, &info) !=
+  if (tbm_surface_get_info(available_tbm_surface_, &info) !=
       TBM_SURFACE_ERROR_NONE) {
-    FT_LOGD("tbm_surface not valid");
-    DestructionTbmSurface();
-    mutex_.unlock();
+    FT_LOGD("[texture id:%ld] tbm_surface is invalid", texture_id_);
+    UnmarkTbmSurfaceToUse(available_tbm_surface_);
+    available_tbm_surface_ = nullptr;
     return false;
   }
 
 #ifdef TIZEN_RENDERER_EVAS_GL
   int attribs[] = {EVAS_GL_IMAGE_PRESERVED, GL_TRUE, 0};
   EvasGLImage egl_src_image = evasglCreateImageForContext(
       g_evas_gl, evas_gl_current_context_get(g_evas_gl),
-      EVAS_GL_NATIVE_SURFACE_TIZEN, (void*)(intptr_t)texture_tbm_surface_,
+      EVAS_GL_NATIVE_SURFACE_TIZEN, (void*)(intptr_t)available_tbm_surface_,
       attribs);
   if (!egl_src_image) {
-    mutex_.unlock();
     return false;
   }
   if (state_->gl_texture == 0) {
@@ -120,10 +141,11 @@ bool ExternalTextureGL::PopulateTextureWithIdentifier(
                             EGL_NONE};
   EGLImageKHR egl_src_image = n_eglCreateImageKHR(
       eglGetCurrentDisplay(), EGL_NO_CONTEXT, EGL_NATIVE_SURFACE_TIZEN,
-      (EGLClientBuffer)texture_tbm_surface_, attribs);
+      (EGLClientBuffer)available_tbm_surface_, attribs);
+
   if (!egl_src_image) {
-    FT_LOGE("egl_src_image create fail!!, errorcode == %d", eglGetError());
-    mutex_.unlock();
+    FT_LOGE("[texture id:%ld] egl_src_image create fail!!, errorcode == %d",
+            texture_id_, eglGetError());
     return false;
   }
   if (state_->gl_texture == 0) {
@@ -154,31 +176,13 @@ bool ExternalTextureGL::PopulateTextureWithIdentifier(
   opengl_texture->target = GL_TEXTURE_EXTERNAL_OES;
   opengl_texture->name = state_->gl_texture;
   opengl_texture->format = GL_RGBA8;
-  opengl_texture->destruction_callback = (VoidCallback)DestructionCallback;
-  opengl_texture->user_data = static_cast<void*>(this);
+  opengl_texture->destruction_callback = (VoidCallback)UnmarkTbmSurfaceToUse;
+
+  // Abandon ownership of tbm_surface
+  opengl_texture->user_data = available_tbm_surface_;
+  available_tbm_surface_ = nullptr;
+
   opengl_texture->width = width;
   opengl_texture->height = height;
-  mutex_.unlock();
   return true;
 }
-
-void ExternalTextureGL::DestructionTbmSurfaceWithLock() {
-  mutex_.lock();
-  DestructionTbmSurface();
-  mutex_.unlock();
-}
-
-void ExternalTextureGL::DestructionTbmSurface() {
-  if (!texture_tbm_surface_) {
-    FT_LOGE("tbm_surface_h is NULL");
-    return;
-  }
-  tbm_surface_destroy(texture_tbm_surface_);
-  texture_tbm_surface_ = NULL;
-}
-
-void ExternalTextureGL::DestructionCallback(void* user_data) {
-  ExternalTextureGL* externalTextureGL =
-      reinterpret_cast<ExternalTextureGL*>(user_data);
-  externalTextureGL->DestructionTbmSurfaceWithLock();
-}
diff --git a/shell/platform/tizen/external_texture_gl.h b/shell/platform/tizen/external_texture_gl.h
@@ -41,15 +41,12 @@ class ExternalTextureGL {
   bool PopulateTextureWithIdentifier(size_t width, size_t height,
                                      FlutterOpenGLTexture* opengl_texture);
   bool OnFrameAvailable(tbm_surface_h tbm_surface);
-  void DestructionTbmSurface();
-  void DestructionTbmSurfaceWithLock();
 
  private:
   std::unique_ptr<ExternalTextureGLState> state_;
   std::mutex mutex_;
-  tbm_surface_h texture_tbm_surface_;
-  static void DestructionCallback(void* user_data);
-  const long texture_id_;
+  tbm_surface_h available_tbm_surface_{nullptr};
+  const long texture_id_{0};
 };
 
 #endif  // FLUTTER_SHELL_PLATFORM_TIZEN_EXTERNAL_TEXTURE_GL_H_
diff --git a/shell/platform/tizen/flutter_tizen.cc b/shell/platform/tizen/flutter_tizen.cc
@@ -181,6 +181,7 @@ void FlutterNotifyLowMemoryWarning(FlutterWindowControllerRef controller) {
 int64_t FlutterRegisterExternalTexture(
     FlutterTextureRegistrarRef texture_registrar) {
   FT_LOGD("FlutterDesktopRegisterExternalTexture");
+  std::lock_guard<std::mutex> lock(texture_registrar->mutex);
   auto texture_gl = std::make_unique<ExternalTextureGL>();
   int64_t texture_id = texture_gl->TextureId();
   texture_registrar->textures[texture_id] = std::move(texture_gl);
@@ -193,28 +194,32 @@ int64_t FlutterRegisterExternalTexture(
 
 bool FlutterUnregisterExternalTexture(
     FlutterTextureRegistrarRef texture_registrar, int64_t texture_id) {
+  std::lock_guard<std::mutex> lock(texture_registrar->mutex);
   auto it = texture_registrar->textures.find(texture_id);
   if (it != texture_registrar->textures.end())
     texture_registrar->textures.erase(it);
-  return (FlutterEngineUnregisterExternalTexture(
-              texture_registrar->flutter_engine, texture_id) == kSuccess);
+  bool ret = FlutterEngineUnregisterExternalTexture(
+                 texture_registrar->flutter_engine, texture_id) == kSuccess;
+  return ret;
 }
 
 bool FlutterMarkExternalTextureFrameAvailable(
     FlutterTextureRegistrarRef texture_registrar, int64_t texture_id,
     void* tbm_surface) {
+  std::lock_guard<std::mutex> lock(texture_registrar->mutex);
   auto it = texture_registrar->textures.find(texture_id);
   if (it == texture_registrar->textures.end()) {
     FT_LOGE("can't find texture texture_id = %" PRId64, texture_id);
     return false;
   }
   if (!texture_registrar->textures[texture_id]->OnFrameAvailable(
           (tbm_surface_h)tbm_surface)) {
-    FT_LOGE("OnFrameAvailable fail texture_id = %" PRId64, texture_id);
+    // If a texture that has not been used already exists, it can fail
     return false;
   }
-  return (FlutterEngineMarkExternalTextureFrameAvailable(
-              texture_registrar->flutter_engine, texture_id) == kSuccess);
+  bool ret = FlutterEngineMarkExternalTextureFrameAvailable(
+                 texture_registrar->flutter_engine, texture_id) == kSuccess;
+  return ret;
 }
 
 void FlutterRegisterViewFactory(

diff --git a/shell/platform/tizen/tizen_embedder_engine.cc b/shell/platform/tizen/tizen_embedder_engine.cc
@@ -231,7 +231,7 @@ bool TizenEmbedderEngine::RunEngine(
 
   if (HasTizenRenderer()) {
     std::unique_ptr<FlutterTextureRegistrar> textures =
-    std::make_unique<FlutterTextureRegistrar>();
+        std::make_unique<FlutterTextureRegistrar>();
     textures->flutter_engine = flutter_engine;
     plugin_registrar_->texture_registrar = std::move(textures);
 
@@ -285,9 +285,16 @@ bool TizenEmbedderEngine::OnAcquireExternalTexture(
     FlutterOpenGLTexture* texture) {
   TizenEmbedderEngine* tizen_embedder_engine =
       reinterpret_cast<TizenEmbedderEngine*>(user_data);
-  return tizen_embedder_engine->plugin_registrar_->texture_registrar
-      ->textures[texture_id]
-      ->PopulateTextureWithIdentifier(width, height, texture);
+  std::lock_guard<std::mutex> lock(
+      tizen_embedder_engine->plugin_registrar_->texture_registrar->mutex);
+  auto it = tizen_embedder_engine->plugin_registrar_->texture_registrar
+                ->textures.find(texture_id);
+  int ret = false;
+  if (it != tizen_embedder_engine->plugin_registrar_->texture_registrar
+                ->textures.end()) {
+    ret = it->second->PopulateTextureWithIdentifier(width, height, texture);
+  }
+  return ret;
 }
 
 void TizenEmbedderEngine::SendWindowMetrics(int32_t width, int32_t height,

diff --git a/shell/platform/tizen/tizen_embedder_engine.h b/shell/platform/tizen/tizen_embedder_engine.h
@@ -60,6 +60,7 @@ struct FlutterTextureRegistrar {
 
   // The texture registrar managing external texture adapters.
   std::map<int64_t, std::unique_ptr<ExternalTextureGL>> textures;
+  std::mutex mutex;
 };
 
 using UniqueAotDataPtr = std::unique_ptr<_FlutterEngineAOTData, AOTDataDeleter>;