[libcamera-devel] [PATCH v2 11/13] ipa: ipu3: agc: Remove condition on exposure correction
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Oct 21 04:35:39 CEST 2021
Hi Jean-Michel,
Thank you for the patch.
On Wed, Oct 20, 2021 at 05:46:05PM +0200, Jean-Michel Hautbois wrote:
> Until now, we can't know when the exposure and gains applied in the
> IPAIPU3::setControls() are really applied (it can be several frames). We
> don't want to use the values calculated as if they are already applied,
> and this is done by testing the frame number.
>
> When the exposure is estimated, we verify if it changed enough for
> exposure and gain to be updated.
>
> There is no need for that because we are now filtering the value with
> the previous one correctly, so if the change is very small the exposure
> and analogue gain my evolve a bit but it should not be visible to the
> user.
Repeating my comment from v1,
I fail to see why a small change won't change the exposure and analogue
gain. As far as I understand, this removes a hysteresis, which I believe
will cause oscillations.
I'd rather work on dropping the kFrameSkipCount and using the correct
exposure time and gain values.
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> src/ipa/ipu3/algorithms/agc.cpp | 78 ++++++++++++++++-----------------
> 1 file changed, 37 insertions(+), 41 deletions(-)
>
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 19f3a420..0417dc99 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -138,53 +138,49 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
> if ((frameCount_ < kInitialFrameMinAECount) || (frameCount_ - lastFrame_ < kFrameSkipCount))
> return;
>
> - /* Are we correctly exposed ? */
> - if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {
> - LOG(IPU3Agc, Debug) << "!!! Good exposure with iqMean = " << iqMean_;
> - } else {
> - double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
> + double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
>
> - /* extracted from Rpi::Agc::computeTargetExposure */
> - utils::Duration currentShutter = exposure * lineDuration_;
> - currentExposureNoDg_ = currentShutter * analogueGain;
> - LOG(IPU3Agc, Debug) << "Actual total exposure " << currentExposureNoDg_
> - << " Shutter speed " << currentShutter
> - << " Gain " << analogueGain
> - << " Needed ev gain " << evGain;
> + /* extracted from Rpi::Agc::computeTargetExposure */
> + utils::Duration currentShutter = exposure * lineDuration_;
> + currentExposureNoDg_ = currentShutter * analogueGain;
> + LOG(IPU3Agc, Debug) << "Actual total exposure " << currentExposureNoDg_
> + << " Shutter speed " << currentShutter
> + << " Gain " << analogueGain
> + << " Needed ev gain " << evGain;
>
> - currentExposure_ = prevExposureValue_ * evGain;
> - utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;
> - utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;
> + currentExposure_ = prevExposureValue_ * evGain;
> + utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;
> + utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;
>
> - utils::Duration maxTotalExposure = maxShutterSpeed * kMaxGain;
> - currentExposure_ = std::min(currentExposure_, maxTotalExposure);
> - LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
> - << ", maximum is " << maxTotalExposure;
> + utils::Duration maxTotalExposure = maxShutterSpeed * kMaxGain;
> + currentExposure_ = std::min(currentExposure_, maxTotalExposure);
> + LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
> + << ", maximum is " << maxTotalExposure;
>
> - /* \todo: estimate if we need to desaturate */
> - filterExposure();
> + /* \todo: estimate if we need to desaturate */
> + filterExposure();
>
> - utils::Duration exposureValue = filteredExposure_;
> - utils::Duration shutterTime = minShutterSpeed;
> + utils::Duration exposureValue = filteredExposure_;
> + utils::Duration shutterTime = minShutterSpeed;
> +
> + /*
> + * Push the shutter time up to the maximum first, and only then
> + * increase the gain.
> + */
> + shutterTime = std::clamp<utils::Duration>(exposureValue / kMinGain,
> + minShutterSpeed, maxShutterSpeed);
> + double stepGain = std::clamp(exposureValue / shutterTime,
> + kMinGain, kMaxGain);
> + LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
> + << shutterTime << " and "
> + << stepGain;
> +
> + exposure = shutterTime / lineDuration_;
> + analogueGain = stepGain;
> +
> + /* Update the exposure value for the next process call */
> + prevExposureValue_ = shutterTime * analogueGain;
>
> - /*
> - * Push the shutter time up to the maximum first, and only then
> - * increase the gain.
> - */
> - shutterTime = std::clamp<utils::Duration>(exposureValue / kMinGain,
> - minShutterSpeed, maxShutterSpeed);
> - double stepGain = std::clamp(exposureValue / shutterTime,
> - kMinGain, kMaxGain);
> - LOG(IPU3Agc, Debug) << "Divided up shutter and gain are "
> - << shutterTime << " and "
> - << stepGain;
> -
> - exposure = shutterTime / lineDuration_;
> - analogueGain = stepGain;
> -
> - /* Update the exposure value for the next process call */
> - prevExposureValue_ = shutterTime * analogueGain;
> - }
> lastFrame_ = frameCount_;
> }
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list