Support configuring number of audio buffers in MediaStream Pepper API.

content/renderer/pepper/pepper_media_stream_audio_track_host.cc

 // Copyright 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 <algorithm>
+
 #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_buffer.h"
+#include "ppapi/host/dispatch_host_message.h"
+#include "ppapi/host/host_message_context.h"
+#include "ppapi/proxy/ppapi_messages.h"
+#include "ppapi/shared_impl/media_stream_audio_track_shared.h"
 #include "ppapi/shared_impl/media_stream_buffer.h"
 
 using media::AudioParameters;
+using ppapi::host::HostMessageContext;
+using ppapi::MediaStreamAudioTrackShared;
 
 namespace {
 
 // Max audio buffer duration in milliseconds.
 const uint32_t kMaxDuration = 10;
 
-// TODO(penghuang): make this configurable.
-const int32_t kNumberOfBuffers = 4;
+const int32_t kDefaultNumberOfBuffers = 4;
+const int32_t kMaxNumberOfBuffers = 1000;  // 10 sec

[dmichael (off chromium), 2014/05/27 20:22:33]

I'm slightly surprised that this isn't part of the new shared Attributes class
instead.

[thembrown, 2014/05/28 10:55:59]

Hm, adding it to Attributes::VerifyAttributes would change behaviour to failing
for greater values. The documentation on PP_MEDIASTREAMAUDIOTRACK_ATTRIB_BUFFERS
states that the value just serves as an advisory for the browser and it may
choose to allocate more or less buffers, depending on available resources. It
currently just clips the value - but I guess this is where some more advanced
treatment would happen in the future.

[dmichael (off chromium), 2014/05/28 15:27:43]

Makes sense, thanks.

 
 // Returns true if the |sample_rate| is supported in
 // |PP_AudioBuffer_SampleRate|, otherwise false.
 PP_AudioBuffer_SampleRate GetPPSampleRate(int sample_rate) {
   switch (sample_rate) {
     case 8000:
       return PP_AUDIOBUFFER_SAMPLERATE_8000;
     case 16000:
       return PP_AUDIOBUFFER_SAMPLERATE_16000;
     case 22050:
@@ skipping 15 matching lines @@
 
 }  // namespace
 
 namespace content {
 
 PepperMediaStreamAudioTrackHost::AudioSink::AudioSink(
     PepperMediaStreamAudioTrackHost* host)
     : host_(host),
       buffer_data_size_(0),
       main_message_loop_proxy_(base::MessageLoopProxy::current()),
-      weak_factory_(this) {}
+      weak_factory_(this),
+      number_of_buffers_(kDefaultNumberOfBuffers),
+      bytes_per_second_(0) {}
 
 PepperMediaStreamAudioTrackHost::AudioSink::~AudioSink() {
   DCHECK_EQ(main_message_loop_proxy_, base::MessageLoopProxy::current());
 }
 
 void PepperMediaStreamAudioTrackHost::AudioSink::EnqueueBuffer(int32_t index) {
   DCHECK_EQ(main_message_loop_proxy_, base::MessageLoopProxy::current());
   DCHECK_GE(index, 0);
   DCHECK_LT(index, host_->buffer_manager()->number_of_buffers());
   base::AutoLock lock(lock_);
   buffers_.push_back(index);
 }
 
+void PepperMediaStreamAudioTrackHost::AudioSink::Configure(
+    int32_t number_of_buffers) {
+  bool changed;

[dmichael (off chromium), 2014/05/27 20:22:33]

I think it would be good to have the same DCHECK_EQ as from 103 since this
should always be on the main thread.

[thembrown, 2014/05/28 10:55:59]

Done.

+  if (number_of_buffers != number_of_buffers_)
+    changed = true;
+  number_of_buffers_ = number_of_buffers;
+
+  // Initialize later in OnSetFormat if bytes_per_second_ is not know yet.
+  if (changed && bytes_per_second_ > 0)
+    InitBuffers();
+}
+
 void PepperMediaStreamAudioTrackHost::AudioSink::InitBuffersOnMainThread(
-    int32_t number_of_buffers,
-    int32_t buffer_size) {
+    int bytes_per_second) {
+  bytes_per_second_ = bytes_per_second;
+  InitBuffers();
+}
+
+void PepperMediaStreamAudioTrackHost::AudioSink::InitBuffers() {

[dmichael (off chromium), 2014/05/27 20:22:33]

These two names (InitBuffers and InitBuffersOnMainThread) are pretty confusing
now. Especially since both are called on the main thread.

What about maybe:
SetFormatOnMainThread
and
InitBuffers
?

[thembrown, 2014/05/28 10:55:59]

Agreed. Changed to SetFormatOnMainThread.

   DCHECK_EQ(main_message_loop_proxy_, base::MessageLoopProxy::current());
-  bool result = host_->InitBuffers(number_of_buffers, buffer_size, kRead);
+  // The size is slightly bigger than necessary, because 8 extra bytes are
+  // added into the struct. Also see |MediaStreamBuffer|.
+  size_t buffer_size = sizeof(ppapi::MediaStreamBuffer::Audio) +
+      bytes_per_second_ * kMaxDuration / base::Time::kMicrosecondsPerSecond;

[dmichael (off chromium), 2014/05/27 20:22:33]

The way I'm reading this, you're now dividing by 10^6 (kMicrosecondsPerSecond)
whereas the old code divided by 1000. kMaxDuration is supposedly in
milliseconds. Did you mean kMillisecondsPerSecond? Or am I missing something?

[thembrown, 2014/05/28 10:55:59]

Changed. I guess it was too late for me yesterday.

+  bool result = host_->InitBuffers(number_of_buffers_,
+                            static_cast<int32_t>(buffer_size),
+                            kRead);

[dmichael (off chromium), 2014/05/27 20:22:33]

nit: indentation is off. It looks like they all will fit if you line up with
where number_of_buffers_ is.

[thembrown, 2014/05/28 10:55:59]

Done.

   // TODO(penghuang): Send PP_ERROR_NOMEMORY to plugin.
   CHECK(result);
   base::AutoLock lock(lock_);
-  for (int32_t i = 0; i < number_of_buffers; ++i) {
+  buffers_.clear();
+  for (int32_t i = 0; i < number_of_buffers_; ++i) {
     int32_t index = host_->buffer_manager()->DequeueBuffer();
     DCHECK_GE(index, 0);
     buffers_.push_back(index);
   }
 }
 
 void PepperMediaStreamAudioTrackHost::AudioSink::
     SendEnqueueBufferMessageOnMainThread(int32_t index) {
   DCHECK_EQ(main_message_loop_proxy_, base::MessageLoopProxy::current());
   host_->SendEnqueueBufferMessageToPlugin(index);
@@ skipping 54 matching lines @@
   buffer_data_size_ = params.GetBytesPerBuffer();
 
   if (original_audio_params_.IsValid()) {
     DCHECK_EQ(params.sample_rate(), original_audio_params_.sample_rate());
     DCHECK_EQ(params.bits_per_sample(),
               original_audio_params_.bits_per_sample());
     DCHECK_EQ(params.channels(), original_audio_params_.channels());
   } else {
     audio_thread_checker_.DetachFromThread();
     original_audio_params_ = params;
-    // The size is slightly bigger than necessary, because 8 extra bytes are
-    // added into the struct. Also see |MediaStreamBuffer|.
-    size_t max_data_size = params.sample_rate() * params.bits_per_sample() / 8 *
-                           params.channels() * kMaxDuration / 1000;
-    size_t size = sizeof(ppapi::MediaStreamBuffer::Audio) + max_data_size;
 
     main_message_loop_proxy_->PostTask(
         FROM_HERE,
         base::Bind(&AudioSink::InitBuffersOnMainThread,
                    weak_factory_.GetWeakPtr(),
-                   kNumberOfBuffers,
-                   static_cast<int32_t>(size)));
+                   params.GetBytesPerSecond()));
   }
 }
 
 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();
 }
 
+int32_t PepperMediaStreamAudioTrackHost::OnResourceMessageReceived(
+    const IPC::Message& msg,
+    HostMessageContext* context) {
+  PPAPI_BEGIN_MESSAGE_MAP(PepperMediaStreamAudioTrackHost, msg)
+    PPAPI_DISPATCH_HOST_RESOURCE_CALL(
+        PpapiHostMsg_MediaStreamAudioTrack_Configure, OnHostMsgConfigure)
+  PPAPI_END_MESSAGE_MAP()
+  return PepperMediaStreamTrackHostBase::OnResourceMessageReceived(msg,
+                                                                   context);
+}
+
+int32_t PepperMediaStreamAudioTrackHost::OnHostMsgConfigure(
+    HostMessageContext* context,
+    const MediaStreamAudioTrackShared::Attributes& attributes) {
+  CHECK(MediaStreamAudioTrackShared::VerifyAttributes(attributes));

[dmichael (off chromium), 2014/05/27 20:22:33]

I don't think we should CHECK here. We're crashing the renderer because of
plugin error. IIRC this kind of thing can make fuzz testing harder. I think we
should just return PP_ERROR_FAILED.

(I notice that we do something similar in part of Video, so we should probably
turn that in to PP_ERROR_FAILED separtely also).

[thembrown, 2014/05/28 10:55:59]

Agreed. How about PP_ERROR_BADARGUMENT like in MediaStreamAudioTrackResource?

[dmichael (off chromium), 2014/05/28 15:27:43]

That's fine.

+
+  int32_t buffers = attributes.buffers
+                        ? std::min(kMaxNumberOfBuffers, attributes.buffers)
+                        : kDefaultNumberOfBuffers;
+  audio_sink_.Configure(buffers);
+
+  context->reply_msg = PpapiPluginMsg_MediaStreamAudioTrack_ConfigureReply();
+  return PP_OK;
+}
+
 void PepperMediaStreamAudioTrackHost::OnClose() {
   if (connected_) {
     MediaStreamAudioSink::RemoveFromAudioTrack(&audio_sink_, track_);
     connected_ = false;
   }
 }
 
 void PepperMediaStreamAudioTrackHost::OnNewBufferEnqueued() {
   int32_t index = buffer_manager()->DequeueBuffer();
   DCHECK_GE(index, 0);
   audio_sink_.EnqueueBuffer(index);
 }
 
 void PepperMediaStreamAudioTrackHost::DidConnectPendingHostToResource() {
   if (!connected_) {
     MediaStreamAudioSink::AddToAudioTrack(&audio_sink_, track_);
     connected_ = true;
   }
 }
 
 }  // namespace content