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

media/base/android/media_source_player.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/base/android/media_source_player.h"
 
 #include <limits>
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #include "base/barrier_closure.h"
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/debug/trace_event.h"
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
 #include "media/base/android/audio_decoder_job.h"
 #include "media/base/android/media_drm_bridge.h"
 #include "media/base/android/media_player_manager.h"
 #include "media/base/android/video_decoder_job.h"
 
-
 namespace media {
 
 MediaSourcePlayer::MediaSourcePlayer(
     int player_id,
     MediaPlayerManager* manager,
     const RequestMediaResourcesCB& request_media_resources_cb,
     scoped_ptr<DemuxerAndroid> demuxer,
     const GURL& frame_url)
     : MediaPlayerAndroid(player_id,
                          manager,
                          request_media_resources_cb,
                          frame_url),
       demuxer_(demuxer.Pass()),
       pending_event_(NO_EVENT_PENDING),
       playing_(false),
       interpolator_(&default_tick_clock_),
       doing_browser_seek_(false),
       pending_seek_(false),
       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),
       weak_factory_(this) {
   audio_decoder_job_.reset(new AudioDecoderJob(
       base::Bind(&DemuxerAndroid::RequestDemuxerData,
                  base::Unretained(demuxer_.get()),
                  DemuxerStream::AUDIO),
       base::Bind(&MediaSourcePlayer::OnDemuxerConfigsChanged,
                  weak_factory_.GetWeakPtr())));
@@ skipping 161 matching lines @@
 bool MediaSourcePlayer::IsPlayerReady() {
   return audio_decoder_job_ || video_decoder_job_;
 }
 
 void MediaSourcePlayer::StartInternal() {
   DVLOG(1) << __FUNCTION__;
   // If there are pending events, wait for them finish.
   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);
   ProcessPendingEvents();
 }
 
 void MediaSourcePlayer::OnDemuxerConfigsAvailable(
     const DemuxerConfigs& configs) {
   DVLOG(1) << __FUNCTION__;
   DCHECK(!HasAudio() && !HasVideo());
@@ skipping 255 matching lines @@
   if (status == MEDIA_CODEC_OUTPUT_END_OF_STREAM)
     return;
 
   if (!playing_) {
     if (is_clock_manager)
       interpolator_.StopInterpolating();
     return;
   }
 
   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.
   if (status == MEDIA_CODEC_STOPPED)
     return;
 
   if (prerolling_ && IsPrerollFinished(is_audio)) {
     if (IsPrerollFinished(!is_audio)) {
       prerolling_ = false;
       StartInternal();
@@ skipping 229 matching lines @@
   if (audio)
     is_waiting_for_audio_decoder_ = false;
   if (video)
     is_waiting_for_video_decoder_ = false;
   if (IsEventPending(DECODER_CREATION_EVENT_PENDING))
     ProcessPendingEvents();
 }
 
 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();
 }
 
 void MediaSourcePlayer::OnCdmUnset() {
   DVLOG(1) << __FUNCTION__;
   DCHECK(drm_bridge_);
   // TODO(xhwang): Currently this is only called during teardown. Support full
   // detachment of CDM during playback. This will be needed when we start to
   // support setMediaKeys(0) (see http://crbug.com/330324), or when we release
   // MediaDrm when the video is paused, or when the device goes to sleep (see
   // http://crbug.com/272421).
   audio_decoder_job_->SetDrmBridge(NULL);
   video_decoder_job_->SetDrmBridge(NULL);
   cdm_registration_id_ = 0;
   drm_bridge_ = NULL;
 }
 
 }  // namespace media