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

[Laurent Pinchart]

Hi Jean-Michel,

On Fri, Nov 19, 2021 at 02:22:23PM +0100, Jean-Michel Hautbois wrote:
> On 19/11/2021 13:47, Laurent Pinchart wrote:
> > 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.

Why is the minimum gain code 1 ? The sensor seems to support 0, at least
according to the datasheet.

> >> 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));
> >>>>    }
> >>>>    
> >>>>    /**

-- 
Regards,

Laurent Pinchart