[libcamera-devel] [PATCH v4 04/10] ipa: Add common contrast based AF implementation
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Mar 21 09:15:42 CET 2023
Hi Daniel
On Tue, Mar 21, 2023 at 08:38:15AM +0100, Daniel Semkowicz wrote:
> Hi Jacopo,
>
> On Mon, Mar 20, 2023 at 9:46 PM Jacopo Mondi
> <jacopo.mondi at ideasonboard.com> wrote:
> >
> > Hi Daniel
> >
> > On Tue, Mar 14, 2023 at 03:48:28PM +0100, Daniel Semkowicz wrote:
> > > Create a new class with contrast based Auto Focus implementation
> > > using hill climbing algorithm. This common implementation is independent
> > > of platform specific code. This way, each platform can just implement
> > > contrast calculation and run the AF control loop basing on this class.
> >
> > Nice!
> >
> > > This implementation is based on the code that was common for IPU3
> > > and RPi AF algorithms.
> > >
> > > Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> > > ---
> > > .../libipa/algorithms/af_hill_climbing.cpp | 358 ++++++++++++++++++
> > > src/ipa/libipa/algorithms/af_hill_climbing.h | 93 +++++
> > > src/ipa/libipa/algorithms/meson.build | 2 +
> > > 3 files changed, 453 insertions(+)
> > > create mode 100644 src/ipa/libipa/algorithms/af_hill_climbing.cpp
> > > create mode 100644 src/ipa/libipa/algorithms/af_hill_climbing.h
> > >
> > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.cpp b/src/ipa/libipa/algorithms/af_hill_climbing.cpp
> > > new file mode 100644
> > > index 00000000..2b99afef
> > > --- /dev/null
> > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp
> > > @@ -0,0 +1,358 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Red Hat
> > > + * Copyright (C) 2022, Ideas On Board
> > > + * Copyright (C) 2023, Theobroma Systems
> > > + *
> > > + * af_hill_climbing.cpp - AF contrast based hill climbing common algorithm
> > > + */
> > > +
> > > +#include "af_hill_climbing.h"
> > > +
> > > +#include "libcamera/internal/yaml_parser.h"
> > > +
> > > +/**
> > > + * \file af_hill_climbing.h
> > > + * \brief AF contrast based hill climbing common algorithm
> > > + */
> > > +
> > > +namespace libcamera::ipa::common::algorithms {
> > > +
> > > +LOG_DEFINE_CATEGORY(Af)
> > > +
> > > +/**
> > > + * \class AfHillClimbing
> > > + * \brief Contrast based hill climbing auto focus control algorithm
> > > + * implementation
> > > + *
> > > + * Control part of the auto focus algorithm. It calculates the lens position
> > > + * basing on the contrast measure supplied by the higher level. This way it is
> >
> > s/basing/based ?
> >
> > "higher level" means the platform-specific implementation ?
> >
> > > + * independent from the platform.
> > > + *
> > > + * Platform layer that use this algorithm should call process() method
> >
> > s/method/function
> >
> > > + * for each measured contrast value and set the lens to the calculated position.
> >
> > Is this meant to be called for each frame ?
>
> Yes it should, because there is a counter of skipped frames. I will extend this
> part accordingly.
>
> >
> > > + *
> > > + * Focus search is divided into two phases:
> > > + * 1. coarse search,
> > > + * 2. fine search.
> > > + *
> > > + * In the first phase, lens are moved with bigger steps to quickly find a rough
> >
> > "the lens is moved"
> >
> > > + * position for the best focus. Then, basing on the outcome of coarse search,
> >
> > "based" again, unelss "basing" it's correct and I'm getting it wrong
> >
> > > + * the second phase is executed. Lens are moved with smaller steps in a limited
> >
> > "Lens is"
>
> Thanks for the hints. English is not my native language, so feel free
> to point out all language errors. This will help me learn it better :)
>
Neither is mine, so take all suggestions with a grain of salt
> >
> > > + * range within the rough position to find the exact position for best focus.
> > > + *
> > > + * Tuning file parameters:
> > > + * - **coarse-search-step:** The value by which the position will change in one
> > > + * step in the *coarse search* phase.
Can you s/"the position will"/"the lens position will"
here and below ?
> > > + * - **fine-search-step:** The value by which the position will change in one
> > > + * step in the *fine search* phase.
> >
> > I presume these values are expressed in lens specific units ?
>
> Yes, you are correct, I guess this also needs to be more clear :D
>
> >
> > > + * - **fine-search-range:** Search range in the *fine search* phase.
> > > + * Valid values are in the [0.0, 1.0] interval. Value 0.05 means 5%. If coarse
> > > + * search stopped at value 200, the fine search range will be [190, 210]
> >
> > It's nice to have this in percentage, I wonder if it is correct to
> > calculate the percentage on the coarse search result and not on the
> > lens movement range.
> >
> > In your example, a 5% search interval for a coarse search resul of 200
> > will differ from a result of 400, while I presume the search range
> > should be constant for the lens movement range ?
>
> This is a part that was moved from the original IPU3 implementation.
> Unfortunately, there was no documentation about the decision. If I needed
> to guess, the reason for that would be the fact, that lens focus change is not
> linear in reference to the position value. On the other hand, we cannot assure
> that all lens will need smaller steps on the lower part of the range and not
> the other way around...
>
Exactly, it is rather hard to make any kind of assumptions...
Could you add a \todo in the documentation that reports that the
search range percentage should be made dependendent on the lens
movement range and not on the coarse value ?
> >
> > > + * - **max-variance-change:** ratio of contrast variance change in the
> > > + * *continuous mode* needed for triggering the focus change. When the variance
> > > + * change exceeds this value, focus search will be triggered. Valid values are
> > > + * in the [0.0, 1.0] interval.
> > > + */
> > > +
> > > +/**
> > > + * \brief Initialize the AfHillClimbing with tuning data
> > > + * \param[in] tuningData The tuning data for the algorithm
> > > + *
> > > + * This method should be called in the libcamera::ipa::Algorithm::init()
> > > + * method of the platform layer.
> > > + *
> > > + * \return 0 if successful, an error code otherwise
> > > + */
> > > +int AfHillClimbing::init(const YamlObject &tuningData)
> > > +{
> > > + coarseSearchStep_ = tuningData["coarse-search-step"].get<uint32_t>(30);
> > > + fineSearchStep_ = tuningData["fine-search-step"].get<uint32_t>(1);
> > > + fineRange_ = tuningData["fine-search-range"].get<double>(0.05);
> > > + maxChange_ = tuningData["max-variance-change"].get<double>(0.5);
> > > +
> > > + LOG(Af, Debug) << "coarseSearchStep_: " << coarseSearchStep_
> > > + << ", fineSearchStep_: " << fineSearchStep_
> > > + << ", fineRange_: " << fineRange_
> > > + << ", maxChange_: " << maxChange_;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * \brief Configure the AfHillClimbing with sensor and lens information
> > > + * \param[in] minFocusPosition Minimum position supported by camera lens
> > > + * \param[in] maxFocusPosition Maximum position supported by camera lens
> > > + *
> > > + * This method should be called in the libcamera::ipa::Algorithm::configure()
> > > + * method of the platform layer.
> > > + */
> > > +void AfHillClimbing::configure(int32_t minFocusPosition,
> > > + int32_t maxFocusPosition)
> > > +{
> > > + minLensPosition_ = minFocusPosition;
> > > + maxLensPosition_ = maxFocusPosition;
> > > +}
> > > +
> > > +/**
> > > + * \brief Run the auto focus algorithm loop
> > > + * \param[in] currentContrast New value of contrast measured for current frame
> >
> > What is the intended unit for the contrast ?
>
> Arbitrary. The only assumption is that the higher value means the image
> is more sharp, more in focus.
>
Should we document such assumption ?
> >
> > > + *
> > > + * This method should be called in the libcamera::ipa::Algorithm::process()
> > > + * method of the platform layer for each new contrast value that was measured.
> > > + *
> > > + * \return New lens position calculated by AF algorithm
> > > + */
> > > +int32_t AfHillClimbing::process(double currentContrast)
> > > +{
> > > + currentContrast_ = currentContrast;
> > > +
> > > + if (shouldSkipFrame())
> > > + return lensPosition_;
> > > +
> > > + switch (mode_) {
> > > + case controls::AfModeManual:
> > > + /* Nothing to process. */
> > > + break;
> > > + case controls::AfModeAuto:
> > > + processAutoMode();
> > > + break;
> > > + case controls::AfModeContinuous:
> > > + processContinousMode();
> > > + break;
> > > + default:
> > > + break;
> >
> > Do we need a default case ?
>
> In this case it can be removed.
>
> >
> > > + }
> > > +
> > > + return lensPosition_;
> > > +}
> > > +
> > > +void AfHillClimbing::processAutoMode()
> > > +{
> > > + if (state_ == controls::AfStateScanning) {
> > > + afCoarseScan();
> > > + afFineScan();
> > > + }
> > > +}
> > > +
> > > +void AfHillClimbing::processContinousMode()
> > > +{
> > > + /* If we are in a paused state, we won't process the stats */
> > > + if (pauseState_ == controls::AfPauseStatePaused)
> > > + return;
> > > +
> > > + if (state_ == controls::AfStateScanning) {
> > > + afCoarseScan();
> > > + afFineScan();
> > > + return;
> > > + }
> > > +
> > > + /* We can re-start the scan at any moment in AfModeContinuous */
> > > + if (afIsOutOfFocus()) {
> > > + afReset();
> > > + }
> >
> > No need for brace.
> >
> > I now see what you meant when you said CAF needs to move the lens back
> > to its initial position..
> >
> > > +}
> > > +
> > > +/**
> > > + * \brief Request AF to skip n frames
> > > + * \param[in] n Number of frames to be skipped
> > > + *
> > > + * Requested number of frames will not be used for AF calculation.
> >
> > When is this function meant to be called by the platform layer ?
>
> Depends on the platform. For RkISP it is used only to force one frame
> wait to configure the AF window.
>
> >
> > Also "reset()" resets this value too, which when working in Auto mode
> > (which requires Triggers to work) will make this value overwritten by
> > the trigger. Is this desired ?
>
> This value will be overwritten only if the higher number of frames that
> is already set was requested. So it should be safe to use.
>
> >
> >
> > > + */
> > > +void AfHillClimbing::setFramesToSkip(uint32_t n)
> > > +{
> > > + if (n > framesToSkip_)
> > > + framesToSkip_ = n;
> > > +}
> > > +
> > > +void AfHillClimbing::setMode(controls::AfModeEnum mode)
> > > +{
> > > + if (mode == mode_)
> > > + return;
> > > +
> > > + LOG(Af, Debug) << "Switched AF mode from " << mode_ << " to " << mode;
> > > + mode_ = mode;
> > > +
> > > + state_ = controls::AfStateIdle;
> > > + pauseState_ = controls::AfPauseStateRunning;
> > > +
> > > + if (mode_ == controls::AfModeContinuous)
> > > + afReset();
> > > +}
> > > +
> > > +void AfHillClimbing::setRange([[maybe_unused]] controls::AfRangeEnum range)
> > > +{
> > > + LOG(Af, Error) << __FUNCTION__ << " not implemented!";
> > > +}
> > > +
> > > +void AfHillClimbing::setSpeed([[maybe_unused]] controls::AfSpeedEnum speed)
> > > +{
> > > + LOG(Af, Error) << __FUNCTION__ << " not implemented!";
> > > +}
> > > +
> > > +void AfHillClimbing::setMeteringMode([[maybe_unused]] controls::AfMeteringEnum metering)
> > > +{
> > > + LOG(Af, Error) << __FUNCTION__ << " not implemented!";
> > > +}
> > > +
> > > +void AfHillClimbing::setWindows([[maybe_unused]] Span<const Rectangle> windows)
> > > +{
> > > + LOG(Af, Error) << __FUNCTION__ << " not implemented!";
> > > +}
> >
> > I wonder if Error is correct here..
>
> I would expect that supported controls are correctly configured in the
> IPA, so it should not be possible to call the unsupported controls.
> In such scenario, entering this function means that something
> is misconfigured and this is an error.
>
> What I would really like to see in the libcamera, is that each
> algorithm expose its own controls during initialisation instead
> of having a hardcoded table of supported controls.
>
> >
> > > +
> > > +void AfHillClimbing::setTrigger(controls::AfTriggerEnum trigger)
> > > +{
> > > + if (mode_ != controls::AfModeAuto) {
> > > + LOG(Af, Warning)
> > > + << __FUNCTION__ << " not possible in mode " << mode_;
> > > + return;
> > > + }
> > > +
> > > + LOG(Af, Debug) << "Trigger called with " << trigger;
> > > +
> > > + if (trigger == controls::AfTriggerStart)
> > > + afReset();
> > > + else
> > > + state_ = controls::AfStateIdle;
> > > +}
> > > +
> > > +void AfHillClimbing::setPause(controls::AfPauseEnum pause)
> > > +{
> > > + if (mode_ != controls::AfModeContinuous) {
> > > + LOG(Af, Warning)
> > > + << __FUNCTION__ << " not possible in mode " << mode_;
> >
> > s/not possible/not valid
> >
> > here and below
> >
> > > + return;
> > > + }
> > > +
> > > + switch (pause) {
> > > + case controls::AfPauseImmediate:
> > > + pauseState_ = controls::AfPauseStatePaused;
> > > + break;
> > > + case controls::AfPauseDeferred:
> > > + /* \todo: add the AfPauseDeferred mode */
> > > + LOG(Af, Warning) << "AfPauseDeferred is not supported!";
> > > + break;
> > > + case controls::AfPauseResume:
> > > + pauseState_ = controls::AfPauseStateRunning;
> > > + break;
> > > + default:
> > > + break;
> >
> > Again I wonder if default is a good idea, as if the controls get
> > augmented I would prefer the compiler to complain if not all cases are
> > handled..
>
> I totally agree. I will remove the "default" branch.
>
> >
> > > + }
> > > +}
> > > +
> > > +void AfHillClimbing::setLensPosition(float lensPosition)
> > > +{
> > > + if (mode_ != controls::AfModeManual) {
> > > + LOG(Af, Warning)
> > > + << __FUNCTION__ << " not possible in mode " << mode_;
> > > + return;
> > > + }
> > > +
> > > + lensPosition_ = lensPosition;
> > > +
> > > + LOG(Af, Debug) << "Requesting lens position " << lensPosition_;
> > > +}
> > > +
> > > +void AfHillClimbing::afCoarseScan()
> > > +{
> > > + if (coarseCompleted_)
> > > + return;
> > > +
> > > + if (afScan(coarseSearchStep_)) {
> > > + coarseCompleted_ = true;
> > > + maxContrast_ = 0;
> > > + int32_t diff = std::abs(lensPosition_) * fineRange_;
> > > + lensPosition_ = std::max(lensPosition_ - diff, minLensPosition_);
> > > + maxStep_ = std::min(lensPosition_ + diff, maxLensPosition_);
> > > + previousContrast_ = 0;
> > > + }
> > > +}
> > > +
> > > +void AfHillClimbing::afFineScan()
> > > +{
> > > + if (!coarseCompleted_)
> > > + return;
> > > +
> > > + if (afScan(fineSearchStep_)) {
> > > + LOG(Af, Debug) << "AF found the best focus position!";
> > > + state_ = controls::AfStateFocused;
> > > + fineCompleted_ = true;
> > > + }
> > > +}
> > > +
> > > +bool AfHillClimbing::afScan(uint32_t steps)
> > > +{
> > > + if (lensPosition_ + static_cast<int32_t>(steps) > maxStep_) {
> > > + /* If the max step is reached, move lens to the position. */
> > > + lensPosition_ = bestPosition_;
> > > + return true;
> > > + } else {
> > > + /*
> > > + * Find the maximum of the variance by estimating its
> > > + * derivative. If the direction changes, it means we have passed
> > > + * a maximum one step before.
> > > + */
> > > + if ((currentContrast_ - maxContrast_) >= -(maxContrast_ * 0.1)) {
> > > + /*
> > > + * Positive and zero derivative:
> > > + * The variance is still increasing. The focus could be
> > > + * increased for the next comparison. Also, the max
> > > + * variance and previous focus value are updated.
> > > + */
> > > + bestPosition_ = lensPosition_;
> > > + lensPosition_ += steps;
> > > + maxContrast_ = currentContrast_;
> > > + } else {
> > > + /*
> > > + * Negative derivative:
> > > + * The variance starts to decrease which means the maximum
> > > + * variance is found. Set focus step to previous good one
> > > + * then return immediately.
> > > + */
> > > + lensPosition_ = bestPosition_;
> > > + return true;
> > > + }
> > > + }
> > > +
> > > + previousContrast_ = currentContrast_;
> > > + LOG(Af, Debug) << "Previous step is " << bestPosition_
> > > + << ", Current step is " << lensPosition_;
> > > + return false;
> > > +}
> > > +
> > > +void AfHillClimbing::afReset()
> > > +{
> > > + LOG(Af, Debug) << "Reset AF parameters";
> > > + lensPosition_ = minLensPosition_;
> > > + maxStep_ = maxLensPosition_;
> > > + state_ = controls::AfStateScanning;
> > > + previousContrast_ = 0.0;
> > > + coarseCompleted_ = false;
> > > + fineCompleted_ = false;
> > > + maxContrast_ = 0.0;
> > > + setFramesToSkip(1);
> > > +}
> > > +
> > > +bool AfHillClimbing::afIsOutOfFocus()
> > > +{
> > > + const uint32_t diff_var = std::abs(currentContrast_ - maxContrast_);
> > > + const double var_ratio = diff_var / maxContrast_;
> > > + LOG(Af, Debug) << "Variance change rate: " << var_ratio
> > > + << ", Current lens step: " << lensPosition_;
> > > + if (var_ratio > maxChange_)
> > > + return true;
> > > + else
> > > + return false;
> > > +}
> >
> > I presume the five functions above come from the existing
> > implementation and I'm not really reviewing them :)
>
> This is true. I made some minor changes to them, mostly a code
> formatting.
>
> >
> > > +
> > > +bool AfHillClimbing::shouldSkipFrame()
> > > +{
> > > + if (framesToSkip_ > 0) {
> > > + framesToSkip_--;
> > > + return true;
> > > + }
> > > +
> > > + return false;
> > > +}
> > > +
> > > +} /* namespace libcamera::ipa::common::algorithms */
> > > diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h
> > > new file mode 100644
> > > index 00000000..b361e5a1
> > > --- /dev/null
> > > +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h
> > > @@ -0,0 +1,93 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Red Hat
> > > + * Copyright (C) 2022, Ideas On Board
> > > + * Copyright (C) 2023, Theobroma Systems
> > > + *
> > > + * af_hill_climbing.h - AF contrast based hill climbing common algorithm
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include <libcamera/base/log.h>
> > > +
> > > +#include "af.h"
> > > +
> > > +namespace libcamera {
> > > +
> > > +class YamlObject;
> > > +
> > > +namespace ipa::common::algorithms {
> > > +
> > > +LOG_DECLARE_CATEGORY(Af)
> > > +
> > > +class AfHillClimbing : public Af
> > > +{
> > > +public:
> > > + int init(const YamlObject &tuningData);
> > > + void configure(int32_t minFocusPosition, int32_t maxFocusPosition);
> > > + int32_t process(double currentContrast);
> > > + void setFramesToSkip(uint32_t n);
> > > +
> > > + controls::AfStateEnum state() final { return state_; }
> > > + controls::AfPauseStateEnum pauseState() final { return pauseState_; }
> > > +
> > > +private:
> > > + void setMode(controls::AfModeEnum mode) final;
> > > + void setRange(controls::AfRangeEnum range) final;
> > > + void setSpeed(controls::AfSpeedEnum speed) final;
> > > + void setMeteringMode(controls::AfMeteringEnum metering) final;
> > > + void setWindows(Span<const Rectangle> windows) final;
> > > + void setTrigger(controls::AfTriggerEnum trigger) final;
> > > + void setPause(controls::AfPauseEnum pause) final;
> > > + void setLensPosition(float lensPosition) final;
> > > +
> > > + void processAutoMode();
> > > + void processContinousMode();
> > > + void afCoarseScan();
> > > + void afFineScan();
> > > + bool afScan(uint32_t steps);
> > > + void afReset();
> > > + bool afIsOutOfFocus();
> > > + bool shouldSkipFrame();
> > > +
> > > + controls::AfModeEnum mode_ = controls::AfModeManual;
> > > + controls::AfStateEnum state_ = controls::AfStateIdle;
> > > + controls::AfPauseStateEnum pauseState_ = controls::AfPauseStateRunning;
> > > +
> > > + /* VCM step configuration. It is the current setting of the VCM step. */
> > > + int32_t lensPosition_ = 0;
> > > + /* The best VCM step. It is a local optimum VCM step during scanning. */
> > > + int32_t bestPosition_ = 0;
> >
> > Do you mean "step" or "position" in the comments ?
>
> Yes, the "position" is more clear.
>
> >
> > > +
> > > + /* Current AF statistic contrast. */
> > > + double currentContrast_ = 0;
> > > + /* It is used to determine the derivative during scanning */
> > > + double previousContrast_ = 0;
> > > + double maxContrast_ = 0;
> > > + /* The designated maximum range of focus scanning. */
> > > + int32_t maxStep_ = 0;
> > > + /* If the coarse scan completes, it is set to true. */
> > > + bool coarseCompleted_ = false;
> > > + /* If the fine scan completes, it is set to true. */
> > > + bool fineCompleted_ = false;
> > > +
> > > + uint32_t framesToSkip_ = 0;
> > > +
> > > + /* Focus steps range of the lens */
> > > + int32_t minLensPosition_;
> > > + int32_t maxLensPosition_;
> > > +
> > > + /* Minimum focus step for searching appropriate focus */
> > > + uint32_t coarseSearchStep_;
> > > + uint32_t fineSearchStep_;
> > > +
> > > + /* Fine scan range 0 < fineRange_ < 1 */
> > > + double fineRange_;
> > > +
> > > + /* Max ratio of variance change, 0.0 < maxChange_ < 1.0 */
> > > + double maxChange_;
> > > +};
> > > +
> > > +} /* namespace ipa::common::algorithms */
> > > +} /* namespace libcamera */
> > > diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build
> > > index 7602976c..fa4b9564 100644
> > > --- a/src/ipa/libipa/algorithms/meson.build
> > > +++ b/src/ipa/libipa/algorithms/meson.build
> > > @@ -2,8 +2,10 @@
> > >
> > > common_ipa_algorithms_headers = files([
> > > 'af.h',
> > > + 'af_hill_climbing.h',
> > > ])
> > >
> > > common_ipa_algorithms_sources = files([
> > > 'af.cpp',
> > > + 'af_hill_climbing.cpp',
> > > ])
> > > --
> > > 2.39.2
> > >
>
> Best regards
> Daniel
More information about the libcamera-devel
mailing list