SurfaceFlinger: Allows Surfaces to outlive their parents.
Currently the only strong reference to a Layer is held by
their parent or the containing layer stack. This has a few
undesirable implications. 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.
Bug: 62536731
Bug: 111373437
Bug: 111297488
Test: Transaction_test.cpp
Change-Id: I04cbc2368a1049b8ebd8913673ed4bfe05a26280
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index f9bc1e7..63a010d 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -118,12 +118,6 @@
c->detachLayer(this);
}
- for (auto& point : mRemoteSyncPoints) {
- point->setTransactionApplied();
- }
- for (auto& point : mLocalSyncPoints) {
- point->setFrameAvailable();
- }
mFrameTracker.logAndResetStats(mName);
}
@@ -141,8 +135,6 @@
void Layer::onRemovedFromCurrentState() {
// the layer is removed from SF mCurrentState to mLayersPendingRemoval
- mPendingRemoval = true;
-
if (mCurrentState.zOrderRelativeOf != nullptr) {
sp<Layer> strongRelative = mCurrentState.zOrderRelativeOf.promote();
if (strongRelative != nullptr) {
@@ -151,7 +143,7 @@
}
mCurrentState.zOrderRelativeOf = nullptr;
}
-
+
for (const auto& child : mCurrentChildren) {
child->onRemovedFromCurrentState();
}
@@ -163,8 +155,13 @@
destroyAllHwcLayers();
- for (const auto& child : mCurrentChildren) {
- child->onRemoved();
+ mRemoved = true;
+
+ for (auto& point : mRemoteSyncPoints) {
+ point->setTransactionApplied();
+ }
+ for (auto& point : mLocalSyncPoints) {
+ point->setFrameAvailable();
}
}
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index 874b551..3e6eba4 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -341,7 +341,7 @@
virtual bool isCreatedFromMainThread() const { return false; }
- bool isPendingRemoval() const { return mPendingRemoval; }
+ bool isRemoved() const { return mRemoved; }
void writeToProto(LayerProto* layerInfo,
LayerVector::StateSet stateSet = LayerVector::StateSet::Drawing);
@@ -589,12 +589,12 @@
*/
class LayerCleaner {
sp<SurfaceFlinger> mFlinger;
- wp<Layer> mLayer;
+ sp<Layer> mLayer;
protected:
~LayerCleaner() {
// destroy client resources
- mFlinger->onLayerDestroyed(mLayer);
+ mFlinger->removeLayer(mLayer, true);
}
public:
@@ -739,7 +739,7 @@
// Whether filtering is needed b/c of the drawingstate
bool mNeedsFiltering{false};
- bool mPendingRemoval{false};
+ bool mRemoved{false};
// page-flip thread (currently main thread)
bool mProtectedByApp{false}; // application requires protected path to external sink
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 9410cdb..7ccf8bc 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2833,6 +2833,7 @@
recordBufferingStats(l->getName().string(),
l->getOccupancyHistory(true));
l->onRemoved();
+ mNumLayers -= 1;
}
mLayersPendingRemoval.clear();
}
@@ -3248,7 +3249,7 @@
if (parent == nullptr) {
mCurrentState.layersSortedByZ.add(lbc);
} else {
- if (parent->isPendingRemoval()) {
+ if (parent->isRemoved()) {
ALOGE("addClientLayer called with a removed parent");
return NAME_NOT_FOUND;
}
@@ -3280,7 +3281,7 @@
status_t SurfaceFlinger::removeLayerLocked(const Mutex&, const sp<Layer>& layer,
bool topLevelOnly) {
- if (layer->isPendingRemoval()) {
+ if (layer->isRemoved()) {
return NO_ERROR;
}
@@ -3290,39 +3291,14 @@
if (topLevelOnly) {
return NO_ERROR;
}
-
- sp<Layer> 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);
return NO_ERROR;
}
@@ -3525,7 +3501,7 @@
return 0;
}
- if (layer->isPendingRemoval()) {
+ if (layer->isRemoved()) {
ALOGW("Attempting to set client state on removed layer: %s", layer->getName().string());
return 0;
}
@@ -3728,7 +3704,7 @@
return;
}
- if (layer->isPendingRemoval()) {
+ if (layer->isRemoved()) {
ALOGW("Attempting to destroy on removed layer: %s", layer->getName().string());
return;
}
@@ -3905,19 +3881,6 @@
return err;
}
-status_t SurfaceFlinger::onLayerDestroyed(const wp<Layer>& layer)
-{
- // called by ~LayerCleaner() when all references to the IBinder (handle)
- // are gone
- sp<Layer> 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);
-}
-
// ---------------------------------------------------------------------------
void SurfaceFlinger::onInitializeDisplays() {
@@ -5291,7 +5254,7 @@
auto layerHandle = reinterpret_cast<Layer::Handle*>(layerHandleBinder.get());
auto parent = layerHandle->owner.promote();
- if (parent == nullptr || parent->isPendingRemoval()) {
+ if (parent == nullptr || parent->isRemoved()) {
ALOGE("captureLayers called with a removed parent");
return NAME_NOT_FOUND;
}
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index b77bf48..1bf1c5a 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -559,11 +559,6 @@
// ISurfaceComposerClient::destroySurface()
status_t onLayerRemoved(const sp<Client>& client, const sp<IBinder>& handle);
- // 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.
- status_t onLayerDestroyed(const wp<Layer>& layer);
-
// remove a layer from SurfaceFlinger immediately
status_t removeLayer(const sp<Layer>& layer, bool topLevelOnly = false);
status_t removeLayerLocked(const Mutex&, const sp<Layer>& layer, bool topLevelOnly = false);
diff --git a/services/surfaceflinger/tests/Transaction_test.cpp b/services/surfaceflinger/tests/Transaction_test.cpp
index 3af98e5..5e37332 100644
--- a/services/surfaceflinger/tests/Transaction_test.cpp
+++ b/services/surfaceflinger/tests/Transaction_test.cpp
@@ -2554,6 +2554,37 @@
}
}
+TEST_F(ChildLayerTest, ChildrenSurviveParentDestruction) {
+ sp<SurfaceControl> mGrandChild =
+ mClient->createSurface(String8("Grand Child"), 10, 10,
+ PIXEL_FORMAT_RGBA_8888, 0, mChild.get());
+ fillSurfaceRGBA8(mGrandChild, 111, 111, 111);
+
+ {
+ SCOPED_TRACE("Grandchild visible");
+ ScreenCapture::captureScreen(&mCapture);
+ mCapture->checkPixel(64, 64, 111, 111, 111);
+ }
+
+ mChild->clear();
+
+ {
+ SCOPED_TRACE("After destroying child");
+ ScreenCapture::captureScreen(&mCapture);
+ mCapture->expectFGColor(64, 64);
+ }
+
+ asTransaction([&](Transaction& t) {
+ t.reparent(mGrandChild, mFGSurfaceControl->getHandle());
+ });
+
+ {
+ SCOPED_TRACE("After reparenting grandchild");
+ ScreenCapture::captureScreen(&mCapture);
+ mCapture->checkPixel(64, 64, 111, 111, 111);
+ }
+}
+
TEST_F(ChildLayerTest, DetachChildrenDifferentClient) {
sp<SurfaceComposerClient> mNewComposerClient = new SurfaceComposerClient;
sp<SurfaceControl> mChildNewClient =