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

Commit 1e8bf10f authored by Vishnu Nair's avatar Vishnu Nair
Browse files

BBQ: Recreate BBQ when SurfaceControl changes 1/2 

Alternative approach to fab15e55 which
was racy because the disconnect callback is called without the
BQ lock and the client can continue to modify the BQ state after
disconnecting from the queue.

This approach resets the BQ and BBQ states when the SurfaceControl is
updated. This solves one concrete problem of not relying on the old
SurfaceControl to be destroyed in order to release the currently presented
buffer back to BQ.

In addition this change resets the sync state in BBQ with the rationale
the system does not want to sync on buffers presented on an older
SurfaceControl.

Bug: 197269223
Test: atest BLASTBufferQueueTest
Test: labtest ag/16407859 cf-foldable * 3 (b/197269223#comment40)

Change-Id: Id7049c3fcb7f68ed1bcaed8b82bd13b4397af000
parent f2869ebf

libs/gui/BLASTBufferQueue.cpp

+68 −22
Original line number Diff line number Diff line
@@ -164,8 +164,7 @@ BLASTBufferQueue::BLASTBufferQueue(const std::string& name) 
    mCurrentMaxAcquiredBufferCount = mMaxAcquiredBuffers;

    mNumAcquired = 0;

    mNumFrameAvailable = 0;

    BQA_LOGV("BLASTBufferQueue created width=%d height=%d format=%d mTransformHint=%d", mSize.width,

             mSize.height, mFormat, mTransformHint);

    BQA_LOGV("BLASTBufferQueue created");

}



BLASTBufferQueue::BLASTBufferQueue(const std::string& name, const sp<SurfaceControl>& surface,
@@ -182,14 +181,14 @@ BLASTBufferQueue::~BLASTBufferQueue() { 
    BQA_LOGE("Applying pending transactions on dtor %d",

             static_cast<uint32_t>(mPendingTransactions.size()));

    SurfaceComposerClient::Transaction t;

    for (auto& [targetFrameNumber, transaction] : mPendingTransactions) {

        t.merge(std::move(transaction));

    }

    t.apply();

    mergePendingTransactions(&t, std::numeric_limits<uint64_t>::max() /* frameNumber */);

    t.setApplyToken(mApplyToken).apply();

}



void BLASTBufferQueue::update(const sp<SurfaceControl>& surface, uint32_t width, uint32_t height,

                              int32_t format, SurfaceComposerClient::Transaction* outTransaction) {

    LOG_ALWAYS_FATAL_IF(surface == nullptr, "BLASTBufferQueue: mSurfaceControl must not be NULL");



    std::unique_lock _lock{mMutex};

    if (mFormat != format) {

        mFormat = format;
@@ -197,21 +196,20 @@ void BLASTBufferQueue::update(const sp<SurfaceControl>& surface, uint32_t width, 
    }



    SurfaceComposerClient::Transaction t;

    const bool setBackpressureFlag = !SurfaceControl::isSameSurface(mSurfaceControl, surface);

    const bool surfaceControlChanged = !SurfaceControl::isSameSurface(mSurfaceControl, surface);

    bool applyTransaction = false;



    // Always update the native object even though they might have the same layer handle, so we can

    // get the updated transform hint from WM.

    mSurfaceControl = surface;

    if (mSurfaceControl != nullptr) {

        if (setBackpressureFlag) {

    if (surfaceControlChanged) {

        BQA_LOGD("Updating SurfaceControl without recreating BBQ");

        t.setFlags(mSurfaceControl, layer_state_t::eEnableBackpressure,

                   layer_state_t::eEnableBackpressure);

        applyTransaction = true;

    }

    mTransformHint = mSurfaceControl->getTransformHint();

    mBufferItemConsumer->setTransformHint(mTransformHint);

    }

    BQA_LOGV("update width=%d height=%d format=%d mTransformHint=%d", width, height, format,

             mTransformHint);
@@ -225,11 +223,9 @@ void BLASTBufferQueue::update(const sp<SurfaceControl>& surface, uint32_t width, 
            mSize = mRequestedSize;

            SurfaceComposerClient::Transaction* destFrameTransaction =

                    (outTransaction) ? outTransaction : &t;

            if (mSurfaceControl != nullptr) {

            destFrameTransaction->setDestinationFrame(mSurfaceControl,

                                                      Rect(0, 0, newSize.getWidth(),

                                                           newSize.getHeight()));

            }

            applyTransaction = true;

        }

    }
@@ -640,7 +636,7 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { 


    // add to shadow queue

    mNumFrameAvailable++;

    if (mWaitForTransactionCallback && mNumFrameAvailable == 2) {

    if (mWaitForTransactionCallback && mNumFrameAvailable >= 2) {

        acquireAndReleaseBuffer();

    }

    ATRACE_INT(mQueuedBufferTrace.c_str(),
@@ -717,7 +713,7 @@ bool BLASTBufferQueue::rejectBuffer(const BufferItem& item) { 
// 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;

    return mNumAcquired >= maxAcquiredBuffers;

}



class BBQSurface : public Surface {
@@ -991,4 +987,54 @@ uint64_t BLASTBufferQueue::getLastAcquiredFrameNum() { 
    return mLastAcquiredFrameNumber;

}



void BLASTBufferQueue::abandon() {

    std::unique_lock _lock{mMutex};

    // flush out the shadow queue

    while (mNumFrameAvailable > 0) {

        acquireAndReleaseBuffer();

    }



    // Clear submitted buffer states

    mNumAcquired = 0;

    mSubmitted.clear();

    mPendingRelease.clear();



    if (!mPendingTransactions.empty()) {

        BQA_LOGD("Applying pending transactions on abandon %d",

                 static_cast<uint32_t>(mPendingTransactions.size()));

        SurfaceComposerClient::Transaction t;

        mergePendingTransactions(&t, std::numeric_limits<uint64_t>::max() /* frameNumber */);

        t.setApplyToken(mApplyToken).apply();

    }



    // Clear sync states

    if (mWaitForTransactionCallback) {

        BQA_LOGD("mWaitForTransactionCallback cleared");

        mWaitForTransactionCallback = false;

    }



    if (mSyncTransaction != nullptr) {

        BQA_LOGD("mSyncTransaction cleared mAcquireSingleBuffer=%s",

                 mAcquireSingleBuffer ? "true" : "false");

        mSyncTransaction = nullptr;

        mAcquireSingleBuffer = false;

    }



    // abandon buffer queue

    if (mBufferItemConsumer != nullptr) {

        mBufferItemConsumer->abandon();

        mBufferItemConsumer->setFrameAvailableListener(nullptr);

        mBufferItemConsumer->setBufferFreedListener(nullptr);

        mBufferItemConsumer->setBlastBufferQueue(nullptr);

    }

    mBufferItemConsumer = nullptr;

    mConsumer = nullptr;

    mProducer = nullptr;

}



bool BLASTBufferQueue::isSameSurfaceControl(const sp<SurfaceControl>& surfaceControl) const {

    std::unique_lock _lock{mMutex};

    return SurfaceControl::isSameSurface(mSurfaceControl, surfaceControl);

}



} // namespace android

libs/gui/include/gui/BLASTBufferQueue.h

+5 −3
Original line number Diff line number Diff line
@@ -82,6 +82,7 @@ public: 
        return mProducer;

    }

    sp<Surface> getSurface(bool includeSurfaceControlHandle);

    bool isSameSurfaceControl(const sp<SurfaceControl>& surfaceControl) const;



    void onBufferFreed(const wp<GraphicBuffer>&/* graphicBuffer*/) override { /* TODO */ }

    void onFrameReplaced(const BufferItem& item) override;
@@ -109,6 +110,7 @@ public: 


    uint32_t getLastTransformHint() const;

    uint64_t getLastAcquiredFrameNum();

    void abandon();



    virtual ~BLASTBufferQueue();
@@ -145,15 +147,15 @@ private: 
    std::string mQueuedBufferTrace;

    sp<SurfaceControl> mSurfaceControl;



    std::mutex mMutex;

    mutable std::mutex mMutex;

    std::condition_variable mCallbackCV;



    // BufferQueue internally allows 1 more than

    // the max to be acquired

    int32_t mMaxAcquiredBuffers = 1;



    int32_t mNumFrameAvailable GUARDED_BY(mMutex);

    int32_t mNumAcquired GUARDED_BY(mMutex);

    int32_t mNumFrameAvailable GUARDED_BY(mMutex) = 0;

    int32_t mNumAcquired GUARDED_BY(mMutex) = 0;



    // Keep a reference to the submitted buffers so we can release when surfaceflinger drops the

    // buffer or the buffer has been presented and a new buffer is ready to be presented.

libs/gui/tests/BLASTBufferQueue_test.cpp

+88 −0
Original line number Diff line number Diff line
@@ -1069,6 +1069,94 @@ TEST_F(BLASTBufferQueueTest, SetSyncTransactionAcquireMultipleBuffers) { 
            checkScreenCapture(255, 0, 0, {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

// approach for the client is to recreate the blastbufferqueue when the surfacecontrol updates.

TEST_F(BLASTBufferQueueTest, DISABLED_DisconnectProducerTest) {

    BLASTBufferQueueHelper adapter(mSurfaceControl, mDisplayWidth, mDisplayHeight);

    std::vector<sp<SurfaceControl>> surfaceControls;

    sp<IGraphicBufferProducer> igbProducer;

    for (int i = 0; i < 10; i++) {

        sp<SurfaceControl> sc =

                mClient->createSurface(String8("TestSurface"), mDisplayWidth, mDisplayHeight,

                                       PIXEL_FORMAT_RGBA_8888,

                                       ISurfaceComposerClient::eFXSurfaceBufferState,

                                       /*parent*/ nullptr);

        Transaction()

                .setLayerStack(mSurfaceControl, ui::DEFAULT_LAYER_STACK)

                .setLayer(mSurfaceControl, std::numeric_limits<int32_t>::max())

                .show(mSurfaceControl)

                .setDataspace(mSurfaceControl, ui::Dataspace::V0_SRGB)

                .apply(true);

        surfaceControls.push_back(sc);

        adapter.update(sc, mDisplayWidth, mDisplayHeight);



        setUpProducer(adapter, igbProducer);

        Transaction next;

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

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

        adapter.setSyncTransaction(&next, false);

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



        CallbackHelper transactionCallback;

        next.addTransactionCompletedCallback(transactionCallback.function,

                                             transactionCallback.getContext())

                .apply();



        CallbackData callbackData;

        transactionCallback.getCallbackData(&callbackData);

        // capture screen and verify that it is red

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

        ASSERT_NO_FATAL_FAILURE(

                checkScreenCapture(255, 0, 0,

                                   {0, 0, (int32_t)mDisplayWidth, (int32_t)mDisplayHeight}));

        igbProducer->disconnect(NATIVE_WINDOW_API_CPU);

    }

}



// See DISABLED_DisconnectProducerTest

TEST_F(BLASTBufferQueueTest, DISABLED_UpdateSurfaceControlTest) {

    BLASTBufferQueueHelper adapter(mSurfaceControl, mDisplayWidth, mDisplayHeight);

    std::vector<sp<SurfaceControl>> surfaceControls;

    sp<IGraphicBufferProducer> igbProducer;

    for (int i = 0; i < 10; i++) {

        sp<SurfaceControl> sc =

                mClient->createSurface(String8("TestSurface"), mDisplayWidth, mDisplayHeight,

                                       PIXEL_FORMAT_RGBA_8888,

                                       ISurfaceComposerClient::eFXSurfaceBufferState,

                                       /*parent*/ nullptr);

        Transaction()

                .setLayerStack(mSurfaceControl, ui::DEFAULT_LAYER_STACK)

                .setLayer(mSurfaceControl, std::numeric_limits<int32_t>::max())

                .show(mSurfaceControl)

                .setDataspace(mSurfaceControl, ui::Dataspace::V0_SRGB)

                .apply(true);

        surfaceControls.push_back(sc);

        adapter.update(sc, mDisplayWidth, mDisplayHeight);

        setUpProducer(adapter, igbProducer);



        Transaction next;

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

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

        adapter.setSyncTransaction(&next, false);

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



        CallbackHelper transactionCallback;

        next.addTransactionCompletedCallback(transactionCallback.function,

                                             transactionCallback.getContext())

                .apply();



        CallbackData callbackData;

        transactionCallback.getCallbackData(&callbackData);

        // capture screen and verify that it is red

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

        ASSERT_NO_FATAL_FAILURE(

                checkScreenCapture(255, 0, 0,

                                   {0, 0, (int32_t)mDisplayWidth, (int32_t)mDisplayHeight}));

    }

}



class TestProducerListener : public BnProducerListener {

public:

    sp<IGraphicBufferProducer> mIgbp;