Abort MSP::OnPrefetchDone() if just after MSP::Release(). Let seek and config change survive.

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..8be8691a62829ae354587c5e258ce69f221ab527 100644
--- a/media/base/android/media_source_player.cc
+++ b/media/base/android/media_source_player.cc
@@ -232,9 +232,17 @@ void MediaSourcePlayer::Release() {
   DVLOG(1) << __FUNCTION__;
   audio_decoder_job_.reset();
   video_decoder_job_.reset();
+
+  // Prevent job re-creation attempts in OnDemuxerConfigsAvailable()
   reconfig_audio_decoder_ = false;
   reconfig_video_decoder_ = false;
+
+  // Prevent player restart, including job re-creation attempts.
   playing_ = false;
+
+  // TODO(qinmin/wolenetz): Maintain channel state to not double-request data
+  // or drop data received across Release()+Start(). See http://crbug.com/306314
+  // and http://crbug.com/304234.
   pending_event_ = NO_EVENT_PENDING;
   decoder_starvation_callback_.Cancel();
   surface_ = gfx::ScopedJavaSurface();
@@ -549,12 +557,16 @@ void MediaSourcePlayer::MediaDecoderCallback(
                            base::IntToString(status));
   }
 
+  // Let tests hook the completion of a decode cycle.
+  manager()->OnMediaDecoderCallback();
+
   bool is_clock_manager = is_audio || !HasAudio();
 
   if (is_clock_manager)
     decoder_starvation_callback_.Cancel();
 
   if (status == MEDIA_CODEC_ERROR) {
+    DVLOG(1) << __FUNCTION__ << " : decode error";
     Release();
     manager()->OnError(player_id(), MEDIA_ERROR_DECODE);
     return;
@@ -835,7 +847,20 @@ void MediaSourcePlayer::OnPrefetchDone() {
   DVLOG(1) << __FUNCTION__;
   DCHECK(!audio_decoder_job_ || !audio_decoder_job_->is_decoding());
   DCHECK(!video_decoder_job_ || !video_decoder_job_->is_decoding());
-  DCHECK(IsEventPending(PREFETCH_DONE_EVENT_PENDING));
+
+  // A previously posted OnPrefetchDone() could race against a Release(). If
+  // Release() won the race, we should no longer have decoder jobs and should
+  // just process any other pending events.
+  // TODO(qinmin/wolenetz): Maintain channel state to not double-request data
+  // or drop data received across Release()+Start(). See http://crbug.com/306314
+  // and http://crbug.com/304234.
+  if (!IsEventPending(PREFETCH_DONE_EVENT_PENDING)) {
+    DVLOG(1) << __FUNCTION__ << " : aborting";
+    DCHECK(!audio_decoder_job_ && !video_decoder_job_);
+    if (pending_event_ != NO_EVENT_PENDING)
+      ProcessPendingEvents();

[qinmin, 2013/10/29 23:49:34]

Do we really need to process the pending events? if we have pending events,
these pending events should arrive after release(). So they are already being
handled by corresponding functions.

[wolenetz, 2013/10/30 00:06:45]

Suppose we had prefetch done pending when those other events arrived after
release(). In that case, ProcessPendingEvents() would not have previously
processed them due to pending prefetch done event. In reality, we probably don't
do the right thing for those events (regardless of ProcessPendingEvents() here)
until the overall bug 306314 is fixed.

[qinmin, 2013/10/30 00:14:40]

But release() will clear the pending_event_, so there is no PREFETCH_DONE event
pending. Right? 

And if a prefetch done task is posted at that state, while a seek is performed
right before the task gets executed, the seek will be executed immediately


On 2013/10/30 00:06:45, wolenetz wrote:

+    return;
+  }
 
   ClearPendingEvent(PREFETCH_DONE_EVENT_PENDING);