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/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);
}