[libcamera-devel] [PATCH v6 1/2] ipa: Create a camera sensor helper class

[Laurent Pinchart]

Hi Jean-Michel,

I know this has been merged already, I should have reviewed this series
sooner. Could you please address the issues pointed out below in a new
series ?

On Tue, Jun 22, 2021 at 11:45:22AM +0200, Jean-Michel Hautbois wrote:
> For various sensor operations, it may be needed to do sensor specific
> computations, like analogue gain or vertical blanking.
> 
> This commit introduces a new camera sensor helper in libipa which aims
> to solve this specific issue.
> It is based on the MIPI alliance Specification for Camera Command Set
> and implements, for now, only the analogue "Global gain" mode.
> Setting analogue gain for a specific sensor is not a straightforward
> operation, as one needs to know how the gain is calculated for it.
> 
> Three helpers are created in this patch: imx219, ov5670 and ov5693.
> 
> Adding a new sensor is pretty straightforward as one only needs to
> implement the sub-class for it and register that class to the
> CameraSensorHelperFactory.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/ipa/libipa/camera_sensor_helper.cpp | 318 ++++++++++++++++++++++++
>  src/ipa/libipa/camera_sensor_helper.h   |  87 +++++++
>  src/ipa/libipa/meson.build              |   2 +
>  3 files changed, 407 insertions(+)
>  create mode 100644 src/ipa/libipa/camera_sensor_helper.cpp
>  create mode 100644 src/ipa/libipa/camera_sensor_helper.h
> 
> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> new file mode 100644
> index 00000000..a114ee73
> --- /dev/null
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> @@ -0,0 +1,318 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * camera_sensor_helper.cpp - Helper class that performs sensor-specific parameter computations

Line break at 80 columns ? There are some long lines below that can be
easily broken at the 80 columns limit too (such as the \brief for the
CameraSensorClass for instance).

> + */
> +#include "camera_sensor_helper.h"
> +
> +#include "libcamera/internal/log.h"

This has turned into "libcamera/base/log.h" but it should be
<libcamera/base/log.h>.

> +
> +/**
> + * \file camera_sensor_helper.h
> + * \brief Helper class that performs sensor-specific parameter computations
> + *
> + * Computation of sensor configuration parameters is a sensor specific

s/sensor specific/sensor-specific/

> + * operation. Each CameraHelper derived class computes the value of
> + * configuration parameters, for example the analogue gain value, using
> + * sensor-specific functions and constants.
> + *
> + * Every subclass of CameraSensorHelper shall be registered with libipa using
> + * the REGISTER_CAMERA_SENSOR_HELPER() macro.
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(CameraSensorHelper)
> +
> +namespace ipa {
> +
> +/**
> + * \class CameraSensorHelper
> + * \brief Base class for computing sensor tuning parameters using sensor-specific constants
> + *
> + * CameraSensorHelper derived class instances are sensor specific. Each
> + * supported sensor will have an associated base class defined.
> + */
> +
> +/**
> + * \brief Construct a CameraSensorHelper instance
> + *
> + * CameraSensorHelper derived class instances shall never be constructed manually
> + * but always through the CameraSensorHelperFactory::create() method.
> + */
> +
> +/**
> + * \brief Compute gain code from the analogue gain absolute value
> + * \param[in] gain The real gain to pass
> + *
> + * This function aims to abstract the calculation of the gain letting the IPA
> + * use the real gain for its estimations.
> + *
> + * The parameters come from the MIPI Alliance Camera Specification for
> + * Camera Command Set (CCS).
> + *
> + * \return The gain code to pass to V4l2

s/V4l2/V4L2/

Same below.

It's however not very nice to make this aware of V4L2. I'll propose a
patch to improve the documentation, which will also define the concept
of gain code explicitly.

> + */
> +uint32_t CameraSensorHelper::gainCode(double gain) const
> +{
> +	ASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));

No need for the inner parentheses. Same below.

> +	ASSERT(analogueGainConstants_.type == AnalogueGainLinear);
> +
> +	return (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /
> +	       (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);
> +}
> +
> +/**
> + * \brief Compute the real gain from the V4l2 subdev control gain code
> + * \param[in] gainCode The V4l2 subdev control gain
> + *
> + * This function aims to abstract the calculation of the gain letting the IPA
> + * use the real gain for its estimations. It is the counterpart of the function
> + * CameraSensorHelper::gainCode.
> + *
> + * The parameters come from the MIPI Alliance Camera Specification for
> + * Camera Command Set (CCS).
> + *
> + * \return The real gain
> + */
> +double CameraSensorHelper::gain(uint32_t gainCode) const
> +{
> +	ASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));
> +	ASSERT(analogueGainConstants_.type == AnalogueGainLinear);
> +
> +	return (analogueGainConstants_.m0 * static_cast<double>(gainCode) + analogueGainConstants_.c0) /
> +	       (analogueGainConstants_.m1 * static_cast<double>(gainCode) + analogueGainConstants_.c1);
> +}
> +
> +/**
> + * \enum CameraSensorHelper::AnalogueGainType
> + * \brief the gain calculation modes as defined by the MIPI CCS
> + *
> + * Describes the image sensor analogue gain capabilities.
> + * Two modes are possible, depending on the sensor: Global and Alternate.

They're called Linear and Exponential here. Mentioning CCS for reference
is fine, but this is our implementation, with our names.

> + */
> +
> +/**
> + * \var CameraSensorHelper::AnalogueGainLinear
> + * \brief Gain is computed using linear gain estimation
> + *
> + * The relationship between the integer gain parameter and the resulting gain
> + * multiplier is given by the following equation:
> + *
> + * \f$gain=\frac{m0x+c0}{m1x+c1}\f$
> + *
> + * Where 'x' is the gain control parameter, and m0, m1, c0 and c1 are
> + * image-sensor-specific constants of the sensor.
> + * These constants are static parameters, and for any given image sensor either
> + * m0 or m1 shall be zero.
> + *
> + * The full Gain equation therefore reduces to either:
> + *
> + * \f$gain=\frac{c0}{m1x+c1}\f$ or \f$\frac{m0x+c0}{c1}\f$
> + */
> +
> +/**
> + * \var CameraSensorHelper::AnalogueGainExponential
> + * \brief Gain is computed using exponential gain estimation (introduced in CCS v1.1)
> + *
> + * Starting with CCS v1.1, Alternate Global Analogue Gain is also available.
> + * If the image sensor supports it, then the global analogue gain can be controlled
> + * by linear and exponential gain formula:
> + *
> + * \f$gain = analogLinearGainGlobal * 2^{analogExponentialGainGlobal}\f$
> + * \todo not implemented in libipa
> + */
> +
> +/**
> + * \struct CameraSensorHelper::AnalogueGainConstants
> + * \brief Analogue gain constants used for gain calculation
> + */
> +
> +/**
> + * \var CameraSensorHelper::AnalogueGainConstants::type
> + * \brief Analogue gain calculation mode
> + */
> +
> +/**
> + * \var CameraSensorHelper::AnalogueGainConstants::m0
> + * \brief Constant used in the analogue Gain coding/decoding
> + *
> + * \note either m0 or m1 shall be zero.
> + */
> +
> +/**
> + * \var CameraSensorHelper::AnalogueGainConstants::c0
> + * \brief Constant used in the analogue gain coding/decoding
> + */
> +
> +/**
> + * \var CameraSensorHelper::AnalogueGainConstants::m1
> + * \brief Constant used in the analogue gain coding/decoding
> + *
> + * \note either m0 or m1 shall be zero.
> + */
> +
> +/**
> + * \var CameraSensorHelper::AnalogueGainConstants::c1
> + * \brief Constant used in the analogue gain coding/decoding
> + */
> +
> +/**
> + * \var CameraSensorHelper::analogueGainConstants_
> + * \brief The analogue gain parameters used for calculation
> + *
> + * The analogue gain is calculated through a formula, and its parameters are
> + * sensor specific. Use this variable to store the values at init time.
> + */
> +
> +/**
> + * \class CameraSensorHelperFactory
> + * \brief Registration of CameraSensorHelperFactory classes and creation of instances
> + *
> + * To facilitate discovery and instantiation of CameraSensorHelper classes, the
> + * CameraSensorHelperFactory class maintains a registry of camera sensor helper
> + * classes. Each CameraSensorHelper subclass shall register itself using the
> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a corresponding
> + * instance of a CameraSensorHelperFactory subclass and register it with the
> + * static list of factories.
> + */
> +
> +/**
> + * \brief Construct a camera sensor helper factory
> + * \param[in] name Name of the camera sensor helper class
> + *
> + * Creating an instance of the factory registers it with the global list of
> + * factories, accessible through the factories() function.
> + *
> + * The factory \a name is used for debug purpose and shall be unique.
> + */
> +CameraSensorHelperFactory::CameraSensorHelperFactory(const std::string name)
> +	: name_(name)
> +{
> +	registerType(this);
> +}
> +
> +/**
> + * \brief Create an instance of the CameraSensorHelper corresponding to the factory

s/the factory/a named factory/

PipelineHandlerFactory::create() is not a static function, so it's
different.

> + * \param[in] name Name of the factory
> + *
> + * \return A unique pointer to a new instance of the CameraSensorHelper subclass
> + * corresponding to the factory

s/the factory/the named factory, or a null pointer if no such factory exists/

> + */
> +std::unique_ptr<CameraSensorHelper> CameraSensorHelperFactory::create(const std::string &name)
> +{
> +	std::vector<CameraSensorHelperFactory *> &factories =
> +		CameraSensorHelperFactory::factories();
> +
> +	for (CameraSensorHelperFactory *factory : factories) {
> +		if (name != factory->name_)
> +			continue;
> +
> +		CameraSensorHelper *helper = factory->createInstance();
> +		return std::unique_ptr<CameraSensorHelper>(helper);
> +	}
> +
> +	return nullptr;
> +}
> +
> +/**
> + * \brief Add a camera sensor helper class to the registry
> + * \param[in] factory Factory to use to construct the camera sensor helper
> + *
> + * The caller is responsible to guarantee the uniqueness of the camera sensor helper
> + * name.
> + */
> +void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory *factory)
> +{
> +	std::vector<CameraSensorHelperFactory *> &factories = CameraSensorHelperFactory::factories();

Line wrap.

> +
> +	factories.push_back(factory);
> +}
> +
> +/**
> + * \brief Retrieve the list of all camera sensor helper factories
> + *
> + * The static factories map is defined inside the function to ensures it gets
> + * initialized on first use, without any dependency on link order.

I know this comes from pipeline_handler.cpp, but this doesn't belong to
the API documentation, it's an implementation detail. You can move this
comment inside the function. Bonus points if you also fix the issue in
pipeline_handler.cpp.

> + *
> + * \return The list of camera sensor helper factories
> + */
> +std::vector<CameraSensorHelperFactory *> &CameraSensorHelperFactory::factories()
> +{
> +	static std::vector<CameraSensorHelperFactory *> factories;
> +	return factories;
> +}
> +
> +/**
> + * \fn CameraSensorHelperFactory::createInstance()
> + * \brief Create an instance of the CameraSensorHelper corresponding to the factory
> + *
> + * This virtual function is implemented by the REGISTER_CAMERA_SENSOR_HELPER()
> + * macro. It creates a camera sensor helper instance associated with the camera
> + * sensor model.
> + *
> + * \return A pointer to a newly constructed instance of the CameraSensorHelper
> + * subclass corresponding to the factory
> + */
> +
> +/**
> + * \var CameraSensorHelperFactory::name_
> + * \brief The name of the factory
> + */
> +
> +/**
> + * \def REGISTER_CAMERA_SENSOR_HELPER
> + * \brief Register a camera sensor helper with the camera sensor helper factory
> + * \param[in] name Sensor model name used to register the class
> + * \param[in] helper Class name of CameraSensorHelper derived class to register
> + *
> + * Register a CameraSensorHelper subclass with the factory and make it available to
> + * try and match devices.

s/devices/sensors/

> + */
> +
> +/**
> + * \class CameraSensorHelperImx219
> + * \brief Create and give helpers for the imx219 sensor
> + */

You can drop the comments for the subclasses, they don't bring any
value. If doxygen complains, you can enclose all the subclasses with a

#ifndef __DOXYGEN__
...
#endif /* __DOXYGEN__ */

You could also add a

/* -----------------------------------------------------------------------------
 * Sensor-speficic subclasses
 */

comment here if you want to separate it from the generic code above.

> +class CameraSensorHelperImx219 : public CameraSensorHelper
> +{
> +public:
> +	CameraSensorHelperImx219()
> +	{
> +		analogueGainConstants_ = { AnalogueGainLinear, 0, -1, 256, 256 };
> +	}
> +};
> +REGISTER_CAMERA_SENSOR_HELPER("imx219", CameraSensorHelperImx219)
> +
> +/**
> + * \class CameraSensorHelperOv5670
> + * \brief Create and give helpers for the ov5670 sensor
> + */
> +class CameraSensorHelperOv5670 : public CameraSensorHelper
> +{
> +public:
> +	CameraSensorHelperOv5670()
> +	{
> +		analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 };
> +	}
> +};
> +REGISTER_CAMERA_SENSOR_HELPER("ov5670", CameraSensorHelperOv5670)
> +
> +/**
> + * \class CameraSensorHelperOv5693
> + * \brief Create and give helpers for the ov5693 sensor
> + */
> +class CameraSensorHelperOv5693 : public CameraSensorHelper
> +{
> +public:
> +	CameraSensorHelperOv5693()
> +	{
> +		analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 16 };
> +	}
> +};
> +REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693)
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h
> new file mode 100644
> index 00000000..c43e4bf6
> --- /dev/null
> +++ b/src/ipa/libipa/camera_sensor_helper.h
> @@ -0,0 +1,87 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * camera_sensor_helper.h - Helper class that performs sensor-specific parameter computations
> + */
> +#ifndef __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__
> +#define __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__
> +
> +#include <stdint.h>
> +
> +#include <memory>
> +#include <string>
> +#include <vector>
> +
> +#include <libcamera/class.h>
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +class CameraSensorHelper
> +{
> +public:
> +	CameraSensorHelper() = default;
> +	virtual ~CameraSensorHelper() = default;
> +
> +	virtual uint32_t gainCode(double gain) const;
> +	virtual double gain(uint32_t gainCode) const;
> +
> +protected:
> +	enum AnalogueGainType {
> +		AnalogueGainLinear = 0,
> +		AnalogueGainExponential = 2,

Why 2 and not 1 ? I'd actually drop the numerical values completely.

> +	};
> +
> +	struct AnalogueGainConstants {
> +		AnalogueGainType type;
> +		int16_t m0;
> +		int16_t c0;
> +		int16_t m1;
> +		int16_t c1;
> +	};
> +
> +	AnalogueGainConstants analogueGainConstants_;
> +
> +private:
> +	LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)
> +};
> +
> +class CameraSensorHelperFactory
> +{
> +public:
> +	CameraSensorHelperFactory(const std::string name);
> +	virtual ~CameraSensorHelperFactory() = default;
> +
> +	static std::unique_ptr<CameraSensorHelper> create(const std::string &name);
> +
> +	static void registerType(CameraSensorHelperFactory *factory);
> +	static std::vector<CameraSensorHelperFactory *> &factories();
> +
> +protected:
> +	LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)

This should be private.

> +	virtual CameraSensorHelper *createInstance() = 0;
> +
> +	std::string name_;

I think this can be private too.

protected:
	virtual CameraSensorHelper *createInstance() = 0;

private:
	LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)

	std::string name_;

> +};
> +
> +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper)		  \
> +class helper##Factory final : public CameraSensorHelperFactory	  \
> +{                                                                 \

We use tabs for indentation.

> +public:                                                           \
> +	helper##Factory() : CameraSensorHelperFactory(name) {}	  \
> +								  \
> +private:                                                          \
> +	CameraSensorHelper *createInstance()			  \
> +	{							  \
> +		return new helper();				  \
> +	}							  \
> +};								  \
> +static helper##Factory global_##helper##Factory;
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index 038fc490..dc90542c 100644
> --- a/src/ipa/libipa/meson.build
> +++ b/src/ipa/libipa/meson.build
> @@ -2,11 +2,13 @@
>  
>  libipa_headers = files([
>      'algorithm.h',
> +    'camera_sensor_helper.h',
>      'histogram.h'
>  ])
>  
>  libipa_sources = files([
>      'algorithm.cpp',
> +    'camera_sensor_helper.cpp',
>      'histogram.cpp'
>  ])
>  

-- 
Regards,

Laurent Pinchart