[libcamera-devel,v2,06/18] libcamera: introduce SoftwareIsp class

19396 diff mbox series

From: Andrey Konovalov <andrey.konovalov@linaro.org>

Doxygen documentation by Dennis Bonke.

Co-authored-by: Dennis Bonke <admin@dennisbonke.com>
Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
Tested-by: Pavel Machek <pavel@ucw.cz>
---
 include/libcamera/internal/meson.build    |   1 +
 include/libcamera/internal/software_isp.h | 231 ++++++++++++++++++++++
 src/libcamera/meson.build                 |   1 +
 src/libcamera/software_isp.cpp            |  62 ++++++
 4 files changed, 295 insertions(+)
 create mode 100644 include/libcamera/internal/software_isp.h
 create mode 100644 src/libcamera/software_isp.cpp

Hans de Goede <hdegoede@redhat.com> writes:

> From: Andrey Konovalov <andrey.konovalov@linaro.org>
>
> Doxygen documentation by Dennis Bonke.
>
> Co-authored-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel@ucw.cz>

I don't feel like being able to say anything intelligent to this, sorry, just a
couple of typos:

> ---
>  include/libcamera/internal/meson.build    |   1 +
>  include/libcamera/internal/software_isp.h | 231 ++++++++++++++++++++++
>  src/libcamera/meson.build                 |   1 +
>  src/libcamera/software_isp.cpp            |  62 ++++++
>  4 files changed, 295 insertions(+)
>  create mode 100644 include/libcamera/internal/software_isp.h
>  create mode 100644 src/libcamera/software_isp.cpp
>
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 5807dfd9..1325941d 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -40,6 +40,7 @@ libcamera_internal_headers = files([
>      'pub_key.h',
>      'request.h',
>      'shared_mem_object.h',
> +    'software_isp.h',
>      'source_paths.h',
>      'sysfs.h',
>      'v4l2_device.h',
> diff --git a/include/libcamera/internal/software_isp.h b/include/libcamera/internal/software_isp.h
> new file mode 100644
> index 00000000..42ff48ec
> --- /dev/null
> +++ b/include/libcamera/internal/software_isp.h
> @@ -0,0 +1,231 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Linaro Ltd
> + *
> + * software_isp.h - Interface for a software implementation of an ISP
> + */
> +
> +#pragma once
> +
> +#include <functional>
> +#include <initializer_list>
> +#include <map>
> +#include <memory>
> +#include <string>
> +#include <tuple>
> +#include <vector>
> +
> +#include <libcamera/base/class.h>
> +#include <libcamera/base/log.h>
> +#include <libcamera/base/signal.h>
> +
> +#include <libcamera/geometry.h>
> +
> +#include "libcamera/internal/pipeline_handler.h"
> +
> +namespace libcamera {
> +
> +class FrameBuffer;
> +class PixelFormat;
> +struct StreamConfiguration;
> +
> +LOG_DECLARE_CATEGORY(SoftwareIsp)
> +
> +/**
> + * \brief Base class for the Software ISP.
> + *
> + * Base class of the SoftwareIsp interface.
> + */
> +class SoftwareIsp
> +{
> +public:
> +	/**
> +	 * \brief Constructor for the SoftwareIsp object.
> +	 * \param[in] pipe The pipeline handler in use.
> +	 * \param[in] sensorControls The sensor controls.
> +	 */
> +	SoftwareIsp(PipelineHandler *pipe, const ControlInfoMap &sensorControls);
> +	virtual ~SoftwareIsp();
> +
> +	/**
> +	 * \brief Load a configuration from a file.
> +	 * \param[in] filename The file to load from.
> +	 *
> +	 * \return 0 on success.
> +	 */
> +	virtual int loadConfiguration(const std::string &filename) = 0;
> +
> +	/**
> +	 * \brief Gets if there is a valid debayer object.
> +	 *
> +	 * \returns true if there is, false otherwise.
> +	 */
> +	virtual bool isValid() const = 0;
> +
> +	/**
> +	 * \brief Get the supported output formats.
> +	 * \param[in] input The input format.
> +	 *
> +	 * \return all supported output formats or an empty vector if there are none.
> +	 */
> +	virtual std::vector<PixelFormat> formats(PixelFormat input) = 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 Get the stride and the frame size.
> +	 * \param[in] pixelFormat 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.
> +	 */
> +	virtual std::tuple<unsigned int, unsigned int>
> +	strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size) = 0;
> +
> +	/**
> +	 * \brief Configure the SwIspSimple object according to the passed in parameters.
> +	 * \param[in] inputCfg The input configuration.
> +	 * \param[in] outputCfgs The output configurations.
> +	 * \param[in] sensorControls The sensor controls.
> +	 *
> +	 * \return 0 on success, a negative errno on failure.
> +	 */
> +	virtual int configure(const StreamConfiguration &inputCfg,
> +			      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,
> +			      const ControlInfoMap &sensorControls) = 0;
> +
> +	/**
> +	 * \brief Exports the buffers for use in processing.
> +	 * \param[in] output The number of outputs requested.
> +	 * \param[in] count The number of planes.
> +	 * \param[out] buffers The exported buffers.
> +	 *
> +	 * \return count when successful, a negative return value if an error occurred.
> +	 */
> +	virtual int exportBuffers(unsigned int output, unsigned int count,
> +				  std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
> +
> +	/**
> +	 * \brief Starts the Software ISP worker.
> +	 *
> +	 * \return 0 on success, any other value indicates an error.
> +	 */
> +	virtual int start() = 0;
> +
> +	/**
> +	 * \brief Stops the Software ISP worker.
> +	 */
> +	virtual void stop() = 0;
> +
> +	/**
> +	 * \brief Queues buffers for processing.
> +	 * \param[in] input The input framebuffer.
> +	 * \param[in] outputs The output framebuffers.
> +	 *
> +	 * \return 0 on success, a negative errno on failure
> +	 */
> +	virtual int queueBuffers(FrameBuffer *input,
> +				 const std::map<unsigned int, FrameBuffer *> &outputs) = 0;
> +
> +	/**
> +	 * \brief Process the statistics gathered.
> +	 * \param[in] sensorControls The sensor controls.
> +	 */
> +	virtual void processStats(const ControlList &sensorControls) = 0; // rather merge with queueBuffers()?
> +
> +	/**
> +	 * \brief Get the signal for when the sensor controls are set.
> +	 *
> +	 * \return The control list of the sensor controls.
> +	 */
> +	virtual Signal<const ControlList &> &getSignalSetSensorControls() = 0;
> +
> +	/**
> +	 * \brief Signals that the input buffer is ready.
> +	 */
> +	Signal<FrameBuffer *> inputBufferReady;
> +	/**
> +	 * \brief Signals that the output buffer is ready.
> +	 */
> +	Signal<FrameBuffer *> outputBufferReady;
> +
> +	/**
> +	 * \brief Signals that the ISP stats are ready.
> +	 *
> +	 * The int parameter isn't actually used.
> +	 */
> +	Signal<int> ispStatsReady;
> +};
> +
> +/**
> + * \brief Base class for the Software ISP Factory.
> + *
> + * Base class of the SoftwareIsp Factory.

Is it necessary to repeat the brief section (with a slightly different wording)
or can this sentence be omitted?

> + */
> +class SoftwareIspFactoryBase
> +{
> +public:
> +	SoftwareIspFactoryBase();
> +	virtual ~SoftwareIspFactoryBase() = default;
> +
> +	/**
> +	 * \brief Creates a SoftwareIsp object.
> +	 * \param[in] pipe The pipeline handler in use.
> +	 * \param[in] sensorControls The sensor controls.
> +	 *
> +	 * \return An unique pointer to the created SoftwareIsp object.

A unique

> +	 */
> +	static std::unique_ptr<SoftwareIsp> create(PipelineHandler *pipe,
> +						   const ControlInfoMap &sensorControls);
> +	/**
> +	 * \brief Gives back a pointer to the factory.
> +	 *
> +	 * \return A static pointer to the factory instance.
> +	 */
> +	static SoftwareIspFactoryBase *&factory();
> +
> +private:
> +	LIBCAMERA_DISABLE_COPY_AND_MOVE(SoftwareIspFactoryBase)
> +
> +	static void registerType(SoftwareIspFactoryBase *factory);
> +	virtual std::unique_ptr<SoftwareIsp> createInstance(PipelineHandler *pipe,
> +							    const ControlInfoMap &sensorControls) const = 0;
> +};
> +
> +/**
> + * \brief Implementation for the Software ISP Factory.
> + */
> +template<typename _SoftwareIsp>
> +class SoftwareIspFactory : public SoftwareIspFactoryBase
> +{
> +public:
> +	SoftwareIspFactory()
> +		: SoftwareIspFactoryBase()
> +	{
> +	}
> +
> +	/**
> +	 * \brief Creates an instance of a SoftwareIsp object.
> +	 * \param[in] pipe The pipeline handler in use.
> +	 * \param[in] sensorControls The sensor controls.
> +	 *
> +	 * \return An unique pointer to the created SoftwareIsp object.

A unique

> +	 */
> +	std::unique_ptr<SoftwareIsp> createInstance(PipelineHandler *pipe,
> +						    const ControlInfoMap &sensorControls) const override
> +	{
> +		return std::make_unique<_SoftwareIsp>(pipe, sensorControls);
> +	}
> +};
> +
> +#define REGISTER_SOFTWAREISP(softwareIsp) \
> +	static SoftwareIspFactory<softwareIsp> global_##softwareIsp##Factory;
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 3c5e43df..86494663 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -41,6 +41,7 @@ libcamera_sources = files([
>      'process.cpp',
>      'pub_key.cpp',
>      'request.cpp',
> +    'software_isp.cpp',
>      'source_paths.cpp',
>      'stream.cpp',
>      'sysfs.cpp',
> diff --git a/src/libcamera/software_isp.cpp b/src/libcamera/software_isp.cpp
> new file mode 100644
> index 00000000..2ff97d70
> --- /dev/null
> +++ b/src/libcamera/software_isp.cpp
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Linaro Ltd
> + *
> + * software_isp.cpp - Interface for a software implementation of an ISP
> + */
> +
> +#include "libcamera/internal/software_isp.h"
> +
> +#include <libcamera/base/log.h>
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(SoftwareIsp)
> +
> +SoftwareIsp::SoftwareIsp([[maybe_unused]] PipelineHandler *pipe,
> +			 [[maybe_unused]] const ControlInfoMap &sensorControls)
> +{
> +}
> +
> +SoftwareIsp::~SoftwareIsp()
> +{
> +}
> +
> +/* SoftwareIspFactoryBase */
> +
> +SoftwareIspFactoryBase::SoftwareIspFactoryBase()
> +{
> +	registerType(this);
> +}
> +
> +void SoftwareIspFactoryBase::registerType(SoftwareIspFactoryBase *factory)
> +{
> +	SoftwareIspFactoryBase *&registered =
> +		SoftwareIspFactoryBase::factory();
> +
> +	ASSERT(!registered && factory);
> +	registered = factory;
> +}
> +
> +SoftwareIspFactoryBase *&SoftwareIspFactoryBase::factory()
> +{
> +	static SoftwareIspFactoryBase *factory;
> +	return factory;
> +}
> +
> +std::unique_ptr<SoftwareIsp>
> +SoftwareIspFactoryBase::create(PipelineHandler *pipe,
> +			       const ControlInfoMap &sensorControls)
> +{
> +	SoftwareIspFactoryBase *factory = SoftwareIspFactoryBase::factory();
> +	if (!factory)
> +		return nullptr;
> +
> +	std::unique_ptr<SoftwareIsp> swIsp = factory->createInstance(pipe, sensorControls);
> +	if (swIsp->isValid())
> +		return swIsp;
> +
> +	return nullptr;
> +}
> +
> +} /* namespace libcamera */

Hi All,

As an early note for the reviewers.

As part of the rework to use "-Dpipelines=simple -Dipas=simple" and to instantiate
the Soft ISP if enabled in the SimplePipelineInfo table, in the next version of my
patches Soft ISP factory will be dropped, IPASoftBase will be merged into IPASoftSimple,
and SwIspSimple will be merged into SoftwareIsp.

Thanks,
Andrei

On 13.01.2024 17:22, Hans de Goede wrote:
> From: Andrey Konovalov <andrey.konovalov@linaro.org>
> 
> Doxygen documentation by Dennis Bonke.
> 
> Co-authored-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel@ucw.cz>
> ---
>   include/libcamera/internal/meson.build    |   1 +
>   include/libcamera/internal/software_isp.h | 231 ++++++++++++++++++++++
>   src/libcamera/meson.build                 |   1 +
>   src/libcamera/software_isp.cpp            |  62 ++++++
>   4 files changed, 295 insertions(+)
>   create mode 100644 include/libcamera/internal/software_isp.h
>   create mode 100644 src/libcamera/software_isp.cpp
> 
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 5807dfd9..1325941d 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -40,6 +40,7 @@ libcamera_internal_headers = files([
>       'pub_key.h',
>       'request.h',
>       'shared_mem_object.h',
> +    'software_isp.h',
>       'source_paths.h',
>       'sysfs.h',
>       'v4l2_device.h',
> diff --git a/include/libcamera/internal/software_isp.h b/include/libcamera/internal/software_isp.h
> new file mode 100644
> index 00000000..42ff48ec
> --- /dev/null
> +++ b/include/libcamera/internal/software_isp.h
> @@ -0,0 +1,231 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Linaro Ltd
> + *
> + * software_isp.h - Interface for a software implementation of an ISP
> + */
> +
> +#pragma once
> +
> +#include <functional>
> +#include <initializer_list>
> +#include <map>
> +#include <memory>
> +#include <string>
> +#include <tuple>
> +#include <vector>
> +
> +#include <libcamera/base/class.h>
> +#include <libcamera/base/log.h>
> +#include <libcamera/base/signal.h>
> +
> +#include <libcamera/geometry.h>
> +
> +#include "libcamera/internal/pipeline_handler.h"
> +
> +namespace libcamera {
> +
> +class FrameBuffer;
> +class PixelFormat;
> +struct StreamConfiguration;
> +
> +LOG_DECLARE_CATEGORY(SoftwareIsp)
> +
> +/**
> + * \brief Base class for the Software ISP.
> + *
> + * Base class of the SoftwareIsp interface.
> + */
> +class SoftwareIsp
> +{
> +public:
> +	/**
> +	 * \brief Constructor for the SoftwareIsp object.
> +	 * \param[in] pipe The pipeline handler in use.
> +	 * \param[in] sensorControls The sensor controls.
> +	 */
> +	SoftwareIsp(PipelineHandler *pipe, const ControlInfoMap &sensorControls);
> +	virtual ~SoftwareIsp();
> +
> +	/**
> +	 * \brief Load a configuration from a file.
> +	 * \param[in] filename The file to load from.
> +	 *
> +	 * \return 0 on success.
> +	 */
> +	virtual int loadConfiguration(const std::string &filename) = 0;
> +
> +	/**
> +	 * \brief Gets if there is a valid debayer object.
> +	 *
> +	 * \returns true if there is, false otherwise.
> +	 */
> +	virtual bool isValid() const = 0;
> +
> +	/**
> +	 * \brief Get the supported output formats.
> +	 * \param[in] input The input format.
> +	 *
> +	 * \return all supported output formats or an empty vector if there are none.
> +	 */
> +	virtual std::vector<PixelFormat> formats(PixelFormat input) = 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 Get the stride and the frame size.
> +	 * \param[in] pixelFormat 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.
> +	 */
> +	virtual std::tuple<unsigned int, unsigned int>
> +	strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size) = 0;
> +
> +	/**
> +	 * \brief Configure the SwIspSimple object according to the passed in parameters.
> +	 * \param[in] inputCfg The input configuration.
> +	 * \param[in] outputCfgs The output configurations.
> +	 * \param[in] sensorControls The sensor controls.
> +	 *
> +	 * \return 0 on success, a negative errno on failure.
> +	 */
> +	virtual int configure(const StreamConfiguration &inputCfg,
> +			      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,
> +			      const ControlInfoMap &sensorControls) = 0;
> +
> +	/**
> +	 * \brief Exports the buffers for use in processing.
> +	 * \param[in] output The number of outputs requested.
> +	 * \param[in] count The number of planes.
> +	 * \param[out] buffers The exported buffers.
> +	 *
> +	 * \return count when successful, a negative return value if an error occurred.
> +	 */
> +	virtual int exportBuffers(unsigned int output, unsigned int count,
> +				  std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
> +
> +	/**
> +	 * \brief Starts the Software ISP worker.
> +	 *
> +	 * \return 0 on success, any other value indicates an error.
> +	 */
> +	virtual int start() = 0;
> +
> +	/**
> +	 * \brief Stops the Software ISP worker.
> +	 */
> +	virtual void stop() = 0;
> +
> +	/**
> +	 * \brief Queues buffers for processing.
> +	 * \param[in] input The input framebuffer.
> +	 * \param[in] outputs The output framebuffers.
> +	 *
> +	 * \return 0 on success, a negative errno on failure
> +	 */
> +	virtual int queueBuffers(FrameBuffer *input,
> +				 const std::map<unsigned int, FrameBuffer *> &outputs) = 0;
> +
> +	/**
> +	 * \brief Process the statistics gathered.
> +	 * \param[in] sensorControls The sensor controls.
> +	 */
> +	virtual void processStats(const ControlList &sensorControls) = 0; // rather merge with queueBuffers()?
> +
> +	/**
> +	 * \brief Get the signal for when the sensor controls are set.
> +	 *
> +	 * \return The control list of the sensor controls.
> +	 */
> +	virtual Signal<const ControlList &> &getSignalSetSensorControls() = 0;
> +
> +	/**
> +	 * \brief Signals that the input buffer is ready.
> +	 */
> +	Signal<FrameBuffer *> inputBufferReady;
> +	/**
> +	 * \brief Signals that the output buffer is ready.
> +	 */
> +	Signal<FrameBuffer *> outputBufferReady;
> +
> +	/**
> +	 * \brief Signals that the ISP stats are ready.
> +	 *
> +	 * The int parameter isn't actually used.
> +	 */
> +	Signal<int> ispStatsReady;
> +};
> +
> +/**
> + * \brief Base class for the Software ISP Factory.
> + *
> + * Base class of the SoftwareIsp Factory.
> + */
> +class SoftwareIspFactoryBase
> +{
> +public:
> +	SoftwareIspFactoryBase();
> +	virtual ~SoftwareIspFactoryBase() = default;
> +
> +	/**
> +	 * \brief Creates a SoftwareIsp object.
> +	 * \param[in] pipe The pipeline handler in use.
> +	 * \param[in] sensorControls The sensor controls.
> +	 *
> +	 * \return An unique pointer to the created SoftwareIsp object.
> +	 */
> +	static std::unique_ptr<SoftwareIsp> create(PipelineHandler *pipe,
> +						   const ControlInfoMap &sensorControls);
> +	/**
> +	 * \brief Gives back a pointer to the factory.
> +	 *
> +	 * \return A static pointer to the factory instance.
> +	 */
> +	static SoftwareIspFactoryBase *&factory();
> +
> +private:
> +	LIBCAMERA_DISABLE_COPY_AND_MOVE(SoftwareIspFactoryBase)
> +
> +	static void registerType(SoftwareIspFactoryBase *factory);
> +	virtual std::unique_ptr<SoftwareIsp> createInstance(PipelineHandler *pipe,
> +							    const ControlInfoMap &sensorControls) const = 0;
> +};
> +
> +/**
> + * \brief Implementation for the Software ISP Factory.
> + */
> +template<typename _SoftwareIsp>
> +class SoftwareIspFactory : public SoftwareIspFactoryBase
> +{
> +public:
> +	SoftwareIspFactory()
> +		: SoftwareIspFactoryBase()
> +	{
> +	}
> +
> +	/**
> +	 * \brief Creates an instance of a SoftwareIsp object.
> +	 * \param[in] pipe The pipeline handler in use.
> +	 * \param[in] sensorControls The sensor controls.
> +	 *
> +	 * \return An unique pointer to the created SoftwareIsp object.
> +	 */
> +	std::unique_ptr<SoftwareIsp> createInstance(PipelineHandler *pipe,
> +						    const ControlInfoMap &sensorControls) const override
> +	{
> +		return std::make_unique<_SoftwareIsp>(pipe, sensorControls);
> +	}
> +};
> +
> +#define REGISTER_SOFTWAREISP(softwareIsp) \
> +	static SoftwareIspFactory<softwareIsp> global_##softwareIsp##Factory;
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 3c5e43df..86494663 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -41,6 +41,7 @@ libcamera_sources = files([
>       'process.cpp',
>       'pub_key.cpp',
>       'request.cpp',
> +    'software_isp.cpp',
>       'source_paths.cpp',
>       'stream.cpp',
>       'sysfs.cpp',
> diff --git a/src/libcamera/software_isp.cpp b/src/libcamera/software_isp.cpp
> new file mode 100644
> index 00000000..2ff97d70
> --- /dev/null
> +++ b/src/libcamera/software_isp.cpp
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Linaro Ltd
> + *
> + * software_isp.cpp - Interface for a software implementation of an ISP
> + */
> +
> +#include "libcamera/internal/software_isp.h"
> +
> +#include <libcamera/base/log.h>
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(SoftwareIsp)
> +
> +SoftwareIsp::SoftwareIsp([[maybe_unused]] PipelineHandler *pipe,
> +			 [[maybe_unused]] const ControlInfoMap &sensorControls)
> +{
> +}
> +
> +SoftwareIsp::~SoftwareIsp()
> +{
> +}
> +
> +/* SoftwareIspFactoryBase */
> +
> +SoftwareIspFactoryBase::SoftwareIspFactoryBase()
> +{
> +	registerType(this);
> +}
> +
> +void SoftwareIspFactoryBase::registerType(SoftwareIspFactoryBase *factory)
> +{
> +	SoftwareIspFactoryBase *&registered =
> +		SoftwareIspFactoryBase::factory();
> +
> +	ASSERT(!registered && factory);
> +	registered = factory;
> +}
> +
> +SoftwareIspFactoryBase *&SoftwareIspFactoryBase::factory()
> +{
> +	static SoftwareIspFactoryBase *factory;
> +	return factory;
> +}
> +
> +std::unique_ptr<SoftwareIsp>
> +SoftwareIspFactoryBase::create(PipelineHandler *pipe,
> +			       const ControlInfoMap &sensorControls)
> +{
> +	SoftwareIspFactoryBase *factory = SoftwareIspFactoryBase::factory();
> +	if (!factory)
> +		return nullptr;
> +
> +	std::unique_ptr<SoftwareIsp> swIsp = factory->createInstance(pipe, sensorControls);
> +	if (swIsp->isValid())
> +		return swIsp;
> +
> +	return nullptr;
> +}
> +
> +} /* namespace libcamera */

Hello,

On Tue, Jan 23, 2024 at 02:44:01PM +0100, Milan Zamazal wrote:
> Hans de Goede <hdegoede@redhat.com> writes:
> 
> > From: Andrey Konovalov <andrey.konovalov@linaro.org>
> >
> > Doxygen documentation by Dennis Bonke.
> >
> > Co-authored-by: Dennis Bonke <admin@dennisbonke.com>
> > Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> > Tested-by: Pavel Machek <pavel@ucw.cz>
> 
> I don't feel like being able to say anything intelligent to this, sorry, just a
> couple of typos:
> 
> > ---
> >  include/libcamera/internal/meson.build    |   1 +
> >  include/libcamera/internal/software_isp.h | 231 ++++++++++++++++++++++
> >  src/libcamera/meson.build                 |   1 +
> >  src/libcamera/software_isp.cpp            |  62 ++++++
> >  4 files changed, 295 insertions(+)
> >  create mode 100644 include/libcamera/internal/software_isp.h
> >  create mode 100644 src/libcamera/software_isp.cpp
> >
> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > index 5807dfd9..1325941d 100644
> > --- a/include/libcamera/internal/meson.build
> > +++ b/include/libcamera/internal/meson.build
> > @@ -40,6 +40,7 @@ libcamera_internal_headers = files([
> >      'pub_key.h',
> >      'request.h',
> >      'shared_mem_object.h',
> > +    'software_isp.h',
> >      'source_paths.h',
> >      'sysfs.h',
> >      'v4l2_device.h',
> > diff --git a/include/libcamera/internal/software_isp.h b/include/libcamera/internal/software_isp.h
> > new file mode 100644
> > index 00000000..42ff48ec
> > --- /dev/null
> > +++ b/include/libcamera/internal/software_isp.h
> > @@ -0,0 +1,231 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2023, Linaro Ltd
> > + *
> > + * software_isp.h - Interface for a software implementation of an ISP
> > + */
> > +
> > +#pragma once
> > +
> > +#include <functional>
> > +#include <initializer_list>
> > +#include <map>
> > +#include <memory>
> > +#include <string>
> > +#include <tuple>
> > +#include <vector>
> > +
> > +#include <libcamera/base/class.h>
> > +#include <libcamera/base/log.h>
> > +#include <libcamera/base/signal.h>
> > +
> > +#include <libcamera/geometry.h>
> > +
> > +#include "libcamera/internal/pipeline_handler.h"
> > +
> > +namespace libcamera {
> > +
> > +class FrameBuffer;
> > +class PixelFormat;
> > +struct StreamConfiguration;
> > +
> > +LOG_DECLARE_CATEGORY(SoftwareIsp)
> > +
> > +/**
> > + * \brief Base class for the Software ISP.
> > + *
> > + * Base class of the SoftwareIsp interface.
> > + */

Documentation goes to .cpp files.

> > +class SoftwareIsp
> > +{
> > +public:
> > +	/**
> > +	 * \brief Constructor for the SoftwareIsp object.
> > +	 * \param[in] pipe The pipeline handler in use.
> > +	 * \param[in] sensorControls The sensor controls.

Please see existing documentation and follow the same style. No period
ad the end of lines for the brief and param.

> > +	 */
> > +	SoftwareIsp(PipelineHandler *pipe, const ControlInfoMap &sensorControls);
> > +	virtual ~SoftwareIsp();
> > +
> > +	/**
> > +	 * \brief Load a configuration from a file.
> > +	 * \param[in] filename The file to load from.
> > +	 *
> > +	 * \return 0 on success.
> > +	 */
> > +	virtual int loadConfiguration(const std::string &filename) = 0;
> > +
> > +	/**
> > +	 * \brief Gets if there is a valid debayer object.

Reading this patch I don't know what a debayer object is, what it means
to have a valid one, or how it could be invalid.

> > +	 *
> > +	 * \returns true if there is, false otherwise.

s/true/True/

Similar comments below.

It's \return, not \returns. Have you compiled the documentation and
fixed all warnings/errors ?

> > +	 */
> > +	virtual bool isValid() const = 0;
> > +
> > +	/**
> > +	 * \brief Get the supported output formats.
> > +	 * \param[in] input The input format.
> > +	 *
> > +	 * \return all supported output formats or an empty vector if there are none.
> > +	 */
> > +	virtual std::vector<PixelFormat> formats(PixelFormat input) = 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 Get the stride and the frame size.
> > +	 * \param[in] pixelFormat 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.
> > +	 */
> > +	virtual std::tuple<unsigned int, unsigned int>
> > +	strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size) = 0;
> > +
> > +	/**
> > +	 * \brief Configure the SwIspSimple object according to the passed in parameters.
> > +	 * \param[in] inputCfg The input configuration.
> > +	 * \param[in] outputCfgs The output configurations.
> > +	 * \param[in] sensorControls The sensor controls.
> > +	 *
> > +	 * \return 0 on success, a negative errno on failure.
> > +	 */
> > +	virtual int configure(const StreamConfiguration &inputCfg,
> > +			      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,
> > +			      const ControlInfoMap &sensorControls) = 0;
> > +
> > +	/**
> > +	 * \brief Exports the buffers for use in processing.
> > +	 * \param[in] output The number of outputs requested.
> > +	 * \param[in] count The number of planes.
> > +	 * \param[out] buffers The exported buffers.
> > +	 *
> > +	 * \return count when successful, a negative return value if an error occurred.
> > +	 */
> > +	virtual int exportBuffers(unsigned int output, unsigned int count,
> > +				  std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
> > +
> > +	/**
> > +	 * \brief Starts the Software ISP worker.
> > +	 *
> > +	 * \return 0 on success, any other value indicates an error.
> > +	 */
> > +	virtual int start() = 0;
> > +
> > +	/**
> > +	 * \brief Stops the Software ISP worker.
> > +	 */
> > +	virtual void stop() = 0;
> > +
> > +	/**
> > +	 * \brief Queues buffers for processing.
> > +	 * \param[in] input The input framebuffer.
> > +	 * \param[in] outputs The output framebuffers.
> > +	 *
> > +	 * \return 0 on success, a negative errno on failure
> > +	 */
> > +	virtual int queueBuffers(FrameBuffer *input,
> > +				 const std::map<unsigned int, FrameBuffer *> &outputs) = 0;
> > +
> > +	/**
> > +	 * \brief Process the statistics gathered.
> > +	 * \param[in] sensorControls The sensor controls.
> > +	 */
> > +	virtual void processStats(const ControlList &sensorControls) = 0; // rather merge with queueBuffers()?

This comment needs to be addressed or dropped.

> > +
> > +	/**
> > +	 * \brief Get the signal for when the sensor controls are set.
> > +	 *
> > +	 * \return The control list of the sensor controls.
> > +	 */
> > +	virtual Signal<const ControlList &> &getSignalSetSensorControls() = 0;

Signals should be named according to the event they represent. For
instance, this may be

	virtual Signal<const ControlList &> &ensorControlsReady() = 0;

But why does it have to be a function, when the next three signals are
plain Signal instances ?

> > +
> > +	/**
> > +	 * \brief Signals that the input buffer is ready.
> > +	 */
> > +	Signal<FrameBuffer *> inputBufferReady;
> > +	/**
> > +	 * \brief Signals that the output buffer is ready.
> > +	 */
> > +	Signal<FrameBuffer *> outputBufferReady;
> > +
> > +	/**
> > +	 * \brief Signals that the ISP stats are ready.
> > +	 *
> > +	 * The int parameter isn't actually used.
> > +	 */
> > +	Signal<int> ispStatsReady;
> > +};
> > +
> > +/**
> > + * \brief Base class for the Software ISP Factory.
> > + *
> > + * Base class of the SoftwareIsp Factory.
> 
> Is it necessary to repeat the brief section (with a slightly different wording)
> or can this sentence be omitted?

It doesn't have to be repeated, and could be omitted. Expect that it
should instead be expanded to explain how the class works :-)
Documentation is overall too terse in this file.

> > + */
> > +class SoftwareIspFactoryBase
> > +{
> > +public:
> > +	SoftwareIspFactoryBase();
> > +	virtual ~SoftwareIspFactoryBase() = default;
> > +
> > +	/**
> > +	 * \brief Creates a SoftwareIsp object.
> > +	 * \param[in] pipe The pipeline handler in use.
> > +	 * \param[in] sensorControls The sensor controls.
> > +	 *
> > +	 * \return An unique pointer to the created SoftwareIsp object.
> 
> A unique
> 
> > +	 */
> > +	static std::unique_ptr<SoftwareIsp> create(PipelineHandler *pipe,
> > +						   const ControlInfoMap &sensorControls);
> > +	/**
> > +	 * \brief Gives back a pointer to the factory.
> > +	 *
> > +	 * \return A static pointer to the factory instance.
> > +	 */
> > +	static SoftwareIspFactoryBase *&factory();
> > +
> > +private:
> > +	LIBCAMERA_DISABLE_COPY_AND_MOVE(SoftwareIspFactoryBase)
> > +
> > +	static void registerType(SoftwareIspFactoryBase *factory);
> > +	virtual std::unique_ptr<SoftwareIsp> createInstance(PipelineHandler *pipe,
> > +							    const ControlInfoMap &sensorControls) const = 0;
> > +};
> > +
> > +/**
> > + * \brief Implementation for the Software ISP Factory.
> > + */
> > +template<typename _SoftwareIsp>
> > +class SoftwareIspFactory : public SoftwareIspFactoryBase
> > +{
> > +public:
> > +	SoftwareIspFactory()
> > +		: SoftwareIspFactoryBase()
> > +	{
> > +	}
> > +
> > +	/**
> > +	 * \brief Creates an instance of a SoftwareIsp object.
> > +	 * \param[in] pipe The pipeline handler in use.
> > +	 * \param[in] sensorControls The sensor controls.
> > +	 *
> > +	 * \return An unique pointer to the created SoftwareIsp object.
> 
> A unique
> 
> > +	 */
> > +	std::unique_ptr<SoftwareIsp> createInstance(PipelineHandler *pipe,
> > +						    const ControlInfoMap &sensorControls) const override
> > +	{
> > +		return std::make_unique<_SoftwareIsp>(pipe, sensorControls);
> > +	}
> > +};
> > +
> > +#define REGISTER_SOFTWAREISP(softwareIsp) \
> > +	static SoftwareIspFactory<softwareIsp> global_##softwareIsp##Factory;
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 3c5e43df..86494663 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -41,6 +41,7 @@ libcamera_sources = files([
> >      'process.cpp',
> >      'pub_key.cpp',
> >      'request.cpp',
> > +    'software_isp.cpp',
> >      'source_paths.cpp',
> >      'stream.cpp',
> >      'sysfs.cpp',
> > diff --git a/src/libcamera/software_isp.cpp b/src/libcamera/software_isp.cpp
> > new file mode 100644
> > index 00000000..2ff97d70
> > --- /dev/null
> > +++ b/src/libcamera/software_isp.cpp
> > @@ -0,0 +1,62 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2023, Linaro Ltd
> > + *
> > + * software_isp.cpp - Interface for a software implementation of an ISP
> > + */
> > +
> > +#include "libcamera/internal/software_isp.h"
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(SoftwareIsp)
> > +
> > +SoftwareIsp::SoftwareIsp([[maybe_unused]] PipelineHandler *pipe,
> > +			 [[maybe_unused]] const ControlInfoMap &sensorControls)
> > +{
> > +}
> > +
> > +SoftwareIsp::~SoftwareIsp()
> > +{
> > +}
> > +
> > +/* SoftwareIspFactoryBase */
> > +
> > +SoftwareIspFactoryBase::SoftwareIspFactoryBase()
> > +{
> > +	registerType(this);
> > +}
> > +
> > +void SoftwareIspFactoryBase::registerType(SoftwareIspFactoryBase *factory)
> > +{
> > +	SoftwareIspFactoryBase *&registered =
> > +		SoftwareIspFactoryBase::factory();
> > +
> > +	ASSERT(!registered && factory);
> > +	registered = factory;
> > +}
> > +
> > +SoftwareIspFactoryBase *&SoftwareIspFactoryBase::factory()
> > +{
> > +	static SoftwareIspFactoryBase *factory;
> > +	return factory;
> > +}
> > +
> > +std::unique_ptr<SoftwareIsp>
> > +SoftwareIspFactoryBase::create(PipelineHandler *pipe,
> > +			       const ControlInfoMap &sensorControls)
> > +{
> > +	SoftwareIspFactoryBase *factory = SoftwareIspFactoryBase::factory();
> > +	if (!factory)
> > +		return nullptr;
> > +
> > +	std::unique_ptr<SoftwareIsp> swIsp = factory->createInstance(pipe, sensorControls);
> > +	if (swIsp->isValid())
> > +		return swIsp;
> > +
> > +	return nullptr;
> > +}
> > +
> > +} /* namespace libcamera */

Hi Andrey, a few drive-by comments

On Sat, Jan 13, 2024 at 03:22:06PM +0100, Hans de Goede via libcamera-devel wrote:
> From: Andrey Konovalov <andrey.konovalov@linaro.org>
>
> Doxygen documentation by Dennis Bonke.
>
> Co-authored-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel@ucw.cz>
> ---
>  include/libcamera/internal/meson.build    |   1 +
>  include/libcamera/internal/software_isp.h | 231 ++++++++++++++++++++++
>  src/libcamera/meson.build                 |   1 +
>  src/libcamera/software_isp.cpp            |  62 ++++++
>  4 files changed, 295 insertions(+)
>  create mode 100644 include/libcamera/internal/software_isp.h
>  create mode 100644 src/libcamera/software_isp.cpp
>
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 5807dfd9..1325941d 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -40,6 +40,7 @@ libcamera_internal_headers = files([
>      'pub_key.h',
>      'request.h',
>      'shared_mem_object.h',
> +    'software_isp.h',
>      'source_paths.h',
>      'sysfs.h',
>      'v4l2_device.h',
> diff --git a/include/libcamera/internal/software_isp.h b/include/libcamera/internal/software_isp.h
> new file mode 100644
> index 00000000..42ff48ec
> --- /dev/null
> +++ b/include/libcamera/internal/software_isp.h
> @@ -0,0 +1,231 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Linaro Ltd
> + *
> + * software_isp.h - Interface for a software implementation of an ISP
> + */
> +
> +#pragma once
> +
> +#include <functional>
> +#include <initializer_list>
> +#include <map>
> +#include <memory>
> +#include <string>
> +#include <tuple>
> +#include <vector>
> +
> +#include <libcamera/base/class.h>
> +#include <libcamera/base/log.h>
> +#include <libcamera/base/signal.h>
> +
> +#include <libcamera/geometry.h>
> +
> +#include "libcamera/internal/pipeline_handler.h"
> +
> +namespace libcamera {
> +
> +class FrameBuffer;
> +class PixelFormat;
> +struct StreamConfiguration;
> +
> +LOG_DECLARE_CATEGORY(SoftwareIsp)

Do you need it here ? (this applies to the log.h inclusion)

> +
> +/**
> + * \brief Base class for the Software ISP.
> + *
> + * Base class of the SoftwareIsp interface.
> + */
> +class SoftwareIsp
> +{
> +public:
> +	/**
> +	 * \brief Constructor for the SoftwareIsp object.
> +	 * \param[in] pipe The pipeline handler in use.
> +	 * \param[in] sensorControls The sensor controls.
> +	 */
> +	SoftwareIsp(PipelineHandler *pipe, const ControlInfoMap &sensorControls);
> +	virtual ~SoftwareIsp();
> +
> +	/**
> +	 * \brief Load a configuration from a file.
> +	 * \param[in] filename The file to load from.
> +	 *
> +	 * \return 0 on success.
> +	 */
> +	virtual int loadConfiguration(const std::string &filename) = 0;
> +
> +	/**
> +	 * \brief Gets if there is a valid debayer object.
> +	 *
> +	 * \returns true if there is, false otherwise.
> +	 */
> +	virtual bool isValid() const = 0;
> +
> +	/**
> +	 * \brief Get the supported output formats.
> +	 * \param[in] input The input format.
> +	 *
> +	 * \return all supported output formats or an empty vector if there are none.
> +	 */
> +	virtual std::vector<PixelFormat> formats(PixelFormat input) = 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 Get the stride and the frame size.
> +	 * \param[in] pixelFormat 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.
> +	 */
> +	virtual std::tuple<unsigned int, unsigned int>
> +	strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size) = 0;
> +
> +	/**
> +	 * \brief Configure the SwIspSimple object according to the passed in parameters.
> +	 * \param[in] inputCfg The input configuration.
> +	 * \param[in] outputCfgs The output configurations.
> +	 * \param[in] sensorControls The sensor controls.
> +	 *
> +	 * \return 0 on success, a negative errno on failure.
> +	 */
> +	virtual int configure(const StreamConfiguration &inputCfg,
> +			      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,
> +			      const ControlInfoMap &sensorControls) = 0;

ControlInfoMap should be forward declared ? (as well as ControlList
below, so you could possibily include controls.h)

> +
> +	/**
> +	 * \brief Exports the buffers for use in processing.
> +	 * \param[in] output The number of outputs requested.
> +	 * \param[in] count The number of planes.
> +	 * \param[out] buffers The exported buffers.
> +	 *
> +	 * \return count when successful, a negative return value if an error occurred.
> +	 */
> +	virtual int exportBuffers(unsigned int output, unsigned int count,
> +				  std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
> +
> +	/**
> +	 * \brief Starts the Software ISP worker.
> +	 *
> +	 * \return 0 on success, any other value indicates an error.
> +	 */
> +	virtual int start() = 0;
> +
> +	/**
> +	 * \brief Stops the Software ISP worker.
> +	 */
> +	virtual void stop() = 0;
> +
> +	/**
> +	 * \brief Queues buffers for processing.
> +	 * \param[in] input The input framebuffer.
> +	 * \param[in] outputs The output framebuffers.
> +	 *
> +	 * \return 0 on success, a negative errno on failure
> +	 */
> +	virtual int queueBuffers(FrameBuffer *input,
> +				 const std::map<unsigned int, FrameBuffer *> &outputs) = 0;
> +
> +	/**
> +	 * \brief Process the statistics gathered.
> +	 * \param[in] sensorControls The sensor controls.
> +	 */
> +	virtual void processStats(const ControlList &sensorControls) = 0; // rather merge with queueBuffers()?
> +
> +	/**
> +	 * \brief Get the signal for when the sensor controls are set.
> +	 *
> +	 * \return The control list of the sensor controls.
> +	 */
> +	virtual Signal<const ControlList &> &getSignalSetSensorControls() = 0;
> +
> +	/**
> +	 * \brief Signals that the input buffer is ready.
> +	 */
> +	Signal<FrameBuffer *> inputBufferReady;
> +	/**
> +	 * \brief Signals that the output buffer is ready.
> +	 */
> +	Signal<FrameBuffer *> outputBufferReady;
> +
> +	/**
> +	 * \brief Signals that the ISP stats are ready.
> +	 *
> +	 * The int parameter isn't actually used.
> +	 */
> +	Signal<int> ispStatsReady;

Let's review the interface once seen in ation ;)

So far it seems it could be realized by extending the existing
Converter interface, we'll see..


> +};
> +
> +/**
> + * \brief Base class for the Software ISP Factory.
> + *
> + * Base class of the SoftwareIsp Factory.
> + */
> +class SoftwareIspFactoryBase
> +{
> +public:
> +	SoftwareIspFactoryBase();
> +	virtual ~SoftwareIspFactoryBase() = default;
> +
> +	/**
> +	 * \brief Creates a SoftwareIsp object.
> +	 * \param[in] pipe The pipeline handler in use.
> +	 * \param[in] sensorControls The sensor controls.
> +	 *
> +	 * \return An unique pointer to the created SoftwareIsp object.
> +	 */
> +	static std::unique_ptr<SoftwareIsp> create(PipelineHandler *pipe,
> +						   const ControlInfoMap &sensorControls);
> +	/**
> +	 * \brief Gives back a pointer to the factory.
> +	 *
> +	 * \return A static pointer to the factory instance.
> +	 */
> +	static SoftwareIspFactoryBase *&factory();
> +
> +private:
> +	LIBCAMERA_DISABLE_COPY_AND_MOVE(SoftwareIspFactoryBase)
> +
> +	static void registerType(SoftwareIspFactoryBase *factory);
> +	virtual std::unique_ptr<SoftwareIsp> createInstance(PipelineHandler *pipe,
> +							    const ControlInfoMap &sensorControls) const = 0;
> +};
> +
> +/**
> + * \brief Implementation for the Software ISP Factory.
> + */
> +template<typename _SoftwareIsp>
> +class SoftwareIspFactory : public SoftwareIspFactoryBase
> +{
> +public:
> +	SoftwareIspFactory()
> +		: SoftwareIspFactoryBase()
> +	{
> +	}
> +
> +	/**
> +	 * \brief Creates an instance of a SoftwareIsp object.
> +	 * \param[in] pipe The pipeline handler in use.
> +	 * \param[in] sensorControls The sensor controls.
> +	 *
> +	 * \return An unique pointer to the created SoftwareIsp object.
> +	 */
> +	std::unique_ptr<SoftwareIsp> createInstance(PipelineHandler *pipe,
> +						    const ControlInfoMap &sensorControls) const override
> +	{
> +		return std::make_unique<_SoftwareIsp>(pipe, sensorControls);
> +	}
> +};
> +
> +#define REGISTER_SOFTWAREISP(softwareIsp) \
> +	static SoftwareIspFactory<softwareIsp> global_##softwareIsp##Factory;
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 3c5e43df..86494663 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -41,6 +41,7 @@ libcamera_sources = files([
>      'process.cpp',
>      'pub_key.cpp',
>      'request.cpp',
> +    'software_isp.cpp',
>      'source_paths.cpp',
>      'stream.cpp',
>      'sysfs.cpp',
> diff --git a/src/libcamera/software_isp.cpp b/src/libcamera/software_isp.cpp
> new file mode 100644
> index 00000000..2ff97d70
> --- /dev/null
> +++ b/src/libcamera/software_isp.cpp
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Linaro Ltd
> + *
> + * software_isp.cpp - Interface for a software implementation of an ISP
> + */
> +
> +#include "libcamera/internal/software_isp.h"
> +
> +#include <libcamera/base/log.h>
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(SoftwareIsp)
> +
> +SoftwareIsp::SoftwareIsp([[maybe_unused]] PipelineHandler *pipe,
> +			 [[maybe_unused]] const ControlInfoMap &sensorControls)
> +{
> +}
> +
> +SoftwareIsp::~SoftwareIsp()
> +{
> +}
> +
> +/* SoftwareIspFactoryBase */
> +
> +SoftwareIspFactoryBase::SoftwareIspFactoryBase()
> +{
> +	registerType(this);
> +}
> +
> +void SoftwareIspFactoryBase::registerType(SoftwareIspFactoryBase *factory)
> +{
> +	SoftwareIspFactoryBase *&registered =
> +		SoftwareIspFactoryBase::factory();
> +
> +	ASSERT(!registered && factory);
> +	registered = factory;
> +}
> +
> +SoftwareIspFactoryBase *&SoftwareIspFactoryBase::factory()
> +{
> +	static SoftwareIspFactoryBase *factory;
> +	return factory;
> +}
> +
> +std::unique_ptr<SoftwareIsp>
> +SoftwareIspFactoryBase::create(PipelineHandler *pipe,
> +			       const ControlInfoMap &sensorControls)
> +{
> +	SoftwareIspFactoryBase *factory = SoftwareIspFactoryBase::factory();
> +	if (!factory)
> +		return nullptr;
> +
> +	std::unique_ptr<SoftwareIsp> swIsp = factory->createInstance(pipe, sensorControls);
> +	if (swIsp->isValid())
> +		return swIsp;
> +
> +	return nullptr;
> +}
> +
> +} /* namespace libcamera */
> --
> 2.43.0
>