libipa: exposure_mode_helper: Set quantizationGain in absence of a sensor helper

25166 diff mbox series

In commit f077c58e087e ("libipa: exposure_mode_helper: Take
exposure/gain quantization into account") calculation of the
quantization gain was added to ExposureModeHelper::clampGain(). This
works as expected when a sensor helper is configured but the gain is not
reset to 1.0 in case the sensor helper is not configured. This leads to
incorrect gain calculations in ExposureModeHelper::splitExposure() as
that expects the quantization gain to be valid in any case. Fix that by
setting the quantization gain to 1.0 in case no sensor helper is
configured.

Fixes: f077c58e087e ("libipa: exposure_mode_helper: Take exposure/gain quantization into account")
Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/292
Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/ipa/libipa/exposure_mode_helper.cpp | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Quoting Stefan Klug (2025-11-24 19:52:41)

> In commit f077c58e087e ("libipa: exposure_mode_helper: Take
> exposure/gain quantization into account") calculation of the
> quantization gain was added to ExposureModeHelper::clampGain(). This
> works as expected when a sensor helper is configured but the gain is not
> reset to 1.0 in case the sensor helper is not configured. This leads to
> incorrect gain calculations in ExposureModeHelper::splitExposure() as
> that expects the quantization gain to be valid in any case. Fix that by
> setting the quantization gain to 1.0 in case no sensor helper is
> configured.
> 
> Fixes: f077c58e087e ("libipa: exposure_mode_helper: Take exposure/gain quantization into account")
> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/292
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  src/ipa/libipa/exposure_mode_helper.cpp | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp
> index 29e316d9d091..01c5cba8e22c 100644
> --- a/src/ipa/libipa/exposure_mode_helper.cpp
> +++ b/src/ipa/libipa/exposure_mode_helper.cpp
> @@ -144,9 +144,13 @@ utils::Duration ExposureModeHelper::clampExposureTime(utils::Duration exposureTi
>  double ExposureModeHelper::clampGain(double gain, double *quantizationGain) const
>  {
>         double clamped = std::clamp(gain, minGain_, maxGain_);
> -       if (!sensorHelper_)
> -               return clamped;
> -       return sensorHelper_->quantizeGain(clamped, quantizationGain);
> +       if (sensorHelper_)
> +               return sensorHelper_->quantizeGain(clamped, quantizationGain);
> +
> +       if (quantizationGain)
> +               *quantizationGain = 1.0;

Looks good to me.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +
> +       return clamped;
>  }
>  
>  /**
> -- 
> 2.51.0
>

Hi Stefan

On Mon, Nov 24, 2025 at 11:29:39PM +0000, Kieran Bingham wrote:
> Quoting Stefan Klug (2025-11-24 19:52:41)
>
> > In commit f077c58e087e ("libipa: exposure_mode_helper: Take
> > exposure/gain quantization into account") calculation of the
> > quantization gain was added to ExposureModeHelper::clampGain(). This
> > works as expected when a sensor helper is configured but the gain is not
> > reset to 1.0 in case the sensor helper is not configured. This leads to
> > incorrect gain calculations in ExposureModeHelper::splitExposure() as
> > that expects the quantization gain to be valid in any case. Fix that by
> > setting the quantization gain to 1.0 in case no sensor helper is
> > configured.
> >
> > Fixes: f077c58e087e ("libipa: exposure_mode_helper: Take exposure/gain quantization into account")
> > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/292
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  src/ipa/libipa/exposure_mode_helper.cpp | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp
> > index 29e316d9d091..01c5cba8e22c 100644
> > --- a/src/ipa/libipa/exposure_mode_helper.cpp
> > +++ b/src/ipa/libipa/exposure_mode_helper.cpp
> > @@ -144,9 +144,13 @@ utils::Duration ExposureModeHelper::clampExposureTime(utils::Duration exposureTi
> >  double ExposureModeHelper::clampGain(double gain, double *quantizationGain) const
> >  {
> >         double clamped = std::clamp(gain, minGain_, maxGain_);
> > -       if (!sensorHelper_)
> > -               return clamped;
> > -       return sensorHelper_->quantizeGain(clamped, quantizationGain);
> > +       if (sensorHelper_)
> > +               return sensorHelper_->quantizeGain(clamped, quantizationGain);
> > +
> > +       if (quantizationGain)
> > +               *quantizationGain = 1.0;
>
> Looks good to me.

I think we should make sensorHelper_ mandatory.

I can do this on top, as I need it anyway for the exposure series I
have in review and we should push for all IPAs that use libipa to be
in par.

>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

But this fixes a bug for me already on Mali, so please push it in the
meantime.

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Tested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

>
> > +
> > +       return clamped;
> >  }
> >
> >  /**
> > --
> > 2.51.0
> >