[libcamera-devel] [PATCH 3/3] libcamera: V4L2Device: Use std::vector in updateControls()

[Kieran Bingham]

Hi Hiro,

On 13/04/2021 07:19, Hirokazu Honda wrote:
> V4L2Device::updateControls() takes two arguments, raw array and
> its size, for the v4l2_ext_control values. This replaces it with
> std::vector.

This patch does more than just replace the raw array for the vector
like-for-like.

It also changes the processing of how the controls are searched and
iterated.

That should be documented here, if it's still appropriate - though to
ease things - it might be clearer/easier to have the change to a vector
directly, and then fix up the parsing if that's a specific improvement.


> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  include/libcamera/internal/v4l2_device.h |  3 +-
>  src/libcamera/v4l2_device.cpp            | 36 +++++++++++++-----------
>  2 files changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index d006bf68..4cce3840 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -55,8 +55,7 @@ protected:
>  private:
>  	void listControls();
>  	void updateControls(ControlList *ctrls,
> -			    const struct v4l2_ext_control *v4l2Ctrls,
> -			    unsigned int count);
> +			    const std::vector<v4l2_ext_control> &v4l2Ctrls);
>  
>  	void eventAvailable(EventNotifier *notifier);
>  
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 8625dde8..8f29cd7f 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -251,7 +251,7 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
>  		v4l2Ctrls.resize(errorIdx);
>  	}
>  
> -	updateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());
> +	updateControls(&ctrls, v4l2Ctrls);
>  
>  	return ctrls;
>  }
> @@ -353,7 +353,7 @@ int V4L2Device::setControls(ControlList *ctrls)
>  		ret = errorIdx;
>  	}
>  
> -	updateControls(ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());
> +	updateControls(ctrls, v4l2Ctrls);
>  
>  	return ret;
>  }
> @@ -517,25 +517,27 @@ void V4L2Device::listControls()
>   * values in \a v4l2Ctrls
>   * \param[inout] ctrls List of V4L2 controls to update
>   * \param[in] v4l2Ctrls List of V4L2 extended controls as returned by the driver
> - * \param[in] count The number of controls to update
>   */
>  void V4L2Device::updateControls(ControlList *ctrls,
> -				const struct v4l2_ext_control *v4l2Ctrls,
> -				unsigned int count)
> +				const std::vector<v4l2_ext_control> &v4l2Ctrls)
>  {
> -	unsigned int i = 0;
> -	for (auto &ctrl : *ctrls) {
> -		if (i == count)
> -			break;
> +	for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) {
> +		if (!ctrls->contains(v4l2Ctrl.id)) {
> +			LOG(V4L2, Error) << "Unknown id: " << v4l2Ctrl.id;
> +			continue;
> +		}

Hrm, are these extra validation checks?
or is it because we've inverted the search now?

> -		const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> -		unsigned int id = ctrl.first;
> -		ControlValue &value = ctrl.second;
> +		const auto it = controls_.find(v4l2Ctrl.id);
> +		if (it == controls_.end()) {
> +			LOG(V4L2, Error) << "Unknown id: " << v4l2Ctrl.id;
> +			continue;
> +		}
>  
> -		const auto iter = controls_.find(id);
> -		switch (iter->first->type()) {
> +		const ControlValue &value = ctrls->get(v4l2Ctrl.id);
> +		ControlValue newValue = value;
> +		switch (it->first->type()) {
>  		case ControlTypeInteger64:
> -			value.set<int64_t>(v4l2Ctrl->value64);
> +			newValue.set<int64_t>(v4l2Ctrl.value64);
>  			break;
>  
>  		case ControlTypeByte:
> @@ -550,11 +552,11 @@ void V4L2Device::updateControls(ControlList *ctrls,
>  			 * \todo To be changed when support for string controls
>  			 * will be added.
>  			 */
> -			value.set<int32_t>(v4l2Ctrl->value);
> +			newValue.set<int32_t>(v4l2Ctrl.value);
>  			break;
>  		}
>  
> -		i++;
> +		ctrls->set(v4l2Ctrl.id, newValue);

It's quite hard to distinguish what's going on in there.
Does this do more than just use a vector instead of an array ?

For instance, was ctrls.set() called previously in some other way which
isn't jumping out at me in the diff above?

Ah, I suspect previously, the 'value' was the ControlValue and it was
already obtained because the search was through the ctrls list...



>  	}
>  }
>  
> 

-- 
Regards
--
Kieran