Upgrade AudioBus to support wrapping, interleaving.

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 0dff19576dde5c5129b206daa2076b689918647b..49e30f91e06928f7aa57ac261936f32de2843da3 100644
--- a/media/base/audio_bus.h
+++ b/media/base/audio_bus.h
@@ -22,7 +22,7 @@ class AudioParameters;
 class MEDIA_EXPORT AudioBus {
  public:
   // Creates a new AudioBus and allocates |channels| of length |frames|.  Uses
-  // channels() and frames_per_buffer() if given an AudioParameters object.
+  // channels() and frames_per_buffer() from AudioParameters if given.
   static scoped_ptr<AudioBus> Create(int channels, int frames);
   static scoped_ptr<AudioBus> Create(const AudioParameters& params);
 
@@ -32,12 +32,31 @@ class MEDIA_EXPORT AudioBus {
   static scoped_ptr<AudioBus> WrapVector(
       int frames, const std::vector<float*>& channel_data);
 
+  // Creates a new AudioBus by wrapping an existing block of memory.  Block must
+  // be large enough to hold BlockSize() bytes of memory.  |data| must outlive
+  // the returned AudioBus.
+  static scoped_ptr<AudioBus> WrapBlock(int channels, int frames, void* data);

[scherkus (not reviewing), 2012/08/16 18:58:19]

WrapData? "Block" isn't a very widely used term

any restrictions on memory alignment?

it's also very strange to see a memory pointer passed in w/o any size -- I'd
almost encourage it for the sake of CHECK()ing


As a side this API is a bit strange (call BlockSize(), allocate it however you
wish, then pass the result in) but I don't have concrete suggestions as to how
to improve things at the moment.

One thought I had was whether this class should be a wrapper for externally
allocated memory so that usage of AudioBus is consistent (i.e., replace Create()
with callers allocating aligned memory alongside an AudioBus), but we don't need
to figure that out in this CL.

[DaleCurtis, 2012/08/16 19:50:33]

Agreed, ideally we'd have no Wrap methods :) Since this is explicitly intended
for wrapping shared memory.  An alternative solution might be to have an
AudioBusSharedMemory subclass which adds WrapSharedMemory() and
CalculateSharedMemorySize().

Even without the subclass, I could name the methods in that style.  I don't
forsee any other callers needing this information.

WDYT?

There are no alignment requirements since we require return our real size  +
alignment leaving us room to slide the given pointer as necessary.

[DaleCurtis, 2012/08/17 02:15:58]

Pared these down a bit to only the known use cases, fixed comments, cleaned up.
|data| now requires kChannelAlignment, have a Q outstanding to chromium-dev
about that. Renamed to WrapMemory().

+  static scoped_ptr<AudioBus> WrapBlock(const AudioParameters& params,
+                                        void* data);
+
+  // Returns the data_size() of an AudioBus created with the given parameters.
+  static int BlockSize(int channels, int frames);

[scherkus (not reviewing), 2012/08/16 18:58:19]

try to use verbs for functions -- this should be CalculateDataSize() or similar

[DaleCurtis, 2012/08/17 02:15:58]

Done.

+  static int BlockSize(const AudioParameters& params);
+
   // Returns a raw pointer to internal channel data.  Useful for copying state
   // between two AudioBus objects created with the same parameters.  data_size()
   // is in bytes.  Can not be used with an AudioBus constructed via wrapping.
   void* data();
   int data_size() const;
 
+  // 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

[scherkus (not reviewing), 2012/08/16 18:58:19]

ToInterleaved()

[DaleCurtis, 2012/08/17 02:15:58]

Done.

+  // uint8, int16, and int32 currently.
+  void FromInterleaved(const void* source, int frames, int bytes_per_sample);

[scherkus (not reviewing), 2012/08/16 18:58:19]

at some point in time it would be *fantastic* if we used an enum for
bytes_per_sample that was set to 1, 2, and 4 instead of an int

we have a lot of switches for bytes_per_sample scattered around and some
compile-time checking would be nice

[DaleCurtis, 2012/08/16 19:50:33]

Yeah, I've been thinking the same thing.  However, a direct bytes per sample
enough doesn't seem like the correct approach, but rather something like
FFMpeg's AVSampleFormat.  It's not obvious that bytes_per_sample == 1 means
uint8 vs int8, and bytes_per_sample == 2  means int16 vs uint16, etc; e.g., the
enum should be something like kSampleFormatU8, kSampleFormatS16, etc.

I'll file a bug for this if this is inline with what you're thinking.

+  void ToInterleaved(int frames, int bytes_per_sample, void* dest);
+
   // Returns a raw pointer to the requested channel.  Pointer is guaranteed to
   // have a 16-byte alignment.
   float* channel(int channel) { return channel_data_[channel]; }
@@ -55,8 +74,16 @@ class MEDIA_EXPORT AudioBus {
   ~AudioBus();
 
   AudioBus(int channels, int frames);
+  AudioBus(int channels, int frames, float* data);
   AudioBus(int frames, const std::vector<float*>& channel_data);
 
+  // Helper method for building |channel_data_| from a block of memory.  |data|
+  // must be at least BlockSize() bytes in size.
+  void BuildChannelData(int channels, int aligned_frame, float* data);
+
+  // Helper method for validating construction parameters.
+  void ValidateConfig(int channels, int frames);

[scherkus (not reviewing), 2012/08/16 18:58:19]

const method

[DaleCurtis, 2012/08/17 02:15:58]

Moved to static, not really any point to have it in the header.

+
   // Contiguous block of channel memory.
   scoped_ptr_malloc<float, base::ScopedPtrAlignedFree> data_;
   int data_size_;