media: Add MediaDrmBridgeCdmContext.

media: Add MediaDrmBridgeCdmContext.

Currently we pass CdmContext to AudioDecoder, VideoDecoder and media
Renderers. This CL adds a CdmContext implementation for MediaDrmBridge
so that we can use it in our media pipeline, e.g. in
MediaCodecAudioDecoder.

BUG=521731
Committed: https://crrev.com/2086800d91c37fd5bbf12251675c433f71540cc7
Cr-Commit-Position: refs/heads/master@{#382653}

[xhwang, 2016-03-17 21:26:51]

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

[xhwang, 2016-03-17 21:26:52]

PTAL

This CL is necessary to use MediaDrmBridge in MediaCodecAudioDecoder, which is
an AudioDecoder that takes CdmContext.

[xhwang, 2016-03-17 22:20:37]

CC timav@chromium.org FYI!

[ddorwin, 2016-03-18 18:27:14]

https://chromiumcodereview.appspot.com/1808983002/diff/20001/content/common/g...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1808983002/diff/20001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:398:
base::Bind(&AndroidVideoDecodeAccelerator::OnMediaCryptoReady,
If I understand correctly, this is the only reason MediaDrmBridgeCdmContext
exists (after the player interface is removed per the TODO). It seems
wrong/hacky to use an interface just to pass an object that can be cast to
something else, especially because the interface APIs aren't implemented, but I
don't have a better solution.

https://chromiumcodereview.appspot.com/1808983002/diff/20001/media/base/andro...
File media/base/android/media_drm_bridge_cdm_context.h (right):

https://chromiumcodereview.appspot.com/1808983002/diff/20001/media/base/andro...
media/base/android/media_drm_bridge_cdm_context.h:21: // Methods can be called
on any thread. The registered callbacks will be fired
Describe the purpose of this class.

Specifically, it's weird that the CdmContext interface only returns invalid
values.

I worry about other pieces of code using this or calling GetCdmContext on
MediaDrmBridge expecting this to be a real CdmContext.

To address the former, perhaps this should be a nested class. I guess the name
implies that already, but scoping might be a bit better.

https://chromiumcodereview.appspot.com/1808983002/diff/20001/media/base/andro...
media/base/android/media_drm_bridge_cdm_context.h:32: // |media_crypto| - global
reference to MediaCrypto object
What does global reference mean?

https://chromiumcodereview.appspot.com/1808983002/diff/20001/media/base/andro...
media/base/android/media_drm_bridge_cdm_context.h:37: // The |media_drm_bridge|
owns |this| is guaranteed to outlive |this|.
There seems to be a word missing. Perhaps either "that owns" or "and is."

https://chromiumcodereview.appspot.com/1808983002/diff/20001/media/base/andro...
media/base/android/media_drm_bridge_cdm_context.h:46: // PlayerTracker
implementation. Can be called on any thread.
Nit: newline after the first sentence. Such lines are usually separate to stand
out.

https://chromiumcodereview.appspot.com/1808983002/diff/20001/media/base/andro...
media/base/android/media_drm_bridge_cdm_context.h:50: // Note: RegisterPlayer()
should be called before SetMediaCryptoReadyCB() to
s/should/must/?

[xhwang, 2016-03-18 22:07:51]

comments addressed

[xhwang, 2016-03-18 22:10:46]

PTAL again.

https://chromiumcodereview.appspot.com/1808983002/diff/20001/content/common/g...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1808983002/diff/20001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:398:
base::Bind(&AndroidVideoDecodeAccelerator::OnMediaCryptoReady,
On 2016/03/18 18:27:14, ddorwin wrote:
> If I understand correctly, this is the only reason MediaDrmBridgeCdmContext
> exists (after the player interface is removed per the TODO). It seems
> wrong/hacky to use an interface just to pass an object that can be cast to
> something else, especially because the interface APIs aren't implemented, but
I
> don't have a better solution.

We also need to register AndroidVideoDecodeAccelerator::OnKeyAdded() somehow.

I agree about the hackiness of the cast. But I don't see any way around it. This
CL just makes sure we are casting a smaller interface than the whole CDM
(MediaKeys), which we agreed would be bad to be exposed to the media player.

https://chromiumcodereview.appspot.com/1808983002/diff/20001/media/base/andro...
File media/base/android/media_drm_bridge_cdm_context.h (right):

https://chromiumcodereview.appspot.com/1808983002/diff/20001/media/base/andro...
media/base/android/media_drm_bridge_cdm_context.h:21: // Methods can be called
on any thread. The registered callbacks will be fired
On 2016/03/18 18:27:14, ddorwin wrote:
> Describe the purpose of this class.
> 
> Specifically, it's weird that the CdmContext interface only returns invalid
> values.
> 
> I worry about other pieces of code using this or calling GetCdmContext on
> MediaDrmBridge expecting this to be a real CdmContext.

I updated the CdmContext documentation to make this clear.
 
> To address the former, perhaps this should be a nested class. I guess the name
> implies that already, but scoping might be a bit better.

I like a separate file better, so that the media player/decoder doesn't need to
know about MediaDrmBridge at all, another layer of separation. But I don't have
strong opinions on this :)

https://chromiumcodereview.appspot.com/1808983002/diff/20001/media/base/andro...
media/base/android/media_drm_bridge_cdm_context.h:32: // |media_crypto| - global
reference to MediaCrypto object
On 2016/03/18 18:27:14, ddorwin wrote:
> What does global reference mean?

I think this is because the JavaObjectPtr is a ScopedJavaGlobalRef. But it seems
we don't need it in this comment. Updated.

https://chromiumcodereview.appspot.com/1808983002/diff/20001/media/base/andro...
media/base/android/media_drm_bridge_cdm_context.h:37: // The |media_drm_bridge|
owns |this| is guaranteed to outlive |this|.
On 2016/03/18 18:27:14, ddorwin wrote:
> There seems to be a word missing. Perhaps either "that owns" or "and is."

Done.

https://chromiumcodereview.appspot.com/1808983002/diff/20001/media/base/andro...
media/base/android/media_drm_bridge_cdm_context.h:46: // PlayerTracker
implementation. Can be called on any thread.
On 2016/03/18 18:27:14, ddorwin wrote:
> Nit: newline after the first sentence. Such lines are usually separate to
stand
> out.

Done.

https://chromiumcodereview.appspot.com/1808983002/diff/20001/media/base/andro...
media/base/android/media_drm_bridge_cdm_context.h:50: // Note: RegisterPlayer()
should be called before SetMediaCryptoReadyCB() to
On 2016/03/18 18:27:14, ddorwin wrote:
> s/should/must/?

Done.

[xhwang, 2016-03-18 22:40:56]

CCing watk@ in case you are interested as well.

[ddorwin, 2016-03-21 18:06:09]

Mostly thoughts on CdmContext.

https://codereview.chromium.org/1808983002/diff/40001/media/base/android/medi...
File media/base/android/media_drm_bridge_cdm_context.h (right):

https://codereview.chromium.org/1808983002/diff/40001/media/base/android/medi...
media/base/android/media_drm_bridge_cdm_context.h:46: // PlayerTracker
implementation. Can be called on any thread.
I shouldn't have used "newline." I meant:

// PlayerTracker implementation.
// Methods can be called on any thread...

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

https://codereview.chromium.org/1808983002/diff/40001/media/base/cdm_context....
media/base/cdm_context.h:21: // to connect the CDM with a media player. In this
case, the CdmContext
Is there a reason that the CDM ID couldn't be used to obtain the MediaCrypto? I
guess we'd have to do a lookup in a manager, which wouldn't be ideal, but
neither is casting.

If that's not ideal, should we even support CDM ID? Any implementation needing
to use an ID could implement it. The primary argument against this is likely
that we still use the ID in the renderer process (right?).

Using the same type for very different reasons in different processes makes
improvements here difficult. We're doing/allowing some things very specific to
the privileged process usage of this interface that don't make sense on the
renderer side.

If we make even AesDecryptor a Mojo service (implemented in the renderer) I
wonder if we even need these interfaces in the renderer side once we switch all
platforms to Mojo. In the meantime, I wonder if we shouldn't have different
objects, starting with PlatformMediaKeys : public MediaKeys, which would allow
different CdmContexts to be returned.

Perhaps we should formalize the pattern recommended here by having a void*
returning method, though that might not be any better.

Alternatively, if we think a CetFooReadyCB might be useful, we could add such a
method that takes a void* and let the implementation do the casting. It seems
better for the casting to be clearly intended that way and to be done by the
implementation than to put that responsibility on the caller/user of the API.

https://codereview.chromium.org/1808983002/diff/40001/media/base/cdm_context....
media/base/cdm_context.h:23: // should cast the CdmContext to the correct type
to access such APIs.
It seems strange to provide such recommendations in an API definition,
especially since there are some subtleties, such as ensuring that is the only
possible type.

https://codereview.chromium.org/1808983002/diff/40001/media/base/cdm_context....
media/base/cdm_context.h:32: // CDM does not support a Decryptor. Must not
return nullptr if GetCdmId()
This is not true in the new child.

https://codereview.chromium.org/1808983002/diff/40001/media/base/cdm_context....
media/base/cdm_context.h:40: // kInvalidCdmId if GetDecryptor() returns nullptr.
ditto

[ddorwin, 2016-03-21 21:40:25]

https://codereview.chromium.org/1808983002/diff/40001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1808983002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:291:
static_cast<media::MediaDrmBridgeCdmContext*>(cdm_->GetCdmContext())
It would be nice to only cast once.
Also, we should avoid using |cdm_| ever since it only exists for holding a
reference.

The best solution then seems to be a MediaDrmBridgeCdmContext* member.

We might also rename |cdm_| to |cdm_reference_do_not_use_| or something like
that.

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

https://codereview.chromium.org/1808983002/diff/40001/media/base/cdm_context....
media/base/cdm_context.h:20: // There are CDMs that support neither Decryptor
nor CDM ID, but use other ways
We should probably move this to the new subclass as an explanation of how it
works. Changing the comments (and thus definitions) below will make this
behavior not so unexpected.

https://codereview.chromium.org/1808983002/diff/40001/media/base/cdm_context....
media/base/cdm_context.h:32: // CDM does not support a Decryptor. Must not
return nullptr if GetCdmId()
On 2016/03/21 18:06:09, ddorwin wrote:
> This is not true in the new child.

As discussed, we should instead describe when each of these are valid. That will
allow the new subclass not to be an exception.

[xhwang, 2016-03-22 17:07:40]

comments addressed

[xhwang, 2016-03-22 17:08:32]

I think I addressed all comments. PTAL again!

https://chromiumcodereview.appspot.com/1808983002/diff/40001/content/common/g...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1808983002/diff/40001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:291:
static_cast<media::MediaDrmBridgeCdmContext*>(cdm_->GetCdmContext())
On 2016/03/21 21:40:25, ddorwin wrote:
> It would be nice to only cast once.
> Also, we should avoid using |cdm_| ever since it only exists for holding a
> reference.
> 
> The best solution then seems to be a MediaDrmBridgeCdmContext* member.
> 
> We might also rename |cdm_| to |cdm_reference_do_not_use_| or something like
> that.

Done.

https://chromiumcodereview.appspot.com/1808983002/diff/40001/media/base/andro...
File media/base/android/media_drm_bridge_cdm_context.h (right):

https://chromiumcodereview.appspot.com/1808983002/diff/40001/media/base/andro...
media/base/android/media_drm_bridge_cdm_context.h:46: // PlayerTracker
implementation. Can be called on any thread.
On 2016/03/21 18:06:09, ddorwin wrote:
> I shouldn't have used "newline." I meant:
> 
> // PlayerTracker implementation.
> // Methods can be called on any thread...

Done.

https://chromiumcodereview.appspot.com/1808983002/diff/40001/media/base/cdm_c...
File media/base/cdm_context.h (right):

https://chromiumcodereview.appspot.com/1808983002/diff/40001/media/base/cdm_c...
media/base/cdm_context.h:20: // There are CDMs that support neither Decryptor
nor CDM ID, but use other ways
On 2016/03/21 21:40:25, ddorwin wrote:
> We should probably move this to the new subclass as an explanation of how it
> works. Changing the comments (and thus definitions) below will make this
> behavior not so unexpected.

Done.

https://chromiumcodereview.appspot.com/1808983002/diff/40001/media/base/cdm_c...
media/base/cdm_context.h:21: // to connect the CDM with a media player. In this
case, the CdmContext
On 2016/03/21 18:06:09, ddorwin wrote:
> Is there a reason that the CDM ID couldn't be used to obtain the MediaCrypto?
I
> guess we'd have to do a lookup in a manager, which wouldn't be ideal, but
> neither is casting.
> 
> If that's not ideal, should we even support CDM ID? Any implementation needing
> to use an ID could implement it. The primary argument against this is likely
> that we still use the ID in the renderer process (right?).
> 
> Using the same type for very different reasons in different processes makes
> improvements here difficult. We're doing/allowing some things very specific to
> the privileged process usage of this interface that don't make sense on the
> renderer side.
> 
> If we make even AesDecryptor a Mojo service (implemented in the renderer) I
> wonder if we even need these interfaces in the renderer side once we switch
all
> platforms to Mojo. In the meantime, I wonder if we shouldn't have different
> objects, starting with PlatformMediaKeys : public MediaKeys, which would allow
> different CdmContexts to be returned.
> 
> Perhaps we should formalize the pattern recommended here by having a void*
> returning method, though that might not be any better.
> 
> Alternatively, if we think a CetFooReadyCB might be useful, we could add such
a
> method that takes a void* and let the implementation do the casting. It seems
> better for the casting to be clearly intended that way and to be done by the
> implementation than to put that responsibility on the caller/user of the API.

Acknowledged.

https://chromiumcodereview.appspot.com/1808983002/diff/40001/media/base/cdm_c...
media/base/cdm_context.h:23: // should cast the CdmContext to the correct type
to access such APIs.
On 2016/03/21 18:06:09, ddorwin wrote:
> It seems strange to provide such recommendations in an API definition,
> especially since there are some subtleties, such as ensuring that is the only
> possible type.

Done.

https://chromiumcodereview.appspot.com/1808983002/diff/40001/media/base/cdm_c...
media/base/cdm_context.h:32: // CDM does not support a Decryptor. Must not
return nullptr if GetCdmId()
On 2016/03/21 18:06:09, ddorwin wrote:
> This is not true in the new child.

Done.

https://chromiumcodereview.appspot.com/1808983002/diff/40001/media/base/cdm_c...
media/base/cdm_context.h:32: // CDM does not support a Decryptor. Must not
return nullptr if GetCdmId()
On 2016/03/21 21:40:25, ddorwin wrote:
> On 2016/03/21 18:06:09, ddorwin wrote:
> > This is not true in the new child.
> 
> As discussed, we should instead describe when each of these are valid. That
will
> allow the new subclass not to be an exception.

Done.

https://chromiumcodereview.appspot.com/1808983002/diff/40001/media/base/cdm_c...
media/base/cdm_context.h:40: // kInvalidCdmId if GetDecryptor() returns nullptr.
On 2016/03/21 18:06:09, ddorwin wrote:
> ditto

Done.

[ddorwin, 2016-03-22 17:20:12]

LGTM with nits. Thanks.

https://chromiumcodereview.appspot.com/1808983002/diff/60001/content/common/g...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1808983002/diff/60001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:380: // On Android
platform the CdmContext must be a MediaDrmBridgeCdmContext.
nit: This sounds like this is true for all instances on Android, but it's only
true on the implementation/privileged side.

https://chromiumcodereview.appspot.com/1808983002/diff/60001/media/base/andro...
File media/base/android/media_drm_bridge_cdm_context.h (right):

https://chromiumcodereview.appspot.com/1808983002/diff/60001/media/base/andro...
media/base/android/media_drm_bridge_cdm_context.h:23: // MediaCodec based
decoders should cast the given CdmContext to this class to
nit '-' before "based"

https://chromiumcodereview.appspot.com/1808983002/diff/60001/media/base/cdm_c...
File media/base/cdm_context.h (right):

https://chromiumcodereview.appspot.com/1808983002/diff/60001/media/base/cdm_c...
media/base/cdm_context.h:27: // CDM does not support a Decryptor. The returned
object is only guaranteed to
nit: Should we add something like "(i.e. platform-based CDMs where decryption
occurs implicitly along with decoding)"?

https://chromiumcodereview.appspot.com/1808983002/diff/60001/media/base/cdm_c...
media/base/cdm_context.h:31: // Returns an ID that can be used to identify a CDM
remotely, in which case
nit: I'm not sure what "identify... remotely" means. "remote CDM" may be
sufficient.

https://chromiumcodereview.appspot.com/1808983002/diff/60001/media/base/cdm_c...
media/base/cdm_context.h:33: // remote CDM is not supported (e.g. this CDM is a
concrete CDM).
nit: I'm not sure what a "concrete CDM" is. Perhaps "this object directly
represents the CDM" or something like that.

[xhwang, 2016-03-22 17:35:11]

https://chromiumcodereview.appspot.com/1808983002/diff/60001/media/base/andro...
File media/base/android/media_drm_bridge_cdm_context.h (right):

https://chromiumcodereview.appspot.com/1808983002/diff/60001/media/base/andro...
media/base/android/media_drm_bridge_cdm_context.h:23: // MediaCodec based
decoders should cast the given CdmContext to this class to
On 2016/03/22 17:20:12, ddorwin wrote:
> nit '-' before "based"

Done.

https://chromiumcodereview.appspot.com/1808983002/diff/60001/media/base/cdm_c...
File media/base/cdm_context.h (right):

https://chromiumcodereview.appspot.com/1808983002/diff/60001/media/base/cdm_c...
media/base/cdm_context.h:27: // CDM does not support a Decryptor. The returned
object is only guaranteed to
On 2016/03/22 17:20:12, ddorwin wrote:
> nit: Should we add something like "(i.e. platform-based CDMs where decryption
> occurs implicitly along with decoding)"?

Done.

https://chromiumcodereview.appspot.com/1808983002/diff/60001/media/base/cdm_c...
media/base/cdm_context.h:31: // Returns an ID that can be used to identify a CDM
remotely, in which case
On 2016/03/22 17:20:12, ddorwin wrote:
> nit: I'm not sure what "identify... remotely" means. "remote CDM" may be
> sufficient.

Done.

https://chromiumcodereview.appspot.com/1808983002/diff/60001/media/base/cdm_c...
media/base/cdm_context.h:33: // remote CDM is not supported (e.g. this CDM is a
concrete CDM).
On 2016/03/22 17:20:12, ddorwin wrote:
> nit: I'm not sure what a "concrete CDM" is. Perhaps "this object directly
> represents the CDM" or something like that.

Done. Used "local" in comparison to "remote".

[xhwang, 2016-03-22 17:35:42]

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

[xhwang, 2016-03-22 17:35:43]

sandersd: Please OWNERS reivew
content/common/gpu/media/android_video_decode_accelerator.*

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

rl lgtm

[xhwang, 2016-03-22 19:11:39]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2016-03-22 19:11:39]

The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1808983002/#ps80001
(title: "nits")

[commit-bot: I haz the power, 2016-03-22 19:12:10]

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

[commit-bot: I haz the power, 2016-03-22 19:49:18]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-22 19:50:30]

Description was changed from

==========
media: Add MediaDrmBridgeCdmContext.

Currently we pass CdmContext to AudioDecoder, VideoDecoder and media
Renderers. This CL adds a CdmContext implementation for MediaDrmBridge
so that we can use it in our media pipeline, e.g. in
MediaCodecAudioDecoder.

BUG=521731
==========

to

==========
media: Add MediaDrmBridgeCdmContext.

Currently we pass CdmContext to AudioDecoder, VideoDecoder and media
Renderers. This CL adds a CdmContext implementation for MediaDrmBridge
so that we can use it in our media pipeline, e.g. in
MediaCodecAudioDecoder.

BUG=521731

Committed: https://crrev.com/2086800d91c37fd5bbf12251675c433f71540cc7
Cr-Commit-Position: refs/heads/master@{#382653}
==========

[commit-bot: I haz the power, 2016-03-22 19:50:31]

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