[libcamera-devel] [PATCH v6 06/10] ipa: rkisp1: Add AF algorithm based on AfHillClimbing
Daniel Semkowicz
dse at thaumatec.com
Fri May 26 11:19:02 CEST 2023
Hi Laurent,
On Wed, Apr 26, 2023 at 6:04 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> On Wed, Apr 26, 2023 at 06:46:58AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > Hi Daniel,
> >
> > Thank you for the patch.
> >
> > On Fri, Mar 31, 2023 at 10:19:26AM +0200, Daniel Semkowicz via libcamera-devel wrote:
> > > Add contrast based auto focus implementation for rkisp1 platform using
> > > the common AfHillClimbing algorithm.
> > >
> > > Rockchip ISP AF block allows calculation of sharpness and luminance
> > > in up to three user defined windows. If no windows are set, there are
> > > some default settings applied for the first window and exposed through
> > > the driver. For each frame, use the sharpness value calculated for this
> > > default window and feed the hill climbing algorithm with them. Then set
> > > the lens position to value calculated by the algorithm.
> > >
> > > Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> > > ---
> > > src/ipa/rkisp1/algorithms/af.cpp | 121 ++++++++++++++++++++++++++
> > > src/ipa/rkisp1/algorithms/af.h | 43 +++++++++
> > > src/ipa/rkisp1/algorithms/meson.build | 1 +
> > > src/ipa/rkisp1/ipa_context.cpp | 13 +++
> > > src/ipa/rkisp1/ipa_context.h | 5 ++
> > > 5 files changed, 183 insertions(+)
> > > create mode 100644 src/ipa/rkisp1/algorithms/af.cpp
> > > create mode 100644 src/ipa/rkisp1/algorithms/af.h
> > >
> > > diff --git a/src/ipa/rkisp1/algorithms/af.cpp b/src/ipa/rkisp1/algorithms/af.cpp
> > > new file mode 100644
> > > index 00000000..fde924d4
> > > --- /dev/null
> > > +++ b/src/ipa/rkisp1/algorithms/af.cpp
> > > @@ -0,0 +1,121 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2023, Theobroma Systems
> > > + *
> > > + * af.cpp - RkISP1 AF hill climbing based control algorithm
> >
> > s/climbing based/climbing-based/
> >
> > > + */
> > > +
> > > +#include "af.h"
> > > +
> > > +/**
> > > + * \file af.h
> >
> > Missing \brief
> >
> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > > +namespace ipa::rkisp1::algorithms {
> > > +
> > > +/**
> > > + * \class Af
> > > + * \brief AF contrast based hill climbing control algorithm for RkISP platforms
> >
> > s/contrast based/contrast-based/
> >
> > > + *
> > > + * Auto focus algorithm for RkISP platforms, based on the common hill climbing
> > > + * auto focus implementation (libcamera::ipa::algorithms::AfHillClimbing).
> > > + *
> > > + * This is the platform layer of the algorithm.
> > > + *
> > > + * Tuning file parameters:
> > > + * - **wait-frames-lens:** Number of frames that should be skipped when lens
> >
> > rkisp1 tuning files use camelCase for parameter names.
I will correct it.
> >
> > > + * position is changed. Lens movement takes some time and statistics measured
> > > + * during the lens movement are unstable. Currently there is no way to know
> > > + * when lens movement finished and this is a workaround for this. Wait a fixed
> > > + * amount of time on each movement. This parameter should be set according
> > > + * to the worst case - the number of frames it takes to move lens between
> > > + * limit positions.
> > > + * .
> >
> > Stray period ?
The same as in the 04/10, dot is used to explicitly mark the end of
the list in the Doxygen.
> >
> > > + * \sa libcamera::ipa::algorithms::AfHillClimbing for additional tuning
> > > + * parameters.
> > > + *
> > > + * \todo Model the lens delay as number of frames required for the lens position
> > > + * to stabilize in the CameraLens class.
> >
> > More than that, we should take the lens movement into account in the
> > algorithm to avoid skipping frames at all.
Yes, I agree. We should only base on the information if the lens
movement has finished instead of skipping frames in the AF. I will
improve that note.
> >
> > > + */
> > > +
> > > +LOG_DEFINE_CATEGORY(RkISP1Af)
> > > +
> > > +/**
> > > + * \copydoc libcamera::ipa::Algorithm::init
> > > + */
> > > +int Af::init(IPAContext &context, const YamlObject &tuningData)
> > > +{
> > > + const auto &lensConfig = context.configuration.lens;
> > > + if (!lensConfig) {
> > > + LOG(RkISP1Af, Error) << "Lens not found";
> > > + return 1;
> > > + }
> > > +
> > > + waitFramesLens_ = tuningData["wait-frames-lens"].get<uint32_t>(1);
> > > +
> > > + LOG(RkISP1Af, Debug) << "waitFramesLens_: " << waitFramesLens_;
> > > +
> > > + return af.init(lensConfig->minFocusPosition,
> > > + lensConfig->maxFocusPosition, tuningData);
> > > +}
> > > +
> > > +/**
> > > + * \copydoc libcamera::ipa::Algorithm::configure
> > > + */
> > > +int Af::configure(IPAContext &context,
> > > + const IPACameraSensorInfo &configInfo)
> > > +{
> > > + /*
> > > + * Lens position is unknown at the startup, so initialize
> > > + * the current position to something out of range.
> > > + */
> > > + context.activeState.af.lensPosition =
> > > + context.configuration.lens->maxFocusPosition + 1;
> > > +
> > > + af.configure(configInfo.outputSize);
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * \copydoc libcamera::ipa::Algorithm::queueRequest
> > > + */
> > > +void Af::queueRequest([[maybe_unused]] IPAContext &context,
> > > + [[maybe_unused]] const uint32_t frame,
> > > + [[maybe_unused]] IPAFrameContext &frameContext,
> > > + const ControlList &controls)
> > > +{
> > > + af.queueRequest(controls);
> > > +}
> > > +
> > > +/**
> > > + * \copydoc libcamera::ipa::Algorithm::process
> > > + */
> > > +void Af::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > + [[maybe_unused]] IPAFrameContext &frameContext,
> > > + const rkisp1_stat_buffer *stats,
> > > + [[maybe_unused]] ControlList &metadata)
> > > +{
> > > + uint32_t sharpness = stats->params.af.window[0].sum;
> > > + uint32_t luminance = stats->params.af.window[0].lum;
> > > +
> > > + LOG(RkISP1Af, Debug)
> > > + << "lensPosition: " << context.activeState.af.lensPosition
> > > + << ", Sharpness: " << sharpness
> >
> > Maybe contrast, as everything in this series talks about contrast-based
> > AF ?
Log category for this debug message is "RkISP1Af". Isn't it easier
for debugging, if We stick with the rkisp naming in the platform related
code?
> >
> > > + << ", Luminance: " << luminance;
> >
> > The luminance isn't used, so you could drop it.
>
> Or better, you should use it :-) The luminance is meant to normalize the
> sharpness values.
Can We introduce the normalization later as an improvement?
>
> > > +
> > > + int32_t newLensPosition = af.process(sharpness);
> > > +
> > > + if (newLensPosition != context.activeState.af.lensPosition) {
> > > + context.activeState.af.lensPosition = newLensPosition;
> > > + context.activeState.af.applyLensCtrls = true;
> > > + af.skipFrames(waitFramesLens_);
> >
> > I'm tempted to implement frame skipping in this class instead of the
> > ipa::algorithms::AfHillClimbing class. It would be simpler, we could
> > just skip calling process(). On the other hand, I suppose
> > ipa::algorithms::AfHillClimbing will need to change when we'll improve
> > the mechanism that takes lens movement into account, and it's hard to
> > predict how the interface will change.
> >
So maybe it would be simpler to use the current approach and improve
it later when needed, when the interface become clear.
> > > + }
> > > +}
> >
> > The IPA module should also report the lens position in metadata.
> >
> > > +
> > > +REGISTER_IPA_ALGORITHM(Af, "Af")
> > > +
> > > +} /* namespace ipa::rkisp1::algorithms */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/ipa/rkisp1/algorithms/af.h b/src/ipa/rkisp1/algorithms/af.h
> > > new file mode 100644
> > > index 00000000..3ba66d38
> > > --- /dev/null
> > > +++ b/src/ipa/rkisp1/algorithms/af.h
> > > @@ -0,0 +1,43 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2023, Theobroma Systems
> > > + *
> > > + * af.h - RkISP1 AF hill climbing based control algorithm
> >
> > s/climbing based/climbing-based/
> >
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include <linux/rkisp1-config.h>
> > > +
> > > +#include "libipa/algorithms/af_hill_climbing.h"
> > > +
> > > +#include "algorithm.h"
> > > +
> > > +namespace libcamera {
> > > +
> > > +namespace ipa::rkisp1::algorithms {
> > > +
> > > +class Af : public Algorithm
> > > +{
> > > +public:
> > > + int init(IPAContext &context, const YamlObject &tuningData) override;
> > > + int configure(IPAContext &context,
> > > + const IPACameraSensorInfo &configInfo) override;
> > > + void queueRequest(IPAContext &context, uint32_t frame,
> > > + IPAFrameContext &frameContext,
> > > + const ControlList &controls) override;
> > > + void process(IPAContext &context, uint32_t frame,
> > > + IPAFrameContext &frameContext,
> > > + const rkisp1_stat_buffer *stats,
> > > + ControlList &metadata) override;
> > > +
> > > +private:
> > > + ipa::algorithms::AfHillClimbing af;
> >
> > s/af/af_/
> >
> > > +
> > > + /* Wait number of frames after changing lens position */
> > > + uint32_t waitFramesLens_ = 0;
> > > +};
> > > +
> > > +} /* namespace ipa::rkisp1::algorithms */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> > > index 93a48329..ab7e44f3 100644
> > > --- a/src/ipa/rkisp1/algorithms/meson.build
> > > +++ b/src/ipa/rkisp1/algorithms/meson.build
> > > @@ -1,6 +1,7 @@
> > > # SPDX-License-Identifier: CC0-1.0
> > >
> > > rkisp1_ipa_algorithms = files([
> > > + 'af.cpp',
> > > 'agc.cpp',
> > > 'awb.cpp',
> > > 'blc.cpp',
> > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > > index aea99299..d80a7b1b 100644
> > > --- a/src/ipa/rkisp1/ipa_context.cpp
> > > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > > @@ -139,6 +139,19 @@ namespace libcamera::ipa::rkisp1 {
> > > * member may be read by any algorithm, but shall only be written by its owner.
> > > */
> > >
> > > +/**
> > > + * \var IPAActiveState::af
> > > + * \brief State for the Automatic Focus Control algorithm
> > > + *
> > > + * \var IPAActiveState::af.lensPosition
> > > + * \brief Lens position calculated by the AF algorithm
> > > + *
> > > + * \var IPAActiveState::af.applyLensCtrls
> > > + * \brief Whether the lens position should be applied
> > > + *
> > > + * If true, IPA should send new controls to the PH to set new lens position.
> > > + */
> > > +
> > > /**
> > > * \var IPAActiveState::agc
> > > * \brief State for the Automatic Gain Control algorithm
> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > index 65b3fbfe..3dcc5aa0 100644
> > > --- a/src/ipa/rkisp1/ipa_context.h
> > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > @@ -62,6 +62,11 @@ struct IPASessionConfiguration {
> > > };
> > >
> > > struct IPAActiveState {
> > > + struct {
> > > + int32_t lensPosition;
> > > + bool applyLensCtrls;
> >
> > I suppose applyLens will be used later in the series.
Should I move it to the later patches?
> >
> > > + } af;
> > > +
> > > struct {
> > > struct {
> > > uint32_t exposure;
>
> --
> Regards,
>
> Laurent Pinchart
Best regards
Daniel
More information about the libcamera-devel
mailing list