Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit c1cf402b authored by chaviw's avatar chaviw
Browse files

Account for release callbacks when determing if sync is finished 

The current code only considers sync complete when BBQ receives a commit
callback for a frame that's >= to the last acquired buffer. This is
because syncs always blocked anything that wasn't a sync so it was
guaranteed that the last acquired buffer was for a sync.

However, this doesn't work if we want to handle sync buffers that are
dropped. Those transactions may never get applied and therefore the
commit callback may never occur. In those cases, we still want to
consider the sync complete if the buffer is released since it's never
going to get applied.

Test: BLASTBufferQueueTest
Bug: 233625646
Change-Id: I43be3f8bfb41ef5d57e64ca118604fbbc7a81514
parent 8d526955

libs/gui/BLASTBufferQueue.cpp

+24 −18
Original line number Diff line number Diff line
@@ -287,18 +287,17 @@ void BLASTBufferQueue::transactionCommittedCallback(nsecs_t /*latchTime*/, 


                // We need to check if we were waiting for a transaction callback in order to

                // process any pending buffers and unblock. It's possible to get transaction

                // callbacks for previous requests so we need to ensure the frame from this

                // transaction callback matches the last acquired buffer. Since acquireNextBuffer

                // will stop processing buffers when mWaitForTransactionCallback is set, we know

                // that mLastAcquiredFrameNumber is the frame we're waiting on.

                // We also want to check if mNextTransaction is null because it's possible another

                // callbacks for previous requests so we need to ensure that there are no pending

                // frame numbers that were in a sync. We remove the frame from mSyncedFrameNumbers

                // set and then check if it's empty. If there are no more pending syncs, we can

                // proceed with flushing the shadow queue.

                // We also want to check if mSyncTransaction is null because it's possible another

                // sync request came in while waiting, but it hasn't started processing yet. In that

                // case, we don't actually want to flush the frames in between since they will get

                // processed and merged with the sync transaction and released earlier than if they

                // were sent to SF

                if (mWaitForTransactionCallback && mSyncTransaction == nullptr &&

                    currFrameNumber >= mLastAcquiredFrameNumber) {

                    mWaitForTransactionCallback = false;

                mSyncedFrameNumbers.erase(currFrameNumber);

                if (mSyncedFrameNumbers.empty() && mSyncTransaction == nullptr) {

                    flushShadowQueue();

                }

            } else {
@@ -416,9 +415,11 @@ void BLASTBufferQueue::releaseBufferCallback( 
        const auto releasedBuffer = mPendingRelease.front();

        mPendingRelease.pop_front();

        releaseBuffer(releasedBuffer.callbackId, releasedBuffer.releaseFence);

        // Don't process the transactions here if mWaitForTransactionCallback is set. Instead, let

        // onFrameAvailable handle processing them since it will merge with the syncTransaction.

        if (!mWaitForTransactionCallback) {

        // Don't process the transactions here if mSyncedFrameNumbers is not empty. That means

        // are still transactions that have sync buffers in them that have not been applied or

        // dropped. Instead, let onFrameAvailable handle processing them since it will merge with

        // the syncTransaction.

        if (mSyncedFrameNumbers.empty()) {

            acquireNextBufferLocked(std::nullopt);

        }

    }
@@ -442,6 +443,9 @@ void BLASTBufferQueue::releaseBuffer(const ReleaseCallbackId& callbackId, 
    BQA_LOGV("released %s", callbackId.to_string().c_str());

    mBufferItemConsumer->releaseBuffer(it->second, releaseFence);

    mSubmitted.erase(it);

    // Remove the frame number from mSyncedFrameNumbers since we can get a release callback

    // without getting a transaction committed if the buffer was dropped.

    mSyncedFrameNumbers.erase(callbackId.framenumber);

}



void BLASTBufferQueue::acquireNextBufferLocked(
@@ -608,7 +612,7 @@ void BLASTBufferQueue::acquireAndReleaseBuffer() { 
}



void BLASTBufferQueue::flushAndWaitForFreeBuffer(std::unique_lock<std::mutex>& lock) {

    if (mWaitForTransactionCallback && mNumFrameAvailable > 0) {

    if (!mSyncedFrameNumbers.empty() && mNumFrameAvailable > 0) {

        // We are waiting on a previous sync's transaction callback so allow another sync

        // transaction to proceed.

        //
@@ -635,6 +639,8 @@ void BLASTBufferQueue::flushAndWaitForFreeBuffer(std::unique_lock<std::mutex>& l 
void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) {

    std::function<void(SurfaceComposerClient::Transaction*)> prevCallback = nullptr;

    SurfaceComposerClient::Transaction* prevTransaction = nullptr;

    bool waitForTransactionCallback = !mSyncedFrameNumbers.empty();



    {

        BBQ_TRACE();

        std::unique_lock _lock{mMutex};
@@ -666,7 +672,7 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { 


        // add to shadow queue

        mNumFrameAvailable++;

        if (mWaitForTransactionCallback && mNumFrameAvailable >= 2) {

        if (waitForTransactionCallback && mNumFrameAvailable >= 2) {

            acquireAndReleaseBuffer();

        }

        ATRACE_INT(mQueuedBufferTrace.c_str(),
@@ -683,14 +689,14 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { 
            incStrong((void*)transactionCommittedCallbackThunk);

            mSyncTransaction->addTransactionCommittedCallback(transactionCommittedCallbackThunk,

                                                              static_cast<void*>(this));

            mWaitForTransactionCallback = true;

            mSyncedFrameNumbers.emplace(item.mFrameNumber);

            if (mAcquireSingleBuffer) {

                prevCallback = mTransactionReadyCallback;

                prevTransaction = mSyncTransaction;

                mTransactionReadyCallback = nullptr;

                mSyncTransaction = nullptr;

            }

        } else if (!mWaitForTransactionCallback) {

        } else if (!waitForTransactionCallback) {

            acquireNextBufferLocked(std::nullopt);

        }

    }
@@ -1097,9 +1103,9 @@ void BLASTBufferQueue::abandon() { 
    }



    // Clear sync states

    if (mWaitForTransactionCallback) {

        BQA_LOGD("mWaitForTransactionCallback cleared");

        mWaitForTransactionCallback = false;

    if (!mSyncedFrameNumbers.empty()) {

        BQA_LOGD("mSyncedFrameNumbers cleared");

        mSyncedFrameNumbers.clear();

    }



    if (mSyncTransaction != nullptr) {

libs/gui/include/gui/BLASTBufferQueue.h

+2 −1
Original line number Diff line number Diff line
@@ -251,7 +251,6 @@ private: 
    std::queue<sp<SurfaceControl>> mSurfaceControlsWithPendingCallback GUARDED_BY(mMutex);



    uint32_t mCurrentMaxAcquiredBufferCount;

    bool mWaitForTransactionCallback GUARDED_BY(mMutex) = false;



    // Flag to determine if syncTransaction should only acquire a single buffer and then clear or

    // continue to acquire buffers until explicitly cleared
@@ -279,6 +278,8 @@ private: 
    uint64_t mLastAppliedFrameNumber = 0;



    std::function<void(bool)> mTransactionHangCallback;



    std::unordered_set<uint64_t> mSyncedFrameNumbers GUARDED_BY(mMutex);

};



} // namespace android

libs/gui/tests/BLASTBufferQueue_test.cpp

+37 −0
Original line number Diff line number Diff line
@@ -161,6 +161,10 @@ public: 
        ASSERT_EQ(numFramesSubmitted, mBlastBufferQueueAdapter->mSubmitted.size());

    }



    void mergeWithNextTransaction(Transaction* merge, uint64_t frameNumber) {

        mBlastBufferQueueAdapter->mergeWithNextTransaction(merge, frameNumber);

    }



private:

    sp<TestBLASTBufferQueue> mBlastBufferQueueAdapter;

};
@@ -1111,6 +1115,39 @@ TEST_F(BLASTBufferQueueTest, SyncNextTransactionOverwrite) { 
    ASSERT_TRUE(receivedCallback);

}



TEST_F(BLASTBufferQueueTest, SyncNextTransactionDropBuffer) {

    uint8_t r = 255;

    uint8_t g = 0;

    uint8_t b = 0;



    BLASTBufferQueueHelper adapter(mSurfaceControl, mDisplayWidth, mDisplayHeight);



    sp<IGraphicBufferProducer> igbProducer;

    setUpProducer(adapter, igbProducer);



    Transaction sync;

    adapter.setSyncTransaction(sync);

    queueBuffer(igbProducer, 0, 255, 0, 0);



    // Merge a transaction that has a complete callback into the next frame so we can get notified

    // when to take a screenshot

    CallbackHelper transactionCallback;

    Transaction t;

    t.addTransactionCompletedCallback(transactionCallback.function,

                                      transactionCallback.getContext());

    adapter.mergeWithNextTransaction(&t, 2);

    queueBuffer(igbProducer, r, g, b, 0);



    // Drop the buffer, but ensure the next one continues to get processed.

    sync.setBuffer(mSurfaceControl, nullptr);



    CallbackData callbackData;

    transactionCallback.getCallbackData(&callbackData);

    ASSERT_EQ(NO_ERROR, captureDisplay(mCaptureArgs, mCaptureResults));

    ASSERT_NO_FATAL_FAILURE(

            checkScreenCapture(r, g, b, {0, 0, (int32_t)mDisplayWidth, (int32_t)mDisplayHeight}));

}



// This test will currently fail because the old surfacecontrol will steal the last presented buffer

// until the old surface control is destroyed. This is not necessarily a bug but to document a

// limitation with the update API and to test any changes to make the api more robust. The current