[{"id":23881,"web_url":"https://patchwork.libcamera.org/comment/23881/","msgid":"<20220714160703.vssoijogyu6zacfj@uno.localdomain>","date":"2022-07-14T16:07:03","subject":"Re: [libcamera-devel] [PATCH v2 01/11] ipa: Add base class defining\n\tAF algorithm interface","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Daniel,\n I like -very much- how you have split the algorithm in the platform\ndependent part and in the generic part! very nice\n\nOn Wed, Jul 13, 2022 at 10:43:07AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> Define common interface with basic functions that should be supported\n> by Auto Focus algorithms.\n>\n> Signed-off-by: Daniel Semkowicz \n> ---\n> src/ipa/libipa/algorithms/af_algorithm.cpp | 78 ++++++++++++++++++++++\n> src/ipa/libipa/algorithms/af_algorithm.h | 41 ++++++++++++\n> src/ipa/libipa/algorithms/meson.build | 9 +++\n> src/ipa/libipa/meson.build | 6 ++\n> 4 files changed, 134 insertions(+)\n> create mode 100644 src/ipa/libipa/algorithms/af_algorithm.cpp\n> create mode 100644 src/ipa/libipa/algorithms/af_algorithm.h\n> create mode 100644 src/ipa/libipa/algorithms/meson.build\n>\n> diff --git a/src/ipa/libipa/algorithms/af_algorithm.cpp b/src/ipa/libipa/algorithms/af_algorithm.cpp\n> new file mode 100644\n> index 00000000..47e54d5a\n> --- /dev/null\n> +++ b/src/ipa/libipa/algorithms/af_algorithm.cpp\n> @@ -0,0 +1,78 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2022, Raspberry Pi (Trading) Limited\n> + * Copyright (C) 2022, Theobroma Systems\n\nFor this and the other new files, is this your copyright ?\n\n> + *\n> + * af_algorithm.cpp - Autofocus control algorithm interface\n> + */\n> +\n> +#include \"af_algorithm.h\"\n> +\n> +/**\n> + * \\file af_algorithm.h\n> + * \\brief AF algorithm common interface\n> + */\n> +\n> +namespace libcamera::ipa::common::algorithms {\n> +\n> +/**\n> + * \\class AfAlgorithm\n> + * \\brief Common interface for auto-focus algorithms\n> + * \\tparam Module The IPA module type for this class of algorithms\n> + *\n> + * The AfAlgorithm class defines a standard interface for IPA auto focus\n> + * algorithms.\n> + */\n> +\n> +/**\n> + * \\fn AfAlgorithm::setMode()\n> + * \\brief Set auto focus mode\n> + * \\param[in] mode AF mode\n> + */\n> +\n> +/**\n> + * \\fn AfAlgorithm::setRange()\n> + * \\brief set the range of focus distances that is scanned\n> + * \\param[in] range AF range\n> + */\n> +\n> +/**\n> + * \\fn AfAlgorithm::setSpeed()\n> + * \\brief Set how fast algorithm should move the lens\n> + * \\param[in] speed Lens move speed\n> + */\n> +\n> +/**\n> + * \\fn AfAlgorithm::setMetering()\n> + * \\brief Set AF metering mode\n> + * \\param[in] metering AF metering mode\n> + */\n> +\n> +/**\n> + * \\fn AfAlgorithm::setWindows()\n> + * \\brief Set AF windows\n> + * \\param[in] windows AF windows\n> + *\n> + * Sets the focus windows used by the AF algorithm when AfMetering is set\n> + * to AfMeteringWindows.\n> + */\n> +\n> +/**\n> + * \\fn AfAlgorithm::setTrigger()\n> + * \\brief Starts or cancels the autofocus scan\n> + * \\param[in] trigger Trigger mode\n> + */\n> +\n> +/**\n> + * \\fn AfAlgorithm::setPause()\n> + * \\brief Pause the autofocus while in AfModeContinuous mode.\n> + * \\param[in] pause Pause mode\n> + */\n> +\n> +/**\n> + * \\fn AfAlgorithm::setLensPosition()\n> + * \\brief Set the lens position while in AfModeManual\n> + * \\param[in] lensPosition Lens position\n> + */\n> +\n> +} /* namespace libcamera::ipa::common::algorithms */\n> diff --git a/src/ipa/libipa/algorithms/af_algorithm.h b/src/ipa/libipa/algorithms/af_algorithm.h\n> new file mode 100644\n> index 00000000..2862042b\n> --- /dev/null\n> +++ b/src/ipa/libipa/algorithms/af_algorithm.h\n> @@ -0,0 +1,41 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2022, Raspberry Pi (Trading) Limited\n> + * Copyright (C) 2022, Theobroma Systems\n> + *\n> + * af_algorithm.h - Autofocus control algorithm interface\n> + */\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \"../algorithm.h\"\n> +\n> +namespace libcamera::ipa::common::algorithms {\n> +\n> +template\n> +class AfAlgorithm : public Algorithm\n\nI wonder... do we need to bring Module up to this level ? We're\ndefining a pure virtual class here, so it's just an interface.\n\nSame for AfHillClimbing, does it need there ? it's not\nintended to be instantiated (it can't be if I'm not mistaken as it\ndoesn't define all the pure virtual methods of the base class).\n\nI have applied this patch, and made only the RkISP1 implementation\ninherit from Algorithm.\n\n--- a/src/ipa/libipa/algorithms/af_algorithm.h\n+++ b/src/ipa/libipa/algorithms/af_algorithm.h\n@@ -13,8 +13,7 @@\n\n namespace libcamera::ipa::common::algorithms {\n\n-template\n-class AfAlgorithm : public Algorithm\n+class AfAlgorithm\n {\n public:\n AfAlgorithm() = default;\ndiff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\nindex e251f3eb6000..33e2348c0fdd 100644\n--- a/src/ipa/libipa/algorithms/af_hill_climbing.h\n+++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n@@ -19,8 +19,7 @@ LOG_DECLARE_CATEGORY(Af)\n\n namespace ipa::common::algorithms {\n\n-template\n-class AfHillClimbing : public AfAlgorithm\n+class AfHillClimbing : public AfAlgorithm\n {\n public:\n AfHillClimbing()\ndiff --git a/src/ipa/rkisp1/algorithms/af.h b/src/ipa/rkisp1/algorithms/af.h\nindex b2da48904229..d27b5da703fc 100644\n--- a/src/ipa/rkisp1/algorithms/af.h\n+++ b/src/ipa/rkisp1/algorithms/af.h\n@@ -10,11 +10,11 @@\n #include \n\n #include \"libipa/algorithms/af_hill_climbing.h\"\n-#include \"module.h\"\n+#include \"algorithm.h\"\n\n namespace libcamera::ipa::rkisp1::algorithms {\n\n-class Af : public ipa::common::algorithms::AfHillClimbing\n+class Af : public ipa::common::algorithms::AfHillClimbing, public Algorithm\n {\n public:\n Af() = default;\n\nIsn't it better to keep the Module template argument in the bottom of\nthe hierarchy ?\n\n\n> +{\n> +public:\n> +\tAfAlgorithm() = default;\n> +\n> +\tvirtual ~AfAlgorithm() {}\n> +\n> +\tvirtual void setMode(controls::AfModeEnum mode) = 0;\n> +\n> +\tvirtual void setRange(controls::AfRangeEnum range) = 0;\n> +\n> +\tvirtual void setSpeed(controls::AfSpeedEnum speed) = 0;\n> +\n> +\tvirtual void setMetering(controls::AfMeteringEnum metering) = 0;\n> +\n> +\tvirtual void setWindows(Span windows) = 0;\n> +\n> +\tvirtual void setTrigger(controls::AfTriggerEnum trigger) = 0;\n> +\n> +\tvirtual void setPause(controls::AfPauseEnum pause) = 0;\n> +\n> +\tvirtual void setLensPosition(float lensPosition) = 0;\n\nThe interface mimics exactly the AF controls in the\napplication->libcamera direction, I wonder if we should expose from\nthe interface AfState and AfPauseState as well.\n\n> +};\n> +\n> +} /* namespace libcamera::ipa::common::algorithms */\n> diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> new file mode 100644\n> index 00000000..ab8da13a\n> --- /dev/null\n> +++ b/src/ipa/libipa/algorithms/meson.build\n> @@ -0,0 +1,9 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +common_ipa_algorithms_headers = files([\n> + 'af_algorithm.h',\n> +])\n> +\n> +common_ipa_algorithms_sources = files([\n> + 'af_algorithm.cpp',\n> +])\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index fb894bc6..1fc3fd56 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -1,5 +1,7 @@\n> # SPDX-License-Identifier: CC0-1.0\n>\n> +subdir('algorithms')\n> +\n> libipa_headers = files([\n> 'algorithm.h',\n> 'camera_sensor_helper.h',\n> @@ -7,6 +9,8 @@ libipa_headers = files([\n> 'module.h',\n> ])\n>\n> +libipa_headers += common_ipa_algorithms_headers\n> +\n> libipa_sources = files([\n> 'algorithm.cpp',\n> 'camera_sensor_helper.cpp',\n> @@ -14,6 +18,8 @@ libipa_sources = files([\n> 'module.cpp',\n> ])\n>\n> +libipa_sources += common_ipa_algorithms_sources\n> +\n> libipa_includes = include_directories('..')\n>\n> libipa = static_library('ipa', [libipa_sources, libipa_headers],\n> --\n> 2.34.1\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 D9040BD1F1\n\tfor ;\n\tThu, 14 Jul 2022 16:07:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3471163312;\n\tThu, 14 Jul 2022 18:07:07 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1706F6330D\n\tfor ;\n\tThu, 14 Jul 2022 18:07:06 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 49C0F40005;\n\tThu, 14 Jul 2022 16:07:05 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657814827;\n\tbh=evvF+vXRdtm2zXZhA+utZqUpHLaG4PHPbI5nW6M4esQ=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=j8mV8nZfQ0VIlSSdhxfGZs6cl9qqWFa8ZQFkPj/gTyhwjfOn78kZRPo+loOleM3bC\n\tWTZjG9BuANIQuG0Jp4V+lAxdY9N9l3EONiN11jtyTxWqz6pcHgjx1fhgp/bZxiqocc\n\tSwr5ogfOQ+t/bP1jj47Oug1vHaLhJM4tOhBpIoGyqTlT2/4/1jMsSRYPLA9zoCmco8\n\tTSc/vO47/bsx6amQPZrrZbj+3Tu6ofdoxaHxIDzCWr3uJlaaPXfRmoijqNYpiSwJ2n\n\tWHfrCR7LkVhdqRxXUusgh48T5SRQjFel/pbKizylLGwy9bwonNdgPHXY2xxn2igwqX\n\toIcfFTZTA9BdA==","Date":"Thu, 14 Jul 2022 18:07:03 +0200","To":"Daniel Semkowicz ","Message-ID":"<20220714160703.vssoijogyu6zacfj@uno.localdomain>","References":"<20220713084317.24268-1-dse@thaumatec.com>\n\t<20220713084317.24268-2-dse@thaumatec.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220713084317.24268-2-dse@thaumatec.com>","Subject":"Re: [libcamera-devel] [PATCH v2 01/11] ipa: Add base class defining\n\tAF algorithm interface","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23883,"web_url":"https://patchwork.libcamera.org/comment/23883/","msgid":"<20220714161825.2psr7lklv4p364s5@uno.localdomain>","date":"2022-07-14T16:18:25","subject":"Re: [libcamera-devel] [PATCH v2 01/11] ipa: Add base class defining\n\tAF algorithm interface","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Daniel,\n one more question\n\nOn Wed, Jul 13, 2022 at 10:43:07AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> Define common interface with basic functions that should be supported\n> by Auto Focus algorithms.\n>\n> Signed-off-by: Daniel Semkowicz \n> ---\n> src/ipa/libipa/algorithms/af_algorithm.cpp | 78 ++++++++++++++++++++++\n> src/ipa/libipa/algorithms/af_algorithm.h | 41 ++++++++++++\n> src/ipa/libipa/algorithms/meson.build | 9 +++\n> src/ipa/libipa/meson.build | 6 ++\n> 4 files changed, 134 insertions(+)\n> create mode 100644 src/ipa/libipa/algorithms/af_algorithm.cpp\n> create mode 100644 src/ipa/libipa/algorithms/af_algorithm.h\n> create mode 100644 src/ipa/libipa/algorithms/meson.build\n>\n> diff --git a/src/ipa/libipa/algorithms/af_algorithm.cpp b/src/ipa/libipa/algorithms/af_algorithm.cpp\n> new file mode 100644\n> index 00000000..47e54d5a\n> --- /dev/null\n> +++ b/src/ipa/libipa/algorithms/af_algorithm.cpp\n> @@ -0,0 +1,78 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n\nAny particular reason to go for BSD ?\n\n> +/*\n> + * Copyright (C) 2022, Raspberry Pi (Trading) Limited\n> + * Copyright (C) 2022, Theobroma Systems\n> + *\n> + * af_algorithm.cpp - Autofocus control algorithm interface\n> + */\n> +\n> +#include \"af_algorithm.h\"\n> +\n> +/**\n> + * \\file af_algorithm.h\n> + * \\brief AF algorithm common interface\n> + */\n> +\n> +namespace libcamera::ipa::common::algorithms {\n> +\n> +/**\n> + * \\class AfAlgorithm\n> + * \\brief Common interface for auto-focus algorithms\n> + * \\tparam Module The IPA module type for this class of algorithms\n> + *\n> + * The AfAlgorithm class defines a standard interface for IPA auto focus\n> + * algorithms.\n> + */\n> +\n> +/**\n> + * \\fn AfAlgorithm::setMode()\n> + * \\brief Set auto focus mode\n> + * \\param[in] mode AF mode\n> + */\n> +\n> +/**\n> + * \\fn AfAlgorithm::setRange()\n> + * \\brief set the range of focus distances that is scanned\n> + * \\param[in] range AF range\n> + */\n> +\n> +/**\n> + * \\fn AfAlgorithm::setSpeed()\n> + * \\brief Set how fast algorithm should move the lens\n> + * \\param[in] speed Lens move speed\n> + */\n> +\n> +/**\n> + * \\fn AfAlgorithm::setMetering()\n> + * \\brief Set AF metering mode\n> + * \\param[in] metering AF metering mode\n> + */\n> +\n> +/**\n> + * \\fn AfAlgorithm::setWindows()\n> + * \\brief Set AF windows\n> + * \\param[in] windows AF windows\n> + *\n> + * Sets the focus windows used by the AF algorithm when AfMetering is set\n> + * to AfMeteringWindows.\n> + */\n> +\n> +/**\n> + * \\fn AfAlgorithm::setTrigger()\n> + * \\brief Starts or cancels the autofocus scan\n> + * \\param[in] trigger Trigger mode\n> + */\n> +\n> +/**\n> + * \\fn AfAlgorithm::setPause()\n> + * \\brief Pause the autofocus while in AfModeContinuous mode.\n> + * \\param[in] pause Pause mode\n> + */\n> +\n> +/**\n> + * \\fn AfAlgorithm::setLensPosition()\n> + * \\brief Set the lens position while in AfModeManual\n> + * \\param[in] lensPosition Lens position\n> + */\n> +\n> +} /* namespace libcamera::ipa::common::algorithms */\n> diff --git a/src/ipa/libipa/algorithms/af_algorithm.h b/src/ipa/libipa/algorithms/af_algorithm.h\n> new file mode 100644\n> index 00000000..2862042b\n> --- /dev/null\n> +++ b/src/ipa/libipa/algorithms/af_algorithm.h\n> @@ -0,0 +1,41 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2022, Raspberry Pi (Trading) Limited\n> + * Copyright (C) 2022, Theobroma Systems\n> + *\n> + * af_algorithm.h - Autofocus control algorithm interface\n> + */\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \"../algorithm.h\"\n> +\n> +namespace libcamera::ipa::common::algorithms {\n> +\n> +template\n> +class AfAlgorithm : public Algorithm\n> +{\n> +public:\n> +\tAfAlgorithm() = default;\n> +\n> +\tvirtual ~AfAlgorithm() {}\n> +\n> +\tvirtual void setMode(controls::AfModeEnum mode) = 0;\n> +\n> +\tvirtual void setRange(controls::AfRangeEnum range) = 0;\n> +\n> +\tvirtual void setSpeed(controls::AfSpeedEnum speed) = 0;\n> +\n> +\tvirtual void setMetering(controls::AfMeteringEnum metering) = 0;\n> +\n> +\tvirtual void setWindows(Span windows) = 0;\n> +\n> +\tvirtual void setTrigger(controls::AfTriggerEnum trigger) = 0;\n> +\n> +\tvirtual void setPause(controls::AfPauseEnum pause) = 0;\n> +\n> +\tvirtual void setLensPosition(float lensPosition) = 0;\n> +};\n> +\n> +} /* namespace libcamera::ipa::common::algorithms */\n> diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> new file mode 100644\n> index 00000000..ab8da13a\n> --- /dev/null\n> +++ b/src/ipa/libipa/algorithms/meson.build\n> @@ -0,0 +1,9 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +common_ipa_algorithms_headers = files([\n> + 'af_algorithm.h',\n> +])\n> +\n> +common_ipa_algorithms_sources = files([\n> + 'af_algorithm.cpp',\n> +])\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index fb894bc6..1fc3fd56 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -1,5 +1,7 @@\n> # SPDX-License-Identifier: CC0-1.0\n>\n> +subdir('algorithms')\n> +\n> libipa_headers = files([\n> 'algorithm.h',\n> 'camera_sensor_helper.h',\n> @@ -7,6 +9,8 @@ libipa_headers = files([\n> 'module.h',\n> ])\n>\n> +libipa_headers += common_ipa_algorithms_headers\n> +\n> libipa_sources = files([\n> 'algorithm.cpp',\n> 'camera_sensor_helper.cpp',\n> @@ -14,6 +18,8 @@ libipa_sources = files([\n> 'module.cpp',\n> ])\n>\n> +libipa_sources += common_ipa_algorithms_sources\n> +\n> libipa_includes = include_directories('..')\n>\n> libipa = static_library('ipa', [libipa_sources, libipa_headers],\n> --\n> 2.34.1\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 E5AC7BE173\n\tfor ;\n\tThu, 14 Jul 2022 16:18:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 96C1063312;\n\tThu, 14 Jul 2022 18:18:30 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 60D716330D\n\tfor ;\n\tThu, 14 Jul 2022 18:18:28 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 9BC791C0005;\n\tThu, 14 Jul 2022 16:18:27 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657815510;\n\tbh=Ug6zNQHbmxfgJO8mKnwC9dztvTZGF52k0rDkjh5STcE=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=vcQakT8rF2KSbizotcN0eipXI9gx5hLhQY5XMZFO5Se1AyVgZNpERqi7GMdhKZ2wE\n\tbTQr7UG6KFEss7DY9r/29zmk6lF9+C+pSEZ4g3H4Tnp9wp8i4F3OD4FbfhaBP2FgCS\n\tfoagT8j89UYE/bYm10JkuERh3RB2/7XysL6zXpW3YKYpQ8/DZEUq9qrYW1Ax3Ew9uI\n\tWMR3aSh7asncyTFvkB+jSs7y7Y9MoNP9a98pI2E2s4oPXmTVojTINVlkafmvVfWmhD\n\t8DBHtItEdngr65Tt2uIPeMp5hydqV1jP2hCCGkuD8C1nA6Wvnr0DTWYxxmwwDLMFAV\n\t3+CUqJ/j6qskA==","Date":"Thu, 14 Jul 2022 18:18:25 +0200","To":"Daniel Semkowicz ","Message-ID":"<20220714161825.2psr7lklv4p364s5@uno.localdomain>","References":"<20220713084317.24268-1-dse@thaumatec.com>\n\t<20220713084317.24268-2-dse@thaumatec.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220713084317.24268-2-dse@thaumatec.com>","Subject":"Re: [libcamera-devel] [PATCH v2 01/11] ipa: Add base class defining\n\tAF algorithm interface","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23951,"web_url":"https://patchwork.libcamera.org/comment/23951/","msgid":"","date":"2022-07-18T14:40:06","subject":"Re: [libcamera-devel] [PATCH v2 01/11] ipa: Add base class defining\n\tAF algorithm interface","submitter":{"id":126,"url":"https://patchwork.libcamera.org/api/people/126/","name":"Daniel Semkowicz","email":"dse@thaumatec.com"},"content":"Hi Jacopo,\n\nOn Thu, Jul 14, 2022 at 6:07 PM Jacopo Mondi wrote:\n>\n> Hi Daniel,\n> I like -very much- how you have split the algorithm in the platform\n> dependent part and in the generic part! very nice\n>\n> On Wed, Jul 13, 2022 at 10:43:07AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > Define common interface with basic functions that should be supported\n> > by Auto Focus algorithms.\n> >\n> > Signed-off-by: Daniel Semkowicz \n> > ---\n> > src/ipa/libipa/algorithms/af_algorithm.cpp | 78 ++++++++++++++++++++++\n> > src/ipa/libipa/algorithms/af_algorithm.h | 41 ++++++++++++\n> > src/ipa/libipa/algorithms/meson.build | 9 +++\n> > src/ipa/libipa/meson.build | 6 ++\n> > 4 files changed, 134 insertions(+)\n> > create mode 100644 src/ipa/libipa/algorithms/af_algorithm.cpp\n> > create mode 100644 src/ipa/libipa/algorithms/af_algorithm.h\n> > create mode 100644 src/ipa/libipa/algorithms/meson.build\n> >\n> > diff --git a/src/ipa/libipa/algorithms/af_algorithm.cpp b/src/ipa/libipa/algorithms/af_algorithm.cpp\n> > new file mode 100644\n> > index 00000000..47e54d5a\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/algorithms/af_algorithm.cpp\n> > @@ -0,0 +1,78 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n>\n> Any particular reason to go for BSD ?\n>\n>\n> > +/*\n> > + * Copyright (C) 2022, Raspberry Pi (Trading) Limited\n> > + * Copyright (C) 2022, Theobroma Systems\n>\n> For this and the other new files, is this your copyright ?\n>\n\nMy copyright is Theobroma Systems. I added Raspberry Pi as I was basing\npartially on their work and their code was published as BSD-2. It was\nhard for me to decide if it was derived work or amount of changes made\nit completely different, so I assumed the first one to be safe. If you\nthink this is independent change then I am ok with LGPL as I see the most\npart of the library use it.\n\n\n> > + *\n> > + * af_algorithm.cpp - Autofocus control algorithm interface\n> > + */\n> > +\n> > +#include \"af_algorithm.h\"\n> > +\n> > +/**\n> > + * \\file af_algorithm.h\n> > + * \\brief AF algorithm common interface\n> > + */\n> > +\n> > +namespace libcamera::ipa::common::algorithms {\n> > +\n> > +/**\n> > + * \\class AfAlgorithm\n> > + * \\brief Common interface for auto-focus algorithms\n> > + * \\tparam Module The IPA module type for this class of algorithms\n> > + *\n> > + * The AfAlgorithm class defines a standard interface for IPA auto focus\n> > + * algorithms.\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfAlgorithm::setMode()\n> > + * \\brief Set auto focus mode\n> > + * \\param[in] mode AF mode\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfAlgorithm::setRange()\n> > + * \\brief set the range of focus distances that is scanned\n> > + * \\param[in] range AF range\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfAlgorithm::setSpeed()\n> > + * \\brief Set how fast algorithm should move the lens\n> > + * \\param[in] speed Lens move speed\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfAlgorithm::setMetering()\n> > + * \\brief Set AF metering mode\n> > + * \\param[in] metering AF metering mode\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfAlgorithm::setWindows()\n> > + * \\brief Set AF windows\n> > + * \\param[in] windows AF windows\n> > + *\n> > + * Sets the focus windows used by the AF algorithm when AfMetering is set\n> > + * to AfMeteringWindows.\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfAlgorithm::setTrigger()\n> > + * \\brief Starts or cancels the autofocus scan\n> > + * \\param[in] trigger Trigger mode\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfAlgorithm::setPause()\n> > + * \\brief Pause the autofocus while in AfModeContinuous mode.\n> > + * \\param[in] pause Pause mode\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfAlgorithm::setLensPosition()\n> > + * \\brief Set the lens position while in AfModeManual\n> > + * \\param[in] lensPosition Lens position\n> > + */\n> > +\n> > +} /* namespace libcamera::ipa::common::algorithms */\n> > diff --git a/src/ipa/libipa/algorithms/af_algorithm.h b/src/ipa/libipa/algorithms/af_algorithm.h\n> > new file mode 100644\n> > index 00000000..2862042b\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/algorithms/af_algorithm.h\n> > @@ -0,0 +1,41 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2022, Raspberry Pi (Trading) Limited\n> > + * Copyright (C) 2022, Theobroma Systems\n> > + *\n> > + * af_algorithm.h - Autofocus control algorithm interface\n> > + */\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \"../algorithm.h\"\n> > +\n> > +namespace libcamera::ipa::common::algorithms {\n> > +\n> > +template\n> > +class AfAlgorithm : public Algorithm\n>\n> I wonder... do we need to bring Module up to this level ? We're\n> defining a pure virtual class here, so it's just an interface.\n>\n> Same for AfHillClimbing, does it need there ? it's not\n> intended to be instantiated (it can't be if I'm not mistaken as it\n> doesn't define all the pure virtual methods of the base class).\n>\n> I have applied this patch, and made only the RkISP1 implementation\n> inherit from Algorithm.\n>\n> --- a/src/ipa/libipa/algorithms/af_algorithm.h\n> +++ b/src/ipa/libipa/algorithms/af_algorithm.h\n> @@ -13,8 +13,7 @@\n>\n> namespace libcamera::ipa::common::algorithms {\n>\n> -template\n> -class AfAlgorithm : public Algorithm\n> +class AfAlgorithm\n> {\n> public:\n> AfAlgorithm() = default;\n> diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> index e251f3eb6000..33e2348c0fdd 100644\n> --- a/src/ipa/libipa/algorithms/af_hill_climbing.h\n> +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> @@ -19,8 +19,7 @@ LOG_DECLARE_CATEGORY(Af)\n>\n> namespace ipa::common::algorithms {\n>\n> -template\n> -class AfHillClimbing : public AfAlgorithm\n> +class AfHillClimbing : public AfAlgorithm\n> {\n> public:\n> AfHillClimbing()\n> diff --git a/src/ipa/rkisp1/algorithms/af.h b/src/ipa/rkisp1/algorithms/af.h\n> index b2da48904229..d27b5da703fc 100644\n> --- a/src/ipa/rkisp1/algorithms/af.h\n> +++ b/src/ipa/rkisp1/algorithms/af.h\n> @@ -10,11 +10,11 @@\n> #include \n>\n> #include \"libipa/algorithms/af_hill_climbing.h\"\n> -#include \"module.h\"\n> +#include \"algorithm.h\"\n>\n> namespace libcamera::ipa::rkisp1::algorithms {\n>\n> -class Af : public ipa::common::algorithms::AfHillClimbing\n> +class Af : public ipa::common::algorithms::AfHillClimbing, public Algorithm\n> {\n> public:\n> Af() = default;\n>\n> Isn't it better to keep the Module template argument in the bottom of\n> the hierarchy ?\n>\n\nI did this to avoid the multiple inheritance. It is more clear if there\nis only one direct path for interface and implementation. Unfortunately,\nthe drawback here is that We need to pass the Module argument from top\nto bottom. It is the choice between scenarios that both have some\ndrawbacks. I would love to hear additional opinions :)\nIf We go for the multiple inheritance approach, then I think it would be\ngood to change naming a bit, as it would be a bit misleading now to have\nAfAlgorithm and Algorithm inheritance. Both sound as almost the same\nthing.\n\n>\n> > +{\n> > +public:\n> > + AfAlgorithm() = default;\n> > +\n> > + virtual ~AfAlgorithm() {}\n> > +\n> > + virtual void setMode(controls::AfModeEnum mode) = 0;\n> > +\n> > + virtual void setRange(controls::AfRangeEnum range) = 0;\n> > +\n> > + virtual void setSpeed(controls::AfSpeedEnum speed) = 0;\n> > +\n> > + virtual void setMetering(controls::AfMeteringEnum metering) = 0;\n> > +\n> > + virtual void setWindows(Span windows) = 0;\n> > +\n> > + virtual void setTrigger(controls::AfTriggerEnum trigger) = 0;\n> > +\n> > + virtual void setPause(controls::AfPauseEnum pause) = 0;\n> > +\n> > + virtual void setLensPosition(float lensPosition) = 0;\n>\n> The interface mimics exactly the AF controls in the\n> application->libcamera direction, I wonder if we should expose from\n> the interface AfState and AfPauseState as well.\n\nI also had this in mind and I totally agree with that. I just wanted to\npush the minimal working code as it can be later extended. However, We\ncan already put it in the interface and add implementation later.\n\nBest regards\nDaniel\n\n>\n> > +};\n> > +\n> > +} /* namespace libcamera::ipa::common::algorithms */\n> > diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> > new file mode 100644\n> > index 00000000..ab8da13a\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/algorithms/meson.build\n> > @@ -0,0 +1,9 @@\n> > +# SPDX-License-Identifier: CC0-1.0\n> > +\n> > +common_ipa_algorithms_headers = files([\n> > + 'af_algorithm.h',\n> > +])\n> > +\n> > +common_ipa_algorithms_sources = files([\n> > + 'af_algorithm.cpp',\n> > +])\n> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > index fb894bc6..1fc3fd56 100644\n> > --- a/src/ipa/libipa/meson.build\n> > +++ b/src/ipa/libipa/meson.build\n> > @@ -1,5 +1,7 @@\n> > # SPDX-License-Identifier: CC0-1.0\n> >\n> > +subdir('algorithms')\n> > +\n> > libipa_headers = files([\n> > 'algorithm.h',\n> > 'camera_sensor_helper.h',\n> > @@ -7,6 +9,8 @@ libipa_headers = files([\n> > 'module.h',\n> > ])\n> >\n> > +libipa_headers += common_ipa_algorithms_headers\n> > +\n> > libipa_sources = files([\n> > 'algorithm.cpp',\n> > 'camera_sensor_helper.cpp',\n> > @@ -14,6 +18,8 @@ libipa_sources = files([\n> > 'module.cpp',\n> > ])\n> >\n> > +libipa_sources += common_ipa_algorithms_sources\n> > +\n> > libipa_includes = include_directories('..')\n> >\n> > libipa = static_library('ipa', [libipa_sources, libipa_headers],\n> > --\n> > 2.34.1\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 F1844BE173\n\tfor ;\n\tMon, 18 Jul 2022 14:40:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6AF9C63312;\n\tMon, 18 Jul 2022 16:40:20 +0200 (CEST)","from mail-lj1-x229.google.com (mail-lj1-x229.google.com\n\t[IPv6:2a00:1450:4864:20::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EDFFF6048B\n\tfor ;\n\tMon, 18 Jul 2022 16:40:18 +0200 (CEST)","by mail-lj1-x229.google.com with SMTP id u14so13864186ljh.2\n\tfor ;\n\tMon, 18 Jul 2022 07:40:18 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658155220;\n\tbh=2t0JIjnrrnVonxdx0sHKdXUphB7ASfOPZWREgn5lb+4=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Va0TZN2hXukmr/PJw+H8SA0hjIcnB++s359xaDhMkRsFVByhl/ZayAV/eRYGKcMfj\n\t0SeNpszkoakiFJMbEuQquhMwTJzZuV04OYdB8Wb6p2e7omYstZgHE8bgKW9WlLUKwy\n\tCP6nauEn6tIEfTSLPWrs0YHe6+9slmdRs0qTuqgqlo7BfuwwkZr7y9FBImNcTZ7qOJ\n\tJ8gQU2NesJtsmIz49y+OE4U+YKKDGedS2SYvsmYcNqod7WxIpU2IIROK+mLLZwnt8q\n\tpj7qU9AWvMyhQQBRE5iPeAIznv4fjOJkibO5GD4X5jD1gCiDpV2pTYYMh1lLGAm7P+\n\tbXCANHS3daaTg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20210112.gappssmtp.com; s=20210112;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=quS1mJVAZSBjDxtYfnDW6lqmQd1rkW2X+12Npc9i5gY=;\n\tb=h7cX0AVqpelli9YFXWZgQVr6tXJt31NYX9LBXNSUtwV9JVydtAr2g2oL6tGLYXEGpF\n\trR2XyUSkU1HRkNTj65Qyd+mhLaUis5zOebATPriWF1fmqW4dRSZlIp+xGx4bGI2o2qQO\n\tZHEHiteWGoubc/zBIowdxRDw/7uA1J7lH/bRTF6J10mpvEMX2QuiyDjBhc4m6IEmw7KU\n\thaP3p8I1Ijpk+klEOFrkx1dxv7PHaeWTeQSJYV5Ej0G4k8+5T9fc2pjBQW4k4mrjFBwm\n\tHvrgPQP/znuEfSGq4AN3aRM8+I/lKW6y1yI4Mt0O1ShsuQr5QDWzj/hYqJ2XIlCF2rbY\n\tJQ/A=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=thaumatec-com.20210112.gappssmtp.com\n\theader.i=@thaumatec-com.20210112.gappssmtp.com header.b=\"h7cX0AVq\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=quS1mJVAZSBjDxtYfnDW6lqmQd1rkW2X+12Npc9i5gY=;\n\tb=USqZnoTbFv1PDHKnOYoNO8+cNHGCu1rnrcREH65A9Ljk4ne+o8ybih1bsmjtY2GwKn\n\tPLdeypo5+JVYLe9ViJ0dqtkNHg5s4Ffw7Nvpi7sO3RdMyisAcZ7m3O/ICGiNsR1y/gSA\n\tb5DUY56BZdtOtv4Qxg0s1M4y+wP2pK7+DohCQy4VipZ+FnRbVYkJGdqbfpPoPrXDhTk9\n\tY89V3jhEEHr6En3maVH8dwc2mPPNyNex9j3uPl4ByTYyyUFTEKe2DgWgS4Y3Kxmxz3mA\n\tDoTRdIrZejVbpkdxscPkckprwkhnFos0LnsIF5baMje/fzevYHILyd1Kz1Dbhf9/6ese\n\tpJsQ==","X-Gm-Message-State":"AJIora/jta5PpOQbn2fn3rNwQlr74ahDAJypLnq4fFXFnxTlALwWbz+1\n\tz+Krr8SaFUvKwIFhgJnBUkiiQUuP8R7+W6uK4hgZCQ==","X-Google-Smtp-Source":"AGRyM1tAMWm85R4mNUZ6/mqBQ1UvqEp7mIiTGVXAPCDFhbtGcQ0kNWo1dRRZ5Z21tjS+YEKMBK8xAPMmt3QyfyAgh4I=","X-Received":"by 2002:a2e:8e8b:0:b0:25d:ae46:6bea with SMTP id\n\tz11-20020a2e8e8b000000b0025dae466beamr4519446ljk.283.1658155218228;\n\tMon, 18 Jul 2022 07:40:18 -0700 (PDT)","MIME-Version":"1.0","References":"<20220713084317.24268-1-dse@thaumatec.com>\n\t<20220713084317.24268-2-dse@thaumatec.com>\n\t<20220714160703.vssoijogyu6zacfj@uno.localdomain>","In-Reply-To":"<20220714160703.vssoijogyu6zacfj@uno.localdomain>","Date":"Mon, 18 Jul 2022 16:40:06 +0200","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 01/11] ipa: Add base class defining\n\tAF algorithm interface","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","From":"Daniel Semkowicz via libcamera-devel\n\t","Reply-To":"Daniel Semkowicz ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]