[{"id":33342,"web_url":"https://patchwork.libcamera.org/comment/33342/","msgid":"<173944423578.2010433.7239116887505560616@ping.linuxembedded.co.uk>","date":"2025-02-13T10:57:15","subject":"Re: [RFC PATCH v1] libcamera: controls: ControlInfo: Ensure types\n\tmatch","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-01-28 12:13:55)\n> It is expected that the min/max/default/etc. values in a `ControlInfo`\n> have the same type. So add assertions to check that.\n> \n\nThis patch looks like a very good addition, which could have helped\ncatch the issue Naush faced yesterday\n(https://patchwork.libcamera.org/patch/22778/)\n\nIs there anyway we can make this a compile time assertion instead of a\nruntime check?\n\n--\nKieran\n\n\n> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>\n> ---\n>  src/libcamera/controls.cpp | 22 +++++++++++++++++++++-\n>  1 file changed, 21 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 70f6f6092..07f276b35 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -580,6 +580,19 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n>   * \\brief The Control template type T\n>   */\n>  \n> +namespace {\n> +\n> +bool sameShape(const ControlValue &a, const ControlValue &b)\n> +{\n> +       /**\n> +        * \\todo This is a best effort approach. Without the `ControlId`\n> +        * there is no way to check if the sizes of fixed size arrays match.\n> +        */\n> +       return a.type() == b.type() && a.isArray() == b.isArray();\n> +}\n> +\n> +}\n> +\n>  /**\n>   * \\class ControlInfo\n>   * \\brief Describe the limits of valid values for a Control\n> @@ -601,6 +614,7 @@ ControlInfo::ControlInfo(const ControlValue &min,\n>                          const ControlValue &def)\n>         : min_(min), max_(max), def_(def)\n>  {\n> +       ASSERT(sameShape(min_, max_) && (def_.isNone() || sameShape(max_, def_)));\n>  }\n>  \n>  /**\n> @@ -616,13 +630,19 @@ ControlInfo::ControlInfo(const ControlValue &min,\n>  ControlInfo::ControlInfo(Span<const ControlValue> values,\n>                          const ControlValue &def)\n>  {\n> +       ASSERT(!values.empty());\n> +\n>         min_ = values.front();\n>         max_ = values.back();\n>         def_ = !def.isNone() ? def : values.front();\n>  \n> +       ASSERT(sameShape(min_, max_) && sameShape(max_, def_));\n> +\n>         values_.reserve(values.size());\n> -       for (const ControlValue &value : values)\n> +       for (const ControlValue &value : values) {\n> +               ASSERT(sameShape(def_, value));\n>                 values_.push_back(value);\n> +       }\n>  }\n>  \n>  /**\n> -- \n> 2.48.1\n> \n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 457D7BD7D8\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 13 Feb 2025 10:57:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 33F3868636;\n\tThu, 13 Feb 2025 11:57:48 +0100 (CET)","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 7195261862\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 13 Feb 2025 11:57:45 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7D3A7166A;\n\tThu, 13 Feb 2025 11:56:12 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Sh7qS1JY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1739444178;\n\tbh=uI4oufBQTLKfcWCha+wDpoqqU32qcea3lifsTu0xvFg=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=Sh7qS1JY1IqjgkjkTs+xDWGS03H50MEpNcabRurlSfCCdakijJve3XN/swIKefXGV\n\tvbqUz1aIWq61I5MMA7fvNTozrxfJ/Wob5hZO3edechvAdlSMG3YsRiliYlTqa+aKnS\n\tQu4UwfzMteiC214WxcSNGWDd5zUdbgYo6Q0zA92o=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250128121352.494582-1-pobrn@protonmail.com>","References":"<20250128121352.494582-1-pobrn@protonmail.com>","Subject":"Re: [RFC PATCH v1] libcamera: controls: ControlInfo: Ensure types\n\tmatch","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 13 Feb 2025 10:57:15 +0000","Message-ID":"<173944423578.2010433.7239116887505560616@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":33343,"web_url":"https://patchwork.libcamera.org/comment/33343/","msgid":"<_OJAFYj10wjoK8i9eDWCypo038YTAzFXe8JE46Np8a88FS4-MH6HoJAeHXQPnTPGEdFD6clJY1FG8CxwGPOHTIAnwbTeI1noA-GyWVg1WWw=@protonmail.com>","date":"2025-02-13T16:21:39","subject":"Re: [RFC PATCH v1] libcamera: controls: ControlInfo: Ensure types\n\tmatch","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2025. február 13., csütörtök 11:57 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:\n\n> Quoting Barnabás Pőcze (2025-01-28 12:13:55)\n> > It is expected that the min/max/default/etc. values in a `ControlInfo`\n> > have the same type. So add assertions to check that.\n> >\n> \n> This patch looks like a very good addition, which could have helped\n> catch the issue Naush faced yesterday\n> (https://patchwork.libcamera.org/patch/22778/)\n> \n> Is there anyway we can make this a compile time assertion instead of a\n> runtime check?\n\nI think it is definitely possible, at least to some degree. But that would be an\nappreciably more intrusive change.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> --\n> Kieran\n> \n> \n> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>\n> > ---\n> >  src/libcamera/controls.cpp | 22 +++++++++++++++++++++-\n> >  1 file changed, 21 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index 70f6f6092..07f276b35 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -580,6 +580,19 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> >   * \\brief The Control template type T\n> >   */\n> >\n> > +namespace {\n> > +\n> > +bool sameShape(const ControlValue &a, const ControlValue &b)\n> > +{\n> > +       /**\n> > +        * \\todo This is a best effort approach. Without the `ControlId`\n> > +        * there is no way to check if the sizes of fixed size arrays match.\n> > +        */\n> > +       return a.type() == b.type() && a.isArray() == b.isArray();\n> > +}\n> > +\n> > +}\n> > +\n> >  /**\n> >   * \\class ControlInfo\n> >   * \\brief Describe the limits of valid values for a Control\n> > @@ -601,6 +614,7 @@ ControlInfo::ControlInfo(const ControlValue &min,\n> >                          const ControlValue &def)\n> >         : min_(min), max_(max), def_(def)\n> >  {\n> > +       ASSERT(sameShape(min_, max_) && (def_.isNone() || sameShape(max_, def_)));\n> >  }\n> >\n> >  /**\n> > @@ -616,13 +630,19 @@ ControlInfo::ControlInfo(const ControlValue &min,\n> >  ControlInfo::ControlInfo(Span<const ControlValue> values,\n> >                          const ControlValue &def)\n> >  {\n> > +       ASSERT(!values.empty());\n> > +\n> >         min_ = values.front();\n> >         max_ = values.back();\n> >         def_ = !def.isNone() ? def : values.front();\n> >\n> > +       ASSERT(sameShape(min_, max_) && sameShape(max_, def_));\n> > +\n> >         values_.reserve(values.size());\n> > -       for (const ControlValue &value : values)\n> > +       for (const ControlValue &value : values) {\n> > +               ASSERT(sameShape(def_, value));\n> >                 values_.push_back(value);\n> > +       }\n> >  }\n> >\n> >  /**\n> > --\n> > 2.48.1\n> >\n> >\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 EDE9EC3272\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 13 Feb 2025 16:21:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D5C4B68633;\n\tThu, 13 Feb 2025 17:21:46 +0100 (CET)","from mail-40134.protonmail.ch (mail-40134.protonmail.ch\n\t[185.70.40.134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1E7936185F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 13 Feb 2025 17:21:45 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"QUkvTG7t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1739463704; x=1739722904;\n\tbh=hn21aXWhj2D9eW9mndT1GYhlsOk7YvclZjgvsCTCErk=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=QUkvTG7tGVLpKNlwYU2hkW/o1HmpeV6cZUZHLp7HVJWyEE8iDUBKdPaqHTAcVTPx9\n\tm4+tjZnEaj+zs5JmbnQxc6zgoWaAsvEU8NPi2Kk+xNsc8fz050+2jxxilK0JbQ7uHo\n\tlESaqNBnZtpIzi2djBXBnOwRTTQbbzgGE3DhKzLtzTa892W8zaw8ws2G1fU05S6z09\n\tB6pSuQIv6T8ApEF2H1PBjtRojhDDkIgP4tuGJRAyMPpSEBxrG/U720O2Niko+TFS8Y\n\tB81NYd1J5pv8Q0LLAWxagDAILHlYofyPlV8rbcNkBk0RYbjgIN6kC1IoL81YV6w953\n\tcAVaHekV2IYKw==","Date":"Thu, 13 Feb 2025 16:21:39 +0000","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1] libcamera: controls: ControlInfo: Ensure types\n\tmatch","Message-ID":"<_OJAFYj10wjoK8i9eDWCypo038YTAzFXe8JE46Np8a88FS4-MH6HoJAeHXQPnTPGEdFD6clJY1FG8CxwGPOHTIAnwbTeI1noA-GyWVg1WWw=@protonmail.com>","In-Reply-To":"<173944423578.2010433.7239116887505560616@ping.linuxembedded.co.uk>","References":"<20250128121352.494582-1-pobrn@protonmail.com>\n\t<173944423578.2010433.7239116887505560616@ping.linuxembedded.co.uk>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"d48d4695114c650b7e88163689bb0848413df955","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":33349,"web_url":"https://patchwork.libcamera.org/comment/33349/","msgid":"<20250213205733.GD22998@pendragon.ideasonboard.com>","date":"2025-02-13T20:57:33","subject":"Re: [RFC PATCH v1] libcamera: controls: ControlInfo: Ensure types\n\tmatch","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Feb 13, 2025 at 04:21:39PM +0000, Barnabás Pőcze wrote:\n> Hi\n> \n> \n> 2025. február 13., csütörtök 11:57 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:\n> \n> > Quoting Barnabás Pőcze (2025-01-28 12:13:55)\n> > > It is expected that the min/max/default/etc. values in a `ControlInfo`\n> > > have the same type. So add assertions to check that.\n> > >\n> > \n> > This patch looks like a very good addition, which could have helped\n> > catch the issue Naush faced yesterday\n> > (https://patchwork.libcamera.org/patch/22778/)\n> > \n> > Is there anyway we can make this a compile time assertion instead of a\n> > runtime check?\n> \n> I think it is definitely possible, at least to some degree. But that would be an\n> appreciably more intrusive change.\n\nI wonder how instrusive it would be. We would need to pass a const\nControl<T> reference to the ControlInfo constructor, and therefore make\nthe constructors inline (part of the constructors could be deferred to a\nseparate private init function, I'm thinking in particular about the\nSpan-based constructor). One possibly upside it that we may be able to\nreplace the ControlValue arguments with const T &, potentially\nsimplifying the callers. I'm sure I'm missing some of the impact this\nwould have.\n\n> > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>\n> > > ---\n> > >  src/libcamera/controls.cpp | 22 +++++++++++++++++++++-\n> > >  1 file changed, 21 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > index 70f6f6092..07f276b35 100644\n> > > --- a/src/libcamera/controls.cpp\n> > > +++ b/src/libcamera/controls.cpp\n> > > @@ -580,6 +580,19 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> > >   * \\brief The Control template type T\n> > >   */\n> > >\n> > > +namespace {\n> > > +\n> > > +bool sameShape(const ControlValue &a, const ControlValue &b)\n> > > +{\n> > > +       /**\n> > > +        * \\todo This is a best effort approach. Without the `ControlId`\n> > > +        * there is no way to check if the sizes of fixed size arrays match.\n> > > +        */\n> > > +       return a.type() == b.type() && a.isArray() == b.isArray();\n> > > +}\n> > > +\n> > > +}\n> > > +\n> > >  /**\n> > >   * \\class ControlInfo\n> > >   * \\brief Describe the limits of valid values for a Control\n> > > @@ -601,6 +614,7 @@ ControlInfo::ControlInfo(const ControlValue &min,\n> > >                          const ControlValue &def)\n> > >         : min_(min), max_(max), def_(def)\n> > >  {\n> > > +       ASSERT(sameShape(min_, max_) && (def_.isNone() || sameShape(max_, def_)));\n> > >  }\n> > >\n> > >  /**\n> > > @@ -616,13 +630,19 @@ ControlInfo::ControlInfo(const ControlValue &min,\n> > >  ControlInfo::ControlInfo(Span<const ControlValue> values,\n> > >                          const ControlValue &def)\n> > >  {\n> > > +       ASSERT(!values.empty());\n> > > +\n> > >         min_ = values.front();\n> > >         max_ = values.back();\n> > >         def_ = !def.isNone() ? def : values.front();\n> > >\n> > > +       ASSERT(sameShape(min_, max_) && sameShape(max_, def_));\n> > > +\n> > >         values_.reserve(values.size());\n> > > -       for (const ControlValue &value : values)\n> > > +       for (const ControlValue &value : values) {\n> > > +               ASSERT(sameShape(def_, value));\n> > >                 values_.push_back(value);\n> > > +       }\n> > >  }\n> > >\n> > >  /**","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 B40B5C3272\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 13 Feb 2025 20:57:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AEBB768640;\n\tThu, 13 Feb 2025 21:57:47 +0100 (CET)","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 03C1D6185F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 13 Feb 2025 21:57:45 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EB576594;\n\tThu, 13 Feb 2025 21:56:26 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XGiqRDdM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1739480187;\n\tbh=iKvuY89mqMoCq/N9c4P0csWuGSYyVwE4+i4sTzpcq44=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XGiqRDdMXJsbGKEonJJA1Niitj9holjDyycjIyRBAyED8pCIOsdYd/CEkLoNGFqGM\n\tJY7xIVHbSbWEqGfcNCe/Wd9Ou5nzECaqFvLGdOIjMunrLyvwEhuJO9kIAekHgL5DO3\n\tFxYQvec0bZs5UAHAZe1FqY/D39Nnm/prgsB4UZsk=","Date":"Thu, 13 Feb 2025 22:57:33 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>","Cc":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1] libcamera: controls: ControlInfo: Ensure types\n\tmatch","Message-ID":"<20250213205733.GD22998@pendragon.ideasonboard.com>","References":"<20250128121352.494582-1-pobrn@protonmail.com>\n\t<173944423578.2010433.7239116887505560616@ping.linuxembedded.co.uk>\n\t<_OJAFYj10wjoK8i9eDWCypo038YTAzFXe8JE46Np8a88FS4-MH6HoJAeHXQPnTPGEdFD6clJY1FG8CxwGPOHTIAnwbTeI1noA-GyWVg1WWw=@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<_OJAFYj10wjoK8i9eDWCypo038YTAzFXe8JE46Np8a88FS4-MH6HoJAeHXQPnTPGEdFD6clJY1FG8CxwGPOHTIAnwbTeI1noA-GyWVg1WWw=@protonmail.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":33352,"web_url":"https://patchwork.libcamera.org/comment/33352/","msgid":"<t-kaM00VN94JrNoOjXzA8D_CuiAmy5VmuaSGFWXVzMMyobE55UA6Dm5Rtzsvz3nSoIw1BbWChEV-rU_w2Qw7fhe8EQFhN5DL9fOE4sekbYk=@protonmail.com>","date":"2025-02-13T22:20:46","subject":"Re: [RFC PATCH v1] libcamera: controls: ControlInfo: Ensure types\n\tmatch","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2025. február 13., csütörtök 21:57 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta:\n\n> On Thu, Feb 13, 2025 at 04:21:39PM +0000, Barnabás Pőcze wrote:\n> > Hi\n> >\n> >\n> > 2025. február 13., csütörtök 11:57 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:\n> >\n> > > Quoting Barnabás Pőcze (2025-01-28 12:13:55)\n> > > > It is expected that the min/max/default/etc. values in a `ControlInfo`\n> > > > have the same type. So add assertions to check that.\n> > > >\n> > >\n> > > This patch looks like a very good addition, which could have helped\n> > > catch the issue Naush faced yesterday\n> > > (https://patchwork.libcamera.org/patch/22778/)\n> > >\n> > > Is there anyway we can make this a compile time assertion instead of a\n> > > runtime check?\n> >\n> > I think it is definitely possible, at least to some degree. But that would be an\n> > appreciably more intrusive change.\n> \n> I wonder how instrusive it would be. We would need to pass a const\n> Control<T> reference to the ControlInfo constructor, and therefore make\n> the constructors inline (part of the constructors could be deferred to a\n> separate private init function, I'm thinking in particular about the\n> Span-based constructor). One possibly upside it that we may be able to\n> replace the ControlValue arguments with const T &, potentially\n> simplifying the callers. I'm sure I'm missing some of the impact this\n> would have.\n\nMaybe something like this:\n\n\ttemplate<typename T, typename... Args>\n\tControlInfo(const Control<T>&, std::optional<typename Control<T>::type> def, const Args&... values)\n\t{\n\t\tstatic_assert(sizeof...(values) > 0);\n\t\tstatic_assert(((details::control_type<T>::value == details::control_type<Args>::value) && ...));\n\t\tstatic_assert(((details::control_type<T>::size == details::control_type<Args>::size) && ...));\n\n\t\t(values_.emplace_back(values), ...);\n\n\t\tmin_ = values_.front();\n\t\tmax_ = values_.back();\n\t\tdef_ = def ? ControlValue(*def) : values_.front();\n\t}\n\nBut there are still some things:\n\n 1) strings, i.e. handling `const char (&)[N]`, `const char *`, (`std::string_view`),\n    and `std::string` (although there are no string controls at the moment);\n 2) arrays, i.e. the handling of `vector<T>`, `T (&)[N]`, `span<T>`, etc;\n 3) the type information about enums is lost, which is not ideal in my opinion.\n\nI am wondering if there is anything preventing (3) from being addressed?\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>\n> > > > ---\n> > > >  src/libcamera/controls.cpp | 22 +++++++++++++++++++++-\n> > > >  1 file changed, 21 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > > index 70f6f6092..07f276b35 100644\n> > > > --- a/src/libcamera/controls.cpp\n> > > > +++ b/src/libcamera/controls.cpp\n> > > > @@ -580,6 +580,19 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> > > >   * \\brief The Control template type T\n> > > >   */\n> > > >\n> > > > +namespace {\n> > > > +\n> > > > +bool sameShape(const ControlValue &a, const ControlValue &b)\n> > > > +{\n> > > > +       /**\n> > > > +        * \\todo This is a best effort approach. Without the `ControlId`\n> > > > +        * there is no way to check if the sizes of fixed size arrays match.\n> > > > +        */\n> > > > +       return a.type() == b.type() && a.isArray() == b.isArray();\n> > > > +}\n> > > > +\n> > > > +}\n> > > > +\n> > > >  /**\n> > > >   * \\class ControlInfo\n> > > >   * \\brief Describe the limits of valid values for a Control\n> > > > @@ -601,6 +614,7 @@ ControlInfo::ControlInfo(const ControlValue &min,\n> > > >                          const ControlValue &def)\n> > > >         : min_(min), max_(max), def_(def)\n> > > >  {\n> > > > +       ASSERT(sameShape(min_, max_) && (def_.isNone() || sameShape(max_, def_)));\n> > > >  }\n> > > >\n> > > >  /**\n> > > > @@ -616,13 +630,19 @@ ControlInfo::ControlInfo(const ControlValue &min,\n> > > >  ControlInfo::ControlInfo(Span<const ControlValue> values,\n> > > >                          const ControlValue &def)\n> > > >  {\n> > > > +       ASSERT(!values.empty());\n> > > > +\n> > > >         min_ = values.front();\n> > > >         max_ = values.back();\n> > > >         def_ = !def.isNone() ? def : values.front();\n> > > >\n> > > > +       ASSERT(sameShape(min_, max_) && sameShape(max_, def_));\n> > > > +\n> > > >         values_.reserve(values.size());\n> > > > -       for (const ControlValue &value : values)\n> > > > +       for (const ControlValue &value : values) {\n> > > > +               ASSERT(sameShape(def_, value));\n> > > >                 values_.push_back(value);\n> > > > +       }\n> > > >  }\n> > > >\n> > > >  /**\n> \n> --\n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 BF766C3272\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 13 Feb 2025 22:20:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DFC5468641;\n\tThu, 13 Feb 2025 23:20:52 +0100 (CET)","from mail-40131.protonmail.ch (mail-40131.protonmail.ch\n\t[185.70.40.131])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 237C16185F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 13 Feb 2025 23:20:51 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"EbRGSy7e\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1739485250; x=1739744450;\n\tbh=HAHM48B5uLgt/75CetaQay/E9D8z5ZgNJJDf+x9Ubz0=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=EbRGSy7eBAYFLuCAvQXwxfaqAhwDmks57dbodhGLc+gIZWPkQn+Ua69P5rn2eWLNS\n\tq0Z9yUZk66hJ39RvWUrb4M71EfMOUI8UWFVvZyanZt4rQiXALPRN2gjlQXnLt36KPs\n\tLzQ9D2mNWYQZqJWF/dKn2RlXEJq49onPBBxRCyEAghuqsYDgJvRzKzvR/i0pqDL5Qk\n\t8HkBvbVw4P6UD8o0It3yepY37A6GsiHBwyiLxHAkXLvxcG0QR6vWYYVTmMqbwQ9fJp\n\tWcUVWFcOS2ROfiuEj6Jsu4dgFYioIPpEoMtiuiAS9U8Gig8wqicEIrIQXsN/0oEWFe\n\tTUG7j5dwCZclg==","Date":"Thu, 13 Feb 2025 22:20:46 +0000","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>","Cc":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1] libcamera: controls: ControlInfo: Ensure types\n\tmatch","Message-ID":"<t-kaM00VN94JrNoOjXzA8D_CuiAmy5VmuaSGFWXVzMMyobE55UA6Dm5Rtzsvz3nSoIw1BbWChEV-rU_w2Qw7fhe8EQFhN5DL9fOE4sekbYk=@protonmail.com>","In-Reply-To":"<20250213205733.GD22998@pendragon.ideasonboard.com>","References":"<20250128121352.494582-1-pobrn@protonmail.com>\n\t<173944423578.2010433.7239116887505560616@ping.linuxembedded.co.uk>\n\t<_OJAFYj10wjoK8i9eDWCypo038YTAzFXe8JE46Np8a88FS4-MH6HoJAeHXQPnTPGEdFD6clJY1FG8CxwGPOHTIAnwbTeI1noA-GyWVg1WWw=@protonmail.com>\n\t<20250213205733.GD22998@pendragon.ideasonboard.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"a0d2f06fe4a6789caac2dbd9a755ee2455e66e2f","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]