Add MultiChannelResampler wrapper for SincResampler.

media/base/multi_channel_resampler.h

+// 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.
+
+#ifndef MEDIA_BASE_MULTI_CHANNEL_RESAMPLER_H_
+#define MEDIA_BASE_MULTI_CHANNEL_RESAMPLER_H_
+
+#include <vector>
+
+#include "base/memory/scoped_ptr.h"
+#include "media/base/sinc_resampler.h"
+
+namespace media {
+
+class MEDIA_EXPORT MultiChannelResampler

[Ami GONE FROM CHROMIUM, 2012/06/30 20:12:14]

Add a class-level comment (what this does, how it's useful).

[DaleCurtis, 2012/07/03 03:02:57]

Done.

+    : public SincResampler::AudioSourceProvider {
+ public:
+  // Abstract base-class for a pulling data into the resampler.

[Ami GONE FROM CHROMIUM, 2012/06/30 20:12:14]

english

[DaleCurtis, 2012/07/03 03:02:57]

Removed.

+  class MultiChannelAudioSourceProvider {

[Ami GONE FROM CHROMIUM, 2012/06/30 20:12:14]

Is this a provider of "audio source"s or a provider of audio?
(I suspect you want one of
s/Source/Data/
s/Source//
s/Provider//
)

[DaleCurtis, 2012/07/03 03:02:57]

Just ReadCB now.

+   public:
+    // ProvideInput() is called repeatedly to render time-slices of a continuous
+    // multi channel audio stream.
+    virtual void ProvideInput(const std::vector<float*>& audio_data,

[Ami GONE FROM CHROMIUM, 2012/06/30 20:12:14]

|audio_data|'s format needs description.
My guess is that the float* array has number_of_frames elements, and the outer
vector has one element per channel(?).  Any reason to mix styles instead of
going with float** or vector<vector<float> >  (my pref is for the latter, esp.
if you can typedef your way to clarity
typedef std::vector<float> SingleChannelSample;
typedef std::vector<SingleChannelAudioData> MultiChannelSample;

(modulo better names; I don't know the difference between sample & frame))

[Ami GONE FROM CHROMIUM, 2012/06/30 20:12:14]

When your interface is just one (or just a few) methods with no state, it can be
clearer & more concise to use a callback:
typedef base::Callback<void(const std::vector<float*>&, int)> DataProviderCB;

The same applies to this class itself (being a Provider for the single-channel
resampler).

[DaleCurtis, 2012/07/01 23:27:46]

I'm keeping style with AudioRendererSink::RenderCallback, since that will be the
primary caller of this API:

http://git.chromium.org/gitweb/?p=chromium.git&a=blob&f=media/base/audio_rend...

Forgot about callbacks while doing the port, that's a much better fit!

[DaleCurtis, 2012/07/03 03:02:57]

Converted to Callback, kept RenderCallback style.

+                              int number_of_frames) = 0;

[Ami GONE FROM CHROMIUM, 2012/06/30 20:12:14]

fwiw, with a type of int, I think |frames| is clear enough (here and below)

[DaleCurtis, 2012/07/01 23:27:46]

Ditto. I prefer frames as well, but am going for consistency unless you think
that's overrated.

[Ami GONE FROM CHROMIUM, 2012/07/01 23:44:44]

Yep, in this case.
(note, also, consistency can work the other way; you can change the other places
to match).
But it's obviously a pretty low-importance bit...

[DaleCurtis, 2012/07/02 17:43:45]

sgtm.

[DaleCurtis, 2012/07/03 03:02:57]

Done.

+
+   protected:
+    virtual ~MultiChannelAudioSourceProvider() {}
+  };
+
+  // Constructs a MultiChannelResampler with the specified |provider|, which is
+  // used to acquire data for all channels during resampling.  |scale_factor| is
+  // input_sample_rate / output_sample_rate.
+  // TODO(dalecurtis): Should provider be a const ref? Need to make ProvideInput

[Ami GONE FROM CHROMIUM, 2012/06/30 20:12:14]

TODO goes away with my callback suggestion above.

[DaleCurtis, 2012/07/03 03:02:57]

Removed.

+  // const if so... Following the RenderCallback model currently.
+  MultiChannelResampler(MultiChannelAudioSourceProvider* provider,

[Ami GONE FROM CHROMIUM, 2012/06/30 20:12:14]

generally output params follow input params, so this should be the last param,
not the first

[DaleCurtis, 2012/07/03 03:02:57]

These are all input parameters? I moved the CB to the end though assuming that's
what you're referring to.

+                        double scale_factor, int number_of_channels);

[Ami GONE FROM CHROMIUM, 2012/06/30 20:12:14]

s/scale_factor/io_sample_rate_ratio/ is clearer?

[Ami GONE FROM CHROMIUM, 2012/06/30 20:12:14]

FWIW, number_of_channels is implicit in the Provider call API, so you could drop
it from here and lazily create resamplers_ on the first invocation of the
provider API, and then 
DCHECK_EQ(resamplers_.size(), audio_data.size());
in the Provider.

[DaleCurtis, 2012/07/03 03:02:57]

Doesn't really save anything and makes the already complicated resample() call
more complicated. Kept as is, but dropped the channel_count_ / channel_index
stuff.

+  virtual ~MultiChannelResampler();
+
+  // Resample |number_of_frames| of data from |provider_| into |audio_data|.
+  void Resample(const std::vector<float*>& audio_data, int number_of_frames);
+
+ private:
+  int number_of_channels_;

[Ami GONE FROM CHROMIUM, 2012/06/30 20:12:14]

const

[DaleCurtis, 2012/07/03 03:02:57]

Removed.

+
+  // Index of the channel currently being processed.

[Ami GONE FROM CHROMIUM, 2012/06/30 20:12:14]

This and the next two members are used mostly for sanity-checking.  If you take
my suggestion above to replace the provider interface with a callback, then you
get to Bind the expected params into the callback and avoid the need for these
explicit members.

IOW the only reason you want these members now is to track state as control
flows between different methods of this class, but those methods are all invoked
in a single stack and you can curry these expectations into the relevant
functions when you use base::Bind.

[DaleCurtis, 2012/07/03 03:02:57]

I was able to bind away channel_index_, but I don't see how the other two can go
away (assuming we keep the sanity checks).

+  size_t channel_index_;
+
+  // Number of frames requested by SincResampler from ProvideInput() when the
+  // |channel_index_| was 0.
+  int last_frame_count_;
+
+  // Sanity check to ensure ProvideInput() is only called once per channel.
+  size_t callback_count_;
+
+  // Source of data for resampling.
+  MultiChannelAudioSourceProvider* provider_;
+
+  // Each channel has its own high quality resampler.
+  std::vector<SincResampler*> resamplers_;

[Ami GONE FROM CHROMIUM, 2012/06/30 20:12:14]

ScopedVector

[DaleCurtis, 2012/07/03 03:02:57]

Done.

+
+  // Staging area for data between the MultiChannelAudioSourceProvider and the
+  // SincResampler.
+  std::vector<float*> resampler_audio_data_;

[Ami GONE FROM CHROMIUM, 2012/06/30 20:12:14]

vector<scoped_array<float> > would be clearer about who owns what.

[DaleCurtis, 2012/07/01 23:27:46]

I don't think I can pass that along via RenderCallback style interface
unfortunately.

[Ami GONE FROM CHROMIUM, 2012/07/01 23:44:44]

Ouch (great example of how API choices propagate...).
You could re-create a vector of raw ptrs per call, or you could have a
vector<scoped_array<>> as a deleter_, but probably neither is better/clearer
than just adding a comment here that this class owns the pointed-to arrays.
(and possibly extend the TODO on the upstream Render() that threatens to convert
to a callback that something with clearer ownership semantics would be nicer).

[DaleCurtis, 2012/07/02 17:43:45]

Actually, I don't know what I was talking about. I don't have to pass this
memory anywhere it's used only inside this class. I'll change it to
vector<scoped...

[DaleCurtis, 2012/07/03 03:02:57]

Actually, that's not true, I remember what I was talking about :) This needs to
be float* for when the MCR::ReadCB is invoked on the caller. I'll update the
comment.

+
+  // SincResampler::AudioSourceProvider implementation.  ProvideInput() will be
+  // called once for each channel, starting with the first channel.  Each time

[Ami GONE FROM CHROMIUM, 2012/06/30 20:12:14]

s/starting with the first channel/in channel order/

[DaleCurtis, 2012/07/03 03:02:57]

Done.

+  // it's called, it will provide the next channel of data.
+  virtual void ProvideInput(float* destination, int number_of_frames) OVERRIDE;
+};
+
+}  // namespace media
+
+#endif  // MEDIA_BASE_MULTI_CHANNEL_RESAMPLER_H_