Have Metadata use android::CameraMetadata.

Ownership is much more clear than with the various double
raw metadata pointers.

BUG: 30140438
TEST: unit tests pass

Change-Id: Ib0de3bb4b9e6dc30594230e46f40ffaea06df51d
diff --git a/modules/camera/3_4/metadata/metadata.cpp b/modules/camera/3_4/metadata/metadata.cpp
index 8867e4d..0d03d5f 100644
--- a/modules/camera/3_4/metadata/metadata.cpp
+++ b/modules/camera/3_4/metadata/metadata.cpp
@@ -33,7 +33,7 @@
   components_.push_back(std::move(component));
 }
 
-int Metadata::FillStaticMetadata(camera_metadata_t** metadata) {
+int Metadata::FillStaticMetadata(android::CameraMetadata* metadata) {
   HAL_LOG_ENTER();
 
   std::vector<int32_t> static_tags;
@@ -42,13 +42,22 @@
   int res = 0;
 
   for (auto& component : components_) {
+    // Prevent components from potentially overriding others.
+    android::CameraMetadata additional_metadata;
     // Populate the fields.
-    res = component->PopulateStaticFields(metadata);
+    res = component->PopulateStaticFields(&additional_metadata);
     if (res) {
-      // Exit on error.
       HAL_LOGE("Failed to get all static properties.");
       return res;
     }
+    // Add it to the overall result.
+    if (!additional_metadata.isEmpty()) {
+      res = metadata->append(additional_metadata);
+      if (res != android::OK) {
+        HAL_LOGE("Failed to append all static properties.");
+        return res;
+      }
+    }
 
     // Note what tags the component adds.
     const std::vector<int32_t>* tags = &component->StaticTags();
@@ -60,38 +69,36 @@
   }
 
   // Populate the meta fields.
-  android::CameraMetadata metadata_wrapper(*metadata);
   static_tags.push_back(ANDROID_REQUEST_AVAILABLE_REQUEST_KEYS);
-  res = metadata_wrapper.update(ANDROID_REQUEST_AVAILABLE_REQUEST_KEYS,
-                                control_tags.data(), control_tags.size());
+  res = metadata->update(ANDROID_REQUEST_AVAILABLE_REQUEST_KEYS,
+                         control_tags.data(), control_tags.size());
   if (res != android::OK) {
     HAL_LOGE("Failed to add request keys meta key.");
     return -ENODEV;
   }
   static_tags.push_back(ANDROID_REQUEST_AVAILABLE_RESULT_KEYS);
-  res = metadata_wrapper.update(ANDROID_REQUEST_AVAILABLE_RESULT_KEYS,
-                                dynamic_tags.data(), dynamic_tags.size());
+  res = metadata->update(ANDROID_REQUEST_AVAILABLE_RESULT_KEYS,
+                         dynamic_tags.data(), dynamic_tags.size());
   if (res != android::OK) {
     HAL_LOGE("Failed to add result keys meta key.");
     return -ENODEV;
   }
   static_tags.push_back(ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS);
-  res = metadata_wrapper.update(ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,
-                                static_tags.data(), static_tags.size());
+  res = metadata->update(ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,
+                         static_tags.data(), static_tags.size());
   if (res != android::OK) {
     HAL_LOGE("Failed to add characteristics keys meta key.");
     return -ENODEV;
   }
 
-  *metadata = metadata_wrapper.release();
   return 0;
 }
 
-bool Metadata::IsValidRequest(const camera_metadata_t* metadata) {
+bool Metadata::IsValidRequest(const android::CameraMetadata& metadata) {
   HAL_LOG_ENTER();
 
-  // Null means "use previous settings", which are inherently valid.
-  if (metadata == nullptr) return true;
+  // Empty means "use previous settings", which are inherently valid.
+  if (metadata.isEmpty()) return true;
 
   for (auto& component : components_) {
     // Check that all components support the values requested of them.
@@ -105,11 +112,11 @@
   return true;
 }
 
-int Metadata::SetRequestSettings(const camera_metadata_t* metadata) {
+int Metadata::SetRequestSettings(const android::CameraMetadata& metadata) {
   HAL_LOG_ENTER();
 
-  // Null means "use previous settings".
-  if (metadata == nullptr) return 0;
+  // Empty means "use previous settings".
+  if (metadata.isEmpty()) return 0;
 
   for (auto& component : components_) {
     int res = component->SetRequestValues(metadata);
@@ -123,14 +130,24 @@
   return 0;
 }
 
-int Metadata::FillResultMetadata(camera_metadata_t** metadata) {
+int Metadata::FillResultMetadata(android::CameraMetadata* metadata) {
   for (auto& component : components_) {
-    int res = component->PopulateDynamicFields(metadata);
+    // Prevent components from potentially overriding others.
+    android::CameraMetadata additional_metadata;
+    int res = component->PopulateDynamicFields(&additional_metadata);
     if (res) {
       // Exit early if possible.
       HAL_LOGE("Failed to get all dynamic result fields.");
       return res;
     }
+    // Add it to the overall result.
+    if (!additional_metadata.isEmpty()) {
+      res = metadata->append(additional_metadata);
+      if (res != android::OK) {
+        HAL_LOGE("Failed to append all dynamic result fields.");
+        return res;
+      }
+    }
   }
 
   return 0;
diff --git a/modules/camera/3_4/metadata/metadata.h b/modules/camera/3_4/metadata/metadata.h
index d25a040..36e238b 100644
--- a/modules/camera/3_4/metadata/metadata.h
+++ b/modules/camera/3_4/metadata/metadata.h
@@ -17,6 +17,7 @@
 #ifndef V4L2_CAMERA_HAL_METADATA_H_
 #define V4L2_CAMERA_HAL_METADATA_H_
 
+#include <camera/CameraMetadata.h>
 #include <hardware/camera3.h>
 
 #include "../common.h"
@@ -28,10 +29,10 @@
   Metadata();
   virtual ~Metadata();
 
-  int FillStaticMetadata(camera_metadata_t** metadata);
-  bool IsValidRequest(const camera_metadata_t* metadata);
-  int SetRequestSettings(const camera_metadata_t* metadata);
-  int FillResultMetadata(camera_metadata_t** metadata);
+  int FillStaticMetadata(android::CameraMetadata* metadata);
+  bool IsValidRequest(const android::CameraMetadata& metadata);
+  int SetRequestSettings(const android::CameraMetadata& metadata);
+  int FillResultMetadata(android::CameraMetadata* metadata);
 
  protected:
   // Helper for the child constructors to fill in metadata components.
diff --git a/modules/camera/3_4/metadata/metadata_test.cpp b/modules/camera/3_4/metadata/metadata_test.cpp
index d7da553..62bb565 100644
--- a/modules/camera/3_4/metadata/metadata_test.cpp
+++ b/modules/camera/3_4/metadata/metadata_test.cpp
@@ -41,6 +41,10 @@
 
     component1_.reset(new PartialMetadataInterfaceMock());
     component2_.reset(new PartialMetadataInterfaceMock());
+    metadata_.reset(new android::CameraMetadata());
+    non_empty_metadata_.reset(new android::CameraMetadata());
+    uint8_t val = 1;
+    non_empty_metadata_->update(ANDROID_COLOR_CORRECTION_MODE, &val, 1);
   }
 
   // Once the component mocks have had expectations set,
@@ -65,14 +69,14 @@
   // Mocks.
   std::unique_ptr<PartialMetadataInterfaceMock> component1_;
   std::unique_ptr<PartialMetadataInterfaceMock> component2_;
+  // Metadata.
+  std::unique_ptr<android::CameraMetadata> metadata_;
+  std::unique_ptr<android::CameraMetadata> non_empty_metadata_;
   // An empty vector to use as necessary.
   std::vector<int32_t> empty_tags_;
 };
 
 TEST_F(MetadataTest, FillStaticSuccess) {
-  android::CameraMetadata metadata(1);
-  camera_metadata_t* metadata_raw = metadata.release();
-
   // Should populate all the component static pieces.
   EXPECT_CALL(*component1_, PopulateStaticFields(_)).WillOnce(Return(0));
   EXPECT_CALL(*component2_, PopulateStaticFields(_)).WillOnce(Return(0));
@@ -93,7 +97,7 @@
 
   AddComponents();
   // Should succeed. If it didn't, no reason to continue checking output.
-  ASSERT_EQ(dut_->FillStaticMetadata(&metadata_raw), 0);
+  ASSERT_EQ(dut_->FillStaticMetadata(metadata_.get()), 0);
 
   // Meta keys should be filled correctly.
   // Note: sets are used here, but it is undefined behavior if
@@ -114,17 +118,15 @@
   static_tags.emplace(ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS);
 
   // Check against what was filled in in the metadata.
-  metadata.acquire(metadata_raw);
   CompareTags(static_tags,
-              metadata.find(ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS));
+              metadata_->find(ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS));
   CompareTags(control_tags,
-              metadata.find(ANDROID_REQUEST_AVAILABLE_REQUEST_KEYS));
+              metadata_->find(ANDROID_REQUEST_AVAILABLE_REQUEST_KEYS));
   CompareTags(dynamic_tags,
-              metadata.find(ANDROID_REQUEST_AVAILABLE_RESULT_KEYS));
+              metadata_->find(ANDROID_REQUEST_AVAILABLE_RESULT_KEYS));
 }
 
 TEST_F(MetadataTest, FillStaticFail) {
-  camera_metadata_t* metadata = nullptr;
   int err = -99;
   // Order undefined, and may or may not exit early; use AtMost.
   EXPECT_CALL(*component1_, PopulateStaticFields(_))
@@ -154,12 +156,10 @@
 
   AddComponents();
   // If any component errors, error should be returned
-  EXPECT_EQ(dut_->FillStaticMetadata(&metadata), err);
+  EXPECT_EQ(dut_->FillStaticMetadata(metadata_.get()), err);
 }
 
 TEST_F(MetadataTest, IsValidSuccess) {
-  android::CameraMetadata metadata(1);
-
   // Should check if all the component request values are valid.
   EXPECT_CALL(*component1_, SupportsRequestValues(_)).WillOnce(Return(true));
   EXPECT_CALL(*component2_, SupportsRequestValues(_)).WillOnce(Return(true));
@@ -168,12 +168,10 @@
   // Should succeed.
   // Note: getAndLock is a lock against pointer invalidation, not concurrency,
   // and unlocks on object destruction.
-  EXPECT_TRUE(dut_->IsValidRequest(metadata.getAndLock()));
+  EXPECT_TRUE(dut_->IsValidRequest(*non_empty_metadata_));
 }
 
 TEST_F(MetadataTest, IsValidFail) {
-  android::CameraMetadata metadata(1);
-
   // Should check if all the component request values are valid.
   // Order undefined, and may or may not exit early; use AtMost.
   EXPECT_CALL(*component1_, SupportsRequestValues(_))
@@ -185,12 +183,10 @@
   // Should fail since one of the components failed.
   // Note: getAndLock is a lock against pointer invalidation, not concurrency,
   // and unlocks on object destruction.
-  EXPECT_FALSE(dut_->IsValidRequest(metadata.getAndLock()));
+  EXPECT_FALSE(dut_->IsValidRequest(*non_empty_metadata_));
 }
 
-TEST_F(MetadataTest, IsValidNull) {
-  camera_metadata_t* metadata = nullptr;
-
+TEST_F(MetadataTest, IsValidEmpty) {
   // Setting null settings is a special case indicating to use the
   // previous (valid) settings. As such it is inherently valid.
   // Should not try to check any components.
@@ -198,12 +194,10 @@
   EXPECT_CALL(*component2_, SupportsRequestValues(_)).Times(0);
 
   AddComponents();
-  EXPECT_TRUE(dut_->IsValidRequest(metadata));
+  EXPECT_TRUE(dut_->IsValidRequest(*metadata_));
 }
 
 TEST_F(MetadataTest, SetSettingsSuccess) {
-  android::CameraMetadata metadata(1);
-
   // Should check if all the components set successfully.
   EXPECT_CALL(*component1_, SetRequestValues(_)).WillOnce(Return(0));
   EXPECT_CALL(*component2_, SetRequestValues(_)).WillOnce(Return(0));
@@ -212,11 +206,10 @@
   // Should succeed.
   // Note: getAndLock is a lock against pointer invalidation, not concurrency,
   // and unlocks on object destruction.
-  EXPECT_EQ(dut_->SetRequestSettings(metadata.getAndLock()), 0);
+  EXPECT_EQ(dut_->SetRequestSettings(*non_empty_metadata_), 0);
 }
 
 TEST_F(MetadataTest, SetSettingsFail) {
-  android::CameraMetadata metadata(1);
   int err = -99;
 
   // Should check if all the components set successfully.
@@ -230,12 +223,10 @@
   // Should fail since one of the components failed.
   // Note: getAndLock is a lock against pointer invalidation, not concurrency,
   // and unlocks on object destruction.
-  EXPECT_EQ(dut_->SetRequestSettings(metadata.getAndLock()), err);
+  EXPECT_EQ(dut_->SetRequestSettings(*non_empty_metadata_), err);
 }
 
-TEST_F(MetadataTest, SetSettingsNull) {
-  camera_metadata_t* metadata = nullptr;
-
+TEST_F(MetadataTest, SetSettingsEmpty) {
   // Setting null settings is a special case indicating to use the
   // previous settings. Should not try to set any components.
   EXPECT_CALL(*component1_, SetRequestValues(_)).Times(0);
@@ -243,23 +234,20 @@
 
   AddComponents();
   // Should succeed.
-  EXPECT_EQ(dut_->SetRequestSettings(metadata), 0);
+  EXPECT_EQ(dut_->SetRequestSettings(*metadata_), 0);
 }
 
 TEST_F(MetadataTest, FillResultSuccess) {
-  camera_metadata_t* metadata = nullptr;
-
   // Should check if all the components fill results successfully.
   EXPECT_CALL(*component1_, PopulateDynamicFields(_)).WillOnce(Return(0));
   EXPECT_CALL(*component2_, PopulateDynamicFields(_)).WillOnce(Return(0));
 
   AddComponents();
   // Should succeed.
-  EXPECT_EQ(dut_->FillResultMetadata(&metadata), 0);
+  EXPECT_EQ(dut_->FillResultMetadata(metadata_.get()), 0);
 }
 
 TEST_F(MetadataTest, FillResultFail) {
-  camera_metadata_t* metadata = nullptr;
   int err = -99;
 
   // Should check if all the components fill results successfully.
@@ -271,7 +259,7 @@
 
   AddComponents();
   // Should fail since one of the components failed.
-  EXPECT_EQ(dut_->FillResultMetadata(&metadata), err);
+  EXPECT_EQ(dut_->FillResultMetadata(metadata_.get()), err);
 }
 
 }  // namespace v4l2_camera_hal
diff --git a/modules/camera/3_4/metadata/partial_metadata_interface.h b/modules/camera/3_4/metadata/partial_metadata_interface.h
index 5a637de..1284bc2 100644
--- a/modules/camera/3_4/metadata/partial_metadata_interface.h
+++ b/modules/camera/3_4/metadata/partial_metadata_interface.h
@@ -19,6 +19,7 @@
 
 #include <vector>
 
+#include <camera/CameraMetadata.h>
 #include <hardware/camera3.h>
 
 namespace v4l2_camera_hal {
@@ -36,17 +37,18 @@
 
   // Add all the static properties this partial metadata
   // is responsible for to |metadata|.
-  virtual int PopulateStaticFields(camera_metadata_t** metadata) const = 0;
+  virtual int PopulateStaticFields(android::CameraMetadata* metadata) const = 0;
   // Add all the dynamic states this partial metadata
   // is responsible for to |metadata|.
-  virtual int PopulateDynamicFields(camera_metadata_t** metadata) const = 0;
+  virtual int PopulateDynamicFields(
+      android::CameraMetadata* metadata) const = 0;
   // Check if the requested control values from |metadata| (for controls
   // this partial metadata owns) are supported.
   virtual bool SupportsRequestValues(
-      const camera_metadata_t* metadata) const = 0;
+      const android::CameraMetadata& metadata) const = 0;
   // Set all the controls this partial metadata
   // is responsible for from |metadata|.
-  virtual int SetRequestValues(const camera_metadata_t* metadata) = 0;
+  virtual int SetRequestValues(const android::CameraMetadata& metadata) = 0;
 };
 
 }  // namespace v4l2_camera_hal
diff --git a/modules/camera/3_4/metadata/partial_metadata_interface_mock.h b/modules/camera/3_4/metadata/partial_metadata_interface_mock.h
index a9bf3cd..63b24db 100644
--- a/modules/camera/3_4/metadata/partial_metadata_interface_mock.h
+++ b/modules/camera/3_4/metadata/partial_metadata_interface_mock.h
@@ -31,11 +31,13 @@
   MOCK_CONST_METHOD0(StaticTags, const std::vector<int32_t>&());
   MOCK_CONST_METHOD0(ControlTags, const std::vector<int32_t>&());
   MOCK_CONST_METHOD0(DynamicTags, const std::vector<int32_t>&());
-  MOCK_CONST_METHOD1(PopulateStaticFields, int(camera_metadata_t** metadata));
-  MOCK_CONST_METHOD1(PopulateDynamicFields, int(camera_metadata_t** metadata));
+  MOCK_CONST_METHOD1(PopulateStaticFields,
+                     int(android::CameraMetadata* metadata));
+  MOCK_CONST_METHOD1(PopulateDynamicFields,
+                     int(android::CameraMetadata* metadata));
   MOCK_CONST_METHOD1(SupportsRequestValues,
-                     bool(const camera_metadata_t* metadata));
-  MOCK_METHOD1(SetRequestValues, int(const camera_metadata_t* metadata));
+                     bool(const android::CameraMetadata& metadata));
+  MOCK_METHOD1(SetRequestValues, int(const android::CameraMetadata& metadata));
 };
 
 }  // namespace v4l2_camera_hal