[libcamera-devel] [PATCH v1 2/7] ipa: libipa: Create a common ISP header to store the structure types

[Kieran Bingham]

Hi JM,

On 28/06/2021 21:22, Jean-Michel Hautbois wrote:
> Each ISP may use the same AWB or AGC algorithms. In order to avoid
> duplicated code, create a header with the main structures used for now.


I'm not sure these are really 'isp' structures, and they are more
'statistics' strutures.

I'd be tempted to call this 'statistics.h' rather than isp.h ...
although - in fact one of the structures is the AwbStatus .. that's the
results of the algorithm isn't it ?

I wonder if the common structures which are specific to an algorithm
should be broken out to their own algorithm specific header.

I.e. anything common to all Awb should be libipa/awb.h ...


> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3_agc.h |   1 +
>  src/ipa/ipu3/ipu3_awb.h |  30 +----------
>  src/ipa/libipa/isp.h    | 110 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 112 insertions(+), 29 deletions(-)
>  create mode 100644 src/ipa/libipa/isp.h
> 
> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
> index 3deca3ae..6ca9af8e 100644
> --- a/src/ipa/ipu3/ipu3_agc.h
> +++ b/src/ipa/ipu3/ipu3_agc.h
> @@ -17,6 +17,7 @@
>  #include <libcamera/geometry.h>
>  
>  #include "libipa/algorithm.h"
> +#include "libipa/isp.h"
>  
>  namespace libcamera {
>  
> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
> index 122cf68c..6ae111fd 100644
> --- a/src/ipa/ipu3/ipu3_awb.h
> +++ b/src/ipa/ipu3/ipu3_awb.h
> @@ -14,6 +14,7 @@
>  #include <libcamera/geometry.h>
>  
>  #include "libipa/algorithm.h"
> +#include "libipa/isp.h"
>  
>  namespace libcamera {
>  
> @@ -42,35 +43,6 @@ public:
>  		unsigned char padding[3];
>  	} __attribute__((packed));
>  
> -	/* \todo Make these three structs available to all the ISPs ? */
> -	struct RGB {
> -		RGB(double _R = 0, double _G = 0, double _B = 0)
> -			: R(_R), G(_G), B(_B)
> -		{
> -		}
> -		double R, G, B;
> -		RGB &operator+=(RGB const &other)
> -		{
> -			R += other.R, G += other.G, B += other.B;
> -			return *this;
> -		}
> -	};
> -
> -	struct IspStatsRegion {
> -		unsigned int counted;
> -		unsigned int uncounted;
> -		unsigned long long rSum;
> -		unsigned long long gSum;
> -		unsigned long long bSum;
> -	};
> -
> -	struct AwbStatus {
> -		double temperatureK;
> -		double redGain;
> -		double greenGain;
> -		double blueGain;
> -	};
> -
>  private:
>  	void generateZones(std::vector<RGB> &zones);
>  	void generateAwbStats(const ipu3_uapi_stats_3a *stats);
> diff --git a/src/ipa/libipa/isp.h b/src/ipa/libipa/isp.h
> new file mode 100644
> index 00000000..a15803d6
> --- /dev/null
> +++ b/src/ipa/libipa/isp.h


I'm torn on the include locations too ...

globally, I would have expected libcamera headers to be under the root
include/ so that would make this include/ipa/ or include/libipa - but I
really think these headers are better kept close to the libipa - and
they are not generating an externally visible library anyway, so perhaps
keeping them local is good for now.



> @@ -0,0 +1,110 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Ideas On Board
> + *
> + * isp.h - ISP statistics interface
> + */
> +#ifndef __LIBCAMERA_IPA_LIBIPA_ISP_H__
> +#define __LIBCAMERA_IPA_LIBIPA_ISP_H__
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +/**
> + * \struct RGB
> + * \brief RGB
> + *
> + * \fn RGB::RGB
> + * \brief Construct a RGB structure
> + * \param[in] _R Red value to set, defaults to 0
> + * \param[in] _G Green value to set, defaults to 0
> + * \param[in] _B Blue value to set, defaults to 0
> + *
> + * \var RGB::R
> + * \brief Red value of the RGB structure
> + *
> + * \var RGB::G
> + * \brief Green value of the RGB structure
> + *
> + * \var RGB::B
> + * \brief Blue value of the RGB structure
> + *
> + * \fn RGB &RGB::operator+=(RGB const &other)
> + * \brief Add each RGB value to another RGB structure
> + * \param[in] other An RGB structure
> + * \return An RGB structure containing the added R, G and B values
> + */
> +struct RGB {
> +	RGB(double _R = 0, double _G = 0, double _B = 0)
> +		: R(_R), G(_G), B(_B)
> +	{
> +	}
> +	double R, G, B;
> +	RGB &operator+=(RGB const &other)
> +	{
> +		R += other.R, G += other.G, B += other.B;
> +		return *this;
> +	}
> +};
> +
> +/**
> + * \struct IspStatsRegion
> + * \brief RGB statistics for a given region
> + *
> + * The IspStatsRegion structure is intended to abstract the ISP specific
> + * statistics and use an agnostic algorithm to compute AWB.
> + *
> + * \var IspStatsRegion::counted
> + * \brief Number of pixels used to calculate the sums
> + *
> + * \var IspStatsRegion::uncounted
> + * \brief Remaining number of pixels in the region
> + *
> + * \var IspStatsRegion::rSum
> + * \brief Sum of the red values in the region
> + *
> + * \var IspStatsRegion::gSum
> + * \brief Sum of the green values in the region
> + *
> + * \var IspStatsRegion::bSum
> + * \brief Sum of the blue values in the region
> + */
> +struct IspStatsRegion {

Is the 'Isp' prefix redundant here?

We generally prefer full names in the rest of the code base - so I think
this could end up being

struct StatisticsRegion

> +	unsigned int counted;
> +	unsigned int uncounted;
> +	unsigned long long rSum;
> +	unsigned long long gSum;
> +	unsigned long long bSum;
> +};
> +
> +/**
> + * \struct AwbStatus

The name 'Status' sounds odd here to me.

The Status of the AWB would be 'converged' or something stateful
wouldn't it?

Should the output of an algorithm be the Results ?

i.e. would this be better known as AwbResults ?


> + * \brief AWB parameters calculated
> + *
> + * The AwbStatus structure is intended to store the AWB
> + * parameters calculated by the algorithm
> + *
> + * \var AwbStatus::temperatureK
> + * \brief Color temperature calculated
> + *
> + * \var AwbStatus::redGain
> + * \brief Gain calculated for the red channel
> + *
> + * \var AwbStatus::greenGain
> + * \brief Gain calculated for the green channel
> + *
> + * \var AwbStatus::blueGain
> + * \brief Gain calculated for the blue channel
> + */
> +struct AwbStatus {
> +	double temperatureK;
> +	double redGain;
> +	double greenGain;
> +	double blueGain;
> +};
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPA_LIBIPA_ISP_H__ */
>