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

12663 diff mbox series

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@ideasonboard.com>
---
 src/ipa/libipa/camera_sensor_helper.cpp | 313 ++++++++++++++++++++++++
 src/ipa/libipa/camera_sensor_helper.h   |  86 +++++++
 src/ipa/libipa/meson.build              |   2 +
 3 files changed, 401 insertions(+)
 create mode 100644 src/ipa/libipa/camera_sensor_helper.cpp
 create mode 100644 src/ipa/libipa/camera_sensor_helper.h

Hi Jean-Micheal,

On Mon, Jun 21, 2021 at 01:47:37PM +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@ideasonboard.com>
> ---
>  src/ipa/libipa/camera_sensor_helper.cpp | 313 ++++++++++++++++++++++++
>  src/ipa/libipa/camera_sensor_helper.h   |  86 +++++++
>  src/ipa/libipa/meson.build              |   2 +
>  3 files changed, 401 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..4bcea021
> --- /dev/null
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> @@ -0,0 +1,313 @@
> +/* 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
> + */
> +#include "camera_sensor_helper.h"

As suggested in v4:

#include <memory>

> +
> +#include "libcamera/internal/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
> + * 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.
> + */
> +
> +/**

As you have moved the constructor you should use the \fn anchor to
tell doxygen what function is this block documenting

weird, I see src/ipa/libipa in the list of Doxygen INPUT files but I
get no complaints for this. Also you're not documenting the public
destructor, we should get a warning o_0 Why doesn't that happen ?


> + * \brief Construct a CameraSensorHelper instance
> + *
> + * CameraSensorHelper derived class instances shall never be constructed manually
> + * but always through the CameraSensorHelperFactory::create() method.
> + */
> +
> +/**
> + * CameraSensorHelper::gainCode(double gain)
> + * \brief Compute gain code from the analogue gain absolute value
> + * \param[in] gain The real gain to pass
> + * \return the gain code to pass to V4l2
> + *
> + * 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).
> + */
> +uint32_t CameraSensorHelper::gainCode(double gain) const
> +{
> +	ASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));
> +	ASSERT(analogueGainConstants_.type == AnalogueGainLinear);
> +

Rogue tab as signaled by 'git am'

> +	return (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /
> +	       (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);
> +}
> +
> +/**
> + * CameraSensorHelper::gain
> + * \brief Compute the real gain from the V4l2 subdev control gain code
> + * \param[in] gainCode The V4l2 subdev control gain
> + * \return The real 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).
> + */
> +double CameraSensorHelper::gain(uint32_t gainCode) const
> +{
> +	ASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));
> +	ASSERT(analogueGainConstants_.type == AnalogueGainLinear);
> +
> +	return (analogueGainConstants_.m0 * gainCode + analogueGainConstants_.c0) /
> +	       (analogueGainConstants_.m1 * gainCode + analogueGainConstants_.c1);

You return a double but all the fields are int16_t. Won't this
truncate the result to a int, then convert it to a double on return ?
IOW if you have a the real division result x,y you would get x,0 ?

Is it necessary to cast gain_code to double ?

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

Doxygen supports \note

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

Rogue empty line

> + */
> +
> +/**
> + * \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
> + *
> + * \return A unique pointer to a new instance of the CameraSensorHelper subclass
> + * corresponding to the factory
> + */
> +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();
> +
> +	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.
> + *
> + * \return The list of camera sensor helper factories
> + */
> +std::vector<CameraSensorHelperFactory *> &CameraSensorHelperFactory::factories()
> +{
> +	static std::vector<CameraSensorHelperFactory *> factories;
> +	return factories;
> +}
> +
> +/**
> + * 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
> + */
> +
> +/**
> + * \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.
> + */
> +
> +/**
> + * \class CameraSensorHelperImx219
> + * \brief Create and give helpers for the imx219 sensor
> + */
> +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..c73900df
> --- /dev/null
> +++ b/src/ipa/libipa/camera_sensor_helper.h
> @@ -0,0 +1,86 @@
> +/* 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 <string>
> +#include <vector>

Ah you have a unique_ptr<> here too, so <memory> should be included
here

> +
> +#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,
> +	};
> +
> +	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();
> +
> +private:
> +	LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)
> +	virtual CameraSensorHelper *createInstance() = 0;

Can a pure virtual private method be overridden by derived classes ?
Apparently so, I was expecting it to be protected

> +
> +	std::string name_;
> +};
> +
> +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper)               \
> +class helper##Factory final : public CameraSensorHelperFactory    \
> +{                                                                 \
> +public:                                                           \
> +	helper##Factory() : CameraSensorHelperFactory(name) {} \
> +								  \
> +private:                                                          \
> +	CameraSensorHelper *createInstance()                      \
> +	{                                                         \
> +		return new helper();                          \
> +	}                                                         \
> +};                                                                \
> +static helper##Factory global_##helper##Factory;

Some of the line closing \ render unaligned. Shouldn't you align with
tabs ?

Overall, just minors again. With the above fixed and the doxygen thing
clarified
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j


> +
> +} /* 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'
>  ])
>
> --
> 2.30.2
>

Hi Jacopo,

On 21/06/2021 19:08, Jacopo Mondi wrote:
> Hi Jean-Micheal,
> 
> On Mon, Jun 21, 2021 at 01:47:37PM +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@ideasonboard.com>
>> ---
>>  src/ipa/libipa/camera_sensor_helper.cpp | 313 ++++++++++++++++++++++++
>>  src/ipa/libipa/camera_sensor_helper.h   |  86 +++++++
>>  src/ipa/libipa/meson.build              |   2 +
>>  3 files changed, 401 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..4bcea021
>> --- /dev/null
>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
>> @@ -0,0 +1,313 @@
>> +/* 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
>> + */
>> +#include "camera_sensor_helper.h"
> 
> As suggested in v4:
> 
> #include <memory>
> 
>> +
>> +#include "libcamera/internal/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
>> + * 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.
>> + */
>> +
>> +/**
> 
> As you have moved the constructor you should use the \fn anchor to
> tell doxygen what function is this block documenting
> 
> weird, I see src/ipa/libipa in the list of Doxygen INPUT files but I
> get no complaints for this. Also you're not documenting the public
> destructor, we should get a warning o_0 Why doesn't that happen ?
> 

I am not really sure of that... Anyone has an idea ?
In the meantime I added a:
	\fn CameraSensorHelper::CameraSensorHelper
And I get a warning:
"warning: Compound libcamera::ipa::CameraSensorHelper is not documented."

> 
>> + * \brief Construct a CameraSensorHelper instance
>> + *
>> + * CameraSensorHelper derived class instances shall never be constructed manually
>> + * but always through the CameraSensorHelperFactory::create() method.
>> + */
>> +
>> +/**
>> + * CameraSensorHelper::gainCode(double gain)
>> + * \brief Compute gain code from the analogue gain absolute value
>> + * \param[in] gain The real gain to pass
>> + * \return the gain code to pass to V4l2
>> + *
>> + * 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).
>> + */
>> +uint32_t CameraSensorHelper::gainCode(double gain) const
>> +{
>> +	ASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));
>> +	ASSERT(analogueGainConstants_.type == AnalogueGainLinear);
>> +
> 
> Rogue tab as signaled by 'git am'
> 

I don't have it here, maybe already solved in-between :-)

>> +	return (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /
>> +	       (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);
>> +}
>> +
>> +/**
>> + * CameraSensorHelper::gain
>> + * \brief Compute the real gain from the V4l2 subdev control gain code
>> + * \param[in] gainCode The V4l2 subdev control gain
>> + * \return The real 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).
>> + */
>> +double CameraSensorHelper::gain(uint32_t gainCode) const
>> +{
>> +	ASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));
>> +	ASSERT(analogueGainConstants_.type == AnalogueGainLinear);
>> +
>> +	return (analogueGainConstants_.m0 * gainCode + analogueGainConstants_.c0) /
>> +	       (analogueGainConstants_.m1 * gainCode + analogueGainConstants_.c1);
> 
> You return a double but all the fields are int16_t. Won't this
> truncate the result to a int, then convert it to a double on return ?
> IOW if you have a the real division result x,y you would get x,0 ?
> 
> Is it necessary to cast gain_code to double ?
> 

Yes, done. Weird, I did not have a warning for that...
We may need to use -Weverything for clang ? :-)

>> +}
>> +
>> +/**
>> + * \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.
>> + */
>> +
>> +/**
>> + * \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.
> 
> Doxygen supports \note
> 
>> + */
>> +
>> +/**
>> + * \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.
>> + *
> 
> Rogue empty line
> 
>> + */
>> +
>> +/**
>> + * \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
>> + *
>> + * \return A unique pointer to a new instance of the CameraSensorHelper subclass
>> + * corresponding to the factory
>> + */
>> +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();
>> +
>> +	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.
>> + *
>> + * \return The list of camera sensor helper factories
>> + */
>> +std::vector<CameraSensorHelperFactory *> &CameraSensorHelperFactory::factories()
>> +{
>> +	static std::vector<CameraSensorHelperFactory *> factories;
>> +	return factories;
>> +}
>> +
>> +/**
>> + * 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
>> + */
>> +
>> +/**
>> + * \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.
>> + */
>> +
>> +/**
>> + * \class CameraSensorHelperImx219
>> + * \brief Create and give helpers for the imx219 sensor
>> + */
>> +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..c73900df
>> --- /dev/null
>> +++ b/src/ipa/libipa/camera_sensor_helper.h
>> @@ -0,0 +1,86 @@
>> +/* 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 <string>
>> +#include <vector>
> 
> Ah you have a unique_ptr<> here too, so <memory> should be included
> here
> 
>> +
>> +#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,
>> +	};
>> +
>> +	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();
>> +
>> +private:
>> +	LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)
>> +	virtual CameraSensorHelper *createInstance() = 0;
> 
> Can a pure virtual private method be overridden by derived classes ?
> Apparently so, I was expecting it to be protected
> 
>> +
>> +	std::string name_;
>> +};
>> +
>> +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper)               \
>> +class helper##Factory final : public CameraSensorHelperFactory    \
>> +{                                                                 \
>> +public:                                                           \
>> +	helper##Factory() : CameraSensorHelperFactory(name) {} \
>> +								  \
>> +private:                                                          \
>> +	CameraSensorHelper *createInstance()                      \
>> +	{                                                         \
>> +		return new helper();                          \
>> +	}                                                         \
>> +};                                                                \
>> +static helper##Factory global_##helper##Factory;
> 
> Some of the line closing \ render unaligned. Shouldn't you align with
> tabs ?
> 
> Overall, just minors again. With the above fixed and the doxygen thing
> clarified
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 

Thx, I don't have Doxygen answer though :-(.

> Thanks
>   j
> 
> 
>> +
>> +} /* 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'
>>  ])
>>
>> --
>> 2.30.2
>>

Hi Jean-Michel,

On Tue, Jun 22, 2021 at 10:23:08AM +0200, Jean-Michel Hautbois wrote:
> Hi Jacopo,
>
> On 21/06/2021 19:08, Jacopo Mondi wrote:
> > Hi Jean-Micheal,
> >
> > On Mon, Jun 21, 2021 at 01:47:37PM +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@ideasonboard.com>
> >> ---
> >>  src/ipa/libipa/camera_sensor_helper.cpp | 313 ++++++++++++++++++++++++
> >>  src/ipa/libipa/camera_sensor_helper.h   |  86 +++++++
> >>  src/ipa/libipa/meson.build              |   2 +
> >>  3 files changed, 401 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..4bcea021
> >> --- /dev/null
> >> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> >> @@ -0,0 +1,313 @@
> >> +/* 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
> >> + */
> >> +#include "camera_sensor_helper.h"
> >
> > As suggested in v4:
> >
> > #include <memory>
> >
> >> +
> >> +#include "libcamera/internal/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
> >> + * 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.
> >> + */
> >> +
> >> +/**
> >
> > As you have moved the constructor you should use the \fn anchor to
> > tell doxygen what function is this block documenting
> >
> > weird, I see src/ipa/libipa in the list of Doxygen INPUT files but I
> > get no complaints for this. Also you're not documenting the public
> > destructor, we should get a warning o_0 Why doesn't that happen ?
> >
>
> I am not really sure of that... Anyone has an idea ?
> In the meantime I added a:
> 	\fn CameraSensorHelper::CameraSensorHelper
> And I get a warning:
> "warning: Compound libcamera::ipa::CameraSensorHelper is not documented."
>

Running doxygen 1.9.1 here, and I don't even get the warning :/

> >
> >> + * \brief Construct a CameraSensorHelper instance
> >> + *
> >> + * CameraSensorHelper derived class instances shall never be constructed manually
> >> + * but always through the CameraSensorHelperFactory::create() method.
> >> + */
> >> +
> >> +/**
> >> + * CameraSensorHelper::gainCode(double gain)
> >> + * \brief Compute gain code from the analogue gain absolute value
> >> + * \param[in] gain The real gain to pass
> >> + * \return the gain code to pass to V4l2

Since I'm here again, \return statement goes last, after the
free-formed text paragraph :)

> >> + *
> >> + * 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).
> >> + */
> >> +uint32_t CameraSensorHelper::gainCode(double gain) const
> >> +{
> >> +	ASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));
> >> +	ASSERT(analogueGainConstants_.type == AnalogueGainLinear);
> >> +
> >
> > Rogue tab as signaled by 'git am'
> >
>
> I don't have it here, maybe already solved in-between :-)
>

My editor eats up rougues tabs and spaces when I hit ':w', that's why
you don't see it here.. I had a look again and it's there in the patch
:)

> >> +	return (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /
> >> +	       (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);
> >> +}
> >> +
> >> +/**
> >> + * CameraSensorHelper::gain
> >> + * \brief Compute the real gain from the V4l2 subdev control gain code
> >> + * \param[in] gainCode The V4l2 subdev control gain
> >> + * \return The real 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).
> >> + */
> >> +double CameraSensorHelper::gain(uint32_t gainCode) const
> >> +{
> >> +	ASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));
> >> +	ASSERT(analogueGainConstants_.type == AnalogueGainLinear);
> >> +
> >> +	return (analogueGainConstants_.m0 * gainCode + analogueGainConstants_.c0) /
> >> +	       (analogueGainConstants_.m1 * gainCode + analogueGainConstants_.c1);
> >
> > You return a double but all the fields are int16_t. Won't this
> > truncate the result to a int, then convert it to a double on return ?
> > IOW if you have a the real division result x,y you would get x,0 ?
> >
> > Is it necessary to cast gain_code to double ?
> >
>
> Yes, done. Weird, I did not have a warning for that...
> We may need to use -Weverything for clang ? :-)
>

I don't get a warning if not from my eyes, possibily because my poor
understanding of C++

> >> +}
> >> +
> >> +/**
> >> + * \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.
> >> + */
> >> +
> >> +/**
> >> + * \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.
> >
> > Doxygen supports \note
> >
> >> + */
> >> +
> >> +/**
> >> + * \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.
> >> + *
> >
> > Rogue empty line
> >
> >> + */
> >> +
> >> +/**
> >> + * \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
> >> + *
> >> + * \return A unique pointer to a new instance of the CameraSensorHelper subclass
> >> + * corresponding to the factory
> >> + */
> >> +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();
> >> +
> >> +	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.
> >> + *
> >> + * \return The list of camera sensor helper factories
> >> + */
> >> +std::vector<CameraSensorHelperFactory *> &CameraSensorHelperFactory::factories()
> >> +{
> >> +	static std::vector<CameraSensorHelperFactory *> factories;
> >> +	return factories;
> >> +}
> >> +
> >> +/**
> >> + * 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
> >> + */
> >> +
> >> +/**
> >> + * \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.
> >> + */
> >> +
> >> +/**
> >> + * \class CameraSensorHelperImx219
> >> + * \brief Create and give helpers for the imx219 sensor
> >> + */
> >> +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..c73900df
> >> --- /dev/null
> >> +++ b/src/ipa/libipa/camera_sensor_helper.h
> >> @@ -0,0 +1,86 @@
> >> +/* 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 <string>
> >> +#include <vector>
> >
> > Ah you have a unique_ptr<> here too, so <memory> should be included
> > here
> >
> >> +
> >> +#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,
> >> +	};
> >> +
> >> +	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();
> >> +
> >> +private:
> >> +	LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)
> >> +	virtual CameraSensorHelper *createInstance() = 0;
> >
> > Can a pure virtual private method be overridden by derived classes ?
> > Apparently so, I was expecting it to be protected
> >
> >> +
> >> +	std::string name_;
> >> +};
> >> +
> >> +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper)               \
> >> +class helper##Factory final : public CameraSensorHelperFactory    \
> >> +{                                                                 \
> >> +public:                                                           \
> >> +	helper##Factory() : CameraSensorHelperFactory(name) {} \
> >> +								  \
> >> +private:                                                          \
> >> +	CameraSensorHelper *createInstance()                      \
> >> +	{                                                         \
> >> +		return new helper();                          \
> >> +	}                                                         \
> >> +};                                                                \
> >> +static helper##Factory global_##helper##Factory;
> >
> > Some of the line closing \ render unaligned. Shouldn't you align with
> > tabs ?
> >
> > Overall, just minors again. With the above fixed and the doxygen thing
> > clarified
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >
>
> Thx, I don't have Doxygen answer though :-(.
>
> > Thanks
> >   j
> >
> >
> >> +
> >> +} /* 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'
> >>  ])
> >>
> >> --
> >> 2.30.2
> >>