[libcamera-devel] [PATCH v2 2/7] delayed_controls: Add user cookie to DelayedControls
Naushir Patuck
naush at raspberrypi.com
Mon Sep 5 11:32:27 CEST 2022
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.
>
> 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
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/20220905/2409a3e6/attachment.htm>
More information about the libcamera-devel
mailing list