[Ozone-GBM] Migrate GBM buffers to use the ScanoutBuffer interface

ui/ozone/platform/dri/gbm_surface.cc

Index: ui/ozone/platform/dri/gbm_surface.cc
diff --git a/ui/ozone/platform/dri/gbm_surface.cc b/ui/ozone/platform/dri/gbm_surface.cc
index 2c5f3e154174e6fdfac6f52726ee43f458139cfa..52126ccdd99cc0d4c61427591f31855c46bbdf94 100644
--- a/ui/ozone/platform/dri/gbm_surface.cc
+++ b/ui/ozone/platform/dri/gbm_surface.cc
@@ -12,9 +12,111 @@
 #include "ui/ozone/platform/dri/dri_buffer.h"
 #include "ui/ozone/platform/dri/dri_wrapper.h"
 #include "ui/ozone/platform/dri/hardware_display_controller.h"
+#include "ui/ozone/platform/dri/scanout_buffer.h"
 
 namespace ui {
 
+namespace {
+
+// Pixel configuration for the current buffer format.
+// TODO(dnicoara) These will need to change once we query the hardware for
+// supported configurations.
+const uint8_t kColorDepth = 24;
+const uint8_t kPixelDepth = 32;
+
+}  // namespace
+
+class GbmSurface::GbmSurfaceBuffer : public ScanoutBuffer {

[dnicoara, 2014/07/18 19:30:20]

This is pretty much BufferData with a few additions.

[alexst (slow to review), 2014/07/18 19:49:07]

This looks a lot like
https://code.google.com/p/chromium/codesearch#chromium/src/ui/ozone/platform/...

Can we refactor and use that instead? 

I'd prefer this lived in its own file as well because it's worth sharing this
code with gbm native pixmap.

[dnicoara, 2014/07/18 20:04:19]

I initially wanted to do that, but the semantics of the 2 implementations are
very different so I didn't want to make it confusing. For GBM buffers taken from
a GBM surface we do not manage their lifetime. The GbmSurfaceBuffer impl is
really a wrapper around the gbm_bo + a way for GBM to call back to destroy the
wrapper (and only the wrapper). While a gbm_bo we allocate will be managed by
the GbmBuffer wrapper and needs to be destroyed by the wrapper.

There could be a base class to share the getters and framebuffer registration,
but I'd like to keep the rest separate for clarity.

[alexst (slow to review), 2014/07/18 21:11:31]

I've been looking at the details and I see what you mean, they are similar but
not quite. I like the idea of a base class, it's a fair bit of code to duplicate
and make mistakes in. With the base class in place, the remainder here and in
pixmap could just be these little private classes.

[dnicoara, 2014/07/18 21:48:27]

Done.

+ public:
+  static scoped_refptr<GbmSurfaceBuffer> CreateBuffer(DriWrapper* dri,
+                                                      gbm_bo* buffer);
+  static scoped_refptr<GbmSurfaceBuffer> GetBuffer(gbm_bo* buffer);
+
+  gbm_bo* bo() const { return bo_; }
+
+  // ScanoutBuffer:
+  virtual uint32_t GetFramebufferId() const OVERRIDE;
+  virtual uint32_t GetHandle() const OVERRIDE;
+  virtual gfx::Size GetSize() const OVERRIDE;
+
+ private:
+  GbmSurfaceBuffer(DriWrapper* dri, gbm_bo* bo);
+  virtual ~GbmSurfaceBuffer();
+
+  static void Destroy(gbm_bo* buffer, void* data);
+
+  // This buffer is special and is released by GBM at any point in time (as
+  // long as it isn't being used). Since GBM should be the only one to
+  // release this buffer, keep a self-reference in order to keep this alive.
+  // When GBM calls Destroy(..) the self-reference will dissapear and this will
+  // be destroyed.
+  scoped_refptr<GbmSurfaceBuffer> self_;
+
+  DriWrapper* dri_;
+  gbm_bo* bo_;
+  uint32_t framebuffer_;
+
+  DISALLOW_COPY_AND_ASSIGN(GbmSurfaceBuffer);
+};
+
+GbmSurface::GbmSurfaceBuffer::GbmSurfaceBuffer(DriWrapper* dri, gbm_bo* bo)
+  : dri_(dri), bo_(bo), framebuffer_(0) {
+  if (!dri_->AddFramebuffer(gbm_bo_get_width(bo),
+                            gbm_bo_get_height(bo),
+                            kColorDepth,
+                            kPixelDepth,
+                            gbm_bo_get_stride(bo),
+                            gbm_bo_get_handle(bo).u32,
+                            &framebuffer_)) {
+    LOG(ERROR) << "Failed to register buffer";
+    return;
+  }
+
+  self_ = this;
+  gbm_bo_set_user_data(bo, this, GbmSurfaceBuffer::Destroy);
+}
+
+GbmSurface::GbmSurfaceBuffer::~GbmSurfaceBuffer() {
+  if (framebuffer_)
+    dri_->RemoveFramebuffer(framebuffer_);
+}
+
+// static
+scoped_refptr<GbmSurface::GbmSurfaceBuffer>
+GbmSurface::GbmSurfaceBuffer::CreateBuffer(DriWrapper* dri, gbm_bo* buffer) {
+  scoped_refptr<GbmSurfaceBuffer> scoped_buffer(new GbmSurfaceBuffer(dri,
+                                                                     buffer));
+  if (!scoped_buffer->GetFramebufferId())
+    return NULL;
+
+  return scoped_buffer;
+}
+
+// static
+scoped_refptr<GbmSurface::GbmSurfaceBuffer>
+GbmSurface::GbmSurfaceBuffer::GetBuffer(gbm_bo* buffer) {
+  return scoped_refptr<GbmSurfaceBuffer>(
+      static_cast<GbmSurfaceBuffer*>(gbm_bo_get_user_data(buffer)));
+}
+
+// static
+void GbmSurface::GbmSurfaceBuffer::Destroy(gbm_bo* buffer, void* data) {
+  GbmSurfaceBuffer* scoped_buffer = static_cast<GbmSurfaceBuffer*>(data);
+  scoped_buffer->self_ = NULL;
+}
+
+uint32_t GbmSurface::GbmSurfaceBuffer::GetFramebufferId() const {
+  return framebuffer_;
+}
+
+uint32_t GbmSurface::GbmSurfaceBuffer::GetHandle() const {
+  return gbm_bo_get_handle(bo_).u32;
+}
+
+gfx::Size GbmSurface::GbmSurfaceBuffer::GetSize() const {
+  return gfx::Size(gbm_bo_get_width(bo_), gbm_bo_get_height(bo_));
+}
+
 GbmSurface::GbmSurface(gbm_device* device,
                        DriWrapper* dri,
                        const gfx::Size& size)
@@ -31,7 +133,7 @@ GbmSurface::GbmSurface(gbm_device* device,
 GbmSurface::~GbmSurface() {
   for (size_t i = 0; i < arraysize(buffers_); ++i) {
     if (buffers_[i]) {
-      gbm_surface_release_buffer(native_surface_, buffers_[i]);
+      gbm_surface_release_buffer(native_surface_, buffers_[i]->bo());
     }
   }
 
@@ -63,18 +165,14 @@ uint32_t GbmSurface::GetFramebufferId() const {
   if (!buffers_[front_buffer_ ^ 1])
     return dumb_buffer_->GetFramebufferId();
 
-  BufferData* data = BufferData::GetData(buffers_[front_buffer_ ^ 1]);
-  CHECK(data);
-  return data->framebuffer();
+  return buffers_[front_buffer_ ^ 1]->GetFramebufferId();
 }
 
 uint32_t GbmSurface::GetHandle() const {
   if (!buffers_[front_buffer_ ^ 1])
     return dumb_buffer_->GetHandle();
 
-  BufferData* data = BufferData::GetData(buffers_[front_buffer_ ^ 1]);
-  CHECK(data);
-  return data->handle();
+  return buffers_[front_buffer_ ^ 1]->GetHandle();
 }
 
 gfx::Size GbmSurface::Size() const {
@@ -90,22 +188,23 @@ gfx::Size GbmSurface::Size() const {
 void GbmSurface::PreSwapBuffers() {
   CHECK(native_surface_);
   // Lock the buffer we want to display.
-  buffers_[front_buffer_ ^ 1] = gbm_surface_lock_front_buffer(native_surface_);
+  gbm_bo* bo = gbm_surface_lock_front_buffer(native_surface_);
 
-  BufferData* data = BufferData::GetData(buffers_[front_buffer_ ^ 1]);
+  buffers_[front_buffer_ ^ 1] = GbmSurfaceBuffer::GetBuffer(bo);
   // If it is a new buffer, it won't have any data associated with it. So we
   // create it. On creation it will associate itself with the buffer and
   // register the buffer.
-  if (!data) {
-    data = BufferData::CreateData(dri_, buffers_[front_buffer_ ^ 1]);
-    DCHECK(data) << "Failed to associate the buffer with the controller";
+  if (!buffers_[front_buffer_ ^ 1]) {
+    buffers_[front_buffer_ ^ 1] = GbmSurfaceBuffer::CreateBuffer(dri_, bo);
+    DCHECK(buffers_[front_buffer_ ^ 1])
+        << "Failed to associate the buffer with the controller";
   }
 }
 
 void GbmSurface::SwapBuffers() {
   // If there was a frontbuffer, is no longer active. Release it back to GBM.
   if (buffers_[front_buffer_])
-    gbm_surface_release_buffer(native_surface_, buffers_[front_buffer_]);
+    gbm_surface_release_buffer(native_surface_, buffers_[front_buffer_]->bo());
 
   // Update the index to the frontbuffer.
   front_buffer_ ^= 1;