libgui: fix a race in CpuConsumer::lockNextBuffer
mCurrentLockedBuffers must be accessed with the mutex. Constify
mMaxLockedBuffers as well for clarity.
Test: libgui_test
Change-Id: I7c40d135bcf2aa6a90ecff74d6d8107fc530e815
diff --git a/libs/gui/CpuConsumer.cpp b/libs/gui/CpuConsumer.cpp
index f909e3b..e5e02b4 100644
--- a/libs/gui/CpuConsumer.cpp
+++ b/libs/gui/CpuConsumer.cpp
@@ -168,6 +168,9 @@
status_t err;
if (!nativeBuffer) return BAD_VALUE;
+
+ Mutex::Autolock _l(mMutex);
+
if (mCurrentLockedBuffers == mMaxLockedBuffers) {
CC_LOGW("Max buffers have been locked (%zd), cannot lock anymore.",
mMaxLockedBuffers);
@@ -175,9 +178,6 @@
}
BufferItem b;
-
- Mutex::Autolock _l(mMutex);
-
err = acquireBufferLocked(&b, 0);
if (err != OK) {
if (err == BufferQueue::NO_BUFFER_AVAILABLE) {
diff --git a/libs/gui/include/gui/CpuConsumer.h b/libs/gui/include/gui/CpuConsumer.h
index 14cfc1a..dda16a5 100644
--- a/libs/gui/include/gui/CpuConsumer.h
+++ b/libs/gui/include/gui/CpuConsumer.h
@@ -119,7 +119,7 @@
private:
// Maximum number of buffers that can be locked at a time
- size_t mMaxLockedBuffers;
+ const size_t mMaxLockedBuffers;
status_t releaseAcquiredBufferLocked(size_t lockedIdx);
diff --git a/libs/gui/tests/CpuConsumer_test.cpp b/libs/gui/tests/CpuConsumer_test.cpp
index 4ef82d8..36be7d9 100644
--- a/libs/gui/tests/CpuConsumer_test.cpp
+++ b/libs/gui/tests/CpuConsumer_test.cpp
@@ -33,6 +33,7 @@
#include <utils/Mutex.h>
#include <utils/Condition.h>
+#include <thread>
#include <vector>
#define CPU_CONSUMER_TEST_FORMAT_RAW 0
#define CPU_CONSUMER_TEST_FORMAT_Y8 0
@@ -690,6 +691,61 @@
ASSERT_EQ(BAD_VALUE, err) << "unlockBuffer did not fail";
}
+TEST_P(CpuConsumerTest, FromCpuMultiThread) {
+ CpuConsumerTestParams params = GetParam();
+ ASSERT_NO_FATAL_FAILURE(configureANW(mANW, params, params.maxLockedBuffers + 1));
+
+ for (int i = 0; i < 10; i++) {
+ std::atomic<int> threadReadyCount(0);
+ auto lockAndUnlock = [&]() {
+ threadReadyCount++;
+ // busy wait
+ while (threadReadyCount < params.maxLockedBuffers + 1);
+
+ CpuConsumer::LockedBuffer b;
+ status_t err = mCC->lockNextBuffer(&b);
+ if (err == NO_ERROR) {
+ usleep(1000);
+ err = mCC->unlockBuffer(b);
+ ASSERT_NO_ERROR(err, "Could not unlock buffer: ");
+ } else if (err == NOT_ENOUGH_DATA) {
+ // there are params.maxLockedBuffers+1 threads so one of the
+ // threads might get this error
+ } else {
+ FAIL() << "Could not lock buffer";
+ }
+ };
+
+ // produce buffers
+ for (int j = 0; j < params.maxLockedBuffers + 1; j++) {
+ const int64_t time = 1234L;
+ uint32_t stride;
+ ASSERT_NO_FATAL_FAILURE(produceOneFrame(mANW, params, time, &stride));
+ }
+
+ // spawn threads
+ std::vector<std::thread> threads;
+ for (int j = 0; j < params.maxLockedBuffers + 1; j++) {
+ threads.push_back(std::thread(lockAndUnlock));
+ }
+
+ // join threads
+ for (auto& thread : threads) {
+ thread.join();
+ }
+
+ // we produced N+1 buffers, but the threads might only consume N
+ CpuConsumer::LockedBuffer b;
+ if (mCC->lockNextBuffer(&b) == NO_ERROR) {
+ mCC->unlockBuffer(b);
+ }
+
+ if (HasFatalFailure()) {
+ break;
+ }
+ }
+}
+
CpuConsumerTestParams y8TestSets[] = {
{ 512, 512, 1, HAL_PIXEL_FORMAT_Y8},
{ 512, 512, 3, HAL_PIXEL_FORMAT_Y8},