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

[Jacopo Mondi]

Hello Jean-Michel,


On Mon, Jun 07, 2021 at 02:35:32PM +0200, Jean-Michel Hautbois wrote:
> Setting analogue gain for a specific sensor is not a straghtforward
> operation, as one needs to know how the gain is calculated for it.

Please bear with my poor understanding of the issue, but looking at
the code it doesn't seem the different is in how the gain is
calculated but rather in which constant parameters are used.

>
> 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.
>

I should first go through this document, so I'll probably have only
drive-by comments and nits here and there

> Is makes it possible to only have 4 parameters to store per sensor, and
> the gain calculation is then done identically for all of them. This
> commit gives the gains for imx219 (based on RPi cam_helper), ov5670 and
> ov5693.
>
> Adding a new sensor is pretty straightfoward as one only needs to
> implement the sub-class for it and register that class to the
> CameraSensorHelperFactory.

As asked offline, the current fatory implementation is the same as the
one we have for pipeline handlers, which requires static
initialization to make sure they are registered at library
initialization time. I suspect without this requirement the factory
here could be made simpler, but that's minor...

>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/libipa/camera_sensor_helper.cpp | 361 ++++++++++++++++++++++++
>  src/ipa/libipa/camera_sensor_helper.h   |  91 ++++++
>  src/ipa/libipa/meson.build              |   2 +
>  3 files changed, 454 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..2bd82861
> --- /dev/null
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> @@ -0,0 +1,361 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2021 Ideas On Board
> + *
> + * camera_sensor_helper.cpp - helper class providing camera informations

Nit: 'Helper'
as I've received the same comment multiple times, 'information' is
uncountable, so no plural

> + */
> +#include "camera_sensor_helper.h"
> +
> +#include <map>
> +
> +#include "libcamera/internal/log.h"
> +
> +/**
> + * \file camera_sensor_helper.h
> + * \brief Create helper class for each sensor
> + *
> + * Each camera sensor supported by libcamera may need specific informations to
> + * be sent (analogue gain is sensor dependant for instance).
> + *
> + * 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 Create and give helpers for each camera sensor in the pipeline

I would here and maybe above in the file description drop the 'create
and give' part. The CameraSensorHelper base class computes sensor
tuning parameters using sensor-specific constants. The factory instead
does the actual object creation

> + *
 sensor-specific constants. The factory instead does the actual object
 creation

> + * CameraSensorHelper instances are unique and sensor dependant.

Is the base class meant to be used as a default if not sensor-specific
sub-class is registered ?

> + */
> +
> +/**
> + * \brief Construct a CameraSensorHelper instance
> + * \param[in] name The sensor model name
> + *
> + * The CameraSensorHelper instances shall never be constructed manually, but always
> + * through the CameraSensorHelperFactory::create() method implemented by the
> + * respective factories.
> + */
> +

Empty line

> +CameraSensorHelper::CameraSensorHelper(const char *name)
> +	: name_(name)
> +{
> +	analogueGain_ = { GlobalGain, 1, 1, 0, 0 };
> +}

I wonder, we don't support AlternateGlobalGain at the moment, do you
expect it to be required soon ?

> +
> +CameraSensorHelper::~CameraSensorHelper()
> +{
> +}
> +
> +/**
> + * \fn CameraSensorHelper::getGainCode(double gain)

If you document the function just defined below, you don't need the
\fn anchor

Library-wide we don't use 'get' for getters, so this might just be
gainCode() ?

> + * \brief Get the analogue gain code to pass to V4L2 subdev control

Aren't we computing the gain code from the analogue gain 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).

Do you mean the m0, c0, etc etc paramteres ? Maybe this is better
specified in the enum documentation ?

> + */
> +uint32_t CameraSensorHelper::getGainCode(double gain) const
> +{
> +	/* \todo we only support the global gain mode for now */
> +	if (analogueGain_.type != GlobalGain)
> +		return UINT32_MAX;
> +
> +	if (analogueGain_.m0 == 0)
> +		return (analogueGain_.c0 - gain * analogueGain_.c1) / (gain * analogueGain_.m1);
> +	if (analogueGain_.m1 == 0)
> +	return (gain * analogueGain_.c1 - analogueGain_.c0) / analogueGain_.m0;

You can break the two lines

> +
> +	LOG(CameraSensorHelper, Error) << "For any given image sensor either m0 or m1 shall be zero.";
> +	return 1;

Aren't the two error conditions verified by assertions ? Otherwise we
return two different error codes (MAXINT and 1) which are difficult to
identify.

> +}
> +
> +/**
> + * \fn CameraSensorHelper::getGain
> + * \brief Get the real gain from the V4l2 subdev control gain
> + * \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::getGainCode.
> + *
> + * The parameters come from the MIPI Alliance Camera Specification for
> + * Camera Command Set (CCS).

Same comments as above

> + */
> +double CameraSensorHelper::getGain(uint32_t gainCode) const
> +{
> +	if (analogueGain_.type != GlobalGain)
> +		return UINT32_MAX;
> +
> +	if (analogueGain_.m0 == 0)
> +		return analogueGain_.c0 / (analogueGain_.m1 * gainCode + analogueGain_.c1);
> +	if (analogueGain_.m1 == 0)
> +		return (analogueGain_.m0 * gainCode + analogueGain_.c0) / analogueGain_.c1;
> +
> +	LOG(CameraSensorHelper, Error) << "For any given image sensor either m0 or m1 shall be zero.";
> +	return 1.0;
> +}

ditto

Also, these smells like static functions as they only use class
members constants for computations and do not hold any state. This
however does not play well with the class hierarchy...

> +
> +/**
> + * \fn CameraSensorHelper::name()
> + * \brief Retrieve the camera sensor helper name
> + * \return The camera sensor helper name
> + */
> +
> +/**
> + * \enum CameraSensorHelper::AnalogueGainCapability
> + * \brief Specify the Gain mode supported by the sensor
> + *
> + * Describes the image sensor analog Gain capabilities.
> + * Two modes are possible, depending on the sensor: Global and Alternate.
> + */
> +
> +/**
> + * \var CameraSensorHelper::GlobalGain
> + * \brief Sensor supports Global gain
> + *
> + * The relationship between the integer Gain parameter and the resulting Gain
> + * multiplier is given by the following equation:
> + *
> + * \f$\frac{m0x+c0}{m1x+c1}\f$
> + *
> + * Where 'x' is the gain control parameter, and m0, m1, c0 and c1 are
> + * image-sensor-specific constants exposed by 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$\frac{c0}{m1x+c1}\f$ or \f$\frac{m0x+c0}{c1}\f$
> + */
> +
> +/**
> + * \var CameraSensorHelper::AlternateGlobalGain
> + * \brief Sensor supports Analogue Global gain (introduced in CCS v1.1)
> + *
> + * Starting with CCS v1.1, Alternate Global Analog Gain is also available.
> + * If the image sensor supports it, then the global analog 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
> + */

How do you envision this class to evolve ? I mean, it will go beyoind
just analogue gain calculations ? As otherwise a single class with a
map of sensor specific gain constants would do...

> +
> +/**
> + * \var CameraSensorHelper::analogueGainConstants::type
> + * \brief Analogue gain coding type
> + */
> +
> +/**
> + * \var CameraSensorHelper::analogueGainConstants::m0
> + * \brief Constant used in the analog Gain control coding/decoding.
> + *
> + * Note: either m0 or m1 shall be zero.
> + */
> +
> +/**
> + * \var CameraSensorHelper::analogueGainConstants::c0
> + * \brief Constant used in the analog Gain control coding/decoding.
> + */
> +
> +/**
> + * \var CameraSensorHelper::analogueGainConstants::m1
> + * \brief Constant used in the analog Gain control coding/decoding.

Nit: briefs do not end with '.'

> + *
> + * Note: either m0 or m1 shall be zero.
> + */
> +
> +/**
> + * \var CameraSensorHelper::analogueGainConstants::c1
> + * \brief Constant used in the analog Gain control coding/decoding.

I presume I have to read the CCS specs to know what this parameters
actually are :)

> + */
> +
> +/**
> + * \var CameraSensorHelper::analogueGain_

Maybe something in the name that makes it clear these are params ?
Analouge gain to me is what 'getGain()' calculates, am I mistaken ?

> + * \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.
> + *

Empty line

> + */
> +

I'll skip the factory review for now

> +/**

[snip]

> +
> +/**
> + * \fn CameraSensorHelperImx219::CameraSensorHelperImx219
> + * \brief Construct a CameraSensorHelperImx219 instance for imx219
> + * \param[in] name The sensor model name
> + */
> +class CameraSensorHelperImx219 : public CameraSensorHelper
> +{
> +public:
> +	CameraSensorHelperImx219(const char *name);
> +};
> +REGISTER_CAMERA_SENSOR_HELPER("imx219", CameraSensorHelperImx219)

I would add an empty line here and make it look like

class CameraSensorHelperXYZ : public CameraSensorHelper
{
public:
	CameraSensorHelperXYZ(const char *name)
        {
                ...
        }
};
REGISTER_CAMERA_SENSOR_HELPER("XYZ", CameraSensorHelperXYZ)


> +CameraSensorHelperImx219::CameraSensorHelperImx219(const char *name)
> +	: CameraSensorHelper(name)
> +{
> +	analogueGain_ = { GlobalGain, 0, -1, 256, 256 };
> +}
> +
> +/**
> + * \class CameraSensorHelperOv5670
> + * \brief Create and give helpers for the ov5670 sensor
> + */
> +
> +/**
> + * \fn CameraSensorHelperOv5670::CameraSensorHelperOv5670
> + * \brief Construct a CameraSensorHelperOv5670 instance for ov5670
> + * \param[in] name The sensor model name
> + */
> +class CameraSensorHelperOv5670 : public CameraSensorHelper
> +{
> +public:
> +	CameraSensorHelperOv5670(const char *name);
> +};
> +REGISTER_CAMERA_SENSOR_HELPER("ov5670", CameraSensorHelperOv5670)
> +CameraSensorHelperOv5670::CameraSensorHelperOv5670(const char *name)
> +	: CameraSensorHelper(name)
> +{
> +	analogueGain_ = { GlobalGain, 1, 0, 0, 256 };
> +}
> +
> +/**
> + * \class CameraSensorHelperOv5693
> + * \brief Create and give helpers for the ov5693 sensor
> + */
> +
> +/**
> + * \fn CameraSensorHelperOv5693::CameraSensorHelperOv5693
> + * \brief Construct a CameraSensorHelperOv5693 instance for ov5693
> + * \param[in] name The sensor model name
> + */
> +class CameraSensorHelperOv5693 : public CameraSensorHelper
> +{
> +public:
> +	CameraSensorHelperOv5693(const char *name);
> +};
> +REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693)
> +CameraSensorHelperOv5693::CameraSensorHelperOv5693(const char *name)
> +	: CameraSensorHelper(name)
> +{
> +	analogueGain_ = { GlobalGain, 1, 0, 0, 16 };
> +}
> +
> +} /* 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..63031902
> --- /dev/null
> +++ b/src/ipa/libipa/camera_sensor_helper.h
> @@ -0,0 +1,91 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2021 Ideas On Board
> + *
> + * camera_sensor_helper.h - helper class providing camera informations
> + */
> +#ifndef __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__
> +#define __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__
> +
> +#include <stdint.h>
> +
> +#include <string>
> +
> +#include <libcamera/class.h>
> +#include <libcamera/object.h>
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +class CameraSensorHelper;
> +
> +class CameraSensorHelper : public Object
> +{
> +public:
> +	CameraSensorHelper(const char *name);
> +	virtual ~CameraSensorHelper();
> +
> +	const std::string &name() const { return name_; }
> +	uint32_t getGainCode(double gain) const;
> +	double getGain(uint32_t gainCode) const;
> +
> +protected:
> +	enum AnalogueGainCapability : uint16_t {
> +		GlobalGain = 0,
> +		AlternateGlobalGain = 2,

Are the gain type values defined by the CCS specs ?

> +	};
> +
> +	struct analogueGainConstants {
> +		uint16_t type;
> +		int16_t m0;
> +		int16_t c0;
> +		int16_t m1;
> +		int16_t c1;
> +	};
> +	analogueGainConstants analogueGain_;
> +
> +private:
> +	std::string name_;
> +	friend class CameraSensorHelperFactory;

Do you need this friend class declaration ?
The constructor is public...

> +};
> +
> +class CameraSensorHelperFactory
> +{
> +public:
> +	CameraSensorHelperFactory(const char *name);
> +	virtual ~CameraSensorHelperFactory() = default;
> +
> +	std::unique_ptr<CameraSensorHelper> create();
> +
> +	const std::string &name() const { return name_; }
> +
> +	static void registerType(CameraSensorHelperFactory *factory);
> +	static std::vector<CameraSensorHelperFactory *> &factories();
> +
> +private:
> +	virtual CameraSensorHelper *createInstance() = 0;
> +
> +	std::string name_;
> +};
> +
> +#define REGISTER_CAMERA_SENSOR_HELPER(name, handler)                        \
> +	class handler##Factory final : public CameraSensorHelperFactory     \
> +	{                                                                   \
> +	public:                                                             \
> +		handler##Factory() : CameraSensorHelperFactory(#handler) {} \
> +                                                                            \
> +	private:                                                            \
> +		CameraSensorHelper *createInstance()                        \
> +		{                                                           \
> +			return new handler(name);                           \
> +		}                                                           \
> +	};                                                                  \
> +	static handler##Factory global_##handler##Factory;
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */
> +

No empty line at EOF otherwise git am complains (at least, that's how
I interpret the error message :)

Thanks
   j

> 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'
>  ])
>
> --
> 2.30.2
>