ui: Add ScheduleOverlayPlane API to GLImage.

ui: Add ScheduleOverlayPlane API to GLImage.

This makes it possible to use GLImage instances as overlay planes.

BUG=

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

[achaulk, 2014-07-30 18:29:51]

https://codereview.chromium.org/415283002/diff/1/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (left):

https://codereview.chromium.org/415283002/diff/1/ui/gl/gl_surface.h#oldcode119
ui/gl/gl_surface.h:119: virtual bool ScheduleOverlayPlane(int z_order,
We still need a way to get something surface-specific to show the overlay. After
talking with alexst, we think the best approach is to add a "virtual
gfx::AcceleratedWidget GetAcceleratedWidget()" function here. You can add it in
here or I can upload another CL when this goes in

[reveman, 2014-07-30 18:56:11]

sorry, forgot about this patch. please take a look.

https://codereview.chromium.org/415283002/diff/1/ui/gl/gl_surface.h
File ui/gl/gl_surface.h (left):

https://codereview.chromium.org/415283002/diff/1/ui/gl/gl_surface.h#oldcode119
ui/gl/gl_surface.h:119: virtual bool ScheduleOverlayPlane(int z_order,
On 2014/07/30 18:29:51, achaulk wrote:
> We still need a way to get something surface-specific to show the overlay.
After
> talking with alexst, we think the best approach is to add a "virtual
> gfx::AcceleratedWidget GetAcceleratedWidget()" function here. You can add it
in
> here or I can upload another CL when this goes in

Not sure but it seems like it would be more appropriate to get to whatever
surface specific handle you need at a lower level from within the ozone
implementation of ScheduleOverlayPlane. If you actually need  some addition to
the GLSurface API then let's do that separately.

[piman, 2014-07-30 20:11:42]

https://codereview.chromium.org/415283002/diff/20001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/415283002/diff/20001/ui/gl/gl_image.h#newcode59
ui/gl/gl_image.h:59: const RectF& crop_rect) = 0;
I'm not sure how this API works without extra context... E.g. how do we choose
which window or screen this is going to go to.
Shouldn't there be some connection with the GLSurface, since that's what gets
the SwapBuffers, which is what activates the overlay?

What is the semantics wrt statefulness? Is an image bound to an overlay from the
next swap until another image replaces it a the same z index? Or is the
expectation that at each frame the full set of overlays is respecified? If the
former, how do you disable an overlay?

[alexst (slow to review), 2014-07-30 20:13:26]

> Not sure but it seems like it would be more appropriate to get to whatever
> surface specific handle you need at a lower level from within the ozone
> implementation of ScheduleOverlayPlane. If you actually need  some addition to
> the GLSurface API then let's do that separately.

I agree, let's address GLSurface API's separately, this change lgtm.

For background, when we create a surface with
CreateViewGLSurface(gfx::AcceleratedWidget window), |window| roughly corresponds
to a monitor on CROS and presenting an overlay is done on the display controller
for that monitor, which is why we added this API on the surface originally
(surface knows its window/monitor). We'll have to introduce some way to access
current GLSurface's AcceleratedWidget in a followup.

[alexst (slow to review), 2014-07-30 20:21:48]

https://codereview.chromium.org/415283002/diff/20001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/415283002/diff/20001/ui/gl/gl_image.h#newcode59
ui/gl/gl_image.h:59: const RectF& crop_rect) = 0;
On 2014/07/30 20:11:42, piman (slow to review) wrote:
> I'm not sure how this API works without extra context... E.g. how do we choose
> which window or screen this is going to go to.
> Shouldn't there be some connection with the GLSurface, since that's what gets
> the SwapBuffers, which is what activates the overlay?
> 
> What is the semantics wrt statefulness? Is an image bound to an overlay from
the
> next swap until another image replaces it a the same z index? Or is the
> expectation that at each frame the full set of overlays is respecified? If the
> former, how do you disable an overlay?

Re: extra context, I just commented on that before I saw your message. We'll
need the current GLSurface's AcceleratedWidget to get that. We can add that API
to the surface now or address as a followup.

Re: statefullness, the expectation is that the full set of overlays is
respecified on each swap.

[piman, 2014-07-30 20:48:33]

On Wed, Jul 30, 2014 at 1:21 PM, <alexst@chromium.org> wrote:

>
> https://codereview.chromium.org/415283002/diff/20001/ui/gl/gl_image.h
> File ui/gl/gl_image.h (right):
>
> https://codereview.chromium.org/415283002/diff/20001/ui/
> gl/gl_image.h#newcode59
> ui/gl/gl_image.h:59: const RectF& crop_rect) = 0;
> On 2014/07/30 20:11:42, piman (slow to review) wrote:
>
>> I'm not sure how this API works without extra context... E.g. how do
>>
> we choose
>
>> which window or screen this is going to go to.
>> Shouldn't there be some connection with the GLSurface, since that's
>>
> what gets
>
>> the SwapBuffers, which is what activates the overlay?
>>
>
>  What is the semantics wrt statefulness? Is an image bound to an
>>
> overlay from the
>
>> next swap until another image replaces it a the same z index? Or is
>>
> the
>
>> expectation that at each frame the full set of overlays is
>>
> respecified? If the
>
>> former, how do you disable an overlay?
>>
>
> Re: extra context, I just commented on that before I saw your message.
> We'll need the current GLSurface's AcceleratedWidget to get that. We can
> add that API to the surface now or address as a followup.
>

Maybe I'm just missing out on how this is supposed to work (because we have
no implementation), but, if the AcceleratedWidget is enough to connect the
dots, shouldn't it be part of this new API?
Note that not all GLSurfaces have an AcceleratedWidget.


>
> Re: statefullness, the expectation is that the full set of overlays is
> respecified on each swap.
>

It's really worth specifying this. As is, the relationship between the
GLImage (which schedules the overlays) and the GLSurface (which commits the
overlays) is not clear at all.


> https://codereview.chromium.org/415283002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[alexst (slow to review), 2014-07-31 00:27:57]

> Maybe I'm just missing out on how this is supposed to work (because we have
> no implementation), but, if the AcceleratedWidget is enough to connect the
> dots, shouldn't it be part of this new API?
> Note that not all GLSurfaces have an AcceleratedWidget.

We've had a lot of discussions and approaches on how to plumb this best and
settled on adding overlay functionality to GLImage as the cleanest way to hide
platform specifics, global maps, #defines and such. The one bit we haven't
worked out fully was the display the overlay should go to, that I suggested we
could add as a followup. Thinking was getting the AcceleratedWidget from the
current surface, and kNullAcceleratedWidget for those that don't have it.

On the other hand, you bring up an good point that I looked straight past,
probably because I've been staring at this for too long. :) Since
AcceleratedWidget is the key to getting the needed extra context, it's probably
worth plumbing it explicitly into GLImage overlay call here. In addition, this
will let us leave the API on GLSurface and implementation in GLES2Decoder
unchanged. 

With widget added to GLImage, the flow becomes:

GLES2Decoder schedules overlays on GLSurface (which makes it clear that surface
is the target for overlays and surface triggers the final commit)
Platform specific GLSurface implementation tells GLImage to schedule itself on
its AcceleratedWidget and disambiguates which display the plane should go on.

What do you guys think?

[piman, 2014-07-31 00:53:06]

On Wed, Jul 30, 2014 at 5:27 PM, <alexst@chromium.org> wrote:

> Maybe I'm just missing out on how this is supposed to work (because we have
>> no implementation), but, if the AcceleratedWidget is enough to connect the
>> dots, shouldn't it be part of this new API?
>> Note that not all GLSurfaces have an AcceleratedWidget.
>>
>
> We've had a lot of discussions and approaches on how to plumb this best and
> settled on adding overlay functionality to GLImage as the cleanest way to
> hide
> platform specifics, global maps, #defines and such. The one bit we haven't
> worked out fully was the display the overlay should go to, that I
> suggested we
> could add as a followup. Thinking was getting the AcceleratedWidget from
> the
> current surface, and kNullAcceleratedWidget for those that don't have it.
>
> On the other hand, you bring up an good point that I looked straight past,
> probably because I've been staring at this for too long. :) Since
> AcceleratedWidget is the key to getting the needed extra context, it's
> probably
> worth plumbing it explicitly into GLImage overlay call here. In addition,
> this
> will let us leave the API on GLSurface and implementation in GLES2Decoder
> unchanged.
>
> With widget added to GLImage, the flow becomes:
>
> GLES2Decoder schedules overlays on GLSurface (which makes it clear that
> surface
> is the target for overlays and surface triggers the final commit)
> Platform specific GLSurface implementation tells GLImage to schedule
> itself on
> its AcceleratedWidget and disambiguates which display the plane should go
> on.
>
> What do you guys think?
>

SGTM

>
> https://codereview.chromium.org/415283002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[reveman, 2014-07-31 01:07:59]

On 2014/07/31 00:27:57, alexst wrote:
> > Maybe I'm just missing out on how this is supposed to work (because we have
> > no implementation), but, if the AcceleratedWidget is enough to connect the
> > dots, shouldn't it be part of this new API?
> > Note that not all GLSurfaces have an AcceleratedWidget.
> 
> We've had a lot of discussions and approaches on how to plumb this best and
> settled on adding overlay functionality to GLImage as the cleanest way to hide
> platform specifics, global maps, #defines and such. The one bit we haven't
> worked out fully was the display the overlay should go to, that I suggested we
> could add as a followup. Thinking was getting the AcceleratedWidget from the
> current surface, and kNullAcceleratedWidget for those that don't have it.
> 
> On the other hand, you bring up an good point that I looked straight past,
> probably because I've been staring at this for too long. :) Since
> AcceleratedWidget is the key to getting the needed extra context, it's
probably
> worth plumbing it explicitly into GLImage overlay call here. In addition, this
> will let us leave the API on GLSurface and implementation in GLES2Decoder
> unchanged. 
> 
> With widget added to GLImage, the flow becomes:
> 
> GLES2Decoder schedules overlays on GLSurface (which makes it clear that
surface
> is the target for overlays and surface triggers the final commit)
> Platform specific GLSurface implementation tells GLImage to schedule itself on
> its AcceleratedWidget and disambiguates which display the plane should go on.
> 
> What do you guys think?

So Decoder::ScheduleOverlayPlane(int texture, ...) ->
GLSurface::ScheduleOverlayPlane(GLImage* image, ...) ->
GLImage::ScheduleOverlayPlane(AcceleratedWidget widget, ...)?

I don't have any major objections to that. I think it's a bit awkward that the
call chain goes from platform independent code (Decoder) to platform specific
code (GLSurfaceGLX/EGL/...) then back to platform independent code by using
AcceleratedWidget and calling GLImage::ScheduleOverlayPlane, which will then
enter platform specific code again..

[piman, 2014-07-31 01:32:34]

On Wed, Jul 30, 2014 at 6:07 PM, <reveman@chromium.org> wrote:

> On 2014/07/31 00:27:57, alexst wrote:
>
>> > Maybe I'm just missing out on how this is supposed to work (because we
>> have
>> > no implementation), but, if the AcceleratedWidget is enough to connect
>> the
>> > dots, shouldn't it be part of this new API?
>> > Note that not all GLSurfaces have an AcceleratedWidget.
>>
>
>  We've had a lot of discussions and approaches on how to plumb this best
>> and
>> settled on adding overlay functionality to GLImage as the cleanest way to
>> hide
>> platform specifics, global maps, #defines and such. The one bit we haven't
>> worked out fully was the display the overlay should go to, that I
>> suggested we
>> could add as a followup. Thinking was getting the AcceleratedWidget from
>> the
>> current surface, and kNullAcceleratedWidget for those that don't have it.
>>
>
>  On the other hand, you bring up an good point that I looked straight past,
>> probably because I've been staring at this for too long. :) Since
>> AcceleratedWidget is the key to getting the needed extra context, it's
>>
> probably
>
>> worth plumbing it explicitly into GLImage overlay call here. In addition,
>> this
>> will let us leave the API on GLSurface and implementation in GLES2Decoder
>> unchanged.
>>
>
>  With widget added to GLImage, the flow becomes:
>>
>
>  GLES2Decoder schedules overlays on GLSurface (which makes it clear that
>>
> surface
>
>> is the target for overlays and surface triggers the final commit)
>> Platform specific GLSurface implementation tells GLImage to schedule
>> itself on
>> its AcceleratedWidget and disambiguates which display the plane should go
>> on.
>>
>
>  What do you guys think?
>>
>
> So Decoder::ScheduleOverlayPlane(int texture, ...) ->
> GLSurface::ScheduleOverlayPlane(GLImage* image, ...) ->
> GLImage::ScheduleOverlayPlane(AcceleratedWidget widget, ...)?
>
> I don't have any major objections to that. I think it's a bit awkward that
> the
> call chain goes from platform independent code (Decoder) to platform
> specific
> code (GLSurfaceGLX/EGL/...) then back to platform independent code by using
> AcceleratedWidget and calling GLImage::ScheduleOverlayPlane, which will
> then
> enter platform specific code again..
>

"platform specific" is on a slightly different axis though... There can be
multiple implementations of GLImage on the same platform.
Also, it's no different from e.g. GLContext::Initialize, where
implementations get passed a "platform independent" GLSurface and call into
it, so there is precedent.

I'm not fundamentally opposed to having Decoder::ScheduleOverlayPlane
directly call GLImage::ScheduleOverlayPlane if we believe
that AcceleratedWidget is enough to represent whatever we need to identify
the screen and what will cause the commit. Just add the parameter now
instead of in another CL.
That said, I'm not convinced it's what we need if one day we want to
support, say, Mac (think CALayers instead of HW overlays). Mac doesn't have
meaningful AcceleratedWidgets. GLSurface::ScheduleOverlayPlane does make
more sense in that case.


>
> https://codereview.chromium.org/415283002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[reveman, 2014-07-31 18:40:32]

Added widget to GLImage API and left GLSurface API as is. PTAL.

https://codereview.chromium.org/415283002/diff/40001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/415283002/diff/40001/ui/gl/gl_image.h#newcode48
ui/gl/gl_image.h:48: // Schedule image as an overlay plane to be shown at swap
time for |widget|.
Note: I think this comment is enough here. I'd rather not duplicate the text in
GL_CHROMIUM_schedule_overlay_plane.txt here.

[alexst (slow to review), 2014-07-31 19:08:24]

lgtm again, thank you!

[achaulk, 2014-07-31 19:21:46]

lgtm

[piman, 2014-07-31 19:21:51]

lgtm

[reveman, 2014-07-31 20:16:06]

The CQ bit was checked by reveman@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/reveman@chromium.org/415283002/40001

[commit-bot: I haz the power, 2014-08-01 00:22:12]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-01 14:39:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-01 14:39:20]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[reveman, 2014-08-01 14:43:17]

The CQ bit was checked by reveman@chromium.org

[alexst (slow to review), 2014-08-01 14:43:21]

The CQ bit was checked by alexst@chromium.org

[commit-bot: I haz the power, 2014-08-01 14:44:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/reveman@chromium.org/415283002/40001

[commit-bot: I haz the power, 2014-08-01 21:50:21]

Change committed as 287093