blast: transaction callbacks should come in order
Previously, if no SurfaceControls where marked to get callbacks,
a callback was sent directly from SurfaceComposerClient so we could
save a binder call into SurfaceFlinger and a binder call for the
callback.
Although this saved us 2 binder calls, it made the transactions
callbacks come out of order. The public api guarantees that all
callbacks must come in order.
This patch moves the callback from SurfaceComposerClient into
SurfaceFlinger so the callbacks are in order.
Bug: 128519264
Test: SurfaceFlinger_test
Change-Id: Ia1cadb81adb69b58a4d6d43ae453c96a1572f833
diff --git a/libs/gui/ISurfaceComposer.cpp b/libs/gui/ISurfaceComposer.cpp
index bc63d31..b74d675 100644
--- a/libs/gui/ISurfaceComposer.cpp
+++ b/libs/gui/ISurfaceComposer.cpp
@@ -69,7 +69,8 @@
const sp<IBinder>& applyToken,
const InputWindowCommands& commands,
int64_t desiredPresentTime,
- const cached_buffer_t& uncacheBuffer) {
+ const cached_buffer_t& uncacheBuffer,
+ const std::vector<ListenerCallbacks>& listenerCallbacks) {
Parcel data, reply;
data.writeInterfaceToken(ISurfaceComposer::getInterfaceDescriptor());
@@ -89,6 +90,14 @@
data.writeInt64(desiredPresentTime);
data.writeStrongBinder(uncacheBuffer.token);
data.writeUint64(uncacheBuffer.cacheId);
+
+ if (data.writeVectorSize(listenerCallbacks) == NO_ERROR) {
+ for (const auto& [listener, callbackIds] : listenerCallbacks) {
+ data.writeStrongBinder(IInterface::asBinder(listener));
+ data.writeInt64Vector(callbackIds);
+ }
+ }
+
remote()->transact(BnSurfaceComposer::SET_TRANSACTION_STATE, data, &reply);
}
@@ -978,8 +987,18 @@
uncachedBuffer.token = data.readStrongBinder();
uncachedBuffer.cacheId = data.readUint64();
+ std::vector<ListenerCallbacks> listenerCallbacks;
+ int32_t listenersSize = data.readInt32();
+ for (int32_t i = 0; i < listenersSize; i++) {
+ auto listener =
+ interface_cast<ITransactionCompletedListener>(data.readStrongBinder());
+ std::vector<CallbackId> callbackIds;
+ data.readInt64Vector(&callbackIds);
+ listenerCallbacks.emplace_back(listener, callbackIds);
+ }
+
setTransactionState(state, displays, stateFlags, applyToken, inputWindowCommands,
- desiredPresentTime, uncachedBuffer);
+ desiredPresentTime, uncachedBuffer, listenerCallbacks);
return NO_ERROR;
}
case BOOT_FINISHED: {
diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp
index f6ca9e8..3077b21 100644
--- a/libs/gui/LayerState.cpp
+++ b/libs/gui/LayerState.cpp
@@ -86,14 +86,7 @@
memcpy(output.writeInplace(16 * sizeof(float)),
colorTransform.asArray(), 16 * sizeof(float));
output.writeFloat(cornerRadius);
-
- if (output.writeVectorSize(listenerCallbacks) == NO_ERROR) {
- for (const auto& [listener, callbackIds] : listenerCallbacks) {
- output.writeStrongBinder(IInterface::asBinder(listener));
- output.writeInt64Vector(callbackIds);
- }
- }
-
+ output.writeBool(hasListenerCallbacks);
output.writeStrongBinder(cachedBuffer.token);
output.writeUint64(cachedBuffer.cacheId);
output.writeParcelable(metadata);
@@ -163,15 +156,7 @@
colorTransform = mat4(static_cast<const float*>(input.readInplace(16 * sizeof(float))));
cornerRadius = input.readFloat();
-
- int32_t listenersSize = input.readInt32();
- for (int32_t i = 0; i < listenersSize; i++) {
- auto listener = interface_cast<ITransactionCompletedListener>(input.readStrongBinder());
- std::vector<CallbackId> callbackIds;
- input.readInt64Vector(&callbackIds);
- listenerCallbacks.emplace_back(listener, callbackIds);
- }
-
+ hasListenerCallbacks = input.readBool();
cachedBuffer.token = input.readStrongBinder();
cachedBuffer.cacheId = input.readUint64();
input.readParcelable(&metadata);
@@ -376,9 +361,9 @@
what |= eColorTransformChanged;
colorTransform = other.colorTransform;
}
- if (other.what & eListenerCallbacksChanged) {
- what |= eListenerCallbacksChanged;
- listenerCallbacks = other.listenerCallbacks;
+ if (other.what & eHasListenerCallbacksChanged) {
+ what |= eHasListenerCallbacksChanged;
+ hasListenerCallbacks = other.hasListenerCallbacks;
}
#ifndef NO_INPUT
diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp
index 39cd62f..84aed5f 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -381,7 +381,7 @@
s.state.parentHandleForChild = nullptr;
composerStates.add(s);
- sf->setTransactionState(composerStates, displayStates, 0, nullptr, {}, -1, {});
+ sf->setTransactionState(composerStates, displayStates, 0, nullptr, {}, -1, {}, {});
}
void SurfaceComposerClient::doUncacheBufferTransaction(uint64_t cacheId) {
@@ -391,7 +391,7 @@
uncacheBuffer.token = BufferCache::getInstance().getToken();
uncacheBuffer.cacheId = cacheId;
- sf->setTransactionState({}, {}, 0, nullptr, {}, -1, uncacheBuffer);
+ sf->setTransactionState({}, {}, 0, nullptr, {}, -1, uncacheBuffer, {});
}
void SurfaceComposerClient::Transaction::cacheBuffers() {
@@ -434,6 +434,8 @@
sp<ISurfaceComposer> sf(ComposerService::getComposerService());
+ std::vector<ListenerCallbacks> listenerCallbacks;
+
// For every listener with registered callbacks
for (const auto& [listener, callbackInfo] : mListenerCallbacks) {
auto& [callbackIds, surfaceControls] = callbackInfo;
@@ -441,11 +443,7 @@
continue;
}
- // If the listener does not have any SurfaceControls set on this Transaction, send the
- // callback now
- if (surfaceControls.empty()) {
- listener->onTransactionCompleted(ListenerStats::createEmpty(listener, callbackIds));
- }
+ listenerCallbacks.emplace_back(listener, std::move(callbackIds));
// If the listener has any SurfaceControls set on this Transaction update the surface state
for (const auto& surfaceControl : surfaceControls) {
@@ -454,8 +452,8 @@
ALOGE("failed to get layer state");
continue;
}
- s->what |= layer_state_t::eListenerCallbacksChanged;
- s->listenerCallbacks.emplace_back(listener, std::move(callbackIds));
+ s->what |= layer_state_t::eHasListenerCallbacksChanged;
+ s->hasListenerCallbacks = true;
}
}
mListenerCallbacks.clear();
@@ -494,7 +492,8 @@
sp<IBinder> applyToken = IInterface::asBinder(TransactionCompletedListener::getIInstance());
sf->setTransactionState(composerStates, displayStates, flags, applyToken, mInputWindowCommands,
mDesiredPresentTime,
- {} /*uncacheBuffer - only set in doUncacheBufferTransaction*/);
+ {} /*uncacheBuffer - only set in doUncacheBufferTransaction*/,
+ listenerCallbacks);
mInputWindowCommands.clear();
mStatus = NO_ERROR;
return NO_ERROR;
diff --git a/libs/gui/include/gui/ISurfaceComposer.h b/libs/gui/include/gui/ISurfaceComposer.h
index 0ef5b39..fe85fdf 100644
--- a/libs/gui/include/gui/ISurfaceComposer.h
+++ b/libs/gui/include/gui/ISurfaceComposer.h
@@ -20,13 +20,10 @@
#include <stdint.h>
#include <sys/types.h>
-#include <utils/RefBase.h>
-#include <utils/Errors.h>
-#include <utils/Timers.h>
-#include <utils/Vector.h>
-
#include <binder/IInterface.h>
+#include <gui/ITransactionCompletedListener.h>
+
#include <ui/ConfigStoreTypes.h>
#include <ui/DisplayedFrameStats.h>
#include <ui/FrameStats.h>
@@ -34,6 +31,11 @@
#include <ui/GraphicTypes.h>
#include <ui/PixelFormat.h>
+#include <utils/Errors.h>
+#include <utils/RefBase.h>
+#include <utils/Timers.h>
+#include <utils/Vector.h>
+
#include <optional>
#include <vector>
@@ -133,7 +135,8 @@
const sp<IBinder>& applyToken,
const InputWindowCommands& inputWindowCommands,
int64_t desiredPresentTime,
- const cached_buffer_t& uncacheBuffer) = 0;
+ const cached_buffer_t& uncacheBuffer,
+ const std::vector<ListenerCallbacks>& listenerCallbacks) = 0;
/* signal that we're done booting.
* Requires ACCESS_SURFACE_FLINGER permission
diff --git a/libs/gui/include/gui/LayerState.h b/libs/gui/include/gui/LayerState.h
index 77bf8f1..2256497 100644
--- a/libs/gui/include/gui/LayerState.h
+++ b/libs/gui/include/gui/LayerState.h
@@ -23,7 +23,6 @@
#include <utils/Errors.h>
#include <gui/IGraphicBufferProducer.h>
-#include <gui/ITransactionCompletedListener.h>
#include <math/mat4.h>
#ifndef NO_INPUT
@@ -86,7 +85,7 @@
eApiChanged = 0x04000000,
eSidebandStreamChanged = 0x08000000,
eColorTransformChanged = 0x10000000,
- eListenerCallbacksChanged = 0x20000000,
+ eHasListenerCallbacksChanged = 0x20000000,
eInputInfoChanged = 0x40000000,
eCornerRadiusChanged = 0x80000000,
eFrameChanged = 0x1'00000000,
@@ -120,6 +119,7 @@
surfaceDamageRegion(),
api(-1),
colorTransform(mat4()),
+ hasListenerCallbacks(false),
bgColorAlpha(0),
bgColorDataspace(ui::Dataspace::UNKNOWN),
colorSpaceAgnostic(false) {
@@ -182,7 +182,7 @@
sp<NativeHandle> sidebandStream;
mat4 colorTransform;
- std::vector<ListenerCallbacks> listenerCallbacks;
+ bool hasListenerCallbacks;
#ifndef NO_INPUT
InputWindowInfo inputInfo;
#endif
diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp
index 94b669d..0ee9bff 100644
--- a/libs/gui/tests/Surface_test.cpp
+++ b/libs/gui/tests/Surface_test.cpp
@@ -561,7 +561,9 @@
const sp<IBinder>& /*applyToken*/,
const InputWindowCommands& /*inputWindowCommands*/,
int64_t /*desiredPresentTime*/,
- const cached_buffer_t& /*cachedBuffer*/) override {}
+ const cached_buffer_t& /*cachedBuffer*/,
+ const std::vector<ListenerCallbacks>& /*listenerCallbacks*/) override {
+ }
void bootFinished() override {}
bool authenticateSurfaceTexture(
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 547f30f..45f3377 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -3509,14 +3509,14 @@
while (!transactionQueue.empty()) {
const auto&
- [states, displays, flags, desiredPresentTime, uncacheBuffer, postTime,
- privileged] = transactionQueue.front();
+ [states, displays, flags, desiredPresentTime, uncacheBuffer, listenerCallbacks,
+ postTime, privileged] = transactionQueue.front();
if (!transactionIsReadyToBeApplied(desiredPresentTime, states)) {
break;
}
applyTransactionState(states, displays, flags, mPendingInputWindowCommands,
- desiredPresentTime, uncacheBuffer, postTime, privileged,
- /*isMainThread*/ true);
+ desiredPresentTime, uncacheBuffer, listenerCallbacks, postTime,
+ privileged, /*isMainThread*/ true);
transactionQueue.pop();
}
@@ -3574,7 +3574,8 @@
const sp<IBinder>& applyToken,
const InputWindowCommands& inputWindowCommands,
int64_t desiredPresentTime,
- const cached_buffer_t& uncacheBuffer) {
+ const cached_buffer_t& uncacheBuffer,
+ const std::vector<ListenerCallbacks>& listenerCallbacks) {
ATRACE_CALL();
const int64_t postTime = systemTime();
@@ -3591,13 +3592,14 @@
if (mTransactionQueues.find(applyToken) != mTransactionQueues.end() ||
!transactionIsReadyToBeApplied(desiredPresentTime, states)) {
mTransactionQueues[applyToken].emplace(states, displays, flags, desiredPresentTime,
- uncacheBuffer, postTime, privileged);
+ uncacheBuffer, listenerCallbacks, postTime,
+ privileged);
setTransactionFlags(eTransactionNeeded);
return;
}
applyTransactionState(states, displays, flags, inputWindowCommands, desiredPresentTime,
- uncacheBuffer, postTime, privileged);
+ uncacheBuffer, listenerCallbacks, postTime, privileged);
}
void SurfaceFlinger::applyTransactionState(const Vector<ComposerState>& states,
@@ -3605,6 +3607,7 @@
const InputWindowCommands& inputWindowCommands,
const int64_t desiredPresentTime,
const cached_buffer_t& uncacheBuffer,
+ const std::vector<ListenerCallbacks>& listenerCallbacks,
const int64_t postTime, bool privileged,
bool isMainThread) {
uint32_t transactionFlags = 0;
@@ -3631,7 +3634,15 @@
uint32_t clientStateFlags = 0;
for (const ComposerState& state : states) {
- clientStateFlags |= setClientStateLocked(state, desiredPresentTime, postTime, privileged);
+ clientStateFlags |= setClientStateLocked(state, desiredPresentTime, listenerCallbacks,
+ postTime, privileged);
+ }
+
+ // In case the client has sent a Transaction that should receive callbacks but without any
+ // SurfaceControls that should be included in the callback, send the listener and callbackIds
+ // to the callback thread so it can send an empty callback
+ for (const auto& [listener, callbackIds] : listenerCallbacks) {
+ mTransactionCompletedThread.addCallback(listener, callbackIds);
}
// If the state doesn't require a traversal and there are callbacks, send them now
if (!(clientStateFlags & eTraversalNeeded)) {
@@ -3752,9 +3763,10 @@
return true;
}
-uint32_t SurfaceFlinger::setClientStateLocked(const ComposerState& composerState,
- int64_t desiredPresentTime, int64_t postTime,
- bool privileged) {
+uint32_t SurfaceFlinger::setClientStateLocked(
+ const ComposerState& composerState, int64_t desiredPresentTime,
+ const std::vector<ListenerCallbacks>& listenerCallbacks, int64_t postTime,
+ bool privileged) {
const layer_state_t& s = composerState.state;
sp<Client> client(static_cast<Client*>(composerState.client.get()));
@@ -3980,9 +3992,9 @@
}
}
std::vector<sp<CallbackHandle>> callbackHandles;
- if ((what & layer_state_t::eListenerCallbacksChanged) && (!s.listenerCallbacks.empty())) {
+ if ((what & layer_state_t::eHasListenerCallbacksChanged) && (!listenerCallbacks.empty())) {
mTransactionCompletedThread.run();
- for (const auto& [listener, callbackIds] : s.listenerCallbacks) {
+ for (const auto& [listener, callbackIds] : listenerCallbacks) {
callbackHandles.emplace_back(new CallbackHandle(listener, callbackIds, s.surface));
}
}
@@ -4243,7 +4255,7 @@
d.width = 0;
d.height = 0;
displays.add(d);
- setTransactionState(state, displays, 0, nullptr, mPendingInputWindowCommands, -1, {});
+ setTransactionState(state, displays, 0, nullptr, mPendingInputWindowCommands, -1, {}, {});
setPowerModeInternal(display, HWC_POWER_MODE_NORMAL);
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 03146d6..b4095e3 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -431,8 +431,8 @@
const Vector<DisplayState>& displays, uint32_t flags,
const sp<IBinder>& applyToken,
const InputWindowCommands& inputWindowCommands,
- int64_t desiredPresentTime,
- const cached_buffer_t& uncacheBuffer) override;
+ int64_t desiredPresentTime, const cached_buffer_t& uncacheBuffer,
+ const std::vector<ListenerCallbacks>& listenerCallbacks) override;
void bootFinished() override;
bool authenticateSurfaceTexture(
const sp<IGraphicBufferProducer>& bufferProducer) const override;
@@ -579,8 +579,10 @@
const Vector<DisplayState>& displays, uint32_t flags,
const InputWindowCommands& inputWindowCommands,
const int64_t desiredPresentTime,
- const cached_buffer_t& uncacheBuffer, const int64_t postTime,
- bool privileged, bool isMainThread = false) REQUIRES(mStateLock);
+ const cached_buffer_t& uncacheBuffer,
+ const std::vector<ListenerCallbacks>& listenerCallbacks,
+ const int64_t postTime, bool privileged, bool isMainThread = false)
+ REQUIRES(mStateLock);
bool flushTransactionQueues();
uint32_t getTransactionFlags(uint32_t flags);
uint32_t peekTransactionFlags();
@@ -593,6 +595,7 @@
bool transactionIsReadyToBeApplied(int64_t desiredPresentTime,
const Vector<ComposerState>& states);
uint32_t setClientStateLocked(const ComposerState& composerState, int64_t desiredPresentTime,
+ const std::vector<ListenerCallbacks>& listenerCallbacks,
int64_t postTime, bool privileged) REQUIRES(mStateLock);
uint32_t setDisplayStateLocked(const DisplayState& s) REQUIRES(mStateLock);
uint32_t addInputWindowCommands(const InputWindowCommands& inputWindowCommands)
@@ -1061,12 +1064,14 @@
TransactionState(const Vector<ComposerState>& composerStates,
const Vector<DisplayState>& displayStates, uint32_t transactionFlags,
int64_t desiredPresentTime, const cached_buffer_t& uncacheBuffer,
- int64_t postTime, bool privileged)
+ const std::vector<ListenerCallbacks>& listenerCallbacks, int64_t postTime,
+ bool privileged)
: states(composerStates),
displays(displayStates),
flags(transactionFlags),
desiredPresentTime(desiredPresentTime),
buffer(uncacheBuffer),
+ callback(listenerCallbacks),
postTime(postTime),
privileged(privileged) {}
@@ -1075,6 +1080,7 @@
uint32_t flags;
const int64_t desiredPresentTime;
cached_buffer_t buffer;
+ std::vector<ListenerCallbacks> callback;
const int64_t postTime;
bool privileged;
};
diff --git a/services/surfaceflinger/TransactionCompletedThread.cpp b/services/surfaceflinger/TransactionCompletedThread.cpp
index d2b7fe0..6e0c1e2 100644
--- a/services/surfaceflinger/TransactionCompletedThread.cpp
+++ b/services/surfaceflinger/TransactionCompletedThread.cpp
@@ -100,26 +100,48 @@
addCallbackHandle(handle);
}
-void TransactionCompletedThread::addCallbackHandle(const sp<CallbackHandle>& handle) {
- const sp<IBinder> listener = IInterface::asBinder(handle->listener);
+void TransactionCompletedThread::addCallback(
+ const sp<ITransactionCompletedListener>& transactionListener,
+ const std::vector<CallbackId>& callbackIds) {
+ std::lock_guard lock(mMutex);
+ addCallbackLocked(transactionListener, callbackIds);
+}
+status_t TransactionCompletedThread::addCallbackHandle(const sp<CallbackHandle>& handle) {
+ status_t err = addCallbackLocked(handle->listener, handle->callbackIds);
+ if (err != NO_ERROR) {
+ ALOGE("cannot add callback, err: %d", err);
+ return err;
+ }
+
+ const sp<IBinder> listener = IInterface::asBinder(handle->listener);
+ auto& listenerStats = mListenerStats[listener];
+ auto& transactionStats = listenerStats.transactionStats[handle->callbackIds];
+
+ transactionStats.latchTime = handle->latchTime;
+ transactionStats.surfaceStats.emplace_back(handle->surfaceControl, handle->acquireTime,
+ handle->previousReleaseFence);
+ return NO_ERROR;
+}
+
+status_t TransactionCompletedThread::addCallbackLocked(
+ const sp<ITransactionCompletedListener>& transactionListener,
+ const std::vector<CallbackId>& callbackIds) {
+ const sp<IBinder> listener = IInterface::asBinder(transactionListener);
// If we don't already have a reference to this listener, linkToDeath so we get a notification
// if it dies.
if (mListenerStats.count(listener) == 0) {
- status_t error = listener->linkToDeath(mDeathRecipient);
- if (error != NO_ERROR) {
- ALOGE("cannot add callback handle because linkToDeath failed, err: %d", error);
- return;
+ status_t err = listener->linkToDeath(mDeathRecipient);
+ if (err != NO_ERROR) {
+ ALOGE("cannot add callback because linkToDeath failed, err: %d", err);
+ return err;
}
}
auto& listenerStats = mListenerStats[listener];
- listenerStats.listener = handle->listener;
-
- auto& transactionStats = listenerStats.transactionStats[handle->callbackIds];
- transactionStats.latchTime = handle->latchTime;
- transactionStats.surfaceStats.emplace_back(handle->surfaceControl, handle->acquireTime,
- handle->previousReleaseFence);
+ listenerStats.listener = transactionListener;
+ listenerStats.transactionStats[callbackIds];
+ return NO_ERROR;
}
void TransactionCompletedThread::addPresentFence(const sp<Fence>& presentFence) {
diff --git a/services/surfaceflinger/TransactionCompletedThread.h b/services/surfaceflinger/TransactionCompletedThread.h
index f49306d..88808fd 100644
--- a/services/surfaceflinger/TransactionCompletedThread.h
+++ b/services/surfaceflinger/TransactionCompletedThread.h
@@ -64,6 +64,11 @@
// presented this frame.
void addUnpresentedCallbackHandle(const sp<CallbackHandle>& handle);
+ // Adds listener and callbackIds in case there are no SurfaceControls that are supposed
+ // to be included in the callback.
+ void addCallback(const sp<ITransactionCompletedListener>& transactionListener,
+ const std::vector<CallbackId>& callbackIds);
+
void addPresentFence(const sp<Fence>& presentFence);
void sendCallbacks();
@@ -71,7 +76,9 @@
private:
void threadMain();
- void addCallbackHandle(const sp<CallbackHandle>& handle) REQUIRES(mMutex);
+ status_t addCallbackHandle(const sp<CallbackHandle>& handle) REQUIRES(mMutex);
+ status_t addCallbackLocked(const sp<ITransactionCompletedListener>& transactionListener,
+ const std::vector<CallbackId>& callbackIds) REQUIRES(mMutex);
class ThreadDeathRecipient : public IBinder::DeathRecipient {
public: