[{"id":19286,"web_url":"https://patchwork.libcamera.org/comment/19286/","msgid":"<7b854b6b-d1ea-568c-34c6-a04933796758@ideasonboard.com>","date":"2021-09-02T12:27:39","subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: ipu3: Rename IspStatsRegion\n\tto Accumulator","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 02/09/2021 09:32, Jean-Michel Hautbois wrote:\n> The structure name is not very explicit, rename it to accumulator, as it\n> is accumulating the pixels in a given region, and amend the documentation.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/awb.cpp | 34 ++++++++++++++++++++-------------\n> src/ipa/ipu3/algorithms/awb.h | 4 ++--\n> 2 files changed, 23 insertions(+), 15 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> index e05647c9..8cfbb23d 100644\n> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> @@ -21,26 +21,34 @@ static constexpr uint32_t kMinZonesCounted = 16;\n> static constexpr uint32_t kMinGreenLevelInZone = 32;\n> \n> /**\n> - * \\struct IspStatsRegion\n> + * \\struct Accumulator\n> * \\brief RGB statistics for a given region\n> *\n> - * The IspStatsRegion structure is intended to abstract the ISP specific\n> - * statistics and use an agnostic algorithm to compute AWB.\n> + * The Accumulator structure stores the sum of the pixel values in a zone of\n> + * the image, as well as the number of relevant regions in this same zone. A\n> + * relevant region is an unsaturated region here, depending on the awb\n> + * thresholds.\n\n\nI'm a bit confused with terminology here.\n\n\n> *\n> - * \\var IspStatsRegion::counted\n> - * \\brief Number of pixels used to calculate the sums\n> + * A frame is divided into zones, each zone is made of multiple regions which\n\nMaybe this should be at the beginning so the terms are defined before\nthey are used? That might have solved my confusion above on my first read.\n\n\n> + * are defined by the grid configuration. The algorithm works with 16x12 zones.\n\nIt might not be wise to hardcode those zone sizes which are parameters\nof the algorithm in the documentation of the structure.\n\nWhat happens if they change, then this documentation needs to be updated\ntoo.\n\n\n> + * For example, a frame of 1296x720 is divided into 81x45 regions of [16x16]\n> + * pixels. And the zones are made of [5x4] regions.\n\nThough examples are helpful.\n\nBut, 5*16*16=1280, what happens to the other 16 horizontal pixels?\nAre the regions / zones left aligned, centered? algorithm specific?\n\nWould it help perhaps to have a dedicated documentation of the 'grid'\nwhich is what I assume these zones, regions, pixels are all defined by,\n... then the documentation for the Accumulator can focus on what it is\ndefining.\n\n\n> + * \\todo extend the notion of relevant to something else ?\n> *\n> - * \\var IspStatsRegion::uncounted\n> - * \\brief Remaining number of pixels in the region\n> + * \\var Accumulator::counted\n> + * \\brief Number of relevant regions used to calculate the sums\n> *\n> - * \\var IspStatsRegion::rSum\n> - * \\brief Sum of the red values in the region\n> + * \\var Accumulator::uncounted\n> + * \\brief Remaining number of regions in the zone\n> *\n> - * \\var IspStatsRegion::gSum\n> - * \\brief Sum of the green values in the region\n> + * \\var Accumulator::rSum\n> + * \\brief Sum of the red values in the zone\n> *\n> - * \\var IspStatsRegion::bSum\n> - * \\brief Sum of the blue values in the region\n> + * \\var Accumulator::gSum\n> + * \\brief Sum of the green values in the zone\n> + *\n> + * \\var Accumulator::bSum\n> + * \\brief Sum of the blue values in the zone\n> */\n> \n> /**\n> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h\n> index cc848060..6ae934fc 100644\n> --- a/src/ipa/ipu3/algorithms/awb.h\n> +++ b/src/ipa/ipu3/algorithms/awb.h\n> @@ -33,7 +33,7 @@ struct Ipu3AwbCell {\n> \tunsigned char padding[3];\n> } __attribute__((packed));\n> \n> -struct IspStatsRegion {\n> +struct Accumulator {\n> \tunsigned int counted;\n> \tunsigned int uncounted;\n> \tunsigned long long rSum;\n> @@ -82,7 +82,7 @@ private:\n> \tuint32_t estimateCCT(double red, double green, double blue);\n> \n> \tstd::vector zones_;\n> -\tIspStatsRegion awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];\n> +\tAccumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];\n> \tAwbStatus asyncResults_;\n> };\n> \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 A1321BDC71\n\tfor ;\n\tThu, 2 Sep 2021 12:27:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1CBD469166;\n\tThu, 2 Sep 2021 14:27: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 281EA60255\n\tfor ;\n\tThu, 2 Sep 2021 14:27:43 +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 A40E7317;\n\tThu, 2 Sep 2021 14:27:42 +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=\"sxPpcuDd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630585662;\n\tbh=m6sewkPT6zpfcO4hK16+ly8k8Dk3Va52ySMIIyIvMfA=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=sxPpcuDdlFq8UABRweTWV1UuatyW0KaUyG3Po1xpIlL9E8vP5ABYzRGRGrizoe4t8\n\tS9YG6JEzKsU+JDVEeXonOl38qMxIA9cRDnTZv1qHwuvrXQddb1Az28jRgc0A0RPWmI\n\tj4HxL/FZEKQE2thkuzMckIkYl/A8nZ7amtA7uRSw=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210902083207.33976-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210902083207.33976-3-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"<7b854b6b-d1ea-568c-34c6-a04933796758@ideasonboard.com>","Date":"Thu, 2 Sep 2021 13:27:39 +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":"<20210902083207.33976-3-jeanmichel.hautbois@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: ipu3: Rename IspStatsRegion\n\tto Accumulator","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":19287,"web_url":"https://patchwork.libcamera.org/comment/19287/","msgid":"<9b2aa24e-7f72-4028-3a14-4ad46ad2e17e@ideasonboard.com>","date":"2021-09-02T12:33:22","subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: ipu3: Replace\n\tAccumulator::uncounted to Accumulator::total","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 02/09/2021 09:32, Jean-Michel Hautbois wrote:\n> Replace uncounted by total as it is clearer on what it contains. It is\n> always possible to know how many pixels are not relevant for the\n> algorithm by calculating total-counted.\n\nWas uncounted not set before? I expected to see code removing references\nto setting the uncounted value.\n\nIf it was previously unused (other than initialisation) add that\ninformation to the commit message.\n\n\n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/awb.cpp | 10 +++++++---\n> src/ipa/ipu3/algorithms/awb.h | 2 +-\n> 2 files changed, 8 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> index 8cfbb23d..9b2cbba7 100644\n> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> @@ -38,8 +38,8 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;\n> * \\var Accumulator::counted\n> * \\brief Number of relevant regions used to calculate the sums\n> *\n> - * \\var Accumulator::uncounted\n> - * \\brief Remaining number of regions in the zone\n> + * \\var Accumulator::total\n> + * \\brief Total number of regions in the zone\n> *\n> * \\var Accumulator::rSum\n> * \\brief Sum of the red values in the zone\n> @@ -189,6 +189,10 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,\n> \tuint32_t regionWidth = round(grid.width / static_cast(kAwbStatsSizeX));\n> \tuint32_t regionHeight = round(grid.height / static_cast(kAwbStatsSizeY));\n> \n> +\tfor (unsigned int i = 0; i < kAwbStatsSizeX; i++)\n> +\t\tfor (unsigned int j = 0; j < kAwbStatsSizeY; j++)\n> +\t\t\tawbStats_[j * kAwbStatsSizeX + i].total = regionWidth * regionHeight;\n\nIf all (zones?) have the same total number of regions, do we even need\nto set it in the structure?\n\nPerhaps if uncounted/total are unused, it could simply be dropped?\n\n> +\n> \t/*\n> \t * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is\n> \t * (grid.width x grid.height).\n> @@ -223,7 +227,7 @@ void Awb::clearAwbStats()\n> \t\tawbStats_[i].rSum = 0;\n> \t\tawbStats_[i].gSum = 0;\n> \t\tawbStats_[i].counted = 0;\n> -\t\tawbStats_[i].uncounted = 0;\n> +\t\tawbStats_[i].total = 0;\n> \t}\n> }\n> \n> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h\n> index 6ae934fc..13490b07 100644\n> --- a/src/ipa/ipu3/algorithms/awb.h\n> +++ b/src/ipa/ipu3/algorithms/awb.h\n> @@ -35,7 +35,7 @@ struct Ipu3AwbCell {\n> \n> struct Accumulator {\n> \tunsigned int counted;\n> -\tunsigned int uncounted;\n> +\tunsigned int total;\n> \tunsigned long long rSum;\n> \tunsigned long long gSum;\n> \tunsigned long long bSum;\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 A3C80BD87D\n\tfor ;\n\tThu, 2 Sep 2021 12:33:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EB4C76916E;\n\tThu, 2 Sep 2021 14:33:26 +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 B45C360255\n\tfor ;\n\tThu, 2 Sep 2021 14:33:25 +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 02E4F317;\n\tThu, 2 Sep 2021 14:33:24 +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=\"I5Ud7XCA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630586005;\n\tbh=MSwl2NVBDeNVWrKGB5yNiR8YsOF5bpcapY8iMIiQigc=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=I5Ud7XCApy1/tUN8SmN+E2TBEvK+8I2s88BZ1GwqJM5hz4zrD+7UkCDaM9nQMdPEm\n\tVS9j6j1APGuUvdp2wGSb4inulol7R4OLQ0f5Qpgxuo8LXO8bTlZxb/aRWrY914/I/7\n\tJI+fmwAYiGd6Yv6Wb/z0dw7Y9UIMC4/ZUBP+s6FU=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210902083207.33976-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210902083207.33976-4-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"<9b2aa24e-7f72-4028-3a14-4ad46ad2e17e@ideasonboard.com>","Date":"Thu, 2 Sep 2021 13:33:22 +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":"<20210902083207.33976-4-jeanmichel.hautbois@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: ipu3: Replace\n\tAccumulator::uncounted to Accumulator::total","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":19289,"web_url":"https://patchwork.libcamera.org/comment/19289/","msgid":"<078bfec1-eb18-114a-d1d3-5935b842604a@ideasonboard.com>","date":"2021-09-02T12:52:04","subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: ipu3: Replace\n\tAccumulator::uncounted to Accumulator::total","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"On 02/09/2021 14:33, Kieran Bingham wrote:\n> On 02/09/2021 09:32, Jean-Michel Hautbois wrote:\n>> Replace uncounted by total as it is clearer on what it contains. It is\n>> always possible to know how many pixels are not relevant for the\n>> algorithm by calculating total-counted.\n> \n> Was uncounted not set before? I expected to see code removing references\n> to setting the uncounted value.\n> \n> If it was previously unused (other than initialisation) add that\n> information to the commit message.\n> \n\nIt was not used and won't be used in a near future either :-).\n\n> \n>> Signed-off-by: Jean-Michel Hautbois \n>> ---\n>> src/ipa/ipu3/algorithms/awb.cpp | 10 +++++++---\n>> src/ipa/ipu3/algorithms/awb.h | 2 +-\n>> 2 files changed, 8 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n>> index 8cfbb23d..9b2cbba7 100644\n>> --- a/src/ipa/ipu3/algorithms/awb.cpp\n>> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n>> @@ -38,8 +38,8 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;\n>> * \\var Accumulator::counted\n>> * \\brief Number of relevant regions used to calculate the sums\n>> *\n>> - * \\var Accumulator::uncounted\n>> - * \\brief Remaining number of regions in the zone\n>> + * \\var Accumulator::total\n>> + * \\brief Total number of regions in the zone\n>> *\n>> * \\var Accumulator::rSum\n>> * \\brief Sum of the red values in the zone\n>> @@ -189,6 +189,10 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,\n>> \tuint32_t regionWidth = round(grid.width / static_cast(kAwbStatsSizeX));\n>> \tuint32_t regionHeight = round(grid.height / static_cast(kAwbStatsSizeY));\n>> \n>> +\tfor (unsigned int i = 0; i < kAwbStatsSizeX; i++)\n>> +\t\tfor (unsigned int j = 0; j < kAwbStatsSizeY; j++)\n>> +\t\t\tawbStats_[j * kAwbStatsSizeX + i].total = regionWidth * regionHeight;\n> \n> If all (zones?) have the same total number of regions, do we even need\n> to set it in the structure?\n> \n> Perhaps if uncounted/total are unused, it could simply be dropped?\n> \n>> +\n>> \t/*\n>> \t * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is\n>> \t * (grid.width x grid.height).\n>> @@ -223,7 +227,7 @@ void Awb::clearAwbStats()\n>> \t\tawbStats_[i].rSum = 0;\n>> \t\tawbStats_[i].gSum = 0;\n>> \t\tawbStats_[i].counted = 0;\n>> -\t\tawbStats_[i].uncounted = 0;\n>> +\t\tawbStats_[i].total = 0;\n>> \t}\n>> }\n>> \n>> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h\n>> index 6ae934fc..13490b07 100644\n>> --- a/src/ipa/ipu3/algorithms/awb.h\n>> +++ b/src/ipa/ipu3/algorithms/awb.h\n>> @@ -35,7 +35,7 @@ struct Ipu3AwbCell {\n>> \n>> struct Accumulator {\n>> \tunsigned int counted;\n>> -\tunsigned int uncounted;\n>> +\tunsigned int total;\n>> \tunsigned long long rSum;\n>> \tunsigned long long gSum;\n>> \tunsigned long long bSum;\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 56FD6BD87D\n\tfor ;\n\tThu, 2 Sep 2021 12:52:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AD0AC6916A;\n\tThu, 2 Sep 2021 14:52:08 +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 3871660255\n\tfor ;\n\tThu, 2 Sep 2021 14:52:07 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:43b0:6633:e0bd:5dc6])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BF578317;\n\tThu, 2 Sep 2021 14:52:06 +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=\"YiTss/dj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630587126;\n\tbh=us2e5WO05GDG9U93+qHIrWlQ0A7g6zYQew3mrXqMMP4=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=YiTss/dj7KQNKGee7ZIJXEdA+yYRcpa7Tq7DJ1WDAZvjRvR+5XUTE+SxAfGt/Gx1r\n\tJOiXSbUo9XPW/ztOl1GhSSvquRwaOVUbDOs6xyb+RHsuRGp89Y8SCPq9uv9GkNjjGB\n\tkQDJGEshL6d9DO7krQ79D5p/eHxh44t4VkGN/5Qc=","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210902083207.33976-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210902083207.33976-4-jeanmichel.hautbois@ideasonboard.com>\n\t<9b2aa24e-7f72-4028-3a14-4ad46ad2e17e@ideasonboard.com>","From":"Jean-Michel Hautbois ","Message-ID":"<078bfec1-eb18-114a-d1d3-5935b842604a@ideasonboard.com>","Date":"Thu, 2 Sep 2021 14:52:04 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.13.0","MIME-Version":"1.0","In-Reply-To":"<9b2aa24e-7f72-4028-3a14-4ad46ad2e17e@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: ipu3: Replace\n\tAccumulator::uncounted to Accumulator::total","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":19291,"web_url":"https://patchwork.libcamera.org/comment/19291/","msgid":"<517abba4-d325-445a-1dcd-5cccf4f6ca43@ideasonboard.com>","date":"2021-09-02T12:59:09","subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: ipu3: Replace\n\tAccumulator::uncounted to Accumulator::total","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 02/09/2021 13:52, Jean-Michel Hautbois wrote:\n> On 02/09/2021 14:33, Kieran Bingham wrote:\n>> On 02/09/2021 09:32, Jean-Michel Hautbois wrote:\n>>> Replace uncounted by total as it is clearer on what it contains. It is\n>>> always possible to know how many pixels are not relevant for the\n>>> algorithm by calculating total-counted.\n>>\n>> Was uncounted not set before? I expected to see code removing references\n>> to setting the uncounted value.\n>>\n>> If it was previously unused (other than initialisation) add that\n>> information to the commit message.\n>>\n> \n> It was not used and won't be used in a near future either :-).\n\nOk - maybe lets remove it instead then.\n\n\n>>> Signed-off-by: Jean-Michel Hautbois \n>>> ---\n>>> src/ipa/ipu3/algorithms/awb.cpp | 10 +++++++---\n>>> src/ipa/ipu3/algorithms/awb.h | 2 +-\n>>> 2 files changed, 8 insertions(+), 4 deletions(-)\n>>>\n>>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n>>> index 8cfbb23d..9b2cbba7 100644\n>>> --- a/src/ipa/ipu3/algorithms/awb.cpp\n>>> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n>>> @@ -38,8 +38,8 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;\n>>> * \\var Accumulator::counted\n>>> * \\brief Number of relevant regions used to calculate the sums\n>>> *\n>>> - * \\var Accumulator::uncounted\n>>> - * \\brief Remaining number of regions in the zone\n>>> + * \\var Accumulator::total\n>>> + * \\brief Total number of regions in the zone\n>>> *\n>>> * \\var Accumulator::rSum\n>>> * \\brief Sum of the red values in the zone\n>>> @@ -189,6 +189,10 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,\n>>> \tuint32_t regionWidth = round(grid.width / static_cast(kAwbStatsSizeX));\n>>> \tuint32_t regionHeight = round(grid.height / static_cast(kAwbStatsSizeY));\n>>> \n>>> +\tfor (unsigned int i = 0; i < kAwbStatsSizeX; i++)\n>>> +\t\tfor (unsigned int j = 0; j < kAwbStatsSizeY; j++)\n>>> +\t\t\tawbStats_[j * kAwbStatsSizeX + i].total = regionWidth * regionHeight;\n>>\n>> If all (zones?) have the same total number of regions, do we even need\n>> to set it in the structure?\n>>\n>> Perhaps if uncounted/total are unused, it could simply be dropped?\n>>\n>>> +\n>>> \t/*\n>>> \t * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is\n>>> \t * (grid.width x grid.height).\n>>> @@ -223,7 +227,7 @@ void Awb::clearAwbStats()\n>>> \t\tawbStats_[i].rSum = 0;\n>>> \t\tawbStats_[i].gSum = 0;\n>>> \t\tawbStats_[i].counted = 0;\n>>> -\t\tawbStats_[i].uncounted = 0;\n>>> +\t\tawbStats_[i].total = 0;\n>>> \t}\n>>> }\n>>> \n>>> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h\n>>> index 6ae934fc..13490b07 100644\n>>> --- a/src/ipa/ipu3/algorithms/awb.h\n>>> +++ b/src/ipa/ipu3/algorithms/awb.h\n>>> @@ -35,7 +35,7 @@ struct Ipu3AwbCell {\n>>> \n>>> struct Accumulator {\n>>> \tunsigned int counted;\n>>> -\tunsigned int uncounted;\n>>> +\tunsigned int total;\n>>> \tunsigned long long rSum;\n>>> \tunsigned long long gSum;\n>>> \tunsigned long long bSum;\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 96648BD87D\n\tfor ;\n\tThu, 2 Sep 2021 12:59:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 16FC86916A;\n\tThu, 2 Sep 2021 14:59:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EAC7D60255\n\tfor ;\n\tThu, 2 Sep 2021 14:59:12 +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 7B9A545E;\n\tThu, 2 Sep 2021 14:59:12 +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=\"Tg9oTPtS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630587552;\n\tbh=GOEZghelNnbz7E+4diz8dSigx6K3SztnBoYupDf+nGk=;\n\th=From:Subject:To:References:Date:In-Reply-To:From;\n\tb=Tg9oTPtShPUG8OmgBIxoLntB8uWH1W0N1k52jSTPIV3+0FtwxebQzlXY3vX2eK6H9\n\t3qbMSZnH/vdIAn/e52tZ6Rvp0VUs5ze25ewzaJLJlb02BSR9Q6KdRp1xVgDPgHLyPv\n\tm/V/4q9Fv+I8Tk8g41LYwsgb/mW+hpo9Hp1yIMio=","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210902083207.33976-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210902083207.33976-4-jeanmichel.hautbois@ideasonboard.com>\n\t<9b2aa24e-7f72-4028-3a14-4ad46ad2e17e@ideasonboard.com>\n\t<078bfec1-eb18-114a-d1d3-5935b842604a@ideasonboard.com>","Message-ID":"<517abba4-d325-445a-1dcd-5cccf4f6ca43@ideasonboard.com>","Date":"Thu, 2 Sep 2021 13:59:09 +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":"<078bfec1-eb18-114a-d1d3-5935b842604a@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: ipu3: Replace\n\tAccumulator::uncounted to Accumulator::total","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\" "}}]