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

[Laurent Pinchart]

Hi Hiro,

On Mon, Apr 26, 2021 at 11:01:11AM +0900, Hirokazu Honda wrote:
> On Mon, Apr 26, 2021 at 9:12 AM Laurent Pinchart wrote:
> > 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.
> 
> Can you clarify more how the new code is fragile and less efficient?
> You're right, I am sorry that this implementation has some problems.
> But I think 1/4 is worth doing and correct, and 2/4 is good if I fix the bugs?

The efficiency is related to the additional lookup in updateControls()
(the O(N*log(N)) we've discussed previously). I think it's a theoretical
problem only, given the expected size of the control list, I expect the
difference to be completely negligible in practice. As for the
fragility, I was referring to the fact that we store ub v4l2Ctrls
pointers to data from the ControlValue, stored in the ControlList. Any
reallocation in the underlying container would make those pointers
invalid. It's an existing issue, but with the current implementation, we
don't touch the ControlList after adding controls to it at the beginning
of getControls(), while with this patch we rely on

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

to not cause a reallocation. I think that's guaranteed by the current
implementation of ControlList, but if that changes later, we'll possibly
have hard to debug bugs.

These two points are not blocker by themselves, but the gains should
outweight the risks. It will be easier to check that after rebasing
patches 1/4 and 2/4 on top of the VLA removal.

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