[{"id":16503,"web_url":"https://patchwork.libcamera.org/comment/16503/","msgid":"<20210422075221.ih42pbyel6jimpjm@uno.localdomain>","date":"2021-04-22T07:52:21","subject":"Re: [libcamera-devel] [PATCH v4 4/4] libcamera: V4L2Device: Use\n\tSpan in updateControls()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Thu, Apr 22, 2021 at 11:18:09AM +0900, Hirokazu Honda wrote:\n> V4L2Device::updateControls() takes two arguments, raw array and\n> its size, for the v4l2_ext_control values. This replaces it with\n> libcamera::Span.\n\nNow that v4l2Ctrls is an std::vector<>, can't updateControls() take a\nreference to it, instead of going through a Span<> ?\n\nThanks\n   j\n\n>\n> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> ---\n>  include/libcamera/internal/v4l2_device.h |  4 ++--\n>  src/libcamera/v4l2_device.cpp            | 11 ++++-------\n>  2 files changed, 6 insertions(+), 9 deletions(-)\n>\n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index d006bf68..087f07e7 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -14,6 +14,7 @@\n>  #include <linux/videodev2.h>\n>\n>  #include <libcamera/signal.h>\n> +#include <libcamera/span.h>\n>\n>  #include \"libcamera/internal/log.h\"\n>  #include \"libcamera/internal/v4l2_controls.h\"\n> @@ -55,8 +56,7 @@ protected:\n>  private:\n>  \tvoid listControls();\n>  \tvoid updateControls(ControlList *ctrls,\n> -\t\t\t    const struct v4l2_ext_control *v4l2Ctrls,\n> -\t\t\t    unsigned int count);\n> +\t\t\t    Span<const v4l2_ext_control> v4l2Ctrls);\n>\n>  \tvoid eventAvailable(EventNotifier *notifier);\n>\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 9f71bf0e..58516609 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -244,7 +244,7 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)\n>  \t\tv4l2Ctrls.resize(errorIdx);\n>  \t}\n>\n> -\tupdateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());\n> +\tupdateControls(&ctrls, v4l2Ctrls);\n>\n>  \treturn ctrls;\n>  }\n> @@ -343,7 +343,7 @@ int V4L2Device::setControls(ControlList *ctrls)\n>  \t\tret = errorIdx;\n>  \t}\n>\n> -\tupdateControls(ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());\n> +\tupdateControls(ctrls, v4l2Ctrls);\n>\n>  \treturn ret;\n>  }\n> @@ -507,14 +507,11 @@ void V4L2Device::listControls()\n>   * values in \\a v4l2Ctrls\n>   * \\param[inout] ctrls List of V4L2 controls to update\n>   * \\param[in] v4l2Ctrls List of V4L2 extended controls as returned by the driver\n> - * \\param[in] count The number of controls to update\n>   */\n>  void V4L2Device::updateControls(ControlList *ctrls,\n> -\t\t\t\tconst struct v4l2_ext_control *v4l2Ctrls,\n> -\t\t\t\tunsigned int count)\n> +\t\t\t\tSpan<const v4l2_ext_control> v4l2Ctrls)\n>  {\n> -\tfor (unsigned int i = 0; i < count; i++) {\n> -\t\tconst struct v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i];\n> +\tfor (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) {\n>  \t\tconst unsigned int id = v4l2Ctrl.id;\n>  \t\tif (!ctrls->contains(id)) {\n>  \t\t\tLOG(V4L2, Error) << \"ControlList doesn't contain id: \"\n> --\n> 2.31.1.368.gbe11c130af-goog\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CBE45BDB15\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 22 Apr 2021 07:51:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9163B68851;\n\tThu, 22 Apr 2021 09:51:43 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 75E4968847\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 22 Apr 2021 09:51:41 +0200 (CEST)","from uno.localdomain (host-82-57-193-192.retail.telecomitalia.it\n\t[82.57.193.192]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id A7277C0013;\n\tThu, 22 Apr 2021 07:51:40 +0000 (UTC)"],"X-Originating-IP":"82.57.193.192","Date":"Thu, 22 Apr 2021 09:52:21 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Hirokazu Honda <hiroh@chromium.org>","Message-ID":"<20210422075221.ih42pbyel6jimpjm@uno.localdomain>","References":"<20210422021809.520675-1-hiroh@chromium.org>\n\t<20210422021809.520675-5-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210422021809.520675-5-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v4 4/4] libcamera: V4L2Device: Use\n\tSpan in updateControls()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16521,"web_url":"https://patchwork.libcamera.org/comment/16521/","msgid":"<YII499Id/IX9uJyV@pendragon.ideasonboard.com>","date":"2021-04-23T03:03:19","subject":"Re: [libcamera-devel] [PATCH v4 4/4] libcamera: V4L2Device: Use\n\tSpan in updateControls()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Apr 22, 2021 at 09:52:21AM +0200, Jacopo Mondi wrote:\n> On Thu, Apr 22, 2021 at 11:18:09AM +0900, Hirokazu Honda wrote:\n> > V4L2Device::updateControls() takes two arguments, raw array and\n> > its size, for the v4l2_ext_control values. This replaces it with\n> > libcamera::Span.\n> \n> Now that v4l2Ctrls is an std::vector<>, can't updateControls() take a\n> reference to it, instead of going through a Span<> ?\n\nThat's what v3 did, and I proposed using span...\n\n> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > ---\n> >  include/libcamera/internal/v4l2_device.h |  4 ++--\n> >  src/libcamera/v4l2_device.cpp            | 11 ++++-------\n> >  2 files changed, 6 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > index d006bf68..087f07e7 100644\n> > --- a/include/libcamera/internal/v4l2_device.h\n> > +++ b/include/libcamera/internal/v4l2_device.h\n> > @@ -14,6 +14,7 @@\n> >  #include <linux/videodev2.h>\n> >\n> >  #include <libcamera/signal.h>\n> > +#include <libcamera/span.h>\n> >\n> >  #include \"libcamera/internal/log.h\"\n> >  #include \"libcamera/internal/v4l2_controls.h\"\n> > @@ -55,8 +56,7 @@ protected:\n> >  private:\n> >  \tvoid listControls();\n> >  \tvoid updateControls(ControlList *ctrls,\n> > -\t\t\t    const struct v4l2_ext_control *v4l2Ctrls,\n> > -\t\t\t    unsigned int count);\n> > +\t\t\t    Span<const v4l2_ext_control> v4l2Ctrls);\n> >\n> >  \tvoid eventAvailable(EventNotifier *notifier);\n> >\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 9f71bf0e..58516609 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -244,7 +244,7 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)\n> >  \t\tv4l2Ctrls.resize(errorIdx);\n> >  \t}\n> >\n> > -\tupdateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());\n> > +\tupdateControls(&ctrls, v4l2Ctrls);\n> >\n> >  \treturn ctrls;\n> >  }\n> > @@ -343,7 +343,7 @@ int V4L2Device::setControls(ControlList *ctrls)\n> >  \t\tret = errorIdx;\n> >  \t}\n> >\n> > -\tupdateControls(ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());\n> > +\tupdateControls(ctrls, v4l2Ctrls);\n> >\n> >  \treturn ret;\n> >  }\n> > @@ -507,14 +507,11 @@ void V4L2Device::listControls()\n> >   * values in \\a v4l2Ctrls\n> >   * \\param[inout] ctrls List of V4L2 controls to update\n> >   * \\param[in] v4l2Ctrls List of V4L2 extended controls as returned by the driver\n> > - * \\param[in] count The number of controls to update\n> >   */\n> >  void V4L2Device::updateControls(ControlList *ctrls,\n> > -\t\t\t\tconst struct v4l2_ext_control *v4l2Ctrls,\n> > -\t\t\t\tunsigned int count)\n> > +\t\t\t\tSpan<const v4l2_ext_control> v4l2Ctrls)\n> >  {\n> > -\tfor (unsigned int i = 0; i < count; i++) {\n> > -\t\tconst struct v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i];\n> > +\tfor (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) {\n> >  \t\tconst unsigned int id = v4l2Ctrl.id;\n> >  \t\tif (!ctrls->contains(id)) {\n> >  \t\t\tLOG(V4L2, Error) << \"ControlList doesn't contain id: \"","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 53C7BBDB15\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 23 Apr 2021 03:03:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D202C6886D;\n\tFri, 23 Apr 2021 05:03:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 17A3360513\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 23 Apr 2021 05:03:25 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 81539EE;\n\tFri, 23 Apr 2021 05:03:24 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qPcPBtAu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619147004;\n\tbh=kBkfBSrmCRxrr433cP/sYvt45rftTijh6gfjaj713uk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qPcPBtAuPOv89LpV9ofS+Tg6sRycFTR+dSBFV686+5VNQ7VixNljWJSWLGNe642k9\n\tLmC7gjIUgVrOrYvYyAHWvVTzbPm1g4YflmSVe5NIvs3n+ikGpZNEbS4XP7ZPiEGUUw\n\t7AuxjRyzZ5ErLxsnlCiCQ5uKAJzc1iQN3S4oyPO4=","Date":"Fri, 23 Apr 2021 06:03:19 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<YII499Id/IX9uJyV@pendragon.ideasonboard.com>","References":"<20210422021809.520675-1-hiroh@chromium.org>\n\t<20210422021809.520675-5-hiroh@chromium.org>\n\t<20210422075221.ih42pbyel6jimpjm@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210422075221.ih42pbyel6jimpjm@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v4 4/4] libcamera: V4L2Device: Use\n\tSpan in updateControls()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16526,"web_url":"https://patchwork.libcamera.org/comment/16526/","msgid":"<CAO5uPHPCJMpa4V6vKOLj6N9T2gGTpZSxP45eBGzyQTjBL4rZFA@mail.gmail.com>","date":"2021-04-23T04:42:00","subject":"Re: [libcamera-devel] [PATCH v4 4/4] libcamera: V4L2Device: Use\n\tSpan in updateControls()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Fri, Apr 23, 2021 at 12:03 PM Laurent Pinchart\n<laurent.pinchart@ideasonboard.com> wrote:\n>\n> Hi Jacopo,\n>\n> On Thu, Apr 22, 2021 at 09:52:21AM +0200, Jacopo Mondi wrote:\n> > On Thu, Apr 22, 2021 at 11:18:09AM +0900, Hirokazu Honda wrote:\n> > > V4L2Device::updateControls() takes two arguments, raw array and\n> > > its size, for the v4l2_ext_control values. This replaces it with\n> > > libcamera::Span.\n> >\n> > Now that v4l2Ctrls is an std::vector<>, can't updateControls() take a\n> > reference to it, instead of going through a Span<> ?\n>\n> That's what v3 did, and I proposed using span...\n>\n\nYep, mail-based review doesn't easily show us the review comments in\nthe previous version. :(\nThat's why I prefer some web app review site like GitHub or Gerrit,\nwhich most of you don't like. :p\n\nEither Span or vector is fine to me. Please feel free to request me.\n\n-Hiro\n> > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > > ---\n> > >  include/libcamera/internal/v4l2_device.h |  4 ++--\n> > >  src/libcamera/v4l2_device.cpp            | 11 ++++-------\n> > >  2 files changed, 6 insertions(+), 9 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > > index d006bf68..087f07e7 100644\n> > > --- a/include/libcamera/internal/v4l2_device.h\n> > > +++ b/include/libcamera/internal/v4l2_device.h\n> > > @@ -14,6 +14,7 @@\n> > >  #include <linux/videodev2.h>\n> > >\n> > >  #include <libcamera/signal.h>\n> > > +#include <libcamera/span.h>\n> > >\n> > >  #include \"libcamera/internal/log.h\"\n> > >  #include \"libcamera/internal/v4l2_controls.h\"\n> > > @@ -55,8 +56,7 @@ protected:\n> > >  private:\n> > >     void listControls();\n> > >     void updateControls(ControlList *ctrls,\n> > > -                       const struct v4l2_ext_control *v4l2Ctrls,\n> > > -                       unsigned int count);\n> > > +                       Span<const v4l2_ext_control> v4l2Ctrls);\n> > >\n> > >     void eventAvailable(EventNotifier *notifier);\n> > >\n> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > index 9f71bf0e..58516609 100644\n> > > --- a/src/libcamera/v4l2_device.cpp\n> > > +++ b/src/libcamera/v4l2_device.cpp\n> > > @@ -244,7 +244,7 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)\n> > >             v4l2Ctrls.resize(errorIdx);\n> > >     }\n> > >\n> > > -   updateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());\n> > > +   updateControls(&ctrls, v4l2Ctrls);\n> > >\n> > >     return ctrls;\n> > >  }\n> > > @@ -343,7 +343,7 @@ int V4L2Device::setControls(ControlList *ctrls)\n> > >             ret = errorIdx;\n> > >     }\n> > >\n> > > -   updateControls(ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());\n> > > +   updateControls(ctrls, v4l2Ctrls);\n> > >\n> > >     return ret;\n> > >  }\n> > > @@ -507,14 +507,11 @@ void V4L2Device::listControls()\n> > >   * values in \\a v4l2Ctrls\n> > >   * \\param[inout] ctrls List of V4L2 controls to update\n> > >   * \\param[in] v4l2Ctrls List of V4L2 extended controls as returned by the driver\n> > > - * \\param[in] count The number of controls to update\n> > >   */\n> > >  void V4L2Device::updateControls(ControlList *ctrls,\n> > > -                           const struct v4l2_ext_control *v4l2Ctrls,\n> > > -                           unsigned int count)\n> > > +                           Span<const v4l2_ext_control> v4l2Ctrls)\n> > >  {\n> > > -   for (unsigned int i = 0; i < count; i++) {\n> > > -           const struct v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i];\n> > > +   for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) {\n> > >             const unsigned int id = v4l2Ctrl.id;\n> > >             if (!ctrls->contains(id)) {\n> > >                     LOG(V4L2, Error) << \"ControlList doesn't contain id: \"\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 34002BDB15\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 23 Apr 2021 04:42:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9AC3E68872;\n\tFri, 23 Apr 2021 06:42:13 +0200 (CEST)","from mail-ej1-x629.google.com (mail-ej1-x629.google.com\n\t[IPv6:2a00:1450:4864:20::629])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7811468861\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 23 Apr 2021 06:42:12 +0200 (CEST)","by mail-ej1-x629.google.com with SMTP id r20so22247675ejo.11\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 22 Apr 2021 21:42:12 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"XVqPF5BR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=EvU4kQnKsL94jwlM0S03/JtGgzrPwgF8fe4G+NtFyfA=;\n\tb=XVqPF5BRG1qTL9Sw4JdCGkP7EpO3ovgib3U5+nhLEo34jXG+plMoJhMHaYn0PuUFGo\n\tuqAfh7snqPce0khmbzOftxJUu/cuEUqN+G4CB0LoLVQgoQ1bzZJ7EM3yYH94Rcr9njXm\n\txZ6Zf9r+dFv2guMN9sRGmX02qVH7k4QVVXjyY=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=EvU4kQnKsL94jwlM0S03/JtGgzrPwgF8fe4G+NtFyfA=;\n\tb=QU8wXQnINdfp92uF+JUZyO/t9UJNUL9CCqWxj4A8NXN21WHlNitkXvbiEAsy8ALRE5\n\t6v94edK/+JDXYB0a0WR7fzgkNXn04U30cQlfpSCzd8NNMYXqmzAbqt6xvugCc6MX9S3u\n\tTyiwZ44Xwr2I+/gShuICXWX5lKUlrZDDymgOoHqT22732A81H2XknlVlwvij6EVu74KU\n\t/2ofbrrA+lFwG6geHO1LMHYtp0RQUD/yqXCBv9Uv/aersV1gKPJOXAdxrf31oqxe1nVD\n\tbW45OTXWxSj5TGkZjUzWzQCn16g7GJMObkt08rVt6XgDCPPF4KvL4uFFxTdtpcUFxnwM\n\tsJeA==","X-Gm-Message-State":"AOAM532vd0NkMQAtRaUMl6/L+CNJh5bwSTWWMh/k7dhPC8EjBtqC6KAz\n\t9AgnNWo0ZiGd4PhDXTTlVyCX+cC0EnpWK3o1w48II4O6QbI=","X-Google-Smtp-Source":"ABdhPJwjH/TdTLNJlobaF9YHqVd7Bm2b72Bsb3HOrpif+5PFr3ejkCJ9fzQK8KTYz4+xYXltCswqOIqydw41Kp+A6FY=","X-Received":"by 2002:a17:906:13d6:: with SMTP id\n\tg22mr2074750ejc.475.1619152932077; \n\tThu, 22 Apr 2021 21:42:12 -0700 (PDT)","MIME-Version":"1.0","References":"<20210422021809.520675-1-hiroh@chromium.org>\n\t<20210422021809.520675-5-hiroh@chromium.org>\n\t<20210422075221.ih42pbyel6jimpjm@uno.localdomain>\n\t<YII499Id/IX9uJyV@pendragon.ideasonboard.com>","In-Reply-To":"<YII499Id/IX9uJyV@pendragon.ideasonboard.com>","From":"Hirokazu Honda <hiroh@chromium.org>","Date":"Fri, 23 Apr 2021 13:42:00 +0900","Message-ID":"<CAO5uPHPCJMpa4V6vKOLj6N9T2gGTpZSxP45eBGzyQTjBL4rZFA@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 4/4] libcamera: V4L2Device: Use\n\tSpan in updateControls()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16528,"web_url":"https://patchwork.libcamera.org/comment/16528/","msgid":"<20210423070533.br2jrgp42ftmf5di@uno.localdomain>","date":"2021-04-23T07:05:33","subject":"Re: [libcamera-devel] [PATCH v4 4/4] libcamera: V4L2Device: Use\n\tSpan in updateControls()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello,\n\nOn Fri, Apr 23, 2021 at 01:42:00PM +0900, Hirokazu Honda wrote:\n> On Fri, Apr 23, 2021 at 12:03 PM Laurent Pinchart\n> <laurent.pinchart@ideasonboard.com> wrote:\n> >\n> > Hi Jacopo,\n> >\n> > On Thu, Apr 22, 2021 at 09:52:21AM +0200, Jacopo Mondi wrote:\n> > > On Thu, Apr 22, 2021 at 11:18:09AM +0900, Hirokazu Honda wrote:\n> > > > V4L2Device::updateControls() takes two arguments, raw array and\n> > > > its size, for the v4l2_ext_control values. This replaces it with\n> > > > libcamera::Span.\n> > >\n> > > Now that v4l2Ctrls is an std::vector<>, can't updateControls() take a\n> > > reference to it, instead of going through a Span<> ?\n> >\n> > That's what v3 did, and I proposed using span...\n> >\n>\n> Yep, mail-based review doesn't easily show us the review comments in\n> the previous version. :(\n> That's why I prefer some web app review site like GitHub or Gerrit,\n> which most of you don't like. :p\n>\n> Either Span or vector is fine to me. Please feel free to request me.\n\nI welcome Span<> when it allows us to wrap a memory location and its\nsize is a convenient way, but what do we gain here where we already\nhave a container ?\n\n>\n> -Hiro\n> > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > > > ---\n> > > >  include/libcamera/internal/v4l2_device.h |  4 ++--\n> > > >  src/libcamera/v4l2_device.cpp            | 11 ++++-------\n> > > >  2 files changed, 6 insertions(+), 9 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > > > index d006bf68..087f07e7 100644\n> > > > --- a/include/libcamera/internal/v4l2_device.h\n> > > > +++ b/include/libcamera/internal/v4l2_device.h\n> > > > @@ -14,6 +14,7 @@\n> > > >  #include <linux/videodev2.h>\n> > > >\n> > > >  #include <libcamera/signal.h>\n> > > > +#include <libcamera/span.h>\n> > > >\n> > > >  #include \"libcamera/internal/log.h\"\n> > > >  #include \"libcamera/internal/v4l2_controls.h\"\n> > > > @@ -55,8 +56,7 @@ protected:\n> > > >  private:\n> > > >     void listControls();\n> > > >     void updateControls(ControlList *ctrls,\n> > > > -                       const struct v4l2_ext_control *v4l2Ctrls,\n> > > > -                       unsigned int count);\n> > > > +                       Span<const v4l2_ext_control> v4l2Ctrls);\n> > > >\n> > > >     void eventAvailable(EventNotifier *notifier);\n> > > >\n> > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > > index 9f71bf0e..58516609 100644\n> > > > --- a/src/libcamera/v4l2_device.cpp\n> > > > +++ b/src/libcamera/v4l2_device.cpp\n> > > > @@ -244,7 +244,7 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)\n> > > >             v4l2Ctrls.resize(errorIdx);\n> > > >     }\n> > > >\n> > > > -   updateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());\n> > > > +   updateControls(&ctrls, v4l2Ctrls);\n> > > >\n> > > >     return ctrls;\n> > > >  }\n> > > > @@ -343,7 +343,7 @@ int V4L2Device::setControls(ControlList *ctrls)\n> > > >             ret = errorIdx;\n> > > >     }\n> > > >\n> > > > -   updateControls(ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());\n> > > > +   updateControls(ctrls, v4l2Ctrls);\n> > > >\n> > > >     return ret;\n> > > >  }\n> > > > @@ -507,14 +507,11 @@ void V4L2Device::listControls()\n> > > >   * values in \\a v4l2Ctrls\n> > > >   * \\param[inout] ctrls List of V4L2 controls to update\n> > > >   * \\param[in] v4l2Ctrls List of V4L2 extended controls as returned by the driver\n> > > > - * \\param[in] count The number of controls to update\n> > > >   */\n> > > >  void V4L2Device::updateControls(ControlList *ctrls,\n> > > > -                           const struct v4l2_ext_control *v4l2Ctrls,\n> > > > -                           unsigned int count)\n> > > > +                           Span<const v4l2_ext_control> v4l2Ctrls)\n> > > >  {\n> > > > -   for (unsigned int i = 0; i < count; i++) {\n> > > > -           const struct v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i];\n> > > > +   for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) {\n> > > >             const unsigned int id = v4l2Ctrl.id;\n> > > >             if (!ctrls->contains(id)) {\n> > > >                     LOG(V4L2, Error) << \"ControlList doesn't contain id: \"\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A95AFBDB15\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 23 Apr 2021 07:04:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1D44068877;\n\tFri, 23 Apr 2021 09:04:55 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5FA3E6886E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 23 Apr 2021 09:04:54 +0200 (CEST)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id B44F0C000D;\n\tFri, 23 Apr 2021 07:04:53 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Fri, 23 Apr 2021 09:05:33 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Hirokazu Honda <hiroh@chromium.org>","Message-ID":"<20210423070533.br2jrgp42ftmf5di@uno.localdomain>","References":"<20210422021809.520675-1-hiroh@chromium.org>\n\t<20210422021809.520675-5-hiroh@chromium.org>\n\t<20210422075221.ih42pbyel6jimpjm@uno.localdomain>\n\t<YII499Id/IX9uJyV@pendragon.ideasonboard.com>\n\t<CAO5uPHPCJMpa4V6vKOLj6N9T2gGTpZSxP45eBGzyQTjBL4rZFA@mail.gmail.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<CAO5uPHPCJMpa4V6vKOLj6N9T2gGTpZSxP45eBGzyQTjBL4rZFA@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v4 4/4] libcamera: V4L2Device: Use\n\tSpan in updateControls()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16538,"web_url":"https://patchwork.libcamera.org/comment/16538/","msgid":"<YIX+Y8QT/LsfY21F@pendragon.ideasonboard.com>","date":"2021-04-25T23:42:27","subject":"Re: [libcamera-devel] [PATCH v4 4/4] libcamera: V4L2Device: Use\n\tSpan in updateControls()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Apr 23, 2021 at 09:05:33AM +0200, Jacopo Mondi wrote:\n> On Fri, Apr 23, 2021 at 01:42:00PM +0900, Hirokazu Honda wrote:\n> > On Fri, Apr 23, 2021 at 12:03 PM Laurent Pinchart wrote:\n> > > On Thu, Apr 22, 2021 at 09:52:21AM +0200, Jacopo Mondi wrote:\n> > > > On Thu, Apr 22, 2021 at 11:18:09AM +0900, Hirokazu Honda wrote:\n> > > > > V4L2Device::updateControls() takes two arguments, raw array and\n> > > > > its size, for the v4l2_ext_control values. This replaces it with\n> > > > > libcamera::Span.\n> > > >\n> > > > Now that v4l2Ctrls is an std::vector<>, can't updateControls() take a\n> > > > reference to it, instead of going through a Span<> ?\n> > >\n> > > That's what v3 did, and I proposed using span...\n> >\n> > Yep, mail-based review doesn't easily show us the review comments in\n> > the previous version. :(\n> > That's why I prefer some web app review site like GitHub or Gerrit,\n> > which most of you don't like. :p\n> >\n> > Either Span or vector is fine to me. Please feel free to request me.\n> \n> I welcome Span<> when it allows us to wrap a memory location and its\n> size is a convenient way, but what do we gain here where we already\n> have a container ?\n\nGenerally speaking, when a function gets a contiguous array of elements\nwith a size, Span is more generic, as other containers than\nstd::vector<> could be used.\n\nIn this case, you're right that it makes no difference. updateControls()\nis an internal API. Making it not depend on the container being a vector\nwould in theory make the function more generic, but there's very little\nchance (or risk :-)) we'll extend the usage. \n\n> > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > > > > ---\n> > > > >  include/libcamera/internal/v4l2_device.h |  4 ++--\n> > > > >  src/libcamera/v4l2_device.cpp            | 11 ++++-------\n> > > > >  2 files changed, 6 insertions(+), 9 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > > > > index d006bf68..087f07e7 100644\n> > > > > --- a/include/libcamera/internal/v4l2_device.h\n> > > > > +++ b/include/libcamera/internal/v4l2_device.h\n> > > > > @@ -14,6 +14,7 @@\n> > > > >  #include <linux/videodev2.h>\n> > > > >\n> > > > >  #include <libcamera/signal.h>\n> > > > > +#include <libcamera/span.h>\n> > > > >\n> > > > >  #include \"libcamera/internal/log.h\"\n> > > > >  #include \"libcamera/internal/v4l2_controls.h\"\n> > > > > @@ -55,8 +56,7 @@ protected:\n> > > > >  private:\n> > > > >     void listControls();\n> > > > >     void updateControls(ControlList *ctrls,\n> > > > > -                       const struct v4l2_ext_control *v4l2Ctrls,\n> > > > > -                       unsigned int count);\n> > > > > +                       Span<const v4l2_ext_control> v4l2Ctrls);\n> > > > >\n> > > > >     void eventAvailable(EventNotifier *notifier);\n> > > > >\n> > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > > > index 9f71bf0e..58516609 100644\n> > > > > --- a/src/libcamera/v4l2_device.cpp\n> > > > > +++ b/src/libcamera/v4l2_device.cpp\n> > > > > @@ -244,7 +244,7 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)\n> > > > >             v4l2Ctrls.resize(errorIdx);\n> > > > >     }\n> > > > >\n> > > > > -   updateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());\n> > > > > +   updateControls(&ctrls, v4l2Ctrls);\n> > > > >\n> > > > >     return ctrls;\n> > > > >  }\n> > > > > @@ -343,7 +343,7 @@ int V4L2Device::setControls(ControlList *ctrls)\n> > > > >             ret = errorIdx;\n> > > > >     }\n> > > > >\n> > > > > -   updateControls(ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());\n> > > > > +   updateControls(ctrls, v4l2Ctrls);\n> > > > >\n> > > > >     return ret;\n> > > > >  }\n> > > > > @@ -507,14 +507,11 @@ void V4L2Device::listControls()\n> > > > >   * values in \\a v4l2Ctrls\n> > > > >   * \\param[inout] ctrls List of V4L2 controls to update\n> > > > >   * \\param[in] v4l2Ctrls List of V4L2 extended controls as returned by the driver\n> > > > > - * \\param[in] count The number of controls to update\n> > > > >   */\n> > > > >  void V4L2Device::updateControls(ControlList *ctrls,\n> > > > > -                           const struct v4l2_ext_control *v4l2Ctrls,\n> > > > > -                           unsigned int count)\n> > > > > +                           Span<const v4l2_ext_control> v4l2Ctrls)\n> > > > >  {\n> > > > > -   for (unsigned int i = 0; i < count; i++) {\n> > > > > -           const struct v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i];\n> > > > > +   for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) {\n> > > > >             const unsigned int id = v4l2Ctrl.id;\n> > > > >             if (!ctrls->contains(id)) {\n> > > > >                     LOG(V4L2, Error) << \"ControlList doesn't contain id: \"","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8EDD4BDC92\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 25 Apr 2021 23:42:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0F1D56887D;\n\tMon, 26 Apr 2021 01:42:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4C392602D1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 26 Apr 2021 01:42:33 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C2B624FB;\n\tMon, 26 Apr 2021 01:42:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GwfCx2W3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619394152;\n\tbh=sSFkzatdE7NEM2X4TGCyQWrLymu9YJ6dwKOQQUG7ar8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GwfCx2W3NxcCMvUXMK+bFOswnvhtFVDFohANHSP8sQHKz5OKjATv0PvVG+nZXM7cR\n\tUwZ9mZqr1FkTdHG+FBdSlDHVqt/Cjty8UY5LLCT/KqLtqdFnqV/9Whden65SxkKu+H\n\tSl06aDlkiDCdiAF9lln6dL4LTKjWwmN/4D/EZLF4=","Date":"Mon, 26 Apr 2021 02:42:27 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<YIX+Y8QT/LsfY21F@pendragon.ideasonboard.com>","References":"<20210422021809.520675-1-hiroh@chromium.org>\n\t<20210422021809.520675-5-hiroh@chromium.org>\n\t<20210422075221.ih42pbyel6jimpjm@uno.localdomain>\n\t<YII499Id/IX9uJyV@pendragon.ideasonboard.com>\n\t<CAO5uPHPCJMpa4V6vKOLj6N9T2gGTpZSxP45eBGzyQTjBL4rZFA@mail.gmail.com>\n\t<20210423070533.br2jrgp42ftmf5di@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210423070533.br2jrgp42ftmf5di@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v4 4/4] libcamera: V4L2Device: Use\n\tSpan in updateControls()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16539,"web_url":"https://patchwork.libcamera.org/comment/16539/","msgid":"<YIX/ajYd8Url8qLE@pendragon.ideasonboard.com>","date":"2021-04-25T23:46:50","subject":"Re: [libcamera-devel] [PATCH v4 4/4] libcamera: V4L2Device: Use\n\tSpan in updateControls()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Fri, Apr 23, 2021 at 01:42:00PM +0900, Hirokazu Honda wrote:\n> On Fri, Apr 23, 2021 at 12:03 PM Laurent Pinchart wrote:\n> > On Thu, Apr 22, 2021 at 09:52:21AM +0200, Jacopo Mondi wrote:\n> > > On Thu, Apr 22, 2021 at 11:18:09AM +0900, Hirokazu Honda wrote:\n> > > > V4L2Device::updateControls() takes two arguments, raw array and\n> > > > its size, for the v4l2_ext_control values. This replaces it with\n> > > > libcamera::Span.\n> > >\n> > > Now that v4l2Ctrls is an std::vector<>, can't updateControls() take a\n> > > reference to it, instead of going through a Span<> ?\n> >\n> > That's what v3 did, and I proposed using span...\n> \n> Yep, mail-based review doesn't easily show us the review comments in\n> the previous version. :(\n> That's why I prefer some web app review site like GitHub or Gerrit,\n> which most of you don't like. :p\n\nYou're right that this is a useful feature of gerrit. There are more\ndrawbacks than advantages in gerrit compared to e-mails in my opinion\n(and that's only my opinion, with my use cases, my workflow, and my\nbias), but I have to recognize that there are useful features in gerrit.\n\nAs for github (and gitlab), that's not comparable, they're just unusable\nfor review :-)\n\n> Either Span or vector is fine to me. Please feel free to request me.\n\nOverall, I think Span is more generic, and I have a preference for using\nit, as a coding guideline. In this specific case, however, std::vector<>\nwould work too, it's an internal API, it doesn't matter much. If you or\nJacopo think that std::vector<> would be better, I'm fine with that.\n\n> > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > > > ---\n> > > >  include/libcamera/internal/v4l2_device.h |  4 ++--\n> > > >  src/libcamera/v4l2_device.cpp            | 11 ++++-------\n> > > >  2 files changed, 6 insertions(+), 9 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > > > index d006bf68..087f07e7 100644\n> > > > --- a/include/libcamera/internal/v4l2_device.h\n> > > > +++ b/include/libcamera/internal/v4l2_device.h\n> > > > @@ -14,6 +14,7 @@\n> > > >  #include <linux/videodev2.h>\n> > > >\n> > > >  #include <libcamera/signal.h>\n> > > > +#include <libcamera/span.h>\n> > > >\n> > > >  #include \"libcamera/internal/log.h\"\n> > > >  #include \"libcamera/internal/v4l2_controls.h\"\n> > > > @@ -55,8 +56,7 @@ protected:\n> > > >  private:\n> > > >     void listControls();\n> > > >     void updateControls(ControlList *ctrls,\n> > > > -                       const struct v4l2_ext_control *v4l2Ctrls,\n> > > > -                       unsigned int count);\n> > > > +                       Span<const v4l2_ext_control> v4l2Ctrls);\n> > > >\n> > > >     void eventAvailable(EventNotifier *notifier);\n> > > >\n> > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > > index 9f71bf0e..58516609 100644\n> > > > --- a/src/libcamera/v4l2_device.cpp\n> > > > +++ b/src/libcamera/v4l2_device.cpp\n> > > > @@ -244,7 +244,7 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)\n> > > >             v4l2Ctrls.resize(errorIdx);\n> > > >     }\n> > > >\n> > > > -   updateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());\n> > > > +   updateControls(&ctrls, v4l2Ctrls);\n> > > >\n> > > >     return ctrls;\n> > > >  }\n> > > > @@ -343,7 +343,7 @@ int V4L2Device::setControls(ControlList *ctrls)\n> > > >             ret = errorIdx;\n> > > >     }\n> > > >\n> > > > -   updateControls(ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());\n> > > > +   updateControls(ctrls, v4l2Ctrls);\n> > > >\n> > > >     return ret;\n> > > >  }\n> > > > @@ -507,14 +507,11 @@ void V4L2Device::listControls()\n> > > >   * values in \\a v4l2Ctrls\n> > > >   * \\param[inout] ctrls List of V4L2 controls to update\n> > > >   * \\param[in] v4l2Ctrls List of V4L2 extended controls as returned by the driver\n> > > > - * \\param[in] count The number of controls to update\n> > > >   */\n> > > >  void V4L2Device::updateControls(ControlList *ctrls,\n> > > > -                           const struct v4l2_ext_control *v4l2Ctrls,\n> > > > -                           unsigned int count)\n> > > > +                           Span<const v4l2_ext_control> v4l2Ctrls)\n> > > >  {\n> > > > -   for (unsigned int i = 0; i < count; i++) {\n> > > > -           const struct v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i];\n> > > > +   for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) {\n> > > >             const unsigned int id = v4l2Ctrl.id;\n> > > >             if (!ctrls->contains(id)) {\n> > > >                     LOG(V4L2, Error) << \"ControlList doesn't contain id: \"","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6160EBDC91\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 25 Apr 2021 23:46:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BF9AE6887D;\n\tMon, 26 Apr 2021 01:46:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7C864602D1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 26 Apr 2021 01:46:56 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D6D984FB;\n\tMon, 26 Apr 2021 01:46:55 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jcgjFddP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619394416;\n\tbh=E18XSm/wMYZ7PAPrDDXYu0PXXh6UzFlUYpUeEHw2gXk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jcgjFddPgv3xaciZXBunMRlN8PbDAcvQzoKizsyjYud+b6eXsFaLoXX/hyXffm1rv\n\tqq3LoiaJn7B2jTE6i72JKWOaTBF5K5iGLwlW7OWcMiuaE0G5o3g2cxObnXg8uG08JU\n\tG+jhKF9JgwnFl2Lto+JiLJ/QBpRctJa9i2NOVMCc=","Date":"Mon, 26 Apr 2021 02:46:50 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Hirokazu Honda <hiroh@chromium.org>","Message-ID":"<YIX/ajYd8Url8qLE@pendragon.ideasonboard.com>","References":"<20210422021809.520675-1-hiroh@chromium.org>\n\t<20210422021809.520675-5-hiroh@chromium.org>\n\t<20210422075221.ih42pbyel6jimpjm@uno.localdomain>\n\t<YII499Id/IX9uJyV@pendragon.ideasonboard.com>\n\t<CAO5uPHPCJMpa4V6vKOLj6N9T2gGTpZSxP45eBGzyQTjBL4rZFA@mail.gmail.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<CAO5uPHPCJMpa4V6vKOLj6N9T2gGTpZSxP45eBGzyQTjBL4rZFA@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v4 4/4] libcamera: V4L2Device: Use\n\tSpan in updateControls()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]