IOSurface CGL context current lifetime cleanup

content/browser/compositor/io_surface_texture_mac.mm

Index: content/browser/compositor/io_surface_texture_mac.mm
diff --git a/content/browser/compositor/io_surface_texture_mac.mm b/content/browser/compositor/io_surface_texture_mac.mm
index 3424cbdfd35f56f3702d2757328111d2f3e6ee49..9a795b285b7145fa675b2a0bbe1da2b87bf817f0 100644
--- a/content/browser/compositor/io_surface_texture_mac.mm
+++ b/content/browser/compositor/io_surface_texture_mac.mm
@@ -11,8 +11,10 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/callback_helpers.h"
 #include "base/debug/trace_event.h"
 #include "base/logging.h"
+#include "base/mac/bind_objc_block.h"
 #include "base/mac/mac_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/threading/platform_thread.h"
@@ -58,24 +60,22 @@ IOSurfaceTexture::~IOSurfaceTexture() {
   {
     gfx::ScopedCGLSetCurrentContext scoped_set_current_context(
         offscreen_context_->cgl_context());

[Ken Russell (switch to Gerrit), 2014/10/07 22:09:44]

Per our over-the-shoulder review, this ScopedCGLSetCurrentContext is redundant
since this is done inside ReleaseIOSurfaceAndTexture.

[ccameron, 2014/10/07 22:33:45]

Done.

-    UnrefIOSurfaceWithContextCurrent();
+    ReleaseIOSurfaceAndTexture();
   }
   offscreen_context_ = NULL;
   DCHECK(eviction_queue_iterator_ == eviction_queue_.Get().end());
 }
 
-int IOSurfaceTexture::GetRendererID() {
-  GLint current_renderer_id = -1;
-  if (CGLGetParameter(offscreen_context_->cgl_context(),
-                      kCGLCPCurrentRendererID,
-                      &current_renderer_id) == kCGLNoError)
-    return current_renderer_id & kCGLRendererIDMatchingMask;
-  return -1;
-}
-
 bool IOSurfaceTexture::DrawIOSurface() {
   TRACE_EVENT0("browser", "IOSurfaceTexture::DrawIOSurface");
-  DCHECK(HasIOSurface());
+
+  // If we have release the IOSurface, clear the screen to light grey and
+  // early-out.
+  if (!io_surface_) {
+    glClearColor(0.9, 0.9, 0.9, 1);
+    glClear(GL_COLOR_BUFFER_BIT);
+    return false;
+  }
 
   // The viewport is the size of the CALayer, which should always match the
   // IOSurface pixel size.
@@ -139,19 +139,15 @@ bool IOSurfaceTexture::DrawIOSurface() {
 }
 
 bool IOSurfaceTexture::SetIOSurface(
-    scoped_refptr<IOSurfaceContext> context,
     IOSurfaceID io_surface_id,
     const gfx::Size& pixel_size) {
   TRACE_EVENT0("browser", "IOSurfaceTexture::MapIOSurfaceToTexture");
 
-  gfx::ScopedCGLSetCurrentContext scoped_set_current_context(
-      context->cgl_context());
-
   // Destroy the old IOSurface and texture if it is no longer needed.
   bool needs_new_iosurface =
       !io_surface_ || io_surface_id != IOSurfaceGetID(io_surface_);
   if (needs_new_iosurface)
-    UnrefIOSurfaceWithContextCurrent();
+    ReleaseIOSurfaceAndTexture();
 
   // Note that because IOSurface sizes are rounded, the same IOSurface may have
   // two different sizes associated with it, so update the sizes before the
@@ -162,12 +158,16 @@ bool IOSurfaceTexture::SetIOSurface(
   if (!needs_new_iosurface)
     return true;
 
+  // If we early-out at any point from now on, it's because of an error, and we
+  // should destroy the texture and release the IOSurface.
+  base::ScopedClosureRunner error_runner(base::BindBlock(^{
+      ReleaseIOSurfaceAndTexture();
+  }));
+
   // Open the IOSurface handle.
   io_surface_.reset(IOSurfaceLookup(io_surface_id));
-  if (!io_surface_) {
-    UnrefIOSurfaceWithContextCurrent();
+  if (!io_surface_)
     return false;
-  }
 
   // Actual IOSurface size is rounded up to reduce reallocations during window
   // resize. Get the actual size to properly map the texture.
@@ -175,46 +175,48 @@ bool IOSurfaceTexture::SetIOSurface(
                          IOSurfaceGetHeight(io_surface_));
 
   // Create the GL texture and set it to be backed by the IOSurface.
-  glGenTextures(1, &texture_);
-  glBindTexture(GL_TEXTURE_RECTANGLE_ARB, texture_);
-  glTexParameterf(GL_TEXTURE_RECTANGLE_ARB, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
-  glTexParameterf(GL_TEXTURE_RECTANGLE_ARB, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
-  GLuint plane = 0;
-  CGLError cgl_error = CGLTexImageIOSurface2D(
-      context->cgl_context(),
-      GL_TEXTURE_RECTANGLE_ARB,
-      GL_RGBA,
-      rounded_size.width(),
-      rounded_size.height(),
-      GL_BGRA,
-      GL_UNSIGNED_INT_8_8_8_8_REV,
-      io_surface_.get(),
-      plane);
-  glBindTexture(GL_TEXTURE_RECTANGLE_ARB, 0);
-  GetAndSaveGLError();
+  CGLError cgl_error = kCGLNoError;
+  {
+    gfx::ScopedCGLSetCurrentContext scoped_set_current_context(
+        offscreen_context_->cgl_context());

[Ken Russell (switch to Gerrit), 2014/10/04 00:33:39]

I'm sure I'm being dense, but am finding it difficult to prove to myself that
offscreen_context_ is in the same share group as the context which is current
when IOSurfaceTexture::DrawIOSurface is called. IOSurfaceLayer initializes its
context from IOSurfaceContext::Get(IOSurfaceContext::kCALayerContext), and
that's the context current when drawing this texture_ to the screen in
IOSurfaceTexture::DrawIOSurface. However, offscreen_context_ is initialized from
IOSurfaceContext::Get(IOSurfaceContext::kOffscreenContext), and as far as I can
tell, IOSurfaceContext deliberately puts those contexts in disjoint share
groups. Am I missing something fundamental?

[ccameron, 2014/10/04 00:49:37]

The types (kCALayerContext v kOffscreenContext) actually identify specific
contexts -- not parameters for creating the context. This is the part of the
guesswork where it seems that I need to keep the 2 types of contexts separate to
avoid crashes (the follow-on CL will show if that is the case or not).

The bit that puts the contexts in the same group is in IOSurfaceContext::Get,
where all contexts retrieved by Get are put in the same sharegroup (the
share_context there just pulls an arbitrary context out of the type->context
map).

When an error is encountered (or the GPU is changed), all of the contexts in the
sharegroup are poisoned and the map is cleared, and from then on a new
sharegroup is created.

+    glGenTextures(1, &texture_);
+    glBindTexture(GL_TEXTURE_RECTANGLE_ARB, texture_);
+    glTexParameterf(
+        GL_TEXTURE_RECTANGLE_ARB, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
+    glTexParameterf(
+        GL_TEXTURE_RECTANGLE_ARB, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
+    cgl_error = CGLTexImageIOSurface2D(
+        offscreen_context_->cgl_context(),
+        GL_TEXTURE_RECTANGLE_ARB,
+        GL_RGBA,
+        rounded_size.width(),
+        rounded_size.height(),
+        GL_BGRA,
+        GL_UNSIGNED_INT_8_8_8_8_REV,
+        io_surface_.get(),
+        0 /* plane */);
+    glBindTexture(GL_TEXTURE_RECTANGLE_ARB, 0);
+    GetAndSaveGLError();
+  }
 
   // Return failure if an error was encountered by CGL or GL.
   if (cgl_error != kCGLNoError) {
     LOG(ERROR) << "CGLTexImageIOSurface2D failed with CGL error: " << cgl_error;
-    UnrefIOSurfaceWithContextCurrent();
     return false;
   }
   if (gl_error_ != GL_NO_ERROR) {
     LOG(ERROR) << "Hit GL error in SetIOSurface: " << gl_error_;
-    UnrefIOSurfaceWithContextCurrent();
     return false;
   }
 
+  ignore_result(error_runner.Release());
   return true;
 }
 
-void IOSurfaceTexture::UnrefIOSurface() {
+void IOSurfaceTexture::ReleaseIOSurfaceAndTexture() {
   gfx::ScopedCGLSetCurrentContext scoped_set_current_context(
       offscreen_context_->cgl_context());
-  UnrefIOSurfaceWithContextCurrent();
-}
 
-void IOSurfaceTexture::UnrefIOSurfaceWithContextCurrent() {
   if (texture_) {
     glDeleteTextures(1, &texture_);
     texture_ = 0;
@@ -285,7 +287,7 @@ void IOSurfaceTexture::EvictionDoEvict() {
       continue;
 
     // Evict the surface.
-    surface->UnrefIOSurface();
+    surface->ReleaseIOSurfaceAndTexture();
   }
 }