SurfaceFlinger: Emit callbacks for non-buffer layer transactions

Ensure we emit callbacks if the transaction contains only non-buffer
layer state changes.

Test: atest SurfaceFlinger_test
Fixes: 205183085
Change-Id: I56bf0dcaff4312628fe2cd1d0b93db520518ec54
diff --git a/services/surfaceflinger/BufferStateLayer.cpp b/services/surfaceflinger/BufferStateLayer.cpp
index c213570..3e09a40 100644
--- a/services/surfaceflinger/BufferStateLayer.cpp
+++ b/services/surfaceflinger/BufferStateLayer.cpp
@@ -549,13 +549,19 @@
 }
 
 bool BufferStateLayer::setTransactionCompletedListeners(
-        const std::vector<sp<CallbackHandle>>& handles) {
+        const std::vector<ListenerCallbacks>& listenerCallbacks, const sp<IBinder>& layerHandle) {
     // If there is no handle, we will not send a callback so reset mReleasePreviousBuffer and return
-    if (handles.empty()) {
+    if (listenerCallbacks.empty()) {
         mReleasePreviousBuffer = false;
         return false;
     }
 
+    std::vector<sp<CallbackHandle>> handles;
+    handles.reserve(listenerCallbacks.size());
+    for (auto& [listener, callbackIds] : listenerCallbacks) {
+        handles.emplace_back(new CallbackHandle(listener, callbackIds, layerHandle));
+    }
+
     const bool willPresent = willPresentCurrentTransaction();
 
     for (const auto& handle : handles) {
@@ -571,9 +577,10 @@
             // Store so latched time and release fence can be set
             mDrawingState.callbackHandles.push_back(handle);
 
-        } else { // If this layer will NOT need to be relatched and presented this frame
+        } else {
+            // If this layer will NOT need to be relatched and presented this frame
             // Notify the transaction completed thread this handle is done
-            mFlinger->getTransactionCallbackInvoker().registerUnpresentedCallbackHandle(handle);
+            mFlinger->getTransactionCallbackInvoker().addUnpresentedCallbackHandle(handle);
         }
     }
 
diff --git a/services/surfaceflinger/BufferStateLayer.h b/services/surfaceflinger/BufferStateLayer.h
index eea700c..ceed188 100644
--- a/services/surfaceflinger/BufferStateLayer.h
+++ b/services/surfaceflinger/BufferStateLayer.h
@@ -65,7 +65,8 @@
     bool setSurfaceDamageRegion(const Region& surfaceDamage) override;
     bool setApi(int32_t api) override;
     bool setSidebandStream(const sp<NativeHandle>& sidebandStream) override;
-    bool setTransactionCompletedListeners(const std::vector<sp<CallbackHandle>>& handles) override;
+    bool setTransactionCompletedListeners(const std::vector<ListenerCallbacks>& handles,
+                                          const sp<IBinder>& layerHandle) override;
     bool addFrameEvent(const sp<Fence>& acquireFence, nsecs_t postedTime,
                        nsecs_t requestedPresentTime) override;
     bool setPosition(float /*x*/, float /*y*/) override;
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index d68cf97..d8854d3 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -2645,6 +2645,17 @@
     return true;
 }
 
+bool Layer::setTransactionCompletedListeners(
+        const std::vector<ListenerCallbacks>& listenerCallbacks, const sp<IBinder>&) {
+    if (listenerCallbacks.empty()) {
+        return false;
+    }
+    for (auto& listener : listenerCallbacks) {
+        mFlinger->getTransactionCallbackInvoker().addEmptyCallback(listener);
+    }
+    return false;
+}
+
 // ---------------------------------------------------------------------------
 
 std::ostream& operator<<(std::ostream& stream, const Layer::FrameRate& rate) {
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index 4569f9a..f6f1621 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -426,10 +426,8 @@
     virtual bool setSurfaceDamageRegion(const Region& /*surfaceDamage*/) { return false; };
     virtual bool setApi(int32_t /*api*/) { return false; };
     virtual bool setSidebandStream(const sp<NativeHandle>& /*sidebandStream*/) { return false; };
-    virtual bool setTransactionCompletedListeners(
-            const std::vector<sp<CallbackHandle>>& /*handles*/) {
-        return false;
-    };
+    virtual bool setTransactionCompletedListeners(const std::vector<ListenerCallbacks>& /*handles*/,
+                                                  const sp<IBinder>& /* layerHandle */);
     virtual bool addFrameEvent(const sp<Fence>& /*acquireFence*/, nsecs_t /*postedTime*/,
                                nsecs_t /*requestedPresentTime*/) {
         return false;
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 53a4ae0..af6d000 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -3727,11 +3727,10 @@
         transactionFlags |= setDisplayStateLocked(display);
     }
 
-    // start and end registration for listeners w/ no surface so they can get their callback.  Note
-    // that listeners with SurfaceControls will start registration during setClientStateLocked
-    // below.
+    // Add listeners w/ surfaces so they can get their callback.  Note that listeners with
+    // SurfaceControls will start registration during setClientStateLocked below.
     for (const auto& listener : listenerCallbacks) {
-        mTransactionCallbackInvoker.addEmptyTransaction(listener);
+        mTransactionCallbackInvoker.addEmptyCallback(listener);
     }
 
     uint32_t clientStateFlags = 0;
@@ -3903,7 +3902,7 @@
     }
     if (layer == nullptr) {
         for (auto& [listener, callbackIds] : s.listeners) {
-            mTransactionCallbackInvoker.registerUnpresentedCallbackHandle(
+            mTransactionCallbackInvoker.addUnpresentedCallbackHandle(
                     new CallbackHandle(listener, callbackIds, s.surface));
         }
         return 0;
@@ -4173,12 +4172,6 @@
             flags |= eTransactionNeeded | eTraversalNeeded;
         }
     }
-    std::vector<sp<CallbackHandle>> callbackHandles;
-    if ((what & layer_state_t::eHasListenerCallbacksChanged) && (!filteredListeners.empty())) {
-        for (auto& [listener, callbackIds] : filteredListeners) {
-            callbackHandles.emplace_back(new CallbackHandle(listener, callbackIds, s.surface));
-        }
-    }
 
     if (what & layer_state_t::eBufferChanged &&
         layer->setBuffer(s.bufferData, postTime, desiredPresentTime, isAutoTimestamp,
@@ -4188,7 +4181,11 @@
         layer->setFrameTimelineVsyncForBufferlessTransaction(frameTimelineInfo, postTime);
     }
 
-    if (layer->setTransactionCompletedListeners(callbackHandles)) flags |= eTraversalNeeded;
+    if ((what & layer_state_t::eHasListenerCallbacksChanged) && (!filteredListeners.empty())) {
+        if (layer->setTransactionCompletedListeners(filteredListeners, s.surface)) {
+            flags |= eTraversalNeeded;
+        }
+    }
     // Do not put anything that updates layer state or modifies flags after
     // setTransactionCompletedListener
     return flags;
diff --git a/services/surfaceflinger/TransactionCallbackInvoker.cpp b/services/surfaceflinger/TransactionCallbackInvoker.cpp
index f3d46ea..418fbc5 100644
--- a/services/surfaceflinger/TransactionCallbackInvoker.cpp
+++ b/services/surfaceflinger/TransactionCallbackInvoker.cpp
@@ -74,10 +74,10 @@
     }
 }
 
-void TransactionCallbackInvoker::addEmptyTransaction(const ListenerCallbacks& listenerCallbacks) {
+void TransactionCallbackInvoker::addEmptyCallback(const ListenerCallbacks& listenerCallbacks) {
     auto& [listener, callbackIds] = listenerCallbacks;
-    auto& transactionStatsDeque = mCompletedTransactions[listener];
-    transactionStatsDeque.emplace_back(callbackIds);
+    TransactionStats* transactionStats;
+    findOrCreateTransactionStats(listener, callbackIds, &transactionStats);
 }
 
 status_t TransactionCallbackInvoker::addOnCommitCallbackHandles(
@@ -116,7 +116,7 @@
     return NO_ERROR;
 }
 
-status_t TransactionCallbackInvoker::registerUnpresentedCallbackHandle(
+status_t TransactionCallbackInvoker::addUnpresentedCallbackHandle(
         const sp<CallbackHandle>& handle) {
     return addCallbackHandle(handle, std::vector<JankData>());
 }
diff --git a/services/surfaceflinger/TransactionCallbackInvoker.h b/services/surfaceflinger/TransactionCallbackInvoker.h
index e203d41..6f67947 100644
--- a/services/surfaceflinger/TransactionCallbackInvoker.h
+++ b/services/surfaceflinger/TransactionCallbackInvoker.h
@@ -71,8 +71,10 @@
 
     // 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);
+    status_t addUnpresentedCallbackHandle(const sp<CallbackHandle>& handle);
+    // Adds the callback handles for empty transactions or for non-buffer layer updates which do not
+    // include layer stats.
+    void addEmptyCallback(const ListenerCallbacks& listenerCallbacks);
 
     void addPresentFence(const sp<Fence>& presentFence);
 
@@ -81,14 +83,12 @@
         mCompletedTransactions.clear();
     }
 
-    status_t addCallbackHandle(const sp<CallbackHandle>& handle,
-                               const std::vector<JankData>& jankData);
-
-
 private:
     status_t findOrCreateTransactionStats(const sp<IBinder>& listener,
                                           const std::vector<CallbackId>& callbackIds,
                                           TransactionStats** outTransactionStats);
+    status_t addCallbackHandle(const sp<CallbackHandle>& handle,
+                               const std::vector<JankData>& jankData);
 
     std::unordered_map<sp<IBinder>, std::deque<TransactionStats>, IListenerHash>
         mCompletedTransactions;
diff --git a/services/surfaceflinger/tests/LayerCallback_test.cpp b/services/surfaceflinger/tests/LayerCallback_test.cpp
index 91a5b52..7beba15 100644
--- a/services/surfaceflinger/tests/LayerCallback_test.cpp
+++ b/services/surfaceflinger/tests/LayerCallback_test.cpp
@@ -1067,7 +1067,7 @@
 }
 
 // b202394221
-TEST_F(LayerCallbackTest, DISABLED_NonBufferLayerStateChanges) {
+TEST_F(LayerCallbackTest, NonBufferLayerStateChanges) {
     sp<SurfaceControl> layer;
     ASSERT_NO_FATAL_FAILURE(layer = createColorLayer("ColorLayer", Color::RED));
 
@@ -1085,4 +1085,47 @@
     EXPECT_NO_FATAL_FAILURE(waitForCallback(callback, expected, true));
 }
 
+class TimedCallbackHelper {
+public:
+    static void function(void* callbackContext, nsecs_t, const sp<Fence>&,
+                         const std::vector<SurfaceControlStats>&) {
+        if (!callbackContext) {
+            ALOGE("failed to get callback context");
+        }
+        TimedCallbackHelper* helper = static_cast<TimedCallbackHelper*>(callbackContext);
+        std::lock_guard lock(helper->mMutex);
+        helper->mInvokedTime = systemTime();
+        helper->mCv.notify_all();
+    }
+
+    void waitForCallback() {
+        std::unique_lock lock(mMutex);
+        ASSERT_TRUE(mCv.wait_for(lock, std::chrono::seconds(3), [&] { return mInvokedTime != -1; }))
+                << "did not receive callback";
+    }
+    void* getContext() { return static_cast<void*>(this); }
+
+    std::mutex mMutex;
+    std::condition_variable mCv;
+    nsecs_t mInvokedTime = -1;
+};
+
+TEST_F(LayerCallbackTest, EmptyTransactionCallbackOrder) {
+    TimedCallbackHelper onCommitCallback;
+    TimedCallbackHelper onCompleteCallback;
+
+    // Add transaction callback before on commit callback
+    Transaction()
+            .addTransactionCompletedCallback(onCompleteCallback.function,
+                                             onCompleteCallback.getContext())
+            .addTransactionCommittedCallback(onCommitCallback.function,
+                                             onCommitCallback.getContext())
+            .apply();
+
+    EXPECT_NO_FATAL_FAILURE(onCompleteCallback.waitForCallback());
+    EXPECT_NO_FATAL_FAILURE(onCommitCallback.waitForCallback());
+    // verify we get the oncomplete at the same time or after the oncommit callback.
+    EXPECT_GE(onCompleteCallback.mInvokedTime, onCommitCallback.mInvokedTime);
+}
+
 } // namespace android