[3/3] libipa: camera_sensor_helper: Fix rounding of gainCode
diff mbox series

Message ID 20240223155954.4139705-4-kieran.bingham@ideasonboard.com
State Superseded
Headers show
Series
  • libipa: Fix CameraSensorHelper gain helpers
Related show

Commit Message

Kieran Bingham Feb. 23, 2024, 3:59 p.m. UTC
The implementation of gainCode for both Exponential and Linear gain
models does not generate a gainCode that matches the result of the
reverse operation.

This can be seen by converting sequential gainCodes to a gain
and converting that back to the gainCode. The values do not
translate back due to rounding errors.

Correct the rounding error and ensure that gainCode translation
produces accurate bi-directional conversions from the perspective
of the gainCode.

This fixes the IMX290, IMX296, IMX327 and IMX335 which use the
Exponential gain model helpers, as well as IMX219 IMX258 and IMX477
which use the Linear gain model helpers.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/libipa/camera_sensor_helper.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Laurent Pinchart Feb. 27, 2024, 2:58 p.m. UTC | #1
Hi Kieran,

Thank you for the patch.

On Fri, Feb 23, 2024 at 03:59:54PM +0000, Kieran Bingham wrote:
> The implementation of gainCode for both Exponential and Linear gain

s/gainCode/gainCode()/

> models does not generate a gainCode that matches the result of the
> reverse operation.
> 
> This can be seen by converting sequential gainCodes to a gain
> and converting that back to the gainCode. The values do not
> translate back due to rounding errors.

It's not due to rounding errors, but to the fact that we only round
down, isn't it ?

> Correct the rounding error and ensure that gainCode translation
> produces accurate bi-directional conversions from the perspective
> of the gainCode.

Even rounding to the closest integer (which I think is a good idea), are
you sure you'll guarantee that gainCode(gain(code)) will always be equal
to code, in *all* cases ?

> This fixes the IMX290, IMX296, IMX327 and IMX335 which use the
> Exponential gain model helpers, as well as IMX219 IMX258 and IMX477

Missing commas.

> which use the Linear gain model helpers.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/ipa/libipa/camera_sensor_helper.cpp | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> index a925b37b9f7c..39e10d86a2c7 100644
> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> @@ -64,13 +64,13 @@ uint32_t CameraSensorHelper::gainCode(double gain) const
>  	case AnalogueGainLinear:
>  		ASSERT(k.linear.m0 == 0 || k.linear.m1 == 0);
>  
> -		return (k.linear.c0 - k.linear.c1 * gain) /
> -		       (k.linear.m1 * gain - k.linear.m0);
> +		return std::round((k.linear.c0 - k.linear.c1 * gain) /
> +				  (k.linear.m1 * gain - k.linear.m0));
>  
>  	case AnalogueGainExponential:
>  		ASSERT(k.exp.a != 0 && k.exp.m != 0);
>  
> -		return std::log2(gain / k.exp.a) / k.exp.m;
> +		return std::round(std::log2(gain / k.exp.a) / k.exp.m);

The code change seems fine, but the commit message may need to be
reworked.

>  
>  	default:
>  		ASSERT(false);
Kieran Bingham Feb. 27, 2024, 5:17 p.m. UTC | #2
Quoting Laurent Pinchart (2024-02-27 14:58:47)
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Fri, Feb 23, 2024 at 03:59:54PM +0000, Kieran Bingham wrote:
> > The implementation of gainCode for both Exponential and Linear gain
> 
> s/gainCode/gainCode()/
> 
> > models does not generate a gainCode that matches the result of the
> > reverse operation.
> > 
> > This can be seen by converting sequential gainCodes to a gain
> > and converting that back to the gainCode. The values do not
> > translate back due to rounding errors.
> 
> It's not due to rounding errors, but to the fact that we only round
> down, isn't it ?

Isn't that a rounding error? An error when rounding (to integer)? ... :-)

> > Correct the rounding error and ensure that gainCode translation
> > produces accurate bi-directional conversions from the perspective
> > of the gainCode.
> 
> Even rounding to the closest integer (which I think is a good idea), are
> you sure you'll guarantee that gainCode(gain(code)) will always be equal
> to code, in *all* cases ?

Run the test with and without the fix...

I haven't done a formal proof here no. But that's also why the test ...
tests every CameraSensorHelper in more or less brute force.

> > This fixes the IMX290, IMX296, IMX327 and IMX335 which use the
> > Exponential gain model helpers, as well as IMX219 IMX258 and IMX477
> 
> Missing commas.

Ok.

> 
> > which use the Linear gain model helpers.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/ipa/libipa/camera_sensor_helper.cpp | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > index a925b37b9f7c..39e10d86a2c7 100644
> > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > @@ -64,13 +64,13 @@ uint32_t CameraSensorHelper::gainCode(double gain) const
> >       case AnalogueGainLinear:
> >               ASSERT(k.linear.m0 == 0 || k.linear.m1 == 0);
> >  
> > -             return (k.linear.c0 - k.linear.c1 * gain) /
> > -                    (k.linear.m1 * gain - k.linear.m0);
> > +             return std::round((k.linear.c0 - k.linear.c1 * gain) /
> > +                               (k.linear.m1 * gain - k.linear.m0));
> >  
> >       case AnalogueGainExponential:
> >               ASSERT(k.exp.a != 0 && k.exp.m != 0);
> >  
> > -             return std::log2(gain / k.exp.a) / k.exp.m;
> > +             return std::round(std::log2(gain / k.exp.a) / k.exp.m);
> 
> The code change seems fine, but the commit message may need to be
> reworked.
> 
> >  
> >       default:
> >               ASSERT(false);
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series

diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
index a925b37b9f7c..39e10d86a2c7 100644
--- a/src/ipa/libipa/camera_sensor_helper.cpp
+++ b/src/ipa/libipa/camera_sensor_helper.cpp
@@ -64,13 +64,13 @@  uint32_t CameraSensorHelper::gainCode(double gain) const
 	case AnalogueGainLinear:
 		ASSERT(k.linear.m0 == 0 || k.linear.m1 == 0);
 
-		return (k.linear.c0 - k.linear.c1 * gain) /
-		       (k.linear.m1 * gain - k.linear.m0);
+		return std::round((k.linear.c0 - k.linear.c1 * gain) /
+				  (k.linear.m1 * gain - k.linear.m0));
 
 	case AnalogueGainExponential:
 		ASSERT(k.exp.a != 0 && k.exp.m != 0);
 
-		return std::log2(gain / k.exp.a) / k.exp.m;
+		return std::round(std::log2(gain / k.exp.a) / k.exp.m);
 
 	default:
 		ASSERT(false);