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

[Laurent Pinchart]

Hello,

On Wed, Jul 07, 2021 at 01:23:22PM +0100, Kieran Bingham wrote:
> 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 ...

Agreed, ISP is a misnommer here. 'statistics' is better, shortening it
to 'stats' may be good in most (all ?) contexts to avoid too long names.

> 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 ...

Agreed too.

> > 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.

I would have expected include/ too. We already have headers in
src/ipa/libipa/ so the issue shouldn't be addressed in this patch, but I
expect libipa to turn into a shared object that will be installed as
part of libcamera, in which case headers should go to include/ (possibly
include/libcamera/libipa/, TBD).

> > @@ -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

And ? :-) What is this ? Is it a pixel ? An accumulator ? Why double
instead of fixed point ? All this needs to be explained.

> > + *
> > + * \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
> > + */

Doxygen documentation goes to .cpp files.

> > +struct RGB {
> > +	RGB(double _R = 0, double _G = 0, double _B = 0)
> > +		: R(_R), G(_G), B(_B)

Variables start with a lower-case letter, so that's r and _r (and so
on).

> > +	{
> > +	}
> > +	double R, G, B;

Blank line before and after, and it should go after all the functions.

> > +	RGB &operator+=(RGB const &other)
> > +	{
> > +		R += other.R, G += other.G, B += other.B;

One statement per line.

> > +		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.

It seems very, very ad-hoc, far from the level of genericity I would
expect from a generic type. Documentation needs to be expanded to
explain what this is.

> > + *
> > + * \var IspStatsRegion::counted
> > + * \brief Number of pixels used to calculate the sums
> > + *
> > + * \var IspStatsRegion::uncounted
> > + * \brief Remaining number of pixels in the region

Those two are extremely unclear as well.

> > + *
> > + * \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

Agreed overall, but for stats, I think spelling it in full all the time
will be painful.

> > +	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__ */

-- 
Regards,

Laurent Pinchart