Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 7190e8af authored by Corey Tabaka's avatar Corey Tabaka
Browse files

Fix VR surface attributes.

VR surface attributes had two issues that prevented the full extent
of their use:
1. The older clang version missed that templated copy constructor
   and assignment operators do not override the default ones in
   the Variant class. This caused issues with certain types when
   copy constructing / assigning from another Variant of the same
   type. This was noticed by running tests with a newer version of
   clang which provided warnings.
2. C++ rules about implicit conversion to bool from types that
   decay to pointers causes subtle issues with Variants that have
   bool elements.

   For example this assignment compiles but produces the wrong
   result:

   const int array[3] = { 1, 2, 3};
   Variant<int, bool, std::array<int, 3>> variant = array;
   EXPECT_FALSE(variant.is<bool>()); // Actually true.

   Here the programmer might accidentally think that the std::array
   element of the variant can be assigned from the regular array.
   This doesn't work, but instead the compiler decays the array to
   a pointer and assigns the bool element to true.

The first issue is addressed by defining copy/move constructors /
assignment operators on Variant and deleting the default ones from
the internal Union type for extra safety.

The second issue is addressed by making a more restrictive version
of the std::is_constructible trait that rejects bool construction
from types that decay to pointers. Once this was put in place the
erroneous use cases no longer compiled and is fixed as part of
this CL.

Tests are updated to verify the fixes to these issues.

Bug: 62557221
Test: pdx_tests and dvr_api-test passes.
Change-Id: Id4647e54e0a7b1753217fe7fe351462fe5bcfd83
parent 49a706d5
Loading
Loading
Loading
Loading
+19 −6
Original line number Diff line number Diff line
@@ -19,6 +19,16 @@ 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,
@@ -33,25 +43,28 @@ bool ConvertSurfaceAttributes(const DvrSurfaceAttribute* attributes,
        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{};
+176 −13
Original line number Diff line number Diff line
#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,26 +27,87 @@ namespace dvr {

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_BOOL;
  attribute.value.bool_value = value;
  attribute.value.type = DVR_SURFACE_ATTRIBUTE_TYPE_NONE;
  return attribute;
}

DvrSurfaceAttribute GetAttribute(DvrSurfaceAttributeKey key, int32_t value) {
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;
  attribute.value.bool_value = value;
  return attribute;
}

DvrSurfaceAttribute GetAttribute(DvrSurfaceAttributeKey key, nullptr_t) {
DvrSurfaceAttribute MakeAttribute(DvrSurfaceAttributeKey key, float value) {
  DvrSurfaceAttribute attribute;
  attribute.key = key;
  attribute.value.type = DVR_SURFACE_ATTRIBUTE_TYPE_NONE;
  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;
}

@@ -52,8 +115,8 @@ Status<UniqueDvrSurface> CreateApplicationSurface(bool visible = false,
                                                  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);
@@ -499,7 +562,7 @@ TEST_F(DvrDisplayManagerTest, SurfaceAttributeEvent) {
  EXPECT_EQ(expected_keys, actual_keys);

  std::vector<DvrSurfaceAttribute> attributes_to_set = {
      GetAttribute(DVR_SURFACE_ATTRIBUTE_Z_ORDER, 0)};
      MakeAttribute(DVR_SURFACE_ATTRIBUTE_Z_ORDER, 0)};

  // Test invalid args.
  EXPECT_EQ(-EINVAL, dvrSurfaceSetAttributes(nullptr, attributes_to_set.data(),
@@ -532,7 +595,7 @@ TEST_F(DvrDisplayManagerTest, SurfaceAttributeEvent) {

  // Test setting and then deleting an attribute.
  const DvrSurfaceAttributeKey kUserKey = 1;
  attributes_to_set = {GetAttribute(kUserKey, 1024)};
  attributes_to_set = {MakeAttribute(kUserKey, 1024)};

  ASSERT_EQ(0, dvrSurfaceSetAttributes(surface.get(), attributes_to_set.data(),
                                       attributes_to_set.size()));
@@ -552,7 +615,7 @@ TEST_F(DvrDisplayManagerTest, SurfaceAttributeEvent) {
  EXPECT_EQ(expected_keys, actual_keys);

  // Delete the attribute.
  attributes_to_set = {GetAttribute(kUserKey, nullptr)};
  attributes_to_set = {MakeAttribute(kUserKey, nullptr)};

  ASSERT_EQ(0, dvrSurfaceSetAttributes(surface.get(), attributes_to_set.data(),
                                       attributes_to_set.size()));
@@ -572,7 +635,7 @@ TEST_F(DvrDisplayManagerTest, SurfaceAttributeEvent) {
  EXPECT_NE(expected_keys, actual_keys);

  // Test deleting a reserved attribute.
  attributes_to_set = {GetAttribute(DVR_SURFACE_ATTRIBUTE_VISIBLE, nullptr)};
  attributes_to_set = {MakeAttribute(DVR_SURFACE_ATTRIBUTE_VISIBLE, nullptr)};

  EXPECT_EQ(0, dvrSurfaceSetAttributes(surface.get(), attributes_to_set.data(),
                                       attributes_to_set.size()));
@@ -594,6 +657,105 @@ TEST_F(DvrDisplayManagerTest, SurfaceAttributeEvent) {
  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) {
  // Create an application surface.
  auto surface_status = CreateApplicationSurface();
@@ -607,7 +769,8 @@ TEST_F(DvrDisplayManagerTest, SurfaceQueueEvent) {
  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.
+71 −5
Original line number Diff line number Diff line
@@ -26,14 +26,35 @@ using TypeForIndex = std::tuple_element_t<I, std::tuple<Types...>>;
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 @@ template <typename... Types>
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 @@ union Union<Type> {
      : 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 @@ union Union<First, Rest...> {
  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 @@ union Union<First, Rest...> {

}  // 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 @@ class Variant {
  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 @@ class Variant {
    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)
+24 −0
Original line number Diff line number Diff line
#include <array>
#include <cstdint>
#include <functional>
#include <memory>
@@ -1097,6 +1098,29 @@ TEST(Variant, HasType) {
  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));