From 9abe1ebc9575dc5a19bf1dfce6e9b02e03374457 Mon Sep 17 00:00:00 2001 From: Daniel Lam Date: Mon, 26 Mar 2012 20:37:15 -0700 Subject: [PATCH] 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 --- include/gui/BufferQueue.h | 7 +- include/gui/SurfaceTexture.h | 1 - libs/gui/BufferQueue.cpp | 103 +++++++++-------- libs/gui/SurfaceTexture.cpp | 27 +++-- libs/gui/tests/SurfaceTexture_test.cpp | 147 ++++++++++++++++++++++++- 5 files changed, 227 insertions(+), 58 deletions(-) diff --git a/include/gui/BufferQueue.h b/include/gui/BufferQueue.h index a8082164f..57b9f8a33 100644 --- a/include/gui/BufferQueue.h +++ b/include/gui/BufferQueue.h @@ -42,6 +42,7 @@ public: 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 @@ private: mTimestamp(0), mFrameNumber(0), mFence(EGL_NO_SYNC_KHR), - mAcquireCalled(false) { + mAcquireCalled(false), + mNeedsCleanupOnRelease(false) { mCrop.makeInvalid(); } @@ -376,6 +378,9 @@ private: // 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 496cfba03..3f5e4df43 100644 --- a/include/gui/SurfaceTexture.h +++ b/include/gui/SurfaceTexture.h @@ -185,7 +185,6 @@ public: 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 1762a4a1c..2d042c83c 100644 --- a/libs/gui/BufferQueue.cpp +++ b/libs/gui/BufferQueue.cpp @@ -163,48 +163,58 @@ status_t BufferQueue::setTransformHint(uint32_t hint) { 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 listener; + { + Mutex::Autolock lock(mMutex); - // Error out if the user has dequeued buffers - for (int i=0 ; i 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= 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::dump(String8& result, const char* prefix, 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 @@ status_t BufferQueue::releaseBuffer(int buf, EGLDisplay display, 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 18c86fab5..dd96f8417 100644 --- a/libs/gui/SurfaceTexture.cpp +++ b/libs/gui/SurfaceTexture.cpp @@ -176,6 +176,8 @@ status_t SurfaceTexture::updateTexImage() { 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 @@ status_t SurfaceTexture::updateTexImage() { 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 @@ status_t SurfaceTexture::updateTexImage() { glBindTexture(mTexTarget, mTexName); } - return OK; + return err; } status_t SurfaceTexture::detachFromContext() { @@ -630,6 +638,9 @@ bool SurfaceTexture::isSynchronousMode() const { 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) { @@ -692,12 +703,6 @@ sp SurfaceTexture::getBufferQueue() const { return mBufferQueue; } -// 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) { @@ -737,8 +742,6 @@ void SurfaceTexture::onBuffersReleased() { 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 bf347c608..30090930d 100644 --- a/libs/gui/tests/SurfaceTexture_test.cpp +++ b/libs/gui/tests/SurfaceTexture_test.cpp @@ -132,7 +132,7 @@ protected: } 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 @@ protected: 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 mST; sp mSTC; sp mANW; @@ -984,6 +1033,102 @@ TEST_F(SurfaceTextureGLTest, TexturingFromCpuFilledRGBABufferPow2) { 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& 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 mANW; + }; + + ASSERT_EQ(OK, mST->setSynchronousMode(true)); + + sp dw(new DisconnectWaiter()); + mST->getBufferQueue()->consumerConnect(dw); + + + sp 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: