<div dir="ltr"><div dir="ltr">Hi Jacopo and David,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, 25 Mar 2023 at 09:19, 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 David<br>
<br>
On Fri, Mar 24, 2023 at 06:11:06PM +0000, David Plowman via libcamera-devel wrote:<br>
> Hi Naush, Jacopo<br>
><br>
> Just wanted to check whether there's no issue with these things<br>
> changing dynamically (by which I mean not a mode-switch time). For<br>
> example if someone sets a new framerate while things are running, or<br>
> something.<br>
<br>
Do I parse your questions right as:<br>
<br>
The 'sensor limits' passed to the AGC does not only change at configure()<br>
time, but also when a new FrameDurationLimits is set by an application.<br>
Particularly, setting a new FrameDurationLimits limits the maximum<br>
shutter time the AGC algorithm can use.<br>
<br>
As I read it, maxShutter is the only parameter that changes<br>
dynamically, all the rest of the structure remains un-changed between<br>
configurations. That's why it feels a bit of a duplication of<br>
CameraMode to me. This highlights that the maxShutter limit calculation<br>
doesn't probably belong to the main IPA module but is rather part of<br>
the AGC. The only other place where you use it is the ControlInfo<br>
limits initialization, again at configure() time.<br></blockquote><div><br></div><div>This is correct. At runtime, only the max shutter value will be updated. As<br>such, I see no reason not to move the SensorLimits fields into the CameraMode<br>structure.<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>
We handle cases like this in other IPA by having each algorithm implement<br>
queueRequest() and handling the controls they're interested in. In<br>
this way the state of an algorithm is as self-contained as possible,<br>
and the main reason I'm not thrilled by this patch is that is<br>
introduces yet-another state representation shared between multiple<br>
layers. I understand that to do the shutter calculation in the algorithm<br>
you would need to have access to the helper there, something you<br>
currently have not. Is this by design ?<br></blockquote><div><br></div><div>This is somewhat by design. We try and keep our algorithm code as clean as<br>possible, i.e. mostly independent of libcamera dependencies like ControlLists as<br>well as sensor dependencies like CamHelpers. The IPA code as a bridge between<br>the algorithms and the rest of the world.<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>
By the way your IPA is kind of "your territory" as you do most of the<br>
maintainance there, so if this feels right/better to you, I won't<br>
certainly oppose<br></blockquote><div><br></div><div>I'll explore the suggestion to move the fields to CameraMode, I think it's<br>probably worth it for convenience.<br></div><div><br></div><div>Regards,</div><div>Naush</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>
> Thanks!<br>
> David<br>
><br>
> On Fri, 24 Mar 2023 at 14:54, Naushir Patuck via libcamera-devel<br>
> <<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a>> wrote:<br>
> ><br>
> > Hi Jacopo,<br>
> ><br>
> > Thank you for the feedback.<br>
> ><br>
> ><br>
> > On Fri, 24 Mar 2023 at 14:23, Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>> wrote:<br>
> >><br>
> >> 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>
> ><br>
> ><br>
> > 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>
> ><br>
> >><br>
> >><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>