Audio software mixer.

media/audio/audio_output_mixer.cc

Index: media/audio/audio_output_mixer.cc
===================================================================
--- media/audio/audio_output_mixer.cc	(revision 0)
+++ media/audio/audio_output_mixer.cc	(revision 0)
@@ -0,0 +1,231 @@
+// Copyright (c) 2012 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 "media/audio/audio_output_mixer.h"
+
+#include <algorithm>
+
+#include "base/bind.h"
+#include "base/compiler_specific.h"
+#include "base/message_loop.h"
+#include "base/time.h"
+#include "media/audio/audio_io.h"
+#include "media/audio/audio_output_proxy.h"
+#include "media/audio/audio_util.h"
+
+AudioOutputMixer::AudioOutputMixer(AudioManager* audio_manager,
+                                   const AudioParameters& params,
+                                   const base::TimeDelta& close_delay)
+    : AudioOutputDispatcher(audio_manager, params),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_this_(this)),
+      close_timer_(FROM_HERE,
+                   close_delay,
+                   weak_this_.GetWeakPtr(),
+                   &AudioOutputMixer::ClosePhysicalStream) {
+  // TODO(enal): align data.
+  mixer_data_.reset(new uint8[params_.GetBytesPerBuffer()]);
+}
+
+AudioOutputMixer::~AudioOutputMixer() {
+}
+
+bool AudioOutputMixer::OpenStream() {
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
+
+  if (physical_stream_.get())
+    return true;
+  AudioOutputStream* stream = audio_manager_->MakeAudioOutputStream(params_);
+  if (!stream)
+    return false;
+  if (!stream->Open()) {
+    stream->Close();
+    return false;
+  }
+  physical_stream_.reset(stream);
+  close_timer_.Reset();
+  return true;
+}
+
+bool AudioOutputMixer::StartStream(
+    AudioOutputStream::AudioSourceCallback* callback,
+    AudioOutputProxy* stream_proxy) {
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
+
+  // May need to re-open the physical stream if no active proxies and
+  // enough time had pass.
+  OpenStream();
+  if (!physical_stream_.get())
+    return false;
+
+  bool should_start = proxies_.empty();
+  {
+    base::AutoLock lock(lock_);
+    proxies_[stream_proxy].pending_bytes = 0;
+  }
+  // We cannot start physical stream under the lock,

[vrk (LEFT CHROMIUM), 2012/04/06 23:37:05]

I don't understand this problem. Start() should not make a blocking call to
OnMoreData(), right? Isn't OnMoreData() called on a different thread, meaning
Start() would have to explicitly block for this deadlock to occur?

Can you update the audio_output_mixer.h with an explanation of the threading
story in this class?

[enal1, 2012/04/16 22:01:35]

Not right. Starting stream will call OnMoreData() to fill initial buffers on the
thread Start() is called. Exact number of buffers depends on the operating
system and type of the stream (normal/low latency).
There are very good descriptions for both interfaces it implements,
AudioOutputDispatcher and AudioSourceCallback, in corresponding files. I prefer
not to duplicate the comments, it's hard to keep even single instance of
comments in sync with code...

[vrk (LEFT CHROMIUM), 2012/04/18 14:57:36]

Ah, my mistake! I was only looking at the linux implementation; I see now that
the windows implementation does exactly what you say.
SGTM.

+  // OnMoreData() would try acquiring it...
+  if (should_start) {
+    physical_stream_->SetVolume(1.0);
+    physical_stream_->Start(this);
+  }
+  return true;
+}
+
+void AudioOutputMixer::StopStream(AudioOutputProxy* stream_proxy) {
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
+
+  // Because of possible deadlock we cannot stop physical stream

[vrk (LEFT CHROMIUM), 2012/04/06 23:37:05]

Again, what is the deadlock you're worried about here?

[enal1, 2012/04/16 22:01:35]

Updated comment.

+  // under the lock, so acquire the lock, update proxy list, release the lock,
+  // and only then stop physical stream if necessary.
+  bool stop_physical_stream = false;
+  {
+    base::AutoLock lock(lock_);
+    ProxyMap::iterator it = proxies_.find(stream_proxy);
+    if (it != proxies_.end()) {
+      proxies_.erase(it);
+      stop_physical_stream = proxies_.empty();
+    }
+  }
+  if (physical_stream_.get()) {
+    if (stop_physical_stream)
+      physical_stream_->Stop();
+    close_timer_.Reset();
+  }
+}
+
+void AudioOutputMixer::StreamVolumeSet(AudioOutputProxy* stream_proxy,
+                                       double volume) {
+}
+
+void AudioOutputMixer::CloseStream(AudioOutputProxy* stream_proxy) {
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
+  StopStream(stream_proxy);
+}
+
+void AudioOutputMixer::Shutdown() {
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
+
+  // Cancel any pending tasks to close physical stream.
+  weak_this_.InvalidateWeakPtrs();
+
+  while (!proxies_.empty()) {
+    StopStream(proxies_.begin()->first);

[vrk (LEFT CHROMIUM), 2012/04/06 23:37:05]

nit: I don't like this because it assumes StopStream() is going to delete the
given stream from the list. Instead, make a copy of the list and stop() on all
the copies, then clear |proxies_|?

[enal1, 2012/04/16 22:01:35]

Updated comment in the header file, now it says that |proxies_| contains only
currently playing streams.

+  }
+  ClosePhysicalStream();
+
+  // No AudioOutputProxy objects should hold a reference to us when we get
+  // to this stage.
+  DCHECK(HasOneRef()) << "Only the AudioManager should hold a reference";
+}
+
+void AudioOutputMixer::ClosePhysicalStream() {
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
+  if (proxies_.empty() && physical_stream_.get() != NULL)
+    physical_stream_.release()->Close();
+}
+
+// AudioSourceCallback implementation.
+uint32 AudioOutputMixer::OnMoreData(AudioOutputStream* stream,

[vrk (LEFT CHROMIUM), 2012/04/06 23:37:05]

What is the first parameter of OnMoreData() used for? Can you get rid of it?

[enal1, 2012/04/16 22:01:35]

It is not used, and I am planning to get rid of it, but it requires changes in
two dozen files or so, so it should be separate CL.

+                                    uint8* dest,
+                                    uint32 max_size,
+                                    AudioBuffersState buffers_state) {
+  max_size = std::min(max_size,
+                      static_cast<uint32>(params_.GetBytesPerBuffer()));
+  // TODO(enal): consider getting rid of lock as it is in time-critical code.
+  //             E.g. swap |proxies_| with local variable, and merge 2 lists
+  //             at the end. That would speed things up but complicate stopping
+  //             the stream.
+  base::AutoLock lock(lock_);
+  if (proxies_.empty())
+    return 0;
+  uint32 actual_total_size = 0;
+  uint32 bytes_per_sample = params_.bits_per_sample() >> 3;
+
+  // Go through all the streams, getting data for every one of them
+  // and mixing it into destination.
+  // Minor optimization: for the first stream we are writing data directly into
+  // destination. This way we don't have to mix the data when there is only one
+  // active stream, and net win in other cases, too.
+  bool first_stream = true;
+  uint8* actual_dest = dest;
+  for (ProxyMap::iterator it = proxies_.begin(); it != proxies_.end(); ++it) {
+    AudioOutputProxy* stream_proxy = it->first;
+    ProxyData* proxy_data = &it->second;
+    // TODO(enal): We don't know |pending _bytes| for individual stream, and we

[vrk (LEFT CHROMIUM), 2012/04/06 23:37:05]

The pending bytes problem is a tricky one indeed... I'm worried about how the
"ended" event will be fired for each of these streams. Have you checked how the
ended event is fired for several short streams mixed into a large stream?

[enal1, 2012/04/16 22:01:35]

Yes, I checked that.

+    // should givw 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

[vrk (LEFT CHROMIUM), 2012/04/06 23:37:05]

(sorry in advance for this huge comment, but I can't think of a better way to
explain!)

I know you're saying it only "sorta works" but I think we can get a better
estimate. This is my understanding of how the current code would work:

stream 1: 57 bytes long
stream 2: 200 bytes long
max_size: 50 bytes

ITERATION ONE: 
OnMoreData: buffers_state.pending_bytes = 0
stream 1:
  pending_bytes = min(0, 0) = 0
  actual_size = 50
  proxy_data->pending_bytes = 0 + 50 = 50
stream 2:
  pending_bytes = min(0, 0) = 0
  actual_size = 50
  proxy_data->pending_bytes = 0 + 50 = 50

ITERATION TWO: 
OnMoreData: buffers_state.pending_bytes = 30
stream 1:
  pending_bytes = min(50, 30) = 30
  actual_size = 7
  proxy_data->pending_bytes = 30 + 7 = 37
stream 2:
  pending_bytes = min(50, 30) = 30
  actual_size = 50
  proxy_data->pending_bytes = 30 + 50 = 80

ITERATION THREE: 
OnMoreData: buffers_state.pending_bytes = 60
stream 1:
  pending_bytes = min(37, 60) = 30
  actual_size = 0
  proxy_data->pending_bytes = max(30 - 50, 0) = 0   ******
stream 2:
  pending_bytes = min(80, 60) = 60
  actual_size = 50
  proxy_data->pending_bytes = 60 + 50 = 110

This isn't correct. In iteration 2, stream 1 was mixed into stream 2. This means
there are 80 bytes at the device at the end of iteration 2. At the beginning of
iteration 3, 80 has gone down to 60, meaning 20 bytes were consumed at the
device. That means stream 1 should have 17 bytes to go, not 0 (see ****** line).

Wouldn't it make more sense to do the computation using the difference between
the previous iteration's |actual_total_size| and the
buffers_state.pending_bytes?

For example:

End of ITERATION TWO:
previous_bytes_sent_to_device =
    actual_total_size + buffers_state.pending_bytes = 80

ITERATION THREE:
bytes_consumed = previous_bytes_sent_to_device - buffers_state.pending_bytes =
20

if (actual_size == 0) {
  pending_bytes -= bytes_consumed
  etc
}

[enal1, 2012/04/16 22:01:35]

We always feed entire buffer to the OS, so numbers would be

ITERATION ONE: 
OnMoreData: buffers_state.pending_bytes = 0
stream 1:
  pending_bytes = min(0, 0) = 0
  actual_size = 50
  proxy_data->pending_bytes = 0 + 50 = 50
stream 2:
  pending_bytes = min(0, 0) = 0
  actual_size = 50
  proxy_data->pending_bytes = 0 + 50 = 50

ITERATION TWO: 
OnMoreData: buffers_state.pending_bytes = 50
stream 1:
  pending_bytes = min(50, 50) = 50
  actual_size = 7
  proxy_data->pending_bytes = 50 + 7 = 57
stream 2:
  pending_bytes = min(50, 50) = 50
  actual_size = 50
  proxy_data->pending_bytes = 50 + 50 = 100

ITERATION THREE: 
OnMoreData: buffers_state.pending_bytes = 100
stream 1:
  pending_bytes = min(57, 100) = 57
  actual_size = 0, and we hit special case in line 176.
  proxy_data->pending_bytes = max(57 - 50, 0) = 7
stream 2:
  pending_bytes = min(80, 60) = 60
  actual_size = 50
  proxy_data->pending_bytes = 60 + 50 = 110

In any case code is very tricky/fragile (see comment) and I'd definitely like to
improve it. We should either pass # of buffers to OnMoreData(), or split
OnMoreData() into 2 calls, adding ReleaseBuffer(). Either way we can keep track
of per-stream pending_bytes.

That requires changes to all (Mac, Linux, 2xWin) AudioOutputStreams, so I want
to make it separate CL, preferably combine it with getting rid of |stream|
OnMoreData() parameter.

I'd prefer to do it immediately after initial CL, maybe even before asking QA
and other groups to start testing the mixer.

Another possible improvement would be to keep physical stream opened for several
seconds after stopping playback, thus speeding up start of new stream. Not sure
how important is that, though, and it may be bad on laptop.

[vrk (LEFT CHROMIUM), 2012/04/18 14:57:36]

Thanks for the explanation! Yeah, sounds like something that can be done in a
future CL.

+    // 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);
+    // Note: there is no way we can deduce hardware_delay_bytes for the
+    // particular proxy stream. Use zero instead.
+    uint32 actual_size = stream_proxy->OnMoreData(
+        stream_proxy,
+        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.

[vrk (LEFT CHROMIUM), 2012/04/06 23:37:05]

After a stream is ended, when does it get removed from |proxies_|?

[enal1, 2012/04/16 22:01:35]

StopStream() does that.

+    if (actual_size == 0) {
+      pending_bytes -= max_size;
+      proxy_data->pending_bytes = std::max(pending_bytes, 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 = 0.0;
+    stream_proxy->GetVolume(&volume);
+    if (volume == 0.0)
+      continue;
+
+    // Different handling for first and all subsequent streams.
+    if (first_stream) {
+      if (volume != 1.0) {
+        media::AdjustVolume(actual_dest,
+                            actual_size,
+                            params_.channels(),
+                            bytes_per_sample,
+                            volume);
+      }
+      if (actual_size < max_size)
+        memset(dest + actual_size, 0, max_size - actual_size);
+      first_stream = false;
+      actual_dest = mixer_data_.get();
+      actual_total_size = actual_size;
+    } else {
+      media::MixStreams(dest,
+                        actual_dest,
+                        actual_size,
+                        bytes_per_sample,
+                        volume);
+      actual_total_size = std::max(actual_size, actual_total_size);
+    }
+  }
+  return actual_total_size;
+}
+
+void AudioOutputMixer::OnError(AudioOutputStream* stream, int code) {
+  base::AutoLock lock(lock_);
+  for (ProxyMap::iterator it = proxies_.begin(); it != proxies_.end(); ++it) {
+    it->first->OnError(it->first, code);
+  }
+}
+
+void AudioOutputMixer::WaitTillDataReady() {
+  base::AutoLock lock(lock_);
+  for (ProxyMap::iterator it = proxies_.begin(); it != proxies_.end(); ++it) {
+    it->first->WaitTillDataReady();
+  }
+}