[libcamera-devel] [PATCH 1/2] ipa: Create a camera sensor helper class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jun 8 20:02:00 CEST 2021
Hi Jacopo,
On Mon, Jun 07, 2021 at 06:05:33PM +0200, Jacopo Mondi wrote:
> 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.
That's partly true (and one could argue that the parameters are part of
the calculation, but that's more of a semantics discussion). There are
sensors that implement a gain model which can't be covered by the
formulas in this patch. We expect more work in this area when we'll have
to support such sensors.
> > 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
> > + */
No need to document each subclass.
> > +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)
> {
> ...
> }
> };
Agreed, an inline constructor seems better.
> 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...
Doesn't seem needed indeed.
> > +};
> > +
> > +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 :)
>
> > 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