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

[Andrey Konovalov]

Hi Hans,

On 18.12.2023 13:05, Hans de Goede wrote:
> 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

Ah OK, I've missed that integral promotion thing. Then the fix would be just adding the
missing brackets (not tested):

  -		g = g + g2 / 2;
  +		g = (g + g2) / 2;

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

OK. Then we could take this commit into the patch set. Having 1024 entries in the gamma-lookup table
would be a nice addition too.

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

OK

Thanks,
Andrey

> Regards,
> 
> Hans
>