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

[Milan Zamazal]

Hans de Goede <hdegoede at redhat.com> writes:

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

I see.

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

Alternatively, x in the loops could start with xShift_, resp. window_.x, to
avoid type mismatches and negative indexes in both debayering and stats.  But
OK, it's a matter of style and if maintainers are happy with any of them then I
am too. :-)

Regards,
Milan