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

Commit b62adaaf authored by Chavi Weingarten's avatar Chavi Weingarten Committed by Vishnu Nair
Browse files

Drop the buffer from the cache if the buffer is dropped from the transaction 

This can happen with the scenario below 1. App sets buffer1 into a
Transaction but doesn't apply it. 2. The Transaction is sent to another
process (system server in this case) 3. The buffer is then cached
locally in the process in writeToParcel 4. App sets buffer2 into a new
Transaction but doesn't apply it. 5. Transaction2 is sent to the other
process (system server) 6. Transaction1 and Transaction2 are merged
causing the buffer in Transaction2 to overwrite the buffer in
Transaction1.

This causes the buffer to never get sent to SF and then never placed in
the SF cache, but the client still thinks this buffer is cached and will
continue to just send cacheId but no buffer when this buffer is reused.
This can result in dropped frames since SF will not be able to find the
buffer when trying to draw it

Bug: 385976595
Test: atest ReleaseBufferCallbackTest#CacheBufferOverwriteTransaction
Flag: EXEMPT bug fix
Change-Id: Iffc316f9ffd00579cba3068e7bb0ffa52b805763
parent 0d77e992

libs/gui/ITransactionCompletedListener.cpp

+3 −2
Original line number Diff line number Diff line
@@ -278,11 +278,12 @@ public: 
    }



    void onReleaseBuffer(ReleaseCallbackId callbackId, sp<Fence> releaseFence,

                         uint32_t currentMaxAcquiredBufferCount) override {

                         uint32_t currentMaxAcquiredBufferCount, bool removeFromCache) override {

        callRemoteAsync<decltype(&ITransactionCompletedListener::

                                         onReleaseBuffer)>(Tag::ON_RELEASE_BUFFER, callbackId,

                                                           releaseFence,

                                                           currentMaxAcquiredBufferCount);

                                                           currentMaxAcquiredBufferCount,

                                                           removeFromCache);

    }



    void onTransactionQueueStalled(const String8& reason) override {

libs/gui/SurfaceComposerClient.cpp

+34 −14
Original line number Diff line number Diff line
@@ -637,7 +637,8 @@ void TransactionCompletedListener::removeQueueStallListener(void* id) { 


void TransactionCompletedListener::onReleaseBuffer(ReleaseCallbackId callbackId,

                                                   sp<Fence> releaseFence,

                                                   uint32_t currentMaxAcquiredBufferCount) {

                                                   uint32_t currentMaxAcquiredBufferCount,

                                                   bool removeFromCache) {

    ReleaseBufferCallback callback;

    {

        std::scoped_lock<std::mutex> lock(mMutex);
@@ -648,6 +649,10 @@ void TransactionCompletedListener::onReleaseBuffer(ReleaseCallbackId callbackId, 
              callbackId.to_string().c_str());

        return;

    }

    if (removeFromCache) {

        ALOGV("Dropping buffer %" PRIu64 " from cache", callbackId.bufferId);

        SurfaceComposerClient::getDefault()->removeBufferFromLocalCache(callbackId.bufferId);

    }

    std::optional<uint32_t> optionalMaxAcquiredBufferCount =

            currentMaxAcquiredBufferCount == UINT_MAX

            ? std::nullopt
@@ -776,9 +781,9 @@ public: 
        return buffer->getId();

    }



    void uncache(uint64_t cacheId) {

    void uncache(uint64_t cacheId, bool uncacheInSf) {

        std::lock_guard<std::mutex> lock(mMutex);

        if (mBuffers.erase(cacheId)) {

        if (mBuffers.erase(cacheId) && uncacheInSf) {

            SurfaceComposerClient::doUncacheBufferTransaction(cacheId);

        }

    }
@@ -818,7 +823,7 @@ ANDROID_SINGLETON_STATIC_INSTANCE(BufferCache); 


void removeDeadBufferCallback(void* /*context*/, uint64_t graphicBufferId) {

    // GraphicBuffer id's are used as the cache ids.

    BufferCache::getInstance().uncache(graphicBufferId);

    BufferCache::getInstance().uncache(graphicBufferId, true /* uncacheInSf */);

}



// ---------------------------------------------------------------------------
@@ -936,22 +941,31 @@ status_t SurfaceComposerClient::Transaction::writeToParcel(Parcel* parcel) const 
}



void SurfaceComposerClient::Transaction::releaseBufferIfOverwriting(const layer_state_t& state) {

    if (!(state.what & layer_state_t::eBufferChanged) || !state.bufferData->hasBuffer()) {

    if (!(state.what & layer_state_t::eBufferChanged)) {

        return;

    }



    auto listener = state.bufferData->releaseBufferListener;

    sp<Fence> fence =

            state.bufferData->acquireFence ? state.bufferData->acquireFence : Fence::NO_FENCE;

    // Tell the owning process to remove the buffer from the cache if the buffer being dropped was

    // going to be added to the SurfaceFlinger cache. Without this, it causes issues where the

    // client thinks it's cached the buffer correctly, but it was never cached in SF causing

    // subsequent look ups to fail.

    bool removeFromCache =

            (state.bufferData->hasBuffer() && state.bufferData->cachedBuffer.isValid());

    ReleaseCallbackId releaseCallbackId = state.bufferData->generateReleaseCallbackId();

    ALOGV("dropping buffer=%" PRIu64 " and removeFromCache=%d", releaseCallbackId.bufferId,

          removeFromCache);

    if (state.bufferData->releaseBufferEndpoint ==

        IInterface::asBinder(TransactionCompletedListener::getIInstance())) {

        // 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->generateReleaseCallbackId(), fence);

    } else {

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

                ->mReleaseCallbackThread.addReleaseCallback(releaseCallbackId, fence,

                                                            removeFromCache);

    } else if (listener) {

        listener->onReleaseBuffer(releaseCallbackId, fence, UINT_MAX, removeFromCache);

    }

}
@@ -3276,6 +3290,11 @@ status_t SurfaceComposerClient::removeWindowInfosListener( 
void SurfaceComposerClient::notifyShutdown() {

    ComposerServiceAIDL::getComposerService()->notifyShutdown();

}



void SurfaceComposerClient::removeBufferFromLocalCache(uint64_t bufferId) {

    BufferCache::getInstance().uncache(bufferId, false /* uncacheInSf */);

}



// ----------------------------------------------------------------------------



status_t ScreenshotClient::captureDisplay(const DisplayCaptureArgs& captureArgs,
@@ -3316,20 +3335,20 @@ status_t ScreenshotClient::captureLayers(const LayerCaptureArgs& captureArgs, 
// ---------------------------------------------------------------------------------



void ReleaseCallbackThread::addReleaseCallback(const ReleaseCallbackId callbackId,

                                               sp<Fence> releaseFence) {

                                               sp<Fence> releaseFence, bool removeFromCache) {

    std::scoped_lock<std::mutex> lock(mMutex);

    if (!mStarted) {

        mThread = std::thread(&ReleaseCallbackThread::threadMain, this);

        mStarted = true;

    }



    mCallbackInfos.emplace(callbackId, std::move(releaseFence));

    mCallbackInfos.emplace(callbackId, std::move(releaseFence), removeFromCache);

    mReleaseCallbackPending.notify_one();

}



void ReleaseCallbackThread::threadMain() {

    const auto listener = TransactionCompletedListener::getInstance();

    std::queue<std::tuple<const ReleaseCallbackId, const sp<Fence>>> callbackInfos;

    std::queue<std::tuple<const ReleaseCallbackId, const sp<Fence>, bool>> callbackInfos;

    while (true) {

        {

            std::unique_lock<std::mutex> lock(mMutex);
@@ -3339,8 +3358,9 @@ void ReleaseCallbackThread::threadMain() { 
        }



        while (!callbackInfos.empty()) {

            auto [callbackId, releaseFence] = callbackInfos.front();

            listener->onReleaseBuffer(callbackId, std::move(releaseFence), UINT_MAX);

            auto [callbackId, releaseFence, removeFromCache] = callbackInfos.front();

            listener->onReleaseBuffer(callbackId, std::move(releaseFence), UINT_MAX,

                                      removeFromCache);

            callbackInfos.pop();

        }

libs/gui/include/gui/ITransactionCompletedListener.h

+1 −1
Original line number Diff line number Diff line
@@ -172,7 +172,7 @@ public: 
    virtual void onTransactionCompleted(ListenerStats stats) = 0;



    virtual void onReleaseBuffer(ReleaseCallbackId callbackId, sp<Fence> releaseFence,

                                 uint32_t currentMaxAcquiredBufferCount) = 0;

                                 uint32_t currentMaxAcquiredBufferCount, bool removeFromCache) = 0;



    virtual void onTransactionQueueStalled(const String8& name) = 0;

libs/gui/include/gui/SurfaceComposerClient.h

+5 −3
Original line number Diff line number Diff line
@@ -122,7 +122,7 @@ using TrustedPresentationCallback = std::function<void(void*, bool)>; 


class ReleaseCallbackThread {

public:

    void addReleaseCallback(const ReleaseCallbackId, sp<Fence>);

    void addReleaseCallback(const ReleaseCallbackId, sp<Fence>, bool removeFromCache);

    void threadMain();



private:
@@ -130,7 +130,7 @@ private: 
    std::mutex mMutex;

    bool mStarted GUARDED_BY(mMutex) = false;

    std::condition_variable mReleaseCallbackPending;

    std::queue<std::tuple<const ReleaseCallbackId, const sp<Fence>>> mCallbackInfos

    std::queue<std::tuple<const ReleaseCallbackId, const sp<Fence>, bool>> mCallbackInfos

            GUARDED_BY(mMutex);

};
@@ -888,6 +888,8 @@ public: 


    static void notifyShutdown();



    void removeBufferFromLocalCache(uint64_t bufferId);



protected:

    ReleaseCallbackThread mReleaseCallbackThread;
@@ -1076,7 +1078,7 @@ public: 
    // BnTransactionCompletedListener overrides

    void onTransactionCompleted(ListenerStats stats) override;

    void onReleaseBuffer(ReleaseCallbackId, sp<Fence> releaseFence,

                         uint32_t currentMaxAcquiredBufferCount) override;

                         uint32_t currentMaxAcquiredBufferCount, bool removeFromCache) override;



    void removeReleaseBufferCallback(const ReleaseCallbackId& callbackId);

services/surfaceflinger/Layer.cpp

+2 −1
Original line number Diff line number Diff line
@@ -732,7 +732,8 @@ void Layer::callReleaseBufferCallback(const sp<ITransactionCompletedListener>& l 
    }



    if (listener) {

        listener->onReleaseBuffer(callbackId, fence, currentMaxAcquiredBufferCount);

        listener->onReleaseBuffer(callbackId, fence, currentMaxAcquiredBufferCount,

                                  false /* removeFromCache */);

    }



    if (!mBufferReleaseChannel) {