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

Commit 49da70dc authored by Alec Mouri's avatar Alec Mouri
Browse files

Don't leak flattened buffers from cached sets. 

unbindExternalTextureBuffer needs to be called when a CachedSet is
destroyed. Otherwise GPU textures are leaked.

This is implemented as a Texture RAII instead of modifying CachedSet's
destructor directly, because that's closer to the long-term buffer
lifecycle solution for RenderEngine.

Bug: 182415252
Test: observe dumpsys SurfaceFlinger while playing youtube video

Change-Id: Ib2d0a497621c121021189827cdc64b11ce9ef458
parent 9b24e29e

services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/CachedSet.h

+29 −7
Original line number Diff line number Diff line
@@ -17,15 +17,12 @@ 
#pragma once



#include <compositionengine/impl/planner/LayerState.h>

#include <renderengine/RenderEngine.h>



#include <chrono>



namespace android {



namespace renderengine {

class RenderEngine;

} // namespace renderengine



namespace compositionengine::impl::planner {



std::string durationString(std::chrono::milliseconds duration);
@@ -63,7 +60,7 @@ public: 
    const Layer& getFirstLayer() const { return mLayers[0]; }

    const Rect& getBounds() const { return mBounds; }

    size_t getAge() const { return mAge; }

    const sp<GraphicBuffer>& getBuffer() const { return mBuffer; }

    const sp<GraphicBuffer>& getBuffer() const { return mTexture.getBuffer(); }

    const sp<Fence>& getDrawFence() const { return mDrawFence; }



    NonBufferHash getNonBufferHash() const;
@@ -82,7 +79,7 @@ public: 


    void setLastUpdate(std::chrono::steady_clock::time_point now) { mLastUpdate = now; }

    void append(const CachedSet& other) {

        mBuffer = nullptr;

        mTexture.setBuffer(nullptr, nullptr);

        mDrawFence = nullptr;



        mLayers.insert(mLayers.end(), other.mLayers.cbegin(), other.mLayers.cend());
@@ -105,7 +102,32 @@ private: 
    std::vector<Layer> mLayers;

    Rect mBounds = Rect::EMPTY_RECT;

    size_t mAge = 0;

    sp<GraphicBuffer> mBuffer;



    class Texture {

    public:

        ~Texture() { setBuffer(nullptr, nullptr); }



        void setBuffer(const sp<GraphicBuffer>& buffer, renderengine::RenderEngine* re) {

            if (mRE && mBuffer) {

                mRE->unbindExternalTextureBuffer(mBuffer->getId());

            }



            mBuffer = buffer;

            mRE = re;



            if (mRE && mBuffer) {

                mRE->cacheExternalTextureBuffer(mBuffer);

            }

        }



        const sp<GraphicBuffer>& getBuffer() const { return mBuffer; }



    private:

        sp<GraphicBuffer> mBuffer = nullptr;

        renderengine::RenderEngine* mRE = nullptr;

    };



    Texture mTexture;

    sp<Fence> mDrawFence;



    static const bool sDebugHighlighLayers;

services/surfaceflinger/CompositionEngine/src/planner/CachedSet.cpp

+3 −2
Original line number Diff line number Diff line
@@ -126,7 +126,7 @@ bool CachedSet::hasBufferUpdate(std::vector<const LayerState*>::const_iterator l 
}



bool CachedSet::hasReadyBuffer() const {

    return mBuffer != nullptr && mDrawFence->getStatus() == Fence::Status::Signaled;

    return mTexture.getBuffer() != nullptr && mDrawFence->getStatus() == Fence::Status::Signaled;

}



std::vector<CachedSet> CachedSet::decompose() const {
@@ -209,11 +209,12 @@ void CachedSet::render(renderengine::RenderEngine& renderEngine) { 
                                                 HAL_PIXEL_FORMAT_RGBA_8888, 1, usageFlags);

    LOG_ALWAYS_FATAL_IF(buffer->initCheck() != OK);

    base::unique_fd drawFence;



    status_t result = renderEngine.drawLayers(displaySettings, layerSettingsPointers, buffer, false,

                                              base::unique_fd(), &drawFence);



    if (result == NO_ERROR) {

        mBuffer = buffer;

        mTexture.setBuffer(buffer, &renderEngine);

        mDrawFence = new Fence(drawFence.release());

    }

}

services/surfaceflinger/CompositionEngine/tests/planner/CachedSetTest.cpp

+6 −0
Original line number Diff line number Diff line
@@ -310,8 +310,14 @@ TEST_F(CachedSetTest, render) { 
    EXPECT_CALL(*layerFE1, prepareClientCompositionList(_)).WillOnce(Return(clientCompList1));

    EXPECT_CALL(*layerFE2, prepareClientCompositionList(_)).WillOnce(Return(clientCompList2));

    EXPECT_CALL(mRenderEngine, drawLayers(_, _, _, _, _, _)).WillOnce(Invoke(drawLayers));

    EXPECT_CALL(mRenderEngine, cacheExternalTextureBuffer(_));

    cachedSet.render(mRenderEngine);

    expectReadyBuffer(cachedSet);



    // Now check that appending a new cached set properly cleans up RenderEngine resources.

    EXPECT_CALL(mRenderEngine, unbindExternalTextureBuffer(_));

    CachedSet::Layer& layer3 = *mTestLayers[2]->cachedSetLayer.get();

    cachedSet.append(CachedSet(layer3));

}



} // namespace

services/surfaceflinger/CompositionEngine/tests/planner/FlattenerTest.cpp

+3 −1
Original line number Diff line number Diff line
@@ -55,8 +55,10 @@ protected: 
    // TODO(b/181192467): Once Flattener starts to do something useful with Predictor,

    // mPredictor should be mocked and checked for expectations.

    Predictor mPredictor;

    std::unique_ptr<Flattener> mFlattener;



    // mRenderEngine may be held as a pointer to mFlattener, so mFlattener must be destroyed first.

    renderengine::mock::RenderEngine mRenderEngine;

    std::unique_ptr<Flattener> mFlattener;



    const std::chrono::steady_clock::time_point kStartTime = std::chrono::steady_clock::now();

    std::chrono::steady_clock::time_point mTime = kStartTime;