Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 43cb3cb3 authored by chaviw's avatar chaviw
Browse files

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
parent 6107273c

services/surfaceflinger/BufferLayer.cpp

+6 −0
Original line number Diff line number Diff line
@@ -545,6 +545,12 @@ void BufferLayer::notifyAvailableFrames() { 
        if (headFrameNumber >= point->getFrameNumber() && headFenceSignaled &&

            presentTimeIsCurrent) {

            point->setFrameAvailable();

            sp<Layer> requestedSyncLayer = point->getRequestedSyncLayer();

            if (requestedSyncLayer) {

                // Need to update the transaction flag to ensure the layer's pending transaction

                // gets applied.

                requestedSyncLayer->setTransactionFlags(eTransactionNeeded);

            }

        }

    }

}

services/surfaceflinger/Layer.cpp

+17 −5
Original line number Diff line number Diff line
@@ -144,6 +144,20 @@ Layer::~Layer() { 
 */

void Layer::onLayerDisplayed(const sp<Fence>& /*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<SyncPoint>(mCurrentState.frameNumber_legacy);

            auto syncPoint = std::make_shared<SyncPoint>(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();

        }

    }

services/surfaceflinger/Layer.h

+10 −2
Original line number Diff line number Diff line
@@ -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<Layer> 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<Layer> getRequestedSyncLayer() { return mRequestedSyncLayer.promote(); }



    private:

        const uint64_t mFrameNumber;

        std::atomic<bool> mFrameIsAvailable;

        std::atomic<bool> mTransactionIsApplied;

        wp<Layer> mRequestedSyncLayer;

    };



    // SyncPoints which will be signaled when the correct frame is at the head
@@ -928,6 +934,8 @@ private: 
    void setZOrderRelativeOf(const wp<Layer>& relativeOf);



    bool mGetHandleCalled = false;



    void removeRemoteSyncPoints();

};



} // namespace android

services/surfaceflinger/tests/Transaction_test.cpp

+42 −0
Original line number Diff line number Diff line
@@ -4725,6 +4725,48 @@ TEST_F(ChildLayerTest, DetachChildrenThenAttach) { 
        mCapture->expectColor(Rect(20, 20, 52, 52), Color::RED);

    }

}

TEST_F(ChildLayerTest, DetachChildrenWithDeferredTransaction) {

    sp<SurfaceComposerClient> newComposerClient = new SurfaceComposerClient;

    sp<SurfaceControl> 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) {