[{"id":21365,"web_url":"https://patchwork.libcamera.org/comment/21365/","msgid":"<7d861314-b2af-19b6-b400-fa629941cbac@ideasonboard.com>","date":"2021-11-29T17:23:11","subject":"Re: [libcamera-devel] [PATCH v5 06/11] ipa: libipa: Introduce\n\tAlgorithm class template","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi JM,\n\nPatches seems okay,\n\nOn 11/25/21 11:12 AM, Jean-Michel Hautbois wrote:\n> The algorithms are using the same function names with specialized\n> parameters. Instead of duplicating code, introduce a libipa Algorithm\n> class which implements a base class with template parameters in libipa,\n> and use it in each IPA.\n>\n> As we now won't need an algorithm class for each IPA, move the\n> documentation to libipa, and make it agnostic of the IPA used. While at\n> it, fix the IPU3::Algorithm::Awb documentation.\n>\n> Signed-off-by: Laurent Pinchart \n> Signed-off-by: Jean-Michel Hautbois \n> Reviewed-by: Kieran Bingham \n>\n> ---\n> v4: use #pragma once\n> ---\n> src/ipa/ipu3/algorithms/algorithm.h | 12 ++----\n> src/ipa/ipu3/algorithms/awb.cpp | 9 +++++\n> src/ipa/ipu3/algorithms/meson.build | 1 -\n> .../{ipu3/algorithms => libipa}/algorithm.cpp | 38 ++++++++-----------\n> src/ipa/libipa/algorithm.h | 38 +++++++++++++++++++\n> src/ipa/libipa/meson.build | 1 +\n> 6 files changed, 67 insertions(+), 32 deletions(-)\n> rename src/ipa/{ipu3/algorithms => libipa}/algorithm.cpp (75%)\n> create mode 100644 src/ipa/libipa/algorithm.h\n>\n> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h\n> index 16310ab1..d2eecc78 100644\n> --- a/src/ipa/ipu3/algorithms/algorithm.h\n> +++ b/src/ipa/ipu3/algorithms/algorithm.h\n> @@ -9,21 +9,15 @@\n> \n> #include \n> \n> +#include \n> +\n> #include \"ipa_context.h\"\n> \n> namespace libcamera {\n> \n> namespace ipa::ipu3 {\n> \n> -class Algorithm\n> -{\n> -public:\n> -\tvirtual ~Algorithm() {}\n> -\n> -\tvirtual int configure(IPAContext &context, const IPAConfigInfo &configInfo);\n> -\tvirtual void prepare(IPAContext &context, ipu3_uapi_params *params);\n> -\tvirtual void process(IPAContext &context, const ipu3_uapi_stats_3a *stats);\n> -};\n> +using Algorithm = libcamera::ipa::Algorithm;\n> \n> } /* namespace ipa::ipu3 */\n> \n> diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp\n> index c7bcb20e..1dc27fc9 100644\n> --- a/src/ipa/ipu3/algorithms/awb.cpp\n> +++ b/src/ipa/ipu3/algorithms/awb.cpp\n> @@ -193,6 +193,9 @@ Awb::Awb()\n> \n> Awb::~Awb() = default;\n> \n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::configure\n> + */\n> int Awb::configure(IPAContext &context,\n> \t\t [[maybe_unused]] const IPAConfigInfo &configInfo)\n> {\n> @@ -373,6 +376,9 @@ void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)\n> \t}\n> }\n> \n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::process\n> + */\n> void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)\n> {\n> \tcalculateWBGains(stats);\n> @@ -394,6 +400,9 @@ constexpr uint16_t Awb::threshold(float value)\n> \treturn value * 8191;\n> }\n> \n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::prepare\n> + */\n> void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)\n> {\n> \t/*\n> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build\n> index 3ec42f72..4db6ae1d 100644\n> --- a/src/ipa/ipu3/algorithms/meson.build\n> +++ b/src/ipa/ipu3/algorithms/meson.build\n> @@ -2,7 +2,6 @@\n> \n> ipu3_ipa_algorithms = files([\n> 'agc.cpp',\n> - 'algorithm.cpp',\n> 'awb.cpp',\n> 'blc.cpp',\n> 'tone_mapping.cpp',\n> diff --git a/src/ipa/ipu3/algorithms/algorithm.cpp b/src/ipa/libipa/algorithm.cpp\n> similarity index 75%\n> rename from src/ipa/ipu3/algorithms/algorithm.cpp\n> rename to src/ipa/libipa/algorithm.cpp\n> index 3e7e3018..398d5372 100644\n> --- a/src/ipa/ipu3/algorithms/algorithm.cpp\n> +++ b/src/ipa/libipa/algorithm.cpp\n> @@ -2,7 +2,7 @@\n> /*\n> * Copyright (C) 2021, Ideas On Board\n> *\n> - * algorithm.cpp - IPU3 control algorithm interface\n> + * algorithm.cpp - IPA control algorithm interface\n> */\n> \n> #include \"algorithm.h\"\n> @@ -14,11 +14,15 @@\n> \n> namespace libcamera {\n> \n> -namespace ipa::ipu3 {\n> +namespace ipa {\n> \n> /**\n> * \\class Algorithm\n> - * \\brief The base class for all IPU3 algorithms\n> + * \\brief The base class for all IPA algorithms\n> + * \\tparam Context The type of shared IPA context\n> + * \\tparam Config The type of the IPA configuration data\n> + * \\tparam Params The type of the ISP specific parameters\n> + * \\tparam Stats The type of the IPA statistics and ISP results\n> *\n> * The Algorithm class defines a standard interface for IPA algorithms. By\n> * abstracting algorithms, it makes possible the implementation of generic code\n> @@ -26,6 +30,7 @@ namespace ipa::ipu3 {\n> */\n> \n> /**\n> + * \\fn Algorithm::configure()\n> * \\brief Configure the Algorithm given an IPAConfigInfo\n> * \\param[in] context The shared IPA context\n> * \\param[in] configInfo The IPA configuration data, received from the pipeline\n> @@ -39,37 +44,30 @@ namespace ipa::ipu3 {\n> *\n> * \\return 0 if successful, an error code otherwise\n> */\n> -int Algorithm::configure([[maybe_unused]] IPAContext &context,\n> -\t\t\t [[maybe_unused]] const IPAConfigInfo &configInfo)\n> -{\n> -\treturn 0;\n> -}\n> \n> /**\n> + * \\fn Algorithm::prepare()\n> * \\brief Fill the \\a params buffer with ISP processing parameters for a frame\n> * \\param[in] context The shared IPA context\n> - * \\param[out] params The IPU3 specific parameters.\n> + * \\param[out] params The ISP specific parameters.\n> *\n> * This function is called for every frame when the camera is running before it\n> - * is processed by the ImgU to prepare the ImgU processing parameters for that\n> + * is processed by the ISP to prepare the ISP processing parameters for that\n> * frame.\n> *\n> * Algorithms shall fill in the parameter structure fields appropriately to\n> - * configure the ImgU processing blocks that they are responsible for. This\n> + * configure the ISP processing blocks that they are responsible for. This\n> * includes setting fields and flags that enable those processing blocks.\n> */\n> -void Algorithm::prepare([[maybe_unused]] IPAContext &context,\n> -\t\t\t[[maybe_unused]] ipu3_uapi_params *params)\n> -{\n> -}\n> \n> /**\n> + * \\fn Algorithm::process()\n> * \\brief Process ISP statistics, and run algorithm operations\n> * \\param[in] context The shared IPA context\n> - * \\param[in] stats The IPU3 statistics and ISP results\n> + * \\param[in] stats The IPA statistics and ISP results\n> *\n> * This function is called while camera is running for every frame processed by\n> - * the ImgU, to process statistics generated from that frame by the ImgU.\n> + * the ISP, to process statistics generated from that frame by the ISP.\n> * Algorithms shall use this data to run calculations and update their state\n> * accordingly.\n> *\n> @@ -91,11 +89,7 @@ void Algorithm::prepare([[maybe_unused]] IPAContext &context,\n> * Care shall be taken to ensure the ordering of access to the information\n> * such that the algorithms use up to date state as required.\n> */\n> -void Algorithm::process([[maybe_unused]] IPAContext &context,\n> -\t\t\t[[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n> -{\n> -}\n> \n> -} /* namespace ipa::ipu3 */\n> +} /* namespace ipa */\n> \n> } /* namespace libcamera */\n> diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h\n> new file mode 100644\n> index 00000000..766aee5d\n> --- /dev/null\n> +++ b/src/ipa/libipa/algorithm.h\n> @@ -0,0 +1,38 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Ideas On Board\n\n\nI think Oy is missing here at the end, that;s what I see in other \nsamples in the code base\n\nReviewed-by: Umang Jain \n\n> + *\n> + * algorithm.h - ISP control algorithm interface\n> + */\n> +#pragma once\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +template\n> +class Algorithm\n> +{\n> +public:\n> +\tvirtual ~Algorithm() {}\n> +\n> +\tvirtual int configure([[maybe_unused]] Context &context,\n> +\t\t\t [[maybe_unused]] const Config &configInfo)\n> +\t{\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tvirtual void prepare([[maybe_unused]] Context &context,\n> +\t\t\t [[maybe_unused]] Params *params)\n> +\t{\n> +\t}\n> +\n> +\tvirtual void process([[maybe_unused]] Context &context,\n> +\t\t\t [[maybe_unused]] const Stats *stats)\n> +\t{\n> +\t}\n> +};\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 4d073a03..161cc5a1 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -1,6 +1,7 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> libipa_headers = files([\n> + 'algorithm.h',\n> 'camera_sensor_helper.h',\n> 'histogram.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 88E16BF415\n\tfor ;\n\tMon, 29 Nov 2021 17:23:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2CEFF605A4;\n\tMon, 29 Nov 2021 18:23:18 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C4CDB60592\n\tfor ;\n\tMon, 29 Nov 2021 18:23:16 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.170])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A3A8B2A5;\n\tMon, 29 Nov 2021 18:23:15 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"V1gaL+zv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638206596;\n\tbh=jhhgmQJIt7SwMMeAVeky3/9zkXoKdAVROBuEl3liUw8=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=V1gaL+zvbDNwfa9vW/jf5bydor0rfxpfoFOfObNh8KWoP94793aZNTxNaEKYxYZzO\n\tictGG2b0gQj3wznqxVFi5gL2OB3wJWOyVmrvV02zJufgYwtgB9LpxSEKBEC288YrK0\n\tDqgelS/w6Gl4mVqBu6icn5BDzZUgpg0pPSrrSGWA=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211125054259.24792-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211125054259.24792-7-jeanmichel.hautbois@ideasonboard.com>","From":"Umang Jain ","Message-ID":"<7d861314-b2af-19b6-b400-fa629941cbac@ideasonboard.com>","Date":"Mon, 29 Nov 2021 22:53:11 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20211125054259.24792-7-jeanmichel.hautbois@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v5 06/11] ipa: libipa: Introduce\n\tAlgorithm class template","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\" "}}]