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

[Bryan O'Donoghue]

On 17/12/2023 20: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.
> 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.
> 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.
> 
> Thanks,
> Andrey
> 
> [1] 
> https://gitlab.freedesktop.org/camera/libcamera-softisp/-/commits/SoftwareISP-v04-hans1--ynk1


Yep, that branch fixes for me.

BTW just to answer the earlier q from Hans, Bayer order is GRBG - 
driver/media/i2c/ov5675.c

---
bod