[libcamera-devel] [PATCH v4 03/10] ipa: Add base class defining AF algorithm interface
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Mon Mar 20 21:02:19 CET 2023
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 ?
> +
> +/**
> + * \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..
> + */
> +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
> + break;
> + }
> + default:
Should we error here ?
> + 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
> + * \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
>
More information about the libcamera-devel
mailing list