[libcamera-devel] [PATCH 1/2] ipa: Create a camera sensor helper class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jun 8 17:46:40 CEST 2021
Hi Jean-Michel,
Thank you for the patch.
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.
>
> 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.
>
> 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.
>
> 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
> + */
> +#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
> + *
> + * CameraSensorHelper instances are unique and sensor dependant.
> + */
> +
> +/**
> + * \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.
The respective factories implement createInstance(), not create(). I'd
drop "by the respective factories".
> + */
> +
> +CameraSensorHelper::CameraSensorHelper(const char *name)
> + : name_(name)
> +{
> + analogueGain_ = { GlobalGain, 1, 1, 0, 0 };
No need for this, it's set by all the subclasses.
> +}
> +
> +CameraSensorHelper::~CameraSensorHelper()
> +{
> +}
> +
> +/**
> + * \fn CameraSensorHelper::getGainCode(double gain)
> + * \brief Get the analogue gain code to pass to V4L2 subdev control
> + * \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).
> + */
> +uint32_t CameraSensorHelper::getGainCode(double gain) const
> +{
> + /* \todo we only support the global gain mode for now */
Then let's drop AlternateGlobalGain for now. Or we can address the todo
item already, it should be easy.
> + if (analogueGain_.type != GlobalGain)
> + return UINT32_MAX;
> +
> + if (analogueGain_.m0 == 0)
> + return (analogueGain_.c0 - gain * analogueGain_.c1) / (gain * analogueGain_.m1);
return (analogueGain_.c0 - gain * analogueGain_.c1) /
(gain * analogueGain_.m1);
> + if (analogueGain_.m1 == 0)
> + return (gain * analogueGain_.c1 - analogueGain_.c0) / analogueGain_.m0;
You can also write
return (analogueGain_.c0 - analogueGain_.c1 * gain) /
(analogueGain_.m1 * gain - analogueGain_.m0);
> +
> + LOG(CameraSensorHelper, Error) << "For any given image sensor either m0 or m1 shall be zero.";
Make this an assert. I'm tempted to pass the analogueGainConstants
pointer to the CameraSensorHelper constructor to assert there.
> + return 1;
> +}
> +
> +/**
> + * \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).
> + */
> +double CameraSensorHelper::getGain(uint32_t gainCode) const
> +{
> + if (analogueGain_.type != GlobalGain)
> + return UINT32_MAX;
That's not a double.
> +
> + if (analogueGain_.m0 == 0)
> + return analogueGain_.c0 / (analogueGain_.m1 * gainCode + analogueGain_.c1);
> + if (analogueGain_.m1 == 0)
> + return (analogueGain_.m0 * gainCode + analogueGain_.c0) / analogueGain_.c1;
You can also write
return (analogueGain_.m0 * gainCode + analogueGain_.c0) /
(analogueGain_.m1 * gainCode + analogueGain_.c1)
> +
> + LOG(CameraSensorHelper, Error) << "For any given image sensor either m0 or m1 shall be zero.";
> + return 1.0;
> +}
> +
> +/**
> + * \fn CameraSensorHelper::name()
> + * \brief Retrieve the camera sensor helper name
> + * \return The camera sensor helper name
> + */
> +
> +/**
> + * \enum CameraSensorHelper::AnalogueGainCapability
That's not a very descriptive name, I'd propose AnalogueGainType (or
AnalogueGainModel).
> + * \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
This is also not very descriptive, I'd propose AnalogueGainLinear (and
AnalogueGainExponential for AlternateGlobalGain).
> + *
> + * 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$
* \f$fain = \frac...
> + */
> +
> +/**
> + * \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
> + */
> +
> +/**
> + * \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.
> + *
> + * Note: either m0 or m1 shall be zero.
> + */
> +
> +/**
> + * \var CameraSensorHelper::analogueGainConstants::c1
> + * \brief Constant used in the analog Gain control coding/decoding.
> + */
> +
> +/**
> + * \var CameraSensorHelper::analogueGain_
> + * \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 char *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()
> +{
> + CameraSensorHelper *handler = createInstance();
> + return std::unique_ptr<CameraSensorHelper>(handler);
There's no handler. s/handler/helper/ (or any other better name you can
think of). Same below.
> +}
> +
> +/**
> + * \fn CameraSensorHelperFactory::name()
> + * \brief Retrieve the factory name
> + * \return The factory name
> + */
> +
> +/**
> + * \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;
> +}
> +
> +/**
> + * \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
> + */
> +
> +/**
> + * \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] handler 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
> + */
> +
> +/**
> + * \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)
> +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>
This doesn't seem to be used, but it should, the CameraSensorHelper and
CameraSensorHelperFactory classes should not be copyable or moveable.
> +#include <libcamera/object.h>
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +class CameraSensorHelper;
> +
> +class CameraSensorHelper : public Object
Any reason this class inherits from Object ?
> +{
> +public:
> + CameraSensorHelper(const char *name);
> + virtual ~CameraSensorHelper();
> +
> + const std::string &name() const { return name_; }
This doesn't seem to be used.
> + uint32_t getGainCode(double gain) const;
> + double getGain(uint32_t gainCode) const;
We don't usually use a "get" prefix, should these be called gainCode()
and gain() ?
> +
> +protected:
> + enum AnalogueGainCapability : uint16_t {
Is there a need to specify the underlying data type explicitly ?
> + GlobalGain = 0,
> + AlternateGlobalGain = 2,
> + };
> +
> + struct analogueGainConstants {
s/analogueGainConstants/AnalogueGainConstants/
> + uint16_t type;
> + int16_t m0;
> + int16_t c0;
> + int16_t m1;
> + int16_t c1;
> + };
> + analogueGainConstants analogueGain_;
The variable name can be mistaken to mean it stores a gain, while it
stores the constants.
It would be nice to make this const.
> +
> +private:
> + std::string name_;
> + friend class CameraSensorHelperFactory;
> +};
> +
> +class CameraSensorHelperFactory
> +{
> +public:
> + CameraSensorHelperFactory(const char *name);
> + virtual ~CameraSensorHelperFactory() = default;
> +
> + std::unique_ptr<CameraSensorHelper> create();
> +
> + const std::string &name() const { return name_; }
This doesn't seem to be used.
> +
> + 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;
You can drop one indentation level (I know checkstyle.py complains).
> +
> +} /* 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
More information about the libcamera-devel
mailing list