[libcamera-devel] [PATCH v2 07/18] libcamera: software_isp: Add SwStats base class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jan 23 17:46:26 CET 2024
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.
If you drop the base SwStats you could also avoid having to improve its
documentation :-)
> 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];
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.
> +
> +} /* 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>
> + *
> + * 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/.
Documentation goes to the .cpp file.
> +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[]);
Aren't you reimplementing virtual functions manually ?
> + /**
> + * \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_;
ySkipMask_
Same where applicable.
> +
> + /**
> + * \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);
> + }
> +
> + /**
> + * \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 at 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.
> +
> +} /* namespace libcamera */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list