[{"id":26736,"web_url":"https://patchwork.libcamera.org/comment/26736/","msgid":"<20230324153040.sk43lwg36qmxwz6t@uno.localdomain>","date":"2023-03-24T15:30:40","subject":"Re: [libcamera-devel] [PATCH v5 04/10] ipa: Add common contrast\n\tbased AF implementation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Daniel\n\nOn Fri, Mar 24, 2023 at 03:29:02PM +0100, Daniel Semkowicz via libcamera-devel wrote:\n> Create a new class with contrast based Auto Focus implementation\n> using hill climbing algorithm. This common implementation is independent\n> of platform specific code. This way, each platform can just implement\n> contrast calculation and run the AF control loop basing on this class.\n> This implementation is based on the code that was common for IPU3\n> and RPi AF algorithms.\n>\n> Signed-off-by: Daniel Semkowicz \n> ---\n> .../libipa/algorithms/af_hill_climbing.cpp | 379 ++++++++++++++++++\n> src/ipa/libipa/algorithms/af_hill_climbing.h | 91 +++++\n> src/ipa/libipa/algorithms/meson.build | 2 +\n> 3 files changed, 472 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..6ee090eb\n> --- /dev/null\n> +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> @@ -0,0 +1,379 @@\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) 2023, Theobroma Systems\n> + *\n> + * af_hill_climbing.cpp - AF contrast based hill climbing common algorithm\n> + */\n> +\n> +#include \"af_hill_climbing.h\"\n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +/**\n> + * \\file af_hill_climbing.h\n> + * \\brief AF contrast based hill climbing common algorithm\n> + */\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::algorithms {\n> +\n> +LOG_DEFINE_CATEGORY(Af)\n> +\n> +/**\n> + * \\class AfHillClimbing\n> + * \\brief Contrast based hill climbing auto focus control algorithm\n> + * implementation\n> + *\n> + * Control part of the auto focus algorithm. It calculates the lens position\n> + * based on the contrast measure supplied by the platform-specific\n> + * implementation. This way it is independent from the platform.\n> + *\n> + * Platform layer that use this algorithm should call process() function\n> + * for each each frame and set the lens to the calculated position.\n> + *\n> + * Focus search is divided into two phases:\n> + * 1. coarse search,\n> + * 2. fine search.\n> + *\n> + * In the first phase, the lens is moved with bigger steps to quickly find\n> + * a rough position for the best focus. Then, based on the outcome of coarse\n> + * search, the second phase is executed. Lens is moved with smaller steps\n> + * in a limited range within the rough position to find the exact position\n> + * for best focus.\n> + *\n> + * Tuning file parameters:\n> + * - **coarse-search-step:** The value by which the lens position will change\n> + * in one step in the *coarse search* phase. Unit is lens specific.\n> + * - **fine-search-step:** The value by which the lens position will change\n> + * in one step in the *fine search* phase. Unit is lens specific.\n> + * - **fine-search-range:** Search range in the *fine search* phase, expressed\n> + * as a percentage of the coarse search result. Valid values are\n> + * in the [0, 100] interval. Value 5 means 5%. If coarse search stopped\n> + * at value 200, the fine search range will be [190, 210].\n> + * - **max-variance-change:** ratio of contrast variance change in the\n> + * *continuous mode* needed for triggering the focus change. When the variance\n> + * change exceeds this value, focus search will be triggered. Valid values are\n> + * in the [0.0, 1.0] interval.\n> + * .\n> + *\n> + * \\todo Search range in the *fine search* phase should depend on the lens\n> + * movement range rather than coarse search result.\n> + * \\todo Implement setRange.\n> + * \\todo Implement setSpeed.\n> + * \\todo Implement setMeteringMode.\n> + * \\todo Implement setWindows.\n> + * \\todo Implement the AfPauseDeferred mode.\n> + */\n> +\n> +/**\n> + * \\brief Initialize the AfHillClimbing with tuning data\n> + * \\param[in] tuningData The tuning data for the algorithm\n> + *\n> + * This method should be called in the libcamera::ipa::Algorithm::init()\n> + * method of the platform layer.\n> + *\n> + * \\return 0 if successful, an error code otherwise\n> + */\n> +int AfHillClimbing::init(const YamlObject &tuningData)\n> +{\n> +\tcoarseSearchStep_ = tuningData[\"coarse-search-step\"].get(30);\n> +\tfineSearchStep_ = tuningData[\"fine-search-step\"].get(1);\n> +\tfineRange_ = tuningData[\"fine-search-range\"].get(5);\n> +\tfineRange_ /= 100;\n> +\tmaxChange_ = tuningData[\"max-variance-change\"].get(0.5);\n> +\n> +\tLOG(Af, Debug) << \"coarseSearchStep_: \" << coarseSearchStep_\n> +\t\t << \", fineSearchStep_: \" << fineSearchStep_\n> +\t\t << \", fineRange_: \" << fineRange_\n> +\t\t << \", maxChange_: \" << maxChange_;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Configure the AfHillClimbing with sensor and lens information\n> + * \\param[in] minFocusPosition Minimum position supported by camera lens\n> + * \\param[in] maxFocusPosition Maximum position supported by camera lens\n> + *\n> + * This method should be called in the libcamera::ipa::Algorithm::configure()\n> + * method of the platform layer.\n> + */\n> +void AfHillClimbing::configure(int32_t minFocusPosition,\n> +\t\t\t int32_t maxFocusPosition)\n> +{\n> +\tminLensPosition_ = minFocusPosition;\n> +\tmaxLensPosition_ = maxFocusPosition;\n> +}\n> +\n> +/**\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 in the libcamera::ipa::Algorithm::process()\n> + * method of the platform layer for each frame.\n> + *\n> + * Contrast value supplied in the \\p currentContrast parameter can be platform\n> + * specific. The only requirement is the contrast value must increase with\n> + * the increasing image focus. Contrast value must be highest when image is in\n> + * focus.\n> + *\n> + * \\return New lens position calculated by the AF algorithm\n> + */\n> +int32_t AfHillClimbing::process(double currentContrast)\n> +{\n> +\tcurrentContrast_ = currentContrast;\n> +\n> +\tif (shouldSkipFrame())\n> +\t\treturn lensPosition_;\n> +\n> +\tswitch (mode_) {\n> +\tcase controls::AfModeManual:\n> +\t\t/* Nothing to process. */\n> +\t\tbreak;\n> +\tcase controls::AfModeAuto:\n> +\t\tprocessAutoMode();\n> +\t\tbreak;\n> +\tcase controls::AfModeContinuous:\n> +\t\tprocessContinuousMode();\n> +\t\tbreak;\n> +\t}\n> +\n> +\treturn lensPosition_;\n> +}\n> +\n> +void AfHillClimbing::processAutoMode()\n> +{\n> +\tif (state_ == controls::AfStateScanning) {\n> +\t\tafCoarseScan();\n> +\t\tafFineScan();\n> +\t}\n> +}\n> +\n> +void AfHillClimbing::processContinuousMode()\n> +{\n> +\t/* If we are in a paused state, we won't process the stats. */\n> +\tif (pauseState_ == controls::AfPauseStatePaused)\n> +\t\treturn;\n> +\n> +\tif (state_ == controls::AfStateScanning) {\n> +\t\tafCoarseScan();\n> +\t\tafFineScan();\n> +\t\treturn;\n> +\t}\n> +\n> +\t/*\n> +\t * AF scan can be started at any moment in AfModeContinuous,\n> +\t * except when the state is already AfStateScanning.\n> +\t */\n> +\tif (afIsOutOfFocus())\n> +\t\tafReset();\n> +}\n> +\n> +/**\n> + * \\brief Request AF to skip n frames\n> + * \\param[in] n Number of frames to be skipped\n> + *\n> + * For the requested number of frames, the AF calculation will be skipped\n> + * and lens position will not change. The platform layer still needs to\n> + * call process() function for each frame during this time.\n> + * This function can be used by the platform layer if the hardware needs more\n> + * time for some operations.\n> + *\n> + * The number of the requested frames (\\p n) will be applied only if \\p n has\n> + * higher value than the number of frames already requested to be skipped.\n> + * For example, if *setFramesToSkip(5)* was already called for the current\n> + * frame, then calling *setFramesToSkip(3)* will not override the previous\n> + * request and 5 frames will be skipped.\n\nThis still puzzles me a bit as it is not clear to me when the platform\nlayer is supposed to set the number of frames to skip.\n\nLooking at the implementation it happens everytime the lens is moved.\nI guess this is fine for now, but this will need to be handled better\nby the CameraLens class modeling the lens movement delay.\n\n> + */\n> +void AfHillClimbing::setFramesToSkip(uint32_t n)\n> +{\n> +\tif (n > framesToSkip_)\n> +\t\tframesToSkip_ = n;\n> +}\n> +\n> +void AfHillClimbing::setMode(controls::AfModeEnum mode)\n> +{\n> +\tif (mode == mode_)\n> +\t\treturn;\n> +\n> +\tLOG(Af, Debug) << \"Switched AF mode from \" << mode_ << \" to \" << mode;\n> +\tmode_ = mode;\n> +\n> +\tstate_ = controls::AfStateIdle;\n> +\tpauseState_ = controls::AfPauseStateRunning;\n> +\n> +\tif (mode_ == controls::AfModeContinuous)\n> +\t\tafReset();\n> +}\n> +\n> +void AfHillClimbing::setRange([[maybe_unused]] controls::AfRangeEnum range)\n> +{\n> +\tLOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> +}\n> +\n> +void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed)\n> +{\n> +\tLOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> +}\n> +\n> +void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering)\n> +{\n> +\tLOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> +}\n> +\n> +void AfHillClimbing::setWindows([[maybe_unused]] Span windows)\n> +{\n> +\tLOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> +}\n> +\n\nI'm still of the idea you should better not implement these functions\nat all. If anything tries to call into them will fail at compile time\ninstead of getting a run-time error in the logs\n\n> +void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)\n> +{\n> +\tif (mode_ != controls::AfModeAuto) {\n> +\t\tLOG(Af, Warning)\n> +\t\t\t<< __FUNCTION__ << \" not valid in mode \" << mode_;\n\nWe don't use __FUNCTION__ for the reason that as far as I know it's\nnot a standard language keyword (while __func__ is iirc). I'm also not\nsure how it works with name mangling as I see gcc has a\n__PRETTY_FUNCTION__ specific extensions to beautify names.\n\nI would just\n << \"setTrigger() not valid in mode \" << mode_;\n\n> +\t\treturn;\n> +\t}\n> +\n> +\tLOG(Af, Debug) << \"Trigger called with \" << trigger;\n> +\n> +\tif (trigger == controls::AfTriggerStart)\n> +\t\tafReset();\n> +\telse\n> +\t\tstate_ = controls::AfStateIdle;\n\nI would switch to be sure to handle all cases. A minor though\n\n> +}\n> +\n> +void AfHillClimbing::setPause(controls::AfPauseEnum pause)\n> +{\n> +\tif (mode_ != controls::AfModeContinuous) {\n> +\t\tLOG(Af, Warning)\n> +\t\t\t<< __FUNCTION__ << \" not valid in mode \" << mode_;\n\nditto\n\n> +\t\treturn;\n> +\t}\n> +\n> +\tswitch (pause) {\n> +\tcase controls::AfPauseImmediate:\n> +\t\tpauseState_ = controls::AfPauseStatePaused;\n> +\t\tbreak;\n> +\tcase controls::AfPauseDeferred:\n> +\t\tLOG(Af, Warning) << \"AfPauseDeferred is not supported!\";\n> +\t\tbreak;\n> +\tcase controls::AfPauseResume:\n> +\t\tpauseState_ = controls::AfPauseStateRunning;\n> +\t\tbreak;\n> +\t}\n> +}\n> +\n> +void AfHillClimbing::setLensPosition(float lensPosition)\n> +{\n> +\tif (mode_ != controls::AfModeManual) {\n> +\t\tLOG(Af, Warning)\n> +\t\t\t<< __FUNCTION__ << \" not valid in mode \" << mode_;\n> +\t\treturn;\n> +\t}\n> +\n> +\tlensPosition_ = static_cast(lensPosition);\n> +\n> +\tLOG(Af, Debug) << \"Requesting lens position \" << lensPosition_;\n> +}\n> +\n> +void AfHillClimbing::afCoarseScan()\n> +{\n> +\tif (coarseCompleted_)\n> +\t\treturn;\n> +\n> +\tif (afScan(coarseSearchStep_)) {\n> +\t\tcoarseCompleted_ = true;\n> +\t\tmaxContrast_ = 0;\n> +\t\tconst auto diff = static_cast(\n> +\t\t\tstd::abs(lensPosition_) * fineRange_);\n> +\t\tlensPosition_ = std::max(lensPosition_ - diff, minLensPosition_);\n> +\t\tmaxStep_ = std::min(lensPosition_ + diff, maxLensPosition_);\n> +\t}\n> +}\n> +\n> +void AfHillClimbing::afFineScan()\n> +{\n> +\tif (!coarseCompleted_)\n> +\t\treturn;\n> +\n> +\tif (afScan(fineSearchStep_)) {\n> +\t\tLOG(Af, Debug) << \"AF found the best focus position!\";\n> +\t\tstate_ = controls::AfStateFocused;\n> +\t}\n> +}\n> +\n> +bool AfHillClimbing::afScan(uint32_t steps)\n> +{\n> +\tif (lensPosition_ + static_cast(steps) > maxStep_) {\n> +\t\t/* If the max step is reached, move lens to the position. */\n> +\t\tlensPosition_ = bestPosition_;\n> +\t\treturn true;\n> +\t} else {\n\nIf you return in the if() clause , you can remove else { } and save\none indendation level\n\n> +\t\t/*\n> +\t\t * Find the maximum of the variance by estimating its\n> +\t\t * derivative. If the direction changes, it means we have passed\n> +\t\t * a maximum one step before.\n> +\t\t */\n> +\t\tif ((currentContrast_ - maxContrast_) >= -(maxContrast_ * 0.1)) {\n> +\t\t\t/*\n> +\t\t\t * Positive and zero derivative:\n> +\t\t\t * The variance is still increasing. The focus could be\n> +\t\t\t * increased for the next comparison. Also, the max\n> +\t\t\t * variance and previous focus value are updated.\n> +\t\t\t */\n> +\t\t\tbestPosition_ = lensPosition_;\n> +\t\t\tlensPosition_ += static_cast(steps);\n> +\t\t\tmaxContrast_ = currentContrast_;\n> +\t\t} else {\n> +\t\t\t/*\n> +\t\t\t * Negative derivative:\n> +\t\t\t * The variance starts to decrease which means the maximum\n> +\t\t\t * variance is found. Set focus step to previous good one\n> +\t\t\t * then return immediately.\n> +\t\t\t */\n> +\t\t\tlensPosition_ = bestPosition_;\n> +\t\t\treturn true;\n> +\t\t}\n> +\t}\n> +\n> +\tLOG(Af, Debug) << \"Previous step is \" << bestPosition_\n> +\t\t << \", Current step is \" << lensPosition_;\n> +\treturn false;\n> +}\n> +\n> +void AfHillClimbing::afReset()\n> +{\n> +\tLOG(Af, Debug) << \"Reset AF parameters\";\n> +\tlensPosition_ = minLensPosition_;\n> +\tmaxStep_ = maxLensPosition_;\n> +\tstate_ = controls::AfStateScanning;\n> +\tcoarseCompleted_ = false;\n> +\tmaxContrast_ = 0.0;\n> +\tsetFramesToSkip(1);\n> +}\n> +\n> +bool AfHillClimbing::afIsOutOfFocus() const\n> +{\n> +\tconst double diff_var = std::abs(currentContrast_ - maxContrast_);\n> +\tconst double var_ratio = diff_var / maxContrast_;\n> +\tLOG(Af, Debug) << \"Variance change rate: \" << var_ratio\n> +\t\t << \", Current lens step: \" << lensPosition_;\n> +\treturn var_ratio > maxChange_;\n> +}\n> +\n> +bool AfHillClimbing::shouldSkipFrame()\n> +{\n> +\tif (framesToSkip_ > 0) {\n> +\t\tframesToSkip_--;\n> +\t\treturn true;\n> +\t}\n> +\n> +\treturn false;\n> +}\n> +\n> +} /* namespace ipa::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..47d2bbec\n> --- /dev/null\n> +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> @@ -0,0 +1,91 @@\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) 2023, Theobroma Systems\n> + *\n> + * af_hill_climbing.h - AF contrast based hill climbing common algorithm\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \"af.h\"\n> +\n> +namespace libcamera {\n> +\n> +class YamlObject;\n> +\n> +namespace ipa::algorithms {\n> +\n> +LOG_DECLARE_CATEGORY(Af)\n> +\n> +class AfHillClimbing : public Af\n\nIsn't it better to declare the class final instead of the single\nfunctions ?\n\nclass AfHillClimbing final : public Af\n\n> +{\n> +public:\n> +\tint init(const YamlObject &tuningData);\n> +\tvoid configure(int32_t minFocusPosition, int32_t maxFocusPosition);\n> +\tint32_t process(double currentContrast);\n> +\tvoid setFramesToSkip(uint32_t n);\n> +\n> +\tcontrols::AfStateEnum state() final { return state_; }\n> +\tcontrols::AfPauseStateEnum pauseState() final { return pauseState_; }\n> +\n> +private:\n> +\tvoid setMode(controls::AfModeEnum mode) final;\n> +\tvoid setRange(controls::AfRangeEnum range) final;\n> +\tvoid setSpeed(controls::AfSpeedEnum speed) final;\n> +\tvoid setMeteringMode(controls::AfMeteringEnum metering) final;\n> +\tvoid setWindows(Span windows) final;\n> +\tvoid setTrigger(controls::AfTriggerEnum trigger) final;\n> +\tvoid setPause(controls::AfPauseEnum pause) final;\n> +\tvoid setLensPosition(float lensPosition) final;\n\nThese functions should have the 'override' specifier\n\n> +\n> +\tvoid processAutoMode();\n> +\tvoid processContinuousMode();\n> +\tvoid afCoarseScan();\n> +\tvoid afFineScan();\n> +\tbool afScan(uint32_t steps);\n> +\tvoid afReset();\n> +\t[[nodiscard]] bool afIsOutOfFocus() const;\n> +\tbool shouldSkipFrame();\n> +\n> +\tcontrols::AfModeEnum mode_ = controls::AfModeManual;\n> +\tcontrols::AfStateEnum state_ = controls::AfStateIdle;\n> +\tcontrols::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning;\n> +\n> +\t/* Current focus lens position. */\n> +\tint32_t lensPosition_ = 0;\n> +\t/* Local optimum focus lens position during scanning. */\n> +\tint32_t bestPosition_ = 0;\n> +\n> +\t/* Current AF statistic contrast. */\n> +\tdouble currentContrast_ = 0;\n> +\t/* It is used to determine the derivative during scanning */\n> +\tdouble maxContrast_ = 0;\n> +\t/* The designated maximum range of focus scanning. */\n> +\tint32_t maxStep_ = 0;\n> +\t/* If the coarse scan completes, it is set to true. */\n> +\tbool coarseCompleted_ = false;\n> +\n> +\tuint32_t framesToSkip_ = 0;\n> +\n> +\t/* Position limits of the focus lens. */\n> +\tint32_t minLensPosition_;\n> +\tint32_t maxLensPosition_;\n> +\n> +\t/* Minimum position step for searching appropriate focus. */\n> +\tuint32_t coarseSearchStep_;\n> +\tuint32_t fineSearchStep_;\n> +\n> +\t/* Fine scan range 0 < fineRange_ < 1. */\n> +\tdouble fineRange_;\n> +\n> +\t/* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */\n> +\tdouble maxChange_;\n> +};\n> +\n> +} /* namespace ipa::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> index 3df4798f..20e437fc 100644\n> --- a/src/ipa/libipa/algorithms/meson.build\n> +++ b/src/ipa/libipa/algorithms/meson.build\n> @@ -2,8 +2,10 @@\n>\n> libipa_algorithms_headers = files([\n> 'af.h',\n> + 'af_hill_climbing.h',\n> ])\n>\n> libipa_algorithms_sources = files([\n> 'af.cpp',\n> + 'af_hill_climbing.cpp',\n> ])\n> --\n> 2.39.2\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 191F3C0F1B\n\tfor ;\n\tFri, 24 Mar 2023 15:30:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4476B62715;\n\tFri, 24 Mar 2023 16:30:46 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 75D27603AA\n\tfor ;\n\tFri, 24 Mar 2023 16:30:44 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D9716A58;\n\tFri, 24 Mar 2023 16:30:43 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679671846;\n\tbh=j22sM2IZZwC90vFAodbmOfkJ+YjU8etRICMYuqM3PYs=;\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=Dgqi0XymK9QyzRlXaL6Y7ICx/Ozr6ec+VelOiHqSgcv37laKWT5D45QVzJ2GJ+t6f\n\tzkNtpulvOClP7duECJioYLzOYfaGfWoUUB1rWHuTOg0/QTV9YA1tto6wGYc+Hz3OzA\n\tCOeFQIy2IxigfQe4QaSkuBmJsE7TMXoKrI956Gp14XyX7Z+C9wZPZjFFe6d+ZnulEi\n\tM2B18Ds/1dr/j8ebq0E/0RBbi0eOtJO2fnEaf/wUuYeiaJeU+PC0PekDE0UvJVtmu/\n\tDa1mmhf3CVzT4FlOmH0TSWSZpEE03btzEG8QutF2gIoG2Q1yKFLdGzG+vwHf9LNSqv\n\tVOnmR0KPhKTFA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1679671844;\n\tbh=j22sM2IZZwC90vFAodbmOfkJ+YjU8etRICMYuqM3PYs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LdAgLKKcyxWQKZnarpWi32g6Km5nAa+pS2qBKzfgZzpv+ZYXmRowA+q8BDmYH+w32\n\tI/CTZMVfgt5VG8r17YQXSzm4Q8cupLh5DPTjDPd114F5x7jFZYnZ7jEmQJEFgM/4Ss\n\tWyUmbDe0BuSEYnm7qdvJdFO2UF6zP8pwWFiLVtvU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"LdAgLKKc\"; dkim-atps=neutral","Date":"Fri, 24 Mar 2023 16:30:40 +0100","To":"Daniel Semkowicz ","Message-ID":"<20230324153040.sk43lwg36qmxwz6t@uno.localdomain>","References":"<20230324142908.64224-1-dse@thaumatec.com>\n\t<20230324142908.64224-5-dse@thaumatec.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230324142908.64224-5-dse@thaumatec.com>","Subject":"Re: [libcamera-devel] [PATCH v5 04/10] ipa: Add common contrast\n\tbased AF implementation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"jacopo.mondi@ideasonboard.com, libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26762,"web_url":"https://patchwork.libcamera.org/comment/26762/","msgid":"","date":"2023-03-27T12:29:45","subject":"Re: [libcamera-devel] [PATCH v5 04/10] ipa: Add common contrast\n\tbased AF implementation","submitter":{"id":126,"url":"https://patchwork.libcamera.org/api/people/126/","name":"Daniel Semkowicz","email":"dse@thaumatec.com"},"content":"Hi Jacopo,\n\nOn Fri, Mar 24, 2023 at 4:30 PM Jacopo Mondi\n wrote:\n>\n> Hi Daniel\n>\n> On Fri, Mar 24, 2023 at 03:29:02PM +0100, Daniel Semkowicz via libcamera-devel wrote:\n> > Create a new class with contrast based Auto Focus implementation\n> > using hill climbing algorithm. This common implementation is independent\n> > of platform specific code. This way, each platform can just implement\n> > contrast calculation and run the AF control loop basing on this class.\n> > This implementation is based on the code that was common for IPU3\n> > and RPi AF algorithms.\n> >\n> > Signed-off-by: Daniel Semkowicz \n> > ---\n> > .../libipa/algorithms/af_hill_climbing.cpp | 379 ++++++++++++++++++\n> > src/ipa/libipa/algorithms/af_hill_climbing.h | 91 +++++\n> > src/ipa/libipa/algorithms/meson.build | 2 +\n> > 3 files changed, 472 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..6ee090eb\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > @@ -0,0 +1,379 @@\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) 2023, Theobroma Systems\n> > + *\n> > + * af_hill_climbing.cpp - AF contrast based hill climbing common algorithm\n> > + */\n> > +\n> > +#include \"af_hill_climbing.h\"\n> > +\n> > +#include \"libcamera/internal/yaml_parser.h\"\n> > +\n> > +/**\n> > + * \\file af_hill_climbing.h\n> > + * \\brief AF contrast based hill climbing common algorithm\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa::algorithms {\n> > +\n> > +LOG_DEFINE_CATEGORY(Af)\n> > +\n> > +/**\n> > + * \\class AfHillClimbing\n> > + * \\brief Contrast based hill climbing auto focus control algorithm\n> > + * implementation\n> > + *\n> > + * Control part of the auto focus algorithm. It calculates the lens position\n> > + * based on the contrast measure supplied by the platform-specific\n> > + * implementation. This way it is independent from the platform.\n> > + *\n> > + * Platform layer that use this algorithm should call process() function\n> > + * for each each frame and set the lens to the calculated position.\n> > + *\n> > + * Focus search is divided into two phases:\n> > + * 1. coarse search,\n> > + * 2. fine search.\n> > + *\n> > + * In the first phase, the lens is moved with bigger steps to quickly find\n> > + * a rough position for the best focus. Then, based on the outcome of coarse\n> > + * search, the second phase is executed. Lens is moved with smaller steps\n> > + * in a limited range within the rough position to find the exact position\n> > + * for best focus.\n> > + *\n> > + * Tuning file parameters:\n> > + * - **coarse-search-step:** The value by which the lens position will change\n> > + * in one step in the *coarse search* phase. Unit is lens specific.\n> > + * - **fine-search-step:** The value by which the lens position will change\n> > + * in one step in the *fine search* phase. Unit is lens specific.\n> > + * - **fine-search-range:** Search range in the *fine search* phase, expressed\n> > + * as a percentage of the coarse search result. Valid values are\n> > + * in the [0, 100] interval. Value 5 means 5%. If coarse search stopped\n> > + * at value 200, the fine search range will be [190, 210].\n> > + * - **max-variance-change:** ratio of contrast variance change in the\n> > + * *continuous mode* needed for triggering the focus change. When the variance\n> > + * change exceeds this value, focus search will be triggered. Valid values are\n> > + * in the [0.0, 1.0] interval.\n> > + * .\n> > + *\n> > + * \\todo Search range in the *fine search* phase should depend on the lens\n> > + * movement range rather than coarse search result.\n> > + * \\todo Implement setRange.\n> > + * \\todo Implement setSpeed.\n> > + * \\todo Implement setMeteringMode.\n> > + * \\todo Implement setWindows.\n> > + * \\todo Implement the AfPauseDeferred mode.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Initialize the AfHillClimbing with tuning data\n> > + * \\param[in] tuningData The tuning data for the algorithm\n> > + *\n> > + * This method should be called in the libcamera::ipa::Algorithm::init()\n> > + * method of the platform layer.\n> > + *\n> > + * \\return 0 if successful, an error code otherwise\n> > + */\n> > +int AfHillClimbing::init(const YamlObject &tuningData)\n> > +{\n> > + coarseSearchStep_ = tuningData[\"coarse-search-step\"].get(30);\n> > + fineSearchStep_ = tuningData[\"fine-search-step\"].get(1);\n> > + fineRange_ = tuningData[\"fine-search-range\"].get(5);\n> > + fineRange_ /= 100;\n> > + maxChange_ = tuningData[\"max-variance-change\"].get(0.5);\n> > +\n> > + LOG(Af, Debug) << \"coarseSearchStep_: \" << coarseSearchStep_\n> > + << \", fineSearchStep_: \" << fineSearchStep_\n> > + << \", fineRange_: \" << fineRange_\n> > + << \", maxChange_: \" << maxChange_;\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Configure the AfHillClimbing with sensor and lens information\n> > + * \\param[in] minFocusPosition Minimum position supported by camera lens\n> > + * \\param[in] maxFocusPosition Maximum position supported by camera lens\n> > + *\n> > + * This method should be called in the libcamera::ipa::Algorithm::configure()\n> > + * method of the platform layer.\n> > + */\n> > +void AfHillClimbing::configure(int32_t minFocusPosition,\n> > + int32_t maxFocusPosition)\n> > +{\n> > + minLensPosition_ = minFocusPosition;\n> > + maxLensPosition_ = maxFocusPosition;\n> > +}\n> > +\n> > +/**\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 in the libcamera::ipa::Algorithm::process()\n> > + * method of the platform layer for each frame.\n> > + *\n> > + * Contrast value supplied in the \\p currentContrast parameter can be platform\n> > + * specific. The only requirement is the contrast value must increase with\n> > + * the increasing image focus. Contrast value must be highest when image is in\n> > + * focus.\n> > + *\n> > + * \\return New lens position calculated by the AF algorithm\n> > + */\n> > +int32_t AfHillClimbing::process(double currentContrast)\n> > +{\n> > + currentContrast_ = currentContrast;\n> > +\n> > + if (shouldSkipFrame())\n> > + return lensPosition_;\n> > +\n> > + switch (mode_) {\n> > + case controls::AfModeManual:\n> > + /* Nothing to process. */\n> > + break;\n> > + case controls::AfModeAuto:\n> > + processAutoMode();\n> > + break;\n> > + case controls::AfModeContinuous:\n> > + processContinuousMode();\n> > + break;\n> > + }\n> > +\n> > + return lensPosition_;\n> > +}\n> > +\n> > +void AfHillClimbing::processAutoMode()\n> > +{\n> > + if (state_ == controls::AfStateScanning) {\n> > + afCoarseScan();\n> > + afFineScan();\n> > + }\n> > +}\n> > +\n> > +void AfHillClimbing::processContinuousMode()\n> > +{\n> > + /* If we are in a paused state, we won't process the stats. */\n> > + if (pauseState_ == controls::AfPauseStatePaused)\n> > + return;\n> > +\n> > + if (state_ == controls::AfStateScanning) {\n> > + afCoarseScan();\n> > + afFineScan();\n> > + return;\n> > + }\n> > +\n> > + /*\n> > + * AF scan can be started at any moment in AfModeContinuous,\n> > + * except when the state is already AfStateScanning.\n> > + */\n> > + if (afIsOutOfFocus())\n> > + afReset();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Request AF to skip n frames\n> > + * \\param[in] n Number of frames to be skipped\n> > + *\n> > + * For the requested number of frames, the AF calculation will be skipped\n> > + * and lens position will not change. The platform layer still needs to\n> > + * call process() function for each frame during this time.\n> > + * This function can be used by the platform layer if the hardware needs more\n> > + * time for some operations.\n> > + *\n> > + * The number of the requested frames (\\p n) will be applied only if \\p n has\n> > + * higher value than the number of frames already requested to be skipped.\n> > + * For example, if *setFramesToSkip(5)* was already called for the current\n> > + * frame, then calling *setFramesToSkip(3)* will not override the previous\n> > + * request and 5 frames will be skipped.\n>\n> This still puzzles me a bit as it is not clear to me when the platform\n> layer is supposed to set the number of frames to skip.\n>\n> Looking at the implementation it happens everytime the lens is moved.\n> I guess this is fine for now, but this will need to be handled better\n> by the CameraLens class modeling the lens movement delay.\n>\n\nIn RkISP it is also needed when setting AF Windows. ISP needs a one\nadditional frame to calculate the new statistics after setting window.\nThis is why I exposed this method as public.\n\n> > + */\n> > +void AfHillClimbing::setFramesToSkip(uint32_t n)\n> > +{\n> > + if (n > framesToSkip_)\n> > + framesToSkip_ = n;\n> > +}\n> > +\n> > +void AfHillClimbing::setMode(controls::AfModeEnum mode)\n> > +{\n> > + if (mode == mode_)\n> > + return;\n> > +\n> > + LOG(Af, Debug) << \"Switched AF mode from \" << mode_ << \" to \" << mode;\n> > + mode_ = mode;\n> > +\n> > + state_ = controls::AfStateIdle;\n> > + pauseState_ = controls::AfPauseStateRunning;\n> > +\n> > + if (mode_ == controls::AfModeContinuous)\n> > + afReset();\n> > +}\n> > +\n> > +void AfHillClimbing::setRange([[maybe_unused]] controls::AfRangeEnum range)\n> > +{\n> > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > +}\n> > +\n> > +void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed)\n> > +{\n> > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > +}\n> > +\n> > +void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering)\n> > +{\n> > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > +}\n> > +\n> > +void AfHillClimbing::setWindows([[maybe_unused]] Span windows)\n> > +{\n> > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > +}\n> > +\n>\n> I'm still of the idea you should better not implement these functions\n> at all. If anything tries to call into them will fail at compile time\n> instead of getting a run-time error in the logs\n\nThese functions are pure virtual, so you are forced by compiler\nto implement them.\n\n>\n> > +void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)\n> > +{\n> > + if (mode_ != controls::AfModeAuto) {\n> > + LOG(Af, Warning)\n> > + << __FUNCTION__ << \" not valid in mode \" << mode_;\n>\n> We don't use __FUNCTION__ for the reason that as far as I know it's\n> not a standard language keyword (while __func__ is iirc). I'm also not\n> sure how it works with name mangling as I see gcc has a\n> __PRETTY_FUNCTION__ specific extensions to beautify names.\n>\n> I would just\n> << \"setTrigger() not valid in mode \" << mode_;\n>\n> > + return;\n> > + }\n> > +\n> > + LOG(Af, Debug) << \"Trigger called with \" << trigger;\n> > +\n> > + if (trigger == controls::AfTriggerStart)\n> > + afReset();\n> > + else\n> > + state_ = controls::AfStateIdle;\n>\n> I would switch to be sure to handle all cases. A minor though\n>\n> > +}\n> > +\n> > +void AfHillClimbing::setPause(controls::AfPauseEnum pause)\n> > +{\n> > + if (mode_ != controls::AfModeContinuous) {\n> > + LOG(Af, Warning)\n> > + << __FUNCTION__ << \" not valid in mode \" << mode_;\n>\n> ditto\n>\n> > + return;\n> > + }\n> > +\n> > + switch (pause) {\n> > + case controls::AfPauseImmediate:\n> > + pauseState_ = controls::AfPauseStatePaused;\n> > + break;\n> > + case controls::AfPauseDeferred:\n> > + LOG(Af, Warning) << \"AfPauseDeferred is not supported!\";\n> > + break;\n> > + case controls::AfPauseResume:\n> > + pauseState_ = controls::AfPauseStateRunning;\n> > + break;\n> > + }\n> > +}\n> > +\n> > +void AfHillClimbing::setLensPosition(float lensPosition)\n> > +{\n> > + if (mode_ != controls::AfModeManual) {\n> > + LOG(Af, Warning)\n> > + << __FUNCTION__ << \" not valid in mode \" << mode_;\n> > + return;\n> > + }\n> > +\n> > + lensPosition_ = static_cast(lensPosition);\n> > +\n> > + LOG(Af, Debug) << \"Requesting lens position \" << lensPosition_;\n> > +}\n> > +\n> > +void AfHillClimbing::afCoarseScan()\n> > +{\n> > + if (coarseCompleted_)\n> > + return;\n> > +\n> > + if (afScan(coarseSearchStep_)) {\n> > + coarseCompleted_ = true;\n> > + maxContrast_ = 0;\n> > + const auto diff = static_cast(\n> > + std::abs(lensPosition_) * fineRange_);\n> > + lensPosition_ = std::max(lensPosition_ - diff, minLensPosition_);\n> > + maxStep_ = std::min(lensPosition_ + diff, maxLensPosition_);\n> > + }\n> > +}\n> > +\n> > +void AfHillClimbing::afFineScan()\n> > +{\n> > + if (!coarseCompleted_)\n> > + return;\n> > +\n> > + if (afScan(fineSearchStep_)) {\n> > + LOG(Af, Debug) << \"AF found the best focus position!\";\n> > + state_ = controls::AfStateFocused;\n> > + }\n> > +}\n> > +\n> > +bool AfHillClimbing::afScan(uint32_t steps)\n> > +{\n> > + if (lensPosition_ + static_cast(steps) > maxStep_) {\n> > + /* If the max step is reached, move lens to the position. */\n> > + lensPosition_ = bestPosition_;\n> > + return true;\n> > + } else {\n>\n> If you return in the if() clause , you can remove else { } and save\n> one indendation level\n\nGood points! I will rework the code :)\n\n>\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_ += static_cast(steps);\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> > + LOG(Af, Debug) << \"Previous step is \" << bestPosition_\n> > + << \", Current step is \" << lensPosition_;\n> > + return false;\n> > +}\n> > +\n> > +void AfHillClimbing::afReset()\n> > +{\n> > + LOG(Af, Debug) << \"Reset AF parameters\";\n> > + lensPosition_ = minLensPosition_;\n> > + maxStep_ = maxLensPosition_;\n> > + state_ = controls::AfStateScanning;\n> > + coarseCompleted_ = false;\n> > + maxContrast_ = 0.0;\n> > + setFramesToSkip(1);\n> > +}\n> > +\n> > +bool AfHillClimbing::afIsOutOfFocus() const\n> > +{\n> > + const double diff_var = std::abs(currentContrast_ - maxContrast_);\n> > + const double var_ratio = diff_var / maxContrast_;\n> > + LOG(Af, Debug) << \"Variance change rate: \" << var_ratio\n> > + << \", Current lens step: \" << lensPosition_;\n> > + return var_ratio > maxChange_;\n> > +}\n> > +\n> > +bool AfHillClimbing::shouldSkipFrame()\n> > +{\n> > + if (framesToSkip_ > 0) {\n> > + framesToSkip_--;\n> > + return true;\n> > + }\n> > +\n> > + return false;\n> > +}\n> > +\n> > +} /* namespace ipa::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..47d2bbec\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > @@ -0,0 +1,91 @@\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) 2023, Theobroma Systems\n> > + *\n> > + * af_hill_climbing.h - AF contrast based hill climbing common algorithm\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \"af.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +class YamlObject;\n> > +\n> > +namespace ipa::algorithms {\n> > +\n> > +LOG_DECLARE_CATEGORY(Af)\n> > +\n> > +class AfHillClimbing : public Af\n>\n> Isn't it better to declare the class final instead of the single\n> functions ?\n\nIt is not the same.\nWhen used in a class definition, final specifies\nthat the class cannot be derived from.\nWhen used in a virtual function declaration or definition, final\nspecifier ensures that the function is virtual and specifies that it\nmay not be overridden by derived classes.\nIt is technically possible to derive from AfHillClimbing to build\non it, even if it was meant to be used in composition style.\nDo We want to force a composition approach or leave possibility to\nderive from it?\n\nBest regards\nDaniel\n\n>\n> class AfHillClimbing final : public Af\n>\n> > +{\n> > +public:\n> > + int init(const YamlObject &tuningData);\n> > + void configure(int32_t minFocusPosition, int32_t maxFocusPosition);\n> > + int32_t process(double currentContrast);\n> > + void setFramesToSkip(uint32_t n);\n> > +\n> > + controls::AfStateEnum state() final { return state_; }\n> > + controls::AfPauseStateEnum pauseState() final { return pauseState_; }\n> > +\n> > +private:\n> > + void setMode(controls::AfModeEnum mode) final;\n> > + void setRange(controls::AfRangeEnum range) final;\n> > + void setSpeed(controls::AfSpeedEnum speed) final;\n> > + void setMeteringMode(controls::AfMeteringEnum metering) final;\n> > + void setWindows(Span windows) final;\n> > + void setTrigger(controls::AfTriggerEnum trigger) final;\n> > + void setPause(controls::AfPauseEnum pause) final;\n> > + void setLensPosition(float lensPosition) final;\n>\n> These functions should have the 'override' specifier\n>\n> > +\n> > + void processAutoMode();\n> > + void processContinuousMode();\n> > + void afCoarseScan();\n> > + void afFineScan();\n> > + bool afScan(uint32_t steps);\n> > + void afReset();\n> > + [[nodiscard]] bool afIsOutOfFocus() const;\n> > + bool shouldSkipFrame();\n> > +\n> > + controls::AfModeEnum mode_ = controls::AfModeManual;\n> > + controls::AfStateEnum state_ = controls::AfStateIdle;\n> > + controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning;\n> > +\n> > + /* Current focus lens position. */\n> > + int32_t lensPosition_ = 0;\n> > + /* Local optimum focus lens position during scanning. */\n> > + int32_t bestPosition_ = 0;\n> > +\n> > + /* Current AF statistic contrast. */\n> > + double currentContrast_ = 0;\n> > + /* It is used to determine the derivative during scanning */\n> > + double maxContrast_ = 0;\n> > + /* The designated maximum range of focus scanning. */\n> > + int32_t maxStep_ = 0;\n> > + /* If the coarse scan completes, it is set to true. */\n> > + bool coarseCompleted_ = false;\n> > +\n> > + uint32_t framesToSkip_ = 0;\n> > +\n> > + /* Position limits of the focus lens. */\n> > + int32_t minLensPosition_;\n> > + int32_t maxLensPosition_;\n> > +\n> > + /* Minimum position step for searching appropriate focus. */\n> > + uint32_t coarseSearchStep_;\n> > + uint32_t fineSearchStep_;\n> > +\n> > + /* Fine scan range 0 < fineRange_ < 1. */\n> > + double fineRange_;\n> > +\n> > + /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */\n> > + double maxChange_;\n> > +};\n> > +\n> > +} /* namespace ipa::algorithms */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> > index 3df4798f..20e437fc 100644\n> > --- a/src/ipa/libipa/algorithms/meson.build\n> > +++ b/src/ipa/libipa/algorithms/meson.build\n> > @@ -2,8 +2,10 @@\n> >\n> > libipa_algorithms_headers = files([\n> > 'af.h',\n> > + 'af_hill_climbing.h',\n> > ])\n> >\n> > libipa_algorithms_sources = files([\n> > 'af.cpp',\n> > + 'af_hill_climbing.cpp',\n> > ])\n> > --\n> > 2.39.2\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 09CB4BD160\n\tfor ;\n\tMon, 27 Mar 2023 12:29:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4E78762722;\n\tMon, 27 Mar 2023 14:29:58 +0200 (CEST)","from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com\n\t[IPv6:2a00:1450:4864:20::52f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D1B27626D7\n\tfor ;\n\tMon, 27 Mar 2023 14:29:56 +0200 (CEST)","by mail-ed1-x52f.google.com with SMTP id er18so24221143edb.9\n\tfor ;\n\tMon, 27 Mar 2023 05:29:56 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679920198;\n\tbh=t1MrZXAsD7kLsrDmnLVycN7vUuiVVOUHxGHu7AWim/w=;\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=U37Gnu9/jZVni+5zxI/6Mt82f37s15YIr/sViabOOJUeZnwoqmuVLYFurrZ27XbXs\n\tXKHynRrhQxMl30ySO/hq3q/cP+vwZxOJLmcAsPQrpAO+Uo3UzsyFsLRMV0HJJb0CGF\n\thCCC4bLHu5h0d8BN4OV7G+AWSi6ranwhapKeKkkW1jbPdWqfdxM3GpcN6KGIKAYMbF\n\tZjG5wjwJ1sdrJc2OfnB8wNvqHmJzWKjheLpgEeR4UCCF7KOeG6PJ6654q7+A+CeAKO\n\tCAsf6eOCRRVt7mRdNrTwi09uTBLxm379Zm6pBvPntPz0btAul7t+qbC4mNr32ZCZvG\n\tCItdlzwDEb9yg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20210112.gappssmtp.com; s=20210112; t=1679920196;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=VXcyD6WSblDv/ZWvLjWQHv9Y3r2P+NHXh+CUAxHfILg=;\n\tb=5ApN/YGcnSp5UA4NYXzpbpmm73riY32vLca+e+nYxBQV/lhZv+lwlqLSI+NertKaDJ\n\tI9JGcpkSbUc4ENWsasxL0EYtino9Q46qVsbDfS+VgvrMf9ipSmRj60KlqA+Lh9591KM6\n\tZ/deUf+a9DyplzonDZumJcjH1Emqf/eG04mEckY2Uxc3HaY5JiPw+19jy96vJ1gLCHri\n\tZV2Ed0+QNmAsSj/QEYrQCnJOUR+js1TX9bczjdw4K6dy3vcH5OGksAJqNdT3N5AQR2UY\n\tHamt8UWB6RXjEqEVu+tgM/7ljqHj1jzWqg6k+N6AtVX2pFL8rxsifzdxdepyWsmmUuxI\n\tY3kw=="],"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=\"5ApN/YGc\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1679920196;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=VXcyD6WSblDv/ZWvLjWQHv9Y3r2P+NHXh+CUAxHfILg=;\n\tb=bApgbHFukLBcFso7Rf1Hv6+C7LUBchfG5o73mY2CR9MtFmthwhy/YZLJWy1Nkk9aA1\n\t7SsZgYp9h8eRIgUMEWX5EouSfp+NTtLN91v1/Koztto1RHEX4Onjk0SGBw1jB9pQqmmu\n\taVNYOngEPjmxLIfOKYSf6xJA2cQV9IwO2WNro49tvB4uuYoYTyBDaz8bNQhyknXZQHBV\n\tmaCiqrIwH3KdqHQ7z1f2YWaAGEdjPHkzqaxpQbE49IoVg89+LlWuWrJQcpf0TR+I+MXQ\n\tBprahyGg8/DncLv0wW2NId5Xd4wMPAdO6ii/rrpYRurebXMAc5i2u5h9n4leRq8GPISh\n\t2d6Q==","X-Gm-Message-State":"AAQBX9dpDALLI/Tp3MLyOrxzfBP5IXFwo4d0i5L8ijrDEq1KwfbcT704\n\tRGX6dT6x6TdfkxNV2ZJ76TzewGByP3Pbungpcw3sDloUN7YCGw0anJA=","X-Google-Smtp-Source":"AKy350bxxvmC5XPngY5ePIQqATQTws72Vfkit547BX4Hv1UgRzi6yyTaKrmwQEcXgNQ252gEXANFinn0rldtVJyYt04=","X-Received":"by 2002:a17:906:cf89:b0:932:4577:6705 with SMTP id\n\tum9-20020a170906cf8900b0093245776705mr5980849ejb.6.1679920196192;\n\tMon, 27 Mar 2023 05:29:56 -0700 (PDT)","MIME-Version":"1.0","References":"<20230324142908.64224-1-dse@thaumatec.com>\n\t<20230324142908.64224-5-dse@thaumatec.com>\n\t<20230324153040.sk43lwg36qmxwz6t@uno.localdomain>","In-Reply-To":"<20230324153040.sk43lwg36qmxwz6t@uno.localdomain>","Date":"Mon, 27 Mar 2023 14:29:45 +0200","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v5 04/10] ipa: Add common contrast\n\tbased AF implementation","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":26765,"web_url":"https://patchwork.libcamera.org/comment/26765/","msgid":"<20230327130315.mw5cqmkt3jtpjwdq@uno.localdomain>","date":"2023-03-27T13:03:15","subject":"Re: [libcamera-devel] [PATCH v5 04/10] ipa: Add common contrast\n\tbased AF implementation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Daniel\n\nOn Mon, Mar 27, 2023 at 02:29:45PM +0200, Daniel Semkowicz wrote:\n> Hi Jacopo,\n>\n> On Fri, Mar 24, 2023 at 4:30 PM Jacopo Mondi\n> wrote:\n> >\n> > Hi Daniel\n> >\n> > On Fri, Mar 24, 2023 at 03:29:02PM +0100, Daniel Semkowicz via libcamera-devel wrote:\n> > > Create a new class with contrast based Auto Focus implementation\n> > > using hill climbing algorithm. This common implementation is independent\n> > > of platform specific code. This way, each platform can just implement\n> > > contrast calculation and run the AF control loop basing on this class.\n> > > This implementation is based on the code that was common for IPU3\n> > > and RPi AF algorithms.\n> > >\n> > > Signed-off-by: Daniel Semkowicz \n> > > ---\n> > > .../libipa/algorithms/af_hill_climbing.cpp | 379 ++++++++++++++++++\n> > > src/ipa/libipa/algorithms/af_hill_climbing.h | 91 +++++\n> > > src/ipa/libipa/algorithms/meson.build | 2 +\n> > > 3 files changed, 472 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..6ee090eb\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > @@ -0,0 +1,379 @@\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) 2023, Theobroma Systems\n> > > + *\n> > > + * af_hill_climbing.cpp - AF contrast based hill climbing common algorithm\n> > > + */\n> > > +\n> > > +#include \"af_hill_climbing.h\"\n> > > +\n> > > +#include \"libcamera/internal/yaml_parser.h\"\n> > > +\n> > > +/**\n> > > + * \\file af_hill_climbing.h\n> > > + * \\brief AF contrast based hill climbing common algorithm\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace ipa::algorithms {\n> > > +\n> > > +LOG_DEFINE_CATEGORY(Af)\n> > > +\n> > > +/**\n> > > + * \\class AfHillClimbing\n> > > + * \\brief Contrast based hill climbing auto focus control algorithm\n> > > + * implementation\n> > > + *\n> > > + * Control part of the auto focus algorithm. It calculates the lens position\n> > > + * based on the contrast measure supplied by the platform-specific\n> > > + * implementation. This way it is independent from the platform.\n> > > + *\n> > > + * Platform layer that use this algorithm should call process() function\n> > > + * for each each frame and set the lens to the calculated position.\n> > > + *\n> > > + * Focus search is divided into two phases:\n> > > + * 1. coarse search,\n> > > + * 2. fine search.\n> > > + *\n> > > + * In the first phase, the lens is moved with bigger steps to quickly find\n> > > + * a rough position for the best focus. Then, based on the outcome of coarse\n> > > + * search, the second phase is executed. Lens is moved with smaller steps\n> > > + * in a limited range within the rough position to find the exact position\n> > > + * for best focus.\n> > > + *\n> > > + * Tuning file parameters:\n> > > + * - **coarse-search-step:** The value by which the lens position will change\n> > > + * in one step in the *coarse search* phase. Unit is lens specific.\n> > > + * - **fine-search-step:** The value by which the lens position will change\n> > > + * in one step in the *fine search* phase. Unit is lens specific.\n> > > + * - **fine-search-range:** Search range in the *fine search* phase, expressed\n> > > + * as a percentage of the coarse search result. Valid values are\n> > > + * in the [0, 100] interval. Value 5 means 5%. If coarse search stopped\n> > > + * at value 200, the fine search range will be [190, 210].\n> > > + * - **max-variance-change:** ratio of contrast variance change in the\n> > > + * *continuous mode* needed for triggering the focus change. When the variance\n> > > + * change exceeds this value, focus search will be triggered. Valid values are\n> > > + * in the [0.0, 1.0] interval.\n> > > + * .\n> > > + *\n> > > + * \\todo Search range in the *fine search* phase should depend on the lens\n> > > + * movement range rather than coarse search result.\n> > > + * \\todo Implement setRange.\n> > > + * \\todo Implement setSpeed.\n> > > + * \\todo Implement setMeteringMode.\n> > > + * \\todo Implement setWindows.\n> > > + * \\todo Implement the AfPauseDeferred mode.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Initialize the AfHillClimbing with tuning data\n> > > + * \\param[in] tuningData The tuning data for the algorithm\n> > > + *\n> > > + * This method should be called in the libcamera::ipa::Algorithm::init()\n> > > + * method of the platform layer.\n> > > + *\n> > > + * \\return 0 if successful, an error code otherwise\n> > > + */\n> > > +int AfHillClimbing::init(const YamlObject &tuningData)\n> > > +{\n> > > + coarseSearchStep_ = tuningData[\"coarse-search-step\"].get(30);\n> > > + fineSearchStep_ = tuningData[\"fine-search-step\"].get(1);\n> > > + fineRange_ = tuningData[\"fine-search-range\"].get(5);\n> > > + fineRange_ /= 100;\n> > > + maxChange_ = tuningData[\"max-variance-change\"].get(0.5);\n> > > +\n> > > + LOG(Af, Debug) << \"coarseSearchStep_: \" << coarseSearchStep_\n> > > + << \", fineSearchStep_: \" << fineSearchStep_\n> > > + << \", fineRange_: \" << fineRange_\n> > > + << \", maxChange_: \" << maxChange_;\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Configure the AfHillClimbing with sensor and lens information\n> > > + * \\param[in] minFocusPosition Minimum position supported by camera lens\n> > > + * \\param[in] maxFocusPosition Maximum position supported by camera lens\n> > > + *\n> > > + * This method should be called in the libcamera::ipa::Algorithm::configure()\n> > > + * method of the platform layer.\n> > > + */\n> > > +void AfHillClimbing::configure(int32_t minFocusPosition,\n> > > + int32_t maxFocusPosition)\n> > > +{\n> > > + minLensPosition_ = minFocusPosition;\n> > > + maxLensPosition_ = maxFocusPosition;\n> > > +}\n> > > +\n> > > +/**\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 in the libcamera::ipa::Algorithm::process()\n> > > + * method of the platform layer for each frame.\n> > > + *\n> > > + * Contrast value supplied in the \\p currentContrast parameter can be platform\n> > > + * specific. The only requirement is the contrast value must increase with\n> > > + * the increasing image focus. Contrast value must be highest when image is in\n> > > + * focus.\n> > > + *\n> > > + * \\return New lens position calculated by the AF algorithm\n> > > + */\n> > > +int32_t AfHillClimbing::process(double currentContrast)\n> > > +{\n> > > + currentContrast_ = currentContrast;\n> > > +\n> > > + if (shouldSkipFrame())\n> > > + return lensPosition_;\n> > > +\n> > > + switch (mode_) {\n> > > + case controls::AfModeManual:\n> > > + /* Nothing to process. */\n> > > + break;\n> > > + case controls::AfModeAuto:\n> > > + processAutoMode();\n> > > + break;\n> > > + case controls::AfModeContinuous:\n> > > + processContinuousMode();\n> > > + break;\n> > > + }\n> > > +\n> > > + return lensPosition_;\n> > > +}\n> > > +\n> > > +void AfHillClimbing::processAutoMode()\n> > > +{\n> > > + if (state_ == controls::AfStateScanning) {\n> > > + afCoarseScan();\n> > > + afFineScan();\n> > > + }\n> > > +}\n> > > +\n> > > +void AfHillClimbing::processContinuousMode()\n> > > +{\n> > > + /* If we are in a paused state, we won't process the stats. */\n> > > + if (pauseState_ == controls::AfPauseStatePaused)\n> > > + return;\n> > > +\n> > > + if (state_ == controls::AfStateScanning) {\n> > > + afCoarseScan();\n> > > + afFineScan();\n> > > + return;\n> > > + }\n> > > +\n> > > + /*\n> > > + * AF scan can be started at any moment in AfModeContinuous,\n> > > + * except when the state is already AfStateScanning.\n> > > + */\n> > > + if (afIsOutOfFocus())\n> > > + afReset();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Request AF to skip n frames\n> > > + * \\param[in] n Number of frames to be skipped\n> > > + *\n> > > + * For the requested number of frames, the AF calculation will be skipped\n> > > + * and lens position will not change. The platform layer still needs to\n> > > + * call process() function for each frame during this time.\n> > > + * This function can be used by the platform layer if the hardware needs more\n> > > + * time for some operations.\n> > > + *\n> > > + * The number of the requested frames (\\p n) will be applied only if \\p n has\n> > > + * higher value than the number of frames already requested to be skipped.\n> > > + * For example, if *setFramesToSkip(5)* was already called for the current\n> > > + * frame, then calling *setFramesToSkip(3)* will not override the previous\n> > > + * request and 5 frames will be skipped.\n> >\n> > This still puzzles me a bit as it is not clear to me when the platform\n> > layer is supposed to set the number of frames to skip.\n> >\n> > Looking at the implementation it happens everytime the lens is moved.\n> > I guess this is fine for now, but this will need to be handled better\n> > by the CameraLens class modeling the lens movement delay.\n> >\n>\n> In RkISP it is also needed when setting AF Windows. ISP needs a one\n> additional frame to calculate the new statistics after setting window.\n> This is why I exposed this method as public.\n>\n> > > + */\n> > > +void AfHillClimbing::setFramesToSkip(uint32_t n)\n> > > +{\n> > > + if (n > framesToSkip_)\n> > > + framesToSkip_ = n;\n> > > +}\n> > > +\n> > > +void AfHillClimbing::setMode(controls::AfModeEnum mode)\n> > > +{\n> > > + if (mode == mode_)\n> > > + return;\n> > > +\n> > > + LOG(Af, Debug) << \"Switched AF mode from \" << mode_ << \" to \" << mode;\n> > > + mode_ = mode;\n> > > +\n> > > + state_ = controls::AfStateIdle;\n> > > + pauseState_ = controls::AfPauseStateRunning;\n> > > +\n> > > + if (mode_ == controls::AfModeContinuous)\n> > > + afReset();\n> > > +}\n> > > +\n> > > +void AfHillClimbing::setRange([[maybe_unused]] controls::AfRangeEnum range)\n> > > +{\n> > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > +}\n> > > +\n> > > +void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed)\n> > > +{\n> > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > +}\n> > > +\n> > > +void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering)\n> > > +{\n> > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > +}\n> > > +\n> > > +void AfHillClimbing::setWindows([[maybe_unused]] Span windows)\n> > > +{\n> > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > +}\n> > > +\n> >\n> > I'm still of the idea you should better not implement these functions\n> > at all. If anything tries to call into them will fail at compile time\n> > instead of getting a run-time error in the logs\n>\n> These functions are pure virtual, so you are forced by compiler\n> to implement them.\n>\n\nAh! I removed the definition in the cpp file but I left the\ndeclaration in the header, so I guess it worked here because of the\ncompiler generated one for me. Sorry for the overlook\n\n> >\n> > > +void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)\n> > > +{\n> > > + if (mode_ != controls::AfModeAuto) {\n> > > + LOG(Af, Warning)\n> > > + << __FUNCTION__ << \" not valid in mode \" << mode_;\n> >\n> > We don't use __FUNCTION__ for the reason that as far as I know it's\n> > not a standard language keyword (while __func__ is iirc). I'm also not\n> > sure how it works with name mangling as I see gcc has a\n> > __PRETTY_FUNCTION__ specific extensions to beautify names.\n> >\n> > I would just\n> > << \"setTrigger() not valid in mode \" << mode_;\n> >\n> > > + return;\n> > > + }\n> > > +\n> > > + LOG(Af, Debug) << \"Trigger called with \" << trigger;\n> > > +\n> > > + if (trigger == controls::AfTriggerStart)\n> > > + afReset();\n> > > + else\n> > > + state_ = controls::AfStateIdle;\n> >\n> > I would switch to be sure to handle all cases. A minor though\n> >\n> > > +}\n> > > +\n> > > +void AfHillClimbing::setPause(controls::AfPauseEnum pause)\n> > > +{\n> > > + if (mode_ != controls::AfModeContinuous) {\n> > > + LOG(Af, Warning)\n> > > + << __FUNCTION__ << \" not valid in mode \" << mode_;\n> >\n> > ditto\n> >\n> > > + return;\n> > > + }\n> > > +\n> > > + switch (pause) {\n> > > + case controls::AfPauseImmediate:\n> > > + pauseState_ = controls::AfPauseStatePaused;\n> > > + break;\n> > > + case controls::AfPauseDeferred:\n> > > + LOG(Af, Warning) << \"AfPauseDeferred is not supported!\";\n> > > + break;\n> > > + case controls::AfPauseResume:\n> > > + pauseState_ = controls::AfPauseStateRunning;\n> > > + break;\n> > > + }\n> > > +}\n> > > +\n> > > +void AfHillClimbing::setLensPosition(float lensPosition)\n> > > +{\n> > > + if (mode_ != controls::AfModeManual) {\n> > > + LOG(Af, Warning)\n> > > + << __FUNCTION__ << \" not valid in mode \" << mode_;\n> > > + return;\n> > > + }\n> > > +\n> > > + lensPosition_ = static_cast(lensPosition);\n> > > +\n> > > + LOG(Af, Debug) << \"Requesting lens position \" << lensPosition_;\n> > > +}\n> > > +\n> > > +void AfHillClimbing::afCoarseScan()\n> > > +{\n> > > + if (coarseCompleted_)\n> > > + return;\n> > > +\n> > > + if (afScan(coarseSearchStep_)) {\n> > > + coarseCompleted_ = true;\n> > > + maxContrast_ = 0;\n> > > + const auto diff = static_cast(\n> > > + std::abs(lensPosition_) * fineRange_);\n> > > + lensPosition_ = std::max(lensPosition_ - diff, minLensPosition_);\n> > > + maxStep_ = std::min(lensPosition_ + diff, maxLensPosition_);\n> > > + }\n> > > +}\n> > > +\n> > > +void AfHillClimbing::afFineScan()\n> > > +{\n> > > + if (!coarseCompleted_)\n> > > + return;\n> > > +\n> > > + if (afScan(fineSearchStep_)) {\n> > > + LOG(Af, Debug) << \"AF found the best focus position!\";\n> > > + state_ = controls::AfStateFocused;\n> > > + }\n> > > +}\n> > > +\n> > > +bool AfHillClimbing::afScan(uint32_t steps)\n> > > +{\n> > > + if (lensPosition_ + static_cast(steps) > maxStep_) {\n> > > + /* If the max step is reached, move lens to the position. */\n> > > + lensPosition_ = bestPosition_;\n> > > + return true;\n> > > + } else {\n> >\n> > If you return in the if() clause , you can remove else { } and save\n> > one indendation level\n>\n> Good points! I will rework the code :)\n>\n> >\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_ += static_cast(steps);\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> > > + LOG(Af, Debug) << \"Previous step is \" << bestPosition_\n> > > + << \", Current step is \" << lensPosition_;\n> > > + return false;\n> > > +}\n> > > +\n> > > +void AfHillClimbing::afReset()\n> > > +{\n> > > + LOG(Af, Debug) << \"Reset AF parameters\";\n> > > + lensPosition_ = minLensPosition_;\n> > > + maxStep_ = maxLensPosition_;\n> > > + state_ = controls::AfStateScanning;\n> > > + coarseCompleted_ = false;\n> > > + maxContrast_ = 0.0;\n> > > + setFramesToSkip(1);\n> > > +}\n> > > +\n> > > +bool AfHillClimbing::afIsOutOfFocus() const\n> > > +{\n> > > + const double diff_var = std::abs(currentContrast_ - maxContrast_);\n> > > + const double var_ratio = diff_var / maxContrast_;\n> > > + LOG(Af, Debug) << \"Variance change rate: \" << var_ratio\n> > > + << \", Current lens step: \" << lensPosition_;\n> > > + return var_ratio > maxChange_;\n> > > +}\n> > > +\n> > > +bool AfHillClimbing::shouldSkipFrame()\n> > > +{\n> > > + if (framesToSkip_ > 0) {\n> > > + framesToSkip_--;\n> > > + return true;\n> > > + }\n> > > +\n> > > + return false;\n> > > +}\n> > > +\n> > > +} /* namespace ipa::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..47d2bbec\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > @@ -0,0 +1,91 @@\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) 2023, Theobroma Systems\n> > > + *\n> > > + * af_hill_climbing.h - AF contrast based hill climbing common algorithm\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +\n> > > +#include \"af.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class YamlObject;\n> > > +\n> > > +namespace ipa::algorithms {\n> > > +\n> > > +LOG_DECLARE_CATEGORY(Af)\n> > > +\n> > > +class AfHillClimbing : public Af\n> >\n> > Isn't it better to declare the class final instead of the single\n> > functions ?\n>\n> It is not the same.\n> When used in a class definition, final specifies\n> that the class cannot be derived from.\n\nYep\n\n> When used in a virtual function declaration or definition, final\n> specifier ensures that the function is virtual and specifies that it\n> may not be overridden by derived classes.\n\nI almost agree. final when applied to a virtual function ensures that\nit cannot be overriden in derived classes.\n\nThe part I don't get in you comment is \"final specifier ensures that\nthe function is -virtual-\" the rest I agree on.\n\n> It is technically possible to derive from AfHillClimbing to build\n> on it, even if it was meant to be used in composition style.\n> Do We want to force a composition approach or leave possibility to\n> derive from it?\n\nThe thing that doesn't click for me is that you have declared as final\nthe methods that implement the AF state machine, which is agnostic\nfrom the HillClimbing algorithm. Is this correct ?\n\n\tvoid setMode(controls::AfModeEnum mode) final;\n\tvoid setRange(controls::AfRangeEnum range) final;\n\tvoid setSpeed(controls::AfSpeedEnum speed) final;\n\tvoid setMeteringMode(controls::AfMeteringEnum metering) final;\n\tvoid setWindows(Span windows) final;\n\tvoid setTrigger(controls::AfTriggerEnum trigger) final;\n\tvoid setPause(controls::AfPauseEnum pause) final;\n\tvoid setLensPosition(float lensPosition) final;\n\n\tvoid processAutoMode();\n\tvoid processContinuousMode();\n\tvoid afCoarseScan();\n\tvoid afFineScan();\n\tbool afScan(uint32_t steps);\n\tvoid afReset();\n\t[[nodiscard]] bool afIsOutOfFocus() const;\n\tbool shouldSkipFrame();\n\nAs I see it, the algorithm state machine method might later be broken\nout to a common AfBase class (or even making the Af class non-purely\nvirtual) as they do not depend on the contrast-computation method.\n\nIf one would like to derive from AfHillClimbing I guess it would be to\ntweak the algorithm's computation, not the state machine which adheres\nto the libcamera's controls definition. That's why I was suggesting\nthat this fine-grained control on what can be overridden is probably a\nbit premature.\n\n>\n> Best regards\n> Daniel\n>\n> >\n> > class AfHillClimbing final : public Af\n> >\n> > > +{\n> > > +public:\n> > > + int init(const YamlObject &tuningData);\n> > > + void configure(int32_t minFocusPosition, int32_t maxFocusPosition);\n> > > + int32_t process(double currentContrast);\n> > > + void setFramesToSkip(uint32_t n);\n> > > +\n> > > + controls::AfStateEnum state() final { return state_; }\n> > > + controls::AfPauseStateEnum pauseState() final { return pauseState_; }\n> > > +\n> > > +private:\n> > > + void setMode(controls::AfModeEnum mode) final;\n> > > + void setRange(controls::AfRangeEnum range) final;\n> > > + void setSpeed(controls::AfSpeedEnum speed) final;\n> > > + void setMeteringMode(controls::AfMeteringEnum metering) final;\n> > > + void setWindows(Span windows) final;\n> > > + void setTrigger(controls::AfTriggerEnum trigger) final;\n> > > + void setPause(controls::AfPauseEnum pause) final;\n> > > + void setLensPosition(float lensPosition) final;\n> >\n> > These functions should have the 'override' specifier\n> >\n> > > +\n> > > + void processAutoMode();\n> > > + void processContinuousMode();\n> > > + void afCoarseScan();\n> > > + void afFineScan();\n> > > + bool afScan(uint32_t steps);\n> > > + void afReset();\n> > > + [[nodiscard]] bool afIsOutOfFocus() const;\n> > > + bool shouldSkipFrame();\n> > > +\n> > > + controls::AfModeEnum mode_ = controls::AfModeManual;\n> > > + controls::AfStateEnum state_ = controls::AfStateIdle;\n> > > + controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning;\n> > > +\n> > > + /* Current focus lens position. */\n> > > + int32_t lensPosition_ = 0;\n> > > + /* Local optimum focus lens position during scanning. */\n> > > + int32_t bestPosition_ = 0;\n> > > +\n> > > + /* Current AF statistic contrast. */\n> > > + double currentContrast_ = 0;\n> > > + /* It is used to determine the derivative during scanning */\n> > > + double maxContrast_ = 0;\n> > > + /* The designated maximum range of focus scanning. */\n> > > + int32_t maxStep_ = 0;\n> > > + /* If the coarse scan completes, it is set to true. */\n> > > + bool coarseCompleted_ = false;\n> > > +\n> > > + uint32_t framesToSkip_ = 0;\n> > > +\n> > > + /* Position limits of the focus lens. */\n> > > + int32_t minLensPosition_;\n> > > + int32_t maxLensPosition_;\n> > > +\n> > > + /* Minimum position step for searching appropriate focus. */\n> > > + uint32_t coarseSearchStep_;\n> > > + uint32_t fineSearchStep_;\n> > > +\n> > > + /* Fine scan range 0 < fineRange_ < 1. */\n> > > + double fineRange_;\n> > > +\n> > > + /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */\n> > > + double maxChange_;\n> > > +};\n> > > +\n> > > +} /* namespace ipa::algorithms */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> > > index 3df4798f..20e437fc 100644\n> > > --- a/src/ipa/libipa/algorithms/meson.build\n> > > +++ b/src/ipa/libipa/algorithms/meson.build\n> > > @@ -2,8 +2,10 @@\n> > >\n> > > libipa_algorithms_headers = files([\n> > > 'af.h',\n> > > + 'af_hill_climbing.h',\n> > > ])\n> > >\n> > > libipa_algorithms_sources = files([\n> > > 'af.cpp',\n> > > + 'af_hill_climbing.cpp',\n> > > ])\n> > > --\n> > > 2.39.2\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 52C9BC0F2A\n\tfor ;\n\tMon, 27 Mar 2023 13:03:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9694662726;\n\tMon, 27 Mar 2023 15:03:20 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9BDC26271C\n\tfor ;\n\tMon, 27 Mar 2023 15:03:19 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1817BAEC;\n\tMon, 27 Mar 2023 15:03:19 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679922200;\n\tbh=le8ieZIXwK+jgj29koDpiqHt/44X/9Q4zQqvmH3E5NI=;\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=RUbgDhvWur76bk7KuLoD47FxFePTCqiU4IgDwX5pDe+OzRRbvLSTkim4pgGrr63Wp\n\tZeu9FEAIxC08/iMu3/2CxjGCn+fudSB736k4FTazIFfAUCKmQfqIUE/BkZMmJqXKWI\n\tYIOd8KRqkl426xdYcOu8lJt/apn4zb2De61y2ZmpyzU66AlNTEQWcu8pOSwI5gLdx0\n\tU13TnJ3MVPhQVF28SsQOQf+sOa0EZ29hsh/NYcAThUwetcFJe6gkKutePai2otmUov\n\tHe+3zvlxzayvsQf1VLk7fSXqIJvZoAT2wo/OEW4cgqrhIi87SIiBl6s/H4wpOPzwb1\n\tb7B/68T6Vd/ew==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1679922199;\n\tbh=le8ieZIXwK+jgj29koDpiqHt/44X/9Q4zQqvmH3E5NI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AFvv37jImFYDE+uVOm4YaejknEYisNd1hrEQwzxiXTBF9jqeYbrBAgbhXfmicPs7Z\n\t+ZF0R4rkfZmOJJviAe9ejmzwS5JbJic/4MyN/wGNKkkAS1KH4Wf4PTDuIOBELDJNYa\n\tDwjPLagfr+bBtZXAe3sBSreihNC6RjCQFUoDoC+w="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"AFvv37jI\"; dkim-atps=neutral","Date":"Mon, 27 Mar 2023 15:03:15 +0200","To":"Daniel Semkowicz ","Message-ID":"<20230327130315.mw5cqmkt3jtpjwdq@uno.localdomain>","References":"<20230324142908.64224-1-dse@thaumatec.com>\n\t<20230324142908.64224-5-dse@thaumatec.com>\n\t<20230324153040.sk43lwg36qmxwz6t@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 04/10] ipa: Add common contrast\n\tbased AF implementation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26780,"web_url":"https://patchwork.libcamera.org/comment/26780/","msgid":"","date":"2023-03-28T05:42:25","subject":"Re: [libcamera-devel] [PATCH v5 04/10] ipa: Add common contrast\n\tbased AF implementation","submitter":{"id":126,"url":"https://patchwork.libcamera.org/api/people/126/","name":"Daniel Semkowicz","email":"dse@thaumatec.com"},"content":"Hi Jacopo,\n\nOn Mon, Mar 27, 2023 at 3:03 PM Jacopo Mondi\n wrote:\n>\n> Hi Daniel\n>\n> On Mon, Mar 27, 2023 at 02:29:45PM +0200, Daniel Semkowicz wrote:\n> > Hi Jacopo,\n> >\n> > On Fri, Mar 24, 2023 at 4:30 PM Jacopo Mondi\n> > wrote:\n> > >\n> > > Hi Daniel\n> > >\n> > > On Fri, Mar 24, 2023 at 03:29:02PM +0100, Daniel Semkowicz via libcamera-devel wrote:\n> > > > Create a new class with contrast based Auto Focus implementation\n> > > > using hill climbing algorithm. This common implementation is independent\n> > > > of platform specific code. This way, each platform can just implement\n> > > > contrast calculation and run the AF control loop basing on this class.\n> > > > This implementation is based on the code that was common for IPU3\n> > > > and RPi AF algorithms.\n> > > >\n> > > > Signed-off-by: Daniel Semkowicz \n> > > > ---\n> > > > .../libipa/algorithms/af_hill_climbing.cpp | 379 ++++++++++++++++++\n> > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 91 +++++\n> > > > src/ipa/libipa/algorithms/meson.build | 2 +\n> > > > 3 files changed, 472 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..6ee090eb\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > @@ -0,0 +1,379 @@\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) 2023, Theobroma Systems\n> > > > + *\n> > > > + * af_hill_climbing.cpp - AF contrast based hill climbing common algorithm\n> > > > + */\n> > > > +\n> > > > +#include \"af_hill_climbing.h\"\n> > > > +\n> > > > +#include \"libcamera/internal/yaml_parser.h\"\n> > > > +\n> > > > +/**\n> > > > + * \\file af_hill_climbing.h\n> > > > + * \\brief AF contrast based hill climbing common algorithm\n> > > > + */\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +namespace ipa::algorithms {\n> > > > +\n> > > > +LOG_DEFINE_CATEGORY(Af)\n> > > > +\n> > > > +/**\n> > > > + * \\class AfHillClimbing\n> > > > + * \\brief Contrast based hill climbing auto focus control algorithm\n> > > > + * implementation\n> > > > + *\n> > > > + * Control part of the auto focus algorithm. It calculates the lens position\n> > > > + * based on the contrast measure supplied by the platform-specific\n> > > > + * implementation. This way it is independent from the platform.\n> > > > + *\n> > > > + * Platform layer that use this algorithm should call process() function\n> > > > + * for each each frame and set the lens to the calculated position.\n> > > > + *\n> > > > + * Focus search is divided into two phases:\n> > > > + * 1. coarse search,\n> > > > + * 2. fine search.\n> > > > + *\n> > > > + * In the first phase, the lens is moved with bigger steps to quickly find\n> > > > + * a rough position for the best focus. Then, based on the outcome of coarse\n> > > > + * search, the second phase is executed. Lens is moved with smaller steps\n> > > > + * in a limited range within the rough position to find the exact position\n> > > > + * for best focus.\n> > > > + *\n> > > > + * Tuning file parameters:\n> > > > + * - **coarse-search-step:** The value by which the lens position will change\n> > > > + * in one step in the *coarse search* phase. Unit is lens specific.\n> > > > + * - **fine-search-step:** The value by which the lens position will change\n> > > > + * in one step in the *fine search* phase. Unit is lens specific.\n> > > > + * - **fine-search-range:** Search range in the *fine search* phase, expressed\n> > > > + * as a percentage of the coarse search result. Valid values are\n> > > > + * in the [0, 100] interval. Value 5 means 5%. If coarse search stopped\n> > > > + * at value 200, the fine search range will be [190, 210].\n> > > > + * - **max-variance-change:** ratio of contrast variance change in the\n> > > > + * *continuous mode* needed for triggering the focus change. When the variance\n> > > > + * change exceeds this value, focus search will be triggered. Valid values are\n> > > > + * in the [0.0, 1.0] interval.\n> > > > + * .\n> > > > + *\n> > > > + * \\todo Search range in the *fine search* phase should depend on the lens\n> > > > + * movement range rather than coarse search result.\n> > > > + * \\todo Implement setRange.\n> > > > + * \\todo Implement setSpeed.\n> > > > + * \\todo Implement setMeteringMode.\n> > > > + * \\todo Implement setWindows.\n> > > > + * \\todo Implement the AfPauseDeferred mode.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\brief Initialize the AfHillClimbing with tuning data\n> > > > + * \\param[in] tuningData The tuning data for the algorithm\n> > > > + *\n> > > > + * This method should be called in the libcamera::ipa::Algorithm::init()\n> > > > + * method of the platform layer.\n> > > > + *\n> > > > + * \\return 0 if successful, an error code otherwise\n> > > > + */\n> > > > +int AfHillClimbing::init(const YamlObject &tuningData)\n> > > > +{\n> > > > + coarseSearchStep_ = tuningData[\"coarse-search-step\"].get(30);\n> > > > + fineSearchStep_ = tuningData[\"fine-search-step\"].get(1);\n> > > > + fineRange_ = tuningData[\"fine-search-range\"].get(5);\n> > > > + fineRange_ /= 100;\n> > > > + maxChange_ = tuningData[\"max-variance-change\"].get(0.5);\n> > > > +\n> > > > + LOG(Af, Debug) << \"coarseSearchStep_: \" << coarseSearchStep_\n> > > > + << \", fineSearchStep_: \" << fineSearchStep_\n> > > > + << \", fineRange_: \" << fineRange_\n> > > > + << \", maxChange_: \" << maxChange_;\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Configure the AfHillClimbing with sensor and lens information\n> > > > + * \\param[in] minFocusPosition Minimum position supported by camera lens\n> > > > + * \\param[in] maxFocusPosition Maximum position supported by camera lens\n> > > > + *\n> > > > + * This method should be called in the libcamera::ipa::Algorithm::configure()\n> > > > + * method of the platform layer.\n> > > > + */\n> > > > +void AfHillClimbing::configure(int32_t minFocusPosition,\n> > > > + int32_t maxFocusPosition)\n> > > > +{\n> > > > + minLensPosition_ = minFocusPosition;\n> > > > + maxLensPosition_ = maxFocusPosition;\n> > > > +}\n> > > > +\n> > > > +/**\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 in the libcamera::ipa::Algorithm::process()\n> > > > + * method of the platform layer for each frame.\n> > > > + *\n> > > > + * Contrast value supplied in the \\p currentContrast parameter can be platform\n> > > > + * specific. The only requirement is the contrast value must increase with\n> > > > + * the increasing image focus. Contrast value must be highest when image is in\n> > > > + * focus.\n> > > > + *\n> > > > + * \\return New lens position calculated by the AF algorithm\n> > > > + */\n> > > > +int32_t AfHillClimbing::process(double currentContrast)\n> > > > +{\n> > > > + currentContrast_ = currentContrast;\n> > > > +\n> > > > + if (shouldSkipFrame())\n> > > > + return lensPosition_;\n> > > > +\n> > > > + switch (mode_) {\n> > > > + case controls::AfModeManual:\n> > > > + /* Nothing to process. */\n> > > > + break;\n> > > > + case controls::AfModeAuto:\n> > > > + processAutoMode();\n> > > > + break;\n> > > > + case controls::AfModeContinuous:\n> > > > + processContinuousMode();\n> > > > + break;\n> > > > + }\n> > > > +\n> > > > + return lensPosition_;\n> > > > +}\n> > > > +\n> > > > +void AfHillClimbing::processAutoMode()\n> > > > +{\n> > > > + if (state_ == controls::AfStateScanning) {\n> > > > + afCoarseScan();\n> > > > + afFineScan();\n> > > > + }\n> > > > +}\n> > > > +\n> > > > +void AfHillClimbing::processContinuousMode()\n> > > > +{\n> > > > + /* If we are in a paused state, we won't process the stats. */\n> > > > + if (pauseState_ == controls::AfPauseStatePaused)\n> > > > + return;\n> > > > +\n> > > > + if (state_ == controls::AfStateScanning) {\n> > > > + afCoarseScan();\n> > > > + afFineScan();\n> > > > + return;\n> > > > + }\n> > > > +\n> > > > + /*\n> > > > + * AF scan can be started at any moment in AfModeContinuous,\n> > > > + * except when the state is already AfStateScanning.\n> > > > + */\n> > > > + if (afIsOutOfFocus())\n> > > > + afReset();\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Request AF to skip n frames\n> > > > + * \\param[in] n Number of frames to be skipped\n> > > > + *\n> > > > + * For the requested number of frames, the AF calculation will be skipped\n> > > > + * and lens position will not change. The platform layer still needs to\n> > > > + * call process() function for each frame during this time.\n> > > > + * This function can be used by the platform layer if the hardware needs more\n> > > > + * time for some operations.\n> > > > + *\n> > > > + * The number of the requested frames (\\p n) will be applied only if \\p n has\n> > > > + * higher value than the number of frames already requested to be skipped.\n> > > > + * For example, if *setFramesToSkip(5)* was already called for the current\n> > > > + * frame, then calling *setFramesToSkip(3)* will not override the previous\n> > > > + * request and 5 frames will be skipped.\n> > >\n> > > This still puzzles me a bit as it is not clear to me when the platform\n> > > layer is supposed to set the number of frames to skip.\n> > >\n> > > Looking at the implementation it happens everytime the lens is moved.\n> > > I guess this is fine for now, but this will need to be handled better\n> > > by the CameraLens class modeling the lens movement delay.\n> > >\n> >\n> > In RkISP it is also needed when setting AF Windows. ISP needs a one\n> > additional frame to calculate the new statistics after setting window.\n> > This is why I exposed this method as public.\n> >\n> > > > + */\n> > > > +void AfHillClimbing::setFramesToSkip(uint32_t n)\n> > > > +{\n> > > > + if (n > framesToSkip_)\n> > > > + framesToSkip_ = n;\n> > > > +}\n> > > > +\n> > > > +void AfHillClimbing::setMode(controls::AfModeEnum mode)\n> > > > +{\n> > > > + if (mode == mode_)\n> > > > + return;\n> > > > +\n> > > > + LOG(Af, Debug) << \"Switched AF mode from \" << mode_ << \" to \" << mode;\n> > > > + mode_ = mode;\n> > > > +\n> > > > + state_ = controls::AfStateIdle;\n> > > > + pauseState_ = controls::AfPauseStateRunning;\n> > > > +\n> > > > + if (mode_ == controls::AfModeContinuous)\n> > > > + afReset();\n> > > > +}\n> > > > +\n> > > > +void AfHillClimbing::setRange([[maybe_unused]] controls::AfRangeEnum range)\n> > > > +{\n> > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > +}\n> > > > +\n> > > > +void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed)\n> > > > +{\n> > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > +}\n> > > > +\n> > > > +void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering)\n> > > > +{\n> > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > +}\n> > > > +\n> > > > +void AfHillClimbing::setWindows([[maybe_unused]] Span windows)\n> > > > +{\n> > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > +}\n> > > > +\n> > >\n> > > I'm still of the idea you should better not implement these functions\n> > > at all. If anything tries to call into them will fail at compile time\n> > > instead of getting a run-time error in the logs\n> >\n> > These functions are pure virtual, so you are forced by compiler\n> > to implement them.\n> >\n>\n> Ah! I removed the definition in the cpp file but I left the\n> declaration in the header, so I guess it worked here because of the\n> compiler generated one for me. Sorry for the overlook\n>\n> > >\n> > > > +void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)\n> > > > +{\n> > > > + if (mode_ != controls::AfModeAuto) {\n> > > > + LOG(Af, Warning)\n> > > > + << __FUNCTION__ << \" not valid in mode \" << mode_;\n> > >\n> > > We don't use __FUNCTION__ for the reason that as far as I know it's\n> > > not a standard language keyword (while __func__ is iirc). I'm also not\n> > > sure how it works with name mangling as I see gcc has a\n> > > __PRETTY_FUNCTION__ specific extensions to beautify names.\n> > >\n> > > I would just\n> > > << \"setTrigger() not valid in mode \" << mode_;\n> > >\n> > > > + return;\n> > > > + }\n> > > > +\n> > > > + LOG(Af, Debug) << \"Trigger called with \" << trigger;\n> > > > +\n> > > > + if (trigger == controls::AfTriggerStart)\n> > > > + afReset();\n> > > > + else\n> > > > + state_ = controls::AfStateIdle;\n> > >\n> > > I would switch to be sure to handle all cases. A minor though\n> > >\n> > > > +}\n> > > > +\n> > > > +void AfHillClimbing::setPause(controls::AfPauseEnum pause)\n> > > > +{\n> > > > + if (mode_ != controls::AfModeContinuous) {\n> > > > + LOG(Af, Warning)\n> > > > + << __FUNCTION__ << \" not valid in mode \" << mode_;\n> > >\n> > > ditto\n> > >\n> > > > + return;\n> > > > + }\n> > > > +\n> > > > + switch (pause) {\n> > > > + case controls::AfPauseImmediate:\n> > > > + pauseState_ = controls::AfPauseStatePaused;\n> > > > + break;\n> > > > + case controls::AfPauseDeferred:\n> > > > + LOG(Af, Warning) << \"AfPauseDeferred is not supported!\";\n> > > > + break;\n> > > > + case controls::AfPauseResume:\n> > > > + pauseState_ = controls::AfPauseStateRunning;\n> > > > + break;\n> > > > + }\n> > > > +}\n> > > > +\n> > > > +void AfHillClimbing::setLensPosition(float lensPosition)\n> > > > +{\n> > > > + if (mode_ != controls::AfModeManual) {\n> > > > + LOG(Af, Warning)\n> > > > + << __FUNCTION__ << \" not valid in mode \" << mode_;\n> > > > + return;\n> > > > + }\n> > > > +\n> > > > + lensPosition_ = static_cast(lensPosition);\n> > > > +\n> > > > + LOG(Af, Debug) << \"Requesting lens position \" << lensPosition_;\n> > > > +}\n> > > > +\n> > > > +void AfHillClimbing::afCoarseScan()\n> > > > +{\n> > > > + if (coarseCompleted_)\n> > > > + return;\n> > > > +\n> > > > + if (afScan(coarseSearchStep_)) {\n> > > > + coarseCompleted_ = true;\n> > > > + maxContrast_ = 0;\n> > > > + const auto diff = static_cast(\n> > > > + std::abs(lensPosition_) * fineRange_);\n> > > > + lensPosition_ = std::max(lensPosition_ - diff, minLensPosition_);\n> > > > + maxStep_ = std::min(lensPosition_ + diff, maxLensPosition_);\n> > > > + }\n> > > > +}\n> > > > +\n> > > > +void AfHillClimbing::afFineScan()\n> > > > +{\n> > > > + if (!coarseCompleted_)\n> > > > + return;\n> > > > +\n> > > > + if (afScan(fineSearchStep_)) {\n> > > > + LOG(Af, Debug) << \"AF found the best focus position!\";\n> > > > + state_ = controls::AfStateFocused;\n> > > > + }\n> > > > +}\n> > > > +\n> > > > +bool AfHillClimbing::afScan(uint32_t steps)\n> > > > +{\n> > > > + if (lensPosition_ + static_cast(steps) > maxStep_) {\n> > > > + /* If the max step is reached, move lens to the position. */\n> > > > + lensPosition_ = bestPosition_;\n> > > > + return true;\n> > > > + } else {\n> > >\n> > > If you return in the if() clause , you can remove else { } and save\n> > > one indendation level\n> >\n> > Good points! I will rework the code :)\n> >\n> > >\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_ += static_cast(steps);\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> > > > + LOG(Af, Debug) << \"Previous step is \" << bestPosition_\n> > > > + << \", Current step is \" << lensPosition_;\n> > > > + return false;\n> > > > +}\n> > > > +\n> > > > +void AfHillClimbing::afReset()\n> > > > +{\n> > > > + LOG(Af, Debug) << \"Reset AF parameters\";\n> > > > + lensPosition_ = minLensPosition_;\n> > > > + maxStep_ = maxLensPosition_;\n> > > > + state_ = controls::AfStateScanning;\n> > > > + coarseCompleted_ = false;\n> > > > + maxContrast_ = 0.0;\n> > > > + setFramesToSkip(1);\n> > > > +}\n> > > > +\n> > > > +bool AfHillClimbing::afIsOutOfFocus() const\n> > > > +{\n> > > > + const double diff_var = std::abs(currentContrast_ - maxContrast_);\n> > > > + const double var_ratio = diff_var / maxContrast_;\n> > > > + LOG(Af, Debug) << \"Variance change rate: \" << var_ratio\n> > > > + << \", Current lens step: \" << lensPosition_;\n> > > > + return var_ratio > maxChange_;\n> > > > +}\n> > > > +\n> > > > +bool AfHillClimbing::shouldSkipFrame()\n> > > > +{\n> > > > + if (framesToSkip_ > 0) {\n> > > > + framesToSkip_--;\n> > > > + return true;\n> > > > + }\n> > > > +\n> > > > + return false;\n> > > > +}\n> > > > +\n> > > > +} /* namespace ipa::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..47d2bbec\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > @@ -0,0 +1,91 @@\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) 2023, Theobroma Systems\n> > > > + *\n> > > > + * af_hill_climbing.h - AF contrast based hill climbing common algorithm\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"af.h\"\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +class YamlObject;\n> > > > +\n> > > > +namespace ipa::algorithms {\n> > > > +\n> > > > +LOG_DECLARE_CATEGORY(Af)\n> > > > +\n> > > > +class AfHillClimbing : public Af\n> > >\n> > > Isn't it better to declare the class final instead of the single\n> > > functions ?\n> >\n> > It is not the same.\n> > When used in a class definition, final specifies\n> > that the class cannot be derived from.\n>\n> Yep\n>\n> > When used in a virtual function declaration or definition, final\n> > specifier ensures that the function is virtual and specifies that it\n> > may not be overridden by derived classes.\n>\n> I almost agree. final when applied to a virtual function ensures that\n> it cannot be overriden in derived classes.\n>\n> The part I don't get in you comment is \"final specifier ensures that\n> the function is -virtual-\" the rest I agree on.\n\nWhen \"final\" is used in a function definition, it implies function\nto be virtual. This is why I said it is a different for class and\nfunction. But now I see, I probably misunderstood your intention\nand your comment was more about architecture - what should be final.\nNot that class final should replace function finals as I understood it.\n\n>\n> > It is technically possible to derive from AfHillClimbing to build\n> > on it, even if it was meant to be used in composition style.\n> > Do We want to force a composition approach or leave possibility to\n> > derive from it?\n>\n> The thing that doesn't click for me is that you have declared as final\n> the methods that implement the AF state machine, which is agnostic\n> from the HillClimbing algorithm. Is this correct ?\n>\n> void setMode(controls::AfModeEnum mode) final;\n> void setRange(controls::AfRangeEnum range) final;\n> void setSpeed(controls::AfSpeedEnum speed) final;\n> void setMeteringMode(controls::AfMeteringEnum metering) final;\n> void setWindows(Span windows) final;\n> void setTrigger(controls::AfTriggerEnum trigger) final;\n> void setPause(controls::AfPauseEnum pause) final;\n> void setLensPosition(float lensPosition) final;\n>\n> void processAutoMode();\n> void processContinuousMode();\n> void afCoarseScan();\n> void afFineScan();\n> bool afScan(uint32_t steps);\n> void afReset();\n> [[nodiscard]] bool afIsOutOfFocus() const;\n> bool shouldSkipFrame();\n>\n> As I see it, the algorithm state machine method might later be broken\n> out to a common AfBase class (or even making the Af class non-purely\n> virtual) as they do not depend on the contrast-computation method.\n>\n> If one would like to derive from AfHillClimbing I guess it would be to\n> tweak the algorithm's computation, not the state machine which adheres\n> to the libcamera's controls definition. That's why I was suggesting\n> that this fine-grained control on what can be overridden is probably a\n> bit premature.\n\nThis is a valid point. Algorithms class hierarchy (and especially this\none) is in early stage. Then maybe I should drop the final specifiers\ncompletely in this class? As it is hard to decide the future direction\nat this point?\n\n>\n> >\n> > Best regards\n> > Daniel\n> >\n> > >\n> > > class AfHillClimbing final : public Af\n> > >\n> > > > +{\n> > > > +public:\n> > > > + int init(const YamlObject &tuningData);\n> > > > + void configure(int32_t minFocusPosition, int32_t maxFocusPosition);\n> > > > + int32_t process(double currentContrast);\n> > > > + void setFramesToSkip(uint32_t n);\n> > > > +\n> > > > + controls::AfStateEnum state() final { return state_; }\n> > > > + controls::AfPauseStateEnum pauseState() final { return pauseState_; }\n> > > > +\n> > > > +private:\n> > > > + void setMode(controls::AfModeEnum mode) final;\n> > > > + void setRange(controls::AfRangeEnum range) final;\n> > > > + void setSpeed(controls::AfSpeedEnum speed) final;\n> > > > + void setMeteringMode(controls::AfMeteringEnum metering) final;\n> > > > + void setWindows(Span windows) final;\n> > > > + void setTrigger(controls::AfTriggerEnum trigger) final;\n> > > > + void setPause(controls::AfPauseEnum pause) final;\n> > > > + void setLensPosition(float lensPosition) final;\n> > >\n> > > These functions should have the 'override' specifier\n> > >\n> > > > +\n> > > > + void processAutoMode();\n> > > > + void processContinuousMode();\n> > > > + void afCoarseScan();\n> > > > + void afFineScan();\n> > > > + bool afScan(uint32_t steps);\n> > > > + void afReset();\n> > > > + [[nodiscard]] bool afIsOutOfFocus() const;\n> > > > + bool shouldSkipFrame();\n> > > > +\n> > > > + controls::AfModeEnum mode_ = controls::AfModeManual;\n> > > > + controls::AfStateEnum state_ = controls::AfStateIdle;\n> > > > + controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning;\n> > > > +\n> > > > + /* Current focus lens position. */\n> > > > + int32_t lensPosition_ = 0;\n> > > > + /* Local optimum focus lens position during scanning. */\n> > > > + int32_t bestPosition_ = 0;\n> > > > +\n> > > > + /* Current AF statistic contrast. */\n> > > > + double currentContrast_ = 0;\n> > > > + /* It is used to determine the derivative during scanning */\n> > > > + double maxContrast_ = 0;\n> > > > + /* The designated maximum range of focus scanning. */\n> > > > + int32_t maxStep_ = 0;\n> > > > + /* If the coarse scan completes, it is set to true. */\n> > > > + bool coarseCompleted_ = false;\n> > > > +\n> > > > + uint32_t framesToSkip_ = 0;\n> > > > +\n> > > > + /* Position limits of the focus lens. */\n> > > > + int32_t minLensPosition_;\n> > > > + int32_t maxLensPosition_;\n> > > > +\n> > > > + /* Minimum position step for searching appropriate focus. */\n> > > > + uint32_t coarseSearchStep_;\n> > > > + uint32_t fineSearchStep_;\n> > > > +\n> > > > + /* Fine scan range 0 < fineRange_ < 1. */\n> > > > + double fineRange_;\n> > > > +\n> > > > + /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */\n> > > > + double maxChange_;\n> > > > +};\n> > > > +\n> > > > +} /* namespace ipa::algorithms */\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> > > > index 3df4798f..20e437fc 100644\n> > > > --- a/src/ipa/libipa/algorithms/meson.build\n> > > > +++ b/src/ipa/libipa/algorithms/meson.build\n> > > > @@ -2,8 +2,10 @@\n> > > >\n> > > > libipa_algorithms_headers = files([\n> > > > 'af.h',\n> > > > + 'af_hill_climbing.h',\n> > > > ])\n> > > >\n> > > > libipa_algorithms_sources = files([\n> > > > 'af.cpp',\n> > > > + 'af_hill_climbing.cpp',\n> > > > ])\n> > > > --\n> > > > 2.39.2\n> > > >\n\nBest regards\nDaniel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D0E1DBD160\n\tfor ;\n\tTue, 28 Mar 2023 05:42:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EE3F262734;\n\tTue, 28 Mar 2023 07:42:38 +0200 (CEST)","from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com\n\t[IPv6:2a00:1450:4864:20::52a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E764D61ECA\n\tfor ;\n\tTue, 28 Mar 2023 07:42:36 +0200 (CEST)","by mail-ed1-x52a.google.com with SMTP id eg48so44809479edb.13\n\tfor ;\n\tMon, 27 Mar 2023 22:42:36 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679982159;\n\tbh=zxrNPL5XKp0PMQ94dlbiN/fh9+ng2WgwBEekoqQdudk=;\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=vH6O25nVMjjJqf7wIMb7MrU3l3DN6DrNUcf36gXNHtSe2OwAn3sBZGuI5iysfpLq3\n\tJusUKftvaGXoWxSRXf6+e/lP6EuX7uFnlE+41GGzukUrs0nkkIU6/7xbKF8kC592PE\n\tIWByt+1TV33etNPgQFoYds3057zXrFYun3RTB520aIz6OJ957Dwm8NIkyjyhUlb0l5\n\t78fzU9q5DtE4qMiwHS4NJA698XUarqB+BG9eXg7bIWw/fLQZfEaWs7z5mTABkcdQhh\n\tE4C3FyaRpOvwX+Vkk3sNLjkrq5zX4FHl/WVCpkAihd/keO67px/H599ba8RNKcl8Ju\n\tmkAx1ajGvxaug==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20210112.gappssmtp.com; s=20210112; t=1679982156;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=YY5ujzknBckt6rlZvOi0LrCn6Ki0hzumm12YoOHV6Xc=;\n\tb=04jM4I/hed+1GTH0VPyJRp0dEHo/FBkGpw5+sar3r4IYu9e6soxoJEedkdUNF5S45M\n\toDgEU9IrMfbka/3+1k1RvnjmYGRrBtdvuip4pF7PVXMJScwEfTHUT1X9PUDRGMjzIbnT\n\tTykiEAKDI0DxZTItg7E7ujTjQKEbaZ5EH5A+e5Q26k/Rhyd+vkBjOuTPz2hEKzYO6lLS\n\tivJK8OwuWTkZZIT13bkixEqdc6a9Xw19/Ybh8Ohh4qBfHOq9+c8KrGzFCwA+MGD9Q9O9\n\tRU/jQeTNE1o3xJbfIdpa/EDT3L1YUOQShVi1SnuMaJxf6NazQvQfTDh4yQTZ0QQudZe0\n\ttN/g=="],"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=\"04jM4I/h\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1679982156;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=YY5ujzknBckt6rlZvOi0LrCn6Ki0hzumm12YoOHV6Xc=;\n\tb=n2KKfDAHyjSWjVfBnUdrY5Xw6DsHUm/KOYYkhXAYefFrNkVVCEDQSU9EZYn3kC1WmD\n\tjZ1lgy6ENmVSd00Jly63oIAMWymiWSPQ/ubRcWeeQEPpoHaPIvEH64y+GPR6UAhaUGbF\n\t9Vr2L3/fvg7FAX1AgGjcuaSN80mz9YW9w1INGqsRUGKl+Fn8LwINNdZixzzTjylDIUtF\n\tcXQ1oJAzpLN7I2/Vn2Jp0jhefmCs2J2w0BPve9nb9x9dxU3q79un5aFA046ALeV2ubSe\n\tBhTHjKxXPpJnnIl7OmburiafrTxPLHjISCTKr/27it00sShoCnz9qE3uWyKVc1NFYpwU\n\tquwA==","X-Gm-Message-State":"AAQBX9eFrBF2gLTHkg4aXdQtOTVkxIhDkaWQxhC+NlijcSikxxzeTunp\n\tawLXVHPv7pfOIsmCh5Ow0fjAmlad4NIP1fygclXflqMraJMM0MylcTcbqg==","X-Google-Smtp-Source":"AKy350YudP6X2jhaY1Nm1LwDL8MhVfqNh2Ilm7C84sx+iEvXG22RGMcYxgWYeSr54vqZfPHrowy+R2r5gchg37f/R60=","X-Received":"by 2002:a50:f69e:0:b0:4fc:8749:cd77 with SMTP id\n\td30-20020a50f69e000000b004fc8749cd77mr7161704edn.3.1679982156178;\n\tMon, 27 Mar 2023 22:42:36 -0700 (PDT)","MIME-Version":"1.0","References":"<20230324142908.64224-1-dse@thaumatec.com>\n\t<20230324142908.64224-5-dse@thaumatec.com>\n\t<20230324153040.sk43lwg36qmxwz6t@uno.localdomain>\n\t\n\t<20230327130315.mw5cqmkt3jtpjwdq@uno.localdomain>","In-Reply-To":"<20230327130315.mw5cqmkt3jtpjwdq@uno.localdomain>","Date":"Tue, 28 Mar 2023 07:42:25 +0200","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v5 04/10] ipa: Add common contrast\n\tbased AF implementation","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":26781,"web_url":"https://patchwork.libcamera.org/comment/26781/","msgid":"<20230328070722.puushui732znpnbu@uno.localdomain>","date":"2023-03-28T07:07:22","subject":"Re: [libcamera-devel] [PATCH v5 04/10] ipa: Add common contrast\n\tbased AF implementation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Daniel\nG\nOn Tue, Mar 28, 2023 at 07:42:25AM +0200, Daniel Semkowicz wrote:\n> Hi Jacopo,\n>\n> On Mon, Mar 27, 2023 at 3:03 PM Jacopo Mondi\n> wrote:\n> >\n> > Hi Daniel\n> >\n> > On Mon, Mar 27, 2023 at 02:29:45PM +0200, Daniel Semkowicz wrote:\n> > > Hi Jacopo,\n> > >\n> > > On Fri, Mar 24, 2023 at 4:30 PM Jacopo Mondi\n> > > wrote:\n> > > >\n> > > > Hi Daniel\n> > > >\n> > > > On Fri, Mar 24, 2023 at 03:29:02PM +0100, Daniel Semkowicz via libcamera-devel wrote:\n> > > > > Create a new class with contrast based Auto Focus implementation\n> > > > > using hill climbing algorithm. This common implementation is independent\n> > > > > of platform specific code. This way, each platform can just implement\n> > > > > contrast calculation and run the AF control loop basing on this class.\n> > > > > This implementation is based on the code that was common for IPU3\n> > > > > and RPi AF algorithms.\n> > > > >\n> > > > > Signed-off-by: Daniel Semkowicz \n> > > > > ---\n> > > > > .../libipa/algorithms/af_hill_climbing.cpp | 379 ++++++++++++++++++\n> > > > > src/ipa/libipa/algorithms/af_hill_climbing.h | 91 +++++\n> > > > > src/ipa/libipa/algorithms/meson.build | 2 +\n> > > > > 3 files changed, 472 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..6ee090eb\n> > > > > --- /dev/null\n> > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > > > @@ -0,0 +1,379 @@\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) 2023, Theobroma Systems\n> > > > > + *\n> > > > > + * af_hill_climbing.cpp - AF contrast based hill climbing common algorithm\n> > > > > + */\n> > > > > +\n> > > > > +#include \"af_hill_climbing.h\"\n> > > > > +\n> > > > > +#include \"libcamera/internal/yaml_parser.h\"\n> > > > > +\n> > > > > +/**\n> > > > > + * \\file af_hill_climbing.h\n> > > > > + * \\brief AF contrast based hill climbing common algorithm\n> > > > > + */\n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +namespace ipa::algorithms {\n> > > > > +\n> > > > > +LOG_DEFINE_CATEGORY(Af)\n> > > > > +\n> > > > > +/**\n> > > > > + * \\class AfHillClimbing\n> > > > > + * \\brief Contrast based hill climbing auto focus control algorithm\n> > > > > + * implementation\n> > > > > + *\n> > > > > + * Control part of the auto focus algorithm. It calculates the lens position\n> > > > > + * based on the contrast measure supplied by the platform-specific\n> > > > > + * implementation. This way it is independent from the platform.\n> > > > > + *\n> > > > > + * Platform layer that use this algorithm should call process() function\n> > > > > + * for each each frame and set the lens to the calculated position.\n> > > > > + *\n> > > > > + * Focus search is divided into two phases:\n> > > > > + * 1. coarse search,\n> > > > > + * 2. fine search.\n> > > > > + *\n> > > > > + * In the first phase, the lens is moved with bigger steps to quickly find\n> > > > > + * a rough position for the best focus. Then, based on the outcome of coarse\n> > > > > + * search, the second phase is executed. Lens is moved with smaller steps\n> > > > > + * in a limited range within the rough position to find the exact position\n> > > > > + * for best focus.\n> > > > > + *\n> > > > > + * Tuning file parameters:\n> > > > > + * - **coarse-search-step:** The value by which the lens position will change\n> > > > > + * in one step in the *coarse search* phase. Unit is lens specific.\n> > > > > + * - **fine-search-step:** The value by which the lens position will change\n> > > > > + * in one step in the *fine search* phase. Unit is lens specific.\n> > > > > + * - **fine-search-range:** Search range in the *fine search* phase, expressed\n> > > > > + * as a percentage of the coarse search result. Valid values are\n> > > > > + * in the [0, 100] interval. Value 5 means 5%. If coarse search stopped\n> > > > > + * at value 200, the fine search range will be [190, 210].\n> > > > > + * - **max-variance-change:** ratio of contrast variance change in the\n> > > > > + * *continuous mode* needed for triggering the focus change. When the variance\n> > > > > + * change exceeds this value, focus search will be triggered. Valid values are\n> > > > > + * in the [0.0, 1.0] interval.\n> > > > > + * .\n> > > > > + *\n> > > > > + * \\todo Search range in the *fine search* phase should depend on the lens\n> > > > > + * movement range rather than coarse search result.\n> > > > > + * \\todo Implement setRange.\n> > > > > + * \\todo Implement setSpeed.\n> > > > > + * \\todo Implement setMeteringMode.\n> > > > > + * \\todo Implement setWindows.\n> > > > > + * \\todo Implement the AfPauseDeferred mode.\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Initialize the AfHillClimbing with tuning data\n> > > > > + * \\param[in] tuningData The tuning data for the algorithm\n> > > > > + *\n> > > > > + * This method should be called in the libcamera::ipa::Algorithm::init()\n> > > > > + * method of the platform layer.\n> > > > > + *\n> > > > > + * \\return 0 if successful, an error code otherwise\n> > > > > + */\n> > > > > +int AfHillClimbing::init(const YamlObject &tuningData)\n> > > > > +{\n> > > > > + coarseSearchStep_ = tuningData[\"coarse-search-step\"].get(30);\n> > > > > + fineSearchStep_ = tuningData[\"fine-search-step\"].get(1);\n> > > > > + fineRange_ = tuningData[\"fine-search-range\"].get(5);\n> > > > > + fineRange_ /= 100;\n> > > > > + maxChange_ = tuningData[\"max-variance-change\"].get(0.5);\n> > > > > +\n> > > > > + LOG(Af, Debug) << \"coarseSearchStep_: \" << coarseSearchStep_\n> > > > > + << \", fineSearchStep_: \" << fineSearchStep_\n> > > > > + << \", fineRange_: \" << fineRange_\n> > > > > + << \", maxChange_: \" << maxChange_;\n> > > > > +\n> > > > > + return 0;\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Configure the AfHillClimbing with sensor and lens information\n> > > > > + * \\param[in] minFocusPosition Minimum position supported by camera lens\n> > > > > + * \\param[in] maxFocusPosition Maximum position supported by camera lens\n> > > > > + *\n> > > > > + * This method should be called in the libcamera::ipa::Algorithm::configure()\n> > > > > + * method of the platform layer.\n> > > > > + */\n> > > > > +void AfHillClimbing::configure(int32_t minFocusPosition,\n> > > > > + int32_t maxFocusPosition)\n> > > > > +{\n> > > > > + minLensPosition_ = minFocusPosition;\n> > > > > + maxLensPosition_ = maxFocusPosition;\n> > > > > +}\n> > > > > +\n> > > > > +/**\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 in the libcamera::ipa::Algorithm::process()\n> > > > > + * method of the platform layer for each frame.\n> > > > > + *\n> > > > > + * Contrast value supplied in the \\p currentContrast parameter can be platform\n> > > > > + * specific. The only requirement is the contrast value must increase with\n> > > > > + * the increasing image focus. Contrast value must be highest when image is in\n> > > > > + * focus.\n> > > > > + *\n> > > > > + * \\return New lens position calculated by the AF algorithm\n> > > > > + */\n> > > > > +int32_t AfHillClimbing::process(double currentContrast)\n> > > > > +{\n> > > > > + currentContrast_ = currentContrast;\n> > > > > +\n> > > > > + if (shouldSkipFrame())\n> > > > > + return lensPosition_;\n> > > > > +\n> > > > > + switch (mode_) {\n> > > > > + case controls::AfModeManual:\n> > > > > + /* Nothing to process. */\n> > > > > + break;\n> > > > > + case controls::AfModeAuto:\n> > > > > + processAutoMode();\n> > > > > + break;\n> > > > > + case controls::AfModeContinuous:\n> > > > > + processContinuousMode();\n> > > > > + break;\n> > > > > + }\n> > > > > +\n> > > > > + return lensPosition_;\n> > > > > +}\n> > > > > +\n> > > > > +void AfHillClimbing::processAutoMode()\n> > > > > +{\n> > > > > + if (state_ == controls::AfStateScanning) {\n> > > > > + afCoarseScan();\n> > > > > + afFineScan();\n> > > > > + }\n> > > > > +}\n> > > > > +\n> > > > > +void AfHillClimbing::processContinuousMode()\n> > > > > +{\n> > > > > + /* If we are in a paused state, we won't process the stats. */\n> > > > > + if (pauseState_ == controls::AfPauseStatePaused)\n> > > > > + return;\n> > > > > +\n> > > > > + if (state_ == controls::AfStateScanning) {\n> > > > > + afCoarseScan();\n> > > > > + afFineScan();\n> > > > > + return;\n> > > > > + }\n> > > > > +\n> > > > > + /*\n> > > > > + * AF scan can be started at any moment in AfModeContinuous,\n> > > > > + * except when the state is already AfStateScanning.\n> > > > > + */\n> > > > > + if (afIsOutOfFocus())\n> > > > > + afReset();\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Request AF to skip n frames\n> > > > > + * \\param[in] n Number of frames to be skipped\n> > > > > + *\n> > > > > + * For the requested number of frames, the AF calculation will be skipped\n> > > > > + * and lens position will not change. The platform layer still needs to\n> > > > > + * call process() function for each frame during this time.\n> > > > > + * This function can be used by the platform layer if the hardware needs more\n> > > > > + * time for some operations.\n> > > > > + *\n> > > > > + * The number of the requested frames (\\p n) will be applied only if \\p n has\n> > > > > + * higher value than the number of frames already requested to be skipped.\n> > > > > + * For example, if *setFramesToSkip(5)* was already called for the current\n> > > > > + * frame, then calling *setFramesToSkip(3)* will not override the previous\n> > > > > + * request and 5 frames will be skipped.\n> > > >\n> > > > This still puzzles me a bit as it is not clear to me when the platform\n> > > > layer is supposed to set the number of frames to skip.\n> > > >\n> > > > Looking at the implementation it happens everytime the lens is moved.\n> > > > I guess this is fine for now, but this will need to be handled better\n> > > > by the CameraLens class modeling the lens movement delay.\n> > > >\n> > >\n> > > In RkISP it is also needed when setting AF Windows. ISP needs a one\n> > > additional frame to calculate the new statistics after setting window.\n> > > This is why I exposed this method as public.\n> > >\n> > > > > + */\n> > > > > +void AfHillClimbing::setFramesToSkip(uint32_t n)\n> > > > > +{\n> > > > > + if (n > framesToSkip_)\n> > > > > + framesToSkip_ = n;\n> > > > > +}\n> > > > > +\n> > > > > +void AfHillClimbing::setMode(controls::AfModeEnum mode)\n> > > > > +{\n> > > > > + if (mode == mode_)\n> > > > > + return;\n> > > > > +\n> > > > > + LOG(Af, Debug) << \"Switched AF mode from \" << mode_ << \" to \" << mode;\n> > > > > + mode_ = mode;\n> > > > > +\n> > > > > + state_ = controls::AfStateIdle;\n> > > > > + pauseState_ = controls::AfPauseStateRunning;\n> > > > > +\n> > > > > + if (mode_ == controls::AfModeContinuous)\n> > > > > + afReset();\n> > > > > +}\n> > > > > +\n> > > > > +void AfHillClimbing::setRange([[maybe_unused]] controls::AfRangeEnum range)\n> > > > > +{\n> > > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > > +}\n> > > > > +\n> > > > > +void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed)\n> > > > > +{\n> > > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > > +}\n> > > > > +\n> > > > > +void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering)\n> > > > > +{\n> > > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > > +}\n> > > > > +\n> > > > > +void AfHillClimbing::setWindows([[maybe_unused]] Span windows)\n> > > > > +{\n> > > > > + LOG(Af, Error) << __FUNCTION__ << \" not implemented!\";\n> > > > > +}\n> > > > > +\n> > > >\n> > > > I'm still of the idea you should better not implement these functions\n> > > > at all. If anything tries to call into them will fail at compile time\n> > > > instead of getting a run-time error in the logs\n> > >\n> > > These functions are pure virtual, so you are forced by compiler\n> > > to implement them.\n> > >\n> >\n> > Ah! I removed the definition in the cpp file but I left the\n> > declaration in the header, so I guess it worked here because of the\n> > compiler generated one for me. Sorry for the overlook\n> >\n> > > >\n> > > > > +void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)\n> > > > > +{\n> > > > > + if (mode_ != controls::AfModeAuto) {\n> > > > > + LOG(Af, Warning)\n> > > > > + << __FUNCTION__ << \" not valid in mode \" << mode_;\n> > > >\n> > > > We don't use __FUNCTION__ for the reason that as far as I know it's\n> > > > not a standard language keyword (while __func__ is iirc). I'm also not\n> > > > sure how it works with name mangling as I see gcc has a\n> > > > __PRETTY_FUNCTION__ specific extensions to beautify names.\n> > > >\n> > > > I would just\n> > > > << \"setTrigger() not valid in mode \" << mode_;\n> > > >\n> > > > > + return;\n> > > > > + }\n> > > > > +\n> > > > > + LOG(Af, Debug) << \"Trigger called with \" << trigger;\n> > > > > +\n> > > > > + if (trigger == controls::AfTriggerStart)\n> > > > > + afReset();\n> > > > > + else\n> > > > > + state_ = controls::AfStateIdle;\n> > > >\n> > > > I would switch to be sure to handle all cases. A minor though\n> > > >\n> > > > > +}\n> > > > > +\n> > > > > +void AfHillClimbing::setPause(controls::AfPauseEnum pause)\n> > > > > +{\n> > > > > + if (mode_ != controls::AfModeContinuous) {\n> > > > > + LOG(Af, Warning)\n> > > > > + << __FUNCTION__ << \" not valid in mode \" << mode_;\n> > > >\n> > > > ditto\n> > > >\n> > > > > + return;\n> > > > > + }\n> > > > > +\n> > > > > + switch (pause) {\n> > > > > + case controls::AfPauseImmediate:\n> > > > > + pauseState_ = controls::AfPauseStatePaused;\n> > > > > + break;\n> > > > > + case controls::AfPauseDeferred:\n> > > > > + LOG(Af, Warning) << \"AfPauseDeferred is not supported!\";\n> > > > > + break;\n> > > > > + case controls::AfPauseResume:\n> > > > > + pauseState_ = controls::AfPauseStateRunning;\n> > > > > + break;\n> > > > > + }\n> > > > > +}\n> > > > > +\n> > > > > +void AfHillClimbing::setLensPosition(float lensPosition)\n> > > > > +{\n> > > > > + if (mode_ != controls::AfModeManual) {\n> > > > > + LOG(Af, Warning)\n> > > > > + << __FUNCTION__ << \" not valid in mode \" << mode_;\n> > > > > + return;\n> > > > > + }\n> > > > > +\n> > > > > + lensPosition_ = static_cast(lensPosition);\n> > > > > +\n> > > > > + LOG(Af, Debug) << \"Requesting lens position \" << lensPosition_;\n> > > > > +}\n> > > > > +\n> > > > > +void AfHillClimbing::afCoarseScan()\n> > > > > +{\n> > > > > + if (coarseCompleted_)\n> > > > > + return;\n> > > > > +\n> > > > > + if (afScan(coarseSearchStep_)) {\n> > > > > + coarseCompleted_ = true;\n> > > > > + maxContrast_ = 0;\n> > > > > + const auto diff = static_cast(\n> > > > > + std::abs(lensPosition_) * fineRange_);\n> > > > > + lensPosition_ = std::max(lensPosition_ - diff, minLensPosition_);\n> > > > > + maxStep_ = std::min(lensPosition_ + diff, maxLensPosition_);\n> > > > > + }\n> > > > > +}\n> > > > > +\n> > > > > +void AfHillClimbing::afFineScan()\n> > > > > +{\n> > > > > + if (!coarseCompleted_)\n> > > > > + return;\n> > > > > +\n> > > > > + if (afScan(fineSearchStep_)) {\n> > > > > + LOG(Af, Debug) << \"AF found the best focus position!\";\n> > > > > + state_ = controls::AfStateFocused;\n> > > > > + }\n> > > > > +}\n> > > > > +\n> > > > > +bool AfHillClimbing::afScan(uint32_t steps)\n> > > > > +{\n> > > > > + if (lensPosition_ + static_cast(steps) > maxStep_) {\n> > > > > + /* If the max step is reached, move lens to the position. */\n> > > > > + lensPosition_ = bestPosition_;\n> > > > > + return true;\n> > > > > + } else {\n> > > >\n> > > > If you return in the if() clause , you can remove else { } and save\n> > > > one indendation level\n> > >\n> > > Good points! I will rework the code :)\n> > >\n> > > >\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_ += static_cast(steps);\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> > > > > + LOG(Af, Debug) << \"Previous step is \" << bestPosition_\n> > > > > + << \", Current step is \" << lensPosition_;\n> > > > > + return false;\n> > > > > +}\n> > > > > +\n> > > > > +void AfHillClimbing::afReset()\n> > > > > +{\n> > > > > + LOG(Af, Debug) << \"Reset AF parameters\";\n> > > > > + lensPosition_ = minLensPosition_;\n> > > > > + maxStep_ = maxLensPosition_;\n> > > > > + state_ = controls::AfStateScanning;\n> > > > > + coarseCompleted_ = false;\n> > > > > + maxContrast_ = 0.0;\n> > > > > + setFramesToSkip(1);\n> > > > > +}\n> > > > > +\n> > > > > +bool AfHillClimbing::afIsOutOfFocus() const\n> > > > > +{\n> > > > > + const double diff_var = std::abs(currentContrast_ - maxContrast_);\n> > > > > + const double var_ratio = diff_var / maxContrast_;\n> > > > > + LOG(Af, Debug) << \"Variance change rate: \" << var_ratio\n> > > > > + << \", Current lens step: \" << lensPosition_;\n> > > > > + return var_ratio > maxChange_;\n> > > > > +}\n> > > > > +\n> > > > > +bool AfHillClimbing::shouldSkipFrame()\n> > > > > +{\n> > > > > + if (framesToSkip_ > 0) {\n> > > > > + framesToSkip_--;\n> > > > > + return true;\n> > > > > + }\n> > > > > +\n> > > > > + return false;\n> > > > > +}\n> > > > > +\n> > > > > +} /* namespace ipa::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..47d2bbec\n> > > > > --- /dev/null\n> > > > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > > > @@ -0,0 +1,91 @@\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) 2023, Theobroma Systems\n> > > > > + *\n> > > > > + * af_hill_climbing.h - AF contrast based hill climbing common algorithm\n> > > > > + */\n> > > > > +\n> > > > > +#pragma once\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \"af.h\"\n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +class YamlObject;\n> > > > > +\n> > > > > +namespace ipa::algorithms {\n> > > > > +\n> > > > > +LOG_DECLARE_CATEGORY(Af)\n> > > > > +\n> > > > > +class AfHillClimbing : public Af\n> > > >\n> > > > Isn't it better to declare the class final instead of the single\n> > > > functions ?\n> > >\n> > > It is not the same.\n> > > When used in a class definition, final specifies\n> > > that the class cannot be derived from.\n> >\n> > Yep\n> >\n> > > When used in a virtual function declaration or definition, final\n> > > specifier ensures that the function is virtual and specifies that it\n> > > may not be overridden by derived classes.\n> >\n> > I almost agree. final when applied to a virtual function ensures that\n> > it cannot be overriden in derived classes.\n> >\n> > The part I don't get in you comment is \"final specifier ensures that\n> > the function is -virtual-\" the rest I agree on.\n>\n> When \"final\" is used in a function definition, it implies function\n> to be virtual. This is why I said it is a different for class and\n> function. But now I see, I probably misunderstood your intention\n> and your comment was more about architecture - what should be final.\n> Not that class final should replace function finals as I understood it.\n>\n> >\n> > > It is technically possible to derive from AfHillClimbing to build\n> > > on it, even if it was meant to be used in composition style.\n> > > Do We want to force a composition approach or leave possibility to\n> > > derive from it?\n> >\n> > The thing that doesn't click for me is that you have declared as final\n> > the methods that implement the AF state machine, which is agnostic\n> > from the HillClimbing algorithm. Is this correct ?\n> >\n> > void setMode(controls::AfModeEnum mode) final;\n> > void setRange(controls::AfRangeEnum range) final;\n> > void setSpeed(controls::AfSpeedEnum speed) final;\n> > void setMeteringMode(controls::AfMeteringEnum metering) final;\n> > void setWindows(Span windows) final;\n> > void setTrigger(controls::AfTriggerEnum trigger) final;\n> > void setPause(controls::AfPauseEnum pause) final;\n> > void setLensPosition(float lensPosition) final;\n> >\n> > void processAutoMode();\n> > void processContinuousMode();\n> > void afCoarseScan();\n> > void afFineScan();\n> > bool afScan(uint32_t steps);\n> > void afReset();\n> > [[nodiscard]] bool afIsOutOfFocus() const;\n> > bool shouldSkipFrame();\n> >\n> > As I see it, the algorithm state machine method might later be broken\n> > out to a common AfBase class (or even making the Af class non-purely\n> > virtual) as they do not depend on the contrast-computation method.\n> >\n> > If one would like to derive from AfHillClimbing I guess it would be to\n> > tweak the algorithm's computation, not the state machine which adheres\n> > to the libcamera's controls definition. That's why I was suggesting\n> > that this fine-grained control on what can be overridden is probably a\n> > bit premature.\n>\n> This is a valid point. Algorithms class hierarchy (and especially this\n> one) is in early stage. Then maybe I should drop the final specifiers\n> completely in this class? As it is hard to decide the future direction\n> at this point?\n>\n\nI would say so. I guess it's a tad early to divine how this hierarchy\nwill be expanded in future.\n\nThanks\n j\n\n> >\n> > >\n> > > Best regards\n> > > Daniel\n> > >\n> > > >\n> > > > class AfHillClimbing final : public Af\n> > > >\n> > > > > +{\n> > > > > +public:\n> > > > > + int init(const YamlObject &tuningData);\n> > > > > + void configure(int32_t minFocusPosition, int32_t maxFocusPosition);\n> > > > > + int32_t process(double currentContrast);\n> > > > > + void setFramesToSkip(uint32_t n);\n> > > > > +\n> > > > > + controls::AfStateEnum state() final { return state_; }\n> > > > > + controls::AfPauseStateEnum pauseState() final { return pauseState_; }\n> > > > > +\n> > > > > +private:\n> > > > > + void setMode(controls::AfModeEnum mode) final;\n> > > > > + void setRange(controls::AfRangeEnum range) final;\n> > > > > + void setSpeed(controls::AfSpeedEnum speed) final;\n> > > > > + void setMeteringMode(controls::AfMeteringEnum metering) final;\n> > > > > + void setWindows(Span windows) final;\n> > > > > + void setTrigger(controls::AfTriggerEnum trigger) final;\n> > > > > + void setPause(controls::AfPauseEnum pause) final;\n> > > > > + void setLensPosition(float lensPosition) final;\n> > > >\n> > > > These functions should have the 'override' specifier\n> > > >\n> > > > > +\n> > > > > + void processAutoMode();\n> > > > > + void processContinuousMode();\n> > > > > + void afCoarseScan();\n> > > > > + void afFineScan();\n> > > > > + bool afScan(uint32_t steps);\n> > > > > + void afReset();\n> > > > > + [[nodiscard]] bool afIsOutOfFocus() const;\n> > > > > + bool shouldSkipFrame();\n> > > > > +\n> > > > > + controls::AfModeEnum mode_ = controls::AfModeManual;\n> > > > > + controls::AfStateEnum state_ = controls::AfStateIdle;\n> > > > > + controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning;\n> > > > > +\n> > > > > + /* Current focus lens position. */\n> > > > > + int32_t lensPosition_ = 0;\n> > > > > + /* Local optimum focus lens position during scanning. */\n> > > > > + int32_t bestPosition_ = 0;\n> > > > > +\n> > > > > + /* Current AF statistic contrast. */\n> > > > > + double currentContrast_ = 0;\n> > > > > + /* It is used to determine the derivative during scanning */\n> > > > > + double maxContrast_ = 0;\n> > > > > + /* The designated maximum range of focus scanning. */\n> > > > > + int32_t maxStep_ = 0;\n> > > > > + /* If the coarse scan completes, it is set to true. */\n> > > > > + bool coarseCompleted_ = false;\n> > > > > +\n> > > > > + uint32_t framesToSkip_ = 0;\n> > > > > +\n> > > > > + /* Position limits of the focus lens. */\n> > > > > + int32_t minLensPosition_;\n> > > > > + int32_t maxLensPosition_;\n> > > > > +\n> > > > > + /* Minimum position step for searching appropriate focus. */\n> > > > > + uint32_t coarseSearchStep_;\n> > > > > + uint32_t fineSearchStep_;\n> > > > > +\n> > > > > + /* Fine scan range 0 < fineRange_ < 1. */\n> > > > > + double fineRange_;\n> > > > > +\n> > > > > + /* Max ratio of variance change, 0.0 < maxChange_ < 1.0. */\n> > > > > + double maxChange_;\n> > > > > +};\n> > > > > +\n> > > > > +} /* namespace ipa::algorithms */\n> > > > > +\n> > > > > +} /* namespace libcamera */\n> > > > > diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> > > > > index 3df4798f..20e437fc 100644\n> > > > > --- a/src/ipa/libipa/algorithms/meson.build\n> > > > > +++ b/src/ipa/libipa/algorithms/meson.build\n> > > > > @@ -2,8 +2,10 @@\n> > > > >\n> > > > > libipa_algorithms_headers = files([\n> > > > > 'af.h',\n> > > > > + 'af_hill_climbing.h',\n> > > > > ])\n> > > > >\n> > > > > libipa_algorithms_sources = files([\n> > > > > 'af.cpp',\n> > > > > + 'af_hill_climbing.cpp',\n> > > > > ])\n> > > > > --\n> > > > > 2.39.2\n> > > > >\n>\n> Best regards\n> Daniel","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 9C953BD160\n\tfor ;\n\tTue, 28 Mar 2023 07:07:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AB39B62742;\n\tTue, 28 Mar 2023 09:07:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 773A5626E2\n\tfor ;\n\tTue, 28 Mar 2023 09:07:27 +0200 (CEST)","from ideasonboard.com (host-82-56-47-78.retail.telecomitalia.it\n\t[82.56.47.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 19B4CBAE;\n\tTue, 28 Mar 2023 09:07:26 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679987248;\n\tbh=MnkynE5UwKZhl7b7mC4riSO0jvqQg6Gn800rASMZfO4=;\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=VERi+KmGUuutVZnLmh2yIpfB3HWVxRoemHX6+ZwViLp+8yogXWHVQM+czSBZAn2ts\n\tfvVxrX03xWSrInUpntlaJRL9Xio2AUinVkbX3Sizb+TM6ibUqoUcDshyVF2Cf5Wobv\n\tS/soF9RTJc8u/+CWYMiQs1Ku+lrY0IdvO9xdXLcMescVfukFHzHxLZAg6MrC6TEYJa\n\tUGlLKQXa+UPBsgXYZcf8zW+RNfiMfWAcAslEmgrASKTYczL056RM3MeNqYCkQS1zGP\n\toTEl7keuvHJ+6+pYxQwWSeTotYiSGLJ0gpdBfv+bqg6V2FVv220i/FvUOhYurAl1YZ\n\tlypXc+8ReE6UA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1679987247;\n\tbh=MnkynE5UwKZhl7b7mC4riSO0jvqQg6Gn800rASMZfO4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=T6peor+9sDjyJxRW9jJrxLqOKCRJ9HLoFv9c+jwT/p9rjZciymr7nx49MDbfR7CHf\n\tczW+KJH/kvDM+0+KYPyXVl3O+BGOAM4601jij09re4d9un6IHQQq60whaQl2Ew5GrT\n\thGDl3acAkPK3FrBBhzMHAc4guFvMS4ySsXtQh08M="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"T6peor+9\"; dkim-atps=neutral","Date":"Tue, 28 Mar 2023 09:07:22 +0200","To":"Daniel Semkowicz ","Message-ID":"<20230328070722.puushui732znpnbu@uno.localdomain>","References":"<20230324142908.64224-1-dse@thaumatec.com>\n\t<20230324142908.64224-5-dse@thaumatec.com>\n\t<20230324153040.sk43lwg36qmxwz6t@uno.localdomain>\n\t\n\t<20230327130315.mw5cqmkt3jtpjwdq@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 04/10] ipa: Add common contrast\n\tbased AF implementation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]