[libcamera-devel,v4,0/3] libcamera: Introduce sensor database

11888 mbox series

Addressed Niklas' review comments and took in his tags.

- in-line getInfo() in SensorDatabase::get()
- align style in 3/3 using std::array

Thanks
   j

Jacopo Mondi (3):
  libcamera: Introduce camera sensor database
  libcamera: camera_sensor: Register static properties
  android: camera_device: Report sensor physical size

 include/libcamera/internal/camera_sensor.h   |  1 +
 include/libcamera/internal/meson.build       |  1 +
 include/libcamera/internal/sensor_database.h | 28 ++++++
 src/android/camera_device.cpp                | 30 +++---
 src/libcamera/camera_sensor.cpp              | 24 ++++-
 src/libcamera/meson.build                    |  1 +
 src/libcamera/sensor_database.cpp            | 97 ++++++++++++++++++++
 7 files changed, 167 insertions(+), 15 deletions(-)
 create mode 100644 include/libcamera/internal/sensor_database.h
 create mode 100644 src/libcamera/sensor_database.cpp

--
2.31.1

Hi Jacopo,

Thanks for your work.

On 2021-04-12 09:57:00 +0200, Jacopo Mondi wrote:
> Introduce a 'database' of camera sensor information, which contains
> a the camera sensor properties which is not possible, or desirable,

s/^a //

> to retrieve at run-time.
> 
> The camera sensor database is accessed through the static SensorDatabase
> class which provides a single method to obtain the sensor information.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  include/libcamera/internal/meson.build       |  1 +
>  include/libcamera/internal/sensor_database.h | 28 ++++++
>  src/libcamera/meson.build                    |  1 +
>  src/libcamera/sensor_database.cpp            | 97 ++++++++++++++++++++
>  4 files changed, 127 insertions(+)
>  create mode 100644 include/libcamera/internal/sensor_database.h
>  create mode 100644 src/libcamera/sensor_database.cpp
> 
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 1fe3918cfe93..4aaa76ac1db1 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -38,6 +38,7 @@ libcamera_internal_headers = files([
>      'process.h',
>      'pub_key.h',
>      'semaphore.h',
> +    'sensor_database.h',
>      'sysfs.h',
>      'thread.h',
>      'timer.h',
> diff --git a/include/libcamera/internal/sensor_database.h b/include/libcamera/internal/sensor_database.h
> new file mode 100644
> index 000000000000..7d743e46102d
> --- /dev/null
> +++ b/include/libcamera/internal/sensor_database.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * sensor_database.h - Database of camera sensor properties
> + */
> +#ifndef __LIBCAMERA_SENSOR_DATABASE_H__
> +#define __LIBCAMERA_SENSOR_DATABASE_H__
> +
> +#include <string>
> +
> +#include <libcamera/geometry.h>
> +
> +namespace libcamera {
> +
> +struct SensorInfo {
> +	Size unitCellSize;
> +};
> +
> +class SensorDatabase
> +{
> +public:
> +	static const SensorInfo *get(const std::string &sensor);
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_SENSOR_DATABASE_H__ */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index e0a48aa23ea5..effae0ef5186 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -44,6 +44,7 @@ libcamera_sources = files([
>      'pub_key.cpp',
>      'request.cpp',
>      'semaphore.cpp',
> +    'sensor_database.cpp',
>      'signal.cpp',
>      'stream.cpp',
>      'sysfs.cpp',
> diff --git a/src/libcamera/sensor_database.cpp b/src/libcamera/sensor_database.cpp
> new file mode 100644
> index 000000000000..68e69e8bf1b6
> --- /dev/null
> +++ b/src/libcamera/sensor_database.cpp
> @@ -0,0 +1,97 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * sensor_database.cpp - Database of camera sensor properties
> + */
> +
> +#include "libcamera/internal/sensor_database.h"
> +
> +#include <algorithm>
> +
> +namespace libcamera {
> +
> +/**
> + * \file sensor_database.h
> + * \brief Database of camera sensor properties
> + *
> + * The camera sensor database collects information on camera sensors
> + * which is not possible or desirable to retrieve at run-time.
> + */
> +
> +/**
> + * \class SensorDatabase
> + * \brief Access the database of camera sensor properties
> + *
> + * The database is indexed using the camera sensor model, as reported by the
> + * properties::Model property, and for each supported sensor it contains a
> + * SensorInfo list of properties.
> + *
> + * The class provides a single static getInfo() method to access the sensor
> + * database entries. If the sensor is not supported in the database nullptr is
> + * returned.
> + */
> +
> +/**
> + * \struct SensorInfo
> + * \brief List of sensor properties
> + *
> + * \var SensorInfo::unitCellSize
> + * \brief The physical size of pixel, including pixel edges. Width x height in
> + * nano-meters.
> + */
> +
> +namespace {
> +
> +/**
> + * \brief Sony IMX219 sensor properties
> + */
> +constexpr SensorInfo imx219Info = {
> +	.unitCellSize = { 1120, 1120 }
> +};
> +
> +/**
> + * \brief Omnivision ov5670 sensor properties
> + */
> +constexpr SensorInfo ov5670Info = {
> +	.unitCellSize = { 1120, 1120 }
> +};
> +
> +/**
> + * \brief Omnivision 13858 sensor properties
> + */
> +constexpr SensorInfo ov13858Info = {
> +	.unitCellSize = { 1120, 1120 }
> +};
> +
> +#define SENSOR_INFO(_sensor) \
> +	{ #_sensor, &_sensor##Info }
> +
> +/*
> + * \brief The database of sensor properties
> + */
> +constexpr std::pair<const char *const, const SensorInfo *> sensorDatabase__[] = {
> +	SENSOR_INFO(imx219),
> +	SENSOR_INFO(ov5670),
> +	SENSOR_INFO(ov13858),
> +};
> +
> +} /* namespace */
> +
> +/**
> + * \brief Retrieve the properties associated with a sensor
> + * \param sensor The sensor model name
> + * \return A pointer to the SensorInfo instance associated with a sensor or
> + * nullptr if the sensor is not supported in the database
> + */
> +const SensorInfo *SensorDatabase::get(const std::string &sensor)
> +{
> +	for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {
> +		if (sensorDatabase__[i].first == sensor)
> +			return sensorDatabase__[i].second;
> +	}
> +
> +	return nullptr;
> +}
> +
> +} /* namespace libcamera */
> -- 
> 2.31.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo, thanks for the patch.


On Mon, Apr 12, 2021 at 6:10 PM Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
>
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2021-04-12 09:57:00 +0200, Jacopo Mondi wrote:
> > Introduce a 'database' of camera sensor information, which contains
> > a the camera sensor properties which is not possible, or desirable,
>
> s/^a //
>
> > to retrieve at run-time.
> >
> > The camera sensor database is accessed through the static SensorDatabase
> > class which provides a single method to obtain the sensor information.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>
> > ---
> >  include/libcamera/internal/meson.build       |  1 +
> >  include/libcamera/internal/sensor_database.h | 28 ++++++
> >  src/libcamera/meson.build                    |  1 +
> >  src/libcamera/sensor_database.cpp            | 97 ++++++++++++++++++++
> >  4 files changed, 127 insertions(+)
> >  create mode 100644 include/libcamera/internal/sensor_database.h
> >  create mode 100644 src/libcamera/sensor_database.cpp
> >
> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > index 1fe3918cfe93..4aaa76ac1db1 100644
> > --- a/include/libcamera/internal/meson.build
> > +++ b/include/libcamera/internal/meson.build
> > @@ -38,6 +38,7 @@ libcamera_internal_headers = files([
> >      'process.h',
> >      'pub_key.h',
> >      'semaphore.h',
> > +    'sensor_database.h',
> >      'sysfs.h',
> >      'thread.h',
> >      'timer.h',
> > diff --git a/include/libcamera/internal/sensor_database.h b/include/libcamera/internal/sensor_database.h
> > new file mode 100644
> > index 000000000000..7d743e46102d
> > --- /dev/null
> > +++ b/include/libcamera/internal/sensor_database.h
> > @@ -0,0 +1,28 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * sensor_database.h - Database of camera sensor properties
> > + */
> > +#ifndef __LIBCAMERA_SENSOR_DATABASE_H__
> > +#define __LIBCAMERA_SENSOR_DATABASE_H__
> > +
> > +#include <string>
> > +
> > +#include <libcamera/geometry.h>
> > +
> > +namespace libcamera {
> > +
> > +struct SensorInfo {
> > +     Size unitCellSize;
> > +};
> > +
> > +class SensorDatabase
> > +{
> > +public:
> > +     static const SensorInfo *get(const std::string &sensor);
> > +};
> > +


Just FYI: https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions
> 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.

However, I am not against this code. I am personally fine with class +
static member functions.

> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_SENSOR_DATABASE_H__ */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index e0a48aa23ea5..effae0ef5186 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -44,6 +44,7 @@ libcamera_sources = files([
> >      'pub_key.cpp',
> >      'request.cpp',
> >      'semaphore.cpp',
> > +    'sensor_database.cpp',
> >      'signal.cpp',
> >      'stream.cpp',
> >      'sysfs.cpp',
> > diff --git a/src/libcamera/sensor_database.cpp b/src/libcamera/sensor_database.cpp
> > new file mode 100644
> > index 000000000000..68e69e8bf1b6
> > --- /dev/null
> > +++ b/src/libcamera/sensor_database.cpp
> > @@ -0,0 +1,97 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * sensor_database.cpp - Database of camera sensor properties
> > + */
> > +
> > +#include "libcamera/internal/sensor_database.h"
> > +
> > +#include <algorithm>
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \file sensor_database.h
> > + * \brief Database of camera sensor properties
> > + *
> > + * The camera sensor database collects information on camera sensors
> > + * which is not possible or desirable to retrieve at run-time.
> > + */
> > +
> > +/**
> > + * \class SensorDatabase
> > + * \brief Access the database of camera sensor properties
> > + *
> > + * The database is indexed using the camera sensor model, as reported by the
> > + * properties::Model property, and for each supported sensor it contains a
> > + * SensorInfo list of properties.
> > + *
> > + * The class provides a single static getInfo() method to access the sensor
> > + * database entries. If the sensor is not supported in the database nullptr is
> > + * returned.
> > + */
> > +
> > +/**
> > + * \struct SensorInfo
> > + * \brief List of sensor properties
> > + *
> > + * \var SensorInfo::unitCellSize
> > + * \brief The physical size of pixel, including pixel edges. Width x height in
> > + * nano-meters.
> > + */
> > +
> > +namespace {
> > +
> > +/**
> > + * \brief Sony IMX219 sensor properties
> > + */
> > +constexpr SensorInfo imx219Info = {
> > +     .unitCellSize = { 1120, 1120 }
> > +};
> > +
> > +/**
> > + * \brief Omnivision ov5670 sensor properties
> > + */
> > +constexpr SensorInfo ov5670Info = {
> > +     .unitCellSize = { 1120, 1120 }
> > +};
> > +
> > +/**
> > + * \brief Omnivision 13858 sensor properties
> > + */
> > +constexpr SensorInfo ov13858Info = {
> > +     .unitCellSize = { 1120, 1120 }
> > +};
> > +
> > +#define SENSOR_INFO(_sensor) \
> > +     { #_sensor, &_sensor##Info }
> > +
> > +/*
> > + * \brief The database of sensor properties
> > + */
> > +constexpr std::pair<const char *const, const SensorInfo *> sensorDatabase__[] = {
> > +     SENSOR_INFO(imx219),
> > +     SENSOR_INFO(ov5670),
> > +     SENSOR_INFO(ov13858),
> > +};
> > +
> > +} /* namespace */
> > +
> > +/**
> > + * \brief Retrieve the properties associated with a sensor
> > + * \param sensor The sensor model name
> > + * \return A pointer to the SensorInfo instance associated with a sensor or
> > + * nullptr if the sensor is not supported in the database
> > + */
> > +const SensorInfo *SensorDatabase::get(const std::string &sensor)
> > +{
> > +     for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {
> > +             if (sensorDatabase__[i].first == sensor)
> > +                     return sensorDatabase__[i].second;
> > +     }
> > +
> > +     return nullptr;
> > +}
> > +
> > +} /* namespace libcamera */
> > --
> > 2.31.1
> >

Reviewed-by: Hirokazu Honda <hiroh@chromium.org>

-Hiro
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

Thank you for the patch.

On Mon, Apr 12, 2021 at 10:07:17PM +0900, Hirokazu Honda wrote:
> On Mon, Apr 12, 2021 at 6:10 PM Niklas Söderlund wrote:
> > On 2021-04-12 09:57:00 +0200, Jacopo Mondi wrote:
> > > Introduce a 'database' of camera sensor information, which contains
> > > a the camera sensor properties which is not possible, or desirable,

s/is not/are not/

> > s/^a //
> >
> > > to retrieve at run-time.
> > >
> > > The camera sensor database is accessed through the static SensorDatabase
> > > class which provides a single method to obtain the sensor information.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >
> > > ---
> > >  include/libcamera/internal/meson.build       |  1 +
> > >  include/libcamera/internal/sensor_database.h | 28 ++++++
> > >  src/libcamera/meson.build                    |  1 +
> > >  src/libcamera/sensor_database.cpp            | 97 ++++++++++++++++++++
> > >  4 files changed, 127 insertions(+)
> > >  create mode 100644 include/libcamera/internal/sensor_database.h
> > >  create mode 100644 src/libcamera/sensor_database.cpp
> > >
> > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > > index 1fe3918cfe93..4aaa76ac1db1 100644
> > > --- a/include/libcamera/internal/meson.build
> > > +++ b/include/libcamera/internal/meson.build
> > > @@ -38,6 +38,7 @@ libcamera_internal_headers = files([
> > >      'process.h',
> > >      'pub_key.h',
> > >      'semaphore.h',
> > > +    'sensor_database.h',
> > >      'sysfs.h',
> > >      'thread.h',
> > >      'timer.h',
> > > diff --git a/include/libcamera/internal/sensor_database.h b/include/libcamera/internal/sensor_database.h
> > > new file mode 100644
> > > index 000000000000..7d743e46102d
> > > --- /dev/null
> > > +++ b/include/libcamera/internal/sensor_database.h
> > > @@ -0,0 +1,28 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Google Inc.
> > > + *
> > > + * sensor_database.h - Database of camera sensor properties
> > > + */
> > > +#ifndef __LIBCAMERA_SENSOR_DATABASE_H__
> > > +#define __LIBCAMERA_SENSOR_DATABASE_H__
> > > +
> > > +#include <string>
> > > +
> > > +#include <libcamera/geometry.h>
> > > +
> > > +namespace libcamera {
> > > +
> > > +struct SensorInfo {

I'd name this CameraSensorInfo, as we may have to support other types of
sensor in the future.

> > > +     Size unitCellSize;
> > > +};
> > > +
> > > +class SensorDatabase
> > > +{
> > > +public:
> > > +     static const SensorInfo *get(const std::string &sensor);
> > > +};
> > > +
> 
> Just FYI: https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions
> > 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.
> 
> However, I am not against this code. I am personally fine with class +
> static member functions.

Moving the static function to the SensorInfo class would solve this
problem.

> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_SENSOR_DATABASE_H__ */
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index e0a48aa23ea5..effae0ef5186 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -44,6 +44,7 @@ libcamera_sources = files([
> > >      'pub_key.cpp',
> > >      'request.cpp',
> > >      'semaphore.cpp',
> > > +    'sensor_database.cpp',
> > >      'signal.cpp',
> > >      'stream.cpp',
> > >      'sysfs.cpp',
> > > diff --git a/src/libcamera/sensor_database.cpp b/src/libcamera/sensor_database.cpp
> > > new file mode 100644
> > > index 000000000000..68e69e8bf1b6
> > > --- /dev/null
> > > +++ b/src/libcamera/sensor_database.cpp
> > > @@ -0,0 +1,97 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Google Inc.
> > > + *
> > > + * sensor_database.cpp - Database of camera sensor properties
> > > + */
> > > +
> > > +#include "libcamera/internal/sensor_database.h"
> > > +
> > > +#include <algorithm>
> > > +
> > > +namespace libcamera {

The namespace goes after \file.

> > > +
> > > +/**
> > > + * \file sensor_database.h
> > > + * \brief Database of camera sensor properties
> > > + *
> > > + * The camera sensor database collects information on camera sensors

s/information on/static information about/

> > > + * which is not possible or desirable to retrieve at run-time.

s/which/that/
s/run-time/run time/

> > > + */
> > > +
> > > +/**
> > > + * \class SensorDatabase
> > > + * \brief Access the database of camera sensor properties
> > > + *
> > > + * The database is indexed using the camera sensor model, as reported by the
> > > + * properties::Model property, and for each supported sensor it contains a
> > > + * SensorInfo list of properties.
> > > + *
> > > + * The class provides a single static getInfo() method to access the sensor

The function is called get(). But in any case, let's move it to the
SensorInfo class.

> > > + * database entries. If the sensor is not supported in the database nullptr is
> > > + * returned.
> > > + */
> > > +
> > > +/**
> > > + * \struct SensorInfo
> > > + * \brief List of sensor properties
> > > + *
> > > + * \var SensorInfo::unitCellSize
> > > + * \brief The physical size of pixel, including pixel edges. Width x height in

s/pixel/a pixel/

> > > + * nano-meters.

s/nano-meters/nanometers/

This field is a Size, so no need to repeat Width x height.

 * \brief The physical size of a pixel, including pixel edges, in nanometers

> > > + */
> > > +
> > > +namespace {
> > > +
> > > +/**
> > > + * \brief Sony IMX219 sensor properties
> > > + */
> > > +constexpr SensorInfo imx219Info = {
> > > +     .unitCellSize = { 1120, 1120 }
> > > +};
> > > +
> > > +/**
> > > + * \brief Omnivision ov5670 sensor properties
> > > + */
> > > +constexpr SensorInfo ov5670Info = {
> > > +     .unitCellSize = { 1120, 1120 }
> > > +};
> > > +
> > > +/**
> > > + * \brief Omnivision 13858 sensor properties
> > > + */
> > > +constexpr SensorInfo ov13858Info = {
> > > +     .unitCellSize = { 1120, 1120 }
> > > +};
> > > +
> > > +#define SENSOR_INFO(_sensor) \
> > > +     { #_sensor, &_sensor##Info }
> > > +
> > > +/*
> > > + * \brief The database of sensor properties
> > > + */
> > > +constexpr std::pair<const char *const, const SensorInfo *> sensorDatabase__[] = {
> > > +     SENSOR_INFO(imx219),
> > > +     SENSOR_INFO(ov5670),
> > > +     SENSOR_INFO(ov13858),
> > > +};
> > > +
> > > +} /* namespace */
> > > +
> > > +/**
> > > + * \brief Retrieve the properties associated with a sensor
> > > + * \param sensor The sensor model name
> > > + * \return A pointer to the SensorInfo instance associated with a sensor or
> > > + * nullptr if the sensor is not supported in the database
> > > + */
> > > +const SensorInfo *SensorDatabase::get(const std::string &sensor)
> > > +{
> > > +     for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {
> > > +             if (sensorDatabase__[i].first == sensor)
> > > +                     return sensorDatabase__[i].second;
> > > +     }

Why not std::map ?

namespace {

const std::map<std::string, SensorInfo> sensorDatabase{
	{ "imx219", {
		.unitCellSize = { 1120, 1120 },
	} },
	...
};

} /* namespace */

and this function can become

const CameraSensorInfo *CameraSensorInfo::get(const std::string &sensor)
{
	auto iter = sensorDatabase.find(sensor);
	if (iter == sensorDatabase.end())
		return nullptr;

	return *iter;
}

> > > +
> > > +     return nullptr;
> > > +}
> > > +
> > > +} /* namespace libcamera */
> 
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>

On Tue, Apr 13, 2021 at 10:45 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Apr 12, 2021 at 10:07:17PM +0900, Hirokazu Honda wrote:
> > On Mon, Apr 12, 2021 at 6:10 PM Niklas Söderlund wrote:
> > > On 2021-04-12 09:57:00 +0200, Jacopo Mondi wrote:
> > > > Introduce a 'database' of camera sensor information, which contains
> > > > a the camera sensor properties which is not possible, or desirable,
>
> s/is not/are not/
>
> > > s/^a //
> > >
> > > > to retrieve at run-time.
> > > >
> > > > The camera sensor database is accessed through the static SensorDatabase
> > > > class which provides a single method to obtain the sensor information.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > >
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > >
> > > > ---
> > > >  include/libcamera/internal/meson.build       |  1 +
> > > >  include/libcamera/internal/sensor_database.h | 28 ++++++
> > > >  src/libcamera/meson.build                    |  1 +
> > > >  src/libcamera/sensor_database.cpp            | 97 ++++++++++++++++++++
> > > >  4 files changed, 127 insertions(+)
> > > >  create mode 100644 include/libcamera/internal/sensor_database.h
> > > >  create mode 100644 src/libcamera/sensor_database.cpp
> > > >
> > > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > > > index 1fe3918cfe93..4aaa76ac1db1 100644
> > > > --- a/include/libcamera/internal/meson.build
> > > > +++ b/include/libcamera/internal/meson.build
> > > > @@ -38,6 +38,7 @@ libcamera_internal_headers = files([
> > > >      'process.h',
> > > >      'pub_key.h',
> > > >      'semaphore.h',
> > > > +    'sensor_database.h',
> > > >      'sysfs.h',
> > > >      'thread.h',
> > > >      'timer.h',
> > > > diff --git a/include/libcamera/internal/sensor_database.h b/include/libcamera/internal/sensor_database.h
> > > > new file mode 100644
> > > > index 000000000000..7d743e46102d
> > > > --- /dev/null
> > > > +++ b/include/libcamera/internal/sensor_database.h
> > > > @@ -0,0 +1,28 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2021, Google Inc.
> > > > + *
> > > > + * sensor_database.h - Database of camera sensor properties
> > > > + */
> > > > +#ifndef __LIBCAMERA_SENSOR_DATABASE_H__
> > > > +#define __LIBCAMERA_SENSOR_DATABASE_H__
> > > > +
> > > > +#include <string>
> > > > +
> > > > +#include <libcamera/geometry.h>
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +struct SensorInfo {
>
> I'd name this CameraSensorInfo, as we may have to support other types of
> sensor in the future.
>
> > > > +     Size unitCellSize;
> > > > +};
> > > > +
> > > > +class SensorDatabase
> > > > +{
> > > > +public:
> > > > +     static const SensorInfo *get(const std::string &sensor);
> > > > +};
> > > > +
> >
> > Just FYI: https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions
> > > 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.
> >
> > However, I am not against this code. I am personally fine with class +
> > static member functions.
>
> Moving the static function to the SensorInfo class would solve this
> problem.
>
> > > > +} /* namespace libcamera */
> > > > +
> > > > +#endif /* __LIBCAMERA_SENSOR_DATABASE_H__ */
> > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > > index e0a48aa23ea5..effae0ef5186 100644
> > > > --- a/src/libcamera/meson.build
> > > > +++ b/src/libcamera/meson.build
> > > > @@ -44,6 +44,7 @@ libcamera_sources = files([
> > > >      'pub_key.cpp',
> > > >      'request.cpp',
> > > >      'semaphore.cpp',
> > > > +    'sensor_database.cpp',
> > > >      'signal.cpp',
> > > >      'stream.cpp',
> > > >      'sysfs.cpp',
> > > > diff --git a/src/libcamera/sensor_database.cpp b/src/libcamera/sensor_database.cpp
> > > > new file mode 100644
> > > > index 000000000000..68e69e8bf1b6
> > > > --- /dev/null
> > > > +++ b/src/libcamera/sensor_database.cpp
> > > > @@ -0,0 +1,97 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2021, Google Inc.
> > > > + *
> > > > + * sensor_database.cpp - Database of camera sensor properties
> > > > + */
> > > > +
> > > > +#include "libcamera/internal/sensor_database.h"
> > > > +
> > > > +#include <algorithm>
> > > > +
> > > > +namespace libcamera {
>
> The namespace goes after \file.
>
> > > > +
> > > > +/**
> > > > + * \file sensor_database.h
> > > > + * \brief Database of camera sensor properties
> > > > + *
> > > > + * The camera sensor database collects information on camera sensors
>
> s/information on/static information about/
>
> > > > + * which is not possible or desirable to retrieve at run-time.
>
> s/which/that/
> s/run-time/run time/
>
> > > > + */
> > > > +
> > > > +/**
> > > > + * \class SensorDatabase
> > > > + * \brief Access the database of camera sensor properties
> > > > + *
> > > > + * The database is indexed using the camera sensor model, as reported by the
> > > > + * properties::Model property, and for each supported sensor it contains a
> > > > + * SensorInfo list of properties.
> > > > + *
> > > > + * The class provides a single static getInfo() method to access the sensor
>
> The function is called get(). But in any case, let's move it to the
> SensorInfo class.
>
> > > > + * database entries. If the sensor is not supported in the database nullptr is
> > > > + * returned.
> > > > + */
> > > > +
> > > > +/**
> > > > + * \struct SensorInfo
> > > > + * \brief List of sensor properties
> > > > + *
> > > > + * \var SensorInfo::unitCellSize
> > > > + * \brief The physical size of pixel, including pixel edges. Width x height in
>
> s/pixel/a pixel/
>
> > > > + * nano-meters.
>
> s/nano-meters/nanometers/
>
> This field is a Size, so no need to repeat Width x height.
>
>  * \brief The physical size of a pixel, including pixel edges, in nanometers
>
> > > > + */
> > > > +
> > > > +namespace {
> > > > +
> > > > +/**
> > > > + * \brief Sony IMX219 sensor properties
> > > > + */
> > > > +constexpr SensorInfo imx219Info = {
> > > > +     .unitCellSize = { 1120, 1120 }
> > > > +};
> > > > +
> > > > +/**
> > > > + * \brief Omnivision ov5670 sensor properties
> > > > + */
> > > > +constexpr SensorInfo ov5670Info = {
> > > > +     .unitCellSize = { 1120, 1120 }
> > > > +};
> > > > +
> > > > +/**
> > > > + * \brief Omnivision 13858 sensor properties
> > > > + */
> > > > +constexpr SensorInfo ov13858Info = {
> > > > +     .unitCellSize = { 1120, 1120 }
> > > > +};
> > > > +
> > > > +#define SENSOR_INFO(_sensor) \
> > > > +     { #_sensor, &_sensor##Info }
> > > > +
> > > > +/*
> > > > + * \brief The database of sensor properties
> > > > + */
> > > > +constexpr std::pair<const char *const, const SensorInfo *> sensorDatabase__[] = {
> > > > +     SENSOR_INFO(imx219),
> > > > +     SENSOR_INFO(ov5670),
> > > > +     SENSOR_INFO(ov13858),
> > > > +};
> > > > +
> > > > +} /* namespace */
> > > > +
> > > > +/**
> > > > + * \brief Retrieve the properties associated with a sensor
> > > > + * \param sensor The sensor model name
> > > > + * \return A pointer to the SensorInfo instance associated with a sensor or
> > > > + * nullptr if the sensor is not supported in the database
> > > > + */
> > > > +const SensorInfo *SensorDatabase::get(const std::string &sensor)
> > > > +{
> > > > +     for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {
> > > > +             if (sensorDatabase__[i].first == sensor)
> > > > +                     return sensorDatabase__[i].second;
> > > > +     }
>
> Why not std::map ?
>

I expect the reason is std::map cannot be constructed as constexpr.

> namespace {
>
> const std::map<std::string, SensorInfo> sensorDatabase{
>         { "imx219", {
>                 .unitCellSize = { 1120, 1120 },
>         } },
>         ...
> };
>
> } /* namespace */
>
> and this function can become
>
> const CameraSensorInfo *CameraSensorInfo::get(const std::string &sensor)
> {
>         auto iter = sensorDatabase.find(sensor);
>         if (iter == sensorDatabase.end())
>                 return nullptr;
>
>         return *iter;
> }
>
> > > > +
> > > > +     return nullptr;
> > > > +}
> > > > +
> > > > +} /* namespace libcamera */
> >
> > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
>
> --
> Regards,
>
> Laurent Pinchart

On Tue, Apr 13, 2021 at 10:56:40AM +0900, Hirokazu Honda wrote:
> On Tue, Apr 13, 2021 at 10:45 AM Laurent Pinchart wrote:
> > On Mon, Apr 12, 2021 at 10:07:17PM +0900, Hirokazu Honda wrote:
> > > On Mon, Apr 12, 2021 at 6:10 PM Niklas Söderlund wrote:
> > > > On 2021-04-12 09:57:00 +0200, Jacopo Mondi wrote:
> > > > > Introduce a 'database' of camera sensor information, which contains
> > > > > a the camera sensor properties which is not possible, or desirable,
> >
> > s/is not/are not/
> >
> > > > s/^a //
> > > >
> > > > > to retrieve at run-time.
> > > > >
> > > > > The camera sensor database is accessed through the static SensorDatabase
> > > > > class which provides a single method to obtain the sensor information.
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > >
> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > >
> > > > > ---
> > > > >  include/libcamera/internal/meson.build       |  1 +
> > > > >  include/libcamera/internal/sensor_database.h | 28 ++++++
> > > > >  src/libcamera/meson.build                    |  1 +
> > > > >  src/libcamera/sensor_database.cpp            | 97 ++++++++++++++++++++
> > > > >  4 files changed, 127 insertions(+)
> > > > >  create mode 100644 include/libcamera/internal/sensor_database.h
> > > > >  create mode 100644 src/libcamera/sensor_database.cpp
> > > > >
> > > > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > > > > index 1fe3918cfe93..4aaa76ac1db1 100644
> > > > > --- a/include/libcamera/internal/meson.build
> > > > > +++ b/include/libcamera/internal/meson.build
> > > > > @@ -38,6 +38,7 @@ libcamera_internal_headers = files([
> > > > >      'process.h',
> > > > >      'pub_key.h',
> > > > >      'semaphore.h',
> > > > > +    'sensor_database.h',
> > > > >      'sysfs.h',
> > > > >      'thread.h',
> > > > >      'timer.h',
> > > > > diff --git a/include/libcamera/internal/sensor_database.h b/include/libcamera/internal/sensor_database.h
> > > > > new file mode 100644
> > > > > index 000000000000..7d743e46102d
> > > > > --- /dev/null
> > > > > +++ b/include/libcamera/internal/sensor_database.h
> > > > > @@ -0,0 +1,28 @@
> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2021, Google Inc.
> > > > > + *
> > > > > + * sensor_database.h - Database of camera sensor properties
> > > > > + */
> > > > > +#ifndef __LIBCAMERA_SENSOR_DATABASE_H__
> > > > > +#define __LIBCAMERA_SENSOR_DATABASE_H__
> > > > > +
> > > > > +#include <string>
> > > > > +
> > > > > +#include <libcamera/geometry.h>
> > > > > +
> > > > > +namespace libcamera {
> > > > > +
> > > > > +struct SensorInfo {
> >
> > I'd name this CameraSensorInfo, as we may have to support other types of
> > sensor in the future.
> >
> > > > > +     Size unitCellSize;
> > > > > +};
> > > > > +
> > > > > +class SensorDatabase
> > > > > +{
> > > > > +public:
> > > > > +     static const SensorInfo *get(const std::string &sensor);
> > > > > +};
> > > > > +
> > >
> > > Just FYI: https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions
> > > > 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.
> > >
> > > However, I am not against this code. I am personally fine with class +
> > > static member functions.
> >
> > Moving the static function to the SensorInfo class would solve this
> > problem.
> >
> > > > > +} /* namespace libcamera */
> > > > > +
> > > > > +#endif /* __LIBCAMERA_SENSOR_DATABASE_H__ */
> > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > > > index e0a48aa23ea5..effae0ef5186 100644
> > > > > --- a/src/libcamera/meson.build
> > > > > +++ b/src/libcamera/meson.build
> > > > > @@ -44,6 +44,7 @@ libcamera_sources = files([
> > > > >      'pub_key.cpp',
> > > > >      'request.cpp',
> > > > >      'semaphore.cpp',
> > > > > +    'sensor_database.cpp',
> > > > >      'signal.cpp',
> > > > >      'stream.cpp',
> > > > >      'sysfs.cpp',
> > > > > diff --git a/src/libcamera/sensor_database.cpp b/src/libcamera/sensor_database.cpp
> > > > > new file mode 100644
> > > > > index 000000000000..68e69e8bf1b6
> > > > > --- /dev/null
> > > > > +++ b/src/libcamera/sensor_database.cpp
> > > > > @@ -0,0 +1,97 @@
> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2021, Google Inc.
> > > > > + *
> > > > > + * sensor_database.cpp - Database of camera sensor properties
> > > > > + */
> > > > > +
> > > > > +#include "libcamera/internal/sensor_database.h"
> > > > > +
> > > > > +#include <algorithm>
> > > > > +
> > > > > +namespace libcamera {
> >
> > The namespace goes after \file.
> >
> > > > > +
> > > > > +/**
> > > > > + * \file sensor_database.h
> > > > > + * \brief Database of camera sensor properties
> > > > > + *
> > > > > + * The camera sensor database collects information on camera sensors
> >
> > s/information on/static information about/
> >
> > > > > + * which is not possible or desirable to retrieve at run-time.
> >
> > s/which/that/
> > s/run-time/run time/
> >
> > > > > + */
> > > > > +
> > > > > +/**
> > > > > + * \class SensorDatabase
> > > > > + * \brief Access the database of camera sensor properties
> > > > > + *
> > > > > + * The database is indexed using the camera sensor model, as reported by the
> > > > > + * properties::Model property, and for each supported sensor it contains a
> > > > > + * SensorInfo list of properties.
> > > > > + *
> > > > > + * The class provides a single static getInfo() method to access the sensor
> >
> > The function is called get(). But in any case, let's move it to the
> > SensorInfo class.
> >
> > > > > + * database entries. If the sensor is not supported in the database nullptr is
> > > > > + * returned.
> > > > > + */
> > > > > +
> > > > > +/**
> > > > > + * \struct SensorInfo
> > > > > + * \brief List of sensor properties
> > > > > + *
> > > > > + * \var SensorInfo::unitCellSize
> > > > > + * \brief The physical size of pixel, including pixel edges. Width x height in
> >
> > s/pixel/a pixel/
> >
> > > > > + * nano-meters.
> >
> > s/nano-meters/nanometers/
> >
> > This field is a Size, so no need to repeat Width x height.
> >
> >  * \brief The physical size of a pixel, including pixel edges, in nanometers
> >
> > > > > + */
> > > > > +
> > > > > +namespace {
> > > > > +
> > > > > +/**
> > > > > + * \brief Sony IMX219 sensor properties
> > > > > + */
> > > > > +constexpr SensorInfo imx219Info = {
> > > > > +     .unitCellSize = { 1120, 1120 }
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * \brief Omnivision ov5670 sensor properties
> > > > > + */
> > > > > +constexpr SensorInfo ov5670Info = {
> > > > > +     .unitCellSize = { 1120, 1120 }
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * \brief Omnivision 13858 sensor properties
> > > > > + */
> > > > > +constexpr SensorInfo ov13858Info = {
> > > > > +     .unitCellSize = { 1120, 1120 }
> > > > > +};
> > > > > +
> > > > > +#define SENSOR_INFO(_sensor) \
> > > > > +     { #_sensor, &_sensor##Info }
> > > > > +
> > > > > +/*
> > > > > + * \brief The database of sensor properties
> > > > > + */
> > > > > +constexpr std::pair<const char *const, const SensorInfo *> sensorDatabase__[] = {
> > > > > +     SENSOR_INFO(imx219),
> > > > > +     SENSOR_INFO(ov5670),
> > > > > +     SENSOR_INFO(ov13858),
> > > > > +};
> > > > > +
> > > > > +} /* namespace */
> > > > > +
> > > > > +/**
> > > > > + * \brief Retrieve the properties associated with a sensor
> > > > > + * \param sensor The sensor model name
> > > > > + * \return A pointer to the SensorInfo instance associated with a sensor or
> > > > > + * nullptr if the sensor is not supported in the database
> > > > > + */
> > > > > +const SensorInfo *SensorDatabase::get(const std::string &sensor)
> > > > > +{
> > > > > +     for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {
> > > > > +             if (sensorDatabase__[i].first == sensor)
> > > > > +                     return sensorDatabase__[i].second;
> > > > > +     }
> >
> > Why not std::map ?
> 
> I expect the reason is std::map cannot be constructed as constexpr.

But the only function that uses it is get(), which isn't constexpr, so
is that really a problem ?

> > namespace {
> >
> > const std::map<std::string, SensorInfo> sensorDatabase{
> >         { "imx219", {
> >                 .unitCellSize = { 1120, 1120 },
> >         } },
> >         ...
> > };
> >
> > } /* namespace */
> >
> > and this function can become
> >
> > const CameraSensorInfo *CameraSensorInfo::get(const std::string &sensor)
> > {
> >         auto iter = sensorDatabase.find(sensor);
> >         if (iter == sensorDatabase.end())
> >                 return nullptr;
> >
> >         return *iter;
> > }
> >
> > > > > +
> > > > > +     return nullptr;
> > > > > +}
> > > > > +
> > > > > +} /* namespace libcamera */
> > >
> > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>

On Tue, Apr 13, 2021 at 11:03 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Tue, Apr 13, 2021 at 10:56:40AM +0900, Hirokazu Honda wrote:
> > On Tue, Apr 13, 2021 at 10:45 AM Laurent Pinchart wrote:
> > > On Mon, Apr 12, 2021 at 10:07:17PM +0900, Hirokazu Honda wrote:
> > > > On Mon, Apr 12, 2021 at 6:10 PM Niklas Söderlund wrote:
> > > > > On 2021-04-12 09:57:00 +0200, Jacopo Mondi wrote:
> > > > > > Introduce a 'database' of camera sensor information, which contains
> > > > > > a the camera sensor properties which is not possible, or desirable,
> > >
> > > s/is not/are not/
> > >
> > > > > s/^a //
> > > > >
> > > > > > to retrieve at run-time.
> > > > > >
> > > > > > The camera sensor database is accessed through the static SensorDatabase
> > > > > > class which provides a single method to obtain the sensor information.
> > > > > >
> > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > >
> > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > > >
> > > > > > ---
> > > > > >  include/libcamera/internal/meson.build       |  1 +
> > > > > >  include/libcamera/internal/sensor_database.h | 28 ++++++
> > > > > >  src/libcamera/meson.build                    |  1 +
> > > > > >  src/libcamera/sensor_database.cpp            | 97 ++++++++++++++++++++
> > > > > >  4 files changed, 127 insertions(+)
> > > > > >  create mode 100644 include/libcamera/internal/sensor_database.h
> > > > > >  create mode 100644 src/libcamera/sensor_database.cpp
> > > > > >
> > > > > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > > > > > index 1fe3918cfe93..4aaa76ac1db1 100644
> > > > > > --- a/include/libcamera/internal/meson.build
> > > > > > +++ b/include/libcamera/internal/meson.build
> > > > > > @@ -38,6 +38,7 @@ libcamera_internal_headers = files([
> > > > > >      'process.h',
> > > > > >      'pub_key.h',
> > > > > >      'semaphore.h',
> > > > > > +    'sensor_database.h',
> > > > > >      'sysfs.h',
> > > > > >      'thread.h',
> > > > > >      'timer.h',
> > > > > > diff --git a/include/libcamera/internal/sensor_database.h b/include/libcamera/internal/sensor_database.h
> > > > > > new file mode 100644
> > > > > > index 000000000000..7d743e46102d
> > > > > > --- /dev/null
> > > > > > +++ b/include/libcamera/internal/sensor_database.h
> > > > > > @@ -0,0 +1,28 @@
> > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > > +/*
> > > > > > + * Copyright (C) 2021, Google Inc.
> > > > > > + *
> > > > > > + * sensor_database.h - Database of camera sensor properties
> > > > > > + */
> > > > > > +#ifndef __LIBCAMERA_SENSOR_DATABASE_H__
> > > > > > +#define __LIBCAMERA_SENSOR_DATABASE_H__
> > > > > > +
> > > > > > +#include <string>
> > > > > > +
> > > > > > +#include <libcamera/geometry.h>
> > > > > > +
> > > > > > +namespace libcamera {
> > > > > > +
> > > > > > +struct SensorInfo {
> > >
> > > I'd name this CameraSensorInfo, as we may have to support other types of
> > > sensor in the future.
> > >
> > > > > > +     Size unitCellSize;
> > > > > > +};
> > > > > > +
> > > > > > +class SensorDatabase
> > > > > > +{
> > > > > > +public:
> > > > > > +     static const SensorInfo *get(const std::string &sensor);
> > > > > > +};
> > > > > > +
> > > >
> > > > Just FYI: https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions
> > > > > 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.
> > > >
> > > > However, I am not against this code. I am personally fine with class +
> > > > static member functions.
> > >
> > > Moving the static function to the SensorInfo class would solve this
> > > problem.
> > >
> > > > > > +} /* namespace libcamera */
> > > > > > +
> > > > > > +#endif /* __LIBCAMERA_SENSOR_DATABASE_H__ */
> > > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > > > > index e0a48aa23ea5..effae0ef5186 100644
> > > > > > --- a/src/libcamera/meson.build
> > > > > > +++ b/src/libcamera/meson.build
> > > > > > @@ -44,6 +44,7 @@ libcamera_sources = files([
> > > > > >      'pub_key.cpp',
> > > > > >      'request.cpp',
> > > > > >      'semaphore.cpp',
> > > > > > +    'sensor_database.cpp',
> > > > > >      'signal.cpp',
> > > > > >      'stream.cpp',
> > > > > >      'sysfs.cpp',
> > > > > > diff --git a/src/libcamera/sensor_database.cpp b/src/libcamera/sensor_database.cpp
> > > > > > new file mode 100644
> > > > > > index 000000000000..68e69e8bf1b6
> > > > > > --- /dev/null
> > > > > > +++ b/src/libcamera/sensor_database.cpp
> > > > > > @@ -0,0 +1,97 @@
> > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > > +/*
> > > > > > + * Copyright (C) 2021, Google Inc.
> > > > > > + *
> > > > > > + * sensor_database.cpp - Database of camera sensor properties
> > > > > > + */
> > > > > > +
> > > > > > +#include "libcamera/internal/sensor_database.h"
> > > > > > +
> > > > > > +#include <algorithm>
> > > > > > +
> > > > > > +namespace libcamera {
> > >
> > > The namespace goes after \file.
> > >
> > > > > > +
> > > > > > +/**
> > > > > > + * \file sensor_database.h
> > > > > > + * \brief Database of camera sensor properties
> > > > > > + *
> > > > > > + * The camera sensor database collects information on camera sensors
> > >
> > > s/information on/static information about/
> > >
> > > > > > + * which is not possible or desirable to retrieve at run-time.
> > >
> > > s/which/that/
> > > s/run-time/run time/
> > >
> > > > > > + */
> > > > > > +
> > > > > > +/**
> > > > > > + * \class SensorDatabase
> > > > > > + * \brief Access the database of camera sensor properties
> > > > > > + *
> > > > > > + * The database is indexed using the camera sensor model, as reported by the
> > > > > > + * properties::Model property, and for each supported sensor it contains a
> > > > > > + * SensorInfo list of properties.
> > > > > > + *
> > > > > > + * The class provides a single static getInfo() method to access the sensor
> > >
> > > The function is called get(). But in any case, let's move it to the
> > > SensorInfo class.
> > >
> > > > > > + * database entries. If the sensor is not supported in the database nullptr is
> > > > > > + * returned.
> > > > > > + */
> > > > > > +
> > > > > > +/**
> > > > > > + * \struct SensorInfo
> > > > > > + * \brief List of sensor properties
> > > > > > + *
> > > > > > + * \var SensorInfo::unitCellSize
> > > > > > + * \brief The physical size of pixel, including pixel edges. Width x height in
> > >
> > > s/pixel/a pixel/
> > >
> > > > > > + * nano-meters.
> > >
> > > s/nano-meters/nanometers/
> > >
> > > This field is a Size, so no need to repeat Width x height.
> > >
> > >  * \brief The physical size of a pixel, including pixel edges, in nanometers
> > >
> > > > > > + */
> > > > > > +
> > > > > > +namespace {
> > > > > > +
> > > > > > +/**
> > > > > > + * \brief Sony IMX219 sensor properties
> > > > > > + */
> > > > > > +constexpr SensorInfo imx219Info = {
> > > > > > +     .unitCellSize = { 1120, 1120 }
> > > > > > +};
> > > > > > +
> > > > > > +/**
> > > > > > + * \brief Omnivision ov5670 sensor properties
> > > > > > + */
> > > > > > +constexpr SensorInfo ov5670Info = {
> > > > > > +     .unitCellSize = { 1120, 1120 }
> > > > > > +};
> > > > > > +
> > > > > > +/**
> > > > > > + * \brief Omnivision 13858 sensor properties
> > > > > > + */
> > > > > > +constexpr SensorInfo ov13858Info = {
> > > > > > +     .unitCellSize = { 1120, 1120 }
> > > > > > +};
> > > > > > +
> > > > > > +#define SENSOR_INFO(_sensor) \
> > > > > > +     { #_sensor, &_sensor##Info }
> > > > > > +
> > > > > > +/*
> > > > > > + * \brief The database of sensor properties
> > > > > > + */
> > > > > > +constexpr std::pair<const char *const, const SensorInfo *> sensorDatabase__[] = {
> > > > > > +     SENSOR_INFO(imx219),
> > > > > > +     SENSOR_INFO(ov5670),
> > > > > > +     SENSOR_INFO(ov13858),
> > > > > > +};
> > > > > > +
> > > > > > +} /* namespace */
> > > > > > +
> > > > > > +/**
> > > > > > + * \brief Retrieve the properties associated with a sensor
> > > > > > + * \param sensor The sensor model name
> > > > > > + * \return A pointer to the SensorInfo instance associated with a sensor or
> > > > > > + * nullptr if the sensor is not supported in the database
> > > > > > + */
> > > > > > +const SensorInfo *SensorDatabase::get(const std::string &sensor)
> > > > > > +{
> > > > > > +     for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {
> > > > > > +             if (sensorDatabase__[i].first == sensor)
> > > > > > +                     return sensorDatabase__[i].second;
> > > > > > +     }
> > >
> > > Why not std::map ?
> >
> > I expect the reason is std::map cannot be constructed as constexpr.
>
> But the only function that uses it is get(), which isn't constexpr, so
> is that really a problem ?
>

Global and static function should be trivially destructible.
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

> > > namespace {
> > >
> > > const std::map<std::string, SensorInfo> sensorDatabase{
> > >         { "imx219", {
> > >                 .unitCellSize = { 1120, 1120 },
> > >         } },
> > >         ...
> > > };
> > >
> > > } /* namespace */
> > >
> > > and this function can become
> > >
> > > const CameraSensorInfo *CameraSensorInfo::get(const std::string &sensor)
> > > {
> > >         auto iter = sensorDatabase.find(sensor);
> > >         if (iter == sensorDatabase.end())
> > >                 return nullptr;
> > >
> > >         return *iter;
> > > }
> > >
> > > > > > +
> > > > > > +     return nullptr;
> > > > > > +}
> > > > > > +
> > > > > > +} /* namespace libcamera */
> > > >
> > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
>
> --
> Regards,
>
> Laurent Pinchart

Hi Hiro,

On Tue, Apr 13, 2021 at 11:19:03AM +0900, Hirokazu Honda wrote:
> On Tue, Apr 13, 2021 at 11:03 AM Laurent Pinchart wrote:
> > On Tue, Apr 13, 2021 at 10:56:40AM +0900, Hirokazu Honda wrote:
> > > On Tue, Apr 13, 2021 at 10:45 AM Laurent Pinchart wrote:
> > > > On Mon, Apr 12, 2021 at 10:07:17PM +0900, Hirokazu Honda wrote:
> > > > > On Mon, Apr 12, 2021 at 6:10 PM Niklas Söderlund wrote:
> > > > > > On 2021-04-12 09:57:00 +0200, Jacopo Mondi wrote:
> > > > > > > Introduce a 'database' of camera sensor information, which contains
> > > > > > > a the camera sensor properties which is not possible, or desirable,
> > > >
> > > > s/is not/are not/
> > > >
> > > > > > s/^a //
> > > > > >
> > > > > > > to retrieve at run-time.
> > > > > > >
> > > > > > > The camera sensor database is accessed through the static SensorDatabase
> > > > > > > class which provides a single method to obtain the sensor information.
> > > > > > >
> > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > >
> > > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > > > >
> > > > > > > ---
> > > > > > >  include/libcamera/internal/meson.build       |  1 +
> > > > > > >  include/libcamera/internal/sensor_database.h | 28 ++++++
> > > > > > >  src/libcamera/meson.build                    |  1 +
> > > > > > >  src/libcamera/sensor_database.cpp            | 97 ++++++++++++++++++++
> > > > > > >  4 files changed, 127 insertions(+)
> > > > > > >  create mode 100644 include/libcamera/internal/sensor_database.h
> > > > > > >  create mode 100644 src/libcamera/sensor_database.cpp
> > > > > > >
> > > > > > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > > > > > > index 1fe3918cfe93..4aaa76ac1db1 100644
> > > > > > > --- a/include/libcamera/internal/meson.build
> > > > > > > +++ b/include/libcamera/internal/meson.build
> > > > > > > @@ -38,6 +38,7 @@ libcamera_internal_headers = files([
> > > > > > >      'process.h',
> > > > > > >      'pub_key.h',
> > > > > > >      'semaphore.h',
> > > > > > > +    'sensor_database.h',
> > > > > > >      'sysfs.h',
> > > > > > >      'thread.h',
> > > > > > >      'timer.h',
> > > > > > > diff --git a/include/libcamera/internal/sensor_database.h b/include/libcamera/internal/sensor_database.h
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..7d743e46102d
> > > > > > > --- /dev/null
> > > > > > > +++ b/include/libcamera/internal/sensor_database.h
> > > > > > > @@ -0,0 +1,28 @@
> > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > > > +/*
> > > > > > > + * Copyright (C) 2021, Google Inc.
> > > > > > > + *
> > > > > > > + * sensor_database.h - Database of camera sensor properties
> > > > > > > + */
> > > > > > > +#ifndef __LIBCAMERA_SENSOR_DATABASE_H__
> > > > > > > +#define __LIBCAMERA_SENSOR_DATABASE_H__
> > > > > > > +
> > > > > > > +#include <string>
> > > > > > > +
> > > > > > > +#include <libcamera/geometry.h>
> > > > > > > +
> > > > > > > +namespace libcamera {
> > > > > > > +
> > > > > > > +struct SensorInfo {
> > > >
> > > > I'd name this CameraSensorInfo, as we may have to support other types of
> > > > sensor in the future.
> > > >
> > > > > > > +     Size unitCellSize;
> > > > > > > +};
> > > > > > > +
> > > > > > > +class SensorDatabase
> > > > > > > +{
> > > > > > > +public:
> > > > > > > +     static const SensorInfo *get(const std::string &sensor);
> > > > > > > +};
> > > > > > > +
> > > > >
> > > > > Just FYI: https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions
> > > > > > 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.
> > > > >
> > > > > However, I am not against this code. I am personally fine with class +
> > > > > static member functions.
> > > >
> > > > Moving the static function to the SensorInfo class would solve this
> > > > problem.
> > > >
> > > > > > > +} /* namespace libcamera */
> > > > > > > +
> > > > > > > +#endif /* __LIBCAMERA_SENSOR_DATABASE_H__ */
> > > > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > > > > > index e0a48aa23ea5..effae0ef5186 100644
> > > > > > > --- a/src/libcamera/meson.build
> > > > > > > +++ b/src/libcamera/meson.build
> > > > > > > @@ -44,6 +44,7 @@ libcamera_sources = files([
> > > > > > >      'pub_key.cpp',
> > > > > > >      'request.cpp',
> > > > > > >      'semaphore.cpp',
> > > > > > > +    'sensor_database.cpp',
> > > > > > >      'signal.cpp',
> > > > > > >      'stream.cpp',
> > > > > > >      'sysfs.cpp',
> > > > > > > diff --git a/src/libcamera/sensor_database.cpp b/src/libcamera/sensor_database.cpp
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..68e69e8bf1b6
> > > > > > > --- /dev/null
> > > > > > > +++ b/src/libcamera/sensor_database.cpp
> > > > > > > @@ -0,0 +1,97 @@
> > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > > > +/*
> > > > > > > + * Copyright (C) 2021, Google Inc.
> > > > > > > + *
> > > > > > > + * sensor_database.cpp - Database of camera sensor properties
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include "libcamera/internal/sensor_database.h"
> > > > > > > +
> > > > > > > +#include <algorithm>
> > > > > > > +
> > > > > > > +namespace libcamera {
> > > >
> > > > The namespace goes after \file.
> > > >
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * \file sensor_database.h
> > > > > > > + * \brief Database of camera sensor properties
> > > > > > > + *
> > > > > > > + * The camera sensor database collects information on camera sensors
> > > >
> > > > s/information on/static information about/
> > > >
> > > > > > > + * which is not possible or desirable to retrieve at run-time.
> > > >
> > > > s/which/that/
> > > > s/run-time/run time/
> > > >
> > > > > > > + */
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * \class SensorDatabase
> > > > > > > + * \brief Access the database of camera sensor properties
> > > > > > > + *
> > > > > > > + * The database is indexed using the camera sensor model, as reported by the
> > > > > > > + * properties::Model property, and for each supported sensor it contains a
> > > > > > > + * SensorInfo list of properties.
> > > > > > > + *
> > > > > > > + * The class provides a single static getInfo() method to access the sensor
> > > >
> > > > The function is called get(). But in any case, let's move it to the
> > > > SensorInfo class.
> > > >
> > > > > > > + * database entries. If the sensor is not supported in the database nullptr is
> > > > > > > + * returned.
> > > > > > > + */
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * \struct SensorInfo
> > > > > > > + * \brief List of sensor properties
> > > > > > > + *
> > > > > > > + * \var SensorInfo::unitCellSize
> > > > > > > + * \brief The physical size of pixel, including pixel edges. Width x height in
> > > >
> > > > s/pixel/a pixel/
> > > >
> > > > > > > + * nano-meters.
> > > >
> > > > s/nano-meters/nanometers/
> > > >
> > > > This field is a Size, so no need to repeat Width x height.
> > > >
> > > >  * \brief The physical size of a pixel, including pixel edges, in nanometers
> > > >
> > > > > > > + */
> > > > > > > +
> > > > > > > +namespace {
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * \brief Sony IMX219 sensor properties
> > > > > > > + */
> > > > > > > +constexpr SensorInfo imx219Info = {
> > > > > > > +     .unitCellSize = { 1120, 1120 }
> > > > > > > +};
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * \brief Omnivision ov5670 sensor properties
> > > > > > > + */
> > > > > > > +constexpr SensorInfo ov5670Info = {
> > > > > > > +     .unitCellSize = { 1120, 1120 }
> > > > > > > +};
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * \brief Omnivision 13858 sensor properties
> > > > > > > + */
> > > > > > > +constexpr SensorInfo ov13858Info = {
> > > > > > > +     .unitCellSize = { 1120, 1120 }
> > > > > > > +};
> > > > > > > +
> > > > > > > +#define SENSOR_INFO(_sensor) \
> > > > > > > +     { #_sensor, &_sensor##Info }
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * \brief The database of sensor properties
> > > > > > > + */
> > > > > > > +constexpr std::pair<const char *const, const SensorInfo *> sensorDatabase__[] = {
> > > > > > > +     SENSOR_INFO(imx219),
> > > > > > > +     SENSOR_INFO(ov5670),
> > > > > > > +     SENSOR_INFO(ov13858),
> > > > > > > +};
> > > > > > > +
> > > > > > > +} /* namespace */
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * \brief Retrieve the properties associated with a sensor
> > > > > > > + * \param sensor The sensor model name
> > > > > > > + * \return A pointer to the SensorInfo instance associated with a sensor or
> > > > > > > + * nullptr if the sensor is not supported in the database
> > > > > > > + */
> > > > > > > +const SensorInfo *SensorDatabase::get(const std::string &sensor)
> > > > > > > +{
> > > > > > > +     for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {
> > > > > > > +             if (sensorDatabase__[i].first == sensor)
> > > > > > > +                     return sensorDatabase__[i].second;
> > > > > > > +     }
> > > >
> > > > Why not std::map ?
> > >
> > > I expect the reason is std::map cannot be constructed as constexpr.
> >
> > But the only function that uses it is get(), which isn't constexpr, so
> > is that really a problem ?
> 
> Global and static function should be trivially destructible.
> https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

We have a more relaxed rule, documented in
Documentation/coding_style.rst. In this case, a map is much more
convenient, and as its initialization and destruction is independent of
all other global variables, it doesn't risk causing most of the issues
mentioned in the above link.

> > > > namespace {
> > > >
> > > > const std::map<std::string, SensorInfo> sensorDatabase{
> > > >         { "imx219", {
> > > >                 .unitCellSize = { 1120, 1120 },
> > > >         } },
> > > >         ...
> > > > };
> > > >
> > > > } /* namespace */
> > > >
> > > > and this function can become
> > > >
> > > > const CameraSensorInfo *CameraSensorInfo::get(const std::string &sensor)
> > > > {
> > > >         auto iter = sensorDatabase.find(sensor);
> > > >         if (iter == sensorDatabase.end())
> > > >                 return nullptr;
> > > >
> > > >         return *iter;
> > > > }
> > > >
> > > > > > > +
> > > > > > > +     return nullptr;
> > > > > > > +}
> > > > > > > +
> > > > > > > +} /* namespace libcamera */
> > > > >
> > > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>

Hi Laurent,

On Tue, Apr 13, 2021 at 11:42 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Tue, Apr 13, 2021 at 11:19:03AM +0900, Hirokazu Honda wrote:
> > On Tue, Apr 13, 2021 at 11:03 AM Laurent Pinchart wrote:
> > > On Tue, Apr 13, 2021 at 10:56:40AM +0900, Hirokazu Honda wrote:
> > > > On Tue, Apr 13, 2021 at 10:45 AM Laurent Pinchart wrote:
> > > > > On Mon, Apr 12, 2021 at 10:07:17PM +0900, Hirokazu Honda wrote:
> > > > > > On Mon, Apr 12, 2021 at 6:10 PM Niklas Söderlund wrote:
> > > > > > > On 2021-04-12 09:57:00 +0200, Jacopo Mondi wrote:
> > > > > > > > Introduce a 'database' of camera sensor information, which contains
> > > > > > > > a the camera sensor properties which is not possible, or desirable,
> > > > >
> > > > > s/is not/are not/
> > > > >
> > > > > > > s/^a //
> > > > > > >
> > > > > > > > to retrieve at run-time.
> > > > > > > >
> > > > > > > > The camera sensor database is accessed through the static SensorDatabase
> > > > > > > > class which provides a single method to obtain the sensor information.
> > > > > > > >
> > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > > >
> > > > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > > > > >
> > > > > > > > ---
> > > > > > > >  include/libcamera/internal/meson.build       |  1 +
> > > > > > > >  include/libcamera/internal/sensor_database.h | 28 ++++++
> > > > > > > >  src/libcamera/meson.build                    |  1 +
> > > > > > > >  src/libcamera/sensor_database.cpp            | 97 ++++++++++++++++++++
> > > > > > > >  4 files changed, 127 insertions(+)
> > > > > > > >  create mode 100644 include/libcamera/internal/sensor_database.h
> > > > > > > >  create mode 100644 src/libcamera/sensor_database.cpp
> > > > > > > >
> > > > > > > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > > > > > > > index 1fe3918cfe93..4aaa76ac1db1 100644
> > > > > > > > --- a/include/libcamera/internal/meson.build
> > > > > > > > +++ b/include/libcamera/internal/meson.build
> > > > > > > > @@ -38,6 +38,7 @@ libcamera_internal_headers = files([
> > > > > > > >      'process.h',
> > > > > > > >      'pub_key.h',
> > > > > > > >      'semaphore.h',
> > > > > > > > +    'sensor_database.h',
> > > > > > > >      'sysfs.h',
> > > > > > > >      'thread.h',
> > > > > > > >      'timer.h',
> > > > > > > > diff --git a/include/libcamera/internal/sensor_database.h b/include/libcamera/internal/sensor_database.h
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..7d743e46102d
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/include/libcamera/internal/sensor_database.h
> > > > > > > > @@ -0,0 +1,28 @@
> > > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > > > > +/*
> > > > > > > > + * Copyright (C) 2021, Google Inc.
> > > > > > > > + *
> > > > > > > > + * sensor_database.h - Database of camera sensor properties
> > > > > > > > + */
> > > > > > > > +#ifndef __LIBCAMERA_SENSOR_DATABASE_H__
> > > > > > > > +#define __LIBCAMERA_SENSOR_DATABASE_H__
> > > > > > > > +
> > > > > > > > +#include <string>
> > > > > > > > +
> > > > > > > > +#include <libcamera/geometry.h>
> > > > > > > > +
> > > > > > > > +namespace libcamera {
> > > > > > > > +
> > > > > > > > +struct SensorInfo {
> > > > >
> > > > > I'd name this CameraSensorInfo, as we may have to support other types of
> > > > > sensor in the future.
> > > > >
> > > > > > > > +     Size unitCellSize;
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +class SensorDatabase
> > > > > > > > +{
> > > > > > > > +public:
> > > > > > > > +     static const SensorInfo *get(const std::string &sensor);
> > > > > > > > +};
> > > > > > > > +
> > > > > >
> > > > > > Just FYI: https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions
> > > > > > > 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.
> > > > > >
> > > > > > However, I am not against this code. I am personally fine with class +
> > > > > > static member functions.
> > > > >
> > > > > Moving the static function to the SensorInfo class would solve this
> > > > > problem.
> > > > >
> > > > > > > > +} /* namespace libcamera */
> > > > > > > > +
> > > > > > > > +#endif /* __LIBCAMERA_SENSOR_DATABASE_H__ */
> > > > > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > > > > > > index e0a48aa23ea5..effae0ef5186 100644
> > > > > > > > --- a/src/libcamera/meson.build
> > > > > > > > +++ b/src/libcamera/meson.build
> > > > > > > > @@ -44,6 +44,7 @@ libcamera_sources = files([
> > > > > > > >      'pub_key.cpp',
> > > > > > > >      'request.cpp',
> > > > > > > >      'semaphore.cpp',
> > > > > > > > +    'sensor_database.cpp',
> > > > > > > >      'signal.cpp',
> > > > > > > >      'stream.cpp',
> > > > > > > >      'sysfs.cpp',
> > > > > > > > diff --git a/src/libcamera/sensor_database.cpp b/src/libcamera/sensor_database.cpp
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..68e69e8bf1b6
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/src/libcamera/sensor_database.cpp
> > > > > > > > @@ -0,0 +1,97 @@
> > > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > > > > +/*
> > > > > > > > + * Copyright (C) 2021, Google Inc.
> > > > > > > > + *
> > > > > > > > + * sensor_database.cpp - Database of camera sensor properties
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > +#include "libcamera/internal/sensor_database.h"
> > > > > > > > +
> > > > > > > > +#include <algorithm>
> > > > > > > > +
> > > > > > > > +namespace libcamera {
> > > > >
> > > > > The namespace goes after \file.
> > > > >
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * \file sensor_database.h
> > > > > > > > + * \brief Database of camera sensor properties
> > > > > > > > + *
> > > > > > > > + * The camera sensor database collects information on camera sensors
> > > > >
> > > > > s/information on/static information about/
> > > > >
> > > > > > > > + * which is not possible or desirable to retrieve at run-time.
> > > > >
> > > > > s/which/that/
> > > > > s/run-time/run time/
> > > > >
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * \class SensorDatabase
> > > > > > > > + * \brief Access the database of camera sensor properties
> > > > > > > > + *
> > > > > > > > + * The database is indexed using the camera sensor model, as reported by the
> > > > > > > > + * properties::Model property, and for each supported sensor it contains a
> > > > > > > > + * SensorInfo list of properties.
> > > > > > > > + *
> > > > > > > > + * The class provides a single static getInfo() method to access the sensor
> > > > >
> > > > > The function is called get(). But in any case, let's move it to the
> > > > > SensorInfo class.
> > > > >
> > > > > > > > + * database entries. If the sensor is not supported in the database nullptr is
> > > > > > > > + * returned.
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * \struct SensorInfo
> > > > > > > > + * \brief List of sensor properties
> > > > > > > > + *
> > > > > > > > + * \var SensorInfo::unitCellSize
> > > > > > > > + * \brief The physical size of pixel, including pixel edges. Width x height in
> > > > >
> > > > > s/pixel/a pixel/
> > > > >
> > > > > > > > + * nano-meters.
> > > > >
> > > > > s/nano-meters/nanometers/
> > > > >
> > > > > This field is a Size, so no need to repeat Width x height.
> > > > >
> > > > >  * \brief The physical size of a pixel, including pixel edges, in nanometers
> > > > >
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > +namespace {
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * \brief Sony IMX219 sensor properties
> > > > > > > > + */
> > > > > > > > +constexpr SensorInfo imx219Info = {
> > > > > > > > +     .unitCellSize = { 1120, 1120 }
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * \brief Omnivision ov5670 sensor properties
> > > > > > > > + */
> > > > > > > > +constexpr SensorInfo ov5670Info = {
> > > > > > > > +     .unitCellSize = { 1120, 1120 }
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * \brief Omnivision 13858 sensor properties
> > > > > > > > + */
> > > > > > > > +constexpr SensorInfo ov13858Info = {
> > > > > > > > +     .unitCellSize = { 1120, 1120 }
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +#define SENSOR_INFO(_sensor) \
> > > > > > > > +     { #_sensor, &_sensor##Info }
> > > > > > > > +
> > > > > > > > +/*
> > > > > > > > + * \brief The database of sensor properties
> > > > > > > > + */
> > > > > > > > +constexpr std::pair<const char *const, const SensorInfo *> sensorDatabase__[] = {
> > > > > > > > +     SENSOR_INFO(imx219),
> > > > > > > > +     SENSOR_INFO(ov5670),
> > > > > > > > +     SENSOR_INFO(ov13858),
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +} /* namespace */
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * \brief Retrieve the properties associated with a sensor
> > > > > > > > + * \param sensor The sensor model name
> > > > > > > > + * \return A pointer to the SensorInfo instance associated with a sensor or
> > > > > > > > + * nullptr if the sensor is not supported in the database
> > > > > > > > + */
> > > > > > > > +const SensorInfo *SensorDatabase::get(const std::string &sensor)
> > > > > > > > +{
> > > > > > > > +     for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {
> > > > > > > > +             if (sensorDatabase__[i].first == sensor)
> > > > > > > > +                     return sensorDatabase__[i].second;
> > > > > > > > +     }
> > > > >
> > > > > Why not std::map ?
> > > >
> > > > I expect the reason is std::map cannot be constructed as constexpr.
> > >
> > > But the only function that uses it is get(), which isn't constexpr, so
> > > is that really a problem ?
> >
> > Global and static function should be trivially destructible.
> > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
>
> We have a more relaxed rule, documented in
> Documentation/coding_style.rst. In this case, a map is much more
> convenient, and as its initialization and destruction is independent of
> all other global variables, it doesn't risk causing most of the issues
> mentioned in the above link.
>

Thanks. I don't remember the description, although I think I read it previously.
That sounds good to me. Perhaps, the map would be put as a const
static variable within CameraSensorInfo::get().

> > > > > namespace {
> > > > >
> > > > > const std::map<std::string, SensorInfo> sensorDatabase{
> > > > >         { "imx219", {
> > > > >                 .unitCellSize = { 1120, 1120 },
> > > > >         } },
> > > > >         ...
> > > > > };
> > > > >
> > > > > } /* namespace */
> > > > >
> > > > > and this function can become
> > > > >
> > > > > const CameraSensorInfo *CameraSensorInfo::get(const std::string &sensor)
> > > > > {
> > > > >         auto iter = sensorDatabase.find(sensor);
> > > > >         if (iter == sensorDatabase.end())
> > > > >                 return nullptr;
> > > > >
> > > > >         return *iter;
> > > > > }
> > > > >
> > > > > > > > +
> > > > > > > > +     return nullptr;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +} /* namespace libcamera */
> > > > > >
> > > > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
>
> --
> Regards,
>
> Laurent Pinchart

Hello,

On Tue, Apr 13, 2021 at 04:00:25PM +0900, Hirokazu Honda wrote:
> > > > > > > > > +
> > > > > > > > > +/**
> > > > > > > > > + * \brief Retrieve the properties associated with a sensor
> > > > > > > > > + * \param sensor The sensor model name
> > > > > > > > > + * \return A pointer to the SensorInfo instance associated with a sensor or
> > > > > > > > > + * nullptr if the sensor is not supported in the database
> > > > > > > > > + */
> > > > > > > > > +const SensorInfo *SensorDatabase::get(const std::string &sensor)
> > > > > > > > > +{
> > > > > > > > > +     for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {
> > > > > > > > > +             if (sensorDatabase__[i].first == sensor)
> > > > > > > > > +                     return sensorDatabase__[i].second;
> > > > > > > > > +     }
> > > > > >
> > > > > > Why not std::map ?
> > > > >
> > > > > I expect the reason is std::map cannot be constructed as constexpr.
> > > >
> > > > But the only function that uses it is get(), which isn't constexpr, so
> > > > is that really a problem ?
> > >
> > > Global and static function should be trivially destructible.
> > > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
> >
> > We have a more relaxed rule, documented in
> > Documentation/coding_style.rst. In this case, a map is much more
> > convenient, and as its initialization and destruction is independent of
> > all other global variables, it doesn't risk causing most of the issues
> > mentioned in the above link.
> >

Indeed I used std::pair to be able to declare it as constexpr.

I'll make it a map if that's a not a requirement. I'll see if it's
possbile to statically initialize it as I'm currently doing with the
pair

>
> Thanks. I don't remember the description, although I think I read it previously.
> That sounds good to me. Perhaps, the map would be put as a const
> static variable within CameraSensorInfo::get().
>

The map needs to be statically initialized, it could be done inside
the ::get() function most probably. Something like (not tested)

const SensorInfo *SensorDatabase::get(const std::string &sensor)
{
	static const std::map<const char *const, const SensorInfo *> sensorDb = {
		SENSOR_INFO(imx219),
		SENSOR_INFO(ov5670),
		SENSOR_INFO(ov13858),
	};

	const auto it = sensorDb.find(sensor.c_str());
	if (it == sensorDb.end())
		return nullptr;

	return it->second;
}

Thanks
   j



> > > > > > namespace {
> > > > > >
> > > > > > const std::map<std::string, SensorInfo> sensorDatabase{
> > > > > >         { "imx219", {
> > > > > >                 .unitCellSize = { 1120, 1120 },
> > > > > >         } },
> > > > > >         ...
> > > > > > };
> > > > > >
> > > > > > } /* namespace */
> > > > > >
> > > > > > and this function can become
> > > > > >
> > > > > > const CameraSensorInfo *CameraSensorInfo::get(const std::string &sensor)
> > > > > > {
> > > > > >         auto iter = sensorDatabase.find(sensor);
> > > > > >         if (iter == sensorDatabase.end())
> > > > > >                 return nullptr;
> > > > > >
> > > > > >         return *iter;
> > > > > > }
> > > > > >
> > > > > > > > > +
> > > > > > > > > +     return nullptr;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +} /* namespace libcamera */
> > > > > > >
> > > > > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> >
> > --
> > Regards,
> >
> > Laurent Pinchart

Hi Jacopo,

On Tue, Apr 13, 2021 at 4:39 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hello,
>
> On Tue, Apr 13, 2021 at 04:00:25PM +0900, Hirokazu Honda wrote:
> > > > > > > > > > +
> > > > > > > > > > +/**
> > > > > > > > > > + * \brief Retrieve the properties associated with a sensor
> > > > > > > > > > + * \param sensor The sensor model name
> > > > > > > > > > + * \return A pointer to the SensorInfo instance associated with a sensor or
> > > > > > > > > > + * nullptr if the sensor is not supported in the database
> > > > > > > > > > + */
> > > > > > > > > > +const SensorInfo *SensorDatabase::get(const std::string &sensor)
> > > > > > > > > > +{
> > > > > > > > > > +     for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {
> > > > > > > > > > +             if (sensorDatabase__[i].first == sensor)
> > > > > > > > > > +                     return sensorDatabase__[i].second;
> > > > > > > > > > +     }
> > > > > > >
> > > > > > > Why not std::map ?
> > > > > >
> > > > > > I expect the reason is std::map cannot be constructed as constexpr.
> > > > >
> > > > > But the only function that uses it is get(), which isn't constexpr, so
> > > > > is that really a problem ?
> > > >
> > > > Global and static function should be trivially destructible.
> > > > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
> > >
> > > We have a more relaxed rule, documented in
> > > Documentation/coding_style.rst. In this case, a map is much more
> > > convenient, and as its initialization and destruction is independent of
> > > all other global variables, it doesn't risk causing most of the issues
> > > mentioned in the above link.
> > >
>
> Indeed I used std::pair to be able to declare it as constexpr.
>
> I'll make it a map if that's a not a requirement. I'll see if it's
> possbile to statically initialize it as I'm currently doing with the
> pair
>
> >
> > Thanks. I don't remember the description, although I think I read it previously.
> > That sounds good to me. Perhaps, the map would be put as a const
> > static variable within CameraSensorInfo::get().
> >
>
> The map needs to be statically initialized, it could be done inside
> the ::get() function most probably. Something like (not tested)
>
> const SensorInfo *SensorDatabase::get(const std::string &sensor)
> {
>         static const std::map<const char *const, const SensorInfo *> sensorDb = {
>                 SENSOR_INFO(imx219),
>                 SENSOR_INFO(ov5670),
>                 SENSOR_INFO(ov13858),
>         };
>
>         const auto it = sensorDb.find(sensor.c_str());
>         if (it == sensorDb.end())
>                 return nullptr;
>
>         return it->second;
> }
>
> Thanks
>    j


Yep, this code looks good to me. (and should work to my best of knowledge)
-Hiro
>
>
>
> > > > > > > namespace {
> > > > > > >
> > > > > > > const std::map<std::string, SensorInfo> sensorDatabase{
> > > > > > >         { "imx219", {
> > > > > > >                 .unitCellSize = { 1120, 1120 },
> > > > > > >         } },
> > > > > > >         ...
> > > > > > > };
> > > > > > >
> > > > > > > } /* namespace */
> > > > > > >
> > > > > > > and this function can become
> > > > > > >
> > > > > > > const CameraSensorInfo *CameraSensorInfo::get(const std::string &sensor)
> > > > > > > {
> > > > > > >         auto iter = sensorDatabase.find(sensor);
> > > > > > >         if (iter == sensorDatabase.end())
> > > > > > >                 return nullptr;
> > > > > > >
> > > > > > >         return *iter;
> > > > > > > }
> > > > > > >
> > > > > > > > > > +
> > > > > > > > > > +     return nullptr;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +} /* namespace libcamera */
> > > > > > > >
> > > > > > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart

Hi Laurent,

On Tue, Apr 13, 2021 at 10:56:40AM +0900, Hirokazu Honda wrote:
> On Tue, Apr 13, 2021 at 10:45 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Mon, Apr 12, 2021 at 10:07:17PM +0900, Hirokazu Honda wrote:
> > > On Mon, Apr 12, 2021 at 6:10 PM Niklas Söderlund wrote:
> > > > On 2021-04-12 09:57:00 +0200, Jacopo Mondi wrote:
> > > > > Introduce a 'database' of camera sensor information, which contains
> > > > > a the camera sensor properties which is not possible, or desirable,
> >
> > s/is not/are not/
> >
> > > > s/^a //
> > > >
> > > > > to retrieve at run-time.
> > > > >
> > > > > The camera sensor database is accessed through the static SensorDatabase
> > > > > class which provides a single method to obtain the sensor information.
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > >
> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > >
> > > > > ---
> > > > >  include/libcamera/internal/meson.build       |  1 +
> > > > >  include/libcamera/internal/sensor_database.h | 28 ++++++
> > > > >  src/libcamera/meson.build                    |  1 +
> > > > >  src/libcamera/sensor_database.cpp            | 97 ++++++++++++++++++++
> > > > >  4 files changed, 127 insertions(+)
> > > > >  create mode 100644 include/libcamera/internal/sensor_database.h
> > > > >  create mode 100644 src/libcamera/sensor_database.cpp
> > > > >
> > > > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > > > > index 1fe3918cfe93..4aaa76ac1db1 100644
> > > > > --- a/include/libcamera/internal/meson.build
> > > > > +++ b/include/libcamera/internal/meson.build
> > > > > @@ -38,6 +38,7 @@ libcamera_internal_headers = files([
> > > > >      'process.h',
> > > > >      'pub_key.h',
> > > > >      'semaphore.h',
> > > > > +    'sensor_database.h',
> > > > >      'sysfs.h',
> > > > >      'thread.h',
> > > > >      'timer.h',
> > > > > diff --git a/include/libcamera/internal/sensor_database.h b/include/libcamera/internal/sensor_database.h
> > > > > new file mode 100644
> > > > > index 000000000000..7d743e46102d
> > > > > --- /dev/null
> > > > > +++ b/include/libcamera/internal/sensor_database.h
> > > > > @@ -0,0 +1,28 @@
> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2021, Google Inc.
> > > > > + *
> > > > > + * sensor_database.h - Database of camera sensor properties
> > > > > + */
> > > > > +#ifndef __LIBCAMERA_SENSOR_DATABASE_H__
> > > > > +#define __LIBCAMERA_SENSOR_DATABASE_H__
> > > > > +
> > > > > +#include <string>
> > > > > +
> > > > > +#include <libcamera/geometry.h>
> > > > > +
> > > > > +namespace libcamera {
> > > > > +
> > > > > +struct SensorInfo {
> >
> > I'd name this CameraSensorInfo, as we may have to support other types of
> > sensor in the future.
> >

We already have CameraSensorInfo defined in camera_sensor.h

I agree SensorInfo is not the best name and does not convey the fact
those are static properties. What about reusing the 'properties' term
with CameraSensorProperties ?

> > > > > +     Size unitCellSize;
> > > > > +};
> > > > > +
> > > > > +class SensorDatabase
> > > > > +{
> > > > > +public:
> > > > > +     static const SensorInfo *get(const std::string &sensor);
> > > > > +};
> > > > > +
> > >
> > > Just FYI: https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions
> > > > 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.
> > >
> > > However, I am not against this code. I am personally fine with class +
> > > static member functions.
> >
> > Moving the static function to the SensorInfo class would solve this
> > problem.
> >
> > > > > +} /* namespace libcamera */
> > > > > +
> > > > > +#endif /* __LIBCAMERA_SENSOR_DATABASE_H__ */
> > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > > > index e0a48aa23ea5..effae0ef5186 100644
> > > > > --- a/src/libcamera/meson.build
> > > > > +++ b/src/libcamera/meson.build
> > > > > @@ -44,6 +44,7 @@ libcamera_sources = files([
> > > > >      'pub_key.cpp',
> > > > >      'request.cpp',
> > > > >      'semaphore.cpp',
> > > > > +    'sensor_database.cpp',
> > > > >      'signal.cpp',
> > > > >      'stream.cpp',
> > > > >      'sysfs.cpp',
> > > > > diff --git a/src/libcamera/sensor_database.cpp b/src/libcamera/sensor_database.cpp
> > > > > new file mode 100644
> > > > > index 000000000000..68e69e8bf1b6
> > > > > --- /dev/null
> > > > > +++ b/src/libcamera/sensor_database.cpp
> > > > > @@ -0,0 +1,97 @@
> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2021, Google Inc.
> > > > > + *
> > > > > + * sensor_database.cpp - Database of camera sensor properties
> > > > > + */
> > > > > +
> > > > > +#include "libcamera/internal/sensor_database.h"
> > > > > +
> > > > > +#include <algorithm>
> > > > > +
> > > > > +namespace libcamera {
> >
> > The namespace goes after \file.
> >
> > > > > +
> > > > > +/**
> > > > > + * \file sensor_database.h
> > > > > + * \brief Database of camera sensor properties
> > > > > + *
> > > > > + * The camera sensor database collects information on camera sensors
> >
> > s/information on/static information about/
> >
> > > > > + * which is not possible or desirable to retrieve at run-time.
> >
> > s/which/that/
> > s/run-time/run time/
> >
> > > > > + */
> > > > > +
> > > > > +/**
> > > > > + * \class SensorDatabase
> > > > > + * \brief Access the database of camera sensor properties
> > > > > + *
> > > > > + * The database is indexed using the camera sensor model, as reported by the
> > > > > + * properties::Model property, and for each supported sensor it contains a
> > > > > + * SensorInfo list of properties.
> > > > > + *
> > > > > + * The class provides a single static getInfo() method to access the sensor
> >
> > The function is called get(). But in any case, let's move it to the
> > SensorInfo class.
> >
> > > > > + * database entries. If the sensor is not supported in the database nullptr is
> > > > > + * returned.
> > > > > + */
> > > > > +
> > > > > +/**
> > > > > + * \struct SensorInfo
> > > > > + * \brief List of sensor properties
> > > > > + *
> > > > > + * \var SensorInfo::unitCellSize
> > > > > + * \brief The physical size of pixel, including pixel edges. Width x height in
> >
> > s/pixel/a pixel/
> >
> > > > > + * nano-meters.
> >
> > s/nano-meters/nanometers/
> >
> > This field is a Size, so no need to repeat Width x height.
> >
> >  * \brief The physical size of a pixel, including pixel edges, in nanometers
> >
> > > > > + */
> > > > > +
> > > > > +namespace {
> > > > > +
> > > > > +/**
> > > > > + * \brief Sony IMX219 sensor properties
> > > > > + */
> > > > > +constexpr SensorInfo imx219Info = {
> > > > > +     .unitCellSize = { 1120, 1120 }
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * \brief Omnivision ov5670 sensor properties
> > > > > + */
> > > > > +constexpr SensorInfo ov5670Info = {
> > > > > +     .unitCellSize = { 1120, 1120 }
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * \brief Omnivision 13858 sensor properties
> > > > > + */
> > > > > +constexpr SensorInfo ov13858Info = {
> > > > > +     .unitCellSize = { 1120, 1120 }
> > > > > +};
> > > > > +
> > > > > +#define SENSOR_INFO(_sensor) \
> > > > > +     { #_sensor, &_sensor##Info }
> > > > > +
> > > > > +/*
> > > > > + * \brief The database of sensor properties
> > > > > + */
> > > > > +constexpr std::pair<const char *const, const SensorInfo *> sensorDatabase__[] = {
> > > > > +     SENSOR_INFO(imx219),
> > > > > +     SENSOR_INFO(ov5670),
> > > > > +     SENSOR_INFO(ov13858),
> > > > > +};
> > > > > +
> > > > > +} /* namespace */
> > > > > +
> > > > > +/**
> > > > > + * \brief Retrieve the properties associated with a sensor
> > > > > + * \param sensor The sensor model name
> > > > > + * \return A pointer to the SensorInfo instance associated with a sensor or
> > > > > + * nullptr if the sensor is not supported in the database
> > > > > + */
> > > > > +const SensorInfo *SensorDatabase::get(const std::string &sensor)
> > > > > +{
> > > > > +     for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {
> > > > > +             if (sensorDatabase__[i].first == sensor)
> > > > > +                     return sensorDatabase__[i].second;
> > > > > +     }
> >
> > Why not std::map ?
> >
>
> I expect the reason is std::map cannot be constructed as constexpr.
>
> > namespace {
> >
> > const std::map<std::string, SensorInfo> sensorDatabase{
> >         { "imx219", {
> >                 .unitCellSize = { 1120, 1120 },
> >         } },
> >         ...
> > };
> >
> > } /* namespace */
> >
> > and this function can become
> >
> > const CameraSensorInfo *CameraSensorInfo::get(const std::string &sensor)
> > {
> >         auto iter = sensorDatabase.find(sensor);
> >         if (iter == sensorDatabase.end())
> >                 return nullptr;
> >
> >         return *iter;
> > }
> >
> > > > > +
> > > > > +     return nullptr;
> > > > > +}
> > > > > +
> > > > > +} /* namespace libcamera */
> > >
> > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> >
> > --
> > Regards,
> >
> > Laurent Pinchart

Hi Jacopo,


On Tue, Apr 13, 2021 at 11:33:01AM +0200, Jacopo Mondi wrote:
> On Tue, Apr 13, 2021 at 10:56:40AM +0900, Hirokazu Honda wrote:
> > On Tue, Apr 13, 2021 at 10:45 AM Laurent Pinchart wrote:
> > > On Mon, Apr 12, 2021 at 10:07:17PM +0900, Hirokazu Honda wrote:
> > > > On Mon, Apr 12, 2021 at 6:10 PM Niklas Söderlund wrote:
> > > > > On 2021-04-12 09:57:00 +0200, Jacopo Mondi wrote:
> > > > > > Introduce a 'database' of camera sensor information, which contains
> > > > > > a the camera sensor properties which is not possible, or desirable,
> > >
> > > s/is not/are not/
> > >
> > > > > s/^a //
> > > > >
> > > > > > to retrieve at run-time.
> > > > > >
> > > > > > The camera sensor database is accessed through the static SensorDatabase
> > > > > > class which provides a single method to obtain the sensor information.
> > > > > >
> > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > >
> > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > > >
> > > > > > ---
> > > > > >  include/libcamera/internal/meson.build       |  1 +
> > > > > >  include/libcamera/internal/sensor_database.h | 28 ++++++
> > > > > >  src/libcamera/meson.build                    |  1 +
> > > > > >  src/libcamera/sensor_database.cpp            | 97 ++++++++++++++++++++
> > > > > >  4 files changed, 127 insertions(+)
> > > > > >  create mode 100644 include/libcamera/internal/sensor_database.h
> > > > > >  create mode 100644 src/libcamera/sensor_database.cpp
> > > > > >
> > > > > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > > > > > index 1fe3918cfe93..4aaa76ac1db1 100644
> > > > > > --- a/include/libcamera/internal/meson.build
> > > > > > +++ b/include/libcamera/internal/meson.build
> > > > > > @@ -38,6 +38,7 @@ libcamera_internal_headers = files([
> > > > > >      'process.h',
> > > > > >      'pub_key.h',
> > > > > >      'semaphore.h',
> > > > > > +    'sensor_database.h',
> > > > > >      'sysfs.h',
> > > > > >      'thread.h',
> > > > > >      'timer.h',
> > > > > > diff --git a/include/libcamera/internal/sensor_database.h b/include/libcamera/internal/sensor_database.h
> > > > > > new file mode 100644
> > > > > > index 000000000000..7d743e46102d
> > > > > > --- /dev/null
> > > > > > +++ b/include/libcamera/internal/sensor_database.h
> > > > > > @@ -0,0 +1,28 @@
> > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > > +/*
> > > > > > + * Copyright (C) 2021, Google Inc.
> > > > > > + *
> > > > > > + * sensor_database.h - Database of camera sensor properties
> > > > > > + */
> > > > > > +#ifndef __LIBCAMERA_SENSOR_DATABASE_H__
> > > > > > +#define __LIBCAMERA_SENSOR_DATABASE_H__
> > > > > > +
> > > > > > +#include <string>
> > > > > > +
> > > > > > +#include <libcamera/geometry.h>
> > > > > > +
> > > > > > +namespace libcamera {
> > > > > > +
> > > > > > +struct SensorInfo {
> > >
> > > I'd name this CameraSensorInfo, as we may have to support other types of
> > > sensor in the future.
> > >
> 
> We already have CameraSensorInfo defined in camera_sensor.h
> 
> I agree SensorInfo is not the best name and does not convey the fact
> those are static properties. What about reusing the 'properties' term
> with CameraSensorProperties ?

Or we could vary languages, with KameraAnturiTieto :-)
CameraSensorProperties sounds good to me.

> > > > > > +     Size unitCellSize;
> > > > > > +};
> > > > > > +
> > > > > > +class SensorDatabase
> > > > > > +{
> > > > > > +public:
> > > > > > +     static const SensorInfo *get(const std::string &sensor);
> > > > > > +};
> > > > > > +
> > > >
> > > > Just FYI: https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions
> > > > > 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.
> > > >
> > > > However, I am not against this code. I am personally fine with class +
> > > > static member functions.
> > >
> > > Moving the static function to the SensorInfo class would solve this
> > > problem.
> > >
> > > > > > +} /* namespace libcamera */
> > > > > > +
> > > > > > +#endif /* __LIBCAMERA_SENSOR_DATABASE_H__ */
> > > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > > > > index e0a48aa23ea5..effae0ef5186 100644
> > > > > > --- a/src/libcamera/meson.build
> > > > > > +++ b/src/libcamera/meson.build
> > > > > > @@ -44,6 +44,7 @@ libcamera_sources = files([
> > > > > >      'pub_key.cpp',
> > > > > >      'request.cpp',
> > > > > >      'semaphore.cpp',
> > > > > > +    'sensor_database.cpp',
> > > > > >      'signal.cpp',
> > > > > >      'stream.cpp',
> > > > > >      'sysfs.cpp',
> > > > > > diff --git a/src/libcamera/sensor_database.cpp b/src/libcamera/sensor_database.cpp
> > > > > > new file mode 100644
> > > > > > index 000000000000..68e69e8bf1b6
> > > > > > --- /dev/null
> > > > > > +++ b/src/libcamera/sensor_database.cpp
> > > > > > @@ -0,0 +1,97 @@
> > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > > +/*
> > > > > > + * Copyright (C) 2021, Google Inc.
> > > > > > + *
> > > > > > + * sensor_database.cpp - Database of camera sensor properties
> > > > > > + */
> > > > > > +
> > > > > > +#include "libcamera/internal/sensor_database.h"
> > > > > > +
> > > > > > +#include <algorithm>
> > > > > > +
> > > > > > +namespace libcamera {
> > >
> > > The namespace goes after \file.
> > >
> > > > > > +
> > > > > > +/**
> > > > > > + * \file sensor_database.h
> > > > > > + * \brief Database of camera sensor properties
> > > > > > + *
> > > > > > + * The camera sensor database collects information on camera sensors
> > >
> > > s/information on/static information about/
> > >
> > > > > > + * which is not possible or desirable to retrieve at run-time.
> > >
> > > s/which/that/
> > > s/run-time/run time/
> > >
> > > > > > + */
> > > > > > +
> > > > > > +/**
> > > > > > + * \class SensorDatabase
> > > > > > + * \brief Access the database of camera sensor properties
> > > > > > + *
> > > > > > + * The database is indexed using the camera sensor model, as reported by the
> > > > > > + * properties::Model property, and for each supported sensor it contains a
> > > > > > + * SensorInfo list of properties.
> > > > > > + *
> > > > > > + * The class provides a single static getInfo() method to access the sensor
> > >
> > > The function is called get(). But in any case, let's move it to the
> > > SensorInfo class.
> > >
> > > > > > + * database entries. If the sensor is not supported in the database nullptr is
> > > > > > + * returned.
> > > > > > + */
> > > > > > +
> > > > > > +/**
> > > > > > + * \struct SensorInfo
> > > > > > + * \brief List of sensor properties
> > > > > > + *
> > > > > > + * \var SensorInfo::unitCellSize
> > > > > > + * \brief The physical size of pixel, including pixel edges. Width x height in
> > >
> > > s/pixel/a pixel/
> > >
> > > > > > + * nano-meters.
> > >
> > > s/nano-meters/nanometers/
> > >
> > > This field is a Size, so no need to repeat Width x height.
> > >
> > >  * \brief The physical size of a pixel, including pixel edges, in nanometers
> > >
> > > > > > + */
> > > > > > +
> > > > > > +namespace {
> > > > > > +
> > > > > > +/**
> > > > > > + * \brief Sony IMX219 sensor properties
> > > > > > + */
> > > > > > +constexpr SensorInfo imx219Info = {
> > > > > > +     .unitCellSize = { 1120, 1120 }
> > > > > > +};
> > > > > > +
> > > > > > +/**
> > > > > > + * \brief Omnivision ov5670 sensor properties
> > > > > > + */
> > > > > > +constexpr SensorInfo ov5670Info = {
> > > > > > +     .unitCellSize = { 1120, 1120 }
> > > > > > +};
> > > > > > +
> > > > > > +/**
> > > > > > + * \brief Omnivision 13858 sensor properties
> > > > > > + */
> > > > > > +constexpr SensorInfo ov13858Info = {
> > > > > > +     .unitCellSize = { 1120, 1120 }
> > > > > > +};
> > > > > > +
> > > > > > +#define SENSOR_INFO(_sensor) \
> > > > > > +     { #_sensor, &_sensor##Info }
> > > > > > +
> > > > > > +/*
> > > > > > + * \brief The database of sensor properties
> > > > > > + */
> > > > > > +constexpr std::pair<const char *const, const SensorInfo *> sensorDatabase__[] = {
> > > > > > +     SENSOR_INFO(imx219),
> > > > > > +     SENSOR_INFO(ov5670),
> > > > > > +     SENSOR_INFO(ov13858),
> > > > > > +};
> > > > > > +
> > > > > > +} /* namespace */
> > > > > > +
> > > > > > +/**
> > > > > > + * \brief Retrieve the properties associated with a sensor
> > > > > > + * \param sensor The sensor model name
> > > > > > + * \return A pointer to the SensorInfo instance associated with a sensor or
> > > > > > + * nullptr if the sensor is not supported in the database
> > > > > > + */
> > > > > > +const SensorInfo *SensorDatabase::get(const std::string &sensor)
> > > > > > +{
> > > > > > +     for (unsigned int i = 0; i < std::size(sensorDatabase__); ++i) {
> > > > > > +             if (sensorDatabase__[i].first == sensor)
> > > > > > +                     return sensorDatabase__[i].second;
> > > > > > +     }
> > >
> > > Why not std::map ?
> >
> > I expect the reason is std::map cannot be constructed as constexpr.
> >
> > > namespace {
> > >
> > > const std::map<std::string, SensorInfo> sensorDatabase{
> > >         { "imx219", {
> > >                 .unitCellSize = { 1120, 1120 },
> > >         } },
> > >         ...
> > > };
> > >
> > > } /* namespace */
> > >
> > > and this function can become
> > >
> > > const CameraSensorInfo *CameraSensorInfo::get(const std::string &sensor)
> > > {
> > >         auto iter = sensorDatabase.find(sensor);
> > >         if (iter == sensorDatabase.end())
> > >                 return nullptr;
> > >
> > >         return *iter;
> > > }
> > >
> > > > > > +
> > > > > > +     return nullptr;
> > > > > > +}
> > > > > > +
> > > > > > +} /* namespace libcamera */
> > > >
> > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>