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

[Laurent Pinchart]

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.

> 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