Fix MediaSourcePlayer unittests and minor code cleanup.

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 "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #include "base/barrier_closure.h"
 #include "base/basictypes.h"
@@ skipping 134 matching lines @@
 base::TimeDelta MediaSourcePlayer::GetCurrentTime() {
   return clock_.Elapsed();
 }
 
 base::TimeDelta MediaSourcePlayer::GetDuration() {
   return duration_;
 }
 
 void MediaSourcePlayer::Release() {
   DVLOG(1) << __FUNCTION__;
-  ClearDecodingData();

[acolwell GONE FROM CHROMIUM, 2013/08/30 18:27:38]

Not needed since the jobs are getting Release()'ed on the following lines.

   audio_decoder_job_.reset();
   video_decoder_job_.reset();
   reconfig_audio_decoder_ = false;
   reconfig_video_decoder_ = false;
   playing_ = false;
   pending_event_ = NO_EVENT_PENDING;
   surface_ = gfx::ScopedJavaSurface();
   ReleaseMediaResourcesFromManager();
 }
 
@@ skipping 144 matching lines @@
     return;
 
   ClearPendingEvent(SEEK_EVENT_PENDING);
 
   OnSeekComplete();
   ProcessPendingEvents();
 }
 
 void MediaSourcePlayer::UpdateTimestamps(
     const base::TimeDelta& presentation_timestamp, size_t audio_output_bytes) {
+  base::TimeDelta new_max_time = presentation_timestamp;
+
   if (audio_output_bytes > 0) {
     audio_timestamp_helper_->AddFrames(
         audio_output_bytes / (kBytesPerAudioOutputSample * num_channels_));
-    clock_.SetMaxTime(audio_timestamp_helper_->GetTimestamp());
-  } else {
-    clock_.SetMaxTime(presentation_timestamp);
+    new_max_time = audio_timestamp_helper_->GetTimestamp();
   }
 
+  clock_.SetMaxTime(new_max_time);
   OnTimeUpdated();
 }
 
 void MediaSourcePlayer::ProcessPendingEvents() {
   DVLOG(1) << __FUNCTION__ << " : 0x"
            << std::hex << pending_event_;
   // Wait for all the decoding jobs to finish before processing pending tasks.
   if ((audio_decoder_job_ && audio_decoder_job_->is_decoding()) ||
       (video_decoder_job_ && video_decoder_job_->is_decoding())) {
     DVLOG(1) << __FUNCTION__ << " : A job is still decoding.";
@@ skipping 51 matching lines @@
   // Now that all pending events have been handled, resume decoding if we are
   // still playing.
   if (playing_)
     StartInternal();
 }
 
 void MediaSourcePlayer::MediaDecoderCallback(
     bool is_audio, MediaDecoderJob::DecodeStatus decode_status,
     const base::TimeDelta& presentation_timestamp, size_t audio_output_bytes) {
   DVLOG(1) << __FUNCTION__;
-  if (is_audio)
+
+  bool is_clock_manager = is_audio || !HasAudio();
+
+  if (is_clock_manager)

[acolwell GONE FROM CHROMIUM, 2013/08/30 18:27:38]

The starvation callback gets scheduled in the video-only case, but wasn't
getting cancelled. This just makes the code consistant about the stream that
schedules the callback below also cancels it.

     decoder_starvation_callback_.Cancel();
 
   if (decode_status == MediaDecoderJob::DECODE_FAILED) {
     Release();
     OnMediaError(MEDIA_ERROR_DECODE);
     return;
   }
 
   if (pending_event_ != NO_EVENT_PENDING) {
     ProcessPendingEvents();
     return;
   }
 
-  if (decode_status == MediaDecoderJob::DECODE_SUCCEEDED &&

[qinmin, 2013/08/30 22:27:43]

why changing the order of these 2 if statement? 
First we have to update the clock, then determine whether media ends or not.
If we check the latter first, then the displayed time might be incorrect.

[acolwell GONE FROM CHROMIUM, 2013/09/06 00:34:55]

The blocks are mutually exclusive. decode status can't be DECODE_SUCCEEDED and
DECODE_OUTPUT_END_OF_STREAM at the same time. I figured it was better to get the
end of stream early return path earlier in the function.

[qinmin, 2013/09/06 00:56:33]

Looks like this is a bug. When decode status is DECODE_OUTPUT_END_OF_STREAM, we
should also update the timestamp. output EOS can come with valid audio data in
the same dequeueOutputBuffer() call.
We can leave this in a separate CL though.


On 2013/09/06 00:34:55, acolwell wrote:

-      (is_audio || !HasAudio())) {
-    UpdateTimestamps(presentation_timestamp, audio_output_bytes);
-  }
-
   if (decode_status == MediaDecoderJob::DECODE_OUTPUT_END_OF_STREAM) {
     PlaybackCompleted(is_audio);
     return;
   }
 
+  if (decode_status == MediaDecoderJob::DECODE_SUCCEEDED && is_clock_manager)
+    UpdateTimestamps(presentation_timestamp, audio_output_bytes);
+
   if (!playing_) {
-    if (is_audio || !HasAudio())
+    if (is_clock_manager)
       clock_.Pause();
     return;
   }
 
-  base::TimeDelta current_timestamp = GetCurrentTime();
+  if (is_clock_manager && decode_status == MediaDecoderJob::DECODE_SUCCEEDED)
+    StartStarvationCallback(presentation_timestamp);
+
   if (is_audio) {
-    if (decode_status == MediaDecoderJob::DECODE_SUCCEEDED) {
-      base::TimeDelta timeout =
-          audio_timestamp_helper_->GetTimestamp() - current_timestamp;
-      StartStarvationCallback(timeout);
-    }
-      DecodeMoreAudio();
+    DecodeMoreAudio();
     return;
   }
 
-  if (!HasAudio() && decode_status == MediaDecoderJob::DECODE_SUCCEEDED) {
-    DCHECK(current_timestamp <= presentation_timestamp);
-    // For video only streams, fps can be estimated from the difference
-    // between the previous and current presentation timestamps. The
-    // previous presentation timestamp is equal to current_timestamp.
-    // TODO(qinmin): determine whether 2 is a good coefficient for estimating
-    // video frame timeout.
-    StartStarvationCallback(2 * (presentation_timestamp - current_timestamp));
-  }
-
   DecodeMoreVideo();
 }
 
 void MediaSourcePlayer::DecodeMoreAudio() {
   DVLOG(1) << __FUNCTION__;
   DCHECK(!audio_decoder_job_->is_decoding());
 
   if (audio_decoder_job_->Decode(
           start_time_ticks_, start_presentation_timestamp_, base::Bind(
               &MediaSourcePlayer::MediaDecoderCallback,
@@ skipping 123 matching lines @@
   OnMediaMetadataChanged(duration_, width_, height_, true);
 }
 
 void MediaSourcePlayer::OnDecoderStarved() {
   DVLOG(1) << __FUNCTION__;
   SetPendingEvent(PREFETCH_REQUEST_EVENT_PENDING);
   ProcessPendingEvents();
 }
 
 void MediaSourcePlayer::StartStarvationCallback(
-    const base::TimeDelta& timeout) {
-  DVLOG(1) << __FUNCTION__ << "(" << timeout.InSecondsF() << ")";
+    const base::TimeDelta& presentation_timestamp) {
+  // 20ms was chosen because it is the typical size of a compressed audio frame.
+  // Anything smaller than this would likely cause unnecessary cycling in and
+  // out of the prefetch state.
+  const base::TimeDelta kMinStarvationTimeout =
+      base::TimeDelta::FromMilliseconds(20);
+
+  base::TimeDelta current_timestamp = GetCurrentTime();
+  base::TimeDelta timeout;
+  if (HasAudio()) {
+    timeout = audio_timestamp_helper_->GetTimestamp() - current_timestamp;
+  } else {
+    DCHECK(current_timestamp <= presentation_timestamp);
+
+    // For video only streams, fps can be estimated from the difference
+    // between the previous and current presentation timestamps. The
+    // previous presentation timestamp is equal to current_timestamp.
+    // TODO(qinmin): determine whether 2 is a good coefficient for estimating
+    // video frame timeout.
+    timeout = 2 * (presentation_timestamp - current_timestamp);
+  }
+
+  timeout = std::max(timeout, kMinStarvationTimeout);

[acolwell GONE FROM CHROMIUM, 2013/08/30 18:27:38]

This fixes a bug that was in the code where it would schedule 0ms starvation
callbacks at the beginning of playback because the current time position hadn't
moved forward yet. This would trigger unnecessary prefetching.

 
   decoder_starvation_callback_.Reset(
       base::Bind(&MediaSourcePlayer::OnDecoderStarved,
                  weak_this_.GetWeakPtr()));
   base::MessageLoop::current()->PostDelayedTask(
       FROM_HERE, decoder_starvation_callback_.callback(), timeout);
 }
 
 void MediaSourcePlayer::SetVolumeInternal() {
   if (audio_decoder_job_ && volume_ >= 0)
@@ skipping 61 matching lines @@
 
 void MediaSourcePlayer::ClearPendingEvent(PendingEventFlags event) {
   DVLOG(1) << __FUNCTION__ << "(" << GetEventName(event) << ")";
   DCHECK_NE(event, NO_EVENT_PENDING);
   DCHECK(IsEventPending(event));
 
   pending_event_ &= ~event;
 }
 
 }  // namespace media