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

Message ID 20250326080034.1733385-4-paul.elder@ideasonboard.com
State New
Headers show
Series
  • libipa: Fix CameraSensorHelper gain helpers
Related show

Commit Message

Paul Elder March 26, 2025, 8 a.m. UTC 
From: Kieran Bingham <kieran.bingham@ideasonboard.com>

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>
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v2:
- recover from bitrot
---
 src/ipa/libipa/camera_sensor_helper.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Kieran Bingham March 27, 2025, 8:39 a.m. UTC | #1 
Hi Paul,

Quoting Paul Elder (2025-03-26 08:00:33)
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> 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.
> 

Thanks for reviving this.

I think this one needs to be thought about carefully though - I recall
some discussion with David, where the key/important information I missed
when I worked on this last was that we shouldn't ever round up in these
equations...

Could you review https://patchwork.libcamera.org/patch/20099/ please?

When I started this work - I mis-interpreted the underlying issue as a
rounding issue - but it's not. And using round() causes different
problems.

I made a playground for graphing all of this at 
 https://git.uk.ideasonboard.com/kbingham/octave-gains

But this should be re-implemented based on the feedback from David.
--
Kieran


> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v2:
> - recover from bitrot
> ---
>  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 7c66cd57d685..5645dd14fda3 100644
> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> @@ -90,12 +90,12 @@ uint32_t CameraSensorHelper::gainCode(double gain) const
>         if (auto *l = std::get_if<AnalogueGainLinear>(&gain_)) {
>                 ASSERT(l->m0 == 0 || l->m1 == 0);
>  
> -               return (l->c0 - l->c1 * gain) /
> -                      (l->m1 * gain - l->m0);
> +               return std::round((l->c0 - l->c1 * gain) /
> +                                 (l->m1 * gain - l->m0));
>         } else if (auto *e = std::get_if<AnalogueGainExp>(&gain_)) {
>                 ASSERT(e->a != 0 && e->m != 0);
>  
> -               return std::log2(gain / e->a) / e->m;
> +               return std::round(std::log2(gain / e->a) / e->m);
>         } else {
>                 ASSERT(false);
>                 return 0;
> -- 
> 2.47.2
>
Paul Elder March 28, 2025, 5:17 a.m. UTC | #2 
Hi Kieran,

On Thu, Mar 27, 2025 at 08:39:10AM +0000, Kieran Bingham wrote:
> Hi Paul,
> 
> Quoting Paul Elder (2025-03-26 08:00:33)
> > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > 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.
> > 
> 
> Thanks for reviving this.
> 
> I think this one needs to be thought about carefully though - I recall
> some discussion with David, where the key/important information I missed
> when I worked on this last was that we shouldn't ever round up in these
> equations...
> 
> Could you review https://patchwork.libcamera.org/patch/20099/ please?

Ah, I didn't notice that. I'll take a look.

> 
> When I started this work - I mis-interpreted the underlying issue as a
> rounding issue - but it's not. And using round() causes different
> problems.
> 
> I made a playground for graphing all of this at 
>  https://git.uk.ideasonboard.com/kbingham/octave-gains

Oh neat, thanks,

> 
> But this should be re-implemented based on the feedback from David.

ack


Paul

> 
> 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > Changes in v2:
> > - recover from bitrot
> > ---
> >  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 7c66cd57d685..5645dd14fda3 100644
> > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > @@ -90,12 +90,12 @@ uint32_t CameraSensorHelper::gainCode(double gain) const
> >         if (auto *l = std::get_if<AnalogueGainLinear>(&gain_)) {
> >                 ASSERT(l->m0 == 0 || l->m1 == 0);
> >  
> > -               return (l->c0 - l->c1 * gain) /
> > -                      (l->m1 * gain - l->m0);
> > +               return std::round((l->c0 - l->c1 * gain) /
> > +                                 (l->m1 * gain - l->m0));
> >         } else if (auto *e = std::get_if<AnalogueGainExp>(&gain_)) {
> >                 ASSERT(e->a != 0 && e->m != 0);
> >  
> > -               return std::log2(gain / e->a) / e->m;
> > +               return std::round(std::log2(gain / e->a) / e->m);
> >         } else {
> >                 ASSERT(false);
> >                 return 0;
> > -- 
> > 2.47.2
> >

Patch
diff mbox series 

diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
index 7c66cd57d685..5645dd14fda3 100644
--- a/src/ipa/libipa/camera_sensor_helper.cpp
+++ b/src/ipa/libipa/camera_sensor_helper.cpp
@@ -90,12 +90,12 @@  uint32_t CameraSensorHelper::gainCode(double gain) const
 	if (auto *l = std::get_if<AnalogueGainLinear>(&gain_)) {
 		ASSERT(l->m0 == 0 || l->m1 == 0);
 
-		return (l->c0 - l->c1 * gain) /
-		       (l->m1 * gain - l->m0);
+		return std::round((l->c0 - l->c1 * gain) /
+				  (l->m1 * gain - l->m0));
 	} else if (auto *e = std::get_if<AnalogueGainExp>(&gain_)) {
 		ASSERT(e->a != 0 && e->m != 0);
 
-		return std::log2(gain / e->a) / e->m;
+		return std::round(std::log2(gain / e->a) / e->m);
 	} else {
 		ASSERT(false);
 		return 0;