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

[Naushir Patuck]

Hi Jacopo and Daniel,

On Mon, 6 Feb 2023 at 11:20, Jacopo Mondi <jacopo.mondi at 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 at 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
> >