[libcamera-devel] [PATCH v2 07/18] libcamera: software_isp: Add SwStats base class
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Wed Jan 31 13:44:52 CET 2024
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 at dennisbonke.com>
> Signed-off-by: Dennis Bonke <admin at dennisbonke.com>
> Co-authored-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> 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>
> ---
> 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 at 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 at 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
>
More information about the libcamera-devel
mailing list