Mac h264: Work around CGLContext+VTDecompresionSession deadlock

ui/gl/gl_image_io_surface.mm

Index: ui/gl/gl_image_io_surface.mm
diff --git a/ui/gl/gl_image_io_surface.mm b/ui/gl/gl_image_io_surface.mm
index d94a60a5e217d4a272b06e477de47da5672d7395..0cb8b07c1ebc846d72dd671ba2772032f21d9408 100644
--- a/ui/gl/gl_image_io_surface.mm
+++ b/ui/gl/gl_image_io_surface.mm
@@ -6,7 +6,10 @@
 
 #include <map>
 
+#include "base/callback_helpers.h"
 #include "base/lazy_instance.h"
+#include "base/mac/bind_objc_block.h"
+#include "base/mac/foundation_util.h"

[Daniele Castagna, 2016/04/06 20:36:58]

Is foundation_util.h included twice?

[ccameron, 2016/04/06 22:35:21]

Oops. Removed.

 #include "base/mac/foundation_util.h"
 #include "base/strings/stringize_macros.h"
 #include "base/strings/stringprintf.h"
@@ -189,6 +192,24 @@ GLenum DataType(BufferFormat format) {
 
 }  // namespace
 
+// TODO(ccameron): Share this across all GLImages that share the same context.
+class GLImageIOSurface::RGBConverter {

[Daniele Castagna, 2016/04/06 20:36:58]

In my original patch I had a similar struct to keep together all the converter
state. Reveman suggested to leave it in GLImageIOSurface.

CCing him.

+ public:
+  RGBConverter();
+  ~RGBConverter() {}
+  bool CopyTexImage(IOSurfaceRef io_surface, const gfx::Size& size);
+  void DestroyGLResources();
+
+ private:
+  unsigned vertex_shader_ = 0;
+  unsigned fragment_shader_ = 0;
+  unsigned program_ = 0;
+  int size_location_ = -1;
+  unsigned vertex_buffer_ = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(RGBConverter);
+};
+
 GLImageIOSurface::GLImageIOSurface(const gfx::Size& size,
                                    unsigned internalformat)
     : size_(size),
@@ -239,18 +260,20 @@ bool GLImageIOSurface::InitializeWithCVPixelBuffer(
   return true;
 }
 
+void GLImageIOSurface::RGBConverter::DestroyGLResources() {
+  glDeleteProgram(program_);
+  glDeleteShader(vertex_shader_);
+  glDeleteShader(fragment_shader_);
+  glDeleteBuffersARB(1, &vertex_buffer_);
+}
+
 void GLImageIOSurface::Destroy(bool have_context) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (have_context && framebuffer_) {
-    glDeleteProgram(program_);
-    glDeleteShader(vertex_shader_);
-    glDeleteShader(fragment_shader_);
-    glDeleteBuffersARB(1, &vertex_buffer_);
-    glDeleteFramebuffersEXT(1, &framebuffer_);
-    glDeleteTextures(2, yuv_textures_);
-  }
+  if (have_context && rgb_converter_)
+    rgb_converter_->DestroyGLResources();
   io_surface_.reset();
   cv_pixel_buffer_.reset();
+  rgb_converter_.reset();
 }
 
 gfx::Size GLImageIOSurface::GetSize() {
@@ -303,54 +326,70 @@ bool GLImageIOSurface::CopyTexImage(unsigned target) {
     return false;
   }
 
-  if (!framebuffer_) {
-    glGenFramebuffersEXT(1, &framebuffer_);
-    vertex_buffer_ = gfx::GLHelper::SetupQuadVertexBuffer();
-    vertex_shader_ = gfx::GLHelper::LoadShader(
-        GL_VERTEX_SHADER,
-        base::StringPrintf("%s\n%s", kGLSLVersion, kVertexShader).c_str());
-    fragment_shader_ = gfx::GLHelper::LoadShader(
-        GL_FRAGMENT_SHADER,
-        base::StringPrintf("%s\n%s\n%s", kGLSLVersion,
-                           kTextureRectangleRequired, kFragmentShader)
-            .c_str());
-    program_ = gfx::GLHelper::SetupProgram(vertex_shader_, fragment_shader_);
-    gfx::ScopedUseProgram use_program(program_);
-
-    size_location_ = glGetUniformLocation(program_, "a_texScale");
-    DCHECK_NE(-1, size_location_);
-    int y_sampler_location = glGetUniformLocation(program_, "a_y_texture");
-    DCHECK_NE(-1, y_sampler_location);
-    int uv_sampler_location = glGetUniformLocation(program_, "a_uv_texture");
-    DCHECK_NE(-1, uv_sampler_location);
-
-    glUniform1i(y_sampler_location, 0);
-    glUniform1i(uv_sampler_location, 1);
-
-    glGenTextures(2, yuv_textures_);
-    DCHECK(yuv_textures_[0]);
-    DCHECK(yuv_textures_[1]);
-  }
+  if (!rgb_converter_)
+    rgb_converter_.reset(new RGBConverter);
+  return rgb_converter_->CopyTexImage(io_surface_.get(), size_);
+}
 
+GLImageIOSurface::RGBConverter::RGBConverter() {
+  vertex_buffer_ = gfx::GLHelper::SetupQuadVertexBuffer();
+  vertex_shader_ = gfx::GLHelper::LoadShader(
+      GL_VERTEX_SHADER,
+      base::StringPrintf("%s\n%s", kGLSLVersion, kVertexShader).c_str());
+  fragment_shader_ = gfx::GLHelper::LoadShader(
+      GL_FRAGMENT_SHADER,
+      base::StringPrintf("%s\n%s\n%s", kGLSLVersion, kTextureRectangleRequired,
+                         kFragmentShader)
+          .c_str());
+  program_ = gfx::GLHelper::SetupProgram(vertex_shader_, fragment_shader_);
+  gfx::ScopedUseProgram use_program(program_);
+
+  size_location_ = glGetUniformLocation(program_, "a_texScale");
+  DCHECK_NE(-1, size_location_);
+  int y_sampler_location = glGetUniformLocation(program_, "a_y_texture");
+  DCHECK_NE(-1, y_sampler_location);
+  int uv_sampler_location = glGetUniformLocation(program_, "a_uv_texture");
+  DCHECK_NE(-1, uv_sampler_location);
+
+  glUniform1i(y_sampler_location, 0);
+  glUniform1i(uv_sampler_location, 1);
+}
+
+bool GLImageIOSurface::RGBConverter::CopyTexImage(IOSurfaceRef io_surface,
+                                                  const gfx::Size& size) {
   CGLContextObj cgl_context =
       static_cast<CGLContextObj>(gfx::GLContext::GetCurrent()->GetHandle());
 
   GLint target_texture = 0;
   glGetIntegerv(GL_TEXTURE_BINDING_RECTANGLE_ARB, &target_texture);
   DCHECK(target_texture);
-  glTexImage2D(GL_TEXTURE_RECTANGLE_ARB, 0, GL_RGB, size_.width(),
-               size_.height(), 0, GL_RGB, GL_UNSIGNED_BYTE, nullptr);
+  glTexImage2D(GL_TEXTURE_RECTANGLE_ARB, 0, GL_RGB, size.width(), size.height(),
+               0, GL_RGB, GL_UNSIGNED_BYTE, nullptr);
+
+  // Ensure that all textures bound to IOSurfaces be destroyed before the
+  // function exits. If they are not destroyed they may cause deadlocks between
+  // VTDecompressionSession at CGLContextDestroy.
+  // https://crbug.com/598388
+  unsigned y_texture = 0;
+  glGenTextures(1, &y_texture);
+  unsigned uv_texture = 0;
+  glGenTextures(1, &uv_texture);
+  unsigned framebuffer = 0;
+  glGenFramebuffersEXT(1, &framebuffer);

[Daniele Castagna, 2016/04/06 20:36:58]

Do we need to regen this one all the time too?

[ccameron, 2016/04/06 22:35:21]

It made the RGBConverter not capture any state of the particular caller, so it
seemed natural. Now that I've removed RGBConverter, it seems less necessary.

+  base::ScopedClosureRunner destroy_resources_runner(base::BindBlock(^{

[Daniele Castagna, 2016/04/06 20:36:58]

nit: I'm assuming the caret has some special meaning in objective c. Could we
use lambda syntax instead?

[ccameron, 2016/04/06 22:35:21]

Yeah, it's for a block, which is the ObjC version of a lambda.

Lambdas are a bit nastier in that I need to bind the values of the variables --
I can't actually capture them and then put them in a base::ScopedClosureRunner.
I've switch to the lambda version ... lmk which you prefer.

[Daniele Castagna, 2016/04/07 01:35:07]

Can't you just add {y,uv}_texture in the closure list?

I'm afraid we're not supposed to bind lambdas though according to
https://chromium-cpp.appspot.com/.

I don't have a strong preference here, I made that comment since I'm not
familiar with objective C and I never saw a caret before a block.

[ccameron, 2016/04/07 07:10:34]

If I include y/uv in the closure list, then I can't bind to create a
base::Closure (I checked with danakj and vmpstr on this).
I think for now BindBlock is better, so I went back to it. It would be nice to
have a base::ScopedLambdaRunner, but that's a bikeshed for another day.

+    glDeleteTextures(1, &y_texture);
+    glDeleteTextures(1, &uv_texture);
+    glDeleteFramebuffersEXT(1, &framebuffer);
+  }));
 
   CGLError cgl_error = kCGLNoError;
   {
-    DCHECK(io_surface_);
-
     gfx::ScopedActiveTexture active_texture0(GL_TEXTURE0);
     gfx::ScopedTextureBinder texture_y_binder(GL_TEXTURE_RECTANGLE_ARB,
-                                              yuv_textures_[0]);
-    cgl_error = CGLTexImageIOSurface2D(
-        cgl_context, GL_TEXTURE_RECTANGLE_ARB, GL_RED, size_.width(),
-        size_.height(), GL_RED, GL_UNSIGNED_BYTE, io_surface_.get(), 0);
+                                              y_texture);
+    cgl_error = CGLTexImageIOSurface2D(cgl_context, GL_TEXTURE_RECTANGLE_ARB,
+                                       GL_RED, size.width(), size.height(),
+                                       GL_RED, GL_UNSIGNED_BYTE, io_surface, 0);
     if (cgl_error != kCGLNoError) {
       LOG(ERROR) << "Error in CGLTexImageIOSurface2D for the Y plane. "
                  << cgl_error;
@@ -359,26 +398,26 @@ bool GLImageIOSurface::CopyTexImage(unsigned target) {
     {
       gfx::ScopedActiveTexture active_texture1(GL_TEXTURE1);
       gfx::ScopedTextureBinder texture_uv_binder(GL_TEXTURE_RECTANGLE_ARB,
-                                                 yuv_textures_[1]);
+                                                 uv_texture);
       cgl_error = CGLTexImageIOSurface2D(
-          cgl_context, GL_TEXTURE_RECTANGLE_ARB, GL_RG, size_.width() / 2,
-          size_.height() / 2, GL_RG, GL_UNSIGNED_BYTE, io_surface_.get(), 1);
+          cgl_context, GL_TEXTURE_RECTANGLE_ARB, GL_RG, size.width() / 2,
+          size.height() / 2, GL_RG, GL_UNSIGNED_BYTE, io_surface, 1);
       if (cgl_error != kCGLNoError) {
         LOG(ERROR) << "Error in CGLTexImageIOSurface2D for the UV plane. "
                    << cgl_error;
         return false;
       }
 
-      gfx::ScopedFrameBufferBinder framebuffer_binder(framebuffer_);
-      gfx::ScopedViewport viewport(0, 0, size_.width(), size_.height());
-      glViewport(0, 0, size_.width(), size_.height());
+      gfx::ScopedFrameBufferBinder framebuffer_binder(framebuffer);
+      gfx::ScopedViewport viewport(0, 0, size.width(), size.height());
+      glViewport(0, 0, size.width(), size.height());
       glFramebufferTexture2DEXT(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
                                 GL_TEXTURE_RECTANGLE_ARB, target_texture, 0);
       DCHECK_EQ(static_cast<GLenum>(GL_FRAMEBUFFER_COMPLETE),
                 glCheckFramebufferStatusEXT(GL_FRAMEBUFFER));
 
       gfx::ScopedUseProgram use_program(program_);
-      glUniform2f(size_location_, size_.width(), size_.height());
+      glUniform2f(size_location_, size.width(), size.height());
 
       gfx::GLHelper::DrawQuad(vertex_buffer_);
       // Detach the output texture from the fbo.