Configure MediaCodec with CDM in ADVA

content/common/gpu/media/android_video_decode_accelerator.cc

Index: content/common/gpu/media/android_video_decode_accelerator.cc
diff --git a/content/common/gpu/media/android_video_decode_accelerator.cc b/content/common/gpu/media/android_video_decode_accelerator.cc
index 00e1bb67856e14163ea5da84a6d04a37607335de..42d9a46fd65ddb63463d6818f61affc7a3398492 100644
--- a/content/common/gpu/media/android_video_decode_accelerator.cc
+++ b/content/common/gpu/media/android_video_decode_accelerator.cc
@@ -12,6 +12,7 @@
 #include "content/common/gpu/gpu_channel.h"
 #include "content/common/gpu/media/avda_return_on_failure.h"
 #include "gpu/command_buffer/service/gles2_cmd_decoder.h"
+#include "media/base/bind_to_current_loop.h"
 #include "media/base/bitstream_buffer.h"
 #include "media/base/limits.h"
 #include "media/base/timestamp_constants.h"
@@ -86,10 +87,19 @@ AndroidVideoDecodeAccelerator::AndroidVideoDecodeAccelerator(
       picturebuffers_requested_(false),
       gl_decoder_(decoder),
       strategy_(strategy.Pass()),
+      cdm_registration_id_(0),
       weak_this_factory_(this) {}
 
 AndroidVideoDecodeAccelerator::~AndroidVideoDecodeAccelerator() {
   DCHECK(thread_checker_.CalledOnValidThread());
+
+#if defined(ENABLE_MOJO_MEDIA_IN_GPU_PROCESS)
+  if (cdm_) {
+    DCHECK(cdm_registration_id_);
+    static_cast<media::MediaDrmBridge*>(cdm_.get())
+        ->UnregisterPlayer(cdm_registration_id_);
+  }
+#endif  // defined(ENABLE_MOJO_MEDIA_IN_GPU_PROCESS)
 }
 
 bool AndroidVideoDecodeAccelerator::Initialize(const Config& config,
@@ -140,25 +150,59 @@ bool AndroidVideoDecodeAccelerator::Initialize(const Config& config,
 
   surface_texture_ = strategy_->CreateSurfaceTexture();

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

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.

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

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

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

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.

 
-  if (!ConfigureMediaCodec()) {
-    LOG(ERROR) << "Failed to create MediaCodec instance.";
-    return false;
-  }
+  // For encrypted streams we postpone configuration until MediaCrypto is

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

all of this would become a call to FinishInitializationIfPossible().  it would
do nothing if |is_encrypted_| && !|media_crypto_|.

+  // available.
+  if (is_encrypted_)
+    return true;
 
-  return true;
+  return ConfigureMediaCodec();
 }
 
 void AndroidVideoDecodeAccelerator::SetCdm(int cdm_id) {

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

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.

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

maybe additional values for |state_| are useful here.

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

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

   DVLOG(2) << __FUNCTION__ << ": " << cdm_id;
 
 #if defined(ENABLE_MOJO_MEDIA_IN_GPU_PROCESS)
-  // TODO(timav): Implement CDM setting here. See http://crbug.com/542417
-  scoped_refptr<media::MediaKeys> cdm = media::MojoCdmService::GetCdm(cdm_id);
-  DCHECK(cdm);
-#endif
+  using media::MediaDrmBridge;
+
+  if (cdm_) {
+    NOTREACHED() << "We do not support resetting CDM.";
+    NotifyCdmAttached(false);
+    return;
+  }
+
+  cdm_ = media::MojoCdmService::GetCdm(cdm_id);
+  DCHECK(cdm_);
+
+  // On Android platform the MediaKeys fill be its subclass MediaDrmBridge.

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

s/fill/will

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

Done.

+  MediaDrmBridge* drm_bridge = static_cast<MediaDrmBridge*>(cdm_.get());
+
+  // Register CDM callbacks. The callbacks registered will be posted back to
+  // this thread via BindToCurrentLoop.
+
+  // Since |this| holds a reference to the |cdm_|, by the time the CDM is
+  // destructed, UnregisterPlayer() must have been called and |this| has been
+  // destructed as well. So the |cdm_unset_cb| will never have a chance to be
+  // called.
+  // TODO(xhwang): Remove |cdm_unset_cb| after it's not used on all platforms.
+  cdm_registration_id_ = drm_bridge->RegisterPlayer(
+      media::BindToCurrentLoop(
+          base::Bind(&AndroidVideoDecodeAccelerator::OnCdmKeyAdded,
+                     weak_this_factory_.GetWeakPtr())),
+      base::Bind(&base::DoNothing));
+
+  drm_bridge->SetMediaCryptoReadyCB(media::BindToCurrentLoop(
+      base::Bind(&AndroidVideoDecodeAccelerator::OnCdmMediaCryptoReady,
+                 weak_this_factory_.GetWeakPtr())));
+
+  // Postpone NotifyCdmAttached() call till we create the MediaCodec after
+  // OnCdmMediaCryptoReady().
+
+#else
 
   NOTIMPLEMENTED();
   NotifyCdmAttached(false);
+
+#endif  // !defined(ENABLE_MOJO_MEDIA_IN_GPU_PROCESS)
 }
 
 void AndroidVideoDecodeAccelerator::DoIOTask() {
@@ -195,11 +239,12 @@ void AndroidVideoDecodeAccelerator::QueueInput() {
                       base::Time::Now() - queued_time);
   media::BitstreamBuffer bitstream_buffer =
       pending_bitstream_buffers_.front().first;
-  pending_bitstream_buffers_.pop();
-  TRACE_COUNTER1("media", "AVDA::PendingBitstreamBufferCount",
-                 pending_bitstream_buffers_.size());
 
   if (bitstream_buffer.id() == -1) {
+    pending_bitstream_buffers_.pop();
+    TRACE_COUNTER1("media", "AVDA::PendingBitstreamBufferCount",
+                   pending_bitstream_buffers_.size());
+
     media_codec_->QueueEOS(input_buf_index);
     return;
   }
@@ -242,6 +287,13 @@ void AndroidVideoDecodeAccelerator::QueueInput() {
            << ": QueueInputBuffer: pts:" << presentation_timestamp
            << " status:" << status;
 
+  if (status == media::MEDIA_CODEC_NO_KEY)
+    return;  // keep trying to enqueue the front pending buffer

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

This is an important event. Add a log here.

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

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

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

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

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

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

+
+  pending_bitstream_buffers_.pop();
+  TRACE_COUNTER1("media", "AVDA::PendingBitstreamBufferCount",
+                 pending_bitstream_buffers_.size());
+
   RETURN_ON_FAILURE(this, status == media::MEDIA_CODEC_OK,
                     "Failed to QueueInputBuffer: " << status, PLATFORM_FAILURE);
 
@@ -505,13 +557,18 @@ bool AndroidVideoDecodeAccelerator::ConfigureMediaCodec() {
 
   gfx::ScopedJavaSurface surface(surface_texture_.get());
 
+  jobject media_crypto = media_crypto_ ? media_crypto_->obj() : nullptr;
+
   // Pass a dummy 320x240 canvas size and let the codec signal the real size
   // when it's known from the bitstream.
   media_codec_.reset(media::VideoCodecBridge::CreateDecoder(
-      codec_, false, gfx::Size(320, 240), surface.j_surface().obj(), NULL));
+      codec_, false, gfx::Size(320, 240), surface.j_surface().obj(),
+      media_crypto));
   strategy_->CodecChanged(media_codec_.get(), output_picture_buffers_);
-  if (!media_codec_)
+  if (!media_codec_) {
+    LOG(ERROR) << "Failed to create MediaCodec instance.";
     return false;
+  }
 
   io_timer_.Start(FROM_HERE,
                   DecodePollDelay(),
@@ -606,6 +663,25 @@ void AndroidVideoDecodeAccelerator::PostError(
   state_ = ERROR;
 }
 
+void AndroidVideoDecodeAccelerator::OnCdmMediaCryptoReady(
+    media::MediaDrmBridge::JavaObjectPtr media_crypto,
+    bool needs_protected_surface) {
+  DVLOG(1) << __FUNCTION__;
+
+  DCHECK(media_crypto);

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

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.

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

Done.

+  DCHECK(!media_crypto->is_null());

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

DCHECK(!media_codec)

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

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

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

it depends if we've sent in any buffers.

+
+  media_crypto_ = media_crypto.Pass();
+
+  // Configure here instead of in Initialize()
+  const bool success = ConfigureMediaCodec();
+  NotifyCdmAttached(success);

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

outside the scope of this CL, but NotifyInitializationFailed might be worth
splitting out someday.

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

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

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

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.

+}
+
+void AndroidVideoDecodeAccelerator::OnCdmKeyAdded() {
+  DVLOG(1) << __FUNCTION__;

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

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.

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

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.

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

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...

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

Added some comments, please take a look.

+}
+
 void AndroidVideoDecodeAccelerator::NotifyCdmAttached(bool success) {
   client_->NotifyCdmAttached(success);
 }