LayerTraceGenerator: Fix duplicate layer ids
Layers can be created from API calls on the binder thread or
by surfaceflinger on the main thread for mirroring and
BSL background layers. Transaction traces keep track of
layer creation API calls to recreate layers when generating
the layer trace.
However, there was a possibility that new layers would be
recorded with a set of transactions that were applied with
an earlier vsync. This is harmless except we could end up
creating layers with duplicate layer ids since layers created
via binder would get an injected sequence id while main
thread created layers would use the global counter.
Test: atest transactiontrace_testsuite
Bug: 235376060
Change-Id: Ia11839d1880bc131ab5314779281c93393d6742f
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 3043809..79320cd 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2056,6 +2056,11 @@
mGpuFrameMissedCount++;
}
+ if (mTracingEnabledChanged) {
+ mLayerTracingEnabled = mLayerTracing.isEnabled();
+ mTracingEnabledChanged = false;
+ }
+
// If we are in the middle of a mode change and the fence hasn't
// fired yet just wait for the next commit.
if (mSetActiveModePending) {
@@ -2104,11 +2109,6 @@
}
}
- if (mTracingEnabledChanged) {
- mLayerTracingEnabled = mLayerTracing.isEnabled();
- mTracingEnabledChanged = false;
- }
-
if (mRefreshRateOverlaySpinner) {
Mutex::Autolock lock(mStateLock);
if (const auto display = getDefaultDisplayDeviceLocked()) {
@@ -2123,7 +2123,7 @@
bool needsTraversal = false;
if (clearTransactionFlags(eTransactionFlushNeeded)) {
- needsTraversal |= commitCreatedLayers();
+ needsTraversal |= commitCreatedLayers(vsyncId);
needsTraversal |= flushTransactionQueues(vsyncId);
}
@@ -7154,7 +7154,7 @@
return calculateMaxAcquiredBufferCount(refreshRate, presentLatency);
}
-void SurfaceFlinger::handleLayerCreatedLocked(const LayerCreatedState& state) {
+void SurfaceFlinger::handleLayerCreatedLocked(const LayerCreatedState& state, int64_t vsyncId) {
sp<Layer> layer = state.layer.promote();
if (!layer) {
ALOGD("Layer was destroyed soon after creation %p", state.layer.unsafe_get());
@@ -7184,7 +7184,9 @@
}
layer->updateTransformHint(mActiveDisplayTransformHint);
-
+ if (mTransactionTracing) {
+ mTransactionTracing->onLayerAddedToDrawingState(layer->getSequence(), vsyncId);
+ }
mInterceptor->saveSurfaceCreation(layer);
}
@@ -7268,7 +7270,7 @@
return buffer;
}
-bool SurfaceFlinger::commitCreatedLayers() {
+bool SurfaceFlinger::commitCreatedLayers(int64_t vsyncId) {
std::vector<LayerCreatedState> createdLayers;
{
std::scoped_lock<std::mutex> lock(mCreatedLayersLock);
@@ -7281,7 +7283,7 @@
Mutex::Autolock _l(mStateLock);
for (const auto& createdLayer : createdLayers) {
- handleLayerCreatedLocked(createdLayer);
+ handleLayerCreatedLocked(createdLayer, vsyncId);
}
createdLayers.clear();
mLayersAdded = true;
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 41de2b6..f7e061f 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -1405,8 +1405,9 @@
// A temporay pool that store the created layers and will be added to current state in main
// thread.
std::vector<LayerCreatedState> mCreatedLayers GUARDED_BY(mCreatedLayersLock);
- bool commitCreatedLayers();
- void handleLayerCreatedLocked(const LayerCreatedState& state) REQUIRES(mStateLock);
+ bool commitCreatedLayers(int64_t vsyncId);
+ void handleLayerCreatedLocked(const LayerCreatedState& state, int64_t vsyncId)
+ REQUIRES(mStateLock);
std::atomic<ui::Transform::RotationFlags> mActiveDisplayTransformHint;
diff --git a/services/surfaceflinger/Tracing/TransactionProtoParser.cpp b/services/surfaceflinger/Tracing/TransactionProtoParser.cpp
index 8ec6c99..65918f6 100644
--- a/services/surfaceflinger/Tracing/TransactionProtoParser.cpp
+++ b/services/surfaceflinger/Tracing/TransactionProtoParser.cpp
@@ -498,8 +498,13 @@
inputInfo.replaceTouchableRegionWithCrop =
windowInfoProto.replace_touchable_region_with_crop();
int64_t layerId = windowInfoProto.crop_layer_id();
- inputInfo.touchableRegionCropHandle =
- mMapper->getLayerHandle(static_cast<int32_t>(layerId));
+ if (layerId != -1) {
+ inputInfo.touchableRegionCropHandle =
+ mMapper->getLayerHandle(static_cast<int32_t>(layerId));
+ } else {
+ inputInfo.touchableRegionCropHandle = nullptr;
+ }
+
layer.windowInfoHandle = sp<gui::WindowInfoHandle>::make(inputInfo);
}
if (proto.what() & layer_state_t::eBackgroundColorChanged) {
diff --git a/services/surfaceflinger/Tracing/TransactionTracing.cpp b/services/surfaceflinger/Tracing/TransactionTracing.cpp
index 6381758..e53feca 100644
--- a/services/surfaceflinger/Tracing/TransactionTracing.cpp
+++ b/services/surfaceflinger/Tracing/TransactionTracing.cpp
@@ -152,17 +152,33 @@
mTransactionQueue.push(state);
}
-void TransactionTracing::addCommittedTransactions(std::vector<TransactionState>& transactions,
- int64_t vsyncId) {
+TransactionTracing::CommittedTransactions&
+TransactionTracing::findOrCreateCommittedTransactionRecord(int64_t vsyncId) {
+ for (auto& pendingTransaction : mPendingTransactions) {
+ if (pendingTransaction.vsyncId == vsyncId) {
+ return pendingTransaction;
+ }
+ }
+
CommittedTransactions committedTransactions;
committedTransactions.vsyncId = vsyncId;
committedTransactions.timestamp = systemTime();
+ mPendingTransactions.emplace_back(committedTransactions);
+ return mPendingTransactions.back();
+}
+
+void TransactionTracing::onLayerAddedToDrawingState(int layerId, int64_t vsyncId) {
+ CommittedTransactions& committedTransactions = findOrCreateCommittedTransactionRecord(vsyncId);
+ committedTransactions.createdLayerIds.emplace_back(layerId);
+}
+
+void TransactionTracing::addCommittedTransactions(std::vector<TransactionState>& transactions,
+ int64_t vsyncId) {
+ CommittedTransactions& committedTransactions = findOrCreateCommittedTransactionRecord(vsyncId);
committedTransactions.transactionIds.reserve(transactions.size());
for (const auto& transaction : transactions) {
committedTransactions.transactionIds.emplace_back(transaction.id);
}
-
- mPendingTransactions.emplace_back(committedTransactions);
tryPushToTracingThread();
}
@@ -235,15 +251,24 @@
for (const CommittedTransactions& entry : committedTransactions) {
entryProto.set_elapsed_realtime_nanos(entry.timestamp);
entryProto.set_vsync_id(entry.vsyncId);
- entryProto.mutable_added_layers()->Reserve(static_cast<int32_t>(mCreatedLayers.size()));
- for (auto& newLayer : mCreatedLayers) {
- entryProto.mutable_added_layers()->Add(std::move(newLayer));
+ entryProto.mutable_added_layers()->Reserve(
+ static_cast<int32_t>(entry.createdLayerIds.size()));
+
+ for (const int32_t& id : entry.createdLayerIds) {
+ auto it = mCreatedLayers.find(id);
+ if (it != mCreatedLayers.end()) {
+ entryProto.mutable_added_layers()->Add(std::move(it->second));
+ mCreatedLayers.erase(it);
+ } else {
+ ALOGW("Could not created layer with id %d", id);
+ }
}
+
entryProto.mutable_removed_layers()->Reserve(static_cast<int32_t>(removedLayers.size()));
for (auto& removedLayer : removedLayers) {
entryProto.mutable_removed_layers()->Add(removedLayer);
+ mCreatedLayers.erase(removedLayer);
}
- mCreatedLayers.clear();
entryProto.mutable_transactions()->Reserve(
static_cast<int32_t>(entry.transactionIds.size()));
for (const uint64_t& id : entry.transactionIds) {
@@ -304,7 +329,7 @@
ALOGW("Duplicate handles found. %p", layerHandle);
}
mLayerHandles[layerHandle] = layerId;
- mCreatedLayers.push_back(mProtoParser.toProto(args));
+ mCreatedLayers[layerId] = mProtoParser.toProto(args);
}
void TransactionTracing::onMirrorLayerAdded(BBinder* layerHandle, int layerId,
@@ -315,7 +340,7 @@
ALOGW("Duplicate handles found. %p", layerHandle);
}
mLayerHandles[layerHandle] = layerId;
- mCreatedLayers.emplace_back(mProtoParser.toProto(args));
+ mCreatedLayers[layerId] = mProtoParser.toProto(args);
}
void TransactionTracing::onLayerRemoved(int32_t layerId) {
diff --git a/services/surfaceflinger/Tracing/TransactionTracing.h b/services/surfaceflinger/Tracing/TransactionTracing.h
index 4c291f9..2f5ee87 100644
--- a/services/surfaceflinger/Tracing/TransactionTracing.h
+++ b/services/surfaceflinger/Tracing/TransactionTracing.h
@@ -64,6 +64,7 @@
int mirrorFromId);
void onLayerRemoved(int layerId);
void onHandleRemoved(BBinder* layerHandle);
+ void onLayerAddedToDrawingState(int layerId, int64_t vsyncId);
void dump(std::string&) const;
static constexpr auto CONTINUOUS_TRACING_BUFFER_SIZE = 512 * 1024;
static constexpr auto ACTIVE_TRACING_BUFFER_SIZE = 100 * 1024 * 1024;
@@ -81,7 +82,7 @@
GUARDED_BY(mTraceLock);
LocklessStack<proto::TransactionState> mTransactionQueue;
nsecs_t mStartingTimestamp GUARDED_BY(mTraceLock);
- std::vector<proto::LayerCreationArgs> mCreatedLayers GUARDED_BY(mTraceLock);
+ std::unordered_map<int, proto::LayerCreationArgs> mCreatedLayers GUARDED_BY(mTraceLock);
std::unordered_map<BBinder* /* layerHandle */, int32_t /* layerId */> mLayerHandles
GUARDED_BY(mTraceLock);
std::vector<int32_t /* layerId */> mRemovedLayerHandles GUARDED_BY(mTraceLock);
@@ -100,6 +101,7 @@
std::condition_variable mTransactionsAddedToBufferCv;
struct CommittedTransactions {
std::vector<uint64_t> transactionIds;
+ std::vector<int32_t> createdLayerIds;
int64_t vsyncId;
int64_t timestamp;
};
@@ -117,7 +119,7 @@
void tryPushToTracingThread() EXCLUDES(mMainThreadLock);
void addStartingStateToProtoLocked(proto::TransactionTraceFile& proto) REQUIRES(mTraceLock);
void updateStartingStateLocked(const proto::TransactionTraceEntry& entry) REQUIRES(mTraceLock);
-
+ CommittedTransactions& findOrCreateCommittedTransactionRecord(int64_t vsyncId);
// TEST
// Wait until all the committed transactions for the specified vsync id are added to the buffer.
void flush(int64_t vsyncId) EXCLUDES(mMainThreadLock);
diff --git a/services/surfaceflinger/Tracing/tools/LayerTraceGenerator.cpp b/services/surfaceflinger/Tracing/tools/LayerTraceGenerator.cpp
index 5bb9d2f..ea33eb5 100644
--- a/services/surfaceflinger/Tracing/tools/LayerTraceGenerator.cpp
+++ b/services/surfaceflinger/Tracing/tools/LayerTraceGenerator.cpp
@@ -248,7 +248,7 @@
(dataMapper->mLayerHandles.find(tracingArgs.parentId) ==
dataMapper->mLayerHandles.end())) {
args.addToRoot = false;
- } else {
+ } else if (tracingArgs.parentId != -1) {
parentHandle = dataMapper->getLayerHandle(tracingArgs.parentId);
}
mFlinger.createLayer(args, &outHandle, parentHandle, &outLayerId,
diff --git a/services/surfaceflinger/tests/tracing/TransactionTraceTestSuite.cpp b/services/surfaceflinger/tests/tracing/TransactionTraceTestSuite.cpp
index ac4354c..0e214af 100644
--- a/services/surfaceflinger/tests/tracing/TransactionTraceTestSuite.cpp
+++ b/services/surfaceflinger/tests/tracing/TransactionTraceTestSuite.cpp
@@ -83,6 +83,37 @@
std::vector<std::filesystem::path> TransactionTraceTestSuite::sTransactionTraces{};
+struct LayerInfo {
+ int id;
+ std::string name;
+ int parent;
+ int z;
+ uint64_t curr_frame;
+ float x;
+ float y;
+};
+
+bool operator==(const LayerInfo& lh, const LayerInfo& rh) {
+ return std::make_tuple(lh.id, lh.name, lh.parent, lh.z, lh.curr_frame) ==
+ std::make_tuple(rh.id, rh.name, rh.parent, rh.z, rh.curr_frame);
+}
+
+bool compareById(const LayerInfo& a, const LayerInfo& b) {
+ return a.id < b.id;
+}
+
+inline void PrintTo(const LayerInfo& info, ::std::ostream* os) {
+ *os << "Layer [" << info.id << "] name=" << info.name << " parent=" << info.parent
+ << " z=" << info.z << " curr_frame=" << info.curr_frame << " x=" << info.x
+ << " y=" << info.y;
+}
+
+struct find_id : std::unary_function<LayerInfo, bool> {
+ int id;
+ find_id(int id) : id(id) {}
+ bool operator()(LayerInfo const& m) const { return m.id == id; }
+};
+
TEST_P(TransactionTraceTestSuite, validateEndState) {
ASSERT_GT(mActualLayersTraceProto.entry_size(), 0);
ASSERT_GT(mExpectedLayersTraceProto.entry_size(), 0);
@@ -92,19 +123,64 @@
auto actualLastEntry = mActualLayersTraceProto.entry(mActualLayersTraceProto.entry_size() - 1);
EXPECT_EQ(expectedLastEntry.layers().layers_size(), actualLastEntry.layers().layers_size());
- for (int i = 0;
- i < expectedLastEntry.layers().layers_size() && i < actualLastEntry.layers().layers_size();
- i++) {
- auto expectedLayer = expectedLastEntry.layers().layers(i);
- auto actualLayer = actualLastEntry.layers().layers(i);
- EXPECT_EQ(expectedLayer.id(), actualLayer.id());
- EXPECT_EQ(expectedLayer.name(), actualLayer.name());
- EXPECT_EQ(expectedLayer.parent(), actualLayer.parent());
- EXPECT_EQ(expectedLayer.z(), actualLayer.z());
- EXPECT_EQ(expectedLayer.curr_frame(), actualLayer.curr_frame());
- ALOGV("Validating %s[%d] parent=%d z=%d frame=%" PRIu64, expectedLayer.name().c_str(),
- expectedLayer.id(), expectedLayer.parent(), expectedLayer.z(),
- expectedLayer.curr_frame());
+
+ 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);
+ expectedLayers.push_back({layer.id(), layer.name(), layer.parent(), layer.z(),
+ layer.curr_frame(),
+ layer.has_position() ? layer.position().x() : -1,
+ layer.has_position() ? layer.position().y() : -1});
+ }
+ std::sort(expectedLayers.begin(), expectedLayers.end(), compareById);
+
+ 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);
+ actualLayers.push_back({layer.id(), layer.name(), layer.parent(), layer.z(),
+ layer.curr_frame(),
+ layer.has_position() ? layer.position().x() : -1,
+ layer.has_position() ? layer.position().y() : -1});
+ }
+ std::sort(actualLayers.begin(), actualLayers.end(), compareById);
+
+ size_t i = 0;
+ for (; i < actualLayers.size() && i < expectedLayers.size(); i++) {
+ auto it = std::find_if(actualLayers.begin(), actualLayers.end(),
+ find_id(expectedLayers[i].id));
+ EXPECT_NE(it, actualLayers.end());
+ EXPECT_EQ(expectedLayers[i], *it);
+ ALOGV("Validating %s[%d] parent=%d z=%d frame=%" PRIu64, expectedLayers[i].name.c_str(),
+ expectedLayers[i].id, expectedLayers[i].parent, expectedLayers[i].z,
+ expectedLayers[i].curr_frame);
+ }
+
+ EXPECT_EQ(expectedLayers.size(), actualLayers.size());
+
+ if (i < actualLayers.size()) {
+ for (size_t j = 0; j < actualLayers.size(); j++) {
+ if (std::find_if(expectedLayers.begin(), expectedLayers.end(),
+ find_id(actualLayers[j].id)) == expectedLayers.end()) {
+ ALOGD("actualLayers [%d]:%s parent=%d z=%d frame=%" PRIu64, actualLayers[j].id,
+ actualLayers[j].name.c_str(), actualLayers[j].parent, actualLayers[j].z,
+ actualLayers[j].curr_frame);
+ }
+ }
+ FAIL();
+ }
+
+ if (i < expectedLayers.size()) {
+ for (size_t j = 0; j < expectedLayers.size(); j++) {
+ if (std::find_if(actualLayers.begin(), actualLayers.end(),
+ find_id(expectedLayers[j].id)) == actualLayers.end()) {
+ ALOGD("expectedLayers [%d]:%s parent=%d z=%d frame=%" PRIu64, expectedLayers[j].id,
+ expectedLayers[j].name.c_str(), expectedLayers[j].parent, expectedLayers[j].z,
+ expectedLayers[j].curr_frame);
+ }
+ }
+ FAIL();
}
}
diff --git a/services/surfaceflinger/tests/unittests/TransactionTracingTest.cpp b/services/surfaceflinger/tests/unittests/TransactionTracingTest.cpp
index 61b72a0..8ec918d 100644
--- a/services/surfaceflinger/tests/unittests/TransactionTracingTest.cpp
+++ b/services/surfaceflinger/tests/unittests/TransactionTracingTest.cpp
@@ -127,6 +127,8 @@
std::vector<TransactionState> transactions;
transactions.emplace_back(transaction);
VSYNC_ID_FIRST_LAYER_CHANGE = ++mVsyncId;
+ mTracing.onLayerAddedToDrawingState(mParentLayerId, VSYNC_ID_FIRST_LAYER_CHANGE);
+ mTracing.onLayerAddedToDrawingState(mChildLayerId, VSYNC_ID_FIRST_LAYER_CHANGE);
mTracing.addCommittedTransactions(transactions, VSYNC_ID_FIRST_LAYER_CHANGE);
flush(VSYNC_ID_FIRST_LAYER_CHANGE);
}
@@ -238,6 +240,8 @@
const sp<IBinder> fakeMirrorLayerHandle = new BBinder();
mTracing.onMirrorLayerAdded(fakeMirrorLayerHandle->localBinder(), mMirrorLayerId, "Mirror",
mLayerId);
+ mTracing.onLayerAddedToDrawingState(mLayerId, mVsyncId);
+ mTracing.onLayerAddedToDrawingState(mMirrorLayerId, mVsyncId);
// add some layer transaction
{
@@ -257,7 +261,7 @@
std::vector<TransactionState> transactions;
transactions.emplace_back(transaction);
- mTracing.addCommittedTransactions(transactions, ++mVsyncId);
+ mTracing.addCommittedTransactions(transactions, mVsyncId);
flush(mVsyncId);
}
}