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

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/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/stringprintf.h"
@@ skipping 321 matching lines @@
               demuxer_->num_data_requests());
 
     // Verify player has decoder job iff the config included the media type for
     // the job and the player is expected to request data due to Start(), above.
     EXPECT_EQ(expect_player_requests_data && has_audio,
               GetMediaDecoderJob(true) != NULL);
     EXPECT_EQ(expect_player_requests_data && has_video,
               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;
 
     unit.status = DemuxerStream::kOk;
     scoped_refptr<DecoderBuffer> buffer = ReadTestDataFile(
         is_audio ? base::StringPrintf("vorbis-packet-%d", audio_packet_id)
             : "vp8-I-frame-320x240");
     unit.data = std::vector<uint8>(
         buffer->data(), buffer->data() + buffer->data_size());
 
@@ skipping 718 matching lines @@
   // No seeks should have occurred.
   EXPECT_EQ(0, demuxer_->num_seek_requests());
 }
 
 TEST_F(MediaSourcePlayerTest, StartTimeTicksResetAfterDecoderUnderruns) {
   SKIP_TEST_IF_MEDIA_CODEC_BRIDGE_IS_NOT_AVAILABLE();
 
   // 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
   // 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_.OnDemuxerDataAvailable(CreateReadFromDemuxerAckForAudio(3));
   EXPECT_TRUE(StartTimeTicks() != base::TimeTicks());
@@ skipping 357 matching lines @@
 TEST_F(MediaSourcePlayerTest, SeekingAfterCompletingPrerollRestartsPreroll) {
   SKIP_TEST_IF_MEDIA_CODEC_BRIDGE_IS_NOT_AVAILABLE();
 
   // Test decoder job will begin prerolling upon seek, when it was not
   // prerolling prior to the seek.
   StartAudioDecoderJob(true);
   MediaDecoderJob* decoder_job = GetMediaDecoderJob(true);
   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));
 
   // Prerolling should have begun again.
   EXPECT_TRUE(IsPrerolling(true));
   EXPECT_EQ(500.0, GetPrerollTimestamp().InMillisecondsF());
 
   // Send data at and after the seek position. Prerolling should complete.
   for (int i = 0; i < 4; ++i) {
@@ skipping 569 matching lines @@
   // player has not yet received media crypto.
   DemuxerConfigs configs = CreateVideoDemuxerConfigs();
   configs.is_video_encrypted = true;
 
   player_.OnDemuxerConfigsAvailable(configs);
   CreateNextTextureAndSetVideoSurface();
   EXPECT_FALSE(IsPendingSurfaceChange());
   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