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

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 8e23a91629331fc6f8df18098a23ee69d37915d2..50b49f9d101e750f94cf14223b797dd77753bfd0 100644
--- a/media/base/android/media_source_player.cc
+++ b/media/base/android/media_source_player.cc
@@ -231,7 +231,7 @@ base::TimeDelta MediaSourcePlayer::GetDuration() {
 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;
@@ -493,7 +493,7 @@ void MediaSourcePlayer::ProcessPendingEvents() {
   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
@@ -718,17 +718,25 @@ void MediaSourcePlayer::ConfigureAudioDecoderJob() {
   }
 }
 
+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.
@@ -736,9 +744,13 @@ void MediaSourcePlayer::ConfigureVideoDecoderJob() {
         << "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)) {
@@ -746,17 +758,16 @@ void MediaSourcePlayer::ConfigureVideoDecoderJob() {
     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(
@@ -771,16 +782,13 @@ void MediaSourcePlayer::ConfigureVideoDecoderJob() {
   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;
-  }
 
-  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);
+    // 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() {