SurfaceFlinger: Remove removeLayer
We remove explicit layer destruction and replace it
with reparent->null, completing the transition to
a reference counted model.
Test: Manual
Bug: 62536731
Bug: 111373437
Bug: 111297488
Change-Id: I8ac7c5c5125e1c8daf84b42db00e1dd93a544bb5
diff --git a/services/surfaceflinger/Client.cpp b/services/surfaceflinger/Client.cpp
index ee4ec50..4f6fb1c 100644
--- a/services/surfaceflinger/Client.cpp
+++ b/services/surfaceflinger/Client.cpp
@@ -39,26 +39,6 @@
{
}
-Client::~Client()
-{
- // We need to post a message to remove our remaining layers rather than
- // do so directly by acquiring the SurfaceFlinger lock. If we were to
- // attempt to directly call the lock it becomes effectively impossible
- // to use sp<Client> while holding the SF lock as descoping it could
- // then trigger a dead-lock.
-
- const size_t count = mLayers.size();
- for (size_t i=0 ; i<count ; i++) {
- sp<Layer> l = mLayers.valueAt(i).promote();
- if (l == nullptr) {
- continue;
- }
- mFlinger->postMessageAsync(new LambdaMessage([flinger = mFlinger, l]() {
- flinger->removeLayer(l);
- }));
- }
-}
-
status_t Client::initCheck() const {
return NO_ERROR;
}
@@ -115,10 +95,6 @@
ownerUid, handle, gbp, &parent);
}
-status_t Client::destroySurface(const sp<IBinder>& handle) {
- return mFlinger->onLayerRemoved(this, handle);
-}
-
status_t Client::clearLayerFrameStats(const sp<IBinder>& handle) const {
sp<Layer> layer = getLayerUser(handle);
if (layer == nullptr) {
diff --git a/services/surfaceflinger/Client.h b/services/surfaceflinger/Client.h
index 4a74739..d0051de 100644
--- a/services/surfaceflinger/Client.h
+++ b/services/surfaceflinger/Client.h
@@ -39,13 +39,12 @@
{
public:
explicit Client(const sp<SurfaceFlinger>& flinger);
- ~Client();
+ ~Client() = default;
status_t initCheck() const;
// protected by SurfaceFlinger::mStateLock
void attachLayer(const sp<IBinder>& handle, const sp<Layer>& layer);
-
void detachLayer(const Layer* layer);
sp<Layer> getLayerUser(const sp<IBinder>& handle) const;
@@ -59,8 +58,6 @@
sp<IBinder>* handle,
sp<IGraphicBufferProducer>* gbp);
- virtual status_t destroySurface(const sp<IBinder>& handle);
-
virtual status_t clearLayerFrameStats(const sp<IBinder>& handle) const;
virtual status_t getLayerFrameStats(const sp<IBinder>& handle, FrameStats* outStats) const;
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index b1827c1..3f2d10a 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -179,6 +179,8 @@
for (const auto& child : mCurrentChildren) {
child->onRemovedFromCurrentState();
}
+
+ mFlinger->markLayerPendingRemovalLocked(this);
}
void Layer::addToCurrentState() {
@@ -1571,6 +1573,7 @@
ssize_t Layer::removeChild(const sp<Layer>& layer) {
layer->setParent(nullptr);
+
return mCurrentChildren.remove(layer);
}
@@ -1605,14 +1608,14 @@
}
bool Layer::reparent(const sp<IBinder>& newParentHandle) {
- if (newParentHandle == nullptr) {
- return false;
- }
+ bool callSetTransactionFlags = false;
- auto handle = static_cast<Handle*>(newParentHandle.get());
- sp<Layer> newParent = handle->owner.promote();
- if (newParent == nullptr) {
- ALOGE("Unable to promote Layer handle");
+ // While layers are detached, we allow most operations
+ // and simply halt performing the actual transaction. However
+ // for reparent != null we would enter the mRemovedFromCurrentState
+ // state, regardless of whether doTransaction was called, and
+ // so we need to prevent the update here.
+ if (mLayerDetached && newParentHandle == nullptr) {
return false;
}
@@ -1620,17 +1623,31 @@
if (parent != nullptr) {
parent->removeChild(this);
}
- newParent->addChild(this);
- if (!newParent->isRemovedFromCurrentState()) {
- addToCurrentState();
+ if (newParentHandle != nullptr) {
+ auto handle = static_cast<Handle*>(newParentHandle.get());
+ sp<Layer> newParent = handle->owner.promote();
+ if (newParent == nullptr) {
+ ALOGE("Unable to promote Layer handle");
+ return false;
+ }
+
+ newParent->addChild(this);
+ if (!newParent->isRemovedFromCurrentState()) {
+ addToCurrentState();
+ } else {
+ onRemovedFromCurrentState();
+ }
+
+ if (mLayerDetached) {
+ mLayerDetached = false;
+ callSetTransactionFlags = true;
+ }
+ } else {
+ onRemovedFromCurrentState();
}
- if (mLayerDetached) {
- mLayerDetached = false;
- setTransactionFlags(eTransactionNeeded);
- }
- if (attachChildren()) {
+ if (callSetTransactionFlags || attachChildren()) {
setTransactionFlags(eTransactionNeeded);
}
return true;
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index fd25abf..3648be4 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2799,10 +2799,8 @@
// showing at its last configured state until we eventually
// abandon the buffer queue.
if (l->isRemovedFromCurrentState()) {
- l->traverseInZOrder(LayerVector::StateSet::Drawing, [&](Layer* child) {
- child->destroyHwcLayersForAllDisplays();
- latchAndReleaseBuffer(child);
- });
+ l->destroyHwcLayersForAllDisplays();
+ latchAndReleaseBuffer(l);
}
}
mLayersPendingRemoval.clear();
@@ -3276,30 +3274,6 @@
return NO_ERROR;
}
-status_t SurfaceFlinger::removeLayer(const sp<Layer>& layer) {
- Mutex::Autolock _l(mStateLock);
- return removeLayerLocked(mStateLock, layer);
-}
-
-status_t SurfaceFlinger::removeLayerLocked(const Mutex& lock, const sp<Layer>& layer) {
- if (layer->isLayerDetached()) {
- return NO_ERROR;
- }
-
- const auto& p = layer->getParent();
- ssize_t index;
- if (p != nullptr) {
- index = p->removeChild(layer);
- } else {
- index = mCurrentState.layersSortedByZ.remove(layer);
- }
-
- layer->onRemovedFromCurrentState();
-
- markLayerPendingRemovalLocked(lock, layer);
- return NO_ERROR;
-}
-
uint32_t SurfaceFlinger::peekTransactionFlags() {
return mTransactionFlags;
}
@@ -3434,14 +3408,6 @@
}
transactionFlags |= clientStateFlags;
- // Iterate through all layers again to determine if any need to be destroyed. Marking layers
- // as destroyed should only occur after setting all other states. This is to allow for a
- // child re-parent to happen before marking its original parent as destroyed (which would
- // then mark the child as destroyed).
- for (const ComposerState& state : states) {
- setDestroyStateLocked(state);
- }
-
transactionFlags |= addInputWindowCommands(inputWindowCommands);
// If a synchronous transaction is explicitly requested without any changes, force a transaction
@@ -3767,20 +3733,6 @@
return flags;
}
-void SurfaceFlinger::setDestroyStateLocked(const ComposerState& composerState) {
- const layer_state_t& state = composerState.state;
- sp<Client> client(static_cast<Client*>(composerState.client.get()));
-
- sp<Layer> layer(client->getLayerUser(state.surface));
- if (layer == nullptr) {
- return;
- }
-
- if (state.what & layer_state_t::eDestroySurface) {
- removeLayerLocked(mStateLock, layer);
- }
-}
-
uint32_t SurfaceFlinger::addInputWindowCommands(const InputWindowCommands& inputWindowCommands) {
uint32_t flags = 0;
if (!inputWindowCommands.transferTouchFocusCommands.empty()) {
@@ -3960,21 +3912,7 @@
}
-status_t SurfaceFlinger::onLayerRemoved(const sp<Client>& client, const sp<IBinder>& handle)
-{
- // called by a client when it wants to remove a Layer
- status_t err = NO_ERROR;
- sp<Layer> l(client->getLayerUser(handle));
- if (l != nullptr) {
- mInterceptor->saveSurfaceDeletion(l);
- err = removeLayer(l);
- ALOGE_IF(err<0 && err != NAME_NOT_FOUND,
- "error removing layer=%p (%s)", l.get(), strerror(-err));
- }
- return err;
-}
-
-void SurfaceFlinger::markLayerPendingRemovalLocked(const Mutex&, const sp<Layer>& layer) {
+void SurfaceFlinger::markLayerPendingRemovalLocked(const sp<Layer>& layer) {
mLayersPendingRemoval.add(layer);
mLayersRemoved = true;
setTransactionFlags(eTransactionNeeded);
@@ -3983,7 +3921,14 @@
void SurfaceFlinger::onHandleDestroyed(sp<Layer>& layer)
{
Mutex::Autolock lock(mStateLock);
- markLayerPendingRemovalLocked(mStateLock, layer);
+ // If a layer has a parent, we allow it to out-live it's handle
+ // with the idea that the parent holds a reference and will eventually
+ // be cleaned up. However no one cleans up the top-level so we do so
+ // here.
+ if (layer->getParent() == nullptr) {
+ mCurrentState.layersSortedByZ.remove(layer);
+ }
+ markLayerPendingRemovalLocked(layer);
layer.clear();
}
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index bfc87a0..b8bfcf1 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -569,7 +569,6 @@
bool composerStateContainsUnsignaledFences(const Vector<ComposerState>& states);
uint32_t setClientStateLocked(const ComposerState& composerState);
uint32_t setDisplayStateLocked(const DisplayState& s);
- void setDestroyStateLocked(const ComposerState& composerState);
uint32_t addInputWindowCommands(const InputWindowCommands& inputWindowCommands);
/* ------------------------------------------------------------------------
@@ -599,20 +598,11 @@
String8 getUniqueLayerName(const String8& name);
- // called in response to the window-manager calling
- // ISurfaceComposerClient::destroySurface()
- status_t onLayerRemoved(const sp<Client>& client, const sp<IBinder>& handle);
-
- void markLayerPendingRemovalLocked(const Mutex& /* mStateLock */, const sp<Layer>& layer);
-
// called when all clients have released all their references to
// this layer meaning it is entirely safe to destroy all
// resources associated to this layer.
void onHandleDestroyed(sp<Layer>& layer);
-
- // remove a layer from SurfaceFlinger immediately
- status_t removeLayer(const sp<Layer>& layer);
- status_t removeLayerLocked(const Mutex&, const sp<Layer>& layer);
+ void markLayerPendingRemovalLocked(const sp<Layer>& layer);
// add a layer to SurfaceFlinger
status_t addClientLayer(const sp<Client>& client,
diff --git a/services/surfaceflinger/tests/Stress_test.cpp b/services/surfaceflinger/tests/Stress_test.cpp
index 3e1be8e..89c26f4 100644
--- a/services/surfaceflinger/tests/Stress_test.cpp
+++ b/services/surfaceflinger/tests/Stress_test.cpp
@@ -34,7 +34,7 @@
auto surf = client->createSurface(String8("t"), 100, 100,
PIXEL_FORMAT_RGBA_8888, 0);
ASSERT_TRUE(surf != nullptr);
- client->destroySurface(surf->getHandle());
+ surf->clear();
}
};
diff --git a/services/surfaceflinger/tests/SurfaceInterceptor_test.cpp b/services/surfaceflinger/tests/SurfaceInterceptor_test.cpp
index e506757..8ec3e15 100644
--- a/services/surfaceflinger/tests/SurfaceInterceptor_test.cpp
+++ b/services/surfaceflinger/tests/SurfaceInterceptor_test.cpp
@@ -806,18 +806,6 @@
Increment::IncrementCase::kSurfaceCreation);
}
-TEST_F(SurfaceInterceptorTest, InterceptSurfaceDeletionWorks) {
- enableInterceptor();
- sp<SurfaceControl> layerToDelete = mComposerClient->createSurface(String8(LAYER_NAME),
- SIZE_UPDATE, SIZE_UPDATE, PIXEL_FORMAT_RGBA_8888, 0);
- mComposerClient->destroySurface(layerToDelete->getHandle());
- disableInterceptor();
-
- Trace capturedTrace;
- ASSERT_EQ(NO_ERROR, readProtoFile(&capturedTrace));
- ASSERT_TRUE(singleIncrementFound(capturedTrace, Increment::IncrementCase::kSurfaceDeletion));
-}
-
TEST_F(SurfaceInterceptorTest, InterceptDisplayCreationWorks) {
captureTest(&SurfaceInterceptorTest::displayCreation,
Increment::IncrementCase::kDisplayCreation);
diff --git a/services/surfaceflinger/tests/Transaction_test.cpp b/services/surfaceflinger/tests/Transaction_test.cpp
index d118ad6..991ea36 100644
--- a/services/surfaceflinger/tests/Transaction_test.cpp
+++ b/services/surfaceflinger/tests/Transaction_test.cpp
@@ -1023,7 +1023,7 @@
.setRelativeLayer(layerG, layerR->getHandle(), 1)
.apply();
- mClient->destroySurface(layerG->getHandle());
+ layerG->clear();
// layerG should have been removed
screenshot()->expectColor(Rect(0, 0, 32, 32), Color::RED);
}
@@ -4030,9 +4030,9 @@
asTransaction([&](Transaction& t) { t.reparent(mChild, nullptr); });
{
mCapture = screenshot();
- // Nothing should have changed.
+ // The surface should now be offscreen.
mCapture->expectFGColor(64, 64);
- mCapture->expectChildColor(74, 74);
+ mCapture->expectFGColor(74, 74);
mCapture->expectFGColor(84, 84);
}
}
@@ -4610,7 +4610,7 @@
ASSERT_NO_FATAL_FAILURE(fillBufferQueueLayerColor(redLayer, Color::RED, 60, 60));
auto redLayerHandle = redLayer->getHandle();
- mClient->destroySurface(redLayerHandle);
+ redLayer->clear();
SurfaceComposerClient::Transaction().apply(true);
sp<GraphicBuffer> outBuffer;