[{"id":34451,"web_url":"https://patchwork.libcamera.org/comment/34451/","msgid":"<20250611140825.GA22730@pendragon.ideasonboard.com>","date":"2025-06-11T14:08:25","subject":"Re: [PATCH v1] ipa: rpi: Initialize enum controls with a list of\n\tvalues","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nOn Mon, Jun 09, 2025 at 05:16:24PM +0200, Barnabás Pőcze wrote:\n> This is how uvcvideo and rkisp1 do it. See ee918b370a08b\n> (\"ipa: rkisp1: agc: Initialize enum controls with a list of values\")\n> for the motivation. In summary, having a list of values is used as a sign\n> that the control is an enum in multiple places (e.g. `cam`, `camshark`).\n> \n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/ipa/rpi/common/ipa_base.cpp | 16 +++++-----------\n> 1 file changed, 5 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> index e0f8b7e78..39e2db86e 100644\n> --- a/src/ipa/rpi/common/ipa_base.cpp\n> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> @@ -57,24 +57,18 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n> const ControlInfoMap::Map ipaControls{\n> \t/* \\todo Move this to the Camera class */\n> \t{ &controls::AeEnable, ControlInfo(false, true, true) },\n> -\t{ &controls::ExposureTimeMode,\n> -\t ControlInfo(static_cast(controls::ExposureTimeModeAuto),\n> -\t\t static_cast(controls::ExposureTimeModeManual),\n> -\t\t static_cast(controls::ExposureTimeModeAuto)) },\n> +\t{ &controls::ExposureTimeMode, ControlInfo(controls::ExposureTimeModeValues) },\n> \t{ &controls::ExposureTime,\n> \t ControlInfo(1, 66666, static_cast(defaultExposureTime.get())) },\n> -\t{ &controls::AnalogueGainMode,\n> -\t ControlInfo(static_cast(controls::AnalogueGainModeAuto),\n> -\t\t static_cast(controls::AnalogueGainModeManual),\n> -\t\t static_cast(controls::AnalogueGainModeAuto)) },\n> +\t{ &controls::AnalogueGainMode, ControlInfo(controls::AnalogueGainModeValues) },\n\nCould we list the values explicitly here, in case new values get added\nto the controls in the future ?\n\n> \t{ &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 1.0f) },\n> \t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> \t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> \t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> \t{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> -\t{ &controls::AeFlickerMode, ControlInfo(static_cast(controls::FlickerOff),\n> -\t\t\t\t\t\tstatic_cast(controls::FlickerManual),\n> -\t\t\t\t\t\tstatic_cast(controls::FlickerOff)) },\n> +\t{ &controls::AeFlickerMode,\n> +\t ControlInfo({ { ControlValue(controls::FlickerOff), ControlValue(controls::FlickerManual) } },\n> +\t\t controls::FlickerOff) },\n> \t{ &controls::AeFlickerPeriod, ControlInfo(100, 1000000) },\n> \t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> \t{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },","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 C231BC3237\n\tfor ;\n\tWed, 11 Jun 2025 14:08:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE1B568DC0;\n\tWed, 11 Jun 2025 16:08:39 +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 5203661552\n\tfor ;\n\tWed, 11 Jun 2025 16:08:38 +0200 (CEST)","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 16561526;\n\tWed, 11 Jun 2025 16:08:30 +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=\"ap0MiNXn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1749650910;\n\tbh=4nwyVx9ZgrUVtbRh0DW4iBrL0KD7raPZlyxqUZDv1K8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ap0MiNXnsBfdmkakxNABCxNDidEkPrc6MCHQLNmAz0/1gJhwvVsiC+lqK0g9HmVRa\n\t2U8x5Hb34eKFAhU6tSP3v2GQggvyTrRgAM/yg7M9WjETvG4OnX10U+oB10mB+KcAom\n\tJz7MkDjH/E1zvW4Y1RgihBdeuoUGtnSNa2zXTBLk=","Date":"Wed, 11 Jun 2025 17:08:25 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] ipa: rpi: Initialize enum controls with a list of\n\tvalues","Message-ID":"<20250611140825.GA22730@pendragon.ideasonboard.com>","References":"<20250609151624.278754-1-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250609151624.278754-1-barnabas.pocze@ideasonboard.com>","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\" "}},{"id":34452,"web_url":"https://patchwork.libcamera.org/comment/34452/","msgid":"<3d63adae-1edd-4624-8594-6dd94a152607@ideasonboard.com>","date":"2025-06-11T14:11:36","subject":"Re: [PATCH v1] ipa: rpi: Initialize enum controls with a list of\n\tvalues","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 06. 11. 16:08 keltezéssel, Laurent Pinchart írta:\n> Hi Barnabás,\n> \n> Thank you for the patch.\n> \n> On Mon, Jun 09, 2025 at 05:16:24PM +0200, Barnabás Pőcze wrote:\n>> This is how uvcvideo and rkisp1 do it. See ee918b370a08b\n>> (\"ipa: rkisp1: agc: Initialize enum controls with a list of values\")\n>> for the motivation. In summary, having a list of values is used as a sign\n>> that the control is an enum in multiple places (e.g. `cam`, `camshark`).\n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> src/ipa/rpi/common/ipa_base.cpp | 16 +++++-----------\n>> 1 file changed, 5 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n>> index e0f8b7e78..39e2db86e 100644\n>> --- a/src/ipa/rpi/common/ipa_base.cpp\n>> +++ b/src/ipa/rpi/common/ipa_base.cpp\n>> @@ -57,24 +57,18 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n>> const ControlInfoMap::Map ipaControls{\n>> \t/* \\todo Move this to the Camera class */\n>> \t{ &controls::AeEnable, ControlInfo(false, true, true) },\n>> -\t{ &controls::ExposureTimeMode,\n>> -\t ControlInfo(static_cast(controls::ExposureTimeModeAuto),\n>> -\t\t static_cast(controls::ExposureTimeModeManual),\n>> -\t\t static_cast(controls::ExposureTimeModeAuto)) },\n>> +\t{ &controls::ExposureTimeMode, ControlInfo(controls::ExposureTimeModeValues) },\n>> \t{ &controls::ExposureTime,\n>> \t ControlInfo(1, 66666, static_cast(defaultExposureTime.get())) },\n>> -\t{ &controls::AnalogueGainMode,\n>> -\t ControlInfo(static_cast(controls::AnalogueGainModeAuto),\n>> -\t\t static_cast(controls::AnalogueGainModeManual),\n>> -\t\t static_cast(controls::AnalogueGainModeAuto)) },\n>> +\t{ &controls::AnalogueGainMode, ControlInfo(controls::AnalogueGainModeValues) },\n> \n> Could we list the values explicitly here, in case new values get added\n> to the controls in the future ?\n\nMost other `ControlInfo`s also specify all values, that's why I did the same here.\nDo these two deserve special treatment? Or maybe all of the should be modified?\n\n\nRegards,\nBarnabás Pőcze\n\n> \n>> \t{ &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 1.0f) },\n>> \t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n>> \t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n>> \t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n>> \t{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n>> -\t{ &controls::AeFlickerMode, ControlInfo(static_cast(controls::FlickerOff),\n>> -\t\t\t\t\t\tstatic_cast(controls::FlickerManual),\n>> -\t\t\t\t\t\tstatic_cast(controls::FlickerOff)) },\n>> +\t{ &controls::AeFlickerMode,\n>> +\t ControlInfo({ { ControlValue(controls::FlickerOff), ControlValue(controls::FlickerManual) } },\n>> +\t\t controls::FlickerOff) },\n>> \t{ &controls::AeFlickerPeriod, ControlInfo(100, 1000000) },\n>> \t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n>> \t{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\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 C9443BDE6B\n\tfor ;\n\tWed, 11 Jun 2025 14:11:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8DE6C68DC2;\n\tWed, 11 Jun 2025 16:11:41 +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 8C31468DBF\n\tfor ;\n\tWed, 11 Jun 2025 16:11:40 +0200 (CEST)","from [192.168.33.24] (185.221.142.30.nat.pool.zt.hu\n\t[185.221.142.30])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5B35E526;\n\tWed, 11 Jun 2025 16:11:32 +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=\"eDi+dLKa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1749651092;\n\tbh=3P3IL9iwK36uZgBoU+cTy7/y3mEIS+6noUegGTc5TdU=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=eDi+dLKaF8DSce2ccqzCSWSPGdOc7jG76Towvd5+PGFaOBs1qECxQkkeK4H7VGs87\n\tWgGJQ7l9iWF5oCKUAk+BeILasgSSoU1GME3v33KlEXGn9PJ2kbrjpGlBL8wALMs4rz\n\tkIOLsrR2CqlbSuaGHqjkTKLBdSLhd+5OR9QPqC10=","Message-ID":"<3d63adae-1edd-4624-8594-6dd94a152607@ideasonboard.com>","Date":"Wed, 11 Jun 2025 16:11:36 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1] ipa: rpi: Initialize enum controls with a list of\n\tvalues","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250609151624.278754-1-barnabas.pocze@ideasonboard.com>\n\t<20250611140825.GA22730@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250611140825.GA22730@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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\" "}},{"id":34453,"web_url":"https://patchwork.libcamera.org/comment/34453/","msgid":"<20250611142242.GJ24465@pendragon.ideasonboard.com>","date":"2025-06-11T14:22:42","subject":"Re: [PATCH v1] ipa: rpi: Initialize enum controls with a list of\n\tvalues","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Jun 11, 2025 at 04:11:36PM +0200, Barnabás Pőcze wrote:\n> 2025. 06. 11. 16:08 keltezéssel, Laurent Pinchart írta:\n> > On Mon, Jun 09, 2025 at 05:16:24PM +0200, Barnabás Pőcze wrote:\n> >> This is how uvcvideo and rkisp1 do it. See ee918b370a08b\n> >> (\"ipa: rkisp1: agc: Initialize enum controls with a list of values\")\n> >> for the motivation. In summary, having a list of values is used as a sign\n> >> that the control is an enum in multiple places (e.g. `cam`, `camshark`).\n> >>\n> >> Signed-off-by: Barnabás Pőcze \n> >> ---\n> >> src/ipa/rpi/common/ipa_base.cpp | 16 +++++-----------\n> >> 1 file changed, 5 insertions(+), 11 deletions(-)\n> >>\n> >> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> >> index e0f8b7e78..39e2db86e 100644\n> >> --- a/src/ipa/rpi/common/ipa_base.cpp\n> >> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> >> @@ -57,24 +57,18 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n> >> const ControlInfoMap::Map ipaControls{\n> >> \t/* \\todo Move this to the Camera class */\n> >> \t{ &controls::AeEnable, ControlInfo(false, true, true) },\n> >> -\t{ &controls::ExposureTimeMode,\n> >> -\t ControlInfo(static_cast(controls::ExposureTimeModeAuto),\n> >> -\t\t static_cast(controls::ExposureTimeModeManual),\n> >> -\t\t static_cast(controls::ExposureTimeModeAuto)) },\n> >> +\t{ &controls::ExposureTimeMode, ControlInfo(controls::ExposureTimeModeValues) },\n> >> \t{ &controls::ExposureTime,\n> >> \t ControlInfo(1, 66666, static_cast(defaultExposureTime.get())) },\n> >> -\t{ &controls::AnalogueGainMode,\n> >> -\t ControlInfo(static_cast(controls::AnalogueGainModeAuto),\n> >> -\t\t static_cast(controls::AnalogueGainModeManual),\n> >> -\t\t static_cast(controls::AnalogueGainModeAuto)) },\n> >> +\t{ &controls::AnalogueGainMode, ControlInfo(controls::AnalogueGainModeValues) },\n> > \n> > Could we list the values explicitly here, in case new values get added\n> > to the controls in the future ?\n> \n> Most other `ControlInfo`s also specify all values, that's why I did the same here.\n> Do these two deserve special treatment? Or maybe all of the should be modified?\n\nI'm tempted to modify them all, or at least to use controls::*Values\nonly in cases where we expect there will never be any new values, or\nwhere adding a new value would require implementing it in all IPA\nmodules due to the nature of the control. Does that make sense ?\n\n> >> \t{ &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 1.0f) },\n> >> \t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> >> \t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> >> \t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> >> \t{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> >> -\t{ &controls::AeFlickerMode, ControlInfo(static_cast(controls::FlickerOff),\n> >> -\t\t\t\t\t\tstatic_cast(controls::FlickerManual),\n> >> -\t\t\t\t\t\tstatic_cast(controls::FlickerOff)) },\n> >> +\t{ &controls::AeFlickerMode,\n> >> +\t ControlInfo({ { ControlValue(controls::FlickerOff), ControlValue(controls::FlickerManual) } },\n> >> +\t\t controls::FlickerOff) },\n> >> \t{ &controls::AeFlickerPeriod, ControlInfo(100, 1000000) },\n> >> \t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> >> \t{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },","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 554FAC3237\n\tfor ;\n\tWed, 11 Jun 2025 14:22:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 33E2F68DC0;\n\tWed, 11 Jun 2025 16:22:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F34D561552\n\tfor ;\n\tWed, 11 Jun 2025 16:22:54 +0200 (CEST)","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 B2F9B526;\n\tWed, 11 Jun 2025 16:22:46 +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=\"Qe8jA0MU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1749651766;\n\tbh=sy1NQaN/UFUpo1fQIle17gZQbL+0thIbXohn2wdPzFc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Qe8jA0MUDf2I/fCWvaWmJOrSwhJ5dDas/HNb0eAuOCkloxd76KoH32CmLyebCOqzA\n\tIjKYeL1PQOIG437ikDYFyVJdfFEDT2xnApxOTfFVp+8BtOrm25JyK27bNpDilNJonY\n\tW5b9NAu2iJoGaxzNJScNnbgRIdCOz/7VOZCD18lk=","Date":"Wed, 11 Jun 2025 17:22:42 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] ipa: rpi: Initialize enum controls with a list of\n\tvalues","Message-ID":"<20250611142242.GJ24465@pendragon.ideasonboard.com>","References":"<20250609151624.278754-1-barnabas.pocze@ideasonboard.com>\n\t<20250611140825.GA22730@pendragon.ideasonboard.com>\n\t<3d63adae-1edd-4624-8594-6dd94a152607@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<3d63adae-1edd-4624-8594-6dd94a152607@ideasonboard.com>","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\" "}}]