| Message ID | 20241118000738.18977-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, thank you for the patches. Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > The Vector class can be useful to represent RGB pixel values. Add r(), > g() and b() accessors, similar to x(), y() and z(), along with an RGB > type that aliases Vector<T, 3>. Good thing to have, including RGB alias. Is this expected to have the same access time as with e.g. struct RGB { T r, g, b; }; ? This is important in places where each CPU cycle counts, like software ISP debayering. > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/ipa/libipa/vector.cpp | 38 ++++++++++++++++++++++++++++++++++++++ > src/ipa/libipa/vector.h | 28 ++++++++++++++++++++++++++++ > 2 files changed, 66 insertions(+) > > diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp > index 8a39bfd27f90..f14f155216f3 100644 > --- a/src/ipa/libipa/vector.cpp > +++ b/src/ipa/libipa/vector.cpp > @@ -126,6 +126,39 @@ namespace ipa { > * \copydoc Vector::z() > */ > > +/** > + * \fn T &Vector::r() > + * \brief Convenience function to access the first element of the vector > + * \return The first element of the vector > + */ > + > +/** > + * \fn T &Vector::g() > + * \brief Convenience function to access the second element of the vector > + * \return The second element of the vector > + */ > + > +/** > + * \fn T &Vector::b() > + * \brief Convenience function to access the third element of the vector > + * \return The third element of the vector > + */ > + > +/** > + * \fn constexpr const T &Vector::r() const > + * \copydoc Vector::r() > + */ > + > +/** > + * \fn constexpr const T &Vector::g() const > + * \copydoc Vector::g() > + */ > + > +/** > + * \fn constexpr const T &Vector::b() const > + * \copydoc Vector::b() > + */ > + > /** > * \fn Vector::length2() > * \brief Get the squared length of the vector > @@ -149,6 +182,11 @@ namespace ipa { > * \return Product of matrix \a m and vector \a v > */ > > +/** > + * \typedef RGB > + * \brief A Vector of 3 elements representing an RGB pixel value > + */ > + > /** > * \fn bool operator==(const Vector<T, Rows> &lhs, const Vector<T, Rows> &rhs) > * \brief Compare vectors for equality > diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h > index 1b11a34deee4..b72ab9851aa3 100644 > --- a/src/ipa/libipa/vector.h > +++ b/src/ipa/libipa/vector.h > @@ -126,6 +126,31 @@ public: > #endif /* __DOXYGEN__ */ > T &z() { return data_[2]; } > > +#ifndef __DOXYGEN__ > + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 1>> > +#endif /* __DOXYGEN__ */ > + constexpr const T &r() const { return data_[0]; } > +#ifndef __DOXYGEN__ > + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 2>> > +#endif /* __DOXYGEN__ */ > + constexpr const T &g() const { return data_[1]; } > +#ifndef __DOXYGEN__ > + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 3>> > +#endif /* __DOXYGEN__ */ > + constexpr const T &b() const { return data_[2]; } > +#ifndef __DOXYGEN__ > + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 1>> > +#endif /* __DOXYGEN__ */ > + T &r() { return data_[0]; } > +#ifndef __DOXYGEN__ > + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 2>> > +#endif /* __DOXYGEN__ */ > + T &g() { return data_[1]; } > +#ifndef __DOXYGEN__ > + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 3>> > +#endif /* __DOXYGEN__ */ > + T &b() { return data_[2]; } > + > constexpr double length2() const > { > double ret = 0; > @@ -143,6 +168,9 @@ private: > std::array<T, Rows> data_; > }; > > +template<typename T> > +using RGB = Vector<T, 3>; > + > template<typename T, unsigned int Rows, unsigned int Cols> > Vector<T, Rows> operator*(const Matrix<T, Rows, Cols> &m, const Vector<T, Cols> &v) > {
Hi Milan, On Mon, Nov 18, 2024 at 12:02:29PM +0100, Milan Zamazal wrote: > Hi Laurent, > > thank you for the patches. > > Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > > > The Vector class can be useful to represent RGB pixel values. Add r(), > > g() and b() accessors, similar to x(), y() and z(), along with an RGB > > type that aliases Vector<T, 3>. > > Good thing to have, including RGB alias. There are a few things that bother me with the RGB alias. One is that we don't have an equivalent alias for YUV. We could add that, but then do we also need one for XYZ (see patch 12/12) ? And for other triplets that represent pixels or pixel-related values, such as gains ? I've also considered making RGB<T> inherit from Vector<T, 3>, and only define the r(), g() and b() accessors in the RGB class. The proble with that approach is that the operators return a Vector, so doing something like RGB<double> pixel; RGB<double> gains; auto result = pixel * gains; return result.r() would fail to compile as result would be a Vector<double, 3> instance. There could be ways around that but I'm not sure they're worth it. > Is this expected to have the same access time as with e.g. > > struct RGB { > T r, g, b; > }; > > ? This is important in places where each CPU cycle counts, like > software ISP debayering. Given that all the functions are inline, I believe the compiler should be able to generate the same code. It should be measured though. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > > > --- > > src/ipa/libipa/vector.cpp | 38 ++++++++++++++++++++++++++++++++++++++ > > src/ipa/libipa/vector.h | 28 ++++++++++++++++++++++++++++ > > 2 files changed, 66 insertions(+) > > > > diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp > > index 8a39bfd27f90..f14f155216f3 100644 > > --- a/src/ipa/libipa/vector.cpp > > +++ b/src/ipa/libipa/vector.cpp > > @@ -126,6 +126,39 @@ namespace ipa { > > * \copydoc Vector::z() > > */ > > > > +/** > > + * \fn T &Vector::r() > > + * \brief Convenience function to access the first element of the vector > > + * \return The first element of the vector > > + */ > > + > > +/** > > + * \fn T &Vector::g() > > + * \brief Convenience function to access the second element of the vector > > + * \return The second element of the vector > > + */ > > + > > +/** > > + * \fn T &Vector::b() > > + * \brief Convenience function to access the third element of the vector > > + * \return The third element of the vector > > + */ > > + > > +/** > > + * \fn constexpr const T &Vector::r() const > > + * \copydoc Vector::r() > > + */ > > + > > +/** > > + * \fn constexpr const T &Vector::g() const > > + * \copydoc Vector::g() > > + */ > > + > > +/** > > + * \fn constexpr const T &Vector::b() const > > + * \copydoc Vector::b() > > + */ > > + > > /** > > * \fn Vector::length2() > > * \brief Get the squared length of the vector > > @@ -149,6 +182,11 @@ namespace ipa { > > * \return Product of matrix \a m and vector \a v > > */ > > > > +/** > > + * \typedef RGB > > + * \brief A Vector of 3 elements representing an RGB pixel value > > + */ > > + > > /** > > * \fn bool operator==(const Vector<T, Rows> &lhs, const Vector<T, Rows> &rhs) > > * \brief Compare vectors for equality > > diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h > > index 1b11a34deee4..b72ab9851aa3 100644 > > --- a/src/ipa/libipa/vector.h > > +++ b/src/ipa/libipa/vector.h > > @@ -126,6 +126,31 @@ public: > > #endif /* __DOXYGEN__ */ > > T &z() { return data_[2]; } > > > > +#ifndef __DOXYGEN__ > > + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 1>> > > +#endif /* __DOXYGEN__ */ > > + constexpr const T &r() const { return data_[0]; } > > +#ifndef __DOXYGEN__ > > + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 2>> > > +#endif /* __DOXYGEN__ */ > > + constexpr const T &g() const { return data_[1]; } > > +#ifndef __DOXYGEN__ > > + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 3>> > > +#endif /* __DOXYGEN__ */ > > + constexpr const T &b() const { return data_[2]; } > > +#ifndef __DOXYGEN__ > > + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 1>> > > +#endif /* __DOXYGEN__ */ > > + T &r() { return data_[0]; } > > +#ifndef __DOXYGEN__ > > + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 2>> > > +#endif /* __DOXYGEN__ */ > > + T &g() { return data_[1]; } > > +#ifndef __DOXYGEN__ > > + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 3>> > > +#endif /* __DOXYGEN__ */ > > + T &b() { return data_[2]; } > > + > > constexpr double length2() const > > { > > double ret = 0; > > @@ -143,6 +168,9 @@ private: > > std::array<T, Rows> data_; > > }; > > > > +template<typename T> > > +using RGB = Vector<T, 3>; > > + > > template<typename T, unsigned int Rows, unsigned int Cols> > > Vector<T, Rows> operator*(const Matrix<T, Rows, Cols> &m, const Vector<T, Cols> &v) > > {
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > Hi Milan, > > On Mon, Nov 18, 2024 at 12:02:29PM +0100, Milan Zamazal wrote: >> Hi Laurent, >> >> thank you for the patches. >> >> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: >> >> > The Vector class can be useful to represent RGB pixel values. Add r(), >> > g() and b() accessors, similar to x(), y() and z(), along with an RGB >> > type that aliases Vector<T, 3>. >> >> Good thing to have, including RGB alias. > > There are a few things that bother me with the RGB alias. One is that > we don't have an equivalent alias for YUV. We could add that, but then > do we also need one for XYZ (see patch 12/12) ? And for other triplets > that represent pixels or pixel-related values, such as gains ? I'd say it should be approached reasonably. RGB is such a widespread thing. And I prefer RGB<double> gains; over /* Three items for red, green, blue. */ Vector<double, 3> gains; or double redGain, greenGain, blueGain; For YUV, maybe. The other cases are probably not worth their own aliases. > I've also considered making RGB<T> inherit from Vector<T, 3>, and only > define the r(), g() and b() accessors in the RGB class. The proble with > that approach is that the operators return a Vector, so doing something > like > > RGB<double> pixel; > RGB<double> gains; > > auto result = pixel * gains; > return result.r() > > would fail to compile as result would be a Vector<double, 3> instance. > There could be ways around that but I'm not sure they're worth it. It depends how much we care about type checking here (I mean e.g. mistakenly using RGB in place of YUV or so). >> Is this expected to have the same access time as with e.g. >> >> struct RGB { >> T r, g, b; >> }; >> >> ? This is important in places where each CPU cycle counts, like >> software ISP debayering. > > Given that all the functions are inline, I believe the compiler should > be able to generate the same code. It should be measured though. > >> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >> Reviewed-by: Milan Zamazal <mzamazal@redhat.com> >> >> > --- >> > src/ipa/libipa/vector.cpp | 38 ++++++++++++++++++++++++++++++++++++++ >> > src/ipa/libipa/vector.h | 28 ++++++++++++++++++++++++++++ >> > 2 files changed, 66 insertions(+) >> > >> > diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp >> > index 8a39bfd27f90..f14f155216f3 100644 >> > --- a/src/ipa/libipa/vector.cpp >> > +++ b/src/ipa/libipa/vector.cpp >> > @@ -126,6 +126,39 @@ namespace ipa { >> > * \copydoc Vector::z() >> > */ >> > >> > +/** >> > + * \fn T &Vector::r() >> > + * \brief Convenience function to access the first element of the vector >> > + * \return The first element of the vector >> > + */ >> > + >> > +/** >> > + * \fn T &Vector::g() >> > + * \brief Convenience function to access the second element of the vector >> > + * \return The second element of the vector >> > + */ >> > + >> > +/** >> > + * \fn T &Vector::b() >> > + * \brief Convenience function to access the third element of the vector >> > + * \return The third element of the vector >> > + */ >> > + >> > +/** >> > + * \fn constexpr const T &Vector::r() const >> > + * \copydoc Vector::r() >> > + */ >> > + >> > +/** >> > + * \fn constexpr const T &Vector::g() const >> > + * \copydoc Vector::g() >> > + */ >> > + >> > +/** >> > + * \fn constexpr const T &Vector::b() const >> > + * \copydoc Vector::b() >> > + */ >> > + >> > /** >> > * \fn Vector::length2() >> > * \brief Get the squared length of the vector >> > @@ -149,6 +182,11 @@ namespace ipa { >> > * \return Product of matrix \a m and vector \a v >> > */ >> > >> > +/** >> > + * \typedef RGB >> > + * \brief A Vector of 3 elements representing an RGB pixel value >> > + */ >> > + >> > /** >> > * \fn bool operator==(const Vector<T, Rows> &lhs, const Vector<T, Rows> &rhs) >> > * \brief Compare vectors for equality >> > diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h >> > index 1b11a34deee4..b72ab9851aa3 100644 >> > --- a/src/ipa/libipa/vector.h >> > +++ b/src/ipa/libipa/vector.h >> > @@ -126,6 +126,31 @@ public: >> > #endif /* __DOXYGEN__ */ >> > T &z() { return data_[2]; } >> > >> > +#ifndef __DOXYGEN__ >> > + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 1>> >> > +#endif /* __DOXYGEN__ */ >> > + constexpr const T &r() const { return data_[0]; } >> > +#ifndef __DOXYGEN__ >> > + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 2>> >> > +#endif /* __DOXYGEN__ */ >> > + constexpr const T &g() const { return data_[1]; } >> > +#ifndef __DOXYGEN__ >> > + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 3>> >> > +#endif /* __DOXYGEN__ */ >> > + constexpr const T &b() const { return data_[2]; } >> > +#ifndef __DOXYGEN__ >> > + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 1>> >> > +#endif /* __DOXYGEN__ */ >> > + T &r() { return data_[0]; } >> > +#ifndef __DOXYGEN__ >> > + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 2>> >> > +#endif /* __DOXYGEN__ */ >> > + T &g() { return data_[1]; } >> > +#ifndef __DOXYGEN__ >> > + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 3>> >> > +#endif /* __DOXYGEN__ */ >> > + T &b() { return data_[2]; } >> > + >> > constexpr double length2() const >> > { >> > double ret = 0; >> > @@ -143,6 +168,9 @@ private: >> > std::array<T, Rows> data_; >> > }; >> > >> > +template<typename T> >> > +using RGB = Vector<T, 3>; >> > + >> > template<typename T, unsigned int Rows, unsigned int Cols> >> > Vector<T, Rows> operator*(const Matrix<T, Rows, Cols> &m, const Vector<T, Cols> &v) >> > {
diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp index 8a39bfd27f90..f14f155216f3 100644 --- a/src/ipa/libipa/vector.cpp +++ b/src/ipa/libipa/vector.cpp @@ -126,6 +126,39 @@ namespace ipa { * \copydoc Vector::z() */ +/** + * \fn T &Vector::r() + * \brief Convenience function to access the first element of the vector + * \return The first element of the vector + */ + +/** + * \fn T &Vector::g() + * \brief Convenience function to access the second element of the vector + * \return The second element of the vector + */ + +/** + * \fn T &Vector::b() + * \brief Convenience function to access the third element of the vector + * \return The third element of the vector + */ + +/** + * \fn constexpr const T &Vector::r() const + * \copydoc Vector::r() + */ + +/** + * \fn constexpr const T &Vector::g() const + * \copydoc Vector::g() + */ + +/** + * \fn constexpr const T &Vector::b() const + * \copydoc Vector::b() + */ + /** * \fn Vector::length2() * \brief Get the squared length of the vector @@ -149,6 +182,11 @@ namespace ipa { * \return Product of matrix \a m and vector \a v */ +/** + * \typedef RGB + * \brief A Vector of 3 elements representing an RGB pixel value + */ + /** * \fn bool operator==(const Vector<T, Rows> &lhs, const Vector<T, Rows> &rhs) * \brief Compare vectors for equality diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h index 1b11a34deee4..b72ab9851aa3 100644 --- a/src/ipa/libipa/vector.h +++ b/src/ipa/libipa/vector.h @@ -126,6 +126,31 @@ public: #endif /* __DOXYGEN__ */ T &z() { return data_[2]; } +#ifndef __DOXYGEN__ + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 1>> +#endif /* __DOXYGEN__ */ + constexpr const T &r() const { return data_[0]; } +#ifndef __DOXYGEN__ + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 2>> +#endif /* __DOXYGEN__ */ + constexpr const T &g() const { return data_[1]; } +#ifndef __DOXYGEN__ + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 3>> +#endif /* __DOXYGEN__ */ + constexpr const T &b() const { return data_[2]; } +#ifndef __DOXYGEN__ + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 1>> +#endif /* __DOXYGEN__ */ + T &r() { return data_[0]; } +#ifndef __DOXYGEN__ + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 2>> +#endif /* __DOXYGEN__ */ + T &g() { return data_[1]; } +#ifndef __DOXYGEN__ + template<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 3>> +#endif /* __DOXYGEN__ */ + T &b() { return data_[2]; } + constexpr double length2() const { double ret = 0; @@ -143,6 +168,9 @@ private: std::array<T, Rows> data_; }; +template<typename T> +using RGB = Vector<T, 3>; + template<typename T, unsigned int Rows, unsigned int Cols> Vector<T, Rows> operator*(const Matrix<T, Rows, Cols> &m, const Vector<T, Cols> &v) {
The Vector class can be useful to represent RGB pixel values. Add r(), g() and b() accessors, similar to x(), y() and z(), along with an RGB type that aliases Vector<T, 3>. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/ipa/libipa/vector.cpp | 38 ++++++++++++++++++++++++++++++++++++++ src/ipa/libipa/vector.h | 28 ++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+)