[{"id":15687,"web_url":"https://patchwork.libcamera.org/comment/15687/","msgid":"","date":"2021-03-15T01:52:34","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/3] WIP: ipa: ipu3: Add\n\tsupport for IPU3 AWB algorithm","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nThank you for the patch.\n\nOn Tue, Feb 23, 2021 at 05:40:40PM +0100, Jean-Michel Hautbois wrote:\n> Inherit from the Algorithm class to implement basic AWB functions.\n> \n> Once AWB is done, a color temperature is estimated and default CCM matrices\n> are used (yet to be tuned).\n> Implement a basic \"grey-world\" AWB algorithm just for demonstration purpose.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/ipu3.cpp | 26 +++--\n> src/ipa/ipu3/ipu3_awb.cpp | 199 ++++++++++++++++++++++++++++++++++++++\n> src/ipa/ipu3/ipu3_awb.h | 47 +++++++++\n> src/ipa/ipu3/meson.build | 7 +-\n> 4 files changed, 270 insertions(+), 9 deletions(-)\n> create mode 100644 src/ipa/ipu3/ipu3_awb.cpp\n> create mode 100644 src/ipa/ipu3/ipu3_awb.h\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index b63e58be..6fae5160 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -21,6 +21,8 @@\n> #include \"libcamera/internal/buffer.h\"\n> #include \"libcamera/internal/log.h\"\n> \n> +#include \"ipu3_awb.h\"\n> +\n> namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(IPAIPU3)\n> @@ -60,6 +62,11 @@ private:\n> \tuint32_t gain_;\n> \tuint32_t minGain_;\n> \tuint32_t maxGain_;\n> +\n> +\t/* Interface to the AWB algorithm */\n> +\tipa::IPU3Awb *awbAlgo_;\n> +\t/* Local parameter storage */\n> +\tipu3_uapi_params params_;\n> };\n> \n> void IPAIPU3::configure(const std::map &entityControls)\n> @@ -83,11 +90,16 @@ void IPAIPU3::configure(const std::map &entityControls\n> \n> \tminExposure_ = std::max(itExp->second.min().get(), 1);\n> \tmaxExposure_ = itExp->second.max().get();\n> -\texposure_ = maxExposure_;\n> +\texposure_ = minExposure_;\n> \n> \tminGain_ = std::max(itGain->second.min().get(), 1);\n> \tmaxGain_ = itGain->second.max().get();\n> -\tgain_ = maxGain_;\n> +\tgain_ = minGain_;\n> +\n> +\tparams_ = {};\n> +\n> +\tawbAlgo_ = new ipa::IPU3Awb();\n> +\tawbAlgo_->initialise(params_);\n> \n> \tsetControls(0);\n> }\n> @@ -161,10 +173,8 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n> \n> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> {\n> -\t/* Prepare parameters buffer. */\n> -\tmemset(params, 0, sizeof(*params));\n> -\n> -\t/* \\todo Fill in parameters buffer. */\n> +\tawbAlgo_->updateWbParameters(params_);\n> +\t*params = params_;\n> \n> \tipa::ipu3::IPU3Action op;\n> \top.op = ipa::ipu3::ActionParamFilled;\n> @@ -177,8 +187,8 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> {\n> \tControlList ctrls(controls::controls);\n> \n> -\t/* \\todo React to statistics and update internal state machine. */\n> -\t/* \\todo Add meta-data information to ctrls. */\n> +\tawbAlgo_->calculateWBGains(Rectangle(250, 160, 800, 400), stats);\n> +\tsetControls(frame);\n> \n> \tipa::ipu3::IPU3Action op;\n> \top.op = ipa::ipu3::ActionMetadataReady;\n\nHow about splitting this patch in two, with a first patch that prepares\nfor the algorithms (all of the above except for the code related to\nawbAlgo_), and a patch that adds the AWB algorithm ? The commit message\nof the first patch should explain why you switch from the maximum\nexposure and gain to the minimum.\n\n> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp\n> new file mode 100644\n> index 00000000..3ff239c0\n> --- /dev/null\n> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n> @@ -0,0 +1,199 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited\n\nIs there actually any code in this file that comes from the RPi IPA ?\n\n> + *\n> + * ipu3_awb.cpp - AWB control algorithm\n> + */\n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/log.h\"\n> +\n> +#include \"ipu3_awb.h\"\n\nThis should be included first, to ensure the header is self-contained\n(see Documentation/coding-style.rst).\n\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +LOG_DEFINE_CATEGORY(IPU3Awb)\n> +\n> +static const struct ipu3_uapi_bnr_static_config imgu_css_bnr_defaults = {\n\nimguCssBnrDefaults\n\nSame below.\n\n> +\t{ 16, 16, 16, 16 }, /* wb_gains */\n> +\t{ 255, 255, 255, 255 }, /* wb_gains_thr */\n> +\t{ 0, 0, 8, 6, 0, 14 }, /* thr_coeffs */\n> +\t{ 0, 0, 0, 0 }, /* thr_ctrl_shd */\n> +\t{ -648, 0, -366, 0 }, /* opt_center */\n> +\t{ /* lut */\n> +\t { 17, 23, 28, 32, 36, 39, 42, 45,\n> +\t 48, 51, 53, 55, 58, 60, 62, 64,\n> +\t 66, 68, 70, 72, 73, 75, 77, 78,\n> +\t 80, 82, 83, 85, 86, 88, 89, 90 } },\n> +\t{ 4, 0, 1, 8, 0, 8, 0, 8, 0 }, /* bp_ctrl */\n> +\t{ 8, 4, 4, 0, 8, 0, 1, 1, 1, 1, 0 }, /* dn_detect_ctrl */\n> +\t1296,\n> +\t{ 419904, 133956 },\n\nCould we use designated initializers instead of comments ?\n\n> +};\n> +\n> +/* settings for Auto White Balance */\n> +static const struct ipu3_uapi_awb_config_s imgu_css_awb_defaults = {\n> +\t8191,\n> +\t8191,\n> +\t8191,\n> +\t8191 | /* rgbs_thr_gr/r/gb/b */\n> +\t\tIPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT,\n\nSame here, designated initializers ?\n\n> +\t.grid = {\n> +\t\t.width = 160,\n> +\t\t.height = 45,\n> +\t\t.block_width_log2 = 3,\n> +\t\t.block_height_log2 = 4,\n> +\t\t.x_start = 0,\n> +\t\t.y_start = 0,\n> +\t},\n> +};\n> +\n> +static const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_6000k = {\n> +\t7239, -750, -37, 0,\n> +\t-215, 8196, -200, 0,\n> +\t-70, -589, 6810, 0\n> +};\n> +\n> +static const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_3800k = {\n> +\t7379, -526, -296, 0,\n> +\t-411, 7397, -415, 0,\n> +\t-224, -564, 7244, 0\n> +};\n\nMaybe we should order those by temperature, with 3800k going first ?\n\n> +\n> +IPU3Awb::IPU3Awb()\n> +\t: Algorithm()\n> +{\n> +}\n> +\n> +IPU3Awb::~IPU3Awb()\n> +{\n> +}\n> +\n> +void IPU3Awb::initialise()\n> +{\n> +}\n> +\n> +void IPU3Awb::initialise(ipu3_uapi_params ¶ms)\n> +{\n> +\tparams.use.acc_awb = 1;\n> +\t/*\\todo fill the grid calculated based on BDS configuration */\n> +\tparams.acc_param.awb.config = imgu_css_awb_defaults;\n> +\n> +\tparams.use.acc_bnr = 1;\n> +\tparams.acc_param.bnr = imgu_css_bnr_defaults;\n> +\n> +\tparams.use.acc_ccm = 1;\n> +\tparams.acc_param.ccm = imgu_css_ccm_3800k;\n> +\n> +\tparams.use.acc_gamma = 1;\n> +\tparams.acc_param.gamma.gc_ctrl.enable = 1;\n> +\n> +\tuint32_t a = (32 * 245) / (245 - 9);\n\nShould this be a float, to avoid rounding errors ?\n\n> +\n> +\tfor (uint32_t i = 0; i < 10; i++)\n> +\t\tparams.acc_param.gamma.gc_lut.lut[i] = 0;\n> +\tfor (uint32_t i = 10; i < 245; i++)\n> +\t\tparams.acc_param.gamma.gc_lut.lut[i] = a * i + (0 - a * 9);\n\nMaybe a * (i - 9) ? Or std::round(a * (i - 9)) if you make a a float ?\n\n> +\tfor (uint32_t i = 245; i < 255; i++)\n> +\t\tparams.acc_param.gamma.gc_lut.lut[i] = 32 * 245;\n> +\n> +\twbGains_[0] = 8192 * 0.8;\n> +\twbGains_[1] = 8192;\n> +\twbGains_[2] = 8192;\n> +\twbGains_[3] = 8192 * 0.8;\n> +\n> +\tframe_count_ = 0;\n> +}\n> +\n> +uint32_t IPU3Awb::estimateCCT(uint8_t red, uint8_t green, uint8_t blue)\n> +{\n> +\tdouble X = (-0.14282) * (red) + (1.54924) * (green) + (-0.95641) * (blue);\n> +\tdouble Y = (-0.32466) * (red) + (1.57837) * (green) + (-0.73191) * (blue);\n> +\tdouble Z = (-0.68202) * (red) + (0.77073) * (green) + (0.56332) * (blue);\n\nNo need for parentheses.\n\n> +\n> +\tdouble x = X / (X + Y + Z);\n> +\tdouble y = Y / (X + Y + Z);\n> +\n> +\tdouble n = (x - 0.3320) / (0.1858 - y);\n> +\treturn static_cast(449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33);\n\nI don't think the explicit cast is needed.\n\n> +}\n> +\n> +void IPU3Awb::calculateWBGains([[maybe_unused]] Rectangle roi,\n> +\t\t\t const ipu3_uapi_stats_3a *stats)\n> +{\n> +\tstd::vector redValues, greenRedValues, greenBlueValues, blueValues;\n> +\tPoint topleft = roi.topLeft();\n> +\tuint32_t startY = (topleft.y / 16) * 160 * 8;\n> +\tuint32_t startX = (topleft.x / 8) * 8;\n\n\tuint32_t startX = utils::alignDown(topleft.x, 8);\n\n> +\tuint32_t endX = (startX + (roi.size().width / 8)) * 8;\n\nThis seems incorrect, startX is expressed in pixels, it shouldn't be\nmultiplied by 8.\n\n\tuint32_t endX = startX + utils::alignDown(roi.size().width, 8);\n\n> +\n> +\tfor (uint32_t j = (topleft.y / 16); j < (topleft.y / 16) + (roi.size().height / 16); j++) {\n\nNo need for the inner parentheses.\n\n> +\t\tfor (uint32_t i = startX + startY; i < endX + startY; i += 8) {\n> +\t\t\tgreenRedValues.push_back(stats->awb_raw_buffer.meta_data[i]);\n> +\t\t\tredValues.push_back(stats->awb_raw_buffer.meta_data[i + 1]);\n> +\t\t\tblueValues.push_back(stats->awb_raw_buffer.meta_data[i + 2]);\n> +\t\t\tgreenBlueValues.push_back(stats->awb_raw_buffer.meta_data[i + 3]);\n> +\t\t}\n\nNothing in the inner loop uses j, this seems weird. The code at least\nrequires comments to explain what's going on. You can assume the reader\nhas no idea how AWB works.\n\n> +\t}\n> +\n> +\tstd::sort(redValues.begin(), redValues.end());\n> +\tstd::sort(greenRedValues.begin(), greenRedValues.end());\n> +\tstd::sort(blueValues.begin(), blueValues.end());\n> +\tstd::sort(greenBlueValues.begin(), greenBlueValues.end());\n> +\n> +\tdouble Grmed = greenRedValues[greenRedValues.size() / 2];\n> +\tdouble Rmed = redValues[redValues.size() / 2];\n> +\tdouble Bmed = blueValues[blueValues.size() / 2];\n> +\tdouble Gbmed = greenBlueValues[greenBlueValues.size() / 2];\n> +\n> +\tdouble Rgain = Grmed / Rmed;\n> +\tdouble Bgain = Gbmed / Bmed;\n> +\tLOG(IPU3Awb, Debug) << \"max R, Gr, B, Gb: \"\n> +\t\t\t << redValues.back() << \",\"\n> +\t\t\t << greenRedValues.back() << \",\"\n> +\t\t\t << blueValues.back() << \",\"\n> +\t\t\t << greenBlueValues.back();\n\nIn the 5 blocks above that deal with bayer components, you order them as\nRGGB, GRBG, RGBG, GRBG and RGBG. Could we pick one order ?\n\n> +\ttint_ = ((Rmed / Grmed) + (Bmed / Gbmed)) / 2;\n\nNo need for the inner parentheses.\n\ntint_ is only used in this function, you can make it a local variable.\n\n> +\n> +\t/* \\todo Those are corrections when light is really low\n> +\t * it should be taken into account by AGC somehow */\n\n\t/*\n\t * \\todo Those are corrections when light is really low, it should be\n\t * taken into account by AGC somehow\n\t */\n\n> +\tif ((Rgain >= 2) && (Bgain < 2)) {\n\nThere are lots of inner parentheses that are not needed. I'll stop\nrepeating the comment, you can apply it to other locations below.\n\n> +\t\twbGains_[0] = 4096 * tint_;\n> +\t\twbGains_[1] = 8192 * Rgain;\n> +\t\twbGains_[2] = 4096 * Bgain;\n> +\t\twbGains_[3] = 4096 * tint_;\n> +\t} else if ((Bgain >= 2) && (Rgain < 2)) {\n> +\t\twbGains_[0] = 4096 * tint_;\n> +\t\twbGains_[1] = 4096 * Rgain;\n> +\t\twbGains_[2] = 8192 * Bgain;\n> +\t\twbGains_[3] = 4096 * tint_;\n> +\t} else {\n> +\t\twbGains_[0] = 8192 * tint_;\n> +\t\twbGains_[1] = 8192 * Rgain;\n> +\t\twbGains_[2] = 8192 * Bgain;\n> +\t\twbGains_[3] = 8192 * tint_;\n> +\t}\n> +\n> +\tframe_count_++;\n> +\n> +\tcct_ = estimateCCT(Rmed, (Grmed + Gbmed) / 2, Bmed);\n> +}\n> +\n> +void IPU3Awb::updateWbParameters(ipu3_uapi_params ¶ms)\n> +{\n> +\tparams.acc_param.bnr.wb_gains.gr = wbGains_[0];\n> +\tparams.acc_param.bnr.wb_gains.r = wbGains_[1];\n> +\tparams.acc_param.bnr.wb_gains.b = wbGains_[2];\n> +\tparams.acc_param.bnr.wb_gains.gb = wbGains_[3];\n> +\tif (cct_ < 5500)\n> +\t\tparams.acc_param.ccm = imgu_css_ccm_3800k;\n> +\telse\n> +\t\tparams.acc_param.ccm = imgu_css_ccm_6000k;\n> +}\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> \\ No newline at end of file\n> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h\n> new file mode 100644\n> index 00000000..ff6906f2\n> --- /dev/null\n> +++ b/src/ipa/ipu3/ipu3_awb.h\n> @@ -0,0 +1,47 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited\n> + *\n> + * ipu3_awb.h - IPU3 AWB control algorithm\n> + */\n> +#ifndef __LIBCAMERA_IPU3_AWB_H__\n> +#define __LIBCAMERA_IPU3_AWB_H__\n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"libipa/algorithm.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class IPU3Awb : public Algorithm\n> +{\n> +public:\n> +\tIPU3Awb();\n> +\t~IPU3Awb();\n> +\n> +\tvoid initialise() override;\n\nThis isn't used, you can drop it.\n\n> +\n> +\tvoid initialise(ipu3_uapi_params ¶ms);\n> +\tvoid calculateWBGains(Rectangle roi,\n> +\t\t\t const ipu3_uapi_stats_3a *stats);\n> +\tvoid updateWbParameters(ipu3_uapi_params ¶ms);\n> +\n> +private:\n> +\tuint32_t estimateCCT(uint8_t red, uint8_t green, uint8_t blue);\n> +\n> +\t/* WB calculated gains */\n> +\tuint16_t wbGains_[4];\n> +\tdouble tint_;\n> +\tuint32_t cct_;\n> +\n> +\tuint32_t frame_count_;\n> +};\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera*/\n> +#endif /* __LIBCAMERA_IPU3_AWB_H__ */\n> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build\n> index a241f617..07a864c8 100644\n> --- a/src/ipa/ipu3/meson.build\n> +++ b/src/ipa/ipu3/meson.build\n> @@ -2,8 +2,13 @@\n> \n> ipa_name = 'ipa_ipu3'\n> \n> +ipu3_ipa_sources = files([\n> + 'ipu3.cpp',\n> + 'ipu3_awb.cpp',\n> +])\n> +\n> mod = shared_module(ipa_name,\n> - ['ipu3.cpp', libcamera_generated_ipa_headers],\n> + [ipu3_ipa_sources, libcamera_generated_ipa_headers],\n> name_prefix : '',\n> include_directories : [ipa_includes, libipa_includes],\n> dependencies : libcamera_dep,","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 38C65BD80E\n\tfor ;\n\tMon, 15 Mar 2021 01:53:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A91F668AA1;\n\tMon, 15 Mar 2021 02:53:11 +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 2F28E602E0\n\tfor ;\n\tMon, 15 Mar 2021 02:53:11 +0100 (CET)","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 7CBEE87A;\n\tMon, 15 Mar 2021 02:53:10 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"C/xeC8P/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615773190;\n\tbh=hCcVetA7ZxbUJMfainLRxYOEJyJptkdurVbEmZ8NUVs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=C/xeC8P/JqefrB/hhHGZ88uCPqxipt+6MOBRIVcbGuK2n5aJ7oPV9DeB/3KcnsJnF\n\toQntowNbSQ/a2/z4jqHbGqn5rPzbgzEXo6hQ0vej6/b20U6Ml5Z+FIe9NmKrSRywf2\n\tHKifacd1rKHl/I7A8LQqjgx7T0J5KHonrjuLD0/Q=","Date":"Mon, 15 Mar 2021 03:52:34 +0200","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210223164041.49932-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210223164041.49932-3-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210223164041.49932-3-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/3] WIP: ipa: ipu3: Add\n\tsupport for IPU3 AWB algorithm","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]