[{"id":23882,"web_url":"https://patchwork.libcamera.org/comment/23882/","msgid":"<20220714161723.2w6p6c6odeapcsfq@uno.localdomain>","date":"2022-07-14T16:17:23","subject":"Re: [libcamera-devel] [PATCH v2 02/11] ipa: Add class that\n\timplements base AF control algorithm","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Daniel\n\nOn Wed, Jul 13, 2022 at 10:43:08AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> Move the code that was common for IPU3 and RPi AF algorithms to\n> a separate class independent of platform specific code.\n> This way each platform can just implement contrast calculation and\n> run the AF control loop basing on this class.\n>\n\nThis is exactly the purpose of having algorithms in libipa. I'm\nexcited it's the first \"generic\" one we have, or that I know of at\nleast!\n\nThis mean the very IPU3 implementation this algorithm is based on\n(Kate in cc) can now be generalized, and the same for the RPi one we\nhave on the list!\n\n> Signed-off-by: Daniel Semkowicz \n> ---\n> .../libipa/algorithms/af_hill_climbing.cpp | 89 +++++++\n> src/ipa/libipa/algorithms/af_hill_climbing.h | 251 ++++++++++++++++++\n> src/ipa/libipa/algorithms/meson.build | 2 +\n> 3 files changed, 342 insertions(+)\n> create mode 100644 src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> create mode 100644 src/ipa/libipa/algorithms/af_hill_climbing.h\n>\n> diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> new file mode 100644\n> index 00000000..f666c6c2\n> --- /dev/null\n> +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> @@ -0,0 +1,89 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Red Hat\n> + * Copyright (C) 2022, Ideas On Board\n> + * Copyright (C) 2022, Theobroma Systems\n> + *\n> + * af_hill_climbing.cpp - AF Hill Climbing common algorithm\n> + */\n\nAny particular reason why the implementation is in the .h file ?\n\n> +\n> +#include \"af_hill_climbing.h\"\n> +\n> +/**\n> + * \\file af_hill_climbing.h\n> + * \\brief AF Hill Climbing common algorithm\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Af)\n> +\n> +namespace ipa::common::algorithms {\n> +\n> +/**\n> + * \\class AfHillClimbing\n> + * \\brief The base class implementing hill climbing AF control algorithm\n> + * \\tparam Module The IPA module type for this class of algorithms\n> + *\n> + * Control part of auto focus algorithm. It calculates the lens position basing\n> + * on contrast measure supplied by the higher level. This way it is independent\n> + * from the platform.\n> + *\n> + * Derived class should call processAutofocus() for each measured contrast value\n> + * and set the lens to the calculated position.\n> + */\n> +\n> +/**\n> + * \\fn AfHillClimbing::setMode()\n> + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setMode\n> + */\n> +\n> +/**\n> + * \\fn AfHillClimbing::setRange()\n> + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setRange\n> + */\n> +\n> +/**\n> + * \\fn AfHillClimbing::setSpeed()\n> + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setSpeed\n> + */\n> +\n> +/**\n> + * \\fn AfHillClimbing::setMetering()\n> + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setMetering\n> + */\n> +\n> +/**\n> + * \\fn AfHillClimbing::setWindows()\n> + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setWindows\n> + */\n> +\n> +/**\n> + * \\fn AfHillClimbing::setTrigger()\n> + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setTrigger\n> + */\n> +\n> +/**\n> + * \\fn AfHillClimbing::setPause()\n> + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setPause\n> + */\n> +\n> +/**\n> + * \\fn AfHillClimbing::setLensPosition()\n> + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setLensPosition\n> + */\n> +\n> +/**\n> + * \\fn AfHillClimbing::processAutofocus()\n> + * \\brief Run the auto focus algorithm loop\n> + * \\param[in] currentContrast New value of contrast measured for current frame\n> + *\n> + * This method should be called for each new contrast value that was measured,\n> + * usually in the process() method.\n> + *\n> + * \\return New lens position calculated by AF algorithm\n> + */\n> +\n> +} /* namespace ipa::common::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> new file mode 100644\n> index 00000000..db9fc058\n> --- /dev/null\n> +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> @@ -0,0 +1,251 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Red Hat\n> + * Copyright (C) 2022, Ideas On Board\n> + * Copyright (C) 2022, Theobroma Systems\n> + *\n> + * af_hill_climbing.h - AF Hill Climbing common algorithm\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \"af_algorithm.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(Af)\n> +\n> +namespace ipa::common::algorithms {\n> +\n> +template\n> +class AfHillClimbing : public AfAlgorithm\n> +{\n> +public:\n> +\tAfHillClimbing()\n> +\t\t: mode_(controls::AfModeAuto), state_(controls::AfStateIdle),\n> +\t\t pauseState_(controls::AfPauseStateRunning),\n> +\t\t lensPosition_(0), bestPosition_(0), currentContrast_(0.0),\n> +\t\t previousContrast_(0.0), maxContrast_(0.0), maxStep_(0),\n> +\t\t coarseCompleted_(false), fineCompleted_(false),\n> +\t\t lowStep_(0), highStep_(kMaxFocusSteps)\n> +\t{\n> +\t}\n\nLet's look at the implementation details later but I have one question\n\n> +\n> +\tvirtual ~AfHillClimbing() {}\n> +\n> +\tvoid setMode(controls::AfModeEnum mode) final\n> +\t{\n> +\t\tif (mode != mode_) {\n> +\t\t\tLOG(Af, Debug) << \"Switched AF mode from \" << mode_\n> +\t\t\t\t << \" to \" << mode;\n> +\t\t\tpauseState_ = libcamera::controls::AfPauseStateRunning;\n> +\t\t\tmode_ = mode;\n> +\t\t}\n> +\t}\n> +\n> +\tvoid setRange([[maybe_unused]] controls::AfRangeEnum range) final\n> +\t{\n> +\t\tLOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> +\t}\n> +\n> +\tvoid setSpeed([[maybe_unused]] controls::AfSpeedEnum speed) final\n> +\t{\n> +\t\tLOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> +\t}\n> +\n> +\tvoid setTrigger(controls::AfTriggerEnum trigger) final\n> +\t{\n> +\t\tLOG(Af, Debug) << \"Trigger called in mode \" << mode_\n> +\t\t\t << \" with \" << trigger;\n> +\t\tif (mode_ == libcamera::controls::AfModeAuto) {\n> +\t\t\tif (trigger == libcamera::controls::AfTriggerStart)\n> +\t\t\t\tafReset();\n\nThis seems out of place..\nWhenever I trigger an autofocus scan I get the lens reset to 0 causing\na not very nice effect. Why are you resetting the lens position here?\n\n> +\t\t\telse\n> +\t\t\t\tstate_ = libcamera::controls::AfStateIdle;\n> +\t\t}\n> +\t}\n> +\n> +\tvoid setPause(controls::AfPauseEnum pause) final\n> +\t{\n> +\t\t/* \\todo: add the AfPauseDeferred mode */\n> +\t\tif (mode_ == libcamera::controls::AfModeContinuous) {\n> +\t\t\tif (pause == libcamera::controls::AfPauseImmediate)\n> +\t\t\t\tpauseState_ = libcamera::controls::AfPauseStatePaused;\n> +\t\t\telse if (pause == libcamera::controls::AfPauseResume)\n> +\t\t\t\tpauseState_ = libcamera::controls::AfPauseStateRunning;\n> +\t\t}\n> +\t}\n> +\n> +\tvoid setLensPosition([[maybe_unused]] float lensPosition) final\n> +\t{\n> +\t\tLOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> +\t}\n> +\n> +\t/* These methods should be implemented by derived class */\n> +\tvirtual void setMetering(controls::AfMeteringEnum metering) = 0;\n> +\tvirtual void setWindows(Span windows) = 0;\n> +\n> +protected:\n> +\tuint32_t processAutofocus(double currentContrast)\n> +\t{\n> +\t\tcurrentContrast_ = currentContrast;\n> +\n> +\t\t/* If we are in a paused state, we won't process the stats */\n> +\t\tif (pauseState_ == libcamera::controls::AfPauseStatePaused)\n> +\t\t\treturn lensPosition_;\n> +\n> +\t\t/* Depending on the mode, we may or may not process the stats */\n> +\t\tif (state_ == libcamera::controls::AfStateIdle)\n> +\t\t\treturn lensPosition_;\n> +\n> +\t\tif (state_ != libcamera::controls::AfStateFocused) {\n> +\t\t\tafCoarseScan();\n> +\t\t\tafFineScan();\n> +\t\t} else {\n> +\t\t\t/* We can re-start the scan at any moment in AfModeContinuous */\n> +\t\t\tif (mode_ == libcamera::controls::AfModeContinuous)\n> +\t\t\t\tif (afIsOutOfFocus())\n> +\t\t\t\t\tafReset();\n> +\t\t}\n> +\n> +\t\treturn lensPosition_;\n\nLet me recap one point. We are still missing a generic definition for\nthe lens position value. As you can see we use\nV4L2_CID_FOCUS_ABSOLUTE which transports the actual value to be set in\nthe v4l2 control. This can't work here and we are planning to remove\nV4L2 controls from the IPA in general, but while for other controls we\nhave defined generic ranges, for the lens position we are still\nmissing a way to compute a \"generic\" value in the IPA, send it to the\npipeline handler by using libcamera::controls::LensPosition and have\nthe CameraLens class translate it to the device-specific value.\n\nI'm a bit lazy and I'm not chasing how lensPosition_ is here computed,\nbut have you considered how this could be generalized already ? As an\nexample, it seems to me the values I get are in the 0-180 range, while\nin example the lens I'm using ranges from 0-4096...\n\n> +\t}\n> +\n> +private:\n> +\tvoid afCoarseScan()\n> +\t{\n> +\t\tif (coarseCompleted_)\n> +\t\t\treturn;\n> +\n> +\t\tif (afScan(kCoarseSearchStep)) {\n> +\t\t\tcoarseCompleted_ = true;\n> +\t\t\tmaxContrast_ = 0;\n> +\t\t\tlensPosition_ = lensPosition_ - (lensPosition_ * kFineRange);\n> +\t\t\tpreviousContrast_ = 0;\n> +\t\t\tmaxStep_ = std::clamp(lensPosition_ + static_cast((lensPosition_ * kFineRange)),\n> +\t\t\t\t\t 0U, highStep_);\n> +\t\t}\n> +\t}\n> +\n> +\tvoid afFineScan()\n> +\t{\n> +\t\tif (!coarseCompleted_)\n> +\t\t\treturn;\n> +\n> +\t\tif (afScan(kFineSearchStep)) {\n> +\t\t\tLOG(Af, Debug) << \"AF found the best focus position !\";\n> +\t\t\tstate_ = libcamera::controls::AfStateFocused;\n> +\t\t\tfineCompleted_ = true;\n> +\t\t}\n> +\t}\n> +\n> +\tbool afScan(uint32_t minSteps)\n> +\t{\n> +\t\tif (lensPosition_ + minSteps > maxStep_) {\n> +\t\t\t/* If the max step is reached, move lens to the position. */\n> +\t\t\tlensPosition_ = bestPosition_;\n> +\t\t\treturn true;\n> +\t\t} else {\n> +\t\t\t/*\n> +\t\t\t* Find the maximum of the variance by estimating its\n> +\t\t\t* derivative. If the direction changes, it means we have passed\n> +\t\t\t* a maximum one step before.\n> +\t\t\t*/\n> +\t\t\tif ((currentContrast_ - maxContrast_) >= -(maxContrast_ * 0.1)) {\n> +\t\t\t\t/*\n> +\t\t\t\t* Positive and zero derivative:\n> +\t\t\t\t* The variance is still increasing. The focus could be\n> +\t\t\t\t* increased for the next comparison. Also, the max\n> +\t\t\t\t* variance and previous focus value are updated.\n> +\t\t\t\t*/\n> +\t\t\t\tbestPosition_ = lensPosition_;\n> +\t\t\t\tlensPosition_ += minSteps;\n> +\t\t\t\tmaxContrast_ = currentContrast_;\n> +\t\t\t} else {\n> +\t\t\t\t/*\n> +\t\t\t\t* Negative derivative:\n> +\t\t\t\t* The variance starts to decrease which means the maximum\n> +\t\t\t\t* variance is found. Set focus step to previous good one\n> +\t\t\t\t* then return immediately.\n> +\t\t\t\t*/\n> +\t\t\t\tlensPosition_ = bestPosition_;\n> +\t\t\t\treturn true;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tpreviousContrast_ = currentContrast_;\n> +\t\tLOG(Af, Debug) << \" Previous step is \" << bestPosition_\n> +\t\t\t << \" Current step is \" << lensPosition_;\n> +\t\treturn false;\n> +\t}\n> +\n> +\tvoid afReset()\n> +\t{\n> +\t\tLOG(Af, Debug) << \"Reset AF parameters\";\n> +\t\tlensPosition_ = lowStep_;\n> +\t\tmaxStep_ = highStep_;\n> +\t\tstate_ = libcamera::controls::AfStateScanning;\n> +\t\tpreviousContrast_ = 0.0;\n> +\t\tcoarseCompleted_ = false;\n> +\t\tfineCompleted_ = false;\n> +\t\tmaxContrast_ = 0.0;\n> +\t}\n> +\n> +\tbool afIsOutOfFocus()\n> +\t{\n> +\t\tconst uint32_t diff_var = std::abs(currentContrast_ -\n> +\t\t\t\t\t\t maxContrast_);\n> +\t\tconst double var_ratio = diff_var / maxContrast_;\n> +\t\tLOG(Af, Debug) << \"Variance change rate: \" << var_ratio\n> +\t\t\t << \" Current VCM step: \" << lensPosition_;\n> +\t\tif (var_ratio > kMaxChange)\n> +\t\t\treturn true;\n> +\t\telse\n> +\t\t\treturn false;\n> +\t}\n> +\n> +\tcontrols::AfModeEnum mode_;\n> +\tcontrols::AfStateEnum state_;\n> +\tcontrols::AfPauseStateEnum pauseState_;\n> +\n> +\t/* VCM step configuration. It is the current setting of the VCM step. */\n> +\tuint32_t lensPosition_;\n> +\t/* The best VCM step. It is a local optimum VCM step during scanning. */\n> +\tuint32_t bestPosition_;\n> +\n> +\t/* Current AF statistic contrast. */\n> +\tdouble currentContrast_;\n> +\t/* It is used to determine the derivative during scanning */\n> +\tdouble previousContrast_;\n> +\tdouble maxContrast_;\n> +\t/* The designated maximum range of focus scanning. */\n> +\tuint32_t maxStep_;\n> +\t/* If the coarse scan completes, it is set to true. */\n> +\tbool coarseCompleted_;\n> +\t/* If the fine scan completes, it is set to true. */\n> +\tbool fineCompleted_;\n> +\n> +\tuint32_t lowStep_;\n> +\tuint32_t highStep_;\n> +\n> +\t/*\n> +\t* Maximum focus steps of the VCM control\n> +\t* \\todo should be obtained from the VCM driver\n> +\t*/\n> +\tstatic constexpr uint32_t kMaxFocusSteps = 1023;\n> +\n> +\t/* Minimum focus step for searching appropriate focus */\n> +\tstatic constexpr uint32_t kCoarseSearchStep = 30;\n> +\tstatic constexpr uint32_t kFineSearchStep = 1;\n> +\n> +\t/* Max ratio of variance change, 0.0 < kMaxChange < 1.0 */\n> +\tstatic constexpr double kMaxChange = 0.5;\n> +\n> +\t/* Fine scan range 0 < kFineRange < 1 */\n> +\tstatic constexpr double kFineRange = 0.05;\n> +};\n> +\n> +} /* namespace ipa::common::algorithms */\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> index ab8da13a..860dc199 100644\n> --- a/src/ipa/libipa/algorithms/meson.build\n> +++ b/src/ipa/libipa/algorithms/meson.build\n> @@ -2,8 +2,10 @@\n>\n> common_ipa_algorithms_headers = files([\n> 'af_algorithm.h',\n> + 'af_hill_climbing.h',\n> ])\n>\n> common_ipa_algorithms_sources = files([\n> 'af_algorithm.cpp',\n> + 'af_hill_climbing.cpp',\n> ])\n> --\n> 2.34.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3D80CBE173\n\tfor ;\n\tThu, 14 Jul 2022 16:17:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7B53763312;\n\tThu, 14 Jul 2022 18:17:27 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A80966330D\n\tfor ;\n\tThu, 14 Jul 2022 18:17:25 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 0E36520004;\n\tThu, 14 Jul 2022 16:17:24 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657815447;\n\tbh=Ks4kmQRZIiQy8r/uaCKQ5PkXuLVNwshI1r+3I7JceCQ=;\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=4Kvqmuf5QUOtxGsWycUbM7wrRTAXhVD+rP+ESL9d4YX+Ma1H4EfizVIAKRhgR/HIk\n\tPORqPwdE5uQHFg89XS1vf+I0p/9mENdvo42RCPnuXyYwtjWO1XFmAc3hmkAYR2VI18\n\t9lHs2TpGhgR5lpItJkeXYQdZKmhM8EKeNkveM6MxDEwssf06dJe5F6cJ4cqGAVoCpo\n\tu6/s3YfbS3m66nUL0Sqfp8WSBG9T//5pwXrHL1Kf2ahSIbnaqURcyWawRIyP2TGWrU\n\tLasaJf04zRWkSL49Vq7fg+CabOHRJ/UBSWNb/8pN16K/v/oQTm4buOhXlyLq0iFLxR\n\tPpbgYMXzURLCg==","Date":"Thu, 14 Jul 2022 18:17:23 +0200","To":"Daniel Semkowicz ","Message-ID":"<20220714161723.2w6p6c6odeapcsfq@uno.localdomain>","References":"<20220713084317.24268-1-dse@thaumatec.com>\n\t<20220713084317.24268-3-dse@thaumatec.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220713084317.24268-3-dse@thaumatec.com>","Subject":"Re: [libcamera-devel] [PATCH v2 02/11] ipa: Add class that\n\timplements base AF control algorithm","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":23884,"web_url":"https://patchwork.libcamera.org/comment/23884/","msgid":"<20220714163324.vjiqxhrkqkvuqil3@uno.localdomain>","date":"2022-07-14T16:33:24","subject":"Re: [libcamera-devel] [PATCH v2 02/11] ipa: Add class that\n\timplements base AF control algorithm","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Kate in cc for real this time!\n\nOn Thu, Jul 14, 2022 at 06:17:23PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> Hi Daniel\n>\n> On Wed, Jul 13, 2022 at 10:43:08AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > Move the code that was common for IPU3 and RPi AF algorithms to\n> > a separate class independent of platform specific code.\n> > This way each platform can just implement contrast calculation and\n> > run the AF control loop basing on this class.\n> >\n>\n> This is exactly the purpose of having algorithms in libipa. I'm\n> excited it's the first \"generic\" one we have, or that I know of at\n> least!\n>\n> This mean the very IPU3 implementation this algorithm is based on\n> (Kate in cc) can now be generalized, and the same for the RPi one we\n> have on the list!\n>\n> > Signed-off-by: Daniel Semkowicz \n> > ---\n> > .../libipa/algorithms/af_hill_climbing.cpp | 89 +++++++\n> > src/ipa/libipa/algorithms/af_hill_climbing.h | 251 ++++++++++++++++++\n> > src/ipa/libipa/algorithms/meson.build | 2 +\n> > 3 files changed, 342 insertions(+)\n> > create mode 100644 src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > create mode 100644 src/ipa/libipa/algorithms/af_hill_climbing.h\n> >\n> > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > new file mode 100644\n> > index 00000000..f666c6c2\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > @@ -0,0 +1,89 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Red Hat\n> > + * Copyright (C) 2022, Ideas On Board\n> > + * Copyright (C) 2022, Theobroma Systems\n> > + *\n> > + * af_hill_climbing.cpp - AF Hill Climbing common algorithm\n> > + */\n>\n> Any particular reason why the implementation is in the .h file ?\n>\n> > +\n> > +#include \"af_hill_climbing.h\"\n> > +\n> > +/**\n> > + * \\file af_hill_climbing.h\n> > + * \\brief AF Hill Climbing common algorithm\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(Af)\n> > +\n> > +namespace ipa::common::algorithms {\n> > +\n> > +/**\n> > + * \\class AfHillClimbing\n> > + * \\brief The base class implementing hill climbing AF control algorithm\n> > + * \\tparam Module The IPA module type for this class of algorithms\n> > + *\n> > + * Control part of auto focus algorithm. It calculates the lens position basing\n> > + * on contrast measure supplied by the higher level. This way it is independent\n> > + * from the platform.\n> > + *\n> > + * Derived class should call processAutofocus() for each measured contrast value\n> > + * and set the lens to the calculated position.\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfHillClimbing::setMode()\n> > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setMode\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfHillClimbing::setRange()\n> > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setRange\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfHillClimbing::setSpeed()\n> > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setSpeed\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfHillClimbing::setMetering()\n> > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setMetering\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfHillClimbing::setWindows()\n> > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setWindows\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfHillClimbing::setTrigger()\n> > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setTrigger\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfHillClimbing::setPause()\n> > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setPause\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfHillClimbing::setLensPosition()\n> > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setLensPosition\n> > + */\n> > +\n> > +/**\n> > + * \\fn AfHillClimbing::processAutofocus()\n> > + * \\brief Run the auto focus algorithm loop\n> > + * \\param[in] currentContrast New value of contrast measured for current frame\n> > + *\n> > + * This method should be called for each new contrast value that was measured,\n> > + * usually in the process() method.\n> > + *\n> > + * \\return New lens position calculated by AF algorithm\n> > + */\n> > +\n> > +} /* namespace ipa::common::algorithms */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > new file mode 100644\n> > index 00000000..db9fc058\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > @@ -0,0 +1,251 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Red Hat\n> > + * Copyright (C) 2022, Ideas On Board\n> > + * Copyright (C) 2022, Theobroma Systems\n> > + *\n> > + * af_hill_climbing.h - AF Hill Climbing common algorithm\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \"af_algorithm.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(Af)\n> > +\n> > +namespace ipa::common::algorithms {\n> > +\n> > +template\n> > +class AfHillClimbing : public AfAlgorithm\n> > +{\n> > +public:\n> > +\tAfHillClimbing()\n> > +\t\t: mode_(controls::AfModeAuto), state_(controls::AfStateIdle),\n> > +\t\t pauseState_(controls::AfPauseStateRunning),\n> > +\t\t lensPosition_(0), bestPosition_(0), currentContrast_(0.0),\n> > +\t\t previousContrast_(0.0), maxContrast_(0.0), maxStep_(0),\n> > +\t\t coarseCompleted_(false), fineCompleted_(false),\n> > +\t\t lowStep_(0), highStep_(kMaxFocusSteps)\n> > +\t{\n> > +\t}\n>\n> Let's look at the implementation details later but I have one question\n>\n> > +\n> > +\tvirtual ~AfHillClimbing() {}\n> > +\n> > +\tvoid setMode(controls::AfModeEnum mode) final\n> > +\t{\n> > +\t\tif (mode != mode_) {\n> > +\t\t\tLOG(Af, Debug) << \"Switched AF mode from \" << mode_\n> > +\t\t\t\t << \" to \" << mode;\n> > +\t\t\tpauseState_ = libcamera::controls::AfPauseStateRunning;\n> > +\t\t\tmode_ = mode;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tvoid setRange([[maybe_unused]] controls::AfRangeEnum range) final\n> > +\t{\n> > +\t\tLOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > +\t}\n> > +\n> > +\tvoid setSpeed([[maybe_unused]] controls::AfSpeedEnum speed) final\n> > +\t{\n> > +\t\tLOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > +\t}\n> > +\n> > +\tvoid setTrigger(controls::AfTriggerEnum trigger) final\n> > +\t{\n> > +\t\tLOG(Af, Debug) << \"Trigger called in mode \" << mode_\n> > +\t\t\t << \" with \" << trigger;\n> > +\t\tif (mode_ == libcamera::controls::AfModeAuto) {\n> > +\t\t\tif (trigger == libcamera::controls::AfTriggerStart)\n> > +\t\t\t\tafReset();\n>\n> This seems out of place..\n> Whenever I trigger an autofocus scan I get the lens reset to 0 causing\n> a not very nice effect. Why are you resetting the lens position here?\n>\n> > +\t\t\telse\n> > +\t\t\t\tstate_ = libcamera::controls::AfStateIdle;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tvoid setPause(controls::AfPauseEnum pause) final\n> > +\t{\n> > +\t\t/* \\todo: add the AfPauseDeferred mode */\n> > +\t\tif (mode_ == libcamera::controls::AfModeContinuous) {\n> > +\t\t\tif (pause == libcamera::controls::AfPauseImmediate)\n> > +\t\t\t\tpauseState_ = libcamera::controls::AfPauseStatePaused;\n> > +\t\t\telse if (pause == libcamera::controls::AfPauseResume)\n> > +\t\t\t\tpauseState_ = libcamera::controls::AfPauseStateRunning;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tvoid setLensPosition([[maybe_unused]] float lensPosition) final\n> > +\t{\n> > +\t\tLOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > +\t}\n> > +\n> > +\t/* These methods should be implemented by derived class */\n> > +\tvirtual void setMetering(controls::AfMeteringEnum metering) = 0;\n> > +\tvirtual void setWindows(Span windows) = 0;\n> > +\n> > +protected:\n> > +\tuint32_t processAutofocus(double currentContrast)\n> > +\t{\n> > +\t\tcurrentContrast_ = currentContrast;\n> > +\n> > +\t\t/* If we are in a paused state, we won't process the stats */\n> > +\t\tif (pauseState_ == libcamera::controls::AfPauseStatePaused)\n> > +\t\t\treturn lensPosition_;\n> > +\n> > +\t\t/* Depending on the mode, we may or may not process the stats */\n> > +\t\tif (state_ == libcamera::controls::AfStateIdle)\n> > +\t\t\treturn lensPosition_;\n> > +\n> > +\t\tif (state_ != libcamera::controls::AfStateFocused) {\n> > +\t\t\tafCoarseScan();\n> > +\t\t\tafFineScan();\n> > +\t\t} else {\n> > +\t\t\t/* We can re-start the scan at any moment in AfModeContinuous */\n> > +\t\t\tif (mode_ == libcamera::controls::AfModeContinuous)\n> > +\t\t\t\tif (afIsOutOfFocus())\n> > +\t\t\t\t\tafReset();\n> > +\t\t}\n> > +\n> > +\t\treturn lensPosition_;\n>\n> Let me recap one point. We are still missing a generic definition for\n> the lens position value. As you can see we use\n> V4L2_CID_FOCUS_ABSOLUTE which transports the actual value to be set in\n> the v4l2 control. This can't work here and we are planning to remove\n> V4L2 controls from the IPA in general, but while for other controls we\n> have defined generic ranges, for the lens position we are still\n> missing a way to compute a \"generic\" value in the IPA, send it to the\n> pipeline handler by using libcamera::controls::LensPosition and have\n> the CameraLens class translate it to the device-specific value.\n>\n> I'm a bit lazy and I'm not chasing how lensPosition_ is here computed,\n> but have you considered how this could be generalized already ? As an\n> example, it seems to me the values I get are in the 0-180 range, while\n> in example the lens I'm using ranges from 0-4096...\n>\n> > +\t}\n> > +\n> > +private:\n> > +\tvoid afCoarseScan()\n> > +\t{\n> > +\t\tif (coarseCompleted_)\n> > +\t\t\treturn;\n> > +\n> > +\t\tif (afScan(kCoarseSearchStep)) {\n> > +\t\t\tcoarseCompleted_ = true;\n> > +\t\t\tmaxContrast_ = 0;\n> > +\t\t\tlensPosition_ = lensPosition_ - (lensPosition_ * kFineRange);\n> > +\t\t\tpreviousContrast_ = 0;\n> > +\t\t\tmaxStep_ = std::clamp(lensPosition_ + static_cast((lensPosition_ * kFineRange)),\n> > +\t\t\t\t\t 0U, highStep_);\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tvoid afFineScan()\n> > +\t{\n> > +\t\tif (!coarseCompleted_)\n> > +\t\t\treturn;\n> > +\n> > +\t\tif (afScan(kFineSearchStep)) {\n> > +\t\t\tLOG(Af, Debug) << \"AF found the best focus position !\";\n> > +\t\t\tstate_ = libcamera::controls::AfStateFocused;\n> > +\t\t\tfineCompleted_ = true;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tbool afScan(uint32_t minSteps)\n> > +\t{\n> > +\t\tif (lensPosition_ + minSteps > maxStep_) {\n> > +\t\t\t/* If the max step is reached, move lens to the position. */\n> > +\t\t\tlensPosition_ = bestPosition_;\n> > +\t\t\treturn true;\n> > +\t\t} else {\n> > +\t\t\t/*\n> > +\t\t\t* Find the maximum of the variance by estimating its\n> > +\t\t\t* derivative. If the direction changes, it means we have passed\n> > +\t\t\t* a maximum one step before.\n> > +\t\t\t*/\n> > +\t\t\tif ((currentContrast_ - maxContrast_) >= -(maxContrast_ * 0.1)) {\n> > +\t\t\t\t/*\n> > +\t\t\t\t* Positive and zero derivative:\n> > +\t\t\t\t* The variance is still increasing. The focus could be\n> > +\t\t\t\t* increased for the next comparison. Also, the max\n> > +\t\t\t\t* variance and previous focus value are updated.\n> > +\t\t\t\t*/\n> > +\t\t\t\tbestPosition_ = lensPosition_;\n> > +\t\t\t\tlensPosition_ += minSteps;\n> > +\t\t\t\tmaxContrast_ = currentContrast_;\n> > +\t\t\t} else {\n> > +\t\t\t\t/*\n> > +\t\t\t\t* Negative derivative:\n> > +\t\t\t\t* The variance starts to decrease which means the maximum\n> > +\t\t\t\t* variance is found. Set focus step to previous good one\n> > +\t\t\t\t* then return immediately.\n> > +\t\t\t\t*/\n> > +\t\t\t\tlensPosition_ = bestPosition_;\n> > +\t\t\t\treturn true;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\tpreviousContrast_ = currentContrast_;\n> > +\t\tLOG(Af, Debug) << \" Previous step is \" << bestPosition_\n> > +\t\t\t << \" Current step is \" << lensPosition_;\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\tvoid afReset()\n> > +\t{\n> > +\t\tLOG(Af, Debug) << \"Reset AF parameters\";\n> > +\t\tlensPosition_ = lowStep_;\n> > +\t\tmaxStep_ = highStep_;\n> > +\t\tstate_ = libcamera::controls::AfStateScanning;\n> > +\t\tpreviousContrast_ = 0.0;\n> > +\t\tcoarseCompleted_ = false;\n> > +\t\tfineCompleted_ = false;\n> > +\t\tmaxContrast_ = 0.0;\n> > +\t}\n> > +\n> > +\tbool afIsOutOfFocus()\n> > +\t{\n> > +\t\tconst uint32_t diff_var = std::abs(currentContrast_ -\n> > +\t\t\t\t\t\t maxContrast_);\n> > +\t\tconst double var_ratio = diff_var / maxContrast_;\n> > +\t\tLOG(Af, Debug) << \"Variance change rate: \" << var_ratio\n> > +\t\t\t << \" Current VCM step: \" << lensPosition_;\n> > +\t\tif (var_ratio > kMaxChange)\n> > +\t\t\treturn true;\n> > +\t\telse\n> > +\t\t\treturn false;\n> > +\t}\n> > +\n> > +\tcontrols::AfModeEnum mode_;\n> > +\tcontrols::AfStateEnum state_;\n> > +\tcontrols::AfPauseStateEnum pauseState_;\n> > +\n> > +\t/* VCM step configuration. It is the current setting of the VCM step. */\n> > +\tuint32_t lensPosition_;\n> > +\t/* The best VCM step. It is a local optimum VCM step during scanning. */\n> > +\tuint32_t bestPosition_;\n> > +\n> > +\t/* Current AF statistic contrast. */\n> > +\tdouble currentContrast_;\n> > +\t/* It is used to determine the derivative during scanning */\n> > +\tdouble previousContrast_;\n> > +\tdouble maxContrast_;\n> > +\t/* The designated maximum range of focus scanning. */\n> > +\tuint32_t maxStep_;\n> > +\t/* If the coarse scan completes, it is set to true. */\n> > +\tbool coarseCompleted_;\n> > +\t/* If the fine scan completes, it is set to true. */\n> > +\tbool fineCompleted_;\n> > +\n> > +\tuint32_t lowStep_;\n> > +\tuint32_t highStep_;\n> > +\n> > +\t/*\n> > +\t* Maximum focus steps of the VCM control\n> > +\t* \\todo should be obtained from the VCM driver\n> > +\t*/\n> > +\tstatic constexpr uint32_t kMaxFocusSteps = 1023;\n> > +\n> > +\t/* Minimum focus step for searching appropriate focus */\n> > +\tstatic constexpr uint32_t kCoarseSearchStep = 30;\n> > +\tstatic constexpr uint32_t kFineSearchStep = 1;\n> > +\n> > +\t/* Max ratio of variance change, 0.0 < kMaxChange < 1.0 */\n> > +\tstatic constexpr double kMaxChange = 0.5;\n> > +\n> > +\t/* Fine scan range 0 < kFineRange < 1 */\n> > +\tstatic constexpr double kFineRange = 0.05;\n> > +};\n> > +\n> > +} /* namespace ipa::common::algorithms */\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> > index ab8da13a..860dc199 100644\n> > --- a/src/ipa/libipa/algorithms/meson.build\n> > +++ b/src/ipa/libipa/algorithms/meson.build\n> > @@ -2,8 +2,10 @@\n> >\n> > common_ipa_algorithms_headers = files([\n> > 'af_algorithm.h',\n> > + 'af_hill_climbing.h',\n> > ])\n> >\n> > common_ipa_algorithms_sources = files([\n> > 'af_algorithm.cpp',\n> > + 'af_hill_climbing.cpp',\n> > ])\n> > --\n> > 2.34.1\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2D80EBD1F1\n\tfor ;\n\tThu, 14 Jul 2022 16:33:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6AD7563312;\n\tThu, 14 Jul 2022 18:33:29 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 164456330D\n\tfor ;\n\tThu, 14 Jul 2022 18:33:28 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 7305110000A;\n\tThu, 14 Jul 2022 16:33:26 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657816409;\n\tbh=0Mshn6sXY259V00m9T0LjqOHw7sq5SFKGVCpLxW7Fsc=;\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:\n\tFrom;\n\tb=VoAG2Fkumo4f35fLYVb30+sdly627a0UIiNUf0xeftD2kmPUHjl6MWqlyjwmHYiZa\n\t4cW666Oz8fnkVL3gbc5ZbY8hzwHcKf2sIXO8Nvt+w2D4D+jVd0y0+rxHl7K73RByg2\n\tSgITZDmjiWiOvbObbqvbS/KoSyG/ngFHfszHKnPSz5zFuP17fHmV5Z61pOYOtqPnWa\n\t0xE7X/KEzpmhvEFqfJ4LqlG2ffJ5f7bP1X8VGV88tGhxR2QFnSAxBmpgn8Gn98WWD5\n\tCLI1GPz8HU85+CiWvrsPXNDO6KsnwKtqWXYyK9FKaLttsZkqK1gTyAU1SW4rtLTT4z\n\t9JJCU0w9FyicA==","Date":"Thu, 14 Jul 2022 18:33:24 +0200","To":"Daniel Semkowicz , libcamera-devel@lists.libcamera.org","Message-ID":"<20220714163324.vjiqxhrkqkvuqil3@uno.localdomain>","References":"<20220713084317.24268-1-dse@thaumatec.com>\n\t<20220713084317.24268-3-dse@thaumatec.com>\n\t<20220714161723.2w6p6c6odeapcsfq@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220714161723.2w6p6c6odeapcsfq@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 02/11] ipa: Add class that\n\timplements base AF control algorithm","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23906,"web_url":"https://patchwork.libcamera.org/comment/23906/","msgid":"<20220715065232.ycxow3pou7nzvasy@uno.localdomain>","date":"2022-07-15T06:52:32","subject":"Re: [libcamera-devel] [PATCH v2 02/11] ipa: Add class that\n\timplements base AF control algorithm","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"One day I'll lear to use email\n\nOn Thu, Jul 14, 2022 at 06:33:24PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> Kate in cc for real this time!\n\nfor real\n\n>\n> On Thu, Jul 14, 2022 at 06:17:23PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > Hi Daniel\n> >\n> > On Wed, Jul 13, 2022 at 10:43:08AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > Move the code that was common for IPU3 and RPi AF algorithms to\n> > > a separate class independent of platform specific code.\n> > > This way each platform can just implement contrast calculation and\n> > > run the AF control loop basing on this class.\n> > >\n> >\n> > This is exactly the purpose of having algorithms in libipa. I'm\n> > excited it's the first \"generic\" one we have, or that I know of at\n> > least!\n> >\n> > This mean the very IPU3 implementation this algorithm is based on\n> > (Kate in cc) can now be generalized, and the same for the RPi one we\n> > have on the list!\n> >\n> > > Signed-off-by: Daniel Semkowicz \n> > > ---\n> > > .../libipa/algorithms/af_hill_climbing.cpp | 89 +++++++\n> > > src/ipa/libipa/algorithms/af_hill_climbing.h | 251 ++++++++++++++++++\n> > > src/ipa/libipa/algorithms/meson.build | 2 +\n> > > 3 files changed, 342 insertions(+)\n> > > create mode 100644 src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > create mode 100644 src/ipa/libipa/algorithms/af_hill_climbing.h\n> > >\n> > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > new file mode 100644\n> > > index 00000000..f666c6c2\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > @@ -0,0 +1,89 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Red Hat\n> > > + * Copyright (C) 2022, Ideas On Board\n> > > + * Copyright (C) 2022, Theobroma Systems\n> > > + *\n> > > + * af_hill_climbing.cpp - AF Hill Climbing common algorithm\n> > > + */\n> >\n> > Any particular reason why the implementation is in the .h file ?\n> >\n> > > +\n> > > +#include \"af_hill_climbing.h\"\n> > > +\n> > > +/**\n> > > + * \\file af_hill_climbing.h\n> > > + * \\brief AF Hill Climbing common algorithm\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DEFINE_CATEGORY(Af)\n> > > +\n> > > +namespace ipa::common::algorithms {\n> > > +\n> > > +/**\n> > > + * \\class AfHillClimbing\n> > > + * \\brief The base class implementing hill climbing AF control algorithm\n> > > + * \\tparam Module The IPA module type for this class of algorithms\n> > > + *\n> > > + * Control part of auto focus algorithm. It calculates the lens position basing\n> > > + * on contrast measure supplied by the higher level. This way it is independent\n> > > + * from the platform.\n> > > + *\n> > > + * Derived class should call processAutofocus() for each measured contrast value\n> > > + * and set the lens to the calculated position.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn AfHillClimbing::setMode()\n> > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setMode\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn AfHillClimbing::setRange()\n> > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setRange\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn AfHillClimbing::setSpeed()\n> > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setSpeed\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn AfHillClimbing::setMetering()\n> > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setMetering\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn AfHillClimbing::setWindows()\n> > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setWindows\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn AfHillClimbing::setTrigger()\n> > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setTrigger\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn AfHillClimbing::setPause()\n> > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setPause\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn AfHillClimbing::setLensPosition()\n> > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setLensPosition\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn AfHillClimbing::processAutofocus()\n> > > + * \\brief Run the auto focus algorithm loop\n> > > + * \\param[in] currentContrast New value of contrast measured for current frame\n> > > + *\n> > > + * This method should be called for each new contrast value that was measured,\n> > > + * usually in the process() method.\n> > > + *\n> > > + * \\return New lens position calculated by AF algorithm\n> > > + */\n> > > +\n> > > +} /* namespace ipa::common::algorithms */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > new file mode 100644\n> > > index 00000000..db9fc058\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > @@ -0,0 +1,251 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Red Hat\n> > > + * Copyright (C) 2022, Ideas On Board\n> > > + * Copyright (C) 2022, Theobroma Systems\n> > > + *\n> > > + * af_hill_climbing.h - AF Hill Climbing common algorithm\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +\n> > > +#include \"af_algorithm.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DECLARE_CATEGORY(Af)\n> > > +\n> > > +namespace ipa::common::algorithms {\n> > > +\n> > > +template\n> > > +class AfHillClimbing : public AfAlgorithm\n> > > +{\n> > > +public:\n> > > +\tAfHillClimbing()\n> > > +\t\t: mode_(controls::AfModeAuto), state_(controls::AfStateIdle),\n> > > +\t\t pauseState_(controls::AfPauseStateRunning),\n> > > +\t\t lensPosition_(0), bestPosition_(0), currentContrast_(0.0),\n> > > +\t\t previousContrast_(0.0), maxContrast_(0.0), maxStep_(0),\n> > > +\t\t coarseCompleted_(false), fineCompleted_(false),\n> > > +\t\t lowStep_(0), highStep_(kMaxFocusSteps)\n> > > +\t{\n> > > +\t}\n> >\n> > Let's look at the implementation details later but I have one question\n> >\n> > > +\n> > > +\tvirtual ~AfHillClimbing() {}\n> > > +\n> > > +\tvoid setMode(controls::AfModeEnum mode) final\n> > > +\t{\n> > > +\t\tif (mode != mode_) {\n> > > +\t\t\tLOG(Af, Debug) << \"Switched AF mode from \" << mode_\n> > > +\t\t\t\t << \" to \" << mode;\n> > > +\t\t\tpauseState_ = libcamera::controls::AfPauseStateRunning;\n> > > +\t\t\tmode_ = mode;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\tvoid setRange([[maybe_unused]] controls::AfRangeEnum range) final\n> > > +\t{\n> > > +\t\tLOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > +\t}\n> > > +\n> > > +\tvoid setSpeed([[maybe_unused]] controls::AfSpeedEnum speed) final\n> > > +\t{\n> > > +\t\tLOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > +\t}\n> > > +\n> > > +\tvoid setTrigger(controls::AfTriggerEnum trigger) final\n> > > +\t{\n> > > +\t\tLOG(Af, Debug) << \"Trigger called in mode \" << mode_\n> > > +\t\t\t << \" with \" << trigger;\n> > > +\t\tif (mode_ == libcamera::controls::AfModeAuto) {\n> > > +\t\t\tif (trigger == libcamera::controls::AfTriggerStart)\n> > > +\t\t\t\tafReset();\n> >\n> > This seems out of place..\n> > Whenever I trigger an autofocus scan I get the lens reset to 0 causing\n> > a not very nice effect. Why are you resetting the lens position here?\n> >\n> > > +\t\t\telse\n> > > +\t\t\t\tstate_ = libcamera::controls::AfStateIdle;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\tvoid setPause(controls::AfPauseEnum pause) final\n> > > +\t{\n> > > +\t\t/* \\todo: add the AfPauseDeferred mode */\n> > > +\t\tif (mode_ == libcamera::controls::AfModeContinuous) {\n> > > +\t\t\tif (pause == libcamera::controls::AfPauseImmediate)\n> > > +\t\t\t\tpauseState_ = libcamera::controls::AfPauseStatePaused;\n> > > +\t\t\telse if (pause == libcamera::controls::AfPauseResume)\n> > > +\t\t\t\tpauseState_ = libcamera::controls::AfPauseStateRunning;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\tvoid setLensPosition([[maybe_unused]] float lensPosition) final\n> > > +\t{\n> > > +\t\tLOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > +\t}\n> > > +\n> > > +\t/* These methods should be implemented by derived class */\n> > > +\tvirtual void setMetering(controls::AfMeteringEnum metering) = 0;\n> > > +\tvirtual void setWindows(Span windows) = 0;\n> > > +\n> > > +protected:\n> > > +\tuint32_t processAutofocus(double currentContrast)\n> > > +\t{\n> > > +\t\tcurrentContrast_ = currentContrast;\n> > > +\n> > > +\t\t/* If we are in a paused state, we won't process the stats */\n> > > +\t\tif (pauseState_ == libcamera::controls::AfPauseStatePaused)\n> > > +\t\t\treturn lensPosition_;\n> > > +\n> > > +\t\t/* Depending on the mode, we may or may not process the stats */\n> > > +\t\tif (state_ == libcamera::controls::AfStateIdle)\n> > > +\t\t\treturn lensPosition_;\n> > > +\n> > > +\t\tif (state_ != libcamera::controls::AfStateFocused) {\n> > > +\t\t\tafCoarseScan();\n> > > +\t\t\tafFineScan();\n> > > +\t\t} else {\n> > > +\t\t\t/* We can re-start the scan at any moment in AfModeContinuous */\n> > > +\t\t\tif (mode_ == libcamera::controls::AfModeContinuous)\n> > > +\t\t\t\tif (afIsOutOfFocus())\n> > > +\t\t\t\t\tafReset();\n> > > +\t\t}\n> > > +\n> > > +\t\treturn lensPosition_;\n> >\n> > Let me recap one point. We are still missing a generic definition for\n> > the lens position value. As you can see we use\n> > V4L2_CID_FOCUS_ABSOLUTE which transports the actual value to be set in\n> > the v4l2 control. This can't work here and we are planning to remove\n> > V4L2 controls from the IPA in general, but while for other controls we\n> > have defined generic ranges, for the lens position we are still\n> > missing a way to compute a \"generic\" value in the IPA, send it to the\n> > pipeline handler by using libcamera::controls::LensPosition and have\n> > the CameraLens class translate it to the device-specific value.\n> >\n> > I'm a bit lazy and I'm not chasing how lensPosition_ is here computed,\n> > but have you considered how this could be generalized already ? As an\n> > example, it seems to me the values I get are in the 0-180 range, while\n> > in example the lens I'm using ranges from 0-4096...\n> >\n> > > +\t}\n> > > +\n> > > +private:\n> > > +\tvoid afCoarseScan()\n> > > +\t{\n> > > +\t\tif (coarseCompleted_)\n> > > +\t\t\treturn;\n> > > +\n> > > +\t\tif (afScan(kCoarseSearchStep)) {\n> > > +\t\t\tcoarseCompleted_ = true;\n> > > +\t\t\tmaxContrast_ = 0;\n> > > +\t\t\tlensPosition_ = lensPosition_ - (lensPosition_ * kFineRange);\n> > > +\t\t\tpreviousContrast_ = 0;\n> > > +\t\t\tmaxStep_ = std::clamp(lensPosition_ + static_cast((lensPosition_ * kFineRange)),\n> > > +\t\t\t\t\t 0U, highStep_);\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\tvoid afFineScan()\n> > > +\t{\n> > > +\t\tif (!coarseCompleted_)\n> > > +\t\t\treturn;\n> > > +\n> > > +\t\tif (afScan(kFineSearchStep)) {\n> > > +\t\t\tLOG(Af, Debug) << \"AF found the best focus position !\";\n> > > +\t\t\tstate_ = libcamera::controls::AfStateFocused;\n> > > +\t\t\tfineCompleted_ = true;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\tbool afScan(uint32_t minSteps)\n> > > +\t{\n> > > +\t\tif (lensPosition_ + minSteps > maxStep_) {\n> > > +\t\t\t/* If the max step is reached, move lens to the position. */\n> > > +\t\t\tlensPosition_ = bestPosition_;\n> > > +\t\t\treturn true;\n> > > +\t\t} else {\n> > > +\t\t\t/*\n> > > +\t\t\t* Find the maximum of the variance by estimating its\n> > > +\t\t\t* derivative. If the direction changes, it means we have passed\n> > > +\t\t\t* a maximum one step before.\n> > > +\t\t\t*/\n> > > +\t\t\tif ((currentContrast_ - maxContrast_) >= -(maxContrast_ * 0.1)) {\n> > > +\t\t\t\t/*\n> > > +\t\t\t\t* Positive and zero derivative:\n> > > +\t\t\t\t* The variance is still increasing. The focus could be\n> > > +\t\t\t\t* increased for the next comparison. Also, the max\n> > > +\t\t\t\t* variance and previous focus value are updated.\n> > > +\t\t\t\t*/\n> > > +\t\t\t\tbestPosition_ = lensPosition_;\n> > > +\t\t\t\tlensPosition_ += minSteps;\n> > > +\t\t\t\tmaxContrast_ = currentContrast_;\n> > > +\t\t\t} else {\n> > > +\t\t\t\t/*\n> > > +\t\t\t\t* Negative derivative:\n> > > +\t\t\t\t* The variance starts to decrease which means the maximum\n> > > +\t\t\t\t* variance is found. Set focus step to previous good one\n> > > +\t\t\t\t* then return immediately.\n> > > +\t\t\t\t*/\n> > > +\t\t\t\tlensPosition_ = bestPosition_;\n> > > +\t\t\t\treturn true;\n> > > +\t\t\t}\n> > > +\t\t}\n> > > +\n> > > +\t\tpreviousContrast_ = currentContrast_;\n> > > +\t\tLOG(Af, Debug) << \" Previous step is \" << bestPosition_\n> > > +\t\t\t << \" Current step is \" << lensPosition_;\n> > > +\t\treturn false;\n> > > +\t}\n> > > +\n> > > +\tvoid afReset()\n> > > +\t{\n> > > +\t\tLOG(Af, Debug) << \"Reset AF parameters\";\n> > > +\t\tlensPosition_ = lowStep_;\n> > > +\t\tmaxStep_ = highStep_;\n> > > +\t\tstate_ = libcamera::controls::AfStateScanning;\n> > > +\t\tpreviousContrast_ = 0.0;\n> > > +\t\tcoarseCompleted_ = false;\n> > > +\t\tfineCompleted_ = false;\n> > > +\t\tmaxContrast_ = 0.0;\n> > > +\t}\n> > > +\n> > > +\tbool afIsOutOfFocus()\n> > > +\t{\n> > > +\t\tconst uint32_t diff_var = std::abs(currentContrast_ -\n> > > +\t\t\t\t\t\t maxContrast_);\n> > > +\t\tconst double var_ratio = diff_var / maxContrast_;\n> > > +\t\tLOG(Af, Debug) << \"Variance change rate: \" << var_ratio\n> > > +\t\t\t << \" Current VCM step: \" << lensPosition_;\n> > > +\t\tif (var_ratio > kMaxChange)\n> > > +\t\t\treturn true;\n> > > +\t\telse\n> > > +\t\t\treturn false;\n> > > +\t}\n> > > +\n> > > +\tcontrols::AfModeEnum mode_;\n> > > +\tcontrols::AfStateEnum state_;\n> > > +\tcontrols::AfPauseStateEnum pauseState_;\n> > > +\n> > > +\t/* VCM step configuration. It is the current setting of the VCM step. */\n> > > +\tuint32_t lensPosition_;\n> > > +\t/* The best VCM step. It is a local optimum VCM step during scanning. */\n> > > +\tuint32_t bestPosition_;\n> > > +\n> > > +\t/* Current AF statistic contrast. */\n> > > +\tdouble currentContrast_;\n> > > +\t/* It is used to determine the derivative during scanning */\n> > > +\tdouble previousContrast_;\n> > > +\tdouble maxContrast_;\n> > > +\t/* The designated maximum range of focus scanning. */\n> > > +\tuint32_t maxStep_;\n> > > +\t/* If the coarse scan completes, it is set to true. */\n> > > +\tbool coarseCompleted_;\n> > > +\t/* If the fine scan completes, it is set to true. */\n> > > +\tbool fineCompleted_;\n> > > +\n> > > +\tuint32_t lowStep_;\n> > > +\tuint32_t highStep_;\n> > > +\n> > > +\t/*\n> > > +\t* Maximum focus steps of the VCM control\n> > > +\t* \\todo should be obtained from the VCM driver\n> > > +\t*/\n> > > +\tstatic constexpr uint32_t kMaxFocusSteps = 1023;\n> > > +\n> > > +\t/* Minimum focus step for searching appropriate focus */\n> > > +\tstatic constexpr uint32_t kCoarseSearchStep = 30;\n> > > +\tstatic constexpr uint32_t kFineSearchStep = 1;\n> > > +\n> > > +\t/* Max ratio of variance change, 0.0 < kMaxChange < 1.0 */\n> > > +\tstatic constexpr double kMaxChange = 0.5;\n> > > +\n> > > +\t/* Fine scan range 0 < kFineRange < 1 */\n> > > +\tstatic constexpr double kFineRange = 0.05;\n> > > +};\n> > > +\n> > > +} /* namespace ipa::common::algorithms */\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> > > index ab8da13a..860dc199 100644\n> > > --- a/src/ipa/libipa/algorithms/meson.build\n> > > +++ b/src/ipa/libipa/algorithms/meson.build\n> > > @@ -2,8 +2,10 @@\n> > >\n> > > common_ipa_algorithms_headers = files([\n> > > 'af_algorithm.h',\n> > > + 'af_hill_climbing.h',\n> > > ])\n> > >\n> > > common_ipa_algorithms_sources = files([\n> > > 'af_algorithm.cpp',\n> > > + 'af_hill_climbing.cpp',\n> > > ])\n> > > --\n> > > 2.34.1\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DA30FBE173\n\tfor ;\n\tFri, 15 Jul 2022 06:52:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4DADA63312;\n\tFri, 15 Jul 2022 08:52:36 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::223])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 41ACD60489\n\tfor ;\n\tFri, 15 Jul 2022 08:52:35 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 2946660003;\n\tFri, 15 Jul 2022 06:52:33 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657867956;\n\tbh=/qOW3v3HCMnZIcHWqadMOdB5PHP98uqnzqtvexPdOyA=;\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:\n\tFrom;\n\tb=G4daUfQawe9rANb1Bf6EbNKAgt6MH0mVxrMhM8D5pJP8XBAioCTXaEoEfvoSllR//\n\tV3TAcERdY6RV9Wpbza2L1SvAGqcdm36Zo4GS2uPqoBhShr9l7kP95qG3OCJB0KooRO\n\tYdqdNU4YbKCyPc7wfRejjbLGP+uRsXPhHAnKNMZD9SlYW78DomTtdP1TAEmfDt00UH\n\tC1/7AWFtZT9ATVoLuQMGIsGl0rgn+InuMF95T41ufpYhFAp+74rPtUWIEhv5/vEGeN\n\t0dr/cWV8txpt8P/Vs7SWq51B1Y5QuAZa/Bo0bHiTnjeH8BmCHtxhYVIiuq9IxEU8oF\n\tXzT7DgorFEgyA==","Date":"Fri, 15 Jul 2022 08:52:32 +0200","To":"Daniel Semkowicz , libcamera-devel@lists.libcamera.org","Message-ID":"<20220715065232.ycxow3pou7nzvasy@uno.localdomain>","References":"<20220713084317.24268-1-dse@thaumatec.com>\n\t<20220713084317.24268-3-dse@thaumatec.com>\n\t<20220714161723.2w6p6c6odeapcsfq@uno.localdomain>\n\t<20220714163324.vjiqxhrkqkvuqil3@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220714163324.vjiqxhrkqkvuqil3@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 02/11] ipa: Add class that\n\timplements base AF control algorithm","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23952,"web_url":"https://patchwork.libcamera.org/comment/23952/","msgid":"","date":"2022-07-18T15:28:17","subject":"Re: [libcamera-devel] [PATCH v2 02/11] ipa: Add class that\n\timplements base AF control algorithm","submitter":{"id":126,"url":"https://patchwork.libcamera.org/api/people/126/","name":"Daniel Semkowicz","email":"dse@thaumatec.com"},"content":"Hi Jacopo,\n\nOn Fri, Jul 15, 2022 at 8:52 AM Jacopo Mondi wrote:\n>\n> One day I'll lear to use email\n>\n> On Thu, Jul 14, 2022 at 06:33:24PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > Kate in cc for real this time!\n>\n> for real\n>\n> >\n> > On Thu, Jul 14, 2022 at 06:17:23PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > Hi Daniel\n> > >\n> > > On Wed, Jul 13, 2022 at 10:43:08AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > Move the code that was common for IPU3 and RPi AF algorithms to\n> > > > a separate class independent of platform specific code.\n> > > > This way each platform can just implement contrast calculation and\n> > > > run the AF control loop basing on this class.\n> > > >\n> > >\n> > > This is exactly the purpose of having algorithms in libipa. I'm\n> > > excited it's the first \"generic\" one we have, or that I know of at\n> > > least!\n> > >\n> > > This mean the very IPU3 implementation this algorithm is based on\n> > > (Kate in cc) can now be generalized, and the same for the RPi one we\n> > > have on the list!\n> > >\n> > > > Signed-off-by: Daniel Semkowicz \n> > > > ---\n> > > > .../libipa/algorithms/af_hill_climbing.cpp | 89 +++++++\n> > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 251 ++++++++++++++++++\n> > > > src/ipa/libipa/algorithms/meson.build | 2 +\n> > > > 3 files changed, 342 insertions(+)\n> > > > create mode 100644 src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > create mode 100644 src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > >\n> > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > new file mode 100644\n> > > > index 00000000..f666c6c2\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > @@ -0,0 +1,89 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Red Hat\n> > > > + * Copyright (C) 2022, Ideas On Board\n> > > > + * Copyright (C) 2022, Theobroma Systems\n> > > > + *\n> > > > + * af_hill_climbing.cpp - AF Hill Climbing common algorithm\n> > > > + */\n> > >\n> > > Any particular reason why the implementation is in the .h file ?\n\nIn my original approach I had to pass the Module template argument, so\nusing the template, forced me to do the implementation in a header file.\n\nThis is an additional point in favor of the approach with multiple\ninheritence proposed by you in 01, as We can hide the implementation of\nthe AfHillClimbing in .cpp file.\n\n> > >\n> > > > +\n> > > > +#include \"af_hill_climbing.h\"\n> > > > +\n> > > > +/**\n> > > > + * \\file af_hill_climbing.h\n> > > > + * \\brief AF Hill Climbing common algorithm\n> > > > + */\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +LOG_DEFINE_CATEGORY(Af)\n> > > > +\n> > > > +namespace ipa::common::algorithms {\n> > > > +\n> > > > +/**\n> > > > + * \\class AfHillClimbing\n> > > > + * \\brief The base class implementing hill climbing AF control algorithm\n> > > > + * \\tparam Module The IPA module type for this class of algorithms\n> > > > + *\n> > > > + * Control part of auto focus algorithm. It calculates the lens position basing\n> > > > + * on contrast measure supplied by the higher level. This way it is independent\n> > > > + * from the platform.\n> > > > + *\n> > > > + * Derived class should call processAutofocus() for each measured contrast value\n> > > > + * and set the lens to the calculated position.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn AfHillClimbing::setMode()\n> > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setMode\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn AfHillClimbing::setRange()\n> > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setRange\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn AfHillClimbing::setSpeed()\n> > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setSpeed\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn AfHillClimbing::setMetering()\n> > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setMetering\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn AfHillClimbing::setWindows()\n> > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setWindows\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn AfHillClimbing::setTrigger()\n> > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setTrigger\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn AfHillClimbing::setPause()\n> > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setPause\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn AfHillClimbing::setLensPosition()\n> > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setLensPosition\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn AfHillClimbing::processAutofocus()\n> > > > + * \\brief Run the auto focus algorithm loop\n> > > > + * \\param[in] currentContrast New value of contrast measured for current frame\n> > > > + *\n> > > > + * This method should be called for each new contrast value that was measured,\n> > > > + * usually in the process() method.\n> > > > + *\n> > > > + * \\return New lens position calculated by AF algorithm\n> > > > + */\n> > > > +\n> > > > +} /* namespace ipa::common::algorithms */\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > new file mode 100644\n> > > > index 00000000..db9fc058\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > @@ -0,0 +1,251 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Red Hat\n> > > > + * Copyright (C) 2022, Ideas On Board\n> > > > + * Copyright (C) 2022, Theobroma Systems\n> > > > + *\n> > > > + * af_hill_climbing.h - AF Hill Climbing common algorithm\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"af_algorithm.h\"\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +LOG_DECLARE_CATEGORY(Af)\n> > > > +\n> > > > +namespace ipa::common::algorithms {\n> > > > +\n> > > > +template\n> > > > +class AfHillClimbing : public AfAlgorithm\n> > > > +{\n> > > > +public:\n> > > > + AfHillClimbing()\n> > > > + : mode_(controls::AfModeAuto), state_(controls::AfStateIdle),\n> > > > + pauseState_(controls::AfPauseStateRunning),\n> > > > + lensPosition_(0), bestPosition_(0), currentContrast_(0.0),\n> > > > + previousContrast_(0.0), maxContrast_(0.0), maxStep_(0),\n> > > > + coarseCompleted_(false), fineCompleted_(false),\n> > > > + lowStep_(0), highStep_(kMaxFocusSteps)\n> > > > + {\n> > > > + }\n> > >\n> > > Let's look at the implementation details later but I have one question\n> > >\n> > > > +\n> > > > + virtual ~AfHillClimbing() {}\n> > > > +\n> > > > + void setMode(controls::AfModeEnum mode) final\n> > > > + {\n> > > > + if (mode != mode_) {\n> > > > + LOG(Af, Debug) << \"Switched AF mode from \" << mode_\n> > > > + << \" to \" << mode;\n> > > > + pauseState_ = libcamera::controls::AfPauseStateRunning;\n> > > > + mode_ = mode;\n> > > > + }\n> > > > + }\n> > > > +\n> > > > + void setRange([[maybe_unused]] controls::AfRangeEnum range) final\n> > > > + {\n> > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > + }\n> > > > +\n> > > > + void setSpeed([[maybe_unused]] controls::AfSpeedEnum speed) final\n> > > > + {\n> > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > + }\n> > > > +\n> > > > + void setTrigger(controls::AfTriggerEnum trigger) final\n> > > > + {\n> > > > + LOG(Af, Debug) << \"Trigger called in mode \" << mode_\n> > > > + << \" with \" << trigger;\n> > > > + if (mode_ == libcamera::controls::AfModeAuto) {\n> > > > + if (trigger == libcamera::controls::AfTriggerStart)\n> > > > + afReset();\n> > >\n> > > This seems out of place..\n> > > Whenever I trigger an autofocus scan I get the lens reset to 0 causing\n> > > a not very nice effect. Why are you resetting the lens position here?\n\nThis was the easiest solution to avoid additional problems in initial\nimplementation. And this one is also done this way in the original\nimplementation for IPU3. We are looking for the local peak value\nwhich for simple scenes is also the global peak, but it is not\nguaranteed. It is easier and safer to always start focus from the point\nbeyond the hyperfocale and go closer and closer until the peak. When We\nstart scanning from the current position, then We have additional problems.\nFor example, which direction of scanning to choose and when to decide that\nthe opposite direction will be better, and We need to move back.\nI am pretty sure We can improve this implementation, but this is\nsomething We can do gradually :)\n\n> > >\n> > > > + else\n> > > > + state_ = libcamera::controls::AfStateIdle;\n> > > > + }\n> > > > + }\n> > > > +\n> > > > + void setPause(controls::AfPauseEnum pause) final\n> > > > + {\n> > > > + /* \\todo: add the AfPauseDeferred mode */\n> > > > + if (mode_ == libcamera::controls::AfModeContinuous) {\n> > > > + if (pause == libcamera::controls::AfPauseImmediate)\n> > > > + pauseState_ = libcamera::controls::AfPauseStatePaused;\n> > > > + else if (pause == libcamera::controls::AfPauseResume)\n> > > > + pauseState_ = libcamera::controls::AfPauseStateRunning;\n> > > > + }\n> > > > + }\n> > > > +\n> > > > + void setLensPosition([[maybe_unused]] float lensPosition) final\n> > > > + {\n> > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > + }\n> > > > +\n> > > > + /* These methods should be implemented by derived class */\n> > > > + virtual void setMetering(controls::AfMeteringEnum metering) = 0;\n> > > > + virtual void setWindows(Span windows) = 0;\n> > > > +\n> > > > +protected:\n> > > > + uint32_t processAutofocus(double currentContrast)\n> > > > + {\n> > > > + currentContrast_ = currentContrast;\n> > > > +\n> > > > + /* If we are in a paused state, we won't process the stats */\n> > > > + if (pauseState_ == libcamera::controls::AfPauseStatePaused)\n> > > > + return lensPosition_;\n> > > > +\n> > > > + /* Depending on the mode, we may or may not process the stats */\n> > > > + if (state_ == libcamera::controls::AfStateIdle)\n> > > > + return lensPosition_;\n> > > > +\n> > > > + if (state_ != libcamera::controls::AfStateFocused) {\n> > > > + afCoarseScan();\n> > > > + afFineScan();\n> > > > + } else {\n> > > > + /* We can re-start the scan at any moment in AfModeContinuous */\n> > > > + if (mode_ == libcamera::controls::AfModeContinuous)\n> > > > + if (afIsOutOfFocus())\n> > > > + afReset();\n> > > > + }\n> > > > +\n> > > > + return lensPosition_;\n> > >\n> > > Let me recap one point. We are still missing a generic definition for\n> > > the lens position value. As you can see we use\n> > > V4L2_CID_FOCUS_ABSOLUTE which transports the actual value to be set in\n> > > the v4l2 control. This can't work here and we are planning to remove\n> > > V4L2 controls from the IPA in general, but while for other controls we\n> > > have defined generic ranges, for the lens position we are still\n> > > missing a way to compute a \"generic\" value in the IPA, send it to the\n> > > pipeline handler by using libcamera::controls::LensPosition and have\n> > > the CameraLens class translate it to the device-specific value.\n> > >\n> > > I'm a bit lazy and I'm not chasing how lensPosition_ is here computed,\n> > > but have you considered how this could be generalized already ? As an\n> > > example, it seems to me the values I get are in the 0-180 range, while\n> > > in example the lens I'm using ranges from 0-4096...\n\nCurrently, there is one hard-coded range of (0-1023). I was thinking that\nWe could specify the range in the algorithm tuning file as these files\nare specific for camera model, but this would require manual\nconfiguration for each camera.\n\nI have seen some discussions here about calculation of the hyperfocale\nand basing the range on that, but this would require additional tests\nand measurement. The simplest solution I can think about is just to\nnormalize the values specific for different lens to one general range,\nfor example (0.00 - 1.00).\n\nBest regards\nDaniel\n\n> > >\n> > > > + }\n> > > > +\n> > > > +private:\n> > > > + void afCoarseScan()\n> > > > + {\n> > > > + if (coarseCompleted_)\n> > > > + return;\n> > > > +\n> > > > + if (afScan(kCoarseSearchStep)) {\n> > > > + coarseCompleted_ = true;\n> > > > + maxContrast_ = 0;\n> > > > + lensPosition_ = lensPosition_ - (lensPosition_ * kFineRange);\n> > > > + previousContrast_ = 0;\n> > > > + maxStep_ = std::clamp(lensPosition_ + static_cast((lensPosition_ * kFineRange)),\n> > > > + 0U, highStep_);\n> > > > + }\n> > > > + }\n> > > > +\n> > > > + void afFineScan()\n> > > > + {\n> > > > + if (!coarseCompleted_)\n> > > > + return;\n> > > > +\n> > > > + if (afScan(kFineSearchStep)) {\n> > > > + LOG(Af, Debug) << \"AF found the best focus position !\";\n> > > > + state_ = libcamera::controls::AfStateFocused;\n> > > > + fineCompleted_ = true;\n> > > > + }\n> > > > + }\n> > > > +\n> > > > + bool afScan(uint32_t minSteps)\n> > > > + {\n> > > > + if (lensPosition_ + minSteps > maxStep_) {\n> > > > + /* If the max step is reached, move lens to the position. */\n> > > > + lensPosition_ = bestPosition_;\n> > > > + return true;\n> > > > + } else {\n> > > > + /*\n> > > > + * Find the maximum of the variance by estimating its\n> > > > + * derivative. If the direction changes, it means we have passed\n> > > > + * a maximum one step before.\n> > > > + */\n> > > > + if ((currentContrast_ - maxContrast_) >= -(maxContrast_ * 0.1)) {\n> > > > + /*\n> > > > + * Positive and zero derivative:\n> > > > + * The variance is still increasing. The focus could be\n> > > > + * increased for the next comparison. Also, the max\n> > > > + * variance and previous focus value are updated.\n> > > > + */\n> > > > + bestPosition_ = lensPosition_;\n> > > > + lensPosition_ += minSteps;\n> > > > + maxContrast_ = currentContrast_;\n> > > > + } else {\n> > > > + /*\n> > > > + * Negative derivative:\n> > > > + * The variance starts to decrease which means the maximum\n> > > > + * variance is found. Set focus step to previous good one\n> > > > + * then return immediately.\n> > > > + */\n> > > > + lensPosition_ = bestPosition_;\n> > > > + return true;\n> > > > + }\n> > > > + }\n> > > > +\n> > > > + previousContrast_ = currentContrast_;\n> > > > + LOG(Af, Debug) << \" Previous step is \" << bestPosition_\n> > > > + << \" Current step is \" << lensPosition_;\n> > > > + return false;\n> > > > + }\n> > > > +\n> > > > + void afReset()\n> > > > + {\n> > > > + LOG(Af, Debug) << \"Reset AF parameters\";\n> > > > + lensPosition_ = lowStep_;\n> > > > + maxStep_ = highStep_;\n> > > > + state_ = libcamera::controls::AfStateScanning;\n> > > > + previousContrast_ = 0.0;\n> > > > + coarseCompleted_ = false;\n> > > > + fineCompleted_ = false;\n> > > > + maxContrast_ = 0.0;\n> > > > + }\n> > > > +\n> > > > + bool afIsOutOfFocus()\n> > > > + {\n> > > > + const uint32_t diff_var = std::abs(currentContrast_ -\n> > > > + maxContrast_);\n> > > > + const double var_ratio = diff_var / maxContrast_;\n> > > > + LOG(Af, Debug) << \"Variance change rate: \" << var_ratio\n> > > > + << \" Current VCM step: \" << lensPosition_;\n> > > > + if (var_ratio > kMaxChange)\n> > > > + return true;\n> > > > + else\n> > > > + return false;\n> > > > + }\n> > > > +\n> > > > + controls::AfModeEnum mode_;\n> > > > + controls::AfStateEnum state_;\n> > > > + controls::AfPauseStateEnum pauseState_;\n> > > > +\n> > > > + /* VCM step configuration. It is the current setting of the VCM step. */\n> > > > + uint32_t lensPosition_;\n> > > > + /* The best VCM step. It is a local optimum VCM step during scanning. */\n> > > > + uint32_t bestPosition_;\n> > > > +\n> > > > + /* Current AF statistic contrast. */\n> > > > + double currentContrast_;\n> > > > + /* It is used to determine the derivative during scanning */\n> > > > + double previousContrast_;\n> > > > + double maxContrast_;\n> > > > + /* The designated maximum range of focus scanning. */\n> > > > + uint32_t maxStep_;\n> > > > + /* If the coarse scan completes, it is set to true. */\n> > > > + bool coarseCompleted_;\n> > > > + /* If the fine scan completes, it is set to true. */\n> > > > + bool fineCompleted_;\n> > > > +\n> > > > + uint32_t lowStep_;\n> > > > + uint32_t highStep_;\n> > > > +\n> > > > + /*\n> > > > + * Maximum focus steps of the VCM control\n> > > > + * \\todo should be obtained from the VCM driver\n> > > > + */\n> > > > + static constexpr uint32_t kMaxFocusSteps = 1023;\n> > > > +\n> > > > + /* Minimum focus step for searching appropriate focus */\n> > > > + static constexpr uint32_t kCoarseSearchStep = 30;\n> > > > + static constexpr uint32_t kFineSearchStep = 1;\n> > > > +\n> > > > + /* Max ratio of variance change, 0.0 < kMaxChange < 1.0 */\n> > > > + static constexpr double kMaxChange = 0.5;\n> > > > +\n> > > > + /* Fine scan range 0 < kFineRange < 1 */\n> > > > + static constexpr double kFineRange = 0.05;\n> > > > +};\n> > > > +\n> > > > +} /* namespace ipa::common::algorithms */\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> > > > index ab8da13a..860dc199 100644\n> > > > --- a/src/ipa/libipa/algorithms/meson.build\n> > > > +++ b/src/ipa/libipa/algorithms/meson.build\n> > > > @@ -2,8 +2,10 @@\n> > > >\n> > > > common_ipa_algorithms_headers = files([\n> > > > 'af_algorithm.h',\n> > > > + 'af_hill_climbing.h',\n> > > > ])\n> > > >\n> > > > common_ipa_algorithms_sources = files([\n> > > > 'af_algorithm.cpp',\n> > > > + 'af_hill_climbing.cpp',\n> > > > ])\n> > > > --\n> > > > 2.34.1\n> > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F0000BE173\n\tfor ;\n\tMon, 18 Jul 2022 15:28:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5F4AA6330E;\n\tMon, 18 Jul 2022 17:28:31 +0200 (CEST)","from mail-lj1-x22b.google.com (mail-lj1-x22b.google.com\n\t[IPv6:2a00:1450:4864:20::22b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CB42F6048B\n\tfor ;\n\tMon, 18 Jul 2022 17:28:29 +0200 (CEST)","by mail-lj1-x22b.google.com with SMTP id w2so14025747ljj.7\n\tfor ;\n\tMon, 18 Jul 2022 08:28:29 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658158111;\n\tbh=1SULYx+LAxXlHjJidOH4ur7QqGlOvfg5u95I3lfbBUI=;\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=xtrlcjar0MTiEqPsVL6TeoFHKFyHx3AzB4u7pG8U/2jL9ZEFzyLHz1PMItawnDavR\n\tp9NINWL7uH05oxcYHY8COtcHoKd0ITqFJDrRkdXOe/Kmnt33VOn93IRG6Jx7mfl4nf\n\tIRTo4blxVoct53Yl10UzjLMFtc0AN5GucIOj2XQ73E86fe0N4p3bUvMk+H1LErHPuP\n\tHQDAhVQHWR2SPfEmafjBr+KowAWaQ+hFFdiLJed0uR6Y2XGvx7sP04QeBrf+c7rifK\n\t/7WKDcHP92exkxthQZhQ8HNwZRijVhtDZ435qI0o2pECw88CTpq7o8W7MnDk+JSzja\n\tU9ZXa5sdpY5+g==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20210112.gappssmtp.com; s=20210112;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=oKL2TIItDLhRjPic5+aIVXEURELfPgch68c+7VmNCnk=;\n\tb=iBShKDH5UqJOaYp3x1nRzvTSOxOEFUYTPYmNYvlZsU9hpsuJ8RTQp+vKB8ZslYsHgm\n\t423vlDFhsVWGtmipU+qEOnNAz6OzzQdPVroxNxg3OgcqPvZgMzl5Rap/ccHiLNqwzf2l\n\t4vKhB2dOxA3POJDUMfysj87tLH/Fy2lvWfTGrLZQnWMLi7FRNzKiajQk3j+DXqYaj0Iz\n\tIdcKnguidwZ1C9pdaVsNMOTv7V43lfrwIeoMIoUbjNLyF+Pqheypn0QwKQZGk2Fiaq31\n\t0EDEh30Sod/LHAlZi9FxC/vPRTLKP/FeAWUl6YSMoYqBkMnIfhNbtabeP6W33F9XEn9i\n\tuQEA=="],"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=\"iBShKDH5\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=oKL2TIItDLhRjPic5+aIVXEURELfPgch68c+7VmNCnk=;\n\tb=eSsWWfSx8JRJNz2yUb3MA//MFA6HdA4lu90/jpyQzHKolMUJ0VdEc8x66Us0y2FgrD\n\tjRLnVGa4DUa8Yzs4t86pYa5QmR6qRIKcu+DZ0lbYwnXfTciSo6UJNKQUnAkA6jlnLLb3\n\tMm76h8rKnxhKauMHTNBbsKJvBvlRKf+O4nP+YIn/I/8Fu/tOUanBuGzi2oJV2EGdlr9H\n\tpjcBxeYVazFu0lyvTVo7lfgIWZkVHDbJZQlKo7PvreKqmj3tH9zcpG4xA8oWt/8T3Haq\n\tPTF8P04bJcPYMgQxHCPkSPd9/UZ0DRzG0gl6YDtcQXYD+dInilTqkq3/nJWmV2nIkp7w\n\tHtOw==","X-Gm-Message-State":"AJIora8rV8i73J3iEnO5cjD5pQWZQN9bbbUj+DumqURLXdeXqpaP2SCm\n\tekgx6cinbMdtfYXR/Q3yVQMGyXTGS3i/diTJrwzLvw==","X-Google-Smtp-Source":"AGRyM1vaD98gfL3aK5hWFOJdJ0990Q8mX1vf1R+tBS7Pr2+rIvHCZq661akpZucLzUvrFMPWsoURKOEROMnPNT6SEK0=","X-Received":"by 2002:a2e:545d:0:b0:25d:84ad:c19e with SMTP id\n\ty29-20020a2e545d000000b0025d84adc19emr13108776ljd.393.1658158108958;\n\tMon, 18 Jul 2022 08:28:28 -0700 (PDT)","MIME-Version":"1.0","References":"<20220713084317.24268-1-dse@thaumatec.com>\n\t<20220713084317.24268-3-dse@thaumatec.com>\n\t<20220714161723.2w6p6c6odeapcsfq@uno.localdomain>\n\t<20220714163324.vjiqxhrkqkvuqil3@uno.localdomain>\n\t<20220715065232.ycxow3pou7nzvasy@uno.localdomain>","In-Reply-To":"<20220715065232.ycxow3pou7nzvasy@uno.localdomain>","Date":"Mon, 18 Jul 2022 17:28:17 +0200","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 02/11] ipa: Add class that\n\timplements base AF control algorithm","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":23956,"web_url":"https://patchwork.libcamera.org/comment/23956/","msgid":"","date":"2022-07-19T00:17:21","subject":"Re: [libcamera-devel] [PATCH v2 02/11] ipa: Add class that\n\timplements base AF control algorithm","submitter":{"id":105,"url":"https://patchwork.libcamera.org/api/people/105/","name":"Kate Hsuan","email":"hpa@redhat.com"},"content":"Hi,\n\nOn Mon, Jul 18, 2022 at 11:28 PM Daniel Semkowicz wrote:\n>\n> Hi Jacopo,\n>\n> On Fri, Jul 15, 2022 at 8:52 AM Jacopo Mondi wrote:\n> >\n> > One day I'll lear to use email\n> >\n> > On Thu, Jul 14, 2022 at 06:33:24PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > Kate in cc for real this time!\n> >\n> > for real\n> >\n> > >\n> > > On Thu, Jul 14, 2022 at 06:17:23PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > > Hi Daniel\n> > > >\n> > > > On Wed, Jul 13, 2022 at 10:43:08AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > Move the code that was common for IPU3 and RPi AF algorithms to\n> > > > > a separate class independent of platform specific code.\n> > > > > This way each platform can just implement contrast calculation and\n> > > > > run the AF control loop basing on this class.\n> > > > >\n> > > >\n> > > > This is exactly the purpose of having algorithms in libipa. I'm\n> > > > excited it's the first \"generic\" one we have, or that I know of at\n> > > > least!\n> > > >\n> > > > This mean the very IPU3 implementation this algorithm is based on\n> > > > (Kate in cc) can now be generalized, and the same for the RPi one we\n> > > > have on the list!\n> > > >\n> > > > > Signed-off-by: Daniel Semkowicz \n> > > > > ---\n> > > > > .../libipa/algorithms/af_hill_climbing.cpp | 89 +++++++\n> > > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 251 ++++++++++++++++++\n> > > > > src/ipa/libipa/algorithms/meson.build | 2 +\n> > > > > 3 files changed, 342 insertions(+)\n> > > > > create mode 100644 src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > create mode 100644 src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > >\n> > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > new file mode 100644\n> > > > > index 00000000..f666c6c2\n> > > > > --- /dev/null\n> > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > @@ -0,0 +1,89 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2021, Red Hat\n> > > > > + * Copyright (C) 2022, Ideas On Board\n> > > > > + * Copyright (C) 2022, Theobroma Systems\n> > > > > + *\n> > > > > + * af_hill_climbing.cpp - AF Hill Climbing common algorithm\n> > > > > + */\n> > > >\n> > > > Any particular reason why the implementation is in the .h file ?\n>\n> In my original approach I had to pass the Module template argument, so\n> using the template, forced me to do the implementation in a header file.\n>\n> This is an additional point in favor of the approach with multiple\n> inheritence proposed by you in 01, as We can hide the implementation of\n> the AfHillClimbing in .cpp file.\n>\n> > > >\n> > > > > +\n> > > > > +#include \"af_hill_climbing.h\"\n> > > > > +\n> > > > > +/**\n> > > > > + * \\file af_hill_climbing.h\n> > > > > + * \\brief AF Hill Climbing common algorithm\n> > > > > + */\n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +LOG_DEFINE_CATEGORY(Af)\n> > > > > +\n> > > > > +namespace ipa::common::algorithms {\n> > > > > +\n> > > > > +/**\n> > > > > + * \\class AfHillClimbing\n> > > > > + * \\brief The base class implementing hill climbing AF control algorithm\n> > > > > + * \\tparam Module The IPA module type for this class of algorithms\n> > > > > + *\n> > > > > + * Control part of auto focus algorithm. It calculates the lens position basing\n> > > > > + * on contrast measure supplied by the higher level. This way it is independent\n> > > > > + * from the platform.\n> > > > > + *\n> > > > > + * Derived class should call processAutofocus() for each measured contrast value\n> > > > > + * and set the lens to the calculated position.\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn AfHillClimbing::setMode()\n> > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setMode\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn AfHillClimbing::setRange()\n> > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setRange\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn AfHillClimbing::setSpeed()\n> > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setSpeed\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn AfHillClimbing::setMetering()\n> > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setMetering\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn AfHillClimbing::setWindows()\n> > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setWindows\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn AfHillClimbing::setTrigger()\n> > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setTrigger\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn AfHillClimbing::setPause()\n> > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setPause\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn AfHillClimbing::setLensPosition()\n> > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setLensPosition\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\fn AfHillClimbing::processAutofocus()\n> > > > > + * \\brief Run the auto focus algorithm loop\n> > > > > + * \\param[in] currentContrast New value of contrast measured for current frame\n> > > > > + *\n> > > > > + * This method should be called for each new contrast value that was measured,\n> > > > > + * usually in the process() method.\n> > > > > + *\n> > > > > + * \\return New lens position calculated by AF algorithm\n> > > > > + */\n> > > > > +\n> > > > > +} /* namespace ipa::common::algorithms */\n> > > > > +\n> > > > > +} /* namespace libcamera */\n> > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > new file mode 100644\n> > > > > index 00000000..db9fc058\n> > > > > --- /dev/null\n> > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > @@ -0,0 +1,251 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2021, Red Hat\n> > > > > + * Copyright (C) 2022, Ideas On Board\n> > > > > + * Copyright (C) 2022, Theobroma Systems\n> > > > > + *\n> > > > > + * af_hill_climbing.h - AF Hill Climbing common algorithm\n> > > > > + */\n> > > > > +\n> > > > > +#pragma once\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \"af_algorithm.h\"\n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +LOG_DECLARE_CATEGORY(Af)\n> > > > > +\n> > > > > +namespace ipa::common::algorithms {\n> > > > > +\n> > > > > +template\n> > > > > +class AfHillClimbing : public AfAlgorithm\n> > > > > +{\n> > > > > +public:\n> > > > > + AfHillClimbing()\n> > > > > + : mode_(controls::AfModeAuto), state_(controls::AfStateIdle),\n> > > > > + pauseState_(controls::AfPauseStateRunning),\n> > > > > + lensPosition_(0), bestPosition_(0), currentContrast_(0.0),\n> > > > > + previousContrast_(0.0), maxContrast_(0.0), maxStep_(0),\n> > > > > + coarseCompleted_(false), fineCompleted_(false),\n> > > > > + lowStep_(0), highStep_(kMaxFocusSteps)\n> > > > > + {\n> > > > > + }\n> > > >\n> > > > Let's look at the implementation details later but I have one question\n> > > >\n> > > > > +\n> > > > > + virtual ~AfHillClimbing() {}\n> > > > > +\n> > > > > + void setMode(controls::AfModeEnum mode) final\n> > > > > + {\n> > > > > + if (mode != mode_) {\n> > > > > + LOG(Af, Debug) << \"Switched AF mode from \" << mode_\n> > > > > + << \" to \" << mode;\n> > > > > + pauseState_ = libcamera::controls::AfPauseStateRunning;\n> > > > > + mode_ = mode;\n> > > > > + }\n> > > > > + }\n> > > > > +\n> > > > > + void setRange([[maybe_unused]] controls::AfRangeEnum range) final\n> > > > > + {\n> > > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > > + }\n> > > > > +\n> > > > > + void setSpeed([[maybe_unused]] controls::AfSpeedEnum speed) final\n> > > > > + {\n> > > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > > + }\n> > > > > +\n> > > > > + void setTrigger(controls::AfTriggerEnum trigger) final\n> > > > > + {\n> > > > > + LOG(Af, Debug) << \"Trigger called in mode \" << mode_\n> > > > > + << \" with \" << trigger;\n> > > > > + if (mode_ == libcamera::controls::AfModeAuto) {\n> > > > > + if (trigger == libcamera::controls::AfTriggerStart)\n> > > > > + afReset();\n> > > >\n> > > > This seems out of place..\n> > > > Whenever I trigger an autofocus scan I get the lens reset to 0 causing\n> > > > a not very nice effect. Why are you resetting the lens position here?\n>\n> This was the easiest solution to avoid additional problems in initial\n> implementation. And this one is also done this way in the original\n> implementation for IPU3. We are looking for the local peak value\n> which for simple scenes is also the global peak, but it is not\n> guaranteed. It is easier and safer to always start focus from the point\n> beyond the hyperfocale and go closer and closer until the peak. When We\n> start scanning from the current position, then We have additional problems.\n> For example, which direction of scanning to choose and when to decide that\n> the opposite direction will be better, and We need to move back.\n> I am pretty sure We can improve this implementation, but this is\n> something We can do gradually :)\n>\n> > > >\n> > > > > + else\n> > > > > + state_ = libcamera::controls::AfStateIdle;\n> > > > > + }\n> > > > > + }\n> > > > > +\n> > > > > + void setPause(controls::AfPauseEnum pause) final\n> > > > > + {\n> > > > > + /* \\todo: add the AfPauseDeferred mode */\n> > > > > + if (mode_ == libcamera::controls::AfModeContinuous) {\n> > > > > + if (pause == libcamera::controls::AfPauseImmediate)\n> > > > > + pauseState_ = libcamera::controls::AfPauseStatePaused;\n> > > > > + else if (pause == libcamera::controls::AfPauseResume)\n> > > > > + pauseState_ = libcamera::controls::AfPauseStateRunning;\n> > > > > + }\n> > > > > + }\n> > > > > +\n> > > > > + void setLensPosition([[maybe_unused]] float lensPosition) final\n> > > > > + {\n> > > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > > + }\n> > > > > +\n> > > > > + /* These methods should be implemented by derived class */\n> > > > > + virtual void setMetering(controls::AfMeteringEnum metering) = 0;\n> > > > > + virtual void setWindows(Span windows) = 0;\n> > > > > +\n> > > > > +protected:\n> > > > > + uint32_t processAutofocus(double currentContrast)\n> > > > > + {\n> > > > > + currentContrast_ = currentContrast;\n> > > > > +\n> > > > > + /* If we are in a paused state, we won't process the stats */\n> > > > > + if (pauseState_ == libcamera::controls::AfPauseStatePaused)\n> > > > > + return lensPosition_;\n> > > > > +\n> > > > > + /* Depending on the mode, we may or may not process the stats */\n> > > > > + if (state_ == libcamera::controls::AfStateIdle)\n> > > > > + return lensPosition_;\n> > > > > +\n> > > > > + if (state_ != libcamera::controls::AfStateFocused) {\n> > > > > + afCoarseScan();\n> > > > > + afFineScan();\n> > > > > + } else {\n> > > > > + /* We can re-start the scan at any moment in AfModeContinuous */\n> > > > > + if (mode_ == libcamera::controls::AfModeContinuous)\n> > > > > + if (afIsOutOfFocus())\n> > > > > + afReset();\n> > > > > + }\n> > > > > +\n> > > > > + return lensPosition_;\n> > > >\n> > > > Let me recap one point. We are still missing a generic definition for\n> > > > the lens position value. As you can see we use\n> > > > V4L2_CID_FOCUS_ABSOLUTE which transports the actual value to be set in\n> > > > the v4l2 control. This can't work here and we are planning to remove\n> > > > V4L2 controls from the IPA in general, but while for other controls we\n> > > > have defined generic ranges, for the lens position we are still\n> > > > missing a way to compute a \"generic\" value in the IPA, send it to the\n> > > > pipeline handler by using libcamera::controls::LensPosition and have\n> > > > the CameraLens class translate it to the device-specific value.\n> > > >\n> > > > I'm a bit lazy and I'm not chasing how lensPosition_ is here computed,\n> > > > but have you considered how this could be generalized already ? As an\n> > > > example, it seems to me the values I get are in the 0-180 range, while\n> > > > in example the lens I'm using ranges from 0-4096...\n>\n> Currently, there is one hard-coded range of (0-1023). I was thinking that\n> We could specify the range in the algorithm tuning file as these files\n> are specific for camera model, but this would require manual\n> configuration for each camera.\n\n\nBefore, I had submitted the patch to get the maximum steps of the lens\nbut not get merged and stopped at v3.\nhttps://patchwork.libcamera.org/project/libcamera/list/?series=3069\n\nThis patch can set the vcm range automatically. If this is helpful for\nsetting vcm range, we could carry on working on this.\n\nThank you :)\n\n\n>\n> I have seen some discussions here about calculation of the hyperfocale\n> and basing the range on that, but this would require additional tests\n> and measurement. The simplest solution I can think about is just to\n> normalize the values specific for different lens to one general range,\n> for example (0.00 - 1.00).\n>\n> Best regards\n> Daniel\n>\n> > > >\n> > > > > + }\n> > > > > +\n> > > > > +private:\n> > > > > + void afCoarseScan()\n> > > > > + {\n> > > > > + if (coarseCompleted_)\n> > > > > + return;\n> > > > > +\n> > > > > + if (afScan(kCoarseSearchStep)) {\n> > > > > + coarseCompleted_ = true;\n> > > > > + maxContrast_ = 0;\n> > > > > + lensPosition_ = lensPosition_ - (lensPosition_ * kFineRange);\n> > > > > + previousContrast_ = 0;\n> > > > > + maxStep_ = std::clamp(lensPosition_ + static_cast((lensPosition_ * kFineRange)),\n> > > > > + 0U, highStep_);\n> > > > > + }\n> > > > > + }\n> > > > > +\n> > > > > + void afFineScan()\n> > > > > + {\n> > > > > + if (!coarseCompleted_)\n> > > > > + return;\n> > > > > +\n> > > > > + if (afScan(kFineSearchStep)) {\n> > > > > + LOG(Af, Debug) << \"AF found the best focus position !\";\n> > > > > + state_ = libcamera::controls::AfStateFocused;\n> > > > > + fineCompleted_ = true;\n> > > > > + }\n> > > > > + }\n> > > > > +\n> > > > > + bool afScan(uint32_t minSteps)\n> > > > > + {\n> > > > > + if (lensPosition_ + minSteps > maxStep_) {\n> > > > > + /* If the max step is reached, move lens to the position. */\n> > > > > + lensPosition_ = bestPosition_;\n> > > > > + return true;\n> > > > > + } else {\n> > > > > + /*\n> > > > > + * Find the maximum of the variance by estimating its\n> > > > > + * derivative. If the direction changes, it means we have passed\n> > > > > + * a maximum one step before.\n> > > > > + */\n> > > > > + if ((currentContrast_ - maxContrast_) >= -(maxContrast_ * 0.1)) {\n> > > > > + /*\n> > > > > + * Positive and zero derivative:\n> > > > > + * The variance is still increasing. The focus could be\n> > > > > + * increased for the next comparison. Also, the max\n> > > > > + * variance and previous focus value are updated.\n> > > > > + */\n> > > > > + bestPosition_ = lensPosition_;\n> > > > > + lensPosition_ += minSteps;\n> > > > > + maxContrast_ = currentContrast_;\n> > > > > + } else {\n> > > > > + /*\n> > > > > + * Negative derivative:\n> > > > > + * The variance starts to decrease which means the maximum\n> > > > > + * variance is found. Set focus step to previous good one\n> > > > > + * then return immediately.\n> > > > > + */\n> > > > > + lensPosition_ = bestPosition_;\n> > > > > + return true;\n> > > > > + }\n> > > > > + }\n> > > > > +\n> > > > > + previousContrast_ = currentContrast_;\n> > > > > + LOG(Af, Debug) << \" Previous step is \" << bestPosition_\n> > > > > + << \" Current step is \" << lensPosition_;\n> > > > > + return false;\n> > > > > + }\n> > > > > +\n> > > > > + void afReset()\n> > > > > + {\n> > > > > + LOG(Af, Debug) << \"Reset AF parameters\";\n> > > > > + lensPosition_ = lowStep_;\n> > > > > + maxStep_ = highStep_;\n> > > > > + state_ = libcamera::controls::AfStateScanning;\n> > > > > + previousContrast_ = 0.0;\n> > > > > + coarseCompleted_ = false;\n> > > > > + fineCompleted_ = false;\n> > > > > + maxContrast_ = 0.0;\n> > > > > + }\n> > > > > +\n> > > > > + bool afIsOutOfFocus()\n> > > > > + {\n> > > > > + const uint32_t diff_var = std::abs(currentContrast_ -\n> > > > > + maxContrast_);\n> > > > > + const double var_ratio = diff_var / maxContrast_;\n> > > > > + LOG(Af, Debug) << \"Variance change rate: \" << var_ratio\n> > > > > + << \" Current VCM step: \" << lensPosition_;\n> > > > > + if (var_ratio > kMaxChange)\n> > > > > + return true;\n> > > > > + else\n> > > > > + return false;\n> > > > > + }\n> > > > > +\n> > > > > + controls::AfModeEnum mode_;\n> > > > > + controls::AfStateEnum state_;\n> > > > > + controls::AfPauseStateEnum pauseState_;\n> > > > > +\n> > > > > + /* VCM step configuration. It is the current setting of the VCM step. */\n> > > > > + uint32_t lensPosition_;\n> > > > > + /* The best VCM step. It is a local optimum VCM step during scanning. */\n> > > > > + uint32_t bestPosition_;\n> > > > > +\n> > > > > + /* Current AF statistic contrast. */\n> > > > > + double currentContrast_;\n> > > > > + /* It is used to determine the derivative during scanning */\n> > > > > + double previousContrast_;\n> > > > > + double maxContrast_;\n> > > > > + /* The designated maximum range of focus scanning. */\n> > > > > + uint32_t maxStep_;\n> > > > > + /* If the coarse scan completes, it is set to true. */\n> > > > > + bool coarseCompleted_;\n> > > > > + /* If the fine scan completes, it is set to true. */\n> > > > > + bool fineCompleted_;\n> > > > > +\n> > > > > + uint32_t lowStep_;\n> > > > > + uint32_t highStep_;\n> > > > > +\n> > > > > + /*\n> > > > > + * Maximum focus steps of the VCM control\n> > > > > + * \\todo should be obtained from the VCM driver\n> > > > > + */\n> > > > > + static constexpr uint32_t kMaxFocusSteps = 1023;\n> > > > > +\n> > > > > + /* Minimum focus step for searching appropriate focus */\n> > > > > + static constexpr uint32_t kCoarseSearchStep = 30;\n> > > > > + static constexpr uint32_t kFineSearchStep = 1;\n> > > > > +\n> > > > > + /* Max ratio of variance change, 0.0 < kMaxChange < 1.0 */\n> > > > > + static constexpr double kMaxChange = 0.5;\n> > > > > +\n> > > > > + /* Fine scan range 0 < kFineRange < 1 */\n> > > > > + static constexpr double kFineRange = 0.05;\n> > > > > +};\n> > > > > +\n> > > > > +} /* namespace ipa::common::algorithms */\n> > > > > +} /* namespace libcamera */\n> > > > > diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> > > > > index ab8da13a..860dc199 100644\n> > > > > --- a/src/ipa/libipa/algorithms/meson.build\n> > > > > +++ b/src/ipa/libipa/algorithms/meson.build\n> > > > > @@ -2,8 +2,10 @@\n> > > > >\n> > > > > common_ipa_algorithms_headers = files([\n> > > > > 'af_algorithm.h',\n> > > > > + 'af_hill_climbing.h',\n> > > > > ])\n> > > > >\n> > > > > common_ipa_algorithms_sources = files([\n> > > > > 'af_algorithm.cpp',\n> > > > > + 'af_hill_climbing.cpp',\n> > > > > ])\n> > > > > --\n> > > > > 2.34.1\n> > > > >\n>\n\n\n--\nBR,\nKate","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 984FFBE173\n\tfor ;\n\tTue, 19 Jul 2022 00:17:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D810763312;\n\tTue, 19 Jul 2022 02:17:39 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1932F601B8\n\tfor ;\n\tTue, 19 Jul 2022 02:17:36 +0200 (CEST)","from mail-yb1-f200.google.com (mail-yb1-f200.google.com\n\t[209.85.219.200]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id\n\tus-mta-294-71pf_IARN8qe_HQ4TKjl6A-1; Mon, 18 Jul 2022 20:17:34 -0400","by mail-yb1-f200.google.com with SMTP id\n\tm5-20020a2598c5000000b0066faab590c5so9705997ybo.7\n\tfor ;\n\tMon, 18 Jul 2022 17:17:34 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658189859;\n\tbh=XmtG2iyh/wzmC6VlR4nuS1BnZ4Ctf1ONou0RxLB2fLU=;\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=nRlcSBgoUaBPynnoYbUkXEEcd1+Fii4/OufZRixqK7EsJEX4X9luKSmX4mRqrq+LI\n\tHJD2NYy0T8f4sR0daMMdK1UrdEFc+CB/Dhq6iAkZ91OZu3I+NUH6jJHAF95LxxxF9k\n\trftpVhMnRTb0uzoXHo/I8z+7NCCSxNbaRXWtYdmB9K2fEIdxkQ0UrunnV0HkfXQanP\n\tUZ41+H8Guh9pTJSaqq9dClC6GxpIYu/XMWZo0wdg7Ft6jRKPds2R4p2K19IpOJNHCO\n\tgWjnSB9lnPrSaK67NzSMhDlPp6WydX6hV8vH8HY5ueWK1y9F6TgNJgBXwCHytTNUiH\n\tQu1eA+IaBShQQ==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1658189855;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=XW0ISCCG3cM7b6LNXfcPG8Dau4hogqBsy+epb0hhJg8=;\n\tb=LVdlqyamr5d/lAdmov3oNmkbJsmkWhq3GTEIvZAburbTzhYet0sp4DppUdidJ7+RFJoAZr\n\tFc7EitUsJu4XJqohg6tsy1y2hD7PcvA6xjZ9eJF3brIdHiTaBtYx/BfeqcQbcuTx4/PSti\n\tM1dNpDkcSb/VYAm3rTGdG7gO2VHQfUY="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"LVdlqyam\"; \n\tdkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=hpa@redhat.com"],"X-MC-Unique":"71pf_IARN8qe_HQ4TKjl6A-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=XW0ISCCG3cM7b6LNXfcPG8Dau4hogqBsy+epb0hhJg8=;\n\tb=C1tMmrSFl5knhfO0aLFRtGNntYiFF4WQsFe7lOnb9PXnrv1O+m5Ht7AocjV5dfhXhq\n\tWxxK+eX0UMGRMJ+i0aaQEggv+3ZQo+9O9Fcl4mhIZnO8Uq2ajFPWlUz8mNQtxOlCBhXr\n\tXY1vTHcp8JclSh6I0pG1FChpRwo7B6IkwFU2GBbVMM40rI3njxJc7zir4rTsOoROFfIN\n\tVrz0QBDN8W8EzsFexlQ+4GgfGd50+pG4XqTT1alIS166B04MXDBY3bSA68v0HS7Y6d1C\n\t+CiS9yl/JEcVUrU4JxttiO/F+HGB8zBatQWTbDSnud/zj3ZVN6FxDXcAFqKDDr9cCwsh\n\tT6ow==","X-Gm-Message-State":"AJIora+iqeChBTtUmhKtEQ6XyBPcFKbRJNPYf9AcyxNnjR4P97JHPhQl\n\to8RNKx2QNedEqDaaZD7Ienyki+AqUmIaOyUVvi8Unas0FEQQ3p4J+n/FQhpnKl3bg72Y6z7U2qO\n\tpgRaDstHEjnyZpEk3O8y+BAjcFgcqrj5LL6ryDGHbwjWPLZVq9Q==","X-Received":["by 2002:a0d:d58d:0:b0:31e:5b1b:c2dc with SMTP id\n\tx135-20020a0dd58d000000b0031e5b1bc2dcmr500180ywd.243.1658189853354; \n\tMon, 18 Jul 2022 17:17:33 -0700 (PDT)","by 2002:a0d:d58d:0:b0:31e:5b1b:c2dc with SMTP id\n\tx135-20020a0dd58d000000b0031e5b1bc2dcmr500151ywd.243.1658189852827;\n\tMon, 18 Jul 2022 17:17:32 -0700 (PDT)"],"X-Google-Smtp-Source":"AGRyM1s3Uq3iwq/3M+S88+dCQBfY3ll0JnqqOgzflly8SUpiFCgkKzzLv9kUmdFASf2i864K5sV6URYpd7m7S9I0zEY=","MIME-Version":"1.0","References":"<20220713084317.24268-1-dse@thaumatec.com>\n\t<20220713084317.24268-3-dse@thaumatec.com>\n\t<20220714161723.2w6p6c6odeapcsfq@uno.localdomain>\n\t<20220714163324.vjiqxhrkqkvuqil3@uno.localdomain>\n\t<20220715065232.ycxow3pou7nzvasy@uno.localdomain>\n\t","In-Reply-To":"","Date":"Tue, 19 Jul 2022 08:17:21 +0800","Message-ID":"","To":"Daniel Semkowicz ","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 02/11] ipa: Add class that\n\timplements base AF control algorithm","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":"Kate Hsuan via libcamera-devel ","Reply-To":"Kate Hsuan ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23957,"web_url":"https://patchwork.libcamera.org/comment/23957/","msgid":"<20220719075832.lzw7gazpryc5reel@uno.localdomain>","date":"2022-07-19T07:58:32","subject":"Re: [libcamera-devel] [PATCH v2 02/11] ipa: Add class that\n\timplements base AF control algorithm","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Daniel, Kate,\n thanks for the input.\n\nOn Tue, Jul 19, 2022 at 08:17:21AM +0800, Kate Hsuan wrote:\n> Hi,\n>\n> On Mon, Jul 18, 2022 at 11:28 PM Daniel Semkowicz wrote:\n> >\n> > Hi Jacopo,\n> >\n> > On Fri, Jul 15, 2022 at 8:52 AM Jacopo Mondi wrote:\n> > >\n> > > One day I'll lear to use email\n> > >\n> > > On Thu, Jul 14, 2022 at 06:33:24PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > > Kate in cc for real this time!\n> > >\n> > > for real\n> > >\n> > > >\n> > > > On Thu, Jul 14, 2022 at 06:17:23PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > > > Hi Daniel\n> > > > >\n> > > > > On Wed, Jul 13, 2022 at 10:43:08AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > > Move the code that was common for IPU3 and RPi AF algorithms to\n> > > > > > a separate class independent of platform specific code.\n> > > > > > This way each platform can just implement contrast calculation and\n> > > > > > run the AF control loop basing on this class.\n> > > > > >\n> > > > >\n> > > > > This is exactly the purpose of having algorithms in libipa. I'm\n> > > > > excited it's the first \"generic\" one we have, or that I know of at\n> > > > > least!\n> > > > >\n> > > > > This mean the very IPU3 implementation this algorithm is based on\n> > > > > (Kate in cc) can now be generalized, and the same for the RPi one we\n> > > > > have on the list!\n> > > > >\n> > > > > > Signed-off-by: Daniel Semkowicz \n> > > > > > ---\n> > > > > > .../libipa/algorithms/af_hill_climbing.cpp | 89 +++++++\n> > > > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 251 ++++++++++++++++++\n> > > > > > src/ipa/libipa/algorithms/meson.build | 2 +\n> > > > > > 3 files changed, 342 insertions(+)\n> > > > > > create mode 100644 src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > create mode 100644 src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > >\n> > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > new file mode 100644\n> > > > > > index 00000000..f666c6c2\n> > > > > > --- /dev/null\n> > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > @@ -0,0 +1,89 @@\n> > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > > +/*\n> > > > > > + * Copyright (C) 2021, Red Hat\n> > > > > > + * Copyright (C) 2022, Ideas On Board\n> > > > > > + * Copyright (C) 2022, Theobroma Systems\n> > > > > > + *\n> > > > > > + * af_hill_climbing.cpp - AF Hill Climbing common algorithm\n> > > > > > + */\n> > > > >\n> > > > > Any particular reason why the implementation is in the .h file ?\n> >\n> > In my original approach I had to pass the Module template argument, so\n> > using the template, forced me to do the implementation in a header file.\n> >\n> > This is an additional point in favor of the approach with multiple\n> > inheritence proposed by you in 01, as We can hide the implementation of\n> > the AfHillClimbing in .cpp file.\n> >\n> > > > >\n> > > > > > +\n> > > > > > +#include \"af_hill_climbing.h\"\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\file af_hill_climbing.h\n> > > > > > + * \\brief AF Hill Climbing common algorithm\n> > > > > > + */\n> > > > > > +\n> > > > > > +namespace libcamera {\n> > > > > > +\n> > > > > > +LOG_DEFINE_CATEGORY(Af)\n> > > > > > +\n> > > > > > +namespace ipa::common::algorithms {\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\class AfHillClimbing\n> > > > > > + * \\brief The base class implementing hill climbing AF control algorithm\n> > > > > > + * \\tparam Module The IPA module type for this class of algorithms\n> > > > > > + *\n> > > > > > + * Control part of auto focus algorithm. It calculates the lens position basing\n> > > > > > + * on contrast measure supplied by the higher level. This way it is independent\n> > > > > > + * from the platform.\n> > > > > > + *\n> > > > > > + * Derived class should call processAutofocus() for each measured contrast value\n> > > > > > + * and set the lens to the calculated position.\n> > > > > > + */\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\fn AfHillClimbing::setMode()\n> > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setMode\n> > > > > > + */\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\fn AfHillClimbing::setRange()\n> > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setRange\n> > > > > > + */\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\fn AfHillClimbing::setSpeed()\n> > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setSpeed\n> > > > > > + */\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\fn AfHillClimbing::setMetering()\n> > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setMetering\n> > > > > > + */\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\fn AfHillClimbing::setWindows()\n> > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setWindows\n> > > > > > + */\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\fn AfHillClimbing::setTrigger()\n> > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setTrigger\n> > > > > > + */\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\fn AfHillClimbing::setPause()\n> > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setPause\n> > > > > > + */\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\fn AfHillClimbing::setLensPosition()\n> > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setLensPosition\n> > > > > > + */\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\fn AfHillClimbing::processAutofocus()\n> > > > > > + * \\brief Run the auto focus algorithm loop\n> > > > > > + * \\param[in] currentContrast New value of contrast measured for current frame\n> > > > > > + *\n> > > > > > + * This method should be called for each new contrast value that was measured,\n> > > > > > + * usually in the process() method.\n> > > > > > + *\n> > > > > > + * \\return New lens position calculated by AF algorithm\n> > > > > > + */\n> > > > > > +\n> > > > > > +} /* namespace ipa::common::algorithms */\n> > > > > > +\n> > > > > > +} /* namespace libcamera */\n> > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > new file mode 100644\n> > > > > > index 00000000..db9fc058\n> > > > > > --- /dev/null\n> > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > @@ -0,0 +1,251 @@\n> > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > > +/*\n> > > > > > + * Copyright (C) 2021, Red Hat\n> > > > > > + * Copyright (C) 2022, Ideas On Board\n> > > > > > + * Copyright (C) 2022, Theobroma Systems\n> > > > > > + *\n> > > > > > + * af_hill_climbing.h - AF Hill Climbing common algorithm\n> > > > > > + */\n> > > > > > +\n> > > > > > +#pragma once\n> > > > > > +\n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \"af_algorithm.h\"\n> > > > > > +\n> > > > > > +namespace libcamera {\n> > > > > > +\n> > > > > > +LOG_DECLARE_CATEGORY(Af)\n> > > > > > +\n> > > > > > +namespace ipa::common::algorithms {\n> > > > > > +\n> > > > > > +template\n> > > > > > +class AfHillClimbing : public AfAlgorithm\n> > > > > > +{\n> > > > > > +public:\n> > > > > > + AfHillClimbing()\n> > > > > > + : mode_(controls::AfModeAuto), state_(controls::AfStateIdle),\n> > > > > > + pauseState_(controls::AfPauseStateRunning),\n> > > > > > + lensPosition_(0), bestPosition_(0), currentContrast_(0.0),\n> > > > > > + previousContrast_(0.0), maxContrast_(0.0), maxStep_(0),\n> > > > > > + coarseCompleted_(false), fineCompleted_(false),\n> > > > > > + lowStep_(0), highStep_(kMaxFocusSteps)\n> > > > > > + {\n> > > > > > + }\n> > > > >\n> > > > > Let's look at the implementation details later but I have one question\n> > > > >\n> > > > > > +\n> > > > > > + virtual ~AfHillClimbing() {}\n> > > > > > +\n> > > > > > + void setMode(controls::AfModeEnum mode) final\n> > > > > > + {\n> > > > > > + if (mode != mode_) {\n> > > > > > + LOG(Af, Debug) << \"Switched AF mode from \" << mode_\n> > > > > > + << \" to \" << mode;\n> > > > > > + pauseState_ = libcamera::controls::AfPauseStateRunning;\n> > > > > > + mode_ = mode;\n> > > > > > + }\n> > > > > > + }\n> > > > > > +\n> > > > > > + void setRange([[maybe_unused]] controls::AfRangeEnum range) final\n> > > > > > + {\n> > > > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > > > + }\n> > > > > > +\n> > > > > > + void setSpeed([[maybe_unused]] controls::AfSpeedEnum speed) final\n> > > > > > + {\n> > > > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > > > + }\n> > > > > > +\n> > > > > > + void setTrigger(controls::AfTriggerEnum trigger) final\n> > > > > > + {\n> > > > > > + LOG(Af, Debug) << \"Trigger called in mode \" << mode_\n> > > > > > + << \" with \" << trigger;\n> > > > > > + if (mode_ == libcamera::controls::AfModeAuto) {\n> > > > > > + if (trigger == libcamera::controls::AfTriggerStart)\n> > > > > > + afReset();\n> > > > >\n> > > > > This seems out of place..\n> > > > > Whenever I trigger an autofocus scan I get the lens reset to 0 causing\n> > > > > a not very nice effect. Why are you resetting the lens position here?\n> >\n> > This was the easiest solution to avoid additional problems in initial\n> > implementation. And this one is also done this way in the original\n> > implementation for IPU3. We are looking for the local peak value\n> > which for simple scenes is also the global peak, but it is not\n> > guaranteed. It is easier and safer to always start focus from the point\n> > beyond the hyperfocale and go closer and closer until the peak. When We\n> > start scanning from the current position, then We have additional problems.\n> > For example, which direction of scanning to choose and when to decide that\n> > the opposite direction will be better, and We need to move back.\n> > I am pretty sure We can improve this implementation, but this is\n> > something We can do gradually :)\n> >\n\nRight, I think it also depends a bit on the usage model. I was\ntriggering an AF scan every 20 frames while testing, and the reset\neffect is rather annoying. With less frequent triggers it might not be\nthat bad, and I missed the current IPU3 implementation does the same.\n\n> > > > >\n> > > > > > + else\n> > > > > > + state_ = libcamera::controls::AfStateIdle;\n> > > > > > + }\n> > > > > > + }\n> > > > > > +\n> > > > > > + void setPause(controls::AfPauseEnum pause) final\n> > > > > > + {\n> > > > > > + /* \\todo: add the AfPauseDeferred mode */\n> > > > > > + if (mode_ == libcamera::controls::AfModeContinuous) {\n> > > > > > + if (pause == libcamera::controls::AfPauseImmediate)\n> > > > > > + pauseState_ = libcamera::controls::AfPauseStatePaused;\n> > > > > > + else if (pause == libcamera::controls::AfPauseResume)\n> > > > > > + pauseState_ = libcamera::controls::AfPauseStateRunning;\n> > > > > > + }\n> > > > > > + }\n> > > > > > +\n> > > > > > + void setLensPosition([[maybe_unused]] float lensPosition) final\n> > > > > > + {\n> > > > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > > > + }\n> > > > > > +\n> > > > > > + /* These methods should be implemented by derived class */\n> > > > > > + virtual void setMetering(controls::AfMeteringEnum metering) = 0;\n> > > > > > + virtual void setWindows(Span windows) = 0;\n> > > > > > +\n> > > > > > +protected:\n> > > > > > + uint32_t processAutofocus(double currentContrast)\n> > > > > > + {\n> > > > > > + currentContrast_ = currentContrast;\n> > > > > > +\n> > > > > > + /* If we are in a paused state, we won't process the stats */\n> > > > > > + if (pauseState_ == libcamera::controls::AfPauseStatePaused)\n> > > > > > + return lensPosition_;\n> > > > > > +\n> > > > > > + /* Depending on the mode, we may or may not process the stats */\n> > > > > > + if (state_ == libcamera::controls::AfStateIdle)\n> > > > > > + return lensPosition_;\n> > > > > > +\n> > > > > > + if (state_ != libcamera::controls::AfStateFocused) {\n> > > > > > + afCoarseScan();\n> > > > > > + afFineScan();\n> > > > > > + } else {\n> > > > > > + /* We can re-start the scan at any moment in AfModeContinuous */\n> > > > > > + if (mode_ == libcamera::controls::AfModeContinuous)\n> > > > > > + if (afIsOutOfFocus())\n> > > > > > + afReset();\n> > > > > > + }\n> > > > > > +\n> > > > > > + return lensPosition_;\n> > > > >\n> > > > > Let me recap one point. We are still missing a generic definition for\n> > > > > the lens position value. As you can see we use\n> > > > > V4L2_CID_FOCUS_ABSOLUTE which transports the actual value to be set in\n> > > > > the v4l2 control. This can't work here and we are planning to remove\n> > > > > V4L2 controls from the IPA in general, but while for other controls we\n> > > > > have defined generic ranges, for the lens position we are still\n> > > > > missing a way to compute a \"generic\" value in the IPA, send it to the\n> > > > > pipeline handler by using libcamera::controls::LensPosition and have\n> > > > > the CameraLens class translate it to the device-specific value.\n> > > > >\n> > > > > I'm a bit lazy and I'm not chasing how lensPosition_ is here computed,\n> > > > > but have you considered how this could be generalized already ? As an\n> > > > > example, it seems to me the values I get are in the 0-180 range, while\n> > > > > in example the lens I'm using ranges from 0-4096...\n> >\n> > Currently, there is one hard-coded range of (0-1023). I was thinking that\n> > We could specify the range in the algorithm tuning file as these files\n> > are specific for camera model, but this would require manual\n> > configuration for each camera.\n>\n>\n> Before, I had submitted the patch to get the maximum steps of the lens\n> but not get merged and stopped at v3.\n> https://patchwork.libcamera.org/project/libcamera/list/?series=3069\n>\n> This patch can set the vcm range automatically. If this is helpful for\n> setting vcm range, we could carry on working on this.\n>\n> Thank you :)\n>\n\nYeah, I recall that series but I think the direction shouldn't be\npassing the range to the IPA. We should define a generic range for the\nIPA and pass the generic value to the CameraLens class, which will\nthen map the generic value onto the VCM actual value range.\n\nBut..\n\n>\n> >\n> > I have seen some discussions here about calculation of the hyperfocale\n> > and basing the range on that, but this would require additional tests\n> > and measurement. The simplest solution I can think about is just to\n> > normalize the values specific for different lens to one general range,\n> > for example (0.00 - 1.00).\n\nDefining such range, as per the current LensPosition control\ndefinition, requires knowing the hyperfocal distance, so that\napplication can combine the two (lens position and hyperfocal distance)\nto know at what distance to focus.\n\nEstimating the hyperfocal distance is not simple unfortunately, unless\nthe lens+vcm has this information already measured (I can't tell how\nfrequent is that) and a simpler range like you proposed it's certainly\neasier but being it an absolute value doesn't give the application any\ninformation about the current focus plane distance.\n\nI won't be against making a simpler implementation first which can be\nused by uncalibrated lenses. What do others think ?\n\n> >\n> > Best regards\n> > Daniel\n> >\n> > > > >\n> > > > > > + }\n> > > > > > +\n> > > > > > +private:\n> > > > > > + void afCoarseScan()\n> > > > > > + {\n> > > > > > + if (coarseCompleted_)\n> > > > > > + return;\n> > > > > > +\n> > > > > > + if (afScan(kCoarseSearchStep)) {\n> > > > > > + coarseCompleted_ = true;\n> > > > > > + maxContrast_ = 0;\n> > > > > > + lensPosition_ = lensPosition_ - (lensPosition_ * kFineRange);\n> > > > > > + previousContrast_ = 0;\n> > > > > > + maxStep_ = std::clamp(lensPosition_ + static_cast((lensPosition_ * kFineRange)),\n> > > > > > + 0U, highStep_);\n> > > > > > + }\n> > > > > > + }\n> > > > > > +\n> > > > > > + void afFineScan()\n> > > > > > + {\n> > > > > > + if (!coarseCompleted_)\n> > > > > > + return;\n> > > > > > +\n> > > > > > + if (afScan(kFineSearchStep)) {\n> > > > > > + LOG(Af, Debug) << \"AF found the best focus position !\";\n> > > > > > + state_ = libcamera::controls::AfStateFocused;\n> > > > > > + fineCompleted_ = true;\n> > > > > > + }\n> > > > > > + }\n> > > > > > +\n> > > > > > + bool afScan(uint32_t minSteps)\n> > > > > > + {\n> > > > > > + if (lensPosition_ + minSteps > maxStep_) {\n> > > > > > + /* If the max step is reached, move lens to the position. */\n> > > > > > + lensPosition_ = bestPosition_;\n> > > > > > + return true;\n> > > > > > + } else {\n> > > > > > + /*\n> > > > > > + * Find the maximum of the variance by estimating its\n> > > > > > + * derivative. If the direction changes, it means we have passed\n> > > > > > + * a maximum one step before.\n> > > > > > + */\n> > > > > > + if ((currentContrast_ - maxContrast_) >= -(maxContrast_ * 0.1)) {\n> > > > > > + /*\n> > > > > > + * Positive and zero derivative:\n> > > > > > + * The variance is still increasing. The focus could be\n> > > > > > + * increased for the next comparison. Also, the max\n> > > > > > + * variance and previous focus value are updated.\n> > > > > > + */\n> > > > > > + bestPosition_ = lensPosition_;\n> > > > > > + lensPosition_ += minSteps;\n> > > > > > + maxContrast_ = currentContrast_;\n> > > > > > + } else {\n> > > > > > + /*\n> > > > > > + * Negative derivative:\n> > > > > > + * The variance starts to decrease which means the maximum\n> > > > > > + * variance is found. Set focus step to previous good one\n> > > > > > + * then return immediately.\n> > > > > > + */\n> > > > > > + lensPosition_ = bestPosition_;\n> > > > > > + return true;\n> > > > > > + }\n> > > > > > + }\n> > > > > > +\n> > > > > > + previousContrast_ = currentContrast_;\n> > > > > > + LOG(Af, Debug) << \" Previous step is \" << bestPosition_\n> > > > > > + << \" Current step is \" << lensPosition_;\n> > > > > > + return false;\n> > > > > > + }\n> > > > > > +\n> > > > > > + void afReset()\n> > > > > > + {\n> > > > > > + LOG(Af, Debug) << \"Reset AF parameters\";\n> > > > > > + lensPosition_ = lowStep_;\n> > > > > > + maxStep_ = highStep_;\n> > > > > > + state_ = libcamera::controls::AfStateScanning;\n> > > > > > + previousContrast_ = 0.0;\n> > > > > > + coarseCompleted_ = false;\n> > > > > > + fineCompleted_ = false;\n> > > > > > + maxContrast_ = 0.0;\n> > > > > > + }\n> > > > > > +\n> > > > > > + bool afIsOutOfFocus()\n> > > > > > + {\n> > > > > > + const uint32_t diff_var = std::abs(currentContrast_ -\n> > > > > > + maxContrast_);\n> > > > > > + const double var_ratio = diff_var / maxContrast_;\n> > > > > > + LOG(Af, Debug) << \"Variance change rate: \" << var_ratio\n> > > > > > + << \" Current VCM step: \" << lensPosition_;\n> > > > > > + if (var_ratio > kMaxChange)\n> > > > > > + return true;\n> > > > > > + else\n> > > > > > + return false;\n> > > > > > + }\n> > > > > > +\n> > > > > > + controls::AfModeEnum mode_;\n> > > > > > + controls::AfStateEnum state_;\n> > > > > > + controls::AfPauseStateEnum pauseState_;\n> > > > > > +\n> > > > > > + /* VCM step configuration. It is the current setting of the VCM step. */\n> > > > > > + uint32_t lensPosition_;\n> > > > > > + /* The best VCM step. It is a local optimum VCM step during scanning. */\n> > > > > > + uint32_t bestPosition_;\n> > > > > > +\n> > > > > > + /* Current AF statistic contrast. */\n> > > > > > + double currentContrast_;\n> > > > > > + /* It is used to determine the derivative during scanning */\n> > > > > > + double previousContrast_;\n> > > > > > + double maxContrast_;\n> > > > > > + /* The designated maximum range of focus scanning. */\n> > > > > > + uint32_t maxStep_;\n> > > > > > + /* If the coarse scan completes, it is set to true. */\n> > > > > > + bool coarseCompleted_;\n> > > > > > + /* If the fine scan completes, it is set to true. */\n> > > > > > + bool fineCompleted_;\n> > > > > > +\n> > > > > > + uint32_t lowStep_;\n> > > > > > + uint32_t highStep_;\n> > > > > > +\n> > > > > > + /*\n> > > > > > + * Maximum focus steps of the VCM control\n> > > > > > + * \\todo should be obtained from the VCM driver\n> > > > > > + */\n> > > > > > + static constexpr uint32_t kMaxFocusSteps = 1023;\n> > > > > > +\n> > > > > > + /* Minimum focus step for searching appropriate focus */\n> > > > > > + static constexpr uint32_t kCoarseSearchStep = 30;\n> > > > > > + static constexpr uint32_t kFineSearchStep = 1;\n> > > > > > +\n> > > > > > + /* Max ratio of variance change, 0.0 < kMaxChange < 1.0 */\n> > > > > > + static constexpr double kMaxChange = 0.5;\n> > > > > > +\n> > > > > > + /* Fine scan range 0 < kFineRange < 1 */\n> > > > > > + static constexpr double kFineRange = 0.05;\n> > > > > > +};\n> > > > > > +\n> > > > > > +} /* namespace ipa::common::algorithms */\n> > > > > > +} /* namespace libcamera */\n> > > > > > diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> > > > > > index ab8da13a..860dc199 100644\n> > > > > > --- a/src/ipa/libipa/algorithms/meson.build\n> > > > > > +++ b/src/ipa/libipa/algorithms/meson.build\n> > > > > > @@ -2,8 +2,10 @@\n> > > > > >\n> > > > > > common_ipa_algorithms_headers = files([\n> > > > > > 'af_algorithm.h',\n> > > > > > + 'af_hill_climbing.h',\n> > > > > > ])\n> > > > > >\n> > > > > > common_ipa_algorithms_sources = files([\n> > > > > > 'af_algorithm.cpp',\n> > > > > > + 'af_hill_climbing.cpp',\n> > > > > > ])\n> > > > > > --\n> > > > > > 2.34.1\n> > > > > >\n> >\n>\n>\n> --\n> BR,\n> Kate\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 D62DBBD1F1\n\tfor ;\n\tTue, 19 Jul 2022 07:58:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 870116331B;\n\tTue, 19 Jul 2022 09:58:36 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::225])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6A95063312\n\tfor ;\n\tTue, 19 Jul 2022 09:58:35 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 2747B1C0015;\n\tTue, 19 Jul 2022 07:58:33 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658217516;\n\tbh=6Jp3nts+GTgNdn3enYTtpvW1bLb6xE7CoOsAyLLU/eo=;\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=dGdm7SJW1oH30S31nSVPMzm4JuhVAEfXVX9IvBJX9nKzzhns7boWQw5f1AIdQnJ8g\n\tM93AXWzduqk31XmLrXZ6nXyl9pcPWBbKM2Bm9hEfJEsNs8xtemPXY5ra22dlGR25MG\n\tscyncWC5mES3ebwuMU9SATy8RozrUaCpFZlEkSaXnjbaTj4B1j2LsG2CXRjr5+8Xpg\n\tVtiqZ94R9/ClzunmEIZZvDq1zfQsoRnHDPLs/6AXK+M092AADzGnizCR3CCoW/jVCV\n\tlmNaTJXS5FE7MRuyftXUbjVBYhANwvjeKRl0LfIFYyqdjuhSiyx65BsP0H67XsXDBr\n\tYWLjGDqQgkBZA==","Date":"Tue, 19 Jul 2022 09:58:32 +0200","To":"Kate Hsuan ","Message-ID":"<20220719075832.lzw7gazpryc5reel@uno.localdomain>","References":"<20220713084317.24268-1-dse@thaumatec.com>\n\t<20220713084317.24268-3-dse@thaumatec.com>\n\t<20220714161723.2w6p6c6odeapcsfq@uno.localdomain>\n\t<20220714163324.vjiqxhrkqkvuqil3@uno.localdomain>\n\t<20220715065232.ycxow3pou7nzvasy@uno.localdomain>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 02/11] ipa: Add class that\n\timplements base AF control algorithm","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23958,"web_url":"https://patchwork.libcamera.org/comment/23958/","msgid":"<20220719091313.bkel2aqoxnmwxohf@uno.localdomain>","date":"2022-07-19T09:13:13","subject":"Re: [libcamera-devel] [PATCH v2 02/11] ipa: Add class that\n\timplements base AF control algorithm","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kate\n\nOn Tue, Jul 19, 2022 at 08:17:21AM +0800, Kate Hsuan wrote:\n> Hi,\n>\n> On Mon, Jul 18, 2022 at 11:28 PM Daniel Semkowicz wrote:\n> >\n> > Hi Jacopo,\n> >\n> > On Fri, Jul 15, 2022 at 8:52 AM Jacopo Mondi wrote:\n> > >\n> > > One day I'll lear to use email\n> > >\n> > > On Thu, Jul 14, 2022 at 06:33:24PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > > Kate in cc for real this time!\n> > >\n> > > for real\n> > >\n> > > >\n> > > > On Thu, Jul 14, 2022 at 06:17:23PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > > > Hi Daniel\n> > > > >\n> > > > > On Wed, Jul 13, 2022 at 10:43:08AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > > Move the code that was common for IPU3 and RPi AF algorithms to\n> > > > > > a separate class independent of platform specific code.\n> > > > > > This way each platform can just implement contrast calculation and\n> > > > > > run the AF control loop basing on this class.\n> > > > > >\n> > > > >\n> > > > > This is exactly the purpose of having algorithms in libipa. I'm\n> > > > > excited it's the first \"generic\" one we have, or that I know of at\n> > > > > least!\n> > > > >\n> > > > > This mean the very IPU3 implementation this algorithm is based on\n> > > > > (Kate in cc) can now be generalized, and the same for the RPi one we\n> > > > > have on the list!\n> > > > >\n\nWhat do you think about this point ? Is porting the IPU3\nimplementation to use the common base class something that makes sense\nin your opinion ?\n\nThanks\n j\n\n> > > > > > Signed-off-by: Daniel Semkowicz \n> > > > > > ---\n> > > > > > .../libipa/algorithms/af_hill_climbing.cpp | 89 +++++++\n> > > > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 251 ++++++++++++++++++\n> > > > > > src/ipa/libipa/algorithms/meson.build | 2 +\n> > > > > > 3 files changed, 342 insertions(+)\n> > > > > > create mode 100644 src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > create mode 100644 src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > >\n> > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > new file mode 100644\n> > > > > > index 00000000..f666c6c2\n> > > > > > --- /dev/null\n> > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > @@ -0,0 +1,89 @@\n> > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > > +/*\n> > > > > > + * Copyright (C) 2021, Red Hat\n> > > > > > + * Copyright (C) 2022, Ideas On Board\n> > > > > > + * Copyright (C) 2022, Theobroma Systems\n> > > > > > + *\n> > > > > > + * af_hill_climbing.cpp - AF Hill Climbing common algorithm\n> > > > > > + */\n> > > > >\n> > > > > Any particular reason why the implementation is in the .h file ?\n> >\n> > In my original approach I had to pass the Module template argument, so\n> > using the template, forced me to do the implementation in a header file.\n> >\n> > This is an additional point in favor of the approach with multiple\n> > inheritence proposed by you in 01, as We can hide the implementation of\n> > the AfHillClimbing in .cpp file.\n> >\n> > > > >\n> > > > > > +\n> > > > > > +#include \"af_hill_climbing.h\"\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\file af_hill_climbing.h\n> > > > > > + * \\brief AF Hill Climbing common algorithm\n> > > > > > + */\n> > > > > > +\n> > > > > > +namespace libcamera {\n> > > > > > +\n> > > > > > +LOG_DEFINE_CATEGORY(Af)\n> > > > > > +\n> > > > > > +namespace ipa::common::algorithms {\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\class AfHillClimbing\n> > > > > > + * \\brief The base class implementing hill climbing AF control algorithm\n> > > > > > + * \\tparam Module The IPA module type for this class of algorithms\n> > > > > > + *\n> > > > > > + * Control part of auto focus algorithm. It calculates the lens position basing\n> > > > > > + * on contrast measure supplied by the higher level. This way it is independent\n> > > > > > + * from the platform.\n> > > > > > + *\n> > > > > > + * Derived class should call processAutofocus() for each measured contrast value\n> > > > > > + * and set the lens to the calculated position.\n> > > > > > + */\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\fn AfHillClimbing::setMode()\n> > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setMode\n> > > > > > + */\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\fn AfHillClimbing::setRange()\n> > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setRange\n> > > > > > + */\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\fn AfHillClimbing::setSpeed()\n> > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setSpeed\n> > > > > > + */\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\fn AfHillClimbing::setMetering()\n> > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setMetering\n> > > > > > + */\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\fn AfHillClimbing::setWindows()\n> > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setWindows\n> > > > > > + */\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\fn AfHillClimbing::setTrigger()\n> > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setTrigger\n> > > > > > + */\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\fn AfHillClimbing::setPause()\n> > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setPause\n> > > > > > + */\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\fn AfHillClimbing::setLensPosition()\n> > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setLensPosition\n> > > > > > + */\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\fn AfHillClimbing::processAutofocus()\n> > > > > > + * \\brief Run the auto focus algorithm loop\n> > > > > > + * \\param[in] currentContrast New value of contrast measured for current frame\n> > > > > > + *\n> > > > > > + * This method should be called for each new contrast value that was measured,\n> > > > > > + * usually in the process() method.\n> > > > > > + *\n> > > > > > + * \\return New lens position calculated by AF algorithm\n> > > > > > + */\n> > > > > > +\n> > > > > > +} /* namespace ipa::common::algorithms */\n> > > > > > +\n> > > > > > +} /* namespace libcamera */\n> > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > new file mode 100644\n> > > > > > index 00000000..db9fc058\n> > > > > > --- /dev/null\n> > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > @@ -0,0 +1,251 @@\n> > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > > +/*\n> > > > > > + * Copyright (C) 2021, Red Hat\n> > > > > > + * Copyright (C) 2022, Ideas On Board\n> > > > > > + * Copyright (C) 2022, Theobroma Systems\n> > > > > > + *\n> > > > > > + * af_hill_climbing.h - AF Hill Climbing common algorithm\n> > > > > > + */\n> > > > > > +\n> > > > > > +#pragma once\n> > > > > > +\n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \"af_algorithm.h\"\n> > > > > > +\n> > > > > > +namespace libcamera {\n> > > > > > +\n> > > > > > +LOG_DECLARE_CATEGORY(Af)\n> > > > > > +\n> > > > > > +namespace ipa::common::algorithms {\n> > > > > > +\n> > > > > > +template\n> > > > > > +class AfHillClimbing : public AfAlgorithm\n> > > > > > +{\n> > > > > > +public:\n> > > > > > + AfHillClimbing()\n> > > > > > + : mode_(controls::AfModeAuto), state_(controls::AfStateIdle),\n> > > > > > + pauseState_(controls::AfPauseStateRunning),\n> > > > > > + lensPosition_(0), bestPosition_(0), currentContrast_(0.0),\n> > > > > > + previousContrast_(0.0), maxContrast_(0.0), maxStep_(0),\n> > > > > > + coarseCompleted_(false), fineCompleted_(false),\n> > > > > > + lowStep_(0), highStep_(kMaxFocusSteps)\n> > > > > > + {\n> > > > > > + }\n> > > > >\n> > > > > Let's look at the implementation details later but I have one question\n> > > > >\n> > > > > > +\n> > > > > > + virtual ~AfHillClimbing() {}\n> > > > > > +\n> > > > > > + void setMode(controls::AfModeEnum mode) final\n> > > > > > + {\n> > > > > > + if (mode != mode_) {\n> > > > > > + LOG(Af, Debug) << \"Switched AF mode from \" << mode_\n> > > > > > + << \" to \" << mode;\n> > > > > > + pauseState_ = libcamera::controls::AfPauseStateRunning;\n> > > > > > + mode_ = mode;\n> > > > > > + }\n> > > > > > + }\n> > > > > > +\n> > > > > > + void setRange([[maybe_unused]] controls::AfRangeEnum range) final\n> > > > > > + {\n> > > > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > > > + }\n> > > > > > +\n> > > > > > + void setSpeed([[maybe_unused]] controls::AfSpeedEnum speed) final\n> > > > > > + {\n> > > > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > > > + }\n> > > > > > +\n> > > > > > + void setTrigger(controls::AfTriggerEnum trigger) final\n> > > > > > + {\n> > > > > > + LOG(Af, Debug) << \"Trigger called in mode \" << mode_\n> > > > > > + << \" with \" << trigger;\n> > > > > > + if (mode_ == libcamera::controls::AfModeAuto) {\n> > > > > > + if (trigger == libcamera::controls::AfTriggerStart)\n> > > > > > + afReset();\n> > > > >\n> > > > > This seems out of place..\n> > > > > Whenever I trigger an autofocus scan I get the lens reset to 0 causing\n> > > > > a not very nice effect. Why are you resetting the lens position here?\n> >\n> > This was the easiest solution to avoid additional problems in initial\n> > implementation. And this one is also done this way in the original\n> > implementation for IPU3. We are looking for the local peak value\n> > which for simple scenes is also the global peak, but it is not\n> > guaranteed. It is easier and safer to always start focus from the point\n> > beyond the hyperfocale and go closer and closer until the peak. When We\n> > start scanning from the current position, then We have additional problems.\n> > For example, which direction of scanning to choose and when to decide that\n> > the opposite direction will be better, and We need to move back.\n> > I am pretty sure We can improve this implementation, but this is\n> > something We can do gradually :)\n> >\n> > > > >\n> > > > > > + else\n> > > > > > + state_ = libcamera::controls::AfStateIdle;\n> > > > > > + }\n> > > > > > + }\n> > > > > > +\n> > > > > > + void setPause(controls::AfPauseEnum pause) final\n> > > > > > + {\n> > > > > > + /* \\todo: add the AfPauseDeferred mode */\n> > > > > > + if (mode_ == libcamera::controls::AfModeContinuous) {\n> > > > > > + if (pause == libcamera::controls::AfPauseImmediate)\n> > > > > > + pauseState_ = libcamera::controls::AfPauseStatePaused;\n> > > > > > + else if (pause == libcamera::controls::AfPauseResume)\n> > > > > > + pauseState_ = libcamera::controls::AfPauseStateRunning;\n> > > > > > + }\n> > > > > > + }\n> > > > > > +\n> > > > > > + void setLensPosition([[maybe_unused]] float lensPosition) final\n> > > > > > + {\n> > > > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > > > + }\n> > > > > > +\n> > > > > > + /* These methods should be implemented by derived class */\n> > > > > > + virtual void setMetering(controls::AfMeteringEnum metering) = 0;\n> > > > > > + virtual void setWindows(Span windows) = 0;\n> > > > > > +\n> > > > > > +protected:\n> > > > > > + uint32_t processAutofocus(double currentContrast)\n> > > > > > + {\n> > > > > > + currentContrast_ = currentContrast;\n> > > > > > +\n> > > > > > + /* If we are in a paused state, we won't process the stats */\n> > > > > > + if (pauseState_ == libcamera::controls::AfPauseStatePaused)\n> > > > > > + return lensPosition_;\n> > > > > > +\n> > > > > > + /* Depending on the mode, we may or may not process the stats */\n> > > > > > + if (state_ == libcamera::controls::AfStateIdle)\n> > > > > > + return lensPosition_;\n> > > > > > +\n> > > > > > + if (state_ != libcamera::controls::AfStateFocused) {\n> > > > > > + afCoarseScan();\n> > > > > > + afFineScan();\n> > > > > > + } else {\n> > > > > > + /* We can re-start the scan at any moment in AfModeContinuous */\n> > > > > > + if (mode_ == libcamera::controls::AfModeContinuous)\n> > > > > > + if (afIsOutOfFocus())\n> > > > > > + afReset();\n> > > > > > + }\n> > > > > > +\n> > > > > > + return lensPosition_;\n> > > > >\n> > > > > Let me recap one point. We are still missing a generic definition for\n> > > > > the lens position value. As you can see we use\n> > > > > V4L2_CID_FOCUS_ABSOLUTE which transports the actual value to be set in\n> > > > > the v4l2 control. This can't work here and we are planning to remove\n> > > > > V4L2 controls from the IPA in general, but while for other controls we\n> > > > > have defined generic ranges, for the lens position we are still\n> > > > > missing a way to compute a \"generic\" value in the IPA, send it to the\n> > > > > pipeline handler by using libcamera::controls::LensPosition and have\n> > > > > the CameraLens class translate it to the device-specific value.\n> > > > >\n> > > > > I'm a bit lazy and I'm not chasing how lensPosition_ is here computed,\n> > > > > but have you considered how this could be generalized already ? As an\n> > > > > example, it seems to me the values I get are in the 0-180 range, while\n> > > > > in example the lens I'm using ranges from 0-4096...\n> >\n> > Currently, there is one hard-coded range of (0-1023). I was thinking that\n> > We could specify the range in the algorithm tuning file as these files\n> > are specific for camera model, but this would require manual\n> > configuration for each camera.\n>\n>\n> Before, I had submitted the patch to get the maximum steps of the lens\n> but not get merged and stopped at v3.\n> https://patchwork.libcamera.org/project/libcamera/list/?series=3069\n>\n> This patch can set the vcm range automatically. If this is helpful for\n> setting vcm range, we could carry on working on this.\n>\n> Thank you :)\n>\n>\n> >\n> > I have seen some discussions here about calculation of the hyperfocale\n> > and basing the range on that, but this would require additional tests\n> > and measurement. The simplest solution I can think about is just to\n> > normalize the values specific for different lens to one general range,\n> > for example (0.00 - 1.00).\n> >\n> > Best regards\n> > Daniel\n> >\n> > > > >\n> > > > > > + }\n> > > > > > +\n> > > > > > +private:\n> > > > > > + void afCoarseScan()\n> > > > > > + {\n> > > > > > + if (coarseCompleted_)\n> > > > > > + return;\n> > > > > > +\n> > > > > > + if (afScan(kCoarseSearchStep)) {\n> > > > > > + coarseCompleted_ = true;\n> > > > > > + maxContrast_ = 0;\n> > > > > > + lensPosition_ = lensPosition_ - (lensPosition_ * kFineRange);\n> > > > > > + previousContrast_ = 0;\n> > > > > > + maxStep_ = std::clamp(lensPosition_ + static_cast((lensPosition_ * kFineRange)),\n> > > > > > + 0U, highStep_);\n> > > > > > + }\n> > > > > > + }\n> > > > > > +\n> > > > > > + void afFineScan()\n> > > > > > + {\n> > > > > > + if (!coarseCompleted_)\n> > > > > > + return;\n> > > > > > +\n> > > > > > + if (afScan(kFineSearchStep)) {\n> > > > > > + LOG(Af, Debug) << \"AF found the best focus position !\";\n> > > > > > + state_ = libcamera::controls::AfStateFocused;\n> > > > > > + fineCompleted_ = true;\n> > > > > > + }\n> > > > > > + }\n> > > > > > +\n> > > > > > + bool afScan(uint32_t minSteps)\n> > > > > > + {\n> > > > > > + if (lensPosition_ + minSteps > maxStep_) {\n> > > > > > + /* If the max step is reached, move lens to the position. */\n> > > > > > + lensPosition_ = bestPosition_;\n> > > > > > + return true;\n> > > > > > + } else {\n> > > > > > + /*\n> > > > > > + * Find the maximum of the variance by estimating its\n> > > > > > + * derivative. If the direction changes, it means we have passed\n> > > > > > + * a maximum one step before.\n> > > > > > + */\n> > > > > > + if ((currentContrast_ - maxContrast_) >= -(maxContrast_ * 0.1)) {\n> > > > > > + /*\n> > > > > > + * Positive and zero derivative:\n> > > > > > + * The variance is still increasing. The focus could be\n> > > > > > + * increased for the next comparison. Also, the max\n> > > > > > + * variance and previous focus value are updated.\n> > > > > > + */\n> > > > > > + bestPosition_ = lensPosition_;\n> > > > > > + lensPosition_ += minSteps;\n> > > > > > + maxContrast_ = currentContrast_;\n> > > > > > + } else {\n> > > > > > + /*\n> > > > > > + * Negative derivative:\n> > > > > > + * The variance starts to decrease which means the maximum\n> > > > > > + * variance is found. Set focus step to previous good one\n> > > > > > + * then return immediately.\n> > > > > > + */\n> > > > > > + lensPosition_ = bestPosition_;\n> > > > > > + return true;\n> > > > > > + }\n> > > > > > + }\n> > > > > > +\n> > > > > > + previousContrast_ = currentContrast_;\n> > > > > > + LOG(Af, Debug) << \" Previous step is \" << bestPosition_\n> > > > > > + << \" Current step is \" << lensPosition_;\n> > > > > > + return false;\n> > > > > > + }\n> > > > > > +\n> > > > > > + void afReset()\n> > > > > > + {\n> > > > > > + LOG(Af, Debug) << \"Reset AF parameters\";\n> > > > > > + lensPosition_ = lowStep_;\n> > > > > > + maxStep_ = highStep_;\n> > > > > > + state_ = libcamera::controls::AfStateScanning;\n> > > > > > + previousContrast_ = 0.0;\n> > > > > > + coarseCompleted_ = false;\n> > > > > > + fineCompleted_ = false;\n> > > > > > + maxContrast_ = 0.0;\n> > > > > > + }\n> > > > > > +\n> > > > > > + bool afIsOutOfFocus()\n> > > > > > + {\n> > > > > > + const uint32_t diff_var = std::abs(currentContrast_ -\n> > > > > > + maxContrast_);\n> > > > > > + const double var_ratio = diff_var / maxContrast_;\n> > > > > > + LOG(Af, Debug) << \"Variance change rate: \" << var_ratio\n> > > > > > + << \" Current VCM step: \" << lensPosition_;\n> > > > > > + if (var_ratio > kMaxChange)\n> > > > > > + return true;\n> > > > > > + else\n> > > > > > + return false;\n> > > > > > + }\n> > > > > > +\n> > > > > > + controls::AfModeEnum mode_;\n> > > > > > + controls::AfStateEnum state_;\n> > > > > > + controls::AfPauseStateEnum pauseState_;\n> > > > > > +\n> > > > > > + /* VCM step configuration. It is the current setting of the VCM step. */\n> > > > > > + uint32_t lensPosition_;\n> > > > > > + /* The best VCM step. It is a local optimum VCM step during scanning. */\n> > > > > > + uint32_t bestPosition_;\n> > > > > > +\n> > > > > > + /* Current AF statistic contrast. */\n> > > > > > + double currentContrast_;\n> > > > > > + /* It is used to determine the derivative during scanning */\n> > > > > > + double previousContrast_;\n> > > > > > + double maxContrast_;\n> > > > > > + /* The designated maximum range of focus scanning. */\n> > > > > > + uint32_t maxStep_;\n> > > > > > + /* If the coarse scan completes, it is set to true. */\n> > > > > > + bool coarseCompleted_;\n> > > > > > + /* If the fine scan completes, it is set to true. */\n> > > > > > + bool fineCompleted_;\n> > > > > > +\n> > > > > > + uint32_t lowStep_;\n> > > > > > + uint32_t highStep_;\n> > > > > > +\n> > > > > > + /*\n> > > > > > + * Maximum focus steps of the VCM control\n> > > > > > + * \\todo should be obtained from the VCM driver\n> > > > > > + */\n> > > > > > + static constexpr uint32_t kMaxFocusSteps = 1023;\n> > > > > > +\n> > > > > > + /* Minimum focus step for searching appropriate focus */\n> > > > > > + static constexpr uint32_t kCoarseSearchStep = 30;\n> > > > > > + static constexpr uint32_t kFineSearchStep = 1;\n> > > > > > +\n> > > > > > + /* Max ratio of variance change, 0.0 < kMaxChange < 1.0 */\n> > > > > > + static constexpr double kMaxChange = 0.5;\n> > > > > > +\n> > > > > > + /* Fine scan range 0 < kFineRange < 1 */\n> > > > > > + static constexpr double kFineRange = 0.05;\n> > > > > > +};\n> > > > > > +\n> > > > > > +} /* namespace ipa::common::algorithms */\n> > > > > > +} /* namespace libcamera */\n> > > > > > diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> > > > > > index ab8da13a..860dc199 100644\n> > > > > > --- a/src/ipa/libipa/algorithms/meson.build\n> > > > > > +++ b/src/ipa/libipa/algorithms/meson.build\n> > > > > > @@ -2,8 +2,10 @@\n> > > > > >\n> > > > > > common_ipa_algorithms_headers = files([\n> > > > > > 'af_algorithm.h',\n> > > > > > + 'af_hill_climbing.h',\n> > > > > > ])\n> > > > > >\n> > > > > > common_ipa_algorithms_sources = files([\n> > > > > > 'af_algorithm.cpp',\n> > > > > > + 'af_hill_climbing.cpp',\n> > > > > > ])\n> > > > > > --\n> > > > > > 2.34.1\n> > > > > >\n> >\n>\n>\n> --\n> BR,\n> Kate\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 260AEBE173\n\tfor ;\n\tTue, 19 Jul 2022 09:13:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E14163312;\n\tTue, 19 Jul 2022 11:13:17 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 07D9C6330D\n\tfor ;\n\tTue, 19 Jul 2022 11:13:16 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 1257F20005;\n\tTue, 19 Jul 2022 09:13:14 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658221997;\n\tbh=1LZh8D9aTK9igck7tNODB8nmeIvfQJrBA8Ur/1kCqoA=;\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=g3cCvirPI8iahuhGBlQ6wY9tX0vn6eA7UPR+vLTaHdKJMcC2GmoVZyisxO04ehG2D\n\te1iWRNSShcBP/upr6b8iyZqZ5pP8J/1ayv8x+saADvfVV0W+qh3cBAQBHGLuh6L+g2\n\t3y4SYerfFB3nXyf1ojcnXnLSHJwjSE7hJ3eyvqhRHm5WjIHZEyCfIlWkLnvgip/a7l\n\ty8mqWgfF8mF/hkzEg74pprVJ7Fm5adlvT/HBwhnmjKCrReXQAOg6uQgCfLm6ybOGnS\n\tq5v6APKC5F488UMud6KD2rJ+7xbsZf3RgpZy+1TjXQ2qx+piKZWNnq65gnNCJAeKBg\n\t8ZIFmb/J4n/Rw==","Date":"Tue, 19 Jul 2022 11:13:13 +0200","To":"Kate Hsuan ","Message-ID":"<20220719091313.bkel2aqoxnmwxohf@uno.localdomain>","References":"<20220713084317.24268-1-dse@thaumatec.com>\n\t<20220713084317.24268-3-dse@thaumatec.com>\n\t<20220714161723.2w6p6c6odeapcsfq@uno.localdomain>\n\t<20220714163324.vjiqxhrkqkvuqil3@uno.localdomain>\n\t<20220715065232.ycxow3pou7nzvasy@uno.localdomain>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 02/11] ipa: Add class that\n\timplements base AF control algorithm","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23960,"web_url":"https://patchwork.libcamera.org/comment/23960/","msgid":"","date":"2022-07-19T09:37:09","subject":"Re: [libcamera-devel] [PATCH v2 02/11] ipa: Add class that\n\timplements base AF control algorithm","submitter":{"id":105,"url":"https://patchwork.libcamera.org/api/people/105/","name":"Kate Hsuan","email":"hpa@redhat.com"},"content":"Hi\n\nOn Tue, Jul 19, 2022 at 5:13 PM Jacopo Mondi wrote:\n>\n> Hi Kate\n>\n> On Tue, Jul 19, 2022 at 08:17:21AM +0800, Kate Hsuan wrote:\n> > Hi,\n> >\n> > On Mon, Jul 18, 2022 at 11:28 PM Daniel Semkowicz wrote:\n> > >\n> > > Hi Jacopo,\n> > >\n> > > On Fri, Jul 15, 2022 at 8:52 AM Jacopo Mondi wrote:\n> > > >\n> > > > One day I'll lear to use email\n> > > >\n> > > > On Thu, Jul 14, 2022 at 06:33:24PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > > > Kate in cc for real this time!\n> > > >\n> > > > for real\n> > > >\n> > > > >\n> > > > > On Thu, Jul 14, 2022 at 06:17:23PM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > > > > Hi Daniel\n> > > > > >\n> > > > > > On Wed, Jul 13, 2022 at 10:43:08AM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > > > Move the code that was common for IPU3 and RPi AF algorithms to\n> > > > > > > a separate class independent of platform specific code.\n> > > > > > > This way each platform can just implement contrast calculation and\n> > > > > > > run the AF control loop basing on this class.\n> > > > > > >\n> > > > > >\n> > > > > > This is exactly the purpose of having algorithms in libipa. I'm\n> > > > > > excited it's the first \"generic\" one we have, or that I know of at\n> > > > > > least!\n> > > > > >\n> > > > > > This mean the very IPU3 implementation this algorithm is based on\n> > > > > > (Kate in cc) can now be generalized, and the same for the RPi one we\n> > > > > > have on the list!\n> > > > > >\n>\n> What do you think about this point ? Is porting the IPU3\n> implementation to use the common base class something that makes sense\n> in your opinion ?\n\n\nIt is ok to me. Since there are several searching methods, such as\nhill-climbing, global seaching, and enhancement schemes for finding\nthe local maximum values to determine the \"Focus\".\nIf those searching methods could be implemented based on a common base\nclass, the developer could select one of the algorithms which is\nsuitable for their AF statistic data through a common interface.\nTherefore, we can use a common algorithm interface to process\nstatistic values rather than implementing, for example, three\nhill-climbing for three platforms.\nSo, I think it is good for the development :).\n\nThank you.\n\n>\n> Thanks\n> j\n>\n> > > > > > > Signed-off-by: Daniel Semkowicz \n> > > > > > > ---\n> > > > > > > .../libipa/algorithms/af_hill_climbing.cpp | 89 +++++++\n> > > > > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 251 ++++++++++++++++++\n> > > > > > > src/ipa/libipa/algorithms/meson.build | 2 +\n> > > > > > > 3 files changed, 342 insertions(+)\n> > > > > > > create mode 100644 src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > > create mode 100644 src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > >\n> > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > > new file mode 100644\n> > > > > > > index 00000000..f666c6c2\n> > > > > > > --- /dev/null\n> > > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > > > @@ -0,0 +1,89 @@\n> > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > > > +/*\n> > > > > > > + * Copyright (C) 2021, Red Hat\n> > > > > > > + * Copyright (C) 2022, Ideas On Board\n> > > > > > > + * Copyright (C) 2022, Theobroma Systems\n> > > > > > > + *\n> > > > > > > + * af_hill_climbing.cpp - AF Hill Climbing common algorithm\n> > > > > > > + */\n> > > > > >\n> > > > > > Any particular reason why the implementation is in the .h file ?\n> > >\n> > > In my original approach I had to pass the Module template argument, so\n> > > using the template, forced me to do the implementation in a header file.\n> > >\n> > > This is an additional point in favor of the approach with multiple\n> > > inheritence proposed by you in 01, as We can hide the implementation of\n> > > the AfHillClimbing in .cpp file.\n> > >\n> > > > > >\n> > > > > > > +\n> > > > > > > +#include \"af_hill_climbing.h\"\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\file af_hill_climbing.h\n> > > > > > > + * \\brief AF Hill Climbing common algorithm\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > +namespace libcamera {\n> > > > > > > +\n> > > > > > > +LOG_DEFINE_CATEGORY(Af)\n> > > > > > > +\n> > > > > > > +namespace ipa::common::algorithms {\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\class AfHillClimbing\n> > > > > > > + * \\brief The base class implementing hill climbing AF control algorithm\n> > > > > > > + * \\tparam Module The IPA module type for this class of algorithms\n> > > > > > > + *\n> > > > > > > + * Control part of auto focus algorithm. It calculates the lens position basing\n> > > > > > > + * on contrast measure supplied by the higher level. This way it is independent\n> > > > > > > + * from the platform.\n> > > > > > > + *\n> > > > > > > + * Derived class should call processAutofocus() for each measured contrast value\n> > > > > > > + * and set the lens to the calculated position.\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\fn AfHillClimbing::setMode()\n> > > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setMode\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\fn AfHillClimbing::setRange()\n> > > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setRange\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\fn AfHillClimbing::setSpeed()\n> > > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setSpeed\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\fn AfHillClimbing::setMetering()\n> > > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setMetering\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\fn AfHillClimbing::setWindows()\n> > > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setWindows\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\fn AfHillClimbing::setTrigger()\n> > > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setTrigger\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\fn AfHillClimbing::setPause()\n> > > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setPause\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\fn AfHillClimbing::setLensPosition()\n> > > > > > > + * \\copydoc libcamera::ipa::common::algorithms::AfAlgorithm::setLensPosition\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\fn AfHillClimbing::processAutofocus()\n> > > > > > > + * \\brief Run the auto focus algorithm loop\n> > > > > > > + * \\param[in] currentContrast New value of contrast measured for current frame\n> > > > > > > + *\n> > > > > > > + * This method should be called for each new contrast value that was measured,\n> > > > > > > + * usually in the process() method.\n> > > > > > > + *\n> > > > > > > + * \\return New lens position calculated by AF algorithm\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > +} /* namespace ipa::common::algorithms */\n> > > > > > > +\n> > > > > > > +} /* namespace libcamera */\n> > > > > > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > > new file mode 100644\n> > > > > > > index 00000000..db9fc058\n> > > > > > > --- /dev/null\n> > > > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > > > @@ -0,0 +1,251 @@\n> > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > > > +/*\n> > > > > > > + * Copyright (C) 2021, Red Hat\n> > > > > > > + * Copyright (C) 2022, Ideas On Board\n> > > > > > > + * Copyright (C) 2022, Theobroma Systems\n> > > > > > > + *\n> > > > > > > + * af_hill_climbing.h - AF Hill Climbing common algorithm\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > +#pragma once\n> > > > > > > +\n> > > > > > > +#include \n> > > > > > > +\n> > > > > > > +#include \"af_algorithm.h\"\n> > > > > > > +\n> > > > > > > +namespace libcamera {\n> > > > > > > +\n> > > > > > > +LOG_DECLARE_CATEGORY(Af)\n> > > > > > > +\n> > > > > > > +namespace ipa::common::algorithms {\n> > > > > > > +\n> > > > > > > +template\n> > > > > > > +class AfHillClimbing : public AfAlgorithm\n> > > > > > > +{\n> > > > > > > +public:\n> > > > > > > + AfHillClimbing()\n> > > > > > > + : mode_(controls::AfModeAuto), state_(controls::AfStateIdle),\n> > > > > > > + pauseState_(controls::AfPauseStateRunning),\n> > > > > > > + lensPosition_(0), bestPosition_(0), currentContrast_(0.0),\n> > > > > > > + previousContrast_(0.0), maxContrast_(0.0), maxStep_(0),\n> > > > > > > + coarseCompleted_(false), fineCompleted_(false),\n> > > > > > > + lowStep_(0), highStep_(kMaxFocusSteps)\n> > > > > > > + {\n> > > > > > > + }\n> > > > > >\n> > > > > > Let's look at the implementation details later but I have one question\n> > > > > >\n> > > > > > > +\n> > > > > > > + virtual ~AfHillClimbing() {}\n> > > > > > > +\n> > > > > > > + void setMode(controls::AfModeEnum mode) final\n> > > > > > > + {\n> > > > > > > + if (mode != mode_) {\n> > > > > > > + LOG(Af, Debug) << \"Switched AF mode from \" << mode_\n> > > > > > > + << \" to \" << mode;\n> > > > > > > + pauseState_ = libcamera::controls::AfPauseStateRunning;\n> > > > > > > + mode_ = mode;\n> > > > > > > + }\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + void setRange([[maybe_unused]] controls::AfRangeEnum range) final\n> > > > > > > + {\n> > > > > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + void setSpeed([[maybe_unused]] controls::AfSpeedEnum speed) final\n> > > > > > > + {\n> > > > > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + void setTrigger(controls::AfTriggerEnum trigger) final\n> > > > > > > + {\n> > > > > > > + LOG(Af, Debug) << \"Trigger called in mode \" << mode_\n> > > > > > > + << \" with \" << trigger;\n> > > > > > > + if (mode_ == libcamera::controls::AfModeAuto) {\n> > > > > > > + if (trigger == libcamera::controls::AfTriggerStart)\n> > > > > > > + afReset();\n> > > > > >\n> > > > > > This seems out of place..\n> > > > > > Whenever I trigger an autofocus scan I get the lens reset to 0 causing\n> > > > > > a not very nice effect. Why are you resetting the lens position here?\n> > >\n> > > This was the easiest solution to avoid additional problems in initial\n> > > implementation. And this one is also done this way in the original\n> > > implementation for IPU3. We are looking for the local peak value\n> > > which for simple scenes is also the global peak, but it is not\n> > > guaranteed. It is easier and safer to always start focus from the point\n> > > beyond the hyperfocale and go closer and closer until the peak. When We\n> > > start scanning from the current position, then We have additional problems.\n> > > For example, which direction of scanning to choose and when to decide that\n> > > the opposite direction will be better, and We need to move back.\n> > > I am pretty sure We can improve this implementation, but this is\n> > > something We can do gradually :)\n> > >\n> > > > > >\n> > > > > > > + else\n> > > > > > > + state_ = libcamera::controls::AfStateIdle;\n> > > > > > > + }\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + void setPause(controls::AfPauseEnum pause) final\n> > > > > > > + {\n> > > > > > > + /* \\todo: add the AfPauseDeferred mode */\n> > > > > > > + if (mode_ == libcamera::controls::AfModeContinuous) {\n> > > > > > > + if (pause == libcamera::controls::AfPauseImmediate)\n> > > > > > > + pauseState_ = libcamera::controls::AfPauseStatePaused;\n> > > > > > > + else if (pause == libcamera::controls::AfPauseResume)\n> > > > > > > + pauseState_ = libcamera::controls::AfPauseStateRunning;\n> > > > > > > + }\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + void setLensPosition([[maybe_unused]] float lensPosition) final\n> > > > > > > + {\n> > > > > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + /* These methods should be implemented by derived class */\n> > > > > > > + virtual void setMetering(controls::AfMeteringEnum metering) = 0;\n> > > > > > > + virtual void setWindows(Span windows) = 0;\n> > > > > > > +\n> > > > > > > +protected:\n> > > > > > > + uint32_t processAutofocus(double currentContrast)\n> > > > > > > + {\n> > > > > > > + currentContrast_ = currentContrast;\n> > > > > > > +\n> > > > > > > + /* If we are in a paused state, we won't process the stats */\n> > > > > > > + if (pauseState_ == libcamera::controls::AfPauseStatePaused)\n> > > > > > > + return lensPosition_;\n> > > > > > > +\n> > > > > > > + /* Depending on the mode, we may or may not process the stats */\n> > > > > > > + if (state_ == libcamera::controls::AfStateIdle)\n> > > > > > > + return lensPosition_;\n> > > > > > > +\n> > > > > > > + if (state_ != libcamera::controls::AfStateFocused) {\n> > > > > > > + afCoarseScan();\n> > > > > > > + afFineScan();\n> > > > > > > + } else {\n> > > > > > > + /* We can re-start the scan at any moment in AfModeContinuous */\n> > > > > > > + if (mode_ == libcamera::controls::AfModeContinuous)\n> > > > > > > + if (afIsOutOfFocus())\n> > > > > > > + afReset();\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + return lensPosition_;\n> > > > > >\n> > > > > > Let me recap one point. We are still missing a generic definition for\n> > > > > > the lens position value. As you can see we use\n> > > > > > V4L2_CID_FOCUS_ABSOLUTE which transports the actual value to be set in\n> > > > > > the v4l2 control. This can't work here and we are planning to remove\n> > > > > > V4L2 controls from the IPA in general, but while for other controls we\n> > > > > > have defined generic ranges, for the lens position we are still\n> > > > > > missing a way to compute a \"generic\" value in the IPA, send it to the\n> > > > > > pipeline handler by using libcamera::controls::LensPosition and have\n> > > > > > the CameraLens class translate it to the device-specific value.\n> > > > > >\n> > > > > > I'm a bit lazy and I'm not chasing how lensPosition_ is here computed,\n> > > > > > but have you considered how this could be generalized already ? As an\n> > > > > > example, it seems to me the values I get are in the 0-180 range, while\n> > > > > > in example the lens I'm using ranges from 0-4096...\n> > >\n> > > Currently, there is one hard-coded range of (0-1023). I was thinking that\n> > > We could specify the range in the algorithm tuning file as these files\n> > > are specific for camera model, but this would require manual\n> > > configuration for each camera.\n> >\n> >\n> > Before, I had submitted the patch to get the maximum steps of the lens\n> > but not get merged and stopped at v3.\n> > https://patchwork.libcamera.org/project/libcamera/list/?series=3069\n> >\n> > This patch can set the vcm range automatically. If this is helpful for\n> > setting vcm range, we could carry on working on this.\n> >\n> > Thank you :)\n> >\n> >\n> > >\n> > > I have seen some discussions here about calculation of the hyperfocale\n> > > and basing the range on that, but this would require additional tests\n> > > and measurement. The simplest solution I can think about is just to\n> > > normalize the values specific for different lens to one general range,\n> > > for example (0.00 - 1.00).\n> > >\n> > > Best regards\n> > > Daniel\n> > >\n> > > > > >\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > +private:\n> > > > > > > + void afCoarseScan()\n> > > > > > > + {\n> > > > > > > + if (coarseCompleted_)\n> > > > > > > + return;\n> > > > > > > +\n> > > > > > > + if (afScan(kCoarseSearchStep)) {\n> > > > > > > + coarseCompleted_ = true;\n> > > > > > > + maxContrast_ = 0;\n> > > > > > > + lensPosition_ = lensPosition_ - (lensPosition_ * kFineRange);\n> > > > > > > + previousContrast_ = 0;\n> > > > > > > + maxStep_ = std::clamp(lensPosition_ + static_cast((lensPosition_ * kFineRange)),\n> > > > > > > + 0U, highStep_);\n> > > > > > > + }\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + void afFineScan()\n> > > > > > > + {\n> > > > > > > + if (!coarseCompleted_)\n> > > > > > > + return;\n> > > > > > > +\n> > > > > > > + if (afScan(kFineSearchStep)) {\n> > > > > > > + LOG(Af, Debug) << \"AF found the best focus position !\";\n> > > > > > > + state_ = libcamera::controls::AfStateFocused;\n> > > > > > > + fineCompleted_ = true;\n> > > > > > > + }\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + bool afScan(uint32_t minSteps)\n> > > > > > > + {\n> > > > > > > + if (lensPosition_ + minSteps > maxStep_) {\n> > > > > > > + /* If the max step is reached, move lens to the position. */\n> > > > > > > + lensPosition_ = bestPosition_;\n> > > > > > > + return true;\n> > > > > > > + } else {\n> > > > > > > + /*\n> > > > > > > + * Find the maximum of the variance by estimating its\n> > > > > > > + * derivative. If the direction changes, it means we have passed\n> > > > > > > + * a maximum one step before.\n> > > > > > > + */\n> > > > > > > + if ((currentContrast_ - maxContrast_) >= -(maxContrast_ * 0.1)) {\n> > > > > > > + /*\n> > > > > > > + * Positive and zero derivative:\n> > > > > > > + * The variance is still increasing. The focus could be\n> > > > > > > + * increased for the next comparison. Also, the max\n> > > > > > > + * variance and previous focus value are updated.\n> > > > > > > + */\n> > > > > > > + bestPosition_ = lensPosition_;\n> > > > > > > + lensPosition_ += minSteps;\n> > > > > > > + maxContrast_ = currentContrast_;\n> > > > > > > + } else {\n> > > > > > > + /*\n> > > > > > > + * Negative derivative:\n> > > > > > > + * The variance starts to decrease which means the maximum\n> > > > > > > + * variance is found. Set focus step to previous good one\n> > > > > > > + * then return immediately.\n> > > > > > > + */\n> > > > > > > + lensPosition_ = bestPosition_;\n> > > > > > > + return true;\n> > > > > > > + }\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + previousContrast_ = currentContrast_;\n> > > > > > > + LOG(Af, Debug) << \" Previous step is \" << bestPosition_\n> > > > > > > + << \" Current step is \" << lensPosition_;\n> > > > > > > + return false;\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + void afReset()\n> > > > > > > + {\n> > > > > > > + LOG(Af, Debug) << \"Reset AF parameters\";\n> > > > > > > + lensPosition_ = lowStep_;\n> > > > > > > + maxStep_ = highStep_;\n> > > > > > > + state_ = libcamera::controls::AfStateScanning;\n> > > > > > > + previousContrast_ = 0.0;\n> > > > > > > + coarseCompleted_ = false;\n> > > > > > > + fineCompleted_ = false;\n> > > > > > > + maxContrast_ = 0.0;\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + bool afIsOutOfFocus()\n> > > > > > > + {\n> > > > > > > + const uint32_t diff_var = std::abs(currentContrast_ -\n> > > > > > > + maxContrast_);\n> > > > > > > + const double var_ratio = diff_var / maxContrast_;\n> > > > > > > + LOG(Af, Debug) << \"Variance change rate: \" << var_ratio\n> > > > > > > + << \" Current VCM step: \" << lensPosition_;\n> > > > > > > + if (var_ratio > kMaxChange)\n> > > > > > > + return true;\n> > > > > > > + else\n> > > > > > > + return false;\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + controls::AfModeEnum mode_;\n> > > > > > > + controls::AfStateEnum state_;\n> > > > > > > + controls::AfPauseStateEnum pauseState_;\n> > > > > > > +\n> > > > > > > + /* VCM step configuration. It is the current setting of the VCM step. */\n> > > > > > > + uint32_t lensPosition_;\n> > > > > > > + /* The best VCM step. It is a local optimum VCM step during scanning. */\n> > > > > > > + uint32_t bestPosition_;\n> > > > > > > +\n> > > > > > > + /* Current AF statistic contrast. */\n> > > > > > > + double currentContrast_;\n> > > > > > > + /* It is used to determine the derivative during scanning */\n> > > > > > > + double previousContrast_;\n> > > > > > > + double maxContrast_;\n> > > > > > > + /* The designated maximum range of focus scanning. */\n> > > > > > > + uint32_t maxStep_;\n> > > > > > > + /* If the coarse scan completes, it is set to true. */\n> > > > > > > + bool coarseCompleted_;\n> > > > > > > + /* If the fine scan completes, it is set to true. */\n> > > > > > > + bool fineCompleted_;\n> > > > > > > +\n> > > > > > > + uint32_t lowStep_;\n> > > > > > > + uint32_t highStep_;\n> > > > > > > +\n> > > > > > > + /*\n> > > > > > > + * Maximum focus steps of the VCM control\n> > > > > > > + * \\todo should be obtained from the VCM driver\n> > > > > > > + */\n> > > > > > > + static constexpr uint32_t kMaxFocusSteps = 1023;\n> > > > > > > +\n> > > > > > > + /* Minimum focus step for searching appropriate focus */\n> > > > > > > + static constexpr uint32_t kCoarseSearchStep = 30;\n> > > > > > > + static constexpr uint32_t kFineSearchStep = 1;\n> > > > > > > +\n> > > > > > > + /* Max ratio of variance change, 0.0 < kMaxChange < 1.0 */\n> > > > > > > + static constexpr double kMaxChange = 0.5;\n> > > > > > > +\n> > > > > > > + /* Fine scan range 0 < kFineRange < 1 */\n> > > > > > > + static constexpr double kFineRange = 0.05;\n> > > > > > > +};\n> > > > > > > +\n> > > > > > > +} /* namespace ipa::common::algorithms */\n> > > > > > > +} /* namespace libcamera */\n> > > > > > > diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> > > > > > > index ab8da13a..860dc199 100644\n> > > > > > > --- a/src/ipa/libipa/algorithms/meson.build\n> > > > > > > +++ b/src/ipa/libipa/algorithms/meson.build\n> > > > > > > @@ -2,8 +2,10 @@\n> > > > > > >\n> > > > > > > common_ipa_algorithms_headers = files([\n> > > > > > > 'af_algorithm.h',\n> > > > > > > + 'af_hill_climbing.h',\n> > > > > > > ])\n> > > > > > >\n> > > > > > > common_ipa_algorithms_sources = files([\n> > > > > > > 'af_algorithm.cpp',\n> > > > > > > + 'af_hill_climbing.cpp',\n> > > > > > > ])\n> > > > > > > --\n> > > > > > > 2.34.1\n> > > > > > >\n> > >\n> >\n> >\n> > --\n> > BR,\n> > Kate\n> >\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 86881BE173\n\tfor ;\n\tTue, 19 Jul 2022 09:37:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D03CD6330D;\n\tTue, 19 Jul 2022 11:37:25 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 14CBD6330D\n\tfor ;\n\tTue, 19 Jul 2022 11:37:23 +0200 (CEST)","from mail-yw1-f198.google.com (mail-yw1-f198.google.com\n\t[209.85.128.198]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id\n\tus-mta-50-5AYlsITCNxyPDFxTBVVGsQ-1; Tue, 19 Jul 2022 05:37:21 -0400","by mail-yw1-f198.google.com with SMTP id\n\t00721157ae682-2dc7bdd666fso114374947b3.7\n\tfor ;\n\tTue, 19 Jul 2022 02:37:21 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658223445;\n\tbh=Vp6UaUoxLtyudKnis1BcGqgNvyWtZkUTyWFwzizi7x4=;\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=TuI2kjbMjUQAO7h7B0LSFbLW3WV7Nb/Hps4j5vyFUNNmSHe9wti9Ng9+tFVJTI369\n\tA9yLJ+/dmcX+QMqaaZ6Dd72D0eOnzS5qUbyCiHn1j2B8J8AUbvOBMRyRv/EOVxXced\n\tKH6VEZqBlXKhg8BXkEg07eUl/J70B2oy19XJMv4I/KVU/fNN54jO/gLgspEZe1Ars4\n\tkNcx0gibW0FsOtdO/UVP09QetxhLRIzMfy6wO1ftvZOizF7zSK/ae6Mvask8D/tmwI\n\t1OM1bH3a4T2GydB9Ez0mDUXwnz/ZhnkxvQyJOg70Ln8CH+t9KrZIYDW5a5uutOuGcc\n\t5B1WBNvzhd9pg==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1658223442;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=txkxBuNbtjTN5hfIemBEW5JyYvR/Xk/WPq34shO9710=;\n\tb=BRA3Q1qUcW6matU8tu+rCWUdpTrdqIrql5JgsejzvInLCcIlKMuKKEbMlSRrqibD/nXplU\n\t+Qy/EBB/C2BeftjCztru/jyTzHrErdxArt46XWpsEQx6weG8h1bfAxlItplgUqG0CCP0/A\n\tG5RRdPLTctT9dSymHGhYTPjSoyB1Roc="],"Authentication-Results":["lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"BRA3Q1qU\"; \n\tdkim-atps=neutral","relay.mimecast.com;\n\tauth=pass smtp.auth=CUSA124A263 smtp.mailfrom=hpa@redhat.com"],"X-MC-Unique":"5AYlsITCNxyPDFxTBVVGsQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=txkxBuNbtjTN5hfIemBEW5JyYvR/Xk/WPq34shO9710=;\n\tb=hB8pIVhRH1jgSEL3TSfH1C/7j4/oQKFfB30YM6gtieQALCYXH8868K87clQgEpRxEZ\n\tZtlFoCj4anPcV7JRp5NbQTATq/jIDWGUTmN528fHCQ5FQuyMSAjys92ZOx60idLMyAZY\n\tKDCoQTfb98TP6E0K6nTc+A9zpdMXKGnOx5SBF9S0HkzczV+CiAG3ecQQX04WZP0tSEyp\n\tylxdmPtQCtG8Ym8Q8/XEJYKvYD/OPxZbhNqL63NyIaRPez+Ibme+e+7AcgBKGpqM1InR\n\tupviYd0PPplfZ8HyHIE3Cl6P8nu9TnhHyFTeAhVXQokvd6sLzvdCaAGDS2pke+9qVEQu\n\tiScw==","X-Gm-Message-State":"AJIora/x0r9dZwyF4290g8/+x0kBazoeSmw4wAFljyqtNbuH9vsDIBJ0\n\tnGnMWUfgwoZM72o3F38WlKHo7oDdEBR28tC3VLSYK5/RreDbpzBMrDa23fsCkHJD9RdukyDpaAK\n\tnvl20634rTHzSIon/BIUsc9wynUxmfcdFgF+VEuVpzQGmkgaVVg==","X-Received":["by 2002:a25:2d0a:0:b0:66e:9660:ca31 with SMTP id\n\tt10-20020a252d0a000000b0066e9660ca31mr33287328ybt.155.1658223440877; \n\tTue, 19 Jul 2022 02:37:20 -0700 (PDT)","by 2002:a25:2d0a:0:b0:66e:9660:ca31 with SMTP id\n\tt10-20020a252d0a000000b0066e9660ca31mr33287296ybt.155.1658223440387;\n\tTue, 19 Jul 2022 02:37:20 -0700 (PDT)"],"X-Google-Smtp-Source":"AGRyM1tnYTvgJU29jRWKh4pRf+QnsaWECjhV5gsdcKrbbb/pptODrd8dU9RfjAaj98dhur77mczB2Lxnh+iSy+ad0Ug=","MIME-Version":"1.0","References":"<20220713084317.24268-1-dse@thaumatec.com>\n\t<20220713084317.24268-3-dse@thaumatec.com>\n\t<20220714161723.2w6p6c6odeapcsfq@uno.localdomain>\n\t<20220714163324.vjiqxhrkqkvuqil3@uno.localdomain>\n\t<20220715065232.ycxow3pou7nzvasy@uno.localdomain>\n\t\n\t\n\t<20220719091313.bkel2aqoxnmwxohf@uno.localdomain>","In-Reply-To":"<20220719091313.bkel2aqoxnmwxohf@uno.localdomain>","Date":"Tue, 19 Jul 2022 17:37:09 +0800","Message-ID":"","To":"Jacopo Mondi ","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 02/11] ipa: Add class that\n\timplements base AF control algorithm","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":"Kate Hsuan via libcamera-devel ","Reply-To":"Kate Hsuan ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]