[libcamera-devel] [PATCH 07/13] ipa: ipu3: agc: Change analogue gain limits

[Laurent Pinchart]

On Thu, Oct 14, 2021 at 12:05:45PM +0100, Kieran Bingham wrote:
> Quoting Jean-Michel Hautbois (2021-10-13 16:41:19)
> > The gains is currently set as a uint32_t while the analogue gain is
> 
> 'gains are currently' ?
> 
> > passed as a double. We also have a default maximum analogue gain of 15
> > which is quite high for a number of sensors.
> > 
> > Use a maximum value of 8 which should really be configured by the IPA
> > and not fixed as it is now. While at it make it a double.
> > 
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> > ---
> >  src/ipa/ipu3/algorithms/agc.cpp | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > index 2dd5c5c1..e58a8a8d 100644
> > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > @@ -30,14 +30,10 @@ static constexpr uint32_t kInitialFrameMinAECount = 4;
> >  /* Number of frames to wait between new gain/exposure estimations */
> >  static constexpr uint32_t kFrameSkipCount = 6;
> >  
> > -/* Maximum ISO value for analogue gain */
> > -static constexpr uint32_t kMinISO = 100;
> > -static constexpr uint32_t kMaxISO = 1500;
> > -
> 
> Do the ISO values no longer make sense to be used here?

ISO values don't make much sense anywhere to be honest :-) It's mostly a
marketing argument. Considering gain = ISO / 100 is as good as anything,
because you can set a random conversion factor (within limits) and still
be compliant with the specification.

> As this needs to come from a camera helper anyway, I guess it doesn't
> hurt to reduce it as it should be changed later all the same.
> 
> Do you reduce it to prevent some overexposure issue? or because the
> sensors you've tested only have x8 as a maximum?
> 
> Anyway, I don't object to this patch otherwise.
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> >  /* Maximum analogue gain value
> >   * \todo grab it from a camera helper */
> > -static constexpr uint32_t kMinGain = kMinISO / 100;
> > -static constexpr uint32_t kMaxGain = kMaxISO / 100;
> > +static constexpr double kMinGain = 1.0;
> > +static constexpr double kMaxGain = 8.0;
> >  
> >  /* \todo use calculated value based on sensor */
> >  static constexpr libcamera::utils::Duration kMinShutterSpeed = 100us;
> > @@ -161,9 +157,9 @@ void Agc::lockExposureGain(uint32_t &exposure, double &gain)
> >                 if (currentShutter < kMaxShutterSpeed) {
> >                         exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_), minExposureLines_, maxExposureLines_);
> >                         newExposure = currentExposure_ / exposure;
> > -                       gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);
> > +                       gain = std::clamp(static_cast<double>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);

This is also a good occasion to shorten lines, and you don't need the
cast:

			gain = std::clamp(gain * currentExposure_ / newExposure,
					  kMinGain, kMaxGain);

Same below.

By the way, in the previous patch, you could have used

			exposure = std::clamp<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_,
							minExposureLines_,
							maxExposureLines_);

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> >                 } else if (currentShutter >= kMaxShutterSpeed) {
> > -                       gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain);
> > +                       gain = std::clamp(static_cast<double>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain);
> >                         newExposure = currentExposure_ / gain;
> >                         exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / newExposure), minExposureLines_, maxExposureLines_);
> >                 }

-- 
Regards,

Laurent Pinchart