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

[Jean-Michel Hautbois]

Hi Umang, Kieran and Laurent,

On 02/03/2022 13:43, Laurent Pinchart wrote:
> 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.
> 

Indeed, sorry for this, I will correct it.

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