Change the way audio mixer gets "pending bytes" (amount of data currently buffered

but not yet played). Underlying code expects per-logical-stream pending bytes,
while all mixer gets when called for more data is pending bytes per combined stream.

Fix is to keep track of
* amount of data in every buffer
* buffers for every logical stream
and manually calculate per-logical-stream pending bytes.

That is last CL in initial audio mixer implementation, after it go through mixer
should be ready for full testing.

Review URL: http://codereview.chromium.org/10154007

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

3 files changed, 56 insertions, 27 deletions

diff --git a/media/audio/audio_output_mixer.cc b/media/audio/audio_output_mixer.cc
index c4537d1..e0dfe37 100644
--- a/media/audio/audio_output_mixer.cc
+++ b/media/audio/audio_output_mixer.cc
@@ -24,7 +24,8 @@ AudioOutputMixer::AudioOutputMixer(AudioManager* audio_manager,
       close_timer_(FROM_HERE,
                    close_delay,
                    weak_this_.GetWeakPtr(),
-                   &AudioOutputMixer::ClosePhysicalStream) {
+                   &AudioOutputMixer::ClosePhysicalStream),
+      pending_bytes_(0) {
   // TODO(enal): align data.
   mixer_data_.reset(new uint8[params_.GetBytesPerBuffer()]);
 }
@@ -44,6 +45,7 @@ bool AudioOutputMixer::OpenStream() {
     stream->Close();
     return false;
   }
+  pending_bytes_ = 0;  // Just in case.
   physical_stream_.reset(stream);
   close_timer_.Reset();
   return true;
@@ -96,8 +98,10 @@ void AudioOutputMixer::StopStream(AudioOutputProxy* stream_proxy) {
     }
   }
   if (physical_stream_.get()) {
-    if (stop_physical_stream)
+    if (stop_physical_stream) {
       physical_stream_->Stop();
+      pending_bytes_ = 0;  // Just in case.
+    }
     close_timer_.Reset();
   }
 }
@@ -155,8 +159,12 @@ uint32 AudioOutputMixer::OnMoreData(uint8* dest,
   //             at the end. That would speed things up but complicate stopping
   //             the stream.
   base::AutoLock lock(lock_);
-  if (proxies_.empty())
+
+  DCHECK_GE(pending_bytes_, buffers_state.pending_bytes);
+  if (proxies_.empty()) {
+    pending_bytes_ = buffers_state.pending_bytes;
     return 0;
+  }
   uint32 actual_total_size = 0;
   uint32 bytes_per_sample = params_.bits_per_sample() >> 3;
 
@@ -169,36 +177,23 @@ uint32 AudioOutputMixer::OnMoreData(uint8* dest,
   uint8* actual_dest = dest;
   for (ProxyMap::iterator it = proxies_.begin(); it != proxies_.end(); ++it) {
     ProxyData* proxy_data = &it->second;
-    // TODO(enal): We don't know |pending _bytes| for individual stream, and we
-    // should give that value to individual stream's OnMoreData(). I believe it
-    // can be used there to evaluate exact position of data it should return.
-    // Current code "sorta works" if everything works perfectly, but would have
-    // problems if some of the buffers are only partially filled -- we don't
-    // know how how much data was in the buffer OS returned to us, so we cannot
-    // correctly calculate new value. If we know number of buffers we can solve
-    // the problem by storing not one value but vector of them.
-    int pending_bytes = std::min(proxy_data->pending_bytes,
-                                 buffers_state.pending_bytes);
+
+    // If proxy's pending bytes are the same as pending bytes for combined
+    // stream, both are either pre-buffering or in the steady state. In either
+    // case new pending bytes for proxy is the same as new pending bytes for
+    // combined stream.
+    // Note: use >= instead of ==, that way is safer.
+    if (proxy_data->pending_bytes >= pending_bytes_)
+      proxy_data->pending_bytes = buffers_state.pending_bytes;
+
     // Note: there is no way we can deduce hardware_delay_bytes for the
     // particular proxy stream. Use zero instead.
     uint32 actual_size = proxy_data->audio_source_callback->OnMoreData(
         actual_dest,
         max_size,
-        AudioBuffersState(pending_bytes, 0));
-
-    // Should update pending_bytes for each proxy.
-    // If stream ended, pending_bytes goes down by max_size.
-    if (actual_size == 0) {
-      pending_bytes -= max_size;
-      proxy_data->pending_bytes = std::max(pending_bytes, 0);
+        AudioBuffersState(proxy_data->pending_bytes, 0));
+    if (actual_size == 0)
       continue;
-    }
-
-    // Otherwise, it goes up by amount of data. It cannot exceed max amount of
-    // data we can buffer, but we don't know that value. So we increment
-    // pending_bytes unconditionally but adjust it before actual use (which
-    // would be on a next OnMoreData() call).
-    proxy_data->pending_bytes = pending_bytes + actual_size;
 
     // No need to mix muted stream.
     double volume = proxy_data->volume;
@@ -228,6 +223,15 @@ uint32 AudioOutputMixer::OnMoreData(uint8* dest,
       actual_total_size = std::max(actual_size, actual_total_size);
     }
   }
+
+  // Now go through all proxies once again and increase pending_bytes
+  // for each proxy. Could not do it earlier because we did not know
+  // actual_total_size.
+  for (ProxyMap::iterator it = proxies_.begin(); it != proxies_.end(); ++it) {
+    it->second.pending_bytes += actual_total_size;
+  }
+  pending_bytes_ = buffers_state.pending_bytes + actual_total_size;
+
   return actual_total_size;
 }
 
diff --git a/media/audio/audio_output_mixer.h b/media/audio/audio_output_mixer.h
index 532f27e..4ddeeef 100644
--- a/media/audio/audio_output_mixer.h
+++ b/media/audio/audio_output_mixer.h
@@ -83,6 +83,9 @@ class MEDIA_EXPORT AudioOutputMixer
   base::WeakPtrFactory<AudioOutputMixer> weak_this_;
   base::DelayTimer<AudioOutputMixer> close_timer_;
 
+  // Size of data in all in-flight buffers.
+  int pending_bytes_;
+
   DISALLOW_COPY_AND_ASSIGN(AudioOutputMixer);
 };
 
diff --git a/media/audio/audio_output_proxy_unittest.cc b/media/audio/audio_output_proxy_unittest.cc
index 674c200..7f85acc 100644
--- a/media/audio/audio_output_proxy_unittest.cc
+++ b/media/audio/audio_output_proxy_unittest.cc
@@ -15,6 +15,8 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::_;
+using ::testing::AllOf;
+using ::testing::Field;
 using ::testing::Mock;
 using ::testing::Return;
 using media::AudioBuffersState;
@@ -434,6 +436,7 @@ TEST_F(AudioOutputProxyTest, TwoStreams_BothPlaying) {
 }
 
 // Two streams, both are playing. Still have to use single device.
+// Also verifies that every proxy stream gets its own pending_bytes.
 TEST_F(AudioOutputProxyTest, TwoStreams_BothPlaying_Mixer) {
   MockAudioOutputStream stream;
 
@@ -459,7 +462,26 @@ TEST_F(AudioOutputProxyTest, TwoStreams_BothPlaying_Mixer) {
   EXPECT_TRUE(proxy2->Open());
 
   proxy1->Start(&callback_);
+  uint8 buf1[4] = {0};
+  EXPECT_CALL(callback_,
+      OnMoreData(_, 4,
+                 AllOf(Field(&AudioBuffersState::pending_bytes, 0),
+                       Field(&AudioBuffersState::hardware_delay_bytes, 0))))
+      .WillOnce(Return(4));
+  mixer_->OnMoreData(buf1, sizeof(buf1), AudioBuffersState(0, 0));
   proxy2->Start(&callback_);
+  uint8 buf2[4] = {0};
+  EXPECT_CALL(callback_,
+      OnMoreData(_, 4,
+                 AllOf(Field(&AudioBuffersState::pending_bytes, 4),
+                       Field(&AudioBuffersState::hardware_delay_bytes, 0))))
+      .WillOnce(Return(4));
+  EXPECT_CALL(callback_,
+      OnMoreData(_, 4,
+                 AllOf(Field(&AudioBuffersState::pending_bytes, 0),
+                       Field(&AudioBuffersState::hardware_delay_bytes, 0))))
+      .WillOnce(Return(4));
+  mixer_->OnMoreData(buf2, sizeof(buf2), AudioBuffersState(4, 0));
   proxy1->Stop();
   proxy2->Stop();