Fix MediaSourcePlayer unittests and minor code cleanup.

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 ba87bf0b38f4d2f6b1f490cbba0a8dce4b90039d..2fa07dcb3a0e4e32f3641fac5d71cffdf7f5c390 100644
--- a/media/base/android/media_source_player.cc
+++ b/media/base/android/media_source_player.cc
@@ -152,7 +152,6 @@ base::TimeDelta MediaSourcePlayer::GetDuration() {
 
 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;
@@ -317,14 +316,15 @@ void MediaSourcePlayer::OnSeekRequestAck(unsigned seek_request_id) {
 
 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();
 }
 
@@ -396,7 +396,10 @@ 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) {
@@ -410,43 +413,28 @@ void MediaSourcePlayer::MediaDecoderCallback(
     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();
 }
 
@@ -590,8 +578,29 @@ void MediaSourcePlayer::OnDecoderStarved() {
 }
 
 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,