[PATCH v6 1/4] ipa: libipa: Add Vector class
Paul Elder
paul.elder at ideasonboard.com
Mon Jun 10 14:57:08 CEST 2024
On Mon, Jun 10, 2024 at 11:21:35AM +0100, Kieran Bingham wrote:
> Quoting Paul Elder (2024-06-07 09:42:58)
> > 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>
> >
> > ---
> > No change in v6
> >
> > New in v5
> > ---
> > src/ipa/libipa/vector.cpp | 162 ++++++++++++++++++++++++++++++
> > src/ipa/libipa/vector.h | 206 ++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 368 insertions(+)
> > create mode 100644 src/ipa/libipa/vector.cpp
> > create mode 100644 src/ipa/libipa/vector.h
> >
> > diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp
> > new file mode 100644
> > index 000000000..bdc3c447c
> > --- /dev/null
> > +++ b/src/ipa/libipa/vector.cpp
> > @@ -0,0 +1,162 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/*
> > + * Copyright (C) 2019, Raspberry Pi Ltd
> > + * 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 R Number of rows in the vector
> > + */
> > +
> > +/**
> > + * \fn Vector::Vector()
> > + * \brief Construct an identity vector
> > + */
> > +
> > +/**
> > + * \fn Vector::Vector(const std::array<T, R> &data)
> > + * \brief Construct vector from supplied data
> > + * \param data Data from which to construct a vector
> > + *
> > + * \a data is a one-dimensional vector and will be turned into a vector in
> > + * row-major order. The size of \a data must be equal to the product of the
> > + * number of rows and columns of the vector (RxC).
>
> Rows and columns of a vector?
Oops, I copied from Matrix and forgot to update the documentation...
>
> > + */
> > +
> > +/**
> > + * \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 product of the number of rows and
> > + * columns of the vector (RxC).
> > + *
> > + * The yaml data is expected to be a list with elements of type T.
> > + *
> > + * \return 0 on success, negative error code otherwise
> > + */
> > +
> > +/**
> > + * \fn Vector::toString
> > + * \brief Assemble and return a string describing the vector
> > + * \return A string describing the vector
> > + */
> > +
> > +/**
> > + * \fn T Vector::operator[](size_t i) const
> > + * \brief Index to a row in the vector
> > + * \param i Index of row to retrieve
> > + *
> > + * This operator[] returns a Span, which can then be indexed into again with
> > + * another operator[], allowing a convenient m[i][j] to access elements of the
> > + * vector. Note that the lifetime of the Span returned by this first-level
> > + * operator[] is bound to that of the Vector itself, so it is not recommended
> > + * to save the Span that is the result of this operator[].
> > + *
> > + * \return Row \a i from the vector, as a Span
>
> Are these needed for a vector? I don't understand the indexing of a
> vector? I could understand on a matrix - but isn't a vector just a set
> of coordinates that represent direction/magnitude?
>
> I don't understand what m[i][j] is used for ? (or represents?)
>
> Perhaps the hint 'm' means this is a left over and needs to be removed?
Yeep...
>
> > + */
> > +
> > +/**
> > + * \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
> > + */
> > +
> > +/**
> > + * \fn Vector::y()
> > + * \brief Convenience function to access the second element of the vector
> > + */
>
> These I understand :-)
>
>
>
> > +
> > +/**
> > + * \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, R> &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 Scale up the vector
> > + * \param[in] factor The factor
> > + * \return The vector scaled up by \a factor
> > + */
> > +
> > +/**
> > + * \fn Vector::operator/()
> > + * \brief Scale down the vector
> > + * \param[in] factor The factor
> > + * \return The vector scaled down by \a factor
> > + */
> > +
> > +/**
> > + * \fn Vector::len2()
> > + * \brief Get the squared length of the vector
> > + * \return The squared length of the vector
> > + */
> > +
> > +/**
> > + * \fn Vector::len()
> > + * \brief Get the length of the vector
> > + * \return The length of the vector
> > + */
>
> I think all of those probably deserve at least some level of a unit
> test. How come we don't seem to have tests for libipa - Seems like the
> sort of thing we should have... Particularly as you've got operations on
> differing types above.
>
>
>
> > +
> > +/**
> > + * \fn bool operator==(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
> > + * \brief Compare vectors for equality
> > + * \return True if the two vectors are equal, false otherwise
> > + */
> > +
> > +/**
> > + * \fn bool operator!=(const Vector<T, R> &lhs, const Vector<T, R> &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 000000000..87aad28b5
> > --- /dev/null
> > +++ b/src/ipa/libipa/vector.h
> > @@ -0,0 +1,206 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/*
> > + * Copyright (C) 2019, Raspberry Pi Ltd
> > + * 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 R,
> > + std::enable_if_t<std::is_arithmetic_v<T> && R >= 2> * = nullptr>
> > +#else
> > +template<typename T, unsigned int R>
>
> Do we anticipate 3d vectors? (or other dimesions?)
>
> How about defaulting R = 2 so that a 2d vector is the default case?
>
> Then we can use
> Vector<int> v = {x, y};
I didn't think that Vector<int, 2> was a very high cost? It's only two
more characters.
>
> or such. (Or some how typedef/using to create Vector2d, Vector3d?)
That would be the preferred alternative imo.
>
> Is R an appropriate name for a vector? Is D for dimensions any better?
> Or is it still better to call it Rows?
Hm, maybe dimensions would be better...
>
>
>
> > +#endif /* __DOXYGEN__ */
> > +class Vector
> > +{
> > +public:
> > + Vector() = default;
> > +
> > + Vector(const std::array<T, R> &data)
> > + {
> > + ASSERT(data.size() == R);
>
> Optional, but as we provide direct accessors for x() and y() perhaps:
>
> ASSERT(R >= 2);
That's already enforced by SFINAE in the class definition... unless
SFINAE doesn't kick in in this case...?
>
> That might stop accidentally creating a vector of one dimesion..
>
>
> > +
> > + for (unsigned int i = 0; i < R; i++)
> > + data_[i] = data[i];
> > + }
> > +
> > + ~Vector() = default;
> > +
> > + int readYaml(const libcamera::YamlObject &yaml)
> > + {
> > + if (yaml.size() != R) {
> > + LOG(Vector, Error)
> > + << "Wrong number of values in vector: expected "
> > + << R << ", 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 std::string toString() const
> > + {
> > + std::stringstream out;
> > +
> > + out << "Vector { ";
> > + for (unsigned int i = 0; i < R; i++) {
> > + out << (*this)[i];
> > + out << ((i + 1 < R) ? ", " : " ");
> > + }
> > + out << " }";
> > +
> > + return out.str();
> > + }
> > +
> > + const T operator[](size_t i) const
> > + {
>
> ASSERT here if out of bounds?
Ah, right.
>
> > + return data_[i];
> > + }
> > +
> > + T &operator[](size_t i)
> > + {
> > + return data_[i];
> > + }
> > +
> > + const T x() const
> > + {
> > + return data_[0];
> > + }
> > +
> > + const T y() const
> > + {
> > + return data_[1];
> > + }
> > +
> > + constexpr Vector<T, R> operator-() const
> > + {
> > + Vector<T, R> ret;
> > + for (unsigned int i = 0; i < R; i++)
> > + ret[i] = -data_[i];
> > + return ret;
> > + }
>
> How would this be used? (differently than below?)
This is a unary negation operator, the below is a binary subtration
operator (did I forget to document it...?)
>
> Unit tests would help here I think to confirm and validate intended
> purposes.
>
I think I'll do that on top (along with the ones for matrices)...
Thanks,
Paul
> > +
> > + constexpr Vector<T, R> operator-(const Vector<T, R> &other) const
> > + {
> > + Vector<T, R> ret;
> > + for (unsigned int i = 0; i < R; i++)
> > + ret[i] = data_[i] - other[i];
> > + return ret;
> > + }
> > +
> > + constexpr Vector<T, R> operator+(const Vector<T, R> &other) const
> > + {
> > + Vector<T, R> ret;
> > + for (unsigned int i = 0; i < R; i++)
> > + ret[i] = data_[i] + other[i];
> > + return ret;
> > + }
> > +
> > + constexpr T operator*(const Vector<T, R> &other) const
> > + {
> > + T ret = 0;
> > + for (unsigned int i = 0; i < R; i++)
> > + ret += data_[i] * other[i];
> > + return ret;
> > + }
> > +
> > + constexpr Vector<T, R> operator*(T factor) const
> > + {
> > + Vector<T, R> ret;
> > + for (unsigned int i = 0; i < R; i++)
> > + ret[i] = data_[i] * factor;
> > + return ret;
> > + }
> > +
> > + constexpr Vector<T, R> operator/(T factor) const
> > + {
> > + Vector<T, R> ret;
> > + for (unsigned int i = 0; i < R; i++)
> > + ret[i] = data_[i] / factor;
> > + return ret;
> > + }
> > +
> > + constexpr T len2() const
> > + {
> > + T ret = 0;
> > + for (unsigned int i = 0; i < R; i++)
> > + ret += data_[i] * data_[i];
> > + return ret;
> > + }
> > +
> > + constexpr double len() const
> > + {
> > + return std::sqrt(len2());
> > + }
> > +
> > +private:
> > + std::array<T, R> data_;
> > +};
>
>
> > +
> > +#ifndef __DOXYGEN__
> > +template<typename T, unsigned int R,
> > + std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> > +#endif /* __DOXYGEN__ */
> > +bool operator==(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
> > +{
> > + for (unsigned int i = 0; i < R; i++)
> > + if (lhs[i] != rhs[i])
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +#ifndef __DOXYGEN__
> > +template<typename T, unsigned int R,
> > + std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> > +#endif /* __DOXYGEN__ */
> > +bool operator!=(const Vector<T, R> &lhs, const Vector<T, R> &rhs)
> > +{
> > + for (unsigned int i = 0; i < R; i++)
> > + if (lhs[i] != rhs[i])
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > +} /* namespace ipa */
> > +
> > +#ifndef __DOXYGEN__
> > +template<typename T, unsigned int R>
> > +std::ostream &operator<<(std::ostream &out, const ipa::Vector<T, R> &v)
> > +{
> > + out << v.toString();
> > + return out;
> > +}
> > +#endif /* __DOXYGEN__ */
> > +
> > +} /* namespace libcamera */
> > --
> > 2.39.2
> >
More information about the libcamera-devel
mailing list