[{"id":17986,"web_url":"https://patchwork.libcamera.org/comment/17986/","msgid":"<20210705133900.yquq2pu7ilwn3bbo@uno.localdomain>","date":"2021-07-05T13:39:00","subject":"Re: [libcamera-devel] [RFC PATCH v3 01/16] controls: Add boolean\n\tconstructor for ControlInfo","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Fri, Jul 02, 2021 at 07:37:45PM +0900, Paul Elder wrote:\n> It would be convenient to be able to iterate over available boolean\n> values, for example for controls that designate if some function can be\n> enabled/disabled. The current min/max/def constructor is insufficient,\n> as .values() is empty, so the values cannot be easily iterated over, and\n> creating a Span of booleans does not work for the values constructor.\n>\n> Add a new constructor to ControlInfo that takes a Span of booleans (to\n> allow specifiying one or two available values), and a default. The\n> default value is not optional, as it doesn't make sense to have a silent\n> default for boolean values.\n\nSo this is intended to be used as\n ControlInfo({true}, true);\n ControlInfo({false}, false);\n ControlInfo({true, true}, true);\n ControlInfo({true, true}, false);\n\n?\n\nWhat is the purpose of having to specify a default if only one value\nis available ? Does it even make sense to have a ControlInfo that only\nrepresents true or false ?\n\nCan you make sure\n ControlInfo({true}, false);\ndoesn't happen ?\n\n>\n> Signed-off-by: Paul Elder \n>\n> ---\n> New in v3\n> ---\n> include/libcamera/controls.h | 1 +\n> src/libcamera/controls.cpp | 20 ++++++++++++++++++++\n> 2 files changed, 21 insertions(+)\n>\n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 1bc958a4..2dd147c8 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -272,6 +272,7 @@ public:\n> \t\t\t const ControlValue &def = 0);\n> \texplicit ControlInfo(Span values,\n> \t\t\t const ControlValue &def = {});\n> +\texplicit ControlInfo(Span values, bool def);\n\nThe explicit keyword is used to disallow implicit conversion and copy\nconstruction. It only applies to constructors with a single parameter\nafaict, so it is not required here.\n\n>\n> \tconst ControlValue &min() const { return min_; }\n> \tconst ControlValue &max() const { return max_; }\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 34317fa0..e32e22e2 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -514,6 +514,26 @@ ControlInfo::ControlInfo(Span values,\n> \t\tvalues_.push_back(value);\n> }\n>\n> +/**\n> + * \\brief Construct a ControlInfo from a list of valid boolean values\n> + * \\param[in] values The control valid boolean vaalues\n\ns/vaalues/values\n\nThe values can be just {true, false}, right ?\n\n> + * \\param[in] def The control default boolean value\n> + *\n> + * Construct a ControlInfo from a list of valid boolean values. The ControlInfo\n> + * minimum and maximum values are set to the first and last members of the\n> + * values list respectively. The default value is set to \\a def.\n\nSo ControlInfo({true, false}) != ControlInfo({false, true}) ?\n\n> + */\n> +ControlInfo::ControlInfo(Span values, bool def)\n> +\t: def_(def)\n> +{\n> +\tmin_ = values.front();\n> +\tmax_ = values.back();\n> +\n> +\tvalues_.reserve(2);\n> +\tfor (const bool &value : values)\n> +\t\tvalues_.push_back(value);\n> +}\n> +\n\nCould you point me to where this constructor is used, as it seems\nsuspicious to me :)\n\n\n> /**\n> * \\fn ControlInfo::min()\n> * \\brief Retrieve the minimum value of the control\n> --\n> 2.27.0\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 CA447C0100\n\tfor ;\n\tMon, 5 Jul 2021 13:38:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 238F9684F9;\n\tMon, 5 Jul 2021 15:38:13 +0200 (CEST)","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 D42BB6050A\n\tfor ;\n\tMon, 5 Jul 2021 15:38:11 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 437861C0019;\n\tMon, 5 Jul 2021 13:38:10 +0000 (UTC)"],"Date":"Mon, 5 Jul 2021 15:39:00 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20210705133900.yquq2pu7ilwn3bbo@uno.localdomain>","References":"<20210702103800.41291-1-paul.elder@ideasonboard.com>\n\t<20210702103800.41291-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210702103800.41291-2-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v3 01/16] controls: Add boolean\n\tconstructor for ControlInfo","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17998,"web_url":"https://patchwork.libcamera.org/comment/17998/","msgid":"<20210706102255.GG2510@pyrite.rasen.tech>","date":"2021-07-06T10:22:55","subject":"Re: [libcamera-devel] [RFC PATCH v3 01/16] controls: Add boolean\n\tconstructor for ControlInfo","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jul 05, 2021 at 03:39:00PM +0200, Jacopo Mondi wrote:\n> Hi Paul,\n> \n> On Fri, Jul 02, 2021 at 07:37:45PM +0900, Paul Elder wrote:\n> > It would be convenient to be able to iterate over available boolean\n> > values, for example for controls that designate if some function can be\n> > enabled/disabled. The current min/max/def constructor is insufficient,\n> > as .values() is empty, so the values cannot be easily iterated over, and\n> > creating a Span of booleans does not work for the values constructor.\n> >\n> > Add a new constructor to ControlInfo that takes a Span of booleans (to\n> > allow specifiying one or two available values), and a default. The\n> > default value is not optional, as it doesn't make sense to have a silent\n> > default for boolean values.\n> \n> So this is intended to be used as\n> ControlInfo({true}, true);\n> ControlInfo({false}, false);\n> ControlInfo({true, true}, true);\n\nControlInfo({false, true}, true)\n\n> ControlInfo({true, true}, false);\n\nControlInfo({false, true}, false)\n> \n> ?\n\nYeah, that was the idea. tbh I don't feel so well about it but I'm not\nsure how else to it it.\n\nAlthough, since we have to declare a Span when we use it anyway, maybe\nwe can just use the exsiting Span constructor. Just have to make sure\nfalse comes first, otherwise min_ = values.front() will choose the wrong\nvalue.\n\nSo yeah, I guess I'll just drop this patch.\n\n\nThanks,\n\nPaul\n\n> \n> What is the purpose of having to specify a default if only one value\n> is available ? Does it even make sense to have a ControlInfo that only\n\nIf we only have ControlInfo({true}) with no second parameter, then it'll\nuse the ControlInfo(Span values, const ControlValue\n&def = {}) constructor.\n\n> represents true or false ?\n> \n> Can you make sure\n> ControlInfo({true}, false);\n> doesn't happen ?\n\natm, not really.\n\n> \n> >\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > New in v3\n> > ---\n> > include/libcamera/controls.h | 1 +\n> > src/libcamera/controls.cpp | 20 ++++++++++++++++++++\n> > 2 files changed, 21 insertions(+)\n> >\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 1bc958a4..2dd147c8 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -272,6 +272,7 @@ public:\n> > \t\t\t const ControlValue &def = 0);\n> > \texplicit ControlInfo(Span values,\n> > \t\t\t const ControlValue &def = {});\n> > +\texplicit ControlInfo(Span values, bool def);\n> \n> The explicit keyword is used to disallow implicit conversion and copy\n> construction. It only applies to constructors with a single parameter\n> afaict, so it is not required here.\n> \n> >\n> > \tconst ControlValue &min() const { return min_; }\n> > \tconst ControlValue &max() const { return max_; }\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index 34317fa0..e32e22e2 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -514,6 +514,26 @@ ControlInfo::ControlInfo(Span values,\n> > \t\tvalues_.push_back(value);\n> > }\n> >\n> > +/**\n> > + * \\brief Construct a ControlInfo from a list of valid boolean values\n> > + * \\param[in] values The control valid boolean vaalues\n> \n> s/vaalues/values\n> \n> The values can be just {true, false}, right ?\n> \n> > + * \\param[in] def The control default boolean value\n> > + *\n> > + * Construct a ControlInfo from a list of valid boolean values. The ControlInfo\n> > + * minimum and maximum values are set to the first and last members of the\n> > + * values list respectively. The default value is set to \\a def.\n> \n> So ControlInfo({true, false}) != ControlInfo({false, true}) ?\n> \n> > + */\n> > +ControlInfo::ControlInfo(Span values, bool def)\n> > +\t: def_(def)\n> > +{\n> > +\tmin_ = values.front();\n> > +\tmax_ = values.back();\n> > +\n> > +\tvalues_.reserve(2);\n> > +\tfor (const bool &value : values)\n> > +\t\tvalues_.push_back(value);\n> > +}\n> > +\n> \n> Could you point me to where this constructor is used, as it seems\n> suspicious to me :)\n\nThe pipeline handler uses this constructor to specify the valid values\nfor boolean controls, like AeEnable, AeLock, and AwbLock in this series.\n\n> \n> \n> > /**\n> > * \\fn ControlInfo::min()\n> > * \\brief Retrieve the minimum value of the control\n> > --\n> > 2.27.0\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 7D65BC3224\n\tfor ;\n\tTue, 6 Jul 2021 10:23:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D9A5368502;\n\tTue, 6 Jul 2021 12:23:04 +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 71E3D60288\n\tfor ;\n\tTue, 6 Jul 2021 12:23:03 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D81C7547;\n\tTue, 6 Jul 2021 12:23:01 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uP4OLrL2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625566983;\n\tbh=GInPlA0cfe7IIR4rb4kdh0hsxkEgkWgaLRt0JjIY1mk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uP4OLrL2x6UplYf4mgAA33KI4fD71drmRBT3SE0O+LFpF2CKU9H1SROJYGBdNId2D\n\tvv0nV2telsRZoYjP2LvXDc3i871wmtWUzfK3CNb/7KCQLRZNiVWZJHkAsaGGQDn0Gu\n\tcvUWIluTp4uQPBx0ZxsT8JtfKgKwuCuQz4RKmn44=","Date":"Tue, 6 Jul 2021 19:22:55 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210706102255.GG2510@pyrite.rasen.tech>","References":"<20210702103800.41291-1-paul.elder@ideasonboard.com>\n\t<20210702103800.41291-2-paul.elder@ideasonboard.com>\n\t<20210705133900.yquq2pu7ilwn3bbo@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210705133900.yquq2pu7ilwn3bbo@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC PATCH v3 01/16] controls: Add boolean\n\tconstructor for ControlInfo","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18086,"web_url":"https://patchwork.libcamera.org/comment/18086/","msgid":"","date":"2021-07-11T21:02:23","subject":"Re: [libcamera-devel] [RFC PATCH v3 01/16] controls: Add boolean\n\tconstructor for ControlInfo","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Tue, Jul 06, 2021 at 07:22:55PM +0900, paul.elder@ideasonboard.com wrote:\n> On Mon, Jul 05, 2021 at 03:39:00PM +0200, Jacopo Mondi wrote:\n> > On Fri, Jul 02, 2021 at 07:37:45PM +0900, Paul Elder wrote:\n> > > It would be convenient to be able to iterate over available boolean\n> > > values, for example for controls that designate if some function can be\n> > > enabled/disabled. The current min/max/def constructor is insufficient,\n> > > as .values() is empty, so the values cannot be easily iterated over, and\n> > > creating a Span of booleans does not work for the values constructor.\n> > >\n> > > Add a new constructor to ControlInfo that takes a Span of booleans (to\n> > > allow specifiying one or two available values), and a default. The\n> > > default value is not optional, as it doesn't make sense to have a silent\n> > > default for boolean values.\n> > \n> > So this is intended to be used as\n> > ControlInfo({true}, true);\n> > ControlInfo({false}, false);\n> > ControlInfo({true, true}, true);\n> \n> ControlInfo({false, true}, true)\n> \n> > ControlInfo({true, true}, false);\n> \n> ControlInfo({false, true}, false)\n> > \n> > ?\n> \n> Yeah, that was the idea. tbh I don't feel so well about it but I'm not\n> sure how else to it it.\n> \n> Although, since we have to declare a Span when we use it anyway, maybe\n> we can just use the exsiting Span constructor. Just have to make sure\n> false comes first, otherwise min_ = values.front() will choose the wrong\n> value.\n\nYou could have a special case in the constructor when the type is bool,\nand sort the values. I'm not sure if it's worth it though.\n\nNow that I think about it, for enumerated types, would it make sense to\npass an std::set to the ControlInfo constructor ? It would\nrequire adding a comparison operator to the ControlValue class though.\n\n> So yeah, I guess I'll just drop this patch.\n> \n> > What is the purpose of having to specify a default if only one value\n> > is available ? Does it even make sense to have a ControlInfo that only\n> \n> If we only have ControlInfo({true}) with no second parameter, then it'll\n> use the ControlInfo(Span values, const ControlValue\n> &def = {}) constructor.\n> \n> > represents true or false ?\n> > \n> > Can you make sure\n> > ControlInfo({true}, false);\n> > doesn't happen ?\n> \n> atm, not really.\n> \n> > > Signed-off-by: Paul Elder \n> > >\n> > > ---\n> > > New in v3\n> > > ---\n> > > include/libcamera/controls.h | 1 +\n> > > src/libcamera/controls.cpp | 20 ++++++++++++++++++++\n> > > 2 files changed, 21 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > index 1bc958a4..2dd147c8 100644\n> > > --- a/include/libcamera/controls.h\n> > > +++ b/include/libcamera/controls.h\n> > > @@ -272,6 +272,7 @@ public:\n> > > \t\t\t const ControlValue &def = 0);\n> > > \texplicit ControlInfo(Span values,\n> > > \t\t\t const ControlValue &def = {});\n> > > +\texplicit ControlInfo(Span values, bool def);\n> > \n> > The explicit keyword is used to disallow implicit conversion and copy\n> > construction. It only applies to constructors with a single parameter\n> > afaict, so it is not required here.\n> > \n> > >\n> > > \tconst ControlValue &min() const { return min_; }\n> > > \tconst ControlValue &max() const { return max_; }\n> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > index 34317fa0..e32e22e2 100644\n> > > --- a/src/libcamera/controls.cpp\n> > > +++ b/src/libcamera/controls.cpp\n> > > @@ -514,6 +514,26 @@ ControlInfo::ControlInfo(Span values,\n> > > \t\tvalues_.push_back(value);\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Construct a ControlInfo from a list of valid boolean values\n> > > + * \\param[in] values The control valid boolean vaalues\n> > \n> > s/vaalues/values\n> > \n> > The values can be just {true, false}, right ?\n> > \n> > > + * \\param[in] def The control default boolean value\n> > > + *\n> > > + * Construct a ControlInfo from a list of valid boolean values. The ControlInfo\n> > > + * minimum and maximum values are set to the first and last members of the\n> > > + * values list respectively. The default value is set to \\a def.\n> > \n> > So ControlInfo({true, false}) != ControlInfo({false, true}) ?\n> > \n> > > + */\n> > > +ControlInfo::ControlInfo(Span values, bool def)\n> > > +\t: def_(def)\n> > > +{\n> > > +\tmin_ = values.front();\n> > > +\tmax_ = values.back();\n> > > +\n> > > +\tvalues_.reserve(2);\n> > > +\tfor (const bool &value : values)\n> > > +\t\tvalues_.push_back(value);\n> > > +}\n> > > +\n> > \n> > Could you point me to where this constructor is used, as it seems\n> > suspicious to me :)\n> \n> The pipeline handler uses this constructor to specify the valid values\n> for boolean controls, like AeEnable, AeLock, and AwbLock in this series.\n> \n> > > /**\n> > > * \\fn ControlInfo::min()\n> > > * \\brief Retrieve the minimum value of the control","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 89DFFC3224\n\tfor ;\n\tSun, 11 Jul 2021 21:03:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E00A66851D;\n\tSun, 11 Jul 2021 23:03:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A55EA68519\n\tfor ;\n\tSun, 11 Jul 2021 23:03: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 17031CC;\n\tSun, 11 Jul 2021 23:03:09 +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=\"AXOgD9Bf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626037389;\n\tbh=nyYrIVPy23k2JSMEWK2cPhc3aw2nNdrqg+Weng8kF94=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AXOgD9BfbgmhsqefDgpl++9ThCHuF5+0wiYYcGzoOF9iguFYBFh0C/ktlhiIVIaR9\n\tLkZUnm4gckSZvCQrs5ubEnYDPbkuQgM0rqfOj5G4OzdDImmOKlOReNVoCXNMHeVDOB\n\t9eXYJUxzfWjzHBkIlMcC6RmPBE7ilGzR51CkSoSY=","Date":"Mon, 12 Jul 2021 00:02:23 +0300","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20210702103800.41291-1-paul.elder@ideasonboard.com>\n\t<20210702103800.41291-2-paul.elder@ideasonboard.com>\n\t<20210705133900.yquq2pu7ilwn3bbo@uno.localdomain>\n\t<20210706102255.GG2510@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210706102255.GG2510@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [RFC PATCH v3 01/16] controls: Add boolean\n\tconstructor for ControlInfo","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]