[libcamera-devel] [PATCH v2 06/11] ipa: libipa: Introduce Algorithm class template
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Tue Nov 23 14:28:32 CET 2021
Hi Laurent,
On 23/11/2021 11:02, Laurent Pinchart wrote:
> Hi Jean-Michel,
>
> Thank you for the patch.
>
> On Tue, Nov 23, 2021 at 10:14:46AM +0100, Jean-Michel Hautbois wrote:
>> The algorithms are using the same function names with specialized
>> parameters. Instead of duplicating code, introduce a libipa Algorithm
>> class which implements a base class with template parameters in libipa,
>> and use it in each IPA.
>>
>> As we now won't need an algorithm class for each IPA, move the
>> documentation to libipa, and make it agnostic of the IPA used.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>> ---
>> src/ipa/ipu3/algorithms/algorithm.h | 12 ++----
>> src/ipa/ipu3/algorithms/meson.build | 1 -
>> .../{ipu3/algorithms => libipa}/algorithm.cpp | 35 ++++++----------
>> src/ipa/libipa/algorithm.h | 41 +++++++++++++++++++
>> src/ipa/libipa/meson.build | 1 +
>> src/ipa/rkisp1/algorithms/algorithm.h | 28 +++++++++++++
>> src/ipa/rkisp1/algorithms/meson.build | 4 ++
>> src/ipa/rkisp1/meson.build | 4 ++
>> src/ipa/rkisp1/rkisp1.cpp | 5 ++-
>
> Shouldn't this have been split in two, with the introduction of
> src/ipa/libipa/algorithm.h and porting of IPU3 first, and then the
> rkisp1 additions ?
OK, I will split it in two :-).
>
>> 9 files changed, 97 insertions(+), 34 deletions(-)
>> rename src/ipa/{ipu3/algorithms => libipa}/algorithm.cpp (74%)
>> create mode 100644 src/ipa/libipa/algorithm.h
>> create mode 100644 src/ipa/rkisp1/algorithms/algorithm.h
>> create mode 100644 src/ipa/rkisp1/algorithms/meson.build
>>
>> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h
>> index 43f5d8b0..3c0ce461 100644
>> --- a/src/ipa/ipu3/algorithms/algorithm.h
>> +++ b/src/ipa/ipu3/algorithms/algorithm.h
>> @@ -9,21 +9,15 @@
>>
>> #include <libcamera/ipa/ipu3_ipa_interface.h>
>>
>> +#include <libipa/algorithm.h>
>> +
>> #include "ipa_context.h"
>>
>> namespace libcamera {
>>
>> namespace ipa::ipu3 {
>>
>> -class Algorithm
>> -{
>> -public:
>> - virtual ~Algorithm() {}
>> -
>> - virtual int configure(IPAContext &context, const IPAConfigInfo &configInfo);
>> - virtual void prepare(IPAContext &context, ipu3_uapi_params *params);
>> - virtual void process(IPAContext &context, const ipu3_uapi_stats_3a *stats);
>> -};
>> +using Algorithm = libcamera::ipa::Algorithm<IPAContext, IPAConfigInfo, ipu3_uapi_params, ipu3_uapi_stats_3a>;
>>
>> } /* namespace ipa::ipu3 */
>>
>> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build
>> index 3ec42f72..4db6ae1d 100644
>> --- a/src/ipa/ipu3/algorithms/meson.build
>> +++ b/src/ipa/ipu3/algorithms/meson.build
>> @@ -2,7 +2,6 @@
>>
>> ipu3_ipa_algorithms = files([
>> 'agc.cpp',
>> - 'algorithm.cpp',
>> 'awb.cpp',
>> 'blc.cpp',
>> 'tone_mapping.cpp',
>> diff --git a/src/ipa/ipu3/algorithms/algorithm.cpp b/src/ipa/libipa/algorithm.cpp
>> similarity index 74%
>> rename from src/ipa/ipu3/algorithms/algorithm.cpp
>> rename to src/ipa/libipa/algorithm.cpp
>> index 3e7e3018..888ffb57 100644
>> --- a/src/ipa/ipu3/algorithms/algorithm.cpp
>> +++ b/src/ipa/libipa/algorithm.cpp
>> @@ -2,7 +2,7 @@
>> /*
>> * Copyright (C) 2021, Ideas On Board
>> *
>> - * algorithm.cpp - IPU3 control algorithm interface
>> + * algorithm.cpp - IPA control algorithm interface
>> */
>>
>> #include "algorithm.h"
>> @@ -14,11 +14,11 @@
>>
>> namespace libcamera {
>>
>> -namespace ipa::ipu3 {
>> +namespace ipa {
>>
>> /**
>> * \class Algorithm
>> - * \brief The base class for all IPU3 algorithms
>> + * \brief The base class for all IPA algorithms
>
> The template parameters need to be documented with \tparam.
>
I haven't been warned by Doxygen, I will change that (did not know there
is a specific template parameters keyword ;-)).
>> *
>> * The Algorithm class defines a standard interface for IPA algorithms. By
>> * abstracting algorithms, it makes possible the implementation of generic code
>> @@ -26,6 +26,7 @@ namespace ipa::ipu3 {
>> */
>>
>> /**
>> + * \fn Algorithm::configure()
>> * \brief Configure the Algorithm given an IPAConfigInfo
>> * \param[in] context The shared IPA context
>> * \param[in] configInfo The IPA configuration data, received from the pipeline
>> @@ -39,37 +40,29 @@ namespace ipa::ipu3 {
>> *
>> * \return 0 if successful, an error code otherwise
>> */
>> -int Algorithm::configure([[maybe_unused]] IPAContext &context,
>> - [[maybe_unused]] const IPAConfigInfo &configInfo)
>> -{
>> - return 0;
>> -}
>>
>> /**
>> + * \fn Algorithm::prepare()
>> * \brief Fill the \a params buffer with ISP processing parameters for a frame
>> * \param[in] context The shared IPA context
>> - * \param[out] params The IPU3 specific parameters.
>> + * \param[out] params The ISP specific parameters.
>> *
>> * This function is called for every frame when the camera is running before it
>> - * is processed by the ImgU to prepare the ImgU processing parameters for that
>> - * frame.
>> + * is processed by the ISP.
>
> * is processed by the ISP to prepare the ISP processing parameters for that
> * frame.
>
>> *
>> * Algorithms shall fill in the parameter structure fields appropriately to
>> - * configure the ImgU processing blocks that they are responsible for. This
>> + * configure the ISP processing blocks that they are responsible for. This
>> * includes setting fields and flags that enable those processing blocks.
>> */
>> -void Algorithm::prepare([[maybe_unused]] IPAContext &context,
>> - [[maybe_unused]] ipu3_uapi_params *params)
>> -{
>> -}
>>
>> /**
>> + * \fn Algorithm::process()
>> * \brief Process ISP statistics, and run algorithm operations
>> * \param[in] context The shared IPA context
>> - * \param[in] stats The IPU3 statistics and ISP results
>> + * \param[in] stats The IPA statistics and ISP results
>> *
>> * This function is called while camera is running for every frame processed by
>> - * the ImgU, to process statistics generated from that frame by the ImgU.
>> + * the ISP, to process statistics generated from that frame by the ISP.
>> * Algorithms shall use this data to run calculations and update their state
>> * accordingly.
>> *
>> @@ -91,11 +84,7 @@ void Algorithm::prepare([[maybe_unused]] IPAContext &context,
>> * Care shall be taken to ensure the ordering of access to the information
>> * such that the algorithms use up to date state as required.
>> */
>> -void Algorithm::process([[maybe_unused]] IPAContext &context,
>> - [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
>> -{
>> -}
>>
>> -} /* namespace ipa::ipu3 */
>> +} /* namespace ipa */
>>
>> } /* namespace libcamera */
>> diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h
>> new file mode 100644
>> index 00000000..74dc49c4
>> --- /dev/null
>> +++ b/src/ipa/libipa/algorithm.h
>> @@ -0,0 +1,41 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2021, Ideas On Board
>> + *
>> + * algorithm.h - ISP control algorithm interface
>> + */
>> +#ifndef __LIBCAMERA_IPA_LIBIPA_ALGORITHM_H__
>> +#define __LIBCAMERA_IPA_LIBIPA_ALGORITHM_H__
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa {
>> +
>> +template<typename Context, typename IPAConfigInfo, typename Params, typename Stats>
>
> s/IPAConfigInfo/Config/
>
>> +class Algorithm
>> +{
>> +public:
>> + virtual ~Algorithm() {}
>> +
>> + virtual int configure([[maybe_unused]] Context &context,
>> + [[maybe_unused]] const IPAConfigInfo &configInfo)
>> + {
>> + return 0;
>> + }
>> +
>> + virtual void prepare([[maybe_unused]] Context &context,
>> + [[maybe_unused]] Params *params)
>> + {
>> + }
>> +
>> + virtual void process([[maybe_unused]] Context &context,
>> + [[maybe_unused]] const Stats *stats)
>> + {
>> + }
>> +};
>> +
>> +} /* namespace ipa */
>> +
>> +} /* namespace libcamera */
>> +
>> +#endif /* __LIBCAMERA_IPA_LIBIPA_ALGORITHM_H__ */
>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
>> index 4d073a03..161cc5a1 100644
>> --- a/src/ipa/libipa/meson.build
>> +++ b/src/ipa/libipa/meson.build
>> @@ -1,6 +1,7 @@
>> # SPDX-License-Identifier: CC0-1.0
>>
>> libipa_headers = files([
>> + 'algorithm.h',
>> 'camera_sensor_helper.h',
>> 'histogram.h'
>> ])
>> diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h
>> new file mode 100644
>> index 00000000..dfa58727
>> --- /dev/null
>> +++ b/src/ipa/rkisp1/algorithms/algorithm.h
>> @@ -0,0 +1,28 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2021, Ideas On Board
>> + *
>> + * algorithm.h - RkISP1 control algorithm interface
>> + */
>> +#ifndef __LIBCAMERA_IPA_RKISP1_ALGORITHM_H__
>> +#define __LIBCAMERA_IPA_RKISP1_ALGORITHM_H__
>> +
>> +#include <linux/rkisp1-config.h>
>> +
>> +#include <libcamera/ipa/rkisp1_ipa_interface.h>
>> +
>> +#include <libipa/algorithm.h>
>> +
>> +#include "ipa_context.h"
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::rkisp1 {
>> +
>> +using Algorithm = libcamera::ipa::Algorithm<IPAContext, IPACameraSensorInfo, rkisp1_params_cfg, rkisp1_stat_buffer>;
>> +
>> +} /* namespace ipa::rkisp1 */
>> +
>> +} /* namespace libcamera */
>> +
>> +#endif /* __LIBCAMERA_IPA_RKISP1_ALGORITHM_H__ */
>> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
>> new file mode 100644
>> index 00000000..1c6c59cf
>> --- /dev/null
>> +++ b/src/ipa/rkisp1/algorithms/meson.build
>> @@ -0,0 +1,4 @@
>> +# SPDX-License-Identifier: CC0-1.0
>> +
>> +rkisp1_ipa_algorithms = files([
>> +])
>> diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build
>> index 3683c922..8c822fbb 100644
>> --- a/src/ipa/rkisp1/meson.build
>> +++ b/src/ipa/rkisp1/meson.build
>> @@ -1,5 +1,7 @@
>> # SPDX-License-Identifier: CC0-1.0
>>
>> +subdir('algorithms')
>> +
>> ipa_name = 'ipa_rkisp1'
>>
>> rkisp1_ipa_sources = files([
>> @@ -7,6 +9,8 @@ rkisp1_ipa_sources = files([
>> 'rkisp1.cpp',
>> ])
>>
>> +rkisp1_ipa_sources += rkisp1_ipa_algorithms
>> +
>> mod = shared_module(ipa_name,
>> [rkisp1_ipa_sources, libcamera_generated_ipa_headers],
>> name_prefix : '',
>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>> index 34c3f9a2..0c54d8ec 100644
>> --- a/src/ipa/rkisp1/rkisp1.cpp
>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>> @@ -25,7 +25,7 @@
>>
>> #include <libcamera/internal/mapped_framebuffer.h>
>>
>> -#include "ipa_context.h"
>> +#include "algorithms/algorithm.h"
>> #include "libipa/camera_sensor_helper.h"
>>
>> namespace libcamera {
>> @@ -82,6 +82,9 @@ private:
>>
>> /* Local parameter storage */
>> struct IPAContext context_;
>> +
>> + /* Maintain the algorithms used by the IPA */
>> + std::list<std::unique_ptr<ipa::rkisp1::Algorithm>> algorithms_;
>> };
>>
>> int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)
>
More information about the libcamera-devel
mailing list