Fix a GraphicBuffer leak in SurfaceTexture
This leak was intentional, it was there to deal with the fact that
some gralloc implementations don't track buffer handles with
file-descriptors so buffers needed to stay alive until there were
registered, which is not guaranteed by binder transactions.
In this new implementation, we use a small BBinder holding a
reference to the buffer, which with tuck into the parcel. This forces
the reference to stay alive until the parcel is destroyed, which
is guaranteed (by construction) to happen after the buffer is
registered.
this allows the public facing API to not expose the previous hack.
Change-Id: I1dd6cd83679a2b7457ad628169e2851acc027143
diff --git a/include/gui/SurfaceTexture.h b/include/gui/SurfaceTexture.h
index 585d288..340daaf 100644
--- a/include/gui/SurfaceTexture.h
+++ b/include/gui/SurfaceTexture.h
@@ -248,12 +248,6 @@
// allocate new GraphicBuffer objects.
sp<IGraphicBufferAlloc> mGraphicBufferAlloc;
- // mAllocdBuffers is mirror of the list of buffers that SurfaceFlinger is
- // referencing. This is kept so that gralloc implementations do not need to
- // properly handle the case where SurfaceFlinger no longer holds a reference
- // to a buffer, but other processes do.
- Vector<sp<GraphicBuffer> > mAllocdBuffers;
-
// mFrameAvailableListener is the listener object that will be called when a
// new frame becomes available. If it is not NULL it will be called from
// queueBuffer.
diff --git a/include/surfaceflinger/IGraphicBufferAlloc.h b/include/surfaceflinger/IGraphicBufferAlloc.h
index d996af7..01e4bd9 100644
--- a/include/surfaceflinger/IGraphicBufferAlloc.h
+++ b/include/surfaceflinger/IGraphicBufferAlloc.h
@@ -32,18 +32,10 @@
public:
DECLARE_META_INTERFACE(GraphicBufferAlloc);
- /* Create a new GraphicBuffer for the client to use. The server will
- * maintain a reference to the newly created GraphicBuffer until
- * freeAllGraphicBuffers is called.
+ /* Create a new GraphicBuffer for the client to use.
*/
virtual sp<GraphicBuffer> createGraphicBuffer(uint32_t w, uint32_t h,
PixelFormat format, uint32_t usage) = 0;
-
- /* Free all but one of the GraphicBuffer objects that the server is
- * currently referencing. If bufIndex is not a valid index of the buffers
- * the server is referencing, then all buffers are freed.
- */
- virtual void freeAllGraphicBuffersExcept(int bufIndex) = 0;
};
// ----------------------------------------------------------------------------
diff --git a/libs/gui/IGraphicBufferAlloc.cpp b/libs/gui/IGraphicBufferAlloc.cpp
index e05da72..0cd51da 100644
--- a/libs/gui/IGraphicBufferAlloc.cpp
+++ b/libs/gui/IGraphicBufferAlloc.cpp
@@ -32,7 +32,6 @@
enum {
CREATE_GRAPHIC_BUFFER = IBinder::FIRST_CALL_TRANSACTION,
- FREE_ALL_GRAPHIC_BUFFERS_EXCEPT,
};
class BpGraphicBufferAlloc : public BpInterface<IGraphicBufferAlloc>
@@ -46,8 +45,7 @@
virtual sp<GraphicBuffer> createGraphicBuffer(uint32_t w, uint32_t h,
PixelFormat format, uint32_t usage) {
Parcel data, reply;
- data.writeInterfaceToken(
- IGraphicBufferAlloc::getInterfaceDescriptor());
+ data.writeInterfaceToken(IGraphicBufferAlloc::getInterfaceDescriptor());
data.writeInt32(w);
data.writeInt32(h);
data.writeInt32(format);
@@ -58,17 +56,12 @@
if (nonNull) {
graphicBuffer = new GraphicBuffer();
reply.read(*graphicBuffer);
+ // reply.readStrongBinder();
+ // here we don't even have to read the BufferReference from
+ // the parcel, it'll die with the parcel.
}
return graphicBuffer;
}
-
- virtual void freeAllGraphicBuffersExcept(int bufIdx) {
- Parcel data, reply;
- data.writeInterfaceToken(
- IGraphicBufferAlloc::getInterfaceDescriptor());
- data.writeInt32(bufIdx);
- remote()->transact(FREE_ALL_GRAPHIC_BUFFERS_EXCEPT, data, &reply);
- }
};
IMPLEMENT_META_INTERFACE(GraphicBufferAlloc, "android.ui.IGraphicBufferAlloc");
@@ -80,6 +73,17 @@
{
// codes that don't require permission check
+ /* BufferReference just keeps a strong reference to a
+ * GraphicBuffer until it is destroyed (that is, until
+ * no local or remote process have a reference to it).
+ */
+ class BufferReference : public BBinder {
+ sp<GraphicBuffer> buffer;
+ public:
+ BufferReference(const sp<GraphicBuffer>& buffer) : buffer(buffer) { }
+ };
+
+
switch(code) {
case CREATE_GRAPHIC_BUFFER: {
CHECK_INTERFACE(IGraphicBufferAlloc, data, reply);
@@ -91,15 +95,16 @@
reply->writeInt32(result != 0);
if (result != 0) {
reply->write(*result);
+ // We add a BufferReference to this parcel to make sure the
+ // buffer stays alive until the GraphicBuffer object on
+ // the other side has been created.
+ // This is needed so that the buffer handle can be
+ // registered before the buffer is destroyed on implementations
+ // that do not use file-descriptors to track their buffers.
+ reply->writeStrongBinder( new BufferReference(result) );
}
return NO_ERROR;
} break;
- case FREE_ALL_GRAPHIC_BUFFERS_EXCEPT: {
- CHECK_INTERFACE(IGraphicBufferAlloc, data, reply);
- int bufIdx = data.readInt32();
- freeAllGraphicBuffersExcept(bufIdx);
- return NO_ERROR;
- } break;
default:
return BBinder::onTransact(code, data, reply, flags);
}
diff --git a/libs/gui/SurfaceTexture.cpp b/libs/gui/SurfaceTexture.cpp
index f4e2a67..e2346f0 100644
--- a/libs/gui/SurfaceTexture.cpp
+++ b/libs/gui/SurfaceTexture.cpp
@@ -172,7 +172,6 @@
mSlots[buf].mEglImage = EGL_NO_IMAGE_KHR;
mSlots[buf].mEglDisplay = EGL_NO_DISPLAY;
}
- mAllocdBuffers.add(graphicBuffer);
}
return graphicBuffer;
}
@@ -425,19 +424,6 @@
mSlots[i].mEglDisplay = EGL_NO_DISPLAY;
}
}
-
- int exceptBuf = -1;
- for (size_t i = 0; i < mAllocdBuffers.size(); i++) {
- if (mAllocdBuffers[i] == mCurrentTextureBuf) {
- exceptBuf = i;
- break;
- }
- }
- mAllocdBuffers.clear();
- if (exceptBuf >= 0) {
- mAllocdBuffers.add(mCurrentTextureBuf);
- }
- mGraphicBufferAlloc->freeAllGraphicBuffersExcept(exceptBuf);
}
EGLImageKHR SurfaceTexture::createImage(EGLDisplay dpy,
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index ea283c606..2f3a144 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2553,22 +2553,9 @@
LOGE("createGraphicBuffer: unable to create GraphicBuffer");
return 0;
}
- Mutex::Autolock _l(mLock);
- mBuffers.add(graphicBuffer);
return graphicBuffer;
}
-void GraphicBufferAlloc::freeAllGraphicBuffersExcept(int bufIdx) {
- Mutex::Autolock _l(mLock);
- if (bufIdx >= 0 && size_t(bufIdx) < mBuffers.size()) {
- sp<GraphicBuffer> b(mBuffers[bufIdx]);
- mBuffers.clear();
- mBuffers.add(b);
- } else {
- mBuffers.clear();
- }
-}
-
// ---------------------------------------------------------------------------
GraphicPlane::GraphicPlane()
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 0964848..8d43157 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -125,14 +125,8 @@
public:
GraphicBufferAlloc();
virtual ~GraphicBufferAlloc();
-
virtual sp<GraphicBuffer> createGraphicBuffer(uint32_t w, uint32_t h,
PixelFormat format, uint32_t usage);
- virtual void freeAllGraphicBuffersExcept(int bufIdx);
-
-private:
- Vector<sp<GraphicBuffer> > mBuffers;
- Mutex mLock;
};
// ---------------------------------------------------------------------------