[{"id":13876,"web_url":"https://patchwork.libcamera.org/comment/13876/","msgid":"<20201125105830.obj6l43rkypgioif@uno.localdomain>","date":"2020-11-25T10:58:30","subject":"Re: [libcamera-devel] [RFC PATCH 8/8] libcamera: camera_sensor:\n\tParse configuration properties","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Mon, Nov 23, 2020 at 04:43:19PM +0000, Kieran Bingham wrote:\n> Search for a camera_sensor.json configuration file specific to libcamera\n> and parse it to see if any properties specific to this CameraSensor are\n> applicable.\n\nFor sake of discussion on the camera sensor database.\n\nI don't think this should be a system-wide configuration file but\nrather part of the libcamera sources, with a known path [1] It will\ncontain details on the sensor that we cannot retrieve easily from the\nkernel interface, or that are not even exposed by it, even if we\nshould keep pushing to get most of those information from a more and\nmore expressive v4l2 subdev interface.\n\nCurrently we have information that we cannot retrieve from the kernel\nthough. To name a few I can think of\n- Colour Filter Arrangement (even if it could be deduced from the RAW\n format components sequence)\n- Control delays (see Niklas' delayed controls and RPi's\n StaggeredControls)\n- Color tuning information and sensitivites (white/black levels ? Gain\n factors ? Probably we have a V4L2 control for that).\n\nIdeally, we should be able to push all of them into the kernel, but\nit's a long process. Having a sensor database helps in that regard,\nbut makes less pressing the need for decent sensor drivers. One\nexample is all the size related information. We -could- get most of\nthem through the V4L2 Selection API, but only a few driver actually\nexpose them correctly. Recording them in userspace has anyway value,\nso I'm a little debated.\n\nAlso, if we have a 'camera module' to deal with, the sensor database\n(which is not only about sensor anymore) would contain information\nrelative to the module integration, like lens information.\n\nThen there's the part relative to how the sensor itself is integrated\nin the device, and I agree this should be expressed in a system-wide\nconfiguration file. From there we will have other information we\ncannot retrieve from the subdev interface (mostly because of the lack\nof support in firmware for describing, in example, lens properties, at\nleast on device tree). In there we will be able to retreive\ninformation on the lens mounted on the sensor, where the sensor is\nmounted and its orientation.\n\nBefore seeing the integration in CameraSensor, I would like to discuss\nand see the desired structure of those files. First thing being how to\nidentify cameras vs sensor in the two files, if we agree about the\nnature and the intended content of those files.\n\nThanks\n j\n\n[1] being the sensor database a libcamera component, is it worth\nparsing it at run time ? I mean, can we pass it through a script, and\ngenerate a .h header with all the information easily accessible at\nrun-time from C++ ?\n\n>\n> Signed-off-by: Kieran Bingham \n> ---\n> include/libcamera/internal/camera_sensor.h | 1 +\n> src/libcamera/camera_sensor.cpp | 50 ++++++++++++++++++++++\n> 2 files changed, 51 insertions(+)\n>\n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index b9eba89f00fb..646f24a966bd 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -69,6 +69,7 @@ protected:\n>\n> private:\n> \tint generateId();\n> +\tint parseConfigurationFile();\n>\n> \tconst MediaEntity *entity_;\n> \tstd::unique_ptr subdev_;\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 935de528c496..5a1bda483b9d 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -17,6 +17,7 @@\n>\n> #include \n>\n> +#include \"libcamera/internal/configuration.h\"\n> #include \"libcamera/internal/formats.h\"\n> #include \"libcamera/internal/sysfs.h\"\n> #include \"libcamera/internal/utils.h\"\n> @@ -251,6 +252,12 @@ int CameraSensor::init()\n> \t\tpropertyValue = 0;\n> \tproperties_.set(properties::Rotation, propertyValue);\n>\n> +\t/*\n> +\t * Properties are pulled from a configuration file on a best effort.\n> +\t * If the file doesn't exist, or has no properties there is no failure.\n> +\t */\n> +\tparseConfigurationFile();\n> +\n> \t/* Enumerate, sort and cache media bus codes and sizes. */\n> \tformats_ = subdev_->formats(pad_);\n> \tif (formats_.empty()) {\n> @@ -584,4 +591,47 @@ int CameraSensor::generateId()\n> \treturn -EINVAL;\n> }\n>\n> +int CameraSensor::parseConfigurationFile()\n> +{\n> +\tConfiguration c;\n> +\n> +\tint ret = c.open(\"camera_sensor.json\");\n> +\tif (ret) {\n> +\t\tLOG(CameraSensor, Debug) << \"No configuration file available\";\n> +\t\treturn -ENODATA;\n> +\t}\n> +\n> +\t/* Find properties based on the Camera model_ */\n> +\t/* Todo: Spilt parsing out for multiple paths. */\n> +\n> +\t/* Find properties based around the Camera Sensor ID */\n> +\tjson::iterator it = c.data().find(id_);\n> +\tif (it == c.data().end())\n> +\t\treturn -ENOENT;\n> +\n> +\tjson j = *it;\n> +\tit = j.find(\"properties\");\n> +\tif (it == j.end())\n> +\t\treturn -ENOENT;\n> +\n> +\tjson deviceProperties = *it;\n> +\n> +\tfor (auto &[key, value] : deviceProperties.items()) {\n> +\t\tLOG(CameraSensor, Debug) << \"Key: \" << key << \" Value: \" << value;\n> +\n> +\t\tif (!value.is_number()) {\n> +\t\t\tLOG(CameraSensor, Debug) << \"Not a number: \" << value;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tif (key == \"Rotation\")\n> +\t\t\tproperties_.set(properties::Rotation, value);\n> +\n> +\t\tif (key == \"Location\")\n> +\t\t\tproperties_.set(properties::Location, value);\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> } /* namespace libcamera */\n> --\n> 2.25.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 7FF0ABE08A\n\tfor ;\n\tWed, 25 Nov 2020 10:58:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 16F2D63407;\n\tWed, 25 Nov 2020 11:58:28 +0100 (CET)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C0E09632EE\n\tfor ;\n\tWed, 25 Nov 2020 11:58:26 +0100 (CET)","from uno.localdomain (host-80-104-176-17.retail.telecomitalia.it\n\t[80.104.176.17]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id C22F3100004;\n\tWed, 25 Nov 2020 10:58:25 +0000 (UTC)"],"Date":"Wed, 25 Nov 2020 11:58:30 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20201125105830.obj6l43rkypgioif@uno.localdomain>","References":"<20201123164319.152742-1-kieran.bingham@ideasonboard.com>\n\t<20201123164319.152742-9-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201123164319.152742-9-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 8/8] libcamera: camera_sensor:\n\tParse configuration properties","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14019,"web_url":"https://patchwork.libcamera.org/comment/14019/","msgid":"<20201201181338.GS4569@pendragon.ideasonboard.com>","date":"2020-12-01T18:13:38","subject":"Re: [libcamera-devel] [RFC PATCH 8/8] libcamera: camera_sensor:\n\tParse configuration properties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo and Kieran,\n\nOn Wed, Nov 25, 2020 at 11:58:30AM +0100, Jacopo Mondi wrote:\n> On Mon, Nov 23, 2020 at 04:43:19PM +0000, Kieran Bingham wrote:\n> > Search for a camera_sensor.json configuration file specific to libcamera\n> > and parse it to see if any properties specific to this CameraSensor are\n> > applicable.\n> \n> For sake of discussion on the camera sensor database.\n\nI don't think the intent of this patch was to create a sensor database.\nThat being said, I agree that we shouldn't override camera sensor\nproperties like done below, I think this patch is more of an example of\nhow the configuration file parser could be used.\n\n> I don't think this should be a system-wide configuration file but\n> rather part of the libcamera sources, with a known path [1] It will\n> contain details on the sensor that we cannot retrieve easily from the\n> kernel interface, or that are not even exposed by it, even if we\n> should keep pushing to get most of those information from a more and\n> more expressive v4l2 subdev interface.\n> \n> Currently we have information that we cannot retrieve from the kernel\n> though. To name a few I can think of\n> - Colour Filter Arrangement (even if it could be deduced from the RAW\n> format components sequence)\n> - Control delays (see Niklas' delayed controls and RPi's\n> StaggeredControls)\n> - Color tuning information and sensitivites (white/black levels ? Gain\n> factors ? Probably we have a V4L2 control for that).\n> \n> Ideally, we should be able to push all of them into the kernel, but\n> it's a long process. Having a sensor database helps in that regard,\n> but makes less pressing the need for decent sensor drivers. One\n> example is all the size related information. We -could- get most of\n> them through the V4L2 Selection API, but only a few driver actually\n> expose them correctly. Recording them in userspace has anyway value,\n> so I'm a little debated.\n> \n> Also, if we have a 'camera module' to deal with, the sensor database\n> (which is not only about sensor anymore) would contain information\n> relative to the module integration, like lens information.\n> \n> Then there's the part relative to how the sensor itself is integrated\n> in the device, and I agree this should be expressed in a system-wide\n> configuration file. From there we will have other information we\n> cannot retrieve from the subdev interface (mostly because of the lack\n> of support in firmware for describing, in example, lens properties, at\n> least on device tree). In there we will be able to retreive\n> information on the lens mounted on the sensor, where the sensor is\n> mounted and its orientation.\n> \n> Before seeing the integration in CameraSensor, I would like to discuss\n> and see the desired structure of those files. First thing being how to\n> identify cameras vs sensor in the two files, if we agree about the\n> nature and the intended content of those files.\n> \n> [1] being the sensor database a libcamera component, is it worth\n> parsing it at run time ? I mean, can we pass it through a script, and\n> generate a .h header with all the information easily accessible at\n> run-time from C++ ?\n\nI'd rather go that way, either writing it directly in C++, or in a YAML\nfile.\n\n> > Signed-off-by: Kieran Bingham \n> > ---\n> > include/libcamera/internal/camera_sensor.h | 1 +\n> > src/libcamera/camera_sensor.cpp | 50 ++++++++++++++++++++++\n> > 2 files changed, 51 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > index b9eba89f00fb..646f24a966bd 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -69,6 +69,7 @@ protected:\n> >\n> > private:\n> > \tint generateId();\n> > +\tint parseConfigurationFile();\n> >\n> > \tconst MediaEntity *entity_;\n> > \tstd::unique_ptr subdev_;\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index 935de528c496..5a1bda483b9d 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -17,6 +17,7 @@\n> >\n> > #include \n> >\n> > +#include \"libcamera/internal/configuration.h\"\n> > #include \"libcamera/internal/formats.h\"\n> > #include \"libcamera/internal/sysfs.h\"\n> > #include \"libcamera/internal/utils.h\"\n> > @@ -251,6 +252,12 @@ int CameraSensor::init()\n> > \t\tpropertyValue = 0;\n> > \tproperties_.set(properties::Rotation, propertyValue);\n> >\n> > +\t/*\n> > +\t * Properties are pulled from a configuration file on a best effort.\n> > +\t * If the file doesn't exist, or has no properties there is no failure.\n> > +\t */\n> > +\tparseConfigurationFile();\n> > +\n> > \t/* Enumerate, sort and cache media bus codes and sizes. */\n> > \tformats_ = subdev_->formats(pad_);\n> > \tif (formats_.empty()) {\n> > @@ -584,4 +591,47 @@ int CameraSensor::generateId()\n> > \treturn -EINVAL;\n> > }\n> >\n> > +int CameraSensor::parseConfigurationFile()\n> > +{\n> > +\tConfiguration c;\n> > +\n> > +\tint ret = c.open(\"camera_sensor.json\");\n> > +\tif (ret) {\n> > +\t\tLOG(CameraSensor, Debug) << \"No configuration file available\";\n> > +\t\treturn -ENODATA;\n> > +\t}\n> > +\n> > +\t/* Find properties based on the Camera model_ */\n> > +\t/* Todo: Spilt parsing out for multiple paths. */\n> > +\n> > +\t/* Find properties based around the Camera Sensor ID */\n> > +\tjson::iterator it = c.data().find(id_);\n> > +\tif (it == c.data().end())\n> > +\t\treturn -ENOENT;\n> > +\n> > +\tjson j = *it;\n\nThis should probably be a const reference ? Thinking about it,\nConfiguration::data() should likely return a const reference too, to\navoid unintentional modifications.\n\n> > +\tit = j.find(\"properties\");\n> > +\tif (it == j.end())\n> > +\t\treturn -ENOENT;\n> > +\n> > +\tjson deviceProperties = *it;\n\nconst reference here too. Unless the json class uses implicit data\nsharing internally ?\n\n> > +\n> > +\tfor (auto &[key, value] : deviceProperties.items()) {\n> > +\t\tLOG(CameraSensor, Debug) << \"Key: \" << key << \" Value: \" << value;\n> > +\n> > +\t\tif (!value.is_number()) {\n> > +\t\t\tLOG(CameraSensor, Debug) << \"Not a number: \" << value;\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > +\n> > +\t\tif (key == \"Rotation\")\n> > +\t\t\tproperties_.set(properties::Rotation, value);\n\nWhat happens if we attempt to convert the value to an integer when it\ncontains a different type ?\n\n> > +\n> > +\t\tif (key == \"Location\")\n> > +\t\t\tproperties_.set(properties::Location, value);\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > } /* namespace libcamera */","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 984FDBE176\n\tfor ;\n\tTue, 1 Dec 2020 18:13:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 21A4B63503;\n\tTue, 1 Dec 2020 19:13:49 +0100 (CET)","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 044F563460\n\tfor ;\n\tTue, 1 Dec 2020 19:13:48 +0100 (CET)","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 6B5EFDBD;\n\tTue, 1 Dec 2020 19:13:47 +0100 (CET)"],"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=\"TVxq3VDT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606846427;\n\tbh=t6HQBGwJhq9iCv1RvuDjcJZnoFstC1nYxGJg8kbgy04=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TVxq3VDTzpM7WtcLHz2xVGtlsg/RnTkQ5utm6o19RUnij63Q7+XbZAXOeLuXX3Ve6\n\tgtCfQj96SWHHq46Ggf9jOFnrJUOERiCHss3MAZz+zeyLY0FvlKBuJeHGyfOGMw8a4x\n\t4B4dNXTKCVEz+UepPQ1aQi+7riBrRCd80vIHKgi4=","Date":"Tue, 1 Dec 2020 20:13:38 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20201201181338.GS4569@pendragon.ideasonboard.com>","References":"<20201123164319.152742-1-kieran.bingham@ideasonboard.com>\n\t<20201123164319.152742-9-kieran.bingham@ideasonboard.com>\n\t<20201125105830.obj6l43rkypgioif@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201125105830.obj6l43rkypgioif@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC PATCH 8/8] libcamera: camera_sensor:\n\tParse configuration properties","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]