[PPAPI] Pepper MediaStream API audio track implementation and example.

content/renderer/pepper/pepper_media_stream_audio_track_host.cc

Index: content/renderer/pepper/pepper_media_stream_audio_track_host.cc
diff --git a/content/renderer/pepper/pepper_media_stream_audio_track_host.cc b/content/renderer/pepper/pepper_media_stream_audio_track_host.cc
new file mode 100644
index 0000000000000000000000000000000000000000..2b64911fb206a4aa1dc989672ef448269044956e
--- /dev/null
+++ b/content/renderer/pepper/pepper_media_stream_audio_track_host.cc
@@ -0,0 +1,117 @@
+// Copyright (c) 2014 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/pepper/pepper_media_stream_audio_track_host.h"
+
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/message_loop/message_loop_proxy.h"
+#include "ppapi/c/pp_errors.h"
+#include "ppapi/c/ppb_audio_frame.h"
+#include "ppapi/shared_impl/media_stream_frame.h"
+
+namespace {
+
+// TODO(penghuang): make it configurable.
+const int32_t kNumberOfFrames = 4;
+
+}  // namespace
+
+namespace content {
+
+PepperMediaStreamAudioTrackHost::PepperMediaStreamAudioTrackHost(
+    RendererPpapiHost* host,
+    PP_Instance instance,
+    PP_Resource resource,
+    const blink::WebMediaStreamTrack& track)
+    : PepperMediaStreamTrackHostBase(host, instance, resource),
+      track_(track),
+      connected_(false),
+      frame_data_size_(0),
+      main_message_loop_proxy_(base::MessageLoopProxy::current()),
+      weak_factory_(this) {
+  DCHECK(!track_.isNull());
+}
+
+PepperMediaStreamAudioTrackHost::~PepperMediaStreamAudioTrackHost() {
+  OnClose();
+}
+
+void PepperMediaStreamAudioTrackHost::InitFramesOnMainThread(
+    uint32_t number_of_frames, uint32_t frame_size) {
+  bool result = InitFrames(number_of_frames, frame_size);
+  DCHECK(result);
+}
+
+void PepperMediaStreamAudioTrackHost::OnClose() {
+  if (connected_) {
+    MediaStreamAudioSink::RemoveFromAudioTrack(this, track_);
+    connected_ = false;
+  }
+}
+
+void PepperMediaStreamAudioTrackHost::OnData(const int16* audio_data,
+                                             int sample_rate,
+                                             int number_of_channels,
+                                             int number_of_frames) {
+  DCHECK(audio_data);
+  DCHECK_EQ(sample_rate, audio_params_.sample_rate());
+  DCHECK_EQ(number_of_channels, audio_params_.channels());
+  DCHECK_EQ(number_of_frames, audio_params_.frames_per_buffer());
+
+  int32_t index = frame_buffer()->DequeueFrame();
+  // Drop frames if the underlying buffer is full or not initialized.
+  if (index >= 0) {
+    // TODO(penghuang): support re-sampling, etc.
+    ppapi::MediaStreamFrame::Audio* frame =
+        &(frame_buffer()->GetFramePointer(index)->audio);

[dmichael (off chromium), 2014/01/21 23:22:53]

It's not obvious that it's safe to call frame_buffer() or
SendEnqueueFrameMessage inside OnData. (I think it is technically safe, but I
first thought there was a bug here in the current implementation. I.e., that the
PepperMediaStreamAudioTrackHost could be destroyed after this point, and so
|frame| and |this| might become invalid.)

The thing that protects you (I think!) is that the destructor calls OnClose,
which calls RemoveFromAudioTrack, which ultimately calls
MediaStreamAudioSinkOwner::Reset, which tries to acquire the same lock that is
held while OnData is running:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

So...  the destructor in that case actually blocks, and so |this| stays valid
until after OnData has finished.

But that's all pretty subtle. (Aside: It makes me kind of sad that the
MediaStreamAudio code already has at least a couple of locks, and we're adding
another. I'm not so much worried about performance as I am our ability to reason
about it and get it right with no race conditions or deadlocks.)

I'm thinking that you should define a separate "AudioSinkDelegate" class (name
up to you) that implements MediaStreamAudioSink. The
PepperMediaStreamAudioTrackHost is responsible for creating it and calling the
Add/RemoveFromAudioTrack functions. It would probably have to have some
callbacks to the Host to relay back the "WroteFrame(index)" and "InitFrames"
events. It doesn't actually simplify anything, but at least there would be some
distinction about which class runs on which threads, which is more in keeping
with Chromium practice, so more readable to a Chromium developer, probably. The
delegate would probably just have a raw pointer to the buffer. (So the downside
there is we still have to make the same assumptions about lifespan. But it still
might be cleaner, and we'd have convenient places to document our assumptions.. 
like, right after we call RemoveFromAudioTrack and right before we delete the
Delegate.)

WDYT?

By the way, I'm having trouble thinking of any nice way to avoid the lock in the
buffer :-/.

[Peng, 2014/01/22 20:13:32]

Done

+    frame->header.size = frame_buffer()->frame_size();
+    frame->header.type = ppapi::MediaStreamFrame::TYPE_AUDIO;
+    frame->timestamp = timestamp_.InMillisecondsF();
+    frame->sample_rate = static_cast<PP_AudioSampleRate>(sample_rate);
+    frame->number_of_channels = number_of_channels;
+    frame->number_of_samples = number_of_channels * number_of_frames;
+    frame->data_size = frame_data_size_;
+
+    memcpy(frame->data, audio_data, frame_data_size_);
+    // This function is called from an audio thread, but
+    // |SendEnqueueFrameMessageToPlugin()| doesn't use any sync IPC,
+    // so it is safe to call it directly.
+    SendEnqueueFrameMessageToPlugin(index);

[dmichael (off chromium), 2014/01/17 23:45:06]

The way that ChannelProxy makes it OK to send from any thread is by doing
PostTask on to the IO thread. So there's always a thread hop. I wonder if you
could make the Host live on the IO thread, and PostTask from here to a host
operation on the IO thread. Then there would probably be a way to avoid another
"PostTask" from there...

You could implement the MediaStreamAudioSink stuff in a separate delegate class
that lives on the audio thread.

I really think we should avoid using the lock the way you have, because it's
unusual in Chrome. It's also unusual and confusing here that this class gets
created on the main thread but called on the audio thread for most operations
here, and called on the main thread for base class operations. I'm not 100% sure
if there's a clean design for separating this without messing up your use of a
base class for the host, but I think the current locking approach plus the mix
of two different calling threads for methods in this Host hierarchy is too
different from usual Chromium practice.

[Peng, 2014/01/20 22:05:44]

Do we have existing host which lives on the IO thread? I don't know how to do
it.
There are some difficulties if lock is not used:

1. The argument audio_data of OnData() is owned by caller, if we want to post a
task to main thread to create and send frames, we have to copy it to a temporary
memory in the audio thread first, and then copy the data from the temporary
memory to the frame buffer in the main thread later. :(

2. If we want to fill the frame in the audio thread, we have to maintain the
|frame_qeue_| in the audio thread. So when we receive the HostEnqueueFrame
message, we need post a task to the audio thread to add frame into the
|frame_queue_|. But the audio thread does not have a message loop.  So this way
does not work. :(

3. I think an atomic integer can resolve issue. We could use 32 (or 64) bits of
the atomic integer to mark which frames are free. So we can access this integer
in the audio and main threads without lock. But unfortunately, base/atomicops.h
does not support bits operations now. We need implement it. :(

Any idea? What's your suggestion?

[dmichael (off chromium), 2014/01/21 23:22:53]

We have "Filters" instead, which are (IIRC) created and destroyed on the IO
thread. Mostly in the browser side.
https://code.google.com/p/chromium/codesearch#search/&q=Pepper.*Filter&sq=pac...
Note how in OverrideTaskRunnerForMessage, they can specify which messages are
handled on which threads. You could The tricky bit there is that you have a
shared Host base class, which might not work right if you transition to a
Filter.

So...  I'm not sure it's worth the trouble. It would still be hard to avoid
another PostTask when sending from the IO thread, I think.
My best alternative I can think of right now is having yet another buffer, sort
of a simple (slightly modified) producer/consumer queue for talking to the audio
thread. When the Host gets an Enqueue message, it immediately dequeues and
pushes it in to the audio thread's queue. Inside the audio thread, there would
be a call on that little queue like ...TryDequeue, which grabs a lock inside the
producer/consumer queue, if there's a frame, pops it, and returns either failure
or the frame. I'm not sure if this is any better. It separates the concerns of
cross-thread from cross-process, so it might actually be clearer.  But it means
implementing a whole separate (though simpler) queue, so that's kind of bad. And
it still requires locking.

[Peng, 2014/01/22 20:13:32]

Leave it. Maybe revisit it in future, if we find any performance issue.
Done.

+  }
+  timestamp_ += frame_duration_;
+}
+
+void PepperMediaStreamAudioTrackHost::OnSetFormat(
+    const media::AudioParameters& params) {
+  DCHECK(!audio_params_.IsValid());
+  DCHECK(params.IsValid());
+  DCHECK_EQ(params.bits_per_sample(), 16);
+
+  // TODO(penghuang): support setting format more than once.
+  audio_params_ = params;
+  frame_duration_ = audio_params_.GetBufferDuration();
+  frame_data_size_ = audio_params_.GetBytesPerBuffer();
+
+  // The size is slight bigger for containing a frame, because 8 extra bytes are

[dmichael (off chromium), 2014/01/17 23:45:06]

nit: "slight bigger"->"slightly bigger than necessary"

[Peng, 2014/01/20 22:05:44]

Done.

+  // added into the struct. Also see |MediaStreamFrame|.
+  int32_t size = sizeof(ppapi::MediaStreamFrame::Audio) + frame_data_size_;
+
+  // This function is called from an audio thread and |InitFrames()| uses some
+  // sync IPC. So we have to call |InitFrames()| in the main thread.
+  main_message_loop_proxy_->PostTask(
+      FROM_HERE,
+      base::Bind(&PepperMediaStreamAudioTrackHost::InitFramesOnMainThread,
+                 weak_factory_.GetWeakPtr(), kNumberOfFrames, size));
+}
+
+void PepperMediaStreamAudioTrackHost::DidConnectPendingHostToResource() {
+  if (!connected_) {
+    MediaStreamAudioSink::AddToAudioTrack(this, track_);
+    connected_ = true;
+  }
+}
+
+}  // namespace content