[libcamera-devel] statistics cleanup was Re: ... libcamera: ipa: Soft IPA: add a Soft IPA implementation

[Hans de Goede]

Hi Pavel,

On 12/13/23 00:00, Pavel Machek wrote:
> Hi!
> 
>> ATM the statistics and debayer classes are mostly ready,
>> but I need to merge this with Andrey's v2 RFC and
>> then produce a clean new series from this
>> (assuming people like the approach).
> 
> Could I get you to apply this?

I can give it a try and assuming it does not cause
a performance regression I would be happy to
squash this in.

Which brings the question of crediting the change,
I can add a:

Suggested-by: Pavel Machek <pavel at ucw.cz>

for these bits, or:

Co-authored-by: Pavel Machek <pavel at ucw.cz>

but then I'm going to need a Signed-off-by from you
(just reply here with it).

Either way let me know which way you want to be credited?

The same question (how to credit) you also applies to
the suggested debayering changes ?

BTW did you see that I added a x_shift_ variable
to both the swstats and the debayer_cpu classes?

This allows shifting the input data start one
pixel to the right, changing e.g. RGGB to GRBG
so that we need less debayer functions.

Unfortunately this trick will not work for
10bpp packed formats because there we have:
MSB MSB MSB MSB LSBS in memory so we cannot
just shift things by skipping the first column
because then we end up with:
MSB MSB MSB LSBS MSB instead, which breaks
the debayer / stats functions expectations.

What we can do even with 10bpp packed is swap
line0 / line1 pointers, so that e.g. RGGB becomes
GBRG using the same stats / debayer functions.

Together with the x_shift_ thing this means that
for 8bits and 10 bits unpacked we will only need
1 line0 and line1 debayer function to cover all
4 basic bayer variants.

Regards,

Hans




> diff --git a/src/libcamera/software_isp/swstats_linaro.cpp b/src/libcamera/software_isp/swstats_linaro.cpp
> index 0323f45f..2f203b17 100644
> --- a/src/libcamera/software_isp/swstats_linaro.cpp
> +++ b/src/libcamera/software_isp/swstats_linaro.cpp
> @@ -38,80 +38,55 @@ static const unsigned int RED_Y_MUL = 77;		/* 0.30 * 256 */
>  static const unsigned int GREEN_Y_MUL = 150;		/* 0.59 * 256 */
>  static const unsigned int BLUE_Y_MUL = 29;		/* 0.11 * 256 */
>  
> -/*
> - * These need to be macros because it accesses a whole bunch of local
> - * variables (and copy and pasting this x times is undesirable)
> - */
> -#define SWISP_LINARO_START_LINE_STATS()			\
> -	uint8_t r, g, b;				\
> -	unsigned int y_val;				\
> -							\
> -	unsigned int sumR = 0;				\
> -	unsigned int sumG = 0;				\
> -	unsigned int sumB = 0;				\
> -							\
> -	unsigned int bright_sum = 0;			\
> -	unsigned int too_bright_sum = 0;
> -
> -#define SWISP_LINARO_ACCUMULATE_LINE_STATS()		\
> -	sumR += r;					\
> -	sumG += g;					\
> -	sumB += b;					\
> -							\
> -	y_val = r * RED_Y_MUL;				\
> -	y_val += g * GREEN_Y_MUL;			\
> -	y_val += b * BLUE_Y_MUL;			\
> -	if (y_val > BRIGHT_LVL) ++bright_sum;		\
> -	if (y_val > TOO_BRIGHT_LVL) ++too_bright_sum;
> -
> -#define SWISP_LINARO_FINISH_LINE_STATS()		\
> -	stats_.sumR_ += sumR;				\
> -	stats_.sumG_ += sumG;				\
> -	stats_.sumB_ += sumB;				\
> -							\
> -	bright_sum_ += bright_sum;			\
> -	too_bright_sum_ += too_bright_sum;
> -
> -void SwStatsLinaro::statsBGGR10PLine(const uint8_t *src0, const uint8_t *src1)
> +static void inline statsBayer(const int width, const uint8_t *src0, const uint8_t *src1, bool bggr, unsigned int &bright_sum, unsigned int &too_bright_sum, StatsLinaro &stats)
>  {
> -	const int width_in_bytes = width_ * 5 / 4;
> -	uint8_t g2;
> +	const int width_in_bytes = width * 5 / 4;
> +	uint8_t r, g, b, g2;
> +	unsigned int y_val;
>  
> -	SWISP_LINARO_START_LINE_STATS()
> +	stats.sumR_ = 0;
> +	stats.sumG_ = 0;
> +	stats.sumB_ = 0;
>  
> -	for (int x = 0; x < width_in_bytes; x += 5) {
> -		/* BGGR */
> -		b  = src0[x];
> -		g  = src0[x + 1];
> -		g2 = src1[x];
> -		r  = src1[x + 1];
> +	bright_sum = 0;
> +	too_bright_sum = 0;
>  
> +	for (int x = 0; x < width_in_bytes; x += 5) {
> +		if (bggr) {
> +			/* BGGR */
> +			b  = src0[x];
> +			g  = src0[x + 1];
> +			g2 = src1[x];
> +			r  = src1[x + 1];
> +		} else {
> +			/* GBRG */
> +			g  = src0[x];
> +			b  = src0[x + 1];
> +			r  = src1[x];
> +			g2 = src1[x + 1];
> +		}
>  		g = g + g2 / 2;
>  
> -		SWISP_LINARO_ACCUMULATE_LINE_STATS()
> +		stats.sumR_ += r;
> +		stats.sumG_ += g;
> +		stats.sumB_ += b;
> +
> +		y_val = r * RED_Y_MUL;
> +		y_val += g * GREEN_Y_MUL;
> +		y_val += b * BLUE_Y_MUL;
> +		if (y_val > BRIGHT_LVL) ++bright_sum;
> +		if (y_val > TOO_BRIGHT_LVL) ++too_bright_sum;
>  	}
> -	SWISP_LINARO_FINISH_LINE_STATS()
>  }
>  
> -void SwStatsLinaro::statsGBRG10PLine(const uint8_t *src0, const uint8_t *src1)
> +void SwStatsLinaro::statsBGGR10PLine(const uint8_t *src0, const uint8_t *src1)
>  {
> -	const int width_in_bytes = width_ * 5 / 4;
> -	uint8_t g2;
> -
> -	SWISP_LINARO_START_LINE_STATS()
> -
> -	for (int x = 0; x < width_in_bytes; x += 5) {
> -		/* GBRG */
> -		g  = src0[x];
> -		b  = src0[x + 1];
> -		r  = src1[x];
> -		g2 = src1[x + 1];
> -
> -		g = g + g2 / 2;
> +	statsBayer(width_, src0, src1, true, bright_sum_, too_bright_sum_, stats_);
> +}
>  
> -		SWISP_LINARO_ACCUMULATE_LINE_STATS()
> -	}
> -	SWISP_LINARO_FINISH_LINE_STATS()
> +void SwStatsLinaro::statsGBRG10PLine(const uint8_t *src0, const uint8_t *src1)
> +{
> +	statsBayer(width_, src0, src1, false, bright_sum_, too_bright_sum_, stats_);
>  }
>  
>  void SwStatsLinaro::statsBGGR10PLine0(const uint8_t *src, unsigned int stride)
>