[libcamera-devel] [PATCH v2 2/7] delayed_controls: Add user cookie to DelayedControls
David Plowman
david.plowman at raspberrypi.com
Fri Sep 23 11:47:32 CEST 2022
Hi Naush
Thanks for the patch.
On Mon, 5 Sept 2022 at 08:40, Naushir Patuck via libcamera-devel
<libcamera-devel at lists.libcamera.org> 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.
>
> 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);
I guess my only question here is whether we're happy that we've caught
all the places affected by this API change. Maybe in code for other
platforms that we don't normally compile? A different API could have
made the change transparent, but if we're happy with this then so am
I!!
>
> 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_;
> };
>
> } /* 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.
> *
> * \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.
> */
> -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);
> /*
> * 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/ ? Though I guess no one is using it, but still...
With or without this very minor typo correction:
Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
Tested-by: David Plowman <david.plowman at raspberrypi.com>
(Tested on Raspberry Pi only.)
Thanks!
David
> 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>();
> --
> 2.25.1
>
More information about the libcamera-devel
mailing list