[libcamera-devel] [PATCH v4 03/10] ipa: Add base class defining AF algorithm interface

[Daniel Semkowicz]

Hi Jacopo,

On Mon, Mar 20, 2023 at 9:02 PM Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> Hi Daniel
>
> On Tue, Mar 14, 2023 at 03:48:27PM +0100, Daniel Semkowicz wrote:
> > Define common interface with basic methods that should be supported
> > by the Auto Focus algorithms. Interface methods match the AF controls
>
> Define common interface with pure virtual methods that should be
> implemented by the Auto Focus algorithm implementations.
>
> > that can be set in the frame request.
> > Common implementation of controls parsing is provided, so the AF
> > algorithms deriving from this interface should be able to reuse it.
> >
> > Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> > ---
> >  src/ipa/libipa/algorithms/af.cpp      | 155 ++++++++++++++++++++++++++
> >  src/ipa/libipa/algorithms/af.h        |  41 +++++++
> >  src/ipa/libipa/algorithms/meson.build |   9 ++
> >  src/ipa/libipa/meson.build            |   6 +
> >  4 files changed, 211 insertions(+)
> >  create mode 100644 src/ipa/libipa/algorithms/af.cpp
> >  create mode 100644 src/ipa/libipa/algorithms/af.h
> >  create mode 100644 src/ipa/libipa/algorithms/meson.build
> >
> > diff --git a/src/ipa/libipa/algorithms/af.cpp b/src/ipa/libipa/algorithms/af.cpp
> > new file mode 100644
> > index 00000000..2052080f
> > --- /dev/null
> > +++ b/src/ipa/libipa/algorithms/af.cpp
> > @@ -0,0 +1,155 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2023, Theobroma Systems
> > + *
> > + * af.cpp - Autofocus control algorithm interface
> > + */
> > +
> > +#include "af.h"
> > +
> > +/**
> > + * \file af.h
> > + * \brief AF algorithm common interface
> > + */
> > +
> > +namespace libcamera::ipa::common::algorithms {
>
> Nit: other algrithms implementations have
>
> namespace libcamera {
>
> namespace ipa::...
>
> }
>
> }
>
> Also, we have namespaces as ipa::ipu3::algorithms and
> ipa::rkisp1::algorithms... Should this be just ipa::algorithms ?
>

Sure, I will fix that.

> > +
> > +/**
> > + * \class Af
> > + * \brief Common interface for auto-focus algorithms
> > + *
> > + * The Af class defines a standard interface for IPA auto focus algorithms.
> > + */
> > +
> > +/**
> > + * \brief Provide control values to the algorithm
> > + * \param[in] controls The list of user controls
> > + *
> > + * This method should be called in the libcamera::ipa::Algorithm::queueRequest()
> > + * method of the platform layer.
>
> It probably make sense not using \copydoc of Algorithm::queueRequest() as
> the function signature is different like you've done..

You are right. I will document the parameters separately. Do you think adding
a reference as "see also" makes sense here, or the difference is too big?

>
> > + */
> > +void Af::queueRequest(const ControlList &controls)
> > +{
> > +     using namespace controls;
> > +
> > +     for (auto const &[id, value] : controls) {
> > +             switch (id) {
>
> Not a big fan of switching on controls' numeric ids, but in this case
> I guess it's effective...
>
> > +             case AF_MODE: {
> > +                     setMode(static_cast<AfModeEnum>(value.get<int32_t>()));
> > +                     break;
> > +             }
> > +             case AF_RANGE: {
> > +                     setRange(static_cast<AfRangeEnum>(value.get<int32_t>()));
> > +                     break;
> > +             }
> > +             case AF_SPEED: {
> > +                     setSpeed(static_cast<AfSpeedEnum>(value.get<int32_t>()));
> > +                     break;
> > +             }
> > +             case AF_METERING: {
> > +                     setMeteringMode(static_cast<AfMeteringEnum>(value.get<int32_t>()));
> > +                     break;
> > +             }
> > +             case AF_WINDOWS: {
> > +                     setWindows(value.get<Span<const Rectangle>>());
> > +                     break;
> > +             }
> > +             case AF_TRIGGER: {
> > +                     setTrigger(static_cast<AfTriggerEnum>(value.get<int32_t>()));
> > +                     break;
> > +             }
> > +             case AF_PAUSE: {
> > +                     setPause(static_cast<AfPauseEnum>(value.get<int32_t>()));
> > +                     break;
> > +             }
> > +             case LENS_POSITION: {
> > +                     setLensPosition(value.get<float>());
>
> Mmmm I wonder if the algorithm's state machine (in example: you can't
> set LensPosition if running in a mode !Manual) could be implemented in
> this base class. However this is not a blocker for this patch

Yes, I had a similar idea, but as this is the first common algorithm
implementation, I am not completely sure which parts will be always
reusable. Since it serves as the Interface for other algorithms, it was
safer to leave the details to the "upper layer". With additional AF
algorithms using this interface, it should be easier to move
the common part here.

>
> > +                     break;
> > +             }
> > +             default:
>
> Should we error here ?

We can't, because controls are not filtered and there can be controls
of other algorithms in the list.

>
> > +                     break;
> > +             }
> > +     }
> > +}
> > +
> > +/**
> > + * \fn Af::setMode()
> > + * \copybrief libcamera::controls::AfMode
>
> it's a bit weird to read a function documentation as:
> "Control to set the mode of the AF (autofocus) algorithm."
>
> I'm all for referring to the controls (maybe with the \sa anchor)
> but I'm afraid the functions need a bit of documentation of their own
>
> We can help with that

So I need to merge the previous version of the functions documentation
and the current one :D

>
> > + * \param[in] mode AF mode
> > + *
> > + * \copydetails libcamera::controls::AfMode
> > + */
> > +
> > +/**
> > + * \fn Af::setRange()
> > + * \copybrief libcamera::controls::AfRange
> > + * \param[in] range AF range
> > + *
> > + * \copydetails libcamera::controls::AfRange
> > + */
> > +
> > +/**
> > + * \fn Af::setSpeed()
> > + * \copybrief libcamera::controls::AfSpeed
> > + * \param[in] speed Lens move speed
> > + *
> > +* \copydetails libcamera::controls::AfSpeed
> > + */
> > +
> > +/**
> > + * \fn Af::setMeteringMode()
> > + * \copybrief libcamera::controls::AfMetering
> > + * \param[in] metering AF metering mode
> > + *
> > + * \copydetails libcamera::controls::AfMetering
> > + */
> > +
> > +/**
> > + * \fn Af::setWindows()
> > + * \copybrief libcamera::controls::AfWindows
> > + * \param[in] windows AF windows
> > + *
> > + * \copydetails libcamera::controls::AfWindows
> > + */
> > +
> > +/**
> > + * \fn Af::setTrigger()
> > + * \copybrief libcamera::controls::AfTrigger
> > + * \param[in] trigger Trigger mode
> > + *
> > + * \copydetails libcamera::controls::AfTrigger
> > + */
> > +
> > +/**
> > + * \fn Af::setPause()
> > + * \copybrief libcamera::controls::AfPause
> > + * \param[in] pause Pause mode
> > + *
> > + * \copydetails libcamera::controls::AfPause
> > + */
> > +
> > +/**
> > + * \fn Af::setLensPosition()
> > + * \copybrief libcamera::controls::LensPosition
> > + * \param[in] lensPosition Lens position
> > + *
> > + * \copydetails libcamera::controls::LensPosition
> > + */
> > +
> > +/**
> > + * \fn Af::state()
> > + * \copybrief libcamera::controls::AfState
> > + * \return AF state
> > + *
> > + * \copydetails libcamera::controls::AfState
> > + */
> > +
> > +/**
> > + * \fn Af::pauseState()
> > + * \copybrief libcamera::controls::AfPauseState
> > + * \return AF pause state
> > + *
> > + * \copydetails libcamera::controls::AfPauseState
> > + */
> > +
> > +} /* namespace libcamera::ipa::common::algorithms */
> > diff --git a/src/ipa/libipa/algorithms/af.h b/src/ipa/libipa/algorithms/af.h
> > new file mode 100644
> > index 00000000..1428902c
> > --- /dev/null
> > +++ b/src/ipa/libipa/algorithms/af.h
> > @@ -0,0 +1,41 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2023, Theobroma Systems
> > + *
> > + * af.h - Autofocus control algorithm interface
> > + */
> > +#pragma once
> > +
> > +#include <libcamera/control_ids.h>
> > +
> > +namespace libcamera::ipa::common::algorithms {
> > +
> > +class Af
> > +{
> > +public:
> > +     virtual ~Af() = default;
> > +
> > +     void queueRequest(const ControlList &controls);
> > +
> > +     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 state() = 0;
> > +
> > +     virtual controls::AfPauseStateEnum pauseState() = 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..7602976c
> > --- /dev/null
> > +++ b/src/ipa/libipa/algorithms/meson.build
> > @@ -0,0 +1,9 @@
> > +# SPDX-License-Identifier: CC0-1.0
> > +
> > +common_ipa_algorithms_headers = files([
>
> I would s/common_ipa_/libipa_/
>
> > +    'af.h',
> > +])
> > +
> > +common_ipa_algorithms_sources = files([
> > +    'af.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
> > +
>
> Mostly minors though! We can fix on top if a new version is not
> required
>
> >  libipa_includes = include_directories('..')
> >
> >  libipa = static_library('ipa', [libipa_sources, libipa_headers],
> > --
> > 2.39.2
> >

Best regards
Daniel