Simplify TransactionCallbackInvoker

The old code was written around the old threading model, and had
to deal with callback registration coming from multiple binder
threads that could interleave with sendCallbacks. The old code
likewise had to deal with cases where a Transaction would apply
but not "latch" in this frame. Both of these cases are removed
by queued transactions, and so we can rework the model to be much
simpler. This is the first step towards another simplification
used to rework the frame queuing model.

Test: Existing tests pass
Bug: 201436596
Change-Id: I6983221ce66bb31c5da1122bde176ab4465556eb

4 files changed, 30 insertions, 212 deletions

diff --git a/services/surfaceflinger/BufferStateLayer.cpp b/services/surfaceflinger/BufferStateLayer.cpp
index 7466c0638c..5ed8f4d437 100644
--- a/services/surfaceflinger/BufferStateLayer.cpp
+++ b/services/surfaceflinger/BufferStateLayer.cpp
@@ -214,7 +214,7 @@ void BufferStateLayer::releasePendingBuffer(nsecs_t dequeueReadyTime) {
                 JankData(surfaceFrame->getToken(), surfaceFrame->getJankType().value()));
     }
 
-    mFlinger->getTransactionCallbackInvoker().finalizePendingCallbackHandles(
+    mFlinger->getTransactionCallbackInvoker().addCallbackHandles(
             mDrawingState.callbackHandles, jankData);
 
     mDrawingState.callbackHandles = {};
@@ -564,10 +564,6 @@ bool BufferStateLayer::setTransactionCompletedListeners(
             handle->acquireTime = mCallbackHandleAcquireTime;
             handle->frameNumber = mDrawingState.frameNumber;
 
-            // Notify the transaction completed thread that there is a pending latched callback
-            // handle
-            mFlinger->getTransactionCallbackInvoker().registerPendingCallbackHandle(handle);
-
             // Store so latched time and release fence can be set
             mDrawingState.callbackHandles.push_back(handle);
 
@@ -765,7 +761,7 @@ status_t BufferStateLayer::updateTexImage(bool& /*recomputeVisibleRegions*/, nse
 
     std::deque<sp<CallbackHandle>> remainingHandles;
     mFlinger->getTransactionCallbackInvoker()
-            .finalizeOnCommitCallbackHandles(mDrawingState.callbackHandles, remainingHandles);
+            .addOnCommitCallbackHandles(mDrawingState.callbackHandles, remainingHandles);
     mDrawingState.callbackHandles = remainingHandles;
 
     mDrawingStateModified = false;
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 88715e36aa..5d47ee8ade 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2158,10 +2158,11 @@ void SurfaceFlinger::onMessageRefresh() {
 
 bool SurfaceFlinger::handleMessageInvalidate() {
     ATRACE_CALL();
+        // Send on commit callbacks
+    mTransactionCallbackInvoker.sendCallbacks();
+
     bool refreshNeeded = handlePageFlip();
 
-    // Send on commit callbacks
-    mTransactionCallbackInvoker.sendCallbacks();
 
     if (mVisibleRegionsDirty) {
         computeLayerBounds();
@@ -2352,6 +2353,7 @@ void SurfaceFlinger::postComposition() {
 
     mTransactionCallbackInvoker.addPresentFence(mPreviousPresentFences[0].fence);
     mTransactionCallbackInvoker.sendCallbacks();
+    mTransactionCallbackInvoker.clearCompletedTransactions();
 
     if (display && display->isInternal() && display->getPowerMode() == hal::PowerMode::ON &&
         mPreviousPresentFences[0].fenceTime->isValid()) {
@@ -3735,8 +3737,7 @@ void SurfaceFlinger::applyTransactionState(const FrameTimelineInfo& frameTimelin
     // that listeners with SurfaceControls will start registration during setClientStateLocked
     // below.
     for (const auto& listener : listenerCallbacks) {
-        mTransactionCallbackInvoker.startRegistration(listener);
-        mTransactionCallbackInvoker.endRegistration(listener);
+        mTransactionCallbackInvoker.addEmptyTransaction(listener);
     }
 
     std::unordered_set<ListenerCallbacks, ListenerCallbacksHash> listenerCallbacksWithSurfaces;
@@ -3754,10 +3755,6 @@ void SurfaceFlinger::applyTransactionState(const FrameTimelineInfo& frameTimelin
         }
     }
 
-    for (const auto& listenerCallback : listenerCallbacksWithSurfaces) {
-        mTransactionCallbackInvoker.endRegistration(listenerCallback);
-    }
-
     // If the state doesn't require a traversal and there are callbacks, send them now
     if (!(clientStateFlags & eTraversalNeeded) && hasListenerCallbacks) {
         mTransactionCallbackInvoker.sendCallbacks();
@@ -3887,14 +3884,12 @@ uint32_t SurfaceFlinger::setClientStateLocked(
 
         ListenerCallbacks onCommitCallbacks = listener.filter(CallbackId::Type::ON_COMMIT);
         if (!onCommitCallbacks.callbackIds.empty()) {
-            mTransactionCallbackInvoker.startRegistration(onCommitCallbacks);
             filteredListeners.push_back(onCommitCallbacks);
             outListenerCallbacks.insert(onCommitCallbacks);
         }
 
         ListenerCallbacks onCompleteCallbacks = listener.filter(CallbackId::Type::ON_COMPLETE);
         if (!onCompleteCallbacks.callbackIds.empty()) {
-            mTransactionCallbackInvoker.startRegistration(onCompleteCallbacks);
             filteredListeners.push_back(onCompleteCallbacks);
             outListenerCallbacks.insert(onCompleteCallbacks);
         }
diff --git a/services/surfaceflinger/TransactionCallbackInvoker.cpp b/services/surfaceflinger/TransactionCallbackInvoker.cpp
index 6af69f0ef2..4b12a2640b 100644
--- a/services/surfaceflinger/TransactionCallbackInvoker.cpp
+++ b/services/surfaceflinger/TransactionCallbackInvoker.cpp
@@ -49,121 +49,25 @@ static bool containsOnCommitCallbacks(const std::vector<CallbackId>& callbacks)
     return !callbacks.empty() && callbacks.front().type == CallbackId::Type::ON_COMMIT;
 }
 
-TransactionCallbackInvoker::~TransactionCallbackInvoker() {
-    {
-        std::lock_guard lock(mMutex);
-        for (const auto& [listener, transactionStats] : mCompletedTransactions) {
-            listener->unlinkToDeath(mDeathRecipient);
-        }
-    }
-}
-
-status_t TransactionCallbackInvoker::startRegistration(const ListenerCallbacks& listenerCallbacks) {
-    std::lock_guard lock(mMutex);
-
-    auto [itr, inserted] = mRegisteringTransactions.insert(listenerCallbacks);
+void TransactionCallbackInvoker::addEmptyTransaction(const ListenerCallbacks& listenerCallbacks) {
     auto& [listener, callbackIds] = listenerCallbacks;
-
-    if (inserted) {
-        if (mCompletedTransactions.count(listener) == 0) {
-            status_t err = listener->linkToDeath(mDeathRecipient);
-            if (err != NO_ERROR) {
-                ALOGE("cannot add callback because linkToDeath failed, err: %d", err);
-                return err;
-            }
-        }
-        auto& transactionStatsDeque = mCompletedTransactions[listener];
-        transactionStatsDeque.emplace_back(callbackIds);
-    }
-
-    return NO_ERROR;
-}
-
-status_t TransactionCallbackInvoker::endRegistration(const ListenerCallbacks& listenerCallbacks) {
-    std::lock_guard lock(mMutex);
-
-    auto itr = mRegisteringTransactions.find(listenerCallbacks);
-    if (itr == mRegisteringTransactions.end()) {
-        ALOGE("cannot end a registration that does not exist");
-        return BAD_VALUE;
-    }
-
-    mRegisteringTransactions.erase(itr);
-
-    return NO_ERROR;
-}
-
-bool TransactionCallbackInvoker::isRegisteringTransaction(
-        const sp<IBinder>& transactionListener, const std::vector<CallbackId>& callbackIds) {
-    ListenerCallbacks listenerCallbacks(transactionListener, callbackIds);
-
-    auto itr = mRegisteringTransactions.find(listenerCallbacks);
-    return itr != mRegisteringTransactions.end();
-}
-
-status_t TransactionCallbackInvoker::registerPendingCallbackHandle(
-        const sp<CallbackHandle>& handle) {
-    std::lock_guard lock(mMutex);
-
-    // If we can't find the transaction stats something has gone wrong. The client should call
-    // startRegistration before trying to register a pending callback handle.
-    TransactionStats* transactionStats;
-    status_t err = findTransactionStats(handle->listener, handle->callbackIds, &transactionStats);
-    if (err != NO_ERROR) {
-        ALOGE("cannot find transaction stats");
-        return err;
-    }
-
-    mPendingTransactions[handle->listener][handle->callbackIds]++;
-    return NO_ERROR;
-}
-
-status_t TransactionCallbackInvoker::finalizeCallbackHandle(const sp<CallbackHandle>& handle,
-                                                            const std::vector<JankData>& jankData) {
-    auto listener = mPendingTransactions.find(handle->listener);
-    if (listener != mPendingTransactions.end()) {
-        auto& pendingCallbacks = listener->second;
-        auto pendingCallback = pendingCallbacks.find(handle->callbackIds);
-
-        if (pendingCallback != pendingCallbacks.end()) {
-            auto& pendingCount = pendingCallback->second;
-
-            // Decrease the pending count for this listener
-            if (--pendingCount == 0) {
-                pendingCallbacks.erase(pendingCallback);
-            }
-        } else {
-            ALOGW("there are more latched callbacks than there were registered callbacks");
-        }
-        if (listener->second.size() == 0) {
-            mPendingTransactions.erase(listener);
-        }
-    } else {
-        ALOGW("cannot find listener in mPendingTransactions");
-    }
-
-    status_t err = addCallbackHandle(handle, jankData);
-    if (err != NO_ERROR) {
-        ALOGE("could not add callback handle");
-        return err;
-    }
-    return NO_ERROR;
+    auto& transactionStatsDeque = mCompletedTransactions[listener];
+    transactionStatsDeque.emplace_back(callbackIds);
 }
 
-status_t TransactionCallbackInvoker::finalizeOnCommitCallbackHandles(
+status_t TransactionCallbackInvoker::addOnCommitCallbackHandles(
         const std::deque<sp<CallbackHandle>>& handles,
         std::deque<sp<CallbackHandle>>& outRemainingHandles) {
     if (handles.empty()) {
         return NO_ERROR;
     }
-    std::lock_guard lock(mMutex);
     const std::vector<JankData>& jankData = std::vector<JankData>();
     for (const auto& handle : handles) {
         if (!containsOnCommitCallbacks(handle->callbackIds)) {
             outRemainingHandles.push_back(handle);
             continue;
         }
-        status_t err = finalizeCallbackHandle(handle, jankData);
+        status_t err = addCallbackHandle(handle, jankData);
         if (err != NO_ERROR) {
             return err;
         }
@@ -172,14 +76,13 @@ status_t TransactionCallbackInvoker::finalizeOnCommitCallbackHandles(
     return NO_ERROR;
 }
 
-status_t TransactionCallbackInvoker::finalizePendingCallbackHandles(
+status_t TransactionCallbackInvoker::addCallbackHandles(
         const std::deque<sp<CallbackHandle>>& handles, const std::vector<JankData>& jankData) {
     if (handles.empty()) {
         return NO_ERROR;
     }
-    std::lock_guard lock(mMutex);
     for (const auto& handle : handles) {
-        status_t err = finalizeCallbackHandle(handle, jankData);
+        status_t err = addCallbackHandle(handle, jankData);
         if (err != NO_ERROR) {
             return err;
         }
@@ -190,8 +93,6 @@ status_t TransactionCallbackInvoker::finalizePendingCallbackHandles(
 
 status_t TransactionCallbackInvoker::registerUnpresentedCallbackHandle(
         const sp<CallbackHandle>& handle) {
-    std::lock_guard lock(mMutex);
-
     return addCallbackHandle(handle, std::vector<JankData>());
 }
 
@@ -208,9 +109,8 @@ status_t TransactionCallbackInvoker::findTransactionStats(
             return NO_ERROR;
         }
     }
-
-    ALOGE("could not find transaction stats");
-    return BAD_VALUE;
+    *outTransactionStats = &transactionStatsDeque.emplace_back(callbackIds);
+    return NO_ERROR;
 }
 
 status_t TransactionCallbackInvoker::addCallbackHandle(const sp<CallbackHandle>& handle,
@@ -244,13 +144,10 @@ status_t TransactionCallbackInvoker::addCallbackHandle(const sp<CallbackHandle>&
 }
 
 void TransactionCallbackInvoker::addPresentFence(const sp<Fence>& presentFence) {
-    std::lock_guard<std::mutex> lock(mMutex);
     mPresentFence = presentFence;
 }
 
 void TransactionCallbackInvoker::sendCallbacks() {
-    std::lock_guard lock(mMutex);
-
     // For each listener
     auto completedTransactionsItr = mCompletedTransactions.begin();
     while (completedTransactionsItr != mCompletedTransactions.end()) {
@@ -263,27 +160,9 @@ void TransactionCallbackInvoker::sendCallbacks() {
         while (transactionStatsItr != transactionStatsDeque.end()) {
             auto& transactionStats = *transactionStatsItr;
 
-            // If this transaction is still registering, it is not safe to send a callback
-            // because there could be surface controls that haven't been added to
-            // transaction stats or mPendingTransactions.
-            if (isRegisteringTransaction(listener, transactionStats.callbackIds)) {
-                break;
-            }
-
-            // If we are still waiting on the callback handles for this transaction, stop
-            // here because all transaction callbacks for the same listener must come in order
-            auto pendingTransactions = mPendingTransactions.find(listener);
-            if (pendingTransactions != mPendingTransactions.end() &&
-                pendingTransactions->second.count(transactionStats.callbackIds) != 0) {
-                break;
-            }
-
             // If the transaction has been latched
             if (transactionStats.latchTime >= 0 &&
                 !containsOnCommitCallbacks(transactionStats.callbackIds)) {
-                if (!mPresentFence) {
-                    break;
-                }
                 transactionStats.presentFence = mPresentFence;
             }
 
@@ -303,20 +182,9 @@ void TransactionCallbackInvoker::sendCallbacks() {
                 // we get pointers that compare unequal in the SF process.
                 interface_cast<ITransactionCompletedListener>(listenerStats.listener)
                         ->onTransactionCompleted(listenerStats);
-                if (transactionStatsDeque.empty()) {
-                    listener->unlinkToDeath(mDeathRecipient);
-                    completedTransactionsItr =
-                            mCompletedTransactions.erase(completedTransactionsItr);
-                } else {
-                    completedTransactionsItr++;
-                }
-            } else {
-                completedTransactionsItr =
-                        mCompletedTransactions.erase(completedTransactionsItr);
             }
-        } else {
-            completedTransactionsItr++;
         }
+        completedTransactionsItr++;
     }
 
     if (mPresentFence) {
diff --git a/services/surfaceflinger/TransactionCallbackInvoker.h b/services/surfaceflinger/TransactionCallbackInvoker.h
index 6f4d812ec5..71ca6e5360 100644
--- a/services/surfaceflinger/TransactionCallbackInvoker.h
+++ b/services/surfaceflinger/TransactionCallbackInvoker.h
@@ -56,79 +56,38 @@ public:
 
 class TransactionCallbackInvoker {
 public:
-    ~TransactionCallbackInvoker();
-
-    // Adds listener and callbackIds in case there are no SurfaceControls that are supposed
-    // to be included in the callback. This functions should be call before attempting to register
-    // any callback handles.
-    status_t startRegistration(const ListenerCallbacks& listenerCallbacks);
-    // Ends the registration. After this is called, no more CallbackHandles will be registered.
-    // It is safe to send a callback if the Transaction doesn't have any Pending callback handles.
-    status_t endRegistration(const ListenerCallbacks& listenerCallbacks);
-
-    // Informs the TransactionCallbackInvoker that there is a Transaction with a CallbackHandle
-    // that needs to be latched and presented this frame. This function should be called once the
-    // layer has received the CallbackHandle so the TransactionCallbackInvoker knows not to send
-    // a callback for that Listener/Transaction pair until that CallbackHandle has been latched and
-    // presented.
-    status_t registerPendingCallbackHandle(const sp<CallbackHandle>& handle);
-    // Notifies the TransactionCallbackInvoker that a pending CallbackHandle has been presented.
-    status_t finalizePendingCallbackHandles(const std::deque<sp<CallbackHandle>>& handles,
+    status_t addCallbackHandles(const std::deque<sp<CallbackHandle>>& handles,
                                             const std::vector<JankData>& jankData);
-    status_t finalizeOnCommitCallbackHandles(const std::deque<sp<CallbackHandle>>& handles,
+    status_t addOnCommitCallbackHandles(const std::deque<sp<CallbackHandle>>& handles,
                                              std::deque<sp<CallbackHandle>>& outRemainingHandles);
 
     // Adds the Transaction CallbackHandle from a layer that does not need to be relatched and
     // presented this frame.
     status_t registerUnpresentedCallbackHandle(const sp<CallbackHandle>& handle);
+    void addEmptyTransaction(const ListenerCallbacks& listenerCallbacks);
 
     void addPresentFence(const sp<Fence>& presentFence);
 
     void sendCallbacks();
+    void clearCompletedTransactions() {
+        mCompletedTransactions.clear();
+    }
 
-private:
+    status_t addCallbackHandle(const sp<CallbackHandle>& handle,
+                               const std::vector<JankData>& jankData);
 
-    bool isRegisteringTransaction(const sp<IBinder>& transactionListener,
-                                  const std::vector<CallbackId>& callbackIds) REQUIRES(mMutex);
 
+private:
     status_t findTransactionStats(const sp<IBinder>& listener,
                                   const std::vector<CallbackId>& callbackIds,
-                                  TransactionStats** outTransactionStats) REQUIRES(mMutex);
+                                  TransactionStats** outTransactionStats);
+
 
-    status_t addCallbackHandle(const sp<CallbackHandle>& handle,
-                               const std::vector<JankData>& jankData) REQUIRES(mMutex);
-
-    status_t finalizeCallbackHandle(const sp<CallbackHandle>& handle,
-                                    const std::vector<JankData>& jankData) REQUIRES(mMutex);
-
-    class CallbackDeathRecipient : public IBinder::DeathRecipient {
-    public:
-        // This function is a no-op. isBinderAlive needs a linked DeathRecipient to work.
-        // Death recipients needs a binderDied function.
-        //
-        // (isBinderAlive checks if BpBinder's mAlive is 0. mAlive is only set to 0 in sendObituary.
-        // sendObituary is only called if linkToDeath was called with a DeathRecipient.)
-        void binderDied(const wp<IBinder>& /*who*/) override {}
-    };
-    sp<CallbackDeathRecipient> mDeathRecipient =
-        new CallbackDeathRecipient();
-
-    std::mutex mMutex;
-    std::condition_variable_any mConditionVariable;
-
-    std::unordered_set<ListenerCallbacks, ListenerCallbacksHash> mRegisteringTransactions
-            GUARDED_BY(mMutex);
-
-    std::unordered_map<
-            sp<IBinder>,
-            std::unordered_map<std::vector<CallbackId>, uint32_t /*count*/, CallbackIdsHash>,
-            IListenerHash>
-            mPendingTransactions GUARDED_BY(mMutex);
 
     std::unordered_map<sp<IBinder>, std::deque<TransactionStats>, IListenerHash>
-            mCompletedTransactions GUARDED_BY(mMutex);
+        mCompletedTransactions;
 
-    sp<Fence> mPresentFence GUARDED_BY(mMutex);
+    sp<Fence> mPresentFence;
 };
 
 } // namespace android