BLASTBufferQueue: Correct deadlocks in mNextTransaction handling.
There are two deadlocks in mNextTransaction handling that we adress
here. The first relates to the wait-condition. Imagine we submit a buffer,
now enable mNextTransaction, and queue our next buffer. In onFrameAvailable
mNumAcquired will be 1 (MAX_ACQUIRED_BUFFERS), and so we will wait on the condition
variable. However, we've only ever submitted one buffer, and so no callback
will ever come, and we deadlock. Instead the condition should be mNumAcquired+1
meaning that we have submitted two buffers and are waiting for a callback on
one of them (capturing the original intent of waiting for the callback).
The second relates to the handling of mNextTransaction in
transactionCallback. Imagine a scenario like this:
1. Submit a buffer, mNumAcquired = 1
2. Submit a buffer, callback for the first doesn't arrive yet, mNumAcquired = 2
3. Submit a third buffer, mNumAcquired > MAX_ACQUIRED + 1, so mAvailable = 1
and we wait for the callback.
4. Enable mNextTransaction
5. The callback arrives, but because of the !mNextTransaction condition in
the callback function, we don't call processNextBuffer. We do however
release one:
mNumAcquired = 1, mAvailable = 1
6. At this point we queue a new buffer (The one that was just released we
dequeue and queue again). But, mAvailable = 1, and so we wait on
the condition variable. However the callback has already fired,
and so nothing will actually process the available buffer
and we wait forever.
The intent of this code was to avoid using the mNextTransaction when we
are processing buffers from the callback function. We capture this intent
directly by using a boolean set from onFrameAvailable before calling
processNextBuffer()
Bug: 146598493
Bug: 149251083
Bug: 149315421
Test: Flip the flag. Play.
Change-Id: I652eacf1a016f8b65fd754e47d468227bf8ecf1d
diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp
index f6493bf..7a8ee8e 100644
--- a/libs/gui/BLASTBufferQueue.cpp
+++ b/libs/gui/BLASTBufferQueue.cpp
@@ -174,9 +174,9 @@
}
mPendingReleaseItem.item = std::move(mSubmitted.front());
mSubmitted.pop();
- if (mNextTransaction == nullptr) {
- processNextBufferLocked();
- }
+
+ processNextBufferLocked();
+
mCallbackCV.notify_all();
decStrong((void*)transactionCallbackThunk);
}
@@ -195,7 +195,7 @@
SurfaceComposerClient::Transaction localTransaction;
bool applyTransaction = true;
SurfaceComposerClient::Transaction* t = &localTransaction;
- if (mNextTransaction != nullptr) {
+ if (mNextTransaction != nullptr && mUseNextTransaction) {
t = mNextTransaction;
mNextTransaction = nullptr;
applyTransaction = false;
@@ -257,9 +257,10 @@
std::unique_lock _lock{mMutex};
if (mNextTransaction != nullptr) {
- while (mNumFrameAvailable > 0 || mNumAcquired == MAX_ACQUIRED_BUFFERS) {
+ while (mNumFrameAvailable > 0 || mNumAcquired == MAX_ACQUIRED_BUFFERS + 1) {
mCallbackCV.wait(_lock);
}
+ mUseNextTransaction = true;
}
// add to shadow queue
mNumFrameAvailable++;
@@ -268,6 +269,7 @@
void BLASTBufferQueue::setNextTransaction(SurfaceComposerClient::Transaction* t) {
std::lock_guard _lock{mMutex};
+ mUseNextTransaction = false;
mNextTransaction = t;
}