[{"id":17443,"web_url":"https://patchwork.libcamera.org/comment/17443/","msgid":"<20210607160533.xogfthhdgmzxy5pm@uno.localdomain>","date":"2021-06-07T16:05:33","subject":"Re: [libcamera-devel] [PATCH 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":"Hello Jean-Michel,\n\n\nOn Mon, Jun 07, 2021 at 02:35:32PM +0200, Jean-Michel Hautbois wrote:\n> Setting analogue gain for a specific sensor is not a straghtforward\n> operation, as one needs to know how the gain is calculated for it.\n\nPlease bear with my poor understanding of the issue, but looking at\nthe code it doesn't seem the different is in how the gain is\ncalculated but rather in which constant parameters are used.\n\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>\n\nI should first go through this document, so I'll probably have only\ndrive-by comments and nits here and there\n\n> Is makes it possible to only have 4 parameters to store per sensor, and\n> the gain calculation is then done identically for all of them. This\n> commit gives the gains for imx219 (based on RPi cam_helper), ov5670 and\n> ov5693.\n>\n> Adding a new sensor is pretty straightfoward as one only needs to\n> implement the sub-class for it and register that class to the\n> CameraSensorHelperFactory.\n\nAs asked offline, the current fatory implementation is the same as the\none we have for pipeline handlers, which requires static\ninitialization to make sure they are registered at library\ninitialization time. I suspect without this requirement the factory\nhere could be made simpler, but that's minor...\n\n>\n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/libipa/camera_sensor_helper.cpp | 361 ++++++++++++++++++++++++\n> src/ipa/libipa/camera_sensor_helper.h | 91 ++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 454 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..2bd82861\n> --- /dev/null\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -0,0 +1,361 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2021 Ideas On Board\n> + *\n> + * camera_sensor_helper.cpp - helper class providing camera informations\n\nNit: 'Helper'\nas I've received the same comment multiple times, 'information' is\nuncountable, so no plural\n\n> + */\n> +#include \"camera_sensor_helper.h\"\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/log.h\"\n> +\n> +/**\n> + * \\file camera_sensor_helper.h\n> + * \\brief Create helper class for each sensor\n> + *\n> + * Each camera sensor supported by libcamera may need specific informations to\n> + * be sent (analogue gain is sensor dependant for instance).\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 Create and give helpers for each camera sensor in the pipeline\n\nI would here and maybe above in the file description drop the 'create\nand give' part. The CameraSensorHelper base class computes sensor\ntuning parameters using sensor-specific constants. The factory instead\ndoes the actual object creation\n\n> + *\n sensor-specific constants. The factory instead does the actual object\n creation\n\n> + * CameraSensorHelper instances are unique and sensor dependant.\n\nIs the base class meant to be used as a default if not sensor-specific\nsub-class is registered ?\n\n> + */\n> +\n> +/**\n> + * \\brief Construct a CameraSensorHelper instance\n> + * \\param[in] name The sensor model name\n> + *\n> + * The CameraSensorHelper instances shall never be constructed manually, but always\n> + * through the CameraSensorHelperFactory::create() method implemented by the\n> + * respective factories.\n> + */\n> +\n\nEmpty line\n\n> +CameraSensorHelper::CameraSensorHelper(const char *name)\n> +\t: name_(name)\n> +{\n> +\tanalogueGain_ = { GlobalGain, 1, 1, 0, 0 };\n> +}\n\nI wonder, we don't support AlternateGlobalGain at the moment, do you\nexpect it to be required soon ?\n\n> +\n> +CameraSensorHelper::~CameraSensorHelper()\n> +{\n> +}\n> +\n> +/**\n> + * \\fn CameraSensorHelper::getGainCode(double gain)\n\nIf you document the function just defined below, you don't need the\n\\fn anchor\n\nLibrary-wide we don't use 'get' for getters, so this might just be\ngainCode() ?\n\n> + * \\brief Get the analogue gain code to pass to V4L2 subdev control\n\nAren't we computing the gain code from the analogue gain value ?\n\n> + * \\param[in] gain The real gain to pass\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\nDo you mean the m0, c0, etc etc paramteres ? Maybe this is better\nspecified in the enum documentation ?\n\n> + */\n> +uint32_t CameraSensorHelper::getGainCode(double gain) const\n> +{\n> +\t/* \\todo we only support the global gain mode for now */\n> +\tif (analogueGain_.type != GlobalGain)\n> +\t\treturn UINT32_MAX;\n> +\n> +\tif (analogueGain_.m0 == 0)\n> +\t\treturn (analogueGain_.c0 - gain * analogueGain_.c1) / (gain * analogueGain_.m1);\n> +\tif (analogueGain_.m1 == 0)\n> +\treturn (gain * analogueGain_.c1 - analogueGain_.c0) / analogueGain_.m0;\n\nYou can break the two lines\n\n> +\n> +\tLOG(CameraSensorHelper, Error) << \"For any given image sensor either m0 or m1 shall be zero.\";\n> +\treturn 1;\n\nAren't the two error conditions verified by assertions ? Otherwise we\nreturn two different error codes (MAXINT and 1) which are difficult to\nidentify.\n\n> +}\n> +\n> +/**\n> + * \\fn CameraSensorHelper::getGain\n> + * \\brief Get the real gain from the V4l2 subdev control gain\n> + * \\param[in] gainCode The V4l2 subdev control 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::getGainCode.\n> + *\n> + * The parameters come from the MIPI Alliance Camera Specification for\n> + * Camera Command Set (CCS).\n\nSame comments as above\n\n> + */\n> +double CameraSensorHelper::getGain(uint32_t gainCode) const\n> +{\n> +\tif (analogueGain_.type != GlobalGain)\n> +\t\treturn UINT32_MAX;\n> +\n> +\tif (analogueGain_.m0 == 0)\n> +\t\treturn analogueGain_.c0 / (analogueGain_.m1 * gainCode + analogueGain_.c1);\n> +\tif (analogueGain_.m1 == 0)\n> +\t\treturn (analogueGain_.m0 * gainCode + analogueGain_.c0) / analogueGain_.c1;\n> +\n> +\tLOG(CameraSensorHelper, Error) << \"For any given image sensor either m0 or m1 shall be zero.\";\n> +\treturn 1.0;\n> +}\n\nditto\n\nAlso, these smells like static functions as they only use class\nmembers constants for computations and do not hold any state. This\nhowever does not play well with the class hierarchy...\n\n> +\n> +/**\n> + * \\fn CameraSensorHelper::name()\n> + * \\brief Retrieve the camera sensor helper name\n> + * \\return The camera sensor helper name\n> + */\n> +\n> +/**\n> + * \\enum CameraSensorHelper::AnalogueGainCapability\n> + * \\brief Specify the Gain mode supported by the sensor\n> + *\n> + * Describes the image sensor analog Gain capabilities.\n> + * Two modes are possible, depending on the sensor: Global and Alternate.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::GlobalGain\n> + * \\brief Sensor supports Global gain\n> + *\n> + * The relationship between the integer Gain parameter and the resulting Gain\n> + * multiplier is given by the following equation:\n> + *\n> + * \\f$\\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 exposed by 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$\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AlternateGlobalGain\n> + * \\brief Sensor supports Analogue Global gain (introduced in CCS v1.1)\n> + *\n> + * Starting with CCS v1.1, Alternate Global Analog Gain is also available.\n> + * If the image sensor supports it, then the global analog 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\nHow do you envision this class to evolve ? I mean, it will go beyoind\njust analogue gain calculations ? As otherwise a single class with a\nmap of sensor specific gain constants would do...\n\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGainConstants::type\n> + * \\brief Analogue gain coding type\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGainConstants::m0\n> + * \\brief Constant used in the analog Gain control 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 analog Gain control coding/decoding.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGainConstants::m1\n> + * \\brief Constant used in the analog Gain control coding/decoding.\n\nNit: briefs do not end with '.'\n\n> + *\n> + * Note: either m0 or m1 shall be zero.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGainConstants::c1\n> + * \\brief Constant used in the analog Gain control coding/decoding.\n\nI presume I have to read the CCS specs to know what this parameters\nactually are :)\n\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGain_\n\nMaybe something in the name that makes it clear these are params ?\nAnalouge gain to me is what 'getGain()' calculates, am I mistaken ?\n\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\nEmpty line\n\n> + */\n> +\n\nI'll skip the factory review for now\n\n> +/**\n\n[snip]\n\n> +\n> +/**\n> + * \\fn CameraSensorHelperImx219::CameraSensorHelperImx219\n> + * \\brief Construct a CameraSensorHelperImx219 instance for imx219\n> + * \\param[in] name The sensor model name\n> + */\n> +class CameraSensorHelperImx219 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperImx219(const char *name);\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n\nI would add an empty line here and make it look like\n\nclass CameraSensorHelperXYZ : public CameraSensorHelper\n{\npublic:\n\tCameraSensorHelperXYZ(const char *name)\n {\n ...\n }\n};\nREGISTER_CAMERA_SENSOR_HELPER(\"XYZ\", CameraSensorHelperXYZ)\n\n\n> +CameraSensorHelperImx219::CameraSensorHelperImx219(const char *name)\n> +\t: CameraSensorHelper(name)\n> +{\n> +\tanalogueGain_ = { GlobalGain, 0, -1, 256, 256 };\n> +}\n> +\n> +/**\n> + * \\class CameraSensorHelperOv5670\n> + * \\brief Create and give helpers for the ov5670 sensor\n> + */\n> +\n> +/**\n> + * \\fn CameraSensorHelperOv5670::CameraSensorHelperOv5670\n> + * \\brief Construct a CameraSensorHelperOv5670 instance for ov5670\n> + * \\param[in] name The sensor model name\n> + */\n> +class CameraSensorHelperOv5670 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperOv5670(const char *name);\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n> +CameraSensorHelperOv5670::CameraSensorHelperOv5670(const char *name)\n> +\t: CameraSensorHelper(name)\n> +{\n> +\tanalogueGain_ = { GlobalGain, 1, 0, 0, 256 };\n> +}\n> +\n> +/**\n> + * \\class CameraSensorHelperOv5693\n> + * \\brief Create and give helpers for the ov5693 sensor\n> + */\n> +\n> +/**\n> + * \\fn CameraSensorHelperOv5693::CameraSensorHelperOv5693\n> + * \\brief Construct a CameraSensorHelperOv5693 instance for ov5693\n> + * \\param[in] name The sensor model name\n> + */\n> +class CameraSensorHelperOv5693 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperOv5693(const char *name);\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> +CameraSensorHelperOv5693::CameraSensorHelperOv5693(const char *name)\n> +\t: CameraSensorHelper(name)\n> +{\n> +\tanalogueGain_ = { GlobalGain, 1, 0, 0, 16 };\n> +}\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\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..63031902\n> --- /dev/null\n> +++ b/src/ipa/libipa/camera_sensor_helper.h\n> @@ -0,0 +1,91 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2021 Ideas On Board\n> + *\n> + * camera_sensor_helper.h - helper class providing camera informations\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> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class CameraSensorHelper;\n> +\n> +class CameraSensorHelper : public Object\n> +{\n> +public:\n> +\tCameraSensorHelper(const char *name);\n> +\tvirtual ~CameraSensorHelper();\n> +\n> +\tconst std::string &name() const { return name_; }\n> +\tuint32_t getGainCode(double gain) const;\n> +\tdouble getGain(uint32_t gainCode) const;\n> +\n> +protected:\n> +\tenum AnalogueGainCapability : uint16_t {\n> +\t\tGlobalGain = 0,\n> +\t\tAlternateGlobalGain = 2,\n\nAre the gain type values defined by the CCS specs ?\n\n> +\t};\n> +\n> +\tstruct analogueGainConstants {\n> +\t\tuint16_t type;\n> +\t\tint16_t m0;\n> +\t\tint16_t c0;\n> +\t\tint16_t m1;\n> +\t\tint16_t c1;\n> +\t};\n> +\tanalogueGainConstants analogueGain_;\n> +\n> +private:\n> +\tstd::string name_;\n> +\tfriend class CameraSensorHelperFactory;\n\nDo you need this friend class declaration ?\nThe constructor is public...\n\n> +};\n> +\n> +class CameraSensorHelperFactory\n> +{\n> +public:\n> +\tCameraSensorHelperFactory(const char *name);\n> +\tvirtual ~CameraSensorHelperFactory() = default;\n> +\n> +\tstd::unique_ptr create();\n> +\n> +\tconst std::string &name() const { return name_; }\n> +\n> +\tstatic void registerType(CameraSensorHelperFactory *factory);\n> +\tstatic std::vector &factories();\n> +\n> +private:\n> +\tvirtual CameraSensorHelper *createInstance() = 0;\n> +\n> +\tstd::string name_;\n> +};\n> +\n> +#define REGISTER_CAMERA_SENSOR_HELPER(name, handler) \\\n> +\tclass handler##Factory final : public CameraSensorHelperFactory \\\n> +\t{ \\\n> +\tpublic: \\\n> +\t\thandler##Factory() : CameraSensorHelperFactory(#handler) {} \\\n> + \\\n> +\tprivate: \\\n> +\t\tCameraSensorHelper *createInstance() \\\n> +\t\t{ \\\n> +\t\t\treturn new handler(name); \\\n> +\t\t} \\\n> +\t}; \\\n> +\tstatic handler##Factory global_##handler##Factory;\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n> +\n\nNo empty line at EOF otherwise git am complains (at least, that's how\nI interpret the error message :)\n\nThanks\n j\n\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 615E0C320B\n\tfor ;\n\tMon, 7 Jun 2021 16:04:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D805B68928;\n\tMon, 7 Jun 2021 18:04:45 +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 D599F602A7\n\tfor ;\n\tMon, 7 Jun 2021 18:04:44 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 19BDA1BF208;\n\tMon, 7 Jun 2021 16:04:43 +0000 (UTC)"],"Date":"Mon, 7 Jun 2021 18:05:33 +0200","From":"Jacopo Mondi ","To":"Jean-Michel Hautbois ","Message-ID":"<20210607160533.xogfthhdgmzxy5pm@uno.localdomain>","References":"<20210607123533.51198-1-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210607123533.51198-1-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 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":17446,"web_url":"https://patchwork.libcamera.org/comment/17446/","msgid":"","date":"2021-06-07T17:31:42","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: Create a camera sensor\n\thelper class","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jean-Michel,\n\n\n\nOn Mon, 7 Jun 2021 at 13:35, Jean-Michel Hautbois <\njeanmichel.hautbois@ideasonboard.com> wrote:\n\n> Setting analogue gain for a specific sensor is not a straghtforward\n> operation, as one needs to know how the gain is calculated for it.\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>\n\nI know this has probably been discussed in detail internally, but what\nprovisions\ndo you expect for gain formulas that do not conform to the CCI formula, or\nthose\nthat are entirely table based?\n\nThanks,\nNaush\n\n\n> Is makes it possible to only have 4 parameters to store per sensor, and\n> the gain calculation is then done identically for all of them. This\n> commit gives the gains for imx219 (based on RPi cam_helper), ov5670 and\n> ov5693.\n>\n> Adding a new sensor is pretty straightfoward 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 | 361 ++++++++++++++++++++++++\n> src/ipa/libipa/camera_sensor_helper.h | 91 ++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 454 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\n> b/src/ipa/libipa/camera_sensor_helper.cpp\n> new file mode 100644\n> index 00000000..2bd82861\n> --- /dev/null\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -0,0 +1,361 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2021 Ideas On Board\n> + *\n> + * camera_sensor_helper.cpp - helper class providing camera informations\n> + */\n> +#include \"camera_sensor_helper.h\"\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/log.h\"\n> +\n> +/**\n> + * \\file camera_sensor_helper.h\n> + * \\brief Create helper class for each sensor\n> + *\n> + * Each camera sensor supported by libcamera may need specific\n> informations to\n> + * be sent (analogue gain is sensor dependant for instance).\n> + *\n> + * Every subclass of CameraSensorHelper shall be registered with libipa\n> 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 Create and give helpers for each camera sensor in the pipeline\n> + *\n> + * CameraSensorHelper instances are unique and sensor dependant.\n> + */\n> +\n> +/**\n> + * \\brief Construct a CameraSensorHelper instance\n> + * \\param[in] name The sensor model name\n> + *\n> + * The CameraSensorHelper instances shall never be constructed manually,\n> but always\n> + * through the CameraSensorHelperFactory::create() method implemented by\n> the\n> + * respective factories.\n> + */\n> +\n> +CameraSensorHelper::CameraSensorHelper(const char *name)\n> + : name_(name)\n> +{\n> + analogueGain_ = { GlobalGain, 1, 1, 0, 0 };\n> +}\n> +\n> +CameraSensorHelper::~CameraSensorHelper()\n> +{\n> +}\n> +\n> +/**\n> + * \\fn CameraSensorHelper::getGainCode(double gain)\n> + * \\brief Get the analogue gain code to pass to V4L2 subdev control\n> + * \\param[in] gain The real gain to pass\n> + *\n> + * This function aims to abstract the calculation of the gain letting the\n> 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::getGainCode(double gain) const\n> +{\n> + /* \\todo we only support the global gain mode for now */\n> + if (analogueGain_.type != GlobalGain)\n> + return UINT32_MAX;\n> +\n> + if (analogueGain_.m0 == 0)\n> + return (analogueGain_.c0 - gain * analogueGain_.c1) /\n> (gain * analogueGain_.m1);\n> + if (analogueGain_.m1 == 0)\n> + return (gain * analogueGain_.c1 - analogueGain_.c0) /\n> analogueGain_.m0;\n> +\n> + LOG(CameraSensorHelper, Error) << \"For any given image sensor\n> either m0 or m1 shall be zero.\";\n> + return 1;\n> +}\n> +\n> +/**\n> + * \\fn CameraSensorHelper::getGain\n> + * \\brief Get the real gain from the V4l2 subdev control gain\n> + * \\param[in] gainCode The V4l2 subdev control gain\n> + *\n> + * This function aims to abstract the calculation of the gain letting the\n> IPA\n> + * use the real gain for its estimations. It is the counterpart of the\n> function\n> + * CameraSensorHelper::getGainCode.\n> + *\n> + * The parameters come from the MIPI Alliance Camera Specification for\n> + * Camera Command Set (CCS).\n> + */\n> +double CameraSensorHelper::getGain(uint32_t gainCode) const\n> +{\n> + if (analogueGain_.type != GlobalGain)\n> + return UINT32_MAX;\n> +\n> + if (analogueGain_.m0 == 0)\n> + return analogueGain_.c0 / (analogueGain_.m1 * gainCode +\n> analogueGain_.c1);\n> + if (analogueGain_.m1 == 0)\n> + return (analogueGain_.m0 * gainCode + analogueGain_.c0) /\n> analogueGain_.c1;\n> +\n> + LOG(CameraSensorHelper, Error) << \"For any given image sensor\n> either m0 or m1 shall be zero.\";\n> + return 1.0;\n> +}\n> +\n> +/**\n> + * \\fn CameraSensorHelper::name()\n> + * \\brief Retrieve the camera sensor helper name\n> + * \\return The camera sensor helper name\n> + */\n> +\n> +/**\n> + * \\enum CameraSensorHelper::AnalogueGainCapability\n> + * \\brief Specify the Gain mode supported by the sensor\n> + *\n> + * Describes the image sensor analog Gain capabilities.\n> + * Two modes are possible, depending on the sensor: Global and Alternate.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::GlobalGain\n> + * \\brief Sensor supports Global gain\n> + *\n> + * The relationship between the integer Gain parameter and the resulting\n> Gain\n> + * multiplier is given by the following equation:\n> + *\n> + * \\f$\\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 exposed by the sensor.\n> + * These constants are static parameters, and for any given image sensor\n> either\n> + * m0 or m1 shall be zero.\n> + *\n> + * The full Gain equation therefore reduces to either:\n> + *\n> + * \\f$\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AlternateGlobalGain\n> + * \\brief Sensor supports Analogue Global gain (introduced in CCS v1.1)\n> + *\n> + * Starting with CCS v1.1, Alternate Global Analog Gain is also available.\n> + * If the image sensor supports it, then the global analog Gain can be\n> 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 coding type\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGainConstants::m0\n> + * \\brief Constant used in the analog Gain control 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 analog Gain control coding/decoding.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGainConstants::m1\n> + * \\brief Constant used in the analog Gain control coding/decoding.\n> + *\n> + * Note: either m0 or m1 shall be zero.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGainConstants::c1\n> + * \\brief Constant used in the analog Gain control coding/decoding.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGain_\n> + * \\brief The analogue gain parameters used for calculation\n> + *\n> + * The analogue gain is calculated through a formula, and its parameters\n> are\n> + * sensor specific. Use this variable to store the values at init time.\n> + *\n> + */\n> +\n> +/**\n> + * \\class CameraSensorHelperFactory\n> + * \\brief Registration of CameraSensorHelperFactory classes and creation\n> of instances\n> + *\n> + * To facilitate discovery and instantiation of CameraSensorHelper\n> classes, the\n> + * CameraSensorHelperFactory class maintains a registry of camera sensor\n> helper\n> + * classes. Each CameraSensorHelper subclass shall register itself using\n> the\n> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a\n> corresponding\n> + * instance of a CameraSensorHelperFactory subclass and register it with\n> 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\n> 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> +\n> +CameraSensorHelperFactory::CameraSensorHelperFactory(const char *name)\n> + : name_(name)\n> +{\n> + registerType(this);\n> +}\n> +\n> +/**\n> + * \\brief Create an instance of the CameraSensorHelper corresponding to\n> the factory\n> + *\n> + * \\return A unique pointer to a new instance of the CameraSensorHelper\n> subclass\n> + * corresponding to the factory\n> + */\n> +std::unique_ptr CameraSensorHelperFactory::create()\n> +{\n> + CameraSensorHelper *handler = createInstance();\n> + return std::unique_ptr(handler);\n> +}\n> +\n> +/**\n> + * \\fn CameraSensorHelperFactory::name()\n> + * \\brief Retrieve the factory name\n> + * \\return The factory name\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\n> sensor helper\n> + * name.\n> + */\n> +void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory\n> *factory)\n> +{\n> + std::vector &factories =\n> CameraSensorHelperFactory::factories();\n> +\n> + factories.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\n> 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\n> &CameraSensorHelperFactory::factories()\n> +{\n> + static std::vector factories;\n> + return factories;\n> +}\n> +\n> +/**\n> + * \\fn CameraSensorHelperFactory::createInstance()\n> + * \\brief Create an instance of the CameraSensorHelper corresponding to\n> the factory\n> + *\n> + * This virtual function is implemented by the\n> REGISTER_CAMERA_SENSOR_HELPER()\n> + * macro. It creates a camera sensor helper instance associated with the\n> camera\n> + * sensor model.\n> + *\n> + * \\return A pointer to a newly constructed instance of the\n> 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\n> factory\n> + * \\param[in] name Sensor model name used to register the class\n> + * \\param[in] handler Class name of CameraSensorHelper derived class to\n> register\n> + *\n> + * Register a CameraSensorHelper subclass with the factory and make it\n> 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> +\n> +/**\n> + * \\fn CameraSensorHelperImx219::CameraSensorHelperImx219\n> + * \\brief Construct a CameraSensorHelperImx219 instance for imx219\n> + * \\param[in] name The sensor model name\n> + */\n> +class CameraSensorHelperImx219 : public CameraSensorHelper\n> +{\n> +public:\n> + CameraSensorHelperImx219(const char *name);\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> +CameraSensorHelperImx219::CameraSensorHelperImx219(const char *name)\n> + : CameraSensorHelper(name)\n> +{\n> + analogueGain_ = { GlobalGain, 0, -1, 256, 256 };\n> +}\n> +\n> +/**\n> + * \\class CameraSensorHelperOv5670\n> + * \\brief Create and give helpers for the ov5670 sensor\n> + */\n> +\n> +/**\n> + * \\fn CameraSensorHelperOv5670::CameraSensorHelperOv5670\n> + * \\brief Construct a CameraSensorHelperOv5670 instance for ov5670\n> + * \\param[in] name The sensor model name\n> + */\n> +class CameraSensorHelperOv5670 : public CameraSensorHelper\n> +{\n> +public:\n> + CameraSensorHelperOv5670(const char *name);\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n> +CameraSensorHelperOv5670::CameraSensorHelperOv5670(const char *name)\n> + : CameraSensorHelper(name)\n> +{\n> + analogueGain_ = { GlobalGain, 1, 0, 0, 256 };\n> +}\n> +\n> +/**\n> + * \\class CameraSensorHelperOv5693\n> + * \\brief Create and give helpers for the ov5693 sensor\n> + */\n> +\n> +/**\n> + * \\fn CameraSensorHelperOv5693::CameraSensorHelperOv5693\n> + * \\brief Construct a CameraSensorHelperOv5693 instance for ov5693\n> + * \\param[in] name The sensor model name\n> + */\n> +class CameraSensorHelperOv5693 : public CameraSensorHelper\n> +{\n> +public:\n> + CameraSensorHelperOv5693(const char *name);\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> +CameraSensorHelperOv5693::CameraSensorHelperOv5693(const char *name)\n> + : CameraSensorHelper(name)\n> +{\n> + analogueGain_ = { GlobalGain, 1, 0, 0, 16 };\n> +}\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\n> diff --git a/src/ipa/libipa/camera_sensor_helper.h\n> b/src/ipa/libipa/camera_sensor_helper.h\n> new file mode 100644\n> index 00000000..63031902\n> --- /dev/null\n> +++ b/src/ipa/libipa/camera_sensor_helper.h\n> @@ -0,0 +1,91 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2021 Ideas On Board\n> + *\n> + * camera_sensor_helper.h - helper class providing camera informations\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> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class CameraSensorHelper;\n> +\n> +class CameraSensorHelper : public Object\n> +{\n> +public:\n> + CameraSensorHelper(const char *name);\n> + virtual ~CameraSensorHelper();\n> +\n> + const std::string &name() const { return name_; }\n> + uint32_t getGainCode(double gain) const;\n> + double getGain(uint32_t gainCode) const;\n> +\n> +protected:\n> + enum AnalogueGainCapability : uint16_t {\n> + GlobalGain = 0,\n> + AlternateGlobalGain = 2,\n> + };\n> +\n> + struct analogueGainConstants {\n> + uint16_t type;\n> + int16_t m0;\n> + int16_t c0;\n> + int16_t m1;\n> + int16_t c1;\n> + };\n> + analogueGainConstants analogueGain_;\n> +\n> +private:\n> + std::string name_;\n> + friend class CameraSensorHelperFactory;\n> +};\n> +\n> +class CameraSensorHelperFactory\n> +{\n> +public:\n> + CameraSensorHelperFactory(const char *name);\n> + virtual ~CameraSensorHelperFactory() = default;\n> +\n> + std::unique_ptr create();\n> +\n> + const std::string &name() const { return name_; }\n> +\n> + static void registerType(CameraSensorHelperFactory *factory);\n> + static std::vector &factories();\n> +\n> +private:\n> + virtual CameraSensorHelper *createInstance() = 0;\n> +\n> + std::string name_;\n> +};\n> +\n> +#define REGISTER_CAMERA_SENSOR_HELPER(name, handler)\n> \\\n> + class handler##Factory final : public CameraSensorHelperFactory\n> \\\n> + {\n> \\\n> + public:\n> \\\n> + handler##Factory() : CameraSensorHelperFactory(#handler)\n> {} \\\n> +\n> \\\n> + private:\n> \\\n> + CameraSensorHelper *createInstance()\n> \\\n> + {\n> \\\n> + return new handler(name);\n> \\\n> + }\n> \\\n> + };\n> \\\n> + static handler##Factory global_##handler##Factory;\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n> +\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>\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 51FB0C3206\n\tfor ;\n\tMon, 7 Jun 2021 17:31:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BDBA56892E;\n\tMon, 7 Jun 2021 19:31:58 +0200 (CEST)","from mail-lj1-x22f.google.com (mail-lj1-x22f.google.com\n\t[IPv6:2a00:1450:4864:20::22f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CF119602A7\n\tfor ;\n\tMon, 7 Jun 2021 19:31:56 +0200 (CEST)","by mail-lj1-x22f.google.com with SMTP id r14so4136665ljd.10\n\tfor ;\n\tMon, 07 Jun 2021 10:31:56 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"G8n/e0pJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=5z98AqJ0A6/qlxE/ioPO5CqakwVoXSw2h2nlscUab84=;\n\tb=G8n/e0pJBoBWAYLFvTqXbGO//MRz7L+R7rimLwOqc8H8NrLPKEnwAf8SJjZoOsD0PT\n\tBohE9m9ev+ci9Ta4gYI2xc0Lie28sJeSir4jKTD50xB8oLWLMJieoGOpD4rPDkeE9LJ6\n\t9tcfIGV8VvgfFWI5MRNrJM39LIDe+CvBkMACY4LRrmOQYWX2WF+cv62XnftzmTD+xSIj\n\tIG7mfg/CJEQiGQHXiUPZ4kjB7jlebw4v2vROQ4595Sf2nA84G5GECTMw02jcwT9TCvJN\n\tczkjxG7ICu2BSkh8P6WQcDnOZbz/ATr5R4VGZ1HSFq0ZNVAWcKbzXoysn/1u65Tt1LlJ\n\t3Cpg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=5z98AqJ0A6/qlxE/ioPO5CqakwVoXSw2h2nlscUab84=;\n\tb=DWSn5bN0mTDGqvvDh1T2WaYetfnw+ZCMceeeu0G3NrrnSzKf5tD1a/kgb9yUUdvlhq\n\tbP3K9WbP79C6O8Pyfgjzj2ZdmSz5ZuaFCZLnuyBSJ+RuMl1FISxrTtFjj2VTfP72uDo/\n\tcX3Kx9wZn/GxcpH66KO5byETQvuQnGm3mdK/+MARM0jow+McPz93Bt92OFTxleiVVGDV\n\tpB92cN3FogQY+s1+E2McNJ9MWqo43jniDGzKJ0D40OUMGhGvj2XWpEXH76sL7R4i8s+8\n\tVsLDJUB6rfimxlx0shp0Q26f4A5noG18i+or3mS0F5GY++f8+IcD1/lQGG9GiAzKeMHF\n\tUMuA==","X-Gm-Message-State":"AOAM532bmEUPpMXfxVUAkyNZLIqEkTrhpzGMcq+hK+B9hmgmpHINkIYD\n\tTv3a1PNFkssEwaZx87YgptrZVgt+aLcR/XhRAezXjfERw7M=","X-Google-Smtp-Source":"ABdhPJxMDjr2B7b1HHzS8aGszroQdpCWNwoGISyvlH3osvfwkDTSdfGYPfrpOe30fcUCdkn9PAQrHpqreSoLNqKFHPc=","X-Received":"by 2002:a2e:90ca:: with SMTP id\n\to10mr10863482ljg.299.1623087116105; \n\tMon, 07 Jun 2021 10:31:56 -0700 (PDT)","MIME-Version":"1.0","References":"<20210607123533.51198-1-jeanmichel.hautbois@ideasonboard.com>","In-Reply-To":"<20210607123533.51198-1-jeanmichel.hautbois@ideasonboard.com>","From":"Naushir Patuck ","Date":"Mon, 7 Jun 2021 18:31:42 +0100","Message-ID":"","To":"Jean-Michel Hautbois ","Content-Type":"multipart/alternative; boundary=\"000000000000fec9b205c4306a31\"","Subject":"Re: [libcamera-devel] [PATCH 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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17447,"web_url":"https://patchwork.libcamera.org/comment/17447/","msgid":"<46fb8e19-1c8e-e23e-6936-1f122781a6d6@ideasonboard.com>","date":"2021-06-07T18:15:31","subject":"Re: [libcamera-devel] [PATCH 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 Naushir,\n\nOn 07/06/2021 19:31, Naushir Patuck wrote:\n> Hi Jean-Michel,\n> \n> \n> \n> On Mon, 7 Jun 2021 at 13:35, Jean-Michel Hautbois\n> > wrote:\n> \n> Setting analogue gain for a specific sensor is not a straghtforward\n> operation, as one needs to know how the gain is calculated for it.\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> \n> \n> I know this has probably been discussed in detail internally, but what\n> provisions\n> do you expect for gain formulas that do not conform to the CCI formula,\n> or  those\n> that are entirely table based?\n\nIt has not been discussed in detail no, and I am sure I have missed a\nlot of cases.\nFor those specific cases, we may, for instance, have a new value in\nAnalogueGainCapability enum and let the subclass calculate the proper\nvalue ?\nI don't have this case right now in my hands, so it is difficult to know\nprecisely what is the best way to do so ;-).\n\n> Thanks,\n> Naush\n> \n> \n> Is makes it possible to only have 4 parameters to store per sensor, and\n> the gain calculation is then done identically for all of them. This\n> commit gives the gains for imx219 (based on RPi cam_helper), ov5670 and\n> ov5693.\n> \n> Adding a new sensor is pretty straightfoward 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> ---\n>  src/ipa/libipa/camera_sensor_helper.cpp | 361 ++++++++++++++++++++++++\n>  src/ipa/libipa/camera_sensor_helper.h   |  91 ++++++\n>  src/ipa/libipa/meson.build              |   2 +\n>  3 files changed, 454 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\n> b/src/ipa/libipa/camera_sensor_helper.cpp\n> new file mode 100644\n> index 00000000..2bd82861\n> --- /dev/null\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -0,0 +1,361 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2021 Ideas On Board\n> + *\n> + * camera_sensor_helper.cpp - helper class providing camera\n> informations\n> + */\n> +#include \"camera_sensor_helper.h\"\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/log.h\"\n> +\n> +/**\n> + * \\file camera_sensor_helper.h\n> + * \\brief Create helper class for each sensor\n> + *\n> + * Each camera sensor supported by libcamera may need specific\n> informations to\n> + * be sent (analogue gain is sensor dependant for instance).\n> + *\n> + * Every subclass of CameraSensorHelper shall be registered with\n> 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 Create and give helpers for each camera sensor in the\n> pipeline\n> + *\n> + * CameraSensorHelper instances are unique and sensor dependant.\n> + */\n> +\n> +/**\n> + * \\brief Construct a CameraSensorHelper instance\n> + * \\param[in] name The sensor model name\n> + *\n> + * The CameraSensorHelper instances shall never be constructed\n> manually, but always\n> + * through the CameraSensorHelperFactory::create() method\n> implemented by the\n> + * respective factories.\n> + */\n> +\n> +CameraSensorHelper::CameraSensorHelper(const char *name)\n> +       : name_(name)\n> +{\n> +       analogueGain_ = { GlobalGain, 1, 1, 0, 0 };\n> +}\n> +\n> +CameraSensorHelper::~CameraSensorHelper()\n> +{\n> +}\n> +\n> +/**\n> + * \\fn CameraSensorHelper::getGainCode(double gain)\n> + * \\brief Get the analogue gain code to pass to V4L2 subdev control\n> + * \\param[in] gain The real gain to pass\n> + *\n> + * This function aims to abstract the calculation of the gain\n> 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::getGainCode(double gain) const\n> +{\n> +       /* \\todo we only support the global gain mode for now */\n> +       if (analogueGain_.type != GlobalGain)\n> +               return UINT32_MAX;\n> +\n> +       if (analogueGain_.m0 == 0)\n> +               return (analogueGain_.c0 - gain * analogueGain_.c1)\n> / (gain * analogueGain_.m1);\n> +       if (analogueGain_.m1 == 0)\n> +               return (gain * analogueGain_.c1 - analogueGain_.c0)\n> / analogueGain_.m0;\n> +\n> +       LOG(CameraSensorHelper, Error) << \"For any given image\n> sensor either m0 or m1 shall be zero.\";\n> +       return 1;\n> +}\n> +\n> +/**\n> + * \\fn CameraSensorHelper::getGain\n> + * \\brief Get the real gain from the V4l2 subdev control gain\n> + * \\param[in] gainCode The V4l2 subdev control gain\n> + *\n> + * This function aims to abstract the calculation of the gain\n> letting the IPA\n> + * use the real gain for its estimations. It is the counterpart of\n> the function\n> + * CameraSensorHelper::getGainCode.\n> + *\n> + * The parameters come from the MIPI Alliance Camera Specification for\n> + * Camera Command Set (CCS).\n> + */\n> +double CameraSensorHelper::getGain(uint32_t gainCode) const\n> +{\n> +       if (analogueGain_.type != GlobalGain)\n> +               return UINT32_MAX;\n> +\n> +       if (analogueGain_.m0 == 0)\n> +               return analogueGain_.c0 / (analogueGain_.m1 *\n> gainCode + analogueGain_.c1);\n> +       if (analogueGain_.m1 == 0)\n> +               return (analogueGain_.m0 * gainCode +\n> analogueGain_.c0) / analogueGain_.c1;\n> +\n> +       LOG(CameraSensorHelper, Error) << \"For any given image\n> sensor either m0 or m1 shall be zero.\";\n> +       return 1.0;\n> +}\n> +\n> +/**\n> + * \\fn CameraSensorHelper::name()\n> + * \\brief Retrieve the camera sensor helper name\n> + * \\return The camera sensor helper name\n> + */\n> +\n> +/**\n> + * \\enum CameraSensorHelper::AnalogueGainCapability\n> + * \\brief Specify the Gain mode supported by the sensor\n> + *\n> + * Describes the image sensor analog Gain capabilities.\n> + * Two modes are possible, depending on the sensor: Global and\n> Alternate.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::GlobalGain\n> + * \\brief Sensor supports Global gain\n> + *\n> + * The relationship between the integer Gain parameter and the\n> resulting Gain\n> + * multiplier is given by the following equation:\n> + *\n> + * \\f$\\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 exposed by the sensor.\n> + * These constants are static parameters, and for any given image\n> sensor either\n> + * m0 or m1 shall be zero.\n> + *\n> + * The full Gain equation therefore reduces to either:\n> + *\n> + * \\f$\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AlternateGlobalGain\n> + * \\brief Sensor supports Analogue Global gain (introduced in CCS v1.1)\n> + *\n> + * Starting with CCS v1.1, Alternate Global Analog Gain is also\n> available.\n> + * If the image sensor supports it, then the global analog Gain can\n> be controlled\n> + * by linear and exponential gain formula:\n> + *\n> + * \\f$gain = analogLinearGainGlobal *\n> 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 coding type\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGainConstants::m0\n> + * \\brief Constant used in the analog Gain control 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 analog Gain control coding/decoding.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGainConstants::m1\n> + * \\brief Constant used in the analog Gain control coding/decoding.\n> + *\n> + * Note: either m0 or m1 shall be zero.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGainConstants::c1\n> + * \\brief Constant used in the analog Gain control coding/decoding.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGain_\n> + * \\brief The analogue gain parameters used for calculation\n> + *\n> + * The analogue gain is calculated through a formula, and its\n> parameters are\n> + * sensor specific. Use this variable to store the values at init time.\n> + *\n> + */\n> +\n> +/**\n> + * \\class CameraSensorHelperFactory\n> + * \\brief Registration of CameraSensorHelperFactory classes and\n> creation of instances\n> + *\n> + * To facilitate discovery and instantiation of CameraSensorHelper\n> classes, the\n> + * CameraSensorHelperFactory class maintains a registry of camera\n> sensor helper\n> + * classes. Each CameraSensorHelper subclass shall register itself\n> using the\n> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a\n> corresponding\n> + * instance of a CameraSensorHelperFactory subclass and register it\n> 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\n> 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> +\n> +CameraSensorHelperFactory::CameraSensorHelperFactory(const char *name)\n> +       : name_(name)\n> +{\n> +       registerType(this);\n> +}\n> +\n> +/**\n> + * \\brief Create an instance of the CameraSensorHelper\n> corresponding to the factory\n> + *\n> + * \\return A unique pointer to a new instance of the\n> CameraSensorHelper subclass\n> + * corresponding to the factory\n> + */\n> +std::unique_ptr CameraSensorHelperFactory::create()\n> +{\n> +       CameraSensorHelper *handler = createInstance();\n> +       return std::unique_ptr(handler);\n> +}\n> +\n> +/**\n> + * \\fn CameraSensorHelperFactory::name()\n> + * \\brief Retrieve the factory name\n> + * \\return The factory name\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\n> helper\n> + *\n> + * The caller is responsible to guarantee the uniqueness of the\n> camera sensor helper\n> + * name.\n> + */\n> +void\n> CameraSensorHelperFactory::registerType(CameraSensorHelperFactory\n> *factory)\n> +{\n> +       std::vector &factories =\n> CameraSensorHelperFactory::factories();\n> +\n> +       factories.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\n> 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\n> &CameraSensorHelperFactory::factories()\n> +{\n> +       static std::vector factories;\n> +       return factories;\n> +}\n> +\n> +/**\n> + * \\fn CameraSensorHelperFactory::createInstance()\n> + * \\brief Create an instance of the CameraSensorHelper\n> corresponding to the factory\n> + *\n> + * This virtual function is implemented by the\n> REGISTER_CAMERA_SENSOR_HELPER()\n> + * macro. It creates a camera sensor helper instance associated\n> with the camera\n> + * sensor model.\n> + *\n> + * \\return A pointer to a newly constructed instance of the\n> 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\n> helper factory\n> + * \\param[in] name Sensor model name used to register the class\n> + * \\param[in] handler Class name of CameraSensorHelper derived\n> class to register\n> + *\n> + * Register a CameraSensorHelper subclass with the factory and make\n> 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> +\n> +/**\n> + * \\fn CameraSensorHelperImx219::CameraSensorHelperImx219\n> + * \\brief Construct a CameraSensorHelperImx219 instance for imx219\n> + * \\param[in] name The sensor model name\n> + */\n> +class CameraSensorHelperImx219 : public CameraSensorHelper\n> +{\n> +public:\n> +       CameraSensorHelperImx219(const char *name);\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> +CameraSensorHelperImx219::CameraSensorHelperImx219(const char *name)\n> +       : CameraSensorHelper(name)\n> +{\n> +       analogueGain_ = { GlobalGain, 0, -1, 256, 256 };\n> +}\n> +\n> +/**\n> + * \\class CameraSensorHelperOv5670\n> + * \\brief Create and give helpers for the ov5670 sensor\n> + */\n> +\n> +/**\n> + * \\fn CameraSensorHelperOv5670::CameraSensorHelperOv5670\n> + * \\brief Construct a CameraSensorHelperOv5670 instance for ov5670\n> + * \\param[in] name The sensor model name\n> + */\n> +class CameraSensorHelperOv5670 : public CameraSensorHelper\n> +{\n> +public:\n> +       CameraSensorHelperOv5670(const char *name);\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n> +CameraSensorHelperOv5670::CameraSensorHelperOv5670(const char *name)\n> +       : CameraSensorHelper(name)\n> +{\n> +       analogueGain_ = { GlobalGain, 1, 0, 0, 256 };\n> +}\n> +\n> +/**\n> + * \\class CameraSensorHelperOv5693\n> + * \\brief Create and give helpers for the ov5693 sensor\n> + */\n> +\n> +/**\n> + * \\fn CameraSensorHelperOv5693::CameraSensorHelperOv5693\n> + * \\brief Construct a CameraSensorHelperOv5693 instance for ov5693\n> + * \\param[in] name The sensor model name\n> + */\n> +class CameraSensorHelperOv5693 : public CameraSensorHelper\n> +{\n> +public:\n> +       CameraSensorHelperOv5693(const char *name);\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> +CameraSensorHelperOv5693::CameraSensorHelperOv5693(const char *name)\n> +       : CameraSensorHelper(name)\n> +{\n> +       analogueGain_ = { GlobalGain, 1, 0, 0, 16 };\n> +}\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\n> diff --git a/src/ipa/libipa/camera_sensor_helper.h\n> b/src/ipa/libipa/camera_sensor_helper.h\n> new file mode 100644\n> index 00000000..63031902\n> --- /dev/null\n> +++ b/src/ipa/libipa/camera_sensor_helper.h\n> @@ -0,0 +1,91 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2021 Ideas On Board\n> + *\n> + * camera_sensor_helper.h - helper class providing camera informations\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> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class CameraSensorHelper;\n> +\n> +class CameraSensorHelper : public Object\n> +{\n> +public:\n> +       CameraSensorHelper(const char *name);\n> +       virtual ~CameraSensorHelper();\n> +\n> +       const std::string &name() const { return name_; }\n> +       uint32_t getGainCode(double gain) const;\n> +       double getGain(uint32_t gainCode) const;\n> +\n> +protected:\n> +       enum AnalogueGainCapability : uint16_t {\n> +               GlobalGain = 0,\n> +               AlternateGlobalGain = 2,\n> +       };\n> +\n> +       struct analogueGainConstants {\n> +               uint16_t type;\n> +               int16_t m0;\n> +               int16_t c0;\n> +               int16_t m1;\n> +               int16_t c1;\n> +       };\n> +       analogueGainConstants analogueGain_;\n> +\n> +private:\n> +       std::string name_;\n> +       friend class CameraSensorHelperFactory;\n> +};\n> +\n> +class CameraSensorHelperFactory\n> +{\n> +public:\n> +       CameraSensorHelperFactory(const char *name);\n> +       virtual ~CameraSensorHelperFactory() = default;\n> +\n> +       std::unique_ptr create();\n> +\n> +       const std::string &name() const { return name_; }\n> +\n> +       static void registerType(CameraSensorHelperFactory *factory);\n> +       static std::vector &factories();\n> +\n> +private:\n> +       virtual CameraSensorHelper *createInstance() = 0;\n> +\n> +       std::string name_;\n> +};\n> +\n> +#define REGISTER_CAMERA_SENSOR_HELPER(name, handler)               \n>         \\\n> +       class handler##Factory final : public\n> CameraSensorHelperFactory     \\\n> +       {                                                           \n>        \\\n> +       public:                                                     \n>        \\\n> +               handler##Factory() :\n> CameraSensorHelperFactory(#handler) {} \\\n> +                                                                   \n>         \\\n> +       private:                                                   \n>         \\\n> +               CameraSensorHelper *createInstance()               \n>         \\\n> +               {                                                   \n>        \\\n> +                       return new handler(name);                   \n>        \\\n> +               }                                                   \n>        \\\n> +       };                                                         \n>         \\\n> +       static handler##Factory global_##handler##Factory;\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n> +\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 4B30EC320B\n\tfor ;\n\tMon, 7 Jun 2021 18:15:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 90F086891F;\n\tMon, 7 Jun 2021 20:15:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4F0EB602A7\n\tfor ;\n\tMon, 7 Jun 2021 20:15:32 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:b035:2cc2:4506:d7d3])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CF02C8DB;\n\tMon, 7 Jun 2021 20:15:31 +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=\"SFoGRNZA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623089731;\n\tbh=yaue7+P/mW/4SzylMk8Sk7yviPQA1DPGK0pAPmhG4Zw=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=SFoGRNZATMt/SI1QovYsz1Ftr1Oyks89pIAWD5WBfUU7NewakFdGbcQ3RWLPuWYxH\n\tCyvav/VFyoNAM4RRhfLQKSgISuQ+k3ibir9jllYIcfi97AQNQYJddQ6PPvT1n7oC5U\n\t6MM6/ISjIh0WcmGqr8v11JjZlBk/gD7S46+TByvk=","To":"Naushir Patuck ,\n\tJean-Michel Hautbois ","References":"<20210607123533.51198-1-jeanmichel.hautbois@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","Message-ID":"<46fb8e19-1c8e-e23e-6936-1f122781a6d6@ideasonboard.com>","Date":"Mon, 7 Jun 2021 20:15:31 +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":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17448,"web_url":"https://patchwork.libcamera.org/comment/17448/","msgid":"<130325f4-b459-40ab-0d7e-ef50eb9e5eb1@ideasonboard.com>","date":"2021-06-07T19:35:52","subject":"Re: [libcamera-devel] [PATCH 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":"Naushir,\n\nOn 07/06/2021 20:15, Jean-Michel Hautbois wrote:\n> Hi Naushir,\n> \n> On 07/06/2021 19:31, Naushir Patuck wrote:\n>> Hi Jean-Michel,\n>>\n>>\n>>\n>> On Mon, 7 Jun 2021 at 13:35, Jean-Michel Hautbois\n>> > > wrote:\n>>\n>> Setting analogue gain for a specific sensor is not a straghtforward\n>> operation, as one needs to know how the gain is calculated for it.\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>>\n>>\n>> I know this has probably been discussed in detail internally, but what\n>> provisions\n>> do you expect for gain formulas that do not conform to the CCI formula,\n>> or  those\n>> that are entirely table based?\n> \n> It has not been discussed in detail no, and I am sure I have missed a\n> lot of cases.\n> For those specific cases, we may, for instance, have a new value in\n> AnalogueGainCapability enum and let the subclass calculate the proper\n> value ?\n> I don't have this case right now in my hands, so it is difficult to know\n> precisely what is the best way to do so ;-).\n> \n>> Thanks,\n>> Naush\n>>\n>>\n>> Is makes it possible to only have 4 parameters to store per sensor, and\n>> the gain calculation is then done identically for all of them. This\n>> commit gives the gains for imx219 (based on RPi cam_helper), ov5670 and\n>> ov5693.\n>>\n>> Adding a new sensor is pretty straightfoward 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>> ---\n>>  src/ipa/libipa/camera_sensor_helper.cpp | 361 ++++++++++++++++++++++++\n>>  src/ipa/libipa/camera_sensor_helper.h   |  91 ++++++\n>>  src/ipa/libipa/meson.build              |   2 +\n>>  3 files changed, 454 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\n>> b/src/ipa/libipa/camera_sensor_helper.cpp\n>> new file mode 100644\n>> index 00000000..2bd82861\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n>> @@ -0,0 +1,361 @@\n>> +/* SPDX-License-Identifier: BSD-2-Clause */\n>> +/*\n>> + * Copyright (C) 2021 Ideas On Board\n>> + *\n>> + * camera_sensor_helper.cpp - helper class providing camera\n>> informations\n>> + */\n>> +#include \"camera_sensor_helper.h\"\n>> +\n>> +#include \n>> +\n>> +#include \"libcamera/internal/log.h\"\n>> +\n>> +/**\n>> + * \\file camera_sensor_helper.h\n>> + * \\brief Create helper class for each sensor\n>> + *\n>> + * Each camera sensor supported by libcamera may need specific\n>> informations to\n>> + * be sent (analogue gain is sensor dependant for instance).\n>> + *\n>> + * Every subclass of CameraSensorHelper shall be registered with\n>> 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 Create and give helpers for each camera sensor in the\n>> pipeline\n>> + *\n>> + * CameraSensorHelper instances are unique and sensor dependant.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct a CameraSensorHelper instance\n>> + * \\param[in] name The sensor model name\n>> + *\n>> + * The CameraSensorHelper instances shall never be constructed\n>> manually, but always\n>> + * through the CameraSensorHelperFactory::create() method\n>> implemented by the\n>> + * respective factories.\n>> + */\n>> +\n>> +CameraSensorHelper::CameraSensorHelper(const char *name)\n>> +       : name_(name)\n>> +{\n>> +       analogueGain_ = { GlobalGain, 1, 1, 0, 0 };\n>> +}\n>> +\n>> +CameraSensorHelper::~CameraSensorHelper()\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\fn CameraSensorHelper::getGainCode(double gain)\n>> + * \\brief Get the analogue gain code to pass to V4L2 subdev control\n>> + * \\param[in] gain The real gain to pass\n>> + *\n>> + * This function aims to abstract the calculation of the gain\n>> 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::getGainCode(double gain) const\n>> +{\n>> +       /* \\todo we only support the global gain mode for now */\n>> +       if (analogueGain_.type != GlobalGain)\n>> +               return UINT32_MAX;\n>> +\n>> +       if (analogueGain_.m0 == 0)\n>> +               return (analogueGain_.c0 - gain * analogueGain_.c1)\n>> / (gain * analogueGain_.m1);\n>> +       if (analogueGain_.m1 == 0)\n>> +               return (gain * analogueGain_.c1 - analogueGain_.c0)\n>> / analogueGain_.m0;\n>> +\n>> +       LOG(CameraSensorHelper, Error) << \"For any given image\n>> sensor either m0 or m1 shall be zero.\";\n>> +       return 1;\n>> +}\n>> +\n>> +/**\n>> + * \\fn CameraSensorHelper::getGain\n>> + * \\brief Get the real gain from the V4l2 subdev control gain\n>> + * \\param[in] gainCode The V4l2 subdev control gain\n>> + *\n>> + * This function aims to abstract the calculation of the gain\n>> letting the IPA\n>> + * use the real gain for its estimations. It is the counterpart of\n>> the function\n>> + * CameraSensorHelper::getGainCode.\n>> + *\n>> + * The parameters come from the MIPI Alliance Camera Specification for\n>> + * Camera Command Set (CCS).\n>> + */\n>> +double CameraSensorHelper::getGain(uint32_t gainCode) const\n>> +{\n>> +       if (analogueGain_.type != GlobalGain)\n>> +               return UINT32_MAX;\n>> +\n>> +       if (analogueGain_.m0 == 0)\n>> +               return analogueGain_.c0 / (analogueGain_.m1 *\n>> gainCode + analogueGain_.c1);\n>> +       if (analogueGain_.m1 == 0)\n>> +               return (analogueGain_.m0 * gainCode +\n>> analogueGain_.c0) / analogueGain_.c1;\n>> +\n>> +       LOG(CameraSensorHelper, Error) << \"For any given image\n>> sensor either m0 or m1 shall be zero.\";\n>> +       return 1.0;\n>> +}\n>> +\n>> +/**\n>> + * \\fn CameraSensorHelper::name()\n>> + * \\brief Retrieve the camera sensor helper name\n>> + * \\return The camera sensor helper name\n>> + */\n>> +\n>> +/**\n>> + * \\enum CameraSensorHelper::AnalogueGainCapability\n>> + * \\brief Specify the Gain mode supported by the sensor\n>> + *\n>> + * Describes the image sensor analog Gain capabilities.\n>> + * Two modes are possible, depending on the sensor: Global and\n>> Alternate.\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::GlobalGain\n>> + * \\brief Sensor supports Global gain\n>> + *\n>> + * The relationship between the integer Gain parameter and the\n>> resulting Gain\n>> + * multiplier is given by the following equation:\n>> + *\n>> + * \\f$\\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 exposed by the sensor.\n>> + * These constants are static parameters, and for any given image\n>> sensor either\n>> + * m0 or m1 shall be zero.\n>> + *\n>> + * The full Gain equation therefore reduces to either:\n>> + *\n>> + * \\f$\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AlternateGlobalGain\n>> + * \\brief Sensor supports Analogue Global gain (introduced in CCS v1.1)\n>> + *\n>> + * Starting with CCS v1.1, Alternate Global Analog Gain is also\n>> available.\n>> + * If the image sensor supports it, then the global analog Gain can\n>> be controlled\n>> + * by linear and exponential gain formula:\n>> + *\n>> + * \\f$gain = analogLinearGainGlobal *\n>> 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 coding type\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::analogueGainConstants::m0\n>> + * \\brief Constant used in the analog Gain control 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 analog Gain control coding/decoding.\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::analogueGainConstants::m1\n>> + * \\brief Constant used in the analog Gain control coding/decoding.\n>> + *\n>> + * Note: either m0 or m1 shall be zero.\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::analogueGainConstants::c1\n>> + * \\brief Constant used in the analog Gain control coding/decoding.\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::analogueGain_\n>> + * \\brief The analogue gain parameters used for calculation\n>> + *\n>> + * The analogue gain is calculated through a formula, and its\n>> parameters are\n>> + * sensor specific. Use this variable to store the values at init time.\n>> + *\n>> + */\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperFactory\n>> + * \\brief Registration of CameraSensorHelperFactory classes and\n>> creation of instances\n>> + *\n>> + * To facilitate discovery and instantiation of CameraSensorHelper\n>> classes, the\n>> + * CameraSensorHelperFactory class maintains a registry of camera\n>> sensor helper\n>> + * classes. Each CameraSensorHelper subclass shall register itself\n>> using the\n>> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a\n>> corresponding\n>> + * instance of a CameraSensorHelperFactory subclass and register it\n>> 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\n>> 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>> +\n>> +CameraSensorHelperFactory::CameraSensorHelperFactory(const char *name)\n>> +       : name_(name)\n>> +{\n>> +       registerType(this);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Create an instance of the CameraSensorHelper\n>> corresponding to the factory\n>> + *\n>> + * \\return A unique pointer to a new instance of the\n>> CameraSensorHelper subclass\n>> + * corresponding to the factory\n>> + */\n>> +std::unique_ptr CameraSensorHelperFactory::create()\n>> +{\n>> +       CameraSensorHelper *handler = createInstance();\n>> +       return std::unique_ptr(handler);\n>> +}\n>> +\n>> +/**\n>> + * \\fn CameraSensorHelperFactory::name()\n>> + * \\brief Retrieve the factory name\n>> + * \\return The factory name\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\n>> helper\n>> + *\n>> + * The caller is responsible to guarantee the uniqueness of the\n>> camera sensor helper\n>> + * name.\n>> + */\n>> +void\n>> CameraSensorHelperFactory::registerType(CameraSensorHelperFactory\n>> *factory)\n>> +{\n>> +       std::vector &factories =\n>> CameraSensorHelperFactory::factories();\n>> +\n>> +       factories.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\n>> 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\n>> &CameraSensorHelperFactory::factories()\n>> +{\n>> +       static std::vector factories;\n>> +       return factories;\n>> +}\n>> +\n>> +/**\n>> + * \\fn CameraSensorHelperFactory::createInstance()\n>> + * \\brief Create an instance of the CameraSensorHelper\n>> corresponding to the factory\n>> + *\n>> + * This virtual function is implemented by the\n>> REGISTER_CAMERA_SENSOR_HELPER()\n>> + * macro. It creates a camera sensor helper instance associated\n>> with the camera\n>> + * sensor model.\n>> + *\n>> + * \\return A pointer to a newly constructed instance of the\n>> 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\n>> helper factory\n>> + * \\param[in] name Sensor model name used to register the class\n>> + * \\param[in] handler Class name of CameraSensorHelper derived\n>> class to register\n>> + *\n>> + * Register a CameraSensorHelper subclass with the factory and make\n>> 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>> +\n>> +/**\n>> + * \\fn CameraSensorHelperImx219::CameraSensorHelperImx219\n>> + * \\brief Construct a CameraSensorHelperImx219 instance for imx219\n>> + * \\param[in] name The sensor model name\n>> + */\n>> +class CameraSensorHelperImx219 : public CameraSensorHelper\n>> +{\n>> +public:\n>> +       CameraSensorHelperImx219(const char *name);\n>> +};\n>> +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n>> +CameraSensorHelperImx219::CameraSensorHelperImx219(const char *name)\n>> +       : CameraSensorHelper(name)\n>> +{\n>> +       analogueGain_ = { GlobalGain, 0, -1, 256, 256 };\n>> +}\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperOv5670\n>> + * \\brief Create and give helpers for the ov5670 sensor\n>> + */\n>> +\n>> +/**\n>> + * \\fn CameraSensorHelperOv5670::CameraSensorHelperOv5670\n>> + * \\brief Construct a CameraSensorHelperOv5670 instance for ov5670\n>> + * \\param[in] name The sensor model name\n>> + */\n>> +class CameraSensorHelperOv5670 : public CameraSensorHelper\n>> +{\n>> +public:\n>> +       CameraSensorHelperOv5670(const char *name);\n>> +};\n>> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n>> +CameraSensorHelperOv5670::CameraSensorHelperOv5670(const char *name)\n>> +       : CameraSensorHelper(name)\n>> +{\n>> +       analogueGain_ = { GlobalGain, 1, 0, 0, 256 };\n>> +}\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperOv5693\n>> + * \\brief Create and give helpers for the ov5693 sensor\n>> + */\n>> +\n>> +/**\n>> + * \\fn CameraSensorHelperOv5693::CameraSensorHelperOv5693\n>> + * \\brief Construct a CameraSensorHelperOv5693 instance for ov5693\n>> + * \\param[in] name The sensor model name\n>> + */\n>> +class CameraSensorHelperOv5693 : public CameraSensorHelper\n>> +{\n>> +public:\n>> +       CameraSensorHelperOv5693(const char *name);\n>> +};\n>> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n>> +CameraSensorHelperOv5693::CameraSensorHelperOv5693(const char *name)\n>> +       : CameraSensorHelper(name)\n>> +{\n>> +       analogueGain_ = { GlobalGain, 1, 0, 0, 16 };\n>> +}\n>> +\n>> +} /* namespace ipa */\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> diff --git a/src/ipa/libipa/camera_sensor_helper.h\n>> b/src/ipa/libipa/camera_sensor_helper.h\n>> new file mode 100644\n>> index 00000000..63031902\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/camera_sensor_helper.h\n>> @@ -0,0 +1,91 @@\n>> +/* SPDX-License-Identifier: BSD-2-Clause */\n>> +/*\n>> + * Copyright (C) 2021 Ideas On Board\n>> + *\n>> + * camera_sensor_helper.h - helper class providing camera informations\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>> +\n>> +#include \n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa {\n>> +\n>> +class CameraSensorHelper;\n>> +\n>> +class CameraSensorHelper : public Object\n>> +{\n>> +public:\n>> +       CameraSensorHelper(const char *name);\n>> +       virtual ~CameraSensorHelper();\n>> +\n>> +       const std::string &name() const { return name_; }\n>> +       uint32_t getGainCode(double gain) const;\n\nWould you prefer getGainCode and getGain to be virtual, having a default\nmeyhod in CameraSensorHelper and letting those beeing overriden if a\nspecific sensor needs to have a different formula ?\n\n>> +       double getGain(uint32_t gainCode) const;\n>> +\n>> +protected:\n>> +       enum AnalogueGainCapability : uint16_t {\n>> +               GlobalGain = 0,\n>> +               AlternateGlobalGain = 2,\n>> +       };\n>> +\n>> +       struct analogueGainConstants {\n>> +               uint16_t type;\n>> +               int16_t m0;\n>> +               int16_t c0;\n>> +               int16_t m1;\n>> +               int16_t c1;\n>> +       };\n>> +       analogueGainConstants analogueGain_;\n>> +\n>> +private:\n>> +       std::string name_;\n>> +       friend class CameraSensorHelperFactory;\n>> +};\n>> +\n>> +class CameraSensorHelperFactory\n>> +{\n>> +public:\n>> +       CameraSensorHelperFactory(const char *name);\n>> +       virtual ~CameraSensorHelperFactory() = default;\n>> +\n>> +       std::unique_ptr create();\n>> +\n>> +       const std::string &name() const { return name_; }\n>> +\n>> +       static void registerType(CameraSensorHelperFactory *factory);\n>> +       static std::vector &factories();\n>> +\n>> +private:\n>> +       virtual CameraSensorHelper *createInstance() = 0;\n>> +\n>> +       std::string name_;\n>> +};\n>> +\n>> +#define REGISTER_CAMERA_SENSOR_HELPER(name, handler)               \n>>         \\\n>> +       class handler##Factory final : public\n>> CameraSensorHelperFactory     \\\n>> +       {                                                           \n>>        \\\n>> +       public:                                                     \n>>        \\\n>> +               handler##Factory() :\n>> CameraSensorHelperFactory(#handler) {} \\\n>> +                                                                   \n>>         \\\n>> +       private:                                                   \n>>         \\\n>> +               CameraSensorHelper *createInstance()               \n>>         \\\n>> +               {                                                   \n>>        \\\n>> +                       return new handler(name);                   \n>>        \\\n>> +               }                                                   \n>>        \\\n>> +       };                                                         \n>>         \\\n>> +       static handler##Factory global_##handler##Factory;\n>> +\n>> +} /* namespace ipa */\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n>> +\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 8DAFDC3206\n\tfor ;\n\tMon, 7 Jun 2021 19:35:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C92066891F;\n\tMon, 7 Jun 2021 21:35:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BCCB6602A7\n\tfor ;\n\tMon, 7 Jun 2021 21:35:53 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:b035:2cc2:4506:d7d3])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 450C08DB;\n\tMon, 7 Jun 2021 21:35:53 +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=\"Ud9N3qbV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623094553;\n\tbh=FC6BWDxZAbiRJ9SdHKFWVEhc8goRw7qT076oUcTPz6o=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=Ud9N3qbVL3ZpC9Ln6CAXNhco2//IRPMSDq+GOTkP9sUktQ/L6M8/caPI32HP5f1mB\n\tBKQX4k79G7iif9UPyCt35O7fgjHjlxeZKpbuC/z2KYffH5EndN2sQF3UOgIp/RF+cv\n\tAl2yN3EsC94p4CB9U1oQyMewViHPLvz5dd+XRWVU=","To":"Jean-Michel Hautbois ,\n\tNaushir Patuck ","References":"<20210607123533.51198-1-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<46fb8e19-1c8e-e23e-6936-1f122781a6d6@ideasonboard.com>","From":"Jean-Michel Hautbois ","Message-ID":"<130325f4-b459-40ab-0d7e-ef50eb9e5eb1@ideasonboard.com>","Date":"Mon, 7 Jun 2021 21:35:52 +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":"<46fb8e19-1c8e-e23e-6936-1f122781a6d6@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17472,"web_url":"https://patchwork.libcamera.org/comment/17472/","msgid":"","date":"2021-06-08T08:42:24","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: Create a camera sensor\n\thelper class","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jean-Michel,\n\n\nOn Mon, 7 Jun 2021 at 20:35, Jean-Michel Hautbois <\njeanmichel.hautbois@ideasonboard.com> wrote:\n\n> Naushir,\n>\n> On 07/06/2021 20:15, Jean-Michel Hautbois wrote:\n> > Hi Naushir,\n> >\n> > On 07/06/2021 19:31, Naushir Patuck wrote:\n> >> Hi Jean-Michel,\n> >>\n> >>\n> >>\n> >> On Mon, 7 Jun 2021 at 13:35, Jean-Michel Hautbois\n> >> >> > wrote:\n> >>\n> >> Setting analogue gain for a specific sensor is not a straghtforward\n> >> operation, as one needs to know how the gain is calculated for it.\n> >>\n> >> This commit introduces a new camera sensor helper in libipa which\n> aims\n> >> to solve this specific issue.\n> >> It is based on the MIPI alliance Specification for Camera Command\n> Set\n> >> and implements, for now, only the analogue \"Global gain\" mode.\n> >>\n> >>\n> >> I know this has probably been discussed in detail internally, but what\n> >> provisions\n> >> do you expect for gain formulas that do not conform to the CCI formula,\n> >> or those\n> >> that are entirely table based?\n> >\n> > It has not been discussed in detail no, and I am sure I have missed a\n> > lot of cases.\n> > For those specific cases, we may, for instance, have a new value in\n> > AnalogueGainCapability enum and let the subclass calculate the proper\n> > value ?\n> > I don't have this case right now in my hands, so it is difficult to know\n> > precisely what is the best way to do so ;-).\n> >\n> >> Thanks,\n> >> Naush\n> >>\n> >>\n> >> Is makes it possible to only have 4 parameters to store per sensor,\n> and\n> >> the gain calculation is then done identically for all of them. This\n> >> commit gives the gains for imx219 (based on RPi cam_helper), ov5670\n> and\n> >> ov5693.\n> >>\n> >> Adding a new sensor is pretty straightfoward 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> >> ---\n> >> src/ipa/libipa/camera_sensor_helper.cpp | 361\n> ++++++++++++++++++++++++\n> >> src/ipa/libipa/camera_sensor_helper.h | 91 ++++++\n> >> src/ipa/libipa/meson.build | 2 +\n> >> 3 files changed, 454 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\n> >> b/src/ipa/libipa/camera_sensor_helper.cpp\n> >> new file mode 100644\n> >> index 00000000..2bd82861\n> >> --- /dev/null\n> >> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> >> @@ -0,0 +1,361 @@\n> >> +/* SPDX-License-Identifier: BSD-2-Clause */\n> >> +/*\n> >> + * Copyright (C) 2021 Ideas On Board\n> >> + *\n> >> + * camera_sensor_helper.cpp - helper class providing camera\n> >> informations\n> >> + */\n> >> +#include \"camera_sensor_helper.h\"\n> >> +\n> >> +#include \n> >> +\n> >> +#include \"libcamera/internal/log.h\"\n> >> +\n> >> +/**\n> >> + * \\file camera_sensor_helper.h\n> >> + * \\brief Create helper class for each sensor\n> >> + *\n> >> + * Each camera sensor supported by libcamera may need specific\n> >> informations to\n> >> + * be sent (analogue gain is sensor dependant for instance).\n> >> + *\n> >> + * Every subclass of CameraSensorHelper shall be registered with\n> >> 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 Create and give helpers for each camera sensor in the\n> >> pipeline\n> >> + *\n> >> + * CameraSensorHelper instances are unique and sensor dependant.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Construct a CameraSensorHelper instance\n> >> + * \\param[in] name The sensor model name\n> >> + *\n> >> + * The CameraSensorHelper instances shall never be constructed\n> >> manually, but always\n> >> + * through the CameraSensorHelperFactory::create() method\n> >> implemented by the\n> >> + * respective factories.\n> >> + */\n> >> +\n> >> +CameraSensorHelper::CameraSensorHelper(const char *name)\n> >> + : name_(name)\n> >> +{\n> >> + analogueGain_ = { GlobalGain, 1, 1, 0, 0 };\n> >> +}\n> >> +\n> >> +CameraSensorHelper::~CameraSensorHelper()\n> >> +{\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\fn CameraSensorHelper::getGainCode(double gain)\n> >> + * \\brief Get the analogue gain code to pass to V4L2 subdev control\n> >> + * \\param[in] gain The real gain to pass\n> >> + *\n> >> + * This function aims to abstract the calculation of the gain\n> >> letting the IPA\n> >> + * use the real gain for its estimations.\n> >> + *\n> >> + * The parameters come from the MIPI Alliance Camera Specification\n> for\n> >> + * Camera Command Set (CCS).\n> >> + */\n> >> +uint32_t CameraSensorHelper::getGainCode(double gain) const\n> >> +{\n> >> + /* \\todo we only support the global gain mode for now */\n> >> + if (analogueGain_.type != GlobalGain)\n> >> + return UINT32_MAX;\n> >> +\n> >> + if (analogueGain_.m0 == 0)\n> >> + return (analogueGain_.c0 - gain * analogueGain_.c1)\n> >> / (gain * analogueGain_.m1);\n> >> + if (analogueGain_.m1 == 0)\n> >> + return (gain * analogueGain_.c1 - analogueGain_.c0)\n> >> / analogueGain_.m0;\n> >> +\n> >> + LOG(CameraSensorHelper, Error) << \"For any given image\n> >> sensor either m0 or m1 shall be zero.\";\n> >> + return 1;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\fn CameraSensorHelper::getGain\n> >> + * \\brief Get the real gain from the V4l2 subdev control gain\n> >> + * \\param[in] gainCode The V4l2 subdev control gain\n> >> + *\n> >> + * This function aims to abstract the calculation of the gain\n> >> letting the IPA\n> >> + * use the real gain for its estimations. It is the counterpart of\n> >> the function\n> >> + * CameraSensorHelper::getGainCode.\n> >> + *\n> >> + * The parameters come from the MIPI Alliance Camera Specification\n> for\n> >> + * Camera Command Set (CCS).\n> >> + */\n> >> +double CameraSensorHelper::getGain(uint32_t gainCode) const\n> >> +{\n> >> + if (analogueGain_.type != GlobalGain)\n> >> + return UINT32_MAX;\n> >> +\n> >> + if (analogueGain_.m0 == 0)\n> >> + return analogueGain_.c0 / (analogueGain_.m1 *\n> >> gainCode + analogueGain_.c1);\n> >> + if (analogueGain_.m1 == 0)\n> >> + return (analogueGain_.m0 * gainCode +\n> >> analogueGain_.c0) / analogueGain_.c1;\n> >> +\n> >> + LOG(CameraSensorHelper, Error) << \"For any given image\n> >> sensor either m0 or m1 shall be zero.\";\n> >> + return 1.0;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\fn CameraSensorHelper::name()\n> >> + * \\brief Retrieve the camera sensor helper name\n> >> + * \\return The camera sensor helper name\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\enum CameraSensorHelper::AnalogueGainCapability\n> >> + * \\brief Specify the Gain mode supported by the sensor\n> >> + *\n> >> + * Describes the image sensor analog Gain capabilities.\n> >> + * Two modes are possible, depending on the sensor: Global and\n> >> Alternate.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var CameraSensorHelper::GlobalGain\n> >> + * \\brief Sensor supports Global gain\n> >> + *\n> >> + * The relationship between the integer Gain parameter and the\n> >> resulting Gain\n> >> + * multiplier is given by the following equation:\n> >> + *\n> >> + * \\f$\\frac{m0x+c0}{m1x+c1}\\f$\n> >> + *\n> >> + * Where 'x' is the gain control parameter, and m0, m1, c0 and c1\n> are\n> >> + * image-sensor-specific constants exposed by the sensor.\n> >> + * These constants are static parameters, and for any given image\n> >> sensor either\n> >> + * m0 or m1 shall be zero.\n> >> + *\n> >> + * The full Gain equation therefore reduces to either:\n> >> + *\n> >> + * \\f$\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var CameraSensorHelper::AlternateGlobalGain\n> >> + * \\brief Sensor supports Analogue Global gain (introduced in CCS\n> v1.1)\n> >> + *\n> >> + * Starting with CCS v1.1, Alternate Global Analog Gain is also\n> >> available.\n> >> + * If the image sensor supports it, then the global analog Gain can\n> >> be controlled\n> >> + * by linear and exponential gain formula:\n> >> + *\n> >> + * \\f$gain = analogLinearGainGlobal *\n> >> 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 coding type\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var CameraSensorHelper::analogueGainConstants::m0\n> >> + * \\brief Constant used in the analog Gain control 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 analog Gain control coding/decoding.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var CameraSensorHelper::analogueGainConstants::m1\n> >> + * \\brief Constant used in the analog Gain control coding/decoding.\n> >> + *\n> >> + * Note: either m0 or m1 shall be zero.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var CameraSensorHelper::analogueGainConstants::c1\n> >> + * \\brief Constant used in the analog Gain control coding/decoding.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\var CameraSensorHelper::analogueGain_\n> >> + * \\brief The analogue gain parameters used for calculation\n> >> + *\n> >> + * The analogue gain is calculated through a formula, and its\n> >> parameters are\n> >> + * sensor specific. Use this variable to store the values at init\n> time.\n> >> + *\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\class CameraSensorHelperFactory\n> >> + * \\brief Registration of CameraSensorHelperFactory classes and\n> >> creation of instances\n> >> + *\n> >> + * To facilitate discovery and instantiation of CameraSensorHelper\n> >> classes, the\n> >> + * CameraSensorHelperFactory class maintains a registry of camera\n> >> sensor helper\n> >> + * classes. Each CameraSensorHelper subclass shall register itself\n> >> using the\n> >> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a\n> >> corresponding\n> >> + * instance of a CameraSensorHelperFactory subclass and register it\n> >> 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\n> >> list of\n> >> + * factories, accessible through the factories() function.\n> >> + *\n> >> + * The factory \\a name is used for debug purpose and shall be\n> unique.\n> >> + */\n> >> +\n> >> +CameraSensorHelperFactory::CameraSensorHelperFactory(const char\n> *name)\n> >> + : name_(name)\n> >> +{\n> >> + registerType(this);\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Create an instance of the CameraSensorHelper\n> >> corresponding to the factory\n> >> + *\n> >> + * \\return A unique pointer to a new instance of the\n> >> CameraSensorHelper subclass\n> >> + * corresponding to the factory\n> >> + */\n> >> +std::unique_ptr\n> CameraSensorHelperFactory::create()\n> >> +{\n> >> + CameraSensorHelper *handler = createInstance();\n> >> + return std::unique_ptr(handler);\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\fn CameraSensorHelperFactory::name()\n> >> + * \\brief Retrieve the factory name\n> >> + * \\return The factory name\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\n> >> helper\n> >> + *\n> >> + * The caller is responsible to guarantee the uniqueness of the\n> >> camera sensor helper\n> >> + * name.\n> >> + */\n> >> +void\n> >> CameraSensorHelperFactory::registerType(CameraSensorHelperFactory\n> >> *factory)\n> >> +{\n> >> + std::vector &factories =\n> >> CameraSensorHelperFactory::factories();\n> >> +\n> >> + factories.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\n> >> 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\n> >> &CameraSensorHelperFactory::factories()\n> >> +{\n> >> + static std::vector factories;\n> >> + return factories;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\fn CameraSensorHelperFactory::createInstance()\n> >> + * \\brief Create an instance of the CameraSensorHelper\n> >> corresponding to the factory\n> >> + *\n> >> + * This virtual function is implemented by the\n> >> REGISTER_CAMERA_SENSOR_HELPER()\n> >> + * macro. It creates a camera sensor helper instance associated\n> >> with the camera\n> >> + * sensor model.\n> >> + *\n> >> + * \\return A pointer to a newly constructed instance of the\n> >> 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\n> >> helper factory\n> >> + * \\param[in] name Sensor model name used to register the class\n> >> + * \\param[in] handler Class name of CameraSensorHelper derived\n> >> class to register\n> >> + *\n> >> + * Register a CameraSensorHelper subclass with the factory and make\n> >> 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> >> +\n> >> +/**\n> >> + * \\fn CameraSensorHelperImx219::CameraSensorHelperImx219\n> >> + * \\brief Construct a CameraSensorHelperImx219 instance for imx219\n> >> + * \\param[in] name The sensor model name\n> >> + */\n> >> +class CameraSensorHelperImx219 : public CameraSensorHelper\n> >> +{\n> >> +public:\n> >> + CameraSensorHelperImx219(const char *name);\n> >> +};\n> >> +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> >> +CameraSensorHelperImx219::CameraSensorHelperImx219(const char\n> *name)\n> >> + : CameraSensorHelper(name)\n> >> +{\n> >> + analogueGain_ = { GlobalGain, 0, -1, 256, 256 };\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\class CameraSensorHelperOv5670\n> >> + * \\brief Create and give helpers for the ov5670 sensor\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn CameraSensorHelperOv5670::CameraSensorHelperOv5670\n> >> + * \\brief Construct a CameraSensorHelperOv5670 instance for ov5670\n> >> + * \\param[in] name The sensor model name\n> >> + */\n> >> +class CameraSensorHelperOv5670 : public CameraSensorHelper\n> >> +{\n> >> +public:\n> >> + CameraSensorHelperOv5670(const char *name);\n> >> +};\n> >> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n> >> +CameraSensorHelperOv5670::CameraSensorHelperOv5670(const char\n> *name)\n> >> + : CameraSensorHelper(name)\n> >> +{\n> >> + analogueGain_ = { GlobalGain, 1, 0, 0, 256 };\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\class CameraSensorHelperOv5693\n> >> + * \\brief Create and give helpers for the ov5693 sensor\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn CameraSensorHelperOv5693::CameraSensorHelperOv5693\n> >> + * \\brief Construct a CameraSensorHelperOv5693 instance for ov5693\n> >> + * \\param[in] name The sensor model name\n> >> + */\n> >> +class CameraSensorHelperOv5693 : public CameraSensorHelper\n> >> +{\n> >> +public:\n> >> + CameraSensorHelperOv5693(const char *name);\n> >> +};\n> >> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> >> +CameraSensorHelperOv5693::CameraSensorHelperOv5693(const char\n> *name)\n> >> + : CameraSensorHelper(name)\n> >> +{\n> >> + analogueGain_ = { GlobalGain, 1, 0, 0, 16 };\n> >> +}\n> >> +\n> >> +} /* namespace ipa */\n> >> +\n> >> +} /* namespace libcamera */\n> >> +\n> >> diff --git a/src/ipa/libipa/camera_sensor_helper.h\n> >> b/src/ipa/libipa/camera_sensor_helper.h\n> >> new file mode 100644\n> >> index 00000000..63031902\n> >> --- /dev/null\n> >> +++ b/src/ipa/libipa/camera_sensor_helper.h\n> >> @@ -0,0 +1,91 @@\n> >> +/* SPDX-License-Identifier: BSD-2-Clause */\n> >> +/*\n> >> + * Copyright (C) 2021 Ideas On Board\n> >> + *\n> >> + * camera_sensor_helper.h - helper class providing camera\n> informations\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> >> +\n> >> +#include \n> >> +#include \n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +namespace ipa {\n> >> +\n> >> +class CameraSensorHelper;\n> >> +\n> >> +class CameraSensorHelper : public Object\n> >> +{\n> >> +public:\n> >> + CameraSensorHelper(const char *name);\n> >> + virtual ~CameraSensorHelper();\n> >> +\n> >> + const std::string &name() const { return name_; }\n> >> + uint32_t getGainCode(double gain) const;\n>\n> Would you prefer getGainCode and getGain to be virtual, having a default\n> meyhod in CameraSensorHelper and letting those beeing overriden if a\n> specific sensor needs to have a different formula ?\n>\n\nThe Raspberry Pi CamHelper does keep these functions as virtual. However,\nI know Laurent and others would have a preference of using static data where\npossible. Perhaps just something to consider for the future rather than\ndoing\nit now.\n\nRegards,\nNaush\n\n\n\n>\n> >> + double getGain(uint32_t gainCode) const;\n> >> +\n> >> +protected:\n> >> + enum AnalogueGainCapability : uint16_t {\n> >> + GlobalGain = 0,\n> >> + AlternateGlobalGain = 2,\n> >> + };\n> >> +\n> >> + struct analogueGainConstants {\n> >> + uint16_t type;\n> >> + int16_t m0;\n> >> + int16_t c0;\n> >> + int16_t m1;\n> >> + int16_t c1;\n> >> + };\n> >> + analogueGainConstants analogueGain_;\n> >> +\n> >> +private:\n> >> + std::string name_;\n> >> + friend class CameraSensorHelperFactory;\n> >> +};\n> >> +\n> >> +class CameraSensorHelperFactory\n> >> +{\n> >> +public:\n> >> + CameraSensorHelperFactory(const char *name);\n> >> + virtual ~CameraSensorHelperFactory() = default;\n> >> +\n> >> + std::unique_ptr create();\n> >> +\n> >> + const std::string &name() const { return name_; }\n> >> +\n> >> + static void registerType(CameraSensorHelperFactory\n> *factory);\n> >> + static std::vector\n> &factories();\n> >> +\n> >> +private:\n> >> + virtual CameraSensorHelper *createInstance() = 0;\n> >> +\n> >> + std::string name_;\n> >> +};\n> >> +\n> >> +#define REGISTER_CAMERA_SENSOR_HELPER(name, handler)\n> >> \\\n> >> + class handler##Factory final : public\n> >> CameraSensorHelperFactory \\\n> >> + {\n> >> \\\n> >> + public:\n> >> \\\n> >> + handler##Factory() :\n> >> CameraSensorHelperFactory(#handler) {} \\\n> >> +\n> >> \\\n> >> + private:\n> >> \\\n> >> + CameraSensorHelper *createInstance()\n> >> \\\n> >> + {\n> >> \\\n> >> + return new handler(name);\n> >> \\\n> >> + }\n> >> \\\n> >> + };\n> >> \\\n> >> + static handler##Factory global_##handler##Factory;\n> >> +\n> >> +} /* namespace ipa */\n> >> +\n> >> +} /* namespace libcamera */\n> >> +\n> >> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n> >> +\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> >>\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 C22C7C3206\n\tfor ;\n\tTue, 8 Jun 2021 08:42:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 064EA6892C;\n\tTue, 8 Jun 2021 10:42:44 +0200 (CEST)","from mail-lj1-x231.google.com (mail-lj1-x231.google.com\n\t[IPv6:2a00:1450:4864:20::231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9C7A168922\n\tfor ;\n\tTue, 8 Jun 2021 10:42:41 +0200 (CEST)","by mail-lj1-x231.google.com with SMTP id u18so5773801lju.12\n\tfor ;\n\tTue, 08 Jun 2021 01:42:41 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"kyG1FSXX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=qBtDDN4A+k6lF2yZsxGZJ6AwJW+yHDqoGhAQJChY9C4=;\n\tb=kyG1FSXX6a1U0GpEvYlwM0dIFoOSAqG1o5Fe1m/gOmYuhzg2WAsqt2f8DkwFkodO4L\n\tJx2UF8P9bPB8X41TGedSEJs8beK+1vNPlNheVsZCiuMxKFqPBJMM9QE8ZQOPJ73qEvSg\n\t+/nEX+U0idkYOYhx4r7i2yr5alJw1/kkKNXW4svza18U57DlbpnvyWn7XY3v3JDRW4N9\n\tlHgStsko+QBX4fmsRYrn2NonamC3yc5Ksb9MpO/gRD5iBNtpOBHt4nA3Y4ROC3BIqifd\n\t1t2pziHjxsExBjBgluQDdQk/xq9wumaDhYp4fU9hAR+z6Nkev7UTxD/0L2SOi77HswcW\n\tEaIw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=qBtDDN4A+k6lF2yZsxGZJ6AwJW+yHDqoGhAQJChY9C4=;\n\tb=iw4b8D8fuiV+TMEMmjKpDeFmxk1s1y3TiUrjMPAStLZKW0ZPRwvrtTQt9ihK6QJoVl\n\t3FdaltMgjau+SCzglfCV0tDg0jLRvXuiXIP++Z/lEhs6PewJtut36O3KDtK9hfdDGklX\n\tbNydSuibiA3mEFc4MLjBpXuzgRQ3oz59tBeKvp4cU1vJ78ZLzWy/LYwh6VuCQIa7Fs3S\n\t8wqvcFVuJH1vTw971D0uownRu2L0Y3HceM0wrfswh4Fc8dRa30esb3VZS1OOzUtUuMyr\n\ty/eDZ3zCk8SFd7RiPrwvROXFZkNtG1lRb0H5ElvIgi9zWGVsQa4mrKekENkxy102GGjT\n\t07kg==","X-Gm-Message-State":"AOAM532E3hAuqumMVrMYC9TsjW7QmgsKDZuVuiAW3fG5dsRwkgzE2oDm\n\t3/8mYjIKtfA1kR2mhWuGLrSTcaGik+aOQEamd24SQearabM=","X-Google-Smtp-Source":"ABdhPJyMEOqNWt/l3aE2zsVHz8je2yO88zFmzCtnVAz5/yI90VIG3XtK8a5B9l9GIiSx0PxTReERIkW395dydKNOLNI=","X-Received":"by 2002:a2e:a230:: with SMTP id\n\ti16mr13881215ljm.169.1623141760710; \n\tTue, 08 Jun 2021 01:42:40 -0700 (PDT)","MIME-Version":"1.0","References":"<20210607123533.51198-1-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<46fb8e19-1c8e-e23e-6936-1f122781a6d6@ideasonboard.com>\n\t<130325f4-b459-40ab-0d7e-ef50eb9e5eb1@ideasonboard.com>","In-Reply-To":"<130325f4-b459-40ab-0d7e-ef50eb9e5eb1@ideasonboard.com>","From":"Naushir Patuck ","Date":"Tue, 8 Jun 2021 09:42:24 +0100","Message-ID":"","To":"Jean-Michel Hautbois ","Content-Type":"multipart/alternative; boundary=\"000000000000114ddd05c43d2404\"","Subject":"Re: [libcamera-devel] [PATCH 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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17482,"web_url":"https://patchwork.libcamera.org/comment/17482/","msgid":"","date":"2021-06-08T15:46:40","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: Create a camera sensor\n\thelper class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nThank you for the patch.\n\nOn Mon, Jun 07, 2021 at 02:35:32PM +0200, Jean-Michel Hautbois wrote:\n> Setting analogue gain for a specific sensor is not a straghtforward\n> operation, as one needs to know how the gain is calculated for it.\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> \n> Is makes it possible to only have 4 parameters to store per sensor, and\n> the gain calculation is then done identically for all of them. This\n> commit gives the gains for imx219 (based on RPi cam_helper), ov5670 and\n> ov5693.\n> \n> Adding a new sensor is pretty straightfoward 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 | 361 ++++++++++++++++++++++++\n> src/ipa/libipa/camera_sensor_helper.h | 91 ++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 454 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..2bd82861\n> --- /dev/null\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -0,0 +1,361 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2021 Ideas On Board\n> + *\n> + * camera_sensor_helper.cpp - helper class providing camera informations\n> + */\n> +#include \"camera_sensor_helper.h\"\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/log.h\"\n> +\n> +/**\n> + * \\file camera_sensor_helper.h\n> + * \\brief Create helper class for each sensor\n> + *\n> + * Each camera sensor supported by libcamera may need specific informations to\n> + * be sent (analogue gain is sensor dependant for instance).\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 Create and give helpers for each camera sensor in the pipeline\n> + *\n> + * CameraSensorHelper instances are unique and sensor dependant.\n> + */\n> +\n> +/**\n> + * \\brief Construct a CameraSensorHelper instance\n> + * \\param[in] name The sensor model name\n> + *\n> + * The CameraSensorHelper instances shall never be constructed manually, but always\n> + * through the CameraSensorHelperFactory::create() method implemented by the\n> + * respective factories.\n\nThe respective factories implement createInstance(), not create(). I'd\ndrop \"by the respective factories\".\n\n> + */\n> +\n> +CameraSensorHelper::CameraSensorHelper(const char *name)\n> +\t: name_(name)\n> +{\n> +\tanalogueGain_ = { GlobalGain, 1, 1, 0, 0 };\n\nNo need for this, it's set by all the subclasses.\n\n> +}\n> +\n> +CameraSensorHelper::~CameraSensorHelper()\n> +{\n> +}\n> +\n> +/**\n> + * \\fn CameraSensorHelper::getGainCode(double gain)\n> + * \\brief Get the analogue gain code to pass to V4L2 subdev control\n> + * \\param[in] gain The real gain to pass\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::getGainCode(double gain) const\n> +{\n> +\t/* \\todo we only support the global gain mode for now */\n\nThen let's drop AlternateGlobalGain for now. Or we can address the todo\nitem already, it should be easy.\n\n> +\tif (analogueGain_.type != GlobalGain)\n> +\t\treturn UINT32_MAX;\n> +\n> +\tif (analogueGain_.m0 == 0)\n> +\t\treturn (analogueGain_.c0 - gain * analogueGain_.c1) / (gain * analogueGain_.m1);\n\n\t\treturn (analogueGain_.c0 - gain * analogueGain_.c1) /\n\t\t (gain * analogueGain_.m1);\n\n> +\tif (analogueGain_.m1 == 0)\n> +\t\treturn (gain * analogueGain_.c1 - analogueGain_.c0) / analogueGain_.m0;\n\nYou can also write\n\n\treturn (analogueGain_.c0 - analogueGain_.c1 * gain) /\n\t (analogueGain_.m1 * gain - analogueGain_.m0);\n\n> +\n> +\tLOG(CameraSensorHelper, Error) << \"For any given image sensor either m0 or m1 shall be zero.\";\n\nMake this an assert. I'm tempted to pass the analogueGainConstants\npointer to the CameraSensorHelper constructor to assert there.\n\n> +\treturn 1;\n> +}\n> +\n> +/**\n> + * \\fn CameraSensorHelper::getGain\n> + * \\brief Get the real gain from the V4l2 subdev control gain\n> + * \\param[in] gainCode The V4l2 subdev control 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::getGainCode.\n> + *\n> + * The parameters come from the MIPI Alliance Camera Specification for\n> + * Camera Command Set (CCS).\n> + */\n> +double CameraSensorHelper::getGain(uint32_t gainCode) const\n> +{\n> +\tif (analogueGain_.type != GlobalGain)\n> +\t\treturn UINT32_MAX;\n\nThat's not a double.\n\n> +\n> +\tif (analogueGain_.m0 == 0)\n> +\t\treturn analogueGain_.c0 / (analogueGain_.m1 * gainCode + analogueGain_.c1);\n> +\tif (analogueGain_.m1 == 0)\n> +\t\treturn (analogueGain_.m0 * gainCode + analogueGain_.c0) / analogueGain_.c1;\n\nYou can also write\n\n\treturn (analogueGain_.m0 * gainCode + analogueGain_.c0) /\n\t (analogueGain_.m1 * gainCode + analogueGain_.c1)\n\n> +\n> +\tLOG(CameraSensorHelper, Error) << \"For any given image sensor either m0 or m1 shall be zero.\";\n> +\treturn 1.0;\n> +}\n> +\n> +/**\n> + * \\fn CameraSensorHelper::name()\n> + * \\brief Retrieve the camera sensor helper name\n> + * \\return The camera sensor helper name\n> + */\n> +\n> +/**\n> + * \\enum CameraSensorHelper::AnalogueGainCapability\n\nThat's not a very descriptive name, I'd propose AnalogueGainType (or\nAnalogueGainModel).\n\n> + * \\brief Specify the Gain mode supported by the sensor\n> + *\n> + * Describes the image sensor analog Gain capabilities.\n> + * Two modes are possible, depending on the sensor: Global and Alternate.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::GlobalGain\n> + * \\brief Sensor supports Global gain\n\nThis is also not very descriptive, I'd propose AnalogueGainLinear (and\nAnalogueGainExponential for AlternateGlobalGain).\n\n> + *\n> + * The relationship between the integer Gain parameter and the resulting Gain\n> + * multiplier is given by the following equation:\n> + *\n> + * \\f$\\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 exposed by 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$\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n\n * \\f$fain = \\frac...\n\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AlternateGlobalGain\n> + * \\brief Sensor supports Analogue Global gain (introduced in CCS v1.1)\n> + *\n> + * Starting with CCS v1.1, Alternate Global Analog Gain is also available.\n> + * If the image sensor supports it, then the global analog 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 coding type\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGainConstants::m0\n> + * \\brief Constant used in the analog Gain control 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 analog Gain control coding/decoding.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGainConstants::m1\n> + * \\brief Constant used in the analog Gain control coding/decoding.\n> + *\n> + * Note: either m0 or m1 shall be zero.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGainConstants::c1\n> + * \\brief Constant used in the analog Gain control coding/decoding.\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::analogueGain_\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> +\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> +\n> +CameraSensorHelperFactory::CameraSensorHelperFactory(const char *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()\n> +{\n> +\tCameraSensorHelper *handler = createInstance();\n> +\treturn std::unique_ptr(handler);\n\nThere's no handler. s/handler/helper/ (or any other better name you can\nthink of). Same below.\n\n> +}\n> +\n> +/**\n> + * \\fn CameraSensorHelperFactory::name()\n> + * \\brief Retrieve the factory name\n> + * \\return The factory name\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> + * \\fn 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] handler 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> +\n> +/**\n> + * \\fn CameraSensorHelperImx219::CameraSensorHelperImx219\n> + * \\brief Construct a CameraSensorHelperImx219 instance for imx219\n> + * \\param[in] name The sensor model name\n> + */\n> +class CameraSensorHelperImx219 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperImx219(const char *name);\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> +CameraSensorHelperImx219::CameraSensorHelperImx219(const char *name)\n> +\t: CameraSensorHelper(name)\n> +{\n> +\tanalogueGain_ = { GlobalGain, 0, -1, 256, 256 };\n> +}\n> +\n> +/**\n> + * \\class CameraSensorHelperOv5670\n> + * \\brief Create and give helpers for the ov5670 sensor\n> + */\n> +\n> +/**\n> + * \\fn CameraSensorHelperOv5670::CameraSensorHelperOv5670\n> + * \\brief Construct a CameraSensorHelperOv5670 instance for ov5670\n> + * \\param[in] name The sensor model name\n> + */\n> +class CameraSensorHelperOv5670 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperOv5670(const char *name);\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n> +CameraSensorHelperOv5670::CameraSensorHelperOv5670(const char *name)\n> +\t: CameraSensorHelper(name)\n> +{\n> +\tanalogueGain_ = { GlobalGain, 1, 0, 0, 256 };\n> +}\n> +\n> +/**\n> + * \\class CameraSensorHelperOv5693\n> + * \\brief Create and give helpers for the ov5693 sensor\n> + */\n> +\n> +/**\n> + * \\fn CameraSensorHelperOv5693::CameraSensorHelperOv5693\n> + * \\brief Construct a CameraSensorHelperOv5693 instance for ov5693\n> + * \\param[in] name The sensor model name\n> + */\n> +class CameraSensorHelperOv5693 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperOv5693(const char *name);\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> +CameraSensorHelperOv5693::CameraSensorHelperOv5693(const char *name)\n> +\t: CameraSensorHelper(name)\n> +{\n> +\tanalogueGain_ = { GlobalGain, 1, 0, 0, 16 };\n> +}\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\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..63031902\n> --- /dev/null\n> +++ b/src/ipa/libipa/camera_sensor_helper.h\n> @@ -0,0 +1,91 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2021 Ideas On Board\n> + *\n> + * camera_sensor_helper.h - helper class providing camera informations\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> +\n> +#include \n\nThis doesn't seem to be used, but it should, the CameraSensorHelper and\nCameraSensorHelperFactory classes should not be copyable or moveable.\n\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +class CameraSensorHelper;\n> +\n> +class CameraSensorHelper : public Object\n\nAny reason this class inherits from Object ?\n\n> +{\n> +public:\n> +\tCameraSensorHelper(const char *name);\n> +\tvirtual ~CameraSensorHelper();\n> +\n> +\tconst std::string &name() const { return name_; }\n\nThis doesn't seem to be used.\n\n> +\tuint32_t getGainCode(double gain) const;\n> +\tdouble getGain(uint32_t gainCode) const;\n\nWe don't usually use a \"get\" prefix, should these be called gainCode()\nand gain() ?\n\n> +\n> +protected:\n> +\tenum AnalogueGainCapability : uint16_t {\n\nIs there a need to specify the underlying data type explicitly ?\n\n> +\t\tGlobalGain = 0,\n> +\t\tAlternateGlobalGain = 2,\n> +\t};\n> +\n> +\tstruct analogueGainConstants {\n\ns/analogueGainConstants/AnalogueGainConstants/\n\n> +\t\tuint16_t type;\n> +\t\tint16_t m0;\n> +\t\tint16_t c0;\n> +\t\tint16_t m1;\n> +\t\tint16_t c1;\n> +\t};\n> +\tanalogueGainConstants analogueGain_;\n\nThe variable name can be mistaken to mean it stores a gain, while it\nstores the constants.\n\nIt would be nice to make this const.\n\n> +\n> +private:\n> +\tstd::string name_;\n> +\tfriend class CameraSensorHelperFactory;\n> +};\n> +\n> +class CameraSensorHelperFactory\n> +{\n> +public:\n> +\tCameraSensorHelperFactory(const char *name);\n> +\tvirtual ~CameraSensorHelperFactory() = default;\n> +\n> +\tstd::unique_ptr create();\n> +\n> +\tconst std::string &name() const { return name_; }\n\nThis doesn't seem to be used.\n\n> +\n> +\tstatic void registerType(CameraSensorHelperFactory *factory);\n> +\tstatic std::vector &factories();\n> +\n> +private:\n> +\tvirtual CameraSensorHelper *createInstance() = 0;\n> +\n> +\tstd::string name_;\n> +};\n> +\n> +#define REGISTER_CAMERA_SENSOR_HELPER(name, handler) \\\n> +\tclass handler##Factory final : public CameraSensorHelperFactory \\\n> +\t{ \\\n> +\tpublic: \\\n> +\t\thandler##Factory() : CameraSensorHelperFactory(#handler) {} \\\n> + \\\n> +\tprivate: \\\n> +\t\tCameraSensorHelper *createInstance() \\\n> +\t\t{ \\\n> +\t\t\treturn new handler(name); \\\n> +\t\t} \\\n> +\t}; \\\n> +\tstatic handler##Factory global_##handler##Factory;\n\nYou can drop one indentation level (I know checkstyle.py complains).\n\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n> +\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>","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 704FBBD22E\n\tfor ;\n\tTue, 8 Jun 2021 15:46:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2E341602C5;\n\tTue, 8 Jun 2021 17:46:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E77ED602A0\n\tfor ;\n\tTue, 8 Jun 2021 17:46:55 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7C1273E6;\n\tTue, 8 Jun 2021 17:46:55 +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=\"m5182bhw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623167215;\n\tbh=wLuqjglU3gi3+dBwsHxqyBxVYZisxvd6ySuf3XOsRlk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=m5182bhwEPTL0itYJKqdnLZ3BeXEOY7Uus+kSss+1lrdqzWdZVOqL0fdSrd/1FVmn\n\twBoGXRT8XbMTuKz1TVZBdn1OjqTgmfh8b7Nsd740sE5KQliW6fBfVzLcuiK3AY0n+R\n\t/iGSfJh6Nm965new5bq3vZtsNkVoiPfKP9D2Cp/8=","Date":"Tue, 8 Jun 2021 18:46:40 +0300","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20210607123533.51198-1-jeanmichel.hautbois@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210607123533.51198-1-jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 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":17483,"web_url":"https://patchwork.libcamera.org/comment/17483/","msgid":"","date":"2021-06-08T18:02:00","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: Create a camera sensor\n\thelper class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jun 07, 2021 at 06:05:33PM +0200, Jacopo Mondi wrote:\n> On Mon, Jun 07, 2021 at 02:35:32PM +0200, Jean-Michel Hautbois wrote:\n> > Setting analogue gain for a specific sensor is not a straghtforward\n> > operation, as one needs to know how the gain is calculated for it.\n> \n> Please bear with my poor understanding of the issue, but looking at\n> the code it doesn't seem the different is in how the gain is\n> calculated but rather in which constant parameters are used.\n\nThat's partly true (and one could argue that the parameters are part of\nthe calculation, but that's more of a semantics discussion). There are\nsensors that implement a gain model which can't be covered by the\nformulas in this patch. We expect more work in this area when we'll have\nto support such sensors.\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> \n> I should first go through this document, so I'll probably have only\n> drive-by comments and nits here and there\n> \n> > Is makes it possible to only have 4 parameters to store per sensor, and\n> > the gain calculation is then done identically for all of them. This\n> > commit gives the gains for imx219 (based on RPi cam_helper), ov5670 and\n> > ov5693.\n> >\n> > Adding a new sensor is pretty straightfoward as one only needs to\n> > implement the sub-class for it and register that class to the\n> > CameraSensorHelperFactory.\n> \n> As asked offline, the current fatory implementation is the same as the\n> one we have for pipeline handlers, which requires static\n> initialization to make sure they are registered at library\n> initialization time. I suspect without this requirement the factory\n> here could be made simpler, but that's minor...\n> \n> > Signed-off-by: Jean-Michel Hautbois \n> > ---\n> > src/ipa/libipa/camera_sensor_helper.cpp | 361 ++++++++++++++++++++++++\n> > src/ipa/libipa/camera_sensor_helper.h | 91 ++++++\n> > src/ipa/libipa/meson.build | 2 +\n> > 3 files changed, 454 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..2bd82861\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > @@ -0,0 +1,361 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2021 Ideas On Board\n> > + *\n> > + * camera_sensor_helper.cpp - helper class providing camera informations\n> \n> Nit: 'Helper'\n> as I've received the same comment multiple times, 'information' is\n> uncountable, so no plural\n> \n> > + */\n> > +#include \"camera_sensor_helper.h\"\n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/log.h\"\n> > +\n> > +/**\n> > + * \\file camera_sensor_helper.h\n> > + * \\brief Create helper class for each sensor\n> > + *\n> > + * Each camera sensor supported by libcamera may need specific informations to\n> > + * be sent (analogue gain is sensor dependant for instance).\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 Create and give helpers for each camera sensor in the pipeline\n> \n> I would here and maybe above in the file description drop the 'create\n> and give' part. The CameraSensorHelper base class computes sensor\n> tuning parameters using sensor-specific constants. The factory instead\n> does the actual object creation\n> \n> > + *\n> sensor-specific constants. The factory instead does the actual object\n> creation\n> \n> > + * CameraSensorHelper instances are unique and sensor dependant.\n> \n> Is the base class meant to be used as a default if not sensor-specific\n> sub-class is registered ?\n> \n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a CameraSensorHelper instance\n> > + * \\param[in] name The sensor model name\n> > + *\n> > + * The CameraSensorHelper instances shall never be constructed manually, but always\n> > + * through the CameraSensorHelperFactory::create() method implemented by the\n> > + * respective factories.\n> > + */\n> > +\n> \n> Empty line\n> \n> > +CameraSensorHelper::CameraSensorHelper(const char *name)\n> > +\t: name_(name)\n> > +{\n> > +\tanalogueGain_ = { GlobalGain, 1, 1, 0, 0 };\n> > +}\n> \n> I wonder, we don't support AlternateGlobalGain at the moment, do you\n> expect it to be required soon ?\n> \n> > +\n> > +CameraSensorHelper::~CameraSensorHelper()\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\fn CameraSensorHelper::getGainCode(double gain)\n> \n> If you document the function just defined below, you don't need the\n> \\fn anchor\n> \n> Library-wide we don't use 'get' for getters, so this might just be\n> gainCode() ?\n> \n> > + * \\brief Get the analogue gain code to pass to V4L2 subdev control\n> \n> Aren't we computing the gain code from the analogue gain value ?\n> \n> > + * \\param[in] gain The real gain to pass\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> Do you mean the m0, c0, etc etc paramteres ? Maybe this is better\n> specified in the enum documentation ?\n> \n> > + */\n> > +uint32_t CameraSensorHelper::getGainCode(double gain) const\n> > +{\n> > +\t/* \\todo we only support the global gain mode for now */\n> > +\tif (analogueGain_.type != GlobalGain)\n> > +\t\treturn UINT32_MAX;\n> > +\n> > +\tif (analogueGain_.m0 == 0)\n> > +\t\treturn (analogueGain_.c0 - gain * analogueGain_.c1) / (gain * analogueGain_.m1);\n> > +\tif (analogueGain_.m1 == 0)\n> > +\treturn (gain * analogueGain_.c1 - analogueGain_.c0) / analogueGain_.m0;\n> \n> You can break the two lines\n> \n> > +\n> > +\tLOG(CameraSensorHelper, Error) << \"For any given image sensor either m0 or m1 shall be zero.\";\n> > +\treturn 1;\n> \n> Aren't the two error conditions verified by assertions ? Otherwise we\n> return two different error codes (MAXINT and 1) which are difficult to\n> identify.\n> \n> > +}\n> > +\n> > +/**\n> > + * \\fn CameraSensorHelper::getGain\n> > + * \\brief Get the real gain from the V4l2 subdev control gain\n> > + * \\param[in] gainCode The V4l2 subdev control 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::getGainCode.\n> > + *\n> > + * The parameters come from the MIPI Alliance Camera Specification for\n> > + * Camera Command Set (CCS).\n> \n> Same comments as above\n> \n> > + */\n> > +double CameraSensorHelper::getGain(uint32_t gainCode) const\n> > +{\n> > +\tif (analogueGain_.type != GlobalGain)\n> > +\t\treturn UINT32_MAX;\n> > +\n> > +\tif (analogueGain_.m0 == 0)\n> > +\t\treturn analogueGain_.c0 / (analogueGain_.m1 * gainCode + analogueGain_.c1);\n> > +\tif (analogueGain_.m1 == 0)\n> > +\t\treturn (analogueGain_.m0 * gainCode + analogueGain_.c0) / analogueGain_.c1;\n> > +\n> > +\tLOG(CameraSensorHelper, Error) << \"For any given image sensor either m0 or m1 shall be zero.\";\n> > +\treturn 1.0;\n> > +}\n> \n> ditto\n> \n> Also, these smells like static functions as they only use class\n> members constants for computations and do not hold any state. This\n> however does not play well with the class hierarchy...\n> \n> > +\n> > +/**\n> > + * \\fn CameraSensorHelper::name()\n> > + * \\brief Retrieve the camera sensor helper name\n> > + * \\return The camera sensor helper name\n> > + */\n> > +\n> > +/**\n> > + * \\enum CameraSensorHelper::AnalogueGainCapability\n> > + * \\brief Specify the Gain mode supported by the sensor\n> > + *\n> > + * Describes the image sensor analog Gain capabilities.\n> > + * Two modes are possible, depending on the sensor: Global and Alternate.\n> > + */\n> > +\n> > +/**\n> > + * \\var CameraSensorHelper::GlobalGain\n> > + * \\brief Sensor supports Global gain\n> > + *\n> > + * The relationship between the integer Gain parameter and the resulting Gain\n> > + * multiplier is given by the following equation:\n> > + *\n> > + * \\f$\\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 exposed by 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$\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n> > + */\n> > +\n> > +/**\n> > + * \\var CameraSensorHelper::AlternateGlobalGain\n> > + * \\brief Sensor supports Analogue Global gain (introduced in CCS v1.1)\n> > + *\n> > + * Starting with CCS v1.1, Alternate Global Analog Gain is also available.\n> > + * If the image sensor supports it, then the global analog 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> How do you envision this class to evolve ? I mean, it will go beyoind\n> just analogue gain calculations ? As otherwise a single class with a\n> map of sensor specific gain constants would do...\n> \n> > +\n> > +/**\n> > + * \\var CameraSensorHelper::analogueGainConstants::type\n> > + * \\brief Analogue gain coding type\n> > + */\n> > +\n> > +/**\n> > + * \\var CameraSensorHelper::analogueGainConstants::m0\n> > + * \\brief Constant used in the analog Gain control 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 analog Gain control coding/decoding.\n> > + */\n> > +\n> > +/**\n> > + * \\var CameraSensorHelper::analogueGainConstants::m1\n> > + * \\brief Constant used in the analog Gain control coding/decoding.\n> \n> Nit: briefs do not end with '.'\n> \n> > + *\n> > + * Note: either m0 or m1 shall be zero.\n> > + */\n> > +\n> > +/**\n> > + * \\var CameraSensorHelper::analogueGainConstants::c1\n> > + * \\brief Constant used in the analog Gain control coding/decoding.\n> \n> I presume I have to read the CCS specs to know what this parameters\n> actually are :)\n> \n> > + */\n> > +\n> > +/**\n> > + * \\var CameraSensorHelper::analogueGain_\n> \n> Maybe something in the name that makes it clear these are params ?\n> Analouge gain to me is what 'getGain()' calculates, am I mistaken ?\n> \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> Empty line\n> \n> > + */\n> > +\n> \n> I'll skip the factory review for now\n> \n> > +/**\n> \n> [snip]\n> \n> > +\n> > +/**\n> > + * \\fn CameraSensorHelperImx219::CameraSensorHelperImx219\n> > + * \\brief Construct a CameraSensorHelperImx219 instance for imx219\n> > + * \\param[in] name The sensor model name\n> > + */\n\nNo need to document each subclass.\n\n> > +class CameraSensorHelperImx219 : public CameraSensorHelper\n> > +{\n> > +public:\n> > +\tCameraSensorHelperImx219(const char *name);\n> > +};\n> > +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> \n> I would add an empty line here and make it look like\n> \n> class CameraSensorHelperXYZ : public CameraSensorHelper\n> {\n> public:\n> \tCameraSensorHelperXYZ(const char *name)\n> {\n> ...\n> }\n> };\n\nAgreed, an inline constructor seems better.\n\n> REGISTER_CAMERA_SENSOR_HELPER(\"XYZ\", CameraSensorHelperXYZ)\n> \n> \n> > +CameraSensorHelperImx219::CameraSensorHelperImx219(const char *name)\n> > +\t: CameraSensorHelper(name)\n> > +{\n> > +\tanalogueGain_ = { GlobalGain, 0, -1, 256, 256 };\n> > +}\n> > +\n> > +/**\n> > + * \\class CameraSensorHelperOv5670\n> > + * \\brief Create and give helpers for the ov5670 sensor\n> > + */\n> > +\n> > +/**\n> > + * \\fn CameraSensorHelperOv5670::CameraSensorHelperOv5670\n> > + * \\brief Construct a CameraSensorHelperOv5670 instance for ov5670\n> > + * \\param[in] name The sensor model name\n> > + */\n> > +class CameraSensorHelperOv5670 : public CameraSensorHelper\n> > +{\n> > +public:\n> > +\tCameraSensorHelperOv5670(const char *name);\n> > +};\n> > +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n> > +CameraSensorHelperOv5670::CameraSensorHelperOv5670(const char *name)\n> > +\t: CameraSensorHelper(name)\n> > +{\n> > +\tanalogueGain_ = { GlobalGain, 1, 0, 0, 256 };\n> > +}\n> > +\n> > +/**\n> > + * \\class CameraSensorHelperOv5693\n> > + * \\brief Create and give helpers for the ov5693 sensor\n> > + */\n> > +\n> > +/**\n> > + * \\fn CameraSensorHelperOv5693::CameraSensorHelperOv5693\n> > + * \\brief Construct a CameraSensorHelperOv5693 instance for ov5693\n> > + * \\param[in] name The sensor model name\n> > + */\n> > +class CameraSensorHelperOv5693 : public CameraSensorHelper\n> > +{\n> > +public:\n> > +\tCameraSensorHelperOv5693(const char *name);\n> > +};\n> > +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> > +CameraSensorHelperOv5693::CameraSensorHelperOv5693(const char *name)\n> > +\t: CameraSensorHelper(name)\n> > +{\n> > +\tanalogueGain_ = { GlobalGain, 1, 0, 0, 16 };\n> > +}\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\n> > +\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..63031902\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/camera_sensor_helper.h\n> > @@ -0,0 +1,91 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2021 Ideas On Board\n> > + *\n> > + * camera_sensor_helper.h - helper class providing camera informations\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> > +\n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa {\n> > +\n> > +class CameraSensorHelper;\n> > +\n> > +class CameraSensorHelper : public Object\n> > +{\n> > +public:\n> > +\tCameraSensorHelper(const char *name);\n> > +\tvirtual ~CameraSensorHelper();\n> > +\n> > +\tconst std::string &name() const { return name_; }\n> > +\tuint32_t getGainCode(double gain) const;\n> > +\tdouble getGain(uint32_t gainCode) const;\n> > +\n> > +protected:\n> > +\tenum AnalogueGainCapability : uint16_t {\n> > +\t\tGlobalGain = 0,\n> > +\t\tAlternateGlobalGain = 2,\n> \n> Are the gain type values defined by the CCS specs ?\n> \n> > +\t};\n> > +\n> > +\tstruct analogueGainConstants {\n> > +\t\tuint16_t type;\n> > +\t\tint16_t m0;\n> > +\t\tint16_t c0;\n> > +\t\tint16_t m1;\n> > +\t\tint16_t c1;\n> > +\t};\n> > +\tanalogueGainConstants analogueGain_;\n> > +\n> > +private:\n> > +\tstd::string name_;\n> > +\tfriend class CameraSensorHelperFactory;\n> \n> Do you need this friend class declaration ?\n> The constructor is public...\n\nDoesn't seem needed indeed.\n\n> > +};\n> > +\n> > +class CameraSensorHelperFactory\n> > +{\n> > +public:\n> > +\tCameraSensorHelperFactory(const char *name);\n> > +\tvirtual ~CameraSensorHelperFactory() = default;\n> > +\n> > +\tstd::unique_ptr create();\n> > +\n> > +\tconst std::string &name() const { return name_; }\n> > +\n> > +\tstatic void registerType(CameraSensorHelperFactory *factory);\n> > +\tstatic std::vector &factories();\n> > +\n> > +private:\n> > +\tvirtual CameraSensorHelper *createInstance() = 0;\n> > +\n> > +\tstd::string name_;\n> > +};\n> > +\n> > +#define REGISTER_CAMERA_SENSOR_HELPER(name, handler) \\\n> > +\tclass handler##Factory final : public CameraSensorHelperFactory \\\n> > +\t{ \\\n> > +\tpublic: \\\n> > +\t\thandler##Factory() : CameraSensorHelperFactory(#handler) {} \\\n> > + \\\n> > +\tprivate: \\\n> > +\t\tCameraSensorHelper *createInstance() \\\n> > +\t\t{ \\\n> > +\t\t\treturn new handler(name); \\\n> > +\t\t} \\\n> > +\t}; \\\n> > +\tstatic handler##Factory global_##handler##Factory;\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n> > +\n> \n> No empty line at EOF otherwise git am complains (at least, that's how\n> I interpret the error message :)\n> \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> >","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 7C109BD22E\n\tfor ;\n\tTue, 8 Jun 2021 18:02:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C8876602A7;\n\tTue, 8 Jun 2021 20:02:16 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CD897602A7\n\tfor ;\n\tTue, 8 Jun 2021 20:02:15 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 40DBF3E6;\n\tTue, 8 Jun 2021 20:02:15 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ssGy1Dv/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623175335;\n\tbh=HxfHGLssQul1olDassFcuJdYN6poe7S1rdIOAMjQBN0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ssGy1Dv/QD1SZ70Cbs96dqhCCNOj75bMk9XM0xLzEI+Ms/tZWJqHoOZ39AK46bDWE\n\tjXOQ6CZD9Q8Ekn3IViIpCBiANs0b/YnRvALiiSYbdBqO1VyTXX7b0gUPWWvABZSPk5\n\tyJ6ffVARykK03omWCE0ltBfOoOAA8kyFc8wtIYPw=","Date":"Tue, 8 Jun 2021 21:02:00 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210607123533.51198-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210607160533.xogfthhdgmzxy5pm@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210607160533.xogfthhdgmzxy5pm@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 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":17484,"web_url":"https://patchwork.libcamera.org/comment/17484/","msgid":"","date":"2021-06-08T18:56:11","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: Create a camera sensor\n\thelper class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Tue, Jun 08, 2021 at 09:42:24AM +0100, Naushir Patuck wrote:\n> On Mon, 7 Jun 2021 at 20:35, Jean-Michel Hautbois wrote:\n> > On 07/06/2021 20:15, Jean-Michel Hautbois wrote:\n> > > On 07/06/2021 19:31, Naushir Patuck wrote:\n> > >> On Mon, 7 Jun 2021 at 13:35, Jean-Michel Hautbois wrote:\n> > >>>\n> > >>> Setting analogue gain for a specific sensor is not a straghtforward\n> > >>> operation, as one needs to know how the gain is calculated for it.\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> > >>\n> > >> I know this has probably been discussed in detail internally, but what provisions\n> > >> do you expect for gain formulas that do not conform to the CCI formula, or those\n> > >> that are entirely table based?\n> > >\n> > > It has not been discussed in detail no, and I am sure I have missed a lot of cases.\n> > > For those specific cases, we may, for instance, have a new value in\n> > > AnalogueGainCapability enum and let the subclass calculate the proper value ?\n> > > I don't have this case right now in my hands, so it is difficult to know\n> > > precisely what is the best way to do so ;-).\n> > >\n> > >>> Is makes it possible to only have 4 parameters to store per sensor, and\n> > >>> the gain calculation is then done identically for all of them. This\n> > >>> commit gives the gains for imx219 (based on RPi cam_helper), ov5670 and\n> > >>> ov5693.\n> > >>>\n> > >>> Adding a new sensor is pretty straightfoward 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> > >>> src/ipa/libipa/camera_sensor_helper.cpp | 361++++++++++++++++++++++++\n> > >>> src/ipa/libipa/camera_sensor_helper.h | 91 ++++++\n> > >>> src/ipa/libipa/meson.build | 2 +\n> > >>> 3 files changed, 454 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\n> > >>> b/src/ipa/libipa/camera_sensor_helper.cpp\n> > >>> new file mode 100644\n> > >>> index 00000000..2bd82861\n> > >>> --- /dev/null\n> > >>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > >>> @@ -0,0 +1,361 @@\n> > >>> +/* SPDX-License-Identifier: BSD-2-Clause */\n> > >>> +/*\n> > >>> + * Copyright (C) 2021 Ideas On Board\n> > >>> + *\n> > >>> + * camera_sensor_helper.cpp - helper class providing camerainformations\n> > >>> + */\n> > >>> +#include \"camera_sensor_helper.h\"\n> > >>> +\n> > >>> +#include \n> > >>> +\n> > >>> +#include \"libcamera/internal/log.h\"\n> > >>> +\n> > >>> +/**\n> > >>> + * \\file camera_sensor_helper.h\n> > >>> + * \\brief Create helper class for each sensor\n> > >>> + *\n> > >>> + * Each camera sensor supported by libcamera may need specificinformations to\n> > >>> + * be sent (analogue gain is sensor dependant for instance).\n> > >>> + *\n> > >>> + * Every subclass of CameraSensorHelper shall be registered withlibipa 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 Create and give helpers for each camera sensor in thepipeline\n> > >>> + *\n> > >>> + * CameraSensorHelper instances are unique and sensor dependant.\n> > >>> + */\n> > >>> +\n> > >>> +/**\n> > >>> + * \\brief Construct a CameraSensorHelper instance\n> > >>> + * \\param[in] name The sensor model name\n> > >>> + *\n> > >>> + * The CameraSensorHelper instances shall never be constructedmanually, but always\n> > >>> + * through the CameraSensorHelperFactory::create() methodimplemented by the\n> > >>> + * respective factories.\n> > >>> + */\n> > >>> +\n> > >>> +CameraSensorHelper::CameraSensorHelper(const char *name)\n> > >>> + : name_(name)\n> > >>> +{\n> > >>> + analogueGain_ = { GlobalGain, 1, 1, 0, 0 };\n> > >>> +}\n> > >>> +\n> > >>> +CameraSensorHelper::~CameraSensorHelper()\n> > >>> +{\n> > >>> +}\n> > >>> +\n> > >>> +/**\n> > >>> + * \\fn CameraSensorHelper::getGainCode(double gain)\n> > >>> + * \\brief Get the analogue gain code to pass to V4L2 subdev control\n> > >>> + * \\param[in] gain The real gain to pass\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::getGainCode(double gain) const\n> > >>> +{\n> > >>> + /* \\todo we only support the global gain mode for now */\n> > >>> + if (analogueGain_.type != GlobalGain)\n> > >>> + return UINT32_MAX;\n> > >>> +\n> > >>> + if (analogueGain_.m0 == 0)\n> > >>> + return (analogueGain_.c0 - gain * analogueGain_.c1) / (gain * analogueGain_.m1);\n> > >>> + if (analogueGain_.m1 == 0)\n> > >>> + return (gain * analogueGain_.c1 - analogueGain_.c0) / analogueGain_.m0;\n> > >>> +\n> > >>> + LOG(CameraSensorHelper, Error) << \"For any given image sensor either m0 or m1 shall be zero.\";\n> > >>> + return 1;\n> > >>> +}\n> > >>> +\n> > >>> +/**\n> > >>> + * \\fn CameraSensorHelper::getGain\n> > >>> + * \\brief Get the real gain from the V4l2 subdev control gain\n> > >>> + * \\param[in] gainCode The V4l2 subdev control 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::getGainCode.\n> > >>> + *\n> > >>> + * The parameters come from the MIPI Alliance Camera Specification for\n> > >>> + * Camera Command Set (CCS).\n> > >>> + */\n> > >>> +double CameraSensorHelper::getGain(uint32_t gainCode) const\n> > >>> +{\n> > >>> + if (analogueGain_.type != GlobalGain)\n> > >>> + return UINT32_MAX;\n> > >>> +\n> > >>> + if (analogueGain_.m0 == 0)\n> > >>> + return analogueGain_.c0 / (analogueGain_.m1 * gainCode + analogueGain_.c1);\n> > >>> + if (analogueGain_.m1 == 0)\n> > >>> + return (analogueGain_.m0 * gainCode + analogueGain_.c0) / analogueGain_.c1;\n> > >>> +\n> > >>> + LOG(CameraSensorHelper, Error) << \"For any given image sensor either m0 or m1 shall be zero.\";\n> > >>> + return 1.0;\n> > >>> +}\n> > >>> +\n> > >>> +/**\n> > >>> + * \\fn CameraSensorHelper::name()\n> > >>> + * \\brief Retrieve the camera sensor helper name\n> > >>> + * \\return The camera sensor helper name\n> > >>> + */\n> > >>> +\n> > >>> +/**\n> > >>> + * \\enum CameraSensorHelper::AnalogueGainCapability\n> > >>> + * \\brief Specify the Gain mode supported by the sensor\n> > >>> + *\n> > >>> + * Describes the image sensor analog Gain capabilities.\n> > >>> + * Two modes are possible, depending on the sensor: Global and Alternate.\n> > >>> + */\n> > >>> +\n> > >>> +/**\n> > >>> + * \\var CameraSensorHelper::GlobalGain\n> > >>> + * \\brief Sensor supports Global gain\n> > >>> + *\n> > >>> + * The relationship between the integer Gain parameter and the resulting Gain\n> > >>> + * multiplier is given by the following equation:\n> > >>> + *\n> > >>> + * \\f$\\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 exposed by 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$\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n> > >>> + */\n> > >>> +\n> > >>> +/**\n> > >>> + * \\var CameraSensorHelper::AlternateGlobalGain\n> > >>> + * \\brief Sensor supports Analogue Global gain (introduced in CCS v1.1)\n> > >>> + *\n> > >>> + * Starting with CCS v1.1, Alternate Global Analog Gain is alsoavailable.\n> > >>> + * If the image sensor supports it, then the global analog Gain canbe 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 coding type\n> > >>> + */\n> > >>> +\n> > >>> +/**\n> > >>> + * \\var CameraSensorHelper::analogueGainConstants::m0\n> > >>> + * \\brief Constant used in the analog Gain control 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 analog Gain control coding/decoding.\n> > >>> + */\n> > >>> +\n> > >>> +/**\n> > >>> + * \\var CameraSensorHelper::analogueGainConstants::m1\n> > >>> + * \\brief Constant used in the analog Gain control coding/decoding.\n> > >>> + *\n> > >>> + * Note: either m0 or m1 shall be zero.\n> > >>> + */\n> > >>> +\n> > >>> +/**\n> > >>> + * \\var CameraSensorHelper::analogueGainConstants::c1\n> > >>> + * \\brief Constant used in the analog Gain control coding/decoding.\n> > >>> + */\n> > >>> +\n> > >>> +/**\n> > >>> + * \\var CameraSensorHelper::analogueGain_\n> > >>> + * \\brief The analogue gain parameters used for calculation\n> > >>> + *\n> > >>> + * The analogue gain is calculated through a formula, and itsparameters are\n> > >>> + * sensor specific. Use this variable to store the values at init time.\n> > >>> + *\n> > >>> + */\n> > >>> +\n> > >>> +/**\n> > >>> + * \\class CameraSensorHelperFactory\n> > >>> + * \\brief Registration of CameraSensorHelperFactory classes andcreation of instances\n> > >>> + *\n> > >>> + * To facilitate discovery and instantiation of CameraSensorHelperclasses, the\n> > >>> + * CameraSensorHelperFactory class maintains a registry of camerasensor helper\n> > >>> + * classes. Each CameraSensorHelper subclass shall register itselfusing the\n> > >>> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create acorresponding\n> > >>> + * instance of a CameraSensorHelperFactory subclass and register itwith 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 globallist 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> > >>> +\n> > >>> +CameraSensorHelperFactory::CameraSensorHelperFactory(const char *name)\n> > >>> + : name_(name)\n> > >>> +{\n> > >>> + registerType(this);\n> > >>> +}\n> > >>> +\n> > >>> +/**\n> > >>> + * \\brief Create an instance of the CameraSensorHelpercorresponding to the factory\n> > >>> + *\n> > >>> + * \\return A unique pointer to a new instance of theCameraSensorHelper subclass\n> > >>> + * corresponding to the factory\n> > >>> + */\n> > >>> +std::unique_ptr CameraSensorHelperFactory::create()\n> > >>> +{\n> > >>> + CameraSensorHelper *handler = createInstance();\n> > >>> + return std::unique_ptr(handler);\n> > >>> +}\n> > >>> +\n> > >>> +/**\n> > >>> + * \\fn CameraSensorHelperFactory::name()\n> > >>> + * \\brief Retrieve the factory name\n> > >>> + * \\return The factory name\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 sensorhelper\n> > >>> + *\n> > >>> + * The caller is responsible to guarantee the uniqueness of thecamera sensor helper\n> > >>> + * name.\n> > >>> + */\n> > >>> +voidCameraSensorHelperFactory::registerType(CameraSensorHelperFactory*factory)\n> > >>> +{\n> > >>> + std::vector &factories =CameraSensorHelperFactory::factories();\n> > >>> +\n> > >>> + factories.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 toensures 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> > >>> + static std::vector factories;\n> > >>> + return factories;\n> > >>> +}\n> > >>> +\n> > >>> +/**\n> > >>> + * \\fn CameraSensorHelperFactory::createInstance()\n> > >>> + * \\brief Create an instance of the CameraSensorHelpercorresponding to the factory\n> > >>> + *\n> > >>> + * This virtual function is implemented by theREGISTER_CAMERA_SENSOR_HELPER()\n> > >>> + * macro. It creates a camera sensor helper instance associatedwith the camera\n> > >>> + * sensor model.\n> > >>> + *\n> > >>> + * \\return A pointer to a newly constructed instance of theCameraSensorHelper\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 sensorhelper factory\n> > >>> + * \\param[in] name Sensor model name used to register the class\n> > >>> + * \\param[in] handler Class name of CameraSensorHelper derivedclass to register\n> > >>> + *\n> > >>> + * Register a CameraSensorHelper subclass with the factory and makeit 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> > >>> +\n> > >>> +/**\n> > >>> + * \\fn CameraSensorHelperImx219::CameraSensorHelperImx219\n> > >>> + * \\brief Construct a CameraSensorHelperImx219 instance for imx219\n> > >>> + * \\param[in] name The sensor model name\n> > >>> + */\n> > >>> +class CameraSensorHelperImx219 : public CameraSensorHelper\n> > >>> +{\n> > >>> +public:\n> > >>> + CameraSensorHelperImx219(const char *name);\n> > >>> +};\n> > >>> +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> > >>> +CameraSensorHelperImx219::CameraSensorHelperImx219(const char *name)\n> > >>> + : CameraSensorHelper(name)\n> > >>> +{\n> > >>> + analogueGain_ = { GlobalGain, 0, -1, 256, 256 };\n> > >>> +}\n> > >>> +\n> > >>> +/**\n> > >>> + * \\class CameraSensorHelperOv5670\n> > >>> + * \\brief Create and give helpers for the ov5670 sensor\n> > >>> + */\n> > >>> +\n> > >>> +/**\n> > >>> + * \\fn CameraSensorHelperOv5670::CameraSensorHelperOv5670\n> > >>> + * \\brief Construct a CameraSensorHelperOv5670 instance for ov5670\n> > >>> + * \\param[in] name The sensor model name\n> > >>> + */\n> > >>> +class CameraSensorHelperOv5670 : public CameraSensorHelper\n> > >>> +{\n> > >>> +public:\n> > >>> + CameraSensorHelperOv5670(const char *name);\n> > >>> +};\n> > >>> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n> > >>> +CameraSensorHelperOv5670::CameraSensorHelperOv5670(const char *name)\n> > >>> + : CameraSensorHelper(name)\n> > >>> +{\n> > >>> + analogueGain_ = { GlobalGain, 1, 0, 0, 256 };\n> > >>> +}\n> > >>> +\n> > >>> +/**\n> > >>> + * \\class CameraSensorHelperOv5693\n> > >>> + * \\brief Create and give helpers for the ov5693 sensor\n> > >>> + */\n> > >>> +\n> > >>> +/**\n> > >>> + * \\fn CameraSensorHelperOv5693::CameraSensorHelperOv5693\n> > >>> + * \\brief Construct a CameraSensorHelperOv5693 instance for ov5693\n> > >>> + * \\param[in] name The sensor model name\n> > >>> + */\n> > >>> +class CameraSensorHelperOv5693 : public CameraSensorHelper\n> > >>> +{\n> > >>> +public:\n> > >>> + CameraSensorHelperOv5693(const char *name);\n> > >>> +};\n> > >>> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> > >>> +CameraSensorHelperOv5693::CameraSensorHelperOv5693(const char *name)\n> > >>> + : CameraSensorHelper(name)\n> > >>> +{\n> > >>> + analogueGain_ = { GlobalGain, 1, 0, 0, 16 };\n> > >>> +}\n> > >>> +\n> > >>> +} /* namespace ipa */\n> > >>> +\n> > >>> +} /* namespace libcamera */\n> > >>> +\n> > >>> diff --git a/src/ipa/libipa/camera_sensor_helper.h\n> > >>> b/src/ipa/libipa/camera_sensor_helper.h\n> > >>> new file mode 100644\n> > >>> index 00000000..63031902\n> > >>> --- /dev/null\n> > >>> +++ b/src/ipa/libipa/camera_sensor_helper.h\n> > >>> @@ -0,0 +1,91 @@\n> > >>> +/* SPDX-License-Identifier: BSD-2-Clause */\n> > >>> +/*\n> > >>> + * Copyright (C) 2021 Ideas On Board\n> > >>> + *\n> > >>> + * camera_sensor_helper.h - helper class providing camera informations\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> > >>> +\n> > >>> +#include \n> > >>> +#include \n> > >>> +\n> > >>> +namespace libcamera {\n> > >>> +\n> > >>> +namespace ipa {\n> > >>> +\n> > >>> +class CameraSensorHelper;\n> > >>> +\n> > >>> +class CameraSensorHelper : public Object\n> > >>> +{\n> > >>> +public:\n> > >>> + CameraSensorHelper(const char *name);\n> > >>> + virtual ~CameraSensorHelper();\n> > >>> +\n> > >>> + const std::string &name() const { return name_; }\n> > >>> + uint32_t getGainCode(double gain) const;\n> >\n> > Would you prefer getGainCode and getGain to be virtual, having a default\n> > meyhod in CameraSensorHelper and letting those beeing overriden if a\n> > specific sensor needs to have a different formula ?\n> \n> The Raspberry Pi CamHelper does keep these functions as virtual. However,\n> I know Laurent and others would have a preference of using static data where\n> possible. Perhaps just something to consider for the future rather than\n> doing it now.\n\nFor sensors that depart from a handful of standard models, I think\nvirtual functions make sense.\n\n> > >>> + double getGain(uint32_t gainCode) const;\n> > >>> +\n> > >>> +protected:\n> > >>> + enum AnalogueGainCapability : uint16_t {\n> > >>> + GlobalGain = 0,\n> > >>> + AlternateGlobalGain = 2,\n> > >>> + };\n> > >>> +\n> > >>> + struct analogueGainConstants {\n> > >>> + uint16_t type;\n> > >>> + int16_t m0;\n> > >>> + int16_t c0;\n> > >>> + int16_t m1;\n> > >>> + int16_t c1;\n> > >>> + };\n> > >>> + analogueGainConstants analogueGain_;\n> > >>> +\n> > >>> +private:\n> > >>> + std::string name_;\n> > >>> + friend class CameraSensorHelperFactory;\n> > >>> +};\n> > >>> +\n> > >>> +class CameraSensorHelperFactory\n> > >>> +{\n> > >>> +public:\n> > >>> + CameraSensorHelperFactory(const char *name);\n> > >>> + virtual ~CameraSensorHelperFactory() = default;\n> > >>> +\n> > >>> + std::unique_ptr create();\n> > >>> +\n> > >>> + const std::string &name() const { return name_; }\n> > >>> +\n> > >>> + static void registerType(CameraSensorHelperFactory *factory);\n> > >>> + static std::vector &factories();\n> > >>> +\n> > >>> +private:\n> > >>> + virtual CameraSensorHelper *createInstance() = 0;\n> > >>> +\n> > >>> + std::string name_;\n> > >>> +};\n> > >>> +\n> > >>> +#define REGISTER_CAMERA_SENSOR_HELPER(name, handler) \\\n> > >>> + class handler##Factory final : publicCameraSensorHelperFactory \\\n> > >>> + { \\\n> > >>> + public: \\\n> > >>> + handler##Factory() :CameraSensorHelperFactory(#handler) {} \\\n> > >>> + \\\n> > >>> + private: \\\n> > >>> + CameraSensorHelper *createInstance() \\\n> > >>> + { \\\n> > >>> + return new handler(name); \\\n> > >>> + } \\\n> > >>> + }; \\\n> > >>> + static handler##Factory global_##handler##Factory;\n> > >>> +\n> > >>> +} /* namespace ipa */\n> > >>> +\n> > >>> +} /* namespace libcamera */\n> > >>> +\n> > >>> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n> > >>> +\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> > >>>","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 C8ACBBD22E\n\tfor ;\n\tTue, 8 Jun 2021 18:56:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 282846892F;\n\tTue, 8 Jun 2021 20:56:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4579B602A7\n\tfor ;\n\tTue, 8 Jun 2021 20:56:28 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BCBF93E6;\n\tTue, 8 Jun 2021 20:56:27 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"D/Wzz4Q9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623178588;\n\tbh=dTNUy34F0kISL4VTTwqfRMACA2AsorZcG0BxTrKJrXs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=D/Wzz4Q9Wc2K/h/pbYJh2kFcApr67/PxizKNEDqewnJA9eBThGlfzQ519DPoD80LX\n\tdSGpqgE35r7hZJL4cnHlaZHl6cnn7v4/5Ilk48qHJMxTVivO1Q9OsqE1eTkQlx4NkY\n\t3NOa230m4CYB8mZJaVqO5kX0GETccPy2c4jAcbnc=","Date":"Tue, 8 Jun 2021 21:56:11 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210607123533.51198-1-jeanmichel.hautbois@ideasonboard.com>\n\t\n\t<46fb8e19-1c8e-e23e-6936-1f122781a6d6@ideasonboard.com>\n\t<130325f4-b459-40ab-0d7e-ef50eb9e5eb1@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]