Improve recommended sample counts for PPAPI.

ppapi/shared_impl/ppb_audio_config_shared.cc

 // Copyright (c) 2012 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 "ppapi/shared_impl/ppb_audio_config_shared.h"
 #include "ppapi/thunk/enter.h"
 #include "ppapi/thunk/ppb_instance_api.h"
 
 namespace ppapi {
 
+// Rounds up requested_size to the nearest multiple of minimum_size.
+static uint32_t CalculateMultipleOfSampleFrameCount(uint32_t minimum_size,
+                                                    uint32_t requested_size) {
+  const uint32_t multiple = (requested_size + minimum_size - 1) / minimum_size;
+  return std::min(minimum_size * multiple,
+                  static_cast<uint32_t>(PP_AUDIOMAXSAMPLEFRAMECOUNT));
+}
+
 PPB_AudioConfig_Shared::PPB_AudioConfig_Shared(ResourceObjectType type,
                                                PP_Instance instance)
     : Resource(type, instance),
       sample_rate_(PP_AUDIOSAMPLERATE_NONE),
       sample_frame_count_(0) {
 }
 
 PPB_AudioConfig_Shared::~PPB_AudioConfig_Shared() {
 }
 
@@ skipping 34 matching lines @@
     return 0;
 
   // Get the hardware config.
   PP_AudioSampleRate hardware_sample_rate = static_cast<PP_AudioSampleRate>(
       enter.functions()->GetAudioHardwareOutputSampleRate(instance));
   uint32_t hardware_sample_frame_count =
       enter.functions()->GetAudioHardwareOutputBufferSize(instance);
   if (sample_frame_count < PP_AUDIOMINSAMPLEFRAMECOUNT)
     sample_frame_count = PP_AUDIOMINSAMPLEFRAMECOUNT;
 
+  // If hardware information isn't available we're connected to a fake audio
+  // output stream on the browser side, so we can use whatever sample count the
+  // client wants.
+  if (!hardware_sample_frame_count || !hardware_sample_rate)
+    return sample_frame_count;
+
+  // Note: All the values below were determined through experimentation to
+  // minimize jitter and back-to-back callbacks from the browser.  Please take
+  // care when modifying these values as they impact a large number of users.
+  // TODO(dalecurtis): Land jitter test and add documentation for updating this.
+
+  // These values have only been tested with 44.1kHz and 48kHz, so ensure a
+  // runtime error occurs when new sample rates are added.
+  if (sample_rate != PP_AUDIOSAMPLERATE_44100 &&
+      sample_rate != PP_AUDIOSAMPLERATE_48000) {
+    NOTREACHED() << "Unsupported sample rate, please update "

[Chris Rogers, 2013/05/03 19:55:06]

I could be wrong, but isn't NOTREACHED() supposed to only be used in cases where
the codepath can (or should) never be reached unless there is a bug in the code.
 It's essentially equivalent to DCHECK (or ASSERT in Webkit/Blink)?

For situations which *can* actually occur in run-time, maybe a DLOG?

[DaleCurtis, 2013/05/03 20:02:08]

There's PP_DCHECK() which wraps assert().  If that's okay to use here, I'll
convert this appropriately.

[DaleCurtis, 2013/05/21 23:16:43]

Actually cpp/ which defines PP_DCHECK is excluded from shared_impl/ DEPS, so
this the best I can do.  Technically this check is already done during Init() so
I've just added a comment there and removed this section.

+                 << "RecommendSampleFrameCount_1_1()!";
+    return 0;
+  }
+
   // If client is using same sample rate as audio hardware, then recommend a
   // multiple of the audio hardware's sample frame count.
-  if (hardware_sample_rate == sample_rate && hardware_sample_frame_count > 0) {
-    // Round up input sample_frame_count to nearest multiple.
-    uint32_t multiple = (sample_frame_count + hardware_sample_frame_count - 1) /
-        hardware_sample_frame_count;
-    uint32_t recommendation = hardware_sample_frame_count * multiple;
-    if (recommendation > PP_AUDIOMAXSAMPLEFRAMECOUNT)
-      recommendation = PP_AUDIOMAXSAMPLEFRAMECOUNT;
-    return recommendation;
+  if (hardware_sample_rate == sample_rate) {
+    return CalculateMultipleOfSampleFrameCount(
+        hardware_sample_frame_count, sample_frame_count);
   }
 
-  // Otherwise, recommend a conservative 50ms buffer based on sample rate.
-  const uint32_t kDefault50msAt44100kHz = 2205;
-  const uint32_t kDefault50msAt48000kHz = 2400;
-  switch (sample_rate) {
-    case PP_AUDIOSAMPLERATE_44100:
-      return kDefault50msAt44100kHz;
-    case PP_AUDIOSAMPLERATE_48000:
-      return kDefault50msAt48000kHz;
-    case PP_AUDIOSAMPLERATE_NONE:
-      return 0;
+  // Should track the value reported by XP and ALSA backends.
+  const uint32_t kHighLatencySampleFrameCount = 2048;
+
+  // If the hardware requires a high latency buffer or we're at a low sample
+  // rate w/ a buffer that's too large, choose the nearest multiple of the high
+  // latency sample frame count.  An example of too low and too large is 16kHz
+  // and a sample frame count greater than 160 frames.
+  if (hardware_sample_frame_count >= kHighLatencySampleFrameCount ||
+      (hardware_sample_rate < 44100 &&
+       hardware_sample_frame_count > hardware_sample_rate / 100)) {
+    return CalculateMultipleOfSampleFrameCount(
+        sample_frame_count,
+        std::max(kHighLatencySampleFrameCount, hardware_sample_frame_count));
   }
-  // Unable to make a recommendation.
-  return 0;
+
+  // All low latency clients should be able to handle a 512 frame buffer with
+  // resampling from 44.1kHz and 48kHz to higher sample rates.
+  const uint32_t kLowLatencySampleFrameCount = 512;

[henrika (OOO until Aug 14), 2013/05/13 08:11:56]

Would it be beneficial to do as for WebRTC and always open up at the preferred
buffer size (e.g. 441, 447 or 448 for 44.1kHz) and then do any buffer-size
compensation on the render side. In WebRTC, that ensures that we never use the
AudioConverter FIFO (should lead to best callback timing over SyncSocket+shmem I
assume).

[DaleCurtis, 2013/05/13 21:57:37]

We could provide AudioConverter to PPAPI, but that's a much larger change and
requires support for renderer side device changes.

+
+  // Special case for 48kHz -> 44.1kHz and buffer sizes too large.  Which

[henrika (OOO until Aug 14), 2013/05/14 07:29:35]

This sentence is a bit vague IMO. Could it be clarified? Especially the "buffer
sizes too large" part feels unclear.

[DaleCurtis, 2013/05/21 23:16:43]

Cleaned up a bit.  Linux part is unnecessary.

+  // includes all Linux clients at the time of this writing (sizes >= 512).
+  int min_sample_frame_count = kLowLatencySampleFrameCount;
+  if (hardware_sample_rate == 44100 && sample_rate == 48000 &&
+      hardware_sample_frame_count > hardware_sample_rate / 100) {
+    min_sample_frame_count = std::max(
+        2 * kLowLatencySampleFrameCount, hardware_sample_frame_count);
+  }
+
+  return CalculateMultipleOfSampleFrameCount(
+      min_sample_frame_count, sample_frame_count);
 }
 
 // static
 PP_AudioSampleRate PPB_AudioConfig_Shared::RecommendSampleRate(
     PP_Instance instance) {
   thunk::EnterInstanceNoLock enter(instance);
   if (enter.failed())
     return PP_AUDIOSAMPLERATE_NONE;
   PP_AudioSampleRate hardware_sample_rate = static_cast<PP_AudioSampleRate>(
     enter.functions()->GetAudioHardwareOutputSampleRate(instance));
@@ skipping 25 matching lines @@
   if (sample_frame_count > PP_AUDIOMAXSAMPLEFRAMECOUNT ||
       sample_frame_count < PP_AUDIOMINSAMPLEFRAMECOUNT)
     return false;
 
   sample_rate_ = sample_rate;
   sample_frame_count_ = sample_frame_count;
   return true;
 }
 
 }  // namespace ppapi