AudioBus: Add a ToInterleavedFloat() method to AudioBus

media/base/audio_bus.h

Index: media/base/audio_bus.h
diff --git a/media/base/audio_bus.h b/media/base/audio_bus.h
index f827f9db87d396667e39d3105f10a33f85e39d11..e4a56f755f2c418e1d9e63b6c37427257c3470f8 100644
--- a/media/base/audio_bus.h
+++ b/media/base/audio_bus.h
@@ -18,11 +18,15 @@
 namespace media {
 class AudioParameters;
 
-// Scoped container for "busing" audio channel data around.  Each channel is
-// stored in planar format and guaranteed to be aligned by kChannelAlignment.
-// AudioBus objects can be created normally or via wrapping.  Normally, AudioBus
-// will dice up a contiguous memory block for channel data.  When wrapped,
-// AudioBus instead routes requests for channel data to the wrapped object.
+// Represents a sequence of audio frames containing frames() audio samples for
+// each of channels() channels. The data is stored as a set of contiguous
+// float arrays with one array per channel. The memory for the arrays is either
+// allocated and owned by the AudioBus or it is provided to one of the factory
+// methods. AudioBus guarantees that it allocates memory such that float array
+// for each channel is aligned by AudioBus::kChannelAlignment bytes and it
+// requires the same for memory passed to its Wrap...() factory methods.
+// TODO(chfremer: When can SetChannelData be used? There are currently no unit
+// tests involving CreateWrapper and SetChannelData, so we need to add them.
 class MEDIA_EXPORT AudioBus {
  public:
   // Guaranteed alignment of each channel's data; use 16-byte alignment for easy
@@ -35,8 +39,8 @@ class MEDIA_EXPORT AudioBus {
   static std::unique_ptr<AudioBus> Create(const AudioParameters& params);
 
   // Creates a new AudioBus with the given number of channels, but zero length.
-  // It's expected to be used with SetChannelData() and set_frames() to
-  // wrap externally allocated memory.
+  // Clients are expected to subsequently call SetChannelData() and set_frames()
+  // to wrap externally allocated memory.
   static std::unique_ptr<AudioBus> CreateWrapper(int channels);
 
   // Creates a new AudioBus from an existing channel vector.  Does not transfer
@@ -54,21 +58,35 @@ class MEDIA_EXPORT AudioBus {
                                               void* data);
   static std::unique_ptr<AudioBus> WrapMemory(const AudioParameters& params,
                                               void* data);
-  static int CalculateMemorySize(const AudioParameters& params);
 
-  // Calculates the required size for an AudioBus given the number of channels
-  // and frames.
+  // Based on the given number of channels and frames, calculates the minimum
+  // required size in bytes of a contiguous block of memory to be passed to
+  // AudioBus for storage of the audio data.
+  // Uses channels() and frames_per_buffer() from AudioParameters if given.
   static int CalculateMemorySize(int channels, int frames);
+  static int CalculateMemorySize(const AudioParameters& params);
+
+  // Methods that are expected to be called after AudioBus::CreateWrapper() in
+  // order to wrap externally allocated memory. Note: It is illegal to call
+  // these methods when using a factory method other than CreateWrapper().
+  void SetChannelData(int channel, float* data);
+  void set_frames(int frames);
 
   // Helper methods for converting an AudioBus from and to interleaved integer
   // data.  Expects interleaving to be [ch0, ch1, ..., chN, ch0, ch1, ...] with
   // |bytes_per_sample| per value.  Values are scaled and bias corrected during
   // conversion.  ToInterleaved() will also clip values to format range.
   // Handles uint8_t, int16_t, and int32_t currently.  FromInterleaved() will
-  // zero out
-  // any unfilled frames when |frames| is less than frames().
+  // zero out any unfilled frames when |frames| is less than frames().
   void FromInterleaved(const void* source, int frames, int bytes_per_sample);
   void ToInterleaved(int frames, int bytes_per_sample, void* dest) const;
+  // TODO(chfremer): Instead of adding extra functions for float, it may be
+  // nicer to reuse the existing methods and replace |int bytes_per_sample" with

[miu, 2016/06/02 20:40:21]

FYI--We're actually trying to get rid of |bytes_per_sample| throughout the audio
stack. :)

The reason is that we want the audio stack to standardize on planar float PCM.
The conversion to/from interleaved int PCM is almost always a
platform-interface-glue detail.

+  // an enum indicating the frame type.
+  void ToInterleavedFloat(int source_offset,

[miu, 2016/06/02 20:40:21]

I agree with Miguel here, but realize it's a much larger change: Seems that we
should have templated From/ToInterleaved methods instead. Something like:

  template <typename InterleavedPODType>
  void FromInterleaved(const InterleavedPODType* source, int frames);

  template <typename InterleavedPODType>
  void ToInterleaved(int frames, InterleavedPODType* dest) const;

...where the only implemented InterleavedPODTypes would be int16_t and float
(unless there is actually code that uses 8-bit or 24-bit integer PCM?).

[chfremer, 2016/06/07 21:21:39]

Done.

+                          int destination_offset,

[miu, 2016/06/02 20:40:21]

nit: Please remove the |destination_offset| argument. Instead the caller should
do pointer math on |buffer| so that it points at the correct starting
destination location.

+                          int num_channels,

[miu, 2016/06/02 20:40:21]

This argument should be named |frames| (it's definitely not the number of
channels!).

+                          float* buffer) const;
   void ToInterleavedPartial(int start_frame, int frames, int bytes_per_sample,
                             void* dest) const;
 
@@ -97,11 +115,11 @@ class MEDIA_EXPORT AudioBus {
   // inf, nan, or between [-1.0, 1.0]) values in the channel data.
   float* channel(int channel) { return channel_data_[channel]; }
   const float* channel(int channel) const { return channel_data_[channel]; }
-  void SetChannelData(int channel, float* data);
 
+  // Returns the number of channels.
   int channels() const { return static_cast<int>(channel_data_.size()); }
+  // Returns the number of frames.
   int frames() const { return frames_; }
-  void set_frames(int frames);
 
   // Helper method for zeroing out all channels of audio data.
   void Zero();
@@ -129,12 +147,16 @@ class MEDIA_EXPORT AudioBus {
 
  private:
   // Helper method for building |channel_data_| from a block of memory.  |data|
-  // must be at least BlockSize() bytes in size.
+  // must be at least CalculateMemorySize(...) bytes in size.
   void BuildChannelData(int channels, int aligned_frame, float* data);
 
   // Contiguous block of channel memory.
   std::unique_ptr<float, base::AlignedFreeDeleter> data_;
 
+  // One float pointer per channel pointing to a contiguous block of memory for
+  // that channel. If the memory is owned by this instance, this will
+  // point to the memory in |data_|. Otherwise, it may point to memory provided
+  // by the client.
   std::vector<float*> channel_data_;
   int frames_;