[{"id":16252,"web_url":"https://patchwork.libcamera.org/comment/16252/","msgid":"","date":"2021-04-13T21:58:35","subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: V4L2Device: Use\n\tstd::vector for v4l2_ext_control in getControls()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Hiro,\n\nOn 13/04/2021 07:19, 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 | 44 +++++++++++++++++------------------\n> 1 file changed, 21 insertions(+), 23 deletions(-)\n> \n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index decd19ef..78c289c2 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> @@ -196,21 +195,26 @@ 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\nThe v4l2ctrls array is created of size count which was ids.size();\nShouldn't we reserve this as ids.size() here instead of ctrls.size()?\n\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\nCan v4l2Ctrl initialisation be simplified to this?:\n\tv4l2_ext_control v4l2Ctrl = {};\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\tcase V4L2_CTRL_TYPE_U8: {\n> +\t\t\t\tControlType type;\n> \t\t\t\ttype = ControlTypeByte;\n> +\t\t\t\tControlValue &value = ctrl.second;\n> +\t\t\t\tvalue.reserve(type, true, info.elems);\n\nIs this the only usage of type? Couldn't we just do\n value.reserve(ControlTypeByte, true, info.elems);\n\nOr at the least make it\n\nControlType = ControlTypeByte;\n\nrather than use an extra line.\n\n\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> +\t\t\t}\n> \n> \t\t\tdefault:\n> \t\t\t\tLOG(V4L2, Error)\n> @@ -218,30 +222,23 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> \t\t\t\t\t<< info.type;\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\nThat switch statement must have been arranged like that to facilitate\neasily adding other types later.\n\nIf we don't need to do that, can we fix this up to lower the indentation\nsomehow? We're four levels in here in places :-S\n\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> \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 +247,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\nI was going to say can we pass v4l2Ctrls directly, but now I've seen\nyour later patch ;=)\n\n\n\n> \n> \treturn ctrls;\n> }\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 1E426BD1F6\n\tfor ;\n\tTue, 13 Apr 2021 21:58:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9D3E0687D6;\n\tTue, 13 Apr 2021 23:58:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 16C60602D1\n\tfor ;\n\tTue, 13 Apr 2021 23:58:39 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9D23A9F0;\n\tTue, 13 Apr 2021 23:58:38 +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=\"S5vn4c2i\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618351118;\n\tbh=phDCF9V4wJURMR87anDxdHmp09rxBh0R/upZGuNcM5U=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=S5vn4c2ifhfd4j27cap+hbIy6ZV1cJMlUNjJaiwaDRZLeRXsRtiT7GbNQIL6CL6Nb\n\tKYeegCtTnU5O+zA52tYQDYWbxiLmugscb54b4mdePcYQm4ymFP1BHLWKYLgMD1AOou\n\trIS2v+ubvVJXBtETVA3aZ4RxvkRaVLjdRWGyVd8k=","To":"Hirokazu Honda , libcamera-devel@lists.libcamera.org","References":"<20210413061925.3054927-1-hiroh@chromium.org>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Tue, 13 Apr 2021 22:58:35 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20210413061925.3054927-1-hiroh@chromium.org>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16257,"web_url":"https://patchwork.libcamera.org/comment/16257/","msgid":"","date":"2021-04-14T01:45:38","subject":"Re: [libcamera-devel] [PATCH 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 Kieran,\n\nOn Wed, Apr 14, 2021 at 6:58 AM Kieran Bingham\n wrote:\n>\n> Hi Hiro,\n>\n> On 13/04/2021 07:19, 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 | 44 +++++++++++++++++------------------\n> > 1 file changed, 21 insertions(+), 23 deletions(-)\n> >\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index decd19ef..78c289c2 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> > @@ -196,21 +195,26 @@ 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> The v4l2ctrls array is created of size count which was ids.size();\n> Shouldn't we reserve this as ids.size() here instead of ctrls.size()?\n>\n\nYeah, I am also confused by the point upon creating this patch.\nThe original implementation seems to contain a bug;\ncount = id.size() and v4l2Ctrls's size is count, but the iteration is\nup to ctrls.size() and thus v4l2Ctrls[ctrls.size() - 1] is set.\nI expect this is out-of-bounds bug.\nThis patch fixes it by setting v4l2Ctrls.size() to ctrls.size() and\nkeeping the original return semantics.\nHowever, I still don't get the original return semantics is correct.\nThe doc states \"This method reads the value of all controls contained\nin \\a ids, and returns their values as a ControlList.\"\nThe implementation looks to return all the control values regardless of ids.\n+Laurent Pinchart do you have any idea?\n\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> Can v4l2Ctrl initialisation be simplified to this?:\n> v4l2_ext_control v4l2Ctrl = {};\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> > + case V4L2_CTRL_TYPE_U8: {\n> > + ControlType type;\n> > type = ControlTypeByte;\n> > + ControlValue &value = ctrl.second;\n> > + value.reserve(type, true, info.elems);\n>\n> Is this the only usage of type? Couldn't we just do\n> value.reserve(ControlTypeByte, true, info.elems);\n>\n> Or at the least make it\n>\n> ControlType = ControlTypeByte;\n>\n> rather than use an extra line.\n>\n>\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> > @@ -218,30 +222,23 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > << info.type;\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> That switch statement must have been arranged like that to facilitate\n> easily adding other types later.\n>\n> If we don't need to do that, can we fix this up to lower the indentation\n> somehow? We're four levels in here in places :-S\n>\n>\n> > }\n> >\n> > - v4l2Ctrls[i].id = id;\n> > - i++;\n> > + v4l2Ctrl.id = id;\n> > + v4l2Ctrls.push_back(std::move(v4l2Ctrl));\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 +247,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> I was going to say can we pass v4l2Ctrls directly, but now I've seen\n> your later patch ;=)\n>\n>\n>\n> >\n> > return ctrls;\n> > }\n> >\n>\n> --\n> Regards\n> --\n> Kieran","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 7199FBD224\n\tfor ;\n\tWed, 14 Apr 2021 01:45:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C8DF7687EC;\n\tWed, 14 Apr 2021 03:45:50 +0200 (CEST)","from mail-ej1-x632.google.com (mail-ej1-x632.google.com\n\t[IPv6:2a00:1450:4864:20::632])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6FC49602CD\n\tfor ;\n\tWed, 14 Apr 2021 03:45:49 +0200 (CEST)","by mail-ej1-x632.google.com with SMTP id w3so28941744ejc.4\n\tfor ;\n\tTue, 13 Apr 2021 18:45:49 -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=\"Qdi0hTXF\"; 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=jY8cbDN2NkYuE4TnqqDyL5Volv9ZG8EMgovaLiy0mXw=;\n\tb=Qdi0hTXFH4OKQNm1gWVCm7N5go99k5f4YRW/ic2mdXDJLk+8WZ0pBnpLEktQDsGfIs\n\trxIHlCQZ4pyNuVCMyBWLQ4NDz4OJMvsUCm3PUEWh50DL/irEP3cbdZrf1vYBJNtEcPrB\n\tAdhqFspAnqoWEjkx2acHapkNEA9hjopDpjs/g=","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=jY8cbDN2NkYuE4TnqqDyL5Volv9ZG8EMgovaLiy0mXw=;\n\tb=pmJ6wPUQIyFaHKebDnzTSvdo50VqRa/RzhDw0m9mFpVQUk2mUuHbg8HfkSlhy4Z2oA\n\tAb7VBqr92c3CeacLUXNCV0FBDzd28KDm+eGCGOoNImMjRGR6iz4SDInOo9ffkjdSW7bG\n\t8KBqiFlql70kHP3uCBK0LrV6mgXRHzLr08NrJzECV4ItAEiXg3vhM2mmXmRx9+KwUWSe\n\tvTLevqKceQLEmuu9KtXWqv7NRGMkaaZ+PNk3TnAP2yZyoUna6X4qgo8mChytyJW7Dkj7\n\tFopgzEWkEMKEa6+TQwBdbqmbpqDEHD2vtYwW3wNJPJU+MMkiRALpCw5JFipvRVrWW0vI\n\t3/eg==","X-Gm-Message-State":"AOAM531gOYFE4RLQfBG5k7KylaBdzLk0ertAMfqynHtzNZu9IS6+HJbe\n\tYPW5T05KRdOkB6lb24Ml5sfh44f6AQKVPuBn805mLdIEA4H7Xg==","X-Google-Smtp-Source":"ABdhPJzODIyFHK0rub/CNwKREfTeFCmnZXMo+e6Sb6qCe+dvhTmtqdQMvXlG+zfAQ6ZskH1XSYT9gnWSmN6aFmh1g4M=","X-Received":"by 2002:a17:907:1b06:: with SMTP id\n\tmp6mr35661553ejc.292.1618364749027; \n\tTue, 13 Apr 2021 18:45:49 -0700 (PDT)","MIME-Version":"1.0","References":"<20210413061925.3054927-1-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 14 Apr 2021 10:45:38 +0900","Message-ID":"","To":"Kieran Bingham , \n\tLaurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH 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":16264,"web_url":"https://patchwork.libcamera.org/comment/16264/","msgid":"<5975558f-6ca3-eef1-573f-8d6b6da5da64@ideasonboard.com>","date":"2021-04-14T08:17:04","subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: V4L2Device: Use\n\tstd::vector for v4l2_ext_control in getControls()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Hiro,\n\nOn 14/04/2021 02:45, Hirokazu Honda wrote:\n> Hi Kieran,\n> \n> On Wed, Apr 14, 2021 at 6:58 AM Kieran Bingham\n> wrote:\n>>\n>> Hi Hiro,\n>>\n>> On 13/04/2021 07:19, 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 | 44 +++++++++++++++++------------------\n>>> 1 file changed, 21 insertions(+), 23 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>>> index decd19ef..78c289c2 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>>> @@ -196,21 +195,26 @@ 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>> The v4l2ctrls array is created of size count which was ids.size();\n>> Shouldn't we reserve this as ids.size() here instead of ctrls.size()?\n>>\n> \n> Yeah, I am also confused by the point upon creating this patch.\n> The original implementation seems to contain a bug;\n> count = id.size() and v4l2Ctrls's size is count, but the iteration is\n> up to ctrls.size() and thus v4l2Ctrls[ctrls.size() - 1] is set.\n> I expect this is out-of-bounds bug.\n> This patch fixes it by setting v4l2Ctrls.size() to ctrls.size() and\n> keeping the original return semantics.\n> However, I still don't get the original return semantics is correct.\n> The doc states \"This method reads the value of all controls contained\n> in \\a ids, and returns their values as a ControlList.\"\n> The implementation looks to return all the control values regardless of ids.\n> +Laurent Pinchart do you have any idea?\n\nI think this is where it's important to keep distinct changes in\nseparate patches.\n\nTry to make this patch perform only the conversion to the vector, with\nlike-for-like (bugz inclusive) functionality, and then it's easier to\ndiscuss and fix the bug if it is there directly in it's own patch.\n\n> \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>> Can v4l2Ctrl initialisation be simplified to this?:\n>> v4l2_ext_control v4l2Ctrl = {};\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>>> + case V4L2_CTRL_TYPE_U8: {\n>>> + ControlType type;\n>>> type = ControlTypeByte;\n>>> + ControlValue &value = ctrl.second;\n>>> + value.reserve(type, true, info.elems);\n>>\n>> Is this the only usage of type? Couldn't we just do\n>> value.reserve(ControlTypeByte, true, info.elems);\n>>\n>> Or at the least make it\n>>\n>> ControlType = ControlTypeByte;\n>>\n>> rather than use an extra line.\n>>\n>>\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>>> @@ -218,30 +222,23 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n>>> << info.type;\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>> That switch statement must have been arranged like that to facilitate\n>> easily adding other types later.\n>>\n>> If we don't need to do that, can we fix this up to lower the indentation\n>> somehow? We're four levels in here in places :-S\n>>\n>>\n>>> }\n>>>\n>>> - v4l2Ctrls[i].id = id;\n>>> - i++;\n>>> + v4l2Ctrl.id = id;\n>>> + v4l2Ctrls.push_back(std::move(v4l2Ctrl));\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 +247,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>> I was going to say can we pass v4l2Ctrls directly, but now I've seen\n>> your later patch ;=)\n>>\n>>\n>>\n>>>\n>>> return ctrls;\n>>> }\n>>>\n>>\n>> --\n>> Regards\n>> --\n>> Kieran","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 545A8BD1F6\n\tfor ;\n\tWed, 14 Apr 2021 08:17:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A367468809;\n\tWed, 14 Apr 2021 10:17:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 50963687EF\n\tfor ;\n\tWed, 14 Apr 2021 10:17:08 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E365A6F2;\n\tWed, 14 Apr 2021 10:17:07 +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=\"maa2iGcW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618388228;\n\tbh=EiDCox8T0UQXbxMnhg6P7K4kBydu/hkSz9jy3joe32I=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=maa2iGcWaTNlMEm0E2/+dxo9wul4y+isB7Ncu9FEnpakYESgK8PC1SGDsyNyYKFGu\n\tfqcpb/QCVaI9tG1Lfnqpt1kbZXVlAIOvwS2iGqGsG/ms9aisekCa2QK43E2M2+fVz5\n\tLHDfrIBQGL6GJg45wrKG2Tppq6UXdBzYSJZ7S2Zc=","To":"Hirokazu Honda ,\n\tLaurent Pinchart ","References":"<20210413061925.3054927-1-hiroh@chromium.org>\n\t\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<5975558f-6ca3-eef1-573f-8d6b6da5da64@ideasonboard.com>","Date":"Wed, 14 Apr 2021 09:17:04 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 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","Reply-To":"kieran.bingham@ideasonboard.com","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":16288,"web_url":"https://patchwork.libcamera.org/comment/16288/","msgid":"","date":"2021-04-15T03:10:59","subject":"Re: [libcamera-devel] [PATCH 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 Wed, Apr 14, 2021 at 5:17 PM Kieran Bingham\n wrote:\n>\n> Hi Hiro,\n>\n> On 14/04/2021 02:45, Hirokazu Honda wrote:\n> > Hi Kieran,\n> >\n> > On Wed, Apr 14, 2021 at 6:58 AM Kieran Bingham\n> > wrote:\n> >>\n> >> Hi Hiro,\n> >>\n> >> On 13/04/2021 07:19, 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 | 44 +++++++++++++++++------------------\n> >>> 1 file changed, 21 insertions(+), 23 deletions(-)\n> >>>\n> >>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> >>> index decd19ef..78c289c2 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> >>> @@ -196,21 +195,26 @@ 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> >> The v4l2ctrls array is created of size count which was ids.size();\n> >> Shouldn't we reserve this as ids.size() here instead of ctrls.size()?\n> >>\n> >\n> > Yeah, I am also confused by the point upon creating this patch.\n> > The original implementation seems to contain a bug;\n> > count = id.size() and v4l2Ctrls's size is count, but the iteration is\n> > up to ctrls.size() and thus v4l2Ctrls[ctrls.size() - 1] is set.\n> > I expect this is out-of-bounds bug.\n> > This patch fixes it by setting v4l2Ctrls.size() to ctrls.size() and\n> > keeping the original return semantics.\n> > However, I still don't get the original return semantics is correct.\n> > The doc states \"This method reads the value of all controls contained\n> > in \\a ids, and returns their values as a ControlList.\"\n> > The implementation looks to return all the control values regardless of ids.\n> > +Laurent Pinchart do you have any idea?\n>\n> I think this is where it's important to keep distinct changes in\n> separate patches.\n>\n> Try to make this patch perform only the conversion to the vector, with\n> like-for-like (bugz inclusive) functionality, and then it's easier to\n> discuss and fix the bug if it is there directly in it's own patch.\n>\n\nI got it. The bug fix is much simpler than I thought.\nI submitted as an independent patch and will rebase this patch series\non top of it.\n\nThanks,\n-Hiro\n> >\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> >> Can v4l2Ctrl initialisation be simplified to this?:\n> >> v4l2_ext_control v4l2Ctrl = {};\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> >>> + case V4L2_CTRL_TYPE_U8: {\n> >>> + ControlType type;\n> >>> type = ControlTypeByte;\n> >>> + ControlValue &value = ctrl.second;\n> >>> + value.reserve(type, true, info.elems);\n> >>\n> >> Is this the only usage of type? Couldn't we just do\n> >> value.reserve(ControlTypeByte, true, info.elems);\n> >>\n> >> Or at the least make it\n> >>\n> >> ControlType = ControlTypeByte;\n> >>\n> >> rather than use an extra line.\n> >>\n> >>\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> >>> @@ -218,30 +222,23 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> >>> << info.type;\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> >> That switch statement must have been arranged like that to facilitate\n> >> easily adding other types later.\n> >>\n> >> If we don't need to do that, can we fix this up to lower the indentation\n> >> somehow? We're four levels in here in places :-S\n> >>\n> >>\n> >>> }\n> >>>\n> >>> - v4l2Ctrls[i].id = id;\n> >>> - i++;\n> >>> + v4l2Ctrl.id = id;\n> >>> + v4l2Ctrls.push_back(std::move(v4l2Ctrl));\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 +247,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> >> I was going to say can we pass v4l2Ctrls directly, but now I've seen\n> >> your later patch ;=)\n> >>\n> >>\n> >>\n> >>>\n> >>> return ctrls;\n> >>> }\n> >>>\n> >>\n> >> --\n> >> Regards\n> >> --\n> >> Kieran\n>\n> --\n> Regards\n> --\n> Kieran","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 BE1CFBD224\n\tfor ;\n\tThu, 15 Apr 2021 03:11:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2474468812;\n\tThu, 15 Apr 2021 05:11:12 +0200 (CEST)","from mail-ej1-x62d.google.com (mail-ej1-x62d.google.com\n\t[IPv6:2a00:1450:4864:20::62d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 51000602C7\n\tfor ;\n\tThu, 15 Apr 2021 05:11:10 +0200 (CEST)","by mail-ej1-x62d.google.com with SMTP id e14so34531459ejz.11\n\tfor ;\n\tWed, 14 Apr 2021 20:11:10 -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=\"O7XY3rXn\"; 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=8ECTe5tlfnxNUUOnwf8m/w0z8726m1ojFxkoh4dYZsk=;\n\tb=O7XY3rXnbB+89by3k49Spi5pt5cpJOZHzs5DV+U20HVoIxGdj9cJsDAvJF3BqkNFtS\n\twnxUs5qZa2vZTA4if690svib3bU749Evg+XmRbv7/TB+bBOw2cCTTEwjuKhUN6KYHcKN\n\tU4WzQNYI+gtOIzNOEOISUPB9OxCsVZJwjOppE=","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=8ECTe5tlfnxNUUOnwf8m/w0z8726m1ojFxkoh4dYZsk=;\n\tb=FFXxlsbeRL4tHS1DFs+nj39SZrEkfJwlasI/KzLUWpafLmks0LS3MmIck71bp7dIDF\n\txpYKdM1L6PHU+PCshI4qmVREvCV777vBd3UNksFOQW4TA7uKWV5QcmCGUiOwZISrnLrq\n\tjWXNigqY9rHnZ+R1WmPwLEvokOTx2LbAtIyWbEYN82oSx80ytHKGnlT67Em1M+LMcmm/\n\t/2fENfwJEht6MWuAipd2Qlwhj3cI47EpUYFABIPws7kK9Vnb6JyX5Yqb1Ikg8mJGbjit\n\ty/I5iLiTfpFO8kacsYvM5+MJEImEcPCo8siLulVO21SEc8jly1pZpjQ6XuCh3TwIPmqz\n\tDgvA==","X-Gm-Message-State":"AOAM532Gd/L73XTHdPrW4bG8yiX2X0ieX+bgAVU8RJN+FcCuJPkBMIto\n\tbdqOZTaLeGMyFZIn/bRWZyCpDLe5NtldwdrPRXfJkg==","X-Google-Smtp-Source":"ABdhPJwQ1laOBEXnkiFe0Ei6lmvdBTX5bWxZSKy/b7yTBevRRL3v17j7G4RSDOZ+1J1eVgTAk4E5/HltQl1+V3Cji04=","X-Received":"by 2002:a17:906:b890:: with SMTP id\n\thb16mr1160458ejb.221.1618456269884; \n\tWed, 14 Apr 2021 20:11:09 -0700 (PDT)","MIME-Version":"1.0","References":"<20210413061925.3054927-1-hiroh@chromium.org>\n\t\n\t\n\t<5975558f-6ca3-eef1-573f-8d6b6da5da64@ideasonboard.com>","In-Reply-To":"<5975558f-6ca3-eef1-573f-8d6b6da5da64@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Thu, 15 Apr 2021 12:10:59 +0900","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH 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":16289,"web_url":"https://patchwork.libcamera.org/comment/16289/","msgid":"","date":"2021-04-15T03:42:03","subject":"Re: [libcamera-devel] [PATCH 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 Thu, Apr 15, 2021 at 12:10 PM Hirokazu Honda wrote:\n>\n> On Wed, Apr 14, 2021 at 5:17 PM Kieran Bingham\n> wrote:\n> >\n> > Hi Hiro,\n> >\n> > On 14/04/2021 02:45, Hirokazu Honda wrote:\n> > > Hi Kieran,\n> > >\n> > > On Wed, Apr 14, 2021 at 6:58 AM Kieran Bingham\n> > > wrote:\n> > >>\n> > >> Hi Hiro,\n> > >>\n> > >> On 13/04/2021 07:19, 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 | 44 +++++++++++++++++------------------\n> > >>> 1 file changed, 21 insertions(+), 23 deletions(-)\n> > >>>\n> > >>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > >>> index decd19ef..78c289c2 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> > >>> @@ -196,21 +195,26 @@ 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> > >> The v4l2ctrls array is created of size count which was ids.size();\n> > >> Shouldn't we reserve this as ids.size() here instead of ctrls.size()?\n> > >>\n> > >\n> > > Yeah, I am also confused by the point upon creating this patch.\n> > > The original implementation seems to contain a bug;\n> > > count = id.size() and v4l2Ctrls's size is count, but the iteration is\n> > > up to ctrls.size() and thus v4l2Ctrls[ctrls.size() - 1] is set.\n> > > I expect this is out-of-bounds bug.\n> > > This patch fixes it by setting v4l2Ctrls.size() to ctrls.size() and\n> > > keeping the original return semantics.\n> > > However, I still don't get the original return semantics is correct.\n> > > The doc states \"This method reads the value of all controls contained\n> > > in \\a ids, and returns their values as a ControlList.\"\n> > > The implementation looks to return all the control values regardless of ids.\n> > > +Laurent Pinchart do you have any idea?\n> >\n> > I think this is where it's important to keep distinct changes in\n> > separate patches.\n> >\n> > Try to make this patch perform only the conversion to the vector, with\n> > like-for-like (bugz inclusive) functionality, and then it's easier to\n> > discuss and fix the bug if it is there directly in it's own patch.\n> >\n>\n> I got it. The bug fix is much simpler than I thought.\n> I submitted as an independent patch and will rebase this patch series\n> on top of it.\n>\n> Thanks,\n> -Hiro\n> > >\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> > >> Can v4l2Ctrl initialisation be simplified to this?:\n> > >> v4l2_ext_control v4l2Ctrl = {};\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> > >>> + case V4L2_CTRL_TYPE_U8: {\n> > >>> + ControlType type;\n> > >>> type = ControlTypeByte;\n> > >>> + ControlValue &value = ctrl.second;\n> > >>> + value.reserve(type, true, info.elems);\n> > >>\n> > >> Is this the only usage of type? Couldn't we just do\n> > >> value.reserve(ControlTypeByte, true, info.elems);\n> > >>\n> > >> Or at the least make it\n> > >>\n> > >> ControlType = ControlTypeByte;\n> > >>\n> > >> rather than use an extra line.\n> > >>\n> > >>\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> > >>> @@ -218,30 +222,23 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > >>> << info.type;\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> > >> That switch statement must have been arranged like that to facilitate\n> > >> easily adding other types later.\n> > >>\n> > >> If we don't need to do that, can we fix this up to lower the indentation\n> > >> somehow? We're four levels in here in places :-S\n> > >>\n\nSorry, I don't get your suggestion. Can you elaborate?\n\n> > >>\n> > >>> }\n> > >>>\n> > >>> - v4l2Ctrls[i].id = id;\n> > >>> - i++;\n> > >>> + v4l2Ctrl.id = id;\n> > >>> + v4l2Ctrls.push_back(std::move(v4l2Ctrl));\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 +247,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> > >> I was going to say can we pass v4l2Ctrls directly, but now I've seen\n> > >> your later patch ;=)\n> > >>\n> > >>\n> > >>\n> > >>>\n> > >>> return ctrls;\n> > >>> }\n> > >>>\n> > >>\n> > >> --\n> > >> Regards\n> > >> --\n> > >> Kieran\n> >\n> > --\n> > Regards\n> > --\n> > Kieran","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 87F07BD1F6\n\tfor ;\n\tThu, 15 Apr 2021 03:42:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 053AF68812;\n\tThu, 15 Apr 2021 05:42:16 +0200 (CEST)","from mail-ed1-x52b.google.com (mail-ed1-x52b.google.com\n\t[IPv6:2a00:1450:4864:20::52b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 20660602C7\n\tfor ;\n\tThu, 15 Apr 2021 05:42:15 +0200 (CEST)","by mail-ed1-x52b.google.com with SMTP id s15so26268060edd.4\n\tfor ;\n\tWed, 14 Apr 2021 20:42:15 -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=\"bdX62Pul\"; 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=GB6jeHAdsk6PiniXPfY+JMqsojPZ4LhFfYlcnimNqVs=;\n\tb=bdX62PulbWka4j6wgV5uTzWGwOyuwmH+Bd7nB9pNo9crfQNQ4aW6kVYcBYkP7juysa\n\t88vx/tIh09KJi2IjYjY8tmuTGXoK2JUyGDeclnAhd5cNdpIF8ErD2VWD6OV4cX0wYgF2\n\tc3KT1/0cKZmrPzdj//uQW6M7G0REMKDv+rPkg=","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=GB6jeHAdsk6PiniXPfY+JMqsojPZ4LhFfYlcnimNqVs=;\n\tb=KfRopcCgZ+uyqnJEF5dgbC9UxdECqLqQ2Ux1R25E+AHJj6EiYG18HeomE/dfz2hkbC\n\ttoI2uICQ0e+m01I0XBdIreKGCAiFrMZy4MISvMn/wjuqaqswG51HJdTOhxt3HyiB+HW9\n\t3Y2QJnQrlZg9Q4HZSwj6BIb6dhC+VOtcWZJ42RxYvUO5vZP8NUr25t52coH61mXTSBoA\n\tyGiRoxMdue1Av2rv8ZOHEKIqO2m6+r+d7AJ87jsWsQYOr+klwOiPSgEA56pPSArAlO2J\n\to04e6mv0Z6wXiYo/2JNuGUB8JZgcuUqOdNUIPsBAH/NqhcdBoDPlh6X2ZigNBPffgP51\n\t9/Ig==","X-Gm-Message-State":"AOAM531gs9FoPlXPfVWf02ynyzhrPHbAUi21gSKzU0JMz1U2DXKgI5lp\n\tvNzINdWRgRHrdCRAuF8bCuzc+usqLL/80pcceC6ro/e0nPo=","X-Google-Smtp-Source":"ABdhPJy7JO5hU2WeJeSqJ2MbEISg/MSy9PIMWMMHuA9zcSAMIgSCJ5/M6epY4y81Mb1LQcxj/XrzE6uUDvcjYTFxYmc=","X-Received":"by 2002:a05:6402:3493:: with SMTP id\n\tv19mr1603022edc.355.1618458134748; \n\tWed, 14 Apr 2021 20:42:14 -0700 (PDT)","MIME-Version":"1.0","References":"<20210413061925.3054927-1-hiroh@chromium.org>\n\t\n\t\n\t<5975558f-6ca3-eef1-573f-8d6b6da5da64@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Thu, 15 Apr 2021 12:42:03 +0900","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH 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":16290,"web_url":"https://patchwork.libcamera.org/comment/16290/","msgid":"","date":"2021-04-15T03:49:04","subject":"Re: [libcamera-devel] [PATCH 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 Thu, Apr 15, 2021 at 12:42 PM Hirokazu Honda wrote:\n>\n> On Thu, Apr 15, 2021 at 12:10 PM Hirokazu Honda wrote:\n> >\n> > On Wed, Apr 14, 2021 at 5:17 PM Kieran Bingham\n> > wrote:\n> > >\n> > > Hi Hiro,\n> > >\n> > > On 14/04/2021 02:45, Hirokazu Honda wrote:\n> > > > Hi Kieran,\n> > > >\n> > > > On Wed, Apr 14, 2021 at 6:58 AM Kieran Bingham\n> > > > wrote:\n> > > >>\n> > > >> Hi Hiro,\n> > > >>\n> > > >> On 13/04/2021 07:19, 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 | 44 +++++++++++++++++------------------\n> > > >>> 1 file changed, 21 insertions(+), 23 deletions(-)\n> > > >>>\n> > > >>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > >>> index decd19ef..78c289c2 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> > > >>> @@ -196,21 +195,26 @@ 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> > > >> The v4l2ctrls array is created of size count which was ids.size();\n> > > >> Shouldn't we reserve this as ids.size() here instead of ctrls.size()?\n> > > >>\n> > > >\n> > > > Yeah, I am also confused by the point upon creating this patch.\n> > > > The original implementation seems to contain a bug;\n> > > > count = id.size() and v4l2Ctrls's size is count, but the iteration is\n> > > > up to ctrls.size() and thus v4l2Ctrls[ctrls.size() - 1] is set.\n> > > > I expect this is out-of-bounds bug.\n> > > > This patch fixes it by setting v4l2Ctrls.size() to ctrls.size() and\n> > > > keeping the original return semantics.\n> > > > However, I still don't get the original return semantics is correct.\n> > > > The doc states \"This method reads the value of all controls contained\n> > > > in \\a ids, and returns their values as a ControlList.\"\n> > > > The implementation looks to return all the control values regardless of ids.\n> > > > +Laurent Pinchart do you have any idea?\n> > >\n> > > I think this is where it's important to keep distinct changes in\n> > > separate patches.\n> > >\n> > > Try to make this patch perform only the conversion to the vector, with\n> > > like-for-like (bugz inclusive) functionality, and then it's easier to\n> > > discuss and fix the bug if it is there directly in it's own patch.\n> > >\n> >\n> > I got it. The bug fix is much simpler than I thought.\n> > I submitted as an independent patch and will rebase this patch series\n> > on top of it.\n> >\n> > Thanks,\n> > -Hiro\n> > > >\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> > > >> Can v4l2Ctrl initialisation be simplified to this?:\n> > > >> v4l2_ext_control v4l2Ctrl = {};\n> > > >>\n\nRegarding this, we should not trust {} for zero initialization.\nThe problem happens c structure has union.\nhttps://en.cppreference.com/w/cpp/language/zero_initialization\n> If T is a union type, the first non-static named data member is zero-initialized and all padding is initialized to zero bits.\nThis means, some union value might not be zero if the first non static\nmember in union is smaller than others.\nSo I would basically do memset to v4l2 structure, which often has union.\n\n-Hiro\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> > > >>> + case V4L2_CTRL_TYPE_U8: {\n> > > >>> + ControlType type;\n> > > >>> type = ControlTypeByte;\n> > > >>> + ControlValue &value = ctrl.second;\n> > > >>> + value.reserve(type, true, info.elems);\n> > > >>\n> > > >> Is this the only usage of type? Couldn't we just do\n> > > >> value.reserve(ControlTypeByte, true, info.elems);\n> > > >>\n> > > >> Or at the least make it\n> > > >>\n> > > >> ControlType = ControlTypeByte;\n> > > >>\n> > > >> rather than use an extra line.\n> > > >>\n> > > >>\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> > > >>> @@ -218,30 +222,23 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > > >>> << info.type;\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> > > >> That switch statement must have been arranged like that to facilitate\n> > > >> easily adding other types later.\n> > > >>\n> > > >> If we don't need to do that, can we fix this up to lower the indentation\n> > > >> somehow? We're four levels in here in places :-S\n> > > >>\n>\n> Sorry, I don't get your suggestion. Can you elaborate?\n>\n> > > >>\n> > > >>> }\n> > > >>>\n> > > >>> - v4l2Ctrls[i].id = id;\n> > > >>> - i++;\n> > > >>> + v4l2Ctrl.id = id;\n> > > >>> + v4l2Ctrls.push_back(std::move(v4l2Ctrl));\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 +247,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> > > >> I was going to say can we pass v4l2Ctrls directly, but now I've seen\n> > > >> your later patch ;=)\n> > > >>\n> > > >>\n> > > >>\n> > > >>>\n> > > >>> return ctrls;\n> > > >>> }\n> > > >>>\n> > > >>\n> > > >> --\n> > > >> Regards\n> > > >> --\n> > > >> Kieran\n> > >\n> > > --\n> > > Regards\n> > > --\n> > > Kieran","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 716AEBD224\n\tfor ;\n\tThu, 15 Apr 2021 03:49:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DD02968812;\n\tThu, 15 Apr 2021 05:49:16 +0200 (CEST)","from mail-ej1-x62c.google.com (mail-ej1-x62c.google.com\n\t[IPv6:2a00:1450:4864:20::62c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8F3E0602C7\n\tfor ;\n\tThu, 15 Apr 2021 05:49:15 +0200 (CEST)","by mail-ej1-x62c.google.com with SMTP id sd23so26007207ejb.12\n\tfor ;\n\tWed, 14 Apr 2021 20:49:15 -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=\"jWerNUwA\"; 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=0e33FJm58OLpx6Kn0dbQxDWh+LZO4KKLlF2lP/bSLAQ=;\n\tb=jWerNUwARme4zAV81UJkBSlB1agzOh7m3e2dL8k2+JQlGCeBC3fNbO54817G+bCniI\n\tDBbMUXeyqtqhAQbB0B9Bg+iwoHEIHLPRS2jQrM0u9Xz/rCdaAxmz6mhNCV2D8/ilmYm/\n\txEF3SbIrrILEgSTNa0DjKhi8KKr7ggNeFfetE=","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=0e33FJm58OLpx6Kn0dbQxDWh+LZO4KKLlF2lP/bSLAQ=;\n\tb=bHeXGsfifdzhM3JnZtu2e1akAjj5lAOhUaCoEneXxpn3Mk2oJ76/ZrhRpFMOTg1OyO\n\tJDLFXf049V8CMz34cvAmHg55rJRsVsA8dFXTA//1idhomL8PRXankgk+rxWHYtr97P/Z\n\t/ekhfLy03fKCrgofgwXy30jW+/6x9lC3QXppaqkAnBy8njuiZsKZuYlxDPrrFeVBk0jL\n\tsY7JoXl+BtGSRrBrKBJbszTRO3N0zBMt7WfF+5S5c1QxoEKnT8zTut/DALTuJ79NNO/j\n\t2J6Vvo/O8fjhHpHz8dTtT5rpS1T1uSrJS6rnEvaPKeIExAUwMv2/su9XjfYkeFOzf/+J\n\t30Mw==","X-Gm-Message-State":"AOAM530hSumta/4a5Jxj2hAeLTX+g4agIASmRycENwrh5+3wulmzvLzx\n\tsmDHmJgrgcsjuaTZk33f8DCzVGOsO5FY18hMlwpeNFydrfz7rg==","X-Google-Smtp-Source":"ABdhPJy3tWpO4xcKOHTu5pVrcZpExE5K7PxzWbddn94V4LLqqD9dlQMJ3ptUvUp83x+39ljvfNKFM9N+Fxe3Rq77+44=","X-Received":"by 2002:a17:906:46d6:: with SMTP id\n\tk22mr1227419ejs.243.1618458555157; \n\tWed, 14 Apr 2021 20:49:15 -0700 (PDT)","MIME-Version":"1.0","References":"<20210413061925.3054927-1-hiroh@chromium.org>\n\t\n\t\n\t<5975558f-6ca3-eef1-573f-8d6b6da5da64@ideasonboard.com>\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Thu, 15 Apr 2021 12:49:04 +0900","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH 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\" "}}]