[{"id":26812,"web_url":"https://patchwork.libcamera.org/comment/26812/","msgid":"<20230403085827.xlh5gzh67orohqtk@uno.localdomain>","date":"2023-04-03T08:58:27","subject":"Re: [libcamera-devel] [PATCH v6 03/10] 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 Fri, Mar 31, 2023 at 10:19:23AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> Define common interface with pure virtual methods that should be\n> implemented by the Auto Focus algorithm implementations.\n> Interface methods match the AF controls that can be set in the frame\n> request. Common implementation of controls parsing is provided,\n> so the AF algorithms deriving from this interface should be able\n> to reuse it.\n>\n> Signed-off-by: Daniel Semkowicz \n\nSeems like you've lost my tag :)\n\nHere it is again\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n> ---\n> src/ipa/libipa/algorithms/af.cpp | 159 ++++++++++++++++++++++++++\n> src/ipa/libipa/algorithms/af.h | 46 ++++++++\n> src/ipa/libipa/algorithms/meson.build | 9 ++\n> src/ipa/libipa/meson.build | 6 +\n> 4 files changed, 220 insertions(+)\n> create mode 100644 src/ipa/libipa/algorithms/af.cpp\n> create mode 100644 src/ipa/libipa/algorithms/af.h\n> create mode 100644 src/ipa/libipa/algorithms/meson.build\n>\n> diff --git a/src/ipa/libipa/algorithms/af.cpp b/src/ipa/libipa/algorithms/af.cpp\n> new file mode 100644\n> index 00000000..0808aa3f\n> --- /dev/null\n> +++ b/src/ipa/libipa/algorithms/af.cpp\n> @@ -0,0 +1,159 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Theobroma Systems\n> + *\n> + * af.cpp - Autofocus control algorithm interface\n> + */\n> +\n> +#include \"af.h\"\n> +\n> +/**\n> + * \\file af.h\n> + * \\brief AF algorithm common interface\n> + */\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::algorithms {\n> +\n> +/**\n> + * \\class Af\n> + * \\brief Common interface for auto-focus algorithms\n> + *\n> + * The Af class defines a standard interface for IPA auto focus algorithms.\n> + */\n> +\n> +/**\n> + * \\brief Provide control values to the algorithm\n> + * \\param[in] controls The list of user controls\n> + *\n> + * This method should be called in the libcamera::ipa::Algorithm::queueRequest()\n> + * method of the platform layer.\n> + */\n> +void Af::queueRequest(const ControlList &controls)\n> +{\n> +\tusing namespace controls;\n> +\n> +\tfor (auto const &[id, value] : controls) {\n> +\t\tswitch (id) {\n> +\t\tcase AF_MODE: {\n> +\t\t\tsetMode(static_cast(value.get()));\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase AF_RANGE: {\n> +\t\t\tsetRange(static_cast(value.get()));\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase AF_SPEED: {\n> +\t\t\tsetSpeed(static_cast(value.get()));\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase AF_METERING: {\n> +\t\t\tsetMeteringMode(static_cast(value.get()));\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase AF_WINDOWS: {\n> +\t\t\tsetWindows(value.get>());\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase AF_TRIGGER: {\n> +\t\t\tsetTrigger(static_cast(value.get()));\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase AF_PAUSE: {\n> +\t\t\tsetPause(static_cast(value.get()));\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcase LENS_POSITION: {\n> +\t\t\tsetLensPosition(value.get());\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tdefault:\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +}\n> +\n> +/**\n> + * \\fn Af::setMode()\n> + * \\brief Set auto focus mode\n> + * \\param[in] mode AF mode\n> + *\n> + * \\sa libcamera::controls::AfMode\n> + */\n> +\n> +/**\n> + * \\fn Af::setRange()\n> + * \\brief Set the range of focus distances that is scanned\n> + * \\param[in] range AF range\n> + *\n> + * \\sa libcamera::controls::AfRange\n> + */\n> +\n> +/**\n> + * \\fn Af::setSpeed()\n> + * \\brief Set how fast algorithm should move the lens\n> + * \\param[in] speed Lens move speed\n> + *\n> +* \\sa libcamera::controls::AfSpeed\n> + */\n> +\n> +/**\n> + * \\fn Af::setMeteringMode()\n> + * \\brief Set AF metering mode\n> + * \\param[in] metering AF metering mode\n> + *\n> + * \\sa libcamera::controls::AfMetering\n> + */\n> +\n> +/**\n> + * \\fn Af::setWindows()\n> + * \\brief Set AF windows\n> + * \\param[in] windows AF windows\n> + *\n> + * \\sa libcamera::controls::AfWindows\n> + */\n> +\n> +/**\n> + * \\fn Af::setTrigger()\n> + * \\brief Starts or cancels the autofocus scan\n> + * \\param[in] trigger Trigger mode\n> + *\n> + * \\sa libcamera::controls::AfTrigger\n> + */\n> +\n> +/**\n> + * \\fn Af::setPause()\n> + * \\brief Pause the autofocus while in AfModeContinuous mode\n> + * \\param[in] pause Pause mode\n> + *\n> + * \\sa libcamera::controls::AfPause\n> + */\n> +\n> +/**\n> + * \\fn Af::setLensPosition()\n> + * \\brief Set the lens position while in AfModeManual\n> + * \\param[in] lensPosition Lens position\n> + *\n> + * \\sa libcamera::controls::LensPosition\n> + */\n> +\n> +/**\n> + * \\fn Af::state()\n> + * \\brief Get the current state of the AF algorithm\n> + * \\return AF state\n> + *\n> + * \\sa libcamera::controls::AfState\n> + */\n> +\n> +/**\n> + * \\fn Af::pauseState()\n> + * \\brief Get the current pause state of the AF algorithm\n> + * \\return AF pause state\n> + *\n> + * \\sa libcamera::controls::AfPauseState\n> + */\n> +\n> +} /* namespace ipa::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/algorithms/af.h b/src/ipa/libipa/algorithms/af.h\n> new file mode 100644\n> index 00000000..47c919fe\n> --- /dev/null\n> +++ b/src/ipa/libipa/algorithms/af.h\n> @@ -0,0 +1,46 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Theobroma Systems\n> + *\n> + * af.h - Autofocus control algorithm interface\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::algorithms {\n> +\n> +class Af\n> +{\n> +public:\n> +\tvirtual ~Af() = default;\n> +\n> +\tvoid queueRequest(const ControlList &controls);\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 state() = 0;\n> +\n> +\tvirtual controls::AfPauseStateEnum pauseState() = 0;\n> +};\n> +\n> +} /* namespace ipa::algorithms */\n> +\n> +} /* namespace libcamera */\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..3df4798f\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> +libipa_algorithms_headers = files([\n> + 'af.h',\n> +])\n> +\n> +libipa_algorithms_sources = files([\n> + 'af.cpp',\n> +])\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 016b8e0e..134c2dba 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 += libipa_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 += libipa_algorithms_sources\n> +\n> libipa_includes = include_directories('..')\n>\n> libipa = static_library('ipa', [libipa_sources, libipa_headers],\n> --\n> 2.39.2\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 1B842C0F2A\n\tfor ;\n\tMon, 3 Apr 2023 08:58:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B1EA36272A;\n\tMon, 3 Apr 2023 10:58:31 +0200 (CEST)","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 070E86271C\n\tfor ;\n\tMon, 3 Apr 2023 10:58:30 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A168F6CF;\n\tMon, 3 Apr 2023 10:58:29 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1680512311;\n\tbh=EKhRcW13fRBftP/d5P467MbI4OOCV65lIa6OFILncQE=;\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=A2tbQcKOUvYgsVv0IwKClf2lTPCbDYe/j0MCSUrPbVcQmvE20ephfAJrx1OmfAP9w\n\tIbLb8DDFNr8phl4kbUbxYJOggB4wLGOGnS0QEIR+UdNKU7Aych0KkJZizJ1L2WFcoa\n\tKPzxXbCjZ3J6DDlBc+ZtsFB7/C6OVVhEUU1LsyzxHPDvPG6EHxrRuz0vuoDkb/aBU4\n\tsBAij7Q20FBVrTwMb2Av2Tm886n8SVKBZNBVHDd/VUrU/m+wE+2RYwR4JdPzB5AqqZ\n\tv4PsYXiTtdw9RaPYWqecwnY+GF0qqbeZRnsLRqUOlXiSK1ss+SyGPlZuh0TdKCddle\n\tqynBh8wRj6Q0w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1680512309;\n\tbh=EKhRcW13fRBftP/d5P467MbI4OOCV65lIa6OFILncQE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bDc2xX0pddDBqkALxxVBW4N5ZOfXt5eWkYlAv886n0wxlrZ0/gdfMPJ4KFcnJF8wN\n\tGBrd4SkD+dD4OZF3j0CuLAY14ZXRgRY9ywygp9NYJHKaIohJeGI0olr7j+AyYLfBYR\n\tBZozSRFrCdevoNWCTTge/yRnLAs8JOn4FffvmFCE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"bDc2xX0p\"; dkim-atps=neutral","Date":"Mon, 3 Apr 2023 10:58:27 +0200","To":"Daniel Semkowicz ","Message-ID":"<20230403085827.xlh5gzh67orohqtk@uno.localdomain>","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-4-dse@thaumatec.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230331081930.19289-4-dse@thaumatec.com>","Subject":"Re: [libcamera-devel] [PATCH v6 03/10] 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":27084,"web_url":"https://patchwork.libcamera.org/comment/27084/","msgid":"","date":"2023-05-12T13:45:30","subject":"Re: [libcamera-devel] [PATCH v6 03/10] 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 Laurent,\n\nOn Wed, Apr 26, 2023 at 3:24 AM Laurent Pinchart\n wrote:\n>\n> Hi Daniel,\n>\n> Thank you for the patch.\n>\n> On Fri, Mar 31, 2023 at 10:19:23AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > Define common interface with pure virtual methods that should be\n> > implemented by the Auto Focus algorithm implementations.\n> > Interface methods match the AF controls that can be set in the frame\n> > request. Common implementation of controls parsing is provided,\n> > so the AF algorithms deriving from this interface should be able\n> > to reuse it.\n>\n> I'm a bit concerned here, as creating abstract classes with a single\n> user always worries me that we don't have enough data points to create a\n> correct abstraction. Your series includes a single AF implementation, do\n> you already envision different implementations that could use the same\n> interface ? I'm sorry if this point has been raised during the review of\n> previous versions, I haven't followed them very closely so far.\n\nThis interface basically maps the AF controls on the separate interface\nfunctions and provides queueRequest() function to translate requested controls\nto function calls. This makes the interface closely tied with the AF controls.\nIn the end each AF algorithm needs to handle the AF controls, so it forces\nalgorithm architecture to be structured in the way, the controls are structured.\nI do not envision another implementation yet, but you can already look on the\nRaspberry Pi implementation of the PDAF. This is a different implementation,\nbut the RPiController::AfAlgorithm is structured almost the same way.\n\nBest regards\nDaniel\n\n>\n> > Signed-off-by: Daniel Semkowicz \n> > ---\n> > src/ipa/libipa/algorithms/af.cpp | 159 ++++++++++++++++++++++++++\n> > src/ipa/libipa/algorithms/af.h | 46 ++++++++\n> > src/ipa/libipa/algorithms/meson.build | 9 ++\n> > src/ipa/libipa/meson.build | 6 +\n> > 4 files changed, 220 insertions(+)\n> > create mode 100644 src/ipa/libipa/algorithms/af.cpp\n> > create mode 100644 src/ipa/libipa/algorithms/af.h\n> > create mode 100644 src/ipa/libipa/algorithms/meson.build\n> >\n> > diff --git a/src/ipa/libipa/algorithms/af.cpp b/src/ipa/libipa/algorithms/af.cpp\n> > new file mode 100644\n> > index 00000000..0808aa3f\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/algorithms/af.cpp\n> > @@ -0,0 +1,159 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2023, Theobroma Systems\n> > + *\n> > + * af.cpp - Autofocus control algorithm interface\n> > + */\n> > +\n> > +#include \"af.h\"\n> > +\n> > +/**\n> > + * \\file af.h\n> > + * \\brief AF algorithm common interface\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa::algorithms {\n> > +\n> > +/**\n> > + * \\class Af\n> > + * \\brief Common interface for auto-focus algorithms\n> > + *\n> > + * The Af class defines a standard interface for IPA auto focus algorithms.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Provide control values to the algorithm\n> > + * \\param[in] controls The list of user controls\n> > + *\n> > + * This method should be called in the libcamera::ipa::Algorithm::queueRequest()\n> > + * method of the platform layer.\n> > + */\n> > +void Af::queueRequest(const ControlList &controls)\n> > +{\n> > + using namespace controls;\n> > +\n> > + for (auto const &[id, value] : controls) {\n> > + switch (id) {\n> > + case AF_MODE: {\n> > + setMode(static_cast(value.get()));\n> > + break;\n> > + }\n> > + case AF_RANGE: {\n> > + setRange(static_cast(value.get()));\n> > + break;\n> > + }\n> > + case AF_SPEED: {\n> > + setSpeed(static_cast(value.get()));\n> > + break;\n> > + }\n> > + case AF_METERING: {\n> > + setMeteringMode(static_cast(value.get()));\n> > + break;\n> > + }\n> > + case AF_WINDOWS: {\n> > + setWindows(value.get>());\n> > + break;\n> > + }\n> > + case AF_TRIGGER: {\n> > + setTrigger(static_cast(value.get()));\n> > + break;\n> > + }\n> > + case AF_PAUSE: {\n> > + setPause(static_cast(value.get()));\n> > + break;\n> > + }\n> > + case LENS_POSITION: {\n> > + setLensPosition(value.get());\n> > + break;\n> > + }\n> > + default:\n> > + break;\n> > + }\n> > + }\n> > +}\n> > +\n> > +/**\n> > + * \\fn Af::setMode()\n> > + * \\brief Set auto focus mode\n> > + * \\param[in] mode AF mode\n> > + *\n> > + * \\sa libcamera::controls::AfMode\n> > + */\n> > +\n> > +/**\n> > + * \\fn Af::setRange()\n> > + * \\brief Set the range of focus distances that is scanned\n> > + * \\param[in] range AF range\n> > + *\n> > + * \\sa libcamera::controls::AfRange\n> > + */\n> > +\n> > +/**\n> > + * \\fn Af::setSpeed()\n> > + * \\brief Set how fast algorithm should move the lens\n> > + * \\param[in] speed Lens move speed\n> > + *\n> > +* \\sa libcamera::controls::AfSpeed\n> > + */\n> > +\n> > +/**\n> > + * \\fn Af::setMeteringMode()\n> > + * \\brief Set AF metering mode\n> > + * \\param[in] metering AF metering mode\n> > + *\n> > + * \\sa libcamera::controls::AfMetering\n> > + */\n> > +\n> > +/**\n> > + * \\fn Af::setWindows()\n> > + * \\brief Set AF windows\n> > + * \\param[in] windows AF windows\n> > + *\n> > + * \\sa libcamera::controls::AfWindows\n> > + */\n> > +\n> > +/**\n> > + * \\fn Af::setTrigger()\n> > + * \\brief Starts or cancels the autofocus scan\n> > + * \\param[in] trigger Trigger mode\n> > + *\n> > + * \\sa libcamera::controls::AfTrigger\n> > + */\n> > +\n> > +/**\n> > + * \\fn Af::setPause()\n> > + * \\brief Pause the autofocus while in AfModeContinuous mode\n> > + * \\param[in] pause Pause mode\n> > + *\n> > + * \\sa libcamera::controls::AfPause\n> > + */\n> > +\n> > +/**\n> > + * \\fn Af::setLensPosition()\n> > + * \\brief Set the lens position while in AfModeManual\n> > + * \\param[in] lensPosition Lens position\n> > + *\n> > + * \\sa libcamera::controls::LensPosition\n> > + */\n> > +\n> > +/**\n> > + * \\fn Af::state()\n> > + * \\brief Get the current state of the AF algorithm\n> > + * \\return AF state\n> > + *\n> > + * \\sa libcamera::controls::AfState\n> > + */\n> > +\n> > +/**\n> > + * \\fn Af::pauseState()\n> > + * \\brief Get the current pause state of the AF algorithm\n> > + * \\return AF pause state\n> > + *\n> > + * \\sa libcamera::controls::AfPauseState\n> > + */\n> > +\n> > +} /* namespace ipa::algorithms */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/algorithms/af.h b/src/ipa/libipa/algorithms/af.h\n> > new file mode 100644\n> > index 00000000..47c919fe\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/algorithms/af.h\n> > @@ -0,0 +1,46 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2023, Theobroma Systems\n> > + *\n> > + * af.h - Autofocus control algorithm interface\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n>\n> You should also include geometry.h and span.h as you're using the\n> Rectangle and Span classes below.\n>\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa::algorithms {\n> > +\n> > +class Af\n> > +{\n> > +public:\n> > + virtual ~Af() = default;\n> > +\n> > + void queueRequest(const ControlList &controls);\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 state() = 0;\n> > +\n> > + virtual controls::AfPauseStateEnum pauseState() = 0;\n> > +};\n> > +\n> > +} /* namespace ipa::algorithms */\n> > +\n> > +} /* namespace libcamera */\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..3df4798f\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> > +libipa_algorithms_headers = files([\n> > + 'af.h',\n> > +])\n> > +\n> > +libipa_algorithms_sources = files([\n> > + 'af.cpp',\n> > +])\n> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > index 016b8e0e..134c2dba 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 += libipa_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 += libipa_algorithms_sources\n> > +\n> > libipa_includes = include_directories('..')\n> >\n> > libipa = static_library('ipa', [libipa_sources, libipa_headers],\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 526FBBE08B\n\tfor ;\n\tFri, 12 May 2023 13:45:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B595A633B4;\n\tFri, 12 May 2023 15:45:43 +0200 (CEST)","from mail-ed1-x532.google.com (mail-ed1-x532.google.com\n\t[IPv6:2a00:1450:4864:20::532])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 40394627DC\n\tfor ;\n\tFri, 12 May 2023 15:45:42 +0200 (CEST)","by mail-ed1-x532.google.com with SMTP id\n\t4fb4d7f45d1cf-50bcae898b2so17601438a12.0\n\tfor ;\n\tFri, 12 May 2023 06:45:42 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1683899143;\n\tbh=ZZmPC1zOoCk0xWJTSnq2F3NAa7hPW5e/2sT1G34cU+I=;\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=f/Il2bR6NZapygxULNZ1OX6i6G3e6CfXpvncGPEqRiixnfVF3bWxk3sQE1zj+eEQX\n\tbXZmr7IKN6wSDKksHvnfb4zlp/s2tEUDk/A0XQ6WzuYVtEw06zS+wP0+Y8YRZpj21Y\n\tMEygP3/yXLBuUaT6WmFjh40oEWSeBShSjY0KhBrVTXo3cMQRY/zBL5951wnyTkH+Ya\n\t+TGLOUBR+wQ0pJbQwMIde5SimU8vR1FNrkEz3bB9xUVFHAC1CnxIbCvLVrX/f0AwEq\n\tjvOd53bjJRciaHWxryPvS51DBhnPbMGSJ+h61raCUgKzl1gDU6nH0m9FtqH/LuHEhs\n\tH4co+2MwLMX/A==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20221208.gappssmtp.com; s=20221208; t=1683899142;\n\tx=1686491142; \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=dX19IG43zL565Ya3V/4mGnSdZ9gze0q8rpKFuCZtSpQ=;\n\tb=xScu2jWwt8dhmCSE6vOnWRpKByNi1c63CQJDzaIzr4ISn4IepcHgPOSdK6/U+cX/iS\n\tR1XRxqbOnIoEhOXZ4mSqDm/mKaB2cXIhHY1l8B828i15nx6lcBHi++4SUlv3b/aUBSek\n\tuc8mJXURsn2xDFRoS1ZJr64HkhmEo2gJUJqVkvMRqjfNk3QD3X27ac+dy2lTCLV4meNa\n\t+5mWYLCs8WUJ1QEnKjHqVTIw/5C2HmVQKa8J2zaUfbAliRdBjNQfsj88E3nm4xrPdg1Y\n\tPpmB2wHQsvL5Ar09M2jAYXooSK/2cT+7uC49cWIhpDCprlXopgn8Q8/KsRBd0uv1oHwK\n\tGocw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=thaumatec-com.20221208.gappssmtp.com\n\theader.i=@thaumatec-com.20221208.gappssmtp.com header.b=\"xScu2jWw\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1683899142; x=1686491142;\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=dX19IG43zL565Ya3V/4mGnSdZ9gze0q8rpKFuCZtSpQ=;\n\tb=A6wk9vwTj3DqgLQ1xGAM3TyBfPnv3x6M/s8G+z1HftT391zQxrHxlAEUPsNV6GI47X\n\tIuA3KSmKu3C99otDoSXS0DsZ4NmL9ymxLq+bZLbfPulhWh2LLUdspJBSpvMeuZSTMquV\n\tD3iahsieLUP3e6rmJKvCjvJo7KJVVQOblXVO8sUu5e/io1QOrMGWPA7FRA2lgmEPSS0n\n\tw0t+KcQtw+MGxkcV/SNag4TmXIo78m06hb3AO+izWG3qUnyTNGx2jZmhZ3ZhoZnwtETp\n\traBiYV3Gft2OPuobCh656g+OQLzlC1S8XiPRc/tE6+3nWn3hE/LsePbHSmv2Fgh7Udyf\n\tyYOw==","X-Gm-Message-State":"AC+VfDw2URRuRvLvXmZP0s9Eu91HULZ/hdlpYEGU1tRSubjHNFTGn2S/\n\tkG8ppjQ8FP2C+X8LdLdtZ+TuiXgQkRCVOoSLfrX04KnXf8mQBPyfNtqPAsT0","X-Google-Smtp-Source":"ACHHUZ4BTV7fvJxDl+ZjQYdh070nDirtqhxmqEtCjuaqA89Q9eqldUU3uYC7s5y4oB0KUus9w1VibppLy8rfqTiY5LE=","X-Received":"by 2002:a17:907:842:b0:94e:c8c:42ec with SMTP id\n\tww2-20020a170907084200b0094e0c8c42ecmr21076533ejb.20.1683899141698;\n\tFri, 12 May 2023 06:45:41 -0700 (PDT)","MIME-Version":"1.0","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-4-dse@thaumatec.com>\n\t<20230426012418.GD1667@pendragon.ideasonboard.com>","In-Reply-To":"<20230426012418.GD1667@pendragon.ideasonboard.com>","Date":"Fri, 12 May 2023 15:45:30 +0200","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v6 03/10] 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\" "}},{"id":27194,"web_url":"https://patchwork.libcamera.org/comment/27194/","msgid":"<20230531151841.GD24749@pendragon.ideasonboard.com>","date":"2023-05-31T15:18:41","subject":"Re: [libcamera-devel] [PATCH v6 03/10] ipa: Add base class defining\n\tAF algorithm interface","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Daniel,\n\nGetting back to this mail thread now that I'm back from vacation :-)\n\nOn Fri, May 12, 2023 at 03:45:30PM +0200, Daniel Semkowicz wrote:\n> On Wed, Apr 26, 2023 at 3:24 AM Laurent Pinchart wrote:\n> > On Fri, Mar 31, 2023 at 10:19:23AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > Define common interface with pure virtual methods that should be\n> > > implemented by the Auto Focus algorithm implementations.\n> > > Interface methods match the AF controls that can be set in the frame\n> > > request. Common implementation of controls parsing is provided,\n> > > so the AF algorithms deriving from this interface should be able\n> > > to reuse it.\n> >\n> > I'm a bit concerned here, as creating abstract classes with a single\n> > user always worries me that we don't have enough data points to create a\n> > correct abstraction. Your series includes a single AF implementation, do\n> > you already envision different implementations that could use the same\n> > interface ? I'm sorry if this point has been raised during the review of\n> > previous versions, I haven't followed them very closely so far.\n> \n> This interface basically maps the AF controls on the separate interface\n> functions and provides queueRequest() function to translate requested controls\n> to function calls. This makes the interface closely tied with the AF controls.\n> In the end each AF algorithm needs to handle the AF controls, so it forces\n> algorithm architecture to be structured in the way, the controls are structured.\n> I do not envision another implementation yet, but you can already look on the\n> Raspberry Pi implementation of the PDAF. This is a different implementation,\n> but the RPiController::AfAlgorithm is structured almost the same way.\n\nI'm afraid I'm not thrilled :-( This forces AF controls to be processed\nseparately by the AF algorithm implementation, as they're passed to\ndifferent functions. I can't tell if this will match the needs of\ndifferent AF implementations. As there's fairly little code in this\nhelper, I would rather squash it with the AF implementation for rkisp1.\nWe can extract it in a shared helper later if we realize it can be\nuseful for other platforms.\n\n> > > Signed-off-by: Daniel Semkowicz \n> > > ---\n> > > src/ipa/libipa/algorithms/af.cpp | 159 ++++++++++++++++++++++++++\n> > > src/ipa/libipa/algorithms/af.h | 46 ++++++++\n> > > src/ipa/libipa/algorithms/meson.build | 9 ++\n> > > src/ipa/libipa/meson.build | 6 +\n> > > 4 files changed, 220 insertions(+)\n> > > create mode 100644 src/ipa/libipa/algorithms/af.cpp\n> > > create mode 100644 src/ipa/libipa/algorithms/af.h\n> > > create mode 100644 src/ipa/libipa/algorithms/meson.build\n> > >\n> > > diff --git a/src/ipa/libipa/algorithms/af.cpp b/src/ipa/libipa/algorithms/af.cpp\n> > > new file mode 100644\n> > > index 00000000..0808aa3f\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/algorithms/af.cpp\n> > > @@ -0,0 +1,159 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2023, Theobroma Systems\n> > > + *\n> > > + * af.cpp - Autofocus control algorithm interface\n> > > + */\n> > > +\n> > > +#include \"af.h\"\n> > > +\n> > > +/**\n> > > + * \\file af.h\n> > > + * \\brief AF algorithm common interface\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace ipa::algorithms {\n> > > +\n> > > +/**\n> > > + * \\class Af\n> > > + * \\brief Common interface for auto-focus algorithms\n> > > + *\n> > > + * The Af class defines a standard interface for IPA auto focus algorithms.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Provide control values to the algorithm\n> > > + * \\param[in] controls The list of user controls\n> > > + *\n> > > + * This method should be called in the libcamera::ipa::Algorithm::queueRequest()\n> > > + * method of the platform layer.\n> > > + */\n> > > +void Af::queueRequest(const ControlList &controls)\n> > > +{\n> > > + using namespace controls;\n> > > +\n> > > + for (auto const &[id, value] : controls) {\n> > > + switch (id) {\n> > > + case AF_MODE: {\n> > > + setMode(static_cast(value.get()));\n> > > + break;\n> > > + }\n> > > + case AF_RANGE: {\n> > > + setRange(static_cast(value.get()));\n> > > + break;\n> > > + }\n> > > + case AF_SPEED: {\n> > > + setSpeed(static_cast(value.get()));\n> > > + break;\n> > > + }\n> > > + case AF_METERING: {\n> > > + setMeteringMode(static_cast(value.get()));\n> > > + break;\n> > > + }\n> > > + case AF_WINDOWS: {\n> > > + setWindows(value.get>());\n> > > + break;\n> > > + }\n> > > + case AF_TRIGGER: {\n> > > + setTrigger(static_cast(value.get()));\n> > > + break;\n> > > + }\n> > > + case AF_PAUSE: {\n> > > + setPause(static_cast(value.get()));\n> > > + break;\n> > > + }\n> > > + case LENS_POSITION: {\n> > > + setLensPosition(value.get());\n> > > + break;\n> > > + }\n> > > + default:\n> > > + break;\n> > > + }\n> > > + }\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn Af::setMode()\n> > > + * \\brief Set auto focus mode\n> > > + * \\param[in] mode AF mode\n> > > + *\n> > > + * \\sa libcamera::controls::AfMode\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Af::setRange()\n> > > + * \\brief Set the range of focus distances that is scanned\n> > > + * \\param[in] range AF range\n> > > + *\n> > > + * \\sa libcamera::controls::AfRange\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Af::setSpeed()\n> > > + * \\brief Set how fast algorithm should move the lens\n> > > + * \\param[in] speed Lens move speed\n> > > + *\n> > > +* \\sa libcamera::controls::AfSpeed\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Af::setMeteringMode()\n> > > + * \\brief Set AF metering mode\n> > > + * \\param[in] metering AF metering mode\n> > > + *\n> > > + * \\sa libcamera::controls::AfMetering\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Af::setWindows()\n> > > + * \\brief Set AF windows\n> > > + * \\param[in] windows AF windows\n> > > + *\n> > > + * \\sa libcamera::controls::AfWindows\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Af::setTrigger()\n> > > + * \\brief Starts or cancels the autofocus scan\n> > > + * \\param[in] trigger Trigger mode\n> > > + *\n> > > + * \\sa libcamera::controls::AfTrigger\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Af::setPause()\n> > > + * \\brief Pause the autofocus while in AfModeContinuous mode\n> > > + * \\param[in] pause Pause mode\n> > > + *\n> > > + * \\sa libcamera::controls::AfPause\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Af::setLensPosition()\n> > > + * \\brief Set the lens position while in AfModeManual\n> > > + * \\param[in] lensPosition Lens position\n> > > + *\n> > > + * \\sa libcamera::controls::LensPosition\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Af::state()\n> > > + * \\brief Get the current state of the AF algorithm\n> > > + * \\return AF state\n> > > + *\n> > > + * \\sa libcamera::controls::AfState\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn Af::pauseState()\n> > > + * \\brief Get the current pause state of the AF algorithm\n> > > + * \\return AF pause state\n> > > + *\n> > > + * \\sa libcamera::controls::AfPauseState\n> > > + */\n> > > +\n> > > +} /* namespace ipa::algorithms */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/libipa/algorithms/af.h b/src/ipa/libipa/algorithms/af.h\n> > > new file mode 100644\n> > > index 00000000..47c919fe\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/algorithms/af.h\n> > > @@ -0,0 +1,46 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2023, Theobroma Systems\n> > > + *\n> > > + * af.h - Autofocus control algorithm interface\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> >\n> > You should also include geometry.h and span.h as you're using the\n> > Rectangle and Span classes below.\n> >\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace ipa::algorithms {\n> > > +\n> > > +class Af\n> > > +{\n> > > +public:\n> > > + virtual ~Af() = default;\n> > > +\n> > > + void queueRequest(const ControlList &controls);\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 state() = 0;\n> > > +\n> > > + virtual controls::AfPauseStateEnum pauseState() = 0;\n> > > +};\n> > > +\n> > > +} /* namespace ipa::algorithms */\n> > > +\n> > > +} /* namespace libcamera */\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..3df4798f\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> > > +libipa_algorithms_headers = files([\n> > > + 'af.h',\n> > > +])\n> > > +\n> > > +libipa_algorithms_sources = files([\n> > > + 'af.cpp',\n> > > +])\n> > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > > index 016b8e0e..134c2dba 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 += libipa_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 += libipa_algorithms_sources\n> > > +\n> > > libipa_includes = include_directories('..')\n> > >\n> > > libipa = static_library('ipa', [libipa_sources, libipa_headers],","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 DC79CC3213\n\tfor ;\n\tWed, 31 May 2023 15:18:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6F80A626FE;\n\tWed, 31 May 2023 17:18:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2E364626FA\n\tfor ;\n\tWed, 31 May 2023 17:18:43 +0200 (CEST)","from pendragon.ideasonboard.com (om126205251136.34.openmobile.ne.jp\n\t[126.205.251.136])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CBECD844;\n\tWed, 31 May 2023 17:18:20 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685546324;\n\tbh=PSHWgjyExmAi8EGC+Np3BdV4XLTw6xKWdMWDURrS0x4=;\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=VwG9G7UtWcvu0FKJJwJuv8b489GBN32vM8MPqjTOhNW0ZTnjNPsF5Pqf2IkCYCh+P\n\tMoShUv/o8AhAuZbqC4I2e5lZAA9oe6s80FFBteNbdWekRitAo971JK14pRvyRd55R6\n\t+dxW7pdWhPkIn2mBuU/PosZpEiYm8CeYjeGgPZ3jt/g07A0nrToiju7ARi9FnlO0TI\n\t8bf1h7gqeVzvk7g1jq0JpKghzX1HCb5rDYHLOtfoaa1fj4YO1VgvUWYjqTBDFZEUhr\n\tne3E+WCZesaUDUYwGZ8TFJJmLBZsCrIR4ZdumjCsJNjMt1x3CLXBmWRlj5TJ4m4mNQ\n\tSCuu3Z2jS10fw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685546301;\n\tbh=PSHWgjyExmAi8EGC+Np3BdV4XLTw6xKWdMWDURrS0x4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=c+jvMje/a4jG5bKqf+O1BZH0VbGpW9I6DgkDA07P0bS1WPVg9Mj/4PoCBYNMVx7Wn\n\tY56X1OA5b4Ot1TDCpRUI8MLM+N54oKOqDO7ZNb8kpA07+w1lJSC85OT9BxtC8OxE+W\n\tPeoCCWH4Rc+lLbgHEkvG6w+Uqi1AEQOwnrVwoPTM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"c+jvMje/\"; dkim-atps=neutral","Date":"Wed, 31 May 2023 18:18:41 +0300","To":"Daniel Semkowicz ","Message-ID":"<20230531151841.GD24749@pendragon.ideasonboard.com>","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-4-dse@thaumatec.com>\n\t<20230426012418.GD1667@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 03/10] 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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27209,"web_url":"https://patchwork.libcamera.org/comment/27209/","msgid":"","date":"2023-06-01T08:13:18","subject":"Re: [libcamera-devel] [PATCH v6 03/10] 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":"Hello Laurent,\n\nOn Wed, May 31, 2023 at 5:18 PM Laurent Pinchart\n wrote:\n>\n> Hi Daniel,\n>\n> Getting back to this mail thread now that I'm back from vacation :-)\n\nI hope you had a good time :)\n\n>\n> On Fri, May 12, 2023 at 03:45:30PM +0200, Daniel Semkowicz wrote:\n> > On Wed, Apr 26, 2023 at 3:24 AM Laurent Pinchart wrote:\n> > > On Fri, Mar 31, 2023 at 10:19:23AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > Define common interface with pure virtual methods that should be\n> > > > implemented by the Auto Focus algorithm implementations.\n> > > > Interface methods match the AF controls that can be set in the frame\n> > > > request. Common implementation of controls parsing is provided,\n> > > > so the AF algorithms deriving from this interface should be able\n> > > > to reuse it.\n> > >\n> > > I'm a bit concerned here, as creating abstract classes with a single\n> > > user always worries me that we don't have enough data points to create a\n> > > correct abstraction. Your series includes a single AF implementation, do\n> > > you already envision different implementations that could use the same\n> > > interface ? I'm sorry if this point has been raised during the review of\n> > > previous versions, I haven't followed them very closely so far.\n> >\n> > This interface basically maps the AF controls on the separate interface\n> > functions and provides queueRequest() function to translate requested controls\n> > to function calls. This makes the interface closely tied with the AF controls.\n> > In the end each AF algorithm needs to handle the AF controls, so it forces\n> > algorithm architecture to be structured in the way, the controls are structured.\n> > I do not envision another implementation yet, but you can already look on the\n> > Raspberry Pi implementation of the PDAF. This is a different implementation,\n> > but the RPiController::AfAlgorithm is structured almost the same way.\n>\n> I'm afraid I'm not thrilled :-( This forces AF controls to be processed\n> separately by the AF algorithm implementation, as they're passed to\n> different functions. I can't tell if this will match the needs of\n> different AF implementations. As there's fairly little code in this\n> helper, I would rather squash it with the AF implementation for rkisp1.\n> We can extract it in a shared helper later if we realize it can be\n> useful for other platforms.\n\nThere are a lot of unknowns at this point, so it is hard to define the\ngood interface that will match all requirements in the future.\n\nIf you would like to avoid defining the base Af interface right now,\nI would rather squash the ipa::algorithms::Af with the\nipa::algorithms::AfHillClimbing. AfHillClimbing can be used at least in\ntwo platforms (rkisp1 and ipu3), so it would be good to have this as a\ncommon part.\n\n>\n> > > > Signed-off-by: Daniel Semkowicz \n> > > > ---\n> > > > src/ipa/libipa/algorithms/af.cpp | 159 ++++++++++++++++++++++++++\n> > > > src/ipa/libipa/algorithms/af.h | 46 ++++++++\n> > > > src/ipa/libipa/algorithms/meson.build | 9 ++\n> > > > src/ipa/libipa/meson.build | 6 +\n> > > > 4 files changed, 220 insertions(+)\n> > > > create mode 100644 src/ipa/libipa/algorithms/af.cpp\n> > > > create mode 100644 src/ipa/libipa/algorithms/af.h\n> > > > create mode 100644 src/ipa/libipa/algorithms/meson.build\n> > > >\n> > > > diff --git a/src/ipa/libipa/algorithms/af.cpp b/src/ipa/libipa/algorithms/af.cpp\n> > > > new file mode 100644\n> > > > index 00000000..0808aa3f\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/libipa/algorithms/af.cpp\n> > > > @@ -0,0 +1,159 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2023, Theobroma Systems\n> > > > + *\n> > > > + * af.cpp - Autofocus control algorithm interface\n> > > > + */\n> > > > +\n> > > > +#include \"af.h\"\n> > > > +\n> > > > +/**\n> > > > + * \\file af.h\n> > > > + * \\brief AF algorithm common interface\n> > > > + */\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +namespace ipa::algorithms {\n> > > > +\n> > > > +/**\n> > > > + * \\class Af\n> > > > + * \\brief Common interface for auto-focus algorithms\n> > > > + *\n> > > > + * The Af class defines a standard interface for IPA auto focus algorithms.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\brief Provide control values to the algorithm\n> > > > + * \\param[in] controls The list of user controls\n> > > > + *\n> > > > + * This method should be called in the libcamera::ipa::Algorithm::queueRequest()\n> > > > + * method of the platform layer.\n> > > > + */\n> > > > +void Af::queueRequest(const ControlList &controls)\n> > > > +{\n> > > > + using namespace controls;\n> > > > +\n> > > > + for (auto const &[id, value] : controls) {\n> > > > + switch (id) {\n> > > > + case AF_MODE: {\n> > > > + setMode(static_cast(value.get()));\n> > > > + break;\n> > > > + }\n> > > > + case AF_RANGE: {\n> > > > + setRange(static_cast(value.get()));\n> > > > + break;\n> > > > + }\n> > > > + case AF_SPEED: {\n> > > > + setSpeed(static_cast(value.get()));\n> > > > + break;\n> > > > + }\n> > > > + case AF_METERING: {\n> > > > + setMeteringMode(static_cast(value.get()));\n> > > > + break;\n> > > > + }\n> > > > + case AF_WINDOWS: {\n> > > > + setWindows(value.get>());\n> > > > + break;\n> > > > + }\n> > > > + case AF_TRIGGER: {\n> > > > + setTrigger(static_cast(value.get()));\n> > > > + break;\n> > > > + }\n> > > > + case AF_PAUSE: {\n> > > > + setPause(static_cast(value.get()));\n> > > > + break;\n> > > > + }\n> > > > + case LENS_POSITION: {\n> > > > + setLensPosition(value.get());\n> > > > + break;\n> > > > + }\n> > > > + default:\n> > > > + break;\n> > > > + }\n> > > > + }\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\fn Af::setMode()\n> > > > + * \\brief Set auto focus mode\n> > > > + * \\param[in] mode AF mode\n> > > > + *\n> > > > + * \\sa libcamera::controls::AfMode\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn Af::setRange()\n> > > > + * \\brief Set the range of focus distances that is scanned\n> > > > + * \\param[in] range AF range\n> > > > + *\n> > > > + * \\sa libcamera::controls::AfRange\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn Af::setSpeed()\n> > > > + * \\brief Set how fast algorithm should move the lens\n> > > > + * \\param[in] speed Lens move speed\n> > > > + *\n> > > > +* \\sa libcamera::controls::AfSpeed\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn Af::setMeteringMode()\n> > > > + * \\brief Set AF metering mode\n> > > > + * \\param[in] metering AF metering mode\n> > > > + *\n> > > > + * \\sa libcamera::controls::AfMetering\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn Af::setWindows()\n> > > > + * \\brief Set AF windows\n> > > > + * \\param[in] windows AF windows\n> > > > + *\n> > > > + * \\sa libcamera::controls::AfWindows\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn Af::setTrigger()\n> > > > + * \\brief Starts or cancels the autofocus scan\n> > > > + * \\param[in] trigger Trigger mode\n> > > > + *\n> > > > + * \\sa libcamera::controls::AfTrigger\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn Af::setPause()\n> > > > + * \\brief Pause the autofocus while in AfModeContinuous mode\n> > > > + * \\param[in] pause Pause mode\n> > > > + *\n> > > > + * \\sa libcamera::controls::AfPause\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn Af::setLensPosition()\n> > > > + * \\brief Set the lens position while in AfModeManual\n> > > > + * \\param[in] lensPosition Lens position\n> > > > + *\n> > > > + * \\sa libcamera::controls::LensPosition\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn Af::state()\n> > > > + * \\brief Get the current state of the AF algorithm\n> > > > + * \\return AF state\n> > > > + *\n> > > > + * \\sa libcamera::controls::AfState\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn Af::pauseState()\n> > > > + * \\brief Get the current pause state of the AF algorithm\n> > > > + * \\return AF pause state\n> > > > + *\n> > > > + * \\sa libcamera::controls::AfPauseState\n> > > > + */\n> > > > +\n> > > > +} /* namespace ipa::algorithms */\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/ipa/libipa/algorithms/af.h b/src/ipa/libipa/algorithms/af.h\n> > > > new file mode 100644\n> > > > index 00000000..47c919fe\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/libipa/algorithms/af.h\n> > > > @@ -0,0 +1,46 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2023, Theobroma Systems\n> > > > + *\n> > > > + * af.h - Autofocus control algorithm interface\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > >\n> > > You should also include geometry.h and span.h as you're using the\n> > > Rectangle and Span classes below.\n> > >\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +namespace ipa::algorithms {\n> > > > +\n> > > > +class Af\n> > > > +{\n> > > > +public:\n> > > > + virtual ~Af() = default;\n> > > > +\n> > > > + void queueRequest(const ControlList &controls);\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 state() = 0;\n> > > > +\n> > > > + virtual controls::AfPauseStateEnum pauseState() = 0;\n> > > > +};\n> > > > +\n> > > > +} /* namespace ipa::algorithms */\n> > > > +\n> > > > +} /* namespace libcamera */\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..3df4798f\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> > > > +libipa_algorithms_headers = files([\n> > > > + 'af.h',\n> > > > +])\n> > > > +\n> > > > +libipa_algorithms_sources = files([\n> > > > + 'af.cpp',\n> > > > +])\n> > > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > > > index 016b8e0e..134c2dba 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 += libipa_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 += libipa_algorithms_sources\n> > > > +\n> > > > libipa_includes = include_directories('..')\n> > > >\n> > > > libipa = static_library('ipa', [libipa_sources, libipa_headers],\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n\nBest regards\nDaniel","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 8E77CC3200\n\tfor ;\n\tThu, 1 Jun 2023 08:13:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A02B626FA;\n\tThu, 1 Jun 2023 10:13:33 +0200 (CEST)","from mail-lj1-x236.google.com (mail-lj1-x236.google.com\n\t[IPv6:2a00:1450:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F27BA61EA6\n\tfor ;\n\tThu, 1 Jun 2023 10:13:30 +0200 (CEST)","by mail-lj1-x236.google.com with SMTP id\n\t38308e7fff4ca-2b1a66e71f9so434261fa.2\n\tfor ;\n\tThu, 01 Jun 2023 01:13:30 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685607213;\n\tbh=goP4Q5uXIm7E6fFjyFubRHg/3PbvSCznNUvDVvh77bM=;\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=NRpfL6k47tu6JW+YrzbSjeBLzT2hm+HGM4xKKBJJ8MhWZUYIsueh/p06V9YghHVpf\n\tAUklanrpu01qYdjQUeeha+F0Jnv9D98GehczDPsW7jkSrNK79CB82FgEoEggk/woWr\n\tRaSFkm4++ldiiK97uc0P3WCyaZ7QgdX5kfM3lM0+uSsHBiEOz+boCdc2YyFJiWHXY7\n\tlA9uTvr1WJikHdjwVjbl6wSwrJ+bxUN7+e2HU/Qv1/ub/uOwIPEavf8uwJc7TE6UuU\n\tzR0CEspmQiJ/oiFIExYXT5fQYEJ/ud3zw5pKYJZ331FzFgR9YaEn76SXH2XPOX66ZQ\n\tnXQPK+D4fEkNA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20221208.gappssmtp.com; s=20221208; t=1685607210;\n\tx=1688199210; \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=1PTaltVYeTW6iglZJq6wwAsIoMSf4nYruf5P+e7HHGA=;\n\tb=N8sZ/18fzqPlulZnD9FKwqiD5Tsuv7lnAOnVHMhmG6Pz8bfrdFpdncFX7Ky1C6oCll\n\tsTj4s0exiVbLy5GnTmW8dRmQYSrIhy55b5vBb77utmkY8MszCnW/AyB5esp1PJbY6J7m\n\ttluqUuwVIcB1cs0yKrkzRHJghx4WT/J2cFija6Zh0lFgNiBSCmW8/2i/O7MFKQDukFO7\n\toCRj98JuELOpioV4L2qLBC/sCBT3+uFDzOw+jHW5iiO89o3yUwulChOsboV5NBj1C6gy\n\tiAeiiM47IpPuV9myfVTfReThiApnwvJUk+9Nqd8aFbWa0xRrJx+eXRvAtKiiVnBzqahl\n\tT1FQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=thaumatec-com.20221208.gappssmtp.com\n\theader.i=@thaumatec-com.20221208.gappssmtp.com header.b=\"N8sZ/18f\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1685607210; x=1688199210;\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=1PTaltVYeTW6iglZJq6wwAsIoMSf4nYruf5P+e7HHGA=;\n\tb=DU1mjLEOt+qtvvAij9CqGIS3wX9DbshTQfvEgSMF6SC0YTofxbeVzew7V00rpz+Hc2\n\tGRfh1NDLJguLZbG7kh00fvYTttq7BTNegjHD48z01RdLivHkis1FPal7kHA7SKp0ojkt\n\tHWNZcNwjZdvtwCiTdN2v2gedRkGmsvN8Y0kud+RJFLo9hBtiYvLoCWNExELBTbwzuNge\n\t0Re9jK0V/qtTlsSYESam7LSoPHutbazUCjueem0UNVOHdLt+xEN1GLOwG6uBy0ivdnCQ\n\tArxSEBygVfe8DXa23hi2P+Uw7amhYMCE+X6Sb3eRIBa9D4GYYVLY84XfXe/wLTQBNM7T\n\tKWSg==","X-Gm-Message-State":"AC+VfDzyeVtHZaPpzHGu9UCGIL02xiNmBICHMex+lXtX2X74we2mW4pP\n\tNSmaH3ydKZvy+M1HjzfTmo1oaMZKhsCg4G44KK3d/g==","X-Google-Smtp-Source":"ACHHUZ4jL+xwI1z0kkf6Xbv/zoByNXM+q/eSV1jXLmaZ2qxKIr2flNWokOwZcxur6acHeJmglVwcYCERfuSmSWAO1OM=","X-Received":"by 2002:a2e:9305:0:b0:2ad:f8d:dea with SMTP id\n\te5-20020a2e9305000000b002ad0f8d0deamr4128694ljh.11.1685607210005; \n\tThu, 01 Jun 2023 01:13:30 -0700 (PDT)","MIME-Version":"1.0","References":"<20230331081930.19289-1-dse@thaumatec.com>\n\t<20230331081930.19289-4-dse@thaumatec.com>\n\t<20230426012418.GD1667@pendragon.ideasonboard.com>\n\t\n\t<20230531151841.GD24749@pendragon.ideasonboard.com>","In-Reply-To":"<20230531151841.GD24749@pendragon.ideasonboard.com>","Date":"Thu, 1 Jun 2023 10:13:18 +0200","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v6 03/10] 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\" "}}]