| Message ID | 20241118150528.1856797-3-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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.. > 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@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 }}; ... } 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. > @@ -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',
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@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
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); + std::copy(data.begin(), data.end(), data_.begin()); + } + Matrix(const std::vector<T> &data) { std::copy(data.begin(), data.end(), data_.begin()); @@ -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 }); + 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" #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',
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. 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@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(-)