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

[Jean-Michel Hautbois]

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.

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