[libcamera-devel] [PATCH v1 2/3] ipa: raspberrypi: Populate SensorLimits
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Sat Mar 25 10:19:40 CET 2023
Hi David
On Fri, Mar 24, 2023 at 06:11:06PM +0000, David Plowman via libcamera-devel wrote:
> Hi Naush, Jacopo
>
> Just wanted to check whether there's no issue with these things
> changing dynamically (by which I mean not a mode-switch time). For
> example if someone sets a new framerate while things are running, or
> something.
Do I parse your questions right as:
The 'sensor limits' passed to the AGC does not only change at configure()
time, but also when a new FrameDurationLimits is set by an application.
Particularly, setting a new FrameDurationLimits limits the maximum
shutter time the AGC algorithm can use.
As I read it, maxShutter is the only parameter that changes
dynamically, all the rest of the structure remains un-changed between
configurations. That's why it feels a bit of a duplication of
CameraMode to me. This highlights that the maxShutter limit calculation
doesn't probably belong to the main IPA module but is rather part of
the AGC. The only other place where you use it is the ControlInfo
limits initialization, again at configure() time.
We handle cases like this in other IPA by having each algorithm implement
queueRequest() and handling the controls they're interested in. In
this way the state of an algorithm is as self-contained as possible,
and the main reason I'm not thrilled by this patch is that is
introduces yet-another state representation shared between multiple
layers. I understand that to do the shutter calculation in the algorithm
you would need to have access to the helper there, something you
currently have not. Is this by design ?
By the way your IPA is kind of "your territory" as you do most of the
maintainance there, so if this feels right/better to you, I won't
certainly oppose
>
> Thanks!
> David
>
> On Fri, 24 Mar 2023 at 14:54, Naushir Patuck via libcamera-devel
> <libcamera-devel at lists.libcamera.org> wrote:
> >
> > Hi Jacopo,
> >
> > Thank you for the feedback.
> >
> >
> > On Fri, 24 Mar 2023 at 14:23, Jacopo Mondi <jacopo.mondi at ideasonboard.com> wrote:
> >>
> >> Hi Naush
> >>
> >> On Wed, Mar 22, 2023 at 04:13:16PM +0000, Naushir Patuck via libcamera-devel wrote:
> >> > Populate all the fields of the SensorLimits structure in configure().
> >> > This allows us to use the cached values instead of re-computing them
> >> > on every frame.
> >> >
> >> > For the gain -> code convertion, ensure we clamp to the analogue gain
> >> > limits set in the SensorLimits structure.
> >>
> >> I wonder if this can't be done by expanding your CameraMode structure.
> >> CameraMode already combines information from the SensorInfo and could
> >> very well include information from sensorControls.
> >>
> >> This would avoid introducing another type to move it around across
> >> different layers of your IPA.
> >>
> >> Have you considered that and decided you prefer introducing
> >> SensorLimits ?
> >
> >
> > That's a great idea! It definitely fits well in the CameraMode structure.
> > If there are no objection from David, I'll rework the series to move these
> > fields into CameraMode.
> >
> > Regards,
> > Naush
> >
> >>
> >>
> >> >
> >> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> >> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> >> > ---
> >> > src/ipa/raspberrypi/raspberrypi.cpp | 67 ++++++++++++++++-------------
> >> > 1 file changed, 38 insertions(+), 29 deletions(-)
> >> >
> >> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> >> > index c3b2c375036f..e8d8023bcfe7 100644
> >> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> >> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> >> > @@ -224,8 +224,8 @@ private:
> >> > Duration minFrameDuration_;
> >> > Duration maxFrameDuration_;
> >> >
> >> > - /* Maximum gain code for the sensor. */
> >> > - uint32_t maxSensorGainCode_;
> >> > + /* Mode specific sensor gain/exposure limits. */
> >> > + RPiController::AgcAlgorithm::SensorLimits sensorLimits_;
> >> >
> >> > /* Track the frame length times over FrameLengthsQueueSize frames. */
> >> > std::deque<Duration> frameLengths_;
> >> > @@ -439,8 +439,6 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
> >> > }
> >> > }
> >> >
> >> > - maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>();
> >> > -
> >> > /* Setup a metadata ControlList to output metadata. */
> >> > libcameraMetadata_ = ControlList(controls::controls);
> >> >
> >> > @@ -473,6 +471,28 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
> >> > /* Pass the camera mode to the CamHelper to setup algorithms. */
> >> > helper_->setCameraMode(mode_);
> >> >
> >> > + /*
> >> > + * Store the sensor gain and shutter limits for the mode.
> >> > + *
> >> > + * The maximum shutter value are calculated from the frame duration limit
> >> > + * as V4L2 will restrict the maximum control value based on the current
> >> > + * VBLANK value.
> >> > + *
> >> > + * These limits get set in the AGC algorithm through applyFrameDurations()
> >> > + * below.
> >> > + */
> >> > + const ControlInfo &gainCtrl = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN);
> >> > + const ControlInfo &shutterCtrl = sensorCtrls_.at(V4L2_CID_EXPOSURE);
> >> > +
> >> > + sensorLimits_.minAnalogueGain = helper_->gain(gainCtrl.min().get<int32_t>());
> >> > + sensorLimits_.maxAnalogueGain = helper_->gain(gainCtrl.max().get<int32_t>());
> >> > + sensorLimits_.minFrameDuration = mode_.minFrameLength * mode_.minLineLength;
> >> > + sensorLimits_.maxFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;
> >> > + sensorLimits_.minShutter = helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength);
> >> > + sensorLimits_.maxShutter = Duration::max();
> >> > + helper_->getBlanking(sensorLimits_.maxShutter,
> >> > + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration);
> >> > +
> >> > /*
> >> > * Initialise this ControlList correctly, even if empty, in case the IPA is
> >> > * running is isolation mode (passing the ControlList through the IPC layer).
> >> > @@ -501,26 +521,17 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip
> >> > * based on the current sensor mode.
> >> > */
> >> > ControlInfoMap::Map ctrlMap = ipaControls;
> >> > - const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;
> >> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;
> >> > ctrlMap[&controls::FrameDurationLimits] =
> >> > - ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
> >> > - static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
> >> > + ControlInfo(static_cast<int64_t>(sensorLimits_.minFrameDuration.get<std::micro>()),
> >> > + static_cast<int64_t>(sensorLimits_.maxFrameDuration.get<std::micro>()));
> >> >
> >> > ctrlMap[&controls::AnalogueGain] =
> >> > - ControlInfo(1.0f, static_cast<float>(helper_->gain(maxSensorGainCode_)));
> >> > -
> >> > - /*
> >> > - * Calculate the max exposure limit from the frame duration limit as V4L2
> >> > - * will limit the maximum control value based on the current VBLANK value.
> >> > - */
> >> > - Duration maxShutter = Duration::max();
> >> > - helper_->getBlanking(maxShutter, minSensorFrameDuration, maxSensorFrameDuration);
> >> > - const uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();
> >> > + ControlInfo(static_cast<float>(sensorLimits_.minAnalogueGain),
> >> > + static_cast<float>(sensorLimits_.maxAnalogueGain));
> >> >
> >> > ctrlMap[&controls::ExposureTime] =
> >> > - ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin, mode_.minLineLength).get<std::micro>()),
> >> > - static_cast<int32_t>(maxShutter.get<std::micro>()));
> >> > + ControlInfo(static_cast<int32_t>(sensorLimits_.minShutter.get<std::micro>()),
> >> > + static_cast<int32_t>(sensorLimits_.maxShutter.get<std::micro>()));
> >> >
> >> > /* Declare Autofocus controls, only if we have a controllable lens */
> >> > if (lensPresent_)
> >> > @@ -1480,9 +1491,6 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
> >> >
> >> > void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration)
> >> > {
> >> > - const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;
> >> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;
> >> > -
> >> > /*
> >> > * This will only be applied once AGC recalculations occur.
> >> > * The values may be clamped based on the sensor mode capabilities as well.
> >> > @@ -1490,9 +1498,9 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur
> >> > minFrameDuration_ = minFrameDuration ? minFrameDuration : defaultMaxFrameDuration;
> >> > maxFrameDuration_ = maxFrameDuration ? maxFrameDuration : defaultMinFrameDuration;
> >> > minFrameDuration_ = std::clamp(minFrameDuration_,
> >> > - minSensorFrameDuration, maxSensorFrameDuration);
> >> > + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration);
> >> > maxFrameDuration_ = std::clamp(maxFrameDuration_,
> >> > - minSensorFrameDuration, maxSensorFrameDuration);
> >> > + sensorLimits_.minFrameDuration, sensorLimits_.maxFrameDuration);
> >> > maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);
> >> >
> >> > /* Return the validated limits via metadata. */
> >> > @@ -1505,17 +1513,18 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur
> >> > * getBlanking() will update maxShutter with the largest exposure
> >> > * value possible.
> >> > */
> >> > - RPiController::AgcAlgorithm::SensorLimits limits;
> >> > - limits.maxShutter = Duration::max();
> >> > - helper_->getBlanking(limits.maxShutter, minFrameDuration_, maxFrameDuration_);
> >> > + sensorLimits_.maxShutter = Duration::max();
> >> > + helper_->getBlanking(sensorLimits_.maxShutter, minFrameDuration_, maxFrameDuration_);
> >> >
> >> > RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> >> > controller_.getAlgorithm("agc"));
> >> > - agc->setSensorLimits(limits);
> >> > + agc->setSensorLimits(sensorLimits_);
> >> > }
> >> >
> >> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
> >> > {
> >> > + const int32_t minGainCode = helper_->gainCode(sensorLimits_.minAnalogueGain);
> >> > + const int32_t maxGainCode = helper_->gainCode(sensorLimits_.maxAnalogueGain);
> >> > int32_t gainCode = helper_->gainCode(agcStatus->analogueGain);
> >> >
> >> > /*
> >> > @@ -1523,7 +1532,7 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
> >> > * DelayedControls. The AGC will correctly handle a lower gain returned
> >> > * by the sensor, provided it knows the actual gain used.
> >> > */
> >> > - gainCode = std::min<int32_t>(gainCode, maxSensorGainCode_);
> >> > + gainCode = std::clamp<int32_t>(gainCode, minGainCode, maxGainCode);
> >> >
> >> > /* getBlanking might clip exposure time to the fps limits. */
> >> > Duration exposure = agcStatus->shutterTime;
> >> > --
> >> > 2.34.1
> >> >
More information about the libcamera-devel
mailing list