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

19397 diff mbox series

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.

This base class also allows the user of an implementation to specify
a window over which to gather statistics instead of processing the
whole frame; and it allows the implementation to choose to only
process 1/2, 1/4th, etc. of the lines instead of processing all
lines (in the window) by setting y_skip_mask_ from configure().
Skipping columns is left up the line-processing functions provided
by the implementation.

Doxygen documentation by Dennis Bonke.

Co-authored-by: Dennis Bonke <admin@dennisbonke.com>
Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
Co-authored-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
Tested-by: Pavel Machek <pavel@ucw.cz>
---
 include/libcamera/internal/meson.build        |   1 +
 .../internal/software_isp/meson.build         |   6 +
 .../internal/software_isp/swisp_stats.h       |  34 +++
 .../libcamera/internal/software_isp/swstats.h | 215 ++++++++++++++++++
 src/libcamera/meson.build                     |   1 +
 src/libcamera/software_isp/meson.build        |   5 +
 src/libcamera/software_isp/swstats.cpp        |  22 ++
 7 files changed, 284 insertions(+)
 create mode 100644 include/libcamera/internal/software_isp/meson.build
 create mode 100644 include/libcamera/internal/software_isp/swisp_stats.h
 create mode 100644 include/libcamera/internal/software_isp/swstats.h
 create mode 100644 src/libcamera/software_isp/meson.build
 create mode 100644 src/libcamera/software_isp/swstats.cpp

Hans de Goede <hdegoede@redhat.com> writes:

> 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.
>
> This base class also allows the user of an implementation to specify
> a window over which to gather statistics instead of processing the
> whole frame; and it allows the implementation to choose to only
> process 1/2, 1/4th, etc. of the lines instead of processing all
> lines (in the window) by setting y_skip_mask_ from configure().
> Skipping columns is left up the line-processing functions provided
> by the implementation.
>
> Doxygen documentation by Dennis Bonke.
>
> Co-authored-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> Co-authored-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel@ucw.cz>
> ---
>  include/libcamera/internal/meson.build        |   1 +
>  .../internal/software_isp/meson.build         |   6 +
>  .../internal/software_isp/swisp_stats.h       |  34 +++
>  .../libcamera/internal/software_isp/swstats.h | 215 ++++++++++++++++++
>  src/libcamera/meson.build                     |   1 +
>  src/libcamera/software_isp/meson.build        |   5 +
>  src/libcamera/software_isp/swstats.cpp        |  22 ++
>  7 files changed, 284 insertions(+)
>  create mode 100644 include/libcamera/internal/software_isp/meson.build
>  create mode 100644 include/libcamera/internal/software_isp/swisp_stats.h
>  create mode 100644 include/libcamera/internal/software_isp/swstats.h
>  create mode 100644 src/libcamera/software_isp/meson.build
>  create mode 100644 src/libcamera/software_isp/swstats.cpp
>
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 1325941d..caa533c4 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -51,3 +51,4 @@ libcamera_internal_headers = files([
>  ])
>  
>  subdir('converter')
> +subdir('software_isp')
> diff --git a/include/libcamera/internal/software_isp/meson.build b/include/libcamera/internal/software_isp/meson.build
> 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];
> +};
> +
> +} /* 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@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.
> + */
> +class SwStats
> +{
> +public:
> +	virtual ~SwStats() = 0;
> +
> +	/**
> +	 * \brief Gets wether the statistics object is valid.
> +	 * 
> +	 * \return true if it's valid, false otherwise.
> +	 */
> +	virtual bool isValid() const = 0;
> +
> +	/**
> +	 * \brief Configure the statistics object for the passed in input format.
> +	 * \param[in] inputCfg The input format
> +	 *
> +	 * \return 0 on success, a negative errno value on failure.
> +	 */
> +	virtual int configure(const StreamConfiguration &inputCfg) = 0;
> +
> +	/**
> +	 * \brief Get the file descriptor for the statistics.
> +	 *
> +	 * \return the file descriptor
> +	 */
> +	virtual const SharedFD &getStatsFD() = 0;
> +
> +protected:
> +	/**
> +	 * \brief Called when there is data to get statistics from.
> +	 * \param[in] src The input data
> +	 */
> +	typedef void (SwStats::*statsProcessFn)(const uint8_t *src[]);
> +	/**
> +	 * \brief Called when the statistics gathering is done or when a new frame starts.
> +	 */
> +	typedef void (SwStats::*statsVoidFn)();
> +
> +	/* Variables set by configure(), used every line */
> +	/**
> +	 * \brief The function called when a line is ready for statistics processing.
> +	 *
> +	 * Used for line 0 and 1, repeating if there isn't a 3rd and a 4th line in the bayer order.
> +	 */
> +	statsProcessFn stats0_;
> +	/**
> +	 * \brief The function called when a line is ready for statistics processing.
> +	 *
> +	 * Used for line 3 and 4, only needed if the bayer order has 4 different lines.
> +	 */
> +	statsProcessFn stats2_;
> +
> +	/**
> +	 * \brief The memory used per pixel in bits.
> +	 */
> +	unsigned int bpp_;
> +	/**
> +	 * \brief Skip lines where this bitmask is set in y.
> +	 */
> +	unsigned int y_skip_mask_;
> +
> +	/**
> +	 * \brief Statistics window, set by setWindow(), used ever line.
> +	 */
> +	Rectangle window_;
> +
> +	/**
> +	 * \brief The function called at the start of a frame.
> +	 */
> +	statsVoidFn startFrame_;
> +	/**
> +	 * \brief The function called at the end of a frame.
> +	 */
> +	statsVoidFn finishFrame_;
> +	/**
> +	 * \brief The size of the bayer pattern.
> +	 */
> +	Size patternSize_;
> +	/**
> +	 * \brief The offset of x, applied to window_.x for bayer variants.
> +	 *
> +	 * This can either be 0 or 1.
> +	 */
> +	unsigned int x_shift_;
> +
> +public:
> +	/**
> +	 * \brief Get the pattern size.
> +	 *
> +	 * For some input-formats, e.g. Bayer data, processing is done multiple lines
> +	 * and/or columns at a time. Get width and height at which the (bayer) pattern
> +	 * repeats. Window values are rounded down to a multiple of this and the height
> +	 * also indicates if processLine2() should be called or not.
> +	 * This may only be called after a successful configure() call.
> +	 *
> +	 * \return the pattern size.
> +	 */
> +	const Size &patternSize() { return patternSize_; }
> +
> +	/**
> +	 * \brief Specify window coordinates over which to gather statistics.
> +	 * \param[in] window The window object.
> +	 */
> +	void setWindow(Rectangle window)
> +	{
> +		window_ = window;
> +
> +		window_.x &= ~(patternSize_.width - 1);
> +		window_.x += x_shift_;
> +		window_.y &= ~(patternSize_.height - 1);
> +
> +		/* width_ - x_shift_ to make sure the window fits */
> +		window_.width -= x_shift_;
> +		window_.width &= ~(patternSize_.width - 1);
> +		window_.height &= ~(patternSize_.height - 1);

These operations work correctly only if width & height are powers of 2, right?
Maybe this is satisfied for all Bayer patterns but I think such implicit
assumptions should be either not introduced or sufficiently visibly documented.

> +	}
> +
> +	/**
> +	 * \brief Reset state to start statistics gathering for a new frame.
> +	 * 
> +	 * This may only be called after a successful setWindow() call.
> +	 */
> +	void startFrame()
> +	{
> +		(this->*startFrame_)();
> +	}
> +
> +	/**
> +	 * \brief Process line 0.
> +	 * \param[in] y The y coordinate.
> +	 * \param[in] src The input data.
> +	 *
> +	 * This function processes line 0 for input formats with patternSize height == 1.
> +	 * It'll process line 0 and 1 for input formats with patternSize height >= 2.
> +	 * This function may only be called after a successful setWindow() call.
> +	 */
> +	void processLine0(unsigned int y, const uint8_t *src[])
> +	{
> +		if ((y & y_skip_mask_) || y < (unsigned int)window_.y ||
> +		    y >= (window_.y + window_.height))
> +			return;
> +
> +		(this->*stats0_)(src);
> +	}
> +
> +	/**
> +	 * \brief Process line 2 and 3.
> +	 * \param[in] y The y coordinate.
> +	 * \param[in] src The input data.
> +	 *
> +	 * This function processes line 2 and 3 for input formats with patternSize height == 4.
> +	 * This function may only be called after a successful setWindow() call.
> +	 */
> +	void processLine2(unsigned int y, const uint8_t *src[])
> +	{
> +		if ((y & y_skip_mask_) || y < (unsigned int)window_.y ||
> +		    y >= (window_.y + window_.height))
> +			return;
> +
> +		(this->*stats2_)(src);
> +	}
> +
> +	/**
> +	 * \brief Finish statistics calculation for the current frame.
> +	 * 
> +	 * This may only be called after a successful setWindow() call.
> +	 */
> +	void finishFrame()
> +	{
> +		(this->*finishFrame_)();
> +	}
> +
> +	/**
> +	 * \brief Signals that the statistics are ready.
> +	 *
> +	 * The int parameter isn't actually used.
> +	 */
> +	Signal<int> statsReady;
> +};
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 86494663..3d63e8a2 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -71,6 +71,7 @@ subdir('converter')
>  subdir('ipa')
>  subdir('pipeline')
>  subdir('proxy')
> +subdir('software_isp')
>  
>  null_dep = dependency('', required : false)
>  
> diff --git a/src/libcamera/software_isp/meson.build b/src/libcamera/software_isp/meson.build
> new file mode 100644
> index 00000000..9359075d
> --- /dev/null
> +++ b/src/libcamera/software_isp/meson.build
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: CC0-1.0
> +
> +libcamera_sources += files([
> +	'swstats.cpp',
> +])
> 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@redhat.com> 
> + *
> + * swstats.cpp - software statistics base class
> + */
> +
> +#include "libcamera/internal/software_isp/swstats.h"
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(SwStats)
> +
> +SwStats::~SwStats()
> +{
> +}
> +
> +} /* namespace libcamera */

Hi Hans

On Sat, Jan 13, 2024 at 03:22:07PM +0100, Hans de Goede via libcamera-devel 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.
>
> This base class also allows the user of an implementation to specify
> a window over which to gather statistics instead of processing the
> whole frame; and it allows the implementation to choose to only
> process 1/2, 1/4th, etc. of the lines instead of processing all
> lines (in the window) by setting y_skip_mask_ from configure().
> Skipping columns is left up the line-processing functions provided
> by the implementation.
>
> Doxygen documentation by Dennis Bonke.
>
> Co-authored-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> Co-authored-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel@ucw.cz>
> ---
>  include/libcamera/internal/meson.build        |   1 +
>  .../internal/software_isp/meson.build         |   6 +
>  .../internal/software_isp/swisp_stats.h       |  34 +++
>  .../libcamera/internal/software_isp/swstats.h | 215 ++++++++++++++++++
>  src/libcamera/meson.build                     |   1 +
>  src/libcamera/software_isp/meson.build        |   5 +
>  src/libcamera/software_isp/swstats.cpp        |  22 ++
>  7 files changed, 284 insertions(+)
>  create mode 100644 include/libcamera/internal/software_isp/meson.build
>  create mode 100644 include/libcamera/internal/software_isp/swisp_stats.h
>  create mode 100644 include/libcamera/internal/software_isp/swstats.h
>  create mode 100644 src/libcamera/software_isp/meson.build
>  create mode 100644 src/libcamera/software_isp/swstats.cpp
>
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 1325941d..caa533c4 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -51,3 +51,4 @@ libcamera_internal_headers = files([
>  ])
>
>  subdir('converter')
> +subdir('software_isp')
> diff --git a/include/libcamera/internal/software_isp/meson.build b/include/libcamera/internal/software_isp/meson.build
> 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];
> +};
> +
> +} /* 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@redhat.com>

Trailing whitespace at the end of the line

> + *
> + * 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.

No . at the end of \brief

> + *
> + * Base class for the software ISP statistics.
> + */
> +class SwStats
> +{
> +public:
> +	virtual ~SwStats() = 0;
> +
> +	/**
> +	 * \brief Gets wether the statistics object is valid.
> +	 *
> +	 * \return true if it's valid, false otherwise.
> +	 */
> +	virtual bool isValid() const = 0;
> +
> +	/**
> +	 * \brief Configure the statistics object for the passed in input format.
> +	 * \param[in] inputCfg The input format
> +	 *
> +	 * \return 0 on success, a negative errno value on failure.
> +	 */
> +	virtual int configure(const StreamConfiguration &inputCfg) = 0;
> +
> +	/**
> +	 * \brief Get the file descriptor for the statistics.
> +	 *
> +	 * \return the file descriptor
> +	 */
> +	virtual const SharedFD &getStatsFD() = 0;
> +
> +protected:
> +	/**
> +	 * \brief Called when there is data to get statistics from.
> +	 * \param[in] src The input data
> +	 */
> +	typedef void (SwStats::*statsProcessFn)(const uint8_t *src[]);

I'll see how this is used later, but passing in an unbound pointer to
memory seems suspicious. Span<> can help maybe or maybe just passing in a
size ?
> +	/**
> +	 * \brief Called when the statistics gathering is done or when a new frame starts.
> +	 */
> +	typedef void (SwStats::*statsVoidFn)();
> +
> +	/* Variables set by configure(), used every line */
> +	/**
> +	 * \brief The function called when a line is ready for statistics processing.
> +	 *
> +	 * Used for line 0 and 1, repeating if there isn't a 3rd and a 4th line in the bayer order.
> +	 */
> +	statsProcessFn stats0_;
> +	/**
> +	 * \brief The function called when a line is ready for statistics processing.
> +	 *
> +	 * Used for line 3 and 4, only needed if the bayer order has 4 different lines.
> +	 */
> +	statsProcessFn stats2_;

Is the idea that derived classes shall provide these ? Isn't a pure
virtual function better ?

...

Ok, I've now read the next patches, and you set stat0_ and stat2_
conditionally on the bitdepth

		switch (bayerFormat.bitDepth) {
		case 8:
			stats0_ = (SwStats::statsProcessFn)&SwStatsCpu::statsBGGR8Line0;
			return 0;
		case 10:
			stats0_ = (SwStats::statsProcessFn)&SwStatsCpu::statsBGGR10Line0;
			return 0;
		}

stats2_ seems not used in you implementation

This makes me wonder if the 8 and 10 bits implementation shouldn't
actually be two dervied classes, each one implementing their own
stats0() functions. It's indeed more verbose though...

> +
> +	/**
> +	 * \brief The memory used per pixel in bits.

Usually called 'bitdepth' in other parts of the library


> +	 */
> +	unsigned int bpp_;
> +	/**
> +	 * \brief Skip lines where this bitmask is set in y.
> +	 */
> +	unsigned int y_skip_mask_;
> +
> +	/**
> +	 * \brief Statistics window, set by setWindow(), used ever line.
> +	 */
> +	Rectangle window_;
> +
> +	/**
> +	 * \brief The function called at the start of a frame.
> +	 */
> +	statsVoidFn startFrame_;
> +	/**
> +	 * \brief The function called at the end of a frame.
> +	 */
> +	statsVoidFn finishFrame_;
> +	/**
> +	 * \brief The size of the bayer pattern.
> +	 */
> +	Size patternSize_;

Do you have a use case already for pattern sizes larger than the usual
2x2 ? Mostly out of curiosity

> +	/**
> +	 * \brief The offset of x, applied to window_.x for bayer variants.
> +	 *
> +	 * This can either be 0 or 1.
> +	 */
> +	unsigned int x_shift_;
> +
> +public:
> +	/**
> +	 * \brief Get the pattern size.
> +	 *
> +	 * For some input-formats, e.g. Bayer data, processing is done multiple lines
> +	 * and/or columns at a time. Get width and height at which the (bayer) pattern
> +	 * repeats. Window values are rounded down to a multiple of this and the height
> +	 * also indicates if processLine2() should be called or not.
> +	 * This may only be called after a successful configure() call.
> +	 *
> +	 * \return the pattern size.

And no . at the end of the line for \return too

> +	 */
> +	const Size &patternSize() { return patternSize_; }
> +
> +	/**
> +	 * \brief Specify window coordinates over which to gather statistics.
> +	 * \param[in] window The window object.
> +	 */
> +	void setWindow(Rectangle window)
> +	{
> +		window_ = window;
> +
> +		window_.x &= ~(patternSize_.width - 1);
> +		window_.x += x_shift_;
> +		window_.y &= ~(patternSize_.height - 1);
> +
> +		/* width_ - x_shift_ to make sure the window fits */
> +		window_.width -= x_shift_;
> +		window_.width &= ~(patternSize_.width - 1);
> +		window_.height &= ~(patternSize_.height - 1);
> +	}

The usage of x_shift (in conjunction with swap_lines (btw, we use
camelCase and not snake_case)) seems to depend on the fact the SwStatCpu
implementation uses a single function to deal with the 4 bayer
permutations. A different implementation might not use the same. I
wonder if all of this shouldn't be moved to SwStatCpu..

> +
> +	/**
> +	 * \brief Reset state to start statistics gathering for a new frame.
> +	 *
> +	 * This may only be called after a successful setWindow() call.
> +	 */
> +	void startFrame()
> +	{
> +		(this->*startFrame_)();
> +	}
> +
> +	/**
> +	 * \brief Process line 0.
> +	 * \param[in] y The y coordinate.
> +	 * \param[in] src The input data.
> +	 *
> +	 * This function processes line 0 for input formats with patternSize height == 1.
> +	 * It'll process line 0 and 1 for input formats with patternSize height >= 2.
> +	 * This function may only be called after a successful setWindow() call.
> +	 */
> +	void processLine0(unsigned int y, const uint8_t *src[])
> +	{
> +		if ((y & y_skip_mask_) || y < (unsigned int)window_.y ||
> +		    y >= (window_.y + window_.height))
> +			return;
> +
> +		(this->*stats0_)(src);
> +	}
> +
> +	/**
> +	 * \brief Process line 2 and 3.
> +	 * \param[in] y The y coordinate.
> +	 * \param[in] src The input data.
> +	 *
> +	 * This function processes line 2 and 3 for input formats with patternSize height == 4.
> +	 * This function may only be called after a successful setWindow() call.
> +	 */
> +	void processLine2(unsigned int y, const uint8_t *src[])
> +	{
> +		if ((y & y_skip_mask_) || y < (unsigned int)window_.y ||
> +		    y >= (window_.y + window_.height))
> +			return;
> +
> +		(this->*stats2_)(src);
> +	}
> +
> +	/**
> +	 * \brief Finish statistics calculation for the current frame.
> +	 *
> +	 * This may only be called after a successful setWindow() call.
> +	 */
> +	void finishFrame()
> +	{
> +		(this->*finishFrame_)();
> +	}

This seems like a potential candidate for a pure virtual function ?

> +
> +	/**
> +	 * \brief Signals that the statistics are ready.
> +	 *
> +	 * The int parameter isn't actually used.
> +	 */
> +	Signal<int> statsReady;
> +};
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 86494663..3d63e8a2 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -71,6 +71,7 @@ subdir('converter')
>  subdir('ipa')
>  subdir('pipeline')
>  subdir('proxy')
> +subdir('software_isp')
>
>  null_dep = dependency('', required : false)
>
> diff --git a/src/libcamera/software_isp/meson.build b/src/libcamera/software_isp/meson.build
> new file mode 100644
> index 00000000..9359075d
> --- /dev/null
> +++ b/src/libcamera/software_isp/meson.build
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: CC0-1.0
> +
> +libcamera_sources += files([
> +	'swstats.cpp',
> +])
> 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@redhat.com>
> + *
> + * swstats.cpp - software statistics base class
> + */
> +
> +#include "libcamera/internal/software_isp/swstats.h"
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(SwStats)
> +
> +SwStats::~SwStats()
> +{
> +}

You'll need a .cpp file anyway for comments, but why having a lot of
inline code in the header (which I would move to the .cpp file, as it
doesn't depend on any template resolution) and have the empty
constructor here ?

Thanks
  j

> +
> +} /* namespace libcamera */
> --
> 2.43.0
>

Hi,

On 1/23/24 16:37, Milan Zamazal wrote:
> Hans de Goede <hdegoede@redhat.com> writes:
> 
>> 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.
>>
>> This base class also allows the user of an implementation to specify
>> a window over which to gather statistics instead of processing the
>> whole frame; and it allows the implementation to choose to only
>> process 1/2, 1/4th, etc. of the lines instead of processing all
>> lines (in the window) by setting y_skip_mask_ from configure().
>> Skipping columns is left up the line-processing functions provided
>> by the implementation.
>>
>> Doxygen documentation by Dennis Bonke.
>>
>> Co-authored-by: Dennis Bonke <admin@dennisbonke.com>
>> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
>> Co-authored-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
>> Tested-by: Pavel Machek <pavel@ucw.cz>
>> ---
>>  include/libcamera/internal/meson.build        |   1 +
>>  .../internal/software_isp/meson.build         |   6 +
>>  .../internal/software_isp/swisp_stats.h       |  34 +++
>>  .../libcamera/internal/software_isp/swstats.h | 215 ++++++++++++++++++
>>  src/libcamera/meson.build                     |   1 +
>>  src/libcamera/software_isp/meson.build        |   5 +
>>  src/libcamera/software_isp/swstats.cpp        |  22 ++
>>  7 files changed, 284 insertions(+)
>>  create mode 100644 include/libcamera/internal/software_isp/meson.build
>>  create mode 100644 include/libcamera/internal/software_isp/swisp_stats.h
>>  create mode 100644 include/libcamera/internal/software_isp/swstats.h
>>  create mode 100644 src/libcamera/software_isp/meson.build
>>  create mode 100644 src/libcamera/software_isp/swstats.cpp
>>
>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
>> index 1325941d..caa533c4 100644
>> --- a/include/libcamera/internal/meson.build
>> +++ b/include/libcamera/internal/meson.build
>> @@ -51,3 +51,4 @@ libcamera_internal_headers = files([
>>  ])
>>  
>>  subdir('converter')
>> +subdir('software_isp')
>> diff --git a/include/libcamera/internal/software_isp/meson.build b/include/libcamera/internal/software_isp/meson.build
>> 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];
>> +};
>> +
>> +} /* 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@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.
>> + */
>> +class SwStats
>> +{
>> +public:
>> +	virtual ~SwStats() = 0;
>> +
>> +	/**
>> +	 * \brief Gets wether the statistics object is valid.
>> +	 * 
>> +	 * \return true if it's valid, false otherwise.
>> +	 */
>> +	virtual bool isValid() const = 0;
>> +
>> +	/**
>> +	 * \brief Configure the statistics object for the passed in input format.
>> +	 * \param[in] inputCfg The input format
>> +	 *
>> +	 * \return 0 on success, a negative errno value on failure.
>> +	 */
>> +	virtual int configure(const StreamConfiguration &inputCfg) = 0;
>> +
>> +	/**
>> +	 * \brief Get the file descriptor for the statistics.
>> +	 *
>> +	 * \return the file descriptor
>> +	 */
>> +	virtual const SharedFD &getStatsFD() = 0;
>> +
>> +protected:
>> +	/**
>> +	 * \brief Called when there is data to get statistics from.
>> +	 * \param[in] src The input data
>> +	 */
>> +	typedef void (SwStats::*statsProcessFn)(const uint8_t *src[]);
>> +	/**
>> +	 * \brief Called when the statistics gathering is done or when a new frame starts.
>> +	 */
>> +	typedef void (SwStats::*statsVoidFn)();
>> +
>> +	/* Variables set by configure(), used every line */
>> +	/**
>> +	 * \brief The function called when a line is ready for statistics processing.
>> +	 *
>> +	 * Used for line 0 and 1, repeating if there isn't a 3rd and a 4th line in the bayer order.
>> +	 */
>> +	statsProcessFn stats0_;
>> +	/**
>> +	 * \brief The function called when a line is ready for statistics processing.
>> +	 *
>> +	 * Used for line 3 and 4, only needed if the bayer order has 4 different lines.
>> +	 */
>> +	statsProcessFn stats2_;
>> +
>> +	/**
>> +	 * \brief The memory used per pixel in bits.
>> +	 */
>> +	unsigned int bpp_;
>> +	/**
>> +	 * \brief Skip lines where this bitmask is set in y.
>> +	 */
>> +	unsigned int y_skip_mask_;
>> +
>> +	/**
>> +	 * \brief Statistics window, set by setWindow(), used ever line.
>> +	 */
>> +	Rectangle window_;
>> +
>> +	/**
>> +	 * \brief The function called at the start of a frame.
>> +	 */
>> +	statsVoidFn startFrame_;
>> +	/**
>> +	 * \brief The function called at the end of a frame.
>> +	 */
>> +	statsVoidFn finishFrame_;
>> +	/**
>> +	 * \brief The size of the bayer pattern.
>> +	 */
>> +	Size patternSize_;
>> +	/**
>> +	 * \brief The offset of x, applied to window_.x for bayer variants.
>> +	 *
>> +	 * This can either be 0 or 1.
>> +	 */
>> +	unsigned int x_shift_;
>> +
>> +public:
>> +	/**
>> +	 * \brief Get the pattern size.
>> +	 *
>> +	 * For some input-formats, e.g. Bayer data, processing is done multiple lines
>> +	 * and/or columns at a time. Get width and height at which the (bayer) pattern
>> +	 * repeats. Window values are rounded down to a multiple of this and the height
>> +	 * also indicates if processLine2() should be called or not.
>> +	 * This may only be called after a successful configure() call.
>> +	 *
>> +	 * \return the pattern size.
>> +	 */
>> +	const Size &patternSize() { return patternSize_; }
>> +
>> +	/**
>> +	 * \brief Specify window coordinates over which to gather statistics.
>> +	 * \param[in] window The window object.
>> +	 */
>> +	void setWindow(Rectangle window)
>> +	{
>> +		window_ = window;
>> +
>> +		window_.x &= ~(patternSize_.width - 1);
>> +		window_.x += x_shift_;
>> +		window_.y &= ~(patternSize_.height - 1);
>> +
>> +		/* width_ - x_shift_ to make sure the window fits */
>> +		window_.width -= x_shift_;
>> +		window_.width &= ~(patternSize_.width - 1);
>> +		window_.height &= ~(patternSize_.height - 1);
> 
> These operations work correctly only if width & height are powers of 2, right?
> Maybe this is satisfied for all Bayer patterns but I think such implicit
> assumptions should be either not introduced or sufficiently visibly documented.

Ack, I have added a comment that patternSize_.width and height always
must be either 2 or 4.

Regards,

Hans

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@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@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

Hi Jacopo,

Thank you for the review.

On 1/31/24 13:44, Jacopo Mondi wrote:

<snip>

>> new file mode 100644
>> index 00000000..dcac7064
>> --- /dev/null
>> +++ b/include/libcamera/internal/software_isp/swstats.h

<snip>

>> +protected:
>> +	/**
>> +	 * \brief Called when there is data to get statistics from.
>> +	 * \param[in] src The input data
>> +	 */
>> +	typedef void (SwStats::*statsProcessFn)(const uint8_t *src[]);
> 
> I'll see how this is used later, but passing in an unbound pointer to
> memory seems suspicious. Span<> can help maybe or maybe just passing in a
> size ?

The size is given by the rectanlge in window_ which is
set by calling setWindow()

>> +	/**
>> +	 * \brief Called when the statistics gathering is done or when a new frame starts.
>> +	 */
>> +	typedef void (SwStats::*statsVoidFn)();
>> +
>> +	/* Variables set by configure(), used every line */
>> +	/**
>> +	 * \brief The function called when a line is ready for statistics processing.
>> +	 *
>> +	 * Used for line 0 and 1, repeating if there isn't a 3rd and a 4th line in the bayer order.
>> +	 */
>> +	statsProcessFn stats0_;
>> +	/**
>> +	 * \brief The function called when a line is ready for statistics processing.
>> +	 *
>> +	 * Used for line 3 and 4, only needed if the bayer order has 4 different lines.
>> +	 */
>> +	statsProcessFn stats2_;
> 
> Is the idea that derived classes shall provide these ? Isn't a pure
> virtual function better ?
> 
> ...
> 
> Ok, I've now read the next patches, and you set stat0_ and stat2_
> conditionally on the bitdepth
> 
> 		switch (bayerFormat.bitDepth) {
> 		case 8:
> 			stats0_ = (SwStats::statsProcessFn)&SwStatsCpu::statsBGGR8Line0;
> 			return 0;
> 		case 10:
> 			stats0_ = (SwStats::statsProcessFn)&SwStatsCpu::statsBGGR10Line0;
> 			return 0;
> 		}
> 
> stats2_ seems not used in you implementation
> 
> This makes me wonder if the 8 and 10 bits implementation shouldn't
> actually be two dervied classes, each one implementing their own
> stats0() functions. It's indeed more verbose though...

The bit you found is just for the unpacked 8 and 10 bpp support,
there is another function for 10bpp packed and v3 will add
12 bpp unpacked and I also have patches adding IGIG_GBGR_IGIG_GRGB
support. This is found on the ov01a1s, which has the following
bayer pattern (4x4 tile repeating):

IGIG
GBGR
IGIG
GRGB

And I also plan to use swstats + the softIPA together with
the atomisp, which has a working HW ISP, but we have no idea
what the statistics mean so my plan so to use swstats
for autogain + awb.

This will mean running swstats after debayering, so on
say RGB888 or YUV420, so yet more formats to support.

And who knows what the future brings (e.g. packed 12bpp bayer?)
adding a derived class for each new supported pixelformat
seems like a lot of extra code / overhead.

Which is why I went with old-school C-style function
pointers for this.

Note the IGIG_GBGR_IGIG_GRGB case is also what the
stats2_ function pointer is for. stats0_ processes
lines 0 and 1 of a 2 or 4 line block and stats2_
processes lines 2 and 3 of a 4 line block.


>> +	/**
>> +	 * \brief The memory used per pixel in bits.
> 
> Usually called 'bitdepth' in other parts of the library

The way bitDepth is used in e.g. bayerFormat is different
from this though.

This is not the number of significant bits per pixel,
this is the number of buts used in memory.

So for e.g. bayerFormat.bitDepth = 10, this is 16 for
unpacked data (1 pixel per 16 bit memory word) and
10 for packed data (4 pixels per 5 bytes of memory).

I'm open to a better name then bpp and also I'm open
to improve the brief, since I was hoping that the brief
explained (already) how this is different from e.g.
bayerFormat.bitDepth .

>> +	 */
>> +	unsigned int bpp_;
>> +	/**
>> +	 * \brief Skip lines where this bitmask is set in y.
>> +	 */
>> +	unsigned int y_skip_mask_;
>> +
>> +	/**
>> +	 * \brief Statistics window, set by setWindow(), used ever line.
>> +	 */
>> +	Rectangle window_;
>> +
>> +	/**
>> +	 * \brief The function called at the start of a frame.
>> +	 */
>> +	statsVoidFn startFrame_;
>> +	/**
>> +	 * \brief The function called at the end of a frame.
>> +	 */
>> +	statsVoidFn finishFrame_;
>> +	/**
>> +	 * \brief The size of the bayer pattern.
>> +	 */
>> +	Size patternSize_;
> 
> Do you have a use case already for pattern sizes larger than the usual
> 2x2 ? Mostly out of curiosity

Yes, the IGIG_GBGR_IGIG_GRGB pattern mentioned above.
I already have this working, but I first need to upstream
the kernel support for the ov01a1s (and finalize the
V4L2_PIXFMT for it) before this can be upstreamed into
libcamera.

<snip>


>> +	/**
>> +	 * \brief Specify window coordinates over which to gather statistics.
>> +	 * \param[in] window The window object.
>> +	 */
>> +	void setWindow(Rectangle window)
>> +	{
>> +		window_ = window;
>> +
>> +		window_.x &= ~(patternSize_.width - 1);
>> +		window_.x += x_shift_;
>> +		window_.y &= ~(patternSize_.height - 1);
>> +
>> +		/* width_ - x_shift_ to make sure the window fits */
>> +		window_.width -= x_shift_;
>> +		window_.width &= ~(patternSize_.width - 1);
>> +		window_.height &= ~(patternSize_.height - 1);
>> +	}
> 
> The usage of x_shift (in conjunction with swap_lines (btw, we use
> camelCase and not snake_case)) seems to depend on the fact the SwStatCpu
> implementation uses a single function to deal with the 4 bayer
> permutations. A different implementation might not use the same. I
> wonder if all of this shouldn't be moved to SwStatCpu..

Ack, Laurent suggested just dropping the base class altogether and
only have a SwStatsCpu class for now. OTOH Bryan mentioned that
he wanted to move more stuff into the base-class to share it
for his GLES work.

So my plan for now is to leave the split as is and figure this out
once we also have some GLES POC / MVP code.

<snip>

>> +	/**
>> +	 * \brief Finish statistics calculation for the current frame.
>> +	 *
>> +	 * This may only be called after a successful setWindow() call.
>> +	 */
>> +	void finishFrame()
>> +	{
>> +		(this->*finishFrame_)();
>> +	}
> 
> This seems like a potential candidate for a pure virtual function ?

Ack and the same for startFrame().

<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@redhat.com>
>> + *
>> + * swstats.cpp - software statistics base class
>> + */
>> +
>> +#include "libcamera/internal/software_isp/swstats.h"
>> +
>> +namespace libcamera {
>> +
>> +LOG_DEFINE_CATEGORY(SwStats)
>> +
>> +SwStats::~SwStats()
>> +{
>> +}
> 
> You'll need a .cpp file anyway for comments, but why having a lot of
> inline code in the header (which I would move to the .cpp file, as it
> doesn't depend on any template resolution) and have the empty
> constructor here ?

Note this is an empty destructor not constructor and
the destructor is declared virtual (pure virtual even)
in the .h file.

So I did not expect to have to implement a destructor in the base-class
at all, but even with it being pure-virtual the derived SwStatsCpu class
destructor will chain up to the base-class destructor leading to a
linker error about that missing from the vtable for the base class
unless I add this.

This might just be me being inexperienced with c++, so if there
is another way to solve this suggestions are welcome.

Regards,

Hans

Hi Hans

On Thu, Feb 08, 2024 at 05:13:58PM +0100, Hans de Goede wrote:
> Hi Jacopo,
>
> Thank you for the review.
>
> On 1/31/24 13:44, Jacopo Mondi wrote:
>
> <snip>
>
> >> new file mode 100644
> >> index 00000000..dcac7064
> >> --- /dev/null
> >> +++ b/include/libcamera/internal/software_isp/swstats.h
>
> <snip>
>
> >> +protected:
> >> +	/**
> >> +	 * \brief Called when there is data to get statistics from.
> >> +	 * \param[in] src The input data
> >> +	 */
> >> +	typedef void (SwStats::*statsProcessFn)(const uint8_t *src[]);
> >
> > I'll see how this is used later, but passing in an unbound pointer to
> > memory seems suspicious. Span<> can help maybe or maybe just passing in a
> > size ?
>
> The size is given by the rectanlge in window_ which is
> set by calling setWindow()
>

mkey, it's just that unbounded memory passed to a function looks not
great

> >> +	/**
> >> +	 * \brief Called when the statistics gathering is done or when a new frame starts.
> >> +	 */
> >> +	typedef void (SwStats::*statsVoidFn)();
> >> +
> >> +	/* Variables set by configure(), used every line */
> >> +	/**
> >> +	 * \brief The function called when a line is ready for statistics processing.
> >> +	 *
> >> +	 * Used for line 0 and 1, repeating if there isn't a 3rd and a 4th line in the bayer order.
> >> +	 */
> >> +	statsProcessFn stats0_;
> >> +	/**
> >> +	 * \brief The function called when a line is ready for statistics processing.
> >> +	 *
> >> +	 * Used for line 3 and 4, only needed if the bayer order has 4 different lines.
> >> +	 */
> >> +	statsProcessFn stats2_;
> >
> > Is the idea that derived classes shall provide these ? Isn't a pure
> > virtual function better ?
> >
> > ...
> >
> > Ok, I've now read the next patches, and you set stat0_ and stat2_
> > conditionally on the bitdepth
> >
> > 		switch (bayerFormat.bitDepth) {
> > 		case 8:
> > 			stats0_ = (SwStats::statsProcessFn)&SwStatsCpu::statsBGGR8Line0;
> > 			return 0;
> > 		case 10:
> > 			stats0_ = (SwStats::statsProcessFn)&SwStatsCpu::statsBGGR10Line0;
> > 			return 0;
> > 		}
> >
> > stats2_ seems not used in you implementation
> >
> > This makes me wonder if the 8 and 10 bits implementation shouldn't
> > actually be two dervied classes, each one implementing their own
> > stats0() functions. It's indeed more verbose though...
>
> The bit you found is just for the unpacked 8 and 10 bpp support,
> there is another function for 10bpp packed and v3 will add
> 12 bpp unpacked and I also have patches adding IGIG_GBGR_IGIG_GRGB
> support. This is found on the ov01a1s, which has the following
> bayer pattern (4x4 tile repeating):
>
> IGIG
> GBGR
> IGIG
> GRGB
>
> And I also plan to use swstats + the softIPA together with
> the atomisp, which has a working HW ISP, but we have no idea
> what the statistics mean so my plan so to use swstats
> for autogain + awb.
>
> This will mean running swstats after debayering, so on
> say RGB888 or YUV420, so yet more formats to support.
>
> And who knows what the future brings (e.g. packed 12bpp bayer?)
> adding a derived class for each new supported pixelformat
> seems like a lot of extra code / overhead.
>
> Which is why I went with old-school C-style function
> pointers for this.
>

I understand. I'm not a fanatic of OOP when old-school C-style methods
make the code more concise, and I don't have time resources to try
propose something different, so I guess what you have is good :)

> Note the IGIG_GBGR_IGIG_GRGB case is also what the
> stats2_ function pointer is for. stats0_ processes
> lines 0 and 1 of a 2 or 4 line block and stats2_
> processes lines 2 and 3 of a 4 line block.
>

Yep, thanks

>
> >> +	/**
> >> +	 * \brief The memory used per pixel in bits.
> >
> > Usually called 'bitdepth' in other parts of the library
>
> The way bitDepth is used in e.g. bayerFormat is different
> from this though.
>
> This is not the number of significant bits per pixel,
> this is the number of buts used in memory.

Thanks for the typo, made me laugh

>
> So for e.g. bayerFormat.bitDepth = 10, this is 16 for
> unpacked data (1 pixel per 16 bit memory word) and
> 10 for packed data (4 pixels per 5 bytes of memory).

Oh, I see, so it's not the size on the wire, but the size in memory
once unpacked.

>
> I'm open to a better name then bpp and also I'm open
> to improve the brief, since I was hoping that the brief
> explained (already) how this is different from e.g.
> bayerFormat.bitDepth .
>

Stupid question, will this ever be != 16 ?

You can expand the description by saying this value represents the
size of the memory word where one pixel sample is stored in and
explicitly state this is different from the raw image bitdepth (the
bit size of the sample).

Quoting the v4l2 documentation for the memory RAW formats for
more inspiration:

        These four pixel formats are raw sRGB / Bayer formats with 10
        bits per sample. Each sample is stored in a 16-bit word, with
        6 unused high bits filled with zeros. Each n-pixel row
        contains n/2 green samples and n/2 blue or red samples, with
        alternating red and blue rows. Bytes are stored in memory in
        little endian order. They are conventionally described as
        GRGR... BGBG..., RGRG... GBGB..., etc. Below is an example of
        one of these formats:



> >> +	 */
> >> +	unsigned int bpp_;
> >> +	/**
> >> +	 * \brief Skip lines where this bitmask is set in y.
> >> +	 */
> >> +	unsigned int y_skip_mask_;
> >> +
> >> +	/**
> >> +	 * \brief Statistics window, set by setWindow(), used ever line.
> >> +	 */
> >> +	Rectangle window_;
> >> +
> >> +	/**
> >> +	 * \brief The function called at the start of a frame.
> >> +	 */
> >> +	statsVoidFn startFrame_;
> >> +	/**
> >> +	 * \brief The function called at the end of a frame.
> >> +	 */
> >> +	statsVoidFn finishFrame_;
> >> +	/**
> >> +	 * \brief The size of the bayer pattern.
> >> +	 */
> >> +	Size patternSize_;
> >
> > Do you have a use case already for pattern sizes larger than the usual
> > 2x2 ? Mostly out of curiosity
>
> Yes, the IGIG_GBGR_IGIG_GRGB pattern mentioned above.
> I already have this working, but I first need to upstream
> the kernel support for the ov01a1s (and finalize the
> V4L2_PIXFMT for it) before this can be upstreamed into
> libcamera.

nice!

>
> <snip>
>
>
> >> +	/**
> >> +	 * \brief Specify window coordinates over which to gather statistics.
> >> +	 * \param[in] window The window object.
> >> +	 */
> >> +	void setWindow(Rectangle window)
> >> +	{
> >> +		window_ = window;
> >> +
> >> +		window_.x &= ~(patternSize_.width - 1);
> >> +		window_.x += x_shift_;
> >> +		window_.y &= ~(patternSize_.height - 1);
> >> +
> >> +		/* width_ - x_shift_ to make sure the window fits */
> >> +		window_.width -= x_shift_;
> >> +		window_.width &= ~(patternSize_.width - 1);
> >> +		window_.height &= ~(patternSize_.height - 1);
> >> +	}
> >
> > The usage of x_shift (in conjunction with swap_lines (btw, we use
> > camelCase and not snake_case)) seems to depend on the fact the SwStatCpu
> > implementation uses a single function to deal with the 4 bayer
> > permutations. A different implementation might not use the same. I
> > wonder if all of this shouldn't be moved to SwStatCpu..
>
> Ack, Laurent suggested just dropping the base class altogether and
> only have a SwStatsCpu class for now. OTOH Bryan mentioned that
> he wanted to move more stuff into the base-class to share it
> for his GLES work.
>
> So my plan for now is to leave the split as is and figure this out
> once we also have some GLES POC / MVP code.
>

Makes sense, let's keep this in mind!

> <snip>
>
> >> +	/**
> >> +	 * \brief Finish statistics calculation for the current frame.
> >> +	 *
> >> +	 * This may only be called after a successful setWindow() call.
> >> +	 */
> >> +	void finishFrame()
> >> +	{
> >> +		(this->*finishFrame_)();
> >> +	}
> >
> > This seems like a potential candidate for a pure virtual function ?
>
> Ack and the same for startFrame().
>
> <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@redhat.com>
> >> + *
> >> + * swstats.cpp - software statistics base class
> >> + */
> >> +
> >> +#include "libcamera/internal/software_isp/swstats.h"
> >> +
> >> +namespace libcamera {
> >> +
> >> +LOG_DEFINE_CATEGORY(SwStats)
> >> +
> >> +SwStats::~SwStats()
> >> +{
> >> +}
> >
> > You'll need a .cpp file anyway for comments, but why having a lot of
> > inline code in the header (which I would move to the .cpp file, as it
> > doesn't depend on any template resolution) and have the empty
> > constructor here ?
>
> Note this is an empty destructor not constructor and
> the destructor is declared virtual (pure virtual even)
> in the .h file.

yeah sorry, typo

>
> So I did not expect to have to implement a destructor in the base-class
> at all, but even with it being pure-virtual the derived SwStatsCpu class
> destructor will chain up to the base-class destructor leading to a
> linker error about that missing from the vtable for the base class
> unless I add this.
>
> This might just be me being inexperienced with c++, so if there
> is another way to solve this suggestions are welcome.
>

My point was that most if not all functions implementation should go
the .cpp file and not in the header. But maybe I missed something ?

> Regards,
>
> Hans
>
>

Hi Jacopo,

On 2/12/24 17:08, Jacopo Mondi wrote:
> Hi Hans
> 
> On Thu, Feb 08, 2024 at 05:13:58PM +0100, Hans de Goede wrote:
>> Hi Jacopo,
>>
>> Thank you for the review.
>>
>> On 1/31/24 13:44, Jacopo Mondi wrote:

<snip>

>>>> +	/**
>>>> +	 * \brief The memory used per pixel in bits.
>>>
>>> Usually called 'bitdepth' in other parts of the library
>>
>> The way bitDepth is used in e.g. bayerFormat is different
>> from this though.
>>
>> This is not the number of significant bits per pixel,
>> this is the number of buts used in memory.
> 
> Thanks for the typo, made me laugh
> 
>>
>> So for e.g. bayerFormat.bitDepth = 10, this is 16 for
>> unpacked data (1 pixel per 16 bit memory word) and
>> 10 for packed data (4 pixels per 5 bytes of memory).
> 
> Oh, I see, so it's not the size on the wire, but the size in memory
> once unpacked.

Ack.

>> I'm open to a better name then bpp and also I'm open
>> to improve the brief, since I was hoping that the brief
>> explained (already) how this is different from e.g.
>> bayerFormat.bitDepth .
>>
> 
> Stupid question, will this ever be != 16 ?

Yes there are 10bpp packed in memory formats and this is actually
the first format both Andrey and I used for testing.

For these this bpp value is 10 and paternSize_.width =
set to 4 so that all sizes / x coords get rounded down
to a multiple of 4, which gives us 5 bytes bounderies
in memory. 

> You can expand the description by saying this value represents the
> size of the memory word where one pixel sample is stored

but is is not necessarily a wordsize, see the 10 bpp in
case of the packed fmt.

> in and
> explicitly state this is different from the raw image bitdepth (the
> bit size of the sample). Raw image bitdepth is what I tried to
say succinctly with "precision". Given the above I think that my:

    unsigned int bpp; /* Memory used per pixel, not precision */

is not that bad and I'm going to stick with that for v3.



> Quoting the v4l2 documentation for the memory RAW formats for
> more inspiration:
> 
>         These four pixel formats are raw sRGB / Bayer formats with 10
>         bits per sample. Each sample is stored in a 16-bit word, with
>         6 unused high bits filled with zeros. Each n-pixel row
>         contains n/2 green samples and n/2 blue or red samples, with
>         alternating red and blue rows. Bytes are stored in memory in
>         little endian order. They are conventionally described as
>         GRGR... BGBG..., RGRG... GBGB..., etc. Below is an example of
>         one of these formats:

Right so this described the unpacked in memory format there also is
a packed fmt...

<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@redhat.com>
>>>> + *
>>>> + * swstats.cpp - software statistics base class
>>>> + */
>>>> +
>>>> +#include "libcamera/internal/software_isp/swstats.h"
>>>> +
>>>> +namespace libcamera {
>>>> +
>>>> +LOG_DEFINE_CATEGORY(SwStats)
>>>> +
>>>> +SwStats::~SwStats()
>>>> +{
>>>> +}
>>>
>>> You'll need a .cpp file anyway for comments, but why having a lot of
>>> inline code in the header (which I would move to the .cpp file, as it
>>> doesn't depend on any template resolution) and have the empty
>>> constructor here ?
>>
>> Note this is an empty destructor not constructor and
>> the destructor is declared virtual (pure virtual even)
>> in the .h file.
> 
> yeah sorry, typo
> 
>>
>> So I did not expect to have to implement a destructor in the base-class
>> at all, but even with it being pure-virtual the derived SwStatsCpu class
>> destructor will chain up to the base-class destructor leading to a
>> linker error about that missing from the vtable for the base class
>> unless I add this.
>>
>> This might just be me being inexperienced with c++, so if there
>> is another way to solve this suggestions are welcome.
>>
> 
> My point was that most if not all functions implementation should go
> the .cpp file and not in the header. But maybe I missed something ?

Ah I see. That is a mostly valid point. Except for the processLine0()
and processLine2() methods which we want to keep inline for
performance reasons.

I'll move all of the non performance critical code to the .cpp file.

Regards,

Hans

Hi!

> And I also plan to use swstats + the softIPA together with
> the atomisp, which has a working HW ISP, but we have no idea
> what the statistics mean so my plan so to use swstats
> for autogain + awb.

Fun hardware. You could still access the raw bayer, no?

> This will mean running swstats after debayering, so on
> say RGB888 or YUV420, so yet more formats to support.

For taking photos on cellphone, best solution would be to do swstats
(but not debayer) most of the time, so ability to do them on bayer
remains important.

Best regards,
							Pavel

Hi,

On 2/13/24 09:58, Pavel Machek wrote:
> Hi!
> 
>> And I also plan to use swstats + the softIPA together with
>> the atomisp, which has a working HW ISP, but we have no idea
>> what the statistics mean so my plan so to use swstats
>> for autogain + awb.
> 
> Fun hardware. You could still access the raw bayer, no?

No, the Android driver the current mainline-staging driver
is based on has RAW bayer format support commented out
with a statement that it is broken...

>> This will mean running swstats after debayering, so on
>> say RGB888 or YUV420, so yet more formats to support.
> 
> For taking photos on cellphone, best solution would be to do swstats
> (but not debayer) most of the time, so ability to do them on bayer
> remains important.

Right, I'm not suggesting that we remove swstats support
for Bayer formats. What I'm trying to say is that we will
want to support some non Bayer formats on top of the Bayer
formats.

Regards,

Hans

Hi!

> >> And I also plan to use swstats + the softIPA together with
> >> the atomisp, which has a working HW ISP, but we have no idea
> >> what the statistics mean so my plan so to use swstats
> >> for autogain + awb.
> > 
> > Fun hardware. You could still access the raw bayer, no?
> 
> No, the Android driver the current mainline-staging driver
> is based on has RAW bayer format support commented out
> with a statement that it is broken...

Fun :-).

> >> This will mean running swstats after debayering, so on
> >> say RGB888 or YUV420, so yet more formats to support.
> > 
> > For taking photos on cellphone, best solution would be to do swstats
> > (but not debayer) most of the time, so ability to do them on bayer
> > remains important.
> 
> Right, I'm not suggesting that we remove swstats support
> for Bayer formats. What I'm trying to say is that we will
> want to support some non Bayer formats on top of the Bayer
> formats.

Ok, interesting. It would eventually be good to do some ISP processing
while debayering (at least on GPU, black level compensation likely
makes sense on CPU, too), but this will make statistics after debayer
even more tricky.

Best regards,

							Pavel