[libcamera-devel] [PATCH v4 2/7] delayed_controls: Add user cookie to DelayedControls
Naushir Patuck
naush at raspberrypi.com
Thu Oct 27 11:35:53 CEST 2022
Hi Kieran,
Thank you for the review!
On Wed, 26 Oct 2022 at 17:38, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:
> Quoting Naushir Patuck via libcamera-devel (2022-10-19 10:01:02)
> > 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.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > Tested-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> > include/libcamera/internal/delayed_controls.h | 8 +++++---
> > src/libcamera/delayed_controls.cpp | 20 ++++++++++++-------
> > 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, 27 insertions(+), 17 deletions(-)
> >
> > diff --git a/include/libcamera/internal/delayed_controls.h
> b/include/libcamera/internal/delayed_controls.h
> > index de8026e3e4f0..c7e79150afb8 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);
>
> The part that worries me here is that we /require/ always getting the
> cookie, but not setting it...
>
> I would feel safer if this wasn't defaulted to 0...
>
Sure, I'll remove the default values from push() and reset(), and make the
below
suggested changes to the ipu3 and rkisp pipeline handlers.
Regards,
Naush
>
> > + 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 *, RingBuffer<Info>> values_;
> > + RingBuffer<unsigned int> cookies_;
> > };
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/delayed_controls.cpp
> b/src/libcamera/delayed_controls.cpp
> > index 777441e8222a..3e7f60f9c421 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,15 @@ 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.
> > + * depth by one. The \a cookie value will be subsequently returned from
> > + * \a get() for the frame with all controls 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 +184,7 @@ bool DelayedControls::push(const ControlList
> &controls)
> > << " at index " << queueCount_;
> > }
> >
> > + cookies_[queueCount_] = cookie;
> > queueCount_++;
> >
> > return true;
> > @@ -198,9 +203,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.
> > */
> > -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 +223,7 @@ ControlList DelayedControls::get(uint32_t sequence)
> > << " at index " << index;
> > }
> >
> > - return out;
> > + return { out, cookies_[index] };
> > }
> >
> > /**
> > @@ -276,7 +282,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 3b892d9671c5..bf612089fdcb 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -1389,7 +1389,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);
>
> Which means this cookie is always zero.
>
> Could you also update the push call to set the cookie?
>
> That looks like it happens at
>
> void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,
> const ControlList &sensorControls,
> const ControlList &lensControls)
> {
> delayedCtrls_->push(sensorControls);
> ...
>
> Where the 'id' is actually the request sequence number in the current
> implementation.
>
> So we can remove [[maybe_unused]] from unsigned int id, and pass it to
> push().
>
> Ultimately, this then means that the cookie is the request sequence
> number of the request which is supposed to reflect these controls. Which
> is a nice way of tieing these together.
>
>
> > 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 343f8cb2c7ed..23f2460190f4 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1867,7 +1867,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);
> > /*
> > * 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 455ee2a0a711..20049d089472 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -1191,8 +1191,9 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer
> *buffer)
> > if (data->frame_ <= buffer->metadata().sequence)
> > data->frame_ = buffer->metadata().sequence + 1;
> >
> > + auto [controls, cookie] =
> data->delayedCtrls_->get(buffer->metadata().sequence);
>
> Same for rkisp1:
>
> void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int
> frame,
> const ControlList &sensorControls)
> {
> delayedCtrls_->push(sensorControls);
> }
>
> ... 'frame' should be a suitable/expected cookie here.
>
>
> > 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
>
> And in here, testing locally - then the pushes here need a cookie - and
> 'i' seems suitable.
>
> With that,
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > @@ -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>();
> > --
> > 2.25.1
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221027/28418f5a/attachment.htm>
More information about the libcamera-devel
mailing list