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..64cd6939dd8ce2490a25aa23bb55973c15094935 100644
--- a/media/base/android/media_source_player.cc
+++ b/media/base/android/media_source_player.cc
@@ -727,8 +727,10 @@ void MediaSourcePlayer::ConfigureVideoDecoderJob() {
   }
 
   // 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,6 +738,15 @@ void MediaSourcePlayer::ConfigureVideoDecoderJob() {
         << "detecting video config change and reconfiguring video decoder";
   }
 
+  DCHECK(!video_decoder_job_ || !video_decoder_job_->is_decoding());
+
+  // 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();

[wolenetz, 2013/10/31 03:03:09]

Note, if is_video_encrypted but there is no valid media_crypto yet, the job is
reset earlier with this change. Is this problematic?

[qinmin, 2013/10/31 16:35:52]

Should be ok. If there is no media_crypto, the job cannot be created.

On 2013/10/31 03:03:09, wolenetz wrote:

[acolwell GONE FROM CHROMIUM, 2013/10/31 18:11:28]

ISTM that we should create a helper function for resetting the
video_decoder_job_ so that we always clear the surface change event whenever the
job is destroyed. I think this would remove duplicate logic along these lines
sprinkled around this file.

[wolenetz, 2013/10/31 18:12:07]

Could there be a regression here, too: Could we previously have media_crypto and
a video_decoder_job_ (or is_video_encrypted_ was false and we had a job, but
later is_video_encrypted_ became true), such that previous behavior would be to
not reset the job until we get non-null media_crypto? Or is it always ok to
reset the job this early?

[qinmin, 2013/10/31 18:32:37]

we always need to reset the vide_decoder_job if media_crypto changes. It doesn't
matter where we reset it.

On 2013/10/31 18:12:07, wolenetz wrote:

[wolenetz, 2013/10/31 20:34:24]

Done, added helper MSP::ResetVideoDecoderJob().

+
+  if (IsEventPending(SURFACE_CHANGE_EVENT_PENDING))

[qinmin, 2013/10/31 16:35:52]

i think all this should happen after the next if statement . Seek should take
priority above surface change. After seek is completed, we will call
processPendingEvents and enter here.

[wolenetz, 2013/10/31 18:12:07]

Done. In combination with eventual landing of
https://codereview.chromium.org/51613002/, Release() will preserve the pending
seek (including browser seek) status, too, and still clear any pending surface
change event.

+    ClearPendingEvent(SURFACE_CHANGE_EVENT_PENDING);
+
   // 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.
@@ -750,13 +761,8 @@ void MediaSourcePlayer::ConfigureVideoDecoderJob() {
   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(
@@ -773,9 +779,6 @@ void MediaSourcePlayer::ConfigureVideoDecoderJob() {
     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.