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

[Stefan Klug]

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 at ideasonboard.com>
> > ---
> >  include/libcamera/internal/delayed_controls.h |  12 +-
> >  src/libcamera/delayed_controls.cpp            | 207 ++++++++++++++----
> >  2 files changed, 174 insertions(+), 45 deletions(-)
> >
> > diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h
> > index 4f8d2424..2738e8bf 100644
> > --- a/include/libcamera/internal/delayed_controls.h
> > +++ b/include/libcamera/internal/delayed_controls.h
> > @@ -30,25 +30,29 @@ public:
> >  	void reset();
> >
> >  	bool push(const ControlList &controls);
> > +	bool pushForFrame(uint32_t sequence, const ControlList &controls);
> >  	ControlList get(uint32_t sequence);
> >
> >  	void applyControls(uint32_t sequence);
> >
> >  private:
> > +	bool controlsAreQueuedForFrame(unsigned int frame, const ControlList &controls);
> 
> Functions should be defined after types. Please move this after the
> ControlRingBuffer type definition below. Also, this is a very long name :)
> 
> > +
> >  	class Info : public ControlValue
> >  	{
> >  	public:
> >  		Info()
> > -			: updated(false)
> > +			: sourceSequence_(0)
> >  		{
> >  		}
> >
> > -		Info(const ControlValue &v, bool updated_ = true)
> > -			: ControlValue(v), updated(updated_)
> > +		Info(const ControlValue &v, std::optional<uint32_t> sourceSequence = std::nullopt)
> > +			: ControlValue(v), sourceSequence_(sourceSequence)
> >  		{
> >  		}
> >
> > -		bool updated;
> > +		/* The sequence id, this info stems from*/
> > +		std::optional<uint32_t> sourceSequence_;
> >  	};
> >
> >  	/* \todo Make the listSize configurable at instance creation time. */
> > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> > index 86571cd4..59314388 100644
> > --- a/src/libcamera/delayed_controls.cpp
> > +++ b/src/libcamera/delayed_controls.cpp
> > @@ -115,8 +115,9 @@ DelayedControls::DelayedControls(V4L2Device *device,
> >   */
> >  void DelayedControls::reset()
> >  {
> > -	queueIndex_ = 1;
> > -	writeIndex_ = 0;
> > +	queueIndex_ = 0;
> > +	/* Frames up to maxDelay_ will be based on sensor init values. */
> 
> Ok, so this is the startup condition!

I'm not sure if I got you right here. Would it be clearer to say
"Frames up to maxDelay_ will be based on the values obtained in reset()?

> 
> > +	writeIndex_ = maxDelay_;
> >
> >  	/* Retrieve control as reported by the device. */
> >  	std::vector<uint32_t> ids;
> > @@ -133,26 +134,129 @@ void DelayedControls::reset()
> >  		 * Do not mark this control value as updated, it does not need
> >  		 * to be written to to device on startup.
> 
> does the comment needs updating too ?
> 

hm, I removed it altogether. There is nothing special to be said here.

> >  		 */
> > -		values_[id][0] = Info(ctrl.second, false);
> > +		values_[id][0] = Info(ctrl.second, 0);
> >  	}
> > +
> > +	/* Copy state from previous frames. */
> 
> Are these from the previous frames or simply the initial control values ?

Yes, initial ones. I updated the comment and factored the loop out.

> 
> > +	for (auto &ctrl : values_) {
> > +		for (auto i = queueIndex_; i < writeIndex_; i++) {
> 
> unsigned int i
> 
> > +			Info &info = ctrl.second[i + 1];
> > +			info = ctrl.second[i];
> > +			info.sourceSequence_.reset();
> > +		}
> > +	}
> > +}
> > +
> > +/**
> > + * \brief Helper function to check if controls are queued already
> > + * \param[in] sequence Sequence number to check
> > + * \param[in] controls List of controls to compare against
> > + *
> > + * This function checks if the controls queued for frame \a sequence
> > + * are equal to \a controls. This is helpful in cases where a control algorithm
> > + * unconditionally queues controls for every frame, but always too late.
> > + * In that case this can be used to check if incoming controls are already queued
> > + * or need to be queued for a later frame.
> 
> I'm interested to see where and how this is used
> 
> > + *
> > + * \returns true if \a controls are queued for the given sequence
> , false otherwise
> > + */
> > +bool DelayedControls::controlsAreQueuedForFrame(unsigned int sequence, const ControlList &controls)
> 
> can easily be made < 80 cols.
> 
> > +{
> > +	auto idMap = controls.idMap();
> > +	if (!idMap) {
> > +		LOG(DelayedControls, Warning) << " idmap is null";
> > +		return false;
> > +	} else {
> 
> No need for an else branch, you have returned already
> 
> > +		for (const auto &[id, value] : controls) {
> > +			if (values_[idMap->at(id)][sequence] != value) {
> > +				return false;
> > +			}
> 
> No {}
> 
> > +		}
> > +	}
> 
> Empty line
> 
> > +	return true;
> >  }
> >
> >  /**
> >   * \brief Push a set of controls on the queue
> >   * \param[in] controls List of controls to add to the device queue
> > + * \deprecated This function is deprecated in favour of pushForFrame
> >   *
> >   * Push a set of controls to the control queue. This increases the control queue
> >   * depth by one.
> >   *
> > - * \returns true if \a controls are accepted, or false otherwise
> > + * \returns See return value of DelayedControls::pushForFrame
> >   */
> >  bool DelayedControls::push(const ControlList &controls)
> >  {
> > -	/* Copy state from previous frame. */
> > -	for (auto &ctrl : values_) {
> > -		Info &info = ctrl.second[queueIndex_];
> > -		info = values_[ctrl.first][queueIndex_ - 1];
> > -		info.updated = false;
> > +	LOG(DelayedControls, Debug) << "Using deprecated function push(controls): " << queueIndex_;
> 
> break line
> 
> > +	return pushForFrame(queueIndex_, controls);
> > +}
> > +
> > +/**
> > + * \brief Push a set of controls for a given frame
> > + * \param[in] sequence Sequence to push the controls for
> > + * \param[in] controls List of controls to add to the device queue
> > + *
> > + * Pushes the controls given by \a controls, to be applied at frame \a sequence.
> 
> Apply \a controls to frame \a sequence

Isn't that a different meaning? I'm not happy with my sentence either.
What about: Store \a controls to be applied for frame \a sequence.

> 
> > + *
> > + * If there are controls already queued for that frame, these get updated.
> 
> s/these/they
> 
> > + *
> > + * If it's too late for frame \a sequence (controls are already sent to the sensor),
> > + * the system checks if the controls that where written out for frame \a sequence
> > + * are the same as the requested ones. In this case, nothing is done.
> > + * If they differ, the controls get queued for the earliest frame possible
> > + * if no other controls with a higher sequence number are queued for that frame already.
> > + *
> > + * \returns true if \a controls are accepted, or false otherwise
> > + */
> > +
> 
> Stray empty line
> 
> > +bool DelayedControls::pushForFrame(uint32_t sequence, const ControlList &controls)
> 
> Can't you just overload ::push() ?

Sure, I can do that. For my brain it is easier to read
d->pushForFrame(x, ctrls)
than
d->push(x, ctrls)
But thats most likely a matter of taste (or I worked too much with
Apple's macOS APIs)

> 
> > +{
> > +	if (sequence < queueIndex_) {
> > +		LOG(DelayedControls, Debug) << "Got updated data for frame:" << sequence;
> > +	}
> 
> No {}
> 
> > +
> > +	if (sequence > queueIndex_) {
> > +		LOG(DelayedControls, Warning) << "Hole in queue sequence. This should not happen. Expected: "
> > +					      << queueIndex_ << " got " << sequence;
> > +	}
> 
> no {}
> very long line
> 
> > +
> > +	uint32_t updateIndex = sequence;
> > +	/* check if its too late for the request */
> > +	if (sequence < writeIndex_) {
> > +		/* Check if we can safely ignore the request */
> > +		if (controls.empty() || controlsAreQueuedForFrame(sequence, controls)) {
> 
> Are we comparing control lists at every push() (so likely for every
> request) ?

Only if we are too late. But in a ISP closed loop, that might happen
continuously.

> 
> 
> > +			if (sequence >= queueIndex_) {
> > +				queueIndex_ = sequence + 1;
> > +				return true;
> > +			}
> > +		} else {
> > +			LOG(DelayedControls, Debug) << "Controls for frame " << sequence
> > +						    << " are already in flight. Will be queued for frame " << writeIndex_;
> > +			updateIndex = writeIndex_;
> > +		}
> 
> veeeery long lines
> 
> > +	}
> > +
> > +	if (static_cast<signed>(updateIndex) - static_cast<signed>(writeIndex_) >= listSize - static_cast<signed>(maxDelay_)) {
> 
> Why signed ?

Yep, updateIndex is always >= writeIndex. You are right. I changed
listSize, to be unsigned also.

> >
> > +		/* The system is in an undefined state now. This will heal itself, as soon as all controls where rewritten */
> > +		LOG(DelayedControls, Error) << "Queue length exceeded. The system is out of sync. Index to update:"
> > +					    << updateIndex << " Next Index to apply: " << writeIndex_;
> > +	}
> > +
> > +	/**
> 
> /** is for doxygen
> 
> > +	 * Prepare the ringbuffer entries with previous data.
> > +	 * Data up to [writeIndex_] gets prepared in applyControls.
> > +	 */
> > +	if (updateIndex > writeIndex_ && updateIndex >= queueIndex_) {
> > +		LOG(DelayedControls, Debug) << "Copy from previous " << queueIndex_;
> > +		for (auto i = queueIndex_; i < updateIndex; i++) {
> > +			/* Copy state from previous queue. */
> > +			for (auto &ctrl : values_) {
> > +				auto &ringBuffer = ctrl.second;
> > +				ringBuffer[i] = ringBuffer[i - 1];
> > +				ringBuffer[i].sourceSequence_.reset();
> > +			}
> > +		}
> >  	}
> >
> >  	/* Update with new controls. */
> > @@ -167,20 +271,34 @@ bool DelayedControls::push(const ControlList &controls)
> >
> >  		const ControlId *id = it->second;
> >
> > -		if (controlParams_.find(id) == controlParams_.end())
> > -			return false;
> > -
> > -		Info &info = values_[id][queueIndex_];
> > +		if (controlParams_.find(id) == controlParams_.end()) {
> > +			LOG(DelayedControls, Error) << "Could not find params for control " << id << " ignored";
> 
> long line
> 
> > +			continue;
> > +		}
> 
> Is ignoring the control better than erroring out ? Is this worth a
> separate change ?
> 
> >
> > -		info = Info(control.second);
> > +		Info &info = values_[id][updateIndex];
> 
> Move after the comment block
> 
> > +		/*
> > +		 * Update the control only, if the already existing value stems from a request
> 
> s/only,/only
> 
> long line
> 
> > +		 * with a sequence number smaller or equal to the current one
> > +		 */
> > +		if (info.sourceSequence_.value_or(0) <= sequence) {
> > +			info = Info(control.second, sequence);
> >
> > -		LOG(DelayedControls, Debug)
> > -			<< "Queuing " << id->name()
> > -			<< " to " << info.toString()
> > -			<< " at index " << queueIndex_;
> > +			LOG(DelayedControls, Debug)
> > +				<< "Queuing " << id->name()
> > +				<< " to " << info.toString()
> > +				<< " at index " << updateIndex;
> > +		} else {
> > +			LOG(DelayedControls, Warning)
> > +				<< "Skipped update " << id->name()
> > +				<< " at index " << updateIndex;
> > +		}
> >  	}
> >
> > -	queueIndex_++;
> > +	LOG(DelayedControls, Debug) << "Queued frame: " << queueIndex_ << " it will be active in " << updateIndex;
> 
> very long line
> 
> > +
> > +	if (sequence >= queueIndex_)
> > +		queueIndex_ = sequence + 1;
> >
> >  	return true;
> >  }
> > @@ -202,19 +320,17 @@ bool DelayedControls::push(const ControlList &controls)
> >   */
> >  ControlList DelayedControls::get(uint32_t sequence)
> >  {
> > -	unsigned int index = std::max<int>(0, sequence - maxDelay_);
> > -
> 
> This can now go because the initial frames are now populated, right ?
> 
> >  	ControlList out(device_->controls());
> >  	for (const auto &ctrl : values_) {
> >  		const ControlId *id = ctrl.first;
> > -		const Info &info = ctrl.second[index];
> > +		const Info &info = ctrl.second[sequence];
> >
> >  		out.set(id->id(), info);
> >
> >  		LOG(DelayedControls, Debug)
> >  			<< "Reading " << id->name()
> >  			<< " to " << info.toString()
> > -			<< " at index " << index;
> > +			<< " at index " << sequence;
> >  	}
> >
> >  	return out;
> > @@ -222,16 +338,21 @@ ControlList DelayedControls::get(uint32_t sequence)
> >
> >  /**
> >   * \brief Inform DelayedControls of the start of a new frame
> > - * \param[in] sequence Sequence number of the frame that started
> > + * \param[in] sequence Sequence number of the frame that started (0-based)
> >   *
> > - * Inform the state machine that a new frame has started and of its sequence
> > - * number. Any user of these helpers is responsible to inform the helper about
> > - * the start of any frame. This can be connected with ease to the start of a
> > - * exposure (SOE) V4L2 event.
> > + * Inform the state machine that a new frame has started to arrive at the receiver
> > + * (e.g. the sensor started to clock out pixels) and of its sequence
> > + * number. This is usually the earliest point in time to update registers in the
> 
> Not sure this comment change is necessary

For me it was unclear if "a new frame has started" meant:

 a) That we are now setting controls to start the exposure of that new frame
 b) That the sensor started exposing that frame
 c) That a new frame started to arrive on the host

Native speakers? Any better ideas?

> 
> > + * sensor for upcoming frames. Any user of these helpers is responsible to inform
> > + * the helper about the start of any frame. This can be connected with ease to
> > + * the start of a exposure (SOE) V4L2 event.
> 
> stay in 80-col, please
> 
> >   */
> >  void DelayedControls::applyControls(uint32_t sequence)
> >  {
> > -	LOG(DelayedControls, Debug) << "frame " << sequence << " started";
> > +	LOG(DelayedControls, Debug) << "Apply controls " << sequence;
> > +	if (sequence != writeIndex_ - maxDelay_) {
> > +		LOG(DelayedControls, Warning) << "Sequence and writeIndex are out of sync. Expected seq: " << writeIndex_ - maxDelay_ << " got " << sequence;
> 
> Not sure I get why this is an error. Is this to guarantee the IPA
> always stays exactly maxDelay_ frames in advance ? can anything like
> this be guaranteed ?
> 

In general I would say so. applyControls is normally called as
applyControls(0)
applyControls(1)
applyControls(2)

This warning is there to inform about missed applyControls. E.g.
applyControls(1) <-- first warning. Index 0 missed
applyControls(3) <-- secod warning. Index 2 missed

If we want that to be acceptable, we would need to accumulate all control
changes from the missed frames...

> > +	}
> 
> no {}
> very long line
> 
> >
> >  	/*
> >  	 * Create control list peeking ahead in the value queue to ensure
> > @@ -241,10 +362,10 @@ void DelayedControls::applyControls(uint32_t sequence)
> >  	for (auto &ctrl : values_) {
> >  		const ControlId *id = ctrl.first;
> >  		unsigned int delayDiff = maxDelay_ - controlParams_[id].delay;
> > -		unsigned int index = std::max<int>(0, writeIndex_ - delayDiff);
> > +		unsigned int index = writeIndex_ - delayDiff;
> >  		Info &info = ctrl.second[index];
> >
> > -		if (info.updated) {
> > +		if (info.sourceSequence_.has_value()) {
> >  			if (controlParams_[id].priorityWrite) {
> >  				/*
> >  				 * This control must be written now, it could
> > @@ -262,21 +383,25 @@ void DelayedControls::applyControls(uint32_t sequence)
> >  			}
> >
> >  			LOG(DelayedControls, Debug)
> > -				<< "Setting " << id->name()
> > -				<< " to " << info.toString()
> > -				<< " at index " << index;
> > -
> > -			/* Done with this update, so mark as completed. */
> > -			info.updated = false;
> > +				<< "Writing " << id->name()
> > +				<< " (" << info.toString() << ") "
> > +				<< " for frame " << index;
> >  		}
> >  	}
> >
> > -	writeIndex_ = sequence + 1;
> > +	auto oldWriteIndex = writeIndex_;
> > +	writeIndex_ = sequence + maxDelay_ + 1;
> >
> > -	while (writeIndex_ > queueIndex_) {
> > +	if (writeIndex_ >= queueIndex_ && writeIndex_ > oldWriteIndex) {
> >  		LOG(DelayedControls, Debug)
> > -			<< "Queue is empty, auto queue no-op.";
> > -		push({});
> > +			<< "Index " << writeIndex_ << " is not yet queued. Prepare with old state";
> > +		/* Copy state from previous frames without resetting the sourceSequence */
> > +		for (auto &ctrl : values_) {
> > +			for (auto i = oldWriteIndex; i < writeIndex_; i++) {
> > +				Info &info = ctrl.second[i + 1];
> > +				info = values_[ctrl.first][i];
> > +			}
> > +		}
> 
> My main worries is now that, if I understand it right, the system
> always tries to keep a maxDelay number of frames between the last
> controls written to the sensor and the head of the queue, and to do so
> it duplicate controls at every push() and apply(). This happens for
> -every- frame and -every- request if I got it right ? Am I too
> over-concerned this is expensive ?

To my understanding that is no change to the way it was before. Before,
that copying happed due to push({}), thereby messing up the
synchronicity of push() and apply().

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

Thank you very much for all the feedback. It's time for a v3. And next
time without the style issues :-)

Cheers,
Stefan