[libcamera-devel] [PATCH v4 03/10] ipa: Add base class defining AF algorithm interface
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Mar 21 09:07:33 CET 2023
Hi Daniel
On Tue, Mar 21, 2023 at 08:09:12AM +0100, Daniel Semkowicz wrote:
> 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?
>
Sorry, my comment was not clear. I was just saying you've done the
right thing not using \copydoc :)
I think this documentation block is fine as it is
> >
> > > + */
> > > +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.
>
Makes sense!
> >
> > > + 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.
>
Right...
> >
> > > + 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
>
Let me know how we can help
> >
> > > + * \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
More information about the libcamera-devel
mailing list