[{"id":18757,"web_url":"https://patchwork.libcamera.org/comment/18757/","msgid":"<7c22452c-710c-3b2c-e2ed-d0d86c7098f1@ideasonboard.com>","date":"2021-08-13T09:49:56","subject":"Re: [libcamera-devel] [PATCH v2 06/10] ipa: ipu3: convert AWB to\n\tthe new algorithm interface","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> When the stats are received, pass it with the context to the existing\n> AWB algorithm. IPAFrameContext now has a new structure to store the\n> gains calculated byt the AWB algorithm.\n\ns/byt/by/\n\n> \n> When a EventFillParams event is received, call prepare() and set the new\n\n/When a/When an/\n\n> gains accordingly in the params structure.\n> There is no more a need for the IPU3Awb::initialise() function, as the\n> params are always set in prepare().\n> \n> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n> ---\n>  src/ipa/ipu3/ipa_context.h |  8 ++++\n>  src/ipa/ipu3/ipu3.cpp      |  5 +--\n>  src/ipa/ipu3/ipu3_awb.cpp  | 76 ++++++++++++++++++++------------------\n>  src/ipa/ipu3/ipu3_awb.h    |  6 +--\n>  4 files changed, 54 insertions(+), 41 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index e995c663..7bb4f598 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -38,6 +38,14 @@ struct IPAFrameContext {\n>  \t\tdouble gain;\n>  \t} agc;\n>  \n> +\tstruct Awb {\n> +\t\tstruct Gains {\n> +\t\t\tdouble redGain;\n> +\t\t\tdouble greenGain;\n> +\t\t\tdouble blueGain;\n\nThey're all in a structure called gains, can they be named simpler as\njust 'red', 'green', 'blue' ?\n\n\n> +\t\t} gains;\n> +\t} awb;\n> +\n>  \tstruct Contrast {\n>  \t\tuint16_t lut[IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES];\n>  \t} contrast;\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index e09adfc3..dba5554d 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -230,7 +230,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n>  \t}\n>  \n>  \tawbAlgo_ = std::make_unique<IPU3Awb>();\n> -\tawbAlgo_->initialise(params_, context_.configuration.grid.bdsOutputSize, context_.configuration.grid.bdsGrid);\n>  \n>  \tagcAlgo_ = std::make_unique<IPU3Agc>();\n>  \tagcAlgo_->initialise(context_.configuration.grid.bdsGrid, sensorInfo_);\n> @@ -311,7 +310,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_);\n> +\t\tawbAlgo_->prepare(context_, params_);\n>  \n>  \t*params = params_;\n>  \n> @@ -337,7 +336,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>  \tgain = context_.frameContext.agc.gain;\n>  \tgain_ = camHelper_->gainCode(gain);\n>  \n> -\tawbAlgo_->calculateWBGains(stats);\n> +\tawbAlgo_->process(context_, stats);\n>  \n>  \tif (agcAlgo_->updateControls())\n>  \t\tsetControls(frame);\n> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp\n> index 0f3bcdc9..b422b095 100644\n> --- a/src/ipa/ipu3/ipu3_awb.cpp\n> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n> @@ -121,42 +121,14 @@ IPU3Awb::IPU3Awb()\n>  \tasyncResults_.greenGain = 1.0;\n>  \tasyncResults_.redGain = 1.0;\n>  \tasyncResults_.temperatureK = 4500;\n> +\n> +\tzones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);\n>  }\n>  \n>  IPU3Awb::~IPU3Awb()\n>  {\n>  }\n>  \n> -void IPU3Awb::initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid)\n> -{\n> -\tparams.use.acc_awb = 1;\n> -\tparams.acc_param.awb.config.rgbs_thr_gr = 8191;\n> -\tparams.acc_param.awb.config.rgbs_thr_r = 8191;\n> -\tparams.acc_param.awb.config.rgbs_thr_gb = 8191;\n> -\tparams.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;\n> -\tawbGrid_ = bdsGrid;\n> -\n> -\tparams.use.acc_bnr = 1;\n> -\tparams.acc_param.bnr = imguCssBnrDefaults;\n> -\t/**\n> -\t * Optical center is column (respectively row) startminus X (respectively Y) center.\n> -\t * For the moment use BDS as a first approximation, but it should\n> -\t * be calculated based on Shading (SHD) parameters.\n> -\t */\n> -\tparams.acc_param.bnr.column_size = bdsOutputSize.width;\n> -\tparams.acc_param.bnr.opt_center.x_reset = awbGrid_.x_start - (bdsOutputSize.width / 2);\n> -\tparams.acc_param.bnr.opt_center.y_reset = awbGrid_.y_start - (bdsOutputSize.height / 2);\n> -\tparams.acc_param.bnr.opt_center_sqr.x_sqr_reset = params.acc_param.bnr.opt_center.x_reset\n> -\t\t\t\t\t\t\t* params.acc_param.bnr.opt_center.x_reset;\n> -\tparams.acc_param.bnr.opt_center_sqr.y_sqr_reset = params.acc_param.bnr.opt_center.y_reset\n> -\t\t\t\t\t\t\t* params.acc_param.bnr.opt_center.y_reset;\n> -\n> -\tparams.use.acc_ccm = 1;\n> -\tparams.acc_param.ccm = imguCssCcmDefault;\n> -\n> -\tzones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);\n> -}\n> -\n>  /**\n>   * The function estimates the correlated color temperature using\n>   * from RGB color space input.\n> @@ -303,17 +275,51 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)\n>  \t}\n>  }\n>  \n> -void IPU3Awb::updateWbParameters(ipu3_uapi_params &params)\n> +void IPU3Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> +{\n> +\tcalculateWBGains(stats);\n\nI'd put a new line here.\n\nPerhaps a comment here saying \"\n\n/*\n * Gains are only recalculated if enough zones were detected.\n * The results are cached, so if no results were calculated, we set the\n * cached values from asyncResults_ here.\n */\n\n\n> +\tcontext.frameContext.awb.gains.blueGain = asyncResults_.blueGain;\n> +\tcontext.frameContext.awb.gains.greenGain = asyncResults_.greenGain;\n> +\tcontext.frameContext.awb.gains.redGain = asyncResults_.redGain;> +}\n> +\n> +void IPU3Awb::prepare(IPAContext &context, ipu3_uapi_params &params)\n>  {\n> +\tparams.use.acc_awb = 1;\n\nSet enable/use flags last... - or in this instance, after setting the\nawb parameters.\n\n\n> +\tparams.acc_param.awb.config.rgbs_thr_gr = 8191;\n> +\tparams.acc_param.awb.config.rgbs_thr_r = 8191;\n> +\tparams.acc_param.awb.config.rgbs_thr_gb = 8191;\n> +\tparams.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;\n> +\tawbGrid_ = context.configuration.grid.bdsGrid;\n> +\n> +\tparams.use.acc_bnr = 1;\n\nAnd this shows that there are modules to further break out of this...\n\nAnything that has it's own 'module' on the IPU3 should have it's own\ncorresponding module in the code base I think.\n\n\n> +\tparams.acc_param.bnr = imguCssBnrDefaults;\n\nnew line?\n\n> +\t/**\n\ns#/**#/*#\n\n> +\t * Optical center is column (respectively row) startminus X (respectively Y) center.\n> +\t * For the moment use BDS as a first approximation, but it should\n> +\t * be calculated based on Shading (SHD) parameters.\n> +\t */\n> +\tSize &bdsOutputSize = context.configuration.grid.bdsOutputSize;\n> +\tparams.acc_param.bnr.column_size = bdsOutputSize.width;\n> +\tparams.acc_param.bnr.opt_center.x_reset = awbGrid_.x_start - (bdsOutputSize.width / 2);\n> +\tparams.acc_param.bnr.opt_center.y_reset = awbGrid_.y_start - (bdsOutputSize.height / 2);\n> +\tparams.acc_param.bnr.opt_center_sqr.x_sqr_reset = params.acc_param.bnr.opt_center.x_reset\n> +\t\t\t\t\t\t\t* params.acc_param.bnr.opt_center.x_reset;\n> +\tparams.acc_param.bnr.opt_center_sqr.y_sqr_reset = params.acc_param.bnr.opt_center.y_reset\n> +\t\t\t\t\t\t\t* params.acc_param.bnr.opt_center.y_reset;\n> +\n> +\tparams.use.acc_ccm = 1;\n\nSame, but keep both BNR and CCM here in this patch, and they can move\nout to their own modules separately.\n\nFairly trivial comments in this patch.\n\nWith those handled as best as you see fit,\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n\n> +\tparams.acc_param.ccm = imguCssCcmDefault;\n> +\n>  \t/*\n>  \t * Green gains should not be touched and considered 1.\n>  \t * Default is 16, so do not change it at all.\n>  \t * 4096 is the value for a gain of 1.0\n>  \t */\n> -\tparams.acc_param.bnr.wb_gains.gr = 16;\n> -\tparams.acc_param.bnr.wb_gains.r = 4096 * asyncResults_.redGain;\n> -\tparams.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain;\n> -\tparams.acc_param.bnr.wb_gains.gb = 16;\n> +\tparams.acc_param.bnr.wb_gains.gr = 16 * context.frameContext.awb.gains.greenGain;\n> +\tparams.acc_param.bnr.wb_gains.r = 4096 * context.frameContext.awb.gains.redGain;\n> +\tparams.acc_param.bnr.wb_gains.b = 4096 * context.frameContext.awb.gains.blueGain;\n> +\tparams.acc_param.bnr.wb_gains.gb = 16 * context.frameContext.awb.gains.greenGain;\n>  \n>  \tLOG(IPU3Awb, Debug) << \"Color temperature estimated: \" << asyncResults_.temperatureK;\n>  \n> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h\n> index eeb2920b..4de3fae2 100644\n> --- a/src/ipa/ipu3/ipu3_awb.h\n> +++ b/src/ipa/ipu3/ipu3_awb.h\n> @@ -29,9 +29,8 @@ public:\n>  \tIPU3Awb();\n>  \t~IPU3Awb();\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);\n> +\tvoid prepare(IPAContext &context, ipu3_uapi_params &params) override;\n> +\tvoid process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;\n>  \n>  \tstruct Ipu3AwbCell {\n>  \t\tunsigned char greenRedAvg;\n> @@ -72,6 +71,7 @@ public:\n>  \t};\n>  \n>  private:\n> +\tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats);\n>  \tvoid generateZones(std::vector<RGB> &zones);\n>  \tvoid generateAwbStats(const ipu3_uapi_stats_3a *stats);\n>  \tvoid clearAwbStats();\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 577F2BD87D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 13 Aug 2021 09:50:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D24996888E;\n\tFri, 13 Aug 2021 11:50:00 +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 32AEA60263\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 13 Aug 2021 11:50:00 +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 ADCB3EE;\n\tFri, 13 Aug 2021 11:49: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=\"mRcD/iqz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628848199;\n\tbh=fLKND/Ef7umW4MtiOE7rdjh9ZGiMoTp0n15I4hKzSQQ=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=mRcD/iqzKk1cGaSw5fNev57ng9+h4NtC9lEB0uBnBYkabJ7Ca18rLmHVq+73HJvQR\n\ttWggN/Aom3egzk8kFED4c27f0p5KqIRKBAmRY50fRQ0vebuaqDQxUInCmlKqNxVaHl\n\tw64gMgjQabMMlc4U7bjOZvI7aE3Kl9ejNcR7CH0s=","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-7-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<7c22452c-710c-3b2c-e2ed-d0d86c7098f1@ideasonboard.com>","Date":"Fri, 13 Aug 2021 10:49:56 +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-7-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 06/10] ipa: ipu3: convert AWB to\n\tthe new algorithm interface","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":18808,"web_url":"https://patchwork.libcamera.org/comment/18808/","msgid":"<YRmoiZRjSnGlsqqi@pendragon.ideasonboard.com>","date":"2021-08-15T23:51:37","subject":"Re: [libcamera-devel] [PATCH v2 06/10] ipa: ipu3: convert AWB to\n\tthe new algorithm interface","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nThank you for the patch.\n\nOn Fri, Aug 13, 2021 at 10:49:56AM +0100, Kieran Bingham wrote:\n> On 12/08/2021 17:52, Jean-Michel Hautbois wrote:\n> > When the stats are received, pass it with the context to the existing\n\ns/it/them/\n\n> > AWB algorithm. IPAFrameContext now has a new structure to store the\n> > gains calculated byt the AWB algorithm.\n> \n> s/byt/by/\n> \n> > \n> > When a EventFillParams event is received, call prepare() and set the new\n> \n> /When a/When an/\n> \n> > gains accordingly in the params structure.\n> > There is no more a need for the IPU3Awb::initialise() function, as the\n> > params are always set in prepare().\n> > \n> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n> > ---\n> >  src/ipa/ipu3/ipa_context.h |  8 ++++\n> >  src/ipa/ipu3/ipu3.cpp      |  5 +--\n> >  src/ipa/ipu3/ipu3_awb.cpp  | 76 ++++++++++++++++++++------------------\n> >  src/ipa/ipu3/ipu3_awb.h    |  6 +--\n> >  4 files changed, 54 insertions(+), 41 deletions(-)\n> > \n> > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> > index e995c663..7bb4f598 100644\n> > --- a/src/ipa/ipu3/ipa_context.h\n> > +++ b/src/ipa/ipu3/ipa_context.h\n> > @@ -38,6 +38,14 @@ struct IPAFrameContext {\n> >  \t\tdouble gain;\n> >  \t} agc;\n> >  \n> > +\tstruct Awb {\n> > +\t\tstruct Gains {\n> > +\t\t\tdouble redGain;\n> > +\t\t\tdouble greenGain;\n> > +\t\t\tdouble blueGain;\n> \n> They're all in a structure called gains, can they be named simpler as\n> just 'red', 'green', 'blue' ?\n\nAgree. Also, we don't necessarily need to name the structures here.\n\n> > +\t\t} gains;\n> > +\t} awb;\n> > +\n> >  \tstruct Contrast {\n> >  \t\tuint16_t lut[IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES];\n> >  \t} contrast;\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index e09adfc3..dba5554d 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -230,7 +230,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> >  \t}\n> >  \n> >  \tawbAlgo_ = std::make_unique<IPU3Awb>();\n> > -\tawbAlgo_->initialise(params_, context_.configuration.grid.bdsOutputSize, context_.configuration.grid.bdsGrid);\n> >  \n> >  \tagcAlgo_ = std::make_unique<IPU3Agc>();\n> >  \tagcAlgo_->initialise(context_.configuration.grid.bdsGrid, sensorInfo_);\n> > @@ -311,7 +310,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_);\n> > +\t\tawbAlgo_->prepare(context_, params_);\n> >  \n> >  \t*params = params_;\n> >  \n> > @@ -337,7 +336,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> >  \tgain = context_.frameContext.agc.gain;\n> >  \tgain_ = camHelper_->gainCode(gain);\n> >  \n> > -\tawbAlgo_->calculateWBGains(stats);\n> > +\tawbAlgo_->process(context_, stats);\n> >  \n> >  \tif (agcAlgo_->updateControls())\n> >  \t\tsetControls(frame);\n> > diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp\n> > index 0f3bcdc9..b422b095 100644\n> > --- a/src/ipa/ipu3/ipu3_awb.cpp\n> > +++ b/src/ipa/ipu3/ipu3_awb.cpp\n> > @@ -121,42 +121,14 @@ IPU3Awb::IPU3Awb()\n> >  \tasyncResults_.greenGain = 1.0;\n> >  \tasyncResults_.redGain = 1.0;\n> >  \tasyncResults_.temperatureK = 4500;\n> > +\n> > +\tzones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);\n> >  }\n> >  \n> >  IPU3Awb::~IPU3Awb()\n> >  {\n> >  }\n> >  \n> > -void IPU3Awb::initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid)\n> > -{\n> > -\tparams.use.acc_awb = 1;\n> > -\tparams.acc_param.awb.config.rgbs_thr_gr = 8191;\n> > -\tparams.acc_param.awb.config.rgbs_thr_r = 8191;\n> > -\tparams.acc_param.awb.config.rgbs_thr_gb = 8191;\n> > -\tparams.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;\n> > -\tawbGrid_ = bdsGrid;\n> > -\n> > -\tparams.use.acc_bnr = 1;\n> > -\tparams.acc_param.bnr = imguCssBnrDefaults;\n> > -\t/**\n> > -\t * Optical center is column (respectively row) startminus X (respectively Y) center.\n> > -\t * For the moment use BDS as a first approximation, but it should\n> > -\t * be calculated based on Shading (SHD) parameters.\n> > -\t */\n> > -\tparams.acc_param.bnr.column_size = bdsOutputSize.width;\n> > -\tparams.acc_param.bnr.opt_center.x_reset = awbGrid_.x_start - (bdsOutputSize.width / 2);\n> > -\tparams.acc_param.bnr.opt_center.y_reset = awbGrid_.y_start - (bdsOutputSize.height / 2);\n> > -\tparams.acc_param.bnr.opt_center_sqr.x_sqr_reset = params.acc_param.bnr.opt_center.x_reset\n> > -\t\t\t\t\t\t\t* params.acc_param.bnr.opt_center.x_reset;\n> > -\tparams.acc_param.bnr.opt_center_sqr.y_sqr_reset = params.acc_param.bnr.opt_center.y_reset\n> > -\t\t\t\t\t\t\t* params.acc_param.bnr.opt_center.y_reset;\n> > -\n> > -\tparams.use.acc_ccm = 1;\n> > -\tparams.acc_param.ccm = imguCssCcmDefault;\n> > -\n> > -\tzones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);\n> > -}\n> > -\n> >  /**\n> >   * The function estimates the correlated color temperature using\n> >   * from RGB color space input.\n> > @@ -303,17 +275,51 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)\n> >  \t}\n> >  }\n> >  \n> > -void IPU3Awb::updateWbParameters(ipu3_uapi_params &params)\n> > +void IPU3Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> > +{\n> > +\tcalculateWBGains(stats);\n> \n> I'd put a new line here.\n> \n> Perhaps a comment here saying \"\n> \n> /*\n>  * Gains are only recalculated if enough zones were detected.\n>  * The results are cached, so if no results were calculated, we set the\n>  * cached values from asyncResults_ here.\n>  */\n> \n> \n> > +\tcontext.frameContext.awb.gains.blueGain = asyncResults_.blueGain;\n> > +\tcontext.frameContext.awb.gains.greenGain = asyncResults_.greenGain;\n> > +\tcontext.frameContext.awb.gains.redGain = asyncResults_.redGain;\n> > +}\n> > +\n> > +void IPU3Awb::prepare(IPAContext &context, ipu3_uapi_params &params)\n> >  {\n> > +\tparams.use.acc_awb = 1;\n> \n> Set enable/use flags last... - or in this instance, after setting the\n> awb parameters.\n\nOn this topic, btw, I tend to set structure fields in the order in which\nthey are defined in the structure. I suppose it's a matter of personal\ntaste.\n\n> > +\tparams.acc_param.awb.config.rgbs_thr_gr = 8191;\n> > +\tparams.acc_param.awb.config.rgbs_thr_r = 8191;\n> > +\tparams.acc_param.awb.config.rgbs_thr_gb = 8191;\n> > +\tparams.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;\n\nLine wrap.\n\n> > +\tawbGrid_ = context.configuration.grid.bdsGrid;\n> > +\n> > +\tparams.use.acc_bnr = 1;\n> \n> And this shows that there are modules to further break out of this...\n\nAgreed, I think Bayer noise reduction should be moved to a separate\nmodule, but note that the ImgU groups parameters in a weird way.\nparams.acc_param.bnr contains much more than noise reduction parameters\n(namely at least white balance, lens shading correction and bad pixel\ncorrection). acc_bnr should thus be set if any of those algorithms are\nenabled, but we then need to make sure that default values are\nprogrammed for all other algorithms.\n\n> Anything that has it's own 'module' on the IPU3 should have it's own\n> corresponding module in the code base I think.\n\nIt's a good starting point, but it can sometimes make sense to group or\nsplit modules in different ways, to match how the algorithms are\nimplemented. I haven't checked, but for instance the AWB and CCM\nparameters may make sense to control from a single algorithm.\n\n> > +\tparams.acc_param.bnr = imguCssBnrDefaults;\n\nBased on the above comment, maybe this should be moved to ipu3.cpp (in a\nseparate patch though).\n\n> new line?\n> \n> > +\t/**\n> \n> s#/**#/*#\n> \n> > +\t * Optical center is column (respectively row) startminus X (respectively Y) center.\n\nI know this is moved from a different location, but I have trouble\nunderstanding the comment. The typo s/startminus/start minus/ could\nalready be fixed here. Oh, and please reflow the comment to 80 columns.\n\n> > +\t * For the moment use BDS as a first approximation, but it should\n> > +\t * be calculated based on Shading (SHD) parameters.\n> > +\t */\n> > +\tSize &bdsOutputSize = context.configuration.grid.bdsOutputSize;\n> > +\tparams.acc_param.bnr.column_size = bdsOutputSize.width;\n> > +\tparams.acc_param.bnr.opt_center.x_reset = awbGrid_.x_start - (bdsOutputSize.width / 2);\n> > +\tparams.acc_param.bnr.opt_center.y_reset = awbGrid_.y_start - (bdsOutputSize.height / 2);\n> > +\tparams.acc_param.bnr.opt_center_sqr.x_sqr_reset = params.acc_param.bnr.opt_center.x_reset\n> > +\t\t\t\t\t\t\t* params.acc_param.bnr.opt_center.x_reset;\n> > +\tparams.acc_param.bnr.opt_center_sqr.y_sqr_reset = params.acc_param.bnr.opt_center.y_reset\n> > +\t\t\t\t\t\t\t* params.acc_param.bnr.opt_center.y_reset;\n> > +\n> > +\tparams.use.acc_ccm = 1;\n> \n> Same, but keep both BNR and CCM here in this patch, and they can move\n> out to their own modules separately.\n> \n> Fairly trivial comments in this patch.\n> \n> With those handled as best as you see fit,\n> \n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> \n> > +\tparams.acc_param.ccm = imguCssCcmDefault;\n> > +\n> >  \t/*\n> >  \t * Green gains should not be touched and considered 1.\n> >  \t * Default is 16, so do not change it at all.\n> >  \t * 4096 is the value for a gain of 1.0\n> >  \t */\n> > -\tparams.acc_param.bnr.wb_gains.gr = 16;\n> > -\tparams.acc_param.bnr.wb_gains.r = 4096 * asyncResults_.redGain;\n> > -\tparams.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain;\n> > -\tparams.acc_param.bnr.wb_gains.gb = 16;\n> > +\tparams.acc_param.bnr.wb_gains.gr = 16 * context.frameContext.awb.gains.greenGain;\n> > +\tparams.acc_param.bnr.wb_gains.r = 4096 * context.frameContext.awb.gains.redGain;\n> > +\tparams.acc_param.bnr.wb_gains.b = 4096 * context.frameContext.awb.gains.blueGain;\n> > +\tparams.acc_param.bnr.wb_gains.gb = 16 * context.frameContext.awb.gains.greenGain;\n\nDoes this mean that a gain of 1.0 is encoded as 16 for the green\nchannels and 4096 for the red and blue channels ?\n\nThis also doesn't match the comment in intel-ipu3.h that states\n\n/**\n * struct ipu3_uapi_bnr_static_config_wb_gains_config - White balance gains\n *\n * @gr: white balance gain for Gr channel.\n * @r:  white balance gain for R channel.\n * @b:  white balance gain for B channel.\n * @gb: white balance gain for Gb channel.\n *\n * Precision u3.13, range [0, 8). White balance correction is done by applying\n * a multiplicative gain to each color channels prior to BNR.\n */\n\nThis indicates that 1.0 maps to 8192. Do we have evidence that this is\nincorrect ?\n\n> >  \n> >  \tLOG(IPU3Awb, Debug) << \"Color temperature estimated: \" << asyncResults_.temperatureK;\n> >  \n> > diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h\n> > index eeb2920b..4de3fae2 100644\n> > --- a/src/ipa/ipu3/ipu3_awb.h\n> > +++ b/src/ipa/ipu3/ipu3_awb.h\n> > @@ -29,9 +29,8 @@ public:\n> >  \tIPU3Awb();\n> >  \t~IPU3Awb();\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);\n> > +\tvoid prepare(IPAContext &context, ipu3_uapi_params &params) override;\n> > +\tvoid process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;\n> >  \n> >  \tstruct Ipu3AwbCell {\n> >  \t\tunsigned char greenRedAvg;\n> > @@ -72,6 +71,7 @@ public:\n> >  \t};\n> >  \n> >  private:\n> > +\tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats);\n> >  \tvoid generateZones(std::vector<RGB> &zones);\n> >  \tvoid generateAwbStats(const ipu3_uapi_stats_3a *stats);\n> >  \tvoid clearAwbStats();","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 83607BD87D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 15 Aug 2021 23:51:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E0E7D68890;\n\tMon, 16 Aug 2021 01:51:44 +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 DA09860261\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 16 Aug 2021 01:51:43 +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 4F3A02C5;\n\tMon, 16 Aug 2021 01:51:43 +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=\"M9WYuA3q\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629071503;\n\tbh=qyWwG/LZ1mup3WpoUSrJY1IiFmoN4Wpbb05/rjuPnCs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=M9WYuA3q6cwvr+bj3fIF7ca6uJ7Yp2kyZmn6K9efzEwla0vn/dradL2zZ+y5/oakx\n\tew0JIDwT8IBmzpq52bV26vwAlt0TKWBEkkUOfW/X39nahZpZYP6a3zW2JeXcB3T5h5\n\tPBDtz3OHsOAG++pnniZngKcQo8tF1j7dtIYEKCro=","Date":"Mon, 16 Aug 2021 02:51:37 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","Message-ID":"<YRmoiZRjSnGlsqqi@pendragon.ideasonboard.com>","References":"<20210812165243.276977-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210812165243.276977-7-jeanmichel.hautbois@ideasonboard.com>\n\t<7c22452c-710c-3b2c-e2ed-d0d86c7098f1@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<7c22452c-710c-3b2c-e2ed-d0d86c7098f1@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 06/10] ipa: ipu3: convert AWB to\n\tthe new algorithm interface","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":18819,"web_url":"https://patchwork.libcamera.org/comment/18819/","msgid":"<ed10afad-b1a0-e836-7ca7-e42cccb9ba2b@ideasonboard.com>","date":"2021-08-16T07:55:20","subject":"Re: [libcamera-devel] [PATCH v2 06/10] ipa: ipu3: convert AWB to\n\tthe new algorithm interface","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 01:51, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> Thank you for the patch.\n> \n> On Fri, Aug 13, 2021 at 10:49:56AM +0100, Kieran Bingham wrote:\n>> On 12/08/2021 17:52, Jean-Michel Hautbois wrote:\n>>> When the stats are received, pass it with the context to the existing\n> \n> s/it/them/\n> \n>>> AWB algorithm. IPAFrameContext now has a new structure to store the\n>>> gains calculated byt the AWB algorithm.\n>>\n>> s/byt/by/\n>>\n>>>\n>>> When a EventFillParams event is received, call prepare() and set the new\n>>\n>> /When a/When an/\n>>\n>>> gains accordingly in the params structure.\n>>> There is no more a need for the IPU3Awb::initialise() function, as the\n>>> params are always set in prepare().\n>>>\n>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n>>> ---\n>>>  src/ipa/ipu3/ipa_context.h |  8 ++++\n>>>  src/ipa/ipu3/ipu3.cpp      |  5 +--\n>>>  src/ipa/ipu3/ipu3_awb.cpp  | 76 ++++++++++++++++++++------------------\n>>>  src/ipa/ipu3/ipu3_awb.h    |  6 +--\n>>>  4 files changed, 54 insertions(+), 41 deletions(-)\n>>>\n>>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n>>> index e995c663..7bb4f598 100644\n>>> --- a/src/ipa/ipu3/ipa_context.h\n>>> +++ b/src/ipa/ipu3/ipa_context.h\n>>> @@ -38,6 +38,14 @@ struct IPAFrameContext {\n>>>  \t\tdouble gain;\n>>>  \t} agc;\n>>>  \n>>> +\tstruct Awb {\n>>> +\t\tstruct Gains {\n>>> +\t\t\tdouble redGain;\n>>> +\t\t\tdouble greenGain;\n>>> +\t\t\tdouble blueGain;\n>>\n>> They're all in a structure called gains, can they be named simpler as\n>> just 'red', 'green', 'blue' ?\n> \n> Agree. Also, we don't necessarily need to name the structures here.\n> \n\nI like the idea of naming the structures...\n\n>>> +\t\t} gains;\n>>> +\t} awb;\n>>> +\n>>>  \tstruct Contrast {\n>>>  \t\tuint16_t lut[IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES];\n>>>  \t} contrast;\n>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>> index e09adfc3..dba5554d 100644\n>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>> @@ -230,7 +230,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n>>>  \t}\n>>>  \n>>>  \tawbAlgo_ = std::make_unique<IPU3Awb>();\n>>> -\tawbAlgo_->initialise(params_, context_.configuration.grid.bdsOutputSize, context_.configuration.grid.bdsGrid);\n>>>  \n>>>  \tagcAlgo_ = std::make_unique<IPU3Agc>();\n>>>  \tagcAlgo_->initialise(context_.configuration.grid.bdsGrid, sensorInfo_);\n>>> @@ -311,7 +310,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_);\n>>> +\t\tawbAlgo_->prepare(context_, params_);\n>>>  \n>>>  \t*params = params_;\n>>>  \n>>> @@ -337,7 +336,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>>>  \tgain = context_.frameContext.agc.gain;\n>>>  \tgain_ = camHelper_->gainCode(gain);\n>>>  \n>>> -\tawbAlgo_->calculateWBGains(stats);\n>>> +\tawbAlgo_->process(context_, stats);\n>>>  \n>>>  \tif (agcAlgo_->updateControls())\n>>>  \t\tsetControls(frame);\n>>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp\n>>> index 0f3bcdc9..b422b095 100644\n>>> --- a/src/ipa/ipu3/ipu3_awb.cpp\n>>> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n>>> @@ -121,42 +121,14 @@ IPU3Awb::IPU3Awb()\n>>>  \tasyncResults_.greenGain = 1.0;\n>>>  \tasyncResults_.redGain = 1.0;\n>>>  \tasyncResults_.temperatureK = 4500;\n>>> +\n>>> +\tzones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);\n>>>  }\n>>>  \n>>>  IPU3Awb::~IPU3Awb()\n>>>  {\n>>>  }\n>>>  \n>>> -void IPU3Awb::initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid)\n>>> -{\n>>> -\tparams.use.acc_awb = 1;\n>>> -\tparams.acc_param.awb.config.rgbs_thr_gr = 8191;\n>>> -\tparams.acc_param.awb.config.rgbs_thr_r = 8191;\n>>> -\tparams.acc_param.awb.config.rgbs_thr_gb = 8191;\n>>> -\tparams.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;\n>>> -\tawbGrid_ = bdsGrid;\n>>> -\n>>> -\tparams.use.acc_bnr = 1;\n>>> -\tparams.acc_param.bnr = imguCssBnrDefaults;\n>>> -\t/**\n>>> -\t * Optical center is column (respectively row) startminus X (respectively Y) center.\n>>> -\t * For the moment use BDS as a first approximation, but it should\n>>> -\t * be calculated based on Shading (SHD) parameters.\n>>> -\t */\n>>> -\tparams.acc_param.bnr.column_size = bdsOutputSize.width;\n>>> -\tparams.acc_param.bnr.opt_center.x_reset = awbGrid_.x_start - (bdsOutputSize.width / 2);\n>>> -\tparams.acc_param.bnr.opt_center.y_reset = awbGrid_.y_start - (bdsOutputSize.height / 2);\n>>> -\tparams.acc_param.bnr.opt_center_sqr.x_sqr_reset = params.acc_param.bnr.opt_center.x_reset\n>>> -\t\t\t\t\t\t\t* params.acc_param.bnr.opt_center.x_reset;\n>>> -\tparams.acc_param.bnr.opt_center_sqr.y_sqr_reset = params.acc_param.bnr.opt_center.y_reset\n>>> -\t\t\t\t\t\t\t* params.acc_param.bnr.opt_center.y_reset;\n>>> -\n>>> -\tparams.use.acc_ccm = 1;\n>>> -\tparams.acc_param.ccm = imguCssCcmDefault;\n>>> -\n>>> -\tzones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);\n>>> -}\n>>> -\n>>>  /**\n>>>   * The function estimates the correlated color temperature using\n>>>   * from RGB color space input.\n>>> @@ -303,17 +275,51 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)\n>>>  \t}\n>>>  }\n>>>  \n>>> -void IPU3Awb::updateWbParameters(ipu3_uapi_params &params)\n>>> +void IPU3Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n>>> +{\n>>> +\tcalculateWBGains(stats);\n>>\n>> I'd put a new line here.\n>>\n>> Perhaps a comment here saying \"\n>>\n>> /*\n>>  * Gains are only recalculated if enough zones were detected.\n>>  * The results are cached, so if no results were calculated, we set the\n>>  * cached values from asyncResults_ here.\n>>  */\n>>\n>>\n>>> +\tcontext.frameContext.awb.gains.blueGain = asyncResults_.blueGain;\n>>> +\tcontext.frameContext.awb.gains.greenGain = asyncResults_.greenGain;\n>>> +\tcontext.frameContext.awb.gains.redGain = asyncResults_.redGain;\n>>> +}\n>>> +\n>>> +void IPU3Awb::prepare(IPAContext &context, ipu3_uapi_params &params)\n>>>  {\n>>> +\tparams.use.acc_awb = 1;\n>>\n>> Set enable/use flags last... - or in this instance, after setting the\n>> awb parameters.\n> \n> On this topic, btw, I tend to set structure fields in the order in which\n> they are defined in the structure. I suppose it's a matter of personal\n> taste.\n> \n>>> +\tparams.acc_param.awb.config.rgbs_thr_gr = 8191;\n>>> +\tparams.acc_param.awb.config.rgbs_thr_r = 8191;\n>>> +\tparams.acc_param.awb.config.rgbs_thr_gb = 8191;\n>>> +\tparams.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;\n> \n> Line wrap.\n> \n>>> +\tawbGrid_ = context.configuration.grid.bdsGrid;\n>>> +\n>>> +\tparams.use.acc_bnr = 1;\n>>\n>> And this shows that there are modules to further break out of this...\n> \n> Agreed, I think Bayer noise reduction should be moved to a separate\n> module, but note that the ImgU groups parameters in a weird way.\n> params.acc_param.bnr contains much more than noise reduction parameters\n> (namely at least white balance, lens shading correction and bad pixel\n> correction). acc_bnr should thus be set if any of those algorithms are\n> enabled, but we then need to make sure that default values are\n> programmed for all other algorithms.\n> \n\nIndeed, and that's really painful. That's why I was tempted to separate\ninto algorithms even if they access the same structure in the prepare()\ncall they will never set all the parameters, only the ones they are\nresponsible for. But then it means we need to make sure we have all the\ndefaults. Spaghetti dish you said :-/ ?\n\n>> Anything that has it's own 'module' on the IPU3 should have it's own\n>> corresponding module in the code base I think.\n> \n> It's a good starting point, but it can sometimes make sense to group or\n> split modules in different ways, to match how the algorithms are\n> implemented. I haven't checked, but for instance the AWB and CCM\n> parameters may make sense to control from a single algorithm.\n> \n>>> +\tparams.acc_param.bnr = imguCssBnrDefaults;\n> \n> Based on the above comment, maybe this should be moved to ipu3.cpp (in a\n> separate patch though).\n> \n>> new line?\n>>\n>>> +\t/**\n>>\n>> s#/**#/*#\n>>\n>>> +\t * Optical center is column (respectively row) startminus X (respectively Y) center.\n> \n> I know this is moved from a different location, but I have trouble\n> understanding the comment. The typo s/startminus/start minus/ could\n> already be fixed here. Oh, and please reflow the comment to 80 columns.\n> \n>>> +\t * For the moment use BDS as a first approximation, but it should\n>>> +\t * be calculated based on Shading (SHD) parameters.\n>>> +\t */\n>>> +\tSize &bdsOutputSize = context.configuration.grid.bdsOutputSize;\n>>> +\tparams.acc_param.bnr.column_size = bdsOutputSize.width;\n>>> +\tparams.acc_param.bnr.opt_center.x_reset = awbGrid_.x_start - (bdsOutputSize.width / 2);\n>>> +\tparams.acc_param.bnr.opt_center.y_reset = awbGrid_.y_start - (bdsOutputSize.height / 2);\n>>> +\tparams.acc_param.bnr.opt_center_sqr.x_sqr_reset = params.acc_param.bnr.opt_center.x_reset\n>>> +\t\t\t\t\t\t\t* params.acc_param.bnr.opt_center.x_reset;\n>>> +\tparams.acc_param.bnr.opt_center_sqr.y_sqr_reset = params.acc_param.bnr.opt_center.y_reset\n>>> +\t\t\t\t\t\t\t* params.acc_param.bnr.opt_center.y_reset;\n>>> +\n>>> +\tparams.use.acc_ccm = 1;\n>>\n>> Same, but keep both BNR and CCM here in this patch, and they can move\n>> out to their own modules separately.\n>>\n>> Fairly trivial comments in this patch.\n>>\n>> With those handled as best as you see fit,\n>>\n>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>>\n>>> +\tparams.acc_param.ccm = imguCssCcmDefault;\n>>> +\n>>>  \t/*\n>>>  \t * Green gains should not be touched and considered 1.\n>>>  \t * Default is 16, so do not change it at all.\n>>>  \t * 4096 is the value for a gain of 1.0\n>>>  \t */\n>>> -\tparams.acc_param.bnr.wb_gains.gr = 16;\n>>> -\tparams.acc_param.bnr.wb_gains.r = 4096 * asyncResults_.redGain;\n>>> -\tparams.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain;\n>>> -\tparams.acc_param.bnr.wb_gains.gb = 16;\n>>> +\tparams.acc_param.bnr.wb_gains.gr = 16 * context.frameContext.awb.gains.greenGain;\n>>> +\tparams.acc_param.bnr.wb_gains.r = 4096 * context.frameContext.awb.gains.redGain;\n>>> +\tparams.acc_param.bnr.wb_gains.b = 4096 * context.frameContext.awb.gains.blueGain;\n>>> +\tparams.acc_param.bnr.wb_gains.gb = 16 * context.frameContext.awb.gains.greenGain;\n> \n> Does this mean that a gain of 1.0 is encoded as 16 for the green\n> channels and 4096 for the red and blue channels ?\n> \n> This also doesn't match the comment in intel-ipu3.h that states\n> \n> /**\n>  * struct ipu3_uapi_bnr_static_config_wb_gains_config - White balance gains\n>  *\n>  * @gr: white balance gain for Gr channel.\n>  * @r:  white balance gain for R channel.\n>  * @b:  white balance gain for B channel.\n>  * @gb: white balance gain for Gb channel.\n>  *\n>  * Precision u3.13, range [0, 8). White balance correction is done by applying\n>  * a multiplicative gain to each color channels prior to BNR.\n>  */\n> \n> This indicates that 1.0 maps to 8192. Do we have evidence that this is\n> incorrect ?\n\nThis is something which is a bad comprehension (from me) in the first\nplace. I did not want to change that here, as it is a move.\nYou are right, a gain of 1.0 is mapped to 8192. Doing so would lead to\ngreenish frames as we don't have the corresponding CCM to compensate.\n\n>>>  \n>>>  \tLOG(IPU3Awb, Debug) << \"Color temperature estimated: \" << asyncResults_.temperatureK;\n>>>  \n>>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h\n>>> index eeb2920b..4de3fae2 100644\n>>> --- a/src/ipa/ipu3/ipu3_awb.h\n>>> +++ b/src/ipa/ipu3/ipu3_awb.h\n>>> @@ -29,9 +29,8 @@ public:\n>>>  \tIPU3Awb();\n>>>  \t~IPU3Awb();\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);\n>>> +\tvoid prepare(IPAContext &context, ipu3_uapi_params &params) override;\n>>> +\tvoid process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;\n>>>  \n>>>  \tstruct Ipu3AwbCell {\n>>>  \t\tunsigned char greenRedAvg;\n>>> @@ -72,6 +71,7 @@ public:\n>>>  \t};\n>>>  \n>>>  private:\n>>> +\tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats);\n>>>  \tvoid generateZones(std::vector<RGB> &zones);\n>>>  \tvoid generateAwbStats(const ipu3_uapi_stats_3a *stats);\n>>>  \tvoid clearAwbStats();\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 8FE55BD87C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 16 Aug 2021 07:55:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D221168892;\n\tMon, 16 Aug 2021 09:55:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B83BD60260\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 16 Aug 2021 09:55:23 +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 44FF88F;\n\tMon, 16 Aug 2021 09:55:23 +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=\"prwy+RYP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629100523;\n\tbh=tcmw+3MvUejUG4vyLUj0yHA9/vH8qv/H2aXIjWyJwMw=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=prwy+RYPTzASxVr9C3PSOGayEy89aZgFBMTl1cvlFqr7awnAMhph6z+uBwt3DUTd6\n\tLla7GGE2lTaFBlBhlBYkn0qjIRhJSNKSOWu3gAbRx4XvhWH+9dBaT7JvEUuih2rSb2\n\tiFlsnoAg1wdOng6CSUAIAJalHAKRrGZSpFSolFkM=","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","References":"<20210812165243.276977-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210812165243.276977-7-jeanmichel.hautbois@ideasonboard.com>\n\t<7c22452c-710c-3b2c-e2ed-d0d86c7098f1@ideasonboard.com>\n\t<YRmoiZRjSnGlsqqi@pendragon.ideasonboard.com>","From":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","Message-ID":"<ed10afad-b1a0-e836-7ca7-e42cccb9ba2b@ideasonboard.com>","Date":"Mon, 16 Aug 2021 09:55:20 +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":"<YRmoiZRjSnGlsqqi@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 06/10] ipa: ipu3: convert AWB to\n\tthe new algorithm interface","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":18823,"web_url":"https://patchwork.libcamera.org/comment/18823/","msgid":"<YRoyDzhaHRzQyu/g@pendragon.ideasonboard.com>","date":"2021-08-16T09:38:23","subject":"Re: [libcamera-devel] [PATCH v2 06/10] ipa: ipu3: convert AWB to\n\tthe new algorithm interface","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 Mon, Aug 16, 2021 at 09:55:20AM +0200, Jean-Michel Hautbois wrote:\n> On 16/08/2021 01:51, Laurent Pinchart wrote:\n> > On Fri, Aug 13, 2021 at 10:49:56AM +0100, Kieran Bingham wrote:\n> >> On 12/08/2021 17:52, Jean-Michel Hautbois wrote:\n> >>> When the stats are received, pass it with the context to the existing\n> > \n> > s/it/them/\n> > \n> >>> AWB algorithm. IPAFrameContext now has a new structure to store the\n> >>> gains calculated byt the AWB algorithm.\n> >>\n> >> s/byt/by/\n> >>\n> >>>\n> >>> When a EventFillParams event is received, call prepare() and set the new\n> >>\n> >> /When a/When an/\n> >>\n> >>> gains accordingly in the params structure.\n> >>> There is no more a need for the IPU3Awb::initialise() function, as the\n> >>> params are always set in prepare().\n> >>>\n> >>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n> >>> ---\n> >>>  src/ipa/ipu3/ipa_context.h |  8 ++++\n> >>>  src/ipa/ipu3/ipu3.cpp      |  5 +--\n> >>>  src/ipa/ipu3/ipu3_awb.cpp  | 76 ++++++++++++++++++++------------------\n> >>>  src/ipa/ipu3/ipu3_awb.h    |  6 +--\n> >>>  4 files changed, 54 insertions(+), 41 deletions(-)\n> >>>\n> >>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> >>> index e995c663..7bb4f598 100644\n> >>> --- a/src/ipa/ipu3/ipa_context.h\n> >>> +++ b/src/ipa/ipu3/ipa_context.h\n> >>> @@ -38,6 +38,14 @@ struct IPAFrameContext {\n> >>>  \t\tdouble gain;\n> >>>  \t} agc;\n> >>>  \n> >>> +\tstruct Awb {\n> >>> +\t\tstruct Gains {\n> >>> +\t\t\tdouble redGain;\n> >>> +\t\t\tdouble greenGain;\n> >>> +\t\t\tdouble blueGain;\n> >>\n> >> They're all in a structure called gains, can they be named simpler as\n> >> just 'red', 'green', 'blue' ?\n> > \n> > Agree. Also, we don't necessarily need to name the structures here.\n> \n> I like the idea of naming the structures...\n\nThere will always be disagreements on personal preferences :-) My view\nin this case is that is clutters the structure definition for little\ngain, but I'm sure that's at least partly caused by coding experience\nthat is very personal.\n\n> >>> +\t\t} gains;\n> >>> +\t} awb;\n> >>> +\n> >>>  \tstruct Contrast {\n> >>>  \t\tuint16_t lut[IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES];\n> >>>  \t} contrast;\n> >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >>> index e09adfc3..dba5554d 100644\n> >>> --- a/src/ipa/ipu3/ipu3.cpp\n> >>> +++ b/src/ipa/ipu3/ipu3.cpp\n> >>> @@ -230,7 +230,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> >>>  \t}\n> >>>  \n> >>>  \tawbAlgo_ = std::make_unique<IPU3Awb>();\n> >>> -\tawbAlgo_->initialise(params_, context_.configuration.grid.bdsOutputSize, context_.configuration.grid.bdsGrid);\n> >>>  \n> >>>  \tagcAlgo_ = std::make_unique<IPU3Agc>();\n> >>>  \tagcAlgo_->initialise(context_.configuration.grid.bdsGrid, sensorInfo_);\n> >>> @@ -311,7 +310,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_);\n> >>> +\t\tawbAlgo_->prepare(context_, params_);\n> >>>  \n> >>>  \t*params = params_;\n> >>>  \n> >>> @@ -337,7 +336,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> >>>  \tgain = context_.frameContext.agc.gain;\n> >>>  \tgain_ = camHelper_->gainCode(gain);\n> >>>  \n> >>> -\tawbAlgo_->calculateWBGains(stats);\n> >>> +\tawbAlgo_->process(context_, stats);\n> >>>  \n> >>>  \tif (agcAlgo_->updateControls())\n> >>>  \t\tsetControls(frame);\n> >>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp\n> >>> index 0f3bcdc9..b422b095 100644\n> >>> --- a/src/ipa/ipu3/ipu3_awb.cpp\n> >>> +++ b/src/ipa/ipu3/ipu3_awb.cpp\n> >>> @@ -121,42 +121,14 @@ IPU3Awb::IPU3Awb()\n> >>>  \tasyncResults_.greenGain = 1.0;\n> >>>  \tasyncResults_.redGain = 1.0;\n> >>>  \tasyncResults_.temperatureK = 4500;\n> >>> +\n> >>> +\tzones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);\n> >>>  }\n> >>>  \n> >>>  IPU3Awb::~IPU3Awb()\n> >>>  {\n> >>>  }\n> >>>  \n> >>> -void IPU3Awb::initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid)\n> >>> -{\n> >>> -\tparams.use.acc_awb = 1;\n> >>> -\tparams.acc_param.awb.config.rgbs_thr_gr = 8191;\n> >>> -\tparams.acc_param.awb.config.rgbs_thr_r = 8191;\n> >>> -\tparams.acc_param.awb.config.rgbs_thr_gb = 8191;\n> >>> -\tparams.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;\n> >>> -\tawbGrid_ = bdsGrid;\n> >>> -\n> >>> -\tparams.use.acc_bnr = 1;\n> >>> -\tparams.acc_param.bnr = imguCssBnrDefaults;\n> >>> -\t/**\n> >>> -\t * Optical center is column (respectively row) startminus X (respectively Y) center.\n> >>> -\t * For the moment use BDS as a first approximation, but it should\n> >>> -\t * be calculated based on Shading (SHD) parameters.\n> >>> -\t */\n> >>> -\tparams.acc_param.bnr.column_size = bdsOutputSize.width;\n> >>> -\tparams.acc_param.bnr.opt_center.x_reset = awbGrid_.x_start - (bdsOutputSize.width / 2);\n> >>> -\tparams.acc_param.bnr.opt_center.y_reset = awbGrid_.y_start - (bdsOutputSize.height / 2);\n> >>> -\tparams.acc_param.bnr.opt_center_sqr.x_sqr_reset = params.acc_param.bnr.opt_center.x_reset\n> >>> -\t\t\t\t\t\t\t* params.acc_param.bnr.opt_center.x_reset;\n> >>> -\tparams.acc_param.bnr.opt_center_sqr.y_sqr_reset = params.acc_param.bnr.opt_center.y_reset\n> >>> -\t\t\t\t\t\t\t* params.acc_param.bnr.opt_center.y_reset;\n> >>> -\n> >>> -\tparams.use.acc_ccm = 1;\n> >>> -\tparams.acc_param.ccm = imguCssCcmDefault;\n> >>> -\n> >>> -\tzones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);\n> >>> -}\n> >>> -\n> >>>  /**\n> >>>   * The function estimates the correlated color temperature using\n> >>>   * from RGB color space input.\n> >>> @@ -303,17 +275,51 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)\n> >>>  \t}\n> >>>  }\n> >>>  \n> >>> -void IPU3Awb::updateWbParameters(ipu3_uapi_params &params)\n> >>> +void IPU3Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> >>> +{\n> >>> +\tcalculateWBGains(stats);\n> >>\n> >> I'd put a new line here.\n> >>\n> >> Perhaps a comment here saying \"\n> >>\n> >> /*\n> >>  * Gains are only recalculated if enough zones were detected.\n> >>  * The results are cached, so if no results were calculated, we set the\n> >>  * cached values from asyncResults_ here.\n> >>  */\n> >>\n> >>\n> >>> +\tcontext.frameContext.awb.gains.blueGain = asyncResults_.blueGain;\n> >>> +\tcontext.frameContext.awb.gains.greenGain = asyncResults_.greenGain;\n> >>> +\tcontext.frameContext.awb.gains.redGain = asyncResults_.redGain;\n> >>> +}\n> >>> +\n> >>> +void IPU3Awb::prepare(IPAContext &context, ipu3_uapi_params &params)\n> >>>  {\n> >>> +\tparams.use.acc_awb = 1;\n> >>\n> >> Set enable/use flags last... - or in this instance, after setting the\n> >> awb parameters.\n> > \n> > On this topic, btw, I tend to set structure fields in the order in which\n> > they are defined in the structure. I suppose it's a matter of personal\n> > taste.\n> > \n> >>> +\tparams.acc_param.awb.config.rgbs_thr_gr = 8191;\n> >>> +\tparams.acc_param.awb.config.rgbs_thr_r = 8191;\n> >>> +\tparams.acc_param.awb.config.rgbs_thr_gb = 8191;\n> >>> +\tparams.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;\n> > \n> > Line wrap.\n> > \n> >>> +\tawbGrid_ = context.configuration.grid.bdsGrid;\n> >>> +\n> >>> +\tparams.use.acc_bnr = 1;\n> >>\n> >> And this shows that there are modules to further break out of this...\n> > \n> > Agreed, I think Bayer noise reduction should be moved to a separate\n> > module, but note that the ImgU groups parameters in a weird way.\n> > params.acc_param.bnr contains much more than noise reduction parameters\n> > (namely at least white balance, lens shading correction and bad pixel\n> > correction). acc_bnr should thus be set if any of those algorithms are\n> > enabled, but we then need to make sure that default values are\n> > programmed for all other algorithms.\n> \n> Indeed, and that's really painful. That's why I was tempted to separate\n> into algorithms even if they access the same structure in the prepare()\n> call they will never set all the parameters, only the ones they are\n> responsible for. But then it means we need to make sure we have all the\n> defaults. Spaghetti dish you said :-/ ?\n\nI don't think it's too bad, as long as we make sure that the design\nensures that everything will be initialized in all cases.\n\n> >> Anything that has it's own 'module' on the IPU3 should have it's own\n> >> corresponding module in the code base I think.\n> > \n> > It's a good starting point, but it can sometimes make sense to group or\n> > split modules in different ways, to match how the algorithms are\n> > implemented. I haven't checked, but for instance the AWB and CCM\n> > parameters may make sense to control from a single algorithm.\n> > \n> >>> +\tparams.acc_param.bnr = imguCssBnrDefaults;\n> > \n> > Based on the above comment, maybe this should be moved to ipu3.cpp (in a\n> > separate patch though).\n> > \n> >> new line?\n> >>\n> >>> +\t/**\n> >>\n> >> s#/**#/*#\n> >>\n> >>> +\t * Optical center is column (respectively row) startminus X (respectively Y) center.\n> > \n> > I know this is moved from a different location, but I have trouble\n> > understanding the comment. The typo s/startminus/start minus/ could\n> > already be fixed here. Oh, and please reflow the comment to 80 columns.\n> > \n> >>> +\t * For the moment use BDS as a first approximation, but it should\n> >>> +\t * be calculated based on Shading (SHD) parameters.\n> >>> +\t */\n> >>> +\tSize &bdsOutputSize = context.configuration.grid.bdsOutputSize;\n> >>> +\tparams.acc_param.bnr.column_size = bdsOutputSize.width;\n> >>> +\tparams.acc_param.bnr.opt_center.x_reset = awbGrid_.x_start - (bdsOutputSize.width / 2);\n> >>> +\tparams.acc_param.bnr.opt_center.y_reset = awbGrid_.y_start - (bdsOutputSize.height / 2);\n> >>> +\tparams.acc_param.bnr.opt_center_sqr.x_sqr_reset = params.acc_param.bnr.opt_center.x_reset\n> >>> +\t\t\t\t\t\t\t* params.acc_param.bnr.opt_center.x_reset;\n> >>> +\tparams.acc_param.bnr.opt_center_sqr.y_sqr_reset = params.acc_param.bnr.opt_center.y_reset\n> >>> +\t\t\t\t\t\t\t* params.acc_param.bnr.opt_center.y_reset;\n> >>> +\n> >>> +\tparams.use.acc_ccm = 1;\n> >>\n> >> Same, but keep both BNR and CCM here in this patch, and they can move\n> >> out to their own modules separately.\n> >>\n> >> Fairly trivial comments in this patch.\n> >>\n> >> With those handled as best as you see fit,\n> >>\n> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >>\n> >>> +\tparams.acc_param.ccm = imguCssCcmDefault;\n> >>> +\n> >>>  \t/*\n> >>>  \t * Green gains should not be touched and considered 1.\n> >>>  \t * Default is 16, so do not change it at all.\n> >>>  \t * 4096 is the value for a gain of 1.0\n> >>>  \t */\n> >>> -\tparams.acc_param.bnr.wb_gains.gr = 16;\n> >>> -\tparams.acc_param.bnr.wb_gains.r = 4096 * asyncResults_.redGain;\n> >>> -\tparams.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain;\n> >>> -\tparams.acc_param.bnr.wb_gains.gb = 16;\n> >>> +\tparams.acc_param.bnr.wb_gains.gr = 16 * context.frameContext.awb.gains.greenGain;\n> >>> +\tparams.acc_param.bnr.wb_gains.r = 4096 * context.frameContext.awb.gains.redGain;\n> >>> +\tparams.acc_param.bnr.wb_gains.b = 4096 * context.frameContext.awb.gains.blueGain;\n> >>> +\tparams.acc_param.bnr.wb_gains.gb = 16 * context.frameContext.awb.gains.greenGain;\n> > \n> > Does this mean that a gain of 1.0 is encoded as 16 for the green\n> > channels and 4096 for the red and blue channels ?\n> > \n> > This also doesn't match the comment in intel-ipu3.h that states\n> > \n> > /**\n> >  * struct ipu3_uapi_bnr_static_config_wb_gains_config - White balance gains\n> >  *\n> >  * @gr: white balance gain for Gr channel.\n> >  * @r:  white balance gain for R channel.\n> >  * @b:  white balance gain for B channel.\n> >  * @gb: white balance gain for Gb channel.\n> >  *\n> >  * Precision u3.13, range [0, 8). White balance correction is done by applying\n> >  * a multiplicative gain to each color channels prior to BNR.\n> >  */\n> > \n> > This indicates that 1.0 maps to 8192. Do we have evidence that this is\n> > incorrect ?\n> \n> This is something which is a bad comprehension (from me) in the first\n> place. I did not want to change that here, as it is a move.\n> You are right, a gain of 1.0 is mapped to 8192. Doing so would lead to\n> greenish frames as we don't have the corresponding CCM to compensate.\n\nCould you give it a try and share the result ?\n\n> >>>  \n> >>>  \tLOG(IPU3Awb, Debug) << \"Color temperature estimated: \" << asyncResults_.temperatureK;\n> >>>  \n> >>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h\n> >>> index eeb2920b..4de3fae2 100644\n> >>> --- a/src/ipa/ipu3/ipu3_awb.h\n> >>> +++ b/src/ipa/ipu3/ipu3_awb.h\n> >>> @@ -29,9 +29,8 @@ public:\n> >>>  \tIPU3Awb();\n> >>>  \t~IPU3Awb();\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);\n> >>> +\tvoid prepare(IPAContext &context, ipu3_uapi_params &params) override;\n> >>> +\tvoid process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;\n> >>>  \n> >>>  \tstruct Ipu3AwbCell {\n> >>>  \t\tunsigned char greenRedAvg;\n> >>> @@ -72,6 +71,7 @@ public:\n> >>>  \t};\n> >>>  \n> >>>  private:\n> >>> +\tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats);\n> >>>  \tvoid generateZones(std::vector<RGB> &zones);\n> >>>  \tvoid generateAwbStats(const ipu3_uapi_stats_3a *stats);\n> >>>  \tvoid clearAwbStats();","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 7DC98BD87D\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 16 Aug 2021 09:38:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC68268890;\n\tMon, 16 Aug 2021 11:38:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F3E1F60260\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 16 Aug 2021 11:38:29 +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 63C503E5;\n\tMon, 16 Aug 2021 11:38:29 +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=\"a2niqTjm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629106709;\n\tbh=vWOmSWcCqnyumtD5Iq2ZHik76eGxGDKyk4CTIZX6j84=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=a2niqTjmMWpZahZkdP2B/x4l67QeBDoaekxCcH7XVDrKUF7lwQ8I89cf/1cmz3E4p\n\trnJ3jNGfcNBCG9W7PSQX9TldBcvHajF/rk/Xf/esMO9AK6qNefMFY2QvXV0L9p7V7j\n\tJ6NG6IQutSJXltP7ETvjByoRob5HgLvo11RzZYA0=","Date":"Mon, 16 Aug 2021 12:38:23 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","Message-ID":"<YRoyDzhaHRzQyu/g@pendragon.ideasonboard.com>","References":"<20210812165243.276977-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210812165243.276977-7-jeanmichel.hautbois@ideasonboard.com>\n\t<7c22452c-710c-3b2c-e2ed-d0d86c7098f1@ideasonboard.com>\n\t<YRmoiZRjSnGlsqqi@pendragon.ideasonboard.com>\n\t<ed10afad-b1a0-e836-7ca7-e42cccb9ba2b@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<ed10afad-b1a0-e836-7ca7-e42cccb9ba2b@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 06/10] ipa: ipu3: convert AWB to\n\tthe new algorithm interface","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>"}}]