Simplify AudioRendererImpl by using AudioDevice.

content/renderer/media/audio_device.cc

 // Copyright (c) 2011 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 "content/renderer/media/audio_device.h"
 
 #include "base/bind.h"
 #include "base/debug/trace_event.h"
 #include "base/message_loop.h"
 #include "base/time.h"
 #include "content/common/child_process.h"
 #include "content/common/media/audio_messages.h"
 #include "content/common/view_messages.h"
 #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,
                          RenderCallback* callback)
     : buffer_size_(buffer_size),
       channels_(channels),
       bits_per_sample_(16),
       sample_rate_(sample_rate),
       callback_(callback),
       audio_delay_milliseconds_(0),
       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];
     audio_data_.push_back(channel_data);
   }
 }
 
+bool AudioDevice::IsInitialized() {
+  return audio_data_.size() > 0;
+}
+
 AudioDevice::~AudioDevice() {
   // The current design requires that the user calls Stop() before deleting
   // this class.
   CHECK_EQ(0, stream_id_);
   for (int i = 0; i < channels_; ++i)
     delete [] audio_data_[i];
 }
 
 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_;
   params.samples_per_packet = buffer_size_;
 
   ChildProcess::current()->io_message_loop()->PostTask(
       FROM_HERE,
       base::Bind(&AudioDevice::InitializeOnIOThread, this, params));

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

Any reason we can't just move all this into Initialize() and eliminate Start()?

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

That would change the client-API contract, as we currently require the Start()
method to be called.  AudioDevice is currently used by WebRTC and WebAudio, so
we don't want to change the API contract ... unless we have a good reason to

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

 }
 
 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.
   const base::TimeDelta kMaxTimeOut = base::TimeDelta::FromMilliseconds(1000);
 
   base::WaitableEvent completion(false, false);
 
   ChildProcess::current()->io_message_loop()->PostTask(
       FROM_HERE,
       base::Bind(&AudioDevice::ShutDownOnIOThread, this, &completion));
 
   // We wait here for the IO task to be completed to remove race conflicts
   // with OnLowLatencyCreated() and to ensure that Stop() acts as a synchronous
   // function call.
   if (completion.TimedWait(kMaxTimeOut)) {
     if (audio_thread_.get()) {
       socket_->Close();
       audio_thread_->Join();
       audio_thread_.reset(NULL);
     }
   } else {
     LOG(ERROR) << "Failed to shut down audio output on IO thread";
     return false;
   }
 
   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;
 
   ChildProcess::current()->io_message_loop()->PostTask(
       FROM_HERE,
       base::Bind(&AudioDevice::SetVolumeOnIOThread, this, volume));
 
   volume_ = volume;
 
   return true;
 }
 
 void AudioDevice::GetVolume(double* volume) {
   // Return a locally cached version of the current scaling factor.
   *volume = volume_;
 }
 
 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;
 
   stream_id_ = filter_->AddDelegate(this);
   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_) {
     completion->Signal();
     return;
   }
 
   filter_->RemoveDelegate(stream_id_);
   Send(new AudioHostMsg_CloseStream(stream_id_));
   stream_id_ = 0;
 
   completion->Signal();
 }
 
 void AudioDevice::SetVolumeOnIOThread(double volume) {
   if (stream_id_)
     Send(new AudioHostMsg_SetVolume(stream_id_, volume));
 }
 
 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(
     base::SharedMemoryHandle handle,
     base::SyncSocket::Handle socket_handle,
     uint32 length) {
   DCHECK(MessageLoop::current() == ChildProcess::current()->io_message_loop());
 #if defined(OS_WIN)
   DCHECK(handle);
   DCHECK(socket_handle);
@@ skipping 19 matching lines @@
   socket_.reset(new base::SyncSocket(socket_handle));
   // Allow the client to pre-populate the buffer.
   FireRenderCallback();
 
   audio_thread_.reset(
       new base::DelegateSimpleThread(this, "renderer_audio_thread"));
   audio_thread_->Start();
 
   MessageLoop::current()->PostTask(
       FROM_HERE,
-      base::Bind(&AudioDevice::StartOnIOThread, this));
+      base::Bind(&AudioDevice::PlayOnIOThread, this));
 }
 
 void AudioDevice::OnVolume(double volume) {
   NOTIMPLEMENTED();
 }
 
 void AudioDevice::Send(IPC::Message* message) {
   filter_->Send(message);
 }
 
 // Our audio thread runs here.
 void AudioDevice::Run() {
   audio_thread_->SetThreadPriority(base::kThreadPriority_RealtimeAudio);
 
   int pending_data;
   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;
     FireRenderCallback();
   }
 }
 
 void AudioDevice::FireRenderCallback() {
   TRACE_EVENT0("audio", "AudioDevice::FireRenderCallback");
 
@@ skipping 20 matching lines @@
 
 size_t AudioDevice::GetAudioHardwareBufferSize() {
   // Uses cached value if possible.
   static size_t buffer_size = 0;
 
   if (!buffer_size)
     buffer_size = media::GetAudioHardwareBufferSize();
 
   return buffer_size;
 }