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