[libcamera-devel] [PATCH v4 1/8] libcamera: delayed_controls: Add helper for controls that applies with a delay
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Jan 10 17:07:16 CET 2021
Hi Niklas,
Thank you for the patch.
In the subject line, s/applies/apply/
On Tue, Dec 15, 2020 at 01:48:04AM +0100, 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 thru V4L2 events.
s/thru/through/
> 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>
> ---
> * Changes since v2
> - 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 | 82 ++++++
> src/libcamera/delayed_controls.cpp | 252 ++++++++++++++++++
> src/libcamera/meson.build | 1 +
> 3 files changed, 335 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..1292b484ec9f53e9
> --- /dev/null
> +++ b/include/libcamera/internal/delayed_controls.h
> @@ -0,0 +1,82 @@
> +/* 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 <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've sent the following comment on v3:
> 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) ?
I see neither a rework of the API, nor a \todo comment. Is that an
oversight, or did you disagree with the idea ? In the latter case, a
reply to the review would be nice.
> +
> + void reset();
> +
> + bool push(const ControlList &controls);
> + ControlList get(uint32_t sequence);
> +
> + void applyControls(uint32_t sequence);
> +
> +private:
> + class Info
> + {
> + public:
> + Info()
> + : updated(false)
> + {
> + }
> +
> + Info(const ControlValue &v)
> + : value(v), updated(true)
> + {
> + }
> +
> + ControlValue value;
> + bool updated;
> + };
Same here, v3 had the following review comment:
> You could also make this class inherit from ControlValue, to avoid the
> explicit .value below.
> +
> + /* \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);
> + }
> + };
There were further comments in the review of v3 that have also not been
addressed.
> +
> + bool queue(const ControlList &controls);
> +
> + V4L2Device *device_;
> + std::unordered_map<const ControlId *, unsigned int> delays_;
> + unsigned int maxDelay_;
> +
> + bool running_;
> + uint32_t firstSequence_;
> +
> + uint32_t queueCount_;
> + uint32_t writeCount_;
> + std::unordered_map<const ControlId *, ControlRingBuffer> 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..db2e51f8c93c4755
> --- /dev/null
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -0,0 +1,252 @@
> +/* 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/controls.h>
> +
> +#include "libcamera/internal/log.h"
> +#include "libcamera/internal/v4l2_device.h"
> +
> +/**
> + * \file delayed_controls.h
> + * \brief Helper to deal with controls that are applied with a delay
s/are applied/take effect/ (matching the documentation of the class
below)
> + */
> +
> +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 a helper class to deal with
Focus is a different beast. Let's s/focus/analog gain/, that's a better
example.
> + * 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.
This is a bit terse, and so is the \class documentation. Reading the
documentation only, not the code, it would be quite difficult to use
this class :-S
> + */
> +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)
> +{
> + return queue(controls);
> +}
> +
> +bool DelayedControls::queue(const ControlList &controls)
> +{
> + /* Copy state from previous frame. */
> + for (auto &ctrl : values_) {
> + Info &info = ctrl.second[queueCount_];
> + info.value = values_[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 &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.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 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_) {
> + const ControlId *id = ctrl.first;
> + const Info &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.
> + */
> +void DelayedControls::applyControls(uint32_t sequence)
> +{
> + LOG(DelayedControls, Debug) << "frame " << sequence << " started";
> +
> + 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 : values_) {
> + const ControlId *id = ctrl.first;
> + unsigned int delayDiff = maxDelay_ - delays_[id];
> + unsigned int index = std::max<int>(0, writeCount_ - delayDiff);
A reply to the v3 review would also be appreciated for the issue pointed
our regarding this code. I still can't see the implementation being
correct :-S
> + const Info &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 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
More information about the libcamera-devel
mailing list