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

[Naushir Patuck]

On Tue, 1 Oct 2024 at 12:11, David Plowman
<david.plowman at raspberrypi.com> wrote:
>
> On Tue, 1 Oct 2024 at 11:28, Naushir Patuck <naush at raspberrypi.com> wrote:
> >
> > 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?
>
> Hmm, yes. I guess on Pi 5 "counted" happens to give us everything,
> whereas on a Pi 4 it probably doesn't. Do we have a way to get the
> total number of all pixels in a portable way? My vague recollection is
> that vc4 has an "uncounted" field, but without looking it up I'm not
> too sure...

VC4 has counted/unconted fields so it's easy to get the region total.

On PISP we have counted, and currently uncounted is set to 0.
However, it ought to be easy enough to get uncounted from (total -
counted).  Then we add proportion * (counted + uncounted) for every
zone even if counted == 0.  If we decide to do this, I think we are
better off doing it now rather than introducing it later.

Naush


>
> David
>
> >
> > 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
> > > >