[libcamera-devel] [PATCH 2/9] libcamera: delayed_controls: Add helper for controls that applies with a delay
Jacopo Mondi
jacopo at jmondi.org
Tue Nov 10 12:53:33 CET 2020
Hi Niklas,
On Tue, Nov 10, 2020 at 12:26:47PM +0100, Niklas Söderlund wrote:
> 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.
>
Please note libcamera controls have a numeric id too
> >
> > > > +
> > > > + 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.
I was, at the contrary bothered by having to push an empy list, as it
feels like working against the API. But if pipeline handlers have to
do the same, then it's fine I guess
>
> >
> > > > +
> > > > + /* 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.
>
delays_.find(id) is equivalent to ctrls_[id] assuming delays_ and
ctrls_ contain the same set of controls
> >
> > > > +
> > > > + 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.
>
If it were a run-time error I could agree. But if this fail it means
the IPA (open or closed) has a bug.
Up to you
> >
> > > > +
> > > > + 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.
>
So this mean the caller must not go past [sequence - 16], right ? Is
it enforced ?
> >
> > > > + *
> > > > + * 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.
>
Ah right :(
> >
> > > > + * \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.
Ah yes, index give us "how far ahead" we have to pick the control
value, not which controls to set as I first thought!
Thanks
>
> >
> > > > + 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().
>
Ack!
Thanks for the explanation!
> >
> > 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