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 64 matching lines @@
   if (playing_)
     StartInternal();
 }
 
 void MediaSourcePlayer::SetDrmBridge(MediaDrmBridge* drm_bridge) {
   // Currently we don't support DRM change during the middle of playback, even
   // if the player is paused.
   // TODO(qinmin): support DRM change after playback has started.
   // http://crbug.com/253792.
   if (GetCurrentTime() > base::TimeDelta()) {
-    LOG(INFO) << "Setting DRM bridge after playback has started. "
-              << "This is not well supported!";
+    VLOG(0) << "Setting DRM bridge after playback has started. "
+            << "This is not well supported!";
   }
 
   drm_bridge_ = drm_bridge;
 
   if (drm_bridge_->GetMediaCrypto().is_null()) {
     drm_bridge_->SetMediaCryptoReadyCB(base::Bind(
         &MediaSourcePlayer::OnMediaCryptoReady, weak_this_.GetWeakPtr()));
     return;
   }
 
@@ skipping 25 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 (!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);
+    int count = ((audio_decoder_job_ && !audio_finished_) ? 1 : 0) +

[acolwell GONE FROM CHROMIUM, 2013/11/25 21:43:59]

I think the audio_decoder_ && !audio_finished_ condition should be moved into a
helper function named something like CanDecodeAudio() or AudioNotFinished().
Something similar should be done for video as well.

[wolenetz, 2013/12/04 00:13:29]

Done, with ternary swapped, as AudioFinishedOrNoAudio() + similar for video.

+        ((video_decoder_job_ && !video_finished_) ? 1 : 0);
 
     base::Closure barrier = BarrierClosure(count, base::Bind(
         &MediaSourcePlayer::OnPrefetchDone, weak_this_.GetWeakPtr()));
 
-    if (audio_decoder_job_)
+    if (audio_decoder_job_ && !audio_finished_)
       audio_decoder_job_->Prefetch(barrier);
 
-    if (video_decoder_job_)
+    if (video_decoder_job_ && !video_finished_)
       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
@@ skipping 37 matching lines @@
     manager()->OnError(player_id(), MEDIA_ERROR_DECODE);
     return;
   }
 
   if (pending_event_ != NO_EVENT_PENDING) {
     ProcessPendingEvents();
     return;
   }
 
   if (status == MEDIA_CODEC_OUTPUT_END_OF_STREAM) {
     PlaybackCompleted(is_audio);

[qinmin, 2013/11/26 23:49:47]

PlaybackCommpleted is executed after ProcessPendingEvents().
If there is a PREFETCH_REQUEST_EVENT, then that event could cause MSP to call
StartInternal() without setting audio_finished_/video_finished_. And causing the
infinite DEQUEUE_OUTPUT_AGAIN_LATER issue. 

We should reverse the order of this if statement and the if statement above. And
change PlaybackCompleted() to check and process all the pending events.

[qinmin, 2013/12/03 23:45:16]

Pending seek is the reason processPendingEvents should have higher priority than
EOS. However, output_EOS should have higher priority than PREFETCH_REQUEST.
Another way to tackle this issue is that we can stop the starvation callback if
input EOS is already reached. In that case, we should not have PREFETCH_REQUEST
pending when we got output_EOS. If we do this, then we don't need the reorder
the if statement.

On 2013/11/26 23:49:47, qinmin wrote:

[wolenetz, 2013/12/04 00:13:29]

Done per the original comment, not the one from 12/3.
One of the new tests I added fails though:
VA_PlaybackCompletionAcrossConfigChange's OnPrefetchDone()'s DecodeMoreAudio()
sees audio kConfigChanged, sets CONFIG_CHANGE_EVENT_PENDING, and then
OnPrefetchDone()'s DecodeMoreVideo() decodes EOS while CONFIG_CHANGE_EVENT is
still pending, leading to PlaybackCompleted() calling ProcessPendingEvents() and
requesting a second time the demuxer configs.

Per our chat, I've filed http://crbug.com/325528 and added reference to this in
the unit test that exposes this bug (and updated the unit test to expect this
redundant config change request meanwhile).

     return;
   }
 
   if (status == MEDIA_CODEC_OK && is_clock_manager &&
       presentation_timestamp != kNoTimestamp()) {
     UpdateTimestamps(presentation_timestamp, audio_output_bytes);
   }
 
   if (!playing_) {
     if (is_clock_manager)
@@ skipping 26 matching lines @@
     DecodeMoreAudio();
     return;
   }
 
   DecodeMoreVideo();
 }
 
 void MediaSourcePlayer::DecodeMoreAudio() {
   DVLOG(1) << __FUNCTION__;
   DCHECK(!audio_decoder_job_->is_decoding());
+  DCHECK(!audio_finished_);
 
   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(!video_finished_);
 
   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_)) {

[acolwell GONE FROM CHROMIUM, 2013/11/25 21:43:59]

This makes me wonder whether audio_decoder_job_ && !audio_finished_ is the right
condition for the "audio isn't finished" condition.

1. Should HasAudio() be factored into the other conditions you've added?
2. Could this be changed to audio_decoder_job_ && !audio_finished_?
3. Could this become (!AudioNotFinished() && !VideoNotFinished())  or
(AudioFinished() && VideoFinished()) and reverse the sense of the checks
elsewhere.

[wolenetz, 2013/12/04 00:13:29]

Done, following the latter option in 3, with a little extra distinction in name
versus the {audio,video}_finished_ members, like
{Audio,Video}FinishedOrNo{Audio,Video}().

Has{Audio,Video}() is now factored into those other conditions.
I added DCHECKs in ProcessPendingEvents() for PREFETCH_REQUEST_EVENT_PENDING
processing and in OnPrefetchDone() to clarify that !AudioFinishedOrNoAudio()
implies audio_decoder_job_ must exist when handling that event/call (plus
similar for video).

Note that the current existence or lack of the decoder job for a finished stream
does not change whether or not that stream is finished.

     playing_ = false;
     clock_.Pause();
     start_time_ticks_ = base::TimeTicks();
     manager()->OnPlaybackComplete(player_id());
   }
 }
 
 void MediaSourcePlayer::ClearDecodingData() {
   DVLOG(1) << __FUNCTION__;
   if (audio_decoder_job_)
@@ skipping 185 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 (audio_decoder_job_ && !audio_finished_)
     DecodeMoreAudio();
-  if (video_decoder_job_)
+  if (video_decoder_job_ && !video_finished_)
     DecodeMoreVideo();
 }
 
 const char* MediaSourcePlayer::GetEventName(PendingEventFlags event) {
   static const char* kPendingEventNames[] = {
     "SEEK",
     "SURFACE_CHANGE",
     "CONFIG_CHANGE",
     "PREFETCH_REQUEST",
     "PREFETCH_DONE",
@@ skipping 22 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