[libcamera-devel] [PATCH v2 08/18] libcamera: software_isp: Add SwStatsCpu class

Laurent Pinchart laurent.pinchart at ideasonboard.com
Tue Jan 23 18:16:09 CET 2024


Hi Hans,

Thank you for the patch.

On Sat, Jan 13, 2024 at 03:22:08PM +0100, Hans de Goede wrote:
> Add a CPU based SwStats implementation for SoftwareISP / SoftIPA use.
> 
> Co-authored-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> Co-authored-by: Pavel Machek <pavel at ucw.cz>
> Signed-off-by: Pavel Machek <pavel at ucw.cz>
> Co-authored-by: Dennis Bonke <admin at dennisbonke.com>
> Signed-off-by: Dennis Bonke <admin at dennisbonke.com>
> Co-authored-by: Marttico <g.martti at gmail.com>
> Signed-off-by: Marttico <g.martti at gmail.com>
> Co-authored-by: Toon Langendam <t.langendam at gmail.com>
> Signed-off-by: Toon Langendam <t.langendam at gmail.com>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue at linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel at ucw.cz>
> ---
>  .../internal/software_isp/meson.build         |   1 +
>  .../internal/software_isp/swstats_cpu.h       |  44 +++++
>  src/libcamera/software_isp/meson.build        |   1 +
>  src/libcamera/software_isp/swstats_cpu.cpp    | 164 ++++++++++++++++++
>  4 files changed, 210 insertions(+)
>  create mode 100644 include/libcamera/internal/software_isp/swstats_cpu.h
>  create mode 100644 src/libcamera/software_isp/swstats_cpu.cpp
> 
> diff --git a/include/libcamera/internal/software_isp/meson.build b/include/libcamera/internal/software_isp/meson.build
> index 1c43acc4..1d9e4018 100644
> --- a/include/libcamera/internal/software_isp/meson.build
> +++ b/include/libcamera/internal/software_isp/meson.build
> @@ -3,4 +3,5 @@
>  libcamera_internal_headers += files([
>      'swisp_stats.h',
>      'swstats.h',
> +    'swstats_cpu.h',
>  ])
> diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h
> new file mode 100644
> index 00000000..8bb86e98
> --- /dev/null
> +++ b/include/libcamera/internal/software_isp/swstats_cpu.h
> @@ -0,0 +1,44 @@
> +/* 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_cpu.h - CPU based software statistics implementation
> + */
> +
> +#pragma once
> +
> +#include "libcamera/internal/shared_mem_object.h"
> +#include "libcamera/internal/software_isp/swisp_stats.h"
> +#include "libcamera/internal/software_isp/swstats.h"
> +
> +namespace libcamera {
> +
> +/**
> + * \class SwStatsCpu
> + * \brief Implementation for the Software statistics on the CPU.
> + */
> +class SwStatsCpu : public SwStats
> +{
> +public:
> +	SwStatsCpu();
> +	~SwStatsCpu() { }
> +
> +	bool isValid() const { return sharedStats_.fd().isValid(); }
> +	const SharedFD &getStatsFD() { return sharedStats_.fd(); }
> +	int configure(const StreamConfiguration &inputCfg);
> +private:
> +	void statsBGGR10PLine0(const uint8_t *src[]);
> +	void statsGBRG10PLine0(const uint8_t *src[]);
> +	void resetStats(void);
> +	void finishStats(void);
> +
> +	SharedMemObject<SwIspStats> sharedStats_;
> +	SwIspStats stats_;
> +	bool swap_lines_;
> +};
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/software_isp/meson.build b/src/libcamera/software_isp/meson.build
> index 9359075d..d31c6217 100644
> --- a/src/libcamera/software_isp/meson.build
> +++ b/src/libcamera/software_isp/meson.build
> @@ -2,4 +2,5 @@
>  
>  libcamera_sources += files([
>  	'swstats.cpp',
> +	'swstats_cpu.cpp',
>  ])
> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
> new file mode 100644
> index 00000000..59453d07
> --- /dev/null
> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
> @@ -0,0 +1,164 @@
> +/* 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_cpu.cpp - CPU based software statistics implementation
> + */
> +
> +#include "libcamera/internal/software_isp/swstats_cpu.h"
> +
> +#include <libcamera/base/log.h>
> +
> +#include <libcamera/stream.h>
> +
> +#include "libcamera/internal/bayer_format.h"
> +
> +namespace libcamera {
> +
> +SwStatsCpu::SwStatsCpu()
> +	: SwStats()
> +{
> +	sharedStats_ = SharedMemObject<SwIspStats>("softIsp_stats");
> +	if (!sharedStats_.fd().isValid())
> +		LOG(SwStats, Error)
> +			<< "Failed to create shared memory for statistics";
> +}
> +
> +/* for brightness values in the 0 to 255 range: */
> +static const unsigned int BRIGHT_LVL = 200U << 8;
> +static const unsigned int TOO_BRIGHT_LVL = 240U << 8;
> +
> +static const unsigned int RED_Y_MUL = 77; /* 0.30 * 256 */

static constexpr unsigned int kRedYMul = 77; /* 0.30 * 256 */

I would even suggest

static constexpr unsigned int kRedYMul = std::lround(0.30 * 256);

but lround is constexpr starting in C++23 only :-(

> +static const unsigned int GREEN_Y_MUL = 150; /* 0.59 * 256 */
> +static const unsigned int BLUE_Y_MUL = 29; /* 0.11 * 256 */
> +
> +#define SWISP_LINARO_START_LINE_STATS(pixel_t) \
> +	pixel_t r, g, g2, b;                   \
> +	unsigned int y_val;                    \
> +                                               \
> +	unsigned int sumR = 0;                 \
> +	unsigned int sumG = 0;                 \
> +	unsigned int sumB = 0;
> +
> +#define SWISP_LINARO_ACCUMULATE_LINE_STATS(div) \
> +	sumR += r;                              \
> +	sumG += g;                              \
> +	sumB += b;                              \
> +                                                \
> +	y_val = r * RED_Y_MUL;                  \
> +	y_val += g * GREEN_Y_MUL;               \
> +	y_val += b * BLUE_Y_MUL;                \
> +	stats_.y_histogram[y_val / (256 * 16 * (div))]++;
> +
> +#define SWISP_LINARO_FINISH_LINE_STATS() \
> +	stats_.sumR_ += sumR;            \
> +	stats_.sumG_ += sumG;            \
> +	stats_.sumB_ += sumB;

As these macros are only used in a single location, wouldn't the code be
more readable if you just inlined them ?

> +
> +static inline __attribute__((always_inline)) void
> +statsBayer10P(const int width, const uint8_t *src0, const uint8_t *src1, bool bggr, SwIspStats &stats_)

[[gnu::always_inline]] would be the C++ way.

[[gnu::always_inline]]
static inline void statsBayer10P(const int width, const uint8_t *src0,
				 const uint8_t *src1, bool bggr,
				 SwIspStats &stats_)

> +{
> +	const int width_in_bytes = width * 5 / 4;

widthInBytes

Same where applicable.

> +
> +	SWISP_LINARO_START_LINE_STATS(uint8_t)
> +
> +	for (int x = 0; x < width_in_bytes; x += 5) {
> +		if (bggr) {
> +			/* BGGR */
> +			b = src0[x];
> +			g = src0[x + 1];
> +			g2 = src1[x];
> +			r = src1[x + 1];
> +		} else {
> +			/* GBRG */
> +			g = src0[x];
> +			b = src0[x + 1];
> +			r = src1[x];
> +			g2 = src1[x + 1];
> +		}
> +		g = (g + g2) / 2;
> +
> +		SWISP_LINARO_ACCUMULATE_LINE_STATS(1)
> +	}
> +
> +	SWISP_LINARO_FINISH_LINE_STATS()
> +}
> +
> +void SwStatsCpu::statsBGGR10PLine0(const uint8_t *src[])
> +{
> +	const uint8_t *src0 = src[1] + window_.x * 5 / 4;
> +	const uint8_t *src1 = src[2] + window_.x * 5 / 4;
> +
> +	if (swap_lines_)
> +		std::swap(src0, src1);
> +
> +	statsBayer10P(window_.width, src0, src1, true, stats_);
> +}
> +
> +void SwStatsCpu::statsGBRG10PLine0(const uint8_t *src[])
> +{
> +	const uint8_t *src0 = src[1] + window_.x * 5 / 4;
> +	const uint8_t *src1 = src[2] + window_.x * 5 / 4;
> +
> +	if (swap_lines_)
> +		std::swap(src0, src1);
> +
> +	statsBayer10P(window_.width, src0, src1, false, stats_);
> +}

These two functions are identical except for the bggr argument they pass
to statsBayer10P(). Can't we store the bayer order in a member variable
and just use statsBayer10P() directly ?

> +
> +void SwStatsCpu::resetStats(void)
> +{
> +	stats_.sumR_ = 0;
> +	stats_.sumB_ = 0;
> +	stats_.sumG_ = 0;
> +	std::fill_n(stats_.y_histogram, 16, 0);
> +}
> +
> +void SwStatsCpu::finishStats(void)
> +{
> +	*sharedStats_ = stats_;
> +	statsReady.emit(0);

If you always emit this signal with 0, you don't have to give it an
argument.

> +}
> +
> +int SwStatsCpu::configure(const StreamConfiguration &inputCfg)
> +{
> +	BayerFormat bayerFormat =
> +		BayerFormat::fromPixelFormat(inputCfg.pixelFormat);
> +
> +	startFrame_ = (SwStats::statsVoidFn)&SwStatsCpu::resetStats;
> +	finishFrame_ = (SwStats::statsVoidFn)&SwStatsCpu::finishStats;

Use virtual functions directly.

> +
> +	if (bayerFormat.bitDepth == 10 &&
> +	    bayerFormat.packing == BayerFormat::Packing::CSI2) {
> +		bpp_ = 10;
> +		patternSize_.height = 2;
> +		patternSize_.width = 4; /* 5 bytes per *4* pixels */
> +		y_skip_mask_ = 0x02; /* Skip every 3th and 4th line */
> +		x_shift_ = 0;
> +
> +		switch (bayerFormat.order) {
> +		case BayerFormat::BGGR:
> +		case BayerFormat::GRBG:
> +			stats0_ = (SwStats::statsProcessFn)&SwStatsCpu::statsBGGR10PLine0;
> +			swap_lines_ = bayerFormat.order == BayerFormat::GRBG;
> +			return 0;
> +		case BayerFormat::GBRG:
> +		case BayerFormat::RGGB:
> +			stats0_ = (SwStats::statsProcessFn)&SwStatsCpu::statsGBRG10PLine0;
> +			swap_lines_ = bayerFormat.order == BayerFormat::RGGB;
> +			return 0;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	LOG(SwStats, Info)

Sounds like an error.

> +		<< "Unsupported input format " << inputCfg.pixelFormat.toString();
> +	return -EINVAL;
> +}
> +
> +} /* namespace libcamera */

-- 
Regards,

Laurent Pinchart


More information about the libcamera-devel mailing list