[PATCH v1 3/6] ipa: rkisp1: awb: Implement ColourTemperature control

[David Plowman]

Hi Stefan

Thanks for resurrecting this topic. I seem to remember submitting
something like this a looong time ago. Don't recall what, if anything,
ever happened to it. But indeed, I think it's useful, so glad to see
it's not been forgotten!

On Tue, 13 Aug 2024 at 08:37, Stefan Klug <stefan.klug at ideasonboard.com> wrote:
>
> Hi Kieran,
>
> On Tue, Aug 06, 2024 at 09:54:41AM +0100, Kieran Bingham wrote:
> > Quoting Stefan Klug (2024-08-06 07:37:54)
> > > Hi Kieran,
> > >
> > > Thanks for the review.
> > >
> > > On Mon, Aug 05, 2024 at 02:46:00PM +0100, Kieran Bingham wrote:
> > > > Quoting Stefan Klug (2024-08-05 13:05:04)
> > > > > 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 at ideasonboard.com>
> > > > > ---
> > > > >  src/ipa/rkisp1/algorithms/awb.cpp | 20 ++++++++++++++++++++
> > > > >  1 file changed, 20 insertions(+)
> > > > >
> > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > > > > index 957d24fe3425..d482eda5b541 100644
> > > > > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > > > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > > > > @@ -31,6 +31,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,6 +48,11 @@ Awb::Awb()
> > > > >   */
> > > > >  int Awb::init(IPAContext &context, const YamlObject &tuningData)
> > > > >  {
> > > > > +       auto &cmap = context.ctrlMap;
> > > > > +       cmap[&controls::ColourTemperature] = ControlInfo(kMinColourTemperature,
> > > > > +                                                        kMaxColourTemperature,
> > > > > +                                                        kDefaultColourTemperature);
> > > > > +
> > > >
> > > > Should this control only be exposed if there are gains available in the
> > > > tuning file?
> > >
> > > As noted in the previous mail I think unconditional is ok as it also
> > > applies to the ccms which should be available.
> >
> > So - perhaps we should be instantiating this control at the top level
> > and not inside the awb. If someone disables the awb module
> > in the tuning file, it would impact CCM ?
> >
>
> I thought about that a bit. In automatic mode, the colour temperature
> gets estimated in the awb module. So we would also need to move the
> AwbEnable handling to the top level. So code wise, the colour
> temperature would be set from two different places and the Awb handling
> would also be spread across two places. I don't think that's worth it.

I think it suits us (RPi) better like this too. It's also like our
AEC/AGC, where fixed values are actually handled by the algorithm even
when you're in manual mode.

Thanks
David

>
> Cheers,
> Stefan
>
> >
> > > > Can/Should the minColourTemperature/maxColourTemperature be based on the
> > > > values that are available in the Tuning file? Or does the interpolator
> > > > make this possible to extend beyond the limits calibrated if it can
> > > > extrapolate for wider values?
> > >
> > > The interpolator extends the values if the temperature out of range.
> > > Here we would also need to check the range of ccms and gains. I don't
> > > know if it's worth the effort. On the other hand the min/max values are
> > > arbitrary. A quick google indicates that we could even lower the min to
> > > 1000.
> > >
> > > > >         MatrixInterpolator<double, 2, 1> gains;
> > > > >         int ret = gains.readYaml(tuningData["gains"], "ct", "gains");
> > > > >         if (ret < 0)
> > > > > @@ -113,6 +122,17 @@ void Awb::queueRequest(IPAContext &context,
> > > > >                         << ", blue: " << awb.gains.manual.blue;
> > > > >         }
> > > > >
> > > > > +       const auto &colourTemperature = controls.get(controls::ColourTemperature);
> > > > > +       if (colourTemperature && !awb.autoEnabled && gains_ && !colourGains) {
> > > > > +               Matrix<double, 2, 1> gains = gains_->get(*colourTemperature);
> > > > > +               awb.gains.manual.red = gains[0][0];
> > > > > +               awb.gains.manual.blue = gains[1][0];
> > > > > +
> > > > > +               LOG(RkISP1Awb, Debug)
> > > > > +                       << "Set colour gains to red: " << awb.gains.manual.red
> > > > > +                       << ", blue: " << awb.gains.manual.blue;
> > > > > +       }
> > > > > +
> > > > >         frameContext.awb.autoEnabled = awb.autoEnabled;
> > > > >
> > > > >         if (!awb.autoEnabled) {
> > > > > --
> > > > > 2.43.0
> > > > >