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
diff --git a/services/surfaceflinger/BufferStateLayer.cpp b/services/surfaceflinger/BufferStateLayer.cpp
index 3e09a40..c213570 100644
--- a/services/surfaceflinger/BufferStateLayer.cpp
+++ b/services/surfaceflinger/BufferStateLayer.cpp
@@ -549,19 +549,13 @@
}
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 @@
// 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 ceed188..eea700c 100644
--- a/services/surfaceflinger/BufferStateLayer.h
+++ b/services/surfaceflinger/BufferStateLayer.h
@@ -65,8 +65,7 @@
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 d8854d3..d68cf97 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -2645,17 +2645,6 @@
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 f6f1621..4569f9a 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -426,8 +426,10 @@
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 af6d000..53a4ae0 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -3727,10 +3727,11 @@
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 @@
}
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 @@
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,
@@ -4181,11 +4188,7 @@
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 418fbc5..f3d46ea 100644
--- a/services/surfaceflinger/TransactionCallbackInvoker.cpp
+++ b/services/surfaceflinger/TransactionCallbackInvoker.cpp
@@ -74,10 +74,10 @@
}
}
-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 @@
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 6f67947..e203d41 100644
--- a/services/surfaceflinger/TransactionCallbackInvoker.h
+++ b/services/surfaceflinger/TransactionCallbackInvoker.h
@@ -71,10 +71,8 @@
// 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 @@
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 7beba15..91a5b52 100644
--- a/services/surfaceflinger/tests/LayerCallback_test.cpp
+++ b/services/surfaceflinger/tests/LayerCallback_test.cpp
@@ -1067,7 +1067,7 @@
}
// 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 @@
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