[libcamera-devel] [PATCH v4 4/4] ipa: ipu3: awb: Clamp gain values

[Laurent Pinchart]

On Wed, Mar 02, 2022 at 12:37:07PM +0000, Kieran Bingham wrote:
> Quoting Umang Jain (2022-02-28 17:02:43)
> > Hi JM
> > 
> > On 2/24/22 20:41, Jean-Michel Hautbois wrote:
> > > The gain values are coded as u3.13 fixed point values, ie they can not
> > > be more than 8. Clampt the values in order to avoid any off limits value
> > 
> > 
> > s/Clampt/Clamp
> > 
> > > which could make the IPU3 behave weirdly.
> > >
> > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> > > ---
> > >   src/ipa/ipu3/algorithms/awb.cpp | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > >
> > > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> > > index 1dc27fc9..dc25be81 100644
> > > --- a/src/ipa/ipu3/algorithms/awb.cpp
> > > +++ b/src/ipa/ipu3/algorithms/awb.cpp
> > > @@ -353,6 +353,14 @@ void Awb::awbGreyWorld()
> > >   
> > >       /* Color temperature is not relevant in Grey world but still useful to estimate it :-) */
> > >       asyncResults_.temperatureK = estimateCCT(sumRed.R, sumRed.G, sumBlue.B);
> > > +
> > > +     /*
> > > +      * Gain values are unsigned integer value, range 0 to 8 with 13 bit
> 
> Techincally, we can't store '8' in this representation though can we.
> It looks like the max is something like 7.99987792969 ... so maybe this
> should be range 0 to < 8 ?
> 
> Unless there's a better notation to say up to but not including 8?

[0, 8[ and [0, 8) are commonly used for this purpose.

> > I am not able to decipher "integer value" with "fractional part". The 
> > redGain and blueGain are floating valuesright..?
> 
> Perhaps stating that they are stored as unsigned integer 3.13 fixed
> point might be more clear.
> 
> I would love to see a proper fixed-point class representing that, but
> that could be a big task, so I think this is fine until we get something
> like that.
> 
> With Clampt/Clamp, and this updated if needed/desired.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > 
> > > +      * fractional part.
> > > +      */
> > > +     redGain = std::clamp(redGain, 0.0, 65535.0 / 8192);
> > > +     blueGain = std::clamp(blueGain, 0.0, 65535.0 / 8192);
> > > +
> > >       asyncResults_.redGain = redGain;
> > >       /* Hardcode the green gain to 1.0. */
> > >       asyncResults_.greenGain = 1.0;

-- 
Regards,

Laurent Pinchart