From f1961f713de2b3f54c8ce7653964b969e1a02bc8 Mon Sep 17 00:00:00 2001 From: chaviw Date: Mon, 18 Sep 2017 16:41:07 -0700 Subject: Re-parent invoked on child instead of on parent. The function to re-parent an individual child is now invoked on the child instead of the parent. This ensures the child ends up with the last parent set if multiple reparent requests are made in the same transaction. This also allows adding a parent to a layer that didn't have one previously. Test: Transaction_test -> Reparent, ReparentToNoParent, ReparentFromNoParent Change-Id: Idab429eb2dca5a4ae1b020a5a7629d719dd4d995 --- libs/gui/LayerState.cpp | 2 - libs/gui/SurfaceComposerClient.cpp | 19 +++--- libs/gui/SurfaceControl.cpp | 5 +- libs/gui/include/gui/SurfaceComposerClient.h | 3 +- libs/gui/include/gui/SurfaceControl.h | 7 +-- libs/gui/include/private/gui/LayerState.h | 3 +- services/surfaceflinger/Layer.cpp | 28 +++------ services/surfaceflinger/Layer.h | 2 +- services/surfaceflinger/SurfaceFlinger.cpp | 6 +- services/surfaceflinger/SurfaceFlinger_hwc1.cpp | 6 +- services/surfaceflinger/tests/Transaction_test.cpp | 67 +++++++++++++++++++++- 11 files changed, 94 insertions(+), 54 deletions(-) diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp index 573f6856d6..3418a4983e 100644 --- a/libs/gui/LayerState.cpp +++ b/libs/gui/LayerState.cpp @@ -46,7 +46,6 @@ status_t layer_state_t::write(Parcel& output) const output.writeStrongBinder(IInterface::asBinder(barrierGbp)); output.writeStrongBinder(relativeLayerHandle); output.writeStrongBinder(parentHandleForChild); - output.writeStrongBinder(childHandle); output.write(transparentRegion); return NO_ERROR; } @@ -80,7 +79,6 @@ status_t layer_state_t::read(const Parcel& input) interface_cast(input.readStrongBinder()); relativeLayerHandle = input.readStrongBinder(); parentHandleForChild = input.readStrongBinder(); - childHandle = input.readStrongBinder(); input.read(transparentRegion); return NO_ERROR; } diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index b0ae7e0bdf..be7b1d2f6a 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -176,9 +176,8 @@ public: status_t reparentChildren(const sp& client, const sp& id, const sp& newParentHandle); - status_t reparentChild(const sp& client, - const sp& id, const sp& newParentHandle, - const sp& childHandle); + status_t reparent(const sp& client, + const sp& id, const sp& newParentHandle); status_t detachChildren(const sp& client, const sp& id); status_t setOverrideScalingMode(const sp& client, @@ -496,18 +495,16 @@ status_t Composer::reparentChildren( return NO_ERROR; } -status_t Composer::reparentChild(const sp& client, +status_t Composer::reparent(const sp& client, const sp& id, - const sp& newParentHandle, - const sp& childHandle) { + const sp& newParentHandle) { Mutex::Autolock lock(mLock); layer_state_t* s = getLayerStateLocked(client, id); if (!s) { return BAD_INDEX; } - s->what |= layer_state_t::eReparentChild; + s->what |= layer_state_t::eReparent; s->parentHandleForChild = newParentHandle; - s->childHandle = childHandle; return NO_ERROR; } @@ -849,9 +846,9 @@ status_t SurfaceComposerClient::reparentChildren(const sp& id, return getComposer().reparentChildren(this, id, newParentHandle); } -status_t SurfaceComposerClient::reparentChild(const sp& id, - const sp& newParentHandle, const sp& childHandle) { - return getComposer().reparentChild(this, id, newParentHandle, childHandle); +status_t SurfaceComposerClient::reparent(const sp& id, + const sp& newParentHandle) { + return getComposer().reparent(this, id, newParentHandle); } status_t SurfaceComposerClient::detachChildren(const sp& id) { diff --git a/libs/gui/SurfaceControl.cpp b/libs/gui/SurfaceControl.cpp index b9c5ef9580..d801d12738 100644 --- a/libs/gui/SurfaceControl.cpp +++ b/libs/gui/SurfaceControl.cpp @@ -191,11 +191,10 @@ status_t SurfaceControl::reparentChildren(const sp& newParentHandle) { return mClient->reparentChildren(mHandle, newParentHandle); } -status_t SurfaceControl::reparentChild(const sp& newParentHandle, - const sp& childHandle) { +status_t SurfaceControl::reparent(const sp& newParentHandle) { status_t err = validate(); if (err < 0) return err; - return mClient->reparentChild(mHandle, newParentHandle, childHandle); + return mClient->reparent(mHandle, newParentHandle); } status_t SurfaceControl::detachChildren() { diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 6e2cb83544..cf2ff5b96e 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -161,8 +161,7 @@ public: const sp& handle, uint64_t frameNumber); status_t reparentChildren(const sp& id, const sp& newParentHandle); - status_t reparentChild(const sp& id, const sp& newParentHandle, - const sp& childHandle); + status_t reparent(const sp& id, const sp& newParentHandle); status_t detachChildren(const sp& id); status_t setOverrideScalingMode(const sp& id, int32_t overrideScalingMode); diff --git a/libs/gui/include/gui/SurfaceControl.h b/libs/gui/include/gui/SurfaceControl.h index d8b67ef96a..b506e00672 100644 --- a/libs/gui/include/gui/SurfaceControl.h +++ b/libs/gui/include/gui/SurfaceControl.h @@ -124,11 +124,10 @@ public: // Reparents all children of this layer to the new parent handle. status_t reparentChildren(const sp& newParentHandle); - // Reparents a specified child from this layer to the new parent handle. - // The child, parent, and new parent must all have the same client. + // Reparents the current layer to the new parent handle. The new parent must not be null. // This can be used instead of reparentChildren if the caller wants to - // only re-parent specific children. - status_t reparentChild(const sp& newParentHandle, const sp& childHandle); + // only re-parent a specific child. + status_t reparent(const sp& newParentHandle); // Detaches all child surfaces (and their children recursively) // from their SurfaceControl. diff --git a/libs/gui/include/private/gui/LayerState.h b/libs/gui/include/private/gui/LayerState.h index 4f73e04e22..4ff2e5e0d6 100644 --- a/libs/gui/include/private/gui/LayerState.h +++ b/libs/gui/include/private/gui/LayerState.h @@ -60,7 +60,7 @@ struct layer_state_t { eReparentChildren = 0x00002000, eDetachChildren = 0x00004000, eRelativeLayerChanged = 0x00008000, - eReparentChild = 0x00010000 + eReparent = 0x00010000 }; layer_state_t() @@ -109,7 +109,6 @@ struct layer_state_t { sp relativeLayerHandle; sp parentHandleForChild; - sp childHandle; // non POD must be last. see write/read Region transparentRegion; diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index 9435a187d9..fd30e1614b 100755 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -2625,8 +2625,8 @@ bool Layer::reparentChildren(const sp& newParentHandle) { return true; } -bool Layer::reparentChild(const sp& newParentHandle, const sp& childHandle) { - if (newParentHandle == nullptr || childHandle == nullptr) { +bool Layer::reparent(const sp& newParentHandle) { + if (newParentHandle == nullptr) { return false; } @@ -2637,29 +2637,19 @@ bool Layer::reparentChild(const sp& newParentHandle, const sp& return false; } - handle = static_cast(childHandle.get()); - sp child = handle->owner.promote(); - if (child == nullptr) { - ALOGE("Unable to promote child Layer handle"); - return false; - } - - if (mCurrentChildren.indexOf(child) < 0) { - ALOGE("Child layer is not child of current layer"); - return false; + sp parent = getParent(); + if (parent != nullptr) { + parent->removeChild(this); } + newParent->addChild(this); - sp parentClient(mClientRef.promote()); - sp childClient(child->mClientRef.promote()); + sp client(mClientRef.promote()); sp newParentClient(newParent->mClientRef.promote()); - if (parentClient != childClient || childClient != newParentClient) { - ALOGE("Current layer, child layer, and new parent layer must have the same client"); - return false; + if (client != newParentClient) { + client->setParentLayer(newParent); } - newParent->addChild(child); - mCurrentChildren.remove(child); return true; } diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index f94833b32f..e7ece4579e 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -241,7 +241,7 @@ public: bool setOverrideScalingMode(int32_t overrideScalingMode); void setInfo(uint32_t type, uint32_t appId); bool reparentChildren(const sp& layer); - bool reparentChild(const sp& newParentHandle, const sp& childHandle); + bool reparent(const sp& newParentHandle); bool detachChildren(); // If we have received a new buffer this frame, we will pass its surface diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index cae4deacb0..6ee14fe72a 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -3121,10 +3121,8 @@ uint32_t SurfaceFlinger::setClientStateLocked( // We don't trigger a traversal here because if no other state is // changed, we don't want this to cause any more work } - // Always re-parent the children that explicitly requested to get - // re-parented before the general re-parent of all children. - if (what & layer_state_t::eReparentChild) { - if (layer->reparentChild(s.parentHandleForChild, s.childHandle)) { + if (what & layer_state_t::eReparent) { + if (layer->reparent(s.parentHandleForChild)) { flags |= eTransactionNeeded|eTraversalNeeded; } } diff --git a/services/surfaceflinger/SurfaceFlinger_hwc1.cpp b/services/surfaceflinger/SurfaceFlinger_hwc1.cpp index 71aa52dee6..1cf6ce33b7 100644 --- a/services/surfaceflinger/SurfaceFlinger_hwc1.cpp +++ b/services/surfaceflinger/SurfaceFlinger_hwc1.cpp @@ -2679,10 +2679,8 @@ uint32_t SurfaceFlinger::setClientStateLocked( // We don't trigger a traversal here because if no other state is // changed, we don't want this to cause any more work } - // Always re-parent the children that explicitly requested to get - // re-parented before the general re-parent of all children. - if (what & layer_state_t::eReparentChild) { - if (layer->reparentChild(s.parentHandleForChild, s.childHandle)) { + if (what & layer_state_t::eReparent) { + if (layer->reparent(s.parentHandleForChild)) { flags |= eTransactionNeeded|eTraversalNeeded; } } diff --git a/services/surfaceflinger/tests/Transaction_test.cpp b/services/surfaceflinger/tests/Transaction_test.cpp index dea6503e4e..21194926da 100644 --- a/services/surfaceflinger/tests/Transaction_test.cpp +++ b/services/surfaceflinger/tests/Transaction_test.cpp @@ -1169,7 +1169,7 @@ TEST_F(ChildLayerTest, Bug36858924) { fillSurfaceRGBA8(mFGSurfaceControl, 0, 255, 0); } -TEST_F(ChildLayerTest, ReparentChild) { +TEST_F(ChildLayerTest, Reparent) { SurfaceComposerClient::openGlobalTransaction(); mChild->show(); mChild->setPosition(10, 10); @@ -1185,7 +1185,7 @@ TEST_F(ChildLayerTest, ReparentChild) { // And 10 more pixels we should be back to the foreground surface mCapture->expectFGColor(84, 84); } - mFGSurfaceControl->reparentChild(mBGSurfaceControl->getHandle(), mChild->getHandle()); + mChild->reparent(mBGSurfaceControl->getHandle()); { ScreenCapture::captureScreen(&mCapture); mCapture->expectFGColor(64, 64); @@ -1198,6 +1198,69 @@ TEST_F(ChildLayerTest, ReparentChild) { } } +TEST_F(ChildLayerTest, ReparentToNoParent) { + SurfaceComposerClient::openGlobalTransaction(); + mChild->show(); + mChild->setPosition(10, 10); + mFGSurfaceControl->setPosition(64, 64); + SurfaceComposerClient::closeGlobalTransaction(true); + + { + ScreenCapture::captureScreen(&mCapture); + // Top left of foreground must now be visible + mCapture->expectFGColor(64, 64); + // But 10 pixels in we should see the child surface + mCapture->expectChildColor(74, 74); + // And 10 more pixels we should be back to the foreground surface + mCapture->expectFGColor(84, 84); + } + mChild->reparent(nullptr); + { + ScreenCapture::captureScreen(&mCapture); + // Nothing should have changed. + mCapture->expectFGColor(64, 64); + mCapture->expectChildColor(74, 74); + mCapture->expectFGColor(84, 84); + } +} + +TEST_F(ChildLayerTest, ReparentFromNoParent) { + sp newSurface = mComposerClient->createSurface( + String8("New Surface"), 10, 10, PIXEL_FORMAT_RGBA_8888, 0); + ASSERT_TRUE(newSurface != NULL); + ASSERT_TRUE(newSurface->isValid()); + + fillSurfaceRGBA8(newSurface, 63, 195, 63); + SurfaceComposerClient::openGlobalTransaction(); + mChild->hide(); + newSurface->show(); + newSurface->setPosition(10, 10); + newSurface->setLayer(INT32_MAX-2); + mFGSurfaceControl->setPosition(64, 64); + SurfaceComposerClient::closeGlobalTransaction(true); + + { + ScreenCapture::captureScreen(&mCapture); + // Top left of foreground must now be visible + mCapture->expectFGColor(64, 64); + // At 10, 10 we should see the new surface + mCapture->checkPixel(10, 10, 63, 195, 63); + } + + SurfaceComposerClient::openGlobalTransaction(); + newSurface->reparent(mFGSurfaceControl->getHandle()); + SurfaceComposerClient::closeGlobalTransaction(true); + + { + ScreenCapture::captureScreen(&mCapture); + // newSurface will now be a child of mFGSurface so it will be 10, 10 offset from + // mFGSurface, putting it at 74, 74. + mCapture->expectFGColor(64, 64); + mCapture->checkPixel(74, 74, 63, 195, 63); + mCapture->expectFGColor(84, 84); + } +} + TEST_F(ChildLayerTest, NestedChildren) { sp grandchild = mComposerClient->createSurface( String8("Grandchild surface"), -- cgit v1.2.3-59-g8ed1b