From 43cb3cb3f14afed11d91db2091fbffecaf7a88e6 Mon Sep 17 00:00:00 2001 From: chaviw Date: Fri, 31 May 2019 15:23:41 -0700 Subject: Clear remoteSyncPoints for detached layers. When a layer gets detached, the remote sync points should get removed so the buffer layer isn't waiting for that transaction to get applied. Detached layers can never apply transactions so the buffer layer will wait forever for the transaction to get applied. Test: Steps from bug Test: ChildLayerTest.DetachChildrenWithDeferredTransaction Fixes: 132125338 Change-Id: I333855a70a40152457f39e953bc7300d696c7c62 --- services/surfaceflinger/BufferLayer.cpp | 6 ++++ services/surfaceflinger/Layer.cpp | 22 +++++++++--- services/surfaceflinger/Layer.h | 12 +++++-- services/surfaceflinger/tests/Transaction_test.cpp | 42 ++++++++++++++++++++++ 4 files changed, 75 insertions(+), 7 deletions(-) diff --git a/services/surfaceflinger/BufferLayer.cpp b/services/surfaceflinger/BufferLayer.cpp index a2b6ab6b4d..9d723998c4 100644 --- a/services/surfaceflinger/BufferLayer.cpp +++ b/services/surfaceflinger/BufferLayer.cpp @@ -545,6 +545,12 @@ void BufferLayer::notifyAvailableFrames() { if (headFrameNumber >= point->getFrameNumber() && headFenceSignaled && presentTimeIsCurrent) { point->setFrameAvailable(); + sp requestedSyncLayer = point->getRequestedSyncLayer(); + if (requestedSyncLayer) { + // Need to update the transaction flag to ensure the layer's pending transaction + // gets applied. + requestedSyncLayer->setTransactionFlags(eTransactionNeeded); + } } } } diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index 379b004f9e..3ca6ef500d 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -144,6 +144,20 @@ Layer::~Layer() { */ void Layer::onLayerDisplayed(const sp& /*releaseFence*/) {} +void Layer::removeRemoteSyncPoints() { + for (auto& point : mRemoteSyncPoints) { + point->setTransactionApplied(); + } + mRemoteSyncPoints.clear(); + + { + Mutex::Autolock pendingStateLock(mPendingStateMutex); + for (State pendingState : mPendingStates) { + pendingState.barrierLayer_legacy = nullptr; + } + } +} + void Layer::onRemovedFromCurrentState() { mRemovedFromCurrentState = true; @@ -162,10 +176,7 @@ void Layer::onRemovedFromCurrentState() { // never finish applying transactions. We signal the sync point // now so that another layer will not become indefinitely // blocked. - for (auto& point: mRemoteSyncPoints) { - point->setTransactionApplied(); - } - mRemoteSyncPoints.clear(); + removeRemoteSyncPoints(); { Mutex::Autolock syncLock(mLocalSyncPointMutex); @@ -667,7 +678,7 @@ void Layer::pushPendingState() { // to be applied as per normal (no synchronization). mCurrentState.barrierLayer_legacy = nullptr; } else { - auto syncPoint = std::make_shared(mCurrentState.frameNumber_legacy); + auto syncPoint = std::make_shared(mCurrentState.frameNumber_legacy, this); if (barrierLayer->addSyncPoint(syncPoint)) { mRemoteSyncPoints.push_back(std::move(syncPoint)); } else { @@ -1510,6 +1521,7 @@ bool Layer::detachChildren() { if (client != nullptr && parentClient != client) { child->mLayerDetached = true; child->detachChildren(); + child->removeRemoteSyncPoints(); } } diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index 3712b2aff9..bbbc5b0f78 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -733,8 +733,11 @@ protected: class SyncPoint { public: - explicit SyncPoint(uint64_t frameNumber) - : mFrameNumber(frameNumber), mFrameIsAvailable(false), mTransactionIsApplied(false) {} + explicit SyncPoint(uint64_t frameNumber, wp requestedSyncLayer) + : mFrameNumber(frameNumber), + mFrameIsAvailable(false), + mTransactionIsApplied(false), + mRequestedSyncLayer(requestedSyncLayer) {} uint64_t getFrameNumber() const { return mFrameNumber; } @@ -746,10 +749,13 @@ protected: void setTransactionApplied() { mTransactionIsApplied = true; } + sp getRequestedSyncLayer() { return mRequestedSyncLayer.promote(); } + private: const uint64_t mFrameNumber; std::atomic mFrameIsAvailable; std::atomic mTransactionIsApplied; + wp mRequestedSyncLayer; }; // SyncPoints which will be signaled when the correct frame is at the head @@ -928,6 +934,8 @@ private: void setZOrderRelativeOf(const wp& relativeOf); bool mGetHandleCalled = false; + + void removeRemoteSyncPoints(); }; } // namespace android diff --git a/services/surfaceflinger/tests/Transaction_test.cpp b/services/surfaceflinger/tests/Transaction_test.cpp index ec1ac4b229..62db5ece38 100644 --- a/services/surfaceflinger/tests/Transaction_test.cpp +++ b/services/surfaceflinger/tests/Transaction_test.cpp @@ -4725,6 +4725,48 @@ TEST_F(ChildLayerTest, DetachChildrenThenAttach) { mCapture->expectColor(Rect(20, 20, 52, 52), Color::RED); } } +TEST_F(ChildLayerTest, DetachChildrenWithDeferredTransaction) { + sp newComposerClient = new SurfaceComposerClient; + sp childNewClient = + newComposerClient->createSurface(String8("New Child Test Surface"), 10, 10, + PIXEL_FORMAT_RGBA_8888, 0, mFGSurfaceControl.get()); + + ASSERT_TRUE(childNewClient != nullptr); + ASSERT_TRUE(childNewClient->isValid()); + + fillSurfaceRGBA8(childNewClient, 200, 200, 200); + + Transaction() + .hide(mChild) + .show(childNewClient) + .setPosition(childNewClient, 10, 10) + .setPosition(mFGSurfaceControl, 64, 64) + .apply(); + + { + mCapture = screenshot(); + Rect rect = Rect(74, 74, 84, 84); + mCapture->expectBorder(rect, Color{195, 63, 63, 255}); + mCapture->expectColor(rect, Color{200, 200, 200, 255}); + } + + Transaction() + .deferTransactionUntil_legacy(childNewClient, mFGSurfaceControl->getHandle(), + mFGSurfaceControl->getSurface()->getNextFrameNumber()) + .apply(); + Transaction().detachChildren(mFGSurfaceControl).apply(); + ASSERT_NO_FATAL_FAILURE(fillBufferQueueLayerColor(mFGSurfaceControl, Color::RED, 32, 32)); + + // BufferLayer can still dequeue buffers even though there's a detached layer with a + // deferred transaction. + { + SCOPED_TRACE("new buffer"); + mCapture = screenshot(); + Rect rect = Rect(74, 74, 84, 84); + mCapture->expectBorder(rect, Color::RED); + mCapture->expectColor(rect, Color{200, 200, 200, 255}); + } +} TEST_F(ChildLayerTest, ChildrenInheritNonTransformScalingFromParent) { asTransaction([&](Transaction& t) { -- cgit v1.2.3-59-g8ed1b