[{"id":25042,"web_url":"https://patchwork.libcamera.org/comment/25042/","msgid":"<166371709003.18961.18168088016364343200@Monstersaurus>","date":"2022-09-20T23:38:10","subject":"Re: [libcamera-devel] [PATCH v4 29/32] ipa: rkisp1: awb: Prevent\n\tRGB means from being negative","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:57)\n> Due to hardware rounding errors in the YCbCr means, the calculated RGB\n> means may be negative. This would lead to negative gains, messing up\n> calculation. Prevent this by clamping the means to positive values.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/rkisp1/algorithms/awb.cpp | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> index ed91e9277a16..de54c4d24650 100644\n> --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> @@ -242,6 +242,16 @@ void Awb::process(IPAContext &context,\n> redMean = 1.1636 * yMean - 0.0623 * cbMean + 1.6008 * crMean;\n> greenMean = 1.1636 * yMean - 0.4045 * cbMean - 0.7949 * crMean;\n> blueMean = 1.1636 * yMean + 1.9912 * cbMean - 0.0250 * crMean;\n> +\n> + /*\n> + * Due to hardware rounding errors in the YCbCr means, the\n> + * calculated RGB means may be negative. This would lead to\n> + * negative gains, messing up calculation. Prevent this by\n> + * clamping the means to positive values.\n> + */\n> + redMean = std::max(redMean, 0.0);\n> + greenMean = std::max(greenMean, 0.0);\n> + blueMean = std::max(blueMean, 0.0);\n\nWe don't need to clamp to a value greater than zero for any reason here\ndo we?\n\nI was trying to conjour up in my head if perhaps this needs to clamp to\na max with 1.0 for any reason, but these are not the gains - but the\nvalues used later I believe.\n\nSo I think it sounds fine...\n\n\nReviewed-by: Kieran Bingham \n\n> }\n> \n> /*\n> -- \n> Regards,\n> \n> Laurent Pinchart\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 491C3C0DA4\n\tfor ;\n\tTue, 20 Sep 2022 23:38:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AA89C621D6;\n\tWed, 21 Sep 2022 01:38:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D702861F7D\n\tfor ;\n\tWed, 21 Sep 2022 01:38:12 +0200 (CEST)","from pendragon.ideasonboard.com\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 575AD415;\n\tWed, 21 Sep 2022 01:38:12 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663717093;\n\tbh=lcRAQaDeBrv7gUdNln4J5UL2l0UV9HCvhUdBBl7eNoM=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=DfoGUDmUBSFhLUxm86v8/+wFal7SSE22vohswRpOSEy5vh8SAc4RGoCVxZcDV8mSw\n\tYoqz1ZUwIOx4Rt5VWs0P9ZcWzpzvDn6xCKKy84HvAWCR3Hf9RqmnGU9uEBRWgCMIse\n\tecjvJbsLrgWEsnwKAAaLX11amcxE5Q5Zwd99PZkuTmRolvQQq6StZ8gQRF1hWR495l\n\tfEJKjoi0pf51/5mpe2RrBAhBm7X4kckj1I3FxMaL13+bAfNFI/iL2Or8PQYn9hHxnn\n\t36k4X9O0ySZmn7CQG11CRVAtfp7GXaxUtWjnE8F5TpNWf3+LV5m7+Iy7Zu+poAmTjK\n\tuPymzwfxzjLUA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663717092;\n\tbh=lcRAQaDeBrv7gUdNln4J5UL2l0UV9HCvhUdBBl7eNoM=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=BLeHuHi3pGtbQDdCaOHYyvPmCUNeO/r/feX67yk8urqWDloo5sguIJ8AvF/YMyzAM\n\tQDFNOBZtzyueMKPXeoBDtiBW+bZmKq5aZmWTV/V58RmRYmEPUG/xsuv9ofzBirA2Sf\n\tIAq/S1hQVatitgYiYV1nKW+j5Zwo6tmkekYkG4Iw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"BLeHuHi3\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220908014200.28728-30-laurent.pinchart@ideasonboard.com>","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-30-laurent.pinchart@ideasonboard.com>","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 21 Sep 2022 00:38:10 +0100","Message-ID":"<166371709003.18961.18168088016364343200@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v4 29/32] ipa: rkisp1: awb: Prevent\n\tRGB means from being negative","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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25079,"web_url":"https://patchwork.libcamera.org/comment/25079/","msgid":"<20220922104248.kx6ucisoyzfkcrbf@uno.localdomain>","date":"2022-09-22T10:42:48","subject":"Re: [libcamera-devel] [PATCH v4 29/32] ipa: rkisp1: awb: Prevent\n\tRGB means from being negative","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Thu, Sep 08, 2022 at 04:41:57AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> Due to hardware rounding errors in the YCbCr means, the calculated RGB\n\nIs it due to rounding or simply because\nthe values of yMean, cbMean and crMean\n\n\t\tdouble yMean = awb->awb_mean[0].mean_y_or_g;\n\t\tdouble cbMean = awb->awb_mean[0].mean_cb_or_b;\n\t\tdouble crMean = awb->awb_mean[0].mean_cr_or_r;\n\n\t\t/*\n\t\t * Convert from YCbCr to RGB.\n\t\t * The hardware uses the following formulas:\n\t\t * Y = 16 + 0.2500 R + 0.5000 G + 0.1094 B\n\t\t * Cb = 128 - 0.1406 R - 0.2969 G + 0.4375 B\n\t\t * Cr = 128 + 0.4375 R - 0.3750 G - 0.0625 B\n\t\t *\n\t\t * The inverse matrix is thus:\n\t\t * [[1,1636, -0,0623, 1,6008]\n\t\t * [1,1636, -0,4045, -0,7949]\n\t\t * [1,1636, 1,9912, -0,0250]]\n\t\t */\n\t\tyMean -= 16;\n\t\tcbMean -= 128;\n\t\tcrMean -= 128;\n\nare smaller than the above offsets ? I guess this can happen when the\nimage is particularly dark, right ?\n\nAnyway, this certainly fixes an issue\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> means may be negative. This would lead to negative gains, messing up\n> calculation. Prevent this by clamping the means to positive values.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/rkisp1/algorithms/awb.cpp | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> index ed91e9277a16..de54c4d24650 100644\n> --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> @@ -242,6 +242,16 @@ void Awb::process(IPAContext &context,\n> \t\tredMean = 1.1636 * yMean - 0.0623 * cbMean + 1.6008 * crMean;\n> \t\tgreenMean = 1.1636 * yMean - 0.4045 * cbMean - 0.7949 * crMean;\n> \t\tblueMean = 1.1636 * yMean + 1.9912 * cbMean - 0.0250 * crMean;\n> +\n> +\t\t/*\n> +\t\t * Due to hardware rounding errors in the YCbCr means, the\n> +\t\t * calculated RGB means may be negative. This would lead to\n> +\t\t * negative gains, messing up calculation. Prevent this by\n> +\t\t * clamping the means to positive values.\n> +\t\t */\n> +\t\tredMean = std::max(redMean, 0.0);\n> +\t\tgreenMean = std::max(greenMean, 0.0);\n> +\t\tblueMean = std::max(blueMean, 0.0);\n> \t}\n>\n> \t/*\n> --\n> Regards,\n>\n> Laurent Pinchart\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 34204C3272\n\tfor ;\n\tThu, 22 Sep 2022 10:42:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A0B5E62203;\n\tThu, 22 Sep 2022 12:42:51 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3AE896219A\n\tfor ;\n\tThu, 22 Sep 2022 12:42:50 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 84FA624000E;\n\tThu, 22 Sep 2022 10:42:49 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663843371;\n\tbh=T2G2ibyQYw/CwHLcRmA+/Xng8URYcGcCkdNSO5TlF38=;\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=AurKXgC+/6MmX/69+U3phPHXujEkQ/pLZlXkMOiy/9V/wAYdUe0AR/y/23uUnJGMB\n\tpsboPxOmKodpsWKrPShzCZdp0LjOhUg9fU6VpNngbXIdEZzRYnswH8I3O0F/QOe7JQ\n\trdsHNh25kDqLH4Ps1qavezdWMPtolyt8fKQ1t4UA0AhfcZXyiGZStm077zDscBoysF\n\tO6hjvhOR5s3ggD1/g4iF2f1PCetY3A2LmESs+HoGfneP005W8anK8oWXruvY6yq+JK\n\t6/vEX4Hc5kjZTe2Y+Covtb/3WJYhTG18GbPTwtvZWJ76Vok1W9MAAgygGDFJAc98p4\n\th9pKTlD20DmWg==","Date":"Thu, 22 Sep 2022 12:42:48 +0200","To":"Laurent Pinchart ","Message-ID":"<20220922104248.kx6ucisoyzfkcrbf@uno.localdomain>","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-30-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220908014200.28728-30-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 29/32] ipa: rkisp1: awb: Prevent\n\tRGB means from being negative","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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25102,"web_url":"https://patchwork.libcamera.org/comment/25102/","msgid":"","date":"2022-09-22T22:47:15","subject":"Re: [libcamera-devel] [PATCH v4 29/32] ipa: rkisp1: awb: Prevent\n\tRGB means from being negative","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Sep 22, 2022 at 12:42:48PM +0200, Jacopo Mondi wrote:\n> Hi Laurent\n> \n> On Thu, Sep 08, 2022 at 04:41:57AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > Due to hardware rounding errors in the YCbCr means, the calculated RGB\n> \n> Is it due to rounding or simply because\n> the values of yMean, cbMean and crMean\n> \n> \t\tdouble yMean = awb->awb_mean[0].mean_y_or_g;\n> \t\tdouble cbMean = awb->awb_mean[0].mean_cb_or_b;\n> \t\tdouble crMean = awb->awb_mean[0].mean_cr_or_r;\n> \n> \t\t/*\n> \t\t * Convert from YCbCr to RGB.\n> \t\t * The hardware uses the following formulas:\n> \t\t * Y = 16 + 0.2500 R + 0.5000 G + 0.1094 B\n> \t\t * Cb = 128 - 0.1406 R - 0.2969 G + 0.4375 B\n> \t\t * Cr = 128 + 0.4375 R - 0.3750 G - 0.0625 B\n> \t\t *\n> \t\t * The inverse matrix is thus:\n> \t\t * [[1,1636, -0,0623, 1,6008]\n> \t\t * [1,1636, -0,4045, -0,7949]\n> \t\t * [1,1636, 1,9912, -0,0250]]\n> \t\t */\n> \t\tyMean -= 16;\n> \t\tcbMean -= 128;\n> \t\tcrMean -= 128;\n> \n> are smaller than the above offsets ? I guess this can happen when the\n> image is particularly dark, right ?\n\nWhen the image is fully black yMean should be equal to 16 and cbMean and crMean\nto 128. I haven't seen yMean smaller than 16, but cbMean and crMean were\noften 127. This leads to RGB means smaller than 0. The only plausible\ncause I found was hardware rounding errors.\n\n> Anyway, this certainly fixes an issue\n> \n> Reviewed-by: Jacopo Mondi \n> \n> > means may be negative. This would lead to negative gains, messing up\n> > calculation. Prevent this by clamping the means to positive values.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/ipa/rkisp1/algorithms/awb.cpp | 10 ++++++++++\n> > 1 file changed, 10 insertions(+)\n> >\n> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > index ed91e9277a16..de54c4d24650 100644\n> > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > @@ -242,6 +242,16 @@ void Awb::process(IPAContext &context,\n> > \t\tredMean = 1.1636 * yMean - 0.0623 * cbMean + 1.6008 * crMean;\n> > \t\tgreenMean = 1.1636 * yMean - 0.4045 * cbMean - 0.7949 * crMean;\n> > \t\tblueMean = 1.1636 * yMean + 1.9912 * cbMean - 0.0250 * crMean;\n> > +\n> > +\t\t/*\n> > +\t\t * Due to hardware rounding errors in the YCbCr means, the\n> > +\t\t * calculated RGB means may be negative. This would lead to\n> > +\t\t * negative gains, messing up calculation. Prevent this by\n> > +\t\t * clamping the means to positive values.\n> > +\t\t */\n> > +\t\tredMean = std::max(redMean, 0.0);\n> > +\t\tgreenMean = std::max(greenMean, 0.0);\n> > +\t\tblueMean = std::max(blueMean, 0.0);\n> > \t}\n> >\n> > \t/*","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 78C20C0DA4\n\tfor ;\n\tThu, 22 Sep 2022 22:47:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DA51B62223;\n\tFri, 23 Sep 2022 00:47:31 +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 9DCC661F7B\n\tfor ;\n\tFri, 23 Sep 2022 00:47:30 +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 D9BFB4F7;\n\tFri, 23 Sep 2022 00:47:29 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663886851;\n\tbh=j2lA277c7Iv9g4Bz9M2GeWhYB9qJoZ9RAosdO1B2izU=;\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=TTEKSIlwFHbEg3DeOQFmqZHdzfEHaTnpf41sba2/5CGambKyPHn7SNFyljQd0bkpd\n\t8ODO+MRJ6fo2Mbc3uCienZG5giVROv4Fq8fcBLCXd+kiOO4KAiDvT4jE1cbTMa96D+\n\tF73yUBlUAx0o3hXMY7voCKwGZyyVS4gvo0jMedVPjl1fOAFWbwdkzgem4qg2eNrznG\n\t6WqOdSyRMarLZFcP5xYDbWkV1tIOZmwCqZ38+EE4dD4aBtXUMRia7DjKlNull4JE+E\n\tD4P0NoGkBeoZMo3TjoDpJM+dy6kewDq+cUK/VMMCDxeZZOZZyLjmh1cnCg0y37SIgf\n\tGqgWafZXJINGA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663886850;\n\tbh=j2lA277c7Iv9g4Bz9M2GeWhYB9qJoZ9RAosdO1B2izU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GMBqHAZT4NgO4flY+vFJuHJzAsZjWzWM6dA19MjlY8o507TnQ2+9vo4U1Kx2xY+BM\n\tDGEVpGsBbOWbGaSY1yPINKbjZF6Tv0YQ0N2o2XzdNWXDmHySmit5vyD5iUnzHpbNDj\n\tfeHbjgKorIaG2J/5D/UjrPLbipAVouexja4pijhI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"GMBqHAZT\"; dkim-atps=neutral","Date":"Fri, 23 Sep 2022 01:47:15 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220908014200.28728-1-laurent.pinchart@ideasonboard.com>\n\t<20220908014200.28728-30-laurent.pinchart@ideasonboard.com>\n\t<20220922104248.kx6ucisoyzfkcrbf@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220922104248.kx6ucisoyzfkcrbf@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v4 29/32] ipa: rkisp1: awb: Prevent\n\tRGB means from being negative","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\" "}}]