[{"id":23470,"web_url":"https://patchwork.libcamera.org/comment/23470/","msgid":"","date":"2022-06-17T20:39:10","subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: awb: Correct the\n\tcoefficient factor","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\nI'd write \"Correct the gains calculation\" in the subject, as it's not\njust the factor.\n\nOn Fri, Jun 17, 2022 at 10:32:11AM +0200, Jean-Michel Hautbois via libcamera-devel wrote:\n> The factor used right now in the IPU3 is 8192, as a multiplier of the\n> estimated gain. This is wrong, as the isp is adding 1.0 to the gain\n> applied, ie Pout = { Pin * (1 + Gx) }.\n> \n> Fix it, and to ease the reading, introduce a small helper function.\n\nThe effect of this patch on image quality is really great !\n\n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/awb.cpp | 21 +++++++++++++++++----\n> src/ipa/ipu3/algorithms/awb.h | 1 +\n> 2 files changed, 18 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> index 5c232d92..6abaf75f 100644\n> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> @@ -409,6 +409,19 @@ constexpr uint16_t Awb::threshold(float value)\n> \treturn value * 8191;\n> }\n> \n> +constexpr uint16_t Awb::fixedGain(double gain)\n\nDoes \"fixed\" refer to the fact that the return value is a fixed-point\nnumber ? As the function does more than conversion to fixed point, I'd\ncall it gainValue().\n\n> +{\n> +\t/*\n> +\t * For BNR parameters WB gain factor for the three channels\n\nI count four channels :-)\n\n> +\t * [Ggr, Ggb, Gb, Gr]. Their precision is U3.13 and the range is (0, 8)\n\nGb and Gr are slightly confusing here, they usually refer to the\ngreen-blue and green-red components, while I suppose they stand for\ngain-blue and gain-red in this context.\n\n> +\t * and the actual gain is Gx + 1, it is typically Gx = 1.\n> +\t *\n> +\t * Pout = {Pin * (1 + Gx)}.\n> +\t */\n\nIf I may propose a small improvement to the documentation (as it's\nimportant to record this, given that the kernel API doesn't document the\ngain format correctly):\n\n\t/*\n\t * The colour gains applied by the BNR for the four channels (Gr, R, B\n\t * and Gb) are expressed in the parameters structure as 16-bit integers\n\t * that store a fixed-point U3.13 value in the range [0, 8[.\n\t *\n\t * The real gain value is equal to the gain parameter plus one, i.e.\n\t *\n\t * Pout = Pin * (1 * gain / 8192)\n\t *\n\t * where 'Pin' is the input pixel value, 'Pout' the output pixel value,\n\t * and 'gain' the gain in the parameters structure as a 16-bit integer.\n\t */\n\nYou can also write [0, 8) instead of [0, 8[.\n\n> +\tgain = std::clamp((gain - 1.0), 0.0, 8.0);\n\nNo need for the inner parentheses.\n\n> +\treturn gain * 8192;\n\nAnd this holds on a single line:\n\n\treturn std::clamp(gain - 1.0, 0.0, 8.0) * 8192;\n\nBut we should actually limit the value to [0.0, 8.0[, not [0.0, 8.0].\nOne option would be\n\n\treturn std::clamp((gain - 1.0) * 8192, 0.0, 65535.0);\n\n> +}\n> +\n> /**\n> * \\copydoc libcamera::ipa::Algorithm::prepare\n> */\n> @@ -451,10 +464,10 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)\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> \t/* Convert to u3.13 fixed point values */\n\nLet's drop this comment, it's now part of fixedGain().\n\nReviewed-by: Laurent Pinchart \n\n> -\tparams->acc_param.bnr.wb_gains.gr = 8192 * context.activeState.awb.gains.green;\n> -\tparams->acc_param.bnr.wb_gains.r = 8192 * context.activeState.awb.gains.red;\n> -\tparams->acc_param.bnr.wb_gains.b = 8192 * context.activeState.awb.gains.blue;\n> -\tparams->acc_param.bnr.wb_gains.gb = 8192 * context.activeState.awb.gains.green;\n> +\tparams->acc_param.bnr.wb_gains.gr = fixedGain(context.activeState.awb.gains.green);\n> +\tparams->acc_param.bnr.wb_gains.r = fixedGain(context.activeState.awb.gains.red);\n> +\tparams->acc_param.bnr.wb_gains.b = fixedGain(context.activeState.awb.gains.blue);\n> +\tparams->acc_param.bnr.wb_gains.gb = fixedGain(context.activeState.awb.gains.green);\n> \n> \tLOG(IPU3Awb, Debug) << \"Color temperature estimated: \" << asyncResults_.temperatureK;\n> \n> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h\n> index 9a50a985..3154541d 100644\n> --- a/src/ipa/ipu3/algorithms/awb.h\n> +++ b/src/ipa/ipu3/algorithms/awb.h\n> @@ -73,6 +73,7 @@ private:\n> \tvoid awbGreyWorld();\n> \tuint32_t estimateCCT(double red, double green, double blue);\n> \tstatic constexpr uint16_t threshold(float value);\n> +\tstatic constexpr uint16_t fixedGain(double gain);\n> \n> \tstd::vector zones_;\n> \tAccumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C66C0BD161\n\tfor ;\n\tFri, 17 Jun 2022 20:39:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DA83565635;\n\tFri, 17 Jun 2022 22:39: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 9079865632\n\tfor ;\n\tFri, 17 Jun 2022 22:39:22 +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 04E74268;\n\tFri, 17 Jun 2022 22:39:21 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655498363;\n\tbh=uUfuMocfkdFD0ylbNDSZTOWABMM0OXxqQKkuzR5TiqY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=1thiYoadOi40KCFVRB+KQMNq6da27eDueuHs5e6Bf3Fb1Coh+jt4PLoFdCwPiZUP/\n\tt+dbavsu2OsQcpZNCU5NtoJ5PxWfGtN7duolM1XwfQrr5kB3j5JSsxuod1rHpqKwgN\n\tYwGCpnCP1b3KEZVxM71ibbmCVL8YLnUQALxPfLKcBabeK6Tgj7tzcTbgAaGBA5FkdY\n\t2mQaScyIMJT2xd66/PwuaiwTpYErn+Bl5bYyxqt7wvaAU4zrMvxRt/YbNmdkdY6YQv\n\tCgIOTOVphBJe1WQoBdezKwizcdJgPa9WjWV6UlqdvX/cVFXijojSr9THVaLnXmH5SD\n\tSj7+QYHsOEKhQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655498362;\n\tbh=uUfuMocfkdFD0ylbNDSZTOWABMM0OXxqQKkuzR5TiqY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GyUxmEotYeLpyK9VhUOdtQd+bUw+19kczdYSxclWgjLxCPuhnuvBonUynaI7bG8Or\n\t9WJY2CMKM0vg4ET/vbis69gsrgzrQeDgHXPb1Ry4bX7iniQ+sKgP5fbJhlpgnHbm7c\n\tjHMaetufGQD5VDSPOtZAGscxmSz/MLF9jLXUYN60="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"GyUxmEot\"; dkim-atps=neutral","Date":"Fri, 17 Jun 2022 23:39:10 +0300","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20220617083211.28407-1-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220617083211.28407-1-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: awb: Correct the\n\tcoefficient factor","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23471,"web_url":"https://patchwork.libcamera.org/comment/23471/","msgid":"<6023c6ed-c74f-6dde-7985-1d917b49c30c@ideasonboard.com>","date":"2022-06-17T21:25:53","subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: awb: Correct the\n\tcoefficient factor","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 17/06/2022 22:39, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> Thank you for the patch.\n> \n> I'd write \"Correct the gains calculation\" in the subject, as it's not\n> just the factor.\n> \n> On Fri, Jun 17, 2022 at 10:32:11AM +0200, Jean-Michel Hautbois via libcamera-devel wrote:\n>> The factor used right now in the IPU3 is 8192, as a multiplier of the\n>> estimated gain. This is wrong, as the isp is adding 1.0 to the gain\n>> applied, ie Pout = { Pin * (1 + Gx) }.\n>>\n>> Fix it, and to ease the reading, introduce a small helper function.\n> \n> The effect of this patch on image quality is really great !\n> \n>> Signed-off-by: Jean-Michel Hautbois \n>> ---\n>> src/ipa/ipu3/algorithms/awb.cpp | 21 +++++++++++++++++----\n>> src/ipa/ipu3/algorithms/awb.h | 1 +\n>> 2 files changed, 18 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n>> index 5c232d92..6abaf75f 100644\n>> --- a/src/ipa/ipu3/algorithms/awb.cpp\n>> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n>> @@ -409,6 +409,19 @@ constexpr uint16_t Awb::threshold(float value)\n>> \treturn value * 8191;\n>> }\n>> \n>> +constexpr uint16_t Awb::fixedGain(double gain)\n> \n> Does \"fixed\" refer to the fact that the return value is a fixed-point\n> number ? As the function does more than conversion to fixed point, I'd\n> call it gainValue().\n> \n>> +{\n>> +\t/*\n>> +\t * For BNR parameters WB gain factor for the three channels\n> \n> I count four channels :-)\n> \n>> +\t * [Ggr, Ggb, Gb, Gr]. Their precision is U3.13 and the range is (0, 8)\n> \n> Gb and Gr are slightly confusing here, they usually refer to the\n> green-blue and green-red components, while I suppose they stand for\n> gain-blue and gain-red in this context.\n> \n>> +\t * and the actual gain is Gx + 1, it is typically Gx = 1.\n>> +\t *\n>> +\t * Pout = {Pin * (1 + Gx)}.\n>> +\t */\n> \n> If I may propose a small improvement to the documentation (as it's\n> important to record this, given that the kernel API doesn't document the\n> gain format correctly):\n> \n> \t/*\n> \t * The colour gains applied by the BNR for the four channels (Gr, R, B\n> \t * and Gb) are expressed in the parameters structure as 16-bit integers\n> \t * that store a fixed-point U3.13 value in the range [0, 8[.\n> \t *\n> \t * The real gain value is equal to the gain parameter plus one, i.e.\n> \t *\n> \t * Pout = Pin * (1 * gain / 8192)\n> \t *\n> \t * where 'Pin' is the input pixel value, 'Pout' the output pixel value,\n> \t * and 'gain' the gain in the parameters structure as a 16-bit integer.\n> \t */\n\nThanks for the documentation rewriting !\nI think it should be:\n\n- * Pout = Pin * (1 * gain / 8192)\n+ * Pout = Pin * (1 + gain / 8192)\n\n> \n> You can also write [0, 8) instead of [0, 8[.\n> \n>> +\tgain = std::clamp((gain - 1.0), 0.0, 8.0);\n> \n> No need for the inner parentheses.\n> \n>> +\treturn gain * 8192;\n> \n> And this holds on a single line:\n> \n> \treturn std::clamp(gain - 1.0, 0.0, 8.0) * 8192;\n> \n> But we should actually limit the value to [0.0, 8.0[, not [0.0, 8.0].\n> One option would be\n> \n> \treturn std::clamp((gain - 1.0) * 8192, 0.0, 65535.0);\n\nI like this one :-) !\n\n> \n>> +}\n>> +\n>> /**\n>> * \\copydoc libcamera::ipa::Algorithm::prepare\n>> */\n>> @@ -451,10 +464,10 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)\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>> \t/* Convert to u3.13 fixed point values */\n> \n> Let's drop this comment, it's now part of fixedGain().\nIndeed.\n\n> \n> Reviewed-by: Laurent Pinchart \n\nThanks.\n\n> \n>> -\tparams->acc_param.bnr.wb_gains.gr = 8192 * context.activeState.awb.gains.green;\n>> -\tparams->acc_param.bnr.wb_gains.r = 8192 * context.activeState.awb.gains.red;\n>> -\tparams->acc_param.bnr.wb_gains.b = 8192 * context.activeState.awb.gains.blue;\n>> -\tparams->acc_param.bnr.wb_gains.gb = 8192 * context.activeState.awb.gains.green;\n>> +\tparams->acc_param.bnr.wb_gains.gr = fixedGain(context.activeState.awb.gains.green);\n>> +\tparams->acc_param.bnr.wb_gains.r = fixedGain(context.activeState.awb.gains.red);\n>> +\tparams->acc_param.bnr.wb_gains.b = fixedGain(context.activeState.awb.gains.blue);\n>> +\tparams->acc_param.bnr.wb_gains.gb = fixedGain(context.activeState.awb.gains.green);\n>> \n>> \tLOG(IPU3Awb, Debug) << \"Color temperature estimated: \" << asyncResults_.temperatureK;\n>> \n>> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h\n>> index 9a50a985..3154541d 100644\n>> --- a/src/ipa/ipu3/algorithms/awb.h\n>> +++ b/src/ipa/ipu3/algorithms/awb.h\n>> @@ -73,6 +73,7 @@ private:\n>> \tvoid awbGreyWorld();\n>> \tuint32_t estimateCCT(double red, double green, double blue);\n>> \tstatic constexpr uint16_t threshold(float value);\n>> +\tstatic constexpr uint16_t fixedGain(double gain);\n>> \n>> \tstd::vector zones_;\n>> \tAccumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4F9CCBD808\n\tfor ;\n\tFri, 17 Jun 2022 21:25:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B08E965635;\n\tFri, 17 Jun 2022 23:25:57 +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 910DF65632\n\tfor ;\n\tFri, 17 Jun 2022 23:25:56 +0200 (CEST)","from [IPV6:2a01:e0a:169:7140:dd2e:f9c7:5350:5a2c] (unknown\n\t[IPv6:2a01:e0a:169:7140:dd2e:f9c7:5350:5a2c])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 35EE3268;\n\tFri, 17 Jun 2022 23:25:56 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655501157;\n\tbh=9x52oQzJl3eTTpCH9TdW2d8r0ToIQ9RX//W+J5j08Gg=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=p0JrR7WkVxBYiaY4bnfjr3HoUhrsJxYYxKlqBJp1eN9NJ80I4j6HQrS0VrOz3O3ci\n\tyaIFZWP1Js4r0gqrJSQAGT8xgi3xQsG82kuymH4uBo0OBg8U5D9mYs5HC6ho7n7Lam\n\tYkT+7DtwPTAtBPndGhN1IqkveMeW6GEar5FCnyulQnhx0KUpAfM/EnEx9YkN9GylYW\n\tk9HFyLB+vkiGukYOlg7KgRTRg2uYaFbbM8dfJLLntwNxFAxRbX5qVJC6oAgWt/k9Y3\n\tFZQ1sdToCEVf7SR82YvJfdmcC/DB+O+bhid87qLFyIAX8yCfNG8JJiGAeVrCpU2LZJ\n\tZ1+4O5VI63haQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655501156;\n\tbh=9x52oQzJl3eTTpCH9TdW2d8r0ToIQ9RX//W+J5j08Gg=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=oBl2LqiHhfT3N+1ZwREb2LHxVzTLJ+CNmOFd5wUqjus4THxV9WiHn5wS/OrbJp3lL\n\tx2Q03no2H7GxsiPQNJaDf/apSeyG5iU+gwgz3i/HF+KFG/rK2tQouY6GuCnjQSjfO0\n\tURoQisS96+0mgnC8ryswbKIZBGR6Aqym9UkzsJS0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"oBl2LqiH\"; dkim-atps=neutral","Message-ID":"<6023c6ed-c74f-6dde-7985-1d917b49c30c@ideasonboard.com>","Date":"Fri, 17 Jun 2022 23:25:53 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.9.1","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220617083211.28407-1-jeanmichel.hautbois@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: awb: Correct the\n\tcoefficient factor","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jean-Michel Hautbois via libcamera-devel\n\t","Reply-To":"Jean-Michel Hautbois ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23472,"web_url":"https://patchwork.libcamera.org/comment/23472/","msgid":"","date":"2022-06-17T21:49:37","subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: awb: Correct the\n\tcoefficient factor","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, Jun 17, 2022 at 11:25:53PM +0200, Jean-Michel Hautbois wrote:\n> On 17/06/2022 22:39, Laurent Pinchart wrote:\n> > Hi Jean-Michel,\n> > \n> > Thank you for the patch.\n> > \n> > I'd write \"Correct the gains calculation\" in the subject, as it's not\n> > just the factor.\n> > \n> > On Fri, Jun 17, 2022 at 10:32:11AM +0200, Jean-Michel Hautbois via libcamera-devel wrote:\n> >> The factor used right now in the IPU3 is 8192, as a multiplier of the\n> >> estimated gain. This is wrong, as the isp is adding 1.0 to the gain\n> >> applied, ie Pout = { Pin * (1 + Gx) }.\n> >>\n> >> Fix it, and to ease the reading, introduce a small helper function.\n> > \n> > The effect of this patch on image quality is really great !\n> > \n> >> Signed-off-by: Jean-Michel Hautbois \n> >> ---\n> >> src/ipa/ipu3/algorithms/awb.cpp | 21 +++++++++++++++++----\n> >> src/ipa/ipu3/algorithms/awb.h | 1 +\n> >> 2 files changed, 18 insertions(+), 4 deletions(-)\n> >>\n> >> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> >> index 5c232d92..6abaf75f 100644\n> >> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> >> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> >> @@ -409,6 +409,19 @@ constexpr uint16_t Awb::threshold(float value)\n> >> \treturn value * 8191;\n> >> }\n> >> \n> >> +constexpr uint16_t Awb::fixedGain(double gain)\n> > \n> > Does \"fixed\" refer to the fact that the return value is a fixed-point\n> > number ? As the function does more than conversion to fixed point, I'd\n> > call it gainValue().\n> > \n> >> +{\n> >> +\t/*\n> >> +\t * For BNR parameters WB gain factor for the three channels\n> > \n> > I count four channels :-)\n> > \n> >> +\t * [Ggr, Ggb, Gb, Gr]. Their precision is U3.13 and the range is (0, 8)\n> > \n> > Gb and Gr are slightly confusing here, they usually refer to the\n> > green-blue and green-red components, while I suppose they stand for\n> > gain-blue and gain-red in this context.\n> > \n> >> +\t * and the actual gain is Gx + 1, it is typically Gx = 1.\n> >> +\t *\n> >> +\t * Pout = {Pin * (1 + Gx)}.\n> >> +\t */\n> > \n> > If I may propose a small improvement to the documentation (as it's\n> > important to record this, given that the kernel API doesn't document the\n> > gain format correctly):\n> > \n> > \t/*\n> > \t * The colour gains applied by the BNR for the four channels (Gr, R, B\n> > \t * and Gb) are expressed in the parameters structure as 16-bit integers\n> > \t * that store a fixed-point U3.13 value in the range [0, 8[.\n> > \t *\n> > \t * The real gain value is equal to the gain parameter plus one, i.e.\n> > \t *\n> > \t * Pout = Pin * (1 * gain / 8192)\n> > \t *\n> > \t * where 'Pin' is the input pixel value, 'Pout' the output pixel value,\n> > \t * and 'gain' the gain in the parameters structure as a 16-bit integer.\n> > \t */\n> \n> Thanks for the documentation rewriting !\n> I think it should be:\n> \n> - * Pout = Pin * (1 * gain / 8192)\n> + * Pout = Pin * (1 + gain / 8192)\n\nIndeed, that's what I meant. If we cross-review our reviews we'll\nachieve a good result :-)\n\n> > \n> > You can also write [0, 8) instead of [0, 8[.\n> > \n> >> +\tgain = std::clamp((gain - 1.0), 0.0, 8.0);\n> > \n> > No need for the inner parentheses.\n> > \n> >> +\treturn gain * 8192;\n> > \n> > And this holds on a single line:\n> > \n> > \treturn std::clamp(gain - 1.0, 0.0, 8.0) * 8192;\n> > \n> > But we should actually limit the value to [0.0, 8.0[, not [0.0, 8.0].\n> > One option would be\n> > \n> > \treturn std::clamp((gain - 1.0) * 8192, 0.0, 65535.0);\n> \n> I like this one :-) !\n> \n> >> +}\n> >> +\n> >> /**\n> >> * \\copydoc libcamera::ipa::Algorithm::prepare\n> >> */\n> >> @@ -451,10 +464,10 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)\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> >> \t/* Convert to u3.13 fixed point values */\n> > \n> > Let's drop this comment, it's now part of fixedGain().\n> \n> Indeed.\n> \n> > \n> > Reviewed-by: Laurent Pinchart \n> \n> Thanks.\n> \n> >> -\tparams->acc_param.bnr.wb_gains.gr = 8192 * context.activeState.awb.gains.green;\n> >> -\tparams->acc_param.bnr.wb_gains.r = 8192 * context.activeState.awb.gains.red;\n> >> -\tparams->acc_param.bnr.wb_gains.b = 8192 * context.activeState.awb.gains.blue;\n> >> -\tparams->acc_param.bnr.wb_gains.gb = 8192 * context.activeState.awb.gains.green;\n> >> +\tparams->acc_param.bnr.wb_gains.gr = fixedGain(context.activeState.awb.gains.green);\n> >> +\tparams->acc_param.bnr.wb_gains.r = fixedGain(context.activeState.awb.gains.red);\n> >> +\tparams->acc_param.bnr.wb_gains.b = fixedGain(context.activeState.awb.gains.blue);\n> >> +\tparams->acc_param.bnr.wb_gains.gb = fixedGain(context.activeState.awb.gains.green);\n> >> \n> >> \tLOG(IPU3Awb, Debug) << \"Color temperature estimated: \" << asyncResults_.temperatureK;\n> >> \n> >> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h\n> >> index 9a50a985..3154541d 100644\n> >> --- a/src/ipa/ipu3/algorithms/awb.h\n> >> +++ b/src/ipa/ipu3/algorithms/awb.h\n> >> @@ -73,6 +73,7 @@ private:\n> >> \tvoid awbGreyWorld();\n> >> \tuint32_t estimateCCT(double red, double green, double blue);\n> >> \tstatic constexpr uint16_t threshold(float value);\n> >> +\tstatic constexpr uint16_t fixedGain(double gain);\n> >> \n> >> \tstd::vector zones_;\n> >> \tAccumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0F9F5BD161\n\tfor ;\n\tFri, 17 Jun 2022 21:49:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4037F65633;\n\tFri, 17 Jun 2022 23:49:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D70DC65632\n\tfor ;\n\tFri, 17 Jun 2022 23:49:49 +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 5BFE12A5;\n\tFri, 17 Jun 2022 23:49:49 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655502591;\n\tbh=HzSMrJHJT3Mb5gTgSpxL4RVOjvHxqS5TXWIfzVHWdYI=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=icyZ8XHzruav4TzwmM0wCH1Y4SBqSPRGLW+DprYvdDMOafRf6b1eXSOoBqdJDOdFf\n\t8p820ZJ+R1cwXdNfNUxnJ+2+xePNY2SI7qVcyH4XOQvMtLYiiPtC5+p7x315+W6ftf\n\tpFg8qwK8zUhA2DzHGTn3bkGvI2xuH9Oop+FQv0tWSXug7waifz0/ZVtFVH8IxAPYr4\n\tpK2EbppgdzrDcgZPkMDaJC/iGsw8vfKJCLpgPT/Ey0aFGJ5VvxzCZ7UaXQYmaGoIQW\n\t3s3Ul7q9NNgjv7EXpCo4Sq5rzA2o+fDVZVr8IRjZ6eJ1T35OyGZRnOgNTjgA9eMlCd\n\ta2b6PU3TPxDAA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655502589;\n\tbh=HzSMrJHJT3Mb5gTgSpxL4RVOjvHxqS5TXWIfzVHWdYI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vupRXULWaylIlziRJMCsk48z4ElzdFTjcoSUxWSr39mXmlNOYzRvuVo+9k/a1BD5C\n\tKRIcUowhaXsStBHygO9iVCDj7XbO9jU2f7U1BsHxxnvioP2xdzb0p7Auz1mTaBXTE1\n\t9ljryz+fhunn7uEr9KsRXzYF78c1cQepWL0nDYtE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"vupRXULW\"; dkim-atps=neutral","Date":"Sat, 18 Jun 2022 00:49:37 +0300","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20220617083211.28407-1-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<6023c6ed-c74f-6dde-7985-1d917b49c30c@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<6023c6ed-c74f-6dde-7985-1d917b49c30c@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] ipa: ipu3: awb: Correct the\n\tcoefficient factor","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]