[libcamera-devel] [PATCH 1/2] libipa: Round gain code before returning the value

[Jean-Michel Hautbois]

On 19/11/2021 13:47, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> Thank you for the patch.
> 
> On Fri, Nov 19, 2021 at 12:14:20PM +0100, Jean-Michel Hautbois wrote:
>> On 19/11/2021 12:11, Kieran Bingham wrote:
>>> Quoting Jean-Michel Hautbois (2021-11-19 10:25:58)
>>>> We can have the case where the gain is very close to 1, as coded in
>>>> double, and after gainCode() returns, the value as an uint32_t is 0. In
>>>> order to avoid that, always round the value halfway cases away from zero
>>>> before returning.
> 
> Why do we want to avoid that, why is rounding to the closest integer
> better than rounding down ?
> 
>>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>>>
>>> This seems to make sense.
>>>
>>> Are there any issues with other values being rounded? I presume not.
>>> Is there any requirement to further protect or warn if gainCode is
>>> returned as zero? or can that still be valid in some situations?
>>
>> That is a tough question... Can we have a gainCode set to 0 in a linear
>> analogue gain controlled sensor... I am not sure of that at all...
> 
> It may depends on the constaints of the linear model. For IMX219, which
> uses and inverse gain model (gain = 256 / (256 - gain code)), a gain
> code set to 0 is valid and produces a gain of 1.0.

The minimum gainCode is 1, which produces a gain of 1.00392.
When this gain is applied the other way around, gainCode returns 0 and 
not 1 as expected.

> 
>> BTW, investigating it (before introducing std::round) I found that using
>> float instead of double made it return 1...
>>
>>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>>
>>>> ---
>>>>    src/ipa/libipa/camera_sensor_helper.cpp | 6 ++++--
>>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
>>>> index 0b0eb503..67cf6913 100644
>>>> --- a/src/ipa/libipa/camera_sensor_helper.cpp
>>>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
>>>> @@ -7,6 +7,8 @@
>>>>     */
>>>>    #include "camera_sensor_helper.h"
>>>>    
>>>> +#include <cmath>
>>>> +
>>>>    #include <libcamera/base/log.h>
>>>>    
>>>>    /**
>>>> @@ -61,8 +63,8 @@ uint32_t CameraSensorHelper::gainCode(double gain) const
>>>>           ASSERT(analogueGainConstants_.m0 == 0 || analogueGainConstants_.m1 == 0);
>>>>           ASSERT(analogueGainConstants_.type == AnalogueGainLinear);
>>>>    
>>>> -       return (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /
>>>> -              (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);
>>>> +       return std::round((analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /
>>>> +                         (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0));
>>>>    }
>>>>    
>>>>    /**
>