[libcamera-devel] [PATCH v2 2/3] ipa: raspberrypi: Better heruistics for calculating Unicam timeout
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Mar 7 11:01:43 CET 2023
Hi Naush,
Btw, I just noticed the "heruistics" typo in the subject line.
On Tue, Mar 07, 2023 at 08:45:44AM +0000, Naushir Patuck wrote:
> On Mon, 6 Mar 2023 at 17:25, Laurent Pinchart wrote:
>
> > Hi Naush,
> >
> > Thank you for the patch.
> >
> > On Thu, Mar 02, 2023 at 01:54:28PM +0000, Naushir Patuck via
> > libcamera-devel wrote:
> > >
> > > The existing mechanism of setting a timeout value simply uses the
> > > maximum possible frame length advertised by the sensor mode. This can be
> > > problematic when, for example, the IMX477 sensor can use a frame length
> > > of over 600 seconds. However, for typical usage the frame length will
> > > never go over several 100s of milliseconds, making the timeout very
> > > impractical.
> > >
> > > Store a list of the last 10 frame length values requested by the AGC. On
> > > startup, and at the end of every frame, take the maximum frame length
> > > value from this list and return that to the pipeline handler through the
> > > setCameraTimeoutValue() signal. This allows the timeout value to better
> > > track the actual sensor usage.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > > src/ipa/raspberrypi/raspberrypi.cpp | 44 +++++++++++++++++++++++++++--
> > > 1 file changed, 41 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index f6826bf27fe1..7b5aed644a67 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -8,6 +8,7 @@
> > > #include <algorithm>
> > > #include <array>
> > > #include <cstring>
> > > +#include <deque>
> > > #include <fcntl.h>
> > > #include <math.h>
> > > #include <stdint.h>
> > > @@ -64,6 +65,9 @@ using utils::Duration;
> > > /* Number of metadata objects available in the context list. */
> > > constexpr unsigned int numMetadataContexts = 16;
> > >
> > > +/* Number of frame length times to hold in the queue. */
> > > +constexpr unsigned int FrameLengthsQueueSize = 10;
> > > +
> > > /* Configure the sensor with these values initially. */
> > > constexpr double defaultAnalogueGain = 1.0;
> > > constexpr Duration defaultExposureTime = 20.0ms;
> > > @@ -121,7 +125,8 @@ class IPARPi : public IPARPiInterface
> > > public:
> > > IPARPi()
> > > : controller_(), frameCount_(0), checkCount_(0), mistrustCount_(0),
> > > - lastRunTimestamp_(0), lsTable_(nullptr), firstStart_(true)
> > > + lastRunTimestamp_(0), lsTable_(nullptr), firstStart_(true),
> > > + lastTimeout_(0s)
> > > {
> > > }
> > >
> > > @@ -155,6 +160,7 @@ private:
> > > void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);
> > > RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;
> > > void processStats(unsigned int bufferId, unsigned int ipaContext);
> > > + void setCameraTimeoutValue();
> > > void applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration);
> > > void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);
> > > void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);
> > > @@ -220,6 +226,10 @@ private:
> > >
> > > /* Maximum gain code for the sensor. */
> > > uint32_t maxSensorGainCode_;
> > > +
> > > + /* Track the frame length times over FrameLengthsQueueSize frames.
> > */
> > > + std::deque<Duration> frameLengths_;
> >
> > Another note to self: create a generic ring buffer class.
> >
> > > + Duration lastTimeout_;
> > > };
> > >
> > > int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)
> > > @@ -284,6 +294,10 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
> > >
> > > controller_.switchMode(mode_, &metadata);
> > >
> > > + /* Reset the frame lengths queue */
> >
> > s/queue/queue./
> >
> > > + frameLengths_.clear();
> > > + frameLengths_.resize(FrameLengthsQueueSize, 0s);
> >
> > Should you also reset lastTimeout_ to 0s to ensure a consistent
> > behaviour at start time ?
>
> Good catch, I will reset lastTimeout_ here.
>
> > > +
> > > /* SwitchMode may supply updated exposure/gain values to use. */
> > > AgcStatus agcStatus;
> > > agcStatus.shutterTime = 0.0s;
> > > @@ -294,6 +308,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
> > > ControlList ctrls(sensorCtrls_);
> > > applyAGC(&agcStatus, ctrls);
> > > startConfig->controls = std::move(ctrls);
> > > + setCameraTimeoutValue();
> > > }
> > >
> > > /*
> > > @@ -340,8 +355,6 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)
> > > }
> > >
> > > startConfig->dropFrameCount = dropFrameCount_;
> > > - const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;
> > > - setCameraTimeout.emit(maxSensorFrameDuration.get<std::milli>());
> > >
> > > firstStart_ = false;
> > > lastRunTimestamp_ = 0;
> > > @@ -1434,6 +1447,22 @@ void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)
> >
> > Adding more context:
> >
> > if (agcStatus.shutterTime && agcStatus.analogueGain) {
> > ControlList ctrls(sensorCtrls_);
> >
> > > applyAGC(&agcStatus, ctrls);
> > >
> > > setDelayedControls.emit(ctrls, ipaContext);
> > > + setCameraTimeoutValue();
> >
> > If the above condition is false, setCameraTimeoutValue() won't be
> > called. When can that happen, and could it cause any issue ?
>
> Short answer is no. Really the test is a bit redundant, agc can never ever
> return with agcStatus.shutterTime or agcStatus.analogueGain set to 0. I ought
> to remove that if () clause in a future tidy up.
That would be nice, thanks.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
with the other issues fixed.
> > > + }
> > > +}
> > > +
> > > +void IPARPi::setCameraTimeoutValue()
> > > +{
> > > + /*
> > > + * Take the maximum value of the exposure queue as the camera timeout
> > > + * value to pass back to the pipeline handler. Only signal if it has changed
> >
> > Line wrap.
> >
> > > + * from the last set value.
> > > + */
> > > + auto max = std::max_element(frameLengths_.begin(), frameLengths_.end());
> > > +
> > > + if (*max != lastTimeout_) {
> > > + setCameraTimeout.emit(max->get<std::milli>());
> > > + lastTimeout_ = *max;
> > > }
> > > }
> > >
> > > @@ -1522,6 +1551,15 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
> > > */
> > > if (mode_.minLineLength != mode_.maxLineLength)
> > > ctrls.set(V4L2_CID_HBLANK, static_cast<int32_t>(hblank));
> > > +
> > > + /*
> > > + * Store the frame length times in a circular queue, up-to FrameLengthsQueueSize
> > > + * elements. This will be used to advertise a camera timeout value to the
> > > + * pipeline handler.
> >
> > Line wraps here too.
> >
> > > + */
> > > + frameLengths_.pop_front();
> > > + frameLengths_.push_back(helper_->exposure(vblank + mode_.height,
> > > + helper_->hblankToLineLength(hblank)));
> > > }
> > >
> > > void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list