Merge "Revert "SurfaceFlinger: Emit callbacks for non-buffer layer transactions""

8 files changed, 32 insertions, 89 deletions

diff --git a/services/surfaceflinger/BufferStateLayer.cpp b/services/surfaceflinger/BufferStateLayer.cpp
index b6cbbb658f..4f0bbd243b 100644
--- a/services/surfaceflinger/BufferStateLayer.cpp
+++ b/services/surfaceflinger/BufferStateLayer.cpp
@@ -549,19 +549,13 @@ bool BufferStateLayer::setSidebandStream(const sp<NativeHandle>& sidebandStream)
 }
 
 bool BufferStateLayer::setTransactionCompletedListeners(
-        const std::vector<ListenerCallbacks>& listenerCallbacks, const sp<IBinder>& layerHandle) {
+        const std::vector<sp<CallbackHandle>>& handles) {
     // If there is no handle, we will not send a callback so reset mReleasePreviousBuffer and return
-    if (listenerCallbacks.empty()) {
+    if (handles.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) {
@@ -577,10 +571,9 @@ bool BufferStateLayer::setTransactionCompletedListeners(
             // 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().addUnpresentedCallbackHandle(handle);
+            mFlinger->getTransactionCallbackInvoker().registerUnpresentedCallbackHandle(handle);
         }
     }
 
diff --git a/services/surfaceflinger/BufferStateLayer.h b/services/surfaceflinger/BufferStateLayer.h
index ceed188f0e..eea700cf7b 100644
--- a/services/surfaceflinger/BufferStateLayer.h
+++ b/services/surfaceflinger/BufferStateLayer.h
@@ -65,8 +65,7 @@ public:
     bool setSurfaceDamageRegion(const Region& surfaceDamage) override;
     bool setApi(int32_t api) override;
     bool setSidebandStream(const sp<NativeHandle>& sidebandStream) override;
-    bool setTransactionCompletedListeners(const std::vector<ListenerCallbacks>& handles,
-                                          const sp<IBinder>& layerHandle) override;
+    bool setTransactionCompletedListeners(const std::vector<sp<CallbackHandle>>& handles) 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 d85e843d83..a27c487822 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -2640,17 +2640,6 @@ bool Layer::setDropInputMode(gui::DropInputMode mode) {
     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 b79903d11b..297ded043f 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -428,8 +428,10 @@ public:
     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<ListenerCallbacks>& /*handles*/,
-                                                  const sp<IBinder>& /* layerHandle */);
+    virtual bool setTransactionCompletedListeners(
+            const std::vector<sp<CallbackHandle>>& /*handles*/) {
+        return false;
+    };
     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 4770feb6b1..d0f2174ad6 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -3795,10 +3795,11 @@ bool SurfaceFlinger::applyTransactionState(const FrameTimelineInfo& frameTimelin
         transactionFlags |= setDisplayStateLocked(display);
     }
 
-    // Add listeners w/ surfaces so they can get their callback.  Note that listeners with
-    // SurfaceControls will start registration during setClientStateLocked below.
+    // 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.
     for (const auto& listener : listenerCallbacks) {
-        mTransactionCallbackInvoker.addEmptyCallback(listener);
+        mTransactionCallbackInvoker.addEmptyTransaction(listener);
     }
 
     uint32_t clientStateFlags = 0;
@@ -3970,7 +3971,7 @@ uint32_t SurfaceFlinger::setClientStateLocked(const FrameTimelineInfo& frameTime
     }
     if (layer == nullptr) {
         for (auto& [listener, callbackIds] : s.listeners) {
-            mTransactionCallbackInvoker.addUnpresentedCallbackHandle(
+            mTransactionCallbackInvoker.registerUnpresentedCallbackHandle(
                     new CallbackHandle(listener, callbackIds, s.surface));
         }
         return 0;
@@ -4241,6 +4242,12 @@ uint32_t SurfaceFlinger::setClientStateLocked(const FrameTimelineInfo& frameTime
             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,
@@ -4250,11 +4257,7 @@ uint32_t SurfaceFlinger::setClientStateLocked(const FrameTimelineInfo& frameTime
         layer->setFrameTimelineVsyncForBufferlessTransaction(frameTimelineInfo, postTime);
     }
 
-    if ((what & layer_state_t::eHasListenerCallbacksChanged) && (!filteredListeners.empty())) {
-        if (layer->setTransactionCompletedListeners(filteredListeners, s.surface)) {
-            flags |= eTraversalNeeded;
-        }
-    }
+    if (layer->setTransactionCompletedListeners(callbackHandles)) 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 418fbc5c44..f3d46ea061 100644
--- a/services/surfaceflinger/TransactionCallbackInvoker.cpp
+++ b/services/surfaceflinger/TransactionCallbackInvoker.cpp
@@ -74,10 +74,10 @@ TransactionCallbackInvoker::~TransactionCallbackInvoker() {
     }
 }
 
-void TransactionCallbackInvoker::addEmptyCallback(const ListenerCallbacks& listenerCallbacks) {
+void TransactionCallbackInvoker::addEmptyTransaction(const ListenerCallbacks& listenerCallbacks) {
     auto& [listener, callbackIds] = listenerCallbacks;
-    TransactionStats* transactionStats;
-    findOrCreateTransactionStats(listener, callbackIds, &transactionStats);
+    auto& transactionStatsDeque = mCompletedTransactions[listener];
+    transactionStatsDeque.emplace_back(callbackIds);
 }
 
 status_t TransactionCallbackInvoker::addOnCommitCallbackHandles(
@@ -116,7 +116,7 @@ status_t TransactionCallbackInvoker::addCallbackHandles(
     return NO_ERROR;
 }
 
-status_t TransactionCallbackInvoker::addUnpresentedCallbackHandle(
+status_t TransactionCallbackInvoker::registerUnpresentedCallbackHandle(
         const sp<CallbackHandle>& handle) {
     return addCallbackHandle(handle, std::vector<JankData>());
 }
diff --git a/services/surfaceflinger/TransactionCallbackInvoker.h b/services/surfaceflinger/TransactionCallbackInvoker.h
index 6f67947081..e203d41bd9 100644
--- a/services/surfaceflinger/TransactionCallbackInvoker.h
+++ b/services/surfaceflinger/TransactionCallbackInvoker.h
@@ -71,10 +71,8 @@ public:
 
     // Adds the Transaction CallbackHandle from a layer that does not need to be relatched and
     // presented this frame.
-    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);
+    status_t registerUnpresentedCallbackHandle(const sp<CallbackHandle>& handle);
+    void addEmptyTransaction(const ListenerCallbacks& listenerCallbacks);
 
     void addPresentFence(const sp<Fence>& presentFence);
 
@@ -83,12 +81,14 @@ public:
         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 7beba15062..91a5b528f8 100644
--- a/services/surfaceflinger/tests/LayerCallback_test.cpp
+++ b/services/surfaceflinger/tests/LayerCallback_test.cpp
@@ -1067,7 +1067,7 @@ TEST_F(LayerCallbackTest, EmptyBufferStateChanges) {
 }
 
 // b202394221
-TEST_F(LayerCallbackTest, NonBufferLayerStateChanges) {
+TEST_F(LayerCallbackTest, DISABLED_NonBufferLayerStateChanges) {
     sp<SurfaceControl> layer;
     ASSERT_NO_FATAL_FAILURE(layer = createColorLayer("ColorLayer", Color::RED));
 
@@ -1085,47 +1085,4 @@ TEST_F(LayerCallbackTest, NonBufferLayerStateChanges) {
     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