media: Support creation and SetCdm() for mojo based CDM.

media: Support creation and SetCdm() for mojo based CDM.

- Add mojo::ContentDecryptionModuleFactory interface which helps request a
  ContentDecryptionModule service. This is needed because we need to pass in
  some information (e.g. key system and cdm ID) to request the correct service.
- MojoCdmFactory is the proxy of mojo::ContentDecryptionModuleFactory. It is
  used when --enable-mojo-media-renderer is specified for HTMLViewer.
- MojoCdmFactoryService implements mojo::ContentDecryptionModuleFactory. It uses
  MojoCdmServiceContext to help create MojoCdmServices.
- MojoCdmServiceContext creates, owns and manages MojoCdmServices. It is owned
  by MojoMediaApplication and is passed to MojoRendererService so that when
  SetCdm(cdm_id) is called, MojoCdmService can find the correct CDM for |cdm_id|
  through MojoCdmServiceContext.
- SetCdm() is implemented in MojoRenderer{Impl|Service}.

BUG=432998
TEST=Clear Key supported in the media mojo app for prefixed EME API.

[xhwang, 2015-01-05 23:52:37]

xhwang@chromium.org changed reviewers:
+ ddorwin@chromium.org

[xhwang, 2015-01-05 23:52:38]

PTAL

https://codereview.chromium.org/840473002/diff/1/media/mojo/services/mojo_med...
File media/mojo/services/mojo_media_application.cc (right):

https://codereview.chromium.org/840473002/diff/1/media/mojo/services/mojo_med...
media/mojo/services/mojo_media_application.cc:19: static void
EnableLoggingFromArgs(const std::vector<std::string>& args) {
This is moved from Initialize().

https://codereview.chromium.org/840473002/diff/1/mojo/services/html_viewer/we...
File mojo/services/html_viewer/webmediaplayer_factory.cc (right):

https://codereview.chromium.org/840473002/diff/1/mojo/services/html_viewer/we...
mojo/services/html_viewer/webmediaplayer_factory.cc:100: cdm_factory.reset(new
media::MojoCdmFactory(cdm_factory_ptr.Pass()));
Background for another CL (https://codereview.chromium.org/835683002/):
HTMLViewer owners perfer to using a run time flag (enable_mojo_media_renderer)
here instead of macros. This is another reason I don't like to have
ENABLE_BROWSER_CDMS here.

[ddorwin, 2015-01-06 00:57:28]

I reviewed through mojo_cdm_factory_service.cc. I'll wait until we resolve the
cdm_id question before reviewing further.

https://codereview.chromium.org/840473002/diff/20001/media/mojo/interfaces/co...
File media/mojo/interfaces/content_decryption_module_factory.mojom (right):

https://codereview.chromium.org/840473002/diff/20001/media/mojo/interfaces/co...
media/mojo/interfaces/content_decryption_module_factory.mojom:9: // An interface
to request a ContentDecryptionModule service.
Should this file have more detailed comments, such as for the parameters?
Specifically, cdm_id might not be obvious.

https://codereview.chromium.org/840473002/diff/20001/media/mojo/interfaces/co...
media/mojo/interfaces/content_decryption_module_factory.mojom:13: int32 cdm_id,
Can we have the factory generate an ID in the CDM before outputting it?

This solves the multiple-client problem and avoids clients passing in
conflicting values.

Regarding the original reason (no synchronous Mojo calls), perhaps we can return
the cdm_id and cdm together here.

https://codereview.chromium.org/840473002/diff/20001/media/mojo/interfaces/me...
File media/mojo/interfaces/media_renderer.mojom (right):

https://codereview.chromium.org/840473002/diff/20001/media/mojo/interfaces/me...
media/mojo/interfaces/media_renderer.mojom:33: // successfully attached.
It returns the result of something that is not mentioned in the main
documentation (first sentence) for the method. I think we decided to keep the
name, but we should at least include "attach" in the main description.

https://codereview.chromium.org/840473002/diff/20001/media/mojo/services/mojo...
File media/mojo/services/mojo_cdm.cc (right):

https://codereview.chromium.org/840473002/diff/20001/media/mojo/services/mojo...
media/mojo/services/mojo_cdm.cc:49: DCHECK_NE(CdmContext::kInvalidCdmId,
cdm_id_);
Will this change if/when Mojo supports GetDecryptor()?

https://codereview.chromium.org/840473002/diff/20001/media/mojo/services/mojo...
media/mojo/services/mojo_cdm.cc:128: return nullptr;
Should this never be called? i.e. NOTREACHED()

https://codereview.chromium.org/840473002/diff/20001/media/mojo/services/mojo...
File media/mojo/services/mojo_cdm_factory.h (right):

https://codereview.chromium.org/840473002/diff/20001/media/mojo/services/mojo...
media/mojo/services/mojo_cdm_factory.h:33: static int next_cdm_id_;
See comment in .mojom.

This will not work when Mojo supports OOP clients and there is more than one
client process.

[xhwang, 2015-01-06 19:54:53]

xhwang@chromium.org changed reviewers:
+ jamesr@chromium.org

[xhwang, 2015-01-06 19:54:54]

+jamesr: Please see my comment in content_decryption_module_factory.mojom. Also,
I am adding the ContentDecryptionModuleFactory interface so that I can pass in
some parameters when requesting a mojo CDM service. Please let me know whether
this seems right to you.

ddorwin: Please see my comments.

https://codereview.chromium.org/840473002/diff/20001/media/mojo/interfaces/co...
File media/mojo/interfaces/content_decryption_module_factory.mojom (right):

https://codereview.chromium.org/840473002/diff/20001/media/mojo/interfaces/co...
media/mojo/interfaces/content_decryption_module_factory.mojom:9: // An interface
to request a ContentDecryptionModule service.
On 2015/01/06 00:57:28, ddorwin wrote:
> Should this file have more detailed comments, such as for the parameters?
> Specifically, cdm_id might not be obvious.

Done.

https://codereview.chromium.org/840473002/diff/20001/media/mojo/interfaces/co...
media/mojo/interfaces/content_decryption_module_factory.mojom:13: int32 cdm_id,
On 2015/01/06 00:57:28, ddorwin wrote:
> Can we have the factory generate an ID in the CDM before outputting it?
> 
> This solves the multiple-client problem and avoids clients passing in
> conflicting values.
> 
> Regarding the original reason (no synchronous Mojo calls), perhaps we can
return
> the cdm_id and cdm together here.

I looked at this more closely and IIUIC we can't get any value/struct back from
a mojo service  synchronously. We can only get requested services back
synchronously. This is why we are providing a client side |cdm_id|. (+jamesr to
double check on this.)

I didn't realize this yesterday (I blame the holidays :) Sorry for the
confusion.

https://codereview.chromium.org/840473002/diff/20001/media/mojo/interfaces/me...
File media/mojo/interfaces/media_renderer.mojom (right):

https://codereview.chromium.org/840473002/diff/20001/media/mojo/interfaces/me...
media/mojo/interfaces/media_renderer.mojom:33: // successfully attached.
On 2015/01/06 00:57:28, ddorwin wrote:
> It returns the result of something that is not mentioned in the main
> documentation (first sentence) for the method. I think we decided to keep the
> name, but we should at least include "attach" in the main description.

Done.

https://codereview.chromium.org/840473002/diff/20001/media/mojo/services/mojo...
File media/mojo/services/mojo_cdm.cc (right):

https://codereview.chromium.org/840473002/diff/20001/media/mojo/services/mojo...
media/mojo/services/mojo_cdm.cc:49: DCHECK_NE(CdmContext::kInvalidCdmId,
cdm_id_);
On 2015/01/06 00:57:28, ddorwin wrote:
> Will this change if/when Mojo supports GetDecryptor()?

For simplicity, I am assigning a valid CDM ID to all MojoCdm instances, not
matter whether the Decryptor interface is supported or not. The media::Renderer
implementation (RendererImpl or MojoRendererImpl) will decide whether it wants
to use a Decryptor interface directly, or use a cdm_id for its remote renderer
service.

https://codereview.chromium.org/840473002/diff/20001/media/mojo/services/mojo...
media/mojo/services/mojo_cdm.cc:128: return nullptr;
On 2015/01/06 00:57:28, ddorwin wrote:
> Should this never be called? i.e. NOTREACHED()

Currently yes. When we start to support the desktop case where we'll use
RendererImpl + MojoCdm, this needs to be implemented. Adds NOTIMPLEMENTED()
here.

https://codereview.chromium.org/840473002/diff/20001/media/mojo/services/mojo...
File media/mojo/services/mojo_cdm_factory.h (right):

https://codereview.chromium.org/840473002/diff/20001/media/mojo/services/mojo...
media/mojo/services/mojo_cdm_factory.h:33: static int next_cdm_id_;
On 2015/01/06 00:57:28, ddorwin wrote:
> See comment in .mojom.
> 
> This will not work when Mojo supports OOP clients and there is more than one
> client process.

The plan for supporting a dedicated CDM/media process is not clear yet. But no
matter what we do, we'll never mix the services for different processes, which
is not secure. So services for different render processes will be hosted in
different mojo application instances (or something similar), even though all
these instances may live in the same process.

This is similar to what we are doing in the browser process, where we have
RenderProcessHostImpl hosting mojo services for the corresponding render
process:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

With this, a unique per-process cdm_id should be safe to use.

[ddorwin, 2015-01-07 00:37:14]

Reviewed the same files. Let's see what jamesr says about sync output params.

https://codereview.chromium.org/840473002/diff/20001/media/mojo/interfaces/co...
File media/mojo/interfaces/content_decryption_module_factory.mojom (right):

https://codereview.chromium.org/840473002/diff/20001/media/mojo/interfaces/co...
media/mojo/interfaces/content_decryption_module_factory.mojom:13: int32 cdm_id,
On 2015/01/06 19:54:54, xhwang wrote:
> On 2015/01/06 00:57:28, ddorwin wrote:
> > Can we have the factory generate an ID in the CDM before outputting it?
> > 
> > This solves the multiple-client problem and avoids clients passing in
> > conflicting values.
> > 
> > Regarding the original reason (no synchronous Mojo calls), perhaps we can
> return
> > the cdm_id and cdm together here.
> 
> I looked at this more closely and IIUIC we can't get any value/struct back
from
> a mojo service  synchronously. We can only get requested services back
> synchronously. This is why we are providing a client side |cdm_id|. (+jamesr
to
> double check on this.)
> 
> I didn't realize this yesterday (I blame the holidays :) Sorry for the
> confusion.

So |cdm| is a special output parameter? That seems very odd.

https://codereview.chromium.org/840473002/diff/20001/media/mojo/services/mojo...
File media/mojo/services/mojo_cdm.cc (right):

https://codereview.chromium.org/840473002/diff/20001/media/mojo/services/mojo...
media/mojo/services/mojo_cdm.cc:49: DCHECK_NE(CdmContext::kInvalidCdmId,
cdm_id_);
On 2015/01/06 19:54:54, xhwang wrote:
> On 2015/01/06 00:57:28, ddorwin wrote:
> > Will this change if/when Mojo supports GetDecryptor()?
> 
> For simplicity, I am assigning a valid CDM ID to all MojoCdm instances, not
> matter whether the Decryptor interface is supported or not. The
media::Renderer
> implementation (RendererImpl or MojoRendererImpl) will decide whether it wants
> to use a Decryptor interface directly, or use a cdm_id for its remote renderer
> service.
> 

Acknowledged. Thanks.

https://codereview.chromium.org/840473002/diff/20001/media/mojo/services/mojo...
File media/mojo/services/mojo_cdm_factory.h (right):

https://codereview.chromium.org/840473002/diff/20001/media/mojo/services/mojo...
media/mojo/services/mojo_cdm_factory.h:33: static int next_cdm_id_;
On 2015/01/06 19:54:54, xhwang wrote:
> On 2015/01/06 00:57:28, ddorwin wrote:
> > See comment in .mojom.
> > 
> > This will not work when Mojo supports OOP clients and there is more than one
> > client process.
> 
> The plan for supporting a dedicated CDM/media process is not clear yet. But no
> matter what we do, we'll never mix the services for different processes, which
> is not secure. So services for different render processes will be hosted in
> different mojo application instances (or something similar), even though all
> these instances may live in the same process.
> 
> This is similar to what we are doing in the browser process, where we have
> RenderProcessHostImpl hosting mojo services for the corresponding render
> process:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
> 
> With this, a unique per-process cdm_id should be safe to use.

I think what you mean is that there will also be a host ID/object that will be
dedicated to this client process, so the IDs will not be shared between clients.
Is that right?

https://codereview.chromium.org/840473002/diff/40001/media/mojo/interfaces/co...
File media/mojo/interfaces/content_decryption_module_factory.mojom (right):

https://codereview.chromium.org/840473002/diff/40001/media/mojo/interfaces/co...
media/mojo/interfaces/content_decryption_module_factory.mojom:10: // and
|security_origin|. The |cdm| can be null if the requested service is not
nit: s/and/for use in/

https://codereview.chromium.org/840473002/diff/40001/media/mojo/interfaces/co...
media/mojo/interfaces/content_decryption_module_factory.mojom:10: // and
|security_origin|. The |cdm| can be null if the requested service is not
The *returned* ...

https://codereview.chromium.org/840473002/diff/40001/media/mojo/interfaces/me...
File media/mojo/interfaces/media_renderer.mojom (right):

https://codereview.chromium.org/840473002/diff/40001/media/mojo/interfaces/me...
media/mojo/interfaces/media_renderer.mojom:32: // Attaches the CDM associated
with |cdm_id| to the renderer service,
nit: *media* renderer? (It just looks odd when reading it, though the file and
class have context. You could also say "this object."

[jamesr, 2015-01-07 02:22:06]

https://codereview.chromium.org/840473002/diff/20001/media/mojo/interfaces/co...
File media/mojo/interfaces/content_decryption_module_factory.mojom (right):

https://codereview.chromium.org/840473002/diff/20001/media/mojo/interfaces/co...
media/mojo/interfaces/content_decryption_module_factory.mojom:13: int32 cdm_id,
On 2015/01/06 19:54:54, xhwang wrote:
> On 2015/01/06 00:57:28, ddorwin wrote:
> > Can we have the factory generate an ID in the CDM before outputting it?
> > 
> > This solves the multiple-client problem and avoids clients passing in
> > conflicting values.
> > 
> > Regarding the original reason (no synchronous Mojo calls), perhaps we can
> return
> > the cdm_id and cdm together here.
> 
> I looked at this more closely and IIUIC we can't get any value/struct back
from
> a mojo service  synchronously. We can only get requested services back
> synchronously. This is why we are providing a client side |cdm_id|. (+jamesr
to
> double check on this.)
> 
> I didn't realize this yesterday (I blame the holidays :) Sorry for the
> confusion.

Uhh, I don't really understand.  All mojo calls are asynchronous.  You can
connect to a service and start using it immediately since connecting to a
service means allocating a message pipe and passing one handle which will
asynchronously arrive at the other end.  In the meantime, you can write into the
other handle.  But that's not the same thing as "getting something
synchronously"

What's the problem you are trying to solve here?

[xhwang, 2015-01-07 07:07:13]

On 2015/01/07 02:22:06, jamesr wrote:
>
https://codereview.chromium.org/840473002/diff/20001/media/mojo/interfaces/co...
> File media/mojo/interfaces/content_decryption_module_factory.mojom (right):
> 
>
https://codereview.chromium.org/840473002/diff/20001/media/mojo/interfaces/co...
> media/mojo/interfaces/content_decryption_module_factory.mojom:13: int32
cdm_id,
> On 2015/01/06 19:54:54, xhwang wrote:
> > On 2015/01/06 00:57:28, ddorwin wrote:
> > > Can we have the factory generate an ID in the CDM before outputting it?
> > > 
> > > This solves the multiple-client problem and avoids clients passing in
> > > conflicting values.
> > > 
> > > Regarding the original reason (no synchronous Mojo calls), perhaps we can
> > return
> > > the cdm_id and cdm together here.
> > 
> > I looked at this more closely and IIUIC we can't get any value/struct back
> from
> > a mojo service  synchronously. We can only get requested services back
> > synchronously. This is why we are providing a client side |cdm_id|. (+jamesr
> to
> > double check on this.)
> > 
> > I didn't realize this yesterday (I blame the holidays :) Sorry for the
> > confusion.
> 
> Uhh, I don't really understand.  All mojo calls are asynchronous.  You can
> connect to a service and start using it immediately since connecting to a
> service means allocating a message pipe and passing one handle which will
> asynchronously arrive at the other end.  In the meantime, you can write into
the
> other handle.  But that's not the same thing as "getting something
> synchronously"
> 
> What's the problem you are trying to solve here?

Thanks for the explanation! I thought it's synchronous because we can use the
interface (message pipe) immediately after we connect to the service. But
apparently I was wrong.

But with this I wonder how nullable interfaces (&?) work in  cases like: 

Create(string key_system,
           string security_origin,
           int32 cdm_id,
           ContentDecryptionModule&? cdm);

We'll always create a message pipe for the |cdm|, and pass it one handle to the
other side. At the other side, if we found that no service can be binded, I
guess the message pipe will be closed. Is that correct?

But that'll happen after Create() returns. What if I make a call on |cdm| before
the message pipe is closed? Will the call be dropped? What if I make a call
after the message pipe is closed? Will it crash? Or shall I check whether |cdm|
is valid before I make any calls to the |cdm|?

For ddorwin's question about the |cdm_id|. We need to have a |cdm_id| available
immediately after Create() returns. Since everything is asyc, there's no way to
pass a |cdm_id| from the service back to the client. That why I choose to have
the client side generating the |cdm_id| and pass it over to the service when
requesting the CDM service (in the above Create() call). I just wanted to double
check with you whether this seems right.

[jamesr, 2015-01-08 01:40:55]

On 2015/01/07 07:07:13, xhwang wrote:
> On 2015/01/07 02:22:06, jamesr wrote:
> >
>
https://codereview.chromium.org/840473002/diff/20001/media/mojo/interfaces/co...
> > File media/mojo/interfaces/content_decryption_module_factory.mojom (right):
> > 
> >
>
https://codereview.chromium.org/840473002/diff/20001/media/mojo/interfaces/co...
> > media/mojo/interfaces/content_decryption_module_factory.mojom:13: int32
> cdm_id,
> > On 2015/01/06 19:54:54, xhwang wrote:
> > > On 2015/01/06 00:57:28, ddorwin wrote:
> > > > Can we have the factory generate an ID in the CDM before outputting it?
> > > > 
> > > > This solves the multiple-client problem and avoids clients passing in
> > > > conflicting values.
> > > > 
> > > > Regarding the original reason (no synchronous Mojo calls), perhaps we
can
> > > return
> > > > the cdm_id and cdm together here.
> > > 
> > > I looked at this more closely and IIUIC we can't get any value/struct back
> > from
> > > a mojo service  synchronously. We can only get requested services back
> > > synchronously. This is why we are providing a client side |cdm_id|.
(+jamesr
> > to
> > > double check on this.)
> > > 
> > > I didn't realize this yesterday (I blame the holidays :) Sorry for the
> > > confusion.
> > 
> > Uhh, I don't really understand.  All mojo calls are asynchronous.  You can
> > connect to a service and start using it immediately since connecting to a
> > service means allocating a message pipe and passing one handle which will
> > asynchronously arrive at the other end.  In the meantime, you can write into
> the
> > other handle.  But that's not the same thing as "getting something
> > synchronously"
> > 
> > What's the problem you are trying to solve here?
> 
> Thanks for the explanation! I thought it's synchronous because we can use the
> interface (message pipe) immediately after we connect to the service. But
> apparently I was wrong.
> 
> But with this I wonder how nullable interfaces (&?) work in  cases like: 
> 
> Create(string key_system,
>            string security_origin,
>            int32 cdm_id,
>            ContentDecryptionModule&? cdm);
> 
> We'll always create a message pipe for the |cdm|, and pass it one handle to
the
> other side. At the other side, if we found that no service can be binded, I
> guess the message pipe will be closed. Is that correct?
> 
> But that'll happen after Create() returns. What if I make a call on |cdm|
before
> the message pipe is closed? Will the call be dropped? What if I make a call
> after the message pipe is closed? Will it crash? Or shall I check whether
|cdm|
> is valid before I make any calls to the |cdm|?

It's the same as if you pass a pipe handle of mojom type ContentDecryptionModule
to another service and the other end closes it without seeing your messages or
crashes.  At some point you detect that the pipe is closed and you figure out
what to do about it.

> 
> For ddorwin's question about the |cdm_id|. We need to have a |cdm_id|
available
> immediately after Create() returns. Since everything is asyc, there's no way
to
> pass a |cdm_id| from the service back to the client. That why I choose to have
> the client side generating the |cdm_id| and pass it over to the service when
> requesting the CDM service (in the above Create() call). I just wanted to
double
> check with you whether this seems right.

I don't think I know enough about the overall flow to give good feedback.  Is
there a higher level doc somewhere describing these pieces and how they
interact?

[xhwang, 2015-01-20 22:26:56]

On 2015/01/08 01:40:55, jamesr wrote:
> On 2015/01/07 07:07:13, xhwang wrote:
> > On 2015/01/07 02:22:06, jamesr wrote:
> > >
> >
>
https://codereview.chromium.org/840473002/diff/20001/media/mojo/interfaces/co...
> > > File media/mojo/interfaces/content_decryption_module_factory.mojom
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/840473002/diff/20001/media/mojo/interfaces/co...
> > > media/mojo/interfaces/content_decryption_module_factory.mojom:13: int32
> > cdm_id,
> > > On 2015/01/06 19:54:54, xhwang wrote:
> > > > On 2015/01/06 00:57:28, ddorwin wrote:
> > > > > Can we have the factory generate an ID in the CDM before outputting
it?
> > > > > 
> > > > > This solves the multiple-client problem and avoids clients passing in
> > > > > conflicting values.
> > > > > 
> > > > > Regarding the original reason (no synchronous Mojo calls), perhaps we
> can
> > > > return
> > > > > the cdm_id and cdm together here.
> > > > 
> > > > I looked at this more closely and IIUIC we can't get any value/struct
back
> > > from
> > > > a mojo service  synchronously. We can only get requested services back
> > > > synchronously. This is why we are providing a client side |cdm_id|.
> (+jamesr
> > > to
> > > > double check on this.)
> > > > 
> > > > I didn't realize this yesterday (I blame the holidays :) Sorry for the
> > > > confusion.
> > > 
> > > Uhh, I don't really understand.  All mojo calls are asynchronous.  You can
> > > connect to a service and start using it immediately since connecting to a
> > > service means allocating a message pipe and passing one handle which will
> > > asynchronously arrive at the other end.  In the meantime, you can write
into
> > the
> > > other handle.  But that's not the same thing as "getting something
> > > synchronously"
> > > 
> > > What's the problem you are trying to solve here?
> > 
> > Thanks for the explanation! I thought it's synchronous because we can use
the
> > interface (message pipe) immediately after we connect to the service. But
> > apparently I was wrong.
> > 
> > But with this I wonder how nullable interfaces (&?) work in  cases like: 
> > 
> > Create(string key_system,
> >            string security_origin,
> >            int32 cdm_id,
> >            ContentDecryptionModule&? cdm);
> > 
> > We'll always create a message pipe for the |cdm|, and pass it one handle to
> the
> > other side. At the other side, if we found that no service can be binded, I
> > guess the message pipe will be closed. Is that correct?
> > 
> > But that'll happen after Create() returns. What if I make a call on |cdm|
> before
> > the message pipe is closed? Will the call be dropped? What if I make a call
> > after the message pipe is closed? Will it crash? Or shall I check whether
> |cdm|
> > is valid before I make any calls to the |cdm|?
> 
> It's the same as if you pass a pipe handle of mojom type
ContentDecryptionModule
> to another service and the other end closes it without seeing your messages or
> crashes.  At some point you detect that the pipe is closed and you figure out
> what to do about it.
> 
> > 
> > For ddorwin's question about the |cdm_id|. We need to have a |cdm_id|
> available
> > immediately after Create() returns. Since everything is asyc, there's no way
> to
> > pass a |cdm_id| from the service back to the client. That why I choose to
have
> > the client side generating the |cdm_id| and pass it over to the service when
> > requesting the CDM service (in the above Create() call). I just wanted to
> double
> > check with you whether this seems right.
> 
> I don't think I know enough about the overall flow to give good feedback.  Is
> there a higher level doc somewhere describing these pieces and how they
> interact?

jamesr:

Sorry for the delay. I was working on something else before the branch cut.

I wrote a short doc introducing what SetCdm() is about and why we are using the
CDM ID. This should give you some background on what I am doing and why I am
doing it this way. Please let me know if you have any further questions.

http://goo.gl/kibNOZ

[jamesr, 2015-01-22 01:04:15]

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

[jamesr, 2015-01-22 01:04:16]

Thanks for writing up the doc but I'm afraid I haven't had time to look in
detail and am unlikely to find enough time to understand this part of the media
stack.  If there's a specific question you want to ask here please do so but I
don't think I can provide any useful advice about the overall architecture of
this system, which appears to be the question.

[xhwang, 2015-01-23 00:51:02]

On 2015/01/22 01:04:16, jamesr wrote:
> Thanks for writing up the doc but I'm afraid I haven't had time to look in
> detail and am unlikely to find enough time to understand this part of the
media
> stack.  If there's a specific question you want to ask here please do so but I
> don't think I can provide any useful advice about the overall architecture of
> this system, which appears to be the question.

jamesr: Thanks! I think your previous comment about the asynchronicity of mojo
calls already clarified my confusion. I'll let ddorwin to review the rest of the
CL. If I have more mojo specific questions I'll let you know.

ddorwin: PTAL

[xhwang, 2015-03-19 21:59:38]

ddorwin: I have rebased my CL and it is now ready for review. Thanks!

[ddorwin, 2015-05-15 19:10:08]

https://codereview.chromium.org/840473002/diff/160001/media/base/cdm_context.h
File media/base/cdm_context.h (right):

https://codereview.chromium.org/840473002/diff/160001/media/base/cdm_context....
media/base/cdm_context.h:52: // Note: |cdm_id| is irrelevant to GetCdmId() of
the returned CdmContext.
This is hard to understand. Perhaps they are not necessarily the same? And
|cdm_id| is for X while GetCdmId() is Y?

https://codereview.chromium.org/840473002/diff/160001/media/mojo/interfaces/c...
File media/mojo/interfaces/content_decryption_module_factory.mojom (right):

https://codereview.chromium.org/840473002/diff/160001/media/mojo/interfaces/c...
media/mojo/interfaces/content_decryption_module_factory.mojom:11: // supported
(e.g. unsupported key system).
Is that the only mechanism we have to report errors?
Does this need to change now that creation was made asynchronous in Blink &
Chromium? Should we use a similar promise model?

Update: I saw the TODOs in the .cc file. Perhaps they should be here since this
is the interface.

https://codereview.chromium.org/840473002/diff/160001/media/mojo/interfaces/c...
media/mojo/interfaces/content_decryption_module_factory.mojom:17: string
security_origin,
FYI, I believe we are now passing more config information.

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm.cc (right):

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
media/mojo/services/mojo_cdm.cc:135: NOTIMPLEMENTED();
Will this be implemented? Should there be a TODO?

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm.h (right):

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
media/mojo/services/mojo_cdm.h:60: CdmContext* GetCdmContext() final;
Is there a need to get this as a CdmContext pointer? The public definition
inherits from CdmContext, so it's clear that the returned value should be
"this". Do we even need this method?

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm_factory.cc (right):

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_factory.cc:11: int MojoCdmFactory::next_cdm_id_ =
MojoCdm::kInvalidCdmId + 1;
This somewhat assumes that the Invalid value is a low number (i.e. 0). Perhaps
we should define kInitialCdmId at the same location and use it here.

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm_service.cc (right):

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service.cc:24: // Only AesDecryptor is supported.
TODO with bug?

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service.cc:130: DVLOG(2) << __FUNCTION__;
Should this be a DLOG(ERROR)?

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm_service_context.cc (right):

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.cc:24: // TODO(xhwang): How to use
|security_origin| here?
It needs to be used for any persistence. Probably also for *checking* user
permissions (the prompting has already happened). Non-Blink-based Mojo clients
would need to handle this themselves.

It looks like we need a TODO to check permissions somewhere.

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm_service_context.h (right):

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.h:22: // Creates and returns a
MojoCdmService for |key_system|, |security_origin|
"for" doesn't really apply to the last two parameters.

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.h:30: CdmContext*
GetCdmContext(int32_t cdm_id) override;
Why wouldn't a caller just get the context from the MojoCdmService?
If there is a use case where the Service is not known, would it make more sense
to get the service based on the ID? (I don't know - just asking.)

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.h:36: // A map between CDM IDs and
MojoCdnServices, which owns all MojoCdmServices
typo: "Cdn"

grammar: ", which" refers to MojoCdnServices. That means MCS owns all of itself.
I think the RHS is referring to the map. Probably make it a separate sentence.

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
File media/mojo/services/mojo_media_application.cc (right):

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
media/mojo/services/mojo_media_application.cc:30:
command_line->InitFromArgv(command_line_args);
What is all this command line stuff used for? The following code doesn't appear
to be based on |args|.

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
File media/mojo/services/mojo_renderer_service.cc (right):

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
media/mojo/services/mojo_renderer_service.cc:118: LOG(ERROR) << "CDM context
provider not available.";
Should this be more helpful? For example, it wasn't provided in the constructor
so EME is not supported.
Should it be a DCHECK to call SetCdm after not providing a context? (I'm not
sure whether the same code owns both responsibilities.)

Note: Making the member "* const" would make it clear that - as I assume above -
the member cannot be changed after construction.

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
media/mojo/services/mojo_renderer_service.cc:125: LOG(ERROR) << "CDM context not
found: " << cdm_id;
Not DLOG? Same above.

suggestion: s/: / for /

[xhwang, 2015-06-01 21:21:25]

Parts of this CL is obsolete. So I am splitting this CL to smaller CLs for easy
discussion and review.

Comments related to MojoCdmService and MojoCdmServiceContext are addressed in
https://codereview.chromium.org/1165633004

I'll address the remaining comments in future CLs.

https://codereview.chromium.org/840473002/diff/160001/media/base/cdm_context.h
File media/base/cdm_context.h (right):

https://codereview.chromium.org/840473002/diff/160001/media/base/cdm_context....
media/base/cdm_context.h:52: // Note: |cdm_id| is irrelevant to GetCdmId() of
the returned CdmContext.
On 2015/05/15 19:10:07, ddorwin wrote:
> This is hard to understand. Perhaps they are not necessarily the same? And
> |cdm_id| is for X while GetCdmId() is Y?

Updated comment with more restrictions.

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm_service.cc (right):

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service.cc:24: // Only AesDecryptor is supported.
On 2015/05/15 19:10:07, ddorwin wrote:
> TODO with bug?

Done.

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service.cc:130: DVLOG(2) << __FUNCTION__;
On 2015/05/15 19:10:07, ddorwin wrote:
> Should this be a DLOG(ERROR)?

This will happen when the client (MojoCdm) is destroyed, so it's not necessarily
an error.

Changed the log level to 1 though.

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm_service_context.cc (right):

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.cc:24: // TODO(xhwang): How to use
|security_origin| here?
On 2015/05/15 19:10:07, ddorwin wrote:
> It needs to be used for any persistence. Probably also for *checking* user
> permissions (the prompting has already happened). Non-Blink-based Mojo clients
> would need to handle this themselves.
> 
> It looks like we need a TODO to check permissions somewhere.

Currently we are using WebEncryptedMediaClientImpl in html_viewer, so permission
will be requested the same way as in Chromium. However, we don't have a
MediaPermission implementation in html_viewer yet. Currently we are using a
default one which always allows the permission request:

https://code.google.com/p/chromium/codesearch#chromium/src/components/html_vi...

I added a TODO in media_factory.cc.

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm_service_context.h (right):

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.h:22: // Creates and returns a
MojoCdmService for |key_system|, |security_origin|
On 2015/05/15 19:10:07, ddorwin wrote:
> "for" doesn't really apply to the last two parameters.

Done.

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.h:30: CdmContext*
GetCdmContext(int32_t cdm_id) override;
On 2015/05/15 19:10:07, ddorwin wrote:
> Why wouldn't a caller just get the context from the MojoCdmService?
> If there is a use case where the Service is not known, would it make more
sense
> to get the service based on the ID? (I don't know - just asking.)

The caller (MojoRendererService) needs to find the correct CdmContext given a
CDM ID. This is because media::Renderer only care about CdmContext:

https://code.google.com/p/chromium/codesearch#chromium/src/media/base/rendere...

We could have something like MojoCdmService* GetCdm(cdm_id), then
mojo_cdm_service->GetCdmContext(). But IMHO, MojoRendererService doesn't need to
know MojoCdmService. So a single GetCdmContext() seems better to me.

https://codereview.chromium.org/840473002/diff/160001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.h:36: // A map between CDM IDs and
MojoCdnServices, which owns all MojoCdmServices
On 2015/05/15 19:10:07, ddorwin wrote:
> typo: "Cdn"
> 
> grammar: ", which" refers to MojoCdnServices. That means MCS owns all of
itself.
> I think the RHS is referring to the map. Probably make it a separate sentence.

Done.

[xhwang, 2015-06-24 16:13:28]

This CL has been rebased, refactored and split into multiple smaller CLs, most
of which have been landed. As such, I'll close this CL now.