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

[Jacopo Mondi]

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 at ideasonboard.com>
> ---
>  include/libcamera/internal/delayed_controls.h |  12 +-
>  src/libcamera/delayed_controls.cpp            | 207 ++++++++++++++----
>  2 files changed, 174 insertions(+), 45 deletions(-)
>
> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h
> index 4f8d2424..2738e8bf 100644
> --- a/include/libcamera/internal/delayed_controls.h
> +++ b/include/libcamera/internal/delayed_controls.h
> @@ -30,25 +30,29 @@ public:
>  	void reset();
>
>  	bool push(const ControlList &controls);
> +	bool pushForFrame(uint32_t sequence, const ControlList &controls);
>  	ControlList get(uint32_t sequence);
>
>  	void applyControls(uint32_t sequence);
>
>  private:
> +	bool controlsAreQueuedForFrame(unsigned int frame, const ControlList &controls);

Functions should be defined after types. Please move this after the
ControlRingBuffer type definition below. Also, this is a very long name :)

> +
>  	class Info : public ControlValue
>  	{
>  	public:
>  		Info()
> -			: updated(false)
> +			: sourceSequence_(0)
>  		{
>  		}
>
> -		Info(const ControlValue &v, bool updated_ = true)
> -			: ControlValue(v), updated(updated_)
> +		Info(const ControlValue &v, std::optional<uint32_t> sourceSequence = std::nullopt)
> +			: ControlValue(v), sourceSequence_(sourceSequence)
>  		{
>  		}
>
> -		bool updated;
> +		/* The sequence id, this info stems from*/
> +		std::optional<uint32_t> sourceSequence_;
>  	};
>
>  	/* \todo Make the listSize configurable at instance creation time. */
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> index 86571cd4..59314388 100644
> --- a/src/libcamera/delayed_controls.cpp
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -115,8 +115,9 @@ DelayedControls::DelayedControls(V4L2Device *device,
>   */
>  void DelayedControls::reset()
>  {
> -	queueIndex_ = 1;
> -	writeIndex_ = 0;
> +	queueIndex_ = 0;
> +	/* Frames up to maxDelay_ will be based on sensor init values. */

Ok, so this is the startup condition!

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