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: