| Message ID | mailman.57.1677765273.25031.libcamera-devel@lists.libcamera.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush On Thu, Mar 02, 2023 at 01:54:28PM +0000, Naushir Patuck via libcamera-devel wrote: > Date: Thu, 2 Mar 2023 13:54:28 +0000 > From: Naushir Patuck <naush@raspberrypi.com> > To: libcamera-devel@lists.libcamera.org > Subject: [PATCH v2 2/3] ipa: raspberrypi: Better heruistics for calculating > Unicam timeout > X-Mailer: git-send-email 2.34.1 > > 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@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.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_; > + 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 */ > + frameLengths_.clear(); > + frameLengths_.resize(FrameLengthsQueueSize, 0s); > + > /* 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) > applyAGC(&agcStatus, ctrls); > > setDelayedControls.emit(ctrls, ipaContext); > + setCameraTimeoutValue(); > + } > +} > + > +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 > + * 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. > + */ > + frameLengths_.pop_front(); > + frameLengths_.push_back(helper_->exposure(vblank + mode_.height, > + helper_->hblankToLineLength(hblank))); > } > > void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls) > -- > 2.34.1 >
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@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@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 ? > + > /* 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 ? > + } > +} > + > +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)
Hi Laurent, Thank you for your feedback. On Mon, 6 Mar 2023 at 17:25, Laurent Pinchart < laurent.pinchart@ideasonboard.com> 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@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@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. Regards, Naush > > > + } > > +} > > + > > +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 >
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@raspberrypi.com> > > > Reviewed-by: David Plowman <david.plowman@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@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)
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_; + 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 */ + frameLengths_.clear(); + frameLengths_.resize(FrameLengthsQueueSize, 0s); + /* 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) applyAGC(&agcStatus, ctrls); setDelayedControls.emit(ctrls, ipaContext); + setCameraTimeoutValue(); + } +} + +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 + * 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. + */ + frameLengths_.pop_front(); + frameLengths_.push_back(helper_->exposure(vblank + mode_.height, + helper_->hblankToLineLength(hblank))); } void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)