[{"id":26682,"web_url":"https://patchwork.libcamera.org/comment/26682/","msgid":"<20230320204609.jyqz65nb6hvkgjll@uno.localdomain>","date":"2023-03-20T20:46:09","subject":"Re: [libcamera-devel] [PATCH v4 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 Tue, Mar 14, 2023 at 03:48:28PM +0100, Daniel Semkowicz 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\nNice!\n\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 | 358 ++++++++++++++++++\n> src/ipa/libipa/algorithms/af_hill_climbing.h | 93 +++++\n> src/ipa/libipa/algorithms/meson.build | 2 +\n> 3 files changed, 453 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..2b99afef\n> --- /dev/null\n> +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> @@ -0,0 +1,358 @@\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::ipa::common::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> + * basing on the contrast measure supplied by the higher level. This way it is\n\ns/basing/based ?\n\n\"higher level\" means the platform-specific implementation ?\n\n> + * independent from the platform.\n> + *\n> + * Platform layer that use this algorithm should call process() method\n\ns/method/function\n\n> + * for each measured contrast value and set the lens to the calculated position.\n\nIs this meant to be called for each frame ?\n\n> + *\n> + * Focus search is divided into two phases:\n> + * 1. coarse search,\n> + * 2. fine search.\n> + *\n> + * In the first phase, lens are moved with bigger steps to quickly find a rough\n\n\"the lens is moved\"\n\n> + * position for the best focus. Then, basing on the outcome of coarse search,\n\n\"based\" again, unelss \"basing\" it's correct and I'm getting it wrong\n\n> + * the second phase is executed. Lens are moved with smaller steps in a limited\n\n\"Lens is\"\n\n> + * range within the rough position to find the exact position for best focus.\n> + *\n> + * Tuning file parameters:\n> + * - **coarse-search-step:** The value by which the position will change in one\n> + * step in the *coarse search* phase.\n> + * - **fine-search-step:** The value by which the position will change in one\n> + * step in the *fine search* phase.\n\nI presume these values are expressed in lens specific units ?\n\n> + * - **fine-search-range:** Search range in the *fine search* phase.\n> + * Valid values are in the [0.0, 1.0] interval. Value 0.05 means 5%. If coarse\n> + * search stopped at value 200, the fine search range will be [190, 210]\n\nIt's nice to have this in percentage, I wonder if it is correct to\ncalculate the percentage on the coarse search result and not on the\nlens movement range.\n\nIn your example, a 5% search interval for a coarse search resul of 200\nwill differ from a result of 400, while I presume the search range\nshould be constant for the lens movement range ?\n\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> +/**\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(0.05);\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\nWhat is the intended unit for the contrast ?\n\n> + *\n> + * This method should be called in the libcamera::ipa::Algorithm::process()\n> + * method of the platform layer for each new contrast value that was measured.\n> + *\n> + * \\return New lens position calculated by 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\tprocessContinousMode();\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tbreak;\n\nDo we need a default case ?\n\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::processContinousMode()\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/* We can re-start the scan at any moment in AfModeContinuous */\n> +\tif (afIsOutOfFocus()) {\n> +\t\tafReset();\n> +\t}\n\nNo need for brace.\n\nI now see what you meant when you said CAF needs to move the lens back\nto its initial position..\n\n> +}\n> +\n> +/**\n> + * \\brief Request AF to skip n frames\n> + * \\param[in] n Number of frames to be skipped\n> + *\n> + * Requested number of frames will not be used for AF calculation.\n\nWhen is this function meant to be called by the platform layer ?\n\nAlso \"reset()\" resets this value too, which when working in Auto mode\n(which requires Triggers to work) will make this value overwritten by\nthe trigger. Is this desired ?\n\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\nI wonder if Error is correct here..\n\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 possible in mode \" << mode_;\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> +}\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 possible in mode \" << mode_;\n\ns/not possible/not valid\n\nhere and below\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\t/* \\todo: add the AfPauseDeferred mode */\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> +\tdefault:\n> +\t\tbreak;\n\nAgain I wonder if default is a good idea, as if the controls get\naugmented I would prefer the compiler to complain if not all cases are\nhandled..\n\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 possible in mode \" << mode_;\n> +\t\treturn;\n> +\t}\n> +\n> +\tlensPosition_ = 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\tint32_t diff = std::abs(lensPosition_) * fineRange_;\n> +\t\tlensPosition_ = std::max(lensPosition_ - diff, minLensPosition_);\n> +\t\tmaxStep_ = std::min(lensPosition_ + diff, maxLensPosition_);\n> +\t\tpreviousContrast_ = 0;\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\tfineCompleted_ = true;\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> +\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_ += 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> +\tpreviousContrast_ = currentContrast_;\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> +\tpreviousContrast_ = 0.0;\n> +\tcoarseCompleted_ = false;\n> +\tfineCompleted_ = false;\n> +\tmaxContrast_ = 0.0;\n> +\tsetFramesToSkip(1);\n> +}\n> +\n> +bool AfHillClimbing::afIsOutOfFocus()\n> +{\n> +\tconst uint32_t 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> +\tif (var_ratio > maxChange_)\n> +\t\treturn true;\n> +\telse\n> +\t\treturn false;\n> +}\n\nI presume the five functions above come from the existing\nimplementation and I'm not really reviewing them :)\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 libcamera::ipa::common::algorithms */\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..b361e5a1\n> --- /dev/null\n> +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> @@ -0,0 +1,93 @@\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::common::algorithms {\n> +\n> +LOG_DECLARE_CATEGORY(Af)\n> +\n> +class AfHillClimbing : public Af\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> +\n> +\tvoid processAutoMode();\n> +\tvoid processContinousMode();\n> +\tvoid afCoarseScan();\n> +\tvoid afFineScan();\n> +\tbool afScan(uint32_t steps);\n> +\tvoid afReset();\n> +\tbool afIsOutOfFocus();\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/* VCM step configuration. It is the current setting of the VCM step. */\n> +\tint32_t lensPosition_ = 0;\n> +\t/* The best VCM step. It is a local optimum VCM step during scanning. */\n> +\tint32_t bestPosition_ = 0;\n\nDo you mean \"step\" or \"position\" in the comments ?\n\n> +\n> +\t/* Current AF statistic contrast. */\n> +\tdouble currentContrast_ = 0;\n> +\t/* It is used to determine the derivative during scanning */\n> +\tdouble previousContrast_ = 0;\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> +\t/* If the fine scan completes, it is set to true. */\n> +\tbool fineCompleted_ = false;\n> +\n> +\tuint32_t framesToSkip_ = 0;\n> +\n> +\t/* Focus steps range of the lens */\n> +\tint32_t minLensPosition_;\n> +\tint32_t maxLensPosition_;\n> +\n> +\t/* Minimum focus 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::common::algorithms */\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> index 7602976c..fa4b9564 100644\n> --- a/src/ipa/libipa/algorithms/meson.build\n> +++ b/src/ipa/libipa/algorithms/meson.build\n> @@ -2,8 +2,10 @@\n>\n> common_ipa_algorithms_headers = files([\n> 'af.h',\n> + 'af_hill_climbing.h',\n> ])\n>\n> common_ipa_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 4701BC0F2A\n\tfor ;\n\tMon, 20 Mar 2023 20:46:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 37E05626E3;\n\tMon, 20 Mar 2023 21:46:16 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4C5FF61ED4\n\tfor ;\n\tMon, 20 Mar 2023 21:46:14 +0100 (CET)","from ideasonboard.com (host-87-18-61-243.retail.telecomitalia.it\n\t[87.18.61.243])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 13E0410A;\n\tMon, 20 Mar 2023 21:46:13 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679345176;\n\tbh=xiHY5Kzxs+MxSBZOKgr12/yckRr7+udAnU1ARUhFU+U=;\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=qZFLbkw1yp9QgBofBs73H5hnrW5XXQYLQLu4BJ1Jj6Fq98BTVTpo68fkdwuoqMgV4\n\tJFeKkOAMx7kfHNdWtR8jwKw4F72DXDRd+PC2RXpn8Ohrtg684rzCDgaAz274gNLkqs\n\tCx4P9qU1e/VFOC6w7I4/7+x+eKY5nqqkHE5t2Mo0lBEFMPdSfXIS8cDLaUuXEs/7bi\n\tDoX4IPf3CEA5surzUYUP60dbhA/dbTxki7pDViLxiLDxpfbEOduET0vUL3xWJ2erKe\n\tLHwTlNRx5CXn2p/cmU/o7tb8j7R4x1XzYa+XrgrAF2zcSd20z2EK4csjCy/rdjtkUO\n\taNJVdUsR3Sr+Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1679345173;\n\tbh=xiHY5Kzxs+MxSBZOKgr12/yckRr7+udAnU1ARUhFU+U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=U5B3gMYqB2FtgtnrlsTtCv8S+JSDsgfJnh+pUd1ql3/KVoBTFJvJvD4EY4GiFsQE3\n\thU0B3zjniCbDewqAQnAehr+CmWQQalhMdFic4VlUI/0MmFa/uCf+UloH0qzE3s6G0l\n\t9hJDgzzuTskQ7ohhJLgOYkTMiBxMkuvJPOqve4bA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"U5B3gMYq\"; dkim-atps=neutral","Date":"Mon, 20 Mar 2023 21:46:09 +0100","To":"Daniel Semkowicz ","Message-ID":"<20230320204609.jyqz65nb6hvkgjll@uno.localdomain>","References":"<20230314144834.85193-1-dse@thaumatec.com>\n\t<20230314144834.85193-5-dse@thaumatec.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230314144834.85193-5-dse@thaumatec.com>","Subject":"Re: [libcamera-devel] [PATCH v4 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":26691,"web_url":"https://patchwork.libcamera.org/comment/26691/","msgid":"","date":"2023-03-21T07:38:15","subject":"Re: [libcamera-devel] [PATCH v4 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 20, 2023 at 9:46 PM Jacopo Mondi\n wrote:\n>\n> Hi Daniel\n>\n> On Tue, Mar 14, 2023 at 03:48:28PM +0100, Daniel Semkowicz 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>\n> Nice!\n>\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 | 358 ++++++++++++++++++\n> > src/ipa/libipa/algorithms/af_hill_climbing.h | 93 +++++\n> > src/ipa/libipa/algorithms/meson.build | 2 +\n> > 3 files changed, 453 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..2b99afef\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > @@ -0,0 +1,358 @@\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::ipa::common::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> > + * basing on the contrast measure supplied by the higher level. This way it is\n>\n> s/basing/based ?\n>\n> \"higher level\" means the platform-specific implementation ?\n>\n> > + * independent from the platform.\n> > + *\n> > + * Platform layer that use this algorithm should call process() method\n>\n> s/method/function\n>\n> > + * for each measured contrast value and set the lens to the calculated position.\n>\n> Is this meant to be called for each frame ?\n\nYes it should, because there is a counter of skipped frames. I will extend this\npart accordingly.\n\n>\n> > + *\n> > + * Focus search is divided into two phases:\n> > + * 1. coarse search,\n> > + * 2. fine search.\n> > + *\n> > + * In the first phase, lens are moved with bigger steps to quickly find a rough\n>\n> \"the lens is moved\"\n>\n> > + * position for the best focus. Then, basing on the outcome of coarse search,\n>\n> \"based\" again, unelss \"basing\" it's correct and I'm getting it wrong\n>\n> > + * the second phase is executed. Lens are moved with smaller steps in a limited\n>\n> \"Lens is\"\n\nThanks for the hints. English is not my native language, so feel free\nto point out all language errors. This will help me learn it better :)\n\n>\n> > + * range within the rough position to find the exact position for best focus.\n> > + *\n> > + * Tuning file parameters:\n> > + * - **coarse-search-step:** The value by which the position will change in one\n> > + * step in the *coarse search* phase.\n> > + * - **fine-search-step:** The value by which the position will change in one\n> > + * step in the *fine search* phase.\n>\n> I presume these values are expressed in lens specific units ?\n\nYes, you are correct, I guess this also needs to be more clear :D\n\n>\n> > + * - **fine-search-range:** Search range in the *fine search* phase.\n> > + * Valid values are in the [0.0, 1.0] interval. Value 0.05 means 5%. If coarse\n> > + * search stopped at value 200, the fine search range will be [190, 210]\n>\n> It's nice to have this in percentage, I wonder if it is correct to\n> calculate the percentage on the coarse search result and not on the\n> lens movement range.\n>\n> In your example, a 5% search interval for a coarse search resul of 200\n> will differ from a result of 400, while I presume the search range\n> should be constant for the lens movement range ?\n\nThis is a part that was moved from the original IPU3 implementation.\nUnfortunately, there was no documentation about the decision. If I needed\nto guess, the reason for that would be the fact, that lens focus change is not\nlinear in reference to the position value. On the other hand, we cannot assure\nthat all lens will need smaller steps on the lower part of the range and not\nthe other way around...\n\n>\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> > +/**\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(0.05);\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> What is the intended unit for the contrast ?\n\nArbitrary. The only assumption is that the higher value means the image\nis more sharp, more in focus.\n\n>\n> > + *\n> > + * This method should be called in the libcamera::ipa::Algorithm::process()\n> > + * method of the platform layer for each new contrast value that was measured.\n> > + *\n> > + * \\return New lens position calculated by 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> > + processContinousMode();\n> > + break;\n> > + default:\n> > + break;\n>\n> Do we need a default case ?\n\nIn this case it can be removed.\n\n>\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::processContinousMode()\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> > + /* We can re-start the scan at any moment in AfModeContinuous */\n> > + if (afIsOutOfFocus()) {\n> > + afReset();\n> > + }\n>\n> No need for brace.\n>\n> I now see what you meant when you said CAF needs to move the lens back\n> to its initial position..\n>\n> > +}\n> > +\n> > +/**\n> > + * \\brief Request AF to skip n frames\n> > + * \\param[in] n Number of frames to be skipped\n> > + *\n> > + * Requested number of frames will not be used for AF calculation.\n>\n> When is this function meant to be called by the platform layer ?\n\nDepends on the platform. For RkISP it is used only to force one frame\nwait to configure the AF window.\n\n>\n> Also \"reset()\" resets this value too, which when working in Auto mode\n> (which requires Triggers to work) will make this value overwritten by\n> the trigger. Is this desired ?\n\nThis value will be overwritten only if the higher number of frames that\nis already set was requested. So it should be safe to use.\n\n>\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> I wonder if Error is correct here..\n\nI would expect that supported controls are correctly configured in the\nIPA, so it should not be possible to call the unsupported controls.\nIn such scenario, entering this function means that something\nis misconfigured and this is an error.\n\nWhat I would really like to see in the libcamera, is that each\nalgorithm expose its own controls during initialisation instead\nof having a hardcoded table of supported controls.\n\n>\n> > +\n> > +void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)\n> > +{\n> > + if (mode_ != controls::AfModeAuto) {\n> > + LOG(Af, Warning)\n> > + << __FUNCTION__ << \" not possible in mode \" << mode_;\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> > +\n> > +void AfHillClimbing::setPause(controls::AfPauseEnum pause)\n> > +{\n> > + if (mode_ != controls::AfModeContinuous) {\n> > + LOG(Af, Warning)\n> > + << __FUNCTION__ << \" not possible in mode \" << mode_;\n>\n> s/not possible/not valid\n>\n> here and below\n>\n> > + return;\n> > + }\n> > +\n> > + switch (pause) {\n> > + case controls::AfPauseImmediate:\n> > + pauseState_ = controls::AfPauseStatePaused;\n> > + break;\n> > + case controls::AfPauseDeferred:\n> > + /* \\todo: add the AfPauseDeferred mode */\n> > + LOG(Af, Warning) << \"AfPauseDeferred is not supported!\";\n> > + break;\n> > + case controls::AfPauseResume:\n> > + pauseState_ = controls::AfPauseStateRunning;\n> > + break;\n> > + default:\n> > + break;\n>\n> Again I wonder if default is a good idea, as if the controls get\n> augmented I would prefer the compiler to complain if not all cases are\n> handled..\n\nI totally agree. I will remove the \"default\" branch.\n\n>\n> > + }\n> > +}\n> > +\n> > +void AfHillClimbing::setLensPosition(float lensPosition)\n> > +{\n> > + if (mode_ != controls::AfModeManual) {\n> > + LOG(Af, Warning)\n> > + << __FUNCTION__ << \" not possible in mode \" << mode_;\n> > + return;\n> > + }\n> > +\n> > + lensPosition_ = 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> > + int32_t diff = std::abs(lensPosition_) * fineRange_;\n> > + lensPosition_ = std::max(lensPosition_ - diff, minLensPosition_);\n> > + maxStep_ = std::min(lensPosition_ + diff, maxLensPosition_);\n> > + previousContrast_ = 0;\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> > + fineCompleted_ = true;\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> > + * 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_ += 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> > + previousContrast_ = currentContrast_;\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> > + previousContrast_ = 0.0;\n> > + coarseCompleted_ = false;\n> > + fineCompleted_ = false;\n> > + maxContrast_ = 0.0;\n> > + setFramesToSkip(1);\n> > +}\n> > +\n> > +bool AfHillClimbing::afIsOutOfFocus()\n> > +{\n> > + const uint32_t 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> > + if (var_ratio > maxChange_)\n> > + return true;\n> > + else\n> > + return false;\n> > +}\n>\n> I presume the five functions above come from the existing\n> implementation and I'm not really reviewing them :)\n\nThis is true. I made some minor changes to them, mostly a code\nformatting.\n\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 libcamera::ipa::common::algorithms */\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..b361e5a1\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > @@ -0,0 +1,93 @@\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::common::algorithms {\n> > +\n> > +LOG_DECLARE_CATEGORY(Af)\n> > +\n> > +class AfHillClimbing : public Af\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> > + void processAutoMode();\n> > + void processContinousMode();\n> > + void afCoarseScan();\n> > + void afFineScan();\n> > + bool afScan(uint32_t steps);\n> > + void afReset();\n> > + bool afIsOutOfFocus();\n> > + bool shouldSkipFrame();\n> > +\n> > + controls::AfModeEnum mode_ = controls::AfModeManual;\n> > + controls::AfStateEnum state_ = controls::AfStateIdle;\n> > + controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning;\n> > +\n> > + /* VCM step configuration. It is the current setting of the VCM step. */\n> > + int32_t lensPosition_ = 0;\n> > + /* The best VCM step. It is a local optimum VCM step during scanning. */\n> > + int32_t bestPosition_ = 0;\n>\n> Do you mean \"step\" or \"position\" in the comments ?\n\nYes, the \"position\" is more clear.\n\n>\n> > +\n> > + /* Current AF statistic contrast. */\n> > + double currentContrast_ = 0;\n> > + /* It is used to determine the derivative during scanning */\n> > + double previousContrast_ = 0;\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> > + /* If the fine scan completes, it is set to true. */\n> > + bool fineCompleted_ = false;\n> > +\n> > + uint32_t framesToSkip_ = 0;\n> > +\n> > + /* Focus steps range of the lens */\n> > + int32_t minLensPosition_;\n> > + int32_t maxLensPosition_;\n> > +\n> > + /* Minimum focus 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::common::algorithms */\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> > index 7602976c..fa4b9564 100644\n> > --- a/src/ipa/libipa/algorithms/meson.build\n> > +++ b/src/ipa/libipa/algorithms/meson.build\n> > @@ -2,8 +2,10 @@\n> >\n> > common_ipa_algorithms_headers = files([\n> > 'af.h',\n> > + 'af_hill_climbing.h',\n> > ])\n> >\n> > common_ipa_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 A547AC0F1B\n\tfor ;\n\tTue, 21 Mar 2023 07:38:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EC4F461ECE;\n\tTue, 21 Mar 2023 08:38:28 +0100 (CET)","from mail-ed1-x52e.google.com (mail-ed1-x52e.google.com\n\t[IPv6:2a00:1450:4864:20::52e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E1C7361ECE\n\tfor ;\n\tTue, 21 Mar 2023 08:38:26 +0100 (CET)","by mail-ed1-x52e.google.com with SMTP id cn12so10433348edb.4\n\tfor ;\n\tTue, 21 Mar 2023 00:38:26 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679384309;\n\tbh=iTWMD6wwR2h/jR9qh5XuOGQVD/bRlqm+FvkSoGxhnvA=;\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=Pa4DVhGObrWm9NLnPRuXtd6u+v1Dsy9mKtDF59NGYp1ecNC5NKQYiB0f3VqC76but\n\tcCgw2lo7Ql2/XCOGyG1a7hK6eC4c8xN7QOkzSX7azbK1AiGBf86veCeWJ25JAvvdDa\n\tVgoIv+tTrGqSkGtgdKJZjfj04A5vMih8MlKTXimvfWrjieiaFo4KFfjk7aIvIvoIi7\n\t30NpK8FJcyfxtTepwFkVOABY+EOMMytwkNNIjgwdy4ZDopJ77DQvi6icMIhB9l6z42\n\tIkqLEdpql1AZAz+L377h4/zrr51TUy5JWwmhsiyQoHvKz/eq1AR623gz3pUQ8lBZnM\n\tjMkTZV1Fy6QRA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20210112.gappssmtp.com; s=20210112; t=1679384306;\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=ZDAdWFvvG253XjJ4D/d4lrHpOX3xGcBu6Aw9+7rmHps=;\n\tb=T7+LFGc4ZEzkfj8qpY5TYxbSgbQqTbzA+Y14h4U0VdZbGi5zs1Ip1klnAMh+oZSwiC\n\tM4EirN1NFKDB69iXpbSIrog/K5x5pFIZDv7InDjTBp7hWnmhoEZ/hOxzxqDiVedquS/J\n\tq1i8PZxUTloSwKG4FjcbeCvaTKZLhe8p2JgCl/gH8BTEjnlPJxxIPCWsa3q1uYU6IYuH\n\tm27NXaPID7futRYifrJXyLD+bCDFbRkcmuzHo55/XT5qu49UusF+hTQ/Kff5zTgXI4Ys\n\tssiA/Tk6VEXijkHYHB4YstRNNpV+993MRNGp3JhEkpnukfpIyum0DV2Mi1K7laR2VIfv\n\tTIqg=="],"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=\"T7+LFGc4\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1679384306;\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=ZDAdWFvvG253XjJ4D/d4lrHpOX3xGcBu6Aw9+7rmHps=;\n\tb=hxpGHM1qlzWO7+cDPluHSegQlaH8p1K09w30QUEbTfBhoySU7KAUetI6czDwG3hDZv\n\tsTvjWKiCP7SiAf8LvMMYcTVVo0VAhnZ2I5lAwuZj7/vhkdqtRCBIBKqxiJZG8mbYuTcS\n\tfuy54Mbh63ItmSoffVN2eGSzbmnepT2Ik6ATAKKUXcs++pPNTfQap4C2RtfwFwPgckEr\n\tRWdqzsWUxzbK1fNWgkWo5UvcSNhvfIDx0y/HUVi9bOaGzIm3S50waywbm70FDKQECbwj\n\tO+9F69dTWm50EIU6ryPh9A5Vo0Or311infFc/H9j1Gk8lyDh6PFCkwmMbGKos2zygcS1\n\t58+A==","X-Gm-Message-State":"AO0yUKVy4fi9YdrCIfq9qk0kwlA1dci+zHWWIbivA+98Rvxam1DEWNpW\n\t2ECBiGhfQVDy1vY0TRbHCV/AiYNQyOXOFZLgHDKj6tIp8KRkfm8eCCE=","X-Google-Smtp-Source":"AK7set+o672V5NtuSIhlMpk6htov5Jfb8msm8ovsk10TB7zFBMUkR/SzwIF1/h/I6jqyjENrMeUv3QtAD623Wocv/y0=","X-Received":"by 2002:a50:c3cf:0:b0:4fb:2593:846 with SMTP id\n\ti15-20020a50c3cf000000b004fb25930846mr1064183edf.3.1679384306153;\n\tTue, 21 Mar 2023 00:38:26 -0700 (PDT)","MIME-Version":"1.0","References":"<20230314144834.85193-1-dse@thaumatec.com>\n\t<20230314144834.85193-5-dse@thaumatec.com>\n\t<20230320204609.jyqz65nb6hvkgjll@uno.localdomain>","In-Reply-To":"<20230320204609.jyqz65nb6hvkgjll@uno.localdomain>","Date":"Tue, 21 Mar 2023 08:38:15 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v4 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":26695,"web_url":"https://patchwork.libcamera.org/comment/26695/","msgid":"<20230321081542.sabwefml6ta5a4aa@uno.localdomain>","date":"2023-03-21T08:15:42","subject":"Re: [libcamera-devel] [PATCH v4 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 Tue, Mar 21, 2023 at 08:38:15AM +0100, Daniel Semkowicz wrote:\n> Hi Jacopo,\n>\n> On Mon, Mar 20, 2023 at 9:46 PM Jacopo Mondi\n> wrote:\n> >\n> > Hi Daniel\n> >\n> > On Tue, Mar 14, 2023 at 03:48:28PM +0100, Daniel Semkowicz 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> >\n> > Nice!\n> >\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 | 358 ++++++++++++++++++\n> > > src/ipa/libipa/algorithms/af_hill_climbing.h | 93 +++++\n> > > src/ipa/libipa/algorithms/meson.build | 2 +\n> > > 3 files changed, 453 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..2b99afef\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > @@ -0,0 +1,358 @@\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::ipa::common::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> > > + * basing on the contrast measure supplied by the higher level. This way it is\n> >\n> > s/basing/based ?\n> >\n> > \"higher level\" means the platform-specific implementation ?\n> >\n> > > + * independent from the platform.\n> > > + *\n> > > + * Platform layer that use this algorithm should call process() method\n> >\n> > s/method/function\n> >\n> > > + * for each measured contrast value and set the lens to the calculated position.\n> >\n> > Is this meant to be called for each frame ?\n>\n> Yes it should, because there is a counter of skipped frames. I will extend this\n> part accordingly.\n>\n> >\n> > > + *\n> > > + * Focus search is divided into two phases:\n> > > + * 1. coarse search,\n> > > + * 2. fine search.\n> > > + *\n> > > + * In the first phase, lens are moved with bigger steps to quickly find a rough\n> >\n> > \"the lens is moved\"\n> >\n> > > + * position for the best focus. Then, basing on the outcome of coarse search,\n> >\n> > \"based\" again, unelss \"basing\" it's correct and I'm getting it wrong\n> >\n> > > + * the second phase is executed. Lens are moved with smaller steps in a limited\n> >\n> > \"Lens is\"\n>\n> Thanks for the hints. English is not my native language, so feel free\n> to point out all language errors. This will help me learn it better :)\n>\n\nNeither is mine, so take all suggestions with a grain of salt\n\n> >\n> > > + * range within the rough position to find the exact position for best focus.\n> > > + *\n> > > + * Tuning file parameters:\n> > > + * - **coarse-search-step:** The value by which the position will change in one\n> > > + * step in the *coarse search* phase.\n\nCan you s/\"the position will\"/\"the lens position will\"\n\nhere and below ?\n\n> > > + * - **fine-search-step:** The value by which the position will change in one\n> > > + * step in the *fine search* phase.\n> >\n> > I presume these values are expressed in lens specific units ?\n>\n> Yes, you are correct, I guess this also needs to be more clear :D\n>\n> >\n> > > + * - **fine-search-range:** Search range in the *fine search* phase.\n> > > + * Valid values are in the [0.0, 1.0] interval. Value 0.05 means 5%. If coarse\n> > > + * search stopped at value 200, the fine search range will be [190, 210]\n> >\n> > It's nice to have this in percentage, I wonder if it is correct to\n> > calculate the percentage on the coarse search result and not on the\n> > lens movement range.\n> >\n> > In your example, a 5% search interval for a coarse search resul of 200\n> > will differ from a result of 400, while I presume the search range\n> > should be constant for the lens movement range ?\n>\n> This is a part that was moved from the original IPU3 implementation.\n> Unfortunately, there was no documentation about the decision. If I needed\n> to guess, the reason for that would be the fact, that lens focus change is not\n> linear in reference to the position value. On the other hand, we cannot assure\n> that all lens will need smaller steps on the lower part of the range and not\n> the other way around...\n>\n\nExactly, it is rather hard to make any kind of assumptions...\nCould you add a \\todo in the documentation that reports that the\nsearch range percentage should be made dependendent on the lens\nmovement range and not on the coarse value ?\n\n> >\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> > > +/**\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(0.05);\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> > What is the intended unit for the contrast ?\n>\n> Arbitrary. The only assumption is that the higher value means the image\n> is more sharp, more in focus.\n>\n\nShould we document such assumption ?\n\n> >\n> > > + *\n> > > + * This method should be called in the libcamera::ipa::Algorithm::process()\n> > > + * method of the platform layer for each new contrast value that was measured.\n> > > + *\n> > > + * \\return New lens position calculated by 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> > > + processContinousMode();\n> > > + break;\n> > > + default:\n> > > + break;\n> >\n> > Do we need a default case ?\n>\n> In this case it can be removed.\n>\n> >\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::processContinousMode()\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> > > + /* We can re-start the scan at any moment in AfModeContinuous */\n> > > + if (afIsOutOfFocus()) {\n> > > + afReset();\n> > > + }\n> >\n> > No need for brace.\n> >\n> > I now see what you meant when you said CAF needs to move the lens back\n> > to its initial position..\n> >\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Request AF to skip n frames\n> > > + * \\param[in] n Number of frames to be skipped\n> > > + *\n> > > + * Requested number of frames will not be used for AF calculation.\n> >\n> > When is this function meant to be called by the platform layer ?\n>\n> Depends on the platform. For RkISP it is used only to force one frame\n> wait to configure the AF window.\n>\n> >\n> > Also \"reset()\" resets this value too, which when working in Auto mode\n> > (which requires Triggers to work) will make this value overwritten by\n> > the trigger. Is this desired ?\n>\n> This value will be overwritten only if the higher number of frames that\n> is already set was requested. So it should be safe to use.\n>\n> >\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> > I wonder if Error is correct here..\n>\n> I would expect that supported controls are correctly configured in the\n> IPA, so it should not be possible to call the unsupported controls.\n> In such scenario, entering this function means that something\n> is misconfigured and this is an error.\n>\n> What I would really like to see in the libcamera, is that each\n> algorithm expose its own controls during initialisation instead\n> of having a hardcoded table of supported controls.\n>\n> >\n> > > +\n> > > +void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)\n> > > +{\n> > > + if (mode_ != controls::AfModeAuto) {\n> > > + LOG(Af, Warning)\n> > > + << __FUNCTION__ << \" not possible in mode \" << mode_;\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> > > +\n> > > +void AfHillClimbing::setPause(controls::AfPauseEnum pause)\n> > > +{\n> > > + if (mode_ != controls::AfModeContinuous) {\n> > > + LOG(Af, Warning)\n> > > + << __FUNCTION__ << \" not possible in mode \" << mode_;\n> >\n> > s/not possible/not valid\n> >\n> > here and below\n> >\n> > > + return;\n> > > + }\n> > > +\n> > > + switch (pause) {\n> > > + case controls::AfPauseImmediate:\n> > > + pauseState_ = controls::AfPauseStatePaused;\n> > > + break;\n> > > + case controls::AfPauseDeferred:\n> > > + /* \\todo: add the AfPauseDeferred mode */\n> > > + LOG(Af, Warning) << \"AfPauseDeferred is not supported!\";\n> > > + break;\n> > > + case controls::AfPauseResume:\n> > > + pauseState_ = controls::AfPauseStateRunning;\n> > > + break;\n> > > + default:\n> > > + break;\n> >\n> > Again I wonder if default is a good idea, as if the controls get\n> > augmented I would prefer the compiler to complain if not all cases are\n> > handled..\n>\n> I totally agree. I will remove the \"default\" branch.\n>\n> >\n> > > + }\n> > > +}\n> > > +\n> > > +void AfHillClimbing::setLensPosition(float lensPosition)\n> > > +{\n> > > + if (mode_ != controls::AfModeManual) {\n> > > + LOG(Af, Warning)\n> > > + << __FUNCTION__ << \" not possible in mode \" << mode_;\n> > > + return;\n> > > + }\n> > > +\n> > > + lensPosition_ = 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> > > + int32_t diff = std::abs(lensPosition_) * fineRange_;\n> > > + lensPosition_ = std::max(lensPosition_ - diff, minLensPosition_);\n> > > + maxStep_ = std::min(lensPosition_ + diff, maxLensPosition_);\n> > > + previousContrast_ = 0;\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> > > + fineCompleted_ = true;\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> > > + * 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_ += 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> > > + previousContrast_ = currentContrast_;\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> > > + previousContrast_ = 0.0;\n> > > + coarseCompleted_ = false;\n> > > + fineCompleted_ = false;\n> > > + maxContrast_ = 0.0;\n> > > + setFramesToSkip(1);\n> > > +}\n> > > +\n> > > +bool AfHillClimbing::afIsOutOfFocus()\n> > > +{\n> > > + const uint32_t 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> > > + if (var_ratio > maxChange_)\n> > > + return true;\n> > > + else\n> > > + return false;\n> > > +}\n> >\n> > I presume the five functions above come from the existing\n> > implementation and I'm not really reviewing them :)\n>\n> This is true. I made some minor changes to them, mostly a code\n> formatting.\n>\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 libcamera::ipa::common::algorithms */\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..b361e5a1\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > @@ -0,0 +1,93 @@\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::common::algorithms {\n> > > +\n> > > +LOG_DECLARE_CATEGORY(Af)\n> > > +\n> > > +class AfHillClimbing : public Af\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> > > + void processAutoMode();\n> > > + void processContinousMode();\n> > > + void afCoarseScan();\n> > > + void afFineScan();\n> > > + bool afScan(uint32_t steps);\n> > > + void afReset();\n> > > + bool afIsOutOfFocus();\n> > > + bool shouldSkipFrame();\n> > > +\n> > > + controls::AfModeEnum mode_ = controls::AfModeManual;\n> > > + controls::AfStateEnum state_ = controls::AfStateIdle;\n> > > + controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning;\n> > > +\n> > > + /* VCM step configuration. It is the current setting of the VCM step. */\n> > > + int32_t lensPosition_ = 0;\n> > > + /* The best VCM step. It is a local optimum VCM step during scanning. */\n> > > + int32_t bestPosition_ = 0;\n> >\n> > Do you mean \"step\" or \"position\" in the comments ?\n>\n> Yes, the \"position\" is more clear.\n>\n> >\n> > > +\n> > > + /* Current AF statistic contrast. */\n> > > + double currentContrast_ = 0;\n> > > + /* It is used to determine the derivative during scanning */\n> > > + double previousContrast_ = 0;\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> > > + /* If the fine scan completes, it is set to true. */\n> > > + bool fineCompleted_ = false;\n> > > +\n> > > + uint32_t framesToSkip_ = 0;\n> > > +\n> > > + /* Focus steps range of the lens */\n> > > + int32_t minLensPosition_;\n> > > + int32_t maxLensPosition_;\n> > > +\n> > > + /* Minimum focus 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::common::algorithms */\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> > > index 7602976c..fa4b9564 100644\n> > > --- a/src/ipa/libipa/algorithms/meson.build\n> > > +++ b/src/ipa/libipa/algorithms/meson.build\n> > > @@ -2,8 +2,10 @@\n> > >\n> > > common_ipa_algorithms_headers = files([\n> > > 'af.h',\n> > > + 'af_hill_climbing.h',\n> > > ])\n> > >\n> > > common_ipa_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 71A87C0F2A\n\tfor ;\n\tTue, 21 Mar 2023 08:15:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 97935626D8;\n\tTue, 21 Mar 2023 09:15:49 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 97161626D8\n\tfor ;\n\tTue, 21 Mar 2023 09:15:47 +0100 (CET)","from ideasonboard.com (host-87-18-61-243.retail.telecomitalia.it\n\t[87.18.61.243])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 33A41496;\n\tTue, 21 Mar 2023 09:15:46 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679386549;\n\tbh=CgMlu9LRLqJ4mfRhuQhXqx0lCaocP2gjmarqcd2FWPk=;\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=CqIDU0PgKd8TV2msenNwm4gxt4+dPrQRBzauUK0wmQ2PFmLGZNfYQGrj3MRQ068BZ\n\tZybEvI8n0jxPG5mezM0KFR8UZ0tHalIGgUrqp9ApZAe4/fWytBL0xRSNlqCuVoVOCg\n\tL+CPeJJRuZUBcqqquUw17CmqIe1zGAuqkni9//qt4VDkZXZB4byciFF5V409lqKfFV\n\tvU4dEfCwO4YAN74bzdeVOCfEW2cac+TkVcMYgwoscLoMtOmk3b46I2Bw6kMdt7yhsN\n\tnhwB6pbP2Fu9bIKeXEo3XyunEeV2mzODSyHSIc9F8pfFGPLEX1GbRxoSWqk82GI9UX\n\tzTtKJYcCE5XIg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1679386547;\n\tbh=CgMlu9LRLqJ4mfRhuQhXqx0lCaocP2gjmarqcd2FWPk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qcdb/12k1CvpZaGGWVEOKEKKbGSrYv7sK6hBuazeHKHFLXaK9UvrzytcS6nmsc8KZ\n\t3/r1+u6H4kqFol6Ahouv1LhHXQE1QJCR4COWOdb+03Mib4ybdSh1o3Sk2Zmmu6Zfc9\n\t/wfmtNq2xqLSy0eLWyyilhheF4BMw3Pb1bJpg0SU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"qcdb/12k\"; dkim-atps=neutral","Date":"Tue, 21 Mar 2023 09:15:42 +0100","To":"Daniel Semkowicz ","Message-ID":"<20230321081542.sabwefml6ta5a4aa@uno.localdomain>","References":"<20230314144834.85193-1-dse@thaumatec.com>\n\t<20230314144834.85193-5-dse@thaumatec.com>\n\t<20230320204609.jyqz65nb6hvkgjll@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 v4 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":26698,"web_url":"https://patchwork.libcamera.org/comment/26698/","msgid":"<20230321142748.s6pnhw3mlvt6vion@uno.localdomain>","date":"2023-03-21T14:27:48","subject":"Re: [libcamera-devel] [PATCH v4 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 Tue, Mar 14, 2023 at 03:48:28PM +0100, Daniel Semkowicz 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 | 358 ++++++++++++++++++\n> src/ipa/libipa/algorithms/af_hill_climbing.h | 93 +++++\n> src/ipa/libipa/algorithms/meson.build | 2 +\n> 3 files changed, 453 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..2b99afef\n> --- /dev/null\n> +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> @@ -0,0 +1,358 @@\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::ipa::common::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> + * basing on the contrast measure supplied by the higher level. This way it is\n> + * independent from the platform.\n> + *\n> + * Platform layer that use this algorithm should call process() method\n> + * for each measured contrast value 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, lens are moved with bigger steps to quickly find a rough\n> + * position for the best focus. Then, basing on the outcome of coarse search,\n> + * the second phase is executed. Lens are moved with smaller steps in a limited\n> + * range within the rough position to find the exact position for best focus.\n> + *\n> + * Tuning file parameters:\n> + * - **coarse-search-step:** The value by which the position will change in one\n> + * step in the *coarse search* phase.\n> + * - **fine-search-step:** The value by which the position will change in one\n> + * step in the *fine search* phase.\n> + * - **fine-search-range:** Search range in the *fine search* phase.\n> + * Valid values are in the [0.0, 1.0] interval. Value 0.05 means 5%. If coarse\n> + * search stopped 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> +/**\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(0.05);\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 new contrast value that was measured.\n> + *\n> + * \\return New lens position calculated by 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\tprocessContinousMode();\n> +\t\tbreak;\n> +\tdefault:\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::processContinousMode()\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/* We can re-start the scan at any moment in AfModeContinuous */\n> +\tif (afIsOutOfFocus()) {\n> +\t\tafReset();\n> +\t}\n> +}\n> +\n> +/**\n> + * \\brief Request AF to skip n frames\n> + * \\param[in] n Number of frames to be skipped\n> + *\n> + * Requested number of frames will not be used for AF calculation.\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> +void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)\n> +{\n> +\tif (mode_ != controls::AfModeAuto) {\n> +\t\tLOG(Af, Warning)\n> +\t\t\t<< __FUNCTION__ << \" not possible in mode \" << mode_;\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> +}\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 possible in mode \" << mode_;\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\t/* \\todo: add the AfPauseDeferred mode */\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> +\tdefault:\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 possible in mode \" << mode_;\n> +\t\treturn;\n> +\t}\n> +\n> +\tlensPosition_ = 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\tint32_t diff = std::abs(lensPosition_) * fineRange_;\n> +\t\tlensPosition_ = std::max(lensPosition_ - diff, minLensPosition_);\n> +\t\tmaxStep_ = std::min(lensPosition_ + diff, maxLensPosition_);\n> +\t\tpreviousContrast_ = 0;\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\tfineCompleted_ = true;\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> +\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\nI just noticed most multi-line comments in the series have a wrong\nindentation. Could you re-check them please ?\n\nThanks\n j\n\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_ += 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> +\tpreviousContrast_ = currentContrast_;\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> +\tpreviousContrast_ = 0.0;\n> +\tcoarseCompleted_ = false;\n> +\tfineCompleted_ = false;\n> +\tmaxContrast_ = 0.0;\n> +\tsetFramesToSkip(1);\n> +}\n> +\n> +bool AfHillClimbing::afIsOutOfFocus()\n> +{\n> +\tconst uint32_t 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> +\tif (var_ratio > maxChange_)\n> +\t\treturn true;\n> +\telse\n> +\t\treturn false;\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 libcamera::ipa::common::algorithms */\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..b361e5a1\n> --- /dev/null\n> +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> @@ -0,0 +1,93 @@\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::common::algorithms {\n> +\n> +LOG_DECLARE_CATEGORY(Af)\n> +\n> +class AfHillClimbing : public Af\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> +\n> +\tvoid processAutoMode();\n> +\tvoid processContinousMode();\n> +\tvoid afCoarseScan();\n> +\tvoid afFineScan();\n> +\tbool afScan(uint32_t steps);\n> +\tvoid afReset();\n> +\tbool afIsOutOfFocus();\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/* VCM step configuration. It is the current setting of the VCM step. */\n> +\tint32_t lensPosition_ = 0;\n> +\t/* The best VCM step. It is a local optimum VCM step 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 previousContrast_ = 0;\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> +\t/* If the fine scan completes, it is set to true. */\n> +\tbool fineCompleted_ = false;\n> +\n> +\tuint32_t framesToSkip_ = 0;\n> +\n> +\t/* Focus steps range of the lens */\n> +\tint32_t minLensPosition_;\n> +\tint32_t maxLensPosition_;\n> +\n> +\t/* Minimum focus 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::common::algorithms */\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> index 7602976c..fa4b9564 100644\n> --- a/src/ipa/libipa/algorithms/meson.build\n> +++ b/src/ipa/libipa/algorithms/meson.build\n> @@ -2,8 +2,10 @@\n>\n> common_ipa_algorithms_headers = files([\n> 'af.h',\n> + 'af_hill_climbing.h',\n> ])\n>\n> common_ipa_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 47EFAC0F1B\n\tfor ;\n\tTue, 21 Mar 2023 14:27:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8FC2B626E3;\n\tTue, 21 Mar 2023 15:27:55 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BA26D61ED3\n\tfor ;\n\tTue, 21 Mar 2023 15:27:53 +0100 (CET)","from ideasonboard.com (host-87-18-61-243.retail.telecomitalia.it\n\t[87.18.61.243])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9AAC48B;\n\tTue, 21 Mar 2023 15:27:52 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679408875;\n\tbh=uXoQY5gQ7Qbi2EgVqO1yJg2diczw0N/O5ekB0KPbyb4=;\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=Mr3mVdmCeBvySmlXhvT+Fvqb89/+hK1Kdp9gzVF7uCU5lKatOw6m+q3ITsrCPVfGf\n\tFAmPvZyoUgCAids2LA5hW3+8T9v+OvSR8BkLElIVrnOmCg0zYOwg2//M2JN1mNg3dr\n\tC7M3nJXdPP10o2ALf8+Bt862Ab7OI8zu01ioE/l4/eZOtnDEFKrWal67DH6eEFC9yY\n\teQ0EWoHKPidPDxF1UNKrOjRH+E6Mhlt2jnC1WjG77+9FAgQe8eQVv8KPTtsxp4giXN\n\tQ6/aYRhOl2QskhVw5I7oHC9H0ry18EgUSSOW82+YD5nrfNJJsWAtCXhnYiNg5lugNk\n\t+GJs5WXZ9Anqw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1679408873;\n\tbh=uXoQY5gQ7Qbi2EgVqO1yJg2diczw0N/O5ekB0KPbyb4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wS71kH9gz0SL1/S2Sf5mjDrhRH44zAFFuLiB4f5eShPUD9Zsetk6j39uurmltkXPT\n\tUMy7tGDE9BcADyA4t2FVdARtNOY/fE9GxTrrUqtRaVJRAWyUyGxmqPHtYicK2mpAZZ\n\tJQYi/bss4aDG4q8lisM8Ohci/lHjHWjUD3NFxi5o="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"wS71kH9g\"; dkim-atps=neutral","Date":"Tue, 21 Mar 2023 15:27:48 +0100","To":"Daniel Semkowicz ","Message-ID":"<20230321142748.s6pnhw3mlvt6vion@uno.localdomain>","References":"<20230314144834.85193-1-dse@thaumatec.com>\n\t<20230314144834.85193-5-dse@thaumatec.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230314144834.85193-5-dse@thaumatec.com>","Subject":"Re: [libcamera-devel] [PATCH v4 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":26699,"web_url":"https://patchwork.libcamera.org/comment/26699/","msgid":"<20230321144607.a7jri74sxcidrgha@uno.localdomain>","date":"2023-03-21T14:46:07","subject":"Re: [libcamera-devel] [PATCH v4 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":"Sorry for this bits&pieces review\n\nOn Tue, Mar 14, 2023 at 03:48:28PM +0100, Daniel Semkowicz 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 | 358 ++++++++++++++++++\n> src/ipa/libipa/algorithms/af_hill_climbing.h | 93 +++++\n> src/ipa/libipa/algorithms/meson.build | 2 +\n> 3 files changed, 453 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..2b99afef\n> --- /dev/null\n> +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> @@ -0,0 +1,358 @@\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::ipa::common::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> + * basing on the contrast measure supplied by the higher level. This way it is\n> + * independent from the platform.\n> + *\n> + * Platform layer that use this algorithm should call process() method\n> + * for each measured contrast value 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, lens are moved with bigger steps to quickly find a rough\n> + * position for the best focus. Then, basing on the outcome of coarse search,\n> + * the second phase is executed. Lens are moved with smaller steps in a limited\n> + * range within the rough position to find the exact position for best focus.\n> + *\n> + * Tuning file parameters:\n> + * - **coarse-search-step:** The value by which the position will change in one\n> + * step in the *coarse search* phase.\n> + * - **fine-search-step:** The value by which the position will change in one\n> + * step in the *fine search* phase.\n> + * - **fine-search-range:** Search range in the *fine search* phase.\n> + * Valid values are in the [0.0, 1.0] interval. Value 0.05 means 5%. If coarse\n> + * search stopped 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> +/**\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(0.05);\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 new contrast value that was measured.\n> + *\n> + * \\return New lens position calculated by 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\tprocessContinousMode();\n> +\t\tbreak;\n> +\tdefault:\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::processContinousMode()\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/* We can re-start the scan at any moment in AfModeContinuous */\n> +\tif (afIsOutOfFocus()) {\n> +\t\tafReset();\n> +\t}\n\nMaybe already pointed out but no need for {}\n\nI'm wondering, should we check first if the image is out-of-focus\nso that we can run a scan immediatly ? (in other words, move this\nblock before the previous one.\n\n> +}\n> +\n> +/**\n> + * \\brief Request AF to skip n frames\n> + * \\param[in] n Number of frames to be skipped\n> + *\n> + * Requested number of frames will not be used for AF calculation.\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> +void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)\n> +{\n> +\tif (mode_ != controls::AfModeAuto) {\n> +\t\tLOG(Af, Warning)\n> +\t\t\t<< __FUNCTION__ << \" not possible in mode \" << mode_;\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> +}\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 possible in mode \" << mode_;\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\t/* \\todo: add the AfPauseDeferred mode */\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> +\tdefault:\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 possible in mode \" << mode_;\n> +\t\treturn;\n> +\t}\n> +\n> +\tlensPosition_ = 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\tint32_t diff = std::abs(lensPosition_) * fineRange_;\n> +\t\tlensPosition_ = std::max(lensPosition_ - diff, minLensPosition_);\n> +\t\tmaxStep_ = std::min(lensPosition_ + diff, maxLensPosition_);\n> +\t\tpreviousContrast_ = 0;\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\tfineCompleted_ = true;\n\nfineCompleted_ is never used, just set here and reset at afReset()\ntime.\n\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> +\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_ += 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> +\tpreviousContrast_ = currentContrast_;\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> +\tpreviousContrast_ = 0.0;\n> +\tcoarseCompleted_ = false;\n> +\tfineCompleted_ = false;\n> +\tmaxContrast_ = 0.0;\n> +\tsetFramesToSkip(1);\n> +}\n> +\n> +bool AfHillClimbing::afIsOutOfFocus()\n> +{\n> +\tconst uint32_t 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> +\tif (var_ratio > maxChange_)\n> +\t\treturn true;\n> +\telse\n> +\t\treturn false;\n\nor simply\n\n return var_ratio > maxChange_;\n\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 libcamera::ipa::common::algorithms */\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..b361e5a1\n> --- /dev/null\n> +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> @@ -0,0 +1,93 @@\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::common::algorithms {\n> +\n> +LOG_DECLARE_CATEGORY(Af)\n> +\n> +class AfHillClimbing : public Af\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> +\n> +\tvoid processAutoMode();\n> +\tvoid processContinousMode();\n> +\tvoid afCoarseScan();\n> +\tvoid afFineScan();\n> +\tbool afScan(uint32_t steps);\n> +\tvoid afReset();\n> +\tbool afIsOutOfFocus();\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/* VCM step configuration. It is the current setting of the VCM step. */\n> +\tint32_t lensPosition_ = 0;\n> +\t/* The best VCM step. It is a local optimum VCM step 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 previousContrast_ = 0;\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> +\t/* If the fine scan completes, it is set to true. */\n> +\tbool fineCompleted_ = false;\n> +\n> +\tuint32_t framesToSkip_ = 0;\n> +\n> +\t/* Focus steps range of the lens */\n> +\tint32_t minLensPosition_;\n> +\tint32_t maxLensPosition_;\n> +\n> +\t/* Minimum focus 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::common::algorithms */\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> index 7602976c..fa4b9564 100644\n> --- a/src/ipa/libipa/algorithms/meson.build\n> +++ b/src/ipa/libipa/algorithms/meson.build\n> @@ -2,8 +2,10 @@\n>\n> common_ipa_algorithms_headers = files([\n> 'af.h',\n> + 'af_hill_climbing.h',\n> ])\n>\n> common_ipa_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 5A9A6C0F2A\n\tfor ;\n\tTue, 21 Mar 2023 14:46:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 832AF626E3;\n\tTue, 21 Mar 2023 15:46:12 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 07DF961ED3\n\tfor ;\n\tTue, 21 Mar 2023 15:46:11 +0100 (CET)","from ideasonboard.com (host-87-18-61-243.retail.telecomitalia.it\n\t[87.18.61.243])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D328FFB;\n\tTue, 21 Mar 2023 15:46:09 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679409972;\n\tbh=4doWQdv9632k7Y8yNJq1CZm2Pu9vl/TRii1zcdF6XDo=;\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=SKlsGf/egFiTV35NKjMgFS73xZTG7NEKDzhikyGJ8xrhZu3duThJp8R+elxsKdNdF\n\tkeyA4qzXhQip9QK4MiHTfwRTk4e7disSBHCPl9/97abjj1bSxcHW470pAcWN+4VO/8\n\toR2YJQPIOoa7cEpyW0MrgQd+kerXszV/TNzw2q5/0S20+f9D+Vsp3YZkz2gZ8A2cHP\n\tafYrz635vxbT6g2qRaTmjGGIK7EZoJb2sNGipZvyJ0E/GEFazrMoAlc5D3RMKf7r2W\n\tDyIbmwsRR0oDw7BQugaiSqTEIh13zddpqec8fe2GWohJkdavv7lVJaO6zSC+3TnSlW\n\tUxVUQbVfK3GXQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1679409970;\n\tbh=4doWQdv9632k7Y8yNJq1CZm2Pu9vl/TRii1zcdF6XDo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TIKhEbICWg0ZOaWe1pJbTpmc8dtGTbKPKanbbYJyVHqG6xtUJkIBFnfSgctILYQzu\n\tYFTBhQP4mRTTYQW//HIbeQ/V0v3o3DuKqXPnmNo8usPuSCXUyCvqc2655lhfJmb2Hg\n\tiVs1mUIDkpTWKYvwghS4Wk6h1NUhxt01q6KS9LBA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"TIKhEbIC\"; dkim-atps=neutral","Date":"Tue, 21 Mar 2023 15:46:07 +0100","To":"Daniel Semkowicz ","Message-ID":"<20230321144607.a7jri74sxcidrgha@uno.localdomain>","References":"<20230314144834.85193-1-dse@thaumatec.com>\n\t<20230314144834.85193-5-dse@thaumatec.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230314144834.85193-5-dse@thaumatec.com>","Subject":"Re: [libcamera-devel] [PATCH v4 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":26707,"web_url":"https://patchwork.libcamera.org/comment/26707/","msgid":"","date":"2023-03-22T11:27:09","subject":"Re: [libcamera-devel] [PATCH v4 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 Tue, Mar 21, 2023 at 3:46 PM Jacopo Mondi\n wrote:\n>\n> Sorry for this bits&pieces review\n>\n> On Tue, Mar 14, 2023 at 03:48:28PM +0100, Daniel Semkowicz 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 | 358 ++++++++++++++++++\n> > src/ipa/libipa/algorithms/af_hill_climbing.h | 93 +++++\n> > src/ipa/libipa/algorithms/meson.build | 2 +\n> > 3 files changed, 453 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..2b99afef\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > @@ -0,0 +1,358 @@\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::ipa::common::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> > + * basing on the contrast measure supplied by the higher level. This way it is\n> > + * independent from the platform.\n> > + *\n> > + * Platform layer that use this algorithm should call process() method\n> > + * for each measured contrast value 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, lens are moved with bigger steps to quickly find a rough\n> > + * position for the best focus. Then, basing on the outcome of coarse search,\n> > + * the second phase is executed. Lens are moved with smaller steps in a limited\n> > + * range within the rough position to find the exact position for best focus.\n> > + *\n> > + * Tuning file parameters:\n> > + * - **coarse-search-step:** The value by which the position will change in one\n> > + * step in the *coarse search* phase.\n> > + * - **fine-search-step:** The value by which the position will change in one\n> > + * step in the *fine search* phase.\n> > + * - **fine-search-range:** Search range in the *fine search* phase.\n> > + * Valid values are in the [0.0, 1.0] interval. Value 0.05 means 5%. If coarse\n> > + * search stopped 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> > +/**\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(0.05);\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 new contrast value that was measured.\n> > + *\n> > + * \\return New lens position calculated by 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> > + processContinousMode();\n> > + break;\n> > + default:\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::processContinousMode()\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> > + /* We can re-start the scan at any moment in AfModeContinuous */\n> > + if (afIsOutOfFocus()) {\n> > + afReset();\n> > + }\n>\n> Maybe already pointed out but no need for {}\n>\n> I'm wondering, should we check first if the image is out-of-focus\n> so that we can run a scan immediatly ? (in other words, move this\n> block before the previous one.\n\nWe can't just move it. Is is intentionally in this place.\nFirst, there is a check if current state is AfStateScanning, because\nin this state We need to handle the already running AF process.\nTherefore, code block with afIsOutOfFocus() is executed only if the\ncurrent state is different than AfStateScanning. And this is correct,\nbecause We do not want to reset the already running AF process.\n>\n> > +}\n> > +\n> > +/**\n> > + * \\brief Request AF to skip n frames\n> > + * \\param[in] n Number of frames to be skipped\n> > + *\n> > + * Requested number of frames will not be used for AF calculation.\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> > +void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)\n> > +{\n> > + if (mode_ != controls::AfModeAuto) {\n> > + LOG(Af, Warning)\n> > + << __FUNCTION__ << \" not possible in mode \" << mode_;\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> > +\n> > +void AfHillClimbing::setPause(controls::AfPauseEnum pause)\n> > +{\n> > + if (mode_ != controls::AfModeContinuous) {\n> > + LOG(Af, Warning)\n> > + << __FUNCTION__ << \" not possible in mode \" << mode_;\n> > + return;\n> > + }\n> > +\n> > + switch (pause) {\n> > + case controls::AfPauseImmediate:\n> > + pauseState_ = controls::AfPauseStatePaused;\n> > + break;\n> > + case controls::AfPauseDeferred:\n> > + /* \\todo: add the AfPauseDeferred mode */\n> > + LOG(Af, Warning) << \"AfPauseDeferred is not supported!\";\n> > + break;\n> > + case controls::AfPauseResume:\n> > + pauseState_ = controls::AfPauseStateRunning;\n> > + break;\n> > + default:\n> > + break;\n> > + }\n> > +}\n> > +\n> > +void AfHillClimbing::setLensPosition(float lensPosition)\n> > +{\n> > + if (mode_ != controls::AfModeManual) {\n> > + LOG(Af, Warning)\n> > + << __FUNCTION__ << \" not possible in mode \" << mode_;\n> > + return;\n> > + }\n> > +\n> > + lensPosition_ = 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> > + int32_t diff = std::abs(lensPosition_) * fineRange_;\n> > + lensPosition_ = std::max(lensPosition_ - diff, minLensPosition_);\n> > + maxStep_ = std::min(lensPosition_ + diff, maxLensPosition_);\n> > + previousContrast_ = 0;\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> > + fineCompleted_ = true;\n>\n> fineCompleted_ is never used, just set here and reset at afReset()\n> time.\n\nGood catch, it can be removed. Thanks!\n\n>\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> > + * 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_ += 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> > + previousContrast_ = currentContrast_;\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> > + previousContrast_ = 0.0;\n> > + coarseCompleted_ = false;\n> > + fineCompleted_ = false;\n> > + maxContrast_ = 0.0;\n> > + setFramesToSkip(1);\n> > +}\n> > +\n> > +bool AfHillClimbing::afIsOutOfFocus()\n> > +{\n> > + const uint32_t 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> > + if (var_ratio > maxChange_)\n> > + return true;\n> > + else\n> > + return false;\n>\n> or simply\n>\n> return var_ratio > maxChange_;\n>\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 libcamera::ipa::common::algorithms */\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..b361e5a1\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > @@ -0,0 +1,93 @@\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::common::algorithms {\n> > +\n> > +LOG_DECLARE_CATEGORY(Af)\n> > +\n> > +class AfHillClimbing : public Af\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> > + void processAutoMode();\n> > + void processContinousMode();\n> > + void afCoarseScan();\n> > + void afFineScan();\n> > + bool afScan(uint32_t steps);\n> > + void afReset();\n> > + bool afIsOutOfFocus();\n> > + bool shouldSkipFrame();\n> > +\n> > + controls::AfModeEnum mode_ = controls::AfModeManual;\n> > + controls::AfStateEnum state_ = controls::AfStateIdle;\n> > + controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning;\n> > +\n> > + /* VCM step configuration. It is the current setting of the VCM step. */\n> > + int32_t lensPosition_ = 0;\n> > + /* The best VCM step. It is a local optimum VCM step 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 previousContrast_ = 0;\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> > + /* If the fine scan completes, it is set to true. */\n> > + bool fineCompleted_ = false;\n> > +\n> > + uint32_t framesToSkip_ = 0;\n> > +\n> > + /* Focus steps range of the lens */\n> > + int32_t minLensPosition_;\n> > + int32_t maxLensPosition_;\n> > +\n> > + /* Minimum focus 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::common::algorithms */\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> > index 7602976c..fa4b9564 100644\n> > --- a/src/ipa/libipa/algorithms/meson.build\n> > +++ b/src/ipa/libipa/algorithms/meson.build\n> > @@ -2,8 +2,10 @@\n> >\n> > common_ipa_algorithms_headers = files([\n> > 'af.h',\n> > + 'af_hill_climbing.h',\n> > ])\n> >\n> > common_ipa_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 291FAC0F2A\n\tfor ;\n\tWed, 22 Mar 2023 11:27:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7569A61ECE;\n\tWed, 22 Mar 2023 12:27:22 +0100 (CET)","from mail-ed1-x532.google.com (mail-ed1-x532.google.com\n\t[IPv6:2a00:1450:4864:20::532])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ACDCA61ECE\n\tfor ;\n\tWed, 22 Mar 2023 12:27:20 +0100 (CET)","by mail-ed1-x532.google.com with SMTP id ek18so71473723edb.6\n\tfor ;\n\tWed, 22 Mar 2023 04:27:20 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679484442;\n\tbh=sCBL47HcmJrZKxQjOJcFOrHywBCw1gYcTXFauXy5YaA=;\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=pnnMgGxK+RvBKdridHeIM/L/eHBTGnfzvzQxv4k0Gw96vKcGkzMWVDI+bKGvPAfAT\n\t8Kr07GQLQQjgXvhR3LX86Vi7RTK+hV82oM8NqtH6dnq6cntJgjv/EORopdGY4zKCw6\n\tPyjX5UXuJTkVD2ncOkuPyE2kM7tqPaNXuL72M6LCglBI9T6LNQKUfFz6s6rkqMhBm0\n\tm4yz+89yI5ShduMH/blogDJBiTC9ZRtApoEm0WURgt6/tjD94cOwurNhaoxvMPyFud\n\tORKeMWRX/BlgMQDwa8t8SG+RBA6RX2UsDqJIH2uOMRYavAmKzZ4w6kEcfxy8siQKbV\n\t7MgFMI8T614gQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20210112.gappssmtp.com; s=20210112; t=1679484440;\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=z9gTWokn5rLjCmcPP66IoKJLY04uESvcvL4xT0/8kxQ=;\n\tb=Drfa0/Ltr1QCuYY7FsCoXhDiBs2ux7xV2jCtCHI2iaEwx6Yrs2+dzg5EKW7mASaDWB\n\t3+FGCv/747P9u8ffCVHAcAK/sksHyHTPuf67C4Z5qZSMMMYXzBxSRaqmvVYQ6AJh6Rj/\n\tITKoUZBwP1mUgf9EWOehLUsEZNpTmobKYs3n3q6ryBuPAqv4fgWPYOZb8ox9rChKFrng\n\tKich2MtLu6SttCP6tZolPqUJNbMUrqzk3Ww3Y+UZ/sjwuws9lunjpZDQ9O4Lw0NqQRb7\n\tmKAVbnDBar/aGJHG6pd7pq4kGb0B6coM0snhrFjDJhyks54nVpsnr8oNxIaT1xAwMQje\n\tbC2g=="],"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=\"Drfa0/Lt\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1679484440;\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=z9gTWokn5rLjCmcPP66IoKJLY04uESvcvL4xT0/8kxQ=;\n\tb=S+bZ0yvtJqzucOi+8tAakmsJBlBo4xaxZEupRyxNb1E3dgbPlYkZw0ATZNn5OsYTwW\n\tf/LiC5Q8rvSPpmlc/ESplxFwWKd1bLKg69zhPX0e6JyLPTmBJRdSm+qXXCBa1yNPlpxB\n\tMwJFCh47ytJ+IfhZoJTpZZiGVszAXOa8mbg2OW40SO0FfLyKhgp0/25jBklxSfssf082\n\tpcDqOH/PHwawMGCQ4XeVXbhb9EVAyvA6ET4VIcWhBwgYHDBSuqirInZZG9FfnZxpfUwT\n\t4aNTebrRGQVoxWH6Xy4GlWscbFxSuj0S4QCw7/MKwwkJxjYUT/bPfmHDHwM4fHSFIgN0\n\tIE8A==","X-Gm-Message-State":"AO0yUKXBAyS6z9ZOPtvRRbMzG8MJjYfyvu1xe13sP0IE6xu+yjGxnUWe\n\tEVgZJ09af7FweWJX+pWWw+fSXuaMAdNcx+CQqB+67A==","X-Google-Smtp-Source":"AK7set+Eu9BFJ+60vIBQ2ksYNq5hJUyUKmooi/fcGQT983qk3lu6ocQs6rMTkBI3iMP5dUJ585TGUtHcl/aZ2QJvEHI=","X-Received":"by 2002:a17:906:f9d1:b0:924:efbb:8a8b with SMTP id\n\tlj17-20020a170906f9d100b00924efbb8a8bmr2929163ejb.6.1679484440128;\n\tWed, 22 Mar 2023 04:27:20 -0700 (PDT)","MIME-Version":"1.0","References":"<20230314144834.85193-1-dse@thaumatec.com>\n\t<20230314144834.85193-5-dse@thaumatec.com>\n\t<20230321144607.a7jri74sxcidrgha@uno.localdomain>","In-Reply-To":"<20230321144607.a7jri74sxcidrgha@uno.localdomain>","Date":"Wed, 22 Mar 2023 12:27:09 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v4 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":26708,"web_url":"https://patchwork.libcamera.org/comment/26708/","msgid":"","date":"2023-03-22T11:31:28","subject":"Re: [libcamera-devel] [PATCH v4 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":"On Wed, Mar 22, 2023 at 12:27 PM Daniel Semkowicz wrote:\n>\n> Hi Jacopo,\n>\n> On Tue, Mar 21, 2023 at 3:46 PM Jacopo Mondi\n> wrote:\n> >\n> > Sorry for this bits&pieces review\n> >\n> > On Tue, Mar 14, 2023 at 03:48:28PM +0100, Daniel Semkowicz 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 | 358 ++++++++++++++++++\n> > > src/ipa/libipa/algorithms/af_hill_climbing.h | 93 +++++\n> > > src/ipa/libipa/algorithms/meson.build | 2 +\n> > > 3 files changed, 453 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..2b99afef\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp\n> > > @@ -0,0 +1,358 @@\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::ipa::common::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> > > + * basing on the contrast measure supplied by the higher level. This way it is\n> > > + * independent from the platform.\n> > > + *\n> > > + * Platform layer that use this algorithm should call process() method\n> > > + * for each measured contrast value 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, lens are moved with bigger steps to quickly find a rough\n> > > + * position for the best focus. Then, basing on the outcome of coarse search,\n> > > + * the second phase is executed. Lens are moved with smaller steps in a limited\n> > > + * range within the rough position to find the exact position for best focus.\n> > > + *\n> > > + * Tuning file parameters:\n> > > + * - **coarse-search-step:** The value by which the position will change in one\n> > > + * step in the *coarse search* phase.\n> > > + * - **fine-search-step:** The value by which the position will change in one\n> > > + * step in the *fine search* phase.\n> > > + * - **fine-search-range:** Search range in the *fine search* phase.\n> > > + * Valid values are in the [0.0, 1.0] interval. Value 0.05 means 5%. If coarse\n> > > + * search stopped 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> > > +/**\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(0.05);\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 new contrast value that was measured.\n> > > + *\n> > > + * \\return New lens position calculated by 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> > > + processContinousMode();\n> > > + break;\n> > > + default:\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::processContinousMode()\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> > > + /* We can re-start the scan at any moment in AfModeContinuous */\n> > > + if (afIsOutOfFocus()) {\n> > > + afReset();\n> > > + }\n> >\n> > Maybe already pointed out but no need for {}\n> >\n> > I'm wondering, should we check first if the image is out-of-focus\n> > so that we can run a scan immediatly ? (in other words, move this\n> > block before the previous one.\n>\n> We can't just move it. Is is intentionally in this place.\n> First, there is a check if current state is AfStateScanning, because\n> in this state We need to handle the already running AF process.\n> Therefore, code block with afIsOutOfFocus() is executed only if the\n> current state is different than AfStateScanning. And this is correct,\n> because We do not want to reset the already running AF process.\n\nAn idea just came to my mind: maybe I should rephrase the comment above\nthe code block to make this logic more clear?\n\nBest regards\nDaniel\n> >\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Request AF to skip n frames\n> > > + * \\param[in] n Number of frames to be skipped\n> > > + *\n> > > + * Requested number of frames will not be used for AF calculation.\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> > > +void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)\n> > > +{\n> > > + if (mode_ != controls::AfModeAuto) {\n> > > + LOG(Af, Warning)\n> > > + << __FUNCTION__ << \" not possible in mode \" << mode_;\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> > > +\n> > > +void AfHillClimbing::setPause(controls::AfPauseEnum pause)\n> > > +{\n> > > + if (mode_ != controls::AfModeContinuous) {\n> > > + LOG(Af, Warning)\n> > > + << __FUNCTION__ << \" not possible in mode \" << mode_;\n> > > + return;\n> > > + }\n> > > +\n> > > + switch (pause) {\n> > > + case controls::AfPauseImmediate:\n> > > + pauseState_ = controls::AfPauseStatePaused;\n> > > + break;\n> > > + case controls::AfPauseDeferred:\n> > > + /* \\todo: add the AfPauseDeferred mode */\n> > > + LOG(Af, Warning) << \"AfPauseDeferred is not supported!\";\n> > > + break;\n> > > + case controls::AfPauseResume:\n> > > + pauseState_ = controls::AfPauseStateRunning;\n> > > + break;\n> > > + default:\n> > > + break;\n> > > + }\n> > > +}\n> > > +\n> > > +void AfHillClimbing::setLensPosition(float lensPosition)\n> > > +{\n> > > + if (mode_ != controls::AfModeManual) {\n> > > + LOG(Af, Warning)\n> > > + << __FUNCTION__ << \" not possible in mode \" << mode_;\n> > > + return;\n> > > + }\n> > > +\n> > > + lensPosition_ = 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> > > + int32_t diff = std::abs(lensPosition_) * fineRange_;\n> > > + lensPosition_ = std::max(lensPosition_ - diff, minLensPosition_);\n> > > + maxStep_ = std::min(lensPosition_ + diff, maxLensPosition_);\n> > > + previousContrast_ = 0;\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> > > + fineCompleted_ = true;\n> >\n> > fineCompleted_ is never used, just set here and reset at afReset()\n> > time.\n>\n> Good catch, it can be removed. Thanks!\n>\n> >\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> > > + * 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_ += 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> > > + previousContrast_ = currentContrast_;\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> > > + previousContrast_ = 0.0;\n> > > + coarseCompleted_ = false;\n> > > + fineCompleted_ = false;\n> > > + maxContrast_ = 0.0;\n> > > + setFramesToSkip(1);\n> > > +}\n> > > +\n> > > +bool AfHillClimbing::afIsOutOfFocus()\n> > > +{\n> > > + const uint32_t 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> > > + if (var_ratio > maxChange_)\n> > > + return true;\n> > > + else\n> > > + return false;\n> >\n> > or simply\n> >\n> > return var_ratio > maxChange_;\n> >\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 libcamera::ipa::common::algorithms */\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..b361e5a1\n> > > --- /dev/null\n> > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h\n> > > @@ -0,0 +1,93 @@\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::common::algorithms {\n> > > +\n> > > +LOG_DECLARE_CATEGORY(Af)\n> > > +\n> > > +class AfHillClimbing : public Af\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> > > + void processAutoMode();\n> > > + void processContinousMode();\n> > > + void afCoarseScan();\n> > > + void afFineScan();\n> > > + bool afScan(uint32_t steps);\n> > > + void afReset();\n> > > + bool afIsOutOfFocus();\n> > > + bool shouldSkipFrame();\n> > > +\n> > > + controls::AfModeEnum mode_ = controls::AfModeManual;\n> > > + controls::AfStateEnum state_ = controls::AfStateIdle;\n> > > + controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning;\n> > > +\n> > > + /* VCM step configuration. It is the current setting of the VCM step. */\n> > > + int32_t lensPosition_ = 0;\n> > > + /* The best VCM step. It is a local optimum VCM step 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 previousContrast_ = 0;\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> > > + /* If the fine scan completes, it is set to true. */\n> > > + bool fineCompleted_ = false;\n> > > +\n> > > + uint32_t framesToSkip_ = 0;\n> > > +\n> > > + /* Focus steps range of the lens */\n> > > + int32_t minLensPosition_;\n> > > + int32_t maxLensPosition_;\n> > > +\n> > > + /* Minimum focus 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::common::algorithms */\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build\n> > > index 7602976c..fa4b9564 100644\n> > > --- a/src/ipa/libipa/algorithms/meson.build\n> > > +++ b/src/ipa/libipa/algorithms/meson.build\n> > > @@ -2,8 +2,10 @@\n> > >\n> > > common_ipa_algorithms_headers = files([\n> > > 'af.h',\n> > > + 'af_hill_climbing.h',\n> > > ])\n> > >\n> > > common_ipa_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 EA05DC0F1B\n\tfor ;\n\tWed, 22 Mar 2023 11:31:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 46C2B626E5;\n\tWed, 22 Mar 2023 12:31:41 +0100 (CET)","from mail-ed1-x535.google.com (mail-ed1-x535.google.com\n\t[IPv6:2a00:1450:4864:20::535])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 68D3961ECE\n\tfor ;\n\tWed, 22 Mar 2023 12:31:40 +0100 (CET)","by mail-ed1-x535.google.com with SMTP id w9so71556081edc.3\n\tfor ;\n\tWed, 22 Mar 2023 04:31:40 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679484701;\n\tbh=kPEYyF7VNyTxegpvoQ5C55P7TftIdKY9CvT11kcJpTk=;\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=wfh39Wos1V1OTUVu4BV1LKSzpoDWF08OD+5mUipU/w8NAwpL6stxUrN3KyIX1yah6\n\tsmyCufqdkVaXaQQK67Waija+Re6XH6reFdv6gCiLvhRlDWS5KAZwC03qWYO9H96uT8\n\t3LG60Q0icYw9BYtCkfIaCFwwrb7Ug1t30AHP7Ep6HU1CJmqFfnLZhrUDf2Vx16JDf8\n\tapmDuq+SoJ9WLYlnPSpvYd/IfZhhaP5EyNeucIAyFPDruaamV8C1rAn3hiYNfJ5yhq\n\t+HpPNw4I/mLCgUTpbGcqHxgDW33lNs5Efa0cV1P0o9s4lu2JR9I1j/HGxmVlV/CdJN\n\tUk9UDbZJu1XsA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20210112.gappssmtp.com; s=20210112; t=1679484700;\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=V+zXkwsxPgYNUMFsSh+k0bHbNLMFfXTY5Wif0blXkL0=;\n\tb=D14n89VJxKCv9Uea0gDz5ErzYVRXPgKdT5sGKCEx5DRd/3qHSx56P7I77kBKl0YFwJ\n\tkq2+3RKanMgY9GvxJ8hiJPbKARgoa4Vxpl89yKQMPbgphnfl2MZ1TFEc42DBS5BUSu0l\n\t6II8YlfJ0zb0BLtRlcM7Jt+0/FMhbpqO08O8x+qW7ibrKkDplxdmeu/+Ezzo+NdrYiX6\n\tvUHl8oCPTE2OYw5YSQQYVc7bOb95wDJfniQp0tHG/myYDI2QzCgbjDKYg0fmOoVvwVZD\n\tak2OlimpAlQ09ZLDgR8eJRViOaYI540pmS41+0ZoLaU4iBZfc+0+Ly2I9Q3ei3J3V5an\n\tWuqg=="],"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=\"D14n89VJ\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1679484700;\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=V+zXkwsxPgYNUMFsSh+k0bHbNLMFfXTY5Wif0blXkL0=;\n\tb=feT/rGLtiw8gBFaKqZJhleNOYSH05nBdvHLMuEBre0Fh2RalNQV/dzMCTV4/h/FkLn\n\trkEXrGSHz1Ub3FpHsxdEAEPW2AKXStxa5/cXW0Li6f3ZoEPU9nzjcO6KmOk2Atakdrmq\n\tHpvjs6AVzdooOp6mIg4KzT6UWN6CTjp9HD3ELWwZajgpVc9Y9cWiZxAKruXIvmFZKlqO\n\tp4RJNM9zLB3hn0Tsv+hJcJHAISS5AyOfega0QS13lIMP2HpLoLnid+Q2i+uwX/dUsraX\n\tW/JvCXGlIXyBqd7sXrw1xMuWv5u3iP8PjtLgwwYMnhaHz/7eb74fsuTMP1Rzgryb801V\n\toaag==","X-Gm-Message-State":"AO0yUKUylYZGSYv4H3fDgVk9lzLYoXKNce26o9zZdxF6TMiqa+L6P7zs\n\tnR4nKCOLNAIcnspIc4+ymDPJkzihdteTp22NV69LEA==","X-Google-Smtp-Source":"AK7set+tZR87k2S4GjrujsjedurYcbKySQymGiYfKOmKSmErfan/vhPD6tjvyGumpf11m+e6OGbeoWE9TPuo9Rskhfo=","X-Received":"by 2002:a17:907:98d0:b0:920:da8c:f7b0 with SMTP id\n\tkd16-20020a17090798d000b00920da8cf7b0mr3025195ejc.6.1679484699812;\n\tWed, 22 Mar 2023 04:31:39 -0700 (PDT)","MIME-Version":"1.0","References":"<20230314144834.85193-1-dse@thaumatec.com>\n\t<20230314144834.85193-5-dse@thaumatec.com>\n\t<20230321144607.a7jri74sxcidrgha@uno.localdomain>\n\t","In-Reply-To":"","Date":"Wed, 22 Mar 2023 12:31:28 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [PATCH v4 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\" "}}]