Mac: Always use surfaceless mode

Mac: Always use surfaceless mode

The goal of this change is to unify all image transport surfaces on Mac.

Prior to this, if we do not support remote CoreAnimation APIs, then we
will create an ImageTransportSurfaceFBO, which will send the IOSurface
that it creates in the GPU process to the browser process.

Change this to use the ImageTranportSurfaceOverlayMac. Using this
surface will trigger the use of
GpuSurfacelessBrowserCompositorOutputSurface instead of
GpuBrowserCompositorOutputSurface. This output surface allocates an
IOSurface-backed GpuMemoryBuffer in BufferQueue, which is rendered to.

When frames are swapped in ImageTranportSurfaceOverlayMac, and remote
CoreAnimation is not supported, send the backbuffer's IOSurface handle
to the browser in GpuHostMsg_AcceleratedSurfaceBuffersSwapped.

There is one snag here, where we are unable to open the IOSurface by
its handle if the GpuMemoryBuffer that was created by BufferQueue
has been destroyed. To fix this, make BufferQueue hold on to the
GpuMemoryBuffer while it may be in use.

Also, in GpuTransportFactory's CreateOverlayCandidateValidator, only
create a validator if the remote CoreAnimation API is supported, because
the non-remote-CoreAnimation path only supports drawing a single layer.

BUG=546795
Committed: https://crrev.com/516ddd741c0f9ec54947f4b23082e8c47d8d2278
Cr-Commit-Position: refs/heads/master@{#356301}

[ccameron, 2015-10-22 22:16:48]

ccameron@chromium.org changed reviewers:
+ kbr@chromium.org

[ccameron, 2015-10-22 22:16:49]

This is the second big part of the "kill it with fire" patch. This kills off
almost all of the divergent rendering paths in the image transport surface.

The next step will be to kill off various ancillary stuff that was only there to
support now-gone paths.

https://codereview.chromium.org/1416363002/diff/20001/content/browser/composi...
File content/browser/compositor/buffer_queue.h (right):

https://codereview.chromium.org/1416363002/diff/20001/content/browser/composi...
content/browser/compositor/buffer_queue.h:66: ~AllocatedSurface();
This is the part where we keep the GMB around while the surface is being tracked
by the browser, so that its IOSurface can be opened when the swap comes back
from the GPU process.

https://codereview.chromium.org/1416363002/diff/20001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

https://codereview.chromium.org/1416363002/diff/20001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host_ui_shim.cc:265:
gfx::GenericSharedMemoryId(io_surface_id)));
Ensuring that this AcquireIOSurface works is the reason why we keep the GMB
handle open in the browser.

https://codereview.chromium.org/1416363002/diff/20001/ui/accelerated_widget_m...
File ui/accelerated_widget_mac/accelerated_widget_mac.h (right):

https://codereview.chromium.org/1416363002/diff/20001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/accelerated_widget_mac.h:156: void
AcceleratedWidgetMacGotAcceleratedFrame(
This has some overlay and redundancy with AcceleratedWidgetGotIOSurfaceFrame.
There is actually quite a bit that can be ripped out here (drawn_callback,
disable_throttling, renderer_id, and flip_y, for starters). I'll have a
follow-up CL that just deletes now-dead code (I tried to keep the functional
changes in this one).

[Ken Russell (switch to Gerrit), 2015-10-22 23:39:49]

Could you please reference a bug ID? This is large enough that it's likely it
may land and be reverted a couple of times.

[ccameron, 2015-10-22 23:48:53]

Description was changed from

==========
Mac: Always use surfaceless mode

The goal of this change is to unify all image transport surfaces on Mac.

Prior to this, if we do not support remote CoreAnimation APIs, then we
will create an ImageTransportSurfaceFBO, which will send the IOSurface
that it creates in the GPU process to the browser process.

Change this to use the ImageTranportSurfaceOverlayMac. Using this
surface will trigger the use of
GpuSurfacelessBrowserCompositorOutputSurface instead of
GpuBrowserCompositorOutputSurface. This output surface allocates an
IOSurface-backed GpuMemoryBuffer in BufferQueue, which is rendered to.

When frames are swapped in ImageTranportSurfaceOverlayMac, and remote
CoreAnimation is not supported, send the backbuffer's IOSurface handle
to the browser in GpuHostMsg_AcceleratedSurfaceBuffersSwapped.

There is one snag here, where we are unable to open the IOSurface by
its handle if the GpuMemoryBuffer that was created by BufferQueue
has been destroyed. To fix this, make BufferQueue hold on to the
GpuMemoryBuffer while it may be in use.

Also, in GpuTransportFactory's CreateOverlayCandidateValidator, only
create a validator if the remote CoreAnimation API is supported, because
the non-remote-CoreAnimation path only supports drawing a single layer.

BUG=
==========

to

==========
Mac: Always use surfaceless mode

The goal of this change is to unify all image transport surfaces on Mac.

Prior to this, if we do not support remote CoreAnimation APIs, then we
will create an ImageTransportSurfaceFBO, which will send the IOSurface
that it creates in the GPU process to the browser process.

Change this to use the ImageTranportSurfaceOverlayMac. Using this
surface will trigger the use of
GpuSurfacelessBrowserCompositorOutputSurface instead of
GpuBrowserCompositorOutputSurface. This output surface allocates an
IOSurface-backed GpuMemoryBuffer in BufferQueue, which is rendered to.

When frames are swapped in ImageTranportSurfaceOverlayMac, and remote
CoreAnimation is not supported, send the backbuffer's IOSurface handle
to the browser in GpuHostMsg_AcceleratedSurfaceBuffersSwapped.

There is one snag here, where we are unable to open the IOSurface by
its handle if the GpuMemoryBuffer that was created by BufferQueue
has been destroyed. To fix this, make BufferQueue hold on to the
GpuMemoryBuffer while it may be in use.

Also, in GpuTransportFactory's CreateOverlayCandidateValidator, only
create a validator if the remote CoreAnimation API is supported, because
the non-remote-CoreAnimation path only supports drawing a single layer.

BUG=546795
==========

[ccameron, 2015-10-22 23:49:03]

On 2015/10/22 23:39:49, Ken Russell wrote:
> Could you please reference a bug ID? This is large enough that it's likely it
> may land and be reverted a couple of times.

Filed crbug.com/546795 on this cleanup.

[Ken Russell (switch to Gerrit), 2015-10-23 01:13:13]

kbr@chromium.org changed reviewers:
+ danakj@chromium.org, enne@chromium.org, reveman@chromium.org

[Ken Russell (switch to Gerrit), 2015-10-23 01:13:13]

The cleanup and code deletion looks awesome.

Could you please ensure the following two cases work:

1) Run with --enable-unsafe-es3-apis, make sure that
http://toji.github.io/webgl2-particles/ displays "WebGL 2", and correctly

2) Run content/test/gpu/run_gpu_test.py --browser=release pixel
and make sure that the ES3 tests run correctly locally.

Thanks. LGTM overall for any parts I'm an owner for.

https://codereview.chromium.org/1416363002/diff/20001/content/browser/composi...
File content/browser/compositor/buffer_queue.h (right):

https://codereview.chromium.org/1416363002/diff/20001/content/browser/composi...
content/browser/compositor/buffer_queue.h:66: ~AllocatedSurface();
On 2015/10/22 22:16:49, ccameron wrote:
> This is the part where we keep the GMB around while the surface is being
tracked
> by the browser, so that its IOSurface can be opened when the swap comes back
> from the GPU process.

I'm not qualified to review this change; suggest danakj@, enne@ or reveman@.

https://codereview.chromium.org/1416363002/diff/20001/ui/accelerated_widget_m...
File ui/accelerated_widget_mac/accelerated_widget_mac.h (right):

https://codereview.chromium.org/1416363002/diff/20001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/accelerated_widget_mac.h:83: void
GotAcceleratedFrame(CAContextID ca_context_id,
Could you document that |ca_context_id| and |io_surface| are mutually exclusive
arguments?

https://codereview.chromium.org/1416363002/diff/20001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/accelerated_widget_mac.h:156: void
AcceleratedWidgetMacGotAcceleratedFrame(
On 2015/10/22 22:16:49, ccameron wrote:
> This has some overlay and redundancy with AcceleratedWidgetGotIOSurfaceFrame.
> There is actually quite a bit that can be ripped out here (drawn_callback,
> disable_throttling, renderer_id, and flip_y, for starters). I'll have a
> follow-up CL that just deletes now-dead code (I tried to keep the functional
> changes in this one).

Sounds good.

[ccameron, 2015-10-23 07:49:44]

On 2015/10/23 01:13:13, Ken Russell wrote:
> The cleanup and code deletion looks awesome.
> 
> Could you please ensure the following two cases work:
> 
> 1) Run with --enable-unsafe-es3-apis, make sure that
> http://toji.github.io/webgl2-particles/ displays "WebGL 2", and correctly
> 
> 2) Run content/test/gpu/run_gpu_test.py --browser=release pixel
> and make sure that the ES3 tests run correctly locally.

Thanks! These appear to be working.

Adding reveman@ to take a look at the BufferQueue and GMB changes

[danakj, 2015-10-23 19:52:08]

https://codereview.chromium.org/1416363002/diff/20001/content/browser/composi...
File content/browser/compositor/buffer_queue.h (right):

https://codereview.chromium.org/1416363002/diff/20001/content/browser/composi...
content/browser/compositor/buffer_queue.h:66: ~AllocatedSurface();
On 2015/10/23 01:13:13, Ken Russell wrote:
> On 2015/10/22 22:16:49, ccameron wrote:
> > This is the part where we keep the GMB around while the surface is being
> tracked
> > by the browser, so that its IOSurface can be opened when the swap comes back
> > from the GPU process.
> 
> I'm not qualified to review this change; suggest danakj@, enne@ or reveman@.

Ya reveman or some of the ozone ppls would be good. (Normally buffer queue
changes come with unit tests?)

[alexst (slow to review), 2015-10-23 19:59:09]

alexst@chromium.org changed reviewers:
+ alexst@chromium.org

[alexst (slow to review), 2015-10-23 19:59:10]

buffer_queue lgtm

https://codereview.chromium.org/1416363002/diff/20001/content/browser/composi...
File content/browser/compositor/buffer_queue.h (right):

https://codereview.chromium.org/1416363002/diff/20001/content/browser/composi...
content/browser/compositor/buffer_queue.h:66: ~AllocatedSurface();
On 2015/10/23 19:52:08, danakj wrote:
> On 2015/10/23 01:13:13, Ken Russell wrote:
> > On 2015/10/22 22:16:49, ccameron wrote:
> > > This is the part where we keep the GMB around while the surface is being
> > tracked
> > > by the browser, so that its IOSurface can be opened when the swap comes
back
> > > from the GPU process.
> > 
> > I'm not qualified to review this change; suggest danakj@, enne@ or reveman@.
> 
> Ya reveman or some of the ozone ppls would be good. (Normally buffer queue
> changes come with unit tests?)

This should be fine, the reason we didn't hold onto the reference for the buffer
is that as soon as you attach it to a gl_image, it refs it internally in the GPU
process and since we only ever needed the gl_image for chrome os, we let the
buffer object go. If the original buffer is required for OSX, I see nothing
wrong with that.

[reveman, 2015-10-23 20:44:40]

https://codereview.chromium.org/1416363002/diff/20001/content/browser/composi...
File content/browser/compositor/buffer_queue.h (right):

https://codereview.chromium.org/1416363002/diff/20001/content/browser/composi...
content/browser/compositor/buffer_queue.h:67: linked_ptr<gfx::GpuMemoryBuffer>
buffer;
Can we avoid having these be reference counted?

https://codereview.chromium.org/1416363002/diff/20001/content/common/gpu/imag...
File content/common/gpu/image_transport_surface_overlay_mac.mm (right):

https://codereview.chromium.org/1416363002/diff/20001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:663: bool
ImageTransportSurfaceOverlayMac::ScheduleOverlayPlane(
Can you remind me what the call stack for this function typically looks like? Is
this going through GLImageIOSurface before this is called? Not need to change
anything in this patch, I just like to be sure that I understand why the static
cast below is safe..

[ccameron, 2015-10-26 22:28:11]

Rebased and updated.

https://codereview.chromium.org/1416363002/diff/20001/content/browser/composi...
File content/browser/compositor/buffer_queue.h (right):

https://codereview.chromium.org/1416363002/diff/20001/content/browser/composi...
content/browser/compositor/buffer_queue.h:67: linked_ptr<gfx::GpuMemoryBuffer>
buffer;
On 2015/10/23 20:44:40, reveman wrote:
> Can we avoid having these be reference counted?

Yes, but that will require making the various instances of AllocatedSurface be
scoped_ptr, which is a bigger refactor than I want to put into this patch. The
refactor is a good one (in particular, we check |texture| when a null check
would be better, so I'm much more of a fan of the newer version), but it has a
high chance of breaking stuff. I've put a TODO and I'll put up the new patch in
a bit.

I'd prefer to do this via a linked_ptr in the mean time.

https://codereview.chromium.org/1416363002/diff/20001/content/common/gpu/imag...
File content/common/gpu/image_transport_surface_overlay_mac.mm (right):

https://codereview.chromium.org/1416363002/diff/20001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_overlay_mac.mm:663: bool
ImageTransportSurfaceOverlayMac::ScheduleOverlayPlane(
On 2015/10/23 20:44:40, reveman wrote:
> Can you remind me what the call stack for this function typically looks like?
Is
> this going through GLImageIOSurface before this is called? Not need to change
> anything in this patch, I just like to be sure that I understand why the
static
> cast below is safe..

I think that this needs to be made safer as well -- it appears that it's only
safe by the coincidence that we only we only schedule overlays for
GLImageIOSurfaces on Mac (which we could easily screw up in the future).

I'll spin up a new patch to clean this up.

[ccameron, 2015-10-26 23:49:38]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2015-10-26 23:49:38]

The patchset sent to the CQ was uploaded after l-g-t-m from alexst@chromium.org,
kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/1416363002/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2015-10-26 23:50:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1416363002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1416363002/40001

[commit-bot: I haz the power, 2015-10-27 01:00:27]

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

[commit-bot: I haz the power, 2015-10-27 01:00:28]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[ccameron, 2015-10-27 07:34:27]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-10-27 07:34:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1416363002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1416363002/40001

[commit-bot: I haz the power, 2015-10-27 08:20:25]

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

[commit-bot: I haz the power, 2015-10-27 08:20:26]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Ken Russell (switch to Gerrit), 2015-10-27 12:41:40]

On 2015/10/27 08:20:26, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

It looks like an export of the AllocatedSurface class is missing in the
component build.

[ccameron, 2015-10-27 14:27:54]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2015-10-27 14:27:55]

The patchset sent to the CQ was uploaded after l-g-t-m from alexst@chromium.org,
kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/1416363002/#ps60001
(title: "Add export")

[ccameron, 2015-10-27 14:28:26]

On 2015/10/27 12:41:40, Ken Russell wrote:
> On 2015/10/27 08:20:26, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
> 
> It looks like an export of the AllocatedSurface class is missing in the
> component build.

Yeah, added a CONTENT_EXPORT (odd that it only affected Windows ... I do
component locally on Mac).

[commit-bot: I haz the power, 2015-10-27 14:28:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1416363002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1416363002/60001

[commit-bot: I haz the power, 2015-10-27 15:28:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-27 15:29:26]

Patchset 4 (id:??) landed as
https://crrev.com/516ddd741c0f9ec54947f4b23082e8c47d8d2278
Cr-Commit-Position: refs/heads/master@{#356301}