Merge changes Id4647e54,I66b6edbd into oc-dr1-dev
am: bf72631a10

Change-Id: Ib1f5d7cc4841124c4f5fc1ea543ce3984dc26d16
diff --git a/libs/vr/libdvr/dvr_surface.cpp b/libs/vr/libdvr/dvr_surface.cpp
index c44f8a6..a3a47f1 100644
--- a/libs/vr/libdvr/dvr_surface.cpp
+++ b/libs/vr/libdvr/dvr_surface.cpp
@@ -2,6 +2,7 @@
 
 #include <inttypes.h>
 
+#include <pdx/rpc/variant.h>
 #include <private/android/AHardwareBufferHelpers.h>
 #include <private/dvr/display_client.h>
 
@@ -14,9 +15,20 @@
 using android::dvr::display::SurfaceAttributes;
 using android::dvr::display::SurfaceAttributeValue;
 using android::dvr::CreateDvrReadBufferFromBufferConsumer;
+using android::pdx::rpc::EmptyVariant;
 
 namespace {
 
+// Sets the Variant |destination| to the target std::array type and copies the C
+// array into it. Unsupported std::array configurations will fail to compile.
+template <typename T, std::size_t N>
+void ArrayCopy(SurfaceAttributeValue* destination, const T (&source)[N]) {
+  using ArrayType = std::array<T, N>;
+  *destination = ArrayType{};
+  std::copy(std::begin(source), std::end(source),
+            std::get<ArrayType>(*destination).begin());
+}
+
 bool ConvertSurfaceAttributes(const DvrSurfaceAttribute* attributes,
                               size_t attribute_count,
                               SurfaceAttributes* surface_attributes,
@@ -31,25 +43,31 @@
         value = attributes[i].value.int64_value;
         break;
       case DVR_SURFACE_ATTRIBUTE_TYPE_BOOL:
-        value = attributes[i].value.bool_value;
+        // bool_value is defined in an extern "C" block, which makes it look
+        // like an int to C++. Use a cast to assign the correct type to the
+        // Variant type SurfaceAttributeValue.
+        value = static_cast<bool>(attributes[i].value.bool_value);
         break;
       case DVR_SURFACE_ATTRIBUTE_TYPE_FLOAT:
         value = attributes[i].value.float_value;
         break;
       case DVR_SURFACE_ATTRIBUTE_TYPE_FLOAT2:
-        value = attributes[i].value.float2_value;
+        ArrayCopy(&value, attributes[i].value.float2_value);
         break;
       case DVR_SURFACE_ATTRIBUTE_TYPE_FLOAT3:
-        value = attributes[i].value.float3_value;
+        ArrayCopy(&value, attributes[i].value.float3_value);
         break;
       case DVR_SURFACE_ATTRIBUTE_TYPE_FLOAT4:
-        value = attributes[i].value.float4_value;
+        ArrayCopy(&value, attributes[i].value.float4_value);
         break;
       case DVR_SURFACE_ATTRIBUTE_TYPE_FLOAT8:
-        value = attributes[i].value.float8_value;
+        ArrayCopy(&value, attributes[i].value.float8_value);
         break;
       case DVR_SURFACE_ATTRIBUTE_TYPE_FLOAT16:
-        value = attributes[i].value.float16_value;
+        ArrayCopy(&value, attributes[i].value.float16_value);
+        break;
+      case DVR_SURFACE_ATTRIBUTE_TYPE_NONE:
+        value = EmptyVariant{};
         break;
       default:
         *error_index = i;
diff --git a/libs/vr/libdvr/tests/dvr_display_manager-test.cpp b/libs/vr/libdvr/tests/dvr_display_manager-test.cpp
index 726949d..f83b26e 100644
--- a/libs/vr/libdvr/tests/dvr_display_manager-test.cpp
+++ b/libs/vr/libdvr/tests/dvr_display_manager-test.cpp
@@ -1,11 +1,13 @@
 #include <android-base/properties.h>
 #include <base/logging.h>
 #include <gtest/gtest.h>
+#include <log/log.h>
 #include <poll.h>
 
 #include <android/hardware_buffer.h>
 
 #include <algorithm>
+#include <array>
 #include <set>
 #include <thread>
 #include <vector>
@@ -25,7 +27,30 @@
 
 namespace {
 
-DvrSurfaceAttribute GetAttribute(DvrSurfaceAttributeKey key, bool value) {
+DvrSurfaceAttribute MakeAttribute(DvrSurfaceAttributeKey key, nullptr_t) {
+  DvrSurfaceAttribute attribute;
+  attribute.key = key;
+  attribute.value.type = DVR_SURFACE_ATTRIBUTE_TYPE_NONE;
+  return attribute;
+}
+
+DvrSurfaceAttribute MakeAttribute(DvrSurfaceAttributeKey key, int32_t value) {
+  DvrSurfaceAttribute attribute;
+  attribute.key = key;
+  attribute.value.type = DVR_SURFACE_ATTRIBUTE_TYPE_INT32;
+  attribute.value.int32_value = value;
+  return attribute;
+}
+
+DvrSurfaceAttribute MakeAttribute(DvrSurfaceAttributeKey key, int64_t value) {
+  DvrSurfaceAttribute attribute;
+  attribute.key = key;
+  attribute.value.type = DVR_SURFACE_ATTRIBUTE_TYPE_INT64;
+  attribute.value.int64_value = value;
+  return attribute;
+}
+
+DvrSurfaceAttribute MakeAttribute(DvrSurfaceAttributeKey key, bool value) {
   DvrSurfaceAttribute attribute;
   attribute.key = key;
   attribute.value.type = DVR_SURFACE_ATTRIBUTE_TYPE_BOOL;
@@ -33,11 +58,56 @@
   return attribute;
 }
 
-DvrSurfaceAttribute GetAttribute(DvrSurfaceAttributeKey key, int32_t value) {
+DvrSurfaceAttribute MakeAttribute(DvrSurfaceAttributeKey key, float value) {
   DvrSurfaceAttribute attribute;
   attribute.key = key;
-  attribute.value.type = DVR_SURFACE_ATTRIBUTE_TYPE_INT32;
-  attribute.value.bool_value = value;
+  attribute.value.type = DVR_SURFACE_ATTRIBUTE_TYPE_FLOAT;
+  attribute.value.float_value = value;
+  return attribute;
+}
+
+DvrSurfaceAttribute MakeAttribute(DvrSurfaceAttributeKey key,
+                                  const std::array<float, 2>& value) {
+  DvrSurfaceAttribute attribute;
+  attribute.key = key;
+  attribute.value.type = DVR_SURFACE_ATTRIBUTE_TYPE_FLOAT2;
+  std::copy(value.begin(), value.end(), attribute.value.float2_value);
+  return attribute;
+}
+
+DvrSurfaceAttribute MakeAttribute(DvrSurfaceAttributeKey key,
+                                  const std::array<float, 3>& value) {
+  DvrSurfaceAttribute attribute;
+  attribute.key = key;
+  attribute.value.type = DVR_SURFACE_ATTRIBUTE_TYPE_FLOAT3;
+  std::copy(value.begin(), value.end(), attribute.value.float3_value);
+  return attribute;
+}
+
+DvrSurfaceAttribute MakeAttribute(DvrSurfaceAttributeKey key,
+                                  const std::array<float, 4>& value) {
+  DvrSurfaceAttribute attribute;
+  attribute.key = key;
+  attribute.value.type = DVR_SURFACE_ATTRIBUTE_TYPE_FLOAT4;
+  std::copy(value.begin(), value.end(), attribute.value.float4_value);
+  return attribute;
+}
+
+DvrSurfaceAttribute MakeAttribute(DvrSurfaceAttributeKey key,
+                                  const std::array<float, 8>& value) {
+  DvrSurfaceAttribute attribute;
+  attribute.key = key;
+  attribute.value.type = DVR_SURFACE_ATTRIBUTE_TYPE_FLOAT8;
+  std::copy(value.begin(), value.end(), attribute.value.float8_value);
+  return attribute;
+}
+
+DvrSurfaceAttribute MakeAttribute(DvrSurfaceAttributeKey key,
+                                  const std::array<float, 16>& value) {
+  DvrSurfaceAttribute attribute;
+  attribute.key = key;
+  attribute.value.type = DVR_SURFACE_ATTRIBUTE_TYPE_FLOAT16;
+  std::copy(value.begin(), value.end(), attribute.value.float16_value);
   return attribute;
 }
 
@@ -45,8 +115,8 @@
                                                   int32_t z_order = 0) {
   DvrSurface* surface = nullptr;
   DvrSurfaceAttribute attributes[] = {
-      GetAttribute(DVR_SURFACE_ATTRIBUTE_Z_ORDER, z_order),
-      GetAttribute(DVR_SURFACE_ATTRIBUTE_VISIBLE, visible)};
+      MakeAttribute(DVR_SURFACE_ATTRIBUTE_Z_ORDER, z_order),
+      MakeAttribute(DVR_SURFACE_ATTRIBUTE_VISIBLE, visible)};
 
   const int ret = dvrSurfaceCreate(
       attributes, std::extent<decltype(attributes)>::value, &surface);
@@ -101,13 +171,14 @@
       return {};
   }
 
-  Status<void> WaitForUpdate() {
+  enum : int { kTimeoutMs = 10000 };  // 10s
+
+  Status<void> WaitForUpdate(int timeout_ms = kTimeoutMs) {
     if (display_manager_event_fd_ < 0)
       return ErrorStatus(-display_manager_event_fd_);
 
-    const int kTimeoutMs = 10000;  // 10s
     pollfd pfd = {display_manager_event_fd_, POLLIN, 0};
-    const int count = poll(&pfd, 1, kTimeoutMs);
+    const int count = poll(&pfd, 1, timeout_ms);
     if (count < 0)
       return ErrorStatus(errno);
     else if (count == 0)
@@ -471,20 +542,218 @@
   auto attributes = attribute_status.take();
   EXPECT_GE(attributes.size(), 2u);
 
-  const std::set<int32_t> expected_keys = {DVR_SURFACE_ATTRIBUTE_Z_ORDER,
-                                           DVR_SURFACE_ATTRIBUTE_VISIBLE};
+  std::set<int32_t> actual_keys;
+  std::set<int32_t> expected_keys = {DVR_SURFACE_ATTRIBUTE_Z_ORDER,
+                                     DVR_SURFACE_ATTRIBUTE_VISIBLE};
 
   // Collect all the keys in attributes that match the expected keys.
-  std::set<int32_t> actual_keys;
-  std::for_each(attributes.begin(), attributes.end(),
-                [&expected_keys, &actual_keys](const auto& attribute) {
-                  if (expected_keys.find(attribute.key) != expected_keys.end())
-                    actual_keys.emplace(attribute.key);
-                });
+  auto compare_keys = [](const auto& attributes, const auto& expected_keys) {
+    std::set<int32_t> keys;
+    for (const auto& attribute : attributes) {
+      if (expected_keys.find(attribute.key) != expected_keys.end())
+        keys.emplace(attribute.key);
+    }
+    return keys;
+  };
 
   // If the sets match then attributes contained at least the expected keys,
   // even if other keys were also present.
+  actual_keys = compare_keys(attributes, expected_keys);
   EXPECT_EQ(expected_keys, actual_keys);
+
+  std::vector<DvrSurfaceAttribute> attributes_to_set = {
+      MakeAttribute(DVR_SURFACE_ATTRIBUTE_Z_ORDER, 0)};
+
+  // Test invalid args.
+  EXPECT_EQ(-EINVAL, dvrSurfaceSetAttributes(nullptr, attributes_to_set.data(),
+                                             attributes_to_set.size()));
+  EXPECT_EQ(-EINVAL, dvrSurfaceSetAttributes(surface.get(), nullptr,
+                                             attributes_to_set.size()));
+
+  // Test attribute change events.
+  ASSERT_EQ(0, dvrSurfaceSetAttributes(surface.get(), attributes_to_set.data(),
+                                       attributes_to_set.size()));
+  ASSERT_STATUS_OK(manager_->WaitForUpdate());
+
+  // Verify the attributes changed flag is set.
+  auto check_flags = [](const auto& value) {
+    return value & DVR_SURFACE_UPDATE_FLAGS_ATTRIBUTES_CHANGED;
+  };
+  EXPECT_STATUS_PRED(check_flags, manager_->GetUpdateFlags(0));
+
+  attribute_status = manager_->GetAttributes(0);
+  ASSERT_STATUS_OK(attribute_status);
+  attributes = attribute_status.take();
+  EXPECT_GE(attributes.size(), 2u);
+
+  expected_keys = {DVR_SURFACE_ATTRIBUTE_Z_ORDER,
+                   DVR_SURFACE_ATTRIBUTE_VISIBLE};
+
+  actual_keys.clear();
+  actual_keys = compare_keys(attributes, expected_keys);
+  EXPECT_EQ(expected_keys, actual_keys);
+
+  // Test setting and then deleting an attribute.
+  const DvrSurfaceAttributeKey kUserKey = 1;
+  attributes_to_set = {MakeAttribute(kUserKey, 1024)};
+
+  ASSERT_EQ(0, dvrSurfaceSetAttributes(surface.get(), attributes_to_set.data(),
+                                       attributes_to_set.size()));
+  ASSERT_STATUS_OK(manager_->WaitForUpdate());
+  EXPECT_STATUS_PRED(check_flags, manager_->GetUpdateFlags(0));
+
+  attribute_status = manager_->GetAttributes(0);
+  ASSERT_STATUS_OK(attribute_status);
+  attributes = attribute_status.take();
+  EXPECT_GE(attributes.size(), 2u);
+
+  expected_keys = {DVR_SURFACE_ATTRIBUTE_Z_ORDER, DVR_SURFACE_ATTRIBUTE_VISIBLE,
+                   kUserKey};
+
+  actual_keys.clear();
+  actual_keys = compare_keys(attributes, expected_keys);
+  EXPECT_EQ(expected_keys, actual_keys);
+
+  // Delete the attribute.
+  attributes_to_set = {MakeAttribute(kUserKey, nullptr)};
+
+  ASSERT_EQ(0, dvrSurfaceSetAttributes(surface.get(), attributes_to_set.data(),
+                                       attributes_to_set.size()));
+  ASSERT_STATUS_OK(manager_->WaitForUpdate());
+  EXPECT_STATUS_PRED(check_flags, manager_->GetUpdateFlags(0));
+
+  attribute_status = manager_->GetAttributes(0);
+  ASSERT_STATUS_OK(attribute_status);
+  attributes = attribute_status.take();
+  EXPECT_GE(attributes.size(), 2u);
+
+  expected_keys = {DVR_SURFACE_ATTRIBUTE_Z_ORDER, DVR_SURFACE_ATTRIBUTE_VISIBLE,
+                   kUserKey};
+
+  actual_keys.clear();
+  actual_keys = compare_keys(attributes, expected_keys);
+  EXPECT_NE(expected_keys, actual_keys);
+
+  // Test deleting a reserved attribute.
+  attributes_to_set = {MakeAttribute(DVR_SURFACE_ATTRIBUTE_VISIBLE, nullptr)};
+
+  EXPECT_EQ(0, dvrSurfaceSetAttributes(surface.get(), attributes_to_set.data(),
+                                       attributes_to_set.size()));
+
+  // Failed attribute operations should not trigger update events.
+  const int kTimeoutMs = 100;  // 0.1s
+  EXPECT_STATUS_ERROR_VALUE(ETIMEDOUT, manager_->WaitForUpdate(kTimeoutMs));
+
+  attribute_status = manager_->GetAttributes(0);
+  ASSERT_STATUS_OK(attribute_status);
+  attributes = attribute_status.take();
+  EXPECT_GE(attributes.size(), 2u);
+
+  expected_keys = {DVR_SURFACE_ATTRIBUTE_Z_ORDER,
+                   DVR_SURFACE_ATTRIBUTE_VISIBLE};
+
+  actual_keys.clear();
+  actual_keys = compare_keys(attributes, expected_keys);
+  EXPECT_EQ(expected_keys, actual_keys);
+}
+
+TEST_F(DvrDisplayManagerTest, SurfaceAttributeTypes) {
+  // Create an application surface.
+  auto surface_status = CreateApplicationSurface();
+  ASSERT_STATUS_OK(surface_status);
+  UniqueDvrSurface surface = surface_status.take();
+  ASSERT_NE(nullptr, surface.get());
+
+  enum : std::int32_t {
+    kInt32Key = 1,
+    kInt64Key,
+    kBoolKey,
+    kFloatKey,
+    kFloat2Key,
+    kFloat3Key,
+    kFloat4Key,
+    kFloat8Key,
+    kFloat16Key,
+  };
+
+  const std::vector<DvrSurfaceAttribute> attributes_to_set = {
+      MakeAttribute(kInt32Key, int32_t{0}),
+      MakeAttribute(kInt64Key, int64_t{0}),
+      MakeAttribute(kBoolKey, false),
+      MakeAttribute(kFloatKey, 0.0f),
+      MakeAttribute(kFloat2Key, std::array<float, 2>{{1.0f, 2.0f}}),
+      MakeAttribute(kFloat3Key, std::array<float, 3>{{3.0f, 4.0f, 5.0f}}),
+      MakeAttribute(kFloat4Key, std::array<float, 4>{{6.0f, 7.0f, 8.0f, 9.0f}}),
+      MakeAttribute(kFloat8Key,
+                    std::array<float, 8>{{10.0f, 11.0f, 12.0f, 13.0f, 14.0f,
+                                          15.0f, 16.0f, 17.0f}}),
+      MakeAttribute(kFloat16Key, std::array<float, 16>{
+                                     {18.0f, 19.0f, 20.0f, 21.0f, 22.0f, 23.0f,
+                                      24.0f, 25.0f, 26.0f, 27.0f, 28.0f, 29.0f,
+                                      30.0f, 31.0f, 32.0f, 33.0f}})};
+
+  EXPECT_EQ(0, dvrSurfaceSetAttributes(surface.get(), attributes_to_set.data(),
+                                       attributes_to_set.size()));
+
+  ASSERT_STATUS_OK(manager_->WaitForUpdate());
+  auto attribute_status = manager_->GetAttributes(0);
+  ASSERT_STATUS_OK(attribute_status);
+  auto attributes = attribute_status.take();
+  EXPECT_GE(attributes.size(), attributes_to_set.size());
+
+  auto HasAttribute = [](const auto& attributes,
+                         DvrSurfaceAttributeKey key) -> bool {
+    for (const auto& attribute : attributes) {
+      if (attribute.key == key)
+        return true;
+    }
+    return false;
+  };
+  auto AttributeType =
+      [](const auto& attributes,
+         DvrSurfaceAttributeKey key) -> DvrSurfaceAttributeType {
+    for (const auto& attribute : attributes) {
+      if (attribute.key == key)
+        return attribute.value.type;
+    }
+    return DVR_SURFACE_ATTRIBUTE_TYPE_NONE;
+  };
+
+  ASSERT_TRUE(HasAttribute(attributes, kInt32Key));
+  EXPECT_EQ(DVR_SURFACE_ATTRIBUTE_TYPE_INT32,
+            AttributeType(attributes, kInt32Key));
+
+  ASSERT_TRUE(HasAttribute(attributes, kInt64Key));
+  EXPECT_EQ(DVR_SURFACE_ATTRIBUTE_TYPE_INT64,
+            AttributeType(attributes, kInt64Key));
+
+  ASSERT_TRUE(HasAttribute(attributes, kBoolKey));
+  EXPECT_EQ(DVR_SURFACE_ATTRIBUTE_TYPE_BOOL,
+            AttributeType(attributes, kBoolKey));
+
+  ASSERT_TRUE(HasAttribute(attributes, kFloatKey));
+  EXPECT_EQ(DVR_SURFACE_ATTRIBUTE_TYPE_FLOAT,
+            AttributeType(attributes, kFloatKey));
+
+  ASSERT_TRUE(HasAttribute(attributes, kFloat2Key));
+  EXPECT_EQ(DVR_SURFACE_ATTRIBUTE_TYPE_FLOAT2,
+            AttributeType(attributes, kFloat2Key));
+
+  ASSERT_TRUE(HasAttribute(attributes, kFloat3Key));
+  EXPECT_EQ(DVR_SURFACE_ATTRIBUTE_TYPE_FLOAT3,
+            AttributeType(attributes, kFloat3Key));
+
+  ASSERT_TRUE(HasAttribute(attributes, kFloat4Key));
+  EXPECT_EQ(DVR_SURFACE_ATTRIBUTE_TYPE_FLOAT4,
+            AttributeType(attributes, kFloat4Key));
+
+  ASSERT_TRUE(HasAttribute(attributes, kFloat8Key));
+  EXPECT_EQ(DVR_SURFACE_ATTRIBUTE_TYPE_FLOAT8,
+            AttributeType(attributes, kFloat8Key));
+
+  ASSERT_TRUE(HasAttribute(attributes, kFloat16Key));
+  EXPECT_EQ(DVR_SURFACE_ATTRIBUTE_TYPE_FLOAT16,
+            AttributeType(attributes, kFloat16Key));
 }
 
 TEST_F(DvrDisplayManagerTest, SurfaceQueueEvent) {
@@ -500,7 +769,8 @@
   ASSERT_STATUS_OK(manager_->WaitForUpdate());
   ASSERT_STATUS_EQ(1u, manager_->GetSurfaceCount());
 
-  // Verify there are no queues for the surface recorded in the state snapshot.
+  // Verify there are no queues for the surface recorded in the state
+  // snapshot.
   EXPECT_STATUS_EQ(std::vector<int>{}, manager_->GetQueueIds(0));
 
   // Create a new queue in the surface.
diff --git a/libs/vr/libpdx/private/pdx/rpc/variant.h b/libs/vr/libpdx/private/pdx/rpc/variant.h
index dc321fa..80b1769 100644
--- a/libs/vr/libpdx/private/pdx/rpc/variant.h
+++ b/libs/vr/libpdx/private/pdx/rpc/variant.h
@@ -26,14 +26,35 @@
 template <std::size_t I, typename... Types>
 using TypeTagForIndex = TypeTag<TypeForIndex<I, Types...>>;
 
+// Similar to std::is_constructible except that it evaluates to false for bool
+// construction from pointer types: this helps prevent subtle to bugs caused by
+// assigning values that decay to pointers to Variants with bool elements.
+//
+// Here is an example of the problematic situation this trait avoids:
+//
+//  Variant<int, bool> v;
+//  const int array[3] = {1, 2, 3};
+//  v = array; // This is allowed by regular std::is_constructible.
+//
+template <typename...>
+struct IsConstructible;
+template <typename T, typename U>
+struct IsConstructible<T, U>
+    : std::integral_constant<bool,
+                             std::is_constructible<T, U>::value &&
+                                 !(std::is_same<std::decay_t<T>, bool>::value &&
+                                   std::is_pointer<std::decay_t<U>>::value)> {};
+template <typename T, typename... Args>
+struct IsConstructible<T, Args...> : std::is_constructible<T, Args...> {};
+
 // Enable if T(Args...) is well formed.
 template <typename R, typename T, typename... Args>
 using EnableIfConstructible =
-    typename std::enable_if<std::is_constructible<T, Args...>::value, R>::type;
+    typename std::enable_if<IsConstructible<T, Args...>::value, R>::type;
 // Enable if T(Args...) is not well formed.
 template <typename R, typename T, typename... Args>
 using EnableIfNotConstructible =
-    typename std::enable_if<!std::is_constructible<T, Args...>::value, R>::type;
+    typename std::enable_if<!IsConstructible<T, Args...>::value, R>::type;
 
 // Determines whether T is an element of Types...;
 template <typename... Types>
@@ -65,12 +86,11 @@
 struct ConstructibleCount;
 template <typename From, typename To>
 struct ConstructibleCount<From, To>
-    : std::integral_constant<std::size_t,
-                             std::is_constructible<To, From>::value> {};
+    : std::integral_constant<std::size_t, IsConstructible<To, From>::value> {};
 template <typename From, typename First, typename... Rest>
 struct ConstructibleCount<From, First, Rest...>
     : std::integral_constant<std::size_t,
-                             std::is_constructible<First, From>::value +
+                             IsConstructible<First, From>::value +
                                  ConstructibleCount<From, Rest...>::value> {};
 
 // Enable if T is an element of Types...
@@ -126,6 +146,18 @@
       : first_(std::forward<T>(value)) {
     *index_out = index;
   }
+  Union(const Union& other, std::int32_t index) {
+    if (index == 0)
+      new (&first_) Type(other.first_);
+  }
+  Union(Union&& other, std::int32_t index) {
+    if (index == 0)
+      new (&first_) Type(std::move(other.first_));
+  }
+  Union(const Union&) = delete;
+  Union(Union&&) = delete;
+  void operator=(const Union&) = delete;
+  void operator=(Union&&) = delete;
 
   Type& get(TypeTag<Type>) { return first_; }
   const Type& get(TypeTag<Type>) const { return first_; }
@@ -217,6 +249,22 @@
   template <typename T, typename U>
   Union(std::int32_t index, std::int32_t* index_out, TypeTag<T>, U&& value)
       : rest_(index + 1, index_out, TypeTag<T>{}, std::forward<U>(value)) {}
+  Union(const Union& other, std::int32_t index) {
+    if (index == 0)
+      new (&first_) First(other.first_);
+    else
+      new (&rest_) Union<Rest...>(other.rest_, index - 1);
+  }
+  Union(Union&& other, std::int32_t index) {
+    if (index == 0)
+      new (&first_) First(std::move(other.first_));
+    else
+      new (&rest_) Union<Rest...>(std::move(other.rest_), index - 1);
+  }
+  Union(const Union&) = delete;
+  Union(Union&&) = delete;
+  void operator=(const Union&) = delete;
+  void operator=(Union&&) = delete;
 
   struct FirstType {};
   struct RestType {};
@@ -351,6 +399,10 @@
 
 }  // namespace detail
 
+// Variant is a type safe union that can store values of any of its element
+// types. A Variant is different than std::tuple in that it only stores one type
+// at a time or a special empty type. Variants are always default constructible
+// to empty, even when none of the element types are default constructible.
 template <typename... Types>
 class Variant {
  private:
@@ -393,6 +445,11 @@
   explicit Variant(EmptyVariant) {}
   ~Variant() { Destruct(); }
 
+  Variant(const Variant& other)
+      : index_{other.index_}, value_{other.value_, other.index_} {}
+  Variant(Variant&& other)
+      : index_{other.index_}, value_{std::move(other.value_), other.index_} {}
+
   // Copy and move construction from Variant types. Each element of OtherTypes
   // must be convertible to an element of Types.
   template <typename... OtherTypes>
@@ -404,6 +461,15 @@
     other.Visit([this](auto&& value) { Construct(std::move(value)); });
   }
 
+  Variant& operator=(const Variant& other) {
+    other.Visit([this](const auto& value) { *this = value; });
+    return *this;
+  }
+  Variant& operator=(Variant&& other) {
+    other.Visit([this](auto&& value) { *this = std::move(value); });
+    return *this;
+  }
+
   // Construction from non-Variant types.
   template <typename T, typename = EnableIfAssignable<void, T>>
   explicit Variant(T&& value)
diff --git a/libs/vr/libpdx/variant_tests.cpp b/libs/vr/libpdx/variant_tests.cpp
index b1ebc9b..e3520f5 100644
--- a/libs/vr/libpdx/variant_tests.cpp
+++ b/libs/vr/libpdx/variant_tests.cpp
@@ -1,3 +1,4 @@
+#include <array>
 #include <cstdint>
 #include <functional>
 #include <memory>
@@ -1097,6 +1098,29 @@
   EXPECT_FALSE((detail::HasType<char&, int, float, bool>::value));
 }
 
+TEST(Variant, IsConstructible) {
+  using ArrayType = const float[3];
+  struct ImplicitBool {
+    operator bool() const { return true; }
+  };
+  struct ExplicitBool {
+    explicit operator bool() const { return true; }
+  };
+  struct NonBool {};
+  struct TwoArgs {
+    TwoArgs(int, bool) {}
+  };
+
+  EXPECT_FALSE((detail::IsConstructible<bool, ArrayType>::value));
+  EXPECT_TRUE((detail::IsConstructible<bool, int>::value));
+  EXPECT_TRUE((detail::IsConstructible<bool, ImplicitBool>::value));
+  EXPECT_TRUE((detail::IsConstructible<bool, ExplicitBool>::value));
+  EXPECT_FALSE((detail::IsConstructible<bool, NonBool>::value));
+  EXPECT_TRUE((detail::IsConstructible<TwoArgs, int, bool>::value));
+  EXPECT_FALSE((detail::IsConstructible<TwoArgs, int, std::string>::value));
+  EXPECT_FALSE((detail::IsConstructible<TwoArgs, int>::value));
+}
+
 TEST(Variant, Set) {
   EXPECT_TRUE((detail::Set<int, bool, float>::template IsSubset<int, bool,
                                                                 float>::value));
diff --git a/libs/vr/libvrflinger/display_surface.cpp b/libs/vr/libvrflinger/display_surface.cpp
index 330b455..6e18781 100644
--- a/libs/vr/libvrflinger/display_surface.cpp
+++ b/libs/vr/libvrflinger/display_surface.cpp
@@ -68,7 +68,7 @@
   display::SurfaceUpdateFlags update_flags;
 
   for (const auto& attribute : attributes) {
-    const auto& key = attribute.first;
+    const auto key = attribute.first;
     const auto* variant = &attribute.second;
     bool invalid_value = false;
     bool visibility_changed = false;
@@ -95,14 +95,23 @@
         break;
     }
 
+    // Only update the attribute map with valid values. This check also has the
+    // effect of preventing special attributes handled above from being deleted
+    // by an empty value.
     if (invalid_value) {
       ALOGW(
           "DisplaySurface::OnClientSetAttributes: Failed to set display "
           "surface attribute '%d' because of incompatible type: %d",
           key, variant->index());
     } else {
-      // Only update the attribute map with valid values.
-      attributes_[attribute.first] = attribute.second;
+      // An empty value indicates the attribute should be deleted.
+      if (variant->empty()) {
+        auto search = attributes_.find(key);
+        if (search != attributes_.end())
+          attributes_.erase(search);
+      } else {
+        attributes_[key] = *variant;
+      }
 
       // All attribute changes generate a notification, even if the value
       // doesn't change. Visibility attributes set a flag only if the value