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

[Jean-Michel Hautbois]

Hi !

On 08/07/2021 13:36, Laurent Pinchart wrote:
> 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.
> 

OK :-).

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

Those structures will be used with the Metadata patch, as each algo (and
IPAIPU3) needs to know the layout of the data (passing the awb results
from awb to agc is using this AwbStatus).
This patch will be moved after the Metadata introduction in v2 and it
should have more sense then ;-).

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

TODO: split the headers into include/ :-)

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

TODO: documentation, documentation, documentation and documentation !

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

I must confess I used the name from RPi :-).
It is indeed a status, and can be used to store previous gains and
current gains.
In the RPi awb algorithm (we don't have that part yet) the results are
store in prev_sync_results_, sync_results_ and async_results_ and each
of them is rolling between calls.
prev_sync_results_ is a filtered sync_results_ through an IIR and
sync_results_ will have a new value at the next frame, calculated in
async_results_ :-).

In Awb::Prepare() the gains are set in the metadata object with:
image_metadata->Set("awb.status", prev_sync_results_);

The structure can be renamed AwbResults though ;-) as all the variables
are results... :-).

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