Fix an issue that audio and video may ran out of sync

media/base/android/media_source_player_unittest.cc

Index: media/base/android/media_source_player_unittest.cc
diff --git a/media/base/android/media_source_player_unittest.cc b/media/base/android/media_source_player_unittest.cc
index 9c6a0627d1184dce97f620e96a59465a5cb0f697..fb51118571f6b24441cf2c2b9b8ca747b84a9bdf 100644
--- a/media/base/android/media_source_player_unittest.cc
+++ b/media/base/android/media_source_player_unittest.cc
@@ -339,6 +339,23 @@ class MediaSourcePlayerTest : public testing::Test {
               GetMediaDecoderJob(false) != NULL);
   }
 
+  // Keeps decoding audio data until the AudioTrack's buffer is full and the
+  // system starts to render the decoded audio.
+  // Gives up if no audio is rendered after decoding 10 frames.
+  void DecodeAudioDataUntilAudioBufferBecomesFull() {
+    EXPECT_TRUE(player_.IsPlaying());
+    base::TimeDelta start_timestamp = player_.GetCurrentTime();
+    int i;
+    for (i = 0; i < 10; ++i) {
+      player_.OnDemuxerDataAvailable(
+          CreateReadFromDemuxerAckForAudio(i > 3 ? 3 : i));
+      WaitForAudioDecodeDone();
+      if ((player_.GetCurrentTime() - start_timestamp).InMilliseconds() > 0)

[wolenetz, 2014/04/15 22:17:34]

nit: Please add a check to ensure player current time doesn't ever go backwards
here.

[qinmin, 2014/04/15 22:43:42]

Done.

+        return;
+    }
+    EXPECT_TRUE(false);
+  }
+
   AccessUnit CreateAccessUnitWithData(bool is_audio, int audio_packet_id) {
     AccessUnit unit;
 
@@ -1077,24 +1094,14 @@ TEST_F(MediaSourcePlayerTest, StartTimeTicksResetAfterDecoderUnderruns) {
   // Test start time ticks will reset after decoder job underruns.
   StartAudioDecoderJob(true);
 
-  // For the first couple chunks, the decoder job may return
-  // 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_.OnDemuxerDataAvailable(CreateReadFromDemuxerAckForAudio(i));
-    EXPECT_TRUE(GetMediaDecoderJob(true)->is_decoding());
-    // Decode data until decoder started requesting new data again.
-    WaitForAudioDecodeDone();
-  }
+  DecodeAudioDataUntilAudioBufferBecomesFull();
 
   // The decoder job should finish and a new request will be sent.
-  EXPECT_EQ(5, demuxer_->num_data_requests());
-  EXPECT_TRUE(GetMediaDecoderJob(true)->is_decoding());
   base::TimeTicks previous = StartTimeTicks();
+  player_.OnDemuxerDataAvailable(CreateReadFromDemuxerAckForAudio(3));

[wolenetz, 2014/04/15 22:17:34]

Curiousity nit: Must we feed more data prior to triggering starvation / why was
this moved prior to the trigger?

[qinmin, 2014/04/15 22:43:42]

The issue is that when AudioTrack's buffer become full, it will block the write.
Therefore, it is possible that MSP is already starved due to the block writing.
Since the starvation here is triggered purposely, it wouldn't happen in real
world if MSP is in the middle of handling a previous starvation. 


On 2014/04/15 22:17:34, wolenetz wrote:

[wolenetz, 2014/04/15 23:48:58]

Hmm. Starvation could come from two directions:
One is the decoder job just being slow or blocked as you describe. The other is
that the renderer's MediaSourceDelegate/ChunkDemuxer is late in delivering
requested new data. I believe the previous sequencing was to simulate the second
source of starvation, rather than slow MediaDecoderJob. Does it matter?

[qinmin, 2014/04/16 01:48:24]

In the new code:
A. if DecodeAudioDataUntilAudioBufferBecomesFull() triggers a starvation
1. MSP will handle the starvation by calling MDJ::Prefetch() before reaching
OnDemuxerDataAvailable().
2. OnDemuxerDataAvailable() will finish the prefetch().
3. OnPrefetchDone() called, MSP now starts to decode new data.
4. MSP got starved again from TriggerPlayerStarvation(), however, this
starvation won't be processed until the decode completes.

B. if DecodeAudioDataUntilAudioBufferBecomesFull() does not trigger a starvation
1. OnDemuxerDataAvailable() is called first.
2. MSP got starved from TriggerPlayerStarvation();
3. OnPrefetchDone() called, MSP starts to decode new data.

In the old code:
1. MSP got starved from TriggerPlayerStarvation();
2. OnDemuxerDataAvailable() is called.
3. OnPrefetchDone() called, MSP starts to decode new data.

So the difference between case A and the old code is step 4. I think we can do a
check there to see if there is a pending prefetch_done event pending. In that
case, case A and old code should be the same.
The difference between B and the old code is that whether MDJ holds the data
before starvation happens. However, MSP should never worry about the timing when
MDJ receives the data.


On 2014/04/15 23:48:58, wolenetz wrote:

 
   // Let the decoder starve.
   TriggerPlayerStarvation();
-  player_.OnDemuxerDataAvailable(CreateReadFromDemuxerAckForAudio(3));
   WaitForAudioDecodeDone();
 
   // Verify the start time ticks is cleared at this point because the
@@ -1472,12 +1479,7 @@ TEST_F(MediaSourcePlayerTest, SeekingAfterCompletingPrerollRestartsPreroll) {
   EXPECT_TRUE(IsPrerolling(true));
 
   // Complete the initial preroll by feeding data to the decoder.
-  for (int i = 0; i < 4; ++i) {
-    player_.OnDemuxerDataAvailable(CreateReadFromDemuxerAckForAudio(i));
-    EXPECT_TRUE(GetMediaDecoderJob(true)->is_decoding());
-    WaitForAudioDecodeDone();
-  }
-  EXPECT_LT(0.0, player_.GetCurrentTime().InMillisecondsF());
+  DecodeAudioDataUntilAudioBufferBecomesFull();
   EXPECT_FALSE(IsPrerolling(true));
 
   SeekPlayerWithAbort(true, base::TimeDelta::FromMilliseconds(500));
@@ -2067,4 +2069,48 @@ TEST_F(MediaSourcePlayerTest, SurfaceChangeClearedEvenIfMediaCryptoAbsent) {
   EXPECT_FALSE(GetMediaDecoderJob(false));
 }
 
+TEST_F(MediaSourcePlayerTest, CurrentTimeUpdatedWhileDecoderStarved) {
+  SKIP_TEST_IF_MEDIA_CODEC_BRIDGE_IS_NOT_AVAILABLE();
+
+  // Test that current time is updated while decoder is starved.
+  StartAudioDecoderJob(true);
+  DecodeAudioDataUntilAudioBufferBecomesFull();
+  base::TimeDelta current_time = player_.GetCurrentTime();
+
+  // Trigger starvation while the decoder is decoding.
+  player_.OnDemuxerDataAvailable(CreateReadFromDemuxerAckForAudio(3));
+  TriggerPlayerStarvation();
+  WaitForAudioDecodeDone();
+
+  // Current time should be updated.
+  EXPECT_LT(current_time.InMillisecondsF(),
+            player_.GetCurrentTime().InMillisecondsF());
+}
+
+TEST_F(MediaSourcePlayerTest, CurrentTimeKeepsIncreasingAfterConfigChange) {
+  SKIP_TEST_IF_MEDIA_CODEC_BRIDGE_IS_NOT_AVAILABLE();
+
+  // Test current time keep on increasing after audio config change.
+  // Test that current time is updated while decoder is starved.
+  StartAudioDecoderJob(true);
+
+  DecodeAudioDataUntilAudioBufferBecomesFull();
+  base::TimeDelta current_time = player_.GetCurrentTime();
+
+  DemuxerData data = CreateReadFromDemuxerAckWithConfigChanged(true, 0);
+  player_.OnDemuxerDataAvailable(data);
+  WaitForAudioDecodeDone();
+
+  // Simulate arrival of new configs.
+  player_.OnDemuxerConfigsAvailable(CreateAudioDemuxerConfigs(kCodecVorbis));
+  for (int i = 0; i < 4; ++i) {

[wolenetz, 2014/04/15 22:17:34]

nit: DecodeAudioDataUntilAudioBufferBecomesFull() here now that it uses time
delta, not just > 0?

[qinmin, 2014/04/15 22:43:42]

Done.

+    player_.OnDemuxerDataAvailable(CreateReadFromDemuxerAckForAudio(i));
+    WaitForAudioDecodeDone();
+    base::TimeDelta new_current_time = player_.GetCurrentTime();
+    EXPECT_LE(current_time.InMillisecondsF(),
+              new_current_time.InMillisecondsF());
+    current_time = new_current_time;
+  }
+}
+
 }  // namespace media