[v3,1/2] libcamera: libipa: Add right-shift operators to Vector

26718 diff mbox series

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>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 include/libcamera/internal/vector.h | 22 ++++++++++++++++++----
 src/libcamera/vector.cpp            | 14 ++++++++++++++
 test/vector.cpp                     |  5 +++++
 3 files changed, 37 insertions(+), 4 deletions(-)

Quoting Javier Tia (2026-05-11 21:23:28)
> 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>
> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  include/libcamera/internal/vector.h | 22 ++++++++++++++++++----
>  src/libcamera/vector.cpp            | 14 ++++++++++++++
>  test/vector.cpp                     |  5 +++++
>  3 files changed, 37 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)

I haven't understood why this needs to change, and does class U get used
implicitly ? I don't see anything changing the template parameters
elsewhere in this patch except...

>         {
>                 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)

here ... which I also can't understand how this is used ?

I removed these two hunks and the code still compiled and ran the tests.

What am I missing ? Does U mean unsigned version of T perhaps? or
something else implicit?



>         {
>                 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..94fbf61e 100644
> --- a/src/libcamera/vector.cpp
> +++ b/src/libcamera/vector.cpp
> @@ -126,6 +126,13 @@ 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+=(Vector const &other)
>   * \brief Add \a other element-wise to this vector
> @@ -182,6 +189,13 @@ LOG_DEFINE_CATEGORY(Vector)
>   * \return This vector
>   */
>  
> +/**
> + * \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::min(const Vector &other) const
>   * \brief Calculate the minimum of this vector and \a other element-wise
> diff --git a/test/vector.cpp b/test/vector.cpp
> index 4fae960d..4ff908e8 100644
> --- a/test/vector.cpp
> +++ b/test/vector.cpp
> @@ -93,6 +93,11 @@ protected:
>                 v2 /= 4.0;
>                 ASSERT_EQ(v2, (Vector<double, 3>{{ 1.0, 4.0, 8.0 }}));
>  
> +               Vector<int, 3> vi{{ 8, 16, 32 }};
> +               ASSERT_EQ(vi >> 2, (Vector<int, 3>{{ 2, 4, 8 }}));
> +               vi >>= 1;
> +               ASSERT_EQ(vi, (Vector<int, 3>{{ 4, 8, 16 }}));
> +

This upsets the checkstyle script / clang format - but I think we should
ignore it - as it's matching the style used for the tests above, and if
we fix checkstyle it should be applied to the whole file, not just this
patch hunk.


>                 return TestPass;
>         }
>  };
> 
> -- 
> 2.54.0
>

2026. 05. 12. 13:09 keltezéssel, Kieran Bingham írta:
> Quoting Javier Tia (2026-05-11 21:23:28)
>> 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>
>> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   include/libcamera/internal/vector.h | 22 ++++++++++++++++++----
>>   src/libcamera/vector.cpp            | 14 ++++++++++++++
>>   test/vector.cpp                     |  5 +++++
>>   3 files changed, 37 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)
> 
> I haven't understood why this needs to change, and does class U get used
> implicitly ? I don't see anything changing the template parameters
> elsewhere in this patch except...

It's so that the `unsigned int shift` parameter is not converted to `T`
in the `apply()` call (i.e. `U=unsigned int` is deduced). Since the range
of `shift` is limited, I'm not sure it makes a lot of practical difference,
but I think it's better to make it possible to preserve the type.


> 
>>          {
>>                  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)
> 
> here ... which I also can't understand how this is used ?
> 
> I removed these two hunks and the code still compiled and ran the tests.
> 
> What am I missing ? Does U mean unsigned version of T perhaps? or
> something else implicit?
> 
> 
> 
>>          {
>>                  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..94fbf61e 100644
>> --- a/src/libcamera/vector.cpp
>> +++ b/src/libcamera/vector.cpp
>> @@ -126,6 +126,13 @@ 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+=(Vector const &other)
>>    * \brief Add \a other element-wise to this vector
>> @@ -182,6 +189,13 @@ LOG_DEFINE_CATEGORY(Vector)
>>    * \return This vector
>>    */
>>
>> +/**
>> + * \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::min(const Vector &other) const
>>    * \brief Calculate the minimum of this vector and \a other element-wise
>> diff --git a/test/vector.cpp b/test/vector.cpp
>> index 4fae960d..4ff908e8 100644
>> --- a/test/vector.cpp
>> +++ b/test/vector.cpp
>> @@ -93,6 +93,11 @@ protected:
>>                  v2 /= 4.0;
>>                  ASSERT_EQ(v2, (Vector<double, 3>{{ 1.0, 4.0, 8.0 }}));
>>
>> +               Vector<int, 3> vi{{ 8, 16, 32 }};
>> +               ASSERT_EQ(vi >> 2, (Vector<int, 3>{{ 2, 4, 8 }}));
>> +               vi >>= 1;
>> +               ASSERT_EQ(vi, (Vector<int, 3>{{ 4, 8, 16 }}));
>> +
> 
> This upsets the checkstyle script / clang format - but I think we should
> ignore it - as it's matching the style used for the tests above, and if
> we fix checkstyle it should be applied to the whole file, not just this
> patch hunk.
> 
> 
>>                  return TestPass;
>>          }
>>   };
>>
>> --
>> 2.54.0
>>

Quoting Barnabás Pőcze (2026-05-12 12:59:12)
> 2026. 05. 12. 13:09 keltezéssel, Kieran Bingham írta:
> > Quoting Javier Tia (2026-05-11 21:23:28)
> >> 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>
> >> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >> ---
> >>   include/libcamera/internal/vector.h | 22 ++++++++++++++++++----
> >>   src/libcamera/vector.cpp            | 14 ++++++++++++++
> >>   test/vector.cpp                     |  5 +++++
> >>   3 files changed, 37 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)
> > 
> > I haven't understood why this needs to change, and does class U get used
> > implicitly ? I don't see anything changing the template parameters
> > elsewhere in this patch except...
> 
> It's so that the `unsigned int shift` parameter is not converted to `T`
> in the `apply()` call (i.e. `U=unsigned int` is deduced). Since the range
> of `shift` is limited, I'm not sure it makes a lot of practical difference,
> but I think it's better to make it possible to preserve the type.


Thanks. Templates are hard, but I'm happy with the rest of this, and
it's passing the tests so:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>