Merge 139345 - Audio in linux became choppy after some system update, I suppose one of the reasons is that previously we try to write to the ALSA with more frames than the available buffer from the snd_pcm_avail_update(), this worked fine before, but in my new test, snd_pcm_writei returns -EAGAIN. Then we

have two cases here:
#1 Part of data has been written to the ALSA.
#2 None of the data has been put into ALSA.
The first case will give us glitches because we don't update  our local buffer after the writing.
The second case may give us potential glitches depending on what we get from GetAvailableFrames().
We should avoid this by checking the snd_pcm_avail_update() and only write what we can write instead.

We also add a catch-up scheme when the available space in the buffer is more than kTargetFramesAvailable to avoid underrun.


BUG=108396
TEST=http://www.eksor.de/songs/symphonie/02-Dawning.mp3
 http://chromium.googlecode.com/svn/trunk/samples/audio/shiny-drum-machine.html demo 5

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

TBR=xians@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10541136

git-svn-id: svn://svn.chromium.org/chrome/branches/1132/src@141822 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 115 insertions, 81 deletions

diff --git a/media/audio/linux/alsa_output.cc b/media/audio/linux/alsa_output.cc
index ce301ce..3d335b2 100644
--- a/media/audio/linux/alsa_output.cc
+++ b/media/audio/linux/alsa_output.cc
@@ -55,13 +55,14 @@ namespace media {
 // busy looping.
 static const uint32 kNoDataSleepMilliseconds = 10;
 
-// Mininum interval between OnMoreData() calls.
-const uint32 kMinIntervalBetweenOnMoreDataCallsInMs = 5;
+// Mininum interval between OnMoreData() calls.  This is to avoid glitches for
+// WebAudio which needs time to generate new data.
+static const uint32 kMinIntervalBetweenOnMoreDataCallsInMs = 5;
 
 // According to the linux nanosleep manpage, nanosleep on linux can miss the
-// deadline by up to 10ms because the kernel timeslice is 10ms.  Give a 2x
-// buffer to compensate for the timeslice, and any additional slowdowns.
-static const uint32 kSleepErrorMilliseconds = 20;
+// deadline by up to 10ms because the kernel timeslice is 10ms.  This should be
+// enough to compensate for the timeslice, and any additional slowdowns.
+static const uint32 kSleepErrorMilliseconds = 10;
 
 // Set to 0 during debugging if you want error messages due to underrun
 // events or other recoverable errors.
@@ -143,10 +144,9 @@ const char AlsaPcmOutputStream::kDefaultDevice[] = "default";
 const char AlsaPcmOutputStream::kAutoSelectDevice[] = "";
 const char AlsaPcmOutputStream::kPlugPrefix[] = "plug:";
 
-// Since we expect to only be able to wake up with a resolution of
-// kSleepErrorMilliseconds, double that for our minimum required latency.
-const uint32 AlsaPcmOutputStream::kMinLatencyMicros =
-    kSleepErrorMilliseconds * 2 * 1000;
+// We use 40ms as our minimum required latency. If it is needed, we may be able
+// to get it down to 20ms.
+const uint32 AlsaPcmOutputStream::kMinLatencyMicros = 40 * 1000;
 
 AlsaPcmOutputStream::AlsaPcmOutputStream(const std::string& device_name,
                                          const AudioParameters& params,
@@ -325,7 +325,7 @@ void AlsaPcmOutputStream::Start(AudioSourceCallback* callback) {
     }
 
     if (!stop_stream_)
-      ScheduleNextWrite(false);
+      WriteTask();
   }
 }
 
@@ -381,6 +381,9 @@ void AlsaPcmOutputStream::BufferPacket(bool* source_exhausted) {
                                                         hardware_delay));
     CHECK_LE(packet_size, packet->GetBufferSize());
 
+    // Reset the |last_fill_time| to avoid back to back RunDataCallback().
+    last_fill_time_ = base::Time::Now();
+
     // This should not happen, but in case it does, drop any trailing bytes
     // that aren't large enough to make a frame.  Without this, packet writing
     // may stall because the last few bytes in the packet may never get used by
@@ -435,9 +438,9 @@ void AlsaPcmOutputStream::WritePacket() {
   int buffer_size;
   if (buffer_->GetCurrentChunk(&buffer_data, &buffer_size)) {
     buffer_size = buffer_size - (buffer_size % bytes_per_output_frame_);
-    snd_pcm_sframes_t frames = buffer_size / bytes_per_output_frame_;
-
-    DCHECK_GT(frames, 0);
+    snd_pcm_sframes_t frames = std::min(
+        static_cast<snd_pcm_sframes_t>(buffer_size / bytes_per_output_frame_),
+        GetAvailableFrames());
 
     snd_pcm_sframes_t frames_written =
         wrapper_->PcmWritei(playback_handle_, buffer_data, frames);
@@ -449,23 +452,16 @@ void AlsaPcmOutputStream::WritePacket() {
       frames_written = wrapper_->PcmRecover(playback_handle_,
                                             frames_written,
                                             kPcmRecoverIsSilent);
-    }
-
-    if (frames_written < 0) {
-      // TODO(ajwong): Is EAGAIN the only error we want to except from stopping
-      // the pcm playback?
-      if (frames_written != -EAGAIN) {
-        LOG(ERROR) << "Failed to write to pcm device: "
-                   << wrapper_->StrError(frames_written);
-        RunErrorCallback(frames_written);
-        stop_stream_ = true;
+      if (frames_written < 0) {
+        if (frames_written != -EAGAIN) {
+          LOG(ERROR) << "Failed to write to pcm device: "
+                     << wrapper_->StrError(frames_written);
+          RunErrorCallback(frames_written);
+          stop_stream_ = true;
+        }
       }
     } else {
-      if (frames_written > frames) {
-        LOG(WARNING)
-            << "snd_pcm_writei() has written more frame that we asked.";
-        frames_written = frames;
-      }
+      DCHECK_EQ(frames_written, frames);
 
       // Seek forward in the buffer after we've written some data to ALSA.
       buffer_->Seek(frames_written * bytes_per_output_frame_);
@@ -503,7 +499,7 @@ void AlsaPcmOutputStream::ScheduleNextWrite(bool source_exhausted) {
   if (stop_stream_)
     return;
 
-  uint32 frames_avail_wanted = alsa_buffer_frames_ / 2;
+  const uint32 kTargetFramesAvailable = alsa_buffer_frames_ / 2;
   uint32 available_frames = GetAvailableFrames();
   uint32 frames_in_buffer = buffer_->forward_bytes() / bytes_per_output_frame_;
 
@@ -512,29 +508,38 @@ void AlsaPcmOutputStream::ScheduleNextWrite(bool source_exhausted) {
   uint32 next_fill_time_ms =
       FramesToMillis(frames_per_packet_ / 2, sample_rate_);
 
-  if (frames_in_buffer && (frames_in_buffer <= available_frames)) {
-    // There is data in the current buffer, consume them immediately if we have
-    // enough space in the soundcard.
-    next_fill_time_ms = 0;
+  if (frames_in_buffer && available_frames) {
+    // There is data in the current buffer, consume them immediately once we
+    // have enough space in the soundcard.
+    if (frames_in_buffer <= available_frames)
+      next_fill_time_ms = 0;
   } else {
-    // Otherwise schedule the next write for the moment when half of the alsa
-    // buffer becomes available.
-    if (available_frames < frames_avail_wanted) {
-      uint32 frames_until_empty_enough = frames_avail_wanted - available_frames;
+    // Otherwise schedule the next write for the moment when the available
+    // buffer of the soundcards hits the |kTargetFramesAvailable|.
+    if (available_frames < kTargetFramesAvailable) {
+      uint32 frames_until_empty_enough =
+          kTargetFramesAvailable - available_frames;
       next_fill_time_ms =
           FramesToMillis(frames_until_empty_enough, sample_rate_);
 
-      // Adjust for time resolution.
-      if (next_fill_time_ms > kNoDataSleepMilliseconds)
-        next_fill_time_ms -= kNoDataSleepMilliseconds;
-
-      // Avoid back-to-back writing.
-      if (next_fill_time_ms < kMinIntervalBetweenOnMoreDataCallsInMs)
-        next_fill_time_ms = kMinIntervalBetweenOnMoreDataCallsInMs;
-    } else if (available_frames == alsa_buffer_frames_) {
-      // Buffer is empty, invoke next write immediately.
+      // Adjust for the kernel timeslice and any additional slowdown.
+      // TODO(xians): Remove this adjustment if it is not required by
+      // low performance machines any more.
+      if (next_fill_time_ms > kSleepErrorMilliseconds)
+        next_fill_time_ms -= kSleepErrorMilliseconds;
+      else
+        next_fill_time_ms = 0;
+    } else {
+      // The sound card has |kTargetFramesAvailable| or more frames available.
+      // Invoke the next write immediately to avoid underrun.
       next_fill_time_ms = 0;
     }
+
+    // Avoid back-to-back writing.
+    base::TimeDelta delay = base::Time::Now() - last_fill_time_;
+    if (delay.InMilliseconds() < kMinIntervalBetweenOnMoreDataCallsInMs &&
+        next_fill_time_ms < kMinIntervalBetweenOnMoreDataCallsInMs)
+      next_fill_time_ms = kMinIntervalBetweenOnMoreDataCallsInMs;
   }
 
   // Avoid busy looping if the data source is exhausted.
@@ -543,18 +548,11 @@ void AlsaPcmOutputStream::ScheduleNextWrite(bool source_exhausted) {
 
   // Only schedule more reads/writes if we are still in the playing state.
   if (state() == kIsPlaying) {
-    if (next_fill_time_ms == 0) {
-      manager_->GetMessageLoop()->PostTask(FROM_HERE, base::Bind(
-          &AlsaPcmOutputStream::WriteTask, weak_factory_.GetWeakPtr()));
-    } else {
-      // TODO(ajwong): Measure the reliability of the delay interval.  Use
-      // base/metrics/histogram.h.
-      manager_->GetMessageLoop()->PostDelayedTask(
-          FROM_HERE,
-          base::Bind(&AlsaPcmOutputStream::WriteTask,
-                     weak_factory_.GetWeakPtr()),
-          base::TimeDelta::FromMilliseconds(next_fill_time_ms));
-    }
+    manager_->GetMessageLoop()->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&AlsaPcmOutputStream::WriteTask,
+                   weak_factory_.GetWeakPtr()),
+                   base::TimeDelta::FromMilliseconds(next_fill_time_ms));
   }
 }
 
diff --git a/media/audio/linux/alsa_output.h b/media/audio/linux/alsa_output.h
index 43e75bb..e11d6ec 100644
--- a/media/audio/linux/alsa_output.h
+++ b/media/audio/linux/alsa_output.h
@@ -29,6 +29,7 @@
 #include "base/gtest_prod_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
+#include "base/time.h"
 #include "media/audio/audio_io.h"
 #include "media/audio/audio_parameters.h"
 
@@ -204,6 +205,8 @@ class MEDIA_EXPORT AlsaPcmOutputStream : public AudioOutputStream {
 
   AudioSourceCallback* source_callback_;
 
+  base::Time last_fill_time_;  // Time for the last OnMoreData() callback.
+
   DISALLOW_COPY_AND_ASSIGN(AlsaPcmOutputStream);
 };
 
diff --git a/media/audio/linux/alsa_output_unittest.cc b/media/audio/linux/alsa_output_unittest.cc
index 4b314ce..9e23543 100644
--- a/media/audio/linux/alsa_output_unittest.cc
+++ b/media/audio/linux/alsa_output_unittest.cc
@@ -434,26 +434,18 @@ TEST_F(AlsaPcmOutputStreamTest, StartStop) {
   // Expect the pre-roll.
   MockAudioSourceCallback mock_callback;
   EXPECT_CALL(mock_alsa_wrapper_, PcmState(kFakeHandle))
-      .Times(3)
       .WillRepeatedly(Return(SND_PCM_STATE_RUNNING));
   EXPECT_CALL(mock_alsa_wrapper_, PcmDelay(kFakeHandle, _))
-      .Times(2)
       .WillRepeatedly(DoAll(SetArgumentPointee<1>(0), Return(0)));
   EXPECT_CALL(mock_callback, OnMoreData(_, kTestPacketSize, _))
-      .Times(2)
-      .WillOnce(Return(kTestPacketSize))
-      .WillOnce(Return(0));
+      .WillRepeatedly(Return(kTestPacketSize));
   EXPECT_CALL(mock_alsa_wrapper_, PcmWritei(kFakeHandle, _, _))
-      .WillOnce(Return(kTestFramesPerPacket));
+      .WillRepeatedly(Return(kTestFramesPerPacket));
 
   // Expect scheduling.
   EXPECT_CALL(mock_alsa_wrapper_, PcmAvailUpdate(kFakeHandle))
-      .Times(AtLeast(3))
-      .WillOnce(Return(kTestFramesPerPacket))  // Buffer is empty.
-      .WillOnce(Return(kTestFramesPerPacket))
-      .WillRepeatedly(DoAll(InvokeWithoutArgs(&message_loop_,
-                                              &MessageLoop::QuitNow),
-                                              Return(0)));  // Buffer is full.
+      .Times(AtLeast(2))
+      .WillRepeatedly(Return(kTestFramesPerPacket));
 
   test_stream->Start(&mock_callback);
   message_loop_.RunAllPending();
@@ -479,12 +471,25 @@ TEST_F(AlsaPcmOutputStreamTest, WritePacket_FinishedPacket) {
 }
 
 TEST_F(AlsaPcmOutputStreamTest, WritePacket_NormalPacket) {
+  // We need to open the stream before writing data to ALSA.
+  EXPECT_CALL(mock_alsa_wrapper_, PcmOpen(_, _, _, _))
+      .WillOnce(DoAll(SetArgumentPointee<0>(kFakeHandle),
+                      Return(0)));
+  EXPECT_CALL(mock_alsa_wrapper_, PcmSetParams(_, _, _, _, _, _, _))
+      .WillOnce(Return(0));
+  EXPECT_CALL(mock_alsa_wrapper_, PcmGetParams(_, _, _))
+      .WillOnce(DoAll(SetArgumentPointee<1>(kTestFramesPerPacket),
+                      SetArgumentPointee<2>(kTestFramesPerPacket / 2),
+                      Return(0)));
   AlsaPcmOutputStream* test_stream = CreateStream(kTestChannelLayout);
+  ASSERT_TRUE(test_stream->Open());
   InitBuffer(test_stream);
 
   // Write a little less than half the data.
   int written = packet_->GetDataSize() / kTestBytesPerFrame / 2 - 1;
-  EXPECT_CALL(mock_alsa_wrapper_, PcmWritei(_, packet_->GetData(), _))
+  EXPECT_CALL(mock_alsa_wrapper_, PcmAvailUpdate(kFakeHandle))
+        .WillOnce(Return(written));
+  EXPECT_CALL(mock_alsa_wrapper_, PcmWritei(kFakeHandle, packet_->GetData(), _))
       .WillOnce(Return(written));
 
   test_stream->WritePacket();
@@ -493,42 +498,70 @@ TEST_F(AlsaPcmOutputStreamTest, WritePacket_NormalPacket) {
             packet_->GetDataSize() - written * kTestBytesPerFrame);
 
   // Write the rest.
+  EXPECT_CALL(mock_alsa_wrapper_, PcmAvailUpdate(kFakeHandle))
+      .WillOnce(Return(kTestFramesPerPacket - written));
   EXPECT_CALL(mock_alsa_wrapper_,
-              PcmWritei(_, packet_->GetData() + written * kTestBytesPerFrame,
+              PcmWritei(kFakeHandle,
+                        packet_->GetData() + written * kTestBytesPerFrame,
                         _))
       .WillOnce(Return(packet_->GetDataSize() / kTestBytesPerFrame - written));
   test_stream->WritePacket();
   EXPECT_EQ(0, test_stream->buffer_->forward_bytes());
+
+  // Now close it and test that everything was released.
+  EXPECT_CALL(mock_alsa_wrapper_, PcmClose(kFakeHandle))
+      .WillOnce(Return(0));
+  EXPECT_CALL(mock_alsa_wrapper_, PcmName(kFakeHandle))
+      .WillOnce(Return(kTestDeviceName));
   test_stream->Close();
 }
 
 TEST_F(AlsaPcmOutputStreamTest, WritePacket_WriteFails) {
+  // We need to open the stream before writing data to ALSA.
+  EXPECT_CALL(mock_alsa_wrapper_, PcmOpen(_, _, _, _))
+      .WillOnce(DoAll(SetArgumentPointee<0>(kFakeHandle),
+                      Return(0)));
+  EXPECT_CALL(mock_alsa_wrapper_, PcmSetParams(_, _, _, _, _, _, _))
+      .WillOnce(Return(0));
+  EXPECT_CALL(mock_alsa_wrapper_, PcmGetParams(_, _, _))
+      .WillOnce(DoAll(SetArgumentPointee<1>(kTestFramesPerPacket),
+                      SetArgumentPointee<2>(kTestFramesPerPacket / 2),
+                      Return(0)));
   AlsaPcmOutputStream* test_stream = CreateStream(kTestChannelLayout);
+  ASSERT_TRUE(test_stream->Open());
   InitBuffer(test_stream);
 
   // Fail due to a recoverable error and see that PcmRecover code path
   // continues normally.
-  EXPECT_CALL(mock_alsa_wrapper_, PcmWritei(_, _, _))
+  EXPECT_CALL(mock_alsa_wrapper_, PcmAvailUpdate(kFakeHandle))
+      .WillOnce(Return(kTestFramesPerPacket));
+  EXPECT_CALL(mock_alsa_wrapper_, PcmWritei(kFakeHandle, _, _))
       .WillOnce(Return(-EINTR));
-  EXPECT_CALL(mock_alsa_wrapper_, PcmRecover(_, _, _))
-      .WillOnce(Return(packet_->GetDataSize() / kTestBytesPerFrame / 2 - 1));
+  EXPECT_CALL(mock_alsa_wrapper_, PcmRecover(kFakeHandle, _, _))
+      .WillOnce(Return(0));
 
   test_stream->WritePacket();
 
-  ASSERT_EQ(test_stream->buffer_->forward_bytes(),
-            packet_->GetDataSize() / 2 + kTestBytesPerFrame);
+  ASSERT_EQ(test_stream->buffer_->forward_bytes(), packet_->GetDataSize());
 
   // Fail the next write, and see that stop_stream_ is set.
-  EXPECT_CALL(mock_alsa_wrapper_, PcmWritei(_, _, _))
+  EXPECT_CALL(mock_alsa_wrapper_, PcmAvailUpdate(kFakeHandle))
+        .WillOnce(Return(kTestFramesPerPacket));
+  EXPECT_CALL(mock_alsa_wrapper_, PcmWritei(kFakeHandle, _, _))
       .WillOnce(Return(kTestFailedErrno));
-  EXPECT_CALL(mock_alsa_wrapper_, PcmRecover(_, _, _))
+  EXPECT_CALL(mock_alsa_wrapper_, PcmRecover(kFakeHandle, _, _))
       .WillOnce(Return(kTestFailedErrno));
   EXPECT_CALL(mock_alsa_wrapper_, StrError(kTestFailedErrno))
       .WillOnce(Return(kDummyMessage));
   test_stream->WritePacket();
-  EXPECT_EQ(test_stream->buffer_->forward_bytes(),
-            packet_->GetDataSize() / 2 + kTestBytesPerFrame);
+  EXPECT_EQ(test_stream->buffer_->forward_bytes(), packet_->GetDataSize());
   EXPECT_TRUE(test_stream->stop_stream_);
+
+  // Now close it and test that everything was released.
+  EXPECT_CALL(mock_alsa_wrapper_, PcmClose(kFakeHandle))
+      .WillOnce(Return(0));
+  EXPECT_CALL(mock_alsa_wrapper_, PcmName(kFakeHandle))
+      .WillOnce(Return(kTestDeviceName));
   test_stream->Close();
 }