[PATCH v2 08/18] libcamera: software_isp: Add SwStatsCpu class

[Hans de Goede]

Hi,

On 1/23/24 21:04, Milan Zamazal wrote:
> Hans de Goede <hdegoede at redhat.com> writes:

<snip>

>> +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 */
> 
> The last two lines comments don't match the given values.
> They should be 0.587 * 256 and 0.114 * 256 to equal.

I've fixed the comment to use the usual 0.299, 0.587, 0, 114 values
now, thanks.

>> +#define SWISP_LINARO_START_LINE_STATS(pixel_t) \
>> +	pixel_t r, g, g2, b;                   \
>> +	unsigned int y_val;                    \
>> +                                               \
>> +	unsigned int sumR = 0;                 \
>> +	unsigned int sumG = 0;                 \
>> +	unsigned int sumB = 0;
>> +
>> +#define SWISP_LINARO_ACCUMULATE_LINE_STATS(div) \
>> +	sumR += r;                              \
>> +	sumG += g;                              \
>> +	sumB += b;                              \
>> +                                                \
>> +	y_val = r * RED_Y_MUL;                  \
>> +	y_val += g * GREEN_Y_MUL;               \
>> +	y_val += b * BLUE_Y_MUL;                \
>> +	stats_.y_histogram[y_val / (256 * 16 * (div))]++;
> 
> As the size of y_histogram (16) is used in multiple places, it should be a named constant.

Ack, fixed.

>> +#define SWISP_LINARO_FINISH_LINE_STATS() \
>> +	stats_.sumR_ += sumR;            \
>> +	stats_.sumG_ += sumG;            \
>> +	stats_.sumB_ += sumB;
>> +
>> +static inline __attribute__((always_inline)) void
>> +statsBayer10P(const int width, const uint8_t *src0, const uint8_t *src1, bool bggr, SwIspStats &stats_)
>> +{
>> +	const int width_in_bytes = width * 5 / 4;
>> +
>> +	SWISP_LINARO_START_LINE_STATS(uint8_t)
>> +
>> +	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(1)
>> +	}
> 
> Does this loop process only every other pixel?  If yes, probably nothing wrong
> with it for stats, but it deserves a comment.

Yes it processes only every other pixel I've added a comment for this.

> 
>> +	SWISP_LINARO_FINISH_LINE_STATS()
>> +}
>> +
>> +void SwStatsCpu::statsBGGR10PLine0(const uint8_t *src[])
>> +{
>> +	const uint8_t *src0 = src[1] + window_.x * 5 / 4;
>> +	const uint8_t *src1 = src[2] + window_.x * 5 / 4;
> 
> This is difficult to understand: why src[1], src[2] and not src[0], src[1]?

I have added the following comment to the statsProcessFn:

        /**
         * \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).
         *
         * See the documentation of DebayerCpu::debayerFn for more details.
         */
        typedef void (SwStatsCpu::*statsProcessFn)(const uint8_t *src[]);

And the following comment to DebayerCpu::debayerFn:

	/**
	 * \brief Called to debayer 1 line of Bayer input data to output format
	 * \param[out] dst Pointer to the start of the output line to write
	 * \param[in] src The input data
	 *
	 * Input data is an array of (patternSize_.height + 1) src
	 * pointers each pointing to a line in the Bayer source. 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).
	 *
	 * These functions take an array of src pointers, rather then
	 * a single src pointer + a stride for the source, so that when the src
	 * is slow uncached memory it can be copied to faster memory before
	 * debayering. Debayering a standard 2x2 Bayer pattern requires access
	 * to the previous and next src lines for interpolating the missing
	 * colors. To allow copying the src lines only once 3 buffers each
	 * holding a single line are used, re-using the oldest buffer for
	 * the next line and the pointers are swizzled so that:
	 * src[0] = previous-line, src[1] = currrent-line, src[2] = next-line.
	 * This way the 3 pointers passed to the debayer functions form
	 * a sliding window over the src avoiding the need to copy each
	 * line more then once.
	 *
	 * Similarly for bayer patterns which repeat every 4 lines, 5 src
	 * pointers are passed holding: src[0] = 2-lines-up, src[1] = 1-line-up
	 * src[2] = current-line, src[3] = 1-line-down, src[4] = 2-lines-down.
	 */
	typedef void (DebayerCpu::*debayerFn)(uint8_t *dst, const uint8_t *src[]);

Regards,

Hans