Committing on behalf of p155off@gmail.com .

Remove duplicated code in AcceleratedSurface using PbufferGLContext instead.
This also fixes a crash caused by the OpenGL bindings not being initialized.

BUG=46286
TEST=flash plugin 10.1 works again in both mac 10.5 and 10.6

Review URL: http://codereview.chromium.org/2782006


git-svn-id: svn://svn.chromium.org/chrome/trunk/src@49836 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 40 insertions, 78 deletions

diff --git a/app/gfx/gl/gl_context.h b/app/gfx/gl/gl_context.h
index 1b04985..9da7583 100644
--- a/app/gfx/gl/gl_context.h
+++ b/app/gfx/gl/gl_context.h
@@ -48,6 +48,8 @@ class GLContext {
 
   // Create a GL context used for offscreen rendering. It is initially backed by
   // a 1x1 pbuffer. Use it to create an FBO to do useful rendering.
+  // |share_context|, if non-NULL, is a context which the internally created
+  // OpenGL context shares textures and other resources.
   static GLContext* CreateOffscreenGLContext(GLContext* shared_context);
 
  protected:
diff --git a/app/gfx/gl/gl_context_mac.cc b/app/gfx/gl/gl_context_mac.cc
index e69f644..7d5bc39 100644
--- a/app/gfx/gl/gl_context_mac.cc
+++ b/app/gfx/gl/gl_context_mac.cc
@@ -6,7 +6,6 @@
 
 #include <OpenGL/OpenGL.h>
 
-#include "app/surface/accelerated_surface_mac.h"
 #include "base/logging.h"
 #include "base/scoped_ptr.h"
 #include "app/gfx/gl/gl_bindings.h"
diff --git a/app/surface/accelerated_surface_mac.cc b/app/surface/accelerated_surface_mac.cc
index 06e48ddb..357b4a3 100644
--- a/app/surface/accelerated_surface_mac.cc
+++ b/app/surface/accelerated_surface_mac.cc
@@ -10,62 +10,21 @@
 #include "gfx/rect.h"
 
 AcceleratedSurface::AcceleratedSurface()
-    : gl_context_(NULL),
-      pbuffer_(NULL),
-      allocate_fbo_(false),
+    : allocate_fbo_(false),
       texture_(0),
       fbo_(0),
       depth_stencil_renderbuffer_(0) {
 }
 
-bool AcceleratedSurface::Initialize(CGLContextObj share_context,
+bool AcceleratedSurface::Initialize(gfx::GLContext* share_context,
                                     bool allocate_fbo) {
   allocate_fbo_ = allocate_fbo;
 
-  // TODO(kbr): we should reuse the code for PbufferGLContext here instead
-  // of duplicating it. However, in order to do so, we need to move the
-  // GLContext classes out of gpu/ and into gfx/.
-
-  // Create a 1x1 pbuffer and associated context to bootstrap things
-  static const CGLPixelFormatAttribute attribs[] = {
-    (CGLPixelFormatAttribute) kCGLPFAPBuffer,
-    (CGLPixelFormatAttribute) 0
-  };
-  CGLPixelFormatObj pixel_format;
-  GLint num_pixel_formats;
-  if (CGLChoosePixelFormat(attribs,
-                           &pixel_format,
-                           &num_pixel_formats) != kCGLNoError) {
-    DLOG(ERROR) << "Error choosing pixel format.";
-    return false;
-  }
-  if (!pixel_format) {
-    return false;
-  }
-  CGLContextObj context;
-  CGLError res = CGLCreateContext(pixel_format, share_context, &context);
-  CGLDestroyPixelFormat(pixel_format);
-  if (res != kCGLNoError) {
-    DLOG(ERROR) << "Error creating context.";
-    return false;
-  }
-  CGLPBufferObj pbuffer;
-  if (CGLCreatePBuffer(1, 1,
-                       GL_TEXTURE_2D, GL_RGBA,
-                       0, &pbuffer) != kCGLNoError) {
-    CGLDestroyContext(context);
-    DLOG(ERROR) << "Error creating pbuffer.";
-    return false;
-  }
-  if (CGLSetPBuffer(context, pbuffer, 0, 0, 0) != kCGLNoError) {
-    CGLDestroyContext(context);
-    CGLDestroyPBuffer(pbuffer);
-    DLOG(ERROR) << "Error attaching pbuffer to context.";
+  gl_context_.reset(gfx::GLContext::CreateOffscreenGLContext(share_context));
+  if (!gl_context_.get())
     return false;
-  }
-  gl_context_ = context;
-  pbuffer_ = pbuffer;
-  // Now we're ready to handle SetWindowSize calls, which will
+
+  // Now we're ready to handle SetSurfaceSize calls, which will
   // allocate and/or reallocate the IOSurface and associated offscreen
   // OpenGL structures for rendering.
   return true;
@@ -82,10 +41,10 @@ void AcceleratedSurface::Destroy() {
     dib_free_callback_->Run(transport_dib_->id());
   }
   transport_dib_.reset();
-  if (gl_context_)
-    CGLDestroyContext(gl_context_);
-  if (pbuffer_)
-    CGLDestroyPBuffer(pbuffer_);
+
+  if (gl_context_.get())
+    gl_context_->Destroy();
+  gl_context_.reset();
 }
 
 // Call after making changes to the surface which require a visual update.
@@ -198,7 +157,7 @@ bool AcceleratedSurface::SetupFrameBufferObject(GLenum target) {
   // Attach the depth and stencil buffer.
   if (fbo_status == GL_FRAMEBUFFER_COMPLETE_EXT) {
     glFramebufferRenderbufferEXT(GL_FRAMEBUFFER_EXT,
-                                 0x8D20, // GL_STENCIL_ATTACHMENT,
+                                 0x8D20,  // GL_STENCIL_ATTACHMENT,
                                  GL_RENDERBUFFER_EXT,
                                  depth_stencil_renderbuffer_);
     fbo_status = glCheckFramebufferStatusEXT(GL_FRAMEBUFFER_EXT);
@@ -207,16 +166,13 @@ bool AcceleratedSurface::SetupFrameBufferObject(GLenum target) {
 }
 
 bool AcceleratedSurface::MakeCurrent() {
-  if (CGLGetCurrentContext() != gl_context_) {
-    if (CGLSetCurrentContext(gl_context_) != kCGLNoError) {
-      DLOG(ERROR) << "Unable to make gl context current.";
-      return false;
-    }
-  }
-  return true;
+  if (!gl_context_.get())
+    return false;
+  return gl_context_->MakeCurrent();
 }
 
 void AcceleratedSurface::Clear(const gfx::Rect& rect) {
+  DCHECK(gl_context_->IsCurrent());
   glClearColor(0, 0, 0, 0);
   glViewport(0, 0, rect.width(), rect.height());
   glMatrixMode(GL_PROJECTION);
@@ -274,15 +230,16 @@ uint64 AcceleratedSurface::SetSurfaceSize(const gfx::Size& size) {
 
   // Don't think we need to identify a plane.
   GLuint plane = 0;
-  io_surface_support->CGLTexImageIOSurface2D(gl_context_,
-                                             target,
-                                             GL_RGBA,
-                                             size.width(),
-                                             size.height(),
-                                             GL_BGRA,
-                                             GL_UNSIGNED_INT_8_8_8_8_REV,
-                                             io_surface_.get(),
-                                             plane);
+  io_surface_support->CGLTexImageIOSurface2D(
+      static_cast<CGLContextObj>(gl_context_->GetHandle()),
+      target,
+      GL_RGBA,
+      size.width(),
+      size.height(),
+      GL_BGRA,
+      GL_UNSIGNED_INT_8_8_8_8_REV,
+      io_surface_.get(),
+      plane);
   if (allocate_fbo_) {
     // Set up the frame buffer object.
     SetupFrameBufferObject(target);
@@ -333,6 +290,7 @@ TransportDIB::Handle AcceleratedSurface::SetTransportDIBSize(
   }
 
   if (allocate_fbo_) {
+    DCHECK(gl_context_->IsCurrent());
     // Set up the render buffers and reserve enough space on the card for the
     // framebuffer texture.
     GLenum target = GL_TEXTURE_RECTANGLE_ARB;
diff --git a/app/surface/accelerated_surface_mac.h b/app/surface/accelerated_surface_mac.h
index d1fe311..608b17b 100644
--- a/app/surface/accelerated_surface_mac.h
+++ b/app/surface/accelerated_surface_mac.h
@@ -7,6 +7,7 @@
 
 #include <CoreFoundation/CoreFoundation.h>
 
+#include "app/gfx/gl/gl_context.h"
 #include "app/surface/transport_dib.h"
 #include "base/callback.h"
 #include "base/scoped_cftyperef.h"
@@ -17,7 +18,6 @@
 // Should not include GL headers in a header file. Forward declare these types
 // instead.
 typedef struct _CGLContextObject* CGLContextObj;
-typedef struct _CGLPBufferObject* CGLPBufferObj;
 typedef unsigned int GLenum;
 typedef unsigned int GLuint;
 
@@ -27,7 +27,7 @@ class Rect;
 
 // Encapsulates an accelerated GL surface that can be shared across processes
 // on systems that support it (10.6 and above). For systems that do not, it
-// uses a regular dib. There will either be a GL Context or a TransportDIB,
+// uses a regular dib. There will either be an IOSurface or a TransportDIB,
 // never both.
 
 class AcceleratedSurface {
@@ -45,11 +45,11 @@ class AcceleratedSurface {
   // implementation does not know to bind the accelerated surface's
   // internal FBO when the default FBO is bound. Returns false upon
   // failure.
-  bool Initialize(CGLContextObj share_context, bool allocate_fbo);
+  bool Initialize(gfx::GLContext* share_context, bool allocate_fbo);
   // Tear down. Must be called before destructor to prevent leaks.
   void Destroy();
 
-  // These methods are used only when there is a GL surface.
+  // These methods are used only once the accelerated surface is initialized.
 
   // Sets the accelerated surface to the given size, creating a new one if
   // the height or width changes. Returns a unique id of the IOSurface to
@@ -64,7 +64,8 @@ class AcceleratedSurface {
   // MakeCurrent().
   void Clear(const gfx::Rect& rect);
   // Call after making changes to the surface which require a visual update.
-  // Makes the rendering show up in other processes.
+  // Makes the rendering show up in other processes. Assumes the caller has
+  // already called MakeCurrent().
   //
   // If this AcceleratedSurface is configured with its own FBO, then
   // this call causes the color buffer to be transmitted. Otherwise,
@@ -84,13 +85,16 @@ class AcceleratedSurface {
   // texture is a legal name in the namespace of the current context.
   void SwapBuffers();
 
-  CGLContextObj context() { return gl_context_; }
+  CGLContextObj context() {
+    return static_cast<CGLContextObj>(gl_context_->GetHandle());
+  }
 
   // These methods are only used when there is a transport DIB.
 
   // Sets the transport DIB to the given size, creating a new one if the
   // height or width changes. Returns a handle to the new DIB, or a default
-  // handle if no changes were made.
+  // handle if no changes were made. Assumes the caller has already called
+  // MakeCurrent().
   TransportDIB::Handle SetTransportDIBSize(const gfx::Size& size);
   // Sets the methods to use for allocating and freeing memory for the
   // transport DIB.
@@ -119,8 +123,7 @@ class AcceleratedSurface {
   // speaking, we do not need to allocate a GL context all of the
   // time. We only need one if (a) we are using the IOSurface code
   // path, or (b) if we are allocating an FBO internally.
-  CGLContextObj gl_context_;
-  CGLPBufferObj pbuffer_;
+  scoped_ptr<gfx::GLContext> gl_context_;
   // Either |io_surface_| or |transport_dib_| is a valid pointer, but not both.
   // |io_surface_| is non-NULL if the IOSurface APIs are supported (Mac OS X
   // 10.6 and later).