[PATCH v2 06/12] libcamera: delayed_controls: Rework delayed controls implementation
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Mar 21 11:52:20 CET 2024
Quoting Jacopo Mondi (2024-03-21 09:20:34)
> Hi Stefan
>
> On Mon, Mar 18, 2024 at 02:22:36PM +0100, Stefan Klug wrote:
> > Hi Jacopo,
> >
> > thank you for the review. Sorry for all the style issues. I'll fix them
> > asap.
> >
> > On Fri, Mar 15, 2024 at 05:15:58PM +0100, Jacopo Mondi wrote:
> > > Hi Stefan
> > >
> > > Not going to review the logic implementation in detail, it will take
> > > me some more time as delayed controls are kind of a complex beast, so
> > > this will be mostly a stylistic review with some stupid questions here
> > > and there
> > >
> > > On Wed, Mar 13, 2024 at 01:12:17PM +0100, Stefan Klug wrote:
> > > > Functional changes:
> > > > - Requests need to be queued for every frame
> > >
> > > Do you mean it is a requirement for the application to queue Request
> > > for every frame ?
> >
> > Yes, that's actually no change. The reason why I wrote that, is that in
> > the old delayed controls tests, there was always a call to
> > applyControls() before the first call to push().
> >
>
> Just to clarify, is this a requirement for PFC to happen, or in
> general ? (see below)
>
> > >
> > > > - The startup phase is no longer treated as special case
> > > > - Requests carry a sequence number, so that we can detect when the system
> > > > gets out of sync
> > >
> > > Are you referring to the request sequence number or the frame number ?
> >
> > Are there actually two different groups of numbers? If yes, I miss something
> > conceptually here. The seuence number is meant to be as in "This is the
> > request with seq x, which needs to be applied for frame x"
> >
>
> I guess this is one of the thing we should clarify first before going
> further in the design.
>
> Both Request and frames have a sequence_number.
>
> Requests are queued to the camera by the application. Their sequence
> number increments for each Request created by the Camera. There are no
> requirments on the timing the application queues requests to the
> camera, one example is RPi Zero users that due to limited memory only
> seldom queue a Request
>
> Frames are produced by the sensor at a fixed time interval, and their
> sequence number increments at every frame produced by the sensor (or better,
> received by the CSI-2 rx, as sometimes frame gets dropped and gaps are
> created, anyway..)
>
> If an application wants full per-frame control, it needs to queue
> enough requests to compensate for the pipeline depth which counts for:
>
> - a full frame time as sensor's settings apply to the 'next' frame, hence
> the one that is getting exposed right now is 'gone'
> - on some ISP some frames of delay to generate statistics (iirc IPU3
> has 1 frame delay between the frame that gets processed and the one
> for which statistics are generated)
> - the sensor's maximum control delay
>
> A reasonable number (to be used as an example here) for the pipeline
> depth is something between 4 and 6 frames.
>
> Now, if an application wants full per frame control it has to queue
> requests early enough for the desired controls associated with a
> request to be realized, so it has to keep 'pipeline_depth' number of requests
> always queued to the camera. If you want controls for frame #10 to be
> realized in time you have to queue Request #10 at the time frame #6
> (or #4 depends on the pipeline_depth) is getting exposed.
>
> If the application doesn't do that, it creates gaps, and while the
> frame sequence number keeps incrementing (as the sensor is producing
> frames) the request sequence number gets only incremented when a new
> request is created, creating a mis-alignment between the request
I agree with most of the above, but I want to clarify that request
sequences are generated/incremented only when a request is *queued* to
the camera, not when the request is created.
> sequence and the frame number.
>
> This is where "Android PFC" and "RPI PFC" differ. RPI wants to allow
> gaps in the Request by jumping ahead and apply the most recent
> controls to the most recent frame. The Android model requires
> applications to keep the requests queue filled in and to queue
> requests at the same pace as the frames gets produced by the sensor.
> if the application 'underrun' the Request queue, PFC simply can't
> happen.
>
> Now, you might have noticed we time the IPA with the application's
> requests. It means that we queue a buffer for statistics and run the
> IPA loop only when a Request is queued. To add confusion, some
> platforms (RkISP1) count requests using the sensor's frame number,
> some others (IPU3) use the Request sequence number.
This is definitely an area I hope we can work to clean up soon. And
indeed we've been talking about it already, but we need to figure this
out cleanly and apply the same decision to any of the libipa IPAs.
> We have been talking recently about making IPAs free running. They
> need to receive stats and produce paramteres for every frame produced
> by the sensor, not for every Request, otherwise we depend on the
> application queue requests in time and at a fast enough pace.
And of course this. Though free-running IPA is essentially a symptom of
how we will handle *not* achieving PFC?
> This however doesn't change the fact that to obtain full PFC
> there need to be one request for each frame, but it's imho the first
> step to allow us to implement PFC correctly.
>
> > > > - Controls for a given sequence number can be updated multiple times
> > > > as long as they are not yet sent out
> > > > - If it's too late, controls get delayed but they are not lost
> > >
> > > What do you mean not lost ? Will they be applied anyway ? what about
> > > the controls for the next frames ?
> >
> > If you request a control for frame 8, but the controls for that frame
> > are already sent to the sensor, the control will be applied at the
> > earliest possible frame (e.g. frame 10). If there are already request
> > queued for frame 9 or 10 the request from frame 8 is discarded.
> >
>
> My gut feeling is that's not what we want, as if an application is
> late in queueing a Request, it's just late and those controls are
> lost. However, if as in your example, the lost control switch an
> algorithm from Auto to Manual, ignoring it might not be the best idea
I disagree here. (But I think it's due to terminology used rather than
what we are all actually expecting here).
If a 'per frame control' is missed/late, indeed it is not possible to
apply to the correct frame. But I believe the state should be updated
and tracked.
Lets imagine an application starts streaming with a manual exposure of
10ms, and after 5 frames wants that to become 30ms exposure, and assume
no other request changes the manual exposure value.
If for a non-determined reason request 5 was late, I wouldn't expect to
continue streaming indefinitely with 10ms, but that the streaming should
(at the earliest opportunity after frame 5) be configured to a 30ms
exposure.
In fact, re-reading the above, both my text and Jacopo's - what we
really need to do here is make the distinction between 'when' the
control is lost. And I would not call the controls 'lost' but
overwritten / superceeded by new incoming controls when applicable. And
if there is no superceeding control/data update - then the control is
not lost, just late. But if there is a new update which is on time -
that would be able to superceed and apply correctly. At that point - it
could be considered that the older control is 'lost' but I would just
say that it has been updated rather than lost.
In 'android model' We should not 'lose' the data. But we can 'update'
(replace) the data.
I think the RPi model is different here in that instead of replace, it
would only push back controls so replacing would not be allowed. But in
the same vein - the global context/state would always be updated.
> > > > - Requests attached to frame 0 don't get lost
> > >
> > > Do you mean the very first request ? Do you mean to apply controls
> > > before streaming is started ?
> > >
> >
> > Yes, controls that are set on the very first request. I added a seperate
> > testcase for that in the next version. Implicitly it was tested in
> > singleControlWithDelayStartUp()
I think we discussed in the past that to establish a configuration at
frame 0, the controls have to be applied during camera->start(controls).
And that for $MaxDelayedControlDelay at the beginning of a stream
otherwise, we simply can not guarantee PFC.
We can not apply manual exposure/gains that have a delay of 2 until the
SoF for the first frame - so while we can set frame 0 - we can *never*
set frame 1 correctly (in theory if it were supposed to be different).
Please correct me if I'm wrong above!
> > > > Technical notes:
> > > > A sourceSequence_ replaces the updated flag to be able to track from which
> > > > frame the update stems. This is needed for the following use case:
> > > > Assume e.g. On frame 10 an ExposureTime=42 is queued because the user wants
> > > > manual exposure. Now the agc gets the stats for frame 8 (where auto
> > > > regulation is still active) and pushes a new ExposureTime for frame 9.
> > > > Frame 9 was already sent out, so it gets delayed to frame 11 (assuming a
> > > > delay of 2 on ExposureTime). This would revert the request from frame 10.
> > > > Taking the sourceSequence into account, the delayed request from frame
> > > > 9 will be discarded, which is correct.
> > >
> > > Also this is not 100% clear to me. The request for frame 10 that says
> > > "manual 42" should be processed by the IPA/pipeline early enough to
> > > make sure the "ExposureTime=42" control is emitted early enough for
> > > being applied in time (assuming 2 frames of delay, this mean the
> > > control should be emitted before frame 8 has started exposing).
> > >
> > > If the AGC gets the stats for frame 8 and computes auto-value for
> > > frame 11, it means it is late and the IPA is not processing requests
> > > early enough ? have you seen this happening ?
> >
> > As the ISP is acting in a closed loop, it can only react on stats that
> > were calculated for the last frame that entered the system. So it is by
> > definition always too late. In the example above, the manual 42 can
> > actually be sent out early enough because it doesn't require information
> > from the stats module. This is the reason for patch 11/12 "rkisp1: Fix
> > per-frame-controls"
> >
>
> I'll look at that patch before commenting further
The one thing I remember from when we looked at this last time is that
we need a better way to convey all the timing in our text. Which means
we need diagrams and pictures IMO.
I really hope we can work on our tracing tools to help us generate such
timing diagram/images in some form beacuse I think we need the same
diagrams to report what /happens/ too! I have ideas ... but lacking in
time currently!
--
Kieran
> > > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > > ---
> > > > include/libcamera/internal/delayed_controls.h | 12 +-
> > > > src/libcamera/delayed_controls.cpp | 207 ++++++++++++++----
> > > > 2 files changed, 174 insertions(+), 45 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h
> > > > index 4f8d2424..2738e8bf 100644
> > > > --- a/include/libcamera/internal/delayed_controls.h
> > > > +++ b/include/libcamera/internal/delayed_controls.h
> > > > @@ -30,25 +30,29 @@ public:
> > > > void reset();
> > > >
> > > > bool push(const ControlList &controls);
> > > > + bool pushForFrame(uint32_t sequence, const ControlList &controls);
> > > > ControlList get(uint32_t sequence);
> > > >
> > > > void applyControls(uint32_t sequence);
> > > >
> > > > private:
> > > > + bool controlsAreQueuedForFrame(unsigned int frame, const ControlList &controls);
> > >
> > > Functions should be defined after types. Please move this after the
> > > ControlRingBuffer type definition below. Also, this is a very long name :)
> > >
> > > > +
> > > > class Info : public ControlValue
> > > > {
> > > > public:
> > > > Info()
> > > > - : updated(false)
> > > > + : sourceSequence_(0)
> > > > {
> > > > }
> > > >
> > > > - Info(const ControlValue &v, bool updated_ = true)
> > > > - : ControlValue(v), updated(updated_)
> > > > + Info(const ControlValue &v, std::optional<uint32_t> sourceSequence = std::nullopt)
> > > > + : ControlValue(v), sourceSequence_(sourceSequence)
> > > > {
> > > > }
> > > >
> > > > - bool updated;
> > > > + /* The sequence id, this info stems from*/
> > > > + std::optional<uint32_t> sourceSequence_;
> > > > };
> > > >
> > > > /* \todo Make the listSize configurable at instance creation time. */
> > > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> > > > index 86571cd4..59314388 100644
> > > > --- a/src/libcamera/delayed_controls.cpp
> > > > +++ b/src/libcamera/delayed_controls.cpp
> > > > @@ -115,8 +115,9 @@ DelayedControls::DelayedControls(V4L2Device *device,
> > > > */
> > > > void DelayedControls::reset()
> > > > {
> > > > - queueIndex_ = 1;
> > > > - writeIndex_ = 0;
> > > > + queueIndex_ = 0;
> > > > + /* Frames up to maxDelay_ will be based on sensor init values. */
> > >
> > > Ok, so this is the startup condition!
> >
> > I'm not sure if I got you right here. Would it be clearer to say
> > "Frames up to maxDelay_ will be based on the values obtained in reset()?
> >
> > >
> > > > + writeIndex_ = maxDelay_;
> > > >
> > > > /* Retrieve control as reported by the device. */
> > > > std::vector<uint32_t> ids;
> > > > @@ -133,26 +134,129 @@ void DelayedControls::reset()
> > > > * Do not mark this control value as updated, it does not need
> > > > * to be written to to device on startup.
> > >
> > > does the comment needs updating too ?
> > >
> >
> > hm, I removed it altogether. There is nothing special to be said here.
> >
> > > > */
> > > > - values_[id][0] = Info(ctrl.second, false);
> > > > + values_[id][0] = Info(ctrl.second, 0);
> > > > }
> > > > +
> > > > + /* Copy state from previous frames. */
> > >
> > > Are these from the previous frames or simply the initial control values ?
> >
> > Yes, initial ones. I updated the comment and factored the loop out.
> >
> > >
> > > > + for (auto &ctrl : values_) {
> > > > + for (auto i = queueIndex_; i < writeIndex_; i++) {
> > >
> > > unsigned int i
> > >
> > > > + Info &info = ctrl.second[i + 1];
> > > > + info = ctrl.second[i];
> > > > + info.sourceSequence_.reset();
> > > > + }
> > > > + }
> > > > +}
> > > > +
> > > > +/**
> > > > + * \brief Helper function to check if controls are queued already
> > > > + * \param[in] sequence Sequence number to check
> > > > + * \param[in] controls List of controls to compare against
> > > > + *
> > > > + * This function checks if the controls queued for frame \a sequence
> > > > + * are equal to \a controls. This is helpful in cases where a control algorithm
> > > > + * unconditionally queues controls for every frame, but always too late.
> > > > + * In that case this can be used to check if incoming controls are already queued
> > > > + * or need to be queued for a later frame.
> > >
> > > I'm interested to see where and how this is used
> > >
> > > > + *
> > > > + * \returns true if \a controls are queued for the given sequence
> > > , false otherwise
> > > > + */
> > > > +bool DelayedControls::controlsAreQueuedForFrame(unsigned int sequence, const ControlList &controls)
> > >
> > > can easily be made < 80 cols.
> > >
> > > > +{
> > > > + auto idMap = controls.idMap();
> > > > + if (!idMap) {
> > > > + LOG(DelayedControls, Warning) << " idmap is null";
> > > > + return false;
> > > > + } else {
> > >
> > > No need for an else branch, you have returned already
> > >
> > > > + for (const auto &[id, value] : controls) {
> > > > + if (values_[idMap->at(id)][sequence] != value) {
> > > > + return false;
> > > > + }
> > >
> > > No {}
> > >
> > > > + }
> > > > + }
> > >
> > > Empty line
> > >
> > > > + return true;
> > > > }
> > > >
> > > > /**
> > > > * \brief Push a set of controls on the queue
> > > > * \param[in] controls List of controls to add to the device queue
> > > > + * \deprecated This function is deprecated in favour of pushForFrame
> > > > *
> > > > * 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
> > > > + * \returns See return value of DelayedControls::pushForFrame
> > > > */
> > > > bool DelayedControls::push(const ControlList &controls)
> > > > {
> > > > - /* Copy state from previous frame. */
> > > > - for (auto &ctrl : values_) {
> > > > - Info &info = ctrl.second[queueIndex_];
> > > > - info = values_[ctrl.first][queueIndex_ - 1];
> > > > - info.updated = false;
> > > > + LOG(DelayedControls, Debug) << "Using deprecated function push(controls): " << queueIndex_;
> > >
> > > break line
> > >
> > > > + return pushForFrame(queueIndex_, controls);
> > > > +}
> > > > +
> > > > +/**
> > > > + * \brief Push a set of controls for a given frame
> > > > + * \param[in] sequence Sequence to push the controls for
> > > > + * \param[in] controls List of controls to add to the device queue
> > > > + *
> > > > + * Pushes the controls given by \a controls, to be applied at frame \a sequence.
> > >
> > > Apply \a controls to frame \a sequence
> >
> > Isn't that a different meaning? I'm not happy with my sentence either.
> > What about: Store \a controls to be applied for frame \a sequence.
> >
> > >
> > > > + *
> > > > + * If there are controls already queued for that frame, these get updated.
> > >
> > > s/these/they
> > >
> > > > + *
> > > > + * If it's too late for frame \a sequence (controls are already sent to the sensor),
> > > > + * the system checks if the controls that where written out for frame \a sequence
> > > > + * are the same as the requested ones. In this case, nothing is done.
> > > > + * If they differ, the controls get queued for the earliest frame possible
> > > > + * if no other controls with a higher sequence number are queued for that frame already.
> > > > + *
> > > > + * \returns true if \a controls are accepted, or false otherwise
> > > > + */
> > > > +
> > >
> > > Stray empty line
> > >
> > > > +bool DelayedControls::pushForFrame(uint32_t sequence, const ControlList &controls)
> > >
> > > Can't you just overload ::push() ?
> >
> > Sure, I can do that. For my brain it is easier to read
> > d->pushForFrame(x, ctrls)
> > than
> > d->push(x, ctrls)
> > But thats most likely a matter of taste (or I worked too much with
> > Apple's macOS APIs)
> >
> > >
> > > > +{
> > > > + if (sequence < queueIndex_) {
> > > > + LOG(DelayedControls, Debug) << "Got updated data for frame:" << sequence;
> > > > + }
> > >
> > > No {}
> > >
> > > > +
> > > > + if (sequence > queueIndex_) {
> > > > + LOG(DelayedControls, Warning) << "Hole in queue sequence. This should not happen. Expected: "
> > > > + << queueIndex_ << " got " << sequence;
> > > > + }
> > >
> > > no {}
> > > very long line
> > >
> > > > +
> > > > + uint32_t updateIndex = sequence;
> > > > + /* check if its too late for the request */
> > > > + if (sequence < writeIndex_) {
> > > > + /* Check if we can safely ignore the request */
> > > > + if (controls.empty() || controlsAreQueuedForFrame(sequence, controls)) {
> > >
> > > Are we comparing control lists at every push() (so likely for every
> > > request) ?
> >
> > Only if we are too late. But in a ISP closed loop, that might happen
> > continuously.
> >
> > >
> > >
> > > > + if (sequence >= queueIndex_) {
> > > > + queueIndex_ = sequence + 1;
> > > > + return true;
> > > > + }
> > > > + } else {
> > > > + LOG(DelayedControls, Debug) << "Controls for frame " << sequence
> > > > + << " are already in flight. Will be queued for frame " << writeIndex_;
> > > > + updateIndex = writeIndex_;
> > > > + }
> > >
> > > veeeery long lines
> > >
> > > > + }
> > > > +
> > > > + if (static_cast<signed>(updateIndex) - static_cast<signed>(writeIndex_) >= listSize - static_cast<signed>(maxDelay_)) {
> > >
> > > Why signed ?
> >
> > Yep, updateIndex is always >= writeIndex. You are right. I changed
> > listSize, to be unsigned also.
> >
> > > >
> > > > + /* The system is in an undefined state now. This will heal itself, as soon as all controls where rewritten */
> > > > + LOG(DelayedControls, Error) << "Queue length exceeded. The system is out of sync. Index to update:"
> > > > + << updateIndex << " Next Index to apply: " << writeIndex_;
> > > > + }
> > > > +
> > > > + /**
> > >
> > > /** is for doxygen
> > >
> > > > + * Prepare the ringbuffer entries with previous data.
> > > > + * Data up to [writeIndex_] gets prepared in applyControls.
> > > > + */
> > > > + if (updateIndex > writeIndex_ && updateIndex >= queueIndex_) {
> > > > + LOG(DelayedControls, Debug) << "Copy from previous " << queueIndex_;
> > > > + for (auto i = queueIndex_; i < updateIndex; i++) {
> > > > + /* Copy state from previous queue. */
> > > > + for (auto &ctrl : values_) {
> > > > + auto &ringBuffer = ctrl.second;
> > > > + ringBuffer[i] = ringBuffer[i - 1];
> > > > + ringBuffer[i].sourceSequence_.reset();
> > > > + }
> > > > + }
> > > > }
> > > >
> > > > /* Update with new controls. */
> > > > @@ -167,20 +271,34 @@ bool DelayedControls::push(const ControlList &controls)
> > > >
> > > > const ControlId *id = it->second;
> > > >
> > > > - if (controlParams_.find(id) == controlParams_.end())
> > > > - return false;
> > > > -
> > > > - Info &info = values_[id][queueIndex_];
> > > > + if (controlParams_.find(id) == controlParams_.end()) {
> > > > + LOG(DelayedControls, Error) << "Could not find params for control " << id << " ignored";
> > >
> > > long line
> > >
> > > > + continue;
> > > > + }
> > >
> > > Is ignoring the control better than erroring out ? Is this worth a
> > > separate change ?
> > >
> > > >
> > > > - info = Info(control.second);
> > > > + Info &info = values_[id][updateIndex];
> > >
> > > Move after the comment block
> > >
> > > > + /*
> > > > + * Update the control only, if the already existing value stems from a request
> > >
> > > s/only,/only
> > >
> > > long line
> > >
> > > > + * with a sequence number smaller or equal to the current one
> > > > + */
> > > > + if (info.sourceSequence_.value_or(0) <= sequence) {
> > > > + info = Info(control.second, sequence);
> > > >
> > > > - LOG(DelayedControls, Debug)
> > > > - << "Queuing " << id->name()
> > > > - << " to " << info.toString()
> > > > - << " at index " << queueIndex_;
> > > > + LOG(DelayedControls, Debug)
> > > > + << "Queuing " << id->name()
> > > > + << " to " << info.toString()
> > > > + << " at index " << updateIndex;
> > > > + } else {
> > > > + LOG(DelayedControls, Warning)
> > > > + << "Skipped update " << id->name()
> > > > + << " at index " << updateIndex;
> > > > + }
> > > > }
> > > >
> > > > - queueIndex_++;
> > > > + LOG(DelayedControls, Debug) << "Queued frame: " << queueIndex_ << " it will be active in " << updateIndex;
> > >
> > > very long line
> > >
> > > > +
> > > > + if (sequence >= queueIndex_)
> > > > + queueIndex_ = sequence + 1;
> > > >
> > > > return true;
> > > > }
> > > > @@ -202,19 +320,17 @@ bool DelayedControls::push(const ControlList &controls)
> > > > */
> > > > ControlList DelayedControls::get(uint32_t sequence)
> > > > {
> > > > - unsigned int index = std::max<int>(0, sequence - maxDelay_);
> > > > -
> > >
> > > This can now go because the initial frames are now populated, right ?
> > >
> > > > ControlList out(device_->controls());
> > > > for (const auto &ctrl : values_) {
> > > > const ControlId *id = ctrl.first;
> > > > - const Info &info = ctrl.second[index];
> > > > + const Info &info = ctrl.second[sequence];
> > > >
> > > > out.set(id->id(), info);
> > > >
> > > > LOG(DelayedControls, Debug)
> > > > << "Reading " << id->name()
> > > > << " to " << info.toString()
> > > > - << " at index " << index;
> > > > + << " at index " << sequence;
> > > > }
> > > >
> > > > return out;
> > > > @@ -222,16 +338,21 @@ ControlList DelayedControls::get(uint32_t sequence)
> > > >
> > > > /**
> > > > * \brief Inform DelayedControls of the start of a new frame
> > > > - * \param[in] sequence Sequence number of the frame that started
> > > > + * \param[in] sequence Sequence number of the frame that started (0-based)
> > > > *
> > > > - * 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.
> > > > + * Inform the state machine that a new frame has started to arrive at the receiver
> > > > + * (e.g. the sensor started to clock out pixels) and of its sequence
> > > > + * number. This is usually the earliest point in time to update registers in the
> > >
> > > Not sure this comment change is necessary
> >
> > For me it was unclear if "a new frame has started" meant:
> >
> > a) That we are now setting controls to start the exposure of that new frame
> > b) That the sensor started exposing that frame
> > c) That a new frame started to arrive on the host
> >
> > Native speakers? Any better ideas?
> >
> > >
> > > > + * sensor for upcoming frames. 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.
> > >
> > > stay in 80-col, please
> > >
> > > > */
> > > > void DelayedControls::applyControls(uint32_t sequence)
> > > > {
> > > > - LOG(DelayedControls, Debug) << "frame " << sequence << " started";
> > > > + LOG(DelayedControls, Debug) << "Apply controls " << sequence;
> > > > + if (sequence != writeIndex_ - maxDelay_) {
> > > > + LOG(DelayedControls, Warning) << "Sequence and writeIndex are out of sync. Expected seq: " << writeIndex_ - maxDelay_ << " got " << sequence;
> > >
> > > Not sure I get why this is an error. Is this to guarantee the IPA
> > > always stays exactly maxDelay_ frames in advance ? can anything like
> > > this be guaranteed ?
> > >
> >
> > In general I would say so. applyControls is normally called as
> > applyControls(0)
> > applyControls(1)
> > applyControls(2)
> >
> > This warning is there to inform about missed applyControls. E.g.
> > applyControls(1) <-- first warning. Index 0 missed
> > applyControls(3) <-- secod warning. Index 2 missed
> >
> > If we want that to be acceptable, we would need to accumulate all control
> > changes from the missed frames...
> >
> > > > + }
> > >
> > > no {}
> > > very long line
> > >
> > > >
> > > > /*
> > > > * Create control list peeking ahead in the value queue to ensure
> > > > @@ -241,10 +362,10 @@ void DelayedControls::applyControls(uint32_t sequence)
> > > > for (auto &ctrl : values_) {
> > > > const ControlId *id = ctrl.first;
> > > > unsigned int delayDiff = maxDelay_ - controlParams_[id].delay;
> > > > - unsigned int index = std::max<int>(0, writeIndex_ - delayDiff);
> > > > + unsigned int index = writeIndex_ - delayDiff;
> > > > Info &info = ctrl.second[index];
> > > >
> > > > - if (info.updated) {
> > > > + if (info.sourceSequence_.has_value()) {
> > > > if (controlParams_[id].priorityWrite) {
> > > > /*
> > > > * This control must be written now, it could
> > > > @@ -262,21 +383,25 @@ void DelayedControls::applyControls(uint32_t sequence)
> > > > }
> > > >
> > > > LOG(DelayedControls, Debug)
> > > > - << "Setting " << id->name()
> > > > - << " to " << info.toString()
> > > > - << " at index " << index;
> > > > -
> > > > - /* Done with this update, so mark as completed. */
> > > > - info.updated = false;
> > > > + << "Writing " << id->name()
> > > > + << " (" << info.toString() << ") "
> > > > + << " for frame " << index;
> > > > }
> > > > }
> > > >
> > > > - writeIndex_ = sequence + 1;
> > > > + auto oldWriteIndex = writeIndex_;
> > > > + writeIndex_ = sequence + maxDelay_ + 1;
> > > >
> > > > - while (writeIndex_ > queueIndex_) {
> > > > + if (writeIndex_ >= queueIndex_ && writeIndex_ > oldWriteIndex) {
> > > > LOG(DelayedControls, Debug)
> > > > - << "Queue is empty, auto queue no-op.";
> > > > - push({});
> > > > + << "Index " << writeIndex_ << " is not yet queued. Prepare with old state";
> > > > + /* Copy state from previous frames without resetting the sourceSequence */
> > > > + for (auto &ctrl : values_) {
> > > > + for (auto i = oldWriteIndex; i < writeIndex_; i++) {
> > > > + Info &info = ctrl.second[i + 1];
> > > > + info = values_[ctrl.first][i];
> > > > + }
> > > > + }
> > >
> > > My main worries is now that, if I understand it right, the system
> > > always tries to keep a maxDelay number of frames between the last
> > > controls written to the sensor and the head of the queue, and to do so
> > > it duplicate controls at every push() and apply(). This happens for
> > > -every- frame and -every- request if I got it right ? Am I too
> > > over-concerned this is expensive ?
> >
> > To my understanding that is no change to the way it was before. Before,
> > that copying happed due to push({}), thereby messing up the
> > synchronicity of push() and apply().
> >
> > >
> > > > }
> > > >
> > > > device_->setControls(&out);
> > > > --
> > > > 2.40.1
> > > >
> >
> > Thank you very much for all the feedback. It's time for a v3. And next
> > time without the style issues :-)
> >
> > Cheers,
> > Stefan
More information about the libcamera-devel
mailing list