From 4861b10c044ff3c6edd71ca9971bde5c8a8fde0e Mon Sep 17 00:00:00 2001 From: Tianhao Yao Date: Thu, 3 Feb 2022 20:18:35 +0000 Subject: Changed setSyncTransaction to syncNextTransaction with callback logic. Replaced setSyncTransation with syncNextTransaciton in BBQ. BBQ will no longer take in transaction pointers but create transactions inside of itself and invoke the callback when the transaction is ready. Also added stopContinuousSyncTransction method to inform BBQ to cease acquiring buffers into the transaction. Test: go/wm-smoke. Test: BLASTBufferQueueTest Bug: 210714235. Change-Id: I7103a95046d0251e5bdaec21b2741ff194f8e81e --- libs/gui/BLASTBufferQueue.cpp | 139 +++++++++++++++++++------------ libs/gui/include/gui/BLASTBufferQueue.h | 6 +- libs/gui/tests/BLASTBufferQueue_test.cpp | 57 ++++++++----- 3 files changed, 127 insertions(+), 75 deletions(-) (limited to 'libs/gui') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index b7a7aa0ccb..8e23eb8766 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -137,6 +137,7 @@ BLASTBufferQueue::BLASTBufferQueue(const std::string& name, bool updateDestinati mSize(1, 1), mRequestedSize(mSize), mFormat(PIXEL_FORMAT_RGBA_8888), + mTransactionReadyCallback(nullptr), mSyncTransaction(nullptr), mUpdateDestinationFrame(updateDestinationFrame) { createBufferQueue(&mProducer, &mConsumer); @@ -608,60 +609,69 @@ void BLASTBufferQueue::flushAndWaitForFreeBuffer(std::unique_lock& l } void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { - BBQ_TRACE(); - std::unique_lock _lock{mMutex}; + std::function prevCallback = nullptr; + SurfaceComposerClient::Transaction* prevTransaction = nullptr; + { + BBQ_TRACE(); + std::unique_lock _lock{mMutex}; + const bool syncTransactionSet = mTransactionReadyCallback != nullptr; + 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. + if (!mAcquireSingleBuffer) { + auto bufferData = mSyncTransaction->getAndClearBuffer(mSurfaceControl); + if (bufferData) { + BQA_LOGD("Releasing previous buffer when syncing: framenumber=%" PRIu64, + 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; + } + } - const bool syncTransactionSet = mSyncTransaction != nullptr; - 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. - if (!mAcquireSingleBuffer) { - auto bufferData = mSyncTransaction->getAndClearBuffer(mSurfaceControl); - if (bufferData) { - BQA_LOGD("Releasing previous buffer when syncing: framenumber=%" PRIu64, - 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 (mayNeedToWaitForBuffer) { - flushAndWaitForFreeBuffer(_lock); + // add to shadow queue + mNumFrameAvailable++; + if (mWaitForTransactionCallback && mNumFrameAvailable >= 2) { + acquireAndReleaseBuffer(); } - } - - // add to shadow queue - mNumFrameAvailable++; - if (mWaitForTransactionCallback && mNumFrameAvailable >= 2) { - acquireAndReleaseBuffer(); - } - ATRACE_INT(mQueuedBufferTrace.c_str(), - mNumFrameAvailable + mNumAcquired - mPendingRelease.size()); - - BQA_LOGV("onFrameAvailable framenumber=%" PRIu64 " syncTransactionSet=%s", item.mFrameNumber, - boolToString(syncTransactionSet)); - - if (syncTransactionSet) { - acquireNextBufferLocked(mSyncTransaction); - - // Only need a commit callback when syncing to ensure the buffer that's synced has been sent - // to SF - incStrong((void*)transactionCommittedCallbackThunk); - mSyncTransaction->addTransactionCommittedCallback(transactionCommittedCallbackThunk, - static_cast(this)); - - if (mAcquireSingleBuffer) { - mSyncTransaction = nullptr; + ATRACE_INT(mQueuedBufferTrace.c_str(), + mNumFrameAvailable + mNumAcquired - mPendingRelease.size()); + + BQA_LOGV("onFrameAvailable framenumber=%" PRIu64 " syncTransactionSet=%s", + item.mFrameNumber, boolToString(syncTransactionSet)); + + if (syncTransactionSet) { + acquireNextBufferLocked(mSyncTransaction); + + // Only need a commit callback when syncing to ensure the buffer that's synced has been + // sent to SF + incStrong((void*)transactionCommittedCallbackThunk); + mSyncTransaction->addTransactionCommittedCallback(transactionCommittedCallbackThunk, + static_cast(this)); + mWaitForTransactionCallback = true; + if (mAcquireSingleBuffer) { + prevCallback = mTransactionReadyCallback; + prevTransaction = mSyncTransaction; + mTransactionReadyCallback = nullptr; + mSyncTransaction = nullptr; + } + } else if (!mWaitForTransactionCallback) { + acquireNextBufferLocked(std::nullopt); } - mWaitForTransactionCallback = true; - } else if (!mWaitForTransactionCallback) { - acquireNextBufferLocked(std::nullopt); + } + if (prevCallback) { + prevCallback(prevTransaction); } } @@ -680,12 +690,37 @@ void BLASTBufferQueue::onFrameCancelled(const uint64_t bufferId) { mDequeueTimestamps.erase(bufferId); }; -void BLASTBufferQueue::setSyncTransaction(SurfaceComposerClient::Transaction* t, - bool acquireSingleBuffer) { +void BLASTBufferQueue::syncNextTransaction( + std::function callback, + bool acquireSingleBuffer) { BBQ_TRACE(); std::lock_guard _lock{mMutex}; - mSyncTransaction = t; - mAcquireSingleBuffer = mSyncTransaction ? acquireSingleBuffer : true; + mTransactionReadyCallback = callback; + if (callback) { + mSyncTransaction = new SurfaceComposerClient::Transaction(); + } else { + mSyncTransaction = nullptr; + } + mAcquireSingleBuffer = mTransactionReadyCallback ? acquireSingleBuffer : true; +} + +void BLASTBufferQueue::stopContinuousSyncTransaction() { + std::function prevCallback = nullptr; + SurfaceComposerClient::Transaction* prevTransaction = nullptr; + { + std::lock_guard _lock{mMutex}; + bool invokeCallback = mTransactionReadyCallback && !mAcquireSingleBuffer; + if (invokeCallback) { + prevCallback = mTransactionReadyCallback; + prevTransaction = mSyncTransaction; + } + mTransactionReadyCallback = nullptr; + mSyncTransaction = nullptr; + mAcquireSingleBuffer = true; + } + if (prevCallback) { + prevCallback(prevTransaction); + } } bool BLASTBufferQueue::rejectBuffer(const BufferItem& item) { diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h index 74addeac4a..265ae24255 100644 --- a/libs/gui/include/gui/BLASTBufferQueue.h +++ b/libs/gui/include/gui/BLASTBufferQueue.h @@ -95,7 +95,9 @@ public: const std::vector& stats); void releaseBufferCallback(const ReleaseCallbackId& id, const sp& releaseFence, std::optional currentMaxAcquiredBufferCount); - void setSyncTransaction(SurfaceComposerClient::Transaction* t, bool acquireSingleBuffer = true); + void syncNextTransaction(std::function callback, + bool acquireSingleBuffer = true); + void stopContinuousSyncTransaction(); void mergeWithNextTransaction(SurfaceComposerClient::Transaction* t, uint64_t frameNumber); void applyPendingTransactions(uint64_t frameNumber); SurfaceComposerClient::Transaction* gatherPendingTransactions(uint64_t frameNumber); @@ -213,6 +215,8 @@ private: sp mProducer; sp mBufferItemConsumer; + std::function mTransactionReadyCallback + GUARDED_BY(mMutex); SurfaceComposerClient::Transaction* mSyncTransaction GUARDED_BY(mMutex); std::vector> mPendingTransactions GUARDED_BY(mMutex); diff --git a/libs/gui/tests/BLASTBufferQueue_test.cpp b/libs/gui/tests/BLASTBufferQueue_test.cpp index 179bdd76aa..0c3236c9ce 100644 --- a/libs/gui/tests/BLASTBufferQueue_test.cpp +++ b/libs/gui/tests/BLASTBufferQueue_test.cpp @@ -110,15 +110,27 @@ public: mBlastBufferQueueAdapter->update(sc, width, height, PIXEL_FORMAT_RGBA_8888); } - void setSyncTransaction(Transaction* next, bool acquireSingleBuffer = true) { - mBlastBufferQueueAdapter->setSyncTransaction(next, acquireSingleBuffer); + void setSyncTransaction(Transaction& next, bool acquireSingleBuffer = true) { + auto callback = [&next](Transaction* t) { next.merge(std::move(*t)); }; + mBlastBufferQueueAdapter->syncNextTransaction(callback, acquireSingleBuffer); + } + + void syncNextTransaction(std::function callback, + bool acquireSingleBuffer = true) { + mBlastBufferQueueAdapter->syncNextTransaction(callback, acquireSingleBuffer); + } + + void stopContinuousSyncTransaction() { + mBlastBufferQueueAdapter->stopContinuousSyncTransaction(); } int getWidth() { return mBlastBufferQueueAdapter->mSize.width; } int getHeight() { return mBlastBufferQueueAdapter->mSize.height; } - Transaction* getSyncTransaction() { return mBlastBufferQueueAdapter->mSyncTransaction; } + std::function getTransactionReadyCallback() { + return mBlastBufferQueueAdapter->mTransactionReadyCallback; + } sp getIGraphicBufferProducer() { return mBlastBufferQueueAdapter->getIGraphicBufferProducer(); @@ -343,7 +355,7 @@ TEST_F(BLASTBufferQueueTest, CreateBLASTBufferQueue) { ASSERT_EQ(mSurfaceControl, adapter.getSurfaceControl()); ASSERT_EQ(mDisplayWidth, adapter.getWidth()); ASSERT_EQ(mDisplayHeight, adapter.getHeight()); - ASSERT_EQ(nullptr, adapter.getSyncTransaction()); + ASSERT_EQ(nullptr, adapter.getTransactionReadyCallback()); } TEST_F(BLASTBufferQueueTest, Update) { @@ -364,11 +376,12 @@ TEST_F(BLASTBufferQueueTest, Update) { ASSERT_EQ(mDisplayHeight / 2, height); } -TEST_F(BLASTBufferQueueTest, SetSyncTransaction) { +TEST_F(BLASTBufferQueueTest, SyncNextTransaction) { BLASTBufferQueueHelper adapter(mSurfaceControl, mDisplayWidth, mDisplayHeight); - Transaction sync; - adapter.setSyncTransaction(&sync); - ASSERT_EQ(&sync, adapter.getSyncTransaction()); + ASSERT_EQ(nullptr, adapter.getTransactionReadyCallback()); + auto callback = [](Transaction*) {}; + adapter.syncNextTransaction(callback); + ASSERT_NE(nullptr, adapter.getTransactionReadyCallback()); } TEST_F(BLASTBufferQueueTest, DISABLED_onFrameAvailable_ApplyDesiredPresentTime) { @@ -808,7 +821,7 @@ TEST_F(BLASTBufferQueueTest, SyncThenNoSync) { setUpProducer(adapter, igbProducer); Transaction sync; - adapter.setSyncTransaction(&sync); + adapter.setSyncTransaction(sync); queueBuffer(igbProducer, 0, 255, 0, 0); // queue non sync buffer, so this one should get blocked @@ -848,12 +861,12 @@ TEST_F(BLASTBufferQueueTest, MultipleSyncTransactions) { Transaction mainTransaction; Transaction sync; - adapter.setSyncTransaction(&sync); + adapter.setSyncTransaction(sync); queueBuffer(igbProducer, 0, 255, 0, 0); mainTransaction.merge(std::move(sync)); - adapter.setSyncTransaction(&sync); + adapter.setSyncTransaction(sync); queueBuffer(igbProducer, r, g, b, 0); mainTransaction.merge(std::move(sync)); @@ -889,7 +902,7 @@ TEST_F(BLASTBufferQueueTest, MultipleSyncTransactionWithNonSync) { Transaction sync; // queue a sync transaction - adapter.setSyncTransaction(&sync); + adapter.setSyncTransaction(sync); queueBuffer(igbProducer, 0, 255, 0, 0); mainTransaction.merge(std::move(sync)); @@ -898,7 +911,7 @@ TEST_F(BLASTBufferQueueTest, MultipleSyncTransactionWithNonSync) { queueBuffer(igbProducer, 0, 0, 255, 0); // queue another sync transaction - adapter.setSyncTransaction(&sync); + adapter.setSyncTransaction(sync); queueBuffer(igbProducer, r, g, b, 0); // Expect 1 buffer to be released because the non sync transaction should merge // with the sync @@ -937,7 +950,7 @@ TEST_F(BLASTBufferQueueTest, MultipleSyncRunOutOfBuffers) { Transaction sync; // queue a sync transaction - adapter.setSyncTransaction(&sync); + adapter.setSyncTransaction(sync); queueBuffer(igbProducer, 0, 255, 0, 0); mainTransaction.merge(std::move(sync)); @@ -948,7 +961,7 @@ TEST_F(BLASTBufferQueueTest, MultipleSyncRunOutOfBuffers) { queueBuffer(igbProducer, 0, 0, 255, 0); // queue another sync transaction - adapter.setSyncTransaction(&sync); + adapter.setSyncTransaction(sync); queueBuffer(igbProducer, r, g, b, 0); // Expect 3 buffers to be released because the non sync transactions should merge // with the sync @@ -994,7 +1007,7 @@ TEST_F(BLASTBufferQueueTest, RunOutOfBuffersWaitingOnSF) { Transaction sync; // queue a sync transaction - adapter.setSyncTransaction(&sync); + adapter.setSyncTransaction(sync); queueBuffer(igbProducer, 0, 255, 0, 0); mainTransaction.merge(std::move(sync)); @@ -1008,7 +1021,7 @@ TEST_F(BLASTBufferQueueTest, RunOutOfBuffersWaitingOnSF) { mainTransaction.apply(); // queue another sync transaction - adapter.setSyncTransaction(&sync); + adapter.setSyncTransaction(sync); queueBuffer(igbProducer, r, g, b, 0); // Expect 2 buffers to be released because the non sync transactions should merge // with the sync @@ -1031,19 +1044,19 @@ TEST_F(BLASTBufferQueueTest, RunOutOfBuffersWaitingOnSF) { checkScreenCapture(r, g, b, {0, 0, (int32_t)mDisplayWidth, (int32_t)mDisplayHeight})); } -TEST_F(BLASTBufferQueueTest, SetSyncTransactionAcquireMultipleBuffers) { +TEST_F(BLASTBufferQueueTest, SyncNextTransactionAcquireMultipleBuffers) { BLASTBufferQueueHelper adapter(mSurfaceControl, mDisplayWidth, mDisplayHeight); sp igbProducer; setUpProducer(adapter, igbProducer); Transaction next; - adapter.setSyncTransaction(&next, false); + adapter.setSyncTransaction(next, false); queueBuffer(igbProducer, 0, 255, 0, 0); queueBuffer(igbProducer, 0, 0, 255, 0); // There should only be one frame submitted since the first frame will be released. adapter.validateNumFramesSubmitted(1); - adapter.setSyncTransaction(nullptr); + adapter.stopContinuousSyncTransaction(); // queue non sync buffer, so this one should get blocked // Add a present delay to allow the first screenshot to get taken. @@ -1097,7 +1110,7 @@ TEST_F(BLASTBufferQueueTest, DISABLED_DisconnectProducerTest) { Transaction next; queueBuffer(igbProducer, 0, 255, 0, 0); queueBuffer(igbProducer, 0, 0, 255, 0); - adapter.setSyncTransaction(&next, false); + adapter.setSyncTransaction(next, true); queueBuffer(igbProducer, 255, 0, 0, 0); CallbackHelper transactionCallback; @@ -1140,7 +1153,7 @@ TEST_F(BLASTBufferQueueTest, DISABLED_UpdateSurfaceControlTest) { Transaction next; queueBuffer(igbProducer, 0, 255, 0, 0); queueBuffer(igbProducer, 0, 0, 255, 0); - adapter.setSyncTransaction(&next, false); + adapter.setSyncTransaction(next, true); queueBuffer(igbProducer, 255, 0, 0, 0); CallbackHelper transactionCallback; -- cgit v1.2.3-59-g8ed1b