[{"id":24616,"web_url":"https://patchwork.libcamera.org/comment/24616/","msgid":"","date":"2022-08-16T20:50:18","subject":"Re: [libcamera-devel] [PATCH] ipa: rkisp1: Add manual color gains","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Tue, Aug 16, 2022 at 02:31:23PM +0900, Paul Elder via libcamera-devel wrote:\n> Add support for manually controlling the color gains on the rkisp1 IPA.\n> To that end, add and plumb the AwbEnable and ColourGains controls. As\n> per-frame controls aren't supported yet in the rkisp1 IPA, simply apply\n> and perform checks on the controls immediately.\n> \n> Signed-off-by: Paul Elder \n> ---\n> src/ipa/rkisp1/algorithms/awb.cpp | 36 +++++++++++++++++++++++++++++--\n> src/ipa/rkisp1/algorithms/awb.h | 3 +++\n> src/ipa/rkisp1/ipa_context.h | 1 +\n> src/ipa/rkisp1/rkisp1.cpp | 6 ++++++\n> 4 files changed, 44 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> index 9f00364d..dcb4b2c9 100644\n> --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> @@ -12,6 +12,7 @@\n> \n> #include \n> \n> +#include \n> #include \n> \n> /**\n> @@ -38,6 +39,7 @@ int Awb::configure(IPAContext &context,\n> \tcontext.frameContext.awb.gains.red = 1.0;\n> \tcontext.frameContext.awb.gains.blue = 1.0;\n> \tcontext.frameContext.awb.gains.green = 1.0;\n> +\tcontext.frameContext.awb.enabled = true;\n> \n> \t/*\n> \t * Define the measurement window for AWB as a centered rectangle\n> @@ -116,6 +118,34 @@ void Awb::prepare(IPAContext &context, rkisp1_params_cfg *params)\n> \tparams->module_ens |= RKISP1_CIF_ISP_MODULE_AWB;\n> }\n> \n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> + */\n> +void Awb::queueRequest(IPAContext &context,\n> +\t\t [[maybe_unused]] const uint32_t frame,\n> +\t\t const ControlList &controls)\n> +{\n> +\tauto &awb = context.frameContext.awb;\n> +\n> +\tconst auto &awbEnable = controls.get(controls::AwbEnable);\n> +\tif (awbEnable) {\n\nWhat would you think of\n\n\tif (awbEnable && *awbEnable != awb.enabled) {\n\nto avoid logging the enabling/disabling message for every request that\nhas the control set even when its value doesn't change ?\n\n> +\t\tawb.enabled = *awbEnable;\n> +\n> +\t\tLOG(RkISP1Awb, Debug)\n> +\t\t\t<< (*awbEnable ? \"Enabling\" : \"Disabling\") << \" AWB\";\n> +\t}\n> +\n> +\tconst auto &colourGains = controls.get(controls::ColourGains);\n> +\tif (colourGains && !awb.enabled) {\n> +\t\tawb.gains.red = (*colourGains)[0];\n> +\t\tawb.gains.blue = (*colourGains)[1];\n> +\n> +\t\tLOG(RkISP1Awb, Debug)\n> +\t\t\t<< \"Set colour gains to red: \" << (*colourGains)[0]\n> +\t\t\t<< \" blue: \" << (*colourGains)[1];\n\ns/ blue/, blue/\n\n\n\t\t\t<< \"Set colour gains to red: \" << awb.gains.red\n\t\t\t<< \"< blue: \" << awb.gains.blue;\n\nis more readable.\n\n> +\t}\n> +}\n> +\n> /**\n> * \\copydoc libcamera::ipa::Algorithm::process\n> */\n> @@ -164,8 +194,10 @@ void Awb::process([[maybe_unused]] IPAContext &context,\n> \t * Gain values are unsigned integer value, range 0 to 4 with 8 bit\n> \t * fractional part.\n> \t */\n> -\tframeContext.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256);\n> -\tframeContext.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);\n> +\tif (frameContext.awb.enabled) {\n> +\t\tframeContext.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256);\n> +\t\tframeContext.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256);\n> +\t}\n\nThere may be room for further refactoring to avoid unneeded calculations\nwhen AWB is in manual mode, but that can be done later.\n\n> \t/* Hardcode the green gain to 1.0. */\n> \tframeContext.awb.gains.green = 1.0;\n> \n> diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h\n> index 667a8beb..4332fac7 100644\n> --- a/src/ipa/rkisp1/algorithms/awb.h\n> +++ b/src/ipa/rkisp1/algorithms/awb.h\n> @@ -21,6 +21,9 @@ public:\n> \n> \tint configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n> \tvoid prepare(IPAContext &context, rkisp1_params_cfg *params) override;\n> +\tvoid queueRequest(IPAContext &context,\n> +\t\t\t const uint32_t frame,\n> +\t\t\t const ControlList &controls);\n> \tvoid process(IPAContext &context, IPAFrameContext *frameCtx,\n> \t\t const rkisp1_stat_buffer *stats) override;\n> \n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 2bdb6a81..fe258b2e 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -55,6 +55,7 @@ struct IPAFrameContext {\n> \t\t} gains;\n> \n> \t\tdouble temperatureK;\n> +\t\tbool enabled;\n\nWe already have an \"enabled\" in IPASessionConfiguration.awb which\nindicates if the AWB algorithm is enabled, without considering if it's\nin auto mode or manual mode. Naming this field just \"enabled\" may be a\nbit confusing. Would autoMode, autoModeEnabled or autoEnabled be better?\nOr is this something that we should consider later, as part of a larger\nnaming overhaul ?\n\nIn any case, the field needs to be documented in ipa_context.cpp.\n\n> \t} awb;\n> \n> \tstruct {\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 17d42d38..55752988 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -89,9 +89,15 @@ private:\n> \n> namespace {\n> \n> +std::array minColourGains = { 0.0f, 0.0f };\n> +std::array maxColourGains = { 3.996f, 3.996f };\n\nconstexpr for both.\n\n> +\n> /* List of controls handled by the RkISP1 IPA */\n> const ControlInfoMap::Map rkisp1Controls{\n> \t{ &controls::AeEnable, ControlInfo(false, true) },\n> +\t{ &controls::AwbEnable, ControlInfo(false, true) },\n> +\t{ &controls::ColourGains, ControlInfo(Span(minColourGains),\n> +\t\t\t\t\t Span(maxColourGains)) },\n\nI don't think this is right, you'll end up using the\n\n\texplicit ControlInfo(Span values,\n\t\t\t const ControlValue &def = {});\n\nmeant for enumerated controls. You should use\n\n\tControlInfo(0.0f, 3.996f, 1.0f)\n\ninstead.\n\nAll those comments are fairly minor. Once addressed,\n\nReviewed-by: Laurent Pinchart \n\n> \t{ &controls::Brightness, ControlInfo(-1.0f, 0.993f) },\n> \t{ &controls::Contrast, ControlInfo(0.0f, 1.993f) },\n> \t{ &controls::Saturation, ControlInfo(0.0f, 1.993f) },","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 6AA69C3272\n\tfor ;\n\tTue, 16 Aug 2022 20:50:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CDA0761FC0;\n\tTue, 16 Aug 2022 22:50:33 +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 7FC8061FA9\n\tfor ;\n\tTue, 16 Aug 2022 22:50:32 +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 E790825B;\n\tTue, 16 Aug 2022 22:50:31 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660683033;\n\tbh=Th0XnDiIxNgBWmBAG/l5oRRzPrhOWUB5UE1EVYBzJrA=;\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=MFffi7zO+GlBLESLiRWqUf9GmFked9kAXD+/XlXwvpVcGsOCw+xU/4ScTJ57Tbwpb\n\tKvUNddcZFgwdE92VJ+jvD2REAV2zJiAzdenwyJ6G+BBKguPBxV5As19woHqzQyKtLI\n\t0Bly7JchiLl/H3Wd8WfjPJJ3tnmkSOV9sZViAkiLzmZNykY7xeRRKAnr6J4T7zqmfi\n\tKvwKaB55aNilVXGs9bgi6dOfK5zVCLDqdDv21ueBj4Gpj4ewBwUWAR8GbgGFvAEKlR\n\tYS3z1saBHdoqF+SECl+LKfWlL8qsnxbLc99inDBivcNM2HSqmpiTa27brNGhb7z47R\n\t2EeNW3wCEcW7Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660683032;\n\tbh=Th0XnDiIxNgBWmBAG/l5oRRzPrhOWUB5UE1EVYBzJrA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=X9rOTn5GtOmF8QeSgzdybuOi2ooLqhOOX5JfNL+M0SfJ2rG5lKt3XAsV2YLonIHM0\n\tAGevwCuL4kdwZxzft9nu+KJAqx5OVOLznNr639p4P7lgiBz7BwRyiEGsnxcqnQ2FUB\n\tE7Y6DdcIWTvoZSuAGGPnuryCYAsEymfRLPBgceEg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"X9rOTn5G\"; dkim-atps=neutral","Date":"Tue, 16 Aug 2022 23:50:18 +0300","To":"Paul Elder ","Message-ID":"","References":"<20220816053123.1100015-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220816053123.1100015-1-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] ipa: rkisp1: Add manual color gains","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\" "}}]