[{"id":19572,"web_url":"https://patchwork.libcamera.org/comment/19572/","msgid":"<812419d0-1497-53d4-3709-ff0c8a1e955c@ideasonboard.com>","date":"2021-09-09T09:12:26","subject":"Re: [libcamera-devel] [PATCH v5 2/4] ipa: ipu3: Rename\n\tIspStatsRegion to Accumulator","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 09/09/2021 09:25, 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 region. 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> ---\n> src/ipa/ipu3/algorithms/awb.cpp | 36 ++++++++++++++++++++-------------\n> src/ipa/ipu3/algorithms/awb.h | 5 ++---\n> 2 files changed, 24 insertions(+), 17 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> index e05647c9..97f5839f 100644\n> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> @@ -21,26 +21,35 @@ 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> + * A frame is divided into zones, each zone is made of multiple regions which\n> + * are defined by the grid configuration. The algorithm works with a fixed\n> + * number of zones \\a kAwbStatsSizeX x \\a kAwbStatsSizeY.\n> + * For example, a frame of 1280x720 is divided into 81x45 regions of [16x16]\n> + * pixels because the BDS output size will be calculated as 1296x720. In the\n> + * case of \\a kAwbStatsSizeX=16 and \\a kAwbStatsSizeY=12 the zones are made of\n> + * [5x4] regions. The regions are left-aligned and calculated by\n> + * IPAIPU3::calculateBdsGrid().\n> *\n> - * \\var IspStatsRegion::counted\n> - * \\brief Number of pixels used to calculate the sums\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> + * \\todo extend the notion of relevant to something else ?\n\nWhat else could it be?\n\nYou've defined relevant as \"unsaturated regions\" so perhaps we could\njust use that ?\n\n\"\"\"\n* The Accumulator structure stores the sum of the pixel values in a zone of\n* the image, as well as the number of unsaturated regions in this same zone.\n*\n* Zones are determined to be saturated if their value exceeds the\n* threshold given by the thresholds in\n* ipu3_uapi_awb_config_s.rgbs_thr_{gr,r,gb,b}.\n*\n* \\todo It is not fully clear from the IPU3 documentation what defines a\n* saturated zone, This should be investigated further.\n\"\"\"\n\n\n\n\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\ns/relevant/unsaturated/\n\nWith those:\n\nReviewed-by: Kieran Bingham \n\n> *\n> - * \\var IspStatsRegion::rSum\n> - * \\brief Sum of the red values in the region\n> + * \\var Accumulator::rSum\n> + * \\brief Sum of the red values in the zone\n> *\n> - * \\var IspStatsRegion::gSum\n> - * \\brief Sum of the green values in the region\n> + * \\var Accumulator::gSum\n> + * \\brief Sum of the green values in the zone\n> *\n> - * \\var IspStatsRegion::bSum\n> - * \\brief Sum of the blue values in the region\n> + * \\var Accumulator::bSum\n> + * \\brief Sum of the blue values in the zone\n> */\n> \n> /**\n> @@ -215,7 +224,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> 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 4D7A7BDC71\n\tfor ;\n\tThu, 9 Sep 2021 09:12:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7F8D66916E;\n\tThu, 9 Sep 2021 11:12:32 +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 629046024E\n\tfor ;\n\tThu, 9 Sep 2021 11:12:30 +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 CB6C8466;\n\tThu, 9 Sep 2021 11:12:29 +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=\"Ky3BV+rq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631178750;\n\tbh=tUcGKCPcAv7PmT4FAbG1G+ZSqIimiy3jYFbXUeohfhg=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=Ky3BV+rqvU9uiSYlWwaU9phfMt3vF0cMe1M4mnWZy3+ogqISrwLMSe6bdC+YpEOsy\n\tz69xkw4y04Tno/Gqp8knmMJqqlqyM6uCSmqA950Wh9EaHTTHQs5eDQJndCQAdbaC8B\n\tGHVarIoe9hP2W9N59mTbNhbwOhmt5wD8Jg654Kyg=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210909082516.26055-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210909082516.26055-3-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"<812419d0-1497-53d4-3709-ff0c8a1e955c@ideasonboard.com>","Date":"Thu, 9 Sep 2021 10:12:26 +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":"<20210909082516.26055-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 v5 2/4] 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19574,"web_url":"https://patchwork.libcamera.org/comment/19574/","msgid":"<947d4508-00a3-5ed1-d210-28b83b8dd002@ideasonboard.com>","date":"2021-09-09T10:50:03","subject":"Re: [libcamera-devel] [PATCH v5 2/4] ipa: ipu3: Rename\n\tIspStatsRegion to Accumulator","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 09/09/2021 09:25, 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 region. Rename it to\n\ns/region/zone/?\n\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> ---\n> src/ipa/ipu3/algorithms/awb.cpp | 36 ++++++++++++++++++++-------------\n> src/ipa/ipu3/algorithms/awb.h | 5 ++---\n> 2 files changed, 24 insertions(+), 17 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> index e05647c9..97f5839f 100644\n> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> @@ -21,26 +21,35 @@ 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> + * A frame is divided into zones, each zone is made of multiple regions which\n\ns/region/cell/\n\n> + * are defined by the grid configuration. The algorithm works with a fixed\n> + * number of zones \\a kAwbStatsSizeX x \\a kAwbStatsSizeY.\n> + * For example, a frame of 1280x720 is divided into 81x45 regions of [16x16]\n\n\ns/regions/cells/\n\n\n> + * pixels because the BDS output size will be calculated as 1296x720. In the\n\npixels with a BDS output size of 1296x720 to account for the 16x16 pixel\nalignment restrictions.\n\n\n> + * case of \\a kAwbStatsSizeX=16 and \\a kAwbStatsSizeY=12 the zones are made of\n> + * [5x4] regions. The regions are left-aligned and calculated by\n\ns/regions/cells/\n\n> + * IPAIPU3::calculateBdsGrid().\n> *\n> - * \\var IspStatsRegion::counted\n> - * \\brief Number of pixels used to calculate the sums\n\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> + * \\todo extend the notion of relevant to something else ?\n\n\nTrying again with my new understandings:\n\n\"\"\"\nEach statistics cell represents the average value of the pixels in that\ncell split by colour components.\n\nThe Accumulator structure stores the sum of the average of each cell in\na zone of the image, as well as the number of cells which were\nunsaturated and therefore included in the average.\n\nCells which are saturated beyond the threshold defined in\nipu3_uapi_awb_config_s are not included in the average.\n\"\"\"\n\n\n\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::rSum\n> + * \\brief Sum of the red values in the zone\n\nFrom my understanding reading the following patches and how this is\ncalculated, these briefs are (slightly) incorrect.\n\nIs it more correct to say:\n\n \\brief Sum of the average red values of each cell in the zone\n\n\nAlso (as discussed directly with you), I was confused between cell,\nzone, and region.\n\nWe've come up with this diagram together to clarify this which might be\nworth adding somewhere as documentation of the grid:\n\n - Cells are defined in Pixels\n - Zones are defined in Cells\n - The total zones may overlap the image\n width and height to meet hardware constraints\n\n\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 │ │ │ │ │ │ | │ │ 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\nI now understand that you have used the term region to mean cell.\n\nCould you try to incorporate that into the beginning of this\ndocumentation? or maybe we should document this as the grid separately\nand reference it from here?\n\n(We could always put something here for now, and then write up the grid\nlater, moving this out to that new documentation).\n\n\n\n\n> *\n> - * \\var IspStatsRegion::gSum\n> - * \\brief Sum of the green values in the region\n> + * \\var Accumulator::gSum\n> + * \\brief Sum of the green values in the zone\n> *\n> - * \\var IspStatsRegion::bSum\n> - * \\brief Sum of the blue values in the region\n> + * \\var Accumulator::bSum\n> + * \\brief Sum of the blue values in the zone\n> */\n> \n> /**\n> @@ -215,7 +224,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> 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 BAA33BDB1D\n\tfor ;\n\tThu, 9 Sep 2021 10:50:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A8D06916E;\n\tThu, 9 Sep 2021 12:50: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 DB2266916B\n\tfor ;\n\tThu, 9 Sep 2021 12:50:06 +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 5FB2A466;\n\tThu, 9 Sep 2021 12:50: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=\"BYq9uEax\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1631184606;\n\tbh=nRJyfYY/e6PZZ6ZE9+KOp0nmEkaleyvSstoXbFdxGBs=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=BYq9uEaxf28VKIwT/NuayQoz58ELiEmzF341wOgwCdvJsYbqW8xDPqhizJsq1R/Ru\n\tJImZBBHQhKbf1Ufbp8f/11jemIFRVRhjnUDZgyfJS4vH8pE/9+BxK5AylR5nFYE5rl\n\tsMsAca6ZWpI0qJ+r0S6xTBxpu4fJ8o1NtQgJm0b8=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210909082516.26055-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210909082516.26055-3-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"<947d4508-00a3-5ed1-d210-28b83b8dd002@ideasonboard.com>","Date":"Thu, 9 Sep 2021 11:50:03 +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":"<20210909082516.26055-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 v5 2/4] 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]