Call release buffer if the buffer is overwritten in the client
Clients can overwrite the buffer in a transaction before it's applied.
When this happens, the buffer is then replaced, but the client doesn't
get a callback that the buffer was released. This could cause the client
to run out of buffers since the dropped ones become lost.
This can happen in two situations:
1. Merging two transactions where each has a buffer for the same
SurfaceControl
2. Transaction.setBuffer is called multiple times before an apply
In both cases, call the release callback so the client can properly
handle the dropped buffers.
It's also possible that the merging occurs in a different process than
the one that set the buffer. Because of that, we need to ensure we call
the correct releaseBufferListener that's associated with the one that
set the buffer.
The transformHint is removed from the releaseCallback since it's already
provided in the transaction callback. It was originally added in the
release callback to give it to BBQ early so it can know the new
transform hint before rendering a new frame. However, the transform hint
is now provided by VRI to BBQ so it no longer needs to be sent back via
release callback.
Test: ReleaseBufferCallbackTest
Change-Id: I9df0c713bf841f53e1a3d092f33179573a680dc4
Bug: 200285149
diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp
index 2b17616..36de581 100644
--- a/libs/gui/BLASTBufferQueue.cpp
+++ b/libs/gui/BLASTBufferQueue.cpp
@@ -162,6 +162,7 @@
ComposerService::getComposerService()->getMaxAcquiredBufferCount(&mMaxAcquiredBuffers);
mBufferItemConsumer->setMaxAcquiredBufferCount(mMaxAcquiredBuffers);
+ mCurrentMaxAcquiredBufferCount = mMaxAcquiredBuffers;
mTransformHint = mSurfaceControl->getTransformHint();
mBufferItemConsumer->setTransformHint(mTransformHint);
@@ -325,31 +326,23 @@
// So we pass in a weak pointer to the BBQ and if it still alive, then we release the buffer.
// Otherwise, this is a no-op.
static void releaseBufferCallbackThunk(wp<BLASTBufferQueue> context, const ReleaseCallbackId& id,
- const sp<Fence>& releaseFence, uint32_t transformHint,
- uint32_t currentMaxAcquiredBufferCount) {
+ const sp<Fence>& releaseFence,
+ std::optional<uint32_t> currentMaxAcquiredBufferCount) {
sp<BLASTBufferQueue> blastBufferQueue = context.promote();
if (blastBufferQueue) {
- blastBufferQueue->releaseBufferCallback(id, releaseFence, transformHint,
- currentMaxAcquiredBufferCount);
+ blastBufferQueue->releaseBufferCallback(id, releaseFence, currentMaxAcquiredBufferCount);
} else {
ALOGV("releaseBufferCallbackThunk %s blastBufferQueue is dead", id.to_string().c_str());
}
}
-void BLASTBufferQueue::releaseBufferCallback(const ReleaseCallbackId& id,
- const sp<Fence>& releaseFence, uint32_t transformHint,
- uint32_t currentMaxAcquiredBufferCount) {
+void BLASTBufferQueue::releaseBufferCallback(
+ const ReleaseCallbackId& id, const sp<Fence>& releaseFence,
+ std::optional<uint32_t> currentMaxAcquiredBufferCount) {
ATRACE_CALL();
std::unique_lock _lock{mMutex};
BQA_LOGV("releaseBufferCallback %s", id.to_string().c_str());
- if (mSurfaceControl != nullptr) {
- mTransformHint = transformHint;
- mSurfaceControl->setTransformHint(transformHint);
- mBufferItemConsumer->setTransformHint(mTransformHint);
- BQA_LOGV("updated mTransformHint=%d", mTransformHint);
- }
-
// Calculate how many buffers we need to hold before we release them back
// to the buffer queue. This will prevent higher latency when we are running
// on a lower refresh rate than the max supported. We only do that for EGL
@@ -359,8 +352,12 @@
return it != mSubmitted.end() && it->second.mApi == NATIVE_WINDOW_API_EGL;
}();
+ if (currentMaxAcquiredBufferCount) {
+ mCurrentMaxAcquiredBufferCount = *currentMaxAcquiredBufferCount;
+ }
+
const auto numPendingBuffersToHold =
- isEGL ? std::max(0u, mMaxAcquiredBuffers - currentMaxAcquiredBufferCount) : 0;
+ isEGL ? std::max(0u, mMaxAcquiredBuffers - mCurrentMaxAcquiredBufferCount) : 0;
mPendingRelease.emplace_back(ReleasedBuffer{id, releaseFence});
// Release all buffers that are beyond the ones that we need to hold
@@ -374,7 +371,7 @@
return;
}
mNumAcquired--;
- BQA_LOGV("released %s", id.to_string().c_str());
+ BQA_LOGV("released %s", releaseBuffer.callbackId.to_string().c_str());
mBufferItemConsumer->releaseBuffer(it->second, releaseBuffer.releaseFence);
mSubmitted.erase(it);
processNextBufferLocked(false /* useNextTransaction */);
@@ -466,8 +463,7 @@
auto releaseBufferCallback =
std::bind(releaseBufferCallbackThunk, wp<BLASTBufferQueue>(this) /* callbackContext */,
- std::placeholders::_1, std::placeholders::_2, std::placeholders::_3,
- std::placeholders::_4);
+ std::placeholders::_1, std::placeholders::_2, std::placeholders::_3);
sp<Fence> fence = bufferItem.mFence ? new Fence(bufferItem.mFence->dup()) : Fence::NO_FENCE;
t->setBuffer(mSurfaceControl, buffer, fence, bufferItem.mFrameNumber, releaseCallbackId,
releaseBufferCallback);
diff --git a/libs/gui/ITransactionCompletedListener.cpp b/libs/gui/ITransactionCompletedListener.cpp
index 98e8b54..aa7ebc9 100644
--- a/libs/gui/ITransactionCompletedListener.cpp
+++ b/libs/gui/ITransactionCompletedListener.cpp
@@ -254,11 +254,10 @@
}
void onReleaseBuffer(ReleaseCallbackId callbackId, sp<Fence> releaseFence,
- uint32_t transformHint, uint32_t currentMaxAcquiredBufferCount) override {
+ uint32_t currentMaxAcquiredBufferCount) override {
callRemoteAsync<decltype(
&ITransactionCompletedListener::onReleaseBuffer)>(Tag::ON_RELEASE_BUFFER,
callbackId, releaseFence,
- transformHint,
currentMaxAcquiredBufferCount);
}
};
diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp
index aca59b6..2713be0 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -214,12 +214,6 @@
mReleaseBufferCallbacks[callbackId] = listener;
}
-void TransactionCompletedListener::removeReleaseBufferCallback(
- const ReleaseCallbackId& callbackId) {
- std::scoped_lock<std::mutex> lock(mMutex);
- mReleaseBufferCallbacks.erase(callbackId);
-}
-
void TransactionCompletedListener::addSurfaceStatsListener(void* context, void* cookie,
sp<SurfaceControl> surfaceControl, SurfaceStatsCallback listener) {
std::scoped_lock<std::recursive_mutex> lock(mSurfaceStatsListenerMutex);
@@ -343,7 +337,6 @@
surfaceStats.previousReleaseFence
? surfaceStats.previousReleaseFence
: Fence::NO_FENCE,
- surfaceStats.transformHint,
surfaceStats.currentMaxAcquiredBufferCount);
}
}
@@ -389,7 +382,7 @@
}
void TransactionCompletedListener::onReleaseBuffer(ReleaseCallbackId callbackId,
- sp<Fence> releaseFence, uint32_t transformHint,
+ sp<Fence> releaseFence,
uint32_t currentMaxAcquiredBufferCount) {
ReleaseBufferCallback callback;
{
@@ -401,7 +394,11 @@
callbackId.to_string().c_str());
return;
}
- callback(callbackId, releaseFence, transformHint, currentMaxAcquiredBufferCount);
+ std::optional<uint32_t> optionalMaxAcquiredBufferCount =
+ currentMaxAcquiredBufferCount == UINT_MAX
+ ? std::nullopt
+ : std::make_optional<uint32_t>(currentMaxAcquiredBufferCount);
+ callback(callbackId, releaseFence, optionalMaxAcquiredBufferCount);
}
ReleaseBufferCallback TransactionCompletedListener::popReleaseBufferCallbackLocked(
@@ -712,11 +709,34 @@
return NO_ERROR;
}
+void SurfaceComposerClient::Transaction::releaseBufferIfOverwriting(const layer_state_t& state) {
+ if (!(state.what & layer_state_t::eBufferChanged)) {
+ return;
+ }
+
+ auto listener = state.bufferData.releaseBufferListener;
+ sp<Fence> fence =
+ state.bufferData.acquireFence ? state.bufferData.acquireFence : Fence::NO_FENCE;
+ if (state.bufferData.releaseBufferEndpoint ==
+ IInterface::asBinder(TransactionCompletedListener::getIInstance())) {
+ // if the callback is in process, run on a different thread to avoid any lock contigency
+ // issues in the client.
+ SurfaceComposerClient::getDefault()
+ ->mReleaseCallbackThread.addReleaseCallback(state.bufferData.releaseCallbackId,
+ fence);
+ } else {
+ listener->onReleaseBuffer(state.bufferData.releaseCallbackId, fence, UINT_MAX);
+ }
+}
+
SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::merge(Transaction&& other) {
for (auto const& [handle, composerState] : other.mComposerStates) {
if (mComposerStates.count(handle) == 0) {
mComposerStates[handle] = composerState;
} else {
+ if (composerState.state.what & layer_state_t::eBufferChanged) {
+ releaseBufferIfOverwriting(mComposerStates[handle].state);
+ }
mComposerStates[handle].state.merge(composerState.state);
}
}
@@ -1296,7 +1316,9 @@
mStatus = BAD_INDEX;
return *this;
}
- removeReleaseBufferCallback(s);
+
+ releaseBufferIfOverwriting(*s);
+
BufferData bufferData;
bufferData.buffer = buffer;
if (frameNumber) {
@@ -1321,15 +1343,6 @@
return *this;
}
-void SurfaceComposerClient::Transaction::removeReleaseBufferCallback(layer_state_t* s) {
- if (!(s->what & layer_state_t::eBufferChanged)) {
- return;
- }
-
- auto listener = TransactionCompletedListener::getInstance();
- listener->removeReleaseBufferCallback(s->bufferData.releaseCallbackId);
-}
-
void SurfaceComposerClient::Transaction::setReleaseBufferCallback(BufferData* bufferData,
const ReleaseCallbackId& id,
ReleaseBufferCallback callback) {
@@ -2210,4 +2223,43 @@
return s->captureLayers(captureArgs, captureListener);
}
+// ---------------------------------------------------------------------------------
+
+void ReleaseCallbackThread::addReleaseCallback(const ReleaseCallbackId callbackId,
+ sp<Fence> releaseFence) {
+ std::scoped_lock<std::mutex> lock(mMutex);
+ if (!mStarted) {
+ mThread = std::thread(&ReleaseCallbackThread::threadMain, this);
+ mStarted = true;
+ }
+
+ mCallbackInfos.emplace(callbackId, std::move(releaseFence));
+ mReleaseCallbackPending.notify_one();
+}
+
+void ReleaseCallbackThread::threadMain() {
+ const auto listener = TransactionCompletedListener::getInstance();
+ std::queue<std::tuple<const ReleaseCallbackId, const sp<Fence>>> callbackInfos;
+ while (true) {
+ {
+ std::unique_lock<std::mutex> lock(mMutex);
+ callbackInfos = std::move(mCallbackInfos);
+ mCallbackInfos = {};
+ }
+
+ while (!callbackInfos.empty()) {
+ auto [callbackId, releaseFence] = callbackInfos.front();
+ listener->onReleaseBuffer(callbackId, std::move(releaseFence), UINT_MAX);
+ callbackInfos.pop();
+ }
+
+ {
+ std::unique_lock<std::mutex> lock(mMutex);
+ if (mCallbackInfos.size() == 0) {
+ mReleaseCallbackPending.wait(lock);
+ }
+ }
+ }
+}
+
} // namespace android
diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h
index 1f517f6..49ece6e 100644
--- a/libs/gui/include/gui/BLASTBufferQueue.h
+++ b/libs/gui/include/gui/BLASTBufferQueue.h
@@ -91,7 +91,7 @@
void transactionCallback(nsecs_t latchTime, const sp<Fence>& presentFence,
const std::vector<SurfaceControlStats>& stats);
void releaseBufferCallback(const ReleaseCallbackId& id, const sp<Fence>& releaseFence,
- uint32_t transformHint, uint32_t currentMaxAcquiredBufferCount);
+ std::optional<uint32_t> currentMaxAcquiredBufferCount);
void setNextTransaction(SurfaceComposerClient::Transaction *t);
void mergeWithNextTransaction(SurfaceComposerClient::Transaction* t, uint64_t frameNumber);
void applyPendingTransactions(uint64_t frameNumber);
@@ -236,6 +236,8 @@
// Keep track of SurfaceControls that have submitted a transaction and BBQ is waiting on a
// callback for them.
std::queue<sp<SurfaceControl>> mSurfaceControlsWithPendingCallback GUARDED_BY(mMutex);
+
+ uint32_t mCurrentMaxAcquiredBufferCount;
};
} // namespace android
diff --git a/libs/gui/include/gui/ITransactionCompletedListener.h b/libs/gui/include/gui/ITransactionCompletedListener.h
index 937095c..0df5822 100644
--- a/libs/gui/include/gui/ITransactionCompletedListener.h
+++ b/libs/gui/include/gui/ITransactionCompletedListener.h
@@ -192,7 +192,6 @@
virtual void onTransactionCompleted(ListenerStats stats) = 0;
virtual void onReleaseBuffer(ReleaseCallbackId callbackId, sp<Fence> releaseFence,
- uint32_t transformHint,
uint32_t currentMaxAcquiredBufferCount) = 0;
};
diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h
index 82249a3..e62c76e 100644
--- a/libs/gui/include/gui/SurfaceComposerClient.h
+++ b/libs/gui/include/gui/SurfaceComposerClient.h
@@ -19,6 +19,7 @@
#include <stdint.h>
#include <sys/types.h>
#include <set>
+#include <thread>
#include <unordered_map>
#include <unordered_set>
@@ -84,7 +85,7 @@
const std::vector<SurfaceControlStats>& /*stats*/)>;
using ReleaseBufferCallback =
std::function<void(const ReleaseCallbackId&, const sp<Fence>& /*releaseFence*/,
- uint32_t transformHint, uint32_t currentMaxAcquiredBufferCount)>;
+ std::optional<uint32_t> currentMaxAcquiredBufferCount)>;
using SurfaceStatsCallback =
std::function<void(void* /*context*/, nsecs_t /*latchTime*/,
@@ -93,6 +94,22 @@
// ---------------------------------------------------------------------------
+class ReleaseCallbackThread {
+public:
+ void addReleaseCallback(const ReleaseCallbackId, sp<Fence>);
+ void threadMain();
+
+private:
+ std::thread mThread;
+ std::mutex mMutex;
+ bool mStarted GUARDED_BY(mMutex) = false;
+ std::condition_variable mReleaseCallbackPending;
+ std::queue<std::tuple<const ReleaseCallbackId, const sp<Fence>>> mCallbackInfos
+ GUARDED_BY(mMutex);
+};
+
+// ---------------------------------------------------------------------------
+
class SurfaceComposerClient : public RefBase
{
friend class Composer;
@@ -350,6 +367,7 @@
private:
static std::atomic<uint32_t> idCounter;
int64_t generateId();
+ void releaseBufferIfOverwriting(const layer_state_t& state);
protected:
std::unordered_map<sp<IBinder>, ComposerState, IBinderHash> mComposerStates;
@@ -400,7 +418,6 @@
void cacheBuffers();
void registerSurfaceControlForCallback(const sp<SurfaceControl>& sc);
void setReleaseBufferCallback(BufferData*, const ReleaseCallbackId&, ReleaseBufferCallback);
- void removeReleaseBufferCallback(layer_state_t*);
public:
Transaction();
@@ -625,6 +642,9 @@
status_t addWindowInfosListener(const sp<gui::WindowInfosListener>& windowInfosListener);
status_t removeWindowInfosListener(const sp<gui::WindowInfosListener>& windowInfosListener);
+protected:
+ ReleaseCallbackThread mReleaseCallbackThread;
+
private:
virtual void onFirstRef();
@@ -725,11 +745,10 @@
void removeSurfaceStatsListener(void* context, void* cookie);
void setReleaseBufferCallback(const ReleaseCallbackId&, ReleaseBufferCallback);
- void removeReleaseBufferCallback(const ReleaseCallbackId&);
// BnTransactionCompletedListener overrides
void onTransactionCompleted(ListenerStats stats) override;
- void onReleaseBuffer(ReleaseCallbackId, sp<Fence> releaseFence, uint32_t transformHint,
+ void onReleaseBuffer(ReleaseCallbackId, sp<Fence> releaseFence,
uint32_t currentMaxAcquiredBufferCount) override;
// For Testing Only