Reduce jitter from uneven SincResampler buffer size requests.

media/base/sinc_resampler.cc

Index: media/base/sinc_resampler.cc
diff --git a/media/base/sinc_resampler.cc b/media/base/sinc_resampler.cc
index 6bce67a3e7e85252240fad089711a2f769be6d14..9b4e7009f9dfb9f52fc84ce7727afeb85bae2472 100644
--- a/media/base/sinc_resampler.cc
+++ b/media/base/sinc_resampler.cc
@@ -2,31 +2,73 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
-// Input buffer layout, dividing the total buffer into regions (r0_ - r5_):
+// Initial input buffer layout, dividing into regions r0_ to r4_ (note: r0_, r3_
+// and r4_ will move after the first load):
 //
 // |----------------|-----------------------------------------|----------------|
 //
-//                                   kBlockSize + kKernelSize / 2
+//                                        request_frames_
 //                   <--------------------------------------------------------->
-//                                              r0_
+//                                    r0_ (during first load)

[Chris Rogers, 2013/05/07 22:57:53]

You say that "r0_ is always request_frames_ in size" and both line 10 and line
21 show this, and yet the length of these regions is different and is obviously
longer on line 10?

I can see that the start of r0_ is offset by kKernelSize/2 in the 2nd case, but
shouldn't the end of r0_ on line 22 show that it extends by this amount more to
the right?    I know 80-col limit and all, not sure...

Looking further, I guess the reason is that r3_ and r4_ also shift to the right
after the 1st time --- And I know the goal is to keep the diagram concise so
maybe it's best as is, not sure if it can be clarified.

[DaleCurtis, 2013/05/07 23:49:41]

Yeah, I ran out of space here and without shrinking the rest of the graph (hard
to do and keep labels), the ellipsis on line 22 is as best as I can do. 

 //
 //  kKernelSize / 2   kKernelSize / 2         kKernelSize / 2   kKernelSize / 2
 // <---------------> <--------------->       <---------------> <--------------->
 //        r1_               r2_                     r3_               r4_
 //
-//                                                     kBlockSize
-//                                     <--------------------------------------->
-//                                                        r5_
+//                             block_size_ == r2_ - r4_

[Chris Rogers, 2013/05/07 22:57:53]

"r2_ - r4_" should be "r4_ - r2_"   ?

[DaleCurtis, 2013/05/07 23:49:41]

Done.

+//                   <--------------------------------------->
+//
+//                                                  request_frames_
+//                                    <------------------ ... ----------------->
+//                                               r0_ (during second load)
+//
+// On the second request r0_ slides to the right by kKernelSize / 2 and r3_, r4_
+// and block_size_ are reinitialized via step (3) in the algorithm below.
+//
+// These new regions remain constant until a Flush() occurs.  While complicated,
+// this allows us to reduce jitter by always requesting the same amount from the
+// provided callback.
 //
 // The algorithm:
 //
-// 1) Consume input frames into r0_ (r1_ is zero-initialized).
-// 2) Position kernel centered at start of r0_ (r2_) and generate output frames
-//    until kernel is centered at start of r4_ or we've finished generating all
-//    the output frames.
-// 3) Copy r3_ to r1_ and r4_ to r2_.
-// 4) Consume input frames into r5_ (zero-pad if we run out of input).
-// 5) Goto (2) until all of input is consumed.
+// 1) Allocate input_buffer of size: request_frames_ + kKernelSize; this ensures
+//    there's enough room to read request_frames_ from the callback into region
+//    r0_ (which will move between the first and subsequent passes).
+//
+// 2) Let r1_, r2_ each represent half the kernel centered around r0_:
+//
+//        r0_ = input_buffer_ + kKernelSize / 2
+//        r1_ = input_buffer_
+//        r2_ = r0_
+//
+//    r0_ is always request_frames_ in size.  r1_, r2_ are kKernelSize / 2 in
+//    size.  r1_ must be zero initialized to avoid convolution with garbage (see
+//    step (5) for why).
+//
+// 3) Let r3_, r4_ each represent half the kernel right aligned with the end of
+//    r0_ and choose block_size_ as the distance in frames between r4_ and r2_:
+//
+//        r3_ = r0_ + request_frames_ - kKernelSize
+//        r4_ = r0_ + request_frames_ - kKernelSize / 2
+//        block_size_ = r4_ - r2_ = request_frames_ - kKernelSize / 2
+//
+// 4) Consume request_frames_ frames into r0_.
+//
+// 5) Position kernel centered at start of r2_ and generate output frames until
+//    the kernel is centered at the start of r4_ or we've finished generating
+//    all the output frames.
+//
+// 6) Wrap left over data from the r3_ to r1_ and r4_ to r2_.
+//
+// 7) If we're on the second load, in order to avoid overwriting the frames we
+//    just wrapped from r4_ we need to slide r0_ to the right by the size of
+//    r4_, which is kKernelSize / 2:
+//
+//        r0_ = r0_ + kKernelSize / 2 = input_buffer_ + kKernelSize
+//
+//    r3_, r4_, and block_size_ then need to be reinitialized, so goto (3).
+//
+// 8) Else, if we're not on the second load, goto (4).
 //
 // Note: we're glossing over how the sub-sample handling works with
 // |virtual_source_idx_|, etc.
@@ -64,11 +106,13 @@ static double SincScaleFactor(double io_ratio) {
   return sinc_scale_factor;
 }
 
-SincResampler::SincResampler(double io_sample_rate_ratio, const ReadCB& read_cb)
+SincResampler::SincResampler(double io_sample_rate_ratio,
+                             size_t request_frames,
+                             const ReadCB& read_cb)
     : io_sample_rate_ratio_(io_sample_rate_ratio),
-      virtual_source_idx_(0),
-      buffer_primed_(false),
       read_cb_(read_cb),
+      request_frames_(request_frames),
+      input_buffer_size_(request_frames_ + kKernelSize),
       // Create input buffers with a 16-byte alignment for SSE optimizations.
       kernel_storage_(static_cast<float*>(
           base::AlignedAlloc(sizeof(float) * kKernelStorageSize, 16))),
@@ -77,36 +121,15 @@ SincResampler::SincResampler(double io_sample_rate_ratio, const ReadCB& read_cb)
       kernel_window_storage_(static_cast<float*>(
           base::AlignedAlloc(sizeof(float) * kKernelStorageSize, 16))),
       input_buffer_(static_cast<float*>(
-          base::AlignedAlloc(sizeof(float) * kBufferSize, 16))),
+          base::AlignedAlloc(sizeof(float) * input_buffer_size_, 16))),
 #if defined(ARCH_CPU_X86_FAMILY) && !defined(__SSE__)
       convolve_proc_(base::CPU().has_sse() ? Convolve_SSE : Convolve_C),
 #endif
-      // Setup various region pointers in the buffer (see diagram above).
-      r0_(input_buffer_.get() + kKernelSize / 2),
       r1_(input_buffer_.get()),
-      r2_(r0_),
-      r3_(r0_ + kBlockSize - kKernelSize / 2),
-      r4_(r0_ + kBlockSize),
-      r5_(r0_ + kKernelSize / 2) {
-  // Ensure kKernelSize is a multiple of 32 for easy SSE optimizations; causes
-  // r0_ and r5_ (used for input) to always be 16-byte aligned by virtue of
-  // input_buffer_ being 16-byte aligned.
-  DCHECK_EQ(kKernelSize % 32, 0) << "kKernelSize must be a multiple of 32!";
-  DCHECK_GT(kBlockSize, kKernelSize)
-      << "kBlockSize must be greater than kKernelSize!";
-  // Basic sanity checks to ensure buffer regions are laid out correctly:
-  // r0_ and r2_ should always be the same position.
-  DCHECK_EQ(r0_, r2_);
-  // r1_ at the beginning of the buffer.
-  DCHECK_EQ(r1_, input_buffer_.get());
-  // r1_ left of r2_, r2_ left of r5_ and r1_, r2_ size correct.
-  DCHECK_EQ(r2_ - r1_, r5_ - r2_);
-  // r3_ left of r4_, r5_ left of r0_ and r3_ size correct.
-  DCHECK_EQ(r4_ - r3_, r5_ - r0_);
-  // r3_, r4_ size correct and r4_ at the end of the buffer.
-  DCHECK_EQ(r4_ + (r4_ - r3_), r1_ + kBufferSize);
-  // r5_ size correct and at the end of the buffer.
-  DCHECK_EQ(r5_ + kBlockSize, r1_ + kBufferSize);
+      r2_(input_buffer_.get() + kKernelSize / 2) {
+  Flush();
+  CHECK_GT(block_size_, static_cast<size_t>(kKernelSize))
+      << "block_size must be greater than kKernelSize!";
 
   memset(kernel_storage_.get(), 0,
          sizeof(*kernel_storage_.get()) * kKernelStorageSize);
@@ -114,13 +137,29 @@ SincResampler::SincResampler(double io_sample_rate_ratio, const ReadCB& read_cb)
          sizeof(*kernel_pre_sinc_storage_.get()) * kKernelStorageSize);
   memset(kernel_window_storage_.get(), 0,
          sizeof(*kernel_window_storage_.get()) * kKernelStorageSize);
-  memset(input_buffer_.get(), 0, sizeof(*input_buffer_.get()) * kBufferSize);
 
   InitializeKernel();
 }
 
 SincResampler::~SincResampler() {}
 
+void SincResampler::UpdateRegions(bool second_load) {
+  // Setup various region pointers in the buffer (see diagram above).  If we're
+  // on the second load we need to slide r0_ to the right by kKernelSize / 2.
+  r0_ = second_load ? r0_ + kKernelSize / 2 :

[Chris Rogers, 2013/05/07 22:57:53]

Maybe clearer and safer to change "r0_ + kKernelSize / 2" ->
"input_buffer_.get() + kKernelSize"

In fact, since r0_ is always just an offset from |input_buffer_|, this whole
expression can probably be something like:

r0_ = input_buffer_.get() + (second_load ? kKernelSize : kKernelSize/2)

[DaleCurtis, 2013/05/07 23:49:41]

Done.

+      input_buffer_.get() + kKernelSize / 2;
+  r3_ = r0_ + request_frames_ - kKernelSize;
+  r4_ = r0_ + request_frames_ - kKernelSize / 2;
+  block_size_ = r4_ - r2_;
+
+  // r1_ at the beginning of the buffer.
+  CHECK_EQ(r1_, input_buffer_.get());
+  // r1_ left of r2_, r4_ left of r3_ and size correct.
+  CHECK_EQ(r2_ - r1_, r4_ - r3_);
+  // r2_ left of r3.
+  CHECK_LT(r2_, r3_);
+}
+
 void SincResampler::InitializeKernel() {
   // Blackman window parameters.
   static const double kAlpha = 0.16;
@@ -201,30 +240,31 @@ void SincResampler::SetRatio(double io_sample_rate_ratio) {
 #define CONVOLVE_FUNC Convolve_C
 #endif
 
-void SincResampler::Resample(float* destination, int frames) {
+void SincResampler::Resample(int frames, float* destination) {
   int remaining_frames = frames;
 
   // Step (1) -- Prime the input buffer at the start of the input stream.
   if (!buffer_primed_) {
-    read_cb_.Run(r0_, kBlockSize + kKernelSize / 2);
+    read_cb_.Run(request_frames_, r0_);
     buffer_primed_ = true;
   }
 
   // Step (2) -- Resample!
   while (remaining_frames) {
-    while (virtual_source_idx_ < kBlockSize) {
+    while (virtual_source_idx_ < block_size_) {
       // |virtual_source_idx_| lies in between two kernel offsets so figure out
       // what they are.
-      int source_idx = static_cast<int>(virtual_source_idx_);
-      double subsample_remainder = virtual_source_idx_ - source_idx;
+      const int source_idx = virtual_source_idx_;
+      const double subsample_remainder = virtual_source_idx_ - source_idx;
 
-      double virtual_offset_idx = subsample_remainder * kKernelOffsetCount;
-      int offset_idx = static_cast<int>(virtual_offset_idx);
+      const double virtual_offset_idx =
+          subsample_remainder * kKernelOffsetCount;
+      const int offset_idx = virtual_offset_idx;
 
       // We'll compute "convolutions" for the two kernels which straddle
       // |virtual_source_idx_|.
-      float* k1 = kernel_storage_.get() + offset_idx * kKernelSize;
-      float* k2 = k1 + kKernelSize;
+      const float* k1 = kernel_storage_.get() + offset_idx * kKernelSize;
+      const float* k2 = k1 + kKernelSize;
 
       // Ensure |k1|, |k2| are 16-byte aligned for SIMD usage.  Should always be
       // true so long as kKernelSize is a multiple of 16.
@@ -232,10 +272,11 @@ void SincResampler::Resample(float* destination, int frames) {
       DCHECK_EQ(0u, reinterpret_cast<uintptr_t>(k2) & 0x0F);
 
       // Initialize input pointer based on quantized |virtual_source_idx_|.
-      float* input_ptr = r1_ + source_idx;
+      const float* input_ptr = r1_ + source_idx;
 
       // Figure out how much to weight each kernel's "convolution".
-      double kernel_interpolation_factor = virtual_offset_idx - offset_idx;
+      const double kernel_interpolation_factor =
+          virtual_offset_idx - offset_idx;
       *destination++ = CONVOLVE_FUNC(
           input_ptr, k1, k2, kernel_interpolation_factor);
 
@@ -247,29 +288,33 @@ void SincResampler::Resample(float* destination, int frames) {
     }
 
     // Wrap back around to the start.
-    virtual_source_idx_ -= kBlockSize;
+    virtual_source_idx_ -= block_size_;
 
-    // Step (3) Copy r3_ to r1_ and r4_ to r2_.
+    // Step (3) -- Copy r3_, r4_ to r1_, r2_.
     // This wraps the last input frames back to the start of the buffer.
-    memcpy(r1_, r3_, sizeof(*input_buffer_.get()) * (kKernelSize / 2));
-    memcpy(r2_, r4_, sizeof(*input_buffer_.get()) * (kKernelSize / 2));
+    memcpy(r1_, r3_, sizeof(*input_buffer_.get()) * kKernelSize);
+
+    // Step (4) -- Reinitialize regions if necessary.
+    if (r0_ == r2_)
+      UpdateRegions(true);
 
-    // Step (4)
-    // Refresh the buffer with more input.
-    read_cb_.Run(r5_, kBlockSize);
+    // Step (5) -- Refresh the buffer with more input.
+    read_cb_.Run(request_frames_, r0_);
   }
 }
 
 #undef CONVOLVE_FUNC
 
 int SincResampler::ChunkSize() const {
-  return kBlockSize / io_sample_rate_ratio_;
+  return block_size_ / io_sample_rate_ratio_;
 }
 
 void SincResampler::Flush() {
   virtual_source_idx_ = 0;
   buffer_primed_ = false;
-  memset(input_buffer_.get(), 0, sizeof(*input_buffer_.get()) * kBufferSize);
+  memset(input_buffer_.get(), 0,
+         sizeof(*input_buffer_.get()) * input_buffer_size_);
+  UpdateRegions(false);
 }
 
 float SincResampler::Convolve_C(const float* input_ptr, const float* k1,