[{"id":20364,"web_url":"https://patchwork.libcamera.org/comment/20364/","msgid":"<163484986237.4046931.11124030780055540161@Monstersaurus>","date":"2021-10-21T20:57:42","subject":"Re: [libcamera-devel] [PATCH v3 03/14] ipa: ipu3: awb: Use\n\tsaturation under 90%","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jean-Michel Hautbois (2021-10-21 17:43:50)\n> The AWB grey world algorithm tries to find a grey value and it can't do\n> it on over-exposed images. To exclude those, the saturation ratio is\n> used for each cell, and the cell is included only if this ratio is 0.\n> \n> Now that we have changed the threshold, more cells may be considered as\n> partially saturated and excluded, preventing the algorithm from running\n> efficiently.\n> \n> Change that behaviour, and consider 90% as a good enough ratio.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> Reviewed-by: Laurent Pinchart \n> ---\n> src/ipa/ipu3/algorithms/awb.cpp | 28 ++++++++++++++++++++++++++--\n> 1 file changed, 26 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> index 4364928c..ce01791b 100644\n> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> @@ -19,6 +19,18 @@ LOG_DEFINE_CATEGORY(IPU3Awb)\n> \n> static constexpr uint32_t kMinGreenLevelInZone = 32;\n> \n> +/*\n> + * Minimum proportion of non-saturated cells in a zone for the zone to be used\n> + * by the AWB algorithm.\n> + */\n> +static constexpr double kMinRelevantCellsRatio = 0.8;\n> +\n> +/*\n> + * Maximum ratio of saturated pixels in a cell for the cell to be considered\n> + * non-saturated and counted by the AWB algorithm.\n> + */\n> +static constexpr uint32_t kSaturationThreshold = 255 * 90 / 100;\n> +\n> /**\n> * \\struct Accumulator\n> * \\brief RGB statistics for a given zone\n> @@ -160,7 +172,8 @@ int Awb::configure(IPAContext &context,\n> * for it to be relevant for the grey world algorithm.\n> * \\todo This proportion could be configured.\n> */\n> - cellsPerZoneThreshold_ = cellsPerZoneX_ * cellsPerZoneY_ * 80 / 100;\n> + cellsPerZoneThreshold_ = cellsPerZoneX_ * cellsPerZoneY_ * kMinRelevantCellsRatio;\n> + LOG(IPU3Awb, Debug) << \"Threshold for AWB is set to \" << cellsPerZoneThreshold_;\n> \n> return 0;\n> }\n> @@ -234,7 +247,18 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)\n> reinterpret_cast(\n> &stats->awb_raw_buffer.meta_data[cellPosition]\n> );\n> - if (currentCell->sat_ratio == 0) {\n> +\n> + /*\n> + * Use cells which have less than 90%\n> + * saturation as an initial means to include\n> + * otherwise bright cells which are not fully\n> + * saturated.\n> + *\n> + * \\todo The 90% saturation rate may require\n> + * further empirical measurements and\n> + * optimisation during camera tuning phases.\n> + */\n\nI'm a little wary of the comments stating the value of the constants\nwhich are declared elsewhere, (and the chance of them getting out of\nsync), but I think this is fine for now. If we ever change this, we're\nlikely moving away from a constant, to some tuned/config file value so\nwe'll be forced to update everything anyway.\n\n\nReviewed-by: Kieran Bingham \n\n> + if (currentCell->sat_ratio <= kSaturationThreshold) {\n> /* The cell is not saturated, use the current cell */\n> awbStats_[awbZonePosition].counted++;\n> uint32_t greenValue = currentCell->Gr_avg + currentCell->Gb_avg;\n> -- \n> 2.32.0\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 6F666BF415\n\tfor ;\n\tThu, 21 Oct 2021 20:57:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C79BF68F58;\n\tThu, 21 Oct 2021 22:57:45 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E870D604FE\n\tfor ;\n\tThu, 21 Oct 2021 22:57:44 +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 8697B1B43;\n\tThu, 21 Oct 2021 22:57:44 +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=\"OyFnO2Qn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634849864;\n\tbh=KeuUbB2YLfOtHQCcWGu2iyG5kQhP55ngM+3Fg72mQQg=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=OyFnO2QnpoXFk7meYNR6FVdOWD4wbYGokWGxChjFz3GaHFEat5xeWxfo70we7Rdj4\n\tdZT5KqcjgqF1xBSeIsytSSSkE/O/sc7z/eqkNwONA5XN04hfrPY3cPNq627VK4KFiw\n\tGEopwQVC0GjUtDnaG9d6wf0pWFHw7IN3ErvxzSsc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211021164401.110033-4-jeanmichel.hautbois@ideasonboard.com>","References":"<20211021164401.110033-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211021164401.110033-4-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 21 Oct 2021 21:57:42 +0100","Message-ID":"<163484986237.4046931.11124030780055540161@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v3 03/14] ipa: ipu3: awb: Use\n\tsaturation under 90%","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20369,"web_url":"https://patchwork.libcamera.org/comment/20369/","msgid":"","date":"2021-10-22T03:59:22","subject":"Re: [libcamera-devel] [PATCH v3 03/14] ipa: ipu3: awb: Use\n\tsaturation under 90%","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 Thu, Oct 21, 2021 at 06:43:50PM +0200, Jean-Michel Hautbois wrote:\n> The AWB grey world algorithm tries to find a grey value and it can't do\n> it on over-exposed images. To exclude those, the saturation ratio is\n> used for each cell, and the cell is included only if this ratio is 0.\n> \n> Now that we have changed the threshold, more cells may be considered as\n> partially saturated and excluded, preventing the algorithm from running\n> efficiently.\n> \n> Change that behaviour, and consider 90% as a good enough ratio.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> Reviewed-by: Laurent Pinchart \n> ---\n> src/ipa/ipu3/algorithms/awb.cpp | 28 ++++++++++++++++++++++++++--\n> 1 file changed, 26 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> index 4364928c..ce01791b 100644\n> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> @@ -19,6 +19,18 @@ LOG_DEFINE_CATEGORY(IPU3Awb)\n> \n> static constexpr uint32_t kMinGreenLevelInZone = 32;\n> \n> +/*\n> + * Minimum proportion of non-saturated cells in a zone for the zone to be used\n> + * by the AWB algorithm.\n> + */\n> +static constexpr double kMinRelevantCellsRatio = 0.8;\n> +\n> +/*\n> + * Maximum ratio of saturated pixels in a cell for the cell to be considered\n> + * non-saturated and counted by the AWB algorithm.\n> + */\n> +static constexpr uint32_t kSaturationThreshold = 255 * 90 / 100;\n\nIn addition to the comments, I've also commented on constant names in\nthe review of v2. You're of course free to disagree with review\ncomments, but with no answer to the review, I can't know if you have\nkept the previous names on purpose or just haven't noticed the proposal.\n\n> +\n> /**\n> * \\struct Accumulator\n> * \\brief RGB statistics for a given zone\n> @@ -160,7 +172,8 @@ int Awb::configure(IPAContext &context,\n> \t * for it to be relevant for the grey world algorithm.\n> \t * \\todo This proportion could be configured.\n> \t */\n> -\tcellsPerZoneThreshold_ = cellsPerZoneX_ * cellsPerZoneY_ * 80 / 100;\n> +\tcellsPerZoneThreshold_ = cellsPerZoneX_ * cellsPerZoneY_ * kMinRelevantCellsRatio;\n> +\tLOG(IPU3Awb, Debug) << \"Threshold for AWB is set to \" << cellsPerZoneThreshold_;\n> \n> \treturn 0;\n> }\n> @@ -234,7 +247,18 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)\n> \t\t\t\treinterpret_cast(\n> \t\t\t\t\t&stats->awb_raw_buffer.meta_data[cellPosition]\n> \t\t\t\t);\n> -\t\t\tif (currentCell->sat_ratio == 0) {\n> +\n> +\t\t\t/*\n> +\t\t\t * Use cells which have less than 90%\n> +\t\t\t * saturation as an initial means to include\n> +\t\t\t * otherwise bright cells which are not fully\n> +\t\t\t * saturated.\n> +\t\t\t *\n> +\t\t\t * \\todo The 90% saturation rate may require\n> +\t\t\t * further empirical measurements and\n> +\t\t\t * optimisation during camera tuning phases.\n> +\t\t\t */\n> +\t\t\tif (currentCell->sat_ratio <= kSaturationThreshold) {\n> \t\t\t\t/* The cell is not saturated, use the current cell */\n> \t\t\t\tawbStats_[awbZonePosition].counted++;\n> \t\t\t\tuint32_t greenValue = currentCell->Gr_avg + currentCell->Gb_avg;","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 C55F3BF415\n\tfor ;\n\tFri, 22 Oct 2021 03:59:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DBA2D68F59;\n\tFri, 22 Oct 2021 05:59:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CB5CC60123\n\tfor ;\n\tFri, 22 Oct 2021 05:59:42 +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 43B4F51D;\n\tFri, 22 Oct 2021 05:59:42 +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=\"Sp+O1X9t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634875182;\n\tbh=kMimrQVicUUWqC0Wk/el7EByxhKehf9/1dXHTtbNQT0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Sp+O1X9tkE9kYeyqPPK7tJXUvSbfrfbwqVQYk3D4eXOzZ6hL1YJ3AnzrZo/AdGP0a\n\tOXtL+vSEMNqK9QkHnd0XgIxwx3SXRZn0N+UR1RLbTfN8mvyQjhgTlKi9624o6Lne2q\n\tfFnlajH84yGYW8QC809C6x0VtZisRGfnwdEwHgXM=","Date":"Fri, 22 Oct 2021 06:59:22 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20211021164401.110033-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211021164401.110033-4-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211021164401.110033-4-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 03/14] ipa: ipu3: awb: Use\n\tsaturation under 90%","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":20374,"web_url":"https://patchwork.libcamera.org/comment/20374/","msgid":"<0502638c-55aa-5cc2-088c-68c206ae599e@ideasonboard.com>","date":"2021-10-22T05:29:00","subject":"Re: [libcamera-devel] [PATCH v3 03/14] ipa: ipu3: awb: Use\n\tsaturation under 90%","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 22/10/2021 05:59, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> Thank you for the patch.\n> \n> On Thu, Oct 21, 2021 at 06:43:50PM +0200, Jean-Michel Hautbois wrote:\n>> The AWB grey world algorithm tries to find a grey value and it can't do\n>> it on over-exposed images. To exclude those, the saturation ratio is\n>> used for each cell, and the cell is included only if this ratio is 0.\n>>\n>> Now that we have changed the threshold, more cells may be considered as\n>> partially saturated and excluded, preventing the algorithm from running\n>> efficiently.\n>>\n>> Change that behaviour, and consider 90% as a good enough ratio.\n>>\n>> Signed-off-by: Jean-Michel Hautbois \n>> Reviewed-by: Laurent Pinchart \n>> ---\n>> src/ipa/ipu3/algorithms/awb.cpp | 28 ++++++++++++++++++++++++++--\n>> 1 file changed, 26 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n>> index 4364928c..ce01791b 100644\n>> --- a/src/ipa/ipu3/algorithms/awb.cpp\n>> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n>> @@ -19,6 +19,18 @@ LOG_DEFINE_CATEGORY(IPU3Awb)\n>> \n>> static constexpr uint32_t kMinGreenLevelInZone = 32;\n>> \n>> +/*\n>> + * Minimum proportion of non-saturated cells in a zone for the zone to be used\n>> + * by the AWB algorithm.\n>> + */\n>> +static constexpr double kMinRelevantCellsRatio = 0.8;\n>> +\n>> +/*\n>> + * Maximum ratio of saturated pixels in a cell for the cell to be considered\n>> + * non-saturated and counted by the AWB algorithm.\n>> + */\n>> +static constexpr uint32_t kSaturationThreshold = 255 * 90 / 100;\n> \n> In addition to the comments, I've also commented on constant names in\n> the review of v2. You're of course free to disagree with review\n> comments, but with no answer to the review, I can't know if you have\n> kept the previous names on purpose or just haven't noticed the proposal.\n> \n\nOh, yes, you are right, I got the comments, but did not read your \ncomment properly. Fixed in v4, thanks and sorry :-/.\n\n>> +\n>> /**\n>> * \\struct Accumulator\n>> * \\brief RGB statistics for a given zone\n>> @@ -160,7 +172,8 @@ int Awb::configure(IPAContext &context,\n>> \t * for it to be relevant for the grey world algorithm.\n>> \t * \\todo This proportion could be configured.\n>> \t */\n>> -\tcellsPerZoneThreshold_ = cellsPerZoneX_ * cellsPerZoneY_ * 80 / 100;\n>> +\tcellsPerZoneThreshold_ = cellsPerZoneX_ * cellsPerZoneY_ * kMinRelevantCellsRatio;\n>> +\tLOG(IPU3Awb, Debug) << \"Threshold for AWB is set to \" << cellsPerZoneThreshold_;\n>> \n>> \treturn 0;\n>> }\n>> @@ -234,7 +247,18 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)\n>> \t\t\t\treinterpret_cast(\n>> \t\t\t\t\t&stats->awb_raw_buffer.meta_data[cellPosition]\n>> \t\t\t\t);\n>> -\t\t\tif (currentCell->sat_ratio == 0) {\n>> +\n>> +\t\t\t/*\n>> +\t\t\t * Use cells which have less than 90%\n>> +\t\t\t * saturation as an initial means to include\n>> +\t\t\t * otherwise bright cells which are not fully\n>> +\t\t\t * saturated.\n>> +\t\t\t *\n>> +\t\t\t * \\todo The 90% saturation rate may require\n>> +\t\t\t * further empirical measurements and\n>> +\t\t\t * optimisation during camera tuning phases.\n>> +\t\t\t */\n>> +\t\t\tif (currentCell->sat_ratio <= kSaturationThreshold) {\n>> \t\t\t\t/* The cell is not saturated, use the current cell */\n>> \t\t\t\tawbStats_[awbZonePosition].counted++;\n>> \t\t\t\tuint32_t greenValue = currentCell->Gr_avg + currentCell->Gb_avg;\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 5E54DBDB1C\n\tfor ;\n\tFri, 22 Oct 2021 05:29:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A6DA768F59;\n\tFri, 22 Oct 2021 07:29:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 79DA660123\n\tfor ;\n\tFri, 22 Oct 2021 07:29:03 +0200 (CEST)","from [IPV6:2a01:e0a:169:7140:22cc:3af6:5ccb:8367] (unknown\n\t[IPv6:2a01:e0a:169:7140:22cc:3af6:5ccb:8367])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0C63051D;\n\tFri, 22 Oct 2021 07:29:03 +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=\"RM8b6UOm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634880543;\n\tbh=ErEepTh/K91c2baP0o1hVIJYKi4epKyzVxEMVFgM2cQ=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=RM8b6UOmJAo5RV1O3PQsjCQeU6DrLDOzY0ioEN9lB4BcOujg1HOjd0pe2nqQ6XDNq\n\tFoyMTLu84v8DRCs22RyN43qll5LOq2MaZ55OC3AriDeSqG1NpmvfdzIXyTsSiEebwX\n\t39XkPunpk4OY6Htl+vS90LD63f3wWyY7u9Gn83wA=","Message-ID":"<0502638c-55aa-5cc2-088c-68c206ae599e@ideasonboard.com>","Date":"Fri, 22 Oct 2021 07:29:00 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.1.2","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20211021164401.110033-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211021164401.110033-4-jeanmichel.hautbois@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 03/14] ipa: ipu3: awb: Use\n\tsaturation under 90%","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\" "}}]