| Message ID | 20230314144834.85193-4-dse@thaumatec.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 >
Hi Jacopo, On Mon, Mar 20, 2023 at 9:02 PM Jacopo Mondi <jacopo.mondi@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@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
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@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@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
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 { + +/** + * \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() + * \copybrief libcamera::controls::AfMode + * \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([ + '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 + libipa_includes = include_directories('..') libipa = static_library('ipa', [libipa_sources, libipa_headers],
Define common interface with basic methods that should be supported by the Auto Focus algorithms. 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. Signed-off-by: Daniel Semkowicz <dse@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