Merge metadata from transaction instead of replace
This was replacing metadata when set in transactions rather
than merging. To fix this, merge has been augmented to also
report if a change occurred and to erase empty entries (as
a mechanism to "unset" metadata)
Bug: 122925737
Test: Added more unittests
Change-Id: Ia854cbcc1ddd334f18ffacea667cbb98778ec210
diff --git a/libs/gui/LayerMetadata.cpp b/libs/gui/LayerMetadata.cpp
index 745433a..04d2871 100644
--- a/libs/gui/LayerMetadata.cpp
+++ b/libs/gui/LayerMetadata.cpp
@@ -31,10 +31,23 @@
LayerMetadata::LayerMetadata(LayerMetadata&& other) = default;
-void LayerMetadata::merge(const LayerMetadata& other) {
+bool LayerMetadata::merge(const LayerMetadata& other, bool eraseEmpty) {
+ bool changed = false;
for (const auto& entry : other.mMap) {
- mMap[entry.first] = entry.second;
+ auto it = mMap.find(entry.first);
+ if (it != mMap.cend() && it->second != entry.second) {
+ if (eraseEmpty && entry.second.empty()) {
+ mMap.erase(it);
+ } else {
+ it->second = entry.second;
+ }
+ changed = true;
+ } else if (it == mMap.cend() && !entry.second.empty()) {
+ mMap[entry.first] = entry.second;
+ changed = true;
+ }
}
+ return changed;
}
status_t LayerMetadata::writeToParcel(Parcel* parcel) const {
diff --git a/libs/gui/include/gui/LayerMetadata.h b/libs/gui/include/gui/LayerMetadata.h
index 3ae10e4..47f0ced 100644
--- a/libs/gui/include/gui/LayerMetadata.h
+++ b/libs/gui/include/gui/LayerMetadata.h
@@ -34,7 +34,9 @@
LayerMetadata& operator=(const LayerMetadata& other);
LayerMetadata& operator=(LayerMetadata&& other);
- void merge(const LayerMetadata& other);
+ // Merges other into this LayerMetadata. If eraseEmpty is true, any entries in
+ // in this whose keys are paired with empty values in other will be erased.
+ bool merge(const LayerMetadata& other, bool eraseEmpty = false);
status_t writeToParcel(Parcel* parcel) const override;
status_t readFromParcel(const Parcel* parcel) override;
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index 2de169d..646402c 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -1334,10 +1334,8 @@
return true;
}
-bool Layer::setMetadata(LayerMetadata data) {
- bool changed = data.mMap != mCurrentState.metadata.mMap;
- if (!changed) return false;
- mCurrentState.metadata = std::move(data);
+bool Layer::setMetadata(const LayerMetadata& data) {
+ if (!mCurrentState.metadata.merge(data, true /* eraseEmpty */)) return false;
mCurrentState.sequence++;
mCurrentState.modified = true;
setTransactionFlags(eTransactionNeeded);
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index f099df6..367129a 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -290,7 +290,7 @@
uint64_t frameNumber);
virtual void deferTransactionUntil_legacy(const sp<Layer>& barrierLayer, uint64_t frameNumber);
virtual bool setOverrideScalingMode(int32_t overrideScalingMode);
- virtual bool setMetadata(LayerMetadata data);
+ virtual bool setMetadata(const LayerMetadata& data);
virtual bool reparentChildren(const sp<IBinder>& layer);
virtual void setChildrenDrawingParent(const sp<Layer>& layer);
virtual bool reparent(const sp<IBinder>& newParentHandle);
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 9d03ae7..b5b0108 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -4116,7 +4116,7 @@
}
}
- layer->setMetadata(std::move(metadata));
+ layer->setMetadata(metadata);
bool addToCurrentState = callingThreadHasUnscopedSurfaceFlingerAccess();
result = addClientLayer(client, *handle, *gbp, layer, *parent,
diff --git a/services/surfaceflinger/tests/unittests/LayerMetadataTest.cpp b/services/surfaceflinger/tests/unittests/LayerMetadataTest.cpp
index 92c9f92..75a061b 100644
--- a/services/surfaceflinger/tests/unittests/LayerMetadataTest.cpp
+++ b/services/surfaceflinger/tests/unittests/LayerMetadataTest.cpp
@@ -62,17 +62,6 @@
metadata.mMap[2] = std::vector<uint8_t>{'a', 'b'};
ASSERT_EQ(0, metadata.getInt32(2, 0));
- LayerMetadata second;
- std::vector<uint8_t> someData{'c', 'd', '\0'};
- second.mMap[2] = someData;
- second.setInt32(6, 5);
- metadata.merge(second);
-
- ASSERT_EQ(3, metadata.mMap.size());
- ASSERT_EQ(someData, second.mMap[2]);
- ASSERT_EQ(5, metadata.getInt32(6, 0));
- ASSERT_EQ(2, metadata.getInt32(4, 0));
-
Parcel p;
metadata.writeToParcel(&p);
LayerMetadata reconstructed;
@@ -82,5 +71,39 @@
ASSERT_EQ(metadata.mMap, reconstructed.mMap);
}
+TEST_F(LayerMetadataTest, merge) {
+ LayerMetadata metadata;
+ metadata.setInt32(4, 2);
+ metadata.mMap[2] = std::vector<uint8_t>{'a', 'b'};
+
+ LayerMetadata second;
+ std::vector<uint8_t> someData{'c', 'd', '\0'};
+ second.mMap[2] = someData;
+ second.setInt32(6, 5);
+ second.mMap[4].clear(); // will not delete if eraseEmpty is false
+ bool changed = metadata.merge(second);
+
+ ASSERT_TRUE(changed);
+ ASSERT_EQ(3, metadata.mMap.size());
+ ASSERT_EQ(someData, second.mMap[2]);
+ ASSERT_EQ(5, metadata.getInt32(6, 0));
+ ASSERT_TRUE(metadata.mMap.at(4).empty());
+
+ LayerMetadata withErase;
+ withErase.mMap[6].clear();
+ changed = metadata.merge(withErase, true /* eraseEmpty */);
+ ASSERT_TRUE(changed);
+ ASSERT_EQ(2, metadata.mMap.size());
+ ASSERT_EQ(someData, second.mMap[2]);
+ ASSERT_EQ(true, metadata.has(4));
+
+ // test for change detection
+ LayerMetadata third;
+ third.mMap[2] = someData;
+ third.mMap[5].clear();
+ changed = metadata.merge(third);
+ ASSERT_FALSE(changed);
+}
+
} // namespace
} // namespace android