[libcamera-devel] [PATCH v5 2/4] libcamera: V4L2Device: Use std::vector for v4l2_ext_control in getControls()

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Fri, Apr 23, 2021 at 06:36:51PM +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 | 41 +++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index ce2860c4..bbe8f154 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -173,13 +173,18 @@ 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_ };
> +	std::vector<v4l2_ext_control> v4l2Ctrls(ids.size());
> +	memset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size());
> +
> +	for (size_t i = 0, j = 0; j < ids.size(); ++j) {
> +		const unsigned int id = ids[j];
> +		if (ctrls.contains(id))
> +			continue;
>  
> -	for (uint32_t id : ids) {
>  		const auto iter = controls_.find(id);
>  		if (iter == controls_.end()) {
>  			LOG(V4L2, Error)
> @@ -187,20 +192,12 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
>  			return {};
>  		}
>  
> -		ctrls.set(id, {});
> -	}
> -
> -	struct v4l2_ext_control v4l2Ctrls[count];
> -	memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));
> -
> -	unsigned int i = 0;
> -	for (auto &ctrl : ctrls) {
> -		unsigned int id = ctrl.first;
> +		v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i++];

You increase i here, ...

>  		const struct v4l2_query_ext_ctrl &info = controlInfo_[id];
> +		ControlValue value{};

ControlValue has a default constructor, no need for {}.

>  
>  		if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
>  			ControlType type;
> -
>  			switch (info.type) {
>  			case V4L2_CTRL_TYPE_U8:
>  				type = ControlTypeByte;
> @@ -213,7 +210,6 @@ 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();
>  
> @@ -221,21 +217,23 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
>  			v4l2Ctrls[i].size = data.size();

... and use it here. I don't think that's correct.

>  		}
>  
> -		v4l2Ctrls[i].id = id;
> -		i++;
> +		v4l2Ctrl.id = id;

Should we move this just after the declaration of v4l2Ctrl above ?

> +		ctrls.set(id, std::move(value));

v4l2Ctrls contains pointers to data stored in the ControlValue
instances. As far as I can tell the pointers will remain valid, but
that's very dependent on the internals of ControlList.

To be honest, I'm not very fond of patches 1/4 and 2/4 in this series.
They make the code more fragile and possibly a bit less efficient
(although that's likely not significant, as there shouldn't be thousands
of controls in the list). The VLA removal is fine, but the rest doesn't
bring much value in my opinion.

Let's split this in two parts, with the fixes first, and the rework on
top, so both can be discussed separately. I've posted the former as a
v6.

>  	}
>  
> +	v4l2Ctrls.resize(ctrls.size());
> +
>  	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 {};
> @@ -244,10 +242,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());
>  
>  	return ctrls;
>  }

-- 
Regards,

Laurent Pinchart