Fix MediaSourcePlayer unittests and minor code cleanup.

Fixes a few tests were broken by minor changes in behavior 
introduced in http://crrev.com/220164.

BUG=281730
TEST=MediaSourcePlayerTest.StartTimeTicksResetAfterDecoderUnderruns
     and MediaSourcePlayerTest.ReadFromDemuxerAfterSeek pass now.

Review URL: https://chromiumcodereview.appspot.com/23620012

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@221781 0039d316-1c4b-4281-b951-d872f2087c98

media/base/android/media_decoder_job.cc

@@ -53,11 +53,6 @@ void MediaDecoderJob::OnDataReceived(const DemuxerData& data) {
   done_cb.Run();
 }
 
-bool MediaDecoderJob::HasData() const {
-  DCHECK(ui_loop_->BelongsToCurrentThread());
-  return access_unit_index_ < received_data_.access_units.size();
-}
-
 void MediaDecoderJob::Prefetch(const base::Closure& prefetch_cb) {
   DCHECK(ui_loop_->BelongsToCurrentThread());
   DCHECK(on_data_received_cb_.is_null());
@@ -156,7 +151,7 @@ MediaCodecStatus MediaDecoderJob::QueueInputBuffer(const AccessUnit& unit) {
   // TODO(qinmin): skip frames if video is falling far behind.
   DCHECK_GE(input_buf_index, 0);
   if (unit.end_of_stream || unit.data.empty()) {
-    media_codec_bridge_->QueueEOS(input_buf_index_);
+    media_codec_bridge_->QueueEOS(input_buf_index);
     return MEDIA_CODEC_INPUT_END_OF_STREAM;
   }
 
@@ -185,10 +180,22 @@ MediaCodecStatus MediaDecoderJob::QueueInputBuffer(const AccessUnit& unit) {
   return status;
 }
 
+bool MediaDecoderJob::HasData() const {
+  DCHECK(ui_loop_->BelongsToCurrentThread());
+  DCHECK(!input_eos_encountered_ ||
+         (received_data_.access_units.size() > 0 &&
+          access_unit_index_ < received_data_.access_units.size()))
+      << "access_unit_index_.size() " << received_data_.access_units.size()
+      << " access_unit_index_ " << access_unit_index_;
+  return access_unit_index_ < received_data_.access_units.size() ||
+      input_eos_encountered_;
+}
+
 void MediaDecoderJob::RequestData(const base::Closure& done_cb) {
   DVLOG(1) << __FUNCTION__;
   DCHECK(ui_loop_->BelongsToCurrentThread());
   DCHECK(on_data_received_cb_.is_null());
+  DCHECK(!input_eos_encountered_);
 
   received_data_ = DemuxerData();
   access_unit_index_ = 0;
@@ -252,10 +259,6 @@ void MediaDecoderJob::DecodeInternal(
       &output_eos_encountered);
 
   if (status != MEDIA_CODEC_OK) {
-    DCHECK(!(status == MEDIA_CODEC_OUTPUT_BUFFERS_CHANGED ||
-             status == MEDIA_CODEC_OUTPUT_FORMAT_CHANGED) ||
-           (input_status != MEDIA_CODEC_INPUT_END_OF_STREAM));
-
     if (status == MEDIA_CODEC_OUTPUT_BUFFERS_CHANGED) {
         media_codec_bridge_->GetOutputBuffers();
         status = MEDIA_CODEC_OK;
@@ -307,14 +310,24 @@ void MediaDecoderJob::OnDecodeCompleted(
   }
 
   DCHECK(!decode_cb_.is_null());
-
-  if (status != MEDIA_CODEC_ERROR &&
-      status != MEDIA_CODEC_DEQUEUE_INPUT_AGAIN_LATER &&
-      status != MEDIA_CODEC_INPUT_END_OF_STREAM &&
-      status != MEDIA_CODEC_NO_KEY &&
-      status != MEDIA_CODEC_STOPPED) {
-    access_unit_index_++;
-  }
+  switch (status) {
+    case MEDIA_CODEC_OK:
+    case MEDIA_CODEC_DEQUEUE_OUTPUT_AGAIN_LATER:
+    case MEDIA_CODEC_OUTPUT_BUFFERS_CHANGED:
+    case MEDIA_CODEC_OUTPUT_FORMAT_CHANGED:
+    case MEDIA_CODEC_OUTPUT_END_OF_STREAM:
+      if (!input_eos_encountered_)
+        access_unit_index_++;
+      break;
+
+    case MEDIA_CODEC_DEQUEUE_INPUT_AGAIN_LATER:
+    case MEDIA_CODEC_INPUT_END_OF_STREAM:
+    case MEDIA_CODEC_NO_KEY:
+    case MEDIA_CODEC_STOPPED:
+    case MEDIA_CODEC_ERROR:
+      // Do nothing.
+      break;
+  };
 
   stop_decode_pending_ = false;
   base::ResetAndReturn(&decode_cb_).Run(status, presentation_timestamp,

media/base/android/media_decoder_job.h

@@ -35,9 +35,6 @@ class MediaDecoderJob {
   // Called by MediaSourcePlayer when more data for this object has arrived.
   void OnDataReceived(const DemuxerData& data);
 
-  // Returns true if this object has data to decode.
-  bool HasData() const;
-
   // Prefetch so we know the decoder job has data when we call Decode().
   // |prefetch_cb| - Run when prefetching has completed.
   void Prefetch(const base::Closure& prefetch_cb);
@@ -90,6 +87,9 @@ class MediaDecoderJob {
 
   MediaCodecStatus QueueInputBuffer(const AccessUnit& unit);
 
+  // Returns true if this object has data to decode.
+  bool HasData() const;
+
   // Initiates a request for more data.
   // |done_cb| is called when more data is available in |received_data_|.
   void RequestData(const base::Closure& done_cb);

media/base/android/media_source_player.cc

@@ -342,14 +342,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();
 }
 
@@ -424,7 +425,10 @@ void MediaSourcePlayer::MediaDecoderCallback(
     bool is_audio, MediaCodecStatus status,
     const base::TimeDelta& presentation_timestamp, size_t audio_output_bytes) {
   DVLOG(1) << __FUNCTION__ << ": " << is_audio << ", " << status;
-  if (is_audio)
+
+  bool is_clock_manager = is_audio || !HasAudio();
+
+  if (is_clock_manager)
     decoder_starvation_callback_.Cancel();
 
   if (status == MEDIA_CODEC_ERROR) {
@@ -438,17 +442,16 @@ void MediaSourcePlayer::MediaDecoderCallback(
     return;
   }
 
-  if (status == MEDIA_CODEC_OK && (is_audio || !HasAudio())) {
-    UpdateTimestamps(presentation_timestamp, audio_output_bytes);
-  }
-
   if (status == MEDIA_CODEC_OUTPUT_END_OF_STREAM) {
     PlaybackCompleted(is_audio);
     return;
   }
 
+  if (status == MEDIA_CODEC_OK && is_clock_manager)
+    UpdateTimestamps(presentation_timestamp, audio_output_bytes);
+
   if (!playing_) {
-    if (is_audio || !HasAudio())
+    if (is_clock_manager)
       clock_.Pause();
     return;
   }
@@ -456,27 +459,14 @@ void MediaSourcePlayer::MediaDecoderCallback(
   if (status == MEDIA_CODEC_NO_KEY)
     return;
 
-  base::TimeDelta current_timestamp = GetCurrentTime();
+  if (status == MEDIA_CODEC_OK && is_clock_manager)
+    StartStarvationCallback(presentation_timestamp);
+
   if (is_audio) {
-    if (status == MEDIA_CODEC_OK) {
-      base::TimeDelta timeout =
-          audio_timestamp_helper_->GetTimestamp() - current_timestamp;
-      StartStarvationCallback(timeout);
-    }
     DecodeMoreAudio();
     return;
   }
 
-  if (!HasAudio() && status == MEDIA_CODEC_OK) {
-    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();
 }
 
@@ -620,8 +610,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);
 
   decoder_starvation_callback_.Reset(
       base::Bind(&MediaSourcePlayer::OnDecoderStarved,

media/base/android/media_source_player.h

@@ -117,7 +117,10 @@ class MEDIA_EXPORT MediaSourcePlayer : public MediaPlayerAndroid {
   void OnDecoderStarved();
 
   // Starts the |decoder_starvation_callback_| task with the timeout value.
-  void StartStarvationCallback(const base::TimeDelta& timeout);
+  // |presentation_timestamp| - The presentation timestamp used for starvation
+  // timeout computations. It represents the timestamp of the last piece of
+  // decoded data.
+  void StartStarvationCallback(const base::TimeDelta& presentation_timestamp);
 
   // Schedules a seek event in |pending_events_| and calls StopDecode() on all
   // the MediaDecoderJobs.

media/base/android/media_source_player_unittest.cc

@@ -277,7 +277,23 @@ TEST_F(MediaSourcePlayerTest, ReadFromDemuxerAfterSeek) {
 
   // Initiate a seek
   player_->SeekTo(base::TimeDelta());
+
+  // Verify that the seek does not occur until the initial prefetch
+  // completes.
+  EXPECT_EQ(0u, manager_->last_seek_request_id());
+
+  // Simulate aborted read caused by the seek. This aborts the initial
+  // prefetch.
+  DemuxerData data;
+  data.type = DemuxerStream::AUDIO;
+  data.access_units.resize(1);
+  data.access_units[0].status = DemuxerStream::kAborted;
+  player_->ReadFromDemuxerAck(data);
+
+  // Verify that the seek is requested now that the initial prefetch
+  // has completed.
   EXPECT_EQ(1u, manager_->last_seek_request_id());
+
   // Sending back the seek ACK, this should trigger the player to call
   // OnReadFromDemuxer() again.
   player_->OnSeekRequestAck(manager_->last_seek_request_id());
@@ -436,15 +452,31 @@ TEST_F(MediaSourcePlayerTest, StartTimeTicksResetAfterDecoderUnderruns) {
 
   // The decoder job should finish and a new request will be sent.
   EXPECT_EQ(5, manager_->num_requests());
-  EXPECT_FALSE(GetMediaDecoderJob(true)->is_decoding());
+  EXPECT_TRUE(GetMediaDecoderJob(true)->is_decoding());
   base::TimeTicks previous = StartTimeTicks();
 
   // Let the decoder timeout and execute the OnDecoderStarved() callback.
   base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100));
+
+  EXPECT_TRUE(GetMediaDecoderJob(true)->is_decoding());
+  EXPECT_TRUE(StartTimeTicks() != base::TimeTicks());
   manager_->message_loop()->RunUntilIdle();
 
-  // Send new data to the decoder. This should reset the start time ticks.
-  player_->ReadFromDemuxerAck(CreateEOSAck(true));
+  // Send new data to the decoder so it can finish the currently
+  // pending decode.
+  player_->ReadFromDemuxerAck(CreateReadFromDemuxerAckForAudio(3));
+  while(GetMediaDecoderJob(true)->is_decoding())
+    manager_->message_loop()->RunUntilIdle();
+
+  // Verify the start time ticks is cleared at this point because the
+  // player is prefetching.
+  EXPECT_TRUE(StartTimeTicks() == base::TimeTicks());
+
+  // Send new data to the decoder so it can finish prefetching. This should
+  // reset the start time ticks.
+  player_->ReadFromDemuxerAck(CreateReadFromDemuxerAckForAudio(3));
+  EXPECT_TRUE(StartTimeTicks() != base::TimeTicks());
+
   base::TimeTicks current = StartTimeTicks();
   EXPECT_LE(100.0, (current - previous).InMillisecondsF());
 }