From 4cb18881b55b82a24873ccd8e298bc2d5a9c17e5 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Fri, 8 Apr 2011 19:10:43 -0700 Subject: [PATCH] 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 --- include/gui/SurfaceTexture.h | 6 --- include/surfaceflinger/IGraphicBufferAlloc.h | 10 +---- libs/gui/IGraphicBufferAlloc.cpp | 39 +++++++++++--------- libs/gui/SurfaceTexture.cpp | 14 ------- services/surfaceflinger/SurfaceFlinger.cpp | 13 ------- services/surfaceflinger/SurfaceFlinger.h | 6 --- 6 files changed, 23 insertions(+), 65 deletions(-) diff --git a/include/gui/SurfaceTexture.h b/include/gui/SurfaceTexture.h index 585d288ed..340daaf14 100644 --- a/include/gui/SurfaceTexture.h +++ b/include/gui/SurfaceTexture.h @@ -248,12 +248,6 @@ private: // allocate new GraphicBuffer objects. sp 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 > 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 d996af75a..01e4bd9ff 100644 --- a/include/surfaceflinger/IGraphicBufferAlloc.h +++ b/include/surfaceflinger/IGraphicBufferAlloc.h @@ -32,18 +32,10 @@ class IGraphicBufferAlloc : public IInterface 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 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 e05da725c..0cd51da53 100644 --- a/libs/gui/IGraphicBufferAlloc.cpp +++ b/libs/gui/IGraphicBufferAlloc.cpp @@ -32,7 +32,6 @@ namespace android { enum { CREATE_GRAPHIC_BUFFER = IBinder::FIRST_CALL_TRANSACTION, - FREE_ALL_GRAPHIC_BUFFERS_EXCEPT, }; class BpGraphicBufferAlloc : public BpInterface @@ -46,8 +45,7 @@ public: virtual sp 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 @@ public: 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 @@ status_t BnGraphicBufferAlloc::onTransact( { // 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 buffer; + public: + BufferReference(const sp& buffer) : buffer(buffer) { } + }; + + switch(code) { case CREATE_GRAPHIC_BUFFER: { CHECK_INTERFACE(IGraphicBufferAlloc, data, reply); @@ -91,15 +95,16 @@ status_t BnGraphicBufferAlloc::onTransact( 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 f4e2a6761..e2346f060 100644 --- a/libs/gui/SurfaceTexture.cpp +++ b/libs/gui/SurfaceTexture.cpp @@ -172,7 +172,6 @@ sp SurfaceTexture::requestBuffer(int buf, mSlots[buf].mEglImage = EGL_NO_IMAGE_KHR; mSlots[buf].mEglDisplay = EGL_NO_DISPLAY; } - mAllocdBuffers.add(graphicBuffer); } return graphicBuffer; } @@ -425,19 +424,6 @@ void SurfaceTexture::freeAllBuffers() { 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..2f3a144ec 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -2553,22 +2553,9 @@ sp GraphicBufferAlloc::createGraphicBuffer(uint32_t w, uint32_t h 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 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 0964848ed..8d431574a 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -125,14 +125,8 @@ class GraphicBufferAlloc : public BnGraphicBufferAlloc public: GraphicBufferAlloc(); virtual ~GraphicBufferAlloc(); - virtual sp createGraphicBuffer(uint32_t w, uint32_t h, PixelFormat format, uint32_t usage); - virtual void freeAllGraphicBuffersExcept(int bufIdx); - -private: - Vector > mBuffers; - Mutex mLock; }; // ---------------------------------------------------------------------------