[{"id":15688,"web_url":"https://patchwork.libcamera.org/comment/15688/","msgid":"","date":"2021-03-15T02:28:49","subject":"Re: [libcamera-devel] [RFC PATCH v2 3/3] WIP: ipa: ipu3: Add\n\tsupport for IPU3 AEC/AGC 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:41PM +0100, Jean-Michel Hautbois wrote:\n> Inherit from the Algorithm class to implement basic auto-exposure and\n> auto-gain functions.\n> \n> While exposure is not locked, AWB is not calculated and corrected.\n> Implement a basic \"skewness-based\" for demonstration purpose.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/ipu3.cpp | 10 ++-\n> src/ipa/ipu3/ipu3_agc.cpp | 171 ++++++++++++++++++++++++++++++++++++++\n> src/ipa/ipu3/ipu3_agc.h | 62 ++++++++++++++\n> src/ipa/ipu3/meson.build | 1 +\n> 4 files changed, 243 insertions(+), 1 deletion(-)\n> create mode 100644 src/ipa/ipu3/ipu3_agc.cpp\n> create mode 100644 src/ipa/ipu3/ipu3_agc.h\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 6fae5160..cabd0c71 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -21,6 +21,7 @@\n> #include \"libcamera/internal/buffer.h\"\n> #include \"libcamera/internal/log.h\"\n> \n> +#include \"ipu3_agc.h\"\n> #include \"ipu3_awb.h\"\n> \n> namespace libcamera {\n> @@ -65,6 +66,8 @@ private:\n> \n> \t/* Interface to the AWB algorithm */\n> \tipa::IPU3Awb *awbAlgo_;\n> +\t/* Interface to the AEC/AGC algorithm */\n> +\tipa::IPU3Agc *agcAlgo_;\n\nI think those two comments could be dropped. Comments for the parts of\nthe code that are not trivial to understand would be more useful ;-)\n\nA blank line would be nice here.\n\n> \t/* Local parameter storage */\n> \tipu3_uapi_params params_;\n> };\n> @@ -101,6 +104,8 @@ void IPAIPU3::configure(const std::map &entityControls\n> \tawbAlgo_ = new ipa::IPU3Awb();\n> \tawbAlgo_->initialise(params_);\n> \n> +\tagcAlgo_ = new ipa::IPU3Agc();\n\nNever deleted. You can use a std::unique_ptr<>. Same for AWB.\n\n> +\n> \tsetControls(0);\n> }\n> \n> @@ -187,7 +192,10 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> {\n> \tControlList ctrls(controls::controls);\n> \n> -\tawbAlgo_->calculateWBGains(Rectangle(250, 160, 800, 400), stats);\n> +\tagcAlgo_->process(stats, exposure_, gain_);\n> +\tif (agcAlgo_->converged())\n> +\t\tawbAlgo_->calculateWBGains(Rectangle(250, 160, 800, 400), stats);\n> +\n> \tsetControls(frame);\n> \n> \tipa::ipu3::IPU3Action op;\n> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp\n> new file mode 100644\n> index 00000000..2636e2ea\n> --- /dev/null\n> +++ b/src/ipa/ipu3/ipu3_agc.cpp\n> @@ -0,0 +1,171 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited\n\nIs there any code here coming from the RPi IPA ?\n\n> + *\n> + * ipu3_agc.cpp - AGC/AEC control algorithm\n> + */\n> +\n> +#include \"ipu3_agc.h\"\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/log.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +LOG_DEFINE_CATEGORY(IPU3Agc)\n> +\n> +/* Number of frames to wait before calculating stats on minimum exposure */\n> +static const uint32_t kInitialFrameMinAECount = 4;\n\ns/const/constexpr/\n\nSame below.\n\n> +/* Number of frames to wait before calculating stats on maximum exposure */\n> +static const uint32_t kInitialFrameMaxAECount = 8;\n> +/* Number of frames to wait before calculating stats and estimate gain/exposure */\n> +static const uint32_t kInitialFrameSkipCount = 12;\n> +\n> +/* Number of frames to wait between new gain/exposure estimations */\n> +static const uint32_t kFrameSkipCount = 4;\n> +\n> +IPU3Agc::IPU3Agc()\n> +\t: frameCount_(0), converged_(false)\n> +{\n> +}\n> +\n> +IPU3Agc::~IPU3Agc()\n> +{\n> +}\n> +\n> +void IPU3Agc::initialise()\n> +{\n> +}\n> +\n> +void IPU3Agc::process()\n> +{\n> +}\n> +\n> +/*\n> + * \\todo This function is taken from numerical recipes and calculates all\n> + * moments. It needs to be rewritten properly and maybe in a \"math\" class ?\n> + */\n> +void IPU3Agc::moments(std::unordered_map &data, int n)\n\ndata is never modified, you can make it const.\n\n> +{\n> +\tint j;\n\nunsigned int ?\n\n> +\tdouble ep = 0.0, s, p;\n> +\tdouble ave, adev, sdev;\n> +\tdouble var, skew, curt;\n> +\n> +\ts = 0.0;\n> +\tfor (j = 1; j <= n; j++)\n> +\t\ts += data[j];\n> +\n> +\tave = s / n;\n> +\tadev = var = skew = curt = 0.0;\n> +\n> +\tfor (j = 1; j <= n; j++) {\n> +\t\tadev += s = data[j] - (ave);\n> +\t\tep += s;\n> +\t\tvar += (p = s * s);\n> +\t\tskew += (p *= s);\n> +\t\tcurt += (p *= s);\n> +\t}\n> +\n> +\tadev /= n;\n> +\tvar = (var - ep * ep / n) / (n - 1);\n> +\tsdev = std::sqrt(var);\n> +\n> +\tif (var) {\n> +\t\tskew /= n * var * sdev;\n> +\t\tcurt = curt / (n * var * var) - 3.0;\n> +\t}\n> +\tskew_ = skew;\n> +}\n> +\n> +void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)\n> +{\n> +\tcellsBrightness_.clear();\n\nOnly used in this function, you can make it a local variable.\n\n> +\n> +\t/*\\todo Replace constant values with real BDS configuration */\n> +\tfor (uint32_t j = 0; j < 45; j++) {\n\nYou still repeat the exact same inner loop 45 times...\n\n> +\t\tfor (uint32_t i = 0; i < 160 * 45 * 8; i += 8) {\n> +\t\t\tuint8_t Gr = stats->awb_raw_buffer.meta_data[i];\n> +\t\t\tuint8_t R = stats->awb_raw_buffer.meta_data[i + 1];\n> +\t\t\tuint8_t B = stats->awb_raw_buffer.meta_data[i + 2];\n> +\t\t\tuint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3];\n> +\t\t\tcellsBrightness_.push_back(static_cast(0.299 * R + 0.587 * (Gr + Gb) / 2 + 0.114 * B));\n> +\t\t}\n> +\t}\n> +\tstd::sort(cellsBrightness_.begin(), cellsBrightness_.end());\n> +\n> +\t/* \\todo create a class to generate histograms ! */\n> +\tstd::unordered_map hist;\n\nIs it me, or are you using a map to store a vector ?\n\n\tstd::vector histogram{ 256 };\n\nOr possibly\n\n\tstd::array histogram{};\n\n> +\tfor (uint32_t const &val : cellsBrightness_)\n> +\t\thist[val]++;\n> +\tmoments(hist, 256);\n\nAnd you can drop the second argument, as it will be conveyed either\nimplicitly by the vector, or explicitly through the array type (I'd use\nSpan in the second case though).\n\n> +}\n> +\n> +/* \\todo make this function a math one ? */\n> +uint32_t IPU3Agc::rootApproximation()\n> +{\n> +\treturn (currentExposure_ * prevSkew_ + prevExposure_ * currentSkew_) / (prevSkew_ + currentSkew_);\n> +}\n> +\n> +void IPU3Agc::lockExposure(uint32_t &exposure, uint32_t &gain)\n> +{\n> +\t/* Algorithm initialization wait for first valid frames */\n> +\t/* \\todo - have a number of frames given by DelayedControls ?\n> +\t * - implement a function for IIR */\n\n\t/*\n\t * ...\n\t * ...\n\t */\n\n> +\tif (frameCount_ == kInitialFrameMinAECount) {\n> +\t\tprevExposure_ = exposure;\n> +\n> +\t\tprevSkew_ = skew_;\n> +\t\t/* \\todo use configured values */\n> +\t\texposure = 800;\n> +\t\tgain = 8;\n> +\t\tcurrentExposure_ = exposure;\n> +\t} else if (frameCount_ == kInitialFrameMaxAECount) {\n> +\t\tcurrentSkew_ = skew_;\n> +\t\texposure = rootApproximation();\n> +\t\tnextExposure_ = exposure;\n> +\t\tlastFrame_ = frameCount_;\n> +\t} else if ((frameCount_ >= kInitialFrameSkipCount) && (frameCount_ - lastFrame_ >= kFrameSkipCount)) {\n> +\t\tcurrentSkew_ = skew_;\n> +\t\t/* \\todo properly calculate a gain */\n> +\t\tif (frameCount_ == kInitialFrameSkipCount)\n> +\t\t\tgain = ((8 * prevSkew_) + (1 * currentSkew_)) / (prevSkew_ + currentSkew_);\n> +\n> +\t\tif (currentSkew_ - prevSkew_ > 1) {\n> +\t\t\t/* under exposed */\n> +\t\t\tprevExposure_ = nextExposure_;\n> +\t\t\texposure = rootApproximation();\n> +\t\t\tnextExposure_ = exposure;\n> +\t\t} else if (currentSkew_ - prevSkew_ < -1) {\n> +\t\t\t/* over exposed */\n> +\t\t\tcurrentExposure_ = nextExposure_;\n> +\t\t\texposure = rootApproximation();\n> +\t\t\tnextExposure_ = exposure;\n> +\t\t} else {\n> +\t\t\t/* we have converged */\n> +\t\t\tconverged_ = true;\n> +\t\t}\n> +\t\tlastFrame_ = frameCount_;\n> +\t\tprevSkew_ = currentSkew_;\n> +\t}\n\nAs commented in 2/3, a comment block to explain what this does would be\nuseful.\n\n> +}\n> +\n> +void IPU3Agc::process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, uint32_t &gain)\n> +{\n> +\tprocessBrightness(stats);\n> +\tif (!converged_)\n> +\t\tlockExposure(exposure, gain);\n> +\telse {\n> +\t\t/* Are we still well exposed ? */\n> +\t\tif ((skew_ < 2) || (skew_ > 4))\n\nNo need for inner parentheses.\n\n> +\t\t\tconverged_ = false;\n> +\t}\n> +\tframeCount_++;\n> +}\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n> new file mode 100644\n> index 00000000..b14a2a2f\n> --- /dev/null\n> +++ b/src/ipa/ipu3/ipu3_agc.h\n> @@ -0,0 +1,62 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited\n> + *\n> + * ipu3_agc.h - IPU3 AGC/AEC control algorithm\n> + */\n> +#ifndef __LIBCAMERA_IPU3_AGC_H__\n> +#define __LIBCAMERA_IPU3_AGC_H__\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n\nThis doesn't seem to be needed here.\n\n> +\n> +#include \"libipa/algorithm.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class IPU3Agc : public Algorithm\n> +{\n> +public:\n> +\tIPU3Agc();\n> +\t~IPU3Agc();\n> +\n> +\tvoid initialise() override;\n> +\tvoid process() override;\n> +\n> +\tvoid process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, uint32_t &gain);\n> +\tbool converged() { return converged_; }\n> +\n> +private:\n> +\tvoid moments(std::unordered_map &data, int n);\n> +\tvoid processBrightness(const ipu3_uapi_stats_3a *stats);\n> +\tuint32_t rootApproximation();\n> +\tvoid lockExposure(uint32_t &exposure, uint32_t &gain);\n> +\n> +\tuint64_t frameCount_;\n> +\tuint64_t lastFrame_;\n> +\n> +\t/* Vector of calculated brightness for each cell */\n> +\tstd::vector cellsBrightness_;\n> +\n> +\t/* Values for filtering */\n> +\tuint32_t prevExposure_;\n> +\tuint32_t currentExposure_;\n> +\tuint32_t nextExposure_;\n\nHere we have prev, current, next\n\n> +\n> +\tdouble skew_;\n> +\tdouble prevSkew_;\n> +\tdouble currentSkew_;\n\nAnd here we have \"\", prev and current. A common naming scheme would make\nit easier to read the code.\n\n> +\tbool converged_;\n> +};\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPU3_AGC_H__ */\n> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build\n> index 07a864c8..43ad0e0d 100644\n> --- a/src/ipa/ipu3/meson.build\n> +++ b/src/ipa/ipu3/meson.build\n> @@ -4,6 +4,7 @@ ipa_name = 'ipa_ipu3'\n> \n> ipu3_ipa_sources = files([\n> 'ipu3.cpp',\n> + 'ipu3_agc.cpp',\n> 'ipu3_awb.cpp',\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 9598BBD80C\n\tfor ;\n\tMon, 15 Mar 2021 02:29:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 04750602E4;\n\tMon, 15 Mar 2021 03:29:27 +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 930B6602E0\n\tfor ;\n\tMon, 15 Mar 2021 03:29:25 +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 EF22A87A;\n\tMon, 15 Mar 2021 03:29:24 +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=\"QhWrgKlM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615775365;\n\tbh=t7hgbXqyxJgYyFYMfpZ8DBZRlccH4l7V9cg1l5MZtJc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QhWrgKlM35v9sfK1SIhX5cHEdYPkWdjmMxJTtrW5+oNkiCWDWUaK3MyxTSyNNey4V\n\tlr2T8dd5iw631SjeNr8HHcEb7NdHHlim4sjaCh23hicFBpb+cIkRIeHNDR4aG61DMl\n\tuyhn0nB/ECZ0qLVxXZPEXJ4jYuIM65thpdyecNYU=","Date":"Mon, 15 Mar 2021 04:28:49 +0200","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210223164041.49932-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210223164041.49932-4-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210223164041.49932-4-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 3/3] WIP: ipa: ipu3: Add\n\tsupport for IPU3 AEC/AGC 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\" "}}]