Cache and uncache buffers in the same transaction
This removes a race condition where clients may attempt to cache a
buffer before an associated uncache buffer request has completed,
leading to full buffer cache errors in SurfaceFlinger.
GLSurfaceViewTest#testPauseResumeWithoutDelay is failing on barbet and
test logs show frequent `ClientCache::add - cache is full` messages.
This CL fixes the "cache is full" error but does not fix the broken
test.
This reverts commit 1088bbf9882d778079eaa7465c1dec2b08848f1a.
Reason for revert: b/269141832
Change-Id: I4ba8cf18a367ae6ca94992aabd695d8984fea76f
diff --git a/libs/gui/ISurfaceComposer.cpp b/libs/gui/ISurfaceComposer.cpp
index a77ca04..cefb9a7 100644
--- a/libs/gui/ISurfaceComposer.cpp
+++ b/libs/gui/ISurfaceComposer.cpp
@@ -63,7 +63,8 @@
Vector<ComposerState>& state, const Vector<DisplayState>& displays,
uint32_t flags, const sp<IBinder>& applyToken,
const InputWindowCommands& commands, int64_t desiredPresentTime,
- bool isAutoTimestamp, const client_cache_t& uncacheBuffer,
+ bool isAutoTimestamp,
+ const std::vector<client_cache_t>& uncacheBuffers,
bool hasListenerCallbacks,
const std::vector<ListenerCallbacks>& listenerCallbacks,
uint64_t transactionId) override {
@@ -87,8 +88,11 @@
SAFE_PARCEL(commands.write, data);
SAFE_PARCEL(data.writeInt64, desiredPresentTime);
SAFE_PARCEL(data.writeBool, isAutoTimestamp);
- SAFE_PARCEL(data.writeStrongBinder, uncacheBuffer.token.promote());
- SAFE_PARCEL(data.writeUint64, uncacheBuffer.id);
+ SAFE_PARCEL(data.writeUint32, static_cast<uint32_t>(uncacheBuffers.size()));
+ for (const client_cache_t& uncacheBuffer : uncacheBuffers) {
+ SAFE_PARCEL(data.writeStrongBinder, uncacheBuffer.token.promote());
+ SAFE_PARCEL(data.writeUint64, uncacheBuffer.id);
+ }
SAFE_PARCEL(data.writeBool, hasListenerCallbacks);
SAFE_PARCEL(data.writeVectorSize, listenerCallbacks);
@@ -158,11 +162,14 @@
SAFE_PARCEL(data.readInt64, &desiredPresentTime);
SAFE_PARCEL(data.readBool, &isAutoTimestamp);
- client_cache_t uncachedBuffer;
+ SAFE_PARCEL_READ_SIZE(data.readUint32, &count, data.dataSize());
+ std::vector<client_cache_t> uncacheBuffers(count);
sp<IBinder> tmpBinder;
- SAFE_PARCEL(data.readNullableStrongBinder, &tmpBinder);
- uncachedBuffer.token = tmpBinder;
- SAFE_PARCEL(data.readUint64, &uncachedBuffer.id);
+ for (size_t i = 0; i < count; i++) {
+ SAFE_PARCEL(data.readNullableStrongBinder, &tmpBinder);
+ uncacheBuffers[i].token = tmpBinder;
+ SAFE_PARCEL(data.readUint64, &uncacheBuffers[i].id);
+ }
bool hasListenerCallbacks = false;
SAFE_PARCEL(data.readBool, &hasListenerCallbacks);
@@ -182,7 +189,7 @@
return setTransactionState(frameTimelineInfo, state, displays, stateFlags, applyToken,
inputWindowCommands, desiredPresentTime, isAutoTimestamp,
- uncachedBuffer, hasListenerCallbacks, listenerCallbacks,
+ uncacheBuffers, hasListenerCallbacks, listenerCallbacks,
transactionId);
}
default: {
diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp
index 92125ea..21a7f78 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -565,11 +565,13 @@
return NO_ERROR;
}
- uint64_t cache(const sp<GraphicBuffer>& buffer) {
+ uint64_t cache(const sp<GraphicBuffer>& buffer,
+ std::optional<client_cache_t>& outUncacheBuffer) {
std::lock_guard<std::mutex> lock(mMutex);
if (mBuffers.size() >= BUFFER_CACHE_MAX_SIZE) {
- evictLeastRecentlyUsedBuffer();
+ outUncacheBuffer = findLeastRecentlyUsedBuffer();
+ mBuffers.erase(outUncacheBuffer->id);
}
buffer->addDeathCallback(removeDeadBufferCallback, nullptr);
@@ -580,16 +582,13 @@
void uncache(uint64_t cacheId) {
std::lock_guard<std::mutex> lock(mMutex);
- uncacheLocked(cacheId);
- }
-
- void uncacheLocked(uint64_t cacheId) REQUIRES(mMutex) {
- mBuffers.erase(cacheId);
- SurfaceComposerClient::doUncacheBufferTransaction(cacheId);
+ if (mBuffers.erase(cacheId)) {
+ SurfaceComposerClient::doUncacheBufferTransaction(cacheId);
+ }
}
private:
- void evictLeastRecentlyUsedBuffer() REQUIRES(mMutex) {
+ client_cache_t findLeastRecentlyUsedBuffer() REQUIRES(mMutex) {
auto itr = mBuffers.begin();
uint64_t minCounter = itr->second;
auto minBuffer = itr;
@@ -603,7 +602,8 @@
}
itr++;
}
- uncacheLocked(minBuffer->first);
+
+ return {.token = getToken(), .id = minBuffer->first};
}
uint64_t getCounter() REQUIRES(mMutex) {
@@ -741,6 +741,18 @@
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);
+ }
+
// Parsing was successful. Update the object.
mId = transactionId;
mTransactionNestCount = transactionNestCount;
@@ -755,6 +767,7 @@
mComposerStates = composerStates;
mInputWindowCommands = inputWindowCommands;
mApplyToken = applyToken;
+ mUncacheBuffers = std::move(uncacheBuffers);
return NO_ERROR;
}
@@ -806,6 +819,13 @@
}
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);
+ }
+
return NO_ERROR;
}
@@ -873,6 +893,10 @@
}
}
+ for (const auto& cacheId : other.mUncacheBuffers) {
+ mUncacheBuffers.push_back(cacheId);
+ }
+
mInputWindowCommands.merge(other.mInputWindowCommands);
mMayContainBuffer |= other.mMayContainBuffer;
@@ -891,6 +915,7 @@
mDisplayStates.clear();
mListenerCallbacks.clear();
mInputWindowCommands.clear();
+ mUncacheBuffers.clear();
mMayContainBuffer = false;
mTransactionNestCount = 0;
mAnimation = false;
@@ -913,10 +938,10 @@
uncacheBuffer.token = BufferCache::getInstance().getToken();
uncacheBuffer.id = cacheId;
Vector<ComposerState> composerStates;
- status_t status =
- sf->setTransactionState(FrameTimelineInfo{}, composerStates, {},
- ISurfaceComposer::eOneWay, Transaction::getDefaultApplyToken(),
- {}, systemTime(), true, uncacheBuffer, false, {}, generateId());
+ status_t status = sf->setTransactionState(FrameTimelineInfo{}, composerStates, {},
+ ISurfaceComposer::eOneWay,
+ Transaction::getDefaultApplyToken(), {}, systemTime(),
+ true, {uncacheBuffer}, false, {}, generateId());
if (status != NO_ERROR) {
ALOGE_AND_TRACE("SurfaceComposerClient::doUncacheBufferTransaction - %s",
strerror(-status));
@@ -954,7 +979,11 @@
s->bufferData->buffer = nullptr;
} else {
// Cache-miss. Include the buffer and send the new cacheId.
- cacheId = BufferCache::getInstance().cache(s->bufferData->buffer);
+ std::optional<client_cache_t> uncacheBuffer;
+ cacheId = BufferCache::getInstance().cache(s->bufferData->buffer, uncacheBuffer);
+ if (uncacheBuffer) {
+ mUncacheBuffers.push_back(*uncacheBuffer);
+ }
}
s->bufferData->flags |= BufferData::BufferDataChange::cachedBufferChanged;
s->bufferData->cachedBuffer.token = BufferCache::getInstance().getToken();
@@ -1087,8 +1116,7 @@
sp<ISurfaceComposer> sf(ComposerService::getComposerService());
sf->setTransactionState(mFrameTimelineInfo, composerStates, displayStates, flags, applyToken,
mInputWindowCommands, mDesiredPresentTime, mIsAutoTimestamp,
- {} /*uncacheBuffer - only set in doUncacheBufferTransaction*/,
- hasListenerCallbacks, listenerCallbacks, mId);
+ mUncacheBuffers, hasListenerCallbacks, listenerCallbacks, mId);
mId = generateId();
// Clear the current states and flags
diff --git a/libs/gui/include/gui/ISurfaceComposer.h b/libs/gui/include/gui/ISurfaceComposer.h
index 045cc2a..ae56f9f 100644
--- a/libs/gui/include/gui/ISurfaceComposer.h
+++ b/libs/gui/include/gui/ISurfaceComposer.h
@@ -113,8 +113,9 @@
const FrameTimelineInfo& frameTimelineInfo, Vector<ComposerState>& state,
const Vector<DisplayState>& displays, uint32_t flags, const sp<IBinder>& applyToken,
const InputWindowCommands& inputWindowCommands, int64_t desiredPresentTime,
- bool isAutoTimestamp, const client_cache_t& uncacheBuffer, bool hasListenerCallbacks,
- const std::vector<ListenerCallbacks>& listenerCallbacks, uint64_t transactionId) = 0;
+ bool isAutoTimestamp, const std::vector<client_cache_t>& uncacheBuffer,
+ bool hasListenerCallbacks, const std::vector<ListenerCallbacks>& listenerCallbacks,
+ uint64_t transactionId) = 0;
};
// ----------------------------------------------------------------------------
diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h
index 2458a40..0e51dcf 100644
--- a/libs/gui/include/gui/SurfaceComposerClient.h
+++ b/libs/gui/include/gui/SurfaceComposerClient.h
@@ -402,6 +402,7 @@
SortedVector<DisplayState> mDisplayStates;
std::unordered_map<sp<ITransactionCompletedListener>, CallbackInfo, TCLHash>
mListenerCallbacks;
+ std::vector<client_cache_t> mUncacheBuffers;
uint64_t mId;
diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp
index 3014804..32d60cd 100644
--- a/libs/gui/tests/Surface_test.cpp
+++ b/libs/gui/tests/Surface_test.cpp
@@ -701,7 +701,7 @@
const sp<IBinder>& /*applyToken*/,
const InputWindowCommands& /*inputWindowCommands*/,
int64_t /*desiredPresentTime*/, bool /*isAutoTimestamp*/,
- const client_cache_t& /*cachedBuffer*/,
+ const std::vector<client_cache_t>& /*cachedBuffer*/,
bool /*hasListenerCallbacks*/,
const std::vector<ListenerCallbacks>& /*listenerCallbacks*/,
uint64_t /*transactionId*/) override {