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

[Andrey Konovalov]

Hi Hans,

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

Yes, I think squashing this change (including making the gamma table 1024 entries) and the missing parenthesis fix
into the original commit is the way to go.

Thanks,
Andrey

> Regards,
> 
> Hans
> 
> 
>