ozone: gbm: Add overlay support

ozone: gbm: Add overlay support

The surface builds up a list of overlays as they come in and HDC
is modified to do the page flip + present all overlays in one go.

BUG=380861
NOTRY=true

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

[dnicoara, 2014-07-07 19:17:55]

Please also add some unittests for the overlay path.

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_wrapper.cc (right):

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_wrapper.cc:16: int ToFixedPoint(double v) {
Return an uint32_t please.

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_wrapper.cc:17: return v * 65536.0;
Could you please add a comment (maybe to this function) at what it's doing. It
took me a minute to remember what the magic constant is.

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/gb...
File ui/ozone/platform/dri/gbm_surface.cc (right):

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/gb...
ui/ozone/platform/dri/gbm_surface.cc:85: LockCurrentDrawable();
Just move the contents of LockCurrentDrawable into here.

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/gb...
File ui/ozone/platform/dri/gbm_surface_factory.cc (right):

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/gb...
ui/ozone/platform/dri/gbm_surface_factory.cc:72: bool ret =
controller_->SchedulePageFlip(&overlays_);
I think it would be better if HDC takes a reference to the buffers it is
scanning out of. GbmSurfaceAdapter can be destroyed before HDC and we want the
buffers to live until HDC is destroyed.

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/ha...
File ui/ozone/platform/dri/hardware_display_controller.cc (right):

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/ha...
ui/ozone/platform/dri/hardware_display_controller.cc:117: ScanoutSurface*
primary = surface_.get();
It seems that surface is going to be ignored. Can we recycle it when
SchedulePageFlip is called with overlays?

I'm thinking that when running on the overlay path |surface_| could be replaced
by the new ScanoutSurface on every SchedulePageFlip(). You probably also want to
optimize the GBM path to create a GbmBuffer rather than a GbmSurface through the
ScanoutSurfaceGenerator (see ozone_platform_gbm.cc).

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/ha...
File ui/ozone/platform/dri/hardware_display_controller.h (right):

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/ha...
ui/ozone/platform/dri/hardware_display_controller.h:138: bool
SchedulePageFlip(std::vector<OzoneOverlayPlane>* overlays);
Pass by const reference.

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/ha...
ui/ozone/platform/dri/hardware_display_controller.h:162: bool
ScheduleOverlayPlane(int plane_z_order,
Please delete. This isn't used or defined anywhere.

[achaulk, 2014-07-07 20:18:05]

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/dr...
File ui/ozone/platform/dri/dri_wrapper.cc (right):

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_wrapper.cc:16: int ToFixedPoint(double v) {
On 2014/07/07 19:17:54, dnicoara wrote:
> Return an uint32_t please.

Done.

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/dr...
ui/ozone/platform/dri/dri_wrapper.cc:17: return v * 65536.0;
On 2014/07/07 19:17:54, dnicoara wrote:
> Could you please add a comment (maybe to this function) at what it's doing. It
> took me a minute to remember what the magic constant is.

Sure

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/gb...
File ui/ozone/platform/dri/gbm_surface.cc (right):

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/gb...
ui/ozone/platform/dri/gbm_surface.cc:85: LockCurrentDrawable();
On 2014/07/07 19:17:54, dnicoara wrote:
> Just move the contents of LockCurrentDrawable into here.

Done.

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/gb...
File ui/ozone/platform/dri/gbm_surface_factory.cc (right):

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/gb...
ui/ozone/platform/dri/gbm_surface_factory.cc:72: bool ret =
controller_->SchedulePageFlip(&overlays_);
On 2014/07/07 19:17:54, dnicoara wrote:
> I think it would be better if HDC takes a reference to the buffers it is
> scanning out of. GbmSurfaceAdapter can be destroyed before HDC and we want the
> buffers to live until HDC is destroyed.

Done.

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/ha...
File ui/ozone/platform/dri/hardware_display_controller.cc (right):

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/ha...
ui/ozone/platform/dri/hardware_display_controller.cc:117: ScanoutSurface*
primary = surface_.get();
On 2014/07/07 19:17:54, dnicoara wrote:
> It seems that surface is going to be ignored. Can we recycle it when
> SchedulePageFlip is called with overlays?
> 
> I'm thinking that when running on the overlay path |surface_| could be
replaced
> by the new ScanoutSurface on every SchedulePageFlip(). You probably also want
to
> optimize the GBM path to create a GbmBuffer rather than a GbmSurface through
the
> ScanoutSurfaceGenerator (see ozone_platform_gbm.cc).

It can't be *directly* replaced since HDC owns the surface via a scoped_ptr. It
can probably be NULLed though

I tried creating only GbmBuffer objects, but that doesn't seem to work, maybe
something with the initial modeset actually requires a gbm_surface?

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/ha...
File ui/ozone/platform/dri/hardware_display_controller.h (right):

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/ha...
ui/ozone/platform/dri/hardware_display_controller.h:138: bool
SchedulePageFlip(std::vector<OzoneOverlayPlane>* overlays);
On 2014/07/07 19:17:55, dnicoara wrote:
> Pass by const reference.

The contents are modified, or will be anyway once we add in figuring out which
entry goes to which plane

https://codereview.chromium.org/371813004/diff/40001/ui/ozone/platform/dri/ha...
ui/ozone/platform/dri/hardware_display_controller.h:162: bool
ScheduleOverlayPlane(int plane_z_order,
On 2014/07/07 19:17:55, dnicoara wrote:
> Please delete. This isn't used or defined anywhere.

Done.

[alexst (slow to review), 2014-07-08 17:22:32]

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
File ui/ozone/platform/dri/dri_wrapper.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri_wrapper.cc:98: const gfx::RectF& source,
In this case source is meant to be in [0, 1] uv space, like texture lookups.
Passing it as is below doesn't give you the right values, you need to multiply
it by size.

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/g...
File ui/ozone/platform/dri/gbm_surface.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/g...
ui/ozone/platform/dri/gbm_surface.cc:106: // If there was a frontbuffer, is no
longer active. Release it back to GBM.
it is no longer...

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/g...
File ui/ozone/platform/dri/gbm_surface_factory.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/g...
ui/ozone/platform/dri/gbm_surface_factory.cc:43:
std::vector<scoped_refptr<NativePixmap> > overlay_refs_;
nit: these vector declarations are re-used in a bunch of places, can you typedef
them to NativePixmapList, etc? This is a pattern used in a lot of place in
chromium code and I'd like to carry it over.

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/g...
ui/ozone/platform/dri/gbm_surface_factory.cc:71: bool ret =
controller_->SchedulePageFlip(&overlays_, &overlay_refs_);
this is hard to read without context, please rename to flip_succeeded

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/h...
File ui/ozone/platform/dri/hardware_display_controller.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/h...
ui/ozone/platform/dri/hardware_display_controller.cc:117:
std::vector<OzoneOverlayPlane>* overlays) {
If you are not modifying this vector please pass it as const ref.

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/h...
ui/ozone/platform/dri/hardware_display_controller.cc:136: ScanoutSurface*
primary = AssignOverlayPlanes(overlays);
This is only dealing with one plane right now, can you rename it to something
like GetPrimaryPlane?

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/o...
File ui/ozone/platform/dri/ozone_platform_gbm.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/o...
ui/ozone/platform/dri/ozone_platform_gbm.cc:77: ScanoutSurface* surf = buffer;
Why the temp assignment here?

[achaulk, 2014-07-08 18:16:38]

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
File ui/ozone/platform/dri/dri_wrapper.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri_wrapper.cc:98: const gfx::RectF& source,
On 2014/07/08 17:22:32, alexst wrote:
> In this case source is meant to be in [0, 1] uv space, like texture lookups.
> Passing it as is below doesn't give you the right values, you need to multiply
> it by size.

Right

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/g...
File ui/ozone/platform/dri/gbm_surface_factory.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/g...
ui/ozone/platform/dri/gbm_surface_factory.cc:43:
std::vector<scoped_refptr<NativePixmap> > overlay_refs_;
On 2014/07/08 17:22:32, alexst wrote:
> nit: these vector declarations are re-used in a bunch of places, can you
typedef
> them to NativePixmapList, etc? This is a pattern used in a lot of place in
> chromium code and I'd like to carry it over.

Sure

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/g...
ui/ozone/platform/dri/gbm_surface_factory.cc:71: bool ret =
controller_->SchedulePageFlip(&overlays_, &overlay_refs_);
On 2014/07/08 17:22:32, alexst wrote:
> this is hard to read without context, please rename to flip_succeeded

Done.

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/h...
File ui/ozone/platform/dri/hardware_display_controller.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/h...
ui/ozone/platform/dri/hardware_display_controller.cc:136: ScanoutSurface*
primary = AssignOverlayPlanes(overlays);
On 2014/07/08 17:22:32, alexst wrote:
> This is only dealing with one plane right now, can you rename it to something
> like GetPrimaryPlane?

Alright, but why make more work for the future when we want to change it back?

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/o...
File ui/ozone/platform/dri/ozone_platform_gbm.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/o...
ui/ozone/platform/dri/ozone_platform_gbm.cc:77: ScanoutSurface* surf = buffer;
On 2014/07/08 17:22:32, alexst wrote:
> Why the temp assignment here?

GbmBuffer is non-deletable because of the refcounted base class, but
ScanoutSurface is. We can't pass back a NativePixmap reference here either
because not all surfaces are pixmaps.

[alexst (slow to review), 2014-07-08 21:28:37]

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/o...
File ui/ozone/platform/dri/ozone_platform_gbm.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/o...
ui/ozone/platform/dri/ozone_platform_gbm.cc:77: ScanoutSurface* surf = buffer;
On 2014/07/08 18:16:37, achaulk wrote:
> On 2014/07/08 17:22:32, alexst wrote:
> > Why the temp assignment here?
> 
> GbmBuffer is non-deletable because of the refcounted base class, but
> ScanoutSurface is. We can't pass back a NativePixmap reference here either
> because not all surfaces are pixmaps.

I see. It's subtle, though, can we make something like this work instead?

scoped_ptr<GbmBuffer> buffer = ...
if(!init) {
  return NULL;
}
return buffer->release();

[achaulk, 2014-07-08 21:36:13]

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/o...
File ui/ozone/platform/dri/ozone_platform_gbm.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/o...
ui/ozone/platform/dri/ozone_platform_gbm.cc:77: ScanoutSurface* surf = buffer;
On 2014/07/08 21:28:37, alexst wrote:
> On 2014/07/08 18:16:37, achaulk wrote:
> > On 2014/07/08 17:22:32, alexst wrote:
> > > Why the temp assignment here?
> > 
> > GbmBuffer is non-deletable because of the refcounted base class, but
> > ScanoutSurface is. We can't pass back a NativePixmap reference here either
> > because not all surfaces are pixmaps.
> 
> I see. It's subtle, though, can we make something like this work instead?
> 
> scoped_ptr<GbmBuffer> buffer = ...
> if(!init) {
>   return NULL;
> }
> return buffer->release();

I suppose that works just as well. We already have a "refcounted" object that is
really only refcounted some of the time, so there shouldn't be any harm in
making it just straight up deletable.

[achaulk, 2014-07-08 21:38:52]

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/o...
File ui/ozone/platform/dri/ozone_platform_gbm.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/o...
ui/ozone/platform/dri/ozone_platform_gbm.cc:77: ScanoutSurface* surf = buffer;
On 2014/07/08 21:36:12, achaulk wrote:
> On 2014/07/08 21:28:37, alexst wrote:
> > On 2014/07/08 18:16:37, achaulk wrote:
> > > On 2014/07/08 17:22:32, alexst wrote:
> > > > Why the temp assignment here?
> > > 
> > > GbmBuffer is non-deletable because of the refcounted base class, but
> > > ScanoutSurface is. We can't pass back a NativePixmap reference here either
> > > because not all surfaces are pixmaps.
> > 
> > I see. It's subtle, though, can we make something like this work instead?
> > 
> > scoped_ptr<GbmBuffer> buffer = ...
> > if(!init) {
> >   return NULL;
> > }
> > return buffer->release();
> 
> I suppose that works just as well. We already have a "refcounted" object that
is
> really only refcounted some of the time, so there shouldn't be any harm in
> making it just straight up deletable.

I spoke too soon. Compiler says no

../../base/macros.h:160:35: error: static assertion failed:
T_is_refcounted_type_and_needs_scoped_refptr
 #define COMPILE_ASSERT(expr, msg) static_assert(expr, #msg)
                                   ^
../../base/memory/scoped_ptr.h:313:3: note: in expansion of macro
'COMPILE_ASSERT'
   COMPILE_ASSERT(base::internal::IsNotRefCounted<T>::value,

We also can't use a scoped_refptr because ScanoutSurface is not also refcounted
(and cannot be using our refcounted template base)

[alexst (slow to review), 2014-07-09 14:21:13]

> I spoke too soon. Compiler says no
> 
> ../../base/macros.h:160:35: error: static assertion failed:
> T_is_refcounted_type_and_needs_scoped_refptr
>  #define COMPILE_ASSERT(expr, msg) static_assert(expr, #msg)
>                                    ^
> ../../base/memory/scoped_ptr.h:313:3: note: in expansion of macro
> 'COMPILE_ASSERT'
>    COMPILE_ASSERT(base::internal::IsNotRefCounted<T>::value,
> 
> We also can't use a scoped_refptr because ScanoutSurface is not also
refcounted
> (and cannot be using our refcounted template base)

Let's refactor to remove this duality. NativePixmap isn't pure virtual because
of the ref counted base, which introduces these multiple inheritance
unpleasantries.
The result of this call is stored in scoped_ptr<ScanoutSurface> inside
screen_manager and as refptr elsewhere depending on how it was cast it. It's not
ideal.

How about making a tiny wrapper class along the lines of

NativePixmapImpl : public NativePixmap { 
void* GetEGLClientBuffer() { return buffer_->bo();}
GbmBuffer buffer_; 
}

and removing the NativePixmap implementation from GbmBuffer?

[achaulk, 2014-07-09 14:44:32]

On 2014/07/09 14:21:13, alexst wrote:
> > I spoke too soon. Compiler says no
> > 
> > ../../base/macros.h:160:35: error: static assertion failed:
> > T_is_refcounted_type_and_needs_scoped_refptr
> >  #define COMPILE_ASSERT(expr, msg) static_assert(expr, #msg)
> >                                    ^
> > ../../base/memory/scoped_ptr.h:313:3: note: in expansion of macro
> > 'COMPILE_ASSERT'
> >    COMPILE_ASSERT(base::internal::IsNotRefCounted<T>::value,
> > 
> > We also can't use a scoped_refptr because ScanoutSurface is not also
> refcounted
> > (and cannot be using our refcounted template base)
> 
> Let's refactor to remove this duality. NativePixmap isn't pure virtual because
> of the ref counted base, which introduces these multiple inheritance
> unpleasantries.
> The result of this call is stored in scoped_ptr<ScanoutSurface> inside
> screen_manager and as refptr elsewhere depending on how it was cast it. It's
not
> ideal.
> 
> How about making a tiny wrapper class along the lines of
> 
> NativePixmapImpl : public NativePixmap { 
> void* GetEGLClientBuffer() { return buffer_->bo();}
> GbmBuffer buffer_; 
> }
> 
> and removing the NativePixmap implementation from GbmBuffer?

Sure we can do something like that

[dnicoara, 2014-07-10 17:00:57]

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
File ui/ozone/platform/dri/dri_wrapper.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri_wrapper.cc:98: const gfx::RectF& source,
On 2014/07/08 17:22:32, alexst wrote:
> In this case source is meant to be in [0, 1] uv space, like texture lookups.
> Passing it as is below doesn't give you the right values, you need to multiply
> it by size.

Seems like an implementation detail that isn't present in the interface
description. Can we have all parameters in the same coordinate space?

https://codereview.chromium.org/371813004/diff/180001/ui/ozone/platform/dri/h...
File ui/ozone/platform/dri/hardware_display_controller.cc (right):

https://codereview.chromium.org/371813004/diff/180001/ui/ozone/platform/dri/h...
ui/ozone/platform/dri/hardware_display_controller.cc:121: if (plane.z_order ==
0) {
Should replace |surface_| in this case otherwise later calls which do not
specify a primary will use a stale buffer.

[achaulk, 2014-07-10 17:07:50]

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
File ui/ozone/platform/dri/dri_wrapper.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri_wrapper.cc:98: const gfx::RectF& source,
On 2014/07/10 17:00:56, dnicoara wrote:
> On 2014/07/08 17:22:32, alexst wrote:
> > In this case source is meant to be in [0, 1] uv space, like texture lookups.
> > Passing it as is below doesn't give you the right values, you need to
multiply
> > it by size.
> 
> Seems like an implementation detail that isn't present in the interface
> description. Can we have all parameters in the same coordinate space?

I guess we could pass the surface size in here and then multiply it here instead

https://codereview.chromium.org/371813004/diff/180001/ui/ozone/platform/dri/h...
File ui/ozone/platform/dri/hardware_display_controller.cc (right):

https://codereview.chromium.org/371813004/diff/180001/ui/ozone/platform/dri/h...
ui/ozone/platform/dri/hardware_display_controller.cc:121: if (plane.z_order ==
0) {
On 2014/07/10 17:00:56, dnicoara wrote:
> Should replace |surface_| in this case otherwise later calls which do not
> specify a primary will use a stale buffer.

We could NULL it, but we can't store any of the overlay planes in |surface_|
because that implies explicit ownership and these pointers can't be deleted.

[dnicoara, 2014-07-10 17:25:47]

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
File ui/ozone/platform/dri/dri_wrapper.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri_wrapper.cc:98: const gfx::RectF& source,
On 2014/07/10 17:07:50, achaulk wrote:
> On 2014/07/10 17:00:56, dnicoara wrote:
> > On 2014/07/08 17:22:32, alexst wrote:
> > > In this case source is meant to be in [0, 1] uv space, like texture
lookups.
> > > Passing it as is below doesn't give you the right values, you need to
> multiply
> > > it by size.
> > 
> > Seems like an implementation detail that isn't present in the interface
> > description. Can we have all parameters in the same coordinate space?
> 
> I guess we could pass the surface size in here and then multiply it here
instead

I guess the placement of the comment is unfortunate. My worry is that CC is
making a call and passing 2 gfx::Rects in different coordinate systems. So the
Ozone interface is not bringing in CC implementation details. I'm thinking that
CC should perform the conversion and pass the gfx::Rects in the same coordinate
space.

https://codereview.chromium.org/371813004/diff/180001/ui/ozone/platform/dri/h...
File ui/ozone/platform/dri/hardware_display_controller.cc (right):

https://codereview.chromium.org/371813004/diff/180001/ui/ozone/platform/dri/h...
ui/ozone/platform/dri/hardware_display_controller.cc:121: if (plane.z_order ==
0) {
On 2014/07/10 17:07:50, achaulk wrote:
> On 2014/07/10 17:00:56, dnicoara wrote:
> > Should replace |surface_| in this case otherwise later calls which do not
> > specify a primary will use a stale buffer.
> 
> We could NULL it, but we can't store any of the overlay planes in |surface_|
> because that implies explicit ownership and these pointers can't be deleted.

You can't NULL it otherwise SchedulePageFlip will fail.

I'm a bit fuzzy on why you can't take a reference? HDC is already doing that for
the other overlay planes. Reading the calling code in gl_renderer.cc is
confusing me a bit. We're explicitly skipping the primary plane in CC. (So CC is
treating the primary plane in a special manner?).

What are all the supported overlay (+primary) combinations?

[achaulk, 2014-07-10 17:42:29]

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
File ui/ozone/platform/dri/dri_wrapper.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri_wrapper.cc:98: const gfx::RectF& source,
On 2014/07/10 17:25:47, dnicoara wrote:
> On 2014/07/10 17:07:50, achaulk wrote:
> > On 2014/07/10 17:00:56, dnicoara wrote:
> > > On 2014/07/08 17:22:32, alexst wrote:
> > > > In this case source is meant to be in [0, 1] uv space, like texture
> lookups.
> > > > Passing it as is below doesn't give you the right values, you need to
> > multiply
> > > > it by size.
> > > 
> > > Seems like an implementation detail that isn't present in the interface
> > > description. Can we have all parameters in the same coordinate space?
> > 
> > I guess we could pass the surface size in here and then multiply it here
> instead
> 
> I guess the placement of the comment is unfortunate. My worry is that CC is
> making a call and passing 2 gfx::Rects in different coordinate systems. So the
> Ozone interface is not bringing in CC implementation details. I'm thinking
that
> CC should perform the conversion and pass the gfx::Rects in the same
coordinate
> space.

Well, arguably this is a drm detail that cc shouldn't have to care about. cc
exports the source rect as a set of GL texture coordinates, which makes sense
for it.

I could move the conversion into gl_surface_ozone since gl images know their
size.

https://codereview.chromium.org/371813004/diff/180001/ui/ozone/platform/dri/h...
File ui/ozone/platform/dri/hardware_display_controller.cc (right):

https://codereview.chromium.org/371813004/diff/180001/ui/ozone/platform/dri/h...
ui/ozone/platform/dri/hardware_display_controller.cc:121: if (plane.z_order ==
0) {
On 2014/07/10 17:25:47, dnicoara wrote:
> On 2014/07/10 17:07:50, achaulk wrote:
> > On 2014/07/10 17:00:56, dnicoara wrote:
> > > Should replace |surface_| in this case otherwise later calls which do not
> > > specify a primary will use a stale buffer.
> > 
> > We could NULL it, but we can't store any of the overlay planes in |surface_|
> > because that implies explicit ownership and these pointers can't be deleted.
> 
> You can't NULL it otherwise SchedulePageFlip will fail.
> 
> I'm a bit fuzzy on why you can't take a reference? HDC is already doing that
for
> the other overlay planes. Reading the calling code in gl_renderer.cc is
> confusing me a bit. We're explicitly skipping the primary plane in CC. (So CC
is
> treating the primary plane in a special manner?).
> 
> What are all the supported overlay (+primary) combinations?

References are only taken on NativePixmap objects - that's the list that is
passed in to SchedulePageFlip. |surface_| is a scoped_ptr and implies ownership.
Since the NativePixmap object will delete the ScanoutSurface object (and in fact
for gbm overlay planes the ScanoutSurface object is a member of the NativePixmap
object, not allocated) we will crash horribly if it is deleted through
|surface_|.

If you're worried about having a reference at all, it will be in the list that
gets passed in.

[dnicoara, 2014-07-10 19:17:13]

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
File ui/ozone/platform/dri/dri_wrapper.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri_wrapper.cc:98: const gfx::RectF& source,
On 2014/07/10 17:42:29, achaulk wrote:
> On 2014/07/10 17:25:47, dnicoara wrote:
> > On 2014/07/10 17:07:50, achaulk wrote:
> > > On 2014/07/10 17:00:56, dnicoara wrote:
> > > > On 2014/07/08 17:22:32, alexst wrote:
> > > > > In this case source is meant to be in [0, 1] uv space, like texture
> > lookups.
> > > > > Passing it as is below doesn't give you the right values, you need to
> > > multiply
> > > > > it by size.
> > > > 
> > > > Seems like an implementation detail that isn't present in the interface
> > > > description. Can we have all parameters in the same coordinate space?
> > > 
> > > I guess we could pass the surface size in here and then multiply it here
> > instead
> > 
> > I guess the placement of the comment is unfortunate. My worry is that CC is
> > making a call and passing 2 gfx::Rects in different coordinate systems. So
the
> > Ozone interface is not bringing in CC implementation details. I'm thinking
> that
> > CC should perform the conversion and pass the gfx::Rects in the same
> coordinate
> > space.
> 
> Well, arguably this is a drm detail that cc shouldn't have to care about. cc
> exports the source rect as a set of GL texture coordinates, which makes sense
> for it.
> 
> I could move the conversion into gl_surface_ozone since gl images know their
> size.

Reading more, I think you're trying to say that CC doesn't actually have the
actual size?

In this case, I don't care much where we perform the conversion as long as we
add some documentation to surface_ozone_egl.h to be very explicit about this
behavior.

https://codereview.chromium.org/371813004/diff/180001/ui/ozone/platform/dri/h...
File ui/ozone/platform/dri/hardware_display_controller.cc (right):

https://codereview.chromium.org/371813004/diff/180001/ui/ozone/platform/dri/h...
ui/ozone/platform/dri/hardware_display_controller.cc:121: if (plane.z_order ==
0) {
On 2014/07/10 17:42:29, achaulk wrote:
> On 2014/07/10 17:25:47, dnicoara wrote:
> > On 2014/07/10 17:07:50, achaulk wrote:
> > > On 2014/07/10 17:00:56, dnicoara wrote:
> > > > Should replace |surface_| in this case otherwise later calls which do
not
> > > > specify a primary will use a stale buffer.
> > > 
> > > We could NULL it, but we can't store any of the overlay planes in
|surface_|
> > > because that implies explicit ownership and these pointers can't be
deleted.
> > 
> > You can't NULL it otherwise SchedulePageFlip will fail.
> > 
> > I'm a bit fuzzy on why you can't take a reference? HDC is already doing that
> for
> > the other overlay planes. Reading the calling code in gl_renderer.cc is
> > confusing me a bit. We're explicitly skipping the primary plane in CC. (So
CC
> is
> > treating the primary plane in a special manner?).
> > 
> > What are all the supported overlay (+primary) combinations?
> 
> References are only taken on NativePixmap objects - that's the list that is
> passed in to SchedulePageFlip. |surface_| is a scoped_ptr and implies
ownership.
> Since the NativePixmap object will delete the ScanoutSurface object (and in
fact
> for gbm overlay planes the ScanoutSurface object is a member of the
NativePixmap
> object, not allocated) we will crash horribly if it is deleted through
> |surface_|.
> 
> If you're worried about having a reference at all, it will be in the list that
> gets passed in.

We clarified this offline :)

[achaulk, 2014-07-10 19:44:40]

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
File ui/ozone/platform/dri/dri_wrapper.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri_wrapper.cc:98: const gfx::RectF& source,
On 2014/07/10 19:17:13, dnicoara wrote:
> On 2014/07/10 17:42:29, achaulk wrote:
> > On 2014/07/10 17:25:47, dnicoara wrote:
> > > On 2014/07/10 17:07:50, achaulk wrote:
> > > > On 2014/07/10 17:00:56, dnicoara wrote:
> > > > > On 2014/07/08 17:22:32, alexst wrote:
> > > > > > In this case source is meant to be in [0, 1] uv space, like texture
> > > lookups.
> > > > > > Passing it as is below doesn't give you the right values, you need
to
> > > > multiply
> > > > > > it by size.
> > > > > 
> > > > > Seems like an implementation detail that isn't present in the
interface
> > > > > description. Can we have all parameters in the same coordinate space?
> > > > 
> > > > I guess we could pass the surface size in here and then multiply it here
> > > instead
> > > 
> > > I guess the placement of the comment is unfortunate. My worry is that CC
is
> > > making a call and passing 2 gfx::Rects in different coordinate systems. So
> the
> > > Ozone interface is not bringing in CC implementation details. I'm thinking
> > that
> > > CC should perform the conversion and pass the gfx::Rects in the same
> > coordinate
> > > space.
> > 
> > Well, arguably this is a drm detail that cc shouldn't have to care about. cc
> > exports the source rect as a set of GL texture coordinates, which makes
sense
> > for it.
> > 
> > I could move the conversion into gl_surface_ozone since gl images know their
> > size.
> 
> Reading more, I think you're trying to say that CC doesn't actually have the
> actual size?
> 
> In this case, I don't care much where we perform the conversion as long as we
> add some documentation to surface_ozone_egl.h to be very explicit about this
> behavior.

CC has the right size for the primary plane (not sure about webgl), but I don't
think it's the right place to convert. CC deals with GL-isms like texture
coordinates.

[dnicoara, 2014-07-10 19:53:52]

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
File ui/ozone/platform/dri/dri_wrapper.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri_wrapper.cc:98: const gfx::RectF& source,
On 2014/07/10 19:44:40, achaulk wrote:
> On 2014/07/10 19:17:13, dnicoara wrote:
> > On 2014/07/10 17:42:29, achaulk wrote:
> > > On 2014/07/10 17:25:47, dnicoara wrote:
> > > > On 2014/07/10 17:07:50, achaulk wrote:
> > > > > On 2014/07/10 17:00:56, dnicoara wrote:
> > > > > > On 2014/07/08 17:22:32, alexst wrote:
> > > > > > > In this case source is meant to be in [0, 1] uv space, like
texture
> > > > lookups.
> > > > > > > Passing it as is below doesn't give you the right values, you need
> to
> > > > > multiply
> > > > > > > it by size.
> > > > > > 
> > > > > > Seems like an implementation detail that isn't present in the
> interface
> > > > > > description. Can we have all parameters in the same coordinate
space?
> > > > > 
> > > > > I guess we could pass the surface size in here and then multiply it
here
> > > > instead
> > > > 
> > > > I guess the placement of the comment is unfortunate. My worry is that CC
> is
> > > > making a call and passing 2 gfx::Rects in different coordinate systems.
So
> > the
> > > > Ozone interface is not bringing in CC implementation details. I'm
thinking
> > > that
> > > > CC should perform the conversion and pass the gfx::Rects in the same
> > > coordinate
> > > > space.
> > > 
> > > Well, arguably this is a drm detail that cc shouldn't have to care about.
cc
> > > exports the source rect as a set of GL texture coordinates, which makes
> sense
> > > for it.
> > > 
> > > I could move the conversion into gl_surface_ozone since gl images know
their
> > > size.
> > 
> > Reading more, I think you're trying to say that CC doesn't actually have the
> > actual size?
> > 
> > In this case, I don't care much where we perform the conversion as long as
we
> > add some documentation to surface_ozone_egl.h to be very explicit about this
> > behavior.
> 
> CC has the right size for the primary plane (not sure about webgl), but I
don't
> think it's the right place to convert. CC deals with GL-isms like texture
> coordinates.

Alex, can you chime in, I don't have enough experience on the CC & webgl.

https://codereview.chromium.org/371813004/diff/180001/ui/ozone/platform/dri/h...
File ui/ozone/platform/dri/hardware_display_controller.cc (right):

https://codereview.chromium.org/371813004/diff/180001/ui/ozone/platform/dri/h...
ui/ozone/platform/dri/hardware_display_controller.cc:154: gfx::RectF
draw_rect(crop_rect.x() * sizef.width(),
I think you can just make crop_rect non-const and replace all of this with:

crop_rect.Scale(size.width(), size.height());

[alexst (slow to review), 2014-07-10 20:22:05]

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
File ui/ozone/platform/dri/dri_wrapper.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri_wrapper.cc:98: const gfx::RectF& source,
Yeah, it's an artifact of GL. CC doesn't know the size of the buffer even if it
knows the size of the quad it renders.

For example, WebGL will downsize its buffers if it's running low on memory, so
your application may be 800x600 on the screen, but only have a 640x480 buffer.
CC doesn't know the buffer size and it's the thing that issues the call. GPU
process does, but even that aside, newer HW wants this in uv space to make sure
pixel perfect compatibility is achieved with GL. I don't know the gory details,
but that's the requirement I took from HWC.

We still want to be able to say, display a subrect of that (expressed as a
texcoord) whichever way you know how. The fact that drm works in fixed point is
a drm detail, but we have enough detail here to convert. Anyway, that's why it's
in [0,1] space, but as for the place to convert it inside ozone, I have no
preference.

[dnicoara, 2014-07-10 20:27:58]

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
File ui/ozone/platform/dri/dri_wrapper.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri_wrapper.cc:98: const gfx::RectF& source,
On 2014/07/10 20:22:05, alexst wrote:
> Yeah, it's an artifact of GL. CC doesn't know the size of the buffer even if
it
> knows the size of the quad it renders.
> 
> For example, WebGL will downsize its buffers if it's running low on memory, so
> your application may be 800x600 on the screen, but only have a 640x480 buffer.
> CC doesn't know the buffer size and it's the thing that issues the call. GPU
> process does, but even that aside, newer HW wants this in uv space to make
sure
> pixel perfect compatibility is achieved with GL. I don't know the gory
details,
> but that's the requirement I took from HWC.
> 
> We still want to be able to say, display a subrect of that (expressed as a
> texcoord) whichever way you know how. The fact that drm works in fixed point
is
> a drm detail, but we have enough detail here to convert. Anyway, that's why
it's
> in [0,1] space, but as for the place to convert it inside ozone, I have no
> preference.

OK, thank you, so we have to do it in Ozone. Please just add a comment to
surface_ozone_egl.h to better describe the input parameters.

[achaulk, 2014-07-11 16:57:49]

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
File ui/ozone/platform/dri/dri_wrapper.cc (right):

https://codereview.chromium.org/371813004/diff/120001/ui/ozone/platform/dri/d...
ui/ozone/platform/dri/dri_wrapper.cc:98: const gfx::RectF& source,
On 2014/07/10 20:27:58, dnicoara wrote:
> On 2014/07/10 20:22:05, alexst wrote:
> > Yeah, it's an artifact of GL. CC doesn't know the size of the buffer even if
> it
> > knows the size of the quad it renders.
> > 
> > For example, WebGL will downsize its buffers if it's running low on memory,
so
> > your application may be 800x600 on the screen, but only have a 640x480
buffer.
> > CC doesn't know the buffer size and it's the thing that issues the call. GPU
> > process does, but even that aside, newer HW wants this in uv space to make
> sure
> > pixel perfect compatibility is achieved with GL. I don't know the gory
> details,
> > but that's the requirement I took from HWC.
> > 
> > We still want to be able to say, display a subrect of that (expressed as a
> > texcoord) whichever way you know how. The fact that drm works in fixed point
> is
> > a drm detail, but we have enough detail here to convert. Anyway, that's why
> it's
> > in [0,1] space, but as for the place to convert it inside ozone, I have no
> > preference.
> 
> OK, thank you, so we have to do it in Ozone. Please just add a comment to
> surface_ozone_egl.h to better describe the input parameters.

Done.

https://codereview.chromium.org/371813004/diff/180001/ui/ozone/platform/dri/h...
File ui/ozone/platform/dri/hardware_display_controller.cc (right):

https://codereview.chromium.org/371813004/diff/180001/ui/ozone/platform/dri/h...
ui/ozone/platform/dri/hardware_display_controller.cc:154: gfx::RectF
draw_rect(crop_rect.x() * sizef.width(),
On 2014/07/10 19:53:52, dnicoara wrote:
> I think you can just make crop_rect non-const and replace all of this with:
> 
> crop_rect.Scale(size.width(), size.height());

Acknowledged.

[dnicoara, 2014-07-11 17:13:13]

Unless Alex has any other comments lgtm

https://codereview.chromium.org/371813004/diff/200001/ui/ozone/platform/dri/h...
File ui/ozone/platform/dri/hardware_display_controller_unittest.cc (right):

https://codereview.chromium.org/371813004/diff/200001/ui/ozone/platform/dri/h...
ui/ozone/platform/dri/hardware_display_controller_unittest.cc:22: const
gfx::SizeF kDefaultModeSizeF(kDefaultMode.hdisplay,
This should be a (1, 1) size to be in sync with what HDC expects.

[achaulk, 2014-07-11 17:13:49]

https://codereview.chromium.org/371813004/diff/200001/ui/ozone/platform/dri/h...
File ui/ozone/platform/dri/hardware_display_controller_unittest.cc (right):

https://codereview.chromium.org/371813004/diff/200001/ui/ozone/platform/dri/h...
ui/ozone/platform/dri/hardware_display_controller_unittest.cc:22: const
gfx::SizeF kDefaultModeSizeF(kDefaultMode.hdisplay,
On 2014/07/11 17:13:12, dnicoara wrote:
> This should be a (1, 1) size to be in sync with what HDC expects.

Right

[alexst (slow to review), 2014-07-11 17:27:10]

LGTM! Thank you!

[spang, 2014-07-11 18:07:46]

lgtm

[achaulk, 2014-07-11 19:14:26]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-07-11 19:15:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/achaulk@chromium.org/371813004/250001

[commit-bot: I haz the power, 2014-07-11 19:21:39]

Change committed as 282662