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
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index 406debf..d33d370 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -1033,7 +1033,7 @@
// 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::detachChildren() {
- traverseInZOrder([this](Layer* child) {
+ traverseInZOrder(LayerVector::StateSet::Drawing, [this](Layer* child) {
if (child == this) {
return;
}
@@ -2553,20 +2553,26 @@
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 @@
/**
* 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 @@
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 @@
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 a5224ec..9f45435 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -476,8 +476,9 @@
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 @@
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 90e6395..2233e78 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 @@
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 7dfa4a0..a9adb41 100644
--- a/services/surfaceflinger/LayerVector.h
+++ b/services/surfaceflinger/LayerVector.h
@@ -32,13 +32,22 @@
*/
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 9cd1214..bc4e6c5 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -4195,7 +4195,7 @@
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 @@
(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 @@
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::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 c01f701..1bc689d 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -238,11 +238,21 @@
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 @@
// 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 @@
// 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 02923ae..019c8e3 100644
--- a/services/surfaceflinger/SurfaceFlinger_hwc1.cpp
+++ b/services/surfaceflinger/SurfaceFlinger_hwc1.cpp
@@ -3808,7 +3808,7 @@
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 @@
(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 @@
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::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 026fe80..db489b2 100644
--- a/services/surfaceflinger/SurfaceInterceptor.cpp
+++ b/services/surfaceflinger/SurfaceInterceptor.cpp
@@ -80,7 +80,7 @@
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 5474fda..1b17e55 100644
--- a/services/surfaceflinger/tests/Transaction_test.cpp
+++ b/services/surfaceflinger/tests/Transaction_test.cpp
@@ -977,4 +977,35 @@
}
}
+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);
+}
+
}