[PATCH 1/2] ipa: rpi: awb: Add a const for the default colour temperature

[Naushir Patuck]

Hi Laurent,

Thank you for the feedback.

On Mon, 30 Sept 2024 at 16:20, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Naush,
>
> Thank you for the patch.
>
> On Mon, Sep 30, 2024 at 09:40:39AM +0100, Naushir Patuck wrote:
> > A default CT of 4500K is used in a couple of places. Add a constexpr
> > value for the default CT value and use it instead.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  src/ipa/rpi/controller/rpi/awb.cpp | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp
> > index f45525bce2d1..24f296fc66fa 100644
> > --- a/src/ipa/rpi/controller/rpi/awb.cpp
> > +++ b/src/ipa/rpi/controller/rpi/awb.cpp
> > @@ -20,6 +20,8 @@ using namespace libcamera;
> >
> >  LOG_DEFINE_CATEGORY(RPiAwb)
> >
> > +constexpr double DefaultCT = 4500.0;
>
> s/DefaultCT/kDefaultCT/

I'd prefer to keep it as-is because it's consistent with the rest of
the algorithm code in RPi land.

>
> > +
> >  #define NAME "rpi.awb"
> >
> >  /*
> > @@ -214,7 +216,7 @@ void Awb::initialise()
> >               syncResults_.gainB = 1.0 / config_.ctB.eval(syncResults_.temperatureK);
> >       } else {
> >               /* random values just to stop the world blowing up */
> > -             syncResults_.temperatureK = 4500;
> > +             syncResults_.temperatureK = DefaultCT;
> >               syncResults_.gainR = syncResults_.gainG = syncResults_.gainB = 1.0;
> >       }
> >       prevSyncResults_ = syncResults_;
> > @@ -716,7 +718,7 @@ void Awb::awbGrey()
> >               sumR += *ri, sumB += *bi;
> >       double gainR = sumR.G / (sumR.R + 1),
> >              gainB = sumB.G / (sumB.B + 1);
> > -     asyncResults_.temperatureK = 4500; /* don't know what it is */
> > +     asyncResults_.temperatureK = DefaultCT; /* don't know what it is */
>
> "don't know what it is" sounds like the person who wrote the code
> doesn't know what it does :-)

Hmmm, I really should remove that comment!

>
> Do I understand correctly that with the grey world model you can't
> estimate the colour temperature ? If so, while at it, I'd write
>
>         /*
>          * The grey world model can't estimate the colour temperature, use a
>          * default value.
>          */
>

Ack, I'll do that.

Naush


> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> >       asyncResults_.gainR = gainR;
> >       asyncResults_.gainG = 1.0;
> >       asyncResults_.gainB = gainB;
>
> --
> Regards,
>
> Laurent Pinchart