[{"id":19644,"web_url":"https://patchwork.libcamera.org/comment/19644/","msgid":"","date":"2021-09-13T15:56:11","subject":"Re: [libcamera-devel] [PATCH v7 2/5] ipa: ipu3: Rename\n\tIspStatsRegion to Accumulator","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 Mon, Sep 13, 2021 at 03:28:00PM +0200, Jean-Michel Hautbois wrote:\n> The IspStatsRegion structure was introduced as an attempt to prepare for\n> a generic AWB algorithm structure. The structure name by itself is not\n> explicit and it is too optimistic to try and make a generic one for now.\n> \n> Its role is to accumulate the pixels in a given zone. Rename it to\n> accumulator, and remove the uncounted field at the same time. It is\n> always possible to know how many pixels are not relevant for the\n> algorithm by calculating total-counted. The uncounted field was only\n> declared and not used. Amend the documentation accordingly.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> Reviewed-by: Kieran Bingham \n> Reviewed-by: Laurent Pinchart \n> ---\n> src/ipa/ipu3/algorithms/awb.cpp | 80 +++++++++++++++++++++++++--------\n> src/ipa/ipu3/algorithms/awb.h | 5 +--\n> 2 files changed, 64 insertions(+), 21 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> index e05647c9..d993e21e 100644\n> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> @@ -21,26 +21,71 @@ static constexpr uint32_t kMinZonesCounted = 16;\n> static constexpr uint32_t kMinGreenLevelInZone = 32;\n> \n> /**\n> - * \\struct IspStatsRegion\n> - * \\brief RGB statistics for a given region\n> + * \\struct Accumulator\n> + * \\brief RGB statistics for a given zone\n> *\n> - * The IspStatsRegion structure is intended to abstract the ISP specific\n> - * statistics and use an agnostic algorithm to compute AWB.\n> + * - Cells are defined in Pixels\n> + * - Zones are defined in Cells\n> + * - The total zones may overlap the image width and height to meet hardware\n> + * constraints\n\nQuoting your reply from v6:\n\n\"[This] means that the zones will be adapted to the BDS output size\nwhich may be greater than the rendered frame (asking for 1280x720 may\nlead to a 1296x720 BDS output and you will have 81 cells of 16 pixels\nwidth and not 80).\"\n\nFrom a statistics point of view, the zones match the output of the BDS.\nThe image is then slightly cropped to achieve the required resolution. I\nthink that's out of scope here. I would thus drop the last point. The\nimage and text below need to be adapted to replace 1280 with 1296.\n\n> *\n> - * \\var IspStatsRegion::counted\n> - * \\brief Number of pixels used to calculate the sums\n> + * 81 cells\n> + * /───────────── 1280 pixels ───────────\\\n> + * 16 zones\n> + * 16\n> + * ┌────┬────┬────┬────┬────┬─ ──────┬────┐ \\\n> + * │Cell│ │ │ │ │ | │ │ │\n> + * 16 │ px │ │ │ │ │ | │ │ │\n> + * ├────┼────┼────┼────┼────┼─ ──────┼────┤ │\n> + * │ │ │ │ │ │ | │ │\n> + * │ │ │ │ │ │ | │ │ 7\n> + * │ ── │ ── │ ── │ ── │ ── │ ── ── ─┤ ── │ 1 2 4\n> + * │ │ │ │ │ │ | │ │ 2 0 5\n> *\n> - * \\var IspStatsRegion::uncounted\n> - * \\brief Remaining number of pixels in the region\n> + * │ │ │ │ │ │ | │ │ z p c\n> + * ├────┼────┼────┼────┼────┼─ ──────┼────┤ o i e\n> + * │ │ │ │ │ │ | │ │ n x l\n> + * │ │ | │ │ e e l\n> + * ├─── ───┼─ ──────┼────┤ s l s\n> + * │ │ | │ │ s\n> + * │ │ | │ │\n> + * ├─── Zone of Cells ───┼─ ──────┼────┤ │\n> + * │ (5 x 4) │ | │ │ │\n> + * │ │ | │ │ │\n> + * ├── ───┼─ ──────┼────┤ │\n> + * │ │ │ | │ │ │\n> + * │ │ │ │ │ │ | │ │ │\n> + * └────┴────┴────┴────┴────┴─ ──────┴────┘ /\n> *\n> - * \\var IspStatsRegion::rSum\n> - * \\brief Sum of the red values in the region\n> + * The algorithm works with a fixed number of zones \\a kAwbStatsSizeX x\n> + * \\a kAwbStatsSizeY. For example, a frame of 1280x720 is divided into 81x45\n> + * cells of [16x16] pixels with a BDS output size of 1296x720 to account for the\n> + * 16x16 pixel alignment restrictions. In the case of \\a kAwbStatsSizeX=16 and\n> + * \\a kAwbStatsSizeY=12 the zones are made of [5x4] cells. The cells are\n> + * left-aligned and calculated by IPAIPU3::calculateBdsGrid().\n> *\n> - * \\var IspStatsRegion::gSum\n> - * \\brief Sum of the green values in the region\n> + * Each statistics cell represents the average value of the pixels in that cell\n> + * split by colour components.\n> *\n> - * \\var IspStatsRegion::bSum\n> - * \\brief Sum of the blue values in the region\n> + * The Accumulator structure stores the sum of the average of each cell in a\n> + * zone of the image, as well as the number of cells which were unsaturated and\n> + * therefore included in the average.\n> + * \\todo move this description and structure into a common header\n> + *\n> + * Cells which are saturated beyond the threshold defined in\n> + * ipu3_uapi_awb_config_s are not included in the average.\n> + *\n> + * \\var Accumulator::counted\n> + * \\brief Number of unsaturated cells used to calculate the sums\n> + *\n> + * \\var Accumulator::rSum\n> + * \\brief Sum of the average red values of each unsaturated cell in the zone\n> + *\n> + * \\var Accumulator::gSum\n> + * \\brief Sum of the average green values of each unsaturated cell in the zone\n> + *\n> + * \\var Accumulator::bSum\n> + * \\brief Sum of the average blue values of each unsaturated cell in the zone\n> */\n> \n> /**\n> @@ -157,7 +202,7 @@ uint32_t Awb::estimateCCT(double red, double green, double blue)\n> \treturn 449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33;\n> }\n> \n> -/* Generate an RGB vector with the average values for each region */\n> +/* Generate an RGB vector with the average values for each zone */\n> void Awb::generateZones(std::vector &zones)\n> {\n> \tfor (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {\n> @@ -174,7 +219,7 @@ void Awb::generateZones(std::vector &zones)\n> \t}\n> }\n> \n> -/* Translate the IPU3 statistics into the default statistics region array */\n> +/* Translate the IPU3 statistics into the default statistics zone array */\n> void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,\n> \t\t\t const ipu3_uapi_grid_config &grid)\n> {\n> @@ -215,7 +260,6 @@ 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}\n> }\n> \n> @@ -304,7 +348,7 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)\n> \n> \t/*\n> \t * Optical center is column start (respectively row start) of the\n> -\t * region of interest minus its X center (respectively Y center).\n> +\t * cell of interest minus its X center (respectively Y center).\n> \t *\n> \t * For the moment use BDS as a first approximation, but it should\n> \t * be calculated based on Shading (SHD) parameters.\n> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h\n> index cc848060..ac8ccc84 100644\n> --- a/src/ipa/ipu3/algorithms/awb.h\n> +++ b/src/ipa/ipu3/algorithms/awb.h\n> @@ -33,9 +33,8 @@ 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> \tunsigned long long gSum;\n> \tunsigned long long bSum;\n> @@ -82,7 +81,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>","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 9386EBDB1D\n\tfor ;\n\tMon, 13 Sep 2021 15:56:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D116769182;\n\tMon, 13 Sep 2021 17:56:37 +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 84FAF6024C\n\tfor ;\n\tMon, 13 Sep 2021 17:56:36 +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 DF989499;\n\tMon, 13 Sep 2021 17:56:35 +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=\"wh8WgyGK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631548596;\n\tbh=kF9CQ6aUpG4C5D/fgVd2/1VzLmEgCeSwhRGrHo2JCUE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wh8WgyGKZXnVswaUPONyjeSVOwV+swq1Y3cYuhk+pG9+z9ll+VhVyHQzSvYd7PyS7\n\ts2y14DkkPYLxN7ivFVWitwFg5RkqYtFNjMwMbqvGOxdEbpnFfBrJecxFMuc3PicByQ\n\t6SFZvbZ9iwDiZ3hlJ4Iu9CMk/5nmO/zHXvx0OeG8=","Date":"Mon, 13 Sep 2021 18:56:11 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210913132803.54881-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210913132803.54881-3-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210913132803.54881-3-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v7 2/5] ipa: ipu3: Rename\n\tIspStatsRegion to 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19646,"web_url":"https://patchwork.libcamera.org/comment/19646/","msgid":"<6c0aed89-a71a-2afe-e011-28dbe0ea5e4c@ideasonboard.com>","date":"2021-09-13T16:45:21","subject":"Re: [libcamera-devel] [PATCH v7 2/5] ipa: ipu3: Rename\n\tIspStatsRegion to Accumulator","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 13/09/2021 17:56, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> Thank you for the patch.\n> \n> On Mon, Sep 13, 2021 at 03:28:00PM +0200, Jean-Michel Hautbois wrote:\n>> The IspStatsRegion structure was introduced as an attempt to prepare for\n>> a generic AWB algorithm structure. The structure name by itself is not\n>> explicit and it is too optimistic to try and make a generic one for now.\n>>\n>> Its role is to accumulate the pixels in a given zone. Rename it to\n>> accumulator, and remove the uncounted field at the same time. It is\n>> always possible to know how many pixels are not relevant for the\n>> algorithm by calculating total-counted. The uncounted field was only\n>> declared and not used. Amend the documentation accordingly.\n>>\n>> Signed-off-by: Jean-Michel Hautbois \n>> Reviewed-by: Kieran Bingham \n>> Reviewed-by: Laurent Pinchart \n>> ---\n>> src/ipa/ipu3/algorithms/awb.cpp | 80 +++++++++++++++++++++++++--------\n>> src/ipa/ipu3/algorithms/awb.h | 5 +--\n>> 2 files changed, 64 insertions(+), 21 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n>> index e05647c9..d993e21e 100644\n>> --- a/src/ipa/ipu3/algorithms/awb.cpp\n>> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n>> @@ -21,26 +21,71 @@ static constexpr uint32_t kMinZonesCounted = 16;\n>> static constexpr uint32_t kMinGreenLevelInZone = 32;\n>> \n>> /**\n>> - * \\struct IspStatsRegion\n>> - * \\brief RGB statistics for a given region\n>> + * \\struct Accumulator\n>> + * \\brief RGB statistics for a given zone\n>> *\n>> - * The IspStatsRegion structure is intended to abstract the ISP specific\n>> - * statistics and use an agnostic algorithm to compute AWB.\n>> + * - Cells are defined in Pixels\n>> + * - Zones are defined in Cells\n>> + * - The total zones may overlap the image width and height to meet hardware\n>> + * constraints\n> \n> Quoting your reply from v6:\n> \n> \"[This] means that the zones will be adapted to the BDS output size\n> which may be greater than the rendered frame (asking for 1280x720 may\n> lead to a 1296x720 BDS output and you will have 81 cells of 16 pixels\n> width and not 80).\"\n> \n> From a statistics point of view, the zones match the output of the BDS.\n> The image is then slightly cropped to achieve the required resolution. I\n> think that's out of scope here. I would thus drop the last point. The\n> image and text below need to be adapted to replace 1280 with 1296.\n> \n>> *\n>> - * \\var IspStatsRegion::counted\n>> - * \\brief Number of pixels used to calculate the sums\n>> + * 81 cells\n>> + * /───────────── 1280 pixels ───────────\\\n>> + * 16 zones\n>> + * 16\n>> + * ┌────┬────┬────┬────┬────┬─ ──────┬────┐ \\\n>> + * │Cell│ │ │ │ │ | │ │ │\n>> + * 16 │ px │ │ │ │ │ | │ │ │\n>> + * ├────┼────┼────┼────┼────┼─ ──────┼────┤ │\n>> + * │ │ │ │ │ │ | │ │\n>> + * │ │ │ │ │ │ | │ │ 7\n>> + * │ ── │ ── │ ── │ ── │ ── │ ── ── ─┤ ── │ 1 2 4\n>> + * │ │ │ │ │ │ | │ │ 2 0 5\n>> *\n>> - * \\var IspStatsRegion::uncounted\n>> - * \\brief Remaining number of pixels in the region\n>> + * │ │ │ │ │ │ | │ │ z p c\n>> + * ├────┼────┼────┼────┼────┼─ ──────┼────┤ o i e\n>> + * │ │ │ │ │ │ | │ │ n x l\n>> + * │ │ | │ │ e e l\n>> + * ├─── ───┼─ ──────┼────┤ s l s\n>> + * │ │ | │ │ s\n>> + * │ │ | │ │\n>> + * ├─── Zone of Cells ───┼─ ──────┼────┤ │\n>> + * │ (5 x 4) │ | │ │ │\n>> + * │ │ | │ │ │\n>> + * ├── ───┼─ ──────┼────┤ │\n>> + * │ │ │ | │ │ │\n>> + * │ │ │ │ │ │ | │ │ │\n>> + * └────┴────┴────┴────┴────┴─ ──────┴────┘ /\n>> *\n>> - * \\var IspStatsRegion::rSum\n>> - * \\brief Sum of the red values in the region\n>> + * The algorithm works with a fixed number of zones \\a kAwbStatsSizeX x\n>> + * \\a kAwbStatsSizeY. For example, a frame of 1280x720 is divided into 81x45\n>> + * cells of [16x16] pixels with a BDS output size of 1296x720 to account for the\n\nReplacing 1280 by 1296 except this sentence, right :-) ?\n\n>> + * 16x16 pixel alignment restrictions. In the case of \\a kAwbStatsSizeX=16 and\n>> + * \\a kAwbStatsSizeY=12 the zones are made of [5x4] cells. The cells are\n>> + * left-aligned and calculated by IPAIPU3::calculateBdsGrid().\n>> *\n>> - * \\var IspStatsRegion::gSum\n>> - * \\brief Sum of the green values in the region\n>> + * Each statistics cell represents the average value of the pixels in that cell\n>> + * split by colour components.\n>> *\n>> - * \\var IspStatsRegion::bSum\n>> - * \\brief Sum of the blue values in the region\n>> + * The Accumulator structure stores the sum of the average of each cell in a\n>> + * zone of the image, as well as the number of cells which were unsaturated and\n>> + * therefore included in the average.\n>> + * \\todo move this description and structure into a common header\n>> + *\n>> + * Cells which are saturated beyond the threshold defined in\n>> + * ipu3_uapi_awb_config_s are not included in the average.\n>> + *\n>> + * \\var Accumulator::counted\n>> + * \\brief Number of unsaturated cells used to calculate the sums\n>> + *\n>> + * \\var Accumulator::rSum\n>> + * \\brief Sum of the average red values of each unsaturated cell in the zone\n>> + *\n>> + * \\var Accumulator::gSum\n>> + * \\brief Sum of the average green values of each unsaturated cell in the zone\n>> + *\n>> + * \\var Accumulator::bSum\n>> + * \\brief Sum of the average blue values of each unsaturated cell in the zone\n>> */\n>> \n>> /**\n>> @@ -157,7 +202,7 @@ uint32_t Awb::estimateCCT(double red, double green, double blue)\n>> \treturn 449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33;\n>> }\n>> \n>> -/* Generate an RGB vector with the average values for each region */\n>> +/* Generate an RGB vector with the average values for each zone */\n>> void Awb::generateZones(std::vector &zones)\n>> {\n>> \tfor (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {\n>> @@ -174,7 +219,7 @@ void Awb::generateZones(std::vector &zones)\n>> \t}\n>> }\n>> \n>> -/* Translate the IPU3 statistics into the default statistics region array */\n>> +/* Translate the IPU3 statistics into the default statistics zone array */\n>> void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,\n>> \t\t\t const ipu3_uapi_grid_config &grid)\n>> {\n>> @@ -215,7 +260,6 @@ 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}\n>> }\n>> \n>> @@ -304,7 +348,7 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)\n>> \n>> \t/*\n>> \t * Optical center is column start (respectively row start) of the\n>> -\t * region of interest minus its X center (respectively Y center).\n>> +\t * cell of interest minus its X center (respectively Y center).\n>> \t *\n>> \t * For the moment use BDS as a first approximation, but it should\n>> \t * be calculated based on Shading (SHD) parameters.\n>> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h\n>> index cc848060..ac8ccc84 100644\n>> --- a/src/ipa/ipu3/algorithms/awb.h\n>> +++ b/src/ipa/ipu3/algorithms/awb.h\n>> @@ -33,9 +33,8 @@ 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>> \tunsigned long long gSum;\n>> \tunsigned long long bSum;\n>> @@ -82,7 +81,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 CBE2ABDB1D\n\tfor ;\n\tMon, 13 Sep 2021 16:45:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 27CC769185;\n\tMon, 13 Sep 2021 18:45:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 148EA6024C\n\tfor ;\n\tMon, 13 Sep 2021 18:45:25 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:edc5:688b:2ede:8b4b])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AFE8A499;\n\tMon, 13 Sep 2021 18:45:24 +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=\"ZLdWc3n2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631551524;\n\tbh=n5JUU+pJeno+hPIAFN9ibwvLwK7m6YAmwjHuk7p6dh8=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=ZLdWc3n2KNhdCopkYNnx7/a7fZbc8h7ejYjB7K1rI/THBlbzQew4BB2Eq5o52/CTo\n\tLoBYUnWJCdJQPe2Iez5OMqLoEG9gysVezO6cUPyQbuXk+EPsSPRJJy3UR5zkemfcSj\n\teXVBeOmqubgAIMbiadZqz2UZMR05qmoAFCSc58vs=","To":"Laurent Pinchart ","References":"<20210913132803.54881-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210913132803.54881-3-jeanmichel.hautbois@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","Message-ID":"<6c0aed89-a71a-2afe-e011-28dbe0ea5e4c@ideasonboard.com>","Date":"Mon, 13 Sep 2021 18:45:21 +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":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v7 2/5] ipa: ipu3: Rename\n\tIspStatsRegion to 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19647,"web_url":"https://patchwork.libcamera.org/comment/19647/","msgid":"","date":"2021-09-13T16:49:20","subject":"Re: [libcamera-devel] [PATCH v7 2/5] ipa: ipu3: Rename\n\tIspStatsRegion to Accumulator","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Sep 13, 2021 at 06:45:21PM +0200, Jean-Michel Hautbois wrote:\n> Hi Laurent,\n> \n> On 13/09/2021 17:56, Laurent Pinchart wrote:\n> > Hi Jean-Michel,\n> > \n> > Thank you for the patch.\n> > \n> > On Mon, Sep 13, 2021 at 03:28:00PM +0200, Jean-Michel Hautbois wrote:\n> >> The IspStatsRegion structure was introduced as an attempt to prepare for\n> >> a generic AWB algorithm structure. The structure name by itself is not\n> >> explicit and it is too optimistic to try and make a generic one for now.\n> >>\n> >> Its role is to accumulate the pixels in a given zone. Rename it to\n> >> accumulator, and remove the uncounted field at the same time. It is\n> >> always possible to know how many pixels are not relevant for the\n> >> algorithm by calculating total-counted. The uncounted field was only\n> >> declared and not used. Amend the documentation accordingly.\n> >>\n> >> Signed-off-by: Jean-Michel Hautbois \n> >> Reviewed-by: Kieran Bingham \n> >> Reviewed-by: Laurent Pinchart \n> >> ---\n> >> src/ipa/ipu3/algorithms/awb.cpp | 80 +++++++++++++++++++++++++--------\n> >> src/ipa/ipu3/algorithms/awb.h | 5 +--\n> >> 2 files changed, 64 insertions(+), 21 deletions(-)\n> >>\n> >> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> >> index e05647c9..d993e21e 100644\n> >> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> >> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> >> @@ -21,26 +21,71 @@ static constexpr uint32_t kMinZonesCounted = 16;\n> >> static constexpr uint32_t kMinGreenLevelInZone = 32;\n> >> \n> >> /**\n> >> - * \\struct IspStatsRegion\n> >> - * \\brief RGB statistics for a given region\n> >> + * \\struct Accumulator\n> >> + * \\brief RGB statistics for a given zone\n> >> *\n> >> - * The IspStatsRegion structure is intended to abstract the ISP specific\n> >> - * statistics and use an agnostic algorithm to compute AWB.\n> >> + * - Cells are defined in Pixels\n> >> + * - Zones are defined in Cells\n> >> + * - The total zones may overlap the image width and height to meet hardware\n> >> + * constraints\n> > \n> > Quoting your reply from v6:\n> > \n> > \"[This] means that the zones will be adapted to the BDS output size\n> > which may be greater than the rendered frame (asking for 1280x720 may\n> > lead to a 1296x720 BDS output and you will have 81 cells of 16 pixels\n> > width and not 80).\"\n> > \n> > From a statistics point of view, the zones match the output of the BDS.\n> > The image is then slightly cropped to achieve the required resolution. I\n> > think that's out of scope here. I would thus drop the last point. The\n> > image and text below need to be adapted to replace 1280 with 1296.\n> > \n> >> *\n> >> - * \\var IspStatsRegion::counted\n> >> - * \\brief Number of pixels used to calculate the sums\n> >> + * 81 cells\n> >> + * /───────────── 1280 pixels ───────────\\\n> >> + * 16 zones\n> >> + * 16\n> >> + * ┌────┬────┬────┬────┬────┬─ ──────┬────┐ \\\n> >> + * │Cell│ │ │ │ │ | │ │ │\n> >> + * 16 │ px │ │ │ │ │ | │ │ │\n> >> + * ├────┼────┼────┼────┼────┼─ ──────┼────┤ │\n> >> + * │ │ │ │ │ │ | │ │\n> >> + * │ │ │ │ │ │ | │ │ 7\n> >> + * │ ── │ ── │ ── │ ── │ ── │ ── ── ─┤ ── │ 1 2 4\n> >> + * │ │ │ │ │ │ | │ │ 2 0 5\n> >> *\n> >> - * \\var IspStatsRegion::uncounted\n> >> - * \\brief Remaining number of pixels in the region\n> >> + * │ │ │ │ │ │ | │ │ z p c\n> >> + * ├────┼────┼────┼────┼────┼─ ──────┼────┤ o i e\n> >> + * │ │ │ │ │ │ | │ │ n x l\n> >> + * │ │ | │ │ e e l\n> >> + * ├─── ───┼─ ──────┼────┤ s l s\n> >> + * │ │ | │ │ s\n> >> + * │ │ | │ │\n> >> + * ├─── Zone of Cells ───┼─ ──────┼────┤ │\n> >> + * │ (5 x 4) │ | │ │ │\n> >> + * │ │ | │ │ │\n> >> + * ├── ───┼─ ──────┼────┤ │\n> >> + * │ │ │ | │ │ │\n> >> + * │ │ │ │ │ │ | │ │ │\n> >> + * └────┴────┴────┴────┴────┴─ ──────┴────┘ /\n> >> *\n> >> - * \\var IspStatsRegion::rSum\n> >> - * \\brief Sum of the red values in the region\n> >> + * The algorithm works with a fixed number of zones \\a kAwbStatsSizeX x\n> >> + * \\a kAwbStatsSizeY. For example, a frame of 1280x720 is divided into 81x45\n> >> + * cells of [16x16] pixels with a BDS output size of 1296x720 to account for the\n> \n> Replacing 1280 by 1296 except this sentence, right :-) ?\n\nI'd write\n\n\"For example, a frame of 1296x720 is divided into 81x45 cells of [16x16] pixels.\"\n\nWe shouldn't mention the BDS here, this belongs to the documentation of\nthe ImgU, not the Accumulator structure.\n\n> >> + * 16x16 pixel alignment restrictions. In the case of \\a kAwbStatsSizeX=16 and\n> >> + * \\a kAwbStatsSizeY=12 the zones are made of [5x4] cells. The cells are\n> >> + * left-aligned and calculated by IPAIPU3::calculateBdsGrid().\n> >> *\n> >> - * \\var IspStatsRegion::gSum\n> >> - * \\brief Sum of the green values in the region\n> >> + * Each statistics cell represents the average value of the pixels in that cell\n> >> + * split by colour components.\n> >> *\n> >> - * \\var IspStatsRegion::bSum\n> >> - * \\brief Sum of the blue values in the region\n> >> + * The Accumulator structure stores the sum of the average of each cell in a\n> >> + * zone of the image, as well as the number of cells which were unsaturated and\n> >> + * therefore included in the average.\n> >> + * \\todo move this description and structure into a common header\n> >> + *\n> >> + * Cells which are saturated beyond the threshold defined in\n> >> + * ipu3_uapi_awb_config_s are not included in the average.\n> >> + *\n> >> + * \\var Accumulator::counted\n> >> + * \\brief Number of unsaturated cells used to calculate the sums\n> >> + *\n> >> + * \\var Accumulator::rSum\n> >> + * \\brief Sum of the average red values of each unsaturated cell in the zone\n> >> + *\n> >> + * \\var Accumulator::gSum\n> >> + * \\brief Sum of the average green values of each unsaturated cell in the zone\n> >> + *\n> >> + * \\var Accumulator::bSum\n> >> + * \\brief Sum of the average blue values of each unsaturated cell in the zone\n> >> */\n> >> \n> >> /**\n> >> @@ -157,7 +202,7 @@ uint32_t Awb::estimateCCT(double red, double green, double blue)\n> >> \treturn 449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33;\n> >> }\n> >> \n> >> -/* Generate an RGB vector with the average values for each region */\n> >> +/* Generate an RGB vector with the average values for each zone */\n> >> void Awb::generateZones(std::vector &zones)\n> >> {\n> >> \tfor (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {\n> >> @@ -174,7 +219,7 @@ void Awb::generateZones(std::vector &zones)\n> >> \t}\n> >> }\n> >> \n> >> -/* Translate the IPU3 statistics into the default statistics region array */\n> >> +/* Translate the IPU3 statistics into the default statistics zone array */\n> >> void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,\n> >> \t\t\t const ipu3_uapi_grid_config &grid)\n> >> {\n> >> @@ -215,7 +260,6 @@ 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}\n> >> }\n> >> \n> >> @@ -304,7 +348,7 @@ void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)\n> >> \n> >> \t/*\n> >> \t * Optical center is column start (respectively row start) of the\n> >> -\t * region of interest minus its X center (respectively Y center).\n> >> +\t * cell of interest minus its X center (respectively Y center).\n> >> \t *\n> >> \t * For the moment use BDS as a first approximation, but it should\n> >> \t * be calculated based on Shading (SHD) parameters.\n> >> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h\n> >> index cc848060..ac8ccc84 100644\n> >> --- a/src/ipa/ipu3/algorithms/awb.h\n> >> +++ b/src/ipa/ipu3/algorithms/awb.h\n> >> @@ -33,9 +33,8 @@ 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> >> \tunsigned long long gSum;\n> >> \tunsigned long long bSum;\n> >> @@ -82,7 +81,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> >>","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 D3CF3BDB1D\n\tfor ;\n\tMon, 13 Sep 2021 16:49:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 650FF69182;\n\tMon, 13 Sep 2021 18:49:46 +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 E7AD36024C\n\tfor ;\n\tMon, 13 Sep 2021 18:49:44 +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 67E5B499;\n\tMon, 13 Sep 2021 18:49:44 +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=\"gd23iWJQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631551784;\n\tbh=eYzyEm4o5K7QNwTxiwhuY8aZjmPCiV3eljeM2sI+zeM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gd23iWJQ3nK6LqHokK2K5t5Fp3ar/XBCxpsPfxAkGCh8kVB69t92l+llCJemIICKI\n\tYrj//OuJOpL4X6yoUNyveiwuSHS1nB1O/yBH+otnDh6khFLMjJ8Gy71LEmAJV560ls\n\tIsJh63HnYehq80Ff3/EFXJtbBRXcdHutXpMLyv9o=","Date":"Mon, 13 Sep 2021 19:49:20 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210913132803.54881-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210913132803.54881-3-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<6c0aed89-a71a-2afe-e011-28dbe0ea5e4c@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<6c0aed89-a71a-2afe-e011-28dbe0ea5e4c@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v7 2/5] ipa: ipu3: Rename\n\tIspStatsRegion to 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]