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..6827bf0c7c7b0a84c1c31e0b61dcd00201449aaf 100644
--- a/media/base/android/media_source_player.cc
+++ b/media/base/android/media_source_player.cc
@@ -230,15 +230,46 @@ base::TimeDelta MediaSourcePlayer::GetDuration() {
 
 void MediaSourcePlayer::Release() {
   DVLOG(1) << __FUNCTION__;
+
+  // Allow SEEK_EVENT_PENDING and CONFIG_CHANGE_EVENT_PENDING to survive the
+  // Release(). Clear all others. If PREFETCH_DONE_EVENT_PENDING was previously
+  // pending, or a job was still decoding, then at end of Release() we need to
+  // ProcessPendingEvents() since seek or config change would not otherwise be
+  // processed.
+  // 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.
+  bool process_pending_events = false;
+  if (IsEventPending(SEEK_EVENT_PENDING) ||

[qinmin, 2013/10/30 23:07:48]

No need for this if statement, processPendingEvents will nothing if there are no
pending seek or config change events.

[wolenetz, 2013/10/31 01:39:11]

Excellent point, thank you. Done.

+       IsEventPending(CONFIG_CHANGE_EVENT_PENDING)) {
+    process_pending_events = IsEventPending(PREFETCH_DONE_EVENT_PENDING) ||
+        (audio_decoder_job_ && audio_decoder_job_->is_decoding()) ||
+        (video_decoder_job_ && video_decoder_job_->is_decoding());
+  }
+  if (IsEventPending(SURFACE_CHANGE_EVENT_PENDING))

[acolwell GONE FROM CHROMIUM, 2013/10/30 23:29:53]

nit: How about the following instead:

// Clear all pending events except seeks and config changes.
pending_event_ &= (SEEK_EVENT_PENDING | CONFIG_CHANGE_EVENT_PENDING);

[wolenetz, 2013/10/31 01:39:11]

Done. I was trying to keep the raw pending_event_ modifications to Set/Clear/Is*
methods, in case the underlying implementation changed from a bitmap. This is
simpler though.

+    ClearPendingEvent(SURFACE_CHANGE_EVENT_PENDING);
+  if (IsEventPending(PREFETCH_REQUEST_EVENT_PENDING))
+    ClearPendingEvent(PREFETCH_REQUEST_EVENT_PENDING);
+  if (IsEventPending(PREFETCH_DONE_EVENT_PENDING))
+    ClearPendingEvent(PREFETCH_DONE_EVENT_PENDING);
+
   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;
-  pending_event_ = NO_EVENT_PENDING;
+
   decoder_starvation_callback_.Cancel();
   surface_ = gfx::ScopedJavaSurface();
   manager()->ReleaseMediaResources(player_id());
+  if (process_pending_events) {
+    DVLOG(1) << __FUNCTION__ << " : Resuming seek or config change processing";
+    ProcessPendingEvents();
+  }
 }
 
 void MediaSourcePlayer::SetVolume(double volume) {
@@ -549,12 +580,16 @@ void MediaSourcePlayer::MediaDecoderCallback(
                            base::IntToString(status));
   }
 
+  // Let tests hook the completion of a decode cycle.
+  manager()->OnMediaDecoderCallbackForTesting();
+
   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 +870,17 @@ 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.
+  // 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_);
+    return;
+  }
 
   ClearPendingEvent(PREFETCH_DONE_EVENT_PENDING);