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

Commit c398c010 authored by Chavi Weingarten's avatar Chavi Weingarten
Browse files

Add explicit clearSyncTransaction instead of passing in null 

When attempting to clear the last syncNextTransaction, the code was
ambiguous and it was unclear what the expectation should be. Instead,
don't allow null when calling syncNextTransaction and don't allow the
syncTransaction to be overwritten. If the caller wants to clear the
syncTransaction before a frame has arrived, they can do so by calling
clearSyncTransaction.

Test: No warning log in sync with no buffer.
Test: BLASTBufferQueueTest
Bug: 272189296
Change-Id: I3a945f5503220225f2147b0331d1fb2f9ea8dc63
parent b6c0bd6a

libs/gui/BLASTBufferQueue.cpp

+32 −27
Original line number Diff line number Diff line
@@ -800,34 +800,24 @@ void BLASTBufferQueue::onFrameCancelled(const uint64_t bufferId) { 
    mDequeueTimestamps.erase(bufferId);

};



void BLASTBufferQueue::syncNextTransaction(

bool BLASTBufferQueue::syncNextTransaction(

        std::function<void(SurfaceComposerClient::Transaction*)> callback,

        bool acquireSingleBuffer) {

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

    SurfaceComposerClient::Transaction* prevTransaction = nullptr;

    LOG_ALWAYS_FATAL_IF(!callback,

                        "BLASTBufferQueue: callback passed in to syncNextTransaction must not be "

                        "NULL");



    {

    std::lock_guard _lock{mMutex};

    BBQ_TRACE();

        // We're about to overwrite the previous call so we should invoke that callback

        // immediately.

    if (mTransactionReadyCallback) {

            prevCallback = mTransactionReadyCallback;

            prevTransaction = mSyncTransaction;

        ALOGW("Attempting to overwrite transaction callback in syncNextTransaction");

        return false;

    }



    mTransactionReadyCallback = callback;

        if (callback) {

    mSyncTransaction = new SurfaceComposerClient::Transaction();

        } else {

            mSyncTransaction = nullptr;

        }

        mAcquireSingleBuffer = mTransactionReadyCallback ? acquireSingleBuffer : true;

    }



    if (prevCallback) {

        prevCallback(prevTransaction);

    }

    mAcquireSingleBuffer = acquireSingleBuffer;

    return true;

}



void BLASTBufferQueue::stopContinuousSyncTransaction() {
@@ -835,20 +825,35 @@ void BLASTBufferQueue::stopContinuousSyncTransaction() { 
    SurfaceComposerClient::Transaction* prevTransaction = nullptr;

    {

        std::lock_guard _lock{mMutex};

        bool invokeCallback = mTransactionReadyCallback && !mAcquireSingleBuffer;

        if (invokeCallback) {

        if (mAcquireSingleBuffer || !mTransactionReadyCallback) {

            ALOGW("Attempting to stop continuous sync when none are active");

            return;

        }



        prevCallback = mTransactionReadyCallback;

        prevTransaction = mSyncTransaction;

        }



        mTransactionReadyCallback = nullptr;

        mSyncTransaction = nullptr;

        mAcquireSingleBuffer = true;

    }



    if (prevCallback) {

        prevCallback(prevTransaction);

    }

}



void BLASTBufferQueue::clearSyncTransaction() {

    std::lock_guard _lock{mMutex};

    if (!mAcquireSingleBuffer) {

        ALOGW("Attempting to clear sync transaction when none are active");

        return;

    }



    mTransactionReadyCallback = nullptr;

    mSyncTransaction = nullptr;

}



bool BLASTBufferQueue::rejectBuffer(const BufferItem& item) {

    if (item.mScalingMode != NATIVE_WINDOW_SCALING_MODE_FREEZE) {

        // Only reject buffers if scaling mode is freeze.

libs/gui/include/gui/BLASTBufferQueue.h

+2 −1
Original line number Diff line number Diff line
@@ -97,9 +97,10 @@ public: 
    void releaseBufferCallbackLocked(const ReleaseCallbackId& id, const sp<Fence>& releaseFence,

                                     std::optional<uint32_t> currentMaxAcquiredBufferCount,

                                     bool fakeRelease) REQUIRES(mMutex);

    void syncNextTransaction(std::function<void(SurfaceComposerClient::Transaction*)> callback,

    bool syncNextTransaction(std::function<void(SurfaceComposerClient::Transaction*)> callback,

                             bool acquireSingleBuffer = true);

    void stopContinuousSyncTransaction();

    void clearSyncTransaction();



    void mergeWithNextTransaction(SurfaceComposerClient::Transaction* t, uint64_t frameNumber);

    void applyPendingTransactions(uint64_t frameNumber);

libs/gui/tests/BLASTBufferQueue_test.cpp

+40 −3
Original line number Diff line number Diff line
@@ -116,15 +116,17 @@ public: 
        mBlastBufferQueueAdapter->syncNextTransaction(callback, acquireSingleBuffer);

    }



    void syncNextTransaction(std::function<void(Transaction*)> callback,

    bool syncNextTransaction(std::function<void(Transaction*)> callback,

                             bool acquireSingleBuffer = true) {

        mBlastBufferQueueAdapter->syncNextTransaction(callback, acquireSingleBuffer);

        return mBlastBufferQueueAdapter->syncNextTransaction(callback, acquireSingleBuffer);

    }



    void stopContinuousSyncTransaction() {

        mBlastBufferQueueAdapter->stopContinuousSyncTransaction();

    }



    void clearSyncTransaction() { mBlastBufferQueueAdapter->clearSyncTransaction(); }



    int getWidth() { return mBlastBufferQueueAdapter->mSize.width; }



    int getHeight() { return mBlastBufferQueueAdapter->mSize.height; }
@@ -1108,7 +1110,11 @@ TEST_F(BLASTBufferQueueTest, SyncNextTransactionOverwrite) { 
    ASSERT_NE(nullptr, adapter.getTransactionReadyCallback());



    auto callback2 = [](Transaction*) {};

    adapter.syncNextTransaction(callback2);

    ASSERT_FALSE(adapter.syncNextTransaction(callback2));



    sp<IGraphicBufferProducer> igbProducer;

    setUpProducer(adapter, igbProducer);

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



    std::unique_lock<std::mutex> lock(mutex);

    if (!receivedCallback) {
@@ -1120,6 +1126,37 @@ TEST_F(BLASTBufferQueueTest, SyncNextTransactionOverwrite) { 
    ASSERT_TRUE(receivedCallback);

}



TEST_F(BLASTBufferQueueTest, ClearSyncTransaction) {

    std::mutex mutex;

    std::condition_variable callbackReceivedCv;

    bool receivedCallback = false;



    BLASTBufferQueueHelper adapter(mSurfaceControl, mDisplayWidth, mDisplayHeight);

    ASSERT_EQ(nullptr, adapter.getTransactionReadyCallback());

    auto callback = [&](Transaction*) {

        std::unique_lock<std::mutex> lock(mutex);

        receivedCallback = true;

        callbackReceivedCv.notify_one();

    };

    adapter.syncNextTransaction(callback);

    ASSERT_NE(nullptr, adapter.getTransactionReadyCallback());



    adapter.clearSyncTransaction();



    sp<IGraphicBufferProducer> igbProducer;

    setUpProducer(adapter, igbProducer);

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



    std::unique_lock<std::mutex> lock(mutex);

    if (!receivedCallback) {

        ASSERT_EQ(callbackReceivedCv.wait_for(lock, std::chrono::seconds(3)),

                  std::cv_status::timeout)

                << "did not receive callback";

    }



    ASSERT_FALSE(receivedCallback);

}



TEST_F(BLASTBufferQueueTest, SyncNextTransactionDropBuffer) {

    uint8_t r = 255;

    uint8_t g = 0;