[{"id":16594,"web_url":"https://patchwork.libcamera.org/comment/16594/","msgid":"","date":"2021-04-27T01:23:44","subject":"Re: [libcamera-devel] [PATCH v6 1/3] libcamera: V4L2Device: Replace\n\tVLA with std::vector in getControls()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thanks for the patch.\nAfter all, I am fine with this.\nWe may want to improve the code a bit by using utils::enumerate(). I\nwill review your patch series today.\n\nOn Mon, Apr 26, 2021 at 9:12 AM Laurent Pinchart\n wrote:\n>\n> From: Hirokazu Honda \n>\n> The original code uses Variable-Length-Array, which is not officially\n> supported in C++. This replaces the array with std::vector.\n>\n> Signed-off-by: Hirokazu Honda \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/v4l2_device.cpp | 28 ++++++++++++++--------------\n> 1 file changed, 14 insertions(+), 14 deletions(-)\n>\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index decd19ef5281..3a4df4f6c098 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,14 +195,17 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> ctrls.set(id, {});\n> }\n>\n> - struct v4l2_ext_control v4l2Ctrls[count];\n> - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));\n> + std::vector v4l2Ctrls(ids.size());\n\nI missed this upon reviewing. v4l2Ctrls(ctrls.size()) is better.\n\n-Hiro\n\n> + memset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size());\n>\n> unsigned int i = 0;\n> for (auto &ctrl : ctrls) {\n> unsigned int id = ctrl.first;\n> const struct v4l2_query_ext_ctrl &info = controlInfo_[id];\n>\n> + v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i++];\n> + v4l2Ctrl.id = id;\n> +\n> if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {\n> ControlType type;\n>\n> @@ -223,25 +225,22 @@ ControlList V4L2Device::getControls(const std::vector &ids)\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> + v4l2Ctrl.p_u8 = data.data();\n> + v4l2Ctrl.size = data.size();\n> }\n> -\n> - v4l2Ctrls[i].id = id;\n> - i++;\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 +249,11 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> /* A specific control failed. */\n> LOG(V4L2, Error) << \"Unable to read control \" << errorIdx\n> << \": \" << strerror(-ret);\n> - count = errorIdx - 1;\n> +\n> + v4l2Ctrls.resize(errorIdx);\n> }\n>\n> - updateControls(&ctrls, v4l2Ctrls, count);\n> + updateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());\n>\n> return ctrls;\n> }\n> --\n> Regards,\n>\n> Laurent Pinchart\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 547E2BDCA1\n\tfor ;\n\tTue, 27 Apr 2021 01:23:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BF040605BD;\n\tTue, 27 Apr 2021 03:23:55 +0200 (CEST)","from mail-ej1-x62b.google.com (mail-ej1-x62b.google.com\n\t[IPv6:2a00:1450:4864:20::62b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 94FCE605BD\n\tfor ;\n\tTue, 27 Apr 2021 03:23:54 +0200 (CEST)","by mail-ej1-x62b.google.com with SMTP id u17so87288339ejk.2\n\tfor ;\n\tMon, 26 Apr 2021 18:23:54 -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=\"CRUajXjd\"; 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=h3dmMFrwbrUN3gF+4N8bCRCT/fBgxUy3gteg7zd71a0=;\n\tb=CRUajXjd+ao9ECd5SdBQy0NcfON+VOMSaHK55ScmmUsLUrsNND2y3TcJpJq5T5ZETe\n\toetjhebdXgWmLdY0Y30tPdSjaNZIpvSI0qgXGK1ZuGLETXrVCDUTi0XM9C4BT8n7nKi0\n\t06w9zFhrHqFY4RojfOYwvR1mC2Z+wXyyE6P74=","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=h3dmMFrwbrUN3gF+4N8bCRCT/fBgxUy3gteg7zd71a0=;\n\tb=O0p4CcssgVNIY4czZVHU1//3swgbp6SysgOoaD+j7+Ms1MC7aibGDJVTdsnKj1LnO0\n\tLsARH4ga+/UdQyvONSEGHLDETd/Cn/k4C0inyLNawKXNp6jCs7GmNtnvrRc9p492lHfi\n\tX4FG7WdR5PyOsmytARC6VaibRnmah34sztKTrUeCCWUPrmSyXI/j2afLY1ZTVZnjS914\n\tBhJ4RYYWp+ozUlPi/4nuUj+aQAcIJ5BRyZrNTia2YlWNiiGf/LMQZiw3wqVkvwoMOqjc\n\tfM8fLOq2igBQR1UMdBXnn6/szt12FQxDN9c3fQQeWoAseosqsFstMAdvfLMTbBOCetYZ\n\tYehg==","X-Gm-Message-State":"AOAM532ufPuOFA0EoTK3ZZpzstTeR9ggWNyKSNwOYjsFXcvMl42fiBcV\n\t7X06iCfryCwsiwNsywd9t/HKLGBrWraQG+8+ZUWmy55ZBJ8=","X-Google-Smtp-Source":"ABdhPJxnyhvyyHQsaadlQqeIjLhBqHdqyLpDjy2vcznvij5eQp7NYy0S+GWYYUXw/d0lesVHtkT004UXONYygdVZ9BQ=","X-Received":"by 2002:a17:906:55c5:: with SMTP id\n\tz5mr5917073ejp.306.1619486634251; \n\tMon, 26 Apr 2021 18:23:54 -0700 (PDT)","MIME-Version":"1.0","References":"<20210426001220.15599-1-laurent.pinchart@ideasonboard.com>\n\t<20210426001220.15599-2-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20210426001220.15599-2-laurent.pinchart@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Tue, 27 Apr 2021 10:23:44 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v6 1/3] libcamera: V4L2Device: Replace\n\tVLA with std::vector 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":16835,"web_url":"https://patchwork.libcamera.org/comment/16835/","msgid":"","date":"2021-05-07T22:04:15","subject":"Re: [libcamera-devel] [PATCH v6 1/3] libcamera: V4L2Device: Replace\n\tVLA with std::vector 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 Tue, Apr 27, 2021 at 10:23:44AM +0900, Hirokazu Honda wrote:\n> Hi Laurent, thanks for the patch.\n> After all, I am fine with this.\n> We may want to improve the code a bit by using utils::enumerate(). I\n> will review your patch series today.\n> \n> On Mon, Apr 26, 2021 at 9:12 AM Laurent Pinchart wrote:\n> >\n> > From: Hirokazu Honda \n> >\n> > The original code uses Variable-Length-Array, which is not officially\n> > supported in C++. This replaces the array with std::vector.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/v4l2_device.cpp | 28 ++++++++++++++--------------\n> > 1 file changed, 14 insertions(+), 14 deletions(-)\n> >\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index decd19ef5281..3a4df4f6c098 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,14 +195,17 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > ctrls.set(id, {});\n> > }\n> >\n> > - struct v4l2_ext_control v4l2Ctrls[count];\n> > - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));\n> > + std::vector v4l2Ctrls(ids.size());\n> \n> I missed this upon reviewing. v4l2Ctrls(ctrls.size()) is better.\n\nI've also noticed that after merging the patch :-( I'll send a fix.\n\n> > + memset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size());\n> >\n> > unsigned int i = 0;\n> > for (auto &ctrl : ctrls) {\n> > unsigned int id = ctrl.first;\n> > const struct v4l2_query_ext_ctrl &info = controlInfo_[id];\n> >\n> > + v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i++];\n> > + v4l2Ctrl.id = id;\n> > +\n> > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {\n> > ControlType type;\n> >\n> > @@ -223,25 +225,22 @@ ControlList V4L2Device::getControls(const std::vector &ids)\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> > + v4l2Ctrl.p_u8 = data.data();\n> > + v4l2Ctrl.size = data.size();\n> > }\n> > -\n> > - v4l2Ctrls[i].id = id;\n> > - i++;\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 +249,11 @@ ControlList V4L2Device::getControls(const std::vector &ids)\n> > /* A specific control failed. */\n> > LOG(V4L2, Error) << \"Unable to read control \" << errorIdx\n> > << \": \" << strerror(-ret);\n> > - count = errorIdx - 1;\n> > +\n> > + v4l2Ctrls.resize(errorIdx);\n> > }\n> >\n> > - updateControls(&ctrls, v4l2Ctrls, count);\n> > + updateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size());\n> >\n> > return ctrls;\n> > }","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CCC61BF829\n\tfor ;\n\tFri, 7 May 2021 22:04:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F283868915;\n\tSat, 8 May 2021 00:04:24 +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 C994B68901\n\tfor ;\n\tSat, 8 May 2021 00:04:22 +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 23F593D7;\n\tSat, 8 May 2021 00:04:22 +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=\"wVbn85bT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620425062;\n\tbh=vzSfIwk2x3zvliQHFZtz/MvjXg7r7ZwY+9MWr7FGTMA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wVbn85bTKS6kNOKRzrWeQgn4j9KyaDWUq8ur33Hj1khyiLrvRuRFRd/3DLXRbE+n9\n\tALaFYVojCPuYXN/WlhzRqVGdKwqbCSTeKgmctc/fMdyRBH8RZdwHIKnCuEKnhQ1fzk\n\tJCrqJr+GtVDvrj5N+LRqtimisXOp3kcB9x9dLW4k=","Date":"Sat, 8 May 2021 01:04:15 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210426001220.15599-1-laurent.pinchart@ideasonboard.com>\n\t<20210426001220.15599-2-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 1/3] libcamera: V4L2Device: Replace\n\tVLA with std::vector 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\" "}}]