[PATCH 01/10] libcamera: matrix: Add cast function

[Barnabás Pőcze]

Hi


2025. február 17., hétfő 16:33 keltezéssel, Stefan Klug <stefan.klug at ideasonboard.com> írta:

> On Mon, Feb 17, 2025 at 04:31:26PM +0100, Stefan Klug wrote:
> > Hi Barnabás,
> >
> > On Mon, Feb 17, 2025 at 03:13:47PM +0000, Barnabás Pőcze wrote:
> > > Hi
> > >
> > >
> > > 2025. február 17., hétfő 16:02 keltezéssel, Stefan Klug <stefan.klug at ideasonboard.com> írta:
> > >
> > > > Hi Laurent,
> > > >
> > > > Thank you for the review.
> > > >
> > > > On Mon, Feb 17, 2025 at 01:42:52PM +0200, Laurent Pinchart wrote:
> > > > > Hi Stefan,
> > > > >
> > > > > Thank you for the patch.
> > > > >
> > > > > On Mon, Feb 17, 2025 at 11:01:42AM +0100, Stefan Klug wrote:
> > > > > > The libcamera code base uses matrices and vectors of type float and
> > > > > > double. Add a cast function to easily convert between different
> > > > > > underlying types.
> > > > >
> > > > > Should we standardize on one of the floating point precisions to avoid
> > > > > casts ?
> > > >
> > > > I thought about that before. But grepping through the source, I don't
> > > > really see that. All controls are in float (which is fine). For
> > > > calculations I tend to go for double. These might work in float also,
> > > > but I don't think it's worth forcing the whole project.
> > > >
> > > > >
> > > > > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > > > > ---
> > > > > >  include/libcamera/internal/matrix.h | 11 +++++++++++
> > > > > >  src/libcamera/matrix.cpp            | 10 ++++++++++
> > > > > >  2 files changed, 21 insertions(+)
> > > > > >
> > > > > > diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h
> > > > > > index a055e6926c94..ca81dcda93c4 100644
> > > > > > --- a/include/libcamera/internal/matrix.h
> > > > > > +++ b/include/libcamera/internal/matrix.h
> > > > > > @@ -90,6 +90,17 @@ public:
> > > > > >  		return *this;
> > > > > >  	}
> > > > > >
> > > > > > +	template<typename T2>
> > > > > > +	Matrix<T2, Rows, Cols> cast() const
> > > > >
> > > > > I wonder if we should use a "copy" constructor instead of a cast()
> > > > > function, to make it clear a copy iw made.
> > > >
> > > > I tried that before. I guess it's a matter of taste and in the end I'm
> > > > fine with either way. The cast function was just easier to type and you
> > > > don't have to repeat the dimensions.
> > > >
> > > > e.g.:
> > > >
> > > > v = Matrix<double, 3, 3>(m) * a;
> > > >
> > > > vs
> > > >
> > > > v = m.cast<double>() * a;
> > > >
> > > > >
> > > > > > +	{
> > > > > > +		Matrix<T2, Rows, Cols> ret;
> > > > > > +		for (unsigned int i = 0; i < Rows; i++)
> > > > > > +			for (unsigned int j = 0; j < Cols; j++)
> > > > > > +				ret[i][j] = static_cast<T2>((*this)[i][j]);
> > > > >
> > > > > You can operator on data_ directly, with a single loop.
> > > >
> > > > In the "copy" constructor case, yes. Not in this case because ret.data_ is
> > > > private from within this class.
> > >
> > > But this is a non-static member function, so it has access to all members of the class.
> > > The visibility rules apply to the type, not to the instance, e.g. even a static
> > > member function `T::f()` can access arbitrary members of an object of type T.
> >
> > Yes, but in this case T::cast() would try to access T2::data_. The class
> > types are different and so the visibility rule applies. I tried with a friend
> > declaration but got nowhere (I'm happy to learn how that would look
> > like) and settled for the 2D loop to get it done :-)

Ahh, sorry, you're right, the types are indeed different... should've checked twice.
A friend declaration could look like this:

  template<typename, unsigned int, unsigned int> friend class Matrix;

I'm not sure if it is possible to make it more restrictive (i.e. require same column/row count).


> >
> > Best regards,
> > Stefan
> 
> Maybe we should add a non-const data() function...

Since there is already an operator[] that provides mutable view, I think that makes sense.


Regards,
Barnabás Pőcze


> 
> >
> > >
> > >
> > > Regards,
> > > Barnabás Pőcze
> > >
> > >
> > > >
> > > > Best regards,
> > > > Stefan
> > > >
> > > > >
> > > > > > +
> > > > > > +		return ret;
> > > > > > +	}
> > > > > > +
> > > > > >  private:
> > > > > >  	std::array<T, Rows * Cols> data_;
> > > > > >  };
> > > > > > diff --git a/src/libcamera/matrix.cpp b/src/libcamera/matrix.cpp
> > > > > > index e7e027225666..91a3f16405a3 100644
> > > > > > --- a/src/libcamera/matrix.cpp
> > > > > > +++ b/src/libcamera/matrix.cpp
> > > > > > @@ -77,6 +77,16 @@ LOG_DEFINE_CATEGORY(Matrix)
> > > > > >   * \return Row \a i from the matrix, as a Span
> > > > > >   */
> > > > > >
> > > > > > +/**
> > > > > > + * \fn template<typename T2> Matrix<T2, Rows, Cols> Matrix::cast() const
> > > > > > + * \brief Cast the matrix to a different type
> > > > > > + *
> > > > > > + * This function returns a new matrix with the same size and values but a
> > > > > > + * different type.
> > > > > > + *
> > > > > > + * \return The new matrix
> > > > > > + */
> > > > > > +
> > > > > >  /**
> > > > > >   * \fn Matrix::operator[](size_t i)
> > > > > >   * \copydoc Matrix::operator[](size_t i) const
> > > > >
> > > > > --
> > > > > Regards,
> > > > >
> > > > > Laurent Pinchart