[libcamera-devel] [PATCH v2 07/18] libcamera: software_isp: Add SwStats base class

[Hans de Goede]

Hi Laurent,

On 1/23/24 17:46, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Sat, Jan 13, 2024 at 03:22:07PM +0100, Hans de Goede wrote:
>> Add a virtual base class for CPU based software statistics gathering
>> implementations.
>>
>> The idea is for the implementations to offer a configure function +
>> functions to gather statistics on a line by line basis. This allows
>> CPU based software debayering to call into interlace debayering and
>> statistics gathering on a line by line bases while the input data
>> is still hot in the cache.
> 
> To you expect multiple implementation to co-exist ? It seems simpler to
> me not to bother with a base class while we have a single
> implementation, especially given that I have fairly little visibility on
> what different implementations would look like and how they would
> coexist.

I'm fine with dropping the base class. But Bryan mentioned that
he actually plans to move some more common code there for the GLES
work he is doing.

So for posting v3 of this upstream I'm going to keep the base-class
around. We can always drop it later.

<snip>

>> new file mode 100644
>> index 00000000..1c43acc4
>> --- /dev/null
>> +++ b/include/libcamera/internal/software_isp/meson.build
>> @@ -0,0 +1,6 @@
>> +# SPDX-License-Identifier: CC0-1.0
>> +
>> +libcamera_internal_headers += files([
>> +    'swisp_stats.h',
>> +    'swstats.h',
>> +])
>> diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h
>> new file mode 100644
>> index 00000000..07ba7d6a
>> --- /dev/null
>> +++ b/include/libcamera/internal/software_isp/swisp_stats.h
>> @@ -0,0 +1,34 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2023, Linaro Ltd
>> + *
>> + * swisp_stats.h - Statistics data format used by the software ISP and software IPA
>> + */
>> +
>> +#pragma once
>> +
>> +namespace libcamera {
>> +
>> +/**
>> + * \brief Struct that holds the statistics for the Software ISP.
>> + */
>> +struct SwIspStats {
>> +	/**
>> +	 * \brief Holds the sum of all sampled red pixels.
>> +	 */
>> +	unsigned long sumR_;
>> +	/**
>> +	 * \brief Holds the sum of all sampled green pixels.
>> +	 */
>> +	unsigned long sumG_;
>> +	/**
>> +	 * \brief Holds the sum of all sampled blue pixels.
>> +	 */
>> +	unsigned long sumB_;
>> +	/**
>> +	 * \brief A histogram of luminance values.
>> +	 */
>> +	unsigned int y_histogram[16];
> 
> s/y_histogram/yHistogram/
> 
> or just histogram, if there's only one.
> 
>> +};
> 
> I assume this will be shared between the soft ISP and its IPA module, so
> it seems fine to have it here.

Correct this will be shared between the soft ISP and its IPA module.


> 
>> +
>> +} /* namespace libcamera */
>> diff --git a/include/libcamera/internal/software_isp/swstats.h b/include/libcamera/internal/software_isp/swstats.h
>> new file mode 100644
>> index 00000000..dcac7064
>> --- /dev/null
>> +++ b/include/libcamera/internal/software_isp/swstats.h
>> @@ -0,0 +1,215 @@
>> +/* 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> 
>> + *
>> + * swstats.h - software statistics base class
>> + */
>> +
>> +#pragma once
>> +
>> +#include <stdint.h>
>> +
>> +#include <libcamera/base/log.h>
>> +#include <libcamera/base/signal.h>
>> +
>> +#include <libcamera/geometry.h>
>> +
>> +namespace libcamera {
>> +
>> +class PixelFormat;
>> +struct SharedFD;
>> +struct StreamConfiguration;
>> +
>> +LOG_DECLARE_CATEGORY(SwStats)
>> +
>> +/**
>> + * \class SwStats
>> + * \brief Base class for the software ISP statistics.
>> + *
>> + * Base class for the software ISP statistics.
>> + */
> 
> This, however, seems internaly to the soft ISP implementation. I would
> move it to src/libcamera/software_isp/.

Do you mean move the entire .h file to src/libcamera/software_isp/ ?

With the exception of the pipeline handlers there are currently
no .h files under src/libcamera and if we allow .h files under
src/libcamera then why have include/libcamera/internal/ ?

I guess that include/libcamera/internal/ is for shared internal
headers and internal headers used only from 1 dir should go into
that dir? I'll move the .h file into the src/libcamera/software_isp/
assuming that this interpretation is the right one...

I have also applied the same change to swstats_cpu.h and
debayer[_cpu].h . Note this also required the following:

git mv src/libcamera/software_isp.cpp  src/libcamera/software_isp/

So that software_isp.cpp can still include these, without having
to resort to:

#include "software_isp/debayer_cpu.h"

which seems undesirable.

> Documentation goes to the .cpp file.

Ack, done.

<snip>

>> +	/**
>> +	 * \brief Skip lines where this bitmask is set in y.
>> +	 */
>> +	unsigned int y_skip_mask_;
> 
> ySkipMask_
> 
> Same where applicable.

Ack, done.

<snip>

>> diff --git a/src/libcamera/software_isp/swstats.cpp b/src/libcamera/software_isp/swstats.cpp
>> new file mode 100644
>> index 00000000..e65a7ada
>> --- /dev/null
>> +++ b/src/libcamera/software_isp/swstats.cpp
>> @@ -0,0 +1,22 @@
>> +/* 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> 
>> + *
>> + * swstats.cpp - software statistics base class
>> + */
>> +
>> +#include "libcamera/internal/software_isp/swstats.h"
>> +
>> +namespace libcamera {
>> +
>> +LOG_DEFINE_CATEGORY(SwStats)
>> +
>> +SwStats::~SwStats()
>> +{
>> +}
> 
> If it's a pure virtual function I'm surprised the compiler allows you to
> define a destructor.

Not only does it allow it, it requires it. AFAICT the destructor
of the derived class always chains up to the base-class destructor,
so there must be one defined.

Note I'm new (or revisiting from 20y ago) to c++ ...

Regards,

Hans