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

6 files changed, 54 insertions, 18 deletions

diff --git a/libs/gui/LayerMetadata.cpp b/libs/gui/LayerMetadata.cpp
index 745433a605..04d2871c77 100644
--- a/libs/gui/LayerMetadata.cpp
+++ b/libs/gui/LayerMetadata.cpp
@@ -31,10 +31,23 @@ LayerMetadata::LayerMetadata(const LayerMetadata& other) = default;
 
 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 3ae10e461d..47f0cede3a 100644
--- a/libs/gui/include/gui/LayerMetadata.h
+++ b/libs/gui/include/gui/LayerMetadata.h
@@ -34,7 +34,9 @@ struct LayerMetadata : public Parcelable {
     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 2de169dcb1..646402ccad 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -1334,10 +1334,8 @@ bool Layer::setOverrideScalingMode(int32_t scalingMode) {
     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 f099df613b..367129aa70 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -290,7 +290,7 @@ public:
                                               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 9d03ae7f5f..b5b0108b2d 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -4116,7 +4116,7 @@ status_t SurfaceFlinger::createLayer(const String8& name, const sp<Client>& clie
         }
     }
 
-    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 92c9f92bf1..75a061bf77 100644
--- a/services/surfaceflinger/tests/unittests/LayerMetadataTest.cpp
+++ b/services/surfaceflinger/tests/unittests/LayerMetadataTest.cpp
@@ -62,24 +62,47 @@ TEST_F(LayerMetadataTest, testLayerMetadata) {
     metadata.mMap[2] = std::vector<uint8_t>{'a', 'b'};
     ASSERT_EQ(0, metadata.getInt32(2, 0));
 
+    Parcel p;
+    metadata.writeToParcel(&p);
+    LayerMetadata reconstructed;
+    reconstructed.setInt32(3, 1); // to make sure it gets replaced
+    p.setDataPosition(0);
+    reconstructed.readFromParcel(&p);
+    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);
-    metadata.merge(second);
+    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_EQ(2, metadata.getInt32(4, 0));
+    ASSERT_TRUE(metadata.mMap.at(4).empty());
 
-    Parcel p;
-    metadata.writeToParcel(&p);
-    LayerMetadata reconstructed;
-    reconstructed.setInt32(3, 1); // to make sure it gets replaced
-    p.setDataPosition(0);
-    reconstructed.readFromParcel(&p);
-    ASSERT_EQ(metadata.mMap, reconstructed.mMap);
+    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