SurfaceFlinger: Fix duplicate callbacks
Transaction callbacks are emitted early, for empty transactions,
after latch for on commit callbacks and on post composition for
transaction complete callbacks. Pending callbacks are stored
together. If a transaction contains a layer that is presented
and a layer that is not presented, the transaction callback will
get added to the pending list after the transaction is committed
and it will get emitted after buffers are latched and then again
at the end of composition. To fix this we make sure to only emit
on commit callbacks after a buffer is latched.
Test: atest SurfaceFlinger_test
Bug: 202394221
Change-Id: I356fbf221812060c17765c53cc3df24cb3cd139a
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 54bcd15..53a4ae0 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2021,17 +2021,23 @@
bool SurfaceFlinger::flushAndCommitTransactions() {
ATRACE_CALL();
+ bool needsTraversal = false;
if (clearTransactionFlags(eTransactionFlushNeeded)) {
- flushTransactionQueues();
+ needsTraversal = flushTransactionQueues();
}
- const bool shouldCommit = (getTransactionFlags() & ~eTransactionFlushNeeded) || mForceTraversal;
+ const bool shouldCommit = (getTransactionFlags() & ~eTransactionFlushNeeded) || needsTraversal;
if (shouldCommit) {
commitTransactions();
}
- // Invoke OnCommit callbacks.
- mTransactionCallbackInvoker.sendCallbacks();
+ if (!needsTraversal) {
+ // Invoke empty transaction callbacks early.
+ mTransactionCallbackInvoker.sendCallbacks(false /* onCommitOnly */);
+ } else {
+ // Invoke OnCommit callbacks.
+ mTransactionCallbackInvoker.sendCallbacks(true /* onCommitOnly */);
+ }
if (transactionFlushNeeded()) {
setTransactionFlags(eTransactionFlushNeeded);
@@ -2328,7 +2334,7 @@
mVisibleRegionsWereDirtyThisFrame = false;
mTransactionCallbackInvoker.addPresentFence(mPreviousPresentFences[0].fence);
- mTransactionCallbackInvoker.sendCallbacks();
+ mTransactionCallbackInvoker.sendCallbacks(false /* onCommitOnly */);
mTransactionCallbackInvoker.clearCompletedTransactions();
if (display && display->isInternal() && display->getPowerMode() == hal::PowerMode::ON &&
@@ -2941,7 +2947,6 @@
processDisplayChangesLocked();
processDisplayHotplugEventsLocked();
}
- mForceTraversal = false;
mForceTransactionDisplayChange = displayTransactionNeeded;
if (mSomeChildrenChanged) {
@@ -3415,11 +3420,8 @@
return old;
}
-void SurfaceFlinger::setTraversalNeeded() {
- mForceTraversal = true;
-}
-
-void SurfaceFlinger::flushTransactionQueues() {
+bool SurfaceFlinger::flushTransactionQueues() {
+ bool needsTraversal = false;
// to prevent onHandleDestroyed from being called while the lock is held,
// we must keep a copy of the transactions (specifically the composer
// states) around outside the scope of the lock
@@ -3488,19 +3490,23 @@
// Now apply all transactions.
for (const auto& transaction : transactions) {
- applyTransactionState(transaction.frameTimelineInfo, transaction.states,
- transaction.displays, transaction.flags,
- transaction.inputWindowCommands, transaction.desiredPresentTime,
- transaction.isAutoTimestamp, transaction.buffer,
- transaction.postTime, transaction.permissions,
- transaction.hasListenerCallbacks, transaction.listenerCallbacks,
- transaction.originPid, transaction.originUid, transaction.id);
+ needsTraversal |=
+ applyTransactionState(transaction.frameTimelineInfo, transaction.states,
+ transaction.displays, transaction.flags,
+ transaction.inputWindowCommands,
+ transaction.desiredPresentTime,
+ transaction.isAutoTimestamp, transaction.buffer,
+ transaction.postTime, transaction.permissions,
+ transaction.hasListenerCallbacks,
+ transaction.listenerCallbacks, transaction.originPid,
+ transaction.originUid, transaction.id);
if (transaction.transactionCommittedSignal) {
mTransactionCommittedSignals.emplace_back(
std::move(transaction.transactionCommittedSignal));
}
}
- }
+ } // unlock mStateLock
+ return needsTraversal;
}
bool SurfaceFlinger::transactionFlushNeeded() {
@@ -3706,7 +3712,7 @@
return NO_ERROR;
}
-void SurfaceFlinger::applyTransactionState(const FrameTimelineInfo& frameTimelineInfo,
+bool SurfaceFlinger::applyTransactionState(const FrameTimelineInfo& frameTimelineInfo,
const Vector<ComposerState>& states,
const Vector<DisplayState>& displays, uint32_t flags,
const InputWindowCommands& inputWindowCommands,
@@ -3728,12 +3734,10 @@
mTransactionCallbackInvoker.addEmptyTransaction(listener);
}
- std::unordered_set<ListenerCallbacks, ListenerCallbacksHash> listenerCallbacksWithSurfaces;
uint32_t clientStateFlags = 0;
for (const ComposerState& state : states) {
- clientStateFlags |=
- setClientStateLocked(frameTimelineInfo, state, desiredPresentTime, isAutoTimestamp,
- postTime, permissions, listenerCallbacksWithSurfaces);
+ clientStateFlags |= setClientStateLocked(frameTimelineInfo, state, desiredPresentTime,
+ isAutoTimestamp, postTime, permissions);
if ((flags & eAnimation) && state.state.surface) {
if (const auto layer = fromHandle(state.state.surface).promote(); layer) {
mScheduler->recordLayerHistory(layer.get(),
@@ -3743,10 +3747,6 @@
}
}
- // If the state doesn't require a traversal and there are callbacks, send them now
- if (!(clientStateFlags & eTraversalNeeded) && hasListenerCallbacks) {
- mTransactionCallbackInvoker.sendCallbacks();
- }
transactionFlags |= clientStateFlags;
if (permissions & Permission::ACCESS_SURFACE_FLINGER) {
@@ -3768,6 +3768,7 @@
transactionFlags = eTransactionNeeded;
}
+ bool needsTraversal = false;
if (transactionFlags) {
if (mInterceptor->isEnabled()) {
mInterceptor->saveTransaction(states, mCurrentState.displays, displays, flags,
@@ -3778,7 +3779,7 @@
// so we don't have to wake up again next frame to preform an unnecessary traversal.
if (transactionFlags & eTraversalNeeded) {
transactionFlags = transactionFlags & (~eTraversalNeeded);
- mForceTraversal = true;
+ needsTraversal = true;
}
if (transactionFlags) {
setTransactionFlags(transactionFlags);
@@ -3788,6 +3789,8 @@
mAnimTransactionPending = true;
}
}
+
+ return needsTraversal;
}
uint32_t SurfaceFlinger::setDisplayStateLocked(const DisplayState& s) {
@@ -3856,10 +3859,10 @@
return true;
}
-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>& outListenerCallbacks) {
+uint32_t SurfaceFlinger::setClientStateLocked(const FrameTimelineInfo& frameTimelineInfo,
+ const ComposerState& composerState,
+ int64_t desiredPresentTime, bool isAutoTimestamp,
+ int64_t postTime, uint32_t permissions) {
const layer_state_t& s = composerState.state;
const bool privileged = permissions & Permission::ACCESS_SURFACE_FLINGER;
@@ -3873,13 +3876,11 @@
ListenerCallbacks onCommitCallbacks = listener.filter(CallbackId::Type::ON_COMMIT);
if (!onCommitCallbacks.callbackIds.empty()) {
filteredListeners.push_back(onCommitCallbacks);
- outListenerCallbacks.insert(onCommitCallbacks);
}
ListenerCallbacks onCompleteCallbacks = listener.filter(CallbackId::Type::ON_COMPLETE);
if (!onCompleteCallbacks.callbackIds.empty()) {
filteredListeners.push_back(onCompleteCallbacks);
- outListenerCallbacks.insert(onCompleteCallbacks);
}
}