Adds AudioFifo class to Chrome media.

media/base/audio_fifo.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_AUDIO_FIFO_H_
+#define MEDIA_BASE_AUDIO_FIFO_H_
+
+#include "media/base/audio_bus.h"
+#include "media/base/media_export.h"
+
+namespace media {
+
+// First-in first-out container for AudioBus elements.
+// The maximum number of audio frames in the FIFO is set at construction and
+// can not be extended dynamically.  Each channel is stored in planar format
+// and guaranteed to be aligned by AudioBus::kChannelAlignment.  The allocated

[Chris Rogers, 2012/09/04 20:07:20]

comment nit: I'd simply remove this comment about alignment, since this is
really a property of AudioBus and not this class.  In other words, Append() and
Remove() both deal with copying from and to AudioBus objects which themselves
have the property of alignment.

[henrika (OOO until Aug 14), 2012/09/05 10:14:36]

Done.

+// memory is utilized as a circular queue.

[Chris Rogers, 2012/09/04 20:07:20]

nit: My preference is to avoid the word "queue" and instead use "buffer" or
"ring buffer", since queue usually involves operations on single elements
instead of variable length amounts of data.

[henrika (OOO until Aug 14), 2012/09/05 10:14:36]

Done.

+class MEDIA_EXPORT AudioFifo {
+ public:
+  // Creates a new AudioFifo and allocates |channels| of length |frames|.
+  AudioFifo(int channels, int frames);
+  virtual ~AudioFifo();
+
+  // Appends all audio channel data from |source| to the FIFO.
+  // Returns false if the allocated space is not sufficient.

[Chris Rogers, 2012/09/04 20:07:20]

and I would add that "partial data is not written" for this failure case

[henrika (OOO until Aug 14), 2012/09/05 10:14:36]

Done.

+  bool Append(const AudioBus* source);

[DaleCurtis, 2012/09/05 10:34:18]

Do we expect a recoverable / normal case where Append() (or Remove()) will fail?
Otherwise it's probably better to simply CHECK() and have these return void.

[henrika (OOO until Aug 14), 2012/09/05 12:49:04]

I discussed this off-line with Chris and his suggestion was to make it bool. It
feels scary to add CHECK() in parts that will run in the browser process and are
connected to audio buffering. Could we really do that even if we wanted?

[DaleCurtis, 2012/09/05 13:09:22]

I'd just prefer to not have to worry about the return value (vs CHECK()) if this
is a state we could only reach by programmer error. I admit I haven't thought it
all the way through though, so if you and Chris have already hashed it out,
that's fine with me.

+
+  // Removes |destination->frames()| audio frames from the FIFO and copies
+  // them to |destination|.

[Chris Rogers, 2012/09/04 20:07:20]

and "Returns false if there's not enough data, not copying partial data"

[henrika (OOO until Aug 14), 2012/09/05 10:14:36]

Done.

+  bool Remove(AudioBus* destination);

[DaleCurtis, 2012/09/05 10:34:18]

Kind of a misleading name, since it doesn't remove the data, just copies it out.
The comment reinforces this by stating that it first removes and then copies the
frames out.

The original Push/Consume names might be better in the sense that they work
around the issue by making the implementation details opaque.

[henrika (OOO until Aug 14), 2012/09/05 12:49:04]

Chris and I discussed this as well and landed at the current naming. You have a
point. Let me think. 

+
+  // Empties the FIFO without deallocating any memory.
+  void Clear();
+
+  // Helper methods.
+  int size() const { return size_; }

[Chris Rogers, 2012/09/04 20:07:20]

I would tend to call this "available()" or "frames_available()" here, since
"size()" makes me think of the total FIFO size (what you call max_size() below)

[henrika (OOO until Aug 14), 2012/09/05 10:14:36]

I will wait for feedback from Dale as well. But available() does not feel OK to
me since I am not referring to "free space in the FIFO" but "how much is used".

[DaleCurtis, 2012/09/05 10:34:18]

+1 for frames_available(). size() is inconsistent with AudioBus, so I'd prefer
whatever the name chosen, frames() is included in it.

frames_available makes the most sense to me since you are advertising the number
of frames that are available for removal/consuming.

[henrika (OOO until Aug 14), 2012/09/05 12:49:04]

OK, now I understand the point of using available. I'll change this name. Check
out the next version.

+  int max_size() const { return audio_bus_->frames(); }

[DaleCurtis, 2012/09/05 10:34:18]

Needed publicly?

[henrika (OOO until Aug 14), 2012/09/05 12:49:04]

nope; I just liked it. Will remove.

+  int channels() const { return audio_bus_->channels(); }

[DaleCurtis, 2012/09/05 10:34:18]

Ditto.

[henrika (OOO until Aug 14), 2012/09/05 12:49:04]

got it, will remove.

+  bool empty() const { return (size_ == 0); }
+  bool full() const { return (size_ == max_size()); }

[DaleCurtis, 2012/09/05 10:34:18]

Ditto / needed at all?

[henrika (OOO until Aug 14), 2012/09/05 12:49:04]

I dropped two of them but would like to keep these. They are useful for testing
and would like to avoid making the test a friend. Other ideas?

[DaleCurtis, 2012/09/05 13:09:22]

Seems you dropped them anyway? If you need them that's fine with me, but
friending tests is usually the way to go to avoid unnecessary API.

+
+ private:
+  // The actual FIFO is an audio bus implemented as a circular queue.

[Chris Rogers, 2012/09/04 20:07:20]

queue -> buffer

[henrika (OOO until Aug 14), 2012/09/05 10:14:36]

Done.

+  scoped_ptr<AudioBus> audio_bus_;
+
+  // Number of actual elements in the FIFO.
+  int size_;

[Chris Rogers, 2012/09/04 20:07:20]

size_ -> available_

[henrika (OOO until Aug 14), 2012/09/05 10:14:36]

I am inspired by:

STL where queue::size() returns the number of elements in the queue.

+
+  // Current read position.
+  int read_pos_;
+
+  // Current write position.
+  int write_pos_;
+
+  DISALLOW_COPY_AND_ASSIGN(AudioFifo);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_BASE_AUDIO_FIFO_H_