[{"id":21043,"web_url":"https://patchwork.libcamera.org/comment/21043/","msgid":"","date":"2021-11-19T13:22:38","subject":"Re: [libcamera-devel] [PATCH v1 4/8] ipa: ipu3: Introduce the\n\tAlgorithm class","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, Nov 19, 2021 at 12:16:50PM +0100, Jean-Michel Hautbois wrote:\n> Now that everything is in place, introduce the Algorithm class and\n> declare it in IPA::RkISP1. There is no functional change yet.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/rkisp1/algorithms/algorithm.cpp | 101 ++++++++++++++++++++++++\n> src/ipa/rkisp1/algorithms/algorithm.h | 32 ++++++++\n> src/ipa/rkisp1/algorithms/meson.build | 5 ++\n> src/ipa/rkisp1/meson.build | 3 +\n> src/ipa/rkisp1/rkisp1.cpp | 5 +-\n> 5 files changed, 145 insertions(+), 1 deletion(-)\n> create mode 100644 src/ipa/rkisp1/algorithms/algorithm.cpp\n> create mode 100644 src/ipa/rkisp1/algorithms/algorithm.h\n> create mode 100644 src/ipa/rkisp1/algorithms/meson.build\n> \n> diff --git a/src/ipa/rkisp1/algorithms/algorithm.cpp b/src/ipa/rkisp1/algorithms/algorithm.cpp\n> new file mode 100644\n> index 00000000..be3cd9f6\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/algorithm.cpp\n> @@ -0,0 +1,101 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Ideas On Board\n> + *\n> + * algorithm.cpp - RkISP1 control algorithm interface\n> + */\n> +\n> +#include \"algorithm.h\"\n> +\n> +/**\n> + * \\file algorithm.h\n> + * \\brief Algorithm common interface\n> + */\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1 {\n> +\n> +/**\n> + * \\class Algorithm\n> + * \\brief The base class for all RkISP1 algorithms\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> + * to manage algorithms regardless of their specific type.\n> + */\n> +\n> +/**\n> + * \\brief Configure the Algorithm given an IPAConfigInfo\n> + * \\param[in] context The shared IPA context\n> + * \\param[in] IPACameraSensorInfo The IPA configuration data, received from the\n> + * pipeline handler\n> + *\n> + * Algorithms may implement a configure operation to pre-calculate\n> + * parameters prior to commencing streaming.\n> + *\n> + * Configuration state may be stored in the IPASessionConfiguration structure of\n> + * the IPAContext.\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 IPACameraSensorInfo &configInfo)\n> +{\n> +\treturn 0;\n> +}\n> +\n> +/**\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 RkISP1 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> + * 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> + * 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]] rkisp1_params_cfg *params)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Process ISP statistics, and run algorithm operations\n> + * \\param[in] context The shared IPA context\n> + * \\param[in] stats The RkISP1 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> + * Algorithms shall use this data to run calculations and update their state\n> + * accordingly.\n> + *\n> + * Processing shall not take an undue amount of time, and any extended or\n> + * computationally expensive calculations or operations must be handled\n> + * asynchronously in a separate thread.\n> + *\n> + * Algorithms can store state in their respective IPAFrameContext structures,\n> + * and reference state from the IPAFrameContext of other algorithms.\n> + *\n> + * \\todo Historical data may be required as part of the processing.\n> + * Either the previous frame, or the IPAFrameContext state of the frame\n> + * that generated the statistics for this operation may be required for\n> + * some advanced algorithms to prevent oscillations or support control\n> + * loops correctly. Only a single IPAFrameContext is available currently,\n> + * and so any data stored may represent the results of the previously\n> + * completed operations.\n> + *\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 rkisp1_stat_buffer *stats)\n> +{\n> +}\n> +\n> +} /* namespace ipa::rkisp1 */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h\n> new file mode 100644\n> index 00000000..5365a860\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/algorithm.h\n> @@ -0,0 +1,32 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Ideas On Board\n> + *\n> + * algorithm.h - RkISP1 control algorithm interface\n> + */\n> +#ifndef __LIBCAMERA_IPA_RKISP1_ALGORITHM_H__\n> +#define __LIBCAMERA_IPA_RKISP1_ALGORITHM_H__\n> +\n> +#include \n> +\n> +#include \"ipa_context.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1 {\n> +\n> +class Algorithm\n> +{\n> +public:\n> +\tvirtual ~Algorithm() {}\n> +\n> +\tvirtual int configure(IPAContext &context, const IPACameraSensorInfo &configInfo);\n> +\tvirtual void prepare(IPAContext &context, rkisp1_params_cfg *params);\n> +\tvirtual void process(IPAContext &context, const rkisp1_stat_buffer *stats);\n> +};\n> +\n> +} /* namespace ipa::rkisp1 */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_RKISP1_ALGORITHM_H__ */\n\nThis is all very similar to the IPU3 Algorithm class. The header file\ndoesn't bother me that much, but the duplicated documentation is\nannoying.\n\nWe obviously can't share the same class between the two IPA modules, but\nI wonder if we could get away with a class template in libipa:\n\nnamespace libcamera {\n\nnamespace ipa {\n\ntemplate\nclass Algorithm\n{\npublic:\n\tvirtual ~Algorithm() {}\n\n\tvirtual int configure(Context &context, const IPACameraSensorInfo &configInfo);\n\tvirtual void prepare(Context &context, Params *params);\n\tvirtual void process(Context &context, const Stats *stats);\n};\n\n} /* namespace ipa */\n\n} /* namespace libcamera */\n\nThen, in src/ipa/rkisp1/algorithms/algorithm.h, we could have\n\nnamespace libcamera {\n\nnamespace ipa::rkisp1 {\n\nusing Algorithm = libcamera::ipa::Algorithm;\n\n} /* namespace ipa::rkisp1 */\n\n} /* namespace libcamera */\n\nGood/bad idea ?\n\n> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> new file mode 100644\n> index 00000000..d98f77e2\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/meson.build\n> @@ -0,0 +1,5 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +rkisp1_ipa_algorithms = files([\n> + 'algorithm.cpp',\n> +])\n> diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build\n> index 3683c922..a6a4856e 100644\n> --- a/src/ipa/rkisp1/meson.build\n> +++ b/src/ipa/rkisp1/meson.build\n> @@ -1,4 +1,5 @@\n> # SPDX-License-Identifier: CC0-1.0\n\nMissing blank line.\n\n> +subdir('algorithms')\n> \n> ipa_name = 'ipa_rkisp1'\n> \n> @@ -7,6 +8,8 @@ rkisp1_ipa_sources = files([\n> 'rkisp1.cpp',\n> ])\n> \n> +rkisp1_ipa_sources += rkisp1_ipa_algorithms\n> +\n> mod = shared_module(ipa_name,\n> [rkisp1_ipa_sources, libcamera_generated_ipa_headers],\n> name_prefix : '',\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index e0933e22..ddddd52d 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -25,7 +25,7 @@\n> \n> #include \n> \n> -#include \"ipa_context.h\"\n> +#include \"algorithms/algorithm.h\"\n> #include \"libipa/camera_sensor_helper.h\"\n> \n> namespace libcamera {\n> @@ -82,6 +82,9 @@ private:\n> \n> \t/* Local parameter storage */\n> \tstruct IPAContext context_;\n> +\n> +\t/* Maintain the algorithms used by the IPA */\n> +\tstd::list> algorithms_;\n> };\n> \n> int IPARkISP1::init([[maybe_unused]] const IPASettings &settings,","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 8E255BF415\n\tfor ;\n\tFri, 19 Nov 2021 13:23:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3679260371;\n\tFri, 19 Nov 2021 14:23:02 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 21E70600B5\n\tfor ;\n\tFri, 19 Nov 2021 14:23:01 +0100 (CET)","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 AEFAE1959;\n\tFri, 19 Nov 2021 14:23:00 +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=\"mF3gDfjW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637328180;\n\tbh=hEQvAXM6nJWYpRV0ZLaVfwBVaVOTdxgPF0yYYdDaIGg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mF3gDfjWF5CQkSYv52NWmSNnSBuXxfnlO9g6etfsCA/d5Anmh6XJhfHLHTU85xNTG\n\tqmmg3JPLAXu4aS9JIj5vSMQscf9XDHnW6FAHIV6iy7C1xbkaAJjjFNssMqPVQaPula\n\twLYE8orq4P27QQ08cNGEJtyRoOgmjJaRQYLv9UtM=","Date":"Fri, 19 Nov 2021 15:22:38 +0200","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20211119111654.68445-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211119111654.68445-5-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211119111654.68445-5-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v1 4/8] ipa: ipu3: Introduce the\n\tAlgorithm class","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":21100,"web_url":"https://patchwork.libcamera.org/comment/21100/","msgid":"<1e07cfe5-3f35-e1fa-00e1-886ce10def48@ideasonboard.com>","date":"2021-11-22T14:18:48","subject":"Re: [libcamera-devel] [PATCH v1 4/8] ipa: ipu3: Introduce the\n\tAlgorithm class","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 19/11/2021 14:22, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> Thank you for the patch.\n> \n> On Fri, Nov 19, 2021 at 12:16:50PM +0100, Jean-Michel Hautbois wrote:\n>> Now that everything is in place, introduce the Algorithm class and\n>> declare it in IPA::RkISP1. There is no functional change yet.\n>>\n>> Signed-off-by: Jean-Michel Hautbois \n>> ---\n>> src/ipa/rkisp1/algorithms/algorithm.cpp | 101 ++++++++++++++++++++++++\n>> src/ipa/rkisp1/algorithms/algorithm.h | 32 ++++++++\n>> src/ipa/rkisp1/algorithms/meson.build | 5 ++\n>> src/ipa/rkisp1/meson.build | 3 +\n>> src/ipa/rkisp1/rkisp1.cpp | 5 +-\n>> 5 files changed, 145 insertions(+), 1 deletion(-)\n>> create mode 100644 src/ipa/rkisp1/algorithms/algorithm.cpp\n>> create mode 100644 src/ipa/rkisp1/algorithms/algorithm.h\n>> create mode 100644 src/ipa/rkisp1/algorithms/meson.build\n>>\n>> diff --git a/src/ipa/rkisp1/algorithms/algorithm.cpp b/src/ipa/rkisp1/algorithms/algorithm.cpp\n>> new file mode 100644\n>> index 00000000..be3cd9f6\n>> --- /dev/null\n>> +++ b/src/ipa/rkisp1/algorithms/algorithm.cpp\n>> @@ -0,0 +1,101 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Ideas On Board\n>> + *\n>> + * algorithm.cpp - RkISP1 control algorithm interface\n>> + */\n>> +\n>> +#include \"algorithm.h\"\n>> +\n>> +/**\n>> + * \\file algorithm.h\n>> + * \\brief Algorithm common interface\n>> + */\n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa::rkisp1 {\n>> +\n>> +/**\n>> + * \\class Algorithm\n>> + * \\brief The base class for all RkISP1 algorithms\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>> + * to manage algorithms regardless of their specific type.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Configure the Algorithm given an IPAConfigInfo\n>> + * \\param[in] context The shared IPA context\n>> + * \\param[in] IPACameraSensorInfo The IPA configuration data, received from the\n>> + * pipeline handler\n>> + *\n>> + * Algorithms may implement a configure operation to pre-calculate\n>> + * parameters prior to commencing streaming.\n>> + *\n>> + * Configuration state may be stored in the IPASessionConfiguration structure of\n>> + * the IPAContext.\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 IPACameraSensorInfo &configInfo)\n>> +{\n>> +\treturn 0;\n>> +}\n>> +\n>> +/**\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 RkISP1 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>> + * 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>> + * 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]] rkisp1_params_cfg *params)\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\brief Process ISP statistics, and run algorithm operations\n>> + * \\param[in] context The shared IPA context\n>> + * \\param[in] stats The RkISP1 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>> + * Algorithms shall use this data to run calculations and update their state\n>> + * accordingly.\n>> + *\n>> + * Processing shall not take an undue amount of time, and any extended or\n>> + * computationally expensive calculations or operations must be handled\n>> + * asynchronously in a separate thread.\n>> + *\n>> + * Algorithms can store state in their respective IPAFrameContext structures,\n>> + * and reference state from the IPAFrameContext of other algorithms.\n>> + *\n>> + * \\todo Historical data may be required as part of the processing.\n>> + * Either the previous frame, or the IPAFrameContext state of the frame\n>> + * that generated the statistics for this operation may be required for\n>> + * some advanced algorithms to prevent oscillations or support control\n>> + * loops correctly. Only a single IPAFrameContext is available currently,\n>> + * and so any data stored may represent the results of the previously\n>> + * completed operations.\n>> + *\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 rkisp1_stat_buffer *stats)\n>> +{\n>> +}\n>> +\n>> +} /* namespace ipa::rkisp1 */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h\n>> new file mode 100644\n>> index 00000000..5365a860\n>> --- /dev/null\n>> +++ b/src/ipa/rkisp1/algorithms/algorithm.h\n>> @@ -0,0 +1,32 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Ideas On Board\n>> + *\n>> + * algorithm.h - RkISP1 control algorithm interface\n>> + */\n>> +#ifndef __LIBCAMERA_IPA_RKISP1_ALGORITHM_H__\n>> +#define __LIBCAMERA_IPA_RKISP1_ALGORITHM_H__\n>> +\n>> +#include \n>> +\n>> +#include \"ipa_context.h\"\n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa::rkisp1 {\n>> +\n>> +class Algorithm\n>> +{\n>> +public:\n>> +\tvirtual ~Algorithm() {}\n>> +\n>> +\tvirtual int configure(IPAContext &context, const IPACameraSensorInfo &configInfo);\n>> +\tvirtual void prepare(IPAContext &context, rkisp1_params_cfg *params);\n>> +\tvirtual void process(IPAContext &context, const rkisp1_stat_buffer *stats);\n>> +};\n>> +\n>> +} /* namespace ipa::rkisp1 */\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_IPA_RKISP1_ALGORITHM_H__ */\n> \n> This is all very similar to the IPU3 Algorithm class. The header file\n> doesn't bother me that much, but the duplicated documentation is\n> annoying.\n> \n> We obviously can't share the same class between the two IPA modules, but\n> I wonder if we could get away with a class template in libipa:\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(Context &context, const IPACameraSensorInfo &configInfo);\n> \tvirtual void prepare(Context &context, Params *params);\n> \tvirtual void process(Context &context, const Stats *stats);\n> };\n> \n> } /* namespace ipa */\n> \n> } /* namespace libcamera */\n> \n> Then, in src/ipa/rkisp1/algorithms/algorithm.h, we could have\n> \n> namespace libcamera {\n> \n> namespace ipa::rkisp1 {\n> \n> using Algorithm = libcamera::ipa::Algorithm;\n> \n> } /* namespace ipa::rkisp1 */\n> \n> } /* namespace libcamera */\n> \n> Good/bad idea ?\n\nIt is obviously a good idea :-). Though, should it be done in this \nseries, or in a dedicated one on top maybe ?\n\n> \n>> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n>> new file mode 100644\n>> index 00000000..d98f77e2\n>> --- /dev/null\n>> +++ b/src/ipa/rkisp1/algorithms/meson.build\n>> @@ -0,0 +1,5 @@\n>> +# SPDX-License-Identifier: CC0-1.0\n>> +\n>> +rkisp1_ipa_algorithms = files([\n>> + 'algorithm.cpp',\n>> +])\n>> diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build\n>> index 3683c922..a6a4856e 100644\n>> --- a/src/ipa/rkisp1/meson.build\n>> +++ b/src/ipa/rkisp1/meson.build\n>> @@ -1,4 +1,5 @@\n>> # SPDX-License-Identifier: CC0-1.0\n> \n> Missing blank line.\n> \n>> +subdir('algorithms')\n>> \n>> ipa_name = 'ipa_rkisp1'\n>> \n>> @@ -7,6 +8,8 @@ rkisp1_ipa_sources = files([\n>> 'rkisp1.cpp',\n>> ])\n>> \n>> +rkisp1_ipa_sources += rkisp1_ipa_algorithms\n>> +\n>> mod = shared_module(ipa_name,\n>> [rkisp1_ipa_sources, libcamera_generated_ipa_headers],\n>> name_prefix : '',\n>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>> index e0933e22..ddddd52d 100644\n>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>> @@ -25,7 +25,7 @@\n>> \n>> #include \n>> \n>> -#include \"ipa_context.h\"\n>> +#include \"algorithms/algorithm.h\"\n>> #include \"libipa/camera_sensor_helper.h\"\n>> \n>> namespace libcamera {\n>> @@ -82,6 +82,9 @@ private:\n>> \n>> \t/* Local parameter storage */\n>> \tstruct IPAContext context_;\n>> +\n>> +\t/* Maintain the algorithms used by the IPA */\n>> +\tstd::list> algorithms_;\n>> };\n>> \n>> int IPARkISP1::init([[maybe_unused]] const IPASettings &settings,\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 357FEBDB13\n\tfor ;\n\tMon, 22 Nov 2021 14:18:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 597E76038A;\n\tMon, 22 Nov 2021 15:18:52 +0100 (CET)","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 35CCB6022D\n\tfor ;\n\tMon, 22 Nov 2021 15:18:51 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:933b:1c23:4e2b:3c0f] (unknown\n\t[IPv6:2a01:e0a:169:7140:933b:1c23:4e2b:3c0f])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DACE014C3;\n\tMon, 22 Nov 2021 15:18:50 +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=\"qm/4+2W0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637590730;\n\tbh=XnRvwioi37G3rl6hDd2N12ABluDxBfAOyU/ltaeKm+U=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=qm/4+2W0/2TJA3UFkZeYewAr6f+qxaiXWVC4OYnRXefbVzLG2HrdUqBMeuhabTJKP\n\tWdW11jJneV97pqhxQeh53j9pm5ZBjqSk3XgLe+rAqoNFbgq5xEvhPcFsU6OpZcJ9Ex\n\t6LbjTkGwSQkekq5j6qPMgKXX1foTt4MaADKgYNCk=","Message-ID":"<1e07cfe5-3f35-e1fa-00e1-886ce10def48@ideasonboard.com>","Date":"Mon, 22 Nov 2021 15:18:48 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.3.1","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20211119111654.68445-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211119111654.68445-5-jeanmichel.hautbois@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v1 4/8] ipa: ipu3: Introduce the\n\tAlgorithm class","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":21102,"web_url":"https://patchwork.libcamera.org/comment/21102/","msgid":"","date":"2021-11-22T14:27:25","subject":"Re: [libcamera-devel] [PATCH v1 4/8] ipa: ipu3: Introduce the\n\tAlgorithm class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nOn Mon, Nov 22, 2021 at 03:18:48PM +0100, Jean-Michel Hautbois wrote:\n> On 19/11/2021 14:22, Laurent Pinchart wrote:\n> > On Fri, Nov 19, 2021 at 12:16:50PM +0100, Jean-Michel Hautbois wrote:\n> >> Now that everything is in place, introduce the Algorithm class and\n> >> declare it in IPA::RkISP1. There is no functional change yet.\n> >>\n> >> Signed-off-by: Jean-Michel Hautbois \n> >> ---\n> >> src/ipa/rkisp1/algorithms/algorithm.cpp | 101 ++++++++++++++++++++++++\n> >> src/ipa/rkisp1/algorithms/algorithm.h | 32 ++++++++\n> >> src/ipa/rkisp1/algorithms/meson.build | 5 ++\n> >> src/ipa/rkisp1/meson.build | 3 +\n> >> src/ipa/rkisp1/rkisp1.cpp | 5 +-\n> >> 5 files changed, 145 insertions(+), 1 deletion(-)\n> >> create mode 100644 src/ipa/rkisp1/algorithms/algorithm.cpp\n> >> create mode 100644 src/ipa/rkisp1/algorithms/algorithm.h\n> >> create mode 100644 src/ipa/rkisp1/algorithms/meson.build\n> >>\n> >> diff --git a/src/ipa/rkisp1/algorithms/algorithm.cpp b/src/ipa/rkisp1/algorithms/algorithm.cpp\n> >> new file mode 100644\n> >> index 00000000..be3cd9f6\n> >> --- /dev/null\n> >> +++ b/src/ipa/rkisp1/algorithms/algorithm.cpp\n> >> @@ -0,0 +1,101 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2021, Ideas On Board\n> >> + *\n> >> + * algorithm.cpp - RkISP1 control algorithm interface\n> >> + */\n> >> +\n> >> +#include \"algorithm.h\"\n> >> +\n> >> +/**\n> >> + * \\file algorithm.h\n> >> + * \\brief Algorithm common interface\n> >> + */\n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +namespace ipa::rkisp1 {\n> >> +\n> >> +/**\n> >> + * \\class Algorithm\n> >> + * \\brief The base class for all RkISP1 algorithms\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> >> + * to manage algorithms regardless of their specific type.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Configure the Algorithm given an IPAConfigInfo\n> >> + * \\param[in] context The shared IPA context\n> >> + * \\param[in] IPACameraSensorInfo The IPA configuration data, received from the\n> >> + * pipeline handler\n> >> + *\n> >> + * Algorithms may implement a configure operation to pre-calculate\n> >> + * parameters prior to commencing streaming.\n> >> + *\n> >> + * Configuration state may be stored in the IPASessionConfiguration structure of\n> >> + * the IPAContext.\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 IPACameraSensorInfo &configInfo)\n> >> +{\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +/**\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 RkISP1 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> >> + * 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> >> + * 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]] rkisp1_params_cfg *params)\n> >> +{\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Process ISP statistics, and run algorithm operations\n> >> + * \\param[in] context The shared IPA context\n> >> + * \\param[in] stats The RkISP1 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> >> + * Algorithms shall use this data to run calculations and update their state\n> >> + * accordingly.\n> >> + *\n> >> + * Processing shall not take an undue amount of time, and any extended or\n> >> + * computationally expensive calculations or operations must be handled\n> >> + * asynchronously in a separate thread.\n> >> + *\n> >> + * Algorithms can store state in their respective IPAFrameContext structures,\n> >> + * and reference state from the IPAFrameContext of other algorithms.\n> >> + *\n> >> + * \\todo Historical data may be required as part of the processing.\n> >> + * Either the previous frame, or the IPAFrameContext state of the frame\n> >> + * that generated the statistics for this operation may be required for\n> >> + * some advanced algorithms to prevent oscillations or support control\n> >> + * loops correctly. Only a single IPAFrameContext is available currently,\n> >> + * and so any data stored may represent the results of the previously\n> >> + * completed operations.\n> >> + *\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 rkisp1_stat_buffer *stats)\n> >> +{\n> >> +}\n> >> +\n> >> +} /* namespace ipa::rkisp1 */\n> >> +\n> >> +} /* namespace libcamera */\n> >> diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h\n> >> new file mode 100644\n> >> index 00000000..5365a860\n> >> --- /dev/null\n> >> +++ b/src/ipa/rkisp1/algorithms/algorithm.h\n> >> @@ -0,0 +1,32 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2021, Ideas On Board\n> >> + *\n> >> + * algorithm.h - RkISP1 control algorithm interface\n> >> + */\n> >> +#ifndef __LIBCAMERA_IPA_RKISP1_ALGORITHM_H__\n> >> +#define __LIBCAMERA_IPA_RKISP1_ALGORITHM_H__\n> >> +\n> >> +#include \n> >> +\n> >> +#include \"ipa_context.h\"\n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +namespace ipa::rkisp1 {\n> >> +\n> >> +class Algorithm\n> >> +{\n> >> +public:\n> >> +\tvirtual ~Algorithm() {}\n> >> +\n> >> +\tvirtual int configure(IPAContext &context, const IPACameraSensorInfo &configInfo);\n> >> +\tvirtual void prepare(IPAContext &context, rkisp1_params_cfg *params);\n> >> +\tvirtual void process(IPAContext &context, const rkisp1_stat_buffer *stats);\n> >> +};\n> >> +\n> >> +} /* namespace ipa::rkisp1 */\n> >> +\n> >> +} /* namespace libcamera */\n> >> +\n> >> +#endif /* __LIBCAMERA_IPA_RKISP1_ALGORITHM_H__ */\n> > \n> > This is all very similar to the IPU3 Algorithm class. The header file\n> > doesn't bother me that much, but the duplicated documentation is\n> > annoying.\n> > \n> > We obviously can't share the same class between the two IPA modules, but\n> > I wonder if we could get away with a class template in libipa:\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(Context &context, const IPACameraSensorInfo &configInfo);\n> > \tvirtual void prepare(Context &context, Params *params);\n> > \tvirtual void process(Context &context, const Stats *stats);\n> > };\n> > \n> > } /* namespace ipa */\n> > \n> > } /* namespace libcamera */\n> > \n> > Then, in src/ipa/rkisp1/algorithms/algorithm.h, we could have\n> > \n> > namespace libcamera {\n> > \n> > namespace ipa::rkisp1 {\n> > \n> > using Algorithm = libcamera::ipa::Algorithm;\n> > \n> > } /* namespace ipa::rkisp1 */\n> > \n> > } /* namespace libcamera */\n> > \n> > Good/bad idea ?\n> \n> It is obviously a good idea :-). Though, should it be done in this \n> series, or in a dedicated one on top maybe ?\n\nIt can be done on top, but it's nicer to review if it you do it as part\nof the series. I'd say it depends on the amount of work required. It\ndoesn't seem too difficult to me, but I'm looking at the issue from a\nhigh level, I may be missing nasty details.\n\nOne point that needs to be considered is if we can design an Algorithm\nclass shared by IPA modules that will only differ in the data types it\nuses, or if functions will also need to be customized. Adding new\nIPA-specific functions is always possible through inheritance, but we\nneed a common set of functions to make this worth it.\n\n> >> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> >> new file mode 100644\n> >> index 00000000..d98f77e2\n> >> --- /dev/null\n> >> +++ b/src/ipa/rkisp1/algorithms/meson.build\n> >> @@ -0,0 +1,5 @@\n> >> +# SPDX-License-Identifier: CC0-1.0\n> >> +\n> >> +rkisp1_ipa_algorithms = files([\n> >> + 'algorithm.cpp',\n> >> +])\n> >> diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build\n> >> index 3683c922..a6a4856e 100644\n> >> --- a/src/ipa/rkisp1/meson.build\n> >> +++ b/src/ipa/rkisp1/meson.build\n> >> @@ -1,4 +1,5 @@\n> >> # SPDX-License-Identifier: CC0-1.0\n> > \n> > Missing blank line.\n> > \n> >> +subdir('algorithms')\n> >> \n> >> ipa_name = 'ipa_rkisp1'\n> >> \n> >> @@ -7,6 +8,8 @@ rkisp1_ipa_sources = files([\n> >> 'rkisp1.cpp',\n> >> ])\n> >> \n> >> +rkisp1_ipa_sources += rkisp1_ipa_algorithms\n> >> +\n> >> mod = shared_module(ipa_name,\n> >> [rkisp1_ipa_sources, libcamera_generated_ipa_headers],\n> >> name_prefix : '',\n> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> >> index e0933e22..ddddd52d 100644\n> >> --- a/src/ipa/rkisp1/rkisp1.cpp\n> >> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> >> @@ -25,7 +25,7 @@\n> >> \n> >> #include \n> >> \n> >> -#include \"ipa_context.h\"\n> >> +#include \"algorithms/algorithm.h\"\n> >> #include \"libipa/camera_sensor_helper.h\"\n> >> \n> >> namespace libcamera {\n> >> @@ -82,6 +82,9 @@ private:\n> >> \n> >> \t/* Local parameter storage */\n> >> \tstruct IPAContext context_;\n> >> +\n> >> +\t/* Maintain the algorithms used by the IPA */\n> >> +\tstd::list> algorithms_;\n> >> };\n> >> \n> >> int IPARkISP1::init([[maybe_unused]] const IPASettings &settings,","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 0278EBDB13\n\tfor ;\n\tMon, 22 Nov 2021 14:27:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 759126022D;\n\tMon, 22 Nov 2021 15:27:50 +0100 (CET)","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 4AE5F6022D\n\tfor ;\n\tMon, 22 Nov 2021 15:27:48 +0100 (CET)","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 B537314C3;\n\tMon, 22 Nov 2021 15:27:47 +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=\"lKCTsh75\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637591267;\n\tbh=5X1Af3XfUz2hlBkPZKezYCkrvS8pq2YFMEos9fFBqQI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lKCTsh753qbPo1l8ehpYAwb16XLxGrWBEcsvVVdpT6BOoGLffKUXKDR/irtun+g5M\n\teFMYqMUzXUxCkcBXrkErUU7gGOhG6uty1lX5wYeIFtYEr6+GX0kd1r28q0OYXvnz25\n\tLl7kQsCl9c/0D/1xIVUXRt4GTZro2+iPf2XnCT2Q=","Date":"Mon, 22 Nov 2021 16:27:25 +0200","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20211119111654.68445-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211119111654.68445-5-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<1e07cfe5-3f35-e1fa-00e1-886ce10def48@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<1e07cfe5-3f35-e1fa-00e1-886ce10def48@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v1 4/8] ipa: ipu3: Introduce the\n\tAlgorithm class","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":21103,"web_url":"https://patchwork.libcamera.org/comment/21103/","msgid":"<85875f27-5b2e-1856-c724-0f774b12d8f5@ideasonboard.com>","date":"2021-11-22T14:43:44","subject":"Re: [libcamera-devel] [PATCH v1 4/8] ipa: ipu3: Introduce the\n\tAlgorithm class","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"On 22/11/2021 15:27, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> On Mon, Nov 22, 2021 at 03:18:48PM +0100, Jean-Michel Hautbois wrote:\n>> On 19/11/2021 14:22, Laurent Pinchart wrote:\n>>> On Fri, Nov 19, 2021 at 12:16:50PM +0100, Jean-Michel Hautbois wrote:\n>>>> Now that everything is in place, introduce the Algorithm class and\n>>>> declare it in IPA::RkISP1. There is no functional change yet.\n>>>>\n>>>> Signed-off-by: Jean-Michel Hautbois \n>>>> ---\n>>>> src/ipa/rkisp1/algorithms/algorithm.cpp | 101 ++++++++++++++++++++++++\n>>>> src/ipa/rkisp1/algorithms/algorithm.h | 32 ++++++++\n>>>> src/ipa/rkisp1/algorithms/meson.build | 5 ++\n>>>> src/ipa/rkisp1/meson.build | 3 +\n>>>> src/ipa/rkisp1/rkisp1.cpp | 5 +-\n>>>> 5 files changed, 145 insertions(+), 1 deletion(-)\n>>>> create mode 100644 src/ipa/rkisp1/algorithms/algorithm.cpp\n>>>> create mode 100644 src/ipa/rkisp1/algorithms/algorithm.h\n>>>> create mode 100644 src/ipa/rkisp1/algorithms/meson.build\n>>>>\n>>>> diff --git a/src/ipa/rkisp1/algorithms/algorithm.cpp b/src/ipa/rkisp1/algorithms/algorithm.cpp\n>>>> new file mode 100644\n>>>> index 00000000..be3cd9f6\n>>>> --- /dev/null\n>>>> +++ b/src/ipa/rkisp1/algorithms/algorithm.cpp\n>>>> @@ -0,0 +1,101 @@\n>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>>> +/*\n>>>> + * Copyright (C) 2021, Ideas On Board\n>>>> + *\n>>>> + * algorithm.cpp - RkISP1 control algorithm interface\n>>>> + */\n>>>> +\n>>>> +#include \"algorithm.h\"\n>>>> +\n>>>> +/**\n>>>> + * \\file algorithm.h\n>>>> + * \\brief Algorithm common interface\n>>>> + */\n>>>> +\n>>>> +namespace libcamera {\n>>>> +\n>>>> +namespace ipa::rkisp1 {\n>>>> +\n>>>> +/**\n>>>> + * \\class Algorithm\n>>>> + * \\brief The base class for all RkISP1 algorithms\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>>>> + * to manage algorithms regardless of their specific type.\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\brief Configure the Algorithm given an IPAConfigInfo\n>>>> + * \\param[in] context The shared IPA context\n>>>> + * \\param[in] IPACameraSensorInfo The IPA configuration data, received from the\n>>>> + * pipeline handler\n>>>> + *\n>>>> + * Algorithms may implement a configure operation to pre-calculate\n>>>> + * parameters prior to commencing streaming.\n>>>> + *\n>>>> + * Configuration state may be stored in the IPASessionConfiguration structure of\n>>>> + * the IPAContext.\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 IPACameraSensorInfo &configInfo)\n>>>> +{\n>>>> +\treturn 0;\n>>>> +}\n>>>> +\n>>>> +/**\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 RkISP1 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>>>> + * 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>>>> + * 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]] rkisp1_params_cfg *params)\n>>>> +{\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Process ISP statistics, and run algorithm operations\n>>>> + * \\param[in] context The shared IPA context\n>>>> + * \\param[in] stats The RkISP1 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>>>> + * Algorithms shall use this data to run calculations and update their state\n>>>> + * accordingly.\n>>>> + *\n>>>> + * Processing shall not take an undue amount of time, and any extended or\n>>>> + * computationally expensive calculations or operations must be handled\n>>>> + * asynchronously in a separate thread.\n>>>> + *\n>>>> + * Algorithms can store state in their respective IPAFrameContext structures,\n>>>> + * and reference state from the IPAFrameContext of other algorithms.\n>>>> + *\n>>>> + * \\todo Historical data may be required as part of the processing.\n>>>> + * Either the previous frame, or the IPAFrameContext state of the frame\n>>>> + * that generated the statistics for this operation may be required for\n>>>> + * some advanced algorithms to prevent oscillations or support control\n>>>> + * loops correctly. Only a single IPAFrameContext is available currently,\n>>>> + * and so any data stored may represent the results of the previously\n>>>> + * completed operations.\n>>>> + *\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 rkisp1_stat_buffer *stats)\n>>>> +{\n>>>> +}\n>>>> +\n>>>> +} /* namespace ipa::rkisp1 */\n>>>> +\n>>>> +} /* namespace libcamera */\n>>>> diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h\n>>>> new file mode 100644\n>>>> index 00000000..5365a860\n>>>> --- /dev/null\n>>>> +++ b/src/ipa/rkisp1/algorithms/algorithm.h\n>>>> @@ -0,0 +1,32 @@\n>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>>> +/*\n>>>> + * Copyright (C) 2021, Ideas On Board\n>>>> + *\n>>>> + * algorithm.h - RkISP1 control algorithm interface\n>>>> + */\n>>>> +#ifndef __LIBCAMERA_IPA_RKISP1_ALGORITHM_H__\n>>>> +#define __LIBCAMERA_IPA_RKISP1_ALGORITHM_H__\n>>>> +\n>>>> +#include \n>>>> +\n>>>> +#include \"ipa_context.h\"\n>>>> +\n>>>> +namespace libcamera {\n>>>> +\n>>>> +namespace ipa::rkisp1 {\n>>>> +\n>>>> +class Algorithm\n>>>> +{\n>>>> +public:\n>>>> +\tvirtual ~Algorithm() {}\n>>>> +\n>>>> +\tvirtual int configure(IPAContext &context, const IPACameraSensorInfo &configInfo);\n>>>> +\tvirtual void prepare(IPAContext &context, rkisp1_params_cfg *params);\n>>>> +\tvirtual void process(IPAContext &context, const rkisp1_stat_buffer *stats);\n>>>> +};\n>>>> +\n>>>> +} /* namespace ipa::rkisp1 */\n>>>> +\n>>>> +} /* namespace libcamera */\n>>>> +\n>>>> +#endif /* __LIBCAMERA_IPA_RKISP1_ALGORITHM_H__ */\n>>>\n>>> This is all very similar to the IPU3 Algorithm class. The header file\n>>> doesn't bother me that much, but the duplicated documentation is\n>>> annoying.\n>>>\n>>> We obviously can't share the same class between the two IPA modules, but\n>>> I wonder if we could get away with a class template in libipa:\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(Context &context, const IPACameraSensorInfo &configInfo);\n>>> \tvirtual void prepare(Context &context, Params *params);\n>>> \tvirtual void process(Context &context, const Stats *stats);\n>>> };\n>>>\n>>> } /* namespace ipa */\n>>>\n>>> } /* namespace libcamera */\n>>>\n>>> Then, in src/ipa/rkisp1/algorithms/algorithm.h, we could have\n>>>\n>>> namespace libcamera {\n>>>\n>>> namespace ipa::rkisp1 {\n>>>\n>>> using Algorithm = libcamera::ipa::Algorithm;\n>>>\n>>> } /* namespace ipa::rkisp1 */\n>>>\n>>> } /* namespace libcamera */\n>>>\n>>> Good/bad idea ?\n>>\n>> It is obviously a good idea :-). Though, should it be done in this\n>> series, or in a dedicated one on top maybe ?\n> \n> It can be done on top, but it's nicer to review if it you do it as part\n> of the series. I'd say it depends on the amount of work required. It\n> doesn't seem too difficult to me, but I'm looking at the issue from a\n> high level, I may be missing nasty details.\n> \n> One point that needs to be considered is if we can design an Algorithm\n> class shared by IPA modules that will only differ in the data types it\n> uses, or if functions will also need to be customized. Adding new\n> IPA-specific functions is always possible through inheritance, but we\n> need a common set of functions to make this worth it.\n> \n\nWell, for IPU3 and RkISP1 I think both will use the same functions and \nwe don't need more, afaik. I will try to implement it then, you made the \nmost difficult part I think ;-).\n\n>>>> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n>>>> new file mode 100644\n>>>> index 00000000..d98f77e2\n>>>> --- /dev/null\n>>>> +++ b/src/ipa/rkisp1/algorithms/meson.build\n>>>> @@ -0,0 +1,5 @@\n>>>> +# SPDX-License-Identifier: CC0-1.0\n>>>> +\n>>>> +rkisp1_ipa_algorithms = files([\n>>>> + 'algorithm.cpp',\n>>>> +])\n>>>> diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build\n>>>> index 3683c922..a6a4856e 100644\n>>>> --- a/src/ipa/rkisp1/meson.build\n>>>> +++ b/src/ipa/rkisp1/meson.build\n>>>> @@ -1,4 +1,5 @@\n>>>> # SPDX-License-Identifier: CC0-1.0\n>>>\n>>> Missing blank line.\n>>>\n>>>> +subdir('algorithms')\n>>>> \n>>>> ipa_name = 'ipa_rkisp1'\n>>>> \n>>>> @@ -7,6 +8,8 @@ rkisp1_ipa_sources = files([\n>>>> 'rkisp1.cpp',\n>>>> ])\n>>>> \n>>>> +rkisp1_ipa_sources += rkisp1_ipa_algorithms\n>>>> +\n>>>> mod = shared_module(ipa_name,\n>>>> [rkisp1_ipa_sources, libcamera_generated_ipa_headers],\n>>>> name_prefix : '',\n>>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>>>> index e0933e22..ddddd52d 100644\n>>>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>>>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>>>> @@ -25,7 +25,7 @@\n>>>> \n>>>> #include \n>>>> \n>>>> -#include \"ipa_context.h\"\n>>>> +#include \"algorithms/algorithm.h\"\n>>>> #include \"libipa/camera_sensor_helper.h\"\n>>>> \n>>>> namespace libcamera {\n>>>> @@ -82,6 +82,9 @@ private:\n>>>> \n>>>> \t/* Local parameter storage */\n>>>> \tstruct IPAContext context_;\n>>>> +\n>>>> +\t/* Maintain the algorithms used by the IPA */\n>>>> +\tstd::list> algorithms_;\n>>>> };\n>>>> \n>>>> int IPARkISP1::init([[maybe_unused]] const IPASettings &settings,\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 A4D50BF415\n\tfor ;\n\tMon, 22 Nov 2021 14:43:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0243D6038C;\n\tMon, 22 Nov 2021 15:43:49 +0100 (CET)","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 176436022D\n\tfor ;\n\tMon, 22 Nov 2021 15:43:48 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:933b:1c23:4e2b:3c0f] (unknown\n\t[IPv6:2a01:e0a:169:7140:933b:1c23:4e2b:3c0f])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 79AD51B40;\n\tMon, 22 Nov 2021 15:43:47 +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=\"gu6Mb4LY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637592227;\n\tbh=idIEBN2aKpLBPmbrFVnw5rMVNXHHZ4mRx+eaD1uEbkg=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=gu6Mb4LY/+Dt8uvNPEeoHxDz1Aarnx7pQueRG/V5Avm58Xm3apOS4bGiwZwE5NCjK\n\tvZbyqyDZLomeiRN8ONmnUaafpW5VPbKNkXLdv2Xpyd8pv2bfx6aMAGnvc/K9/w1H+4\n\tOxy0OWlGXPrpbiudfMbpgidoZpJ2odlgBnXwvAYE=","Message-ID":"<85875f27-5b2e-1856-c724-0f774b12d8f5@ideasonboard.com>","Date":"Mon, 22 Nov 2021 15:43:44 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.3.1","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20211119111654.68445-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211119111654.68445-5-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<1e07cfe5-3f35-e1fa-00e1-886ce10def48@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v1 4/8] ipa: ipu3: Introduce the\n\tAlgorithm class","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\" "}}]