[libcamera-devel] [PATCH 1/3] libcamera: V4L2Device: Use std::vector for v4l2_ext_control in getControls()

[Kieran Bingham]

Hi Hiro,

On 13/04/2021 07:19, Hirokazu Honda wrote:
> The original code uses Variable-Length-Array, which is not
> officially supported in C++. This replaces the array with
> std::vector.
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  src/libcamera/v4l2_device.cpp | 44 +++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index decd19ef..78c289c2 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -173,8 +173,7 @@ void V4L2Device::close()
>   */
>  ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
>  {
> -	unsigned int count = ids.size();
> -	if (count == 0)
> +	if (ids.empty())
>  		return {};
>  
>  	ControlList ctrls{ controls_ };
> @@ -196,21 +195,26 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
>  		ctrls.set(id, {});
>  	}
>  
> -	struct v4l2_ext_control v4l2Ctrls[count];
> -	memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));
> -
> -	unsigned int i = 0;
> +	std::vector<v4l2_ext_control> v4l2Ctrls;
> +	v4l2Ctrls.reserve(ctrls.size());

The v4l2ctrls array is created of size count which was ids.size();
Shouldn't we reserve this as ids.size() here instead of ctrls.size()?


>  	for (auto &ctrl : ctrls) {
>  		unsigned int id = ctrl.first;
>  		const struct v4l2_query_ext_ctrl &info = controlInfo_[id];
> +		v4l2_ext_control v4l2Ctrl;
> +		memset(&v4l2Ctrl, 0, sizeof(v4l2Ctrl));

Can v4l2Ctrl initialisation be simplified to this?:
	v4l2_ext_control v4l2Ctrl = {};

>  
>  		if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> -			ControlType type;
> -
>  			switch (info.type) {
> -			case V4L2_CTRL_TYPE_U8:
> +			case V4L2_CTRL_TYPE_U8: {
> +				ControlType type;
>  				type = ControlTypeByte;
> +				ControlValue &value = ctrl.second;
> +				value.reserve(type, true, info.elems);

Is this the only usage of type? Couldn't we just do
 value.reserve(ControlTypeByte, true, info.elems);

Or at the least make it

ControlType = ControlTypeByte;

rather than use an extra line.


> +				Span<uint8_t> data = value.data();
> +				v4l2Ctrl.p_u8 = data.data();
> +				v4l2Ctrl.size = data.size();
>  				break;
> +			}
>  
>  			default:
>  				LOG(V4L2, Error)
> @@ -218,30 +222,23 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
>  					<< info.type;
>  				return {};
>  			}
> -
> -			ControlValue &value = ctrl.second;
> -			value.reserve(type, true, info.elems);
> -			Span<uint8_t> data = value.data();
> -
> -			v4l2Ctrls[i].p_u8 = data.data();
> -			v4l2Ctrls[i].size = data.size();

That switch statement must have been arranged like that to facilitate
easily adding other types later.

If we don't need to do that, can we fix this up to lower the indentation
somehow? We're four levels in here in places :-S


>  		}
>  
> -		v4l2Ctrls[i].id = id;
> -		i++;
> +		v4l2Ctrl.id = id;
> +		v4l2Ctrls.push_back(std::move(v4l2Ctrl));
>  	}
>  
>  	struct v4l2_ext_controls v4l2ExtCtrls = {};
>  	v4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> -	v4l2ExtCtrls.controls = v4l2Ctrls;
> -	v4l2ExtCtrls.count = count;
> +	v4l2ExtCtrls.controls = v4l2Ctrls.data();
> +	v4l2ExtCtrls.count = v4l2Ctrls.size();
>  
>  	int ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls);
>  	if (ret) {
>  		unsigned int errorIdx = v4l2ExtCtrls.error_idx;
>  
>  		/* Generic validation error. */
> -		if (errorIdx == 0 || errorIdx >= count) {
> +		if (errorIdx == 0 || errorIdx >= v4l2Ctrls.size()) {
>  			LOG(V4L2, Error) << "Unable to read controls: "
>  					 << strerror(-ret);
>  			return {};
> @@ -250,10 +247,11 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
>  		/* A specific control failed. */
>  		LOG(V4L2, Error) << "Unable to read control " << errorIdx
>  				 << ": " << strerror(-ret);
> -		count = errorIdx - 1;
> +
> +		v4l2Ctrls.resize(errorIdx);
>  	}
>  
> -	updateControls(&ctrls, v4l2Ctrls, count);
> +	updateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());

I was going to say can we pass v4l2Ctrls directly, but now I've seen
your later patch ;=)



>  
>  	return ctrls;
>  }
> 

-- 
Regards
--
Kieran