| Message ID | 20230519162215.413831-1-jacopo.mondi@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Fri, May 19, 2023 at 06:22:15PM +0200, Jacopo Mondi via libcamera-devel wrote: > As the sensor's analogue gain range is known drop the arbitrary > limits for the sensor analogue gain. As written in the review of the corresponding rkisp1 patch, I think limits should be handled in the IPA module. While I don't expect many camera sensors to support gains smaller than 1.0, some may, and the CameraSensor class should expose the real capabilities of the sensor. The IPA module is responsible for setting the policies of the algorithms. For the minimum gain, we shouldn't go below 1.0 as I really don't see any major use case. For the maximum gain, there's a policy decision to consider, but that should come from the tuning data, not be hardcoded here. I'm thus OK with the change for the maximum gain. There's a risk it will cause regressions for sensors that have a very high maximum gain value, so we may need to add AGC tuning data sooner than later. > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/agc.cpp | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > index b5309bdbea25..74a14675fca0 100644 > --- a/src/ipa/ipu3/algorithms/agc.cpp > +++ b/src/ipa/ipu3/algorithms/agc.cpp > @@ -47,10 +47,6 @@ namespace ipa::ipu3::algorithms { > > LOG_DEFINE_CATEGORY(IPU3Agc) > > -/* Limits for analogue gain values */ > -static constexpr double kMinAnalogueGain = 1.0; > -static constexpr double kMaxAnalogueGain = 8.0; > - > /* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */ > static constexpr utils::Duration kMaxShutterSpeed = 60ms; > > @@ -96,11 +92,11 @@ int Agc::configure(IPAContext &context, > maxShutterSpeed_ = std::min(configuration.agc.maxShutterSpeed, > kMaxShutterSpeed); > > - minAnalogueGain_ = std::max(configuration.agc.minAnalogueGain, kMinAnalogueGain); > - maxAnalogueGain_ = std::min(configuration.agc.maxAnalogueGain, kMaxAnalogueGain); > + minAnalogueGain_ = configuration.agc.minAnalogueGain; > + maxAnalogueGain_ = configuration.agc.maxAnalogueGain; > > /* Configure the default exposure and gain. */ > - activeState.agc.gain = std::max(minAnalogueGain_, kMinAnalogueGain); > + activeState.agc.gain = minAnalogueGain_; > activeState.agc.exposure = 10ms / configuration.sensor.lineDuration; > > frameCount_ = 0;
diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp index b5309bdbea25..74a14675fca0 100644 --- a/src/ipa/ipu3/algorithms/agc.cpp +++ b/src/ipa/ipu3/algorithms/agc.cpp @@ -47,10 +47,6 @@ namespace ipa::ipu3::algorithms { LOG_DEFINE_CATEGORY(IPU3Agc) -/* Limits for analogue gain values */ -static constexpr double kMinAnalogueGain = 1.0; -static constexpr double kMaxAnalogueGain = 8.0; - /* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */ static constexpr utils::Duration kMaxShutterSpeed = 60ms; @@ -96,11 +92,11 @@ int Agc::configure(IPAContext &context, maxShutterSpeed_ = std::min(configuration.agc.maxShutterSpeed, kMaxShutterSpeed); - minAnalogueGain_ = std::max(configuration.agc.minAnalogueGain, kMinAnalogueGain); - maxAnalogueGain_ = std::min(configuration.agc.maxAnalogueGain, kMaxAnalogueGain); + minAnalogueGain_ = configuration.agc.minAnalogueGain; + maxAnalogueGain_ = configuration.agc.maxAnalogueGain; /* Configure the default exposure and gain. */ - activeState.agc.gain = std::max(minAnalogueGain_, kMinAnalogueGain); + activeState.agc.gain = minAnalogueGain_; activeState.agc.exposure = 10ms / configuration.sensor.lineDuration; frameCount_ = 0;
As the sensor's analogue gain range is known drop the arbitrary limits for the sensor analogue gain. Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- src/ipa/ipu3/algorithms/agc.cpp | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)