Revert "SurfaceFlinger: Allows Surfaces to outlive their parents." (4bba3709) · Commits · e / os / android_frameworks_native-old

services/surfaceflinger/Layer.cpp

+11 −8

Original line number	Diff line number	Diff line
		@@ -118,6 +118,12 @@ Layer::~Layer() {
		c->detachLayer(this);
		}

		for (auto& point : mRemoteSyncPoints) {
		point->setTransactionApplied();
		}
		for (auto& point : mLocalSyncPoints) {
		point->setFrameAvailable();
		}
		mFrameTracker.logAndResetStats(mName);
		}

		@@ -135,6 +141,8 @@ void Layer::onLayerDisplayed(const sp<Fence>& /releaseFence/) {}
		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) {
		@@ -155,13 +163,8 @@ void Layer::onRemoved() {

		destroyAllHwcLayers();

		mRemoved = true;

		for (auto& point : mRemoteSyncPoints) {
		point->setTransactionApplied();
		}
		for (auto& point : mLocalSyncPoints) {
		point->setFrameAvailable();
		for (const auto& child : mCurrentChildren) {
		child->onRemoved();
		}
		}

services/surfaceflinger/Layer.h

+4 −4

Original line number	Diff line number	Diff line
		@@ -341,7 +341,7 @@ public:
		virtual bool isCreatedFromMainThread() const { return false; }


		bool isRemoved() const { return mRemoved; }
		bool isPendingRemoval() const { return mPendingRemoval; }

		void writeToProto(LayerProto* layerInfo,
		LayerVector::StateSet stateSet = LayerVector::StateSet::Drawing);
		@@ -589,12 +589,12 @@ protected:
		*/
		class LayerCleaner {
		sp<SurfaceFlinger> mFlinger;
		sp<Layer> mLayer;
		wp<Layer> mLayer;

		protected:
		~LayerCleaner() {
		// destroy client resources
		mFlinger->removeLayer(mLayer, true);
		mFlinger->onLayerDestroyed(mLayer);
		}

		public:
		@@ -739,7 +739,7 @@ protected:
		// Whether filtering is needed b/c of the drawingstate
		bool mNeedsFiltering{false};

		bool mRemoved{false};
		bool mPendingRemoval{false};

		// page-flip thread (currently main thread)
		bool mProtectedByApp{false}; // application requires protected path to external sink

services/surfaceflinger/SurfaceFlinger.cpp

+43 −6

Original line number	Diff line number	Diff line
		@@ -2833,7 +2833,6 @@ void SurfaceFlinger::commitTransaction()
		recordBufferingStats(l->getName().string(),
		l->getOccupancyHistory(true));
		l->onRemoved();
		mNumLayers -= 1;
		}
		mLayersPendingRemoval.clear();
		}
		@@ -3249,7 +3248,7 @@ status_t SurfaceFlinger::addClientLayer(const sp<Client>& client,
		if (parent == nullptr) {
		mCurrentState.layersSortedByZ.add(lbc);
		} else {
		if (parent->isRemoved()) {
		if (parent->isPendingRemoval()) {
		ALOGE("addClientLayer called with a removed parent");
		return NAME_NOT_FOUND;
		}
		@@ -3281,7 +3280,7 @@ status_t SurfaceFlinger::removeLayer(const sp<Layer>& layer, bool topLevelOnly)

		status_t SurfaceFlinger::removeLayerLocked(const Mutex&, const sp<Layer>& layer,
		bool topLevelOnly) {
		if (layer->isRemoved()) {
		if (layer->isPendingRemoval()) {
		return NO_ERROR;
		}

		@@ -3291,14 +3290,39 @@ status_t SurfaceFlinger::removeLayerLocked(const Mutex&, const sp<Layer>& layer,
		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;
		}
		@@ -3501,7 +3525,7 @@ uint32_t SurfaceFlinger::setClientStateLocked(const ComposerState& composerState
		return 0;
		}

		if (layer->isRemoved()) {
		if (layer->isPendingRemoval()) {
		ALOGW("Attempting to set client state on removed layer: %s", layer->getName().string());
		return 0;
		}
		@@ -3704,7 +3728,7 @@ void SurfaceFlinger::setDestroyStateLocked(const ComposerState& composerState) {
		return;
		}

		if (layer->isRemoved()) {
		if (layer->isPendingRemoval()) {
		ALOGW("Attempting to destroy on removed layer: %s", layer->getName().string());
		return;
		}
		@@ -3881,6 +3905,19 @@ status_t SurfaceFlinger::onLayerRemoved(const sp<Client>& client, const sp<IBind
		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() {
		@@ -5254,7 +5291,7 @@ status_t SurfaceFlinger::captureLayers(const sp<IBinder>& layerHandleBinder,
		auto layerHandle = reinterpret_cast<Layer::Handle*>(layerHandleBinder.get());
		auto parent = layerHandle->owner.promote();

		if (parent == nullptr \|\| parent->isRemoved()) {
		if (parent == nullptr \|\| parent->isPendingRemoval()) {
		ALOGE("captureLayers called with a removed parent");
		return NAME_NOT_FOUND;
		}

services/surfaceflinger/SurfaceFlinger.h

+5 −0

Original line number	Diff line number	Diff line
		@@ -559,6 +559,11 @@ private:
		// 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);

services/surfaceflinger/tests/Transaction_test.cpp

+0 −31

Original line number	Diff line number	Diff line
		@@ -2554,37 +2554,6 @@ TEST_F(ChildLayerTest, DetachChildrenSameClient) {
		}
		}

		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 =