[libcamera-devel] [PATCH 2/9] libcamera: delayed_controls: Add helper for controls that applies with a delay

[Jacopo Mondi]

Hi Niklas,

On Wed, Oct 28, 2020 at 02:00:44AM +0100, Niklas Söderlund wrote:
> Some sensor controls take effect with a delay as the sensor needs time
> to adjust, for example exposure. Add a 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 thru V4L2 events.

s/thru/though ?
>
> 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>
> ---
>  include/libcamera/internal/delayed_controls.h |  87 ++++++
>  src/libcamera/delayed_controls.cpp            | 282 ++++++++++++++++++
>  src/libcamera/meson.build                     |   1 +
>  3 files changed, 370 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..df5520d240a54e4b
> --- /dev/null
> +++ b/include/libcamera/internal/delayed_controls.h
> @@ -0,0 +1,87 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * delayed_controls.h - Helper to deal with controls that are applied with a delay
> + */
> +#ifndef __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__
> +#define __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__
> +
> +#include <mutex>
> +#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(ControlList *controls = nullptr);
> +
> +	bool push(const ControlList &controls);
> +	ControlList get(uint32_t sequence);
> +
> +	void frameStart(uint32_t sequence);
> +
> +private:
> +	class ControlInfo

With no other scope but public:, is it worth a class ?

> +	{
> +	public:
> +		ControlInfo()
> +			: updated(false)
> +		{
> +		}
> +
> +		ControlInfo(const ControlValue &v)
> +			: value(v), updated(true)
> +		{
> +		}
> +
> +		ControlValue value;
> +		bool updated;
> +	};
> +
> +	static constexpr int listSize = 16;
> +	class ControlArray : public std::array<ControlInfo, listSize>
> +	{
> +	public:
> +		ControlInfo &operator[](unsigned int index)
> +		{
> +			return std::array<ControlInfo, listSize>::operator[](index % listSize);
> +		}
> +
> +		const ControlInfo &operator[](unsigned int index) const
> +		{
> +			return std::array<ControlInfo, listSize>::operator[](index % listSize);
> +		}
> +	};
> +
> +	using ControlsDelays = std::unordered_map<const ControlId *, unsigned int>;
> +	using ControlsValues = std::unordered_map<const ControlId *, ControlArray>;
> +
> +	bool queue(const ControlList &controls);
> +
> +	std::mutex lock_;
> +
> +	V4L2Device *device_;
> +	ControlsDelays delays_;
> +	unsigned int maxDelay_;
> +
> +	bool running_;
> +	uint32_t fistSequence_;

Should this be 'first' ?

> +
> +	uint32_t queueCount_;
> +	uint32_t writeCount_;
> +	ControlsValues ctrls_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__ */
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> new file mode 100644
> index 0000000000000000..0e32f417c5cc68b7
> --- /dev/null
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -0,0 +1,282 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * delayed_controls.h - Helper to deal with controls that are applied with a delay
> + */
> +
> +#include "libcamera/internal/delayed_controls.h"
> +#include "libcamera/internal/v4l2_device.h"
> +
> +#include <libcamera/controls.h>
> +
> +#include "libcamera/internal/log.h"
> +
> +/**
> + * \file delayed_controls.h
> + * \brief Helper to deal with controls that are applied with a delay
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(DelayedControls)
> +
> +/**
> + * \class DelayedControls
> + * \brief Helper to deal with controls that takes effect with a delay

controls -> take

> + *
> + * Some sensor controls take effect with a delay as the sensor needs time to

controls -> take

> + * adjust, for example exposure and focus. This is an optional helper class to
> + * deal with such controls and the intended user is pipeline handlers.

users are pipeline handlers

> + *
> + * The idea is to extend the concept of the pipeline depth the users needs to
> + * maintain for buffers to controls. The depth is determined with by the control
> + * with the grates delay. As long as the pipeline keeps the control queue above
> + * this level the control values are guaranteed to be in effect at the specified
> + * point in time.

I would drop this as it mentions other concepts that might be more
confusing than helping.

> + *
> + * The downside is of course that the pipeline needs to know what controls to
> + * set control depth frames in advance. But there really is no way around this
> + * as the delay is a consequence of the physical world. Compare this with
> + * parameter buffers where the buffer may be queued to the device while it's
> + * still being written to as long as it's ready before it's consumed, this is
> + * because the parameter buffer (usually) does not contain controls that
> + * requires time to take effect.

I find this confusing too, I'm sorry.

I think the key concept is that controls have to be applied in
advance, the usage of "Delayed" took me a bit off-road as a
DelayedControls class seems to be designed to apply controls at a
later (delayed) point in time while it's actually the other way
around. One key point is that for each Control the pipeline handler
has to provide a known delay value, something I didn't realize until I
looked at the code. The class takes care of applying the control the
control in -advance- enough to make sure the control is fully applied
at the time the Request it is associated to completes.

In the class description I would describe what the class offers and
how it has to be used, its API and how I should set
controls specifying their 'delay'. Design documentation is good but
only to complement usage documentation.

> + */
> +
> +
> +/**
> + * \brief Construct a DelayedControls
> + * \param[in] device The V4L2 device containing the delayed controls

The V4L2 device the controls have to be applied to

> + * \param[in] delays Map of numerical V4L2 control id to its delay to take
> + * effect in frames

isn't 'latency' better than delay ? Or any other term ? See how heavy
is the description you had to make of this parameter...

\param[in] delays Map of V4L2 control ids and their associated latencies (in frames)

?
> + *
> + * Only controls specified in \a delays are handled by the DelayedControls
> + * instance. If it's desired to mix delayed controls and controls that takes
> + * effect immediately the immediate controls must be listed in the \a delays map
> + * with a delay value of 0.

delays is a map, you have to provide a delay value by design

"Controls with an associated 0 delay (latency) are applied immediately"


> + */
> +DelayedControls::DelayedControls(V4L2Device *device,
> +				 const std::unordered_map<uint32_t, unsigned int> &delays)
> +	: device_(device), maxDelay_(0)
> +{
> +	const ControlInfoMap &controls = device_->controls();
> +
> +	/*
> +	 * Sanity check that all controls where delays are requested are

s/where delays//

> +	 * exposed byt the device.

by

> +	 */
> +	for (auto const &delay : delays) {
> +		unsigned int id = delay.first;
> +
> +		if (controls.find(id) == controls.end())
> +			LOG(DelayedControls, Error)
> +				<< "Delay request for control id "
> +				<< utils::hex(id)
> +				<< " but control is not exposed by device "
> +				<< device_->deviceNode();
> +	}
> +
> +	/*
> +	 * Create a map of control to delay for all controls exposed by the

controls to delays

> +	 * device. If no delay is specified assume the control applies directly.

assume a 0 delay (latency)

> +	 */
> +	for (auto const &control : controls) {
> +		const ControlId *id = control.first;
> +
> +		auto it = delays.find(id->id());
> +		if (it == delays.end())
> +			continue;
> +
> +		delays_[id] = it->second;
> +
> +		LOG(DelayedControls, Debug)
> +			<< "Set a delay of " << delays_[id]
> +			<< " for " << id->name();
> +
> +		maxDelay_ = std::max(maxDelay_, delays_[id]);
> +	}
> +
> +	reset();
> +}
> +
> +/**
> + * \brief Reset the V4L2 device

Reset controls to their default values ?

> + * \param[in] controls List of controls to reset the device to or nullptr

Optional list of controls to apply to the device ?

These is mostly on documentation, I'll look at implementation later!

Thanks
  j


> + *
> + * Resets the delayed controls state machine to its starting state. All controls
> + * are fetched from the V4L2 device to provide a good starting point for the
> + * first frames (length of control depth).
> + *
> + * Optionally \a controls can be specified to set some or all of the handled
> + * V4L2 controls prior to reading them back. If no controls needs to be set
> + * nullptr may be used.
> + */
> +void DelayedControls::reset(ControlList *controls)
> +{
> +	std::lock_guard<std::mutex> lock(lock_);
> +
> +	running_ = false;
> +	fistSequence_ = 0;
> +	queueCount_ = 0;
> +	writeCount_ = 0;
> +
> +	/* Set the controls on the device if requested. */
> +	if (controls)
> +		device_->setControls(controls);
> +
> +	/* Retrieve current control values reported by the device. */
> +	std::vector<uint32_t> ids;
> +	for (auto const &delay : delays_)
> +		ids.push_back(delay.first->id());
> +
> +	ControlList devCtrls = device_->getControls(ids);
> +
> +	/* Seed the control queue with the controls reported by the device. */
> +	ctrls_.clear();
> +	for (const auto &ctrl : devCtrls) {
> +		const ControlId *id = devCtrls.infoMap()->idmap().at(ctrl.first);
> +		ctrls_[id][queueCount_] = ControlInfo(ctrl.second);
> +	}
> +
> +	queueCount_++;
> +}
> +
> +/**
> + * \brief Push a set of controls on the queue
> + * \param[in] controls List of controls to add 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)
> +{
> +	std::lock_guard<std::mutex> lock(lock_);
> +
> +	return queue(controls);
> +}
> +
> +bool DelayedControls::queue(const ControlList &controls)
> +{
> +	/* Copy state from previous frame. */
> +	for (auto &ctrl : ctrls_) {
> +		ControlInfo &info = ctrls_[ctrl.first][queueCount_];
> +		info.value = ctrls_[ctrl.first][queueCount_ - 1].value;
> +		info.updated = false;
> +	}
> +
> +	/* Update with new controls. */
> +	for (const auto &control : controls) {
> +		const ControlId *id = device_->controls().idmap().at(control.first);
> +
> +		if (delays_.find(id) == delays_.end())
> +			return false;
> +
> +		ControlInfo &info = ctrls_[id][queueCount_];
> +
> +		info.value = control.second;
> +		info.updated = true;
> +
> +		LOG(DelayedControls, Debug)
> +			<< "Queuing " << id->name()
> +			<< " to " << info.value.toString()
> +			<< " at index " << queueCount_;
> +	}
> +
> +	queueCount_++;
> +
> +	return true;
> +}
> +
> +/**
> + * \brief Read back controls in effect at a specific sequence number
> + * \param[in] sequence Sequence number to get read back 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 to 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.
> + *
> + * \returns List of controls in effect at \a sequence
> + */
> +ControlList DelayedControls::get(uint32_t sequence)
> +{
> +	std::lock_guard<std::mutex> lock(lock_);
> +
> +	uint32_t adjustedSeq = sequence - fistSequence_ + 1;
> +	unsigned int index = std::max<int>(0, adjustedSeq - maxDelay_);
> +
> +	ControlList out(device_->controls());
> +	for (const auto &ctrl : ctrls_) {
> +		const ControlId *id = ctrl.first;
> +		const ControlInfo &info = ctrl.second[index];
> +
> +		out.set(id->id(), info.value);
> +
> +		LOG(DelayedControls, Debug)
> +			<< "Reading " << id->name()
> +			<< " to " << info.value.toString()
> +			<< " at index " << index;
> +	}
> +
> +	return out;
> +}
> +
> +/**
> + * \brief Inform DelayedControls of a start of a new frame
> + * \param[in] sequence Sequence number of the frame that started
> + *
> + * Inform the state machine that a new frame have started and it's sequence
> + * number. It's user of this helpers responsibility to inform the helper
> + * at the start of every frame. This can with ease be connected to the start
> + * of exposure (SOE) V4L2 event.
> + */
> +void DelayedControls::frameStart(uint32_t sequence)
> +{
> +	LOG(DelayedControls, Debug) << "frame " << sequence << " started";
> +
> +	std::lock_guard<std::mutex> lock(lock_);
> +
> +	if (!running_) {
> +		fistSequence_ = sequence;
> +		running_ = true;
> +	}
> +
> +	/*
> +	 * Create control list peaking ahead in the value queue to ensure
> +	 * values are set in time to satisfy the sensor delay.
> +	 */
> +	ControlList out(device_->controls());
> +	for (const auto &ctrl : ctrls_) {
> +		const ControlId *id = ctrl.first;
> +		unsigned int delayDiff = maxDelay_ - delays_[id];
> +		unsigned int index = std::max<int>(0, writeCount_ - delayDiff);
> +		const ControlInfo &info = ctrl.second[index];
> +
> +		if (info.updated) {
> +			out.set(id->id(), info.value);
> +			LOG(DelayedControls, Debug)
> +				<< "Setting " << id->name()
> +				<< " to " << info.value.toString()
> +				<< " at index " << index;
> +		}
> +	}
> +
> +	writeCount_++;
> +
> +	while (writeCount_ >= queueCount_) {
> +		LOG(DelayedControls, Debug)
> +			<< "Queue is empty, auto queue no-op.";
> +		queue({});
> +	}
> +
> +	device_->setControls(&out);
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 07711b5f93bcc921..19f22f9c94e1d64d 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',
> --
> 2.29.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel