[PATCH 2/4] libcamera: Enable and use matrix implementation from libcamera/internal
Stefan Klug
stefan.klug at ideasonboard.com
Mon Nov 18 17:52:41 CET 2024
Hi Laurent,
Thank you for the review.
On Mon, Nov 18, 2024 at 05:59:13PM +0200, Laurent Pinchart wrote:
> Hi Stefan,
>
> Thank you for the patch.
>
> On Mon, Nov 18, 2024 at 04:05:05PM +0100, Stefan Klug wrote:
> > The matrix code copied from libipa needs to be adapted to compile and
> > work from the new location. To keep the project buildable at all times,
> > these changes are not split into multiple commits but kept as a single
> > one.
>
> If you first rename the Matrix class in the RPi IPA to Matrix3x3 then
> you will be able to split this patch better, with the rework of the
> Matrix class (and wiring up to the build system) in separate patches..
Is that really worth the effort? But yes, I can do it.
>
> > The changes are:
> > - Add matrix class to meson.build
> > - Move matrix class from libipa namespace into libcamera
> > - Add std::initializer_list based constructor to be able to replace the
> > Matrix implementation contained in the rpi pipeline
> > - Replace Matrix class in rpi pipeline with the new one to prevent name
> > clashes
> >
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > ---
> > include/libcamera/internal/matrix.h | 20 ++++-----
> > include/libcamera/internal/meson.build | 1 +
> > src/ipa/rpi/controller/rpi/ccm.cpp | 56 +++++++-------------------
> > src/ipa/rpi/controller/rpi/ccm.h | 35 +---------------
> > src/libcamera/matrix.cpp | 8 +---
> > src/libcamera/meson.build | 1 +
> > 6 files changed, 32 insertions(+), 89 deletions(-)
> >
> > diff --git a/include/libcamera/internal/matrix.h b/include/libcamera/internal/matrix.h
> > index 5471e6975b74..b82d33f98658 100644
> > --- a/include/libcamera/internal/matrix.h
> > +++ b/include/libcamera/internal/matrix.h
> > @@ -19,8 +19,6 @@ namespace libcamera {
> >
> > LOG_DECLARE_CATEGORY(Matrix)
> >
> > -namespace ipa {
> > -
> > #ifndef __DOXYGEN__
> > template<typename T, unsigned int Rows, unsigned int Cols,
> > std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> > @@ -35,6 +33,12 @@ public:
> > data_.fill(static_cast<T>(0));
> > }
> >
> > + Matrix(std::initializer_list<T> data)
> > + {
> > + ASSERT(data.size() == Rows * Cols);
>
> I considered adding a similar constructor to Vector in my latest series,
> but it's problematic for two reasons. One is that the ASSERT() will only
> trigger at runtime, while this should be a compile-time check. We
> coulduse static_assert() instead (and mark the constructor as
> constexpr):
>
> constexpr Matrix(std::initializer_list<T> data)
> {
> static_assert(data.size() == Rows * Cols);
> std::copy(data.begin(), data.end(), data_.begin());
> }
>
> But this will only work if the initializer list is a compile-time
> constant. The following will compile fine:
>
> void foo()
> {
> Matrix<double, 2, 2> m{ 1.0, 2.0, 3.0, 4.0 };
> ...
> }
>
> But this won't:
>
> void foo(double v)
> {
> Matrix<double, 2, 2> m{ v, v, v, v };
> ...
> }
>
> as the initializer list isn't a compile-time constant.
>
> This is why I didn't add a constructor taking an initializer_list, but
> instead rely on the constructor taking an std::array. For the Matrix
> class, this would be
>
> constexpr Matrix(const std::array<T, Rows, Cols> &data)
> {
> std::copy(data.begin(), data.end(), data_.begin());
> }
>
> The user needs to be slightly adapted (note the double curly braces):
>
> void foo(double v)
> {
> Matrix<double, 2, 2> m{{ v, v, v, v }};
I had exactly the same process (static_assert etc.). Just my conclusion
was a bit different. I would rate the syntax (the double braces are
unexpected and require too much knowledge on the internals) over the
missing compile time check (as the assert is on a pretty likely path and
will happen during testing). So I chose the initializer_list. Now I
tried the array and funny enough, there is no need to switch to double
braces - even with our old compilers. They manage to fold it.
> ...
> }
>
> The addition of the new constructor could go to a separate patch, before
> this one.
>
> > + std::copy(data.begin(), data.end(), data_.begin());
> > + }
> > +
> > Matrix(const std::vector<T> &data)
> > {
> > std::copy(data.begin(), data.end(), data_.begin());
>
> This seems very unsafe :-S Let's see if the constructor taking an
> std::array could replace this one.
I already read your comment on the other patch, so I'll remove it.
Cheers,
Stefan
>
> > @@ -166,24 +170,22 @@ Matrix<T, Rows, Cols> operator+(const Matrix<T, Rows, Cols> &m1, const Matrix<T,
> > bool matrixValidateYaml(const YamlObject &obj, unsigned int size);
> > #endif /* __DOXYGEN__ */
> >
> > -} /* namespace ipa */
> > -
> > #ifndef __DOXYGEN__
> > template<typename T, unsigned int Rows, unsigned int Cols>
> > -std::ostream &operator<<(std::ostream &out, const ipa::Matrix<T, Rows, Cols> &m)
> > +std::ostream &operator<<(std::ostream &out, const Matrix<T, Rows, Cols> &m)
> > {
> > out << m.toString();
> > return out;
> > }
> >
> > template<typename T, unsigned int Rows, unsigned int Cols>
> > -struct YamlObject::Getter<ipa::Matrix<T, Rows, Cols>> {
> > - std::optional<ipa::Matrix<T, Rows, Cols>> get(const YamlObject &obj) const
> > +struct YamlObject::Getter<Matrix<T, Rows, Cols>> {
> > + std::optional<Matrix<T, Rows, Cols>> get(const YamlObject &obj) const
> > {
> > - if (!ipa::matrixValidateYaml(obj, Rows * Cols))
> > + if (!matrixValidateYaml(obj, Rows * Cols))
> > return std::nullopt;
> >
> > - ipa::Matrix<T, Rows, Cols> matrix;
> > + Matrix<T, Rows, Cols> matrix;
> > T *data = &matrix[0][0];
> >
> > unsigned int i = 0;
> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > index 1dddcd50c90b..7d6aa8b72bd7 100644
> > --- a/include/libcamera/internal/meson.build
> > +++ b/include/libcamera/internal/meson.build
> > @@ -29,6 +29,7 @@ libcamera_internal_headers = files([
> > 'ipc_pipe.h',
> > 'ipc_unixsocket.h',
> > 'mapped_framebuffer.h',
> > + 'matrix.h',
> > 'media_device.h',
> > 'media_object.h',
> > 'pipeline_handler.h',
> > diff --git a/src/ipa/rpi/controller/rpi/ccm.cpp b/src/ipa/rpi/controller/rpi/ccm.cpp
> > index aefa580c9a4b..418bead56ecd 100644
> > --- a/src/ipa/rpi/controller/rpi/ccm.cpp
> > +++ b/src/ipa/rpi/controller/rpi/ccm.cpp
> > @@ -29,34 +29,7 @@ LOG_DEFINE_CATEGORY(RPiCcm)
> >
> > #define NAME "rpi.ccm"
> >
> > -Matrix::Matrix()
> > -{
> > - memset(m, 0, sizeof(m));
> > -}
> > -Matrix::Matrix(double m0, double m1, double m2, double m3, double m4, double m5,
> > - double m6, double m7, double m8)
> > -{
> > - m[0][0] = m0, m[0][1] = m1, m[0][2] = m2, m[1][0] = m3, m[1][1] = m4,
> > - m[1][2] = m5, m[2][0] = m6, m[2][1] = m7, m[2][2] = m8;
> > -}
> > -int Matrix::read(const libcamera::YamlObject ¶ms)
> > -{
> > - double *ptr = (double *)m;
> > -
> > - if (params.size() != 9) {
> > - LOG(RPiCcm, Error) << "Wrong number of values in CCM";
> > - return -EINVAL;
> > - }
> > -
> > - for (const auto ¶m : params.asList()) {
> > - auto value = param.get<double>();
> > - if (!value)
> > - return -EINVAL;
> > - *ptr++ = *value;
> > - }
> > -
> > - return 0;
> > -}
> > +using Matrix3x3 = Matrix<double, 3, 3>;
> >
> > Ccm::Ccm(Controller *controller)
> > : CcmAlgorithm(controller), saturation_(1.0) {}
> > @@ -68,8 +41,6 @@ char const *Ccm::name() const
> >
> > int Ccm::read(const libcamera::YamlObject ¶ms)
> > {
> > - int ret;
> > -
> > if (params.contains("saturation")) {
> > config_.saturation = params["saturation"].get<ipa::Pwl>(ipa::Pwl{});
> > if (config_.saturation.empty())
> > @@ -83,9 +54,12 @@ int Ccm::read(const libcamera::YamlObject ¶ms)
> >
> > CtCcm ctCcm;
> > ctCcm.ct = *value;
> > - ret = ctCcm.ccm.read(p["ccm"]);
> > - if (ret)
> > - return ret;
> > +
> > + auto ccm = p["ccm"].get<Matrix3x3>();
> > + if (!ccm)
> > + return -EINVAL;
> > +
> > + ctCcm.ccm = *ccm;
> >
> > if (!config_.ccms.empty() && ctCcm.ct <= config_.ccms.back().ct) {
> > LOG(RPiCcm, Error)
> > @@ -125,7 +99,7 @@ bool getLocked(Metadata *metadata, std::string const &tag, T &value)
> > return true;
> > }
> >
> > -Matrix calculateCcm(std::vector<CtCcm> const &ccms, double ct)
> > +Matrix3x3 calculateCcm(std::vector<CtCcm> const &ccms, double ct)
> > {
> > if (ct <= ccms.front().ct)
> > return ccms.front().ccm;
> > @@ -141,13 +115,13 @@ Matrix calculateCcm(std::vector<CtCcm> const &ccms, double ct)
> > }
> > }
> >
> > -Matrix applySaturation(Matrix const &ccm, double saturation)
> > +Matrix3x3 applySaturation(Matrix3x3 const &ccm, double saturation)
> > {
> > - Matrix RGB2Y(0.299, 0.587, 0.114, -0.169, -0.331, 0.500, 0.500, -0.419,
> > - -0.081);
> > - Matrix Y2RGB(1.000, 0.000, 1.402, 1.000, -0.345, -0.714, 1.000, 1.771,
> > - 0.000);
> > - Matrix S(1, 0, 0, 0, saturation, 0, 0, 0, saturation);
> > + Matrix3x3 RGB2Y({ 0.299, 0.587, 0.114, -0.169, -0.331, 0.500, 0.500,
> > + -0.419, -0.081 });
> > + Matrix3x3 Y2RGB({ 1.000, 0.000, 1.402, 1.000, -0.345, -0.714, 1.000,
> > + 1.771, 0.000 });
>
> While at it, these two should be static const.
>
> > + Matrix3x3 S({ 1, 0, 0, 0, saturation, 0, 0, 0, saturation });
> > return Y2RGB * S * RGB2Y * ccm;
> > }
> >
> > @@ -181,7 +155,7 @@ void Ccm::prepare(Metadata *imageMetadata)
> > for (int j = 0; j < 3; j++)
> > for (int i = 0; i < 3; i++)
> > ccmStatus.matrix[j * 3 + i] =
> > - std::max(-8.0, std::min(7.9999, ccm.m[j][i]));
> > + std::max(-8.0, std::min(7.9999, ccm[j][i]));
> > LOG(RPiCcm, Debug)
> > << "colour temperature " << awb.temperatureK << "K";
> > LOG(RPiCcm, Debug)
> > diff --git a/src/ipa/rpi/controller/rpi/ccm.h b/src/ipa/rpi/controller/rpi/ccm.h
> > index 4e5b33fefa4e..c05dbb17a264 100644
> > --- a/src/ipa/rpi/controller/rpi/ccm.h
> > +++ b/src/ipa/rpi/controller/rpi/ccm.h
> > @@ -8,6 +8,7 @@
> >
> > #include <vector>
> >
> > +#include "libcamera/internal/matrix.h"
>
> Missing blank line.
>
> > #include <libipa/pwl.h>
> >
> > #include "../ccm_algorithm.h"
> > @@ -16,41 +17,9 @@ namespace RPiController {
> >
> > /* Algorithm to calculate colour matrix. Should be placed after AWB. */
> >
> > -struct Matrix {
> > - Matrix(double m0, double m1, double m2, double m3, double m4, double m5,
> > - double m6, double m7, double m8);
> > - Matrix();
> > - double m[3][3];
> > - int read(const libcamera::YamlObject ¶ms);
> > -};
> > -static inline Matrix operator*(double d, Matrix const &m)
> > -{
> > - return Matrix(m.m[0][0] * d, m.m[0][1] * d, m.m[0][2] * d,
> > - m.m[1][0] * d, m.m[1][1] * d, m.m[1][2] * d,
> > - m.m[2][0] * d, m.m[2][1] * d, m.m[2][2] * d);
> > -}
> > -static inline Matrix operator*(Matrix const &m1, Matrix const &m2)
> > -{
> > - Matrix m;
> > - for (int i = 0; i < 3; i++)
> > - for (int j = 0; j < 3; j++)
> > - m.m[i][j] = m1.m[i][0] * m2.m[0][j] +
> > - m1.m[i][1] * m2.m[1][j] +
> > - m1.m[i][2] * m2.m[2][j];
> > - return m;
> > -}
> > -static inline Matrix operator+(Matrix const &m1, Matrix const &m2)
> > -{
> > - Matrix m;
> > - for (int i = 0; i < 3; i++)
> > - for (int j = 0; j < 3; j++)
> > - m.m[i][j] = m1.m[i][j] + m2.m[i][j];
> > - return m;
> > -}
> > -
> > struct CtCcm {
> > double ct;
> > - Matrix ccm;
> > + libcamera::Matrix<double, 3, 3> ccm;
> > };
> >
> > struct CcmConfig {
> > diff --git a/src/libcamera/matrix.cpp b/src/libcamera/matrix.cpp
> > index 8346f0d34160..c3ed54895b22 100644
> > --- a/src/libcamera/matrix.cpp
> > +++ b/src/libcamera/matrix.cpp
> > @@ -5,7 +5,7 @@
> > * Matrix and related operations
> > */
> >
> > -#include "matrix.h"
> > +#include "libcamera/internal/matrix.h"
> >
> > #include <libcamera/base/log.h>
> >
> > @@ -18,8 +18,6 @@ namespace libcamera {
> >
> > LOG_DEFINE_CATEGORY(Matrix)
> >
> > -namespace ipa {
> > -
> > /**
> > * \class Matrix
> > * \brief Matrix class
> > @@ -34,7 +32,7 @@ namespace ipa {
> > */
> >
> > /**
> > - * \fn Matrix::Matrix(const std::vector<T> &data)
> > + * \fn Matrix::Matrix(const Span<T> &data)
> > * \brief Construct a matrix from supplied data
> > * \param[in] data Data from which to construct a matrix
> > *
> > @@ -144,6 +142,4 @@ bool matrixValidateYaml(const YamlObject &obj, unsigned int size)
> > }
> > #endif /* __DOXYGEN__ */
> >
> > -} /* namespace ipa */
> > -
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 21cae11756d6..57fde8a8fab0 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -40,6 +40,7 @@ libcamera_internal_sources = files([
> > 'ipc_pipe_unixsocket.cpp',
> > 'ipc_unixsocket.cpp',
> > 'mapped_framebuffer.cpp',
> > + 'matrix.cpp',
> > 'media_device.cpp',
> > 'media_object.cpp',
> > 'pipeline_handler.cpp',
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list