[{"id":16502,"web_url":"https://patchwork.libcamera.org/comment/16502/","msgid":"<20210422075046.jgej24vdbgmkan5p@uno.localdomain>","date":"2021-04-22T07:50:46","subject":"Re: [libcamera-devel] [PATCH v4 2/4] libcamera: V4L2Device: Use\n\tstd::vector for v4l2_ext_control in getControls()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nInteresting https://stackoverflow.com/a/21519063\n\nI initially thougth replacing VLA in C++ code was mostly a matter of\nstyle...\n\nOn Thu, Apr 22, 2021 at 11:18:07AM +0900, Hirokazu Honda wrote:\n> The original code uses Variable-Length-Array, which is not\n> officially supported in C++. This replaces the array with\n> std::vector.\n\nOne thing that always bothered me of this class is\n\n\tstd::map controlInfo_;\n\tstd::vector> controlIds_;\n\neverytime I see these identifiers I forget they refer to v4l2 stuff.\nShould they be renamed ?\n\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/libcamera/v4l2_device.cpp | 31 ++++++++++++++-----------------\n> 1 file changed, 14 insertions(+), 17 deletions(-)\n>\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 3c32d682..617e6ec9 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -173,8 +173,7 @@ void V4L2Device::close()\n> */\n> ControlList V4L2Device::getControls(const std::vector &ids)\n> {\n> -\tunsigned int count = ids.size();\n> -\tif (count == 0)\n> +\tif (ids.empty())\n> \t\treturn {};\n>\n> \tControlList ctrls{ controls_ };\n> @@ -190,22 +189,20 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> \t\tctrls.set(id, {});\n> \t}\n>\n> -\tstruct v4l2_ext_control v4l2Ctrls[count];\n> -\tmemset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));\n> +\tstd::vector v4l2Ctrls(ctrls.size());\n> +\tmemset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size());\n\nDo you need the memset ?\n\nI read:\n\n explicit vector( size_type count );\n\n 4) Constructs the container with count default-inserted instances of T. No copies are made.\n\n\"default inserted instances of T\" makes me wonder if a\ndefault-inserted v4l2_ext_control is actually zeroed\n\nI wonder if using reserve() would be better\n\n>\n> -\tunsigned int i = 0;\n> -\tfor (auto &ctrl : ctrls) {\n> -\t\tunsigned int id = ctrl.first;\n> +\tfor (auto [ctrl, i] = std::pair(ctrls.begin(), 0u); i < ctrls.size(); ctrl++, i++) {\n\nC++ is the language of the beast.\nNeat though\n\n> +\t\tconst unsigned int id = ctrl->first;\n> \t\tconst struct v4l2_query_ext_ctrl &info = controlInfo_[id];\n> +\t\tv4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i];\n>\n> \t\tif (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {\n> \t\t\tControlType type;\n> -\n> \t\t\tswitch (info.type) {\n> \t\t\tcase V4L2_CTRL_TYPE_U8:\n> \t\t\t\ttype = ControlTypeByte;\n> \t\t\t\tbreak;\n> -\n> \t\t\tdefault:\n> \t\t\t\tLOG(V4L2, Error)\n> \t\t\t\t\t<< \"Unsupported payload control type \"\n> @@ -213,7 +210,7 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> \t\t\t\treturn {};\n> \t\t\t}\n>\n> -\t\t\tControlValue &value = ctrl.second;\n> +\t\t\tControlValue &value = ctrl->second;\n> \t\t\tvalue.reserve(type, true, info.elems);\n> \t\t\tSpan data = value.data();\n>\n> @@ -221,21 +218,20 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> \t\t\tv4l2Ctrls[i].size = data.size();\n> \t\t}\n>\n> -\t\tv4l2Ctrls[i].id = id;\n> -\t\ti++;\n> +\t\tv4l2Ctrl.id = id;\n\nHowever, shouldn't we try to squeeze the two loops (the one that\ninitialize ctrls and this one) in a single one now that the ordering\nrestrictions is gone ?\n\n> \t}\n>\n> \tstruct v4l2_ext_controls v4l2ExtCtrls = {};\n> \tv4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> -\tv4l2ExtCtrls.controls = v4l2Ctrls;\n> -\tv4l2ExtCtrls.count = count;\n> +\tv4l2ExtCtrls.controls = v4l2Ctrls.data();\n> +\tv4l2ExtCtrls.count = v4l2Ctrls.size();\n>\n> \tint ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls);\n> \tif (ret) {\n> \t\tunsigned int errorIdx = v4l2ExtCtrls.error_idx;\n>\n> \t\t/* Generic validation error. */\n> -\t\tif (errorIdx == 0 || errorIdx >= count) {\n> +\t\tif (errorIdx == 0 || errorIdx >= v4l2Ctrls.size()) {\n> \t\t\tLOG(V4L2, Error) << \"Unable to read controls: \"\n> \t\t\t\t\t << strerror(-ret);\n> \t\t\treturn {};\n> @@ -244,10 +240,11 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> \t\t/* A specific control failed. */\n> \t\tLOG(V4L2, Error) << \"Unable to read control \" << errorIdx\n> \t\t\t\t << \": \" << strerror(-ret);\n> -\t\tcount = errorIdx - 1;\n> +\n> +\t\tv4l2Ctrls.resize(errorIdx);\n> \t}\n>\n> -\tupdateControls(&ctrls, v4l2Ctrls, count);\n> +\tupdateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());\n>\n> \treturn ctrls;\n> }\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":"","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 C931BBDB15\n\tfor ;\n\tThu, 22 Apr 2021 07:50:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 16F9B68852;\n\tThu, 22 Apr 2021 09:50:08 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 592D568847\n\tfor ;\n\tThu, 22 Apr 2021 09:50:06 +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 relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 7E726FF804;\n\tThu, 22 Apr 2021 07:50:05 +0000 (UTC)"],"X-Originating-IP":"82.57.193.192","Date":"Thu, 22 Apr 2021 09:50:46 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210422075046.jgej24vdbgmkan5p@uno.localdomain>","References":"<20210422021809.520675-1-hiroh@chromium.org>\n\t<20210422021809.520675-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210422021809.520675-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v4 2/4] libcamera: V4L2Device: Use\n\tstd::vector for v4l2_ext_control in getControls()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","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\" "}},{"id":16522,"web_url":"https://patchwork.libcamera.org/comment/16522/","msgid":"","date":"2021-04-23T03:05:50","subject":"Re: [libcamera-devel] [PATCH v4 2/4] libcamera: V4L2Device: Use\n\tstd::vector for v4l2_ext_control in getControls()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Apr 22, 2021 at 09:50:46AM +0200, Jacopo Mondi wrote:\n> Hi Hiro,\n> \n> Interesting https://stackoverflow.com/a/21519063\n\n\"An aesthetic or correct way of invoking an action that requires DOM\nmanipulation as well as controller related tasks in EmberJS\"\n\nPlease don't tell me you plan to rewrite libcamera in JS :-)\n\n> I initially thougth replacing VLA in C++ code was mostly a matter of\n> style...\n> \n> On Thu, Apr 22, 2021 at 11:18:07AM +0900, Hirokazu Honda wrote:\n> > The original code uses Variable-Length-Array, which is not\n> > officially supported in C++. This replaces the array with\n> > std::vector.\n> \n> One thing that always bothered me of this class is\n> \n> \tstd::map controlInfo_;\n> \tstd::vector> controlIds_;\n> \n> everytime I see these identifiers I forget they refer to v4l2 stuff.\n> Should they be renamed ?\n> \n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/libcamera/v4l2_device.cpp | 31 ++++++++++++++-----------------\n> > 1 file changed, 14 insertions(+), 17 deletions(-)\n> >\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 3c32d682..617e6ec9 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -173,8 +173,7 @@ void V4L2Device::close()\n> > */\n> > ControlList V4L2Device::getControls(const std::vector &ids)\n> > {\n> > -\tunsigned int count = ids.size();\n> > -\tif (count == 0)\n> > +\tif (ids.empty())\n> > \t\treturn {};\n> >\n> > \tControlList ctrls{ controls_ };\n> > @@ -190,22 +189,20 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > \t\tctrls.set(id, {});\n> > \t}\n> >\n> > -\tstruct v4l2_ext_control v4l2Ctrls[count];\n> > -\tmemset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));\n> > +\tstd::vector v4l2Ctrls(ctrls.size());\n> > +\tmemset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size());\n> \n> Do you need the memset ?\n> \n> I read:\n> \n> explicit vector( size_type count );\n> \n> 4) Constructs the container with count default-inserted instances of T. No copies are made.\n> \n> \"default inserted instances of T\" makes me wonder if a\n> default-inserted v4l2_ext_control is actually zeroed\n> \n> I wonder if using reserve() would be better\n\nIn a previous reply, Hiro mentioned that the zero-initialization will\nonly initialize the first member of unions. memset() is possibly safer.\n\n> >\n> > -\tunsigned int i = 0;\n> > -\tfor (auto &ctrl : ctrls) {\n> > -\t\tunsigned int id = ctrl.first;\n> > +\tfor (auto [ctrl, i] = std::pair(ctrls.begin(), 0u); i < ctrls.size(); ctrl++, i++) {\n> \n> C++ is the language of the beast.\n> Neat though\n\nI've posted \"[PATCH/RFC 1/3] libcamera: utils: Add enumerate view for\nrange-based for loops\" which would allow writing this as\n\n\tfor (auto &[i, ctrl] = utils::enumerate(ctrls)) {\n\nIt's not a blocker though, we can merge this series and rework it later.\nI'd however appreciate feedback on the above patch (and the rest of the\nseries).\n\n> > +\t\tconst unsigned int id = ctrl->first;\n> > \t\tconst struct v4l2_query_ext_ctrl &info = controlInfo_[id];\n> > +\t\tv4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i];\n> >\n> > \t\tif (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {\n> > \t\t\tControlType type;\n> > -\n> > \t\t\tswitch (info.type) {\n> > \t\t\tcase V4L2_CTRL_TYPE_U8:\n> > \t\t\t\ttype = ControlTypeByte;\n> > \t\t\t\tbreak;\n> > -\n\nI really think those blank lines help readability.\n\n> > \t\t\tdefault:\n> > \t\t\t\tLOG(V4L2, Error)\n> > \t\t\t\t\t<< \"Unsupported payload control type \"\n> > @@ -213,7 +210,7 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > \t\t\t\treturn {};\n> > \t\t\t}\n> >\n> > -\t\t\tControlValue &value = ctrl.second;\n> > +\t\t\tControlValue &value = ctrl->second;\n> > \t\t\tvalue.reserve(type, true, info.elems);\n> > \t\t\tSpan data = value.data();\n> >\n> > @@ -221,21 +218,20 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > \t\t\tv4l2Ctrls[i].size = data.size();\n> > \t\t}\n> >\n> > -\t\tv4l2Ctrls[i].id = id;\n> > -\t\ti++;\n> > +\t\tv4l2Ctrl.id = id;\n> \n> However, shouldn't we try to squeeze the two loops (the one that\n> initialize ctrls and this one) in a single one now that the ordering\n> restrictions is gone ?\n\nThat's what I replied to 1/4 :-) I'd have a small preference for doing\nit in 1/4 as that's where the restriction is removed, or at least in a\npatch inserted before 1/4 and 2/4.\n\n> > \t}\n> >\n> > \tstruct v4l2_ext_controls v4l2ExtCtrls = {};\n> > \tv4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> > -\tv4l2ExtCtrls.controls = v4l2Ctrls;\n> > -\tv4l2ExtCtrls.count = count;\n> > +\tv4l2ExtCtrls.controls = v4l2Ctrls.data();\n> > +\tv4l2ExtCtrls.count = v4l2Ctrls.size();\n> >\n> > \tint ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls);\n> > \tif (ret) {\n> > \t\tunsigned int errorIdx = v4l2ExtCtrls.error_idx;\n> >\n> > \t\t/* Generic validation error. */\n> > -\t\tif (errorIdx == 0 || errorIdx >= count) {\n> > +\t\tif (errorIdx == 0 || errorIdx >= v4l2Ctrls.size()) {\n> > \t\t\tLOG(V4L2, Error) << \"Unable to read controls: \"\n> > \t\t\t\t\t << strerror(-ret);\n> > \t\t\treturn {};\n> > @@ -244,10 +240,11 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > \t\t/* A specific control failed. */\n> > \t\tLOG(V4L2, Error) << \"Unable to read control \" << errorIdx\n> > \t\t\t\t << \": \" << strerror(-ret);\n> > -\t\tcount = errorIdx - 1;\n> > +\n> > +\t\tv4l2Ctrls.resize(errorIdx);\n> > \t}\n> >\n> > -\tupdateControls(&ctrls, v4l2Ctrls, count);\n> > +\tupdateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());\n> >\n> > \treturn ctrls;\n> > }","headers":{"Return-Path":"","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 1310DBDB15\n\tfor ;\n\tFri, 23 Apr 2021 03:05:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 660726886D;\n\tFri, 23 Apr 2021 05:05: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 E88B360513\n\tfor ;\n\tFri, 23 Apr 2021 05:05:55 +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 692F7EE;\n\tFri, 23 Apr 2021 05:05: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=\"LfzysQC7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619147155;\n\tbh=3+DCV5b3UqklY9E+pXsK9S6H0TD//noTJTE1fJpf2sA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LfzysQC7PQQCLTPxyL61UChe0nfo/J3Sz/fIm0ejm35mXbRrbJh/4EN8aL+F5eUB/\n\teVNnK2UKvHtA6olM0kjanCwrTLyLxEDrHksqQopVvhgvbz63Q/qA+aJXVDcQOGDXoJ\n\tsA6uiwMUKRb61ZuFkwycsSQekA7ww6G0Or9OLnu0=","Date":"Fri, 23 Apr 2021 06:05:50 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210422021809.520675-1-hiroh@chromium.org>\n\t<20210422021809.520675-3-hiroh@chromium.org>\n\t<20210422075046.jgej24vdbgmkan5p@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210422075046.jgej24vdbgmkan5p@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v4 2/4] libcamera: V4L2Device: Use\n\tstd::vector for v4l2_ext_control in getControls()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","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\" "}},{"id":16525,"web_url":"https://patchwork.libcamera.org/comment/16525/","msgid":"","date":"2021-04-23T04:38:14","subject":"Re: [libcamera-devel] [PATCH v4 2/4] libcamera: V4L2Device: Use\n\tstd::vector for v4l2_ext_control in getControls()","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:05 PM Laurent Pinchart\n wrote:\n>\n> On Thu, Apr 22, 2021 at 09:50:46AM +0200, Jacopo Mondi wrote:\n> > Hi Hiro,\n> >\n> > Interesting https://stackoverflow.com/a/21519063\n>\n> \"An aesthetic or correct way of invoking an action that requires DOM\n> manipulation as well as controller related tasks in EmberJS\"\n>\n> Please don't tell me you plan to rewrite libcamera in JS :-)\n>\n> > I initially thougth replacing VLA in C++ code was mostly a matter of\n> > style...\n> >\n> > On Thu, Apr 22, 2021 at 11:18:07AM +0900, Hirokazu Honda wrote:\n> > > The original code uses Variable-Length-Array, which is not\n> > > officially supported in C++. This replaces the array with\n> > > std::vector.\n> >\n> > One thing that always bothered me of this class is\n> >\n> > std::map controlInfo_;\n> > std::vector> controlIds_;\n> >\n> > everytime I see these identifiers I forget they refer to v4l2 stuff.\n> > Should they be renamed ?\n> >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > src/libcamera/v4l2_device.cpp | 31 ++++++++++++++-----------------\n> > > 1 file changed, 14 insertions(+), 17 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > index 3c32d682..617e6ec9 100644\n> > > --- a/src/libcamera/v4l2_device.cpp\n> > > +++ b/src/libcamera/v4l2_device.cpp\n> > > @@ -173,8 +173,7 @@ void V4L2Device::close()\n> > > */\n> > > ControlList V4L2Device::getControls(const std::vector &ids)\n> > > {\n> > > - unsigned int count = ids.size();\n> > > - if (count == 0)\n> > > + if (ids.empty())\n> > > return {};\n> > >\n> > > ControlList ctrls{ controls_ };\n> > > @@ -190,22 +189,20 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > > ctrls.set(id, {});\n> > > }\n> > >\n> > > - struct v4l2_ext_control v4l2Ctrls[count];\n> > > - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));\n> > > + std::vector v4l2Ctrls(ctrls.size());\n> > > + memset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size());\n> >\n> > Do you need the memset ?\n> >\n> > I read:\n> >\n> > explicit vector( size_type count );\n> >\n> > 4) Constructs the container with count default-inserted instances of T. No copies are made.\n> >\n> > \"default inserted instances of T\" makes me wonder if a\n> > default-inserted v4l2_ext_control is actually zeroed\n> >\n> > I wonder if using reserve() would be better\n>\n> In a previous reply, Hiro mentioned that the zero-initialization will\n> only initialize the first member of unions. memset() is possibly safer.\n>\n> > >\n> > > - unsigned int i = 0;\n> > > - for (auto &ctrl : ctrls) {\n> > > - unsigned int id = ctrl.first;\n> > > + for (auto [ctrl, i] = std::pair(ctrls.begin(), 0u); i < ctrls.size(); ctrl++, i++) {\n> >\n> > C++ is the language of the beast.\n> > Neat though\n>\n> I've posted \"[PATCH/RFC 1/3] libcamera: utils: Add enumerate view for\n> range-based for loops\" which would allow writing this as\n>\n> for (auto &[i, ctrl] = utils::enumerate(ctrls)) {\n>\n> It's not a blocker though, we can merge this series and rework it later.\n> I'd however appreciate feedback on the above patch (and the rest of the\n> series).\n\nThanks for informing me.\nI would love zip and enumerator iterators. :)\nI will review them later.\n\n>\n> > > + const unsigned int id = ctrl->first;\n> > > const struct v4l2_query_ext_ctrl &info = controlInfo_[id];\n> > > + v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i];\n> > >\n> > > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {\n> > > ControlType type;\n> > > -\n> > > switch (info.type) {\n> > > case V4L2_CTRL_TYPE_U8:\n> > > type = ControlTypeByte;\n> > > break;\n> > > -\n>\n> I really think those blank lines help readability.\n>\n> > > default:\n> > > LOG(V4L2, Error)\n> > > << \"Unsupported payload control type \"\n> > > @@ -213,7 +210,7 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > > return {};\n> > > }\n> > >\n> > > - ControlValue &value = ctrl.second;\n> > > + ControlValue &value = ctrl->second;\n> > > value.reserve(type, true, info.elems);\n> > > Span data = value.data();\n> > >\n> > > @@ -221,21 +218,20 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > > v4l2Ctrls[i].size = data.size();\n> > > }\n> > >\n> > > - v4l2Ctrls[i].id = id;\n> > > - i++;\n> > > + v4l2Ctrl.id = id;\n> >\n> > However, shouldn't we try to squeeze the two loops (the one that\n> > initialize ctrls and this one) in a single one now that the ordering\n> > restrictions is gone ?\n>\n> That's what I replied to 1/4 :-) I'd have a small preference for doing\n> it in 1/4 as that's where the restriction is removed, or at least in a\n> patch inserted before 1/4 and 2/4.\n>\n> > > }\n> > >\n> > > struct v4l2_ext_controls v4l2ExtCtrls = {};\n> > > v4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> > > - v4l2ExtCtrls.controls = v4l2Ctrls;\n> > > - v4l2ExtCtrls.count = count;\n> > > + v4l2ExtCtrls.controls = v4l2Ctrls.data();\n> > > + v4l2ExtCtrls.count = v4l2Ctrls.size();\n> > >\n> > > int ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls);\n> > > if (ret) {\n> > > unsigned int errorIdx = v4l2ExtCtrls.error_idx;\n> > >\n> > > /* Generic validation error. */\n> > > - if (errorIdx == 0 || errorIdx >= count) {\n> > > + if (errorIdx == 0 || errorIdx >= v4l2Ctrls.size()) {\n> > > LOG(V4L2, Error) << \"Unable to read controls: \"\n> > > << strerror(-ret);\n> > > return {};\n> > > @@ -244,10 +240,11 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > > /* A specific control failed. */\n> > > LOG(V4L2, Error) << \"Unable to read control \" << errorIdx\n> > > << \": \" << strerror(-ret);\n> > > - count = errorIdx - 1;\n> > > +\n> > > + v4l2Ctrls.resize(errorIdx);\n> > > }\n> > >\n> > > - updateControls(&ctrls, v4l2Ctrls, count);\n> > > + updateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());\n> > >\n> > > return ctrls;\n> > > }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","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 5C08BBDB1A\n\tfor ;\n\tFri, 23 Apr 2021 04:38:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6A8C768872;\n\tFri, 23 Apr 2021 06:38:27 +0200 (CEST)","from mail-ej1-x633.google.com (mail-ej1-x633.google.com\n\t[IPv6:2a00:1450:4864:20::633])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 26BA168861\n\tfor ;\n\tFri, 23 Apr 2021 06:38:26 +0200 (CEST)","by mail-ej1-x633.google.com with SMTP id g5so65252197ejx.0\n\tfor ;\n\tThu, 22 Apr 2021 21:38:26 -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=\"XNxpugqN\"; 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=V5a+vke2eZSNq4HpOGu3GvWjju0QFv0JBxPLT/L84Nk=;\n\tb=XNxpugqNsMQYmcR8pJsF5t1XJrVtXxGisaKY0nZ9odGs3Ei7RTjYj4kcTkCv1MvkuR\n\tOima1jzJ6/S/FhyVbpq/EaKIlm2ADakkGRqICRZDr0JeMH670+vSiwSrSM81IubzmeUR\n\tapDgtA29Ft/C/trFO1uM5idPTmddzVUMnuhpI=","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=V5a+vke2eZSNq4HpOGu3GvWjju0QFv0JBxPLT/L84Nk=;\n\tb=fmtX2m+M0UI2nQyfheKnta2anCLMWJU0oEXCYOPJjBKpXWDxY6qhiGW4w6/d7SDcwN\n\tMeuy37rImP4IVvv0X272S4djviWCZYR/SI6cKKbSZjePjtSjnb6L1k7Af9xejiT9DI3x\n\tdTE43sPlQ9RD2xZjdajDi6xOfpJx6s8QxEWOApYPMVD9jCwT4qI0hHL++dGrKfbu3KLF\n\tCyqRQUYTo97/+3yuZirCGMC4IlbdrYDXfzXo79bMasExru/QTVmAPt1x14QHb8ivcLr4\n\tzQWgp/sRU1HANrGNNoYeXfWqSy9Ep0yxNOZj6n3Nrof96Ht9WiJTCpUbqruF62L16n8b\n\t02Gg==","X-Gm-Message-State":"AOAM5311CXYh0nIrjMJasCbySelF1DiYf+JDV5nqx4Yroi1mLR2/snBQ\n\t5DkCHZ1QEjr8Z6j/7M4OmO6d9ltU/sGAs21iFDE1og==","X-Google-Smtp-Source":"ABdhPJzYctVgL52FCSkcj2qYkKKPVJt9m0di9Tvat6loWCZr8hQTOP0Pspy+6jrbA6y/BuV6IJSuL8N9zLD/mWiTLGA=","X-Received":"by 2002:a17:906:fcc4:: with SMTP id\n\tqx4mr2075969ejb.42.1619152705633; \n\tThu, 22 Apr 2021 21:38:25 -0700 (PDT)","MIME-Version":"1.0","References":"<20210422021809.520675-1-hiroh@chromium.org>\n\t<20210422021809.520675-3-hiroh@chromium.org>\n\t<20210422075046.jgej24vdbgmkan5p@uno.localdomain>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 23 Apr 2021 13:38:14 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v4 2/4] libcamera: V4L2Device: Use\n\tstd::vector for v4l2_ext_control in getControls()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16527,"web_url":"https://patchwork.libcamera.org/comment/16527/","msgid":"<20210423070317.hs3qzvffblp6tcf4@uno.localdomain>","date":"2021-04-23T07:03:17","subject":"Re: [libcamera-devel] [PATCH v4 2/4] libcamera: V4L2Device: Use\n\tstd::vector for v4l2_ext_control in getControls()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Fri, Apr 23, 2021 at 06:05:50AM +0300, Laurent Pinchart wrote:\n> On Thu, Apr 22, 2021 at 09:50:46AM +0200, Jacopo Mondi wrote:\n> > Hi Hiro,\n> >\n> > Interesting https://stackoverflow.com/a/21519063\n>\n> \"An aesthetic or correct way of invoking an action that requires DOM\n> manipulation as well as controller related tasks in EmberJS\"\n\nWhat the heck! I don't even know what EmberJS is! I clicked on the\n'share' button of stackoverflow post about VLA in C++ and that's the\nlink I get. Didn't know \"share\" actually meant \"share a random\nstackoverflow thread\" :)\n\n>\n> Please don't tell me you plan to rewrite libcamera in JS :-)\n>\n> > I initially thougth replacing VLA in C++ code was mostly a matter of\n> > style...\n> >\n> > On Thu, Apr 22, 2021 at 11:18:07AM +0900, Hirokazu Honda wrote:\n> > > The original code uses Variable-Length-Array, which is not\n> > > officially supported in C++. This replaces the array with\n> > > std::vector.\n> >\n> > One thing that always bothered me of this class is\n> >\n> > \tstd::map controlInfo_;\n> > \tstd::vector> controlIds_;\n> >\n> > everytime I see these identifiers I forget they refer to v4l2 stuff.\n> > Should they be renamed ?\n> >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > src/libcamera/v4l2_device.cpp | 31 ++++++++++++++-----------------\n> > > 1 file changed, 14 insertions(+), 17 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > index 3c32d682..617e6ec9 100644\n> > > --- a/src/libcamera/v4l2_device.cpp\n> > > +++ b/src/libcamera/v4l2_device.cpp\n> > > @@ -173,8 +173,7 @@ void V4L2Device::close()\n> > > */\n> > > ControlList V4L2Device::getControls(const std::vector &ids)\n> > > {\n> > > -\tunsigned int count = ids.size();\n> > > -\tif (count == 0)\n> > > +\tif (ids.empty())\n> > > \t\treturn {};\n> > >\n> > > \tControlList ctrls{ controls_ };\n> > > @@ -190,22 +189,20 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > > \t\tctrls.set(id, {});\n> > > \t}\n> > >\n> > > -\tstruct v4l2_ext_control v4l2Ctrls[count];\n> > > -\tmemset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));\n> > > +\tstd::vector v4l2Ctrls(ctrls.size());\n> > > +\tmemset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size());\n> >\n> > Do you need the memset ?\n> >\n> > I read:\n> >\n> > explicit vector( size_type count );\n> >\n> > 4) Constructs the container with count default-inserted instances of T. No copies are made.\n> >\n> > \"default inserted instances of T\" makes me wonder if a\n> > default-inserted v4l2_ext_control is actually zeroed\n> >\n> > I wonder if using reserve() would be better\n>\n> In a previous reply, Hiro mentioned that the zero-initialization will\n> only initialize the first member of unions. memset() is possibly safer.\n>\n> > >\n> > > -\tunsigned int i = 0;\n> > > -\tfor (auto &ctrl : ctrls) {\n> > > -\t\tunsigned int id = ctrl.first;\n> > > +\tfor (auto [ctrl, i] = std::pair(ctrls.begin(), 0u); i < ctrls.size(); ctrl++, i++) {\n> >\n> > C++ is the language of the beast.\n> > Neat though\n>\n> I've posted \"[PATCH/RFC 1/3] libcamera: utils: Add enumerate view for\n> range-based for loops\" which would allow writing this as\n>\n> \tfor (auto &[i, ctrl] = utils::enumerate(ctrls)) {\n>\n> It's not a blocker though, we can merge this series and rework it later.\n> I'd however appreciate feedback on the above patch (and the rest of the\n> series).\n>\n> > > +\t\tconst unsigned int id = ctrl->first;\n> > > \t\tconst struct v4l2_query_ext_ctrl &info = controlInfo_[id];\n> > > +\t\tv4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i];\n> > >\n> > > \t\tif (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {\n> > > \t\t\tControlType type;\n> > > -\n> > > \t\t\tswitch (info.type) {\n> > > \t\t\tcase V4L2_CTRL_TYPE_U8:\n> > > \t\t\t\ttype = ControlTypeByte;\n> > > \t\t\t\tbreak;\n> > > -\n>\n> I really think those blank lines help readability.\n>\n> > > \t\t\tdefault:\n> > > \t\t\t\tLOG(V4L2, Error)\n> > > \t\t\t\t\t<< \"Unsupported payload control type \"\n> > > @@ -213,7 +210,7 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > > \t\t\t\treturn {};\n> > > \t\t\t}\n> > >\n> > > -\t\t\tControlValue &value = ctrl.second;\n> > > +\t\t\tControlValue &value = ctrl->second;\n> > > \t\t\tvalue.reserve(type, true, info.elems);\n> > > \t\t\tSpan data = value.data();\n> > >\n> > > @@ -221,21 +218,20 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > > \t\t\tv4l2Ctrls[i].size = data.size();\n> > > \t\t}\n> > >\n> > > -\t\tv4l2Ctrls[i].id = id;\n> > > -\t\ti++;\n> > > +\t\tv4l2Ctrl.id = id;\n> >\n> > However, shouldn't we try to squeeze the two loops (the one that\n> > initialize ctrls and this one) in a single one now that the ordering\n> > restrictions is gone ?\n>\n> That's what I replied to 1/4 :-) I'd have a small preference for doing\n> it in 1/4 as that's where the restriction is removed, or at least in a\n> patch inserted before 1/4 and 2/4.\n>\n\nWherever works !\n\nThanks\n j\n\n> > > \t}\n> > >\n> > > \tstruct v4l2_ext_controls v4l2ExtCtrls = {};\n> > > \tv4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> > > -\tv4l2ExtCtrls.controls = v4l2Ctrls;\n> > > -\tv4l2ExtCtrls.count = count;\n> > > +\tv4l2ExtCtrls.controls = v4l2Ctrls.data();\n> > > +\tv4l2ExtCtrls.count = v4l2Ctrls.size();\n> > >\n> > > \tint ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls);\n> > > \tif (ret) {\n> > > \t\tunsigned int errorIdx = v4l2ExtCtrls.error_idx;\n> > >\n> > > \t\t/* Generic validation error. */\n> > > -\t\tif (errorIdx == 0 || errorIdx >= count) {\n> > > +\t\tif (errorIdx == 0 || errorIdx >= v4l2Ctrls.size()) {\n> > > \t\t\tLOG(V4L2, Error) << \"Unable to read controls: \"\n> > > \t\t\t\t\t << strerror(-ret);\n> > > \t\t\treturn {};\n> > > @@ -244,10 +240,11 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > > \t\t/* A specific control failed. */\n> > > \t\tLOG(V4L2, Error) << \"Unable to read control \" << errorIdx\n> > > \t\t\t\t << \": \" << strerror(-ret);\n> > > -\t\tcount = errorIdx - 1;\n> > > +\n> > > +\t\tv4l2Ctrls.resize(errorIdx);\n> > > \t}\n> > >\n> > > -\tupdateControls(&ctrls, v4l2Ctrls, count);\n> > > +\tupdateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());\n> > >\n> > > \treturn ctrls;\n> > > }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","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 80A13BDB15\n\tfor ;\n\tFri, 23 Apr 2021 07:02:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EC14068879;\n\tFri, 23 Apr 2021 09:02:39 +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 18CCC6886E\n\tfor ;\n\tFri, 23 Apr 2021 09:02:39 +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 09E20C000D;\n\tFri, 23 Apr 2021 07:02:37 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Fri, 23 Apr 2021 09:03:17 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210423070317.hs3qzvffblp6tcf4@uno.localdomain>","References":"<20210422021809.520675-1-hiroh@chromium.org>\n\t<20210422021809.520675-3-hiroh@chromium.org>\n\t<20210422075046.jgej24vdbgmkan5p@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 2/4] libcamera: V4L2Device: Use\n\tstd::vector for v4l2_ext_control in getControls()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","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\" "}}]