[PATCH v2 06/18] libcamera: introduce SoftwareIsp class

Laurent Pinchart laurent.pinchart at ideasonboard.com
Tue Jan 23 15:57:31 CET 2024 

Hello,

On Tue, Jan 23, 2024 at 02:44:01PM +0100, Milan Zamazal wrote:
> Hans de Goede <hdegoede at redhat.com> writes:
> 
> > From: Andrey Konovalov <andrey.konovalov at linaro.org>
> >
> > Doxygen documentation by Dennis Bonke.
> >
> > Co-authored-by: Dennis Bonke <admin at dennisbonke.com>
> > Signed-off-by: Dennis Bonke <admin at dennisbonke.com>
> > 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>
> 
> 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 */

-- 
Regards,

Laurent Pinchart

More information about the libcamera-devel mailing list