Merge "Fix a GraphicBuffer leak in SurfaceTexture"

6 files changed, 23 insertions, 65 deletions

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 @@ private:
     // 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 @@ 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<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 @@ namespace android {
 
 enum {
     CREATE_GRAPHIC_BUFFER = IBinder::FIRST_CALL_TRANSACTION,
-    FREE_ALL_GRAPHIC_BUFFERS_EXCEPT,
 };
 
 class BpGraphicBufferAlloc : public BpInterface<IGraphicBufferAlloc>
@@ -46,8 +45,7 @@ public:
     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 @@ 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<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 @@ 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 f4e2a67..e2346f0 100644
--- a/libs/gui/SurfaceTexture.cpp
+++ b/libs/gui/SurfaceTexture.cpp
@@ -172,7 +172,6 @@ sp<GraphicBuffer> 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 ea283c6..2f3a144 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2553,22 +2553,9 @@ sp<GraphicBuffer> 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<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 @@ class GraphicBufferAlloc : public BnGraphicBufferAlloc
 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;
 };
 
 // ---------------------------------------------------------------------------