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

16599 diff mbox series

Define common interface with basic functions that should be supported
by Auto Focus algorithms.

Signed-off-by: Daniel Semkowicz <dse@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

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@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
>

Hi Daniel,
  one more question

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@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 */

Any particular reason to go for BSD ?

> +/*
> + * Copyright (C) 2022, Raspberry Pi (Trading) Limited
> + * Copyright (C) 2022, Theobroma Systems
> + *
> + * 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>
> +{
> +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;
> +};
> +
> +} /* 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
>

Hi Jacopo,

On Thu, Jul 14, 2022 at 6:07 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> 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@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 */
>
> Any particular reason to go for BSD ?
>
>
> > +/*
> > + * Copyright (C) 2022, Raspberry Pi (Trading) Limited
> > + * Copyright (C) 2022, Theobroma Systems
>
> For this and the other new files, is this your copyright ?
>

My copyright is Theobroma Systems. I added Raspberry Pi as I was basing
partially on their work and their code was published as BSD-2. It was
hard for me to decide if it was derived work or amount of changes made
it completely different, so I assumed the first one to be safe. If you
think this is independent change then I am ok with LGPL as I see the most
part of the library use it.


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

I did this to avoid the multiple inheritance. It is more clear if there
is only one direct path for interface and implementation. Unfortunately,
the drawback here is that We need to pass the Module argument from top
to bottom. It is the choice between scenarios that both have some
drawbacks. I would love to hear additional opinions :)
If We go for the multiple inheritance approach, then I think it would be
good to change naming a bit, as it would be a bit misleading now to have
AfAlgorithm and Algorithm inheritance. Both sound as almost the same
thing.

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

I also had this in mind and I totally agree with that. I just wanted to
push the minimal working code as it can be later extended. However, We
can already put it in the interface and add implementation later.

Best regards
Daniel

>
> > +};
> > +
> > +} /* 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
> >