From 76cd4ddc6ad664257739b3d3713fd9ebdc9a4ad9 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Fri, 14 Jan 2011 17:37:42 -0800 Subject: [PATCH] partially fix [3306150] HTML5 video with H/W acceleration blackout 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. Change-Id: I305c830dd722b30a6d53cbf3a9c714fd3cf7eb06 --- services/surfaceflinger/SurfaceFlinger.cpp | 19 ++++++++++++++++++- services/surfaceflinger/SurfaceFlinger.h | 1 + 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index c982ea589..b41100015 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -1099,8 +1099,12 @@ status_t SurfaceFlinger::removeLayer_l(const sp& layerBase) status_t SurfaceFlinger::purgatorizeLayer_l(const sp& 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(); @@ -1349,6 +1353,19 @@ status_t SurfaceFlinger::destroySurface(const sp& 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; diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 48642d4ed..549e28221 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -359,6 +359,7 @@ private: volatile int32_t mTransactionFlags; volatile int32_t mTransactionCount; Condition mTransactionCV; + SortedVector< sp > mLayerPurgatory; bool mResizeTransationPending; // protected by mStateLock (but we could use another lock)