[{"id":18756,"web_url":"https://patchwork.libcamera.org/comment/18756/","msgid":"<2d0ac1d0-9c00-e160-f60f-c5005f6239c1@ideasonboard.com>","date":"2021-08-13T09:31:17","subject":"Re: [libcamera-devel] [PATCH v2 05/10] 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 12/08/2021 17:52, 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\nI don't think this is the 'initial algorithm' any more ...\n\n\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 init time anymore.\n\ns/at init time/at initialisation/\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 needs to be 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> ---\n>  src/ipa/ipu3/algorithms/algorithm.h  |  2 ++\n>  src/ipa/ipu3/algorithms/contrast.cpp | 51 ++++++++++++++++++++++++++++\n>  src/ipa/ipu3/algorithms/contrast.h   | 33 ++++++++++++++++++\n>  src/ipa/ipu3/algorithms/meson.build  |  3 +-\n>  src/ipa/ipu3/ipa_context.h           |  8 +++++\n>  src/ipa/ipu3/ipu3.cpp                | 12 +++++--\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>  10 files changed, 114 insertions(+), 52 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/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h\n> index b571f55a..13e5f2d4 100644\n> --- a/src/ipa/ipu3/algorithms/algorithm.h\n> +++ b/src/ipa/ipu3/algorithms/algorithm.h\n> @@ -28,6 +28,8 @@ public:\n>  \t}\n>  \n>  \tvirtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params &params) {}\n> +\n> +\tvirtual void process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats) {}\n>  };\n>  \n>  } /* namespace ipa::ipu3 */\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..bc00490f\n> --- /dev/null\n> +++ b/src/ipa/ipu3/algorithms/contrast.cpp\n> @@ -0,0 +1,51 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Ideas On Board\n\nKeep this consistent with contrast.h please.\n\n\n> + *\n> + * constrast.cpp - IPU3 Contrast and Gamma control\n\ns/constrast/contrast/\n\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> +}\n> +\n> +void Contrast::prepare([[maybe_unused]] IPAContext &context, ipu3_uapi_params &params)\n> +{\n> +\tparams.use.acc_gamma = 1;\n> +\tparams.acc_param.gamma.gc_ctrl.enable = 1;\n\nThe ordering is almost irrelevant to the code, but for some reason, I\nthink you should set the enable and use flags /after/ filling in the data.\n\nTechnically it doesn't matter, but conceptually, they should only be\nenabled, after they are filled in correctly.\n\n\n> +\n> +\tfor (uint32_t i = 0; i < IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES; i++)\n> +\t\tparams.acc_param.gamma.gc_lut.lut[i] = context.frameContext.contrast.lut[i];\n\nWe have two tables of a fixed number of entries.\nI wonder if a memcpy() would be more efficient than a loop, but perhaps\nit doesn't make a difference ?\n\nWould the compiler do the right thing if you said:\n\n  params.acc_param.gamma.gc_lut.lut = context.frameContext.contrast.lut;\n\nIn fact, I see that\n  params.acc_param.gamma.gc_lut.lut stores unsigned shorts,\nwhile\n  context.frameContext.contrast.lut stores uint16_t\n\nMaybe we should store a\n\tstruct ipu3_uapi_gamma_corr_lut\n\nin context.frameContext.contrast instead, Yes they're all the same size\ntypes - but we have access to the original type of the array, so why not\nuse it to keep them identical..\n\n\n\n> +}\n> +\n> +void Contrast::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n> +{\n> +\t/* Limit the gamma effect for now */\n\nCan this be explained any more? Why do we set this here and not\ninitialise to 1.1 in the initialisation list?\n\nPerhaps a\n   \\todo expose gamma control setting through the libcamera control API\n\nshould be added here too to make it clear that this is hardcoded, when\nit should somehow be a parameter or control into the IPA or such.\n\n\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 = 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\tcontext.frameContext.contrast.lut[i] = gamma * 8191;\n\n8191 still looks like a magic number, even with the comment.\nI wonder if we should define this somewhere, perhaps as\n\n\n/* The maximum value 255 is represented on 13 bits in the IPU3 */\n#define LUT_INTEGER_SCALE(g) (g * ((1 << 13) - 1))\n\n\tcontext.frameContext.contrast.lut[i] = LUT_INTEGER_SCALE(gamma);\n\n\nLUT_INTEGER_SCALE may be a completely inappropriate name ... I'm guessing.\n\n\nAlso - even having said that - this patch is moving existing code - so\nthis shouldn't be changed here - but it might be a distinct patch of\nit's own to come later.\n\n\n\n\n> +\t}\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..50cb626f\n> --- /dev/null\n> +++ b/src/ipa/ipu3/algorithms/contrast.h\n> @@ -0,0 +1,33 @@\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> +\tvoid prepare(IPAContext &context, ipu3_uapi_params &params) override;\n> +\tvoid process(IPAContext &context, const ipu3_uapi_stats_3a *stats) 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 3fb3ce56..69a59096 100644\n> --- a/src/ipa/ipu3/algorithms/meson.build\n> +++ b/src/ipa/ipu3/algorithms/meson.build\n> @@ -1,5 +1,6 @@\n>  # SPDX-License-Identifier: CC0-1.0\n>  \n>  ipu3_ipa_algorithms = files([\n> -        'grid.cpp',\n\nPlease fix this alignment in the patch that adds it.\n\n\n\n> +    'contrast.cpp',\n> +    'grid.cpp',\n>  ])\n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index 90b2f2c2..e995c663 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -33,6 +33,14 @@ struct IPAConfiguration {\n>   * lifetime, though for now a single instance is used.\n>   */\n>  struct IPAFrameContext {\n> +\tstruct Agc {\n> +\t\tuint32_t exposure;\n> +\t\tdouble gain;\n> +\t} agc;\n> +\n> +\tstruct Contrast {\n> +\t\tuint16_t lut[IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES];\n\nAs highilghted earlier, I think this should be:\n\t\tstruct ipu3_uapi_gamma_corr_lut lut\n\n\n> +\t} contrast;\n>  };\n>  \n>  /* Global context of the IPA */\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 394a7a45..e09adfc3 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 \"algorithms/grid.h\"\n>  #include \"ipu3_agc.h\"\n>  #include \"ipu3_awb.h\"\n> @@ -167,6 +168,7 @@ int IPAIPU3::init(const IPASettings &settings,\n>  \t*ipaControls = ControlInfoMap(std::move(controls), controls::controls);\n>  \t/* Construct our Algorithms */\n>  \talgorithms_.emplace_back(new algorithms::Grid());\n> +\talgorithms_.emplace_back(new algorithms::Contrast());\n>  \n>  \treturn 0;\n>  }\n> @@ -309,7 +311,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> @@ -324,9 +326,15 @@ 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\nNew line here to separate local variable declarations from code segments.\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> +\tcontext_.frameContext.agc.exposure = exposure_;\n> +\tcontext_.frameContext.agc.gain = gain;\n\nthe IPU3 layer sould not be filling in agc specific structures. That's a\nlayering violation to me.\n\nThe algorithms (in this instance, the agc?) own those structures, so\nthey should be manipulating them.\n\nIt's also showing where the algorithms need to know historical data.\nIn this instance, presumably they need to know what gain or expsosure\nwas set at the time that this buffer ran.\n\nThis shows where we will need to keep historical versions of the\nFrameContext, and perhaps track them along with the frame counter.\n\nFor now, while we get things moving to be more modular, I think we could\nkeep this as it is - but it should be commented with a todo\n\n\n> +\tagcAlgo_->process(context_, stats);\n> +\texposure_ = context_.frameContext.agc.exposure;\n> +\tgain = context_.frameContext.agc.gain;\n>  \tgain_ = camHelper_->gainCode(gain);\n>  \n>  \tawbAlgo_->calculateWBGains(stats);\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..8b32e52f 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) override;\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 911760b3..0f3bcdc9 100644\n> --- a/src/ipa/ipu3/ipu3_awb.cpp\n> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n> @@ -114,31 +114,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> @@ -179,10 +154,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> @@ -332,7 +303,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> @@ -344,18 +315,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 D98D4C3240\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 13 Aug 2021 09:31:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1EDB56888E;\n\tFri, 13 Aug 2021 11:31:23 +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 7A15960263\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 13 Aug 2021 11:31:21 +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 E19BBEE;\n\tFri, 13 Aug 2021 11:31:20 +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=\"Hj6N7zCc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628847081;\n\tbh=PA+/kPAQlNYmjUWi9kqf0iJy3Q0uJ3kJ/BNSAptUtPs=;\n\th=From:To:References:Subject:Date:In-Reply-To:From;\n\tb=Hj6N7zCcAtzLpW0xhrrf6sO1rorIPT41jE2CLW4II3jzxyk4fcAoZOfbETdS700lv\n\tnwkVqBm1js7tes+tIDUwwZrr2SUVmYhl9SFci6ojpxZzeACAWvHm8B0ngyY+6lHvOs\n\tp1MuBYc5yRWEQ/mWlcktdWqalPQcuORW7K3PBUbw=","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210812165243.276977-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210812165243.276977-6-jeanmichel.hautbois@ideasonboard.com>","Message-ID":"<2d0ac1d0-9c00-e160-f60f-c5005f6239c1@ideasonboard.com>","Date":"Fri, 13 Aug 2021 10:31:17 +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":"<20210812165243.276977-6-jeanmichel.hautbois@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 05/10] 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":18807,"web_url":"https://patchwork.libcamera.org/comment/18807/","msgid":"<YRmbAxPwzb0w4tKJ@pendragon.ideasonboard.com>","date":"2021-08-15T22:53:55","subject":"Re: [libcamera-devel] [PATCH v2 05/10] ipa: ipu3: Introduce a\n\tmodular contrast 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 Fri, Aug 13, 2021 at 10:31:17AM +0100, Kieran Bingham wrote:\n> On 12/08/2021 17:52, 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> \n> I don't think this is the 'initial algorithm' any more ...\n\nIt could in v3 unless there's a disagreement regarding my comments on\n03/10 though.\n\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 init time anymore.\n> \n> s/at init time/at initialisation/\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 needs to be 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> > ---\n> >  src/ipa/ipu3/algorithms/algorithm.h  |  2 ++\n> >  src/ipa/ipu3/algorithms/contrast.cpp | 51 ++++++++++++++++++++++++++++\n> >  src/ipa/ipu3/algorithms/contrast.h   | 33 ++++++++++++++++++\n> >  src/ipa/ipu3/algorithms/meson.build  |  3 +-\n> >  src/ipa/ipu3/ipa_context.h           |  8 +++++\n> >  src/ipa/ipu3/ipu3.cpp                | 12 +++++--\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> >  10 files changed, 114 insertions(+), 52 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/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h\n> > index b571f55a..13e5f2d4 100644\n> > --- a/src/ipa/ipu3/algorithms/algorithm.h\n> > +++ b/src/ipa/ipu3/algorithms/algorithm.h\n> > @@ -28,6 +28,8 @@ public:\n> >  \t}\n> >  \n> >  \tvirtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params &params) {}\n> > +\n> > +\tvirtual void process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats) {}\n\nLine wrap here too.\n\n> >  };\n> >  \n> >  } /* namespace ipa::ipu3 */\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..bc00490f\n> > --- /dev/null\n> > +++ b/src/ipa/ipu3/algorithms/contrast.cpp\n> > @@ -0,0 +1,51 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Ideas On Board\n> \n> Keep this consistent with contrast.h please.\n> \n> > + *\n> > + * constrast.cpp - IPU3 Contrast and Gamma control\n> \n> s/constrast/contrast/\n> \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\nThis isn't used, so you could drop it.\n\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> > +\tparams.use.acc_gamma = 1;\n> > +\tparams.acc_param.gamma.gc_ctrl.enable = 1;\n> \n> The ordering is almost irrelevant to the code, but for some reason, I\n> think you should set the enable and use flags /after/ filling in the data.\n> \n> Technically it doesn't matter, but conceptually, they should only be\n> enabled, after they are filled in correctly.\n\nI wonder if Kieran has his kernel developer hat on, thinking this will\nbe written to registers :-)\n\n> > +\n> > +\tfor (uint32_t i = 0; i < IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES; i++)\n> > +\t\tparams.acc_param.gamma.gc_lut.lut[i] = context.frameContext.contrast.lut[i];\n> \n> We have two tables of a fixed number of entries.\n> I wonder if a memcpy() would be more efficient than a loop, but perhaps\n> it doesn't make a difference ?\n\nIt shouldn't be less efficient at least.\n\n> Would the compiler do the right thing if you said:\n> \n>   params.acc_param.gamma.gc_lut.lut = context.frameContext.contrast.lut;\n\nYou can assign structs, but not arrays.\n\n> In fact, I see that\n>   params.acc_param.gamma.gc_lut.lut stores unsigned shorts,\n> while\n>   context.frameContext.contrast.lut stores uint16_t\n> \n> Maybe we should store a\n> \tstruct ipu3_uapi_gamma_corr_lut\n> \n> in context.frameContext.contrast instead, Yes they're all the same size\n> types - but we have access to the original type of the array, so why not\n> use it to keep them identical..\n> \n> > +}\n> > +\n> > +void Contrast::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n> > +{\n> > +\t/* Limit the gamma effect for now */\n> \n> Can this be explained any more? Why do we set this here and not\n> initialise to 1.1 in the initialisation list?\n> \n> Perhaps a\n>    \\todo expose gamma control setting through the libcamera control API\n> \n> should be added here too to make it clear that this is hardcoded, when\n> it should somehow be a parameter or control into the IPA or such.\n> \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 = 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\tcontext.frameContext.contrast.lut[i] = gamma * 8191;\n> \n> 8191 still looks like a magic number, even with the comment.\n\nSo does 255.0, which I assume is IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1.\nMaybe\n\n\t\tdouble j = static_cast<double>(i)\n\t\t\t / (IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1);\n\n> I wonder if we should define this somewhere, perhaps as\n> \n> /* The maximum value 255 is represented on 13 bits in the IPU3 */\n\nI'm not sure to understand the 255 here. The gamma table contains values\nin the [0.0, 1.0] range and maps them to [0, 8191]. 8191 thus\ncorresponds to 1.0, there's no 255 anywhere there.\n\n(By the way, I only assume that 8191 is 1.0, I wonder if it could be\n8192 that is mapped to 1.0, in which case the highest possible value\nwould be 0.9998779296875)\n\n> #define LUT_INTEGER_SCALE(g) (g * ((1 << 13) - 1))\n> \n> \tcontext.frameContext.contrast.lut[i] = LUT_INTEGER_SCALE(gamma);\n> \n> LUT_INTEGER_SCALE may be a completely inappropriate name ... I'm guessing.\n\nDo we need to round to the closest integer instead of the lower one, or\ndoes this make little difference ?\n\n> Also - even having said that - this patch is moving existing code - so\n> this shouldn't be changed here - but it might be a distinct patch of\n> it's own to come later.\n\nA fixed point helper template class, with template parameters for the\nnumber of integer bits and the number of fractional bits:\n\ntemplate<unsigned int, unsigned int>\nclass FixedPoint\n{\n\t...\n};\n\nwith conversion functions, in a fashion similar to\nstd::chrono::duration. Something for later.\n\n> > +\t}\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..50cb626f\n> > --- /dev/null\n> > +++ b/src/ipa/ipu3/algorithms/contrast.h\n> > @@ -0,0 +1,33 @@\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\nI don't think this is needed, and if it is, let's not make it an inline\nfunction, you can define it in contrast.cpp as\n\nContrast::~Contrast() = default;\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> > +private:\n> > +\tdouble gamma_;\n\nI wonder if the gamma value should be stored in the context structure. A\nbit more design documentation could answer this question :-)\n\nCould you also please explain (just as a reply to this e-mail, not in\ncomments) why the gamma value was provided by the AGC algorithm, and the\ngamma LUT filled by the AWB algorithm ? How does gamma relate, in\ngenetal, to AGC and/or AWB ?\n\nAlso, this doesn't handle contrast at the moment. Do you plan to expand\nthe algorithm ? I also wonder if it shouldn't be called tonemapping\ninstead of contrast.\n\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 3fb3ce56..69a59096 100644\n> > --- a/src/ipa/ipu3/algorithms/meson.build\n> > +++ b/src/ipa/ipu3/algorithms/meson.build\n> > @@ -1,5 +1,6 @@\n> >  # SPDX-License-Identifier: CC0-1.0\n> >  \n> >  ipu3_ipa_algorithms = files([\n> > -        'grid.cpp',\n> \n> Please fix this alignment in the patch that adds it.\n> \n> > +    'contrast.cpp',\n> > +    'grid.cpp',\n> >  ])\n> > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> > index 90b2f2c2..e995c663 100644\n> > --- a/src/ipa/ipu3/ipa_context.h\n> > +++ b/src/ipa/ipu3/ipa_context.h\n> > @@ -33,6 +33,14 @@ struct IPAConfiguration {\n> >   * lifetime, though for now a single instance is used.\n> >   */\n> >  struct IPAFrameContext {\n> > +\tstruct Agc {\n> > +\t\tuint32_t exposure;\n> > +\t\tdouble gain;\n> > +\t} agc;\n> > +\n> > +\tstruct Contrast {\n> > +\t\tuint16_t lut[IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES];\n> \n> As highilghted earlier, I think this should be:\n> \t\tstruct ipu3_uapi_gamma_corr_lut lut\n> \n> > +\t} contrast;\n> >  };\n> >  \n> >  /* Global context of the IPA */\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index 394a7a45..e09adfc3 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 \"algorithms/grid.h\"\n> >  #include \"ipu3_agc.h\"\n> >  #include \"ipu3_awb.h\"\n> > @@ -167,6 +168,7 @@ int IPAIPU3::init(const IPASettings &settings,\n> >  \t*ipaControls = ControlInfoMap(std::move(controls), controls::controls);\n> >  \t/* Construct our Algorithms */\n> >  \talgorithms_.emplace_back(new algorithms::Grid());\n> > +\talgorithms_.emplace_back(new algorithms::Contrast());\n> >  \n> >  \treturn 0;\n> >  }\n> > @@ -309,7 +311,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> > @@ -324,9 +326,15 @@ 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> \n> New line here to separate local variable declarations from code segments.\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> > +\tcontext_.frameContext.agc.exposure = exposure_;\n> > +\tcontext_.frameContext.agc.gain = gain;\n> \n> the IPU3 layer sould not be filling in agc specific structures. That's a\n> layering violation to me.\n> \n> The algorithms (in this instance, the agc?) own those structures, so\n> they should be manipulating them.\n\nI think I agree, but how would you pass the exposure and gain parameters\nto the algorithm in that case ?\n\n> It's also showing where the algorithms need to know historical data.\n> In this instance, presumably they need to know what gain or expsosure\n> was set at the time that this buffer ran.\n> \n> This shows where we will need to keep historical versions of the\n> FrameContext, and perhaps track them along with the frame counter.\n> \n> For now, while we get things moving to be more modular, I think we could\n> keep this as it is - but it should be commented with a todo\n> \n> > +\tagcAlgo_->process(context_, stats);\n> > +\texposure_ = context_.frameContext.agc.exposure;\n> > +\tgain = context_.frameContext.agc.gain;\n> >  \tgain_ = camHelper_->gainCode(gain);\n> >  \n> >  \tawbAlgo_->calculateWBGains(stats);\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..8b32e52f 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) override;\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 911760b3..0f3bcdc9 100644\n> > --- a/src/ipa/ipu3/ipu3_awb.cpp\n> > +++ b/src/ipa/ipu3/ipu3_awb.cpp\n> > @@ -114,31 +114,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> > @@ -179,10 +154,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> > @@ -332,7 +303,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> > @@ -344,18 +315,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;","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 A8CD7BD87C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 15 Aug 2021 22:54:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 140A86888C;\n\tMon, 16 Aug 2021 00:54:04 +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 5F2C060261\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 16 Aug 2021 00:54:02 +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 BBC512C5;\n\tMon, 16 Aug 2021 00:54:01 +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=\"p+sJ9ija\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629068041;\n\tbh=RwKI9Ew7urD4y5tDwF3A63kuKf5OW18BaDCqBGDqGMs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=p+sJ9ijaZc4DF0/9bK71w2M4zhyDx8wG88cdg3e2FeKarc1tHbuSpjfM8UtFEE9sf\n\tbNIXv0QYbsLrp7hOq7L5WqbADKy15OHmmVKhIYZd9Mm6YVwLeZnCeUHcVfJ3lDe5i/\n\tjrk/tTZ7Nqn79mWhpF1bIu7lu3BHVUeS9cw+Fo5Q=","Date":"Mon, 16 Aug 2021 01:53:55 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YRmbAxPwzb0w4tKJ@pendragon.ideasonboard.com>","References":"<20210812165243.276977-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210812165243.276977-6-jeanmichel.hautbois@ideasonboard.com>\n\t<2d0ac1d0-9c00-e160-f60f-c5005f6239c1@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<2d0ac1d0-9c00-e160-f60f-c5005f6239c1@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 05/10] 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>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18818,"web_url":"https://patchwork.libcamera.org/comment/18818/","msgid":"<12b70465-1767-2c5b-3168-c2c537f3c0e2@ideasonboard.com>","date":"2021-08-16T07:47:57","subject":"Re: [libcamera-devel] [PATCH v2 05/10] 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 Laurent,\n\nOn 16/08/2021 00:53, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> On Fri, Aug 13, 2021 at 10:31:17AM +0100, Kieran Bingham wrote:\n>> On 12/08/2021 17:52, 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>>\n>> I don't think this is the 'initial algorithm' any more ...\n> \n> It could in v3 unless there's a disagreement regarding my comments on\n> 03/10 though.\n> \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 init time anymore.\n>>\n>> s/at init time/at initialisation/\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 needs to be 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>>> ---\n>>>  src/ipa/ipu3/algorithms/algorithm.h  |  2 ++\n>>>  src/ipa/ipu3/algorithms/contrast.cpp | 51 ++++++++++++++++++++++++++++\n>>>  src/ipa/ipu3/algorithms/contrast.h   | 33 ++++++++++++++++++\n>>>  src/ipa/ipu3/algorithms/meson.build  |  3 +-\n>>>  src/ipa/ipu3/ipa_context.h           |  8 +++++\n>>>  src/ipa/ipu3/ipu3.cpp                | 12 +++++--\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>>>  10 files changed, 114 insertions(+), 52 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/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h\n>>> index b571f55a..13e5f2d4 100644\n>>> --- a/src/ipa/ipu3/algorithms/algorithm.h\n>>> +++ b/src/ipa/ipu3/algorithms/algorithm.h\n>>> @@ -28,6 +28,8 @@ public:\n>>>  \t}\n>>>  \n>>>  \tvirtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params &params) {}\n>>> +\n>>> +\tvirtual void process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats) {}\n> \n> Line wrap here too.\n> \n>>>  };\n>>>  \n>>>  } /* namespace ipa::ipu3 */\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..bc00490f\n>>> --- /dev/null\n>>> +++ b/src/ipa/ipu3/algorithms/contrast.cpp\n>>> @@ -0,0 +1,51 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Copyright (C) 2021, Ideas On Board\n>>\n>> Keep this consistent with contrast.h please.\n>>\n>>> + *\n>>> + * constrast.cpp - IPU3 Contrast and Gamma control\n>>\n>> s/constrast/contrast/\n>>\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> This isn't used, so you could drop it.\n> \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>>> +\tparams.use.acc_gamma = 1;\n>>> +\tparams.acc_param.gamma.gc_ctrl.enable = 1;\n>>\n>> The ordering is almost irrelevant to the code, but for some reason, I\n>> think you should set the enable and use flags /after/ filling in the data.\n>>\n>> Technically it doesn't matter, but conceptually, they should only be\n>> enabled, after they are filled in correctly.\n> \n> I wonder if Kieran has his kernel developer hat on, thinking this will\n> be written to registers :-)\n> \n>>> +\n>>> +\tfor (uint32_t i = 0; i < IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES; i++)\n>>> +\t\tparams.acc_param.gamma.gc_lut.lut[i] = context.frameContext.contrast.lut[i];\n>>\n>> We have two tables of a fixed number of entries.\n>> I wonder if a memcpy() would be more efficient than a loop, but perhaps\n>> it doesn't make a difference ?\n> \n> It shouldn't be less efficient at least.\n> \n>> Would the compiler do the right thing if you said:\n>>\n>>   params.acc_param.gamma.gc_lut.lut = context.frameContext.contrast.lut;\n> \n> You can assign structs, but not arrays.\n\nYes, memcpy is probably a better option ;-).\n\n>> In fact, I see that\n>>   params.acc_param.gamma.gc_lut.lut stores unsigned shorts,\n>> while\n>>   context.frameContext.contrast.lut stores uint16_t\n>>\n>> Maybe we should store a\n>> \tstruct ipu3_uapi_gamma_corr_lut\n>>\n>> in context.frameContext.contrast instead, Yes they're all the same size\n>> types - but we have access to the original type of the array, so why not\n>> use it to keep them identical..\n>>\n>>> +}\n>>> +\n>>> +void Contrast::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n>>> +{\n>>> +\t/* Limit the gamma effect for now */\n>>\n>> Can this be explained any more? Why do we set this here and not\n>> initialise to 1.1 in the initialisation list?\n>>\n>> Perhaps a\n>>    \\todo expose gamma control setting through the libcamera control API\n>>\n>> should be added here too to make it clear that this is hardcoded, when\n>> it should somehow be a parameter or control into the IPA or such.\n>>\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 = 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\tcontext.frameContext.contrast.lut[i] = gamma * 8191;\n>>\n>> 8191 still looks like a magic number, even with the comment.\n> \n> So does 255.0, which I assume is IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1.\n> Maybe\n> \n> \t\tdouble j = static_cast<double>(i)\n> \t\t\t / (IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1);\n> \n>> I wonder if we should define this somewhere, perhaps as\n>>\n>> /* The maximum value 255 is represented on 13 bits in the IPU3 */\n> \n> I'm not sure to understand the 255 here. The gamma table contains values\n> in the [0.0, 1.0] range and maps them to [0, 8191]. 8191 thus\n> corresponds to 1.0, there's no 255 anywhere there.\n> \n> (By the way, I only assume that 8191 is 1.0, I wonder if it could be\n> 8192 that is mapped to 1.0, in which case the highest possible value\n> would be 0.9998779296875)\n> \n\nWell, the maximum is 8191 (we have 8192 values). Each input pixel is\nbetween 0 and 255 and this is mapped to a new value between 0 and 8191.\n\n\n>> #define LUT_INTEGER_SCALE(g) (g * ((1 << 13) - 1))\n>>\n>> \tcontext.frameContext.contrast.lut[i] = LUT_INTEGER_SCALE(gamma);\n>>\n>> LUT_INTEGER_SCALE may be a completely inappropriate name ... I'm guessing.\n> \n> Do we need to round to the closest integer instead of the lower one, or\n> does this make little difference ?\n> \n>> Also - even having said that - this patch is moving existing code - so\n>> this shouldn't be changed here - but it might be a distinct patch of\n>> it's own to come later.\n> \n> A fixed point helper template class, with template parameters for the\n> number of integer bits and the number of fractional bits:\n> \n> template<unsigned int, unsigned int>\n> class FixedPoint\n> {\n> \t...\n> };\n> \n> with conversion functions, in a fashion similar to\n> std::chrono::duration. Something for later.\n> \n>>> +\t}\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..50cb626f\n>>> --- /dev/null\n>>> +++ b/src/ipa/ipu3/algorithms/contrast.h\n>>> @@ -0,0 +1,33 @@\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> I don't think this is needed, and if it is, let's not make it an inline\n> function, you can define it in contrast.cpp as\n> \n> Contrast::~Contrast() = default;\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>>> +private:\n>>> +\tdouble gamma_;\n> \n> I wonder if the gamma value should be stored in the context structure. A\n> bit more design documentation could answer this question :-)\n> \n> Could you also please explain (just as a reply to this e-mail, not in\n> comments) why the gamma value was provided by the AGC algorithm, and the\n> gamma LUT filled by the AWB algorithm ? How does gamma relate, in\n> genetal, to AGC and/or AWB ?\n\nThe original AGC algorithm comes partially from a paper \"A New Auto\nExposure and Auto White-Balance Algorithm to Detect High Dynamic Range\nConditions Using CMOS Technology\", Quoc Kien Vuong, Se-Hwan Yun, and\nSuki Kim\n\nThe idea was to be able to distinguish when we are in a back lighting\ncondition, in particular. On this original algorithm, adding a\ncalculated gamma value to compensate the over/under exposure was used.\n\nThis has been abandonned but beeing able to change the default gamma\ncorrection table is interesting.\nNow, the gamma correction should not be controlled by AGC or AWB, but on\nits own, as well as the ipu3_uapi_isp_lin_vmem_params structure, which\nreally controls the contrast (as it is here to linearize the non linear\nsensor).\n\n> Also, this doesn't handle contrast at the moment. Do you plan to expand\n> the algorithm ? I also wonder if it shouldn't be called tonemapping\n> instead of contrast.\n> \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 3fb3ce56..69a59096 100644\n>>> --- a/src/ipa/ipu3/algorithms/meson.build\n>>> +++ b/src/ipa/ipu3/algorithms/meson.build\n>>> @@ -1,5 +1,6 @@\n>>>  # SPDX-License-Identifier: CC0-1.0\n>>>  \n>>>  ipu3_ipa_algorithms = files([\n>>> -        'grid.cpp',\n>>\n>> Please fix this alignment in the patch that adds it.\n>>\n>>> +    'contrast.cpp',\n>>> +    'grid.cpp',\n>>>  ])\n>>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n>>> index 90b2f2c2..e995c663 100644\n>>> --- a/src/ipa/ipu3/ipa_context.h\n>>> +++ b/src/ipa/ipu3/ipa_context.h\n>>> @@ -33,6 +33,14 @@ struct IPAConfiguration {\n>>>   * lifetime, though for now a single instance is used.\n>>>   */\n>>>  struct IPAFrameContext {\n>>> +\tstruct Agc {\n>>> +\t\tuint32_t exposure;\n>>> +\t\tdouble gain;\n>>> +\t} agc;\n>>> +\n>>> +\tstruct Contrast {\n>>> +\t\tuint16_t lut[IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES];\n>>\n>> As highilghted earlier, I think this should be:\n>> \t\tstruct ipu3_uapi_gamma_corr_lut lut\n>>\n>>> +\t} contrast;\n>>>  };\n>>>  \n>>>  /* Global context of the IPA */\n>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>> index 394a7a45..e09adfc3 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 \"algorithms/grid.h\"\n>>>  #include \"ipu3_agc.h\"\n>>>  #include \"ipu3_awb.h\"\n>>> @@ -167,6 +168,7 @@ int IPAIPU3::init(const IPASettings &settings,\n>>>  \t*ipaControls = ControlInfoMap(std::move(controls), controls::controls);\n>>>  \t/* Construct our Algorithms */\n>>>  \talgorithms_.emplace_back(new algorithms::Grid());\n>>> +\talgorithms_.emplace_back(new algorithms::Contrast());\n>>>  \n>>>  \treturn 0;\n>>>  }\n>>> @@ -309,7 +311,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>>> @@ -324,9 +326,15 @@ 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>>\n>> New line here to separate local variable declarations from code segments.\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>>> +\tcontext_.frameContext.agc.exposure = exposure_;\n>>> +\tcontext_.frameContext.agc.gain = gain;\n>>\n>> the IPU3 layer sould not be filling in agc specific structures. That's a\n>> layering violation to me.\n>>\n>> The algorithms (in this instance, the agc?) own those structures, so\n>> they should be manipulating them.\n> \n> I think I agree, but how would you pass the exposure and gain parameters\n> to the algorithm in that case ?\n\nKieran, I agree with Laurent on this :-)\n\n>> It's also showing where the algorithms need to know historical data.\n>> In this instance, presumably they need to know what gain or expsosure\n>> was set at the time that this buffer ran.\n>>\n>> This shows where we will need to keep historical versions of the\n>> FrameContext, and perhaps track them along with the frame counter.\n>>\n>> For now, while we get things moving to be more modular, I think we could\n>> keep this as it is - but it should be commented with a todo\n>>\n>>> +\tagcAlgo_->process(context_, stats);\n>>> +\texposure_ = context_.frameContext.agc.exposure;\n>>> +\tgain = context_.frameContext.agc.gain;\n>>>  \tgain_ = camHelper_->gainCode(gain);\n>>>  \n>>>  \tawbAlgo_->calculateWBGains(stats);\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..8b32e52f 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) override;\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 911760b3..0f3bcdc9 100644\n>>> --- a/src/ipa/ipu3/ipu3_awb.cpp\n>>> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n>>> @@ -114,31 +114,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>>> @@ -179,10 +154,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>>> @@ -332,7 +303,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>>> @@ -344,18 +315,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 67F30BD87D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 16 Aug 2021 07:48:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E353C68890;\n\tMon, 16 Aug 2021 09:48:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3409360260\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 16 Aug 2021 09:48:00 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:8b4d:65fb:4074:a212])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C03E28F;\n\tMon, 16 Aug 2021 09:47:59 +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=\"KXzAkK7/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629100079;\n\tbh=En+WEMzxlWiz0ploEHhfJ60sFRq7BsudsOKBGymaO9w=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=KXzAkK7/oFrqIBpiCYijubbmaaSxuHOjzdVwKrg5/PwuLSV9mTgGBKR/98vEkx0H2\n\tmr2knN3t0Ew7g/OKOVUOmXLmrMNon4CE7ciRys0Becp4z1DxdZ4D++vsn8LRX9Cbak\n\t6uIBZ/qPhSnYpEjF3BGopbipszBDAtd32t0jfSug=","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tKieran Bingham <kieran.bingham@ideasonboard.com>","References":"<20210812165243.276977-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210812165243.276977-6-jeanmichel.hautbois@ideasonboard.com>\n\t<2d0ac1d0-9c00-e160-f60f-c5005f6239c1@ideasonboard.com>\n\t<YRmbAxPwzb0w4tKJ@pendragon.ideasonboard.com>","From":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","Message-ID":"<12b70465-1767-2c5b-3168-c2c537f3c0e2@ideasonboard.com>","Date":"Mon, 16 Aug 2021 09:47:57 +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":"<YRmbAxPwzb0w4tKJ@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 05/10] 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>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18821,"web_url":"https://patchwork.libcamera.org/comment/18821/","msgid":"<c7e4a3b4-c931-b473-326d-74337faace93@ideasonboard.com>","date":"2021-08-16T08:17:47","subject":"Re: [libcamera-devel] [PATCH v2 05/10] 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 16/08/2021 08:47, Jean-Michel Hautbois wrote:\n> Hi Laurent,\n> \n> On 16/08/2021 00:53, Laurent Pinchart wrote:\n>> Hi Jean-Michel,\n>>\n>> On Fri, Aug 13, 2021 at 10:31:17AM +0100, Kieran Bingham wrote:\n>>> On 12/08/2021 17:52, 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>>>\n>>> I don't think this is the 'initial algorithm' any more ...\n>>\n>> It could in v3 unless there's a disagreement regarding my comments on\n>> 03/10 though.\n>>\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 init time anymore.\n>>>\n>>> s/at init time/at initialisation/\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 needs to be 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>>>> ---\n>>>>  src/ipa/ipu3/algorithms/algorithm.h  |  2 ++\n>>>>  src/ipa/ipu3/algorithms/contrast.cpp | 51 ++++++++++++++++++++++++++++\n>>>>  src/ipa/ipu3/algorithms/contrast.h   | 33 ++++++++++++++++++\n>>>>  src/ipa/ipu3/algorithms/meson.build  |  3 +-\n>>>>  src/ipa/ipu3/ipa_context.h           |  8 +++++\n>>>>  src/ipa/ipu3/ipu3.cpp                | 12 +++++--\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>>>>  10 files changed, 114 insertions(+), 52 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/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h\n>>>> index b571f55a..13e5f2d4 100644\n>>>> --- a/src/ipa/ipu3/algorithms/algorithm.h\n>>>> +++ b/src/ipa/ipu3/algorithms/algorithm.h\n>>>> @@ -28,6 +28,8 @@ public:\n>>>>  \t}\n>>>>  \n>>>>  \tvirtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params &params) {}\n>>>> +\n>>>> +\tvirtual void process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats) {}\n>>\n>> Line wrap here too.\n>>\n>>>>  };\n>>>>  \n>>>>  } /* namespace ipa::ipu3 */\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..bc00490f\n>>>> --- /dev/null\n>>>> +++ b/src/ipa/ipu3/algorithms/contrast.cpp\n>>>> @@ -0,0 +1,51 @@\n>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>>> +/*\n>>>> + * Copyright (C) 2021, Ideas On Board\n>>>\n>>> Keep this consistent with contrast.h please.\n>>>\n>>>> + *\n>>>> + * constrast.cpp - IPU3 Contrast and Gamma control\n>>>\n>>> s/constrast/contrast/\n>>>\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>> This isn't used, so you could drop it.\n>>\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>>>> +\tparams.use.acc_gamma = 1;\n>>>> +\tparams.acc_param.gamma.gc_ctrl.enable = 1;\n>>>\n>>> The ordering is almost irrelevant to the code, but for some reason, I\n>>> think you should set the enable and use flags /after/ filling in the data.\n>>>\n>>> Technically it doesn't matter, but conceptually, they should only be\n>>> enabled, after they are filled in correctly.\n>>\n>> I wonder if Kieran has his kernel developer hat on, thinking this will\n>> be written to registers :-)\n\nOf course ;-)\n\nBut perhaps it makes more sense to me to say something is 'valid' after\nit is configured. Like I said - it doesn't matter to the code or the\ncompiler here, so there's no requirement from me, it was just a comment.\n\n\n>>\n>>>> +\n>>>> +\tfor (uint32_t i = 0; i < IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES; i++)\n>>>> +\t\tparams.acc_param.gamma.gc_lut.lut[i] = context.frameContext.contrast.lut[i];\n>>>\n>>> We have two tables of a fixed number of entries.\n>>> I wonder if a memcpy() would be more efficient than a loop, but perhaps\n>>> it doesn't make a difference ?\n>>\n>> It shouldn't be less efficient at least.\n>>\n>>> Would the compiler do the right thing if you said:\n>>>\n>>>   params.acc_param.gamma.gc_lut.lut = context.frameContext.contrast.lut;\n>>\n>> You can assign structs, but not arrays.\n> \n> Yes, memcpy is probably a better option ;-).\n> \n>>> In fact, I see that\n>>>   params.acc_param.gamma.gc_lut.lut stores unsigned shorts,\n>>> while\n>>>   context.frameContext.contrast.lut stores uint16_t\n>>>\n>>> Maybe we should store a\n>>> \tstruct ipu3_uapi_gamma_corr_lut\n>>>\n>>> in context.frameContext.contrast instead, Yes they're all the same size\n>>> types - but we have access to the original type of the array, so why not\n>>> use it to keep them identical..\n>>>\n>>>> +}\n>>>> +\n>>>> +void Contrast::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n>>>> +{\n>>>> +\t/* Limit the gamma effect for now */\n>>>\n>>> Can this be explained any more? Why do we set this here and not\n>>> initialise to 1.1 in the initialisation list?\n>>>\n>>> Perhaps a\n>>>    \\todo expose gamma control setting through the libcamera control API\n>>>\n>>> should be added here too to make it clear that this is hardcoded, when\n>>> it should somehow be a parameter or control into the IPA or such.\n>>>\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 = 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\tcontext.frameContext.contrast.lut[i] = gamma * 8191;\n>>>\n>>> 8191 still looks like a magic number, even with the comment.\n>>\n>> So does 255.0, which I assume is IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1.\n>> Maybe\n>>\n>> \t\tdouble j = static_cast<double>(i)\n>> \t\t\t / (IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1);\n>>\n>>> I wonder if we should define this somewhere, perhaps as\n>>>\n>>> /* The maximum value 255 is represented on 13 bits in the IPU3 */\n>>\n>> I'm not sure to understand the 255 here. The gamma table contains values\n>> in the [0.0, 1.0] range and maps them to [0, 8191]. 8191 thus\n>> corresponds to 1.0, there's no 255 anywhere there.\n>>\n>> (By the way, I only assume that 8191 is 1.0, I wonder if it could be\n>> 8192 that is mapped to 1.0, in which case the highest possible value\n>> would be 0.9998779296875)\n>>\n> \n> Well, the maximum is 8191 (we have 8192 values). Each input pixel is\n> between 0 and 255 and this is mapped to a new value between 0 and 8191.\n> \n> \n>>> #define LUT_INTEGER_SCALE(g) (g * ((1 << 13) - 1))\n>>>\n>>> \tcontext.frameContext.contrast.lut[i] = LUT_INTEGER_SCALE(gamma);\n>>>\n>>> LUT_INTEGER_SCALE may be a completely inappropriate name ... I'm guessing.\n>>\n>> Do we need to round to the closest integer instead of the lower one, or\n>> does this make little difference ?\n>>\n>>> Also - even having said that - this patch is moving existing code - so\n>>> this shouldn't be changed here - but it might be a distinct patch of\n>>> it's own to come later.\n>>\n>> A fixed point helper template class, with template parameters for the\n>> number of integer bits and the number of fractional bits:\n>>\n>> template<unsigned int, unsigned int>\n>> class FixedPoint\n>> {\n>> \t...\n>> };\n>>\n>> with conversion functions, in a fashion similar to\n>> std::chrono::duration. Something for later.\n>>\n>>>> +\t}\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..50cb626f\n>>>> --- /dev/null\n>>>> +++ b/src/ipa/ipu3/algorithms/contrast.h\n>>>> @@ -0,0 +1,33 @@\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>> I don't think this is needed, and if it is, let's not make it an inline\n>> function, you can define it in contrast.cpp as\n>>\n>> Contrast::~Contrast() = default;\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>>>> +private:\n>>>> +\tdouble gamma_;\n>>\n>> I wonder if the gamma value should be stored in the context structure. A\n>> bit more design documentation could answer this question :-)\n>>\n>> Could you also please explain (just as a reply to this e-mail, not in\n>> comments) why the gamma value was provided by the AGC algorithm, and the\n>> gamma LUT filled by the AWB algorithm ? How does gamma relate, in\n>> genetal, to AGC and/or AWB ?\n> \n> The original AGC algorithm comes partially from a paper \"A New Auto\n> Exposure and Auto White-Balance Algorithm to Detect High Dynamic Range\n> Conditions Using CMOS Technology\", Quoc Kien Vuong, Se-Hwan Yun, and\n> Suki Kim\n> \n> The idea was to be able to distinguish when we are in a back lighting\n> condition, in particular. On this original algorithm, adding a\n> calculated gamma value to compensate the over/under exposure was used.\n> \n> This has been abandonned but beeing able to change the default gamma\n> correction table is interesting.\n> Now, the gamma correction should not be controlled by AGC or AWB, but on\n> its own, as well as the ipu3_uapi_isp_lin_vmem_params structure, which\n> really controls the contrast (as it is here to linearize the non linear\n> sensor).\n> \n>> Also, this doesn't handle contrast at the moment. Do you plan to expand\n>> the algorithm ? I also wonder if it shouldn't be called tonemapping\n>> instead of contrast.\n>>\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 3fb3ce56..69a59096 100644\n>>>> --- a/src/ipa/ipu3/algorithms/meson.build\n>>>> +++ b/src/ipa/ipu3/algorithms/meson.build\n>>>> @@ -1,5 +1,6 @@\n>>>>  # SPDX-License-Identifier: CC0-1.0\n>>>>  \n>>>>  ipu3_ipa_algorithms = files([\n>>>> -        'grid.cpp',\n>>>\n>>> Please fix this alignment in the patch that adds it.\n>>>\n>>>> +    'contrast.cpp',\n>>>> +    'grid.cpp',\n>>>>  ])\n>>>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n>>>> index 90b2f2c2..e995c663 100644\n>>>> --- a/src/ipa/ipu3/ipa_context.h\n>>>> +++ b/src/ipa/ipu3/ipa_context.h\n>>>> @@ -33,6 +33,14 @@ struct IPAConfiguration {\n>>>>   * lifetime, though for now a single instance is used.\n>>>>   */\n>>>>  struct IPAFrameContext {\n>>>> +\tstruct Agc {\n>>>> +\t\tuint32_t exposure;\n>>>> +\t\tdouble gain;\n>>>> +\t} agc;\n>>>> +\n>>>> +\tstruct Contrast {\n>>>> +\t\tuint16_t lut[IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES];\n>>>\n>>> As highilghted earlier, I think this should be:\n>>> \t\tstruct ipu3_uapi_gamma_corr_lut lut\n>>>\n>>>> +\t} contrast;\n>>>>  };\n>>>>  \n>>>>  /* Global context of the IPA */\n>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>>> index 394a7a45..e09adfc3 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 \"algorithms/grid.h\"\n>>>>  #include \"ipu3_agc.h\"\n>>>>  #include \"ipu3_awb.h\"\n>>>> @@ -167,6 +168,7 @@ int IPAIPU3::init(const IPASettings &settings,\n>>>>  \t*ipaControls = ControlInfoMap(std::move(controls), controls::controls);\n>>>>  \t/* Construct our Algorithms */\n>>>>  \talgorithms_.emplace_back(new algorithms::Grid());\n>>>> +\talgorithms_.emplace_back(new algorithms::Contrast());\n>>>>  \n>>>>  \treturn 0;\n>>>>  }\n>>>> @@ -309,7 +311,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>>>> @@ -324,9 +326,15 @@ 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>>>\n>>> New line here to separate local variable declarations from code segments.\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>>>> +\tcontext_.frameContext.agc.exposure = exposure_;\n>>>> +\tcontext_.frameContext.agc.gain = gain;\n>>>\n>>> the IPU3 layer sould not be filling in agc specific structures. That's a\n>>> layering violation to me.\n>>>\n>>> The algorithms (in this instance, the agc?) own those structures, so\n>>> they should be manipulating them.\n>>\n>> I think I agree, but how would you pass the exposure and gain parameters\n>> to the algorithm in that case ?\n> \n> Kieran, I agree with Laurent on this :-)\n\n\nAs I said below, I think we should keep this as is for now, I'm not\nsaying that we need to fix this for this patch - But it should be marked\nwith a todo as it's not 'right'.\n\nWe could ..\nAs an initial idea which should be disregarded other than to highlight\nwe can solve this issue later, and this is not thoughtout or designed or\nconsidered:\n\n\n - Have a hook 'processControls()' which passing an incoming control\n   list into algorithms as a point to say \"The app is requesting these,\n   are any for you\" - where an algorithm could then read and handle\n   controls it cares for.\n\n\nMy point was, simply from my perspective so far _frameContext.agc ...\ndoes not belong to IPU3.\n\n\nDealing with incoming controls is an entire topic that needs to be\nthought out (soon), such as \"Is this a manually set override, so we\nreally must be setting a gain of X.\n\nBut ... and I'll stress this again - The aim of /this/ series is to move\nthe framework to be more modular such that it can be further developed.\nWe shouldn't try to tackle the entire redesign in this series.\n\n\n\n\n>>> It's also showing where the algorithms need to know historical data.\n>>> In this instance, presumably they need to know what gain or expsosure\n>>> was set at the time that this buffer ran.\n>>>\n>>> This shows where we will need to keep historical versions of the\n>>> FrameContext, and perhaps track them along with the frame counter.\n>>>\n>>> For now, while we get things moving to be more modular, I think we could\n>>> keep this as it is - but it should be commented with a todo>>>\n>>>> +\tagcAlgo_->process(context_, stats);\n>>>> +\texposure_ = context_.frameContext.agc.exposure;\n>>>> +\tgain = context_.frameContext.agc.gain;\n>>>>  \tgain_ = camHelper_->gainCode(gain);\n>>>>  \n>>>>  \tawbAlgo_->calculateWBGains(stats);\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..8b32e52f 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) override;\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 911760b3..0f3bcdc9 100644\n>>>> --- a/src/ipa/ipu3/ipu3_awb.cpp\n>>>> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n>>>> @@ -114,31 +114,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>>>> @@ -179,10 +154,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>>>> @@ -332,7 +303,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>>>> @@ -344,18 +315,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 7762DBD87D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 16 Aug 2021 08:17:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D14A468892;\n\tMon, 16 Aug 2021 10:17:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7F7AB60260\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 16 Aug 2021 10:17:51 +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 BBD90DD;\n\tMon, 16 Aug 2021 10:17:49 +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=\"iIDroGRD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629101871;\n\tbh=9bvBzEp6FFgThlbw6iv78feljKtb672yo202OqhmObo=;\n\th=From:To:Cc:References:Subject:Date:In-Reply-To:From;\n\tb=iIDroGRDC6LupbumpxE5DmqWHPV5R8kurzh7xBR5ybT6XVKl1EFqjjnZ1uqVLUgQU\n\tz45Ub67fZI1nydWtYyrUSzBg7pyc63tYvej+cw1014+6Do2Agp6IOzv1WTACARZUcd\n\tDwOY7qtfix3fVd33f6uKE/Fptr8i+uoRCn7Nkj/k=","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>,\n\tLaurent Pinchart <laurent.pinchart@ideasonboard.com>","References":"<20210812165243.276977-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210812165243.276977-6-jeanmichel.hautbois@ideasonboard.com>\n\t<2d0ac1d0-9c00-e160-f60f-c5005f6239c1@ideasonboard.com>\n\t<YRmbAxPwzb0w4tKJ@pendragon.ideasonboard.com>\n\t<12b70465-1767-2c5b-3168-c2c537f3c0e2@ideasonboard.com>","Message-ID":"<c7e4a3b4-c931-b473-326d-74337faace93@ideasonboard.com>","Date":"Mon, 16 Aug 2021 09:17:47 +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":"<12b70465-1767-2c5b-3168-c2c537f3c0e2@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 05/10] 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>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]