[libcamera-devel] [PATCH v6 06/10] ipa: rkisp1: Add AF algorithm based on AfHillClimbing
Daniel Semkowicz
dse at thaumatec.com
Wed Jun 14 07:39:14 CEST 2023
Hi Laurent,
On Wed, May 31, 2023 at 7:53 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Daniel,
>
> On Fri, May 26, 2023 at 11:19:02AM +0200, Daniel Semkowicz wrote:
> > On Wed, Apr 26, 2023 at 6:04 AM Laurent Pinchart wrote:
> > > On Wed, Apr 26, 2023 at 06:46:58AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > > 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?
>
> I was talking about "Sharpness", not the category :-) But the ISP
> documentation talks about sharpness, so I'm fine with that term.
>
> > > > > + << ", 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?
>
> I've explained in the review of the next patch how to handle the shift
> calculation. The normalization should be equally simple (in terms of
> code at least).
>
> The idea is that the sharpness is proportional to the square of the
> pixel values, so higher illumination will result in higher sharpness. To
> compensate for the illumination, you can simply divide the sharpness by
> the square of the mean illumination over the window:
>
> - Multiply the illumination computed by the hardware by the shift factor
> to make it independent of the window size.
> - Divide the resulting value by the window size in pixels, to obtain a
> mean illumination in the [0.0, 1.0] range.
> - Divide the sharpness by the square of the mean illumination. Make sure
> the mean illumination value is > 0 to avoid divisions by 0.
>
> It would be nice if you could do so already, as it should just be a few
> lines of code.
Ok, I will do some tests with that.
>
> > > > > +
> > > > > + 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.
>
> OK. Let's keep frame skipping where it is, but I'd like frame skipping
> to cover the lens movement only if possible, not the other delays. That
> may be lots of yak shaving though, so this could be done on top too.
> Would you agree to give it a try once this series gets merged ? :-)
I think the discussions in other patches in this series already impacted the
delays handling, so I will probably need to change it already in the next
version.
>
> > > > > + }
> > > > > +}
> > > >
> > > > 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?
>
> No, it's OK, it was a note for myself.
>
> > > >
> > > > > + } af;
> > > > > +
> > > > > struct {
> > > > > struct {
> > > > > uint32_t exposure;
>
> --
> Regards,
>
> Laurent Pinchart
Best regards
Daniel
More information about the libcamera-devel
mailing list