MediaService: Implement GetChannelToken IPC

MediaService: Implement GetChannelToken IPC

This new IPC provides a non-forgeable token identifying a GpuChannel so
that it can be passed to Mojo services that want to share the same
channel in the GPU process (namely VDAv2 in the MojoMediaApplication).

For simplicity, the IPC is sync. It is handled on the IO thread in the
GPU process, so latency is expected to be manageable (and vastly better
that the already sync CreateVideoDecodeAccelerator IPC).

BUG=522298
Committed: https://crrev.com/777e4aae05428625f99dcabddfdd4b581d6516d5
Cr-Commit-Position: refs/heads/master@{#419826}

[sandersd (OOO until July 31), 2016-09-16 00:30:42]

sandersd@chromium.org changed reviewers:
+ dcheng@chromium.org, tguilbert@chromium.org

[sandersd (OOO until July 31), 2016-09-16 00:31:54]

Description was changed from

==========
MediaService: Implement GetChannelToken IPC

This new IPC provides a non-forgeable token identifying a GpuChannel so
that it can be passed to Mojo services that want to share the same
channel in the GPU process (namely VDAv2 in the MojoMediaApplication).

For simplicity, the IPC is sync. It is handled on the IO thread in the
GPU process, so latency is expected to be manageable (and vastly better
that the already sync CreateVideoDecodeAccelerator IPC).

The return type, uint64_t, is not what will be shipping; it's just a
workable temporary type that allows progress on VDAv2. It will be
replaced with a 128-bit nonce before shipping VDAv2. (See issue
2333443002.)

BUG=522298
==========

to

==========
MediaService: Implement GetChannelToken IPC

This new IPC provides a non-forgeable token identifying a GpuChannel so
that it can be passed to Mojo services that want to share the same
channel in the GPU process (namely VDAv2 in the MojoMediaApplication).

For simplicity, the IPC is sync. It is handled on the IO thread in the
GPU process, so latency is expected to be manageable (and vastly better
that the already sync CreateVideoDecodeAccelerator IPC).

The return type, uint64_t, is not what will be shipping; it's just a
workable temporary type that allows progress on VDAv2. It will be
replaced with a 128-bit nonce before shipping VDAv2. (c.f.
https://codereview.chromium.org/2333443002/)

BUG=522298
==========

[dcheng, 2016-09-16 19:20:42]

https://codereview.chromium.org/2349463003/diff/1/media/gpu/ipc/common/media_...
File media/gpu/ipc/common/media_messages.h (right):

https://codereview.chromium.org/2349463003/diff/1/media/gpu/ipc/common/media_...
media/gpu/ipc/common/media_messages.h:55: uint64_t /* channel_token */)
NonceToken is going to land shortly (hopefully today with any luck), would it be
OK to wait for that?

[sandersd (OOO until July 31), 2016-09-16 19:49:32]

On 2016/09/16 19:20:42, dcheng wrote:
>
https://codereview.chromium.org/2349463003/diff/1/media/gpu/ipc/common/media_...
> File media/gpu/ipc/common/media_messages.h (right):
> 
>
https://codereview.chromium.org/2349463003/diff/1/media/gpu/ipc/common/media_...
> media/gpu/ipc/common/media_messages.h:55: uint64_t /* channel_token */)
> NonceToken is going to land shortly (hopefully today with any luck), would it
be
> OK to wait for that?

No objection, but I don't want to wait too long if the bikeshedding continues.

[sandersd (OOO until July 31), 2016-09-19 22:21:25]

Patchset #4 (id:60001) has been deleted

[sandersd (OOO until July 31), 2016-09-19 22:21:40]

Description was changed from

==========
MediaService: Implement GetChannelToken IPC

This new IPC provides a non-forgeable token identifying a GpuChannel so
that it can be passed to Mojo services that want to share the same
channel in the GPU process (namely VDAv2 in the MojoMediaApplication).

For simplicity, the IPC is sync. It is handled on the IO thread in the
GPU process, so latency is expected to be manageable (and vastly better
that the already sync CreateVideoDecodeAccelerator IPC).

The return type, uint64_t, is not what will be shipping; it's just a
workable temporary type that allows progress on VDAv2. It will be
replaced with a 128-bit nonce before shipping VDAv2. (c.f.
https://codereview.chromium.org/2333443002/)

BUG=522298
==========

to

==========
MediaService: Implement GetChannelToken IPC

This new IPC provides a non-forgeable token identifying a GpuChannel so
that it can be passed to Mojo services that want to share the same
channel in the GPU process (namely VDAv2 in the MojoMediaApplication).

For simplicity, the IPC is sync. It is handled on the IO thread in the
GPU process, so latency is expected to be manageable (and vastly better
that the already sync CreateVideoDecodeAccelerator IPC).

BUG=522298
==========

[sandersd (OOO until July 31), 2016-09-19 22:21:51]

PTAL.

[tguilbert, 2016-09-19 22:32:23]

LGTM % nits

(I am technically still not a full chromium committer however... So my LGTM is
not an official one :))

https://codereview.chromium.org/2349463003/diff/80001/media/gpu/ipc/common/me...
File media/gpu/ipc/common/media_messages.h (right):

https://codereview.chromium.org/2349463003/diff/80001/media/gpu/ipc/common/me...
media/gpu/ipc/common/media_messages.h:8: #include <stdint.h>
No longer needed.

https://codereview.chromium.org/2349463003/diff/80001/media/gpu/ipc/common/me...
media/gpu/ipc/common/media_messages.h:54: // TODO(sandersd): Switch to
base::NonceToken once that exists.
Remove the TODO.

https://codereview.chromium.org/2349463003/diff/80001/media/gpu/ipc/service/m...
File media/gpu/ipc/service/media_channel.cc (right):

https://codereview.chromium.org/2349463003/diff/80001/media/gpu/ipc/service/m...
media/gpu/ipc/service/media_channel.cc:63: explicit
MediaChannelFilter(base::UnguessableToken channel_token)
I think the official guidance danakj gave us is to use const ref (they had
tested the performance difference with gfx::size in a micro benchmark, and found
that it was still better to use a const ref for anything other than a built-in
type).

[sandersd (OOO until July 31), 2016-09-19 22:40:33]

https://codereview.chromium.org/2349463003/diff/80001/media/gpu/ipc/common/me...
File media/gpu/ipc/common/media_messages.h (right):

https://codereview.chromium.org/2349463003/diff/80001/media/gpu/ipc/common/me...
media/gpu/ipc/common/media_messages.h:8: #include <stdint.h>
On 2016/09/19 22:32:22, ThomasGuilbert wrote:
> No longer needed.

It was needed before, just missing.

https://codereview.chromium.org/2349463003/diff/80001/media/gpu/ipc/common/me...
media/gpu/ipc/common/media_messages.h:54: // TODO(sandersd): Switch to
base::NonceToken once that exists.
On 2016/09/19 22:32:22, ThomasGuilbert wrote:
> Remove the TODO.

Done.

https://codereview.chromium.org/2349463003/diff/80001/media/gpu/ipc/service/m...
File media/gpu/ipc/service/media_channel.cc (right):

https://codereview.chromium.org/2349463003/diff/80001/media/gpu/ipc/service/m...
media/gpu/ipc/service/media_channel.cc:63: explicit
MediaChannelFilter(base::UnguessableToken channel_token)
On 2016/09/19 22:32:23, ThomasGuilbert wrote:
> I think the official guidance danakj gave us is to use const ref (they had
> tested the performance difference with gfx::size in a micro benchmark, and
found
> that it was still better to use a const ref for anything other than a built-in
> type).

Very well then.

[dcheng, 2016-09-19 22:47:35]

https://codereview.chromium.org/2349463003/diff/100001/media/gpu/ipc/service/...
File media/gpu/ipc/service/media_channel.cc (right):

https://codereview.chromium.org/2349463003/diff/100001/media/gpu/ipc/service/...
media/gpu/ipc/service/media_channel.cc:66: void OnFilterAdded(IPC::Channel*
channel) override { channel_ = channel; }
Do we need this override?

[sandersd (OOO until July 31), 2016-09-19 22:55:49]

https://codereview.chromium.org/2349463003/diff/100001/media/gpu/ipc/service/...
File media/gpu/ipc/service/media_channel.cc (right):

https://codereview.chromium.org/2349463003/diff/100001/media/gpu/ipc/service/...
media/gpu/ipc/service/media_channel.cc:66: void OnFilterAdded(IPC::Channel*
channel) override { channel_ = channel; }
On 2016/09/19 22:47:35, dcheng wrote:
> Do we need this override?

No, it could have been safely passed AFAIK. Is there a reason why passing would
be preferred?

[dcheng, 2016-09-19 23:05:58]

lgtm

https://codereview.chromium.org/2349463003/diff/100001/media/gpu/ipc/service/...
File media/gpu/ipc/service/media_channel.cc (right):

https://codereview.chromium.org/2349463003/diff/100001/media/gpu/ipc/service/...
media/gpu/ipc/service/media_channel.cc:66: void OnFilterAdded(IPC::Channel*
channel) override { channel_ = channel; }
On 2016/09/19 22:55:49, sandersd wrote:
> On 2016/09/19 22:47:35, dcheng wrote:
> > Do we need this override?
> 
> No, it could have been safely passed AFAIK. Is there a reason why passing
would
> be preferred?

I misread this originally. I guess it's a pretty common pattern anyway so it's
fine =)

[sandersd (OOO until July 31), 2016-09-19 23:07:51]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2016-09-19 23:07:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
tguilbert@chromium.org
Link to the patchset: https://codereview.chromium.org/2349463003/#ps100001
(title: "Nits")

[commit-bot: I haz the power, 2016-09-19 23:08:21]

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-09-19 23:17:00]

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

[commit-bot: I haz the power, 2016-09-19 23:17:01]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sandersd (OOO until July 31), 2016-09-19 23:37:52]

On 2016/09/19 23:17:01, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

dgheng@: What do you want me to do about the OWNERS issue (quoted below)? The
current list is not the same as the IPC SECURITY_OWNERS, should I just be adding
the recommended lines?

** Presubmit ERRORS **
Found changes to IPC files without a security OWNER!

***************
media/gpu/ipc/common/OWNERS is missing the following lines:

per-file *_messages*.h=set noparent
per-file *_messages*.h=file://ipc/SECURITY_OWNERS

for changed files:
  media/gpu/ipc/common/media_messages.h
***************

[dcheng, 2016-09-19 23:40:42]

On 2016/09/19 23:37:52, sandersd wrote:
> On 2016/09/19 23:17:01, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> dgheng@: What do you want me to do about the OWNERS issue (quoted below)? The
> current list is not the same as the IPC SECURITY_OWNERS, should I just be
adding
> the recommended lines?
> 
> ** Presubmit ERRORS **
> Found changes to IPC files without a security OWNER!
> 
> ***************
> media/gpu/ipc/common/OWNERS is missing the following lines:
> 
> per-file *_messages*.h=set noparent
> per-file *_messages*.h=file://ipc/SECURITY_OWNERS
> 
> for changed files:
>   media/gpu/ipc/common/media_messages.h
> ***************

If you don't mind, let's just replace the old, legacy owners list with those two
short lines =)
I did a global search-replace when we changed this, but there's undoubtedly a
few files that got missed. Sorry for the trouble!

[sandersd (OOO until July 31), 2016-09-20 17:50:48]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2016-09-20 17:51:20]

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-09-20 18:40:26]

Description was changed from

==========
MediaService: Implement GetChannelToken IPC

This new IPC provides a non-forgeable token identifying a GpuChannel so
that it can be passed to Mojo services that want to share the same
channel in the GPU process (namely VDAv2 in the MojoMediaApplication).

For simplicity, the IPC is sync. It is handled on the IO thread in the
GPU process, so latency is expected to be manageable (and vastly better
that the already sync CreateVideoDecodeAccelerator IPC).

BUG=522298
==========

to

==========
MediaService: Implement GetChannelToken IPC

This new IPC provides a non-forgeable token identifying a GpuChannel so
that it can be passed to Mojo services that want to share the same
channel in the GPU process (namely VDAv2 in the MojoMediaApplication).

For simplicity, the IPC is sync. It is handled on the IO thread in the
GPU process, so latency is expected to be manageable (and vastly better
that the already sync CreateVideoDecodeAccelerator IPC).

BUG=522298
==========

[commit-bot: I haz the power, 2016-09-20 18:40:28]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-09-20 18:42:58]

Description was changed from

==========
MediaService: Implement GetChannelToken IPC

This new IPC provides a non-forgeable token identifying a GpuChannel so
that it can be passed to Mojo services that want to share the same
channel in the GPU process (namely VDAv2 in the MojoMediaApplication).

For simplicity, the IPC is sync. It is handled on the IO thread in the
GPU process, so latency is expected to be manageable (and vastly better
that the already sync CreateVideoDecodeAccelerator IPC).

BUG=522298
==========

to

==========
MediaService: Implement GetChannelToken IPC

This new IPC provides a non-forgeable token identifying a GpuChannel so
that it can be passed to Mojo services that want to share the same
channel in the GPU process (namely VDAv2 in the MojoMediaApplication).

For simplicity, the IPC is sync. It is handled on the IO thread in the
GPU process, so latency is expected to be manageable (and vastly better
that the already sync CreateVideoDecodeAccelerator IPC).

BUG=522298

Committed: https://crrev.com/777e4aae05428625f99dcabddfdd4b581d6516d5
Cr-Commit-Position: refs/heads/master@{#419826}
==========

[commit-bot: I haz the power, 2016-09-20 18:42:59]

Patchset 5 (id:??) landed as
https://crrev.com/777e4aae05428625f99dcabddfdd4b581d6516d5
Cr-Commit-Position: refs/heads/master@{#419826}