[libcamera-devel] [PATCH 3/3] libcamera: V4L2Device: Use std::vector in updateControls()

[Hirokazu Honda]

Hi Kieran,

On Wed, Apr 14, 2021 at 7:32 AM Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On 13/04/2021 07:19, Hirokazu Honda wrote:
> > V4L2Device::updateControls() takes two arguments, raw array and
> > its size, for the v4l2_ext_control values. This replaces it with
> > std::vector.
>
> This patch does more than just replace the raw array for the vector
> like-for-like.
>
> It also changes the processing of how the controls are searched and
> iterated.
>
> That should be documented here, if it's still appropriate - though to
> ease things - it might be clearer/easier to have the change to a vector
> directly, and then fix up the parsing if that's a specific improvement.
>
>
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> >  include/libcamera/internal/v4l2_device.h |  3 +-
> >  src/libcamera/v4l2_device.cpp            | 36 +++++++++++++-----------
> >  2 files changed, 20 insertions(+), 19 deletions(-)
> >
> > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> > index d006bf68..4cce3840 100644
> > --- a/include/libcamera/internal/v4l2_device.h
> > +++ b/include/libcamera/internal/v4l2_device.h
> > @@ -55,8 +55,7 @@ protected:
> >  private:
> >       void listControls();
> >       void updateControls(ControlList *ctrls,
> > -                         const struct v4l2_ext_control *v4l2Ctrls,
> > -                         unsigned int count);
> > +                         const std::vector<v4l2_ext_control> &v4l2Ctrls);
> >
> >       void eventAvailable(EventNotifier *notifier);
> >
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 8625dde8..8f29cd7f 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -251,7 +251,7 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
> >               v4l2Ctrls.resize(errorIdx);
> >       }
> >
> > -     updateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());
> > +     updateControls(&ctrls, v4l2Ctrls);
> >
> >       return ctrls;
> >  }
> > @@ -353,7 +353,7 @@ int V4L2Device::setControls(ControlList *ctrls)
> >               ret = errorIdx;
> >       }
> >
> > -     updateControls(ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());
> > +     updateControls(ctrls, v4l2Ctrls);
> >
> >       return ret;
> >  }
> > @@ -517,25 +517,27 @@ void V4L2Device::listControls()
> >   * values in \a v4l2Ctrls
> >   * \param[inout] ctrls List of V4L2 controls to update
> >   * \param[in] v4l2Ctrls List of V4L2 extended controls as returned by the driver
> > - * \param[in] count The number of controls to update
> >   */
> >  void V4L2Device::updateControls(ControlList *ctrls,
> > -                             const struct v4l2_ext_control *v4l2Ctrls,
> > -                             unsigned int count)
> > +                             const std::vector<v4l2_ext_control> &v4l2Ctrls)
> >  {
> > -     unsigned int i = 0;
> > -     for (auto &ctrl : *ctrls) {
> > -             if (i == count)
> > -                     break;
> > +     for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) {
> > +             if (!ctrls->contains(v4l2Ctrl.id)) {
> > +                     LOG(V4L2, Error) << "Unknown id: " << v4l2Ctrl.id;
> > +                     continue;
> > +             }
>
> Hrm, are these extra validation checks?
> or is it because we've inverted the search now?
>
> > -             const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> > -             unsigned int id = ctrl.first;
> > -             ControlValue &value = ctrl.second;
> > +             const auto it = controls_.find(v4l2Ctrl.id);
> > +             if (it == controls_.end()) {
> > +                     LOG(V4L2, Error) << "Unknown id: " << v4l2Ctrl.id;
> > +                     continue;
> > +             }
> >
> > -             const auto iter = controls_.find(id);
> > -             switch (iter->first->type()) {
> > +             const ControlValue &value = ctrls->get(v4l2Ctrl.id);
> > +             ControlValue newValue = value;
> > +             switch (it->first->type()) {
> >               case ControlTypeInteger64:
> > -                     value.set<int64_t>(v4l2Ctrl->value64);
> > +                     newValue.set<int64_t>(v4l2Ctrl.value64);
> >                       break;
> >
> >               case ControlTypeByte:
> > @@ -550,11 +552,11 @@ void V4L2Device::updateControls(ControlList *ctrls,
> >                        * \todo To be changed when support for string controls
> >                        * will be added.
> >                        */
> > -                     value.set<int32_t>(v4l2Ctrl->value);
> > +                     newValue.set<int32_t>(v4l2Ctrl.value);
> >                       break;
> >               }
> >
> > -             i++;
> > +             ctrls->set(v4l2Ctrl.id, newValue);
>
> It's quite hard to distinguish what's going on in there.
> Does this do more than just use a vector instead of an array ?
>
> For instance, was ctrls.set() called previously in some other way which
> isn't jumping out at me in the diff above?
>
> Ah, I suspect previously, the 'value' was the ControlValue and it was
> already obtained because the search was through the ctrls list...
>

The problem is ControlList() somehow allows to return a reference or
pointer via begin-end, but somehow doesn't via find or get.
It is very strange that I can only change it by get & set if without begin-end.
I separate the patch for the change except vector-array change.
PTAL https://patchwork.libcamera.org/patch/11934/

Best Regards,
-Hiro

>
>
> >       }
> >  }
> >
> >
>
> --
> Regards
> --
> Kieran