[libcamera-devel] [PATCH v1 2/3] ipa: raspberrypi: Populate SensorLimits
Naushir Patuck
naush at raspberrypi.com
Mon Mar 27 09:57:27 CEST 2023
Hi Jacopo and David,
On Sat, 25 Mar 2023 at 09:19, Jacopo Mondi <jacopo.mondi at ideasonboard.com>
wrote:
> 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.
>
This is correct. At runtime, only the max shutter value will be updated.
As
such, I see no reason not to move the SensorLimits fields into the
CameraMode
structure.
>
> 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 ?
>
This is somewhat by design. We try and keep our algorithm code as clean as
possible, i.e. mostly independent of libcamera dependencies like
ControlLists as
well as sensor dependencies like CamHelpers. The IPA code as a bridge
between
the algorithms and the rest of the world.
>
> 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
>
I'll explore the suggestion to move the fields to CameraMode, I think it's
probably worth it for convenience.
Regards,
Naush
> >
> > 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
> > >> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230327/165e5053/attachment.htm>
More information about the libcamera-devel
mailing list