oboe: some code cleanup suggested by Chrome

Check for errors from getFramesWritten().
Preserve const pointer in transfer().
Check for errors from updateServiceFrameCounter().
Check for overflows in capacity calculation.

Fixes #590

3 files changed, 45 insertions, 17 deletions

diff --git a/src/opensles/AudioStreamBuffered.cpp b/src/opensles/AudioStreamBuffered.cpp
index 8ee01667..298c1eb8 100644
--- a/src/opensles/AudioStreamBuffered.cpp
+++ b/src/opensles/AudioStreamBuffered.cpp
@@ -110,12 +110,22 @@ int64_t AudioStreamBuffered::predictNextCallbackTime() {
 
 // Common code for read/write.
 // @return Result::OK with frames read/written, or Result::Error*
-ResultWithValue<int32_t> AudioStreamBuffered::transfer(void *buffer,
-                                      int32_t numFrames,
-                                      int64_t timeoutNanoseconds) {
+ResultWithValue<int32_t> AudioStreamBuffered::transfer(
+        void *readBuffer,
+        const void *writeBuffer,
+        int32_t numFrames,
+        int64_t timeoutNanoseconds) {
     // Validate arguments.
-    if (buffer == nullptr) {
-        LOGE("AudioStreamBuffered::%s(): buffer is NULL", __func__);
+    if (readBuffer != nullptr && writeBuffer != nullptr) {
+        LOGE("AudioStreamBuffered::%s(): both buffers are not NULL", __func__);
+        return ResultWithValue<int32_t>(Result::ErrorInternal);
+    }
+    if (getDirection() == Direction::Input && readBuffer == nullptr) {
+        LOGE("AudioStreamBuffered::%s(): readBuffer is NULL", __func__);
+        return ResultWithValue<int32_t>(Result::ErrorNull);
+    }
+    if (getDirection() == Direction::Output && writeBuffer == nullptr) {
+        LOGE("AudioStreamBuffered::%s(): writeBuffer is NULL", __func__);
         return ResultWithValue<int32_t>(Result::ErrorNull);
     }
     if (numFrames < 0) {
@@ -130,7 +140,8 @@ ResultWithValue<int32_t> AudioStreamBuffered::transfer(void *buffer,
     }
 
     int32_t result = 0;
-    uint8_t *data = reinterpret_cast<uint8_t *>(buffer);
+    uint8_t *readData = reinterpret_cast<uint8_t *>(readBuffer);
+    const uint8_t *writeData = reinterpret_cast<const uint8_t *>(writeBuffer);
     int32_t framesLeft = numFrames;
     int64_t timeToQuit = 0;
     bool repeat = true;
@@ -144,18 +155,22 @@ ResultWithValue<int32_t> AudioStreamBuffered::transfer(void *buffer,
     do {
         // read or write
         if (getDirection() == Direction::Input) {
-            result = mFifoBuffer->read(data, framesLeft);
+            result = mFifoBuffer->read(readData, framesLeft);
+            if (result > 0) {
+                readData += mFifoBuffer->convertFramesToBytes(result);
+                framesLeft -= result;
+            }
         } else {
             // between zero and capacity
             uint32_t fullFrames = mFifoBuffer->getFullFramesAvailable();
             // Do not write above threshold size.
             int32_t emptyFrames = getBufferSizeInFrames() - static_cast<int32_t>(fullFrames);
             int32_t framesToWrite = std::max(0, std::min(framesLeft, emptyFrames));
-            result = mFifoBuffer->write(data, framesToWrite);
-        }
-        if (result > 0) {
-            data += mFifoBuffer->convertFramesToBytes(result);
-            framesLeft -= result;
+            result = mFifoBuffer->write(writeData, framesToWrite);
+            if (result > 0) {
+                writeData += mFifoBuffer->convertFramesToBytes(result);
+                framesLeft -= result;
+            }
         }
 
         // If we need more data then sleep and try again.
@@ -211,8 +226,9 @@ ResultWithValue<int32_t> AudioStreamBuffered::write(const void *buffer,
     if (getDirection() == Direction::Input) {
         return ResultWithValue<int32_t>(Result::ErrorUnavailable); // TODO review, better error code?
     }
-    updateServiceFrameCounter();
-    return transfer(const_cast<void *>(buffer), numFrames, timeoutNanoseconds);
+    Result result = updateServiceFrameCounter();
+    if (result != Result::OK) return ResultWithValue<int32_t>(static_cast<Result>(result));
+    return transfer(nullptr, buffer, numFrames, timeoutNanoseconds);
 }
 
 // Read data from the FIFO that was written by the callback.
@@ -226,8 +242,9 @@ ResultWithValue<int32_t> AudioStreamBuffered::read(void *buffer,
     if (getDirection() == Direction::Output) {
         return ResultWithValue<int32_t>(Result::ErrorUnavailable); // TODO review, better error code?
     }
-    updateServiceFrameCounter();
-    return transfer(buffer, numFrames, timeoutNanoseconds);
+    Result result = updateServiceFrameCounter();
+    if (result != Result::OK) return ResultWithValue<int32_t>(static_cast<Result>(result));
+    return transfer(buffer, nullptr, numFrames, timeoutNanoseconds);
 }
 
 // Only supported when we are not using a callback.
diff --git a/src/opensles/AudioStreamBuffered.h b/src/opensles/AudioStreamBuffered.h
index 2b6152b6..d7e88850 100644
--- a/src/opensles/AudioStreamBuffered.h
+++ b/src/opensles/AudioStreamBuffered.h
@@ -74,7 +74,11 @@ private:
     void markCallbackTime(int32_t numFrames);
 
     // Read or write to the FIFO.
-    ResultWithValue<int32_t> transfer(void *buffer, int32_t numFrames, int64_t timeoutNanoseconds);
+    // Only pass one pointer and set the other to nullptr.
+    ResultWithValue<int32_t> transfer(void *readBuffer,
+            const void *writeBuffer,
+            int32_t numFrames,
+            int64_t timeoutNanoseconds);
 
     void incrementXRunCount() {
         ++mXRunCount;
diff --git a/src/opensles/AudioStreamOpenSLES.cpp b/src/opensles/AudioStreamOpenSLES.cpp
index 9a70700f..c0487468 100644
--- a/src/opensles/AudioStreamOpenSLES.cpp
+++ b/src/opensles/AudioStreamOpenSLES.cpp
@@ -134,6 +134,13 @@ Result AudioStreamOpenSLES::configureBufferSizes(int32_t sampleRate) {
 
     if (!usingFIFO()) {
         mBufferCapacityInFrames = mFramesPerBurst * kBufferQueueLength;
+        // Check for overflow.
+        if (mBufferCapacityInFrames <= 0) {
+            mBufferCapacityInFrames = 0;
+            LOGE("AudioStreamOpenSLES::open() numeric overflow because mFramesPerBurst = %d",
+                 mFramesPerBurst);
+            return Result::ErrorOutOfRange;
+        }
         mBufferSizeInFrames = mBufferCapacityInFrames;
     }