[PATCH v6 1/4] ipa: libipa: Add Vector class
David Plowman
david.plowman at raspberrypi.com
Mon Jun 10 12:33:39 CEST 2024
Hi Paul
Thank you for this patch.
On Mon, 10 Jun 2024 at 11:21, Kieran Bingham
<kieran.bingham at ideasonboard.com> 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
I just wanted to comment that this expression "identity vector" had me
confused for a moment. I assume we just mean the "zero vector" which,
it's true, is the identity element under the addition operation. But
then we often multiply vectors by scalars, or matrices, so it does
just make you pause for thought. Anyway, "zero vector" would have
caused my brain cells slightly less cognitive dislocation!
Thanks
David
> > + */
> > +
> > +/**
> > + * \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?
>
> > + */
> > +
> > +/**
> > + * \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?
>
> > + */
> > +
> > +/**
> > + * \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};
>
> or such. (Or some how typedef/using to create Vector2d, Vector3d?)
>
> Is R an appropriate name for a vector? Is D for dimensions any better?
> Or is it still better to call it Rows?
>
>
>
> > +#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 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?
>
> > + 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?)
>
> Unit tests would help here I think to confirm and validate intended
> purposes.
>
> > +
> > + 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