[libcamera-devel] [PATCH v3 3/8] ipa: Add base class defining AF algorithm interface
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Feb 6 16:00:32 CET 2023
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 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.
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
> > >
More information about the libcamera-devel
mailing list