Support configuring the audio buffer duration in the Pepper MediaStream 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 d972c2c061e2882eb1257d92fc9981e18a296ba5..38063bf038da43202c279ae3c0ec68d64818e27e 100644
--- a/content/renderer/pepper/pepper_media_stream_audio_track_host.cc
+++ b/content/renderer/pepper/pepper_media_stream_audio_track_host.cc
@@ -26,8 +26,9 @@ 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
@@ -65,10 +66,13 @@ 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());
@@ -83,38 +87,51 @@ void PepperMediaStreamAudioTrackHost::AudioSink::EnqueueBuffer(int32_t 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();
@@ -138,33 +155,75 @@ void PepperMediaStreamAudioTrackHost::AudioSink::OnData(const int16* 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_);
-
-    main_message_loop_proxy_->PostTask(
-        FROM_HERE,
-        base::Bind(&AudioSink::SendEnqueueBufferMessageOnMainThread,
-                   weak_factory_.GetWeakPtr(),
-                   index));
+        &(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);
+
+    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_;
 }
@@ -172,7 +231,12 @@ void PepperMediaStreamAudioTrackHost::AudioSink::OnData(const int16* audio_data,
 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);
@@ -192,11 +256,13 @@ void PepperMediaStreamAudioTrackHost::AudioSink::OnSetFormat(
     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));
   }
 }
 
@@ -236,7 +302,7 @@ int32_t PepperMediaStreamAudioTrackHost::OnHostMsgConfigure(
   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;