From a04cda9986366ab480ad8008c4d923271b05d78e Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Wed, 10 Aug 2011 15:28:58 -0700 Subject: error out when SurfaceTexture APIs are called while not connected - also log a warning when freeAllBuffers is called with a non empty buffer queue - rename freeAllBuffers to freeAllBuffersLocked Change-Id: Idb71fdcf233b9ccae62d5a2a7c3c4bad2501d877 --- libs/gui/SurfaceTexture.cpp | 42 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 36 insertions(+), 6 deletions(-) (limited to 'libs/gui/SurfaceTexture.cpp') diff --git a/libs/gui/SurfaceTexture.cpp b/libs/gui/SurfaceTexture.cpp index 4bbf7ebcf6f5..806fbb1be8e1 100644 --- a/libs/gui/SurfaceTexture.cpp +++ b/libs/gui/SurfaceTexture.cpp @@ -104,7 +104,7 @@ SurfaceTexture::SurfaceTexture(GLuint tex, bool allowSynchronousMode) : SurfaceTexture::~SurfaceTexture() { LOGV("SurfaceTexture::~SurfaceTexture"); - freeAllBuffers(); + freeAllBuffersLocked(); } status_t SurfaceTexture::setBufferCountServerLocked(int bufferCount) { @@ -154,6 +154,10 @@ status_t SurfaceTexture::setBufferCount(int bufferCount) { LOGE("setBufferCount: SurfaceTexture has been abandoned!"); return NO_INIT; } + if (mConnectedApi == NO_CONNECTED_API) { + LOGE("setBufferCount: SurfaceTexture is not connected!"); + return NO_INIT; + } if (bufferCount > NUM_BUFFER_SLOTS) { LOGE("setBufferCount: bufferCount larger than slots available"); @@ -185,7 +189,7 @@ status_t SurfaceTexture::setBufferCount(int bufferCount) { // here we're guaranteed that the client doesn't have dequeued buffers // and will release all of its buffer references. - freeAllBuffers(); + freeAllBuffersLocked(); mBufferCount = bufferCount; mClientBufferCount = bufferCount; mCurrentTexture = INVALID_BUFFER_SLOT; @@ -214,6 +218,10 @@ status_t SurfaceTexture::requestBuffer(int slot, sp* buf) { LOGE("requestBuffer: SurfaceTexture has been abandoned!"); return NO_INIT; } + if (mConnectedApi == NO_CONNECTED_API) { + LOGE("requestBuffer: SurfaceTexture is not connected!"); + return NO_INIT; + } if (slot < 0 || mBufferCount <= slot) { LOGE("requestBuffer: slot index out of range [0, %d]: %d", mBufferCount, slot); @@ -232,6 +240,10 @@ status_t SurfaceTexture::dequeueBuffer(int *outBuf, uint32_t w, uint32_t h, LOGE("dequeueBuffer: SurfaceTexture has been abandoned!"); return NO_INIT; } + if (mConnectedApi == NO_CONNECTED_API) { + LOGE("dequeueBuffer: SurfaceTexture is not connected!"); + return NO_INIT; + } if ((w && !h) || (!w && h)) { LOGE("dequeueBuffer: invalid size: w=%u, h=%u", w, h); @@ -285,7 +297,7 @@ status_t SurfaceTexture::dequeueBuffer(int *outBuf, uint32_t w, uint32_t h, ((mServerBufferCount != mBufferCount) || (mServerBufferCount < minBufferCountNeeded))) { // here we're guaranteed that mQueue is empty - freeAllBuffers(); + freeAllBuffersLocked(); mBufferCount = mServerBufferCount; if (mBufferCount < minBufferCountNeeded) mBufferCount = minBufferCountNeeded; @@ -442,6 +454,10 @@ status_t SurfaceTexture::queueBuffer(int buf, int64_t timestamp, LOGE("queueBuffer: SurfaceTexture has been abandoned!"); return NO_INIT; } + if (mConnectedApi == NO_CONNECTED_API) { + LOGE("queueBuffer: SurfaceTexture is not connected!"); + return NO_INIT; + } if (buf < 0 || buf >= mBufferCount) { LOGE("queueBuffer: slot index out of range [0, %d]: %d", mBufferCount, buf); @@ -512,6 +528,10 @@ void SurfaceTexture::cancelBuffer(int buf) { LOGW("cancelBuffer: SurfaceTexture has been abandoned!"); return; } + if (mConnectedApi == NO_CONNECTED_API) { + LOGE("cancelBuffer: SurfaceTexture is not connected!"); + return; + } if (buf < 0 || buf >= mBufferCount) { LOGE("cancelBuffer: slot index out of range [0, %d]: %d", @@ -533,6 +553,10 @@ status_t SurfaceTexture::setCrop(const Rect& crop) { LOGE("setCrop: SurfaceTexture has been abandoned!"); return NO_INIT; } + if (mConnectedApi == NO_CONNECTED_API) { + LOGE("setCrop: SurfaceTexture is not connected!"); + return NO_INIT; + } mNextCrop = crop; return OK; } @@ -544,6 +568,10 @@ status_t SurfaceTexture::setTransform(uint32_t transform) { LOGE("setTransform: SurfaceTexture has been abandoned!"); return NO_INIT; } + if (mConnectedApi == NO_CONNECTED_API) { + LOGE("setTransform: SurfaceTexture is not connected!"); + return NO_INIT; + } mNextTransform = transform; return OK; } @@ -599,7 +627,7 @@ status_t SurfaceTexture::disconnect(int api) { case NATIVE_WINDOW_API_CAMERA: if (mConnectedApi == api) { mConnectedApi = NO_CONNECTED_API; - freeAllBuffers(); + freeAllBuffersLocked(); } else { LOGE("disconnect: connected to another api (cur=%d, req=%d)", mConnectedApi, api); @@ -848,7 +876,9 @@ void SurfaceTexture::setFrameAvailableListener( mFrameAvailableListener = listener; } -void SurfaceTexture::freeAllBuffers() { +void SurfaceTexture::freeAllBuffersLocked() { + LOGW_IF(!mQueue.isEmpty(), + "freeAllBuffersLocked called but mQueue is not empty"); for (int i = 0; i < NUM_BUFFER_SLOTS; i++) { mSlots[i].mGraphicBuffer = 0; mSlots[i].mBufferState = BufferSlot::FREE; @@ -929,7 +959,7 @@ int SurfaceTexture::query(int what, int* outValue) void SurfaceTexture::abandon() { Mutex::Autolock lock(mMutex); - freeAllBuffers(); + freeAllBuffersLocked(); mAbandoned = true; mCurrentTextureBuf.clear(); mDequeueCondition.signal(); -- cgit v1.2.3-59-g8ed1b From 71fd1213b49e6a33bea42348876eb1db2ab3d362 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Wed, 10 Aug 2011 16:33:23 -0700 Subject: rework dequeueBuffer()'s main loop. this simplifies the code a bit and also makes sure we reevaluate mAbandoned and mConnectedApi each time we come back from waiting on mDequeueCondition Change-Id: I1f8538b62ad321b51ed79d953b700036daba796d --- include/gui/SurfaceTexture.h | 5 ++++ libs/gui/SurfaceTexture.cpp | 66 +++++++++++++++++++++++--------------------- 2 files changed, 39 insertions(+), 32 deletions(-) (limited to 'libs/gui/SurfaceTexture.cpp') diff --git a/include/gui/SurfaceTexture.h b/include/gui/SurfaceTexture.h index f93930d43094..71d818c1f3c5 100644 --- a/include/gui/SurfaceTexture.h +++ b/include/gui/SurfaceTexture.h @@ -211,6 +211,11 @@ protected: // freeAllBuffers frees the resources (both GraphicBuffer and EGLImage) for // all slots. void freeAllBuffersLocked(); + + // drainQueueLocked drains the buffer queue if we're in synchronous mode + // returns immediately otherwise. + void drainQueueLocked(); + static bool isExternalFormat(uint32_t format); private: diff --git a/libs/gui/SurfaceTexture.cpp b/libs/gui/SurfaceTexture.cpp index 806fbb1be8e1..631cc1c97857 100644 --- a/libs/gui/SurfaceTexture.cpp +++ b/libs/gui/SurfaceTexture.cpp @@ -158,7 +158,6 @@ status_t SurfaceTexture::setBufferCount(int bufferCount) { LOGE("setBufferCount: SurfaceTexture is not connected!"); return NO_INIT; } - if (bufferCount > NUM_BUFFER_SLOTS) { LOGE("setBufferCount: bufferCount larger than slots available"); return BAD_VALUE; @@ -236,15 +235,6 @@ status_t SurfaceTexture::dequeueBuffer(int *outBuf, uint32_t w, uint32_t h, uint32_t format, uint32_t usage) { LOGV("SurfaceTexture::dequeueBuffer"); - if (mAbandoned) { - LOGE("dequeueBuffer: SurfaceTexture has been abandoned!"); - return NO_INIT; - } - if (mConnectedApi == NO_CONNECTED_API) { - LOGE("dequeueBuffer: SurfaceTexture is not connected!"); - return NO_INIT; - } - if ((w && !h) || (!w && h)) { LOGE("dequeueBuffer: invalid size: w=%u, h=%u", w, h); return BAD_VALUE; @@ -258,10 +248,19 @@ status_t SurfaceTexture::dequeueBuffer(int *outBuf, uint32_t w, uint32_t h, int dequeuedCount = 0; bool tryAgain = true; while (tryAgain) { + if (mAbandoned) { + LOGE("dequeueBuffer: SurfaceTexture has been abandoned!"); + return NO_INIT; + } + if (mConnectedApi == NO_CONNECTED_API) { + LOGE("dequeueBuffer: SurfaceTexture is not connected!"); + return NO_INIT; + } + // We need to wait for the FIFO to drain if the number of buffer // needs to change. // - // The condition "number of buffer needs to change" is true if + // The condition "number of buffers needs to change" is true if // - the client doesn't care about how many buffers there are // - AND the actual number of buffer is different from what was // set in the last setBufferCountServer() @@ -273,29 +272,22 @@ status_t SurfaceTexture::dequeueBuffer(int *outBuf, uint32_t w, uint32_t h, // As long as this condition is true AND the FIFO is not empty, we // wait on mDequeueCondition. - int minBufferCountNeeded = mSynchronousMode ? + const int minBufferCountNeeded = mSynchronousMode ? MIN_SYNC_BUFFER_SLOTS : MIN_ASYNC_BUFFER_SLOTS; - if (!mClientBufferCount && + const bool numberOfBuffersNeedsToChange = !mClientBufferCount && ((mServerBufferCount != mBufferCount) || - (mServerBufferCount < minBufferCountNeeded))) { + (mServerBufferCount < minBufferCountNeeded)); + + if (!mQueue.isEmpty() && numberOfBuffersNeedsToChange) { // wait for the FIFO to drain - while (!mQueue.isEmpty()) { - mDequeueCondition.wait(mMutex); - if (mAbandoned) { - LOGE("dequeueBuffer: SurfaceTexture was abandoned while " - "blocked!"); - return NO_INIT; - } - } - minBufferCountNeeded = mSynchronousMode ? - MIN_SYNC_BUFFER_SLOTS : MIN_ASYNC_BUFFER_SLOTS; + mDequeueCondition.wait(mMutex); + // NOTE: we continue here because we need to reevaluate our + // whole state (eg: we could be abandoned or disconnected) + continue; } - - if (!mClientBufferCount && - ((mServerBufferCount != mBufferCount) || - (mServerBufferCount < minBufferCountNeeded))) { + if (numberOfBuffersNeedsToChange) { // here we're guaranteed that mQueue is empty freeAllBuffersLocked(); mBufferCount = mServerBufferCount; @@ -426,9 +418,7 @@ status_t SurfaceTexture::setSynchronousMode(bool enabled) { if (!enabled) { // going to asynchronous mode, drain the queue - while (mSynchronousMode != enabled && !mQueue.isEmpty()) { - mDequeueCondition.wait(mMutex); - } + drainQueueLocked(); } if (mSynchronousMode != enabled) { @@ -627,7 +617,12 @@ status_t SurfaceTexture::disconnect(int api) { case NATIVE_WINDOW_API_CAMERA: if (mConnectedApi == api) { mConnectedApi = NO_CONNECTED_API; - freeAllBuffersLocked(); + if (mQueue.isEmpty()) { + // if the queue is not empty, we need to wait for it + // to drain before we can free all buffers. This is + // done in updateTexImage(). + freeAllBuffersLocked(); + } } else { LOGE("disconnect: connected to another api (cur=%d, req=%d)", mConnectedApi, api); @@ -890,6 +885,12 @@ void SurfaceTexture::freeAllBuffersLocked() { } } +void SurfaceTexture::drainQueueLocked() { + while (mSynchronousMode && !mQueue.isEmpty()) { + mDequeueCondition.wait(mMutex); + } +} + EGLImageKHR SurfaceTexture::createImage(EGLDisplay dpy, const sp& graphicBuffer) { EGLClientBuffer cbuf = (EGLClientBuffer)graphicBuffer->getNativeBuffer(); @@ -959,6 +960,7 @@ int SurfaceTexture::query(int what, int* outValue) void SurfaceTexture::abandon() { Mutex::Autolock lock(mMutex); + // clear the queue freeAllBuffersLocked(); mAbandoned = true; mCurrentTextureBuf.clear(); -- cgit v1.2.3-59-g8ed1b From 5c71575983e96e6e4c5149e7e39d92f760f5c1fc Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Wed, 10 Aug 2011 17:35:09 -0700 Subject: fix a crasher in SurfaceTexture::updateTexImage() we now make sure to drain the buffer queue on disconnect. this happens only when in synchrnous mode. in async mode we clear all buffers except the head of the queue. for extra safety we also catch the null pointer in updateTexImage (which should never happen) and return an error. Bug: 5111008 Change-Id: I5174a6ecbb0de641c6510ef56a611cbb4e9e1f59 --- include/gui/SurfaceTexture.h | 22 +++++++++--- libs/gui/SurfaceTexture.cpp | 81 ++++++++++++++++++++++++++++++-------------- 2 files changed, 74 insertions(+), 29 deletions(-) (limited to 'libs/gui/SurfaceTexture.cpp') diff --git a/include/gui/SurfaceTexture.h b/include/gui/SurfaceTexture.h index 71d818c1f3c5..a6fb12e29638 100644 --- a/include/gui/SurfaceTexture.h +++ b/include/gui/SurfaceTexture.h @@ -208,13 +208,27 @@ public: protected: - // freeAllBuffers frees the resources (both GraphicBuffer and EGLImage) for - // all slots. + // freeBufferLocked frees the resources (both GraphicBuffer and EGLImage) + // for the given slot. + void freeBufferLocked(int index); + + // freeAllBuffersLocked frees the resources (both GraphicBuffer and + // EGLImage) for all slots. void freeAllBuffersLocked(); + // freeAllBuffersExceptHeadLocked frees the resources (both GraphicBuffer + // and EGLImage) for all slots except the head of mQueue + void freeAllBuffersExceptHeadLocked(); + // drainQueueLocked drains the buffer queue if we're in synchronous mode - // returns immediately otherwise. - void drainQueueLocked(); + // returns immediately otherwise. return NO_INIT if SurfaceTexture + // became abandoned or disconnected during this call. + status_t drainQueueLocked(); + + // drainQueueAndFreeBuffersLocked drains the buffer queue if we're in + // synchronous mode and free all buffers. In asynchronous mode, all buffers + // are freed except the current buffer. + status_t drainQueueAndFreeBuffersLocked(); static bool isExternalFormat(uint32_t format); diff --git a/libs/gui/SurfaceTexture.cpp b/libs/gui/SurfaceTexture.cpp index 631cc1c97857..4afca68849b8 100644 --- a/libs/gui/SurfaceTexture.cpp +++ b/libs/gui/SurfaceTexture.cpp @@ -418,7 +418,9 @@ status_t SurfaceTexture::setSynchronousMode(bool enabled) { if (!enabled) { // going to asynchronous mode, drain the queue - drainQueueLocked(); + err = drainQueueLocked(); + if (err != NO_ERROR) + return err; } if (mSynchronousMode != enabled) { @@ -616,13 +618,9 @@ status_t SurfaceTexture::disconnect(int api) { case NATIVE_WINDOW_API_MEDIA: case NATIVE_WINDOW_API_CAMERA: if (mConnectedApi == api) { + drainQueueAndFreeBuffersLocked(); mConnectedApi = NO_CONNECTED_API; - if (mQueue.isEmpty()) { - // if the queue is not empty, we need to wait for it - // to drain before we can free all buffers. This is - // done in updateTexImage(). - freeAllBuffersLocked(); - } + mDequeueCondition.signal(); } else { LOGE("disconnect: connected to another api (cur=%d, req=%d)", mConnectedApi, api); @@ -658,7 +656,7 @@ status_t SurfaceTexture::updateTexImage() { if (mAbandoned) { LOGE("calling updateTexImage() on an abandoned SurfaceTexture"); - //return NO_INIT; + return NO_INIT; } // In asynchronous mode the list is guaranteed to be one buffer @@ -667,21 +665,14 @@ status_t SurfaceTexture::updateTexImage() { Fifo::iterator front(mQueue.begin()); int buf = *front; - if (uint32_t(buf) >= NUM_BUFFER_SLOTS) { - LOGE("buffer index out of range (index=%d)", buf); - //return BAD_VALUE; - } - // Update the GL texture object. EGLImageKHR image = mSlots[buf].mEglImage; if (image == EGL_NO_IMAGE_KHR) { EGLDisplay dpy = eglGetCurrentDisplay(); - if (mSlots[buf].mGraphicBuffer == 0) { LOGE("buffer at slot %d is null", buf); - //return BAD_VALUE; + return BAD_VALUE; } - image = createImage(dpy, mSlots[buf].mGraphicBuffer); mSlots[buf].mEglImage = image; mSlots[buf].mEglDisplay = dpy; @@ -871,24 +862,64 @@ void SurfaceTexture::setFrameAvailableListener( mFrameAvailableListener = listener; } +void SurfaceTexture::freeBufferLocked(int i) { + mSlots[i].mGraphicBuffer = 0; + mSlots[i].mBufferState = BufferSlot::FREE; + if (mSlots[i].mEglImage != EGL_NO_IMAGE_KHR) { + eglDestroyImageKHR(mSlots[i].mEglDisplay, mSlots[i].mEglImage); + mSlots[i].mEglImage = EGL_NO_IMAGE_KHR; + mSlots[i].mEglDisplay = EGL_NO_DISPLAY; + } +} + void SurfaceTexture::freeAllBuffersLocked() { LOGW_IF(!mQueue.isEmpty(), "freeAllBuffersLocked called but mQueue is not empty"); for (int i = 0; i < NUM_BUFFER_SLOTS; i++) { - mSlots[i].mGraphicBuffer = 0; - mSlots[i].mBufferState = BufferSlot::FREE; - if (mSlots[i].mEglImage != EGL_NO_IMAGE_KHR) { - eglDestroyImageKHR(mSlots[i].mEglDisplay, mSlots[i].mEglImage); - mSlots[i].mEglImage = EGL_NO_IMAGE_KHR; - mSlots[i].mEglDisplay = EGL_NO_DISPLAY; + freeBufferLocked(i); + } +} + +void SurfaceTexture::freeAllBuffersExceptHeadLocked() { + LOGW_IF(!mQueue.isEmpty(), + "freeAllBuffersExceptCurrentLocked called but mQueue is not empty"); + int head = -1; + if (!mQueue.empty()) { + Fifo::iterator front(mQueue.begin()); + head = *front; + } + for (int i = 0; i < NUM_BUFFER_SLOTS; i++) { + if (i != head) { + freeBufferLocked(i); } } } -void SurfaceTexture::drainQueueLocked() { +status_t SurfaceTexture::drainQueueLocked() { while (mSynchronousMode && !mQueue.isEmpty()) { mDequeueCondition.wait(mMutex); + if (mAbandoned) { + LOGE("drainQueueLocked: SurfaceTexture has been abandoned!"); + return NO_INIT; + } + if (mConnectedApi == NO_CONNECTED_API) { + LOGE("drainQueueLocked: SurfaceTexture is not connected!"); + return NO_INIT; + } } + return NO_ERROR; +} + +status_t SurfaceTexture::drainQueueAndFreeBuffersLocked() { + status_t err = drainQueueLocked(); + if (err == NO_ERROR) { + if (mSynchronousMode) { + freeAllBuffersLocked(); + } else { + freeAllBuffersExceptHeadLocked(); + } + } + return err; } EGLImageKHR SurfaceTexture::createImage(EGLDisplay dpy, @@ -960,10 +991,10 @@ int SurfaceTexture::query(int what, int* outValue) void SurfaceTexture::abandon() { Mutex::Autolock lock(mMutex); - // clear the queue - freeAllBuffersLocked(); + mQueue.clear(); mAbandoned = true; mCurrentTextureBuf.clear(); + freeAllBuffersLocked(); mDequeueCondition.signal(); } -- cgit v1.2.3-59-g8ed1b