[PATCH v2 08/18] libcamera: software_isp: Add SwStatsCpu class
Milan Zamazal
mzamazal at redhat.com
Tue Jan 23 21:04:18 CET 2024
Hans de Goede <hdegoede at redhat.com> writes:
> 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 const unsigned int GREEN_Y_MUL = 150; /* 0.59 * 256 */
> +static const unsigned int BLUE_Y_MUL = 29; /* 0.11 * 256 */
The last two lines comments don't match the given values.
They should be 0.587 * 256 and 0.114 * 256 to equal.
> +#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))]++;
As the size of y_histogram (16) is used in multiple places, it should be a named constant.
> +#define SWISP_LINARO_FINISH_LINE_STATS() \
> + stats_.sumR_ += sumR; \
> + stats_.sumG_ += sumG; \
> + stats_.sumB_ += sumB;
> +
> +static inline __attribute__((always_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;
> +
> + 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)
> + }
Does this loop process only every other pixel? If yes, probably nothing wrong
with it for stats, but it deserves a comment.
> + 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;
This is difficult to understand: why src[1], src[2] and not src[0], src[1]?
> + 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_);
> +}
> +
> +void SwStatsCpu::resetStats(void)
> +{
> + stats_.sumR_ = 0;
> + stats_.sumB_ = 0;
> + stats_.sumG_ = 0;
> + std::fill_n(stats_.y_histogram, 16, 0);
Another use of the y_histogram size constant.
> +}
> +
> +void SwStatsCpu::finishStats(void)
> +{
> + *sharedStats_ = stats_;
> + statsReady.emit(0);
> +}
> +
> +int SwStatsCpu::configure(const StreamConfiguration &inputCfg)
> +{
> + BayerFormat bayerFormat =
> + BayerFormat::fromPixelFormat(inputCfg.pixelFormat);
> +
> + startFrame_ = (SwStats::statsVoidFn)&SwStatsCpu::resetStats;
> + finishFrame_ = (SwStats::statsVoidFn)&SwStatsCpu::finishStats;
> +
> + 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 */
Why? Worth an explanation in the comment?
> + 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)
> + << "Unsupported input format " << inputCfg.pixelFormat.toString();
> + return -EINVAL;
> +}
> +
> +} /* namespace libcamera */
More information about the libcamera-devel
mailing list