Configure MediaCodec with CDM in ADVA

Configure MediaCodec with CDM in ADVA

Implemented SetCdm() by retrieving the CDM object from mojo
(it should be MediaDrmBridge), registering this bridge
with nesessary callbacks and postponing the MediaCodec configuration
till we get the MediaCrypto object.

After MediaCrypto object is obtained we configure MediaCodec
and only then report the SetCdm() status by NotifyCdmAttached(bool).
With this approach any failure in MediaCodec creation is reported as
a CDM failure.

BUG=542417
Committed: https://crrev.com/39cbd4546d7a6300afde7e8c1816541323a1abb5
Cr-Commit-Position: refs/heads/master@{#368679}

[Tima Vaisburd, 2015-11-30 19:51:23]

Description was changed from

==========
Configure MediaCodec with CDM in ADVA - DO NOT LAND, JUST TESTING

Implemented SetCdm() by retrieving the CDM object from mojo
(it should be MediaDrmBridge), registering this bridge
with nesessary callbacks and postponing the MediaCodec configuration
till we get the MediaCrypto object.

The reply about SetCdm() status is postponed after the MediaCodec
configuration - this is currently wrong and is the reason why this
CL should not land.

BUG=542417
==========

to

==========
Configure MediaCodec with CDM in ADVA - DO NOT LAND, JUST TESTING

Implemented SetCdm() by retrieving the CDM object from mojo
(it should be MediaDrmBridge), registering this bridge
with nesessary callbacks and postponing the MediaCodec configuration
till we get the MediaCrypto object.

After MediaCrypto object is obtained we configure MediaCodec
and only then report the SetCdm() status by NotifyCdmAttached(bool).
With this approach any failure in MediaCodec creation is reported as a CDM
failure.

BUG=542417
==========

[Tima Vaisburd, 2015-11-30 19:51:40]

Description was changed from

==========
Configure MediaCodec with CDM in ADVA - DO NOT LAND, JUST TESTING

Implemented SetCdm() by retrieving the CDM object from mojo
(it should be MediaDrmBridge), registering this bridge
with nesessary callbacks and postponing the MediaCodec configuration
till we get the MediaCrypto object.

After MediaCrypto object is obtained we configure MediaCodec
and only then report the SetCdm() status by NotifyCdmAttached(bool).
With this approach any failure in MediaCodec creation is reported as a CDM
failure.

BUG=542417
==========

to

==========
Configure MediaCodec with CDM in ADVA - DO NOT LAND, JUST TESTING

Implemented SetCdm() by retrieving the CDM object from mojo
(it should be MediaDrmBridge), registering this bridge
with nesessary callbacks and postponing the MediaCodec configuration
till we get the MediaCrypto object.

After MediaCrypto object is obtained we configure MediaCodec
and only then report the SetCdm() status by NotifyCdmAttached(bool).
With this approach any failure in MediaCodec creation is reported as
a CDM failure.

BUG=542417
==========

[Tima Vaisburd, 2015-12-03 20:51:27]

Description was changed from

==========
Configure MediaCodec with CDM in ADVA - DO NOT LAND, JUST TESTING

Implemented SetCdm() by retrieving the CDM object from mojo
(it should be MediaDrmBridge), registering this bridge
with nesessary callbacks and postponing the MediaCodec configuration
till we get the MediaCrypto object.

After MediaCrypto object is obtained we configure MediaCodec
and only then report the SetCdm() status by NotifyCdmAttached(bool).
With this approach any failure in MediaCodec creation is reported as
a CDM failure.

BUG=542417
==========

to

==========
Configure MediaCodec with CDM in ADVA

Implemented SetCdm() by retrieving the CDM object from mojo
(it should be MediaDrmBridge), registering this bridge
with nesessary callbacks and postponing the MediaCodec configuration
till we get the MediaCrypto object.

After MediaCrypto object is obtained we configure MediaCodec
and only then report the SetCdm() status by NotifyCdmAttached(bool).
With this approach any failure in MediaCodec creation is reported as
a CDM failure.

BUG=542417
==========

[Tima Vaisburd, 2015-12-04 00:41:54]

timav@chromium.org changed reviewers:
+ liberato@chromium.org, xhwang@chromium.org

[Tima Vaisburd, 2015-12-04 00:41:54]

PTAL

[liberato (no reviews please), 2015-12-04 16:20:11]

liberato@chromium.org changed reviewers:
+ watk@chromium.org

[liberato (no reviews please), 2015-12-04 16:20:11]

overall looks good.  also adding watk to see how this fits with the full screen
picture he has.

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

https://codereview.chromium.org/1469353010/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:151:
surface_texture_ = strategy_->CreateSurfaceTexture();
how does this fit with |needs_protected_surface|?  it will have to be deferred
until we know that.  plus, this will interact with full screen, where we also
don't know what type of surface to use.

it might be worthwhile to centralize the deferred initialization logic in a new
function, e.g., FinishInitializationIfPossible(). it checks to see if it has
enough info to configure surface and create the media codec, then does so if it
can.

eventually, i think that our initialization steps have to be more explicit than
"missing a media codec" or not, since it's going to get more complicated.  we
might be waiting for the cdm and/or waiting for the protected/fullscreen
surface.  i don't think that we have a picture of this written down anywhere
that includes both watk's fullscreen and cdm, but we should.

|state_| might also be useful, to record where in the initialization process we
are, or at least if we're UNINITIALIZED, PARTIALLY_INITIALIED, or NO_ERROR
(fully initialized and ready to go).  right now, we pretty much have to use
|codec_| for that, but it might be more explicit.  then, it's much more clear in
DoIOTask etc. what we're really trying to check for.  we don't need to change
how state_ is interpreted as part of this CL, but it's worth keeping in mind.

https://codereview.chromium.org/1469353010/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:153: // For
encrypted streams we postpone configuration until MediaCrypto is
all of this would become a call to FinishInitializationIfPossible().  it would
do nothing if |is_encrypted_| && !|media_crypto_|.

https://codereview.chromium.org/1469353010/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:176: // On Android
platform the MediaKeys fill be its subclass MediaDrmBridge.
s/fill/will

https://codereview.chromium.org/1469353010/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:214: 
might want to check for |media_codec_| here, since there are more ways into this
function than just the timer started in ConfigureMediaCodec.  an errant decode
request would get here.

https://codereview.chromium.org/1469353010/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:672:
DCHECK(!media_crypto->is_null());
DCHECK(!media_codec)

https://codereview.chromium.org/1469353010/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:678:
NotifyCdmAttached(success);
outside the scope of this CL, but NotifyInitializationFailed might be worth
splitting out someday.

[Tima Vaisburd, 2015-12-04 20:12:01]

timav@chromium.org changed reviewers:
+ posciak@chromium.org

[Tima Vaisburd, 2015-12-04 20:12:01]

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

https://codereview.chromium.org/1469353010/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:151:
surface_texture_ = strategy_->CreateSurfaceTexture();
On 2015/12/04 16:20:11, liberato wrote:
> how does this fit with |needs_protected_surface|?  it will have to be deferred
> until we know that.  plus, this will interact with full screen, where we also
> don't know what type of surface to use.
> 
> it might be worthwhile to centralize the deferred initialization logic in a
new
> function, e.g., FinishInitializationIfPossible(). it checks to see if it has
> enough info to configure surface and create the media codec, then does so if
it
> can.
> 
> eventually, i think that our initialization steps have to be more explicit
than
> "missing a media codec" or not, since it's going to get more complicated.  we
> might be waiting for the cdm and/or waiting for the protected/fullscreen
> surface.  i don't think that we have a picture of this written down anywhere
> that includes both watk's fullscreen and cdm, but we should.
> 
> |state_| might also be useful, to record where in the initialization process
we
> are, or at least if we're UNINITIALIZED, PARTIALLY_INITIALIED, or NO_ERROR
> (fully initialized and ready to go).  right now, we pretty much have to use
> |codec_| for that, but it might be more explicit.  then, it's much more clear
in
> DoIOTask etc. what we're really trying to check for.  we don't need to change
> how state_ is interpreted as part of this CL, but it's worth keeping in mind.
> 

Can the protected surface be deferred in the subsequent CL? Meanwhile I just
fail the initialization, saying we do not support protected surfaces. WDYT?

https://codereview.chromium.org/1469353010/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:176: // On Android
platform the MediaKeys fill be its subclass MediaDrmBridge.
On 2015/12/04 16:20:11, liberato wrote:
> s/fill/will

Done.

https://codereview.chromium.org/1469353010/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:214: 
On 2015/12/04 16:20:11, liberato wrote:
> might want to check for |media_codec_| here, since there are more ways into
this
> function than just the timer started in ConfigureMediaCodec.  an errant decode
> request would get here.

Please explain how. I've assumed that initialization must succeed in order to
start buffers processing.

https://codereview.chromium.org/1469353010/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:672:
DCHECK(!media_crypto->is_null());
On 2015/12/04 16:20:11, liberato wrote:
> DCHECK(!media_codec)

I added this DCHECK, but ConfigureMediaCodec() does media_codec_.reset(...), so
it seems even having prior MediaCodec would be harmless?

https://codereview.chromium.org/1469353010/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:678:
NotifyCdmAttached(success);
On 2015/12/04 16:20:11, liberato wrote:
> outside the scope of this CL, but NotifyInitializationFailed might be worth
> splitting out someday.

I heard that there is a work on Version 2 which will have asynchronous
initialization, I think this kind of notification belong in there?

[xhwang, 2015-12-04 20:22:03]

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

https://chromiumcodereview.appspot.com/1469353010/diff/20001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:161: void
AndroidVideoDecodeAccelerator::SetCdm(int cdm_id) {
This should only be called after Initialize(), otherwise bad things will happen.
How do we DCHECK that? We can check client_, but it seems hacky.

https://chromiumcodereview.appspot.com/1469353010/diff/20001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:291: return;  //
keep trying to enqueue the front pending buffer
This is an important event. Add a log here.

https://chromiumcodereview.appspot.com/1469353010/diff/20001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:671:
DCHECK(media_crypto);
With my CL, |media_crypto| can actually be null if creation failed (e.g. due to
provisioning failure). We need to handle that case here.

https://chromiumcodereview.appspot.com/1469353010/diff/20001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:682: DVLOG(1) <<
__FUNCTION__;
NOTIMPLEMENTED()

Also add a TODO to finish this part.

Or do you want to finish this in this CL? Without this, I am not sure whether
the change in AndroidVideoDecodeAccelerator::QueueInput() is sufficient.

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

https://chromiumcodereview.appspot.com/1469353010/diff/20001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.h:157: void
OnCdmKeyAdded();
OnKeyAdded() to be consistent with other places:

https://code.google.com/p/chromium/codesearch#search/&q=OnKeyAdded&sq=package...

[liberato (no reviews please), 2015-12-04 21:40:13]

lgtm

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

https://codereview.chromium.org/1469353010/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:151:
surface_texture_ = strategy_->CreateSurfaceTexture();
On 2015/12/04 20:12:01, Tima Vaisburd wrote:
> On 2015/12/04 16:20:11, liberato wrote:
> > how does this fit with |needs_protected_surface|?  it will have to be
deferred
> > until we know that.  plus, this will interact with full screen, where we
also
> > don't know what type of surface to use.
> > 
> > it might be worthwhile to centralize the deferred initialization logic in a
> new
> > function, e.g., FinishInitializationIfPossible(). it checks to see if it has
> > enough info to configure surface and create the media codec, then does so if
> it
> > can.
> > 
> > eventually, i think that our initialization steps have to be more explicit
> than
> > "missing a media codec" or not, since it's going to get more complicated. 
we
> > might be waiting for the cdm and/or waiting for the protected/fullscreen
> > surface.  i don't think that we have a picture of this written down anywhere
> > that includes both watk's fullscreen and cdm, but we should.
> > 
> > |state_| might also be useful, to record where in the initialization process
> we
> > are, or at least if we're UNINITIALIZED, PARTIALLY_INITIALIED, or NO_ERROR
> > (fully initialized and ready to go).  right now, we pretty much have to use
> > |codec_| for that, but it might be more explicit.  then, it's much more
clear
> in
> > DoIOTask etc. what we're really trying to check for.  we don't need to
change
> > how state_ is interpreted as part of this CL, but it's worth keeping in
mind.
> > 
> 
> Can the protected surface be deferred in the subsequent CL? Meanwhile I just
> fail the initialization, saying we do not support protected surfaces. WDYT?

sure, i didn't mean that protected surfaces have to be supported now.  that
would be hard :)

right now, deferred initialization feels a little tangled up with the cdm code.
it'll get more so when OnCdmMediaCryptoReady memorizes that protected surfaces
don't work because of code in Initialize.

i predict that avda will end up with a more explicit state-based initialization
(WAITING_FOR_CDM, WAITING_FOR_SURFACE).  i was suggesting just a small step in
that direction.

not a big deal.

https://codereview.chromium.org/1469353010/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:161: void
AndroidVideoDecodeAccelerator::SetCdm(int cdm_id) {
On 2015/12/04 20:22:03, xhwang wrote:
> This should only be called after Initialize(), otherwise bad things will
happen.
> How do we DCHECK that? We can check client_, but it seems hacky.

maybe additional values for |state_| are useful here.

https://codereview.chromium.org/1469353010/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:214: 
On 2015/12/04 20:12:01, Tima Vaisburd wrote:
> On 2015/12/04 16:20:11, liberato wrote:
> > might want to check for |media_codec_| here, since there are more ways into
> this
> > function than just the timer started in ConfigureMediaCodec.  an errant
decode
> > request would get here.
> 
> Please explain how. I've assumed that initialization must succeed in order to
> start buffers processing.

yeah, i suppose you're right.  |init_cb_| doesn't get signalled until after
NotifyCdmAttached.

https://codereview.chromium.org/1469353010/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:672:
DCHECK(!media_crypto->is_null());
On 2015/12/04 20:12:01, Tima Vaisburd wrote:
> On 2015/12/04 16:20:11, liberato wrote:
> > DCHECK(!media_codec)
> 
> I added this DCHECK, but ConfigureMediaCodec() does media_codec_.reset(...),
so
> it seems even having prior MediaCodec would be harmless?

it depends if we've sent in any buffers.

https://codereview.chromium.org/1469353010/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:678:
NotifyCdmAttached(success);
On 2015/12/04 20:12:01, Tima Vaisburd wrote:
> On 2015/12/04 16:20:11, liberato wrote:
> > outside the scope of this CL, but NotifyInitializationFailed might be worth
> > splitting out someday.
> 
> I heard that there is a work on Version 2 which will have asynchronous
> initialization, I think this kind of notification belong in there?

yes, that's why i think it's outside the scope of this CL.  mentioned it just
for completeness, in response to your comment about it in the CL description.

[Tima Vaisburd, 2015-12-04 22:54:51]

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

https://chromiumcodereview.appspot.com/1469353010/diff/20001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:161: void
AndroidVideoDecodeAccelerator::SetCdm(int cdm_id) {
On 2015/12/04 21:40:13, liberato wrote:
> On 2015/12/04 20:22:03, xhwang wrote:
> > This should only be called after Initialize(), otherwise bad things will
> happen.
> > How do we DCHECK that? We can check client_, but it seems hacky.
> 
> maybe additional values for |state_| are useful here.

Yes, I agree. For now I DCHECKed client_, I'll try to add states in a later CL

https://chromiumcodereview.appspot.com/1469353010/diff/20001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:291: return;  //
keep trying to enqueue the front pending buffer
On 2015/12/04 20:22:03, xhwang wrote:
> This is an important event. Add a log here.

The log line above already prints the status, unless you want this NO_KEY
message to be printed once.

https://chromiumcodereview.appspot.com/1469353010/diff/20001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:671:
DCHECK(media_crypto);
On 2015/12/04 20:22:03, xhwang wrote:
> With my CL, |media_crypto| can actually be null if creation failed (e.g. due
to
> provisioning failure). We need to handle that case here.

Done.

https://chromiumcodereview.appspot.com/1469353010/diff/20001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:682: DVLOG(1) <<
__FUNCTION__;
On 2015/12/04 20:22:03, xhwang wrote:
> NOTIMPLEMENTED()
> 
> Also add a TODO to finish this part.
> 
> Or do you want to finish this in this CL? Without this, I am not sure whether
> the change in AndroidVideoDecodeAccelerator::QueueInput() is sufficient.

Since in this CL we do not try to stop the decoding process in response to
NO_KEY error, I assume that when the key arrives, the next enqueue attempt will
just succeed. In this case there is nothing to do here.

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

https://chromiumcodereview.appspot.com/1469353010/diff/20001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.h:157: void
OnCdmKeyAdded();
On 2015/12/04 20:22:03, xhwang wrote:
> OnKeyAdded() to be consistent with other places:
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=OnKeyAdded&sq=package...

Done.

[xhwang, 2015-12-04 23:24:09]

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

https://chromiumcodereview.appspot.com/1469353010/diff/20001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:291: return;  //
keep trying to enqueue the front pending buffer
On 2015/12/04 22:54:51, Tima Vaisburd wrote:
> On 2015/12/04 20:22:03, xhwang wrote:
> > This is an important event. Add a log here.
> 
> The log line above already prints the status, unless you want this NO_KEY
> message to be printed once.

We need a log with higher level for unexpected events. How about a DVLOG(1)
here?

https://chromiumcodereview.appspot.com/1469353010/diff/20001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:682: DVLOG(1) <<
__FUNCTION__;
On 2015/12/04 22:54:51, Tima Vaisburd wrote:
> On 2015/12/04 20:22:03, xhwang wrote:
> > NOTIMPLEMENTED()
> > 
> > Also add a TODO to finish this part.
> > 
> > Or do you want to finish this in this CL? Without this, I am not sure
whether
> > the change in AndroidVideoDecodeAccelerator::QueueInput() is sufficient.
> 
> Since in this CL we do not try to stop the decoding process in response to
> NO_KEY error, I assume that when the key arrives, the next enqueue attempt
will
> just succeed. In this case there is nothing to do here.

I don't fully understand how this works. What's the plan next? Will we ever stop
the decoding process for no_key? If yes, please add a TODO here describing your
plan. If not, we don't need OnKeyAdded() at all...

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

https://chromiumcodereview.appspot.com/1469353010/diff/60001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:169:
DCHECK(client_);
Put the comment in the DCHECK statement:

DCHECK(client_) << "SetCdm() must be called after Initialize().";

https://chromiumcodereview.appspot.com/1469353010/diff/60001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:195:
base::Bind(&base::DoNothing));
add include for DoNothing()?

https://chromiumcodereview.appspot.com/1469353010/diff/60001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:688: LOG(ERROR) <<
"Protected surface is not supported.";
Is this NOTIMPLEMENTED() as well?

[Tima Vaisburd, 2015-12-05 01:35:25]

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

https://chromiumcodereview.appspot.com/1469353010/diff/20001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:291: return;  //
keep trying to enqueue the front pending buffer
On 2015/12/04 23:24:09, xhwang wrote:
> On 2015/12/04 22:54:51, Tima Vaisburd wrote:
> > On 2015/12/04 20:22:03, xhwang wrote:
> > > This is an important event. Add a log here.
> > 
> > The log line above already prints the status, unless you want this NO_KEY
> > message to be printed once.
> 
> We need a log with higher level for unexpected events. How about a DVLOG(1)
> here?

Done, but it would be better to have states and have something like
if (state_ != WAITING_FOR_KEY)
  DVLOG(1) << ...;

https://chromiumcodereview.appspot.com/1469353010/diff/20001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:682: DVLOG(1) <<
__FUNCTION__;
On 2015/12/04 23:24:09, xhwang wrote:
> On 2015/12/04 22:54:51, Tima Vaisburd wrote:
> > On 2015/12/04 20:22:03, xhwang wrote:
> > > NOTIMPLEMENTED()
> > > 
> > > Also add a TODO to finish this part.
> > > 
> > > Or do you want to finish this in this CL? Without this, I am not sure
> whether
> > > the change in AndroidVideoDecodeAccelerator::QueueInput() is sufficient.
> > 
> > Since in this CL we do not try to stop the decoding process in response to
> > NO_KEY error, I assume that when the key arrives, the next enqueue attempt
> will
> > just succeed. In this case there is nothing to do here.
> 
> I don't fully understand how this works. What's the plan next? Will we ever
stop
> the decoding process for no_key? If yes, please add a TODO here describing
your
> plan. If not, we don't need OnKeyAdded() at all...

Added some comments, please take a look.

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

https://chromiumcodereview.appspot.com/1469353010/diff/60001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:169:
DCHECK(client_);
On 2015/12/04 23:24:09, xhwang wrote:
> Put the comment in the DCHECK statement:
> 
> DCHECK(client_) << "SetCdm() must be called after Initialize().";

Done.

https://chromiumcodereview.appspot.com/1469353010/diff/60001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:195:
base::Bind(&base::DoNothing));
On 2015/12/04 23:24:09, xhwang wrote:
> add include for DoNothing()?

Done.

https://chromiumcodereview.appspot.com/1469353010/diff/60001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:688: LOG(ERROR) <<
"Protected surface is not supported.";
On 2015/12/04 23:24:09, xhwang wrote:
> Is this NOTIMPLEMENTED() as well?

Yes, and actually, the codec part is easy to implement (just pass the flag),
which I did. If the surface does not satisfy the security requirements,
VideoCodecBridge::CreateDecoder() just fails.

[xhwang, 2015-12-05 06:22:26]

LGTM

Please hold off before the MediaCrypto creation refactoring CL is landed.
Thanks!

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

https://chromiumcodereview.appspot.com/1469353010/diff/80001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:698: media_crypto_
= media_crypto.Pass();
nit: std::move

[Tima Vaisburd, 2015-12-07 21:28:26]

Since I added Config::AsHumanReadableString() which required a human readable
string for the video profile I'm humbly asking to look at the CL again.

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

https://chromiumcodereview.appspot.com/1469353010/diff/80001/content/common/g...
content/common/gpu/media/android_video_decode_accelerator.cc:698: media_crypto_
= media_crypto.Pass();
On 2015/12/05 06:22:26, xhwang wrote:
> nit: std::move

Done.

[xhwang, 2015-12-07 22:14:15]

https://chromiumcodereview.appspot.com/1469353010/diff/100001/media/base/vide...
File media/base/video_decoder_config.cc (right):

https://chromiumcodereview.appspot.com/1469353010/diff/100001/media/base/vide...
media/base/video_decoder_config.cc:147: // media/base/video_codecs.h
If you move this to media/base/video_codecs.h then you don't need this comment.

https://chromiumcodereview.appspot.com/1469353010/diff/100001/media/base/vide...
media/base/video_decoder_config.cc:174: return "vp8 profile";
"profile" seems redundant.

https://chromiumcodereview.appspot.com/1469353010/diff/100001/media/base/vide...
media/base/video_decoder_config.cc:176: return "vp9 profile";
ditto

https://chromiumcodereview.appspot.com/1469353010/diff/100001/media/base/vide...
File media/base/video_decoder_config.h (right):

https://chromiumcodereview.appspot.com/1469353010/diff/100001/media/base/vide...
media/base/video_decoder_config.h:68: static std::string
GetHumanReadableProfile(VideoCodecProfile profile);
A static function seems to belong to media/base/video_codecs.h. If we do this,
we can make a static GetHumanReadableCodecName() in video_codec.h as well. So
the GetHumanReadableCodecName() at line 66 can be as simple as GetCodecName().

Also nit: GetHumanReadableProfileName() to be consistent with the codec one?

[Tima Vaisburd, 2015-12-08 02:12:24]

https://codereview.chromium.org/1469353010/diff/100001/media/base/video_decod...
File media/base/video_decoder_config.cc (right):

https://codereview.chromium.org/1469353010/diff/100001/media/base/video_decod...
media/base/video_decoder_config.cc:147: // media/base/video_codecs.h
On 2015/12/07 22:14:15, xhwang wrote:
> If you move this to media/base/video_codecs.h then you don't need this
comment.

Done.

https://codereview.chromium.org/1469353010/diff/100001/media/base/video_decod...
media/base/video_decoder_config.cc:174: return "vp8 profile";
On 2015/12/07 22:14:15, xhwang wrote:
> "profile" seems redundant.

Done.

https://codereview.chromium.org/1469353010/diff/100001/media/base/video_decod...
media/base/video_decoder_config.cc:176: return "vp9 profile";
On 2015/12/07 22:14:15, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/1469353010/diff/100001/media/base/video_decod...
File media/base/video_decoder_config.h (right):

https://codereview.chromium.org/1469353010/diff/100001/media/base/video_decod...
media/base/video_decoder_config.h:68: static std::string
GetHumanReadableProfile(VideoCodecProfile profile);
On 2015/12/07 22:14:15, xhwang wrote:
> A static function seems to belong to media/base/video_codecs.h. If we do this,
> we can make a static GetHumanReadableCodecName() in video_codec.h as well. So
> the GetHumanReadableCodecName() at line 66 can be as simple as GetCodecName().
> 
> Also nit: GetHumanReadableProfileName() to be consistent with the codec one?

Created separate functions GetCodecName() and GetProfileName(). I also put them
into new file video_codec.cc

I hope the cleanup is in the right direction, but the scope of this CL has
expanded because of this change.

[xhwang, 2015-12-08 04:58:47]

Thanks for the cleanup. LGTM++

[Tima Vaisburd, 2016-01-09 00:00:06]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2016-01-09 00:00:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org, xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1469353010/#ps160001
(title: "Rebased")

[commit-bot: I haz the power, 2016-01-09 00:03:46]

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

[Tima Vaisburd, 2016-01-09 00:34:42]

timav@chromium.org changed reviewers:
+ dalecurtis@google.com

[Tima Vaisburd, 2016-01-09 00:34:43]

Dale, Pawel, need your approval as well. Please take a look.

[commit-bot: I haz the power, 2016-01-09 00:36:14]

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

[commit-bot: I haz the power, 2016-01-09 00:36:15]

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

[DaleCurtis, 2016-01-09 00:36:30]

dalecurtis@chromium.org changed reviewers:
+ sandersd@chromium.org
- dalecurtis@google.com

[DaleCurtis, 2016-01-09 00:36:31]

=>sandersd

[sandersd (OOO until July 31), 2016-01-11 19:55:06]

RS LGTM for content/common/gpu/media.

[Tima Vaisburd, 2016-01-11 19:59:34]

The CQ bit was checked by timav@chromium.org

[commit-bot: I haz the power, 2016-01-11 20:01:11]

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

[commit-bot: I haz the power, 2016-01-11 20:12:49]

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

[commit-bot: I haz the power, 2016-01-11 20:12:49]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  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...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Tima Vaisburd, 2016-01-11 20:32:56]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2016-01-11 20:32:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org, xhwang@chromium.org, sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/1469353010/#ps180001
(title: "Rebase again")

[commit-bot: I haz the power, 2016-01-11 20:35:54]

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

[commit-bot: I haz the power, 2016-01-11 21:48:18]

Description was changed from

==========
Configure MediaCodec with CDM in ADVA

Implemented SetCdm() by retrieving the CDM object from mojo
(it should be MediaDrmBridge), registering this bridge
with nesessary callbacks and postponing the MediaCodec configuration
till we get the MediaCrypto object.

After MediaCrypto object is obtained we configure MediaCodec
and only then report the SetCdm() status by NotifyCdmAttached(bool).
With this approach any failure in MediaCodec creation is reported as
a CDM failure.

BUG=542417
==========

to

==========
Configure MediaCodec with CDM in ADVA

Implemented SetCdm() by retrieving the CDM object from mojo
(it should be MediaDrmBridge), registering this bridge
with nesessary callbacks and postponing the MediaCodec configuration
till we get the MediaCrypto object.

After MediaCrypto object is obtained we configure MediaCodec
and only then report the SetCdm() status by NotifyCdmAttached(bool).
With this approach any failure in MediaCodec creation is reported as
a CDM failure.

BUG=542417
==========

[commit-bot: I haz the power, 2016-01-11 21:48:19]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-01-11 21:49:28]

Description was changed from

==========
Configure MediaCodec with CDM in ADVA

Implemented SetCdm() by retrieving the CDM object from mojo
(it should be MediaDrmBridge), registering this bridge
with nesessary callbacks and postponing the MediaCodec configuration
till we get the MediaCrypto object.

After MediaCrypto object is obtained we configure MediaCodec
and only then report the SetCdm() status by NotifyCdmAttached(bool).
With this approach any failure in MediaCodec creation is reported as
a CDM failure.

BUG=542417
==========

to

==========
Configure MediaCodec with CDM in ADVA

Implemented SetCdm() by retrieving the CDM object from mojo
(it should be MediaDrmBridge), registering this bridge
with nesessary callbacks and postponing the MediaCodec configuration
till we get the MediaCrypto object.

After MediaCrypto object is obtained we configure MediaCodec
and only then report the SetCdm() status by NotifyCdmAttached(bool).
With this approach any failure in MediaCodec creation is reported as
a CDM failure.

BUG=542417

Committed: https://crrev.com/39cbd4546d7a6300afde7e8c1816541323a1abb5
Cr-Commit-Position: refs/heads/master@{#368679}
==========

[commit-bot: I haz the power, 2016-01-11 21:49:29]

Patchset 10 (id:??) landed as
https://crrev.com/39cbd4546d7a6300afde7e8c1816541323a1abb5
Cr-Commit-Position: refs/heads/master@{#368679}