[libcamera-devel] [PATCH v2 01/11] ipa: Add base class defining AF algorithm interface

[Jacopo Mondi]

Hi Daniel,
  I like -very much- how you have split the algorithm in the platform
dependent part and in the generic part! very nice

On Wed, Jul 13, 2022 at 10:43:07AM +0200, Daniel Semkowicz via libcamera-devel wrote:
> Define common interface with basic functions that should be supported
> by Auto Focus algorithms.
>
> Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> ---
>  src/ipa/libipa/algorithms/af_algorithm.cpp | 78 ++++++++++++++++++++++
>  src/ipa/libipa/algorithms/af_algorithm.h   | 41 ++++++++++++
>  src/ipa/libipa/algorithms/meson.build      |  9 +++
>  src/ipa/libipa/meson.build                 |  6 ++
>  4 files changed, 134 insertions(+)
>  create mode 100644 src/ipa/libipa/algorithms/af_algorithm.cpp
>  create mode 100644 src/ipa/libipa/algorithms/af_algorithm.h
>  create mode 100644 src/ipa/libipa/algorithms/meson.build
>
> diff --git a/src/ipa/libipa/algorithms/af_algorithm.cpp b/src/ipa/libipa/algorithms/af_algorithm.cpp
> new file mode 100644
> index 00000000..47e54d5a
> --- /dev/null
> +++ b/src/ipa/libipa/algorithms/af_algorithm.cpp
> @@ -0,0 +1,78 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2022, Raspberry Pi (Trading) Limited
> + * Copyright (C) 2022, Theobroma Systems

For this and the other new files, is this your copyright ?

> + *
> + * af_algorithm.cpp - Autofocus control algorithm interface
> + */
> +
> +#include "af_algorithm.h"
> +
> +/**
> + * \file af_algorithm.h
> + * \brief AF algorithm common interface
> + */
> +
> +namespace libcamera::ipa::common::algorithms {
> +
> +/**
> + * \class AfAlgorithm
> + * \brief Common interface for auto-focus algorithms
> + * \tparam Module The IPA module type for this class of algorithms
> + *
> + * The AfAlgorithm class defines a standard interface for IPA auto focus
> + * algorithms.
> + */
> +
> +/**
> + * \fn AfAlgorithm::setMode()
> + * \brief Set auto focus mode
> + * \param[in] mode AF mode
> + */
> +
> +/**
> + * \fn AfAlgorithm::setRange()
> + * \brief set the range of focus distances that is scanned
> + * \param[in] range AF range
> + */
> +
> +/**
> + * \fn AfAlgorithm::setSpeed()
> + * \brief Set how fast algorithm should move the lens
> + * \param[in] speed Lens move speed
> + */
> +
> +/**
> + * \fn AfAlgorithm::setMetering()
> + * \brief Set AF metering mode
> + * \param[in] metering AF metering mode
> + */
> +
> +/**
> + * \fn AfAlgorithm::setWindows()
> + * \brief Set AF windows
> + * \param[in] windows AF windows
> + *
> + * Sets the focus windows used by the AF algorithm when AfMetering is set
> + * to AfMeteringWindows.
> + */
> +
> +/**
> + * \fn AfAlgorithm::setTrigger()
> + * \brief Starts or cancels the autofocus scan
> + * \param[in] trigger Trigger mode
> + */
> +
> +/**
> + * \fn AfAlgorithm::setPause()
> + * \brief Pause the autofocus while in AfModeContinuous mode.
> + * \param[in] pause Pause mode
> + */
> +
> +/**
> + * \fn AfAlgorithm::setLensPosition()
> + * \brief Set the lens position while in AfModeManual
> + * \param[in] lensPosition Lens position
> + */
> +
> +} /* namespace libcamera::ipa::common::algorithms */
> diff --git a/src/ipa/libipa/algorithms/af_algorithm.h b/src/ipa/libipa/algorithms/af_algorithm.h
> new file mode 100644
> index 00000000..2862042b
> --- /dev/null
> +++ b/src/ipa/libipa/algorithms/af_algorithm.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2022, Raspberry Pi (Trading) Limited
> + * Copyright (C) 2022, Theobroma Systems
> + *
> + * af_algorithm.h - Autofocus control algorithm interface
> + */
> +#pragma once
> +
> +#include <libcamera/control_ids.h>
> +
> +#include "../algorithm.h"
> +
> +namespace libcamera::ipa::common::algorithms {
> +
> +template<typename Module>
> +class AfAlgorithm : public Algorithm<Module>

I wonder... do we need to bring Module up to this level ? We're
defining a pure virtual class here, so it's just an interface.

Same for AfHillClimbing, does it need <Module> there ? it's not
intended to be instantiated (it can't be if I'm not mistaken as it
doesn't define all the pure virtual methods of the base class).

I have applied  this patch, and made only the RkISP1 implementation
inherit from Algorithm.

--- a/src/ipa/libipa/algorithms/af_algorithm.h
+++ b/src/ipa/libipa/algorithms/af_algorithm.h
@@ -13,8 +13,7 @@

 namespace libcamera::ipa::common::algorithms {

-template<typename Module>
-class AfAlgorithm : public Algorithm<Module>
+class AfAlgorithm
 {
 public:
        AfAlgorithm() = default;
diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h
index e251f3eb6000..33e2348c0fdd 100644
--- a/src/ipa/libipa/algorithms/af_hill_climbing.h
+++ b/src/ipa/libipa/algorithms/af_hill_climbing.h
@@ -19,8 +19,7 @@ LOG_DECLARE_CATEGORY(Af)

 namespace ipa::common::algorithms {

-template<typename Module>
-class AfHillClimbing : public AfAlgorithm<Module>
+class AfHillClimbing : public AfAlgorithm
 {
 public:
        AfHillClimbing()
diff --git a/src/ipa/rkisp1/algorithms/af.h b/src/ipa/rkisp1/algorithms/af.h
index b2da48904229..d27b5da703fc 100644
--- a/src/ipa/rkisp1/algorithms/af.h
+++ b/src/ipa/rkisp1/algorithms/af.h
@@ -10,11 +10,11 @@
 #include <linux/rkisp1-config.h>

 #include "libipa/algorithms/af_hill_climbing.h"
-#include "module.h"
+#include "algorithm.h"

 namespace libcamera::ipa::rkisp1::algorithms {

-class Af : public ipa::common::algorithms::AfHillClimbing<Module>
+class Af : public ipa::common::algorithms::AfHillClimbing, public Algorithm
 {
 public:
        Af() = default;

Isn't it better to keep the Module template argument in the bottom of
the hierarchy ?


> +{
> +public:
> +	AfAlgorithm() = default;
> +
> +	virtual ~AfAlgorithm() {}
> +
> +	virtual void setMode(controls::AfModeEnum mode) = 0;
> +
> +	virtual void setRange(controls::AfRangeEnum range) = 0;
> +
> +	virtual void setSpeed(controls::AfSpeedEnum speed) = 0;
> +
> +	virtual void setMetering(controls::AfMeteringEnum metering) = 0;
> +
> +	virtual void setWindows(Span<const Rectangle> windows) = 0;
> +
> +	virtual void setTrigger(controls::AfTriggerEnum trigger) = 0;
> +
> +	virtual void setPause(controls::AfPauseEnum pause) = 0;
> +
> +	virtual void setLensPosition(float lensPosition) = 0;

The interface mimics exactly the AF controls in the
application->libcamera direction, I wonder if we should expose from
the interface AfState and AfPauseState as well.

> +};
> +
> +} /* namespace libcamera::ipa::common::algorithms */
> diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build
> new file mode 100644
> index 00000000..ab8da13a
> --- /dev/null
> +++ b/src/ipa/libipa/algorithms/meson.build
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: CC0-1.0
> +
> +common_ipa_algorithms_headers = files([
> +    'af_algorithm.h',
> +])
> +
> +common_ipa_algorithms_sources = files([
> +    'af_algorithm.cpp',
> +])
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index fb894bc6..1fc3fd56 100644
> --- a/src/ipa/libipa/meson.build
> +++ b/src/ipa/libipa/meson.build
> @@ -1,5 +1,7 @@
>  # SPDX-License-Identifier: CC0-1.0
>
> +subdir('algorithms')
> +
>  libipa_headers = files([
>      'algorithm.h',
>      'camera_sensor_helper.h',
> @@ -7,6 +9,8 @@ libipa_headers = files([
>      'module.h',
>  ])
>
> +libipa_headers += common_ipa_algorithms_headers
> +
>  libipa_sources = files([
>      'algorithm.cpp',
>      'camera_sensor_helper.cpp',
> @@ -14,6 +18,8 @@ libipa_sources = files([
>      'module.cpp',
>  ])
>
> +libipa_sources += common_ipa_algorithms_sources
> +
>  libipa_includes = include_directories('..')
>
>  libipa = static_library('ipa', [libipa_sources, libipa_headers],
> --
> 2.34.1
>