From a5505cb8fa988023685e8d67fcc8117ee7c63b1e Mon Sep 17 00:00:00 2001 From: Jiakai Zhang Date: Tue, 9 Nov 2021 11:46:30 +0000 Subject: Revert "SurfaceFlinger: Emit callbacks for non-buffer layer transactions" This reverts commit da1fd1508c914c7f0849e4e00a5fae5412433337. Reason for revert: Potential culprit CL for broken test b/205501709 Change-Id: Iece745e0690f4d31b93918697b306ded3abfd0d0 --- services/surfaceflinger/BufferStateLayer.cpp | 15 ++------ services/surfaceflinger/BufferStateLayer.h | 3 +- services/surfaceflinger/Layer.cpp | 11 ------ services/surfaceflinger/Layer.h | 6 ++- services/surfaceflinger/SurfaceFlinger.cpp | 21 +++++----- .../surfaceflinger/TransactionCallbackInvoker.cpp | 8 ++-- .../surfaceflinger/TransactionCallbackInvoker.h | 12 +++--- .../surfaceflinger/tests/LayerCallback_test.cpp | 45 +--------------------- 8 files changed, 32 insertions(+), 89 deletions(-) diff --git a/services/surfaceflinger/BufferStateLayer.cpp b/services/surfaceflinger/BufferStateLayer.cpp index 3e09a40eba..c213570c7f 100644 --- a/services/surfaceflinger/BufferStateLayer.cpp +++ b/services/surfaceflinger/BufferStateLayer.cpp @@ -549,19 +549,13 @@ bool BufferStateLayer::setSidebandStream(const sp& sidebandStream) } bool BufferStateLayer::setTransactionCompletedListeners( - const std::vector& listenerCallbacks, const sp& layerHandle) { + const std::vector>& 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> 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& sidebandStream) override; - bool setTransactionCompletedListeners(const std::vector& handles, - const sp& layerHandle) override; + bool setTransactionCompletedListeners(const std::vector>& handles) override; bool addFrameEvent(const sp& 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 d8854d3033..d68cf9720f 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -2645,17 +2645,6 @@ bool Layer::setDropInputMode(gui::DropInputMode mode) { return true; } -bool Layer::setTransactionCompletedListeners( - const std::vector& listenerCallbacks, const sp&) { - 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 f6f162128f..4569f9af23 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -426,8 +426,10 @@ public: virtual bool setSurfaceDamageRegion(const Region& /*surfaceDamage*/) { return false; }; virtual bool setApi(int32_t /*api*/) { return false; }; virtual bool setSidebandStream(const sp& /*sidebandStream*/) { return false; }; - virtual bool setTransactionCompletedListeners(const std::vector& /*handles*/, - const sp& /* layerHandle */); + virtual bool setTransactionCompletedListeners( + const std::vector>& /*handles*/) { + return false; + }; virtual bool addFrameEvent(const sp& /*acquireFence*/, nsecs_t /*postedTime*/, nsecs_t /*requestedPresentTime*/) { return false; diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index af6d00041e..53a4ae02eb 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -3727,10 +3727,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; @@ -3902,7 +3903,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; @@ -4172,6 +4173,12 @@ uint32_t SurfaceFlinger::setClientStateLocked(const FrameTimelineInfo& frameTime flags |= eTransactionNeeded | eTraversalNeeded; } } + std::vector> 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, @@ -4181,11 +4188,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& handle) { return addCallbackHandle(handle, std::vector()); } 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& 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& handle); + void addEmptyTransaction(const ListenerCallbacks& listenerCallbacks); void addPresentFence(const sp& presentFence); @@ -83,12 +81,14 @@ public: mCompletedTransactions.clear(); } + status_t addCallbackHandle(const sp& handle, + const std::vector& jankData); + + private: status_t findOrCreateTransactionStats(const sp& listener, const std::vector& callbackIds, TransactionStats** outTransactionStats); - status_t addCallbackHandle(const sp& handle, - const std::vector& jankData); std::unordered_map, std::deque, 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 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&, - const std::vector&) { - if (!callbackContext) { - ALOGE("failed to get callback context"); - } - TimedCallbackHelper* helper = static_cast(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(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 -- cgit v1.2.3-59-g8ed1b