<div dir="ltr"><div dir="ltr">Hi Jacopo,<div><br></div><div>Thank you for the feedback.</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 24 Mar 2023 at 14:23, Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com">jacopo.mondi@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush<br>
<br>
On Wed, Mar 22, 2023 at 04:13:16PM +0000, Naushir Patuck via libcamera-devel wrote:<br>
> Populate all the fields of the SensorLimits structure in configure().<br>
> This allows us to use the cached values instead of re-computing them<br>
> on every frame.<br>
><br>
> For the gain -> code convertion, ensure we clamp to the analogue gain<br>
> limits set in the SensorLimits structure.<br>
<br>
I wonder if this can't be done by expanding your CameraMode structure.<br>
CameraMode already combines information from the SensorInfo and could<br>
very well include information from sensorControls.<br>
<br>
This would avoid introducing another type to move it around across<br>
different layers of your IPA.<br>
<br>
Have you considered that and decided you prefer introducing<br>
SensorLimits ?<br></blockquote><div><br></div><div>That's a great idea! It definitely fits well in the CameraMode structure.<br>If there are no objection from David, I'll rework the series to move these<br>fields into CameraMode.<br><br>Regards,<br>Naush<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> ---<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 67 ++++++++++++++++-------------<br>
> 1 file changed, 38 insertions(+), 29 deletions(-)<br>
><br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index c3b2c375036f..e8d8023bcfe7 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -224,8 +224,8 @@ private:<br>
> Duration minFrameDuration_;<br>
> Duration maxFrameDuration_;<br>
><br>
> - /* Maximum gain code for the sensor. */<br>
> - uint32_t maxSensorGainCode_;<br>
> + /* Mode specific sensor gain/exposure limits. */<br>
> + RPiController::AgcAlgorithm::SensorLimits sensorLimits_;<br>
><br>
> /* Track the frame length times over FrameLengthsQueueSize frames. */<br>
> std::deque<Duration> frameLengths_;<br>
> @@ -439,8 +439,6 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip<br>
> }<br>
> }<br>
><br>
> - maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>();<br>
> -<br>
> /* Setup a metadata ControlList to output metadata. */<br>
> libcameraMetadata_ = ControlList(controls::controls);<br>
><br>
> @@ -473,6 +471,28 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip<br>
> /* Pass the camera mode to the CamHelper to setup algorithms. */<br>
> helper_->setCameraMode(mode_);<br>
><br>
> + /*<br>
> + * Store the sensor gain and shutter limits for the mode.<br>
> + *<br>
> + * The maximum shutter value are calculated from the frame duration limit<br>
> + * as V4L2 will restrict the maximum control value based on the current<br>
> + * VBLANK value.<br>
> + *<br>
> + * These limits get set in the AGC algorithm through applyFrameDurations()<br>
> + * below.<br>
> + */<br>
> + const ControlInfo &gainCtrl = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN);<br>
> + const ControlInfo &shutterCtrl = sensorCtrls_.at(V4L2_CID_EXPOSURE);<br>
> +<br>
> + sensorLimits_.minAnalogueGain = helper_->gain(gainCtrl.min().get<int32_t>());<br>
> + sensorLimits_.maxAnalogueGain = helper_->gain(gainCtrl.max().get<int32_t>());<br>
> + sensorLimits_.minFrameDuration = mode_.minFrameLength * mode_.minLineLength;<br>
> + sensorLimits_.maxFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;<br>
> + sensorLimits_.minShutter = helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength);<br>
> + sensorLimits_.maxShutter = Duration::max();<br>
> + helper_->getBlanking(sensorLimits_.maxShutter,<br>
> + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration);<br>
> +<br>
> /*<br>
> * Initialise this ControlList correctly, even if empty, in case the IPA is<br>
> * running is isolation mode (passing the ControlList through the IPC layer).<br>
> @@ -501,26 +521,17 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip<br>
> * based on the current sensor mode.<br>
> */<br>
> ControlInfoMap::Map ctrlMap = ipaControls;<br>
> - const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;<br>
> - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;<br>
> ctrlMap[&controls::FrameDurationLimits] =<br>
> - ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),<br>
> - static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));<br>
> + ControlInfo(static_cast<int64_t>(sensorLimits_.minFrameDuration.get<std::micro>()),<br>
> + static_cast<int64_t>(sensorLimits_.maxFrameDuration.get<std::micro>()));<br>
><br>
> ctrlMap[&controls::AnalogueGain] =<br>
> - ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_)));<br>
> -<br>
> - /*<br>
> - * Calculate the max exposure limit from the frame duration limit as V4L2<br>
> - * will limit the maximum control value based on the current VBLANK value.<br>
> - */<br>
> - Duration maxShutter = Duration::max();<br>
> - helper_->getBlanking(maxShutter, minSensorFrameDuration, maxSensorFrameDuration);<br>
> - const uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();<br>
> + ControlInfo(static_cast<float>(sensorLimits_.minAnalogueGain),<br>
> + static_cast<float>(sensorLimits_.maxAnalogueGain));<br>
><br>
> ctrlMap[&controls::ExposureTime] =<br>
> - ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin, mode_.minLineLength).get<std::micro>()),<br>
> - static_cast<int32_t>(maxShutter.get<std::micro>()));<br>
> + ControlInfo(static_cast<int32_t>(sensorLimits_.minShutter.get<std::micro>()),<br>
> + static_cast<int32_t>(sensorLimits_.maxShutter.get<std::micro>()));<br>
><br>
> /* Declare Autofocus controls, only if we have a controllable lens */<br>
> if (lensPresent_)<br>
> @@ -1480,9 +1491,6 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)<br>
><br>
> void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration)<br>
> {<br>
> - const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;<br>
> - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;<br>
> -<br>
> /*<br>
> * This will only be applied once AGC recalculations occur.<br>
> * The values may be clamped based on the sensor mode capabilities as well.<br>
> @@ -1490,9 +1498,9 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur<br>
> minFrameDuration_ = minFrameDuration ? minFrameDuration : defaultMaxFrameDuration;<br>
> maxFrameDuration_ = maxFrameDuration ? maxFrameDuration : defaultMinFrameDuration;<br>
> minFrameDuration_ = std::clamp(minFrameDuration_,<br>
> - minSensorFrameDuration, maxSensorFrameDuration);<br>
> + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration);<br>
> maxFrameDuration_ = std::clamp(maxFrameDuration_,<br>
> - minSensorFrameDuration, maxSensorFrameDuration);<br>
> + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration);<br>
> maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);<br>
><br>
> /* Return the validated limits via metadata. */<br>
> @@ -1505,17 +1513,18 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur<br>
> * getBlanking() will update maxShutter with the largest exposure<br>
> * value possible.<br>
> */<br>
> - RPiController::AgcAlgorithm::SensorLimits limits;<br>
> - limits.maxShutter = Duration::max();<br>
> - helper_->getBlanking(limits.maxShutter, minFrameDuration_, maxFrameDuration_);<br>
> + sensorLimits_.maxShutter = Duration::max();<br>
> + helper_->getBlanking(sensorLimits_.maxShutter, minFrameDuration_, maxFrameDuration_);<br>
><br>
> RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(<br>
> controller_.getAlgorithm("agc"));<br>
> - agc->setSensorLimits(limits);<br>
> + agc->setSensorLimits(sensorLimits_);<br>
> }<br>
><br>
> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)<br>
> {<br>
> + const int32_t minGainCode = helper_->gainCode(sensorLimits_.minAnalogueGain);<br>
> + const int32_t maxGainCode = helper_->gainCode(sensorLimits_.maxAnalogueGain);<br>
> int32_t gainCode = helper_->gainCode(agcStatus->analogueGain);<br>
><br>
> /*<br>
> @@ -1523,7 +1532,7 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)<br>
> * DelayedControls. The AGC will correctly handle a lower gain returned<br>
> * by the sensor, provided it knows the actual gain used.<br>
> */<br>
> - gainCode = std::min<int32_t>(gainCode, maxSensorGainCode_);<br>
> + gainCode = std::clamp<int32_t>(gainCode, minGainCode, maxGainCode);<br>
><br>
> /* getBlanking might clip exposure time to the fps limits. */<br>
> Duration exposure = agcStatus->shutterTime;<br>
> --<br>
> 2.34.1<br>
><br>
</blockquote></div></div>