mus+ash: Enable Chrome HW rendering in mus+ash

mus+ash: Enable Chrome HW rendering in mus+ash

This change enables the HW supports in mus+ash by
 * Wire up ui service GPU channel.
 * Always create offscreen GL context for gfx::AcceleratedWidget (mus window) in
   browser process.
 * Provide a MusBrowserCompositorOutputSurface.
   In MusBrowserCompositorOutputSurface::SwapBuffers(), we use
   CommandBufferProxyImpl::TakeFrontBuffer() to take the offscreen GL context's
   front buffer into a mailbox, insert a sync token, and send mailbox+sync to
   the ui service process.

Know issue:
 GPUInfo is not sent to renderer, so some webgl demos don't work.

BUG=622708
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/e1d86515496285e3b5f593fa79d72daec6e64feb
Cr-Commit-Position: refs/heads/master@{#404441}

[Peng, 2016-06-24 19:25:12]

Description was changed from

==========
WIP


WIP


HW RENDERING


WIP


WIP


WIP


WIP


WIP


WIP


WIP


WIP


WIP


WIP


WIP


GpuService Refactory

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

to

==========
WIP


WIP


HW RENDERING


WIP


WIP


WIP


WIP


WIP


WIP


WIP


WIP


WIP


WIP


WIP


GpuService Refactory

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Peng, 2016-06-27 16:18:35]

Description was changed from

==========
WIP


WIP


HW RENDERING


WIP


WIP


WIP


WIP


WIP


WIP


WIP


WIP


WIP


WIP


WIP


GpuService Refactory

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
mus+ash: Enable HW renderering in mus+ash

This change enables the HW supports in mus+ash.
Know issues:
 FPS of webgl is very low.
 GPUInfo is not sent to renderer, so some webgl demos don't work.

BUG=622708
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Peng, 2016-06-27 16:22:34]

penghuang@chromium.org changed reviewers:
+ jbauman@chromium.org, piman@chromium.org

[Peng, 2016-06-27 16:22:34]

piman@ for //content/*
jbauman@ for //cc/surfaces/*

Hi Antoine & John, PTAL. Thanks.

[Peng, 2016-06-27 17:58:24]

Description was changed from

==========
mus+ash: Enable HW renderering in mus+ash

This change enables the HW supports in mus+ash.
Know issues:
 FPS of webgl is very low.
 GPUInfo is not sent to renderer, so some webgl demos don't work.

BUG=622708
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
mus+ash: Enable HW rendering in mus+ash

This change enables the HW supports in mus+ash.
Know issues:
 FPS of WebGL is very low.
 GPUInfo is not sent to renderer, so some webgl demos don't work.


BUG=622708
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Peng, 2016-06-27 18:20:14]

Description was changed from

==========
mus+ash: Enable HW rendering in mus+ash

This change enables the HW supports in mus+ash.
Know issues:
 FPS of WebGL is very low.
 GPUInfo is not sent to renderer, so some webgl demos don't work.


BUG=622708
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
mus+ash: Enable HW rendering in mus+ash

This change enables the HW supports in mus+ash.
Know issues:
 FPS of WebGL is a little low.
 GPUInfo is not sent to renderer, so some webgl demos don't work.


BUG=622708
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[danakj, 2016-06-27 18:48:38]

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

[danakj, 2016-06-27 18:48:39]

https://codereview.chromium.org/2096843002/diff/90001/cc/surfaces/display.cc
File cc/surfaces/display.cc (right):

https://codereview.chromium.org/2096843002/diff/90001/cc/surfaces/display.cc#...
cc/surfaces/display.cc:172: std::unique_ptr<DelegatingRenderer> renderer =
DelegatingRenderer::Create(
I don't understand this, and am trying to do the opposite in other parts of the
code. Why would a Display do delegated rendering?

[danakj, 2016-06-27 18:57:29]

https://codereview.chromium.org/2096843002/diff/90001/cc/surfaces/display.cc
File cc/surfaces/display.cc (right):

https://codereview.chromium.org/2096843002/diff/90001/cc/surfaces/display.cc#...
cc/surfaces/display.cc:172: std::unique_ptr<DelegatingRenderer> renderer =
DelegatingRenderer::Create(
On 2016/06/27 18:48:39, danakj wrote:
> I don't understand this, and am trying to do the opposite in other parts of
the
> code. Why would a Display do delegated rendering?

re: offline explanation.

I don't think there should be a Display on the delegating side. There should
just be a LayerTreeHostImpl sending frames to mus like in the renderer process.

So there should be no BrowserCompositorOutputSurface at all, instead there can
be a mus output surface like we have in the renderer, that is given to the
ui::Compositor instead of the SurfaceDisplayOutputSurface.

The outcome I'm expecting from mus is to have the browser process look more like
the renderer.

[danakj, 2016-06-27 21:28:48]

After a bunch of discussion I now understand that mus contexts do not support
onscreen views, which is why the Display is being delegated here. Because that
introduces a bunch of dependencies on Renderer being direct/delegating which is
the thing I'm working on deleting now, we considered maybe having the Display
draw to a texture and send that to the mus process instead. It would mean a
different OutputSurface given to the Display that deals with this transport, and
is a fair bit of work that will end up deleted when the ui::Compositor uses mus
to send frames out of process instead. But maybe the simplest way if we want to
use a mus context/command buffer right now? Unless it's easier to make mus
contexts support on-screen.

https://codereview.chromium.org/2096843002/diff/90001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2096843002/diff/90001/content/renderer/render...
content/renderer/render_thread_impl.cc:640: auto shell_connection =
MojoShellConnection::GetForProcess();
auto*

https://codereview.chromium.org/2096843002/diff/90001/content/renderer/render...
content/renderer/render_thread_impl.cc:1618: return
mus::GpuService::GetInstance()->gpu_memory_buffer_manager();
Why isn't gpu_memory_buffer_manager() the mus one? That means we have 2.

https://codereview.chromium.org/2096843002/diff/90001/content/renderer/render...
content/renderer/render_thread_impl.cc:1797: return NULL;
nullptr

https://codereview.chromium.org/2096843002/diff/90001/content/renderer/render...
content/renderer/render_thread_impl.cc:1810: gpu_channel_ =
mus::GpuService::GetInstance()->EstablishGpuChannelSync();
Would it be possible to have a non-mash in-process mus service that does the
above and makes the oldschool GpuChannelHost, so this code always goes through
mus?

[fadysamuel, 2016-06-27 23:45:12]

fadysamuel@gmail.com changed reviewers:
+ fadysamuel@gmail.com, sadrul@chromium.org

[fadysamuel, 2016-06-27 23:45:13]

+sadrul@: I think we should take the high road here and submit CompositorFrames
directly to mus from ui::Compositor and the renderer. This avoids an effort in
the opposite direction of Dana's work. This requires stabilizing the
--use-mus-in-renderer code path. WDYT?

[danakj, 2016-06-28 00:09:03]

On Mon, Jun 27, 2016 at 4:45 PM, <fadysamuel@gmail.com> wrote:

> +sadrul@: I think we should take the high road here and submit
> CompositorFrames
> directly to mus from ui::Compositor and the renderer. This avoids an
> effort in
> the opposite direction of Dana's work. This requires stabilizing the
> --use-mus-in-renderer code path. WDYT?
>

FWIW it sounds like we will need to support on-screen mus command buffers
anyhow for some time, for the display compositor. So we may as well make
the mus contexts support this, and then this patch doesn't need to change
Display or how it works at all in this step. At least, that may be a more
simple/parallel way forward.


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

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peng, 2016-06-30 19:30:52]

https://codereview.chromium.org/2096843002/diff/90001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2096843002/diff/90001/content/renderer/render...
content/renderer/render_thread_impl.cc:640: auto shell_connection =
MojoShellConnection::GetForProcess();
On 2016/06/27 21:28:48, danakj wrote:
> auto*

Done.

https://codereview.chromium.org/2096843002/diff/90001/content/renderer/render...
content/renderer/render_thread_impl.cc:1618: return
mus::GpuService::GetInstance()->gpu_memory_buffer_manager();
On 2016/06/27 21:28:48, danakj wrote:
> Why isn't gpu_memory_buffer_manager() the mus one? That means we have 2.

Done.

https://codereview.chromium.org/2096843002/diff/90001/content/renderer/render...
content/renderer/render_thread_impl.cc:1797: return NULL;
On 2016/06/27 21:28:47, danakj wrote:
> nullptr

Done.

https://codereview.chromium.org/2096843002/diff/90001/content/renderer/render...
content/renderer/render_thread_impl.cc:1810: gpu_channel_ =
mus::GpuService::GetInstance()->EstablishGpuChannelSync();
On 2016/06/27 21:28:48, danakj wrote:
> Would it be possible to have a non-mash in-process mus service that does the
> above and makes the oldschool GpuChannelHost, so this code always goes through
> mus?

It is possible. But currently some features are not implemented in the mus
GpuServer (WatchDog, ShaderCache, etc). We have to implement all those features
first, and then switch to the mus gpu service.

[Peng, 2016-06-30 19:50:55]

https://codereview.chromium.org/2096843002/diff/90001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2096843002/diff/90001/content/renderer/render...
content/renderer/render_thread_impl.cc:1618: return
mus::GpuService::GetInstance()->gpu_memory_buffer_manager();
Yeah. Unfortunately, mus::GpuService has his own buffer manager. To unify them,
I think we have to refactor many code. I will do it in following CLs.

[Fady Samuel, 2016-06-30 20:28:03]

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

[Fady Samuel, 2016-06-30 20:28:03]

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

[Fady Samuel, 2016-06-30 20:28:04]

https://codereview.chromium.org/2096843002/diff/130001/content/browser/compos...
File content/browser/compositor/mus_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2096843002/diff/130001/content/browser/compos...
content/browser/compositor/mus_browser_compositor_output_surface.cc:87:
GetCommandBufferProxy()->TakeFrontBuffer(mailbox);
Question for my own edification: Does this mean we pull the FBO output of
GLRenderer and stick a reference to it in a TextureDrawQuad and give it to Mus?

[Fady Samuel, 2016-06-30 20:28:04]

https://codereview.chromium.org/2096843002/diff/130001/content/browser/compos...
File content/browser/compositor/mus_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2096843002/diff/130001/content/browser/compos...
content/browser/compositor/mus_browser_compositor_output_surface.cc:87:
GetCommandBufferProxy()->TakeFrontBuffer(mailbox);
Question for my own edification: Does this mean we pull the FBO output of
GLRenderer and stick a reference to it in a TextureDrawQuad and give it to Mus?

[Peng, 2016-06-30 20:49:07]

https://codereview.chromium.org/2096843002/diff/130001/content/browser/compos...
File content/browser/compositor/mus_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2096843002/diff/130001/content/browser/compos...
content/browser/compositor/mus_browser_compositor_output_surface.cc:87:
GetCommandBufferProxy()->TakeFrontBuffer(mailbox);
On 2016/06/30 20:28:04, Fady Samuel wrote:
> Question for my own edification: Does this mean we pull the FBO output of
> GLRenderer and stick a reference to it in a TextureDrawQuad and give it to
Mus?

As my understanding, for the chrome offscreen chrome gl context, we draw on one
back buffer (FBO), when the SwapBuffers() is called, we copy it into a front
buffer by default. The TakeFrontBuffer() will borrow the texture of the front
buffer from the context, and return if when it is not used anymore. See [1] &
[2] for detail. But we can override the behavior by using
attributes.buffer_preserved = false when create the context. But I am not sure
why we use attributes.buffer_preserved = true by default, it will cause an extra
texture copy I think.

[1]
https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gles2_cmd_dec...
[2]
https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gles2_cmd_dec...

[piman, 2016-06-30 20:56:32]

https://codereview.chromium.org/2096843002/diff/130001/content/browser/compos...
File content/browser/compositor/mus_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2096843002/diff/130001/content/browser/compos...
content/browser/compositor/mus_browser_compositor_output_surface.cc:87:
GetCommandBufferProxy()->TakeFrontBuffer(mailbox);
On 2016/06/30 20:49:07, Peng wrote:
> On 2016/06/30 20:28:04, Fady Samuel wrote:
> > Question for my own edification: Does this mean we pull the FBO output of
> > GLRenderer and stick a reference to it in a TextureDrawQuad and give it to
> Mus?
> 
> As my understanding, for the chrome offscreen chrome gl context, we draw on
one
> back buffer (FBO), when the SwapBuffers() is called, we copy it into a front
> buffer by default. The TakeFrontBuffer() will borrow the texture of the front
> buffer from the context, and return if when it is not used anymore. See [1] &
> [2] for detail. But we can override the behavior by using
> attributes.buffer_preserved = false when create the context. But I am not sure
> why we use attributes.buffer_preserved = true by default, it will cause an
extra
> texture copy I think.

Yes, please, set it to false. The default is true because that's the EGL
default, but you most likely don't want the preserved behavior.

[Peng, 2016-07-04 19:23:14]

Patchset #11 (id:190001) has been deleted

[Peng, 2016-07-04 20:08:11]

On 2016/06/30 20:56:32, piman OOO back 2016-7-6 wrote:
>
https://codereview.chromium.org/2096843002/diff/130001/content/browser/compos...
> File content/browser/compositor/mus_browser_compositor_output_surface.cc
> (right):
> 
>
https://codereview.chromium.org/2096843002/diff/130001/content/browser/compos...
> content/browser/compositor/mus_browser_compositor_output_surface.cc:87:
> GetCommandBufferProxy()->TakeFrontBuffer(mailbox);
> On 2016/06/30 20:49:07, Peng wrote:
> > On 2016/06/30 20:28:04, Fady Samuel wrote:
> > > Question for my own edification: Does this mean we pull the FBO output of
> > > GLRenderer and stick a reference to it in a TextureDrawQuad and give it to
> > Mus?
> > 
> > As my understanding, for the chrome offscreen chrome gl context, we draw on
> one
> > back buffer (FBO), when the SwapBuffers() is called, we copy it into a front
> > buffer by default. The TakeFrontBuffer() will borrow the texture of the
front
> > buffer from the context, and return if when it is not used anymore. See [1]
&
> > [2] for detail. But we can override the behavior by using
> > attributes.buffer_preserved = false when create the context. But I am not
sure
> > why we use attributes.buffer_preserved = true by default, it will cause an
> extra
> > texture copy I think.
> 
> Yes, please, set it to false. The default is true because that's the EGL
> default, but you most likely don't want the preserved behavior.

Tried set it to fale, but chrome crashes at
https://cs.chromium.org/chromium/src/content/common/gpu/client/context_provid....

Can we remove this DCHECK?

[Fady Samuel, 2016-07-06 14:44:19]

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
File content/browser/compositor/mus_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/mus_browser_compositor_output_surface.cc:13: #include
"content/common/gpu/client/context_provider_command_buffer.h"
Is this moving out of content?

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/mus_browser_compositor_output_surface.cc:24:
MusBrowserCompositorOutputSurface::MusBrowserCompositorOutputSurface(
We are moving away from the name Mus it seems. I also wonder if this can live in
the services/ui/public directory once context_provider_command_buffer is
factored out?

[Peng, 2016-07-06 15:10:04]

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
File content/browser/compositor/mus_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/mus_browser_compositor_output_surface.cc:13: #include
"content/common/gpu/client/context_provider_command_buffer.h"
On 2016/07/06 14:44:19, Fady Samuel wrote:
> Is this moving out of content?

They cannot go to //gpu/ipc. Maybe they need a new folder in components.

https://bugs.chromium.org/p/chromium/issues/detail?id=586380#c15

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/mus_browser_compositor_output_surface.cc:24:
MusBrowserCompositorOutputSurface::MusBrowserCompositorOutputSurface(
On 2016/07/06 14:44:19, Fady Samuel wrote:
> We are moving away from the name Mus it seems. I also wonder if this can live
in
> the services/ui/public directory once context_provider_command_buffer is
> factored out?

This class inherit from GpuBrowserCompositorOutputSurface which is in content,
so it cannot be in service/ui/ I think.

[Peng, 2016-07-06 19:38:07]

Description was changed from

==========
mus+ash: Enable HW rendering in mus+ash

This change enables the HW supports in mus+ash.
Know issues:
 FPS of WebGL is a little low.
 GPUInfo is not sent to renderer, so some webgl demos don't work.


BUG=622708
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
mus+ash: Enable HW rendering in mus+ash

This change enables the HW supports in mus+ash.
Know issue:
 GPUInfo is not sent to renderer, so some webgl demos don't work.


BUG=622708
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[piman, 2016-07-06 21:03:32]

On Mon, Jul 4, 2016 at 1:08 PM, <penghuang@chromium.org> wrote:

> On 2016/06/30 20:56:32, piman OOO back 2016-7-6 wrote:
> >
>
>
https://codereview.chromium.org/2096843002/diff/130001/content/browser/compos...
> > File content/browser/compositor/mus_browser_compositor_output_surface.cc
> > (right):
> >
> >
>
>
https://codereview.chromium.org/2096843002/diff/130001/content/browser/compos...
> > content/browser/compositor/mus_browser_compositor_output_surface.cc:87:
> > GetCommandBufferProxy()->TakeFrontBuffer(mailbox);
> > On 2016/06/30 20:49:07, Peng wrote:
> > > On 2016/06/30 20:28:04, Fady Samuel wrote:
> > > > Question for my own edification: Does this mean we pull the FBO
> output of
> > > > GLRenderer and stick a reference to it in a TextureDrawQuad and give
> it to
> > > Mus?
> > >
> > > As my understanding, for the chrome offscreen chrome gl context, we
> draw on
> > one
> > > back buffer (FBO), when the SwapBuffers() is called, we copy it into a
> front
> > > buffer by default. The TakeFrontBuffer() will borrow the texture of the
> front
> > > buffer from the context, and return if when it is not used anymore.
> See [1]
> &
> > > [2] for detail. But we can override the behavior by using
> > > attributes.buffer_preserved = false when create the context. But I am
> not
> sure
> > > why we use attributes.buffer_preserved = true by default, it will
> cause an
> > extra
> > > texture copy I think.
> >
> > Yes, please, set it to false. The default is true because that's the EGL
> > default, but you most likely don't want the preserved behavior.
>
> Tried set it to fale, but chrome crashes at
>
>
https://cs.chromium.org/chromium/src/content/common/gpu/client/context_provid...
> .
>
> Can we remove this DCHECK?
>

Yes, we can remove that DCHECK.


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

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peng, 2016-07-07 19:01:12]

Description was changed from

==========
mus+ash: Enable HW rendering in mus+ash

This change enables the HW supports in mus+ash.
Know issue:
 GPUInfo is not sent to renderer, so some webgl demos don't work.


BUG=622708
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
mus+ash: Enable HW rendering in mus+ash


This change enables the HW supports in mus+ash by
 * Wire up ui service GPU channel.
 * Always create offscreen GL context for gfx::AcceleratedWidget (mus window).
 * Provide a MusBrowserCompositorOutputSurface.
   In MusBrowserCompositorOutputSurface::SwapBuffers(), we use  
   CommandBufferProxyImpl::TakeFrontBuffer() to take the offscreen GL context's
   front buffer into a mailbox, insert a sync token, and send mailbox+sync to
   the ui service process.

Know issue:
 GPUInfo is not sent to renderer, so some webgl demos don't work.


BUG=622708
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Peng, 2016-07-07 19:02:57]

Description was changed from

==========
mus+ash: Enable HW rendering in mus+ash


This change enables the HW supports in mus+ash by
 * Wire up ui service GPU channel.
 * Always create offscreen GL context for gfx::AcceleratedWidget (mus window).
 * Provide a MusBrowserCompositorOutputSurface.
   In MusBrowserCompositorOutputSurface::SwapBuffers(), we use  
   CommandBufferProxyImpl::TakeFrontBuffer() to take the offscreen GL context's
   front buffer into a mailbox, insert a sync token, and send mailbox+sync to
   the ui service process.

Know issue:
 GPUInfo is not sent to renderer, so some webgl demos don't work.


BUG=622708
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
mus+ash: Enable HW rendering in mus+ash


This change enables the HW supports in mus+ash by
 * Wire up ui service GPU channel.
 * Always create offscreen GL context for gfx::AcceleratedWidget (mus window) in
   browser process.
 * Provide a MusBrowserCompositorOutputSurface.
   In MusBrowserCompositorOutputSurface::SwapBuffers(), we use  
   CommandBufferProxyImpl::TakeFrontBuffer() to take the offscreen GL context's
   front buffer into a mailbox, insert a sync token, and send mailbox+sync to
   the ui service process.

Know issue:
 GPUInfo is not sent to renderer, so some webgl demos don't work.


BUG=622708
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Peng, 2016-07-07 19:08:06]

Description was changed from

==========
mus+ash: Enable HW rendering in mus+ash


This change enables the HW supports in mus+ash by
 * Wire up ui service GPU channel.
 * Always create offscreen GL context for gfx::AcceleratedWidget (mus window) in
   browser process.
 * Provide a MusBrowserCompositorOutputSurface.
   In MusBrowserCompositorOutputSurface::SwapBuffers(), we use  
   CommandBufferProxyImpl::TakeFrontBuffer() to take the offscreen GL context's
   front buffer into a mailbox, insert a sync token, and send mailbox+sync to
   the ui service process.

Know issue:
 GPUInfo is not sent to renderer, so some webgl demos don't work.


BUG=622708
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
mus+ash: Enable Chrome HW rendering in mus+ash


This change enables the HW supports in mus+ash by
 * Wire up ui service GPU channel.
 * Always create offscreen GL context for gfx::AcceleratedWidget (mus window) in
   browser process.
 * Provide a MusBrowserCompositorOutputSurface.
   In MusBrowserCompositorOutputSurface::SwapBuffers(), we use  
   CommandBufferProxyImpl::TakeFrontBuffer() to take the offscreen GL context's
   front buffer into a mailbox, insert a sync token, and send mailbox+sync to
   the ui service process.

Know issue:
 GPUInfo is not sent to renderer, so some webgl demos don't work.


BUG=622708
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[danakj, 2016-07-07 20:16:41]

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:138: // For mus, we
need the alpha channel for "onscreen" context.
For mus the context is not onscreen anymore, so the comments above wrong. Can
you fix them instead?

Here's some suggestions

".. and by the display compositor (onscreen, except when using mus) for actual
GL drawing"

"The display compositor does not use these things either (except for alpha
channel when using mus for non-opaque ui that overlaps the system's window
borders), so we can request only that when needed."

Is that right what I wrote?

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:149: // For mus, we
create offscreen context.
"an offscreen context"

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:150: surface_handle
= IsUsingMus() ? gpu::kNullSurfaceHandle : surface_handle;
As the comment above says, this code is used for creating more than one context.
I think this code should be at a callsite for this method, not inside the common
code.

Same for the above IsUsingMus(), this method should take parameters for things
it wants to be optional like "needs_alpha_channel".

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:272: // Chrome
running as a mojo app currently can only use software compositing.
This comment is wrong now?

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:536: #if
defined(MOJO_RUNNER_CLIENT)
Why are things inside #if defined, but the source files are always present in
the build?

[piman, 2016-07-07 21:02:21]

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:323: } else {
nit: I would prefer to avoid mixing compile-time #ifdefs and run-time brackets -
it makes the code really hard to maintain.

How about:

    if (!use_mus) {
      BrowserGpuChannelHostFactory::instance()->EstablishGpuChannel(
          CAUSE_FOR_GPU_LAUNCH_SHARED_WORKER_THREAD_CONTEXT, callback);
    } else {
#if defined(MOJO_RUNNER_CLIENT)
      ui::GpuService::GetInstance()->EstablishGpuChannel(callback);
#else
      NOTREACHED();
#endif
    }

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:388: } else {
nit: ditto

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:456: } else {
nit: ditto

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:788: } else {
nit: ditto

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
File content/browser/compositor/mus_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/mus_browser_compositor_output_surface.cc:50:
ui_frame.metadata.device_scale_factor = 1.0f;
Should you get the metadata from |frame| ?

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/mus_browser_compositor_output_surface.cc:115:
base::Unretained(this), std::vector<ui::LatencyInfo>(),
What makes Unretained safe?

https://codereview.chromium.org/2096843002/diff/250001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2096843002/diff/250001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1822: } else {
nit: same here re: #ifdefs vs {}

https://codereview.chromium.org/2096843002/diff/250001/services/ui/gpu/gpu_se...
File services/ui/gpu/gpu_service_mus.cc (right):

https://codereview.chromium.org/2096843002/diff/250001/services/ui/gpu/gpu_se...
services/ui/gpu/gpu_service_mus.cc:155: // NOTIMPLEMENTED();
nit: either remove the code (e.g. write a TODO) or leave it in, but commenting
it has no purpose.

[Peng, 2016-07-07 21:33:37]

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:138: // For mus, we
need the alpha channel for "onscreen" context.
On 2016/07/07 20:16:41, danakj wrote:
> For mus the context is not onscreen anymore, so the comments above wrong. Can
> you fix them instead?
> 
> Here's some suggestions
> 
> ".. and by the display compositor (onscreen, except when using mus) for actual
> GL drawing"
> 
> "The display compositor does not use these things either (except for alpha
> channel when using mus for non-opaque ui that overlaps the system's window
> borders), so we can request only that when needed."
> 
> Is that right what I wrote?

Done.

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:149: // For mus, we
create offscreen context.
On 2016/07/07 20:16:41, danakj wrote:
> "an offscreen context"

Done.

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:150: surface_handle
= IsUsingMus() ? gpu::kNullSurfaceHandle : surface_handle;
On 2016/07/07 20:16:41, danakj wrote:
> As the comment above says, this code is used for creating more than one
context.
> I think this code should be at a callsite for this method, not inside the
common
> code.
> 
> Same for the above IsUsingMus(), this method should take parameters for things
> it wants to be optional like "needs_alpha_channel".

Done.

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:272: // Chrome
running as a mojo app currently can only use software compositing.
On 2016/07/07 20:16:41, danakj wrote:
> This comment is wrong now?

Done.

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:323: } else {
On 2016/07/07 21:02:20, piman wrote:
> nit: I would prefer to avoid mixing compile-time #ifdefs and run-time brackets
-
> it makes the code really hard to maintain.
> 
> How about:
> 
>     if (!use_mus) {
>       BrowserGpuChannelHostFactory::instance()->EstablishGpuChannel(
>           CAUSE_FOR_GPU_LAUNCH_SHARED_WORKER_THREAD_CONTEXT, callback);
>     } else {
> #if defined(MOJO_RUNNER_CLIENT)
>       ui::GpuService::GetInstance()->EstablishGpuChannel(callback);
> #else
>       NOTREACHED();
> #endif
>     }

Done.

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:388: } else {
On 2016/07/07 21:02:20, piman wrote:
> nit: ditto

Done.

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:456: } else {
On 2016/07/07 21:02:20, piman wrote:
> nit: ditto

Done.

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:536: #if
defined(MOJO_RUNNER_CLIENT)
On 2016/07/07 20:16:41, danakj wrote:
> Why are things inside #if defined, but the source files are always present in
> the build?

I think the MOJO_RUNNER_CLIENT is only defined with GN build. For gyp build, the
macro is not defined, and mus_browser_compositor_output_surface.{cc,h} are not
in gyp files too.

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:788: } else {
On 2016/07/07 21:02:20, piman wrote:
> nit: ditto

Done.

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
File content/browser/compositor/mus_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/mus_browser_compositor_output_surface.cc:50:
ui_frame.metadata.device_scale_factor = 1.0f;
On 2016/07/07 21:02:20, piman wrote:
> Should you get the metadata from |frame| ?

Done.

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/mus_browser_compositor_output_surface.cc:115:
base::Unretained(this), std::vector<ui::LatencyInfo>(),
On 2016/07/07 21:02:20, piman wrote:
> What makes Unretained safe?

ui_window_surface_ will be destroyed with MusBrowserCompositorOutputSurface. So
the callback should not be fired after ui_window_surface_ is destroyed.

https://codereview.chromium.org/2096843002/diff/250001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2096843002/diff/250001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1822: } else {
On 2016/07/07 21:02:20, piman wrote:
> nit: same here re: #ifdefs vs {}

Done.

https://codereview.chromium.org/2096843002/diff/250001/services/ui/gpu/gpu_se...
File services/ui/gpu/gpu_service_mus.cc (right):

https://codereview.chromium.org/2096843002/diff/250001/services/ui/gpu/gpu_se...
services/ui/gpu/gpu_service_mus.cc:155: // NOTIMPLEMENTED();
On 2016/07/07 21:02:21, piman wrote:
> nit: either remove the code (e.g. write a TODO) or leave it in, but commenting
> it has no purpose.

Done.

[danakj, 2016-07-07 21:42:45]

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
File content/browser/compositor/mus_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/mus_browser_compositor_output_surface.cc:115:
base::Unretained(this), std::vector<ui::LatencyInfo>(),
On 2016/07/07 21:33:37, Peng wrote:
> On 2016/07/07 21:02:20, piman wrote:
> > What makes Unretained safe?
> 
> ui_window_surface_ will be destroyed with MusBrowserCompositorOutputSurface.
So
> the callback should not be fired after ui_window_surface_ is destroyed.

That should go in a comment.

[danakj, 2016-07-07 21:55:13]

danakj@chromium.org changed reviewers:
- danakj@chromium.org

[danakj, 2016-07-07 21:55:15]

Seems okay to me, there's a lot of ppl reviewing this patch. I'll let piman
finish it since he owns more of the files. Thanks for the changes!

https://codereview.chromium.org/2096843002/diff/270001/content/browser/compos...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/2096843002/diff/270001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:130: //   (onscreen)
for actual GL drawing.
missed this: "(onscreen, except for with mus)"

https://codereview.chromium.org/2096843002/diff/270001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:550: #if
defined(MOJO_RUNNER_CLIENT)
move this into the else like the others

[piman, 2016-07-07 21:55:19]

piman@chromium.org changed reviewers:
+ danakj@chromium.org

[piman, 2016-07-07 21:55:21]

We're close...

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
File content/browser/compositor/mus_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/mus_browser_compositor_output_surface.cc:50:
ui_frame.metadata.device_scale_factor = 1.0f;
On 2016/07/07 21:33:37, Peng wrote:
> On 2016/07/07 21:02:20, piman wrote:
> > Should you get the metadata from |frame| ?
> 
> Done.

I meant, get the entire metadata, including latency_info, satisfies_sequences,
referenced_surfaces etc.

iow, can you do ui_frame.metadata = std::move(frame.metadata) ?

https://codereview.chromium.org/2096843002/diff/270001/content/browser/compos...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/2096843002/diff/270001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:551: } else {
nit: can you avoid mixing #ifdefs and {} here too?

[Peng, 2016-07-07 22:46:51]

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
File content/browser/compositor/mus_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/mus_browser_compositor_output_surface.cc:50:
ui_frame.metadata.device_scale_factor = 1.0f;
On 2016/07/07 21:55:20, piman wrote:
> On 2016/07/07 21:33:37, Peng wrote:
> > On 2016/07/07 21:02:20, piman wrote:
> > > Should you get the metadata from |frame| ?
> > 
> > Done.
> 
> I meant, get the entire metadata, including latency_info, satisfies_sequences,
> referenced_surfaces etc.
> 
> iow, can you do ui_frame.metadata = std::move(frame.metadata) ?

Done.

https://codereview.chromium.org/2096843002/diff/250001/content/browser/compos...
content/browser/compositor/mus_browser_compositor_output_surface.cc:115:
base::Unretained(this), std::vector<ui::LatencyInfo>(),
On 2016/07/07 21:42:45, danakj wrote:
> On 2016/07/07 21:33:37, Peng wrote:
> > On 2016/07/07 21:02:20, piman wrote:
> > > What makes Unretained safe?
> > 
> > ui_window_surface_ will be destroyed with MusBrowserCompositorOutputSurface.
> So
> > the callback should not be fired after ui_window_surface_ is destroyed.
> 
> That should go in a comment.

Done.

https://codereview.chromium.org/2096843002/diff/270001/content/browser/compos...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/2096843002/diff/270001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:130: //   (onscreen)
for actual GL drawing.
On 2016/07/07 21:55:14, danakj wrote:
> missed this: "(onscreen, except for with mus)"

Done.

https://codereview.chromium.org/2096843002/diff/270001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:550: #if
defined(MOJO_RUNNER_CLIENT)
On 2016/07/07 21:55:14, danakj wrote:
> move this into the else like the others

Done.

https://codereview.chromium.org/2096843002/diff/270001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:551: } else {
On 2016/07/07 21:55:21, piman wrote:
> nit: can you avoid mixing #ifdefs and {} here too?

Done.

[piman, 2016-07-07 22:49:17]

lgtm

[Peng, 2016-07-07 22:52:01]

penghuang@chromium.org changed reviewers:
+ sky@chromium.org

[Peng, 2016-07-07 22:52:03]

+sky for //services/ui & //ui/views/mus

Hi Scott, PTAL. Thanks.

[Peng, 2016-07-07 22:53:03]

The CQ bit was checked by penghuang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-07 22:54:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 00:56:17]

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

[commit-bot: I haz the power, 2016-07-08 00:56:18]

Dry run: Try jobs failed on following builders:
  mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)

[sky, 2016-07-08 15:34:22]

LGTM with the following changes

https://codereview.chromium.org/2096843002/diff/310001/services/ui/gpu/gpu_se...
File services/ui/gpu/gpu_service_mus.cc (right):

https://codereview.chromium.org/2096843002/diff/310001/services/ui/gpu/gpu_se...
services/ui/gpu/gpu_service_mus.cc:154: // TODO(penghuang): implement this
function.
nit: leave the NOTIMPLEMENTED.

https://codereview.chromium.org/2096843002/diff/310001/ui/views/mus/window_tr...
File ui/views/mus/window_tree_host_mus.h (right):

https://codereview.chromium.org/2096843002/diff/310001/ui/views/mus/window_tr...
ui/views/mus/window_tree_host_mus.h:32: NativeWidgetMus* native_widget() const {
return native_widget_; }
This function shouldn't be const, or if it is the return type should be const
too. In other words, one or both of:

const NativeWidgetMus* native_widget() const;
NativeWidgetMus* native_widget();

[Peng, 2016-07-08 16:13:48]

https://codereview.chromium.org/2096843002/diff/310001/services/ui/gpu/gpu_se...
File services/ui/gpu/gpu_service_mus.cc (right):

https://codereview.chromium.org/2096843002/diff/310001/services/ui/gpu/gpu_se...
services/ui/gpu/gpu_service_mus.cc:154: // TODO(penghuang): implement this
function.
On 2016/07/08 15:34:22, sky wrote:
> nit: leave the NOTIMPLEMENTED.

NOTIMPLEMENTED() is too verbose here. That is why I removed it.

https://codereview.chromium.org/2096843002/diff/310001/ui/views/mus/window_tr...
File ui/views/mus/window_tree_host_mus.h (right):

https://codereview.chromium.org/2096843002/diff/310001/ui/views/mus/window_tr...
ui/views/mus/window_tree_host_mus.h:32: NativeWidgetMus* native_widget() const {
return native_widget_; }
On 2016/07/08 15:34:22, sky wrote:
> This function shouldn't be const, or if it is the return type should be const
> too. In other words, one or both of:
> 
> const NativeWidgetMus* native_widget() const;
> NativeWidgetMus* native_widget();

Done.

[Peng, 2016-07-08 16:14:02]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2016-07-08 16:14:02]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2096843002/#ps330001
(title: "Update")

[commit-bot: I haz the power, 2016-07-08 16:14:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 18:15:40]

Description was changed from

==========
mus+ash: Enable Chrome HW rendering in mus+ash


This change enables the HW supports in mus+ash by
 * Wire up ui service GPU channel.
 * Always create offscreen GL context for gfx::AcceleratedWidget (mus window) in
   browser process.
 * Provide a MusBrowserCompositorOutputSurface.
   In MusBrowserCompositorOutputSurface::SwapBuffers(), we use  
   CommandBufferProxyImpl::TakeFrontBuffer() to take the offscreen GL context's
   front buffer into a mailbox, insert a sync token, and send mailbox+sync to
   the ui service process.

Know issue:
 GPUInfo is not sent to renderer, so some webgl demos don't work.


BUG=622708
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
mus+ash: Enable Chrome HW rendering in mus+ash


This change enables the HW supports in mus+ash by
 * Wire up ui service GPU channel.
 * Always create offscreen GL context for gfx::AcceleratedWidget (mus window) in
   browser process.
 * Provide a MusBrowserCompositorOutputSurface.
   In MusBrowserCompositorOutputSurface::SwapBuffers(), we use  
   CommandBufferProxyImpl::TakeFrontBuffer() to take the offscreen GL context's
   front buffer into a mailbox, insert a sync token, and send mailbox+sync to
   the ui service process.

Know issue:
 GPUInfo is not sent to renderer, so some webgl demos don't work.


BUG=622708
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-07-08 18:15:43]

Committed patchset #17 (id:330001)

[commit-bot: I haz the power, 2016-07-08 18:15:55]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 18:18:27]

Description was changed from

==========
mus+ash: Enable Chrome HW rendering in mus+ash


This change enables the HW supports in mus+ash by
 * Wire up ui service GPU channel.
 * Always create offscreen GL context for gfx::AcceleratedWidget (mus window) in
   browser process.
 * Provide a MusBrowserCompositorOutputSurface.
   In MusBrowserCompositorOutputSurface::SwapBuffers(), we use  
   CommandBufferProxyImpl::TakeFrontBuffer() to take the offscreen GL context's
   front buffer into a mailbox, insert a sync token, and send mailbox+sync to
   the ui service process.

Know issue:
 GPUInfo is not sent to renderer, so some webgl demos don't work.


BUG=622708
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
mus+ash: Enable Chrome HW rendering in mus+ash

This change enables the HW supports in mus+ash by
 * Wire up ui service GPU channel.
 * Always create offscreen GL context for gfx::AcceleratedWidget (mus window) in
   browser process.
 * Provide a MusBrowserCompositorOutputSurface.
   In MusBrowserCompositorOutputSurface::SwapBuffers(), we use
   CommandBufferProxyImpl::TakeFrontBuffer() to take the offscreen GL context's
   front buffer into a mailbox, insert a sync token, and send mailbox+sync to
   the ui service process.

Know issue:
 GPUInfo is not sent to renderer, so some webgl demos don't work.

BUG=622708
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/e1d86515496285e3b5f593fa79d72daec6e64feb
Cr-Commit-Position: refs/heads/master@{#404441}
==========

[commit-bot: I haz the power, 2016-07-08 18:18:29]

Patchset 17 (id:??) landed as
https://crrev.com/e1d86515496285e3b5f593fa79d72daec6e64feb
Cr-Commit-Position: refs/heads/master@{#404441}