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

[Jacopo Mondi]

Hi Hiro,

Interesting https://stackoverflow.com/a/21519063

I initially thougth replacing VLA in C++ code was mostly a matter of
style...

On Thu, Apr 22, 2021 at 11:18:07AM +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.

One thing that always bothered me of this class is

	std::map<unsigned int, struct v4l2_query_ext_ctrl> controlInfo_;
	std::vector<std::unique_ptr<V4L2ControlId>> controlIds_;

everytime I see these identifiers I forget they refer to v4l2 stuff.
Should they be renamed ?

>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  src/libcamera/v4l2_device.cpp | 31 ++++++++++++++-----------------
>  1 file changed, 14 insertions(+), 17 deletions(-)
>
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 3c32d682..617e6ec9 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_ };
> @@ -190,22 +189,20 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
>  		ctrls.set(id, {});
>  	}
>
> -	struct v4l2_ext_control v4l2Ctrls[count];
> -	memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));
> +	std::vector<v4l2_ext_control> v4l2Ctrls(ctrls.size());
> +	memset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size());

Do you need the memset ?

I read:

        explicit vector( size_type count );

        4) Constructs the container with count default-inserted instances of T. No copies are made.

"default inserted instances of T" makes me wonder if a
default-inserted v4l2_ext_control is actually zeroed

I wonder if using reserve() would be better

>
> -	unsigned int i = 0;
> -	for (auto &ctrl : ctrls) {
> -		unsigned int id = ctrl.first;
> +	for (auto [ctrl, i] = std::pair(ctrls.begin(), 0u); i < ctrls.size(); ctrl++, i++) {

C++ is the language of the beast.
Neat though

> +		const unsigned int id = ctrl->first;
>  		const struct v4l2_query_ext_ctrl &info = controlInfo_[id];
> +		v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i];
>
>  		if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
>  			ControlType type;
> -
>  			switch (info.type) {
>  			case V4L2_CTRL_TYPE_U8:
>  				type = ControlTypeByte;
>  				break;
> -
>  			default:
>  				LOG(V4L2, Error)
>  					<< "Unsupported payload control type "
> @@ -213,7 +210,7 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
>  				return {};
>  			}
>
> -			ControlValue &value = ctrl.second;
> +			ControlValue &value = ctrl->second;
>  			value.reserve(type, true, info.elems);
>  			Span<uint8_t> data = value.data();
>
> @@ -221,21 +218,20 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
>  			v4l2Ctrls[i].size = data.size();
>  		}
>
> -		v4l2Ctrls[i].id = id;
> -		i++;
> +		v4l2Ctrl.id = id;

However, shouldn't we try to squeeze the two loops (the one that
initialize ctrls and this one) in a single one now that the ordering
restrictions is gone ?

>  	}
>
>  	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 +240,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;
>  }
> --
> 2.31.1.368.gbe11c130af-goog
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel