[PATCH v2 12/17] libipa: awb: Make result of gainsFromColourTemp optional
Stefan Klug
stefan.klug at ideasonboard.com
Wed Apr 2 17:01:22 CEST 2025
Hi Laurent,
Thank you for the review.
On Tue, Apr 01, 2025 at 04:29:01AM +0300, Laurent Pinchart wrote:
> Hi Stefan,
>
> Thank you for the patch.
>
> On Wed, Mar 19, 2025 at 05:11:17PM +0100, Stefan Klug wrote:
> > In the grey world AWB case, if no colour gains are contained in the
> > tuning file, the color gains get reset to 1 when the colour temperature
> > is set manually. This is unexpected and undesirable. Allow the
> > gainsFromColourTemp() function to return a std::nullopt to handle that
> > case.
> >
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> >
> > ---
> >
> > Changes in v2:
> > - Added this patch
> > ---
> > src/ipa/libipa/awb.cpp | 2 +-
> > src/ipa/libipa/awb.h | 2 +-
> > src/ipa/libipa/awb_bayes.cpp | 4 ++--
> > src/ipa/libipa/awb_bayes.h | 2 +-
> > src/ipa/libipa/awb_grey.cpp | 6 +++---
> > src/ipa/libipa/awb_grey.h | 2 +-
> > src/ipa/rkisp1/algorithms/awb.cpp | 16 +++++++++++-----
> > 7 files changed, 20 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/ipa/libipa/awb.cpp b/src/ipa/libipa/awb.cpp
> > index 925fac232709..214bac8b56a6 100644
> > --- a/src/ipa/libipa/awb.cpp
> > +++ b/src/ipa/libipa/awb.cpp
> > @@ -114,7 +114,7 @@ namespace ipa {
> > * does not take any statistics into account. It is used to compute the colour
> > * gains when the user manually specifies a colour temperature.
> > *
> > - * \return The colour gains
> > + * \return The colour gains or std::nullopt if the conversion is not possible
> > */
> >
> > /**
> > diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h
> > index 4bab7a451ce5..789a1d534f9a 100644
> > --- a/src/ipa/libipa/awb.h
> > +++ b/src/ipa/libipa/awb.h
> > @@ -39,7 +39,7 @@ public:
> >
> > virtual int init(const YamlObject &tuningData) = 0;
> > virtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0;
> > - virtual RGB<double> gainsFromColourTemperature(double colourTemperature) = 0;
> > + virtual std::optional<RGB<double>> gainsFromColourTemperature(double colourTemperature) = 0;
>
> You need to include <optional>
Right, and remove it from rkisp1/awb.h.
>
> >
> > const ControlInfoMap::Map &controls() const
> > {
> > diff --git a/src/ipa/libipa/awb_bayes.cpp b/src/ipa/libipa/awb_bayes.cpp
> > index d1d0eaf0e68f..d2bcbd83d7f8 100644
> > --- a/src/ipa/libipa/awb_bayes.cpp
> > +++ b/src/ipa/libipa/awb_bayes.cpp
> > @@ -270,7 +270,7 @@ void AwbBayes::handleControls(const ControlList &controls)
> > }
> > }
> >
> > -RGB<double> AwbBayes::gainsFromColourTemperature(double colourTemperature)
> > +std::optional<RGB<double>> AwbBayes::gainsFromColourTemperature(double colourTemperature)
> > {
> > /*
> > * \todo In the RaspberryPi code, the ct curve was interpolated in
> > @@ -278,7 +278,7 @@ RGB<double> AwbBayes::gainsFromColourTemperature(double colourTemperature)
> > * intuitive, as the gains are in linear space. But I can't prove it.
> > */
> > const auto &gains = colourGainCurve_.getInterpolated(colourTemperature);
> > - return { { gains[0], 1.0, gains[1] } };
> > + return RGB<double>{ { gains[0], 1.0, gains[1] } };
> > }
> >
> > AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux)
> > diff --git a/src/ipa/libipa/awb_bayes.h b/src/ipa/libipa/awb_bayes.h
> > index bb933038d6d2..47ef3cce4d58 100644
> > --- a/src/ipa/libipa/awb_bayes.h
> > +++ b/src/ipa/libipa/awb_bayes.h
> > @@ -27,7 +27,7 @@ public:
> >
> > int init(const YamlObject &tuningData) override;
> > AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;
> > - RGB<double> gainsFromColourTemperature(double temperatureK) override;
> > + std::optional<RGB<double>> gainsFromColourTemperature(double temperatureK) override;
> > void handleControls(const ControlList &controls) override;
> >
> > private:
> > diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp
> > index d3d2132b8869..d252edb2b6c6 100644
> > --- a/src/ipa/libipa/awb_grey.cpp
> > +++ b/src/ipa/libipa/awb_grey.cpp
> > @@ -98,15 +98,15 @@ AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned
> > * \return The colour gains if a colour temperature curve is available,
> > * [1, 1, 1] otherwise.
> > */
> > -RGB<double> AwbGrey::gainsFromColourTemperature(double colourTemperature)
> > +std::optional<RGB<double>> AwbGrey::gainsFromColourTemperature(double colourTemperature)
> > {
> > if (!colourGainCurve_) {
> > LOG(Awb, Error) << "No gains defined";
> > - return RGB<double>({ 1.0, 1.0, 1.0 });
> > + return std::nullopt;
> > }
> >
> > auto gains = colourGainCurve_->getInterpolated(colourTemperature);
> > - return { { gains[0], 1.0, gains[1] } };
> > + return RGB<double>{ { gains[0], 1.0, gains[1] } };
> > }
> >
> > } /* namespace ipa */
> > diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h
> > index 7ec7bfa5da9a..f82a368d11cc 100644
> > --- a/src/ipa/libipa/awb_grey.h
> > +++ b/src/ipa/libipa/awb_grey.h
> > @@ -25,7 +25,7 @@ public:
> >
> > int init(const YamlObject &tuningData) override;
> > AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override;
> > - RGB<double> gainsFromColourTemperature(double colourTemperature) override;
> > + std::optional<RGB<double>> gainsFromColourTemperature(double colourTemperature) override;
> >
> > private:
> > std::optional<Interpolator<Vector<double, 2>>> colourGainCurve_;
> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > index 5e067e50cd52..58b8370d2c61 100644
> > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > @@ -125,8 +125,12 @@ int Awb::configure(IPAContext &context,
> > const IPACameraSensorInfo &configInfo)
> > {
> > context.activeState.awb.manual.gains = RGB<double>{ 1.0 };
> > - context.activeState.awb.automatic.gains =
> > - awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);
> > + auto gains = awbAlgo_->gainsFromColourTemperature(kDefaultColourTemperature);
> > + if (gains)
> > + context.activeState.awb.automatic.gains = *gains;
> > + else
> > + context.activeState.awb.automatic.gains = RGB<double>{ 1.0 };
> > +
> > context.activeState.awb.autoEnabled = true;
> > context.activeState.awb.manual.temperatureK = kDefaultColourTemperature;
> > context.activeState.awb.automatic.temperatureK = kDefaultColourTemperature;
> > @@ -184,10 +188,12 @@ void Awb::queueRequest(IPAContext &context,
> > update = true;
> > } else if (colourTemperature) {
> > const auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);
> > - awb.manual.gains.r() = gains.r();
> > - awb.manual.gains.b() = gains.b();
> > + if (gains) {
> > + awb.manual.gains.r() = gains->r();
> > + awb.manual.gains.b() = gains->b();
> > + update = true;
> > + }
> > awb.manual.temperatureK = *colourTemperature;
>
> Shouldn't we skip the colour temperature update too ?
I don't think so, as the temperature is still reported in metadata. And
it arrived correctly in libcamera, only awb can't create gains out of
it. Oh and ccm might still use it. So it makes sense to report it in
metadata.
Best regards,
Stefan
>
> > - update = true;
> > }
> >
> > if (update)
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list