[{"id":18013,"web_url":"https://patchwork.libcamera.org/comment/18013/","msgid":"","date":"2021-07-07T12:23:22","subject":"Re: [libcamera-devel] [PATCH v1 2/7] ipa: libipa: Create a common\n\tISP header to store the structure types","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi JM,\n\nOn 28/06/2021 21:22, Jean-Michel Hautbois wrote:\n> Each ISP may use the same AWB or AGC algorithms. In order to avoid\n> duplicated code, create a header with the main structures used for now.\n\n\nI'm not sure these are really 'isp' structures, and they are more\n'statistics' strutures.\n\nI'd be tempted to call this 'statistics.h' rather than isp.h ...\nalthough - in fact one of the structures is the AwbStatus .. that's the\nresults of the algorithm isn't it ?\n\nI wonder if the common structures which are specific to an algorithm\nshould be broken out to their own algorithm specific header.\n\nI.e. anything common to all Awb should be libipa/awb.h ...\n\n\n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/ipu3_agc.h | 1 +\n> src/ipa/ipu3/ipu3_awb.h | 30 +----------\n> src/ipa/libipa/isp.h | 110 ++++++++++++++++++++++++++++++++++++++++\n> 3 files changed, 112 insertions(+), 29 deletions(-)\n> create mode 100644 src/ipa/libipa/isp.h\n> \n> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n> index 3deca3ae..6ca9af8e 100644\n> --- a/src/ipa/ipu3/ipu3_agc.h\n> +++ b/src/ipa/ipu3/ipu3_agc.h\n> @@ -17,6 +17,7 @@\n> #include \n> \n> #include \"libipa/algorithm.h\"\n> +#include \"libipa/isp.h\"\n> \n> namespace libcamera {\n> \n> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h\n> index 122cf68c..6ae111fd 100644\n> --- a/src/ipa/ipu3/ipu3_awb.h\n> +++ b/src/ipa/ipu3/ipu3_awb.h\n> @@ -14,6 +14,7 @@\n> #include \n> \n> #include \"libipa/algorithm.h\"\n> +#include \"libipa/isp.h\"\n> \n> namespace libcamera {\n> \n> @@ -42,35 +43,6 @@ public:\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 generateZones(std::vector &zones);\n> \tvoid generateAwbStats(const ipu3_uapi_stats_3a *stats);\n> diff --git a/src/ipa/libipa/isp.h b/src/ipa/libipa/isp.h\n> new file mode 100644\n> index 00000000..a15803d6\n> --- /dev/null\n> +++ b/src/ipa/libipa/isp.h\n\n\nI'm torn on the include locations too ...\n\nglobally, I would have expected libcamera headers to be under the root\ninclude/ so that would make this include/ipa/ or include/libipa - but I\nreally think these headers are better kept close to the libipa - and\nthey are not generating an externally visible library anyway, so perhaps\nkeeping them local is good for now.\n\n\n\n> @@ -0,0 +1,110 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Ideas On Board\n> + *\n> + * isp.h - ISP statistics interface\n> + */\n> +#ifndef __LIBCAMERA_IPA_LIBIPA_ISP_H__\n> +#define __LIBCAMERA_IPA_LIBIPA_ISP_H__\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +/**\n> + * \\struct RGB\n> + * \\brief RGB\n> + *\n> + * \\fn RGB::RGB\n> + * \\brief Construct a RGB structure\n> + * \\param[in] _R Red value to set, defaults to 0\n> + * \\param[in] _G Green value to set, defaults to 0\n> + * \\param[in] _B Blue value to set, defaults to 0\n> + *\n> + * \\var RGB::R\n> + * \\brief Red value of the RGB structure\n> + *\n> + * \\var RGB::G\n> + * \\brief Green value of the RGB structure\n> + *\n> + * \\var RGB::B\n> + * \\brief Blue value of the RGB structure\n> + *\n> + * \\fn RGB &RGB::operator+=(RGB const &other)\n> + * \\brief Add each RGB value to another RGB structure\n> + * \\param[in] other An RGB structure\n> + * \\return An RGB structure containing the added R, G and B values\n> + */\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> +/**\n> + * \\struct IspStatsRegion\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> + *\n> + * \\var IspStatsRegion::counted\n> + * \\brief Number of pixels used to calculate the sums\n> + *\n> + * \\var IspStatsRegion::uncounted\n> + * \\brief Remaining number of pixels in the region\n> + *\n> + * \\var IspStatsRegion::rSum\n> + * \\brief Sum of the red values in the region\n> + *\n> + * \\var IspStatsRegion::gSum\n> + * \\brief Sum of the green values in the region\n> + *\n> + * \\var IspStatsRegion::bSum\n> + * \\brief Sum of the blue values in the region\n> + */\n> +struct IspStatsRegion {\n\nIs the 'Isp' prefix redundant here?\n\nWe generally prefer full names in the rest of the code base - so I think\nthis could end up being\n\nstruct StatisticsRegion\n\n> +\tunsigned int counted;\n> +\tunsigned int uncounted;\n> +\tunsigned long long rSum;\n> +\tunsigned long long gSum;\n> +\tunsigned long long bSum;\n> +};\n> +\n> +/**\n> + * \\struct AwbStatus\n\nThe name 'Status' sounds odd here to me.\n\nThe Status of the AWB would be 'converged' or something stateful\nwouldn't it?\n\nShould the output of an algorithm be the Results ?\n\ni.e. would this be better known as AwbResults ?\n\n\n> + * \\brief AWB parameters calculated\n> + *\n> + * The AwbStatus structure is intended to store the AWB\n> + * parameters calculated by the algorithm\n> + *\n> + * \\var AwbStatus::temperatureK\n> + * \\brief Color temperature calculated\n> + *\n> + * \\var AwbStatus::redGain\n> + * \\brief Gain calculated for the red channel\n> + *\n> + * \\var AwbStatus::greenGain\n> + * \\brief Gain calculated for the green channel\n> + *\n> + * \\var AwbStatus::blueGain\n> + * \\brief Gain calculated for the blue channel\n> + */\n> +struct AwbStatus {\n> +\tdouble temperatureK;\n> +\tdouble redGain;\n> +\tdouble greenGain;\n> +\tdouble blueGain;\n> +};\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_LIBIPA_ISP_H__ */\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 38B7FBD794\n\tfor ;\n\tWed, 7 Jul 2021 12:23:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A34EC68500;\n\tWed, 7 Jul 2021 14:23: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 8BE7560284\n\tfor ;\n\tWed, 7 Jul 2021 14:23:24 +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 271852E4;\n\tWed, 7 Jul 2021 14:23: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=\"c1XHN2ov\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625660604;\n\tbh=PSBQ16bG6bTkjw3ewaOj4YcE9Wv7XkWqtpXt1x32rPA=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=c1XHN2ovJ+z6IOazbgrDm4AALwNetY/dYLOJRbev+fwbHUJfprbKVovmOai9LaCyH\n\tbCQpG8Jj0iJyA/qOI6+4gNWBB9YtNym1YEKzDq5TbhNwJtcZb6z0AXehAK2lffonXo\n\td54ocXSwup26L0LfsTAJGXdssagX/RAyoho+LNRk=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210628202255.138874-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210628202255.138874-3-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"","Date":"Wed, 7 Jul 2021 13:23: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":"<20210628202255.138874-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 v1 2/7] ipa: libipa: Create a common\n\tISP header to store the structure types","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":18028,"web_url":"https://patchwork.libcamera.org/comment/18028/","msgid":"","date":"2021-07-08T11:36:10","subject":"Re: [libcamera-devel] [PATCH v1 2/7] ipa: libipa: Create a common\n\tISP header to store the structure types","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Wed, Jul 07, 2021 at 01:23:22PM +0100, Kieran Bingham wrote:\n> On 28/06/2021 21:22, Jean-Michel Hautbois wrote:\n> > Each ISP may use the same AWB or AGC algorithms. In order to avoid\n> > duplicated code, create a header with the main structures used for now.\n> \n> I'm not sure these are really 'isp' structures, and they are more\n> 'statistics' strutures.\n> \n> I'd be tempted to call this 'statistics.h' rather than isp.h ...\n\nAgreed, ISP is a misnommer here. 'statistics' is better, shortening it\nto 'stats' may be good in most (all ?) contexts to avoid too long names.\n\n> although - in fact one of the structures is the AwbStatus .. that's the\n> results of the algorithm isn't it ?\n> \n> I wonder if the common structures which are specific to an algorithm\n> should be broken out to their own algorithm specific header.\n> \n> I.e. anything common to all Awb should be libipa/awb.h ...\n\nAgreed too.\n\n> > Signed-off-by: Jean-Michel Hautbois \n> > ---\n> > src/ipa/ipu3/ipu3_agc.h | 1 +\n> > src/ipa/ipu3/ipu3_awb.h | 30 +----------\n> > src/ipa/libipa/isp.h | 110 ++++++++++++++++++++++++++++++++++++++++\n> > 3 files changed, 112 insertions(+), 29 deletions(-)\n> > create mode 100644 src/ipa/libipa/isp.h\n> > \n> > diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n> > index 3deca3ae..6ca9af8e 100644\n> > --- a/src/ipa/ipu3/ipu3_agc.h\n> > +++ b/src/ipa/ipu3/ipu3_agc.h\n> > @@ -17,6 +17,7 @@\n> > #include \n> > \n> > #include \"libipa/algorithm.h\"\n> > +#include \"libipa/isp.h\"\n> > \n> > namespace libcamera {\n> > \n> > diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h\n> > index 122cf68c..6ae111fd 100644\n> > --- a/src/ipa/ipu3/ipu3_awb.h\n> > +++ b/src/ipa/ipu3/ipu3_awb.h\n> > @@ -14,6 +14,7 @@\n> > #include \n> > \n> > #include \"libipa/algorithm.h\"\n> > +#include \"libipa/isp.h\"\n> > \n> > namespace libcamera {\n> > \n> > @@ -42,35 +43,6 @@ public:\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 generateZones(std::vector &zones);\n> > \tvoid generateAwbStats(const ipu3_uapi_stats_3a *stats);\n> > diff --git a/src/ipa/libipa/isp.h b/src/ipa/libipa/isp.h\n> > new file mode 100644\n> > index 00000000..a15803d6\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/isp.h\n> \n> I'm torn on the include locations too ...\n> \n> globally, I would have expected libcamera headers to be under the root\n> include/ so that would make this include/ipa/ or include/libipa - but I\n> really think these headers are better kept close to the libipa - and\n> they are not generating an externally visible library anyway, so perhaps\n> keeping them local is good for now.\n\nI would have expected include/ too. We already have headers in\nsrc/ipa/libipa/ so the issue shouldn't be addressed in this patch, but I\nexpect libipa to turn into a shared object that will be installed as\npart of libcamera, in which case headers should go to include/ (possibly\ninclude/libcamera/libipa/, TBD).\n\n> > @@ -0,0 +1,110 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Ideas On Board\n> > + *\n> > + * isp.h - ISP statistics interface\n> > + */\n> > +#ifndef __LIBCAMERA_IPA_LIBIPA_ISP_H__\n> > +#define __LIBCAMERA_IPA_LIBIPA_ISP_H__\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa {\n> > +/**\n> > + * \\struct RGB\n> > + * \\brief RGB\n\nAnd ? :-) What is this ? Is it a pixel ? An accumulator ? Why double\ninstead of fixed point ? All this needs to be explained.\n\n> > + *\n> > + * \\fn RGB::RGB\n> > + * \\brief Construct a RGB structure\n> > + * \\param[in] _R Red value to set, defaults to 0\n> > + * \\param[in] _G Green value to set, defaults to 0\n> > + * \\param[in] _B Blue value to set, defaults to 0\n> > + *\n> > + * \\var RGB::R\n> > + * \\brief Red value of the RGB structure\n> > + *\n> > + * \\var RGB::G\n> > + * \\brief Green value of the RGB structure\n> > + *\n> > + * \\var RGB::B\n> > + * \\brief Blue value of the RGB structure\n> > + *\n> > + * \\fn RGB &RGB::operator+=(RGB const &other)\n> > + * \\brief Add each RGB value to another RGB structure\n> > + * \\param[in] other An RGB structure\n> > + * \\return An RGB structure containing the added R, G and B values\n> > + */\n\nDoxygen documentation goes to .cpp files.\n\n> > +struct RGB {\n> > +\tRGB(double _R = 0, double _G = 0, double _B = 0)\n> > +\t\t: R(_R), G(_G), B(_B)\n\nVariables start with a lower-case letter, so that's r and _r (and so\non).\n\n> > +\t{\n> > +\t}\n> > +\tdouble R, G, B;\n\nBlank line before and after, and it should go after all the functions.\n\n> > +\tRGB &operator+=(RGB const &other)\n> > +\t{\n> > +\t\tR += other.R, G += other.G, B += other.B;\n\nOne statement per line.\n\n> > +\t\treturn *this;\n> > +\t}\n> > +};\n> > +\n> > +/**\n> > + * \\struct IspStatsRegion\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\nIt seems very, very ad-hoc, far from the level of genericity I would\nexpect from a generic type. Documentation needs to be expanded to\nexplain what this is.\n\n> > + *\n> > + * \\var IspStatsRegion::counted\n> > + * \\brief Number of pixels used to calculate the sums\n> > + *\n> > + * \\var IspStatsRegion::uncounted\n> > + * \\brief Remaining number of pixels in the region\n\nThose two are extremely unclear as well.\n\n> > + *\n> > + * \\var IspStatsRegion::rSum\n> > + * \\brief Sum of the red values in the region\n> > + *\n> > + * \\var IspStatsRegion::gSum\n> > + * \\brief Sum of the green values in the region\n> > + *\n> > + * \\var IspStatsRegion::bSum\n> > + * \\brief Sum of the blue values in the region\n> > + */\n> > +struct IspStatsRegion {\n> \n> Is the 'Isp' prefix redundant here?\n> \n> We generally prefer full names in the rest of the code base - so I think\n> this could end up being\n> \n> struct StatisticsRegion\n\nAgreed overall, but for stats, I think spelling it in full all the time\nwill be painful.\n\n> > +\tunsigned int counted;\n> > +\tunsigned int uncounted;\n> > +\tunsigned long long rSum;\n> > +\tunsigned long long gSum;\n> > +\tunsigned long long bSum;\n> > +};\n> > +\n> > +/**\n> > + * \\struct AwbStatus\n> \n> The name 'Status' sounds odd here to me.\n> \n> The Status of the AWB would be 'converged' or something stateful\n> wouldn't it?\n> \n> Should the output of an algorithm be the Results ?\n> \n> i.e. would this be better known as AwbResults ?\n> \n> > + * \\brief AWB parameters calculated\n> > + *\n> > + * The AwbStatus structure is intended to store the AWB\n> > + * parameters calculated by the algorithm\n> > + *\n> > + * \\var AwbStatus::temperatureK\n> > + * \\brief Color temperature calculated\n> > + *\n> > + * \\var AwbStatus::redGain\n> > + * \\brief Gain calculated for the red channel\n> > + *\n> > + * \\var AwbStatus::greenGain\n> > + * \\brief Gain calculated for the green channel\n> > + *\n> > + * \\var AwbStatus::blueGain\n> > + * \\brief Gain calculated for the blue channel\n> > + */\n> > +struct AwbStatus {\n> > +\tdouble temperatureK;\n> > +\tdouble redGain;\n> > +\tdouble greenGain;\n> > +\tdouble blueGain;\n> > +};\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_IPA_LIBIPA_ISP_H__ */","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 D0FB1C3224\n\tfor ;\n\tThu, 8 Jul 2021 11:36:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 460D068513;\n\tThu, 8 Jul 2021 13:36:58 +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 ED31268506\n\tfor ;\n\tThu, 8 Jul 2021 13:36:55 +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 4101EE7;\n\tThu, 8 Jul 2021 13:36:55 +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=\"KztS+PSN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625744215;\n\tbh=gtKjHktbXmZtyy39PwKbl0ycn7wAiPy3iAHe8smpkLU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KztS+PSNXDMywW+n9z8TBPtaGvhq7IbXc5QeDtp293vnSG30Y59Vxm8bj4Kh86XH7\n\tc50R4ljbKxiAaChqGsPo3X5w0U3W/xIb//wgVm2xv4QuLpUC20BOqen1yyvhbrKSBc\n\tpe0G9b6dEMPpvtCA8SIrg21YMGnTrzUiFKsBSPtM=","Date":"Thu, 8 Jul 2021 14:36:10 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210628202255.138874-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210628202255.138874-3-jeanmichel.hautbois@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v1 2/7] ipa: libipa: Create a common\n\tISP header to store the structure types","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":18038,"web_url":"https://patchwork.libcamera.org/comment/18038/","msgid":"","date":"2021-07-09T06:15:38","subject":"Re: [libcamera-devel] [PATCH v1 2/7] ipa: libipa: Create a common\n\tISP header to store the structure types","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi !\n\nOn 08/07/2021 13:36, Laurent Pinchart wrote:\n> Hello,\n> \n> On Wed, Jul 07, 2021 at 01:23:22PM +0100, Kieran Bingham wrote:\n>> On 28/06/2021 21:22, Jean-Michel Hautbois wrote:\n>>> Each ISP may use the same AWB or AGC algorithms. In order to avoid\n>>> duplicated code, create a header with the main structures used for now.\n>>\n>> I'm not sure these are really 'isp' structures, and they are more\n>> 'statistics' strutures.\n>>\n>> I'd be tempted to call this 'statistics.h' rather than isp.h ...\n> \n> Agreed, ISP is a misnommer here. 'statistics' is better, shortening it\n> to 'stats' may be good in most (all ?) contexts to avoid too long names.\n> \n\nOK :-).\n\n>> although - in fact one of the structures is the AwbStatus .. that's the\n>> results of the algorithm isn't it ?\n>>\n>> I wonder if the common structures which are specific to an algorithm\n>> should be broken out to their own algorithm specific header.\n>>\n>> I.e. anything common to all Awb should be libipa/awb.h ...\n> \n> Agreed too.\n> \n\nThose structures will be used with the Metadata patch, as each algo (and\nIPAIPU3) needs to know the layout of the data (passing the awb results\nfrom awb to agc is using this AwbStatus).\nThis patch will be moved after the Metadata introduction in v2 and it\nshould have more sense then ;-).\n\n>>> Signed-off-by: Jean-Michel Hautbois \n>>> ---\n>>> src/ipa/ipu3/ipu3_agc.h | 1 +\n>>> src/ipa/ipu3/ipu3_awb.h | 30 +----------\n>>> src/ipa/libipa/isp.h | 110 ++++++++++++++++++++++++++++++++++++++++\n>>> 3 files changed, 112 insertions(+), 29 deletions(-)\n>>> create mode 100644 src/ipa/libipa/isp.h\n>>>\n>>> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h\n>>> index 3deca3ae..6ca9af8e 100644\n>>> --- a/src/ipa/ipu3/ipu3_agc.h\n>>> +++ b/src/ipa/ipu3/ipu3_agc.h\n>>> @@ -17,6 +17,7 @@\n>>> #include \n>>> \n>>> #include \"libipa/algorithm.h\"\n>>> +#include \"libipa/isp.h\"\n>>> \n>>> namespace libcamera {\n>>> \n>>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h\n>>> index 122cf68c..6ae111fd 100644\n>>> --- a/src/ipa/ipu3/ipu3_awb.h\n>>> +++ b/src/ipa/ipu3/ipu3_awb.h\n>>> @@ -14,6 +14,7 @@\n>>> #include \n>>> \n>>> #include \"libipa/algorithm.h\"\n>>> +#include \"libipa/isp.h\"\n>>> \n>>> namespace libcamera {\n>>> \n>>> @@ -42,35 +43,6 @@ public:\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 generateZones(std::vector &zones);\n>>> \tvoid generateAwbStats(const ipu3_uapi_stats_3a *stats);\n>>> diff --git a/src/ipa/libipa/isp.h b/src/ipa/libipa/isp.h\n>>> new file mode 100644\n>>> index 00000000..a15803d6\n>>> --- /dev/null\n>>> +++ b/src/ipa/libipa/isp.h\n>>\n>> I'm torn on the include locations too ...\n>>\n>> globally, I would have expected libcamera headers to be under the root\n>> include/ so that would make this include/ipa/ or include/libipa - but I\n>> really think these headers are better kept close to the libipa - and\n>> they are not generating an externally visible library anyway, so perhaps\n>> keeping them local is good for now.\n> \n> I would have expected include/ too. We already have headers in\n> src/ipa/libipa/ so the issue shouldn't be addressed in this patch, but I\n> expect libipa to turn into a shared object that will be installed as\n> part of libcamera, in which case headers should go to include/ (possibly\n> include/libcamera/libipa/, TBD).\n> \n\nTODO: split the headers into include/ :-)\n\n>>> @@ -0,0 +1,110 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Copyright (C) 2021, Ideas On Board\n>>> + *\n>>> + * isp.h - ISP statistics interface\n>>> + */\n>>> +#ifndef __LIBCAMERA_IPA_LIBIPA_ISP_H__\n>>> +#define __LIBCAMERA_IPA_LIBIPA_ISP_H__\n>>> +\n>>> +namespace libcamera {\n>>> +\n>>> +namespace ipa {\n>>> +/**\n>>> + * \\struct RGB\n>>> + * \\brief RGB\n> \n> And ? :-) What is this ? Is it a pixel ? An accumulator ? Why double\n> instead of fixed point ? All this needs to be explained.\n> \n>>> + *\n>>> + * \\fn RGB::RGB\n>>> + * \\brief Construct a RGB structure\n>>> + * \\param[in] _R Red value to set, defaults to 0\n>>> + * \\param[in] _G Green value to set, defaults to 0\n>>> + * \\param[in] _B Blue value to set, defaults to 0\n>>> + *\n>>> + * \\var RGB::R\n>>> + * \\brief Red value of the RGB structure\n>>> + *\n>>> + * \\var RGB::G\n>>> + * \\brief Green value of the RGB structure\n>>> + *\n>>> + * \\var RGB::B\n>>> + * \\brief Blue value of the RGB structure\n>>> + *\n>>> + * \\fn RGB &RGB::operator+=(RGB const &other)\n>>> + * \\brief Add each RGB value to another RGB structure\n>>> + * \\param[in] other An RGB structure\n>>> + * \\return An RGB structure containing the added R, G and B values\n>>> + */\n> \n> Doxygen documentation goes to .cpp files.\n> \n>>> +struct RGB {\n>>> +\tRGB(double _R = 0, double _G = 0, double _B = 0)\n>>> +\t\t: R(_R), G(_G), B(_B)\n> \n> Variables start with a lower-case letter, so that's r and _r (and so\n> on).\n> \n>>> +\t{\n>>> +\t}\n>>> +\tdouble R, G, B;\n> \n> Blank line before and after, and it should go after all the functions.\n> \n>>> +\tRGB &operator+=(RGB const &other)\n>>> +\t{\n>>> +\t\tR += other.R, G += other.G, B += other.B;\n> \n> One statement per line.\n> \n>>> +\t\treturn *this;\n>>> +\t}\n>>> +};\n>>> +\n>>> +/**\n>>> + * \\struct IspStatsRegion\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> \n> It seems very, very ad-hoc, far from the level of genericity I would\n> expect from a generic type. Documentation needs to be expanded to\n> explain what this is.\n> \n\nTODO: documentation, documentation, documentation and documentation !\n\n>>> + *\n>>> + * \\var IspStatsRegion::counted\n>>> + * \\brief Number of pixels used to calculate the sums\n>>> + *\n>>> + * \\var IspStatsRegion::uncounted\n>>> + * \\brief Remaining number of pixels in the region\n> \n> Those two are extremely unclear as well.\n> \n>>> + *\n>>> + * \\var IspStatsRegion::rSum\n>>> + * \\brief Sum of the red values in the region\n>>> + *\n>>> + * \\var IspStatsRegion::gSum\n>>> + * \\brief Sum of the green values in the region\n>>> + *\n>>> + * \\var IspStatsRegion::bSum\n>>> + * \\brief Sum of the blue values in the region\n>>> + */\n>>> +struct IspStatsRegion {\n>>\n>> Is the 'Isp' prefix redundant here?\n>>\n>> We generally prefer full names in the rest of the code base - so I think\n>> this could end up being\n>>\n>> struct StatisticsRegion\n> \n> Agreed overall, but for stats, I think spelling it in full all the time\n> will be painful.\n> \n>>> +\tunsigned int counted;\n>>> +\tunsigned int uncounted;\n>>> +\tunsigned long long rSum;\n>>> +\tunsigned long long gSum;\n>>> +\tunsigned long long bSum;\n>>> +};\n>>> +\n>>> +/**\n>>> + * \\struct AwbStatus\n>>\n>> The name 'Status' sounds odd here to me.\n>>\n>> The Status of the AWB would be 'converged' or something stateful\n>> wouldn't it?\n>>\n>> Should the output of an algorithm be the Results ?\n>>\n>> i.e. would this be better known as AwbResults ?\n>>\n\nI must confess I used the name from RPi :-).\nIt is indeed a status, and can be used to store previous gains and\ncurrent gains.\nIn the RPi awb algorithm (we don't have that part yet) the results are\nstore in prev_sync_results_, sync_results_ and async_results_ and each\nof them is rolling between calls.\nprev_sync_results_ is a filtered sync_results_ through an IIR and\nsync_results_ will have a new value at the next frame, calculated in\nasync_results_ :-).\n\nIn Awb::Prepare() the gains are set in the metadata object with:\nimage_metadata->Set(\"awb.status\", prev_sync_results_);\n\nThe structure can be renamed AwbResults though ;-) as all the variables\nare results... :-).\n\n>>> + * \\brief AWB parameters calculated\n>>> + *\n>>> + * The AwbStatus structure is intended to store the AWB\n>>> + * parameters calculated by the algorithm\n>>> + *\n>>> + * \\var AwbStatus::temperatureK\n>>> + * \\brief Color temperature calculated\n>>> + *\n>>> + * \\var AwbStatus::redGain\n>>> + * \\brief Gain calculated for the red channel\n>>> + *\n>>> + * \\var AwbStatus::greenGain\n>>> + * \\brief Gain calculated for the green channel\n>>> + *\n>>> + * \\var AwbStatus::blueGain\n>>> + * \\brief Gain calculated for the blue channel\n>>> + */\n>>> +struct AwbStatus {\n>>> +\tdouble temperatureK;\n>>> +\tdouble redGain;\n>>> +\tdouble greenGain;\n>>> +\tdouble blueGain;\n>>> +};\n>>> +\n>>> +} /* namespace ipa */\n>>> +\n>>> +} /* namespace libcamera */\n>>> +\n>>> +#endif /* __LIBCAMERA_IPA_LIBIPA_ISP_H__ */\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 23ADDBD794\n\tfor ;\n\tFri, 9 Jul 2021 06:15:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8E18C6851F;\n\tFri, 9 Jul 2021 08:15:42 +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 516EE6059F\n\tfor ;\n\tFri, 9 Jul 2021 08:15:41 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:d47a:9456:f481:a0f8])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C810AE7;\n\tFri, 9 Jul 2021 08:15:40 +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=\"AZCOO8Ct\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625811340;\n\tbh=Ffp+6Krs7VnKjtQ3HMsVMgZ28dDA0N9rGy0sKZUZQf4=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=AZCOO8CtW8dGciU3IHu+ppCeDTAIx0UaPyTNANT0L8/dnhjVFVg8A8vp0sk4KNONa\n\tJ5xwD1bWeISJaWCsof8+V3XEpxiEEFKPH4UHfYJPfJG+65sJn0zdXv5D1V6LOUcnjO\n\tvD12ZN7wdPbw8zKIaXI2xV52WvMjfIrMUZ0817VE=","To":"Laurent Pinchart ,\n\tKieran Bingham ","References":"<20210628202255.138874-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210628202255.138874-3-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t","From":"Jean-Michel Hautbois ","Message-ID":"","Date":"Fri, 9 Jul 2021 08:15:38 +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 v1 2/7] ipa: libipa: Create a common\n\tISP header to store the structure types","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\" "}}]