[{"id":16625,"web_url":"https://patchwork.libcamera.org/comment/16625/","msgid":"","date":"2021-04-27T06:47:35","subject":"Re: [libcamera-devel] [PATCH v5 1/3] libcamera: Introduce camera\n\tsensor database","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Tue, Apr 13, 2021 at 12:14:37PM +0200, Jacopo Mondi wrote:\n> Introduce a 'database' of camera sensor information, which contains\n> the camera sensor properties which are not possible, or desirable,\n> to retrieve at run time.\n> \n> The camera sensor database is accessed through the static SensorDatabase\n> class which provides a single method to obtain the sensor properties.\n\nIt's not the class that is static, but the function.\n\n> Reviewed-by: Hirokazu Honda \n> Reviewed-by: Niklas Söderlund \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/internal/meson.build | 1 +\n> include/libcamera/internal/sensor_database.h | 28 ++++++\n> src/libcamera/meson.build | 1 +\n> src/libcamera/sensor_database.cpp | 94 ++++++++++++++++++++\n> 4 files changed, 124 insertions(+)\n> create mode 100644 include/libcamera/internal/sensor_database.h\n> create mode 100644 src/libcamera/sensor_database.cpp\n> \n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index 1fe3918cfe93..4aaa76ac1db1 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -38,6 +38,7 @@ libcamera_internal_headers = files([\n> 'process.h',\n> 'pub_key.h',\n> 'semaphore.h',\n> + 'sensor_database.h',\n> 'sysfs.h',\n> 'thread.h',\n> 'timer.h',\n> diff --git a/include/libcamera/internal/sensor_database.h b/include/libcamera/internal/sensor_database.h\n> new file mode 100644\n> index 000000000000..5f58b17a7b1d\n> --- /dev/null\n> +++ b/include/libcamera/internal/sensor_database.h\n> @@ -0,0 +1,28 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * sensor_database.h - Database of camera sensor properties\n> + */\n> +#ifndef __LIBCAMERA_SENSOR_DATABASE_H__\n> +#define __LIBCAMERA_SENSOR_DATABASE_H__\n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +struct CameraSensorProperties {\n> +\tSize unitCellSize;\n> +};\n> +\n> +class SensorDatabase\n> +{\n> +public:\n> +\tstatic const CameraSensorProperties *get(const std::string &sensor);\n> +};\n\nDo we need this class ? It would seem simpler to me to move this\nfunction to the CameraSensorProperties class. It's not big deal though,\nbut if you keep the class, I'd name it CameraSensorDatabase for\nconsistency.\n\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_SENSOR_DATABASE_H__ */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index e0a48aa23ea5..effae0ef5186 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -44,6 +44,7 @@ libcamera_sources = files([\n> 'pub_key.cpp',\n> 'request.cpp',\n> 'semaphore.cpp',\n> + 'sensor_database.cpp',\n> 'signal.cpp',\n> 'stream.cpp',\n> 'sysfs.cpp',\n> diff --git a/src/libcamera/sensor_database.cpp b/src/libcamera/sensor_database.cpp\n> new file mode 100644\n> index 000000000000..d6a97e06adfc\n> --- /dev/null\n> +++ b/src/libcamera/sensor_database.cpp\n> @@ -0,0 +1,94 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * sensor_database.cpp - Database of camera sensor properties\n> + */\n> +\n> +#include \"libcamera/internal/sensor_database.h\"\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/log.h\"\n> +\n> +/**\n> + * \\file sensor_database.h\n> + * \\brief Database of camera sensor properties\n> + *\n> + * The camera sensor database collects static information about camera sensors\n> + * that is not possible or desirable to retrieve at run time.\n\nShould we say \"to retrieve from the device\", as, from the point of view\nof a user of this class, calling get() is a runtime operation ?\n\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(SensorDatabase)\n> +\n> +/**\n> + * \\struct CameraSensorProperties\n> + * \\brief List of sensor properties\n> + *\n> + * \\var CameraSensorProperties::unitCellSize\n> + * \\brief The physical size of a pixel, including pixel edges, in nanometers.\n> + */\n> +\n> +/**\n> + * \\class SensorDatabase\n> + * \\brief Access the database of camera sensor properties\n> + *\n> + * The database is indexed using the camera sensor model, as reported by the\n> + * properties::Model property, and for each supported sensor it contains a\n> + * CameraSensorProperties list of properties.\n> + *\n> + * The class provides a single static get() method to access the sensor database\n> + * entries. If the sensor is not supported in the database nullptr is returned.\n\nYou can drop the last sentence, that's documented below already.\n\n> + */\n> +\n> +namespace {\n> +\n> +/** \\brief Sony IMX219 sensor properties */\n> +constexpr CameraSensorProperties imx219Props = {\n> +\t.unitCellSize = { 1120, 1120 }\n> +};\n> +\n> +/** \\brief Omnivision ov5670 sensor properties */\n> +constexpr CameraSensorProperties ov5670Props = {\n> +\t.unitCellSize = { 1120, 1120 }\n> +};\n> +\n> +/** \\brief Omnivision 13858 sensor properties */\n> +constexpr CameraSensorProperties ov13858Props = {\n> +\t.unitCellSize = { 1120, 1120 }\n> +};\n> +\n> +#define SENSOR_PROPERTIES(_sensor) \\\n> +\t{ #_sensor, &_sensor##Props }\n> +\n> +} /* namespace */\n> +\n> +/**\n> + * \\brief Retrieve the properties associated with a sensor\n> + * \\param sensor The sensor model name\n> + * \\return A pointer to the CameraSensorProperties instance associated with a sensor\n> + * or nullptr if the sensor is not supported in the database\n> + */\n> +const CameraSensorProperties *SensorDatabase::get(const std::string &sensor)\n> +{\n> +\tstatic const std::map sensorDatabase = {\n> +\t\tSENSOR_PROPERTIES(imx219),\n> +\t\tSENSOR_PROPERTIES(ov5670),\n> +\t\tSENSOR_PROPERTIES(ov13858),\n> +\t};\n\nI don't really see what the constexpr data above brings us, expect a\nmacro that obfuscates the code a bit. Isn't this better ?\n\n\tstatic const std::map sensorDatabase = {\n\t\t{ \"imx219\", {\n\t\t\t.unitCellSize = { 1120, 1120 },\n\t\t} },\n\t\t{ \"ov5670\", {\n\t\t\t.unitCellSize = { 1120, 1120 },\n\t\t} },\n\t\t{ \"ov13858\", {\n\t\t\t.unitCellSize = { 1120, 1120 },\n\t\t} },\n\t};\n\n> +\n> +\tconst auto it = sensorDatabase.find(sensor);\n> +\tif (it == sensorDatabase.end()) {\n> +\t\tLOG(SensorDatabase, Warning)\n> +\t\t\t<< \"No static properties available for '\" << sensor << \"'\";\n> +\t\tLOG(SensorDatabase, Warning)\n> +\t\t\t<< \"Please consider updating the sensor database\";\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\treturn it->second;\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 58497BDCC3\n\tfor ;\n\tTue, 27 Apr 2021 06:47:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BF01D68883;\n\tTue, 27 Apr 2021 08:47:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 14D0460512\n\tfor ;\n\tTue, 27 Apr 2021 08:47:42 +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 7D266E9;\n\tTue, 27 Apr 2021 08:47:41 +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=\"f7OQfks0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619506061;\n\tbh=b1MNKb9jLCsYUqLZt0V1WcIgXtvu+Qxc2/LrvOTNBLs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=f7OQfks0Gw/c34kRxbk+5JVbXpLQVfmk6QlH+uffySFUzUI/zwknxVLQHeTO528pA\n\t4dKgyk7g39lrmLsJoFaGKQQr0N8UJuyGLQ092gArb9CrW8hGgfcEqW3wzyXl2awk4n\n\t7pMslqhYioo5L8LAF2FWYCvuWs10kz7Au22q2rzY=","Date":"Tue, 27 Apr 2021 09:47:35 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210413101439.14659-1-jacopo@jmondi.org>\n\t<20210413101439.14659-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210413101439.14659-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v5 1/3] libcamera: Introduce camera\n\tsensor database","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16637,"web_url":"https://patchwork.libcamera.org/comment/16637/","msgid":"<20210427072903.s3fphgofjhlc26vs@uno.localdomain>","date":"2021-04-27T07:29:03","subject":"Re: [libcamera-devel] [PATCH v5 1/3] libcamera: Introduce camera\n\tsensor database","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Tue, Apr 27, 2021 at 09:47:35AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Tue, Apr 13, 2021 at 12:14:37PM +0200, Jacopo Mondi wrote:\n> > Introduce a 'database' of camera sensor information, which contains\n> > the camera sensor properties which are not possible, or desirable,\n> > to retrieve at run time.\n> >\n> > The camera sensor database is accessed through the static SensorDatabase\n> > class which provides a single method to obtain the sensor properties.\n>\n> It's not the class that is static, but the function.\n>\n> > Reviewed-by: Hirokazu Honda \n> > Reviewed-by: Niklas Söderlund \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/internal/meson.build | 1 +\n> > include/libcamera/internal/sensor_database.h | 28 ++++++\n> > src/libcamera/meson.build | 1 +\n> > src/libcamera/sensor_database.cpp | 94 ++++++++++++++++++++\n> > 4 files changed, 124 insertions(+)\n> > create mode 100644 include/libcamera/internal/sensor_database.h\n> > create mode 100644 src/libcamera/sensor_database.cpp\n> >\n> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > index 1fe3918cfe93..4aaa76ac1db1 100644\n> > --- a/include/libcamera/internal/meson.build\n> > +++ b/include/libcamera/internal/meson.build\n> > @@ -38,6 +38,7 @@ libcamera_internal_headers = files([\n> > 'process.h',\n> > 'pub_key.h',\n> > 'semaphore.h',\n> > + 'sensor_database.h',\n> > 'sysfs.h',\n> > 'thread.h',\n> > 'timer.h',\n> > diff --git a/include/libcamera/internal/sensor_database.h b/include/libcamera/internal/sensor_database.h\n> > new file mode 100644\n> > index 000000000000..5f58b17a7b1d\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/sensor_database.h\n> > @@ -0,0 +1,28 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * sensor_database.h - Database of camera sensor properties\n> > + */\n> > +#ifndef __LIBCAMERA_SENSOR_DATABASE_H__\n> > +#define __LIBCAMERA_SENSOR_DATABASE_H__\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +struct CameraSensorProperties {\n> > +\tSize unitCellSize;\n> > +};\n> > +\n> > +class SensorDatabase\n> > +{\n> > +public:\n> > +\tstatic const CameraSensorProperties *get(const std::string &sensor);\n> > +};\n>\n> Do we need this class ? It would seem simpler to me to move this\n> function to the CameraSensorProperties class. It's not big deal though,\n\nWhat's the CameraSensorProperties class ?\n\n> but if you keep the class, I'd name it CameraSensorDatabase for\n> consistency.\n>\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_SENSOR_DATABASE_H__ */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index e0a48aa23ea5..effae0ef5186 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -44,6 +44,7 @@ libcamera_sources = files([\n> > 'pub_key.cpp',\n> > 'request.cpp',\n> > 'semaphore.cpp',\n> > + 'sensor_database.cpp',\n> > 'signal.cpp',\n> > 'stream.cpp',\n> > 'sysfs.cpp',\n> > diff --git a/src/libcamera/sensor_database.cpp b/src/libcamera/sensor_database.cpp\n> > new file mode 100644\n> > index 000000000000..d6a97e06adfc\n> > --- /dev/null\n> > +++ b/src/libcamera/sensor_database.cpp\n> > @@ -0,0 +1,94 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021, Google Inc.\n> > + *\n> > + * sensor_database.cpp - Database of camera sensor properties\n> > + */\n> > +\n> > +#include \"libcamera/internal/sensor_database.h\"\n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/log.h\"\n> > +\n> > +/**\n> > + * \\file sensor_database.h\n> > + * \\brief Database of camera sensor properties\n> > + *\n> > + * The camera sensor database collects static information about camera sensors\n> > + * that is not possible or desirable to retrieve at run time.\n>\n> Should we say \"to retrieve from the device\", as, from the point of view\n> of a user of this class, calling get() is a runtime operation ?\n>\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(SensorDatabase)\n> > +\n> > +/**\n> > + * \\struct CameraSensorProperties\n> > + * \\brief List of sensor properties\n> > + *\n> > + * \\var CameraSensorProperties::unitCellSize\n> > + * \\brief The physical size of a pixel, including pixel edges, in nanometers.\n> > + */\n> > +\n> > +/**\n> > + * \\class SensorDatabase\n> > + * \\brief Access the database of camera sensor properties\n> > + *\n> > + * The database is indexed using the camera sensor model, as reported by the\n> > + * properties::Model property, and for each supported sensor it contains a\n> > + * CameraSensorProperties list of properties.\n> > + *\n> > + * The class provides a single static get() method to access the sensor database\n> > + * entries. If the sensor is not supported in the database nullptr is returned.\n>\n> You can drop the last sentence, that's documented below already.\n>\n> > + */\n> > +\n> > +namespace {\n> > +\n> > +/** \\brief Sony IMX219 sensor properties */\n> > +constexpr CameraSensorProperties imx219Props = {\n> > +\t.unitCellSize = { 1120, 1120 }\n> > +};\n> > +\n> > +/** \\brief Omnivision ov5670 sensor properties */\n> > +constexpr CameraSensorProperties ov5670Props = {\n> > +\t.unitCellSize = { 1120, 1120 }\n> > +};\n> > +\n> > +/** \\brief Omnivision 13858 sensor properties */\n> > +constexpr CameraSensorProperties ov13858Props = {\n> > +\t.unitCellSize = { 1120, 1120 }\n> > +};\n> > +\n> > +#define SENSOR_PROPERTIES(_sensor) \\\n> > +\t{ #_sensor, &_sensor##Props }\n> > +\n> > +} /* namespace */\n> > +\n> > +/**\n> > + * \\brief Retrieve the properties associated with a sensor\n> > + * \\param sensor The sensor model name\n> > + * \\return A pointer to the CameraSensorProperties instance associated with a sensor\n> > + * or nullptr if the sensor is not supported in the database\n> > + */\n> > +const CameraSensorProperties *SensorDatabase::get(const std::string &sensor)\n> > +{\n> > +\tstatic const std::map sensorDatabase = {\n> > +\t\tSENSOR_PROPERTIES(imx219),\n> > +\t\tSENSOR_PROPERTIES(ov5670),\n> > +\t\tSENSOR_PROPERTIES(ov13858),\n> > +\t};\n>\n> I don't really see what the constexpr data above brings us, expect a\n> macro that obfuscates the code a bit. Isn't this better ?\n>\n> \tstatic const std::map sensorDatabase = {\n> \t\t{ \"imx219\", {\n> \t\t\t.unitCellSize = { 1120, 1120 },\n> \t\t} },\n> \t\t{ \"ov5670\", {\n> \t\t\t.unitCellSize = { 1120, 1120 },\n> \t\t} },\n> \t\t{ \"ov13858\", {\n> \t\t\t.unitCellSize = { 1120, 1120 },\n> \t\t} },\n\nThinking forward, when the sensor db will have more fields, I think\ndeclaring the map using the macro is more compact. I can drop it if\nyou feel strongly about this.\n\n> \t};\n>\n> > +\n> > +\tconst auto it = sensorDatabase.find(sensor);\n> > +\tif (it == sensorDatabase.end()) {\n> > +\t\tLOG(SensorDatabase, Warning)\n> > +\t\t\t<< \"No static properties available for '\" << sensor << \"'\";\n> > +\t\tLOG(SensorDatabase, Warning)\n> > +\t\t\t<< \"Please consider updating the sensor database\";\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> > +\treturn it->second;\n> > +}\n> > +\n> > +} /* namespace libcamera */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 95C8ABDCC3\n\tfor ;\n\tTue, 27 Apr 2021 07:28:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5DF82688BA;\n\tTue, 27 Apr 2021 09:28:22 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CAB0968883\n\tfor ;\n\tTue, 27 Apr 2021 09:28:21 +0200 (CEST)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 48409100002;\n\tTue, 27 Apr 2021 07:28:20 +0000 (UTC)"],"Date":"Tue, 27 Apr 2021 09:29:03 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210427072903.s3fphgofjhlc26vs@uno.localdomain>","References":"<20210413101439.14659-1-jacopo@jmondi.org>\n\t<20210413101439.14659-2-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 1/3] libcamera: Introduce camera\n\tsensor database","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16643,"web_url":"https://patchwork.libcamera.org/comment/16643/","msgid":"","date":"2021-04-27T07:48:37","subject":"Re: [libcamera-devel] [PATCH v5 1/3] libcamera: Introduce camera\n\tsensor database","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Apr 27, 2021 at 09:29:03AM +0200, Jacopo Mondi wrote:\n> On Tue, Apr 27, 2021 at 09:47:35AM +0300, Laurent Pinchart wrote:\n> > On Tue, Apr 13, 2021 at 12:14:37PM +0200, Jacopo Mondi wrote:\n> > > Introduce a 'database' of camera sensor information, which contains\n> > > the camera sensor properties which are not possible, or desirable,\n> > > to retrieve at run time.\n> > >\n> > > The camera sensor database is accessed through the static SensorDatabase\n> > > class which provides a single method to obtain the sensor properties.\n> >\n> > It's not the class that is static, but the function.\n> >\n> > > Reviewed-by: Hirokazu Honda \n> > > Reviewed-by: Niklas Söderlund \n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/internal/meson.build | 1 +\n> > > include/libcamera/internal/sensor_database.h | 28 ++++++\n> > > src/libcamera/meson.build | 1 +\n> > > src/libcamera/sensor_database.cpp | 94 ++++++++++++++++++++\n> > > 4 files changed, 124 insertions(+)\n> > > create mode 100644 include/libcamera/internal/sensor_database.h\n> > > create mode 100644 src/libcamera/sensor_database.cpp\n> > >\n> > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > > index 1fe3918cfe93..4aaa76ac1db1 100644\n> > > --- a/include/libcamera/internal/meson.build\n> > > +++ b/include/libcamera/internal/meson.build\n> > > @@ -38,6 +38,7 @@ libcamera_internal_headers = files([\n> > > 'process.h',\n> > > 'pub_key.h',\n> > > 'semaphore.h',\n> > > + 'sensor_database.h',\n> > > 'sysfs.h',\n> > > 'thread.h',\n> > > 'timer.h',\n> > > diff --git a/include/libcamera/internal/sensor_database.h b/include/libcamera/internal/sensor_database.h\n> > > new file mode 100644\n> > > index 000000000000..5f58b17a7b1d\n> > > --- /dev/null\n> > > +++ b/include/libcamera/internal/sensor_database.h\n> > > @@ -0,0 +1,28 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Google Inc.\n> > > + *\n> > > + * sensor_database.h - Database of camera sensor properties\n> > > + */\n> > > +#ifndef __LIBCAMERA_SENSOR_DATABASE_H__\n> > > +#define __LIBCAMERA_SENSOR_DATABASE_H__\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +struct CameraSensorProperties {\n> > > +\tSize unitCellSize;\n> > > +};\n> > > +\n> > > +class SensorDatabase\n> > > +{\n> > > +public:\n> > > +\tstatic const CameraSensorProperties *get(const std::string &sensor);\n> > > +};\n> >\n> > Do we need this class ? It would seem simpler to me to move this\n> > function to the CameraSensorProperties class. It's not big deal though,\n> \n> What's the CameraSensorProperties class ?\n\nSeems to be a struct :-)\n\n> > but if you keep the class, I'd name it CameraSensorDatabase for\n> > consistency.\n> >\n> > > +\n> > > +} /* namespace libcamera */\n> > > +\n> > > +#endif /* __LIBCAMERA_SENSOR_DATABASE_H__ */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index e0a48aa23ea5..effae0ef5186 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -44,6 +44,7 @@ libcamera_sources = files([\n> > > 'pub_key.cpp',\n> > > 'request.cpp',\n> > > 'semaphore.cpp',\n> > > + 'sensor_database.cpp',\n> > > 'signal.cpp',\n> > > 'stream.cpp',\n> > > 'sysfs.cpp',\n> > > diff --git a/src/libcamera/sensor_database.cpp b/src/libcamera/sensor_database.cpp\n> > > new file mode 100644\n> > > index 000000000000..d6a97e06adfc\n> > > --- /dev/null\n> > > +++ b/src/libcamera/sensor_database.cpp\n> > > @@ -0,0 +1,94 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2021, Google Inc.\n> > > + *\n> > > + * sensor_database.cpp - Database of camera sensor properties\n> > > + */\n> > > +\n> > > +#include \"libcamera/internal/sensor_database.h\"\n> > > +\n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/log.h\"\n> > > +\n> > > +/**\n> > > + * \\file sensor_database.h\n> > > + * \\brief Database of camera sensor properties\n> > > + *\n> > > + * The camera sensor database collects static information about camera sensors\n> > > + * that is not possible or desirable to retrieve at run time.\n> >\n> > Should we say \"to retrieve from the device\", as, from the point of view\n> > of a user of this class, calling get() is a runtime operation ?\n> >\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DEFINE_CATEGORY(SensorDatabase)\n> > > +\n> > > +/**\n> > > + * \\struct CameraSensorProperties\n> > > + * \\brief List of sensor properties\n> > > + *\n> > > + * \\var CameraSensorProperties::unitCellSize\n> > > + * \\brief The physical size of a pixel, including pixel edges, in nanometers.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\class SensorDatabase\n> > > + * \\brief Access the database of camera sensor properties\n> > > + *\n> > > + * The database is indexed using the camera sensor model, as reported by the\n> > > + * properties::Model property, and for each supported sensor it contains a\n> > > + * CameraSensorProperties list of properties.\n> > > + *\n> > > + * The class provides a single static get() method to access the sensor database\n> > > + * entries. If the sensor is not supported in the database nullptr is returned.\n> >\n> > You can drop the last sentence, that's documented below already.\n> >\n> > > + */\n> > > +\n> > > +namespace {\n> > > +\n> > > +/** \\brief Sony IMX219 sensor properties */\n> > > +constexpr CameraSensorProperties imx219Props = {\n> > > +\t.unitCellSize = { 1120, 1120 }\n> > > +};\n> > > +\n> > > +/** \\brief Omnivision ov5670 sensor properties */\n> > > +constexpr CameraSensorProperties ov5670Props = {\n> > > +\t.unitCellSize = { 1120, 1120 }\n> > > +};\n> > > +\n> > > +/** \\brief Omnivision 13858 sensor properties */\n> > > +constexpr CameraSensorProperties ov13858Props = {\n> > > +\t.unitCellSize = { 1120, 1120 }\n> > > +};\n> > > +\n> > > +#define SENSOR_PROPERTIES(_sensor) \\\n> > > +\t{ #_sensor, &_sensor##Props }\n> > > +\n> > > +} /* namespace */\n> > > +\n> > > +/**\n> > > + * \\brief Retrieve the properties associated with a sensor\n> > > + * \\param sensor The sensor model name\n> > > + * \\return A pointer to the CameraSensorProperties instance associated with a sensor\n> > > + * or nullptr if the sensor is not supported in the database\n> > > + */\n> > > +const CameraSensorProperties *SensorDatabase::get(const std::string &sensor)\n> > > +{\n> > > +\tstatic const std::map sensorDatabase = {\n> > > +\t\tSENSOR_PROPERTIES(imx219),\n> > > +\t\tSENSOR_PROPERTIES(ov5670),\n> > > +\t\tSENSOR_PROPERTIES(ov13858),\n> > > +\t};\n> >\n> > I don't really see what the constexpr data above brings us, expect a\n> > macro that obfuscates the code a bit. Isn't this better ?\n> >\n> > \tstatic const std::map sensorDatabase = {\n> > \t\t{ \"imx219\", {\n> > \t\t\t.unitCellSize = { 1120, 1120 },\n> > \t\t} },\n> > \t\t{ \"ov5670\", {\n> > \t\t\t.unitCellSize = { 1120, 1120 },\n> > \t\t} },\n> > \t\t{ \"ov13858\", {\n> > \t\t\t.unitCellSize = { 1120, 1120 },\n> > \t\t} },\n> \n> Thinking forward, when the sensor db will have more fields, I think\n> declaring the map using the macro is more compact. I can drop it if\n> you feel strongly about this.\n\nI find the style that we use in, for example, the pixelFormatInfo map in\nformats.cpp, more readable than the constexpr + macro here.\n\n> > \t};\n> >\n> > > +\n> > > +\tconst auto it = sensorDatabase.find(sensor);\n> > > +\tif (it == sensorDatabase.end()) {\n> > > +\t\tLOG(SensorDatabase, Warning)\n> > > +\t\t\t<< \"No static properties available for '\" << sensor << \"'\";\n> > > +\t\tLOG(SensorDatabase, Warning)\n> > > +\t\t\t<< \"Please consider updating the sensor database\";\n> > > +\t\treturn nullptr;\n> > > +\t}\n> > > +\n> > > +\treturn it->second;\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 7AE26BDCC3\n\tfor ;\n\tTue, 27 Apr 2021 07:48:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B90A1688BD;\n\tTue, 27 Apr 2021 09:48:45 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 65560688B1\n\tfor ;\n\tTue, 27 Apr 2021 09:48:44 +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 DF7C2E9;\n\tTue, 27 Apr 2021 09:48:43 +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=\"qrSPohJ1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619509724;\n\tbh=Q5mboe6wmvyMTcJJsZ3TtS68Ucc9GlMjvl6LX6M20w4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qrSPohJ1CAxUYNSph9AEJusaWetqabsi2HgoPklN/xGWfl/URhfI69SsewbsqUgjn\n\tUsWXRAIkEW7EuTrbxGQyx9/SRKbHZfpMWV2vpqv3xhiWBfALPMJKf8bWFmonLIdIsF\n\tgiAMfT4hSsynrATE2CHhUEYHX3IneDSpzk6iYRLM=","Date":"Tue, 27 Apr 2021 10:48:37 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210413101439.14659-1-jacopo@jmondi.org>\n\t<20210413101439.14659-2-jacopo@jmondi.org>\n\t\n\t<20210427072903.s3fphgofjhlc26vs@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210427072903.s3fphgofjhlc26vs@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v5 1/3] libcamera: Introduce camera\n\tsensor database","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16703,"web_url":"https://patchwork.libcamera.org/comment/16703/","msgid":"<20210430164341.5ot6qo6xa7hg4o7z@uno.localdomain>","date":"2021-04-30T16:43:41","subject":"Re: [libcamera-devel] [PATCH v5 1/3] libcamera: Introduce camera\n\tsensor database","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Apr 27, 2021 at 10:48:37AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Tue, Apr 27, 2021 at 09:29:03AM +0200, Jacopo Mondi wrote:\n> > On Tue, Apr 27, 2021 at 09:47:35AM +0300, Laurent Pinchart wrote:\n> > > On Tue, Apr 13, 2021 at 12:14:37PM +0200, Jacopo Mondi wrote:\n> > > > Introduce a 'database' of camera sensor information, which contains\n> > > > the camera sensor properties which are not possible, or desirable,\n> > > > to retrieve at run time.\n> > > >\n> > > > The camera sensor database is accessed through the static SensorDatabase\n> > > > class which provides a single method to obtain the sensor properties.\n> > >\n> > > It's not the class that is static, but the function.\n> > >\n> > > > Reviewed-by: Hirokazu Honda \n> > > > Reviewed-by: Niklas Söderlund \n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > include/libcamera/internal/meson.build | 1 +\n> > > > include/libcamera/internal/sensor_database.h | 28 ++++++\n> > > > src/libcamera/meson.build | 1 +\n> > > > src/libcamera/sensor_database.cpp | 94 ++++++++++++++++++++\n> > > > 4 files changed, 124 insertions(+)\n> > > > create mode 100644 include/libcamera/internal/sensor_database.h\n> > > > create mode 100644 src/libcamera/sensor_database.cpp\n> > > >\n> > > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > > > index 1fe3918cfe93..4aaa76ac1db1 100644\n> > > > --- a/include/libcamera/internal/meson.build\n> > > > +++ b/include/libcamera/internal/meson.build\n> > > > @@ -38,6 +38,7 @@ libcamera_internal_headers = files([\n> > > > 'process.h',\n> > > > 'pub_key.h',\n> > > > 'semaphore.h',\n> > > > + 'sensor_database.h',\n> > > > 'sysfs.h',\n> > > > 'thread.h',\n> > > > 'timer.h',\n> > > > diff --git a/include/libcamera/internal/sensor_database.h b/include/libcamera/internal/sensor_database.h\n> > > > new file mode 100644\n> > > > index 000000000000..5f58b17a7b1d\n> > > > --- /dev/null\n> > > > +++ b/include/libcamera/internal/sensor_database.h\n> > > > @@ -0,0 +1,28 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Google Inc.\n> > > > + *\n> > > > + * sensor_database.h - Database of camera sensor properties\n> > > > + */\n> > > > +#ifndef __LIBCAMERA_SENSOR_DATABASE_H__\n> > > > +#define __LIBCAMERA_SENSOR_DATABASE_H__\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +struct CameraSensorProperties {\n> > > > +\tSize unitCellSize;\n> > > > +};\n> > > > +\n> > > > +class SensorDatabase\n> > > > +{\n> > > > +public:\n> > > > +\tstatic const CameraSensorProperties *get(const std::string &sensor);\n> > > > +};\n> > >\n> > > Do we need this class ? It would seem simpler to me to move this\n> > > function to the CameraSensorProperties class. It's not big deal though,\n> >\n> > What's the CameraSensorProperties class ?\n>\n> Seems to be a struct :-)\n\nTook me a while to get what you meant\n\n const CameraSensorProperties *props = CameraSensorProperties::get(\"imx219\");\n\nThis might work as well, the SensorDatabase class contains that single\nfunction after all.\n\nA small nit: if CameraSensorProperties is all it remains, should I\nname the file(s) camera_sensor_properties.cpp(.h) ?\n\n\n\n>\n> > > but if you keep the class, I'd name it CameraSensorDatabase for\n> > > consistency.\n> > >\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > +\n> > > > +#endif /* __LIBCAMERA_SENSOR_DATABASE_H__ */\n> > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > > index e0a48aa23ea5..effae0ef5186 100644\n> > > > --- a/src/libcamera/meson.build\n> > > > +++ b/src/libcamera/meson.build\n> > > > @@ -44,6 +44,7 @@ libcamera_sources = files([\n> > > > 'pub_key.cpp',\n> > > > 'request.cpp',\n> > > > 'semaphore.cpp',\n> > > > + 'sensor_database.cpp',\n> > > > 'signal.cpp',\n> > > > 'stream.cpp',\n> > > > 'sysfs.cpp',\n> > > > diff --git a/src/libcamera/sensor_database.cpp b/src/libcamera/sensor_database.cpp\n> > > > new file mode 100644\n> > > > index 000000000000..d6a97e06adfc\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/sensor_database.cpp\n> > > > @@ -0,0 +1,94 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2021, Google Inc.\n> > > > + *\n> > > > + * sensor_database.cpp - Database of camera sensor properties\n> > > > + */\n> > > > +\n> > > > +#include \"libcamera/internal/sensor_database.h\"\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"libcamera/internal/log.h\"\n> > > > +\n> > > > +/**\n> > > > + * \\file sensor_database.h\n> > > > + * \\brief Database of camera sensor properties\n> > > > + *\n> > > > + * The camera sensor database collects static information about camera sensors\n> > > > + * that is not possible or desirable to retrieve at run time.\n> > >\n> > > Should we say \"to retrieve from the device\", as, from the point of view\n> > > of a user of this class, calling get() is a runtime operation ?\n> > >\n> > > > + */\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +LOG_DEFINE_CATEGORY(SensorDatabase)\n> > > > +\n> > > > +/**\n> > > > + * \\struct CameraSensorProperties\n> > > > + * \\brief List of sensor properties\n> > > > + *\n> > > > + * \\var CameraSensorProperties::unitCellSize\n> > > > + * \\brief The physical size of a pixel, including pixel edges, in nanometers.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\class SensorDatabase\n> > > > + * \\brief Access the database of camera sensor properties\n> > > > + *\n> > > > + * The database is indexed using the camera sensor model, as reported by the\n> > > > + * properties::Model property, and for each supported sensor it contains a\n> > > > + * CameraSensorProperties list of properties.\n> > > > + *\n> > > > + * The class provides a single static get() method to access the sensor database\n> > > > + * entries. If the sensor is not supported in the database nullptr is returned.\n> > >\n> > > You can drop the last sentence, that's documented below already.\n> > >\n> > > > + */\n> > > > +\n> > > > +namespace {\n> > > > +\n> > > > +/** \\brief Sony IMX219 sensor properties */\n> > > > +constexpr CameraSensorProperties imx219Props = {\n> > > > +\t.unitCellSize = { 1120, 1120 }\n> > > > +};\n> > > > +\n> > > > +/** \\brief Omnivision ov5670 sensor properties */\n> > > > +constexpr CameraSensorProperties ov5670Props = {\n> > > > +\t.unitCellSize = { 1120, 1120 }\n> > > > +};\n> > > > +\n> > > > +/** \\brief Omnivision 13858 sensor properties */\n> > > > +constexpr CameraSensorProperties ov13858Props = {\n> > > > +\t.unitCellSize = { 1120, 1120 }\n> > > > +};\n> > > > +\n> > > > +#define SENSOR_PROPERTIES(_sensor) \\\n> > > > +\t{ #_sensor, &_sensor##Props }\n> > > > +\n> > > > +} /* namespace */\n> > > > +\n> > > > +/**\n> > > > + * \\brief Retrieve the properties associated with a sensor\n> > > > + * \\param sensor The sensor model name\n> > > > + * \\return A pointer to the CameraSensorProperties instance associated with a sensor\n> > > > + * or nullptr if the sensor is not supported in the database\n> > > > + */\n> > > > +const CameraSensorProperties *SensorDatabase::get(const std::string &sensor)\n> > > > +{\n> > > > +\tstatic const std::map sensorDatabase = {\n> > > > +\t\tSENSOR_PROPERTIES(imx219),\n> > > > +\t\tSENSOR_PROPERTIES(ov5670),\n> > > > +\t\tSENSOR_PROPERTIES(ov13858),\n> > > > +\t};\n> > >\n> > > I don't really see what the constexpr data above brings us, expect a\n> > > macro that obfuscates the code a bit. Isn't this better ?\n> > >\n> > > \tstatic const std::map sensorDatabase = {\n> > > \t\t{ \"imx219\", {\n> > > \t\t\t.unitCellSize = { 1120, 1120 },\n> > > \t\t} },\n> > > \t\t{ \"ov5670\", {\n> > > \t\t\t.unitCellSize = { 1120, 1120 },\n> > > \t\t} },\n> > > \t\t{ \"ov13858\", {\n> > > \t\t\t.unitCellSize = { 1120, 1120 },\n> > > \t\t} },\n> >\n> > Thinking forward, when the sensor db will have more fields, I think\n> > declaring the map using the macro is more compact. I can drop it if\n> > you feel strongly about this.\n>\n> I find the style that we use in, for example, the pixelFormatInfo map in\n> formats.cpp, more readable than the constexpr + macro here.\n>\n> > > \t};\n> > >\n> > > > +\n> > > > +\tconst auto it = sensorDatabase.find(sensor);\n> > > > +\tif (it == sensorDatabase.end()) {\n> > > > +\t\tLOG(SensorDatabase, Warning)\n> > > > +\t\t\t<< \"No static properties available for '\" << sensor << \"'\";\n> > > > +\t\tLOG(SensorDatabase, Warning)\n> > > > +\t\t\t<< \"Please consider updating the sensor database\";\n> > > > +\t\treturn nullptr;\n> > > > +\t}\n> > > > +\n> > > > +\treturn it->second;\n> > > > +}\n> > > > +\n> > > > +} /* namespace libcamera */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 3DC06BDE4D\n\tfor ;\n\tFri, 30 Apr 2021 16:43:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D7476890E;\n\tFri, 30 Apr 2021 18:43:00 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 95BF3688A5\n\tfor ;\n\tFri, 30 Apr 2021 18:42:59 +0200 (CEST)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 1DFEE40008;\n\tFri, 30 Apr 2021 16:42:58 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Fri, 30 Apr 2021 18:43:41 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210430164341.5ot6qo6xa7hg4o7z@uno.localdomain>","References":"<20210413101439.14659-1-jacopo@jmondi.org>\n\t<20210413101439.14659-2-jacopo@jmondi.org>\n\t\n\t<20210427072903.s3fphgofjhlc26vs@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 1/3] libcamera: Introduce camera\n\tsensor database","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16707,"web_url":"https://patchwork.libcamera.org/comment/16707/","msgid":"","date":"2021-05-01T02:45:19","subject":"Re: [libcamera-devel] [PATCH v5 1/3] libcamera: Introduce camera\n\tsensor database","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Apr 30, 2021 at 06:43:41PM +0200, Jacopo Mondi wrote:\n> On Tue, Apr 27, 2021 at 10:48:37AM +0300, Laurent Pinchart wrote:\n> > On Tue, Apr 27, 2021 at 09:29:03AM +0200, Jacopo Mondi wrote:\n> > > On Tue, Apr 27, 2021 at 09:47:35AM +0300, Laurent Pinchart wrote:\n> > > > On Tue, Apr 13, 2021 at 12:14:37PM +0200, Jacopo Mondi wrote:\n> > > > > Introduce a 'database' of camera sensor information, which contains\n> > > > > the camera sensor properties which are not possible, or desirable,\n> > > > > to retrieve at run time.\n> > > > >\n> > > > > The camera sensor database is accessed through the static SensorDatabase\n> > > > > class which provides a single method to obtain the sensor properties.\n> > > >\n> > > > It's not the class that is static, but the function.\n> > > >\n> > > > > Reviewed-by: Hirokazu Honda \n> > > > > Reviewed-by: Niklas Söderlund \n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > include/libcamera/internal/meson.build | 1 +\n> > > > > include/libcamera/internal/sensor_database.h | 28 ++++++\n> > > > > src/libcamera/meson.build | 1 +\n> > > > > src/libcamera/sensor_database.cpp | 94 ++++++++++++++++++++\n> > > > > 4 files changed, 124 insertions(+)\n> > > > > create mode 100644 include/libcamera/internal/sensor_database.h\n> > > > > create mode 100644 src/libcamera/sensor_database.cpp\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > > > > index 1fe3918cfe93..4aaa76ac1db1 100644\n> > > > > --- a/include/libcamera/internal/meson.build\n> > > > > +++ b/include/libcamera/internal/meson.build\n> > > > > @@ -38,6 +38,7 @@ libcamera_internal_headers = files([\n> > > > > 'process.h',\n> > > > > 'pub_key.h',\n> > > > > 'semaphore.h',\n> > > > > + 'sensor_database.h',\n> > > > > 'sysfs.h',\n> > > > > 'thread.h',\n> > > > > 'timer.h',\n> > > > > diff --git a/include/libcamera/internal/sensor_database.h b/include/libcamera/internal/sensor_database.h\n> > > > > new file mode 100644\n> > > > > index 000000000000..5f58b17a7b1d\n> > > > > --- /dev/null\n> > > > > +++ b/include/libcamera/internal/sensor_database.h\n> > > > > @@ -0,0 +1,28 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2021, Google Inc.\n> > > > > + *\n> > > > > + * sensor_database.h - Database of camera sensor properties\n> > > > > + */\n> > > > > +#ifndef __LIBCAMERA_SENSOR_DATABASE_H__\n> > > > > +#define __LIBCAMERA_SENSOR_DATABASE_H__\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +struct CameraSensorProperties {\n> > > > > +\tSize unitCellSize;\n> > > > > +};\n> > > > > +\n> > > > > +class SensorDatabase\n> > > > > +{\n> > > > > +public:\n> > > > > +\tstatic const CameraSensorProperties *get(const std::string &sensor);\n> > > > > +};\n> > > >\n> > > > Do we need this class ? It would seem simpler to me to move this\n> > > > function to the CameraSensorProperties class. It's not big deal though,\n> > >\n> > > What's the CameraSensorProperties class ?\n> >\n> > Seems to be a struct :-)\n> \n> Took me a while to get what you meant\n> \n> const CameraSensorProperties *props = CameraSensorProperties::get(\"imx219\");\n> \n> This might work as well, the SensorDatabase class contains that single\n> function after all.\n> \n> A small nit: if CameraSensorProperties is all it remains, should I\n> name the file(s) camera_sensor_properties.cpp(.h) ?\n\nThat would work for me. I was even thinking about moving this to\ncamera_sensor.cpp, but it may grow a bit big in the future. Up to you.\n\n> > > > but if you keep the class, I'd name it CameraSensorDatabase for\n> > > > consistency.\n> > > >\n> > > > > +\n> > > > > +} /* namespace libcamera */\n> > > > > +\n> > > > > +#endif /* __LIBCAMERA_SENSOR_DATABASE_H__ */\n> > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > > > index e0a48aa23ea5..effae0ef5186 100644\n> > > > > --- a/src/libcamera/meson.build\n> > > > > +++ b/src/libcamera/meson.build\n> > > > > @@ -44,6 +44,7 @@ libcamera_sources = files([\n> > > > > 'pub_key.cpp',\n> > > > > 'request.cpp',\n> > > > > 'semaphore.cpp',\n> > > > > + 'sensor_database.cpp',\n> > > > > 'signal.cpp',\n> > > > > 'stream.cpp',\n> > > > > 'sysfs.cpp',\n> > > > > diff --git a/src/libcamera/sensor_database.cpp b/src/libcamera/sensor_database.cpp\n> > > > > new file mode 100644\n> > > > > index 000000000000..d6a97e06adfc\n> > > > > --- /dev/null\n> > > > > +++ b/src/libcamera/sensor_database.cpp\n> > > > > @@ -0,0 +1,94 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2021, Google Inc.\n> > > > > + *\n> > > > > + * sensor_database.cpp - Database of camera sensor properties\n> > > > > + */\n> > > > > +\n> > > > > +#include \"libcamera/internal/sensor_database.h\"\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \"libcamera/internal/log.h\"\n> > > > > +\n> > > > > +/**\n> > > > > + * \\file sensor_database.h\n> > > > > + * \\brief Database of camera sensor properties\n> > > > > + *\n> > > > > + * The camera sensor database collects static information about camera sensors\n> > > > > + * that is not possible or desirable to retrieve at run time.\n> > > >\n> > > > Should we say \"to retrieve from the device\", as, from the point of view\n> > > > of a user of this class, calling get() is a runtime operation ?\n> > > >\n> > > > > + */\n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +LOG_DEFINE_CATEGORY(SensorDatabase)\n> > > > > +\n> > > > > +/**\n> > > > > + * \\struct CameraSensorProperties\n> > > > > + * \\brief List of sensor properties\n> > > > > + *\n> > > > > + * \\var CameraSensorProperties::unitCellSize\n> > > > > + * \\brief The physical size of a pixel, including pixel edges, in nanometers.\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\class SensorDatabase\n> > > > > + * \\brief Access the database of camera sensor properties\n> > > > > + *\n> > > > > + * The database is indexed using the camera sensor model, as reported by the\n> > > > > + * properties::Model property, and for each supported sensor it contains a\n> > > > > + * CameraSensorProperties list of properties.\n> > > > > + *\n> > > > > + * The class provides a single static get() method to access the sensor database\n> > > > > + * entries. If the sensor is not supported in the database nullptr is returned.\n> > > >\n> > > > You can drop the last sentence, that's documented below already.\n> > > >\n> > > > > + */\n> > > > > +\n> > > > > +namespace {\n> > > > > +\n> > > > > +/** \\brief Sony IMX219 sensor properties */\n> > > > > +constexpr CameraSensorProperties imx219Props = {\n> > > > > +\t.unitCellSize = { 1120, 1120 }\n> > > > > +};\n> > > > > +\n> > > > > +/** \\brief Omnivision ov5670 sensor properties */\n> > > > > +constexpr CameraSensorProperties ov5670Props = {\n> > > > > +\t.unitCellSize = { 1120, 1120 }\n> > > > > +};\n> > > > > +\n> > > > > +/** \\brief Omnivision 13858 sensor properties */\n> > > > > +constexpr CameraSensorProperties ov13858Props = {\n> > > > > +\t.unitCellSize = { 1120, 1120 }\n> > > > > +};\n> > > > > +\n> > > > > +#define SENSOR_PROPERTIES(_sensor) \\\n> > > > > +\t{ #_sensor, &_sensor##Props }\n> > > > > +\n> > > > > +} /* namespace */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Retrieve the properties associated with a sensor\n> > > > > + * \\param sensor The sensor model name\n> > > > > + * \\return A pointer to the CameraSensorProperties instance associated with a sensor\n> > > > > + * or nullptr if the sensor is not supported in the database\n> > > > > + */\n> > > > > +const CameraSensorProperties *SensorDatabase::get(const std::string &sensor)\n> > > > > +{\n> > > > > +\tstatic const std::map sensorDatabase = {\n> > > > > +\t\tSENSOR_PROPERTIES(imx219),\n> > > > > +\t\tSENSOR_PROPERTIES(ov5670),\n> > > > > +\t\tSENSOR_PROPERTIES(ov13858),\n> > > > > +\t};\n> > > >\n> > > > I don't really see what the constexpr data above brings us, expect a\n> > > > macro that obfuscates the code a bit. Isn't this better ?\n> > > >\n> > > > \tstatic const std::map sensorDatabase = {\n> > > > \t\t{ \"imx219\", {\n> > > > \t\t\t.unitCellSize = { 1120, 1120 },\n> > > > \t\t} },\n> > > > \t\t{ \"ov5670\", {\n> > > > \t\t\t.unitCellSize = { 1120, 1120 },\n> > > > \t\t} },\n> > > > \t\t{ \"ov13858\", {\n> > > > \t\t\t.unitCellSize = { 1120, 1120 },\n> > > > \t\t} },\n> > >\n> > > Thinking forward, when the sensor db will have more fields, I think\n> > > declaring the map using the macro is more compact. I can drop it if\n> > > you feel strongly about this.\n> >\n> > I find the style that we use in, for example, the pixelFormatInfo map in\n> > formats.cpp, more readable than the constexpr + macro here.\n> >\n> > > > \t};\n> > > >\n> > > > > +\n> > > > > +\tconst auto it = sensorDatabase.find(sensor);\n> > > > > +\tif (it == sensorDatabase.end()) {\n> > > > > +\t\tLOG(SensorDatabase, Warning)\n> > > > > +\t\t\t<< \"No static properties available for '\" << sensor << \"'\";\n> > > > > +\t\tLOG(SensorDatabase, Warning)\n> > > > > +\t\t\t<< \"Please consider updating the sensor database\";\n> > > > > +\t\treturn nullptr;\n> > > > > +\t}\n> > > > > +\n> > > > > +\treturn it->second;\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 879E6BDE50\n\tfor ;\n\tSat, 1 May 2021 02:45:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E02C3602BF;\n\tSat, 1 May 2021 04:45:24 +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 BF577602BF\n\tfor ;\n\tSat, 1 May 2021 04:45:22 +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 34BD1A46;\n\tSat, 1 May 2021 04:45:22 +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=\"bilW5lTl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619837122;\n\tbh=pU8gbPyZtDbPJoWa02C6OSr9yxREL+XYCEPYYqBNo8E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bilW5lTlam9w7RYToSUonCvlXWO9wBVQxnZvaa6nl62vnuqV+22hhArGsWkP/kCna\n\tqRvGf0W+Fz64q1YpWwK2dHxK5xmW2AJfHGXCOeCu75x8b7f3z5YFOMZuumio4Ua5Yz\n\tPPIK4f4Wd7OtbXltdpR73CMN92E4ninKN4gRPrGM=","Date":"Sat, 1 May 2021 05:45:19 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210413101439.14659-1-jacopo@jmondi.org>\n\t<20210413101439.14659-2-jacopo@jmondi.org>\n\t\n\t<20210427072903.s3fphgofjhlc26vs@uno.localdomain>\n\t\n\t<20210430164341.5ot6qo6xa7hg4o7z@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210430164341.5ot6qo6xa7hg4o7z@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v5 1/3] libcamera: Introduce camera\n\tsensor database","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]