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

[Umang Jain]

Hi JM,

On 6/28/21 1:45 PM, Jean-Michel Hautbois wrote:
> Hi Umang,
>
> On 24/06/2021 13:45, Umang Jain wrote:
>> HI JM,
>>
>> few minors below:
>>
>> On 6/22/21 3:15 PM, 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>
>>> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>>> ---
>>>    src/ipa/libipa/camera_sensor_helper.cpp | 318 ++++++++++++++++++++++++
>>>    src/ipa/libipa/camera_sensor_helper.h   |  87 +++++++
>>>    src/ipa/libipa/meson.build              |   2 +
>>>    3 files changed, 407 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..a114ee73
>>> --- /dev/null
>>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
>>> @@ -0,0 +1,318 @@
>>> +/* 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"
>>> +
>>> +#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.
>> +1
>>
>> this is nice and how each CameraSensorHelper is registered down below.
>> The usage of this macro is apt.
>>
>>> + */
>>> +
>>> +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
>> minor: I would write this as for clarity:
>>
>>    * Instances derived from CameraSensorHelper class are sensor specific.
>> Each
>>
>>> + * supported sensor will have an associated base class defined.
>>> + */
>>> +
>>> +/**
>>> + * \brief Construct a CameraSensorHelper instance
>>> + *
>>> + * CameraSensorHelper derived class instances shall never be
>>> constructed manually
>>> + * but always through the CameraSensorHelperFactory::create() method.
>>> + */
>>> +
>>> +/**
>>> + * \brief Compute gain code from the analogue gain absolute 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).
>>> + *
>>> + * \return The gain code to pass to V4l2
>>> + */
>>> +uint32_t CameraSensorHelper::gainCode(double gain) const
>>> +{
>>> +    ASSERT((analogueGainConstants_.m0 == 0) ||
>>> (analogueGainConstants_.m1 == 0));
>>> +    ASSERT(analogueGainConstants_.type == AnalogueGainLinear);
>>> +
>>> +    return (analogueGainConstants_.c0 - analogueGainConstants_.c1 *
>>> gain) /
>>> +           (analogueGainConstants_.m1 * gain -
>>> analogueGainConstants_.m0);
>> This won't yield a floating value, ever? In case, it does, are we OK
>> returning it as uint32_t with loss of precision. I am not sure, I
>> haven't read the spec or understand this (maybe it's right, since it's
>> gain-"code")
>>
>>
> I don't think so, but I can have intermediate values if it scares you :-).
> The gain is a integer on kernel side, the loss of precision results in a
> minor difference AFAIK.


hmm, in that case, I think you can capture this context as a small-ish 
comment here, describing that, one can except intermediate values on 
division but gain is an integer on kernel side...

>
>>> +}
>>> +
>>> +/**
>>> + * \brief Compute the real gain from the V4l2 subdev control gain code
>>> + * \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::gainCode.
>>> + *
>>> + * The parameters come from the MIPI Alliance Camera Specification for
>>> + * Camera Command Set (CCS).
>>> + *
>>> + * \return The real gain
>>> + */
>>> +double CameraSensorHelper::gain(uint32_t gainCode) const
>>> +{
>>> +    ASSERT((analogueGainConstants_.m0 == 0) ||
>>> (analogueGainConstants_.m1 == 0));
>>> +    ASSERT(analogueGainConstants_.type == AnalogueGainLinear);
>>> +
>>> +    return (analogueGainConstants_.m0 * static_cast<double>(gainCode)
>>> + analogueGainConstants_.c0) /
>>> +           (analogueGainConstants_.m1 * static_cast<double>(gainCode)
>>> + analogueGainConstants_.c1);
>>> +}
>>> +
>>> +/**
>>> + * \enum CameraSensorHelper::AnalogueGainType
>>> + * \brief the gain calculation modes as defined by the MIPI CCS
>> s/the/The
>>> + *
>>> + * 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.
>> s/either/Either
>>> + */
>>> +
>>> +/**
>>> + * \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.
>> Same
>>> + */
>>> +
>>> +/**
>>> + * \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.
>>> + */
>>> +
>>> +/**
>>> + * \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
>> s/classes./sub-classes. maybe?
>>> + * 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
>>> + * \param[in] name Name of 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;
>>> +}
>>> +
>>> +/**
>>> + * \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
>>> + */
>>> +
>>> +/**
>>> + * \var CameraSensorHelperFactory::name_
>>> + * \brief The name of 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.
>> I do wonder about the reasons that registering a camera sensor helper
>> needs to create a derived factory class first
>> and  then, it is responsible to create Instance(s) of that camera sensor
>> helper per se?
>>
>> Can CameraSensorHelper base class be flexible enough to create those
>> instances by itself and keeping the registry internal?
>> I am not sure, there might be some road-blocks I am not able to see
>> right now.
>>
>> .... but this isn't that complicated either :-)
>>
>> So,
>>
>> Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> Thanks.
>
> The way it is designed is based on a Factory design pattern, and mostly
> comes from PipelineHandler implementation.
> I like it that way and can't see an obvious performance gain to have it
> keeping the registry. It is called only at init time, so we are not in a
> rush :-).
>
>>> + */
>>> +
>>> +/**
>>> + * \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..c43e4bf6
>>> --- /dev/null
>>> +++ b/src/ipa/libipa/camera_sensor_helper.h
>>> @@ -0,0 +1,87 @@
>>> +/* 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 <memory>
>>> +#include <string>
>>> +#include <vector>
>>> +
>>> +#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();
>>> +
>>> +protected:
>>> +    LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)
>>> +    virtual CameraSensorHelper *createInstance() = 0;
>>> +
>>> +    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;
>>> +
>>> +} /* 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'
>>>    ])
>>>