[PATCH v2 08/18] libcamera: software_isp: Add SwStatsCpu class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jan 23 22:55:20 CET 2024
On Tue, Jan 23, 2024 at 09:04:18PM +0100, Milan Zamazal wrote:
> 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[])
This should take a std::array<const uint8_t *, 3>.
> > +{
> > + 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.
You could make the histogram a std::array<unsigned int, 16>.
> > +}
> > +
> > +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 */
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list