[{"id":15271,"web_url":"https://patchwork.libcamera.org/comment/15271/","msgid":"","date":"2021-02-22T04:06:53","subject":"Re: [libcamera-devel] [RFC PATCH 1/2] WIP: ipa: Add Controller and\n\tAlgorithm skeleton","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, Feb 19, 2021 at 06:22:23PM +0100, Jean-Michel Hautbois wrote:\n> In order to instanciate and control IPA algorithms (AWB, AGC, etc.)\n> there is a need for an IPA algorithm class to define mandatory methods,\n> and an IPA controller class to operate algorithms together.\n> \n> Instead of reinventing the wheel, reuse what Raspberry Pi has done and\n> adapt to the minimum requirements expected.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> include/libcamera/ipa/agc_algorithm.h | 32 ++++++++++++++++++\n> include/libcamera/ipa/awb_algorithm.h | 27 +++++++++++++++\n> include/libcamera/ipa/ipa_algorithm.h | 46 ++++++++++++++++++++++++++\n> include/libcamera/ipa/ipa_controller.h | 39 ++++++++++++++++++++++\n> include/libcamera/ipa/meson.build | 4 +++\n> src/ipa/libipa/ipa_algorithm.cpp | 20 +++++++++++\n> src/ipa/libipa/ipa_controller.cpp | 45 +++++++++++++++++++++++++\n> src/ipa/libipa/meson.build | 2 ++\n> 8 files changed, 215 insertions(+)\n> create mode 100644 include/libcamera/ipa/agc_algorithm.h\n> create mode 100644 include/libcamera/ipa/awb_algorithm.h\n> create mode 100644 include/libcamera/ipa/ipa_algorithm.h\n> create mode 100644 include/libcamera/ipa/ipa_controller.h\n> create mode 100644 src/ipa/libipa/ipa_algorithm.cpp\n> create mode 100644 src/ipa/libipa/ipa_controller.cpp\n> \n> diff --git a/include/libcamera/ipa/agc_algorithm.h b/include/libcamera/ipa/agc_algorithm.h\n> new file mode 100644\n> index 00000000..4dd17103\n> --- /dev/null\n> +++ b/include/libcamera/ipa/agc_algorithm.h\n> @@ -0,0 +1,32 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited\n> + *\n> + * agc_algorithm.h - AGC/AEC control algorithm interface\n> + */\n> +#ifndef __LIBCAMERA_AGC_ALGORITHM_H__\n\nThis should be __LIBCAMERA_IPA_AGC_ALGORITHM_H__ as the file is in\nlibcamera/ipa/.\n\nLots of the comments I'll make here apply in many locations, I won't\nrepeat them everywhere.\n\n> +#define __LIBCAMERA_AGC_ALGORITHM_H__\n> +\n> +#include \n\nShould this be considered as an internal header that won't be installed\n? It should then use double quotes instead of <>.\n\n> +\n> +namespace libcamera {\n\nThe IPA IPC generated code uses an ipa namespace, should we do the same\nhere ? To be clear, that would be\n\nnamespace libcamera {\n\nnamespace ipa {\n\n> +\n> +class AgcAlgorithm : public IPAAlgorithm\n> +{\n> +public:\n> +\tAgcAlgorithm(IPAController *controller)\n> +\t\t: IPAAlgorithm(controller) {}\n\nThe usual coding style is\n\n\tAgcAlgorithm(IPAController *controller)\n\t\t: IPAAlgorithm(controller)\n\t{\n\t}\n\n> +\t/* An AGC algorithm must provide the following: */\n\nI'd drop this, as it's implied by virtual ... = 0.\n\n> +\tvirtual unsigned int GetConvergenceFrames() const = 0;\n\nFunctions should start with a lower-case letter. Getter shouldn't start with\n'get', so this should be\n\n\tvirtual unsigned int convergenceFrames() const = 0;\n\n> +\tvirtual void SetEv(double ev) = 0;\n> +\tvirtual void SetFlickerPeriod(double flicker_period) = 0;\n\nVariables should use camelCase.\n\n> +\tvirtual void SetFixedShutter(double fixed_shutter) = 0; // microseconds\n> +\tvirtual void SetMaxShutter(double max_shutter) = 0; // microseconds\n\nWe use C-style comments.\n\nInstead of documenting the function parameter in the .h file, it should\nbe documented in a doxygen comment block in a .cpp file. This can go in\nipa_algorithm.cpp, or you can create an agc_algorithm.cpp with\ndocumentation only for this purpose. The latter would likely scale\nbetter.\n\nWe can also consider switching to std::chrono::microseconds, that will\nincrease type safety, at the cost of not allowing sub-microseconds\nresolution, which is probably fine here, but maybe we could then use\nnanoseconds instead.\n\nWe should also consider if we want to always express all times in IPA\ncode in the same unit.\n\n> +\tvirtual void SetFixedAnalogueGain(double fixed_analogue_gain) = 0;\n> +\tvirtual void SetMeteringMode(std::string const &metering_mode_name) = 0;\n\nWe usually put the const keyword before the type.\n\n> +\tvirtual void SetExposureMode(std::string const &exposure_mode_name) = 0;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_AGC_ALGORITHM_H__ */\n> diff --git a/include/libcamera/ipa/awb_algorithm.h b/include/libcamera/ipa/awb_algorithm.h\n> new file mode 100644\n> index 00000000..37464d12\n> --- /dev/null\n> +++ b/include/libcamera/ipa/awb_algorithm.h\n> @@ -0,0 +1,27 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited\n> + *\n> + * awb_algorithm.h - AWB control algorithm interface\n> + */\n> +#ifndef __LIBCAMERA_AWB_ALGORITHM_H__\n> +#define __LIBCAMERA_AWB_ALGORITHM_H__\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class AwbAlgorithm : public IPAAlgorithm\n> +{\n> +public:\n> +\tAwbAlgorithm(IPAController *controller)\n> +\t\t: IPAAlgorithm(controller) {}\n> +\t/* An AWB algorithm must provide the following: */\n> +\tvirtual unsigned int GetConvergenceFrames() const = 0;\n> +\tvirtual void SetMode(std::string const &mode_name) = 0;\n> +\tvirtual void SetManualGains(double manual_r, double manual_b) = 0;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_AWB_ALGORITHM_H__ */\n> diff --git a/include/libcamera/ipa/ipa_algorithm.h b/include/libcamera/ipa/ipa_algorithm.h\n> new file mode 100644\n> index 00000000..e48b99a6\n> --- /dev/null\n> +++ b/include/libcamera/ipa/ipa_algorithm.h\n> @@ -0,0 +1,46 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited\n> + *\n> + * ipa_algorithm.h - ISP control algorithm interface\n> + */\n> +#ifndef __LIBCAMERA_IPA_ALGORITHM_H__\n> +#define __LIBCAMERA_IPA_ALGORITHM_H__\n> +\n> +/* All algorithms should be derived from this class and made available to the\n> + * Controller. */\n\n/*\n * All algorithms should be derived from this class and made available\n * to the Controller.\n */\n\nBut this should go to the .cpp file, in a doxygen comment.\n\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"ipa_controller.h\"\n\n#include \"libcamera/ipa/ipa_controller.h\"\n\n> +\n> +namespace libcamera {\n> +\n> +/* This defines the basic interface for all control algorithms. */\n> +\n> +class IPAAlgorithm\n> +{\n> +public:\n> +\tIPAAlgorithm(IPAController *controller)\n> +\t\t: controller_(controller), paused_(false)\n> +\t{\n> +\t}\n> +\tvirtual ~IPAAlgorithm() = default;\n> +\tvirtual char const *Name() const = 0;\n> +\tvirtual bool IsPaused() const { return paused_; }\n> +\tvirtual void Pause() { paused_ = true; }\n> +\tvirtual void Resume() { paused_ = false; }\n> +\tvirtual void Initialise();\n> +\tvirtual void Prepare();\n> +\tvirtual void Process();\n> +\n> +private:\n> +\t[[maybe_unused]] IPAController *controller_;\n\nIs the [[maybe_unused]] needed ? Or, rather, is this field even needed ?\nYou don't use it in this series, so I'd drop it. You could then remove\nthe controller parameter from the constructor.\n\n> +\tbool paused_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_ALGORITHM_H__ */\n> diff --git a/include/libcamera/ipa/ipa_controller.h b/include/libcamera/ipa/ipa_controller.h\n> new file mode 100644\n> index 00000000..953cad4a\n> --- /dev/null\n> +++ b/include/libcamera/ipa/ipa_controller.h\n> @@ -0,0 +1,39 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited\n> + *\n> + * ipa_controller.h - ISP controller interface\n> + */\n> +#ifndef __LIBCAMERA_IPA_CONTROLLER_H__\n> +#define __LIBCAMERA_IPA_CONTROLLER_H__\n> +\n> +// The Controller is simply a container for a collecting together a number of\n> +// \"control algorithms\" (such as AWB etc.) and for running them all in a\n> +// convenient manner.\n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class IPAAlgorithm;\n> +typedef std::unique_ptr IPAAlgorithmPtr;\n> +\n> +class IPAController\n> +{\n> +public:\n> +\tIPAController();\n> +\t~IPAController();\n\nA blank line would be nice here.\n\nhttps://en.cppreference.com/w/cpp/language/rule_of_three\n\nThe copy constructor and copy assignment operator should be defined as\ndeleted. As there's also no use case for moving the IPAController, you\ncan use the LIBCAMERA_DISABLE_COPY_AND_MOVE() macro defined in class.h.\n\n> +\tIPAAlgorithm *CreateAlgorithm(char const *name);\n> +\tvoid Initialise();\n> +\tvoid Prepare();\n> +\tvoid Process();\n> +\tIPAAlgorithm *GetAlgorithm(std::string const &name) const;\n\nI'd use the same type for the name argument to both CreateAlgorithm()\nand GetAlgorithm().\n\nActually, neither functions are used, so you can drop them. And, reading\npatch 2/2, the IPAController class is instantiated, but not used yet.\nShould it be dropped for now ?\n\n> +\n> +protected:\n> +\tstd::vector algorithms_;\n\nThe IPAAlgorithmPtr type is only used here, and hinders readability as\nit requires looking it up. You can just use\n\n\tstd::vector> algorithms_;\n\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_CONTROLLER_H__ */\n> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build\n> index a4d3f868..e56d8b00 100644\n> --- a/include/libcamera/ipa/meson.build\n> +++ b/include/libcamera/ipa/meson.build\n> @@ -1,6 +1,10 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> libcamera_ipa_headers = files([\n> + 'agc_algorithm.h',\n> + 'awb_algorithm.h',\n> + 'ipa_algorithm.h',\n> + 'ipa_controller.h',\n\nThese should go to libipa_headers.\n\n> 'ipa_controls.h',\n> 'ipa_interface.h',\n> 'ipa_module_info.h',\n> diff --git a/src/ipa/libipa/ipa_algorithm.cpp b/src/ipa/libipa/ipa_algorithm.cpp\n> new file mode 100644\n> index 00000000..16fb29ce\n> --- /dev/null\n> +++ b/src/ipa/libipa/ipa_algorithm.cpp\n> @@ -0,0 +1,20 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited\n> + *\n> + * ipa_algorithm.cpp - ISP control algorithms\n> + */\n> +#include \n\nWe have a logging infrastructure.\n\n> +\n> +#include \n> +\n> +using namespace libcamera;\n\nLet's open the libcamera namespace instead\n\nnamespace libcamera {\n\n> +\n> +void IPAAlgorithm::Initialise()\n> +{\n> +\tstd::cout << \"Entering: \" << __func__ << std::endl;\n\nI'm not sure if logging all calls to this function will be very useful,\nespecially given that it will always print the name of this function,\nwithout any information about the derived class. Let's just drop it.\n\n> +}\n> +\n> +void IPAAlgorithm::Prepare() {}\n\nvoid IPAAlgorithm::Prepare()\n{\n}\n\n> +\n> +void IPAAlgorithm::Process() {}\n> diff --git a/src/ipa/libipa/ipa_controller.cpp b/src/ipa/libipa/ipa_controller.cpp\n> new file mode 100644\n> index 00000000..e2cde8ce\n> --- /dev/null\n> +++ b/src/ipa/libipa/ipa_controller.cpp\n> @@ -0,0 +1,45 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited\n> + *\n> + * ipa_controller.cpp - ISP controller\n> + */\n> +\n> +#include \"libcamera/internal/log.h\"\n> +\n> +#include \n> +#include \n> +\n> +using namespace libcamera;\n> +\n> +LOG_DEFINE_CATEGORY(IPAController)\n> +\n> +IPAController::IPAController() {}\n\nThis can also be written\n\nIPAController::IPAController() = default;\n\n> +\n> +IPAController::~IPAController() {}\n\nHere too.\n\n> +\n> +IPAAlgorithm *IPAController::CreateAlgorithm(char const *name)\n> +{\n> +\tLOG(IPAController, Error) << \"Create algorithm \" << name;\n> +\treturn nullptr;\n> +}\n> +\n> +void IPAController::Initialise()\n> +{\n> +\tfor (auto &algo : algorithms_)\n\nLet's avoid auto when possible:\n\n\tfor (IPAAlgorithm &algo : algorithms_)\n\n> +\t\talgo->Initialise();\n> +}\n> +\n> +void IPAController::Prepare()\n> +{\n> +\tfor (auto &algo : algorithms_)\n> +\t\tif (!algo->IsPaused())\n> +\t\t\talgo->Prepare();\n\n\tfor (auto &algo : algorithms_) {\n\t\tif (!algo->IsPaused())\n\t\t\talgo->Prepare();\n\t}\n\nI foresee a need for a more complex scheduling implementation that will\ntake into account dependencies between algorithms. This will be an\ninteresting research topic.\n\n> +}\n> +\n> +void IPAController::Process()\n> +{\n> +\tfor (auto &algo : algorithms_)\n> +\t\tif (!algo->IsPaused())\n> +\t\t\talgo->Process();\n> +}\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index b29ef0f4..1693e489 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -4,6 +4,8 @@ libipa_headers = files([\n> ])\n> \n> libipa_sources = files([\n> + 'ipa_algorithm.cpp',\n> + 'ipa_controller.cpp',\n> ])\n> \n> libipa_includes = include_directories('..')","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 D9386BD1F6\n\tfor ;\n\tMon, 22 Feb 2021 04:07:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 489A6689F8;\n\tMon, 22 Feb 2021 05:07:30 +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 30FE7689EF\n\tfor ;\n\tMon, 22 Feb 2021 05:07:29 +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 F40DA344;\n\tMon, 22 Feb 2021 05:07:20 +0100 (CET)"],"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=\"ngXf9XfE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1613966848;\n\tbh=CF9JGvdBqOUgBkl1fyoT/bwx9Nf9jnNQNc4x1XLcYRw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ngXf9XfEfrdNn8LC1/bK3yrkDhdUf/bl3/edI8y835njdEH7Et/iiZsBZdun5UAr/\n\t4QDAGPK/oLb/JwnbPfltawjzXBdrfZNMujs4mGPY77MDLm/j/UZrLIKZ7t0dHucDQM\n\t5NuEf5mD852hBI0p7N8IyJZp98pmIFCvU15YJHmY=","Date":"Mon, 22 Feb 2021 06:06:53 +0200","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210219172224.69862-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210219172224.69862-2-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210219172224.69862-2-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 1/2] WIP: ipa: Add Controller and\n\tAlgorithm skeleton","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]