Add a new API to create a surfaceless GLSurface for Ozone

ui/gl/gl_surface_ozone.cc

Index: ui/gl/gl_surface_ozone.cc
diff --git a/ui/gl/gl_surface_ozone.cc b/ui/gl/gl_surface_ozone.cc
index cba706bda4e030b6daa599525846ebebbc847640..8765e71579bdcb4ece89a6b0d8df3e78240577ae 100644
--- a/ui/gl/gl_surface_ozone.cc
+++ b/ui/gl/gl_surface_ozone.cc
@@ -4,16 +4,19 @@
 
 #include "ui/gl/gl_surface.h"
 
+#include "base/bind.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "ui/gfx/native_widget_types.h"
 #include "ui/gl/gl_context.h"
 #include "ui/gl/gl_image.h"
+#include "ui/gl/gl_image_linux_dma_buffer.h"
 #include "ui/gl/gl_implementation.h"
 #include "ui/gl/gl_surface_egl.h"
 #include "ui/gl/gl_surface_osmesa.h"
 #include "ui/gl/gl_surface_stub.h"
 #include "ui/gl/scoped_make_current.h"
+#include "ui/ozone/public/native_pixmap.h"
 #include "ui/ozone/public/surface_factory_ozone.h"
 #include "ui/ozone/public/surface_ozone_egl.h"
 
@@ -21,6 +24,9 @@ namespace gfx {
 
 namespace {
 
+static void EmptyCallback() {

[dnicoara, 2015/02/23 22:48:26]

Use base::DoNothing()

+}
+
 // A thin wrapper around GLSurfaceEGL that owns the EGLNativeWindow
 class GL_EXPORT GLSurfaceOzoneEGL : public NativeViewGLSurfaceEGL {
  public:
@@ -157,7 +163,7 @@ class GL_EXPORT GLSurfaceOzoneSurfaceless : public SurfacelessEGL {
     return SwapBuffersAsync(callback);
   }
 
- private:
+ protected:
   ~GLSurfaceOzoneSurfaceless() override {
     Destroy();  // EGL surface must be destroyed before SurfaceOzone
   }
@@ -194,6 +200,137 @@ class GL_EXPORT GLSurfaceOzoneSurfaceless : public SurfacelessEGL {
   DISALLOW_COPY_AND_ASSIGN(GLSurfaceOzoneSurfaceless);
 };
 
+// This provides surface-like semantics implemented through surfaceless.
+// A framebuffer is bound automatically and re-bound at each swap.
+class GL_EXPORT GLSurfaceOzoneSurfacelessSurfaceImpl

[Pawel Osciak, 2015/02/24 00:13:47]

Please define methods outside of the class body
(http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts).

[achaulk, 2015/02/24 19:07:21]

The rest of this file uses the inline-class style

+    : public GLSurfaceOzoneSurfaceless {
+ public:
+  GLSurfaceOzoneSurfacelessSurfaceImpl(
+      scoped_ptr<ui::SurfaceOzoneEGL> ozone_surface,
+      AcceleratedWidget widget)
+      : GLSurfaceOzoneSurfaceless(ozone_surface.Pass(), widget),
+        fbo_(0),
+        current_surface_(0) {
+    textures_[0] = 0;
+    textures_[1] = 0;
+  }
+
+  unsigned int GetBackingFrameBufferObject() override { return fbo_; }

[Pawel Osciak, 2015/02/24 00:13:47]

fbo_ is a GLuint and here we use unsigned int, perhaps we should use one or the
other consistently.

+
+  bool OnMakeCurrent(GLContext* context) override {
+    if (!fbo_) {
+      glGenFramebuffersEXT(1, &fbo_);
+      if (!fbo_)
+        return false;
+      glGenTextures(2, textures_);
+      // Create and bind our pixmaps.
+      if (!ResizePixmaps())

[Pawel Osciak, 2015/02/24 00:13:47]

Please rename ResizePixmaps to something more descriptive of what it's actually
doing (CreateAndBindPixmaps() perhaps?). The comment won't be needed then
either.

[achaulk, 2015/02/24 19:07:21]

Done.

+        return false;
+    }
+    BindFramebuffer();
+    return SurfacelessEGL::OnMakeCurrent(context);
+  }
+
+  bool Resize(const gfx::Size& size) override {
+    return GLSurfaceOzoneSurfaceless::Resize(size) && ResizePixmaps();
+  }
+
+  bool SupportsPostSubBuffer() override { return false; }
+
+  bool SwapBuffers() override {
+    return SwapBuffersAsync(base::Bind(&EmptyCallback));

[dnicoara, 2015/02/23 22:48:26]

This is problematic since we're not guaranteeing that the PageFlip event
completes. We probably need to implement this properly in the platform.

[piman, 2015/02/24 03:00:57]

Right, we need the ordering guarantee here (further GL commands will execute
after the flip is done), as well as the backpressure.

[achaulk, 2015/02/24 19:07:21]

Can we block waiting for it?

+  }
+
+  bool SwapBuffersAsync(const SwapCompletionCallback& callback) override {
+    bool ret = images_[current_surface_]->ScheduleOverlayPlane(
+                   widget_, 0, OverlayTransform::OVERLAY_TRANSFORM_NONE,
+                   gfx::Rect(GetSize()), gfx::RectF(1, 1)) &&
+               GLSurfaceOzoneSurfaceless::SwapBuffersAsync(callback);

[Pawel Osciak, 2015/02/24 00:13:47]

This is difficult to read. Please split this statement.

[achaulk, 2015/02/24 19:07:21]

Done.

+    current_surface_ ^= 1;
+    BindFramebuffer();

[Pawel Osciak, 2015/02/24 00:13:47]

Do we still want to do this if ret==false?

[achaulk, 2015/02/24 19:07:21]

Hmm, probably not

+    return ret;
+  }
+
+  void Destroy() override {
+    GLContext* current_context = GLContext::GetCurrent();
+    DCHECK(current_context && current_context->IsCurrent(this));
+    glBindFramebufferEXT(GL_FRAMEBUFFER, 0);
+    if (fbo_) {

[dnicoara, 2015/02/24 16:46:53]

Should probably reset these values and DCHECK in the destructor to make sure the
resources are freed.

+      glDeleteTextures(2, textures_);
+      glDeleteFramebuffersEXT(1, &fbo_);
+    }
+    for (int i = 0; i < 2; i++) {
+      if (images_[i])
+        images_[i]->Destroy(true);
+    }
+  }
+
+ private:
+  class SurfaceImage : public GLImageLinuxDMABuffer {
+   public:
+    SurfaceImage(const gfx::Size& size, unsigned internalformat)
+        : GLImageLinuxDMABuffer(size, internalformat) {}
+
+    bool Initialize(ui::NativePixmap* pixmap,
+                    gfx::GpuMemoryBuffer::Format format) {
+      base::FileDescriptor handle(pixmap->GetDmaBufFd(), false);
+      if (!GLImageLinuxDMABuffer::Initialize(handle, format,
+                                             pixmap->GetDmaBufPitch()))
+        return false;
+      pixmap_ = pixmap;
+      return true;
+    }
+    bool ScheduleOverlayPlane(gfx::AcceleratedWidget widget,
+                              int z_order,
+                              gfx::OverlayTransform transform,
+                              const gfx::Rect& bounds_rect,
+                              const gfx::RectF& crop_rect) override {
+      return ui::SurfaceFactoryOzone::GetInstance()->ScheduleOverlayPlane(
+          widget, z_order, transform, pixmap_, bounds_rect, crop_rect);
+    }
+
+   private:
+    scoped_refptr<ui::NativePixmap> pixmap_;
+  };
+
+  ~GLSurfaceOzoneSurfacelessSurfaceImpl() override {}
+
+  void BindFramebuffer() {
+    glBindFramebufferEXT(GL_FRAMEBUFFER, fbo_);
+    glFramebufferTexture2DEXT(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
+                              GL_TEXTURE_2D, textures_[current_surface_], 0);
+  }
+
+  bool ResizePixmaps() {
+    if (!fbo_)
+      return true;

[Pawel Osciak, 2015/02/24 00:13:47]

Is this ok to succeed if !fbo_ ?

[achaulk, 2015/02/24 19:07:21]

Resizing before binding should be fine - this will be executed on both paths
anyway, so it will be guaranteed to happen at least from OnMakeCurrent

+    for (int i = 0; i < 2; i++) {
+      scoped_refptr<ui::NativePixmap> pixmap =
+          ui::SurfaceFactoryOzone::GetInstance()->CreateNativePixmap(
+              widget_, GetSize(), ui::SurfaceFactoryOzone::RGBA_8888,
+              ui::SurfaceFactoryOzone::SCANOUT);
+      if (!pixmap)
+        return false;
+      scoped_refptr<SurfaceImage> image = new SurfaceImage(GetSize(), GL_RGBA);
+      if (!image->Initialize(pixmap.get(),

[Pawel Osciak, 2015/02/24 00:13:47]

Please pass const scoped_refptr& instead of a pointer.

[achaulk, 2015/02/24 19:07:21]

Done.

+                             gfx::GpuMemoryBuffer::Format::BGRA_8888))
+        return false;
+      images_[i] = image;
+      // Bind image to texture.
+      glBindTexture(GL_TEXTURE_2D, textures_[i]);

[Pawel Osciak, 2015/02/24 00:13:47]

Should we use ScopedTextureBinder instead?

[achaulk, 2015/02/24 19:07:21]

It probably won't matter since this is at init, but sure

+      if (!images_[i]->BindTexImage(GL_TEXTURE_2D))
+        return false;
+    }
+    return true;
+  }
+
+  GLuint fbo_;
+  GLuint textures_[2];

[Pawel Osciak, 2015/02/24 00:13:47]

Please use std::vectors here and below.

[piman, 2015/02/24 02:57:32]

Why? This is fine to me. Avoids extra indirections/allocations, and (IMO)
produces clearer code.

+  scoped_refptr<GLImage> images_[2];
+  int current_surface_;
+  DISALLOW_COPY_AND_ASSIGN(GLSurfaceOzoneSurfacelessSurfaceImpl);
+};
+
 }  // namespace
 
 // static
@@ -215,6 +352,27 @@ bool GLSurface::InitializeOneOffInternal() {
 }
 
 // static
+scoped_refptr<GLSurface> GLSurface::CreateSurfacelessViewGLSurface(
+    gfx::AcceleratedWidget window) {
+  if (GetGLImplementation() == kGLImplementationEGLGLES2 &&
+      window != kNullAcceleratedWidget &&
+      GLSurfaceEGL::IsEGLSurfacelessContextSupported() &&
+      ui::SurfaceFactoryOzone::GetInstance()->CanShowPrimaryPlaneAsOverlay()) {
+    scoped_ptr<ui::SurfaceOzoneEGL> surface_ozone =
+        ui::SurfaceFactoryOzone::GetInstance()
+            ->CreateSurfacelessEGLSurfaceForWidget(window);
+    if (!surface_ozone)
+      return nullptr;
+    scoped_refptr<GLSurface> surface;
+    surface = new GLSurfaceOzoneSurfaceless(surface_ozone.Pass(), window);
+    if (surface->Initialize())
+      return surface;
+  }
+
+  return nullptr;
+}
+
+// static
 scoped_refptr<GLSurface> GLSurface::CreateViewGLSurface(
     gfx::AcceleratedWidget window) {
   if (GetGLImplementation() == kGLImplementationOSMesaGL) {
@@ -234,7 +392,8 @@ scoped_refptr<GLSurface> GLSurface::CreateViewGLSurface(
               ->CreateSurfacelessEGLSurfaceForWidget(window);
       if (!surface_ozone)
         return NULL;
-      surface = new GLSurfaceOzoneSurfaceless(surface_ozone.Pass(), window);
+      surface = new GLSurfaceOzoneSurfacelessSurfaceImpl(surface_ozone.Pass(),
+                                                         window);
     } else {
       scoped_ptr<ui::SurfaceOzoneEGL> surface_ozone =
           ui::SurfaceFactoryOzone::GetInstance()->CreateEGLSurfaceForWidget(