partially fix [3306150] HTML5 video with H/W acceleration blackout (DO NOT MERGE)

We used to guarantee that a layer in SurfaceFlinger would never be
destroyed before all references (to its ISurface) on the client
side would be released. At some point, this guarantee got
relaxed to allow to free gralloc resources sooner. This last
change was incorrect, because:
- in implementations with reference-counting the gralloc resources
wouldn't be released anyways, until all the mapping were gone
- in implementations without ref counting, the client side
would most likely crash or do something bad
- it also caused the SharedBufferStack slot to be reallocated
to another surface, which could be problematic if the client
continued to use the surface after the window manager destroyed it.

So, we essentially reinstate the guarantee that layers won't be
destroyed until after all references to their ISurface are
released.

NOTE: This doesn't entirely fix 3306150 because there is another
problem there where the Browser continues to use a surface after it
has been destroyed.

also improve SurfaceFlinger 'dumpsys' log

list the purgatory, which shows windows that have been closed,
but for which the client still has references.

diff --git a/libs/ui/GraphicBufferAllocator.cpp b/libs/ui/GraphicBufferAllocator.cpp
index fa46ab7..9847a5f 100644 (file)
--- a/libs/ui/GraphicBufferAllocator.cpp
+++ b/libs/ui/GraphicBufferAllocator.cpp
@@ -63,7 +63,7 @@ void GraphicBufferAllocator::dump(String8& result) const
     const size_t c = list.size();
     for (size_t i=0 ; i<c ; i++) {
         const alloc_rec_t& rec(list.valueAt(i));
-        snprintf(buffer, SIZE, "%10p: %7.2f KiB | %4u (%4u) x %4u | %2d | 0x%08x\n",
+        snprintf(buffer, SIZE, "%10p: %7.2f KiB | %4u (%4u) x %4u | %8X | 0x%08x\n",
             list.keyAt(i), rec.size/1024.0f, 
             rec.w, rec.s, rec.h, rec.format, rec.usage);
         result.append(buffer);

diff --git a/services/surfaceflinger/LayerBase.cpp b/services/surfaceflinger/LayerBase.cpp
index 79c6d0d..49b7c06 100644 (file)
--- a/services/surfaceflinger/LayerBase.cpp
+++ b/services/surfaceflinger/LayerBase.cpp
@@ -519,6 +519,12 @@ void LayerBase::dump(String8& result, char* buffer, size_t SIZE) const
     result.append(buffer);
 }
 
+void LayerBase::shortDump(String8& result, char* scratch, size_t size) const
+{
+    LayerBase::dump(result, scratch, size);
+}
+
+
 // ---------------------------------------------------------------------------
 
 int32_t LayerBaseClient::sIdentity = 1;
@@ -570,6 +576,12 @@ void LayerBaseClient::dump(String8& result, char* buffer, size_t SIZE) const
     result.append(buffer);
 }
 
+
+void LayerBaseClient::shortDump(String8& result, char* scratch, size_t size) const
+{
+    LayerBaseClient::dump(result, scratch, size);
+}
+
 // ---------------------------------------------------------------------------
 
 LayerBaseClient::Surface::Surface(

diff --git a/services/surfaceflinger/LayerBase.h b/services/surfaceflinger/LayerBase.h
index 3ec8ac3..aea5754 100644 (file)
--- a/services/surfaceflinger/LayerBase.h
+++ b/services/surfaceflinger/LayerBase.h
@@ -211,6 +211,7 @@ public:
     
     /** always call base class first */
     virtual void dump(String8& result, char* scratch, size_t size) const;
+    virtual void shortDump(String8& result, char* scratch, size_t size) const;
 
 
     enum { // flags for doTransaction()
@@ -330,6 +331,7 @@ public:
 
 protected:
     virtual void dump(String8& result, char* scratch, size_t size) const;
+    virtual void shortDump(String8& result, char* scratch, size_t size) const;
 
 private:
     mutable Mutex mLock;

diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 50f4914..3f07d39 100644 (file)
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -1082,8 +1082,12 @@ status_t SurfaceFlinger::removeLayer_l(const sp<LayerBase>& layerBase)
 
 status_t SurfaceFlinger::purgatorizeLayer_l(const sp<LayerBase>& layerBase)
 {
-    // remove the layer from the main list (through a transaction).
+    // 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);
+    }
 
     layerBase->onRemoved();
 
@@ -1354,6 +1358,19 @@ status_t SurfaceFlinger::destroySurface(const sp<LayerBaseClient>& layer)
              * to use the purgatory.
              */
             status_t err = flinger->removeLayer_l(l);
+            if (err == NAME_NOT_FOUND) {
+                // The surface wasn't in the current list, which means it was
+                // removed already, which means it is in the purgatory,
+                // and need to be removed from there.
+                // This needs to happen from the main thread since its dtor
+                // must run from there (b/c of OpenGL ES). Additionally, we
+                // can't really acquire our internal lock from
+                // destroySurface() -- see postMessage() below.
+                ssize_t idx = flinger->mLayerPurgatory.remove(l);
+                LOGE_IF(idx < 0,
+                        "layer=%p is not in the purgatory list", l.get());
+            }
+
             LOGE_IF(err<0 && err != NAME_NOT_FOUND,
                     "error removing layer=%p (%s)", l.get(), strerror(-err));
             return true;
@@ -1469,8 +1486,13 @@ status_t SurfaceFlinger::dump(int fd, const Vector<String16>& args)
             result.append(buffer);
         }
 
+        /*
+         * Dump the visible layer list
+         */
         const LayerVector& currentLayers = mCurrentState.layersSortedByZ;
         const size_t count = currentLayers.size();
+        snprintf(buffer, SIZE, "Visible layers (count = %d)\n", count);
+        result.append(buffer);
         for (size_t i=0 ; i<count ; i++) {
             const sp<LayerBase>& layer(currentLayers[i]);
             layer->dump(result, buffer, SIZE);
@@ -1480,6 +1502,24 @@ status_t SurfaceFlinger::dump(int fd, const Vector<String16>& args)
             layer->visibleRegionScreen.dump(result, "visibleRegionScreen");
         }
 
+        /*
+         * Dump the layers in the purgatory
+         */
+
+        const size_t purgatorySize =  mLayerPurgatory.size();
+        snprintf(buffer, SIZE, "Purgatory state (%d entries)\n", purgatorySize);
+        result.append(buffer);
+        for (size_t i=0 ; i<purgatorySize ; i++) {
+            const sp<LayerBase>& layer(mLayerPurgatory.itemAt(i));
+            layer->shortDump(result, buffer, SIZE);
+        }
+
+        /*
+         * Dump SurfaceFlinger global state
+         */
+
+        snprintf(buffer, SIZE, "SurfaceFlinger global state\n");
+        result.append(buffer);
         mWormholeRegion.dump(result, "WormholeRegion");
         const DisplayHardware& hw(graphicPlane(0).displayHardware());
         snprintf(buffer, SIZE,
@@ -1505,6 +1545,9 @@ status_t SurfaceFlinger::dump(int fd, const Vector<String16>& args)
             result.append(buffer);
         }
 
+        /*
+         * Dump gralloc state
+         */
         const GraphicBufferAllocator& alloc(GraphicBufferAllocator::get());
         alloc.dump(result);
 

diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index ca57292..714a1b7 100644 (file)
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -371,6 +371,7 @@ private:
     volatile    int32_t                 mTransactionFlags;
     volatile    int32_t                 mTransactionCount;
                 Condition               mTransactionCV;
+                SortedVector< sp<LayerBase> > mLayerPurgatory;
                 bool                    mResizeTransationPending;
 
                 // protected by mStateLock (but we could use another lock)