[libcamera-devel] [PATCH 2/9] libcamera: delayed_controls: Add helper for controls that applies with a delay
Niklas Söderlund
niklas.soderlund at ragnatech.se
Tue Nov 10 12:26:47 CET 2020
Hi Jacopo,
Thanks for your feedback.
On 2020-11-05 21:08:48 +0100, Jacopo Mondi wrote:
> Hi again Niklas,
>
> On Wed, Nov 04, 2020 at 05:08:00PM +0100, Jacopo Mondi wrote:
> > 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>;
>
> Do you think indexing on ControlId is a good idea ? I see quite some
> churn below to get from the integer id obtained by iterating a
> ControlList to the ControlId used to index these maps:
>
> const ControlId *id = device_->controls().idmap().at(control.first);
>
> While from ControlId we can always easily get the id back
I think it's worth it as we try to stay away form using V4L2 identifiers
for as long as possible. As a bonus we get access to the string name of
the control for the logging.
>
> > > +
> > > + 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.
> >
> > > + */
> > > +
>
> double empy line
>
> > > +
> > > +/**
> > > + * \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//
> >
>
> Actually I meant to make this
> Sanity check that all requested controls are exposed by the device
>
> > > + * 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]);
> > > + }
>
> I wonder if these loops should not be unified. After all, if a
> device's control is not in the delay map it is skipped, so it is the
> same as iterating the delay map as done in the previous loop ?
They should and are in v2.
>
> > > +
> > > + 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).
>
> I don't get this last statement
>
> > > + *
> > > + * 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.
>
> I would drop the last sentence
>
> > > + */
> > > +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.
>
> control queue depth really confuses me.
>
> Isn't it better to just say:
> "Push a new set of values for the controls"
> or something similar ?
>
> > > + *
> > > + * \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_];
>
> Am I wrong of
> for (auto &ctrl : ctrls_) {
> ControlInfo &info = ctrls_[ctrl.first][queueCount_];
>
> Is equivalent to
> for (auto &ctrl : ctrls_) {
> ControlInfo &info = ctrl.second[queueCount_];
>
> (the compiler agrees afaict)
It does, thanks for this so much nicer to read!
>
> > > + info.value = ctrls_[ctrl.first][queueCount_ - 1].value;
> > > + info.updated = false;
> > > + }
>
> Have you considered breaking this in 2 ? This would avoid the below
> queue({}) which seems a workaround.
>
> Can't we:
> advance() {
> copy values from previous entry
> queueCount++;
> }
>
> update() {
> copy the new values in;
> }
>
> So that below you only call advance() ?
I like the notion of one having to queue an empty list as it fits the
concept of this helper. Then maybe we can optimize it further if an
empty set is queue. Maybe this could be a future improvement as this
will only effect the internal API of this class anyhow. And we still
need to allow for pipelines to que empty controls list to keep in sync.
>
> > > +
> > > + /* 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_];
>
> That's a double lookup that can be avoided. If I'm not mistaken all
> controls in delays_ are inserted in ctrls_, am I wrong ? If that's the
> case you can just ctrls_.find() == ctls_.end() ?
I'm not sure I get this one.
>
> > > +
> > > + info.value = control.second;
> > > + info.updated = true;
> > > +
> > > + LOG(DelayedControls, Debug)
> > > + << "Queuing " << id->name()
> > > + << " to " << info.value.toString()
> > > + << " at index " << queueCount_;
> > > + }
> > > +
> > > + queueCount_++;
>
> Ah then you have to backtrack on queueCount if any of the supplied
> Control is not supported. That's anyway an error that should happen
> only during development, if a distracted pipeline handler mis-uses
> this class, applications have no way to get it wrong, right ?
For OSS IPA yes but we also need to think about closed source IPAs.
>
> In this case, wouldn't a LOG(Fatal) help to catch development errors
> earlier ? If you agree you can safely queueCount++, because if we
> fail, the whole library gets torn down.
Interesting, I'm not sure which one I like. I'm slightly prefering this
approach I think as then it will be a pipeline handler decision if to
try and recover or fatally fail.
>
> > > +
> > > + return true;
>
> I've not looked ahead in the series, but is there value in this return
> value ? I mean, if a pipeline handler gets this wrong is an
> implementation error, not a runtime one, right ?
As stated above I think it's better to let the pipeline handler judge
how to best handle this case, but I might be wrong.
>
> > > +}
> > > +
> > > +/**
> > > + * \brief Read back controls in effect at a specific sequence number
>
> Read back the controls' values at a specific point in time (or
> sequence number as you've used) ?
>
> > > + * \param[in] sequence Sequence number to get read back controls for
>
> The sequence number used to read controls values
>
> > > + *
> > > + * Read back what controls where in effect at a specific sequence number. The
>
> Maybe it's me, but I cannot parse well "controls where in effect".
> Native speakers to the rescue ? I would anyway use "controls' values"
> here and in other places.
>
> > > + * 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.
>
> Can we invalidate them with a flag maybe ?
We can't as they are overwritten and reused as it's cyclic buffer.
>
> > > + *
> > > + * 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.
>
> This looks like class documentation I was asking for :)
>
> > > + *
> > > + * \returns List of controls in effect at \a sequence
>
> As per above suggestion
> \return The values of controls at \a sequence time
>
> > > + */
> > > +ControlList DelayedControls::get(uint32_t sequence)
> > > +{
> > > + std::lock_guard<std::mutex> lock(lock_);
> > > +
> > > + uint32_t adjustedSeq = sequence - fistSequence_ + 1;
>
> Can it happen that sequence < firstSequence ?
Yes, when sequence wraps around 32 bits. There is a specific test for
this in the next patch.
>
> > > + unsigned int index = std::max<int>(0, adjustedSeq - maxDelay_);
> > > +
> > > + ControlList out(device_->controls());
>
> You access device_->controls() quite often, can it be cached ?
>
> > > + 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
>
> Maybe I'm missing some piece that will come in the next patches, but
> we have the video device, right ? Can't we connect to its frameStart
> signal and handle this internally ?
Yes and no :-)
This is intended to be connected to frameStart() but unfortunately it's
not the sensors V4L2Device that emits the SOE event but the CSI-2
recivers. So the connection between the two needs to be done in the
pipeline handler.
>
> > > + * \param[in] sequence Sequence number of the frame that started
> > > + *
> > > + * Inform the state machine that a new frame have started and it's sequence
>
> s/have/has
> s/it's/its
>
> Seems like a verb for "sequence number is missing"
>
> Inform the state machine that a new frame has started and its sequence
> number is \a 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.
>
> It's responsibility of the users of this class to notify the start of
> every frame.
> (I would not mention V4L2, but I'm might be too concerned, it's fine
> if you like it)
>
> > > + */
> > > +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);
>
> It's me but I don't get this logic as I didn't get it in
> StaggeredControls. It was there already, so I'm sure it's correct, but
> I don't get it :)
>
> Why is maxDelay_ relevant ?
It's used to calculate how far in advanced a control needs to be set,
it's basically the control depth. If Exposure needs 2 frames to take
effect and Gain 1, we have a depth of 2. So in when frame 100 start we
need to set the Exposure for 102 and Gain for 101.
For exposure,
delayDiff = maxDelay_ - delays_[Exposure] = 2 - 2 = 0
For gain
delayDiff = maxDelay_ - delays_[Gain] = 2 - 1 = 1
So the controls we set are gain for the frame+1 and exposure for
frame+2.
>
> > > + 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);
>
> Any value in returning this ? This come from the device, something
> might be happening there and maybe pipelines want to be informed ?
We can't return anything from this as this functinon is meant to be
connected to frameStart().
>
> Thanks
> j
>
> > > +}
> > > +
> > > +} /* 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
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list