[PATCH v6 07/18] libcamera: software_isp: Add Debayer base class

[Laurent Pinchart]

Hi Milan and Hans,

Thank you for the patch.

On Tue, Mar 19, 2024 at 01:35:54PM +0100, Milan Zamazal wrote:
> From: Hans de Goede <hdegoede at redhat.com>
> 
> Add a base class for debayer implementations. This is intended to be
> suitable for both GPU (or otherwise) accelerated debayer implementations
> as well as CPU based debayering.
> 
> Doxygen documentation by Dennis Bonke.
> 
> Tested-by: Bryan O'Donoghue <bryan.odonoghue at linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel at ucw.cz>
> Reviewed-by: Pavel Machek <pavel at ucw.cz>
> Reviewed-by: Milan Zamazal <mzamazal at redhat.com>
> Co-developed-by: Dennis Bonke <admin at dennisbonke.com>
> Signed-off-by: Dennis Bonke <admin at dennisbonke.com>
> Co-developed-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>
> ---
>  .../internal/software_isp/debayer_params.h    |  48 ++++++++
>  .../internal/software_isp/meson.build         |   1 +
>  src/libcamera/software_isp/debayer.cpp        |  29 +++++
>  src/libcamera/software_isp/debayer.h          | 104 ++++++++++++++++++
>  src/libcamera/software_isp/meson.build        |   1 +
>  5 files changed, 183 insertions(+)
>  create mode 100644 include/libcamera/internal/software_isp/debayer_params.h
>  create mode 100644 src/libcamera/software_isp/debayer.cpp
>  create mode 100644 src/libcamera/software_isp/debayer.h
> 
> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h
> new file mode 100644
> index 00000000..98965fa1
> --- /dev/null
> +++ b/include/libcamera/internal/software_isp/debayer_params.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Red Hat Inc.
> + *
> + * Authors:
> + * Hans de Goede <hdegoede at redhat.com>
> + *
> + * debayer_params.h - DebayerParams header
> + */
> +
> +#pragma once
> +
> +namespace libcamera {
> +
> +/**
> + * \brief Struct to hold the debayer parameters.
> + */

Documentation in .cpp files.

> +struct DebayerParams {
> +	/**
> +	 * \brief const value for 1.0 gain
> +	 */
> +	static constexpr unsigned int kGain10 = 256;

Forcing the caller to deal with the internal representation of gains
isn't nice, especially given that it precludes implementing gains of
different precisions in different backend. Wouldn't it be better to pass
the values as floating point numbers, and convert them to the internal
representation in the implementation of process() before using them ?

> +
> +	/**
> +	 * \brief Red Gain
> +	 *
> +	 * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.
> +	 */
> +	unsigned int gainR;
> +	/**
> +	 * \brief Green Gain
> +	 *
> +	 * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.
> +	 */
> +	unsigned int gainG;
> +	/**
> +	 * \brief Blue Gain
> +	 *
> +	 * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.
> +	 */
> +	unsigned int gainB;
> +	/**
> +	 * \brief Gamma correction, 1.0 is no correction
> +	 */
> +	float gamma;
> +};
> +
> +} /* namespace libcamera */
> diff --git a/include/libcamera/internal/software_isp/meson.build b/include/libcamera/internal/software_isp/meson.build
> index 66c9c3fb..a620e16d 100644
> --- a/include/libcamera/internal/software_isp/meson.build
> +++ b/include/libcamera/internal/software_isp/meson.build
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
>  libcamera_internal_headers += files([
> +    'debayer_params.h',
>      'swisp_stats.h',
>  ])
> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
> new file mode 100644
> index 00000000..64f0b5a0
> --- /dev/null
> +++ b/src/libcamera/software_isp/debayer.cpp
> @@ -0,0 +1,29 @@
> +/* 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>
> + *
> + * debayer.cpp - debayer base class
> + */
> +
> +#include "debayer.h"
> +
> +namespace libcamera {
> +
> +/**
> + * \class Debayer
> + * \brief Base debayering class
> + *
> + * Base class that provides functions for setting up the debayering process.
> + */
> +
> +LOG_DEFINE_CATEGORY(Debayer)
> +
> +Debayer::~Debayer()
> +{
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h
> new file mode 100644
> index 00000000..8880ff99
> --- /dev/null
> +++ b/src/libcamera/software_isp/debayer.h
> @@ -0,0 +1,104 @@
> +/* 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>
> + *
> + * debayer.h - debayering base class
> + */
> +
> +#pragma once
> +
> +#include <stdint.h>
> +
> +#include <libcamera/base/log.h>
> +#include <libcamera/base/signal.h>
> +
> +#include <libcamera/geometry.h>
> +#include <libcamera/stream.h>
> +
> +#include "libcamera/internal/software_isp/debayer_params.h"
> +
> +namespace libcamera {
> +
> +class FrameBuffer;
> +
> +LOG_DECLARE_CATEGORY(Debayer)
> +
> +class Debayer
> +{
> +public:
> +	virtual ~Debayer() = 0;
> +
> +	/**
> +	 * \brief Configure the debayer object according to the passed in parameters.
> +	 * \param[in] inputCfg The input configuration.
> +	 * \param[in] outputCfgs The output configurations.
> +	 *
> +	 * \return 0 on success, a negative errno on failure.
> +	 */

Documentation in .cpp files.

> +	virtual int configure(const StreamConfiguration &inputCfg,
> +			      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) = 0;
> +
> +	/**
> +	 * \brief Get the width and height at which the bayer pattern repeats.
> +	 * \param[in] inputFormat The input format.
> +	 *
> +	 * Valid sizes are: 2x2, 4x2 or 4x4.
> +	 *
> +	 * \return pattern size or an empty size for unsupported inputFormats.
> +	 */
> +	virtual Size patternSize(PixelFormat inputFormat) = 0;

Unless I'm mistaken, this is used internally in the DebayerCpu class
only. I think you can drop the function from the interface, and make it
private.

> +
> +	/**
> +	 * \brief Get the supported output formats.
> +	 * \param[in] inputFormat The input format.
> +	 *
> +	 * \return all supported output formats or an empty vector if there are none.
> +	 */
> +	virtual std::vector<PixelFormat> formats(PixelFormat inputFormat) = 0;
> +
> +	/**
> +	 * \brief Get the stride and the frame size.
> +	 * \param[in] outputFormat The output format.
> +	 * \param[in] size The output size.
> +	 *
> +	 * \return a tuple of the stride and the frame size, or a tuple with 0,0 if there is no valid output config.

Line wrap.

> +	 */
> +	virtual std::tuple<unsigned int, unsigned int>
> +	strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0;
> +
> +	/**
> +	 * \brief Process the bayer data into the requested format.
> +	 * \param[in] input The input buffer.
> +	 * \param[in] output The output buffer.
> +	 * \param[in] params The parameters to be used in debayering.
> +	 *
> +	 * \note DebayerParams is passed by value deliberately so that a copy is passed
> +	 * when this is run in another thread by invokeMethod().

Possibly something to address later, by storing ISP parameters in
per-frame buffers like we do for hardware ISPs.

> +	 */
> +	virtual void process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0;
> +
> +	/**
> +	 * \brief Get the supported output sizes for the given input format and size.
> +	 * \param[in] inputFormat The input format.
> +	 * \param[in] inputSize The input size.
> +	 *
> +	 * \return The valid size ranges or an empty range if there are none.
> +	 */
> +	virtual SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) = 0;
> +
> +	/**
> +	 * \brief Signals when the input buffer is ready.
> +	 */
> +	Signal<FrameBuffer *> inputBufferReady;
> +
> +	/**
> +	 * \brief Signals when the output buffer is ready.
> +	 */
> +	Signal<FrameBuffer *> outputBufferReady;

Do you envision that we would have backends that could emit those
signals at different times ? If not you could combine them into a single
signal, that would be more efficient.

> +};
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/software_isp/meson.build b/src/libcamera/software_isp/meson.build
> index fcfff74a..62095f61 100644
> --- a/src/libcamera/software_isp/meson.build
> +++ b/src/libcamera/software_isp/meson.build
> @@ -8,5 +8,6 @@ if not (softisp_enabled)
>  endif
>  
>  libcamera_sources += files([
> +    'debayer.cpp',
>      'swstats_cpu.cpp',
>  ])

-- 
Regards,

Laurent Pinchart