| Message ID | 20240805120522.1613342-3-stefan.klug@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Stefan Klug (2024-08-05 13:05:03) > For the implementation of a manual colour temperature setting, it is > necessary to read predefined colour gains per colour temperature from > the tuning file. Implement this in a backwards compatible way. If no > gains are contained in the tuning file, loading just continues as > before. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/awb.cpp | 18 ++++++++++++++++++ > src/ipa/rkisp1/algorithms/awb.h | 6 ++++++ > 2 files changed, 24 insertions(+) > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > index 4ccafd48dedd..957d24fe3425 100644 > --- a/src/ipa/rkisp1/algorithms/awb.cpp > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > @@ -39,6 +39,24 @@ Awb::Awb() > { > } > > +/** > + * \copydoc libcamera::ipa::Algorithm::init > + */ > +int Awb::init(IPAContext &context, const YamlObject &tuningData) > +{ > + MatrixInterpolator<double, 2, 1> gains; > + int ret = gains.readYaml(tuningData["gains"], "ct", "gains"); Do we have a way to check if there are gains set at all? If there are no gains set in the yaml - we wouldn't report a warning at all I'd expect? (And presumably we wouldn't expose/report a manual color temperature control as setable either?) > + if (ret < 0) > + LOG(RkISP1Awb, Warning) > + << "Failed to parse 'gains' " > + << "parameter from tuning file; " > + << "rgb gains will not be set based on colour temperature"; > + else > + gains_ = gains; > + > + return 0; > +} > + > /** > * \copydoc libcamera::ipa::Algorithm::configure > */ > diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h > index 06c92896e2dc..a010e6d1cb3c 100644 > --- a/src/ipa/rkisp1/algorithms/awb.h > +++ b/src/ipa/rkisp1/algorithms/awb.h > @@ -7,6 +7,10 @@ > > #pragma once > > +#include <optional> > + > +#include "libipa/matrix_interpolator.h" > + > #include "algorithm.h" > > namespace libcamera { > @@ -19,6 +23,7 @@ public: > Awb(); > ~Awb() = default; > > + int init(IPAContext &context, const YamlObject &tuningData) override; > int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override; > void queueRequest(IPAContext &context, const uint32_t frame, > IPAFrameContext &frameContext, > @@ -34,6 +39,7 @@ public: > private: > uint32_t estimateCCT(double red, double green, double blue); > > + std::optional<MatrixInterpolator<double, 2, 1>> gains_; > bool rgbMode_; > }; > > -- > 2.43.0 >
On Mon, Aug 05, 2024 at 02:43:39PM +0100, Kieran Bingham wrote: > Quoting Stefan Klug (2024-08-05 13:05:03) > > For the implementation of a manual colour temperature setting, it is > > necessary to read predefined colour gains per colour temperature from > > the tuning file. Implement this in a backwards compatible way. If no > > gains are contained in the tuning file, loading just continues as > > before. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > src/ipa/rkisp1/algorithms/awb.cpp | 18 ++++++++++++++++++ > > src/ipa/rkisp1/algorithms/awb.h | 6 ++++++ > > 2 files changed, 24 insertions(+) > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > index 4ccafd48dedd..957d24fe3425 100644 > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > @@ -39,6 +39,24 @@ Awb::Awb() > > { > > } > > > > +/** > > + * \copydoc libcamera::ipa::Algorithm::init > > + */ > > +int Awb::init(IPAContext &context, const YamlObject &tuningData) > > +{ > > + MatrixInterpolator<double, 2, 1> gains; > > + int ret = gains.readYaml(tuningData["gains"], "ct", "gains"); > > Do we have a way to check if there are gains set at all? If there are no > gains set in the yaml - we wouldn't report a warning at all I'd expect? If there are no gains in the yaml, readYaml returns an error. We could add a check for that specific case and silence the warning. But do we really want to silence that? It basically says "You have an old tuning file, some features are limited but I'll continue". So I think it's reasonable - up to you. > > (And presumably we wouldn't expose/report a manual color temperature > control as setable either?) That's a bit difficult here as the temperature also controls the ccm. So we would need to check the existence of ccms also. As the ccms are necessary for a properly tuned sensor, I think it is ok to unconditionally add the control. > > > + if (ret < 0) > > + LOG(RkISP1Awb, Warning) > > + << "Failed to parse 'gains' " > > + << "parameter from tuning file; " > > + << "rgb gains will not be set based on colour temperature"; > > + else > > + gains_ = gains; > > + > > + return 0; > > +} > > + > > /** > > * \copydoc libcamera::ipa::Algorithm::configure > > */ > > diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h > > index 06c92896e2dc..a010e6d1cb3c 100644 > > --- a/src/ipa/rkisp1/algorithms/awb.h > > +++ b/src/ipa/rkisp1/algorithms/awb.h > > @@ -7,6 +7,10 @@ > > > > #pragma once > > > > +#include <optional> > > + > > +#include "libipa/matrix_interpolator.h" > > + > > #include "algorithm.h" > > > > namespace libcamera { > > @@ -19,6 +23,7 @@ public: > > Awb(); > > ~Awb() = default; > > > > + int init(IPAContext &context, const YamlObject &tuningData) override; > > int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override; > > void queueRequest(IPAContext &context, const uint32_t frame, > > IPAFrameContext &frameContext, > > @@ -34,6 +39,7 @@ public: > > private: > > uint32_t estimateCCT(double red, double green, double blue); > > > > + std::optional<MatrixInterpolator<double, 2, 1>> gains_; > > bool rgbMode_; > > }; > > > > -- > > 2.43.0 > >
Quoting Stefan Klug (2024-08-06 07:26:59) > On Mon, Aug 05, 2024 at 02:43:39PM +0100, Kieran Bingham wrote: > > Quoting Stefan Klug (2024-08-05 13:05:03) > > > For the implementation of a manual colour temperature setting, it is > > > necessary to read predefined colour gains per colour temperature from > > > the tuning file. Implement this in a backwards compatible way. If no > > > gains are contained in the tuning file, loading just continues as > > > before. > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > --- > > > src/ipa/rkisp1/algorithms/awb.cpp | 18 ++++++++++++++++++ > > > src/ipa/rkisp1/algorithms/awb.h | 6 ++++++ > > > 2 files changed, 24 insertions(+) > > > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > > index 4ccafd48dedd..957d24fe3425 100644 > > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > > @@ -39,6 +39,24 @@ Awb::Awb() > > > { > > > } > > > > > > +/** > > > + * \copydoc libcamera::ipa::Algorithm::init > > > + */ > > > +int Awb::init(IPAContext &context, const YamlObject &tuningData) > > > +{ > > > + MatrixInterpolator<double, 2, 1> gains; > > > + int ret = gains.readYaml(tuningData["gains"], "ct", "gains"); > > > > Do we have a way to check if there are gains set at all? If there are no > > gains set in the yaml - we wouldn't report a warning at all I'd expect? > > If there are no gains in the yaml, readYaml returns an error. We could > add a check for that specific case and silence the warning. But do we > really want to silence that? It basically says "You have an old tuning > file, some features are limited but I'll continue". So I think it's > reasonable - up to you. I think if we can get the schema validation then we can add this as an expected/required tuning entry then and maybe that also solves the issue as then the warning really would be a warning, so I'm fine with leaving this as it is given we're aiming in that direction. > > > > > (And presumably we wouldn't expose/report a manual color temperature > > control as setable either?) > > That's a bit difficult here as the temperature also controls the ccm. > So we would need to check the existence of ccms also. As the ccms are > necessary for a properly tuned sensor, I think it is ok to > unconditionally add the control. Ok so we have a dependency on both algo's for the suitable / acceptable parameters for the control? That makes me think back to how we determine what the min/max values can be - but that's in a later patch I think ;-) > > > > > > + if (ret < 0) > > > + LOG(RkISP1Awb, Warning) > > > + << "Failed to parse 'gains' " > > > + << "parameter from tuning file; " > > > + << "rgb gains will not be set based on colour temperature"; > > > + else > > > + gains_ = gains; > > > + > > > + return 0; > > > +} > > > + > > > /** > > > * \copydoc libcamera::ipa::Algorithm::configure > > > */ > > > diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h > > > index 06c92896e2dc..a010e6d1cb3c 100644 > > > --- a/src/ipa/rkisp1/algorithms/awb.h > > > +++ b/src/ipa/rkisp1/algorithms/awb.h > > > @@ -7,6 +7,10 @@ > > > > > > #pragma once > > > > > > +#include <optional> > > > + > > > +#include "libipa/matrix_interpolator.h" > > > + > > > #include "algorithm.h" > > > > > > namespace libcamera { > > > @@ -19,6 +23,7 @@ public: > > > Awb(); > > > ~Awb() = default; > > > > > > + int init(IPAContext &context, const YamlObject &tuningData) override; > > > int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override; > > > void queueRequest(IPAContext &context, const uint32_t frame, > > > IPAFrameContext &frameContext, > > > @@ -34,6 +39,7 @@ public: > > > private: > > > uint32_t estimateCCT(double red, double green, double blue); > > > > > > + std::optional<MatrixInterpolator<double, 2, 1>> gains_; > > > bool rgbMode_; > > > }; > > > > > > -- > > > 2.43.0 > > >
Quoting Kieran Bingham (2024-08-06 09:52:25) > Quoting Stefan Klug (2024-08-06 07:26:59) > > On Mon, Aug 05, 2024 at 02:43:39PM +0100, Kieran Bingham wrote: > > > Quoting Stefan Klug (2024-08-05 13:05:03) > > > > For the implementation of a manual colour temperature setting, it is > > > > necessary to read predefined colour gains per colour temperature from > > > > the tuning file. Implement this in a backwards compatible way. If no > > > > gains are contained in the tuning file, loading just continues as > > > > before. > > > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > --- > > > > src/ipa/rkisp1/algorithms/awb.cpp | 18 ++++++++++++++++++ > > > > src/ipa/rkisp1/algorithms/awb.h | 6 ++++++ > > > > 2 files changed, 24 insertions(+) > > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > > > index 4ccafd48dedd..957d24fe3425 100644 > > > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > > > @@ -39,6 +39,24 @@ Awb::Awb() > > > > { > > > > } > > > > > > > > +/** > > > > + * \copydoc libcamera::ipa::Algorithm::init > > > > + */ > > > > +int Awb::init(IPAContext &context, const YamlObject &tuningData) > > > > +{ > > > > + MatrixInterpolator<double, 2, 1> gains; > > > > + int ret = gains.readYaml(tuningData["gains"], "ct", "gains"); > > > > > > Do we have a way to check if there are gains set at all? If there are no > > > gains set in the yaml - we wouldn't report a warning at all I'd expect? > > > > If there are no gains in the yaml, readYaml returns an error. We could > > add a check for that specific case and silence the warning. But do we > > really want to silence that? It basically says "You have an old tuning > > file, some features are limited but I'll continue". So I think it's > > reasonable - up to you. > > I think if we can get the schema validation then we can add this as an > expected/required tuning entry then and maybe that also solves the issue > as then the warning really would be a warning, so I'm fine with leaving > this as it is given we're aiming in that direction. > > > > > > > > > (And presumably we wouldn't expose/report a manual color temperature > > > control as setable either?) > > > > That's a bit difficult here as the temperature also controls the ccm. > > So we would need to check the existence of ccms also. As the ccms are > > necessary for a properly tuned sensor, I think it is ok to > > unconditionally add the control. > > Ok so we have a dependency on both algo's for the suitable / acceptable > parameters for the control? That makes me think back to how we determine > what the min/max values can be - but that's in a later patch I think ;-) Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > > + if (ret < 0) > > > > + LOG(RkISP1Awb, Warning) > > > > + << "Failed to parse 'gains' " > > > > + << "parameter from tuning file; " > > > > + << "rgb gains will not be set based on colour temperature"; > > > > + else > > > > + gains_ = gains; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > /** > > > > * \copydoc libcamera::ipa::Algorithm::configure > > > > */ > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h > > > > index 06c92896e2dc..a010e6d1cb3c 100644 > > > > --- a/src/ipa/rkisp1/algorithms/awb.h > > > > +++ b/src/ipa/rkisp1/algorithms/awb.h > > > > @@ -7,6 +7,10 @@ > > > > > > > > #pragma once > > > > > > > > +#include <optional> > > > > + > > > > +#include "libipa/matrix_interpolator.h" > > > > + > > > > #include "algorithm.h" > > > > > > > > namespace libcamera { > > > > @@ -19,6 +23,7 @@ public: > > > > Awb(); > > > > ~Awb() = default; > > > > > > > > + int init(IPAContext &context, const YamlObject &tuningData) override; > > > > int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override; > > > > void queueRequest(IPAContext &context, const uint32_t frame, > > > > IPAFrameContext &frameContext, > > > > @@ -34,6 +39,7 @@ public: > > > > private: > > > > uint32_t estimateCCT(double red, double green, double blue); > > > > > > > > + std::optional<MatrixInterpolator<double, 2, 1>> gains_; > > > > bool rgbMode_; > > > > }; > > > > > > > > -- > > > > 2.43.0 > > > >
diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp index 4ccafd48dedd..957d24fe3425 100644 --- a/src/ipa/rkisp1/algorithms/awb.cpp +++ b/src/ipa/rkisp1/algorithms/awb.cpp @@ -39,6 +39,24 @@ Awb::Awb() { } +/** + * \copydoc libcamera::ipa::Algorithm::init + */ +int Awb::init(IPAContext &context, const YamlObject &tuningData) +{ + MatrixInterpolator<double, 2, 1> gains; + int ret = gains.readYaml(tuningData["gains"], "ct", "gains"); + if (ret < 0) + LOG(RkISP1Awb, Warning) + << "Failed to parse 'gains' " + << "parameter from tuning file; " + << "rgb gains will not be set based on colour temperature"; + else + gains_ = gains; + + return 0; +} + /** * \copydoc libcamera::ipa::Algorithm::configure */ diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h index 06c92896e2dc..a010e6d1cb3c 100644 --- a/src/ipa/rkisp1/algorithms/awb.h +++ b/src/ipa/rkisp1/algorithms/awb.h @@ -7,6 +7,10 @@ #pragma once +#include <optional> + +#include "libipa/matrix_interpolator.h" + #include "algorithm.h" namespace libcamera { @@ -19,6 +23,7 @@ public: Awb(); ~Awb() = default; + int init(IPAContext &context, const YamlObject &tuningData) override; int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override; void queueRequest(IPAContext &context, const uint32_t frame, IPAFrameContext &frameContext, @@ -34,6 +39,7 @@ public: private: uint32_t estimateCCT(double red, double green, double blue); + std::optional<MatrixInterpolator<double, 2, 1>> gains_; bool rgbMode_; };
For the implementation of a manual colour temperature setting, it is necessary to read predefined colour gains per colour temperature from the tuning file. Implement this in a backwards compatible way. If no gains are contained in the tuning file, loading just continues as before. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/ipa/rkisp1/algorithms/awb.cpp | 18 ++++++++++++++++++ src/ipa/rkisp1/algorithms/awb.h | 6 ++++++ 2 files changed, 24 insertions(+)