[libcamera-devel,v3,3/8] ipa: Add base class defining AF algorithm interface

18142 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_interface.cpp | 92 ++++++++++++++++++++++
 src/ipa/libipa/algorithms/af_interface.h   | 41 ++++++++++
 src/ipa/libipa/algorithms/meson.build      |  9 +++
 src/ipa/libipa/meson.build                 |  6 ++
 4 files changed, 148 insertions(+)
 create mode 100644 src/ipa/libipa/algorithms/af_interface.cpp
 create mode 100644 src/ipa/libipa/algorithms/af_interface.h
 create mode 100644 src/ipa/libipa/algorithms/meson.build

Hi Daniel

On Thu, Jan 19, 2023 at 09:41:07AM +0100, Daniel Semkowicz via libcamera-devel wrote:
> Define common interface with basic functions that should be supported
> by Auto Focus algorithms.
>

As commented in the previous version, I like this approach very much

> Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>
> ---
>  src/ipa/libipa/algorithms/af_interface.cpp | 92 ++++++++++++++++++++++
>  src/ipa/libipa/algorithms/af_interface.h   | 41 ++++++++++
>  src/ipa/libipa/algorithms/meson.build      |  9 +++
>  src/ipa/libipa/meson.build                 |  6 ++
>  4 files changed, 148 insertions(+)
>  create mode 100644 src/ipa/libipa/algorithms/af_interface.cpp
>  create mode 100644 src/ipa/libipa/algorithms/af_interface.h
>  create mode 100644 src/ipa/libipa/algorithms/meson.build
>
> diff --git a/src/ipa/libipa/algorithms/af_interface.cpp b/src/ipa/libipa/algorithms/af_interface.cpp
> new file mode 100644
> index 00000000..af341d13
> --- /dev/null
> +++ b/src/ipa/libipa/algorithms/af_interface.cpp

Nit: af_interface.[cpp|h] or just af.[cpp|h] ?

> @@ -0,0 +1,92 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Theobroma Systems
> + *
> + * af_interface.cpp - Autofocus control algorithm interface
> + */
> +
> +#include "af_interface.h"
> +
> +/**
> + * \file af_interface.h
> + * \brief AF algorithm common interface
> + */
> +
> +namespace libcamera::ipa::common::algorithms {
> +
> +/**
> + * \class AfInterface
> + * \brief Common interface for auto-focus algorithms
> + * \tparam Module The IPA module type for this class of algorithms

You don't have the Module template argument anymore

> + *
> + * The AfInterface class defines a standard interface for IPA auto focus
> + * algorithms.
> + */
> +
> +/**
> + * \fn AfInterface::setMode()
> + * \brief Set auto focus mode
> + * \param[in] mode AF mode

I was about to suggest \sa controls::AfModeEnum but doxygen already
links the parameter type to the right documentation page, so it's not
necessary.

However, as this interface implements the Af controls defined in
control_ids.yaml I would be tempted to say that we might even /copydoc
so that the documentation is centralized in a single place ??


> + */
> +
> +/**
> + * \fn AfInterface::setRange()
> + * \brief set the range of focus distances that is scanned
> + * \param[in] range AF range
> + */
> +
> +/**
> + * \fn AfInterface::setSpeed()
> + * \brief Set how fast algorithm should move the lens
> + * \param[in] speed Lens move speed
> + */
> +
> +/**
> + * \fn AfInterface::setMeteringMode()
> + * \brief Set AF metering mode
> + * \param[in] metering AF metering mode
> + */
> +
> +/**
> + * \fn AfInterface::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 AfInterface::setTrigger()
> + * \brief Starts or cancels the autofocus scan
> + * \param[in] trigger Trigger mode
> + */
> +
> +/**
> + * \fn AfInterface::setPause()
> + * \brief Pause the autofocus while in AfModeContinuous mode.
> + * \param[in] pause Pause mode
> + */
> +
> +/**
> + * \fn AfInterface::setLensPosition()
> + * \brief Set the lens position while in AfModeManual
> + * \param[in] lensPosition Lens position
> + */
> +
> +/**
> + * \fn AfInterface::getState()

Minor nit: for getters we don't usually prepend "get", so this could
just be AfInterface::state() ?

> + * \brief Get the current state of the AF algorithm
> + * \return AF state
> + */
> +
> +/**
> + * \fn AfInterface::getPauseState()
> + * \brief Get the current pause state of the AF algorithm.
> + * \return AF pause state
> + *
> + * Only applicable in continuous (AfModeContinuous) mode. In other modes,
> + * AfPauseStateRunning is always returned.

See, in this case the documentation of controls::AfPauseState has gone
through a lot of discussions, and trying to resume it here might not
be ideal. Plus it has to be kept in sync with the control
documentation.

I tried

        \copydoc libcamera::controls::AfPauseState

but it doesn't work here..


> + */
> +
> +} /* namespace libcamera::ipa::common::algorithms */
> diff --git a/src/ipa/libipa/algorithms/af_interface.h b/src/ipa/libipa/algorithms/af_interface.h
> new file mode 100644
> index 00000000..b6b7bea6
> --- /dev/null
> +++ b/src/ipa/libipa/algorithms/af_interface.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Theobroma Systems
> + *
> + * af_interface.h - Autofocus control algorithm interface
> + */
> +#pragma once
> +
> +#include <libcamera/control_ids.h>
> +
> +namespace libcamera::ipa::common::algorithms {
> +
> +class AfInterface
> +{
> +public:
> +	AfInterface() = default;

Do you even need a constructor ?

> +
> +	virtual ~AfInterface() {}
> +
> +	virtual void setMode(controls::AfModeEnum mode) = 0;
> +
> +	virtual void setRange(controls::AfRangeEnum range) = 0;
> +
> +	virtual void setSpeed(controls::AfSpeedEnum speed) = 0;
> +
> +	virtual void setMeteringMode(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;
> +
> +	virtual controls::AfStateEnum getState() = 0;
> +
> +	virtual controls::AfPauseStateEnum getPauseState() = 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..0a1f18fa
> --- /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_interface.h',
> +])
> +
> +common_ipa_algorithms_sources = files([
> +    'af_interface.cpp',
> +])
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index 016b8e0e..0cfc551a 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',
> @@ -8,6 +10,8 @@ libipa_headers = files([
>      'module.h',
>  ])
>
> +libipa_headers += common_ipa_algorithms_headers
> +
>  libipa_sources = files([
>      'algorithm.cpp',
>      'camera_sensor_helper.cpp',
> @@ -16,6 +20,8 @@ libipa_sources = files([
>      'module.cpp',
>  ])
>
> +libipa_sources += common_ipa_algorithms_sources
> +
>  libipa_includes = include_directories('..')
>
>  libipa = static_library('ipa', [libipa_sources, libipa_headers],

Minors on the doc apart, it seems all good to me.

Let me copy RPi folks in, as they just upstreamed a PDAF based
auto-focus algorithm to check with them if there's a chance their
implementation can inherit from this class.

Thanks again!
    j
> --
> 2.39.0
>

Hi Jacopo and Daniel,

On Mon, 6 Feb 2023 at 11:20, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Daniel
>
> On Thu, Jan 19, 2023 at 09:41:07AM +0100, Daniel Semkowicz via libcamera-devel wrote:
> > Define common interface with basic functions that should be supported
> > by Auto Focus algorithms.
> >
>
> As commented in the previous version, I like this approach very much
>
> > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>
> > ---
> >  src/ipa/libipa/algorithms/af_interface.cpp | 92 ++++++++++++++++++++++
> >  src/ipa/libipa/algorithms/af_interface.h   | 41 ++++++++++
> >  src/ipa/libipa/algorithms/meson.build      |  9 +++
> >  src/ipa/libipa/meson.build                 |  6 ++
> >  4 files changed, 148 insertions(+)
> >  create mode 100644 src/ipa/libipa/algorithms/af_interface.cpp
> >  create mode 100644 src/ipa/libipa/algorithms/af_interface.h
> >  create mode 100644 src/ipa/libipa/algorithms/meson.build
> >
> > diff --git a/src/ipa/libipa/algorithms/af_interface.cpp b/src/ipa/libipa/algorithms/af_interface.cpp
> > new file mode 100644
> > index 00000000..af341d13
> > --- /dev/null
> > +++ b/src/ipa/libipa/algorithms/af_interface.cpp
>
> Nit: af_interface.[cpp|h] or just af.[cpp|h] ?
>
> > @@ -0,0 +1,92 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2022, Theobroma Systems
> > + *
> > + * af_interface.cpp - Autofocus control algorithm interface
> > + */
> > +
> > +#include "af_interface.h"
> > +
> > +/**
> > + * \file af_interface.h
> > + * \brief AF algorithm common interface
> > + */
> > +
> > +namespace libcamera::ipa::common::algorithms {
> > +
> > +/**
> > + * \class AfInterface
> > + * \brief Common interface for auto-focus algorithms
> > + * \tparam Module The IPA module type for this class of algorithms
>
> You don't have the Module template argument anymore
>
> > + *
> > + * The AfInterface class defines a standard interface for IPA auto focus
> > + * algorithms.
> > + */
> > +
> > +/**
> > + * \fn AfInterface::setMode()
> > + * \brief Set auto focus mode
> > + * \param[in] mode AF mode
>
> I was about to suggest \sa controls::AfModeEnum but doxygen already
> links the parameter type to the right documentation page, so it's not
> necessary.
>
> However, as this interface implements the Af controls defined in
> control_ids.yaml I would be tempted to say that we might even /copydoc
> so that the documentation is centralized in a single place ??
>
>
> > + */
> > +
> > +/**
> > + * \fn AfInterface::setRange()
> > + * \brief set the range of focus distances that is scanned
> > + * \param[in] range AF range
> > + */
> > +
> > +/**
> > + * \fn AfInterface::setSpeed()
> > + * \brief Set how fast algorithm should move the lens
> > + * \param[in] speed Lens move speed
> > + */
> > +
> > +/**
> > + * \fn AfInterface::setMeteringMode()
> > + * \brief Set AF metering mode
> > + * \param[in] metering AF metering mode
> > + */
> > +
> > +/**
> > + * \fn AfInterface::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 AfInterface::setTrigger()
> > + * \brief Starts or cancels the autofocus scan
> > + * \param[in] trigger Trigger mode
> > + */
> > +
> > +/**
> > + * \fn AfInterface::setPause()
> > + * \brief Pause the autofocus while in AfModeContinuous mode.
> > + * \param[in] pause Pause mode
> > + */
> > +
> > +/**
> > + * \fn AfInterface::setLensPosition()
> > + * \brief Set the lens position while in AfModeManual
> > + * \param[in] lensPosition Lens position
> > + */
> > +
> > +/**
> > + * \fn AfInterface::getState()
>
> Minor nit: for getters we don't usually prepend "get", so this could
> just be AfInterface::state() ?
>
> > + * \brief Get the current state of the AF algorithm
> > + * \return AF state
> > + */
> > +
> > +/**
> > + * \fn AfInterface::getPauseState()
> > + * \brief Get the current pause state of the AF algorithm.
> > + * \return AF pause state
> > + *
> > + * Only applicable in continuous (AfModeContinuous) mode. In other modes,
> > + * AfPauseStateRunning is always returned.
>
> See, in this case the documentation of controls::AfPauseState has gone
> through a lot of discussions, and trying to resume it here might not
> be ideal. Plus it has to be kept in sync with the control
> documentation.
>
> I tried
>
>         \copydoc libcamera::controls::AfPauseState
>
> but it doesn't work here..
>
>
> > + */
> > +
> > +} /* namespace libcamera::ipa::common::algorithms */
> > diff --git a/src/ipa/libipa/algorithms/af_interface.h b/src/ipa/libipa/algorithms/af_interface.h
> > new file mode 100644
> > index 00000000..b6b7bea6
> > --- /dev/null
> > +++ b/src/ipa/libipa/algorithms/af_interface.h
> > @@ -0,0 +1,41 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2022, Theobroma Systems
> > + *
> > + * af_interface.h - Autofocus control algorithm interface
> > + */
> > +#pragma once
> > +
> > +#include <libcamera/control_ids.h>
> > +
> > +namespace libcamera::ipa::common::algorithms {
> > +
> > +class AfInterface
> > +{
> > +public:
> > +     AfInterface() = default;
>
> Do you even need a constructor ?
>
> > +
> > +     virtual ~AfInterface() {}
> > +
> > +     virtual void setMode(controls::AfModeEnum mode) = 0;
> > +
> > +     virtual void setRange(controls::AfRangeEnum range) = 0;
> > +
> > +     virtual void setSpeed(controls::AfSpeedEnum speed) = 0;
> > +
> > +     virtual void setMeteringMode(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;
> > +
> > +     virtual controls::AfStateEnum getState() = 0;
> > +
> > +     virtual controls::AfPauseStateEnum getPauseState() = 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..0a1f18fa
> > --- /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_interface.h',
> > +])
> > +
> > +common_ipa_algorithms_sources = files([
> > +    'af_interface.cpp',
> > +])
> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > index 016b8e0e..0cfc551a 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',
> > @@ -8,6 +10,8 @@ libipa_headers = files([
> >      'module.h',
> >  ])
> >
> > +libipa_headers += common_ipa_algorithms_headers
> > +
> >  libipa_sources = files([
> >      'algorithm.cpp',
> >      'camera_sensor_helper.cpp',
> > @@ -16,6 +20,8 @@ libipa_sources = files([
> >      'module.cpp',
> >  ])
> >
> > +libipa_sources += common_ipa_algorithms_sources
> > +
> >  libipa_includes = include_directories('..')
> >
> >  libipa = static_library('ipa', [libipa_sources, libipa_headers],
>
> Minors on the doc apart, it seems all good to me.
>
> Let me copy RPi folks in, as they just upstreamed a PDAF based
> auto-focus algorithm to check with them if there's a chance their
> implementation can inherit from this class.

This does indeed look very similar to the RPi AfAlgorithm interface found at
src/ipa/raspberrypi/controller/af_algorithm.h.  Unfortunately all our algorithm
interfaces need to inherit from our Algorithm interface class, so we will not be
able to use this class directly.

Regards,
Naush

>
> Thanks again!
>     j
> > --
> > 2.39.0
> >

Quoting Naushir Patuck via libcamera-devel (2023-02-06 11:39:56)
> Hi Jacopo and Daniel,
> 
> On Mon, 6 Feb 2023 at 11:20, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
> >
> > Hi Daniel
> >
> > On Thu, Jan 19, 2023 at 09:41:07AM +0100, Daniel Semkowicz via libcamera-devel wrote:
> > > Define common interface with basic functions that should be supported
> > > by Auto Focus algorithms.
> > >
> >
> > As commented in the previous version, I like this approach very much
> >
> > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>
> > > ---
> > >  src/ipa/libipa/algorithms/af_interface.cpp | 92 ++++++++++++++++++++++
> > >  src/ipa/libipa/algorithms/af_interface.h   | 41 ++++++++++
> > >  src/ipa/libipa/algorithms/meson.build      |  9 +++
> > >  src/ipa/libipa/meson.build                 |  6 ++
> > >  4 files changed, 148 insertions(+)
> > >  create mode 100644 src/ipa/libipa/algorithms/af_interface.cpp
> > >  create mode 100644 src/ipa/libipa/algorithms/af_interface.h
> > >  create mode 100644 src/ipa/libipa/algorithms/meson.build
> > >
> > > diff --git a/src/ipa/libipa/algorithms/af_interface.cpp b/src/ipa/libipa/algorithms/af_interface.cpp
> > > new file mode 100644
> > > index 00000000..af341d13
> > > --- /dev/null
> > > +++ b/src/ipa/libipa/algorithms/af_interface.cpp
> >
> > Nit: af_interface.[cpp|h] or just af.[cpp|h] ?
> >
> > > @@ -0,0 +1,92 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2022, Theobroma Systems
> > > + *
> > > + * af_interface.cpp - Autofocus control algorithm interface
> > > + */
> > > +
> > > +#include "af_interface.h"
> > > +
> > > +/**
> > > + * \file af_interface.h
> > > + * \brief AF algorithm common interface
> > > + */
> > > +
> > > +namespace libcamera::ipa::common::algorithms {
> > > +
> > > +/**
> > > + * \class AfInterface
> > > + * \brief Common interface for auto-focus algorithms
> > > + * \tparam Module The IPA module type for this class of algorithms
> >
> > You don't have the Module template argument anymore
> >
> > > + *
> > > + * The AfInterface class defines a standard interface for IPA auto focus
> > > + * algorithms.
> > > + */
> > > +
> > > +/**
> > > + * \fn AfInterface::setMode()
> > > + * \brief Set auto focus mode
> > > + * \param[in] mode AF mode
> >
> > I was about to suggest \sa controls::AfModeEnum but doxygen already
> > links the parameter type to the right documentation page, so it's not
> > necessary.
> >
> > However, as this interface implements the Af controls defined in
> > control_ids.yaml I would be tempted to say that we might even /copydoc
> > so that the documentation is centralized in a single place ??
> >
> >
> > > + */
> > > +
> > > +/**
> > > + * \fn AfInterface::setRange()
> > > + * \brief set the range of focus distances that is scanned
> > > + * \param[in] range AF range
> > > + */
> > > +
> > > +/**
> > > + * \fn AfInterface::setSpeed()
> > > + * \brief Set how fast algorithm should move the lens
> > > + * \param[in] speed Lens move speed
> > > + */
> > > +
> > > +/**
> > > + * \fn AfInterface::setMeteringMode()
> > > + * \brief Set AF metering mode
> > > + * \param[in] metering AF metering mode
> > > + */
> > > +
> > > +/**
> > > + * \fn AfInterface::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 AfInterface::setTrigger()
> > > + * \brief Starts or cancels the autofocus scan
> > > + * \param[in] trigger Trigger mode
> > > + */
> > > +
> > > +/**
> > > + * \fn AfInterface::setPause()
> > > + * \brief Pause the autofocus while in AfModeContinuous mode.
> > > + * \param[in] pause Pause mode
> > > + */
> > > +
> > > +/**
> > > + * \fn AfInterface::setLensPosition()
> > > + * \brief Set the lens position while in AfModeManual
> > > + * \param[in] lensPosition Lens position
> > > + */
> > > +
> > > +/**
> > > + * \fn AfInterface::getState()
> >
> > Minor nit: for getters we don't usually prepend "get", so this could
> > just be AfInterface::state() ?
> >
> > > + * \brief Get the current state of the AF algorithm
> > > + * \return AF state
> > > + */
> > > +
> > > +/**
> > > + * \fn AfInterface::getPauseState()
> > > + * \brief Get the current pause state of the AF algorithm.
> > > + * \return AF pause state
> > > + *
> > > + * Only applicable in continuous (AfModeContinuous) mode. In other modes,
> > > + * AfPauseStateRunning is always returned.
> >
> > See, in this case the documentation of controls::AfPauseState has gone
> > through a lot of discussions, and trying to resume it here might not
> > be ideal. Plus it has to be kept in sync with the control
> > documentation.
> >
> > I tried
> >
> >         \copydoc libcamera::controls::AfPauseState
> >
> > but it doesn't work here..
> >
> >
> > > + */
> > > +
> > > +} /* namespace libcamera::ipa::common::algorithms */
> > > diff --git a/src/ipa/libipa/algorithms/af_interface.h b/src/ipa/libipa/algorithms/af_interface.h
> > > new file mode 100644
> > > index 00000000..b6b7bea6
> > > --- /dev/null
> > > +++ b/src/ipa/libipa/algorithms/af_interface.h
> > > @@ -0,0 +1,41 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2022, Theobroma Systems
> > > + *
> > > + * af_interface.h - Autofocus control algorithm interface
> > > + */
> > > +#pragma once
> > > +
> > > +#include <libcamera/control_ids.h>
> > > +
> > > +namespace libcamera::ipa::common::algorithms {
> > > +
> > > +class AfInterface
> > > +{
> > > +public:
> > > +     AfInterface() = default;
> >
> > Do you even need a constructor ?
> >
> > > +
> > > +     virtual ~AfInterface() {}
> > > +
> > > +     virtual void setMode(controls::AfModeEnum mode) = 0;
> > > +
> > > +     virtual void setRange(controls::AfRangeEnum range) = 0;
> > > +
> > > +     virtual void setSpeed(controls::AfSpeedEnum speed) = 0;
> > > +
> > > +     virtual void setMeteringMode(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;
> > > +
> > > +     virtual controls::AfStateEnum getState() = 0;
> > > +
> > > +     virtual controls::AfPauseStateEnum getPauseState() = 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..0a1f18fa
> > > --- /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_interface.h',
> > > +])
> > > +
> > > +common_ipa_algorithms_sources = files([
> > > +    'af_interface.cpp',
> > > +])
> > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > > index 016b8e0e..0cfc551a 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',
> > > @@ -8,6 +10,8 @@ libipa_headers = files([
> > >      'module.h',
> > >  ])
> > >
> > > +libipa_headers += common_ipa_algorithms_headers
> > > +
> > >  libipa_sources = files([
> > >      'algorithm.cpp',
> > >      'camera_sensor_helper.cpp',
> > > @@ -16,6 +20,8 @@ libipa_sources = files([
> > >      'module.cpp',
> > >  ])
> > >
> > > +libipa_sources += common_ipa_algorithms_sources
> > > +
> > >  libipa_includes = include_directories('..')
> > >
> > >  libipa = static_library('ipa', [libipa_sources, libipa_headers],
> >
> > Minors on the doc apart, it seems all good to me.
> >
> > Let me copy RPi folks in, as they just upstreamed a PDAF based
> > auto-focus algorithm to check with them if there's a chance their
> > implementation can inherit from this class.

That was one thing I was wondering while looking through this series.
Given that the base implementation ends up with things like 'initBase',
and 'queueRequestBase' ... It feels like perhaps this might be better
with composition rather than inheritance, so that if an implementation
wants to use this - it can just create the object in that algorithm
class and make calls on that object directly?

--
Kieran


> 
> This does indeed look very similar to the RPi AfAlgorithm interface found at
> src/ipa/raspberrypi/controller/af_algorithm.h.  Unfortunately all our algorithm
> interfaces need to inherit from our Algorithm interface class, so we will not be
> able to use this class directly.
> 
> Regards,
> Naush
> 
> >
> > Thanks again!
> >     j
> > > --
> > > 2.39.0
> > >

Hi Kieran

On Mon, Feb 06, 2023 at 03:00:32PM +0000, Kieran Bingham via libcamera-devel wrote:
> Quoting Naushir Patuck via libcamera-devel (2023-02-06 11:39:56)
> > Hi Jacopo and Daniel,
> >
> > On Mon, 6 Feb 2023 at 11:20, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
> > >
> > > Hi Daniel
> > >
> > > On Thu, Jan 19, 2023 at 09:41:07AM +0100, Daniel Semkowicz via libcamera-devel wrote:
> > > > Define common interface with basic functions that should be supported
> > > > by Auto Focus algorithms.
> > > >
> > >
> > > As commented in the previous version, I like this approach very much
> > >
> > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>
> > > > ---
> > > >  src/ipa/libipa/algorithms/af_interface.cpp | 92 ++++++++++++++++++++++
> > > >  src/ipa/libipa/algorithms/af_interface.h   | 41 ++++++++++
> > > >  src/ipa/libipa/algorithms/meson.build      |  9 +++
> > > >  src/ipa/libipa/meson.build                 |  6 ++
> > > >  4 files changed, 148 insertions(+)
> > > >  create mode 100644 src/ipa/libipa/algorithms/af_interface.cpp
> > > >  create mode 100644 src/ipa/libipa/algorithms/af_interface.h
> > > >  create mode 100644 src/ipa/libipa/algorithms/meson.build
> > > >
> > > > diff --git a/src/ipa/libipa/algorithms/af_interface.cpp b/src/ipa/libipa/algorithms/af_interface.cpp
> > > > new file mode 100644
> > > > index 00000000..af341d13
> > > > --- /dev/null
> > > > +++ b/src/ipa/libipa/algorithms/af_interface.cpp
> > >
> > > Nit: af_interface.[cpp|h] or just af.[cpp|h] ?
> > >
> > > > @@ -0,0 +1,92 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2022, Theobroma Systems
> > > > + *
> > > > + * af_interface.cpp - Autofocus control algorithm interface
> > > > + */
> > > > +
> > > > +#include "af_interface.h"
> > > > +
> > > > +/**
> > > > + * \file af_interface.h
> > > > + * \brief AF algorithm common interface
> > > > + */
> > > > +
> > > > +namespace libcamera::ipa::common::algorithms {
> > > > +
> > > > +/**
> > > > + * \class AfInterface
> > > > + * \brief Common interface for auto-focus algorithms
> > > > + * \tparam Module The IPA module type for this class of algorithms
> > >
> > > You don't have the Module template argument anymore
> > >
> > > > + *
> > > > + * The AfInterface class defines a standard interface for IPA auto focus
> > > > + * algorithms.
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn AfInterface::setMode()
> > > > + * \brief Set auto focus mode
> > > > + * \param[in] mode AF mode
> > >
> > > I was about to suggest \sa controls::AfModeEnum but doxygen already
> > > links the parameter type to the right documentation page, so it's not
> > > necessary.
> > >
> > > However, as this interface implements the Af controls defined in
> > > control_ids.yaml I would be tempted to say that we might even /copydoc
> > > so that the documentation is centralized in a single place ??
> > >
> > >
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn AfInterface::setRange()
> > > > + * \brief set the range of focus distances that is scanned
> > > > + * \param[in] range AF range
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn AfInterface::setSpeed()
> > > > + * \brief Set how fast algorithm should move the lens
> > > > + * \param[in] speed Lens move speed
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn AfInterface::setMeteringMode()
> > > > + * \brief Set AF metering mode
> > > > + * \param[in] metering AF metering mode
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn AfInterface::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 AfInterface::setTrigger()
> > > > + * \brief Starts or cancels the autofocus scan
> > > > + * \param[in] trigger Trigger mode
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn AfInterface::setPause()
> > > > + * \brief Pause the autofocus while in AfModeContinuous mode.
> > > > + * \param[in] pause Pause mode
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn AfInterface::setLensPosition()
> > > > + * \brief Set the lens position while in AfModeManual
> > > > + * \param[in] lensPosition Lens position
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn AfInterface::getState()
> > >
> > > Minor nit: for getters we don't usually prepend "get", so this could
> > > just be AfInterface::state() ?
> > >
> > > > + * \brief Get the current state of the AF algorithm
> > > > + * \return AF state
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn AfInterface::getPauseState()
> > > > + * \brief Get the current pause state of the AF algorithm.
> > > > + * \return AF pause state
> > > > + *
> > > > + * Only applicable in continuous (AfModeContinuous) mode. In other modes,
> > > > + * AfPauseStateRunning is always returned.
> > >
> > > See, in this case the documentation of controls::AfPauseState has gone
> > > through a lot of discussions, and trying to resume it here might not
> > > be ideal. Plus it has to be kept in sync with the control
> > > documentation.
> > >
> > > I tried
> > >
> > >         \copydoc libcamera::controls::AfPauseState
> > >
> > > but it doesn't work here..
> > >
> > >
> > > > + */
> > > > +
> > > > +} /* namespace libcamera::ipa::common::algorithms */
> > > > diff --git a/src/ipa/libipa/algorithms/af_interface.h b/src/ipa/libipa/algorithms/af_interface.h
> > > > new file mode 100644
> > > > index 00000000..b6b7bea6
> > > > --- /dev/null
> > > > +++ b/src/ipa/libipa/algorithms/af_interface.h
> > > > @@ -0,0 +1,41 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2022, Theobroma Systems
> > > > + *
> > > > + * af_interface.h - Autofocus control algorithm interface
> > > > + */
> > > > +#pragma once
> > > > +
> > > > +#include <libcamera/control_ids.h>
> > > > +
> > > > +namespace libcamera::ipa::common::algorithms {
> > > > +
> > > > +class AfInterface
> > > > +{
> > > > +public:
> > > > +     AfInterface() = default;
> > >
> > > Do you even need a constructor ?
> > >
> > > > +
> > > > +     virtual ~AfInterface() {}
> > > > +
> > > > +     virtual void setMode(controls::AfModeEnum mode) = 0;
> > > > +
> > > > +     virtual void setRange(controls::AfRangeEnum range) = 0;
> > > > +
> > > > +     virtual void setSpeed(controls::AfSpeedEnum speed) = 0;
> > > > +
> > > > +     virtual void setMeteringMode(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;
> > > > +
> > > > +     virtual controls::AfStateEnum getState() = 0;
> > > > +
> > > > +     virtual controls::AfPauseStateEnum getPauseState() = 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..0a1f18fa
> > > > --- /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_interface.h',
> > > > +])
> > > > +
> > > > +common_ipa_algorithms_sources = files([
> > > > +    'af_interface.cpp',
> > > > +])
> > > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > > > index 016b8e0e..0cfc551a 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',
> > > > @@ -8,6 +10,8 @@ libipa_headers = files([
> > > >      'module.h',
> > > >  ])
> > > >
> > > > +libipa_headers += common_ipa_algorithms_headers
> > > > +
> > > >  libipa_sources = files([
> > > >      'algorithm.cpp',
> > > >      'camera_sensor_helper.cpp',
> > > > @@ -16,6 +20,8 @@ libipa_sources = files([
> > > >      'module.cpp',
> > > >  ])
> > > >
> > > > +libipa_sources += common_ipa_algorithms_sources
> > > > +
> > > >  libipa_includes = include_directories('..')
> > > >
> > > >  libipa = static_library('ipa', [libipa_sources, libipa_headers],
> > >
> > > Minors on the doc apart, it seems all good to me.
> > >
> > > Let me copy RPi folks in, as they just upstreamed a PDAF based
> > > auto-focus algorithm to check with them if there's a chance their
> > > implementation can inherit from this class.
>
> That was one thing I was wondering while looking through this series.
> Given that the base implementation ends up with things like 'initBase',
> and 'queueRequestBase' ... It feels like perhaps this might be better
> with composition rather than inheritance, so that if an implementation
> wants to use this - it can just create the object in that algorithm
> class and make calls on that object directly?

Kind of agree with the observation.

At the least, if inheritance should be used (which I'm not sure)
it is not necessary to name the "base" class methods with the "base"
suffix

------------------------------------------------------------------------------
--- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp
+++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp
@@ -69,7 +69,7 @@ int AfHillClimbing::initBase(const YamlObject &tuningData)
  * This function should be called in the queueRequest() function of the derived class.
  * See alse: libcamera::ipa::Algorithm::queueRequest()
  */
-void AfHillClimbing::queueRequestBase([[maybe_unused]] const uint32_t frame, const ControlList &controls)
+void AfHillClimbing::queueRequest([[maybe_unused]] const uint32_t frame, const ControlList &controls)
 {
        for (auto const &[id, value] : controls) {
                switch (id) {
diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h
index 6ce95884c03d..9d68968865c5 100644
--- a/src/ipa/libipa/algorithms/af_hill_climbing.h
+++ b/src/ipa/libipa/algorithms/af_hill_climbing.h
@@ -36,7 +36,7 @@ public:

 protected:
        int initBase(const YamlObject &tuningData);
-       void queueRequestBase(const uint32_t frame, const ControlList &controls);
+       void queueRequest(const uint32_t frame, const ControlList &controls);
        uint32_t processAutofocus(double currentContrast);
        void setFramesToSkip(uint32_t n);

diff --git a/src/ipa/rkisp1/algorithms/af.cpp b/src/ipa/rkisp1/algorithms/af.cpp
index 65768fc45e5b..bda8f273f695 100644
--- a/src/ipa/rkisp1/algorithms/af.cpp
+++ b/src/ipa/rkisp1/algorithms/af.cpp
@@ -74,7 +74,7 @@ void Af::queueRequest([[maybe_unused]] IPAContext &context,
                      [[maybe_unused]] IPAFrameContext &frameContext,
                      const ControlList &controls)
 {
-       queueRequestBase(frame, controls);
+       AfHillClimbing::queueRequest(frame, controls);

        for (auto const &[id, value] : controls) {
                switch (id) {
------------------------------------------------------------------------------

When it comes to inheritance vs composition, I think what makes a
difference is if and IPA module will always need a platform-specific
implementation in src/ipa/[rkisp1|ipu3|...]/algorithms/af.cpp or it
could potentially instantiate the generic
src/ipa/libipa/algorithms/af_hill_climbing.cpp or not.

As there will be always be a platform specific part I don't think
that's the case.

Right now (behold! UML with ascii) the class hierarchy looks
like:


        +-----------------------+
        |      <interface>      |
        |Algorithm::AfInterface |
        |                       |
        +-----------------------+
                   |
                   |
                   _
                   v
        +---------------------------+    +--------------+
        | Algorithm::AfHillClimbing |    | <interface>  |
        |---------------------------|    | Algorithm    |
        |                           |    |              |
        +---------------------------+    +--------------+
                   |                          |
                   |                          |
                   _                          |
                   v                          |
        +---------------------------+         |
        | Algorithm::RkISP1::Af     |         |
        |---------------------------|<|-------+
        |                           |
        +---------------------------+


Should we instead split the hierarchy and have the platform specific
implementation inherit from Algorithm (so that the IPA module can use
it by calling on it the usual init(), configure() and queueRequest()
methods) and have the AutoFocus hierachy used by composition ?

Something like

  +------------------------+               +-----------------------+
  |      <interface>       |               |      <interface>      |
  |      Algorithm         |               |Algorithm::AfInterface |
  |                        |               |                       |
  +------------------------+               +-----------------------+
             |                                      |
             |                                      |
             |                                      _
             _                                      v
             v                            +---------------------------+
  +---------------------------+           | Algorithm::AfHillClimbing |
  | Algorithm::RkISP1::Af     |---------<>|---------------------------|
  |---------------------------|           |                           |
  |                           |           +---------------------------+
  +---------------------------+


True, each platform specific implementation (rkisp1, IPU3 etc) will
need to reimplement the Algorithm interface ?

What do others think ?


>
> --
> Kieran
>
>
> >
> > This does indeed look very similar to the RPi AfAlgorithm interface found at
> > src/ipa/raspberrypi/controller/af_algorithm.h.  Unfortunately all our algorithm
> > interfaces need to inherit from our Algorithm interface class, so we will not be
> > able to use this class directly.
> >
> > Regards,
> > Naush
> >
> > >
> > > Thanks again!
> > >     j
> > > > --
> > > > 2.39.0
> > > >

Hi Jacopo, hi Kieran, hi Naushir!

On Mon, Feb 6, 2023 at 6:47 PM Jacopo Mondi via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> Hi Kieran
>
> On Mon, Feb 06, 2023 at 03:00:32PM +0000, Kieran Bingham via libcamera-devel wrote:
> > Quoting Naushir Patuck via libcamera-devel (2023-02-06 11:39:56)
> > > Hi Jacopo and Daniel,
> > >
> > > On Mon, 6 Feb 2023 at 11:20, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
> > > >
> > > > Hi Daniel
> > > >
> > > > On Thu, Jan 19, 2023 at 09:41:07AM +0100, Daniel Semkowicz via libcamera-devel wrote:
> > > > > Define common interface with basic functions that should be supported
> > > > > by Auto Focus algorithms.
> > > > >
> > > >
> > > > As commented in the previous version, I like this approach very much
> > > >
> > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com>
> > > > > ---
> > > > >  src/ipa/libipa/algorithms/af_interface.cpp | 92 ++++++++++++++++++++++
> > > > >  src/ipa/libipa/algorithms/af_interface.h   | 41 ++++++++++
> > > > >  src/ipa/libipa/algorithms/meson.build      |  9 +++
> > > > >  src/ipa/libipa/meson.build                 |  6 ++
> > > > >  4 files changed, 148 insertions(+)
> > > > >  create mode 100644 src/ipa/libipa/algorithms/af_interface.cpp
> > > > >  create mode 100644 src/ipa/libipa/algorithms/af_interface.h
> > > > >  create mode 100644 src/ipa/libipa/algorithms/meson.build
> > > > >
> > > > > diff --git a/src/ipa/libipa/algorithms/af_interface.cpp b/src/ipa/libipa/algorithms/af_interface.cpp
> > > > > new file mode 100644
> > > > > index 00000000..af341d13
> > > > > --- /dev/null
> > > > > +++ b/src/ipa/libipa/algorithms/af_interface.cpp
> > > >
> > > > Nit: af_interface.[cpp|h] or just af.[cpp|h] ?
> > > >
> > > > > @@ -0,0 +1,92 @@
> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2022, Theobroma Systems
> > > > > + *
> > > > > + * af_interface.cpp - Autofocus control algorithm interface
> > > > > + */
> > > > > +
> > > > > +#include "af_interface.h"
> > > > > +
> > > > > +/**
> > > > > + * \file af_interface.h
> > > > > + * \brief AF algorithm common interface
> > > > > + */
> > > > > +
> > > > > +namespace libcamera::ipa::common::algorithms {
> > > > > +
> > > > > +/**
> > > > > + * \class AfInterface
> > > > > + * \brief Common interface for auto-focus algorithms
> > > > > + * \tparam Module The IPA module type for this class of algorithms
> > > >
> > > > You don't have the Module template argument anymore
> > > >
> > > > > + *
> > > > > + * The AfInterface class defines a standard interface for IPA auto focus
> > > > > + * algorithms.
> > > > > + */
> > > > > +
> > > > > +/**
> > > > > + * \fn AfInterface::setMode()
> > > > > + * \brief Set auto focus mode
> > > > > + * \param[in] mode AF mode
> > > >
> > > > I was about to suggest \sa controls::AfModeEnum but doxygen already
> > > > links the parameter type to the right documentation page, so it's not
> > > > necessary.
> > > >
> > > > However, as this interface implements the Af controls defined in
> > > > control_ids.yaml I would be tempted to say that we might even /copydoc
> > > > so that the documentation is centralized in a single place ??
> > > >
> > > >
> > > > > + */
> > > > > +
> > > > > +/**
> > > > > + * \fn AfInterface::setRange()
> > > > > + * \brief set the range of focus distances that is scanned
> > > > > + * \param[in] range AF range
> > > > > + */
> > > > > +
> > > > > +/**
> > > > > + * \fn AfInterface::setSpeed()
> > > > > + * \brief Set how fast algorithm should move the lens
> > > > > + * \param[in] speed Lens move speed
> > > > > + */
> > > > > +
> > > > > +/**
> > > > > + * \fn AfInterface::setMeteringMode()
> > > > > + * \brief Set AF metering mode
> > > > > + * \param[in] metering AF metering mode
> > > > > + */
> > > > > +
> > > > > +/**
> > > > > + * \fn AfInterface::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 AfInterface::setTrigger()
> > > > > + * \brief Starts or cancels the autofocus scan
> > > > > + * \param[in] trigger Trigger mode
> > > > > + */
> > > > > +
> > > > > +/**
> > > > > + * \fn AfInterface::setPause()
> > > > > + * \brief Pause the autofocus while in AfModeContinuous mode.
> > > > > + * \param[in] pause Pause mode
> > > > > + */
> > > > > +
> > > > > +/**
> > > > > + * \fn AfInterface::setLensPosition()
> > > > > + * \brief Set the lens position while in AfModeManual
> > > > > + * \param[in] lensPosition Lens position
> > > > > + */
> > > > > +
> > > > > +/**
> > > > > + * \fn AfInterface::getState()
> > > >
> > > > Minor nit: for getters we don't usually prepend "get", so this could
> > > > just be AfInterface::state() ?
> > > >
> > > > > + * \brief Get the current state of the AF algorithm
> > > > > + * \return AF state
> > > > > + */
> > > > > +
> > > > > +/**
> > > > > + * \fn AfInterface::getPauseState()
> > > > > + * \brief Get the current pause state of the AF algorithm.
> > > > > + * \return AF pause state
> > > > > + *
> > > > > + * Only applicable in continuous (AfModeContinuous) mode. In other modes,
> > > > > + * AfPauseStateRunning is always returned.
> > > >
> > > > See, in this case the documentation of controls::AfPauseState has gone
> > > > through a lot of discussions, and trying to resume it here might not
> > > > be ideal. Plus it has to be kept in sync with the control
> > > > documentation.
> > > >
> > > > I tried
> > > >
> > > >         \copydoc libcamera::controls::AfPauseState
> > > >
> > > > but it doesn't work here..
> > > >
> > > >
> > > > > + */
> > > > > +
> > > > > +} /* namespace libcamera::ipa::common::algorithms */
> > > > > diff --git a/src/ipa/libipa/algorithms/af_interface.h b/src/ipa/libipa/algorithms/af_interface.h
> > > > > new file mode 100644
> > > > > index 00000000..b6b7bea6
> > > > > --- /dev/null
> > > > > +++ b/src/ipa/libipa/algorithms/af_interface.h
> > > > > @@ -0,0 +1,41 @@
> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2022, Theobroma Systems
> > > > > + *
> > > > > + * af_interface.h - Autofocus control algorithm interface
> > > > > + */
> > > > > +#pragma once
> > > > > +
> > > > > +#include <libcamera/control_ids.h>
> > > > > +
> > > > > +namespace libcamera::ipa::common::algorithms {
> > > > > +
> > > > > +class AfInterface
> > > > > +{
> > > > > +public:
> > > > > +     AfInterface() = default;
> > > >
> > > > Do you even need a constructor ?
> > > >
> > > > > +
> > > > > +     virtual ~AfInterface() {}
> > > > > +
> > > > > +     virtual void setMode(controls::AfModeEnum mode) = 0;
> > > > > +
> > > > > +     virtual void setRange(controls::AfRangeEnum range) = 0;
> > > > > +
> > > > > +     virtual void setSpeed(controls::AfSpeedEnum speed) = 0;
> > > > > +
> > > > > +     virtual void setMeteringMode(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;
> > > > > +
> > > > > +     virtual controls::AfStateEnum getState() = 0;
> > > > > +
> > > > > +     virtual controls::AfPauseStateEnum getPauseState() = 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..0a1f18fa
> > > > > --- /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_interface.h',
> > > > > +])
> > > > > +
> > > > > +common_ipa_algorithms_sources = files([
> > > > > +    'af_interface.cpp',
> > > > > +])
> > > > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > > > > index 016b8e0e..0cfc551a 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',
> > > > > @@ -8,6 +10,8 @@ libipa_headers = files([
> > > > >      'module.h',
> > > > >  ])
> > > > >
> > > > > +libipa_headers += common_ipa_algorithms_headers
> > > > > +
> > > > >  libipa_sources = files([
> > > > >      'algorithm.cpp',
> > > > >      'camera_sensor_helper.cpp',
> > > > > @@ -16,6 +20,8 @@ libipa_sources = files([
> > > > >      'module.cpp',
> > > > >  ])
> > > > >
> > > > > +libipa_sources += common_ipa_algorithms_sources
> > > > > +
> > > > >  libipa_includes = include_directories('..')
> > > > >
> > > > >  libipa = static_library('ipa', [libipa_sources, libipa_headers],
> > > >
> > > > Minors on the doc apart, it seems all good to me.
> > > >
> > > > Let me copy RPi folks in, as they just upstreamed a PDAF based
> > > > auto-focus algorithm to check with them if there's a chance their
> > > > implementation can inherit from this class.
> >
> > That was one thing I was wondering while looking through this series.
> > Given that the base implementation ends up with things like 'initBase',
> > and 'queueRequestBase' ... It feels like perhaps this might be better
> > with composition rather than inheritance, so that if an implementation
> > wants to use this - it can just create the object in that algorithm
> > class and make calls on that object directly?
>
> Kind of agree with the observation.
>
> At the least, if inheritance should be used (which I'm not sure)
> it is not necessary to name the "base" class methods with the "base"
> suffix
>
> ------------------------------------------------------------------------------
> --- a/src/ipa/libipa/algorithms/af_hill_climbing.cpp
> +++ b/src/ipa/libipa/algorithms/af_hill_climbing.cpp
> @@ -69,7 +69,7 @@ int AfHillClimbing::initBase(const YamlObject &tuningData)
>   * This function should be called in the queueRequest() function of the derived class.
>   * See alse: libcamera::ipa::Algorithm::queueRequest()
>   */
> -void AfHillClimbing::queueRequestBase([[maybe_unused]] const uint32_t frame, const ControlList &controls)
> +void AfHillClimbing::queueRequest([[maybe_unused]] const uint32_t frame, const ControlList &controls)
>  {
>         for (auto const &[id, value] : controls) {
>                 switch (id) {
> diff --git a/src/ipa/libipa/algorithms/af_hill_climbing.h b/src/ipa/libipa/algorithms/af_hill_climbing.h
> index 6ce95884c03d..9d68968865c5 100644
> --- a/src/ipa/libipa/algorithms/af_hill_climbing.h
> +++ b/src/ipa/libipa/algorithms/af_hill_climbing.h
> @@ -36,7 +36,7 @@ public:
>
>  protected:
>         int initBase(const YamlObject &tuningData);
> -       void queueRequestBase(const uint32_t frame, const ControlList &controls);
> +       void queueRequest(const uint32_t frame, const ControlList &controls);
>         uint32_t processAutofocus(double currentContrast);
>         void setFramesToSkip(uint32_t n);
>
> diff --git a/src/ipa/rkisp1/algorithms/af.cpp b/src/ipa/rkisp1/algorithms/af.cpp
> index 65768fc45e5b..bda8f273f695 100644
> --- a/src/ipa/rkisp1/algorithms/af.cpp
> +++ b/src/ipa/rkisp1/algorithms/af.cpp
> @@ -74,7 +74,7 @@ void Af::queueRequest([[maybe_unused]] IPAContext &context,
>                       [[maybe_unused]] IPAFrameContext &frameContext,
>                       const ControlList &controls)
>  {
> -       queueRequestBase(frame, controls);
> +       AfHillClimbing::queueRequest(frame, controls);
>
>         for (auto const &[id, value] : controls) {
>                 switch (id) {
> ------------------------------------------------------------------------------
>
> When it comes to inheritance vs composition, I think what makes a
> difference is if and IPA module will always need a platform-specific
> implementation in src/ipa/[rkisp1|ipu3|...]/algorithms/af.cpp or it
> could potentially instantiate the generic
> src/ipa/libipa/algorithms/af_hill_climbing.cpp or not.
>
> As there will be always be a platform specific part I don't think
> that's the case.
>
> Right now (behold! UML with ascii) the class hierarchy looks
> like:
>
>
>         +-----------------------+
>         |      <interface>      |
>         |Algorithm::AfInterface |
>         |                       |
>         +-----------------------+
>                    |
>                    |
>                    _
>                    v
>         +---------------------------+    +--------------+
>         | Algorithm::AfHillClimbing |    | <interface>  |
>         |---------------------------|    | Algorithm    |
>         |                           |    |              |
>         +---------------------------+    +--------------+
>                    |                          |
>                    |                          |
>                    _                          |
>                    v                          |
>         +---------------------------+         |
>         | Algorithm::RkISP1::Af     |         |
>         |---------------------------|<|-------+
>         |                           |
>         +---------------------------+
>
>
> Should we instead split the hierarchy and have the platform specific
> implementation inherit from Algorithm (so that the IPA module can use
> it by calling on it the usual init(), configure() and queueRequest()
> methods) and have the AutoFocus hierachy used by composition ?
>
> Something like
>
>   +------------------------+               +-----------------------+
>   |      <interface>       |               |      <interface>      |
>   |      Algorithm         |               |Algorithm::AfInterface |
>   |                        |               |                       |
>   +------------------------+               +-----------------------+
>              |                                      |
>              |                                      |
>              |                                      _
>              _                                      v
>              v                            +---------------------------+
>   +---------------------------+           | Algorithm::AfHillClimbing |
>   | Algorithm::RkISP1::Af     |---------<>|---------------------------|
>   |---------------------------|           |                           |
>   |                           |           +---------------------------+
>   +---------------------------+
>
>
> True, each platform specific implementation (rkisp1, IPU3 etc) will
> need to reimplement the Algorithm interface ?
>
> What do others think ?

With the current usage, both approaches are not so different.
As you already mentioned, We will always need a platform specific
implementation on top, so it will not be possible to implement
the complete interface in the common way.
For example, init(), configure(), queueRequest() methods from the base
class will need to be called explicitly in the derive init(),
configure(), queueRequest() methods due to parameters mismatch.
And when you look at the code it even looks more like a composition
approach:

    AfHillClimbing::queueRequest(...)

vs

    afHillClimbing->queueRequest(...)

To be honest, I am not a big fan of multiple inheritance, so if it is
possible, I would avoid it.

Best regards
Daniel

>
>
> >
> > --
> > Kieran
> >
> >
> > >
> > > This does indeed look very similar to the RPi AfAlgorithm interface found at
> > > src/ipa/raspberrypi/controller/af_algorithm.h.  Unfortunately all our algorithm
> > > interfaces need to inherit from our Algorithm interface class, so we will not be
> > > able to use this class directly.
> > >
> > > Regards,
> > > Naush
> > >
> > > >
> > > > Thanks again!
> > > >     j
> > > > > --
> > > > > 2.39.0
> > > > >