[{"id":34279,"web_url":"https://patchwork.libcamera.org/comment/34279/","msgid":"<174766747416.55052.2143543023460668771@localhost>","date":"2025-05-19T15:11:14","subject":"Re: [PATCH v2 2/2] Revert \"controls: Add boolean constructors for\n\tControlInfo\"","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nQuoting Barnabás Pőcze (2025-05-19 11:12:07)\n> This reverts commit 10cdc914dad282b4ca0ad11067d5c6d446af1fcc.\n> \n> The contstructors introduced by that commit are not used anywhere,\n> and they do not match the existing practice for boolean controls.\n> \n> Specifically, every single boolean control is described by calling\n> the `ControlInfo(ControlValue, ControlValue, ControlValue)`\n> constructor. Crucially, that constructor does not set `values_`,\n> while the two removed contructors do. And whether or not `values_`\n> has any elements is currently used as an implicit sign to decide\n> whether or not the control is \"enum-like\", and those are assumed\n> to have type `int32_t`.\n> \n> For example, any boolean control described using any of the two\n> removed constructors would cause an assertion in failure in\n> `CameraSession::listControls()` when calling `value.get()`.\n> \n> Signed-off-by: Barnabás Pőcze \n> Reviewed-by: Paul Elder \n\nReading the initial commit I don't really understand the original\nreason. But as these constructors are not used anymore, the revert seems\nreasonable.\n\nReviewed-by: Stefan Klug \n\nCheers,\nStefan\n\n> ---\n> changes in v2:\n> * keep tests\n> ---\n> include/libcamera/controls.h | 3 ---\n> src/libcamera/controls.cpp | 29 -----------------------------\n> test/controls/control_info.cpp | 9 ++++-----\n> 3 files changed, 4 insertions(+), 37 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 2ae4ec3d4..32fb31f78 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -10,7 +10,6 @@\n> #include \n> #include \n> #include \n> -#include \n> #include \n> #include \n> #include \n> @@ -334,8 +333,6 @@ public:\n> const ControlValue &def = {});\n> explicit ControlInfo(Span values,\n> const ControlValue &def = {});\n> - explicit ControlInfo(std::set values, bool def);\n> - explicit ControlInfo(bool value);\n> \n> const ControlValue &min() const { return min_; }\n> const ControlValue &max() const { return max_; }\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 70f6f6092..eaa34e70b 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -625,35 +625,6 @@ ControlInfo::ControlInfo(Span values,\n> values_.push_back(value);\n> }\n> \n> -/**\n> - * \\brief Construct a boolean ControlInfo with both boolean values\n> - * \\param[in] values The control valid boolean values (both true and false)\n> - * \\param[in] def The control default boolean value\n> - *\n> - * Construct a ControlInfo for a boolean control, where both true and false are\n> - * valid values. \\a values must be { false, true } (the order is irrelevant).\n> - * The minimum value will always be false, and the maximum always true. The\n> - * default value is \\a def.\n> - */\n> -ControlInfo::ControlInfo(std::set values, bool def)\n> - : min_(false), max_(true), def_(def), values_({ false, true })\n> -{\n> - ASSERT(values.count(def) && values.size() == 2);\n> -}\n> -\n> -/**\n> - * \\brief Construct a boolean ControlInfo with only one valid value\n> - * \\param[in] value The control valid boolean value\n> - *\n> - * Construct a ControlInfo for a boolean control, where there is only valid\n> - * value. The minimum, maximum, and default values will all be \\a value.\n> - */\n> -ControlInfo::ControlInfo(bool value)\n> - : min_(value), max_(value), def_(value)\n> -{\n> - values_ = { value };\n> -}\n> -\n> /**\n> * \\fn ControlInfo::min()\n> * \\brief Retrieve the minimum value of the control\n> diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp\n> index e1bb43f0e..905d92dd5 100644\n> --- a/test/controls/control_info.cpp\n> +++ b/test/controls/control_info.cpp\n> @@ -50,7 +50,7 @@ protected:\n> * Test information retrieval from a control with boolean\n> * values.\n> */\n> - ControlInfo aeEnable({ false, true }, false);\n> + ControlInfo aeEnable(false, true, false);\n> \n> if (aeEnable.min().get() != false ||\n> aeEnable.def().get() != false ||\n> @@ -59,13 +59,12 @@ protected:\n> return TestFail;\n> }\n> \n> - if (aeEnable.values()[0].get() != false ||\n> - aeEnable.values()[1].get() != true) {\n> + if (!aeEnable.values().empty()) {\n> cout << \"Invalid control values for AeEnable\" << endl;\n> return TestFail;\n> }\n> \n> - ControlInfo awbEnable(true);\n> + ControlInfo awbEnable(true, true, true);\n> \n> if (awbEnable.min().get() != true ||\n> awbEnable.def().get() != true ||\n> @@ -74,7 +73,7 @@ protected:\n> return TestFail;\n> }\n> \n> - if (awbEnable.values()[0].get() != true) {\n> + if (!awbEnable.values().empty()) {\n> cout << \"Invalid control values for AwbEnable\" << endl;\n> return TestFail;\n> }\n> --\n> 2.49.0","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 5CCB8BD78E\n\tfor ;\n\tMon, 19 May 2025 15:11:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9DE3D68B6C;\n\tMon, 19 May 2025 17:11:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2565168B67\n\tfor ;\n\tMon, 19 May 2025 17:11:17 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:cab0:c1aa:ead7:82ef])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D3C18446;\n\tMon, 19 May 2025 17:10:56 +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=\"EBqp6SJr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1747667456;\n\tbh=zTe+Y7lf1HBDqomVEyitZBOE6vNfx/YRItRLPahmcgE=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=EBqp6SJrs/U1EEscX6YsxYwh2teDadqoNBPzGzJ4XxZv03aWLJV7FC/kU8rvPtgUZ\n\tv4CWYByIcRTw/Yz6lByoseQj4h4mxjoUyZ5IJYA/zTYcAc+7e+Qk/6+nKbP/Giku1j\n\tFQN43axcxeZELgKM0KjW5wtkXcNo2upOO8A343MI=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250519091207.561269-2-barnabas.pocze@ideasonboard.com>","References":"<20250519091207.561269-1-barnabas.pocze@ideasonboard.com>\n\t<20250519091207.561269-2-barnabas.pocze@ideasonboard.com>","Subject":"Re: [PATCH v2 2/2] Revert \"controls: Add boolean constructors for\n\tControlInfo\"","From":"Stefan Klug ","Cc":"Paul Elder ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 19 May 2025 17:11:14 +0200","Message-ID":"<174766747416.55052.2143543023460668771@localhost>","User-Agent":"alot/0.12.dev16+g501a9541e2e6.d20250519","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]