[layertracegenerator] be more resilient to unknown layer ids
While unexpected, the layer lifecycle in legacy which is used to
generate the transaction traces is less explicit. We rely on layer
destruction when the object becomes unreachable. The new
front end logic will crash if called with an unknown layer id.
While we have to interop between both sets of logic, log and
ignore the unknown layer ids so we can still generate useful
layer traces.
Fixes: 275630566
Test: presbumit
Change-Id: I5b2a217c78b4a9e94f0772459d6aa0c68a8a51d0
diff --git a/services/surfaceflinger/FrontEnd/LayerCreationArgs.cpp b/services/surfaceflinger/FrontEnd/LayerCreationArgs.cpp
index ce21233..6af352c 100644
--- a/services/surfaceflinger/FrontEnd/LayerCreationArgs.cpp
+++ b/services/surfaceflinger/FrontEnd/LayerCreationArgs.cpp
@@ -74,4 +74,23 @@
return LayerCreationArgs(other.flinger, other.client, other.name, other.flags, other.metadata);
}
+std::string LayerCreationArgs::getDebugString() const {
+ std::stringstream stream;
+ stream << "LayerCreationArgs{" << name << "[" << sequence << "] flags=" << flags
+ << " pid=" << ownerPid << " uid=" << ownerUid;
+ if (addToRoot) {
+ stream << " addToRoot=" << addToRoot;
+ }
+ if (parentId != UNASSIGNED_LAYER_ID) {
+ stream << " parentId=" << parentId;
+ }
+ if (layerIdToMirror != UNASSIGNED_LAYER_ID) {
+ stream << " layerIdToMirror=" << layerIdToMirror;
+ }
+ if (layerStackToMirror != ui::INVALID_LAYER_STACK) {
+ stream << " layerStackToMirror=" << layerStackToMirror.id;
+ }
+ return stream.str();
+}
+
} // namespace android::surfaceflinger
diff --git a/services/surfaceflinger/FrontEnd/LayerCreationArgs.h b/services/surfaceflinger/FrontEnd/LayerCreationArgs.h
index 011250c..3a0fc6d 100644
--- a/services/surfaceflinger/FrontEnd/LayerCreationArgs.h
+++ b/services/surfaceflinger/FrontEnd/LayerCreationArgs.h
@@ -44,6 +44,7 @@
bool internalLayer = false);
LayerCreationArgs(std::optional<uint32_t> id, bool internalLayer = false);
LayerCreationArgs() = default; // for tracing
+ std::string getDebugString() const;
android::SurfaceFlinger* flinger;
sp<android::Client> client;
diff --git a/services/surfaceflinger/FrontEnd/LayerLifecycleManager.cpp b/services/surfaceflinger/FrontEnd/LayerLifecycleManager.cpp
index 3706225..33d9dbe 100644
--- a/services/surfaceflinger/FrontEnd/LayerLifecycleManager.cpp
+++ b/services/surfaceflinger/FrontEnd/LayerLifecycleManager.cpp
@@ -164,7 +164,8 @@
}
}
-void LayerLifecycleManager::applyTransactions(const std::vector<TransactionState>& transactions) {
+void LayerLifecycleManager::applyTransactions(const std::vector<TransactionState>& transactions,
+ bool ignoreUnknownLayers) {
for (const auto& transaction : transactions) {
for (const auto& resolvedComposerState : transaction.states) {
const auto& clientState = resolvedComposerState.state;
@@ -176,7 +177,8 @@
RequestedLayerState* layer = getLayerFromId(layerId);
if (layer == nullptr) {
- LOG_ALWAYS_FATAL("%s Layer with layerid=%d not found", __func__, layerId);
+ LOG_ALWAYS_FATAL_IF(!ignoreUnknownLayers, "%s Layer with layerid=%d not found",
+ __func__, layerId);
continue;
}
diff --git a/services/surfaceflinger/FrontEnd/LayerLifecycleManager.h b/services/surfaceflinger/FrontEnd/LayerLifecycleManager.h
index 3d9a74c..f258678 100644
--- a/services/surfaceflinger/FrontEnd/LayerLifecycleManager.h
+++ b/services/surfaceflinger/FrontEnd/LayerLifecycleManager.h
@@ -39,7 +39,11 @@
public:
// External state changes should be updated in the following order:
void addLayers(std::vector<std::unique_ptr<RequestedLayerState>>);
- void applyTransactions(const std::vector<TransactionState>&);
+ // Ignore unknown layers when interoping with legacy front end. In legacy we destroy
+ // the layers it is unreachable. When using the LayerLifecycleManager for layer trace
+ // generation we may encounter layers which are known because we don't have an explicit
+ // lifecycle. Ignore these errors while we have to interop with legacy.
+ void applyTransactions(const std::vector<TransactionState>&, bool ignoreUnknownLayers = false);
// Ignore unknown handles when iteroping with legacy front end. In the old world, we
// would create child layers which are not necessary with the new front end. This means
// we will get notified for handle changes that don't exist in the new front end.
diff --git a/services/surfaceflinger/Tracing/TransactionTracing.cpp b/services/surfaceflinger/Tracing/TransactionTracing.cpp
index 26ed878..87a633f 100644
--- a/services/surfaceflinger/Tracing/TransactionTracing.cpp
+++ b/services/surfaceflinger/Tracing/TransactionTracing.cpp
@@ -271,6 +271,7 @@
for (const proto::LayerState& layerState : transaction.layer_changes()) {
auto it = mStartingStates.find(layerState.layer_id());
if (it == mStartingStates.end()) {
+ // TODO(b/238781169) make this log fatal when we switch over to using new fe
ALOGW("Could not find layer id %d", layerState.layer_id());
continue;
}
diff --git a/services/surfaceflinger/Tracing/tools/LayerTraceGenerator.cpp b/services/surfaceflinger/Tracing/tools/LayerTraceGenerator.cpp
index 4b4ce18..55004c5 100644
--- a/services/surfaceflinger/Tracing/tools/LayerTraceGenerator.cpp
+++ b/services/surfaceflinger/Tracing/tools/LayerTraceGenerator.cpp
@@ -14,14 +14,6 @@
* limitations under the License.
*/
-#include <ios>
-#include <memory>
-#include <vector>
-#include "FrontEnd/LayerCreationArgs.h"
-#include "FrontEnd/RequestedLayerState.h"
-#include "Tracing/LayerTracing.h"
-#include "TransactionState.h"
-#include "cutils/properties.h"
#undef LOG_TAG
#define LOG_TAG "LayerTraceGenerator"
//#define LOG_NDEBUG 0
@@ -33,8 +25,15 @@
#include <utils/String16.h>
#include <filesystem>
#include <fstream>
+#include <ios>
#include <string>
+#include <vector>
+#include "FrontEnd/LayerCreationArgs.h"
+#include "FrontEnd/RequestedLayerState.h"
#include "LayerProtoHelper.h"
+#include "Tracing/LayerTracing.h"
+#include "TransactionState.h"
+#include "cutils/properties.h"
#include "LayerTraceGenerator.h"
@@ -84,6 +83,7 @@
for (int j = 0; j < entry.added_layers_size(); j++) {
LayerCreationArgs args;
parser.fromProto(entry.added_layers(j), args);
+ ALOGV(" %s", args.getDebugString().c_str());
addedLayers.emplace_back(std::make_unique<frontend::RequestedLayerState>(args));
}
@@ -105,9 +105,14 @@
transactions.emplace_back(std::move(transaction));
}
+ for (int j = 0; j < entry.destroyed_layers_size(); j++) {
+ ALOGV(" destroyedHandles=%d", entry.destroyed_layers(j));
+ }
+
std::vector<uint32_t> destroyedHandles;
destroyedHandles.reserve((size_t)entry.destroyed_layer_handles_size());
for (int j = 0; j < entry.destroyed_layer_handles_size(); j++) {
+ ALOGV(" destroyedHandles=%d", entry.destroyed_layer_handles(j));
destroyedHandles.push_back(entry.destroyed_layer_handles(j));
}
@@ -118,7 +123,7 @@
// apply updates
lifecycleManager.addLayers(std::move(addedLayers));
- lifecycleManager.applyTransactions(transactions);
+ lifecycleManager.applyTransactions(transactions, /*ignoreUnknownHandles=*/true);
lifecycleManager.onHandlesDestroyed(destroyedHandles, /*ignoreUnknownHandles=*/true);
if (lifecycleManager.getGlobalChanges().test(
diff --git a/services/surfaceflinger/tests/tracing/TransactionTraceTestSuite.cpp b/services/surfaceflinger/tests/tracing/TransactionTraceTestSuite.cpp
index 3e3d8f0..2b29530 100644
--- a/services/surfaceflinger/tests/tracing/TransactionTraceTestSuite.cpp
+++ b/services/surfaceflinger/tests/tracing/TransactionTraceTestSuite.cpp
@@ -148,6 +148,35 @@
touchableRegionBounds};
}
+static std::vector<LayerInfo> getLayerInfosFromProto(
+ android::surfaceflinger::LayersTraceProto& entry) {
+ std::unordered_map<int32_t /* snapshotId*/, int32_t /*layerId*/> snapshotIdToLayerId;
+ std::vector<LayerInfo> layers;
+ layers.reserve(static_cast<size_t>(entry.layers().layers_size()));
+ bool mapSnapshotIdToLayerId = false;
+ for (int i = 0; i < entry.layers().layers_size(); i++) {
+ auto layer = entry.layers().layers(i);
+ LayerInfo layerInfo = getLayerInfoFromProto(layer);
+
+ snapshotIdToLayerId[layerInfo.id] = static_cast<int32_t>(layer.original_id());
+ if (layer.original_id() != 0) {
+ mapSnapshotIdToLayerId = true;
+ }
+ layers.push_back(layerInfo);
+ }
+ std::sort(layers.begin(), layers.end(), compareById);
+
+ if (!mapSnapshotIdToLayerId) {
+ return layers;
+ }
+ for (auto& layer : layers) {
+ layer.id = snapshotIdToLayerId[layer.id];
+ auto it = snapshotIdToLayerId.find(layer.parent);
+ layer.parent = it == snapshotIdToLayerId.end() ? -1 : it->second;
+ }
+ return layers;
+}
+
TEST_P(TransactionTraceTestSuite, validateEndState) {
ASSERT_GT(mActualLayersTraceProto.entry_size(), 0);
ASSERT_GT(mExpectedLayersTraceProto.entry_size(), 0);
@@ -158,32 +187,9 @@
EXPECT_EQ(expectedLastEntry.layers().layers_size(), actualLastEntry.layers().layers_size());
- std::vector<LayerInfo> expectedLayers;
- expectedLayers.reserve(static_cast<size_t>(expectedLastEntry.layers().layers_size()));
- for (int i = 0; i < expectedLastEntry.layers().layers_size(); i++) {
- auto layer = expectedLastEntry.layers().layers(i);
- LayerInfo layerInfo = getLayerInfoFromProto(layer);
- expectedLayers.push_back(layerInfo);
- }
- std::sort(expectedLayers.begin(), expectedLayers.end(), compareById);
-
- std::unordered_map<int32_t /* snapshotId*/, int32_t /*layerId*/> snapshotIdToLayerId;
- std::vector<LayerInfo> actualLayers;
- actualLayers.reserve(static_cast<size_t>(actualLastEntry.layers().layers_size()));
- for (int i = 0; i < actualLastEntry.layers().layers_size(); i++) {
- auto layer = actualLastEntry.layers().layers(i);
- LayerInfo layerInfo = getLayerInfoFromProto(layer);
- snapshotIdToLayerId[layerInfo.id] = static_cast<int32_t>(layer.original_id());
- actualLayers.push_back(layerInfo);
- }
-
- for (auto& layer : actualLayers) {
- layer.id = snapshotIdToLayerId[layer.id];
- auto it = snapshotIdToLayerId.find(layer.parent);
- layer.parent = it == snapshotIdToLayerId.end() ? -1 : it->second;
- }
-
- std::sort(actualLayers.begin(), actualLayers.end(), compareById);
+ std::vector<LayerInfo> expectedLayers = getLayerInfosFromProto(expectedLastEntry);
+ std::vector<LayerInfo> actualLayers = getLayerInfosFromProto(actualLastEntry);
+ ;
size_t i = 0;
for (; i < actualLayers.size() && i < expectedLayers.size(); i++) {
diff --git a/services/surfaceflinger/tests/tracing/testdata/layers_trace_b275630566.winscope b/services/surfaceflinger/tests/tracing/testdata/layers_trace_b275630566.winscope
new file mode 100644
index 0000000..fe504d7
--- /dev/null
+++ b/services/surfaceflinger/tests/tracing/testdata/layers_trace_b275630566.winscope
Binary files differ
diff --git a/services/surfaceflinger/tests/tracing/testdata/transactions_trace_b275630566.winscope b/services/surfaceflinger/tests/tracing/testdata/transactions_trace_b275630566.winscope
new file mode 100644
index 0000000..6f7ba15
--- /dev/null
+++ b/services/surfaceflinger/tests/tracing/testdata/transactions_trace_b275630566.winscope
Binary files differ