[libcamera-devel] [PATCH v2 2/7] delayed_controls: Add user cookie to DelayedControls
Umang Jain
umang.jain at ideasonboard.com
Mon Sep 5 11:09:13 CEST 2022
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?
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?
> 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>();
More information about the libcamera-devel
mailing list