[{"id":26403,"web_url":"https://patchwork.libcamera.org/comment/26403/","msgid":"<20230206111958.aqlhed5mcht3feyl@uno.localdomain>","date":"2023-02-06T11:19:58","subject":"Re: [libcamera-devel] [PATCH v3 3/8] ipa: Add base class defining\n\tAF algorithm interface","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Daniel\n\nOn Thu, Jan 19, 2023 at 09:41:07AM +0100, Daniel Semkowicz via libcamera-devel wrote:\n> Define common interface with basic functions that should be supported\n> by Auto Focus algorithms.\n>\n\nAs commented in the previous version, I like this approach very much\n\n> Signed-off-by: Daniel Semkowicz \n> ---\n> src/ipa/libipa/algorithms/af_interface.cpp | 92 ++++++++++++++++++++++\n> src/ipa/libipa/algorithms/af_interface.h | 41 ++++++++++\n> src/ipa/libipa/algorithms/meson.build | 9 +++\n> src/ipa/libipa/meson.build | 6 ++\n> 4 files changed, 148 insertions(+)\n> create mode 100644 src/ipa/libipa/algorithms/af_interface.cpp\n> create mode 100644 src/ipa/libipa/algorithms/af_interface.h\n> create mode 100644 src/ipa/libipa/algorithms/meson.build\n>\n> diff --git a/src/ipa/libipa/algorithms/af_interface.cpp b/src/ipa/libipa/algorithms/af_interface.cpp\n> new file mode 100644\n> index 00000000..af341d13\n> --- /dev/null\n> +++ b/src/ipa/libipa/algorithms/af_interface.cpp\n\nNit: af_interface.[cpp|h] or just af.[cpp|h] ?\n\n> @@ -0,0 +1,92 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022, Theobroma Systems\n> + *\n> + * af_interface.cpp - Autofocus control algorithm interface\n> + */\n> +\n> +#include \"af_interface.h\"\n> +\n> +/**\n> + * \\file af_interface.h\n> + * \\brief AF algorithm common interface\n> + */\n> +\n> +namespace libcamera::ipa::common::algorithms {\n> +\n> +/**\n> + * \\class AfInterface\n> + * \\brief Common interface for auto-focus algorithms\n> + * \\tparam Module The IPA module type for this class of algorithms\n\nYou don't have the Module template argument anymore\n\n> + *\n> + * The AfInterface class defines a standard interface for IPA auto focus\n> + * algorithms.\n> + */\n> +\n> +/**\n> + * \\fn AfInterface::setMode()\n> + * \\brief Set auto focus mode\n> + * \\param[in] mode AF mode\n\nI was about to suggest \\sa controls::AfModeEnum but doxygen already\nlinks the parameter type to the right documentation page, so it's not\nnecessary.\n\nHowever, as this interface implements the Af controls defined in\ncontrol_ids.yaml I would be tempted to say that we might even /copydoc\nso that the documentation is centralized in a single place ??\n\n\n> + */\n> +\n> +/**\n> + * \\fn AfInterface::setRange()\n> + * \\brief set the range of focus distances that is scanned\n> + * \\param[in] range AF range\n> + */\n> +\n> +/**\n> + * \\fn AfInterface::setSpeed()\n> + * \\brief Set how fast algorithm should move the lens\n> + * \\param[in] speed Lens move speed\n> + */\n> +\n> +/**\n> + * \\fn AfInterface::setMeteringMode()\n> + * \\brief Set AF metering mode\n> + * \\param[in] metering AF metering mode\n> + */\n> +\n> +/**\n> + * \\fn AfInterface::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 AfInterface::setTrigger()\n> + * \\brief Starts or cancels the autofocus scan\n> + * \\param[in] trigger Trigger mode\n> + */\n> +\n> +/**\n> + * \\fn AfInterface::setPause()\n> + * \\brief Pause the autofocus while in AfModeContinuous mode.\n> + * \\param[in] pause Pause mode\n> + */\n> +\n> +/**\n> + * \\fn AfInterface::setLensPosition()\n> + * \\brief Set the lens position while in AfModeManual\n> + * \\param[in] lensPosition Lens position\n> + */\n> +\n> +/**\n> + * \\fn AfInterface::getState()\n\nMinor nit: for getters we don't usually prepend \"get\", so this could\njust be AfInterface::state() ?\n\n> + * \\brief Get the current state of the AF algorithm\n> + * \\return AF state\n> + */\n> +\n> +/**\n> + * \\fn AfInterface::getPauseState()\n> + * \\brief Get the current pause state of the AF algorithm.\n> + * \\return AF pause state\n> + *\n> + * Only applicable in continuous (AfModeContinuous) mode. In other modes,\n> + * AfPauseStateRunning is always returned.\n\nSee, in this case the documentation of controls::AfPauseState has gone\nthrough a lot of discussions, and trying to resume it here might not\nbe ideal. Plus it has to be kept in sync with the control\ndocumentation.\n\nI tried\n\n \\copydoc libcamera::controls::AfPauseState\n\nbut it doesn't work here..\n\n\n> + */\n> +\n> +} /* namespace libcamera::ipa::common::algorithms */\n> diff --git a/src/ipa/libipa/algorithms/af_interface.h b/src/ipa/libipa/algorithms/af_interface.h\n> new file mode 100644\n> index 00000000..b6b7bea6\n> --- /dev/null\n> +++ b/src/ipa/libipa/algorithms/af_interface.h\n> @@ -0,0 +1,41 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022, Theobroma Systems\n> + *\n> + * af_interface.h - Autofocus control algorithm interface\n> + */\n> +#pragma once\n> +\n> +#include \n> +\n> +namespace libcamera::ipa::common::algorithms {\n> +\n> +class AfInterface\n> +{\n> +public:\n> +\tAfInterface() = default;\n\nDo you even need a constructor ?\n\n> +\n> +\tvirtual ~AfInterface() {}\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 setMeteringMode(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> +\tvirtual controls::AfStateEnum getState() = 0;\n> +\n> +\tvirtual controls::AfPauseStateEnum getPauseState() = 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..0a1f18fa\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_interface.h',\n> +])\n> +\n> +common_ipa_algorithms_sources = files([\n> + 'af_interface.cpp',\n> +])\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 016b8e0e..0cfc551a 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> @@ -8,6 +10,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> @@ -16,6 +20,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\nMinors on the doc apart, it seems all good to me.\n\nLet me copy RPi folks in, as they just upstreamed a PDAF based\nauto-focus algorithm to check with them if there's a chance their\nimplementation can inherit from this class.\n\nThanks again!\n j\n> --\n> 2.39.0\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 472F9BEFBE\n\tfor ;\n\tMon, 6 Feb 2023 11:20:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 60FE461EF4;\n\tMon, 6 Feb 2023 12:20:04 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6BC83603B8\n\tfor ;\n\tMon, 6 Feb 2023 12:20:02 +0100 (CET)","from ideasonboard.com (host-79-55-56-167.retail.telecomitalia.it\n\t[79.55.56.167])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B0BA74DA;\n\tMon, 6 Feb 2023 12:20:01 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675682404;\n\tbh=Vrba6aK7X5NJsfM0arhoyzT3PXrAhwt6DPMm0kSKJaU=;\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=Y9vVSeD2+powuCRYp9aHu8+ab6bbtoLAj5NN8MPDxjVuTct06qDjYgM1Zk0cmy/Qi\n\tCj9RXE1kXlvp2TJwb82JumsT7ACDR22Ur8F1/zQM+VnbK3MwK1ahafXCU4AoDY4SWJ\n\tyJh2hvKMFGNeTdkyWpKrNCwdA6DWH/mJK9o3I6bTZ2PzmM6cWYoYAF3ztXzicwrBx5\n\tDxqjcoPle3hOITw2ZIXROeH8nBJtJvNchY65KZ/Jg1C2Nualh9O5y2PLQ8HQ+7lKO5\n\t/YPdWgfuXqgi/+TIL0LCFU23gZFyAVX/WXMrUn2S1tmFnrZnAyRHI/gMup6Uja176V\n\tSjowSAo/yj3Hw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1675682402;\n\tbh=Vrba6aK7X5NJsfM0arhoyzT3PXrAhwt6DPMm0kSKJaU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=q49/SCGfj/aCk0jCd920PyiOP3FZ31s22zcZewOtiCBMq4hRBn/F2xmYU3O/MzeUO\n\tceV1Awv7T6KNN2TELWZj3EgbyWA2dQ4sCCBoB6D0dB9Tu9ZbyKzWkDku+sAlLH101j\n\tFdxOGQpjgeLkbvrpMzH/xjiDT6XM2J0s/WAw/If8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"q49/SCGf\"; dkim-atps=neutral","Date":"Mon, 6 Feb 2023 12:19:58 +0100","To":"Daniel Semkowicz ,\n\tDavid Plowman ,\n\tNaushir Patuck ,\n\tNick Hollinghurst ","Message-ID":"<20230206111958.aqlhed5mcht3feyl@uno.localdomain>","References":"<20230119084112.20564-1-dse@thaumatec.com>\n\t<20230119084112.20564-4-dse@thaumatec.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230119084112.20564-4-dse@thaumatec.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/8] 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":26405,"web_url":"https://patchwork.libcamera.org/comment/26405/","msgid":"","date":"2023-02-06T11:39:56","subject":"Re: [libcamera-devel] [PATCH v3 3/8] ipa: Add base class defining\n\tAF algorithm interface","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo and Daniel,\n\nOn Mon, 6 Feb 2023 at 11:20, Jacopo Mondi wrote:\n>\n> Hi Daniel\n>\n> On Thu, Jan 19, 2023 at 09:41:07AM +0100, Daniel Semkowicz via libcamera-devel wrote:\n> > Define common interface with basic functions that should be supported\n> > by Auto Focus algorithms.\n> >\n>\n> As commented in the previous version, I like this approach very much\n>\n> > Signed-off-by: Daniel Semkowicz \n> > ---\n> > src/ipa/libipa/algorithms/af_interface.cpp | 92 ++++++++++++++++++++++\n> > src/ipa/libipa/algorithms/af_interface.h | 41 ++++++++++\n> > src/ipa/libipa/algorithms/meson.build | 9 +++\n> > src/ipa/libipa/meson.build | 6 ++\n> > 4 files changed, 148 insertions(+)\n> > create mode 100644 src/ipa/libipa/algorithms/af_interface.cpp\n> > create mode 100644 src/ipa/libipa/algorithms/af_interface.h\n> > create mode 100644 src/ipa/libipa/algorithms/meson.build\n> >\n> > diff --git a/src/ipa/libipa/algorithms/af_interface.cpp b/src/ipa/libipa/algorithms/af_interface.cpp\n> > new file mode 100644\n> > index 00000000..af341d13\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/algorithms/af_interface.cpp\n>\n> Nit: af_interface.[cpp|h] or just af.[cpp|h] ?\n>\n> > @@ -0,0 +1,92 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2022, Theobroma Systems\n> > + *\n> > + * af_interface.cpp - Autofocus control algorithm interface\n> > + */\n> > +\n> > +#include \"af_interface.h\"\n> > +\n> > +/**\n> > + * \\file af_interface.h\n> > + * \\brief AF algorithm common interface\n> > + */\n> > +\n> > +namespace libcamera::ipa::common::algorithms {\n> > +\n> > +/**\n> > + * \\class AfInterface\n> > + * \\brief Common interface for auto-focus algorithms\n> > + * \\tparam Module The IPA module type for this class of algorithms\n>\n> You don't have the Module template argument anymore\n>\n> > + *\n> > + * The AfInterface class defines a standard interface for IPA auto focus\n> > + * algorithms.\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfInterface::setMode()\n> > + * \\brief Set auto focus mode\n> > + * \\param[in] mode AF mode\n>\n> I was about to suggest \\sa controls::AfModeEnum but doxygen already\n> links the parameter type to the right documentation page, so it's not\n> necessary.\n>\n> However, as this interface implements the Af controls defined in\n> control_ids.yaml I would be tempted to say that we might even /copydoc\n> so that the documentation is centralized in a single place ??\n>\n>\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfInterface::setRange()\n> > + * \\brief set the range of focus distances that is scanned\n> > + * \\param[in] range AF range\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfInterface::setSpeed()\n> > + * \\brief Set how fast algorithm should move the lens\n> > + * \\param[in] speed Lens move speed\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfInterface::setMeteringMode()\n> > + * \\brief Set AF metering mode\n> > + * \\param[in] metering AF metering mode\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfInterface::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 AfInterface::setTrigger()\n> > + * \\brief Starts or cancels the autofocus scan\n> > + * \\param[in] trigger Trigger mode\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfInterface::setPause()\n> > + * \\brief Pause the autofocus while in AfModeContinuous mode.\n> > + * \\param[in] pause Pause mode\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfInterface::setLensPosition()\n> > + * \\brief Set the lens position while in AfModeManual\n> > + * \\param[in] lensPosition Lens position\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfInterface::getState()\n>\n> Minor nit: for getters we don't usually prepend \"get\", so this could\n> just be AfInterface::state() ?\n>\n> > + * \\brief Get the current state of the AF algorithm\n> > + * \\return AF state\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfInterface::getPauseState()\n> > + * \\brief Get the current pause state of the AF algorithm.\n> > + * \\return AF pause state\n> > + *\n> > + * Only applicable in continuous (AfModeContinuous) mode. In other modes,\n> > + * AfPauseStateRunning is always returned.\n>\n> See, in this case the documentation of controls::AfPauseState has gone\n> through a lot of discussions, and trying to resume it here might not\n> be ideal. Plus it has to be kept in sync with the control\n> documentation.\n>\n> I tried\n>\n> \\copydoc libcamera::controls::AfPauseState\n>\n> but it doesn't work here..\n>\n>\n> > + */\n> > +\n> > +} /* namespace libcamera::ipa::common::algorithms */\n> > diff --git a/src/ipa/libipa/algorithms/af_interface.h b/src/ipa/libipa/algorithms/af_interface.h\n> > new file mode 100644\n> > index 00000000..b6b7bea6\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/algorithms/af_interface.h\n> > @@ -0,0 +1,41 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2022, Theobroma Systems\n> > + *\n> > + * af_interface.h - Autofocus control algorithm interface\n> > + */\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +namespace libcamera::ipa::common::algorithms {\n> > +\n> > +class AfInterface\n> > +{\n> > +public:\n> > + AfInterface() = default;\n>\n> Do you even need a constructor ?\n>\n> > +\n> > + virtual ~AfInterface() {}\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 setMeteringMode(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> > + virtual controls::AfStateEnum getState() = 0;\n> > +\n> > + virtual controls::AfPauseStateEnum getPauseState() = 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..0a1f18fa\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_interface.h',\n> > +])\n> > +\n> > +common_ipa_algorithms_sources = files([\n> > + 'af_interface.cpp',\n> > +])\n> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > index 016b8e0e..0cfc551a 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> > @@ -8,6 +10,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> > @@ -16,6 +20,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> Minors on the doc apart, it seems all good to me.\n>\n> Let me copy RPi folks in, as they just upstreamed a PDAF based\n> auto-focus algorithm to check with them if there's a chance their\n> implementation can inherit from this class.\n\nThis does indeed look very similar to the RPi AfAlgorithm interface found at\nsrc/ipa/raspberrypi/controller/af_algorithm.h. Unfortunately all our algorithm\ninterfaces need to inherit from our Algorithm interface class, so we will not be\nable to use this class directly.\n\nRegards,\nNaush\n\n>\n> Thanks again!\n> j\n> > --\n> > 2.39.0\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 9FE22BEFBE\n\tfor ;\n\tMon, 6 Feb 2023 11:40:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0246061EF4;\n\tMon, 6 Feb 2023 12:40:16 +0100 (CET)","from mail-yb1-xb2f.google.com (mail-yb1-xb2f.google.com\n\t[IPv6:2607:f8b0:4864:20::b2f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 46B90603B8\n\tfor ;\n\tMon, 6 Feb 2023 12:40:14 +0100 (CET)","by mail-yb1-xb2f.google.com with SMTP id cf30so8679106ybb.11\n\tfor ;\n\tMon, 06 Feb 2023 03:40:14 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675683616;\n\tbh=ZvPB/8ALeRZB218nMcdjF8cQyW9rKyWMHG10NV+wK3w=;\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=gwNfIXcV0fzTQYEWd24+YARK4r/ogCLyMY3xo12R8LpELstrC08MbhByFMjA4wS/K\n\t1OSEqCiEMcPGduuVkj78SV7sqlyHnd9CPjHOQNIMtwaX00g82wTXp+eRPRwIvTg1Xh\n\tuwYBCzkOh9wH9bRQEjXHsLZdjXarm/lkuNLHcAdvX4C6mgvq2T4yVRSqbDnQZ1R3cH\n\tTdP86LUm9rkgs3RZfETPKRCxZIRbR0aJri1BcbM4BgJCw95xi3G2utJ2TB3zhnVis+\n\tIawROXuSP+07GIlVX7miIQl3EDO96d+N/1QZ0GCzZPu+zwAiZfIeDuv2CXVRqzKaWR\n\tumfjM/uKXK/Kg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=v/orET8QbqQhCGj4Q7eBSoNjMsg9n1q1p9tLPS28CD0=;\n\tb=jqotKMAd6sA2WTKvYyM9oNpnnBbQXvW+FnKhEVmAbAmnjrfIdKjnRFWaAhgZ+hnhTs\n\tuO0ZiTv/cHvJDp1GoNbbqssU9vRUGsY2NdBC+hwlF7tbDdJkUMSs+k/xUAduolPAZDKe\n\t0gmu6XwiFGNXD7qdLAnDzRP28Yd3EIpesja7UxgmwbWpaC86OMQxiBInZZkte+E2YYZG\n\tRrFkCv1eGvGHHDR0Qg84QM9q0YODc/Rdgjc3WxQ4jnXz2OAAhJzsJ/Uv13T6eHW8AGWA\n\tKIOg3aLEl18dTqQxXmWIjBI6uT+NClN4CyiZat44cdhblHVz9mimkrr6AQMeARpNvgn6\n\tDlbg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"jqotKMAd\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=v/orET8QbqQhCGj4Q7eBSoNjMsg9n1q1p9tLPS28CD0=;\n\tb=FVd0UNXcEiHTNN26Msj7SVenxcV5Lrfo784hSCR1dAStJ92FrMtTgCTS7WD9AhDsdO\n\tOHi/v/1m4rr7GfaH21rlchPJMAp41kgHWUR77JuhDiCF7+ZaIEOGw5aylbG78AkAlKOd\n\tz/8Pztvrg48VHmFFlwPpFFHhvCFDggg1DBAMLjr1P1MlsV2d31EQkBN+PSAZdTvsVwKy\n\tHuIbyDZgeaPWHtr6WiN7sI4URl/SNl//6UcYSqM9C0yRUJiPeZApbRwou6CyWh+cI/Cu\n\tB2E2GOSuVqgenuoE8P4BMaiN3vjQFmogIFE14plBEfYGfhUAZIVLknZvIIOFygg45aqT\n\tXJWA==","X-Gm-Message-State":"AO0yUKWAxMtUXX5oTFL+4/N9+PDJRb4ryvBBkA/QozjYOjDSJPGiHK/e\n\tQAeK6S47l4lc2KUODxRpk3ksExCTTXc7xWnKzKIDq7ARHx3gjv88LWI=","X-Google-Smtp-Source":"AK7set82MDMMtKKmbw95qRq5Byue8xKs98pamG6V+wrzjC8DUX7ogY0GZj8iuRt/t5Ptsr/fimm3OGMX7ylFqGPfb3I=","X-Received":"by 2002:a25:d6cc:0:b0:85d:7d3a:ee73 with SMTP id\n\tn195-20020a25d6cc000000b0085d7d3aee73mr1234256ybg.223.1675683613075;\n\tMon, 06 Feb 2023 03:40:13 -0800 (PST)","MIME-Version":"1.0","References":"<20230119084112.20564-1-dse@thaumatec.com>\n\t<20230119084112.20564-4-dse@thaumatec.com>\n\t<20230206111958.aqlhed5mcht3feyl@uno.localdomain>","In-Reply-To":"<20230206111958.aqlhed5mcht3feyl@uno.localdomain>","Date":"Mon, 6 Feb 2023 11:39:56 +0000","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 3/8] 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":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org,\n\tNick Hollinghurst ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26407,"web_url":"https://patchwork.libcamera.org/comment/26407/","msgid":"<167569563276.4026431.9508377143472972975@Monstersaurus>","date":"2023-02-06T15:00:32","subject":"Re: [libcamera-devel] [PATCH v3 3/8] ipa: Add base class defining\n\tAF algorithm interface","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck via libcamera-devel (2023-02-06 11:39:56)\n> Hi Jacopo and Daniel,\n> \n> On Mon, 6 Feb 2023 at 11:20, Jacopo Mondi wrote:\n> >\n> > Hi Daniel\n> >\n> > On Thu, Jan 19, 2023 at 09:41:07AM +0100, Daniel Semkowicz via libcamera-devel wrote:\n> > > Define common interface with basic functions that should be supported\n> > > by Auto Focus algorithms.\n> > >\n> >\n> > As commented in the previous version, I like this approach very much\n> >\n> > > Signed-off-by: Daniel Semkowicz \n> > > ---\n> > > src/ipa/libipa/algorithms/af_interface.cpp | 92 ++++++++++++++++++++++\n> > > src/ipa/libipa/algorithms/af_interface.h | 41 ++++++++++\n> > > src/ipa/libipa/algorithms/meson.build | 9 +++\n> > > src/ipa/libipa/meson.build | 6 ++\n> > > 4 files changed, 148 insertions(+)\n> > > create mode 100644 src/ipa/libipa/algorithms/af_interface.cpp\n> > > create mode 100644 src/ipa/libipa/algorithms/af_interface.h\n> > > create mode 100644 src/ipa/libipa/algorithms/meson.build\n> > >\n> > > diff --git a/src/ipa/libipa/algorithms/af_interface.cpp b/src/ipa/libipa/algorithms/af_interface.cpp\n> > > new file mode 100644\n> > > index 00000000..af341d13\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/algorithms/af_interface.cpp\n> >\n> > Nit: af_interface.[cpp|h] or just af.[cpp|h] ?\n> >\n> > > @@ -0,0 +1,92 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2022, Theobroma Systems\n> > > + *\n> > > + * af_interface.cpp - Autofocus control algorithm interface\n> > > + */\n> > > +\n> > > +#include \"af_interface.h\"\n> > > +\n> > > +/**\n> > > + * \\file af_interface.h\n> > > + * \\brief AF algorithm common interface\n> > > + */\n> > > +\n> > > +namespace libcamera::ipa::common::algorithms {\n> > > +\n> > > +/**\n> > > + * \\class AfInterface\n> > > + * \\brief Common interface for auto-focus algorithms\n> > > + * \\tparam Module The IPA module type for this class of algorithms\n> >\n> > You don't have the Module template argument anymore\n> >\n> > > + *\n> > > + * The AfInterface class defines a standard interface for IPA auto focus\n> > > + * algorithms.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn AfInterface::setMode()\n> > > + * \\brief Set auto focus mode\n> > > + * \\param[in] mode AF mode\n> >\n> > I was about to suggest \\sa controls::AfModeEnum but doxygen already\n> > links the parameter type to the right documentation page, so it's not\n> > necessary.\n> >\n> > However, as this interface implements the Af controls defined in\n> > control_ids.yaml I would be tempted to say that we might even /copydoc\n> > so that the documentation is centralized in a single place ??\n> >\n> >\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn AfInterface::setRange()\n> > > + * \\brief set the range of focus distances that is scanned\n> > > + * \\param[in] range AF range\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn AfInterface::setSpeed()\n> > > + * \\brief Set how fast algorithm should move the lens\n> > > + * \\param[in] speed Lens move speed\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn AfInterface::setMeteringMode()\n> > > + * \\brief Set AF metering mode\n> > > + * \\param[in] metering AF metering mode\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn AfInterface::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 AfInterface::setTrigger()\n> > > + * \\brief Starts or cancels the autofocus scan\n> > > + * \\param[in] trigger Trigger mode\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn AfInterface::setPause()\n> > > + * \\brief Pause the autofocus while in AfModeContinuous mode.\n> > > + * \\param[in] pause Pause mode\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn AfInterface::setLensPosition()\n> > > + * \\brief Set the lens position while in AfModeManual\n> > > + * \\param[in] lensPosition Lens position\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn AfInterface::getState()\n> >\n> > Minor nit: for getters we don't usually prepend \"get\", so this could\n> > just be AfInterface::state() ?\n> >\n> > > + * \\brief Get the current state of the AF algorithm\n> > > + * \\return AF state\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn AfInterface::getPauseState()\n> > > + * \\brief Get the current pause state of the AF algorithm.\n> > > + * \\return AF pause state\n> > > + *\n> > > + * Only applicable in continuous (AfModeContinuous) mode. In other modes,\n> > > + * AfPauseStateRunning is always returned.\n> >\n> > See, in this case the documentation of controls::AfPauseState has gone\n> > through a lot of discussions, and trying to resume it here might not\n> > be ideal. Plus it has to be kept in sync with the control\n> > documentation.\n> >\n> > I tried\n> >\n> > \\copydoc libcamera::controls::AfPauseState\n> >\n> > but it doesn't work here..\n> >\n> >\n> > > + */\n> > > +\n> > > +} /* namespace libcamera::ipa::common::algorithms */\n> > > diff --git a/src/ipa/libipa/algorithms/af_interface.h b/src/ipa/libipa/algorithms/af_interface.h\n> > > new file mode 100644\n> > > index 00000000..b6b7bea6\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/algorithms/af_interface.h\n> > > @@ -0,0 +1,41 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2022, Theobroma Systems\n> > > + *\n> > > + * af_interface.h - Autofocus control algorithm interface\n> > > + */\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +\n> > > +namespace libcamera::ipa::common::algorithms {\n> > > +\n> > > +class AfInterface\n> > > +{\n> > > +public:\n> > > + AfInterface() = default;\n> >\n> > Do you even need a constructor ?\n> >\n> > > +\n> > > + virtual ~AfInterface() {}\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 setMeteringMode(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> > > + virtual controls::AfStateEnum getState() = 0;\n> > > +\n> > > + virtual controls::AfPauseStateEnum getPauseState() = 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..0a1f18fa\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_interface.h',\n> > > +])\n> > > +\n> > > +common_ipa_algorithms_sources = files([\n> > > + 'af_interface.cpp',\n> > > +])\n> > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > > index 016b8e0e..0cfc551a 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> > > @@ -8,6 +10,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> > > @@ -16,6 +20,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> > Minors on the doc apart, it seems all good to me.\n> >\n> > Let me copy RPi folks in, as they just upstreamed a PDAF based\n> > auto-focus algorithm to check with them if there's a chance their\n> > implementation can inherit from this class.\n\nThat was one thing I was wondering while looking through this series.\nGiven that the base implementation ends up with things like 'initBase',\nand 'queueRequestBase' ... It feels like perhaps this might be better\nwith composition rather than inheritance, so that if an implementation\nwants to use this - it can just create the object in that algorithm\nclass and make calls on that object directly?\n\n--\nKieran\n\n\n> \n> This does indeed look very similar to the RPi AfAlgorithm interface found at\n> src/ipa/raspberrypi/controller/af_algorithm.h. Unfortunately all our algorithm\n> interfaces need to inherit from our Algorithm interface class, so we will not be\n> able to use this class directly.\n> \n> Regards,\n> Naush\n> \n> >\n> > Thanks again!\n> > j\n> > > --\n> > > 2.39.0\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 2B1F7BDB1C\n\tfor ;\n\tMon, 6 Feb 2023 15:00:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7B25C61EF4;\n\tMon, 6 Feb 2023 16:00:37 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DA831603B8\n\tfor ;\n\tMon, 6 Feb 2023 16:00:35 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 503584DA;\n\tMon, 6 Feb 2023 16:00:35 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675695637;\n\tbh=wX/tmIDD8gMja41CuQrx7oM5tTdhC4wEZz93rA6hM8k=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=F+mZCGd3epYjFInLsjOVi2sOgLjQl/itSLom/uDbapuhcIGUo/RiMYqvkcM6leJxw\n\tvmJuSba9ejIeBxYGZflCr2V6CoGvnRDAG8C4YOvY+Bl32FOxjhyYhisPNUzhAgHb+A\n\tytTBd1Habo4rHf9Z4H6S2g751cYTptpMHJ6yc+69BP7ar1nAUSmUxvolqhZwbEWhI5\n\t0ABMaBhz445h5Pv8wkIva5zg7wOvO0fzqroEVCcN9OkwpQtrjA8hEccBrbBCaH6mkX\n\tpKP5CHd0QLXTvhQlKZfuP4Qvtrw9+L+rHePIrIbSqKIm0mvpPFgA0RjDSkrec4Xizw\n\ttN8kBH3/pWhGg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1675695635;\n\tbh=wX/tmIDD8gMja41CuQrx7oM5tTdhC4wEZz93rA6hM8k=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=BHtwQxnsj8Qct+pfjgKB4pfGnlZJ5tOydRKpDp93zkefa8kqATt3Nuf5CEo8Yy4+S\n\t5B/+5zFYeJ+D4g2Uaea51c1531w6XOKJPu7Cu3oTrMqoNdFZ5+dXZsNE9vZVrYj3JO\n\ttqtzjFlCCrWwOWyz0q7152TVjhgq6LfM263ojWTc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"BHtwQxns\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20230119084112.20564-1-dse@thaumatec.com>\n\t<20230119084112.20564-4-dse@thaumatec.com>\n\t<20230206111958.aqlhed5mcht3feyl@uno.localdomain>\n\t","To":"Jacopo Mondi ,\n\tNaushir Patuck ,\n\tNaushir Patuck via libcamera-devel ","Date":"Mon, 06 Feb 2023 15:00:32 +0000","Message-ID":"<167569563276.4026431.9508377143472972975@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3 3/8] 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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"Nick Hollinghurst ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26410,"web_url":"https://patchwork.libcamera.org/comment/26410/","msgid":"<20230206174655.7mcgtaae7uit2j2g@uno.localdomain>","date":"2023-02-06T17:46:55","subject":"Re: [libcamera-devel] [PATCH v3 3/8] ipa: Add base class defining\n\tAF algorithm interface","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Kieran\n\nOn Mon, Feb 06, 2023 at 03:00:32PM +0000, Kieran Bingham via libcamera-devel wrote:\n> Quoting Naushir Patuck via libcamera-devel (2023-02-06 11:39:56)\n> > Hi Jacopo and Daniel,\n> >\n> > On Mon, 6 Feb 2023 at 11:20, Jacopo Mondi wrote:\n> > >\n> > > Hi Daniel\n> > >\n> > > On Thu, Jan 19, 2023 at 09:41:07AM +0100, Daniel Semkowicz via libcamera-devel wrote:\n> > > > Define common interface with basic functions that should be supported\n> > > > by Auto Focus algorithms.\n> > > >\n> > >\n> > > As commented in the previous version, I like this approach very much\n> > >\n> > > > Signed-off-by: Daniel Semkowicz \n> > > > ---\n> > > > src/ipa/libipa/algorithms/af_interface.cpp | 92 ++++++++++++++++++++++\n> > > > src/ipa/libipa/algorithms/af_interface.h | 41 ++++++++++\n> > > > src/ipa/libipa/algorithms/meson.build | 9 +++\n> > > > src/ipa/libipa/meson.build | 6 ++\n> > > > 4 files changed, 148 insertions(+)\n> > > > create mode 100644 src/ipa/libipa/algorithms/af_interface.cpp\n> > > > create mode 100644 src/ipa/libipa/algorithms/af_interface.h\n> > > > create mode 100644 src/ipa/libipa/algorithms/meson.build\n> > > >\n> > > > diff --git a/src/ipa/libipa/algorithms/af_interface.cpp b/src/ipa/libipa/algorithms/af_interface.cpp\n> > > > new file mode 100644\n> > > > index 00000000..af341d13\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/libipa/algorithms/af_interface.cpp\n> > >\n> > > Nit: af_interface.[cpp|h] or just af.[cpp|h] ?\n> > >\n> > > > @@ -0,0 +1,92 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2022, Theobroma Systems\n> > > > + *\n> > > > + * af_interface.cpp - Autofocus control algorithm interface\n> > > > + */\n> > > > +\n> > > > +#include \"af_interface.h\"\n> > > > +\n> > > > +/**\n> > > > + * \\file af_interface.h\n> > > > + * \\brief AF algorithm common interface\n> > > > + */\n> > > > +\n> > > > +namespace libcamera::ipa::common::algorithms {\n> > > > +\n> > > > +/**\n> > > > + * \\class AfInterface\n> > > > + * \\brief Common interface for auto-focus algorithms\n> > > > + * \\tparam Module The IPA module type for this class of algorithms\n> > >\n> > > You don't have the Module template argument anymore\n> > >\n> > > > + *\n> > > > + * The AfInterface class defines a standard interface for IPA auto focus\n> > > > + * algorithms.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn AfInterface::setMode()\n> > > > + * \\brief Set auto focus mode\n> > > > + * \\param[in] mode AF mode\n> > >\n> > > I was about to suggest \\sa controls::AfModeEnum but doxygen already\n> > > links the parameter type to the right documentation page, so it's not\n> > > necessary.\n> > >\n> > > However, as this interface implements the Af controls defined in\n> > > control_ids.yaml I would be tempted to say that we might even /copydoc\n> > > so that the documentation is centralized in a single place ??\n> > >\n> > >\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn AfInterface::setRange()\n> > > > + * \\brief set the range of focus distances that is scanned\n> > > > + * \\param[in] range AF range\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn AfInterface::setSpeed()\n> > > > + * \\brief Set how fast algorithm should move the lens\n> > > > + * \\param[in] speed Lens move speed\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn AfInterface::setMeteringMode()\n> > > > + * \\brief Set AF metering mode\n> > > > + * \\param[in] metering AF metering mode\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn AfInterface::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 AfInterface::setTrigger()\n> > > > + * \\brief Starts or cancels the autofocus scan\n> > > > + * \\param[in] trigger Trigger mode\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn AfInterface::setPause()\n> > > > + * \\brief Pause the autofocus while in AfModeContinuous mode.\n> > > > + * \\param[in] pause Pause mode\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn AfInterface::setLensPosition()\n> > > > + * \\brief Set the lens position while in AfModeManual\n> > > > + * \\param[in] lensPosition Lens position\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn AfInterface::getState()\n> > >\n> > > Minor nit: for getters we don't usually prepend \"get\", so this could\n> > > just be AfInterface::state() ?\n> > >\n> > > > + * \\brief Get the current state of the AF algorithm\n> > > > + * \\return AF state\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn AfInterface::getPauseState()\n> > > > + * \\brief Get the current pause state of the AF algorithm.\n> > > > + * \\return AF pause state\n> > > > + *\n> > > > + * Only applicable in continuous (AfModeContinuous) mode. In other modes,\n> > > > + * AfPauseStateRunning is always returned.\n> > >\n> > > See, in this case the documentation of controls::AfPauseState has gone\n> > > through a lot of discussions, and trying to resume it here might not\n> > > be ideal. Plus it has to be kept in sync with the control\n> > > documentation.\n> > >\n> > > I tried\n> > >\n> > > \\copydoc libcamera::controls::AfPauseState\n> > >\n> > > but it doesn't work here..\n> > >\n> > >\n> > > > + */\n> > > > +\n> > > > +} /* namespace libcamera::ipa::common::algorithms */\n> > > > diff --git a/src/ipa/libipa/algorithms/af_interface.h b/src/ipa/libipa/algorithms/af_interface.h\n> > > > new file mode 100644\n> > > > index 00000000..b6b7bea6\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/libipa/algorithms/af_interface.h\n> > > > @@ -0,0 +1,41 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2022, Theobroma Systems\n> > > > + *\n> > > > + * af_interface.h - Autofocus control algorithm interface\n> > > > + */\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +namespace libcamera::ipa::common::algorithms {\n> > > > +\n> > > > +class AfInterface\n> > > > +{\n> > > > +public:\n> > > > + AfInterface() = default;\n> > >\n> > > Do you even need a constructor ?\n> > >\n> > > > +\n> > > > + virtual ~AfInterface() {}\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 setMeteringMode(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> > > > + virtual controls::AfStateEnum getState() = 0;\n> > > > +\n> > > > + virtual controls::AfPauseStateEnum getPauseState() = 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..0a1f18fa\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_interface.h',\n> > > > +])\n> > > > +\n> > > > +common_ipa_algorithms_sources = files([\n> > > > + 'af_interface.cpp',\n> > > > +])\n> > > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > > > index 016b8e0e..0cfc551a 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> > > > @@ -8,6 +10,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> > > > @@ -16,6 +20,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> > > Minors on the doc apart, it seems all good to me.\n> > >\n> > > Let me copy RPi folks in, as they just upstreamed a PDAF based\n> > > auto-focus algorithm to check with them if there's a chance their\n> > > implementation can inherit from this class.\n>\n> That was one thing I was wondering while looking through this series.\n> Given that the base implementation ends up with things like 'initBase',\n> and 'queueRequestBase' ... It feels like perhaps this might be better\n> with composition rather than inheritance, so that if an implementation\n> wants to use this - it can just create the object in that algorithm\n> class and make calls on that object directly?\n\nKind of agree with the observation.\n\nAt the least, if inheritance should be used (which I'm not sure)\nit is not necessary to name the \"base\" class methods with the \"base\"\nsuffix\n\n------------------------------------------------------------------------------\n--- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n+++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n@@ -69,7 +69,7 @@ int AfHillClimbing::initBase(const YamlObject &tuningData)\n * This function should be called in the queueRequest() function of the derived class.\n * See alse: libcamera::ipa::Algorithm::queueRequest()\n */\n-void AfHillClimbing::queueRequestBase([[maybe_unused]] const uint32_t frame, const ControlList &controls)\n+void AfHillClimbing::queueRequest([[maybe_unused]] const uint32_t frame, const ControlList &controls)\n {\n for (auto const &[id, value] : controls) {\n switch (id) {\ndiff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\nindex 6ce95884c03d..9d68968865c5 100644\n--- a/src/ipa/libipa/algorithms/af_hill_climbing.h\n+++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n@@ -36,7 +36,7 @@ public:\n\n protected:\n int initBase(const YamlObject &tuningData);\n- void queueRequestBase(const uint32_t frame, const ControlList &controls);\n+ void queueRequest(const uint32_t frame, const ControlList &controls);\n uint32_t processAutofocus(double currentContrast);\n void setFramesToSkip(uint32_t n);\n\ndiff --git a/src/ipa/rkisp1/algorithms/af.cpp b/src/ipa/rkisp1/algorithms/af.cpp\nindex 65768fc45e5b..bda8f273f695 100644\n--- a/src/ipa/rkisp1/algorithms/af.cpp\n+++ b/src/ipa/rkisp1/algorithms/af.cpp\n@@ -74,7 +74,7 @@ void Af::queueRequest([[maybe_unused]] IPAContext &context,\n [[maybe_unused]] IPAFrameContext &frameContext,\n const ControlList &controls)\n {\n- queueRequestBase(frame, controls);\n+ AfHillClimbing::queueRequest(frame, controls);\n\n for (auto const &[id, value] : controls) {\n switch (id) {\n------------------------------------------------------------------------------\n\nWhen it comes to inheritance vs composition, I think what makes a\ndifference is if and IPA module will always need a platform-specific\nimplementation in src/ipa/[rkisp1|ipu3|...]/algorithms/af.cpp or it\ncould potentially instantiate the generic\nsrc/ipa/libipa/algorithms/af_hill_climbing.cpp or not.\n\nAs there will be always be a platform specific part I don't think\nthat's the case.\n\nRight now (behold! UML with ascii) the class hierarchy looks\nlike:\n\n\n +-----------------------+\n | |\n |Algorithm::AfInterface |\n | |\n +-----------------------+\n |\n |\n _\n v\n +---------------------------+ +--------------+\n | Algorithm::AfHillClimbing | | |\n |---------------------------| | Algorithm |\n | | | |\n +---------------------------+ +--------------+\n | |\n | |\n _ |\n v |\n +---------------------------+ |\n | Algorithm::RkISP1::Af | |\n |---------------------------|<|-------+\n | |\n +---------------------------+\n\n\nShould we instead split the hierarchy and have the platform specific\nimplementation inherit from Algorithm (so that the IPA module can use\nit by calling on it the usual init(), configure() and queueRequest()\nmethods) and have the AutoFocus hierachy used by composition ?\n\nSomething like\n\n +------------------------+ +-----------------------+\n | | | |\n | Algorithm | |Algorithm::AfInterface |\n | | | |\n +------------------------+ +-----------------------+\n | |\n | |\n | _\n _ v\n v +---------------------------+\n +---------------------------+ | Algorithm::AfHillClimbing |\n | Algorithm::RkISP1::Af |---------<>|---------------------------|\n |---------------------------| | |\n | | +---------------------------+\n +---------------------------+\n\n\nTrue, each platform specific implementation (rkisp1, IPU3 etc) will\nneed to reimplement the Algorithm interface ?\n\nWhat do others think ?\n\n\n>\n> --\n> Kieran\n>\n>\n> >\n> > This does indeed look very similar to the RPi AfAlgorithm interface found at\n> > src/ipa/raspberrypi/controller/af_algorithm.h. Unfortunately all our algorithm\n> > interfaces need to inherit from our Algorithm interface class, so we will not be\n> > able to use this class directly.\n> >\n> > Regards,\n> > Naush\n> >\n> > >\n> > > Thanks again!\n> > > j\n> > > > --\n> > > > 2.39.0\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 B7629BDB1C\n\tfor ;\n\tMon, 6 Feb 2023 17:47:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 35E4F625F1;\n\tMon, 6 Feb 2023 18:47:08 +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 0B0DB603B8\n\tfor ;\n\tMon, 6 Feb 2023 18:47:07 +0100 (CET)","from ideasonboard.com (host-79-35-57-126.retail.telecomitalia.it\n\t[79.35.57.126])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 15BE94DA;\n\tMon, 6 Feb 2023 18:47:03 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1675705628;\n\tbh=+nLbzelxhb6067Ki0Ywse1Z/Y7e33PjO59E2ScE5kIE=;\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=wOq9KHr/sgJAq5A4+jgy8Vs0x09apQUMUxWgcM9yzYOcpoCBVBAN3WddL7bwJmy+f\n\tstSTiukry74VnSBk+6EbSMHbLtpGSGqgpXrMHrf7OlLcjxx9D89LpsxzGi9EkBw9JR\n\tkBEdKCtDDphg/corANjGszdqDGG+QCnvY8rGvcJ+wwu1ZqxaPuEXvu5oGi7ojAYVSL\n\t/JiVUH0Yl2oA1rvZRtuu1Oeag65Qxz74PW6tTxqQr4VGppwG4t3KAUiUUKcJhyWcCO\n\tX8LZqOvdY/bqM23mBytVM3LZeRspRhELcnijCxDy2OHLx7we+ZaPRCjCc4h/L9MVY9\n\taodsRh5OqFq2Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1675705626;\n\tbh=+nLbzelxhb6067Ki0Ywse1Z/Y7e33PjO59E2ScE5kIE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ESNOIF6TyTR0ulQv+vH8KAUNi0nJi1YkLSKQU4UMlXxjSmsUSmX1K8JMcxdI0Q1ll\n\tgd4kh4Do0e0gSLSS3ZAl8snCXWqQLwC8j+KNEEKrVJ2YXZCygCTJ8Esqz3eeT5XvDv\n\tygDmZ3zIR6Ee+yYYgsN0mW/OJXfcOksn9OuCjK+w="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ESNOIF6T\"; dkim-atps=neutral","Date":"Mon, 6 Feb 2023 18:46:55 +0100","To":"Kieran Bingham ","Message-ID":"<20230206174655.7mcgtaae7uit2j2g@uno.localdomain>","References":"<20230119084112.20564-1-dse@thaumatec.com>\n\t<20230119084112.20564-4-dse@thaumatec.com>\n\t<20230206111958.aqlhed5mcht3feyl@uno.localdomain>\n\t\n\t<167569563276.4026431.9508377143472972975@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<167569563276.4026431.9508377143472972975@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v3 3/8] 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":"Jacopo Mondi ,\n\tNick Hollinghurst ,\n\tNaushir Patuck via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26613,"web_url":"https://patchwork.libcamera.org/comment/26613/","msgid":"","date":"2023-03-09T15:47:59","subject":"Re: [libcamera-devel] [PATCH v3 3/8] 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, hi Kieran, hi Naushir!\n\nOn Mon, Feb 6, 2023 at 6:47 PM Jacopo Mondi via libcamera-devel\n wrote:\n>\n> Hi Kieran\n>\n> On Mon, Feb 06, 2023 at 03:00:32PM +0000, Kieran Bingham via libcamera-devel wrote:\n> > Quoting Naushir Patuck via libcamera-devel (2023-02-06 11:39:56)\n> > > Hi Jacopo and Daniel,\n> > >\n> > > On Mon, 6 Feb 2023 at 11:20, Jacopo Mondi wrote:\n> > > >\n> > > > Hi Daniel\n> > > >\n> > > > On Thu, Jan 19, 2023 at 09:41:07AM +0100, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > Define common interface with basic functions that should be supported\n> > > > > by Auto Focus algorithms.\n> > > > >\n> > > >\n> > > > As commented in the previous version, I like this approach very much\n> > > >\n> > > > > Signed-off-by: Daniel Semkowicz \n> > > > > ---\n> > > > > src/ipa/libipa/algorithms/af_interface.cpp | 92 ++++++++++++++++++++++\n> > > > > src/ipa/libipa/algorithms/af_interface.h | 41 ++++++++++\n> > > > > src/ipa/libipa/algorithms/meson.build | 9 +++\n> > > > > src/ipa/libipa/meson.build | 6 ++\n> > > > > 4 files changed, 148 insertions(+)\n> > > > > create mode 100644 src/ipa/libipa/algorithms/af_interface.cpp\n> > > > > create mode 100644 src/ipa/libipa/algorithms/af_interface.h\n> > > > > create mode 100644 src/ipa/libipa/algorithms/meson.build\n> > > > >\n> > > > > diff --git a/src/ipa/libipa/algorithms/af_interface.cpp b/src/ipa/libipa/algorithms/af_interface.cpp\n> > > > > new file mode 100644\n> > > > > index 00000000..af341d13\n> > > > > --- /dev/null\n> > > > > +++ b/src/ipa/libipa/algorithms/af_interface.cpp\n> > > >\n> > > > Nit: af_interface.[cpp|h] or just af.[cpp|h] ?\n> > > >\n> > > > > @@ -0,0 +1,92 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2022, Theobroma Systems\n> > > > > + *\n> > > > > + * af_interface.cpp - Autofocus control algorithm interface\n> > > > > + */\n> > > > > +\n> > > > > +#include \"af_interface.h\"\n> > > > > +\n> > > > > +/**\n> > > > > + * \\file af_interface.h\n> > > > > + * \\brief AF algorithm common interface\n> > > > > + */\n> > > > > +\n> > > > > +namespace libcamera::ipa::common::algorithms {\n> > > > > +\n> > > > > +/**\n> > > > > + * \\class AfInterface\n> > > > > + * \\brief Common interface for auto-focus algorithms\n> > > > > + * \\tparam Module The IPA module type for this class of algorithms\n> > > >\n> > > > You don't have the Module template argument anymore\n> > > >\n> > > > > + *\n> > > > > + * The AfInterface class defines a standard interface for IPA auto focus\n> > > > > + * algorithms.\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn AfInterface::setMode()\n> > > > > + * \\brief Set auto focus mode\n> > > > > + * \\param[in] mode AF mode\n> > > >\n> > > > I was about to suggest \\sa controls::AfModeEnum but doxygen already\n> > > > links the parameter type to the right documentation page, so it's not\n> > > > necessary.\n> > > >\n> > > > However, as this interface implements the Af controls defined in\n> > > > control_ids.yaml I would be tempted to say that we might even /copydoc\n> > > > so that the documentation is centralized in a single place ??\n> > > >\n> > > >\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn AfInterface::setRange()\n> > > > > + * \\brief set the range of focus distances that is scanned\n> > > > > + * \\param[in] range AF range\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn AfInterface::setSpeed()\n> > > > > + * \\brief Set how fast algorithm should move the lens\n> > > > > + * \\param[in] speed Lens move speed\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn AfInterface::setMeteringMode()\n> > > > > + * \\brief Set AF metering mode\n> > > > > + * \\param[in] metering AF metering mode\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn AfInterface::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 AfInterface::setTrigger()\n> > > > > + * \\brief Starts or cancels the autofocus scan\n> > > > > + * \\param[in] trigger Trigger mode\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn AfInterface::setPause()\n> > > > > + * \\brief Pause the autofocus while in AfModeContinuous mode.\n> > > > > + * \\param[in] pause Pause mode\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn AfInterface::setLensPosition()\n> > > > > + * \\brief Set the lens position while in AfModeManual\n> > > > > + * \\param[in] lensPosition Lens position\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn AfInterface::getState()\n> > > >\n> > > > Minor nit: for getters we don't usually prepend \"get\", so this could\n> > > > just be AfInterface::state() ?\n> > > >\n> > > > > + * \\brief Get the current state of the AF algorithm\n> > > > > + * \\return AF state\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn AfInterface::getPauseState()\n> > > > > + * \\brief Get the current pause state of the AF algorithm.\n> > > > > + * \\return AF pause state\n> > > > > + *\n> > > > > + * Only applicable in continuous (AfModeContinuous) mode. In other modes,\n> > > > > + * AfPauseStateRunning is always returned.\n> > > >\n> > > > See, in this case the documentation of controls::AfPauseState has gone\n> > > > through a lot of discussions, and trying to resume it here might not\n> > > > be ideal. Plus it has to be kept in sync with the control\n> > > > documentation.\n> > > >\n> > > > I tried\n> > > >\n> > > > \\copydoc libcamera::controls::AfPauseState\n> > > >\n> > > > but it doesn't work here..\n> > > >\n> > > >\n> > > > > + */\n> > > > > +\n> > > > > +} /* namespace libcamera::ipa::common::algorithms */\n> > > > > diff --git a/src/ipa/libipa/algorithms/af_interface.h b/src/ipa/libipa/algorithms/af_interface.h\n> > > > > new file mode 100644\n> > > > > index 00000000..b6b7bea6\n> > > > > --- /dev/null\n> > > > > +++ b/src/ipa/libipa/algorithms/af_interface.h\n> > > > > @@ -0,0 +1,41 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2022, Theobroma Systems\n> > > > > + *\n> > > > > + * af_interface.h - Autofocus control algorithm interface\n> > > > > + */\n> > > > > +#pragma once\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +namespace libcamera::ipa::common::algorithms {\n> > > > > +\n> > > > > +class AfInterface\n> > > > > +{\n> > > > > +public:\n> > > > > + AfInterface() = default;\n> > > >\n> > > > Do you even need a constructor ?\n> > > >\n> > > > > +\n> > > > > + virtual ~AfInterface() {}\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 setMeteringMode(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> > > > > + virtual controls::AfStateEnum getState() = 0;\n> > > > > +\n> > > > > + virtual controls::AfPauseStateEnum getPauseState() = 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..0a1f18fa\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_interface.h',\n> > > > > +])\n> > > > > +\n> > > > > +common_ipa_algorithms_sources = files([\n> > > > > + 'af_interface.cpp',\n> > > > > +])\n> > > > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > > > > index 016b8e0e..0cfc551a 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> > > > > @@ -8,6 +10,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> > > > > @@ -16,6 +20,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> > > > Minors on the doc apart, it seems all good to me.\n> > > >\n> > > > Let me copy RPi folks in, as they just upstreamed a PDAF based\n> > > > auto-focus algorithm to check with them if there's a chance their\n> > > > implementation can inherit from this class.\n> >\n> > That was one thing I was wondering while looking through this series.\n> > Given that the base implementation ends up with things like 'initBase',\n> > and 'queueRequestBase' ... It feels like perhaps this might be better\n> > with composition rather than inheritance, so that if an implementation\n> > wants to use this - it can just create the object in that algorithm\n> > class and make calls on that object directly?\n>\n> Kind of agree with the observation.\n>\n> At the least, if inheritance should be used (which I'm not sure)\n> it is not necessary to name the \"base\" class methods with the \"base\"\n> suffix\n>\n> ------------------------------------------------------------------------------\n> --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> @@ -69,7 +69,7 @@ int AfHillClimbing::initBase(const YamlObject &tuningData)\n> * This function should be called in the queueRequest() function of the derived class.\n> * See alse: libcamera::ipa::Algorithm::queueRequest()\n> */\n> -void AfHillClimbing::queueRequestBase([[maybe_unused]] const uint32_t frame, const ControlList &controls)\n> +void AfHillClimbing::queueRequest([[maybe_unused]] const uint32_t frame, const ControlList &controls)\n> {\n> for (auto const &[id, value] : controls) {\n> switch (id) {\n> diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> index 6ce95884c03d..9d68968865c5 100644\n> --- a/src/ipa/libipa/algorithms/af_hill_climbing.h\n> +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> @@ -36,7 +36,7 @@ public:\n>\n> protected:\n> int initBase(const YamlObject &tuningData);\n> - void queueRequestBase(const uint32_t frame, const ControlList &controls);\n> + void queueRequest(const uint32_t frame, const ControlList &controls);\n> uint32_t processAutofocus(double currentContrast);\n> void setFramesToSkip(uint32_t n);\n>\n> diff --git a/src/ipa/rkisp1/algorithms/af.cpp b/src/ipa/rkisp1/algorithms/af.cpp\n> index 65768fc45e5b..bda8f273f695 100644\n> --- a/src/ipa/rkisp1/algorithms/af.cpp\n> +++ b/src/ipa/rkisp1/algorithms/af.cpp\n> @@ -74,7 +74,7 @@ void Af::queueRequest([[maybe_unused]] IPAContext &context,\n> [[maybe_unused]] IPAFrameContext &frameContext,\n> const ControlList &controls)\n> {\n> - queueRequestBase(frame, controls);\n> + AfHillClimbing::queueRequest(frame, controls);\n>\n> for (auto const &[id, value] : controls) {\n> switch (id) {\n> ------------------------------------------------------------------------------\n>\n> When it comes to inheritance vs composition, I think what makes a\n> difference is if and IPA module will always need a platform-specific\n> implementation in src/ipa/[rkisp1|ipu3|...]/algorithms/af.cpp or it\n> could potentially instantiate the generic\n> src/ipa/libipa/algorithms/af_hill_climbing.cpp or not.\n>\n> As there will be always be a platform specific part I don't think\n> that's the case.\n>\n> Right now (behold! UML with ascii) the class hierarchy looks\n> like:\n>\n>\n> +-----------------------+\n> | |\n> |Algorithm::AfInterface |\n> | |\n> +-----------------------+\n> |\n> |\n> _\n> v\n> +---------------------------+ +--------------+\n> | Algorithm::AfHillClimbing | | |\n> |---------------------------| | Algorithm |\n> | | | |\n> +---------------------------+ +--------------+\n> | |\n> | |\n> _ |\n> v |\n> +---------------------------+ |\n> | Algorithm::RkISP1::Af | |\n> |---------------------------|<|-------+\n> | |\n> +---------------------------+\n>\n>\n> Should we instead split the hierarchy and have the platform specific\n> implementation inherit from Algorithm (so that the IPA module can use\n> it by calling on it the usual init(), configure() and queueRequest()\n> methods) and have the AutoFocus hierachy used by composition ?\n>\n> Something like\n>\n> +------------------------+ +-----------------------+\n> | | | |\n> | Algorithm | |Algorithm::AfInterface |\n> | | | |\n> +------------------------+ +-----------------------+\n> | |\n> | |\n> | _\n> _ v\n> v +---------------------------+\n> +---------------------------+ | Algorithm::AfHillClimbing |\n> | Algorithm::RkISP1::Af |---------<>|---------------------------|\n> |---------------------------| | |\n> | | +---------------------------+\n> +---------------------------+\n>\n>\n> True, each platform specific implementation (rkisp1, IPU3 etc) will\n> need to reimplement the Algorithm interface ?\n>\n> What do others think ?\n\nWith the current usage, both approaches are not so different.\nAs you already mentioned, We will always need a platform specific\nimplementation on top, so it will not be possible to implement\nthe complete interface in the common way.\nFor example, init(), configure(), queueRequest() methods from the base\nclass will need to be called explicitly in the derive init(),\nconfigure(), queueRequest() methods due to parameters mismatch.\nAnd when you look at the code it even looks more like a composition\napproach:\n\n AfHillClimbing::queueRequest(...)\n\nvs\n\n afHillClimbing->queueRequest(...)\n\nTo be honest, I am not a big fan of multiple inheritance, so if it is\npossible, I would avoid it.\n\nBest regards\nDaniel\n\n>\n>\n> >\n> > --\n> > Kieran\n> >\n> >\n> > >\n> > > This does indeed look very similar to the RPi AfAlgorithm interface found at\n> > > src/ipa/raspberrypi/controller/af_algorithm.h. Unfortunately all our algorithm\n> > > interfaces need to inherit from our Algorithm interface class, so we will not be\n> > > able to use this class directly.\n> > >\n> > > Regards,\n> > > Naush\n> > >\n> > > >\n> > > > Thanks again!\n> > > > j\n> > > > > --\n> > > > > 2.39.0\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 BC23FBE080\n\tfor ;\n\tThu, 9 Mar 2023 15:48:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 823DB626D6;\n\tThu, 9 Mar 2023 16:48:12 +0100 (CET)","from mail-ed1-x529.google.com (mail-ed1-x529.google.com\n\t[IPv6:2a00:1450:4864:20::529])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 717CF61ED7\n\tfor ;\n\tThu, 9 Mar 2023 16:48:11 +0100 (CET)","by mail-ed1-x529.google.com with SMTP id a25so8965208edb.0\n\tfor ;\n\tThu, 09 Mar 2023 07:48:11 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1678376892;\n\tbh=KlQG5vMuWI2v6LZ7peqI/RmEmNTf2vBuQt7YFPBCchk=;\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=sgmcZdz8+2O5O359hJOCNjbQtAVWj6o/HzWBfOzOvlDEn21e26lPxAs7Q2sykAlfL\n\tlArp+KPdbY64TcWPADPiv1R74+USziWbpkgMA47Coxa1z9t6EEgzXSSc2W14v8sh02\n\tCSHgwXNDPPzrF6QP8RTcb5g2rqTrjKJ19uIqHwX1vW7teM6PKmsVm1o4tayvPvSuYJ\n\tgf1WBfc8NdKTmV2yXn4jMwAroq3NaFOgd2ii8FB20jGHADdV/7COg0jmImEE5ZW+5r\n\tjfJxbUWaqGZdKvSksLvwDejsa2FhkyDEEr/5M6Gc7Ksd6NeqvndElQPUhgkUJD0zee\n\tlAofLjrYCvZxQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20210112.gappssmtp.com; s=20210112; t=1678376891;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=+IJh0wgpIRmabMsYKn5de0evnqpeHZBOaMrXU2/MTlk=;\n\tb=kurIYSAVJd/cc28OF5gpEe4RADrVvxWVp/vezMrkiWxiGcv27vEZTQGthmC0VIXOBi\n\t8Zep3wVxFXPY30Q7btRRXCY0Ucu3bTqu6/TuVgAbqlt+X8x+bNmrv0XIf58g8wUGA9Iy\n\t1h7R5n6d/teRIemQRl5qxbq1KstksCMCp+oTFUFCdp9kdnP/o9oHMUkhrhVyboOkrnLZ\n\tfCRPcOSdEJRp98DdqzpViwmZ/l/vfB9qNLWuAiMww1weLLy/vJrYEn7L7UCbfOsSbxW0\n\tm55gv0/3M6NlzIQLZTF3yGbaccewtA11v7xqN1JtK3QJhXUtN9x7FXbiqkwFEptfm25C\n\tlgPA=="],"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=\"kurIYSAV\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1678376891;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=+IJh0wgpIRmabMsYKn5de0evnqpeHZBOaMrXU2/MTlk=;\n\tb=k01Ejfn2sVD+XQM80aKPRDL4j4CUnuTGBHKOWU4a1c60W6/zk+MTJg62VpGuNMHxpH\n\tfHFpxK9gEtLdZR+y7+WdJQr1+mx9Ty6iTyt5ylHeKhjj0vyYVvPw0cjB2U9/8jAhQEEL\n\tepFZKN+OAmsOoprzebuP+pQkYZ4kW047UK57y+h9nLBJBRkdP+t9eqdBlm8dyHVTldtK\n\tnjViYneCHLyVPTdmSAjuSQPCj2H4TCP6XfFcdxoZM93K8BdsV3I1aMQg2li9UJDfyd5k\n\tFFXWZaEXfnHHXaBlj0SoaMbMEF29yrNaPWRr12A9+5Kbcd6IZP62KAkkZLXTdMLtxtps\n\tSCrQ==","X-Gm-Message-State":"AO0yUKUKVMxFL8RTb93KQIuwj8BwT+xASUtOYwtlHBInSvOLzpSnEyhT\n\tVJCTQ5OlCjTSxOTTEOT+5/3x596nBkKzwnLHYbGwIg==","X-Google-Smtp-Source":"AK7set+R3Mv9z0atOYH9XghE8sKltO5B91eDoj/x86DRnGFHtRBmZWkLi5Uo5gbpQ1p31kihdX6BUWW6TWxwGeedJXg=","X-Received":"by 2002:a17:906:6083:b0:919:2e39:95e5 with SMTP id\n\tt3-20020a170906608300b009192e3995e5mr3507378ejj.6.1678376890923;\n\tThu, 09 Mar 2023 07:48:10 -0800 (PST)","MIME-Version":"1.0","References":"<20230119084112.20564-1-dse@thaumatec.com>\n\t<20230119084112.20564-4-dse@thaumatec.com>\n\t<20230206111958.aqlhed5mcht3feyl@uno.localdomain>\n\t\n\t<167569563276.4026431.9508377143472972975@Monstersaurus>\n\t<20230206174655.7mcgtaae7uit2j2g@uno.localdomain>","In-Reply-To":"<20230206174655.7mcgtaae7uit2j2g@uno.localdomain>","Date":"Thu, 9 Mar 2023 16:47:59 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v3 3/8] 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":"Nick Hollinghurst ,\n\tNaushir Patuck via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]