Simplify AudioRendererImpl by using AudioDevice.

content/renderer/media/audio_device.cc

Index: content/renderer/media/audio_device.cc
===================================================================
--- content/renderer/media/audio_device.cc	(revision 107169)
+++ content/renderer/media/audio_device.cc	(working copy)
@@ -14,6 +14,19 @@
 #include "content/renderer/render_thread_impl.h"
 #include "media/audio/audio_util.h"
 
+AudioDevice::AudioDevice(RenderCallback* callback)
+  : buffer_size_(0),

[scherkus (not reviewing), 2011/11/09 02:39:05]

indent by two more

[Chris Rogers, 2011/11/10 02:17:22]

Done.

+    channels_(0),
+    bits_per_sample_(16),
+    sample_rate_(0),
+    latency_format_(AudioParameters::AUDIO_PCM_LOW_LATENCY),
+    callback_(callback),
+    audio_delay_milliseconds_(0),
+    volume_(1.0),
+    stream_id_(0) {
+  filter_ = RenderThreadImpl::current()->audio_message_filter();
+}
+
 AudioDevice::AudioDevice(size_t buffer_size,
                          int channels,
                          double sample_rate,
@@ -27,6 +40,29 @@
       volume_(1.0),
       stream_id_(0) {
   filter_ = RenderThreadImpl::current()->audio_message_filter();
+  Initialize(buffer_size,
+             channels,
+             sample_rate,
+             AudioParameters::AUDIO_PCM_LOW_LATENCY);
+}
+
+void AudioDevice::Initialize(size_t buffer_size,
+                             int channels,
+                             double sample_rate,
+                             AudioParameters::Format latency_format) {
+  CHECK_EQ(0, stream_id_);

[scherkus (not reviewing), 2011/11/09 02:39:05]

nit: perhaps add logging?
CHECK_EQ(0, stream_id) << "reason why this is bad";

also return statement won't get executed as a failed CHECK is a crash

[Chris Rogers, 2011/11/10 02:17:22]

Done.

+  if (stream_id_)
+    return;
+
+  buffer_size_ = buffer_size;
+  channels_ = channels;
+  sample_rate_ = sample_rate;
+  latency_format_ = latency_format;
+
+  // Cleanup from any previous initialization.
+  for (size_t i = 0; i < audio_data_.size(); ++i)
+    delete [] audio_data_[i];

[acolwell GONE FROM CHROMIUM, 2011/11/08 21:44:20]

How about changing audio_data_ to use scoped_array<float>. Then you could simply
do audio_data_.clear();

[Chris Rogers, 2011/11/10 02:17:22]

It seems that scoped_array<> cannot be used with STL containers, like:
std::vector<scoped_array<float> >

Do you think shared_array should be used here?  

On 2011/11/08 21:44:20, acolwell wrote:

+
   audio_data_.reserve(channels);
   for (int i = 0; i < channels; ++i) {
     float* channel_data = new float[buffer_size];
@@ -34,6 +70,10 @@
   }
 }
 
+bool AudioDevice::IsInitialized() {
+  return audio_data_.size() > 0;
+}
+
 AudioDevice::~AudioDevice() {
   // The current design requires that the user calls Stop() before deleting
   // this class.
@@ -43,8 +83,11 @@
 }
 
 void AudioDevice::Start() {
+  if (stream_id_)

[acolwell GONE FROM CHROMIUM, 2011/11/08 21:44:20]

Should this be a CHECK_EQ() like elsewhere? We want to catch inappropriate
Start() calls right?

[henrika (OOO until Aug 14), 2011/11/09 17:05:22]

Why is the existing check in InitializeOnIOThread() not sufficient? One of the
goals with the previous AudioDevice CL was to improve thread safety and only
access stream_id_ from the IO thread.

[Chris Rogers, 2011/11/10 02:17:22]

I believe Henrik is right here.  This check is not needed since the check is
made in InitializedOnIOThread()

Removing

On 2011/11/09 17:05:22, henrika1 wrote:

+    return;
+
   AudioParameters params;
-  params.format = AudioParameters::AUDIO_PCM_LOW_LATENCY;
+  params.format = latency_format_;
   params.channels = channels_;
   params.sample_rate = static_cast<int>(sample_rate_);
   params.bits_per_sample = bits_per_sample_;
@@ -56,6 +99,9 @@
 }
 
 bool AudioDevice::Stop() {
+  if (!stream_id_)

[acolwell GONE FROM CHROMIUM, 2011/11/08 21:44:20]

CHECK_NE(0, stream_id_) ?

[henrika (OOO until Aug 14), 2011/11/09 17:05:22]

Similar comment as for Start().

[Chris Rogers, 2011/11/10 02:17:22]

Yes the check that's already in ShutDownOnIOThread() should be sufficient.  I
think Henrik's design for this was that it would be OK to call Stop() multiple
times - without crashing.  It will just be a NOP if called multiple times.

Removing unnecessary check.
On 2011/11/09 17:05:22, henrika1 wrote:

+    return true;
+
   // Max waiting time for Stop() to complete. If this time limit is passed,
   // we will stop waiting and return false. It ensures that Stop() can't block
   // the calling thread forever.
@@ -84,6 +130,18 @@
   return true;
 }
 
+void AudioDevice::Play() {
+  ChildProcess::current()->io_message_loop()->PostTask(
+      FROM_HERE,
+      base::Bind(&AudioDevice::PlayOnIOThread, this));
+}
+
+void AudioDevice::Pause(bool flush) {

[henrika (OOO until Aug 14), 2011/11/09 17:05:22]

What happens if a user calls Start(), Pause(), Play() in one sequence? To me it
feels like a potential race condition.

Same question on Start(), Play() in one sequence.

[Chris Rogers, 2011/11/10 02:17:22]

Yes, I think we can make it even simpler and consider a potential race between:

Start(), Pause()

Because OnLowLatencyCreated() might get called back late (after
PauseOnIOThread() )

The good news is that we're effectively dealing with a single-thread (the IO
thread) so there are easy ways to detect this situation and defer the pause
until after OnLowLatencyCreated() is called.  Similarly if there's any arbitrary
sequence of Pause()/Play() calls in this period we can keep track of the "final"
state and do the right thing when OnLowLatencyCreated() is finally called.

I think this can be as simple as a "bool" member variable representing if
OnLowLatencyCreated() has yet been called, and another "bool" for the final play
state for when OnLowLatencyCreated() is finally called.  No need to worry about
locks since this is all single-threaded.

Does this make sense to you?

On 2011/11/09 17:05:22, henrika1 wrote:

[henrika (OOO until Aug 14), 2011/11/10 11:45:07]

Fine by me. In general I prefer simple and readable solutions where it is
obvious what we are trying to prevent (race in this case). Even if the solution
is not perfect, it will at least be of help if more complicated calling
sequences are required. And yes, no locks.

[Chris Rogers, 2011/11/15 22:48:29]

I think I've addressed this problem with |play_on_start_| and |is_started_|

On 2011/11/10 11:45:07, henrika1 wrote:

+  ChildProcess::current()->io_message_loop()->PostTask(
+      FROM_HERE,
+      base::Bind(&AudioDevice::PauseOnIOThread, this, flush));
+}
+
 bool AudioDevice::SetVolume(double volume) {
   if (volume < 0 || volume > 1.0)
     return false;
@@ -103,7 +161,7 @@
 }
 
 void AudioDevice::InitializeOnIOThread(const AudioParameters& params) {
-  // Make sure we don't call Start() more than once.
+  // Make sure we don't create the stream more than once.
   DCHECK_EQ(0, stream_id_);
   if (stream_id_)
     return;
@@ -112,11 +170,19 @@
   Send(new AudioHostMsg_CreateStream(stream_id_, params, true));
 }
 
-void AudioDevice::StartOnIOThread() {
+void AudioDevice::PlayOnIOThread() {
   if (stream_id_)
     Send(new AudioHostMsg_PlayStream(stream_id_));
 }
 
+void AudioDevice::PauseOnIOThread(bool flush) {
+  if (stream_id_) {
+    Send(new AudioHostMsg_PauseStream(stream_id_));
+    if (flush)
+      Send(new AudioHostMsg_FlushStream(stream_id_));
+  }
+}
+
 void AudioDevice::ShutDownOnIOThread(base::WaitableEvent* completion) {
   // Make sure we don't call shutdown more than once.
   if (!stream_id_) {
@@ -138,20 +204,17 @@
 
 void AudioDevice::OnRequestPacket(AudioBuffersState buffers_state) {
   // This method does not apply to the low-latency system.
-  NOTIMPLEMENTED();

[acolwell GONE FROM CHROMIUM, 2011/11/08 21:44:20]

Why? This and the 2 below.

[Chris Rogers, 2011/11/10 02:17:22]

These are unrelated changes - just part of general cleanup.  The
NOTIMPLEMENTED() macro causes noisy console output which is unhelpful and
misleading in the sense that it implies we should be implementing something
here.  In fact, the correct implementation is to "do nothing", and not have this
macro here.
On 2011/11/08 21:44:20, acolwell wrote:

 }
 
 void AudioDevice::OnStateChanged(AudioStreamState state) {
   if (state == kAudioStreamError) {
     DLOG(WARNING) << "AudioDevice::OnStateChanged(kError)";
   }
-  NOTIMPLEMENTED();
 }
 
 void AudioDevice::OnCreated(
     base::SharedMemoryHandle handle, uint32 length) {
   // Not needed in this simple implementation.
-  NOTIMPLEMENTED();
 }
 
 void AudioDevice::OnLowLatencyCreated(
@@ -191,7 +254,7 @@
 
   MessageLoop::current()->PostTask(
       FROM_HERE,
-      base::Bind(&AudioDevice::StartOnIOThread, this));
+      base::Bind(&AudioDevice::PlayOnIOThread, this));
 }
 
 void AudioDevice::OnVolume(double volume) {
@@ -210,10 +273,8 @@
   const int samples_per_ms = static_cast<int>(sample_rate_) / 1000;
   const int bytes_per_ms = channels_ * (bits_per_sample_ / 8) * samples_per_ms;
 
-  while ((sizeof(pending_data) == socket_->Receive(&pending_data,
-                                                   sizeof(pending_data))) &&
-         (pending_data >= 0)) {

[acolwell GONE FROM CHROMIUM, 2011/11/08 21:44:20]

Why is it safe to remove the >= 0 check?

[Chris Rogers, 2011/11/10 02:17:22]

We need to remove this check, because this case comes up when we "pause" the
stream (with the addition of the Pause() method).  If we leave the check in,
then we exit the while() loop (and the thread) and audio never plays again after
it's been paused.

The thread *does* still properly shut down at the end.

On 2011/11/08 21:44:20, acolwell wrote:

[acolwell GONE FROM CHROMIUM, 2011/11/10 21:58:15]

Won't allow pending_data < 0 break the audio_delay_milliseconds_ update below? I
don't think we want this value to go negative.

On 2011/11/10 02:17:22, Chris Rogers wrote:

[Chris Rogers, 2011/11/15 22:48:29]

Yes, good point -- fixed.

On 2011/11/10 21:58:15, acolwell wrote:

-
+  while (sizeof(pending_data) ==
+      socket_->Receive(&pending_data, sizeof(pending_data))) {
     // Convert the number of pending bytes in the render buffer
     // into milliseconds.
     audio_delay_milliseconds_ = pending_data / bytes_per_ms;