Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 5f05f99a authored by Mathias Agopian's avatar Mathias Agopian
Browse files

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
parent 7797e647

include/gui/SurfaceTexture.h

+0 −6
Original line number Diff line number Diff line
@@ -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.

include/surfaceflinger/IGraphicBufferAlloc.h

+1 −9
Original line number Diff line number Diff line
@@ -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;

};



// ----------------------------------------------------------------------------

libs/gui/IGraphicBufferAlloc.cpp

+22 −17
Original line number Diff line number Diff line
@@ -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);

    }

libs/gui/SurfaceTexture.cpp

+0 −14
Original line number Diff line number Diff line
@@ -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,

services/surfaceflinger/SurfaceFlinger.cpp

+0 −13
Original line number Diff line number Diff line
@@ -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()