[{"id":14318,"web_url":"https://patchwork.libcamera.org/comment/14318/","msgid":"","date":"2020-12-23T05:56:50","subject":"Re: [libcamera-devel] [PATCH v2 6/9] libcamera: controls:\n\tList-initialize ControlList","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Fri, Dec 18, 2020 at 05:47:51PM +0100, Jacopo Mondi wrote:\n> Provide an additional constructor to the ControlList class to\n> allow the construction of instances of the class with the usage\n> of an std::intializer_list<> of ControlId and an associated\n> ControlValue.\n> \n> The new constructor allows to intialize a ControlList instance\n\ns/intialize/initialize/\n\n> at construction time, allowing the declaration of populated\n> ControlList instances.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/controls.h | 2 ++\n> src/libcamera/controls.cpp | 11 +++++++++++\n> 2 files changed, 13 insertions(+)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 3634dc431618..4df6615a3c7b 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -348,9 +348,11 @@ class ControlList\n> {\n> private:\n> \tusing ControlListMap = std::unordered_map;\n> +\tusing ControlsMap = std::unordered_map;\n\nAs the ControlId should never be null, should this be a reference ?\n\n> \n> public:\n> \tControlList();\n> +\tControlList(std::initializer_list controls);\n> \tControlList(const ControlIdMap &idmap, ControlValidator *validator = nullptr);\n> \tControlList(const ControlInfoMap &infoMap, ControlValidator *validator = nullptr);\n> \n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index c58ed3946f3b..7650057dde7d 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -797,6 +797,17 @@ ControlList::ControlList()\n> {\n> }\n> \n> +/**\n> + * \\brief Construct a ControlList with a list of values\n> + * \\param[in] controls The controls and associated values to initialize the list with\n> + */\n> +ControlList::ControlList(std::initializer_list controls)\n> +\t: ControlList()\n> +{\n> +\tfor (const auto &ctrl : controls)\n> +\t\tset(ctrl.first->id(), ctrl.second);\n\nThe annoying part here is to we lose type safety. The ControlValue in\nthe controls variable is constructor without any type inference based on\na Control. This leads to the explicit construction of, for instance,\nSpan, in patch 7/9. Is there a way we could do better ?\n\n> +}\n> +\n> /**\n> * \\brief Construct a ControlList with an optional control validator\n> * \\param[in] idmap The ControlId map for the control list target object","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 26E72C0F1B\n\tfor ;\n\tWed, 23 Dec 2020 05:57:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AB22C61591;\n\tWed, 23 Dec 2020 06:57:00 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C304660320\n\tfor ;\n\tWed, 23 Dec 2020 06:56:58 +0100 (CET)","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 4B8A5A32;\n\tWed, 23 Dec 2020 06:56:58 +0100 (CET)"],"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=\"P+WpJ1Yn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1608703018;\n\tbh=dlk/GKPwv4dG+UoFUTqqV3u/y+rKtKZ/WE7lJSe2/ug=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=P+WpJ1Ynvnuz1S+yGDi3BFubRsmD8fAM/PMKFDlPL0eChQakmWqlym50jK9idG0Kr\n\tHIJDV6xuxCqvezdA8EaPPhJFNQ32/yh50Fuoem4rmIBjV7fdAP8tVzxfwKz0EL4HaQ\n\tU7XyAeetefW+M7cR5zdaye3IJ0ebk9W0Ab1mcDVk=","Date":"Wed, 23 Dec 2020 07:56:50 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20201218164754.81422-1-jacopo@jmondi.org>\n\t<20201218164754.81422-7-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201218164754.81422-7-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 6/9] libcamera: controls:\n\tList-initialize ControlList","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":14321,"web_url":"https://patchwork.libcamera.org/comment/14321/","msgid":"<20201223163434.moqoubvtsezf6onn@uno.localdomain>","date":"2020-12-23T16:34:34","subject":"Re: [libcamera-devel] [PATCH v2 6/9] libcamera: controls:\n\tList-initialize ControlList","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Wed, Dec 23, 2020 at 07:56:50AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, Dec 18, 2020 at 05:47:51PM +0100, Jacopo Mondi wrote:\n> > Provide an additional constructor to the ControlList class to\n> > allow the construction of instances of the class with the usage\n> > of an std::intializer_list<> of ControlId and an associated\n> > ControlValue.\n> >\n> > The new constructor allows to intialize a ControlList instance\n>\n> s/intialize/initialize/\n>\n> > at construction time, allowing the declaration of populated\n> > ControlList instances.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/controls.h | 2 ++\n> > src/libcamera/controls.cpp | 11 +++++++++++\n> > 2 files changed, 13 insertions(+)\n> >\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 3634dc431618..4df6615a3c7b 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -348,9 +348,11 @@ class ControlList\n> > {\n> > private:\n> > \tusing ControlListMap = std::unordered_map;\n> > +\tusing ControlsMap = std::unordered_map;\n>\n> As the ControlId should never be null, should this be a reference ?\n>\n> >\n> > public:\n> > \tControlList();\n> > +\tControlList(std::initializer_list controls);\n> > \tControlList(const ControlIdMap &idmap, ControlValidator *validator = nullptr);\n> > \tControlList(const ControlInfoMap &infoMap, ControlValidator *validator = nullptr);\n> >\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index c58ed3946f3b..7650057dde7d 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -797,6 +797,17 @@ ControlList::ControlList()\n> > {\n> > }\n> >\n> > +/**\n> > + * \\brief Construct a ControlList with a list of values\n> > + * \\param[in] controls The controls and associated values to initialize the list with\n> > + */\n> > +ControlList::ControlList(std::initializer_list controls)\n> > +\t: ControlList()\n> > +{\n> > +\tfor (const auto &ctrl : controls)\n> > +\t\tset(ctrl.first->id(), ctrl.second);\n>\n> The annoying part here is to we lose type safety. The ControlValue in\n> the controls variable is constructor without any type inference based on\n> a Control. This leads to the explicit construction of, for instance,\n> Span, in patch 7/9. Is there a way we could do better ?\n>\n\nI'll keep the patch like it is right now in next version as I've got\nso many piled up patches this seems like a minor issue, as the only\nuse case for statically initialized ControlList is the sensor database\ncurrent implementation. It can be changed on top if desired.\n\n> > +}\n> > +\n> > /**\n> > * \\brief Construct a ControlList with an optional control validator\n> > * \\param[in] idmap The ControlId map for the control list target object\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 A3594C0F1A\n\tfor ;\n\tWed, 23 Dec 2020 16:34:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 35F2C615AC;\n\tWed, 23 Dec 2020 17:34:24 +0100 (CET)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1D46160527\n\tfor ;\n\tWed, 23 Dec 2020 17:34:23 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 7F54D1C0003;\n\tWed, 23 Dec 2020 16:34:22 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 23 Dec 2020 17:34:34 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20201223163434.moqoubvtsezf6onn@uno.localdomain>","References":"<20201218164754.81422-1-jacopo@jmondi.org>\n\t<20201218164754.81422-7-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 6/9] libcamera: controls:\n\tList-initialize ControlList","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\" "}}]