Let only seeks reset Android MSE stream playback completion

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 4c6cb9c1215602e934600f547b83d2470eb53928..e1268a533e105ff240225ad12f9c153ea8b20947 100644
--- a/media/base/android/media_source_player.cc
+++ b/media/base/android/media_source_player.cc
@@ -73,8 +73,8 @@ MediaSourcePlayer::MediaSourcePlayer(
       video_codec_(kUnknownVideoCodec),
       num_channels_(0),
       sampling_rate_(0),
-      audio_finished_(true),
-      video_finished_(true),
+      audio_finished_(false),
+      video_finished_(false),
       playing_(false),
       is_audio_encrypted_(false),
       is_video_encrypted_(false),
@@ -318,8 +318,6 @@ void MediaSourcePlayer::StartInternal() {
     return;
   }
 
-  audio_finished_ = false;
-  video_finished_ = false;
   SetPendingEvent(PREFETCH_REQUEST_EVENT_PENDING);
   ProcessPendingEvents();
 }
@@ -451,6 +449,9 @@ void MediaSourcePlayer::OnDemuxerSeekDone(
       audio_timestamp_helper_->SetBaseTimestamp(actual_browser_seek_time);
   }
 
+  audio_finished_ = false;
+  video_finished_ = false;
+
   base::TimeDelta current_time = GetCurrentTime();
   // TODO(qinmin): Simplify the logic by using |start_presentation_timestamp_|
   // to preroll media decoder jobs. Currently |start_presentation_timestamp_|
@@ -530,19 +531,29 @@ void MediaSourcePlayer::ProcessPendingEvents() {
 
   if (IsEventPending(PREFETCH_REQUEST_EVENT_PENDING)) {
     DVLOG(1) << __FUNCTION__ << " : Handling PREFETCH_REQUEST_EVENT.";
-    int count = (audio_decoder_job_ ? 1 : 0) + (video_decoder_job_ ? 1 : 0);
+    DCHECK(audio_decoder_job_ || AudioFinishedOrNoAudio());
+    DCHECK(video_decoder_job_ || VideoFinishedOrNoVideo());
+
+    int count = ((AudioFinishedOrNoAudio()) ? 0 : 1) +
+        ((VideoFinishedOrNoVideo()) ? 0 : 1);
+
+    // It is possible that all streams have finished decode, yet starvation
+    // occurred during the last stream's EOS decode. In this case, prefetch is a
+    // no-op.
+    ClearPendingEvent(PREFETCH_REQUEST_EVENT_PENDING);
+    if (count == 0)
+      return;
 
+    SetPendingEvent(PREFETCH_DONE_EVENT_PENDING);
     base::Closure barrier = BarrierClosure(count, base::Bind(
         &MediaSourcePlayer::OnPrefetchDone, weak_this_.GetWeakPtr()));
 
-    if (audio_decoder_job_)
+    if (!AudioFinishedOrNoAudio())
       audio_decoder_job_->Prefetch(barrier);
 
-    if (video_decoder_job_)
+    if (!VideoFinishedOrNoVideo())
       video_decoder_job_->Prefetch(barrier);
 
-    SetPendingEvent(PREFETCH_DONE_EVENT_PENDING);
-    ClearPendingEvent(PREFETCH_REQUEST_EVENT_PENDING);
     return;
   }
 
@@ -590,13 +601,23 @@ void MediaSourcePlayer::MediaDecoderCallback(
     return;
   }
 
-  if (pending_event_ != NO_EVENT_PENDING) {
+  DCHECK(!IsEventPending(PREFETCH_DONE_EVENT_PENDING));
+
+  // Let |SEEK_EVENT_PENDING| (the highest priority event outside of
+  // |PREFETCH_DONE_EVENT_PENDING|) preempt output EOS detection here. Process
+  // any other pending events only after handling EOS detection.
+  if (IsEventPending(SEEK_EVENT_PENDING)) {
     ProcessPendingEvents();
     return;
   }
 
   if (status == MEDIA_CODEC_OUTPUT_END_OF_STREAM) {
-    PlaybackCompleted(is_audio);
+    PlaybackCompleted(is_audio);  // Includes ProcessPendingEvents() if needed.

[acolwell GONE FROM CHROMIUM, 2013/12/06 18:28:30]

nit: Why don't you just let this fall through to the statement below? Then you
don't need this comment or the call inside PlaybackCompleted(). This appears to
be the only callsite so I think it is safe.

[wolenetz, 2013/12/09 22:58:27]

A little tweaking is also required to ensure that we do not proceed with
remainder of this method beyond PlaybackCompleted() + conditional
ProcessPendingEvents() after detecting output_eos here. Done.

+    return;
+  }
+
+  if (pending_event_ != NO_EVENT_PENDING) {
+    ProcessPendingEvents();
     return;
   }
 
@@ -643,6 +664,7 @@ void MediaSourcePlayer::MediaDecoderCallback(
 void MediaSourcePlayer::DecodeMoreAudio() {
   DVLOG(1) << __FUNCTION__;
   DCHECK(!audio_decoder_job_->is_decoding());
+  DCHECK(!AudioFinishedOrNoAudio());
 
   if (audio_decoder_job_->Decode(
           start_time_ticks_, start_presentation_timestamp_, base::Bind(
@@ -672,6 +694,7 @@ void MediaSourcePlayer::DecodeMoreAudio() {
 void MediaSourcePlayer::DecodeMoreVideo() {
   DVLOG(1) << __FUNCTION__;
   DCHECK(!video_decoder_job_->is_decoding());
+  DCHECK(!VideoFinishedOrNoVideo());
 
   if (video_decoder_job_->Decode(
           start_time_ticks_, start_presentation_timestamp_, base::Bind(
@@ -710,12 +733,15 @@ void MediaSourcePlayer::PlaybackCompleted(bool is_audio) {
   else
     video_finished_ = true;
 
-  if ((!HasAudio() || audio_finished_) && (!HasVideo() || video_finished_)) {
+  if (AudioFinishedOrNoAudio() && VideoFinishedOrNoVideo()) {
     playing_ = false;
     clock_.Pause();
     start_time_ticks_ = base::TimeTicks();
     manager()->OnPlaybackComplete(player_id());
   }
+
+  if (pending_event_ != NO_EVENT_PENDING)
+    ProcessPendingEvents();
 }
 
 void MediaSourcePlayer::ClearDecodingData() {
@@ -735,6 +761,14 @@ bool MediaSourcePlayer::HasAudio() {
   return kUnknownAudioCodec != audio_codec_;
 }
 
+bool MediaSourcePlayer::AudioFinishedOrNoAudio() {

[acolwell GONE FROM CHROMIUM, 2013/12/06 18:28:30]

nit: I think the OrNoAudio() just confuses things at the call sites. How about
just always initalizing 'audio_finished_ = !HasAudio()' everywhere you
'audio_finished_ = false;'

You could put this in a ResetFinishedFlags() helper so you don't have to
duplicate the code & explanitory comments.

[wolenetz, 2013/12/09 22:58:27]

This makes sense with the current *_finished_ flags' naming, though additional
tweaking of OnDemuxerConfigsAvailable() would be necessary: Has{Audio,Video}()
potentially change across configuration change, from initial construction to
changing the {audio, video} codec from unknown.

An alternative approach is to rename *_finished_ to reached_*_eos_, and
*FinishedOrNoVideo() to just *Finished(), without altering anything in the
config change handling (since
1) eos cannot be reached for an unknown codec, and
2) everywhere we want to know if a stream is "finished", if the stream is
unconfigured, we expect it to be finished).

I've chosen the latter (renaming) approach. Outside of constructor
initialization, these reached_*_eos_ members are only set to false in
OnDemuxerSeekDone(), so I have not added a reset eos helper. Done.

+  return audio_finished_ || !HasAudio();
+}
+
+bool MediaSourcePlayer::VideoFinishedOrNoVideo() {

[acolwell GONE FROM CHROMIUM, 2013/12/06 18:28:30]

ditto

[wolenetz, 2013/12/09 22:58:27]

Done.

+  return video_finished_ || !HasVideo();
+}
+
 void MediaSourcePlayer::ConfigureAudioDecoderJob() {
   if (!HasAudio()) {
     audio_decoder_job_.reset();
@@ -916,10 +950,15 @@ void MediaSourcePlayer::OnPrefetchDone() {
   if (!clock_.IsPlaying())
     clock_.Play();
 
-  if (audio_decoder_job_)
+  if (!AudioFinishedOrNoAudio()) {
+    DCHECK(audio_decoder_job_);

[acolwell GONE FROM CHROMIUM, 2013/12/06 18:28:30]

nit: This seems redundant since DecodeMoreAudio() will crash if this isn't true.
I'd rather have it crash there then leak preconditions to this callsite.

[wolenetz, 2013/12/09 22:58:27]

Done.

     DecodeMoreAudio();
-  if (video_decoder_job_)
+  }
+
+  if (!VideoFinishedOrNoVideo()) {
+    DCHECK(video_decoder_job_);

[acolwell GONE FROM CHROMIUM, 2013/12/06 18:28:30]

ditto

[wolenetz, 2013/12/09 22:58:27]

Done.

     DecodeMoreVideo();
+  }
 }
 
 const char* MediaSourcePlayer::GetEventName(PendingEventFlags event) {