Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ci/licenses_golden/licenses_flutter
Original file line number Diff line number Diff line change
Expand Up @@ -1288,6 +1288,8 @@ ORIGIN: ../../../flutter/impeller/geometry/constants.h + ../../../flutter/LICENS
ORIGIN: ../../../flutter/impeller/geometry/geometry_benchmarks.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/geometry/gradient.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/geometry/gradient.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/geometry/half.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/geometry/half.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/geometry/matrix.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/geometry/matrix.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/geometry/matrix_decomposition.cc + ../../../flutter/LICENSE
Expand Down Expand Up @@ -3851,6 +3853,8 @@ FILE: ../../../flutter/impeller/geometry/constants.h
FILE: ../../../flutter/impeller/geometry/geometry_benchmarks.cc
FILE: ../../../flutter/impeller/geometry/gradient.cc
FILE: ../../../flutter/impeller/geometry/gradient.h
FILE: ../../../flutter/impeller/geometry/half.cc
FILE: ../../../flutter/impeller/geometry/half.h
FILE: ../../../flutter/impeller/geometry/matrix.cc
FILE: ../../../flutter/impeller/geometry/matrix.h
FILE: ../../../flutter/impeller/geometry/matrix_decomposition.cc
Expand Down
75 changes: 75 additions & 0 deletions impeller/compiler/reflector.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "impeller/compiler/code_gen_template.h"
#include "impeller/compiler/uniform_sorter.h"
#include "impeller/compiler/utilities.h"
#include "impeller/geometry/half.h"
#include "impeller/geometry/matrix.h"
#include "impeller/geometry/scalar.h"

Expand Down Expand Up @@ -534,6 +535,11 @@ static std::optional<KnownType> ReadKnownScalarType(
.name = "Scalar",
.byte_size = sizeof(Scalar),
};
case spirv_cross::SPIRType::BaseType::Half:
return KnownType{
.name = "Half",
.byte_size = sizeof(Half),
};
case spirv_cross::SPIRType::BaseType::UInt:
return KnownType{
.name = "uint32_t",
Expand Down Expand Up @@ -767,6 +773,75 @@ std::vector<StructMember> Reflector::ReadStructMembers(
continue;
}

// Tightly packed half Point (vec2).
if (member.basetype == spirv_cross::SPIRType::BaseType::Half && //
member.width == sizeof(float) * 4 && //
Comment thread
gaaclarke marked this conversation as resolved.
Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you double check this? The member width doesn't make sense for half base types. I may be misunderstanding.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, this was a hack I left in. Should be sizeof(Half) * 8

member.columns == 1 && //
member.vecsize == 2 //
) {
uint32_t stride =
GetArrayStride<sizeof(HalfVector2)>(struct_type, member, i);
uint32_t element_padding = stride - sizeof(HalfVector2);
result.emplace_back(StructMember{
"HalfVector2", // type

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argument comments are of the format /*arg_name=*/: https://google.github.io/styleguide/cppguide.html#Function_Argument_Comments

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is following the style of the rest of the file.

@jonahwilliams jonahwilliams Mar 27, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...also this is a struct, not a function call.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use /*field_name=*/ we can use the linter to verify that their name is correct. I turned this on years ago. I don't know if it still runs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a function call, but with C++20, you can use aggregate initialization and do this without comments: {.type = "HalfVector2"}.

BaseTypeToString(member.basetype), // basetype
GetMemberNameAtIndex(struct_type, i), // name
struct_member_offset, // offset
sizeof(HalfVector2), // size
stride * array_elements.value_or(1), // byte_length
array_elements, // array_elements
element_padding, // element_padding
});
current_byte_offset += stride * array_elements.value_or(1);
continue;
}

// Tightly packed Half Float Vector3.
if (member.basetype == spirv_cross::SPIRType::BaseType::Half && //
member.width == sizeof(float) * 4 && //
member.columns == 1 && //
member.vecsize == 3 //
) {
uint32_t stride =
GetArrayStride<sizeof(HalfVector3)>(struct_type, member, i);
uint32_t element_padding = stride - sizeof(HalfVector3);
result.emplace_back(StructMember{
"HalfVector3", // type
BaseTypeToString(member.basetype), // basetype
GetMemberNameAtIndex(struct_type, i), // name
struct_member_offset, // offset
sizeof(HalfVector3), // size
stride * array_elements.value_or(1), // byte_length
array_elements, // array_elements
element_padding, // element_padding
});
current_byte_offset += stride * array_elements.value_or(1);
continue;
}

// Tightly packed Half Float Vector4.
if (member.basetype == spirv_cross::SPIRType::BaseType::Half && //
member.width == sizeof(float) * 4 && //
member.columns == 1 && //
member.vecsize == 4 //
) {
uint32_t stride =
GetArrayStride<sizeof(HalfVector4)>(struct_type, member, i);
uint32_t element_padding = stride - sizeof(HalfVector4);
result.emplace_back(StructMember{
"HalfVector4", // type
BaseTypeToString(member.basetype), // basetype
GetMemberNameAtIndex(struct_type, i), // name
struct_member_offset, // offset
sizeof(HalfVector4), // size
stride * array_elements.value_or(1), // byte_length
array_elements, // array_elements
element_padding, // element_padding
});
current_byte_offset += stride * array_elements.value_or(1);
continue;
}

// Other isolated scalars (like bool, int, float/Scalar, etc..).
{
auto maybe_known_type = ReadKnownScalarType(member.basetype);
Expand Down
3 changes: 3 additions & 0 deletions impeller/compiler/shader_lib/impeller/types.glsl
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
#define TYPES_GLSL_

#extension GL_AMD_gpu_shader_half_float : enable
#extension GL_AMD_gpu_shader_half_float_fetch : enable
#extension GL_EXT_shader_explicit_arithmetic_types_float16 : enable

#ifndef IMPELLER_TARGET_METAL

Expand All @@ -17,6 +19,7 @@ precision mediump float;
#define f16vec3 vec3
#define f16vec4 vec4
#define f16mat4 mat4
#define f16sampler2D sampler2D

#endif // IMPELLER_TARGET_METAL

Expand Down
4 changes: 2 additions & 2 deletions impeller/entity/shaders/solid_fill.frag
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@
#include <impeller/types.glsl>

uniform FragInfo {
vec4 color;
f16vec4 color;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this require #extension GL_EXT_shader_16bit_storage : require up top?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not, I'm not actually using a f16vec4 on glsl, here is the compiled shader:

#version 100
precision mediump float;
precision highp int;

struct FragInfo
{
    vec4 color;
};

uniform FragInfo frag_info;

void main()
{
    gl_FragData[0] = frag_info.color;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh - but this is all in types.glsl too

}
frag_info;

out vec4 frag_color;
out f16vec4 frag_color;

void main() {
frag_color = frag_info.color;
Expand Down
2 changes: 2 additions & 0 deletions impeller/geometry/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ impeller_component("geometry") {
"constants.h",
"gradient.cc",
"gradient.h",
"half.cc",
"half.h",
"matrix.cc",
"matrix.h",
"matrix_decomposition.cc",
Expand Down
33 changes: 33 additions & 0 deletions impeller/geometry/geometry_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@
#include <limits>
#include <sstream>

#include "flutter/fml/build_config.h"
#include "flutter/testing/testing.h"
#include "impeller/geometry/constants.h"
#include "impeller/geometry/gradient.h"
#include "impeller/geometry/half.h"
#include "impeller/geometry/path.h"
#include "impeller/geometry/path_builder.h"
#include "impeller/geometry/path_component.h"
Expand Down Expand Up @@ -2090,5 +2092,36 @@ TEST(GeometryTest, Gradient) {
}
}

TEST(GeometryTest, HalfConversions) {
#ifndef FML_OS_WIN

@dnfield dnfield Mar 27, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#ifndef FML_OS_WIN
#ifdef FML_OS_WIN
GTEST_SKIP() << "The reason this doesn't work on Windows";
#endif

And then remove the #endif below. This helps document why we're skipping it and makes it clear at execution time that the test is not actually doing anything on Windows.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can try this but I don't believe the half precision floating point literals below even compile on windows.

ASSERT_EQ(ScalarToHalf(0.0), 0.0f16);
ASSERT_EQ(ScalarToHalf(0.05), 0.05f16);
ASSERT_EQ(ScalarToHalf(2.43), 2.43f16);
ASSERT_EQ(ScalarToHalf(-1.45), -1.45f16);

// 65504 is the largest possible half.
ASSERT_EQ(ScalarToHalf(65504.0f), 65504.0f16);
ASSERT_EQ(ScalarToHalf(65504.0f + 1), 65504.0f16);

// Colors
ASSERT_EQ(HalfVector4(Color::Red()),
HalfVector4(1.0f16, 0.0f16, 0.0f16, 1.0f16));
ASSERT_EQ(HalfVector4(Color::Green()),
HalfVector4(0.0f16, 1.0f16, 0.0f16, 1.0f16));
ASSERT_EQ(HalfVector4(Color::Blue()),
HalfVector4(0.0f16, 0.0f16, 1.0f16, 1.0f16));
ASSERT_EQ(HalfVector4(Color::Black().WithAlpha(0)),
HalfVector4(0.0f16, 0.0f16, 0.0f16, 0.0f16));

ASSERT_EQ(HalfVector3(Vector3(4.0, 6.0, -1.0)),
HalfVector3(4.0f16, 6.0f16, -1.0f16));
ASSERT_EQ(HalfVector2(Vector2(4.0, 6.0)), HalfVector2(4.0f16, 6.0f16));

ASSERT_EQ(Half(0.5f), Half(0.5f16));
ASSERT_EQ(Half(0.5), Half(0.5f16));
ASSERT_EQ(Half(5), Half(5.0f16));
#endif // FML_OS_WIN
}

} // namespace testing
} // namespace impeller
18 changes: 18 additions & 0 deletions impeller/geometry/half.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "half.h"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(this should be based from the project root too)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops!


#include "flutter/fml/logging.h"

namespace impeller {

_Half ScalarToHalf(Scalar f) {
#ifdef FML_OS_WIN
FML_LOG(ERROR) << "ScalarToHalf conversion on unsupported platform.";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What action can a developer who sees this take? This seems like a bug in the engine rather than a message for the end developer. Maybe a DCHECK for testing purposes?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, why isn't this supported on Windows? It seems like we should be able to write an implementation that truncates the way we want to uint16_t and back, but maybe I'm missing something.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now there is no reason to support this on any platform besides macOS/iOS. I'm using a clang specific feature to (hopefully) get hardware supported conversions. I could add a software conversion too

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could make this fatal?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A DCHECK is fatal for our unit tests but not users. Making it fatal for users seems too severe to me, but I'm not sure I understand the implications of going down this code path on an unexpected platform.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using this code on an unsupported platform implied that you constructed a HalfVectorN type manually. Nowhere in impeller do we do this except for unit tests, as the only references to it should be in the generated headers. On an unsupported platform the headers will not contain HalfVectorN types, and thus the implicit constructor will never be invoked.

So the possibility of this happening would be similar to the case of leaving exit(0) in our code somewhere. I should make it FML_UNREACHABLE just to be sure though.

#endif
return static_cast<_Half>(f);
}

} // namespace impeller
176 changes: 176 additions & 0 deletions impeller/geometry/half.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,176 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include <cstdint>

#include "flutter/fml/build_config.h"

#include "color.h"
#include "point.h"
#include "scalar.h"
#include "vector.h"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Includes should be rooted at the base of the project as seen above: https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


#ifdef FML_OS_WIN
using _Half = uint16_t;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a typename prefaced with _ doesn't match naming conventions: https://google.github.io/styleguide/cppguide.html#Type_Names

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

#else
using _Half = _Float16;
#endif

namespace impeller {

/// @brief Convert a scalar to a half precision float.
///
/// See also: https://clang.llvm.org/docs/LanguageExtensions.html
/// This is not currently supported on windows toolchains.
_Half ScalarToHalf(Scalar f);

/// @brief A storage only class for half precision floating point.
struct Half {
_Half x = 0;

constexpr Half() {}

constexpr Half(double value) : x(ScalarToHalf(static_cast<Scalar>(value))) {}

constexpr Half(Scalar value) : x(ScalarToHalf(value)) {}

constexpr Half(int value) : x(ScalarToHalf(static_cast<Scalar>(value))) {}

constexpr Half(_Half x) : x(x) {}

constexpr bool operator==(const Half& v) const { return v.x == x; }

constexpr bool operator!=(const Half& v) const { return v.x != x; }
};

/// @brief A storage only class for half precision floating point vector 4.
struct HalfVector4 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did templating the existing Scalar type vectors not work? This is fine too but seems like we can dry this up.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this above and backed it out. The big caveat is that 1) We need implicit constructor conversion to make the headers work 2) we generally don't want to ever use these in the backend besides that as it implies we're dropping precision before we need to. There is no performance improvement to be gained from this. In addition, if we end up needing to emulate _Float16 support for windows we'll likely end up needing to using uint16_t as a storage type and then we'd have to define all of the operators to make that work.

Given all that, I'd really rather just leave them as storage only conversions.

union {
struct {
_Half x = 0;
_Half y = 0;
_Half z = 0;
_Half w = 0;
};
_Half e[4];
};

constexpr HalfVector4() {}

constexpr HalfVector4(const Color& a)
: x(ScalarToHalf(a.red)),
y(ScalarToHalf(a.green)),
z(ScalarToHalf(a.blue)),
w(ScalarToHalf(a.alpha)) {}

constexpr HalfVector4(const Vector4& a)
: x(ScalarToHalf(a.x)),
y(ScalarToHalf(a.y)),
z(ScalarToHalf(a.z)),
w(ScalarToHalf(a.w)) {}

constexpr HalfVector4(_Half x, _Half y, _Half z, _Half w)
: x(x), y(y), z(z), w(w) {}

constexpr bool operator==(const HalfVector4& v) const {
return v.x == x && v.y == y && v.z == z && v.w == w;
}

constexpr bool operator!=(const HalfVector4& v) const {
return v.x != x || v.y != y || v.z != z || v.w != w;
}
};

/// @brief A storage only class for half precision floating point vector 3.
struct HalfVector3 {
union {
struct {
_Half x = 0;
_Half y = 0;
_Half z = 0;
};
_Half e[3];
};

constexpr HalfVector3() {}

constexpr HalfVector3(const Vector3& a)
: x(ScalarToHalf(a.x)), y(ScalarToHalf(a.y)), z(ScalarToHalf(a.z)) {}

constexpr HalfVector3(_Half x, _Half y, _Half z) : x(x), y(y), z(z) {}

constexpr bool operator==(const HalfVector3& v) const {
return v.x == x && v.y == y && v.z == z;
}

constexpr bool operator!=(const HalfVector3& v) const {
return v.x != x || v.y != y || v.z != z;
}
};

/// @brief A storage only class for half precision floating point vector 2.
struct HalfVector2 {
union {
struct {
_Half x = 0;
_Half y = 0;
};
_Half e[2];
};

constexpr HalfVector2() {}

constexpr HalfVector2(const Vector2& a)
: x(ScalarToHalf(a.x)), y(ScalarToHalf(a.y)) {}

constexpr HalfVector2(_Half x, _Half y) : x(x), y(y){};

constexpr bool operator==(const HalfVector2& v) const {
return v.x == x && v.y == y;
}

constexpr bool operator!=(const HalfVector2& v) const {
return v.x != x || v.y != y;
}
};

static_assert(sizeof(Half) == sizeof(uint16_t));
static_assert(sizeof(HalfVector2) == 2 * sizeof(Half));
static_assert(sizeof(HalfVector3) == 3 * sizeof(Half));
static_assert(sizeof(HalfVector4) == 4 * sizeof(Half));

} // namespace impeller

namespace std {

inline std::ostream& operator<<(std::ostream& out, const impeller::Half& p) {
out << "(" << static_cast<impeller::Scalar>(p.x) << ")";
return out;
}

inline std::ostream& operator<<(std::ostream& out,
const impeller::HalfVector2& p) {
out << "(" << static_cast<impeller::Scalar>(p.x) << ", "
<< static_cast<impeller::Scalar>(p.y) << ")";
return out;
}

inline std::ostream& operator<<(std::ostream& out,
const impeller::HalfVector3& p) {
out << "(" << static_cast<impeller::Scalar>(p.x) << ", "
<< static_cast<impeller::Scalar>(p.y) << ", "
<< static_cast<impeller::Scalar>(p.z) << ")";
return out;
}

inline std::ostream& operator<<(std::ostream& out,
const impeller::HalfVector4& p) {
out << "(" << static_cast<impeller::Scalar>(p.x) << ", "
<< static_cast<impeller::Scalar>(p.y) << ", "
<< static_cast<impeller::Scalar>(p.z) << ", "
<< static_cast<impeller::Scalar>(p.w) << ")";
return out;
}

} // namespace std
Loading