[PATCH v3 06/16] libcamera: software_isp: Add SwStatsCpu class

[Hans de Goede]

Hi,

Thank you for the review.

One kind request for future reviews can you please remove
(<snip>) parts of the patch which you are not replying too ?

Quoting the entire patch makes it somewhat hard to
find the bits which you are actually commenting on.


On 2/15/24 15:20, Milan Zamazal wrote:
> Hans de Goede <hdegoede at redhat.com> writes:

<snip>

>> --- /dev/null
>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
>> @@ -0,0 +1,208 @@

<snip>

>> +/**
>> + * \class SwStatsCpu
>> + * \brief Class for gathering statistics on the CPU
>> + *
>> + * CPU based software ISP statistics implementation.
>> + *
>> + * This class offers a configure function + functions to gather statistics
>> + * on a line by line basis. This allows CPU based software debayering to
>> + * interlace debayering and statistics gathering on a line by line bases
>                                                                       ^^^^^
> 
> basis?

Ack, fixed in my personal tree now.

<snip>

>> +/**
>> + * \brief Configure the statistics object for the passed in input format.
>> + * \param[in] inputCfg The input format
>> + *
>> + * \return 0 on success, a negative errno value on failure
>> + */
>> +int SwStatsCpu::configure(const StreamConfiguration &inputCfg)
>> +{
>> +	BayerFormat bayerFormat =
>> +		BayerFormat::fromPixelFormat(inputCfg.pixelFormat);
>> +
>> +	if (bayerFormat.bitDepth == 10 &&
>> +	    bayerFormat.packing == BayerFormat::Packing::CSI2) {
>> +		patternSize_.height = 2;
>> +		patternSize_.width = 4; /* 5 bytes per *4* pixels */
>> +		/* Skip every 3th and 4th line, sample every other 2x2 block */
>> +		ySkipMask_ = 0x02;
>> +		xShift_ = 0;
>> +
>> +		switch (bayerFormat.order) {
>> +		case BayerFormat::BGGR:
>> +		case BayerFormat::GRBG:
>> +			stats0_ = (SwStatsCpu::statsProcessFn)&SwStatsCpu::statsBGGR10PLine0;
>                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Isn't the type cast superfluous?  (The same below.)

You would think so but gcc enforces this for c++ when assigning
function pointers. I tried removing the cast when reworking
Andrey's original code and gcc gave an error without the cast.


<snip>

>> +private:
>> +	/**
>> +	 * \brief Called when there is data to get statistics from.
>> +	 * \param[in] src The input data
>> +	 *
>> +	 * These functions take an array of (patternSize_.height + 1) src
>> +	 * pointers each pointing to a line in the source image. The middle
>> +	 * element of the array will point to the actual line being processed.
>> +	 * Earlier element(s) will point to the previous line(s) and later
>> +	 * element(s) to the next line(s).
> 
> And are there previous lines processed anywhere?

For debayering yes they are and since the swstats code is called from
the debayering code (to process the line while it is still in cache),
the debayer code is simply passing in the same array of line pointers
it gets passed. This is the main reason why the swstats code processLine
function prototype takes the same array of line pointers.

> If not, what happens to the
> very first line, must it be always excluded by the specified window?

In your reply to the DebayerCpu class you have some remarks about
the border which gets enforced there. That border ensures that the very
first line of the raw bayer data indeed is always excluded by the window.

Note the window inside the debayer code that is. The swstats window can
then be used to select a sub-area of the debayered area to gather stats
over, but atm it is simply set to the entire debayered area.

Regards,

Hans