CpuConsumer: Properly track acquired buffers

CpuConsumer cannot simply assume a slot's buffer is the same buffer
between acquire and release, and therefore it could be possible for
the same slot to get used for a second acquired buffer, if there's a
producer disconnect in between. This would cause a problem when the
first buffer is released by the consumer.

Instead, use an independent list of acquired buffers to properly track
their state.

Bug: 8291751
Change-Id: I0241ad8704e53d47318c7179b13daed8181b1fab

2 files changed, 63 insertions, 34 deletions

diff --git a/include/gui/CpuConsumer.h b/include/gui/CpuConsumer.h
index 93dff32..4b956c7 100644
--- a/include/gui/CpuConsumer.h
+++ b/include/gui/CpuConsumer.h
@@ -37,7 +37,7 @@ namespace android {
  * This queue is synchronous by default.
  */
 
-class CpuConsumer: public ConsumerBase
+class CpuConsumer : public ConsumerBase
 {
   public:
     typedef ConsumerBase::FrameAvailableListener FrameAvailableListener;
@@ -88,14 +88,25 @@ class CpuConsumer: public ConsumerBase
     // Maximum number of buffers that can be locked at a time
     uint32_t mMaxLockedBuffers;
 
+    status_t releaseAcquiredBufferLocked(int lockedIdx);
+
     virtual void freeBufferLocked(int slotIndex);
 
-    // Array for tracking pointers passed to the consumer, matching the
-    // mSlots indexing
-    struct LockedSlot {
+    // Tracking for buffers acquired by the user
+    struct AcquiredBuffer {
+        // Need to track the original mSlot index and the buffer itself because
+        // the mSlot entry may be freed/reused before the acquired buffer is
+        // released.
+        int mSlot;
         sp<GraphicBuffer> mGraphicBuffer;
         void *mBufferPointer;
-    } mLockedSlots[BufferQueue::NUM_BUFFER_SLOTS];
+
+        AcquiredBuffer() :
+                mSlot(BufferQueue::INVALID_BUFFER_SLOT),
+                mBufferPointer(NULL) {
+        }
+    };
+    Vector<AcquiredBuffer> mAcquiredBuffers;
 
     // Count of currently locked buffers
     uint32_t mCurrentLockedBuffers;
diff --git a/libs/gui/CpuConsumer.cpp b/libs/gui/CpuConsumer.cpp
index 1ee6673..a638cfa 100644
--- a/libs/gui/CpuConsumer.cpp
+++ b/libs/gui/CpuConsumer.cpp
@@ -17,8 +17,9 @@
 //#define LOG_NDEBUG 0
 #define LOG_TAG "CpuConsumer"
 #define ATRACE_TAG ATRACE_TAG_GRAPHICS
-#include <utils/Log.h>
 
+#include <cutils/compiler.h>
+#include <utils/Log.h>
 #include <gui/CpuConsumer.h>
 
 #define CC_LOGV(x, ...) ALOGV("[%s] "x, mName.string(), ##__VA_ARGS__)
@@ -34,9 +35,8 @@ CpuConsumer::CpuConsumer(uint32_t maxLockedBuffers, bool synchronousMode) :
     mMaxLockedBuffers(maxLockedBuffers),
     mCurrentLockedBuffers(0)
 {
-    for (size_t i=0; i < BufferQueue::NUM_BUFFER_SLOTS; i++) {
-        mLockedSlots[i].mBufferPointer = NULL;
-    }
+    // Create tracking entries for locked buffers
+    mAcquiredBuffers.insertAt(0, maxLockedBuffers);
 
     mBufferQueue->setSynchronousMode(synchronousMode);
     mBufferQueue->setConsumerUsageBits(GRALLOC_USAGE_SW_READ_OFTEN);
@@ -44,21 +44,11 @@ CpuConsumer::CpuConsumer(uint32_t maxLockedBuffers, bool synchronousMode) :
 }
 
 CpuConsumer::~CpuConsumer() {
-    status_t err;
-    for (size_t i=0; i < BufferQueue::NUM_BUFFER_SLOTS; i++) {
-        if (mLockedSlots[i].mBufferPointer != NULL) {
-            mLockedSlots[i].mBufferPointer = NULL;
-            err = mLockedSlots[i].mGraphicBuffer->unlock();
-            mLockedSlots[i].mGraphicBuffer.clear();
-            if (err != OK) {
-                CC_LOGE("%s: Unable to unlock graphic buffer %d", __FUNCTION__,
-                        i);
-            }
-
-        }
-    }
+    // ConsumerBase destructor does all the work.
 }
 
+
+
 void CpuConsumer::setName(const String8& name) {
     Mutex::Autolock _l(mMutex);
     mName = name;
@@ -109,8 +99,19 @@ status_t CpuConsumer::lockNextBuffer(LockedBuffer *nativeBuffer) {
                 err);
         return err;
     }
-    mLockedSlots[buf].mBufferPointer = bufferPointer;
-    mLockedSlots[buf].mGraphicBuffer = mSlots[buf].mGraphicBuffer;
+    size_t lockedIdx = 0;
+    for (; lockedIdx < mMaxLockedBuffers; lockedIdx++) {
+        if (mAcquiredBuffers[lockedIdx].mSlot ==
+                BufferQueue::INVALID_BUFFER_SLOT) {
+            break;
+        }
+    }
+    assert(lockedIdx < mMaxLockedBuffers);
+
+    AcquiredBuffer &ab = mAcquiredBuffers.editItemAt(lockedIdx);
+    ab.mSlot = buf;
+    ab.mBufferPointer = bufferPointer;
+    ab.mGraphicBuffer = mSlots[buf].mGraphicBuffer;
 
     nativeBuffer->data   =
             reinterpret_cast<uint8_t*>(bufferPointer);
@@ -132,29 +133,46 @@ status_t CpuConsumer::lockNextBuffer(LockedBuffer *nativeBuffer) {
 
 status_t CpuConsumer::unlockBuffer(const LockedBuffer &nativeBuffer) {
     Mutex::Autolock _l(mMutex);
-    int slotIndex = 0;
+    size_t lockedIdx = 0;
     status_t err;
 
     void *bufPtr = reinterpret_cast<void *>(nativeBuffer.data);
-    for (; slotIndex < BufferQueue::NUM_BUFFER_SLOTS; slotIndex++) {
-        if (bufPtr == mLockedSlots[slotIndex].mBufferPointer) break;
+    for (; lockedIdx < mMaxLockedBuffers; lockedIdx++) {
+        if (bufPtr == mAcquiredBuffers[lockedIdx].mBufferPointer) break;
     }
-    if (slotIndex == BufferQueue::NUM_BUFFER_SLOTS) {
+    if (lockedIdx == mMaxLockedBuffers) {
         CC_LOGE("%s: Can't find buffer to free", __FUNCTION__);
         return BAD_VALUE;
     }
 
-    mLockedSlots[slotIndex].mBufferPointer = NULL;
-    err = mLockedSlots[slotIndex].mGraphicBuffer->unlock();
-    mLockedSlots[slotIndex].mGraphicBuffer.clear();
+    return releaseAcquiredBufferLocked(lockedIdx);
+}
+
+status_t CpuConsumer::releaseAcquiredBufferLocked(int lockedIdx) {
+    status_t err;
+
+    err = mAcquiredBuffers[lockedIdx].mGraphicBuffer->unlock();
     if (err != OK) {
-        CC_LOGE("%s: Unable to unlock graphic buffer %d", __FUNCTION__, slotIndex);
+        CC_LOGE("%s: Unable to unlock graphic buffer %d", __FUNCTION__,
+                lockedIdx);
         return err;
     }
-    releaseBufferLocked(slotIndex, EGL_NO_DISPLAY, EGL_NO_SYNC_KHR);
+    int buf = mAcquiredBuffers[lockedIdx].mSlot;
+
+    // release the buffer if it hasn't already been freed by the BufferQueue.
+    // This can happen, for example, when the producer of this buffer
+    // disconnected after this buffer was acquired.
+    if (CC_LIKELY(mAcquiredBuffers[lockedIdx].mGraphicBuffer ==
+            mSlots[buf].mGraphicBuffer)) {
+        releaseBufferLocked(buf, EGL_NO_DISPLAY, EGL_NO_SYNC_KHR);
+    }
 
-    mCurrentLockedBuffers--;
+    AcquiredBuffer &ab = mAcquiredBuffers.editItemAt(lockedIdx);
+    ab.mSlot = BufferQueue::INVALID_BUFFER_SLOT;
+    ab.mBufferPointer = NULL;
+    ab.mGraphicBuffer.clear();
 
+    mCurrentLockedBuffers--;
     return OK;
 }