Support configuring number of audio buffers in MediaStream Pepper API.

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
index f45f6019d858aa00d9e5fcc0f9f50206f03c4b14..99057a8a7918801e51597c442b0897d7b9697ca0 100644
--- a/content/renderer/pepper/pepper_media_stream_audio_track_host.cc
+++ b/content/renderer/pepper/pepper_media_stream_audio_track_host.cc
@@ -4,6 +4,8 @@
 
 #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"
@@ -11,17 +13,23 @@
 #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.
@@ -57,7 +65,9 @@ PepperMediaStreamAudioTrackHost::AudioSink::AudioSink(
     : 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());
@@ -71,15 +81,38 @@ void PepperMediaStreamAudioTrackHost::AudioSink::EnqueueBuffer(int32_t index) {
   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);
@@ -154,18 +187,12 @@ void PepperMediaStreamAudioTrackHost::AudioSink::OnSetFormat(
   } 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()));
   }
 }
 
@@ -185,6 +212,31 @@ 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_);