[libcamera-devel] [PATCH v6 03/10] ipa: Add base class defining AF algorithm interface
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed May 31 17:18:41 CEST 2023
Hi Daniel,
Getting back to this mail thread now that I'm back from vacation :-)
On Fri, May 12, 2023 at 03:45:30PM +0200, Daniel Semkowicz wrote:
> On Wed, Apr 26, 2023 at 3:24 AM Laurent Pinchart wrote:
> > On Fri, Mar 31, 2023 at 10:19:23AM +0200, Daniel Semkowicz via libcamera-devel wrote:
> > > Define common interface with pure virtual methods that should be
> > > implemented by the Auto Focus algorithm implementations.
> > > Interface methods match the AF controls 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.
> >
> > I'm a bit concerned here, as creating abstract classes with a single
> > user always worries me that we don't have enough data points to create a
> > correct abstraction. Your series includes a single AF implementation, do
> > you already envision different implementations that could use the same
> > interface ? I'm sorry if this point has been raised during the review of
> > previous versions, I haven't followed them very closely so far.
>
> This interface basically maps the AF controls on the separate interface
> functions and provides queueRequest() function to translate requested controls
> to function calls. This makes the interface closely tied with the AF controls.
> In the end each AF algorithm needs to handle the AF controls, so it forces
> algorithm architecture to be structured in the way, the controls are structured.
> I do not envision another implementation yet, but you can already look on the
> Raspberry Pi implementation of the PDAF. This is a different implementation,
> but the RPiController::AfAlgorithm is structured almost the same way.
I'm afraid I'm not thrilled :-( This forces AF controls to be processed
separately by the AF algorithm implementation, as they're passed to
different functions. I can't tell if this will match the needs of
different AF implementations. As there's fairly little code in this
helper, I would rather squash it with the AF implementation for rkisp1.
We can extract it in a shared helper later if we realize it can be
useful for other platforms.
> > > Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> > > ---
> > > src/ipa/libipa/algorithms/af.cpp | 159 ++++++++++++++++++++++++++
> > > src/ipa/libipa/algorithms/af.h | 46 ++++++++
> > > src/ipa/libipa/algorithms/meson.build | 9 ++
> > > src/ipa/libipa/meson.build | 6 +
> > > 4 files changed, 220 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..0808aa3f
> > > --- /dev/null
> > > +++ b/src/ipa/libipa/algorithms/af.cpp
> > > @@ -0,0 +1,159 @@
> > > +/* 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 {
> > > +
> > > +namespace ipa::algorithms {
> > > +
> > > +/**
> > > + * \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.
> > > + */
> > > +void Af::queueRequest(const ControlList &controls)
> > > +{
> > > + using namespace controls;
> > > +
> > > + for (auto const &[id, value] : controls) {
> > > + switch (id) {
> > > + 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>());
> > > + break;
> > > + }
> > > + default:
> > > + break;
> > > + }
> > > + }
> > > +}
> > > +
> > > +/**
> > > + * \fn Af::setMode()
> > > + * \brief Set auto focus mode
> > > + * \param[in] mode AF mode
> > > + *
> > > + * \sa libcamera::controls::AfMode
> > > + */
> > > +
> > > +/**
> > > + * \fn Af::setRange()
> > > + * \brief Set the range of focus distances that is scanned
> > > + * \param[in] range AF range
> > > + *
> > > + * \sa libcamera::controls::AfRange
> > > + */
> > > +
> > > +/**
> > > + * \fn Af::setSpeed()
> > > + * \brief Set how fast algorithm should move the lens
> > > + * \param[in] speed Lens move speed
> > > + *
> > > +* \sa libcamera::controls::AfSpeed
> > > + */
> > > +
> > > +/**
> > > + * \fn Af::setMeteringMode()
> > > + * \brief Set AF metering mode
> > > + * \param[in] metering AF metering mode
> > > + *
> > > + * \sa libcamera::controls::AfMetering
> > > + */
> > > +
> > > +/**
> > > + * \fn Af::setWindows()
> > > + * \brief Set AF windows
> > > + * \param[in] windows AF windows
> > > + *
> > > + * \sa libcamera::controls::AfWindows
> > > + */
> > > +
> > > +/**
> > > + * \fn Af::setTrigger()
> > > + * \brief Starts or cancels the autofocus scan
> > > + * \param[in] trigger Trigger mode
> > > + *
> > > + * \sa libcamera::controls::AfTrigger
> > > + */
> > > +
> > > +/**
> > > + * \fn Af::setPause()
> > > + * \brief Pause the autofocus while in AfModeContinuous mode
> > > + * \param[in] pause Pause mode
> > > + *
> > > + * \sa libcamera::controls::AfPause
> > > + */
> > > +
> > > +/**
> > > + * \fn Af::setLensPosition()
> > > + * \brief Set the lens position while in AfModeManual
> > > + * \param[in] lensPosition Lens position
> > > + *
> > > + * \sa libcamera::controls::LensPosition
> > > + */
> > > +
> > > +/**
> > > + * \fn Af::state()
> > > + * \brief Get the current state of the AF algorithm
> > > + * \return AF state
> > > + *
> > > + * \sa libcamera::controls::AfState
> > > + */
> > > +
> > > +/**
> > > + * \fn Af::pauseState()
> > > + * \brief Get the current pause state of the AF algorithm
> > > + * \return AF pause state
> > > + *
> > > + * \sa libcamera::controls::AfPauseState
> > > + */
> > > +
> > > +} /* namespace ipa::algorithms */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/ipa/libipa/algorithms/af.h b/src/ipa/libipa/algorithms/af.h
> > > new file mode 100644
> > > index 00000000..47c919fe
> > > --- /dev/null
> > > +++ b/src/ipa/libipa/algorithms/af.h
> > > @@ -0,0 +1,46 @@
> > > +/* 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>
> >
> > You should also include geometry.h and span.h as you're using the
> > Rectangle and Span classes below.
> >
> > > +
> > > +namespace libcamera {
> > > +
> > > +namespace ipa::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 ipa::algorithms */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/ipa/libipa/algorithms/meson.build b/src/ipa/libipa/algorithms/meson.build
> > > new file mode 100644
> > > index 00000000..3df4798f
> > > --- /dev/null
> > > +++ b/src/ipa/libipa/algorithms/meson.build
> > > @@ -0,0 +1,9 @@
> > > +# SPDX-License-Identifier: CC0-1.0
> > > +
> > > +libipa_algorithms_headers = files([
> > > + 'af.h',
> > > +])
> > > +
> > > +libipa_algorithms_sources = files([
> > > + 'af.cpp',
> > > +])
> > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > > index 016b8e0e..134c2dba 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 += libipa_algorithms_headers
> > > +
> > > libipa_sources = files([
> > > 'algorithm.cpp',
> > > 'camera_sensor_helper.cpp',
> > > @@ -16,6 +20,8 @@ libipa_sources = files([
> > > 'module.cpp',
> > > ])
> > >
> > > +libipa_sources += libipa_algorithms_sources
> > > +
> > > libipa_includes = include_directories('..')
> > >
> > > libipa = static_library('ipa', [libipa_sources, libipa_headers],
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list