Fix MediaSourcePlayer unittests and minor code cleanup.

media/base/android/media_source_player_unittest.cc

 // Copyright 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 <string>
 
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/stringprintf.h"
 #include "media/base/android/media_codec_bridge.h"
@@ skipping 250 matching lines @@
   if (!MediaCodecBridge::IsAvailable())
     return;
 
   // Test decoder job will resend a ReadFromDemuxer request after seek.
   StartAudioDecoderJob();
   EXPECT_TRUE(NULL != GetMediaDecoderJob(true));
   EXPECT_EQ(1, manager_->num_requests());
 
   // 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);

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

The new implementation always expects to hear back from the demuxer. The old
implementation didn't wait for pending reads to complete, which I believe could
have triggered buggy behavior if multiple seeks were in flight.

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

Previously, if a ReadFromDemuxerAck comes back while seek ack has not arrived,
we ignore the ReadFromDemuxerAck. Since read and seek are sent to the same
thread in render process, the ack should also come back in order, unless
MediaSourceDelegate won't cancel a read when a seek is requested.
If the new behavior requires that we always wait for ReadFromDemuxerAck before
we can send out a seek， it seems that we should remove the seek_request_id, if
all the seeks are acked

On 2013/08/30 18:27:38, acolwell wrote:

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

Unfortunately, I can't remove the ID yet. Multiple seeks can still be in flight.
This only handles the case where the seek occurs while the initial prefetch is
happening. If the prefetch is done, but a second seek is requested before a
previous one completes, we still need to use the ID to sort out which one is the
"last" one. I believe Matt is working on a CL that will make the multiple
pending seek situation go away, but until that happens we still need the id.

+
+  // 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());
   EXPECT_EQ(2, manager_->num_requests());
 }
 
 TEST_F(MediaSourcePlayerTest, SetSurfaceWhileSeeking) {
   if (!MediaCodecBridge::IsAvailable())
     return;
 
@@ skipping 138 matching lines @@
   // DECODE_FORMAT_CHANGED status instead of DECODE_SUCCEEDED status. Decode
   // more frames to guarantee that DECODE_SUCCEEDED will be returned.
   for (int i = 0; i < 4; ++i) {
     player_->ReadFromDemuxerAck(CreateReadFromDemuxerAckForAudio(i));
     EXPECT_TRUE(GetMediaDecoderJob(true)->is_decoding());
     manager_->message_loop()->Run();
   }
 
   // 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());
+  DCHECK(StartTimeTicks() != base::TimeTicks());

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

EXPECT_TRUE？ Same for all the DCHECKs below.

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

Done.

   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));

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

The new implementation has to complete the pending decode before it is able to
prefetch. In the case of this test the decoder job is already waiting for a read
so technically the prefetch could be fulfilled by that pending read. For now I
have just updated the test to capture the current behavior. I plan to follow up
this change with one that tries to avoid the need for an extra read for the
prefetch.

+  while(GetMediaDecoderJob(true)->is_decoding())
+    manager_->message_loop()->RunUntilIdle();
+
+  // Verify the start time ticks is cleared at this point because the
+  // player is prefetching.
+  DCHECK(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));
+  DCHECK(StartTimeTicks() != base::TimeTicks());
+
   base::TimeTicks current = StartTimeTicks();
   EXPECT_LE(100.0, (current - previous).InMillisecondsF());
 }
 
 TEST_F(MediaSourcePlayerTest, NoRequestForDataAfterInputEOS) {
   if (!MediaCodecBridge::IsAvailable())
     return;
 
   // Test MediaSourcePlayer will not request for new data after input EOS is
   // reached.
@@ skipping 10 matching lines @@
   EXPECT_EQ(2, manager_->num_requests());
 
   // Send EOS.
   player_->ReadFromDemuxerAck(CreateEOSAck(false));
   manager_->message_loop()->Run();
   // No more request for data should be made.
   EXPECT_EQ(2, manager_->num_requests());
 }
 
 }  // namespace media