fix [2873058] Surface::dequeueBuffer blocks on last buffer, i.e. cannot dequeue all allocated buffers at once.
this situation happened when the last buffer needed to be resized
(or allocated, the first time). the assumption was that the buffer
was in use by SF itself as the current buffer (obviously, this
assumption made no sense when the buffer had never been allocated, btw).
the system would wait until some other buffer became the "front" buffer.
we fix this problem by entirely removing the requirement that the
buffer being resized cannot be the front buffer. instead, we just
allocate a new buffer and replace the front buffer by the new one.
the downside is that this uses more memory (an extra buffer) for a
brief amount of time while the old buffer is being reallocated and
before it has actually been replaced.
Change-Id: I022e4621209474ceb1c671b23deb4188eaaa7285
diff --git a/include/private/surfaceflinger/SharedBufferStack.h b/include/private/surfaceflinger/SharedBufferStack.h
index 633b543..1eb178e 100644
--- a/include/private/surfaceflinger/SharedBufferStack.h
+++ b/include/private/surfaceflinger/SharedBufferStack.h
@@ -273,7 +273,6 @@
void setStatus(status_t status);
status_t reallocateAll();
status_t reallocateAllExcept(int buffer);
- status_t assertReallocate(int buffer);
int32_t getQueuedCount() const;
Region getDirtyRegion(int buffer) const;
@@ -356,13 +355,6 @@
inline StatusUpdate(SharedBufferBase* sbb, status_t status);
inline ssize_t operator()();
};
-
- struct ReallocateCondition : public ConditionBase {
- int buf;
- inline ReallocateCondition(SharedBufferBase* sbb, int buf);
- inline bool operator()() const;
- inline const char* name() const { return "ReallocateCondition"; }
- };
};
// ===========================================================================
diff --git a/libs/surfaceflinger_client/SharedBufferStack.cpp b/libs/surfaceflinger_client/SharedBufferStack.cpp
index d67a589..156a7db 100644
--- a/libs/surfaceflinger_client/SharedBufferStack.cpp
+++ b/libs/surfaceflinger_client/SharedBufferStack.cpp
@@ -243,21 +243,6 @@
(stack.queued > 0 && stack.inUse != buf));
}
-SharedBufferServer::ReallocateCondition::ReallocateCondition(
- SharedBufferBase* sbb, int buf) : ConditionBase(sbb), buf(buf) {
-}
-bool SharedBufferServer::ReallocateCondition::operator()() const {
- int32_t head = stack.head;
- if (uint32_t(head) >= SharedBufferStack::NUM_BUFFER_MAX) {
- // if stack.head is messed up, we cannot allow the server to
- // crash (since stack.head is mapped on the client side)
- stack.status = BAD_VALUE;
- return false;
- }
- // TODO: we should also check that buf has been dequeued
- return (buf != stack.index[head]);
-}
-
// ----------------------------------------------------------------------------
SharedBufferClient::QueueUpdate::QueueUpdate(SharedBufferBase* sbb)
@@ -558,21 +543,6 @@
return stack.queued;
}
-status_t SharedBufferServer::assertReallocate(int buf)
-{
- /*
- * NOTE: it's safe to hold mLock for read while waiting for
- * the ReallocateCondition because that condition is not updated
- * by the thread that holds mLock for write.
- */
- RWLock::AutoRLock _l(mLock);
-
- // TODO: need to validate "buf"
- ReallocateCondition condition(this, buf);
- status_t err = waitForCondition(condition);
- return err;
-}
-
Region SharedBufferServer::getDirtyRegion(int buf) const
{
SharedBufferStack& stack( *mSharedStack );
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index 758da4e..629d993 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -294,19 +294,9 @@
* This is called from the client's Surface::dequeue(). This can happen
* at any time, especially while we're in the middle of using the
* buffer 'index' as our front buffer.
- *
- * Make sure the buffer we're resizing is not the front buffer and has been
- * dequeued. Once this condition is asserted, we are guaranteed that this
- * buffer cannot become the front buffer under our feet, since we're called
- * from Surface::dequeue()
*/
- status_t err = lcblk->assertReallocate(index);
- LOGE_IF(err, "assertReallocate(%d) failed (%s)", index, strerror(-err));
- if (err != NO_ERROR) {
- // the surface may have died
- return buffer;
- }
+ status_t err = NO_ERROR;
uint32_t w, h, f;
{ // scope for the lock
Mutex::Autolock _l(mLock);
@@ -319,23 +309,17 @@
w = reqWidth ? reqWidth : mWidth;
h = reqHeight ? reqHeight : mHeight;
f = reqFormat ? reqFormat : mFormat;
- buffer = mBufferManager.detachBuffer(index);
if (fixedSizeChanged || formatChanged) {
lcblk->reallocateAllExcept(index);
}
}
+ // here we have to reallocate a new buffer because the buffer could be
+ // used as the front buffer, or by a client in our process
+ // (eg: status bar), and we can't release the handle under its feet.
const uint32_t effectiveUsage = getEffectiveUsage(usage);
- if (buffer!=0 && buffer->getStrongCount() == 1) {
- err = buffer->reallocate(w, h, f, effectiveUsage);
- } else {
- // here we have to reallocate a new buffer because we could have a
- // client in our process with a reference to it (eg: status bar),
- // and we can't release the handle under its feet.
- buffer.clear();
- buffer = new GraphicBuffer(w, h, f, effectiveUsage);
- err = buffer->initCheck();
- }
+ buffer = new GraphicBuffer(w, h, f, effectiveUsage);
+ err = buffer->initCheck();
if (err || buffer->handle == 0) {
LOGE_IF(err || buffer->handle == 0,
diff --git a/services/surfaceflinger/tests/surface/Android.mk b/services/surfaceflinger/tests/surface/Android.mk
new file mode 100644
index 0000000..ce0e807
--- /dev/null
+++ b/services/surfaceflinger/tests/surface/Android.mk
@@ -0,0 +1,18 @@
+LOCAL_PATH:= $(call my-dir)
+include $(CLEAR_VARS)
+
+LOCAL_SRC_FILES:= \
+ surface.cpp
+
+LOCAL_SHARED_LIBRARIES := \
+ libcutils \
+ libutils \
+ libbinder \
+ libui \
+ libsurfaceflinger_client
+
+LOCAL_MODULE:= test-surface
+
+LOCAL_MODULE_TAGS := tests
+
+include $(BUILD_EXECUTABLE)
diff --git a/services/surfaceflinger/tests/surface/surface.cpp b/services/surfaceflinger/tests/surface/surface.cpp
new file mode 100644
index 0000000..b4de4b4
--- /dev/null
+++ b/services/surfaceflinger/tests/surface/surface.cpp
@@ -0,0 +1,54 @@
+#include <cutils/memory.h>
+
+#include <utils/Log.h>
+
+#include <binder/IPCThreadState.h>
+#include <binder/ProcessState.h>
+#include <binder/IServiceManager.h>
+
+#include <surfaceflinger/Surface.h>
+#include <surfaceflinger/ISurface.h>
+#include <surfaceflinger/SurfaceComposerClient.h>
+
+#include <ui/Overlay.h>
+
+using namespace android;
+
+int main(int argc, char** argv)
+{
+ // set up the thread-pool
+ sp<ProcessState> proc(ProcessState::self());
+ ProcessState::self()->startThreadPool();
+
+ // create a client to surfaceflinger
+ sp<SurfaceComposerClient> client = new SurfaceComposerClient();
+
+ // create pushbuffer surface
+ sp<SurfaceControl> surfaceControl = client->createSurface(
+ getpid(), 0, 160, 240, PIXEL_FORMAT_RGB_565);
+ client->openTransaction();
+ surfaceControl->setLayer(100000);
+ client->closeTransaction();
+
+ // pretend it went cross-process
+ Parcel parcel;
+ SurfaceControl::writeSurfaceToParcel(surfaceControl, &parcel);
+ parcel.setDataPosition(0);
+ sp<Surface> surface = Surface::readFromParcel(parcel);
+ ANativeWindow* window = surface.get();
+
+ printf("window=%p\n", window);
+
+ int err = native_window_set_buffer_count(window, 8);
+ android_native_buffer_t* buffer;
+
+ for (int i=0 ; i<8 ; i++) {
+ window->dequeueBuffer(window, &buffer);
+ printf("buffer %d: %p\n", i, buffer);
+ }
+
+ printf("test complete. CTRL+C to finish.\n");
+
+ IPCThreadState::self()->joinThreadPool();
+ return 0;
+}