[PATCH v3 13/16] libcamera: swstats_cpu: Add support for 8, 10 and 12 bpp unpacked bayer input

[Hans de Goede]

Hi,

On 2/16/24 11:11, Milan Zamazal wrote:
> Hans de Goede <hdegoede at redhat.com> writes:
> 
>> Add support for 8, 10 and 12 bpp unpacked bayer input for all 4 standard
>> bayer orders.
>>
>> Tested-by: Bryan O'Donoghue <bryan.odonoghue at linaro.org> # sc8280xp Lenovo x13s
>> Tested-by: Pavel Machek <pavel at ucw.cz>
>> Reviewed-by: Pavel Machek <pavel at ucw.cz>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>> ---
>>  src/libcamera/software_isp/swstats_cpu.cpp | 128 +++++++++++++++++++++
>>  src/libcamera/software_isp/swstats_cpu.h   |   9 ++
>>  2 files changed, 137 insertions(+)
>>
>> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
>> index a3a2eb94..c7e85f2e 100644
>> --- a/src/libcamera/software_isp/swstats_cpu.cpp
>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
>> @@ -71,6 +71,83 @@ static const unsigned int kBlueYMul = 29; /* 0.114 * 256 */
>>  	stats_.sumG_ += sumG;       \
>>  	stats_.sumB_ += sumB;
>>  
>> +void SwStatsCpu::statsBGGR8Line0(const uint8_t *src[])
>> +{
>> +	const uint8_t *src0 = src[1] + window_.x;
>> +	const uint8_t *src1 = src[2] + window_.x;
>> +
>> +	SWSTATS_START_LINE_STATS(uint8_t)
>> +
>> +	if (swapLines_)
>> +		std::swap(src0, src1);
>> +
>> +	/* x += 4 sample every other 2x2 block */
>> +	for (int x = 0; x < (int)window_.width; x += 4) {
> 
> Why not `unsigned int x = 0'?  (The same at the other similar places.)

This mirrors the DebayerCpu code which has e.g.:

        for (int x = 0; x < (int)window_.width;) {
                /*
                 * GBGR line pixel 0: GBGRG
                 *                    IGIGI
                 *                    GRGBG
                 *                    IGIGI
                 *                    GBGRG
                 */
                *dst++ = blue_[curr[x + 1] / 4];
                *dst++ = green_[curr[x] / 4];
                *dst++ = red_[curr[x - 1] / 4];
                x++;

Notice the curr[x - 1] that (x - 1) will become 2^32 - 1 as
a positive array index, rather then a negative array index
of - 1 if x is unsigned.

Admittedly it could be unsigned in the swstats case since
there is no [x - 1] usage there. But the Debayer example
shows why making x unsigned can be troublesome so I would
rather keep it signed.

Regards,

Hans