[{"id":18931,"web_url":"https://patchwork.libcamera.org/comment/18931/","msgid":"<YR1+oKJaHmfzJxxL@pendragon.ideasonboard.com>","date":"2021-08-18T21:41:52","subject":"Re: [libcamera-devel] [PATCH v3 5/9] ipa: ipu3: Introduce a modular\n\tcontrast 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 Wed, Aug 18, 2021 at 05:53:59PM +0200, Jean-Michel Hautbois wrote:\n> Introduce a new algorithm to manage the contrast handling of the IPU3.\n> \n> The initial algorithm is chosen to configure the gamma contrast curve\n> which moves the implementation out of AWB for simplicity. As it is\n> initialised with a default gamma value of 1.1, there is no need to use\n> the default table at initialisation anymore.\n> \n> This demonstrates the way to use process() call when the EventStatReady\n> comes in. The function calculates the LUT in the context of a frame, and\n> when prepare() is called, the parameters are filled with the updated\n> values.\n> \n> AGC is modified to take the new process interface into account.\n> \n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> ---\n>  src/ipa/ipu3/algorithms/contrast.cpp | 53 ++++++++++++++++++++++++++++\n>  src/ipa/ipu3/algorithms/contrast.h   | 32 +++++++++++++++++\n>  src/ipa/ipu3/algorithms/meson.build  |  1 +\n>  src/ipa/ipu3/ipa_context.h           |  8 +++++\n>  src/ipa/ipu3/ipu3.cpp                | 15 +++++---\n>  src/ipa/ipu3/ipu3_agc.cpp            |  9 +++--\n>  src/ipa/ipu3/ipu3_agc.h              |  5 +--\n>  src/ipa/ipu3/ipu3_awb.cpp            | 41 ++-------------------\n>  src/ipa/ipu3/ipu3_awb.h              |  2 +-\n>  9 files changed, 113 insertions(+), 53 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..eb92d3c7\n> --- /dev/null\n> +++ b/src/ipa/ipu3/algorithms/contrast.cpp\n> @@ -0,0 +1,53 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google inc.\n> + *\n> + * contrast.cpp - IPU3 Contrast and Gamma control\n> + */\n> +\n> +#include \"contrast.h\"\n> +\n> +#include <cmath>\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::ipu3::algorithms {\n> +\n> +\n\nOne extra blank line.\n\n> +Contrast::Contrast()\n> +\t: gamma_(1.0)\n> +{\n> +}\n> +\n> +void Contrast::prepare([[maybe_unused]] IPAContext &context, ipu3_uapi_params &params)\n> +{\n> +\t/* Copy the calculated LUT into the parameters buffer */\n\ns/buffer/buffer./\n\n> +\tmemcpy(params.acc_param.gamma.gc_lut.lut,\n\nMissing header.\n\n> +\t       context.frameContext.contrast.gammaCorrection.lut,\n> +\t       IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES * sizeof(params.acc_param.gamma.gc_lut.lut[0]));\n\nLine wrap:\n\n\t       IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES *\n\t       sizeof(params.acc_param.gamma.gc_lut.lut[0]));\n\n> +\n> +\t/* Enable the custom gamma table */\n\ns/table/table./\n\nI won't repeat this comment everywhere, you can apply it through the\nseries.\n\n> +\tparams.use.acc_gamma = 1;\n> +\tparams.acc_param.gamma.gc_ctrl.enable = 1;\n> +}\n> +\n> +void Contrast::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n\nLine wrap.\n\nI'll stop repeating this comment too :-)\n\n> +{\n> +\t/* Limit the gamma effect for now */\n\nWhat do you mean by \"limit\" ? I understand it as\n\n\t/*\n\t * Hardcode gamma to 1.1 as a default for now.\n\t *\n\t * \\todo Expose gamma control setting through the libcamera control API\n\t */\n\n> +\t/* \\todo expose gamma control setting through the libcamera control API */\n> +\tgamma_ = 1.1;\n> +\n> +\t/* Plot the gamma curve into the look up table */\n> +\tfor (uint32_t i = 0; i < IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES; i++) {\n> +\t\tdouble j = static_cast<double>(i)\n> +\t\t\t / (IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1);\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\nI think \"The output value is expressed on 13 bits.\" would be clearer.\n\n> +\t\tcontext.frameContext.contrast.gammaCorrection.lut[i] = gamma * 8191;\n> +\t}\n\nHow about using std::size to avoid overflowing the array by mistake ?\n\n\tstruct ipu3_uapi_gamma_corr_lut &lut =\n\t\tcontext.frameContext.contrast.gammaCorrection;\n\n\tfor (uint32_t i = 0; i < std::size(lut.lut); i++) {\n\t\tdouble j = static_cast<double>(i) / (std::size(lut.lut) - 1);\n\t\tdouble gamma = std::pow(j, 1.0 / gamma_);\n\n\t\t/* The output value is expressed on 13 bits. */\n\t\tlut.lut[i] = gamma * 8191;\n\t}\n\nUp to you.\n\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..6cd6c5db\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 inc.\n> + *\n> + * contrast.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\nI wonder of the algorithm shouldn't be called ToneMapping as it does\nmore than just contrast.\n\n> +{\n> +public:\n> +\tContrast();\n> +\t~Contrast() = default;\n\nYou can drop the destructor, it's not needed.\n\n> +\n> +\tvoid prepare(IPAContext &context, ipu3_uapi_params &params) override;\n> +\tvoid process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;\n\nMissing blank line.\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 dc538b79..e3ff3b78 100644\n> --- a/src/ipa/ipu3/algorithms/meson.build\n> +++ b/src/ipa/ipu3/algorithms/meson.build\n> @@ -2,4 +2,5 @@\n>  \n>  ipu3_ipa_algorithms = files([\n>      'algorithm.cpp',\n> +    'contrast.cpp',\n>  ])\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index 46291d9e..5964ba6d 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -24,6 +24,14 @@ struct IPAConfiguration {\n>  };\n>  \n>  struct IPAFrameContext {\n> +\tstruct Agc {\n> +\t\tuint32_t exposure;\n> +\t\tdouble gain;\n> +\t} agc;\n\nI think this belongs to patch 7/9, along with the changes to\nipu3_agc.cpp and ipu3_agc.h.\n\n> +\n> +\tstruct Contrast {\n> +\t\tstruct ipu3_uapi_gamma_corr_lut gammaCorrection;\n> +\t} contrast;\n\nMissing documentation :-)\n\n>  };\n>  \n>  struct IPAContext {\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index dc6f0735..ee0dd9fe 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -30,6 +30,7 @@\n>  #include \"libcamera/internal/mapped_framebuffer.h\"\n>  \n>  #include \"algorithms/algorithm.h\"\n> +#include \"algorithms/contrast.h\"\n>  #include \"ipu3_agc.h\"\n>  #include \"ipu3_awb.h\"\n>  #include \"libipa/camera_sensor_helper.h\"\n> @@ -218,6 +219,9 @@ int IPAIPU3::init(const IPASettings &settings,\n>  \n>  \t*ipaControls = ControlInfoMap(std::move(controls), controls::controls);\n>  \n> +\t/* Construct our Algorithms */\n> +\talgorithms_.emplace_back(new algorithms::Contrast());\n\nOr\n\n\talgorithms_.push_back(std::make_unique<algorithms::Contrast>());\n\n? It won't make much practical difference, but may better show we're\ndealing with unique pointers.\n\n> +\n>  \treturn 0;\n>  }\n>  \n> @@ -416,7 +420,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n>  \t\talgo->prepare(context_, params_);\n>  \n>  \tif (agcAlgo_->updateControls())\n> -\t\tawbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());\n> +\t\tawbAlgo_->updateWbParameters(params_);\n>  \n>  \t*params = params_;\n>  \n> @@ -431,13 +435,16 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>  \t\t\t      [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n>  {\n>  \tControlList ctrls(controls::controls);\n> +\t/* \\todo These fields should not be written by the IPAIPU3 layer */\n> +\tcontext_.frameContext.agc.gain = camHelper_->gain(gain_);\n> +\tcontext_.frameContext.agc.exposure = exposure_;\n>  \n>  \tfor (auto const &algo : algorithms_)\n>  \t\talgo->process(context_, stats);\n>  \n> -\tdouble gain = camHelper_->gain(gain_);\n> -\tagcAlgo_->process(stats, exposure_, gain);\n> -\tgain_ = camHelper_->gainCode(gain);\n> +\tagcAlgo_->process(context_, stats);\n> +\texposure_ = context_.frameContext.agc.exposure;\n> +\tgain_ = camHelper_->gainCode(context_.frameContext.agc.gain);\n>  \n>  \tawbAlgo_->calculateWBGains(stats);\n>  \n> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp\n> index 408eb849..c6782ff4 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> @@ -193,8 +190,10 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)\n>  \tlastFrame_ = frameCount_;\n>  }\n>  \n> -void IPU3Agc::process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain)\n> +void IPU3Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n>  {\n> +\tuint32_t &exposure = context.frameContext.agc.exposure;\n> +\tdouble &gain = context.frameContext.agc.gain;\n>  \tprocessBrightness(stats);\n>  \tlockExposureGain(exposure, gain);\n>  \tframeCount_++;\n> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n> index f00b98d6..f8290abd 100644\n> --- a/src/ipa/ipu3/ipu3_agc.h\n> +++ b/src/ipa/ipu3/ipu3_agc.h\n> @@ -30,11 +30,9 @@ public:\n>  \t~IPU3Agc() = default;\n>  \n>  \tvoid initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo);\n> -\tvoid process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain);\n> +\tvoid process(IPAContext &context, const ipu3_uapi_stats_3a *stats);\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> @@ -50,7 +48,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 4bb321b3..c2d9e0c1 100644\n> --- a/src/ipa/ipu3/ipu3_awb.cpp\n> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n> @@ -133,31 +133,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> @@ -197,10 +172,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> @@ -350,7 +321,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> @@ -362,18 +333,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 ea2d4320..eeb2920b 100644\n> --- a/src/ipa/ipu3/ipu3_awb.h\n> +++ b/src/ipa/ipu3/ipu3_awb.h\n> @@ -31,7 +31,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 6323DBD87D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 18 Aug 2021 21:42:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CA77668895;\n\tWed, 18 Aug 2021 23:42:02 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 286D36888A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 18 Aug 2021 23:42:01 +0200 (CEST)","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 99BEDEE;\n\tWed, 18 Aug 2021 23:42:00 +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=\"cajzUYII\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629322920;\n\tbh=MAa/llqRwKKSSvYp2geP4ONmU9WPW5St5OA6gqmDn1U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cajzUYIILB2nKf01WNS5ie7DhMMYT4ZHylkWpZQ9p8Hl5nSu/Bgg86S7S6a1TQdUi\n\tzcvo2nSqJn0EvsIBLwYcsaL/ZZcYSWsHTzqjDgffLFZjf3ojN8hiuCNeh9g4P4eIC7\n\tzqbdinqQLZz6+ULEaoXfn6M7Nha6AW+FEpLtgPXU=","Date":"Thu, 19 Aug 2021 00:41:52 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","Message-ID":"<YR1+oKJaHmfzJxxL@pendragon.ideasonboard.com>","References":"<20210818155403.268694-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210818155403.268694-6-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210818155403.268694-6-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 5/9] ipa: ipu3: Introduce a modular\n\tcontrast 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>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18944,"web_url":"https://patchwork.libcamera.org/comment/18944/","msgid":"<ebe2bb53-f45b-675f-6e3b-1fc037452b12@ideasonboard.com>","date":"2021-08-19T08:11:11","subject":"Re: [libcamera-devel] [PATCH v3 5/9] ipa: ipu3: Introduce a modular\n\tcontrast algorithm","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 18/08/2021 23:41, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> Thank you for the patch.\n> \n> On Wed, Aug 18, 2021 at 05:53:59PM +0200, Jean-Michel Hautbois wrote:\n>> Introduce a new algorithm to manage the contrast handling of the IPU3.\n>>\n>> The initial algorithm is chosen to configure the gamma contrast curve\n>> which moves the implementation out of AWB for simplicity. As it is\n>> initialised with a default gamma value of 1.1, there is no need to use\n>> the default table at initialisation anymore.\n>>\n>> This demonstrates the way to use process() call when the EventStatReady\n>> comes in. The function calculates the LUT in the context of a frame, and\n>> when prepare() is called, the parameters are filled with the updated\n>> values.\n>>\n>> AGC is modified to take the new process interface into account.\n>>\n>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>> ---\n>>  src/ipa/ipu3/algorithms/contrast.cpp | 53 ++++++++++++++++++++++++++++\n>>  src/ipa/ipu3/algorithms/contrast.h   | 32 +++++++++++++++++\n>>  src/ipa/ipu3/algorithms/meson.build  |  1 +\n>>  src/ipa/ipu3/ipa_context.h           |  8 +++++\n>>  src/ipa/ipu3/ipu3.cpp                | 15 +++++---\n>>  src/ipa/ipu3/ipu3_agc.cpp            |  9 +++--\n>>  src/ipa/ipu3/ipu3_agc.h              |  5 +--\n>>  src/ipa/ipu3/ipu3_awb.cpp            | 41 ++-------------------\n>>  src/ipa/ipu3/ipu3_awb.h              |  2 +-\n>>  9 files changed, 113 insertions(+), 53 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..eb92d3c7\n>> --- /dev/null\n>> +++ b/src/ipa/ipu3/algorithms/contrast.cpp\n>> @@ -0,0 +1,53 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Google inc.\n>> + *\n>> + * contrast.cpp - IPU3 Contrast and Gamma control\n>> + */\n>> +\n>> +#include \"contrast.h\"\n>> +\n>> +#include <cmath>\n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa::ipu3::algorithms {\n>> +\n>> +\n> \n> One extra blank line.\n> \n>> +Contrast::Contrast()\n>> +\t: gamma_(1.0)\n>> +{\n>> +}\n>> +\n>> +void Contrast::prepare([[maybe_unused]] IPAContext &context, ipu3_uapi_params &params)\n>> +{\n>> +\t/* Copy the calculated LUT into the parameters buffer */\n> \n> s/buffer/buffer./\n> \n>> +\tmemcpy(params.acc_param.gamma.gc_lut.lut,\n> \n> Missing header.\n> \n>> +\t       context.frameContext.contrast.gammaCorrection.lut,\n>> +\t       IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES * sizeof(params.acc_param.gamma.gc_lut.lut[0]));\n> \n> Line wrap:\n> \n> \t       IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES *\n> \t       sizeof(params.acc_param.gamma.gc_lut.lut[0]));\n> \n>> +\n>> +\t/* Enable the custom gamma table */\n> \n> s/table/table./\n> \n> I won't repeat this comment everywhere, you can apply it through the\n> series.\n> \n>> +\tparams.use.acc_gamma = 1;\n>> +\tparams.acc_param.gamma.gc_ctrl.enable = 1;\n>> +}\n>> +\n>> +void Contrast::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n> \n> Line wrap.\n> \n> I'll stop repeating this comment too :-)\n> \n>> +{\n>> +\t/* Limit the gamma effect for now */\n> \n> What do you mean by \"limit\" ? I understand it as\n> \n> \t/*\n> \t * Hardcode gamma to 1.1 as a default for now.\n> \t *\n> \t * \\todo Expose gamma control setting through the libcamera control API\n> \t */\n> \n>> +\t/* \\todo expose gamma control setting through the libcamera control API */\n>> +\tgamma_ = 1.1;\n>> +\n>> +\t/* Plot the gamma curve into the look up table */\n>> +\tfor (uint32_t i = 0; i < IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES; i++) {\n>> +\t\tdouble j = static_cast<double>(i)\n>> +\t\t\t / (IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1);\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> \n> I think \"The output value is expressed on 13 bits.\" would be clearer.\n> \n>> +\t\tcontext.frameContext.contrast.gammaCorrection.lut[i] = gamma * 8191;\n>> +\t}\n> \n> How about using std::size to avoid overflowing the array by mistake ?\n> \n> \tstruct ipu3_uapi_gamma_corr_lut &lut =\n> \t\tcontext.frameContext.contrast.gammaCorrection;\n> \n> \tfor (uint32_t i = 0; i < std::size(lut.lut); i++) {\n> \t\tdouble j = static_cast<double>(i) / (std::size(lut.lut) - 1);\n> \t\tdouble gamma = std::pow(j, 1.0 / gamma_);\n> \n> \t\t/* The output value is expressed on 13 bits. */\n> \t\tlut.lut[i] = gamma * 8191;\n> \t}\n> \n> Up to you.\n> \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..6cd6c5db\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 inc.\n>> + *\n>> + * contrast.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> I wonder of the algorithm shouldn't be called ToneMapping as it does\n> more than just contrast.\n> \n>> +{\n>> +public:\n>> +\tContrast();\n>> +\t~Contrast() = default;\n> \n> You can drop the destructor, it's not needed.\n> \n>> +\n>> +\tvoid prepare(IPAContext &context, ipu3_uapi_params &params) override;\n>> +\tvoid process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;\n> \n> Missing blank line.\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 dc538b79..e3ff3b78 100644\n>> --- a/src/ipa/ipu3/algorithms/meson.build\n>> +++ b/src/ipa/ipu3/algorithms/meson.build\n>> @@ -2,4 +2,5 @@\n>>  \n>>  ipu3_ipa_algorithms = files([\n>>      'algorithm.cpp',\n>> +    'contrast.cpp',\n>>  ])\n>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n>> index 46291d9e..5964ba6d 100644\n>> --- a/src/ipa/ipu3/ipa_context.h\n>> +++ b/src/ipa/ipu3/ipa_context.h\n>> @@ -24,6 +24,14 @@ struct IPAConfiguration {\n>>  };\n>>  \n>>  struct IPAFrameContext {\n>> +\tstruct Agc {\n>> +\t\tuint32_t exposure;\n>> +\t\tdouble gain;\n>> +\t} agc;\n> \n> I think this belongs to patch 7/9, along with the changes to\n> ipu3_agc.cpp and ipu3_agc.h.\n> \n>> +\n>> +\tstruct Contrast {\n>> +\t\tstruct ipu3_uapi_gamma_corr_lut gammaCorrection;\n>> +\t} contrast;\n> \n> Missing documentation :-)\n> \n\nShould I add the doc in tone_mapping.cpp (yes, I changed the name :-))\nor in ipu3.cpp as the rest of the structure ?\n\n>>  };\n>>  \n>>  struct IPAContext {\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index dc6f0735..ee0dd9fe 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -30,6 +30,7 @@\n>>  #include \"libcamera/internal/mapped_framebuffer.h\"\n>>  \n>>  #include \"algorithms/algorithm.h\"\n>> +#include \"algorithms/contrast.h\"\n>>  #include \"ipu3_agc.h\"\n>>  #include \"ipu3_awb.h\"\n>>  #include \"libipa/camera_sensor_helper.h\"\n>> @@ -218,6 +219,9 @@ int IPAIPU3::init(const IPASettings &settings,\n>>  \n>>  \t*ipaControls = ControlInfoMap(std::move(controls), controls::controls);\n>>  \n>> +\t/* Construct our Algorithms */\n>> +\talgorithms_.emplace_back(new algorithms::Contrast());\n> \n> Or\n> \n> \talgorithms_.push_back(std::make_unique<algorithms::Contrast>());\n> \n> ? It won't make much practical difference, but may better show we're\n> dealing with unique pointers.\n> \n\nOK for me !\n\n>> +\n>>  \treturn 0;\n>>  }\n>>  \n>> @@ -416,7 +420,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n>>  \t\talgo->prepare(context_, params_);\n>>  \n>>  \tif (agcAlgo_->updateControls())\n>> -\t\tawbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());\n>> +\t\tawbAlgo_->updateWbParameters(params_);\n>>  \n>>  \t*params = params_;\n>>  \n>> @@ -431,13 +435,16 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>>  \t\t\t      [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n>>  {\n>>  \tControlList ctrls(controls::controls);\n>> +\t/* \\todo These fields should not be written by the IPAIPU3 layer */\n>> +\tcontext_.frameContext.agc.gain = camHelper_->gain(gain_);\n>> +\tcontext_.frameContext.agc.exposure = exposure_;\n>>  \n>>  \tfor (auto const &algo : algorithms_)\n>>  \t\talgo->process(context_, stats);\n>>  \n>> -\tdouble gain = camHelper_->gain(gain_);\n>> -\tagcAlgo_->process(stats, exposure_, gain);\n>> -\tgain_ = camHelper_->gainCode(gain);\n>> +\tagcAlgo_->process(context_, stats);\n>> +\texposure_ = context_.frameContext.agc.exposure;\n>> +\tgain_ = camHelper_->gainCode(context_.frameContext.agc.gain);\n>>  \n>>  \tawbAlgo_->calculateWBGains(stats);\n>>  \n>> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp\n>> index 408eb849..c6782ff4 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>> @@ -193,8 +190,10 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)\n>>  \tlastFrame_ = frameCount_;\n>>  }\n>>  \n>> -void IPU3Agc::process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain)\n>> +void IPU3Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n>>  {\n>> +\tuint32_t &exposure = context.frameContext.agc.exposure;\n>> +\tdouble &gain = context.frameContext.agc.gain;\n>>  \tprocessBrightness(stats);\n>>  \tlockExposureGain(exposure, gain);\n>>  \tframeCount_++;\n>> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n>> index f00b98d6..f8290abd 100644\n>> --- a/src/ipa/ipu3/ipu3_agc.h\n>> +++ b/src/ipa/ipu3/ipu3_agc.h\n>> @@ -30,11 +30,9 @@ public:\n>>  \t~IPU3Agc() = default;\n>>  \n>>  \tvoid initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo);\n>> -\tvoid process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain);\n>> +\tvoid process(IPAContext &context, const ipu3_uapi_stats_3a *stats);\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>> @@ -50,7 +48,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 4bb321b3..c2d9e0c1 100644\n>> --- a/src/ipa/ipu3/ipu3_awb.cpp\n>> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n>> @@ -133,31 +133,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>> @@ -197,10 +172,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>> @@ -350,7 +321,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>> @@ -362,18 +333,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 ea2d4320..eeb2920b 100644\n>> --- a/src/ipa/ipu3/ipu3_awb.h\n>> +++ b/src/ipa/ipu3/ipu3_awb.h\n>> @@ -31,7 +31,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>>\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 5F63ABD87D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 19 Aug 2021 08:11:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C2A5168895;\n\tThu, 19 Aug 2021 10:11:15 +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 77AB86888E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 19 Aug 2021 10:11:14 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:d68d:3cb6:f3c7:c19c])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 197A52A8;\n\tThu, 19 Aug 2021 10:11:14 +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=\"dJ2grtCl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629360674;\n\tbh=szZAwSmi295Qxe3nDSXwNUxjYAyO3Y88LpayL8bau4U=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=dJ2grtClAQzdhLivq1mju7Okg07iLGv57G9CYTYel8EORQzedZwGFhiR/CQkpaiP+\n\tRG9mQa8vMUH2+0caxJJHIO0v3AieiDgtfCyMZeCZ5/zahlUKjQhAjiPdLEo2uYDgf3\n\tAPmZszhnt+jBrW3Tdhj59qYK7M3G3k7uUS0SA9bk=","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","References":"<20210818155403.268694-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210818155403.268694-6-jeanmichel.hautbois@ideasonboard.com>\n\t<YR1+oKJaHmfzJxxL@pendragon.ideasonboard.com>","From":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","Message-ID":"<ebe2bb53-f45b-675f-6e3b-1fc037452b12@ideasonboard.com>","Date":"Thu, 19 Aug 2021 10:11:11 +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":"<YR1+oKJaHmfzJxxL@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 5/9] ipa: ipu3: Introduce a modular\n\tcontrast 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>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18945,"web_url":"https://patchwork.libcamera.org/comment/18945/","msgid":"<YR4p/FT7s3g3K8ZH@pendragon.ideasonboard.com>","date":"2021-08-19T09:53:00","subject":"Re: [libcamera-devel] [PATCH v3 5/9] ipa: ipu3: Introduce a modular\n\tcontrast 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\nOn Thu, Aug 19, 2021 at 10:11:11AM +0200, Jean-Michel Hautbois wrote:\n> On 18/08/2021 23:41, Laurent Pinchart wrote:\n> > On Wed, Aug 18, 2021 at 05:53:59PM +0200, Jean-Michel Hautbois wrote:\n> >> Introduce a new algorithm to manage the contrast handling of the IPU3.\n> >>\n> >> The initial algorithm is chosen to configure the gamma contrast curve\n> >> which moves the implementation out of AWB for simplicity. As it is\n> >> initialised with a default gamma value of 1.1, there is no need to use\n> >> the default table at initialisation anymore.\n> >>\n> >> This demonstrates the way to use process() call when the EventStatReady\n> >> comes in. The function calculates the LUT in the context of a frame, and\n> >> when prepare() is called, the parameters are filled with the updated\n> >> values.\n> >>\n> >> AGC is modified to take the new process interface into account.\n> >>\n> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >> ---\n> >>  src/ipa/ipu3/algorithms/contrast.cpp | 53 ++++++++++++++++++++++++++++\n> >>  src/ipa/ipu3/algorithms/contrast.h   | 32 +++++++++++++++++\n> >>  src/ipa/ipu3/algorithms/meson.build  |  1 +\n> >>  src/ipa/ipu3/ipa_context.h           |  8 +++++\n> >>  src/ipa/ipu3/ipu3.cpp                | 15 +++++---\n> >>  src/ipa/ipu3/ipu3_agc.cpp            |  9 +++--\n> >>  src/ipa/ipu3/ipu3_agc.h              |  5 +--\n> >>  src/ipa/ipu3/ipu3_awb.cpp            | 41 ++-------------------\n> >>  src/ipa/ipu3/ipu3_awb.h              |  2 +-\n> >>  9 files changed, 113 insertions(+), 53 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..eb92d3c7\n> >> --- /dev/null\n> >> +++ b/src/ipa/ipu3/algorithms/contrast.cpp\n> >> @@ -0,0 +1,53 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2021, Google inc.\n> >> + *\n> >> + * contrast.cpp - IPU3 Contrast and Gamma control\n> >> + */\n> >> +\n> >> +#include \"contrast.h\"\n> >> +\n> >> +#include <cmath>\n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +namespace ipa::ipu3::algorithms {\n> >> +\n> >> +\n> > \n> > One extra blank line.\n> > \n> >> +Contrast::Contrast()\n> >> +\t: gamma_(1.0)\n> >> +{\n> >> +}\n> >> +\n> >> +void Contrast::prepare([[maybe_unused]] IPAContext &context, ipu3_uapi_params &params)\n> >> +{\n> >> +\t/* Copy the calculated LUT into the parameters buffer */\n> > \n> > s/buffer/buffer./\n> > \n> >> +\tmemcpy(params.acc_param.gamma.gc_lut.lut,\n> > \n> > Missing header.\n> > \n> >> +\t       context.frameContext.contrast.gammaCorrection.lut,\n> >> +\t       IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES * sizeof(params.acc_param.gamma.gc_lut.lut[0]));\n> > \n> > Line wrap:\n> > \n> > \t       IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES *\n> > \t       sizeof(params.acc_param.gamma.gc_lut.lut[0]));\n> > \n> >> +\n> >> +\t/* Enable the custom gamma table */\n> > \n> > s/table/table./\n> > \n> > I won't repeat this comment everywhere, you can apply it through the\n> > series.\n> > \n> >> +\tparams.use.acc_gamma = 1;\n> >> +\tparams.acc_param.gamma.gc_ctrl.enable = 1;\n> >> +}\n> >> +\n> >> +void Contrast::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n> > \n> > Line wrap.\n> > \n> > I'll stop repeating this comment too :-)\n> > \n> >> +{\n> >> +\t/* Limit the gamma effect for now */\n> > \n> > What do you mean by \"limit\" ? I understand it as\n> > \n> > \t/*\n> > \t * Hardcode gamma to 1.1 as a default for now.\n> > \t *\n> > \t * \\todo Expose gamma control setting through the libcamera control API\n> > \t */\n> > \n> >> +\t/* \\todo expose gamma control setting through the libcamera control API */\n> >> +\tgamma_ = 1.1;\n> >> +\n> >> +\t/* Plot the gamma curve into the look up table */\n> >> +\tfor (uint32_t i = 0; i < IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES; i++) {\n> >> +\t\tdouble j = static_cast<double>(i)\n> >> +\t\t\t / (IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1);\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> > \n> > I think \"The output value is expressed on 13 bits.\" would be clearer.\n> > \n> >> +\t\tcontext.frameContext.contrast.gammaCorrection.lut[i] = gamma * 8191;\n> >> +\t}\n> > \n> > How about using std::size to avoid overflowing the array by mistake ?\n> > \n> > \tstruct ipu3_uapi_gamma_corr_lut &lut =\n> > \t\tcontext.frameContext.contrast.gammaCorrection;\n> > \n> > \tfor (uint32_t i = 0; i < std::size(lut.lut); i++) {\n> > \t\tdouble j = static_cast<double>(i) / (std::size(lut.lut) - 1);\n> > \t\tdouble gamma = std::pow(j, 1.0 / gamma_);\n> > \n> > \t\t/* The output value is expressed on 13 bits. */\n> > \t\tlut.lut[i] = gamma * 8191;\n> > \t}\n> > \n> > Up to you.\n> > \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..6cd6c5db\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 inc.\n> >> + *\n> >> + * contrast.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> > I wonder of the algorithm shouldn't be called ToneMapping as it does\n> > more than just contrast.\n> > \n> >> +{\n> >> +public:\n> >> +\tContrast();\n> >> +\t~Contrast() = default;\n> > \n> > You can drop the destructor, it's not needed.\n> > \n> >> +\n> >> +\tvoid prepare(IPAContext &context, ipu3_uapi_params &params) override;\n> >> +\tvoid process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;\n> > \n> > Missing blank line.\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 dc538b79..e3ff3b78 100644\n> >> --- a/src/ipa/ipu3/algorithms/meson.build\n> >> +++ b/src/ipa/ipu3/algorithms/meson.build\n> >> @@ -2,4 +2,5 @@\n> >>  \n> >>  ipu3_ipa_algorithms = files([\n> >>      'algorithm.cpp',\n> >> +    'contrast.cpp',\n> >>  ])\n> >> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> >> index 46291d9e..5964ba6d 100644\n> >> --- a/src/ipa/ipu3/ipa_context.h\n> >> +++ b/src/ipa/ipu3/ipa_context.h\n> >> @@ -24,6 +24,14 @@ struct IPAConfiguration {\n> >>  };\n> >>  \n> >>  struct IPAFrameContext {\n> >> +\tstruct Agc {\n> >> +\t\tuint32_t exposure;\n> >> +\t\tdouble gain;\n> >> +\t} agc;\n> > \n> > I think this belongs to patch 7/9, along with the changes to\n> > ipu3_agc.cpp and ipu3_agc.h.\n> > \n> >> +\n> >> +\tstruct Contrast {\n> >> +\t\tstruct ipu3_uapi_gamma_corr_lut gammaCorrection;\n> >> +\t} contrast;\n> > \n> > Missing documentation :-)\n> \n> Should I add the doc in tone_mapping.cpp (yes, I changed the name :-))\n> or in ipu3.cpp as the rest of the structure ?\n\nThe structure is documented in ipu3.cpp, so that's where the field\nshould be documented. If desired we could move the Contrast structure\ndefinition to tone_mapping.cpp, and the documentation of the structure\nand its gammaCorrection field should then move to tone_mapping.cpp, but\nthe contract field of IPAFrameContext should go with the definition of\nIPAFrameContext itself.\n\n> >>  };\n> >>  \n> >>  struct IPAContext {\n> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >> index dc6f0735..ee0dd9fe 100644\n> >> --- a/src/ipa/ipu3/ipu3.cpp\n> >> +++ b/src/ipa/ipu3/ipu3.cpp\n> >> @@ -30,6 +30,7 @@\n> >>  #include \"libcamera/internal/mapped_framebuffer.h\"\n> >>  \n> >>  #include \"algorithms/algorithm.h\"\n> >> +#include \"algorithms/contrast.h\"\n> >>  #include \"ipu3_agc.h\"\n> >>  #include \"ipu3_awb.h\"\n> >>  #include \"libipa/camera_sensor_helper.h\"\n> >> @@ -218,6 +219,9 @@ int IPAIPU3::init(const IPASettings &settings,\n> >>  \n> >>  \t*ipaControls = ControlInfoMap(std::move(controls), controls::controls);\n> >>  \n> >> +\t/* Construct our Algorithms */\n> >> +\talgorithms_.emplace_back(new algorithms::Contrast());\n> > \n> > Or\n> > \n> > \talgorithms_.push_back(std::make_unique<algorithms::Contrast>());\n> > \n> > ? It won't make much practical difference, but may better show we're\n> > dealing with unique pointers.\n> \n> OK for me !\n> \n> >> +\n> >>  \treturn 0;\n> >>  }\n> >>  \n> >> @@ -416,7 +420,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> >>  \t\talgo->prepare(context_, params_);\n> >>  \n> >>  \tif (agcAlgo_->updateControls())\n> >> -\t\tawbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());\n> >> +\t\tawbAlgo_->updateWbParameters(params_);\n> >>  \n> >>  \t*params = params_;\n> >>  \n> >> @@ -431,13 +435,16 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> >>  \t\t\t      [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n> >>  {\n> >>  \tControlList ctrls(controls::controls);\n> >> +\t/* \\todo These fields should not be written by the IPAIPU3 layer */\n> >> +\tcontext_.frameContext.agc.gain = camHelper_->gain(gain_);\n> >> +\tcontext_.frameContext.agc.exposure = exposure_;\n> >>  \n> >>  \tfor (auto const &algo : algorithms_)\n> >>  \t\talgo->process(context_, stats);\n> >>  \n> >> -\tdouble gain = camHelper_->gain(gain_);\n> >> -\tagcAlgo_->process(stats, exposure_, gain);\n> >> -\tgain_ = camHelper_->gainCode(gain);\n> >> +\tagcAlgo_->process(context_, stats);\n> >> +\texposure_ = context_.frameContext.agc.exposure;\n> >> +\tgain_ = camHelper_->gainCode(context_.frameContext.agc.gain);\n> >>  \n> >>  \tawbAlgo_->calculateWBGains(stats);\n> >>  \n> >> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp\n> >> index 408eb849..c6782ff4 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> >> @@ -193,8 +190,10 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)\n> >>  \tlastFrame_ = frameCount_;\n> >>  }\n> >>  \n> >> -void IPU3Agc::process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain)\n> >> +void IPU3Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> >>  {\n> >> +\tuint32_t &exposure = context.frameContext.agc.exposure;\n> >> +\tdouble &gain = context.frameContext.agc.gain;\n> >>  \tprocessBrightness(stats);\n> >>  \tlockExposureGain(exposure, gain);\n> >>  \tframeCount_++;\n> >> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n> >> index f00b98d6..f8290abd 100644\n> >> --- a/src/ipa/ipu3/ipu3_agc.h\n> >> +++ b/src/ipa/ipu3/ipu3_agc.h\n> >> @@ -30,11 +30,9 @@ public:\n> >>  \t~IPU3Agc() = default;\n> >>  \n> >>  \tvoid initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo);\n> >> -\tvoid process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain);\n> >> +\tvoid process(IPAContext &context, const ipu3_uapi_stats_3a *stats);\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> >> @@ -50,7 +48,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 4bb321b3..c2d9e0c1 100644\n> >> --- a/src/ipa/ipu3/ipu3_awb.cpp\n> >> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n> >> @@ -133,31 +133,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> >> @@ -197,10 +172,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> >> @@ -350,7 +321,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> >> @@ -362,18 +333,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 ea2d4320..eeb2920b 100644\n> >> --- a/src/ipa/ipu3/ipu3_awb.h\n> >> +++ b/src/ipa/ipu3/ipu3_awb.h\n> >> @@ -31,7 +31,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 64BE9BD87D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 19 Aug 2021 09:53:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B313468892;\n\tThu, 19 Aug 2021 11:53:10 +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 7CEB96888E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 19 Aug 2021 11:53:08 +0200 (CEST)","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 E7CD12A8;\n\tThu, 19 Aug 2021 11:53:07 +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=\"nNn5Uwaj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629366788;\n\tbh=RXhYMIDHoFO0zo3hQoYgn1P/FC05y94ECPn7Oe6Hqbg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nNn5Uwajh9kipzODtkIyV8/G8fM6SNuoEb3exMu0asoIY6oClcIUPpTXGIKUclUQf\n\tLyfnLolja8+/GXJdAT6PkVj9nIG17Uw0wL8ClPAvs97Zjack/OV//NtO/PUXeyTU4t\n\t5cBxO6/nU9dyPgIEMZPiBsf1lzWygNzkW7gp5gzg=","Date":"Thu, 19 Aug 2021 12:53:00 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","Message-ID":"<YR4p/FT7s3g3K8ZH@pendragon.ideasonboard.com>","References":"<20210818155403.268694-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210818155403.268694-6-jeanmichel.hautbois@ideasonboard.com>\n\t<YR1+oKJaHmfzJxxL@pendragon.ideasonboard.com>\n\t<ebe2bb53-f45b-675f-6e3b-1fc037452b12@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<ebe2bb53-f45b-675f-6e3b-1fc037452b12@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 5/9] ipa: ipu3: Introduce a modular\n\tcontrast 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>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]