[PATCH 2/2] ipa: rpi: awb: Add a bias to the AWB search

[Naushir Patuck]

Hi David,

On Mon, 30 Sept 2024 at 16:14, David Plowman
<david.plowman at raspberrypi.com> wrote:
>
> Hi Naush
>
> Thanks for this! I think it's an elegant solution, where failure cases
> tend in the limit to the "default", rather than having hard cut-offs
> that are then difficult to manage.
>
> On Mon, 30 Sept 2024 at 09:40, Naushir Patuck <naush at raspberrypi.com> wrote:
> >
> > In the case of an AWB search failure, the current algorithm logic will
> > return a point on the CT curve closest to where the search finisned.
> > This can be quite undesirable. Instead, add some bias params to the AWB
> > algorithm which will direct the search to a set CT value in the case
> > where statistics become unreliable causing the search to fail.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  src/ipa/rpi/controller/rpi/awb.cpp | 17 +++++++++++++++--
> >  src/ipa/rpi/controller/rpi/awb.h   |  4 ++++
> >  2 files changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp
> > index 24f296fc66fa..8f46dae0a961 100644
> > --- a/src/ipa/rpi/controller/rpi/awb.cpp
> > +++ b/src/ipa/rpi/controller/rpi/awb.cpp
> > @@ -169,6 +169,8 @@ int AwbConfig::read(const libcamera::YamlObject &params)
> >         whitepointB = params["whitepoint_b"].get<double>(0.0);
> >         if (bayes == false)
> >                 sensitivityR = sensitivityB = 1.0; /* nor do sensitivities make any sense */
> > +       biasProportion = params["bias_proportion"].get<double>(0.0);
>
> I wonder if some comment somewhere that suggests a reasonable bias
> proportion might be helpful? I know it should go in our official
> documentation, which is probably long overdue an update, but it might
> not hurt to put something here in the meantime?

Sure, will do, but see below...

>
> > +       biasCT = params["bias_ct"].get<double>(DefaultCT);
> >         return 0;
> >  }
> >
> > @@ -409,7 +411,8 @@ void Awb::asyncFunc()
> >
> >  static void generateStats(std::vector<Awb::RGB> &zones,
> >                           StatisticsPtr &stats, double minPixels,
> > -                         double minG, Metadata &globalMetadata)
> > +                         double minG, Metadata &globalMetadata,
> > +                         double biasProportion, double biasCtR, double biasCtB)
> >  {
> >         std::scoped_lock<RPiController::Metadata> l(globalMetadata);
> >
> > @@ -422,6 +425,14 @@ static void generateStats(std::vector<Awb::RGB> &zones,
> >                                 continue;
> >                         zone.R = region.val.rSum / region.counted;
> >                         zone.B = region.val.bSum / region.counted;
> > +                       /*
> > +                        * Add some bias samples to allow the search to tend to a
> > +                        * bias CT in failure cases.
> > +                        */
> > +                       const unsigned int proportion = biasProportion * region.counted;
>
> Being perhaps overly paranoid, is there a risk (probably on a vc4
> platform rather than a PiSP one), that region.counted could be zero?
> In which case the failure avoidance going on here would itself fail.
> Maybe ensure proportion is at least 1?
>
> But these minor quibbles notwithstanding:
>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
>
> Now all we need to do is think about the other hard cut-off for the
> number of valid zones... :)

I'm half wondering whether we change things so we use the proportion
of the total region pixel area rather than the proportion of the
counted pixels?  This would fix the problem on vc4 where
region.counted might be 0, and probably give us enough to not need to
bother with the hard cut-off?

Naush


>
> David
>
> > +                       zone.R += proportion * biasCtR;
> > +                       zone.B += proportion * biasCtB;
> > +                       zone.G += proportion * 1.0;
> >                         /* Factor in the ALSC applied colour shading correction if required. */
> >                         const AlscStatus *alscStatus = globalMetadata.getLocked<AlscStatus>("alsc.status");
> >                         if (stats->colourStatsPos == Statistics::ColourStatsPos::PreLsc && alscStatus) {
> > @@ -442,7 +453,9 @@ void Awb::prepareStats()
> >          * any LSC compensation.  We also ignore config_.fast in this version.
> >          */
> >         generateStats(zones_, statistics_, config_.minPixels,
> > -                     config_.minG, getGlobalMetadata());
> > +                     config_.minG, getGlobalMetadata(),
> > +                     config_.biasProportion, config_.ctR.eval(config_.biasCT),
> > +                     config_.ctB.eval(config_.biasCT));
> >         /*
> >          * apply sensitivities, so values appear to come from our "canonical"
> >          * sensor.
> > diff --git a/src/ipa/rpi/controller/rpi/awb.h b/src/ipa/rpi/controller/rpi/awb.h
> > index ab30f4fa51c1..5d628b47c8a6 100644
> > --- a/src/ipa/rpi/controller/rpi/awb.h
> > +++ b/src/ipa/rpi/controller/rpi/awb.h
> > @@ -87,6 +87,10 @@ struct AwbConfig {
> >         double whitepointR;
> >         double whitepointB;
> >         bool bayes; /* use Bayesian algorithm */
> > +       /* proportion of counted samples to add for the search bias */
> > +       double biasProportion;
> > +       /* CT target for the search bias */
> > +       double biasCT;
> >  };
> >
> >  class Awb : public AwbAlgorithm
> > --
> > 2.34.1
> >