[v3,08/16] libcamera: delayed_controls: Add ctrls list parameter to reset() function

19748 diff mbox series

This makes it easier for the caller. There is often this pattern

  sensor_->setControls(controls);
  delayedControls_->reset();

which can then be reduced to

  delayedControls_->reset(controls);

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

Hi Stefan

On Tue, Mar 19, 2024 at 01:05:09PM +0100, Stefan Klug wrote:
> This makes it easier for the caller. There is often this pattern
>
>   sensor_->setControls(controls);
>   delayedControls_->reset();
>
> which can then be reduced to
>
>   delayedControls_->reset(controls);
>

Nit: I would then feel more confortable having the constructor accept
a CameraSensor &sensor and initialize device_ with sensor->device()
so that we guarantee the device_ is the same.

However, I wonder, to have controls applied immediately the sensor
should not be streaming and nothing enforces delayed controls to be
reset only when the sensor is not streaming.

I guess that's not different than what we have right now

> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> ---
>  include/libcamera/internal/delayed_controls.h |  2 +-
>  src/libcamera/delayed_controls.cpp            | 22 +++++++++++++++----
>  2 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h
> index ccbe7239..224c1f7e 100644
> --- a/include/libcamera/internal/delayed_controls.h
> +++ b/include/libcamera/internal/delayed_controls.h
> @@ -27,7 +27,7 @@ public:
>  	DelayedControls(V4L2Device *device,
>  			const std::unordered_map<uint32_t, ControlParams> &controlParams);
>
> -	void reset();
> +	void reset(ControlList *controls = nullptr);
>
>  	bool push(const ControlList &controls);
>  	ControlList get(uint32_t sequence);
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> index 6c766ede..6f06950e 100644
> --- a/src/libcamera/delayed_controls.cpp
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -109,11 +109,13 @@ DelayedControls::DelayedControls(V4L2Device *device,
>
>  /**
>   * \brief Reset state machine
> + * \param[in,out] controls The controls to apply to the device
>   *
>   * Resets the state machine to a starting position based on control values
> - * retrieved from the device.
> + * retrieved from the device. If \a controls is given, these controls are set
> + * on the device before retrieving the reset values.
>   */
> -void DelayedControls::reset()
> +void DelayedControls::reset(ControlList *controls)
>  {
>  	queueIndex_ = 1;
>  	writeIndex_ = 0;
> @@ -123,11 +125,23 @@ void DelayedControls::reset()
>  	for (auto const &param : controlParams_)
>  		ids.push_back(param.first->id());
>
> -	ControlList controls = device_->getControls(ids);
> +	if (controls) {
> +		device_->setControls(controls);
> +
> +		LOG(DelayedControls, Debug) << "reset:";
> +		auto idMap = controls->idMap();
> +		if (idMap) {
> +			for (const auto &[id, value] : *controls)
> +				LOG(DelayedControls, Debug) << "  " << idMap->at(id)->name()
> +							    << " : " << value.toString();
> +		}
> +	}
> +
> +	ControlList ctrls = device_->getControls(ids);
>
>  	/* Seed the control queue with the controls reported by the device. */
>  	values_.clear();
> -	for (const auto &ctrl : controls) {
> +	for (const auto &ctrl : ctrls) {
>  		const ControlId *id = device_->controls().idmap().at(ctrl.first);
>  		/*
>  		 * Do not mark this control value as updated, it does not need
> --
> 2.40.1
>