Decouple Media Service

Decouple Media Service

This CL moves media object creation IPCs into a separate
MediaChannel object that lives above GpuChannel.

content/gpu (or mus in the future) becomes the layer that
lives above both gpu and media and glues the two together.

This may not reflect what a hypothetical future mojo
interface might look like. If we want to preserve the
ordering guarantees we have now, media interfaces should be
associated interfaces that branch off a gpu channel and its
gpu interfaces. This seems to suggest that gpu should know
about media or else there should exist a factory interface
that glues both media and gpu as we have here.

I don't want to block this CL on long term mojo interface
discussions as any direction we choose is just a refactor
away. Until then, in the interest of making forward
progress, I'd like to assume there exists a layer above
both media and gpu.

Thoughts?

BUG=586384
Committed: https://crrev.com/028d76aa627788a7252fd6de39304f67e00837ab
Cr-Commit-Position: refs/heads/master@{#379211}

Committed: https://crrev.com/2ddf3b764c008dfe984af88c247a46815243ce16
Cr-Commit-Position: refs/heads/master@{#379411}

[Fady Samuel, 2016-02-26 02:49:52]

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

[Fady Samuel, 2016-02-26 02:49:53]

Hi Antoine,

This patch is not done yet but I wanted to get some early feedback.

I gave GpuCommandBufferStub a delegate that is constructed in GpuChildThread
(through the GpuChannelManagerDelegate interface).

The MediaService implements this GpuCommandBufferStubDelegate. It creates video
decoders and encoders. These services need to be tied to a particular command
buffer so this code still needs to be plumbed through CommandBufferStub
unfortunately.

Thoughts?

[Fady Samuel, 2016-02-26 02:52:18]

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

[commit-bot: I haz the power, 2016-02-26 02:56:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1736643005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1736643005/20001

[Fady Samuel, 2016-02-26 03:01:07]

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

[commit-bot: I haz the power, 2016-02-26 03:03:25]

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

[piman, 2016-02-26 03:38:53]

On Thu, Feb 25, 2016 at 6:49 PM, <fsamuel@chromium.org> wrote:

> Reviewers: piman
> CL: https://codereview.chromium.org/1736643005/
>
> Message:
> Hi Antoine,
>
> This patch is not done yet but I wanted to get some early feedback.
>
> I gave GpuCommandBufferStub a delegate that is constructed in
> GpuChildThread
> (through the GpuChannelManagerDelegate interface).
>
> The MediaService implements this GpuCommandBufferStubDelegate. It creates
> video
> decoders and encoders. These services need to be tied to a particular
> command
> buffer so this code still needs to be plumbed through CommandBufferStub
> unfortunately.
>
> Thoughts?
>

Maybe we could simplify a bit if we did everything inside of GpuChannel,
then we would only need one MediaService per channel (or maybe even only
one at all):
1- in GpuChildThread::OnEstablishChannel, it would create a MediaService
(or use a single one?) and attach it to the newly created GpuChannel (e.g.
GpuChannel::SetUnhandledMessageListener(IPC::Listener*))
2- in GpuChannel::HandleMessageHelper, if handled is false after routing
the message, it would try first to give it to its listener (the
MediaService).
3- the MediaService would look up the stub from the channel using the
routing id, and pass it to the GpuVideo*Accelerator

For (1) it would make it a bit easier
if GpuChannelManager::EstablishChannel would return the handle and
GpuChannel* instead of calling back via ChannelEstablished (simplifies
the GpuChannelManagerDelegate API as a side bonus).
WDYT?


> Description:
> [Work in Progress] Decouple Media Service
>
> BUG=
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+224, -7 lines):
> M base/tuple.h
> M content/common/gpu/gpu_channel.cc
> M content/common/gpu/gpu_channel_manager_delegate.h
> M content/common/gpu/gpu_channel_test_common.h
> M content/common/gpu/gpu_channel_test_common.cc
> M content/common/gpu/gpu_command_buffer_stub.h
> M content/common/gpu/gpu_command_buffer_stub.cc
> A content/common/gpu/gpu_command_buffer_stub_delegate.h
> A content/common/gpu/media/media_service.h
> A content/common/gpu/media/media_service.cc
> M content/content_common.gypi
> M content/gpu/gpu_child_thread.h
> M content/gpu/gpu_child_thread.cc
> M ipc/ipc_message_templates.h
>
>
>

-- 
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.

[Fady Samuel, 2016-02-29 21:26:14]

PTAL Antoine!

What do you think of this approach?

GpuChildThread has a MediaService object that manages MediaChannels which handle
unhandled GpuChannel messages. Thanks!

[Fady Samuel, 2016-02-29 21:47:15]

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

[commit-bot: I haz the power, 2016-02-29 21:47:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1736643005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1736643005/80001

[Fady Samuel, 2016-03-02 21:42:30]

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

[commit-bot: I haz the power, 2016-03-02 21:43:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1736643005/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1736643005/220001

[Fady Samuel, 2016-03-02 21:49:11]

Description was changed from

==========
[Work in Progress] Decouple Media Service

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

to

==========
Decouple Media Service

This CL moves media object creation IPCs into a separate
MediaChannel object that lives above GpuChannel.

content/gpu (or mus in the future) becomes the layer that
lives above both gpu and media and glues the two together.

This doesn't quite reflect what a hypothetical future mojo
interface might look like. If we want to preserve ordering
guarantees we have now, media interfaces should be
associated interfaces that branch off a gpu channel and its gpu interfaces. This
seems to suggest that gpu should know about media or else there should exist a
factory interface that glues both media and gpu as we have here. 

I don't want to block this CL on long term mojo interface discussions as any
direction we choose is just a refactor away. Until then, in the interest of
making forward progress, I'd assume there exists a layer above both media
and gpu.

BUG=586384
==========

[Fady Samuel, 2016-03-02 21:49:43]

Description was changed from

==========
Decouple Media Service

This CL moves media object creation IPCs into a separate
MediaChannel object that lives above GpuChannel.

content/gpu (or mus in the future) becomes the layer that
lives above both gpu and media and glues the two together.

This doesn't quite reflect what a hypothetical future mojo
interface might look like. If we want to preserve ordering
guarantees we have now, media interfaces should be
associated interfaces that branch off a gpu channel and its gpu interfaces. This
seems to suggest that gpu should know about media or else there should exist a
factory interface that glues both media and gpu as we have here. 

I don't want to block this CL on long term mojo interface discussions as any
direction we choose is just a refactor away. Until then, in the interest of
making forward progress, I'd assume there exists a layer above both media
and gpu.

BUG=586384
==========

to

==========
Decouple Media Service

This CL moves media object creation IPCs into a separate
MediaChannel object that lives above GpuChannel.

content/gpu (or mus in the future) becomes the layer that
lives above both gpu and media and glues the two together.

This doesn't quite reflect what a hypothetical future mojo
interface might look like. If we want to preserve ordering
guarantees we have now, media interfaces should be
associated interfaces that branch off a gpu channel and its
gpu interfaces. This seems to suggest that gpu should know
about media or else there should exist a factory interface
that glues both media and gpu as we have here. 

I don't want to block this CL on long term mojo interface discussions as any
direction we choose is just a refactor away. Until then, in the interest of
making forward progress, I'd assume there exists a layer above both media
and gpu.

BUG=586384
==========

[Fady Samuel, 2016-03-02 21:50:27]

Description was changed from

==========
Decouple Media Service

This CL moves media object creation IPCs into a separate
MediaChannel object that lives above GpuChannel.

content/gpu (or mus in the future) becomes the layer that
lives above both gpu and media and glues the two together.

This doesn't quite reflect what a hypothetical future mojo
interface might look like. If we want to preserve ordering
guarantees we have now, media interfaces should be
associated interfaces that branch off a gpu channel and its
gpu interfaces. This seems to suggest that gpu should know
about media or else there should exist a factory interface
that glues both media and gpu as we have here. 

I don't want to block this CL on long term mojo interface discussions as any
direction we choose is just a refactor away. Until then, in the interest of
making forward progress, I'd assume there exists a layer above both media
and gpu.

BUG=586384
==========

to

==========
Decouple Media Service

This CL moves media object creation IPCs into a separate
MediaChannel object that lives above GpuChannel.

content/gpu (or mus in the future) becomes the layer that
lives above both gpu and media and glues the two together.

This doesn't quite reflect what a hypothetical future mojo
interface might look like. If we want to preserve ordering
guarantees we have now, media interfaces should be
associated interfaces that branch off a gpu channel and its
gpu interfaces. This seems to suggest that gpu should know
about media or else there should exist a factory interface
that glues both media and gpu as we have here. 

I don't want to block this CL on long term mojo interface
discussions as any direction we choose is just a refactor
away. Until then, in the interest of making forward
progress, I'd like to assume there exists a layer above
both media and gpu.

Thoughts?

BUG=586384
==========

[Fady Samuel, 2016-03-02 21:51:18]

Description was changed from

==========
Decouple Media Service

This CL moves media object creation IPCs into a separate
MediaChannel object that lives above GpuChannel.

content/gpu (or mus in the future) becomes the layer that
lives above both gpu and media and glues the two together.

This doesn't quite reflect what a hypothetical future mojo
interface might look like. If we want to preserve ordering
guarantees we have now, media interfaces should be
associated interfaces that branch off a gpu channel and its
gpu interfaces. This seems to suggest that gpu should know
about media or else there should exist a factory interface
that glues both media and gpu as we have here. 

I don't want to block this CL on long term mojo interface
discussions as any direction we choose is just a refactor
away. Until then, in the interest of making forward
progress, I'd like to assume there exists a layer above
both media and gpu.

Thoughts?

BUG=586384
==========

to

==========
Decouple Media Service

This CL moves media object creation IPCs into a separate
MediaChannel object that lives above GpuChannel.

content/gpu (or mus in the future) becomes the layer that
lives above both gpu and media and glues the two together.

This may not reflect what a hypothetical future mojo
interface might look like. If we want to preserve the
ordering guarantees we have now, media interfaces should be
associated interfaces that branch off a gpu channel and its
gpu interfaces. This seems to suggest that gpu should know
about media or else there should exist a factory interface
that glues both media and gpu as we have here. 

I don't want to block this CL on long term mojo interface
discussions as any direction we choose is just a refactor
away. Until then, in the interest of making forward
progress, I'd like to assume there exists a layer above
both media and gpu.

Thoughts?

BUG=586384
==========

[Fady Samuel, 2016-03-02 21:52:10]

PTAL Antoine, I think this patch is now ready for your review.

[commit-bot: I haz the power, 2016-03-02 22:18:08]

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

[commit-bot: I haz the power, 2016-03-02 22:18:09]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Fady Samuel, 2016-03-02 23:05:50]

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

[commit-bot: I haz the power, 2016-03-02 23:06:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1736643005/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1736643005/240001

[commit-bot: I haz the power, 2016-03-02 23:48:26]

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

[commit-bot: I haz the power, 2016-03-02 23:48:27]

Dry run: 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...)

[Fady Samuel, 2016-03-03 01:40:32]

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

[commit-bot: I haz the power, 2016-03-03 01:40:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1736643005/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1736643005/260001

[commit-bot: I haz the power, 2016-03-03 02:20:38]

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

[commit-bot: I haz the power, 2016-03-03 02:20:40]

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

[Fady Samuel, 2016-03-03 02:21:34]

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

[commit-bot: I haz the power, 2016-03-03 02:22:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1736643005/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1736643005/260001

[commit-bot: I haz the power, 2016-03-03 02:50:37]

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

[commit-bot: I haz the power, 2016-03-03 02:50:38]

Dry run: 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...)

[Fady Samuel, 2016-03-03 17:39:53]

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

[commit-bot: I haz the power, 2016-03-03 17:40:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1736643005/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1736643005/280001

[commit-bot: I haz the power, 2016-03-03 18:50:15]

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

[commit-bot: I haz the power, 2016-03-03 18:50:16]

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

[Fady Samuel, 2016-03-03 18:58:35]

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

[commit-bot: I haz the power, 2016-03-03 18:58:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1736643005/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1736643005/300001

[piman, 2016-03-03 20:09:59]

LGTM, this is great.

[Fady Samuel, 2016-03-03 20:29:46]

+danakj@ for base/tuple.h
+jam@ for ipc/ipc_message_templates.h

IPC_BEGIN_MESSAGE_MAP_WITH_PARAM didn't play nice with
IPC_MESSAGE_HANDLER_DELAY_REPLY.

Unfortunately, as far as I can tell, VC2013 doesn't like n-ary expressions in
std::enable_if so the template parameters are not ideal for DispatchToMethod but
it serves its purpose on all platforms now.

[Fady Samuel, 2016-03-03 20:30:13]

fsamuel@chromium.org changed reviewers:
+ danakj@chromium.org, jam@chromium.org

[Fady Samuel, 2016-03-03 20:30:14]

+jam@, +danakj@ for real.

[danakj, 2016-03-03 21:00:28]

https://codereview.chromium.org/1736643005/diff/300001/base/tuple.h
File base/tuple.h (right):

https://codereview.chromium.org/1736643005/diff/300001/base/tuple.h#newcode252
base/tuple.h:252: typename std::enable_if<!std::is_void<P>::value>::type* =
nullptr>
I know you're working around msvc bugs but I don't agree with this being in the
base library, it has some pretty strange behavior wrt void*.

I'd also like to not make these Dispatch things more complicated, and prefer you
to just do something one-off and preferably not templated where you want to use
this if possible.

[danakj, 2016-03-03 21:01:40]

https://codereview.chromium.org/1736643005/diff/300001/ipc/ipc_message_templa...
File ipc/ipc_message_templates.h (right):

https://codereview.chromium.org/1736643005/diff/300001/ipc/ipc_message_templa...
ipc/ipc_message_templates.h:45: DispatchToMethod(ObjT* obj,
Or use this?

[commit-bot: I haz the power, 2016-03-03 21:37:45]

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

[commit-bot: I haz the power, 2016-03-03 21:37:46]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2016-03-03 23:23:19]

fsamuel@chromium.org changed reviewers:
+ dcheng@chromium.org
- danakj@chromium.org, jam@chromium.org

[Fady Samuel, 2016-03-03 23:23:20]

-jam@, -danakj@, +dcheng@ for _message* files: just moving some messages around.

[Fady Samuel, 2016-03-03 23:25:36]

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

[commit-bot: I haz the power, 2016-03-03 23:26:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1736643005/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1736643005/320001

[dcheng, 2016-03-03 23:46:43]

https://codereview.chromium.org/1736643005/diff/320001/content/common/gpu/med...
File content/common/gpu/media/media_channel.cc (right):

https://codereview.chromium.org/1736643005/diff/320001/content/common/gpu/med...
content/common/gpu/media/media_channel.cc:44:
IPC_MESSAGE_HANDLER_GENERIC(GpuCommandBufferMsg_CreateVideoDecoder,
Why IPC_MESSAGE_HANDLER_GENERIC? Why not use the macros that automatically
unpickle the message?

https://codereview.chromium.org/1736643005/diff/320001/content/common/gpu/med...
File content/common/gpu/media/media_service.cc (right):

https://codereview.chromium.org/1736643005/diff/320001/content/common/gpu/med...
content/common/gpu/media/media_service.cc:27: media_channels_.set(client_id,
std::move(media_channel));
#include <utility>

https://codereview.chromium.org/1736643005/diff/320001/content/common/gpu/med...
File content/common/gpu/media/media_service.h (right):

https://codereview.chromium.org/1736643005/diff/320001/content/common/gpu/med...
content/common/gpu/media/media_service.h:25: void AddChannel(int32_t client_id);
#include <stdint.h>

[Fady Samuel, 2016-03-04 01:46:22]

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

[Fady Samuel, 2016-03-04 01:46:32]

PTAL Daniel! I switched to control messages!

https://codereview.chromium.org/1736643005/diff/320001/content/common/gpu/med...
File content/common/gpu/media/media_channel.cc (right):

https://codereview.chromium.org/1736643005/diff/320001/content/common/gpu/med...
content/common/gpu/media/media_channel.cc:44:
IPC_MESSAGE_HANDLER_GENERIC(GpuCommandBufferMsg_CreateVideoDecoder,
On 2016/03/03 23:46:42, dcheng wrote:
> Why IPC_MESSAGE_HANDLER_GENERIC? Why not use the macros that automatically
> unpickle the message?

Switched to control messages.

https://codereview.chromium.org/1736643005/diff/320001/content/common/gpu/med...
File content/common/gpu/media/media_service.cc (right):

https://codereview.chromium.org/1736643005/diff/320001/content/common/gpu/med...
content/common/gpu/media/media_service.cc:27: media_channels_.set(client_id,
std::move(media_channel));
On 2016/03/03 23:46:42, dcheng wrote:
> #include <utility>

Done.

https://codereview.chromium.org/1736643005/diff/320001/content/common/gpu/med...
File content/common/gpu/media/media_service.h (right):

https://codereview.chromium.org/1736643005/diff/320001/content/common/gpu/med...
content/common/gpu/media/media_service.h:25: void AddChannel(int32_t client_id);
On 2016/03/03 23:46:42, dcheng wrote:
> #include <stdint.h>

Done.

[commit-bot: I haz the power, 2016-03-04 01:46:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1736643005/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1736643005/360001

[dcheng, 2016-03-04 02:00:11]

https://codereview.chromium.org/1736643005/diff/360001/content/common/gpu/med...
File content/common/gpu/media/create_video_encoder_params.h (right):

https://codereview.chromium.org/1736643005/diff/360001/content/common/gpu/med...
content/common/gpu/media/create_video_encoder_params.h:15:
CreateVideoEncoderParams();
Formatting.

https://codereview.chromium.org/1736643005/diff/360001/content/common/gpu/med...
content/common/gpu/media/create_video_encoder_params.h:25: }  // namespace
content
Nit: newline after for consistency

https://codereview.chromium.org/1736643005/diff/360001/content/common/gpu/med...
File content/common/gpu/media/media_channel.h (right):

https://codereview.chromium.org/1736643005/diff/360001/content/common/gpu/med...
content/common/gpu/media/media_channel.h:33: void OnCreateVideoDecoder(int
command_buffer_route_id,
Nit: int32_t here to match message definition.

https://codereview.chromium.org/1736643005/diff/360001/content/common/gpu/med...
File content/common/gpu/media/media_messages.h (right):

https://codereview.chromium.org/1736643005/diff/360001/content/common/gpu/med...
content/common/gpu/media/media_messages.h:124:
IPC_SYNC_MESSAGE_CONTROL3_1(GpuCommandBufferMsg_CreateVideoDecoder,
Btw, are these renderer to GPU messages? Or something else? Can we document that
here?

(It's hard for me to tell, since so much of the gpu bits live in content/common)

[commit-bot: I haz the power, 2016-03-04 02:09:04]

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

[commit-bot: I haz the power, 2016-03-04 02:09:05]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Fady Samuel, 2016-03-04 02:29:51]

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

[Fady Samuel, 2016-03-04 02:30:00]

PTAL Daniel!

https://codereview.chromium.org/1736643005/diff/360001/content/common/gpu/med...
File content/common/gpu/media/create_video_encoder_params.h (right):

https://codereview.chromium.org/1736643005/diff/360001/content/common/gpu/med...
content/common/gpu/media/create_video_encoder_params.h:15:
CreateVideoEncoderParams();
On 2016/03/04 02:00:11, dcheng wrote:
> Formatting.

Done.

https://codereview.chromium.org/1736643005/diff/360001/content/common/gpu/med...
content/common/gpu/media/create_video_encoder_params.h:25: }  // namespace
content
On 2016/03/04 02:00:11, dcheng wrote:
> Nit: newline after for consistency

Done.

https://codereview.chromium.org/1736643005/diff/360001/content/common/gpu/med...
File content/common/gpu/media/media_channel.h (right):

https://codereview.chromium.org/1736643005/diff/360001/content/common/gpu/med...
content/common/gpu/media/media_channel.h:33: void OnCreateVideoDecoder(int
command_buffer_route_id,
On 2016/03/04 02:00:11, dcheng wrote:
> Nit: int32_t here to match message definition.

Done.

https://codereview.chromium.org/1736643005/diff/360001/content/common/gpu/med...
File content/common/gpu/media/media_messages.h (right):

https://codereview.chromium.org/1736643005/diff/360001/content/common/gpu/med...
content/common/gpu/media/media_messages.h:124:
IPC_SYNC_MESSAGE_CONTROL3_1(GpuCommandBufferMsg_CreateVideoDecoder,
On 2016/03/04 02:00:11, dcheng wrote:
> Btw, are these renderer to GPU messages? Or something else? Can we document
that
> here?
> 
> (It's hard for me to tell, since so much of the gpu bits live in
content/common)

Good point! I moved them to their appropriate sections in the file.

[commit-bot: I haz the power, 2016-03-04 02:30:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1736643005/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1736643005/380001

[Fady Samuel, 2016-03-04 02:51:52]

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

[commit-bot: I haz the power, 2016-03-04 02:52:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1736643005/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1736643005/400001

[dcheng, 2016-03-04 02:58:11]

lgtm

[Fady Samuel, 2016-03-04 03:10:51]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-03-04 03:10:51]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1736643005/#ps420001
(title: "Removed a bad DCHECK")

[commit-bot: I haz the power, 2016-03-04 03:11:12]

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

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

Description was changed from

==========
Decouple Media Service

This CL moves media object creation IPCs into a separate
MediaChannel object that lives above GpuChannel.

content/gpu (or mus in the future) becomes the layer that
lives above both gpu and media and glues the two together.

This may not reflect what a hypothetical future mojo
interface might look like. If we want to preserve the
ordering guarantees we have now, media interfaces should be
associated interfaces that branch off a gpu channel and its
gpu interfaces. This seems to suggest that gpu should know
about media or else there should exist a factory interface
that glues both media and gpu as we have here. 

I don't want to block this CL on long term mojo interface
discussions as any direction we choose is just a refactor
away. Until then, in the interest of making forward
progress, I'd like to assume there exists a layer above
both media and gpu.

Thoughts?

BUG=586384
==========

to

==========
Decouple Media Service

This CL moves media object creation IPCs into a separate
MediaChannel object that lives above GpuChannel.

content/gpu (or mus in the future) becomes the layer that
lives above both gpu and media and glues the two together.

This may not reflect what a hypothetical future mojo
interface might look like. If we want to preserve the
ordering guarantees we have now, media interfaces should be
associated interfaces that branch off a gpu channel and its
gpu interfaces. This seems to suggest that gpu should know
about media or else there should exist a factory interface
that glues both media and gpu as we have here. 

I don't want to block this CL on long term mojo interface
discussions as any direction we choose is just a refactor
away. Until then, in the interest of making forward
progress, I'd like to assume there exists a layer above
both media and gpu.

Thoughts?

BUG=586384
==========

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

Committed patchset #22 (id:420001)

[commit-bot: I haz the power, 2016-03-04 04:34:32]

Description was changed from

==========
Decouple Media Service

This CL moves media object creation IPCs into a separate
MediaChannel object that lives above GpuChannel.

content/gpu (or mus in the future) becomes the layer that
lives above both gpu and media and glues the two together.

This may not reflect what a hypothetical future mojo
interface might look like. If we want to preserve the
ordering guarantees we have now, media interfaces should be
associated interfaces that branch off a gpu channel and its
gpu interfaces. This seems to suggest that gpu should know
about media or else there should exist a factory interface
that glues both media and gpu as we have here. 

I don't want to block this CL on long term mojo interface
discussions as any direction we choose is just a refactor
away. Until then, in the interest of making forward
progress, I'd like to assume there exists a layer above
both media and gpu.

Thoughts?

BUG=586384
==========

to

==========
Decouple Media Service

This CL moves media object creation IPCs into a separate
MediaChannel object that lives above GpuChannel.

content/gpu (or mus in the future) becomes the layer that
lives above both gpu and media and glues the two together.

This may not reflect what a hypothetical future mojo
interface might look like. If we want to preserve the
ordering guarantees we have now, media interfaces should be
associated interfaces that branch off a gpu channel and its
gpu interfaces. This seems to suggest that gpu should know
about media or else there should exist a factory interface
that glues both media and gpu as we have here.

I don't want to block this CL on long term mojo interface
discussions as any direction we choose is just a refactor
away. Until then, in the interest of making forward
progress, I'd like to assume there exists a layer above
both media and gpu.

Thoughts?

BUG=586384

Committed: https://crrev.com/028d76aa627788a7252fd6de39304f67e00837ab
Cr-Commit-Position: refs/heads/master@{#379211}
==========

[commit-bot: I haz the power, 2016-03-04 04:34:34]

Patchset 22 (id:??) landed as
https://crrev.com/028d76aa627788a7252fd6de39304f67e00837ab
Cr-Commit-Position: refs/heads/master@{#379211}

[Jamie Madill, 2016-03-04 16:13:26]

On 2016/03/04 04:34:34, commit-bot: I haz the power wrote:
> Patchset 22 (id:??) landed as
> https://crrev.com/028d76aa627788a7252fd6de39304f67e00837ab
> Cr-Commit-Position: refs/heads/master@{#379211}

I think this broke the Windows 8 Debug GPU FYI bot WebGL video tests, see first
failing build:

https://build.chromium.org/p/chromium.gpu.fyi/builders/Win8%20Debug%20%28NVID...

Might have to revert.

[Jamie Madill, 2016-03-04 16:22:10]

A revert of this CL (patchset #22 id:420001) has been created in
https://codereview.chromium.org/1767673002/ by jmadill@chromium.org.

The reason for reverting is: Likely causing Windows 8 Debug GPU FYI WebGL video
test failures.

First failing build:

https://build.chromium.org/p/chromium.gpu.fyi/builders/Win8%20Debug%20%28NVID...

Regression range:

https://chromium.googlesource.com/chromium/src/+log/643dba13a5f5af08e4aed492e...

Failing tests:

WebglConformance.conformance2_textures_video_tex_image_and_sub_image_2d_with_video_r8_red_unsigned_byte
WebglConformance.conformance2_textures_video_tex_image_and_sub_image_2d_with_video_r16f_red_half_float
WebglConformance.conformance2_textures_video_tex_image_and_sub_image_2d_with_video_r16f_red_float
WebglConformance.conformance2_textures_video_tex_image_and_sub_image_2d_with_video_r32f_red_float
WebglConformance.conformance2_textures_video_tex_image_and_sub_image_2d_with_video_r8ui_red_integer_unsigned_byte
WebglConformance.conformance2_textures_video_tex_image_and_sub_image_2d_with_video_rg8_rg_unsigned_byte
WebglConformance.conformance2_textures_video_tex_image_and_sub_image_2d_with_video_rg16f_rg_half_float
WebglConformance.conformance2_textures_video_tex_image_and_sub_image_2d_with_video_rg16f_rg_float
WebglConformance.conformance2_textures_video_tex_image_and_sub_image_2d_with_video_rg32f_rg_float
WebglConformance.conformance2_textures_video_tex_image_and_sub_image_2d_with_video_rg8ui_rg_integer_unsigned_byte
WebglConformance.conformance2_textures_video_tex_image_and_sub_image_2d_with_video_rgb8_rgb_unsigned_byte

WebglConformance.conformance_extensions_oes_texture_float_with_video
WebglConformance.conformance_extensions_oes_texture_half_float_with_video
WebglConformance.conformance_textures_misc_texture_upload_size
WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_rgb_rgb_unsigned_byte
WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_rgb_rgb_unsigned_short_5_6_5
WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_rgba_rgba_unsigned_byte
WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_rgba_rgba_unsigned_short_4_4_4_4
WebglConformance.conformance_textures_video_tex_image_and_sub_image_2d_with_video_rgba_rgba_unsigned_short_5_5_5_1

BUG=592015.

[piman, 2016-03-04 20:13:28]

https://codereview.chromium.org/1736643005/diff/320001/content/common/gpu/med...
File content/common/gpu/media/media_channel.cc (right):

https://codereview.chromium.org/1736643005/diff/320001/content/common/gpu/med...
content/common/gpu/media/media_channel.cc:44:
IPC_MESSAGE_HANDLER_GENERIC(GpuCommandBufferMsg_CreateVideoDecoder,
On 2016/03/04 01:46:32, Fady Samuel wrote:
> On 2016/03/03 23:46:42, dcheng wrote:
> > Why IPC_MESSAGE_HANDLER_GENERIC? Why not use the macros that automatically
> > unpickle the message?
> 
> Switched to control messages.

You can't do that. That's probably related to the issues that caused the revert.

We use routing ids to order messages relative to their stream. E.g. this
guarantees that GpuCommandBufferMsg_CreateVideoDecoder is well ordered relative
to the Create*Context message.

[Fady Samuel, 2016-03-04 22:13:46]

Description was changed from

==========
Decouple Media Service

This CL moves media object creation IPCs into a separate
MediaChannel object that lives above GpuChannel.

content/gpu (or mus in the future) becomes the layer that
lives above both gpu and media and glues the two together.

This may not reflect what a hypothetical future mojo
interface might look like. If we want to preserve the
ordering guarantees we have now, media interfaces should be
associated interfaces that branch off a gpu channel and its
gpu interfaces. This seems to suggest that gpu should know
about media or else there should exist a factory interface
that glues both media and gpu as we have here.

I don't want to block this CL on long term mojo interface
discussions as any direction we choose is just a refactor
away. Until then, in the interest of making forward
progress, I'd like to assume there exists a layer above
both media and gpu.

Thoughts?

BUG=586384

Committed: https://crrev.com/028d76aa627788a7252fd6de39304f67e00837ab
Cr-Commit-Position: refs/heads/master@{#379211}
==========

to

==========
Decouple Media Service

This CL moves media object creation IPCs into a separate
MediaChannel object that lives above GpuChannel.

content/gpu (or mus in the future) becomes the layer that
lives above both gpu and media and glues the two together.

This may not reflect what a hypothetical future mojo
interface might look like. If we want to preserve the
ordering guarantees we have now, media interfaces should be
associated interfaces that branch off a gpu channel and its
gpu interfaces. This seems to suggest that gpu should know
about media or else there should exist a factory interface
that glues both media and gpu as we have here.

I don't want to block this CL on long term mojo interface
discussions as any direction we choose is just a refactor
away. Until then, in the interest of making forward
progress, I'd like to assume there exists a layer above
both media and gpu.

Thoughts?

BUG=586384

Committed: https://crrev.com/028d76aa627788a7252fd6de39304f67e00837ab
Cr-Commit-Position: refs/heads/master@{#379211}
==========

[Fady Samuel, 2016-03-04 22:53:59]

PTAL Antoine and Daniel! I've taken the approach we discussed online! Thanks for
your help!

[Fady Samuel, 2016-03-04 22:54:04]

PTAL Antoine and Daniel! I've taken the approach we discussed offline! Thanks
for your help!

[Fady Samuel, 2016-03-04 22:54:51]

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

[commit-bot: I haz the power, 2016-03-04 22:55:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1736643005/440001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1736643005/440001

[piman, 2016-03-04 23:02:04]

lgtm

[commit-bot: I haz the power, 2016-03-04 23:07:37]

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

[commit-bot: I haz the power, 2016-03-04 23:07:39]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[dcheng, 2016-03-04 23:07:39]

lgtm with nits, thanks

https://codereview.chromium.org/1736643005/diff/440001/content/common/gpu/med...
File content/common/gpu/media/media_channel.cc (right):

https://codereview.chromium.org/1736643005/diff/440001/content/common/gpu/med...
content/common/gpu/media/media_channel.cc:120: void
MediaChannel::OnCreateVideoEncoder(int command_buffer_route_id,
Nit: int32_t

[Fady Samuel, 2016-03-04 23:19:01]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-03-04 23:19:02]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1736643005/#ps450001
(title: "Addressed nit + fixed compile issue")

[commit-bot: I haz the power, 2016-03-04 23:19:23]

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

[commit-bot: I haz the power, 2016-03-05 00:32:21]

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

[commit-bot: I haz the power, 2016-03-05 00:32:23]

Failed to apply patch for content/common/gpu/media/media_messages.cc:
While running git apply --index -3 -p1;
  error: content/common/gpu/media_messages.cc: does not exist in index

Patch:   NR 
content/common/gpu/media_messages.cc->content/common/gpu/media/media_messages.cc
Index: content/common/gpu/media/media_messages.cc
diff --git a/content/common/gpu/media_messages.cc
b/content/common/gpu/media/media_messages.cc
similarity index 97%
rename from content/common/gpu/media_messages.cc
rename to content/common/gpu/media/media_messages.cc
index
74bd702850ad978d692deaa4ee6a5d2f256dc566..88364d9465eeb6029d1d6ee58151f4ed8a190fca
100644
--- a/content/common/gpu/media_messages.cc
+++ b/content/common/gpu/media/media_messages.cc
@@ -8,7 +8,7 @@
 
 #include "base/logging.h"
 #include "base/numerics/safe_math.h"
-#include "content/common/gpu/media_messages.h"
+#include "content/common/gpu/media/media_messages.h"
 
 namespace IPC {

[commit-bot: I haz the power, 2016-03-05 00:32:33]

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

[commit-bot: I haz the power, 2016-03-05 00:32:44]

Description was changed from

==========
Decouple Media Service

This CL moves media object creation IPCs into a separate
MediaChannel object that lives above GpuChannel.

content/gpu (or mus in the future) becomes the layer that
lives above both gpu and media and glues the two together.

This may not reflect what a hypothetical future mojo
interface might look like. If we want to preserve the
ordering guarantees we have now, media interfaces should be
associated interfaces that branch off a gpu channel and its
gpu interfaces. This seems to suggest that gpu should know
about media or else there should exist a factory interface
that glues both media and gpu as we have here.

I don't want to block this CL on long term mojo interface
discussions as any direction we choose is just a refactor
away. Until then, in the interest of making forward
progress, I'd like to assume there exists a layer above
both media and gpu.

Thoughts?

BUG=586384

Committed: https://crrev.com/028d76aa627788a7252fd6de39304f67e00837ab
Cr-Commit-Position: refs/heads/master@{#379211}
==========

to

==========
Decouple Media Service

This CL moves media object creation IPCs into a separate
MediaChannel object that lives above GpuChannel.

content/gpu (or mus in the future) becomes the layer that
lives above both gpu and media and glues the two together.

This may not reflect what a hypothetical future mojo
interface might look like. If we want to preserve the
ordering guarantees we have now, media interfaces should be
associated interfaces that branch off a gpu channel and its
gpu interfaces. This seems to suggest that gpu should know
about media or else there should exist a factory interface
that glues both media and gpu as we have here.

I don't want to block this CL on long term mojo interface
discussions as any direction we choose is just a refactor
away. Until then, in the interest of making forward
progress, I'd like to assume there exists a layer above
both media and gpu.

Thoughts?

BUG=586384

Committed: https://crrev.com/028d76aa627788a7252fd6de39304f67e00837ab
Cr-Commit-Position: refs/heads/master@{#379211}

Committed: https://crrev.com/2ddf3b764c008dfe984af88c247a46815243ce16
Cr-Commit-Position: refs/heads/master@{#379411}
==========

[commit-bot: I haz the power, 2016-03-05 00:32:46]

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