[{"id":17667,"web_url":"https://patchwork.libcamera.org/comment/17667/","msgid":"<20210621170835.65rp5quso23gj4dp@uno.localdomain>","date":"2021-06-21T17:08:35","subject":"Re: [libcamera-devel] [PATCH v5 1/2] ipa: Create a camera sensor\n\thelper class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Jean-Micheal,\n\nOn Mon, Jun 21, 2021 at 01:47:37PM +0200, Jean-Michel Hautbois wrote:\n> For various sensor operations, it may be needed to do sensor specific\n> computations, like analogue gain or vertical blanking.\n>\n> This commit introduces a new camera sensor helper in libipa which aims\n> to solve this specific issue.\n> It is based on the MIPI alliance Specification for Camera Command Set\n> and implements, for now, only the analogue \"Global gain\" mode.\n> Setting analogue gain for a specific sensor is not a straightforward\n> operation, as one needs to know how the gain is calculated for it.\n>\n> Three helpers are created in this patch: imx219, ov5670 and ov5693.\n>\n> Adding a new sensor is pretty straightforward as one only needs to\n> implement the sub-class for it and register that class to the\n> CameraSensorHelperFactory.\n>\n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/libipa/camera_sensor_helper.cpp | 313 ++++++++++++++++++++++++\n> src/ipa/libipa/camera_sensor_helper.h | 86 +++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 401 insertions(+)\n> create mode 100644 src/ipa/libipa/camera_sensor_helper.cpp\n> create mode 100644 src/ipa/libipa/camera_sensor_helper.h\n>\n> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> new file mode 100644\n> index 00000000..4bcea021\n> --- /dev/null\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -0,0 +1,313 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * camera_sensor_helper.cpp - Helper class that performs sensor-specific parameter computations\n> + */\n> +#include \"camera_sensor_helper.h\"\n\nAs suggested in v4:\n\n#include \n\n> +\n> +#include \"libcamera/internal/log.h\"\n> +\n> +/**\n> + * \\file camera_sensor_helper.h\n> + * \\brief Helper class that performs sensor-specific parameter computations\n> + *\n> + * Computation of sensor configuration parameters is a sensor specific\n> + * operation. Each CameraHelper derived class computes the value of\n> + * configuration parameters, for example the analogue gain value, using\n> + * sensor-specific functions and constants.\n> + *\n> + * Every subclass of CameraSensorHelper shall be registered with libipa using\n> + * the REGISTER_CAMERA_SENSOR_HELPER() macro.\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(CameraSensorHelper)\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\class CameraSensorHelper\n> + * \\brief Base class for computing sensor tuning parameters using sensor-specific constants\n> + *\n> + * CameraSensorHelper derived class instances are sensor specific. Each\n> + * supported sensor will have an associated base class defined.\n> + */\n> +\n> +/**\n\nAs you have moved the constructor you should use the \\fn anchor to\ntell doxygen what function is this block documenting\n\nweird, I see src/ipa/libipa in the list of Doxygen INPUT files but I\nget no complaints for this. Also you're not documenting the public\ndestructor, we should get a warning o_0 Why doesn't that happen ?\n\n\n> + * \\brief Construct a CameraSensorHelper instance\n> + *\n> + * CameraSensorHelper derived class instances shall never be constructed manually\n> + * but always through the CameraSensorHelperFactory::create() method.\n> + */\n> +\n> +/**\n> + * CameraSensorHelper::gainCode(double gain)\n> + * \\brief Compute gain code from the analogue gain absolute value\n> + * \\param[in] gain The real gain to pass\n> + * \\return the gain code to pass to V4l2\n> + *\n> + * This function aims to abstract the calculation of the gain letting the IPA\n> + * use the real gain for its estimations.\n> + *\n> + * The parameters come from the MIPI Alliance Camera Specification for\n> + * Camera Command Set (CCS).\n> + */\n> +uint32_t CameraSensorHelper::gainCode(double gain) const\n> +{\n> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> +\n\nRogue tab as signaled by 'git am'\n\n> +\treturn (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n> +\t (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n> +}\n> +\n> +/**\n> + * CameraSensorHelper::gain\n> + * \\brief Compute the real gain from the V4l2 subdev control gain code\n> + * \\param[in] gainCode The V4l2 subdev control gain\n> + * \\return The real gain\n> + *\n> + * This function aims to abstract the calculation of the gain letting the IPA\n> + * use the real gain for its estimations. It is the counterpart of the function\n> + * CameraSensorHelper::gainCode.\n> + *\n> + * The parameters come from the MIPI Alliance Camera Specification for\n> + * Camera Command Set (CCS).\n> + */\n> +double CameraSensorHelper::gain(uint32_t gainCode) const\n> +{\n> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> +\n> +\treturn (analogueGainConstants_.m0 * gainCode + analogueGainConstants_.c0) /\n> +\t (analogueGainConstants_.m1 * gainCode + analogueGainConstants_.c1);\n\nYou return a double but all the fields are int16_t. Won't this\ntruncate the result to a int, then convert it to a double on return ?\nIOW if you have a the real division result x,y you would get x,0 ?\n\nIs it necessary to cast gain_code to double ?\n\n> +}\n> +\n> +/**\n> + * \\enum CameraSensorHelper::AnalogueGainType\n> + * \\brief the gain calculation modes as defined by the MIPI CCS\n> + *\n> + * Describes the image sensor analogue gain capabilities.\n> + * Two modes are possible, depending on the sensor: Global and Alternate.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainLinear\n> + * \\brief Gain is computed using linear gain estimation\n> + *\n> + * The relationship between the integer gain parameter and the resulting gain\n> + * multiplier is given by the following equation:\n> + *\n> + * \\f$gain=\\frac{m0x+c0}{m1x+c1}\\f$\n> + *\n> + * Where 'x' is the gain control parameter, and m0, m1, c0 and c1 are\n> + * image-sensor-specific constants of the sensor.\n> + * These constants are static parameters, and for any given image sensor either\n> + * m0 or m1 shall be zero.\n> + *\n> + * The full Gain equation therefore reduces to either:\n> + *\n> + * \\f$gain=\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainExponential\n> + * \\brief Gain is computed using exponential gain estimation (introduced in CCS v1.1)\n> + *\n> + * Starting with CCS v1.1, Alternate Global Analogue Gain is also available.\n> + * If the image sensor supports it, then the global analogue gain can be controlled\n> + * by linear and exponential gain formula:\n> + *\n> + * \\f$gain = analogLinearGainGlobal * 2^{analogExponentialGainGlobal}\\f$\n> + * \\todo not implemented in libipa\n> + */\n> +\n> +/**\n> + * \\struct CameraSensorHelper::AnalogueGainConstants\n> + * \\brief Analogue gain constants used for gain calculation\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::type\n> + * \\brief Analogue gain calculation mode\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::m0\n> + * \\brief Constant used in the analogue Gain coding/decoding\n> + *\n> + * Note: either m0 or m1 shall be zero.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::c0\n> + * \\brief Constant used in the analogue gain coding/decoding\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::m1\n> + * \\brief Constant used in the analogue gain coding/decoding\n> + *\n> + * Note: either m0 or m1 shall be zero.\n\nDoxygen supports \\note\n\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainConstants::c1\n> + * \\brief Constant used in the analogue gain coding/decoding\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGainConstants_\n> + * \\brief The analogue gain parameters used for calculation\n> + *\n> + * The analogue gain is calculated through a formula, and its parameters are\n> + * sensor specific. Use this variable to store the values at init time.\n> + *\n\nRogue empty line\n\n> + */\n> +\n> +/**\n> + * \\class CameraSensorHelperFactory\n> + * \\brief Registration of CameraSensorHelperFactory classes and creation of instances\n> + *\n> + * To facilitate discovery and instantiation of CameraSensorHelper classes, the\n> + * CameraSensorHelperFactory class maintains a registry of camera sensor helper\n> + * classes. Each CameraSensorHelper subclass shall register itself using the\n> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a corresponding\n> + * instance of a CameraSensorHelperFactory subclass and register it with the\n> + * static list of factories.\n> + */\n> +\n> +/**\n> + * \\brief Construct a camera sensor helper factory\n> + * \\param[in] name Name of the camera sensor helper class\n> + *\n> + * Creating an instance of the factory registers it with the global list of\n> + * factories, accessible through the factories() function.\n> + *\n> + * The factory \\a name is used for debug purpose and shall be unique.\n> + */\n> +CameraSensorHelperFactory::CameraSensorHelperFactory(const std::string name)\n> +\t: name_(name)\n> +{\n> +\tregisterType(this);\n> +}\n> +\n> +/**\n> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n> + *\n> + * \\return A unique pointer to a new instance of the CameraSensorHelper subclass\n> + * corresponding to the factory\n> + */\n> +std::unique_ptr CameraSensorHelperFactory::create(const std::string &name)\n> +{\n> +\tstd::vector &factories =\n> +\t\tCameraSensorHelperFactory::factories();\n> +\n> +\tfor (CameraSensorHelperFactory *factory : factories) {\n> +\t\tif (name != factory->name_)\n> +\t\t\tcontinue;\n> +\n> +\t\tCameraSensorHelper *helper = factory->createInstance();\n> +\t\treturn std::unique_ptr(helper);\n> +\t}\n> +\n> +\treturn nullptr;\n> +}\n> +\n> +/**\n> + * \\brief Add a camera sensor helper class to the registry\n> + * \\param[in] factory Factory to use to construct the camera sensor helper\n> + *\n> + * The caller is responsible to guarantee the uniqueness of the camera sensor helper\n> + * name.\n> + */\n> +void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory *factory)\n> +{\n> +\tstd::vector &factories = CameraSensorHelperFactory::factories();\n> +\n> +\tfactories.push_back(factory);\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the list of all camera sensor helper factories\n> + *\n> + * The static factories map is defined inside the function to ensures it gets\n> + * initialized on first use, without any dependency on link order.\n> + *\n> + * \\return The list of camera sensor helper factories\n> + */\n> +std::vector &CameraSensorHelperFactory::factories()\n> +{\n> +\tstatic std::vector factories;\n> +\treturn factories;\n> +}\n> +\n> +/**\n> + * CameraSensorHelperFactory::createInstance()\n> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n> + *\n> + * This virtual function is implemented by the REGISTER_CAMERA_SENSOR_HELPER()\n> + * macro. It creates a camera sensor helper instance associated with the camera\n> + * sensor model.\n> + *\n> + * \\return A pointer to a newly constructed instance of the CameraSensorHelper\n> + * subclass corresponding to the factory\n> + */\n> +\n> +/**\n> + * \\def REGISTER_CAMERA_SENSOR_HELPER\n> + * \\brief Register a camera sensor helper with the camera sensor helper factory\n> + * \\param[in] name Sensor model name used to register the class\n> + * \\param[in] helper Class name of CameraSensorHelper derived class to register\n> + *\n> + * Register a CameraSensorHelper subclass with the factory and make it available to\n> + * try and match devices.\n> + */\n> +\n> +/**\n> + * \\class CameraSensorHelperImx219\n> + * \\brief Create and give helpers for the imx219 sensor\n> + */\n> +class CameraSensorHelperImx219 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperImx219()\n> +\t{\n> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 0, -1, 256, 256 };\n> +\t}\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> +\n> +/**\n> + * \\class CameraSensorHelperOv5670\n> + * \\brief Create and give helpers for the ov5670 sensor\n> + */\n> +class CameraSensorHelperOv5670 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperOv5670()\n> +\t{\n> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 };\n> +\t}\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n> +\n> +/**\n> + * \\class CameraSensorHelperOv5693\n> + * \\brief Create and give helpers for the ov5693 sensor\n> + */\n> +class CameraSensorHelperOv5693 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperOv5693()\n> +\t{\n> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 16 };\n> +\t}\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> new file mode 100644\n> index 00000000..c73900df\n> --- /dev/null\n> +++ b/src/ipa/libipa/camera_sensor_helper.h\n> @@ -0,0 +1,86 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * camera_sensor_helper.h - Helper class that performs sensor-specific parameter computations\n> + */\n> +#ifndef __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n> +#define __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n> +\n> +#include \n> +\n> +#include \n> +#include \n\nAh you have a unique_ptr<> here too, so should be included\nhere\n\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelper() = default;\n> +\tvirtual ~CameraSensorHelper() = default;\n> +\n> +\tvirtual uint32_t gainCode(double gain) const;\n> +\tvirtual double gain(uint32_t gainCode) const;\n> +\n> +protected:\n> +\tenum AnalogueGainType {\n> +\t\tAnalogueGainLinear = 0,\n> +\t\tAnalogueGainExponential = 2,\n> +\t};\n> +\n> +\tstruct AnalogueGainConstants {\n> +\t\tAnalogueGainType type;\n> +\t\tint16_t m0;\n> +\t\tint16_t c0;\n> +\t\tint16_t m1;\n> +\t\tint16_t c1;\n> +\t};\n> +\n> +\tAnalogueGainConstants analogueGainConstants_;\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)\n> +};\n> +\n> +class CameraSensorHelperFactory\n> +{\n> +public:\n> +\tCameraSensorHelperFactory(const std::string name);\n> +\tvirtual ~CameraSensorHelperFactory() = default;\n> +\n> +\tstatic std::unique_ptr create(const std::string &name);\n> +\n> +\tstatic void registerType(CameraSensorHelperFactory *factory);\n> +\tstatic std::vector &factories();\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)\n> +\tvirtual CameraSensorHelper *createInstance() = 0;\n\nCan a pure virtual private method be overridden by derived classes ?\nApparently so, I was expecting it to be protected\n\n> +\n> +\tstd::string name_;\n> +};\n> +\n> +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper) \\\n> +class helper##Factory final : public CameraSensorHelperFactory \\\n> +{ \\\n> +public: \\\n> +\thelper##Factory() : CameraSensorHelperFactory(name) {} \\\n> +\t\t\t\t\t\t\t\t \\\n> +private: \\\n> +\tCameraSensorHelper *createInstance() \\\n> +\t{ \\\n> +\t\treturn new helper(); \\\n> +\t} \\\n> +}; \\\n> +static helper##Factory global_##helper##Factory;\n\nSome of the line closing \\ render unaligned. Shouldn't you align with\ntabs ?\n\nOverall, just minors again. With the above fixed and the doxygen thing\nclarified\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 038fc490..dc90542c 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -2,11 +2,13 @@\n>\n> libipa_headers = files([\n> 'algorithm.h',\n> + 'camera_sensor_helper.h',\n> 'histogram.h'\n> ])\n>\n> libipa_sources = files([\n> 'algorithm.cpp',\n> + 'camera_sensor_helper.cpp',\n> 'histogram.cpp'\n> ])\n>\n> --\n> 2.30.2\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 15C81C3218\n\tfor ;\n\tMon, 21 Jun 2021 17:07:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3525A68935;\n\tMon, 21 Jun 2021 19:07:48 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F1E7A60295\n\tfor ;\n\tMon, 21 Jun 2021 19:07:46 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 6455B1BF204;\n\tMon, 21 Jun 2021 17:07:46 +0000 (UTC)"],"Date":"Mon, 21 Jun 2021 19:08:35 +0200","From":"Jacopo Mondi ","To":"Jean-Michel Hautbois ","Message-ID":"<20210621170835.65rp5quso23gj4dp@uno.localdomain>","References":"<20210621114738.52267-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210621114738.52267-2-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210621114738.52267-2-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 1/2] ipa: Create a camera sensor\n\thelper class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17680,"web_url":"https://patchwork.libcamera.org/comment/17680/","msgid":"","date":"2021-06-22T08:23:08","subject":"Re: [libcamera-devel] [PATCH v5 1/2] ipa: Create a camera sensor\n\thelper class","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 21/06/2021 19:08, Jacopo Mondi wrote:\n> Hi Jean-Micheal,\n> \n> On Mon, Jun 21, 2021 at 01:47:37PM +0200, Jean-Michel Hautbois wrote:\n>> For various sensor operations, it may be needed to do sensor specific\n>> computations, like analogue gain or vertical blanking.\n>>\n>> This commit introduces a new camera sensor helper in libipa which aims\n>> to solve this specific issue.\n>> It is based on the MIPI alliance Specification for Camera Command Set\n>> and implements, for now, only the analogue \"Global gain\" mode.\n>> Setting analogue gain for a specific sensor is not a straightforward\n>> operation, as one needs to know how the gain is calculated for it.\n>>\n>> Three helpers are created in this patch: imx219, ov5670 and ov5693.\n>>\n>> Adding a new sensor is pretty straightforward as one only needs to\n>> implement the sub-class for it and register that class to the\n>> CameraSensorHelperFactory.\n>>\n>> Signed-off-by: Jean-Michel Hautbois \n>> ---\n>> src/ipa/libipa/camera_sensor_helper.cpp | 313 ++++++++++++++++++++++++\n>> src/ipa/libipa/camera_sensor_helper.h | 86 +++++++\n>> src/ipa/libipa/meson.build | 2 +\n>> 3 files changed, 401 insertions(+)\n>> create mode 100644 src/ipa/libipa/camera_sensor_helper.cpp\n>> create mode 100644 src/ipa/libipa/camera_sensor_helper.h\n>>\n>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n>> new file mode 100644\n>> index 00000000..4bcea021\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n>> @@ -0,0 +1,313 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Google Inc.\n>> + *\n>> + * camera_sensor_helper.cpp - Helper class that performs sensor-specific parameter computations\n>> + */\n>> +#include \"camera_sensor_helper.h\"\n> \n> As suggested in v4:\n> \n> #include \n> \n>> +\n>> +#include \"libcamera/internal/log.h\"\n>> +\n>> +/**\n>> + * \\file camera_sensor_helper.h\n>> + * \\brief Helper class that performs sensor-specific parameter computations\n>> + *\n>> + * Computation of sensor configuration parameters is a sensor specific\n>> + * operation. Each CameraHelper derived class computes the value of\n>> + * configuration parameters, for example the analogue gain value, using\n>> + * sensor-specific functions and constants.\n>> + *\n>> + * Every subclass of CameraSensorHelper shall be registered with libipa using\n>> + * the REGISTER_CAMERA_SENSOR_HELPER() macro.\n>> + */\n>> +\n>> +namespace libcamera {\n>> +\n>> +LOG_DEFINE_CATEGORY(CameraSensorHelper)\n>> +\n>> +namespace ipa {\n>> +\n>> +/**\n>> + * \\class CameraSensorHelper\n>> + * \\brief Base class for computing sensor tuning parameters using sensor-specific constants\n>> + *\n>> + * CameraSensorHelper derived class instances are sensor specific. Each\n>> + * supported sensor will have an associated base class defined.\n>> + */\n>> +\n>> +/**\n> \n> As you have moved the constructor you should use the \\fn anchor to\n> tell doxygen what function is this block documenting\n> \n> weird, I see src/ipa/libipa in the list of Doxygen INPUT files but I\n> get no complaints for this. Also you're not documenting the public\n> destructor, we should get a warning o_0 Why doesn't that happen ?\n> \n\nI am not really sure of that... Anyone has an idea ?\nIn the meantime I added a:\n\t\\fn CameraSensorHelper::CameraSensorHelper\nAnd I get a warning:\n\"warning: Compound libcamera::ipa::CameraSensorHelper is not documented.\"\n\n> \n>> + * \\brief Construct a CameraSensorHelper instance\n>> + *\n>> + * CameraSensorHelper derived class instances shall never be constructed manually\n>> + * but always through the CameraSensorHelperFactory::create() method.\n>> + */\n>> +\n>> +/**\n>> + * CameraSensorHelper::gainCode(double gain)\n>> + * \\brief Compute gain code from the analogue gain absolute value\n>> + * \\param[in] gain The real gain to pass\n>> + * \\return the gain code to pass to V4l2\n>> + *\n>> + * This function aims to abstract the calculation of the gain letting the IPA\n>> + * use the real gain for its estimations.\n>> + *\n>> + * The parameters come from the MIPI Alliance Camera Specification for\n>> + * Camera Command Set (CCS).\n>> + */\n>> +uint32_t CameraSensorHelper::gainCode(double gain) const\n>> +{\n>> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n>> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n>> +\n> \n> Rogue tab as signaled by 'git am'\n> \n\nI don't have it here, maybe already solved in-between :-)\n\n>> +\treturn (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n>> +\t (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n>> +}\n>> +\n>> +/**\n>> + * CameraSensorHelper::gain\n>> + * \\brief Compute the real gain from the V4l2 subdev control gain code\n>> + * \\param[in] gainCode The V4l2 subdev control gain\n>> + * \\return The real gain\n>> + *\n>> + * This function aims to abstract the calculation of the gain letting the IPA\n>> + * use the real gain for its estimations. It is the counterpart of the function\n>> + * CameraSensorHelper::gainCode.\n>> + *\n>> + * The parameters come from the MIPI Alliance Camera Specification for\n>> + * Camera Command Set (CCS).\n>> + */\n>> +double CameraSensorHelper::gain(uint32_t gainCode) const\n>> +{\n>> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n>> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n>> +\n>> +\treturn (analogueGainConstants_.m0 * gainCode + analogueGainConstants_.c0) /\n>> +\t (analogueGainConstants_.m1 * gainCode + analogueGainConstants_.c1);\n> \n> You return a double but all the fields are int16_t. Won't this\n> truncate the result to a int, then convert it to a double on return ?\n> IOW if you have a the real division result x,y you would get x,0 ?\n> \n> Is it necessary to cast gain_code to double ?\n> \n\nYes, done. Weird, I did not have a warning for that...\nWe may need to use -Weverything for clang ? :-)\n\n>> +}\n>> +\n>> +/**\n>> + * \\enum CameraSensorHelper::AnalogueGainType\n>> + * \\brief the gain calculation modes as defined by the MIPI CCS\n>> + *\n>> + * Describes the image sensor analogue gain capabilities.\n>> + * Two modes are possible, depending on the sensor: Global and Alternate.\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainLinear\n>> + * \\brief Gain is computed using linear gain estimation\n>> + *\n>> + * The relationship between the integer gain parameter and the resulting gain\n>> + * multiplier is given by the following equation:\n>> + *\n>> + * \\f$gain=\\frac{m0x+c0}{m1x+c1}\\f$\n>> + *\n>> + * Where 'x' is the gain control parameter, and m0, m1, c0 and c1 are\n>> + * image-sensor-specific constants of the sensor.\n>> + * These constants are static parameters, and for any given image sensor either\n>> + * m0 or m1 shall be zero.\n>> + *\n>> + * The full Gain equation therefore reduces to either:\n>> + *\n>> + * \\f$gain=\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainExponential\n>> + * \\brief Gain is computed using exponential gain estimation (introduced in CCS v1.1)\n>> + *\n>> + * Starting with CCS v1.1, Alternate Global Analogue Gain is also available.\n>> + * If the image sensor supports it, then the global analogue gain can be controlled\n>> + * by linear and exponential gain formula:\n>> + *\n>> + * \\f$gain = analogLinearGainGlobal * 2^{analogExponentialGainGlobal}\\f$\n>> + * \\todo not implemented in libipa\n>> + */\n>> +\n>> +/**\n>> + * \\struct CameraSensorHelper::AnalogueGainConstants\n>> + * \\brief Analogue gain constants used for gain calculation\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainConstants::type\n>> + * \\brief Analogue gain calculation mode\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainConstants::m0\n>> + * \\brief Constant used in the analogue Gain coding/decoding\n>> + *\n>> + * Note: either m0 or m1 shall be zero.\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainConstants::c0\n>> + * \\brief Constant used in the analogue gain coding/decoding\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainConstants::m1\n>> + * \\brief Constant used in the analogue gain coding/decoding\n>> + *\n>> + * Note: either m0 or m1 shall be zero.\n> \n> Doxygen supports \\note\n> \n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainConstants::c1\n>> + * \\brief Constant used in the analogue gain coding/decoding\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::analogueGainConstants_\n>> + * \\brief The analogue gain parameters used for calculation\n>> + *\n>> + * The analogue gain is calculated through a formula, and its parameters are\n>> + * sensor specific. Use this variable to store the values at init time.\n>> + *\n> \n> Rogue empty line\n> \n>> + */\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperFactory\n>> + * \\brief Registration of CameraSensorHelperFactory classes and creation of instances\n>> + *\n>> + * To facilitate discovery and instantiation of CameraSensorHelper classes, the\n>> + * CameraSensorHelperFactory class maintains a registry of camera sensor helper\n>> + * classes. Each CameraSensorHelper subclass shall register itself using the\n>> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a corresponding\n>> + * instance of a CameraSensorHelperFactory subclass and register it with the\n>> + * static list of factories.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct a camera sensor helper factory\n>> + * \\param[in] name Name of the camera sensor helper class\n>> + *\n>> + * Creating an instance of the factory registers it with the global list of\n>> + * factories, accessible through the factories() function.\n>> + *\n>> + * The factory \\a name is used for debug purpose and shall be unique.\n>> + */\n>> +CameraSensorHelperFactory::CameraSensorHelperFactory(const std::string name)\n>> +\t: name_(name)\n>> +{\n>> +\tregisterType(this);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n>> + *\n>> + * \\return A unique pointer to a new instance of the CameraSensorHelper subclass\n>> + * corresponding to the factory\n>> + */\n>> +std::unique_ptr CameraSensorHelperFactory::create(const std::string &name)\n>> +{\n>> +\tstd::vector &factories =\n>> +\t\tCameraSensorHelperFactory::factories();\n>> +\n>> +\tfor (CameraSensorHelperFactory *factory : factories) {\n>> +\t\tif (name != factory->name_)\n>> +\t\t\tcontinue;\n>> +\n>> +\t\tCameraSensorHelper *helper = factory->createInstance();\n>> +\t\treturn std::unique_ptr(helper);\n>> +\t}\n>> +\n>> +\treturn nullptr;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Add a camera sensor helper class to the registry\n>> + * \\param[in] factory Factory to use to construct the camera sensor helper\n>> + *\n>> + * The caller is responsible to guarantee the uniqueness of the camera sensor helper\n>> + * name.\n>> + */\n>> +void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory *factory)\n>> +{\n>> +\tstd::vector &factories = CameraSensorHelperFactory::factories();\n>> +\n>> +\tfactories.push_back(factory);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Retrieve the list of all camera sensor helper factories\n>> + *\n>> + * The static factories map is defined inside the function to ensures it gets\n>> + * initialized on first use, without any dependency on link order.\n>> + *\n>> + * \\return The list of camera sensor helper factories\n>> + */\n>> +std::vector &CameraSensorHelperFactory::factories()\n>> +{\n>> +\tstatic std::vector factories;\n>> +\treturn factories;\n>> +}\n>> +\n>> +/**\n>> + * CameraSensorHelperFactory::createInstance()\n>> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n>> + *\n>> + * This virtual function is implemented by the REGISTER_CAMERA_SENSOR_HELPER()\n>> + * macro. It creates a camera sensor helper instance associated with the camera\n>> + * sensor model.\n>> + *\n>> + * \\return A pointer to a newly constructed instance of the CameraSensorHelper\n>> + * subclass corresponding to the factory\n>> + */\n>> +\n>> +/**\n>> + * \\def REGISTER_CAMERA_SENSOR_HELPER\n>> + * \\brief Register a camera sensor helper with the camera sensor helper factory\n>> + * \\param[in] name Sensor model name used to register the class\n>> + * \\param[in] helper Class name of CameraSensorHelper derived class to register\n>> + *\n>> + * Register a CameraSensorHelper subclass with the factory and make it available to\n>> + * try and match devices.\n>> + */\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperImx219\n>> + * \\brief Create and give helpers for the imx219 sensor\n>> + */\n>> +class CameraSensorHelperImx219 : public CameraSensorHelper\n>> +{\n>> +public:\n>> +\tCameraSensorHelperImx219()\n>> +\t{\n>> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 0, -1, 256, 256 };\n>> +\t}\n>> +};\n>> +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperOv5670\n>> + * \\brief Create and give helpers for the ov5670 sensor\n>> + */\n>> +class CameraSensorHelperOv5670 : public CameraSensorHelper\n>> +{\n>> +public:\n>> +\tCameraSensorHelperOv5670()\n>> +\t{\n>> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 };\n>> +\t}\n>> +};\n>> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperOv5693\n>> + * \\brief Create and give helpers for the ov5693 sensor\n>> + */\n>> +class CameraSensorHelperOv5693 : public CameraSensorHelper\n>> +{\n>> +public:\n>> +\tCameraSensorHelperOv5693()\n>> +\t{\n>> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 16 };\n>> +\t}\n>> +};\n>> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n>> +\n>> +} /* namespace ipa */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n>> new file mode 100644\n>> index 00000000..c73900df\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/camera_sensor_helper.h\n>> @@ -0,0 +1,86 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2021, Google Inc.\n>> + *\n>> + * camera_sensor_helper.h - Helper class that performs sensor-specific parameter computations\n>> + */\n>> +#ifndef __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n>> +#define __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n>> +\n>> +#include \n>> +\n>> +#include \n>> +#include \n> \n> Ah you have a unique_ptr<> here too, so should be included\n> here\n> \n>> +\n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa {\n>> +\n>> +class CameraSensorHelper\n>> +{\n>> +public:\n>> +\tCameraSensorHelper() = default;\n>> +\tvirtual ~CameraSensorHelper() = default;\n>> +\n>> +\tvirtual uint32_t gainCode(double gain) const;\n>> +\tvirtual double gain(uint32_t gainCode) const;\n>> +\n>> +protected:\n>> +\tenum AnalogueGainType {\n>> +\t\tAnalogueGainLinear = 0,\n>> +\t\tAnalogueGainExponential = 2,\n>> +\t};\n>> +\n>> +\tstruct AnalogueGainConstants {\n>> +\t\tAnalogueGainType type;\n>> +\t\tint16_t m0;\n>> +\t\tint16_t c0;\n>> +\t\tint16_t m1;\n>> +\t\tint16_t c1;\n>> +\t};\n>> +\n>> +\tAnalogueGainConstants analogueGainConstants_;\n>> +\n>> +private:\n>> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)\n>> +};\n>> +\n>> +class CameraSensorHelperFactory\n>> +{\n>> +public:\n>> +\tCameraSensorHelperFactory(const std::string name);\n>> +\tvirtual ~CameraSensorHelperFactory() = default;\n>> +\n>> +\tstatic std::unique_ptr create(const std::string &name);\n>> +\n>> +\tstatic void registerType(CameraSensorHelperFactory *factory);\n>> +\tstatic std::vector &factories();\n>> +\n>> +private:\n>> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)\n>> +\tvirtual CameraSensorHelper *createInstance() = 0;\n> \n> Can a pure virtual private method be overridden by derived classes ?\n> Apparently so, I was expecting it to be protected\n> \n>> +\n>> +\tstd::string name_;\n>> +};\n>> +\n>> +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper) \\\n>> +class helper##Factory final : public CameraSensorHelperFactory \\\n>> +{ \\\n>> +public: \\\n>> +\thelper##Factory() : CameraSensorHelperFactory(name) {} \\\n>> +\t\t\t\t\t\t\t\t \\\n>> +private: \\\n>> +\tCameraSensorHelper *createInstance() \\\n>> +\t{ \\\n>> +\t\treturn new helper(); \\\n>> +\t} \\\n>> +}; \\\n>> +static helper##Factory global_##helper##Factory;\n> \n> Some of the line closing \\ render unaligned. Shouldn't you align with\n> tabs ?\n> \n> Overall, just minors again. With the above fixed and the doxygen thing\n> clarified\n> Reviewed-by: Jacopo Mondi \n> \n\nThx, I don't have Doxygen answer though :-(.\n\n> Thanks\n> j\n> \n> \n>> +\n>> +} /* namespace ipa */\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n>> index 038fc490..dc90542c 100644\n>> --- a/src/ipa/libipa/meson.build\n>> +++ b/src/ipa/libipa/meson.build\n>> @@ -2,11 +2,13 @@\n>>\n>> libipa_headers = files([\n>> 'algorithm.h',\n>> + 'camera_sensor_helper.h',\n>> 'histogram.h'\n>> ])\n>>\n>> libipa_sources = files([\n>> 'algorithm.cpp',\n>> + 'camera_sensor_helper.cpp',\n>> 'histogram.cpp'\n>> ])\n>>\n>> --\n>> 2.30.2\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0C4B9C321A\n\tfor ;\n\tTue, 22 Jun 2021 08:23:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3704568936;\n\tTue, 22 Jun 2021 10:23:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D857D60297\n\tfor ;\n\tTue, 22 Jun 2021 10:23:10 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:59b9:69b1:8141:7f28])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 91673A66;\n\tTue, 22 Jun 2021 10:23:10 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"viJmBKzL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624350190;\n\tbh=mYrYtD4YELnDUtIv9/xaP8PuPyltSKlXaK+S8VV4/cU=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=viJmBKzLE7C7DrGv6Wa9zNd+x0GpE18c5qgPy/O6rXmcP23TmyiQeISzSgWk2vNfm\n\t83/v68IHHaf79toxD1UTV5km9ShOEjCPwCRAZQewRg/KR0QHCXNLn489ghuC5NIxqz\n\t6un3eiIKq+yz1dnsVc2vr1qaXkqoEnphZv7l5UN0=","To":"Jacopo Mondi ","References":"<20210621114738.52267-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210621114738.52267-2-jeanmichel.hautbois@ideasonboard.com>\n\t<20210621170835.65rp5quso23gj4dp@uno.localdomain>","From":"Jean-Michel Hautbois ","Message-ID":"","Date":"Tue, 22 Jun 2021 10:23:08 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.8.1","MIME-Version":"1.0","In-Reply-To":"<20210621170835.65rp5quso23gj4dp@uno.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v5 1/2] ipa: Create a camera sensor\n\thelper class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17684,"web_url":"https://patchwork.libcamera.org/comment/17684/","msgid":"<20210622090813.czpn6fayuun34wcv@uno.localdomain>","date":"2021-06-22T09:08:13","subject":"Re: [libcamera-devel] [PATCH v5 1/2] ipa: Create a camera sensor\n\thelper class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Jean-Michel,\n\nOn Tue, Jun 22, 2021 at 10:23:08AM +0200, Jean-Michel Hautbois wrote:\n> Hi Jacopo,\n>\n> On 21/06/2021 19:08, Jacopo Mondi wrote:\n> > Hi Jean-Micheal,\n> >\n> > On Mon, Jun 21, 2021 at 01:47:37PM +0200, Jean-Michel Hautbois wrote:\n> >> For various sensor operations, it may be needed to do sensor specific\n> >> computations, like analogue gain or vertical blanking.\n> >>\n> >> This commit introduces a new camera sensor helper in libipa which aims\n> >> to solve this specific issue.\n> >> It is based on the MIPI alliance Specification for Camera Command Set\n> >> and implements, for now, only the analogue \"Global gain\" mode.\n> >> Setting analogue gain for a specific sensor is not a straightforward\n> >> operation, as one needs to know how the gain is calculated for it.\n> >>\n> >> Three helpers are created in this patch: imx219, ov5670 and ov5693.\n> >>\n> >> Adding a new sensor is pretty straightforward as one only needs to\n> >> implement the sub-class for it and register that class to the\n> >> CameraSensorHelperFactory.\n> >>\n> >> Signed-off-by: Jean-Michel Hautbois \n> >> ---\n> >> src/ipa/libipa/camera_sensor_helper.cpp | 313 ++++++++++++++++++++++++\n> >> src/ipa/libipa/camera_sensor_helper.h | 86 +++++++\n> >> src/ipa/libipa/meson.build | 2 +\n> >> 3 files changed, 401 insertions(+)\n> >> create mode 100644 src/ipa/libipa/camera_sensor_helper.cpp\n> >> create mode 100644 src/ipa/libipa/camera_sensor_helper.h\n> >>\n> >> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> >> new file mode 100644\n> >> index 00000000..4bcea021\n> >> --- /dev/null\n> >> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> >> @@ -0,0 +1,313 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2021, Google Inc.\n> >> + *\n> >> + * camera_sensor_helper.cpp - Helper class that performs sensor-specific parameter computations\n> >> + */\n> >> +#include \"camera_sensor_helper.h\"\n> >\n> > As suggested in v4:\n> >\n> > #include \n> >\n> >> +\n> >> +#include \"libcamera/internal/log.h\"\n> >> +\n> >> +/**\n> >> + * \\file camera_sensor_helper.h\n> >> + * \\brief Helper class that performs sensor-specific parameter computations\n> >> + *\n> >> + * Computation of sensor configuration parameters is a sensor specific\n> >> + * operation. Each CameraHelper derived class computes the value of\n> >> + * configuration parameters, for example the analogue gain value, using\n> >> + * sensor-specific functions and constants.\n> >> + *\n> >> + * Every subclass of CameraSensorHelper shall be registered with libipa using\n> >> + * the REGISTER_CAMERA_SENSOR_HELPER() macro.\n> >> + */\n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +LOG_DEFINE_CATEGORY(CameraSensorHelper)\n> >> +\n> >> +namespace ipa {\n> >> +\n> >> +/**\n> >> + * \\class CameraSensorHelper\n> >> + * \\brief Base class for computing sensor tuning parameters using sensor-specific constants\n> >> + *\n> >> + * CameraSensorHelper derived class instances are sensor specific. Each\n> >> + * supported sensor will have an associated base class defined.\n> >> + */\n> >> +\n> >> +/**\n> >\n> > As you have moved the constructor you should use the \\fn anchor to\n> > tell doxygen what function is this block documenting\n> >\n> > weird, I see src/ipa/libipa in the list of Doxygen INPUT files but I\n> > get no complaints for this. Also you're not documenting the public\n> > destructor, we should get a warning o_0 Why doesn't that happen ?\n> >\n>\n> I am not really sure of that... Anyone has an idea ?\n> In the meantime I added a:\n> \t\\fn CameraSensorHelper::CameraSensorHelper\n> And I get a warning:\n> \"warning: Compound libcamera::ipa::CameraSensorHelper is not documented.\"\n>\n\nRunning doxygen 1.9.1 here, and I don't even get the warning :/\n\n> >\n> >> + * \\brief Construct a CameraSensorHelper instance\n> >> + *\n> >> + * CameraSensorHelper derived class instances shall never be constructed manually\n> >> + * but always through the CameraSensorHelperFactory::create() method.\n> >> + */\n> >> +\n> >> +/**\n> >> + * CameraSensorHelper::gainCode(double gain)\n> >> + * \\brief Compute gain code from the analogue gain absolute value\n> >> + * \\param[in] gain The real gain to pass\n> >> + * \\return the gain code to pass to V4l2\n\nSince I'm here again, \\return statement goes last, after the\nfree-formed text paragraph :)\n\n> >> + *\n> >> + * This function aims to abstract the calculation of the gain letting the IPA\n> >> + * use the real gain for its estimations.\n> >> + *\n> >> + * The parameters come from the MIPI Alliance Camera Specification for\n> >> + * Camera Command Set (CCS).\n> >> + */\n> >> +uint32_t CameraSensorHelper::gainCode(double gain) const\n> >> +{\n> >> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n> >> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> >> +\n> >\n> > Rogue tab as signaled by 'git am'\n> >\n>\n> I don't have it here, maybe already solved in-between :-)\n>\n\nMy editor eats up rougues tabs and spaces when I hit ':w', that's why\nyou don't see it here.. I had a look again and it's there in the patch\n:)\n\n> >> +\treturn (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n> >> +\t (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n> >> +}\n> >> +\n> >> +/**\n> >> + * CameraSensorHelper::gain\n> >> + * \\brief Compute the real gain from the V4l2 subdev control gain code\n> >> + * \\param[in] gainCode The V4l2 subdev control gain\n> >> + * \\return The real gain\n> >> + *\n> >> + * This function aims to abstract the calculation of the gain letting the IPA\n> >> + * use the real gain for its estimations. It is the counterpart of the function\n> >> + * CameraSensorHelper::gainCode.\n> >> + *\n> >> + * The parameters come from the MIPI Alliance Camera Specification for\n> >> + * Camera Command Set (CCS).\n> >> + */\n> >> +double CameraSensorHelper::gain(uint32_t gainCode) const\n> >> +{\n> >> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n> >> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> >> +\n> >> +\treturn (analogueGainConstants_.m0 * gainCode + analogueGainConstants_.c0) /\n> >> +\t (analogueGainConstants_.m1 * gainCode + analogueGainConstants_.c1);\n> >\n> > You return a double but all the fields are int16_t. Won't this\n> > truncate the result to a int, then convert it to a double on return ?\n> > IOW if you have a the real division result x,y you would get x,0 ?\n> >\n> > Is it necessary to cast gain_code to double ?\n> >\n>\n> Yes, done. Weird, I did not have a warning for that...\n> We may need to use -Weverything for clang ? :-)\n>\n\nI don't get a warning if not from my eyes, possibily because my poor\nunderstanding of C++\n\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\enum CameraSensorHelper::AnalogueGainType\n> >> + * \\brief the gain calculation modes as defined by the MIPI CCS\n> >> + *\n> >> + * Describes the image sensor analogue gain capabilities.\n> >> + * Two modes are possible, depending on the sensor: Global and Alternate.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var CameraSensorHelper::AnalogueGainLinear\n> >> + * \\brief Gain is computed using linear gain estimation\n> >> + *\n> >> + * The relationship between the integer gain parameter and the resulting gain\n> >> + * multiplier is given by the following equation:\n> >> + *\n> >> + * \\f$gain=\\frac{m0x+c0}{m1x+c1}\\f$\n> >> + *\n> >> + * Where 'x' is the gain control parameter, and m0, m1, c0 and c1 are\n> >> + * image-sensor-specific constants of the sensor.\n> >> + * These constants are static parameters, and for any given image sensor either\n> >> + * m0 or m1 shall be zero.\n> >> + *\n> >> + * The full Gain equation therefore reduces to either:\n> >> + *\n> >> + * \\f$gain=\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var CameraSensorHelper::AnalogueGainExponential\n> >> + * \\brief Gain is computed using exponential gain estimation (introduced in CCS v1.1)\n> >> + *\n> >> + * Starting with CCS v1.1, Alternate Global Analogue Gain is also available.\n> >> + * If the image sensor supports it, then the global analogue gain can be controlled\n> >> + * by linear and exponential gain formula:\n> >> + *\n> >> + * \\f$gain = analogLinearGainGlobal * 2^{analogExponentialGainGlobal}\\f$\n> >> + * \\todo not implemented in libipa\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\struct CameraSensorHelper::AnalogueGainConstants\n> >> + * \\brief Analogue gain constants used for gain calculation\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var CameraSensorHelper::AnalogueGainConstants::type\n> >> + * \\brief Analogue gain calculation mode\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var CameraSensorHelper::AnalogueGainConstants::m0\n> >> + * \\brief Constant used in the analogue Gain coding/decoding\n> >> + *\n> >> + * Note: either m0 or m1 shall be zero.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var CameraSensorHelper::AnalogueGainConstants::c0\n> >> + * \\brief Constant used in the analogue gain coding/decoding\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var CameraSensorHelper::AnalogueGainConstants::m1\n> >> + * \\brief Constant used in the analogue gain coding/decoding\n> >> + *\n> >> + * Note: either m0 or m1 shall be zero.\n> >\n> > Doxygen supports \\note\n> >\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var CameraSensorHelper::AnalogueGainConstants::c1\n> >> + * \\brief Constant used in the analogue gain coding/decoding\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var CameraSensorHelper::analogueGainConstants_\n> >> + * \\brief The analogue gain parameters used for calculation\n> >> + *\n> >> + * The analogue gain is calculated through a formula, and its parameters are\n> >> + * sensor specific. Use this variable to store the values at init time.\n> >> + *\n> >\n> > Rogue empty line\n> >\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\class CameraSensorHelperFactory\n> >> + * \\brief Registration of CameraSensorHelperFactory classes and creation of instances\n> >> + *\n> >> + * To facilitate discovery and instantiation of CameraSensorHelper classes, the\n> >> + * CameraSensorHelperFactory class maintains a registry of camera sensor helper\n> >> + * classes. Each CameraSensorHelper subclass shall register itself using the\n> >> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a corresponding\n> >> + * instance of a CameraSensorHelperFactory subclass and register it with the\n> >> + * static list of factories.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Construct a camera sensor helper factory\n> >> + * \\param[in] name Name of the camera sensor helper class\n> >> + *\n> >> + * Creating an instance of the factory registers it with the global list of\n> >> + * factories, accessible through the factories() function.\n> >> + *\n> >> + * The factory \\a name is used for debug purpose and shall be unique.\n> >> + */\n> >> +CameraSensorHelperFactory::CameraSensorHelperFactory(const std::string name)\n> >> +\t: name_(name)\n> >> +{\n> >> +\tregisterType(this);\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n> >> + *\n> >> + * \\return A unique pointer to a new instance of the CameraSensorHelper subclass\n> >> + * corresponding to the factory\n> >> + */\n> >> +std::unique_ptr CameraSensorHelperFactory::create(const std::string &name)\n> >> +{\n> >> +\tstd::vector &factories =\n> >> +\t\tCameraSensorHelperFactory::factories();\n> >> +\n> >> +\tfor (CameraSensorHelperFactory *factory : factories) {\n> >> +\t\tif (name != factory->name_)\n> >> +\t\t\tcontinue;\n> >> +\n> >> +\t\tCameraSensorHelper *helper = factory->createInstance();\n> >> +\t\treturn std::unique_ptr(helper);\n> >> +\t}\n> >> +\n> >> +\treturn nullptr;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Add a camera sensor helper class to the registry\n> >> + * \\param[in] factory Factory to use to construct the camera sensor helper\n> >> + *\n> >> + * The caller is responsible to guarantee the uniqueness of the camera sensor helper\n> >> + * name.\n> >> + */\n> >> +void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory *factory)\n> >> +{\n> >> +\tstd::vector &factories = CameraSensorHelperFactory::factories();\n> >> +\n> >> +\tfactories.push_back(factory);\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Retrieve the list of all camera sensor helper factories\n> >> + *\n> >> + * The static factories map is defined inside the function to ensures it gets\n> >> + * initialized on first use, without any dependency on link order.\n> >> + *\n> >> + * \\return The list of camera sensor helper factories\n> >> + */\n> >> +std::vector &CameraSensorHelperFactory::factories()\n> >> +{\n> >> +\tstatic std::vector factories;\n> >> +\treturn factories;\n> >> +}\n> >> +\n> >> +/**\n> >> + * CameraSensorHelperFactory::createInstance()\n> >> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n> >> + *\n> >> + * This virtual function is implemented by the REGISTER_CAMERA_SENSOR_HELPER()\n> >> + * macro. It creates a camera sensor helper instance associated with the camera\n> >> + * sensor model.\n> >> + *\n> >> + * \\return A pointer to a newly constructed instance of the CameraSensorHelper\n> >> + * subclass corresponding to the factory\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\def REGISTER_CAMERA_SENSOR_HELPER\n> >> + * \\brief Register a camera sensor helper with the camera sensor helper factory\n> >> + * \\param[in] name Sensor model name used to register the class\n> >> + * \\param[in] helper Class name of CameraSensorHelper derived class to register\n> >> + *\n> >> + * Register a CameraSensorHelper subclass with the factory and make it available to\n> >> + * try and match devices.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\class CameraSensorHelperImx219\n> >> + * \\brief Create and give helpers for the imx219 sensor\n> >> + */\n> >> +class CameraSensorHelperImx219 : public CameraSensorHelper\n> >> +{\n> >> +public:\n> >> +\tCameraSensorHelperImx219()\n> >> +\t{\n> >> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 0, -1, 256, 256 };\n> >> +\t}\n> >> +};\n> >> +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> >> +\n> >> +/**\n> >> + * \\class CameraSensorHelperOv5670\n> >> + * \\brief Create and give helpers for the ov5670 sensor\n> >> + */\n> >> +class CameraSensorHelperOv5670 : public CameraSensorHelper\n> >> +{\n> >> +public:\n> >> +\tCameraSensorHelperOv5670()\n> >> +\t{\n> >> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 };\n> >> +\t}\n> >> +};\n> >> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n> >> +\n> >> +/**\n> >> + * \\class CameraSensorHelperOv5693\n> >> + * \\brief Create and give helpers for the ov5693 sensor\n> >> + */\n> >> +class CameraSensorHelperOv5693 : public CameraSensorHelper\n> >> +{\n> >> +public:\n> >> +\tCameraSensorHelperOv5693()\n> >> +\t{\n> >> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 16 };\n> >> +\t}\n> >> +};\n> >> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> >> +\n> >> +} /* namespace ipa */\n> >> +\n> >> +} /* namespace libcamera */\n> >> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> >> new file mode 100644\n> >> index 00000000..c73900df\n> >> --- /dev/null\n> >> +++ b/src/ipa/libipa/camera_sensor_helper.h\n> >> @@ -0,0 +1,86 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2021, Google Inc.\n> >> + *\n> >> + * camera_sensor_helper.h - Helper class that performs sensor-specific parameter computations\n> >> + */\n> >> +#ifndef __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n> >> +#define __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__\n> >> +\n> >> +#include \n> >> +\n> >> +#include \n> >> +#include \n> >\n> > Ah you have a unique_ptr<> here too, so should be included\n> > here\n> >\n> >> +\n> >> +#include \n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +namespace ipa {\n> >> +\n> >> +class CameraSensorHelper\n> >> +{\n> >> +public:\n> >> +\tCameraSensorHelper() = default;\n> >> +\tvirtual ~CameraSensorHelper() = default;\n> >> +\n> >> +\tvirtual uint32_t gainCode(double gain) const;\n> >> +\tvirtual double gain(uint32_t gainCode) const;\n> >> +\n> >> +protected:\n> >> +\tenum AnalogueGainType {\n> >> +\t\tAnalogueGainLinear = 0,\n> >> +\t\tAnalogueGainExponential = 2,\n> >> +\t};\n> >> +\n> >> +\tstruct AnalogueGainConstants {\n> >> +\t\tAnalogueGainType type;\n> >> +\t\tint16_t m0;\n> >> +\t\tint16_t c0;\n> >> +\t\tint16_t m1;\n> >> +\t\tint16_t c1;\n> >> +\t};\n> >> +\n> >> +\tAnalogueGainConstants analogueGainConstants_;\n> >> +\n> >> +private:\n> >> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)\n> >> +};\n> >> +\n> >> +class CameraSensorHelperFactory\n> >> +{\n> >> +public:\n> >> +\tCameraSensorHelperFactory(const std::string name);\n> >> +\tvirtual ~CameraSensorHelperFactory() = default;\n> >> +\n> >> +\tstatic std::unique_ptr create(const std::string &name);\n> >> +\n> >> +\tstatic void registerType(CameraSensorHelperFactory *factory);\n> >> +\tstatic std::vector &factories();\n> >> +\n> >> +private:\n> >> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)\n> >> +\tvirtual CameraSensorHelper *createInstance() = 0;\n> >\n> > Can a pure virtual private method be overridden by derived classes ?\n> > Apparently so, I was expecting it to be protected\n> >\n> >> +\n> >> +\tstd::string name_;\n> >> +};\n> >> +\n> >> +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper) \\\n> >> +class helper##Factory final : public CameraSensorHelperFactory \\\n> >> +{ \\\n> >> +public: \\\n> >> +\thelper##Factory() : CameraSensorHelperFactory(name) {} \\\n> >> +\t\t\t\t\t\t\t\t \\\n> >> +private: \\\n> >> +\tCameraSensorHelper *createInstance() \\\n> >> +\t{ \\\n> >> +\t\treturn new helper(); \\\n> >> +\t} \\\n> >> +}; \\\n> >> +static helper##Factory global_##helper##Factory;\n> >\n> > Some of the line closing \\ render unaligned. Shouldn't you align with\n> > tabs ?\n> >\n> > Overall, just minors again. With the above fixed and the doxygen thing\n> > clarified\n> > Reviewed-by: Jacopo Mondi \n> >\n>\n> Thx, I don't have Doxygen answer though :-(.\n>\n> > Thanks\n> > j\n> >\n> >\n> >> +\n> >> +} /* namespace ipa */\n> >> +\n> >> +} /* namespace libcamera */\n> >> +\n> >> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n> >> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> >> index 038fc490..dc90542c 100644\n> >> --- a/src/ipa/libipa/meson.build\n> >> +++ b/src/ipa/libipa/meson.build\n> >> @@ -2,11 +2,13 @@\n> >>\n> >> libipa_headers = files([\n> >> 'algorithm.h',\n> >> + 'camera_sensor_helper.h',\n> >> 'histogram.h'\n> >> ])\n> >>\n> >> libipa_sources = files([\n> >> 'algorithm.cpp',\n> >> + 'camera_sensor_helper.cpp',\n> >> 'histogram.cpp'\n> >> ])\n> >>\n> >> --\n> >> 2.30.2\n> >>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6EB67C321B\n\tfor ;\n\tTue, 22 Jun 2021 09:07:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3D6AF68936;\n\tTue, 22 Jun 2021 11:07:26 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B8A6A6050B\n\tfor ;\n\tTue, 22 Jun 2021 11:07:24 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 2E2BD240005;\n\tTue, 22 Jun 2021 09:07:23 +0000 (UTC)"],"Date":"Tue, 22 Jun 2021 11:08:13 +0200","From":"Jacopo Mondi ","To":"Jean-Michel Hautbois ","Message-ID":"<20210622090813.czpn6fayuun34wcv@uno.localdomain>","References":"<20210621114738.52267-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210621114738.52267-2-jeanmichel.hautbois@ideasonboard.com>\n\t<20210621170835.65rp5quso23gj4dp@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 1/2] ipa: Create a camera sensor\n\thelper class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]