Fixed disconnect bug in SurfaceTexture
BufferQueue's disconnect could race with updateTexImage
where invalid buffers could be released. Additionally
fixed similar bug with setBufferCount. Tests were added
to stress the disconnect mechanism.
Change-Id: I9afa4c64f3e025984e8a9e8d924852a71d044716
diff --git a/include/gui/BufferQueue.h b/include/gui/BufferQueue.h
index a808216..57b9f8a 100644
--- a/include/gui/BufferQueue.h
+++ b/include/gui/BufferQueue.h
@@ -42,6 +42,7 @@
enum { NUM_BUFFER_SLOTS = 32 };
enum { NO_CONNECTED_API = 0 };
enum { INVALID_BUFFER_SLOT = -1 };
+ enum { STALE_BUFFER_SLOT = 1 };
// ConsumerListener is the interface through which the BufferQueue notifies
// the consumer of events that the consumer may wish to react to. Because
@@ -297,7 +298,8 @@
mTimestamp(0),
mFrameNumber(0),
mFence(EGL_NO_SYNC_KHR),
- mAcquireCalled(false) {
+ mAcquireCalled(false),
+ mNeedsCleanupOnRelease(false) {
mCrop.makeInvalid();
}
@@ -376,6 +378,9 @@
// Indicates whether this buffer has been seen by a consumer yet
bool mAcquireCalled;
+
+ // Indicates whether this buffer needs to be cleaned up by consumer
+ bool mNeedsCleanupOnRelease;
};
// mSlots is the array of buffer slots that must be mirrored on the client
diff --git a/include/gui/SurfaceTexture.h b/include/gui/SurfaceTexture.h
index 496cfba..3f5e4df 100644
--- a/include/gui/SurfaceTexture.h
+++ b/include/gui/SurfaceTexture.h
@@ -185,7 +185,6 @@
status_t setConsumerUsageBits(uint32_t usage);
status_t setTransformHint(uint32_t hint);
virtual status_t setSynchronousMode(bool enabled);
- virtual status_t setBufferCount(int bufferCount);
virtual status_t connect(int api,
uint32_t* outWidth, uint32_t* outHeight, uint32_t* outTransform);
diff --git a/libs/gui/BufferQueue.cpp b/libs/gui/BufferQueue.cpp
index 1762a4a..2d042c8 100644
--- a/libs/gui/BufferQueue.cpp
+++ b/libs/gui/BufferQueue.cpp
@@ -163,48 +163,58 @@
status_t BufferQueue::setBufferCount(int bufferCount) {
ST_LOGV("setBufferCount: count=%d", bufferCount);
- Mutex::Autolock lock(mMutex);
- if (mAbandoned) {
- ST_LOGE("setBufferCount: SurfaceTexture has been abandoned!");
- return NO_INIT;
- }
- if (bufferCount > NUM_BUFFER_SLOTS) {
- ST_LOGE("setBufferCount: bufferCount larger than slots available");
- return BAD_VALUE;
- }
+ sp<ConsumerListener> listener;
+ {
+ Mutex::Autolock lock(mMutex);
- // Error out if the user has dequeued buffers
- for (int i=0 ; i<mBufferCount ; i++) {
- if (mSlots[i].mBufferState == BufferSlot::DEQUEUED) {
- ST_LOGE("setBufferCount: client owns some buffers");
- return -EINVAL;
+ if (mAbandoned) {
+ ST_LOGE("setBufferCount: SurfaceTexture has been abandoned!");
+ return NO_INIT;
}
+ if (bufferCount > NUM_BUFFER_SLOTS) {
+ ST_LOGE("setBufferCount: bufferCount larger than slots available");
+ return BAD_VALUE;
+ }
+
+ // Error out if the user has dequeued buffers
+ for (int i=0 ; i<mBufferCount ; i++) {
+ if (mSlots[i].mBufferState == BufferSlot::DEQUEUED) {
+ ST_LOGE("setBufferCount: client owns some buffers");
+ return -EINVAL;
+ }
+ }
+
+ const int minBufferSlots = mSynchronousMode ?
+ MIN_SYNC_BUFFER_SLOTS : MIN_ASYNC_BUFFER_SLOTS;
+ if (bufferCount == 0) {
+ mClientBufferCount = 0;
+ bufferCount = (mServerBufferCount >= minBufferSlots) ?
+ mServerBufferCount : minBufferSlots;
+ return setBufferCountServerLocked(bufferCount);
+ }
+
+ if (bufferCount < minBufferSlots) {
+ ST_LOGE("setBufferCount: requested buffer count (%d) is less than "
+ "minimum (%d)", bufferCount, minBufferSlots);
+ return BAD_VALUE;
+ }
+
+ // here we're guaranteed that the client doesn't have dequeued buffers
+ // and will release all of its buffer references.
+ freeAllBuffersLocked();
+ mBufferCount = bufferCount;
+ mClientBufferCount = bufferCount;
+ mBufferHasBeenQueued = false;
+ mQueue.clear();
+ mDequeueCondition.broadcast();
+ listener = mConsumerListener;
+ } // scope for lock
+
+ if (listener != NULL) {
+ listener->onBuffersReleased();
}
- const int minBufferSlots = mSynchronousMode ?
- MIN_SYNC_BUFFER_SLOTS : MIN_ASYNC_BUFFER_SLOTS;
- if (bufferCount == 0) {
- mClientBufferCount = 0;
- bufferCount = (mServerBufferCount >= minBufferSlots) ?
- mServerBufferCount : minBufferSlots;
- return setBufferCountServerLocked(bufferCount);
- }
-
- if (bufferCount < minBufferSlots) {
- ST_LOGE("setBufferCount: requested buffer count (%d) is less than "
- "minimum (%d)", bufferCount, minBufferSlots);
- return BAD_VALUE;
- }
-
- // here we're guaranteed that the client doesn't have dequeued buffers
- // and will release all of its buffer references.
- freeAllBuffersLocked();
- mBufferCount = bufferCount;
- mClientBufferCount = bufferCount;
- mBufferHasBeenQueued = false;
- mQueue.clear();
- mDequeueCondition.broadcast();
return OK;
}
@@ -780,6 +790,9 @@
void BufferQueue::freeBufferLocked(int i) {
mSlots[i].mGraphicBuffer = 0;
+ if (mSlots[i].mBufferState == BufferSlot::ACQUIRED) {
+ mSlots[i].mNeedsCleanupOnRelease = true;
+ }
mSlots[i].mBufferState = BufferSlot::FREE;
mSlots[i].mFrameNumber = 0;
mSlots[i].mAcquireCalled = false;
@@ -853,15 +866,19 @@
mSlots[buf].mEglDisplay = display;
mSlots[buf].mFence = fence;
- // The current buffer becomes FREE if it was still in the queued
- // state. If it has already been given to the client
- // (synchronous mode), then it stays in DEQUEUED state.
- if (mSlots[buf].mBufferState == BufferSlot::QUEUED
- || mSlots[buf].mBufferState == BufferSlot::ACQUIRED) {
+ // The buffer can now only be released if its in the acquired state
+ if (mSlots[buf].mBufferState == BufferSlot::ACQUIRED) {
mSlots[buf].mBufferState = BufferSlot::FREE;
+ } else if (mSlots[buf].mNeedsCleanupOnRelease) {
+ ST_LOGV("releasing a stale buf %d its state was %d", buf, mSlots[buf].mBufferState);
+ mSlots[buf].mNeedsCleanupOnRelease = false;
+ return STALE_BUFFER_SLOT;
+ } else {
+ ST_LOGE("attempted to release buf %d but its state was %d", buf, mSlots[buf].mBufferState);
+ return -EINVAL;
}
- mDequeueCondition.broadcast();
+ mDequeueCondition.broadcast();
return OK;
}
diff --git a/libs/gui/SurfaceTexture.cpp b/libs/gui/SurfaceTexture.cpp
index 18c86fa..dd96f84 100644
--- a/libs/gui/SurfaceTexture.cpp
+++ b/libs/gui/SurfaceTexture.cpp
@@ -176,6 +176,8 @@
ST_LOGV("updateTexImage");
Mutex::Autolock lock(mMutex);
+ status_t err = NO_ERROR;
+
if (mAbandoned) {
ST_LOGE("updateTexImage: SurfaceTexture is abandoned!");
return NO_INIT;
@@ -269,11 +271,17 @@
mCurrentTextureBuf != NULL ? mCurrentTextureBuf->handle : 0,
buf, item.mGraphicBuffer != NULL ? item.mGraphicBuffer->handle : 0);
- // Release the old buffer
+ // release old buffer
if (mCurrentTexture != BufferQueue::INVALID_BUFFER_SLOT) {
- mBufferQueue->releaseBuffer(mCurrentTexture, dpy,
- mEGLSlots[mCurrentTexture].mFence);
+ status_t status = mBufferQueue->releaseBuffer(mCurrentTexture, dpy, mEGLSlots[mCurrentTexture].mFence);
+
mEGLSlots[mCurrentTexture].mFence = EGL_NO_SYNC_KHR;
+ if (status == BufferQueue::STALE_BUFFER_SLOT) {
+ freeBufferLocked(mCurrentTexture);
+ } else if (status != OK) {
+ ST_LOGE("updateTexImage: released invalid buffer");
+ err = status;
+ }
}
// Update the SurfaceTexture state.
@@ -289,7 +297,7 @@
glBindTexture(mTexTarget, mTexName);
}
- return OK;
+ return err;
}
status_t SurfaceTexture::detachFromContext() {
@@ -630,6 +638,9 @@
void SurfaceTexture::freeBufferLocked(int slotIndex) {
ST_LOGV("freeBufferLocked: slotIndex=%d", slotIndex);
mEGLSlots[slotIndex].mGraphicBuffer = 0;
+ if (slotIndex == mCurrentTexture) {
+ mCurrentTexture = BufferQueue::INVALID_BUFFER_SLOT;
+ }
if (mEGLSlots[slotIndex].mEglImage != EGL_NO_IMAGE_KHR) {
EGLImageKHR img = mEGLSlots[slotIndex].mEglImage;
if (img != EGL_NO_IMAGE_KHR) {
@@ -693,12 +704,6 @@
}
// Used for refactoring, should not be in final interface
-status_t SurfaceTexture::setBufferCount(int bufferCount) {
- Mutex::Autolock lock(mMutex);
- return mBufferQueue->setBufferCount(bufferCount);
-}
-
-// Used for refactoring, should not be in final interface
status_t SurfaceTexture::connect(int api,
uint32_t* outWidth, uint32_t* outHeight, uint32_t* outTransform) {
Mutex::Autolock lock(mMutex);
@@ -737,8 +742,6 @@
freeBufferLocked(i);
}
}
-
- mCurrentTexture = BufferQueue::INVALID_BUFFER_SLOT;
}
void SurfaceTexture::dump(String8& result) const
diff --git a/libs/gui/tests/SurfaceTexture_test.cpp b/libs/gui/tests/SurfaceTexture_test.cpp
index bf347c6..3009093 100644
--- a/libs/gui/tests/SurfaceTexture_test.cpp
+++ b/libs/gui/tests/SurfaceTexture_test.cpp
@@ -132,7 +132,7 @@
}
virtual void TearDown() {
- // Display the result
+ // Display the result
if (mDisplaySecs > 0 && mEglSurface != EGL_NO_SURFACE) {
eglSwapBuffers(mEglDisplay, mEglSurface);
sleep(mDisplaySecs);
@@ -486,6 +486,55 @@
Condition mCondition;
};
+ // Note that SurfaceTexture will lose the notifications
+ // onBuffersReleased and onFrameAvailable as there is currently
+ // no way to forward the events. This DisconnectWaiter will not let the
+ // disconnect finish until finishDisconnect() is called. It will
+ // also block until a disconnect is called
+ class DisconnectWaiter : public BufferQueue::ConsumerListener {
+ public:
+ DisconnectWaiter () :
+ mWaitForDisconnect(false),
+ mPendingFrames(0) {
+ }
+
+ void waitForFrame() {
+ Mutex::Autolock lock(mMutex);
+ while (mPendingFrames == 0) {
+ mFrameCondition.wait(mMutex);
+ }
+ mPendingFrames--;
+ }
+
+ virtual void onFrameAvailable() {
+ Mutex::Autolock lock(mMutex);
+ mPendingFrames++;
+ mFrameCondition.signal();
+ }
+
+ virtual void onBuffersReleased() {
+ Mutex::Autolock lock(mMutex);
+ while (!mWaitForDisconnect) {
+ mDisconnectCondition.wait(mMutex);
+ }
+ }
+
+ void finishDisconnect() {
+ Mutex::Autolock lock(mMutex);
+ mWaitForDisconnect = true;
+ mDisconnectCondition.signal();
+ }
+
+ private:
+ Mutex mMutex;
+
+ bool mWaitForDisconnect;
+ Condition mDisconnectCondition;
+
+ int mPendingFrames;
+ Condition mFrameCondition;
+ };
+
sp<SurfaceTexture> mST;
sp<SurfaceTextureClient> mSTC;
sp<ANativeWindow> mANW;
@@ -984,6 +1033,102 @@
EXPECT_TRUE(checkPixel( 3, 52, 35, 231, 35, 35));
}
+// Tests if SurfaceTexture and BufferQueue are robust enough
+// to handle a special case where updateTexImage is called
+// in the middle of disconnect. This ordering is enforced
+// by blocking in the disconnect callback.
+TEST_F(SurfaceTextureGLTest, DisconnectStressTest) {
+
+ class ProducerThread : public Thread {
+ public:
+ ProducerThread(const sp<ANativeWindow>& anw):
+ mANW(anw) {
+ }
+
+ virtual ~ProducerThread() {
+ }
+
+ virtual bool threadLoop() {
+ ANativeWindowBuffer* anb;
+
+ native_window_api_connect(mANW.get(), NATIVE_WINDOW_API_EGL);
+
+ for (int numFrames =0 ; numFrames < 2; numFrames ++) {
+
+ if (mANW->dequeueBuffer(mANW.get(), &anb) != NO_ERROR) {
+ return false;
+ }
+ if (anb == NULL) {
+ return false;
+ }
+ if (mANW->queueBuffer(mANW.get(), anb)
+ != NO_ERROR) {
+ return false;
+ }
+ }
+
+ native_window_api_disconnect(mANW.get(), NATIVE_WINDOW_API_EGL);
+
+ return false;
+ }
+
+ private:
+ sp<ANativeWindow> mANW;
+ };
+
+ ASSERT_EQ(OK, mST->setSynchronousMode(true));
+
+ sp<DisconnectWaiter> dw(new DisconnectWaiter());
+ mST->getBufferQueue()->consumerConnect(dw);
+
+
+ sp<Thread> pt(new ProducerThread(mANW));
+ pt->run();
+
+ // eat a frame so SurfaceTexture will own an at least one slot
+ dw->waitForFrame();
+ EXPECT_EQ(OK,mST->updateTexImage());
+
+ dw->waitForFrame();
+ // Could fail here as SurfaceTexture thinks it still owns the slot
+ // but bufferQueue has released all slots
+ EXPECT_EQ(OK,mST->updateTexImage());
+
+ dw->finishDisconnect();
+}
+
+
+// This test ensures that the SurfaceTexture clears the mCurrentTexture
+// when it is disconnected and reconnected. Otherwise it will
+// attempt to release a buffer that it does not owned
+TEST_F(SurfaceTextureGLTest, DisconnectClearsCurrentTexture) {
+ ASSERT_EQ(OK, mST->setSynchronousMode(true));
+
+ native_window_api_connect(mANW.get(), NATIVE_WINDOW_API_EGL);
+
+ ANativeWindowBuffer *anb;
+
+ EXPECT_EQ (OK, mANW->dequeueBuffer(mANW.get(), &anb));
+ EXPECT_EQ(OK, mANW->queueBuffer(mANW.get(), anb));
+
+ EXPECT_EQ (OK, mANW->dequeueBuffer(mANW.get(), &anb));
+ EXPECT_EQ(OK, mANW->queueBuffer(mANW.get(), anb));
+
+ EXPECT_EQ(OK,mST->updateTexImage());
+ EXPECT_EQ(OK,mST->updateTexImage());
+
+ native_window_api_disconnect(mANW.get(), NATIVE_WINDOW_API_EGL);
+ native_window_api_connect(mANW.get(), NATIVE_WINDOW_API_EGL);
+
+ ASSERT_EQ(OK, mST->setSynchronousMode(true));
+
+ EXPECT_EQ (OK, mANW->dequeueBuffer(mANW.get(), &anb));
+ EXPECT_EQ(OK, mANW->queueBuffer(mANW.get(), anb));
+
+ // Will fail here if mCurrentTexture is not cleared properly
+ EXPECT_EQ(OK,mST->updateTexImage());
+}
+
TEST_F(SurfaceTextureGLTest, AbandonUnblocksDequeueBuffer) {
class ProducerThread : public Thread {
public: