Support configuring the audio buffer duration in the Pepper MediaStream 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 "base/numerics/safe_math.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;
+// Audio buffer durations in milliseconds.
+const uint32_t kMinDuration = 10;
+const uint32_t kDefaultDuration = 123;

[dmichael (off chromium), 2014/07/23 20:08:41]

This seems like a weird default...  did you benchmark or something?

[Anand Mistry (off Chromium), 2014/07/24 01:28:28]

Oops. A hold over from testing. It should be 10, which is the existing
behaviour.

 
 const int32_t kDefaultNumberOfBuffers = 4;
 const int32_t kMaxNumberOfBuffers = 1000;  // 10 sec
 
 // 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;
@@ skipping 17 matching lines @@
 }
 
 }  // namespace
 
 namespace content {
 
 PepperMediaStreamAudioTrackHost::AudioSink::AudioSink(
     PepperMediaStreamAudioTrackHost* host)
     : host_(host),
       buffer_data_size_(0),
+      active_buffer_index_(-1),
+      active_buffer_offset_(0),
       main_message_loop_proxy_(base::MessageLoopProxy::current()),
       weak_factory_(this),
       number_of_buffers_(kDefaultNumberOfBuffers),
-      bytes_per_second_(0) {}
+      bytes_per_second_(0),
+      user_buffer_duration_(kDefaultDuration) {}
 
 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) {
+    int32_t number_of_buffers, int32_t duration) {
   DCHECK_EQ(main_message_loop_proxy_, base::MessageLoopProxy::current());
   bool changed = false;
   if (number_of_buffers != number_of_buffers_)
     changed = true;
+  if (duration != 0 && duration != user_buffer_duration_) {
+    user_buffer_duration_ = duration;
+    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)
+  if (changed && bytes_per_second_ > 0 && bytes_per_frame_ > 0)
     InitBuffers();
 }
 
 void PepperMediaStreamAudioTrackHost::AudioSink::SetFormatOnMainThread(
-    int bytes_per_second) {
+    int bytes_per_second, int bytes_per_frame) {
   bytes_per_second_ = bytes_per_second;
+  bytes_per_frame_ = bytes_per_frame;
   InitBuffers();
 }
 
 void PepperMediaStreamAudioTrackHost::AudioSink::InitBuffers() {
   DCHECK_EQ(main_message_loop_proxy_, base::MessageLoopProxy::current());
+  int32_t frame_rate = bytes_per_second_ / bytes_per_frame_;
+  int32_t frames_per_buffer = (user_buffer_duration_ * frame_rate) /
+      base::Time::kMillisecondsPerSecond;

[dmichael (off chromium), 2014/07/23 20:08:41]

Please use CheckedNumeric from base/numerics/safe_math.h for this stuff to avoid
potential overflow.

[Anand Mistry (off Chromium), 2014/07/24 01:28:28]

Done.

+  int32_t buffer_audio_size = frames_per_buffer * bytes_per_frame_;
   // The size is slightly bigger than necessary, because 8 extra bytes are
-  // added into the struct. Also see |MediaStreamBuffer|.
-  base::CheckedNumeric<int32_t> buffer_size = bytes_per_second_;
-  buffer_size *= kMaxDuration;
-  buffer_size /= base::Time::kMillisecondsPerSecond;
+  // added into the struct. Also see |MediaStreamBuffer|. Also, the size of the
+  // buffer may be larger than requested, since the size of each buffer will be
+  // 4-byte aligned.
+  base::CheckedNumeric<int32_t> buffer_size = buffer_audio_size;
   buffer_size += sizeof(ppapi::MediaStreamBuffer::Audio);
+  // Re-initialising buffers will unmap the existing buffers. If the audio
+  // thread is writing to one, then it will SEGV.
+  // TODO(amistry): Fix me!

[dmichael (off chromium), 2014/07/23 20:08:41]

Good catch! File a bug to reference here, please.

Unless your intent is to fix it prior to or in this CL, which I'd also
support...

[Peng, 2014/07/23 20:40:31]

I has a CL[1] to fix this issue. With the CL [1], probably this CL need some
modifications. Or maybe you could fix this issue within this CL.

[1] https://codereview.chromium.org/417753002/

[Anand Mistry (off Chromium), 2014/07/24 01:28:28]

Applied that CL to this.

   bool result = host_->InitBuffers(number_of_buffers_,
                                    buffer_size.ValueOrDie(),
                                    kRead);
   // TODO(penghuang): Send PP_ERROR_NOMEMORY to plugin.
   CHECK(result);
   base::AutoLock lock(lock_);
+  output_buffer_size_ = buffer_audio_size;
   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);
 }
 
 void PepperMediaStreamAudioTrackHost::AudioSink::OnData(const int16* audio_data,
                                                         int sample_rate,
                                                         int number_of_channels,
                                                         int number_of_frames) {
   DCHECK(audio_thread_checker_.CalledOnValidThread());
   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());

[dmichael (off chromium), 2014/07/23 20:08:41]

Given this ^^^ Why do you need the new code to allow using partial buffers? Or
is this DCHECK wrong now?

[Anand Mistry (off Chromium), 2014/07/24 01:28:28]

More like this DCHECK is irrelevant because this code doesn't care what the
configured number of frames is. But I think it's still worth leaving here as a
check of the API. I believe the audio source is supposed to guarantee this
property, and if this number does change, OnSetFormat() should have been called.

[dmichael (off chromium), 2014/07/29 16:41:24]

So if we're guaranteed to have this property, can't we keep the code simpler?
Error out of OnData if it doesn't match?

Your code looks right, but it's complex, and there's bound to be some subtle bug
in it. If we don't need the complexity, we shouldn't add it and have to maintain
it.

[Anand Mistry (off Chromium), 2014/07/30 00:22:05]

The reason this code is complex is to handle cases where the requested buffer
duration is not an integer multiple of the received buffer duration. i.e. if the
user configures a buffer of 11ms and we get 10ms chunks, the second chuck we
receive will span 2 output buffer, and the complexity is pretty much devoted to
handling that correctly.

[dmichael (off chromium), 2014/07/30 23:09:22]

So audio_params_.frames_per_buffer() might not reflect what the user configured.
I think that's what I was missing.

Can you make a comment to clarify that? I.e., buffer->number_of_samples may not
be equal to audio_params_.frames_per_buffer().

[Anand Mistry (off Chromium), 2014/07/31 00:41:21]

Done.

-  int32_t index = -1;
-  {
-    base::AutoLock lock(lock_);
-    if (!buffers_.empty()) {
-      index = buffers_.front();
-      buffers_.pop_front();
+
+  const uint32_t bytes_per_frame = number_of_channels *
+      audio_params_.bits_per_sample() / 8;
+
+  int frames_remaining = number_of_frames;
+  base::TimeDelta timestamp_offset;
+  while (frames_remaining) {
+    uint32_t output_buffer_size = 0;
+    if (active_buffer_index_ == -1) {
+      active_buffer_offset_ = 0;
+      active_buffer_ptr_ = NULL;
+      base::AutoLock lock(lock_);
+      output_buffer_size = output_buffer_size_;
+      if (!buffers_.empty()) {
+        active_buffer_index_ = buffers_.front();
+        buffers_.pop_front();
+      }
     }
-  }
+    if (active_buffer_index_ == -1) {
+      // Eek! We're dropping frames. Bad, bad, bad!
+      break;
+    }
 
-  if (index != -1) {
     // TODO(penghuang): support re-sampling, etc.
+    // TODO(amistry): NOT SAFE!

[dmichael (off chromium), 2014/07/23 20:08:42]

Can you elaborate?

[Anand Mistry (off Chromium), 2014/07/24 01:28:28]

It was to do with the thread safety of the buffer manager. Not an issue any
more.

     ppapi::MediaStreamBuffer::Audio* buffer =
-        &(host_->buffer_manager()->GetBufferPointer(index)->audio);
-    buffer->header.size = host_->buffer_manager()->buffer_size();
-    buffer->header.type = ppapi::MediaStreamBuffer::TYPE_AUDIO;
-    buffer->timestamp = timestamp_.InMillisecondsF();
-    buffer->sample_rate = static_cast<PP_AudioBuffer_SampleRate>(sample_rate);
-    buffer->number_of_channels = number_of_channels;
-    buffer->number_of_samples = number_of_channels * number_of_frames;
-    buffer->data_size = buffer_data_size_;
-    memcpy(buffer->data, audio_data, buffer_data_size_);
+        &(host_->buffer_manager()->GetBufferPointer(active_buffer_index_)
+          ->audio);
+    if (active_buffer_offset_ == 0) {
+      // Next buffer. Not using an active buffer.
+      active_buffer_ptr_ = buffer;
+      buffer->header.size = host_->buffer_manager()->buffer_size();
+      buffer->header.type = ppapi::MediaStreamBuffer::TYPE_AUDIO;
+      buffer->timestamp = (timestamp_ + timestamp_offset).InMillisecondsF();
+      buffer->sample_rate = static_cast<PP_AudioBuffer_SampleRate>(sample_rate);
+      DCHECK_NE(output_buffer_size, 0U);
+      buffer->data_size = output_buffer_size;
+      buffer->number_of_channels = number_of_channels;
+      buffer->number_of_samples = buffer->data_size / bytes_per_frame;
+    } else if (buffer != active_buffer_ptr_ || buffer->data_size == 0) {
+      // Buffer no longer valid.
+      active_buffer_index_ = -1;
+      continue;
+    }
+    uint32_t buffer_bytes_remaining =
+        buffer->data_size - active_buffer_offset_;
+    DCHECK_EQ(buffer_bytes_remaining % bytes_per_frame, 0U);
+    uint32_t incoming_bytes_remaining = frames_remaining * bytes_per_frame;
+    uint32_t bytes_to_copy = std::min(buffer_bytes_remaining,
+                                      incoming_bytes_remaining);
+    uint32_t frames_to_copy = bytes_to_copy / bytes_per_frame;
+    DCHECK_EQ(bytes_to_copy % bytes_per_frame, 0U);
+    memcpy(buffer->data + active_buffer_offset_,
+           audio_data, bytes_to_copy);
+    active_buffer_offset_ += bytes_to_copy;
+    audio_data += bytes_to_copy / 2;
+    frames_remaining -= frames_to_copy;
+    timestamp_offset += base::TimeDelta::FromMilliseconds(
+        frames_to_copy * base::Time::kMillisecondsPerSecond / sample_rate);
 
-    main_message_loop_proxy_->PostTask(
-        FROM_HERE,
-        base::Bind(&AudioSink::SendEnqueueBufferMessageOnMainThread,
-                   weak_factory_.GetWeakPtr(),
-                   index));
+    DCHECK_LE(active_buffer_offset_, buffer->data_size);
+    if (active_buffer_offset_ == buffer->data_size) {
+      main_message_loop_proxy_->PostTask(
+          FROM_HERE,
+          base::Bind(&AudioSink::SendEnqueueBufferMessageOnMainThread,
+                     weak_factory_.GetWeakPtr(),
+                     active_buffer_index_));
+      active_buffer_index_ = -1;
+    }
   }
   timestamp_ += buffer_duration_;
 }
 
 void PepperMediaStreamAudioTrackHost::AudioSink::OnSetFormat(
     const AudioParameters& params) {
   DCHECK(params.IsValid());
-  DCHECK_LE(params.GetBufferDuration().InMilliseconds(), kMaxDuration);
+  // TODO(amistry): How do you handle the case where the user configures a
+  // duration that's shorter than the received buffer duration? One option is to
+  // double buffer, where the size of the intermediate ring buffer is at least
+  // max(user requested duration, received buffer duration). There are other
+  // ways of dealing with it, but which one is "correct"?
+  DCHECK_LE(params.GetBufferDuration().InMilliseconds(), kMinDuration);
   DCHECK_EQ(params.bits_per_sample(), 16);
   DCHECK_NE(GetPPSampleRate(params.sample_rate()),
             PP_AUDIOBUFFER_SAMPLERATE_UNKNOWN);
 
   audio_params_ = params;
 
   // TODO(penghuang): support setting format more than once.
   buffer_duration_ = params.GetBufferDuration();
   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;
 
+    int bytes_per_frame = params.channels() * params.bits_per_sample() / 8;
     main_message_loop_proxy_->PostTask(
         FROM_HERE,
         base::Bind(&AudioSink::SetFormatOnMainThread,
                    weak_factory_.GetWeakPtr(),
-                   params.GetBytesPerSecond()));
+                   params.GetBytesPerSecond(),
+                   bytes_per_frame));
   }
 }
 
 PepperMediaStreamAudioTrackHost::PepperMediaStreamAudioTrackHost(
     RendererPpapiHost* host,
     PP_Instance instance,
     PP_Resource resource,
     const blink::WebMediaStreamTrack& track)
     : PepperMediaStreamTrackHostBase(host, instance, resource),
       track_(track),
@@ skipping 19 matching lines @@
 
 int32_t PepperMediaStreamAudioTrackHost::OnHostMsgConfigure(
     HostMessageContext* context,
     const MediaStreamAudioTrackShared::Attributes& attributes) {
   if (!MediaStreamAudioTrackShared::VerifyAttributes(attributes))
     return PP_ERROR_BADARGUMENT;
 
   int32_t buffers = attributes.buffers
                         ? std::min(kMaxNumberOfBuffers, attributes.buffers)
                         : kDefaultNumberOfBuffers;
-  audio_sink_.Configure(buffers);
+  audio_sink_.Configure(buffers, attributes.duration);
 
   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