[libcamera-devel] [PATCH v6 06/10] ipa: rkisp1: Add AF algorithm based on AfHillClimbing

[Laurent Pinchart]

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.

> + *   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 ?

> + * \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.

> + */
> +
> +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 ?

> +		<< ", Luminance: " << luminance;

The luminance isn't used, so you could drop it.

> +
> +	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.

> +	}
> +}

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.

> +	} af;
> +
>  	struct {
>  		struct {
>  			uint32_t exposure;

-- 
Regards,

Laurent Pinchart