[BBQ] Only wait when acquiring a buffer if in a sync.

There's no need to do the explicit maxBuffersAcquired check since the
acquireBuffer call will return a status of NO_BUFFER_AVAILABLE if it
can't acquire any more buffers. Therefore, the code can be simplified so
it will return early in most cases if NO_BUFFER_AVAILABLE is returned.
If there was a request to sync the buffer, then we need to wait on a
free buffer and not return early.

Test: BBQ syncs
Bug: 263340543
Bug: 263570429
Change-Id: I11a81a440bc0d22ba85bcf4d4a068b853ca5cf9c
Merged-In: I11a81a440bc0d22ba85bcf4d4a068b853ca5cf9c

2 files changed, 28 insertions, 56 deletions

diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp
index 24a5295112..8ee9b14945 100644
--- a/libs/gui/BLASTBufferQueue.cpp
+++ b/libs/gui/BLASTBufferQueue.cpp
@@ -480,20 +480,18 @@ void BLASTBufferQueue::releaseBuffer(const ReleaseCallbackId& callbackId,
     mSyncedFrameNumbers.erase(callbackId.framenumber);
 }
 
-void BLASTBufferQueue::acquireNextBufferLocked(
+status_t BLASTBufferQueue::acquireNextBufferLocked(
         const std::optional<SurfaceComposerClient::Transaction*> transaction) {
     // If the next transaction is set, we want to guarantee the our acquire will not fail, so don't
     // include the extra buffer when checking if we can acquire the next buffer.
-    const bool includeExtraAcquire = !transaction;
-    const bool maxAcquired = maxBuffersAcquired(includeExtraAcquire);
-    if (mNumFrameAvailable == 0 || maxAcquired) {
-        BQA_LOGV("Can't process next buffer maxBuffersAcquired=%s", boolToString(maxAcquired));
-        return;
+    if (mNumFrameAvailable == 0) {
+        BQA_LOGV("Can't process next buffer. No available frames");
+        return NOT_ENOUGH_DATA;
     }
 
     if (mSurfaceControl == nullptr) {
         BQA_LOGE("ERROR : surface control is null");
-        return;
+        return NAME_NOT_FOUND;
     }
 
     SurfaceComposerClient::Transaction localTransaction;
@@ -510,10 +508,10 @@ void BLASTBufferQueue::acquireNextBufferLocked(
             mBufferItemConsumer->acquireBuffer(&bufferItem, 0 /* expectedPresent */, false);
     if (status == BufferQueue::NO_BUFFER_AVAILABLE) {
         BQA_LOGV("Failed to acquire a buffer, err=NO_BUFFER_AVAILABLE");
-        return;
+        return status;
     } else if (status != OK) {
         BQA_LOGE("Failed to acquire a buffer, err=%s", statusToString(status).c_str());
-        return;
+        return status;
     }
 
     auto buffer = bufferItem.mGraphicBuffer;
@@ -523,7 +521,7 @@ void BLASTBufferQueue::acquireNextBufferLocked(
     if (buffer == nullptr) {
         mBufferItemConsumer->releaseBuffer(bufferItem, Fence::NO_FENCE);
         BQA_LOGE("Buffer was empty");
-        return;
+        return BAD_VALUE;
     }
 
     if (rejectBuffer(bufferItem)) {
@@ -532,8 +530,7 @@ void BLASTBufferQueue::acquireNextBufferLocked(
                  mSize.width, mSize.height, mRequestedSize.width, mRequestedSize.height,
                  buffer->getWidth(), buffer->getHeight(), bufferItem.mTransform);
         mBufferItemConsumer->releaseBuffer(bufferItem, Fence::NO_FENCE);
-        acquireNextBufferLocked(transaction);
-        return;
+        return acquireNextBufferLocked(transaction);
     }
 
     mNumAcquired++;
@@ -621,6 +618,7 @@ void BLASTBufferQueue::acquireNextBufferLocked(
              bufferItem.mTimestamp, bufferItem.mIsAutoTimestamp ? "(auto)" : "",
              static_cast<uint32_t>(mPendingTransactions.size()), bufferItem.mGraphicBuffer->getId(),
              bufferItem.mAutoRefresh ? " mAutoRefresh" : "", bufferItem.mTransform);
+    return OK;
 }
 
 Rect BLASTBufferQueue::computeCrop(const BufferItem& item) {
@@ -643,31 +641,6 @@ void BLASTBufferQueue::acquireAndReleaseBuffer() {
     mBufferItemConsumer->releaseBuffer(bufferItem, bufferItem.mFence);
 }
 
-void BLASTBufferQueue::flushAndWaitForFreeBuffer(std::unique_lock<std::mutex>& lock) {
-    if (!mSyncedFrameNumbers.empty() && mNumFrameAvailable > 0) {
-        // We are waiting on a previous sync's transaction callback so allow another sync
-        // transaction to proceed.
-        //
-        // We need to first flush out the transactions that were in between the two syncs.
-        // We do this by merging them into mSyncTransaction so any buffer merging will get
-        // a release callback invoked. The release callback will be async so we need to wait
-        // on max acquired to make sure we have the capacity to acquire another buffer.
-        if (maxBuffersAcquired(false /* includeExtraAcquire */)) {
-            BQA_LOGD("waiting to flush shadow queue...");
-            mCallbackCV.wait(lock);
-        }
-        while (mNumFrameAvailable > 0) {
-            // flush out the shadow queue
-            acquireAndReleaseBuffer();
-        }
-    }
-
-    while (maxBuffersAcquired(false /* includeExtraAcquire */)) {
-        BQA_LOGD("waiting for free buffer.");
-        mCallbackCV.wait(lock);
-    }
-}
-
 void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) {
     std::function<void(SurfaceComposerClient::Transaction*)> prevCallback = nullptr;
     SurfaceComposerClient::Transaction* prevTransaction = nullptr;
@@ -680,7 +653,6 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) {
         BQA_LOGV("onFrameAvailable-start syncTransactionSet=%s", boolToString(syncTransactionSet));
 
         if (syncTransactionSet) {
-            bool mayNeedToWaitForBuffer = true;
             // If we are going to re-use the same mSyncTransaction, release the buffer that may
             // already be set in the Transaction. This is to allow us a free slot early to continue
             // processing a new buffer.
@@ -691,14 +663,20 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) {
                              bufferData->frameNumber);
                     releaseBuffer(bufferData->generateReleaseCallbackId(),
                                   bufferData->acquireFence);
-                    // Because we just released a buffer, we know there's no need to wait for a free
-                    // buffer.
-                    mayNeedToWaitForBuffer = false;
                 }
             }
 
-            if (mayNeedToWaitForBuffer) {
-                flushAndWaitForFreeBuffer(_lock);
+            if (waitForTransactionCallback) {
+                // We are waiting on a previous sync's transaction callback so allow another sync
+                // transaction to proceed.
+                //
+                // We need to first flush out the transactions that were in between the two syncs.
+                // We do this by merging them into mSyncTransaction so any buffer merging will get
+                // a release callback invoked.
+                while (mNumFrameAvailable > 0) {
+                    // flush out the shadow queue
+                    acquireAndReleaseBuffer();
+                }
             }
         }
 
@@ -714,7 +692,12 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) {
                  item.mFrameNumber, boolToString(syncTransactionSet));
 
         if (syncTransactionSet) {
-            acquireNextBufferLocked(mSyncTransaction);
+            // If there's no available buffer and we're in a sync transaction, we need to wait
+            // instead of returning since we guarantee a buffer will be acquired for the sync.
+            while (acquireNextBufferLocked(mSyncTransaction) == BufferQueue::NO_BUFFER_AVAILABLE) {
+                BQA_LOGD("waiting for available buffer");
+                mCallbackCV.wait(_lock);
+            }
 
             // Only need a commit callback when syncing to ensure the buffer that's synced has been
             // sent to SF
@@ -824,15 +807,6 @@ bool BLASTBufferQueue::rejectBuffer(const BufferItem& item) {
     return mSize != bufferSize;
 }
 
-// Check if we have acquired the maximum number of buffers.
-// Consumer can acquire an additional buffer if that buffer is not droppable. Set
-// includeExtraAcquire is true to include this buffer to the count. Since this depends on the state
-// of the buffer, the next acquire may return with NO_BUFFER_AVAILABLE.
-bool BLASTBufferQueue::maxBuffersAcquired(bool includeExtraAcquire) const {
-    int maxAcquiredBuffers = mMaxAcquiredBuffers + (includeExtraAcquire ? 2 : 1);
-    return mNumAcquired >= maxAcquiredBuffers;
-}
-
 class BBQSurface : public Surface {
 private:
     std::mutex mMutex;
diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h
index 48226b9641..f074bd37d1 100644
--- a/libs/gui/include/gui/BLASTBufferQueue.h
+++ b/libs/gui/include/gui/BLASTBufferQueue.h
@@ -136,12 +136,11 @@ private:
     void createBufferQueue(sp<IGraphicBufferProducer>* outProducer,
                            sp<IGraphicBufferConsumer>* outConsumer);
 
-    void acquireNextBufferLocked(
+    status_t acquireNextBufferLocked(
             const std::optional<SurfaceComposerClient::Transaction*> transaction) REQUIRES(mMutex);
     Rect computeCrop(const BufferItem& item) REQUIRES(mMutex);
     // Return true if we need to reject the buffer based on the scaling mode and the buffer size.
     bool rejectBuffer(const BufferItem& item) REQUIRES(mMutex);
-    bool maxBuffersAcquired(bool includeExtraAcquire) const REQUIRES(mMutex);
     static PixelFormat convertBufferFormat(PixelFormat& format);
     void mergePendingTransactions(SurfaceComposerClient::Transaction* t, uint64_t frameNumber)
             REQUIRES(mMutex);
@@ -150,7 +149,6 @@ private:
     void acquireAndReleaseBuffer() REQUIRES(mMutex);
     void releaseBuffer(const ReleaseCallbackId& callbackId, const sp<Fence>& releaseFence)
             REQUIRES(mMutex);
-    void flushAndWaitForFreeBuffer(std::unique_lock<std::mutex>& lock);
 
     std::string mName;
     // Represents the queued buffer count from buffer queue,