[{"id":17512,"web_url":"https://patchwork.libcamera.org/comment/17512/","msgid":"<49bfb018-73a8-93fb-bbcd-64dcbd9541eb@ideasonboard.com>","date":"2021-06-11T09:53:52","subject":"Re: [libcamera-devel] [PATCH v3 1/2] ipa: Create a camera sensor\n\thelper class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi JM\n\nOn 11/06/2021 08:03, Jean-Michel Hautbois wrote:\n> Setting analogue gain for a specific sensor is not a straghtforward\n\ns/straghtforward/straightforward/\n\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\ns/straightfoward/straightforward/\n\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 | 319 ++++++++++++++++++++++++\n> src/ipa/libipa/camera_sensor_helper.h | 88 +++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 409 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..4e50ee53\n> --- /dev/null\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -0,0 +1,319 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * camera_sensor_helper.cpp - Helper class providing camera information\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 Helper class for each sensor\n> + *\n> + * Each camera sensor supported by libcamera may need specific informations to\n\ns/informations/information/\n\n\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 Compute sensor tuning parameters using sensor-specific constants\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.\n> + */\n> +CameraSensorHelper::CameraSensorHelper()\n> +{\n> +}\n> +\n> +CameraSensorHelper::~CameraSensorHelper()\n> +{\n> +}\n\nDo these need to be implemented if they're empty ? (Or are they\nplaceholders so they can be populated later? But if that's the case they\ncould be added when needed...\n\n> +\n> +/**\n> + * CameraSensorHelper::GainCode(double gain)\n> + * \\brief Compute gain code from the analogue gain value\n> + * \\param[in] gain The real gain to pass\n> + * \\return the gain code to pass to V4l2\n> + *\n> + * This function aims to abstract the calculation of the gain letting the IPA\n> + * use the real gain for its estimations.\n> + *\n> + * The parameters come from the MIPI Alliance Camera Specification for\n> + * Camera Command Set (CCS).\n> + */\n> +uint32_t CameraSensorHelper::GainCode(double gain) const\n> +{\n> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> +\t\n> +\treturn (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n> +\t (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n> +}\n> +\n> +/**\n> + * CameraSensorHelper::Gain\n> + * \\brief Compute the real gain from the V4l2 subdev control gain\n> + * \\param[in] gainCode The V4l2 subdev control gain\n> + * \\return The real gain\n> + *\n> + * This function aims to abstract the calculation of the gain letting the IPA\n> + * use the real gain for its estimations. It is the counterpart of the function\n> + * CameraSensorHelper::getGainCode.\n> + *\n> + * The parameters come from the MIPI Alliance Camera Specification for\n> + * Camera Command Set (CCS).\n> + */\n> +double CameraSensorHelper::Gain(uint32_t gainCode) const\n> +{\n> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> +\n> +\treturn (analogueGainConstants_.m0 * gainCode + analogueGainConstants_.c0) /\n> +\t (analogueGainConstants_.m1 * gainCode + analogueGainConstants_.c1);\n> +}\n> +\n> +/**\n> + * \\enum CameraSensorHelper::AnalogueGainType\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::AnalogueGainLinear\n> + * \\brief Sensor supports linear 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$gain=\\frac{m0x+c0}{m1x+c1}\\f$\n> + *\n> + * Where 'x' is the gain control parameter, and m0, m1, c0 and c1 are\n> + * image-sensor-specific constants 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$gain=\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n> + */\n> +\n> +/**\n> + * \\var CameraSensorHelper::AnalogueGainExponential\n\nThis mixes Analogue and Analog below. We should at least be consistent\nin the spelling type used.\n\nSame in many places.\n\n\n> + * \\brief Sensor supports exponential 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::analogueGainConstants_\n> + * \\brief The analogue gain parameters used for calculation\n> + *\n> + * The analogue gain is calculated through a formula, and its parameters are\n> + * sensor specific. Use this variable to store the values at init time.\n> + *\n> + */\n> +\n> +/**\n> + * \\class CameraSensorHelperFactory\n> + * \\brief Registration of CameraSensorHelperFactory classes and creation of instances\n> + *\n> + * To facilitate discovery and instantiation of CameraSensorHelper classes, the\n> + * CameraSensorHelperFactory class maintains a registry of camera sensor helper\n> + * classes. Each CameraSensorHelper subclass shall register itself using the\n> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a corresponding\n> + * instance of a CameraSensorHelperFactory subclass and register it with the\n> + * static list of factories.\n> + */\n> +\n> +/**\n> + * \\brief Construct a camera sensor helper factory\n> + * \\param[in] name Name of the camera sensor helper class\n> + *\n> + * Creating an instance of the factory registers it with the global list of\n> + * factories, accessible through the factories() function.\n> + *\n> + * The factory \\a name is used for debug purpose and shall be unique.\n> + */\n> +CameraSensorHelperFactory::CameraSensorHelperFactory(const std::string name)\n> +\t: name_(name)\n> +{\n> +\tregisterType(this);\n> +}\n> +\n> +/**\n> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n> + *\n> + * \\return A unique pointer to a new instance of the CameraSensorHelper subclass\n> + * corresponding to the factory\n> + */\n> +std::unique_ptr CameraSensorHelperFactory::create(const std::string &name)\n> +{\n> +\tstd::vector &factories =\n> +\t\tCameraSensorHelperFactory::factories();\n> +\n> +\tfor (CameraSensorHelperFactory *factory : factories) {\n> +\t\tif (name != factory->name_)\n> +\t\t\tcontinue;\n> +\n> +\t\tCameraSensorHelper *helper = factory->createInstance();\n> +\t\treturn std::unique_ptr(helper);\n> +\t}\n> +\treturn nullptr;\n> +}\n> +\n> +/**\n> + * \\brief Add a camera sensor helper class to the registry\n> + * \\param[in] factory Factory to use to construct the camera sensor helper\n> + *\n> + * The caller is responsible to guarantee the uniqueness of the camera sensor helper\n> + * name.\n> + */\n> +void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory *factory)\n> +{\n> +\tstd::vector &factories = CameraSensorHelperFactory::factories();\n> +\n> +\tfactories.push_back(factory);\n> +}\n> +\n> +/**\n> + * \\brief Retrieve the list of all camera sensor helper factories\n> + *\n> + * The static factories map is defined inside the function to ensures it gets\n> + * initialized on first use, without any dependency on link order.\n> + *\n> + * \\return The list of camera sensor helper factories\n> + */\n> +std::vector &CameraSensorHelperFactory::factories()\n> +{\n> +\tstatic std::vector factories;\n> +\treturn factories;\n> +}\n> +\n> +/**\n> + * CameraSensorHelperFactory::createInstance()\n> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n> + *\n> + * This virtual function is implemented by the REGISTER_CAMERA_SENSOR_HELPER()\n> + * macro. It creates a camera sensor helper instance associated with the camera\n> + * sensor model.\n> + *\n> + * \\return A pointer to a newly constructed instance of the CameraSensorHelper\n> + * subclass corresponding to the factory\n> + */\n> +\n> +/**\n> + * \\def REGISTER_CAMERA_SENSOR_HELPER\n> + * \\brief Register a camera sensor helper with the camera sensor helper factory\n> + * \\param[in] name Sensor model name used to register the class\n> + * \\param[in] helper Class name of CameraSensorHelper derived class to register\n> + *\n> + * Register a CameraSensorHelper subclass with the factory and make it available to\n> + * try and match devices.\n> + */\n> +\n> +/**\n> + * \\class CameraSensorHelperImx219\n> + * \\brief Create and give helpers for the imx219 sensor\n> + */\n> +class CameraSensorHelperImx219 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperImx219()\n> +\t{\n> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 0, -1, 256, 256 };\n> +\t}\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> +\n> +/**\n> + * \\class CameraSensorHelperOv5670\n> + * \\brief Create and give helpers for the ov5670 sensor\n> + */\n> +class CameraSensorHelperOv5670 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperOv5670()\n> +\t{\n> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 };\n> +\t}\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n> +\n> +/**\n> + * \\class CameraSensorHelperOv5693\n> + * \\brief Create and give helpers for the ov5693 sensor\n> + */\n> +class CameraSensorHelperOv5693 : public CameraSensorHelper\n> +{\n> +public:\n> +\tCameraSensorHelperOv5693()\n> +\t{\n> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 16 };\n> +\t}\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n\n\nThis is a lot of code for what so far is a table. I assume there will be\nmore complex requirements on the class though so I think it's ok.\n\n\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> new file mode 100644\n> index 00000000..cc486be8\n> --- /dev/null\n> +++ b/src/ipa/libipa/camera_sensor_helper.h\n> @@ -0,0 +1,88 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n\nWhy is this BSD-2 instead of LGPL-2.1-or-later?\n\nIs this specifically derived from RPi code?\nIn which case should that be mentioned in the Copyright?\n\nSame for the .cpp I think.\n\n\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * camera_sensor_helper.h - Helper class providing camera information\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\n> +{\n> +public:\n> +\tCameraSensorHelper();\n> +\tvirtual ~CameraSensorHelper();\n> +\n> +\tuint32_t GainCode(double gain) const;\n> +\tdouble Gain(uint32_t gainCode) const;\n> +\n> +protected:\n> +\tenum AnalogueGainType {\n> +\t\tAnalogueGainLinear = 0,\n> +\t\tAnalogueGainExponential = 2,\n> +\t};\n> +\n> +\tstruct AnalogueGainConstants {\n> +\t\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> +\n> +\tAnalogueGainConstants analogueGainConstants_;\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)\n> +};\n> +\n> +class CameraSensorHelperFactory\n> +{\n> +public:\n> +\tCameraSensorHelperFactory(const std::string name);\n> +\tvirtual ~CameraSensorHelperFactory() = default;\n> +\n> +\tstatic std::unique_ptr create(const std::string &name);\n> +\n> +\tstatic void registerType(CameraSensorHelperFactory *factory);\n> +\tstatic std::vector &factories();\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)\n> +\tvirtual CameraSensorHelper *createInstance() = 0;\n> +\n> +\tstd::string name_;\n> +};\n> +\n> +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper) \\\n> +class helper##Factory final : public CameraSensorHelperFactory \\\n> +{ \\\n> +public: \\\n> +\thelper##Factory() : CameraSensorHelperFactory(name) {} \\\n> +\t\t\t\t\t\t\t\t \\\n> +private: \\\n> +\tCameraSensorHelper *createInstance() \\\n> +\t{ \\\n> +\t\treturn new helper(); \\\n> +\t} \\\n> +}; \\\n> +static helper##Factory global_##helper##Factory;\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 038fc490..dc90542c 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -2,11 +2,13 @@\n> \n> libipa_headers = files([\n> 'algorithm.h',\n> + 'camera_sensor_helper.h',\n> 'histogram.h'\n> ])\n> \n> libipa_sources = files([\n> 'algorithm.cpp',\n> + 'camera_sensor_helper.cpp',\n> 'histogram.cpp'\n> ])\n> \n>","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 BC89EBD78E\n\tfor ;\n\tFri, 11 Jun 2021 09:53:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0D1CD6892E;\n\tFri, 11 Jun 2021 11:53:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E03E96029D\n\tfor ;\n\tFri, 11 Jun 2021 11:53:55 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4EE0E4AD;\n\tFri, 11 Jun 2021 11:53: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=\"s/pipajq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623405235;\n\tbh=syQuRdQOL+hn1ZZr4ktadOt8LLu1e1Icu4t4DuNcn34=;\n\th=Reply-To:To:References:From:Subject:Date:In-Reply-To:From;\n\tb=s/pipajq+CCK3hz8SBzsfHx1pU12agMDVcA9ddvGDUj6C0JFW4aFOryC1zHqv09Lh\n\tP6kGKQkuPdhAWbFvFdEeyqAFIT1UCafifDtL0fsZn5VASzP0ecHMxBqpcjQNuDiTOr\n\tnd9r3XraXc/h4pIb8YB9f7YtILEklCt7Aw8TEgcA=","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210611070311.12080-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210611070311.12080-2-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"<49bfb018-73a8-93fb-bbcd-64dcbd9541eb@ideasonboard.com>","Date":"Fri, 11 Jun 2021 10:53:52 +0100","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":"<20210611070311.12080-2-jeanmichel.hautbois@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3 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","Reply-To":"kieran.bingham@ideasonboard.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17522,"web_url":"https://patchwork.libcamera.org/comment/17522/","msgid":"","date":"2021-06-14T07:51:01","subject":"Re: [libcamera-devel] [PATCH v3 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 Kieran,\n\nOn 11/06/2021 11:53, Kieran Bingham wrote:\n> Hi JM\n> \n> On 11/06/2021 08:03, Jean-Michel Hautbois wrote:\n>> Setting analogue gain for a specific sensor is not a straghtforward\n> \n> s/straghtforward/straightforward/\n> \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> \n> s/straightfoward/straightforward/\n> \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 | 319 ++++++++++++++++++++++++\n>> src/ipa/libipa/camera_sensor_helper.h | 88 +++++++\n>> src/ipa/libipa/meson.build | 2 +\n>> 3 files changed, 409 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..4e50ee53\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n>> @@ -0,0 +1,319 @@\n>> +/* SPDX-License-Identifier: BSD-2-Clause */\n>> +/*\n>> + * Copyright (C) 2021, Google Inc.\n>> + *\n>> + * camera_sensor_helper.cpp - Helper class providing camera information\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 Helper class for each sensor\n>> + *\n>> + * Each camera sensor supported by libcamera may need specific informations to\n> \n> s/informations/information/\n> \n> \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 Compute sensor tuning parameters using sensor-specific constants\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.\n>> + */\n>> +CameraSensorHelper::CameraSensorHelper()\n>> +{\n>> +}\n>> +\n>> +CameraSensorHelper::~CameraSensorHelper()\n>> +{\n>> +}\n> \n> Do these need to be implemented if they're empty ? (Or are they\n> placeholders so they can be populated later? But if that's the case they\n> could be added when needed...\n> \n>> +\n>> +/**\n>> + * CameraSensorHelper::GainCode(double gain)\n>> + * \\brief Compute gain code from the analogue gain value\n>> + * \\param[in] gain The real gain to pass\n>> + * \\return the gain code to pass to V4l2\n>> + *\n>> + * This function aims to abstract the calculation of the gain letting the IPA\n>> + * use the real gain for its estimations.\n>> + *\n>> + * The parameters come from the MIPI Alliance Camera Specification for\n>> + * Camera Command Set (CCS).\n>> + */\n>> +uint32_t CameraSensorHelper::GainCode(double gain) const\n>> +{\n>> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n>> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n>> +\t\n>> +\treturn (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n>> +\t (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n>> +}\n>> +\n>> +/**\n>> + * CameraSensorHelper::Gain\n>> + * \\brief Compute the real gain from the V4l2 subdev control gain\n>> + * \\param[in] gainCode The V4l2 subdev control gain\n>> + * \\return The real gain\n>> + *\n>> + * This function aims to abstract the calculation of the gain letting the IPA\n>> + * use the real gain for its estimations. It is the counterpart of the function\n>> + * CameraSensorHelper::getGainCode.\n>> + *\n>> + * The parameters come from the MIPI Alliance Camera Specification for\n>> + * Camera Command Set (CCS).\n>> + */\n>> +double CameraSensorHelper::Gain(uint32_t gainCode) const\n>> +{\n>> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n>> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n>> +\n>> +\treturn (analogueGainConstants_.m0 * gainCode + analogueGainConstants_.c0) /\n>> +\t (analogueGainConstants_.m1 * gainCode + analogueGainConstants_.c1);\n>> +}\n>> +\n>> +/**\n>> + * \\enum CameraSensorHelper::AnalogueGainType\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::AnalogueGainLinear\n>> + * \\brief Sensor supports linear 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$gain=\\frac{m0x+c0}{m1x+c1}\\f$\n>> + *\n>> + * Where 'x' is the gain control parameter, and m0, m1, c0 and c1 are\n>> + * image-sensor-specific constants 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$gain=\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n>> + */\n>> +\n>> +/**\n>> + * \\var CameraSensorHelper::AnalogueGainExponential\n> \n> This mixes Analogue and Analog below. We should at least be consistent\n> in the spelling type used.\n> \n> Same in many places.\n> \n> \n>> + * \\brief Sensor supports exponential 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::analogueGainConstants_\n>> + * \\brief The analogue gain parameters used for calculation\n>> + *\n>> + * The analogue gain is calculated through a formula, and its parameters are\n>> + * sensor specific. Use this variable to store the values at init time.\n>> + *\n>> + */\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperFactory\n>> + * \\brief Registration of CameraSensorHelperFactory classes and creation of instances\n>> + *\n>> + * To facilitate discovery and instantiation of CameraSensorHelper classes, the\n>> + * CameraSensorHelperFactory class maintains a registry of camera sensor helper\n>> + * classes. Each CameraSensorHelper subclass shall register itself using the\n>> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a corresponding\n>> + * instance of a CameraSensorHelperFactory subclass and register it with the\n>> + * static list of factories.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct a camera sensor helper factory\n>> + * \\param[in] name Name of the camera sensor helper class\n>> + *\n>> + * Creating an instance of the factory registers it with the global list of\n>> + * factories, accessible through the factories() function.\n>> + *\n>> + * The factory \\a name is used for debug purpose and shall be unique.\n>> + */\n>> +CameraSensorHelperFactory::CameraSensorHelperFactory(const std::string name)\n>> +\t: name_(name)\n>> +{\n>> +\tregisterType(this);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n>> + *\n>> + * \\return A unique pointer to a new instance of the CameraSensorHelper subclass\n>> + * corresponding to the factory\n>> + */\n>> +std::unique_ptr CameraSensorHelperFactory::create(const std::string &name)\n>> +{\n>> +\tstd::vector &factories =\n>> +\t\tCameraSensorHelperFactory::factories();\n>> +\n>> +\tfor (CameraSensorHelperFactory *factory : factories) {\n>> +\t\tif (name != factory->name_)\n>> +\t\t\tcontinue;\n>> +\n>> +\t\tCameraSensorHelper *helper = factory->createInstance();\n>> +\t\treturn std::unique_ptr(helper);\n>> +\t}\n>> +\treturn nullptr;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Add a camera sensor helper class to the registry\n>> + * \\param[in] factory Factory to use to construct the camera sensor helper\n>> + *\n>> + * The caller is responsible to guarantee the uniqueness of the camera sensor helper\n>> + * name.\n>> + */\n>> +void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory *factory)\n>> +{\n>> +\tstd::vector &factories = CameraSensorHelperFactory::factories();\n>> +\n>> +\tfactories.push_back(factory);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Retrieve the list of all camera sensor helper factories\n>> + *\n>> + * The static factories map is defined inside the function to ensures it gets\n>> + * initialized on first use, without any dependency on link order.\n>> + *\n>> + * \\return The list of camera sensor helper factories\n>> + */\n>> +std::vector &CameraSensorHelperFactory::factories()\n>> +{\n>> +\tstatic std::vector factories;\n>> +\treturn factories;\n>> +}\n>> +\n>> +/**\n>> + * CameraSensorHelperFactory::createInstance()\n>> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n>> + *\n>> + * This virtual function is implemented by the REGISTER_CAMERA_SENSOR_HELPER()\n>> + * macro. It creates a camera sensor helper instance associated with the camera\n>> + * sensor model.\n>> + *\n>> + * \\return A pointer to a newly constructed instance of the CameraSensorHelper\n>> + * subclass corresponding to the factory\n>> + */\n>> +\n>> +/**\n>> + * \\def REGISTER_CAMERA_SENSOR_HELPER\n>> + * \\brief Register a camera sensor helper with the camera sensor helper factory\n>> + * \\param[in] name Sensor model name used to register the class\n>> + * \\param[in] helper Class name of CameraSensorHelper derived class to register\n>> + *\n>> + * Register a CameraSensorHelper subclass with the factory and make it available to\n>> + * try and match devices.\n>> + */\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperImx219\n>> + * \\brief Create and give helpers for the imx219 sensor\n>> + */\n>> +class CameraSensorHelperImx219 : public CameraSensorHelper\n>> +{\n>> +public:\n>> +\tCameraSensorHelperImx219()\n>> +\t{\n>> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 0, -1, 256, 256 };\n>> +\t}\n>> +};\n>> +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperOv5670\n>> + * \\brief Create and give helpers for the ov5670 sensor\n>> + */\n>> +class CameraSensorHelperOv5670 : public CameraSensorHelper\n>> +{\n>> +public:\n>> +\tCameraSensorHelperOv5670()\n>> +\t{\n>> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 };\n>> +\t}\n>> +};\n>> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n>> +\n>> +/**\n>> + * \\class CameraSensorHelperOv5693\n>> + * \\brief Create and give helpers for the ov5693 sensor\n>> + */\n>> +class CameraSensorHelperOv5693 : public CameraSensorHelper\n>> +{\n>> +public:\n>> +\tCameraSensorHelperOv5693()\n>> +\t{\n>> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 16 };\n>> +\t}\n>> +};\n>> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> \n> \n> This is a lot of code for what so far is a table. I assume there will be\n> more complex requirements on the class though so I think it's ok.\n> \n\nThanks for all the remarks :-).\nIndeed the helper aims to do more than just calculate an analogue gain :-).\n\n> \n>> +\n>> +} /* namespace ipa */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n>> new file mode 100644\n>> index 00000000..cc486be8\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/camera_sensor_helper.h\n>> @@ -0,0 +1,88 @@\n>> +/* SPDX-License-Identifier: BSD-2-Clause */\n> \n> Why is this BSD-2 instead of LGPL-2.1-or-later?\n> \n> Is this specifically derived from RPi code?\n> In which case should that be mentioned in the Copyright?\n> \n> Same for the .cpp I think.\n> \n\nIt is more coming from PipelineHandler :-).\nI changed it to LGPL-2.1-or-later to be consistent with the other IPA\nclasses.\n\n> \n>> +/*\n>> + * Copyright (C) 2021, Google Inc.\n>> + *\n>> + * camera_sensor_helper.h - Helper class providing camera information\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\n>> +{\n>> +public:\n>> +\tCameraSensorHelper();\n>> +\tvirtual ~CameraSensorHelper();\n>> +\n>> +\tuint32_t GainCode(double gain) const;\n>> +\tdouble Gain(uint32_t gainCode) const;\n>> +\n>> +protected:\n>> +\tenum AnalogueGainType {\n>> +\t\tAnalogueGainLinear = 0,\n>> +\t\tAnalogueGainExponential = 2,\n>> +\t};\n>> +\n>> +\tstruct AnalogueGainConstants {\n>> +\t\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>> +\n>> +\tAnalogueGainConstants analogueGainConstants_;\n>> +\n>> +private:\n>> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)\n>> +};\n>> +\n>> +class CameraSensorHelperFactory\n>> +{\n>> +public:\n>> +\tCameraSensorHelperFactory(const std::string name);\n>> +\tvirtual ~CameraSensorHelperFactory() = default;\n>> +\n>> +\tstatic std::unique_ptr create(const std::string &name);\n>> +\n>> +\tstatic void registerType(CameraSensorHelperFactory *factory);\n>> +\tstatic std::vector &factories();\n>> +\n>> +private:\n>> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)\n>> +\tvirtual CameraSensorHelper *createInstance() = 0;\n>> +\n>> +\tstd::string name_;\n>> +};\n>> +\n>> +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper) \\\n>> +class helper##Factory final : public CameraSensorHelperFactory \\\n>> +{ \\\n>> +public: \\\n>> +\thelper##Factory() : CameraSensorHelperFactory(name) {} \\\n>> +\t\t\t\t\t\t\t\t \\\n>> +private: \\\n>> +\tCameraSensorHelper *createInstance() \\\n>> +\t{ \\\n>> +\t\treturn new helper(); \\\n>> +\t} \\\n>> +}; \\\n>> +static helper##Factory global_##helper##Factory;\n>> +\n>> +} /* namespace ipa */\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n>> index 038fc490..dc90542c 100644\n>> --- a/src/ipa/libipa/meson.build\n>> +++ b/src/ipa/libipa/meson.build\n>> @@ -2,11 +2,13 @@\n>> \n>> libipa_headers = files([\n>> 'algorithm.h',\n>> + 'camera_sensor_helper.h',\n>> 'histogram.h'\n>> ])\n>> \n>> libipa_sources = files([\n>> 'algorithm.cpp',\n>> + 'camera_sensor_helper.cpp',\n>> 'histogram.cpp'\n>> ])\n>> \n>>\n>","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 9D2BBC3216\n\tfor ;\n\tMon, 14 Jun 2021 07:51:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B53CF68934;\n\tMon, 14 Jun 2021 09:51:05 +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 CCFF56029A\n\tfor ;\n\tMon, 14 Jun 2021 09:51:02 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:109a:9a37:eaa0:66eb])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 505EC8C4;\n\tMon, 14 Jun 2021 09:51:02 +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=\"NC0AJ35b\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623657062;\n\tbh=Q8g7oY/x1nyV/oEZvjlug2EC1fC9t2cQ3aFb39PDgP4=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=NC0AJ35brpAi2PXdRPg5MJUWwk30Cp+TAgcmweW9Nme6zTOv69KEAnwznr9LWtbuI\n\tjWXBXSdvI7xdNInoG0BtDNw9Jay+PFGdUv1X7FIJLj2nWHb4p9DZH770+RUGIJSCDR\n\tcXShkZg6SFagssvAtI7WNGT7yWCEMo9PhZ1j5bw4=","To":"kieran.bingham@ideasonboard.com,\n\tJean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210611070311.12080-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210611070311.12080-2-jeanmichel.hautbois@ideasonboard.com>\n\t<49bfb018-73a8-93fb-bbcd-64dcbd9541eb@ideasonboard.com>","From":"Jean-Michel Hautbois ","Message-ID":"","Date":"Mon, 14 Jun 2021 09:51:01 +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":"<49bfb018-73a8-93fb-bbcd-64dcbd9541eb@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17523,"web_url":"https://patchwork.libcamera.org/comment/17523/","msgid":"<327d46a7-e356-5f35-063b-9f41034cc087@ideasonboard.com>","date":"2021-06-14T07:59:25","subject":"Re: [libcamera-devel] [PATCH v3 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":"On 14/06/2021 09:51, Jean-Michel Hautbois wrote:\n> Hi Kieran,\n> \n> On 11/06/2021 11:53, Kieran Bingham wrote:\n>> Hi JM\n>>\n>> On 11/06/2021 08:03, Jean-Michel Hautbois wrote:\n>>> Setting analogue gain for a specific sensor is not a straghtforward\n>>\n>> s/straghtforward/straightforward/\n>>\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>>\n>> s/straightfoward/straightforward/\n>>\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 | 319 ++++++++++++++++++++++++\n>>> src/ipa/libipa/camera_sensor_helper.h | 88 +++++++\n>>> src/ipa/libipa/meson.build | 2 +\n>>> 3 files changed, 409 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..4e50ee53\n>>> --- /dev/null\n>>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n>>> @@ -0,0 +1,319 @@\n>>> +/* SPDX-License-Identifier: BSD-2-Clause */\n>>> +/*\n>>> + * Copyright (C) 2021, Google Inc.\n>>> + *\n>>> + * camera_sensor_helper.cpp - Helper class providing camera information\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 Helper class for each sensor\n>>> + *\n>>> + * Each camera sensor supported by libcamera may need specific informations to\n>>\n>> s/informations/information/\n>>\n>>\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 Compute sensor tuning parameters using sensor-specific constants\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.\n>>> + */\n>>> +CameraSensorHelper::CameraSensorHelper()\n>>> +{\n>>> +}\n>>> +\n>>> +CameraSensorHelper::~CameraSensorHelper()\n>>> +{\n>>> +}\n>>\n>> Do these need to be implemented if they're empty ? (Or are they\n>> placeholders so they can be populated later? But if that's the case they\n>> could be added when needed...\n>>\n\nI think you need those as you call them through\nCameraSensorHelperFactory, and if you don't, then you will have\nsomething like :\nsymbol lookup error: /usr/lib/x86_64-linux-gnu/libcamera/ipa_ipu3.so:\nundefined symbol:\nlibcamera::ipa::CameraSensorHelperFactory::create(std::__1::basic_string, std::__1::allocator > const&)\n\nEven if I suppose we may have them declared by another mean ?\n\n>>> +\n>>> +/**\n>>> + * CameraSensorHelper::GainCode(double gain)\n>>> + * \\brief Compute gain code from the analogue gain value\n>>> + * \\param[in] gain The real gain to pass\n>>> + * \\return the gain code to pass to V4l2\n>>> + *\n>>> + * This function aims to abstract the calculation of the gain letting the IPA\n>>> + * use the real gain for its estimations.\n>>> + *\n>>> + * The parameters come from the MIPI Alliance Camera Specification for\n>>> + * Camera Command Set (CCS).\n>>> + */\n>>> +uint32_t CameraSensorHelper::GainCode(double gain) const\n>>> +{\n>>> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n>>> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n>>> +\t\n>>> +\treturn (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n>>> +\t (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n>>> +}\n>>> +\n>>> +/**\n>>> + * CameraSensorHelper::Gain\n>>> + * \\brief Compute the real gain from the V4l2 subdev control gain\n>>> + * \\param[in] gainCode The V4l2 subdev control gain\n>>> + * \\return The real gain\n>>> + *\n>>> + * This function aims to abstract the calculation of the gain letting the IPA\n>>> + * use the real gain for its estimations. It is the counterpart of the function\n>>> + * CameraSensorHelper::getGainCode.\n>>> + *\n>>> + * The parameters come from the MIPI Alliance Camera Specification for\n>>> + * Camera Command Set (CCS).\n>>> + */\n>>> +double CameraSensorHelper::Gain(uint32_t gainCode) const\n>>> +{\n>>> +\tASSERT((analogueGainConstants_.m0 == 0) || (analogueGainConstants_.m1 == 0));\n>>> +\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n>>> +\n>>> +\treturn (analogueGainConstants_.m0 * gainCode + analogueGainConstants_.c0) /\n>>> +\t (analogueGainConstants_.m1 * gainCode + analogueGainConstants_.c1);\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\enum CameraSensorHelper::AnalogueGainType\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::AnalogueGainLinear\n>>> + * \\brief Sensor supports linear 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$gain=\\frac{m0x+c0}{m1x+c1}\\f$\n>>> + *\n>>> + * Where 'x' is the gain control parameter, and m0, m1, c0 and c1 are\n>>> + * image-sensor-specific constants 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$gain=\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\var CameraSensorHelper::AnalogueGainExponential\n>>\n>> This mixes Analogue and Analog below. We should at least be consistent\n>> in the spelling type used.\n>>\n>> Same in many places.\n>>\n>>\n>>> + * \\brief Sensor supports exponential 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::analogueGainConstants_\n>>> + * \\brief The analogue gain parameters used for calculation\n>>> + *\n>>> + * The analogue gain is calculated through a formula, and its parameters are\n>>> + * sensor specific. Use this variable to store the values at init time.\n>>> + *\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\class CameraSensorHelperFactory\n>>> + * \\brief Registration of CameraSensorHelperFactory classes and creation of instances\n>>> + *\n>>> + * To facilitate discovery and instantiation of CameraSensorHelper classes, the\n>>> + * CameraSensorHelperFactory class maintains a registry of camera sensor helper\n>>> + * classes. Each CameraSensorHelper subclass shall register itself using the\n>>> + * REGISTER_CAMERA_SENSOR_HELPER() macro, which will create a corresponding\n>>> + * instance of a CameraSensorHelperFactory subclass and register it with the\n>>> + * static list of factories.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\brief Construct a camera sensor helper factory\n>>> + * \\param[in] name Name of the camera sensor helper class\n>>> + *\n>>> + * Creating an instance of the factory registers it with the global list of\n>>> + * factories, accessible through the factories() function.\n>>> + *\n>>> + * The factory \\a name is used for debug purpose and shall be unique.\n>>> + */\n>>> +CameraSensorHelperFactory::CameraSensorHelperFactory(const std::string name)\n>>> +\t: name_(name)\n>>> +{\n>>> +\tregisterType(this);\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n>>> + *\n>>> + * \\return A unique pointer to a new instance of the CameraSensorHelper subclass\n>>> + * corresponding to the factory\n>>> + */\n>>> +std::unique_ptr CameraSensorHelperFactory::create(const std::string &name)\n>>> +{\n>>> +\tstd::vector &factories =\n>>> +\t\tCameraSensorHelperFactory::factories();\n>>> +\n>>> +\tfor (CameraSensorHelperFactory *factory : factories) {\n>>> +\t\tif (name != factory->name_)\n>>> +\t\t\tcontinue;\n>>> +\n>>> +\t\tCameraSensorHelper *helper = factory->createInstance();\n>>> +\t\treturn std::unique_ptr(helper);\n>>> +\t}\n>>> +\treturn nullptr;\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Add a camera sensor helper class to the registry\n>>> + * \\param[in] factory Factory to use to construct the camera sensor helper\n>>> + *\n>>> + * The caller is responsible to guarantee the uniqueness of the camera sensor helper\n>>> + * name.\n>>> + */\n>>> +void CameraSensorHelperFactory::registerType(CameraSensorHelperFactory *factory)\n>>> +{\n>>> +\tstd::vector &factories = CameraSensorHelperFactory::factories();\n>>> +\n>>> +\tfactories.push_back(factory);\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\brief Retrieve the list of all camera sensor helper factories\n>>> + *\n>>> + * The static factories map is defined inside the function to ensures it gets\n>>> + * initialized on first use, without any dependency on link order.\n>>> + *\n>>> + * \\return The list of camera sensor helper factories\n>>> + */\n>>> +std::vector &CameraSensorHelperFactory::factories()\n>>> +{\n>>> +\tstatic std::vector factories;\n>>> +\treturn factories;\n>>> +}\n>>> +\n>>> +/**\n>>> + * CameraSensorHelperFactory::createInstance()\n>>> + * \\brief Create an instance of the CameraSensorHelper corresponding to the factory\n>>> + *\n>>> + * This virtual function is implemented by the REGISTER_CAMERA_SENSOR_HELPER()\n>>> + * macro. It creates a camera sensor helper instance associated with the camera\n>>> + * sensor model.\n>>> + *\n>>> + * \\return A pointer to a newly constructed instance of the CameraSensorHelper\n>>> + * subclass corresponding to the factory\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\def REGISTER_CAMERA_SENSOR_HELPER\n>>> + * \\brief Register a camera sensor helper with the camera sensor helper factory\n>>> + * \\param[in] name Sensor model name used to register the class\n>>> + * \\param[in] helper Class name of CameraSensorHelper derived class to register\n>>> + *\n>>> + * Register a CameraSensorHelper subclass with the factory and make it available to\n>>> + * try and match devices.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\class CameraSensorHelperImx219\n>>> + * \\brief Create and give helpers for the imx219 sensor\n>>> + */\n>>> +class CameraSensorHelperImx219 : public CameraSensorHelper\n>>> +{\n>>> +public:\n>>> +\tCameraSensorHelperImx219()\n>>> +\t{\n>>> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 0, -1, 256, 256 };\n>>> +\t}\n>>> +};\n>>> +REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n>>> +\n>>> +/**\n>>> + * \\class CameraSensorHelperOv5670\n>>> + * \\brief Create and give helpers for the ov5670 sensor\n>>> + */\n>>> +class CameraSensorHelperOv5670 : public CameraSensorHelper\n>>> +{\n>>> +public:\n>>> +\tCameraSensorHelperOv5670()\n>>> +\t{\n>>> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 };\n>>> +\t}\n>>> +};\n>>> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n>>> +\n>>> +/**\n>>> + * \\class CameraSensorHelperOv5693\n>>> + * \\brief Create and give helpers for the ov5693 sensor\n>>> + */\n>>> +class CameraSensorHelperOv5693 : public CameraSensorHelper\n>>> +{\n>>> +public:\n>>> +\tCameraSensorHelperOv5693()\n>>> +\t{\n>>> +\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 16 };\n>>> +\t}\n>>> +};\n>>> +REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n>>\n>>\n>> This is a lot of code for what so far is a table. I assume there will be\n>> more complex requirements on the class though so I think it's ok.\n>>\n> \n> Thanks for all the remarks :-).\n> Indeed the helper aims to do more than just calculate an analogue gain :-).\n> \n>>\n>>> +\n>>> +} /* namespace ipa */\n>>> +\n>>> +} /* namespace libcamera */\n>>> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n>>> new file mode 100644\n>>> index 00000000..cc486be8\n>>> --- /dev/null\n>>> +++ b/src/ipa/libipa/camera_sensor_helper.h\n>>> @@ -0,0 +1,88 @@\n>>> +/* SPDX-License-Identifier: BSD-2-Clause */\n>>\n>> Why is this BSD-2 instead of LGPL-2.1-or-later?\n>>\n>> Is this specifically derived from RPi code?\n>> In which case should that be mentioned in the Copyright?\n>>\n>> Same for the .cpp I think.\n>>\n> \n> It is more coming from PipelineHandler :-).\n> I changed it to LGPL-2.1-or-later to be consistent with the other IPA\n> classes.\n> \n>>\n>>> +/*\n>>> + * Copyright (C) 2021, Google Inc.\n>>> + *\n>>> + * camera_sensor_helper.h - Helper class providing camera information\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\n>>> +{\n>>> +public:\n>>> +\tCameraSensorHelper();\n>>> +\tvirtual ~CameraSensorHelper();\n>>> +\n>>> +\tuint32_t GainCode(double gain) const;\n>>> +\tdouble Gain(uint32_t gainCode) const;\n>>> +\n>>> +protected:\n>>> +\tenum AnalogueGainType {\n>>> +\t\tAnalogueGainLinear = 0,\n>>> +\t\tAnalogueGainExponential = 2,\n>>> +\t};\n>>> +\n>>> +\tstruct AnalogueGainConstants {\n>>> +\t\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>>> +\n>>> +\tAnalogueGainConstants analogueGainConstants_;\n>>> +\n>>> +private:\n>>> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)\n>>> +};\n>>> +\n>>> +class CameraSensorHelperFactory\n>>> +{\n>>> +public:\n>>> +\tCameraSensorHelperFactory(const std::string name);\n>>> +\tvirtual ~CameraSensorHelperFactory() = default;\n>>> +\n>>> +\tstatic std::unique_ptr create(const std::string &name);\n>>> +\n>>> +\tstatic void registerType(CameraSensorHelperFactory *factory);\n>>> +\tstatic std::vector &factories();\n>>> +\n>>> +private:\n>>> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelperFactory)\n>>> +\tvirtual CameraSensorHelper *createInstance() = 0;\n>>> +\n>>> +\tstd::string name_;\n>>> +};\n>>> +\n>>> +#define REGISTER_CAMERA_SENSOR_HELPER(name, helper) \\\n>>> +class helper##Factory final : public CameraSensorHelperFactory \\\n>>> +{ \\\n>>> +public: \\\n>>> +\thelper##Factory() : CameraSensorHelperFactory(name) {} \\\n>>> +\t\t\t\t\t\t\t\t \\\n>>> +private: \\\n>>> +\tCameraSensorHelper *createInstance() \\\n>>> +\t{ \\\n>>> +\t\treturn new helper(); \\\n>>> +\t} \\\n>>> +}; \\\n>>> +static helper##Factory global_##helper##Factory;\n>>> +\n>>> +} /* namespace ipa */\n>>> +\n>>> +} /* namespace libcamera */\n>>> +\n>>> +#endif /* __LIBCAMERA_IPA_LIBIPA_CAMERA_SENSOR_HELPER_H__ */\n>>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n>>> index 038fc490..dc90542c 100644\n>>> --- a/src/ipa/libipa/meson.build\n>>> +++ b/src/ipa/libipa/meson.build\n>>> @@ -2,11 +2,13 @@\n>>> \n>>> libipa_headers = files([\n>>> 'algorithm.h',\n>>> + 'camera_sensor_helper.h',\n>>> 'histogram.h'\n>>> ])\n>>> \n>>> libipa_sources = files([\n>>> 'algorithm.cpp',\n>>> + 'camera_sensor_helper.cpp',\n>>> 'histogram.cpp'\n>>> ])\n>>> \n>>>\n>>","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 DF672BD78E\n\tfor ;\n\tMon, 14 Jun 2021 07:59:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EB20568931;\n\tMon, 14 Jun 2021 09:59:27 +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 A61C26029A\n\tfor ;\n\tMon, 14 Jun 2021 09:59:26 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:109a:9a37:eaa0:66eb])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 26A8F8C4;\n\tMon, 14 Jun 2021 09:59:26 +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=\"haIt7wzd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623657566;\n\tbh=KDv0cQ98Fpo2zNmG19CxflGIbBHk+AiI9TV77nvef9g=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=haIt7wzdnJYHHCRKmJ9eVVAHxPYu7fWL3uuaWkOteSfYznkxJ3kwOhH55NqBiy6mV\n\trK++qrzr23b3rSIZG+XWmNquD8+t7LPEghwP2mcHcszmMBPYn/lrHEuj7Lxmp0Adql\n\t8Y1QF2K0uP01EjJZUyPgo4MhE0NKwMBd2vn+rmtE=","To":"Jean-Michel Hautbois ,\n\tkieran.bingham@ideasonboard.com, libcamera-devel@lists.libcamera.org","References":"<20210611070311.12080-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20210611070311.12080-2-jeanmichel.hautbois@ideasonboard.com>\n\t<49bfb018-73a8-93fb-bbcd-64dcbd9541eb@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","Message-ID":"<327d46a7-e356-5f35-063b-9f41034cc087@ideasonboard.com>","Date":"Mon, 14 Jun 2021 09:59:25 +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":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]