[libcamera-devel] [PATCH v1 4/8] ipa: ipu3: Introduce the Algorithm class

[Jean-Michel Hautbois]

Hi Laurent,

On 19/11/2021 14:22, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> Thank you for the patch.
> 
> On Fri, Nov 19, 2021 at 12:16:50PM +0100, Jean-Michel Hautbois wrote:
>> Now that everything is in place, introduce the Algorithm class and
>> declare it in IPA::RkISP1. There is no functional change yet.
>>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>> ---
>>   src/ipa/rkisp1/algorithms/algorithm.cpp | 101 ++++++++++++++++++++++++
>>   src/ipa/rkisp1/algorithms/algorithm.h   |  32 ++++++++
>>   src/ipa/rkisp1/algorithms/meson.build   |   5 ++
>>   src/ipa/rkisp1/meson.build              |   3 +
>>   src/ipa/rkisp1/rkisp1.cpp               |   5 +-
>>   5 files changed, 145 insertions(+), 1 deletion(-)
>>   create mode 100644 src/ipa/rkisp1/algorithms/algorithm.cpp
>>   create mode 100644 src/ipa/rkisp1/algorithms/algorithm.h
>>   create mode 100644 src/ipa/rkisp1/algorithms/meson.build
>>
>> diff --git a/src/ipa/rkisp1/algorithms/algorithm.cpp b/src/ipa/rkisp1/algorithms/algorithm.cpp
>> new file mode 100644
>> index 00000000..be3cd9f6
>> --- /dev/null
>> +++ b/src/ipa/rkisp1/algorithms/algorithm.cpp
>> @@ -0,0 +1,101 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2021, Ideas On Board
>> + *
>> + * algorithm.cpp - RkISP1 control algorithm interface
>> + */
>> +
>> +#include "algorithm.h"
>> +
>> +/**
>> + * \file algorithm.h
>> + * \brief Algorithm common interface
>> + */
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::rkisp1 {
>> +
>> +/**
>> + * \class Algorithm
>> + * \brief The base class for all RkISP1 algorithms
>> + *
>> + * The Algorithm class defines a standard interface for IPA algorithms. By
>> + * abstracting algorithms, it makes possible the implementation of generic code
>> + * to manage algorithms regardless of their specific type.
>> + */
>> +
>> +/**
>> + * \brief Configure the Algorithm given an IPAConfigInfo
>> + * \param[in] context The shared IPA context
>> + * \param[in] IPACameraSensorInfo The IPA configuration data, received from the
>> + * pipeline handler
>> + *
>> + * Algorithms may implement a configure operation to pre-calculate
>> + * parameters prior to commencing streaming.
>> + *
>> + * Configuration state may be stored in the IPASessionConfiguration structure of
>> + * the IPAContext.
>> + *
>> + * \return 0 if successful, an error code otherwise
>> + */
>> +int Algorithm::configure([[maybe_unused]] IPAContext &context,
>> +			 [[maybe_unused]] const IPACameraSensorInfo &configInfo)
>> +{
>> +	return 0;
>> +}
>> +
>> +/**
>> + * \brief Fill the \a params buffer with ISP processing parameters for a frame
>> + * \param[in] context The shared IPA context
>> + * \param[out] params The RkISP1 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.
>> + *
>> + * Algorithms shall fill in the parameter structure fields appropriately to
>> + * configure the ImgU 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]] rkisp1_params_cfg *params)
>> +{
>> +}
>> +
>> +/**
>> + * \brief Process ISP statistics, and run algorithm operations
>> + * \param[in] context The shared IPA context
>> + * \param[in] stats The RkISP1 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.
>> + * Algorithms shall use this data to run calculations and update their state
>> + * accordingly.
>> + *
>> + * Processing shall not take an undue amount of time, and any extended or
>> + * computationally expensive calculations or operations must be handled
>> + * asynchronously in a separate thread.
>> + *
>> + * Algorithms can store state in their respective IPAFrameContext structures,
>> + * and reference state from the IPAFrameContext of other algorithms.
>> + *
>> + * \todo Historical data may be required as part of the processing.
>> + * Either the previous frame, or the IPAFrameContext state of the frame
>> + * that generated the statistics for this operation may be required for
>> + * some advanced algorithms to prevent oscillations or support control
>> + * loops correctly. Only a single IPAFrameContext is available currently,
>> + * and so any data stored may represent the results of the previously
>> + * completed operations.
>> + *
>> + * 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 rkisp1_stat_buffer *stats)
>> +{
>> +}
>> +
>> +} /* namespace ipa::rkisp1 */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h
>> new file mode 100644
>> index 00000000..5365a860
>> --- /dev/null
>> +++ b/src/ipa/rkisp1/algorithms/algorithm.h
>> @@ -0,0 +1,32 @@
>> +/* 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 <libcamera/ipa/rkisp1_ipa_interface.h>
>> +
>> +#include "ipa_context.h"
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::rkisp1 {
>> +
>> +class Algorithm
>> +{
>> +public:
>> +	virtual ~Algorithm() {}
>> +
>> +	virtual int configure(IPAContext &context, const IPACameraSensorInfo &configInfo);
>> +	virtual void prepare(IPAContext &context, rkisp1_params_cfg *params);
>> +	virtual void process(IPAContext &context, const rkisp1_stat_buffer *stats);
>> +};
>> +
>> +} /* namespace ipa::rkisp1 */
>> +
>> +} /* namespace libcamera */
>> +
>> +#endif /* __LIBCAMERA_IPA_RKISP1_ALGORITHM_H__ */
> 
> This is all very similar to the IPU3 Algorithm class. The header file
> doesn't bother me that much, but the duplicated documentation is
> annoying.
> 
> We obviously can't share the same class between the two IPA modules, but
> I wonder if we could get away with a class template in libipa:
> 
> namespace libcamera {
> 
> namespace ipa {
> 
> template<typename Context, typename Params, typename Stats>
> class Algorithm
> {
> public:
> 	virtual ~Algorithm() {}
> 
> 	virtual int configure(Context &context, const IPACameraSensorInfo &configInfo);
> 	virtual void prepare(Context &context, Params *params);
> 	virtual void process(Context &context, const Stats *stats);
> };
> 
> } /* namespace ipa */
> 
> } /* namespace libcamera */
> 
> Then, in src/ipa/rkisp1/algorithms/algorithm.h, we could have
> 
> namespace libcamera {
> 
> namespace ipa::rkisp1 {
> 
> using Algorithm = libcamera::ipa::Algorithm<IPAContext, rkisp1_params_cfg, rkisp1_stat_buffer>;
> 
> } /* namespace ipa::rkisp1 */
> 
> } /* namespace libcamera */
> 
> Good/bad idea ?

It is obviously a good idea :-). Though, should it be done in this 
series, or in a dedicated one on top maybe ?

> 
>> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
>> new file mode 100644
>> index 00000000..d98f77e2
>> --- /dev/null
>> +++ b/src/ipa/rkisp1/algorithms/meson.build
>> @@ -0,0 +1,5 @@
>> +# SPDX-License-Identifier: CC0-1.0
>> +
>> +rkisp1_ipa_algorithms = files([
>> +    'algorithm.cpp',
>> +])
>> diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build
>> index 3683c922..a6a4856e 100644
>> --- a/src/ipa/rkisp1/meson.build
>> +++ b/src/ipa/rkisp1/meson.build
>> @@ -1,4 +1,5 @@
>>   # SPDX-License-Identifier: CC0-1.0
> 
> Missing blank line.
> 
>> +subdir('algorithms')
>>   
>>   ipa_name = 'ipa_rkisp1'
>>   
>> @@ -7,6 +8,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 e0933e22..ddddd52d 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([[maybe_unused]] const IPASettings &settings,
>