[{"id":33064,"web_url":"https://patchwork.libcamera.org/comment/33064/","msgid":"","date":"2025-01-13T22:59:38","subject":"Re: [PATCH v1 07/11] ipa: rkisp1: Use grey world algorithm from\n\tlibipa","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Thu, Jan 09, 2025 at 12:53:58PM +0100, Stefan Klug wrote:\n> Now that libipa contains a grey world algorithm, use that.\n> \n> Signed-off-by: Stefan Klug \n\nReviewed-by: Paul Elder \n\n> ---\n> src/ipa/rkisp1/algorithms/awb.cpp | 72 ++++++++++++++++++++-----------\n> src/ipa/rkisp1/algorithms/awb.h | 4 +-\n> 2 files changed, 49 insertions(+), 27 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> index f6449565834b..42a4784998bc 100644\n> --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> @@ -16,6 +16,7 @@\n> \n> #include \n> \n> +#include \"libipa/awb_grey.h\"\n> #include \"libipa/colours.h\"\n> \n> /**\n> @@ -40,6 +41,28 @@ constexpr int32_t kDefaultColourTemperature = 5000;\n> /* Minimum mean value below which AWB can't operate. */\n> constexpr double kMeanMinThreshold = 2.0;\n> \n> +class RkISP1AwbStats : public AwbStats\n> +{\n> +public:\n> +\tRkISP1AwbStats(const RGB &rgbMeans)\n> +\t\t: rgbMeans_(rgbMeans) {}\n> +\n> +\tdouble computeColourError([[maybe_unused]] const RGB &gains) const override\n> +\t{\n> +\t\tLOG(RkISP1Awb, Error)\n> +\t\t\t<< \"RkISP1AwbStats::computeColourError is not implemented\";\n> +\t\treturn 0.0;\n> +\t}\n> +\n> +\tRGB getRGBMeans() const override\n> +\t{\n> +\t\treturn rgbMeans_;\n> +\t};\n> +\n> +private:\n> +\tRGB rgbMeans_;\n> +};\n> +\n> Awb::Awb()\n> \t: rgbMode_(false)\n> {\n> @@ -55,15 +78,12 @@ int Awb::init(IPAContext &context, const YamlObject &tuningData)\n> \t\t\t\t\t\t\t kMaxColourTemperature,\n> \t\t\t\t\t\t\t kDefaultColourTemperature);\n> \n> -\tInterpolator> gainCurve;\n> -\tint ret = gainCurve.readYaml(tuningData[\"colourGains\"], \"ct\", \"gains\");\n> -\tif (ret < 0)\n> -\t\tLOG(RkISP1Awb, Warning)\n> -\t\t\t<< \"Failed to parse 'colourGains' \"\n> -\t\t\t<< \"parameter from tuning file; \"\n> -\t\t\t<< \"manual colour temperature will not work properly\";\n> -\telse\n> -\t\tcolourGainCurve_ = gainCurve;\n> +\tawbAlgo_ = std::make_unique();\n> +\tint ret = awbAlgo_->init(tuningData);\n> +\tif (ret) {\n> +\t\tLOG(RkISP1Awb, Error) << \"Failed to init awb algorithm\";\n> +\t\treturn ret;\n> +\t}\n> \n> \treturn 0;\n> }\n> @@ -128,10 +148,10 @@ void Awb::queueRequest(IPAContext &context,\n> \t\t * This will be fixed with the bayes AWB algorithm.\n> \t\t */\n> \t\tupdate = true;\n> -\t} else if (colourTemperature && colourGainCurve_) {\n> -\t\tconst auto &gains = colourGainCurve_->getInterpolated(*colourTemperature);\n> -\t\tawb.gains.manual.r() = gains[0];\n> -\t\tawb.gains.manual.b() = gains[1];\n> +\t} else if (colourTemperature) {\n> +\t\tconst auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);\n> +\t\tawb.gains.manual.r() = gains.r();\n> +\t\tawb.gains.manual.b() = gains.b();\n> \t\tawb.temperatureK = *colourTemperature;\n> \t\tupdate = true;\n> \t}\n> @@ -251,33 +271,33 @@ void Awb::process(IPAContext &context,\n> \t rgbMeans.b() < kMeanMinThreshold)\n> \t\treturn;\n> \n> -\tactiveState.awb.temperatureK = estimateCCT(rgbMeans);\n> +\t/*\n> +\t * \\Todo: Hardcode lux to a fixed value, until an estimation is\n> +\t * implemented.\n> +\t */\n> +\tint lux = 1000;\n> +\n> +\tRkISP1AwbStats awbStats{ rgbMeans };\n> +\tAwbResult r = awbAlgo_->calculateAwb(awbStats, lux);\n> +\n> +\tactiveState.awb.temperatureK = r.colourTemperature;\n> \n> \t/* Metadata shall contain the up to date measurement */\n> \tmetadata.set(controls::ColourTemperature, activeState.awb.temperatureK);\n> \n> -\t/*\n> -\t * Estimate the red and blue gains to apply in a grey world. The green\n> -\t * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the\n> -\t * divisor to a minimum value of 1.0.\n> -\t */\n> -\tRGB gains({ rgbMeans.g() / std::max(rgbMeans.r(), 1.0),\n> -\t\t\t 1.0,\n> -\t\t\t rgbMeans.g() / std::max(rgbMeans.b(), 1.0) });\n> -\n> \t/*\n> \t * Clamp the gain values to the hardware, which expresses gains as Q2.8\n> \t * unsigned integer values. Set the minimum just above zero to avoid\n> \t * divisions by zero when computing the raw means in subsequent\n> \t * iterations.\n> \t */\n> -\tgains = gains.max(1.0 / 256).min(1023.0 / 256);\n> +\tr.gains = r.gains.max(1.0 / 256).min(1023.0 / 256);\n> \n> \t/* Filter the values to avoid oscillations. */\n> \tdouble speed = 0.2;\n> -\tgains = gains * speed + activeState.awb.gains.automatic * (1 - speed);\n> +\tr.gains = r.gains * speed + activeState.awb.gains.automatic * (1 - speed);\n> \n> -\tactiveState.awb.gains.automatic = gains;\n> +\tactiveState.awb.gains.automatic = r.gains;\n> \n> \tLOG(RkISP1Awb, Debug)\n> \t\t<< std::showpoint\n> diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h\n> index 2a64cb60d5f4..3382b2178567 100644\n> --- a/src/ipa/rkisp1/algorithms/awb.h\n> +++ b/src/ipa/rkisp1/algorithms/awb.h\n> @@ -9,6 +9,7 @@\n> \n> #include \n> \n> +#include \"libipa/awb.h\"\n> #include \"libipa/interpolator.h\"\n> #include \"libipa/vector.h\"\n> \n> @@ -41,7 +42,8 @@ private:\n> \tRGB calculateRgbMeans(const IPAFrameContext &frameContext,\n> \t\t\t\t const rkisp1_cif_isp_awb_stat *awb) const;\n> \n> -\tstd::optional>> colourGainCurve_;\n> +\tstd::unique_ptr awbAlgo_;\n> +\n> \tbool rgbMode_;\n> };\n> \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 D1D04C326C\n\tfor ;\n\tMon, 13 Jan 2025 23:00:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 873A46851D;\n\tTue, 14 Jan 2025 00:00:19 +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 BABD968503\n\tfor ;\n\tTue, 14 Jan 2025 00:00:17 +0100 (CET)","from pyrite.rasen.tech (unknown [173.16.167.215])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 80F559FF;\n\tMon, 13 Jan 2025 23:59:18 +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=\"VySXsOOo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736809160;\n\tbh=uZrCTT0VvzZsQyJeoCp5pv+Jo/UnSdCla5AN+44JO2A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VySXsOOoLNRquBrZNPDfFvwToV1LhQCKGidkMemZxkTinNK/yTWSq90mqpk4sVhlw\n\tFZwsUTcHBiO0TaLSffbhC8ET1h+Veqs4Mk2Nwms5xgyugOhOuCs4VBJmBuALkiaO0L\n\tygf3JAkgMR0Pv663C4CuDdfU90X6d4+kl1mW79hI=","Date":"Mon, 13 Jan 2025 16:59:38 -0600","From":"Paul Elder ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 07/11] ipa: rkisp1: Use grey world algorithm from\n\tlibipa","Message-ID":"","References":"<20250109115412.356768-1-stefan.klug@ideasonboard.com>\n\t<20250109115412.356768-8-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20250109115412.356768-8-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":33100,"web_url":"https://patchwork.libcamera.org/comment/33100/","msgid":"","date":"2025-01-20T11:59:56","subject":"Re: [PATCH v1 07/11] ipa: rkisp1: Use grey world algorithm from\n\tlibipa","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Stefan\n\nOn 09/01/2025 11:53, Stefan Klug wrote:\n> Now that libipa contains a grey world algorithm, use that.\n>\n> Signed-off-by: Stefan Klug \n> ---\n> src/ipa/rkisp1/algorithms/awb.cpp | 72 ++++++++++++++++++++-----------\n> src/ipa/rkisp1/algorithms/awb.h | 4 +-\n> 2 files changed, 49 insertions(+), 27 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> index f6449565834b..42a4784998bc 100644\n> --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> @@ -16,6 +16,7 @@\n> \n> #include \n> \n> +#include \"libipa/awb_grey.h\"\n> #include \"libipa/colours.h\"\n> \n> /**\n> @@ -40,6 +41,28 @@ constexpr int32_t kDefaultColourTemperature = 5000;\n> /* Minimum mean value below which AWB can't operate. */\n> constexpr double kMeanMinThreshold = 2.0;\n> \n> +class RkISP1AwbStats : public AwbStats\n> +{\n> +public:\n> +\tRkISP1AwbStats(const RGB &rgbMeans)\n> +\t\t: rgbMeans_(rgbMeans) {}\n> +\n> +\tdouble computeColourError([[maybe_unused]] const RGB &gains) const override\n> +\t{\n> +\t\tLOG(RkISP1Awb, Error)\n> +\t\t\t<< \"RkISP1AwbStats::computeColourError is not implemented\";\n> +\t\treturn 0.0;\n> +\t}\nIf it's optional, perhaps rather than a pure virtual function it should have this \"not implemented\" \nvariant in the base class?\n> +\n> +\tRGB getRGBMeans() const override\n> +\t{\n> +\t\treturn rgbMeans_;\n> +\t};\n> +\n> +private:\n> +\tRGB rgbMeans_;\n> +};\n> +\n> Awb::Awb()\n> \t: rgbMode_(false)\n> {\n> @@ -55,15 +78,12 @@ int Awb::init(IPAContext &context, const YamlObject &tuningData)\n> \t\t\t\t\t\t\t kMaxColourTemperature,\n> \t\t\t\t\t\t\t kDefaultColourTemperature);\n> \n> -\tInterpolator> gainCurve;\n> -\tint ret = gainCurve.readYaml(tuningData[\"colourGains\"], \"ct\", \"gains\");\n> -\tif (ret < 0)\n> -\t\tLOG(RkISP1Awb, Warning)\n> -\t\t\t<< \"Failed to parse 'colourGains' \"\n> -\t\t\t<< \"parameter from tuning file; \"\n> -\t\t\t<< \"manual colour temperature will not work properly\";\n> -\telse\n> -\t\tcolourGainCurve_ = gainCurve;\n> +\tawbAlgo_ = std::make_unique();\n> +\tint ret = awbAlgo_->init(tuningData);\n> +\tif (ret) {\n> +\t\tLOG(RkISP1Awb, Error) << \"Failed to init awb algorithm\";\n> +\t\treturn ret;\n> +\t}\nI'm a bit surprised to see a pointer to the base class, I was expecting this class to derive the \nother one (and just call the base class init() here). Why'd you choose to do it this way?\n> \n> \treturn 0;\n> }\n> @@ -128,10 +148,10 @@ void Awb::queueRequest(IPAContext &context,\n> \t\t * This will be fixed with the bayes AWB algorithm.\n> \t\t */\n> \t\tupdate = true;\n> -\t} else if (colourTemperature && colourGainCurve_) {\n> -\t\tconst auto &gains = colourGainCurve_->getInterpolated(*colourTemperature);\n> -\t\tawb.gains.manual.r() = gains[0];\n> -\t\tawb.gains.manual.b() = gains[1];\n> +\t} else if (colourTemperature) {\n> +\t\tconst auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);\n> +\t\tawb.gains.manual.r() = gains.r();\n> +\t\tawb.gains.manual.b() = gains.b();\n> \t\tawb.temperatureK = *colourTemperature;\n> \t\tupdate = true;\n> \t}\n> @@ -251,33 +271,33 @@ void Awb::process(IPAContext &context,\n> \t rgbMeans.b() < kMeanMinThreshold)\n> \t\treturn;\n> \n> -\tactiveState.awb.temperatureK = estimateCCT(rgbMeans);\n> +\t/*\n> +\t * \\Todo: Hardcode lux to a fixed value, until an estimation is\n> +\t * implemented.\n> +\t */\n> +\tint lux = 1000;\n> +\n> +\tRkISP1AwbStats awbStats{ rgbMeans };\n> +\tAwbResult r = awbAlgo_->calculateAwb(awbStats, lux);\nA more descriptive variable name would be slightly better I think; \"r\" in this context makes me \nthink \"red\" not \"results\".\n> +\n> +\tactiveState.awb.temperatureK = r.colourTemperature;\n> \n> \t/* Metadata shall contain the up to date measurement */\n> \tmetadata.set(controls::ColourTemperature, activeState.awb.temperatureK);\n> \n> -\t/*\n> -\t * Estimate the red and blue gains to apply in a grey world. The green\n> -\t * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the\n> -\t * divisor to a minimum value of 1.0.\n> -\t */\n> -\tRGB gains({ rgbMeans.g() / std::max(rgbMeans.r(), 1.0),\n> -\t\t\t 1.0,\n> -\t\t\t rgbMeans.g() / std::max(rgbMeans.b(), 1.0) });\n> -\n> \t/*\n> \t * Clamp the gain values to the hardware, which expresses gains as Q2.8\n> \t * unsigned integer values. Set the minimum just above zero to avoid\n> \t * divisions by zero when computing the raw means in subsequent\n> \t * iterations.\n> \t */\n> -\tgains = gains.max(1.0 / 256).min(1023.0 / 256);\n> +\tr.gains = r.gains.max(1.0 / 256).min(1023.0 / 256);\n> \n> \t/* Filter the values to avoid oscillations. */\n> \tdouble speed = 0.2;\n> -\tgains = gains * speed + activeState.awb.gains.automatic * (1 - speed);\n> +\tr.gains = r.gains * speed + activeState.awb.gains.automatic * (1 - speed);\n> \n> -\tactiveState.awb.gains.automatic = gains;\n> +\tactiveState.awb.gains.automatic = r.gains;\n> \n> \tLOG(RkISP1Awb, Debug)\n> \t\t<< std::showpoint\n> diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h\n> index 2a64cb60d5f4..3382b2178567 100644\n> --- a/src/ipa/rkisp1/algorithms/awb.h\n> +++ b/src/ipa/rkisp1/algorithms/awb.h\n> @@ -9,6 +9,7 @@\n> \n> #include \n> \n> +#include \"libipa/awb.h\"\n> #include \"libipa/interpolator.h\"\n> #include \"libipa/vector.h\"\n> \n> @@ -41,7 +42,8 @@ private:\n> \tRGB calculateRgbMeans(const IPAFrameContext &frameContext,\n> \t\t\t\t const rkisp1_cif_isp_awb_stat *awb) const;\n> \n> -\tstd::optional>> colourGainCurve_;\n> +\tstd::unique_ptr awbAlgo_;\n> +\n> \tbool rgbMode_;\n> };\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 3EB47C3273\n\tfor ;\n\tMon, 20 Jan 2025 12:00:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B8ECB6854B;\n\tMon, 20 Jan 2025 13:00:01 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AB38E60354\n\tfor ;\n\tMon, 20 Jan 2025 12:59:59 +0100 (CET)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4663D2E0\n\tfor ;\n\tMon, 20 Jan 2025 12:58:58 +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=\"rHy6lkOW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737374338;\n\tbh=m6She4+j9//UsKpK8GTVSCmnlnyQi4hioln6BuGeO4w=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=rHy6lkOW9ZMl9P8/8OH5GNdHTe4Czx3Mtrh4r1KskgWW0SyQIpdwOX6jtYShfMsCv\n\tTXc8suPEXbFUWyS15FSpPhzxz3xxu/JOndK7vJdejRj1bPeLwI2RaspLq7JqVaXnYP\n\t9jVzC+kH8RIf0ptHg09g73q+lv93CKBia5aqk9+o=","Message-ID":"","Date":"Mon, 20 Jan 2025 11:59:56 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 07/11] ipa: rkisp1: Use grey world algorithm from\n\tlibipa","To":"libcamera-devel@lists.libcamera.org","References":"<20250109115412.356768-1-stefan.klug@ideasonboard.com>\n\t<20250109115412.356768-8-stefan.klug@ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"<20250109115412.356768-8-stefan.klug@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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":33109,"web_url":"https://patchwork.libcamera.org/comment/33109/","msgid":"","date":"2025-01-21T06:08:12","subject":"Re: [PATCH v1 07/11] ipa: rkisp1: Use grey world algorithm from\n\tlibipa","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Dan,\n\nThank you for the review. \n\nOn Mon, Jan 20, 2025 at 11:59:56AM +0000, Dan Scally wrote:\n> Hi Stefan\n> \n> On 09/01/2025 11:53, Stefan Klug wrote:\n> > Now that libipa contains a grey world algorithm, use that.\n> > \n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/ipa/rkisp1/algorithms/awb.cpp | 72 ++++++++++++++++++++-----------\n> > src/ipa/rkisp1/algorithms/awb.h | 4 +-\n> > 2 files changed, 49 insertions(+), 27 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > index f6449565834b..42a4784998bc 100644\n> > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > @@ -16,6 +16,7 @@\n> > #include \n> > +#include \"libipa/awb_grey.h\"\n> > #include \"libipa/colours.h\"\n> > /**\n> > @@ -40,6 +41,28 @@ constexpr int32_t kDefaultColourTemperature = 5000;\n> > /* Minimum mean value below which AWB can't operate. */\n> > constexpr double kMeanMinThreshold = 2.0;\n> > +class RkISP1AwbStats : public AwbStats\n> > +{\n> > +public:\n> > +\tRkISP1AwbStats(const RGB &rgbMeans)\n> > +\t\t: rgbMeans_(rgbMeans) {}\n> > +\n> > +\tdouble computeColourError([[maybe_unused]] const RGB &gains) const override\n> > +\t{\n> > +\t\tLOG(RkISP1Awb, Error)\n> > +\t\t\t<< \"RkISP1AwbStats::computeColourError is not implemented\";\n> > +\t\treturn 0.0;\n> > +\t}\n> If it's optional, perhaps rather than a pure virtual function it should have\n> this \"not implemented\" variant in the base class?\n\nYes that would be possible. This was merely to get the patch series into\nlogical blocks. It is removed in patch 10/11. I believe the \"not\nimplemented\" shouldn't be a default case, so that the IPA implementation\nneeds to take active action if something should not be implemented.\n\n> > +\n> > +\tRGB getRGBMeans() const override\n> > +\t{\n> > +\t\treturn rgbMeans_;\n> > +\t};\n> > +\n> > +private:\n> > +\tRGB rgbMeans_;\n> > +};\n> > +\n> > Awb::Awb()\n> > \t: rgbMode_(false)\n> > {\n> > @@ -55,15 +78,12 @@ int Awb::init(IPAContext &context, const YamlObject &tuningData)\n> > \t\t\t\t\t\t\t kMaxColourTemperature,\n> > \t\t\t\t\t\t\t kDefaultColourTemperature);\n> > -\tInterpolator> gainCurve;\n> > -\tint ret = gainCurve.readYaml(tuningData[\"colourGains\"], \"ct\", \"gains\");\n> > -\tif (ret < 0)\n> > -\t\tLOG(RkISP1Awb, Warning)\n> > -\t\t\t<< \"Failed to parse 'colourGains' \"\n> > -\t\t\t<< \"parameter from tuning file; \"\n> > -\t\t\t<< \"manual colour temperature will not work properly\";\n> > -\telse\n> > -\t\tcolourGainCurve_ = gainCurve;\n> > +\tawbAlgo_ = std::make_unique();\n> > +\tint ret = awbAlgo_->init(tuningData);\n> > +\tif (ret) {\n> > +\t\tLOG(RkISP1Awb, Error) << \"Failed to init awb algorithm\";\n> > +\t\treturn ret;\n> > +\t}\n> I'm a bit surprised to see a pointer to the base class, I was expecting this\n> class to derive the other one (and just call the base class init() here).\n> Why'd you choose to do it this way?\n\nThe reason was that the top level (lets' call it RkISPAwb) algorithm\nshould support both cases grey world and bayes and be able to switch\nbased on the tuning file. I like the clear separation of these two\nalgorithms as it keeps the door open to easily improve by adding another\none without breaking the old ones.\n\nWe could add a \"master-AWB\" algorithm that contains the\nswitching/instantiation logic and add that to libipa. This could then be\nsubclassed by RkISPAwb. I don't know exactly if it is worth it. It\ndepends a bit on how we see libipa: a) the place to put some often used\nbuilding blocks for IPAs or b) the place where we put as much of the IPA\nlogic as we can so that all IPAs behave the same eventually. This\nimplements a) but b) could later be added on top. What do you think?\n\nCheers,\nStefan\n\n> > \treturn 0;\n> > }\n> > @@ -128,10 +148,10 @@ void Awb::queueRequest(IPAContext &context,\n> > \t\t * This will be fixed with the bayes AWB algorithm.\n> > \t\t */\n> > \t\tupdate = true;\n> > -\t} else if (colourTemperature && colourGainCurve_) {\n> > -\t\tconst auto &gains = colourGainCurve_->getInterpolated(*colourTemperature);\n> > -\t\tawb.gains.manual.r() = gains[0];\n> > -\t\tawb.gains.manual.b() = gains[1];\n> > +\t} else if (colourTemperature) {\n> > +\t\tconst auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);\n> > +\t\tawb.gains.manual.r() = gains.r();\n> > +\t\tawb.gains.manual.b() = gains.b();\n> > \t\tawb.temperatureK = *colourTemperature;\n> > \t\tupdate = true;\n> > \t}\n> > @@ -251,33 +271,33 @@ void Awb::process(IPAContext &context,\n> > \t rgbMeans.b() < kMeanMinThreshold)\n> > \t\treturn;\n> > -\tactiveState.awb.temperatureK = estimateCCT(rgbMeans);\n> > +\t/*\n> > +\t * \\Todo: Hardcode lux to a fixed value, until an estimation is\n> > +\t * implemented.\n> > +\t */\n> > +\tint lux = 1000;\n> > +\n> > +\tRkISP1AwbStats awbStats{ rgbMeans };\n> > +\tAwbResult r = awbAlgo_->calculateAwb(awbStats, lux);\n> A more descriptive variable name would be slightly better I think; \"r\" in\n> this context makes me think \"red\" not \"results\".\n> > +\n> > +\tactiveState.awb.temperatureK = r.colourTemperature;\n> > \t/* Metadata shall contain the up to date measurement */\n> > \tmetadata.set(controls::ColourTemperature, activeState.awb.temperatureK);\n> > -\t/*\n> > -\t * Estimate the red and blue gains to apply in a grey world. The green\n> > -\t * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the\n> > -\t * divisor to a minimum value of 1.0.\n> > -\t */\n> > -\tRGB gains({ rgbMeans.g() / std::max(rgbMeans.r(), 1.0),\n> > -\t\t\t 1.0,\n> > -\t\t\t rgbMeans.g() / std::max(rgbMeans.b(), 1.0) });\n> > -\n> > \t/*\n> > \t * Clamp the gain values to the hardware, which expresses gains as Q2.8\n> > \t * unsigned integer values. Set the minimum just above zero to avoid\n> > \t * divisions by zero when computing the raw means in subsequent\n> > \t * iterations.\n> > \t */\n> > -\tgains = gains.max(1.0 / 256).min(1023.0 / 256);\n> > +\tr.gains = r.gains.max(1.0 / 256).min(1023.0 / 256);\n> > \t/* Filter the values to avoid oscillations. */\n> > \tdouble speed = 0.2;\n> > -\tgains = gains * speed + activeState.awb.gains.automatic * (1 - speed);\n> > +\tr.gains = r.gains * speed + activeState.awb.gains.automatic * (1 - speed);\n> > -\tactiveState.awb.gains.automatic = gains;\n> > +\tactiveState.awb.gains.automatic = r.gains;\n> > \tLOG(RkISP1Awb, Debug)\n> > \t\t<< std::showpoint\n> > diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h\n> > index 2a64cb60d5f4..3382b2178567 100644\n> > --- a/src/ipa/rkisp1/algorithms/awb.h\n> > +++ b/src/ipa/rkisp1/algorithms/awb.h\n> > @@ -9,6 +9,7 @@\n> > #include \n> > +#include \"libipa/awb.h\"\n> > #include \"libipa/interpolator.h\"\n> > #include \"libipa/vector.h\"\n> > @@ -41,7 +42,8 @@ private:\n> > \tRGB calculateRgbMeans(const IPAFrameContext &frameContext,\n> > \t\t\t\t const rkisp1_cif_isp_awb_stat *awb) const;\n> > -\tstd::optional>> colourGainCurve_;\n> > +\tstd::unique_ptr awbAlgo_;\n> > +\n> > \tbool rgbMode_;\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 2FD60BD160\n\tfor ;\n\tTue, 21 Jan 2025 06:08:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 19A6C6851B;\n\tTue, 21 Jan 2025 07:08:19 +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 9BF4B6187A\n\tfor ;\n\tTue, 21 Jan 2025 07:08:16 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:6225:6f7e:a861:87ce])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1D79C788;\n\tTue, 21 Jan 2025 07:07:13 +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=\"PCrX6hdD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737439634;\n\tbh=geTgTSKcHbJea9uKPDzViFMdn1Ic9UZNoRQY0qqG4Pk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PCrX6hdDMgUlXLuwkYonjGcQklsS36gDpvKcyqxXxhe4Vdxr85elX9iAFpFa+XgZE\n\togsHYqRrBG72rkBstoibh+LPLlUHp38bpNOJtr0852iQUJ1s9Or6SQLUsdKSLATFMH\n\thYCCxASybwY6alge3s/kR/1f1Pk7JMPPEzRa1KPs=","Date":"Tue, 21 Jan 2025 07:08:12 +0100","From":"Stefan Klug ","To":"Dan Scally ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 07/11] ipa: rkisp1: Use grey world algorithm from\n\tlibipa","Message-ID":"","References":"<20250109115412.356768-1-stefan.klug@ideasonboard.com>\n\t<20250109115412.356768-8-stefan.klug@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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":33111,"web_url":"https://patchwork.libcamera.org/comment/33111/","msgid":"<578998e5-dc37-43ea-9334-3761ed700d39@ideasonboard.com>","date":"2025-01-21T09:30:46","subject":"Re: [PATCH v1 07/11] ipa: rkisp1: Use grey world algorithm from\n\tlibipa","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Morning Stefan\n\nOn 21/01/2025 06:08, Stefan Klug wrote:\n> Hi Dan,\n>\n> Thank you for the review.\n>\n> On Mon, Jan 20, 2025 at 11:59:56AM +0000, Dan Scally wrote:\n>> Hi Stefan\n>>\n>> On 09/01/2025 11:53, Stefan Klug wrote:\n>>> Now that libipa contains a grey world algorithm, use that.\n>>>\n>>> Signed-off-by: Stefan Klug \n>>> ---\n>>> src/ipa/rkisp1/algorithms/awb.cpp | 72 ++++++++++++++++++++-----------\n>>> src/ipa/rkisp1/algorithms/awb.h | 4 +-\n>>> 2 files changed, 49 insertions(+), 27 deletions(-)\n>>>\n>>> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n>>> index f6449565834b..42a4784998bc 100644\n>>> --- a/src/ipa/rkisp1/algorithms/awb.cpp\n>>> +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n>>> @@ -16,6 +16,7 @@\n>>> #include \n>>> +#include \"libipa/awb_grey.h\"\n>>> #include \"libipa/colours.h\"\n>>> /**\n>>> @@ -40,6 +41,28 @@ constexpr int32_t kDefaultColourTemperature = 5000;\n>>> /* Minimum mean value below which AWB can't operate. */\n>>> constexpr double kMeanMinThreshold = 2.0;\n>>> +class RkISP1AwbStats : public AwbStats\n>>> +{\n>>> +public:\n>>> +\tRkISP1AwbStats(const RGB &rgbMeans)\n>>> +\t\t: rgbMeans_(rgbMeans) {}\n>>> +\n>>> +\tdouble computeColourError([[maybe_unused]] const RGB &gains) const override\n>>> +\t{\n>>> +\t\tLOG(RkISP1Awb, Error)\n>>> +\t\t\t<< \"RkISP1AwbStats::computeColourError is not implemented\";\n>>> +\t\treturn 0.0;\n>>> +\t}\n>> If it's optional, perhaps rather than a pure virtual function it should have\n>> this \"not implemented\" variant in the base class?\n> Yes that would be possible. This was merely to get the patch series into\n> logical blocks. It is removed in patch 10/11. I believe the \"not\n> implemented\" shouldn't be a default case, so that the IPA implementation\n> needs to take active action if something should not be implemented.\nAlright, works for me\n>\n>>> +\n>>> +\tRGB getRGBMeans() const override\n>>> +\t{\n>>> +\t\treturn rgbMeans_;\n>>> +\t};\n>>> +\n>>> +private:\n>>> +\tRGB rgbMeans_;\n>>> +};\n>>> +\n>>> Awb::Awb()\n>>> \t: rgbMode_(false)\n>>> {\n>>> @@ -55,15 +78,12 @@ int Awb::init(IPAContext &context, const YamlObject &tuningData)\n>>> \t\t\t\t\t\t\t kMaxColourTemperature,\n>>> \t\t\t\t\t\t\t kDefaultColourTemperature);\n>>> -\tInterpolator> gainCurve;\n>>> -\tint ret = gainCurve.readYaml(tuningData[\"colourGains\"], \"ct\", \"gains\");\n>>> -\tif (ret < 0)\n>>> -\t\tLOG(RkISP1Awb, Warning)\n>>> -\t\t\t<< \"Failed to parse 'colourGains' \"\n>>> -\t\t\t<< \"parameter from tuning file; \"\n>>> -\t\t\t<< \"manual colour temperature will not work properly\";\n>>> -\telse\n>>> -\t\tcolourGainCurve_ = gainCurve;\n>>> +\tawbAlgo_ = std::make_unique();\n>>> +\tint ret = awbAlgo_->init(tuningData);\n>>> +\tif (ret) {\n>>> +\t\tLOG(RkISP1Awb, Error) << \"Failed to init awb algorithm\";\n>>> +\t\treturn ret;\n>>> +\t}\n>> I'm a bit surprised to see a pointer to the base class, I was expecting this\n>> class to derive the other one (and just call the base class init() here).\n>> Why'd you choose to do it this way?\n> The reason was that the top level (lets' call it RkISPAwb) algorithm\n> should support both cases grey world and bayes and be able to switch\n> based on the tuning file.\nYes I encountered that when I reached the later patches - maybe I should read the whole series \nbefore commenting on anything :D. It's different but better than how I was expecting it to work, so \nit's good.\n> I like the clear separation of these two\n> algorithms as it keeps the door open to easily improve by adding another\n> one without breaking the old ones.\n>\n> We could add a \"master-AWB\" algorithm that contains the\n> switching/instantiation logic and add that to libipa. This could then be\n> subclassed by RkISPAwb. I don't know exactly if it is worth it. It\n> depends a bit on how we see libipa: a) the place to put some often used\n> building blocks for IPAs or b) the place where we put as much of the IPA\n> logic as we can so that all IPAs behave the same eventually. This\n> implements a) but b) could later be added on top. What do you think?\n\nPersonally I think b is desirable, but I don't think it needs to be done right now - particularly \nsince no other IPA to my knowledge has multiple implementations of the same algorithm yet.\n\n\nSo, all good by me:\n\n\nReviewed-by: Daniel Scally \n\n>\n> Cheers,\n> Stefan\n>\n>>> \treturn 0;\n>>> }\n>>> @@ -128,10 +148,10 @@ void Awb::queueRequest(IPAContext &context,\n>>> \t\t * This will be fixed with the bayes AWB algorithm.\n>>> \t\t */\n>>> \t\tupdate = true;\n>>> -\t} else if (colourTemperature && colourGainCurve_) {\n>>> -\t\tconst auto &gains = colourGainCurve_->getInterpolated(*colourTemperature);\n>>> -\t\tawb.gains.manual.r() = gains[0];\n>>> -\t\tawb.gains.manual.b() = gains[1];\n>>> +\t} else if (colourTemperature) {\n>>> +\t\tconst auto &gains = awbAlgo_->gainsFromColourTemperature(*colourTemperature);\n>>> +\t\tawb.gains.manual.r() = gains.r();\n>>> +\t\tawb.gains.manual.b() = gains.b();\n>>> \t\tawb.temperatureK = *colourTemperature;\n>>> \t\tupdate = true;\n>>> \t}\n>>> @@ -251,33 +271,33 @@ void Awb::process(IPAContext &context,\n>>> \t rgbMeans.b() < kMeanMinThreshold)\n>>> \t\treturn;\n>>> -\tactiveState.awb.temperatureK = estimateCCT(rgbMeans);\n>>> +\t/*\n>>> +\t * \\Todo: Hardcode lux to a fixed value, until an estimation is\n>>> +\t * implemented.\n>>> +\t */\n>>> +\tint lux = 1000;\n>>> +\n>>> +\tRkISP1AwbStats awbStats{ rgbMeans };\n>>> +\tAwbResult r = awbAlgo_->calculateAwb(awbStats, lux);\n>> A more descriptive variable name would be slightly better I think; \"r\" in\n>> this context makes me think \"red\" not \"results\".\n>>> +\n>>> +\tactiveState.awb.temperatureK = r.colourTemperature;\n>>> \t/* Metadata shall contain the up to date measurement */\n>>> \tmetadata.set(controls::ColourTemperature, activeState.awb.temperatureK);\n>>> -\t/*\n>>> -\t * Estimate the red and blue gains to apply in a grey world. The green\n>>> -\t * gain is hardcoded to 1.0. Avoid divisions by zero by clamping the\n>>> -\t * divisor to a minimum value of 1.0.\n>>> -\t */\n>>> -\tRGB gains({ rgbMeans.g() / std::max(rgbMeans.r(), 1.0),\n>>> -\t\t\t 1.0,\n>>> -\t\t\t rgbMeans.g() / std::max(rgbMeans.b(), 1.0) });\n>>> -\n>>> \t/*\n>>> \t * Clamp the gain values to the hardware, which expresses gains as Q2.8\n>>> \t * unsigned integer values. Set the minimum just above zero to avoid\n>>> \t * divisions by zero when computing the raw means in subsequent\n>>> \t * iterations.\n>>> \t */\n>>> -\tgains = gains.max(1.0 / 256).min(1023.0 / 256);\n>>> +\tr.gains = r.gains.max(1.0 / 256).min(1023.0 / 256);\n>>> \t/* Filter the values to avoid oscillations. */\n>>> \tdouble speed = 0.2;\n>>> -\tgains = gains * speed + activeState.awb.gains.automatic * (1 - speed);\n>>> +\tr.gains = r.gains * speed + activeState.awb.gains.automatic * (1 - speed);\n>>> -\tactiveState.awb.gains.automatic = gains;\n>>> +\tactiveState.awb.gains.automatic = r.gains;\n>>> \tLOG(RkISP1Awb, Debug)\n>>> \t\t<< std::showpoint\n>>> diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h\n>>> index 2a64cb60d5f4..3382b2178567 100644\n>>> --- a/src/ipa/rkisp1/algorithms/awb.h\n>>> +++ b/src/ipa/rkisp1/algorithms/awb.h\n>>> @@ -9,6 +9,7 @@\n>>> #include \n>>> +#include \"libipa/awb.h\"\n>>> #include \"libipa/interpolator.h\"\n>>> #include \"libipa/vector.h\"\n>>> @@ -41,7 +42,8 @@ private:\n>>> \tRGB calculateRgbMeans(const IPAFrameContext &frameContext,\n>>> \t\t\t\t const rkisp1_cif_isp_awb_stat *awb) const;\n>>> -\tstd::optional>> colourGainCurve_;\n>>> +\tstd::unique_ptr awbAlgo_;\n>>> +\n>>> \tbool rgbMode_;\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 A8078BD160\n\tfor ;\n\tTue, 21 Jan 2025 09:30:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A550C68543;\n\tTue, 21 Jan 2025 10:30:51 +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 1DF4968516\n\tfor ;\n\tTue, 21 Jan 2025 10:30:50 +0100 (CET)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C3E0E55A;\n\tTue, 21 Jan 2025 10:29:47 +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=\"fO3l1BfB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737451788;\n\tbh=xeedeJXjU6Q5nGGKZGv3ASz3vGoXwI7ik5xwzOr5y9w=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=fO3l1BfBpwFvh1QWV0/avISvGnrlGtV1xaqd503pzl3Vq45eF52rCBNdp499xNloR\n\tISpwGBMEbM7oU2hYEgQiwJk2rOvRsfXB0L3tkym69lMHgSA2QEJaBK4SoQgttFvf5e\n\tOuIcLsHkHyLJ5liPDHowJTf+KDgxn6r+cN52ebww=","Message-ID":"<578998e5-dc37-43ea-9334-3761ed700d39@ideasonboard.com>","Date":"Tue, 21 Jan 2025 09:30:46 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 07/11] ipa: rkisp1: Use grey world algorithm from\n\tlibipa","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250109115412.356768-1-stefan.klug@ideasonboard.com>\n\t<20250109115412.356768-8-stefan.klug@ideasonboard.com>\n\t\n\t","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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\" "}}]