[v2,06/12] libcamera: delayed_controls: Rework delayed controls implementation

19708 diff mbox series

Functional changes:
- Requests need to be queued for every frame
- 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
- 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
- Requests attached to frame 0 don't get lost

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.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 include/libcamera/internal/delayed_controls.h |  12 +-
 src/libcamera/delayed_controls.cpp            | 207 ++++++++++++++----
 2 files changed, 174 insertions(+), 45 deletions(-)

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 ?

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

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

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

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

>
> Signed-off-by: Stefan Klug <stefan.klug@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!

> +	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 ?

>  		 */
> -		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 ?

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

> + *
> + * 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() ?

> +{
> +	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) ?


> +			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 ?
>
> +		/* 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

> + * 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 ?

> +	}

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 ?

>  	}
>
>  	device_->setControls(&out);
> --
> 2.40.1
>

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

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

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

> 
> > - 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()

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

> 
> >
> > Signed-off-by: Stefan Klug <stefan.klug@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

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

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.

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

> >
> > > - 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()
>
> > >
> > > 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

> >
> > >
> > > Signed-off-by: Stefan Klug <stefan.klug@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

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

Hi everyone

Good to see this discussion continuing. Let me add a few observations,
though I won't comment exhaustively because I think there's just too
much to unpack in this topic!

On Thu, 21 Mar 2024 at 10:52, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> 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)

Good point. Obviously we wouldn't want anything to be mandated for our
pipeline handler that didn't work for us!

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

In our implementation we have a sequence number for the control lists
in a request. That is, it goes up by one only when you supply some
controls. Our experience is that it is easier for applications to use,
understand and debug, rather than have numbers that are shooting up
the whole time regardless. It's also much easier to debug internally
when implementing PFC within your pipeline handler as you aren't
always saying to yourself "now what request number did I send those
controls on again...??"

(Partly this is a reflection of the fact that we'd prefer separate
queues for control lists, and not associate them with
requests/buffers, though if buffers get removed from requests then you
start to wonder what a request really is. But that's the start of a
bigger discussion!)

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

Yes, I think I agree with most of that. Just on the use of the term
"mis-alignment" - for us, we regard this as quite "normal", it
happens. So it's important to us to define a scheme that is usable
when this occurs.

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

In a sense, yes, but it seems like a slightly narrow interpretation.
In the Raspberry Pi scheme you still have PFC with just a single
buffer. What I mean is that every buffer that comes out tells you (via
a sequence number) exactly which set of controls have been applied. In
fact, you can have back-to-back sequences of frames coming out, with
perfect knowledge of how the controls change on each one, with fewer
requests than the pipeline depth.

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

I agree with the "disagreeing" here. It's certainly important to
define what happens, and for us, leaving the system in an uncertain
state would be bad.

One of the big runtime risks for us is not hitting the interrupts we
need for camera exposure updates. There are two things we could do:

* Wait for the next frame and try again. So you might get the 10ms
exposure coming out twice, and then changing to 30ms. Personally, I
think this is the most useful behaviour.

* Merge the missed controls into the next set of controls, and carry
on. In the above example, you could find that you never got 10ms, as
the 10ms would have been applied with the next frame, except that the
next frame is already going to get 30ms (which takes precedence). So
the 10ms is lost in a sense, though the system always ends in a known
state which matches the last controls that you sent. And importantly,
the reporting via control list sequence numbers tells you the exact
state of every frame that you get, whatever happens.

Actually our implementation takes the 2nd approach even though I
prefer the first. The reason is that the coupling of requests and
controls means you end up with a theoretically unbounded delay between
them which is (theoretically) annoying to handle.

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

Yes, I think that agrees with what I wrote just above!

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

I think I agree with that!

David

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