libgui: Fix attaching buffers without allocation

This changes the way that BufferQueue selects slots in
waitForFreeSlotThenRelock. This method is called from both
dequeueBuffer and attachBuffer, but those two methods actually have
different preferences:

dequeueBuffer wants a slot with a buffer if possible (to avoid
unnecessary allocations), but will settle for a slot without a buffer
if no free buffers are available.

attachBuffer wants a slot without a buffer if possible (to avoid
clobbering an existing buffer), but will settle with clobbering a free
buffer if no empty slots are available.

These preferences are now respected, which has the side-effect of
fixing a bug where it was not possible to attach a buffer if allocation
is disabled (since the previous implementation assumed finding a slot
without a buffer meant that the caller intended to allocate a buffer,
which would accordingly be blocked since allocation is disabled).

Bug: 26387372
Change-Id: Iefd550fd01925d8c51d6f062d5708d1f6d517edd

3 files changed, 84 insertions, 20 deletions

diff --git a/include/gui/BufferQueueProducer.h b/include/gui/BufferQueueProducer.h
index 835593fdac..645a07bafd 100644
--- a/include/gui/BufferQueueProducer.h
+++ b/include/gui/BufferQueueProducer.h
@@ -183,12 +183,24 @@ private:
     // This is required by the IBinder::DeathRecipient interface
     virtual void binderDied(const wp<IBinder>& who);
 
+    // Returns the slot of the next free buffer if one is available or
+    // BufferQueueCore::INVALID_BUFFER_SLOT otherwise
+    int getFreeBufferLocked() const;
+
+    // Returns the next free slot if one less than or equal to maxBufferCount
+    // is available or BufferQueueCore::INVALID_BUFFER_SLOT otherwise
+    int getFreeSlotLocked(int maxBufferCount) const;
+
     // waitForFreeSlotThenRelock finds the oldest slot in the FREE state. It may
     // block if there are no available slots and we are not in non-blocking
     // mode (producer and consumer controlled by the application). If it blocks,
     // it will release mCore->mMutex while blocked so that other operations on
     // the BufferQueue may succeed.
-    status_t waitForFreeSlotThenRelock(const char* caller, int* found,
+    enum class FreeSlotCaller {
+        Dequeue,
+        Attach,
+    };
+    status_t waitForFreeSlotThenRelock(FreeSlotCaller caller, int* found,
             status_t* returnFlags) const;
 
     sp<BufferQueueCore> mCore;
diff --git a/libs/gui/BufferQueueProducer.cpp b/libs/gui/BufferQueueProducer.cpp
index c48f23728c..56f1a0905c 100644
--- a/libs/gui/BufferQueueProducer.cpp
+++ b/libs/gui/BufferQueueProducer.cpp
@@ -184,12 +184,35 @@ status_t BufferQueueProducer::setAsyncMode(bool async) {
     return NO_ERROR;
 }
 
-status_t BufferQueueProducer::waitForFreeSlotThenRelock(const char* caller,
+int BufferQueueProducer::getFreeBufferLocked() const {
+    if (mCore->mFreeBuffers.empty()) {
+        return BufferQueueCore::INVALID_BUFFER_SLOT;
+    }
+    auto slot = mCore->mFreeBuffers.front();
+    mCore->mFreeBuffers.pop_front();
+    return slot;
+}
+
+int BufferQueueProducer::getFreeSlotLocked(int maxBufferCount) const {
+    if (mCore->mFreeSlots.empty()) {
+        return BufferQueueCore::INVALID_BUFFER_SLOT;
+    }
+    auto slot = *(mCore->mFreeSlots.begin());
+    if (slot < maxBufferCount) {
+        mCore->mFreeSlots.erase(slot);
+        return slot;
+    }
+    return BufferQueueCore::INVALID_BUFFER_SLOT;
+}
+
+status_t BufferQueueProducer::waitForFreeSlotThenRelock(FreeSlotCaller caller,
         int* found, status_t* returnFlags) const {
+    auto callerString = (caller == FreeSlotCaller::Dequeue) ?
+            "dequeueBuffer" : "attachBuffer";
     bool tryAgain = true;
     while (tryAgain) {
         if (mCore->mIsAbandoned) {
-            BQ_LOGE("%s: BufferQueue has been abandoned", caller);
+            BQ_LOGE("%s: BufferQueue has been abandoned", callerString);
             return NO_INIT;
         }
 
@@ -221,7 +244,7 @@ status_t BufferQueueProducer::waitForFreeSlotThenRelock(const char* caller,
         if (mCore->mBufferHasBeenQueued &&
                 dequeuedCount >= mCore->mMaxDequeuedBufferCount) {
             BQ_LOGE("%s: attempting to exceed the max dequeued buffer count "
-                    "(%d)", caller, mCore->mMaxDequeuedBufferCount);
+                    "(%d)", callerString, mCore->mMaxDequeuedBufferCount);
             return INVALID_OPERATION;
         }
 
@@ -234,7 +257,7 @@ status_t BufferQueueProducer::waitForFreeSlotThenRelock(const char* caller,
         bool tooManyBuffers = mCore->mQueue.size()
                             > static_cast<size_t>(maxBufferCount);
         if (tooManyBuffers) {
-            BQ_LOGV("%s: queue size is %zu, waiting", caller,
+            BQ_LOGV("%s: queue size is %zu, waiting", callerString,
                     mCore->mQueue.size());
         } else {
             // If in single buffer mode and a shared buffer exists, always
@@ -242,16 +265,23 @@ status_t BufferQueueProducer::waitForFreeSlotThenRelock(const char* caller,
             if (mCore->mSingleBufferMode && mCore->mSingleBufferSlot !=
                     BufferQueueCore::INVALID_BUFFER_SLOT) {
                 *found = mCore->mSingleBufferSlot;
-            } else if (!mCore->mFreeBuffers.empty()) {
-                auto slot = mCore->mFreeBuffers.begin();
-                *found = *slot;
-                mCore->mFreeBuffers.erase(slot);
-            } else if (mCore->mAllowAllocation && !mCore->mFreeSlots.empty()) {
-                auto slot = mCore->mFreeSlots.begin();
-                // Only return free slots up to the max buffer count
-                if (*slot < maxBufferCount) {
-                    *found = *slot;
-                    mCore->mFreeSlots.erase(slot);
+            } else {
+                if (caller == FreeSlotCaller::Dequeue) {
+                    // If we're calling this from dequeue, prefer free buffers
+                    auto slot = getFreeBufferLocked();
+                    if (slot != BufferQueueCore::INVALID_BUFFER_SLOT) {
+                        *found = slot;
+                    } else if (mCore->mAllowAllocation) {
+                        *found = getFreeSlotLocked(maxBufferCount);
+                    }
+                } else {
+                    // If we're calling this from attach, prefer free slots
+                    auto slot = getFreeSlotLocked(maxBufferCount);
+                    if (slot != BufferQueueCore::INVALID_BUFFER_SLOT) {
+                        *found = slot;
+                    } else {
+                        *found = getFreeBufferLocked();
+                    }
                 }
             }
         }
@@ -338,8 +368,8 @@ status_t BufferQueueProducer::dequeueBuffer(int *outSlot,
 
         int found = BufferItem::INVALID_BUFFER_SLOT;
         while (found == BufferItem::INVALID_BUFFER_SLOT) {
-            status_t status = waitForFreeSlotThenRelock("dequeueBuffer", &found,
-                    &returnFlags);
+            status_t status = waitForFreeSlotThenRelock(FreeSlotCaller::Dequeue,
+                    &found, &returnFlags);
             if (status != NO_ERROR) {
                 return status;
             }
@@ -614,7 +644,7 @@ status_t BufferQueueProducer::attachBuffer(int* outSlot,
 
     status_t returnFlags = NO_ERROR;
     int found;
-    status_t status = waitForFreeSlotThenRelock("attachBuffer", &found,
+    status_t status = waitForFreeSlotThenRelock(FreeSlotCaller::Attach, &found,
             &returnFlags);
     if (status != NO_ERROR) {
         return status;
diff --git a/libs/gui/tests/BufferQueue_test.cpp b/libs/gui/tests/BufferQueue_test.cpp
index a45a5be8e2..ac9af0796e 100644
--- a/libs/gui/tests/BufferQueue_test.cpp
+++ b/libs/gui/tests/BufferQueue_test.cpp
@@ -500,7 +500,7 @@ TEST_F(BufferQueueTest, TestSingleBufferMode) {
         ASSERT_EQ(HAL_DATASPACE_UNKNOWN, item.mDataSpace);
         ASSERT_EQ(Rect(0, 0, 1, 1), item.mCrop);
         ASSERT_EQ(NATIVE_WINDOW_SCALING_MODE_FREEZE, item.mScalingMode);
-        ASSERT_EQ(0, item.mTransform);
+        ASSERT_EQ(0u, item.mTransform);
         ASSERT_EQ(Fence::NO_FENCE, item.mFence);
 
         ASSERT_EQ(OK, mConsumer->releaseBuffer(item.mSlot, item.mFrameNumber,
@@ -527,7 +527,7 @@ TEST_F(BufferQueueTest, TestSingleBufferMode) {
         ASSERT_EQ(HAL_DATASPACE_UNKNOWN, item.mDataSpace);
         ASSERT_EQ(Rect(0, 0, 1, 1), item.mCrop);
         ASSERT_EQ(NATIVE_WINDOW_SCALING_MODE_FREEZE, item.mScalingMode);
-        ASSERT_EQ(0, item.mTransform);
+        ASSERT_EQ(0u, item.mTransform);
         ASSERT_EQ(Fence::NO_FENCE, item.mFence);
 
         ASSERT_EQ(OK, mConsumer->releaseBuffer(item.mSlot, item.mFrameNumber,
@@ -597,4 +597,26 @@ TEST_F(BufferQueueTest, TestTimeouts) {
     ASSERT_GE(systemTime() - startTime, TIMEOUT);
 }
 
+TEST_F(BufferQueueTest, CanAttachWhileDisallowingAllocation) {
+    createBufferQueue();
+    sp<DummyConsumer> dc(new DummyConsumer);
+    ASSERT_EQ(OK, mConsumer->consumerConnect(dc, true));
+    IGraphicBufferProducer::QueueBufferOutput output;
+    ASSERT_EQ(OK, mProducer->connect(new DummyProducerListener,
+            NATIVE_WINDOW_API_CPU, true, &output));
+
+    int slot = BufferQueue::INVALID_BUFFER_SLOT;
+    sp<Fence> sourceFence;
+    ASSERT_EQ(IGraphicBufferProducer::BUFFER_NEEDS_REALLOCATION,
+            mProducer->dequeueBuffer(&slot, &sourceFence, 0, 0, 0, 0));
+    sp<GraphicBuffer> buffer;
+    ASSERT_EQ(OK, mProducer->requestBuffer(slot, &buffer));
+    ASSERT_EQ(OK, mProducer->detachBuffer(slot));
+
+    ASSERT_EQ(OK, mProducer->allowAllocation(false));
+
+    slot = BufferQueue::INVALID_BUFFER_SLOT;
+    ASSERT_EQ(OK, mProducer->attachBuffer(&slot, buffer));
+}
+
 } // namespace android