[libcamera-devel] [PATCH v1 4/8] ipa: ipu3: Introduce the Algorithm class
Jean-Michel Hautbois
jeanmichel.hautbois at ideasonboard.com
Mon Nov 22 15:43:44 CET 2021
On 22/11/2021 15:27, Laurent Pinchart wrote:
> Hi Jean-Michel,
>
> On Mon, Nov 22, 2021 at 03:18:48PM +0100, Jean-Michel Hautbois wrote:
>> On 19/11/2021 14:22, Laurent Pinchart wrote:
>>> 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 ?
>
> It can be done on top, but it's nicer to review if it you do it as part
> of the series. I'd say it depends on the amount of work required. It
> doesn't seem too difficult to me, but I'm looking at the issue from a
> high level, I may be missing nasty details.
>
> One point that needs to be considered is if we can design an Algorithm
> class shared by IPA modules that will only differ in the data types it
> uses, or if functions will also need to be customized. Adding new
> IPA-specific functions is always possible through inheritance, but we
> need a common set of functions to make this worth it.
>
Well, for IPU3 and RkISP1 I think both will use the same functions and
we don't need more, afaik. I will try to implement it then, you made the
most difficult part I think ;-).
>>>> 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,
>
More information about the libcamera-devel
mailing list