Implement renderer AudioDevice API for low-latency audio output...

chrome/renderer/audio_device.cc

+// Copyright (c) 2010 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 "chrome/renderer/audio_device.h"
+
+#include "chrome/common/render_messages_params.h"
+#include "chrome/renderer/render_thread.h"
+
+scoped_refptr<AudioMessageFilter> AudioDevice::filter_;
+
+AudioDevice::AudioDevice(size_t buffer_size,
+                         int channels,
+                         double sample_rate,
+                         RenderCallback* callback)
+    : buffer_size_(buffer_size),
+      channels_(channels),
+      sample_rate_(sample_rate),
+      callback_(callback),
+      stream_id_(0) {
+  audio_data_.reserve(channels);

[scherkus (not reviewing), 2011/01/10 23:42:32]

(thinking aloud) I wonder if it makes sense to make audio_data_ a class that
takes care of this stuff... probably not

[Chris Rogers, 2011/01/11 23:01:02]

I think for the simple case here it might not be necessary, but if we start
doing a lot more with planar format like this in chromium then we could consider
something like what I've done in WebKit with the AudioChannel and AudioBus
classes:

https://svn.webkit.org/repository/webkit/trunk/Source/WebCore/platform/audio/...
https://svn.webkit.org/repository/webkit/trunk/Source/WebCore/platform/audio/...

On 2011/01/10 23:42:32, scherkus wrote:

+  for (int i = 0; i < channels; ++i) {
+    float* channel_data = static_cast<float*>(
+        malloc(buffer_size * sizeof(float)));

[scherkus (not reviewing), 2011/01/10 23:42:32]

malloc is rarely used -- I'd just go for new float[]

[Chris Rogers, 2011/01/11 23:01:02]

Done.

+    audio_data_.push_back(channel_data);
+  }
+}
+
+AudioDevice::~AudioDevice() {
+  Stop();
+  for (int i = 0; i < channels_; ++i)
+    free(audio_data_[i]);

[scherkus (not reviewing), 2011/01/10 23:42:32]

and delete[] here

[Chris Rogers, 2011/01/11 23:01:02]

Done.

+}
+
+bool AudioDevice::Start() {

[neb, 2011/01/10 23:11:57]

For PPAPI, there was a requirement that all the audio calls happen on one
thread, so we can get away with not making them thread safe. Checking stream_id_
non-atomically, using scoped_ptrs without mutexes and so on. What threading
model does this support?

[Chris Rogers, 2011/01/11 23:01:02]

Andrew also asked the same question.  The class is not thread-safe, so all calls
must be made on the same thread.  I've added a class comment in the header file.

On 2011/01/10 23:11:57, neb wrote:

+  // Make sure we don't call Start() more than once.
+  DCHECK_EQ(0, stream_id_);
+  if (stream_id_)
+    return false;
+
+  // Lazily create the message filter and share across AudioDevice instances.
+  if (!filter_.get()) {

[scherkus (not reviewing), 2011/01/10 23:42:32]

sanity check: are we sure there's no race condition here?

which threads can Start() be executed on?

[Chris Rogers, 2011/01/11 23:01:02]

Yes, you're right.  Unless the API contract is that calls for all instances can
only be made on the same thread (like main thread) then we need a lock which
I've added.

On 2011/01/10 23:42:32, scherkus wrote:

+    int routing_id;
+    RenderThread::current()->Send(
+        new ViewHostMsg_GenerateRoutingID(&routing_id));
+    filter_ = new AudioMessageFilter(routing_id);
+    RenderThread::current()->AddFilter(filter_);
+  }
+
+  stream_id_ = filter_->AddDelegate(this);
+
+  ViewHostMsg_Audio_CreateStream_Params params;
+  params.params.format = AudioParameters::AUDIO_PCM_LINEAR;
+  params.params.channels = channels_;
+  params.params.sample_rate = sample_rate_;
+  params.params.bits_per_sample = 16;
+  params.params.samples_per_packet = buffer_size_;
+
+  filter_->Send(
+      new ViewHostMsg_CreateAudioStream(0, stream_id_, params, true));
+
+  return true;
+}
+
+bool AudioDevice::Stop() {
+  if (stream_id_) {
+    OnDestroy();
+    return true;
+  }
+  return false;
+}
+
+void AudioDevice::OnDestroy() {
+  // Make sure we don't call destroy more than once.
+  DCHECK_NE(0, stream_id_);
+  if (!stream_id_)
+    return;
+
+  filter_->RemoveDelegate(stream_id_);
+  filter_->Send(new ViewHostMsg_CloseAudioStream(0, stream_id_));
+  stream_id_ = 0;
+  if (audio_thread_.get()) {
+    socket_->Close();
+    audio_thread_->Join();
+  }
+}
+
+void AudioDevice::OnRequestPacket(AudioBuffersState buffers_state) {

[scherkus (not reviewing), 2011/01/10 23:42:32]

is there any plan to implement these?

perhaps have NOTIMPLEMENTED or a comment or a TODO etc

[Chris Rogers, 2011/01/11 23:01:02]

Done.

+}
+
+void AudioDevice::OnStateChanged(
+    const ViewMsg_AudioStreamState_Params& state) {
+}
+
+void AudioDevice::OnCreated(
+    base::SharedMemoryHandle handle, uint32 length) {
+}
+
+void AudioDevice::OnLowLatencyCreated(
+    base::SharedMemoryHandle handle, base::SyncSocket::Handle socket_handle,

[scherkus (not reviewing), 2011/01/10 23:42:32]

drop 2nd arg to next line

[Chris Rogers, 2011/01/11 23:01:02]

Done.

+    uint32 length) {
+
+#if defined(OS_WIN)
+  DCHECK(handle);
+  DCHECK(socket_handle);
+#else
+  DCHECK_NE(-1, handle.fd);

[scherkus (not reviewing), 2011/01/10 23:42:32]

sanity check: is -1 the only possible bad value or should this be
DCHECK_GE(handle.fd, 0) ?

[Chris Rogers, 2011/01/11 23:01:02]

This was taken from Neb's code, but yes, your check might be better -- changing.

On 2011/01/10 23:42:32, scherkus wrote:

+  DCHECK_NE(-1, socket_handle);
+#endif
+  DCHECK(length);
+  DCHECK(!audio_thread_.get());
+
+  // TODO(crogers) : check that length is big enough for buffer_size_
+
+  shared_memory_.reset(new base::SharedMemory(handle, false));
+  shared_memory_->Map(length);
+
+  socket_.reset(new base::SyncSocket(socket_handle));
+  // Allow the client to pre-populate the buffer.
+  FireRenderCallback();
+
+  // TODO(crogers) : we could optionally set the thread to high-priority

[scherkus (not reviewing), 2011/01/10 23:42:32]

nit: no space between ) and :

[Chris Rogers, 2011/01/11 23:01:02]

Done.

+  audio_thread_.reset(
+      new base::DelegateSimpleThread(this, "renderer_audio_thread"));
+  audio_thread_->Start();
+
+  filter_->Send(new ViewHostMsg_PlayAudioStream(0, stream_id_));
+}
+
+void AudioDevice::OnVolume(double volume) {

[scherkus (not reviewing), 2011/01/10 23:42:32]

ditto for this method

[Chris Rogers, 2011/01/11 23:01:02]

Done.

+}
+
+// Our audio thread runs here.
+void AudioDevice::Run() {
+  int pending_data;
+  while (sizeof(pending_data) == socket_->Receive(&pending_data,
+                                                  sizeof(pending_data)) &&
+      pending_data >= 0) {

[scherkus (not reviewing), 2011/01/10 23:42:32]

this should be aligned at (

[Chris Rogers, 2011/01/11 23:01:02]

Done.

+    FireRenderCallback();
+  }
+}
+
+void AudioDevice::FireRenderCallback() {
+  if (callback_) {
+    // For now, only handle stereo case.
+    DCHECK_EQ(2, channels_);

[scherkus (not reviewing), 2011/01/10 23:42:32]

instead of failing late perhaps this should get checked in Start() and return
false?

I haven't done the math+checks but if the client assumes there's 6 channels and
we're only supporting 2 it sounds ripe for a buffer overflow

[Chris Rogers, 2011/01/11 23:01:02]

In looking over this code again, I don't think the check for stereo is necessary
- removing.

On 2011/01/10 23:42:32, scherkus wrote:

+
+    // Ask client to render audio.
+    callback_->render(audio_data_, buffer_size_);

[scherkus (not reviewing), 2011/01/10 23:42:32]

double-checking something just to be safe..

the header file says 2nd arg is number_of_frames but if I understand this
correctly this is buffer_size_ (in bytes?) which I guess is technically
number_of_frames * sizeof(float)

[Chris Rogers, 2011/01/11 23:01:02]

Actually, buffer_size_ is in sample-frames, so this should be OK.  In the header
file, I've added a comment for the constructor and the member variable.

I don't like the field name "samples_per_packet" in
ViewHostMsg_Audio_CreateStream_Params, but it appears to actually mean
"number_of_sample_frames".


On 2011/01/10 23:42:32, scherkus wrote:

+
+    // Interleave, scale, and clip to int16.

[neb, 2011/01/10 23:11:57]

In PPAPI, we expect to provide different sample types in the future, such as
floats. Are you sure you want to do this conversion here?

[scherkus (not reviewing), 2011/01/10 23:42:32]

Is it worth converting everything to float at some point?  If I recall I think
we do a few conversions between int/float before we hit the sound card

[Chris Rogers, 2011/01/11 23:01:02]

I would be in favor of switching most of our code to float.  One advantage is
that it's not lossy like int16 is.  Some audio hardware outputs at 24bits, so
we're currently losing information when converting to int16.  Floating-point is
easy to work with, and is fast nowadays.  The entire web audio engine operates
on floating-point data.

We still need to keep in mind that FFmpeg (I believe) generally decodes to
int16, and some of the OS-level audio APIs only support int16 output, but that's
OK.

We can talk about moving more of our code to deal with float as a possible goal.


On 2011/01/10 23:42:32, scherkus wrote:

[Chris Rogers, 2011/01/11 23:01:02]

Currently the API requires the client provide the audio data in floating-point
format (in the callback).  We could engineer this to be a bit more flexible
where the AudioDevice can be configured to either deal with int16 or float data.
 Then we'd have to deal with void* pointers in the callback, which get casted to
one or the other depending on type.  Or, we could extend the "RenderCallback"
class to have two or more different flavors of callback which get invoked
depending on how it's configured.  I'm very open to making this more flexible,
but it would be good to get a base-level implementation landed, then work on
extending the API in subsequent patches.

On 2011/01/10 23:11:57, neb wrote:

+    int16* output_buffer16 = static_cast<int16*>(shared_memory_data());

[scherkus (not reviewing), 2011/01/10 23:42:32]

is there anything in media/audio/audio_util.h we can consider using and/or
moving there?

[Chris Rogers, 2011/01/11 23:01:02]

Sure, I made a more general function in audio_util.h called
InterleaveFloatToInt16()

On 2011/01/10 23:42:32, scherkus wrote:

+    const float kScale = 32768.0f;
+    for (int i = 0; i < channels_; ++i) {
+      float* channel_data = audio_data_[i];
+      for (size_t j = 0; j < buffer_size_; ++j) {
+        float sample = kScale * channel_data[j];
+        if (sample < -32768.0)
+          sample = -32768.0;
+        else if (sample > 32767.0)
+          sample = 32767.0;
+
+        output_buffer16[j * channels_ + i] = static_cast<int16>(sample);
+      }
+    }
+  }
+}