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

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 212 matching lines @@
 base::TimeDelta MediaSourcePlayer::GetCurrentTime() {
   return clock_.Elapsed();
 }
 
 base::TimeDelta MediaSourcePlayer::GetDuration() {
   return duration_;
 }
 
 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) {
   volume_ = volume;
   SetVolumeInternal();
 }
 
 void MediaSourcePlayer::OnKeyAdded() {
   DVLOG(1) << __FUNCTION__;
   if (!is_waiting_for_key_)
@@ skipping 290 matching lines @@
                            "MediaCodecStatus",
                            base::IntToString(status));
   } else {
     TRACE_EVENT_ASYNC_END1("media",
                            "MediaSourcePlayer::DecodeMoreVideo",
                            video_decoder_job_.get(),
                            "MediaCodecStatus",
                            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;
   }
 
   if (pending_event_ != NO_EVENT_PENDING) {
     ProcessPendingEvents();
     return;
   }
 
@@ skipping 260 matching lines @@
 
 bool MediaSourcePlayer::IsProtectedSurfaceRequired() {
   return is_video_encrypted_ &&
       drm_bridge_ && drm_bridge_->IsProtectedSurfaceRequired();
 }
 
 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);
 
   if (pending_event_ != NO_EVENT_PENDING) {
     ProcessPendingEvents();
     return;
   }
 
   start_time_ticks_ = base::TimeTicks::Now();
   start_presentation_timestamp_ = GetCurrentTime();
@@ skipping 38 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