From b4b484aca143cf86d91ed86824d9e5081e0ddbf0 Mon Sep 17 00:00:00 2001
From: Vishnu Nair <vishnun@google.com>
Date: Fri, 20 Jan 2023 10:00:18 -0800
Subject: BBQ: Check if we have already acquired max num of buffers

When the IGBP is disconnected, the BQ state is reset but the
BBQ state is not. Without the max acquire check in BBQ, we
will continue to acquire buffers. This may cause issues in some
stress tests which continuously disconnect and reconnect while
queuing buffers. The buffers will only be released once they are
released by SF. This can cause some systems to run out of memory.
Ideal fix would be to track and free the buffers once the BQ is
disconnected but that will be more risky for QPR.

Test: atest GLSurfaceViewTest#testPauseResumeWithoutDelay
Bug: 263340543
Change-Id: I77b34f6151a9d1b461649c575be98df1f00f2464
---
 libs/gui/BLASTBufferQueue.cpp | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

(limited to 'libs/gui/BLASTBufferQueue.cpp')

diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp
index 5d12463f88..422002dfdc 100644
--- a/libs/gui/BLASTBufferQueue.cpp
+++ b/libs/gui/BLASTBufferQueue.cpp
@@ -485,11 +485,19 @@ void BLASTBufferQueue::releaseBuffer(const ReleaseCallbackId& callbackId,
 
 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.
+    // Check if we have frames available and we have not acquired the maximum number of buffers.
+    // Even with this check, the consumer can fail to acquire an additional buffer if the consumer
+    // has already acquired (mMaxAcquiredBuffers + 1) and the new buffer is not droppable. In this
+    // case mBufferItemConsumer->acquireBuffer will return with NO_BUFFER_AVAILABLE.
     if (mNumFrameAvailable == 0) {
-        BQA_LOGV("Can't process next buffer. No available frames");
-        return NOT_ENOUGH_DATA;
+        BQA_LOGV("Can't acquire next buffer. No available frames");
+        return BufferQueue::NO_BUFFER_AVAILABLE;
+    }
+
+    if (mNumAcquired >= (mMaxAcquiredBuffers + 2)) {
+        BQA_LOGV("Can't acquire next buffer. Already acquired max frames %d max:%d + 2",
+                 mNumAcquired, mMaxAcquiredBuffers);
+        return BufferQueue::NO_BUFFER_AVAILABLE;
     }
 
     if (mSurfaceControl == nullptr) {
@@ -662,12 +670,12 @@ void BLASTBufferQueue::acquireAndReleaseBuffer() {
 void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) {
     std::function<void(SurfaceComposerClient::Transaction*)> prevCallback = nullptr;
     SurfaceComposerClient::Transaction* prevTransaction = nullptr;
-    bool waitForTransactionCallback = !mSyncedFrameNumbers.empty();
 
     {
         std::unique_lock _lock{mMutex};
         BBQ_TRACE();
 
+        bool waitForTransactionCallback = !mSyncedFrameNumbers.empty();
         const bool syncTransactionSet = mTransactionReadyCallback != nullptr;
         BQA_LOGV("onFrameAvailable-start syncTransactionSet=%s", boolToString(syncTransactionSet));
 
@@ -696,6 +704,15 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) {
                     // flush out the shadow queue
                     acquireAndReleaseBuffer();
                 }
+            } else {
+                // Make sure the frame available count is 0 before proceeding with a sync to ensure
+                // the correct frame is used for the sync. The only way mNumFrameAvailable would be
+                // greater than 0 is if we already ran out of buffers previously. This means we
+                // need to flush the buffers before proceeding with the sync.
+                while (mNumFrameAvailable > 0) {
+                    BQA_LOGD("waiting until no queued buffers");
+                    mCallbackCV.wait(_lock);
+                }
             }
         }
 
@@ -711,6 +728,11 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) {
                  item.mFrameNumber, boolToString(syncTransactionSet));
 
         if (syncTransactionSet) {
+            // Add to mSyncedFrameNumbers before waiting in case any buffers are released
+            // while waiting for a free buffer. The release and commit callback will try to
+            // acquire buffers if there are any available, but we don't want it to acquire
+            // in the case where a sync transaction wants the buffer.
+            mSyncedFrameNumbers.emplace(item.mFrameNumber);
             // 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) {
@@ -723,7 +745,6 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) {
             incStrong((void*)transactionCommittedCallbackThunk);
             mSyncTransaction->addTransactionCommittedCallback(transactionCommittedCallbackThunk,
                                                               static_cast<void*>(this));
-            mSyncedFrameNumbers.emplace(item.mFrameNumber);
             if (mAcquireSingleBuffer) {
                 prevCallback = mTransactionReadyCallback;
                 prevTransaction = mSyncTransaction;
-- 
cgit v1.2.3-59-g8ed1b