[libcamera-devel] [PATCH v4 2/7] delayed_controls: Add user cookie to DelayedControls
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Oct 26 18:38:11 CEST 2022
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...
> + 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
>
More information about the libcamera-devel
mailing list