[{"id":16372,"web_url":"https://patchwork.libcamera.org/comment/16372/","msgid":"","date":"2021-04-20T00:59:45","subject":"Re: [libcamera-devel] [PATCH v2 1/3] 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 Thu, Apr 15, 2021 at 01:48:52PM +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 | 43 ++++++++++++++++-------------------\n> 1 file changed, 20 insertions(+), 23 deletions(-)\n> \n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 8fd79934..ee8c3fed 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\nUnrelated to this patch, it occurred to me that the function should take\na Span instead of a vector. Would you like to submit an\nadditional patch ?\n\n> {\n> -\tunsigned int count = ids.size();\n> -\tif (count == 0)\n> +\tif (ids.empty())\n> \t\treturn {};\n> \n> \tControlList ctrls;\n> @@ -196,22 +195,25 @@ 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> -\n> -\tunsigned int i = 0;\n> +\tstd::vector v4l2Ctrls;\n> +\tv4l2Ctrls.reserve(ctrls.size());\n\nThis changes the behaviour slightly, in that if the ids vector contains\nthe same ID multiple times, you'll allocate less memory here. That's\nactually better :-)\n\n> \tfor (auto &ctrl : ctrls) {\n> \t\tunsigned int id = ctrl.first;\n> \t\tconst struct v4l2_query_ext_ctrl &info = controlInfo_[id];\n> +\t\tv4l2_ext_control v4l2Ctrl;\n> +\t\tmemset(&v4l2Ctrl, 0, sizeof(v4l2Ctrl));\n> \n> \t\tif (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {\n> -\t\t\tControlType type;\n> +\t\t\tControlValue &value = ctrl.second;\n> \n> \t\t\tswitch (info.type) {\n> -\t\t\tcase V4L2_CTRL_TYPE_U8:\n> -\t\t\t\ttype = ControlTypeByte;\n> +\t\t\tcase V4L2_CTRL_TYPE_U8: {\n> +\t\t\t\tvalue.reserve(ControlTypeByte, true, info.elems);\n> +\t\t\t\tSpan data = value.data();\n> +\t\t\t\tv4l2Ctrl.p_u8 = data.data();\n> +\t\t\t\tv4l2Ctrl.size = data.size();\n> \t\t\t\tbreak;\n> -\n> +\t\t\t}\n> \t\t\tdefault:\n> \t\t\t\tLOG(V4L2, Error)\n> \t\t\t\t\t<< \"Unsupported payload control type \"\n> @@ -219,29 +221,23 @@ 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> -\t\t\tv4l2Ctrls[i].p_u8 = data.data();\n> -\t\t\tv4l2Ctrls[i].size = data.size();\n\nIs there a need to move this code to the switch ? It looks fairly\ngeneric, it would be nice not to duplicate it when (if) we latter add\nsupport for other control types (such as V4L2_CTRL_TYPE_U16).\n\n> \t\t}\n> \n> -\t\tv4l2Ctrls[i].id = id;\n> -\t\ti++;\n> +\t\tv4l2Ctrl.id = id;\n> +\t\tv4l2Ctrls.push_back(std::move(v4l2Ctrl));\n\nThe move won't help much, it will copy the v4l2_ext_control instance.\nShould we resize() instead of reserve(), and operate directly on the\nvector entry in this loop ?\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> @@ -250,10 +246,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\nupdateControls should also take a Span argument, that could be done in\nthe same patch that updates the argument to getControls().\n\nOverall this patch is good, the comments above are small issues.\n\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 76FFDBD814\n\tfor ;\n\tTue, 20 Apr 2021 00:59:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D486C6883C;\n\tTue, 20 Apr 2021 02:59:51 +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 89786602CA\n\tfor ;\n\tTue, 20 Apr 2021 02:59:50 +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 02F4E470;\n\tTue, 20 Apr 2021 02:59:49 +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=\"KZSFS76R\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618880390;\n\tbh=Vn84rwhddIULvJNMxlE0uQvTIp9GWIKpyo3Z8qapOVY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KZSFS76RND7G1fg+Lnri4v6g+Kz4fAVikxBfZAxftgbwUFpOet0wRAprJvlYdsxZj\n\tkP0FcZ7HRaBH3oErTqodKSQ0T5ClCK+UlIzCFk8xM8eBiaz0W8fbx94h4znxotmYSm\n\toRqGuY1mOXeIFdPM6s+K0PYRYi3ZRBcsHTyjsXhE=","Date":"Tue, 20 Apr 2021 03:59:45 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210415044854.3348529-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210415044854.3348529-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] 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":16472,"web_url":"https://patchwork.libcamera.org/comment/16472/","msgid":"","date":"2021-04-22T02:09:06","subject":"Re: [libcamera-devel] [PATCH v2 1/3] 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 Tue, Apr 20, 2021 at 9:59 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Thu, Apr 15, 2021 at 01:48:52PM +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 | 43 ++++++++++++++++-------------------\n> > 1 file changed, 20 insertions(+), 23 deletions(-)\n> >\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 8fd79934..ee8c3fed 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> Unrelated to this patch, it occurred to me that the function should take\n> a Span instead of a vector. Would you like to submit an\n> additional patch ?\n>\n\nI tried this today. Span has an issue in a call like the following?\n\n../src/libcamera/camera_sensor.cpp:804:43: error: no matching\nconstructor for initialization of 'Span' (aka\n'Span')\n ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,\n\n\n> > {\n> > - unsigned int count = ids.size();\n> > - if (count == 0)\n> > + if (ids.empty())\n> > return {};\n> >\n> > ControlList ctrls;\n> > @@ -196,22 +195,25 @@ 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> > -\n> > - unsigned int i = 0;\n> > + std::vector v4l2Ctrls;\n> > + v4l2Ctrls.reserve(ctrls.size());\n>\n> This changes the behaviour slightly, in that if the ids vector contains\n> the same ID multiple times, you'll allocate less memory here. That's\n> actually better :-)\n>\n> > for (auto &ctrl : ctrls) {\n> > unsigned int id = ctrl.first;\n> > const struct v4l2_query_ext_ctrl &info = controlInfo_[id];\n> > + v4l2_ext_control v4l2Ctrl;\n> > + memset(&v4l2Ctrl, 0, sizeof(v4l2Ctrl));\n> >\n> > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {\n> > - ControlType type;\n> > + ControlValue &value = ctrl.second;\n> >\n> > switch (info.type) {\n> > - case V4L2_CTRL_TYPE_U8:\n> > - type = ControlTypeByte;\n> > + case V4L2_CTRL_TYPE_U8: {\n> > + value.reserve(ControlTypeByte, true, info.elems);\n> > + Span data = value.data();\n> > + v4l2Ctrl.p_u8 = data.data();\n> > + v4l2Ctrl.size = data.size();\n> > break;\n> > -\n> > + }\n> > default:\n> > LOG(V4L2, Error)\n> > << \"Unsupported payload control type \"\n> > @@ -219,29 +221,23 @@ 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> > - v4l2Ctrls[i].p_u8 = data.data();\n> > - v4l2Ctrls[i].size = data.size();\n>\n> Is there a need to move this code to the switch ? It looks fairly\n> generic, it would be nice not to duplicate it when (if) we latter add\n> support for other control types (such as V4L2_CTRL_TYPE_U16).\n>\n> > }\n> >\n> > - v4l2Ctrls[i].id = id;\n> > - i++;\n> > + v4l2Ctrl.id = id;\n> > + v4l2Ctrls.push_back(std::move(v4l2Ctrl));\n>\n> The move won't help much, it will copy the v4l2_ext_control instance.\n> Should we resize() instead of reserve(), and operate directly on the\n> vector entry in this loop ?\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> > @@ -250,10 +246,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> updateControls should also take a Span argument, that could be done in\n> the same patch that updates the argument to getControls().\n>\n> Overall this patch is good, the comments above are small issues.\n>\n> >\n> > return ctrls;\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 ACA65BDB15\n\tfor ;\n\tThu, 22 Apr 2021 02:09:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C176B68847;\n\tThu, 22 Apr 2021 04:09:18 +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 4FEE460516\n\tfor ;\n\tThu, 22 Apr 2021 04:09:17 +0200 (CEST)","by mail-ed1-x535.google.com with SMTP id e7so51474945edu.10\n\tfor ;\n\tWed, 21 Apr 2021 19:09:17 -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=\"SLz/cOvc\"; 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=b+pM2aXJStIsIiHet3nC1n/EO6feOUcA8RrY/hZDUcQ=;\n\tb=SLz/cOvcDREypxd1wOgbhQtQwxTS7kLgbC+rGp0HS6dJb1YbbS6b7rZszjKSOAyH3G\n\tBysUCj4CKGodvKgt+qivFZQh8BV3Es++g7khapPcYkGsAIYa0Hr1Het+WD/7veppAYYO\n\ts+3dGKviX9OAvKVYLdzHGoqJPmK1DxxYfhppw=","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=b+pM2aXJStIsIiHet3nC1n/EO6feOUcA8RrY/hZDUcQ=;\n\tb=P0ysnFsTFnmlibkTFj08uP9cgYj59Yz1eqAAA6FkqaX/eVHJVT3Y5sTRR2yBQbXO84\n\tGpEoXNY0GOx19qkBjdGS/A/NrvdEOcw1H+Y7uNPiES3jvRaXLuDPknd4pUFBLBbYBRt1\n\tVjIjRd8nfr7FtRO8tzzg4xyydwj3rMBPxRR61OzFAlTuOm2JiSMLwta9Z+55xcZ+9SYg\n\tkAsBA9B8XxRtV0SQLOtsj959y2Gh+UM40mY2rGBKI3JmYHSB359Csh7XonR/UITMqrzf\n\txl4HorduOdrxEMn9gQ2a/wRFwlsCqR0IzMXElejORivDQhF+NCRw+x+O19OZg8OvyWCl\n\tG9aw==","X-Gm-Message-State":"AOAM533lHt4vmGleY/mHQQD/YuBRPj9EKWjSxmaE/uk3nYpV1kgtOKs/\n\tnJlcr4czGGXpqYNGkLK7gkrAwyQtgGS9B5/x1bE9NaJEE6JZYw==","X-Google-Smtp-Source":"ABdhPJyzFjuUyTgCV+hzQlcr3fEbUXKdVsZCohzaOiHlF5VFZVk+dz2HA1hOwgAZ5w5rTbKyrgr30JB5CPb6uch5f2o=","X-Received":"by 2002:a05:6402:5211:: with SMTP id\n\ts17mr980197edd.327.1619057356779; \n\tWed, 21 Apr 2021 19:09:16 -0700 (PDT)","MIME-Version":"1.0","References":"<20210415044854.3348529-1-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Thu, 22 Apr 2021 11:09:06 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] 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":16595,"web_url":"https://patchwork.libcamera.org/comment/16595/","msgid":"","date":"2021-04-27T01:25:02","subject":"Re: [libcamera-devel] [PATCH v2 1/3] 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 Thu, Apr 22, 2021 at 11:09:06AM +0900, Hirokazu Honda wrote:\n> On Tue, Apr 20, 2021 at 9:59 AM Laurent Pinchart wrote:\n> > On Thu, Apr 15, 2021 at 01:48:52PM +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 | 43 ++++++++++++++++-------------------\n> > > 1 file changed, 20 insertions(+), 23 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > index 8fd79934..ee8c3fed 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> > Unrelated to this patch, it occurred to me that the function should take\n> > a Span instead of a vector. Would you like to submit an\n> > additional patch ?\n> \n> I tried this today. Span has an issue in a call like the following?\n> \n> ../src/libcamera/camera_sensor.cpp:804:43: error: no matching\n> constructor for initialization of 'Span' (aka\n> 'Span')\n> ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,\n\nIndeed, the span class has no constructor that takes an\nstd::initializer_list. This could be solved by constructing the\ninitializer list explicitly in the caller (but would still require the\nfollowing patch).\n\ndiff --git a/include/libcamera/span.h b/include/libcamera/span.h\nindex 7a35806b1710..33e059aac102 100644\n--- a/include/libcamera/span.h\n+++ b/include/libcamera/span.h\n@@ -67,6 +67,12 @@ constexpr T *data(T (&array)[N]) noexcept\n \treturn array;\n }\n\n+template\n+constexpr const T *data(std::initializer_list c) noexcept\n+{\n+\treturn c.begin();\n+}\n+\n template\n struct tuple_element;\n\nAlternatively, we could add a consructor take takes an initializer list.\nLet's postpone this for now.\n\n> > > {\n> > > - unsigned int count = ids.size();\n> > > - if (count == 0)\n> > > + if (ids.empty())\n> > > return {};\n> > >\n> > > ControlList ctrls;\n> > > @@ -196,22 +195,25 @@ 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> > > -\n> > > - unsigned int i = 0;\n> > > + std::vector v4l2Ctrls;\n> > > + v4l2Ctrls.reserve(ctrls.size());\n> >\n> > This changes the behaviour slightly, in that if the ids vector contains\n> > the same ID multiple times, you'll allocate less memory here. That's\n> > actually better :-)\n> >\n> > > for (auto &ctrl : ctrls) {\n> > > unsigned int id = ctrl.first;\n> > > const struct v4l2_query_ext_ctrl &info = controlInfo_[id];\n> > > + v4l2_ext_control v4l2Ctrl;\n> > > + memset(&v4l2Ctrl, 0, sizeof(v4l2Ctrl));\n> > >\n> > > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {\n> > > - ControlType type;\n> > > + ControlValue &value = ctrl.second;\n> > >\n> > > switch (info.type) {\n> > > - case V4L2_CTRL_TYPE_U8:\n> > > - type = ControlTypeByte;\n> > > + case V4L2_CTRL_TYPE_U8: {\n> > > + value.reserve(ControlTypeByte, true, info.elems);\n> > > + Span data = value.data();\n> > > + v4l2Ctrl.p_u8 = data.data();\n> > > + v4l2Ctrl.size = data.size();\n> > > break;\n> > > -\n> > > + }\n> > > default:\n> > > LOG(V4L2, Error)\n> > > << \"Unsupported payload control type \"\n> > > @@ -219,29 +221,23 @@ 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> > > - v4l2Ctrls[i].p_u8 = data.data();\n> > > - v4l2Ctrls[i].size = data.size();\n> >\n> > Is there a need to move this code to the switch ? It looks fairly\n> > generic, it would be nice not to duplicate it when (if) we latter add\n> > support for other control types (such as V4L2_CTRL_TYPE_U16).\n> >\n> > > }\n> > >\n> > > - v4l2Ctrls[i].id = id;\n> > > - i++;\n> > > + v4l2Ctrl.id = id;\n> > > + v4l2Ctrls.push_back(std::move(v4l2Ctrl));\n> >\n> > The move won't help much, it will copy the v4l2_ext_control instance.\n> > Should we resize() instead of reserve(), and operate directly on the\n> > vector entry in this loop ?\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> > > @@ -250,10 +246,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> > updateControls should also take a Span argument, that could be done in\n> > the same patch that updates the argument to getControls().\n> >\n> > Overall this patch is good, the comments above are small issues.\n> >\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 86327BDCA1\n\tfor ;\n\tTue, 27 Apr 2021 01:25:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4AE38688AE;\n\tTue, 27 Apr 2021 03:25:11 +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 55A07605BD\n\tfor ;\n\tTue, 27 Apr 2021 03:25:09 +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 DC54DE9;\n\tTue, 27 Apr 2021 03:25:08 +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=\"bCUFZlH0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619486709;\n\tbh=XpnZEjn1gOCQhTm0sXmQOI6+oP4BJfn11DiKdZMVq9A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bCUFZlH0b1GfRB7r8pvmFNInl7SpmbmuB7p8RJFFUx/XR5RRjWfnww59yp14ewGnx\n\tIbLI+TJgwkyb0jkYgQKHTEfPLT4UEfNrExaY8sy8vqOHje7VfDi60uqNo6GXj1pQ8C\n\tZhWxRJhcJie8EamHO7XBGMhQ4SdGet+abkQXKanQ=","Date":"Tue, 27 Apr 2021 04:25:02 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210415044854.3348529-1-hiroh@chromium.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] 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":16596,"web_url":"https://patchwork.libcamera.org/comment/16596/","msgid":"","date":"2021-04-27T01:28:26","subject":"Re: [libcamera-devel] [PATCH v2 1/3] 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 Tue, Apr 27, 2021 at 10:25 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Thu, Apr 22, 2021 at 11:09:06AM +0900, Hirokazu Honda wrote:\n> > On Tue, Apr 20, 2021 at 9:59 AM Laurent Pinchart wrote:\n> > > On Thu, Apr 15, 2021 at 01:48:52PM +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 | 43 ++++++++++++++++-------------------\n> > > > 1 file changed, 20 insertions(+), 23 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > > index 8fd79934..ee8c3fed 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> > > Unrelated to this patch, it occurred to me that the function should take\n> > > a Span instead of a vector. Would you like to submit an\n> > > additional patch ?\n> >\n> > I tried this today. Span has an issue in a call like the following?\n> >\n> > ../src/libcamera/camera_sensor.cpp:804:43: error: no matching\n> > constructor for initialization of 'Span' (aka\n> > 'Span')\n> > ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,\n>\n> Indeed, the span class has no constructor that takes an\n> std::initializer_list. This could be solved by constructing the\n> initializer list explicitly in the caller (but would still require the\n> following patch).\n>\n\nThanks. I note the task to my task sets which I deal with when I have\nquite a bit time.\n-Hiro\n\n> diff --git a/include/libcamera/span.h b/include/libcamera/span.h\n> index 7a35806b1710..33e059aac102 100644\n> --- a/include/libcamera/span.h\n> +++ b/include/libcamera/span.h\n> @@ -67,6 +67,12 @@ constexpr T *data(T (&array)[N]) noexcept\n> return array;\n> }\n>\n> +template\n> +constexpr const T *data(std::initializer_list c) noexcept\n> +{\n> + return c.begin();\n> +}\n> +\n> template\n> struct tuple_element;\n>\n> Alternatively, we could add a consructor take takes an initializer list.\n> Let's postpone this for now.\n>\n> > > > {\n> > > > - unsigned int count = ids.size();\n> > > > - if (count == 0)\n> > > > + if (ids.empty())\n> > > > return {};\n> > > >\n> > > > ControlList ctrls;\n> > > > @@ -196,22 +195,25 @@ 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> > > > -\n> > > > - unsigned int i = 0;\n> > > > + std::vector v4l2Ctrls;\n> > > > + v4l2Ctrls.reserve(ctrls.size());\n> > >\n> > > This changes the behaviour slightly, in that if the ids vector contains\n> > > the same ID multiple times, you'll allocate less memory here. That's\n> > > actually better :-)\n> > >\n> > > > for (auto &ctrl : ctrls) {\n> > > > unsigned int id = ctrl.first;\n> > > > const struct v4l2_query_ext_ctrl &info = controlInfo_[id];\n> > > > + v4l2_ext_control v4l2Ctrl;\n> > > > + memset(&v4l2Ctrl, 0, sizeof(v4l2Ctrl));\n> > > >\n> > > > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {\n> > > > - ControlType type;\n> > > > + ControlValue &value = ctrl.second;\n> > > >\n> > > > switch (info.type) {\n> > > > - case V4L2_CTRL_TYPE_U8:\n> > > > - type = ControlTypeByte;\n> > > > + case V4L2_CTRL_TYPE_U8: {\n> > > > + value.reserve(ControlTypeByte, true, info.elems);\n> > > > + Span data = value.data();\n> > > > + v4l2Ctrl.p_u8 = data.data();\n> > > > + v4l2Ctrl.size = data.size();\n> > > > break;\n> > > > -\n> > > > + }\n> > > > default:\n> > > > LOG(V4L2, Error)\n> > > > << \"Unsupported payload control type \"\n> > > > @@ -219,29 +221,23 @@ 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> > > > - v4l2Ctrls[i].p_u8 = data.data();\n> > > > - v4l2Ctrls[i].size = data.size();\n> > >\n> > > Is there a need to move this code to the switch ? It looks fairly\n> > > generic, it would be nice not to duplicate it when (if) we latter add\n> > > support for other control types (such as V4L2_CTRL_TYPE_U16).\n> > >\n> > > > }\n> > > >\n> > > > - v4l2Ctrls[i].id = id;\n> > > > - i++;\n> > > > + v4l2Ctrl.id = id;\n> > > > + v4l2Ctrls.push_back(std::move(v4l2Ctrl));\n> > >\n> > > The move won't help much, it will copy the v4l2_ext_control instance.\n> > > Should we resize() instead of reserve(), and operate directly on the\n> > > vector entry in this loop ?\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> > > > @@ -250,10 +246,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> > > updateControls should also take a Span argument, that could be done in\n> > > the same patch that updates the argument to getControls().\n> > >\n> > > Overall this patch is good, the comments above are small issues.\n> > >\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 EE8A2BDCA4\n\tfor ;\n\tTue, 27 Apr 2021 01:28:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5F33968883;\n\tTue, 27 Apr 2021 03:28:37 +0200 (CEST)","from mail-ed1-x52d.google.com (mail-ed1-x52d.google.com\n\t[IPv6:2a00:1450:4864:20::52d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 33848605BD\n\tfor ;\n\tTue, 27 Apr 2021 03:28:36 +0200 (CEST)","by mail-ed1-x52d.google.com with SMTP id y3so31713329eds.5\n\tfor ;\n\tMon, 26 Apr 2021 18:28:36 -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=\"SWVD3Ds9\"; 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=hsOst5vEedK5/nJYEMHOGtddbLJB5GbKUTeqb7xqHGY=;\n\tb=SWVD3Ds9D65Up4Nn5K471pw81inkIODFOSBXP+IHlLPrd5fNfNxGunaHHuo8lQWKS7\n\t9jlWWIDJ56HwAgspIMBJYin96hCNXKQgLQAKRfrNOowvar/yelPLi3Gte1+9qW2NPz3e\n\tfgJ233+FwMkNTVgaIQtkZ3T7dyEGM2xWUV8Vw=","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=hsOst5vEedK5/nJYEMHOGtddbLJB5GbKUTeqb7xqHGY=;\n\tb=jp/scNYoiWRJhZXQzeNd+KN+NadHuQV4goYJCarMtOuAQEAdJFqnYAkMp3KUWhZEc0\n\trGikyJSznGF2JzI/oBCRx5r1DQElc682yLrgQMtqss5ppEbK0u9SH0fm2Ut0cDrLohVV\n\ts0/VLCN/Ry6/ZwKSsIazWO3Jx5u9X6LQFNeSDPSBrWJLCQtQoSwhe2E8Lt/l8V9TBaYj\n\tTlbYJ6FoProSNdrSbiJTDLaHuupZ7vuAB6aC3faxUg9SHeP8av3yv9LEwuZc+hWasv2V\n\tUhEzCdA2rbIL7p9GrRRR/vhb3s+tHUv5vLu8W2sSgLkcgNRc0SLO3HZn+tR8Max6w4Sx\n\tOZOQ==","X-Gm-Message-State":"AOAM533S6cGHrbs8p+Bxfut0M7CnxcJErmxpoeqJbYaQq419ly7fyqMe\n\tHi2dWzpaFIqTDR3bSlMHdhXiUzKQ3MDdELWbqCLXCvrS1Ok=","X-Google-Smtp-Source":"ABdhPJwE1WIRxhVayQSVC6t4TfLVTQPZu9rix1KPOOWnclONJdU5wRgi15EdrB6T/blkZakQ+nih+rMf/pmj583AeP4=","X-Received":"by 2002:a05:6402:488:: with SMTP id\n\tk8mr1516635edv.233.1619486915811; \n\tMon, 26 Apr 2021 18:28:35 -0700 (PDT)","MIME-Version":"1.0","References":"<20210415044854.3348529-1-hiroh@chromium.org>\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 27 Apr 2021 10:28:26 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] 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\" "}}]