Let only seeks reset Android MSE stream playback completion

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 55 matching lines @@
     scoped_ptr<DemuxerAndroid> demuxer)
     : MediaPlayerAndroid(player_id, manager),
       demuxer_(demuxer.Pass()),
       pending_event_(NO_EVENT_PENDING),
       width_(0),
       height_(0),
       audio_codec_(kUnknownAudioCodec),
       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),
       volume_(-1.0),
       clock_(&default_tick_clock_),
       next_video_data_is_iframe_(true),
       doing_browser_seek_(false),
       pending_seek_(false),
       reconfig_audio_decoder_(false),
       reconfig_video_decoder_(false),
@@ skipping 223 matching lines @@
   // Create decoder jobs if they are not created
   ConfigureAudioDecoderJob();
   ConfigureVideoDecoderJob();
 
   // If one of the decoder job is not ready, do nothing.
   if ((HasAudio() && !audio_decoder_job_) ||
       (HasVideo() && !video_decoder_job_)) {
     return;
   }
 
-  audio_finished_ = false;
-  video_finished_ = false;
   SetPendingEvent(PREFETCH_REQUEST_EVENT_PENDING);
   ProcessPendingEvents();
 }
 
 void MediaSourcePlayer::OnDemuxerConfigsAvailable(
     const DemuxerConfigs& configs) {
   DVLOG(1) << __FUNCTION__;
   duration_ = base::TimeDelta::FromMilliseconds(configs.duration_ms);
   clock_.SetDuration(duration_);
 
@@ skipping 111 matching lines @@
     // A browser seek must not jump into the past. Ideally, it seeks to the
     // requested time, but it might jump into the future.
     DCHECK(actual_browser_seek_time >= GetCurrentTime());
     DVLOG(1) << __FUNCTION__ << " : setting clock to actual browser seek time: "
              << actual_browser_seek_time.InSecondsF();
     clock_.SetTime(actual_browser_seek_time, actual_browser_seek_time);
     if (audio_timestamp_helper_)
       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_|
   // is calculated from decoder output, while preroll relies on the access
   // unit's timestamp. There are some differences between the two.
   preroll_timestamp_ = current_time;
   if (audio_decoder_job_)
     audio_decoder_job_->BeginPrerolling(preroll_timestamp_);
   if (video_decoder_job_)
     video_decoder_job_->BeginPrerolling(preroll_timestamp_);
@@ skipping 59 matching lines @@
 
     // Return early if we can't successfully configure a new video decoder job
     // yet, except continue processing other pending events if |surface_| is
     // empty.
     if (HasVideo() && !video_decoder_job_ && !surface_.IsEmpty())
       return;
   }
 
   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;
   }
 
   DCHECK_EQ(pending_event_, NO_EVENT_PENDING);
 
   // Now that all pending events have been handled, resume decoding if we are
   // still playing.
   if (playing_)
     StartInternal();
 }
@@ skipping 27 matching lines @@
   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;
   }
 
+  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);  // 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;
   }
 
-  if (status == MEDIA_CODEC_OUTPUT_END_OF_STREAM) {
-    PlaybackCompleted(is_audio);
-    return;
-  }
-
   if (status == MEDIA_CODEC_OK && is_clock_manager &&
       presentation_timestamp != kNoTimestamp()) {
     UpdateTimestamps(presentation_timestamp, audio_output_bytes);
   }
 
   if (!playing_) {
     if (is_clock_manager)
       clock_.Pause();
     return;
   }
@@ skipping 23 matching lines @@
     DecodeMoreAudio();
     return;
   }
 
   DecodeMoreVideo();
 }
 
 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(
               &MediaSourcePlayer::MediaDecoderCallback,
               weak_this_.GetWeakPtr(), true))) {
     TRACE_EVENT_ASYNC_BEGIN0("media", "MediaSourcePlayer::DecodeMoreAudio",
                              audio_decoder_job_.get());
     return;
   }
 
   // Failed to start the next decode.
   // Wait for demuxer ready message.
   DCHECK(!reconfig_audio_decoder_);
   reconfig_audio_decoder_ = true;
 
   // Config change may have just been detected on the other stream. If so,
   // don't send a duplicate demuxer config request.
   if (IsEventPending(CONFIG_CHANGE_EVENT_PENDING)) {
     DCHECK(reconfig_video_decoder_);
     return;
   }
 
   SetPendingEvent(CONFIG_CHANGE_EVENT_PENDING);
   ProcessPendingEvents();
 }
 
 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(
               &MediaSourcePlayer::MediaDecoderCallback,
               weak_this_.GetWeakPtr(), false))) {
     TRACE_EVENT_ASYNC_BEGIN0("media", "MediaSourcePlayer::DecodeMoreVideo",
                              video_decoder_job_.get());
     return;
   }
 
@@ skipping 18 matching lines @@
   ProcessPendingEvents();
 }
 
 void MediaSourcePlayer::PlaybackCompleted(bool is_audio) {
   DVLOG(1) << __FUNCTION__ << "(" << is_audio << ")";
   if (is_audio)
     audio_finished_ = true;
   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() {
   DVLOG(1) << __FUNCTION__;
   if (audio_decoder_job_)
     audio_decoder_job_->Flush();
   if (video_decoder_job_)
     video_decoder_job_->Flush();
   start_time_ticks_ = base::TimeTicks();
 }
 
 bool MediaSourcePlayer::HasVideo() {
   return kUnknownVideoCodec != video_codec_;
 }
 
 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();
     return;
   }
 
   // Create audio decoder job only if config changes.
   if (audio_decoder_job_ && !reconfig_audio_decoder_)
     return;
 
@@ skipping 161 matching lines @@
   if (pending_event_ != NO_EVENT_PENDING) {
     ProcessPendingEvents();
     return;
   }
 
   start_time_ticks_ = base::TimeTicks::Now();
   start_presentation_timestamp_ = GetCurrentTime();
   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) {
   static const char* kPendingEventNames[] = {
     "SEEK",
     "SURFACE_CHANGE",
     "CONFIG_CHANGE",
     "PREFETCH_REQUEST",
     "PREFETCH_DONE",
   };
@@ skipping 21 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