[{"id":33146,"web_url":"https://patchwork.libcamera.org/comment/33146/","msgid":"<20250123225631.GA20415@pendragon.ideasonboard.com>","date":"2025-01-23T22:56:31","subject":"Re: [RFC PATCH] ipa: rkisp1: agc: Initialize enum controls with a\n\tlist of values","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\nThank you for the patch.\n\nOn Thu, Jan 23, 2025 at 01:09:14PM +0100, Stefan Klug wrote:\n> The controls ExposureTimeMode and AnalogueGainMode are shown in camshark\n> as normal int entries instead of enum popups. The reason is that\n> ControlInfos for these controls are constructed using min/max instead\n> of a list of valid ControlValues. Camshark uses the values() vector to\n> deduce if the control is an enum or not. It might be debatable if this\n> is the correct check, but all other ControlInfos for enum controls in\n> libcamera are initialized using a list.\n\nMaybe the Control (or ControlId) class should report if a control is an\nenum. We don't have that now though, and even if we did, this patch\nlooks good as it makes the two controls consistent with the other\nenumerated controls.\n\n> Modify the construction of the ControlInfos to use the Span based\n> constructor to fix that issue.\n> \n> Signed-off-by: Stefan Klug \n\nReviewed-by: Laurent Pinchart \n\nCould you please do the same for the RPi IPA module ?\n\n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++------\n> 1 file changed, 6 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 1680669c6875..e7c6de757593 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -149,13 +149,13 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)\n> \t\treturn ret;\n> \n> \tcontext.ctrlMap[&controls::ExposureTimeMode] =\n> -\t\tControlInfo(static_cast(controls::ExposureTimeModeAuto),\n> -\t\t\t static_cast(controls::ExposureTimeModeManual),\n> -\t\t\t static_cast(controls::ExposureTimeModeAuto));\n> +\t\tControlInfo({ { ControlValue(controls::ExposureTimeModeAuto),\n> +\t\t\t\tControlValue(controls::ExposureTimeModeManual) } },\n> +\t\t\t controls::ExposureTimeModeAuto);\n> \tcontext.ctrlMap[&controls::AnalogueGainMode] =\n> -\t\tControlInfo(static_cast(controls::AnalogueGainModeAuto),\n> -\t\t\t static_cast(controls::AnalogueGainModeManual),\n> -\t\t\t static_cast(controls::AnalogueGainModeAuto));\n> +\t\tControlInfo({ { ControlValue(controls::AnalogueGainModeAuto),\n> +\t\t\t\tControlValue(controls::AnalogueGainModeManual) } },\n> +\t\t\t controls::AnalogueGainModeAuto);\n> \t/* \\todo Move this to the Camera class */\n> \tcontext.ctrlMap[&controls::AeEnable] = ControlInfo(false, true, true);\n> \tcontext.ctrlMap.merge(controls());","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 E96FAC322E\n\tfor ;\n\tThu, 23 Jan 2025 22:56:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 142046855B;\n\tThu, 23 Jan 2025 23:56:43 +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 87D8161878\n\tfor ;\n\tThu, 23 Jan 2025 23:56:41 +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 7130389A;\n\tThu, 23 Jan 2025 23:55:37 +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=\"P4cKm4JZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737672937;\n\tbh=aqdx66CSEzQ23yQZpSXM/jhw/Oi8bwjKApd3hicnj+A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=P4cKm4JZ1Tz83Igvbn17LomrSJh0ckq1OuMvy6NyshuywToQ2pXUBg5hsxZ1dve93\n\tV9C5KH4/K1tlRS47R8rDaoLnT4I7nta35fhlktkk21RCN3wMTqgVLSI12B/eBv4h8M\n\tdzPgWlcVg3MqYfkrsxsrOEIT9eyRAlj/aOAB3GkU=","Date":"Fri, 24 Jan 2025 00:56:31 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH] ipa: rkisp1: agc: Initialize enum controls with a\n\tlist of values","Message-ID":"<20250123225631.GA20415@pendragon.ideasonboard.com>","References":"<20250123120918.86780-1-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250123120918.86780-1-stefan.klug@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":33198,"web_url":"https://patchwork.libcamera.org/comment/33198/","msgid":"","date":"2025-01-27T09:16:08","subject":"Re: [RFC PATCH] ipa: rkisp1: agc: Initialize enum controls with a\n\tlist of values","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Thu, Jan 23, 2025 at 01:09:14PM +0100, Stefan Klug wrote:\n> The controls ExposureTimeMode and AnalogueGainMode are shown in camshark\n> as normal int entries instead of enum popups. The reason is that\n> ControlInfos for these controls are constructed using min/max instead\n> of a list of valid ControlValues. Camshark uses the values() vector to\n> deduce if the control is an enum or not. It might be debatable if this\n> is the correct check, but all other ControlInfos for enum controls in\n\nI think that is the correct way. (I guess technically you could also\ncheck ControlId::enumerators(), but in cam we check\ninfo.values().empty() first before iterating over enumerators() anyway)\n\n> libcamera are initialized using a list.\n> \n> Modify the construction of the ControlInfos to use the Span based\n> constructor to fix that issue.\n> \n> Signed-off-by: Stefan Klug \n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++------\n> 1 file changed, 6 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 1680669c6875..e7c6de757593 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -149,13 +149,13 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)\n> \t\treturn ret;\n> \n> \tcontext.ctrlMap[&controls::ExposureTimeMode] =\n> -\t\tControlInfo(static_cast(controls::ExposureTimeModeAuto),\n> -\t\t\t static_cast(controls::ExposureTimeModeManual),\n> -\t\t\t static_cast(controls::ExposureTimeModeAuto));\n> +\t\tControlInfo({ { ControlValue(controls::ExposureTimeModeAuto),\n> +\t\t\t\tControlValue(controls::ExposureTimeModeManual) } },\n> +\t\t\t controls::ExposureTimeModeAuto);\n> \tcontext.ctrlMap[&controls::AnalogueGainMode] =\n> -\t\tControlInfo(static_cast(controls::AnalogueGainModeAuto),\n> -\t\t\t static_cast(controls::AnalogueGainModeManual),\n> -\t\t\t static_cast(controls::AnalogueGainModeAuto));\n> +\t\tControlInfo({ { ControlValue(controls::AnalogueGainModeAuto),\n> +\t\t\t\tControlValue(controls::AnalogueGainModeManual) } },\n> +\t\t\t controls::AnalogueGainModeAuto);\n\nLooks good to me.\n\n\nReviewed-by: Paul Elder \n\n> \t/* \\todo Move this to the Camera class */\n> \tcontext.ctrlMap[&controls::AeEnable] = ControlInfo(false, true, true);\n> \tcontext.ctrlMap.merge(controls());\n> -- \n> 2.43.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 EDC32BEFBE\n\tfor ;\n\tMon, 27 Jan 2025 09:16:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A28B96855D;\n\tMon, 27 Jan 2025 10:16:17 +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 02FE26851B\n\tfor ;\n\tMon, 27 Jan 2025 10:16:15 +0100 (CET)","from pyrite.rasen.tech (unknown [206.0.71.13])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 89AC7352;\n\tMon, 27 Jan 2025 10:15:08 +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=\"g0jImTVP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737969309;\n\tbh=A9OegKh4bxGIGaz5/XcXI5aMlP5QVd9RoUA0UI08zTk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=g0jImTVPGzTzZJ4rGdITdJx5EuwfVXGNtn9UXBZyISoqdB18votMWfpo1LoWgLKBs\n\t2/I5dCCyBbg31BEyoVuHPjNszueqFFDh7TPSYSiA9Iq6VVLPlHflwivlc5YdLNdzbf\n\tdHy7moXL5vtbtJWHkHYg7ZfiATiw4jteEZfbGvPc=","Date":"Mon, 27 Jan 2025 04:16:08 -0500","From":"Paul Elder ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH] ipa: rkisp1: agc: Initialize enum controls with a\n\tlist of values","Message-ID":"","References":"<20250123120918.86780-1-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20250123120918.86780-1-stefan.klug@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\" "}}]