<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>