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

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Thu, Apr 15, 2021 at 01:48:52PM +0900, 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 | 43 ++++++++++++++++-------------------
>  1 file changed, 20 insertions(+), 23 deletions(-)
> 
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 8fd79934..ee8c3fed 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)

Unrelated to this patch, it occurred to me that the function should take
a Span<const uint32_t> instead of a vector. Would you like to submit an
additional patch ?

>  {
> -	unsigned int count = ids.size();
> -	if (count == 0)
> +	if (ids.empty())
>  		return {};
>  
>  	ControlList ctrls;
> @@ -196,22 +195,25 @@ 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());

This changes the behaviour slightly, in that if the ids vector contains
the same ID multiple times, you'll allocate less memory here. That's
actually better :-)

>  	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));
>  
>  		if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> -			ControlType type;
> +			ControlValue &value = ctrl.second;
>  
>  			switch (info.type) {
> -			case V4L2_CTRL_TYPE_U8:
> -				type = ControlTypeByte;
> +			case V4L2_CTRL_TYPE_U8: {
> +				value.reserve(ControlTypeByte, true, info.elems);
> +				Span<uint8_t> data = value.data();
> +				v4l2Ctrl.p_u8 = data.data();
> +				v4l2Ctrl.size = data.size();
>  				break;
> -
> +			}
>  			default:
>  				LOG(V4L2, Error)
>  					<< "Unsupported payload control type "
> @@ -219,29 +221,23 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
>  				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();

Is there a need to move this code to the switch ? It looks fairly
generic, it would be nice not to duplicate it when (if) we latter add
support for other control types (such as V4L2_CTRL_TYPE_U16).

>  		}
>  
> -		v4l2Ctrls[i].id = id;
> -		i++;
> +		v4l2Ctrl.id = id;
> +		v4l2Ctrls.push_back(std::move(v4l2Ctrl));

The move won't help much, it will copy the v4l2_ext_control instance.
Should we resize() instead of reserve(), and operate directly on the
vector entry in this loop ?

>  	}
>  
>  	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 +246,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());

updateControls should also take a Span argument, that could be done in
the same patch that updates the argument to getControls().

Overall this patch is good, the comments above are small issues.

>  
>  	return ctrls;
>  }
-- 
Regards,

Laurent Pinchart