[{"id":18612,"web_url":"https://patchwork.libcamera.org/comment/18612/","msgid":"<0ad91245-309a-648a-3f04-059f04e3e395@ideasonboard.com>","date":"2021-08-09T09:52:42","subject":"Re: [libcamera-devel] [RFC PATCH 3/5] ipa: ipu3: Introduce a\n\tmodular contrast algorithm","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> Implement a new modular framework for algorithms with a common context\n> structure that is passed to each algorithm through a common API.\n\nThis doesn't implement the framework. (This text is likely leftover from\na previous split).\n\nThis patch:\n\n\"\"\"\nIntroduce a new algorithm to manage the contrast handling of the IPU3.\n\n\"\"\"\n\n> \n> The initial algorithm is chosen to configure the gamma contrast curve\n> which replicates the implementation from AWB for simplicity. As it is\n\nYou don't replicate it anymore, you move it.\n\n> initialised with a default gamma value of 1.0, there is no need to use\n> the default table at init time anymore.>\n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n> ---\n>  src/ipa/ipu3/algorithms/contrast.cpp | 52 ++++++++++++++++++++++++++++\n>  src/ipa/ipu3/algorithms/contrast.h   | 32 +++++++++++++++++\n>  src/ipa/ipu3/algorithms/meson.build  |  1 +\n>  src/ipa/ipu3/ipu3.cpp                |  6 +++-\n>  src/ipa/ipu3/ipu3_agc.cpp            |  5 +--\n>  src/ipa/ipu3/ipu3_agc.h              |  3 --\n>  src/ipa/ipu3/ipu3_awb.cpp            | 41 ++--------------------\n>  src/ipa/ipu3/ipu3_awb.h              |  2 +-\n>  8 files changed, 94 insertions(+), 48 deletions(-)\n>  create mode 100644 src/ipa/ipu3/algorithms/contrast.cpp\n>  create mode 100644 src/ipa/ipu3/algorithms/contrast.h\n> \n> diff --git a/src/ipa/ipu3/algorithms/contrast.cpp b/src/ipa/ipu3/algorithms/contrast.cpp\n> new file mode 100644\n> index 00000000..832dbf35\n> --- /dev/null\n> +++ b/src/ipa/ipu3/algorithms/contrast.cpp\n> @@ -0,0 +1,52 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Ideas On Board\n> + *\n> + * constrast.cpp - IPU3 Contrast and Gamma control\n> + */\n> +\n> +#include \"contrast.h\"\n> +\n> +#include <cmath>\n> +\n> +#include <libcamera/base/log.h>\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::ipu3::algorithms {\n> +\n> +LOG_DEFINE_CATEGORY(IPU3Contrast)\n> +\n> +Contrast::Contrast()\n> +\t: gamma_(1.0)\n> +{\n> +\tLOG(IPU3Contrast, Info) << \"Instantiate Gamma\";\n> +}\n> +\n> +int Contrast::initialise(IPAContext &context)\n> +{\n> +\tipu3_uapi_params &params = context.params;\n> +\n> +\tparams.use.acc_gamma = 1;\n> +\tparams.acc_param.gamma.gc_ctrl.enable = 1;\n> +\n> +\t/* Limit the gamma effect for now */\n> +\tgamma_ = 1.1;\n\nThe commit message stated we initialise to 1.0 (which we do on\nconstruction) but here we actually initialise to 1.1...\n\n\n\n> +\n> +\t/* Plot the gamma curve into the look up table */\n> +\tfor (uint32_t i = 0; i < 256; i++) {\n> +\t\tdouble j = i / 255.0;\n> +\t\tdouble gamma = std::pow(j, 1.0 / gamma_);\n> +\n> +\t\t/* The maximum value 255 is represented on 13 bits in the IPU3 */\n> +\t\tparams.acc_param.gamma.gc_lut.lut[i] = gamma * 8191;\n> +\t}\n> +\n> +\tLOG(IPU3Contrast, Info) << \"Processed Gamma Curve\";\n\nI think we can drop Info line now or move it to Debug if you think it's\nhelpful to keep it... But if we keep it as Debug, we should print\nsomething more useful, like what gamma value we actually set.\n\nI think it's probably not helpful though.\n\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +} /* namespace ipa::ipu3::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/ipu3/algorithms/contrast.h b/src/ipa/ipu3/algorithms/contrast.h\n> new file mode 100644\n> index 00000000..958e8bb8\n> --- /dev/null\n> +++ b/src/ipa/ipu3/algorithms/contrast.h\n> @@ -0,0 +1,32 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google\n> + *\n> + * constrast.h - IPU3 Contrast and Gamma control\n> + */\n> +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__\n> +#define __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__\n> +\n> +#include \"algorithm.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::ipu3::algorithms {\n> +\n> +class Contrast : public Algorithm\n> +{\n> +public:\n> +\tContrast();\n> +\t~Contrast() = default;\n> +\n> +\tint initialise(IPAContext &context) override;\n> +\n> +private:\n> +\tdouble gamma_;\n> +};\n> +\n> +} /* namespace ipa::ipu3::algorithms */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__ */\n> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build\n> index 67148333..f71d1e61 100644\n> --- a/src/ipa/ipu3/algorithms/meson.build\n> +++ b/src/ipa/ipu3/algorithms/meson.build\n> @@ -1,4 +1,5 @@\n>  # SPDX-License-Identifier: CC0-1.0\n>  \n>  ipu3_ipa_algorithms = files([\n> +    'contrast.cpp',\n>  ])\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 55c4da72..7035802f 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -23,6 +23,7 @@\n>  #include \"libcamera/internal/framebuffer.h\"\n>  \n>  #include \"algorithms/algorithm.h\"\n> +#include \"algorithms/contrast.h\"\n>  #include \"ipa_context.h\"\n>  \n>  #include \"ipu3_agc.h\"\n> @@ -103,6 +104,9 @@ int IPAIPU3::init(const IPASettings &settings)\n>  \t\treturn -ENODEV;\n>  \t}\n>  \n> +\t/* Construct our Algorithms */\n> +\talgorithms_.emplace_back(new algorithms::Contrast());\n> +\n>  \t/* Reset all the hardware settings */\n>  \tcontext_.params = {};\n>  \n> @@ -290,7 +294,7 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n>  void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n>  {\n>  \tif (agcAlgo_->updateControls())\n> -\t\tawbAlgo_->updateWbParameters(context_.params, agcAlgo_->gamma());\n> +\t\tawbAlgo_->updateWbParameters(context_.params);\n>  \n>  \t*params = context_.params;\n>  \n> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp\n> index 6253ab94..733814dd 100644\n> --- a/src/ipa/ipu3/ipu3_agc.cpp\n> +++ b/src/ipa/ipu3/ipu3_agc.cpp\n> @@ -52,7 +52,7 @@ static constexpr uint8_t kCellSize = 8;\n>  \n>  IPU3Agc::IPU3Agc()\n>  \t: frameCount_(0), lastFrame_(0), converged_(false),\n> -\t  updateControls_(false), iqMean_(0.0), gamma_(1.0),\n> +\t  updateControls_(false), iqMean_(0.0),\n>  \t  lineDuration_(0s), maxExposureTime_(0s),\n>  \t  prevExposure_(0s), prevExposureNoDg_(0s),\n>  \t  currentExposure_(0s), currentExposureNoDg_(0s)\n> @@ -104,9 +104,6 @@ void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)\n>  \t\t}\n>  \t}\n>  \n> -\t/* Limit the gamma effect for now */\n> -\tgamma_ = 1.1;\n> -\n>  \t/* Estimate the quantile mean of the top 2% of the histogram */\n>  \tiqMean_ = Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);\n>  }\n> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n> index e3e1d28b..3b7f781e 100644\n> --- a/src/ipa/ipu3/ipu3_agc.h\n> +++ b/src/ipa/ipu3/ipu3_agc.h\n> @@ -37,8 +37,6 @@ public:\n>  \tvoid process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain);\n>  \tbool converged() { return converged_; }\n>  \tbool updateControls() { return updateControls_; }\n> -\t/* \\todo Use a metadata exchange between IPAs */\n> -\tdouble gamma() { return gamma_; }\n>  \n>  private:\n>  \tvoid processBrightness(const ipu3_uapi_stats_3a *stats);\n> @@ -54,7 +52,6 @@ private:\n>  \tbool updateControls_;\n>  \n>  \tdouble iqMean_;\n> -\tdouble gamma_;\n>  \n>  \tDuration lineDuration_;\n>  \tDuration maxExposureTime_;\n> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp\n> index 9b409c8f..043c3838 100644\n> --- a/src/ipa/ipu3/ipu3_awb.cpp\n> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n> @@ -134,31 +134,6 @@ static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {\n>  \t0, 0, 8191, 0\n>  };\n>  \n> -/* Default settings for Gamma correction */\n> -const struct ipu3_uapi_gamma_corr_lut imguCssGammaLut = { {\n> -\t63, 79, 95, 111, 127, 143, 159, 175, 191, 207, 223, 239, 255, 271, 287,\n> -\t303, 319, 335, 351, 367, 383, 399, 415, 431, 447, 463, 479, 495, 511,\n> -\t527, 543, 559, 575, 591, 607, 623, 639, 655, 671, 687, 703, 719, 735,\n> -\t751, 767, 783, 799, 815, 831, 847, 863, 879, 895, 911, 927, 943, 959,\n> -\t975, 991, 1007, 1023, 1039, 1055, 1071, 1087, 1103, 1119, 1135, 1151,\n> -\t1167, 1183, 1199, 1215, 1231, 1247, 1263, 1279, 1295, 1311, 1327, 1343,\n> -\t1359, 1375, 1391, 1407, 1423, 1439, 1455, 1471, 1487, 1503, 1519, 1535,\n> -\t1551, 1567, 1583, 1599, 1615, 1631, 1647, 1663, 1679, 1695, 1711, 1727,\n> -\t1743, 1759, 1775, 1791, 1807, 1823, 1839, 1855, 1871, 1887, 1903, 1919,\n> -\t1935, 1951, 1967, 1983, 1999, 2015, 2031, 2047, 2063, 2079, 2095, 2111,\n> -\t2143, 2175, 2207, 2239, 2271, 2303, 2335, 2367, 2399, 2431, 2463, 2495,\n> -\t2527, 2559, 2591, 2623, 2655, 2687, 2719, 2751, 2783, 2815, 2847, 2879,\n> -\t2911, 2943, 2975, 3007, 3039, 3071, 3103, 3135, 3167, 3199, 3231, 3263,\n> -\t3295, 3327, 3359, 3391, 3423, 3455, 3487, 3519, 3551, 3583, 3615, 3647,\n> -\t3679, 3711, 3743, 3775, 3807, 3839, 3871, 3903, 3935, 3967, 3999, 4031,\n> -\t4063, 4095, 4127, 4159, 4223, 4287, 4351, 4415, 4479, 4543, 4607, 4671,\n> -\t4735, 4799, 4863, 4927, 4991, 5055, 5119, 5183, 5247, 5311, 5375, 5439,\n> -\t5503, 5567, 5631, 5695, 5759, 5823, 5887, 5951, 6015, 6079, 6143, 6207,\n> -\t6271, 6335, 6399, 6463, 6527, 6591, 6655, 6719, 6783, 6847, 6911, 6975,\n> -\t7039, 7103, 7167, 7231, 7295, 7359, 7423, 7487, 7551, 7615, 7679, 7743,\n> -\t7807, 7871, 7935, 7999, 8063, 8127, 8191\n> -} };\n> -\n>  IPU3Awb::IPU3Awb()\n>  \t: Algorithm()\n>  {\n> @@ -198,10 +173,6 @@ void IPU3Awb::initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, st\n>  \tparams.use.acc_ccm = 1;\n>  \tparams.acc_param.ccm = imguCssCcmDefault;\n>  \n> -\tparams.use.acc_gamma = 1;\n> -\tparams.acc_param.gamma.gc_lut = imguCssGammaLut;\n> -\tparams.acc_param.gamma.gc_ctrl.enable = 1;\n> -\n>  \tzones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);\n>  }\n>  \n> @@ -351,7 +322,7 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)\n>  \t}\n>  }\n>  \n> -void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, double agcGamma)\n> +void IPU3Awb::updateWbParameters(ipu3_uapi_params &params)\n>  {\n>  \t/*\n>  \t * Green gains should not be touched and considered 1.\n> @@ -363,18 +334,10 @@ void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, double agcGamma)\n>  \tparams.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain;\n>  \tparams.acc_param.bnr.wb_gains.gb = 16;\n>  \n> -\tLOG(IPU3Awb, Debug) << \"Color temperature estimated: \" << asyncResults_.temperatureK\n> -\t\t\t    << \" and gamma calculated: \" << agcGamma;\n> +\tLOG(IPU3Awb, Debug) << \"Color temperature estimated: \" << asyncResults_.temperatureK;\n>  \n>  \t/* The CCM matrix may change when color temperature will be used */\n>  \tparams.acc_param.ccm = imguCssCcmDefault;\n> -\n> -\tfor (uint32_t i = 0; i < 256; i++) {\n> -\t\tdouble j = i / 255.0;\n> -\t\tdouble gamma = std::pow(j, 1.0 / agcGamma);\n> -\t\t/* The maximum value 255 is represented on 13 bits in the IPU3 */\n> -\t\tparams.acc_param.gamma.gc_lut.lut[i] = gamma * 8191;\n> -\t}\n>  }\n>  \n>  } /* namespace ipa::ipu3 */\n> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h\n> index 284e3844..8b05ac03 100644\n> --- a/src/ipa/ipu3/ipu3_awb.h\n> +++ b/src/ipa/ipu3/ipu3_awb.h\n> @@ -32,7 +32,7 @@ public:\n>  \n>  \tvoid initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);\n>  \tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats);\n> -\tvoid updateWbParameters(ipu3_uapi_params &params, double agcGamma);\n> +\tvoid updateWbParameters(ipu3_uapi_params &params);\n>  \n>  \tstruct Ipu3AwbCell {\n>  \t\tunsigned char greenRedAvg;\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 0CEBABD87D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  9 Aug 2021 09:52:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 787DE6884F;\n\tMon,  9 Aug 2021 11:52:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8C36F68823\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  9 Aug 2021 11:52:45 +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 8F7F2EE;\n\tMon,  9 Aug 2021 11:52:44 +0200 (CEST)"],"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=\"E0qMQSXD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628502765;\n\tbh=Dw2nizmrCX7oXSuQqKtTvji/dQSIN32nmb/QD8PgcnM=;\n\th=From:To:References:Subject:Date:In-Reply-To:From;\n\tb=E0qMQSXDtWVZC/IlD9Cm2Ml3uupwBshbXgygbuSXIukpXkqzPTcDYFLkPDLMv35xe\n\tggwHIyKXChamHYqzAATymo7kBPwuFRy4HvJDkDWgNK683kbzNS+WXIz0psCLnZioxx\n\tyo+EjNIOOhdBUW3Cfrt5fSkfiaDJokV0OkCfH0fA=","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210809092007.79067-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210809092007.79067-4-jeanmichel.hautbois@ideasonboard.com>","Message-ID":"<0ad91245-309a-648a-3f04-059f04e3e395@ideasonboard.com>","Date":"Mon, 9 Aug 2021 10:52:42 +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-4-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 3/5] ipa: ipu3: Introduce a\n\tmodular contrast algorithm","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18613,"web_url":"https://patchwork.libcamera.org/comment/18613/","msgid":"<2e941f31-1254-9fd2-b53f-6d35ba0c8273@ideasonboard.com>","date":"2021-08-09T10:10:04","subject":"Re: [libcamera-devel] [RFC PATCH 3/5] ipa: ipu3: Introduce a\n\tmodular contrast algorithm","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi JM and Kieran,\n\nOn 8/9/21 3:22 PM, Kieran Bingham wrote:\n> On 09/08/2021 10:20, Jean-Michel Hautbois wrote:\n>> Implement a new modular framework for algorithms with a common context\n>> structure that is passed to each algorithm through a common API.\n> This doesn't implement the framework. (This text is likely leftover from\n> a previous split).\n>\n> This patch:\n>\n> \"\"\"\n> Introduce a new algorithm to manage the contrast handling of the IPU3.\n>\n> \"\"\"\n>\n>> The initial algorithm is chosen to configure the gamma contrast curve\n>> which replicates the implementation from AWB for simplicity. As it is\n> You don't replicate it anymore, you move it.\nIndeed. I think the commit message can be a more descriptive, as things \nhappening here are:\n- Remove existing gamma constrast handling from IPU3Awb\n- Introduce Constrast class which handles gamma constrast handling\n- Params are now set directly by Contrast::Initialise() through IPAContext\n\nI wonder if it can be split up in two patches?\n>\n>> initialised with a default gamma value of 1.0, there is no need to use\n>> the default table at init time anymore.>\n>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n>> ---\n>>   src/ipa/ipu3/algorithms/contrast.cpp | 52 ++++++++++++++++++++++++++++\n>>   src/ipa/ipu3/algorithms/contrast.h   | 32 +++++++++++++++++\n>>   src/ipa/ipu3/algorithms/meson.build  |  1 +\n>>   src/ipa/ipu3/ipu3.cpp                |  6 +++-\n>>   src/ipa/ipu3/ipu3_agc.cpp            |  5 +--\n>>   src/ipa/ipu3/ipu3_agc.h              |  3 --\n>>   src/ipa/ipu3/ipu3_awb.cpp            | 41 ++--------------------\n>>   src/ipa/ipu3/ipu3_awb.h              |  2 +-\n>>   8 files changed, 94 insertions(+), 48 deletions(-)\n>>   create mode 100644 src/ipa/ipu3/algorithms/contrast.cpp\n>>   create mode 100644 src/ipa/ipu3/algorithms/contrast.h\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/contrast.cpp b/src/ipa/ipu3/algorithms/contrast.cpp\n>> new file mode 100644\n>> index 00000000..832dbf35\n>> --- /dev/null\n>> +++ b/src/ipa/ipu3/algorithms/contrast.cpp\n>> @@ -0,0 +1,52 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Ideas On Board\n>> + *\n>> + * constrast.cpp - IPU3 Contrast and Gamma control\n>> + */\n>> +\n>> +#include \"contrast.h\"\n>> +\n>> +#include <cmath>\n>> +\n>> +#include <libcamera/base/log.h>\n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa::ipu3::algorithms {\n>> +\n>> +LOG_DEFINE_CATEGORY(IPU3Contrast)\n>> +\n>> +Contrast::Contrast()\n>> +\t: gamma_(1.0)\n>> +{\n>> +\tLOG(IPU3Contrast, Info) << \"Instantiate Gamma\";\n>> +}\n>> +\n>> +int Contrast::initialise(IPAContext &context)\n>> +{\n>> +\tipu3_uapi_params &params = context.params;\n>> +\n>> +\tparams.use.acc_gamma = 1;\n>> +\tparams.acc_param.gamma.gc_ctrl.enable = 1;\n>> +\n>> +\t/* Limit the gamma effect for now */\n>> +\tgamma_ = 1.1;\n> The commit message stated we initialise to 1.0 (which we do on\n> construction) but here we actually initialise to 1.1...\n>\n>\n>\n>> +\n>> +\t/* Plot the gamma curve into the look up table */\n>> +\tfor (uint32_t i = 0; i < 256; i++) {\n>> +\t\tdouble j = i / 255.0;\n>> +\t\tdouble gamma = std::pow(j, 1.0 / gamma_);\n>> +\n>> +\t\t/* The maximum value 255 is represented on 13 bits in the IPU3 */\n>> +\t\tparams.acc_param.gamma.gc_lut.lut[i] = gamma * 8191;\n>> +\t}\n>> +\n>> +\tLOG(IPU3Contrast, Info) << \"Processed Gamma Curve\";\n> I think we can drop Info line now or move it to Debug if you think it's\n> helpful to keep it... But if we keep it as Debug, we should print\n> something more useful, like what gamma value we actually set.\n>\n> I think it's probably not helpful though.\n+1\n>\n>\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +} /* namespace ipa::ipu3::algorithms */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/ipu3/algorithms/contrast.h b/src/ipa/ipu3/algorithms/contrast.h\n>> new file mode 100644\n>> index 00000000..958e8bb8\n>> --- /dev/null\n>> +++ b/src/ipa/ipu3/algorithms/contrast.h\n>> @@ -0,0 +1,32 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Google\n>> + *\n>> + * constrast.h - IPU3 Contrast and Gamma control\n>> + */\n>> +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__\n>> +#define __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__\n>> +\n>> +#include \"algorithm.h\"\n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa::ipu3::algorithms {\n>> +\n>> +class Contrast : public Algorithm\n>> +{\n>> +public:\n>> +\tContrast();\n>> +\t~Contrast() = default;\n>> +\n>> +\tint initialise(IPAContext &context) override;\n\nThere  is no need for configure() and process() here? Is creating the \ntable is all what needs to be done in Initialise()?\n\n\n>> +\n>> +private:\n>> +\tdouble gamma_;\n>> +};\n>> +\n>> +} /* namespace ipa::ipu3::algorithms */\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__ */\n>> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build\n>> index 67148333..f71d1e61 100644\n>> --- a/src/ipa/ipu3/algorithms/meson.build\n>> +++ b/src/ipa/ipu3/algorithms/meson.build\n>> @@ -1,4 +1,5 @@\n>>   # SPDX-License-Identifier: CC0-1.0\n>>   \n>>   ipu3_ipa_algorithms = files([\n>> +    'contrast.cpp',\n>>   ])\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 55c4da72..7035802f 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -23,6 +23,7 @@\n>>   #include \"libcamera/internal/framebuffer.h\"\n>>   \n>>   #include \"algorithms/algorithm.h\"\n>> +#include \"algorithms/contrast.h\"\n>>   #include \"ipa_context.h\"\n>>   \n>>   #include \"ipu3_agc.h\"\n>> @@ -103,6 +104,9 @@ int IPAIPU3::init(const IPASettings &settings)\n>>   \t\treturn -ENODEV;\n>>   \t}\n>>   \n>> +\t/* Construct our Algorithms */\n>> +\talgorithms_.emplace_back(new algorithms::Contrast());\n>> +\n>>   \t/* Reset all the hardware settings */\n>>   \tcontext_.params = {};\n>>   \n>> @@ -290,7 +294,7 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n>>   void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n>>   {\n>>   \tif (agcAlgo_->updateControls())\n>> -\t\tawbAlgo_->updateWbParameters(context_.params, agcAlgo_->gamma());\n>> +\t\tawbAlgo_->updateWbParameters(context_.params);\n>>   \n>>   \t*params = context_.params;\n>>   \n>> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp\n>> index 6253ab94..733814dd 100644\n>> --- a/src/ipa/ipu3/ipu3_agc.cpp\n>> +++ b/src/ipa/ipu3/ipu3_agc.cpp\n>> @@ -52,7 +52,7 @@ static constexpr uint8_t kCellSize = 8;\n>>   \n>>   IPU3Agc::IPU3Agc()\n>>   \t: frameCount_(0), lastFrame_(0), converged_(false),\n>> -\t  updateControls_(false), iqMean_(0.0), gamma_(1.0),\n>> +\t  updateControls_(false), iqMean_(0.0),\n>>   \t  lineDuration_(0s), maxExposureTime_(0s),\n>>   \t  prevExposure_(0s), prevExposureNoDg_(0s),\n>>   \t  currentExposure_(0s), currentExposureNoDg_(0s)\n>> @@ -104,9 +104,6 @@ void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)\n>>   \t\t}\n>>   \t}\n>>   \n>> -\t/* Limit the gamma effect for now */\n>> -\tgamma_ = 1.1;\n>> -\n>>   \t/* Estimate the quantile mean of the top 2% of the histogram */\n>>   \tiqMean_ = Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);\n>>   }\n>> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n>> index e3e1d28b..3b7f781e 100644\n>> --- a/src/ipa/ipu3/ipu3_agc.h\n>> +++ b/src/ipa/ipu3/ipu3_agc.h\n>> @@ -37,8 +37,6 @@ public:\n>>   \tvoid process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain);\n>>   \tbool converged() { return converged_; }\n>>   \tbool updateControls() { return updateControls_; }\n>> -\t/* \\todo Use a metadata exchange between IPAs */\n>> -\tdouble gamma() { return gamma_; }\n>>   \n>>   private:\n>>   \tvoid processBrightness(const ipu3_uapi_stats_3a *stats);\n>> @@ -54,7 +52,6 @@ private:\n>>   \tbool updateControls_;\n>>   \n>>   \tdouble iqMean_;\n>> -\tdouble gamma_;\n>>   \n>>   \tDuration lineDuration_;\n>>   \tDuration maxExposureTime_;\n>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp\n>> index 9b409c8f..043c3838 100644\n>> --- a/src/ipa/ipu3/ipu3_awb.cpp\n>> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n>> @@ -134,31 +134,6 @@ static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {\n>>   \t0, 0, 8191, 0\n>>   };\n>>   \n>> -/* Default settings for Gamma correction */\n>> -const struct ipu3_uapi_gamma_corr_lut imguCssGammaLut = { {\n>> -\t63, 79, 95, 111, 127, 143, 159, 175, 191, 207, 223, 239, 255, 271, 287,\n>> -\t303, 319, 335, 351, 367, 383, 399, 415, 431, 447, 463, 479, 495, 511,\n>> -\t527, 543, 559, 575, 591, 607, 623, 639, 655, 671, 687, 703, 719, 735,\n>> -\t751, 767, 783, 799, 815, 831, 847, 863, 879, 895, 911, 927, 943, 959,\n>> -\t975, 991, 1007, 1023, 1039, 1055, 1071, 1087, 1103, 1119, 1135, 1151,\n>> -\t1167, 1183, 1199, 1215, 1231, 1247, 1263, 1279, 1295, 1311, 1327, 1343,\n>> -\t1359, 1375, 1391, 1407, 1423, 1439, 1455, 1471, 1487, 1503, 1519, 1535,\n>> -\t1551, 1567, 1583, 1599, 1615, 1631, 1647, 1663, 1679, 1695, 1711, 1727,\n>> -\t1743, 1759, 1775, 1791, 1807, 1823, 1839, 1855, 1871, 1887, 1903, 1919,\n>> -\t1935, 1951, 1967, 1983, 1999, 2015, 2031, 2047, 2063, 2079, 2095, 2111,\n>> -\t2143, 2175, 2207, 2239, 2271, 2303, 2335, 2367, 2399, 2431, 2463, 2495,\n>> -\t2527, 2559, 2591, 2623, 2655, 2687, 2719, 2751, 2783, 2815, 2847, 2879,\n>> -\t2911, 2943, 2975, 3007, 3039, 3071, 3103, 3135, 3167, 3199, 3231, 3263,\n>> -\t3295, 3327, 3359, 3391, 3423, 3455, 3487, 3519, 3551, 3583, 3615, 3647,\n>> -\t3679, 3711, 3743, 3775, 3807, 3839, 3871, 3903, 3935, 3967, 3999, 4031,\n>> -\t4063, 4095, 4127, 4159, 4223, 4287, 4351, 4415, 4479, 4543, 4607, 4671,\n>> -\t4735, 4799, 4863, 4927, 4991, 5055, 5119, 5183, 5247, 5311, 5375, 5439,\n>> -\t5503, 5567, 5631, 5695, 5759, 5823, 5887, 5951, 6015, 6079, 6143, 6207,\n>> -\t6271, 6335, 6399, 6463, 6527, 6591, 6655, 6719, 6783, 6847, 6911, 6975,\n>> -\t7039, 7103, 7167, 7231, 7295, 7359, 7423, 7487, 7551, 7615, 7679, 7743,\n>> -\t7807, 7871, 7935, 7999, 8063, 8127, 8191\n>> -} };\n>> -\n>>   IPU3Awb::IPU3Awb()\n>>   \t: Algorithm()\n>>   {\n>> @@ -198,10 +173,6 @@ void IPU3Awb::initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, st\n>>   \tparams.use.acc_ccm = 1;\n>>   \tparams.acc_param.ccm = imguCssCcmDefault;\n>>   \n>> -\tparams.use.acc_gamma = 1;\n>> -\tparams.acc_param.gamma.gc_lut = imguCssGammaLut;\n>> -\tparams.acc_param.gamma.gc_ctrl.enable = 1;\n>> -\n>>   \tzones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);\n>>   }\n>>   \n>> @@ -351,7 +322,7 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)\n>>   \t}\n>>   }\n>>   \n>> -void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, double agcGamma)\n>> +void IPU3Awb::updateWbParameters(ipu3_uapi_params &params)\n>>   {\n>>   \t/*\n>>   \t * Green gains should not be touched and considered 1.\n>> @@ -363,18 +334,10 @@ void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, double agcGamma)\n>>   \tparams.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain;\n>>   \tparams.acc_param.bnr.wb_gains.gb = 16;\n>>   \n>> -\tLOG(IPU3Awb, Debug) << \"Color temperature estimated: \" << asyncResults_.temperatureK\n>> -\t\t\t    << \" and gamma calculated: \" << agcGamma;\n>> +\tLOG(IPU3Awb, Debug) << \"Color temperature estimated: \" << asyncResults_.temperatureK;\n>>   \n>>   \t/* The CCM matrix may change when color temperature will be used */\n>>   \tparams.acc_param.ccm = imguCssCcmDefault;\n>> -\n>> -\tfor (uint32_t i = 0; i < 256; i++) {\n>> -\t\tdouble j = i / 255.0;\n>> -\t\tdouble gamma = std::pow(j, 1.0 / agcGamma);\n>> -\t\t/* The maximum value 255 is represented on 13 bits in the IPU3 */\n>> -\t\tparams.acc_param.gamma.gc_lut.lut[i] = gamma * 8191;\n>> -\t}\n>>   }\n>>   \n>>   } /* namespace ipa::ipu3 */\n>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h\n>> index 284e3844..8b05ac03 100644\n>> --- a/src/ipa/ipu3/ipu3_awb.h\n>> +++ b/src/ipa/ipu3/ipu3_awb.h\n>> @@ -32,7 +32,7 @@ public:\n>>   \n>>   \tvoid initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);\n>>   \tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats);\n>> -\tvoid updateWbParameters(ipu3_uapi_params &params, double agcGamma);\n>> +\tvoid updateWbParameters(ipu3_uapi_params &params);\n>>   \n>>   \tstruct Ipu3AwbCell {\n>>   \t\tunsigned char greenRedAvg;\n>>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 01882C3240\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  9 Aug 2021 10:10:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 69B9268826;\n\tMon,  9 Aug 2021 12:10: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 1F7F560269\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  9 Aug 2021 12:10:10 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.61])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8B3DEEE;\n\tMon,  9 Aug 2021 12:10:08 +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=\"p56TN8KO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628503809;\n\tbh=/ZNMuaxg9tGPPmvwscXnd7WzWNfMpdaKflNu6UH4Cnw=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=p56TN8KOxGr/uw41f6DmXrN1xOgFdtY1JJNHyXweMrL1x1cR5d508hqh0E2FAxw46\n\tZlH34sLwnBHR8+J/wpMXSgm2D1+/WS/f3O2CE/mX/FugLo9xWNAgKoCjjXq98T/eYH\n\tJGmvzpNz1Wlet1qfAYgiNb7hv2k2BKaDe9cbOje4=","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tJean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210809092007.79067-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210809092007.79067-4-jeanmichel.hautbois@ideasonboard.com>\n\t<0ad91245-309a-648a-3f04-059f04e3e395@ideasonboard.com>","From":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<2e941f31-1254-9fd2-b53f-6d35ba0c8273@ideasonboard.com>","Date":"Mon, 9 Aug 2021 15:40:04 +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":"<0ad91245-309a-648a-3f04-059f04e3e395@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC PATCH 3/5] ipa: ipu3: Introduce a\n\tmodular contrast algorithm","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18661,"web_url":"https://patchwork.libcamera.org/comment/18661/","msgid":"<6caadf8d-73a1-7b9a-313b-c5adebe667d1@ideasonboard.com>","date":"2021-08-10T07:49:38","subject":"Re: [libcamera-devel] [RFC PATCH 3/5] ipa: ipu3: Introduce a\n\tmodular contrast algorithm","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Kieran,\n\nOn 09/08/2021 11:52, Kieran Bingham wrote:\n> On 09/08/2021 10:20, Jean-Michel Hautbois wrote:\n>> Implement a new modular framework for algorithms with a common context\n>> structure that is passed to each algorithm through a common API.\n> \n> This doesn't implement the framework. (This text is likely leftover from\n> a previous split).\n> \n> This patch:\n> \n> \"\"\"\n> Introduce a new algorithm to manage the contrast handling of the IPU3.\n> \n> \"\"\"\n> \n>>\n>> The initial algorithm is chosen to configure the gamma contrast curve\n>> which replicates the implementation from AWB for simplicity. As it is\n> \n> You don't replicate it anymore, you move it.\n> \n>> initialised with a default gamma value of 1.0, there is no need to use\n>> the default table at init time anymore.>\n>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n>> ---\n>>  src/ipa/ipu3/algorithms/contrast.cpp | 52 ++++++++++++++++++++++++++++\n>>  src/ipa/ipu3/algorithms/contrast.h   | 32 +++++++++++++++++\n>>  src/ipa/ipu3/algorithms/meson.build  |  1 +\n>>  src/ipa/ipu3/ipu3.cpp                |  6 +++-\n>>  src/ipa/ipu3/ipu3_agc.cpp            |  5 +--\n>>  src/ipa/ipu3/ipu3_agc.h              |  3 --\n>>  src/ipa/ipu3/ipu3_awb.cpp            | 41 ++--------------------\n>>  src/ipa/ipu3/ipu3_awb.h              |  2 +-\n>>  8 files changed, 94 insertions(+), 48 deletions(-)\n>>  create mode 100644 src/ipa/ipu3/algorithms/contrast.cpp\n>>  create mode 100644 src/ipa/ipu3/algorithms/contrast.h\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/contrast.cpp b/src/ipa/ipu3/algorithms/contrast.cpp\n>> new file mode 100644\n>> index 00000000..832dbf35\n>> --- /dev/null\n>> +++ b/src/ipa/ipu3/algorithms/contrast.cpp\n>> @@ -0,0 +1,52 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Ideas On Board\n>> + *\n>> + * constrast.cpp - IPU3 Contrast and Gamma control\n>> + */\n>> +\n>> +#include \"contrast.h\"\n>> +\n>> +#include <cmath>\n>> +\n>> +#include <libcamera/base/log.h>\n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa::ipu3::algorithms {\n>> +\n>> +LOG_DEFINE_CATEGORY(IPU3Contrast)\n>> +\n>> +Contrast::Contrast()\n>> +\t: gamma_(1.0)\n>> +{\n>> +\tLOG(IPU3Contrast, Info) << \"Instantiate Gamma\";\n>> +}\n>> +\n>> +int Contrast::initialise(IPAContext &context)\n>> +{\n>> +\tipu3_uapi_params &params = context.params;\n>> +\n>> +\tparams.use.acc_gamma = 1;\n>> +\tparams.acc_param.gamma.gc_ctrl.enable = 1;\n>> +\n>> +\t/* Limit the gamma effect for now */\n>> +\tgamma_ = 1.1;\n> \n> The commit message stated we initialise to 1.0 (which we do on\n> construction) but here we actually initialise to 1.1...\n> \n\nIndeed ! I will change the commit message :-).\n\n> \n>> +\n>> +\t/* Plot the gamma curve into the look up table */\n>> +\tfor (uint32_t i = 0; i < 256; i++) {\n>> +\t\tdouble j = i / 255.0;\n>> +\t\tdouble gamma = std::pow(j, 1.0 / gamma_);\n>> +\n>> +\t\t/* The maximum value 255 is represented on 13 bits in the IPU3 */\n>> +\t\tparams.acc_param.gamma.gc_lut.lut[i] = gamma * 8191;\n>> +\t}\n>> +\n>> +\tLOG(IPU3Contrast, Info) << \"Processed Gamma Curve\";\n> \n> I think we can drop Info line now or move it to Debug if you think it's\n> helpful to keep it... But if we keep it as Debug, we should print\n> something more useful, like what gamma value we actually set.\n> \n> I think it's probably not helpful though.\n\nIt might not be helpful, indeed, and the one in the constructor could\nalso be removed ?\n\n> \n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +} /* namespace ipa::ipu3::algorithms */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/ipu3/algorithms/contrast.h b/src/ipa/ipu3/algorithms/contrast.h\n>> new file mode 100644\n>> index 00000000..958e8bb8\n>> --- /dev/null\n>> +++ b/src/ipa/ipu3/algorithms/contrast.h\n>> @@ -0,0 +1,32 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Google\n>> + *\n>> + * constrast.h - IPU3 Contrast and Gamma control\n>> + */\n>> +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__\n>> +#define __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__\n>> +\n>> +#include \"algorithm.h\"\n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa::ipu3::algorithms {\n>> +\n>> +class Contrast : public Algorithm\n>> +{\n>> +public:\n>> +\tContrast();\n>> +\t~Contrast() = default;\n>> +\n>> +\tint initialise(IPAContext &context) override;\n>> +\n>> +private:\n>> +\tdouble gamma_;\n>> +};\n>> +\n>> +} /* namespace ipa::ipu3::algorithms */\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__ */\n>> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build\n>> index 67148333..f71d1e61 100644\n>> --- a/src/ipa/ipu3/algorithms/meson.build\n>> +++ b/src/ipa/ipu3/algorithms/meson.build\n>> @@ -1,4 +1,5 @@\n>>  # SPDX-License-Identifier: CC0-1.0\n>>  \n>>  ipu3_ipa_algorithms = files([\n>> +    'contrast.cpp',\n>>  ])\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 55c4da72..7035802f 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -23,6 +23,7 @@\n>>  #include \"libcamera/internal/framebuffer.h\"\n>>  \n>>  #include \"algorithms/algorithm.h\"\n>> +#include \"algorithms/contrast.h\"\n>>  #include \"ipa_context.h\"\n>>  \n>>  #include \"ipu3_agc.h\"\n>> @@ -103,6 +104,9 @@ int IPAIPU3::init(const IPASettings &settings)\n>>  \t\treturn -ENODEV;\n>>  \t}\n>>  \n>> +\t/* Construct our Algorithms */\n>> +\talgorithms_.emplace_back(new algorithms::Contrast());\n>> +\n>>  \t/* Reset all the hardware settings */\n>>  \tcontext_.params = {};\n>>  \n>> @@ -290,7 +294,7 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,\n>>  void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n>>  {\n>>  \tif (agcAlgo_->updateControls())\n>> -\t\tawbAlgo_->updateWbParameters(context_.params, agcAlgo_->gamma());\n>> +\t\tawbAlgo_->updateWbParameters(context_.params);\n>>  \n>>  \t*params = context_.params;\n>>  \n>> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp\n>> index 6253ab94..733814dd 100644\n>> --- a/src/ipa/ipu3/ipu3_agc.cpp\n>> +++ b/src/ipa/ipu3/ipu3_agc.cpp\n>> @@ -52,7 +52,7 @@ static constexpr uint8_t kCellSize = 8;\n>>  \n>>  IPU3Agc::IPU3Agc()\n>>  \t: frameCount_(0), lastFrame_(0), converged_(false),\n>> -\t  updateControls_(false), iqMean_(0.0), gamma_(1.0),\n>> +\t  updateControls_(false), iqMean_(0.0),\n>>  \t  lineDuration_(0s), maxExposureTime_(0s),\n>>  \t  prevExposure_(0s), prevExposureNoDg_(0s),\n>>  \t  currentExposure_(0s), currentExposureNoDg_(0s)\n>> @@ -104,9 +104,6 @@ void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)\n>>  \t\t}\n>>  \t}\n>>  \n>> -\t/* Limit the gamma effect for now */\n>> -\tgamma_ = 1.1;\n>> -\n>>  \t/* Estimate the quantile mean of the top 2% of the histogram */\n>>  \tiqMean_ = Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);\n>>  }\n>> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n>> index e3e1d28b..3b7f781e 100644\n>> --- a/src/ipa/ipu3/ipu3_agc.h\n>> +++ b/src/ipa/ipu3/ipu3_agc.h\n>> @@ -37,8 +37,6 @@ public:\n>>  \tvoid process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain);\n>>  \tbool converged() { return converged_; }\n>>  \tbool updateControls() { return updateControls_; }\n>> -\t/* \\todo Use a metadata exchange between IPAs */\n>> -\tdouble gamma() { return gamma_; }\n>>  \n>>  private:\n>>  \tvoid processBrightness(const ipu3_uapi_stats_3a *stats);\n>> @@ -54,7 +52,6 @@ private:\n>>  \tbool updateControls_;\n>>  \n>>  \tdouble iqMean_;\n>> -\tdouble gamma_;\n>>  \n>>  \tDuration lineDuration_;\n>>  \tDuration maxExposureTime_;\n>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp\n>> index 9b409c8f..043c3838 100644\n>> --- a/src/ipa/ipu3/ipu3_awb.cpp\n>> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n>> @@ -134,31 +134,6 @@ static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {\n>>  \t0, 0, 8191, 0\n>>  };\n>>  \n>> -/* Default settings for Gamma correction */\n>> -const struct ipu3_uapi_gamma_corr_lut imguCssGammaLut = { {\n>> -\t63, 79, 95, 111, 127, 143, 159, 175, 191, 207, 223, 239, 255, 271, 287,\n>> -\t303, 319, 335, 351, 367, 383, 399, 415, 431, 447, 463, 479, 495, 511,\n>> -\t527, 543, 559, 575, 591, 607, 623, 639, 655, 671, 687, 703, 719, 735,\n>> -\t751, 767, 783, 799, 815, 831, 847, 863, 879, 895, 911, 927, 943, 959,\n>> -\t975, 991, 1007, 1023, 1039, 1055, 1071, 1087, 1103, 1119, 1135, 1151,\n>> -\t1167, 1183, 1199, 1215, 1231, 1247, 1263, 1279, 1295, 1311, 1327, 1343,\n>> -\t1359, 1375, 1391, 1407, 1423, 1439, 1455, 1471, 1487, 1503, 1519, 1535,\n>> -\t1551, 1567, 1583, 1599, 1615, 1631, 1647, 1663, 1679, 1695, 1711, 1727,\n>> -\t1743, 1759, 1775, 1791, 1807, 1823, 1839, 1855, 1871, 1887, 1903, 1919,\n>> -\t1935, 1951, 1967, 1983, 1999, 2015, 2031, 2047, 2063, 2079, 2095, 2111,\n>> -\t2143, 2175, 2207, 2239, 2271, 2303, 2335, 2367, 2399, 2431, 2463, 2495,\n>> -\t2527, 2559, 2591, 2623, 2655, 2687, 2719, 2751, 2783, 2815, 2847, 2879,\n>> -\t2911, 2943, 2975, 3007, 3039, 3071, 3103, 3135, 3167, 3199, 3231, 3263,\n>> -\t3295, 3327, 3359, 3391, 3423, 3455, 3487, 3519, 3551, 3583, 3615, 3647,\n>> -\t3679, 3711, 3743, 3775, 3807, 3839, 3871, 3903, 3935, 3967, 3999, 4031,\n>> -\t4063, 4095, 4127, 4159, 4223, 4287, 4351, 4415, 4479, 4543, 4607, 4671,\n>> -\t4735, 4799, 4863, 4927, 4991, 5055, 5119, 5183, 5247, 5311, 5375, 5439,\n>> -\t5503, 5567, 5631, 5695, 5759, 5823, 5887, 5951, 6015, 6079, 6143, 6207,\n>> -\t6271, 6335, 6399, 6463, 6527, 6591, 6655, 6719, 6783, 6847, 6911, 6975,\n>> -\t7039, 7103, 7167, 7231, 7295, 7359, 7423, 7487, 7551, 7615, 7679, 7743,\n>> -\t7807, 7871, 7935, 7999, 8063, 8127, 8191\n>> -} };\n>> -\n>>  IPU3Awb::IPU3Awb()\n>>  \t: Algorithm()\n>>  {\n>> @@ -198,10 +173,6 @@ void IPU3Awb::initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, st\n>>  \tparams.use.acc_ccm = 1;\n>>  \tparams.acc_param.ccm = imguCssCcmDefault;\n>>  \n>> -\tparams.use.acc_gamma = 1;\n>> -\tparams.acc_param.gamma.gc_lut = imguCssGammaLut;\n>> -\tparams.acc_param.gamma.gc_ctrl.enable = 1;\n>> -\n>>  \tzones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);\n>>  }\n>>  \n>> @@ -351,7 +322,7 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)\n>>  \t}\n>>  }\n>>  \n>> -void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, double agcGamma)\n>> +void IPU3Awb::updateWbParameters(ipu3_uapi_params &params)\n>>  {\n>>  \t/*\n>>  \t * Green gains should not be touched and considered 1.\n>> @@ -363,18 +334,10 @@ void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, double agcGamma)\n>>  \tparams.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain;\n>>  \tparams.acc_param.bnr.wb_gains.gb = 16;\n>>  \n>> -\tLOG(IPU3Awb, Debug) << \"Color temperature estimated: \" << asyncResults_.temperatureK\n>> -\t\t\t    << \" and gamma calculated: \" << agcGamma;\n>> +\tLOG(IPU3Awb, Debug) << \"Color temperature estimated: \" << asyncResults_.temperatureK;\n>>  \n>>  \t/* The CCM matrix may change when color temperature will be used */\n>>  \tparams.acc_param.ccm = imguCssCcmDefault;\n>> -\n>> -\tfor (uint32_t i = 0; i < 256; i++) {\n>> -\t\tdouble j = i / 255.0;\n>> -\t\tdouble gamma = std::pow(j, 1.0 / agcGamma);\n>> -\t\t/* The maximum value 255 is represented on 13 bits in the IPU3 */\n>> -\t\tparams.acc_param.gamma.gc_lut.lut[i] = gamma * 8191;\n>> -\t}\n>>  }\n>>  \n>>  } /* namespace ipa::ipu3 */\n>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h\n>> index 284e3844..8b05ac03 100644\n>> --- a/src/ipa/ipu3/ipu3_awb.h\n>> +++ b/src/ipa/ipu3/ipu3_awb.h\n>> @@ -32,7 +32,7 @@ public:\n>>  \n>>  \tvoid initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);\n>>  \tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats);\n>> -\tvoid updateWbParameters(ipu3_uapi_params &params, double agcGamma);\n>> +\tvoid updateWbParameters(ipu3_uapi_params &params);\n>>  \n>>  \tstruct Ipu3AwbCell {\n>>  \t\tunsigned char greenRedAvg;\n>>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 D6253C3240\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 10 Aug 2021 07:49:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A430B68826;\n\tTue, 10 Aug 2021 09:49:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B518D687DE\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 10 Aug 2021 09:49:40 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:257d:f5fc:a3d5:6682])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 714D6EE;\n\tTue, 10 Aug 2021 09:49:40 +0200 (CEST)"],"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=\"pPO8P/e6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628581780;\n\tbh=4BDzoL3GXSi/n4+BcbTzL70ctL3x1JNo4nwNVZiPBbQ=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=pPO8P/e63j23tSVPpP9uY8LotHhGHXb/e0SHQ46xAqwaaQIAiyQw3LFwmJyB+wQe+\n\tFO1Lq69KMNbyo4y9VOh6vSW0vC/HelUH5uaI7JQRP3bQPuE2rKyq+L+yctbePgHyJe\n\t5AX0ICtt0I/kfuV+DCvJF/AcOlRPU1lE520Uqp9Y=","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210809092007.79067-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210809092007.79067-4-jeanmichel.hautbois@ideasonboard.com>\n\t<0ad91245-309a-648a-3f04-059f04e3e395@ideasonboard.com>","From":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","Message-ID":"<6caadf8d-73a1-7b9a-313b-c5adebe667d1@ideasonboard.com>","Date":"Tue, 10 Aug 2021 09:49:38 +0200","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":"<0ad91245-309a-648a-3f04-059f04e3e395@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [RFC PATCH 3/5] ipa: ipu3: Introduce a\n\tmodular contrast algorithm","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]