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