[libcamera-devel] [PATCH v5 1/2] ipa: Create a camera sensor helper class
Jacopo Mondi
jacopo at jmondi.org
Tue Jun 22 11:08:13 CEST 2021
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 at 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 at 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
> >>
More information about the libcamera-devel
mailing list