more Surface lifetime management

Surfaces are now destroyed once all references from the clients are gone, but they go through a partial destruction as soon as the window manager requests it.
This last part is still buggy. see comments in SurfaceFlinger::destroySurface()

9 files changed, 141 insertions, 44 deletions

diff --git a/libs/surfaceflinger/BootAnimation.cpp b/libs/surfaceflinger/BootAnimation.cpp
index 519b112..ee36b67 100644
--- a/libs/surfaceflinger/BootAnimation.cpp
+++ b/libs/surfaceflinger/BootAnimation.cpp
@@ -179,6 +179,7 @@ bool BootAnimation::threadLoop() {
     eglMakeCurrent(mDisplay, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT);
     eglDestroyContext(mDisplay, mContext);
     eglDestroySurface(mDisplay, mSurface);
+    mFlingerSurface.clear();
     mFlingerSurfaceControl.clear();
     return r;
 }
diff --git a/libs/surfaceflinger/Layer.cpp b/libs/surfaceflinger/Layer.cpp
index 980b78b..5fdec3f 100644
--- a/libs/surfaceflinger/Layer.cpp
+++ b/libs/surfaceflinger/Layer.cpp
@@ -87,6 +87,12 @@ sp<LayerBaseClient::Surface> Layer::createSurface() const
     return mSurface;
 }
 
+status_t Layer::ditch()
+{
+    mSurface.clear();
+    return NO_ERROR;
+}
+
 status_t Layer::setBuffers( Client* client,
                             uint32_t w, uint32_t h,
                             PixelFormat format, uint32_t flags)
@@ -119,7 +125,7 @@ status_t Layer::setBuffers( Client* client,
             return err;
         }
     }
-    mSurface = new SurfaceLayer(clientIndex(), this);
+    mSurface = new SurfaceLayer(mFlinger, clientIndex(), this);
     return NO_ERROR;
 }
 
@@ -626,8 +632,13 @@ void Layer::finishPageFlip()
 
 // ---------------------------------------------------------------------------
 
-Layer::SurfaceLayer::SurfaceLayer(SurfaceID id, const sp<Layer>& owner)
-    : Surface(id, owner->getIdentity(), owner)
+Layer::SurfaceLayer::SurfaceLayer(const sp<SurfaceFlinger>& flinger,
+        SurfaceID id, const sp<Layer>& owner)
+    : Surface(flinger, id, owner->getIdentity(), owner)
+{
+}
+
+Layer::SurfaceLayer::~SurfaceLayer()
 {
 }
 
diff --git a/libs/surfaceflinger/Layer.h b/libs/surfaceflinger/Layer.h
index dc03d35..3f3953f 100644
--- a/libs/surfaceflinger/Layer.h
+++ b/libs/surfaceflinger/Layer.h
@@ -81,6 +81,7 @@ public:
     virtual bool needsBlending() const      { return mNeedsBlending; }
     virtual bool isSecure() const           { return mSecure; }
     virtual sp<Surface> createSurface() const;
+    virtual status_t ditch();
 
     const LayerBitmap& getBuffer(int i) const { return mBuffers[i]; }
           LayerBitmap& getBuffer(int i)       { return mBuffers[i]; }
@@ -108,7 +109,9 @@ private:
     class SurfaceLayer : public LayerBaseClient::Surface
     {
     public:
-                SurfaceLayer(SurfaceID id, const sp<Layer>& owner);
+                SurfaceLayer(const sp<SurfaceFlinger>& flinger,
+                        SurfaceID id, const sp<Layer>& owner);
+                ~SurfaceLayer();
 
     private:
         virtual sp<SurfaceBuffer> getBuffer();
diff --git a/libs/surfaceflinger/LayerBase.cpp b/libs/surfaceflinger/LayerBase.cpp
index 6da0bf7..8609225 100644
--- a/libs/surfaceflinger/LayerBase.cpp
+++ b/libs/surfaceflinger/LayerBase.cpp
@@ -681,25 +681,32 @@ sp<LayerBaseClient::Surface> LayerBaseClient::getSurface()
 
 sp<LayerBaseClient::Surface> LayerBaseClient::createSurface() const
 {
-    return new Surface(clientIndex(), mIdentity,
+    return new Surface(mFlinger, clientIndex(), mIdentity,
             const_cast<LayerBaseClient *>(this));
 }
 
 // ---------------------------------------------------------------------------
 
-LayerBaseClient::Surface::Surface(SurfaceID id, int identity, 
+LayerBaseClient::Surface::Surface(
+        const sp<SurfaceFlinger>& flinger,
+        SurfaceID id, int identity, 
         const sp<LayerBaseClient>& owner) 
-    : mToken(id), mIdentity(identity), mOwner(owner)
-{ 
+    : mFlinger(flinger), mToken(id), mIdentity(identity), mOwner(owner)
+{
 }
 
 
-LayerBaseClient::Surface::~Surface() {
-    // TODO: We now have a point here were we can clean-up the
-    // client's mess.
-    // This is also where surface id should be recycled.
-    //LOGD("Surface %d, heaps={%p, %p} destroyed",
-    //        mId, mHeap[0].get(), mHeap[1].get());
+LayerBaseClient::Surface::~Surface() 
+{
+    /*
+     * This is a good place to clean-up all client resources 
+     */
+
+    // destroy client resources
+    sp<LayerBaseClient> layer = getOwner();
+    if (layer != 0) {
+        mFlinger->destroySurface(layer);
+    }
 }
 
 sp<LayerBaseClient> LayerBaseClient::Surface::getOwner() const {
diff --git a/libs/surfaceflinger/LayerBase.h b/libs/surfaceflinger/LayerBase.h
index 2a4e133..ccff36d 100644
--- a/libs/surfaceflinger/LayerBase.h
+++ b/libs/surfaceflinger/LayerBase.h
@@ -203,14 +203,19 @@ public:
 
     /**
      * isSecure - true if this surface is secure, that is if it prevents
-     * screenshots or vns servers.
+     * screenshots or VNC servers.
      */
     virtual bool isSecure() const       { return false; }
 
-            enum { // flags for doTransaction()
-                eVisibleRegion      = 0x00000002,
-                eRestartTransaction = 0x00000008
-            };
+    /** signal this layer that it's not needed any longer */
+    virtual status_t ditch() { return NO_ERROR; }
+
+    
+    
+    enum { // flags for doTransaction()
+        eVisibleRegion      = 0x00000002,
+        eRestartTransaction = 0x00000008
+    };
 
 
     inline  const State&    drawingState() const    { return mDrawingState; }
@@ -244,7 +249,7 @@ protected:
                   GLint textureName, const GGLSurface& t,
                   GLuint& textureWidth, GLuint& textureHeight) const;
 
-                SurfaceFlinger* mFlinger;
+                sp<SurfaceFlinger> mFlinger;
                 uint32_t        mFlags;
 
                 // cached during validateVisibility()
@@ -316,7 +321,9 @@ public:
                 ISurfaceFlingerClient::surface_data_t* params) const;
 
     protected:
-        Surface(SurfaceID id, int identity, const sp<LayerBaseClient>& owner);
+        Surface(const sp<SurfaceFlinger>& flinger, 
+                SurfaceID id, int identity, 
+                const sp<LayerBaseClient>& owner);
         virtual ~Surface();
         virtual status_t onTransact(uint32_t code, const Parcel& data,
                 Parcel* reply, uint32_t flags);
@@ -330,8 +337,9 @@ public:
         virtual sp<OverlayRef> createOverlay(uint32_t w, uint32_t h,
                 int32_t format);
 
-    private:
+    protected:
         friend class LayerBaseClient;
+        sp<SurfaceFlinger>  mFlinger;
         int32_t             mToken;
         int32_t             mIdentity;
         wp<LayerBaseClient> mOwner;
diff --git a/libs/surfaceflinger/LayerBuffer.cpp b/libs/surfaceflinger/LayerBuffer.cpp
index f22ff59..97d6f97 100644
--- a/libs/surfaceflinger/LayerBuffer.cpp
+++ b/libs/surfaceflinger/LayerBuffer.cpp
@@ -50,10 +50,21 @@ LayerBuffer::~LayerBuffer()
 {
 }
 
+void LayerBuffer::onFirstRef()
+{
+    mSurface = new SurfaceBuffer(mFlinger, clientIndex(),
+            const_cast<LayerBuffer *>(this));
+}
+
 sp<LayerBaseClient::Surface> LayerBuffer::createSurface() const
 {
-    return new SurfaceBuffer(clientIndex(),
-                const_cast<LayerBuffer *>(this));
+    return mSurface;
+}
+
+status_t LayerBuffer::ditch()
+{
+    mSurface.clear();
+    return NO_ERROR;
 }
 
 bool LayerBuffer::needsBlending() const {
@@ -167,9 +178,9 @@ sp<LayerBuffer::Source> LayerBuffer::clearSource() {
 // LayerBuffer::SurfaceBuffer
 // ============================================================================
 
-LayerBuffer::SurfaceBuffer::SurfaceBuffer(SurfaceID id, 
-        const sp<LayerBuffer>& owner)
-    : LayerBaseClient::Surface(id, owner->getIdentity(), owner)
+LayerBuffer::SurfaceBuffer::SurfaceBuffer(const sp<SurfaceFlinger>& flinger,
+        SurfaceID id, const sp<LayerBuffer>& owner)
+    : LayerBaseClient::Surface(flinger, id, owner->getIdentity(), owner)
 {
 }
 
diff --git a/libs/surfaceflinger/LayerBuffer.h b/libs/surfaceflinger/LayerBuffer.h
index 2d44121..5284409 100644
--- a/libs/surfaceflinger/LayerBuffer.h
+++ b/libs/surfaceflinger/LayerBuffer.h
@@ -50,7 +50,6 @@ class LayerBuffer : public LayerBaseClient
         LayerBuffer& mLayer;
     };
 
-
 public:
     static const uint32_t typeInfo;
     static const char* const typeID;
@@ -61,9 +60,11 @@ public:
                         Client* client, int32_t i);
         virtual ~LayerBuffer();
 
+    virtual void onFirstRef();
     virtual bool needsBlending() const;
 
     virtual sp<LayerBaseClient::Surface> createSurface() const;
+    virtual status_t ditch();
     virtual void onDraw(const Region& clip) const;
     virtual uint32_t doTransaction(uint32_t flags);
     virtual void unlockPageFlip(const Transform& planeTransform, Region& outDirtyRegion);
@@ -177,7 +178,8 @@ private:
     class SurfaceBuffer : public LayerBaseClient::Surface
     {
     public:
-                SurfaceBuffer(SurfaceID id, const sp<LayerBuffer>& owner);
+                SurfaceBuffer(const sp<SurfaceFlinger>& flinger,
+                        SurfaceID id, const sp<LayerBuffer>& owner);
         virtual ~SurfaceBuffer();
 
         virtual status_t registerBuffers(const ISurface::BufferHeap& buffers);
@@ -191,12 +193,10 @@ private:
             return static_cast<LayerBuffer*>(Surface::getOwner().get());
         }
     };
-
-    friend class SurfaceFlinger;
-    sp<SurfaceBuffer>   getClientSurface() const;
-
+    
     mutable Mutex   mLock;
     sp<Source>      mSource;
+    sp<Surface>     mSurface;
     bool            mInvalidate;
     bool            mNeedsBlending;
 };
diff --git a/libs/surfaceflinger/SurfaceFlinger.cpp b/libs/surfaceflinger/SurfaceFlinger.cpp
index a63a282..6b2a103 100644
--- a/libs/surfaceflinger/SurfaceFlinger.cpp
+++ b/libs/surfaceflinger/SurfaceFlinger.cpp
@@ -1033,7 +1033,8 @@ status_t SurfaceFlinger::removeLayer_l(const sp<LayerBase>& layerBase)
     ssize_t index = mCurrentState.layersSortedByZ.remove(layerBase);
     if (index >= 0) {
         mLayersRemoved = true;
-        sp<LayerBaseClient> layer = LayerBase::dynamicCast< LayerBaseClient* >(layerBase.get());
+        sp<LayerBaseClient> layer =
+            LayerBase::dynamicCast< LayerBaseClient* >(layerBase.get());
         if (layer != 0) {
             mLayerMap.removeItem(layer->serverIndex());
         }
@@ -1041,11 +1042,23 @@ status_t SurfaceFlinger::removeLayer_l(const sp<LayerBase>& layerBase)
     }
     // it's possible that we don't find a layer, because it might
     // have been destroyed already -- this is not technically an error
-    // from the user because there is a race between destroySurface,
-    // destroyclient and destroySurface-from-a-transaction.
+    // from the user because there is a race between BClient::destroySurface(),
+    // ~BClient() and destroySurface-from-a-transaction.
     return (index == NAME_NOT_FOUND) ? status_t(NO_ERROR) : index;
 }
 
+status_t SurfaceFlinger::purgatorizeLayer_l(const sp<LayerBase>& layerBase)
+{
+    // First add the layer to the purgatory list, which makes sure it won't 
+    // go away, then remove it from the main list (through a transaction).
+    ssize_t err = removeLayer_l(layerBase);
+    if (err >= 0) {
+        mLayerPurgatory.add(layerBase);
+    }
+    return (err == NAME_NOT_FOUND) ? status_t(NO_ERROR) : err;
+}
+
+
 void SurfaceFlinger::free_resources_l()
 {
     // Destroy layers that were removed
@@ -1252,14 +1265,53 @@ sp<LayerBaseClient> SurfaceFlinger::createPushBuffersSurfaceLocked(
     return layer;
 }
 
-status_t SurfaceFlinger::destroySurface(SurfaceID index)
+status_t SurfaceFlinger::removeSurface(SurfaceID index)
+{
+    /*
+     * called by the window manager, when a surface should be marked for
+     * destruction.
+     */
+    
+    // TODO: here we should make the surface disappear from the screen
+    // and mark it for removal. however, we can't free anything until all
+    // client are done. All operations on this surface should return errors.
+    
+    status_t err = NAME_NOT_FOUND;
+    sp<LayerBaseClient> layer;
+    
+    { // scope for the lock
+        Mutex::Autolock _l(mStateLock);
+        layer = getLayerUser_l(index);
+        err = purgatorizeLayer_l(layer);
+        if (err == NO_ERROR) {
+            setTransactionFlags(eTransactionNeeded);
+        }
+    }
+
+    if (layer != 0) {
+        // do this outside of mStateLock
+        layer->ditch();
+    }
+    return err;
+}
+
+status_t SurfaceFlinger::destroySurface(const sp<LayerBaseClient>& layer)
 {
+    /*
+     * called by ~ISurface() when all references are gone
+     */
+
+    /* FIXME: 
+     * - this can calls ~Layer(), which is wrong because we're not in the
+     * GL thread, and ~Layer() currently calls OpenGL.
+     * - ideally we want to release as much GL state as possible after
+     * purgatorizeLayer_l() has been called and the surface is not in any
+     * active list.
+     * - ideally we'd call ~Layer() without mStateLock held
+     */
+    
     Mutex::Autolock _l(mStateLock);
-    const sp<LayerBaseClient>& layer = getLayerUser_l(index);
-    status_t err = removeLayer_l(layer);
-    if (err < 0)
-        return err;
-    setTransactionFlags(eTransactionNeeded);
+    mLayerPurgatory.remove(layer);
     return NO_ERROR;
 }
 
@@ -1626,7 +1678,7 @@ sp<ISurface> BClient::createSurface(
 status_t BClient::destroySurface(SurfaceID sid)
 {
     sid |= (mId << 16); // add the client-part to id
-    return mFlinger->destroySurface(sid);
+    return mFlinger->removeSurface(sid);
 }
 
 status_t BClient::setState(int32_t count, const layer_state_t* states)
diff --git a/libs/surfaceflinger/SurfaceFlinger.h b/libs/surfaceflinger/SurfaceFlinger.h
index cb84542..b488ab5 100644
--- a/libs/surfaceflinger/SurfaceFlinger.h
+++ b/libs/surfaceflinger/SurfaceFlinger.h
@@ -203,7 +203,8 @@ private:
             Client* client, DisplayID display,
             int32_t id, uint32_t w, uint32_t h, uint32_t flags);
 
-    status_t destroySurface(SurfaceID surface_id);
+    status_t removeSurface(SurfaceID surface_id);
+    status_t destroySurface(const sp<LayerBaseClient>& layer);
     status_t setClientState(ClientID cid, int32_t count, const layer_state_t* states);
 
 
@@ -287,6 +288,7 @@ private:
             sp<LayerBaseClient> getLayerUser_l(SurfaceID index) const;
             status_t    addLayer_l(const sp<LayerBase>& layer);
             status_t    removeLayer_l(const sp<LayerBase>& layer);
+            status_t    purgatorizeLayer_l(const sp<LayerBase>& layer);
             void        free_resources_l();
 
             uint32_t    getTransactionFlags(uint32_t flags);
@@ -315,7 +317,9 @@ private:
     volatile    int32_t                 mTransactionFlags;
     volatile    int32_t                 mTransactionCount;
                 Condition               mTransactionCV;
+                SortedVector< sp<LayerBase> > mLayerPurgatory;
 
+                
                 // protected by mStateLock (but we could use another lock)
                 Tokenizer                               mTokens;
                 DefaultKeyedVector<ClientID, Client*>   mClientsMap;