[libcamera-devel] [PATCH 00/11] libcamera: introduce Software ISP and Software IPA

[Hans de Goede]

Hi Andrey,

On 12/17/23 21:23, Andrey Konovalov wrote:
> Hi All,
> 
> On 14.12.2023 18:16, Bryan O'Donoghue wrote:
>> On 14/12/2023 14:47, Hans de Goede wrote:
>>> Hi Bryan,
>>>
>>> On 12/14/23 15:40, Bryan O'Donoghue wrote:
>>>> On 14/12/2023 12:13, Hans de Goede wrote:
>>>>> Hi All,
>>>>>
>>>>> Here is a new, now non RFC because believed to be mostly ready
>>>>> for merging, version of the Software ISP work.
>>>>
>>>>
>>>> Thanks for sending this out.
>>>
>>> You're welcome.
>>>
>>>> One thing I'm noticing when compiling is a load of warnings about undocumented variables.
>>> <snip>
>>>> Not sure if you or Andrey see that yourselves but one to be aware of for V2.
>>>
>>> Quoting from the cover-letter:
>>>
>>> """
>>> Known open items:
>>> - The AWB red/blue gain calculations needs to be moved to the IPA
>>>    and IPA then needs to pass a DebayerParams struct back to the
>>>    SwIsp
>>> - Properly document all methods / attributed for doxygen
>>> """
>>>
>>> So this is a known issue.
>>>
>>> BTW I'm currently working on adding support for 10bpp unpacked
>>> bayer and while working on this I noticed a small bug in the swstats
>>> code, you may want to squash in this fix:
>>
>> Applied that change.
>>
>> I'm comparing to your earlier branch
>>
>> commit ae92fa44991ded151c63d5d202efdacc9d640aff (HEAD -> SoftwareISP-v01-hans1, softisp/SoftwareISP-v01-hans1)
>> Author: Hans de Goede <hdegoede at redhat.com>
>> Date:   Thu Nov 30 20:13:29 2023 +0100
>>
>>
>> On the earlier branch I get 30fps @ 68% CPU usage. On this branch I'm getting 18fps @ 100%.
>>
>> Old branch:
>> https://gitlab.freedesktop.org/camera/libcamera-softisp/-/blob/bod/SoftwareISP-v04-review/v1-image.jpg
>>
>> New branch:
>> https://gitlab.freedesktop.org/camera/libcamera-softisp/-/blob/bod/SoftwareISP-v04-review/v1-posted-image.jpg
>>
>> Seems to be eating alot more cycles and producing a more pinkish result on my hw.
> 
> RE the more pinkish result.
> 
> This is a bug in swstats_cpu.cpp, which in fact broke both AWB and AGC (the exposure/gain could only increase, and
> never go back down).
> 
> I've pushed a branch [1] with the three commits on top of SoftwareISP-v04-hans1 by Hans:
> 
> 6baa50e7 ("libcamera: software_isp: fix statistics calculations") is the fix to the "pinkish" image issue.
>     Needs to be included into the patch set.

For those reading along, this is the fix:

--- a/src/libcamera/software_isp/swstats_cpu.cpp
+++ b/src/libcamera/software_isp/swstats_cpu.cpp
@@ -63,7 +63,7 @@ statsBayer10P(const int width, const uint8_t *src0, const uint8_t *src1, bool bg
 			r  = src1[x];
 			g2 = src1[x + 1];
 		}
-		g = g + g2 / 2;
+		g = ((unsigned int)g + g2) / 2;
 
 		sumR += r;
 		sumG += g;


Hmm, that smells like a compiler bug TBH. g and g2 are both uint8_t, so they have a range
of 0-255 so C/C++ integer promotion says these should both be promoted to an int
(0-255 fits in in an int) before doing the + operation:

https://en.cppreference.com/w/cpp/language/implicit_conversion#Integral_promotion

So "g + g2 / 2" should be done using all int values (so no overflow) and then
the resulting int which is in the 0-255 range again gets stored in uint8_t g, which just
takes the low 8 bits which should contain the right value.

I guess the compiler may be changing this to:

	g += g2;
	g /= 2;

but that is wrong and a compiler bug. With all that said I'm fine with adding the explicit
cast to (unsigned int), but unless I am missing something here this really does feel
like a compiler bug.

> f8efb3be ("libcamera: software_isp: fix AWB algorithm not to increase luminance") makes the luminance not to change
>     due to the colour gains applied. This improves the image a bit (makes it a bit less bright), but the effect
>     is relatively small - within 10% depending on the light and the objects in the particular image. This change
>     increases the load on CPU, so I am not sure if this image improvement is worth this higher CPU utilization.

Interesting since we have r g b lookup tables already the extra CPU load is only
in generating the green table
once per frame, the load of that should be negligible.

And with the separate gamma lookup we can then also give the gamma-lookup table a bit
more precision giving it 1024 entries. The impact of this on performance should be
minimal.

> f5906f15 ("[DNI] libcamera: software_isp: print colour gains to the log") prints the colour gains to the console
>     so that one could see the effect of the previous patch - without it the green gain would always be 256, and the
>     other two gains would increase in the same proportion. E.g. "gains R/G/B = 253/240/406" would become
>     "gains R/G/B = 270/256/433" without commit f8efb3be.

I think we probably want something like this (but then in the IPA once the AWB calculations
have moved there) and then using a log-level of debug. Doing a debug log once per frame
should have no performance impact when debug logging is disabled.

Regards,

Hans