SurfaceFlinger: Select which layer state to visit

Modifies the traverseIn[Reverse]ZOrder methods to also take an enum
value specifying whether to traverse the current state or the drawing
state.

This has the effect of fixing a bug where we weren't performing
transactions on a child layer because its parent was only visiting its
drawing layers (rather than its current layers) and was thus skipping
the child, which had not yet been moved from current to drawing.

Bug: 36858924
Test: ChildLayerTest.Bug36858924 doesn't hang
Change-Id: I1959f40bc07e77864ba024511d429592a398a67a

9 files changed, 115 insertions, 48 deletions

diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index 406debf703..d33d3704e8 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -1033,7 +1033,7 @@ void Layer::onDraw(const sp<const DisplayDevice>& hw, const Region& clip,
         // figure out if there is something below us
         Region under;
         bool finished = false;
-        mFlinger->mDrawingState.layersSortedByZ.traverseInZOrder([&](Layer* layer) {
+        mFlinger->mDrawingState.traverseInZOrder([&](Layer* layer) {
             if (finished || layer == static_cast<Layer const*>(this)) {
                 finished = true;
                 return;
@@ -2519,7 +2519,7 @@ bool Layer::reparentChildren(const sp<IBinder>& newParentHandle) {
 }
 
 bool Layer::detachChildren() {
-    traverseInZOrder([this](Layer* child) {
+    traverseInZOrder(LayerVector::StateSet::Drawing, [this](Layer* child) {
         if (child == this) {
             return;
         }
@@ -2553,20 +2553,26 @@ int32_t Layer::getZ() const {
     return mDrawingState.z;
 }
 
-LayerVector Layer::makeTraversalList() {
-    if (mDrawingState.zOrderRelatives.size() == 0) {
-        return mDrawingChildren;
+LayerVector Layer::makeTraversalList(LayerVector::StateSet stateSet) {
+    LOG_ALWAYS_FATAL_IF(stateSet == LayerVector::StateSet::Invalid,
+                        "makeTraversalList received invalid stateSet");
+    const bool useDrawing = stateSet == LayerVector::StateSet::Drawing;
+    const LayerVector& children = useDrawing ? mDrawingChildren : mCurrentChildren;
+    const State& state = useDrawing ? mDrawingState : mCurrentState;
+
+    if (state.zOrderRelatives.size() == 0) {
+        return children;
     }
     LayerVector traverse;
 
-    for (const wp<Layer>& weakRelative : mDrawingState.zOrderRelatives) {
+    for (const wp<Layer>& weakRelative : state.zOrderRelatives) {
         sp<Layer> strongRelative = weakRelative.promote();
         if (strongRelative != nullptr) {
             traverse.add(strongRelative);
         }
     }
 
-    for (const sp<Layer>& child : mDrawingChildren) {
+    for (const sp<Layer>& child : children) {
         traverse.add(child);
     }
 
@@ -2576,8 +2582,8 @@ LayerVector Layer::makeTraversalList() {
 /**
  * Negatively signed relatives are before 'this' in Z-order.
  */
-void Layer::traverseInZOrder(const std::function<void(Layer*)>& exec) {
-    LayerVector list = makeTraversalList();
+void Layer::traverseInZOrder(LayerVector::StateSet stateSet, const LayerVector::Visitor& visitor) {
+    LayerVector list = makeTraversalList(stateSet);
 
     size_t i = 0;
     for (; i < list.size(); i++) {
@@ -2585,20 +2591,21 @@ void Layer::traverseInZOrder(const std::function<void(Layer*)>& exec) {
         if (relative->getZ() >= 0) {
             break;
         }
-        relative->traverseInZOrder(exec);
+        relative->traverseInZOrder(stateSet, visitor);
     }
-    exec(this);
+    visitor(this);
     for (; i < list.size(); i++) {
         const auto& relative = list[i];
-        relative->traverseInZOrder(exec);
+        relative->traverseInZOrder(stateSet, visitor);
     }
 }
 
 /**
  * Positively signed relatives are before 'this' in reverse Z-order.
  */
-void Layer::traverseInReverseZOrder(const std::function<void(Layer*)>& exec) {
-    LayerVector list = makeTraversalList();
+void Layer::traverseInReverseZOrder(LayerVector::StateSet stateSet,
+                                    const LayerVector::Visitor& visitor) {
+    LayerVector list = makeTraversalList(stateSet);
 
     int32_t i = 0;
     for (i = list.size()-1; i>=0; i--) {
@@ -2606,12 +2613,12 @@ void Layer::traverseInReverseZOrder(const std::function<void(Layer*)>& exec) {
         if (relative->getZ() < 0) {
             break;
         }
-        relative->traverseInReverseZOrder(exec);
+        relative->traverseInReverseZOrder(stateSet, visitor);
     }
-    exec(this);
+    visitor(this);
     for (; i>=0; i--) {
         const auto& relative = list[i];
-        relative->traverseInReverseZOrder(exec);
+        relative->traverseInReverseZOrder(stateSet, visitor);
     }
 }
 
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index a5224ecb4b..9f45435327 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -476,8 +476,9 @@ public:
     uint8_t getAlpha() const;
 #endif
 
-    void traverseInReverseZOrder(const std::function<void(Layer*)>& exec);
-    void traverseInZOrder(const std::function<void(Layer*)>& exec);
+    void traverseInReverseZOrder(LayerVector::StateSet stateSet,
+                                 const LayerVector::Visitor& visitor);
+    void traverseInZOrder(LayerVector::StateSet stateSet, const LayerVector::Visitor& visitor);
 
     void addChild(const sp<Layer>& layer);
     // Returns index if removed, or negative value otherwise
@@ -557,7 +558,7 @@ private:
 
     void setParent(const sp<Layer>& layer);
 
-    LayerVector makeTraversalList();
+    LayerVector makeTraversalList(LayerVector::StateSet stateSet);
     void addZOrderRelative(const wp<Layer>& relative);
     void removeZOrderRelative(const wp<Layer>& relative);
 
diff --git a/services/surfaceflinger/LayerVector.cpp b/services/surfaceflinger/LayerVector.cpp
index 90e6395ea1..2233e78c2b 100644
--- a/services/surfaceflinger/LayerVector.cpp
+++ b/services/surfaceflinger/LayerVector.cpp
@@ -18,9 +18,14 @@
 #include "Layer.h"
 
 namespace android {
+
+LayerVector::LayerVector() = default;
+
 LayerVector::LayerVector(const LayerVector& rhs) : SortedVector<sp<Layer>>(rhs) {
 }
 
+LayerVector::~LayerVector() = default;
+
 int LayerVector::do_compare(const void* lhs, const void* rhs) const
 {
     // sort layers per layer-stack, then by z-order and finally by sequence
@@ -40,23 +45,27 @@ int LayerVector::do_compare(const void* lhs, const void* rhs) const
     return l->sequence - r->sequence;
 }
 
-void LayerVector::traverseInZOrder(const std::function<void(Layer*)>& consume) const {
+void LayerVector::traverseInZOrder(StateSet stateSet, const Visitor& visitor) const {
     for (size_t i = 0; i < size(); i++) {
         const auto& layer = (*this)[i];
-        if (layer->getDrawingState().zOrderRelativeOf != nullptr) {
+        auto& state = (stateSet == StateSet::Current) ? layer->getCurrentState()
+                                                      : layer->getDrawingState();
+        if (state.zOrderRelativeOf != nullptr) {
             continue;
         }
-        layer->traverseInZOrder(consume);
+        layer->traverseInZOrder(stateSet, visitor);
     }
 }
 
-void LayerVector::traverseInReverseZOrder(const std::function<void(Layer*)>& consume) const {
+void LayerVector::traverseInReverseZOrder(StateSet stateSet, const Visitor& visitor) const {
     for (auto i = static_cast<int64_t>(size()) - 1; i >= 0; i--) {
         const auto& layer = (*this)[i];
-        if (layer->getDrawingState().zOrderRelativeOf != nullptr) {
+        auto& state = (stateSet == StateSet::Current) ? layer->getCurrentState()
+                                                      : layer->getDrawingState();
+        if (state.zOrderRelativeOf != nullptr) {
             continue;
         }
-        layer->traverseInReverseZOrder(consume);
+        layer->traverseInReverseZOrder(stateSet, visitor);
      }
 }
 } // namespace android
diff --git a/services/surfaceflinger/LayerVector.h b/services/surfaceflinger/LayerVector.h
index 7dfa4a0350..a9adb4113a 100644
--- a/services/surfaceflinger/LayerVector.h
+++ b/services/surfaceflinger/LayerVector.h
@@ -32,13 +32,22 @@ class Layer;
  */
 class LayerVector : public SortedVector<sp<Layer>> {
 public:
-    LayerVector() = default;
+    LayerVector();
     LayerVector(const LayerVector& rhs);
+    ~LayerVector() override;
+
+    enum class StateSet {
+        Invalid,
+        Current,
+        Drawing,
+    };
+
     // Sorts layer by layer-stack, Z order, and finally creation order (sequence).
     int do_compare(const void* lhs, const void* rhs) const override;
 
-    void traverseInReverseZOrder(const std::function<void(Layer*)>& consume) const;
-    void traverseInZOrder(const std::function<void(Layer*)>& consume) const;
+    using Visitor = std::function<void(Layer*)>;
+    void traverseInReverseZOrder(StateSet stateSet, const Visitor& visitor) const;
+    void traverseInZOrder(StateSet stateSet, const Visitor& visitor) const;
 };
 }
 
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 9cd1214be6..bc4e6c5696 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -4195,7 +4195,7 @@ void SurfaceFlinger::renderScreenImplLocked(
         if (state.z < minLayerZ || state.z > maxLayerZ) {
             continue;
         }
-        layer->traverseInZOrder([&](Layer* layer) {
+        layer->traverseInZOrder(LayerVector::StateSet::Drawing, [&](Layer* layer) {
             if (!layer->isVisible()) {
                 return;
             }
@@ -4243,7 +4243,7 @@ status_t SurfaceFlinger::captureScreenImplLocked(
                 (state.z < minLayerZ || state.z > maxLayerZ)) {
             continue;
         }
-        layer->traverseInZOrder([&](Layer *layer) {
+        layer->traverseInZOrder(LayerVector::StateSet::Drawing, [&](Layer *layer) {
             secureLayerIsVisible = secureLayerIsVisible || (layer->isVisible() &&
                     layer->isSecure());
         });
@@ -4391,7 +4391,7 @@ void SurfaceFlinger::checkScreenshot(size_t w, size_t s, size_t h, void const* v
             const Layer::State& state(layer->getDrawingState());
             if (layer->getLayerStack() == hw->getLayerStack() && state.z >= minLayerZ &&
                     state.z <= maxLayerZ) {
-                layer->traverseInZOrder([&](Layer* layer) {
+                layer->traverseInZOrder(LayerVector::StateSet::Drawing, [&](Layer* layer) {
                     ALOGE("%c index=%zu, name=%s, layerStack=%d, z=%d, visible=%d, flags=%x, alpha=%.3f",
                             layer->isVisible() ? '+' : '-',
                             i, layer->getName().string(), layer->getLayerStack(), state.z,
@@ -4405,12 +4405,12 @@ void SurfaceFlinger::checkScreenshot(size_t w, size_t s, size_t h, void const* v
 
 // ---------------------------------------------------------------------------
 
-void SurfaceFlinger::State::traverseInZOrder(const std::function<void(Layer*)>& consume) const {
-    layersSortedByZ.traverseInZOrder(consume);
+void SurfaceFlinger::State::traverseInZOrder(const LayerVector::Visitor& visitor) const {
+    layersSortedByZ.traverseInZOrder(stateSet, visitor);
 }
 
-void SurfaceFlinger::State::traverseInReverseZOrder(const std::function<void(Layer*)>& consume) const {
-    layersSortedByZ.traverseInReverseZOrder(consume);
+void SurfaceFlinger::State::traverseInReverseZOrder(const LayerVector::Visitor& visitor) const {
+    layersSortedByZ.traverseInReverseZOrder(stateSet, visitor);
 }
 
 }; // namespace android
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index c01f70122a..1bc689dc79 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -238,11 +238,21 @@ private:
 
     class State {
     public:
+        explicit State(LayerVector::StateSet set) : stateSet(set) {}
+        State& operator=(const State& other) {
+            // We explicitly don't copy stateSet so that, e.g., mDrawingState
+            // always uses the Drawing StateSet.
+            layersSortedByZ = other.layersSortedByZ;
+            displays = other.displays;
+            return *this;
+        }
+
+        const LayerVector::StateSet stateSet = LayerVector::StateSet::Invalid;
         LayerVector layersSortedByZ;
         DefaultKeyedVector< wp<IBinder>, DisplayDeviceState> displays;
 
-        void traverseInZOrder(const std::function<void(Layer*)>& consume) const;
-        void traverseInReverseZOrder(const std::function<void(Layer*)>& consume) const;
+        void traverseInZOrder(const LayerVector::Visitor& visitor) const;
+        void traverseInReverseZOrder(const LayerVector::Visitor& visitor) const;
     };
 
     /* ------------------------------------------------------------------------
@@ -577,7 +587,7 @@ private:
 
     // access must be protected by mStateLock
     mutable Mutex mStateLock;
-    State mCurrentState;
+    State mCurrentState{LayerVector::StateSet::Current};
     volatile int32_t mTransactionFlags;
     Condition mTransactionCV;
     bool mTransactionPending;
@@ -617,7 +627,7 @@ private:
 
     // Can only accessed from the main thread, these members
     // don't need synchronization
-    State mDrawingState;
+    State mDrawingState{LayerVector::StateSet::Drawing};
     bool mVisibleRegionsDirty;
 #ifndef USE_HWC2
     bool mHwWorkListDirty;
diff --git a/services/surfaceflinger/SurfaceFlinger_hwc1.cpp b/services/surfaceflinger/SurfaceFlinger_hwc1.cpp
index 02923ae5bb..019c8e31ff 100644
--- a/services/surfaceflinger/SurfaceFlinger_hwc1.cpp
+++ b/services/surfaceflinger/SurfaceFlinger_hwc1.cpp
@@ -3808,7 +3808,7 @@ void SurfaceFlinger::renderScreenImplLocked(
         if (state.z < minLayerZ || state.z > maxLayerZ) {
             continue;
         }
-        layer->traverseInZOrder([&](Layer* layer) {
+        layer->traverseInZOrder(LayerVector::StateSet::Drawing, [&](Layer* layer) {
             if (!layer->isVisible()) {
                 return;
             }
@@ -3858,7 +3858,7 @@ status_t SurfaceFlinger::captureScreenImplLocked(
                 (state.z < minLayerZ || state.z > maxLayerZ)) {
             continue;
         }
-        layer->traverseInZOrder([&](Layer *layer) {
+        layer->traverseInZOrder(LayerVector::StateSet::Drawing, [&](Layer *layer) {
             secureLayerIsVisible = secureLayerIsVisible || (layer->isVisible() &&
                     layer->isSecure());
         });
@@ -3997,7 +3997,7 @@ void SurfaceFlinger::checkScreenshot(size_t w, size_t s, size_t h, void const* v
             const Layer::State& state(layer->getDrawingState());
             if (layer->getLayerStack() == hw->getLayerStack() && state.z >= minLayerZ &&
                     state.z <= maxLayerZ) {
-                layer->traverseInZOrder([&](Layer* layer) {
+                layer->traverseInZOrder(LayerVector::StateSet::Drawing, [&](Layer* layer) {
                     ALOGE("%c index=%zu, name=%s, layerStack=%d, z=%d, visible=%d, flags=%x, alpha=%x",
                             layer->isVisible() ? '+' : '-',
                             i, layer->getName().string(), layer->getLayerStack(), state.z,
@@ -4011,12 +4011,12 @@ void SurfaceFlinger::checkScreenshot(size_t w, size_t s, size_t h, void const* v
 
 // ---------------------------------------------------------------------------
 
-void SurfaceFlinger::State::traverseInZOrder(const std::function<void(Layer*)>& consume) const {
-    layersSortedByZ.traverseInZOrder(consume);
+void SurfaceFlinger::State::traverseInZOrder(const LayerVector::Visitor& visitor) const {
+    layersSortedByZ.traverseInZOrder(stateSet, visitor);
 }
 
-void SurfaceFlinger::State::traverseInReverseZOrder(const std::function<void(Layer*)>& consume) const {
-    layersSortedByZ.traverseInReverseZOrder(consume);
+void SurfaceFlinger::State::traverseInReverseZOrder(const LayerVector::Visitor& visitor) const {
+    layersSortedByZ.traverseInReverseZOrder(stateSet, visitor);
 }
 
 }; // namespace android
diff --git a/services/surfaceflinger/SurfaceInterceptor.cpp b/services/surfaceflinger/SurfaceInterceptor.cpp
index 026fe803c0..db489b2456 100644
--- a/services/surfaceflinger/SurfaceInterceptor.cpp
+++ b/services/surfaceflinger/SurfaceInterceptor.cpp
@@ -80,7 +80,7 @@ void SurfaceInterceptor::saveExistingDisplaysLocked(
 void SurfaceInterceptor::saveExistingSurfacesLocked(const SortedVector<sp<Layer>>& layers) {
     ATRACE_CALL();
     for (const auto& l : layers) {
-        l->traverseInZOrder([this](Layer* layer) {
+        l->traverseInZOrder(LayerVector::StateSet::Drawing, [this](Layer* layer) {
             addSurfaceCreationLocked(createTraceIncrementLocked(), layer);
             addInitialSurfaceStateLocked(createTraceIncrementLocked(), layer);
         });
diff --git a/services/surfaceflinger/tests/Transaction_test.cpp b/services/surfaceflinger/tests/Transaction_test.cpp
index 5474fda3b7..1b17e55b30 100644
--- a/services/surfaceflinger/tests/Transaction_test.cpp
+++ b/services/surfaceflinger/tests/Transaction_test.cpp
@@ -977,4 +977,35 @@ TEST_F(ChildLayerTest, ChildrenWithParentBufferTransform) {
     }
 }
 
+TEST_F(ChildLayerTest, Bug36858924) {
+    // Destroy the child layer
+    mChild.clear();
+
+    // Now recreate it as hidden
+    mChild = mComposerClient->createSurface(String8("Child surface"), 10, 10,
+                                            PIXEL_FORMAT_RGBA_8888, ISurfaceComposerClient::eHidden,
+                                            mFGSurfaceControl.get());
+
+    // Show the child layer in a deferred transaction
+    SurfaceComposerClient::openGlobalTransaction();
+    mChild->deferTransactionUntil(mFGSurfaceControl->getHandle(),
+                                  mFGSurfaceControl->getSurface()->getNextFrameNumber());
+    mChild->show();
+    SurfaceComposerClient::closeGlobalTransaction(true);
+
+    // Render the foreground surface a few times
+    //
+    // Prior to the bugfix for b/36858924, this would usually hang while trying to fill the third
+    // frame because SurfaceFlinger would never process the deferred transaction and would therefore
+    // never acquire/release the first buffer
+    ALOGI("Filling 1");
+    fillSurfaceRGBA8(mFGSurfaceControl, 0, 255, 0);
+    ALOGI("Filling 2");
+    fillSurfaceRGBA8(mFGSurfaceControl, 0, 0, 255);
+    ALOGI("Filling 3");
+    fillSurfaceRGBA8(mFGSurfaceControl, 255, 0, 0);
+    ALOGI("Filling 4");
+    fillSurfaceRGBA8(mFGSurfaceControl, 0, 255, 0);
+}
+
 }