[libcamera-devel] [PATCH v5 1/8] libcamera: delayed_controls: Add helper for controls that apply with a delay

[Kieran Bingham]

Hi Niklas,

On 16/01/2021 11:47, Niklas Söderlund wrote:
> Some sensor controls take effect with a delay as the sensor needs time
> to adjust, for example exposure. Add an optional helper DelayedControls
> to help pipelines deal with such controls.
> 
> The idea is to provide a queue of controls towards the V4L2 device and
> apply individual controls with the specified delay with the aim to get
> predictable and retrievable control values for any given frame. To do
> this the queue of controls needs to be at least as deep as the control
> with the largest delay.
> 
> The DelayedControls needs to be informed of every start of exposure.
> This can be emulated but the helper is designed to be used with this
> event being provide by the kernel through V4L2 events.
> 
> This helper is based on StaggeredCtrl from the Raspberry Pi pipeline
> handler but expands on its API. This helpers aims to replace the
> Raspberry Pi implementations and mimics it behavior perfectly.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
> ---
> * Changes since v4
> - Fold queue() into push() as the mutex was dropped in v3 having a
>   public accessors that takes the mutex is no longer needed.
> - Add delayed_controls.h to meson.build.
> - Update patch subject and commit message.
> - Have class Info inherit from ControlValue.
> - Add todo to reevaluate if maps should be indexed on ControlID or
>   unsigned int.
> - Update documentation.
> 
> * Changes since v3
> - Drop optional argument to reset().
> - Update commit message.
> - Remove usage of Mutex.
> - Rename frameStart() to applyControls)_.
> - Rename ControlInfo to Into.
> - Rename ControlArray to ControlRingBuffer.
> - Drop ControlsDelays and ControlsValues.
> - Sort headers.
> - Rename iterators.
> - Simplify queueCount_ handeling in reset().
> - Add more warnings.
> - Update documentation.
> 
> * Changes since v2
> - Improve error logic in queue() as suggested by Jean-Michel Hautbois.
> - s/fistSequence_/firstSequence_/
> 
> * Changes since v1
> - Correct copyright to reflect work is derived from Raspberry Pi
>   pipeline handler. This was always the intention and was wrong in v1.
> - Rewrite large parts of the documentation.
> - Join two loops to one in DelayedControls::DelayedControls()
> ---
>  include/libcamera/internal/delayed_controls.h |  81 ++++++
>  include/libcamera/internal/meson.build        |   1 +
>  src/libcamera/delayed_controls.cpp            | 247 ++++++++++++++++++
>  src/libcamera/meson.build                     |   1 +
>  4 files changed, 330 insertions(+)
>  create mode 100644 include/libcamera/internal/delayed_controls.h
>  create mode 100644 src/libcamera/delayed_controls.cpp
> 
> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h
> new file mode 100644
> index 0000000000000000..dc447a882514182f
> --- /dev/null
> +++ b/include/libcamera/internal/delayed_controls.h
> @@ -0,0 +1,81 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> + *
> + * delayed_controls.h - Helper to deal with controls that take effect with a delay
> + */
> +#ifndef __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__
> +#define __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__
> +
> +#include <stdint.h>
> +#include <unordered_map>
> +
> +#include <libcamera/controls.h>
> +
> +namespace libcamera {
> +
> +class V4L2Device;
> +
> +class DelayedControls
> +{
> +public:
> +	DelayedControls(V4L2Device *device,
> +			const std::unordered_map<uint32_t, unsigned int> &delays);
> +
> +	void reset();
> +
> +	bool push(const ControlList &controls);
> +	ControlList get(uint32_t sequence);
> +
> +	void applyControls(uint32_t sequence);
> +
> +private:
> +	class Info : public ControlValue
> +	{
> +	public:
> +		Info()
> +			: updated(false)
> +		{
> +		}
> +
> +		Info(const ControlValue &v)
> +			: ControlValue(v), updated(true)
> +		{
> +		}
> +
> +		bool updated;
> +	};
> +
> +	/* \todo: Make the listSize configurable at instance creation time. */
> +	static constexpr int listSize = 16;
> +	class ControlRingBuffer : public std::array<Info, listSize>
> +	{
> +	public:
> +		Info &operator[](unsigned int index)
> +		{
> +			return std::array<Info, listSize>::operator[](index % listSize);
> +		}
> +
> +		const Info &operator[](unsigned int index) const
> +		{
> +			return std::array<Info, listSize>::operator[](index % listSize);
> +		}
> +	};
> +
> +	V4L2Device *device_;
> +	/* \todo Evaluate if we should index on ControlId * or unsigned int */
> +	std::unordered_map<const ControlId *, unsigned int> delays_;
> +	unsigned int maxDelay_;
> +
> +	bool running_;
> +	uint32_t firstSequence_;
> +
> +	uint32_t queueCount_;
> +	uint32_t writeCount_;
> +	/* \todo Evaluate if we should index on ControlId * or unsigned int */
> +	std::unordered_map<const ControlId *, ControlRingBuffer> values_;

Oh, so we have a ring buffer for every control. That sort of surprises
me a little.

Would a ring buffer of ControlLists make sense? Or are they more
difficult to instantiate? (That wouldn't surprise me ;D)


> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__ */
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 1b1bdc77951235a5..e67a359fb8656f71 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -17,6 +17,7 @@ libcamera_internal_headers = files([
>      'camera_sensor.h',
>      'control_serializer.h',
>      'control_validator.h',
> +    'delayed_controls.h',
>      'device_enumerator.h',
>      'device_enumerator_sysfs.h',
>      'device_enumerator_udev.h',
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> new file mode 100644
> index 0000000000000000..f24db43d7f1e357d
> --- /dev/null
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -0,0 +1,247 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> + *
> + * delayed_controls.h - Helper to deal with controls that take effect with a delay
> + */
> +
> +#include "libcamera/internal/delayed_controls.h"
> +
> +#include <libcamera/controls.h>
> +
> +#include "libcamera/internal/log.h"
> +#include "libcamera/internal/v4l2_device.h"
> +
> +/**
> + * \file delayed_controls.h
> + * \brief Helper to deal with controls that take effect with a delay
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(DelayedControls)
> +
> +/**
> + * \class DelayedControls
> + * \brief Helper to deal with controls that take effect with a delay
> + *
> + * Some sensor controls take effect with a delay as the sensor needs time to
> + * adjust, for example exposure and analog gain. This is a helper class to deal
> + * with such controls and the intended users are pipeline handlers.
> + *
> + * The idea is to extend the concept of the buffer depth of a pipeline the
> + * application needs to maintain to also cover controls. Just as with buffer
> + * depth if the application keeps the number of requests queued above the
> + * control depth the controls are guaranteed to take effect for the correct
> + * request. The control depth is determined by the control with the greatest
> + * delay.
> + */
> +
> +/**
> + * \brief Construct a DelayedControls instance
> + * \param[in] device The V4L2 device the controls have to be applied to
> + * \param[in] delays Map of the numerical V4L2 control ids to their associated
> + * delays (in frames)
> + *
> + * Only controls specified in \a delays are handled. If it's desired to mix
> + * delayed controls and controls that take effect immediately the immediate
> + * controls must be listed in the \a delays map with a delay value of 0.
> + */
> +DelayedControls::DelayedControls(V4L2Device *device,
> +				 const std::unordered_map<uint32_t, unsigned int> &delays)
> +	: device_(device), maxDelay_(0)
> +{
> +	const ControlInfoMap &controls = device_->controls();
> +
> +	/*
> +	 * Create a map of control ids to delays for controls exposed by the
> +	 * device.
> +	 */
> +	for (auto const &delay : delays) {
> +		auto it = controls.find(delay.first);
> +		if (it == controls.end()) {
> +			LOG(DelayedControls, Error)
> +				<< "Delay request for control id "
> +				<< utils::hex(delay.first)
> +				<< " but control is not exposed by device "
> +				<< device_->deviceNode();
> +			continue;
> +		}
> +
> +		const ControlId *id = it->first;
> +
> +		delays_[id] = delay.second;
> +
> +		LOG(DelayedControls, Debug)
> +			<< "Set a delay of " << delays_[id]
> +			<< " for " << id->name();
> +
> +		maxDelay_ = std::max(maxDelay_, delays_[id]);
> +	}
> +
> +	reset();
> +}
> +
> +/**
> + * \brief Reset state machine
> + *
> + * Resets the state machine to a starting position based on control values
> + * retrieved from the device.
> + */
> +void DelayedControls::reset()
> +{
> +	running_ = false;
> +	firstSequence_ = 0;
> +	queueCount_ = 1;
> +	writeCount_ = 0;
> +
> +	/* Retrieve control as reported by the device. */
> +	std::vector<uint32_t> ids;
> +	for (auto const &delay : delays_)
> +		ids.push_back(delay.first->id());
> +
> +	ControlList controls = device_->getControls(ids);
> +
> +	/* Seed the control queue with the controls reported by the device. */
> +	values_.clear();
> +	for (const auto &ctrl : controls) {
> +		const ControlId *id = device_->controls().idmap().at(ctrl.first);
> +		values_[id][0] = Info(ctrl.second);
> +	}
> +}
> +
> +/**
> + * \brief Push a set of controls on the queue
> + * \param[in] controls List of controls to add to the device queue
> + *
> + * 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)
> +{
> +	/* Copy state from previous frame. */
> +	for (auto &ctrl : values_) {
> +		Info &info = ctrl.second[queueCount_];
> +		info = values_[ctrl.first][queueCount_ - 1];
> +		info.updated = false;
> +	}
> +
> +	/* Update with new controls. */
> +	const ControlIdMap &idmap = device_->controls().idmap();
> +	for (const auto &control : controls) {
> +		const auto &it = idmap.find(control.first);
> +		if (it == idmap.end()) {
> +			LOG(DelayedControls, Warning)
> +				<< "Unknown control " << control.first;
> +			return false;
> +		}
> +
> +		const ControlId *id = it->second;
> +
> +		if (delays_.find(id) == delays_.end())
> +			return false;
> +
> +		Info &info = values_[id][queueCount_];
> +
> +		info = control.second;
> +		info.updated = true;
> +
> +		LOG(DelayedControls, Debug)
> +			<< "Queuing " << id->name()
> +			<< " to " << info.toString()
> +			<< " at index " << queueCount_;
> +	}
> +
> +	queueCount_++;
> +
> +	return true;
> +}
> +
> +/**
> + * \brief Read back controls in effect at a sequence number
> + * \param[in] sequence The sequence number to get controls for
> + *
> + * Read back what controls where in effect at a specific sequence number. The
> + * history is a ring buffer of 16 entries where new and old values coexist. It's
> + * the callers responsibility to not read too old sequence numbers that have been
> + * pushed out of the history.
> + *
> + * Historic values are evicted by pushing new values onto the queue using
> + * 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
> + */
> +ControlList DelayedControls::get(uint32_t sequence)
> +{
> +	uint32_t adjustedSeq = sequence - firstSequence_ + 1;
> +	unsigned int index = std::max<int>(0, adjustedSeq - maxDelay_);
> +
> +	ControlList out(device_->controls());
> +	for (const auto &ctrl : values_) {

I think I was a bit surprised to see that we have a ring per control, so
I now see that this function is taking all the controls in parallel ring
buffers of a specific sequence and generating a list from them.

That makes me think even more that the ControlList could be the item on
the RingBuffer in the first place - but the list would have to be reset
when 'removed' from the Ring I expect.

Perhaps that's as 'simple' as a std::move() operation though.



> +		const ControlId *id = ctrl.first;
> +		const Info &info = ctrl.second[index];
> +
> +		out.set(id->id(), info);
> +
> +		LOG(DelayedControls, Debug)
> +			<< "Reading " << id->name()
> +			<< " to " << info.toString()
> +			<< " at index " << index;
> +	}
> +
> +	return out;
> +}
> +
> +/**
> + * \brief Inform DelayedControls of the start of a new frame
> + * \param[in] sequence Sequence number of the frame that started
> + *
> + * Inform the state machine that a new frame has started and of its sequence
> + * number. Any user of these helpers is responsible to inform the helper about
> + * the start of any frame.This can be connected with ease to the start of a

s/frame.This/frame. This/

> + * exposure (SOE) V4L2 event.

This doesn't feel very clear to me.

This function 'writes' controls to the device, and

> + */
> +void DelayedControls::applyControls(uint32_t sequence)
> +{

If this is occurring as a callback from the SOE v4L2 event, should there
be locking in here to protect variables such as writeCount_ or queueCount_ ?


> +	LOG(DelayedControls, Debug) << "frame " << sequence << " started";
> +
> +	if (!running_) {
> +		firstSequence_ = sequence;
> +		running_ = true;
> +	}
> +
> +	/*
> +	 * Create control list peaking ahead in the value queue to ensure

s/peaking/peeking/ ?


> +	 * values are set in time to satisfy the sensor delay.
> +	 */
> +	ControlList out(device_->controls());
> +	for (const auto &ctrl : values_) {
> +		const ControlId *id = ctrl.first;
> +		unsigned int delayDiff = maxDelay_ - delays_[id];

Aha - actually - seeing that each control has different delays probably
changes my mind about storing a full ControlList in the ring buffer....



> +		unsigned int index = std::max<int>(0, writeCount_ - delayDiff);
> +		const Info &info = ctrl.second[index];
> +
> +		if (info.updated) {
> +			out.set(id->id(), info);
> +			LOG(DelayedControls, Debug)
> +				<< "Setting " << id->name()
> +				<< " to " << info.toString()
> +				<< " at index " << index;
> +		}
> +	}
> +
> +	writeCount_++;
> +
> +	while (writeCount_ >= queueCount_) {
> +		LOG(DelayedControls, Debug)
> +			<< "Queue is empty, auto queue no-op.";
> +		push({});
> +	}
> +
> +	device_->setControls(&out);
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 387d5d88ecae11ad..5a4bf0d7ba4fd231 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -12,6 +12,7 @@ libcamera_sources = files([
>      'controls.cpp',
>      'control_serializer.cpp',
>      'control_validator.cpp',
> +    'delayed_controls.cpp',
>      'device_enumerator.cpp',
>      'device_enumerator_sysfs.cpp',
>      'event_dispatcher.cpp',
> 

-- 
Regards
--
Kieran