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

content/renderer/pepper/pepper_media_stream_audio_track_host.cc

+// 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/macros.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"
+
+using media::AudioParameters;
+
+namespace {
+
+// TODO(penghuang): make it configurable.
+const int32_t kNumberOfFrames = 4;
+
+}  // namespace
+
+namespace content {
+
+PepperMediaStreamAudioTrackHost::AudioSink::AudioSink(
+    PepperMediaStreamAudioTrackHost* host)
+    : host_(host),
+      frame_data_size_(0),
+      frames_(0),
+      main_message_loop_proxy_(base::MessageLoopProxy::current()) {
+}
+
+PepperMediaStreamAudioTrackHost::AudioSink::~AudioSink() {}
+
+void PepperMediaStreamAudioTrackHost::AudioSink::EnqueueFrame(int32_t index) {
+  DCHECK_GE(index, 0);

[yzshen1, 2014/01/29 21:28:06]

Please also also DCHECK that this is called on the correct thread.

[Peng, 2014/01/31 18:54:43]

Done.

+  DCHECK_LT(index, host_->frame_buffer()->number_of_frames());
+  DCHECK(!(base::subtle::NoBarrier_Load(&frames_) & (1 << index)));
+
+  // Set the bit for the frame index. Automic does not support and/or
+  // operations, so we use add/sub instead.
+  base::subtle::NoBarrier_AtomicIncrement(&frames_, 1 << index);

[yzshen1, 2014/01/29 21:28:06]

This kind of code is, as the name suggested, subtle.
If there is no evidence that this is performance bottleneck, we should consider
using Lock. 

I haven't looked into details, but without barrier, how is memory visibility?

[Peng, 2014/01/31 18:54:43]

Atomic's performance is slightly better than lock. And David said we should try
to 
avoid using lock. So I think Atomic is a good candidate. I am OK to use lock as
well.
As I understand, memory barrier is for forbidding out-of-order accessing. So it
is not necessary here.

See the example in http://en.wikipedia.org/wiki/Memory_barrier

[yzshen1, 2014/01/31 19:50:26]

IMO, the biggest reason that we avoid lock is to make things more intuitive and
less error prone. We use message loops + posting tasks to achieve that.
Here avoiding lock only makes the code harder to understand or modify. :/ If
performance is not an issue, I personally think using lock may be a good idea.

What do you think, Peng and David?
I thought without memory barrier, theoretically it is possible that the other
threads could not observe the change for quite a while. (Imagine that the data
is cached separately for different cores.)

But my understanding of memory model is poor, so I am probably talking nonsense.
:)

[dmichael (off chromium), 2014/01/31 20:14:28]

Yuzhu's exactly right; I was only concerned about Lock because it's usually
against Chromium style. I'm sorry I wasn't more clear.

See:
http://www.chromium.org/developers/design-documents/threading
(and also
http://www.chromium.org/developers/design-documents/threading/suble-threading...
)
The performance of lock is almost inconsequential here; the thread hop for
sending messages on the IO thread is much more expensive. We should aim for
"obviously correct".

In this case, after the discussions we've had, I think making Buffer a
thread-safe object is appropriate. The right way to do that is using base::Lock.

Again, sorry I was unclear before.

[Peng, 2014/01/31 21:09:25]

Done.

[Peng, 2014/01/31 21:09:25]

Fixed by using base::Lock. Done

+}
+
+void PepperMediaStreamAudioTrackHost::AudioSink::InitFramesOnMainThread(
+    uint32_t number_of_frames, uint32_t frame_size) {
+  // Maybe use Atomic64 to support more frames.
+  DCHECK_LE(number_of_frames, 32u);
+  bool result = host_->InitFrames(number_of_frames, frame_size);
+  DCHECK(result);
+  for (uint32_t i = 0; i < number_of_frames; ++i) {
+    int32_t index = host_->frame_buffer()->DequeueFrame();
+    DCHECK_GE(index, 0);
+  }
+
+  base::subtle::Atomic32 frames = base::subtle::NoBarrier_CompareAndSwap(
+      &frames_, 0, (1 << number_of_frames) - 1);
+  DCHECK(!frames);
+}
+
+void PepperMediaStreamAudioTrackHost::AudioSink::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());
+
+  base::subtle::Atomic32 frames = base::subtle::NoBarrier_Load(&frames_);
+  if (frames) {
+    // If frames isn't zero, the |frame_buffer()| should be initialized already,
+    // and |frame_buffer()|'s attributes (|number_of_frames()|, |frame_size()|,
+    // etc) will not be changed. So it is safe to read them in the audio thread.
+    int32_t index = 0;
+    int32_t n = host_->frame_buffer()->number_of_frames();
+
+    // Find a free frame.
+    while(index < n && !(frames & (1 << index)))
+      index ++;

[yzshen1, 2014/01/29 21:28:06]

no space in the middle, please.

[Peng, 2014/01/31 18:54:43]

Done.

+
+    if (index < n) {
+      // TODO(penghuang): support re-sampling, etc.
+      ppapi::MediaStreamFrame::Audio* frame =
+          &(host_->frame_buffer()->GetFramePointer(index)->audio);

[yzshen1, 2014/01/29 21:28:06]

We are accessing the frame buffer from multiple threads without a lock. I saw
your comments on 73-76, but it has an implicit assumption about how frame buffer
is implemented.

[Peng, 2014/01/31 18:54:43]

Yes. After the frame_buffer() being initialized, the frame pointers will not be
changed.
So it is save to call GetFramePointer() in multiple threads. Any suggestion to
make
the code looks better?

+      frame->header.size = host_->frame_buffer()->frame_size();
+      frame->header.type = ppapi::MediaStreamFrame::TYPE_AUDIO;
+      frame->timestamp = timestamp_.InMillisecondsF();
+      frame->sample_rate = static_cast<PP_AudioFrame_SampleRate>(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_);
+
+      // Clear the bit for the frame index. Automic does not support and/or
+      // operations, so we use add/sub instead.
+      base::subtle::NoBarrier_AtomicIncrement(&frames_, -(1 << index));

[yzshen1, 2014/01/29 21:28:06]

what if index == 31?

[Peng, 2014/01/31 18:54:43]

I think (1 << 32) will be 0x80000000. Because it is 32 bits integer, -(1 << 32)
will be 0x80000000 too. frames_ + 0x80000000 == frames_ - 0x80000000, when
frame_'s 31th bit is 1.

+
+      // This function is called from the audio thread, but
+      // |SendEnqueueFrameMessageToPlugin()| doesn't use any sync IPC,
+      // so it is safe to call it from the audio thread directly.
+      host_->SendEnqueueFrameMessageToPlugin(index);

[yzshen1, 2014/01/29 21:28:06]

Unless we said explicitly comment at the declaration that this method is thread
safe.

But I really don't think this is a good idea.

[Peng, 2014/01/31 18:54:43]

I understand. But I really don't have any good idea how to make it better.
Or PostTask to main thread to add another thread hopping.

WDYT?

+    }
+  }
+  timestamp_ += frame_duration_;
+}
+
+void PepperMediaStreamAudioTrackHost::AudioSink::OnSetFormat(
+    const AudioParameters& params) {
+  DCHECK(!audio_params_.IsValid());
+  DCHECK(params.IsValid());
+  DCHECK_EQ(params.bits_per_sample(), 16);
+  DCHECK((params.sample_rate() == AudioParameters::kTelephoneSampleRate) ||
+         (params.sample_rate() == AudioParameters::kAudioCDSampleRate));
+
+  COMPILE_ASSERT(AudioParameters::kTelephoneSampleRate == 8000,
+                 audio_sample_rate_does_not_match);
+  COMPILE_ASSERT(AudioParameters::kAudioCDSampleRate == 44100,
+                 audio_sample_rate_does_not_match);
+
+  // 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 slightly bigger than necessary, because 8 extra bytes are added
+  // into the struct. Also see |MediaStreamFrame|.
+  int32_t size = sizeof(ppapi::MediaStreamFrame::Audio) + frame_data_size_;
+
+  // This function is called from the audio thread and |InitFrames()| uses some
+  // sync IPC. So we have to call |InitFrames()| from the main thread.

[yzshen1, 2014/01/29 21:28:06]

I have no problem with running InitFrames() on the main thread, but can we avoid
sync IPCs?

[Peng, 2014/01/31 18:54:43]

Currently, InitFrames() uses
content::RenderThread::HostAllocateSharedMemoryBuffer(size).
It is a sync function. Do you know how to do it in async way? Maybe it is better
to do
it in a separate CL as well. I added a TODO in
pepper_media_stream_track_host_base.cc.

[yzshen1, 2014/02/03 18:14:36]

Ah, I see. :)

I thought you meant that you added new sync IPCs. I am fine with
HostAllocateSharedMemoryBuffer().

On 2014/01/31 18:54:43, Peng wrote:

+  main_message_loop_proxy_->PostTask(
+      FROM_HERE,
+      base::Bind(&AudioSink::InitFramesOnMainThread,
+                 AsWeakPtr(), kNumberOfFrames, size));
+}
+
+PepperMediaStreamAudioTrackHost::PepperMediaStreamAudioTrackHost(
+    RendererPpapiHost* host,
+    PP_Instance instance,
+    PP_Resource resource,
+    const blink::WebMediaStreamTrack& track)
+    : PepperMediaStreamTrackHostBase(host, instance, resource),
+      track_(track),
+      connected_(false),
+      audio_sink_(this) {
+  DCHECK(!track_.isNull());
+}
+
+PepperMediaStreamAudioTrackHost::~PepperMediaStreamAudioTrackHost() {
+  OnClose();

[yzshen1, 2014/01/29 21:28:06]

(I haven't read the relevant backend code, but want to make sure you have
considered that.)
Can we be sure that at this point, audio_sink_ is not used on the audio thread?
I think that requires MediaStreamAudioSink::RemoveFromAudioTrack() to be
blocking if there is any ongoing activity on the audio thread.

[Peng, 2014/01/31 18:54:43]

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
I do see a lock is used in AddSink(), RemoveSink(), OnData() and OnSetFromant().
I assume it is thread safe. I think xians@ may give us more detail.

+}
+
+void PepperMediaStreamAudioTrackHost::OnClose() {
+  if (connected_) {
+    MediaStreamAudioSink::RemoveFromAudioTrack(&audio_sink_, track_);
+    connected_ = false;
+  }
+}
+
+bool PepperMediaStreamAudioTrackHost::OnNewFramePreEnqueued(int32_t index) {
+  audio_sink_.EnqueueFrame(index);
+  return true;
+}
+
+void PepperMediaStreamAudioTrackHost::DidConnectPendingHostToResource() {
+  if (!connected_) {
+    MediaStreamAudioSink::AddToAudioTrack(&audio_sink_, track_);
+    connected_ = true;
+  }
+}
+
+}  // namespace content