Surfaces are now destroyed properly in SurfaceFlinger.

First, the window manager tells us when a surface is no longer needed. At this point, several things happen:
- the surface is removed from the active/visible list
- it is added to a purgatory list, where it waits for all clients to release their reference
- it destroys all data/state that can be spared

Later, when all clients are done, the remains of the Surface are disposed off: it is removed from the purgatory and destroyed.
In particular its gralloc buffers are destroyed at that point (when we're sure nobody is using them anymore).

5 files changed, 51 insertions, 47 deletions

diff --git a/libs/surfaceflinger/Layer.cpp b/libs/surfaceflinger/Layer.cpp
index 5fdec3f64e3a..4e3f3a9b8435 100644
--- a/libs/surfaceflinger/Layer.cpp
+++ b/libs/surfaceflinger/Layer.cpp
@@ -61,15 +61,23 @@ Layer::Layer(SurfaceFlinger* flinger, DisplayID display, Client* c, int32_t i)
 
 Layer::~Layer()
 {
+    destroy();
+    // the actual buffers will be destroyed here
+}
+
+void Layer::destroy()
+{
     for (int i=0 ; i<NUM_BUFFERS ; i++) {
         if (mTextures[i].name != -1U) {
             // FIXME: this was originally to work-around a bug in the
             // adreno driver. this should be fixed now.
             deletedTextures.add(mTextures[i].name);
+            mTextures[i].name = -1U;
         }
         if (mTextures[i].image != EGL_NO_IMAGE_KHR) {
             EGLDisplay dpy(mFlinger->graphicPlane(0).getEGLDisplay());
             eglDestroyImageKHR(dpy, mTextures[i].image);
+            mTextures[i].image = EGL_NO_IMAGE_KHR;
         }
     }
 }
@@ -89,7 +97,9 @@ sp<LayerBaseClient::Surface> Layer::createSurface() const
 
 status_t Layer::ditch()
 {
+    // the layer is not on screen anymore. free as much resources as possible
     mSurface.clear();
+    destroy();
     return NO_ERROR;
 }
 
diff --git a/libs/surfaceflinger/Layer.h b/libs/surfaceflinger/Layer.h
index 3f3953fb9133..e16d9f40d953 100644
--- a/libs/surfaceflinger/Layer.h
+++ b/libs/surfaceflinger/Layer.h
@@ -104,7 +104,8 @@ private:
     status_t resize(int32_t index, uint32_t w, uint32_t h, const char* what);
     Region post(uint32_t* oldState, bool& recomputeVisibleRegions);
     sp<SurfaceBuffer> peekBuffer();
-    
+    void destroy();
+
     
     class SurfaceLayer : public LayerBaseClient::Surface
     {
diff --git a/libs/surfaceflinger/LayerBase.h b/libs/surfaceflinger/LayerBase.h
index ccff36d36cf1..2ab6f6721bf3 100644
--- a/libs/surfaceflinger/LayerBase.h
+++ b/libs/surfaceflinger/LayerBase.h
@@ -207,7 +207,8 @@ public:
      */
     virtual bool isSecure() const       { return false; }
 
-    /** signal this layer that it's not needed any longer */
+    /** signal this layer that it's not needed any longer. called from the 
+     * main thread */
     virtual status_t ditch() { return NO_ERROR; }
 
     
diff --git a/libs/surfaceflinger/MessageQueue.h b/libs/surfaceflinger/MessageQueue.h
index d894fe28d43b..c118897dd699 100644
--- a/libs/surfaceflinger/MessageQueue.h
+++ b/libs/surfaceflinger/MessageQueue.h
@@ -148,8 +148,9 @@ public:
     uint32_t    what;
     int32_t     arg0;    
 
-    MessageBase(uint32_t what=0, int32_t arg0=0)
-    : when(0), what(what), arg0(arg0) { }
+    MessageBase() : when(0), what(0), arg0(0) { }
+    MessageBase(uint32_t what, int32_t arg0=0)
+        : when(0), what(what), arg0(arg0) { }
     
     // return true if message has a handler
     virtual bool handler() { return false; }
diff --git a/libs/surfaceflinger/SurfaceFlinger.cpp b/libs/surfaceflinger/SurfaceFlinger.cpp
index 1b6d6def59c5..993a6ae02a2d 100644
--- a/libs/surfaceflinger/SurfaceFlinger.cpp
+++ b/libs/surfaceflinger/SurfaceFlinger.cpp
@@ -639,16 +639,26 @@ void SurfaceFlinger::handleTransaction(uint32_t transactionFlags)
             mFreezeDisplay = mCurrentState.freezeDisplay;
         }
 
-        // some layers might have been removed, so
-        // we need to update the regions they're exposing.
-        if (mLayersRemoved) {
+        if (currentLayers.size() > mDrawingState.layersSortedByZ.size()) {
+            // layers have been added
             mVisibleRegionsDirty = true;
         }
 
-        const LayerVector& currentLayers = mCurrentState.layersSortedByZ;
-        if (currentLayers.size() > mDrawingState.layersSortedByZ.size()) {
-            // layers have been added
+        // some layers might have been removed, so
+        // we need to update the regions they're exposing.
+        if (mLayersRemoved) {
             mVisibleRegionsDirty = true;
+            const LayerVector& previousLayers(mDrawingState.layersSortedByZ);
+            const ssize_t count = previousLayers.size();
+            for (ssize_t i=0 ; i<count ; i++) {
+                const sp<LayerBase>& layer(previousLayers[i]);
+                if (currentLayers.indexOf( layer ) < 0) {
+                    // this layer is not visible anymore
+                    // FIXME: would be better to call without the lock held
+                    //LOGD("ditching layer %p", layer.get());
+                    layer->ditch();
+                }
+            }
         }
 
         // get rid of all resources we don't need anymore
@@ -1275,27 +1285,17 @@ status_t SurfaceFlinger::removeSurface(SurfaceID index)
     /*
      * called by the window manager, when a surface should be marked for
      * destruction.
+     * 
+     * The surface is removed from the current and drawing lists, but placed
+     * in the purgatory queue, so it's not destroyed right-away (we need
+     * to wait for all client's references to go away first).
      */
-    
-    // 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();
+    Mutex::Autolock _l(mStateLock);
+    sp<LayerBaseClient> layer = getLayerUser_l(index);
+    status_t err = purgatorizeLayer_l(layer);
+    if (err == NO_ERROR) {
+        setTransactionFlags(eTransactionNeeded);
     }
     return err;
 }
@@ -1304,35 +1304,26 @@ status_t SurfaceFlinger::destroySurface(const sp<LayerBaseClient>& layer)
 {
     /*
      * called by ~ISurface() when all references are gone
-     */
-
-    /* FIXME: 
-     * - 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.
+     * 
+     * the surface must be removed from purgatory from the main thread
+     * since its dtor must run from there (b/c of OpenGL ES).
      */
     
     class MessageDestroySurface : public MessageBase {
-        sp<SurfaceFlinger> flinger;
+        SurfaceFlinger* flinger;
         sp<LayerBaseClient> layer;
     public:
-        MessageDestroySurface(const sp<SurfaceFlinger>& flinger,
-                const sp<LayerBaseClient>& layer)
-            : MessageBase(0), flinger(flinger), layer(layer) {
-        }
-        ~MessageDestroySurface() {
-            //LOGD("~MessageDestroySurface, layer=%p", layer.get());
-        }
+        MessageDestroySurface(
+                SurfaceFlinger* flinger, const sp<LayerBaseClient>& layer)
+            : flinger(flinger), layer(layer) { }
         virtual bool handler() {
-            //LOGD("MessageDestroySurface handler, layer=%p", layer.get());
             Mutex::Autolock _l(flinger->mStateLock);
-            flinger->mLayerPurgatory.remove(layer);
+            ssize_t idx = flinger->mLayerPurgatory.remove(layer);
+            LOGE_IF(idx<0, "layer=%p is not in the purgatory list", layer.get());
             return true;
         }
     };
-
     mEventQueue.postMessage( new MessageDestroySurface(this, layer) );
-    
     return NO_ERROR;
 }