Impl mus::mojom::GpuService to enable using Chrome IPC version gpu CmdBuf in mus

Impl mus::mojom::GpuService to enable using Chrome IPC version gpu CmdBuf in mus

 * Implements mus::mojo::GpuService which allows clients requesting a gpu IPC::ChannelHandle from mus, and then use it to construct GpuChannelHost and CommandBufferProxyImpl.
 * Uses the Chrome IPC CmdBuf in mus::GLES2Context.
 * Adds --use-chrome-gpu-command-buffer-in-mus flag which is used for switching between mojo CmdBuf and Chrome IPC CmdBuf.
 * Tested with ./out/mus_Debug/mojo_runner mojo:mus_demo

Known issues:
 * The singleton mus::GpuService doesn't work for component build.
 * GpuMemoryBuffer is not implemented. The mash crashes because of it.

BUG=586390
Committed: https://crrev.com/591151b6e0b55a6e30dc7b789e6ffcd2da02e6cf
Cr-Commit-Position: refs/heads/master@{#397921}

[Peng, 2016-05-24 18:07:34]

Description was changed from

==========
Impl mus::mojom::GpuService

BUG=586390
==========

to

==========
Impl mus::mojom::GpuService to enable using Chrome IPC version gpu CmdBuf in mus

This CL implements mus::mojo::GpuService which allows clients
requesting a gpu IPC::ChannelHandle from mus, and then use it
to construct GpuChannelHost and CommandBufferProxyImpl.

This CL also adds --use-chrome-gpu-command-buffer-in-mus
flag which is used for switching between mojo CmdBuf and
Chrome IPC CmdBuf.

BUG=586390
==========

[Peng, 2016-05-24 18:09:26]

Description was changed from

==========
Impl mus::mojom::GpuService to enable using Chrome IPC version gpu CmdBuf in mus

This CL implements mus::mojo::GpuService which allows clients
requesting a gpu IPC::ChannelHandle from mus, and then use it
to construct GpuChannelHost and CommandBufferProxyImpl.

This CL also adds --use-chrome-gpu-command-buffer-in-mus
flag which is used for switching between mojo CmdBuf and
Chrome IPC CmdBuf.

BUG=586390
==========

to

==========
Impl mus::mojom::GpuService to enable using Chrome IPC version gpu CmdBuf in mus

 1. Implements mus::mojo::GpuService which allows clients requesting a gpu
IPC::ChannelHandle from mus, and then use it to construct GpuChannelHost and
CommandBufferProxyImpl.
 3. Uses the Chrome IPC CmdBuf in mus::GLES2Context.
 2. Adds --use-chrome-gpu-command-buffer-in-mus flag which is used for switching
between mojo CmdBuf and Chrome IPC CmdBuf.

BUG=586390
==========

[Peng, 2016-05-24 18:09:44]

Description was changed from

==========
Impl mus::mojom::GpuService to enable using Chrome IPC version gpu CmdBuf in mus

 1. Implements mus::mojo::GpuService which allows clients requesting a gpu
IPC::ChannelHandle from mus, and then use it to construct GpuChannelHost and
CommandBufferProxyImpl.
 3. Uses the Chrome IPC CmdBuf in mus::GLES2Context.
 2. Adds --use-chrome-gpu-command-buffer-in-mus flag which is used for switching
between mojo CmdBuf and Chrome IPC CmdBuf.

BUG=586390
==========

to

==========
Impl mus::mojom::GpuService to enable using Chrome IPC version gpu CmdBuf in mus

 * Implements mus::mojo::GpuService which allows clients requesting a gpu
IPC::ChannelHandle from mus, and then use it to construct GpuChannelHost and
CommandBufferProxyImpl.
 * Uses the Chrome IPC CmdBuf in mus::GLES2Context.
 * Adds --use-chrome-gpu-command-buffer-in-mus flag which is used for switching
between mojo CmdBuf and Chrome IPC CmdBuf.

BUG=586390
==========

[Peng, 2016-05-24 18:19:06]

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

[Peng, 2016-05-24 18:19:07]

Hi piman, PTAL. Thanks.

[piman, 2016-05-24 22:46:54]

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
File components/mus/gpu/gpu_memory_buffer_manager_mus_local.h (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
components/mus/gpu/gpu_memory_buffer_manager_mus_local.h:35: const
gpu::SyncToken& sync_token) override;
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
File components/mus/gpu/gpu_service_impl.cc (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
components/mus/gpu/gpu_service_impl.cc:33: allow_real_time_streams);
I think it would be preferable to have EstablishGpuChannel be asynchronous,
taking a callback to pass back the channel handle (and GPU info etc.) -
otherwise here you have to serialize all the clients, which would likely impact
startup time.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
File components/mus/gpu/gpu_service_mus.cc (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
components/mus/gpu/gpu_service_mus.cc:63: CHECK_GT(client_id,
kLocalGpuChannelClientId);
nit: DCHECK_GT

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
File components/mus/gpu/gpu_service_mus.h (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
components/mus/gpu/gpu_service_mus.h:133: // The thread that handles IO events
for GpuChannelHost local.
Does this have to be different from io_thread_?
I think when we run with --in-process-gpu in Chrome they are the same.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
File components/mus/public/cpp/lib/gles2_context.cc (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
components/mus/public/cpp/lib/gles2_context.cc:56: LOG(WARNING) << "Parse
attribs failed!";
nit: DLOG

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
File components/mus/public/cpp/lib/gpu_service.cc (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
components/mus/public/cpp/lib/gpu_service.cc:80:
CHECK(gpu_service.WaitForIncomingResponse());
I assume this can return false if the mus process dies. What will be the
strategy to handle this case?

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
File components/mus/public/interfaces/gpu_service.mojom (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
components/mus/public/interfaces/gpu_service.mojom:19: bool
allow_real_time_streams)
These 3 bools are security-sensitive. In Chrome we only allow the (trusted)
browser channel to have this on, which we do by making sure the renderer's
requests set this to false.

In the case of mus, the equivalent to the browser channel is I think the
GpuServiceMus's GpuChannelHost which connects directly without the mojom. So I
think these 3 bools should just be removed from the interface, and have the
GpuServiceImpl set those to false for the requests it processes.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/surface...
File components/mus/surfaces/surfaces_context_provider.cc (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/surface...
components/mus/surfaces/surfaces_context_provider.cc:89: gpu_control =
command_buffer_proxy_impl_.get();
CommandBufferProxyImpl::Create can fail, so you also want to return false if
it's null here.

[Fady Samuel, 2016-05-25 17:06:46]

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

[Fady Samuel, 2016-05-25 17:06:48]

Sorry for the drive-by but could we please split this off into a few small CLs?
Thanks!

https://codereview.chromium.org/1976703003/diff/220001/components/bitmap_uplo...
File components/bitmap_uploader/bitmap_uploader.cc (right):

https://codereview.chromium.org/1976703003/diff/220001/components/bitmap_uplo...
components/bitmap_uploader/bitmap_uploader.cc:53: if
(!mus::GpuService::UseChromeGpuCommandBuffer()) {
Drive by: Could we hide these details in the client library?

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
File components/mus/gpu/gpu_service_impl.cc (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
components/mus/gpu/gpu_service_impl.cc:17:
client_id_(connection->GetRemoteInstanceID() + 1) {
What is this client ID? A comment would be helpful to explain what it's used
for.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
File components/mus/gpu/gpu_service_mus.cc (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
components/mus/gpu/gpu_service_mus.cc:201: owned_sync_point_manager_.reset(new
gpu::SyncPointManager(false));
What's this false?

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
components/mus/gpu/gpu_service_mus.cc:206: gpu::GpuWatchdog* watchdog = nullptr;
Add a TODO here please to implement a watchdog thread.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
File components/mus/public/cpp/context_provider.h (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
components/mus/public/cpp/context_provider.h:21: class MUS_PUBLIC_EXPORT
ContextProvider : public cc::ContextProvider {
Why did you make this a component?

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
File components/mus/public/cpp/lib/gpu_channel_host_factory_mus.h (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
components/mus/public/cpp/lib/gpu_channel_host_factory_mus.h:13: class
GpuMemoryBufferManagerMus : public gpu::GpuMemoryBufferManager {
nit: mus seems redundant here.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
File components/mus/public/cpp/lib/gpu_service.cc (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
components/mus/public/cpp/lib/gpu_service.cc:37: io_thread_("GPUIOThread") {}
This seems odd. Why are we forcing a new thread upon clients.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
components/mus/public/cpp/lib/gpu_service.cc:42: bool
GpuService::UseChromeGpuCommandBuffer() {
Do we need this?

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
File components/mus/public/interfaces/gpu_service.mojom (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
components/mus/public/interfaces/gpu_service.mojom:19: bool
allow_real_time_streams)
On 2016/05/24 22:46:54, piman wrote:
> These 3 bools are security-sensitive. In Chrome we only allow the (trusted)
> browser channel to have this on, which we do by making sure the renderer's
> requests set this to false.
> 
> In the case of mus, the equivalent to the browser channel is I think the
> GpuServiceMus's GpuChannelHost which connects directly without the mojom. So I
> think these 3 bools should just be removed from the interface, and have the
> GpuServiceImpl set those to false for the requests it processes.

Could we split this off to two interfaces? Mus Window server will be in a
separate (privileged) process and will likely want to control the same
parameters the browser process does today.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/surface...
File components/mus/surfaces/surfaces_context_provider.cc (left):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/surface...
components/mus/surfaces/surfaces_context_provider.cc:24:
SurfacesContextProvider::SurfacesContextProvider(
Could we have two ContextProviders instead? I prefer polymorphism to
conditionals so I would prefer two ContextProviders. That way we can easily
delete the old code path.

[Peng, 2016-05-25 19:42:56]

https://codereview.chromium.org/1976703003/diff/220001/components/bitmap_uplo...
File components/bitmap_uploader/bitmap_uploader.cc (right):

https://codereview.chromium.org/1976703003/diff/220001/components/bitmap_uplo...
components/bitmap_uploader/bitmap_uploader.cc:53: if
(!mus::GpuService::UseChromeGpuCommandBuffer()) {
On 2016/05/25 17:06:47, Fady Samuel wrote:
> Drive by: Could we hide these details in the client library?

Yes of course. But to make this CL less complex, probably it better refactor it
later. WDYT?

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
File components/mus/gpu/gpu_memory_buffer_manager_mus_local.h (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
components/mus/gpu/gpu_memory_buffer_manager_mus_local.h:35: const
gpu::SyncToken& sync_token) override;
On 2016/05/24 22:46:53, piman wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
File components/mus/gpu/gpu_service_impl.cc (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
components/mus/gpu/gpu_service_impl.cc:17:
client_id_(connection->GetRemoteInstanceID() + 1) {
On 2016/05/25 17:06:47, Fady Samuel wrote:
> What is this client ID? A comment would be helpful to explain what it's used
> for.

The unique client id is for constructing the GpuChannel. I added comment in
header file.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
components/mus/gpu/gpu_service_impl.cc:33: allow_real_time_streams);
On 2016/05/24 22:46:53, piman wrote:
> I think it would be preferable to have EstablishGpuChannel be asynchronous,
> taking a callback to pass back the channel handle (and GPU info etc.) -
> otherwise here you have to serialize all the clients, which would likely
impact
> startup time.

Done.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
File components/mus/gpu/gpu_service_mus.cc (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
components/mus/gpu/gpu_service_mus.cc:63: CHECK_GT(client_id,
kLocalGpuChannelClientId);
On 2016/05/24 22:46:54, piman wrote:
> nit: DCHECK_GT

Done.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
components/mus/gpu/gpu_service_mus.cc:201: owned_sync_point_manager_.reset(new
gpu::SyncPointManager(false));
On 2016/05/25 17:06:47, Fady Samuel wrote:
> What's this false?

Done.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
components/mus/gpu/gpu_service_mus.cc:206: gpu::GpuWatchdog* watchdog = nullptr;
On 2016/05/25 17:06:47, Fady Samuel wrote:
> Add a TODO here please to implement a watchdog thread.

Done.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
File components/mus/gpu/gpu_service_mus.h (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/gpu/gpu...
components/mus/gpu/gpu_service_mus.h:133: // The thread that handles IO events
for GpuChannelHost local.
On 2016/05/24 22:46:54, piman wrote:
> Does this have to be different from io_thread_?
> I think when we run with --in-process-gpu in Chrome they are the same.

Done.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
File components/mus/public/cpp/context_provider.h (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
components/mus/public/cpp/context_provider.h:21: class MUS_PUBLIC_EXPORT
ContextProvider : public cc::ContextProvider {
On 2016/05/25 17:06:47, Fady Samuel wrote:
> Why did you make this a component?

Becatse GpuServiceMus has a global variable g_gpu_service, to avoid duplicate
copy in one executable image, we have to make this a shared library.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
File components/mus/public/cpp/lib/gles2_context.cc (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
components/mus/public/cpp/lib/gles2_context.cc:56: LOG(WARNING) << "Parse
attribs failed!";
On 2016/05/24 22:46:54, piman wrote:
> nit: DLOG

Done.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
File components/mus/public/cpp/lib/gpu_channel_host_factory_mus.h (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
components/mus/public/cpp/lib/gpu_channel_host_factory_mus.h:13: class
GpuMemoryBufferManagerMus : public gpu::GpuMemoryBufferManager {
On 2016/05/25 17:06:47, Fady Samuel wrote:
> nit: mus seems redundant here.
oops! Remove this file.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
File components/mus/public/cpp/lib/gpu_service.cc (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
components/mus/public/cpp/lib/gpu_service.cc:37: io_thread_("GPUIOThread") {}
On 2016/05/25 17:06:47, Fady Samuel wrote:
> This seems odd. Why are we forcing a new thread upon clients.

This class is used in client process. Every client process only has one instance
of this class. So just one extra GPU IO
thread in client process.

I also check the mojo code. Seems there is not a good way to access mojo's IO
thread. so we have to create a dedicated IO thread right now.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
components/mus/public/cpp/lib/gpu_service.cc:42: bool
GpuService::UseChromeGpuCommandBuffer() {
On 2016/05/25 17:06:47, Fady Samuel wrote:
> Do we need this?

Yes. Probably we can hide this switch in client side library, but it will need
change interface of GLES2Context. I would like to avoid it in this CL and do the
refactor later.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
components/mus/public/cpp/lib/gpu_service.cc:80:
CHECK(gpu_service.WaitForIncomingResponse());
On 2016/05/24 22:46:54, piman wrote:
> I assume this can return false if the mus process dies. What will be the
> strategy to handle this case?

I don't what should we do for it, so I just make it crash for now. Any
suggestion?

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
File components/mus/public/interfaces/gpu_service.mojom (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
components/mus/public/interfaces/gpu_service.mojom:19: bool
allow_real_time_streams)
On 2016/05/24 22:46:54, piman wrote:
> These 3 bools are security-sensitive. In Chrome we only allow the (trusted)
> browser channel to have this on, which we do by making sure the renderer's
> requests set this to false.
> 
> In the case of mus, the equivalent to the browser channel is I think the
> GpuServiceMus's GpuChannelHost which connects directly without the mojom. So I
> think these 3 bools should just be removed from the interface, and have the
> GpuServiceImpl set those to false for the requests it processes.

Done.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
components/mus/public/interfaces/gpu_service.mojom:19: bool
allow_real_time_streams)
On 2016/05/25 17:06:47, Fady Samuel wrote:
> On 2016/05/24 22:46:54, piman wrote:
> > These 3 bools are security-sensitive. In Chrome we only allow the (trusted)
> > browser channel to have this on, which we do by making sure the renderer's
> > requests set this to false.
> > 
> > In the case of mus, the equivalent to the browser channel is I think the
> > GpuServiceMus's GpuChannelHost which connects directly without the mojom. So
I
> > think these 3 bools should just be removed from the interface, and have the
> > GpuServiceImpl set those to false for the requests it processes.
> 
> Could we split this off to two interfaces? Mus Window server will be in a
> separate (privileged) process and will likely want to control the same
> parameters the browser process does today.

I think we could add the private interface later(when we need it). A TODO has
been added in impl.cc

https://codereview.chromium.org/1976703003/diff/220001/components/mus/surface...
File components/mus/surfaces/surfaces_context_provider.cc (left):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/surface...
components/mus/surfaces/surfaces_context_provider.cc:24:
SurfacesContextProvider::SurfacesContextProvider(
On 2016/05/25 17:06:47, Fady Samuel wrote:
> Could we have two ContextProviders instead? I prefer polymorphism to
> conditionals so I would prefer two ContextProviders. That way we can easily
> delete the old code path.

I don't think it has too much different. Even if we have two ContextProivder, we
still need check the switch somewhere, and then create the right provider
accordingly. Maybe need add another factory class as well.

BTW, I plan to do refactor in a followup CL. It should hide this detail in the
GLES2Context class.

https://codereview.chromium.org/1976703003/diff/220001/components/mus/surface...
File components/mus/surfaces/surfaces_context_provider.cc (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/surface...
components/mus/surfaces/surfaces_context_provider.cc:89: gpu_control =
command_buffer_proxy_impl_.get();
On 2016/05/24 22:46:54, piman wrote:
> CommandBufferProxyImpl::Create can fail, so you also want to return false if
> it's null here.

Done.

[piman, 2016-05-25 22:53:17]

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
File components/mus/public/cpp/lib/gpu_service.cc (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
components/mus/public/cpp/lib/gpu_service.cc:80:
CHECK(gpu_service.WaitForIncomingResponse());
On 2016/05/25 19:42:56, Peng wrote:
> On 2016/05/24 22:46:54, piman wrote:
> > I assume this can return false if the mus process dies. What will be the
> > strategy to handle this case?
> 
> I don't what should we do for it, so I just make it crash for now. Any
> suggestion?

In Chrome, in RenderThreadImpl, we create the channel lazily, when the first
client creates a command buffer. If creating the channel fails, we fail the
command buffer creation, but we allow the client to retry, or it may fallback
(e.g. to software). If the client retries, the browser (which services the
EstablishGpuChannel) tries to recreate a GPU process. I imagine something
similar will happen with the mus process.

Similarly, if the channel was successfully created, but later on the GPU process
(respectively, the mus process) crashes, later requests to create command
buffers will fail, but the RenderThreadImpl code (respectively this GpuService?)
attempts to recreate a channel before failing.

Maybe this all doesn't have to be solved now, or maybe not solved the same way,
but given the levels of GPU crash rate (even only on Chrome OS), I think this
needs to be addressed at some point. In particular I think it's important to
have an API that allows it to work, so just vending a static GpuChannelHost may
not be enough.

https://codereview.chromium.org/1976703003/diff/240001/content/renderer/mus/r...
File content/renderer/mus/render_widget_mus_connection.cc (right):

https://codereview.chromium.org/1976703003/diff/240001/content/renderer/mus/r...
content/renderer/mus/render_widget_mus_connection.cc:54: if
(mus::GpuService::UseChromeGpuCommandBuffer()) {
did you mean !mus::GpuService::UseChromeGpuCommandBuffer() here?

[Peng, 2016-05-30 21:59:21]

Patchset #14 (id:260001) has been deleted

[Peng, 2016-05-30 21:59:32]

Patchset #14 (id:280001) has been deleted

[Peng, 2016-05-30 21:59:44]

Patchset #14 (id:300001) has been deleted

[Peng, 2016-05-30 21:59:56]

Patchset #14 (id:320001) has been deleted

[Peng, 2016-05-30 22:00:06]

Patchset #14 (id:340001) has been deleted

[Peng, 2016-05-31 14:24:04]

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
File components/mus/public/cpp/lib/gpu_service.cc (right):

https://codereview.chromium.org/1976703003/diff/220001/components/mus/public/...
components/mus/public/cpp/lib/gpu_service.cc:80:
CHECK(gpu_service.WaitForIncomingResponse());
On 2016/05/25 22:53:17, piman OOO back 2016-5-31 wrote:
> On 2016/05/25 19:42:56, Peng wrote:
> > On 2016/05/24 22:46:54, piman wrote:
> > > I assume this can return false if the mus process dies. What will be the
> > > strategy to handle this case?
> > 
> > I don't what should we do for it, so I just make it crash for now. Any
> > suggestion?
> 
> In Chrome, in RenderThreadImpl, we create the channel lazily, when the first
> client creates a command buffer. If creating the channel fails, we fail the
> command buffer creation, but we allow the client to retry, or it may fallback
> (e.g. to software). If the client retries, the browser (which services the
> EstablishGpuChannel) tries to recreate a GPU process. I imagine something
> similar will happen with the mus process.
> 
> Similarly, if the channel was successfully created, but later on the GPU
process
> (respectively, the mus process) crashes, later requests to create command
> buffers will fail, but the RenderThreadImpl code (respectively this
GpuService?)
> attempts to recreate a channel before failing.
> 
> Maybe this all doesn't have to be solved now, or maybe not solved the same
way,
> but given the levels of GPU crash rate (even only on Chrome OS), I think this
> needs to be addressed at some point. In particular I think it's important to
> have an API that allows it to work, so just vending a static GpuChannelHost
may
> not be enough.

I renamed the Init() to EstablishGpuChannel(), and this function will establish
the gpu channel
if the channel is null or it is lost.

Done

https://codereview.chromium.org/1976703003/diff/240001/content/renderer/mus/r...
File content/renderer/mus/render_widget_mus_connection.cc (right):

https://codereview.chromium.org/1976703003/diff/240001/content/renderer/mus/r...
content/renderer/mus/render_widget_mus_connection.cc:54: if
(mus::GpuService::UseChromeGpuCommandBuffer()) {
On 2016/05/25 22:53:17, piman OOO back 2016-5-31 wrote:
> did you mean !mus::GpuService::UseChromeGpuCommandBuffer() here?

Done.

[Peng, 2016-05-31 14:27:21]

Description was changed from

==========
Impl mus::mojom::GpuService to enable using Chrome IPC version gpu CmdBuf in mus

 * Implements mus::mojo::GpuService which allows clients requesting a gpu
IPC::ChannelHandle from mus, and then use it to construct GpuChannelHost and
CommandBufferProxyImpl.
 * Uses the Chrome IPC CmdBuf in mus::GLES2Context.
 * Adds --use-chrome-gpu-command-buffer-in-mus flag which is used for switching
between mojo CmdBuf and Chrome IPC CmdBuf.

BUG=586390
==========

to

==========
Impl mus::mojom::GpuService to enable using Chrome IPC version gpu CmdBuf in mus

 * Implements mus::mojo::GpuService which allows clients requesting a gpu
IPC::ChannelHandle from mus, and then use it to construct GpuChannelHost and
CommandBufferProxyImpl.
 * Uses the Chrome IPC CmdBuf in mus::GLES2Context.
 * Adds --use-chrome-gpu-command-buffer-in-mus flag which is used for switching
between mojo CmdBuf and Chrome IPC CmdBuf.
 * Tested with ./out/mus_Debug/mojo_runner mojo:mus_demo 

Known issues:
 * The singleton mus::GpuService doesn't work for component build.
 * GpuMemoryBuffer is not implemented. The mash crashes because of it.

BUG=586390
==========

[piman, 2016-05-31 23:14:05]

one last think I think, then LGTM.

https://codereview.chromium.org/1976703003/diff/380001/components/mus/public/...
File components/mus/public/cpp/lib/gles2_context.cc (right):

https://codereview.chromium.org/1976703003/diff/380001/components/mus/public/...
components/mus/public/cpp/lib/gles2_context.cc:49:
GpuService::GetInstance()->EstablishGpuChannel(connector);
gpu_channel_host can be null, in which case you want to return failure.

[Peng, 2016-06-01 14:11:28]

https://codereview.chromium.org/1976703003/diff/380001/components/mus/public/...
File components/mus/public/cpp/lib/gles2_context.cc (right):

https://codereview.chromium.org/1976703003/diff/380001/components/mus/public/...
components/mus/public/cpp/lib/gles2_context.cc:49:
GpuService::GetInstance()->EstablishGpuChannel(connector);
On 2016/05/31 23:14:05, piman OOO back 2016-6-2 wrote:
> gpu_channel_host can be null, in which case you want to return failure.

Done.

[Peng, 2016-06-01 14:17:17]

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

[Peng, 2016-06-01 14:17:19]

+sky@ OWNER of components/mus & ui/views
+sandersd@ OWNER of media/gpu

Hi Scott & Dan, PTAL. Thanks.

[Peng, 2016-06-01 14:39:45]

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

[Peng, 2016-06-01 14:39:46]

+sky@ OWNER of components/mus & ui/views
+sandersd@ OWNER of media/gpu

Hi Scott & Dan, PTAL. Thanks.

[sky, 2016-06-01 15:45:29]

https://codereview.chromium.org/1976703003/diff/440001/components/bitmap_uplo...
File components/bitmap_uploader/bitmap_uploader.cc (left):

https://codereview.chromium.org/1976703003/diff/440001/components/bitmap_uplo...
components/bitmap_uploader/bitmap_uploader.cc:41: void
BitmapUploader::Init(shell::Connector* connector) {
Isn't this in services, which you also removed the dep from?

https://codereview.chromium.org/1976703003/diff/440001/components/bitmap_uplo...
File components/bitmap_uploader/bitmap_uploader.h (left):

https://codereview.chromium.org/1976703003/diff/440001/components/bitmap_uplo...
components/bitmap_uploader/bitmap_uploader.h:18: #include
"components/mus/public/interfaces/compositor_frame.mojom.h"
You removed the dep on components/mus/public/interfaces from BUILD.gn. Doesn't
gn check fail?

https://codereview.chromium.org/1976703003/diff/440001/ui/views/mus/surface_b...
File ui/views/mus/surface_binding.cc (left):

https://codereview.chromium.org/1976703003/diff/440001/ui/views/mus/surface_b...
ui/views/mus/surface_binding.cc:134: gpu_.set_connection_error_handler([]{});
Where does this code end up now?

[Peng, 2016-06-01 16:23:21]

https://codereview.chromium.org/1976703003/diff/440001/components/bitmap_uplo...
File components/bitmap_uploader/bitmap_uploader.cc (left):

https://codereview.chromium.org/1976703003/diff/440001/components/bitmap_uplo...
components/bitmap_uploader/bitmap_uploader.cc:41: void
BitmapUploader::Init(shell::Connector* connector) {
On 2016/06/01 15:45:29, sky wrote:
> Isn't this in services, which you also removed the dep from?

Yes. But this file just passes it as incomplete pointer to GLES2Context (never
really use it). So probably it is OK to remove it from dep, right?

https://codereview.chromium.org/1976703003/diff/440001/components/bitmap_uplo...
File components/bitmap_uploader/bitmap_uploader.h (left):

https://codereview.chromium.org/1976703003/diff/440001/components/bitmap_uplo...
components/bitmap_uploader/bitmap_uploader.h:18: #include
"components/mus/public/interfaces/compositor_frame.mojom.h"
On 2016/06/01 15:45:29, sky wrote:
> You removed the dep on components/mus/public/interfaces from BUILD.gn. Doesn't
> gn check fail?

No. Because the public/cpp uses interfaces as public_deps.

But I think it is better to add interfaces in bitmap_uploader's deps too, so
added it back.

Done

https://codereview.chromium.org/1976703003/diff/440001/ui/views/mus/surface_b...
File ui/views/mus/surface_binding.cc (left):

https://codereview.chromium.org/1976703003/diff/440001/ui/views/mus/surface_b...
ui/views/mus/surface_binding.cc:134: gpu_.set_connection_error_handler([]{});
On 2016/06/01 15:45:29, sky wrote:
> Where does this code end up now?

With CL, we will create a new GpuPtr in GLES2Context::Initialize() every time,
and that method will find out errors if GPU crashes. So I think we don't need
check the error here anymore.

[sky, 2016-06-01 17:07:23]

Ok, LGTM

[Peng, 2016-06-01 17:53:46]

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

[Peng, 2016-06-01 17:53:47]

+dcheng for components/mus/public/interfaces/gpu_service.mojom

Hi Danial, PTAL. Thanks.

[sandersd (OOO until July 31), 2016-06-01 18:36:27]

media/ LGTM

[sadrul, 2016-06-01 19:16:55]

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

[sadrul, 2016-06-01 19:16:57]

https://codereview.chromium.org/1976703003/diff/460001/components/mus/public/...
File components/mus/public/cpp/lib/context_provider.cc (right):

https://codereview.chromium.org/1976703003/diff/460001/components/mus/public/...
components/mus/public/cpp/lib/context_provider.cc:19:
GLES2Context::CreateOffscreenContext(std::vector<int32_t>(), connector_);
Note that if |connector_| is coming from a different thread (which I believe is
the case with --use-mus-in-renderer), then this wouldn't work, and you would
need to Clone() the connector first (the clone needs to happen in the thread
|connector_| lives in)

[Peng, 2016-06-01 20:00:42]

https://codereview.chromium.org/1976703003/diff/460001/components/mus/public/...
File components/mus/public/cpp/lib/context_provider.cc (right):

https://codereview.chromium.org/1976703003/diff/460001/components/mus/public/...
components/mus/public/cpp/lib/context_provider.cc:19:
GLES2Context::CreateOffscreenContext(std::vector<int32_t>(), connector_);
On 2016/06/01 19:16:57, sadrul wrote:
> Note that if |connector_| is coming from a different thread (which I believe
is
> the case with --use-mus-in-renderer), then this wouldn't work, and you would
> need to Clone() the connector first (the clone needs to happen in the thread
> |connector_| lives in)

Good catch. Done.

[Peng, 2016-06-03 14:09:09]

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

[Peng, 2016-06-03 14:09:11]

Try +wfh for *.mojom security review.

Hi Will, PTAL. Thanks.

[Peng, 2016-06-03 14:29:07]

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

[Peng, 2016-06-03 14:29:08]

+tsepez for *.mojom security review too.

Hi Tom, seems you are working more close to mustash. PTAL. Thanks.

[Tom Sepez, 2016-06-03 17:11:11]

https://codereview.chromium.org/1976703003/diff/500001/components/mus/public/...
File components/mus/public/interfaces/gpu_service.mojom (right):

https://codereview.chromium.org/1976703003/diff/500001/components/mus/public/...
components/mus/public/interfaces/gpu_service.mojom:18: => (int32 client_id,
ChannelHandle channel_handle, GpuInfo? gpu_info);
How does it know that its less trustworthy client is telling the truth about its
client_id and isn't trying to create a conflict with an existing unrelated
client?

[dcheng, 2016-06-03 17:34:48]

https://codereview.chromium.org/1976703003/diff/500001/components/mus/public/...
File components/mus/public/cpp/lib/gpu_service.cc (right):

https://codereview.chromium.org/1976703003/diff/500001/components/mus/public/...
components/mus/public/cpp/lib/gpu_service.cc:73: if
(!gpu_service.WaitForIncomingResponse()) {
Why is EstablishGpuChannel not just a sync method?

https://codereview.chromium.org/1976703003/diff/500001/components/mus/public/...
File components/mus/public/interfaces/gpu_service.mojom (right):

https://codereview.chromium.org/1976703003/diff/500001/components/mus/public/...
components/mus/public/interfaces/gpu_service.mojom:18: => (int32 client_id,
ChannelHandle channel_handle, GpuInfo? gpu_info);
Is GpuInfo nullable only because it's not yet implemented? Why add that
parameter at all? Why would the receiver ever want to not return GpuInfo?

[dcheng, 2016-06-03 17:34:59]

A few drive-bys.

[Peng, 2016-06-03 18:34:07]

https://codereview.chromium.org/1976703003/diff/500001/components/mus/public/...
File components/mus/public/cpp/lib/gpu_service.cc (right):

https://codereview.chromium.org/1976703003/diff/500001/components/mus/public/...
components/mus/public/cpp/lib/gpu_service.cc:73: if
(!gpu_service.WaitForIncomingResponse()) {
On 2016/06/03 17:34:48, dcheng wrote:
> Why is EstablishGpuChannel not just a sync method?

Right now, the GpuService is the only consumer of this mojom interface, but I
think the GpuProcessTransportFactory will use it to create gpu channel browser
process in mus environment soon. In that moment, I believe we will want to use
the async IPC.

https://codereview.chromium.org/1976703003/diff/500001/components/mus/public/...
File components/mus/public/interfaces/gpu_service.mojom (right):

https://codereview.chromium.org/1976703003/diff/500001/components/mus/public/...
components/mus/public/interfaces/gpu_service.mojom:18: => (int32 client_id,
ChannelHandle channel_handle, GpuInfo? gpu_info);
On 2016/06/03 17:34:48, dcheng wrote:
> Is GpuInfo nullable only because it's not yet implemented? Why add that
> parameter at all? Why would the receiver ever want to not return GpuInfo?

Yes. I changed it to nullbale because we don't have struct traits for
gpu::GPUInfo right now.

Change GPUInfo back. And use a empty GpuInfo for now.

Done

https://codereview.chromium.org/1976703003/diff/500001/components/mus/public/...
components/mus/public/interfaces/gpu_service.mojom:18: => (int32 client_id,
ChannelHandle channel_handle, GpuInfo? gpu_info);
On 2016/06/03 17:11:11, Tom Sepez wrote:
> How does it know that its less trustworthy client is telling the truth about
its
> client_id and isn't trying to create a conflict with an existing unrelated
> client?

With chrome IPC command buffer, the client_id is used by client to generate a
unique command buffer id, the command buffer id is used for generating
SyncToken. I think the gpu process should be able to verify synctoken in the
command buffer.

piman@, Could you please comment on it? Thanks.

[piman, 2016-06-03 19:20:47]

https://codereview.chromium.org/1976703003/diff/500001/components/mus/public/...
File components/mus/public/interfaces/gpu_service.mojom (right):

https://codereview.chromium.org/1976703003/diff/500001/components/mus/public/...
components/mus/public/interfaces/gpu_service.mojom:18: => (int32 client_id,
ChannelHandle channel_handle, GpuInfo? gpu_info);
On 2016/06/03 18:34:07, Peng wrote:
> On 2016/06/03 17:11:11, Tom Sepez wrote:
> > How does it know that its less trustworthy client is telling the truth about
> its
> > client_id and isn't trying to create a conflict with an existing unrelated
> > client?
> 
> With chrome IPC command buffer, the client_id is used by client to generate a
> unique command buffer id, the command buffer id is used for generating
> SyncToken. I think the gpu process should be able to verify synctoken in the
> command buffer.
> 
> piman@, Could you please comment on it? Thanks.

The sync token is meant to indicate a particular point in the command stream of
a given client. client_id/command_buffer_id/release_id is the "path" to that
point. This is used to allow synchronization between different command streams -
a given command stream can "wait" for a particular point (represented by the
sync token) in another command stream - which means the later commands of the
first command stream will only execute after the commands of the second stream
before that particular point.

The goal is to allow the browser compositor to present a fully formed frame from
its client. Missing synchronization would mean that the graphics resources from
that client would appear corrupted: unfinished rendering, or early reuse of the
resource. But it doesn't give that client access to resources it shouldn't have
access to, or allow it to elevate privilege or anything.

If an untrusted client generates a sync token with an invalid client id, one of
2 things could happen:
- the token references an invalid stream, in which case the wait is ignored
(i.e. no worse than missing synchronization)
- the wait creates a dependency loop. the code detects this, by guaranteeing a
global order, where a wait is ignored if it references a point that is not
before it in the global order. So again, no worse than missing synchronization.

See
https://docs.google.com/document/d/1XwBYFuTcINI84ShNvqifkPREs3sw5NdaKzKqDDxye...
for more details.

[Tom Sepez, 2016-06-03 21:33:46]

> See
>
https://docs.google.com/document/d/1XwBYFuTcINI84ShNvqifkPREs3sw5NdaKzKqDDxye...
> for more details.

Thanks for the explanation.  LGTM.

[Peng, 2016-06-03 21:37:51]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2016-06-03 21:37:52]

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

[commit-bot: I haz the power, 2016-06-03 21:38:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1976703003/540001

[commit-bot: I haz the power, 2016-06-04 01:33:57]

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

[commit-bot: I haz the power, 2016-06-04 01:33:58]

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

[Peng, 2016-06-04 03:05:13]

The CQ bit was checked by penghuang@chromium.org

[commit-bot: I haz the power, 2016-06-04 03:05:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1976703003/540001

[commit-bot: I haz the power, 2016-06-04 04:47:45]

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

[commit-bot: I haz the power, 2016-06-04 04:47:47]

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

[Peng, 2016-06-04 12:04:00]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2016-06-04 12:04:01]

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

[commit-bot: I haz the power, 2016-06-04 12:04:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1976703003/560001

[commit-bot: I haz the power, 2016-06-04 13:45:44]

Description was changed from

==========
Impl mus::mojom::GpuService to enable using Chrome IPC version gpu CmdBuf in mus

 * Implements mus::mojo::GpuService which allows clients requesting a gpu
IPC::ChannelHandle from mus, and then use it to construct GpuChannelHost and
CommandBufferProxyImpl.
 * Uses the Chrome IPC CmdBuf in mus::GLES2Context.
 * Adds --use-chrome-gpu-command-buffer-in-mus flag which is used for switching
between mojo CmdBuf and Chrome IPC CmdBuf.
 * Tested with ./out/mus_Debug/mojo_runner mojo:mus_demo 

Known issues:
 * The singleton mus::GpuService doesn't work for component build.
 * GpuMemoryBuffer is not implemented. The mash crashes because of it.

BUG=586390
==========

to

==========
Impl mus::mojom::GpuService to enable using Chrome IPC version gpu CmdBuf in mus

 * Implements mus::mojo::GpuService which allows clients requesting a gpu
IPC::ChannelHandle from mus, and then use it to construct GpuChannelHost and
CommandBufferProxyImpl.
 * Uses the Chrome IPC CmdBuf in mus::GLES2Context.
 * Adds --use-chrome-gpu-command-buffer-in-mus flag which is used for switching
between mojo CmdBuf and Chrome IPC CmdBuf.
 * Tested with ./out/mus_Debug/mojo_runner mojo:mus_demo 

Known issues:
 * The singleton mus::GpuService doesn't work for component build.
 * GpuMemoryBuffer is not implemented. The mash crashes because of it.

BUG=586390
==========

[commit-bot: I haz the power, 2016-06-04 13:45:46]

Committed patchset #24 (id:560001)

[commit-bot: I haz the power, 2016-06-04 13:47:01]

Description was changed from

==========
Impl mus::mojom::GpuService to enable using Chrome IPC version gpu CmdBuf in mus

 * Implements mus::mojo::GpuService which allows clients requesting a gpu
IPC::ChannelHandle from mus, and then use it to construct GpuChannelHost and
CommandBufferProxyImpl.
 * Uses the Chrome IPC CmdBuf in mus::GLES2Context.
 * Adds --use-chrome-gpu-command-buffer-in-mus flag which is used for switching
between mojo CmdBuf and Chrome IPC CmdBuf.
 * Tested with ./out/mus_Debug/mojo_runner mojo:mus_demo 

Known issues:
 * The singleton mus::GpuService doesn't work for component build.
 * GpuMemoryBuffer is not implemented. The mash crashes because of it.

BUG=586390
==========

to

==========
Impl mus::mojom::GpuService to enable using Chrome IPC version gpu CmdBuf in mus

 * Implements mus::mojo::GpuService which allows clients requesting a gpu
IPC::ChannelHandle from mus, and then use it to construct GpuChannelHost and
CommandBufferProxyImpl.
 * Uses the Chrome IPC CmdBuf in mus::GLES2Context.
 * Adds --use-chrome-gpu-command-buffer-in-mus flag which is used for switching
between mojo CmdBuf and Chrome IPC CmdBuf.
 * Tested with ./out/mus_Debug/mojo_runner mojo:mus_demo

Known issues:
 * The singleton mus::GpuService doesn't work for component build.
 * GpuMemoryBuffer is not implemented. The mash crashes because of it.

BUG=586390

Committed: https://crrev.com/591151b6e0b55a6e30dc7b789e6ffcd2da02e6cf
Cr-Commit-Position: refs/heads/master@{#397921}
==========

[commit-bot: I haz the power, 2016-06-04 13:47:03]

Patchset 24 (id:??) landed as
https://crrev.com/591151b6e0b55a6e30dc7b789e6ffcd2da02e6cf
Cr-Commit-Position: refs/heads/master@{#397921}