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..af2ccaf3fc0ef7917e5fc19de9f71002bbe4fa70 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,15 +531,19 @@ 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);
 
     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);
@@ -590,13 +595,13 @@ void MediaSourcePlayer::MediaDecoderCallback(
     return;
   }
 
-  if (pending_event_ != NO_EVENT_PENDING) {
-    ProcessPendingEvents();
+  if (status == MEDIA_CODEC_OUTPUT_END_OF_STREAM) {
+    PlaybackCompleted(is_audio);
     return;
   }
 
-  if (status == MEDIA_CODEC_OUTPUT_END_OF_STREAM) {
-    PlaybackCompleted(is_audio);
+  if (pending_event_ != NO_EVENT_PENDING) {
+    ProcessPendingEvents();
     return;
   }
 
@@ -643,6 +648,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 +678,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 +717,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)

[qinmin, 2013/12/04 00:20:16]

i think you should process the pending events first.
For example, playing_ may not be changed if the pending event is seek.

That's why i think my other comments is better: rather than switch the order of
complete event and pending event, just do a check to see if the stream is
already reaching the end before sending out prefetch request.

[wolenetz, 2013/12/05 01:03:28]

From our chats/analyses, MediaDecoderCallback() can protect against completing
playback if there was a seek that became pending during EOS decode by checking
for that exact condition. All other pending events are unaffected by decode of
EOS triggering playback completion (except PREFETCH_DONE_EVENT_PENDING, which is
illegal during decode). We need to retain PREFETCH_REQUEST processing for any
remaining unfinished stream, even if one of the potentially multiple streams
decodes EOS. Patch set 5 includes these adjustments and associated new unit
tests.

We also discussed adding defense in depth by having MDJ reply with output_eos if
it has had no intervening flush since original decoding output_eos. I've added a
DCHECK+elevated output_eos_encountered to a member instead of hiding what would
be an incorrect usage of MDJ.

+    ProcessPendingEvents();
 }
 
 void MediaSourcePlayer::ClearDecodingData() {
@@ -735,6 +745,14 @@ bool MediaSourcePlayer::HasAudio() {
   return kUnknownAudioCodec != audio_codec_;
 }
 
+bool MediaSourcePlayer::AudioFinishedOrNoAudio() {
+  return audio_finished_ || !HasAudio();
+}
+
+bool MediaSourcePlayer::VideoFinishedOrNoVideo() {
+  return video_finished_ || !HasVideo();
+}
+
 void MediaSourcePlayer::ConfigureAudioDecoderJob() {
   if (!HasAudio()) {
     audio_decoder_job_.reset();
@@ -916,10 +934,15 @@ void MediaSourcePlayer::OnPrefetchDone() {
   if (!clock_.IsPlaying())
     clock_.Play();
 
-  if (audio_decoder_job_)
+  if (!AudioFinishedOrNoAudio()) {
+    DCHECK(audio_decoder_job_);
     DecodeMoreAudio();
-  if (video_decoder_job_)
+  }
+
+  if (!VideoFinishedOrNoVideo()) {
+    DCHECK(video_decoder_job_);
     DecodeMoreVideo();
+  }
 }
 
 const char* MediaSourcePlayer::GetEventName(PendingEventFlags event) {