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 709 matching lines @@
 
 void MediaSourcePlayer::ConfigureVideoDecoderJob() {
   if (!HasVideo() || surface_.IsEmpty()) {
     video_decoder_job_.reset();
     if (IsEventPending(SURFACE_CHANGE_EVENT_PENDING))
       ClearPendingEvent(SURFACE_CHANGE_EVENT_PENDING);
     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());
+
+  // 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.
   // 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;
   }
 
   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_) {
     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(

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

Why do we blindly notify the manager even when the video_decoder_job_ isn't
created?

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

If the decoder job is created, this is to let the fullscreen video view know
that Media metadata is available. 
When swiching to fullscreen, we show a circling animation to indicate that the
player is not ready. When metadata is available, we call this so that
contentVideoView know when to start showing the video.

However, we don't need to do this if video_decoder_job_ is not created. I think
originally VideoDecoderJob::Create() always return a non-null pointer. But since
it can return a null pointer now, so we need to move this statement into the
above if statement to avoid removing the animation


On 2013/10/31 18:11:28, acolwell wrote:

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

Done.

       player_id(), duration_, width_, height_, true);
 }
 
 void MediaSourcePlayer::OnDecoderStarved() {
   DVLOG(1) << __FUNCTION__;
   SetPendingEvent(PREFETCH_REQUEST_EVENT_PENDING);
   ProcessPendingEvents();
 }
 
 void MediaSourcePlayer::StartStarvationCallback(
@@ skipping 94 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