<div dir="ltr"><div dir="ltr">Hi Umang,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 5 Sept 2022 at 10:09, Umang Jain <<a href="mailto:umang.jain@ideasonboard.com">umang.jain@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 Naushir,<br>
<br>
Thank you for the patch. Few clarifications questions below...<br>
<br>
On 9/5/22 1:09 PM, Naushir Patuck via libcamera-devel wrote:<br>
> Allow the caller to provide a cookie value to DelayedControls::reset and<br>
> DelayedControls::push. This cookie value is returned from DelayedControls::get<br>
> for the frame that has the control values applied to it.<br>
><br>
> The cookie value is useful in tracking when a set of controls has been applied<br>
> to a frame. In a subsequent commit, it will be used by the Raspberry Pi IPA to<br>
> track the IPA context used when setting the control values.<br>
<br>
<br>
Do you mean that the latest controls applied on the sensor are <br>
represented by the cookie? Meaning the sequence parameter in <br>
DelayedControls::get(sequence) has no co-relation with cookie being <br>
returned?<br></blockquote><div><br></div><div>DelayedControls staggers sensor writes so that all controls in the ControlList</div><div>passed into push() are applied on the same frame, and the cookie is returned</div><div>out on that particular frame. In that sense, the cookie does have a relation</div><div>to the (future) frame sequence number.</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>
This brings me to a follow up question that, if <br>
DelayedControls::get(sequence) being called multiple times on same <br>
sequence number - does it mean the cookie returned, will get changed <br>
everytime? Will that be expected behavior?<br></blockquote><div><br></div><div>If you call get(sequence) multiple times with the same sequence number,</div><div>you will get back the same cookie value as the latter corresponds with</div><div>the frame controls, not the internal state of the DelayedControls. This of course</div><div>assumes that we don't overrun the DelayedControls queue of 16 values,</div><div>but that is not likely to happen...</div><div><br></div><div>Hope that makes sense, if not I'm happy to provide more details.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></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>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> include/libcamera/internal/delayed_controls.h | 8 +++++---<br>
> src/libcamera/delayed_controls.cpp | 17 +++++++++++------<br>
> src/libcamera/pipeline/ipu3/ipu3.cpp | 3 ++-<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-<br>
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 ++-<br>
> test/delayed_controls.cpp | 8 ++++----<br>
> 6 files changed, 25 insertions(+), 16 deletions(-)<br>
><br>
> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h<br>
> index f6e622f1dfba..fa222bb17d23 100644<br>
> --- a/include/libcamera/internal/delayed_controls.h<br>
> +++ b/include/libcamera/internal/delayed_controls.h<br>
> @@ -9,6 +9,7 @@<br>
> <br>
> #include <stdint.h><br>
> #include <unordered_map><br>
> +#include <utility><br>
> <br>
> #include <libcamera/controls.h><br>
> <br>
> @@ -27,10 +28,10 @@ public:<br>
> DelayedControls(V4L2Device *device,<br>
> const std::unordered_map<uint32_t, ControlParams> &controlParams);<br>
> <br>
> - void reset();<br>
> + void reset(unsigned int cookie = 0);<br>
> <br>
> - bool push(const ControlList &controls);<br>
> - ControlList get(uint32_t sequence);<br>
> + bool push(const ControlList &controls, unsigned int cookie = 0);<br>
> + std::pair<ControlList, unsigned int> get(uint32_t sequence);<br>
> <br>
> void applyControls(uint32_t sequence);<br>
> <br>
> @@ -77,6 +78,7 @@ private:<br>
> uint32_t writeCount_;<br>
> /* \todo Evaluate if we should index on ControlId * or unsigned int */<br>
> std::unordered_map<const ControlId *, ControlRingBuffer<Info>> values_;<br>
> + ControlRingBuffer<unsigned int> cookies_;<br>
<br>
Maybe some level of naming bikeshedding is required on <br>
"ControlRingBuffer", since now it's templated (1/7) But that can be later...<br>
> };<br>
> <br>
> } /* namespace libcamera */<br>
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp<br>
> index 777441e8222a..c2198d0e8d77 100644<br>
> --- a/src/libcamera/delayed_controls.cpp<br>
> +++ b/src/libcamera/delayed_controls.cpp<br>
> @@ -109,14 +109,16 @@ DelayedControls::DelayedControls(V4L2Device *device,<br>
> <br>
> /**<br>
> * \brief Reset state machine<br>
> + * \param[in] cookie User supplied reset cookie value<br>
> *<br>
> * Resets the state machine to a starting position based on control values<br>
> * retrieved from the device.<br>
> */<br>
> -void DelayedControls::reset()<br>
> +void DelayedControls::reset(unsigned int cookie)<br>
> {<br>
> queueCount_ = 1;<br>
> writeCount_ = 0;<br>
> + cookies_[0] = cookie;<br>
> <br>
> /* Retrieve control as reported by the device. */<br>
> std::vector<uint32_t> ids;<br>
> @@ -140,13 +142,14 @@ void DelayedControls::reset()<br>
> /**<br>
> * \brief Push a set of controls on the queue<br>
> * \param[in] controls List of controls to add to the device queue<br>
> + * \param[in] cookie User supplied cookie value for \a controls<br>
> *<br>
> * Push a set of controls to the control queue. This increases the control queue<br>
> * depth by one.<br>
> *<br>
<br>
Maybe its worth to clarify here that the cookie is returned back as part <br>
of ::get() mechanism, to know the controls have been applied.<br>
> * \returns true if \a controls are accepted, or false otherwise<br>
> */<br>
> -bool DelayedControls::push(const ControlList &controls)<br>
> +bool DelayedControls::push(const ControlList &controls, const unsigned int cookie)<br>
> {<br>
> /* Copy state from previous frame. */<br>
> for (auto &ctrl : values_) {<br>
> @@ -180,6 +183,7 @@ bool DelayedControls::push(const ControlList &controls)<br>
> << " at index " << queueCount_;<br>
> }<br>
> <br>
> + cookies_[queueCount_] = cookie;<br>
> queueCount_++;<br>
> <br>
> return true;<br>
> @@ -198,9 +202,10 @@ bool DelayedControls::push(const ControlList &controls)<br>
> * push(). The max history from the current sequence number that yields valid<br>
> * values are thus 16 minus number of controls pushed.<br>
> *<br>
> - * \return The controls at \a sequence number<br>
> + * \return The controls at \a sequence number and associated user supplied<br>
> + * cookie value.<br>
<br>
Is the sequence number of cookie returned, 'associated' ? I have asked a <br>
similar question above so apologies for repeating.<br>
<br>
My understanding is that the ControlList returned here corresponds to <br>
sequence parameter and cookie corresponds to a frame whose's controls <br>
have been applied on the sensor. The sequence's and cookie's frame <br>
number differ, no ? In that case, I would repharse it to:<br>
<br>
+ * \return The controls at \a sequence number and user supplied cookie<br>
+ * value representing the latest controls applied for it's associated<br>
+ * sequence.<br>
<br>
> */<br>
> -ControlList DelayedControls::get(uint32_t sequence)<br>
> +std::pair<ControlList, unsigned int> DelayedControls::get(uint32_t sequence)<br>
> {<br>
> unsigned int index = std::max<int>(0, sequence - maxDelay_);<br>
> <br>
> @@ -217,7 +222,7 @@ ControlList DelayedControls::get(uint32_t sequence)<br>
> << " at index " << index;<br>
> }<br>
> <br>
> - return out;<br>
> + return { out, cookies_[index] };<br>
> }<br>
> <br>
> /**<br>
> @@ -276,7 +281,7 @@ void DelayedControls::applyControls(uint32_t sequence)<br>
> while (writeCount_ > queueCount_) {<br>
> LOG(DelayedControls, Debug)<br>
> << "Queue is empty, auto queue no-op.";<br>
> - push({});<br>
> + push({}, cookies_[queueCount_ - 1]);<br>
> }<br>
> <br>
> device_->setControls(&out);<br>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> index 93219a6c1134..b0f0b22c5298 100644<br>
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> @@ -1385,7 +1385,8 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)<br>
> request->metadata().set(controls::SensorTimestamp,<br>
> buffer->metadata().timestamp);<br>
> <br>
> - info->effectiveSensorControls = delayedCtrls_->get(buffer->metadata().sequence);<br>
> + auto [controls, cookie] = delayedCtrls_->get(buffer->metadata().sequence);<br>
> + info->effectiveSensorControls = std::move(controls);<br>
> <br>
> if (request->findBuffer(&rawStream_))<br>
> pipe()->completeBuffer(request, buffer);<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index b4094898ca6c..d34a906c3cea 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -1848,7 +1848,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)<br>
> * Lookup the sensor controls used for this frame sequence from<br>
> * DelayedControl and queue them along with the frame buffer.<br>
> */<br>
> - ControlList ctrl = delayedCtrls_->get(buffer->metadata().sequence);<br>
> + auto [ctrl, cookie] = delayedCtrls_->get(buffer->metadata().sequence);<br>
<br>
I would grab this opportunity to rename s/ctrl/ctrls/<br>
> /*<br>
> * Add the frame timestamp to the ControlList for the IPA to use<br>
> * as it does not receive the FrameBuffer object.<br>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
> index 03bbe6b467ae..cec90810ff95 100644<br>
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
> @@ -1159,8 +1159,9 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)<br>
> if (data->frame_ <= buffer->metadata().sequence)<br>
> data->frame_ = buffer->metadata().sequence + 1;<br>
> <br>
> + auto [controls, cooke] = data->delayedCtrls_->get(buffer->metadata().sequence);<br>
<br>
s/cooke/cookie/<br>
> data->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),<br>
> - data->delayedCtrls_->get(buffer->metadata().sequence));<br>
> + controls);<br>
> }<br>
> <br>
> REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)<br>
> diff --git a/test/delayed_controls.cpp b/test/delayed_controls.cpp<br>
> index a8ce9828d73d..322c545998b2 100644<br>
> --- a/test/delayed_controls.cpp<br>
> +++ b/test/delayed_controls.cpp<br>
> @@ -96,7 +96,7 @@ protected:<br>
> <br>
> delayed->applyControls(i);<br>
> <br>
> - ControlList result = delayed->get(i);<br>
> + auto [result, cookie] = delayed->get(i);<br>
> int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();<br>
> if (brightness != value) {<br>
> cerr << "Failed single control without delay"<br>
> @@ -138,7 +138,7 @@ protected:<br>
> <br>
> delayed->applyControls(i);<br>
> <br>
> - ControlList result = delayed->get(i);<br>
> + auto [result, cookie] = delayed->get(i);<br>
> int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();<br>
> if (brightness != expected) {<br>
> cerr << "Failed single control with delay"<br>
> @@ -187,7 +187,7 @@ protected:<br>
> <br>
> delayed->applyControls(i);<br>
> <br>
> - ControlList result = delayed->get(i);<br>
> + auto [result, cookie] = delayed->get(i);<br>
> int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();<br>
> int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>();<br>
> if (brightness != expected || contrast != expected + 1) {<br>
> @@ -247,7 +247,7 @@ protected:<br>
> <br>
> delayed->applyControls(i);<br>
> <br>
> - ControlList result = delayed->get(i);<br>
> + auto [result, cookie] = delayed->get(i);<br>
> <br>
> int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();<br>
> int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>();<br>
<br>
</blockquote></div></div>