[v5,3/8] ipa: rkisp1: awb: Implement ColourTemperature control

22220 diff mbox series

There are many use-cases (tuning-validation, working in static
environments) where a manual ColourTemperature control is helpful.
Implement that by interpolating and applying the white balance gains
from the tuning file according to the requested colour temperature. If
colour gains are provided on the same request, they take precedence. As
the colour temperature reported in the metadata is always based on the
measurements, we don't have to touch that.

Note that in the automatic case, the colour gains are still based on the
gray world model and the ones from the tuning file get ignored.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/awb.cpp | 35 ++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

Quoting Stefan Klug (2024-12-06 14:52:23)
> There are many use-cases (tuning-validation, working in static
> environments) where a manual ColourTemperature control is helpful.
> Implement that by interpolating and applying the white balance gains
> from the tuning file according to the requested colour temperature. If
> colour gains are provided on the same request, they take precedence. As
> the colour temperature reported in the metadata is always based on the
> measurements, we don't have to touch that.
> 
> Note that in the automatic case, the colour gains are still based on the
> gray world model and the ones from the tuning file get ignored.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/awb.cpp | 35 ++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> index 23a81e75d3d3..41f260e7089c 100644
> --- a/src/ipa/rkisp1/algorithms/awb.cpp
> +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> @@ -33,6 +33,10 @@ namespace ipa::rkisp1::algorithms {
>  
>  LOG_DEFINE_CATEGORY(RkISP1Awb)
>  
> +constexpr int32_t kMinColourTemperature = 2500;
> +constexpr int32_t kMaxColourTemperature = 10000;
> +constexpr int32_t kDefaultColourTemperature = 6500;
> +
>  /* Minimum mean value below which AWB can't operate. */
>  constexpr double kMeanMinThreshold = 2.0;
>  
> @@ -44,8 +48,13 @@ Awb::Awb()
>  /**
>   * \copydoc libcamera::ipa::Algorithm::init
>   */
> -int Awb::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> +int Awb::init(IPAContext &context, const YamlObject &tuningData)
>  {
> +       auto &cmap = context.ctrlMap;
> +       cmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature,
> +                                                        kMaxColourTemperature,
> +                                                        kDefaultColourTemperature);
> +
>         Interpolator<Vector<double, 2>> gains;
>         int ret = gains.readYaml(tuningData["gains"], "ct", "gains");
>         if (ret < 0)
> @@ -101,19 +110,31 @@ void Awb::queueRequest(IPAContext &context,
>                         << (*awbEnable ? "Enabling" : "Disabling") << " AWB";
>         }
>  
> +       frameContext.awb.autoEnabled = awb.autoEnabled;
> +
>         const auto &colourGains = controls.get(controls::ColourGains);
> -       if (colourGains && !awb.autoEnabled) {
> +       const auto &colourTemperature = controls.get(controls::ColourTemperature);

Why do you get the colourTemperature here

> +
> +       if (awb.autoEnabled)

And then potentially return without using it here ?

> +               return;

Perhaps we should only get it here ?

> +
> +       bool update = false;
> +       if (colourGains) {
>                 awb.gains.manual.r() = (*colourGains)[0];
>                 awb.gains.manual.b() = (*colourGains)[1];
> +               update = true;
> +       } else if (colourTemperature && gains_) {
> +               auto gains = gains_->getInterpolated(*colourTemperature);
> +               awb.gains.manual.r() = gains[0];
> +               awb.gains.manual.b() = gains[1];
> +               update = true;
> +       }

I'm getting more convinced that queueRequest for each algorithm that
uses libipa should be using a common path... This would need to be
implemented in the same way for Mali, IPU3, RKISP1 ... (and Pi
separately)

(I had the same comment earlier on AGC for Mali ... but I don't think it
blocks getting these merged - but I wouldn't want to see all the libipa
implementations having a different response to control handling in the
end).

With the adjustment for when you read the
controls.get(controls::ColourTemperature); to be after the
awb.autoEnabled()


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


>  
> +       if (update)
>                 LOG(RkISP1Awb, Debug)
>                         << "Set colour gains to " << awb.gains.manual;
> -       }
> -
> -       frameContext.awb.autoEnabled = awb.autoEnabled;
>  
> -       if (!awb.autoEnabled)
> -               frameContext.awb.gains = awb.gains.manual;
> +       frameContext.awb.gains = awb.gains.manual;
>  }
>  
>  /**
> -- 
> 2.43.0
>

On Fri, Dec 06, 2024 at 03:10:37PM +0000, Kieran Bingham wrote:
> Quoting Stefan Klug (2024-12-06 14:52:23)
> > There are many use-cases (tuning-validation, working in static
> > environments) where a manual ColourTemperature control is helpful.
> > Implement that by interpolating and applying the white balance gains
> > from the tuning file according to the requested colour temperature. If
> > colour gains are provided on the same request, they take precedence. As
> > the colour temperature reported in the metadata is always based on the
> > measurements, we don't have to touch that.

This last sentence may need to be revisited based on the outcome of the
discussion on the control definition.

> > 
> > Note that in the automatic case, the colour gains are still based on the
> > gray world model and the ones from the tuning file get ignored.

s/the ones/the CT curve/ if you agree with my comment on 2/8.

> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/algorithms/awb.cpp | 35 ++++++++++++++++++++++++-------
> >  1 file changed, 28 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > index 23a81e75d3d3..41f260e7089c 100644
> > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > @@ -33,6 +33,10 @@ namespace ipa::rkisp1::algorithms {
> >  
> >  LOG_DEFINE_CATEGORY(RkISP1Awb)
> >  
> > +constexpr int32_t kMinColourTemperature = 2500;
> > +constexpr int32_t kMaxColourTemperature = 10000;
> > +constexpr int32_t kDefaultColourTemperature = 6500;
> > +
> >  /* Minimum mean value below which AWB can't operate. */
> >  constexpr double kMeanMinThreshold = 2.0;
> >  
> > @@ -44,8 +48,13 @@ Awb::Awb()
> >  /**
> >   * \copydoc libcamera::ipa::Algorithm::init
> >   */
> > -int Awb::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
> > +int Awb::init(IPAContext &context, const YamlObject &tuningData)
> >  {
> > +       auto &cmap = context.ctrlMap;
> > +       cmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature,
> > +                                                        kMaxColourTemperature,
> > +                                                        kDefaultColourTemperature);
> > +
> >         Interpolator<Vector<double, 2>> gains;
> >         int ret = gains.readYaml(tuningData["gains"], "ct", "gains");
> >         if (ret < 0)
> > @@ -101,19 +110,31 @@ void Awb::queueRequest(IPAContext &context,
> >                         << (*awbEnable ? "Enabling" : "Disabling") << " AWB";
> >         }
> >  
> > +       frameContext.awb.autoEnabled = awb.autoEnabled;
> > +
> >         const auto &colourGains = controls.get(controls::ColourGains);
> > -       if (colourGains && !awb.autoEnabled) {
> > +       const auto &colourTemperature = controls.get(controls::ColourTemperature);
> 
> Why do you get the colourTemperature here
> 
> > +
> > +       if (awb.autoEnabled)
> 
> And then potentially return without using it here ?
> 
> > +               return;
> 
> Perhaps we should only get it here ?

Retrieval of colourGains should also be moved here.

> 
> > +
> > +       bool update = false;
> > +       if (colourGains) {
> >                 awb.gains.manual.r() = (*colourGains)[0];
> >                 awb.gains.manual.b() = (*colourGains)[1];
> > +               update = true;
> > +       } else if (colourTemperature && gains_) {

If we have no CT curve, should we expose the ColourTemperature control
as output only ? And now that I'm writing that, I don't think we have a
way to indicate if a control is inout or out only in a dynamic way,
Paul's series only handles the control definition.

Will we need to extend's Paul's series to differentiate "optionally
input" vs. "mandatory input", and also add the direction to ControlInfo
in addition to ControlId ?

> > +               auto gains = gains_->getInterpolated(*colourTemperature);

const auto &gains

> > +               awb.gains.manual.r() = gains[0];
> > +               awb.gains.manual.b() = gains[1];
> > +               update = true;
> > +       }
> 
> I'm getting more convinced that queueRequest for each algorithm that
> uses libipa should be using a common path... This would need to be
> implemented in the same way for Mali, IPU3, RKISP1 ... (and Pi
> separately)
> 
> (I had the same comment earlier on AGC for Mali ... but I don't think it
> blocks getting these merged - but I wouldn't want to see all the libipa
> implementations having a different response to control handling in the
> end).
> 
> With the adjustment for when you read the
> controls.get(controls::ColourTemperature); to be after the
> awb.autoEnabled()
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> >  
> > +       if (update)
> >                 LOG(RkISP1Awb, Debug)
> >                         << "Set colour gains to " << awb.gains.manual;
> > -       }
> > -
> > -       frameContext.awb.autoEnabled = awb.autoEnabled;
> >  
> > -       if (!awb.autoEnabled)
> > -               frameContext.awb.gains = awb.gains.manual;
> > +       frameContext.awb.gains = awb.gains.manual;
> >  }
> >  
> >  /**