[{"id":25320,"web_url":"https://patchwork.libcamera.org/comment/25320/","msgid":"","date":"2022-10-06T22:11:18","subject":"Re: [libcamera-devel] [PATCH v3] libcamera: controls: validate all\n\tControlInfo values","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Christian,\n\nThank you for the patch, and sorry for the delay.\n\nOn Thu, Sep 22, 2022 at 10:22:13PM +0200, Christian Rauch via libcamera-devel wrote:\n> ControlInfoMap::validate only checks the 'min' type against the ControlId\n> type. Extend this with checks against the 'max' type and the 'def' type,\n> if a default is specified. This forces the min/max bounds to have the same\n> type as the controlled value, but leaves the default optional.\n> \n> Signed-off-by: Christian Rauch \n> Reviewed-by: Kieran Bingham \n> ---\n> src/libcamera/controls.cpp | 25 ++++++++++++++++++++++++-\n> 1 file changed, 24 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index bc3db4f6..1e54a712 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -701,9 +701,32 @@ bool ControlInfoMap::validate()\n> \t\t\t\t ? ControlTypeInteger32 : id->type();\n> \t\tconst ControlInfo &info = ctrl.second;\n> \n> +\t\tif (!(info.min().isArray() == info.max().isArray() &&\n> +\t\t info.min().numElements() == info.max().numElements())) {\n> +\t\t\tLOG(Controls, Error)\n> +\t\t\t\t<< \"Control \" << id->name()\n> +\t\t\t\t<< \" range must have the same dimension.\";\n> +\t\t\treturn false;\n> +\t\t}\n\nThis I'm not too sure about, we still haven't reached a conclusion on\nhow min and max should be interpreted for array controls. Could you\nsplit this change to a separate patch, to continue that discussion\nwithout blocking the rest ?\n\n> +\n> +\t\tif (info.min().type() != info.max().type()) {\n\nThis will fail to validate if the control has a minimum and no maximum,\nor the other way around. We don't clearly document if that's allowed,\nand I think it should be. Let's add the following documentation change\nto this patch:\n\ndiff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\nindex bc3db4f69388..5318930aa0fe 100644\n--- a/src/libcamera/controls.cpp\n+++ b/src/libcamera/controls.cpp\n@@ -477,6 +477,11 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen\n * The constraints depend on the object the control applies to, and are\n * constant for the lifetime of that object. They are typically constructed by\n * pipeline handlers to describe the controls they support.\n+ *\n+ * The min(), max() and def() values may be of type ControlTypeNone if the\n+ * control has no minimum bound, maximum bound or default value respectively.\n+ * They shall otherwise store a value of the same type as the control that the\n+ * ControlInfo instance refers to.\n */\n \n /**\n\n> +\t\t\tLOG(Controls, Error)\n> +\t\t\t\t<< \"Control \" << id->name()\n> +\t\t\t\t<< \" range types mismatch\";\n> +\t\t\treturn false;\n> +\t\t}\n> +\n> +\t\tif (info.def().type() != ControlTypeNone &&\n> +\t\t\tinfo.min().type() != info.def().type()) {\n\nThere's a convenient isNone() function that you can use here.\n\n\t\tif (!info.def().isNone() && info.min().type() != info.def().type()) {\n\n> +\t\t\tLOG(Controls, Error)\n> +\t\t\t\t<< \"Control \" << id->name()\n> +\t\t\t\t<< \" default value and info type mismatch\";\n> +\t\t\treturn false;\n> +\t\t}\n> +\n> \t\tif (info.min().type() != rangeType) {\n\nBut I think we can simplify the min, max and def checks and accept the\nnone type for any of them with this:\n\n\t\tif ((!info.min().isNone() && info.min().type() != rangeType) ||\n\t\t (!info.max().isNone() && info.max().type() != rangeType) ||\n\t\t (!info.def().isNone() && info.def().type() != rangeType)) {\n\n> \t\t\tLOG(Controls, Error)\n> -\t\t\t\t<< \"Control \" << utils::hex(id->id())\n> +\t\t\t\t<< \"Control \" << id->name()\n> \t\t\t\t<< \" type and info type mismatch\";\n> \t\t\treturn false;\n> \t\t}","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 EE009BD16B\n\tfor ;\n\tThu, 6 Oct 2022 22:11:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0BA2E62CF5;\n\tFri, 7 Oct 2022 00:11:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9BB64601C7\n\tfor ;\n\tFri, 7 Oct 2022 00:11:23 +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 D4EE06D6;\n\tFri, 7 Oct 2022 00:11:22 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1665094286;\n\tbh=eQFvRgXXl//d79fHRu0RZr99zV8J2IHkBvvWOLgppUg=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=X9Cv4tz2HYuTWuLwbyOPNcSHod7h+aIr5/kK1UHrDItOiT/KDQGd4/ArAQ3CGEM94\n\tysfgkqVbsrVJ7vtHxIxgEnnsdYMTNwhtF523EAxd2gJ0mo61ShHhvsmrXhNASslsbi\n\tJv+1HFNAFQ39ZEOnEQrNjGVyYdPCS51cPrJtCnrTpTX6qXGiPb5BYOTAP7ljSrLJGA\n\tOKVisiMp8jbRaZucHqLR/br/D+4FFweWm/hdzzjEWzqc7X+JarmCJ/IiYjBT9Pmn8L\n\t7ECS8iyjfoCONl+3V2vdVJjun4tyEVXsdOsdwwjys3I+lZebCCCnhCXD6h0GI4J8MU\n\tbB1/m1yAMStWA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1665094283;\n\tbh=eQFvRgXXl//d79fHRu0RZr99zV8J2IHkBvvWOLgppUg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VDbZqJZgSIAxDwzmijrNjD7Sh5BtUyheY1sJq6FA/oQMr9Vbf6w1f/lEpze6farkD\n\tDLkjgI+cK6Ps8NWLNFUDzRp1cZBDJ7BhEXNiukJjClZq1GUuC8lN3ljBKVTevt21mn\n\tFzjml3TaNFJ80MUMCZujME/+r7bxTr1+iUP6rtig="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"VDbZqJZg\"; dkim-atps=neutral","Date":"Fri, 7 Oct 2022 01:11:18 +0300","To":"Christian Rauch ","Message-ID":"","References":"<20220922202213.582824-1-Rauch.Christian@gmx.de>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220922202213.582824-1-Rauch.Christian@gmx.de>","Subject":"Re: [libcamera-devel] [PATCH v3] libcamera: controls: validate all\n\tControlInfo values","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25389,"web_url":"https://patchwork.libcamera.org/comment/25389/","msgid":"","date":"2022-10-11T22:31:43","subject":"Re: [libcamera-devel] [PATCH v3] libcamera: controls: validate all\n\tControlInfo values","submitter":{"id":111,"url":"https://patchwork.libcamera.org/api/people/111/","name":"Christian Rauch","email":"Rauch.Christian@gmx.de"},"content":"Hi Laurent,\n\nThanks for the review.\n\nAm 07.10.22 um 00:11 schrieb Laurent Pinchart:\n> Hi Christian,\n>\n> Thank you for the patch, and sorry for the delay.\n>\n> On Thu, Sep 22, 2022 at 10:22:13PM +0200, Christian Rauch via libcamera-devel wrote:\n>> ControlInfoMap::validate only checks the 'min' type against the ControlId\n>> type. Extend this with checks against the 'max' type and the 'def' type,\n>> if a default is specified. This forces the min/max bounds to have the same\n>> type as the controlled value, but leaves the default optional.\n>>\n>> Signed-off-by: Christian Rauch \n>> Reviewed-by: Kieran Bingham \n>> ---\n>> src/libcamera/controls.cpp | 25 ++++++++++++++++++++++++-\n>> 1 file changed, 24 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>> index bc3db4f6..1e54a712 100644\n>> --- a/src/libcamera/controls.cpp\n>> +++ b/src/libcamera/controls.cpp\n>> @@ -701,9 +701,32 @@ bool ControlInfoMap::validate()\n>> \t\t\t\t ? ControlTypeInteger32 : id->type();\n>> \t\tconst ControlInfo &info = ctrl.second;\n>>\n>> +\t\tif (!(info.min().isArray() == info.max().isArray() &&\n>> +\t\t info.min().numElements() == info.max().numElements())) {\n>> +\t\t\tLOG(Controls, Error)\n>> +\t\t\t\t<< \"Control \" << id->name()\n>> +\t\t\t\t<< \" range must have the same dimension.\";\n>> +\t\t\treturn false;\n>> +\t\t}\n>\n> This I'm not too sure about, we still haven't reached a conclusion on\n> how min and max should be interpreted for array controls. Could you\n> split this change to a separate patch, to continue that discussion\n> without blocking the rest ?\n\nWe do not make this decision here. This only checks that both are either\nscalar or array and have the same dimensions. In both cases, \"isArray\"\nand \"numElements\" are expected to return the same values, but the\nControlInfo can describe a scalar or an array control value.\n\n>> +\n>> +\t\tif (info.min().type() != info.max().type()) {\n>\n> This will fail to validate if the control has a minimum and no maximum,\n> or the other way around. We don't clearly document if that's allowed,\n\nAre there real cases where only the minimum or maximum make sense? For\nnow, I wanted to make sure that min/max are correctly formatted. That\nmeans that either they are both not provided or they both have the same\ndimensionality.\nCurrently, I do not see such mixed cases and would wait until those\narise in practice. Until then, we can verify that the controls follow\nthe conventions that we have currently in place (i.e. all controls have\neither min and max or neither).\n\n> and I think it should be. Let's add the following documentation change\n> to this patch:\n>\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index bc3db4f69388..5318930aa0fe 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -477,6 +477,11 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen\n> * The constraints depend on the object the control applies to, and are\n> * constant for the lifetime of that object. They are typically constructed by\n> * pipeline handlers to describe the controls they support.\n> + *\n> + * The min(), max() and def() values may be of type ControlTypeNone if the\n> + * control has no minimum bound, maximum bound or default value respectively.\n> + * They shall otherwise store a value of the same type as the control that the\n> + * ControlInfo instance refers to.\n> */\n>\n> /**\n>\n>> +\t\t\tLOG(Controls, Error)\n>> +\t\t\t\t<< \"Control \" << id->name()\n>> +\t\t\t\t<< \" range types mismatch\";\n>> +\t\t\treturn false;\n>> +\t\t}\n>> +\n>> +\t\tif (info.def().type() != ControlTypeNone &&\n>> +\t\t\tinfo.min().type() != info.def().type()) {\n>\n> There's a convenient isNone() function that you can use here.\n>\n> \t\tif (!info.def().isNone() && info.min().type() != info.def().type()) {\n>\n>> +\t\t\tLOG(Controls, Error)\n>> +\t\t\t\t<< \"Control \" << id->name()\n>> +\t\t\t\t<< \" default value and info type mismatch\";\n>> +\t\t\treturn false;\n>> +\t\t}\n>> +\n>> \t\tif (info.min().type() != rangeType) {\n>\n> But I think we can simplify the min, max and def checks and accept the\n> none type for any of them with this:\n>\n> \t\tif ((!info.min().isNone() && info.min().type() != rangeType) ||\n> \t\t (!info.max().isNone() && info.max().type() != rangeType) ||\n> \t\t (!info.def().isNone() && info.def().type() != rangeType)) {\n>\n>> \t\t\tLOG(Controls, Error)\n>> -\t\t\t\t<< \"Control \" << utils::hex(id->id())\n>> +\t\t\t\t<< \"Control \" << id->name()\n>> \t\t\t\t<< \" type and info type mismatch\";\n>> \t\t\treturn false;\n>> \t\t}\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 EEBA0C0DA4\n\tfor ;\n\tTue, 11 Oct 2022 22:31:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 15F9062D83;\n\tWed, 12 Oct 2022 00:31:49 +0200 (CEST)","from mout.gmx.net (mout.gmx.net [212.227.15.15])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9DFFE62272\n\tfor ;\n\tWed, 12 Oct 2022 00:31:46 +0200 (CEST)","from [192.168.0.158] ([88.152.184.103]) by mail.gmx.net (mrgmx005\n\t[212.227.17.190]) with ESMTPSA (Nemesis) id\n\t1MQe9s-1oUZjN3caU-00NfWt; Wed, 12 Oct 2022 00:31:43 +0200"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1665527509;\n\tbh=KzjMl+Jj+Z6eeUw5VCVPO0r6s9YbZ3LQGTk783kJj20=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=0VmjE7JhMIfcDuTI7UBrTHomRNjs/SZRewwaXo80rAXtfN7oXdJSMA2ftw7qjyuEK\n\tky1ekTdnEi0QnQwVjuW6xVTZCAiOHquWJj//nvu2b5bSh1/RwFiMYpUI1zo84wmfRs\n\tSauHH9TmWcjGMrNy9knIh8O6KfvmK80QNvF1WHu53Tlc5ApeFmFCNWaYXeiXRi1xIh\n\t5HxDmxXtW8xWdAXUckmzAn/nI3Ic5NnzR+mEm/ZwlVv9HbaIA4ctRA40MtEJaCeBch\n\t0Q6m3m8JyQ+y6/sLGQfUy2UM+YaeZxSE/7ARtjUMB0alzYvAHMdnTLTFhLy31Lo5IO\n\tkU+kd42rdxedw==","v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net;\n\ts=badeba3b8450; t=1665527504;\n\tbh=KzjMl+Jj+Z6eeUw5VCVPO0r6s9YbZ3LQGTk783kJj20=;\n\th=X-UI-Sender-Class:Date:Subject:To:Cc:References:From:In-Reply-To;\n\tb=ffIdGaXr5SBOe8NBdag16ehRiuPfvSpNAGL7tHJvPbFkTC/4HziQbCzNc0oI/tdac\n\tf8lCF+m0yzKN8GHzEFpuuquMgFvXOAPB/9FIWuZnD2C0D+5KHKeHqC4F4nNfFvc4Nj\n\thB7WWuwfT/Wzdw33lwZbZX6+xqElcn0kXw9L1il4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=gmx.net header.i=@gmx.net\n\theader.b=\"ffIdGaXr\"; dkim-atps=neutral","X-UI-Sender-Class":"01bb95c1-4bf8-414a-932a-4f6e2808ef9c","Message-ID":"","Date":"Wed, 12 Oct 2022 00:31:43 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.2.2","To":"Laurent Pinchart ","References":"<20220922202213.582824-1-Rauch.Christian@gmx.de>\n\t","Content-Language":"en-GB","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"quoted-printable","X-Provags-ID":"V03:K1:RmeHGWbpXtatcx02PZOVtvtBP9fob00jtld1/M161O0nFVnz5Zl\n\t0Z/i3rT3Rw60ZLP48W3inKZyeva48p+lKc47RNd5oRq9ZIGISb3kOodE5McZ/JufdfNflUE\n\tGyfwB6wtR7491yBpQzxvKwp4f9BAQ4ciuxS5Uiv3uLSBfuuaJq5P5OU5aDYnew6sCArUj5B\n\tac+ruw2Vd+cF4GxF/5oZw==","X-Spam-Flag":"NO","X-UI-Out-Filterresults":"notjunk:1; V03:K0:huYvVp1bEl8=:b8vAbPrprbP7xTio/Sdehj\n\tzeqhq6ah5BLFgfPB3GmPLjdgreg0wgEOQ+vVVx7oxlINyD91Swt1WEd9KVhch2zdajrpbyCb9\n\tED6rKWzhsoq9vy5kRqenNmIZtLlneOQRfSUA84R/c428iXo69VAhQfM+QPiSMt7DgqPf8bUze\n\tbA4Qb80Xm0b9PB7rFrDgBbXs2jZslJPr6jPUrw8YQkETlYYWqs9O34S27dPf4HLyYlDzG0zVG\n\tIOVjSfl0rnYMDF8Ftf9EqLLD11h5VpmzgcjGAWWiWCxJcJAE6XASWd6ZQ6GopCXa9FS81mmuq\n\tMBYnI4NAoH0hHLGdLmiZgehH9Iw5tKIMvyz5Mv07TvltEpmXt9xdvUxN5LARH3RjmFooEUiUK\n\tMandXsnKuxIqDNrnHGYdg1SN7t8SUnHBYaDaQ33fSrxQIMmosKu80aK6um8mGJBeURkHfkv4Q\n\tUYZxMiYP3biJLkLnfvxYPI1leGhibLTFUQEp9GQpqnR/Cc0Gdx3PvVL8ygyreGo/n4YywedZZ\n\tpDqhQOSMFxFhihxOVdph5LA3MZq/+SLimJ4v5v8Yh5zGjE26hNHdeLjE5ptryiRK2Q0dhFNIy\n\tBGB8UIuWPqLlffmVRG+EnC2KJDp/Mzwgry641NSCxbXpEE59tZI7h0SNz4tXhMs9/C3U6lwjB\n\tEnesCmyFwKlqjs3U1Ef0gMLuKeRSCAUo0Z1vx+F737rk/BjFNyXZEnN+ee2f739l+aV/JfAsp\n\t61LAajE09+DSnIGaLbVVKxwNJsRgcCT+HRBo+QqKJN96gGngef/k/SHKXI7Avg6EZX2oGFndE\n\tC2VLIuqL+8iuMDkmUuo5Nn9hubQZb0SYqjI7QAzwmb8vvgzIhlQLnJox2LH2wKoyuhqdTUJPS\n\tlo6ScskEUZN5i2ek5ApHsyN8oR2wyjo0vdas7vxoKgU6EqhP16SlvMLMfMVdzv1tkGvXpi/LL\n\tPnk/GngtUEuRIgGwb4QJ4ntXGwTyQ4STdZP8qMtPjy3OV3nNt3N8bD42cdtN3y8Vd0uc4XmDP\n\t/9lcXwTLHlqwIfAz8OWwCVLsa84LqL8rng9Dmm8mBk5WsAxESOapI5Iw79+7GL0LRvQ+VeIoY\n\thcpiFR74SbSyqNlLtv5SArIf0G70/BBIu691UFeBJnKy24xSoMtA+c5LtdevckHs48DenMT7J\n\tphcggwOMrwH0eQvLSsdJobErIA2RGAchMzg+cYmn3YZYitQGJp/Js3koaEVXMQlzDfQ5VuHtg\n\tXQAzGsnzWlZdJwrM8+IYu1VfKUnPeKPKusbSMe9li/x35Qgb5cCULpmZ3sxGlZmR2vETruwbS\n\thgy9UUQWUqpaiBRlHNM3qd+4/x5ssBqcKO8SuitHZznNRqjWz+yRAP+AaHTYdgfAxSsNI74rt\n\t7a7RKlgCH+fVoa++dVMIme0Lt0t2wcZ1h++dfiRsBhm5msyTzfOxjQ2fLbzqWgIl/ieOoFcu+\n\tV3bzSvcIXlU23dilB3ayaAjkzyuaWUSJushuoMA1Ouo2XTpsPbrXoScrtjX/WnCun64GUso+Z\n\ttbTN1UTCjxNKuIFRMLdb0rQ3V6/k70x01PDbwTEdW3xsj","Subject":"Re: [libcamera-devel] [PATCH v3] libcamera: controls: validate all\n\tControlInfo values","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","From":"Christian Rauch via libcamera-devel\n\t","Reply-To":"Christian Rauch ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]