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
diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/CachedSet.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/CachedSet.h
index 00424b2..fa87fb8 100644
--- a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/CachedSet.h
+++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/CachedSet.h
@@ -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 @@
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 @@
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 @@
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;
diff --git a/services/surfaceflinger/CompositionEngine/src/planner/CachedSet.cpp b/services/surfaceflinger/CompositionEngine/src/planner/CachedSet.cpp
index ab3fe9e..ba03655 100644
--- a/services/surfaceflinger/CompositionEngine/src/planner/CachedSet.cpp
+++ b/services/surfaceflinger/CompositionEngine/src/planner/CachedSet.cpp
@@ -126,7 +126,7 @@
}
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 @@
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());
}
}
diff --git a/services/surfaceflinger/CompositionEngine/tests/planner/CachedSetTest.cpp b/services/surfaceflinger/CompositionEngine/tests/planner/CachedSetTest.cpp
index 6d1ce4c..c33828f 100644
--- a/services/surfaceflinger/CompositionEngine/tests/planner/CachedSetTest.cpp
+++ b/services/surfaceflinger/CompositionEngine/tests/planner/CachedSetTest.cpp
@@ -310,8 +310,14 @@
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
diff --git a/services/surfaceflinger/CompositionEngine/tests/planner/FlattenerTest.cpp b/services/surfaceflinger/CompositionEngine/tests/planner/FlattenerTest.cpp
index 42bbfcc..c4bd5b3 100644
--- a/services/surfaceflinger/CompositionEngine/tests/planner/FlattenerTest.cpp
+++ b/services/surfaceflinger/CompositionEngine/tests/planner/FlattenerTest.cpp
@@ -55,8 +55,10 @@
// 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;