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 214 matching lines @@
 }
 
 base::TimeDelta MediaSourcePlayer::GetDuration() {
   return duration_;
 }
 
 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();
   manager()->ReleaseMediaResources(player_id());
 }
 
 void MediaSourcePlayer::SetVolume(double volume) {
   volume_ = volume;
   SetVolumeInternal();
 }
@@ skipping 294 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()->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;
   }
 
   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 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);
 
   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