Skip to content

Commit f1a5340

Browse files
patrickdwilliamspatrickdwilliams
committed
Remove ScopedAddToTraversalPath
ScopedAddToTraversalPath is an RAII wrapper that copies the existing traversal path, modifies it, and then restores the modified properties from the copy when deleted. It's simpler to make the child path a copy and not modify the parent. Bug: 403312802 Flag: EXEMPT refactor Test: presubmits Change-Id: I8f06ed557f29444be6df51c1c8ea60958a82ee95
1 parent 2dc2afe commit f1a5340

6 files changed

Lines changed: 48 additions & 85 deletions

File tree

‎services/surfaceflinger/FrontEnd/LayerHierarchy.cpp‎

Lines changed: 18 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,8 @@ LayerHierarchy::LayerHierarchy(const LayerHierarchy& hierarchy, bool childrenOnl
5454
mChildren = hierarchy.mChildren;
5555
}
5656

57-
void LayerHierarchy::traverse(const Visitor& visitor, LayerHierarchy::TraversalPath& traversalPath,
57+
void LayerHierarchy::traverse(const Visitor& visitor,
58+
const LayerHierarchy::TraversalPath& traversalPath,
5859
uint32_t depth) const {
5960
LLOG_ALWAYS_FATAL_WITH_TRACE_IF(depth > 50,
6061
"Cycle detected in LayerHierarchy::traverse. See "
@@ -70,14 +71,13 @@ void LayerHierarchy::traverse(const Visitor& visitor, LayerHierarchy::TraversalP
7071
LLOG_ALWAYS_FATAL_WITH_TRACE_IF(traversalPath.hasRelZLoop(), "Found relative z loop layerId:%d",
7172
traversalPath.invalidRelativeRootId);
7273
for (auto& [child, childVariant] : mChildren) {
73-
ScopedAddToTraversalPath addChildToTraversalPath(traversalPath, child->mLayer->id,
74-
childVariant);
75-
child->traverse(visitor, traversalPath, depth + 1);
74+
child->traverse(visitor, traversalPath.makeChild(child->mLayer->id, childVariant),
75+
depth + 1);
7676
}
7777
}
7878

7979
void LayerHierarchy::traverseInZOrder(const Visitor& visitor,
80-
LayerHierarchy::TraversalPath& traversalPath) const {
80+
const LayerHierarchy::TraversalPath& traversalPath) const {
8181
bool traverseThisLayer = (mLayer != nullptr);
8282
for (auto it = mChildren.begin(); it < mChildren.end(); it++) {
8383
auto& [child, childVariant] = *it;
@@ -91,9 +91,7 @@ void LayerHierarchy::traverseInZOrder(const Visitor& visitor,
9191
if (childVariant == LayerHierarchy::Variant::Detached) {
9292
continue;
9393
}
94-
ScopedAddToTraversalPath addChildToTraversalPath(traversalPath, child->mLayer->id,
95-
childVariant);
96-
child->traverseInZOrder(visitor, traversalPath);
94+
child->traverseInZOrder(visitor, traversalPath.makeChild(child->mLayer->id, childVariant));
9795
}
9896

9997
if (traverseThisLayer) {
@@ -568,42 +566,23 @@ std::string LayerHierarchy::TraversalPath::toString() const {
568566
return ss.str();
569567
}
570568

571-
// Helper class to update a passed in TraversalPath when visiting a child. When the object goes out
572-
// of scope the TraversalPath is reset to its original state.
573-
LayerHierarchy::ScopedAddToTraversalPath::ScopedAddToTraversalPath(TraversalPath& traversalPath,
574-
uint32_t layerId,
575-
LayerHierarchy::Variant variant)
576-
: mTraversalPath(traversalPath), mParentPath(traversalPath) {
577-
// Update the traversal id with the child layer id and variant. Parent id and variant are
578-
// stored to reset the id upon destruction.
579-
traversalPath.id = layerId;
580-
traversalPath.variant = variant;
569+
LayerHierarchy::TraversalPath LayerHierarchy::TraversalPath::makeChild(
570+
uint32_t layerId, LayerHierarchy::Variant variant) const {
571+
TraversalPath child{*this};
572+
child.id = layerId;
573+
child.variant = variant;
581574
if (LayerHierarchy::isMirror(variant)) {
582-
traversalPath.mirrorRootIds.emplace_back(mParentPath.id);
575+
child.mirrorRootIds.emplace_back(id);
583576
} else if (variant == LayerHierarchy::Variant::Relative) {
584-
if (std::find(traversalPath.relativeRootIds.begin(), traversalPath.relativeRootIds.end(),
585-
layerId) != traversalPath.relativeRootIds.end()) {
586-
traversalPath.invalidRelativeRootId = layerId;
577+
if (std::find(relativeRootIds.begin(), relativeRootIds.end(), layerId) !=
578+
relativeRootIds.end()) {
579+
child.invalidRelativeRootId = layerId;
587580
}
588-
traversalPath.relativeRootIds.emplace_back(layerId);
581+
child.relativeRootIds.emplace_back(layerId);
589582
} else if (variant == LayerHierarchy::Variant::Detached) {
590-
traversalPath.detached = true;
583+
child.detached = true;
591584
}
592-
}
593-
LayerHierarchy::ScopedAddToTraversalPath::~ScopedAddToTraversalPath() {
594-
// Reset the traversal id to its original parent state using the state that was saved in
595-
// the constructor.
596-
if (LayerHierarchy::isMirror(mTraversalPath.variant)) {
597-
mTraversalPath.mirrorRootIds.pop_back();
598-
} else if (mTraversalPath.variant == LayerHierarchy::Variant::Relative) {
599-
mTraversalPath.relativeRootIds.pop_back();
600-
}
601-
if (mTraversalPath.invalidRelativeRootId == mTraversalPath.id) {
602-
mTraversalPath.invalidRelativeRootId = UNASSIGNED_LAYER_ID;
603-
}
604-
mTraversalPath.id = mParentPath.id;
605-
mTraversalPath.variant = mParentPath.variant;
606-
mTraversalPath.detached = mParentPath.detached;
585+
return child;
607586
}
608587

609588
} // namespace android::surfaceflinger::frontend

‎services/surfaceflinger/FrontEnd/LayerHierarchy.h‎

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,8 @@ class LayerHierarchy {
104104

105105
TraversalPath getClonedFrom() const { return {.id = id, .variant = variant}; }
106106

107+
TraversalPath makeChild(uint32_t layerId, LayerHierarchy::Variant variant) const;
108+
107109
bool operator==(const TraversalPath& other) const {
108110
return id == other.id && mirrorRootIds == other.mirrorRootIds;
109111
}
@@ -122,18 +124,6 @@ class LayerHierarchy {
122124
}
123125
};
124126

125-
// Helper class to add nodes to an existing traversal id and removes the
126-
// node when it goes out of scope.
127-
class ScopedAddToTraversalPath {
128-
public:
129-
ScopedAddToTraversalPath(TraversalPath& traversalPath, uint32_t layerId,
130-
LayerHierarchy::Variant variantArg);
131-
~ScopedAddToTraversalPath();
132-
133-
private:
134-
TraversalPath& mTraversalPath;
135-
TraversalPath mParentPath;
136-
};
137127
LayerHierarchy(RequestedLayerState* layer);
138128

139129
// Visitor function that provides the hierarchy node and a traversal id which uniquely
@@ -191,8 +181,9 @@ class LayerHierarchy {
191181
void removeChild(LayerHierarchy*);
192182
void sortChildrenByZOrder();
193183
void updateChild(LayerHierarchy*, LayerHierarchy::Variant);
194-
void traverseInZOrder(const Visitor& visitor, LayerHierarchy::TraversalPath& parent) const;
195-
void traverse(const Visitor& visitor, LayerHierarchy::TraversalPath& parent,
184+
void traverseInZOrder(const Visitor& visitor,
185+
const LayerHierarchy::TraversalPath& parent) const;
186+
void traverse(const Visitor& visitor, const LayerHierarchy::TraversalPath& parent,
196187
uint32_t depth = 0) const;
197188
void dump(std::ostream& out, const std::string& prefix, LayerHierarchy::Variant variant,
198189
bool isLastChild, bool includeMirroredHierarchy) const;

‎services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.cpp‎

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -447,15 +447,14 @@ void LayerSnapshotBuilder::updateSnapshots(const Args& args) {
447447
if (args.root.getLayer()) {
448448
// The hierarchy can have a root layer when used for screenshots otherwise, it will have
449449
// multiple children.
450-
LayerHierarchy::ScopedAddToTraversalPath addChildToPath(root, args.root.getLayer()->id,
451-
LayerHierarchy::Variant::Attached);
452-
updateSnapshotsInHierarchy(args, args.root, root, rootSnapshot, /*depth=*/0);
450+
LayerHierarchy::TraversalPath childPath =
451+
root.makeChild(args.root.getLayer()->id, LayerHierarchy::Variant::Attached);
452+
updateSnapshotsInHierarchy(args, args.root, childPath, rootSnapshot, /*depth=*/0);
453453
} else {
454454
for (auto& [childHierarchy, variant] : args.root.mChildren) {
455-
LayerHierarchy::ScopedAddToTraversalPath addChildToPath(root,
456-
childHierarchy->getLayer()->id,
457-
variant);
458-
updateSnapshotsInHierarchy(args, *childHierarchy, root, rootSnapshot, /*depth=*/0);
455+
LayerHierarchy::TraversalPath childPath =
456+
root.makeChild(childHierarchy->getLayer()->id, variant);
457+
updateSnapshotsInHierarchy(args, *childHierarchy, childPath, rootSnapshot, /*depth=*/0);
459458
}
460459
}
461460

@@ -520,7 +519,7 @@ void LayerSnapshotBuilder::update(const Args& args) {
520519

521520
const LayerSnapshot& LayerSnapshotBuilder::updateSnapshotsInHierarchy(
522521
const Args& args, const LayerHierarchy& hierarchy,
523-
LayerHierarchy::TraversalPath& traversalPath, const LayerSnapshot& parentSnapshot,
522+
const LayerHierarchy::TraversalPath& traversalPath, const LayerSnapshot& parentSnapshot,
524523
int depth) {
525524
LLOG_ALWAYS_FATAL_WITH_TRACE_IF(depth > 50,
526525
"Cycle detected in LayerSnapshotBuilder. See "
@@ -549,12 +548,10 @@ const LayerSnapshot& LayerSnapshotBuilder::updateSnapshotsInHierarchy(
549548

550549
bool childHasValidFrameRate = false;
551550
for (auto& [childHierarchy, variant] : hierarchy.mChildren) {
552-
LayerHierarchy::ScopedAddToTraversalPath addChildToPath(traversalPath,
553-
childHierarchy->getLayer()->id,
554-
variant);
551+
LayerHierarchy::TraversalPath childPath =
552+
traversalPath.makeChild(childHierarchy->getLayer()->id, variant);
555553
const LayerSnapshot& childSnapshot =
556-
updateSnapshotsInHierarchy(args, *childHierarchy, traversalPath, *snapshot,
557-
depth + 1);
554+
updateSnapshotsInHierarchy(args, *childHierarchy, childPath, *snapshot, depth + 1);
558555
updateFrameRateFromChildSnapshot(*snapshot, childSnapshot, *childHierarchy->getLayer(),
559556
args, &childHasValidFrameRate);
560557
}

‎services/surfaceflinger/FrontEnd/LayerSnapshotBuilder.h‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -106,9 +106,10 @@ class LayerSnapshotBuilder {
106106

107107
void updateSnapshots(const Args& args);
108108

109-
const LayerSnapshot& updateSnapshotsInHierarchy(const Args&, const LayerHierarchy& hierarchy,
110-
LayerHierarchy::TraversalPath& traversalPath,
111-
const LayerSnapshot& parentSnapshot, int depth);
109+
const LayerSnapshot& updateSnapshotsInHierarchy(
110+
const Args&, const LayerHierarchy& hierarchy,
111+
const LayerHierarchy::TraversalPath& traversalPath, const LayerSnapshot& parentSnapshot,
112+
int depth);
112113
void updateSnapshot(LayerSnapshot&, const Args&, const RequestedLayerState&,
113114
const LayerSnapshot& parentSnapshot, const LayerHierarchy::TraversalPath&);
114115
static void updateRelativeState(LayerSnapshot& snapshot, const LayerSnapshot& parentSnapshot,

‎services/surfaceflinger/LayerProtoHelper.cpp‎

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -278,10 +278,9 @@ LayerProtoFromSnapshotGenerator& LayerProtoFromSnapshotGenerator::with(
278278
stackIdsToSkip.find(child->getLayer()->layerStack.id) != stackIdsToSkip.end()) {
279279
continue;
280280
}
281-
frontend::LayerHierarchy::ScopedAddToTraversalPath addChildToPath(path,
282-
child->getLayer()->id,
283-
variant);
284-
LayerProtoFromSnapshotGenerator::writeHierarchyToProto(*child, path);
281+
LayerProtoFromSnapshotGenerator::writeHierarchyToProto(*child,
282+
path.makeChild(child->getLayer()->id,
283+
variant));
285284
}
286285

287286
// fill in relative and parent info
@@ -338,7 +337,8 @@ LayerProtoFromSnapshotGenerator& LayerProtoFromSnapshotGenerator::withOffscreenL
338337
}
339338

340339
frontend::LayerSnapshot* LayerProtoFromSnapshotGenerator::getSnapshot(
341-
frontend::LayerHierarchy::TraversalPath& path, const frontend::RequestedLayerState& layer) {
340+
const frontend::LayerHierarchy::TraversalPath& path,
341+
const frontend::RequestedLayerState& layer) {
342342
frontend::LayerSnapshot* snapshot = mSnapshotBuilder.getSnapshot(path);
343343
if (snapshot) {
344344
return snapshot;
@@ -349,7 +349,7 @@ frontend::LayerSnapshot* LayerProtoFromSnapshotGenerator::getSnapshot(
349349
}
350350

351351
void LayerProtoFromSnapshotGenerator::writeHierarchyToProto(
352-
const frontend::LayerHierarchy& root, frontend::LayerHierarchy::TraversalPath& path) {
352+
const frontend::LayerHierarchy& root, const frontend::LayerHierarchy::TraversalPath& path) {
353353
using Variant = frontend::LayerHierarchy::Variant;
354354
perfetto::protos::LayerProto* layerProto = mLayersProto.add_layers();
355355
const frontend::RequestedLayerState& layer = *root.getLayer();
@@ -362,10 +362,8 @@ void LayerProtoFromSnapshotGenerator::writeHierarchyToProto(
362362
LayerProtoHelper::writeSnapshotToProto(layerProto, layer, *snapshot, mTraceFlags);
363363

364364
for (const auto& [child, variant] : root.mChildren) {
365-
frontend::LayerHierarchy::ScopedAddToTraversalPath addChildToPath(path,
366-
child->getLayer()->id,
367-
variant);
368-
frontend::LayerSnapshot* childSnapshot = getSnapshot(path, layer);
365+
frontend::LayerSnapshot* childSnapshot =
366+
getSnapshot(path.makeChild(child->getLayer()->id, variant), layer);
369367
if (variant == Variant::Attached || variant == Variant::Detached ||
370368
frontend::LayerHierarchy::isMirror(variant)) {
371369
mChildToParent[childSnapshot->uniqueSequence] = snapshot->uniqueSequence;
@@ -388,10 +386,7 @@ void LayerProtoFromSnapshotGenerator::writeHierarchyToProto(
388386
if (variant == Variant::Detached) {
389387
continue;
390388
}
391-
frontend::LayerHierarchy::ScopedAddToTraversalPath addChildToPath(path,
392-
child->getLayer()->id,
393-
variant);
394-
writeHierarchyToProto(*child, path);
389+
writeHierarchyToProto(*child, path.makeChild(child->getLayer()->id, variant));
395390
}
396391
}
397392

‎services/surfaceflinger/LayerProtoHelper.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,8 @@ class LayerProtoFromSnapshotGenerator {
9898

9999
private:
100100
void writeHierarchyToProto(const frontend::LayerHierarchy& root,
101-
frontend::LayerHierarchy::TraversalPath& path);
102-
frontend::LayerSnapshot* getSnapshot(frontend::LayerHierarchy::TraversalPath& path,
101+
const frontend::LayerHierarchy::TraversalPath& path);
102+
frontend::LayerSnapshot* getSnapshot(const frontend::LayerHierarchy::TraversalPath& path,
103103
const frontend::RequestedLayerState& layer);
104104

105105
const frontend::LayerSnapshotBuilder& mSnapshotBuilder;

0 commit comments

Comments
 (0)