Introduce ASurfaceTransaction_setOnCommit api

Introduce a new callback for SurfaceControl transactions that
fire after we commit a transaction in SurfaceFlinger. This
will help some clients pace when they should apply the next
transaction so it get applied on the next vsync. If they wait for
the existing transaction complete callback, there may not be
enough time between when the client applies the transaction
and surface flinger waking up and apply it on the new vsync.
This would mean the update would arrive a frame late.

This callback is guaranteed to fire before the transaction complete
callback. It includes all the stats as the transaction complete
callback with the exception of jank data, present fence
and the previous buffer release fence.

This callback piggybacks of the oncomplete callback implementation
by modifying the callback id to provide a callback type. The
callbacks are filtered in SurfaceFlinger to invoke them earlier. In
SurfaceComposerClient, they are filtered again to make sure the
callbacks are invoked in order, oncommit before oncomplete.

Bug: 185843251
Test: atest ASurfaceControlTest
Change-Id: I57e85d75214376935e366d3825a6f3f1a8a4e79b

11 files changed, 265 insertions, 68 deletions

diff --git a/include/android/surface_control.h b/include/android/surface_control.h
index c349024839..b7eafcd6cd 100644
--- a/include/android/surface_control.h
+++ b/include/android/surface_control.h
@@ -147,6 +147,28 @@ typedef struct ASurfaceTransactionStats ASurfaceTransactionStats;
 typedef void (*ASurfaceTransaction_OnComplete)(void* context, ASurfaceTransactionStats* stats)
                                                __INTRODUCED_IN(29);
 
+
+/**
+ * The ASurfaceTransaction_OnCommit callback is invoked when transaction is applied and the updates
+ * are ready to be presented. This callback will be invoked before the
+ * ASurfaceTransaction_OnComplete callback.
+ *
+ * \param context Optional context provided by the client that is passed into the callback.
+ *
+ * \param stats Opaque handle that can be passed to ASurfaceTransactionStats functions to query
+ * information about the transaction. The handle is only valid during the callback.
+ * Present and release fences are not available for this callback. Querying them using
+ * ASurfaceTransactionStats_getPresentFenceFd and ASurfaceTransactionStats_getPreviousReleaseFenceFd
+ * will result in failure.
+ *
+ * THREADING
+ * The transaction committed callback can be invoked on any thread.
+ *
+ * Available since API level 31.
+ */
+typedef void (*ASurfaceTransaction_OnCommit)(void* context, ASurfaceTransactionStats* stats)
+                                               __INTRODUCED_IN(31);
+
 /**
  * Returns the timestamp of when the frame was latched by the framework. Once a frame is
  * latched by the framework, it is presented at the following hardware vsync.
@@ -161,6 +183,8 @@ int64_t ASurfaceTransactionStats_getLatchTime(ASurfaceTransactionStats* surface_
  * The recipient of the callback takes ownership of the fence and is responsible for closing
  * it. If a device does not support present fences, a -1 will be returned.
  *
+ * This query is not valid for ASurfaceTransaction_OnCommit callback.
+ *
  * Available since API level 29.
  */
 int ASurfaceTransactionStats_getPresentFenceFd(ASurfaceTransactionStats* surface_transaction_stats)
@@ -218,6 +242,8 @@ int64_t ASurfaceTransactionStats_getAcquireTime(ASurfaceTransactionStats* surfac
  * The client must ensure that all pending refs on a buffer are released before attempting to reuse
  * this buffer, otherwise synchronization errors may occur.
  *
+ * This query is not valid for ASurfaceTransaction_OnCommit callback.
+ *
  * Available since API level 29.
  */
 int ASurfaceTransactionStats_getPreviousReleaseFenceFd(
@@ -236,6 +262,16 @@ void ASurfaceTransaction_setOnComplete(ASurfaceTransaction* transaction, void* c
                                        ASurfaceTransaction_OnComplete func) __INTRODUCED_IN(29);
 
 /**
+ * Sets the callback that will be invoked when the updates from this transaction are applied and are
+ * ready to be presented. This callback will be invoked before the ASurfaceTransaction_OnComplete
+ * callback.
+ *
+ * Available since API level 31.
+ */
+void ASurfaceTransaction_setOnCommit(ASurfaceTransaction* transaction, void* context,
+                                    ASurfaceTransaction_OnCommit func) __INTRODUCED_IN(31);
+
+/**
  * Reparents the \a surface_control from its old parent to the \a new_parent surface control.
  * Any children of the reparented \a surface_control will remain children of the \a surface_control.
  *
diff --git a/libs/gui/ISurfaceComposer.cpp b/libs/gui/ISurfaceComposer.cpp
index 454aa9eb7b..0e28966a9b 100644
--- a/libs/gui/ISurfaceComposer.cpp
+++ b/libs/gui/ISurfaceComposer.cpp
@@ -99,7 +99,7 @@ public:
         SAFE_PARCEL(data.writeVectorSize, listenerCallbacks);
         for (const auto& [listener, callbackIds] : listenerCallbacks) {
             SAFE_PARCEL(data.writeStrongBinder, listener);
-            SAFE_PARCEL(data.writeInt64Vector, callbackIds);
+            SAFE_PARCEL(data.writeParcelableVector, callbackIds);
         }
 
         SAFE_PARCEL(data.writeUint64, transactionId);
@@ -1246,7 +1246,7 @@ status_t BnSurfaceComposer::onTransact(
             for (int32_t i = 0; i < listenersSize; i++) {
                 SAFE_PARCEL(data.readStrongBinder, &tmpBinder);
                 std::vector<CallbackId> callbackIds;
-                SAFE_PARCEL(data.readInt64Vector, &callbackIds);
+                SAFE_PARCEL(data.readParcelableVector, &callbackIds);
                 listenerCallbacks.emplace_back(tmpBinder, callbackIds);
             }
 
diff --git a/libs/gui/ITransactionCompletedListener.cpp b/libs/gui/ITransactionCompletedListener.cpp
index b42793b8ac..f74f91e034 100644
--- a/libs/gui/ITransactionCompletedListener.cpp
+++ b/libs/gui/ITransactionCompletedListener.cpp
@@ -152,7 +152,7 @@ status_t SurfaceStats::readFromParcel(const Parcel* input) {
 }
 
 status_t TransactionStats::writeToParcel(Parcel* output) const {
-    status_t err = output->writeInt64Vector(callbackIds);
+    status_t err = output->writeParcelableVector(callbackIds);
     if (err != NO_ERROR) {
         return err;
     }
@@ -176,7 +176,7 @@ status_t TransactionStats::writeToParcel(Parcel* output) const {
 }
 
 status_t TransactionStats::readFromParcel(const Parcel* input) {
-    status_t err = input->readInt64Vector(&callbackIds);
+    status_t err = input->readParcelableVector(&callbackIds);
     if (err != NO_ERROR) {
         return err;
     }
@@ -227,8 +227,9 @@ status_t ListenerStats::readFromParcel(const Parcel* input) {
     return NO_ERROR;
 }
 
-ListenerStats ListenerStats::createEmpty(const sp<IBinder>& listener,
-                                         const std::unordered_set<CallbackId>& callbackIds) {
+ListenerStats ListenerStats::createEmpty(
+        const sp<IBinder>& listener,
+        const std::unordered_set<CallbackId, CallbackIdHash>& callbackIds) {
     ListenerStats listenerStats;
     listenerStats.listener = listener;
     listenerStats.transactionStats.emplace_back(callbackIds);
@@ -278,4 +279,28 @@ status_t BnTransactionCompletedListener::onTransact(uint32_t code, const Parcel&
     }
 }
 
+ListenerCallbacks ListenerCallbacks::filter(CallbackId::Type type) const {
+    std::vector<CallbackId> filteredCallbackIds;
+    for (const auto& callbackId : callbackIds) {
+        if (callbackId.type == type) {
+            filteredCallbackIds.push_back(callbackId);
+        }
+    }
+    return ListenerCallbacks(transactionCompletedListener, filteredCallbackIds);
+}
+
+status_t CallbackId::writeToParcel(Parcel* output) const {
+    SAFE_PARCEL(output->writeInt64, id);
+    SAFE_PARCEL(output->writeInt32, static_cast<int32_t>(type));
+    return NO_ERROR;
+}
+
+status_t CallbackId::readFromParcel(const Parcel* input) {
+    SAFE_PARCEL(input->readInt64, &id);
+    int32_t typeAsInt;
+    SAFE_PARCEL(input->readInt32, &typeAsInt);
+    type = static_cast<CallbackId::Type>(typeAsInt);
+    return NO_ERROR;
+}
+
 }; // namespace android
diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp
index 55ed7fe298..517b49e6b5 100644
--- a/libs/gui/LayerState.cpp
+++ b/libs/gui/LayerState.cpp
@@ -140,7 +140,7 @@ status_t layer_state_t::write(Parcel& output) const
 
     for (auto listener : listeners) {
         SAFE_PARCEL(output.writeStrongBinder, listener.transactionCompletedListener);
-        SAFE_PARCEL(output.writeInt64Vector, listener.callbackIds);
+        SAFE_PARCEL(output.writeParcelableVector, listener.callbackIds);
     }
     SAFE_PARCEL(output.writeFloat, shadowRadius);
     SAFE_PARCEL(output.writeInt32, frameRateSelectionPriority);
@@ -258,7 +258,7 @@ status_t layer_state_t::read(const Parcel& input)
         sp<IBinder> listener;
         std::vector<CallbackId> callbackIds;
         SAFE_PARCEL(input.readNullableStrongBinder, &listener);
-        SAFE_PARCEL(input.readInt64Vector, &callbackIds);
+        SAFE_PARCEL(input.readParcelableVector, &callbackIds);
         listeners.emplace_back(listener, callbackIds);
     }
     SAFE_PARCEL(input.readFloat, &shadowRadius);
diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp
index 6d198a105f..49a10300b8 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -139,7 +139,7 @@ JankDataListener::~JankDataListener() {
 // 0 is an invalid callback id
 TransactionCompletedListener::TransactionCompletedListener() : mCallbackIdCounter(1) {}
 
-CallbackId TransactionCompletedListener::getNextIdLocked() {
+int64_t TransactionCompletedListener::getNextIdLocked() {
     return mCallbackIdCounter++;
 }
 
@@ -163,13 +163,13 @@ void TransactionCompletedListener::startListeningLocked() {
 CallbackId TransactionCompletedListener::addCallbackFunction(
         const TransactionCompletedCallback& callbackFunction,
         const std::unordered_set<sp<SurfaceControl>, SurfaceComposerClient::SCHash>&
-                surfaceControls) {
+                surfaceControls,
+        CallbackId::Type callbackType) {
     std::lock_guard<std::mutex> lock(mMutex);
     startListeningLocked();
 
-    CallbackId callbackId = getNextIdLocked();
+    CallbackId callbackId(getNextIdLocked(), callbackType);
     mCallbacks[callbackId].callbackFunction = callbackFunction;
-
     auto& callbackSurfaceControls = mCallbacks[callbackId].surfaceControls;
 
     for (const auto& surfaceControl : surfaceControls) {
@@ -228,7 +228,7 @@ void TransactionCompletedListener::removeSurfaceStatsListener(void* context, voi
 
 void TransactionCompletedListener::addSurfaceControlToCallbacks(
         const sp<SurfaceControl>& surfaceControl,
-        const std::unordered_set<CallbackId>& callbackIds) {
+        const std::unordered_set<CallbackId, CallbackIdHash>& callbackIds) {
     std::lock_guard<std::mutex> lock(mMutex);
 
     for (auto callbackId : callbackIds) {
@@ -240,7 +240,7 @@ void TransactionCompletedListener::addSurfaceControlToCallbacks(
 }
 
 void TransactionCompletedListener::onTransactionCompleted(ListenerStats listenerStats) {
-    std::unordered_map<CallbackId, CallbackTranslation> callbacksMap;
+    std::unordered_map<CallbackId, CallbackTranslation, CallbackIdHash> callbacksMap;
     std::multimap<sp<IBinder>, sp<JankDataListener>> jankListenersMap;
     std::multimap<sp<IBinder>, SurfaceStatsCallbackEntry> surfaceListeners;
     {
@@ -267,7 +267,36 @@ void TransactionCompletedListener::onTransactionCompleted(ListenerStats listener
         }
     }
     for (const auto& transactionStats : listenerStats.transactionStats) {
+        // handle on commit callbacks
+        for (auto callbackId : transactionStats.callbackIds) {
+            if (callbackId.type != CallbackId::Type::ON_COMMIT) {
+                continue;
+            }
+            auto& [callbackFunction, callbackSurfaceControls] = callbacksMap[callbackId];
+            if (!callbackFunction) {
+                ALOGE("cannot call null callback function, skipping");
+                continue;
+            }
+            std::vector<SurfaceControlStats> surfaceControlStats;
+            for (const auto& surfaceStats : transactionStats.surfaceStats) {
+                surfaceControlStats
+                        .emplace_back(callbacksMap[callbackId]
+                                              .surfaceControls[surfaceStats.surfaceControl],
+                                      transactionStats.latchTime, surfaceStats.acquireTime,
+                                      transactionStats.presentFence,
+                                      surfaceStats.previousReleaseFence, surfaceStats.transformHint,
+                                      surfaceStats.eventStats);
+            }
+
+            callbackFunction(transactionStats.latchTime, transactionStats.presentFence,
+                             surfaceControlStats);
+        }
+
+        // handle on complete callbacks
         for (auto callbackId : transactionStats.callbackIds) {
+            if (callbackId.type != CallbackId::Type::ON_COMPLETE) {
+                continue;
+            }
             auto& [callbackFunction, callbackSurfaceControls] = callbacksMap[callbackId];
             if (!callbackFunction) {
                 ALOGE("cannot call null callback function, skipping");
@@ -542,7 +571,9 @@ status_t SurfaceComposerClient::Transaction::readFromParcel(const Parcel* parcel
             return BAD_VALUE;
         }
         for (size_t j = 0; j < numCallbackIds; j++) {
-            listenerCallbacks[listener].callbackIds.insert(parcel->readInt64());
+            CallbackId id;
+            parcel->readParcelable(&id);
+            listenerCallbacks[listener].callbackIds.insert(id);
         }
         size_t numSurfaces = parcel->readUint32();
         if (numSurfaces > parcel->dataSize()) {
@@ -628,7 +659,7 @@ status_t SurfaceComposerClient::Transaction::writeToParcel(Parcel* parcel) const
         parcel->writeStrongBinder(ITransactionCompletedListener::asBinder(listener));
         parcel->writeUint32(static_cast<uint32_t>(callbackInfo.callbackIds.size()));
         for (auto callbackId : callbackInfo.callbackIds) {
-            parcel->writeInt64(callbackId);
+            parcel->writeParcelable(callbackId);
         }
         parcel->writeUint32(static_cast<uint32_t>(callbackInfo.surfaceControls.size()));
         for (auto surfaceControl : callbackInfo.surfaceControls) {
@@ -1389,9 +1420,9 @@ SurfaceComposerClient::Transaction::setFrameRateSelectionPriority(const sp<Surfa
     return *this;
 }
 
-SurfaceComposerClient::Transaction&
-SurfaceComposerClient::Transaction::addTransactionCompletedCallback(
-        TransactionCompletedCallbackTakesContext callback, void* callbackContext) {
+SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::addTransactionCallback(
+        TransactionCompletedCallbackTakesContext callback, void* callbackContext,
+        CallbackId::Type callbackType) {
     auto listener = TransactionCompletedListener::getInstance();
 
     auto callbackWithContext = std::bind(callback, callbackContext, std::placeholders::_1,
@@ -1399,13 +1430,26 @@ SurfaceComposerClient::Transaction::addTransactionCompletedCallback(
     const auto& surfaceControls =
             mListenerCallbacks[TransactionCompletedListener::getIInstance()].surfaceControls;
 
-    CallbackId callbackId = listener->addCallbackFunction(callbackWithContext, surfaceControls);
+    CallbackId callbackId =
+            listener->addCallbackFunction(callbackWithContext, surfaceControls, callbackType);
 
     mListenerCallbacks[TransactionCompletedListener::getIInstance()].callbackIds.emplace(
             callbackId);
     return *this;
 }
 
+SurfaceComposerClient::Transaction&
+SurfaceComposerClient::Transaction::addTransactionCompletedCallback(
+        TransactionCompletedCallbackTakesContext callback, void* callbackContext) {
+    return addTransactionCallback(callback, callbackContext, CallbackId::Type::ON_COMPLETE);
+}
+
+SurfaceComposerClient::Transaction&
+SurfaceComposerClient::Transaction::addTransactionCommittedCallback(
+        TransactionCompletedCallbackTakesContext callback, void* callbackContext) {
+    return addTransactionCallback(callback, callbackContext, CallbackId::Type::ON_COMMIT);
+}
+
 SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::notifyProducerDisconnect(
         const sp<SurfaceControl>& sc) {
     layer_state_t* s = getLayerState(sc);
diff --git a/libs/gui/include/gui/ITransactionCompletedListener.h b/libs/gui/include/gui/ITransactionCompletedListener.h
index 098760e89d..2d71194f70 100644
--- a/libs/gui/include/gui/ITransactionCompletedListener.h
+++ b/libs/gui/include/gui/ITransactionCompletedListener.h
@@ -36,7 +36,22 @@ namespace android {
 class ITransactionCompletedListener;
 class ListenerCallbacks;
 
-using CallbackId = int64_t;
+class CallbackId : public Parcelable {
+public:
+    int64_t id;
+    enum class Type : int32_t { ON_COMPLETE, ON_COMMIT } type;
+
+    CallbackId() {}
+    CallbackId(int64_t id, Type type) : id(id), type(type) {}
+    status_t writeToParcel(Parcel* output) const override;
+    status_t readFromParcel(const Parcel* input) override;
+
+    bool operator==(const CallbackId& rhs) const { return id == rhs.id && type == rhs.type; }
+};
+
+struct CallbackIdHash {
+    std::size_t operator()(const CallbackId& key) const { return std::hash<int64_t>()(key.id); }
+};
 
 class FrameEventHistoryStats : public Parcelable {
 public:
@@ -112,7 +127,7 @@ public:
 
     TransactionStats() = default;
     TransactionStats(const std::vector<CallbackId>& ids) : callbackIds(ids) {}
-    TransactionStats(const std::unordered_set<CallbackId>& ids)
+    TransactionStats(const std::unordered_set<CallbackId, CallbackIdHash>& ids)
           : callbackIds(ids.begin(), ids.end()) {}
     TransactionStats(const std::vector<CallbackId>& ids, nsecs_t latch, const sp<Fence>& present,
                      const std::vector<SurfaceStats>& surfaces)
@@ -129,8 +144,9 @@ public:
     status_t writeToParcel(Parcel* output) const override;
     status_t readFromParcel(const Parcel* input) override;
 
-    static ListenerStats createEmpty(const sp<IBinder>& listener,
-                                     const std::unordered_set<CallbackId>& callbackIds);
+    static ListenerStats createEmpty(
+            const sp<IBinder>& listener,
+            const std::unordered_set<CallbackId, CallbackIdHash>& callbackIds);
 
     sp<IBinder> listener;
     std::vector<TransactionStats> transactionStats;
@@ -156,7 +172,8 @@ public:
 
 class ListenerCallbacks {
 public:
-    ListenerCallbacks(const sp<IBinder>& listener, const std::unordered_set<CallbackId>& callbacks)
+    ListenerCallbacks(const sp<IBinder>& listener,
+                      const std::unordered_set<CallbackId, CallbackIdHash>& callbacks)
           : transactionCompletedListener(listener),
             callbackIds(callbacks.begin(), callbacks.end()) {}
 
@@ -170,9 +187,12 @@ public:
         if (callbackIds.empty()) {
             return rhs.callbackIds.empty();
         }
-        return callbackIds.front() == rhs.callbackIds.front();
+        return callbackIds.front().id == rhs.callbackIds.front().id;
     }
 
+    // Returns a new ListenerCallbacks filtered by type
+    ListenerCallbacks filter(CallbackId::Type type) const;
+
     sp<IBinder> transactionCompletedListener;
     std::vector<CallbackId> callbackIds;
 };
@@ -191,7 +211,7 @@ struct CallbackIdsHash {
     // same members. It is sufficient to just check the first CallbackId in the vectors. If
     // they match, they are the same. If they do not match, they are not the same.
     std::size_t operator()(const std::vector<CallbackId>& callbackIds) const {
-        return std::hash<CallbackId>{}((callbackIds.empty()) ? 0 : callbackIds.front());
+        return std::hash<int64_t>{}((callbackIds.empty()) ? 0 : callbackIds.front().id);
     }
 };
 
diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h
index fe6a30aa42..171c05e0c5 100644
--- a/libs/gui/include/gui/SurfaceComposerClient.h
+++ b/libs/gui/include/gui/SurfaceComposerClient.h
@@ -336,7 +336,7 @@ public:
     struct CallbackInfo {
         // All the callbacks that have been requested for a TransactionCompletedListener in the
         // Transaction
-        std::unordered_set<CallbackId> callbackIds;
+        std::unordered_set<CallbackId, CallbackIdHash> callbackIds;
         // All the SurfaceControls that have been modified in this TransactionCompletedListener's
         // process that require a callback if there is one or more callbackIds set.
         std::unordered_set<sp<SurfaceControl>, SCHash> surfaceControls;
@@ -484,9 +484,15 @@ public:
         // Sets information about the priority of the frame.
         Transaction& setFrameRateSelectionPriority(const sp<SurfaceControl>& sc, int32_t priority);
 
+        Transaction& addTransactionCallback(TransactionCompletedCallbackTakesContext callback,
+                                            void* callbackContext, CallbackId::Type callbackType);
+
         Transaction& addTransactionCompletedCallback(
                 TransactionCompletedCallbackTakesContext callback, void* callbackContext);
 
+        Transaction& addTransactionCommittedCallback(
+                TransactionCompletedCallbackTakesContext callback, void* callbackContext);
+
         // ONLY FOR BLAST ADAPTER
         Transaction& notifyProducerDisconnect(const sp<SurfaceControl>& sc);
         // Set the framenumber generated by the graphics producer to mimic BufferQueue behaviour.
@@ -619,14 +625,13 @@ public:
 class TransactionCompletedListener : public BnTransactionCompletedListener {
     TransactionCompletedListener();
 
-    CallbackId getNextIdLocked() REQUIRES(mMutex);
+    int64_t getNextIdLocked() REQUIRES(mMutex);
 
     std::mutex mMutex;
 
     bool mListening GUARDED_BY(mMutex) = false;
 
-    CallbackId mCallbackIdCounter GUARDED_BY(mMutex) = 1;
-
+    int64_t mCallbackIdCounter GUARDED_BY(mMutex) = 1;
     struct CallbackTranslation {
         TransactionCompletedCallback callbackFunction;
         std::unordered_map<sp<IBinder>, sp<SurfaceControl>, SurfaceComposerClient::IBinderHash>
@@ -644,7 +649,8 @@ class TransactionCompletedListener : public BnTransactionCompletedListener {
         SurfaceStatsCallback callback;
     };
 
-    std::unordered_map<CallbackId, CallbackTranslation> mCallbacks GUARDED_BY(mMutex);
+    std::unordered_map<CallbackId, CallbackTranslation, CallbackIdHash> mCallbacks
+            GUARDED_BY(mMutex);
     std::multimap<sp<IBinder>, sp<JankDataListener>> mJankListeners GUARDED_BY(mMutex);
     std::unordered_map<uint64_t /* graphicsBufferId */, ReleaseBufferCallback>
             mReleaseBufferCallbacks GUARDED_BY(mMutex);
@@ -660,10 +666,12 @@ public:
     CallbackId addCallbackFunction(
             const TransactionCompletedCallback& callbackFunction,
             const std::unordered_set<sp<SurfaceControl>, SurfaceComposerClient::SCHash>&
-                    surfaceControls);
+                    surfaceControls,
+            CallbackId::Type callbackType);
 
-    void addSurfaceControlToCallbacks(const sp<SurfaceControl>& surfaceControl,
-                                      const std::unordered_set<CallbackId>& callbackIds);
+    void addSurfaceControlToCallbacks(
+            const sp<SurfaceControl>& surfaceControl,
+            const std::unordered_set<CallbackId, CallbackIdHash>& callbackIds);
 
     /*
      * Adds a jank listener to be informed about SurfaceFlinger's jank classification for a specific
diff --git a/services/surfaceflinger/BufferStateLayer.cpp b/services/surfaceflinger/BufferStateLayer.cpp
index ac2edbe717..627ff53abf 100644
--- a/services/surfaceflinger/BufferStateLayer.cpp
+++ b/services/surfaceflinger/BufferStateLayer.cpp
@@ -674,6 +674,11 @@ status_t BufferStateLayer::updateTexImage(bool& /*recomputeVisibleRegions*/, nse
                                           latchTime);
     }
 
+    std::deque<sp<CallbackHandle>> remainingHandles;
+    mFlinger->getTransactionCallbackInvoker()
+            .finalizeOnCommitCallbackHandles(mDrawingState.callbackHandles, remainingHandles);
+    mDrawingState.callbackHandles = remainingHandles;
+
     mCurrentStateModified = false;
 
     return NO_ERROR;
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 5f5987f8a3..8e2d5e5253 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2029,6 +2029,9 @@ bool SurfaceFlinger::handleMessageInvalidate() {
     ATRACE_CALL();
     bool refreshNeeded = handlePageFlip();
 
+    // Send on commit callbacks
+    mTransactionCallbackInvoker.sendCallbacks();
+
     if (mVisibleRegionsDirty) {
         computeLayerBounds();
     }
@@ -3789,14 +3792,30 @@ bool SurfaceFlinger::callingThreadHasUnscopedSurfaceFlingerAccess(bool usePermis
 uint32_t SurfaceFlinger::setClientStateLocked(
         const FrameTimelineInfo& frameTimelineInfo, const ComposerState& composerState,
         int64_t desiredPresentTime, bool isAutoTimestamp, int64_t postTime, uint32_t permissions,
-        std::unordered_set<ListenerCallbacks, ListenerCallbacksHash>& listenerCallbacks) {
+        std::unordered_set<ListenerCallbacks, ListenerCallbacksHash>& outListenerCallbacks) {
     const layer_state_t& s = composerState.state;
     const bool privileged = permissions & Permission::ACCESS_SURFACE_FLINGER;
+
+    std::vector<ListenerCallbacks> filteredListeners;
     for (auto& listener : s.listeners) {
+        // Starts a registration but separates the callback ids according to callback type. This
+        // allows the callback invoker to send on latch callbacks earlier.
         // note that startRegistration will not re-register if the listener has
         // already be registered for a prior surface control
-        mTransactionCallbackInvoker.startRegistration(listener);
-        listenerCallbacks.insert(listener);
+
+        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);
+        }
     }
 
     sp<Layer> layer = nullptr;
@@ -4049,8 +4068,8 @@ uint32_t SurfaceFlinger::setClientStateLocked(
         }
     }
     std::vector<sp<CallbackHandle>> callbackHandles;
-    if ((what & layer_state_t::eHasListenerCallbacksChanged) && (!s.listeners.empty())) {
-        for (auto& [listener, callbackIds] : s.listeners) {
+    if ((what & layer_state_t::eHasListenerCallbacksChanged) && (!filteredListeners.empty())) {
+        for (auto& [listener, callbackIds] : filteredListeners) {
             callbackHandles.emplace_back(new CallbackHandle(listener, callbackIds, s.surface));
         }
     }
diff --git a/services/surfaceflinger/TransactionCallbackInvoker.cpp b/services/surfaceflinger/TransactionCallbackInvoker.cpp
index 3590e76cb9..4f4c02be6c 100644
--- a/services/surfaceflinger/TransactionCallbackInvoker.cpp
+++ b/services/surfaceflinger/TransactionCallbackInvoker.cpp
@@ -36,13 +36,17 @@ namespace android {
 //         <0 if the first id that doesn't match is lower in c2 or all ids match but c2 is shorter
 //         >0 if the first id that doesn't match is greater in c2 or all ids match but c2 is longer
 //
-// See CallbackIdsHash for a explaniation of why this works
+// See CallbackIdsHash for a explanation of why this works
 static int compareCallbackIds(const std::vector<CallbackId>& c1,
                               const std::vector<CallbackId>& c2) {
     if (c1.empty()) {
         return !c2.empty();
     }
-    return c1.front() - c2.front();
+    return c1.front().id - c2.front().id;
+}
+
+static bool containsOnCommitCallbacks(const std::vector<CallbackId>& callbacks) {
+    return !callbacks.empty() && callbacks.front().type == CallbackId::Type::ON_COMMIT;
 }
 
 TransactionCallbackInvoker::~TransactionCallbackInvoker() {
@@ -114,39 +118,69 @@ status_t TransactionCallbackInvoker::registerPendingCallbackHandle(
     return NO_ERROR;
 }
 
-status_t TransactionCallbackInvoker::finalizePendingCallbackHandles(
-        const std::deque<sp<CallbackHandle>>& handles, const std::vector<JankData>& jankData) {
+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;
+}
+
+status_t TransactionCallbackInvoker::finalizeOnCommitCallbackHandles(
+        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) {
-        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");
+        if (!containsOnCommitCallbacks(handle->callbackIds)) {
+            outRemainingHandles.push_back(handle);
+            continue;
         }
+        status_t err = finalizeCallbackHandle(handle, jankData);
+        if (err != NO_ERROR) {
+            return err;
+        }
+    }
 
-        status_t err = addCallbackHandle(handle, jankData);
+    return NO_ERROR;
+}
+
+status_t TransactionCallbackInvoker::finalizePendingCallbackHandles(
+        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);
         if (err != NO_ERROR) {
-            ALOGE("could not add callback handle");
             return err;
         }
     }
@@ -243,7 +277,8 @@ void TransactionCallbackInvoker::sendCallbacks() {
             }
 
             // If the transaction has been latched
-            if (transactionStats.latchTime >= 0) {
+            if (transactionStats.latchTime >= 0 &&
+                !containsOnCommitCallbacks(transactionStats.callbackIds)) {
                 if (!mPresentFence) {
                     break;
                 }
diff --git a/services/surfaceflinger/TransactionCallbackInvoker.h b/services/surfaceflinger/TransactionCallbackInvoker.h
index caa8a4fb45..184b15103e 100644
--- a/services/surfaceflinger/TransactionCallbackInvoker.h
+++ b/services/surfaceflinger/TransactionCallbackInvoker.h
@@ -74,6 +74,8 @@ public:
     // Notifies the TransactionCallbackInvoker that a pending CallbackHandle has been presented.
     status_t finalizePendingCallbackHandles(const std::deque<sp<CallbackHandle>>& handles,
                                             const std::vector<JankData>& jankData);
+    status_t finalizeOnCommitCallbackHandles(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.
@@ -95,6 +97,9 @@ private:
     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.