Mac h264: Plug GL program leak

ui/gl/gl_image_io_surface.mm

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/gl/gl_image_io_surface.h"
 
 #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"
 #include "base/strings/stringize_macros.h"
 #include "base/strings/stringprintf.h"
 #include "base/trace_event/memory_allocator_dump.h"
 #include "base/trace_event/memory_dump_manager.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "ui/gl/gl_bindings.h"
 #include "ui/gl/gl_context.h"
 #include "ui/gl/gl_helper.h"
+#include "ui/gl/scoped_api.h"
 #include "ui/gl/scoped_binders.h"
+#include "ui/gl/scoped_cgl.h"
 
 // Note that this must be included after gl_bindings.h to avoid conflicts.
 #include <OpenGL/CGLIOSurface.h>
 #include <Quartz/Quartz.h>
 #include <stddef.h>
 
 using gfx::BufferFormat;
 
 namespace gl {
 namespace {
@@ skipping 149 matching lines @@
       NOTREACHED();
       return 0;
   }
 
   NOTREACHED();
   return 0;
 }
 
 }  // namespace
 
+class GLImageIOSurface::RGBConverter
+    : public base::RefCounted<GLImageIOSurface::RGBConverter> {
+ public:
+  static scoped_refptr<RGBConverter> GetForCurrentContext();
+  bool CopyTexImage(IOSurfaceRef io_surface, const gfx::Size& size);
+
+ private:
+  friend class base::RefCounted<RGBConverter>;
+  RGBConverter(CGLContextObj cgl_context);
+  ~RGBConverter();
+
+  unsigned vertex_shader_ = 0;
+  unsigned fragment_shader_ = 0;
+  unsigned program_ = 0;
+  int size_location_ = -1;
+  unsigned vertex_buffer_ = 0;
+  base::ScopedTypeRef<CGLContextObj> cgl_context_;
+
+  static base::LazyInstance<
+      std::map<CGLContextObj, GLImageIOSurface::RGBConverter*>>
+      g_rgb_converters;
+};
+
+base::LazyInstance<std::map<CGLContextObj, GLImageIOSurface::RGBConverter*>>
+    GLImageIOSurface::RGBConverter::g_rgb_converters;
+
+scoped_refptr<GLImageIOSurface::RGBConverter>
+GLImageIOSurface::RGBConverter::GetForCurrentContext() {
+  CGLContextObj current_context = CGLGetCurrentContext();

[Daniele Castagna, 2016/04/11 19:55:30]

Should we DCHECK that current_context is not null?

[ccameron, 2016/04/11 20:41:21]

Good idea -- added.

+  auto found = g_rgb_converters.Get().find(current_context);

[Daniele Castagna, 2016/04/11 19:55:29]

Is it possible that we get GLImageIOSurface::BindTexImage from different threads
and we end up here from multiple threads?

[ccameron, 2016/04/11 20:41:21]

Yes -- the GLImageIOSurface::thread_checker_ should ensure that all calls to the
GLImageIOSurface are on the same thread.

[Daniele Castagna, 2016/04/11 20:56:16]

That checks that all the calls to an instance of GLImageIOSurface are on the
same thread, but now we have a singleton that could potentially be accessed from
different image instances.
Should we make that thread-safe?

We can just put the converter in the GLContext in that way we don't have to
worry about this.

[ccameron, 2016/04/12 00:34:34]

My understanding is that there is only one valid thread to access all GLImages
on.

That said, I'm open to attaching this to the GLContext -- would that be
preferable?

+  if (found != g_rgb_converters.Get().end())
+    return make_scoped_refptr(found->second);
+  return make_scoped_refptr(new RGBConverter(current_context));
+}
+
+GLImageIOSurface::RGBConverter::RGBConverter(CGLContextObj cgl_context)
+    : cgl_context_(cgl_context, base::scoped_policy::RETAIN) {
+  gfx::ScopedSetGLToRealGLApi scoped_set_gl_api;
+  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);
+
+  DCHECK(g_rgb_converters.Get().find(cgl_context) ==
+         g_rgb_converters.Get().end());
+  g_rgb_converters.Get()[cgl_context] = this;
+}
+
+GLImageIOSurface::RGBConverter::~RGBConverter() {
+  DCHECK(g_rgb_converters.Get()[cgl_context_] == this);
+  g_rgb_converters.Get().erase(cgl_context_.get());
+  {
+    gfx::ScopedCGLSetCurrentContext(cgl_context_.get());
+    gfx::ScopedSetGLToRealGLApi scoped_set_gl_api;
+    glDeleteProgram(program_);
+    glDeleteShader(vertex_shader_);
+    glDeleteShader(fragment_shader_);
+    glDeleteBuffersARB(1, &vertex_buffer_);
+  }
+  cgl_context_.reset();
+}
+
+bool GLImageIOSurface::RGBConverter::CopyTexImage(IOSurfaceRef io_surface,
+                                                  const gfx::Size& size) {
+  gfx::ScopedSetGLToRealGLApi scoped_set_gl_api;
+  DCHECK_EQ(CGLGetCurrentContext(), cgl_context_.get());
+  glTexImage2D(GL_TEXTURE_RECTANGLE_ARB, 0, GL_RGB, size.width(), size.height(),
+               0, GL_RGB, GL_UNSIGNED_BYTE, nullptr);
+  GLint target_texture = 0;
+  glGetIntegerv(GL_TEXTURE_BINDING_RECTANGLE_ARB, &target_texture);
+  DCHECK(target_texture);
+
+  GLint old_active_texture = -1;

[Daniele Castagna, 2016/04/11 19:55:29]

It'd be nice to solve the Scoped* issue before landing this patch, but since it
won't happen, should we mention why we're not using ScopedActiveTexture and
ScopedTextureBinder here, you can also mention the bug you opened about it.

[ccameron, 2016/04/11 20:41:21]

Added a comment for this.

+  glGetIntegerv(GL_ACTIVE_TEXTURE, &old_active_texture);
+  GLint old_texture0_binding = -1;
+  glActiveTexture(GL_TEXTURE0);
+  glGetIntegerv(GL_TEXTURE_BINDING_RECTANGLE_ARB, &old_texture0_binding);
+  GLint old_texture1_binding = -1;
+  glActiveTexture(GL_TEXTURE1);
+  glGetIntegerv(GL_TEXTURE_BINDING_RECTANGLE_ARB, &old_texture1_binding);
+
+  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/11 19:55:29]

Why did you decide not to put framebuffer in the RGBConverter?

[ccameron, 2016/04/11 20:41:21]

No reason -- updated the patch to leave it in the RGBConverter.

+
+  base::ScopedClosureRunner destroy_resources_runner(base::BindBlock(^{
+    glActiveTexture(GL_TEXTURE0);
+    glBindTexture(GL_TEXTURE_RECTANGLE_ARB, old_texture0_binding);
+    glActiveTexture(GL_TEXTURE1);
+    glBindTexture(GL_TEXTURE_RECTANGLE_ARB, old_texture1_binding);
+    glActiveTexture(old_active_texture);
+
+    glDeleteTextures(1, &y_texture);
+    glDeleteTextures(1, &uv_texture);
+    glDeleteFramebuffersEXT(1, &framebuffer);
+  }));
+
+  CGLError cgl_error = kCGLNoError;
+  glActiveTexture(GL_TEXTURE0);
+  glBindTexture(GL_TEXTURE_RECTANGLE_ARB, 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;
+    return false;
+  }
+  glActiveTexture(GL_TEXTURE1);
+  glBindTexture(GL_TEXTURE_RECTANGLE_ARB, 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, 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());
+  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());
+  gfx::GLHelper::DrawQuad(vertex_buffer_);
+  return true;
+}
+
 GLImageIOSurface::GLImageIOSurface(const gfx::Size& size,
                                    unsigned internalformat)
     : size_(size),
       internalformat_(internalformat),
       format_(BufferFormat::RGBA_8888) {}
 
 GLImageIOSurface::~GLImageIOSurface() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!io_surface_);
 }
@@ skipping 30 matching lines @@
     return false;
   }
 
   if (!Initialize(io_surface, io_surface_id, format))
     return false;
 
   cv_pixel_buffer_.reset(cv_pixel_buffer, base::scoped_policy::RETAIN);
   return true;
 }
 
 void GLImageIOSurface::Destroy(bool have_context) {

[Daniele Castagna, 2016/04/11 20:56:16]

Why was have_context always false in the HW decoder case?

[ccameron, 2016/04/12 00:34:34]

The HW decoder will free the GLImage at unpredictable times (like, in a callback
from when it finished decoding a frame). As a result, it doesn't know which
context will be current.

The exact mechanism is that it ends up calling the destructor on the GLImage,
which causes causes Destroy(false) to be called.

   DCHECK(thread_checker_.CalledOnValidThread());
-  if (have_context && framebuffer_) {
-    glDeleteProgram(program_);
-    glDeleteShader(vertex_shader_);
-    glDeleteShader(fragment_shader_);
-    glDeleteBuffersARB(1, &vertex_buffer_);
-    glDeleteFramebuffersEXT(1, &framebuffer_);
-  }
   io_surface_.reset();
   cv_pixel_buffer_.reset();
 }
 
 gfx::Size GLImageIOSurface::GetSize() {
   return size_;
 }
 
 unsigned GLImageIOSurface::GetInternalFormat() {
   return internalformat_;
@@ skipping 34 matching lines @@
 bool GLImageIOSurface::CopyTexImage(unsigned target) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (format_ != BufferFormat::YUV_420_BIPLANAR)
     return false;
   if (target != GL_TEXTURE_RECTANGLE_ARB) {
     LOG(ERROR) << "YUV_420_BIPLANAR requires GL_TEXTURE_RECTANGLE_ARB target";
     return false;
   }
 
-  // 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);
-  base::ScopedClosureRunner destroy_resources_runner(base::BindBlock(^{
-    glDeleteTextures(1, &y_texture);
-    glDeleteTextures(1, &uv_texture);
-  }));
-
-  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);
-  }
-
-  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);
-
-  CGLError cgl_error = kCGLNoError;
-  {
-    DCHECK(io_surface_);
-
-    gfx::ScopedActiveTexture active_texture0(GL_TEXTURE0);
-    gfx::ScopedTextureBinder texture_y_binder(GL_TEXTURE_RECTANGLE_ARB,
-                                              y_texture);
-    cgl_error = CGLTexImageIOSurface2D(
-        cgl_context, GL_TEXTURE_RECTANGLE_ARB, GL_RED, size_.width(),
-        size_.height(), GL_RED, GL_UNSIGNED_BYTE, io_surface_.get(), 0);
-    if (cgl_error != kCGLNoError) {
-      LOG(ERROR) << "Error in CGLTexImageIOSurface2D for the Y plane. "
-                 << cgl_error;
-      return false;
-    }
-    {
-      gfx::ScopedActiveTexture active_texture1(GL_TEXTURE1);
-      gfx::ScopedTextureBinder texture_uv_binder(GL_TEXTURE_RECTANGLE_ARB,
-                                                 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);
-      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());
-      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());
-
-      gfx::GLHelper::DrawQuad(vertex_buffer_);
-      // Detach the output texture from the fbo.
-      glFramebufferTexture2DEXT(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
-                                GL_TEXTURE_RECTANGLE_ARB, 0, 0);
-    }
-  }
-  return true;
+  rgb_converter_ = RGBConverter::GetForCurrentContext();
+  return rgb_converter_->CopyTexImage(io_surface_.get(), size_);
 }
 
 bool GLImageIOSurface::CopyTexSubImage(unsigned target,
                                        const gfx::Point& offset,
                                        const gfx::Rect& rect) {
   return false;
 }
 
 bool GLImageIOSurface::ScheduleOverlayPlane(gfx::AcceleratedWidget widget,
                                             int z_order,
@@ skipping 31 matching lines @@
   return cv_pixel_buffer_;
 }
 
 // static
 unsigned GLImageIOSurface::GetInternalFormatForTesting(
     gfx::BufferFormat format) {
   DCHECK(ValidFormat(format));
   return TextureFormat(format);
 }
 }  // namespace gl