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

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

BUG=none
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284217

[dnicoara, 2014-07-18 19:30:21]

Yet another refactoring CL. Thank you for the fast reviews :)

https://codereview.chromium.org/400283002/diff/1/ui/ozone/platform/dri/gbm_su...
File ui/ozone/platform/dri/gbm_surface.cc (right):

https://codereview.chromium.org/400283002/diff/1/ui/ozone/platform/dri/gbm_su...
ui/ozone/platform/dri/gbm_surface.cc:29: class GbmSurface::GbmSurfaceBuffer :
public ScanoutBuffer {
This is pretty much BufferData with a few additions.

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

https://codereview.chromium.org/400283002/diff/1/ui/ozone/platform/dri/gbm_su...
File ui/ozone/platform/dri/gbm_surface.cc (right):

https://codereview.chromium.org/400283002/diff/1/ui/ozone/platform/dri/gbm_su...
ui/ozone/platform/dri/gbm_surface.cc:29: class GbmSurface::GbmSurfaceBuffer :
public ScanoutBuffer {
On 2014/07/18 19:30:20, dnicoara wrote:
> This is pretty much BufferData with a few additions.

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]

https://codereview.chromium.org/400283002/diff/1/ui/ozone/platform/dri/gbm_su...
File ui/ozone/platform/dri/gbm_surface.cc (right):

https://codereview.chromium.org/400283002/diff/1/ui/ozone/platform/dri/gbm_su...
ui/ozone/platform/dri/gbm_surface.cc:29: class GbmSurface::GbmSurfaceBuffer :
public ScanoutBuffer {
On 2014/07/18 19:49:07, alexst wrote:
> On 2014/07/18 19:30:20, dnicoara wrote:
> > This is pretty much BufferData with a few additions.
> 
> 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.

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]

https://codereview.chromium.org/400283002/diff/1/ui/ozone/platform/dri/gbm_su...
File ui/ozone/platform/dri/gbm_surface.cc (right):

https://codereview.chromium.org/400283002/diff/1/ui/ozone/platform/dri/gbm_su...
ui/ozone/platform/dri/gbm_surface.cc:29: class GbmSurface::GbmSurfaceBuffer :
public ScanoutBuffer {
> There could be a base class to share the getters and framebuffer registration,
> but I'd like to keep the rest separate for clarity.

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.

https://codereview.chromium.org/400283002/diff/1/ui/ozone/platform/dri/gbm_su...
File ui/ozone/platform/dri/gbm_surface.h (right):

https://codereview.chromium.org/400283002/diff/1/ui/ozone/platform/dri/gbm_su...
ui/ozone/platform/dri/gbm_surface.h:41: class GbmSurfaceBuffer;
I would prefer you declared it here fully, it should be very small once the base
class is implemented. With just the forward declaration, it's unclear where to
go looking for what this is.

[dnicoara, 2014-07-18 21:48:28]

https://codereview.chromium.org/400283002/diff/1/ui/ozone/platform/dri/gbm_su...
File ui/ozone/platform/dri/gbm_surface.cc (right):

https://codereview.chromium.org/400283002/diff/1/ui/ozone/platform/dri/gbm_su...
ui/ozone/platform/dri/gbm_surface.cc:29: class GbmSurface::GbmSurfaceBuffer :
public ScanoutBuffer {
On 2014/07/18 21:11:31, alexst wrote:
> > There could be a base class to share the getters and framebuffer
registration,
> > but I'd like to keep the rest separate for clarity.
> 
> 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.

Done.

https://codereview.chromium.org/400283002/diff/1/ui/ozone/platform/dri/gbm_su...
File ui/ozone/platform/dri/gbm_surface.h (right):

https://codereview.chromium.org/400283002/diff/1/ui/ozone/platform/dri/gbm_su...
ui/ozone/platform/dri/gbm_surface.h:41: class GbmSurfaceBuffer;
On 2014/07/18 21:11:31, alexst wrote:
> I would prefer you declared it here fully, it should be very small once the
base
> class is implemented. With just the forward declaration, it's unclear where to
> go looking for what this is.

Done.

[alexst (slow to review), 2014-07-18 21:53:11]

cool, thank you! lgtm.

[dnicoara, 2014-07-18 21:58:07]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-07-18 21:59:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dnicoara@chromium.org/400283002/20001

[commit-bot: I haz the power, 2014-07-18 22:14:30]

Change committed as 284217