[libcamera-devel] [PATCH 1/5] ipa: ipu3: agc: Drop kMaxLuminance constant

[Kieran Bingham]

Quoting Laurent Pinchart (2021-11-17 10:12:56)
> Hi Jean-Michel,
> 
> On Wed, Nov 17, 2021 at 11:09:55AM +0100, Jean-Michel Hautbois wrote:
> > Hi Laurent,
> > 
> > Thanks for the patch !
> > 
> > On 16/11/2021 17:26, Laurent Pinchart wrote:
> > > The kMaxLuminance constant is badly named, it's not a maximum luminance,
> > > but the maximum integer value output by the AWB statistics engine for
> > > per-channel averages. The constant is used in a single place, hardcoding
> > > the value is actually more readable.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> > I hesitated and made it a constexpr to avoid "magic values"... :-)
> 
> I also prefer avoiding magic values. Here I think "kMaxLuminance" isn't
> a very good name. It's really the maximum value of the RGB averages
> computed by the ImgU in AWB statistics. As it's only used in a single
> function, and as I couldn't think of a good name for the constant, I
> decided to drop it.

I thought MaxLuminance conveyed that. As in - the maximum lumincance is
255, so the maximum average possible is 255.

But I don't mind it being hardcoded if that's prefered.


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> > 
> > > ---
> > >   src/ipa/ipu3/algorithms/agc.cpp | 6 +-----
> > >   1 file changed, 1 insertion(+), 5 deletions(-)
> > > 
> > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > > index bd02c474611c..43a39ffd57d6 100644
> > > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > > @@ -61,9 +61,6 @@ static constexpr double kEvGainTarget = 0.5;
> > >   /* Number of frames to wait before calculating stats on minimum exposure */
> > >   static constexpr uint32_t kNumStartupFrames = 10;
> > >   
> > > -/* Maximum luminance used for brightness normalization */
> > > -static constexpr uint32_t kMaxLuminance = 255;
> > > -
> > >   /*
> > >    * Normalized luma value target.
> > >    *
> > > @@ -298,8 +295,7 @@ double Agc::computeInitialY(IPAFrameContext &frameContext,
> > >                    greenSum * frameContext.awb.gains.green * .587 +
> > >                    blueSum * frameContext.awb.gains.blue * .114;
> > >   
> > > -   /* Return the normalized relative luminance. */
> > > -   return Y_sum / (grid.height * grid.width) / kMaxLuminance;
> > > +   return Y_sum / (grid.height * grid.width) / 255;
> > >   }
> > >   
> > >   /**
> 
> -- 
> Regards,
> 
> Laurent Pinchart