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

[paul.elder at ideasonboard.com]

Hi Naush,

On Tue, Feb 16, 2021 at 08:53:41AM +0000, 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")

Looks good to me.

Reviewed-by: Paul Elder <paul.elder 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 21dc3e164fe9..d1b79dc3570e 100644
> --- a/src/libcamera/delayed_controls.cpp
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -225,11 +225,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) {
> @@ -252,12 +252,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({});
> -- 
> 2.25.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel