| Message ID | 20211119102559.56522-2-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@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? Reviewed-by: Kieran Bingham <kieran.bingham@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)); > } > > /** > -- > 2.32.0 >
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. >> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@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... 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@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)); >> } >> >> /** >> -- >> 2.32.0 >>
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@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@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)); > >> } > >> > >> /**
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@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@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)); >>>> } >>>> >>>> /** >
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@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@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)); > >>>> } > >>>> > >>>> /**
On 19/11/2021 14:27, Laurent Pinchart wrote: > 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@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. > The driver sets it to 0... The real issue being in configure(): minGain_ = std::max<uint32_t>(itGain->second.min().get<int32_t>(), 1); I will remove this patch :-) and have another one instead, thanks ! >>>> 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@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)); >>>>>> } >>>>>> >>>>>> /** >
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)); } /**
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. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/ipa/libipa/camera_sensor_helper.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)