| Message ID | 20260508223733.DA0831EA006C@mailuser.phl.internal |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi 2026. 05. 08. 23:35 keltezéssel, Javier Tia írta: > Add operator>> and operator>>= for right-shifting all elements by a > scalar shift amount. Both operate element-wise: operator>> returns a > new Vector with each element shifted, operator>>= shifts in place and > returns a reference to *this. > > The motivating use case is SwStatsCpu::finishFrame(), which right-shifts > three individual components of the RGB sum by the same sumShift_ value. > With these operators that becomes a single sum_ >>= sumShift_ expression. > > The private apply() helpers for non-mutating and mutating scalar > operations are templated on the scalar type so the existing operators > keep working with T while shift can pass an unsigned int. A > static_assert in each shift operator restricts them to integer element > types since right-shift is undefined on floating-point Vectors. Unfortunately `std::is_integral_v<bool> == true`, but I think we should ignore that. > > Suggested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Javier Tia <floss@jetm.me> > --- > include/libcamera/internal/vector.h | 22 ++++++++++++++++++---- > src/libcamera/vector.cpp | 14 ++++++++++++++ > 2 files changed, 32 insertions(+), 4 deletions(-) > > diff --git a/include/libcamera/internal/vector.h b/include/libcamera/internal/vector.h > index ed7490e1..e3ea723d 100644 > --- a/include/libcamera/internal/vector.h > +++ b/include/libcamera/internal/vector.h > @@ -111,6 +111,13 @@ public: > return apply(*this, scalar, std::divides<>{}); > } > > + constexpr Vector operator>>(unsigned int shift) const > + { > + static_assert(std::is_integral_v<T>, > + "Vector::operator>> requires an integer element type"); > + return apply(*this, shift, [](T a, unsigned int b) { return a >> b; }); > + } > + > Vector &operator+=(const Vector &other) > { > return apply(other, [](T a, T b) { return a + b; }); > @@ -151,6 +158,13 @@ public: > return apply(scalar, [](T a, T b) { return a / b; }); > } > > + Vector &operator>>=(unsigned int shift) > + { > + static_assert(std::is_integral_v<T>, > + "Vector::operator>>= requires an integer element type"); > + return apply(shift, [](T a, unsigned int b) { return a >> b; }); > + } > + > constexpr Vector min(const Vector &other) const > { > return apply(*this, other, [](T a, T b) { return std::min(a, b); }); > @@ -260,8 +274,8 @@ private: > return result; > } > > - template<class BinaryOp> > - static constexpr Vector apply(const Vector &lhs, T rhs, BinaryOp op) > + template<class U, class BinaryOp> > + static constexpr Vector apply(const Vector &lhs, U rhs, BinaryOp op) > { > Vector result; > std::transform(lhs.data_.begin(), lhs.data_.end(), > @@ -281,8 +295,8 @@ private: > return *this; > } > > - template<class BinaryOp> > - Vector &apply(T scalar, BinaryOp op) > + template<class U, class BinaryOp> > + Vector &apply(U scalar, BinaryOp op) > { > std::for_each(data_.begin(), data_.end(), > [&op, scalar](T &v) { v = op(v, scalar); }); > diff --git a/src/libcamera/vector.cpp b/src/libcamera/vector.cpp > index 86b9f9bb..d3b7e4ab 100644 > --- a/src/libcamera/vector.cpp > +++ b/src/libcamera/vector.cpp > @@ -126,6 +126,20 @@ LOG_DEFINE_CATEGORY(Vector) > * \return The element-wise division of this vector by \a scalar > */ > > +/** > + * \fn Vector::operator>>(unsigned int shift) const > + * \brief Right-shift each element of this vector by \a shift bits > + * \param[in] shift The shift amount > + * \return A new vector with each element right-shifted by \a shift > + */ > + > +/** > + * \fn Vector::operator>>=(unsigned int shift) > + * \brief Right-shift each element of this vector by \a shift bits in place > + * \param[in] shift The shift amount > + * \return This vector > + */ I think it would be best to preserve the order, i.e. moving the `=` variant to be after `operator/=`. Otherwise the code looks ok to me. Maybe adding a couple tests to `test/vector.cpp` would be nice. Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > + > /** > * \fn Vector::operator+=(Vector const &other) > * \brief Add \a other element-wise to this vector
diff --git a/include/libcamera/internal/vector.h b/include/libcamera/internal/vector.h index ed7490e1..e3ea723d 100644 --- a/include/libcamera/internal/vector.h +++ b/include/libcamera/internal/vector.h @@ -111,6 +111,13 @@ public: return apply(*this, scalar, std::divides<>{}); } + constexpr Vector operator>>(unsigned int shift) const + { + static_assert(std::is_integral_v<T>, + "Vector::operator>> requires an integer element type"); + return apply(*this, shift, [](T a, unsigned int b) { return a >> b; }); + } + Vector &operator+=(const Vector &other) { return apply(other, [](T a, T b) { return a + b; }); @@ -151,6 +158,13 @@ public: return apply(scalar, [](T a, T b) { return a / b; }); } + Vector &operator>>=(unsigned int shift) + { + static_assert(std::is_integral_v<T>, + "Vector::operator>>= requires an integer element type"); + return apply(shift, [](T a, unsigned int b) { return a >> b; }); + } + constexpr Vector min(const Vector &other) const { return apply(*this, other, [](T a, T b) { return std::min(a, b); }); @@ -260,8 +274,8 @@ private: return result; } - template<class BinaryOp> - static constexpr Vector apply(const Vector &lhs, T rhs, BinaryOp op) + template<class U, class BinaryOp> + static constexpr Vector apply(const Vector &lhs, U rhs, BinaryOp op) { Vector result; std::transform(lhs.data_.begin(), lhs.data_.end(), @@ -281,8 +295,8 @@ private: return *this; } - template<class BinaryOp> - Vector &apply(T scalar, BinaryOp op) + template<class U, class BinaryOp> + Vector &apply(U scalar, BinaryOp op) { std::for_each(data_.begin(), data_.end(), [&op, scalar](T &v) { v = op(v, scalar); }); diff --git a/src/libcamera/vector.cpp b/src/libcamera/vector.cpp index 86b9f9bb..d3b7e4ab 100644 --- a/src/libcamera/vector.cpp +++ b/src/libcamera/vector.cpp @@ -126,6 +126,20 @@ LOG_DEFINE_CATEGORY(Vector) * \return The element-wise division of this vector by \a scalar */ +/** + * \fn Vector::operator>>(unsigned int shift) const + * \brief Right-shift each element of this vector by \a shift bits + * \param[in] shift The shift amount + * \return A new vector with each element right-shifted by \a shift + */ + +/** + * \fn Vector::operator>>=(unsigned int shift) + * \brief Right-shift each element of this vector by \a shift bits in place + * \param[in] shift The shift amount + * \return This vector + */ + /** * \fn Vector::operator+=(Vector const &other) * \brief Add \a other element-wise to this vector
Add operator>> and operator>>= for right-shifting all elements by a scalar shift amount. Both operate element-wise: operator>> returns a new Vector with each element shifted, operator>>= shifts in place and returns a reference to *this. The motivating use case is SwStatsCpu::finishFrame(), which right-shifts three individual components of the RGB sum by the same sumShift_ value. With these operators that becomes a single sum_ >>= sumShift_ expression. The private apply() helpers for non-mutating and mutating scalar operations are templated on the scalar type so the existing operators keep working with T while shift can pass an unsigned int. A static_assert in each shift operator restricts them to integer element types since right-shift is undefined on floating-point Vectors. Suggested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Javier Tia <floss@jetm.me> --- include/libcamera/internal/vector.h | 22 ++++++++++++++++++---- src/libcamera/vector.cpp | 14 ++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-)