[libcamera-devel] [PATCH v4 31/32] ipa: rkisp1: awb: Freeze AWB when means are too small

[Laurent Pinchart]

Hi Kieran,

On Wed, Sep 21, 2022 at 12:46:19AM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:59)
> > When the RGB means are too small, gains and color temperature can't be
> > meaningfully calculated. Freeze the AWB in that case, using the
> > previously calculated values.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/algorithms/awb.cpp | 17 ++++++++++++++++-
> >  src/ipa/rkisp1/ipa_context.cpp    |  3 +++
> >  src/ipa/rkisp1/ipa_context.h      |  1 +
> >  3 files changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp
> > index 5f2535688c93..404ad66e6953 100644
> > --- a/src/ipa/rkisp1/algorithms/awb.cpp
> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp
> > @@ -31,6 +31,9 @@ namespace ipa::rkisp1::algorithms {
> >  
> >  LOG_DEFINE_CATEGORY(RkISP1Awb)
> >  
> > +/* Minimum mean value below which AWB can't operate. */
> > +constexpr double kMeanMinThreshold = 2.0;
> 
> Will this be a tuning parameter? Is it something to calibrate later? or
> better document the expected ranges that should be used here?
> 
> What has led to the choice of 2.0, - is it simply that we can't estimate
> a colour temparature from a near black / dark image?
> 
> If so - should we have a representation of overall brightness?

The means are supposed to be in the 0-255 range. A value below 2 is just
too low to do any meaningful computation. We'll probably need to adjust
this, as I think the threshold is too low, but I haven't spent any time
on that. I don't think it should be a tuning parameter, it's just a
threshold to avoid numerical instability.

> > +
> >  Awb::Awb()
> >         : rgbMode_(false)
> >  {
> > @@ -263,7 +266,17 @@ void Awb::process(IPAContext &context,
> >         greenMean /= frameContext.awb.gains.green;
> >         blueMean /= frameContext.awb.gains.blue;
> >  
> > -       frameContext.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean);
> > +       /*
> > +        * If the means are too small we don't have enough information to
> > +        * meaningfully calculate gains. Freeze the algorithm in that case.
> > +        */
> > +       if (redMean < kMeanMinThreshold && greenMean < kMeanMinThreshold &&
> > +           blueMean < kMeanMinThreshold) {
> > +               frameContext.awb.temperatureK = activeState.awb.temperatureK;
> > +               return;
> > +       }
> > +
> > +       activeState.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean);
> 
> Aha - good, here's the active state view of the colour temperature.
> 
> 
> While I have questions about how the threshold is determined, I think
> this is still good to get in.
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> >  
> >         /*
> >          * Estimate the red and blue gains to apply in a grey world. The green
> > @@ -290,6 +303,8 @@ void Awb::process(IPAContext &context,
> >         activeState.awb.gains.automatic.blue = blueGain;
> >         activeState.awb.gains.automatic.green = 1.0;
> >  
> > +       frameContext.awb.temperatureK = activeState.awb.temperatureK;
> > +
> >         LOG(RkISP1Awb, Debug) << std::showpoint
> >                 << "Means [" << redMean << ", " << greenMean << ", " << blueMean
> >                 << "], gains [" << activeState.awb.gains.automatic.red << ", "
> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > index 335cb32c538d..9cac05bec79b 100644
> > --- a/src/ipa/rkisp1/ipa_context.cpp
> > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > @@ -156,6 +156,9 @@ namespace libcamera::ipa::rkisp1 {
> >   * \var IPAActiveState::awb.gains.automatic.blue
> >   * \brief Automatic white balance gain for B channel
> >   *
> > + * \var IPAActiveState::awb.temperatureK
> > + * \brief Estimated color temperature
> > + *
> >   * \var IPAActiveState::awb.autoEnabled
> >   * \brief Whether the Auto White Balance algorithm is enabled
> >   */
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index 53787320e565..d0bc9090a8f1 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -68,6 +68,7 @@ struct IPAActiveState {
> >                         } automatic;
> >                 } gains;
> >  
> > +               unsigned int temperatureK;
> >                 bool autoEnabled;
> >         } awb;
> >  

-- 
Regards,

Laurent Pinchart