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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thu, Nov 26, 2020 at 06:38:59PM +0100, Jacopo Mondi wrote:
> On Mon, Nov 23, 2020 at 11:12:27PM +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

s/a optional/an optional/

> > 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.

I think that's a good design. If we ever have to emulate this using
timers, that can always be implement this as an extension (possibly in a
separate class whose purpose would be to generate the emulate 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.
> 
> Now tha the whole mechanism clicked for me (it only took three days
> after all) I mostly have comments on the 'cosmetic' part
> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > * 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 |  87 ++++++
> >  src/libcamera/delayed_controls.cpp            | 265 ++++++++++++++++++
> >  src/libcamera/meson.build                     |   1 +
> >  3 files changed, 353 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..e9cb0b90aac99847
> > --- /dev/null
> > +++ b/include/libcamera/internal/delayed_controls.h
> > @@ -0,0 +1,87 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> > + *
> > + * 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);

I could imagine this being useful for non-V4L2 devices too. We could
instead pass a ControlInfoMap to the constructor, and move the
setControls() call to the user of this class. There's no need to do so
now (but of course if you think it's a good idea, I'm not asking to
delay such rework :-)), but could you capture this in a \todo comment
(if you agree with the idea) ?

> > +
> > +	void reset(ControlList *controls = nullptr);
> > +
> > +	bool push(const ControlList &controls);
> > +	ControlList get(uint32_t sequence);
> 
> I liked the set/get pair that we had in StaggeredControls more
> 
> > +
> > +	void frameStart(uint32_t sequence);
> 
> I also think 'write' was probably nicer, but that's minor.
> I would have called this 'notifySOF()' but that's just my preference,
> so keep whatever you have here

applyControls() could also be an option. Or, if we decide to move
setControls() out of this class, we would need to make this function
return a ControlList.

> > +
> > +private:
> > +	class ControlInfo
> 
> As I've commented on v2, this is already the name of a library wide
> type.
> 
> To be honest I would drop the 'Control' suffix for all the internal
> types
> 
>         ControlInfo -> Info
>         ControlsDelays -> Delays
>         ControlsValues -> Values
> 
> They're internal after all (I know this goes both ways, but for sure
> they're shorter)

That could indeed avoid confusion.

> > +	{
> > +	public:
> > +		ControlInfo()
> > +			: updated(false)
> > +		{
> > +		}
> > +
> > +		ControlInfo(const ControlValue &v)
> > +			: value(v), updated(true)
> > +		{
> > +		}
> > +
> > +		ControlValue value;
> > +		bool updated;
> > +	};

You could also make this class inherit from ControlValue, to avoid the
explicit .value below.

> > +
> > +	static constexpr int listSize = 16;
> > +	class ControlArray : public std::array<ControlInfo, listSize>

How about a name that conveys this is a ring buffer, such as
ControlCircularArray, ControlRingBuffer, CircularArray, ... ? 

> > +	{
> > +	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>;

As these two types are only used once, below in the class declaration,
how about dropping the aliases ? Aliases are useful to avoid typing
types out explicitly in every location, but they have the drawback of
possibly obfuscating the code. I would then rename the above ControlInfo
to Value, as it stores a value.

I wonder if we should bundle the delay and array in a single class, to
have a single map (now or on top).

I also wonder if we shouldn't use the integer control ID as the key,
this would simplify DelayedControls::queue() that wouldn't have to look
up the ControlId pointer from the idmap.

> > +
> > +	bool queue(const ControlList &controls);
> > +
> > +	std::mutex lock_;

Do we need a lock, isn't this class used in a single thread ? If we want
to make it thread-safe, we should document this explicitly for each
function.

> > +
> > +	V4L2Device *device_;
> > +	ControlsDelays delays_;
> > +	unsigned int maxDelay_;
> > +
> > +	bool running_;
> > +	uint32_t firstSequence_;
> > +
> > +	uint32_t queueCount_;
> > +	uint32_t writeCount_;
> > +	ControlsValues ctrls_;
> 
> I would then call this 'values_'
> 
> > +};
> > +
> > +} /* 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..c810be48a36be515
> > --- /dev/null
> > +++ b/src/libcamera/delayed_controls.cpp
> > @@ -0,0 +1,265 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> > + *
> > + * 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"

This header...

> > +
> > +#include <libcamera/controls.h>
> > +
> > +#include "libcamera/internal/log.h"

... should go here.

> > +
> > +/**
> > + * \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 take effect with a delay
> > + *
> > + * Some sensor controls take effect with a delay as the sensor needs time to
> > + * adjust, for example exposure and focus. This is an optional helper class to

s/an optional helper/a helper/ ?

> > + * 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 need to maintain to also cover controls. Just as with buffer

s/need/needs/

> > + * 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 grates
> 
> 'greatest'
> 
> I find the reference to the buffer depth more confusing than helping.
> 
> > + * delay.
> > + */
> > +
> > +/**
> > + * \brief Construct a DelayedControls
>
>  \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 itControl = controls.find(delay.first);

As there's a single iterator in this function we usually use "it" or
"iter". Up to you.

> > +		if (itControl == 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 = itControl->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 and controls
> > + * \param[in] controls Optional list of controls to apply to the device
> > + *
> > + * Resets the state machine to a starting position based on control values
> > + * retrieved from the device. Controls may optionally be set before they are
> > + * read back using \a controls.
> > + */
> > +void DelayedControls::reset(ControlList *controls)
> > +{
> > +	std::lock_guard<std::mutex> lock(lock_);
> > +
> > +	running_ = false;
> > +	firstSequence_ = 0;
> > +	queueCount_ = 0;
> > +	writeCount_ = 0;
> > +
> > +	/* Set the controls on the device if requested. */
> > +	if (controls)
> > +		device_->setControls(controls);
> 
> Checking for the return value would guarantee Controls not supported
> by the device are not supplied and not erronously added to the ctrls_
> map below ?

Should this be moved to the caller ? Looking at how this is used, only
the Raspberry Pi pipeline handler calls this function with a non-null
controls parameter, to set initial control values at configure time. It
doesn't seem to really fit in the DelayedControls helper.

> > +
> > +	/* 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);

(This variable could then be called controls)

> > +
> > +	/* 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);

You use devCtrls.infoMap() here, and device_->controls() below. They
should be identical (or there's something I don't get :-)), so I think
the code would be easier to understand if we used one of the two
consistently.

> As .at() with an unsupported id would fail afaict
> 
> > +		ctrls_[id][queueCount_] = ControlInfo(ctrl.second);

Maybe s/queueCount/0/ to make this more explicit ?

> > +	}
> > +
> > +	queueCount_++;

You could then fold this above, making it queueCount_ = 1.

> > +}
> > +
> > +/**
> > + * \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)
> > +{
> > +	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 = ctrl.second[queueCount_];
> > +		info.value = ctrls_[ctrl.first][queueCount_ - 1].value;
> > +		info.updated = false;
> > +	}
> > +
> > +	/* Update with new controls. */
> > +	const ControlIdMap &idmap = device_->controls().idmap();
> > +	for (const auto &control : controls) {
> > +		const auto &itCtrl = idmap.find(control.first);

Same comment about the iterator name.

> > +		if (itCtrl == idmap.end())

Should a warning message be printed here ?

> > +			return false;
> > +
> > +		const ControlId *id = itCtrl->second;
> > +
> > +		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 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 to old sequence numbers that have been

s/to/too/

> > + * pushed out of the history.

What history size do we need in practice, do we need to go higher than
the maximum delay ? I wonder if we should make this dynamic (or just
lower, 16 seems fairly large). Making it dynamic could be done on top (a
\todo comment would then be nice).

> > + *
> > + * 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)
> > +{
> > +	std::lock_guard<std::mutex> lock(lock_);
> > +
> > +	uint32_t adjustedSeq = sequence - firstSequence_ + 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 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
> > + * exposure (SOE) V4L2 event.
> 
> This is all true, this function informs about an SOE events, but
> what's relevant is that in response to this event a new set of
> controls are written to the device. It is not mentioned anywhere in
> documentation.

Once we agree on the review comments (and a new version is posted to
address them if needed), I can help with the documentation if that would
be useful.

> > + */
> > +void DelayedControls::frameStart(uint32_t sequence)
> > +{
> > +	LOG(DelayedControls, Debug) << "frame " << sequence << " started";
> > +
> > +	std::lock_guard<std::mutex> lock(lock_);
> > +
> > +	if (!running_) {
> > +		firstSequence_ = 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);

Let's assume a sensor that only a single control, with a delay of 1
frame. It has been pre-programmed before stream start with an initial
value X with reset(). Then, two requests are queued before the first
SOF, with values A and B. push() is called twice, with A and B.
queueCount is thus 3 (initial value set to 1 in reset(), and increased
twice by push), and the queue contains X, A and B in positions 0, 1 and
2.

The first frame start arrives, frameStart(0) is called. writeCount_ is
0, maxDelay_ is 1, delays_[id] is 1. index will thus be 0, which causes
X to be written to the sensor.

Isn't that an incorrect behaviour ? Shouldn't B be written instead,
given that the value will take effect in frame 1, which will be captured
in request B ?

> > +		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({});
> 
> I would check at the beginning of the function if writeCount_ >
> queueCount_ and in the case exit immediately to avoid pushing a set of
> empty controls just to create a new slot. If I'm not mistaken it has
> the same effect ?
> 
> This would allow you merge queue() and push() in a single function (I
> still don't like queue({}) and still seems a bit of an hack :)
> 
> > +	}
> > +
> > +	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,

Laurent Pinchart