[{"id":19188,"web_url":"https://patchwork.libcamera.org/comment/19188/","msgid":"","date":"2021-08-30T22:59:38","subject":"Re: [libcamera-devel] [PATCH v2 1/4] ipa: ipu3: Move the AWB stats\n\tstructures","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 Fri, Aug 27, 2021 at 10:02:24AM +0200, Jean-Michel Hautbois wrote:\n> The structure Ipu3AwbCell describes the AWB stats layout on the kernel\n> side. We will need it to be used by the AGC algorithm to be introduced\n> later, so let's make it visible from ipa::ipu3::algorithms and not only\n> for the AWB class.\n> \n> This structure should probably go into the intel-ipu3.h file, whichs\n> means a kernel patch, let's keep it in mind for the moment.\n> \n> - IspStatsRegion is renamed Accumulator, and is storing the total number\n> of pixels along with the counted pixels now.\n> \n> - The RGB structure is a more generic structure, it should be in a more\n> generic header.\n> \n> - AwbStatus is used for the internal storage of the awb gains to update\n> the context in prepare. Align the IPAFrameContext to have the same\n> structure layout.\n\nWhy, what does having the same structure layout bring ? And why are\nthere two different structures if they're the same ? Please re-read my\nreview of v1.\n\nThis is all a big melting pot of random changes with no structure. The\neasiest way to fix that is to split it in multiple patches:\n\n- Move Ipu3AwbCell, RGB and IspStatsRegion from awb.cpp to awb.h\n- Rename IspStatsRegion to Accumulator\n- Replace Accumulator::uncounted with Accumulator::total\n- Fix the documentation of Accumulator::uncounted and Accumulator::total\n (see below)\n\nEach commit message should explain why.\n\n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/algorithms/awb.cpp | 47 ++++++++++---------\n> src/ipa/ipu3/algorithms/awb.h | 81 +++++++++++++++++----------------\n> src/ipa/ipu3/ipa_context.h | 1 +\n> src/ipa/ipu3/ipu3.cpp | 3 ++\n> 4 files changed, 71 insertions(+), 61 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> index e05647c9..136e79e0 100644\n> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> @@ -21,25 +21,27 @@ 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 region of\n> + * the image, as well as the number of relevant pixels in this same region. A\n> + * relevant pixel is an unsaturated pixel for this algorithm.\n> + * \\todo extend the notion of relevant to something else ?\n\nSounds confusing indeed.\n\n> *\n> - * \\var IspStatsRegion::counted\n> - * \\brief Number of pixels used to calculate the sums\n> + * \\var Accumulator::total\n> + * \\brief Total number of pixels in the region\n> *\n> - * \\var IspStatsRegion::uncounted\n> - * \\brief Remaining number of pixels in the region\n> + * \\var Accumulator::counted\n> + * \\brief Number of relevant pixels used to calculate the sums\n\nThis doesn't seem to be correct, don't those two fields store a number\nof zones, not a number of pixels ?\n\n> *\n> - * \\var IspStatsRegion::rSum\n> + * \\var Accumulator::rSum\n> * \\brief Sum of the red values in the region\n> *\n> - * \\var IspStatsRegion::gSum\n> + * \\var Accumulator::gSum\n> * \\brief Sum of the green values in the region\n> *\n> - * \\var IspStatsRegion::bSum\n> + * \\var Accumulator::bSum\n> * \\brief Sum of the blue values in the region\n> */\n> \n> @@ -117,9 +119,9 @@ static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {\n> Awb::Awb()\n> \t: Algorithm()\n> {\n> -\tasyncResults_.blueGain = 1.0;\n> -\tasyncResults_.greenGain = 1.0;\n> -\tasyncResults_.redGain = 1.0;\n> +\tasyncResults_.gains.blue = 1.0;\n> +\tasyncResults_.gains.green = 1.0;\n> +\tasyncResults_.gains.red = 1.0;\n> \tasyncResults_.temperatureK = 4500;\n> \n> \tzones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);\n> @@ -204,6 +206,7 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,\n> \t\t\t\tawbStats_[awbRegionPosition].rSum += currentCell->redAvg;\n> \t\t\t\tawbStats_[awbRegionPosition].bSum += currentCell->blueAvg;\n> \t\t\t}\n> +\t\t\tawbStats_[awbRegionPosition].total++;\n\nThis can be moved outside of the loops, you can calculate the total\nvalue instead of incrementing it for each pixel.\n\n> \t\t}\n> \t}\n> }\n> @@ -215,7 +218,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> @@ -254,9 +257,9 @@ void Awb::awbGreyWorld()\n> \n> \t/* Color temperature is not relevant in Grey world but still useful to estimate it :-) */\n> \tasyncResults_.temperatureK = estimateCCT(sumRed.R, sumRed.G, sumBlue.B);\n> -\tasyncResults_.redGain = redGain;\n> -\tasyncResults_.greenGain = 1.0;\n> -\tasyncResults_.blueGain = blueGain;\n> +\tasyncResults_.gains.red = redGain;\n> +\tasyncResults_.gains.green = 1.0;\n> +\tasyncResults_.gains.blue = blueGain;\n> }\n> \n> void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats,\n> @@ -270,8 +273,8 @@ void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats,\n> \tLOG(IPU3Awb, Debug) << \"Valid zones: \" << zones_.size();\n> \tif (zones_.size() > 10) {\n> \t\tawbGreyWorld();\n> -\t\tLOG(IPU3Awb, Debug) << \"Gain found for red: \" << asyncResults_.redGain\n> -\t\t\t\t << \" and for blue: \" << asyncResults_.blueGain;\n> +\t\tLOG(IPU3Awb, Debug) << \"Gain found for red: \" << asyncResults_.gains.red\n> +\t\t\t\t << \" and for blue: \" << asyncResults_.gains.blue;\n> \t}\n> }\n> \n> @@ -284,9 +287,9 @@ void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> \t * The results are cached, so if no results were calculated, we set the\n> \t * cached values from asyncResults_ here.\n> \t */\n> -\tcontext.frameContext.awb.gains.blue = asyncResults_.blueGain;\n> -\tcontext.frameContext.awb.gains.green = asyncResults_.greenGain;\n> -\tcontext.frameContext.awb.gains.red = asyncResults_.redGain;\n> +\tcontext.frameContext.awb.gains.blue = asyncResults_.gains.blue;\n> +\tcontext.frameContext.awb.gains.green = asyncResults_.gains.green;\n> +\tcontext.frameContext.awb.gains.red = asyncResults_.gains.red;\n> }\n> \n> void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)\n> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h\n> index a16dd68d..f7e2f4cd 100644\n> --- a/src/ipa/ipu3/algorithms/awb.h\n> +++ b/src/ipa/ipu3/algorithms/awb.h\n> @@ -23,6 +23,38 @@ namespace ipa::ipu3::algorithms {\n> static constexpr uint32_t kAwbStatsSizeX = 16;\n> static constexpr uint32_t kAwbStatsSizeY = 12;\n> \n> +/* \\todo Move the cell layout into intel-ipu3.h kernel header */\n> +struct Ipu3AwbCell {\n> +\tunsigned char greenRedAvg;\n> +\tunsigned char redAvg;\n> +\tunsigned char blueAvg;\n> +\tunsigned char greenBlueAvg;\n> +\tunsigned char satRatio;\n> +\tunsigned char padding[3];\n> +} __attribute__((packed));\n> +\n> +/* \\todo Make these structs available to all the ISPs ? */\n> +struct RGB {\n> +\tRGB(double _R = 0, double _G = 0, double _B = 0)\n> +\t\t: R(_R), G(_G), B(_B)\n> +\t{\n> +\t}\n> +\tdouble R, G, B;\n> +\tRGB &operator+=(RGB const &other)\n> +\t{\n> +\t\tR += other.R, G += other.G, B += other.B;\n> +\t\treturn *this;\n> +\t}\n> +};\n\nThere's nothing AWB-specific in this structure, this is really not the\nright place to store it. Given that it's not used in the rest of this\nseries, you can take the easy option and leave it where it is.\n\n> +\n\nThis is missing a \\todo comment to tell where this should be moved, but\nthe best would be to move it already.\n\n> +struct Accumulator {\n> +\tunsigned int total;\n> +\tunsigned int counted;\n> +\tunsigned long long rSum;\n> +\tunsigned long long gSum;\n> +\tunsigned long long bSum;\n\nAnother patch in this series should change the type of the last three\nfields to uint64_t. If we follow the AwbStatus model, you could also\nchange it to\n\nstruct Accumulator {\n\tunsigned int total;\n\tunsigned int counted;\n\tstruct {\n\t\tuint64_t red;\n\t\tuint64_t green;\n\t\tuint64_t blue;\n\t} sum;\n};\n\nUp to you for this last change (which can be in the same patch as the\nswitch to uint64_t if you decide to change the structure layout).\n\n> +};\n> +\n> class Awb : public Algorithm\n> {\n> public:\n> @@ -32,44 +64,6 @@ public:\n> \tvoid prepare(IPAContext &context, ipu3_uapi_params *params) override;\n> \tvoid process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;\n> \n> -\tstruct Ipu3AwbCell {\n> -\t\tunsigned char greenRedAvg;\n> -\t\tunsigned char redAvg;\n> -\t\tunsigned char blueAvg;\n> -\t\tunsigned char greenBlueAvg;\n> -\t\tunsigned char satRatio;\n> -\t\tunsigned char padding[3];\n> -\t} __attribute__((packed));\n> -\n> -\t/* \\todo Make these three structs available to all the ISPs ? */\n> -\tstruct RGB {\n> -\t\tRGB(double _R = 0, double _G = 0, double _B = 0)\n> -\t\t\t: R(_R), G(_G), B(_B)\n> -\t\t{\n> -\t\t}\n> -\t\tdouble R, G, B;\n> -\t\tRGB &operator+=(RGB const &other)\n> -\t\t{\n> -\t\t\tR += other.R, G += other.G, B += other.B;\n> -\t\t\treturn *this;\n> -\t\t}\n> -\t};\n> -\n> -\tstruct IspStatsRegion {\n> -\t\tunsigned int counted;\n> -\t\tunsigned int uncounted;\n> -\t\tunsigned long long rSum;\n> -\t\tunsigned long long gSum;\n> -\t\tunsigned long long bSum;\n> -\t};\n> -\n> -\tstruct AwbStatus {\n> -\t\tdouble temperatureK;\n> -\t\tdouble redGain;\n> -\t\tdouble greenGain;\n> -\t\tdouble blueGain;\n> -\t};\n> -\n> private:\n> \tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats,\n> \t\t\t const ipu3_uapi_grid_config &grid);\n> @@ -80,8 +74,17 @@ private:\n> \tvoid awbGreyWorld();\n> \tuint32_t estimateCCT(double red, double green, double blue);\n> \n> +\tstruct AwbStatus {\n> +\t\tdouble temperatureK;\n> +\t\tstruct {\n> +\t\t\tdouble red;\n> +\t\t\tdouble green;\n> +\t\t\tdouble blue;\n> +\t\t} gains;\n> +\t};\n> +\n> \tstd::vector zones_;\n> -\tIspStatsRegion awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];\n> +\tAccumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];\n> \tAwbStatus asyncResults_;\n> };\n> \n> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n> index 9d9444dc..3a292ad7 100644\n> --- a/src/ipa/ipu3/ipa_context.h\n> +++ b/src/ipa/ipu3/ipa_context.h\n> @@ -30,6 +30,7 @@ struct IPAFrameContext {\n> \t} agc;\n> \n> \tstruct {\n> +\t\tdouble temperatureK;\n> \t\tstruct {\n> \t\t\tdouble red;\n> \t\t\tdouble green;\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 0ed0a6f1..cbb3f440 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -112,6 +112,9 @@\n> * \\struct IPAFrameContext::awb\n> * \\brief Context for the Automatic White Balance algorithm\n> *\n> + * \\var IPAFrameContext::awb::temperatureK\n> + * \\brief Estimated color temperature in Kelvin\n> + *\n> * \\struct IPAFrameContext::awb::gains\n> * \\brief White balance gains\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 6F1B3BDC71\n\tfor ;\n\tMon, 30 Aug 2021 23:00:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B0A756916A;\n\tTue, 31 Aug 2021 01:00:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0498B68891\n\tfor ;\n\tTue, 31 Aug 2021 00:59:53 +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 684FE5A7;\n\tTue, 31 Aug 2021 00:59:52 +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=\"I5Sbyhsp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630364392;\n\tbh=+bDKH7o3o1KsCKOnol2Tc/jdGDjw1FvrGpwdUu8xLMw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=I5Sbyhsp1uYc0G6G84wKizzAIf/fv49CG2M1Wxmk8eGUEn0TfPz8B6A4bh0qM1ILe\n\tywWLzUif66wuhq+lZVJJbOWv2zAY4n/SVGi0H1BeljH67God9rld3zTpKHtXfVEV5i\n\t+BfHc1bmGBIuDp1dWybEiqn+nCz2eDk3jrKTtgBk=","Date":"Tue, 31 Aug 2021 01:59:38 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210827080227.26370-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210827080227.26370-2-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210827080227.26370-2-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/4] ipa: ipu3: Move the AWB stats\n\tstructures","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":19205,"web_url":"https://patchwork.libcamera.org/comment/19205/","msgid":"","date":"2021-08-31T08:32:18","subject":"Re: [libcamera-devel] [PATCH v2 1/4] ipa: ipu3: Move the AWB stats\n\tstructures","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 31/08/2021 00:59, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> Thank you for the patch.\n> \n> On Fri, Aug 27, 2021 at 10:02:24AM +0200, Jean-Michel Hautbois wrote:\n>> The structure Ipu3AwbCell describes the AWB stats layout on the kernel\n>> side. We will need it to be used by the AGC algorithm to be introduced\n>> later, so let's make it visible from ipa::ipu3::algorithms and not only\n>> for the AWB class.\n>>\n>> This structure should probably go into the intel-ipu3.h file, whichs\n>> means a kernel patch, let's keep it in mind for the moment.\n>>\n>> - IspStatsRegion is renamed Accumulator, and is storing the total number\n>> of pixels along with the counted pixels now.\n>>\n>> - The RGB structure is a more generic structure, it should be in a more\n>> generic header.\n>>\n>> - AwbStatus is used for the internal storage of the awb gains to update\n>> the context in prepare. Align the IPAFrameContext to have the same\n>> structure layout.\n> \n> Why, what does having the same structure layout bring ? And why are\n> there two different structures if they're the same ? Please re-read my\n> review of v1.\n\nOh, yeah, I removed it in another branch (-ETOOMUCHBRANCHES)...\nSorry for the noise here...\n\n> This is all a big melting pot of random changes with no structure. The\n> easiest way to fix that is to split it in multiple patches:\n> \n> - Move Ipu3AwbCell, RGB and IspStatsRegion from awb.cpp to awb.h\n\nThose are already in awb.h, just getting out from\nlibcamera::ipa::ipu3::algorithms::awb to\nlibcamera::ipa::ipu3::algorithms directly.\n\n> - Rename IspStatsRegion to Accumulator\nOne patch only for renaming the structure ? Sounds like a really short one ?\n\n> - Replace Accumulator::uncounted with Accumulator::total\n> - Fix the documentation of Accumulator::uncounted and Accumulator::total\n> (see below)\n\nThanks (but isn't only one patch enough for everything related to\nIspStatsRegion ?).\n\n\n> Each commit message should explain why.\n> \n>> Signed-off-by: Jean-Michel Hautbois \n>> ---\n>> src/ipa/ipu3/algorithms/awb.cpp | 47 ++++++++++---------\n>> src/ipa/ipu3/algorithms/awb.h | 81 +++++++++++++++++----------------\n>> src/ipa/ipu3/ipa_context.h | 1 +\n>> src/ipa/ipu3/ipu3.cpp | 3 ++\n>> 4 files changed, 71 insertions(+), 61 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n>> index e05647c9..136e79e0 100644\n>> --- a/src/ipa/ipu3/algorithms/awb.cpp\n>> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n>> @@ -21,25 +21,27 @@ 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 region of\n>> + * the image, as well as the number of relevant pixels in this same region. A\n>> + * relevant pixel is an unsaturated pixel for this algorithm.\n>> + * \\todo extend the notion of relevant to something else ?\n> \n> Sounds confusing indeed.\n> \n>> *\n>> - * \\var IspStatsRegion::counted\n>> - * \\brief Number of pixels used to calculate the sums\n>> + * \\var Accumulator::total\n>> + * \\brief Total number of pixels in the region\n>> *\n>> - * \\var IspStatsRegion::uncounted\n>> - * \\brief Remaining number of pixels in the region\n>> + * \\var Accumulator::counted\n>> + * \\brief Number of relevant pixels used to calculate the sums\n> \n> This doesn't seem to be correct, don't those two fields store a number\n> of zones, not a number of pixels ?\n> \n>> *\n>> - * \\var IspStatsRegion::rSum\n>> + * \\var Accumulator::rSum\n>> * \\brief Sum of the red values in the region\n>> *\n>> - * \\var IspStatsRegion::gSum\n>> + * \\var Accumulator::gSum\n>> * \\brief Sum of the green values in the region\n>> *\n>> - * \\var IspStatsRegion::bSum\n>> + * \\var Accumulator::bSum\n>> * \\brief Sum of the blue values in the region\n>> */\n>> \n>> @@ -117,9 +119,9 @@ static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {\n>> Awb::Awb()\n>> \t: Algorithm()\n>> {\n>> -\tasyncResults_.blueGain = 1.0;\n>> -\tasyncResults_.greenGain = 1.0;\n>> -\tasyncResults_.redGain = 1.0;\n>> +\tasyncResults_.gains.blue = 1.0;\n>> +\tasyncResults_.gains.green = 1.0;\n>> +\tasyncResults_.gains.red = 1.0;\n>> \tasyncResults_.temperatureK = 4500;\n>> \n>> \tzones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);\n>> @@ -204,6 +206,7 @@ void Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats,\n>> \t\t\t\tawbStats_[awbRegionPosition].rSum += currentCell->redAvg;\n>> \t\t\t\tawbStats_[awbRegionPosition].bSum += currentCell->blueAvg;\n>> \t\t\t}\n>> +\t\t\tawbStats_[awbRegionPosition].total++;\n> \n> This can be moved outside of the loops, you can calculate the total\n> value instead of incrementing it for each pixel.\n> \n\nIndeed :-).\n\n>> \t\t}\n>> \t}\n>> }\n>> @@ -215,7 +218,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>> @@ -254,9 +257,9 @@ void Awb::awbGreyWorld()\n>> \n>> \t/* Color temperature is not relevant in Grey world but still useful to estimate it :-) */\n>> \tasyncResults_.temperatureK = estimateCCT(sumRed.R, sumRed.G, sumBlue.B);\n>> -\tasyncResults_.redGain = redGain;\n>> -\tasyncResults_.greenGain = 1.0;\n>> -\tasyncResults_.blueGain = blueGain;\n>> +\tasyncResults_.gains.red = redGain;\n>> +\tasyncResults_.gains.green = 1.0;\n>> +\tasyncResults_.gains.blue = blueGain;\n>> }\n>> \n>> void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats,\n>> @@ -270,8 +273,8 @@ void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats,\n>> \tLOG(IPU3Awb, Debug) << \"Valid zones: \" << zones_.size();\n>> \tif (zones_.size() > 10) {\n>> \t\tawbGreyWorld();\n>> -\t\tLOG(IPU3Awb, Debug) << \"Gain found for red: \" << asyncResults_.redGain\n>> -\t\t\t\t << \" and for blue: \" << asyncResults_.blueGain;\n>> +\t\tLOG(IPU3Awb, Debug) << \"Gain found for red: \" << asyncResults_.gains.red\n>> +\t\t\t\t << \" and for blue: \" << asyncResults_.gains.blue;\n>> \t}\n>> }\n>> \n>> @@ -284,9 +287,9 @@ void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n>> \t * The results are cached, so if no results were calculated, we set the\n>> \t * cached values from asyncResults_ here.\n>> \t */\n>> -\tcontext.frameContext.awb.gains.blue = asyncResults_.blueGain;\n>> -\tcontext.frameContext.awb.gains.green = asyncResults_.greenGain;\n>> -\tcontext.frameContext.awb.gains.red = asyncResults_.redGain;\n>> +\tcontext.frameContext.awb.gains.blue = asyncResults_.gains.blue;\n>> +\tcontext.frameContext.awb.gains.green = asyncResults_.gains.green;\n>> +\tcontext.frameContext.awb.gains.red = asyncResults_.gains.red;\n>> }\n>> \n>> void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)\n>> diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h\n>> index a16dd68d..f7e2f4cd 100644\n>> --- a/src/ipa/ipu3/algorithms/awb.h\n>> +++ b/src/ipa/ipu3/algorithms/awb.h\n>> @@ -23,6 +23,38 @@ namespace ipa::ipu3::algorithms {\n>> static constexpr uint32_t kAwbStatsSizeX = 16;\n>> static constexpr uint32_t kAwbStatsSizeY = 12;\n>> \n>> +/* \\todo Move the cell layout into intel-ipu3.h kernel header */\n>> +struct Ipu3AwbCell {\n>> +\tunsigned char greenRedAvg;\n>> +\tunsigned char redAvg;\n>> +\tunsigned char blueAvg;\n>> +\tunsigned char greenBlueAvg;\n>> +\tunsigned char satRatio;\n>> +\tunsigned char padding[3];\n>> +} __attribute__((packed));\n>> +\n>> +/* \\todo Make these structs available to all the ISPs ? */\n>> +struct RGB {\n>> +\tRGB(double _R = 0, double _G = 0, double _B = 0)\n>> +\t\t: R(_R), G(_G), B(_B)\n>> +\t{\n>> +\t}\n>> +\tdouble R, G, B;\n>> +\tRGB &operator+=(RGB const &other)\n>> +\t{\n>> +\t\tR += other.R, G += other.G, B += other.B;\n>> +\t\treturn *this;\n>> +\t}\n>> +};\n> \n> There's nothing AWB-specific in this structure, this is really not the\n> right place to store it. Given that it's not used in the rest of this\n> series, you can take the easy option and leave it where it is.\n> \n\nThat's probably what I will do ;-).\n\n>> +\n> \n> This is missing a \\todo comment to tell where this should be moved, but\n> the best would be to move it already.\n> \n>> +struct Accumulator {\n>> +\tunsigned int total;\n>> +\tunsigned int counted;\n>> +\tunsigned long long rSum;\n>> +\tunsigned long long gSum;\n>> +\tunsigned long long bSum;\n> \n> Another patch in this series should change the type of the last three\n> fields to uint64_t. If we follow the AwbStatus model, you could also\n> change it to\n> \n> struct Accumulator {\n> \tunsigned int total;\n> \tunsigned int counted;\n> \tstruct {\n> \t\tuint64_t red;\n> \t\tuint64_t green;\n> \t\tuint64_t blue;\n> \t} sum;\n> };\n> \n> Up to you for this last change (which can be in the same patch as the\n> switch to uint64_t if you decide to change the structure layout).\n\nIt could be in the same patch as uncounted->total probably.\n\n>> +};\n>> +\n>> class Awb : public Algorithm\n>> {\n>> public:\n>> @@ -32,44 +64,6 @@ public:\n>> \tvoid prepare(IPAContext &context, ipu3_uapi_params *params) override;\n>> \tvoid process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;\n>> \n>> -\tstruct Ipu3AwbCell {\n>> -\t\tunsigned char greenRedAvg;\n>> -\t\tunsigned char redAvg;\n>> -\t\tunsigned char blueAvg;\n>> -\t\tunsigned char greenBlueAvg;\n>> -\t\tunsigned char satRatio;\n>> -\t\tunsigned char padding[3];\n>> -\t} __attribute__((packed));\n>> -\n>> -\t/* \\todo Make these three structs available to all the ISPs ? */\n>> -\tstruct RGB {\n>> -\t\tRGB(double _R = 0, double _G = 0, double _B = 0)\n>> -\t\t\t: R(_R), G(_G), B(_B)\n>> -\t\t{\n>> -\t\t}\n>> -\t\tdouble R, G, B;\n>> -\t\tRGB &operator+=(RGB const &other)\n>> -\t\t{\n>> -\t\t\tR += other.R, G += other.G, B += other.B;\n>> -\t\t\treturn *this;\n>> -\t\t}\n>> -\t};\n>> -\n>> -\tstruct IspStatsRegion {\n>> -\t\tunsigned int counted;\n>> -\t\tunsigned int uncounted;\n>> -\t\tunsigned long long rSum;\n>> -\t\tunsigned long long gSum;\n>> -\t\tunsigned long long bSum;\n>> -\t};\n>> -\n>> -\tstruct AwbStatus {\n>> -\t\tdouble temperatureK;\n>> -\t\tdouble redGain;\n>> -\t\tdouble greenGain;\n>> -\t\tdouble blueGain;\n>> -\t};\n>> -\n>> private:\n>> \tvoid calculateWBGains(const ipu3_uapi_stats_3a *stats,\n>> \t\t\t const ipu3_uapi_grid_config &grid);\n>> @@ -80,8 +74,17 @@ private:\n>> \tvoid awbGreyWorld();\n>> \tuint32_t estimateCCT(double red, double green, double blue);\n>> \n>> +\tstruct AwbStatus {\n>> +\t\tdouble temperatureK;\n>> +\t\tstruct {\n>> +\t\t\tdouble red;\n>> +\t\t\tdouble green;\n>> +\t\t\tdouble blue;\n>> +\t\t} gains;\n>> +\t};\n>> +\n>> \tstd::vector zones_;\n>> -\tIspStatsRegion awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];\n>> +\tAccumulator awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];\n>> \tAwbStatus asyncResults_;\n>> };\n>> \n>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h\n>> index 9d9444dc..3a292ad7 100644\n>> --- a/src/ipa/ipu3/ipa_context.h\n>> +++ b/src/ipa/ipu3/ipa_context.h\n>> @@ -30,6 +30,7 @@ struct IPAFrameContext {\n>> \t} agc;\n>> \n>> \tstruct {\n>> +\t\tdouble temperatureK;\n>> \t\tstruct {\n>> \t\t\tdouble red;\n>> \t\t\tdouble green;\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 0ed0a6f1..cbb3f440 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -112,6 +112,9 @@\n>> * \\struct IPAFrameContext::awb\n>> * \\brief Context for the Automatic White Balance algorithm\n>> *\n>> + * \\var IPAFrameContext::awb::temperatureK\n>> + * \\brief Estimated color temperature in Kelvin\n>> + *\n>> * \\struct IPAFrameContext::awb::gains\n>> * \\brief White balance gains\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 C132BBD87D\n\tfor ;\n\tTue, 31 Aug 2021 08:32:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2F3516916A;\n\tTue, 31 Aug 2021 10:32:23 +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 CBB2260288\n\tfor ;\n\tTue, 31 Aug 2021 10:32:21 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:b904:a32:6762:6e37])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6009C323;\n\tTue, 31 Aug 2021 10:32:21 +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=\"dZFTiG69\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630398741;\n\tbh=hMia0KbivWWiT/rlpbMq9Eh7mLwdM6uSHU+5vF4EGhs=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=dZFTiG694y02WMP6XkKlFcwePPf3kkCYGUE+Smj9M1H1oyXLkfyDrSkqbyvKJyEiJ\n\tgNNsoSkBHno3b+iVHRF7zOEbK2Y45NzLSOci9eCL1ieTC0xlF4QNQ1ME1WKdUMP5QF\n\tHHDXE98eJKiWvCC51FYxYsV2186xzUTgcrXVw3x8=","To":"Laurent Pinchart ","References":"<20210827080227.26370-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210827080227.26370-2-jeanmichel.hautbois@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","Message-ID":"","Date":"Tue, 31 Aug 2021 10:32:18 +0200","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":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 1/4] ipa: ipu3: Move the AWB stats\n\tstructures","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\" "}}]