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

[Hans de Goede]

Hi Andrey,

On 12/18/23 11:34, Andrey Konovalov wrote:
> 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;

Ah I missed that you also added the brackets / parenthesis. Yeah the missing parenthesis
are my bad, I'll squash a fix into the original commit for this.

Thank you for catching this.

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

Agreed. I think it would be best to squash this change (including making the gamma table 1024 entries)
into the original commit adding debayer_cpu.cpp is that ok with you ?

Regards,

Hans