From 2e102c9d5e2333cd8fe984ed67739eefb3b08f19 Mon Sep 17 00:00:00 2001 From: Robert Carr Date: Tue, 23 Oct 2018 12:11:15 -0700 Subject: SurfaceFlinger: Share ownership of layers between State and Handle. Currently the only strong reference to a Layer is held by their parent or the containing layer stack. Firstly, it means that we can not create a SurfaceControl with a null parent. For example, a media decoding library may wish to offer an unparented SurfaceControl representing the decoding Surface, and allow the consumer to parent it between various SurfaceControls as they wish. Secondly it requires lifetime coordination between various levels of the hierarchy, when adding a child you have to be careful to observe the lifetime of the parent because your BufferQueue may become suddenly abandoned at any point. In this change we switch to a reference counted model, such that the Layer remains valid as long as there is a handle to it. We also end the behavior of passing on BufferQueue abandon to children. Layers are only abandoned/disposed when an explicit call to remove is made, or the last reference is dropped. In this CL we switch the handle to holding a strong pointer. We have the handle and the current state forward their references to the main thread to be dropped. We move the contents of onRemoved to the destructor to ensure they are dropped when the last reference is dropped. A second CL will replace the concept of "onRemovedFromCurrentState" with the concept of parent=null, and remove the explicit destroy method, replacing it with an IPC reparent to null. Two additional refactorings are required to follow the other changes. First since we moved mNumLayers to ~Layer it will be executed for the fake parent layer created during screenshots but mNumLayers++ (in addClientLayer) will not be incremented. The most straightforward solution is to balance mNumLayers from the c'tor/d'tor. Second since abandon() is virtual calling it from the d'tor is not safe. We move the call to BufferQueueLayer and remove the abstract method entirely. Bug: 62536731 Bug: 111373437 Bug: 111297488 Test: Transaction_test.cpp Change-Id: I3e815eb9cee39f6def1ef459811448698a93f5c8 --- services/surfaceflinger/SurfaceFlinger.cpp | 80 +++++++++--------------------- 1 file changed, 23 insertions(+), 57 deletions(-) (limited to 'services/surfaceflinger/SurfaceFlinger.cpp') diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 5c31ada5ef..f8ad156254 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -2735,7 +2735,15 @@ void SurfaceFlinger::commitTransaction() for (const auto& l : mLayersPendingRemoval) { recordBufferingStats(l->getName().string(), l->getOccupancyHistory(true)); - l->onRemoved(); + + // We need to release the HWC layers when the Layer is removed + // from the current state otherwise the HWC layer just continues + // showing at its last configured state until we eventually + // abandon the buffer queue. + if (l->isRemovedFromCurrentState()) { + l->destroyAllHwcLayers(); + l->releasePendingBuffer(systemTime()); + } } mLayersPendingRemoval.clear(); } @@ -3168,7 +3176,7 @@ status_t SurfaceFlinger::addClientLayer(const sp& client, if (parent == nullptr) { mCurrentState.layersSortedByZ.add(lbc); } else { - if (parent->isPendingRemoval()) { + if (parent->isRemovedFromCurrentState()) { ALOGE("addClientLayer called with a removed parent"); return NAME_NOT_FOUND; } @@ -3184,7 +3192,6 @@ status_t SurfaceFlinger::addClientLayer(const sp& client, mMaxGraphicBufferProducerListSize, mNumLayers); } mLayersAdded = true; - mNumLayers++; } // attach this layer to the client @@ -3198,52 +3205,22 @@ status_t SurfaceFlinger::removeLayer(const sp& layer, bool topLevelOnly) return removeLayerLocked(mStateLock, layer, topLevelOnly); } -status_t SurfaceFlinger::removeLayerLocked(const Mutex&, const sp& layer, +status_t SurfaceFlinger::removeLayerLocked(const Mutex& lock, const sp& layer, bool topLevelOnly) { - if (layer->isPendingRemoval()) { - return NO_ERROR; - } - const auto& p = layer->getParent(); ssize_t index; if (p != nullptr) { if (topLevelOnly) { return NO_ERROR; } - - sp ancestor = p; - while (ancestor->getParent() != nullptr) { - ancestor = ancestor->getParent(); - } - if (mCurrentState.layersSortedByZ.indexOf(ancestor) < 0) { - ALOGE("removeLayer called with a layer whose parent has been removed"); - return NAME_NOT_FOUND; - } - index = p->removeChild(layer); } else { index = mCurrentState.layersSortedByZ.remove(layer); } - // As a matter of normal operation, the LayerCleaner will produce a second - // attempt to remove the surface. The Layer will be kept alive in mDrawingState - // so we will succeed in promoting it, but it's already been removed - // from mCurrentState. As long as we can find it in mDrawingState we have no problem - // otherwise something has gone wrong and we are leaking the layer. - if (index < 0 && mDrawingState.layersSortedByZ.indexOf(layer) < 0) { - ALOGE("Failed to find layer (%s) in layer parent (%s).", - layer->getName().string(), - (p != nullptr) ? p->getName().string() : "no-parent"); - return BAD_VALUE; - } else if (index < 0) { - return NO_ERROR; - } - layer->onRemovedFromCurrentState(); - mLayersPendingRemoval.add(layer); - mLayersRemoved = true; - mNumLayers -= 1 + layer->getChildrenCount(); - setTransactionFlags(eTransactionNeeded); + + markLayerPendingRemovalLocked(lock, layer); return NO_ERROR; } @@ -3444,11 +3421,6 @@ uint32_t SurfaceFlinger::setClientStateLocked(const ComposerState& composerState return 0; } - if (layer->isPendingRemoval()) { - ALOGW("Attempting to set client state on removed layer: %s", layer->getName().string()); - return 0; - } - uint32_t flags = 0; const uint32_t what = s.what; @@ -3652,11 +3624,6 @@ void SurfaceFlinger::setDestroyStateLocked(const ComposerState& composerState) { return; } - if (layer->isPendingRemoval()) { - ALOGW("Attempting to destroy on removed layer: %s", layer->getName().string()); - return; - } - if (state.what & layer_state_t::eDestroySurface) { removeLayerLocked(mStateLock, layer); } @@ -3830,17 +3797,16 @@ status_t SurfaceFlinger::onLayerRemoved(const sp& client, const sp& layer) +void SurfaceFlinger::markLayerPendingRemovalLocked(const Mutex&, const sp& layer) { + mLayersPendingRemoval.add(layer); + mLayersRemoved = true; + setTransactionFlags(eTransactionNeeded); +} + +void SurfaceFlinger::onHandleDestroyed(const sp& layer) { - // called by ~LayerCleaner() when all references to the IBinder (handle) - // are gone - sp l = layer.promote(); - if (l == nullptr) { - // The layer has already been removed, carry on - return NO_ERROR; - } - // If we have a parent, then we can continue to live as long as it does. - return removeLayer(l, true); + Mutex::Autolock lock(mStateLock); + markLayerPendingRemovalLocked(mStateLock, layer); } // --------------------------------------------------------------------------- @@ -5189,7 +5155,7 @@ status_t SurfaceFlinger::captureLayers(const sp& layerHandleBinder, auto layerHandle = reinterpret_cast(layerHandleBinder.get()); auto parent = layerHandle->owner.promote(); - if (parent == nullptr || parent->isPendingRemoval()) { + if (parent == nullptr || parent->isRemovedFromCurrentState()) { ALOGE("captureLayers called with a removed parent"); return NAME_NOT_FOUND; } -- cgit v1.2.3-59-g8ed1b