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

Commit 402c3464 authored by Mathias Agopian's avatar Mathias Agopian
Browse files

fix some issues with Surface's lifetime management. 

To deal with Java's lack of destructors and delayed garbage collection, we used to duplicate Surface.cpp objects in some case; this caused some issues because Surface is supposed to be reference-counted and unique.
parent a6b40ba5

core/java/android/view/Surface.java

+4 −2
Original line number Diff line number Diff line
@@ -304,7 +304,7 @@ public class Surface implements Parcelable { 
    /* no user serviceable parts here ... */

    @Override

    protected void finalize() throws Throwable {

        clear();

        release();

    }

    

    private native void init(SurfaceSession s,
@@ -312,4 +312,6 @@ public class Surface implements Parcelable { 
            throws OutOfResourcesException;



    private native void init(Parcel source);

    

    private native void release();

}

core/jni/android_view_Surface.cpp

+27 −17
Original line number Diff line number Diff line
@@ -173,6 +173,15 @@ static void Surface_initParcel(JNIEnv* env, jobject clazz, jobject argParcel) 
}



static void Surface_clear(JNIEnv* env, jobject clazz, uintptr_t *ostack)

{

    const sp<Surface>& surface = getSurface(env, clazz);

    if (Surface::isValid(surface)) {

        surface->clear();

    }

    setSurface(env, clazz, 0);

}



static void Surface_release(JNIEnv* env, jobject clazz, uintptr_t *ostack)

{

    setSurface(env, clazz, 0);

}
@@ -180,7 +189,7 @@ static void Surface_clear(JNIEnv* env, jobject clazz, uintptr_t *ostack) 
static jboolean Surface_isValid(JNIEnv* env, jobject clazz)

{

    const sp<Surface>& surface = getSurface(env, clazz);

    return surface->isValid() ? JNI_TRUE : JNI_FALSE;

    return Surface::isValid(surface) ? JNI_TRUE : JNI_FALSE;

}



static inline SkBitmap::Config convertPixelFormat(PixelFormat format)
@@ -201,7 +210,7 @@ static inline SkBitmap::Config convertPixelFormat(PixelFormat format) 
static jobject Surface_lockCanvas(JNIEnv* env, jobject clazz, jobject dirtyRect)

{

    const sp<Surface>& surface = getSurface(env, clazz);

    if (!surface->isValid())

    if (!Surface::isValid(surface))

        return 0;



    // get dirty region
@@ -270,7 +279,7 @@ static void Surface_unlockCanvasAndPost( 
    }

    

    const sp<Surface>& surface = getSurface(env, clazz);

    if (!surface->isValid())

    if (!Surface::isValid(surface))

        return;



    // detach the canvas from the surface
@@ -337,7 +346,7 @@ static void Surface_setLayer( 
        JNIEnv* env, jobject clazz, jint zorder)

{

    const sp<Surface>& surface = getSurface(env, clazz);

    if (surface->isValid()) {

    if (Surface::isValid(surface)) {

        if (surface->setLayer(zorder) < 0) {

            doThrow(env, "java/lang/IllegalArgumentException", NULL);

        }
@@ -348,7 +357,7 @@ static void Surface_setPosition( 
        JNIEnv* env, jobject clazz, jint x, jint y)

{

    const sp<Surface>& surface = getSurface(env, clazz);

    if (surface->isValid()) {

    if (Surface::isValid(surface)) {

        if (surface->setPosition(x, y) < 0) {

            doThrow(env, "java/lang/IllegalArgumentException", NULL);

        }
@@ -359,7 +368,7 @@ static void Surface_setSize( 
        JNIEnv* env, jobject clazz, jint w, jint h)

{

    const sp<Surface>& surface = getSurface(env, clazz);

    if (surface->isValid()) {

    if (Surface::isValid(surface)) {

        if (surface->setSize(w, h) < 0) {

            doThrow(env, "java/lang/IllegalArgumentException", NULL);

        }
@@ -370,7 +379,7 @@ static void Surface_hide( 
        JNIEnv* env, jobject clazz)

{

    const sp<Surface>& surface = getSurface(env, clazz);

    if (surface->isValid()) {

    if (Surface::isValid(surface)) {

        if (surface->hide() < 0) {

            doThrow(env, "java/lang/IllegalArgumentException", NULL);

        }
@@ -381,7 +390,7 @@ static void Surface_show( 
        JNIEnv* env, jobject clazz)

{

    const sp<Surface>& surface = getSurface(env, clazz);

    if (surface->isValid()) {

    if (Surface::isValid(surface)) {

        if (surface->show() < 0) {

            doThrow(env, "java/lang/IllegalArgumentException", NULL);

        }
@@ -392,7 +401,7 @@ static void Surface_freeze( 
        JNIEnv* env, jobject clazz)

{

    const sp<Surface>& surface = getSurface(env, clazz);

    if (surface->isValid()) {

    if (Surface::isValid(surface)) {

        if (surface->freeze() < 0) {

            doThrow(env, "java/lang/IllegalArgumentException", NULL);

        }
@@ -403,7 +412,7 @@ static void Surface_unfreeze( 
        JNIEnv* env, jobject clazz)

{

    const sp<Surface>& surface = getSurface(env, clazz);

    if (surface->isValid()) {

    if (Surface::isValid(surface)) {

        if (surface->unfreeze() < 0) {

            doThrow(env, "java/lang/IllegalArgumentException", NULL);

        }
@@ -414,7 +423,7 @@ static void Surface_setFlags( 
        JNIEnv* env, jobject clazz, jint flags, jint mask)

{

    const sp<Surface>& surface = getSurface(env, clazz);

    if (surface->isValid()) {

    if (Surface::isValid(surface)) {

        if (surface->setFlags(flags, mask) < 0) {

            doThrow(env, "java/lang/IllegalArgumentException", NULL);

        }
@@ -425,7 +434,7 @@ static void Surface_setTransparentRegion( 
        JNIEnv* env, jobject clazz, jobject argRegion)

{

    const sp<Surface>& surface = getSurface(env, clazz);

    if (surface->isValid()) {

    if (Surface::isValid(surface)) {

        SkRegion* nativeRegion = (SkRegion*)env->GetIntField(argRegion, no.native_region);

        if (surface->setTransparentRegionHint(Region(*nativeRegion)) < 0) {

            doThrow(env, "java/lang/IllegalArgumentException", NULL);
@@ -437,7 +446,7 @@ static void Surface_setAlpha( 
        JNIEnv* env, jobject clazz, jfloat alpha)

{

    const sp<Surface>& surface = getSurface(env, clazz);

    if (surface->isValid()) {

    if (Surface::isValid(surface)) {

        if (surface->setAlpha(alpha) < 0) {

            doThrow(env, "java/lang/IllegalArgumentException", NULL);

        }
@@ -449,7 +458,7 @@ static void Surface_setMatrix( 
        jfloat dsdx, jfloat dtdx, jfloat dsdy, jfloat dtdy)

{

    const sp<Surface>& surface = getSurface(env, clazz);

    if (surface->isValid()) {

    if (Surface::isValid(surface)) {

        if (surface->setMatrix(dsdx, dtdx, dsdy, dtdy) < 0) {

            doThrow(env, "java/lang/IllegalArgumentException", NULL);

        }
@@ -461,7 +470,7 @@ static void Surface_setFreezeTint( 
        jint tint)

{

    const sp<Surface>& surface = getSurface(env, clazz);

    if (surface->isValid()) {

    if (Surface::isValid(surface)) {

        if (surface->setFreezeTint(tint) < 0) {

            doThrow(env, "java/lang/IllegalArgumentException", NULL);

        }
@@ -484,7 +493,7 @@ static void Surface_copyFrom( 
    if (!Surface::isSameSurface(surface, rhs)) {

        // we reassign the surface only if it's a different one

        // otherwise we would loose our client-side state.

        setSurface(env, clazz, rhs->dup());

        setSurface(env, clazz, rhs);

    }

}
@@ -541,6 +550,7 @@ static JNINativeMethod gSurfaceMethods[] = { 
    {"init",                "(Landroid/view/SurfaceSession;IIIIII)V",  (void*)Surface_init },

    {"init",                "(Landroid/os/Parcel;)V",  (void*)Surface_initParcel },

    {"clear",               "()V",  (void*)Surface_clear },

    {"release",             "()V",  (void*)Surface_release },

	{"copyFrom",            "(Landroid/view/Surface;)V",  (void*)Surface_copyFrom },

	{"isValid",             "()Z",  (void*)Surface_isValid },

	{"lockCanvasNative",    "(Landroid/graphics/Rect;)Landroid/graphics/Canvas;",  (void*)Surface_lockCanvas },

include/ui/Surface.h

+9 −2
Original line number Diff line number Diff line
@@ -90,9 +90,15 @@ public: 
        uint32_t    reserved[2];

    };



    bool        isValid() const { return this && mToken>=0 && mClient!=0; }

    static bool isValid(const sp<Surface>& surface) {

        return (surface != 0) && surface->mToken>=0 && surface->mClient!=0;

    }

        

    SurfaceID   ID() const      { return mToken; }



    // release surface data from java

    void        clear();



    status_t    lock(SurfaceInfo* info, bool blocking = true);

    status_t    lock(SurfaceInfo* info, Region* dirty, bool blocking = true);

    status_t    unlockAndPost();
@@ -103,7 +109,6 @@ public: 
    void        setSwapRectangle(const Rect& r);

    const Rect& swapRectangle() const;



    sp<Surface>         dup() const;

    static sp<Surface>  readFromParcel(Parcel* parcel);

    static status_t     writeToParcel(const sp<Surface>& surface, Parcel* parcel);

    static bool         isSameSurface(const sp<Surface>& lhs, const sp<Surface>& rhs);
@@ -149,6 +154,8 @@ private: 


    ~Surface();

    

    void destroy();



    Region dirtyRegion() const;

    void setDirtyRegion(const Region& region) const;

libs/ui/Surface.cpp

+45 −39
Original line number Diff line number Diff line
@@ -176,61 +176,60 @@ Surface::Surface(const sp<SurfaceComposerClient>& client, 
    const_cast<uint32_t&>(android_native_window_t::flags) = 0;

}



Surface::Surface(Surface const* rhs)

    : mOwner(false)

Surface::~Surface()

{

    // FIXME: we really should get rid of this ctor. the memcpy below

    //should be safe for now, but android_native_window_t is not supposed

    // to be clonable.

    memcpy( static_cast<android_native_window_t*>(this),

            static_cast<android_native_window_t const *>(rhs),

            sizeof(android_native_window_t));



    mToken      = rhs->mToken;

    mIdentity   = rhs->mIdentity;

    mClient     = rhs->mClient;

    mSurface    = rhs->mSurface;

    mBuffers[0] = rhs->mBuffers[0];

    mBuffers[1] = rhs->mBuffers[1];

    mFormat     = rhs->mFormat;

    mFlags      = rhs->mFlags;

    mSwapRectangle.makeInvalid();

    // this is a client-side operation, the surface is destroyed, unmap

    // its buffers in this process.

    for (int i=0 ; i<2 ; i++) {

        if (mBuffers[i] != 0) {

            BufferMapper::get().unmap(mBuffers[i]->getHandle(), this);

        }

    }



Surface::~Surface()

    destroy();

}



void Surface::destroy()

{

    // Destroy the surface in SurfaceFlinger if we were the owner

    // (in any case, a client won't be able to, because it won't have the

    // right permission).

    if (mOwner && mToken>=0 && mClient!=0) {

        mClient->destroySurface(mToken);

        if (mBuffers[0] != 0) {

            BufferMapper::get().unmap(mBuffers[0]->getHandle(), this);

        }

        if (mBuffers[1] != 0) {

            BufferMapper::get().unmap(mBuffers[1]->getHandle(), this);

        }

    }



    // clear all references and trigger an IPC now, to make sure things

    // happen without delay, since these resources are quite heavy.

    mClient.clear();

    mSurface.clear();

    IPCThreadState::self()->flushCommands();

}



sp<Surface> Surface::dup() const

void Surface::clear() 

{

    // FIXME: we should get rid of Surface::dup()

    Surface const * r = this;

    if (this && mOwner) {

        // the only reason we need to do this is because of Java's garbage

        // collector: because we're creating a copy of the Surface

        // instead of a reference, we can guarantee that when our last

        // reference goes away, the real surface will be deleted.

        // Without this hack (the code is correct too), we'd have to

        // wait for a GC for the surface to go away.

        r = new Surface(this);        

    }

    return const_cast<Surface*>(r);

    // here, the window manager tells us explicitly that we should destroy

    // the surface's resource. Soon after this call, it will also release

    // its last reference (which will call the dtor); however, it is possible

    // that a client living in the same process still holds references which

    // would delay the call to the dtor -- that is why we need this explicit

    // "clear()" call.



    // FIXME: we should probably unmap the buffers here. The problem is that

    // the app could be in the middle of using them, and if we don't unmap now

    // and we're in the system process, the mapping will be lost (because

    // the buffer will be freed, and the handles destroyed)

    

    Mutex::Autolock _l(mSurfaceLock);

    destroy();

}



status_t Surface::validate(per_client_cblk_t const* cblk) const

{

    if (mToken<0 || mClient==0) {

        LOGE("invalid token (%d, identity=%u) or client (%p)", 

                mToken, mIdentity, mClient.get());

        return NO_INIT;

    }

    if (cblk == 0) {

        LOGE("cblk is null (surface id=%d, identity=%u)", mToken, mIdentity);

        return NO_INIT;
@@ -330,6 +329,13 @@ int Surface::dequeueBuffer(android_native_buffer_t** buffer) 


int Surface::lockBuffer(android_native_buffer_t* buffer)

{

    Mutex::Autolock _l(mSurfaceLock);



    per_client_cblk_t* const cblk = mClient->mControl;

    status_t err = validate(cblk);

    if (err != NO_ERROR)

        return err;



    // FIXME: lockBuffer() needs proper implementation

    return 0;

}
@@ -543,7 +549,7 @@ status_t Surface::writeToParcel(const sp<Surface>& surface, Parcel* parcel) 
    uint32_t identity = 0;

    sp<SurfaceComposerClient> client;

    sp<ISurface> sur;

    if (surface->isValid()) {

    if (Surface::isValid(surface)) {

        token = surface->mToken;

        identity = surface->mIdentity;

        client = surface->mClient;