fifo: address concerns about signed vs unsigned
Limit available to capacity of buffer.
Remove threshold.
diff --git a/src/fifo/FifoBuffer.cpp b/src/fifo/FifoBuffer.cpp
index 2655915..501fee6 100644
--- a/src/fifo/FifoBuffer.cpp
+++ b/src/fifo/FifoBuffer.cpp
@@ -18,6 +18,7 @@
#include <time.h>
#include <memory.h>
#include <cassert>
+#include <algorithm>
#include "common/OboeDebug.h"
#include "fifo/FifoControllerBase.h"
@@ -29,16 +30,12 @@
namespace oboe {
FifoBuffer::FifoBuffer(uint32_t bytesPerFrame, uint32_t capacityInFrames)
- : mFrameCapacity(capacityInFrames)
- , mBytesPerFrame(bytesPerFrame)
+ : mBytesPerFrame(bytesPerFrame)
, mStorage(nullptr)
, mFramesReadCount(0)
, mFramesUnderrunCount(0)
- , mUnderrunCount(0)
{
- assert(bytesPerFrame > 0);
- assert(capacityInFrames > 0);
- mFifo = new FifoController(capacityInFrames, capacityInFrames);
+ mFifo = new FifoController(capacityInFrames);
// allocate buffer
int32_t bytesPerBuffer = bytesPerFrame * capacityInFrames;
mStorage = new uint8_t[bytesPerBuffer];
@@ -47,23 +44,18 @@
capacityInFrames, bytesPerFrame);
}
-FifoBuffer::FifoBuffer( uint32_t bytesPerFrame,
- uint32_t capacityInFrames,
- int64_t * readIndexAddress,
- int64_t * writeIndexAddress,
- uint8_t * dataStorageAddress
+FifoBuffer::FifoBuffer( uint32_t bytesPerFrame,
+ uint32_t capacityInFrames,
+ int64_t *readIndexAddress,
+ int64_t *writeIndexAddress,
+ uint8_t *dataStorageAddress
)
- : mFrameCapacity(capacityInFrames)
- , mBytesPerFrame(bytesPerFrame)
+ : mBytesPerFrame(bytesPerFrame)
, mStorage(dataStorageAddress)
, mFramesReadCount(0)
, mFramesUnderrunCount(0)
- , mUnderrunCount(0)
{
- assert(bytesPerFrame > 0);
- assert(capacityInFrames > 0);
mFifo = new FifoControllerIndirect(capacityInFrames,
- capacityInFrames,
readIndexAddress,
writeIndexAddress);
mStorage = dataStorageAddress;
@@ -85,22 +77,19 @@
}
int32_t FifoBuffer::read(void *buffer, int32_t numFrames) {
- int32_t framesAvailable = mFifo->getFullFramesAvailable();
- int32_t framesToRead = numFrames;
- // Is there enough data in the FIFO
- if (framesToRead > framesAvailable) {
- framesToRead = framesAvailable;
- }
- if (framesToRead <= 0) {
+ if (numFrames <= 0) {
return 0;
}
+ uint32_t framesToRead = static_cast<uint32_t>(numFrames);
+ uint32_t framesAvailable = mFifo->getFullFramesAvailable();
+ framesToRead = std::min(framesToRead, framesAvailable);
uint32_t readIndex = mFifo->getReadIndex();
uint8_t *destination = reinterpret_cast<uint8_t *>(buffer);
uint8_t *source = &mStorage[convertFramesToBytes(readIndex)];
- if ((readIndex + framesToRead) > mFrameCapacity) {
+ if ((readIndex + framesToRead) > mFifo->getFrameCapacity()) {
// read in two parts, first part here is at the end of the mStorage buffer
- uint32_t frames1 = mFrameCapacity - readIndex;
+ uint32_t frames1 = mFifo->getFrameCapacity() - readIndex;
int32_t numBytes = convertFramesToBytes(frames1);
if (numBytes < 0) {
return static_cast<int32_t>(Result::ErrorOutOfRange);
@@ -128,22 +117,21 @@
return framesToRead;
}
-int32_t FifoBuffer::write(const void *buffer, int32_t framesToWrite) {
- int32_t framesAvailable = mFifo->getEmptyFramesAvailable();
- if (framesToWrite > framesAvailable) {
- framesToWrite = framesAvailable;
- }
- if (framesToWrite <= 0) {
+int32_t FifoBuffer::write(const void *buffer, int32_t numFrames) {
+ if (numFrames <= 0) {
return 0;
}
+ uint32_t framesToWrite = static_cast<uint32_t>(numFrames);
+ uint32_t framesAvailable = mFifo->getEmptyFramesAvailable();
+ framesToWrite = std::min(framesToWrite, framesAvailable);
uint32_t writeIndex = mFifo->getWriteIndex();
int byteIndex = convertFramesToBytes(writeIndex);
const uint8_t *source = reinterpret_cast<const uint8_t *>(buffer);
uint8_t *destination = &mStorage[byteIndex];
- if ((writeIndex + framesToWrite) > mFrameCapacity) {
+ if ((writeIndex + framesToWrite) > mFifo->getFrameCapacity()) {
// write in two parts, first part here
- int frames1 = mFrameCapacity - writeIndex;
+ int frames1 = mFifo->getFrameCapacity() - writeIndex;
int32_t numBytes = convertFramesToBytes(frames1);
if (numBytes < 0) {
return static_cast<int32_t>(Result::ErrorOutOfRange);
@@ -172,14 +160,12 @@
}
int32_t FifoBuffer::readNow(void *buffer, int32_t numFrames) {
- int32_t framesLeft = numFrames;
int32_t framesRead = read(buffer, numFrames);
- framesLeft -= framesRead;
+ int32_t framesLeft = numFrames - framesRead;
mFramesReadCount += framesRead;
mFramesUnderrunCount += framesLeft;
// Zero out any samples we could not set.
if (framesLeft > 0) {
- mUnderrunCount++;
uint8_t *destination = reinterpret_cast<uint8_t *>(buffer);
destination += convertFramesToBytes(framesRead); // point to first byte not set
int32_t bytesToZero = convertFramesToBytes(framesLeft);
@@ -189,16 +175,9 @@
return framesRead;
}
-uint32_t FifoBuffer::getThresholdFrames() const {
- return mFifo->getThreshold();
-}
uint32_t FifoBuffer::getBufferCapacityInFrames() const {
return mFifo->getFrameCapacity();
}
-void FifoBuffer::setThresholdFrames(uint32_t threshold) {
- mFifo->setThreshold(threshold);
-}
-
} // namespace oboe
\ No newline at end of file
diff --git a/src/fifo/FifoBuffer.h b/src/fifo/FifoBuffer.h
index ac0b6e1..4512b16 100644
--- a/src/fifo/FifoBuffer.h
+++ b/src/fifo/FifoBuffer.h
@@ -50,15 +50,11 @@
int32_t write(const void *source, int32_t framesToWrite);
- uint32_t getThresholdFrames() const;
-
- void setThresholdFrames(uint32_t threshold);
-
uint32_t getBufferCapacityInFrames() const;
/**
* Calls read(). If all of the frames cannot be read then the remainder of the buffer
- * is set to zero and the underruncount is incremented.
+ * is set to zero.
*
* @param destination
* @param framesToRead number of frames requested
@@ -66,8 +62,6 @@
*/
int32_t readNow(void *destination, int32_t numFrames);
- uint32_t getUnderrunCount() const { return mUnderrunCount; }
-
FifoControllerBase *getFifoControllerBase() { return mFifo; }
uint32_t getBytesPerFrame() const {
@@ -90,14 +84,13 @@
}
private:
- uint32_t mFrameCapacity;
+// uint32_t mFrameCapacity;
uint32_t mBytesPerFrame;
uint8_t* mStorage;
bool mStorageOwned; // did this object allocate the storage?
FifoControllerBase *mFifo;
uint64_t mFramesReadCount;
uint64_t mFramesUnderrunCount;
- uint32_t mUnderrunCount; // count of underruns when reading the buffer
};
} // namespace oboe
diff --git a/src/fifo/FifoController.cpp b/src/fifo/FifoController.cpp
index a4bcc7b..b625286 100644
--- a/src/fifo/FifoController.cpp
+++ b/src/fifo/FifoController.cpp
@@ -21,8 +21,8 @@
namespace oboe {
-FifoController::FifoController(uint32_t numFrames, uint32_t threshold)
- : FifoControllerBase(numFrames, threshold)
+FifoController::FifoController(uint32_t numFrames)
+ : FifoControllerBase(numFrames)
{
setReadCounter(0);
setWriteCounter(0);
diff --git a/src/fifo/FifoController.h b/src/fifo/FifoController.h
index ca1e7aa..6562e6d 100644
--- a/src/fifo/FifoController.h
+++ b/src/fifo/FifoController.h
@@ -29,10 +29,9 @@
class FifoController : public FifoControllerBase
{
public:
- FifoController(uint32_t bufferSize, uint32_t threshold);
+ FifoController(uint32_t bufferSize);
virtual ~FifoController() = default;
- // TODO review use atomics or memory barriers
virtual uint64_t getReadCounter() const override {
return mReadCounter.load(std::memory_order_acquire);
}
@@ -53,8 +52,8 @@
}
private:
- std::atomic<uint64_t> mReadCounter;
- std::atomic<uint64_t> mWriteCounter;
+ std::atomic<uint64_t> mReadCounter{};
+ std::atomic<uint64_t> mWriteCounter{};
};
} // namespace oboe
diff --git a/src/fifo/FifoControllerBase.cpp b/src/fifo/FifoControllerBase.cpp
index 6025f11..6aceea0 100644
--- a/src/fifo/FifoControllerBase.cpp
+++ b/src/fifo/FifoControllerBase.cpp
@@ -18,23 +18,36 @@
#include <cassert>
#include <sys/types.h>
+#include <algorithm>
#include "FifoControllerBase.h"
#include "common/OboeDebug.h"
namespace oboe {
-FifoControllerBase::FifoControllerBase(uint32_t totalFrames, uint32_t threshold)
- : mTotalFrames(totalFrames)
- , mThreshold(threshold)
+FifoControllerBase::FifoControllerBase(uint32_t capacityInFrames)
+ : mTotalFrames(capacityInFrames)
{
+ // Avoid ridiculously large buffers and the arithmetic wraparound issues that can follow.
+ assert(capacityInFrames <= (UINT32_MAX / 4));
}
-int32_t FifoControllerBase::getFullFramesAvailable() const {
- return static_cast<int32_t>(getWriteCounter() - getReadCounter());
+uint32_t FifoControllerBase::getFullFramesAvailable() const {
+ uint64_t writeCounter = getWriteCounter();
+ uint64_t readCounter = getReadCounter();
+ if (readCounter > writeCounter) {
+ return 0;
+ }
+ uint64_t delta = writeCounter - readCounter;
+ if (delta >= mTotalFrames) {
+ return mTotalFrames;
+ }
+ // delta is now guaranteed to fit within the range of a uint32_t
+ return static_cast<uint32_t>(delta);
}
uint32_t FifoControllerBase::getReadIndex() const {
+ // % works with non-power of two sizes
return static_cast<uint32_t>(getReadCounter() % mTotalFrames);
}
@@ -42,22 +55,17 @@
incrementReadCounter(numFrames);
}
-int32_t FifoControllerBase::getEmptyFramesAvailable() const {
- int32_t fullFramesAvailable = getFullFramesAvailable();
- int32_t available = static_cast<int32_t>(mThreshold - fullFramesAvailable);
- return available;
+uint32_t FifoControllerBase::getEmptyFramesAvailable() const {
+ return static_cast<uint32_t>(mTotalFrames - getFullFramesAvailable());
}
uint32_t FifoControllerBase::getWriteIndex() const {
- return static_cast<uint32_t>(getWriteCounter() % mTotalFrames); // % works with non-power of two sizes
+ // % works with non-power of two sizes
+ return static_cast<uint32_t>(getWriteCounter() % mTotalFrames);
}
void FifoControllerBase::advanceWriteIndex(uint32_t numFrames) {
incrementWriteCounter(numFrames);
}
-void FifoControllerBase::setThreshold(uint32_t threshold) {
- mThreshold = threshold;
-}
-
} // namespace oboe
diff --git a/src/fifo/FifoControllerBase.h b/src/fifo/FifoControllerBase.h
index 4ee6414..c8041e0 100644
--- a/src/fifo/FifoControllerBase.h
+++ b/src/fifo/FifoControllerBase.h
@@ -36,18 +36,19 @@
public:
/**
- * Constructor for FifoControllerBase
- * @param numFrames Size of the circular buffer in frames. Must be a power of 2.
+ * @param totalFrames capacity of the circular buffer in frames.
*/
- FifoControllerBase(uint32_t totalFrames, uint32_t threshold);
+ FifoControllerBase(uint32_t totalFrames);
virtual ~FifoControllerBase() = default;
/**
- * This may be negative if an unthrottled reader has read beyond the available data.
- * @return number of valid frames available to read. Never read more than this.
+ * The frames available to read will be calculated from the read and write counters.
+ * The result will be clipped to the capacity of the buffer.
+ * If the buffer has underflowed then this will return zero.
+ * @return number of valid frames available to read.
*/
- int32_t getFullFramesAvailable() const;
+ uint32_t getFullFramesAvailable() const;
/**
* The index in a circular buffer of the next frame to read.
@@ -60,9 +61,9 @@
void advanceReadIndex(uint32_t numFrames);
/**
- * @return number of frames that can be written. Never write more than this.
+ * @return maximum number of frames that can be written without exceeding the threshold.
*/
- int32_t getEmptyFramesAvailable() const;
+ uint32_t getEmptyFramesAvailable() const;
/**
* The index in a circular buffer of the next frame to write.
@@ -74,10 +75,6 @@
*/
void advanceWriteIndex(uint32_t numFrames);
- void setThreshold(uint32_t threshold);
-
- uint32_t getThreshold() const { return mThreshold; }
-
uint32_t getFrameCapacity() const { return mTotalFrames; }
virtual uint64_t getReadCounter() const = 0;
@@ -89,7 +86,6 @@
private:
uint32_t mTotalFrames;
- uint32_t mThreshold;
};
} // namespace oboe
diff --git a/src/fifo/FifoControllerIndirect.cpp b/src/fifo/FifoControllerIndirect.cpp
index ad3b17d..86f98bc 100644
--- a/src/fifo/FifoControllerIndirect.cpp
+++ b/src/fifo/FifoControllerIndirect.cpp
@@ -20,10 +20,9 @@
namespace oboe {
FifoControllerIndirect::FifoControllerIndirect(uint32_t numFrames,
- uint32_t threshold,
- int64_t * readCounterAddress,
- int64_t * writeCounterAddress)
- : FifoControllerBase(numFrames, threshold)
+ int64_t *readCounterAddress,
+ int64_t *writeCounterAddress)
+ : FifoControllerBase(numFrames)
, mReadCounterAddress(reinterpret_cast<std::atomic<uint64_t> *>(readCounterAddress))
, mWriteCounterAddress(reinterpret_cast<std::atomic<uint64_t> *>(writeCounterAddress))
{
diff --git a/src/fifo/FifoControllerIndirect.h b/src/fifo/FifoControllerIndirect.h
index 09f47fe..bf0f5e5 100644
--- a/src/fifo/FifoControllerIndirect.h
+++ b/src/fifo/FifoControllerIndirect.h
@@ -29,12 +29,10 @@
public:
FifoControllerIndirect(uint32_t bufferSize,
- uint32_t threshold,
- int64_t * readCounterAddress,
- int64_t * writeCounterAddress);
+ int64_t *readCounterAddress,
+ int64_t *writeCounterAddress);
virtual ~FifoControllerIndirect() = default;
- // TODO review use of memory barriers, probably incorrect
virtual uint64_t getReadCounter() const override {
return mReadCounterAddress->load(std::memory_order_acquire);
}