From 4545a8a3ffece9db0732cb9183ed253dc22e8216 Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Thu, 8 Aug 2019 20:05:32 -0700 Subject: [SurfaceFlinger] Callback to renderengine when erasing BLAST buffers Otherwise we may leak if BufferStateLayer is destroyed first. Bug: 137514000 Test: Over 61 hours, ran: while [ true ]; do am start -n \ com.android.chrome/com.google.android.apps.chrome.Main \ http://m.youtube.com; sleep 10; input tap 740 740 ; sleep 10; input \ keyevent HOME; sleep 0.5; am force-stop com.android.chrome; sleep 0.5; \ done Test: Over >30 minutes: while [ true ]; do am start -n \ com.android.chrome/com.google.android.apps.chrome.Main \ http://m.youtube.com; sleep 10; input tap 740 740; \ sleep 1; content insert --uri content://settings/system --bind \ name:s:user_rotation --bind value:i:1; sleep 4; content insert --uri \ content://settings/system --bind name:s:user_rotation --bind value:i:0; \ sleep 5; input keyevent HOME; sleep 0.5; \ am force-stop com.android.chrome; sleep 0.5; done Test: CtsViewTestCases:ASurfaceControlTest Test: CtsViewTestCases:SurfaceControlTest Test: Transaction_test Change-Id: I743eb8bd9887d17e08b6f1b8e8ec5874359df175 --- services/surfaceflinger/BufferLayer.h | 2 +- services/surfaceflinger/BufferStateLayer.cpp | 14 ++++++++++---- services/surfaceflinger/BufferStateLayer.h | 5 ++--- services/surfaceflinger/ClientCache.cpp | 18 ++++++++++-------- services/surfaceflinger/ClientCache.h | 4 ++-- services/surfaceflinger/Layer.h | 5 +---- services/surfaceflinger/SurfaceFlinger.cpp | 21 ++++++++++++++++----- services/surfaceflinger/SurfaceFlinger.h | 7 +++++-- 8 files changed, 47 insertions(+), 29 deletions(-) diff --git a/services/surfaceflinger/BufferLayer.h b/services/surfaceflinger/BufferLayer.h index b679380b79..46a62eda91 100644 --- a/services/surfaceflinger/BufferLayer.h +++ b/services/surfaceflinger/BufferLayer.h @@ -48,7 +48,7 @@ namespace android { class BufferLayer : public Layer { public: explicit BufferLayer(const LayerCreationArgs& args); - ~BufferLayer() override; + virtual ~BufferLayer() override; // ----------------------------------------------------------------------- // Overriden from Layer diff --git a/services/surfaceflinger/BufferStateLayer.cpp b/services/surfaceflinger/BufferStateLayer.cpp index 2abc1a75a9..63a07c35ca 100644 --- a/services/surfaceflinger/BufferStateLayer.cpp +++ b/services/surfaceflinger/BufferStateLayer.cpp @@ -51,6 +51,16 @@ BufferStateLayer::BufferStateLayer(const LayerCreationArgs& args) mCurrentState.dataspace = ui::Dataspace::V0_SRGB; } +BufferStateLayer::~BufferStateLayer() { + if (mActiveBuffer != nullptr) { + // Ensure that mActiveBuffer is uncached from RenderEngine here, as + // RenderEngine may have been using the buffer as an external texture + // after the client uncached the buffer. + auto& engine(mFlinger->getRenderEngine()); + engine.unbindExternalTextureBuffer(mActiveBuffer->getId()); + } +} + // ----------------------------------------------------------------------- // Interface implementation for Layer // ----------------------------------------------------------------------- @@ -610,10 +620,6 @@ void BufferStateLayer::onFirstRef() { } } -void BufferStateLayer::bufferErased(const client_cache_t& clientCacheId) { - mFlinger->getRenderEngine().unbindExternalTextureBuffer(clientCacheId.id); -} - void BufferStateLayer::HwcSlotGenerator::bufferErased(const client_cache_t& clientCacheId) { std::lock_guard lock(mMutex); if (!clientCacheId.isValid()) { diff --git a/services/surfaceflinger/BufferStateLayer.h b/services/surfaceflinger/BufferStateLayer.h index db8ae0d337..97e24cb277 100644 --- a/services/surfaceflinger/BufferStateLayer.h +++ b/services/surfaceflinger/BufferStateLayer.h @@ -35,6 +35,8 @@ class BufferStateLayer : public BufferLayer { public: explicit BufferStateLayer(const LayerCreationArgs&); + ~BufferStateLayer() override; + // ----------------------------------------------------------------------- // Interface implementation for Layer // ----------------------------------------------------------------------- @@ -102,9 +104,6 @@ public: bool fenceHasSignaled() const override; bool framePresentTimeIsCurrent() const override; - // Inherit from ClientCache::ErasedRecipient - void bufferErased(const client_cache_t& clientCacheId) override; - private: nsecs_t getDesiredPresentTime() override; std::shared_ptr getCurrentFenceTime() const override; diff --git a/services/surfaceflinger/ClientCache.cpp b/services/surfaceflinger/ClientCache.cpp index 77f2f5765c..16fe27c00a 100644 --- a/services/surfaceflinger/ClientCache.cpp +++ b/services/surfaceflinger/ClientCache.cpp @@ -55,16 +55,16 @@ bool ClientCache::getBuffer(const client_cache_t& cacheId, return true; } -void ClientCache::add(const client_cache_t& cacheId, const sp& buffer) { +bool ClientCache::add(const client_cache_t& cacheId, const sp& buffer) { auto& [processToken, id] = cacheId; if (processToken == nullptr) { ALOGE("failed to cache buffer: invalid process token"); - return; + return false; } if (!buffer) { ALOGE("failed to cache buffer: invalid buffer"); - return; + return false; } std::lock_guard lock(mMutex); @@ -77,13 +77,13 @@ void ClientCache::add(const client_cache_t& cacheId, const sp& bu token = processToken.promote(); if (!token) { ALOGE("failed to cache buffer: invalid token"); - return; + return false; } status_t err = token->linkToDeath(mDeathRecipient); if (err != NO_ERROR) { ALOGE("failed to cache buffer: could not link to death"); - return; + return false; } auto [itr, success] = mBuffers.emplace(processToken, std::unordered_map()); @@ -95,10 +95,11 @@ void ClientCache::add(const client_cache_t& cacheId, const sp& bu if (processBuffers.size() > BUFFER_CACHE_MAX_SIZE) { ALOGE("failed to cache buffer: cache is full"); - return; + return false; } processBuffers[id].buffer = buffer; + return true; } void ClientCache::erase(const client_cache_t& cacheId) { @@ -139,16 +140,17 @@ sp ClientCache::get(const client_cache_t& cacheId) { return buf->buffer; } -void ClientCache::registerErasedRecipient(const client_cache_t& cacheId, +bool ClientCache::registerErasedRecipient(const client_cache_t& cacheId, const wp& recipient) { std::lock_guard lock(mMutex); ClientCacheBuffer* buf = nullptr; if (!getBuffer(cacheId, &buf)) { ALOGE("failed to register erased recipient, could not retrieve buffer"); - return; + return false; } buf->recipients.insert(recipient); + return true; } void ClientCache::unregisterErasedRecipient(const client_cache_t& cacheId, diff --git a/services/surfaceflinger/ClientCache.h b/services/surfaceflinger/ClientCache.h index 9f057c45d4..aa6c80dfa7 100644 --- a/services/surfaceflinger/ClientCache.h +++ b/services/surfaceflinger/ClientCache.h @@ -36,7 +36,7 @@ class ClientCache : public Singleton { public: ClientCache(); - void add(const client_cache_t& cacheId, const sp& buffer); + bool add(const client_cache_t& cacheId, const sp& buffer); void erase(const client_cache_t& cacheId); sp get(const client_cache_t& cacheId); @@ -48,7 +48,7 @@ public: virtual void bufferErased(const client_cache_t& clientCacheId) = 0; }; - void registerErasedRecipient(const client_cache_t& cacheId, + bool registerErasedRecipient(const client_cache_t& cacheId, const wp& recipient); void unregisterErasedRecipient(const client_cache_t& cacheId, const wp& recipient); diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index ec7389ebad..b46eb112e7 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -93,7 +93,7 @@ struct LayerCreationArgs { LayerMetadata metadata; }; -class Layer : public virtual compositionengine::LayerFE, public ClientCache::ErasedRecipient { +class Layer : public virtual compositionengine::LayerFE { static std::atomic sSequence; public: @@ -700,9 +700,6 @@ public: compositionengine::OutputLayer* findOutputLayerForDisplay( const sp& display) const; - // Inherit from ClientCache::ErasedRecipient - void bufferErased(const client_cache_t& /*clientCacheId*/) override {} - protected: // constant sp mFlinger; diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index e6d0dcb3a6..b31bc3813a 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -3843,6 +3843,7 @@ void SurfaceFlinger::applyTransactionState(const Vector& states, if (uncacheBuffer.isValid()) { ClientCache::getInstance().erase(uncacheBuffer); + getRenderEngine().unbindExternalTextureBuffer(uncacheBuffer.id); } // If a synchronous transaction is explicitly requested without any changes, force a transaction @@ -4197,12 +4198,18 @@ uint32_t SurfaceFlinger::setClientStateLocked( bool bufferChanged = what & layer_state_t::eBufferChanged; bool cacheIdChanged = what & layer_state_t::eCachedBufferChanged; sp buffer; - if (bufferChanged && cacheIdChanged) { - ClientCache::getInstance().add(s.cachedBuffer, s.buffer); - ClientCache::getInstance().registerErasedRecipient(s.cachedBuffer, - wp(layer)); - getRenderEngine().cacheExternalTextureBuffer(s.buffer); + if (bufferChanged && cacheIdChanged && s.buffer != nullptr) { buffer = s.buffer; + bool success = ClientCache::getInstance().add(s.cachedBuffer, s.buffer); + if (success) { + getRenderEngine().cacheExternalTextureBuffer(s.buffer); + success = ClientCache::getInstance() + .registerErasedRecipient(s.cachedBuffer, + wp(this)); + if (!success) { + getRenderEngine().unbindExternalTextureBuffer(s.buffer->getId()); + } + } } else if (cacheIdChanged) { buffer = ClientCache::getInstance().get(s.cachedBuffer); } else if (bufferChanged) { @@ -6249,6 +6256,10 @@ sp SurfaceFlinger::fromHandle(const sp& handle) { return nullptr; } +void SurfaceFlinger::bufferErased(const client_cache_t& clientCacheId) { + getRenderEngine().unbindExternalTextureBuffer(clientCacheId.id); +} + } // namespace android #if defined(__gl_h_) diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 97d67aaf4f..0c6da0077f 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -170,9 +170,9 @@ public: class SurfaceFlinger : public BnSurfaceComposer, public PriorityDumper, + public ClientCache::ErasedRecipient, private IBinder::DeathRecipient, - private HWC2::ComposerCallback -{ + private HWC2::ComposerCallback { public: SurfaceFlingerBE& getBE() { return mBE; } const SurfaceFlingerBE& getBE() const { return mBE; } @@ -328,6 +328,9 @@ public: sp fromHandle(const sp& handle) REQUIRES(mStateLock); + // Inherit from ClientCache::ErasedRecipient + void bufferErased(const client_cache_t& clientCacheId) override; + private: friend class BufferLayer; friend class BufferQueueLayer; -- cgit v1.2.3-59-g8ed1b