[{"id":16173,"web_url":"https://patchwork.libcamera.org/comment/16173/","msgid":"","date":"2021-04-12T09:10:18","subject":"Re: [libcamera-devel] [PATCH v4 1/3] libcamera: Introduce camera\n\tsensor database","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2021-04-12 09:57:00 +0200, Jacopo Mondi wrote:\n> Introduce a 'database' of camera sensor information, which contains\n> a the camera sensor properties which is not possible, or desirable,\n\ns/^a //\n\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 information.\n> \n> Signed-off-by: Jacopo Mondi \n\nReviewed-by: Niklas Söderlund \n\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 | 97 ++++++++++++++++++++\n> 4 files changed, 127 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..7d743e46102d\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 SensorInfo {\n> +\tSize unitCellSize;\n> +};\n> +\n> +class SensorDatabase\n> +{\n> +public:\n> +\tstatic const SensorInfo *get(const std::string &sensor);\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..68e69e8bf1b6\n> --- /dev/null\n> +++ b/src/libcamera/sensor_database.cpp\n> @@ -0,0 +1,97 @@\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> +namespace libcamera {\n> +\n> +/**\n> + * \\file sensor_database.h\n> + * \\brief Database of camera sensor properties\n> + *\n> + * The camera sensor database collects information on camera sensors\n> + * which is not possible or desirable to retrieve at run-time.\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> + * SensorInfo list of properties.\n> + *\n> + * The class provides a single static getInfo() method to access the sensor\n> + * database entries. If the sensor is not supported in the database nullptr is\n> + * returned.\n> + */\n> +\n> +/**\n> + * \\struct SensorInfo\n> + * \\brief List of sensor properties\n> + *\n> + * \\var SensorInfo::unitCellSize\n> + * \\brief The physical size of pixel, including pixel edges. Width x height in\n> + * nano-meters.\n> + */\n> +\n> +namespace {\n> +\n> +/**\n> + * \\brief Sony IMX219 sensor properties\n> + */\n> +constexpr SensorInfo imx219Info = {\n> +\t.unitCellSize = { 1120, 1120 }\n> +};\n> +\n> +/**\n> + * \\brief Omnivision ov5670 sensor properties\n> + */\n> +constexpr SensorInfo ov5670Info = {\n> +\t.unitCellSize = { 1120, 1120 }\n> +};\n> +\n> +/**\n> + * \\brief Omnivision 13858 sensor properties\n> + */\n> +constexpr SensorInfo ov13858Info = {\n> +\t.unitCellSize = { 1120, 1120 }\n> +};\n> +\n> +#define SENSOR_INFO(_sensor) \\\n> +\t{ #_sensor, &_sensor##Info }\n> +\n> +/*\n> + * \\brief The database of sensor properties\n> + */\n> +constexpr std::pair sensorDatabase__[] = {\n> +\tSENSOR_INFO(imx219),\n> +\tSENSOR_INFO(ov5670),\n> +\tSENSOR_INFO(ov13858),\n> +};\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 SensorInfo instance associated with a sensor or\n> + * nullptr if the sensor is not supported in the database\n> + */\n> +const SensorInfo *SensorDatabase::get(const std::string &sensor)\n> +{\n> +\tfor (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {\n> +\t\tif (sensorDatabase__[i].first == sensor)\n> +\t\t\treturn sensorDatabase__[i].second;\n> +\t}\n> +\n> +\treturn nullptr;\n> +}\n> +\n> +} /* namespace libcamera */\n> -- \n> 2.31.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 B2E61BD1F6\n\tfor ;\n\tMon, 12 Apr 2021 09:10:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2F818687F6;\n\tMon, 12 Apr 2021 11:10:21 +0200 (CEST)","from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2C74E687F5\n\tfor ;\n\tMon, 12 Apr 2021 11:10:20 +0200 (CEST)","by mail-lf1-x135.google.com with SMTP id b4so20178909lfi.6\n\tfor ;\n\tMon, 12 Apr 2021 02:10:20 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\t67sm2245875lfa.108.2021.04.12.02.10.18\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 12 Apr 2021 02:10:19 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"DQKQXeXd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=JmNEVjqCy0D2NWyiGwLPdEZHKz00Gdt3MQFv5GpMnbo=;\n\tb=DQKQXeXdnru3LjAFIctHhihXnFvQTdAP/raYu6ZtsQEH/Po8QmwSSQ00/0P2sRfUsz\n\tXhlQEHyFHqdCzg0Xm0IVYPBi1P2Lf7kMpGlrEDYV7Up460Ebcqz7feH8PtdEb9uWUl3v\n\texIlUFf0gTeYQCkSOlu6WME0Dg4ZmeQOD15tLhlUaeMNN6QIoMObp9FoE1WZy6pPxdQL\n\t1Yk8FiBhjPo/4KAAwoow8sxUMYCy79UM6X20mKCBZn/0pXBVdTqUF7H/pRdbd/VAKk4z\n\thjVIW4j90S0E7F5p3my3W68SfbVPrVN22HdwD7l28hWxJ0+KXvQH01r1qgBT5zRGQTyR\n\txlnA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=JmNEVjqCy0D2NWyiGwLPdEZHKz00Gdt3MQFv5GpMnbo=;\n\tb=JRDuanMjOM7IwZpVcIImsM2VgeR6lDygmpCoIKFOPByO7nv/Ho20NkHrMyUKtF5LGn\n\tI60LwQEknokGFjyfx6WGjrVIguJeNMtNfmzl2kXRfSpo2Ln1kDlRwuocB2YDx0pe/M37\n\tm91zJ2Z5yHb77GRN2q11wGfz/iD9ptUPVIOUW4oCnRw1B1VN4VRO1BUq468qLGxLiJz/\n\tkNs+gn+S0eTdPdZ1odOhOn23svcYEInIJZHScpmcMF4MyUUuDfeMu1XXVjL2pCtFIzQt\n\tXaGgM4eyq4C8uboxpqx2PO5aJqnmcaR5X6BD5012oaG9lQmNqWIIiaZwky22/GYmFfd7\n\tBvrw==","X-Gm-Message-State":"AOAM530SXIOmh8ucnoHEDYUMOH3f9heLRsC0EfeXt4Jo78SYlSL0SQ2P\n\tTUnamPB1hBEchbxvHDERXw+hEw==","X-Google-Smtp-Source":"ABdhPJwaa0UP0V/fML2AT3JpsTK0gY6vxk8azoRFIqhtMHHDVK1y601ZbHmKLujsx3EaSXnKhITYZg==","X-Received":"by 2002:a05:6512:991:: with SMTP id\n\tw17mr19135769lft.85.1618218619569; \n\tMon, 12 Apr 2021 02:10:19 -0700 (PDT)","Date":"Mon, 12 Apr 2021 11:10:18 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210412075702.9955-1-jacopo@jmondi.org>\n\t<20210412075702.9955-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210412075702.9955-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v4 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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16176,"web_url":"https://patchwork.libcamera.org/comment/16176/","msgid":"","date":"2021-04-12T13:07:17","subject":"Re: [libcamera-devel] [PATCH v4 1/3] libcamera: Introduce camera\n\tsensor database","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thanks for the patch.\n\n\nOn Mon, Apr 12, 2021 at 6:10 PM Niklas Söderlund\n wrote:\n>\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2021-04-12 09:57:00 +0200, Jacopo Mondi wrote:\n> > Introduce a 'database' of camera sensor information, which contains\n> > a the camera sensor properties which is not possible, or desirable,\n>\n> s/^a //\n>\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 information.\n> >\n> > Signed-off-by: Jacopo Mondi \n>\n> Reviewed-by: Niklas Söderlund \n>\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 | 97 ++++++++++++++++++++\n> > 4 files changed, 127 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..7d743e46102d\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 SensorInfo {\n> > + Size unitCellSize;\n> > +};\n> > +\n> > +class SensorDatabase\n> > +{\n> > +public:\n> > + static const SensorInfo *get(const std::string &sensor);\n> > +};\n> > +\n\n\nJust FYI: https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions\n> Do not create classes only to group static members; this is no different than just giving the names a common prefix, and such grouping is usually unnecessary anyway.\n\nHowever, I am not against this code. I am personally fine with class +\nstatic member functions.\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..68e69e8bf1b6\n> > --- /dev/null\n> > +++ b/src/libcamera/sensor_database.cpp\n> > @@ -0,0 +1,97 @@\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> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\file sensor_database.h\n> > + * \\brief Database of camera sensor properties\n> > + *\n> > + * The camera sensor database collects information on camera sensors\n> > + * which is not possible or desirable to retrieve at run-time.\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> > + * SensorInfo list of properties.\n> > + *\n> > + * The class provides a single static getInfo() method to access the sensor\n> > + * database entries. If the sensor is not supported in the database nullptr is\n> > + * returned.\n> > + */\n> > +\n> > +/**\n> > + * \\struct SensorInfo\n> > + * \\brief List of sensor properties\n> > + *\n> > + * \\var SensorInfo::unitCellSize\n> > + * \\brief The physical size of pixel, including pixel edges. Width x height in\n> > + * nano-meters.\n> > + */\n> > +\n> > +namespace {\n> > +\n> > +/**\n> > + * \\brief Sony IMX219 sensor properties\n> > + */\n> > +constexpr SensorInfo imx219Info = {\n> > + .unitCellSize = { 1120, 1120 }\n> > +};\n> > +\n> > +/**\n> > + * \\brief Omnivision ov5670 sensor properties\n> > + */\n> > +constexpr SensorInfo ov5670Info = {\n> > + .unitCellSize = { 1120, 1120 }\n> > +};\n> > +\n> > +/**\n> > + * \\brief Omnivision 13858 sensor properties\n> > + */\n> > +constexpr SensorInfo ov13858Info = {\n> > + .unitCellSize = { 1120, 1120 }\n> > +};\n> > +\n> > +#define SENSOR_INFO(_sensor) \\\n> > + { #_sensor, &_sensor##Info }\n> > +\n> > +/*\n> > + * \\brief The database of sensor properties\n> > + */\n> > +constexpr std::pair sensorDatabase__[] = {\n> > + SENSOR_INFO(imx219),\n> > + SENSOR_INFO(ov5670),\n> > + SENSOR_INFO(ov13858),\n> > +};\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 SensorInfo instance associated with a sensor or\n> > + * nullptr if the sensor is not supported in the database\n> > + */\n> > +const SensorInfo *SensorDatabase::get(const std::string &sensor)\n> > +{\n> > + for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {\n> > + if (sensorDatabase__[i].first == sensor)\n> > + return sensorDatabase__[i].second;\n> > + }\n> > +\n> > + return nullptr;\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > --\n> > 2.31.1\n> >\n\nReviewed-by: Hirokazu Honda \n\n-Hiro\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund\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 D5E39BD1F6\n\tfor ;\n\tMon, 12 Apr 2021 13:07:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 42C99687F8;\n\tMon, 12 Apr 2021 15:07:31 +0200 (CEST)","from mail-ed1-x533.google.com (mail-ed1-x533.google.com\n\t[IPv6:2a00:1450:4864:20::533])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4834A602C8\n\tfor ;\n\tMon, 12 Apr 2021 15:07:30 +0200 (CEST)","by mail-ed1-x533.google.com with SMTP id s15so14987243edd.4\n\tfor ;\n\tMon, 12 Apr 2021 06:07:30 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"n+ZwQfPR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=wXaHE3u6cFEvaBJSBpA5KejkFG4MXIToqi9TGAtAVHw=;\n\tb=n+ZwQfPRbkgBD0lVERk/6IZ2Y0jY/5GxVza5Kn8eu15A3bRxcuOBLo+vNJM+0fT3WQ\n\ttZEbwJFmy81bgwr/0dzqukVEQXLokUkNWtKeyYuiJkLRHlgOl8O819LrWdHi6i7Ms5gK\n\tcKas62b1XhIsPLE1EQrhCsdluu0ensJ5+npkM=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=wXaHE3u6cFEvaBJSBpA5KejkFG4MXIToqi9TGAtAVHw=;\n\tb=KnjTacCT4VmDy6UIJOR/jOxyEubTXNlLCO6TApP/tUAmuet6YAr6OCZ5iQNbc38iU3\n\tmWIo89Xtjm3WCO3Vs4QBmKdXw+Fsf8IR1tszfp+AB4HCHeYq3YipPW4IUh6TnFUxej8h\n\tTuOsaYWYyOqC2C+wtgKV0a3cdVDpbpWmKpt5AUzs4GNORRbCmrxa6A/TVznFG4ZjSPqj\n\tq3nbVBysGRUPtFio6VS94aa5O0i4bTvqqBkHTnSfEzRNAXZyGhQkuSRh2kG/CZ9KIGwr\n\tFEhsZX7BSmvndRWC4oq4knWyX/XRR77zCySrhbJslu5vDJAYBKm42HfBD53sdvTOFAld\n\tlKLg==","X-Gm-Message-State":"AOAM531QvUrcgfhEt7TZ4+cWSmTIlYgUA28XTpNLY9azIyLFXQFio8w7\n\tt6Vtd1ebZAaxFccLuzMuttnzwcLSbovTCnYBRpUN5w==","X-Google-Smtp-Source":"ABdhPJzJq+HK+xkmkjxjsbjFC1ICvKx+HfFk1eFpK1yAWan/aOU+QQ0wYWyL2k/i3iRSwJZBnq+IC4Aqmg1T/6wULvU=","X-Received":"by 2002:a05:6402:51cd:: with SMTP id\n\tr13mr29333105edd.116.1618232849853; \n\tMon, 12 Apr 2021 06:07:29 -0700 (PDT)","MIME-Version":"1.0","References":"<20210412075702.9955-1-jacopo@jmondi.org>\n\t<20210412075702.9955-2-jacopo@jmondi.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 12 Apr 2021 22:07:17 +0900","Message-ID":"","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Subject":"Re: [libcamera-devel] [PATCH v4 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 ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16199,"web_url":"https://patchwork.libcamera.org/comment/16199/","msgid":"","date":"2021-04-13T01:44:13","subject":"Re: [libcamera-devel] [PATCH v4 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 Mon, Apr 12, 2021 at 10:07:17PM +0900, Hirokazu Honda wrote:\n> On Mon, Apr 12, 2021 at 6:10 PM Niklas Söderlund wrote:\n> > On 2021-04-12 09:57:00 +0200, Jacopo Mondi wrote:\n> > > Introduce a 'database' of camera sensor information, which contains\n> > > a the camera sensor properties which is not possible, or desirable,\n\ns/is not/are not/\n\n> > s/^a //\n> >\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 information.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> >\n> > Reviewed-by: Niklas Söderlund \n> >\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 | 97 ++++++++++++++++++++\n> > > 4 files changed, 127 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..7d743e46102d\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 SensorInfo {\n\nI'd name this CameraSensorInfo, as we may have to support other types of\nsensor in the future.\n\n> > > + Size unitCellSize;\n> > > +};\n> > > +\n> > > +class SensorDatabase\n> > > +{\n> > > +public:\n> > > + static const SensorInfo *get(const std::string &sensor);\n> > > +};\n> > > +\n> \n> Just FYI: https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions\n> > Do not create classes only to group static members; this is no different than just giving the names a common prefix, and such grouping is usually unnecessary anyway.\n> \n> However, I am not against this code. I am personally fine with class +\n> static member functions.\n\nMoving the static function to the SensorInfo class would solve this\nproblem.\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..68e69e8bf1b6\n> > > --- /dev/null\n> > > +++ b/src/libcamera/sensor_database.cpp\n> > > @@ -0,0 +1,97 @@\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> > > +namespace libcamera {\n\nThe namespace goes after \\file.\n\n> > > +\n> > > +/**\n> > > + * \\file sensor_database.h\n> > > + * \\brief Database of camera sensor properties\n> > > + *\n> > > + * The camera sensor database collects information on camera sensors\n\ns/information on/static information about/\n\n> > > + * which is not possible or desirable to retrieve at run-time.\n\ns/which/that/\ns/run-time/run time/\n\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> > > + * SensorInfo list of properties.\n> > > + *\n> > > + * The class provides a single static getInfo() method to access the sensor\n\nThe function is called get(). But in any case, let's move it to the\nSensorInfo class.\n\n> > > + * database entries. If the sensor is not supported in the database nullptr is\n> > > + * returned.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\struct SensorInfo\n> > > + * \\brief List of sensor properties\n> > > + *\n> > > + * \\var SensorInfo::unitCellSize\n> > > + * \\brief The physical size of pixel, including pixel edges. Width x height in\n\ns/pixel/a pixel/\n\n> > > + * nano-meters.\n\ns/nano-meters/nanometers/\n\nThis field is a Size, so no need to repeat Width x height.\n\n * \\brief The physical size of a pixel, including pixel edges, in nanometers\n\n> > > + */\n> > > +\n> > > +namespace {\n> > > +\n> > > +/**\n> > > + * \\brief Sony IMX219 sensor properties\n> > > + */\n> > > +constexpr SensorInfo imx219Info = {\n> > > + .unitCellSize = { 1120, 1120 }\n> > > +};\n> > > +\n> > > +/**\n> > > + * \\brief Omnivision ov5670 sensor properties\n> > > + */\n> > > +constexpr SensorInfo ov5670Info = {\n> > > + .unitCellSize = { 1120, 1120 }\n> > > +};\n> > > +\n> > > +/**\n> > > + * \\brief Omnivision 13858 sensor properties\n> > > + */\n> > > +constexpr SensorInfo ov13858Info = {\n> > > + .unitCellSize = { 1120, 1120 }\n> > > +};\n> > > +\n> > > +#define SENSOR_INFO(_sensor) \\\n> > > + { #_sensor, &_sensor##Info }\n> > > +\n> > > +/*\n> > > + * \\brief The database of sensor properties\n> > > + */\n> > > +constexpr std::pair sensorDatabase__[] = {\n> > > + SENSOR_INFO(imx219),\n> > > + SENSOR_INFO(ov5670),\n> > > + SENSOR_INFO(ov13858),\n> > > +};\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 SensorInfo instance associated with a sensor or\n> > > + * nullptr if the sensor is not supported in the database\n> > > + */\n> > > +const SensorInfo *SensorDatabase::get(const std::string &sensor)\n> > > +{\n> > > + for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {\n> > > + if (sensorDatabase__[i].first == sensor)\n> > > + return sensorDatabase__[i].second;\n> > > + }\n\nWhy not std::map ?\n\nnamespace {\n\nconst std::map sensorDatabase{\n\t{ \"imx219\", {\n\t\t.unitCellSize = { 1120, 1120 },\n\t} },\n\t...\n};\n\n} /* namespace */\n\nand this function can become\n\nconst CameraSensorInfo *CameraSensorInfo::get(const std::string &sensor)\n{\n\tauto iter = sensorDatabase.find(sensor);\n\tif (iter == sensorDatabase.end())\n\t\treturn nullptr;\n\n\treturn *iter;\n}\n\n> > > +\n> > > + return nullptr;\n> > > +}\n> > > +\n> > > +} /* namespace libcamera */\n> \n> Reviewed-by: Hirokazu Honda ","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 55F9EBD224\n\tfor ;\n\tTue, 13 Apr 2021 01:45:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AF27F687F4;\n\tTue, 13 Apr 2021 03:45:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 21315605AE\n\tfor ;\n\tTue, 13 Apr 2021 03:45:03 +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 890146F2;\n\tTue, 13 Apr 2021 03:45:02 +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=\"urk+suoJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618278302;\n\tbh=zXvV8mj7zBhtJC8uqbCLGWfgk+BRHQuJYqFIjX+lzF0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=urk+suoJL/MCtHJNRKstgEeKsyc1l2jweJfEDIbBKYJej3BiE7Cf3HsNGCf92CVQC\n\tkISxYnQCaLcnwtUnVodEqcBOjrxQfv9JsGqTdl7w45IWGTXsCTJ9X++qhakwduZDBs\n\t1yCr1af59sWIz2C/xInMIWfYuH1WAvGhaJcTgdz8=","Date":"Tue, 13 Apr 2021 04:44:13 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210412075702.9955-1-jacopo@jmondi.org>\n\t<20210412075702.9955-2-jacopo@jmondi.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 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 ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16202,"web_url":"https://patchwork.libcamera.org/comment/16202/","msgid":"","date":"2021-04-13T01:56:40","subject":"Re: [libcamera-devel] [PATCH v4 1/3] libcamera: Introduce camera\n\tsensor database","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Tue, Apr 13, 2021 at 10:45 AM Laurent Pinchart\n wrote:\n>\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Mon, Apr 12, 2021 at 10:07:17PM +0900, Hirokazu Honda wrote:\n> > On Mon, Apr 12, 2021 at 6:10 PM Niklas Söderlund wrote:\n> > > On 2021-04-12 09:57:00 +0200, Jacopo Mondi wrote:\n> > > > Introduce a 'database' of camera sensor information, which contains\n> > > > a the camera sensor properties which is not possible, or desirable,\n>\n> s/is not/are not/\n>\n> > > s/^a //\n> > >\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 information.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > >\n> > > Reviewed-by: Niklas Söderlund \n> > >\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 | 97 ++++++++++++++++++++\n> > > > 4 files changed, 127 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..7d743e46102d\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 SensorInfo {\n>\n> I'd name this CameraSensorInfo, as we may have to support other types of\n> sensor in the future.\n>\n> > > > + Size unitCellSize;\n> > > > +};\n> > > > +\n> > > > +class SensorDatabase\n> > > > +{\n> > > > +public:\n> > > > + static const SensorInfo *get(const std::string &sensor);\n> > > > +};\n> > > > +\n> >\n> > Just FYI: https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions\n> > > Do not create classes only to group static members; this is no different than just giving the names a common prefix, and such grouping is usually unnecessary anyway.\n> >\n> > However, I am not against this code. I am personally fine with class +\n> > static member functions.\n>\n> Moving the static function to the SensorInfo class would solve this\n> problem.\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..68e69e8bf1b6\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/sensor_database.cpp\n> > > > @@ -0,0 +1,97 @@\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> > > > +namespace libcamera {\n>\n> The namespace goes after \\file.\n>\n> > > > +\n> > > > +/**\n> > > > + * \\file sensor_database.h\n> > > > + * \\brief Database of camera sensor properties\n> > > > + *\n> > > > + * The camera sensor database collects information on camera sensors\n>\n> s/information on/static information about/\n>\n> > > > + * which is not possible or desirable to retrieve at run-time.\n>\n> s/which/that/\n> s/run-time/run time/\n>\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> > > > + * SensorInfo list of properties.\n> > > > + *\n> > > > + * The class provides a single static getInfo() method to access the sensor\n>\n> The function is called get(). But in any case, let's move it to the\n> SensorInfo class.\n>\n> > > > + * database entries. If the sensor is not supported in the database nullptr is\n> > > > + * returned.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\struct SensorInfo\n> > > > + * \\brief List of sensor properties\n> > > > + *\n> > > > + * \\var SensorInfo::unitCellSize\n> > > > + * \\brief The physical size of pixel, including pixel edges. Width x height in\n>\n> s/pixel/a pixel/\n>\n> > > > + * nano-meters.\n>\n> s/nano-meters/nanometers/\n>\n> This field is a Size, so no need to repeat Width x height.\n>\n> * \\brief The physical size of a pixel, including pixel edges, in nanometers\n>\n> > > > + */\n> > > > +\n> > > > +namespace {\n> > > > +\n> > > > +/**\n> > > > + * \\brief Sony IMX219 sensor properties\n> > > > + */\n> > > > +constexpr SensorInfo imx219Info = {\n> > > > + .unitCellSize = { 1120, 1120 }\n> > > > +};\n> > > > +\n> > > > +/**\n> > > > + * \\brief Omnivision ov5670 sensor properties\n> > > > + */\n> > > > +constexpr SensorInfo ov5670Info = {\n> > > > + .unitCellSize = { 1120, 1120 }\n> > > > +};\n> > > > +\n> > > > +/**\n> > > > + * \\brief Omnivision 13858 sensor properties\n> > > > + */\n> > > > +constexpr SensorInfo ov13858Info = {\n> > > > + .unitCellSize = { 1120, 1120 }\n> > > > +};\n> > > > +\n> > > > +#define SENSOR_INFO(_sensor) \\\n> > > > + { #_sensor, &_sensor##Info }\n> > > > +\n> > > > +/*\n> > > > + * \\brief The database of sensor properties\n> > > > + */\n> > > > +constexpr std::pair sensorDatabase__[] = {\n> > > > + SENSOR_INFO(imx219),\n> > > > + SENSOR_INFO(ov5670),\n> > > > + SENSOR_INFO(ov13858),\n> > > > +};\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 SensorInfo instance associated with a sensor or\n> > > > + * nullptr if the sensor is not supported in the database\n> > > > + */\n> > > > +const SensorInfo *SensorDatabase::get(const std::string &sensor)\n> > > > +{\n> > > > + for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {\n> > > > + if (sensorDatabase__[i].first == sensor)\n> > > > + return sensorDatabase__[i].second;\n> > > > + }\n>\n> Why not std::map ?\n>\n\nI expect the reason is std::map cannot be constructed as constexpr.\n\n> namespace {\n>\n> const std::map sensorDatabase{\n> { \"imx219\", {\n> .unitCellSize = { 1120, 1120 },\n> } },\n> ...\n> };\n>\n> } /* namespace */\n>\n> and this function can become\n>\n> const CameraSensorInfo *CameraSensorInfo::get(const std::string &sensor)\n> {\n> auto iter = sensorDatabase.find(sensor);\n> if (iter == sensorDatabase.end())\n> return nullptr;\n>\n> return *iter;\n> }\n>\n> > > > +\n> > > > + return nullptr;\n> > > > +}\n> > > > +\n> > > > +} /* namespace libcamera */\n> >\n> > Reviewed-by: Hirokazu Honda \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 9D5A4BD224\n\tfor ;\n\tTue, 13 Apr 2021 01:56:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0FF3D687F4;\n\tTue, 13 Apr 2021 03:56:54 +0200 (CEST)","from mail-ed1-x536.google.com (mail-ed1-x536.google.com\n\t[IPv6:2a00:1450:4864:20::536])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5A295687EC\n\tfor ;\n\tTue, 13 Apr 2021 03:56:52 +0200 (CEST)","by mail-ed1-x536.google.com with SMTP id ba6so17431405edb.1\n\tfor ;\n\tMon, 12 Apr 2021 18:56:52 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"BmlQLPfe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=ku1UnXKtYqKIhlckjnFnB2GM0IiosmARnDpLwXkhWH0=;\n\tb=BmlQLPfeKmgr7awv+esXQfSuQI+KtlDurIKvnDuZ+c2ADToh9cqYrDmlOo9N1LwuIC\n\tljmxZxQRLwpqvMM5upOh0ak+N1183JdE1saqJVLXs3ZDYTaCwZOz7mmIx07vKEHyY7vC\n\t5VycFIl3pkD0UvhYJHkq33MtwkJWwf6L8kENA=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=ku1UnXKtYqKIhlckjnFnB2GM0IiosmARnDpLwXkhWH0=;\n\tb=k1Tz4kAa6ykv0eegwnJVovO0X54wJ3cJ22BJNQFhFDtb1v+yB+ZCz4V7222iuXYPW6\n\tEaj1Dbq4J3bg2wUes6fWtAo7XkfmopmxsPahXFLDP9jtDwiEY2j3/NshoFSkjRHx+HyY\n\tseNDPgoqqcbTJPCMXvD/CuvNJduoqe/180BOjbVED2+chik2XCVPncKXP91rNsV69mcy\n\tg4o7EI79fEc9K4PfZ8WbNG5+t/fXephsSDM7MhVjBX/8DohA3jSSQXgR0VcePnPkFncS\n\tCT4wQrAhBRzlQHh+k8dGYdcaGxWMRck97Hzl826l6Ygz5lUvYzUVZic84nklw0foJ3hX\n\t24Cw==","X-Gm-Message-State":"AOAM531nPsWQQjeko/S93eeUwXIogJ1Rvqb9GjsfU3r0tSq3yUm6c5qE\n\tLd5q0bRwqH71ALuDPy3VKerc+EWOOrQ9EBWNr9z8fTrBrZs=","X-Google-Smtp-Source":"ABdhPJxEYV9Ohe3NtxJ/bOgeDXg1KHycXGUo8I5Xw6t2HJDhQoRsbtPS5R4XSWkJVDNCaCO6adihNvbiCFBNKZGp9uk=","X-Received":"by 2002:a05:6402:5211:: with SMTP id\n\ts17mr32550069edd.327.1618279011906; \n\tMon, 12 Apr 2021 18:56:51 -0700 (PDT)","MIME-Version":"1.0","References":"<20210412075702.9955-1-jacopo@jmondi.org>\n\t<20210412075702.9955-2-jacopo@jmondi.org>\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 13 Apr 2021 10:56:40 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v4 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 ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16203,"web_url":"https://patchwork.libcamera.org/comment/16203/","msgid":"","date":"2021-04-13T02:03:03","subject":"Re: [libcamera-devel] [PATCH v4 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":"On Tue, Apr 13, 2021 at 10:56:40AM +0900, Hirokazu Honda wrote:\n> On Tue, Apr 13, 2021 at 10:45 AM Laurent Pinchart wrote:\n> > On Mon, Apr 12, 2021 at 10:07:17PM +0900, Hirokazu Honda wrote:\n> > > On Mon, Apr 12, 2021 at 6:10 PM Niklas Söderlund wrote:\n> > > > On 2021-04-12 09:57:00 +0200, Jacopo Mondi wrote:\n> > > > > Introduce a 'database' of camera sensor information, which contains\n> > > > > a the camera sensor properties which is not possible, or desirable,\n> >\n> > s/is not/are not/\n> >\n> > > > s/^a //\n> > > >\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 information.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > >\n> > > > Reviewed-by: Niklas Söderlund \n> > > >\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 | 97 ++++++++++++++++++++\n> > > > > 4 files changed, 127 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..7d743e46102d\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 SensorInfo {\n> >\n> > I'd name this CameraSensorInfo, as we may have to support other types of\n> > sensor in the future.\n> >\n> > > > > + Size unitCellSize;\n> > > > > +};\n> > > > > +\n> > > > > +class SensorDatabase\n> > > > > +{\n> > > > > +public:\n> > > > > + static const SensorInfo *get(const std::string &sensor);\n> > > > > +};\n> > > > > +\n> > >\n> > > Just FYI: https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions\n> > > > Do not create classes only to group static members; this is no different than just giving the names a common prefix, and such grouping is usually unnecessary anyway.\n> > >\n> > > However, I am not against this code. I am personally fine with class +\n> > > static member functions.\n> >\n> > Moving the static function to the SensorInfo class would solve this\n> > problem.\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..68e69e8bf1b6\n> > > > > --- /dev/null\n> > > > > +++ b/src/libcamera/sensor_database.cpp\n> > > > > @@ -0,0 +1,97 @@\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> > > > > +namespace libcamera {\n> >\n> > The namespace goes after \\file.\n> >\n> > > > > +\n> > > > > +/**\n> > > > > + * \\file sensor_database.h\n> > > > > + * \\brief Database of camera sensor properties\n> > > > > + *\n> > > > > + * The camera sensor database collects information on camera sensors\n> >\n> > s/information on/static information about/\n> >\n> > > > > + * which is not possible or desirable to retrieve at run-time.\n> >\n> > s/which/that/\n> > s/run-time/run time/\n> >\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> > > > > + * SensorInfo list of properties.\n> > > > > + *\n> > > > > + * The class provides a single static getInfo() method to access the sensor\n> >\n> > The function is called get(). But in any case, let's move it to the\n> > SensorInfo class.\n> >\n> > > > > + * database entries. If the sensor is not supported in the database nullptr is\n> > > > > + * returned.\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\struct SensorInfo\n> > > > > + * \\brief List of sensor properties\n> > > > > + *\n> > > > > + * \\var SensorInfo::unitCellSize\n> > > > > + * \\brief The physical size of pixel, including pixel edges. Width x height in\n> >\n> > s/pixel/a pixel/\n> >\n> > > > > + * nano-meters.\n> >\n> > s/nano-meters/nanometers/\n> >\n> > This field is a Size, so no need to repeat Width x height.\n> >\n> > * \\brief The physical size of a pixel, including pixel edges, in nanometers\n> >\n> > > > > + */\n> > > > > +\n> > > > > +namespace {\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Sony IMX219 sensor properties\n> > > > > + */\n> > > > > +constexpr SensorInfo imx219Info = {\n> > > > > + .unitCellSize = { 1120, 1120 }\n> > > > > +};\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Omnivision ov5670 sensor properties\n> > > > > + */\n> > > > > +constexpr SensorInfo ov5670Info = {\n> > > > > + .unitCellSize = { 1120, 1120 }\n> > > > > +};\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Omnivision 13858 sensor properties\n> > > > > + */\n> > > > > +constexpr SensorInfo ov13858Info = {\n> > > > > + .unitCellSize = { 1120, 1120 }\n> > > > > +};\n> > > > > +\n> > > > > +#define SENSOR_INFO(_sensor) \\\n> > > > > + { #_sensor, &_sensor##Info }\n> > > > > +\n> > > > > +/*\n> > > > > + * \\brief The database of sensor properties\n> > > > > + */\n> > > > > +constexpr std::pair sensorDatabase__[] = {\n> > > > > + SENSOR_INFO(imx219),\n> > > > > + SENSOR_INFO(ov5670),\n> > > > > + SENSOR_INFO(ov13858),\n> > > > > +};\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 SensorInfo instance associated with a sensor or\n> > > > > + * nullptr if the sensor is not supported in the database\n> > > > > + */\n> > > > > +const SensorInfo *SensorDatabase::get(const std::string &sensor)\n> > > > > +{\n> > > > > + for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {\n> > > > > + if (sensorDatabase__[i].first == sensor)\n> > > > > + return sensorDatabase__[i].second;\n> > > > > + }\n> >\n> > Why not std::map ?\n> \n> I expect the reason is std::map cannot be constructed as constexpr.\n\nBut the only function that uses it is get(), which isn't constexpr, so\nis that really a problem ?\n\n> > namespace {\n> >\n> > const std::map sensorDatabase{\n> > { \"imx219\", {\n> > .unitCellSize = { 1120, 1120 },\n> > } },\n> > ...\n> > };\n> >\n> > } /* namespace */\n> >\n> > and this function can become\n> >\n> > const CameraSensorInfo *CameraSensorInfo::get(const std::string &sensor)\n> > {\n> > auto iter = sensorDatabase.find(sensor);\n> > if (iter == sensorDatabase.end())\n> > return nullptr;\n> >\n> > return *iter;\n> > }\n> >\n> > > > > +\n> > > > > + return nullptr;\n> > > > > +}\n> > > > > +\n> > > > > +} /* namespace libcamera */\n> > >\n> > > Reviewed-by: Hirokazu Honda ","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 08D8BBD1F6\n\tfor ;\n\tTue, 13 Apr 2021 02:03:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E4E468801;\n\tTue, 13 Apr 2021 04:03:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EAB48687EC\n\tfor ;\n\tTue, 13 Apr 2021 04:03:52 +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 6E3016F2;\n\tTue, 13 Apr 2021 04:03:52 +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=\"RDct3f9m\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618279432;\n\tbh=LCQ9I6cscdKMo52B1IPSTJUljMX059KhdzbxSG4txeg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RDct3f9mSFO5jgvdutUz7Fn65KBGqVH19xet9dQ+grNNdxBLvxxYP8kaog5mDmFqH\n\tkU0+IjgYSSkqatM/W12njHnv2y/Keqp/axlVqgI5IsXQ3RSs6C875eOAvrsII/S+/Y\n\tc15MFzSQylvaQKccVsi/ZWw7EuQLAIwUk/8BT0Rk=","Date":"Tue, 13 Apr 2021 05:03:03 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210412075702.9955-1-jacopo@jmondi.org>\n\t<20210412075702.9955-2-jacopo@jmondi.org>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 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 ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16204,"web_url":"https://patchwork.libcamera.org/comment/16204/","msgid":"","date":"2021-04-13T02:19:03","subject":"Re: [libcamera-devel] [PATCH v4 1/3] libcamera: Introduce camera\n\tsensor database","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Tue, Apr 13, 2021 at 11:03 AM Laurent Pinchart\n wrote:\n>\n> On Tue, Apr 13, 2021 at 10:56:40AM +0900, Hirokazu Honda wrote:\n> > On Tue, Apr 13, 2021 at 10:45 AM Laurent Pinchart wrote:\n> > > On Mon, Apr 12, 2021 at 10:07:17PM +0900, Hirokazu Honda wrote:\n> > > > On Mon, Apr 12, 2021 at 6:10 PM Niklas Söderlund wrote:\n> > > > > On 2021-04-12 09:57:00 +0200, Jacopo Mondi wrote:\n> > > > > > Introduce a 'database' of camera sensor information, which contains\n> > > > > > a the camera sensor properties which is not possible, or desirable,\n> > >\n> > > s/is not/are not/\n> > >\n> > > > > s/^a //\n> > > > >\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 information.\n> > > > > >\n> > > > > > Signed-off-by: Jacopo Mondi \n> > > > >\n> > > > > Reviewed-by: Niklas Söderlund \n> > > > >\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 | 97 ++++++++++++++++++++\n> > > > > > 4 files changed, 127 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..7d743e46102d\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 SensorInfo {\n> > >\n> > > I'd name this CameraSensorInfo, as we may have to support other types of\n> > > sensor in the future.\n> > >\n> > > > > > + Size unitCellSize;\n> > > > > > +};\n> > > > > > +\n> > > > > > +class SensorDatabase\n> > > > > > +{\n> > > > > > +public:\n> > > > > > + static const SensorInfo *get(const std::string &sensor);\n> > > > > > +};\n> > > > > > +\n> > > >\n> > > > Just FYI: https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions\n> > > > > Do not create classes only to group static members; this is no different than just giving the names a common prefix, and such grouping is usually unnecessary anyway.\n> > > >\n> > > > However, I am not against this code. I am personally fine with class +\n> > > > static member functions.\n> > >\n> > > Moving the static function to the SensorInfo class would solve this\n> > > problem.\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..68e69e8bf1b6\n> > > > > > --- /dev/null\n> > > > > > +++ b/src/libcamera/sensor_database.cpp\n> > > > > > @@ -0,0 +1,97 @@\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> > > > > > +namespace libcamera {\n> > >\n> > > The namespace goes after \\file.\n> > >\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\file sensor_database.h\n> > > > > > + * \\brief Database of camera sensor properties\n> > > > > > + *\n> > > > > > + * The camera sensor database collects information on camera sensors\n> > >\n> > > s/information on/static information about/\n> > >\n> > > > > > + * which is not possible or desirable to retrieve at run-time.\n> > >\n> > > s/which/that/\n> > > s/run-time/run time/\n> > >\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> > > > > > + * SensorInfo list of properties.\n> > > > > > + *\n> > > > > > + * The class provides a single static getInfo() method to access the sensor\n> > >\n> > > The function is called get(). But in any case, let's move it to the\n> > > SensorInfo class.\n> > >\n> > > > > > + * database entries. If the sensor is not supported in the database nullptr is\n> > > > > > + * returned.\n> > > > > > + */\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\struct SensorInfo\n> > > > > > + * \\brief List of sensor properties\n> > > > > > + *\n> > > > > > + * \\var SensorInfo::unitCellSize\n> > > > > > + * \\brief The physical size of pixel, including pixel edges. Width x height in\n> > >\n> > > s/pixel/a pixel/\n> > >\n> > > > > > + * nano-meters.\n> > >\n> > > s/nano-meters/nanometers/\n> > >\n> > > This field is a Size, so no need to repeat Width x height.\n> > >\n> > > * \\brief The physical size of a pixel, including pixel edges, in nanometers\n> > >\n> > > > > > + */\n> > > > > > +\n> > > > > > +namespace {\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\brief Sony IMX219 sensor properties\n> > > > > > + */\n> > > > > > +constexpr SensorInfo imx219Info = {\n> > > > > > + .unitCellSize = { 1120, 1120 }\n> > > > > > +};\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\brief Omnivision ov5670 sensor properties\n> > > > > > + */\n> > > > > > +constexpr SensorInfo ov5670Info = {\n> > > > > > + .unitCellSize = { 1120, 1120 }\n> > > > > > +};\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\brief Omnivision 13858 sensor properties\n> > > > > > + */\n> > > > > > +constexpr SensorInfo ov13858Info = {\n> > > > > > + .unitCellSize = { 1120, 1120 }\n> > > > > > +};\n> > > > > > +\n> > > > > > +#define SENSOR_INFO(_sensor) \\\n> > > > > > + { #_sensor, &_sensor##Info }\n> > > > > > +\n> > > > > > +/*\n> > > > > > + * \\brief The database of sensor properties\n> > > > > > + */\n> > > > > > +constexpr std::pair sensorDatabase__[] = {\n> > > > > > + SENSOR_INFO(imx219),\n> > > > > > + SENSOR_INFO(ov5670),\n> > > > > > + SENSOR_INFO(ov13858),\n> > > > > > +};\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 SensorInfo instance associated with a sensor or\n> > > > > > + * nullptr if the sensor is not supported in the database\n> > > > > > + */\n> > > > > > +const SensorInfo *SensorDatabase::get(const std::string &sensor)\n> > > > > > +{\n> > > > > > + for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {\n> > > > > > + if (sensorDatabase__[i].first == sensor)\n> > > > > > + return sensorDatabase__[i].second;\n> > > > > > + }\n> > >\n> > > Why not std::map ?\n> >\n> > I expect the reason is std::map cannot be constructed as constexpr.\n>\n> But the only function that uses it is get(), which isn't constexpr, so\n> is that really a problem ?\n>\n\nGlobal and static function should be trivially destructible.\nhttps://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables\n\n> > > namespace {\n> > >\n> > > const std::map sensorDatabase{\n> > > { \"imx219\", {\n> > > .unitCellSize = { 1120, 1120 },\n> > > } },\n> > > ...\n> > > };\n> > >\n> > > } /* namespace */\n> > >\n> > > and this function can become\n> > >\n> > > const CameraSensorInfo *CameraSensorInfo::get(const std::string &sensor)\n> > > {\n> > > auto iter = sensorDatabase.find(sensor);\n> > > if (iter == sensorDatabase.end())\n> > > return nullptr;\n> > >\n> > > return *iter;\n> > > }\n> > >\n> > > > > > +\n> > > > > > + return nullptr;\n> > > > > > +}\n> > > > > > +\n> > > > > > +} /* namespace libcamera */\n> > > >\n> > > > Reviewed-by: Hirokazu Honda \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 839DCBD224\n\tfor ;\n\tTue, 13 Apr 2021 02:19:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F0886687F3;\n\tTue, 13 Apr 2021 04:19:15 +0200 (CEST)","from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com\n\t[IPv6:2a00:1450:4864:20::52a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E8E7B687EC\n\tfor ;\n\tTue, 13 Apr 2021 04:19:14 +0200 (CEST)","by mail-ed1-x52a.google.com with SMTP id f8so17470929edd.11\n\tfor ;\n\tMon, 12 Apr 2021 19:19:14 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"HszwbUD/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=W5XWUdcx+8kiri16Bii0GhwkOkO0ZGGQ2n93yU7Biag=;\n\tb=HszwbUD/gAaTmdBn56n/hL1fXX74Q3nS1IQrwHbToGhSADg2nVhyFyZMTQmEqfhC/v\n\tRzzGd+ADR+UkVA+ZChrKnSuGRSu2kTIRQyl11xAaII8ed0VSQmi0JUTLld57wfzIG+De\n\tBvBoihxfG3p0cQ8EFmI0TBsSA7WFMSqbPzqm4=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=W5XWUdcx+8kiri16Bii0GhwkOkO0ZGGQ2n93yU7Biag=;\n\tb=pWD6smLn9A7mY77z+7WNU5qf3yjluxqwlkZspbSrezbLxwhcvwtEB19GBBnRopeTJ6\n\ty5eLu4Cc6hVcv1KR31x+VDcykWDkWEAKFZ7+Gs1qoXlgDimpLlOOBg5l9yhKCvfXkKQR\n\tb81OZf1a//C/o7DQFoKdovbtruSH5sTPxLCXphw14IRhgn/VxwYdKyDPWgGY0m1MmY6M\n\tJT7NvfhXyhSUjGTkuDelOhwmZ5+CBVMMWRpP0m1rRVSaqQBjExI0DG5AOFTTp7+b0vYh\n\tZZHj0QMZS0eesqUBkDdKZ9qwEnVF0HJVECjaXijGaZkI6yUxVUCnjYhbs1+0px0W3ivL\n\tcviw==","X-Gm-Message-State":"AOAM5329pHTbDvqanwCV1Zyi+VEabZKd+O+1fnSfkm1XcEPIQbo4blS0\n\thBYI0erZsnKVn0tXZ/I4/DMLp52nneIYeRt9goyZKw==","X-Google-Smtp-Source":"ABdhPJwJb3wAtpDAl2cb1CmpFa5VkGY3rnsguFkb4BVlv5AhDXCqAqcDhJc20VTssJALZ4tyWSVSDCLkyVh7PiKKw0Q=","X-Received":"by 2002:a05:6402:5211:: with SMTP id\n\ts17mr32624309edd.327.1618280354516; \n\tMon, 12 Apr 2021 19:19:14 -0700 (PDT)","MIME-Version":"1.0","References":"<20210412075702.9955-1-jacopo@jmondi.org>\n\t<20210412075702.9955-2-jacopo@jmondi.org>\n\t\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 13 Apr 2021 11:19:03 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v4 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 ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16206,"web_url":"https://patchwork.libcamera.org/comment/16206/","msgid":"","date":"2021-04-13T02:42:09","subject":"Re: [libcamera-devel] [PATCH v4 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 Hiro,\n\nOn Tue, Apr 13, 2021 at 11:19:03AM +0900, Hirokazu Honda wrote:\n> On Tue, Apr 13, 2021 at 11:03 AM Laurent Pinchart wrote:\n> > On Tue, Apr 13, 2021 at 10:56:40AM +0900, Hirokazu Honda wrote:\n> > > On Tue, Apr 13, 2021 at 10:45 AM Laurent Pinchart wrote:\n> > > > On Mon, Apr 12, 2021 at 10:07:17PM +0900, Hirokazu Honda wrote:\n> > > > > On Mon, Apr 12, 2021 at 6:10 PM Niklas Söderlund wrote:\n> > > > > > On 2021-04-12 09:57:00 +0200, Jacopo Mondi wrote:\n> > > > > > > Introduce a 'database' of camera sensor information, which contains\n> > > > > > > a the camera sensor properties which is not possible, or desirable,\n> > > >\n> > > > s/is not/are not/\n> > > >\n> > > > > > s/^a //\n> > > > > >\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 information.\n> > > > > > >\n> > > > > > > Signed-off-by: Jacopo Mondi \n> > > > > >\n> > > > > > Reviewed-by: Niklas Söderlund \n> > > > > >\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 | 97 ++++++++++++++++++++\n> > > > > > > 4 files changed, 127 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..7d743e46102d\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 SensorInfo {\n> > > >\n> > > > I'd name this CameraSensorInfo, as we may have to support other types of\n> > > > sensor in the future.\n> > > >\n> > > > > > > + Size unitCellSize;\n> > > > > > > +};\n> > > > > > > +\n> > > > > > > +class SensorDatabase\n> > > > > > > +{\n> > > > > > > +public:\n> > > > > > > + static const SensorInfo *get(const std::string &sensor);\n> > > > > > > +};\n> > > > > > > +\n> > > > >\n> > > > > Just FYI: https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions\n> > > > > > Do not create classes only to group static members; this is no different than just giving the names a common prefix, and such grouping is usually unnecessary anyway.\n> > > > >\n> > > > > However, I am not against this code. I am personally fine with class +\n> > > > > static member functions.\n> > > >\n> > > > Moving the static function to the SensorInfo class would solve this\n> > > > problem.\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..68e69e8bf1b6\n> > > > > > > --- /dev/null\n> > > > > > > +++ b/src/libcamera/sensor_database.cpp\n> > > > > > > @@ -0,0 +1,97 @@\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> > > > > > > +namespace libcamera {\n> > > >\n> > > > The namespace goes after \\file.\n> > > >\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\file sensor_database.h\n> > > > > > > + * \\brief Database of camera sensor properties\n> > > > > > > + *\n> > > > > > > + * The camera sensor database collects information on camera sensors\n> > > >\n> > > > s/information on/static information about/\n> > > >\n> > > > > > > + * which is not possible or desirable to retrieve at run-time.\n> > > >\n> > > > s/which/that/\n> > > > s/run-time/run time/\n> > > >\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> > > > > > > + * SensorInfo list of properties.\n> > > > > > > + *\n> > > > > > > + * The class provides a single static getInfo() method to access the sensor\n> > > >\n> > > > The function is called get(). But in any case, let's move it to the\n> > > > SensorInfo class.\n> > > >\n> > > > > > > + * database entries. If the sensor is not supported in the database nullptr is\n> > > > > > > + * returned.\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\struct SensorInfo\n> > > > > > > + * \\brief List of sensor properties\n> > > > > > > + *\n> > > > > > > + * \\var SensorInfo::unitCellSize\n> > > > > > > + * \\brief The physical size of pixel, including pixel edges. Width x height in\n> > > >\n> > > > s/pixel/a pixel/\n> > > >\n> > > > > > > + * nano-meters.\n> > > >\n> > > > s/nano-meters/nanometers/\n> > > >\n> > > > This field is a Size, so no need to repeat Width x height.\n> > > >\n> > > > * \\brief The physical size of a pixel, including pixel edges, in nanometers\n> > > >\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > +namespace {\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\brief Sony IMX219 sensor properties\n> > > > > > > + */\n> > > > > > > +constexpr SensorInfo imx219Info = {\n> > > > > > > + .unitCellSize = { 1120, 1120 }\n> > > > > > > +};\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\brief Omnivision ov5670 sensor properties\n> > > > > > > + */\n> > > > > > > +constexpr SensorInfo ov5670Info = {\n> > > > > > > + .unitCellSize = { 1120, 1120 }\n> > > > > > > +};\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\brief Omnivision 13858 sensor properties\n> > > > > > > + */\n> > > > > > > +constexpr SensorInfo ov13858Info = {\n> > > > > > > + .unitCellSize = { 1120, 1120 }\n> > > > > > > +};\n> > > > > > > +\n> > > > > > > +#define SENSOR_INFO(_sensor) \\\n> > > > > > > + { #_sensor, &_sensor##Info }\n> > > > > > > +\n> > > > > > > +/*\n> > > > > > > + * \\brief The database of sensor properties\n> > > > > > > + */\n> > > > > > > +constexpr std::pair sensorDatabase__[] = {\n> > > > > > > + SENSOR_INFO(imx219),\n> > > > > > > + SENSOR_INFO(ov5670),\n> > > > > > > + SENSOR_INFO(ov13858),\n> > > > > > > +};\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 SensorInfo instance associated with a sensor or\n> > > > > > > + * nullptr if the sensor is not supported in the database\n> > > > > > > + */\n> > > > > > > +const SensorInfo *SensorDatabase::get(const std::string &sensor)\n> > > > > > > +{\n> > > > > > > + for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {\n> > > > > > > + if (sensorDatabase__[i].first == sensor)\n> > > > > > > + return sensorDatabase__[i].second;\n> > > > > > > + }\n> > > >\n> > > > Why not std::map ?\n> > >\n> > > I expect the reason is std::map cannot be constructed as constexpr.\n> >\n> > But the only function that uses it is get(), which isn't constexpr, so\n> > is that really a problem ?\n> \n> Global and static function should be trivially destructible.\n> https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables\n\nWe have a more relaxed rule, documented in\nDocumentation/coding_style.rst. In this case, a map is much more\nconvenient, and as its initialization and destruction is independent of\nall other global variables, it doesn't risk causing most of the issues\nmentioned in the above link.\n\n> > > > namespace {\n> > > >\n> > > > const std::map sensorDatabase{\n> > > > { \"imx219\", {\n> > > > .unitCellSize = { 1120, 1120 },\n> > > > } },\n> > > > ...\n> > > > };\n> > > >\n> > > > } /* namespace */\n> > > >\n> > > > and this function can become\n> > > >\n> > > > const CameraSensorInfo *CameraSensorInfo::get(const std::string &sensor)\n> > > > {\n> > > > auto iter = sensorDatabase.find(sensor);\n> > > > if (iter == sensorDatabase.end())\n> > > > return nullptr;\n> > > >\n> > > > return *iter;\n> > > > }\n> > > >\n> > > > > > > +\n> > > > > > > + return nullptr;\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > +} /* namespace libcamera */\n> > > > >\n> > > > > Reviewed-by: Hirokazu Honda ","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 82615BD224\n\tfor ;\n\tTue, 13 Apr 2021 02:43:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D8492687F6;\n\tTue, 13 Apr 2021 04:43:00 +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 C1110687EC\n\tfor ;\n\tTue, 13 Apr 2021 04:42:59 +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 214F96F2;\n\tTue, 13 Apr 2021 04:42:59 +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=\"J4h3i6RX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618281779;\n\tbh=k21Lj/mVAN0wWixJ2H06Db8P5Iy3t8uwLwA1RufR7Xo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=J4h3i6RXIzk4qYm1rSgE1AMx8tBvvmWuTG7ltXkvVx/Jq1EScQaEyiixVmttOOTKL\n\tZXje7SdDOFB93cdTWZ9hsNdHehNOchvw/9s7mRnhWDWOco9VjngTJxn3T/uqv6l/iE\n\taTf6DVoAhgwefPrGkbpdBoG9q+mFT9tJHJ1A5yos=","Date":"Tue, 13 Apr 2021 05:42:09 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210412075702.9955-1-jacopo@jmondi.org>\n\t<20210412075702.9955-2-jacopo@jmondi.org>\n\t\n\t\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 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 ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16216,"web_url":"https://patchwork.libcamera.org/comment/16216/","msgid":"","date":"2021-04-13T07:00:25","subject":"Re: [libcamera-devel] [PATCH v4 1/3] libcamera: Introduce camera\n\tsensor database","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Tue, Apr 13, 2021 at 11:42 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Tue, Apr 13, 2021 at 11:19:03AM +0900, Hirokazu Honda wrote:\n> > On Tue, Apr 13, 2021 at 11:03 AM Laurent Pinchart wrote:\n> > > On Tue, Apr 13, 2021 at 10:56:40AM +0900, Hirokazu Honda wrote:\n> > > > On Tue, Apr 13, 2021 at 10:45 AM Laurent Pinchart wrote:\n> > > > > On Mon, Apr 12, 2021 at 10:07:17PM +0900, Hirokazu Honda wrote:\n> > > > > > On Mon, Apr 12, 2021 at 6:10 PM Niklas Söderlund wrote:\n> > > > > > > On 2021-04-12 09:57:00 +0200, Jacopo Mondi wrote:\n> > > > > > > > Introduce a 'database' of camera sensor information, which contains\n> > > > > > > > a the camera sensor properties which is not possible, or desirable,\n> > > > >\n> > > > > s/is not/are not/\n> > > > >\n> > > > > > > s/^a //\n> > > > > > >\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 information.\n> > > > > > > >\n> > > > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > >\n> > > > > > > Reviewed-by: Niklas Söderlund \n> > > > > > >\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 | 97 ++++++++++++++++++++\n> > > > > > > > 4 files changed, 127 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..7d743e46102d\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 SensorInfo {\n> > > > >\n> > > > > I'd name this CameraSensorInfo, as we may have to support other types of\n> > > > > sensor in the future.\n> > > > >\n> > > > > > > > + Size unitCellSize;\n> > > > > > > > +};\n> > > > > > > > +\n> > > > > > > > +class SensorDatabase\n> > > > > > > > +{\n> > > > > > > > +public:\n> > > > > > > > + static const SensorInfo *get(const std::string &sensor);\n> > > > > > > > +};\n> > > > > > > > +\n> > > > > >\n> > > > > > Just FYI: https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions\n> > > > > > > Do not create classes only to group static members; this is no different than just giving the names a common prefix, and such grouping is usually unnecessary anyway.\n> > > > > >\n> > > > > > However, I am not against this code. I am personally fine with class +\n> > > > > > static member functions.\n> > > > >\n> > > > > Moving the static function to the SensorInfo class would solve this\n> > > > > problem.\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..68e69e8bf1b6\n> > > > > > > > --- /dev/null\n> > > > > > > > +++ b/src/libcamera/sensor_database.cpp\n> > > > > > > > @@ -0,0 +1,97 @@\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> > > > > > > > +namespace libcamera {\n> > > > >\n> > > > > The namespace goes after \\file.\n> > > > >\n> > > > > > > > +\n> > > > > > > > +/**\n> > > > > > > > + * \\file sensor_database.h\n> > > > > > > > + * \\brief Database of camera sensor properties\n> > > > > > > > + *\n> > > > > > > > + * The camera sensor database collects information on camera sensors\n> > > > >\n> > > > > s/information on/static information about/\n> > > > >\n> > > > > > > > + * which is not possible or desirable to retrieve at run-time.\n> > > > >\n> > > > > s/which/that/\n> > > > > s/run-time/run time/\n> > > > >\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> > > > > > > > + * SensorInfo list of properties.\n> > > > > > > > + *\n> > > > > > > > + * The class provides a single static getInfo() method to access the sensor\n> > > > >\n> > > > > The function is called get(). But in any case, let's move it to the\n> > > > > SensorInfo class.\n> > > > >\n> > > > > > > > + * database entries. If the sensor is not supported in the database nullptr is\n> > > > > > > > + * returned.\n> > > > > > > > + */\n> > > > > > > > +\n> > > > > > > > +/**\n> > > > > > > > + * \\struct SensorInfo\n> > > > > > > > + * \\brief List of sensor properties\n> > > > > > > > + *\n> > > > > > > > + * \\var SensorInfo::unitCellSize\n> > > > > > > > + * \\brief The physical size of pixel, including pixel edges. Width x height in\n> > > > >\n> > > > > s/pixel/a pixel/\n> > > > >\n> > > > > > > > + * nano-meters.\n> > > > >\n> > > > > s/nano-meters/nanometers/\n> > > > >\n> > > > > This field is a Size, so no need to repeat Width x height.\n> > > > >\n> > > > > * \\brief The physical size of a pixel, including pixel edges, in nanometers\n> > > > >\n> > > > > > > > + */\n> > > > > > > > +\n> > > > > > > > +namespace {\n> > > > > > > > +\n> > > > > > > > +/**\n> > > > > > > > + * \\brief Sony IMX219 sensor properties\n> > > > > > > > + */\n> > > > > > > > +constexpr SensorInfo imx219Info = {\n> > > > > > > > + .unitCellSize = { 1120, 1120 }\n> > > > > > > > +};\n> > > > > > > > +\n> > > > > > > > +/**\n> > > > > > > > + * \\brief Omnivision ov5670 sensor properties\n> > > > > > > > + */\n> > > > > > > > +constexpr SensorInfo ov5670Info = {\n> > > > > > > > + .unitCellSize = { 1120, 1120 }\n> > > > > > > > +};\n> > > > > > > > +\n> > > > > > > > +/**\n> > > > > > > > + * \\brief Omnivision 13858 sensor properties\n> > > > > > > > + */\n> > > > > > > > +constexpr SensorInfo ov13858Info = {\n> > > > > > > > + .unitCellSize = { 1120, 1120 }\n> > > > > > > > +};\n> > > > > > > > +\n> > > > > > > > +#define SENSOR_INFO(_sensor) \\\n> > > > > > > > + { #_sensor, &_sensor##Info }\n> > > > > > > > +\n> > > > > > > > +/*\n> > > > > > > > + * \\brief The database of sensor properties\n> > > > > > > > + */\n> > > > > > > > +constexpr std::pair sensorDatabase__[] = {\n> > > > > > > > + SENSOR_INFO(imx219),\n> > > > > > > > + SENSOR_INFO(ov5670),\n> > > > > > > > + SENSOR_INFO(ov13858),\n> > > > > > > > +};\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 SensorInfo instance associated with a sensor or\n> > > > > > > > + * nullptr if the sensor is not supported in the database\n> > > > > > > > + */\n> > > > > > > > +const SensorInfo *SensorDatabase::get(const std::string &sensor)\n> > > > > > > > +{\n> > > > > > > > + for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {\n> > > > > > > > + if (sensorDatabase__[i].first == sensor)\n> > > > > > > > + return sensorDatabase__[i].second;\n> > > > > > > > + }\n> > > > >\n> > > > > Why not std::map ?\n> > > >\n> > > > I expect the reason is std::map cannot be constructed as constexpr.\n> > >\n> > > But the only function that uses it is get(), which isn't constexpr, so\n> > > is that really a problem ?\n> >\n> > Global and static function should be trivially destructible.\n> > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables\n>\n> We have a more relaxed rule, documented in\n> Documentation/coding_style.rst. In this case, a map is much more\n> convenient, and as its initialization and destruction is independent of\n> all other global variables, it doesn't risk causing most of the issues\n> mentioned in the above link.\n>\n\nThanks. I don't remember the description, although I think I read it previously.\nThat sounds good to me. Perhaps, the map would be put as a const\nstatic variable within CameraSensorInfo::get().\n\n> > > > > namespace {\n> > > > >\n> > > > > const std::map sensorDatabase{\n> > > > > { \"imx219\", {\n> > > > > .unitCellSize = { 1120, 1120 },\n> > > > > } },\n> > > > > ...\n> > > > > };\n> > > > >\n> > > > > } /* namespace */\n> > > > >\n> > > > > and this function can become\n> > > > >\n> > > > > const CameraSensorInfo *CameraSensorInfo::get(const std::string &sensor)\n> > > > > {\n> > > > > auto iter = sensorDatabase.find(sensor);\n> > > > > if (iter == sensorDatabase.end())\n> > > > > return nullptr;\n> > > > >\n> > > > > return *iter;\n> > > > > }\n> > > > >\n> > > > > > > > +\n> > > > > > > > + return nullptr;\n> > > > > > > > +}\n> > > > > > > > +\n> > > > > > > > +} /* namespace libcamera */\n> > > > > >\n> > > > > > Reviewed-by: Hirokazu Honda \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 DA205BD1F6\n\tfor ;\n\tTue, 13 Apr 2021 07:00:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 40EEC687FE;\n\tTue, 13 Apr 2021 09:00:38 +0200 (CEST)","from mail-ed1-x531.google.com (mail-ed1-x531.google.com\n\t[IPv6:2a00:1450:4864:20::531])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C8F97687F3\n\tfor ;\n\tTue, 13 Apr 2021 09:00:36 +0200 (CEST)","by mail-ed1-x531.google.com with SMTP id x4so18129648edd.2\n\tfor ;\n\tTue, 13 Apr 2021 00:00:36 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"mzQzmyKg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=WfNoVk28wUbq6AQL0BKRXaPOmK30FbHIPKKWDCMreG4=;\n\tb=mzQzmyKgg9L5l1KgPs/C4/74HRckTEz/PoCFRgqLcDByhMm6KQGqEasj5K9qnnMVQ7\n\t31iWtLc14J8a4ooSoEO/TF8I0KY/NPr09urZX6DDxfXbrgrAXGTtsBRoTTA6N4hdP3A6\n\tKUy/LaSP983+wNY08nYw1KPa3UMxoOMDTlkrc=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=WfNoVk28wUbq6AQL0BKRXaPOmK30FbHIPKKWDCMreG4=;\n\tb=ZdlQ1jizudlU6DUpchwu7KB0pQcIjPKIUvEZh7cAQR1FJW1WXHDdgYn7PcoaqucbFJ\n\tudaQqL/U41O8j6nR0X88jANpXNw7eLRYjgyawR0Nn19kc/qqTkdCKBNRdrXpVKQiZLT9\n\tn0L8BJ/JGQ5KDjyKYez0SopIUpx/ajPSt7A49p9nZOEN02ZjJF94m4Wurkh5DXVnvDWj\n\texiTjjWKdvbgwn5Qtw2EIJsS5RDEbsHSsUDrIFcsmNCVH9rytcANRs2mh0grxo9FHqaq\n\tcLcueRCMrgthPBxBoubA9c0I9yYIEvkwrT0WnHW0FYWRaTz96X+XVpgkiUv9O6qtcihe\n\tu5Gw==","X-Gm-Message-State":"AOAM532h9T4v7ZJtkmvkPpA0rb4hswhIOfLNYIAL44Z1qDnvor7sl3rj\n\tFFcXp9/UNzP7YFjExQp+bQGHFppeogOAUR70DF3BWQ==","X-Google-Smtp-Source":"ABdhPJw2timc0T6J884+VIc7d+Rk7QwEHPdz7VUwWnVN4MVfZ8yHnGzpeAfymB4RiCeJD9E4OYxnGEWM4/gssYGNT0c=","X-Received":"by 2002:aa7:c7da:: with SMTP id\n\to26mr33334891eds.244.1618297236256; \n\tTue, 13 Apr 2021 00:00:36 -0700 (PDT)","MIME-Version":"1.0","References":"<20210412075702.9955-1-jacopo@jmondi.org>\n\t<20210412075702.9955-2-jacopo@jmondi.org>\n\t\n\t\n\t\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 13 Apr 2021 16:00:25 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v4 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 ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16217,"web_url":"https://patchwork.libcamera.org/comment/16217/","msgid":"<20210413073950.6qqresqtyqtazvbu@uno.localdomain>","date":"2021-04-13T07:39:50","subject":"Re: [libcamera-devel] [PATCH v4 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":"Hello,\n\nOn Tue, Apr 13, 2021 at 04:00:25PM +0900, Hirokazu Honda wrote:\n> > > > > > > > > +\n> > > > > > > > > +/**\n> > > > > > > > > + * \\brief Retrieve the properties associated with a sensor\n> > > > > > > > > + * \\param sensor The sensor model name\n> > > > > > > > > + * \\return A pointer to the SensorInfo instance associated with a sensor or\n> > > > > > > > > + * nullptr if the sensor is not supported in the database\n> > > > > > > > > + */\n> > > > > > > > > +const SensorInfo *SensorDatabase::get(const std::string &sensor)\n> > > > > > > > > +{\n> > > > > > > > > + for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {\n> > > > > > > > > + if (sensorDatabase__[i].first == sensor)\n> > > > > > > > > + return sensorDatabase__[i].second;\n> > > > > > > > > + }\n> > > > > >\n> > > > > > Why not std::map ?\n> > > > >\n> > > > > I expect the reason is std::map cannot be constructed as constexpr.\n> > > >\n> > > > But the only function that uses it is get(), which isn't constexpr, so\n> > > > is that really a problem ?\n> > >\n> > > Global and static function should be trivially destructible.\n> > > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables\n> >\n> > We have a more relaxed rule, documented in\n> > Documentation/coding_style.rst. In this case, a map is much more\n> > convenient, and as its initialization and destruction is independent of\n> > all other global variables, it doesn't risk causing most of the issues\n> > mentioned in the above link.\n> >\n\nIndeed I used std::pair to be able to declare it as constexpr.\n\nI'll make it a map if that's a not a requirement. I'll see if it's\npossbile to statically initialize it as I'm currently doing with the\npair\n\n>\n> Thanks. I don't remember the description, although I think I read it previously.\n> That sounds good to me. Perhaps, the map would be put as a const\n> static variable within CameraSensorInfo::get().\n>\n\nThe map needs to be statically initialized, it could be done inside\nthe ::get() function most probably. Something like (not tested)\n\nconst SensorInfo *SensorDatabase::get(const std::string &sensor)\n{\n\tstatic const std::map sensorDb = {\n\t\tSENSOR_INFO(imx219),\n\t\tSENSOR_INFO(ov5670),\n\t\tSENSOR_INFO(ov13858),\n\t};\n\n\tconst auto it = sensorDb.find(sensor.c_str());\n\tif (it == sensorDb.end())\n\t\treturn nullptr;\n\n\treturn it->second;\n}\n\nThanks\n j\n\n\n\n> > > > > > namespace {\n> > > > > >\n> > > > > > const std::map sensorDatabase{\n> > > > > > { \"imx219\", {\n> > > > > > .unitCellSize = { 1120, 1120 },\n> > > > > > } },\n> > > > > > ...\n> > > > > > };\n> > > > > >\n> > > > > > } /* namespace */\n> > > > > >\n> > > > > > and this function can become\n> > > > > >\n> > > > > > const CameraSensorInfo *CameraSensorInfo::get(const std::string &sensor)\n> > > > > > {\n> > > > > > auto iter = sensorDatabase.find(sensor);\n> > > > > > if (iter == sensorDatabase.end())\n> > > > > > return nullptr;\n> > > > > >\n> > > > > > return *iter;\n> > > > > > }\n> > > > > >\n> > > > > > > > > +\n> > > > > > > > > + return nullptr;\n> > > > > > > > > +}\n> > > > > > > > > +\n> > > > > > > > > +} /* namespace libcamera */\n> > > > > > >\n> > > > > > > Reviewed-by: Hirokazu Honda \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 1B6E7BD224\n\tfor ;\n\tTue, 13 Apr 2021 07:39:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9BD1B687F4;\n\tTue, 13 Apr 2021 09:39:13 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6ABA3687F3\n\tfor ;\n\tTue, 13 Apr 2021 09:39:12 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id AF247E0005;\n\tTue, 13 Apr 2021 07:39:11 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 13 Apr 2021 09:39:50 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210413073950.6qqresqtyqtazvbu@uno.localdomain>","References":"<20210412075702.9955-1-jacopo@jmondi.org>\n\t<20210412075702.9955-2-jacopo@jmondi.org>\n\t\n\t\n\t\n\t\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16219,"web_url":"https://patchwork.libcamera.org/comment/16219/","msgid":"","date":"2021-04-13T07:42:27","subject":"Re: [libcamera-devel] [PATCH v4 1/3] libcamera: Introduce camera\n\tsensor database","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Tue, Apr 13, 2021 at 4:39 PM Jacopo Mondi wrote:\n>\n> Hello,\n>\n> On Tue, Apr 13, 2021 at 04:00:25PM +0900, Hirokazu Honda wrote:\n> > > > > > > > > > +\n> > > > > > > > > > +/**\n> > > > > > > > > > + * \\brief Retrieve the properties associated with a sensor\n> > > > > > > > > > + * \\param sensor The sensor model name\n> > > > > > > > > > + * \\return A pointer to the SensorInfo instance associated with a sensor or\n> > > > > > > > > > + * nullptr if the sensor is not supported in the database\n> > > > > > > > > > + */\n> > > > > > > > > > +const SensorInfo *SensorDatabase::get(const std::string &sensor)\n> > > > > > > > > > +{\n> > > > > > > > > > + for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {\n> > > > > > > > > > + if (sensorDatabase__[i].first == sensor)\n> > > > > > > > > > + return sensorDatabase__[i].second;\n> > > > > > > > > > + }\n> > > > > > >\n> > > > > > > Why not std::map ?\n> > > > > >\n> > > > > > I expect the reason is std::map cannot be constructed as constexpr.\n> > > > >\n> > > > > But the only function that uses it is get(), which isn't constexpr, so\n> > > > > is that really a problem ?\n> > > >\n> > > > Global and static function should be trivially destructible.\n> > > > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables\n> > >\n> > > We have a more relaxed rule, documented in\n> > > Documentation/coding_style.rst. In this case, a map is much more\n> > > convenient, and as its initialization and destruction is independent of\n> > > all other global variables, it doesn't risk causing most of the issues\n> > > mentioned in the above link.\n> > >\n>\n> Indeed I used std::pair to be able to declare it as constexpr.\n>\n> I'll make it a map if that's a not a requirement. I'll see if it's\n> possbile to statically initialize it as I'm currently doing with the\n> pair\n>\n> >\n> > Thanks. I don't remember the description, although I think I read it previously.\n> > That sounds good to me. Perhaps, the map would be put as a const\n> > static variable within CameraSensorInfo::get().\n> >\n>\n> The map needs to be statically initialized, it could be done inside\n> the ::get() function most probably. Something like (not tested)\n>\n> const SensorInfo *SensorDatabase::get(const std::string &sensor)\n> {\n> static const std::map sensorDb = {\n> SENSOR_INFO(imx219),\n> SENSOR_INFO(ov5670),\n> SENSOR_INFO(ov13858),\n> };\n>\n> const auto it = sensorDb.find(sensor.c_str());\n> if (it == sensorDb.end())\n> return nullptr;\n>\n> return it->second;\n> }\n>\n> Thanks\n> j\n\n\nYep, this code looks good to me. (and should work to my best of knowledge)\n-Hiro\n>\n>\n>\n> > > > > > > namespace {\n> > > > > > >\n> > > > > > > const std::map sensorDatabase{\n> > > > > > > { \"imx219\", {\n> > > > > > > .unitCellSize = { 1120, 1120 },\n> > > > > > > } },\n> > > > > > > ...\n> > > > > > > };\n> > > > > > >\n> > > > > > > } /* namespace */\n> > > > > > >\n> > > > > > > and this function can become\n> > > > > > >\n> > > > > > > const CameraSensorInfo *CameraSensorInfo::get(const std::string &sensor)\n> > > > > > > {\n> > > > > > > auto iter = sensorDatabase.find(sensor);\n> > > > > > > if (iter == sensorDatabase.end())\n> > > > > > > return nullptr;\n> > > > > > >\n> > > > > > > return *iter;\n> > > > > > > }\n> > > > > > >\n> > > > > > > > > > +\n> > > > > > > > > > + return nullptr;\n> > > > > > > > > > +}\n> > > > > > > > > > +\n> > > > > > > > > > +} /* namespace libcamera */\n> > > > > > > >\n> > > > > > > > Reviewed-by: Hirokazu Honda \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 F309FBD224\n\tfor ;\n\tTue, 13 Apr 2021 07:42:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7B95F687FE;\n\tTue, 13 Apr 2021 09:42:39 +0200 (CEST)","from mail-ed1-x534.google.com (mail-ed1-x534.google.com\n\t[IPv6:2a00:1450:4864:20::534])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 871EB687F3\n\tfor ;\n\tTue, 13 Apr 2021 09:42:38 +0200 (CEST)","by mail-ed1-x534.google.com with SMTP id bx20so17058827edb.12\n\tfor ;\n\tTue, 13 Apr 2021 00:42:38 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"mKciL3+8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=JR7AZJyf9rjJJYK70MjCgYYoGjKu3zaTjT9KvxEh8mg=;\n\tb=mKciL3+8NpOwz7BtMZYAmdif8UgHmw7g5gMDj5AWxHZIAIei2Z8TKHC6JYfcNKxUUr\n\tTq17IuNx/nRYT6z+K9vAqE5Nw6CdPS+SZXATmcZpkgaMWU7dC2Fisq+fiekGU9CgpCWy\n\tP0/R/rthauRs+J9j9IxkE5cvGqGx2sdm2FYec=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=JR7AZJyf9rjJJYK70MjCgYYoGjKu3zaTjT9KvxEh8mg=;\n\tb=T9BQQXXKXDyxhSTGoNFwVKeX0PQ3RbhSpYBX7oAJtHOtakH9tKULbOg9pzpS4WV198\n\tEE673cY2Zo3Dq3dILRM0dDGmOCd4X/3eCz0F+816EoMCtHLkCgFk7OZJ4i8xi+M/yhKZ\n\t/DeSj3WMUwVdArdHX7GRmbqKthZpKrTnwH8avros/IpCYTco3SQucIPXAkZL0nOAzuR/\n\tURwCAW+9DgW8BCNMe4w3LO3FoSVVuMGQ00woCurBPTfywhYnTN386JaHe7jl1bC8Zxsr\n\tHOhV3835hLPWvEGJPvkgQXJxlTHYfwqTqnLyue6uACFHR9zgr+N0QcHODmRdYEtmraqB\n\tQv6w==","X-Gm-Message-State":"AOAM53091+WjxaR5rlW9GA8TqzioIH7P/53P96jFTpCMlP00JekJk4Kp\n\txvoBzz5GCI+iUVtDSHC2le1PxkREqrfklN6QYI7OS6Ybgx4=","X-Google-Smtp-Source":"ABdhPJzt/lssNuqA42eQwv6AzG8w0zF1ESnvZudNJMmfTIt7VnDn8yapebizKE584aja5qViTHEqaFpbPW6odX0gdrM=","X-Received":"by 2002:a05:6402:34cd:: with SMTP id\n\tw13mr33867320edc.73.1618299758213; \n\tTue, 13 Apr 2021 00:42:38 -0700 (PDT)","MIME-Version":"1.0","References":"<20210412075702.9955-1-jacopo@jmondi.org>\n\t<20210412075702.9955-2-jacopo@jmondi.org>\n\t\n\t\n\t\n\t\n\t\n\t\n\t\n\t\n\t<20210413073950.6qqresqtyqtazvbu@uno.localdomain>","In-Reply-To":"<20210413073950.6qqresqtyqtazvbu@uno.localdomain>","From":"Hirokazu Honda ","Date":"Tue, 13 Apr 2021 16:42:27 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v4 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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16228,"web_url":"https://patchwork.libcamera.org/comment/16228/","msgid":"<20210413093301.5yaaod5xes3pdppy@uno.localdomain>","date":"2021-04-13T09:33:01","subject":"Re: [libcamera-devel] [PATCH v4 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 13, 2021 at 10:56:40AM +0900, Hirokazu Honda wrote:\n> On Tue, Apr 13, 2021 at 10:45 AM Laurent Pinchart\n> wrote:\n> >\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > On Mon, Apr 12, 2021 at 10:07:17PM +0900, Hirokazu Honda wrote:\n> > > On Mon, Apr 12, 2021 at 6:10 PM Niklas Söderlund wrote:\n> > > > On 2021-04-12 09:57:00 +0200, Jacopo Mondi wrote:\n> > > > > Introduce a 'database' of camera sensor information, which contains\n> > > > > a the camera sensor properties which is not possible, or desirable,\n> >\n> > s/is not/are not/\n> >\n> > > > s/^a //\n> > > >\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 information.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > >\n> > > > Reviewed-by: Niklas Söderlund \n> > > >\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 | 97 ++++++++++++++++++++\n> > > > > 4 files changed, 127 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..7d743e46102d\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 SensorInfo {\n> >\n> > I'd name this CameraSensorInfo, as we may have to support other types of\n> > sensor in the future.\n> >\n\nWe already have CameraSensorInfo defined in camera_sensor.h\n\nI agree SensorInfo is not the best name and does not convey the fact\nthose are static properties. What about reusing the 'properties' term\nwith CameraSensorProperties ?\n\n> > > > > + Size unitCellSize;\n> > > > > +};\n> > > > > +\n> > > > > +class SensorDatabase\n> > > > > +{\n> > > > > +public:\n> > > > > + static const SensorInfo *get(const std::string &sensor);\n> > > > > +};\n> > > > > +\n> > >\n> > > Just FYI: https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions\n> > > > Do not create classes only to group static members; this is no different than just giving the names a common prefix, and such grouping is usually unnecessary anyway.\n> > >\n> > > However, I am not against this code. I am personally fine with class +\n> > > static member functions.\n> >\n> > Moving the static function to the SensorInfo class would solve this\n> > problem.\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..68e69e8bf1b6\n> > > > > --- /dev/null\n> > > > > +++ b/src/libcamera/sensor_database.cpp\n> > > > > @@ -0,0 +1,97 @@\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> > > > > +namespace libcamera {\n> >\n> > The namespace goes after \\file.\n> >\n> > > > > +\n> > > > > +/**\n> > > > > + * \\file sensor_database.h\n> > > > > + * \\brief Database of camera sensor properties\n> > > > > + *\n> > > > > + * The camera sensor database collects information on camera sensors\n> >\n> > s/information on/static information about/\n> >\n> > > > > + * which is not possible or desirable to retrieve at run-time.\n> >\n> > s/which/that/\n> > s/run-time/run time/\n> >\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> > > > > + * SensorInfo list of properties.\n> > > > > + *\n> > > > > + * The class provides a single static getInfo() method to access the sensor\n> >\n> > The function is called get(). But in any case, let's move it to the\n> > SensorInfo class.\n> >\n> > > > > + * database entries. If the sensor is not supported in the database nullptr is\n> > > > > + * returned.\n> > > > > + */\n> > > > > +\n> > > > > +/**\n> > > > > + * \\struct SensorInfo\n> > > > > + * \\brief List of sensor properties\n> > > > > + *\n> > > > > + * \\var SensorInfo::unitCellSize\n> > > > > + * \\brief The physical size of pixel, including pixel edges. Width x height in\n> >\n> > s/pixel/a pixel/\n> >\n> > > > > + * nano-meters.\n> >\n> > s/nano-meters/nanometers/\n> >\n> > This field is a Size, so no need to repeat Width x height.\n> >\n> > * \\brief The physical size of a pixel, including pixel edges, in nanometers\n> >\n> > > > > + */\n> > > > > +\n> > > > > +namespace {\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Sony IMX219 sensor properties\n> > > > > + */\n> > > > > +constexpr SensorInfo imx219Info = {\n> > > > > + .unitCellSize = { 1120, 1120 }\n> > > > > +};\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Omnivision ov5670 sensor properties\n> > > > > + */\n> > > > > +constexpr SensorInfo ov5670Info = {\n> > > > > + .unitCellSize = { 1120, 1120 }\n> > > > > +};\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Omnivision 13858 sensor properties\n> > > > > + */\n> > > > > +constexpr SensorInfo ov13858Info = {\n> > > > > + .unitCellSize = { 1120, 1120 }\n> > > > > +};\n> > > > > +\n> > > > > +#define SENSOR_INFO(_sensor) \\\n> > > > > + { #_sensor, &_sensor##Info }\n> > > > > +\n> > > > > +/*\n> > > > > + * \\brief The database of sensor properties\n> > > > > + */\n> > > > > +constexpr std::pair sensorDatabase__[] = {\n> > > > > + SENSOR_INFO(imx219),\n> > > > > + SENSOR_INFO(ov5670),\n> > > > > + SENSOR_INFO(ov13858),\n> > > > > +};\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 SensorInfo instance associated with a sensor or\n> > > > > + * nullptr if the sensor is not supported in the database\n> > > > > + */\n> > > > > +const SensorInfo *SensorDatabase::get(const std::string &sensor)\n> > > > > +{\n> > > > > + for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {\n> > > > > + if (sensorDatabase__[i].first == sensor)\n> > > > > + return sensorDatabase__[i].second;\n> > > > > + }\n> >\n> > Why not std::map ?\n> >\n>\n> I expect the reason is std::map cannot be constructed as constexpr.\n>\n> > namespace {\n> >\n> > const std::map sensorDatabase{\n> > { \"imx219\", {\n> > .unitCellSize = { 1120, 1120 },\n> > } },\n> > ...\n> > };\n> >\n> > } /* namespace */\n> >\n> > and this function can become\n> >\n> > const CameraSensorInfo *CameraSensorInfo::get(const std::string &sensor)\n> > {\n> > auto iter = sensorDatabase.find(sensor);\n> > if (iter == sensorDatabase.end())\n> > return nullptr;\n> >\n> > return *iter;\n> > }\n> >\n> > > > > +\n> > > > > + return nullptr;\n> > > > > +}\n> > > > > +\n> > > > > +} /* namespace libcamera */\n> > >\n> > > Reviewed-by: Hirokazu Honda \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 CAF74BD224\n\tfor ;\n\tTue, 13 Apr 2021 09:32:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2BB92687FE;\n\tTue, 13 Apr 2021 11:32:24 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DA005687F3\n\tfor ;\n\tTue, 13 Apr 2021 11:32:22 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 1536960004;\n\tTue, 13 Apr 2021 09:32:21 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 13 Apr 2021 11:33:01 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210413093301.5yaaod5xes3pdppy@uno.localdomain>","References":"<20210412075702.9955-1-jacopo@jmondi.org>\n\t<20210412075702.9955-2-jacopo@jmondi.org>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 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 ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16242,"web_url":"https://patchwork.libcamera.org/comment/16242/","msgid":"","date":"2021-04-13T20:46:14","subject":"Re: [libcamera-devel] [PATCH v4 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\n\nOn Tue, Apr 13, 2021 at 11:33:01AM +0200, Jacopo Mondi wrote:\n> On Tue, Apr 13, 2021 at 10:56:40AM +0900, Hirokazu Honda wrote:\n> > On Tue, Apr 13, 2021 at 10:45 AM Laurent Pinchart wrote:\n> > > On Mon, Apr 12, 2021 at 10:07:17PM +0900, Hirokazu Honda wrote:\n> > > > On Mon, Apr 12, 2021 at 6:10 PM Niklas Söderlund wrote:\n> > > > > On 2021-04-12 09:57:00 +0200, Jacopo Mondi wrote:\n> > > > > > Introduce a 'database' of camera sensor information, which contains\n> > > > > > a the camera sensor properties which is not possible, or desirable,\n> > >\n> > > s/is not/are not/\n> > >\n> > > > > s/^a //\n> > > > >\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 information.\n> > > > > >\n> > > > > > Signed-off-by: Jacopo Mondi \n> > > > >\n> > > > > Reviewed-by: Niklas Söderlund \n> > > > >\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 | 97 ++++++++++++++++++++\n> > > > > > 4 files changed, 127 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..7d743e46102d\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 SensorInfo {\n> > >\n> > > I'd name this CameraSensorInfo, as we may have to support other types of\n> > > sensor in the future.\n> > >\n> \n> We already have CameraSensorInfo defined in camera_sensor.h\n> \n> I agree SensorInfo is not the best name and does not convey the fact\n> those are static properties. What about reusing the 'properties' term\n> with CameraSensorProperties ?\n\nOr we could vary languages, with KameraAnturiTieto :-)\nCameraSensorProperties sounds good to me.\n\n> > > > > > + Size unitCellSize;\n> > > > > > +};\n> > > > > > +\n> > > > > > +class SensorDatabase\n> > > > > > +{\n> > > > > > +public:\n> > > > > > + static const SensorInfo *get(const std::string &sensor);\n> > > > > > +};\n> > > > > > +\n> > > >\n> > > > Just FYI: https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions\n> > > > > Do not create classes only to group static members; this is no different than just giving the names a common prefix, and such grouping is usually unnecessary anyway.\n> > > >\n> > > > However, I am not against this code. I am personally fine with class +\n> > > > static member functions.\n> > >\n> > > Moving the static function to the SensorInfo class would solve this\n> > > problem.\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..68e69e8bf1b6\n> > > > > > --- /dev/null\n> > > > > > +++ b/src/libcamera/sensor_database.cpp\n> > > > > > @@ -0,0 +1,97 @@\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> > > > > > +namespace libcamera {\n> > >\n> > > The namespace goes after \\file.\n> > >\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\file sensor_database.h\n> > > > > > + * \\brief Database of camera sensor properties\n> > > > > > + *\n> > > > > > + * The camera sensor database collects information on camera sensors\n> > >\n> > > s/information on/static information about/\n> > >\n> > > > > > + * which is not possible or desirable to retrieve at run-time.\n> > >\n> > > s/which/that/\n> > > s/run-time/run time/\n> > >\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> > > > > > + * SensorInfo list of properties.\n> > > > > > + *\n> > > > > > + * The class provides a single static getInfo() method to access the sensor\n> > >\n> > > The function is called get(). But in any case, let's move it to the\n> > > SensorInfo class.\n> > >\n> > > > > > + * database entries. If the sensor is not supported in the database nullptr is\n> > > > > > + * returned.\n> > > > > > + */\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\struct SensorInfo\n> > > > > > + * \\brief List of sensor properties\n> > > > > > + *\n> > > > > > + * \\var SensorInfo::unitCellSize\n> > > > > > + * \\brief The physical size of pixel, including pixel edges. Width x height in\n> > >\n> > > s/pixel/a pixel/\n> > >\n> > > > > > + * nano-meters.\n> > >\n> > > s/nano-meters/nanometers/\n> > >\n> > > This field is a Size, so no need to repeat Width x height.\n> > >\n> > > * \\brief The physical size of a pixel, including pixel edges, in nanometers\n> > >\n> > > > > > + */\n> > > > > > +\n> > > > > > +namespace {\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\brief Sony IMX219 sensor properties\n> > > > > > + */\n> > > > > > +constexpr SensorInfo imx219Info = {\n> > > > > > + .unitCellSize = { 1120, 1120 }\n> > > > > > +};\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\brief Omnivision ov5670 sensor properties\n> > > > > > + */\n> > > > > > +constexpr SensorInfo ov5670Info = {\n> > > > > > + .unitCellSize = { 1120, 1120 }\n> > > > > > +};\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\brief Omnivision 13858 sensor properties\n> > > > > > + */\n> > > > > > +constexpr SensorInfo ov13858Info = {\n> > > > > > + .unitCellSize = { 1120, 1120 }\n> > > > > > +};\n> > > > > > +\n> > > > > > +#define SENSOR_INFO(_sensor) \\\n> > > > > > + { #_sensor, &_sensor##Info }\n> > > > > > +\n> > > > > > +/*\n> > > > > > + * \\brief The database of sensor properties\n> > > > > > + */\n> > > > > > +constexpr std::pair sensorDatabase__[] = {\n> > > > > > + SENSOR_INFO(imx219),\n> > > > > > + SENSOR_INFO(ov5670),\n> > > > > > + SENSOR_INFO(ov13858),\n> > > > > > +};\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 SensorInfo instance associated with a sensor or\n> > > > > > + * nullptr if the sensor is not supported in the database\n> > > > > > + */\n> > > > > > +const SensorInfo *SensorDatabase::get(const std::string &sensor)\n> > > > > > +{\n> > > > > > + for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {\n> > > > > > + if (sensorDatabase__[i].first == sensor)\n> > > > > > + return sensorDatabase__[i].second;\n> > > > > > + }\n> > >\n> > > Why not std::map ?\n> >\n> > I expect the reason is std::map cannot be constructed as constexpr.\n> >\n> > > namespace {\n> > >\n> > > const std::map sensorDatabase{\n> > > { \"imx219\", {\n> > > .unitCellSize = { 1120, 1120 },\n> > > } },\n> > > ...\n> > > };\n> > >\n> > > } /* namespace */\n> > >\n> > > and this function can become\n> > >\n> > > const CameraSensorInfo *CameraSensorInfo::get(const std::string &sensor)\n> > > {\n> > > auto iter = sensorDatabase.find(sensor);\n> > > if (iter == sensorDatabase.end())\n> > > return nullptr;\n> > >\n> > > return *iter;\n> > > }\n> > >\n> > > > > > +\n> > > > > > + return nullptr;\n> > > > > > +}\n> > > > > > +\n> > > > > > +} /* namespace libcamera */\n> > > >\n> > > > Reviewed-by: Hirokazu Honda ","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 5DA9CBD1F6\n\tfor ;\n\tTue, 13 Apr 2021 20:47:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D098A687FE;\n\tTue, 13 Apr 2021 22:47:07 +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 44B3C602D1\n\tfor ;\n\tTue, 13 Apr 2021 22:47:06 +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 9FB439F0;\n\tTue, 13 Apr 2021 22:47:05 +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=\"iD4y/6Ta\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618346825;\n\tbh=t6FQ+XhhN7TIm2+zt0hRlHlPskYUHZgrogYhj3LF/vM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iD4y/6TafYb80Ly4JhD+1k1iqf6zA6kMYjEnkNfYD9Occv++EQzf8utQhpPMBhQg2\n\tlHXfWJGdflC1xs90QdEye4mFIwsa5vRQJzlUBCbnvAtskCS1W1UGDFmoMx8/vsFYa5\n\tVjgPyTcsYdSMXfSHJKzGcyPqReTqoDLU6iGE1Kg0=","Date":"Tue, 13 Apr 2021 23:46:14 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210412075702.9955-1-jacopo@jmondi.org>\n\t<20210412075702.9955-2-jacopo@jmondi.org>\n\t\n\t\n\t\n\t\n\t<20210413093301.5yaaod5xes3pdppy@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210413093301.5yaaod5xes3pdppy@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v4 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 ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]