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

[Jacopo Mondi]

On Fri, Apr 23, 2021 at 06:05:50AM +0300, Laurent Pinchart wrote:
> 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"

What the heck! I don't even know what EmberJS is!  I clicked on the
'share' button of stackoverflow post about VLA in C++ and that's the
link I get. Didn't know "share" actually meant "share a random
stackoverflow thread"  :)

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

Wherever works !

Thanks
   j

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