libgui: add BQ consumer buffer free notifications
This change adds a new callback for BufferQueue consumers to be notified
when the BufferQueue frees some or all of its buffers. This is needed
to retain SurfaceTexture behavior where all buffers would be freed when
the producer disconnects. This change also modifies the
SurfaceTextureGLToGLTest.EglDestroySurfaceUnrefsBuffers test to catch
when the buffers are not freed.
The implementation is a little complicated because it needs to avoid
circular sp<> references across what will be a binder interface (so wp<>
can't be used directly). It also needs to avoid the possibility of
locking the BufferQueue and consumer (e.g. SurfaceTexture) mutexes in
the wrong order.
This change also includes a few additional fixes and test cleanups.
Change-Id: I27b77d0af15cb4b135f4b63573f634f5f0da2182
diff --git a/libs/gui/BufferQueue.cpp b/libs/gui/BufferQueue.cpp
index 0f9c7c5..ffc5fa0 100644
--- a/libs/gui/BufferQueue.cpp
+++ b/libs/gui/BufferQueue.cpp
@@ -15,8 +15,8 @@
*/
#define LOG_TAG "BufferQueue"
-//#define LOG_NDEBUG 0
#define ATRACE_TAG ATRACE_TAG_GRAPHICS
+//#define LOG_NDEBUG 0
#define GL_GLEXT_PROTOTYPES
#define EGL_EGLEXT_PROTOTYPES
@@ -146,13 +146,6 @@
mConsumerName = name;
}
-void BufferQueue::setFrameAvailableListener(
- const sp<FrameAvailableListener>& listener) {
- ST_LOGV("setFrameAvailableListener");
- Mutex::Autolock lock(mMutex);
- mFrameAvailableListener = listener;
-}
-
status_t BufferQueue::setDefaultBufferFormat(uint32_t defaultFormat) {
Mutex::Autolock lock(mMutex);
mDefaultBufferFormat = defaultFormat;
@@ -531,7 +524,7 @@
ST_LOGV("queueBuffer: slot=%d time=%lld", buf, timestamp);
- sp<FrameAvailableListener> listener;
+ sp<ConsumerListener> listener;
{ // scope for the lock
Mutex::Autolock lock(mMutex);
@@ -559,7 +552,7 @@
// Synchronous mode always signals that an additional frame should
// be consumed.
- listener = mFrameAvailableListener;
+ listener = mConsumerListener;
} else {
// In asynchronous mode we only keep the most recent buffer.
if (mQueue.empty()) {
@@ -568,7 +561,7 @@
// Asynchronous mode only signals that a frame should be
// consumed if no previous frame was pending. If a frame were
// pending then the consumer would have already been notified.
- listener = mFrameAvailableListener;
+ listener = mConsumerListener;
} else {
Fifo::iterator front(mQueue.begin());
// buffer currently queued is freed
@@ -682,6 +675,11 @@
return NO_INIT;
}
+ if (mConsumerListener == NULL) {
+ ST_LOGE("connect: BufferQueue has no consumer!");
+ return NO_INIT;
+ }
+
int err = NO_ERROR;
switch (api) {
case NATIVE_WINDOW_API_EGL:
@@ -712,38 +710,49 @@
status_t BufferQueue::disconnect(int api) {
ATRACE_CALL();
ST_LOGV("disconnect: api=%d", api);
- Mutex::Autolock lock(mMutex);
-
- if (mAbandoned) {
- // it is not really an error to disconnect after the surface
- // has been abandoned, it should just be a no-op.
- return NO_ERROR;
- }
int err = NO_ERROR;
- switch (api) {
- case NATIVE_WINDOW_API_EGL:
- case NATIVE_WINDOW_API_CPU:
- case NATIVE_WINDOW_API_MEDIA:
- case NATIVE_WINDOW_API_CAMERA:
- if (mConnectedApi == api) {
- drainQueueAndFreeBuffersLocked();
- mConnectedApi = NO_CONNECTED_API;
- mNextCrop.makeInvalid();
- mNextScalingMode = NATIVE_WINDOW_SCALING_MODE_FREEZE;
- mNextTransform = 0;
- mDequeueCondition.broadcast();
- } else {
- ST_LOGE("disconnect: connected to another api (cur=%d, req=%d)",
- mConnectedApi, api);
+ sp<ConsumerListener> listener;
+
+ { // Scope for the lock
+ Mutex::Autolock lock(mMutex);
+
+ if (mAbandoned) {
+ // it is not really an error to disconnect after the surface
+ // has been abandoned, it should just be a no-op.
+ return NO_ERROR;
+ }
+
+ switch (api) {
+ case NATIVE_WINDOW_API_EGL:
+ case NATIVE_WINDOW_API_CPU:
+ case NATIVE_WINDOW_API_MEDIA:
+ case NATIVE_WINDOW_API_CAMERA:
+ if (mConnectedApi == api) {
+ drainQueueAndFreeBuffersLocked();
+ mConnectedApi = NO_CONNECTED_API;
+ mNextCrop.makeInvalid();
+ mNextScalingMode = NATIVE_WINDOW_SCALING_MODE_FREEZE;
+ mNextTransform = 0;
+ mDequeueCondition.broadcast();
+ listener = mConsumerListener;
+ } else {
+ ST_LOGE("disconnect: connected to another api (cur=%d, req=%d)",
+ mConnectedApi, api);
+ err = -EINVAL;
+ }
+ break;
+ default:
+ ST_LOGE("disconnect: unknown API %d", api);
err = -EINVAL;
- }
- break;
- default:
- ST_LOGE("disconnect: unknown API %d", api);
- err = -EINVAL;
- break;
+ break;
+ }
}
+
+ if (listener != NULL) {
+ listener->onBuffersReleased();
+ }
+
return err;
}
@@ -841,7 +850,7 @@
}
}
-status_t BufferQueue::acquire(BufferItem *buffer) {
+status_t BufferQueue::acquireBuffer(BufferItem *buffer) {
ATRACE_CALL();
Mutex::Autolock _l(mMutex);
// check if queue is empty
@@ -855,8 +864,7 @@
if (mSlots[buf].mAcquireCalled) {
buffer->mGraphicBuffer = NULL;
- }
- else {
+ } else {
buffer->mGraphicBuffer = mSlots[buf].mGraphicBuffer;
}
buffer->mCrop = mSlots[buf].mCrop;
@@ -872,8 +880,7 @@
mDequeueCondition.broadcast();
ATRACE_INT(mConsumerName.string(), mQueue.size());
- }
- else {
+ } else {
// should be a better return code?
return -EINVAL;
}
@@ -907,17 +914,58 @@
return OK;
}
-status_t BufferQueue::consumerDisconnect() {
+status_t BufferQueue::consumerConnect(const sp<ConsumerListener>& consumerListener) {
+ ST_LOGV("consumerConnect");
Mutex::Autolock lock(mMutex);
- mAbandoned = true;
+ if (mAbandoned) {
+ ST_LOGE("consumerConnect: BufferQueue has been abandoned!");
+ return NO_INIT;
+ }
+ mConsumerListener = consumerListener;
+
+ return OK;
+}
+
+status_t BufferQueue::consumerDisconnect() {
+ ST_LOGV("consumerDisconnect");
+ Mutex::Autolock lock(mMutex);
+
+ if (mConsumerListener == NULL) {
+ ST_LOGE("consumerDisconnect: No consumer is connected!");
+ return -EINVAL;
+ }
+
+ mAbandoned = true;
+ mConsumerListener = NULL;
mQueue.clear();
freeAllBuffersLocked();
mDequeueCondition.broadcast();
return OK;
}
+status_t BufferQueue::getReleasedBuffers(uint32_t* slotMask) {
+ ST_LOGV("getReleasedBuffers");
+ Mutex::Autolock lock(mMutex);
+
+ if (mAbandoned) {
+ ST_LOGE("getReleasedBuffers: BufferQueue has been abandoned!");
+ return NO_INIT;
+ }
+
+ uint32_t mask = 0;
+ for (int i = 0; i < NUM_BUFFER_SLOTS; i++) {
+ if (!mSlots[i].mAcquireCalled) {
+ mask |= 1 << i;
+ }
+ }
+ *slotMask = mask;
+
+ ST_LOGV("getReleasedBuffers: returning mask %#x", mask);
+ return NO_ERROR;
+}
+
status_t BufferQueue::setDefaultBufferSize(uint32_t w, uint32_t h)
{
ST_LOGV("setDefaultBufferSize: w=%d, h=%d", w, h);
@@ -982,4 +1030,24 @@
return err;
}
+BufferQueue::ProxyConsumerListener::ProxyConsumerListener(
+ const wp<BufferQueue::ConsumerListener>& consumerListener):
+ mConsumerListener(consumerListener) {}
+
+BufferQueue::ProxyConsumerListener::~ProxyConsumerListener() {}
+
+void BufferQueue::ProxyConsumerListener::onFrameAvailable() {
+ sp<BufferQueue::ConsumerListener> listener(mConsumerListener.promote());
+ if (listener != NULL) {
+ listener->onFrameAvailable();
+ }
+}
+
+void BufferQueue::ProxyConsumerListener::onBuffersReleased() {
+ sp<BufferQueue::ConsumerListener> listener(mConsumerListener.promote());
+ if (listener != NULL) {
+ listener->onBuffersReleased();
+ }
+}
+
}; // namespace android
diff --git a/libs/gui/SurfaceTexture.cpp b/libs/gui/SurfaceTexture.cpp
index 5fb933b..d8dd5bd 100644
--- a/libs/gui/SurfaceTexture.cpp
+++ b/libs/gui/SurfaceTexture.cpp
@@ -122,17 +122,32 @@
mName = String8::format("unnamed-%d-%d", getpid(), createProcessUniqueId());
ST_LOGV("SurfaceTexture");
if (bufferQueue == 0) {
-
ST_LOGV("Creating a new BufferQueue");
mBufferQueue = new BufferQueue(allowSynchronousMode);
}
else {
mBufferQueue = bufferQueue;
}
- mBufferQueue->setConsumerName(mName);
memcpy(mCurrentTransformMatrix, mtxIdentity,
sizeof(mCurrentTransformMatrix));
+
+ // Note that we can't create an sp<...>(this) in a ctor that will not keep a
+ // reference once the ctor ends, as that would cause the refcount of 'this'
+ // dropping to 0 at the end of the ctor. Since all we need is a wp<...>
+ // that's what we create.
+ wp<BufferQueue::ConsumerListener> listener;
+ sp<BufferQueue::ConsumerListener> proxy;
+ listener = static_cast<BufferQueue::ConsumerListener*>(this);
+ proxy = new BufferQueue::ProxyConsumerListener(listener);
+
+ status_t err = mBufferQueue->consumerConnect(proxy);
+ if (err != NO_ERROR) {
+ ST_LOGE("SurfaceTexture: error connecting to BufferQueue: %s (%d)",
+ strerror(-err), err);
+ } else {
+ mBufferQueue->setConsumerName(mName);
+ }
}
SurfaceTexture::~SurfaceTexture() {
@@ -167,7 +182,7 @@
// In asynchronous mode the list is guaranteed to be one buffer
// deep, while in synchronous mode we use the oldest buffer.
- if (mBufferQueue->acquire(&item) == NO_ERROR) {
+ if (mBufferQueue->acquireBuffer(&item) == NO_ERROR) {
int buf = item.mBuf;
// This buffer was newly allocated, so we need to clean up on our side
if (item.mGraphicBuffer != NULL) {
@@ -394,7 +409,7 @@
const sp<FrameAvailableListener>& listener) {
ST_LOGV("setFrameAvailableListener");
Mutex::Autolock lock(mMutex);
- mBufferQueue->setFrameAvailableListener(listener);
+ mFrameAvailableListener = listener;
}
EGLImageKHR SurfaceTexture::createImage(EGLDisplay dpy,
@@ -438,24 +453,36 @@
return mBufferQueue->isSynchronousMode();
}
-void SurfaceTexture::abandon() {
- Mutex::Autolock lock(mMutex);
- mAbandoned = true;
- mCurrentTextureBuf.clear();
-
- // destroy all egl buffers
- for (int i =0; i < BufferQueue::NUM_BUFFER_SLOTS; i++) {
- mEGLSlots[i].mGraphicBuffer = 0;
- if (mEGLSlots[i].mEglImage != EGL_NO_IMAGE_KHR) {
- eglDestroyImageKHR(mEGLSlots[i].mEglDisplay,
- mEGLSlots[i].mEglImage);
- mEGLSlots[i].mEglImage = EGL_NO_IMAGE_KHR;
- mEGLSlots[i].mEglDisplay = EGL_NO_DISPLAY;
+void SurfaceTexture::freeBufferLocked(int slotIndex) {
+ ST_LOGV("freeBufferLocked: slotIndex=%d", slotIndex);
+ mEGLSlots[slotIndex].mGraphicBuffer = 0;
+ if (mEGLSlots[slotIndex].mEglImage != EGL_NO_IMAGE_KHR) {
+ EGLImageKHR img = mEGLSlots[slotIndex].mEglImage;
+ if (img != EGL_NO_IMAGE_KHR) {
+ eglDestroyImageKHR(mEGLSlots[slotIndex].mEglDisplay, img);
}
+ mEGLSlots[slotIndex].mEglImage = EGL_NO_IMAGE_KHR;
+ mEGLSlots[slotIndex].mEglDisplay = EGL_NO_DISPLAY;
}
+}
- // disconnect from the BufferQueue
- mBufferQueue->consumerDisconnect();
+void SurfaceTexture::abandon() {
+ ST_LOGV("abandon");
+ Mutex::Autolock lock(mMutex);
+
+ if (!mAbandoned) {
+ mAbandoned = true;
+ mCurrentTextureBuf.clear();
+
+ // destroy all egl buffers
+ for (int i =0; i < BufferQueue::NUM_BUFFER_SLOTS; i++) {
+ freeBufferLocked(i);
+ }
+
+ // disconnect from the BufferQueue
+ mBufferQueue->consumerDisconnect();
+ mBufferQueue.clear();
+ }
}
void SurfaceTexture::setName(const String8& name) {
@@ -505,6 +532,40 @@
return mBufferQueue->connect(api, outWidth, outHeight, outTransform);
}
+void SurfaceTexture::onFrameAvailable() {
+ ST_LOGV("onFrameAvailable");
+
+ sp<FrameAvailableListener> listener;
+ { // scope for the lock
+ Mutex::Autolock lock(mMutex);
+ listener = mFrameAvailableListener;
+ }
+
+ if (listener != NULL) {
+ ST_LOGV("actually calling onFrameAvailable");
+ listener->onFrameAvailable();
+ }
+}
+
+void SurfaceTexture::onBuffersReleased() {
+ ST_LOGV("onBuffersReleased");
+
+ Mutex::Autolock lock(mMutex);
+
+ if (mAbandoned) {
+ // Nothing to do if we're already abandoned.
+ return;
+ }
+
+ uint32_t mask = 0;
+ mBufferQueue->getReleasedBuffers(&mask);
+ for (int i = 0; i < BufferQueue::NUM_BUFFER_SLOTS; i++) {
+ if (mask & (1 << i)) {
+ freeBufferLocked(i);
+ }
+ }
+}
+
void SurfaceTexture::dump(String8& result) const
{
char buffer[1024];
@@ -515,19 +576,21 @@
char* buffer, size_t SIZE) const
{
Mutex::Autolock _l(mMutex);
- snprintf(buffer, SIZE, "%smTexName=%d\n", prefix, mTexName);
+ snprintf(buffer, SIZE, "%smTexName=%d, mAbandoned=%d\n", prefix, mTexName,
+ int(mAbandoned));
result.append(buffer);
snprintf(buffer, SIZE,
- "%snext : {crop=[%d,%d,%d,%d], transform=0x%02x, current=%d}\n"
- ,prefix, mCurrentCrop.left,
+ "%snext : {crop=[%d,%d,%d,%d], transform=0x%02x, current=%d}\n",
+ prefix, mCurrentCrop.left,
mCurrentCrop.top, mCurrentCrop.right, mCurrentCrop.bottom,
mCurrentTransform, mCurrentTexture
);
result.append(buffer);
-
- mBufferQueue->dump(result, prefix, buffer, SIZE);
+ if (!mAbandoned) {
+ mBufferQueue->dump(result, prefix, buffer, SIZE);
+ }
}
static void mtxMul(float out[16], const float a[16], const float b[16]) {
diff --git a/libs/gui/tests/SurfaceTextureClient_test.cpp b/libs/gui/tests/SurfaceTextureClient_test.cpp
index c1a3c98..aa1f94e 100644
--- a/libs/gui/tests/SurfaceTextureClient_test.cpp
+++ b/libs/gui/tests/SurfaceTextureClient_test.cpp
@@ -14,10 +14,14 @@
* limitations under the License.
*/
+#define LOG_TAG "SurfaceTextureClient_test"
+//#define LOG_NDEBUG 0
+
#include <EGL/egl.h>
#include <gtest/gtest.h>
#include <gui/SurfaceTextureClient.h>
-#include <utils/threads.h>
+#include <utils/Log.h>
+#include <utils/Thread.h>
namespace android {
@@ -30,6 +34,11 @@
}
virtual void SetUp() {
+ const ::testing::TestInfo* const testInfo =
+ ::testing::UnitTest::GetInstance()->current_test_info();
+ ALOGV("Begin test: %s.%s", testInfo->test_case_name(),
+ testInfo->name());
+
mST = new SurfaceTexture(123);
mSTC = new SurfaceTextureClient(mST);
mANW = mSTC;
@@ -76,6 +85,11 @@
eglDestroyContext(mEglDisplay, mEglContext);
eglDestroySurface(mEglDisplay, mEglSurface);
eglTerminate(mEglDisplay);
+
+ const ::testing::TestInfo* const testInfo =
+ ::testing::UnitTest::GetInstance()->current_test_info();
+ ALOGV("End test: %s.%s", testInfo->test_case_name(),
+ testInfo->name());
}
virtual EGLint const* getConfigAttribs() {
@@ -147,6 +161,10 @@
EXPECT_NE(EGL_NO_SURFACE, eglSurface);
EXPECT_EQ(EGL_SUCCESS, eglGetError());
+ if (eglSurface != EGL_NO_SURFACE) {
+ eglDestroySurface(dpy, eglSurface);
+ }
+
eglTerminate(dpy);
}
diff --git a/libs/gui/tests/SurfaceTexture_test.cpp b/libs/gui/tests/SurfaceTexture_test.cpp
index 8c6defe..71cf577 100644
--- a/libs/gui/tests/SurfaceTexture_test.cpp
+++ b/libs/gui/tests/SurfaceTexture_test.cpp
@@ -47,6 +47,11 @@
}
virtual void SetUp() {
+ const ::testing::TestInfo* const testInfo =
+ ::testing::UnitTest::GetInstance()->current_test_info();
+ ALOGV("Begin test: %s.%s", testInfo->test_case_name(),
+ testInfo->name());
+
mEglDisplay = eglGetDisplay(EGL_DEFAULT_DISPLAY);
ASSERT_EQ(EGL_SUCCESS, eglGetError());
ASSERT_NE(EGL_NO_DISPLAY, mEglDisplay);
@@ -148,6 +153,11 @@
eglTerminate(mEglDisplay);
}
ASSERT_EQ(EGL_SUCCESS, eglGetError());
+
+ const ::testing::TestInfo* const testInfo =
+ ::testing::UnitTest::GetInstance()->current_test_info();
+ ALOGV("End test: %s.%s", testInfo->test_case_name(),
+ testInfo->name());
}
virtual EGLint const* getConfigAttribs() {
@@ -1188,7 +1198,10 @@
}
TEST_F(SurfaceTextureGLToGLTest, EglDestroySurfaceUnrefsBuffers) {
- sp<GraphicBuffer> buffers[3];
+ sp<GraphicBuffer> buffers[2];
+
+ sp<FrameWaiter> fw(new FrameWaiter);
+ mST->setFrameAvailableListener(fw);
// This test requires async mode to run on a single thread.
EXPECT_TRUE(eglMakeCurrent(mEglDisplay, mProducerEglSurface,
@@ -1197,7 +1210,7 @@
EXPECT_TRUE(eglSwapInterval(mEglDisplay, 0));
ASSERT_EQ(EGL_SUCCESS, eglGetError());
- for (int i = 0; i < 3; i++) {
+ for (int i = 0; i < 2; i++) {
// Produce a frame
EXPECT_TRUE(eglMakeCurrent(mEglDisplay, mProducerEglSurface,
mProducerEglSurface, mProducerEglContext));
@@ -1209,6 +1222,7 @@
EXPECT_TRUE(eglMakeCurrent(mEglDisplay, mEglSurface, mEglSurface,
mEglContext));
ASSERT_EQ(EGL_SUCCESS, eglGetError());
+ fw->waitForFrame();
mST->updateTexImage();
buffers[i] = mST->getCurrentBuffer();
}
@@ -1220,24 +1234,23 @@
// Destroy the EGLSurface
EXPECT_TRUE(eglDestroySurface(mEglDisplay, mProducerEglSurface));
ASSERT_EQ(EGL_SUCCESS, eglGetError());
+ mProducerEglSurface = EGL_NO_SURFACE;
- // Release the ref that the SurfaceTexture has on buffers[2].
- mST->abandon();
-
+ // This test should have the only reference to buffer 0.
EXPECT_EQ(1, buffers[0]->getStrongCount());
- EXPECT_EQ(1, buffers[1]->getStrongCount());
- // Depending on how lazily the GL driver dequeues buffers, we may end up
- // with either two or three total buffers. If there are three, make sure
- // the last one was properly down-ref'd.
- if (buffers[2] != buffers[0]) {
- EXPECT_EQ(1, buffers[2]->getStrongCount());
- }
+ // The SurfaceTexture should hold a single reference to buffer 1 in its
+ // mCurrentBuffer member. All of the references in the slots should have
+ // been released.
+ EXPECT_EQ(2, buffers[1]->getStrongCount());
}
TEST_F(SurfaceTextureGLToGLTest, EglDestroySurfaceAfterAbandonUnrefsBuffers) {
sp<GraphicBuffer> buffers[3];
+ sp<FrameWaiter> fw(new FrameWaiter);
+ mST->setFrameAvailableListener(fw);
+
// This test requires async mode to run on a single thread.
EXPECT_TRUE(eglMakeCurrent(mEglDisplay, mProducerEglSurface,
mProducerEglSurface, mProducerEglContext));
@@ -1258,6 +1271,7 @@
EXPECT_TRUE(eglMakeCurrent(mEglDisplay, mEglSurface, mEglSurface,
mEglContext));
ASSERT_EQ(EGL_SUCCESS, eglGetError());
+ fw->waitForFrame();
ASSERT_EQ(NO_ERROR, mST->updateTexImage());
buffers[i] = mST->getCurrentBuffer();
}
@@ -1273,6 +1287,7 @@
// Destroy the EGLSurface.
EXPECT_TRUE(eglDestroySurface(mEglDisplay, mProducerEglSurface));
ASSERT_EQ(EGL_SUCCESS, eglGetError());
+ mProducerEglSurface = EGL_NO_SURFACE;
EXPECT_EQ(1, buffers[0]->getStrongCount());
EXPECT_EQ(1, buffers[1]->getStrongCount());