[{"id":19937,"web_url":"https://patchwork.libcamera.org/comment/19937/","msgid":"<20210928161056.7jj72pzihhomrrxt@ideasonboard.com>","date":"2021-09-28T16:10:56","subject":"Re: [libcamera-devel] [PATCH 10/12] ipa: ipu3: awb: Introduce Black\n\tLevel Correction","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On Thu, Sep 23, 2021 at 10:16:23AM +0200, Jean-Michel Hautbois wrote:\n> The pixels output by the camera normally include a black level, because\n> sensors do not always report a signal level of '0' for black. Pixels at\n> or below this level should be considered black and to achieve that, we\n> need to substract an offset to all the pixels. This can be taken into\n> account by reading the lowest value of a special region on sensors which\n> is not exposed to the lens. This provides a substracting factor to be\n> able to adjust the expected black levels in the resultant images.\n> \n> For a camera outputting 10-bit pixel values (in the range 0 to 1023) a\n> typical black level might be 64. It is a fixed value, obtained by\n> capturing a raw frame with minimum exposure and gain fixed to 1.0 while\n> covering the sensor (the darker the better). We consider it good enough\n> as a very first approximation, until we measure it during a tuning\n> process and include it in a configuration file\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/awb.cpp | 8 ++++++++\n> 1 file changed, 8 insertions(+)\n> \n> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> index a5391653..3013870b 100644\n> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> @@ -388,9 +388,17 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)\n> \t/* The CCM matrix may change when color temperature will be used */\n> \tparams->acc_param.ccm = imguCssCcmDefault;\n> \n> +\t/* The Optical Black Level correction values */\n\nI think I'd like to see a clear todo highlighting that these values are\nad-hoc:\n\n\t/*\n\t * \\todo The correction values should come from sensor specific\n\t * tuning processes. This is a first approximation\n\t */\n\n> +\tparams->obgrid_param.gr = 64;\n> +\tparams->obgrid_param.r = 64;\n> +\tparams->obgrid_param.b = 64;\n> +\tparams->obgrid_param.gb = 64;\n> +\n> \tparams->use.acc_awb = 1;\n> \tparams->use.acc_bnr = 1;\n> \tparams->use.acc_ccm = 1;\n\nI'd be tempted by a blank line here to split out the obgrid to make it\nclear that it's a separate component.\n\nWith those:\n\nReviewed-by: Kieran Bingham \n\n> +\tparams->use.obgrid = 1;\n> +\tparams->use.obgrid_param = 1;\n> }\n> \n> } /* namespace ipa::ipu3::algorithms */\n> -- \n> 2.30.2\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 66791BDC71\n\tfor ;\n\tTue, 28 Sep 2021 16:11:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B0BFB69191;\n\tTue, 28 Sep 2021 18:10:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 491BC69188\n\tfor ;\n\tTue, 28 Sep 2021 18:10:58 +0200 (CEST)","from 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 CDA143F1;\n\tTue, 28 Sep 2021 18:10:57 +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=\"kRJS9j6b\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632845457;\n\tbh=2XTDfGe5vT/i5ioEqhcV5qMO/B/jHOvz/p3WNk6algY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kRJS9j6bNAdOqOzMKve/FzVead82uCCywKoYd0m0kpL4mL/YKYzkXwMx4IQO6NvzM\n\tzLXl/GEfRslENre9CrDwtZn8CM0sooOv6ibA6NeUo4HZeA1kIR6R1UqdSKgrvcn8vf\n\tSfGKicJfTl4/Tvv7E968/z3y+sJox5SmT4z5t6bM=","Date":"Tue, 28 Sep 2021 17:10:56 +0100","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ","Message-ID":"<20210928161056.7jj72pzihhomrrxt@ideasonboard.com>","References":"<20210923081625.60276-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210923081625.60276-11-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210923081625.60276-11-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 10/12] ipa: ipu3: awb: Introduce Black\n\tLevel Correction","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19998,"web_url":"https://patchwork.libcamera.org/comment/19998/","msgid":"","date":"2021-09-29T12:51:23","subject":"Re: [libcamera-devel] [PATCH 10/12] ipa: ipu3: awb: Introduce Black\n\tLevel Correction","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Sep 28, 2021 at 05:10:56PM +0100, Kieran Bingham wrote:\n> On Thu, Sep 23, 2021 at 10:16:23AM +0200, Jean-Michel Hautbois wrote:\n> > The pixels output by the camera normally include a black level, because\n> > sensors do not always report a signal level of '0' for black. Pixels at\n> > or below this level should be considered black and to achieve that, we\n> > need to substract an offset to all the pixels. This can be taken into\n> > account by reading the lowest value of a special region on sensors which\n> > is not exposed to the lens. This provides a substracting factor to be\n\ns/the lens/light/\n\n> > able to adjust the expected black levels in the resultant images.\n> > \n> > For a camera outputting 10-bit pixel values (in the range 0 to 1023) a\n> > typical black level might be 64. It is a fixed value, obtained by\n> > capturing a raw frame with minimum exposure and gain fixed to 1.0 while\n> > covering the sensor (the darker the better). We consider it good enough\n> > as a very first approximation, until we measure it during a tuning\n> > process and include it in a configuration file\n> > \n> > Signed-off-by: Jean-Michel Hautbois \n> > ---\n> > src/ipa/ipu3/algorithms/awb.cpp | 8 ++++++++\n> > 1 file changed, 8 insertions(+)\n> > \n> > diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> > index a5391653..3013870b 100644\n> > --- a/src/ipa/ipu3/algorithms/awb.cpp\n> > +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> > @@ -388,9 +388,17 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)\n> > \t/* The CCM matrix may change when color temperature will be used */\n> > \tparams->acc_param.ccm = imguCssCcmDefault;\n> > \n> > +\t/* The Optical Black Level correction values */\n> \n> I think I'd like to see a clear todo highlighting that these values are\n> ad-hoc:\n> \n> \t/*\n> \t * \\todo The correction values should come from sensor specific\n> \t * tuning processes. This is a first approximation\n> \t */\n\nIndeed, this is really a hack, and should be documented as such.\n\n> > +\tparams->obgrid_param.gr = 64;\n> > +\tparams->obgrid_param.r = 64;\n> > +\tparams->obgrid_param.b = 64;\n> > +\tparams->obgrid_param.gb = 64;\n> > +\n> > \tparams->use.acc_awb = 1;\n> > \tparams->use.acc_bnr = 1;\n> > \tparams->use.acc_ccm = 1;\n> \n> I'd be tempted by a blank line here to split out the obgrid to make it\n> clear that it's a separate component.\n\nWhy do we set the optical black level compensation parameters in AWB\nthough ? Shouldn't it be a separate algorithm ?\n\n> With those:\n> \n> Reviewed-by: Kieran Bingham \n> \n> > +\tparams->use.obgrid = 1;\n> > +\tparams->use.obgrid_param = 1;\n> > }\n> > \n> > } /* namespace ipa::ipu3::algorithms */","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 7C375BDC71\n\tfor ;\n\tWed, 29 Sep 2021 12:51:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BF5A1691AA;\n\tWed, 29 Sep 2021 14:51:27 +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 195B769185\n\tfor ;\n\tWed, 29 Sep 2021 14:51:26 +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 7C7D63F0;\n\tWed, 29 Sep 2021 14:51:25 +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=\"HbiNlJ/w\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632919885;\n\tbh=QAv+s37XbmkPI8/bM1uVf4lnOeDVMopypWXACD7rK+k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HbiNlJ/wh3x+s+o7WKBS0KXyJ5OvfW72itnv6+CGylh4vousDxTSdQ8r6bfaSICTe\n\tlp/Ybm683giSdlgqS4sLnyhoxgKq375Gc+lEw8k1+cnu7kvIKhhjrywhPpQL5l3W9e\n\tDjLTeeqUgSbw05+r2kDWV4cJvJJPpP0+qqCXV0Tw=","Date":"Wed, 29 Sep 2021 15:51:23 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210923081625.60276-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210923081625.60276-11-jeanmichel.hautbois@ideasonboard.com>\n\t<20210928161056.7jj72pzihhomrrxt@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210928161056.7jj72pzihhomrrxt@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 10/12] ipa: ipu3: awb: Introduce Black\n\tLevel Correction","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]