Revert^2 "Use TransactionState in SurfaceFlinger."
This reverts commit 4f8b4f371947dce3abece1c0d64af0b5c990e0c2.
Reason for revert: Rolling forward with fix for use-after-move in
SurfaceFlinger::setTransactionState
Flag: EXEMPT refactor
Bug: 385156191
Test: presubmit
Change-Id: I66d45b1e125dcb50cbe7a3cfcefa8eaa02705419
diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp
index 9854274..69ba1d7 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -824,34 +824,24 @@
// ---------------------------------------------------------------------------
SurfaceComposerClient::Transaction::Transaction() {
- mId = generateId();
+ mState.mId = generateId();
mTransactionCompletedListener = TransactionCompletedListener::getInstance();
}
-SurfaceComposerClient::Transaction::Transaction(const Transaction& other)
- : mId(other.mId),
- mFlags(other.mFlags),
- mMayContainBuffer(other.mMayContainBuffer),
- mDesiredPresentTime(other.mDesiredPresentTime),
- mIsAutoTimestamp(other.mIsAutoTimestamp),
- mFrameTimelineInfo(other.mFrameTimelineInfo),
- mApplyToken(other.mApplyToken) {
- mDisplayStates = other.mDisplayStates;
- mComposerStates = other.mComposerStates;
- mInputWindowCommands = other.mInputWindowCommands;
- mListenerCallbacks = other.mListenerCallbacks;
- mTransactionCompletedListener = TransactionCompletedListener::getInstance();
-}
+SurfaceComposerClient::Transaction::Transaction(Transaction&& other)
+ : mTransactionCompletedListener(TransactionCompletedListener::getInstance()),
+ mState(std::move(other.mState)),
+ mListenerCallbacks(std::move(other.mListenerCallbacks)) {}
void SurfaceComposerClient::Transaction::sanitize(int pid, int uid) {
uint32_t permissions = LayerStatePermissions::getTransactionPermissions(pid, uid);
- for (auto& composerState : mComposerStates) {
+ for (auto& composerState : mState.mComposerStates) {
composerState.state.sanitize(permissions);
}
- if (!mInputWindowCommands.empty() &&
+ if (!mState.mInputWindowCommands.empty() &&
(permissions & layer_state_t::Permission::ACCESS_SURFACE_FLINGER) == 0) {
ALOGE("Only privileged callers are allowed to send input commands.");
- mInputWindowCommands.clear();
+ mState.mInputWindowCommands.clear();
}
}
@@ -866,34 +856,13 @@
status_t SurfaceComposerClient::Transaction::readFromParcel(const Parcel* parcel) {
- const uint64_t transactionId = parcel->readUint64();
- const uint32_t flags = parcel->readUint32();
- const int64_t desiredPresentTime = parcel->readInt64();
- const bool isAutoTimestamp = parcel->readBool();
- const bool logCallPoints = parcel->readBool();
- FrameTimelineInfo frameTimelineInfo;
- frameTimelineInfo.readFromParcel(parcel);
+ TransactionState tmpState;
+ SAFE_PARCEL(tmpState.readFromParcel, parcel);
- sp<IBinder> applyToken;
- parcel->readNullableStrongBinder(&applyToken);
size_t count = static_cast<size_t>(parcel->readUint32());
if (count > parcel->dataSize()) {
return BAD_VALUE;
}
- Vector<DisplayState> displayStates;
- displayStates.setCapacity(count);
- for (size_t i = 0; i < count; i++) {
- DisplayState displayState;
- if (displayState.read(*parcel) == BAD_VALUE) {
- return BAD_VALUE;
- }
- displayStates.add(displayState);
- }
-
- count = static_cast<size_t>(parcel->readUint32());
- if (count > parcel->dataSize()) {
- return BAD_VALUE;
- }
std::unordered_map<sp<ITransactionCompletedListener>, CallbackInfo, TCLHash> listenerCallbacks;
listenerCallbacks.reserve(count);
for (size_t i = 0; i < count; i++) {
@@ -919,57 +888,8 @@
}
}
- count = static_cast<size_t>(parcel->readUint32());
- if (count > parcel->dataSize()) {
- return BAD_VALUE;
- }
- Vector<ComposerState> composerStates;
- composerStates.setCapacity(count);
- for (size_t i = 0; i < count; i++) {
- ComposerState composerState;
- if (composerState.read(*parcel) == BAD_VALUE) {
- return BAD_VALUE;
- }
- composerStates.add(composerState);
- }
-
- InputWindowCommands inputWindowCommands;
- inputWindowCommands.read(*parcel);
-
- count = static_cast<size_t>(parcel->readUint32());
- if (count > parcel->dataSize()) {
- return BAD_VALUE;
- }
- std::vector<client_cache_t> uncacheBuffers(count);
- for (size_t i = 0; i < count; i++) {
- sp<IBinder> tmpBinder;
- SAFE_PARCEL(parcel->readStrongBinder, &tmpBinder);
- uncacheBuffers[i].token = tmpBinder;
- SAFE_PARCEL(parcel->readUint64, &uncacheBuffers[i].id);
- }
-
- count = static_cast<size_t>(parcel->readUint32());
- if (count > parcel->dataSize()) {
- return BAD_VALUE;
- }
- std::vector<uint64_t> mergedTransactionIds(count);
- for (size_t i = 0; i < count; i++) {
- SAFE_PARCEL(parcel->readUint64, &mergedTransactionIds[i]);
- }
-
- // Parsing was successful. Update the object.
- mId = transactionId;
- mFlags = flags;
- mDesiredPresentTime = desiredPresentTime;
- mIsAutoTimestamp = isAutoTimestamp;
- mFrameTimelineInfo = frameTimelineInfo;
- mDisplayStates = std::move(displayStates);
- mListenerCallbacks = listenerCallbacks;
- mComposerStates = std::move(composerStates);
- mInputWindowCommands = inputWindowCommands;
- mApplyToken = applyToken;
- mUncacheBuffers = std::move(uncacheBuffers);
- mMergedTransactionIds = std::move(mergedTransactionIds);
+ mState = std::move(tmpState);
+ mListenerCallbacks = std::move(listenerCallbacks);
return NO_ERROR;
}
@@ -987,17 +907,7 @@
const_cast<SurfaceComposerClient::Transaction*>(this)->cacheBuffers();
- parcel->writeUint64(mId);
- parcel->writeUint32(mFlags);
- parcel->writeInt64(mDesiredPresentTime);
- parcel->writeBool(mIsAutoTimestamp);
- parcel->writeBool(mLogCallPoints);
- mFrameTimelineInfo.writeToParcel(parcel);
- parcel->writeStrongBinder(mApplyToken);
- parcel->writeUint32(static_cast<uint32_t>(mDisplayStates.size()));
- for (auto const& displayState : mDisplayStates) {
- displayState.write(*parcel);
- }
+ SAFE_PARCEL(mState.writeToParcel, parcel);
parcel->writeUint32(static_cast<uint32_t>(mListenerCallbacks.size()));
for (auto const& [listener, callbackInfo] : mListenerCallbacks) {
@@ -1012,24 +922,6 @@
}
}
- parcel->writeUint32(static_cast<uint32_t>(mComposerStates.size()));
- for (auto const& composerState : mComposerStates) {
- composerState.write(*parcel);
- }
-
- mInputWindowCommands.write(*parcel);
-
- SAFE_PARCEL(parcel->writeUint32, static_cast<uint32_t>(mUncacheBuffers.size()));
- for (const client_cache_t& uncacheBuffer : mUncacheBuffers) {
- SAFE_PARCEL(parcel->writeStrongBinder, uncacheBuffer.token.promote());
- SAFE_PARCEL(parcel->writeUint64, uncacheBuffer.id);
- }
-
- SAFE_PARCEL(parcel->writeUint32, static_cast<uint32_t>(mMergedTransactionIds.size()));
- for (auto mergedTransactionId : mMergedTransactionIds) {
- SAFE_PARCEL(parcel->writeUint64, mergedTransactionId);
- }
-
return NO_ERROR;
}
@@ -1054,50 +946,8 @@
}
SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::merge(Transaction&& other) {
- while (mMergedTransactionIds.size() + other.mMergedTransactionIds.size() >
- MAX_MERGE_HISTORY_LENGTH - 1 &&
- mMergedTransactionIds.size() > 0) {
- mMergedTransactionIds.pop_back();
- }
- if (other.mMergedTransactionIds.size() == MAX_MERGE_HISTORY_LENGTH) {
- mMergedTransactionIds.insert(mMergedTransactionIds.begin(),
- other.mMergedTransactionIds.begin(),
- other.mMergedTransactionIds.end() - 1);
- } else if (other.mMergedTransactionIds.size() > 0u) {
- mMergedTransactionIds.insert(mMergedTransactionIds.begin(),
- other.mMergedTransactionIds.begin(),
- other.mMergedTransactionIds.end());
- }
- mMergedTransactionIds.insert(mMergedTransactionIds.begin(), other.mId);
-
- for (auto const& otherState : other.mComposerStates) {
- if (auto it = std::find_if(mComposerStates.begin(), mComposerStates.end(),
- [&otherState](const auto& composerState) {
- return composerState.state.surface ==
- otherState.state.surface;
- });
- it != mComposerStates.end()) {
- if (otherState.state.what & layer_state_t::eBufferChanged) {
- releaseBufferIfOverwriting(it->state);
- }
- it->state.merge(otherState.state);
- } else {
- mComposerStates.add(otherState);
- }
- }
-
- for (auto const& state : other.mDisplayStates) {
- if (auto it = std::find_if(mDisplayStates.begin(), mDisplayStates.end(),
- [&state](const auto& displayState) {
- return displayState.token == state.token;
- });
- it != mDisplayStates.end()) {
- it->merge(state);
- } else {
- mDisplayStates.add(state);
- }
- }
-
+ mState.merge(std::move(other.mState),
+ std::bind(&Transaction::releaseBufferIfOverwriting, this, std::placeholders::_1));
for (const auto& [listener, callbackInfo] : other.mListenerCallbacks) {
auto& [callbackIds, surfaceControls] = callbackInfo;
mListenerCallbacks[listener].callbackIds.insert(std::make_move_iterator(
@@ -1121,50 +971,21 @@
}
}
- for (const auto& cacheId : other.mUncacheBuffers) {
- mUncacheBuffers.push_back(cacheId);
- }
-
- mInputWindowCommands.merge(other.mInputWindowCommands);
-
- mMayContainBuffer |= other.mMayContainBuffer;
- mFlags |= other.mFlags;
- mApplyToken = other.mApplyToken;
-
- mergeFrameTimelineInfo(mFrameTimelineInfo, other.mFrameTimelineInfo);
-
- mLogCallPoints |= other.mLogCallPoints;
- if (mLogCallPoints) {
- ALOG(LOG_DEBUG, LOG_SURFACE_CONTROL_REGISTRY,
- "Transaction %" PRIu64 " merged with transaction %" PRIu64, other.getId(), mId);
- }
-
other.clear();
return *this;
}
void SurfaceComposerClient::Transaction::clear() {
- mComposerStates.clear();
- mDisplayStates.clear();
+ mState.clear();
mListenerCallbacks.clear();
- mInputWindowCommands.clear();
- mUncacheBuffers.clear();
- mMayContainBuffer = false;
- mDesiredPresentTime = 0;
- mIsAutoTimestamp = true;
- mFrameTimelineInfo = {};
- mApplyToken = nullptr;
- mMergedTransactionIds.clear();
- mLogCallPoints = false;
- mFlags = 0;
}
-uint64_t SurfaceComposerClient::Transaction::getId() {
- return mId;
+uint64_t SurfaceComposerClient::Transaction::getId() const {
+ return mState.mId;
}
std::vector<uint64_t> SurfaceComposerClient::Transaction::getMergedTransactionIds() {
- return mMergedTransactionIds;
+ return mState.mMergedTransactionIds;
}
void SurfaceComposerClient::doUncacheBufferTransaction(uint64_t cacheId) {
@@ -1173,12 +994,13 @@
client_cache_t uncacheBuffer;
uncacheBuffer.token = BufferCache::getInstance().getToken();
uncacheBuffer.id = cacheId;
- Vector<ComposerState> composerStates;
- Vector<DisplayState> displayStates;
- status_t status = sf->setTransactionState(FrameTimelineInfo{}, composerStates, displayStates,
- ISurfaceComposer::eOneWay,
- Transaction::getDefaultApplyToken(), {}, systemTime(),
- true, {uncacheBuffer}, false, {}, generateId(), {});
+ TransactionState state;
+ state.mId = generateId();
+ state.mApplyToken = Transaction::getDefaultApplyToken();
+ state.mUncacheBuffers.emplace_back(std::move(uncacheBuffer));
+ state.mFlags = ISurfaceComposer::eOneWay;
+ state.mDesiredPresentTime = systemTime();
+ status_t status = sf->setTransactionState(std::move(state));
if (status != NO_ERROR) {
ALOGE_AND_TRACE("SurfaceComposerClient::doUncacheBufferTransaction - %s",
strerror(-status));
@@ -1186,12 +1008,12 @@
}
void SurfaceComposerClient::Transaction::cacheBuffers() {
- if (!mMayContainBuffer) {
+ if (!mState.mMayContainBuffer) {
return;
}
size_t count = 0;
- for (auto& cs : mComposerStates) {
+ for (auto& cs : mState.mComposerStates) {
layer_state_t* s = &cs.state;
if (!(s->what & layer_state_t::eBufferChanged)) {
continue;
@@ -1219,7 +1041,7 @@
std::optional<client_cache_t> uncacheBuffer;
cacheId = BufferCache::getInstance().cache(s->bufferData->buffer, uncacheBuffer);
if (uncacheBuffer) {
- mUncacheBuffers.push_back(*uncacheBuffer);
+ mState.mUncacheBuffers.emplace_back(*uncacheBuffer);
}
}
s->bufferData->flags |= BufferData::BufferDataChange::cachedBufferChanged;
@@ -1288,8 +1110,7 @@
/*callbackContext=*/nullptr);
}
- bool hasListenerCallbacks = !mListenerCallbacks.empty();
- std::vector<ListenerCallbacks> listenerCallbacks;
+ mState.mHasListenerCallbacks = !mListenerCallbacks.empty();
// For every listener with registered callbacks
for (const auto& [listener, callbackInfo] : mListenerCallbacks) {
auto& [callbackIds, surfaceControls] = callbackInfo;
@@ -1298,7 +1119,8 @@
}
if (surfaceControls.empty()) {
- listenerCallbacks.emplace_back(IInterface::asBinder(listener), std::move(callbackIds));
+ mState.mListenerCallbacks.emplace_back(IInterface::asBinder(listener),
+ std::move(callbackIds));
} else {
// If the listener has any SurfaceControls set on this Transaction update the surface
// state
@@ -1310,7 +1132,7 @@
}
std::vector<CallbackId> callbacks(callbackIds.begin(), callbackIds.end());
s->what |= layer_state_t::eHasListenerCallbacksChanged;
- s->listeners.emplace_back(IInterface::asBinder(listener), callbacks);
+ s->listeners.emplace_back(IInterface::asBinder(listener), std::move(callbacks));
}
}
}
@@ -1322,25 +1144,21 @@
ALOGE("Transaction attempted to set synchronous and one way at the same time"
" this is an invalid request. Synchronous will win for safety");
} else {
- mFlags |= ISurfaceComposer::eOneWay;
+ mState.mFlags |= ISurfaceComposer::eOneWay;
}
}
// If both ISurfaceComposer::eEarlyWakeupStart and ISurfaceComposer::eEarlyWakeupEnd are set
// it is equivalent for none
uint32_t wakeupFlags = ISurfaceComposer::eEarlyWakeupStart | ISurfaceComposer::eEarlyWakeupEnd;
- if ((mFlags & wakeupFlags) == wakeupFlags) {
- mFlags &= ~(wakeupFlags);
+ if ((mState.mFlags & wakeupFlags) == wakeupFlags) {
+ mState.mFlags &= ~(wakeupFlags);
}
- sp<IBinder> applyToken = mApplyToken ? mApplyToken : getDefaultApplyToken();
+ if (!mState.mApplyToken) mState.mApplyToken = getDefaultApplyToken();
sp<ISurfaceComposer> sf(ComposerService::getComposerService());
- status_t binderStatus =
- sf->setTransactionState(mFrameTimelineInfo, mComposerStates, mDisplayStates, mFlags,
- applyToken, mInputWindowCommands, mDesiredPresentTime,
- mIsAutoTimestamp, mUncacheBuffers, hasListenerCallbacks,
- listenerCallbacks, mId, mMergedTransactionIds);
- mId = generateId();
+ status_t binderStatus = sf->setTransactionState(std::move(mState));
+ mState.mId = generateId();
// Clear the current states and flags
clear();
@@ -1349,8 +1167,8 @@
syncCallback->wait();
}
- if (mLogCallPoints) {
- ALOG(LOG_DEBUG, LOG_SURFACE_CONTROL_REGISTRY, "Transaction %" PRIu64 " applied", mId);
+ if (mState.mLogCallPoints) {
+ ALOG(LOG_DEBUG, LOG_SURFACE_CONTROL_REGISTRY, "Transaction %" PRIu64 " applied", getId());
}
mStatus = NO_ERROR;
@@ -1385,7 +1203,7 @@
}
void SurfaceComposerClient::Transaction::enableDebugLogCallPoints() {
- mLogCallPoints = true;
+ mState.mLogCallPoints = true;
}
// ---------------------------------------------------------------------------
@@ -1443,34 +1261,19 @@
}
void SurfaceComposerClient::Transaction::setAnimationTransaction() {
- mFlags |= ISurfaceComposer::eAnimation;
+ mState.mFlags |= ISurfaceComposer::eAnimation;
}
void SurfaceComposerClient::Transaction::setEarlyWakeupStart() {
- mFlags |= ISurfaceComposer::eEarlyWakeupStart;
+ mState.mFlags |= ISurfaceComposer::eEarlyWakeupStart;
}
void SurfaceComposerClient::Transaction::setEarlyWakeupEnd() {
- mFlags |= ISurfaceComposer::eEarlyWakeupEnd;
+ mState.mFlags |= ISurfaceComposer::eEarlyWakeupEnd;
}
layer_state_t* SurfaceComposerClient::Transaction::getLayerState(const sp<SurfaceControl>& sc) {
- auto handle = sc->getLayerStateHandle();
- if (auto it = std::find_if(mComposerStates.begin(), mComposerStates.end(),
- [&handle](const auto& composerState) {
- return composerState.state.surface == handle;
- });
- it != mComposerStates.end()) {
- return &it->state;
- }
-
- // we don't have it, add an initialized layer_state to our list
- ComposerState s;
- s.state.surface = handle;
- s.state.layerId = sc->getLayerId();
- mComposerStates.add(s);
-
- return &mComposerStates.editItemAt(mComposerStates.size() - 1).state;
+ return mState.getLayerState(sc);
}
void SurfaceComposerClient::Transaction::registerSurfaceControlForCallback(
@@ -1846,8 +1649,8 @@
setReleaseBufferCallback(bufferData.get(), callback);
}
- if (mIsAutoTimestamp) {
- mDesiredPresentTime = systemTime();
+ if (mState.mIsAutoTimestamp) {
+ mState.mDesiredPresentTime = systemTime();
}
s->what |= layer_state_t::eBufferChanged;
s->bufferData = std::move(bufferData);
@@ -1865,7 +1668,7 @@
const std::vector<SurfaceControlStats>&) {},
nullptr);
- mMayContainBuffer = true;
+ mState.mMayContainBuffer = true;
return *this;
}
@@ -2041,8 +1844,8 @@
SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setDesiredPresentTime(
nsecs_t desiredPresentTime) {
- mDesiredPresentTime = desiredPresentTime;
- mIsAutoTimestamp = false;
+ mState.mDesiredPresentTime = desiredPresentTime;
+ mState.mIsAutoTimestamp = false;
return *this;
}
@@ -2131,14 +1934,14 @@
SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setFocusedWindow(
const FocusRequest& request) {
- mInputWindowCommands.addFocusRequest(request);
+ mState.mInputWindowCommands.addFocusRequest(request);
return *this;
}
SurfaceComposerClient::Transaction&
SurfaceComposerClient::Transaction::addWindowInfosReportedListener(
sp<gui::IWindowInfosReportedListener> windowInfosReportedListener) {
- mInputWindowCommands.addWindowInfosReportedListener(windowInfosReportedListener);
+ mState.mInputWindowCommands.addWindowInfosReportedListener(windowInfosReportedListener);
return *this;
}
@@ -2302,7 +2105,7 @@
SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setFrameTimelineInfo(
const FrameTimelineInfo& frameTimelineInfo) {
- mergeFrameTimelineInfo(mFrameTimelineInfo, frameTimelineInfo);
+ mState.mergeFrameTimelineInfo(frameTimelineInfo);
return *this;
}
@@ -2341,7 +2144,7 @@
SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setApplyToken(
const sp<IBinder>& applyToken) {
- mApplyToken = applyToken;
+ mState.mApplyToken = applyToken;
return *this;
}
@@ -2469,17 +2272,7 @@
// ---------------------------------------------------------------------------
DisplayState& SurfaceComposerClient::Transaction::getDisplayState(const sp<IBinder>& token) {
- if (auto it = std::find_if(mDisplayStates.begin(), mDisplayStates.end(),
- [token](const auto& display) { return display.token == token; });
- it != mDisplayStates.end()) {
- return *it;
- }
-
- // If display state doesn't exist, add a new one.
- DisplayState s;
- s.token = token;
- mDisplayStates.add(s);
- return mDisplayStates.editItemAt(mDisplayStates.size() - 1);
+ return mState.getDisplayState(token);
}
status_t SurfaceComposerClient::Transaction::setDisplaySurface(const sp<IBinder>& token,
@@ -2532,20 +2325,6 @@
s.what |= DisplayState::eDisplaySizeChanged;
}
-// copied from FrameTimelineInfo::merge()
-void SurfaceComposerClient::Transaction::mergeFrameTimelineInfo(FrameTimelineInfo& t,
- const FrameTimelineInfo& other) {
- // When merging vsync Ids we take the oldest valid one
- if (t.vsyncId != FrameTimelineInfo::INVALID_VSYNC_ID &&
- other.vsyncId != FrameTimelineInfo::INVALID_VSYNC_ID) {
- if (other.vsyncId > t.vsyncId) {
- t = other;
- }
- } else if (t.vsyncId == FrameTimelineInfo::INVALID_VSYNC_ID) {
- t = other;
- }
-}
-
SurfaceComposerClient::Transaction&
SurfaceComposerClient::Transaction::setTrustedPresentationCallback(
const sp<SurfaceControl>& sc, TrustedPresentationCallback cb,