[{"id":16540,"web_url":"https://patchwork.libcamera.org/comment/16540/","msgid":"","date":"2021-04-26T00:12:41","subject":"Re: [libcamera-devel] [PATCH v5 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":"Hi Hiro,\n\nThank you for the patch.\n\nOn Fri, Apr 23, 2021 at 06:36:51PM +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> Signed-off-by: Hirokazu Honda \n> ---\n> src/libcamera/v4l2_device.cpp | 41 +++++++++++++++++------------------\n> 1 file changed, 20 insertions(+), 21 deletions(-)\n> \n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index ce2860c4..bbe8f154 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -173,13 +173,18 @@ 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> +\tstd::vector v4l2Ctrls(ids.size());\n> +\tmemset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size());\n> +\n> +\tfor (size_t i = 0, j = 0; j < ids.size(); ++j) {\n> +\t\tconst unsigned int id = ids[j];\n> +\t\tif (ctrls.contains(id))\n> +\t\t\tcontinue;\n> \n> -\tfor (uint32_t id : ids) {\n> \t\tconst auto iter = controls_.find(id);\n> \t\tif (iter == controls_.end()) {\n> \t\t\tLOG(V4L2, Error)\n> @@ -187,20 +192,12 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> \t\t\treturn {};\n> \t\t}\n> \n> -\t\tctrls.set(id, {});\n> -\t}\n> -\n> -\tstruct v4l2_ext_control v4l2Ctrls[count];\n> -\tmemset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));\n> -\n> -\tunsigned int i = 0;\n> -\tfor (auto &ctrl : ctrls) {\n> -\t\tunsigned int id = ctrl.first;\n> +\t\tv4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i++];\n\nYou increase i here, ...\n\n> \t\tconst struct v4l2_query_ext_ctrl &info = controlInfo_[id];\n> +\t\tControlValue value{};\n\nControlValue has a default constructor, no need for {}.\n\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> @@ -213,7 +210,6 @@ 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\tvalue.reserve(type, true, info.elems);\n> \t\t\tSpan data = value.data();\n> \n> @@ -221,21 +217,23 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> \t\t\tv4l2Ctrls[i].size = data.size();\n\n... and use it here. I don't think that's correct.\n\n> \t\t}\n> \n> -\t\tv4l2Ctrls[i].id = id;\n> -\t\ti++;\n> +\t\tv4l2Ctrl.id = id;\n\nShould we move this just after the declaration of v4l2Ctrl above ?\n\n> +\t\tctrls.set(id, std::move(value));\n\nv4l2Ctrls contains pointers to data stored in the ControlValue\ninstances. As far as I can tell the pointers will remain valid, but\nthat's very dependent on the internals of ControlList.\n\nTo be honest, I'm not very fond of patches 1/4 and 2/4 in this series.\nThey make the code more fragile and possibly a bit less efficient\n(although that's likely not significant, as there shouldn't be thousands\nof controls in the list). The VLA removal is fine, but the rest doesn't\nbring much value in my opinion.\n\nLet's split this in two parts, with the fixes first, and the rework on\ntop, so both can be discussed separately. I've posted the former as a\nv6.\n\n> \t}\n> \n> +\tv4l2Ctrls.resize(ctrls.size());\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 +242,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 A75E0BDC92\n\tfor ;\n\tMon, 26 Apr 2021 00:12:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 64D8C6887D;\n\tMon, 26 Apr 2021 02:12:47 +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 2904D6887A\n\tfor ;\n\tMon, 26 Apr 2021 02:12:46 +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 A9217E9;\n\tMon, 26 Apr 2021 02:12:45 +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=\"wMZ6DUKt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619395965;\n\tbh=GXGgliz8/HeyxnQZCEtRodjdFW2SwltPQ8/RVSYd5kg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wMZ6DUKtQ8BdJ5mUEIvOwk4hUlT4cZ8R5gZfLwXbjxzz6DspJ/MafHeS22KFJKUY2\n\tZXPk/gZj3WPmDBYeU6Jkag/GwEf16BFMnMwHfaaLtjjEfjS//RayVp80PCoJSSgfWD\n\tH6DqdDWgMeaUalkEmbEk0bX2+F5m2nsfgYpAabLY=","Date":"Mon, 26 Apr 2021 03:12:41 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210423093653.1726488-1-hiroh@chromium.org>\n\t<20210423093653.1726488-2-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210423093653.1726488-2-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v5 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":16544,"web_url":"https://patchwork.libcamera.org/comment/16544/","msgid":"","date":"2021-04-26T02:01:11","subject":"Re: [libcamera-devel] [PATCH v5 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":"Hi Laurent,\n\nOn Mon, Apr 26, 2021 at 9:12 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Fri, Apr 23, 2021 at 06:36:51PM +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> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/libcamera/v4l2_device.cpp | 41 +++++++++++++++++------------------\n> > 1 file changed, 20 insertions(+), 21 deletions(-)\n> >\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index ce2860c4..bbe8f154 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -173,13 +173,18 @@ 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> > + std::vector v4l2Ctrls(ids.size());\n> > + memset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size());\n> > +\n> > + for (size_t i = 0, j = 0; j < ids.size(); ++j) {\n> > + const unsigned int id = ids[j];\n> > + if (ctrls.contains(id))\n> > + continue;\n> >\n> > - for (uint32_t id : ids) {\n> > const auto iter = controls_.find(id);\n> > if (iter == controls_.end()) {\n> > LOG(V4L2, Error)\n> > @@ -187,20 +192,12 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > return {};\n> > }\n> >\n> > - ctrls.set(id, {});\n> > - }\n> > -\n> > - struct v4l2_ext_control v4l2Ctrls[count];\n> > - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));\n> > -\n> > - unsigned int i = 0;\n> > - for (auto &ctrl : ctrls) {\n> > - unsigned int id = ctrl.first;\n> > + v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i++];\n>\n> You increase i here, ...\n>\n> > const struct v4l2_query_ext_ctrl &info = controlInfo_[id];\n> > + ControlValue value{};\n>\n> ControlValue has a default constructor, no need for {}.\n>\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> > @@ -213,7 +210,6 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > return {};\n> > }\n> >\n> > - ControlValue &value = ctrl.second;\n> > value.reserve(type, true, info.elems);\n> > Span data = value.data();\n> >\n> > @@ -221,21 +217,23 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > v4l2Ctrls[i].size = data.size();\n>\n> ... and use it here. I don't think that's correct.\n>\n> > }\n> >\n> > - v4l2Ctrls[i].id = id;\n> > - i++;\n> > + v4l2Ctrl.id = id;\n>\n> Should we move this just after the declaration of v4l2Ctrl above ?\n>\n> > + ctrls.set(id, std::move(value));\n>\n> v4l2Ctrls contains pointers to data stored in the ControlValue\n> instances. As far as I can tell the pointers will remain valid, but\n> that's very dependent on the internals of ControlList.\n>\n> To be honest, I'm not very fond of patches 1/4 and 2/4 in this series.\n> They make the code more fragile and possibly a bit less efficient\n> (although that's likely not significant, as there shouldn't be thousands\n> of controls in the list). The VLA removal is fine, but the rest doesn't\n> bring much value in my opinion.\n>\n\nCan you clarify more how the new code is fragile and less efficient?\nYou're right, I am sorry that this implementation has some problems.\nBut I think 1/4 is worth doing and correct, and 2/4 is good if I fix the bugs?\n\n-Hiro\n> Let's split this in two parts, with the fixes first, and the rework on\n> top, so both can be discussed separately. I've posted the former as a\n> v6.\n>\n> > }\n> >\n> > + v4l2Ctrls.resize(ctrls.size());\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 +242,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 193ECBDC92\n\tfor ;\n\tMon, 26 Apr 2021 02:01:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5698A6887B;\n\tMon, 26 Apr 2021 04:01:23 +0200 (CEST)","from mail-ej1-x635.google.com (mail-ej1-x635.google.com\n\t[IPv6:2a00:1450:4864:20::635])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3A8606884B\n\tfor ;\n\tMon, 26 Apr 2021 04:01:22 +0200 (CEST)","by mail-ej1-x635.google.com with SMTP id n2so81993702ejy.7\n\tfor ;\n\tSun, 25 Apr 2021 19:01:22 -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=\"M3NgNcS1\"; 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=Lfqoy5DxhXjbTw6K2KgaTYjX10V+Tb3i2qNkhlrCF3E=;\n\tb=M3NgNcS18ecm1HxZQJMemHhcAnOpHzYil4Mz4sEmZYhULozJe2Z/HWiGdYcwHQdOYP\n\tFwPRxlSXpG0WaOPUM1yUiSzcC7G7z/WWDhVtjQLmmLuNq+FjL1Uaefd0l8sqhKgRJkvW\n\tTARhCrTBluFhIg35EXjTcK8geMyJ14D5NKa64=","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=Lfqoy5DxhXjbTw6K2KgaTYjX10V+Tb3i2qNkhlrCF3E=;\n\tb=Ps5L+IORQL40LGKvED/aVqCO7C0LI4+WWpBK4WYBYtBQ82g5oPjFV1fwDR8qiU7q8D\n\to4Ln563FkNnyL0bYa3yGK2MSulnpjbu6ZTuznqFkKwxaCUfmVuCUdTk5L5mRXznOTaiE\n\tRObtJgl/6FP5Q1k/2kRYsvtwLr/3jgdRz31CRVRsYKtS6x9ZUDWi645M9YjE1wJNudbT\n\tOAX+gtGJSO8aFKmViIjiJNNY3l4C4RYj9UTHn5Ng5YIoESV71Zex7dFNqnKEDWTR5QSj\n\tkKN6bbg+3+bv5QG/yof0jdyQM5ScuHSQORYvdYbMXHu4Ht/pVKNeViLDyTxPqBD2JGM8\n\tJafw==","X-Gm-Message-State":"AOAM5335qiFUxtYPT6QcjOtfF8OuCsI4eygnR44wz2B0RUAT9Va8RO5l\n\tLvfU81PyiEIGc3PZDcbALGz0yRO4z+d154DDfcaxSg==","X-Google-Smtp-Source":"ABdhPJxWIrcipq5BU/9BQSFYv+CLMshTNwDftuZbGpR58CILC9o5pd0ovENWh0EjlqEHAJVUGoM7A0tPsShbt78MWtQ=","X-Received":"by 2002:a17:906:46d6:: with SMTP id\n\tk22mr15670844ejs.243.1619402481798; \n\tSun, 25 Apr 2021 19:01:21 -0700 (PDT)","MIME-Version":"1.0","References":"<20210423093653.1726488-1-hiroh@chromium.org>\n\t<20210423093653.1726488-2-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 26 Apr 2021 11:01:11 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v5 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":16547,"web_url":"https://patchwork.libcamera.org/comment/16547/","msgid":"","date":"2021-04-26T02:45:55","subject":"Re: [libcamera-devel] [PATCH v5 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":"Hi Hiro,\n\nOn Mon, Apr 26, 2021 at 11:01:11AM +0900, Hirokazu Honda wrote:\n> On Mon, Apr 26, 2021 at 9:12 AM Laurent Pinchart wrote:\n> > On Fri, Apr 23, 2021 at 06:36:51PM +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> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > src/libcamera/v4l2_device.cpp | 41 +++++++++++++++++------------------\n> > > 1 file changed, 20 insertions(+), 21 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > index ce2860c4..bbe8f154 100644\n> > > --- a/src/libcamera/v4l2_device.cpp\n> > > +++ b/src/libcamera/v4l2_device.cpp\n> > > @@ -173,13 +173,18 @@ 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> > > + std::vector v4l2Ctrls(ids.size());\n> > > + memset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size());\n> > > +\n> > > + for (size_t i = 0, j = 0; j < ids.size(); ++j) {\n> > > + const unsigned int id = ids[j];\n> > > + if (ctrls.contains(id))\n> > > + continue;\n> > >\n> > > - for (uint32_t id : ids) {\n> > > const auto iter = controls_.find(id);\n> > > if (iter == controls_.end()) {\n> > > LOG(V4L2, Error)\n> > > @@ -187,20 +192,12 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > > return {};\n> > > }\n> > >\n> > > - ctrls.set(id, {});\n> > > - }\n> > > -\n> > > - struct v4l2_ext_control v4l2Ctrls[count];\n> > > - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));\n> > > -\n> > > - unsigned int i = 0;\n> > > - for (auto &ctrl : ctrls) {\n> > > - unsigned int id = ctrl.first;\n> > > + v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i++];\n> >\n> > You increase i here, ...\n> >\n> > > const struct v4l2_query_ext_ctrl &info = controlInfo_[id];\n> > > + ControlValue value{};\n> >\n> > ControlValue has a default constructor, no need for {}.\n> >\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> > > @@ -213,7 +210,6 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > > return {};\n> > > }\n> > >\n> > > - ControlValue &value = ctrl.second;\n> > > value.reserve(type, true, info.elems);\n> > > Span data = value.data();\n> > >\n> > > @@ -221,21 +217,23 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > > v4l2Ctrls[i].size = data.size();\n> >\n> > ... and use it here. I don't think that's correct.\n> >\n> > > }\n> > >\n> > > - v4l2Ctrls[i].id = id;\n> > > - i++;\n> > > + v4l2Ctrl.id = id;\n> >\n> > Should we move this just after the declaration of v4l2Ctrl above ?\n> >\n> > > + ctrls.set(id, std::move(value));\n> >\n> > v4l2Ctrls contains pointers to data stored in the ControlValue\n> > instances. As far as I can tell the pointers will remain valid, but\n> > that's very dependent on the internals of ControlList.\n> >\n> > To be honest, I'm not very fond of patches 1/4 and 2/4 in this series.\n> > They make the code more fragile and possibly a bit less efficient\n> > (although that's likely not significant, as there shouldn't be thousands\n> > of controls in the list). The VLA removal is fine, but the rest doesn't\n> > bring much value in my opinion.\n> \n> Can you clarify more how the new code is fragile and less efficient?\n> You're right, I am sorry that this implementation has some problems.\n> But I think 1/4 is worth doing and correct, and 2/4 is good if I fix the bugs?\n\nThe efficiency is related to the additional lookup in updateControls()\n(the O(N*log(N)) we've discussed previously). I think it's a theoretical\nproblem only, given the expected size of the control list, I expect the\ndifference to be completely negligible in practice. As for the\nfragility, I was referring to the fact that we store ub v4l2Ctrls\npointers to data from the ControlValue, stored in the ControlList. Any\nreallocation in the underlying container would make those pointers\ninvalid. It's an existing issue, but with the current implementation, we\ndon't touch the ControlList after adding controls to it at the beginning\nof getControls(), while with this patch we rely on\n\n\tctrls.set(id, std::move(value));\n\nto not cause a reallocation. I think that's guaranteed by the current\nimplementation of ControlList, but if that changes later, we'll possibly\nhave hard to debug bugs.\n\nThese two points are not blocker by themselves, but the gains should\noutweight the risks. It will be easier to check that after rebasing\npatches 1/4 and 2/4 on top of the VLA removal.\n\n> > Let's split this in two parts, with the fixes first, and the rework on\n> > top, so both can be discussed separately. I've posted the former as a\n> > v6.\n> >\n> > > }\n> > >\n> > > + v4l2Ctrls.resize(ctrls.size());\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 +242,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> > > }","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 D06F0BDC92\n\tfor ;\n\tMon, 26 Apr 2021 02:46:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0EE536887B;\n\tMon, 26 Apr 2021 04:46:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 880DE6884B\n\tfor ;\n\tMon, 26 Apr 2021 04:46:01 +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 DF7634FB;\n\tMon, 26 Apr 2021 04:46:00 +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=\"d2SpquQU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619405161;\n\tbh=dh6lf5ZrIHb/MEwH54Od3ymoi2LMQSASMXbc6d/xVjk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=d2SpquQUFurCcIOEfJoRQToxJlJ/Qf7IEcnMGTeuygVPtLEbG70lKn56K5Fgm9tf+\n\tYUdFPpRISIMVdmKOI4oqHDuA9R5fMvGmeuGCXqgdleU0iTnYSBxKF0Djj5x/Kupwyo\n\tuMEw8dliR+lQW1Bv4RV662xlby7zJjzlaPqt2njY=","Date":"Mon, 26 Apr 2021 05:45:55 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210423093653.1726488-1-hiroh@chromium.org>\n\t<20210423093653.1726488-2-hiroh@chromium.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 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":16550,"web_url":"https://patchwork.libcamera.org/comment/16550/","msgid":"","date":"2021-04-26T02:55:53","subject":"Re: [libcamera-devel] [PATCH v5 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 Mon, Apr 26, 2021 at 11:46 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Mon, Apr 26, 2021 at 11:01:11AM +0900, Hirokazu Honda wrote:\n> > On Mon, Apr 26, 2021 at 9:12 AM Laurent Pinchart wrote:\n> > > On Fri, Apr 23, 2021 at 06:36:51PM +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> > > > Signed-off-by: Hirokazu Honda \n> > > > ---\n> > > > src/libcamera/v4l2_device.cpp | 41 +++++++++++++++++------------------\n> > > > 1 file changed, 20 insertions(+), 21 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > > index ce2860c4..bbe8f154 100644\n> > > > --- a/src/libcamera/v4l2_device.cpp\n> > > > +++ b/src/libcamera/v4l2_device.cpp\n> > > > @@ -173,13 +173,18 @@ 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> > > > + std::vector v4l2Ctrls(ids.size());\n> > > > + memset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size());\n> > > > +\n> > > > + for (size_t i = 0, j = 0; j < ids.size(); ++j) {\n> > > > + const unsigned int id = ids[j];\n> > > > + if (ctrls.contains(id))\n> > > > + continue;\n> > > >\n> > > > - for (uint32_t id : ids) {\n> > > > const auto iter = controls_.find(id);\n> > > > if (iter == controls_.end()) {\n> > > > LOG(V4L2, Error)\n> > > > @@ -187,20 +192,12 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > > > return {};\n> > > > }\n> > > >\n> > > > - ctrls.set(id, {});\n> > > > - }\n> > > > -\n> > > > - struct v4l2_ext_control v4l2Ctrls[count];\n> > > > - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));\n> > > > -\n> > > > - unsigned int i = 0;\n> > > > - for (auto &ctrl : ctrls) {\n> > > > - unsigned int id = ctrl.first;\n> > > > + v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i++];\n> > >\n> > > You increase i here, ...\n> > >\n> > > > const struct v4l2_query_ext_ctrl &info = controlInfo_[id];\n> > > > + ControlValue value{};\n> > >\n> > > ControlValue has a default constructor, no need for {}.\n> > >\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> > > > @@ -213,7 +210,6 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > > > return {};\n> > > > }\n> > > >\n> > > > - ControlValue &value = ctrl.second;\n> > > > value.reserve(type, true, info.elems);\n> > > > Span data = value.data();\n> > > >\n> > > > @@ -221,21 +217,23 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > > > v4l2Ctrls[i].size = data.size();\n> > >\n> > > ... and use it here. I don't think that's correct.\n> > >\n> > > > }\n> > > >\n> > > > - v4l2Ctrls[i].id = id;\n> > > > - i++;\n> > > > + v4l2Ctrl.id = id;\n> > >\n> > > Should we move this just after the declaration of v4l2Ctrl above ?\n> > >\n> > > > + ctrls.set(id, std::move(value));\n> > >\n> > > v4l2Ctrls contains pointers to data stored in the ControlValue\n> > > instances. As far as I can tell the pointers will remain valid, but\n> > > that's very dependent on the internals of ControlList.\n> > >\n> > > To be honest, I'm not very fond of patches 1/4 and 2/4 in this series.\n> > > They make the code more fragile and possibly a bit less efficient\n> > > (although that's likely not significant, as there shouldn't be thousands\n> > > of controls in the list). The VLA removal is fine, but the rest doesn't\n> > > bring much value in my opinion.\n> >\n> > Can you clarify more how the new code is fragile and less efficient?\n> > You're right, I am sorry that this implementation has some problems.\n> > But I think 1/4 is worth doing and correct, and 2/4 is good if I fix the bugs?\n>\n> The efficiency is related to the additional lookup in updateControls()\n> (the O(N*log(N)) we've discussed previously). I think it's a theoretical\n> problem only, given the expected size of the control list, I expect the\n> difference to be completely negligible in practice. As for the\n> fragility, I was referring to the fact that we store ub v4l2Ctrls\n> pointers to data from the ControlValue, stored in the ControlList. Any\n> reallocation in the underlying container would make those pointers\n> invalid. It's an existing issue, but with the current implementation, we\n> don't touch the ControlList after adding controls to it at the beginning\n> of getControls(), while with this patch we rely on\n>\n> ctrls.set(id, std::move(value));\n>\n\nYes, this should be ctrls.set(id, {}) or ctrls.set(id, value).\nSorry this std::move() is just confusing, which is actually equivalent\nto ctrls.set(id, value).\n\n> to not cause a reallocation. I think that's guaranteed by the current\n> implementation of ControlList, but if that changes later, we'll possibly\n> have hard to debug bugs.\n>\n> These two points are not blocker by themselves, but the gains should\n> outweight the risks. It will be easier to check that after rebasing\n> patches 1/4 and 2/4 on top of the VLA removal.\n>\n\nOkay, I would like to merge patch 1/4 at least.\nI think our life will be easier if we have utils::enumerate().\n\n> > > Let's split this in two parts, with the fixes first, and the rework on\n> > > top, so both can be discussed separately. I've posted the former as a\n> > > v6.\n> > >\n> > > > }\n> > > >\n> > > > + v4l2Ctrls.resize(ctrls.size());\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 +242,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 8C42CBDC92\n\tfor ;\n\tMon, 26 Apr 2021 02:56:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0DA2A6887B;\n\tMon, 26 Apr 2021 04:56:06 +0200 (CEST)","from mail-ed1-x535.google.com (mail-ed1-x535.google.com\n\t[IPv6:2a00:1450:4864:20::535])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 47224605BD\n\tfor ;\n\tMon, 26 Apr 2021 04:56:04 +0200 (CEST)","by mail-ed1-x535.google.com with SMTP id d14so91638edc.12\n\tfor ;\n\tSun, 25 Apr 2021 19:56:04 -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=\"dK56BR5L\"; 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=242Kq9IedKw2Y6kid5F568/UzzjIZeE9q+tmwHxhxmA=;\n\tb=dK56BR5L7U845KaLFL5U4LJAzPwjc4SHQDgMYR9x4TNcAuG76b2zfb5G8y5hpfEjBx\n\tUoPPiZQre2qQMMFEdMmg2kIc8lDqVZxFmECcAVa6NHkLcJuLA4MYeeCI9VTHw7uqBPzY\n\tWW+JHGq4S1AMGwp6eE/ctjpnlNHUoibJBtkrU=","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=242Kq9IedKw2Y6kid5F568/UzzjIZeE9q+tmwHxhxmA=;\n\tb=Mz2rL09JvFmjQlkUUxJYPuKExkCqo2fwm/IyorZzl8zOg2IcWlgxCwwQ1uyVTvKASc\n\tr7Y9B41e+8SeNzGnPxl3B7IpVjMlP38Elq7sDRaRgqkWUsLFL6LEVRI9EX0ydHeDRWcE\n\tr/vfdHfAFOCb0KVwBTQ0hKavOJr9dzR/Cs4y56N4m+oXZ+10QCpblxQXnNk7L77apdxU\n\t+TreOFx9aBLNggvY7mwJKo8/mtgM18vWzU+XOwy6DuEn1uDN8nBYHOsmcPv4KVXQhatk\n\ta6hieA54oak3RK85XhYb77B3e+/26hHO5R/iNeiVXapOO4MJ5/hy9B8JhIizdAHHJdYm\n\t6z+Q==","X-Gm-Message-State":"AOAM530OBhon73eaafniKUl4uKo3GvQXNFz3BzAiumArnFBS4/qfK5ab\n\tYdoApRyPFefWa2b9k0gn+qPLGFh2V0zeTCBMyDqu/tw7VbM=","X-Google-Smtp-Source":"ABdhPJz9ixM4/P2rVmQBzZkE7SvZPD8ESaLWaZlzkLgKv5yJ/wfDvyxjRimEMyCMm6xgDY5KQ/je6sxE8nA9FYmMX/Q=","X-Received":"by 2002:aa7:d30b:: with SMTP id p11mr567804edq.325.1619405763900;\n\tSun, 25 Apr 2021 19:56:03 -0700 (PDT)","MIME-Version":"1.0","References":"<20210423093653.1726488-1-hiroh@chromium.org>\n\t<20210423093653.1726488-2-hiroh@chromium.org>\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 26 Apr 2021 11:55:53 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v5 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":16554,"web_url":"https://patchwork.libcamera.org/comment/16554/","msgid":"","date":"2021-04-26T03:22:13","subject":"Re: [libcamera-devel] [PATCH v5 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":"Hi Hiro,\n\nOn Mon, Apr 26, 2021 at 11:55:53AM +0900, Hirokazu Honda wrote:\n> On Mon, Apr 26, 2021 at 11:46 AM Laurent Pinchart wrote:\n> > On Mon, Apr 26, 2021 at 11:01:11AM +0900, Hirokazu Honda wrote:\n> > > On Mon, Apr 26, 2021 at 9:12 AM Laurent Pinchart wrote:\n> > > > On Fri, Apr 23, 2021 at 06:36:51PM +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> > > > > Signed-off-by: Hirokazu Honda \n> > > > > ---\n> > > > > src/libcamera/v4l2_device.cpp | 41 +++++++++++++++++------------------\n> > > > > 1 file changed, 20 insertions(+), 21 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > > > index ce2860c4..bbe8f154 100644\n> > > > > --- a/src/libcamera/v4l2_device.cpp\n> > > > > +++ b/src/libcamera/v4l2_device.cpp\n> > > > > @@ -173,13 +173,18 @@ 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> > > > > + std::vector v4l2Ctrls(ids.size());\n> > > > > + memset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size());\n> > > > > +\n> > > > > + for (size_t i = 0, j = 0; j < ids.size(); ++j) {\n> > > > > + const unsigned int id = ids[j];\n> > > > > + if (ctrls.contains(id))\n> > > > > + continue;\n> > > > >\n> > > > > - for (uint32_t id : ids) {\n> > > > > const auto iter = controls_.find(id);\n> > > > > if (iter == controls_.end()) {\n> > > > > LOG(V4L2, Error)\n> > > > > @@ -187,20 +192,12 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > > > > return {};\n> > > > > }\n> > > > >\n> > > > > - ctrls.set(id, {});\n> > > > > - }\n> > > > > -\n> > > > > - struct v4l2_ext_control v4l2Ctrls[count];\n> > > > > - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));\n> > > > > -\n> > > > > - unsigned int i = 0;\n> > > > > - for (auto &ctrl : ctrls) {\n> > > > > - unsigned int id = ctrl.first;\n> > > > > + v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i++];\n> > > >\n> > > > You increase i here, ...\n> > > >\n> > > > > const struct v4l2_query_ext_ctrl &info = controlInfo_[id];\n> > > > > + ControlValue value{};\n> > > >\n> > > > ControlValue has a default constructor, no need for {}.\n> > > >\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> > > > > @@ -213,7 +210,6 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > > > > return {};\n> > > > > }\n> > > > >\n> > > > > - ControlValue &value = ctrl.second;\n> > > > > value.reserve(type, true, info.elems);\n> > > > > Span data = value.data();\n> > > > >\n> > > > > @@ -221,21 +217,23 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > > > > v4l2Ctrls[i].size = data.size();\n> > > >\n> > > > ... and use it here. I don't think that's correct.\n> > > >\n> > > > > }\n> > > > >\n> > > > > - v4l2Ctrls[i].id = id;\n> > > > > - i++;\n> > > > > + v4l2Ctrl.id = id;\n> > > >\n> > > > Should we move this just after the declaration of v4l2Ctrl above ?\n> > > >\n> > > > > + ctrls.set(id, std::move(value));\n> > > >\n> > > > v4l2Ctrls contains pointers to data stored in the ControlValue\n> > > > instances. As far as I can tell the pointers will remain valid, but\n> > > > that's very dependent on the internals of ControlList.\n> > > >\n> > > > To be honest, I'm not very fond of patches 1/4 and 2/4 in this series.\n> > > > They make the code more fragile and possibly a bit less efficient\n> > > > (although that's likely not significant, as there shouldn't be thousands\n> > > > of controls in the list). The VLA removal is fine, but the rest doesn't\n> > > > bring much value in my opinion.\n> > >\n> > > Can you clarify more how the new code is fragile and less efficient?\n> > > You're right, I am sorry that this implementation has some problems.\n> > > But I think 1/4 is worth doing and correct, and 2/4 is good if I fix the bugs?\n> >\n> > The efficiency is related to the additional lookup in updateControls()\n> > (the O(N*log(N)) we've discussed previously). I think it's a theoretical\n> > problem only, given the expected size of the control list, I expect the\n> > difference to be completely negligible in practice. As for the\n> > fragility, I was referring to the fact that we store ub v4l2Ctrls\n> > pointers to data from the ControlValue, stored in the ControlList. Any\n> > reallocation in the underlying container would make those pointers\n> > invalid. It's an existing issue, but with the current implementation, we\n> > don't touch the ControlList after adding controls to it at the beginning\n> > of getControls(), while with this patch we rely on\n> >\n> > ctrls.set(id, std::move(value));\n> \n> Yes, this should be ctrls.set(id, {}) or ctrls.set(id, value).\n> Sorry this std::move() is just confusing, which is actually equivalent\n> to ctrls.set(id, value).\n> \n> > to not cause a reallocation. I think that's guaranteed by the current\n> > implementation of ControlList, but if that changes later, we'll possibly\n> > have hard to debug bugs.\n> >\n> > These two points are not blocker by themselves, but the gains should\n> > outweight the risks. It will be easier to check that after rebasing\n> > patches 1/4 and 2/4 on top of the VLA removal.\n> \n> Okay, I would like to merge patch 1/4 at least.\n> I think our life will be easier if we have utils::enumerate().\n\nIf you can review that patch, I'll merge it :-)\n\n> > > > Let's split this in two parts, with the fixes first, and the rework on\n> > > > top, so both can be discussed separately. I've posted the former as a\n> > > > v6.\n> > > >\n> > > > > }\n> > > > >\n> > > > > + v4l2Ctrls.resize(ctrls.size());\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 +242,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> > > > > }","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 706BEBDC92\n\tfor ;\n\tMon, 26 Apr 2021 03:22:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D1AB76884B;\n\tMon, 26 Apr 2021 05:22:19 +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 DD86A6884B\n\tfor ;\n\tMon, 26 Apr 2021 05:22:18 +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 502154FB;\n\tMon, 26 Apr 2021 05:22:18 +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=\"kTn2MQQN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619407338;\n\tbh=Bmi/yMxLvcdBi38O/Q4TXdXO7HRAtIuSbaoO67HkbKw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kTn2MQQNhtGcX0g21z2sJeM5Fvn+RgzJ/zj0kF74LGomxCoC8yiB5cheF/8cRzk2T\n\t0SN0xb7hxy3zxhOXcBbl0qZYh6hIT2EPW5Ev5vXwgn7PkS1WfN/rxto8Qr7N85+SSQ\n\tUElKVfxocNORuOULr/+B5eeTS74RFIvjSB6UbAL4=","Date":"Mon, 26 Apr 2021 06:22:13 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210423093653.1726488-1-hiroh@chromium.org>\n\t<20210423093653.1726488-2-hiroh@chromium.org>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 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\" "}}]