[libcamera-devel] [PATCH v5 06/11] ipa: libipa: Introduce Algorithm class template

[Umang Jain]

Hi JM,

Patches seems okay,

On 11/25/21 11:12 AM, 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. While at
> it, fix the IPU3::Algorithm::Awb documentation.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> ---
> v4: use #pragma once
> ---
>   src/ipa/ipu3/algorithms/algorithm.h           | 12 ++----
>   src/ipa/ipu3/algorithms/awb.cpp               |  9 +++++
>   src/ipa/ipu3/algorithms/meson.build           |  1 -
>   .../{ipu3/algorithms => libipa}/algorithm.cpp | 38 ++++++++-----------
>   src/ipa/libipa/algorithm.h                    | 38 +++++++++++++++++++
>   src/ipa/libipa/meson.build                    |  1 +
>   6 files changed, 67 insertions(+), 32 deletions(-)
>   rename src/ipa/{ipu3/algorithms => libipa}/algorithm.cpp (75%)
>   create mode 100644 src/ipa/libipa/algorithm.h
>
> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h
> index 16310ab1..d2eecc78 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/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp
> index c7bcb20e..1dc27fc9 100644
> --- a/src/ipa/ipu3/algorithms/awb.cpp
> +++ b/src/ipa/ipu3/algorithms/awb.cpp
> @@ -193,6 +193,9 @@ Awb::Awb()
>   
>   Awb::~Awb() = default;
>   
> +/**
> + * \copydoc libcamera::ipa::Algorithm::configure
> + */
>   int Awb::configure(IPAContext &context,
>   		   [[maybe_unused]] const IPAConfigInfo &configInfo)
>   {
> @@ -373,6 +376,9 @@ void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
>   	}
>   }
>   
> +/**
> + * \copydoc libcamera::ipa::Algorithm::process
> + */
>   void Awb::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>   {
>   	calculateWBGains(stats);
> @@ -394,6 +400,9 @@ constexpr uint16_t Awb::threshold(float value)
>   	return value * 8191;
>   }
>   
> +/**
> + * \copydoc libcamera::ipa::Algorithm::prepare
> + */
>   void Awb::prepare(IPAContext &context, ipu3_uapi_params *params)
>   {
>   	/*
> 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 75%
> rename from src/ipa/ipu3/algorithms/algorithm.cpp
> rename to src/ipa/libipa/algorithm.cpp
> index 3e7e3018..398d5372 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,15 @@
>   
>   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
> + * \tparam Context The type of shared IPA context
> + * \tparam Config The type of the IPA configuration data
> + * \tparam Params The type of the ISP specific parameters
> + * \tparam Stats The type of the IPA statistics and ISP results
>    *
>    * The Algorithm class defines a standard interface for IPA algorithms. By
>    * abstracting algorithms, it makes possible the implementation of generic code
> @@ -26,6 +30,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 +44,30 @@ 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
> + * 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 +89,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..766aee5d
> --- /dev/null
> +++ b/src/ipa/libipa/algorithm.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Ideas On Board


I think Oy is missing here at the end, that;s what I see in other 
samples in the code base

Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

> + *
> + * algorithm.h - ISP control algorithm interface
> + */
> +#pragma once
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +template<typename Context, typename Config, typename Params, typename Stats>
> +class Algorithm
> +{
> +public:
> +	virtual ~Algorithm() {}
> +
> +	virtual int configure([[maybe_unused]] Context &context,
> +			      [[maybe_unused]] const Config &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 */
> 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'
>   ])