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

Commit 8dd181f3 authored by chaviw's avatar chaviw
Browse files

Replace releaseCallbackId with generateReleaseCallbackId in BufferData 

releaseCallbackId was confusing data in BufferData because it wasn't
being parceled, even though the rest of the object was. This is because
the ReleaseCallbackId can be generated from info in BufferData.
Therefore, remove releaseCallbackId and instead replace with a function
that generates the ReleaseCallbackId

This fixes an issue when Transactions that contained buffers for the
same layer were being merged. The merge was expected to release the old
buffer. However, if the Transaction was parceled before it was merged,
the ReleaseCallbackId would be lost and the release callback would send
an invalid callback id, resulting in a lost buffer that never got
released. By using generateReleaseCallbackId, the callback id is
recreated when the release callback needs to be invoked since the
buffer and framenumber are already being parceled.

Test: ReleaseBufferCallbackTest
Test: AppConfigurationTests
Bug: 209920544
Change-Id: I2a24b8a9764959173c960048dc82e68f4c083898
parent 38e78ac1

libs/gui/BLASTBufferQueue.cpp

+2 −3
Original line number Diff line number Diff line
@@ -507,8 +507,7 @@ void BLASTBufferQueue::acquireNextBufferLocked( 
            std::bind(releaseBufferCallbackThunk, wp<BLASTBufferQueue>(this) /* callbackContext */,

                      std::placeholders::_1, std::placeholders::_2, std::placeholders::_3);

    sp<Fence> fence = bufferItem.mFence ? new Fence(bufferItem.mFence->dup()) : Fence::NO_FENCE;

    t->setBuffer(mSurfaceControl, buffer, fence, bufferItem.mFrameNumber, releaseCallbackId,

                 releaseBufferCallback);

    t->setBuffer(mSurfaceControl, buffer, fence, bufferItem.mFrameNumber, releaseBufferCallback);

    t->setDataspace(mSurfaceControl, static_cast<ui::Dataspace>(bufferItem.mDataSpace));

    t->setHdrMetadata(mSurfaceControl, bufferItem.mHdrMetadata);

    t->setSurfaceDamageRegion(mSurfaceControl, bufferItem.mSurfaceDamage);
@@ -622,7 +621,7 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { 
            if (bufferData) {

                BQA_LOGD("Releasing previous buffer when syncing: framenumber=%" PRIu64,

                         bufferData->frameNumber);

                releaseBuffer(bufferData->releaseCallbackId, bufferData->acquireFence);

                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;

libs/gui/LayerState.cpp

+4 −0
Original line number Diff line number Diff line
@@ -677,6 +677,10 @@ status_t LayerCaptureArgs::read(const Parcel& input) { 
    return NO_ERROR;

}



ReleaseCallbackId BufferData::generateReleaseCallbackId() const {

    return {buffer->getId(), frameNumber};

}



status_t BufferData::write(Parcel& output) const {

    SAFE_PARCEL(output.writeInt32, flags.get());

libs/gui/SurfaceComposerClient.cpp

+7 −9
Original line number Diff line number Diff line
@@ -736,10 +736,10 @@ void SurfaceComposerClient::Transaction::releaseBufferIfOverwriting(const layer_ 
        // if the callback is in process, run on a different thread to avoid any lock contigency

        // issues in the client.

        SurfaceComposerClient::getDefault()

                ->mReleaseCallbackThread.addReleaseCallback(state.bufferData.releaseCallbackId,

                                                            fence);

                ->mReleaseCallbackThread

                .addReleaseCallback(state.bufferData.generateReleaseCallbackId(), fence);

    } else {

        listener->onReleaseBuffer(state.bufferData.releaseCallbackId, fence, UINT_MAX);

        listener->onReleaseBuffer(state.bufferData.generateReleaseCallbackId(), fence, UINT_MAX);

    }

}
@@ -1335,7 +1335,7 @@ std::optional<BufferData> SurfaceComposerClient::Transaction::getAndClearBuffer( 
    BufferData bufferData = s->bufferData;



    TransactionCompletedListener::getInstance()->removeReleaseBufferCallback(

            bufferData.releaseCallbackId);

            bufferData.generateReleaseCallbackId());

    BufferData emptyBufferData;

    s->what &= ~layer_state_t::eBufferChanged;

    s->bufferData = emptyBufferData;
@@ -1347,7 +1347,7 @@ std::optional<BufferData> SurfaceComposerClient::Transaction::getAndClearBuffer( 
SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setBuffer(

        const sp<SurfaceControl>& sc, const sp<GraphicBuffer>& buffer,

        const std::optional<sp<Fence>>& fence, const std::optional<uint64_t>& frameNumber,

        const ReleaseCallbackId& id, ReleaseBufferCallback callback) {

        ReleaseBufferCallback callback) {

    layer_state_t* s = getLayerState(sc);

    if (!s) {

        mStatus = BAD_INDEX;
@@ -1371,7 +1371,7 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setBuffe 
    if (mIsAutoTimestamp) {

        mDesiredPresentTime = systemTime();

    }

    setReleaseBufferCallback(&bufferData, id, callback);

    setReleaseBufferCallback(&bufferData, callback);

    s->what |= layer_state_t::eBufferChanged;

    s->bufferData = bufferData;

    registerSurfaceControlForCallback(sc);
@@ -1381,7 +1381,6 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setBuffe 
}



void SurfaceComposerClient::Transaction::setReleaseBufferCallback(BufferData* bufferData,

                                                                  const ReleaseCallbackId& id,

                                                                  ReleaseBufferCallback callback) {

    if (!callback) {

        return;
@@ -1394,9 +1393,8 @@ void SurfaceComposerClient::Transaction::setReleaseBufferCallback(BufferData* bu 
    }



    bufferData->releaseBufferListener = TransactionCompletedListener::getIInstance();

    bufferData->releaseCallbackId = id;

    auto listener = TransactionCompletedListener::getInstance();

    listener->setReleaseBufferCallback(id, callback);

    listener->setReleaseBufferCallback(bufferData->generateReleaseCallbackId(), callback);

}



SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setDataspace(

libs/gui/include/gui/LayerState.h

+3 −6
Original line number Diff line number Diff line
@@ -77,12 +77,6 @@ struct BufferData { 
    // buffer id to identify the buffer.

    sp<ITransactionCompletedListener> releaseBufferListener = nullptr;



    // Keeps track of the release callback id associated with the listener. This

    // is not sent to the server since the id can be reconstructed there. This

    // is used to remove the old callback from the client process map if it is

    // overwritten by another setBuffer call.

    ReleaseCallbackId releaseCallbackId = ReleaseCallbackId::INVALID_ID;



    // Stores which endpoint the release information should be sent to. We don't want to send the

    // releaseCallbackId and release fence to all listeners so we store which listener the setBuffer

    // was called with.
@@ -92,6 +86,9 @@ struct BufferData { 


    client_cache_t cachedBuffer;



    // Generates the release callback id based on the buffer id and frame number.

    // This is used as an identifier when release callbacks are invoked.

    ReleaseCallbackId generateReleaseCallbackId() const;

    status_t write(Parcel& output) const;

    status_t read(const Parcel& input);

};

libs/gui/include/gui/SurfaceComposerClient.h

+1 −2
Original line number Diff line number Diff line
@@ -416,7 +416,7 @@ public: 


        void cacheBuffers();

        void registerSurfaceControlForCallback(const sp<SurfaceControl>& sc);

        void setReleaseBufferCallback(BufferData*, const ReleaseCallbackId&, ReleaseBufferCallback);

        void setReleaseBufferCallback(BufferData*, ReleaseBufferCallback);



    public:

        Transaction();
@@ -490,7 +490,6 @@ public: 
        Transaction& setBuffer(const sp<SurfaceControl>& sc, const sp<GraphicBuffer>& buffer,

                               const std::optional<sp<Fence>>& fence = std::nullopt,

                               const std::optional<uint64_t>& frameNumber = std::nullopt,

                               const ReleaseCallbackId& id = ReleaseCallbackId::INVALID_ID,

                               ReleaseBufferCallback callback = nullptr);

        std::optional<BufferData> getAndClearBuffer(const sp<SurfaceControl>& sc);

        Transaction& setDataspace(const sp<SurfaceControl>& sc, ui::Dataspace dataspace);