<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
HI Naushir,<br>
<br>
<div class="moz-cite-prefix">On 9/5/22 3:02 PM, Naushir Patuck
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAEmqJPqUnZ6MDKsgqfURpLDgF7frx5vn50fptYS599T9eXwFhg@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<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"
moz-do-not-send="true" class="moz-txt-link-freetext">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>
<br>
Ah yes, I see that now<br>
<blockquote type="cite"
cite="mid:CAEmqJPqUnZ6MDKsgqfURpLDgF7frx5vn50fptYS599T9eXwFhg@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<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>
</div>
</blockquote>
<br>
This makes sense now. Thanks for the explanation.<br>
<br>
<blockquote type="cite"
cite="mid:CAEmqJPqUnZ6MDKsgqfURpLDgF7frx5vn50fptYS599T9eXwFhg@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<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"
moz-do-not-send="true" class="moz-txt-link-freetext">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>
</blockquote>
<br>
</body>
</html>