MediaSourcePlayer: Handle the case where key is added during decoding.

media/base/android/media_source_player.cc

Index: media/base/android/media_source_player.cc
diff --git a/media/base/android/media_source_player.cc b/media/base/android/media_source_player.cc
index 8306ad969b1a5eed27c7ba584c2db7322ad98260..a35dddf594c6c4f356dfe29923afe26f32c8f5e0 100644
--- a/media/base/android/media_source_player.cc
+++ b/media/base/android/media_source_player.cc
@@ -20,7 +20,6 @@
 #include "media/base/android/media_player_manager.h"
 #include "media/base/android/video_decoder_job.h"
 
-
 namespace media {
 
 MediaSourcePlayer::MediaSourcePlayer(
@@ -42,6 +41,7 @@ MediaSourcePlayer::MediaSourcePlayer(
       drm_bridge_(NULL),
       cdm_registration_id_(0),
       is_waiting_for_key_(false),
+      key_added_while_decode_pending_(false),
       is_waiting_for_audio_decoder_(false),
       is_waiting_for_video_decoder_(false),
       prerolling_(true),
@@ -223,10 +223,11 @@ void MediaSourcePlayer::StartInternal() {
   if (pending_event_ != NO_EVENT_PENDING)
     return;
 
-  // When we start, we'll have new demuxed data coming in. This new data could
-  // be clear (not encrypted) or encrypted with different keys. So
-  // |is_waiting_for_key_| condition may not be true anymore.
+  // When we start, we could have new demuxed data coming in. This new data
+  // could be clear (not encrypted) or encrypted with different keys. So key
+  // related info should all be cleared.
   is_waiting_for_key_ = false;
+  key_added_while_decode_pending_ = false;
   AttachListener(NULL);
 
   SetPendingEvent(PREFETCH_REQUEST_EVENT_PENDING);
@@ -502,10 +503,25 @@ void MediaSourcePlayer::MediaDecoderCallback(
   }
 
   if (status == MEDIA_CODEC_NO_KEY) {
-    is_waiting_for_key_ = true;
+    if (key_added_while_decode_pending_) {
+      DVLOG(2) << __FUNCTION__ << ": Key was added during decoding.";
+      ResumePlaybackAfterKeyAdded();
+    } else {
+      is_waiting_for_key_ = true;
+    }
     return;
   }
 
+  // If |key_added_while_decode_pending_| is true and both audio and video
+  // decoding succeeded, we should clear |key_added_while_decode_pending_| here.
+  // But that would add more complexity into this function. If we don't clear it
+  // here, the worst case would be we call ResumePlaybackAfterKeyAdded() when
+  // we don't really have a new key. This should rarely happen and the
+  // performance impact should be pretty small.

[xhwang, 2014/10/24 23:24:11]

qinmin: let me know what you think of this.

[qinmin, 2014/10/24 23:45:56]

I saw an issue that audio can clear out video's key_added_while_decode_pending_
and the same thing could happen vice versa, not sure how to handle this
properly, we need to know whether the key is added for audio or video 

[xhwang, 2014/10/25 00:11:36]

Thanks for the review. Here's what I thought that could happen:

1) both audio and video are encrypted and are decoding
2) key added so we set key_added_while_decode_pending_ = true
3) audio decoder returned NO_KEY
4) We call ResumePlaybackAfterKeyAdded().
5) We set PREFETCH_REQUEST_EVENT_PENDING event and try to
ProcessPendingEvents().
6) Since video_decoder_job_->is_decoding(), we won't start prefetching right
away.
7) video decoder returned NO_KEY
8) Before we handle NO_KEY, since (pending_event_ != NO_EVENT_PENDING), we go
ahead to ProcessPendingEvents(), which starts to prefetch....
9) We'll try to decode audio and video again using existing data.

It's a fairly complicated process. I really hope we could simplify them.

+  // TODO(qinmin/xhwang): This class is complicated because we handle both audio
+  // and video in one file. If we separate them, we should be able to remove a
+  // lot of duplication.
+
   // If the status is MEDIA_CODEC_STOPPED, stop decoding new data. The player is
   // in the middle of a seek or stop event and needs to wait for the IPCs to
   // come.
@@ -755,10 +771,27 @@ void MediaSourcePlayer::RetryDecoderCreation(bool audio, bool video) {
 
 void MediaSourcePlayer::OnKeyAdded() {
   DVLOG(1) << __FUNCTION__;
-  if (!is_waiting_for_key_)
+
+  if (is_waiting_for_key_) {
+    ResumePlaybackAfterKeyAdded();
     return;
+  }
+
+  if ((audio_decoder_job_->is_content_encrypted() &&
+       audio_decoder_job_->is_decoding()) ||
+      (video_decoder_job_->is_content_encrypted() &&
+       video_decoder_job_->is_decoding())) {
+    DVLOG(2) << __FUNCTION__ << ": " << "Key added during pending decode.";
+    key_added_while_decode_pending_ = true;
+  }
+}
 
+void MediaSourcePlayer::ResumePlaybackAfterKeyAdded() {
+  DVLOG(2) << __FUNCTION__;

[gunsch, 2014/10/24 23:51:03]

nit: rest of file is DVLOG(1)

[xhwang, 2014/10/25 00:11:36]

Done.

   is_waiting_for_key_ = false;
+  key_added_while_decode_pending_ = false;
+  // StartInternal() will trigger a prefetch. But since we already HasData(),

[gunsch, 2014/10/24 23:51:03]

is it worth asserting HasData() on both jobs here?

[xhwang, 2014/10/25 00:11:36]

I don't have confidence in a DCHECK here given all the complexity in this
class... I modified the comment a bit to reflect what I observed...

+  // we'll just use previously received data.
   if (playing_)
     StartInternal();
 }