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

Commit cd52e2db authored by Vishnu Nair's avatar Vishnu Nair
Browse files

SurfaceFlinger: Fix duplicate callbacks 

Transaction callbacks are emitted early, for empty transactions,
after latch for on commit callbacks and on post composition for
transaction complete callbacks. Pending callbacks are stored
together. If a transaction contains a layer that is presented
and a layer that is not presented, the transaction callback will
get added to the pending list after the transaction is committed
and it will get emitted after buffers are latched and then again
at the end of composition. To fix this we make sure to only emit
on commit callbacks after a buffer is latched.

Test: atest SurfaceFlinger_test
Bug: 202394221

Change-Id: I356fbf221812060c17765c53cc3df24cb3cd139a
parent 6e461bf8

services/surfaceflinger/SurfaceFlinger.cpp

+36 −35
Original line number Original line Diff line number Diff line
@@ -2021,17 +2021,23 @@ bool SurfaceFlinger::commit(nsecs_t frameTime, int64_t vsyncId, nsecs_t expected 
bool SurfaceFlinger::flushAndCommitTransactions() {

bool SurfaceFlinger::flushAndCommitTransactions() {

    ATRACE_CALL();

    ATRACE_CALL();





    bool needsTraversal = false;

    if (clearTransactionFlags(eTransactionFlushNeeded)) {

    if (clearTransactionFlags(eTransactionFlushNeeded)) {

        flushTransactionQueues();

        needsTraversal = flushTransactionQueues();

    }

    }





    const bool shouldCommit = (getTransactionFlags() & ~eTransactionFlushNeeded) || mForceTraversal;

    const bool shouldCommit = (getTransactionFlags() & ~eTransactionFlushNeeded) || needsTraversal;

    if (shouldCommit) {

    if (shouldCommit) {

        commitTransactions();

        commitTransactions();

    }

    }





    if (!needsTraversal) {

        // Invoke empty transaction callbacks early.

        mTransactionCallbackInvoker.sendCallbacks(false /* onCommitOnly */);

    } else {

        // Invoke OnCommit callbacks.

        // Invoke OnCommit callbacks.

    mTransactionCallbackInvoker.sendCallbacks();

        mTransactionCallbackInvoker.sendCallbacks(true /* onCommitOnly */);

    }





    if (transactionFlushNeeded()) {

    if (transactionFlushNeeded()) {

        setTransactionFlags(eTransactionFlushNeeded);

        setTransactionFlags(eTransactionFlushNeeded);
@@ -2328,7 +2334,7 @@ void SurfaceFlinger::postComposition() { 
    mVisibleRegionsWereDirtyThisFrame = false;

    mVisibleRegionsWereDirtyThisFrame = false;





    mTransactionCallbackInvoker.addPresentFence(mPreviousPresentFences[0].fence);

    mTransactionCallbackInvoker.addPresentFence(mPreviousPresentFences[0].fence);

    mTransactionCallbackInvoker.sendCallbacks();

    mTransactionCallbackInvoker.sendCallbacks(false /* onCommitOnly */);

    mTransactionCallbackInvoker.clearCompletedTransactions();

    mTransactionCallbackInvoker.clearCompletedTransactions();





    if (display && display->isInternal() && display->getPowerMode() == hal::PowerMode::ON &&

    if (display && display->isInternal() && display->getPowerMode() == hal::PowerMode::ON &&
@@ -2941,7 +2947,6 @@ void SurfaceFlinger::commitTransactionsLocked(uint32_t transactionFlags) { 
        processDisplayChangesLocked();

        processDisplayChangesLocked();

        processDisplayHotplugEventsLocked();

        processDisplayHotplugEventsLocked();

    }

    }

    mForceTraversal = false;

    mForceTransactionDisplayChange = displayTransactionNeeded;

    mForceTransactionDisplayChange = displayTransactionNeeded;





    if (mSomeChildrenChanged) {

    if (mSomeChildrenChanged) {
@@ -3415,11 +3420,8 @@ uint32_t SurfaceFlinger::setTransactionFlags(uint32_t mask, TransactionSchedule 
    return old;

    return old;

}

}





void SurfaceFlinger::setTraversalNeeded() {

bool SurfaceFlinger::flushTransactionQueues() {

    mForceTraversal = true;

    bool needsTraversal = false;

}



void SurfaceFlinger::flushTransactionQueues() {

    // to prevent onHandleDestroyed from being called while the lock is held,

    // to prevent onHandleDestroyed from being called while the lock is held,

    // we must keep a copy of the transactions (specifically the composer

    // we must keep a copy of the transactions (specifically the composer

    // states) around outside the scope of the lock

    // states) around outside the scope of the lock
@@ -3488,19 +3490,23 @@ void SurfaceFlinger::flushTransactionQueues() { 




        // Now apply all transactions.

        // Now apply all transactions.

        for (const auto& transaction : transactions) {

        for (const auto& transaction : transactions) {

            needsTraversal |=

                    applyTransactionState(transaction.frameTimelineInfo, transaction.states,

                    applyTransactionState(transaction.frameTimelineInfo, transaction.states,

                                          transaction.displays, transaction.flags,

                                          transaction.displays, transaction.flags,

                                  transaction.inputWindowCommands, transaction.desiredPresentTime,

                                          transaction.inputWindowCommands,

                                          transaction.desiredPresentTime,

                                          transaction.isAutoTimestamp, transaction.buffer,

                                          transaction.isAutoTimestamp, transaction.buffer,

                                          transaction.postTime, transaction.permissions,

                                          transaction.postTime, transaction.permissions,

                                  transaction.hasListenerCallbacks, transaction.listenerCallbacks,

                                          transaction.hasListenerCallbacks,

                                  transaction.originPid, transaction.originUid, transaction.id);

                                          transaction.listenerCallbacks, transaction.originPid,

                                          transaction.originUid, transaction.id);

            if (transaction.transactionCommittedSignal) {

            if (transaction.transactionCommittedSignal) {

                mTransactionCommittedSignals.emplace_back(

                mTransactionCommittedSignals.emplace_back(

                        std::move(transaction.transactionCommittedSignal));

                        std::move(transaction.transactionCommittedSignal));

            }

            }

        }

        }

    }

    } // unlock mStateLock

    return needsTraversal;

}

}





bool SurfaceFlinger::transactionFlushNeeded() {

bool SurfaceFlinger::transactionFlushNeeded() {
@@ -3706,7 +3712,7 @@ status_t SurfaceFlinger::setTransactionState( 
    return NO_ERROR;

    return NO_ERROR;

}

}





void SurfaceFlinger::applyTransactionState(const FrameTimelineInfo& frameTimelineInfo,

bool SurfaceFlinger::applyTransactionState(const FrameTimelineInfo& frameTimelineInfo,

                                           const Vector<ComposerState>& states,

                                           const Vector<ComposerState>& states,

                                           const Vector<DisplayState>& displays, uint32_t flags,

                                           const Vector<DisplayState>& displays, uint32_t flags,

                                           const InputWindowCommands& inputWindowCommands,

                                           const InputWindowCommands& inputWindowCommands,
@@ -3728,12 +3734,10 @@ void SurfaceFlinger::applyTransactionState(const FrameTimelineInfo& frameTimelin 
        mTransactionCallbackInvoker.addEmptyTransaction(listener);

        mTransactionCallbackInvoker.addEmptyTransaction(listener);

    }

    }





    std::unordered_set<ListenerCallbacks, ListenerCallbacksHash> listenerCallbacksWithSurfaces;

    uint32_t clientStateFlags = 0;

    uint32_t clientStateFlags = 0;

    for (const ComposerState& state : states) {

    for (const ComposerState& state : states) {

        clientStateFlags |=

        clientStateFlags |= setClientStateLocked(frameTimelineInfo, state, desiredPresentTime,

                setClientStateLocked(frameTimelineInfo, state, desiredPresentTime, isAutoTimestamp,

                                                 isAutoTimestamp, postTime, permissions);

                                     postTime, permissions, listenerCallbacksWithSurfaces);

        if ((flags & eAnimation) && state.state.surface) {

        if ((flags & eAnimation) && state.state.surface) {

            if (const auto layer = fromHandle(state.state.surface).promote(); layer) {

            if (const auto layer = fromHandle(state.state.surface).promote(); layer) {

                mScheduler->recordLayerHistory(layer.get(),

                mScheduler->recordLayerHistory(layer.get(),
@@ -3743,10 +3747,6 @@ void SurfaceFlinger::applyTransactionState(const FrameTimelineInfo& frameTimelin 
        }

        }

    }

    }





    // If the state doesn't require a traversal and there are callbacks, send them now

    if (!(clientStateFlags & eTraversalNeeded) && hasListenerCallbacks) {

        mTransactionCallbackInvoker.sendCallbacks();

    }

    transactionFlags |= clientStateFlags;

    transactionFlags |= clientStateFlags;





    if (permissions & Permission::ACCESS_SURFACE_FLINGER) {

    if (permissions & Permission::ACCESS_SURFACE_FLINGER) {
@@ -3768,6 +3768,7 @@ void SurfaceFlinger::applyTransactionState(const FrameTimelineInfo& frameTimelin 
        transactionFlags = eTransactionNeeded;

        transactionFlags = eTransactionNeeded;

    }

    }





    bool needsTraversal = false;

    if (transactionFlags) {

    if (transactionFlags) {

        if (mInterceptor->isEnabled()) {

        if (mInterceptor->isEnabled()) {

            mInterceptor->saveTransaction(states, mCurrentState.displays, displays, flags,

            mInterceptor->saveTransaction(states, mCurrentState.displays, displays, flags,
@@ -3778,7 +3779,7 @@ void SurfaceFlinger::applyTransactionState(const FrameTimelineInfo& frameTimelin 
        // so we don't have to wake up again next frame to preform an unnecessary traversal.

        // so we don't have to wake up again next frame to preform an unnecessary traversal.

        if (transactionFlags & eTraversalNeeded) {

        if (transactionFlags & eTraversalNeeded) {

            transactionFlags = transactionFlags & (~eTraversalNeeded);

            transactionFlags = transactionFlags & (~eTraversalNeeded);

            mForceTraversal = true;

            needsTraversal = true;

        }

        }

        if (transactionFlags) {

        if (transactionFlags) {

            setTransactionFlags(transactionFlags);

            setTransactionFlags(transactionFlags);
@@ -3788,6 +3789,8 @@ void SurfaceFlinger::applyTransactionState(const FrameTimelineInfo& frameTimelin 
            mAnimTransactionPending = true;

            mAnimTransactionPending = true;

        }

        }

    }

    }



    return needsTraversal;

}

}





uint32_t SurfaceFlinger::setDisplayStateLocked(const DisplayState& s) {

uint32_t SurfaceFlinger::setDisplayStateLocked(const DisplayState& s) {
@@ -3856,10 +3859,10 @@ bool SurfaceFlinger::callingThreadHasUnscopedSurfaceFlingerAccess(bool usePermis 
    return true;

    return true;

}

}





uint32_t SurfaceFlinger::setClientStateLocked(

uint32_t SurfaceFlinger::setClientStateLocked(const FrameTimelineInfo& frameTimelineInfo,

        const FrameTimelineInfo& frameTimelineInfo, const ComposerState& composerState,

                                              const ComposerState& composerState,

        int64_t desiredPresentTime, bool isAutoTimestamp, int64_t postTime, uint32_t permissions,

                                              int64_t desiredPresentTime, bool isAutoTimestamp,

        std::unordered_set<ListenerCallbacks, ListenerCallbacksHash>& outListenerCallbacks) {

                                              int64_t postTime, uint32_t permissions) {

    const layer_state_t& s = composerState.state;

    const layer_state_t& s = composerState.state;

    const bool privileged = permissions & Permission::ACCESS_SURFACE_FLINGER;

    const bool privileged = permissions & Permission::ACCESS_SURFACE_FLINGER;
@@ -3873,13 +3876,11 @@ uint32_t SurfaceFlinger::setClientStateLocked( 
        ListenerCallbacks onCommitCallbacks = listener.filter(CallbackId::Type::ON_COMMIT);

        ListenerCallbacks onCommitCallbacks = listener.filter(CallbackId::Type::ON_COMMIT);

        if (!onCommitCallbacks.callbackIds.empty()) {

        if (!onCommitCallbacks.callbackIds.empty()) {

            filteredListeners.push_back(onCommitCallbacks);

            filteredListeners.push_back(onCommitCallbacks);

            outListenerCallbacks.insert(onCommitCallbacks);

        }

        }





        ListenerCallbacks onCompleteCallbacks = listener.filter(CallbackId::Type::ON_COMPLETE);

        ListenerCallbacks onCompleteCallbacks = listener.filter(CallbackId::Type::ON_COMPLETE);

        if (!onCompleteCallbacks.callbackIds.empty()) {

        if (!onCompleteCallbacks.callbackIds.empty()) {

            filteredListeners.push_back(onCompleteCallbacks);

            filteredListeners.push_back(onCompleteCallbacks);

            outListenerCallbacks.insert(onCompleteCallbacks);

        }

        }

    }

    }

services/surfaceflinger/SurfaceFlinger.h

+3 −6
Original line number Original line Diff line number Diff line
@@ -712,7 +712,7 @@ private: 
    /*

    /*

     * Transactions

     * Transactions

     */

     */

    void applyTransactionState(const FrameTimelineInfo& info, const Vector<ComposerState>& state,

    bool applyTransactionState(const FrameTimelineInfo& info, const Vector<ComposerState>& state,

                               const Vector<DisplayState>& displays, uint32_t flags,

                               const Vector<DisplayState>& displays, uint32_t flags,

                               const InputWindowCommands& inputWindowCommands,

                               const InputWindowCommands& inputWindowCommands,

                               const int64_t desiredPresentTime, bool isAutoTimestamp,

                               const int64_t desiredPresentTime, bool isAutoTimestamp,
@@ -722,15 +722,13 @@ private: 
                               int originPid, int originUid, uint64_t transactionId)

                               int originPid, int originUid, uint64_t transactionId)

            REQUIRES(mStateLock);

            REQUIRES(mStateLock);

    // flush pending transaction that was presented after desiredPresentTime.

    // flush pending transaction that was presented after desiredPresentTime.

    void flushTransactionQueues();

    bool flushTransactionQueues();

    // Returns true if there is at least one transaction that needs to be flushed

    // Returns true if there is at least one transaction that needs to be flushed

    bool transactionFlushNeeded();

    bool transactionFlushNeeded();





    uint32_t setClientStateLocked(const FrameTimelineInfo&, const ComposerState&,

    uint32_t setClientStateLocked(const FrameTimelineInfo&, const ComposerState&,

                                  int64_t desiredPresentTime, bool isAutoTimestamp,

                                  int64_t desiredPresentTime, bool isAutoTimestamp,

                                  int64_t postTime, uint32_t permissions,

                                  int64_t postTime, uint32_t permissions) REQUIRES(mStateLock);

                                  std::unordered_set<ListenerCallbacks, ListenerCallbacksHash>&)

            REQUIRES(mStateLock);





    uint32_t getTransactionFlags() const;

    uint32_t getTransactionFlags() const;
@@ -1115,7 +1113,6 @@ private: 
    std::vector<std::shared_ptr<CountDownLatch>> mTransactionCommittedSignals;

    std::vector<std::shared_ptr<CountDownLatch>> mTransactionCommittedSignals;

    bool mAnimTransactionPending = false;

    bool mAnimTransactionPending = false;

    SortedVector<sp<Layer>> mLayersPendingRemoval;

    SortedVector<sp<Layer>> mLayersPendingRemoval;

    bool mForceTraversal = false;





    // global color transform states

    // global color transform states

    Daltonizer mDaltonizer;

    Daltonizer mDaltonizer;

services/surfaceflinger/TransactionCallbackInvoker.cpp

+8 −3
Original line number Original line Diff line number Diff line
@@ -121,7 +121,7 @@ status_t TransactionCallbackInvoker::registerUnpresentedCallbackHandle( 
    return addCallbackHandle(handle, std::vector<JankData>());

    return addCallbackHandle(handle, std::vector<JankData>());

}

}





status_t TransactionCallbackInvoker::findTransactionStats(

status_t TransactionCallbackInvoker::findOrCreateTransactionStats(

        const sp<IBinder>& listener, const std::vector<CallbackId>& callbackIds,

        const sp<IBinder>& listener, const std::vector<CallbackId>& callbackIds,

        TransactionStats** outTransactionStats) {

        TransactionStats** outTransactionStats) {

    auto& transactionStatsDeque = mCompletedTransactions[listener];

    auto& transactionStatsDeque = mCompletedTransactions[listener];
@@ -143,7 +143,8 @@ status_t TransactionCallbackInvoker::addCallbackHandle(const sp<CallbackHandle>& 
    // If we can't find the transaction stats something has gone wrong. The client should call

    // If we can't find the transaction stats something has gone wrong. The client should call

    // startRegistration before trying to add a callback handle.

    // startRegistration before trying to add a callback handle.

    TransactionStats* transactionStats;

    TransactionStats* transactionStats;

    status_t err = findTransactionStats(handle->listener, handle->callbackIds, &transactionStats);

    status_t err =

            findOrCreateTransactionStats(handle->listener, handle->callbackIds, &transactionStats);

    if (err != NO_ERROR) {

    if (err != NO_ERROR) {

        return err;

        return err;

    }

    }
@@ -204,7 +205,7 @@ void TransactionCallbackInvoker::addPresentFence(const sp<Fence>& presentFence) 
    mPresentFence = presentFence;

    mPresentFence = presentFence;

}

}





void TransactionCallbackInvoker::sendCallbacks() {

void TransactionCallbackInvoker::sendCallbacks(bool onCommitOnly) {

    // For each listener

    // For each listener

    auto completedTransactionsItr = mCompletedTransactions.begin();

    auto completedTransactionsItr = mCompletedTransactions.begin();

    while (completedTransactionsItr != mCompletedTransactions.end()) {

    while (completedTransactionsItr != mCompletedTransactions.end()) {
@@ -216,6 +217,10 @@ void TransactionCallbackInvoker::sendCallbacks() { 
        auto transactionStatsItr = transactionStatsDeque.begin();

        auto transactionStatsItr = transactionStatsDeque.begin();

        while (transactionStatsItr != transactionStatsDeque.end()) {

        while (transactionStatsItr != transactionStatsDeque.end()) {

            auto& transactionStats = *transactionStatsItr;

            auto& transactionStats = *transactionStatsItr;

            if (onCommitOnly && !containsOnCommitCallbacks(transactionStats.callbackIds)) {

                transactionStatsItr++;

                continue;

            }





            // If the transaction has been latched

            // If the transaction has been latched

            if (transactionStats.latchTime >= 0 &&

            if (transactionStats.latchTime >= 0 &&

services/surfaceflinger/TransactionCallbackInvoker.h

+4 −6
Original line number Original line Diff line number Diff line
@@ -76,7 +76,7 @@ public: 




    void addPresentFence(const sp<Fence>& presentFence);

    void addPresentFence(const sp<Fence>& presentFence);





    void sendCallbacks();

    void sendCallbacks(bool onCommitOnly);

    void clearCompletedTransactions() {

    void clearCompletedTransactions() {

        mCompletedTransactions.clear();

        mCompletedTransactions.clear();

    }

    }
@@ -86,12 +86,10 @@ public: 








private:

private:

    status_t findTransactionStats(const sp<IBinder>& listener,

    status_t findOrCreateTransactionStats(const sp<IBinder>& listener,

                                          const std::vector<CallbackId>& callbackIds,

                                          const std::vector<CallbackId>& callbackIds,

                                          TransactionStats** outTransactionStats);

                                          TransactionStats** outTransactionStats);









    std::unordered_map<sp<IBinder>, std::deque<TransactionStats>, IListenerHash>

    std::unordered_map<sp<IBinder>, std::deque<TransactionStats>, IListenerHash>

        mCompletedTransactions;

        mCompletedTransactions;

services/surfaceflinger/tests/LayerCallback_test.cpp

+56 −0
Original line number Original line Diff line number Diff line
@@ -1029,4 +1029,60 @@ TEST_F(LayerCallbackTest, ExpectedPresentTime) { 
    EXPECT_NO_FATAL_FAILURE(waitForCallback(callback, expected, true));

    EXPECT_NO_FATAL_FAILURE(waitForCallback(callback, expected, true));

}

}





// b202394221

TEST_F(LayerCallbackTest, EmptyBufferStateChanges) {

    sp<SurfaceControl> bufferLayer, emptyBufferLayer;

    ASSERT_NO_FATAL_FAILURE(bufferLayer = createBufferStateLayer());

    ASSERT_NO_FATAL_FAILURE(emptyBufferLayer = createBufferStateLayer());



    Transaction transaction;

    CallbackHelper callback;

    for (size_t i = 0; i < 10; i++) {

        int err = fillTransaction(transaction, &callback, bufferLayer);

        if (err) {

            GTEST_SUCCEED() << "test not supported";

            return;

        }



        ui::Size bufferSize = getBufferSize();



        TransactionUtils::setFrame(transaction, bufferLayer,

                                   Rect(0, 0, bufferSize.width, bufferSize.height),

                                   Rect(0, 0, 32, 32));

        transaction.setPosition(emptyBufferLayer, 1 + i, 2 + i);

        transaction.apply();



        ExpectedResult expected;

        expected.addSurface(ExpectedResult::Transaction::PRESENTED, bufferLayer,

                            ExpectedResult::Buffer::ACQUIRED,

                            (i == 0) ? ExpectedResult::PreviousBuffer::NOT_RELEASED

                                     : ExpectedResult::PreviousBuffer::RELEASED);

        expected.addSurface(ExpectedResult::Transaction::PRESENTED, emptyBufferLayer,

                            ExpectedResult::Buffer::NOT_ACQUIRED,

                            ExpectedResult::PreviousBuffer::NOT_RELEASED);



        EXPECT_NO_FATAL_FAILURE(waitForCallback(callback, expected));

    }

    ASSERT_NO_FATAL_FAILURE(callback.verifyFinalState());

}



// b202394221

TEST_F(LayerCallbackTest, DISABLED_NonBufferLayerStateChanges) {

    sp<SurfaceControl> layer;

    ASSERT_NO_FATAL_FAILURE(layer = createColorLayer("ColorLayer", Color::RED));



    Transaction transaction;

    CallbackHelper callback;

    int err = fillTransaction(transaction, &callback);

    if (err) {

        GTEST_SUCCEED() << "test not supported";

        return;

    }

    transaction.setPosition(layer, 1, 2);

    transaction.apply();



    ExpectedResult expected;

    EXPECT_NO_FATAL_FAILURE(waitForCallback(callback, expected, true));

}



} // namespace android
 
} // namespace android