[PATCH v3 08/16] libcamera: software_isp: Add DebayerCpu class

[Hans de Goede]

Hi Stefan,

On 2/20/24 13:26, Stefan Klug wrote:
> Hi Andrei,
> 
> Am 20.02.24 um 11:54 schrieb Andrei Konovalov:
>> Hi Stefan,
>>
>> On 19.02.2024 20:09, Stefan Klug wrote:
>>> Hi Hans,
>>>
>>> thanks for your work on the SoftISP.
>>>
>>> Am 14.02.24 um 18:01 schrieb Hans de Goede:
>>>> Add CPU based debayering implementation. This initial implementation
>>>> only supports debayering packed 10 bits per pixel bayer data in
>>>> the 4 standard bayer orders.
>>>>
>>>> Doxygen documentation by Dennis Bonke.
>>>>
>>>> Tested-by: Bryan O'Donoghue <bryan.odonoghue at linaro.org> # sc8280xp Lenovo x13s
>>>> Tested-by: Pavel Machek <pavel at ucw.cz>
>>>> Reviewed-by: Pavel Machek <pavel at ucw.cz>
>>>> Co-developed-by: Dennis Bonke <admin at dennisbonke.com>
>>>> Signed-off-by: Dennis Bonke <admin at dennisbonke.com>
>>>> Co-developed-by: Andrey Konovalov <andrey.konovalov at linaro.org>
>>>> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
>>>> Co-developed-by: Pavel Machek <pavel at ucw.cz>
>>>> Signed-off-by: Pavel Machek <pavel at ucw.cz>
>>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>>> ---
>>>> Changes in v3:
>>>> - Move debayer_cpu.h to src/libcamera/software_isp/
>>>> - Move documentation to .cpp file
>>>> - Document how/why an array of src pointers is passed to
>>>>    the debayer functions
>>>> ---
>>>>   src/libcamera/software_isp/debayer_cpu.cpp | 619 +++++++++++++++++++++
>>>>   src/libcamera/software_isp/debayer_cpu.h   | 143 +++++
>>>>   src/libcamera/software_isp/meson.build     |   1 +
>>>>   3 files changed, 763 insertions(+)
>>>>   create mode 100644 src/libcamera/software_isp/debayer_cpu.cpp
>>>>   create mode 100644 src/libcamera/software_isp/debayer_cpu.h
>>>>
>>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>>>> new file mode 100644
>>>> index 00000000..53e90776
>>>> --- /dev/null
>>>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
>>>> @@ -0,0 +1,619 @@
>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>> +/*
>>>> + * Copyright (C) 2023, Linaro Ltd
>>>> + * Copyright (C) 2023, Red Hat Inc.
>>>> + *
>>>> + * Authors:
>>>> + * Hans de Goede <hdegoede at redhat.com>
>>>> + *
>>>> + * debayer_cpu.cpp - CPU based debayering class
>>>> + */
>>>> +
>>>> +#include "debayer_cpu.h"
>>>> +
>>>> +#include <math.h>
>>>> +#include <stdlib.h>
>>>> +#include <time.h>
>>>> +
>>>> +#include <libcamera/formats.h>
>>>> +
>>>> +#include "libcamera/internal/bayer_format.h"
>>>> +#include "libcamera/internal/framebuffer.h"
>>>> +#include "libcamera/internal/mapped_framebuffer.h"
>>>> +
>>>> +namespace libcamera {
>>>> +
>>>> +/**
>>>> + * \class DebayerCpu
>>>> + * \brief Class for debayering on the CPU
>>>> + *
>>>> + * Implementation for CPU based debayering
>>>> + */
>>>> +
>>>> +/**
>>>> + * \brief Constructs a DebayerCpu object.
>>>> + * \param[in] stats Pointer to the stats object to use.
>>>> + */
>>>> +DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
>>>
>>> I tried to use the ISP without statistics/regulation altogether and just set stats to a nullptr. This fails in a few places. IMHO it would improve flexibility & reusability to allow that.
>>> Attched is a patch with the modifications as I needed them anyways. Feel free to include them.
>>
>> Your patch itself looks OK for me (I haven't tested it though).
>>
>> But I am curious what is the reason for not using stats?
>> Leaving the debayer step only, one looses AWB (which doesn't need any particular
>> support from the hardware and prevents the typical raw bayer green tint) and AE/AGC (which only
>> needs at least one of the two camera sensor controls, and with wrong exposure the
>> image may loose the information, and this can't be fully compensated by post processing).
> 
> I was working on the camera sensor implementation in libcamera and needed to quickly display the debayered sensor image. No auto regulation should happen as I wanted to manually control gain/exposure. One could even expose manual whitebalance gains for usecases under known conditions. Having the SoftIsp as a modular playground for such cases is quite helpful.

I too have no objections against the patch, but I wonder if it is necessary to add
all the NULL pointer checks to the SoftIPA ? Would it not be better to just not create
the SoftIPA at all in this case ?

Note either way is fine, just wondering ...

Regards,

Hans