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

[Hans de Goede]

Hi,

On 12/15/23 01:34, Bryan O'Donoghue wrote:
> On 14/12/2023 16:25, Hans de Goede wrote:
>> Hi,
>>
>> On 12/14/23 17:02, Hans de Goede wrote:
>>> Hi Bryan,
>>>
>>> On 12/14/23 16:16, Bryan O'Donoghue wrote:
>>>> On 14/12/2023 14:47, Hans de Goede wrote:
>>>
>>> <snip>
>>>
>>>>> 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%.
>>>> Seems to be eating alot more cycles
>>>
>>> Ok, that is no good.
>>>
>>> Can you try:
>>>
>>> https://gitlab.freedesktop.org/camera/libcamera-softisp/-/commits/SoftwareISP-v02-hans3
> 
> So this branch has the same performance issue 18fps, pinked-out display and ~ 100% on one of the cores.
> 
>>>
>>> And then start with checking out:
>>>
>>> 64c9bc0f55f459b9722fe78dfd35a07fb35d2a7b ("libcamera: software_isp: Add debayer-line callbacks for bayer patterns repeating every 4 lines")
> 
> I get 30fps here with 60-9x% cpu usage. Looks a bit dark but not pink.
> 
> Looks like a reasonable baseline.
> 
>>>
>>> that is basically softisp/SoftwareISP-v01-hans1 rebased on top of SoftwareISP-v02,
>>> so that should give you to same performance as before.
>>>
>>> First please check this indeed restores performance ?
>>>
>>> And then after that try newer commits from that branch in this order
>>> (I'm skipping commits which should not have a performance impact here):
>>>
>>> 7136d4d59aafb2564a24a2d4be773ca220257fdc
> 
> brighter but pink, lower cpu occupancy 40%, 30fps
> 
>>> e3c2a5931dd825c58f626da8c12429b70d20219b (not really expecting a performance impact from this one but maybe)
> 
> 
> Way brighter - too much IMO in comparsion to the previous, occupancy 100% fps 21.

Ok, so the CPU change here is weird. I know building with -O3 helps, but it looks
like there is still more CPU load gain to have.

Can you try adding this change on top of SoftwareISP-v04 ? :

diff --git a/include/libcamera/internal/software_isp/debayer.h b/include/libcamera/internal/software_isp/debayer.h
index 206bc2ac..e2a63f24 100644
--- a/include/libcamera/internal/software_isp/debayer.h
+++ b/include/libcamera/internal/software_isp/debayer.h
@@ -77,10 +77,8 @@ public:
 			return {};
 		}
 
-		return SizeRange(Size(pattern_size.width, pattern_size.height),
-				 Size((inputSize.width - 2 * pattern_size.width) & ~(pattern_size.width - 1),
-				      (inputSize.height - 2 * pattern_size.height) & ~(pattern_size.height - 1)),
-				 pattern_size.width, pattern_size.height);
+		return SizeRange(Size((inputSize.width - 2 * pattern_size.width) & ~(pattern_size.width - 1),
+				      (inputSize.height - 2 * pattern_size.height) & ~(pattern_size.height - 1)));
 	}
 
 	Signal<FrameBuffer *> inputBufferReady;

That basically undoes e3c2a5931dd825c58f626da8c12429b70d20219b but then
the SoftwareISP-v04 equivalent of it.

I wonder if that will give you another CPU load drop :)

As for the way brighter that probably is the gamma correction which
I added which I've hardcoded to 0.5 which is probably way too much.
This change (also in the AWB debug patch I send) disables the gamma
correction:

diff --git a/src/libcamera/software_isp/swisp_simple.cpp b/src/libcamera/software_isp/swisp_simple.cpp
index ff05b6fe..0f9213cc 100644
--- a/src/libcamera/software_isp/swisp_simple.cpp
+++ b/src/libcamera/software_isp/swisp_simple.cpp
@@ -22,7 +22,7 @@
 namespace libcamera {
 
 SwIspSimple::SwIspSimple(PipelineHandler *pipe, const ControlInfoMap &sensorControls)
-	: SoftwareIsp(pipe, sensorControls), debayer_(nullptr), debayerParams_{256, 256, 0.5f}
+	: SoftwareIsp(pipe, sensorControls), debayer_(nullptr), debayerParams_{256, 256, 1.0f}
 {
 	std::unique_ptr<SwStatsCpu> stats;
 
Once we have the purple issue fixed I wouldn't mind getting some opinions on what is
a good gamma-correction default value. Note this is not entirely linear, 0.75
does much less (close to none) correction then 0.5, so you could e.g. try 0.6 0.65
but lets first fix the pink / purple haze issue.

Regards,

Hans