Clear any pending surface change prior to checking media crypto in MSP::ConfigureVideoDecoderJob()

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"
@@ skipping 213 matching lines @@
   return clock_.Elapsed();
 }
 
 base::TimeDelta MediaSourcePlayer::GetDuration() {
   return duration_;
 }
 
 void MediaSourcePlayer::Release() {
   DVLOG(1) << __FUNCTION__;
   audio_decoder_job_.reset();
-  video_decoder_job_.reset();
+  ResetVideoDecoderJob();
   reconfig_audio_decoder_ = false;
   reconfig_video_decoder_ = false;
   playing_ = false;
   pending_event_ = NO_EVENT_PENDING;
   decoder_starvation_callback_.Cancel();
   surface_ = gfx::ScopedJavaSurface();
   manager()->ReleaseMediaResources(player_id());
 }
 
 void MediaSourcePlayer::SetVolume(double volume) {
@@ skipping 241 matching lines @@
   if (IsEventPending(CONFIG_CHANGE_EVENT_PENDING)) {
     DVLOG(1) << __FUNCTION__ << " : Handling CONFIG_CHANGE_EVENT.";
     DCHECK(reconfig_audio_decoder_ || reconfig_video_decoder_);
     demuxer_->RequestDemuxerConfigs();
     return;
   }
 
   if (IsEventPending(SURFACE_CHANGE_EVENT_PENDING)) {
     DVLOG(1) << __FUNCTION__ << " : Handling SURFACE_CHANGE_EVENT.";
     // Setting a new surface will require a new MediaCodec to be created.
-    video_decoder_job_.reset();
+    ResetVideoDecoderJob();
     ConfigureVideoDecoderJob();
 
     // Return early if we can't successfully configure a new video decoder job
     // yet, except continue processing other pending events if |surface_| is
     // empty.
     if (!video_decoder_job_ && !surface_.IsEmpty())
       return;
   }
 
   if (IsEventPending(PREFETCH_REQUEST_EVENT_PENDING)) {
@@ skipping 204 matching lines @@
       base::Bind(&DemuxerAndroid::RequestDemuxerData,
                  base::Unretained(demuxer_.get()), DemuxerStream::AUDIO)));
 
   if (audio_decoder_job_) {
     SetVolumeInternal();
     audio_decoder_job_->BeginPrerolling(preroll_timestamp_);
     reconfig_audio_decoder_ =  false;
   }
 }
 
+void MediaSourcePlayer::ResetVideoDecoderJob() {
+  video_decoder_job_.reset();
+
+  // Any eventual video decoder job re-creation will use the current |surface_|.
+  if (IsEventPending(SURFACE_CHANGE_EVENT_PENDING))
+    ClearPendingEvent(SURFACE_CHANGE_EVENT_PENDING);
+}
+
 void MediaSourcePlayer::ConfigureVideoDecoderJob() {
   if (!HasVideo() || surface_.IsEmpty()) {
-    video_decoder_job_.reset();
-    if (IsEventPending(SURFACE_CHANGE_EVENT_PENDING))
-      ClearPendingEvent(SURFACE_CHANGE_EVENT_PENDING);
+    ResetVideoDecoderJob();
     return;
   }
 
   // Create video decoder job only if config changes or we don't have a job.
-  if (video_decoder_job_ && !reconfig_video_decoder_)
+  if (video_decoder_job_ && !reconfig_video_decoder_) {
+    DCHECK(!IsEventPending(SURFACE_CHANGE_EVENT_PENDING));
     return;
+  }
 
   if (reconfig_video_decoder_) {
     // No hack browser seek should be required. I-Frame must be next.
     DCHECK(next_video_data_is_iframe_) << "Received video data between "
         << "detecting video config change and reconfiguring video decoder";
   }
 
+  DCHECK(!video_decoder_job_ || !video_decoder_job_->is_decoding());

[xhwang, 2013/10/31 21:37:18]

Do we ever call ConfigureVideoDecoderJob() when
video_decoder_job_->is_decoding()? If not, put this check at the begining of
this function? It's a bit confusing to DCHECK this after so many "if" blocks. Or
am I missing anything?

[wolenetz, 2013/11/02 00:00:57]

Good point. Thanks for chat that helped clarify EME scenarios, and (current)
lack of their MSP::[EME config method]()->StartInternal()->CVDJ() possibly
racing a current video decoder job decode of previously received data. Details:
1. OnKeyAdded()->CVDJ() cannot race an existing decode, even though it can
arrive any time. |is_waiting_for_key_| is reset in StartInternal() and protects
against calling StartInternal() from OnKeyAdded().
2. OnMediaCryptoReady() must occur prior to initial encrypted video job's
successful creation. SBS::UpdateVideoConfig() prevents subsequent change of
video's is_encrypted. "For now, we only call OnMediaCryptoReady() once."
3. SetDrmBridge(): "For now, we only call SetDrmBridge() once."

Given the EME spec might change or other refactoring could introduce other race
vectors, keeping the DCHECK after our existing protection against reconfiguring
an existing job that we're not expecting to reconfigure
(|reconfig_video_decoder_|) seems the safest approach for now. Hence, I'll move
the DCHECK no earlier than after the second if() clause in this CL.

+
   // If uncertain that video I-frame data is next and there is no seek already
   // in process, request browser demuxer seek so the new decoder will decode
   // an I-frame first. Otherwise, the new MediaCodec might crash. See b/8950387.
+  // Eventual OnDemuxerSeekDone() will trigger ProcessPendingEvents() and
+  // continue from here.
   // TODO(wolenetz): Instead of doing hack browser seek, replay cached data
   // since last keyframe. See http://crbug.com/304234.
   if (!next_video_data_is_iframe_ && !IsEventPending(SEEK_EVENT_PENDING)) {
     BrowserSeekToCurrentTime();
     return;
   }
 
+  // Release the old VideoDecoderJob first so the surface can get released.
+  // Android does not allow 2 MediaCodec instances use the same surface.
+  ResetVideoDecoderJob();
+
   base::android::ScopedJavaLocalRef<jobject> media_crypto = GetMediaCrypto();
   if (is_video_encrypted_ && media_crypto.is_null())
     return;
 
-  DCHECK(!video_decoder_job_ || !video_decoder_job_->is_decoding());
-
   DVLOG(1) << __FUNCTION__ << " : creating new video decoder job";
 
-  // Release the old VideoDecoderJob first so the surface can get released.
-  // Android does not allow 2 MediaCodec instances use the same surface.
-  video_decoder_job_.reset();
   // Create the new VideoDecoderJob.
   bool is_secure = IsProtectedSurfaceRequired();
   video_decoder_job_.reset(
       VideoDecoderJob::Create(video_codec_,
                               is_secure,
                               gfx::Size(width_, height_),
                               surface_.j_surface().obj(),
                               media_crypto.obj(),
                               base::Bind(&DemuxerAndroid::RequestDemuxerData,
                                          base::Unretained(demuxer_.get()),
                                          DemuxerStream::VIDEO)));
   if (video_decoder_job_) {

[acolwell GONE FROM CHROMIUM, 2013/10/31 21:45:08]

nit: reverse condition and return early.

[wolenetz, 2013/11/02 00:00:57]

Done. This is a pattern I'll start watching for more proactively :)
Thanks.

     video_decoder_job_->BeginPrerolling(preroll_timestamp_);
     reconfig_video_decoder_ = false;
+
+    // Inform the fullscreen view the player is ready.
+    // TODO(qinmin): refactor MediaPlayerBridge so that we have a better way
+    // to inform ContentVideoView.
+    manager()->OnMediaMetadataChanged(
+        player_id(), duration_, width_, height_, true);
   }
-
-  if (IsEventPending(SURFACE_CHANGE_EVENT_PENDING))
-    ClearPendingEvent(SURFACE_CHANGE_EVENT_PENDING);
-
-  // Inform the fullscreen view the player is ready.
-  // TODO(qinmin): refactor MediaPlayerBridge so that we have a better way
-  // to inform ContentVideoView.
-  manager()->OnMediaMetadataChanged(
-      player_id(), duration_, width_, height_, true);
 }
 
 void MediaSourcePlayer::OnDecoderStarved() {
   DVLOG(1) << __FUNCTION__;
   SetPendingEvent(PREFETCH_REQUEST_EVENT_PENDING);
   ProcessPendingEvents();
 }
 
 void MediaSourcePlayer::StartStarvationCallback(
     const base::TimeDelta& presentation_timestamp) {
@@ skipping 93 matching lines @@
 
 void MediaSourcePlayer::ClearPendingEvent(PendingEventFlags event) {
   DVLOG(1) << __FUNCTION__ << "(" << GetEventName(event) << ")";
   DCHECK_NE(event, NO_EVENT_PENDING);
   DCHECK(IsEventPending(event)) << GetEventName(event);
 
   pending_event_ &= ~event;
 }
 
 }  // namespace media