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

[Laurent Pinchart]

On Thu, Apr 22, 2021 at 09:50:46AM +0200, Jacopo Mondi wrote:
> Hi Hiro,
> 
> Interesting https://stackoverflow.com/a/21519063

"An aesthetic or correct way of invoking an action that requires DOM
manipulation as well as controller related tasks in EmberJS"

Please don't tell me you plan to rewrite libcamera in JS :-)

> 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

In a previous reply, Hiro mentioned that the zero-initialization will
only initialize the first member of unions. memset() is possibly safer.

> >
> > -	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

I've posted "[PATCH/RFC 1/3] libcamera: utils: Add enumerate view for
range-based for loops" which would allow writing this as

	for (auto &[i, ctrl] = utils::enumerate(ctrls)) {

It's not a blocker though, we can merge this series and rework it later.
I'd however appreciate feedback on the above patch (and the rest of the
series).

> > +		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;
> > -

I really think those blank lines help readability.

> >  			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 ?

That's what I replied to 1/4 :-) I'd have a small preference for doing
it in 1/4 as that's where the restriction is removed, or at least in a
patch inserted before 1/4 and 2/4.

> >  	}
> >
> >  	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;
> >  }

-- 
Regards,

Laurent Pinchart