[{"id":18614,"web_url":"https://patchwork.libcamera.org/comment/18614/","msgid":"","date":"2021-08-09T10:20:53","subject":"Re: [libcamera-devel] [RFC PATCH 4/5] ipa: ipu3: Move IPU3 awb into\n\talgorithms","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi JM\n\nThank you for the patch\n\nOn 8/9/21 2:50 PM, Jean-Michel Hautbois wrote:\n> Use the Context class and algorithm interface to properly call the AWB\n> algorithm from IPAIPU3.\n> We need to pass the BDS grid size calculated, pass those through the Context\n> class in a dedicated Awb structure.\n>\n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> .../ipu3/{ipu3_awb.cpp => algorithms/awb.cpp} | 42 ++++++++++++-------\n> src/ipa/ipu3/{ipu3_awb.h => algorithms/awb.h} | 29 ++++++-------\n> src/ipa/ipu3/algorithms/meson.build | 1 +\n> src/ipa/ipu3/ipa_context.h | 12 ++++++\n> src/ipa/ipu3/ipu3.cpp | 30 ++++++-------\n> src/ipa/ipu3/meson.build | 1 -\n> 6 files changed, 66 insertions(+), 49 deletions(-)\n> rename src/ipa/ipu3/{ipu3_awb.cpp => algorithms/awb.cpp} (92%)\n> rename src/ipa/ipu3/{ipu3_awb.h => algorithms/awb.h} (79%)\n>\n> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> similarity index 92%\n> rename from src/ipa/ipu3/ipu3_awb.cpp\n> rename to src/ipa/ipu3/algorithms/awb.cpp\n> index 043c3838..92a41b65 100644\n> --- a/src/ipa/ipu3/ipu3_awb.cpp\n> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> @@ -2,9 +2,9 @@\n> /*\n> * Copyright (C) 2021, Ideas On Board\n> *\n> - * ipu3_awb.cpp - AWB control algorithm\n> + * awb.cpp - AWB control algorithm\n> */\n> -#include \"ipu3_awb.h\"\n> +#include \"awb.h\"\n> \n> #include \n> #include \n> @@ -14,7 +14,7 @@\n> \n> namespace libcamera {\n> \n> -namespace ipa::ipu3 {\n> +namespace ipa::ipu3::algorithms {\n> \n> LOG_DEFINE_CATEGORY(IPU3Awb)\n> \n> @@ -134,7 +134,7 @@ static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {\n> \t0, 0, 8191, 0\n> };\n> \n> -IPU3Awb::IPU3Awb()\n> +Awb::Awb()\n> \t: Algorithm()\n> {\n> \tasyncResults_.blueGain = 1.0;\n> @@ -143,17 +143,19 @@ IPU3Awb::IPU3Awb()\n> \tasyncResults_.temperatureK = 4500;\n> }\n> \n> -IPU3Awb::~IPU3Awb()\n> +Awb::~Awb()\n> {\n> }\n> \n> -void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid)\n> +int Awb::configure(IPAContext &context)\nMapping old Initialise() directly to Algorithm::configure() ? If there \nis a reason, I would expect something about it in the commit message maybe?\n> {\n> +\tipu3_uapi_params ¶ms = context.params;\n> +\tSize &bdsOutputSize = context.awb.grid.bdsOutputSize;\n> +\n> \tparams.use.acc_awb = 1;\n> \tparams.acc_param.awb.config = imguCssAwbDefaults;\n> \n> -\tawbGrid_ = bdsGrid;\n> -\tparams.acc_param.awb.config.grid = awbGrid_;\n> +\tawbGrid_ = context.awb.grid.bdsGrid;\n> \n> \tparams.use.acc_bnr = 1;\n> \tparams.acc_param.bnr = imguCssBnrDefaults;\n> @@ -174,6 +176,8 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st\n> \tparams.acc_param.ccm = imguCssCcmDefault;\n> \n> \tzones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);\n> +\n> +\treturn 0;\n> }\n> \n> /**\n> @@ -190,7 +194,7 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st\n> * More detailed information can be found in:\n> * https://en.wikipedia.org/wiki/Color_temperature#Approximation\n> */\n> -uint32_t IPU3Awb::estimateCCT(double red, double green, double blue)\n> +uint32_t Awb::estimateCCT(double red, double green, double blue)\n> {\n> \t/* Convert the RGB values to CIE tristimulus values (XYZ) */\n> \tdouble X = (-0.14282) * (red) + (1.54924) * (green) + (-0.95641) * (blue);\n> @@ -207,7 +211,7 @@ uint32_t IPU3Awb::estimateCCT(double red, double green, double blue)\n> }\n> \n> /* Generate an RGB vector with the average values for each region */\n> -void IPU3Awb::generateZones(std::vector &zones)\n> +void Awb::generateZones(std::vector &zones)\n> {\n> \tfor (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {\n> \t\tRGB zone;\n> @@ -224,7 +228,7 @@ void IPU3Awb::generateZones(std::vector &zones)\n> }\n> \n> /* Translate the IPU3 statistics into the default statistics region array */\n> -void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)\n> +void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)\n> {\n> \tuint32_t regionWidth = round(awbGrid_.width / static_cast(kAwbStatsSizeX));\n> \tuint32_t regionHeight = round(awbGrid_.height / static_cast(kAwbStatsSizeY));\n> @@ -256,7 +260,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)\n> \t}\n> }\n> \n> -void IPU3Awb::clearAwbStats()\n> +void Awb::clearAwbStats()\n> {\n> \tfor (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {\n> \t\tawbStats_[i].bSum = 0;\n> @@ -267,7 +271,7 @@ void IPU3Awb::clearAwbStats()\n> \t}\n> }\n> \n> -void IPU3Awb::awbGreyWorld()\n> +void Awb::awbGreyWorld()\n> {\n> \tLOG(IPU3Awb, Debug) << \"Grey world AWB\";\n> \t/*\n> @@ -307,7 +311,7 @@ void IPU3Awb::awbGreyWorld()\n> \tasyncResults_.blueGain = blueGain;\n> }\n> \n> -void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)\n> +void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)\n> {\n> \tASSERT(stats->stats_3a_status.awb_en);\n> \tzones_.clear();\n> @@ -322,7 +326,7 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)\n> \t}\n> }\n> \n> -void IPU3Awb::updateWbParameters(ipu3_uapi_params ¶ms)\n> +void Awb::updateWbParameters(ipu3_uapi_params ¶ms)\n> {\n> \t/*\n> \t * Green gains should not be touched and considered 1.\n> @@ -340,6 +344,12 @@ void IPU3Awb::updateWbParameters(ipu3_uapi_params ¶ms)\n> \tparams.acc_param.ccm = imguCssCcmDefault;\n> }\n> \n> -} /* namespace ipa::ipu3 */\n> +void Awb::process(IPAContext &context)\n> +{\n> +\tcalculateWBGains(context.stats);\n> +\tupdateWbParameters(context.params);\n> +}\n> +\n> +} /* namespace ipa::ipu3::algorithms */\n> \n> } /* namespace libcamera */\n> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/algorithms/awb.h\n> similarity index 79%\n> rename from src/ipa/ipu3/ipu3_awb.h\n> rename to src/ipa/ipu3/algorithms/awb.h\n> index 8b05ac03..37690b37 100644\n> --- a/src/ipa/ipu3/ipu3_awb.h\n> +++ b/src/ipa/ipu3/algorithms/awb.h\n> @@ -2,10 +2,10 @@\n> /*\n> * Copyright (C) 2021, Ideas On Board\n> *\n> - * ipu3_awb.h - IPU3 AWB control algorithm\n> + * awb.h - IPU3 AWB control algorithm\n> */\n> -#ifndef __LIBCAMERA_IPU3_AWB_H__\n> -#define __LIBCAMERA_IPU3_AWB_H__\n> +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_AWB_H__\n> +#define __LIBCAMERA_IPU3_ALGORITHMS_AWB_H__\n> \n> #include \n> \n> @@ -13,26 +13,24 @@\n> \n> #include \n> \n> -#include \"algorithms/algorithm.h\"\n> -#include \"libipa/algorithm.h\"\n> +#include \"algorithm.h\"\n> \n> namespace libcamera {\n> \n> -namespace ipa::ipu3 {\n> +namespace ipa::ipu3::algorithms {\n> \n> /* Region size for the statistics generation algorithm */\n> static constexpr uint32_t kAwbStatsSizeX = 16;\n> static constexpr uint32_t kAwbStatsSizeY = 12;\n> \n> -class IPU3Awb : public ipa::Algorithm\n> +class Awb : public Algorithm\n> {\n> public:\n> -\tIPU3Awb();\n> -\t~IPU3Awb();\n> +\tAwb();\n> +\t~Awb();\n> \n> -\tvoid initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);\n> -\tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats);\n> -\tvoid updateWbParameters(ipu3_uapi_params ¶ms);\n> +\tint configure(IPAContext &context) override;\n> +\tvoid process(IPAContext &context) override;\nhmm, no initialise() here? Doesn't seem sane to have a direct call to \nconfigure(). I wonder, as in the last patch where there was only \nInitialise() and no over-ridden configure()/process().\n> \n> \tstruct Ipu3AwbCell {\n> \t\tunsigned char greenRedAvg;\n> @@ -73,6 +71,9 @@ public:\n> \t};\n> \n> private:\n> +\tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats);\n> +\tvoid updateWbParameters(ipu3_uapi_params ¶ms);\n> +\n> \tvoid generateZones(std::vector &zones);\n> \tvoid generateAwbStats(const ipu3_uapi_stats_3a *stats);\n> \tvoid clearAwbStats();\n> @@ -86,7 +87,7 @@ private:\n> \tAwbStatus asyncResults_;\n> };\n> \n> -} /* namespace ipa::ipu3 */\n> +} /* namespace ipa::ipu3::algorithms */\n> \n> } /* namespace libcamera*/\n> -#endif /* __LIBCAMERA_IPU3_AWB_H__ */\n> +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_AWB_H__ */\n> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build\n> index f71d1e61..df36d719 100644\n> --- a/src/ipa/ipu3/algorithms/meson.build\n> +++ b/src/ipa/ipu3/algorithms/meson.build\n> @@ -1,5 +1,6 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> ipu3_ipa_algorithms = files([\n> + 'awb.cpp',\n> 'contrast.cpp',\n> ])\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index 5d717be5..c43b275b 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -11,6 +11,8 @@\n> \n> #include \n> \n> +#include \n> +\n> namespace libcamera {\n> \n> namespace ipa::ipu3 {\n> @@ -21,6 +23,16 @@ struct IPAContext {\n> \n> \t/* Output Parameters which will be written to the hardware */\n> \tipu3_uapi_params params;\n> +\n> +\t/* AWB specific parameters to share */\n> +\tstruct Awb {\n> +\t\tstruct Grid {\n> +\t\t\t/* BDS grid plane config used by the kernel */\n> +\t\t\tipu3_uapi_grid_config bdsGrid;\n> +\t\t\t/* BDS output size configured by the pipeline handler */\n> +\t\t\tSize bdsOutputSize;\n> +\t\t} grid;\n> +\t} awb;\n> };\n> \n> } /* namespace ipa::ipu3 */\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 7035802f..82506461 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -23,11 +23,11 @@\n> #include \"libcamera/internal/framebuffer.h\"\n> \n> #include \"algorithms/algorithm.h\"\n> +#include \"algorithms/awb.h\"\n> #include \"algorithms/contrast.h\"\n> #include \"ipa_context.h\"\n> \n> #include \"ipu3_agc.h\"\n> -#include \"ipu3_awb.h\"\n> #include \"libipa/camera_sensor_helper.h\"\n> \n> static constexpr uint32_t kMaxCellWidthPerSet = 160;\n> @@ -81,8 +81,6 @@ private:\n> \tuint32_t minGain_;\n> \tuint32_t maxGain_;\n> \n> -\t/* Interface to the AWB algorithm */\n> -\tstd::unique_ptr awbAlgo_;\n> \t/* Interface to the AEC/AGC algorithm */\n> \tstd::unique_ptr agcAlgo_;\n> \t/* Interface to the Camera Helper */\n> @@ -105,6 +103,7 @@ int IPAIPU3::init(const IPASettings &settings)\n> \t}\n> \n> \t/* Construct our Algorithms */\n> +\talgorithms_.emplace_back(new algorithms::Awb());\n> \talgorithms_.emplace_back(new algorithms::Contrast());\n> \n> \t/* Reset all the hardware settings */\n> @@ -138,7 +137,7 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n> \tuint32_t minError = std::numeric_limits::max();\n> \tSize best;\n> \tSize bestLog2;\n> -\tbdsGrid_ = {};\n> +\tipu3_uapi_grid_config &bdsGrid = context_.awb.grid.bdsGrid;\n> \n> \tfor (uint32_t widthShift = 3; widthShift <= 7; ++widthShift) {\n> \t\tuint32_t width = std::min(kMaxCellWidthPerSet,\n> @@ -162,14 +161,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n> \t\t}\n> \t}\n> \n> -\tbdsGrid_.width = best.width >> bestLog2.width;\n> -\tbdsGrid_.block_width_log2 = bestLog2.width;\n> -\tbdsGrid_.height = best.height >> bestLog2.height;\n> -\tbdsGrid_.block_height_log2 = bestLog2.height;\n> +\tbdsGrid.width = best.width >> bestLog2.width;\n> +\tbdsGrid.block_width_log2 = bestLog2.width;\n> +\tbdsGrid.height = best.height >> bestLog2.height;\n> +\tbdsGrid.block_height_log2 = bestLog2.height;\n> \n> \tLOG(IPAIPU3, Debug) << \"Best grid found is: (\"\n> -\t\t\t << (int)bdsGrid_.width << \" << \" << (int)bdsGrid_.block_width_log2 << \") x (\"\n> -\t\t\t << (int)bdsGrid_.height << \" << \" << (int)bdsGrid_.block_height_log2 << \")\";\n> +\t\t\t << (int)bdsGrid.width << \" << \" << (int)bdsGrid.block_width_log2 << \") x (\"\n> +\t\t\t << (int)bdsGrid.height << \" << \" << (int)bdsGrid.block_height_log2 << \")\";\n> }\n> \n> int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> @@ -211,13 +210,13 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> \n> \tdefVBlank_ = itVBlank->second.def().get();\n> \n> +\t/* Prepare AWB parameters */\n> \tcalculateBdsGrid(configInfo.bdsOutputSize);\n> +\tcontext_.awb.grid.bdsOutputSize = configInfo.bdsOutputSize;\n> \n> \tconfigureAlgorithms();\n> \n> -\tawbAlgo_ = std::make_unique();\n> -\tawbAlgo_->initialise(context_.params, configInfo.bdsOutputSize, bdsGrid_);\n> -\n> +\tbdsGrid_ = context_.awb.grid.bdsGrid;\n> \tagcAlgo_ = std::make_unique();\n> \tagcAlgo_->initialise(bdsGrid_, sensorInfo_);\n> \n> @@ -293,9 +292,6 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> \n> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> {\n> -\tif (agcAlgo_->updateControls())\n> -\t\tawbAlgo_->updateWbParameters(context_.params);\n> -\n> \t*params = context_.params;\n> \n> \tIPU3Action op;\n> @@ -349,8 +345,6 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> \tagcAlgo_->process(stats, exposure_, gain);\n> \tgain_ = camHelper_->gainCode(gain);\n> \n> -\tawbAlgo_->calculateWBGains(stats);\n> -\n> \tif (agcAlgo_->updateControls())\n> \t\tsetControls(frame);\n> \n> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build\n> index fcb27d68..d1126947 100644\n> --- a/src/ipa/ipu3/meson.build\n> +++ b/src/ipa/ipu3/meson.build\n> @@ -7,7 +7,6 @@ ipa_name = 'ipa_ipu3'\n> ipu3_ipa_sources = files([\n> 'ipu3.cpp',\n> 'ipu3_agc.cpp',\n> - 'ipu3_awb.cpp',\n> ])\n> \n> ipu3_ipa_sources += ipu3_ipa_algorithms","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 51D4ABD87D\n\tfor ;\n\tMon, 9 Aug 2021 10:21:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 909A168826;\n\tMon, 9 Aug 2021 12:21:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E034E60269\n\tfor ;\n\tMon, 9 Aug 2021 12:20:58 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.61])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BD531466;\n\tMon, 9 Aug 2021 12:20:57 +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=\"XPIANI6h\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628504458;\n\tbh=z1nX54WRsl7Dcu+4BKrW1oyrcrgPFXHgmW/ALdmvc+s=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=XPIANI6hL9JN2IV0cw3MN9MBkom7vUeUEmCNuB2uO0PX5O4LITx8v+Z1uuzEgeP7l\n\t7xZRw1tyok/7XBAUQ9OOaIW2z6nKidOBLPaMSDDMKINY11zAt0NJ9vrCYv40PffDhg\n\tiN+lGpAiTJ6f6RLCkeGPOo1PpyzgnrQHcsnQRfNI=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210809092007.79067-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210809092007.79067-5-jeanmichel.hautbois@ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Mon, 9 Aug 2021 15:50:53 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210809092007.79067-5-jeanmichel.hautbois@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC PATCH 4/5] ipa: ipu3: Move IPU3 awb into\n\talgorithms","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":18615,"web_url":"https://patchwork.libcamera.org/comment/18615/","msgid":"<957004a2-1553-e742-e998-0991e8549788@ideasonboard.com>","date":"2021-08-09T10:22:52","subject":"Re: [libcamera-devel] [RFC PATCH 4/5] ipa: ipu3: Move IPU3 awb into\n\talgorithms","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 09/08/2021 10:20, Jean-Michel Hautbois wrote:\n> Use the Context class and algorithm interface to properly call the AWB\n\ns/Context/IPAContext/\ns/algorithm/Algorithm/\n\nand instead of 'properly call' I'd say \"to call the AWB algorithm using\nthe new modular interface\"\n\n\n> algorithm from IPAIPU3.\n> We need to pass the BDS grid size calculated, pass those through the Context\n> class in a dedicated Awb structure.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> .../ipu3/{ipu3_awb.cpp => algorithms/awb.cpp} | 42 ++++++++++++-------\n> src/ipa/ipu3/{ipu3_awb.h => algorithms/awb.h} | 29 ++++++-------\n> src/ipa/ipu3/algorithms/meson.build | 1 +\n> src/ipa/ipu3/ipa_context.h | 12 ++++++\n> src/ipa/ipu3/ipu3.cpp | 30 ++++++-------\n> src/ipa/ipu3/meson.build | 1 -\n> 6 files changed, 66 insertions(+), 49 deletions(-)\n> rename src/ipa/ipu3/{ipu3_awb.cpp => algorithms/awb.cpp} (92%)\n> rename src/ipa/ipu3/{ipu3_awb.h => algorithms/awb.h} (79%)\n> \n> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> similarity index 92%\n> rename from src/ipa/ipu3/ipu3_awb.cpp\n> rename to src/ipa/ipu3/algorithms/awb.cpp\n> index 043c3838..92a41b65 100644\n> --- a/src/ipa/ipu3/ipu3_awb.cpp\n> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> @@ -2,9 +2,9 @@\n> /*\n> * Copyright (C) 2021, Ideas On Board\n> *\n> - * ipu3_awb.cpp - AWB control algorithm\n> + * awb.cpp - AWB control algorithm\n> */\n> -#include \"ipu3_awb.h\"\n> +#include \"awb.h\"\n> \n> #include \n> #include \n> @@ -14,7 +14,7 @@\n> \n> namespace libcamera {\n> \n> -namespace ipa::ipu3 {\n> +namespace ipa::ipu3::algorithms {\n> \n> LOG_DEFINE_CATEGORY(IPU3Awb)\n> \n> @@ -134,7 +134,7 @@ static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {\n> \t0, 0, 8191, 0\n> };\n> \n> -IPU3Awb::IPU3Awb()\n> +Awb::Awb()\n> \t: Algorithm()\n> {\n> \tasyncResults_.blueGain = 1.0;\n> @@ -143,17 +143,19 @@ IPU3Awb::IPU3Awb()\n> \tasyncResults_.temperatureK = 4500;\n> }\n> \n> -IPU3Awb::~IPU3Awb()\n> +Awb::~Awb()\n> {\n> }\n> \n> -void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid)\n> +int Awb::configure(IPAContext &context)\n> {\n> +\tipu3_uapi_params ¶ms = context.params;\n> +\tSize &bdsOutputSize = context.awb.grid.bdsOutputSize;\n> +\n\nWe'll need to give more thought as to what parameters are coming in as\nConfiguration parameters, and they might get passed in (perhaps as a\nseparate structure, or perhaps still using the IPAContext).\n\nBut we've already discussed whether the Grid configuration would be\nsomething calculated by the top level ipu3.cpp and passed in, or if it\nshould be it's own 'algorithm/module' and handled as part of the\nprocessing chain.\n\nBut for now, I think this is fine.\n\n\n\n> \tparams.use.acc_awb = 1;\n> \tparams.acc_param.awb.config = imguCssAwbDefaults;\n> \n> -\tawbGrid_ = bdsGrid;\n> -\tparams.acc_param.awb.config.grid = awbGrid_;\n> +\tawbGrid_ = context.awb.grid.bdsGrid;\n\ndoes this Awb class still need a local copy of the grid in awbGrid_ ?\n\nPerhaps it does if it's something that will be passed in, and\npotentially not available in the IPAContext at process() time ... but at\nthe moment, it 'could' always get it from the Context ...\n\n> \n> \tparams.use.acc_bnr = 1;\n> \tparams.acc_param.bnr = imguCssBnrDefaults;\n> @@ -174,6 +176,8 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st\n> \tparams.acc_param.ccm = imguCssCcmDefault;\n> \n> \tzones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);\n> +\n> +\treturn 0;\n> }\n> \n> /**\n> @@ -190,7 +194,7 @@ void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, st\n> * More detailed information can be found in:\n> * https://en.wikipedia.org/wiki/Color_temperature#Approximation\n> */\n> -uint32_t IPU3Awb::estimateCCT(double red, double green, double blue)\n> +uint32_t Awb::estimateCCT(double red, double green, double blue)\n> {\n> \t/* Convert the RGB values to CIE tristimulus values (XYZ) */\n> \tdouble X = (-0.14282) * (red) + (1.54924) * (green) + (-0.95641) * (blue);\n> @@ -207,7 +211,7 @@ uint32_t IPU3Awb::estimateCCT(double red, double green, double blue)\n> }\n> \n> /* Generate an RGB vector with the average values for each region */\n> -void IPU3Awb::generateZones(std::vector &zones)\n> +void Awb::generateZones(std::vector &zones)\n> {\n> \tfor (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {\n> \t\tRGB zone;\n> @@ -224,7 +228,7 @@ void IPU3Awb::generateZones(std::vector &zones)\n> }\n> \n> /* Translate the IPU3 statistics into the default statistics region array */\n> -void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)\n> +void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)\n> {\n> \tuint32_t regionWidth = round(awbGrid_.width / static_cast(kAwbStatsSizeX));\n> \tuint32_t regionHeight = round(awbGrid_.height / static_cast(kAwbStatsSizeY));\n> @@ -256,7 +260,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)\n> \t}\n> }\n> \n> -void IPU3Awb::clearAwbStats()\n> +void Awb::clearAwbStats()\n> {\n> \tfor (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {\n> \t\tawbStats_[i].bSum = 0;\n> @@ -267,7 +271,7 @@ void IPU3Awb::clearAwbStats()\n> \t}\n> }\n> \n> -void IPU3Awb::awbGreyWorld()\n> +void Awb::awbGreyWorld()\n> {\n> \tLOG(IPU3Awb, Debug) << \"Grey world AWB\";\n> \t/*\n> @@ -307,7 +311,7 @@ void IPU3Awb::awbGreyWorld()\n> \tasyncResults_.blueGain = blueGain;\n> }\n> \n> -void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)\n> +void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)\n> {\n> \tASSERT(stats->stats_3a_status.awb_en);\n> \tzones_.clear();\n> @@ -322,7 +326,7 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)\n> \t}\n> }\n> \n> -void IPU3Awb::updateWbParameters(ipu3_uapi_params ¶ms)\n> +void Awb::updateWbParameters(ipu3_uapi_params ¶ms)\n> {\n> \t/*\n> \t * Green gains should not be touched and considered 1.\n> @@ -340,6 +344,12 @@ void IPU3Awb::updateWbParameters(ipu3_uapi_params ¶ms)\n> \tparams.acc_param.ccm = imguCssCcmDefault;\n> }\n> \n> -} /* namespace ipa::ipu3 */\n> +void Awb::process(IPAContext &context)\n> +{\n> +\tcalculateWBGains(context.stats);\n> +\tupdateWbParameters(context.params);\n> +}\n> +\n> +} /* namespace ipa::ipu3::algorithms */\n> \n> } /* namespace libcamera */\n> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/algorithms/awb.h\n> similarity index 79%\n> rename from src/ipa/ipu3/ipu3_awb.h\n> rename to src/ipa/ipu3/algorithms/awb.h\n> index 8b05ac03..37690b37 100644\n> --- a/src/ipa/ipu3/ipu3_awb.h\n> +++ b/src/ipa/ipu3/algorithms/awb.h\n> @@ -2,10 +2,10 @@\n> /*\n> * Copyright (C) 2021, Ideas On Board\n> *\n> - * ipu3_awb.h - IPU3 AWB control algorithm\n> + * awb.h - IPU3 AWB control algorithm\n> */\n> -#ifndef __LIBCAMERA_IPU3_AWB_H__\n> -#define __LIBCAMERA_IPU3_AWB_H__\n> +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_AWB_H__\n> +#define __LIBCAMERA_IPU3_ALGORITHMS_AWB_H__\n> \n> #include \n> \n> @@ -13,26 +13,24 @@\n> \n> #include \n> \n> -#include \"algorithms/algorithm.h\"\n> -#include \"libipa/algorithm.h\"\n> +#include \"algorithm.h\"\n> \n> namespace libcamera {\n> \n> -namespace ipa::ipu3 {\n> +namespace ipa::ipu3::algorithms {\n> \n> /* Region size for the statistics generation algorithm */\n> static constexpr uint32_t kAwbStatsSizeX = 16;\n> static constexpr uint32_t kAwbStatsSizeY = 12;\n> \n> -class IPU3Awb : public ipa::Algorithm\n> +class Awb : public Algorithm\n> {\n> public:\n> -\tIPU3Awb();\n> -\t~IPU3Awb();\n> +\tAwb();\n> +\t~Awb();\n> \n> -\tvoid initialise(ipu3_uapi_params ¶ms, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);\n> -\tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats);\n> -\tvoid updateWbParameters(ipu3_uapi_params ¶ms);\n> +\tint configure(IPAContext &context) override;\n> +\tvoid process(IPAContext &context) override;\n> \n> \tstruct Ipu3AwbCell {\n> \t\tunsigned char greenRedAvg;\n> @@ -73,6 +71,9 @@ public:\n> \t};\n> \n> private:\n> +\tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats);\n> +\tvoid updateWbParameters(ipu3_uapi_params ¶ms);\n> +\n> \tvoid generateZones(std::vector &zones);\n> \tvoid generateAwbStats(const ipu3_uapi_stats_3a *stats);\n> \tvoid clearAwbStats();\n> @@ -86,7 +87,7 @@ private:\n> \tAwbStatus asyncResults_;\n> };\n> \n> -} /* namespace ipa::ipu3 */\n> +} /* namespace ipa::ipu3::algorithms */\n> \n> } /* namespace libcamera*/\n> -#endif /* __LIBCAMERA_IPU3_AWB_H__ */\n> +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_AWB_H__ */\n> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build\n> index f71d1e61..df36d719 100644\n> --- a/src/ipa/ipu3/algorithms/meson.build\n> +++ b/src/ipa/ipu3/algorithms/meson.build\n> @@ -1,5 +1,6 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> ipu3_ipa_algorithms = files([\n> + 'awb.cpp',\n> 'contrast.cpp',\n> ])\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index 5d717be5..c43b275b 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -11,6 +11,8 @@\n> \n> #include \n> \n> +#include \n> +\n> namespace libcamera {\n> \n> namespace ipa::ipu3 {\n> @@ -21,6 +23,16 @@ struct IPAContext {\n> \n> \t/* Output Parameters which will be written to the hardware */\n> \tipu3_uapi_params params;\n> +\n> +\t/* AWB specific parameters to share */\n> +\tstruct Awb {\n> +\t\tstruct Grid {\n> +\t\t\t/* BDS grid plane config used by the kernel */\n> +\t\t\tipu3_uapi_grid_config bdsGrid;\n> +\t\t\t/* BDS output size configured by the pipeline handler */\n> +\t\t\tSize bdsOutputSize;\n> +\t\t} grid;\n\nDiscussed off-list, but re-discussing here for the list:\n\nI think the BDS grid sounds like it's own module. It references 'BDS'\nwhich is a distinct entity on the IPU3, and it does a calculation at\nconfigure time. The Grid isn't a property of the AWB, but something that\nit uses, (as does the AGC, I believe).\n\nSo I'd expect to see this stored outside of the Awb context structure.\nHowever - As this is currently calculated /inside/ the Awb algorithm\nmodule - I'm fine with keeping it here to show that.\n\nIf or rather when, the grid calculation gets moved out - I would expect\nthat this will likely move out of this structure and to a structure that\nmatches the component which generates it.\n\n\n\n> +\t} awb;\n> };\n> \n> } /* namespace ipa::ipu3 */\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 7035802f..82506461 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -23,11 +23,11 @@\n> #include \"libcamera/internal/framebuffer.h\"\n> \n> #include \"algorithms/algorithm.h\"\n> +#include \"algorithms/awb.h\"\n> #include \"algorithms/contrast.h\"\n> #include \"ipa_context.h\"\n> \n> #include \"ipu3_agc.h\"\n> -#include \"ipu3_awb.h\"\n> #include \"libipa/camera_sensor_helper.h\"\n> \n> static constexpr uint32_t kMaxCellWidthPerSet = 160;\n> @@ -81,8 +81,6 @@ private:\n> \tuint32_t minGain_;\n> \tuint32_t maxGain_;\n> \n> -\t/* Interface to the AWB algorithm */\n> -\tstd::unique_ptr awbAlgo_;\n> \t/* Interface to the AEC/AGC algorithm */\n> \tstd::unique_ptr agcAlgo_;\n> \t/* Interface to the Camera Helper */\n> @@ -105,6 +103,7 @@ int IPAIPU3::init(const IPASettings &settings)\n> \t}\n> \n> \t/* Construct our Algorithms */\n\nPerhaps in the previous patch we should state here that algorithms will\nbe initialised, configured, and processed in the order that they are\nadded to this list.\n\n\n> +\talgorithms_.emplace_back(new algorithms::Awb());\n> \talgorithms_.emplace_back(new algorithms::Contrast());\n> \n> \t/* Reset all the hardware settings */\n> @@ -138,7 +137,7 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n\nOh! The grid *isn't* calculated in the Awb currently, so it really isn't\nan Awb structure ?\n\nIt's just that the Awb uses the structure ? is that right?\n\nIt looks like this is something that the IPU3 top level code does\ncurrently - and really could be moved out to an 'algorithm' to handle\nduring the Configure stage.\n\nI would be quite tempted to see that done as a patch /before/ converting\nthe Awb given it's a pre-requisite to handling the Awb ...\n\n\nIt might also show that we should pass \"const IPAConfigInfo &configInfo\"\ninto the Configure() operations of the modules too.\n\n\nAlternatively, if you really believe this is part of Awb and not it's\nown 'module' - should/could it move into the Configure phase of the Awb\n/in this patch/ ?\n\n\nIf the grid configuration is used only for Awb, then it can be part of\nthe Awb configure.\n\nIf it is used for any other algorithm stage (and I mean on the IPU3, not\njust our current algorithms) then I think it deserves it's own module.\n\n\n\n> \tuint32_t minError = std::numeric_limits::max();\n> \tSize best;\n> \tSize bestLog2;\n> -\tbdsGrid_ = {};\n> +\tipu3_uapi_grid_config &bdsGrid = context_.awb.grid.bdsGrid;\n> \n> \tfor (uint32_t widthShift = 3; widthShift <= 7; ++widthShift) {\n> \t\tuint32_t width = std::min(kMaxCellWidthPerSet,\n> @@ -162,14 +161,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)\n> \t\t}\n> \t}\n> \n> -\tbdsGrid_.width = best.width >> bestLog2.width;\n> -\tbdsGrid_.block_width_log2 = bestLog2.width;\n> -\tbdsGrid_.height = best.height >> bestLog2.height;\n> -\tbdsGrid_.block_height_log2 = bestLog2.height;\n> +\tbdsGrid.width = best.width >> bestLog2.width;\n> +\tbdsGrid.block_width_log2 = bestLog2.width;\n> +\tbdsGrid.height = best.height >> bestLog2.height;\n> +\tbdsGrid.block_height_log2 = bestLog2.height;\n> \n> \tLOG(IPAIPU3, Debug) << \"Best grid found is: (\"\n> -\t\t\t << (int)bdsGrid_.width << \" << \" << (int)bdsGrid_.block_width_log2 << \") x (\"\n> -\t\t\t << (int)bdsGrid_.height << \" << \" << (int)bdsGrid_.block_height_log2 << \")\";\n> +\t\t\t << (int)bdsGrid.width << \" << \" << (int)bdsGrid.block_width_log2 << \") x (\"\n> +\t\t\t << (int)bdsGrid.height << \" << \" << (int)bdsGrid.block_height_log2 << \")\";\n> }\n> \n> int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> @@ -211,13 +210,13 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> \n> \tdefVBlank_ = itVBlank->second.def().get();\n> \n> +\t/* Prepare AWB parameters */\n> \tcalculateBdsGrid(configInfo.bdsOutputSize);\n> +\tcontext_.awb.grid.bdsOutputSize = configInfo.bdsOutputSize;\n\n\nThis looks like evidence suggesting we need an algorithm to handle the\nconfiguration of the BDS which stores it's 'output' in our IPAContext as\na BDS specific data structure.\n\n\n> \n> \tconfigureAlgorithms();\n> \n> -\tawbAlgo_ = std::make_unique();\n> -\tawbAlgo_->initialise(context_.params, configInfo.bdsOutputSize, bdsGrid_);\n> -\n> +\tbdsGrid_ = context_.awb.grid.bdsGrid;\n\nIs this just a copy of the grid in the top level IPU3 framework ? I\nsuspect it shouldn't be needed when we have it on the IPAContext...\n\nIs it still used somehow?\n\nAnyway, that might be all irrelevant if you create a BDS algorithm that\nruns first.\n\n\n> \tagcAlgo_ = std::make_unique();\n> \tagcAlgo_->initialise(bdsGrid_, sensorInfo_);\n> \n> @@ -293,9 +292,6 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> \n> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> {\n> -\tif (agcAlgo_->updateControls())\n> -\t\tawbAlgo_->updateWbParameters(context_.params);\n> -\n> \t*params = context_.params;\n> \n> \tIPU3Action op;\n> @@ -349,8 +345,6 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> \tagcAlgo_->process(stats, exposure_, gain);\n> \tgain_ = camHelper_->gainCode(gain);\n> \n> -\tawbAlgo_->calculateWBGains(stats);\n> -\n\nVery happy to see these algorithm specific hooks being removed though.\n\n> \tif (agcAlgo_->updateControls())\n> \t\tsetControls(frame);\n> \n> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build\n> index fcb27d68..d1126947 100644\n> --- a/src/ipa/ipu3/meson.build\n> +++ b/src/ipa/ipu3/meson.build\n> @@ -7,7 +7,6 @@ ipa_name = 'ipa_ipu3'\n> ipu3_ipa_sources = files([\n> 'ipu3.cpp',\n> 'ipu3_agc.cpp',\n> - 'ipu3_awb.cpp',\n> ])\n> \n> ipu3_ipa_sources += ipu3_ipa_algorithms\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 B86F9BD87D\n\tfor ;\n\tMon, 9 Aug 2021 10:22:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1E0FE6884F;\n\tMon, 9 Aug 2021 12:22:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 198E160269\n\tfor ;\n\tMon, 9 Aug 2021 12:22:56 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8DD6A466;\n\tMon, 9 Aug 2021 12:22:55 +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=\"uRu0LHnm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628504575;\n\tbh=V/gvX92lLxl52R5nYcOIhWH2SWabUUubGQjgabDz45U=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=uRu0LHnmEq30/n5gJi3jeKxtTViUNUDp9L9zEYgMrmoRBHfUNEfJQSSnd7vy5rLCc\n\tqQ/oHUxbFWw2rB3YYpdeIECrJwTiKNaWcxcCFS/hInudZLfCi7+4MXPqYYfjyjJFva\n\tUrixSvcCKjaTDI/ICWyL1CgsA3BEyDMfeINHdKvo=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210809092007.79067-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210809092007.79067-5-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"<957004a2-1553-e742-e998-0991e8549788@ideasonboard.com>","Date":"Mon, 9 Aug 2021 11:22:52 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210809092007.79067-5-jeanmichel.hautbois@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH 4/5] ipa: ipu3: Move IPU3 awb into\n\talgorithms","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":18616,"web_url":"https://patchwork.libcamera.org/comment/18616/","msgid":"<0d9338b2-5a94-a115-34f4-4eccf61bc0ac@ideasonboard.com>","date":"2021-08-09T10:26:07","subject":"Re: [libcamera-devel] [RFC PATCH 4/5] ipa: ipu3: Move IPU3 awb into\n\talgorithms","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 09/08/2021 11:20, Umang Jain wrote:\n> Hi JM\n> \n> Thank you for the patch\n> \n\n\n\n>> +Awb::~Awb()\n>>   {\n>>   }\n>>   -void IPU3Awb::initialise(ipu3_uapi_params ¶ms, const Size\n>> &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid)\n>> +int Awb::configure(IPAContext &context)\n> Mapping old Initialise() directly to Algorithm::configure() ? If there\n> is a reason, I would expect something about it in the commit message maybe?\n>>   {\n>> +    ipu3_uapi_params ¶ms = context.params;\n\n\n\n>> +class Awb : public Algorithm\n>>   {\n>>   public:\n>> -    IPU3Awb();\n>> -    ~IPU3Awb();\n>> +    Awb();\n>> +    ~Awb();\n>>   -    void initialise(ipu3_uapi_params ¶ms, const Size\n>> &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);\n>> -    void calculateWBGains(const ipu3_uapi_stats_3a *stats);\n>> -    void updateWbParameters(ipu3_uapi_params ¶ms);\n>> +    int configure(IPAContext &context) override;\n>> +    void process(IPAContext &context) override;\n\n> hmm, no initialise() here? Doesn't seem sane to have a direct call to\n> configure(). I wonder, as in the last patch where there was only\n> Initialise() and no over-ridden configure()/process().\n\nYes, I think we should explicitly state in the commit message that as\npart of the conversion, the initialisation phase of the algorithm is now\nhandled as part of the configuration phase in the new algorithm structure.\n--\nKieran","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 20DE5C3240\n\tfor ;\n\tMon, 9 Aug 2021 10:26:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 86F5968826;\n\tMon, 9 Aug 2021 12:26:11 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1275A60269\n\tfor ;\n\tMon, 9 Aug 2021 12:26:10 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7F5C6466;\n\tMon, 9 Aug 2021 12:26:09 +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=\"n4xCX2Y4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628504769;\n\tbh=OJTSBXnQ2weSF4PZVvcp9FLMiIg7HqklqRpqxdg5IZg=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=n4xCX2Y4U7ZcCsFZFvP8ep8J99u6GhhAhonzzecPONSW35Kz/cjeankSz/CUawjdc\n\tG+YFLBs7udwuq9iowCxhpkqUXG1UxQDGynylUAXi/wef9pxOnVxlJJbU1doJ0/QNuE\n\thvT563ncOuJNAnGGM+rYdU13kFRIiHDNEKue3QsQ=","To":"Umang Jain ,\n\tJean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210809092007.79067-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210809092007.79067-5-jeanmichel.hautbois@ideasonboard.com>\n\t","From":"Kieran Bingham ","Message-ID":"<0d9338b2-5a94-a115-34f4-4eccf61bc0ac@ideasonboard.com>","Date":"Mon, 9 Aug 2021 11:26:07 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH 4/5] ipa: ipu3: Move IPU3 awb into\n\talgorithms","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\" "}}]