[libcamera-devel] [PATCH v4 4/4] libcamera: V4L2Device: Use Span in updateControls()

[Jacopo Mondi]

Hello,

On Fri, Apr 23, 2021 at 01:42:00PM +0900, Hirokazu Honda wrote:
> On Fri, Apr 23, 2021 at 12:03 PM Laurent Pinchart
> <laurent.pinchart at ideasonboard.com> wrote:
> >
> > Hi Jacopo,
> >
> > On Thu, Apr 22, 2021 at 09:52:21AM +0200, Jacopo Mondi wrote:
> > > On Thu, Apr 22, 2021 at 11:18:09AM +0900, Hirokazu Honda wrote:
> > > > V4L2Device::updateControls() takes two arguments, raw array and
> > > > its size, for the v4l2_ext_control values. This replaces it with
> > > > libcamera::Span.
> > >
> > > Now that v4l2Ctrls is an std::vector<>, can't updateControls() take a
> > > reference to it, instead of going through a Span<> ?
> >
> > That's what v3 did, and I proposed using span...
> >
>
> Yep, mail-based review doesn't easily show us the review comments in
> the previous version. :(
> That's why I prefer some web app review site like GitHub or Gerrit,
> which most of you don't like. :p
>
> Either Span or vector is fine to me. Please feel free to request me.

I welcome Span<> when it allows us to wrap a memory location and its
size is a convenient way, but what do we gain here where we already
have a container ?

>
> -Hiro
> > > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > > ---
> > > >  include/libcamera/internal/v4l2_device.h |  4 ++--
> > > >  src/libcamera/v4l2_device.cpp            | 11 ++++-------
> > > >  2 files changed, 6 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> > > > index d006bf68..087f07e7 100644
> > > > --- a/include/libcamera/internal/v4l2_device.h
> > > > +++ b/include/libcamera/internal/v4l2_device.h
> > > > @@ -14,6 +14,7 @@
> > > >  #include <linux/videodev2.h>
> > > >
> > > >  #include <libcamera/signal.h>
> > > > +#include <libcamera/span.h>
> > > >
> > > >  #include "libcamera/internal/log.h"
> > > >  #include "libcamera/internal/v4l2_controls.h"
> > > > @@ -55,8 +56,7 @@ protected:
> > > >  private:
> > > >     void listControls();
> > > >     void updateControls(ControlList *ctrls,
> > > > -                       const struct v4l2_ext_control *v4l2Ctrls,
> > > > -                       unsigned int count);
> > > > +                       Span<const v4l2_ext_control> v4l2Ctrls);
> > > >
> > > >     void eventAvailable(EventNotifier *notifier);
> > > >
> > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > > > index 9f71bf0e..58516609 100644
> > > > --- a/src/libcamera/v4l2_device.cpp
> > > > +++ b/src/libcamera/v4l2_device.cpp
> > > > @@ -244,7 +244,7 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
> > > >             v4l2Ctrls.resize(errorIdx);
> > > >     }
> > > >
> > > > -   updateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());
> > > > +   updateControls(&ctrls, v4l2Ctrls);
> > > >
> > > >     return ctrls;
> > > >  }
> > > > @@ -343,7 +343,7 @@ int V4L2Device::setControls(ControlList *ctrls)
> > > >             ret = errorIdx;
> > > >     }
> > > >
> > > > -   updateControls(ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());
> > > > +   updateControls(ctrls, v4l2Ctrls);
> > > >
> > > >     return ret;
> > > >  }
> > > > @@ -507,14 +507,11 @@ 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)
> > > > +                           Span<const v4l2_ext_control> v4l2Ctrls)
> > > >  {
> > > > -   for (unsigned int i = 0; i < count; i++) {
> > > > -           const struct v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i];
> > > > +   for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) {
> > > >             const unsigned int id = v4l2Ctrl.id;
> > > >             if (!ctrls->contains(id)) {
> > > >                     LOG(V4L2, Error) << "ControlList doesn't contain id: "
> >
> > --
> > Regards,
> >
> > Laurent Pinchart