Replace fifo C API and implementation by C++
Improvements to library:
- Replace Android atomics by C++ atomics
- struct -> class
- init -> constructor, deinit -> destructor
- Avoid signed arithmetic for security
- Fix usage in test
- Add README
- Improve error recovery by returning a status_t if indices are corrupt
- Fix bug in diff when assertions enabled
- Add local indices in preparation for multi-reader and protected pages
- Add more documentation for internal methods
Improvements to unit test:
- More error checks
- Fix double delete of fifoBuffer
- Support 8-bit wav files
- Enable assertion checks
Change-Id: I8b391b17084084d8e692765ed210a35aaa0b6747
diff --git a/audio_utils/README.md b/audio_utils/README.md
new file mode 100644
index 0000000..df78972
--- /dev/null
+++ b/audio_utils/README.md
@@ -0,0 +1,8 @@
+# How to build and view documentation
+
+* doxygen Doxyfile
+* cd html
+* python -m SimpleHTTPServer
+* open in web browser
+ http://localhost:8000/classaudio__utils__fifo.html
+* when done: rm -rf html
diff --git a/audio_utils/fifo.cpp b/audio_utils/fifo.cpp
index c818a50..2d70b95 100644
--- a/audio_utils/fifo.cpp
+++ b/audio_utils/fifo.cpp
@@ -21,114 +21,126 @@
#include <string.h>
#include <audio_utils/fifo.h>
#include <audio_utils/roundup.h>
-#include <cutils/atomic.h>
#include <cutils/log.h>
+#include <utils/Errors.h>
-void audio_utils_fifo_init(struct audio_utils_fifo *fifo, size_t frameCount, size_t frameSize,
- void *buffer)
+audio_utils_fifo::audio_utils_fifo(uint32_t frameCount, uint32_t frameSize, void *buffer) :
+ mFrameCount(frameCount), mFrameCountP2(roundup(frameCount)),
+ mFudgeFactor(mFrameCountP2 - mFrameCount), mFrameSize(frameSize), mBuffer(buffer),
+ mLocalFront(0), mLocalRear(0), mSharedFront(0), mSharedRear(0)
{
- // We would need a 64-bit roundup to support larger frameCount.
- ALOG_ASSERT(fifo != NULL && frameCount > 0 && frameSize > 0 && buffer != NULL);
- fifo->mFrameCount = frameCount;
- fifo->mFrameCountP2 = roundup(frameCount);
- fifo->mFudgeFactor = fifo->mFrameCountP2 - fifo->mFrameCount;
- fifo->mFrameSize = frameSize;
- fifo->mBuffer = buffer;
- fifo->mFront = 0;
- fifo->mRear = 0;
+ // maximum value of frameCount * frameSize is INT_MAX (2^31 - 1), not 2^31, because we need to
+ // be able to distinguish successful and error return values from read and write.
+ ALOG_ASSERT(frameCount > 0 && frameSize > 0 && buffer != NULL &&
+ frameCount <= ((uint32_t) INT_MAX) / frameSize);
}
-void audio_utils_fifo_deinit(struct audio_utils_fifo *fifo __unused)
+audio_utils_fifo::~audio_utils_fifo()
{
}
-// Return a new index as the sum of an old index (either mFront or mRear) and a specified increment.
-static inline int32_t audio_utils_fifo_sum(struct audio_utils_fifo *fifo, int32_t index,
- uint32_t increment)
+uint32_t audio_utils_fifo::sum(uint32_t index, uint32_t increment)
+ __attribute__((no_sanitize("integer")))
{
- if (fifo->mFudgeFactor) {
- uint32_t mask = fifo->mFrameCountP2 - 1;
- ALOG_ASSERT((index & mask) < fifo->mFrameCount);
- ALOG_ASSERT(/*0 <= increment &&*/ increment <= fifo->mFrameCountP2);
- if ((index & mask) + increment >= fifo->mFrameCount) {
- increment += fifo->mFudgeFactor;
+ if (mFudgeFactor) {
+ uint32_t mask = mFrameCountP2 - 1;
+ ALOG_ASSERT((index & mask) < mFrameCount);
+ ALOG_ASSERT(increment <= mFrameCountP2);
+ if ((index & mask) + increment >= mFrameCount) {
+ increment += mFudgeFactor;
}
index += increment;
- ALOG_ASSERT((index & mask) < fifo->mFrameCount);
+ ALOG_ASSERT((index & mask) < mFrameCount);
return index;
} else {
return index + increment;
}
}
-// Return the difference between two indices: rear - front, where 0 <= difference <= mFrameCount.
-static inline size_t audio_utils_fifo_diff(struct audio_utils_fifo *fifo, int32_t rear,
- int32_t front)
+int32_t audio_utils_fifo::diff(uint32_t rear, uint32_t front)
+ __attribute__((no_sanitize("integer")))
{
- int32_t diff = rear - front;
- if (fifo->mFudgeFactor) {
- uint32_t mask = ~(fifo->mFrameCountP2 - 1);
- int32_t genDiff = (rear & mask) - (front & mask);
+ uint32_t diff = rear - front;
+ if (mFudgeFactor) {
+ uint32_t mask = mFrameCountP2 - 1;
+ uint32_t rearMasked = rear & mask;
+ uint32_t frontMasked = front & mask;
+ if (rearMasked >= mFrameCount || frontMasked >= mFrameCount) {
+ return (int32_t) android::UNKNOWN_ERROR;
+ }
+ uint32_t genDiff = (rear & ~mask) - (front & ~mask);
if (genDiff != 0) {
- ALOG_ASSERT(genDiff == (int32_t) fifo->mFrameCountP2);
- diff -= fifo->mFudgeFactor;
+ if (genDiff > mFrameCountP2) {
+ return (int32_t) android::UNKNOWN_ERROR;
+ }
+ diff -= mFudgeFactor;
}
}
// FIFO should not be overfull
- ALOG_ASSERT(0 <= diff && diff <= (int32_t) fifo->mFrameCount);
- return (size_t) diff;
+ if (diff > mFrameCount) {
+ return (int32_t) android::UNKNOWN_ERROR;
+ }
+ return (int32_t) diff;
}
-ssize_t audio_utils_fifo_write(struct audio_utils_fifo *fifo, const void *buffer, size_t count)
+ssize_t audio_utils_fifo::write(const void *buffer, size_t count)
+ __attribute__((no_sanitize("integer")))
{
- int32_t front = android_atomic_acquire_load(&fifo->mFront);
- int32_t rear = fifo->mRear;
- size_t availToWrite = fifo->mFrameCount - audio_utils_fifo_diff(fifo, rear, front);
+ uint32_t front = (uint32_t) atomic_load_explicit(&mSharedFront, std::memory_order_acquire);
+ uint32_t rear = mLocalRear;
+ int32_t filled = diff(rear, front);
+ if (filled < 0) {
+ return (ssize_t) filled;
+ }
+ size_t availToWrite = (size_t) mFrameCount - (size_t) filled;
if (availToWrite > count) {
availToWrite = count;
}
- rear &= fifo->mFrameCountP2 - 1;
- size_t part1 = fifo->mFrameCount - rear;
+ uint32_t rearMasked = rear & (mFrameCountP2 - 1);
+ size_t part1 = mFrameCount - rearMasked;
if (part1 > availToWrite) {
part1 = availToWrite;
}
if (part1 > 0) {
- memcpy((char *) fifo->mBuffer + (rear * fifo->mFrameSize), buffer,
- part1 * fifo->mFrameSize);
+ memcpy((char *) mBuffer + (rearMasked * mFrameSize), buffer, part1 * mFrameSize);
size_t part2 = availToWrite - part1;
if (part2 > 0) {
- memcpy(fifo->mBuffer, (char *) buffer + (part1 * fifo->mFrameSize),
- part2 * fifo->mFrameSize);
+ memcpy(mBuffer, (char *) buffer + (part1 * mFrameSize), part2 * mFrameSize);
}
- android_atomic_release_store(audio_utils_fifo_sum(fifo, fifo->mRear, availToWrite),
- &fifo->mRear);
+ mLocalRear = sum(rear, availToWrite);
+ atomic_store_explicit(&mSharedRear, (uint_fast32_t) mLocalRear,
+ std::memory_order_release);
}
return availToWrite;
}
-ssize_t audio_utils_fifo_read(struct audio_utils_fifo *fifo, void *buffer, size_t count)
+ssize_t audio_utils_fifo::read(void *buffer, size_t count)
+ __attribute__((no_sanitize("integer")))
{
- int32_t rear = android_atomic_acquire_load(&fifo->mRear);
- int32_t front = fifo->mFront;
- size_t availToRead = audio_utils_fifo_diff(fifo, rear, front);
+ uint32_t rear = (uint32_t) atomic_load_explicit(&mSharedRear, std::memory_order_acquire);
+ uint32_t front = mLocalFront;
+ int32_t filled = diff(rear, front);
+ if (filled < 0) {
+ return (ssize_t) filled;
+ }
+ size_t availToRead = (size_t) filled;
if (availToRead > count) {
availToRead = count;
}
- front &= fifo->mFrameCountP2 - 1;
- size_t part1 = fifo->mFrameCount - front;
+ uint32_t frontMasked = front & (mFrameCountP2 - 1);
+ size_t part1 = mFrameCount - frontMasked;
if (part1 > availToRead) {
part1 = availToRead;
}
if (part1 > 0) {
- memcpy(buffer, (char *) fifo->mBuffer + (front * fifo->mFrameSize),
- part1 * fifo->mFrameSize);
+ memcpy(buffer, (char *) mBuffer + (frontMasked * mFrameSize), part1 * mFrameSize);
size_t part2 = availToRead - part1;
if (part2 > 0) {
- memcpy((char *) buffer + (part1 * fifo->mFrameSize), fifo->mBuffer,
- part2 * fifo->mFrameSize);
+ memcpy((char *) buffer + (part1 * mFrameSize), mBuffer, part2 * mFrameSize);
}
- android_atomic_release_store(audio_utils_fifo_sum(fifo, fifo->mFront, availToRead),
- &fifo->mFront);
+ mLocalFront = sum(front, availToRead);
+ atomic_store_explicit(&mSharedFront, (uint_fast32_t) mLocalFront,
+ std::memory_order_release);
}
return availToRead;
}
diff --git a/audio_utils/include/audio_utils/fifo.h b/audio_utils/include/audio_utils/fifo.h
index f882368..8ac1729 100644
--- a/audio_utils/include/audio_utils/fifo.h
+++ b/audio_utils/include/audio_utils/fifo.h
@@ -17,58 +17,63 @@
#ifndef ANDROID_AUDIO_FIFO_H
#define ANDROID_AUDIO_FIFO_H
+#include <atomic>
#include <stdlib.h>
-// FIXME use atomic_int_least32_t and new atomic operations instead of legacy Android ones
-// #include <stdatomic.h>
-
-#ifdef __cplusplus
-extern "C" {
+#ifndef __cplusplus
+#error C API is no longer supported
#endif
// Single writer, single reader non-blocking FIFO.
// Writer and reader must be in same process.
// No user-serviceable parts within.
-struct audio_utils_fifo {
- // These fields are const after initialization
- size_t mFrameCount; // max number of significant frames to be stored in the FIFO > 0
- size_t mFrameCountP2; // roundup(mFrameCount)
- size_t mFudgeFactor; // mFrameCountP2 - mFrameCount, the number of "wasted" frames after
- // the end of mBuffer. Only the indices are wasted, not any memory.
- size_t mFrameSize; // size of each frame in bytes
- void *mBuffer; // pointer to caller-allocated buffer of size mFrameCount frames
+class audio_utils_fifo {
- volatile int32_t mFront; // frame index of first frame slot available to read, or read index
- volatile int32_t mRear; // frame index of next frame slot available to write, or write index
-};
+public:
/**
* Initialize a FIFO object.
*
- * \param fifo Pointer to the FIFO object.
* \param frameCount Max number of significant frames to be stored in the FIFO > 0.
* If writes and reads always use the same count, and that count is a divisor of
* frameCount, then the writes and reads will never do a partial transfer.
- * \param frameSize Size of each frame in bytes.
+ * \param frameSize Size of each frame in bytes > 0, and frameSize * frameCount <= INT_MAX.
* \param buffer Pointer to a caller-allocated buffer of frameCount frames.
*/
-void audio_utils_fifo_init(struct audio_utils_fifo *fifo, size_t frameCount, size_t frameSize,
- void *buffer);
+ audio_utils_fifo(uint32_t frameCount, uint32_t frameSize, void *buffer);
/**
* De-initialize a FIFO object.
- *
- * \param fifo Pointer to the FIFO object.
*/
-void audio_utils_fifo_deinit(struct audio_utils_fifo *fifo);
+ ~audio_utils_fifo();
+
+private:
+
+ // These fields are const after initialization
+ const uint32_t mFrameCount; // max number of significant frames to be stored in the FIFO > 0
+ const uint32_t mFrameCountP2; // roundup(mFrameCount)
+ const uint32_t mFudgeFactor; // mFrameCountP2 - mFrameCount, the number of "wasted" frames
+ // after the end of mBuffer. Only the indices are wasted, not any
+ // memory.
+ const uint32_t mFrameSize; // size of each frame in bytes
+ void * const mBuffer; // pointer to caller-allocated buffer of size mFrameCount frames
+
+ // These are accessed by one side only using ordinary operations
+ uint32_t mLocalFront; // frame index of first frame slot available to read, or read index
+ uint32_t mLocalRear; // frame index of next frame slot available to write, or write index
+
+ // These are accessed by both sides using atomic operations
+ std::atomic_uint_fast32_t mSharedFront;
+ std::atomic_uint_fast32_t mSharedRear;
+
+public:
/**
* Write to FIFO.
*
- * \param fifo Pointer to the FIFO object.
- * \param buffer Pointer to source buffer containing 'count' frames of data.
- * \param count Desired number of frames to write.
+ * \param buffer Pointer to source buffer containing 'count' frames of data.
+ * \param count Desired number of frames to write.
*
* \return actual number of frames written <= count.
*
@@ -76,13 +81,12 @@
* or partial if the FIFO was almost full.
* A negative return value indicates an error. Currently there are no errors defined.
*/
-ssize_t audio_utils_fifo_write(struct audio_utils_fifo *fifo, const void *buffer, size_t count);
+ ssize_t write(const void *buffer, size_t count);
/** Read from FIFO.
*
- * \param fifo Pointer to the FIFO object.
- * \param buffer Pointer to destination buffer to be filled with up to 'count' frames of data.
- * \param count Desired number of frames to read.
+ * \param buffer Pointer to destination buffer to be filled with up to 'count' frames of data.
+ * \param count Desired number of frames to read.
*
* \return actual number of frames read <= count.
*
@@ -90,10 +94,28 @@
* or partial if the FIFO was almost empty.
* A negative return value indicates an error. Currently there are no errors defined.
*/
-ssize_t audio_utils_fifo_read(struct audio_utils_fifo *fifo, void *buffer, size_t count);
+ ssize_t read(void *buffer, size_t count);
-#ifdef __cplusplus
-}
-#endif
+private:
+
+/** Return a new index as the sum of a validated index and a specified increment.
+ *
+ * \param index Caller should supply a validated mFront or mRear.
+ * \param increment Value to be added to the index <= mFrameCount.
+ *
+ * \return the sum of index plus increment.
+ */
+ uint32_t sum(uint32_t index, uint32_t increment);
+
+/** Return the difference between two indices: rear - front.
+ *
+ * \param rear Caller should supply an unvalidated mRear.
+ * \param front Caller should supply an unvalidated mFront.
+ *
+ * \return the zero or positive difference <= mFrameCount, or a negative error code.
+ */
+ int32_t diff(uint32_t rear, uint32_t front);
+
+};
#endif // !ANDROID_AUDIO_FIFO_H
diff --git a/audio_utils/tests/Android.mk b/audio_utils/tests/Android.mk
index 0db09dc..c085997 100644
--- a/audio_utils/tests/Android.mk
+++ b/audio_utils/tests/Android.mk
@@ -44,5 +44,5 @@
LOCAL_MODULE := limiter_tests
LOCAL_C_INCLUDES := $(call include-path-for, audio-utils)
LOCAL_STATIC_LIBRARIES := libaudioutils
-LOCAL_CFLAGS := -Werror -Wall
+LOCAL_CFLAGS := -Werror -Wall -UNDEBUG
include $(BUILD_HOST_EXECUTABLE)
diff --git a/audio_utils/tests/fifo_tests.cpp b/audio_utils/tests/fifo_tests.cpp
index 99e73c9..637d60c 100644
--- a/audio_utils/tests/fifo_tests.cpp
+++ b/audio_utils/tests/fifo_tests.cpp
@@ -23,6 +23,10 @@
#include <audio_utils/fifo.h>
#include <audio_utils/sndfile.h>
+#ifndef min
+#define min(x, y) (((x) < (y)) ? (x) : (y))
+#endif
+
int main(int argc, char **argv)
{
size_t frameCount = 256;
@@ -51,7 +55,7 @@
if (argc - i != 2) {
usage:
- fprintf(stderr, "usage: %s [-c#] in.wav out.wav\n", argv[0]);
+ fprintf(stderr, "usage: %s [-c#] [-r#] [-w#] in.wav out.wav\n", argv[0]);
return EXIT_FAILURE;
}
char *inputFile = argv[i];
@@ -64,12 +68,11 @@
perror(inputFile);
return EXIT_FAILURE;
}
- // sf_readf_short() does conversion, so not strictly necessary to check the file format.
- // But I want to do "cmp" on input and output files afterwards,
- // and it is easier if they are all the same format.
- // Enforcing that everything is 16-bit is convenient for this.
- if ((sfinfoin.format & (SF_FORMAT_TYPEMASK | SF_FORMAT_SUBMASK)) !=
- (SF_FORMAT_WAV | SF_FORMAT_PCM_16)) {
+ switch (sfinfoin.format & (SF_FORMAT_TYPEMASK | SF_FORMAT_SUBMASK)) {
+ case SF_FORMAT_WAV | SF_FORMAT_PCM_16:
+ case SF_FORMAT_WAV | SF_FORMAT_PCM_U8:
+ break;
+ default:
fprintf(stderr, "%s: unsupported format\n", inputFile);
sf_close(sfin);
return EXIT_FAILURE;
@@ -87,9 +90,8 @@
short *outputBuffer = new short[sfinfoin.frames * sfinfoin.channels];
size_t framesWritten = 0;
size_t framesRead = 0;
- struct audio_utils_fifo fifo;
short *fifoBuffer = new short[frameCount * sfinfoin.channels];
- audio_utils_fifo_init(&fifo, frameCount, frameSize, fifoBuffer);
+ audio_utils_fifo fifo(frameCount, frameSize, fifoBuffer);
int fifoWriteCount = 0, fifoReadCount = 0;
int fifoFillLevel = 0, minFillLevel = INT_MAX, maxFillLevel = INT_MIN;
for (;;) {
@@ -103,12 +105,16 @@
framesToWrite = maxFramesPerWrite;
}
framesToWrite = rand() % (framesToWrite + 1);
- ssize_t actualWritten = audio_utils_fifo_write(&fifo,
+ ssize_t actualWritten = fifo.write(
&inputBuffer[framesWritten * sfinfoin.channels], framesToWrite);
if (actualWritten < 0 || (size_t) actualWritten > framesToWrite) {
fprintf(stderr, "write to FIFO failed\n");
break;
}
+ if (actualWritten < min((int) frameCount - fifoFillLevel, (int) framesToWrite)) {
+ fprintf(stderr, "only wrote %d when should have written min(%d, %d)\n",
+ (int) actualWritten, (int) frameCount - fifoFillLevel, (int) framesToWrite);
+ }
framesWritten += actualWritten;
if (actualWritten > 0) {
fifoWriteCount++;
@@ -116,20 +122,26 @@
fifoFillLevel += actualWritten;
if (fifoFillLevel > maxFillLevel) {
maxFillLevel = fifoFillLevel;
- if (maxFillLevel > (int) frameCount)
+ if (maxFillLevel > (int) frameCount) {
+ printf("maxFillLevel=%d > frameCount=%d\n", maxFillLevel, (int) frameCount);
abort();
+ }
}
if (framesToRead > maxFramesPerRead) {
framesToRead = maxFramesPerRead;
}
framesToRead = rand() % (framesToRead + 1);
- ssize_t actualRead = audio_utils_fifo_read(&fifo,
+ ssize_t actualRead = fifo.read(
&outputBuffer[framesRead * sfinfoin.channels], framesToRead);
if (actualRead < 0 || (size_t) actualRead > framesToRead) {
fprintf(stderr, "read from FIFO failed\n");
break;
}
+ if (actualRead < min(fifoFillLevel, (int) framesToRead)) {
+ fprintf(stderr, "only read %d when should have read min(%d, %d)\n",
+ (int) actualRead, fifoFillLevel, (int) framesToRead);
+ }
framesRead += actualRead;
if (actualRead > 0) {
fifoReadCount++;
@@ -137,14 +149,19 @@
fifoFillLevel -= actualRead;
if (fifoFillLevel < minFillLevel) {
minFillLevel = fifoFillLevel;
- if (minFillLevel < 0)
+ if (minFillLevel < 0) {
+ printf("minFillLevel=%d < 0\n", minFillLevel);
abort();
+ }
}
}
+ delete[] inputBuffer;
+ inputBuffer = NULL;
+ delete[] fifoBuffer;
+ fifoBuffer = NULL;
+
printf("FIFO non-empty writes: %d, non-empty reads: %d\n", fifoWriteCount, fifoReadCount);
printf("fill=%d, min=%d, max=%d\n", fifoFillLevel, minFillLevel, maxFillLevel);
- audio_utils_fifo_deinit(&fifo);
- delete[] fifoBuffer;
SF_INFO sfinfoout;
memset(&sfinfoout, 0, sizeof(sfinfoout));
@@ -157,9 +174,9 @@
return EXIT_FAILURE;
}
sf_count_t actualWritten = sf_writef_short(sfout, outputBuffer, framesRead);
- delete[] inputBuffer;
delete[] outputBuffer;
- delete[] fifoBuffer;
+ outputBuffer = NULL;
+
if (actualWritten != (sf_count_t) framesRead) {
fprintf(stderr, "%s: unexpected error\n", outputFile);
sf_close(sfout);