Add MediaGpuChannelManager::LookupChannel().

Add MediaGpuChannelManager::LookupChannel().

This adds a path for VDAv2 clients to look up a command buffer identified by |command_buffer_id|.

Also in this CL is a method to look up the route ID from the accelerated GPU factories so that it's easy to pass the |command_buffer_id|.

BUG=522298
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

[sandersd (OOO until July 31), 2016-11-23 23:22:48]

Description was changed from

==========
Add MediaGpuChannelManager::LookupChannel().

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

to

==========
Add MediaGpuChannelManager::LookupChannel().

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[sandersd (OOO until July 31), 2017-01-09 23:59:08]

Description was changed from

==========
Add MediaGpuChannelManager::LookupChannel().

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add MediaGpuChannelManager::LookupChannel().

This adds a path for VDAv2 clients to look up a command buffer identified by
|command_buffer_id|.

Also in this CL is a method to look up the route ID from the accelerated GPU
factories so that it's easy to pass the |command_buffer_id|.

BUG=522298
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[sandersd (OOO until July 31), 2017-01-09 23:59:08]

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

[tguilbert, 2017-01-10 01:16:36]

LGTM

I however do not have strong feelings as to the API names (and am leaving it up
to you whether you want a 2nd opinion or not, as discussed offline)

https://codereview.chromium.org/2526953003/diff/40001/media/renderers/gpu_vid...
File media/renderers/gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/2526953003/diff/40001/media/renderers/gpu_vid...
media/renderers/gpu_video_accelerator_factories.h:77: virtual int32_t
GetCommandBufferRouteId() = 0;
Should '0' as the error value be documented?

[sandersd (OOO until July 31), 2017-01-10 01:36:58]

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

[sandersd (OOO until July 31), 2017-01-10 01:36:58]

dcheng@: Please security review changes to video_decoder.mojom.

This change finally hooks up all the steps so far to actually pass a command
buffer ID across the mojo channel.

You can see an example of what using the token on the GPU side will look like in
this mockup I wrote:
https://codereview.chromium.org/2544113002/diff/1/media/mojo/services/gpu_moj...

[dcheng, 2017-01-10 06:03:42]

https://codereview.chromium.org/2526953003/diff/60001/content/renderer/media/...
File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right):

https://codereview.chromium.org/2526953003/diff/60001/content/renderer/media/...
content/renderer/media/renderer_gpu_video_accelerator_factories.cc:132: if
(!context_provider_)
How come most other things use CheckContextLost()?

https://codereview.chromium.org/2526953003/diff/60001/media/gpu/ipc/service/m...
File media/gpu/ipc/service/media_gpu_channel.h (right):

https://codereview.chromium.org/2526953003/diff/60001/media/gpu/ipc/service/m...
media/gpu/ipc/service/media_gpu_channel.h:31: base::UnguessableToken
channel_token);
Nit: const ref here and elsewhere (it's a POD, moving it won't do much)

https://codereview.chromium.org/2526953003/diff/60001/media/mojo/services/moj...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://codereview.chromium.org/2526953003/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_video_decoder_service.cc:40: <<
command_buffer_id->route_id;
Is it possible to upstream the implementation of this function into this CL?

[sandersd (OOO until July 31), 2017-01-10 23:54:14]

https://codereview.chromium.org/2526953003/diff/60001/content/renderer/media/...
File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right):

https://codereview.chromium.org/2526953003/diff/60001/content/renderer/media/...
content/renderer/media/renderer_gpu_video_accelerator_factories.cc:132: if
(!context_provider_)
On 2017/01/10 06:03:42, dcheng wrote:
> How come most other things use CheckContextLost()?

I don't care if the returned route_id is invalid, since it's also tied to a
channel which is separately queried. That said, it's probably better to be
consistent, so I've changed it.

https://codereview.chromium.org/2526953003/diff/60001/media/gpu/ipc/service/m...
File media/gpu/ipc/service/media_gpu_channel.h (right):

https://codereview.chromium.org/2526953003/diff/60001/media/gpu/ipc/service/m...
media/gpu/ipc/service/media_gpu_channel.h:31: base::UnguessableToken
channel_token);
On 2017/01/10 06:03:42, dcheng wrote:
> Nit: const ref here and elsewhere (it's a POD, moving it won't do much)

Done.

https://codereview.chromium.org/2526953003/diff/60001/media/mojo/services/moj...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://codereview.chromium.org/2526953003/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_video_decoder_service.cc:40: <<
command_buffer_id->route_id;
On 2017/01/10 06:03:42, dcheng wrote:
> Is it possible to upstream the implementation of this function into this CL?

Done. (It's now plumbed to the factory which does not yet have an
implementation.)

[sandersd (OOO until July 31), 2017-01-10 23:58:48]

Patchset #5 (id:80001) has been deleted

[dcheng, 2017-01-11 09:14:19]

For future CLs, it would be better if the full implementation backing the IPC is
included. It's hard to a security review like this, since there are a lot of
missing pieces still.

To unblock things, LGTM, but please include a security reviewer on the
implementation CL.

https://codereview.chromium.org/2526953003/diff/100001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://codereview.chromium.org/2526953003/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder_service.cc:38: // TODO(sandersd): Enter
an error state.
What does this mean?

[sandersd (OOO until July 31), 2017-01-11 21:33:09]

> For future CLs, it would be better if the full implementation backing the
> IPC is included. It's hard to a security review like this, since there are
> a lot of missing pieces still.

I'm worried there is a miscommunication here, since I believe that everything
important is already implemented.

What we do have:

  [Renderer]
  GpuVideoAcceleratorFactories provides access to channel token Chrome IPC
  MojoVideoDecoder sends channel token to GPU process over Mojo

  [GPU]
  MediaGpuChannel provides Chrome IPC service to get channel tokens
  MediaGpuChannel provides API to look up channels by token
  MojoVideoDecoderService accepts channel tokens over Mojo and passes them to
decoder implementations


  [Not implemented yet]
  Decoders running under MojoVideoDecoderService take the channel tokens and
look them up via MediaGpuChannel API


This last step is fully on the GPU side and not implemented because there are
not actually any decoders yet. The mockup I linked to earlier shows the steps
(basically: hop to GPU main thread, call
MediaGpuChannelManager::LookupChannel()).

https://codereview.chromium.org/2526953003/diff/100001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://codereview.chromium.org/2526953003/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder_service.cc:38: // TODO(sandersd): Enter
an error state.
On 2017/01/11 09:14:18, dcheng wrote:
> What does this mean?

The client should not have called this method at this time; we should probably
be disconnecting them. At the time this was written we didn't have a complete
story for how such errors should be handled. (We have most of a plan now, but it
wasn't critical to include in this CL.)

[dcheng, 2017-01-11 22:01:59]

On 2017/01/11 21:33:09, sandersd wrote:
> > For future CLs, it would be better if the full implementation backing the
> > IPC is included. It's hard to a security review like this, since there are
> > a lot of missing pieces still.
> 
> I'm worried there is a miscommunication here, since I believe that everything
> important is already implemented.
> 
> What we do have:
> 
>   [Renderer]
>   GpuVideoAcceleratorFactories provides access to channel token Chrome IPC
>   MojoVideoDecoder sends channel token to GPU process over Mojo
> 
>   [GPU]
>   MediaGpuChannel provides Chrome IPC service to get channel tokens
>   MediaGpuChannel provides API to look up channels by token
>   MojoVideoDecoderService accepts channel tokens over Mojo and passes them to
> decoder implementations
> 
> 
>   [Not implemented yet]
>   Decoders running under MojoVideoDecoderService take the channel tokens and
> look them up via MediaGpuChannel API
> 
> 
> This last step is fully on the GPU side and not implemented because there are
> not actually any decoders yet. The mockup I linked to earlier shows the steps
> (basically: hop to GPU main thread, call
> MediaGpuChannelManager::LookupChannel()).
> 

From the perspective of a security review, it's not just the interfaces being
implemented: it's how the values passed over the interfaces are used. In this
CL, |command_buffer_id| is plumbed through but not consumed by anything. It
passes two bits of information over, and it's not entirely clear to me (who
doesn't know media or gpu that well) how those bits will be used.

https://codereview.chromium.org/2526953003/diff/100001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://codereview.chromium.org/2526953003/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder_service.cc:38: // TODO(sandersd): Enter
an error state.
On 2017/01/11 21:33:09, sandersd wrote:
> On 2017/01/11 09:14:18, dcheng wrote:
> > What does this mean?
> 
> The client should not have called this method at this time; we should probably
> be disconnecting them. At the time this was written we didn't have a complete
> story for how such errors should be handled. (We have most of a plan now, but
it
> wasn't critical to include in this CL.)

Out of curiosity, how will they be handled? =)

[sandersd (OOO until July 31), 2017-01-11 22:21:31]

> From the perspective of a security review, it's not just the interfaces
> being implemented: it's how the values passed over the interfaces are
> used. In this CL, |command_buffer_id| is plumbed through but not
> consumed by anything. It passes two bits of information over, and it's
> not entirely clear to me (who doesn't know media or gpu that well) how
> those bits will be used.

This implies that the decoder implementations should be security reviewed
as well, since that's where the connection will be (possibly abstracted
in a single helper, not sure yet).

Is there a development workflow that has worked well for this sort of thing
before? (ie. where infrastructure is being build without impls ready yet.)

Thanks!

https://codereview.chromium.org/2526953003/diff/100001/media/mojo/services/mo...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://codereview.chromium.org/2526953003/diff/100001/media/mojo/services/mo...
media/mojo/services/mojo_video_decoder_service.cc:38: // TODO(sandersd): Enter
an error state.
On 2017/01/11 22:01:59, dcheng wrote:
> On 2017/01/11 21:33:09, sandersd wrote:
> > On 2017/01/11 09:14:18, dcheng wrote:
> > > What does this mean?
> > 
> > The client should not have called this method at this time; we should
probably
> > be disconnecting them. At the time this was written we didn't have a
complete
> > story for how such errors should be handled. (We have most of a plan now,
but
> it
> > wasn't critical to include in this CL.)
> 
> Out of curiosity, how will they be handled? =)

The plan is to pass a callback into the constructor that will close the string
binding (which for media services is owned by a StrongBindingSet); we will just
call that for incorrect API usage. In any egregious cases (I don't think
MojoVideoDecoder has any), the plan is to also call ReportBadMessage().

[dcheng, 2017-01-11 22:36:31]

On 2017/01/11 22:21:31, sandersd wrote:
> > From the perspective of a security review, it's not just the interfaces
> > being implemented: it's how the values passed over the interfaces are
> > used. In this CL, |command_buffer_id| is plumbed through but not
> > consumed by anything. It passes two bits of information over, and it's
> > not entirely clear to me (who doesn't know media or gpu that well) how
> > those bits will be used.
> 
> This implies that the decoder implementations should be security reviewed
> as well, since that's where the connection will be (possibly abstracted
> in a single helper, not sure yet).
> 
> Is there a development workflow that has worked well for this sort of thing
> before? (ie. where infrastructure is being build without impls ready yet.)

Sadly, not really. I think it would be helpful to have even one example of this
being consumed. If that's not possible, perhaps CC an ipc security reviewer in
the followup. (this patch is already approved as far as I'm concerned)

> 
> Thanks!
> 
>
https://codereview.chromium.org/2526953003/diff/100001/media/mojo/services/mo...
> File media/mojo/services/mojo_video_decoder_service.cc (right):
> 
>
https://codereview.chromium.org/2526953003/diff/100001/media/mojo/services/mo...
> media/mojo/services/mojo_video_decoder_service.cc:38: // TODO(sandersd): Enter
> an error state.
> On 2017/01/11 22:01:59, dcheng wrote:
> > On 2017/01/11 21:33:09, sandersd wrote:
> > > On 2017/01/11 09:14:18, dcheng wrote:
> > > > What does this mean?
> > > 
> > > The client should not have called this method at this time; we should
> probably
> > > be disconnecting them. At the time this was written we didn't have a
> complete
> > > story for how such errors should be handled. (We have most of a plan now,
> but
> > it
> > > wasn't critical to include in this CL.)
> > 
> > Out of curiosity, how will they be handled? =)
> 
> The plan is to pass a callback into the constructor that will close the string
> binding (which for media services is owned by a StrongBindingSet); we will
just
> call that for incorrect API usage. In any egregious cases (I don't think
> MojoVideoDecoder has any), the plan is to also call ReportBadMessage().

[sandersd (OOO until July 31), 2017-01-11 22:38:25]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2017-01-11 22:38:26]

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/2526953003/#ps100001
(title: "Requested changes.")

[commit-bot: I haz the power, 2017-01-11 22:40:12]

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, 2017-01-11 22:59:34]

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

[commit-bot: I haz the power, 2017-01-11 22:59:35]

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

[sandersd (OOO until July 31), 2017-01-11 23:04:16]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2017-01-11 23:04:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
tguilbert@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2526953003/#ps120001
(title: "Implement
MockGpuVideoAcceleratorFactories::GetCommandBufferRouteId().")

[commit-bot: I haz the power, 2017-01-11 23:05:24]

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, 2017-01-12 01:02:03]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484175856748440,
"parent_rev": "ff1a6112240cb285eab6a15a62419c2359b84ff5", "commit_rev":
"50dfde3153a055f19e836c1400d697e1e95ce954"}

[commit-bot: I haz the power, 2017-01-12 01:18:57]

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

[commit-bot: I haz the power, 2017-01-12 01:18:59]

Prior attempt to commit was detected, but we were not able to check whether the
issue was successfully committed. Please check Git history manually and re-check
CQ or close this issue as needed.