[PATCH v8 1/4] ipa: libipa: Add Vector class

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Tue, Jun 11, 2024 at 10:24:27PM +0900, Paul Elder wrote:
> Add a vector class to libipa. The original purpose of this is to replace
> the floating-point Point class that Raspberry Pi used in their Pwl, as
> that implementation of Point seemed more akin to a Vector than a Point.
> 
> This is added to libipa instead of to geometry.h to avoid public API
> issues, plus this is not expected to be needed by applications.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> Reviewed-by: Stefan Klug <stefan.klug at ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> ---
> Changes in v8:
> - s/D/Rows/ in Vector class
> - add z accessor function
> - move SFINAE for x() and y() (and z()) to the functions instead of the
>   class
> - add vector to meson
> 
> Changes in v7:
> - fix documentation
> - fix license and copyright
> - remove toString()
> 
> No change in v6
> 
> New in v5
> ---
>  src/ipa/libipa/meson.build |   2 +
>  src/ipa/libipa/vector.cpp  | 150 +++++++++++++++++++++++++++
>  src/ipa/libipa/vector.h    | 203 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 355 insertions(+)
>  create mode 100644 src/ipa/libipa/vector.cpp
>  create mode 100644 src/ipa/libipa/vector.h
> 
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index 7ce885da7b99..8b0c8fff901b 100644
> --- a/src/ipa/libipa/meson.build
> +++ b/src/ipa/libipa/meson.build
> @@ -8,6 +8,7 @@ libipa_headers = files([
>      'fc_queue.h',
>      'histogram.h',
>      'module.h',
> +    'vector.h',
>  ])
>  
>  libipa_sources = files([
> @@ -18,6 +19,7 @@ libipa_sources = files([
>      'fc_queue.cpp',
>      'histogram.cpp',
>      'module.cpp',
> +    'vector.cpp',
>  ])
>  
>  libipa_includes = include_directories('..')
> diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp
> new file mode 100644
> index 000000000000..b9fad90386a1
> --- /dev/null
> +++ b/src/ipa/libipa/vector.cpp
> @@ -0,0 +1,150 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Paul Elder <paul.elder at ideasonboard.com>
> + *
> + * Vector and related operations
> + */
> +
> +#include "vector.h"
> +
> +#include <libcamera/base/log.h>
> +
> +/**
> + * \file vector.h
> + * \brief Vector class
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(Vector)
> +
> +namespace ipa {
> +
> +/**
> + * \class Vector
> + * \brief Vector class
> + * \tparam T Type of numerical values to be stored in the vector
> + * \tparam Rows Number of dimension of the vector (= number of elements)
> + */
> +
> +/**
> + * \fn Vector::Vector()
> + * \brief Construct a zero vector
> + */
> +
> +/**
> + * \fn Vector::Vector(const std::array<T, Rows> &data)
> + * \brief Construct vector from supplied data
> + * \param data Data from which to construct a vector
> + *
> + * The size of \a data must be equal to the dimension size Rows of the vector.
> + */
> +
> +/**
> + * \fn Vector::readYaml
> + * \brief Populate the vector with yaml data
> + * \param yaml Yaml data to populate the vector with
> + *
> + * Any existing data in the vector will be overwritten. The size of the data
> + * read from \a yaml must be equal to the dimension size Rows of the vector.
> + *
> + * The yaml data is expected to be a list with elements of type T.
> + *
> + * \return 0 on success, negative error code otherwise
> + */
> +
> +/**
> + * \fn T Vector::operator[](size_t i) const
> + * \brief Index to an element in the vector
> + * \param i Index of element to retrieve
> + * \return Element at index \a i from the vector
> + */
> +
> +/**
> + * \fn T &Vector::operator[](size_t i)
> + * \copydoc Vector::operator[](size_t i) const
> + */
> +
> +/**
> + * \fn Vector::x()
> + * \brief Convenience function to access the first element of the vector

 * \return The first element of the vector

Same below.

> + */
> +
> +/**
> + * \fn Vector::y()
> + * \brief Convenience function to access the second element of the vector
> + */
> +
> +/**
> + * \fn Vector::z()
> + * \brief Convenience function to access the third element of the vector
> + */
> +
> +/**
> + * \fn Vector::operator-() const
> + * \brief Negate a Vector by negating both all of its coordinates
> + * \return The negated vector
> + */
> +
> +/**
> + * \fn Vector::operator-(Vector const &other) const
> + * \brief Subtract one vector from another
> + * \param[in] other The other vector
> + * \return The difference of \a other from this vector
> + */
> +
> +/**
> + * \fn Vector::operator+()
> + * \brief Add two vectors together
> + * \param[in] other The other vector
> + * \return The sum of the two vectors
> + */
> +
> +/**
> + * \fn Vector::operator*(const Vector<T, Rows> &other) const
> + * \brief Compute the dot product
> + * \param[in] other The other vector
> + * \return The dot product of the two vectors
> + */
> +
> +/**
> + * \fn Vector::operator*(T factor) const
> + * \brief Multiply the vector by a scalar
> + * \param[in] factor The factor
> + * \return The vector multiplied by \a factor
> + */
> +
> +/**
> + * \fn Vector::operator/()
> + * \brief Divide the vector by a scalar
> + * \param[in] factor The factor
> + * \return The vector divided by \a factor
> + */
> +
> +/**
> + * \fn Vector::length2()
> + * \brief Get the squared length of the vector
> + * \return The squared length of the vector
> + */
> +
> +/**
> + * \fn Vector::length()
> + * \brief Get the length of the vector
> + * \return The length of the vector
> + */
> +
> +/**
> + * \fn bool operator==(const Vector<T, Rows> &lhs, const Vector<T, Rows> &rhs)
> + * \brief Compare vectors for equality
> + * \return True if the two vectors are equal, false otherwise
> + */
> +
> +/**
> + * \fn bool operator!=(const Vector<T, Rows> &lhs, const Vector<T, Rows> &rhs)
> + * \brief Compare vectors for inequality
> + * \return True if the two vectors are not equal, false otherwise
> + */
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h
> new file mode 100644
> index 000000000000..b281f079365a
> --- /dev/null
> +++ b/src/ipa/libipa/vector.h
> @@ -0,0 +1,203 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Paul Elder <paul.elder at ideasonboard.com>
> + *
> + * Vector and related operations
> + */
> +#pragma once
> +
> +#include <algorithm>
> +#include <array>
> +#include <cmath>
> +#include <sstream>
> +
> +#include <libcamera/base/log.h>
> +#include <libcamera/base/span.h>
> +
> +#include "libcamera/internal/yaml_parser.h"
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(Vector)
> +
> +namespace ipa {
> +
> +#ifndef __DOXYGEN__
> +template<typename T, unsigned int Rows,
> +	 std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> +#else
> +template<typename T, unsigned int Rows>
> +#endif /* __DOXYGEN__ */
> +class Vector
> +{
> +public:
> +	constexpr Vector() = default;
> +
> +	constexpr Vector(const std::array<T, Rows> &data)
> +	{
> +		for (unsigned int i = 0; i < Rows; i++)
> +			data_[i] = data[i];
> +	}

A constructor taking an initializer list may be useful. It can be added
later.

> +
> +	int readYaml(const libcamera::YamlObject &yaml)

I still don't like having this as a member function. I'm working on
handling this on top, so it's not a blocker.

> +	{
> +		if (yaml.size() != Rows) {
> +			LOG(Vector, Error)
> +				<< "Wrong number of values in vector: expected "
> +				<< Rows << ", got " << yaml.size();
> +			return -EINVAL;
> +		}
> +
> +		unsigned int i = 0;
> +		for (const auto &x : yaml.asList()) {
> +			auto value = x.get<T>();
> +			if (!value) {
> +				LOG(Vector, Error) << "Failed to read vector value";
> +				return -EINVAL;
> +			}
> +
> +			data_[i++] = *value;
> +		}
> +
> +		return 0;
> +	}
> +
> +	const T &operator[](size_t i) const
> +	{
> +		ASSERT(i < data_.size());
> +		return data_[i];
> +	}
> +
> +	T &operator[](size_t i)
> +	{
> +		ASSERT(i < data_.size());
> +		return data_[i];
> +	}
> +
> +#ifndef __DOXYGEN__
> +	template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 1>>
> +#endif /* __DOXYGEN__ */
> +	constexpr T x() const
> +	{
> +		return data_[0];
> +	}
> +
> +#ifndef __DOXYGEN__
> +	template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 2>>
> +#endif /* __DOXYGEN__ */
> +	constexpr T y() const
> +	{
> +		return data_[1];
> +	}
> +
> +#ifndef __DOXYGEN__
> +	template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 3>>
> +#endif /* __DOXYGEN__ */
> +	constexpr T z() const
> +	{
> +		return data_[2];
> +	}
> +
> +	constexpr Vector<T, Rows> operator-() const
> +	{
> +		Vector<T, Rows> ret;
> +		for (unsigned int i = 0; i < Rows; i++)
> +			ret[i] = -data_[i];
> +		return ret;
> +	}
> +
> +	constexpr Vector<T, Rows> operator-(const Vector<T, Rows> &other) const
> +	{
> +		Vector<T, Rows> ret;
> +		for (unsigned int i = 0; i < Rows; i++)
> +			ret[i] = data_[i] - other[i];
> +		return ret;
> +	}
> +
> +	constexpr Vector<T, Rows> operator+(const Vector<T, Rows> &other) const
> +	{
> +		Vector<T, Rows> ret;
> +		for (unsigned int i = 0; i < Rows; i++)
> +			ret[i] = data_[i] + other[i];
> +		return ret;
> +	}
> +
> +	constexpr T operator*(const Vector<T, Rows> &other) const
> +	{
> +		T ret = 0;
> +		for (unsigned int i = 0; i < Rows; i++)
> +			ret += data_[i] * other[i];
> +		return ret;
> +	}
> +
> +	constexpr Vector<T, Rows> operator*(T factor) const
> +	{
> +		Vector<T, Rows> ret;
> +		for (unsigned int i = 0; i < Rows; i++)
> +			ret[i] = data_[i] * factor;
> +		return ret;
> +	}
> +
> +	constexpr Vector<T, Rows> operator/(T factor) const
> +	{
> +		Vector<T, Rows> ret;
> +		for (unsigned int i = 0; i < Rows; i++)
> +			ret[i] = data_[i] / factor;
> +		return ret;
> +	}
> +
> +	constexpr double length2() const
> +	{
> +		double ret = 0;
> +		for (unsigned int i = 0; i < Rows; i++)
> +			ret += data_[i] * data_[i];
> +		return ret;
> +	}
> +
> +	constexpr double length() const
> +	{
> +		return std::sqrt(length2());
> +	}
> +
> +private:
> +	std::array<T, Rows> data_;
> +};
> +
> +template<typename T, unsigned int Rows>
> +bool operator==(const Vector<T, Rows> &lhs, const Vector<T, Rows> &rhs)
> +{
> +	for (unsigned int i = 0; i < Rows; i++)
> +		if (lhs[i] != rhs[i])
> +			return false;

Curly braces around the for statement. Same below.

> +
> +	return true;
> +}
> +
> +template<typename T, unsigned int Rows>
> +bool operator!=(const Vector<T, Rows> &lhs, const Vector<T, Rows> &rhs)
> +{

	return !(lhs == rhs);

should do.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +	for (unsigned int i = 0; i < Rows; i++)
> +		if (lhs[i] != rhs[i])
> +			return true;
> +
> +	return false;
> +}
> +
> +} /* namespace ipa */
> +
> +#ifndef __DOXYGEN__
> +template<typename T, unsigned int Rows>
> +std::ostream &operator<<(std::ostream &out, const ipa::Vector<T, Rows> &v)
> +{
> +	out << "Vector { ";
> +	for (unsigned int i = 0; i < Rows; i++) {
> +		out << v[i];
> +		out << ((i + 1 < Rows) ? ", " : " ");
> +	}
> +	out << " }";
> +
> +	return out;
> +}
> +#endif /* __DOXYGEN__ */
> +
> +} /* namespace libcamera */

-- 
Regards,

Laurent Pinchart