Improve Planner stability
This fixes two crashes in Planner:
1. During device rotation, an Output's layer stack may change, which
invalidates OutputLayer pointers. When updating a LayerState, swap in the
updated OutputLayer pointer as well to keep references from going stale.
2. Retain the set of removedLayers until after Flattener has updated its
internal state in flattenLayers(). Otherwise, removed LayerStates may be
deleted which invalidates dangling pointers in the Flattener.
Bug: 188743022
Test: builds
Test: libcompositionengine_test
Test: rotate Chrome
Change-Id: I889703a2675bc0eddadd344f1a550d93d7f4c8df
diff --git a/services/surfaceflinger/CompositionEngine/src/planner/LayerState.cpp b/services/surfaceflinger/CompositionEngine/src/planner/LayerState.cpp
index 8423a12..936dba3 100644
--- a/services/surfaceflinger/CompositionEngine/src/planner/LayerState.cpp
+++ b/services/surfaceflinger/CompositionEngine/src/planner/LayerState.cpp
@@ -42,7 +42,13 @@
}
Flags<LayerStateField> LayerState::update(compositionengine::OutputLayer* layer) {
- ALOGE_IF(layer != mOutputLayer, "[%s] Expected mOutputLayer to never change", __func__);
+ ALOGE_IF(mOutputLayer != layer && layer->getLayerFE().getSequence() != mId.get(),
+ "[%s] Expected mOutputLayer ID to never change: %d, %d", __func__,
+ layer->getLayerFE().getSequence(), mId.get());
+
+ // It's possible for the OutputLayer pointer to change even when the layer is logically the
+ // same, i.e., the LayerFE is the same. An example use-case is screen rotation.
+ mOutputLayer = layer;
Flags<LayerStateField> differences;
diff --git a/services/surfaceflinger/CompositionEngine/src/planner/Planner.cpp b/services/surfaceflinger/CompositionEngine/src/planner/Planner.cpp
index be2510f..f077470 100644
--- a/services/surfaceflinger/CompositionEngine/src/planner/Planner.cpp
+++ b/services/surfaceflinger/CompositionEngine/src/planner/Planner.cpp
@@ -113,15 +113,6 @@
}
}
- for (LayerId removedLayer : removedLayers) {
- if (const auto layerEntry = mPreviousLayers.find(removedLayer);
- layerEntry != mPreviousLayers.end()) {
- const auto& [id, state] = *layerEntry;
- ALOGV("Removed layer %s", state.getName().c_str());
- mPreviousLayers.erase(removedLayer);
- }
- }
-
mCurrentLayers.clear();
mCurrentLayers.reserve(currentLayerIds.size());
std::transform(currentLayerIds.cbegin(), currentLayerIds.cend(),
@@ -135,6 +126,7 @@
mFlattenedHash =
mFlattener.flattenLayers(mCurrentLayers, hash, std::chrono::steady_clock::now());
const bool layersWereFlattened = hash != mFlattenedHash;
+
ALOGV("[%s] Initial hash %zx flattened hash %zx", __func__, hash, mFlattenedHash);
if (mPredictorEnabled) {
@@ -148,6 +140,17 @@
ALOGV("[%s] No prediction found\n", __func__);
}
}
+
+ // Clean up the set of previous layers now that the view of the LayerStates in the flattener are
+ // up-to-date.
+ for (LayerId removedLayer : removedLayers) {
+ if (const auto layerEntry = mPreviousLayers.find(removedLayer);
+ layerEntry != mPreviousLayers.end()) {
+ const auto& [id, state] = *layerEntry;
+ ALOGV("Removed layer %s", state.getName().c_str());
+ mPreviousLayers.erase(removedLayer);
+ }
+ }
}
void Planner::reportFinalPlan(
diff --git a/services/surfaceflinger/CompositionEngine/tests/planner/LayerStateTest.cpp b/services/surfaceflinger/CompositionEngine/tests/planner/LayerStateTest.cpp
index a09ce14..9ad3ab4 100644
--- a/services/surfaceflinger/CompositionEngine/tests/planner/LayerStateTest.cpp
+++ b/services/surfaceflinger/CompositionEngine/tests/planner/LayerStateTest.cpp
@@ -117,6 +117,22 @@
EXPECT_EQ(&mOutputLayer, mLayerState->getOutputLayer());
}
+TEST_F(LayerStateTest, updateOutputLayer) {
+ OutputLayerCompositionState outputLayerCompositionState;
+ LayerFECompositionState layerFECompositionState;
+ setupMocksForLayer(mOutputLayer, mLayerFE, outputLayerCompositionState,
+ layerFECompositionState);
+ mLayerState = std::make_unique<LayerState>(&mOutputLayer);
+ EXPECT_EQ(&mOutputLayer, mLayerState->getOutputLayer());
+
+ mock::OutputLayer newOutputLayer;
+ mock::LayerFE newLayerFE;
+ setupMocksForLayer(newOutputLayer, newLayerFE, outputLayerCompositionState,
+ layerFECompositionState);
+ mLayerState->update(&newOutputLayer);
+ EXPECT_EQ(&newOutputLayer, mLayerState->getOutputLayer());
+}
+
TEST_F(LayerStateTest, getId) {
OutputLayerCompositionState outputLayerCompositionState;
LayerFECompositionState layerFECompositionState;