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

[Jean-Michel Hautbois]

Hi Laurent,

On 15/10/2021 02:48, Laurent Pinchart wrote:
> 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.

Ah ah you spent too much time in this spec, right ;-) ?

> 
>> 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_);

I did not think about passing the template type to std::clamp()
because... I did not know we could do that, thanks !

> 
> 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_);
>>>                 }
>