[libcamera-devel] [PATCH v4 4/7] libcamera: delayed_controls: Remove spurious no-op queued controls

[Jean-Michel Hautbois]

On 04/03/2021 09:17, Naushir Patuck wrote:
> In DelayedControls::applyControls(), the controls queue would be
> extended via a no-op control push to fill the intermittent slots with
> unchanged control values. This is needed so that we read back unchanged
> control values correctly on every frame.
> 
> However, there was one additional no-op performed on every frame that is
> not required, meaning that any controls queued by the pipeline handler
> would have their write delayed by one further frame. The original
> StaggeredCtrl did not do this, as it only had one index to manage,
> whereas DelayedControls uses two.
> 
> Remove this last no-op push so that the pipeline_handler provided
> controls would be written on the very next frame if possible. As a
> consequence, we must mark the control update as completed within the
> DelayedControls::applyControls() loop, otherwise it might get reused
> when cycling through the ring buffer.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reported-by: David Plowman <david.plowman at raspberrypi.com>
> Fixes: 3d4b7b005911 ("libcamera: delayed_controls: Add helper for controls that apply with a delay")
> Tested-by: David Plowman <david.plowman at raspberrypi.com>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/libcamera/delayed_controls.cpp | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> index f6d761d1cc41..5a05741c0285 100644
> --- a/src/libcamera/delayed_controls.cpp
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -226,11 +226,11 @@ void DelayedControls::applyControls(uint32_t sequence)
>  	 * values are set in time to satisfy the sensor delay.
>  	 */
>  	ControlList out(device_->controls());
> -	for (const auto &ctrl : values_) {
> +	for (auto &ctrl : values_) {
>  		const ControlId *id = ctrl.first;
>  		unsigned int delayDiff = maxDelay_ - controlParams_[id].delay;
>  		unsigned int index = std::max<int>(0, writeCount_ - delayDiff);
> -		const Info &info = ctrl.second[index];
> +		Info &info = ctrl.second[index];
>  
>  		if (info.updated) {
>  			if (controlParams_[id].priorityWrite) {
> @@ -253,12 +253,15 @@ void DelayedControls::applyControls(uint32_t sequence)
>  				<< "Setting " << id->name()
>  				<< " to " << info.toString()
>  				<< " at index " << index;
> +
> +			/* Done with this update, so mark as completed. */
> +			info.updated = false;
>  		}
>  	}
>  
>  	writeCount_++;
>  
> -	while (writeCount_ >= queueCount_) {
> +	while (writeCount_ > queueCount_) {
>  		LOG(DelayedControls, Debug)
>  			<< "Queue is empty, auto queue no-op.";
>  		push({});
>